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 some failing hive tests in the devp2p/snap sync test suite #3847

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

scorbajio
Copy link
Contributor

This change does two things:

  • Remove the TTD check in common that results in failed geth genesis parsing when importing a genesis with a terminalTotalDifficulty (TTD) field
  • Add missing forks to fork map in common

These two changes are necessary in being able to parse and import blockchain data correctly when the deprecated TTD field is being used.

Thanks to @acolytec3 and @jochem-brouwer for their invaluable contributions in debugging this issue.

Copy link

codecov bot commented Jan 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.49%. Comparing base (60ed998) to head (28ac7e7).

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block ?
blockchain ?
client ?
common ?
devp2p 75.66% <ø> (ø)
ethash 80.80% <ø> (ø)
evm ?
genesis 99.84% <ø> (ø)
mpt 57.08% <ø> (+0.03%) ⬆️
rlp 69.70% <ø> (ø)
statemanager ?
tx ?
util 76.91% <ø> (ø)
vm ?
wallet 82.80% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@@ -195,10 +188,10 @@ function parseGethParams(json: any) {
}

if (config.terminalTotalDifficulty !== undefined) {
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 this entire if block should be removed. We don't care about terminalTotalDifficulty anymore. Note that if mergeForkBlock is set in the config, then both the Paris and the MergeForkIdTransition will point to that block (thus it is not necessary to set it again).

(But please re-test the hive test if this is changed :) )

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.

2 participants