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(primitives): remove Hardfork::Eip150, Eip158 #926

Merged
merged 1 commit into from
Jan 19, 2023

Conversation

Rjected
Copy link
Member

@Rjected Rjected commented Jan 19, 2023

This removes Hardfork::Eip150 and Hardfork::Eip158, going back to Hardfork::Tangerine. The From<EthersGenesis> for ChainSpec impl is now only determined by the value of eip_150_fork_block. The value of eip_158_fork_block is ignored.

 * removes Hardfork::Eip150 and Hardfork::Eip158, going back to
   Hardfork::Tangerine.
 * the From<EthersGenesis> for ChainSpec impl is now only determined by
   the value of eip_150_fork_block. the value of eip_158_fork_block is
   ignored.
@codecov-commenter
Copy link

Codecov Report

Merging #926 (0205149) into main (82b10fa) will decrease coverage by 0.05%.
The diff coverage is 29.41%.

@@            Coverage Diff             @@
##             main     #926      +/-   ##
==========================================
- Coverage   74.05%   74.00%   -0.05%     
==========================================
  Files         292      292              
  Lines       31722    31716       -6     
==========================================
- Hits        23493    23473      -20     
- Misses       8229     8243      +14     
Flag Coverage Δ
unit-tests 74.00% <29.41%> (-0.05%) ⬇️

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

Impacted Files Coverage Δ
crates/primitives/src/hardfork.rs 36.95% <0.00%> (+0.78%) ⬆️
crates/primitives/src/chain_spec.rs 73.79% <80.00%> (-0.02%) ⬇️
crates/executor/src/config.rs 98.59% <100.00%> (-0.02%) ⬇️
crates/transaction-pool/src/test_utils/mock.rs 53.11% <0.00%> (-6.60%) ⬇️
crates/transaction-pool/src/pool/txpool.rs 58.87% <0.00%> (-0.50%) ⬇️
crates/net/network/src/manager.rs 49.79% <0.00%> (-0.21%) ⬇️
crates/net/network/src/peers/manager.rs 83.42% <0.00%> (+0.18%) ⬆️
crates/net/network/src/session/active.rs 84.65% <0.00%> (+0.36%) ⬆️
crates/stages/src/stages/bodies.rs 93.47% <0.00%> (+0.38%) ⬆️
crates/net/eth-wire/src/types/version.rs 80.00% <0.00%> (+2.22%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@gakonst
Copy link
Member

gakonst commented Jan 19, 2023

When was EIP158 included, seems weird to ignore it? If it's part of tangerine can we remove it?

Should we call the Config blocks by the hard fork name? Instead of calling it eip150 block?

@Rjected
Copy link
Member Author

Rjected commented Jan 19, 2023

When was EIP158 included, seems weird to ignore it? If it's part of tangerine can we remove it?

EIP158 was in fact succeeded by EIP161 (as @rakita said), and only EIP150 was included in tangerine:
ethereum/EIPs#158 (comment)

However, the flag still exists in geth's ChainConfig:
https://github.com/ethereum/go-ethereum/blob/a35b654f25fdac518d5775b55aa064025040b01a/params/config.go#L68-L69

This is an inner struct as part of geth's Genesis struct:
https://github.com/ethereum/go-ethereum/blob/a35b654f25fdac518d5775b55aa064025040b01a/core/genesis.go#L50

Which means geth's genesis.json format (which we use to initialize ethers_core::utils::Geth with custom settings like clique configs) does not support any tangerine_block field, and instead still uses eip_150_block and eip_158_block.

Geth checks for these fields on startup if we want later forks to be activated (such as EIP155 or EIP1559), and will not start unless previous forks are activated. Additionally, this also still refers to EIP150 and EIP158, instead of Tangerine:
https://github.com/ethereum/go-ethereum/blob/a35b654f25fdac518d5775b55aa064025040b01a/params/config.go#L627-L677

So we can't remove it from the ethers ChainConfig format. From the above geth code, checkConfigForkOrder first checks for eip150Block, then eip155Block, then eip158Block. If we removed eip_150_block from the ethers ChainConfig struct, we would not be able to activate EIP-150, and would not be able to activate EIP-155 or any future forks. For example, we would get invalid sender errors when producing transactions for clique blocks if:

  • EIP-155 is not active AND
  • We do not explicitly modify the signature after signing, to get rid of the chainid from the v before sending.

I ran into this when trying to configure clique - rather than the chainid being incorrect, EIP-155 was not active.

Should we call the Config blocks by the hard fork name? Instead of calling it eip150 block?

We can use fork names on reth's end, and in our own configuration formats, if we want to deviate from geth's genesis.json format. But when we are working with custom geth-compatible genesis configs, we definitely can't, unless the field is already a fork name - eip_150_block, eip_155_block, and eip_158_block are outliers in that respect.

@gakonst gakonst merged commit 78ffd0a into main Jan 19, 2023
@gakonst gakonst deleted the dan/back-to-tangerine branch January 19, 2023 03:38
@gakonst
Copy link
Member

gakonst commented Jan 19, 2023

Understood. Appreciate the really detailed answer.

@rakita
Copy link
Collaborator

rakita commented Jan 19, 2023

In essence, the granularity of EIP's activation is different, it shouldn't be a problem as this is old change from 2016 and most ethereum like chains activated it from genesis.

Little detail here, EIP-150 is Tangerine but EIP-155 and EIP-161 (Supperseeds EIP-158) is SpuriousDragon, those are different hardforks. https://github.com/ethereum/execution-specs

Rinkeby activated Tangerine at block 2 and SpuriousDragon at block 3: https://github.com/ethereum/go-ethereum/blob/a35b654f25fdac518d5775b55aa064025040b01a/params/config.go#L184-L187

Goerly has them active from genesis: https://github.com/ethereum/go-ethereum/blob/a35b654f25fdac518d5775b55aa064025040b01a/params/config.go#L228-L231

Either way, calling it EIP is the legacy thing that we don't need to backport from geth.

literallymarvellous pushed a commit to literallymarvellous/reth that referenced this pull request Feb 5, 2023
literallymarvellous pushed a commit to literallymarvellous/reth that referenced this pull request Feb 5, 2023
literallymarvellous pushed a commit to literallymarvellous/reth that referenced this pull request Feb 6, 2023
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.

4 participants