-
Notifications
You must be signed in to change notification settings - Fork 291
fix fulu builder api changes #7340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…pose on behalf of BN
…pose on behalf of BN
@@ -233,11 +233,11 @@ func asConsensusTypeFulu*( | |||
# The `mapIt` calls below are necessary only because we use different distinct | |||
# types for KZG commitments and Blobs in the `web3` and the `deneb` spec types. | |||
# Both are defined as `array[N, byte]` under the hood. | |||
blobsBundle: deneb.BlobsBundle( | |||
blobsBundle: fulu.BlobsBundleV2( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't accurate; GetPayloadV4
uses BlobsBundleV1
: https://github.com/ethereum/execution-apis/blob/v1.0.0-beta.4/src/engine/prague.md#engine_getpayloadv4
@@ -570,7 +570,7 @@ proc makeBeaconBlockWithRewards*( | |||
hash_tree_root(validator_changes.proposer_slashings), | |||
hash_tree_root(validator_changes.electra_attester_slashings), | |||
hash_tree_root( | |||
List[electra.Attestation, Limit MAX_ATTESTATIONS]( | |||
List[electra.Attestation, Limit MAX_ATTESTATIONS_ELECTRA]( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the actual fix.
@@ -550,6 +550,7 @@ type | |||
GetHeaderResponseElectra* = DataVersionEnclosedObject[electra_mev.SignedBuilderBid] | |||
GetHeaderResponseFulu* = DataVersionEnclosedObject[fulu_mev.SignedBuilderBid] | |||
SubmitBlindedBlockResponseElectra* = DataVersionEnclosedObject[electra_mev.ExecutionPayloadAndBlobsBundle] | |||
SubmitBlindedBlockResponseFulu* = DataVersionEnclosedObject[fulu_mev.ExecutionPayloadAndBlobsBundle] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per ethereum/builder-specs#123 the v2
API which Nimbus now uses for Fulu instead of the deprecated v1
API "does not return the execution payload and blobs".
There's no decoding to do, unlike Pectra. So this SubmitBlindedBlockResponseFulu
type is neither necessary nor useful.
@@ -270,6 +270,7 @@ RestJson.useDefaultSerializationFor( | |||
fulu_mev.BlindedBeaconBlock, | |||
fulu_mev.BlindedBeaconBlockBody, | |||
fulu_mev.BuilderBid, | |||
fulu_mev.ExecutionPayloadAndBlobsBundle, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because SubmitBlindedBlockResponseFulu
isn't used, neither is this.
commitments: KzgCommitments.init( | ||
payload.blobsBundle.commitments.mapIt( | ||
kzg_abi.KzgCommitment(bytes: it.data))), | ||
proofs: KzgProofs.init( | ||
proofs: KzgProofsV2.init( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also not accurate.
@@ -1042,7 +1042,8 @@ proc installBeaconApiHandlers*(router: var RestRouter, node: BeaconNode) = | |||
doAssert strictVerification notin node.dag.updateFlags | |||
return RestApiResponse.jsonError(Http400, InvalidBlockObjectError) | |||
|
|||
when consensusFork >= ConsensusFork.Deneb: | |||
when consensusFork >= ConsensusFork.Deneb and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two points:
-
https://ethereum.github.io/beacon-APIs/#/Beacon/publishBlock shows " Warning: Deprecated". That is, I'll remove (
Http410
) this entire handler as soon as feasible come Fulu. It doesn't matter if it works under Fulu; it will never have to. -
Even otherwise, this endpoint isn't about blinded blocks, but non-MEV blocks, and those should be sending their blob/column sidecars somewhere.
But the first point is more important. There's no reason to change this to work in Fulu, nor should it be changed.
@@ -489,7 +489,8 @@ template kind*( | |||
fulu.MsgTrustedSignedBeaconBlock | | |||
fulu.TrustedSignedBeaconBlock | | |||
fulu_mev.SignedBlindedBeaconBlock | | |||
fulu_mev.SignedBuilderBid]): ConsensusFork = | |||
fulu_mev.SignedBuilderBid | | |||
fulu_mev.ExecutionPayloadAndBlobsBundle]): ConsensusFork = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be used either.
I removed all this, for good reason, back in #7308
There's still no actual block/blob return from submitBlindedBlock
in Fulu, so all of this is pointless. The stateroot bug was related, as far as it seems so far, in the construction of the shim block in state_transition
, and that fix is elsewhere. There's still nothing to decode from the relay for Fulu, aside from checking the 202-or-not-204 HTTP return.
@@ -69,11 +70,17 @@ type | |||
message*: BlindedBeaconBlock | |||
signature*: ValidatorSig | |||
|
|||
# https://github.com/ethereum/builder-specs/blob/ae1d97d080a12bfb7ca248b58fb1fc6b10aed02e/specs/fulu/builder.md#executionpayloadandblobsbundle | |||
ExecutionPayloadAndBlobsBundle* = object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See elsewhere, not useful/necessary.
doAssert engineBid.blobsBundle.commitments == | ||
forkyBlck.body.blob_kzg_commitments | ||
ResultType.ok(( | ||
blck: consensusFork.MaybeBlindedBeaconBlock( | ||
isBlinded: false, | ||
data: consensusFork.BlockContents( | ||
`block`: forkyBlck, | ||
kzg_proofs: engineBid.blobsBundle.proofs, | ||
kzg_proofs: KzgProofs(engineBid.blobsBundle.proofs), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/github-runner/github-runner-node-01/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/validators/beacon_validators.nim(2076, 32) Hint: conversion from KzgProofs to itself is pointless [ConvFromXtoItselfNotNeeded]
isBlinded: false, | ||
data: consensusFork.BlockContents( | ||
`block`: forkyBlck, | ||
kzg_proofs: KzgProofsV2(engineBid.blobsBundleV2.proofs), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/github-runner/github-runner-node-01/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/validators/beacon_validators.nim(2064, 34) Hint: conversion from KzgProofsV2 to itself is pointless [ConvFromXtoItselfNotNeeded]
res.get() | ||
signedBlock = consensusFork.SignedBeaconBlock( | ||
message: forkyBlck, signature: signature, root: blockRoot) | ||
blobsOpt = | ||
when consensusFork >= ConsensusFork.Deneb: | ||
Opt.some(signedBlock.create_blob_sidecars( | ||
engineBid.blobsBundle.proofs, engineBid.blobsBundle.blobs)) | ||
KzgProofs(engineBid.blobsBundle.proofs), engineBid.blobsBundle.blobs)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/github-runner/github-runner-node-01/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/validators/beacon_validators.nim(1249, 22) Hint: conversion from KzgProofs to itself is pointless [ConvFromXtoItselfNotNeeded]
Overtaken by events. |
No description provided.