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

Make genesis params settable for any hardfork in block headers #1153

Merged
merged 1 commit into from
Mar 17, 2021

Conversation

cgewecke
Copy link
Contributor

@cgewecke cgewecke commented Mar 17, 2021

PR removes a restriction introduced in #863 that disallows commons with custom genesis settings unless their hardforks are set to chainstart.

This is blocking integration of the block module in the Hardhat testrpc because they set a custom common with special gas limits, chainId, etc as an initial step on launch, but need to run on arbitrary hardforks.

https://github.com/nomiclabs/hardhat/blob/579d44a105d6a8323d270ea55876c6694c212316/packages/hardhat-core/src/internal/hardhat-network/provider/utils/makeCommon.ts#L20-L42

Perhaps this restriction should be preserved but gated behind a flag - not sure. Am opening in the simplest form for further discussion.

@cgewecke cgewecke changed the title Make genenis params settable for any hardfork in block headers Make genesis params settable for any hardfork in block headers Mar 17, 2021
@codecov
Copy link

codecov bot commented Mar 17, 2021

Codecov Report

Merging #1153 (6502403) into master (619ab5b) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

Flag Coverage Δ
block 82.00% <ø> (+0.17%) ⬆️
blockchain 84.19% <ø> (ø)
client 84.03% <ø> (ø)
common 86.87% <ø> (-0.53%) ⬇️
devp2p 84.16% <ø> (-0.03%) ⬇️
ethash 82.08% <ø> (ø)
tx 84.25% <ø> (ø)
vm 81.67% <ø> (ø)

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

@holgerd77
Copy link
Member

On thinking about this I have the impression this restriction is just plain wrong and is rather a bug fix. I hadn't it on the radar that several hardforks can be applied together on one block with the special case of this happening on the genesis block (as it is the case for goerli e.g.).

So seems to safe to me to merge "as is".

@holgerd77
Copy link
Member

(also recently had to fix some similar - or at least somewhat related bug - on some hardfork determination logic in Common)

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

@holgerd77 holgerd77 merged commit f97d9eb into master Mar 17, 2021
@holgerd77 holgerd77 deleted the remove-genesis-hf-check branch March 17, 2021 19:57
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