Update types to support EIP-4844

[jimmygchen]

Add types to support new fields in EIP-4844, main changes are:

• add blob_kzg_commitments to BeaconBlockBodyCommon
• add excess_data_gas to ExecutionPayloadCommon

Consensus spec:
https://github.com/ethereum/consensus-specs/blob/master/specs/eip4844/beacon-chain.md#beaconblockbody

[0xGabi]

Well done. I left a couple of comments. And I notice we were missing some eip4844 enum values so I create a PR against your branch with the addition: jimmygchen#1

One thing I was considering is if we want to add an event stream for blob_kzg_commitments: https://github.com/ethereum/beacon-APIs/blob/master/apis/eventstream/index.yaml#L18-L34

[0xGabi]

I missed this one on the first pass.

[jimmygchen]

Well done. I left a couple of comments. And I notice we were missing some eip4844 enum values so I create a PR against your branch with the addition: jimmygchen#1

One thing I was considering is if we want to add an event stream for blob_kzg_commitments: https://github.com/ethereum/beacon-APIs/blob/master/apis/eventstream/index.yaml#L18-L34

I'm not sure about the usage of having a blob_kzg_commitments event stream, what's in your mind?

[0xGabi]

I thought that it could be helpful to know when the node received a block with a blob transaction so the event listener can perform validations similar to verify_kzg_commitments_against_transactions.

[jimmygchen]

I thought that it could be helpful to know when the node received a block with a blob transaction so the event listener can perform validations similar to verify_kzg_commitments_against_transactions.

These are Server-Sent-Events from beacon node though, and are for consumers of the Beacon API. For the beacon node itself it should already be able to handle blob messages sent via gossip, which is specified as part of the consensus spec - if this is what you mean?

[0xGabi]

For the beacon node itself it should already be able to handle blob messages sent via gossip, which is specified as part of the consensus spec - if this is what you mean?

No, as we discussed async I was thinking about having a blob_kzg_commitments API endpoint so external components can work with the data (i.e. validations).

[rolfyone]

Currently all the SSE are JSON with a leading line to say what kind of data. I'm not sure how sending blobs would go, potentially in the first instance it's better to get the types in, and follow up the SSE in a separate issue?

If there's no great call for an SSE, potentially we could live without it in the first instance, unless there's an actual use for it we know about...

[jimmygchen]

Thanks @rolfyone - I agree, I'm not aware of any discussions on blobs over SSE before this and is probably not a prerequisite for 4844, and we can follow up in a separate issue if necessary.

[0xGabi]

Make sense @rolfyone, thank you for your input.

[jimmygchen]

@0xGabi @rolfyone I've just added SignedBeaconBlockAndBlobsSidecar schema as well - there were some discussions on the builders-spec PR, and we think it belongs here, although it's not currently consumed by any endpoints yet - let me know if you think otherwise.

[ralexstokes]

to follow up on @jimmygchen 's point, how do we expect users to download blobs from their local CL nodes?

I would assume we would have a beacon API endpoint to support this

[0xGabi]

LGTM. I left one small request for change.

how do we expect users to download blobs from their local CL nodes?

I think we should only allow to download a list of blobs_sidecar using the block_id identifier. For example: /eth/v2/beacon/blobs_sidecar/{block_id}. Downloading a single blob could be handle with a higher level library and reduce CL complexity.

On a final note, I propose to merge the current changes and address the new API endpoint on a new PR.

[jimmygchen]

@0xGabi @ralexstokes thanks for the review guys! I have addressed the comments.
Agreed with you both on the blobs endpoint, happy to do it in a separate PR it's ok?

[jimmygchen]

This PR has already grown quite big (Capella + EIP-4844 types), so I've created a separate issue to track the new endpoint for blob download: #282

[0xGabi]

LGTM

[rolfyone]

LGTM, may merge to a branch though so that we can progress this while we're waiting for a non EIP name...

[rolfyone]

just be aware - new branch: EIP4844. Once we've got a better name for the fork, this can come onto master but this will unblock any further work needed for EIP-4844

[ghost]

Ok, thank's!!!