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 phase 0-only versions of beacon_blocks_by_{range,root} #3014

Merged
merged 1 commit into from
Nov 29, 2022
Merged

deprecate phase 0-only versions of beacon_blocks_by_{range,root} #3014

merged 1 commit into from
Nov 29, 2022

Conversation

tersec
Copy link
Contributor

@tersec tersec commented Sep 26, 2022

There haven't been any phase0 blocks in mainnet or any of the still-supported testnets in MIN_EPOCHS_FOR_BLOCK_REQUESTS regardless, so the spec already technically supports not serving said blocks.

https://github.com/ethereum/consensus-specs/blob/dev/specs/altair/p2p-interface.md#beaconblocksbyrange-v2 and https://github.com/ethereum/consensus-specs/blob/dev/specs/altair/p2p-interface.md#beaconblocksbyroot-v2 work across all supported forks, including phase 0, and any CL client supporting Altair or newer already has to support those messages, so this maintains req/resp compatibility for phase 0 blocks.

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.

looks reasonable to me.

will leave this open for a week for comment

Copy link
Contributor

@nisdas nisdas left a comment

Choose a reason for hiding this comment

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

LGTM, should we also apply the same principle to v1 metadata requests ? They are deprecated and all clients should support v2 from altair onwards.

@hwwhww
Copy link
Contributor

hwwhww commented Nov 22, 2022

Pinging Teku for approval @ajsutton @rolfyone 👋

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants