Fix failing blockchain tests #338
Comments
|
Blockchain tests are now running here: #341 |
|
On-top-note: we are running low on CircleCI usage capacity (at 91%). If we want to keep the CircleCI switch (which I would assume regarding the advantages) we would have to upgrade rather sooner than later. Think spending an additional 50-100 $ here per month should be in the budget. |
|
Started with sending a mail to the CircleCI support if we qualify for the OSS package. |
|
Update on CircleCI usage capacity (cc @axic): had some very nice support exchange with Circle, OSS package is project/repo-wise. Seems that we are already on the OSS package with the VM, but it is not displayed on Circle (bug!), the capacity message seems to be another (sigh 😄) bug in the Circle UI and this should not apply for us and we should reach back if we have problems. |
|
First note on this: Many failures preceed the following error stack output: Invalid block error: Error: Error: Error: invalid receiptTrie invalid gasUsed invalid block stateRoot
at /Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/dist/runBlock.js:159:19
at /Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/dist/cache.js:106:7
at /Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:473:16
at replenish (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:1006:25)
at /Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:1016:9
at eachLimit$1 (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:3196:24)
at Object.<anonymous> (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:1046:16)
at /Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/dist/cache.js:102:11
at /Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:969:16
at next (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:5225:18)
at /Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/dist/cache.js:99:7
at next (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:5223:28)
at /Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/dist/cache.js:99:7
at next (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:5223:28)
at /Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/dist/cache.js:94:9
at /Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/merkle-patricia-tree/util.js:51:36
not ok 571 last block hash
---
operator: equal
expected: |-
'2bda3f5684b144a955a28a1ffdc2dae25e51603f685aae03d0fcc885184d568f'
actual: |-
'f7666415a06fe86f39a614be3e707683426ca0cec5a92ccce6664bebbe2ba55c'
at: self.blockDb.get (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/ethereumjs-blockchain/index.js:331:7) |
|
Same level as before (66 failing) also with updated blockchain and block libraries. |
|
Below is a script that can tell us the last commit at which as test was passing and the commit at which it started failing. The code is at bottom. Example: the output for the script when running
Once the failing blockchain tests are the top priority, this script can be expanded to iterate over all of the files listed above (#338 (comment)) and output a summary of the commits at which each test started failing. Which should be a good starting point for debugging.
|
|
Hi @danjm, sorry I didn't have the time to have a look earlier. That is really great, I also already had some similar script idea but wasn't able to realize this with an appropriate amount of effort. Will be hopefully very helpful to get a grip on the blockchain tests. |
|
@holgerd77 I ran the tests across all files you listed above and formatted the results a bit for readability. The result is at the end of this comment. Importantly, searching for unique commit hashes shows that there are only five commits responsible for the failures: We can then find the test names associated with each of these commits. For instance, 914dfd6 causes the following tests to fail: 'RPC_API_Test', 'randomStatetest224BC', 'randomStatetest234BC', 'randomStatetest529BC', 'ExtraData32', 'log1_correct', 'timeDiff0', 'timeDiff12', 'timeDiff13', 'timeDiff14', 'wallet2outOf3txs2', 'wallet2outOf3txsRevoke', 'wallet2outOf3txsRevokeAndConfirmAgain', 'walletReorganizeOwners', Here is the full report |
|
28 out of the 72 test failures are from this PR: |
|
Hmm, yes, this probably make sense. Could you if you take on this make this context very clear in the comments on the skipped tests? |
|
Here is a more useful summary, where each failing test has been grouped with all other tests that started failing on the same commit, along with the url for the PR where the respective commit was merged. |
|
I propose that we treat the resolution of these tests as five separate tasks:
I've started investigating the first group of these. Unless claimed by others, I will attempt to work through all of them this week. cc @holgerd77 |
|
Hi @danjm, thanks for such a deep analysis. Be careful a bit, the latest large amount of failing blockchain tests (see this PR #341, over 300) is largely induced by a new parameter Nevertheless the change points you identified above mostly look like things where stuff could have been broken (on a first look and just judging the nature of changes), so probably most of the stuff is worth being investigated. |
|
@holgerd77 You were right to caution me about the Is there any reason we don't release In other news, I am narrowing in on the cause of the failures of #147. It seems that the code required for the account deletion/cleanup of EIP158 (https://github.com/ethereumjs/ethereumjs-vm/blob/master/lib/runTx.js#L190) is inadvertently causing invalid state roots at the point of flushing the state managers cash (https://github.com/ethereumjs/ethereumjs-vm/blob/master/lib/runBlock.js#L172). I will continue investigating tomorrow. |
|
Hi @danjm, |
|
Blockchain tests are now run by default, see #374. This should make it easier to proceed with this. |
|
I've submitted a PR (#381 ) that fixes the following tests:
This is mostly the set that started with #184 set (the ones I was actually looking at) but has also fixed some from the set starting at #168. The failing test from #197 is also fixed by #380 |
|
Last 20 failing tests (state after merge of #329):
|
|
I've opened a do-not-merge PR #384 which fixes the remaining blockchain tests but has some issues of it's own. Basically the change in bloom filter implementation from #295 has been hiding missing validation of the block. The following blockchain tests have therefore been passing when they shouldn't have been:
Not sure what the best approach is here. My current thought is that it involves leaving the bloom filter as per #295, skip the last couple of test failures caused by this and then write our own tests to ensure the block validation is happening correctly? My hesitation is caused by the scope of the failing tests. Some of these really look like they should be running. |
|
Tada. Done. 😄 Thanks everyone! Special thanks also again to @danjm, we couldn't use everything of your work, but your write-up here was really outstanding! Should we integrate your ruby script into the |
and others
Just ran the blockchain tests after a long time (see also associated issue #337 on a revived BlockchainTests test run strategy).
Currently 66 tests are failing:
I will list all here so we have a basis to fix without the need (for the initial fixes) to re-run the tests again and again:
Test runs on the listed files can be triggered with
node tests/tester.js -b --file='blockhashTests'.Extra note: I think atm blockchain tests are not updated to take the HF setting into account and explicitly instantiate the VM with it (like on the state tests). Atm this shouldn't be a (the) problem, but this should be updated anyhow.
The text was updated successfully, but these errors were encountered: