Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Add missing forks to fork ID #11747

Merged
merged 1 commit into from
Jun 1, 2020
Merged

Conversation

vorot93
Copy link

@vorot93 vorot93 commented Jun 1, 2020

Fixes #11744

@vorot93 vorot93 requested a review from sorpaas June 1, 2020 13:53
@vorot93 vorot93 added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jun 1, 2020
@wesrer wesrer self-requested a review June 1, 2020 13:58
@meowsbits
Copy link
Contributor

meowsbits commented Jun 1, 2020

Thank you @vorot93 🙏 . Indeed, far better than the hack. And fast! 🏎️

@GregTheGreek
Copy link

GregTheGreek commented Jun 1, 2020

Great work guys!

I understand the issue, but curious @vorot93 what exactly is going on under the hood here?

@vorot93
Copy link
Author

vorot93 commented Jun 1, 2020

@GregTheGreek Clients using eth/64 protocol and higher send a FORK_ID which is a hash of all fork heights. It is used to aggressively cut off incompatible clients. Unfortunately, not all heights were included for Classic which made OpenEthereum's ForkId incompatible on a network protocol (not consensus) layer.

Fortunately, the clients have not yet removed support for eth/63 protocol, with Parity Ethereum 2.7.2 serving as an eth/63 bridge between Core-Geth and OpenEthereum 3.0. After we mandate FORK_ID any bug in its generation would instantly lead to a complete network partition.

@vorot93 vorot93 added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 1, 2020
@meowsbits
Copy link
Contributor

meowsbits commented Jun 1, 2020

not all heights were included for Classic which made OpenEthereum's ForkId incompatible

Which ones weren't included? (And why wasn't the incompatibility happening before?)

The reason I ask is the changes include new fork features as follows, and I don't see any that look as though they were introduced to the Classic config with the latest fork (Phoenix)

					ethash.params.difficulty_hardfork_transition,
					ethash.params.bomb_defuse_transition,
					ethash.params.eip100b_transition,
					ethash.params.ecip1010_pause_transition,
					ethash.params.ecip1010_continue_transition,
					ethash.params.ecip1017_era_rounds,
					ethash.params.expip2_transition,

https://github.com/openethereum/openethereum/pull/11747/files#diff-9a0d4c3f753d90ed4b2dd57d597ee2c1R433-R439

(Althought I don't know what expip2_transition is...)

@GregTheGreek
Copy link

Right on - I understand that side of it, im actually curious at the changs you made and how that solves it

ethash.params.ecip1010_pause_transition,
ethash.params.ecip1010_continue_transition,
ethash.params.ecip1017_era_rounds,
ethash.params.expip2_transition,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a typo there?

exip instead of ecip

Copy link
Contributor

@YazzyYaz YazzyYaz Jun 1, 2020

Choose a reason for hiding this comment

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

Whelps, looks like I misread it as exip instead of expip. explains why I couldn't grep it. Ignore my message, sorry!

Copy link
Author

Choose a reason for hiding this comment

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

No typo here. It's EXPIP-2 related to Expanse chain, not Classic.

@vorot93
Copy link
Author

vorot93 commented Jun 1, 2020

@GregTheGreek I just added the transitions that were overlooked, along with a test for Classic. I admit that it should have been caught before, but we only had the test vectors for Foundation, Ropsten, Rinkeby and Goerli to check against.

@meowsbits
Copy link
Contributor

meowsbits commented Jun 1, 2020

Does the test show that the the ForkID for Classic at and beyond 10500839 is hash=9007bfcc next=0?

@vorot93
Copy link
Author

vorot93 commented Jun 1, 2020

Also, don't worry about expip2_transition - it's not defined in any chain specification except Expanse's one, and thus will not affect FORK_ID generation for Classic in any way.

@sorpaas sorpaas merged commit d4733b8 into master Jun 1, 2020
@sorpaas sorpaas deleted the vorot93/missing-forkid-transitions branch June 1, 2020 15:11
vorot93 added a commit that referenced this pull request Jun 1, 2020
@bobsummerwill
Copy link

Thanks for the speedy fix, @vorot93!

adria0 pushed a commit that referenced this pull request Jun 9, 2020
dvdplm added a commit that referenced this pull request Jun 30, 2020
* master:
  Update parity-crypto dependency (#11791)
  ethcore/private-tcx: fix deadlock caused by conflicting lock order (#11764)
  Feature/publish draft release ci (#11786)
  Trigger custom docker build when adding ci-build-docker to the commit (#11782)
  Fix alpine docker image entry point (#11749)
  Use BigUint::modpow for ModExp precompile (#11772)
  Add YOLO-1 testnet (#11757)
  Upgrade num to 0.2, use enum-primitive-derive (#11636)
  Update Simple Subroutines to the latest spec  (#11731)
  update secret-store reference (#11761)
  Implementation of EIP 2537 (#11707)
  Add missing forks to fork ID (#11747)
  Use nightly tag for master branch (#11732)
dvdplm added a commit that referenced this pull request Jun 30, 2020
…ereum/openethereum into dp/chore/upgrade-to-rocksdb-0.14

* 'dp/chore/upgrade-to-rocksdb-0.14' of github.com:openethereum/openethereum:
  Update parity-crypto dependency (#11791)
  ethcore/private-tcx: fix deadlock caused by conflicting lock order (#11764)
  Feature/publish draft release ci (#11786)
  Trigger custom docker build when adding ci-build-docker to the commit (#11782)
  Fix alpine docker image entry point (#11749)
  Use BigUint::modpow for ModExp precompile (#11772)
  Add YOLO-1 testnet (#11757)
  Upgrade num to 0.2, use enum-primitive-derive (#11636)
  Update Simple Subroutines to the latest spec  (#11731)
  update secret-store reference (#11761)
  Implementation of EIP 2537 (#11707)
  Add missing forks to fork ID (#11747)
  Use nightly tag for master branch (#11732)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Massive mainnet reorganizations of the Ethereum Classic chain after Phoenix hard fork
7 participants