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

Add arrowGlacier HF #1527

Merged
merged 5 commits into from
Nov 8, 2021
Merged

Add arrowGlacier HF #1527

merged 5 commits into from
Nov 8, 2021

Conversation

jochem-brouwer
Copy link
Member

This PR adds the arrow glacier HF, which delays the difficulty bomb.

Since we have no test cases, do not merge yet.

@codecov
Copy link

codecov bot commented Oct 15, 2021

Codecov Report

Merging #1527 (7807f02) into master (c230e51) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

Flag Coverage Δ
block 84.96% <ø> (ø)
blockchain 85.08% <ø> (ø)
common 100.00% <ø> (ø)
devp2p 82.89% <ø> (ø)
ethash ∅ <ø> (∅)
tx 87.44% <ø> (ø)
vm 79.08% <ø> (ø)

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

@holgerd77
Copy link
Member

For reference: ethereum/tests#965 (ethereum/tests tracking issue for test implementation)

@holgerd77
Copy link
Member

For reference: ethereum/tests#972 (new difficulty tests on ethereum/tests)

@holgerd77
Copy link
Member

I guess at some point we should also get over the finish line with this. Any updates here? What's the current status? 🤔

@jochem-brouwer
Copy link
Member Author

If we want we can merge this and then at a later point run it against ethereum/tests? I can try running the test locally to see if they pass, if thats the case then we can also merge.

@holgerd77
Copy link
Member

Ok, maybe let's then wait for the official ethereum/tests release, guess that can't be too far away, and some in between step we should also update to v10.1 since this release comes with some significant structural changes (legacy tests moved to own repo).

@jochem-brouwer Can you do rebase in between and bring this up-to-date so that we can keep up here a bit with keeping this close to master?

@holgerd77
Copy link
Member

Will mainnet be the only fork here or will there be a test fork on ropsten? Or would this not working out due to differing practical TD values anyhow? 🤔

@jochem-brouwer
Copy link
Member Author

jochem-brouwer commented Nov 7, 2021

Ok this is weird, the tests do not pass, I tried locally. To test;

cd packages/ethereum-tests
git checkout develop
git pull

This pulls the latest data from the repo, the difficulty tests are not added yet.

Now to /block/test/difficulty.spec.ts add this to hardforkTestData (line 37)

arrowGlacier: require('../../ethereum-tests/DifficultyTests/dfArrowGlacier/difficultyArrowGlacier.json').difficultyArrowGlacier.ArrowGlacier

Only running these tests;

1..10776
# tests 10776
# pass  9896
# fail  880

@holgerd77
Copy link
Member

Yes, can confirm.

Also ran this locally, when digging a bit deeper one sees that every second test with "parentUncles" : "0x00" is failing.

Not sure yet what is happening, the format of this uncles field changed substantially (there was a full hash before), will ask on ethereum/tests.

@holgerd77
Copy link
Member

The semantics of the uncles field has changed, see here.

If one is replacing 0x00 (which now stands for "no uncles") with an just an empty field (which translates for our instantiation to "instantiate with no uncles", this works again.

Will push.

@jochem-brouwer
Copy link
Member Author

@holgerd77 I see, but I don't understand how this passes. I don't see anything in your commits which handles this parentUncles field, so I do not understand why this passes. Is it an edited tests file?

(Small sidenote: we should at some point make these block tests not use the copied JSON files from ethereum/tests, but directly read them from ethereum/tests)

@holgerd77
Copy link
Member

Yes, edited test file.

@jochem-brouwer
Copy link
Member Author

Ah right. We should fix that at some point, I don't think it is so hard, seems like the difficulty formula is adjusted if either the uncle hash is the empty RLP list hash OR it is not, so we could just create a bogus hash which should (?) not crash the difficulty calculation. Will approve here.

@jochem-brouwer
Copy link
Member Author

Oh right, I can't approve my own PR 😂

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

LGTM and x-approved by @jochem-brouwer, will merge.

@holgerd77 holgerd77 merged commit 309988c into master Nov 8, 2021
@holgerd77 holgerd77 deleted the arrowglacier branch November 8, 2021 15:59
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.

3 participants