Skip to content

Builder spec changes for Capella & EIP-4844#58

Closed
jimmygchen wants to merge 30 commits intoethereum:mainfrom
jimmygchen:main
Closed

Builder spec changes for Capella & EIP-4844#58
jimmygchen wants to merge 30 commits intoethereum:mainfrom
jimmygchen:main

Conversation

@jimmygchen
Copy link
Contributor

@jimmygchen jimmygchen commented Dec 1, 2022

Description

  • 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 blob_kzg_commitments to getExecutionPayloadHeader response (SignedBuilderBid), so that validator can include it in the block and create a signature over the blinded block.
  • Update submitBlindedBlock endpoint for 4844, that includes blobs_sidecar in the response (ExecutionPayloadAndBlobsSidecar).

Changes for EIP-4844

sequenceDiagram
    participant validator
    participant consensus
    participant mev_boost
    Title: Block Proposal with Builder API (EIP-4844)
    validator->>consensus: GET v2/validator/blinded_blocks/{slot}
    consensus->>mev_boost: GET v1/builder/header/{slot}/{parent_hash}/{pubkey}
    mev_boost-->>consensus: GET v1/builder/header/{slot}/{parent_hash}/{pubkey} response
    Note left of mev_boost: add `blob_kzg_commitments` to response (`SignedBuilderBid`)
    consensus-->>validator: GET v2/validator/blinded_blocks/{slot} response
    Note left of consensus: add `blob_kzg_commitments` to response
    Note over validator: include `blob_kzg_commitments` in block and sign the header
    validator->>consensus: POST beacon/blinded_blocks
    consensus->>mev_boost: POST /eth/v2/builder/blinded_blocks
    mev_boost-->>consensus: POST /eth/v2/builder/blinded_blocks response
    Note left of mev_boost: revealed payload will have to include `blobs_sidecar` and kzg proof
    Note over consensus: construct block and broadcast block
Loading

Copy link

@0xGabi 0xGabi left a comment

Choose a reason for hiding this comment

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

Great work on adding the blobs_sidecar, I believe we are getting close to mark the PR ready for review.

I had some general thoughts about writing the spec:

  • While looking through the execution-apis spec I start wondering if we want to further clarify the changes between submitBlindedBlockV1 and submitBlindedBlockV2.

  • If we follow the same versioning criteria of the execution API. We may need to version the structures (i.e. SignedBuilderBidV2) and as a consequence have a getHeaderV2. I'm not sure we want to follow that path but wanted to bring it for discussion.

@jimmygchen
Copy link
Contributor Author

  • While looking through the execution-apis spec I start wondering if we want to further clarify the changes between submitBlindedBlockV1 and submitBlindedBlockV2.
  • If we follow the same versioning criteria of the execution API. We may need to version the structures (i.e. SignedBuilderBidV2) and as a consequence have a getHeaderV2. I'm not sure we want to follow that path but wanted to bring it for discussion.

Great point, I'm not sure tbh, and I'd like to hear some more discussions on this too. There were some discussions on discord earlier, and it looks like we don't need a new version just for new fields, however in this case we're changing the endpoint to return a SignedBeaconBlockAndSidecar instead of ExecutionPayload, so it's a breaking change.
https://discord.com/channels/595666850260713488/1031999860997619843/1038023245900820531

@@ -0,0 +1,58 @@
post:
Copy link
Contributor

@metachris metachris Dec 2, 2022

Choose a reason for hiding this comment

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

from the discussions i've had with EF/CL peers, there seems to be a preference for reusing the existing API call, rather than introducing a new V2 one. cc/ @jtraglia @lightclient @ralexstokes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey thanks for the insight @metachris. I've seen some discussion on discord earlier too. My understanding was that the version header is used for determining the payload structure, so clients should be able to parse the same payload type from different forks. It make sense to me to keep the same API if the response type is the same, i.e. if we return Eip4844.ExeuctionPayload.

However I think we'd also want to include blobs and the kzg proof, so it would be more convenient to change the response type to SignedBeaconBlockAndBlobsSidecar (with the revealed ExecutionPayload wrapped within the block), and the clients can just broadcast the block after receiving it, rather than having to retrieve the sidecar and construct the beacon block. Does this make sense? I haven't seen an API allowing a completely different response type, so I thought we may want to increment the version, but keen to hear your thoughts!

(cc: @ajsutton)

Copy link
Member

Choose a reason for hiding this comment

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

I haven't seen an API allowing a completely different response type

this should be fine -- we should be able to just add the right type to the v1 endpoint

let's drop v2 here and extend v1 to also support a new type that contains the ExecutionPayload and the BlobsSidecar, calling it something like ExecutionPayloadWithBlobsSidecar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

@metachris
Copy link
Contributor

Just wanted to say thanks for the contribution! 🚀
Would love to understand more about where this is coming from.

pinging @lightclient @jtraglia @lightclient @terencechain @tbenr @ajsutton (and others) for review and feedback

@jimmygchen jimmygchen marked this pull request as ready for review December 2, 2022 15:58
@jimmygchen jimmygchen changed the title [wip] Builder spec changes for Capella & EIP-4844 Builder spec changes for Capella & EIP-4844 Dec 2, 2022
@tbenr
Copy link

tbenr commented Dec 12, 2022

Regarding submitBlindedBlock, It makes sense to me to have a V2 returning the entireSignedBeaconBlockAndBlobsSidecar. It reduces complexity and the overhead is minimal since ExecutionPayload and BlobsSidecar are the biggest objects we need to get anyway.

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!

I left some questions and feedback -- this is a great start :)

This PR is fine as is but something to keep in mind for the future is that it is much easier to review smaller PRs, so this one could easily be a PR just for capella changes and then a separate one for 4844 changes

Comment on lines 9 to 14
value:
$ref: "../../beacon-apis/types/primitive.yaml#/Uint256"
description: "Payment in wei that will be paid to the `fee_recipient` account."
pubkey:
$ref: "../../beacon-apis/types/primitive.yaml#/Pubkey"
description: "BLS public key of builder."
Copy link
Member

Choose a reason for hiding this comment

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

I think we can define a BuilderBidCommon schema to reduce the repetition here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks

Comment on lines 21 to 23
"withdrawals": [
"0x02f878831469668303f51d843b9ac9f9843b9aca0082520894c93269b73096998db66be0441e836d873535cb9c8894a19041886f000080c001a031cc29234036afbf9a1fb9476b463367cb1f957ac0b919b69bbc798436e604aaa018c4e9c3914eb27aadd0b91e10b18655739fcf8c1fc398763a9f1beecb8ddc86"
]
Copy link
Member

Choose a reason for hiding this comment

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

ideally we have an actual hex-encoded withdrawal here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah nice catch, updated now - just realised withdrawals is a list of objects.. wonder why validation didn't pick up 🤔

"0x02f878831469668303f51d843b9ac9f9843b9aca0082520894c93269b73096998db66be0441e836d873535cb9c8894a19041886f000080c001a031cc29234036afbf9a1fb9476b463367cb1f957ac0b919b69bbc798436e604aaa018c4e9c3914eb27aadd0b91e10b18655739fcf8c1fc398763a9f1beecb8ddc86"
],
"withdrawals": [
"0x02f878831469668303f51d843b9ac9f9843b9aca0082520894c93269b73096998db66be0441e836d873535cb9c8894a19041886f000080c001a031cc29234036afbf9a1fb9476b463367cb1f957ac0b919b69bbc798436e604aaa018c4e9c3914eb27aadd0b91e10b18655739fcf8c1fc398763a9f1beecb8ddc86"
Copy link
Member

Choose a reason for hiding this comment

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

same on withdrawal example here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 2 to 38
SignedBeaconBlockAndBlobsSidecar:
type: object
description: "The `SignedBeaconBlockAndBlobsSidecar` object from the EIP-4844 CL spec."
properties:
beacon_block:
$ref: "../../beacon-apis/types/eip4844/block.yaml#/EIP4844/SignedBeaconBlock"
blobs_sidecar:
$ref: "#/EIP4844/BlobsSidecar"

BlobsSidecar:
type: object
description: "The `BlobsSidecar` object from the EIP-4844 CL spec."
properties:
beacon_block_root:
$ref: "../../beacon-apis/types/primitive.yaml#/Root"
beacon_block_slot:
$ref: "../../beacon-apis/types/primitive.yaml#/Uint64"
blobs:
type: array
maxItems: 16
minItems: 0
items:
$ref: "#/EIP4844/Blob"
kzg_aggregated_proof:
$ref: "#/EIP4844/KZGProof"

KZGProof:
type: string
format: hex
pattern: "^0x[a-fA-F0-9]{96}$"
description: "An aggregated KZG proof. Same check as `KZGCommitment`"

Blob:
type: string
format: hex
pattern: "^0x[a-fA-F0-9]{262144}$"
description: "A blob is `FIELD_ELEMENTS_PER_BLOB * size_of(BLSFieldElement) = 4096 * 32 = 131072` bytes (`DATA`) representing a SSZ-encoded Blob as defined in EIP-4844"
Copy link
Member

Choose a reason for hiding this comment

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

what do you think about moving SignedBeaconBlockAndBlobsSidecar to the beacon-apis and then just referencing them here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@0xGabi made a comment about this here as well - I wasn't sure about this as it's not used in beacon-apis (and may lead to validation warnings if there's one), but happy to move this since we've got more arguments for moving over to beacon-apis

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

example: "bellatrix"
data:
oneOf:
anyOf:
Copy link
Member

Choose a reason for hiding this comment

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

not sure on the semantics of oneOf vs anyOf but it sounds like we want oneOf here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should be oneOf, I had a bit of issues getting this to work earlier (noted in this comment) - perhaps we'll need to make all new properties in schema versions required in order for the examples pass validation::

I'm having some issues with lint here complaining about these examples matching multiple schemas, and not sure what's the best way to resolve this.
I think the json examples are matching multiple schemas because the schema allows for additional properties. I think we could set additionalProperties to false so that it doesn't match all, but I haven't seen this approach used anywhere else in the API repos.

~/builder-specs/examples/bellatrix/signed_builder_bid.json
 4:13  error  oas3-valid-media-example  "data" property must match exactly one schema in oneOf  value.data

~/builder-specs/examples/capella/signed_builder_bid.json
 4:13  error  oas3-valid-media-example  "data" property must match exactly one schema in oneOf  value.data

~/builder-specs/examples/eip4844/signed_builder_bid.json
 4:13  error  oas3-valid-media-example  "data" property must match exactly one schema in oneOf  value.data

I don't see any easy way around without adding required for new properties everywhere, which is a bit ugly, so I went with using anyOf instead - if anyone has a better solution for this please let me know!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to oneOf. Got this working by adding additioanlProperties: false to ExecutionPayloadHeader and BuilderBid types.

@@ -0,0 +1,58 @@
post:
Copy link
Member

Choose a reason for hiding this comment

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

I haven't seen an API allowing a completely different response type

this should be fine -- we should be able to just add the right type to the v1 endpoint

let's drop v2 here and extend v1 to also support a new type that contains the ExecutionPayload and the BlobsSidecar, calling it something like ExecutionPayloadWithBlobsSidecar

Copy link

@0xGabi 0xGabi left a comment

Choose a reason for hiding this comment

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

Well done. I like ExecutionPayloadAndBlobSidecar.

@jimmygchen
Copy link
Contributor Author

Thanks for the review @ralexstokes @0xGabi! 🙏 I've addressed your comments, please review again.

@jimmygchen jimmygchen requested review from 0xGabi and ralexstokes and removed request for 0xGabi and ralexstokes December 16, 2022 03:42
Copy link

@0xGabi 0xGabi left a comment

Choose a reason for hiding this comment

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

LGTM

],
"withdrawals": [
"0x02f878831469668303f51d843b9ac9f9843b9aca0082520894c93269b73096998db66be0441e836d873535cb9c8894a19041886f000080c001a031cc29234036afbf9a1fb9476b463367cb1f957ac0b919b69bbc798436e604aaa018c4e9c3914eb27aadd0b91e10b18655739fcf8c1fc398763a9f1beecb8ddc86"
{
Copy link

Choose a reason for hiding this comment

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

👍

@metachris
Copy link
Contributor

I think it would make sense to split this PR into two -- one for Capella changes, and another one for EIP4844 changes? (The 4844 PR could point to the Capella PR to reuse anything that's already in there)

"0x02f878831469668303f51d843b9ac9f9843b9aca0082520894c93269b73096998db66be0441e836d873535cb9c8894a19041886f000080c001a031cc29234036afbf9a1fb9476b463367cb1f957ac0b919b69bbc798436e604aaa018c4e9c3914eb27aadd0b91e10b18655739fcf8c1fc398763a9f1beecb8ddc86"
],
"withdrawals": [
"0x02f878831469668303f51d843b9ac9f9843b9aca0082520894c93269b73096998db66be0441e836d873535cb9c8894a19041886f000080c001a031cc29234036afbf9a1fb9476b463367cb1f957ac0b919b69bbc798436e604aaa018c4e9c3914eb27aadd0b91e10b18655739fcf8c1fc398763a9f1beecb8ddc86"
Copy link
Contributor

Choose a reason for hiding this comment

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

withdrawals json here looks incorrect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @g11tech that's fixed now!

@jimmygchen
Copy link
Contributor Author

jimmygchen commented Jan 3, 2023

I think it would make sense to split this PR into two -- one for Capella changes, and another one for EIP4844 changes? (The 4844 PR could point to the Capella PR to reuse anything that's already in there)

Hi @metachris @ralexstokes I've addressed all comments and created separated PRs below:

Closing this PR.

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