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

Add Electra blob sidecars type to support max blobs from EIP-7691 #488

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nflaig
Copy link
Member

@nflaig nflaig commented Jan 21, 2025

Adds new BlobSidecars type to Electra with max items of 9 to match MAX_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.

@nflaig nflaig marked this pull request as ready for review January 21, 2025 11:57
@mcdee
Copy link
Contributor

mcdee commented Jan 21, 2025

The change to the existing endpoint to now require Eth-Consensus-Version is technically breaking, it's worth doing a quick check of the endpoint on existing beacon nodes to confirm if they supply this, and if not to raise an issue so that they are aware of this and can address it ahead of time.

But in general LGTM.

@nflaig
Copy link
Member Author

nflaig commented Jan 21, 2025

The change to the existing endpoint to now require Eth-Consensus-Version is technically breaking, it's worth doing a quick check of the endpoint on existing beacon nodes to confirm if they supply this

I updated the PR description to highlight this, Lodestar already sets this header in the stable release.

@mcdee
Copy link
Contributor

mcdee commented Jan 21, 2025

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.

@nflaig
Copy link
Member Author

nflaig commented Jan 21, 2025

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

Copy link
Contributor

@rkapka rkapka left a 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

@Tumas
Copy link

Tumas commented Jan 21, 2025

Grandine returns the metadata as well

Copy link
Contributor

@rolfyone rolfyone left a 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
Copy link
Member

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

Copy link
Member Author

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

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.

Copy link
Contributor

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.

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.

7 participants