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

EIP4844: Add block and sidecar retrival by root #3089

Merged

Conversation

terencechain
Copy link
Contributor

Part of #3088

This PR adds a req-resp RPC for retrieving beacon block and blobs sidecar by root


```
(
List[SignedBeaconBlockAndBlobsSidecar, MAX_REQUEST_BLOCKS]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I rename MAX_REQUEST_BLOCKS to include sidecars? I didn't feel strongly about this

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine by me. Though another thing is do we wanna tweak the current MAX_REQUEST_BLOCKS value? If a client requests the max and they all contain sidecars, that's a huge amount of data transmitted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless this is also constrained by the MAX_CHUNK_SIZE.

Copy link
Member

Choose a reason for hiding this comment

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

I think it is clear as it is

Copy link
Contributor

Choose a reason for hiding this comment

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

I do think that we should at least consider MAX_REQUEST_BLOCKS for this and range requests given the size changes. That said, you only MUST respond with 1 in a given request/range so even if the max stays high, clients can down regulate responses and stay to spec


Per `context = compute_fork_digest(fork_version, genesis_validators_root)`:
Clients MUST disable `BeaconBlocksByRoot v2` after `EIP4844_FORK_EPOCH`.
Copy link
Member

Choose a reason for hiding this comment

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

what if I want to get a beacon block by root from before the 4844 fork?

seems like we want to keep BeaconBlocksByRootV2 for this purpose or support previous types with the context enum like in the ByRange spec 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.

Makes sense. I added back the previous types except for 4844 type

Copy link
Contributor

@Inphi Inphi left a comment

Choose a reason for hiding this comment

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

LGTM! I'm still not sure if clients, prysm in particular, do not sometimes need to use BlocksByRoot (and BlocksAndBlobsSidecarByRoot) during historical sync. I'll defer to your decision on whether we should explicitly permit ByRoot req-res for older blocks.

@hwwhww hwwhww added the Deneb was called: eip-4844 label Nov 10, 2022
@hwwhww hwwhww merged commit 11a037f into ethereum:dev Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deneb was called: eip-4844
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants