search

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

Uninitialized dif variable cases in canonicalDifficulty() #91

Closed holgerd77 closed 4 years ago

holgerd77 commented 4 years ago

Just wanted to update the blockchain library with the new v2.2.2 block version. Tests are currently breaking in the "mismatched chains" section with:

TAP version 13
# blockchain test
# mismatched chains

/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-blockchain/node_modules/ethereumjs-block/header.js:172
  if (dif.cmp(minimumDifficulty) === -1) {
          ^
TypeError: Cannot read property 'cmp' of undefined
    at module.exports.BlockHeader.canonicalDifficulty (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-blockchain/node_modules/ethereumjs-block/header.js:172:11)
    at Test.<anonymous> (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-blockchain/test/index.ts:888:52)
    at Test.bound [as _cb] (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-blockchain/node_modules/tape/lib/test.js:76:32)
    at Test.run (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-blockchain/node_modules/tape/lib/test.js:95:10)
    at Test.bound [as run] (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-blockchain/node_modules/tape/lib/test.js:76:32)
    at Test._end (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-blockchain/node_modules/tape/lib/test.js:164:11)
    at Test.bound [as _end] (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-blockchain/node_modules/tape/lib/test.js:76:32)
    at Immediate._onImmediate (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-blockchain/node_modules/tape/lib/test.js:117:18)
    at runCallback (timers.js:810:20)
    at tryOnImmediate (timers.js:768:5)

This is caused by a triggered case where dif is not initialized and can be fixed by initializing diff with var dif = BN(0).

This would unfortunately need another v2.2.3 release on the backport side, we should also make sure that this gets applied to master as well, so there might be the need to open two PRs here (one fix + v2.2.3 release PR branched off from the release/v2.2.2 backport branch and one just with the fix (TypeScript-adjusted) towards master.

I will unfortunately not find the time to do this, @evertonfraga: not sure, can you jump in here?

holgerd77 commented 4 years ago

There should be also at least one test confirming the fix being added, so breaking before and passing after.

holgerd77 commented 4 years ago

Analysis here was wrong, this could be solved by an update of the blockchain tests, see https://github.com/ethereumjs/ethereumjs-blockchain/pull/142. Will close.

evertonfraga commented 4 years ago

thanks!