Skip to content

feat!: remove support for eth/62, eth/63, eth/64 and eth/65 #8492

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

Merged

Conversation

Gabriel-Trintinalia
Copy link
Contributor

@Gabriel-Trintinalia Gabriel-Trintinalia commented Mar 31, 2025

Description

fixes #7695

This pull request removes support for the older Ethereum protocol versions eth/62, eth/63, eth/64, and eth/65.

Changes:

  • Remove eth/62 and eth/63 Protocol Versions:

    • Eliminate all references and implementations related to eth/62 and eth/63.
  • Remove eth/64 Protocol Version:

    • Remove all references to legacyForkId.
    • Deprecate the --compatibility-eth64-forkid-enabled flag.
    • Make ForkId a required component for constructing a StatusMessage.
    • Ensure that decoding and encoding of ForkId are always present.
  • Remove eth/65 Protocol Version:

    • Remove the isEth66Compatible check, as all newer protocol versions are inherently compatible.
    • Eliminate all checks related to isEth66Compatible.

Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
@Gabriel-Trintinalia Gabriel-Trintinalia added doc-change-required Indicates an issue or PR that requires doc to be updated breaking This can only be addressed/merged for a release that allows user-facing changes to be breaking. techdebt maintenance, cleanup, refactoring, documentation labels Mar 31, 2025
Signed-off-by: Gabriel-Trintinalia <[email protected]>
@Gabriel-Trintinalia Gabriel-Trintinalia changed the title Remove support to eth/62, eth/63, eth/64 and eth/65 Remove support for eth/62, eth/63, eth/64 and eth/65 Mar 31, 2025
@Gabriel-Trintinalia Gabriel-Trintinalia changed the title Remove support for eth/62, eth/63, eth/64 and eth/65 feat!: remove support for eth/62, eth/63, eth/64 and eth/65 Mar 31, 2025
Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

Nice cleanup!

@Gabriel-Trintinalia
Copy link
Contributor Author

@fab-10 thanks! There may be some clean-up to be done in the TransactionBroadcaster as well when this PR is merged since we don't need to announce full transactions anymore

names = {LEGACY_ETH_64_FORK_ID_ENABLED},
paramLabel = "<Boolean>",
description = "Enable the legacy Eth/64 fork id. (default: ${DEFAULT-VALUE})")
private Boolean legacyEth64ForkIdEnabled =
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it's worth keeping just the CLI option as a no-op for one release so we don't break existing configs? Make it a hidden option, flag it as a upcoming breaking change. The rest of the PR is non-breaking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@pinges pinges left a comment

Choose a reason for hiding this comment

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

A few comments :-)

@@ -178,13 +174,6 @@ private List<Capability> calculateCapabilities(
final SynchronizerConfiguration synchronizerConfiguration,
final EthProtocolConfiguration ethProtocolConfiguration) {
final List<Capability> capabilities = new ArrayList<>();

if (SyncMode.isFullSync(synchronizerConfiguration.getSyncMode())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does that mean that eth62 is not needed anymore for full sync (why was it needed) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not needed. Probably left there to talk to really old archive nodes.

boolean supportsRequestId =
EthProtocol.isEth66Compatible(msg.capability)
&& EthProtocol.requestIdCompatible(msg.messageData.getCode());
boolean supportsRequestId = EthProtocol.requestIdCompatible(msg.messageData.getCode());
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that all our supported protocols now are request id compatible. Do we still need that method/check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The requestIdCompatible is checking if the message itself requires the requestId. (Some messages don't, such as Status, Transactions)

@Gabriel-Trintinalia Gabriel-Trintinalia merged commit f795473 into hyperledger:main Apr 1, 2025
43 checks passed
@alexandratran alexandratran removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Apr 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This can only be addressed/merged for a release that allows user-facing changes to be breaking. techdebt maintenance, cleanup, refactoring, documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove old eth protocols
5 participants