Chain setup API / Centralize default HF / Common options dict / Block HFbyBlockNumber & initWithGenesisHeader options

[holgerd77]

This PR prepares for the rework of EIP support as a follow-up on #856

I moved Common to be instantiated with an option dictionary - like we do in most of the other libraries too - should be a good occasion to do this now since we are right before a breaking release anyhow.

This prepares for a new to be added eips option, things would otherwise slowly getting too ugly with all the optional parameters chained. It also makes the library more future proof by preparing the ground for future non-breaking option additions.

First push here to see if all tests in related consuming libraries pass.

[codecov]

Codecov Report

Merging #863 into master will decrease coverage by 0.14%.
The diff coverage is 84.61%.

Flag	Coverage Δ
#account	92.85% <ø> (ø)
#block	77.63% <83.33%> (-0.53%)	⬇️
#blockchain	81.10% <71.42%> (-0.24%)	⬇️
#common	94.58% <100.00%> (+0.08%)	⬆️
#ethash	83.33% <ø> (ø)
#tx	94.11% <50.00%> (+0.08%)	⬆️
#vm	82.19% <66.66%> (-0.09%)	⬇️

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

[holgerd77]

Ok, this got a bit out of hand and one thing lead to another. 😄 I will repurpose the target of this PR to be just preparatory work, otherwise things would get too hard too review, hope I am not yet too much out of the scope here. Nevertheless once tests pass here this would be open for review, you should likely do commit-by-commit for a better overview, the single commits are relatively well structured.

This PR addresses a lot of issues we had discussed lately with the Common library and touches also some other issues:

• Already mentioned above: Common switches to an options dictionary for instantiation, 14bc686
• The chain and hardfork constructor options have been removed on all libraries and passing in a Common instance is now the single way to adopt the chain setup, see c1bf90e and follow-up commits. This was raised several times on chat and calls and simplifies constructor APIs and is a direct preparation for the eips option addition since this would spin-up a too-complex-to-handle possibility matrix in constructors regarding the chain setup
• The default HF setting is now centralized in Common and static references in consuming libraries (VM, tx, block, blockchain) have been removed 2ba5701, this solves Use constant for default hardfork #822
• There is a new Common.setHardforkByBlockNumber() method. This is used by the Block library to allow for a new hardforkByBlockNumber option for blocks and headers which trigger a HF adoption by the block number set to the block, this solves the tx initialization problems we had along 116ea78, see Tx initialization on the block library crashes on initialization with only chain set on Common #757. Since there were some immutability discussion (which we won't completely solve on this first round) along I also made the setGenesisParams() function private and switched to a constructor option initWithGenesisHeader to initialize a genesis header 623dbe0, which was one of the major inconsistency creators as one step towards an immutable Block library and also added a "Please add header data only along instantiation" warning in the constructor docs.

Sorry that this got so complex but actually all of the changes from above got triggered by follow-up demands of previous changes so that things got consistent again. Hope it was worth the effort and we get this merged. 😀 If you see improvements to be done feel free - with a short note on Discord maybe - to directly work on this and push here.

[jochem-brouwer]

Wow this is a heavy one - but the description of these changes sound all super good to me! I don't think it is efficient if I start looking at why tests fail at this point but let me know if I can help over there. I will quickly glance over this code and comment on it.

[holgerd77]

@jochem-brouwer thanks! 😄 I think one think which would be the most effective would be if you do a commit-by-commit review - I won't do any rebases here any more or something and this will remain stable - and if you then explicitly confirm which commits have been reviewed.

[holgerd77]

@jochem-brouwer pushed again and fixed (hopefully, we'll see on CI) VM API tests failing. Selected state tests are still failing if you want to have a look there once time permits that would be extremely helpful.

[jochem-brouwer]

State tests fail to run:

Error: Chain/hardfork options are not allowed any more on initialization
    at new Transaction (/home/runner/work/ethereumjs-vm/ethereumjs-vm/packages/tx/src/transaction.ts:75:13)
    at exports.makeTx (/home/runner/work/ethereumjs-vm/ethereumjs-vm/packages/vm/tests/util.js:92:12)
    at runTestCase (/home/runner/work/ethereumjs-vm/ethereumjs-vm/packages/vm/tests/GeneralStateTestsRunner.js:61:12)
    at runStateTest (/home/runner/work/ethereumjs-vm/ethereumjs-vm/packages/vm/tests/GeneralStateTestsRunner.js:133:7)
    at /home/runner/work/ethereumjs-vm/ethereumjs-vm/packages/vm/tests/tester.js:112:13
    at fileCallback (/home/runner/work/ethereumjs-vm/ethereumjs-vm/packages/vm/tests/testLoader.js:56:11)

Looks like the state test runner still uses the chain/hf options in the constructor.

[jochem-brouwer]

Looked over the commits, have to do a more thorough review later.

[holgerd77]

Rebased this, had a final more distant look and I think I am now confident on all changes. Did some last fixes and modifications - with the most notable being DEFAULT_HARDFORK moved inside the Common class as a readonly param to be accessible from a Common instance.

Tests are now passing, the block tests can be safely ignored, these randomly pass (e.g. on the run before) or fail. This will likely be fixed along #861

So this should be ready to review now. 😄

[ryanio]

love it, great PR and nice improvements!

i saw updates to the changelogs for blockchain and vm, but not for:

• block
  • constructor opts
  • new opts: hardforkByBlockNumber, initwithGenesisHeader
• tx
  • constructor opts (i made a note and can include this in my tx refactor pr since I set up the unreleased changelog notes in there)
• common
  • setHardforkForBlockNumber

This PR is big enough as it is so I will merge in and we can handle the above separately :)

(also need to regenerate the docs, but that can be done closer to v5 release prep)