-
Notifications
You must be signed in to change notification settings - Fork 49
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make sense to directly update this to v1.1.0 (so ^1.1.0
(once released), then the library will be more safe to be used in a Petersburg
context and we can directly state this in the subsequent release notes here.
@danjm If you want you can directly update to safe time. I can then re-trigger CI once the new Common
version is released.
ca01e95
to
7fc7200
Compare
@holgerd77 Updated to |
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?). |
Some in-between output, here I check for the hardfork chosen in the
This looks ok, nevertheless the difficulty if falsely calculated. I suspect the params read from Common subsequently. |
Ok, got it: this is failing because we run the difficulty tests on const chainTestData = {
'mainnet': require('./difficultyMainNetwork.json').tests,
'ropsten': require('./difficultyRopstenByzantium.json').tests
} Since the new If test file is changed to Can you update this here? |
7fc7200
to
9dbb00b
Compare
@holgerd77 Thanks for the investigation on the test failures. I've made the changes and tests are now passing. |
Ok, will directly merge and prepare a release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
This PR updates code and package.json to use ethereumjs-common v1.1.0