ethereumjs / ethereumjs-block

Project is in active development and has been moved to the EthereumJS VM monorepo.
https://github.com/ethereumjs/ethereumjs-vm/tree/master/packages/block
Mozilla Public License 2.0
42 stars 49 forks source link

Update to ethereumjs-common v1.1.0 #64

Closed danjm closed 5 years ago

danjm commented 5 years ago

This PR updates code and package.json to use ethereumjs-common v1.1.0

coveralls commented 5 years ago

Coverage Status

Coverage remained the same at 73.818% when pulling 9dbb00b429776938c45eb80f5a6388f4932b9965 on update-ethereumjs-common-version into 69c73b13da01beee311f83c6491af4e0278c9372 on master.

danjm commented 5 years ago

@holgerd77 Updated to ^1.1.0

holgerd77 commented 5 years ago

Ok, various difficulty related tests failing, maybe that's an issue introduced in the hardfork lineup in Common along with the Petersburg HF, will also investigate a bit. Eventually we have to do a follow-up bugfix release (I would suspect some HF comparison logic bug?).

holgerd77 commented 5 years ago

Some in-between output, here I check for the hardfork chosen in the BlockHeader.canonicalDifficulty() function as well as how the greatThanEqual Common function evaluates:

Chosen hardfork: constantinople
> Constantinople?: true
> Byzantium?: true
not ok 23313 test canonicalDifficulty (fork determination by block number)
  ---
    operator: equal
    expected: '4841178247367196615'
    actual:   '4841178247367192519'
    at: runDifficultyTests (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-block/tests/difficulty.js:19:7)
    stack: |-
      Error: test canonicalDifficulty (fork determination by block number)
          at Test.assert [as _assert] (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-block/node_modules/tape/lib/test.js:224:54)
          at Test.bound [as _assert] (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-block/node_modules/tape/lib/test.js:76:32)
          at Test.equal (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-block/node_modules/tape/lib/test.js:384:10)
          at Test.bound [as equal] (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-block/node_modules/tape/lib/test.js:76:32)
          at runDifficultyTests (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-block/tests/difficulty.js:19:7)
          at Test.t (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-block/tests/difficulty.js:67:7)
          at Test.bound [as _cb] (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-block/node_modules/tape/lib/test.js:76:32)
          at Test.run (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-block/node_modules/tape/lib/test.js:95:10)
          at Test.bound [as run] (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-block/node_modules/tape/lib/test.js:76:32)
          at Immediate.next (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-block/node_modules/tape/lib/results.js:71:15)
  ...

This looks ok, nevertheless the difficulty if falsely calculated. I suspect the params read from Common subsequently.

holgerd77 commented 5 years ago

Ok, got it: this is failing because we run the difficulty tests on Ropsten in difficulty.js on difficultyRopstenByzantium.json:

const chainTestData = {
    'mainnet': require('./difficultyMainNetwork.json').tests,
    'ropsten': require('./difficultyRopstenByzantium.json').tests
  }

Since the new Common release also defines a hardfork number for Constantinople for Ropsten and block numbers seems to be above the threshold, difficulty is calculated within the Constantinople context.

If test file is changed to difficultyRopstenConstantinople.json (file already included in the tests folder), tests pass again.

Can you update this here?

danjm commented 5 years ago

@holgerd77 Thanks for the investigation on the test failures. I've made the changes and tests are now passing.

holgerd77 commented 5 years ago

Ok, will directly merge and prepare a release.