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

deprecate BeaconBlocksByRange.step #2856

Merged
merged 2 commits into from
May 20, 2022
Merged

Conversation

arnetheduck
Copy link
Contributor

@arnetheduck arnetheduck commented Mar 18, 2022

The step parameter has not seen much implementation in real life
clients which instead opt to request variations on a few epochs at a
time (instead of interleaving single blocks, entire epochs are
interleaved).

At the same time, supporting step on the server side brings several
complications: more complex bounds checking logic, more complex loading
of blocks from linear storage (which presumably stores all blocks and
not just certain increments).

This PR suggests that we deprecate the whole idea. Backwards
compatibility is kept by simply responding with a single block when
step > 1 - this is allowed by the spec and should thus be handled
gracefully by requesting clients already, should there exist any that
use larger step counts.

Removing step now allows simplifying the EL-CL protocol for serving
execution data from the EL to avoid double storage.

The `step` parameter has not seen much implementation in real life
clients which instead opt to request variations on a few epochs at a
time (instead of interleaving single blocks, entire epochs are
interleaved).

At the same time, supporting `step` on the server side brings several
complications: more complex bounds checking logic, more complex loading
of blocks from linear storage (which presumably stores all blocks and
not just certain increments).

This PR suggests that we deprecate the whole idea. Backwards
compatibility is kept by simply responding with a single block when
`step > 0` - this is allowed by the spec and should thus be handled
gracefully by requesting clients already, should there exist any that
use larger step counts.

Removing `step` now allows simplifying the EL-CL protocol for serving
execution data from the EL to avoid double storage.
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

I'm happy to remove this (generally) unused optimization path.

The backwards compatible deprecation looks good to me but want to hear from client devs

specs/phase0/p2p-interface.md Outdated Show resolved Hide resolved
Co-authored-by: Danny Ryan <dannyjryan@gmail.com>
@arnetheduck
Copy link
Contributor Author

The corresponding execution API call is being proposed here: ethereum/execution-apis#218

@djrtwo
Copy link
Contributor

djrtwo commented May 13, 2022

I added this for discussion in the next CL call: ethereum/pm#527

@djrtwo djrtwo merged commit 0e6a7cd into ethereum:dev May 20, 2022
arnetheduck added a commit to status-im/nimbus-eth2 that referenced this pull request May 31, 2022
Deprecated in the spec:
ethereum/consensus-specs#2856 - future PR:s will
deprecate server support as well.
zah pushed a commit to status-im/nimbus-eth2 that referenced this pull request Jun 6, 2022
* sync: remove `step` from sync client implementation

Deprecated in the spec:
ethereum/consensus-specs#2856 - future PR:s will
deprecate server support as well.
@divagant-martian
Copy link

shouldn't we have a new protocol_id for this change?

@AgeManning
Copy link
Contributor

@arnetheduck - As @divagant-martian suggests, wouldn't it be nice to increment the version at some point so we can remove the parameter all-together?

@arnetheduck
Copy link
Contributor Author

arnetheduck commented Jun 20, 2022

@arnetheduck - As @divagant-martian suggests, wouldn't it be nice to increment the version at some point so we can remove the parameter all-together?

I would 👍 a PR but it's also a fair bit of coordination work, ie we'd have to ensure all clients implement then keep the old request around over a hard fork and get the new request implemented before.

The current wording requires that we return one block - the way I worded it makes it slightly ambiguous in that it doesn't specify what to do if the block at start is missing: should we return the first non-empty block that satisfies the non-1 step condition or return nothing? this should be addressed in such a PR - the latter interpretation effectively might turn into a backwards-compatibility issue if clients overinterpret empty responses.

@AgeManning
Copy link
Contributor

Yeah, there is a small overhead. Like if each client just made a v2 and set it as a priority, no-coordination is needed. Once we hard-fork and know all nodes on the network have a v2, all clients in their own time can remove v1. So I guess the only co-ordination we need is that all clients should implement and prioritize a v2 before some hard fork.

Saves us very little bandwidth, but just seems like a nicer approach.

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.

5 participants