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

Fix eth capabilities for archive nodes #5655

Merged
merged 2 commits into from
May 4, 2023
Merged

Conversation

marcindsobczak
Copy link
Contributor

Changes

Right now eth/67 and eth/68 capabilities are not enabled when they should if:

  • node is in archive mode (SnapSync and FastSync flags are false)
  • there is no state

Capabilites are enabled if node with any state is restarted.

It is a bit problematic for 4844 testnets and very problematic for 4844 hive tests.
It is not affecting production nodes right now, but would be a problem for archive nodes after dropping eth/66 by Geth

After this fix, eth/67 and eth/68 capabilities would be enabled always for archive nodes (when FastSync is false)

Types of changes

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

Copy link
Contributor

@MarekM25 MarekM25 left a comment

Choose a reason for hiding this comment

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

Important fix! I'm wondering if we can add unit tests

@marcindsobczak
Copy link
Contributor Author

We can, but it will not be a simple unit test, it will take significant amount of work and time, as it is a change in method triggered only during client initialization - we would need to simulate that somehow and test with various node types (archive, fast-only, snap) and states (no state, existing state). All logic with switching capabilities will be removed soon - after delivering snap serving, so probably writing tests to it is not the best allocation of resources

@marcindsobczak marcindsobczak merged commit 41dedcb into master May 4, 2023
@marcindsobczak marcindsobczak deleted the fix/eth_capabilities branch May 4, 2023 13:35
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.

3 participants