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 blob download endpoint (getBlobs) #286

Merged
merged 29 commits into from
May 17, 2023

Conversation

jimmygchen
Copy link
Contributor

@jimmygchen jimmygchen commented Jan 9, 2023

Description

Addresses #282. Add /eth/v1/beacon/blobs/{block_id} endpoint to support retrieval of blobs by block id.

@jimmygchen
Copy link
Contributor Author

jimmygchen commented Jan 9, 2023

From what I can see the request / response structures look similar, just some differences in paths. I think a combination of the two makes sense to me (e.g. /eth/v1/beacon/blobs/sidecar/{block_id}) for the following reasons:

  1. It feels like the blob sidecar belongs under beacon as it's closely related to the beacon block
  2. Having blobs/sidecars instead of blobs_sidecar may provide more flexibility, although I don't see any other potential endpoints that may fall under blobs.. so no strong opinion on this.

I'm still pretty new to this so I might be missing something - keen to hear suggestions / opinions.

@arnetheduck
Copy link
Contributor

One potential issue of note: in the underlying libp2p protocol, we don't support fetching finalized items by root, instead only supporting by-slot lookups - it might be prudent to note this in this spec as well, that sidecar retrieval for finalized blocks should by done by slot else the underlying index might not be available.

Doing it this way allows clients to implement this API using linear storage which is cheaper and more efficient and more closely matches the requirements that clients have to support in order to implement the mandatory parts of the protocol.

@mcdee
Copy link
Contributor

mcdee commented Jan 9, 2023

I'd be inclined to go for just /beacon/blobs as the path, I don't see the _sidecar piece adding any useful information (it's a information about how we're treating the blobs, not what they are). There's also an argument to have just /blobs, especially if the plan is to make them available from places other then the beacon node, although I'm not overly bothered about that one.

@terencechain
Copy link

Added Prysm implementation here: prysmaticlabs/prysm@50b672a

@dapplion dapplion mentioned this pull request Jan 10, 2023
10 tasks
@jimmygchen
Copy link
Contributor Author

One potential issue of note: in the underlying libp2p protocol, we don't support fetching finalized items by root, instead only supporting by-slot lookups - it might be prudent to note this in this spec as well, that sidecar retrieval for finalized blocks should by done by slot else the underlying index might not be available.

Doing it this way allows clients to implement this API using linear storage which is cheaper and more efficient and more closely matches the requirements that clients have to support in order to implement the mandatory parts of the protocol.

@arnetheduck thanks for this! I don't fully understand the implication of this on the Beacon API, would appreciate if you could elaborate a bit more - if the blob is already available in CL storage and p2p lookup isn't required, would this still be an issue?

@arnetheduck
Copy link
Contributor

arnetheduck commented Jan 11, 2023

blob is already available in CL storage

in the core protocol, there is no requirement to index finalized data by hash, only by slot - this applies equally to blobs as it does to blocks.

Because finalized data is strictly linear, the index-by-hash is not needed and looking things up by hash is comparatively inefficient, as is building and maintaining the hash index itself.

"CL storage" for finalized data can be implemented as a flat file to serve core protocol needs - by suggesting consumers to use slots, the same efficiency can be had in the REST API as well, which saves on disk space, processing and complexity, in the CL.

edit: you're indeed right that it is also more simple to forward a request to the network when topping up local storage if that request is done by slot, since the network doesn't carry said hash index.

@jimmygchen
Copy link
Contributor Author

jimmygchen commented Jan 12, 2023

I'd be inclined to go for just /beacon/blobs as the path, I don't see the _sidecar piece adding any useful information (it's a information about how we're treating the blobs, not what they are). There's also an argument to have just /blobs, especially if the plan is to make them available from places other then the beacon node, although I'm not overly bothered about that one.

Good point on _sidecar - I think I'm just getting too used to the term as a spec type now. One reason I think it could be helpful is that it tells user that the endpoint returns the BlobsSidecar spec object rather than just the blobs. If that's not a strong enough reason, I think we're ok with dropping _sidecar.

@@ -95,6 +95,8 @@ paths:
$ref: "./apis/beacon/blocks/root.yaml"
/eth/v1/beacon/blocks/{block_id}/attestations:
$ref: "./apis/beacon/blocks/attestations.yaml"
/eth/v1/beacon/blobs_sidecars/{block_id}:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually be inclined to make this the same pattern as attestations, so the path would be more like
/eth/v1/beacon/blocks/{block_id}/blobs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't that imply that the blobs are part of the block structure, though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to /eth/v1/beacon/blobs/{block_id}

@rolfyone
Copy link
Collaborator

We should add to top level CHANGES.md too

@jimmygchen
Copy link
Contributor Author

@rolfyone @mcdee
I've taken your suggestions to remove _sidecar from the path and added the endpoint to CHANGES.md. I originally thought about having blocks in the path as well, but I think @mcdee made a good point and blobs aren't part of the block structure.

@arnetheduck thanks a lot for the explanation, If I understand you correctly, you're suggesting to not support lookup by root for blobs (and perhaps blocks too) as it would allow clients implement this more efficiently. I made the assumption we would want the same lookup (block id) for blobs as well, as looking up by root is already supported for blocks in the Beacon API. I don't really understand the libp2p protocol and storage implementations enough to make a comment about this (but interested to find out more if there's any resources you could point me to) - I'll wait for others to chime in on this.

@arnetheduck
Copy link
Contributor

you're suggesting to not support lookup by root for blobs (and perhaps blocks too) as it would allow clients implement this more efficiently.

Perhaps more weakly than this, ie "recommend" that consumers look up by slot - it's possible that some clients build a by-root index for the entire history (consider archive nodes for example) but it is certainly more "compatible" to not rely on it and instead use slot (which also can offer better performance) - it would indeed be the case that the same recommendation should be added to block lookups - might get to this at some point unless someone beats me to it.

@realbigsean realbigsean mentioned this pull request Feb 8, 2023
@jimmygchen
Copy link
Contributor Author

jimmygchen commented Feb 13, 2023

Just going through the changes in the "Free the blobs" PR ethereum/consensus-specs#3244, and will soon be updating this branch accordingly.

@realbigsean
Copy link
Contributor

@jimmygchen

I don't think this will have to change too much, I think it should probably return the same struct we return in the BlobsByRange request as this is likely how we'll store blobs in our database. This is what it looks like right now https://github.com/ethereum/consensus-specs/pull/3244/files#diff-21cd1dde2a4815e714ef42d0cefa1bbd2997a4358ecf6e9e51025332c1d642fdR222-R224

@jimmygchen jimmygchen changed the base branch from EIP4844 to master February 14, 2023 04:39
types/eip4844/block.yaml Outdated Show resolved Hide resolved
types/eip4844/block.yaml Outdated Show resolved Hide resolved
types/eip4844/block.yaml Outdated Show resolved Hide resolved
types/eip4844/block.yaml Outdated Show resolved Hide resolved
types/eip4844/block.yaml Outdated Show resolved Hide resolved
types/eip4844/block.yaml Outdated Show resolved Hide resolved
@@ -95,6 +95,8 @@ paths:
$ref: "./apis/beacon/blocks/root.yaml"
/eth/v1/beacon/blocks/{block_id}/attestations:
$ref: "./apis/beacon/blocks/attestations.yaml"
/eth/v1/beacon/blobs_sidecars/{block_id}:
$ref: "./apis/beacon/blobs_sidecars/blobs_sidecar.yaml"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do both need to be plurals? is blob_sidecars accurate?

if the paths / files / object names can be consistent that'll make it easier to navigate

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I think this is on an old commit as well, I just did a global search for blobs_ and not finding anything. I'll make sure the paths / file name are matching if we end up changing the paths 👍

beacon-node-oapi.yaml Outdated Show resolved Hide resolved
@rolfyone
Copy link
Collaborator

So i think this is probably close, just whether namings are correct blob_sidecars v. blobs_sidecars and being consistent...

also my changes markdown comment about whatever the path ends up as :)

Then I think we should be right to merge...

@rolfyone
Copy link
Collaborator

Hope for one last merge to fix conflicts...

@jimmygchen
Copy link
Contributor Author

Hope for one last merge to fix conflicts...

Thanks @rolfyone I've merged master into this branch and updated the paths.

rolfyone
rolfyone previously approved these changes May 17, 2023
Copy link
Collaborator

@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

type: object
properties:
data:
type: array
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We appear to have the type BlobSidecars that is an array of blob sidecars with the same parameters as this. Should that be used in place of redefining the array here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Updated in 096aadc, thanks!

g11tech
g11tech previously approved these changes May 17, 2023
@jimmygchen jimmygchen dismissed stale reviews from g11tech and rolfyone via 096aadc May 17, 2023 09:57
@jimmygchen jimmygchen requested a review from mcdee May 17, 2023 09:58
Copy link
Collaborator

@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, thanks team!

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.