Add Electra blob sidecars type to support max blobs from EIP-7691#488
Add Electra blob sidecars type to support max blobs from EIP-7691#488
Conversation
|
The change to the existing endpoint to now require But in general LGTM. |
I updated the PR description to highlight this, Lodestar already sets this header in the stable release. |
|
Sure, but it's still a breaking change to an existing endpoint which is unexpected and may not be picked up by client teams without a nudge. |
|
From references of the previous PR, looks like at least Prysm and Teku should be fine as well, so there is only Nimbus, Lighthouse and Grandine left to check |
rkapka
left a comment
rkapka
left a comment
There was a problem hiding this comment.
Prysm does already return the metadata
|
Grandine returns the metadata as well |
| items: | ||
| $ref: "../deneb/blob_sidecar.yaml#/Deneb/BlobSidecar" | ||
| minItems: 0 | ||
| maxItems: 9 |
There was a problem hiding this comment.
This API returns JSON, why is it relevant this maxItems value? Even for SSZ, in it's serialized form the max value is not relevant
There was a problem hiding this comment.
It returns both JSON and SSZ
beacon-APIs/apis/beacon/blob_sidecars/blob_sidecars.yaml
Lines 50 to 52 in aa1be25
Even for SSZ, in it's serialized form the max value is not relevant
I quickly mentioned this in discord, we could just use MAX_BLOB_COMMITMENTS_PER_BLOCK which is 4096 as max items which is what we use in Lodestar for the list limit.
There was a problem hiding this comment.
It seems better to discard invalid data as quickly as possible, and anything past MAX_BLOBS_PER_BLOCK_ELECTRA is invalid. No need to build in capacity in the rest of the system to handle it, either on the client or server end, for theoretical possibilities which even if working in isolation aren't useful as part of a broader system.
There was a problem hiding this comment.
You are talking about a consumer of the REST API, not a p2p network with adversarial peers. I think MAX_BLOB_COMMITMENTS_PER_BLOCK is fine and we don't have to deal with forks on this type anymore
There was a problem hiding this comment.
Unclear that https://ethereum.github.io/beacon-APIs/?urls.primaryName=dev#/Beacon/getBlobSidecars will even be useful/salient in Fulu; the beacon API might end up with a new getColumnSidecars or similar endpoint. A similar discussion has been happening in the req/resp area, where the v3 Fulu-only getBlobsByRange was removed in lieu of keeping only the column one.
There was a problem hiding this comment.
we can always change this to MAX_BLOB_COMMITMENTS_PER_BLOCK in a follow-up PR, I don't feel strongly about this as both approaches work
8 participants
Adds new
BlobSidecarstype to Electra with max items of 9 to matchMAX_BLOBS_PER_BLOCK_ELECTRAfrom EIP-7691.In addition this PR makes the metadata fields added in #441 and the
Eth-Consensus-Versionheader required. Implementations should make sure to support this at latest in their Electra release.