-
Notifications
You must be signed in to change notification settings - Fork 174
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
Add Electra blob sidecars type to support max blobs from EIP-7691 #488
base: master
Are you sure you want to change the base?
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 |
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.
Prysm does already return the metadata
Grandine returns the metadata as well |
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.
LGTM
items: | ||
$ref: "../deneb/blob_sidecar.yaml#/Deneb/BlobSidecar" | ||
minItems: 0 | ||
maxItems: 9 |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It returns both JSON and SSZ
beacon-APIs/apis/beacon/blob_sidecars/blob_sidecars.yaml
Lines 50 to 52 in aa1be25
application/octet-stream: | |
schema: | |
description: "SSZ serialized `BlobSidecars` bytes. Use Accept header to choose this response type" |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Adds new
BlobSidecars
type to Electra with max items of 9 to matchMAX_BLOBS_PER_BLOCK_ELECTRA
from EIP-7691.In addition this PR makes the metadata fields added in #441 and the
Eth-Consensus-Version
header required. Implementations should make sure to support this at latest in their Electra release.