Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run blockchain tests #341

Closed
wants to merge 1 commit into from
Closed

Run blockchain tests #341

wants to merge 1 commit into from

Conversation

holgerd77
Copy link
Member

This is now finally the usage of the mechanism running the blockchain tests on CircleCI by renaming the config files.

@ethereumjs ethereumjs deleted a comment Aug 27, 2018
@ethereumjs ethereumjs deleted a comment from jwasinger Aug 27, 2018
@holgerd77 holgerd77 force-pushed the run-blockchain-tests branch 2 times, most recently from 1884222 to 3b96373 Compare August 29, 2018 13:44
@holgerd77 holgerd77 changed the title [DO-NOT-MERGE] [TEST-RUN-PR] Run blockchain tests [DO-NOT-MERGE] Run blockchain tests Sep 20, 2018
@holgerd77 holgerd77 changed the title [DO-NOT-MERGE] Run blockchain tests Run blockchain tests Sep 20, 2018
@holgerd77
Copy link
Member Author

Have retriggered a test run on VM e24909e latest commit on master, ethereumjs-testing v1.2.4.

@holgerd77
Copy link
Member Author

Result blockchain tests:

1..848
# tests 848
# pass  526
# fail  322

Too much.

@holgerd77
Copy link
Member Author

Many of the failures above seem difficulty related. I tested:

node ./tests/tester.js -b --test='notxs_Byzantium'

This fails with Error: invalid POW, thrown from within the blockchain class and can be made explicit (normally hidden) by adding console.log(err) in the BlockchainTestRunner.js test class after blockchain.putBlock() (respectively in the callback).

This doesn't necessarily mean that the latest updates from the blockchain or block classes have introduced an error. There were lately various discussions about wrong difficulty values in the blockchain tests on tests Gitter, maybe these test cases are wrong (one could compare with an older snapshot blockchain test from ethereumjs-testing). I have also tested directly within ethereumjs-block in the node_modules folder, byzantium hardfork setting is passed correctly and the difficulty tests are all passing if one goes to the repo and checks.

@holgerd77
Copy link
Member Author

Have investigated this further, this fails in ethashjs in Ethash.prototype._verifyPOW in the mixHash comparison, a.mix.toString('hex') === header.mixHash.toString('hex') evaluates to false, with the following values:

1639a35bab884e5d9b095bd860d6f776768b9be157275792a758e1e3909308e0
0000000000000000000000000000000000000000000000000000000000000000

The second one is coming from the mixHash field in the blockchain tests from ethereum/tests repo. I cross-checked an older version, before it had the correct hash, so this is definitely an error over on the test repo. Will file an issue there.

@holgerd77
Copy link
Member Author

Have filed an issue here: ethereum/tests#530

@holgerd77
Copy link
Member Author

Retriggered a test run after merging of #373, also to get an impression about changes in blockchain test run time after introduction of sealEngine usage (things should speed up).

@holgerd77
Copy link
Member Author

Ok, this is now back to "normal" 60 failing tests (actually 6 less than before), see also #338 for comparison and context, some results:

1..848
# tests 848
# pass  788
# fail  60

Execution speed of test run on CircleCI is now down to a whopping 12:39 min, which is fantastic since this means we can now (after getting test failures down) run this on every PR CI run!!! ✨ 😄

@holgerd77
Copy link
Member Author

Now obsolete to run periodic blockchain test runs through merging of #374.

@holgerd77 holgerd77 closed this Oct 31, 2018
@holgerd77 holgerd77 deleted the run-blockchain-tests branch October 31, 2018 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants