-
Notifications
You must be signed in to change notification settings - Fork 200
Add getBlobs endpoint #546
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
Conversation
rolfyone
left a comment
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
apis/beacon/blobs/blobs.yaml
Outdated
| Retrieves blobs for a given block id. | ||
| Depending on `Accept` header it can be returned either as json or as bytes serialized by SSZ. | ||
|
|
||
| If the `indices` parameter is specified, only the blobs with the specified indices will be returned. There are no guarantees |
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.
I'm not sure how useful is it for users to get a list of unordered blobs without indices. If this is intended for L2s, we might as well implement retrieval by version hashes?
#332
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.
seems like from the previous discussion it might be better, would we still keep the block_id in that case as noted in #332 (comment)? We index data columns sidecars by block root or slot so dropping this would require additional indexing
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.
yep you're right, I'm bringing this up because the intent of this endpoint is for L2 users - I'm not exactly sure how they use it, but i assume they have block_id and version_hash, and so they can retrieve the blob they need?
With the getBlobSidecar endpoint, they're able to do this by computing the version_hash of individual sidecars from kzg_commitment - but without the sidecar metadata this won't work anymore.
I see a few possible options:
- return KZG commitments / version hash together with the blob
- implement version hash filtering on the server side
- returned an ordered list, so they're able to figure out which blobs they're after from the
block.kzg_commitments
I think #3 may be the least work on the beacon node, and also keep the API clean?
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.
but i assume they have block_id and version_hash, and so they can retrieve the blob they need?
Not sure how they use it either but since the blob sidecars endpoint required block_id already it should be fine
I think #3 may be the least work on the beacon node, and also keep the API clean?
(3) is the simplest for us and that's how I implemented it for now but using version_hashes for filtering should be easy change, if we go with (3) it makes sense to add explicit note that an ordered list must be returned
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.
For Scroll, it would be more convenient to have a "get blob(s) by versioned hash(es)" endpoint (without block id).
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 like from @Tristan-Wilson 's description slot numbers have concrete debugging and diagnostic advantages, too.
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.
I think it's best to keep block_id
In that case, from our perspective, I don't see much benefit to adding a new API.
Or is the idea here that later when we'll have much more blobs in each block, the response would grow too large? In that case, filtering based on versioned hashes would be useful.
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.
In that case, from our perspective, I don't see much benefit to adding a new API.
the reason for the new api is that we no longer use and store BlobSidecars on the CL side once Fusaka goes live, see #524 (comment)
Or is the idea here that later when we'll have much more blobs in each block, the response would grow too large?
yeah, just to avoid returning data you don't need and with increasing blob count this matters more
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.
Chiming in a little - I understand the OP folks are having some issues with plans to deprecate the blob_sidecars endpoint
Fulu deprecates the /eth/v1/beacon/blob_sidecars Beacon node endpoint, replacing it with /eth/v1/beacon/blobs. However, this new endpoint only provides the raw blobs, without the new kzg commitments (for cell proofs), without which verification doesn't work. So we need to investigate how we want to support this new endpoint for untrusted beacon nodes.
This is an endpoint we are currently using in production for data indexing as well, and would appreciate if it could be retained.
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 is an endpoint we are currently using in production for data indexing as well, and would appreciate if it could be retained.
The reason for deprecating the blob_sidecars endpoint is because blob sidecars no longer exist after Fulu, we only have data column sidecars.
I'd rather update the response of the blobs endpoint to include additional data as previously suggested by @jimmygchen
**Motivation** - ethereum/beacon-APIs#546 **Description** Add `GET /eth/v1/beacon/blobs/{block_id}` endpoint to retrieve blobs - pre-fulu we can just return data stored in db - post-fulu we need to reconstruct the blobs from stored data column sidecars
|
going to check with l2s for this endpoint |
|
Pushed some updates to the PR based on feedback
|
rolfyone
left a comment
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.
looks like the discussion is completed here, approving
…s as filter (#8264) **Motivation** Latest updates from ethereum/beacon-APIs#546 **Description** Update `getBlobs` beacon api to use `versioned_hashes` instead of `indices` as filter
As suggested in #524 (comment), adds new
getBlobsendpoint which only returns blobs.