Skip to content
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

Update getHeader and submitBlindedBlock to support blobs (EIP-4844) #61

Merged
merged 18 commits into from
May 24, 2023

Conversation

jimmygchen
Copy link
Contributor

@jimmygchen jimmygchen commented Jan 3, 2023

Builder spec changes for EIP-4844, separated from PR #58.

Description

getHeader changes

  • Updated ExecutionPayload & BeaconBlockBody types to align with EIP-4844 specs. (Depends on types from this Beacon API PR: Update types to support EIP-4844 beacon-APIs#271)
  • Add blinded_blobs to getHeader response (SignedBuilderBid), so that validators can create signatures over the blinded block and blinded blobs.

submitBlindedBlock changes

  • Endpoint updated to accept SignedBlindedBlockContents after Deneb.
  • Additionally returns unblinded blobs after Deneb; new type: ExecutionPayloadAndBlobsBundle.
  • I've modified the response to return blobs_bundle instead of blob_sidecars, as I think it's more consistent with what we get from the EL, and BN should have enough to build the blob sidecars.

I'm currently pointing the beacon-APIs submodule to this PR, as it contains the new SignedBlindedBlockContents type: ethereum/beacon-APIs#302

Moved design diagrams to this HackMD page as it was getting outdated here.

@jimmygchen
Copy link
Contributor Author

jimmygchen commented Jan 6, 2023

@ralexstokes @0xGabi would you mind reviewing this please? I've addressed all comments from the previous PR. This PR includes changes for EIP-4844 only.

capella:
$ref: "../../builder-oapi.yaml#/components/examples/Capella.ExecutionPayload"
eip4844:
$ref: "../../builder-oapi.yaml#/components/examples/EIP4844.ExecutionPayloadAndBlobsSidecar"
Copy link
Contributor

@g11tech g11tech Jan 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jimmygchen: Can we shift this to a new version: submitBlindedBlockV2, it would help so much with the code flow and the type handling and hence cleaner code as this literally changes the structure of the underlying response

(I know there was a submitBlindedBlockV2 in the parent PR of this PR which returned full signedbeaconblock and was cleared out)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@g11tech there's been a bit of back and forth on this one in the previous PR and discord as well - originally I had similar thoughts about adding a new version, but I also agree that it's not necessary to add a new version to support new types if the fork information is provided in the request.

Perhaps we could include the "consensus version" in the request headers (similar to the Beacon APIs), so it would be easier for the builder to support this? There's a PR in mev-boost to fallback to Bellatrix if it's not able to decode the payload, but I think this will get more complicated once the 4844 fork / payload is introduced.

To your point on code flow on the consensus side, I've tried implementing both in Lighthouse and haven't experienced too many issues with this (yet), but I don't have a strong opinion either way. The builder would need to know what version the payload is regardless of whether we do V1 or V2.

I think I'd go with the conventions (using version version without adding new api version) unless there's any strong reasons for adding v2? perhaps @ralexstokes can share some thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as @ralexstokes pointed out, beacon apis insist on api version bump on format changes, i recon the response format has changed from direct execution payload to {execution_payload,...} object.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@g11tech not needing a new API versions for these changes was already discussed previously, i would keep it as is for now, and if you feel strongly, please initiate a new discussion about this in an issue.

@jimmygchen jimmygchen changed the title Add and update types for EIP-4844 Add and update types for EIP-4844/Deneb Jan 19, 2023
@jimmygchen
Copy link
Contributor Author

I've replaced all EIP-4844 references with Deneb.
The PR for the rename in Beacon API is here: ethereum/beacon-APIs#289

@jimmygchen
Copy link
Contributor Author

Waiting for the ethereum/beacon-APIs#289 to be merged to pass the build

Copy link
Contributor

@terencechain terencechain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it stands today, blobs will be decoupled which changes how validator interacts with relayers (https://hackmd.io/cmYisgxkRuGe9NjX4gr97A) I'd recommend wait on it for a little bit until the spec matures

@jimmygchen
Copy link
Contributor Author

There is currently a discussion in the below PR regarding blob signing and publishing, and could have an impact on the builder API, I'll come back to this PR once we get more input on the decisison:

ethereum/beacon-APIs#302 (comment)

@jimmygchen jimmygchen changed the title Add and update types for EIP-4844/Deneb Update getHeader and submitBlindedBlock to support blobs (EIP-4844) May 12, 2023
@jimmygchen
Copy link
Contributor Author

@metachris @terencechain @g11tech @ralexstokes
I've updated this PR to reflect some recent changes - there are quite a few changes to the types, so I've re-written the PR description. Please see above.

Thanks!

sync_aggregate: SyncAggregate
execution_payload_header: ExecutionPayloadHeader # [Modified in Deneb]
bls_to_execution_changes: List[SignedBLSToExecutionChange, MAX_BLS_TO_EXECUTION_CHANGES]
blob_kzg_commitments: List[KZGCommitment, MAX_BLOBS_PER_BLOCK] # [New in Deneb]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May have to update MAX_BLOBS_PER_BLOCK to MAX_BLOB_COMMITMENTS_PER_BLOCK.
https://github.com/ethereum/consensus-specs/pull/3338/files

@metachris
Copy link
Contributor

Thanks for your efforts @jimmygchen, much appreciated 🙏

We've discussed this PR with the mev-boost maintainers group, and believe a single submitBlindedBlock request would be preferable to requiring multiple API calls.

  • Since all requests would be required to succeed anyway, it adds several downsides to split up the functionality over multiple API requests.
  • Not least that the relay is responsible for broadcasting the block before it is allowed to send the payload back to the user. Having multiple API calls will require the relay to accumulate state internally, but at the same time block the response of other API calls until all are received, and the final block is validated, equivocation-checked and broadcast, and then there's still the remaining 1s delay before responding.

Also it may be a preferable approach to not return the payload to the proposer, but have it be the relay responsibility to broadcast the block. Returning the payload is currently delayed by relays, and is problematic due to equivocation attacks, and there has been some discussion recently by core devs about it perhaps being safer to not return the payload on this API.

cc/ @jtraglia @terencechain @ralexstokes @michaelneuder

@jimmygchen
Copy link
Contributor Author

@metachris the PR is updated to send a single submitBlindedBlock, but diagram was outdated. It's now updated, thanks for pointing this out!

Also it may be a preferable approach to not return the payload to the proposer, but have it be the relay responsibility to broadcast the block. Returning the payload is currently delayed by relays, and is problematic due to equivocation attacks, and there has been some discussion recently by core devs about it perhaps being safer to not return the payload on this API.

Thanks for sharing this - I'll keep an eye on the development!

@@ -0,0 +1,41 @@
{
"value": {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't actually have this top level value field right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I think this is the format openapi lint expects when validating the example files - we have them in all other example jsons, and is required for lint to work.

Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks amazing overall 👍 , few suggestions

Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excellent work and I think this reflects the latest direction around 4844 well

I'd say we move ahead with merging this so we can start prototyping at various places in the mev-boost ecosystem/stack

specs/deneb/builder.md Outdated Show resolved Hide resolved
@jimmygchen
Copy link
Contributor Author

jimmygchen commented May 18, 2023

excellent work and I think this reflects the latest direction around 4844 well

I'd say we move ahead with merging this so we can start prototyping at various places in the mev-boost ecosystem/stack

Thanks @ralexstokes ! I've updated the beacon-API submodule to latest (which contains the deneb types we need in this PR), so I think this should be ready to go from my POV.

There's a pending CL spec PR to update MAX_BLOBS_PER_BLOCK to MAX_BLOB_COMMITMENTS_PER_BLOCK in deneb BeaconBlockBody - I'm happy to update this when the other PR is finalised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants