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

Grayglacier HF Support (master) #1983

Closed
wants to merge 4 commits into from
Closed

Grayglacier HF Support (master) #1983

wants to merge 4 commits into from

Conversation

ScottyPoi
Copy link
Contributor

@ScottyPoi ScottyPoi commented Jun 22, 2022

Adds support for GrayGlacier HF re: #1960

Duplicated for v5-maintenance in #1984

@codecov
Copy link

codecov bot commented Jun 22, 2022

Codecov Report

Merging #1983 (8eabfb2) into master (1bd5c4f) will decrease coverage by 5.40%.
The diff coverage is 100.00%.

Impacted file tree graph

Flag Coverage Δ
block 84.30% <ø> (?)
blockchain 81.03% <ø> (+0.02%) ⬆️
client 78.34% <ø> (?)
common 95.03% <100.00%> (?)
devp2p 82.78% <ø> (?)
ethash 90.81% <ø> (ø)
evm 42.62% <ø> (?)
statemanager 84.55% <ø> (?)
trie 73.72% <ø> (ø)
tx 92.17% <ø> (?)
util 87.03% <ø> (?)
vm 76.87% <ø> (ø)

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

@ScottyPoi
Copy link
Contributor Author

The latest develop branch for ethereum-tests has the .json test files for Gray Glacier.
I think the block test is failing because it is not looking at the current branch.

I don't know how to address this

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.

Nice, thanks Scotty, looks good already, some few comments!

{
"name": "grayGlacier",
"block": 15050000,
"forkHash": null
Copy link
Member

Choose a reason for hiding this comment

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

Please calculate an explicit forkHash here, you can do this with the respective method in the Common library. If you would get some confirmation that the forkHash is correct - e.g. by looking at other client implementation GrayGlacier HF PRs - that would also be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. the grayGlacier markdown file had forkHash: TBA -- i'll check with other clients

@@ -21,6 +21,7 @@ export enum Hardfork {
Berlin = 'berlin',
London = 'london',
ArrowGlacier = 'arrowGlacier',
GrayGlacier = 'grayGlacier',
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated, but since you are already working on the code: can you please move Shanghai to the last position here? This is some relic from times where people thought Shanghai would happen before the Merge. 😋

(general note: please do all change requests on both PRs, maybe do all in one commit and then cherry-pick over?)

@@ -37,6 +37,9 @@ const hardforkTestData: TestData = {
arrowGlacier:
require('../../ethereum-tests/DifficultyTests/dfArrowGlacier/difficultyArrowGlacier.json')
.difficultyArrowGlacier.ArrowGlacier,
grayGlacier:
require('../../ethereum-tests/DifficultyTests/dfGrayGlacier/difficultyGrayGlacier.json')
.difficultyGrayGlacier.GrayGlacier,
Copy link
Member

Choose a reason for hiding this comment

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

So to your question with the tests: just copy the whole file over and move to the local Block test data folder and reference through the local path. You can maybe add a TODO here that this should be replaced with the ethereum-tests folder again at some point.

@holgerd77 holgerd77 changed the title Grayglacier HF Support Grayglacier HF Support (master) Jun 23, 2022
@ScottyPoi ScottyPoi closed this Jun 23, 2022
@holgerd77 holgerd77 deleted the grayglacier branch March 2, 2023 09:53
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

2 participants