[VM] fix blockchain tests and add Homestead support #815

jochem-brouwer · 2020-07-15T19:21:32Z

Closes #813. Closes #824.

Note: I have put more forks in the PR CI file.

codecov · 2020-07-15T19:22:12Z

Codecov Report

Merging #815 into master will decrease coverage by 0.31%.
The diff coverage is 58.62%.

Flag	Coverage Δ
#account	`92.85% <ø> (ø)`
#block	`79.11% <ø> (ø)`
#blockchain	`82.88% <ø> (ø)`
#common	`94.14% <ø> (ø)`
#ethash	`81.93% <0.00%> (+0.55%)`	⬆️
#tx	`94.02% <ø> (ø)`
#vm	`92.35% <60.71%> (-0.56%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

jochem-brouwer · 2020-07-15T19:25:36Z

Right this is bad.

TangerineWhistle tests are not passing at all. Byzantium+ seems to locally pass (haven't ran all tests though but it seems like those forks work).

TangerineWhistle:

1..3745
# tests 3745
# pass  2530
# fail  1215

SpuriousDragon:

1..3769
# tests 3769
# pass  2546
# fail  1223

My gut says it is because of Embedding Transaction Status in Receipt Trie (introduced in Byzantium). VM also has a note about this.

jochem-brouwer · 2020-07-15T21:30:47Z

If I generate the state root and put that state root in the receipt then the tests pass for Spurious Dragon and for Tangerine Whistle 1 test fails. Failing test: OOGStateCopyContainingDeletedContract

We thus need a way to keep the checkpoint/revert/commit functionality of StateManager, but we also need to be able to generate the state root if we have not yet committed this. I can think of a rather dirty way to do this; we add a method to getStateRootTemp to StateManager which flushes the cache to the trie, but keeps the original values in memory. It thus temporarily commits the state to the trie, gets the state root and then reverts all changes. This is essentially a checkpointing mechanism on top of the checkpointing mechanism, it is dirty, but it will do the job. Thoughts?

holgerd77 · 2020-07-16T12:03:04Z

packages/vm/tests/BlockchainTestsRunner.js

+    t.comment("skipping test: no data available for " + options.forkConfigTestSuite)
+    return
+  }
+


This is too late in the process, false forks should be already filtered out when the test files are read (currently the code logic in ethereumjs-util) and there should be no output on this, this is otherwise causing too much unnecessary noise.

Will do a follow-up comment here: #808

I see where you are getting from, but this is not the case for state tests either: files are read and are then filtered out (this is thus not done in the ethereumjs-testing module, but actually in the state test runner which we define in the VM package).

So this is inconsistent. Do you think we should move this check to ethereumjs-testing? (Such that the runner never skips any tests?). I see in #808 you have moved all the testing logic in the VM package and I think this is a good idea; we can then move these fork checks to a more 'natural' place (i.e. the tests filter) and then let the runner only do anything if it actually has the right data to test.

Ok, will see this as resolved for now.

jochem-brouwer · 2020-07-30T10:04:25Z

Will push some commits soon which has #814 merged on top of it.

This changes getStateRoot() of StateManager to getStateManager(force: boolean), where force is a parameter which forces the cache to be flushed even if there are uncommited checkpoints. This is necessary to get the intermediate state roots pre-Byzantium. This has the side-effect that if there is an internal VM error which rolls back the block, the state trie (MPT) now has values in them which should not be there (as they do not seem to be rolled back if the cache is reverted). This might be an undesirable side-effect. Idea proposed by @s1na (thanks! 😄 ) after a discussion on discord.

jochem-brouwer · 2020-07-30T10:15:00Z

npm run build && npm run test:blockchain -- --fork=TangerineWhistle

1..2568
# tests 2568
# pass  2568

npm run build && npm run test:blockchain -- --fork=SpuriousDragon

1..2583
# tests 2583
# pass  2583

jochem-brouwer · 2020-07-31T09:42:30Z

Homestead:

1..4370
# tests 4370
# pass  4370

This does need the ethereumjs/ethereumjs-util#269 PR merged though, otherwise randomStatetest94 fails. Will push asap.

jochem-brouwer · 2020-07-31T10:21:04Z

packages/vm/lib/evm/opFns.ts

+      runState.eei.useGas(new BN(runState._common.param('gasPrices', 'callNewAccount')))
+    }
+
+    gasLimit = maxCallGas(


Note that maxCallGas is called after we have deducted all the call gas (this was not done before, callNewAccount could be deducted after maxCallGas). This was a difference from geth, so this might actually fix some consensus issues if we would do a mainnet sync (although it is very strange that no state test has found this!). The issue is thus that the forwarded gas between geth/EthereumJS might be different.

jochem-brouwer · 2020-07-31T11:13:36Z

Blockchain tests, local:

Homestead
1..4370
# tests 4370
# pass  4370

TangerineWhistle
1..2562
# tests 2562
# pass  2562

SpuriousDragon
1..2577
# tests 2577
# pass  2577

Byzantium
1..9958
# tests 9958
# pass  9958

Constantinople
1..21554
# tests 21554
# pass  21554

Petersburg
1..21544
# tests 21544
# pass  21544

Istanbul
1..23665
# tests 23665
# pass  23665

MuirGlacier
1..23665
# tests 23665
# pass  23665

holgerd77 · 2020-08-06T16:50:19Z

What is missing on this on this PR? Can this be updated easily? Would have some time for a review later during the evening.

jochem-brouwer · 2020-08-06T16:55:37Z

@holgerd77 will update!

jochem-brouwer · 2020-08-06T19:22:04Z

@holgerd77 I spent more time on this than I want to admit 😅 But it works! Note that in the tests all forks supported forks are added as state test; Homestead is also checked in the blockchain test.

The tests are still a bit ugly (but they do work now for older forks). I could make them a bit more prettier but I think that should be done in a new PR.

@evertonfraga could you specifically review the CI? 😄

EDIT: also note the state tests pass for all supported forks! 😄

holgerd77

Didn't got so deep into opFns.ts yet, some comments to address.

PR also needs a conflict resolve.

holgerd77 · 2020-08-10T20:34:06Z

packages/vm/lib/evm/memory.ts

    }
-    return Buffer.from(loaded)
+
+    return returnBuffer


Just for my understanding, since I've read through both code versions a couple of times now and would assume that both would work: what has been changed/fixed here?

There is a KECCAK test in the Homestead suite which requires one to hash ~1.5GB of data (randomStateTest94). The old approach uses too much memory (I assume because of how JS-arrays are implemented; one keeps pushing values into this array, thus expanding it). It runs out of memory on my pc (not sure if it also runs OOM on the CI) which has 16GB of RAM (also tested it with bumping the amount of RAM node can access).

The issue is thus not that this changes any functionality; this however uses less memory as the new approach does not go OOM on my pc.

Ah, nice explanation, thanks!

(and also cool "digging-deep")

holgerd77 · 2020-08-10T20:38:05Z

packages/vm/lib/state/stateManager.ts

-  async getStateRoot(): Promise<Buffer> {
-    if (this._checkpointCount !== 0) {
+  async getStateRoot(force: boolean = false): Promise<Buffer> {
+    if (!force && this._checkpointCount !== 0) {
      throw new Error('Cannot get state root with uncommitted checkpoints')
    }
    await this._cache.flush()


Ok, looks good.

holgerd77 · 2020-08-10T20:40:56Z

packages/vm/lib/state/interface.ts

@@ -22,7 +22,7 @@ export interface StateManager {
  checkpoint(): Promise<void>
  commit(): Promise<void>
  revert(): Promise<void>
-  getStateRoot(): Promise<Buffer>
+  getStateRoot(force: boolean): Promise<Buffer>


Correct me if I am wrong, but I think force should be marked as optional in the interface, since there is a default value in the implementation.

I can try this, I tried using a default value in the interface (false) but TypeScript does not support this. Marking it optional in the interface sounds like a good idea, I think that might work, but I don't know if TypeScript supports these optional values in the interface. Will try.

holgerd77 · 2020-08-10T20:47:33Z

packages/vm/package.json

    "test:state:slow": "npm run test:state -- --runSkipped=slow",
    "test:buildIntegrity": "npm run test:state -- --test='stackOverflow'",
    "test:blockchain": "node -r ts-node/register --stack-size=1500 ./tests/tester --blockchain",
-    "test:API": "tape -r ts-node/register --stack-size=1500 './tests/api/**/*.js'",
-    "test:API:browser": "karma start karma.conf.js",
+    "test:blockchain:allForks": "node -r ts-node/register --stack-size=1500 ./tests/tester --blockchain --fork=Homestead && node -r ts-node/register --stack-size=1500 ./tests/tester --blockchain --fork=TangerineWhistle && node -r ts-node/register --stack-size=1500 ./tests/tester --blockchain --fork=SpuriousDragon && node -r ts-node/register --stack-size=1500 ./tests/tester --blockchain --fork=Byzantium && node -r ts-node/register --stack-size=1500 ./tests/tester --blockchain --fork=Constantinople && node -r ts-node/register --stack-size=1500 ./tests/tester --blockchain --fork=Petersburg && node -r ts-node/register --stack-size=1500 ./tests/tester --blockchain --fork=Istanbul && node -r ts-node/register --stack-size=1500 ./tests/tester --blockchain --fork=MuirGlacier",


Wonder if this can be simplified with something build upon something like echo {'Istanbul','Byzantium'} | xargs -n1 | xargs -I v1 echo v1, see this StackExchange question for some inspiration.

holgerd77 · 2020-08-10T20:48:15Z

packages/vm/package.json

@@ -14,13 +14,14 @@
    "coverage:test": "tape './tests/api/**/*.js' ./tests/tester.js --state --dist",
    "docs:build": "typedoc --options typedoc.js",
    "test:state": "ts-node ./tests/tester --state",
-    "test:state:allForks": "npm run test:state -- --fork=TangerineWhistle && npm run test:state -- --fork=SpuriousDragon && npm run test:state -- --fork=Byzantium && npm run test:state -- --fork=Constantinople && npm run test:state -- --fork=Petersburg && npm run test:state -- --fork=Istanbul && npm run test:state -- --fork=MuirGlacier",
-    "test:state:selectedForks": "npm run test:state -- --fork=Petersburg && npm run test:state -- --fork=TangerineWhistle",
+    "test:state:allForks": "npm run test:state -- --fork=Homestead && npm run test:state -- --fork=TangerineWhistle && npm run test:state -- --fork=SpuriousDragon && npm run test:state -- --fork=Byzantium && npm run test:state -- --fork=Constantinople && npm run test:state -- --fork=Petersburg && npm run test:state -- --fork=Istanbul && npm run test:state -- --fork=MuirGlacier",


Similar suggestion as for blockchain:allForks

holgerd77 · 2020-08-10T20:49:06Z

packages/vm/tests/BlockchainTestsRunner.js

+    t.comment("skipping test: no data available for " + options.forkConfigTestSuite)
+    return
+  }
+


Ok, will see this as resolved for now.

packages/vm/lib/evm/opFns.ts

[VM] fix blockchain tests

[VM] TangerineWhistle: fix tests [VM] remove obsolete test [VM] remove allforks test from default workflow (it is still in nightly tests)

[VM] Pre-TangerineWhistle: make too much gas forwarded in calls go OOG [VM] lint [VM] update package scripts [VM] fix workflow [VM] include Homestead in PR test runner [VM] fix package-lock

[VM] change docs

jochem-brouwer · 2020-08-12T13:07:06Z

Looks like the rebase introduced an error.

[Ethash] lint

jochem-brouwer · 2020-08-12T14:52:27Z

Okay, this should be fixed now (Ethash had an unhandled rejection in a Promise).

Ready for re-review @holgerd77

holgerd77 · 2020-08-12T14:54:22Z

@jochem-brouwer ah, great, will start in 10 min. 😛

holgerd77

This looks good now, also checked the test run results (so: check that both blockchain and state tests are actually executed), thanks Jochem! 😄

Will merge.

packages/vm/lib/evm/opFns.ts

holgerd77 · 2020-08-12T15:15:02Z

packages/vm/package.json

-    "test:state:allForks": "npm run test:state -- --fork=TangerineWhistle && npm run test:state -- --fork=SpuriousDragon && npm run test:state -- --fork=Byzantium && npm run test:state -- --fork=Constantinople && npm run test:state -- --fork=Petersburg && npm run test:state -- --fork=Istanbul && npm run test:state -- --fork=MuirGlacier",
-    "test:state:selectedForks": "npm run test:state -- --fork=Petersburg && npm run test:state -- --fork=TangerineWhistle",
+    "test:state:allForks": "echo {'Homestead','TangerineWhistle','SpuriousDragon','Byzantium','Constantinople','Petersburg','Istanbul','MuirGlacier'} | xargs -n1 | xargs -I v1 npm run test:state -- --fork=v1",
+    "test:state:selectedForks": "echo {'Homestead','TangerineWhistle','SpuriousDragon','Petersburg',} | xargs -n1 | xargs -I v1 npm run test:state -- --fork=v1",


holgerd77 · 2020-08-12T15:20:30Z

packages/vm/lib/evm/opFns.ts

@@ -713,7 +718,11 @@ export const handlers: { [k: string]: OpHandler } = {
    if (!value.isZero()) {
      runState.eei.useGas(new BN(runState._common.param('gasPrices', 'callValueTransfer')))
    }
-    gasLimit = maxCallGas(gasLimit, runState.eei.getGasLeft())
+    gasLimit = maxCallGas(gasLimit, runState.eei.getGasLeft(), runState)
+    // note that TW or later this cannot happen (it could have ran out of gas prior to getting here though)


Won't make this a blocker either, but such abbreviations like TW are suboptimal since hard to read (I didn't make it at all to "Tangerine Whistle" on first round), better to fully write stuff like this down.

Good point, will keep it in mind 👍

jochem-brouwer added the PR state: needs review label Jul 15, 2020

jochem-brouwer added the PR state: open discussion label Jul 15, 2020

holgerd77 reviewed Jul 16, 2020

View reviewed changes

jochem-brouwer mentioned this pull request Jul 29, 2020

Integrate ethereumjs testing logic #808

Merged

jochem-brouwer added PR state: WIP and removed PR state: needs review labels Jul 30, 2020

jochem-brouwer marked this pull request as draft July 30, 2020 06:56

jochem-brouwer mentioned this pull request Jul 31, 2020

Fix large string copies ethereumjs/ethereumjs-util#269

Merged

jochem-brouwer changed the title ~~[VM] fix blockchain tests~~ [VM] fix blockchain tests and add Homestead support Jul 31, 2020

jochem-brouwer mentioned this pull request Jul 31, 2020

Backporting old HF rules #652

Closed

5 tasks

jochem-brouwer commented Jul 31, 2020

View reviewed changes

This was referenced Jul 31, 2020

Add frontier support #828

Merged

New release v1.3.3 ethereumjs/ethereumjs-testing#49

Merged

jochem-brouwer force-pushed the fix-blockchain-tests branch 5 times, most recently from 17e0b70 to b303a21 Compare August 6, 2020 19:08

jochem-brouwer marked this pull request as ready for review August 6, 2020 19:22

jochem-brouwer removed the PR state: WIP label Aug 6, 2020

jochem-brouwer added PR state: needs review package: vm labels Aug 6, 2020

holgerd77 mentioned this pull request Aug 9, 2020

VM v5 Release Planning #681

Closed

27 tasks

jochem-brouwer requested review from evertonfraga, holgerd77, ryanio and s1na August 10, 2020 18:52

holgerd77 requested changes Aug 10, 2020

View reviewed changes

jochem-brouwer force-pushed the fix-blockchain-tests branch from 582ce78 to bc4f199 Compare August 12, 2020 12:41

jochem-brouwer added 4 commits August 12, 2020 14:42

[VM] add tangerineWhistle to state all forks test

fe80cc9

[VM] fix blockchain tests

[VM] fix account exists if account was previously deleted in cache

0c88b64

[VM] TangerineWhistle: fix tests [VM] remove obsolete test [VM] remove allforks test from default workflow (it is still in nightly tests)

[VM] add homestead support; fix very big KECCAK operations

2c7c345

[VM] Pre-TangerineWhistle: make too much gas forwarded in calls go OOG [VM] lint [VM] update package scripts [VM] fix workflow [VM] include Homestead in PR test runner [VM] fix package-lock

[VM] address requested changes

8546612

[VM] change docs

jochem-brouwer force-pushed the fix-blockchain-tests branch from bc4f199 to 8546612 Compare August 12, 2020 12:48

[Ethash] fix unhandled promise rejection

fccea35

[Ethash] lint

jochem-brouwer force-pushed the fix-blockchain-tests branch from 655767d to fccea35 Compare August 12, 2020 14:42

holgerd77 mentioned this pull request Aug 12, 2020

Implement EIP-2315: subroutines #754

Merged

holgerd77 approved these changes Aug 12, 2020

View reviewed changes

holgerd77 merged commit e0c56b2 into master Aug 12, 2020

holgerd77 deleted the fix-blockchain-tests branch August 12, 2020 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VM] fix blockchain tests and add Homestead support #815

[VM] fix blockchain tests and add Homestead support #815

jochem-brouwer commented Jul 15, 2020 •

edited

Loading

codecov bot commented Jul 15, 2020 •

edited

Loading

jochem-brouwer commented Jul 15, 2020 •

edited

Loading

jochem-brouwer commented Jul 15, 2020 •

edited

Loading

holgerd77 Jul 16, 2020

jochem-brouwer Jul 29, 2020

holgerd77 Aug 10, 2020

jochem-brouwer commented Jul 30, 2020 •

edited

Loading

jochem-brouwer commented Jul 30, 2020 •

edited

Loading

jochem-brouwer commented Jul 31, 2020 •

edited

Loading

jochem-brouwer Jul 31, 2020

jochem-brouwer commented Jul 31, 2020

holgerd77 commented Aug 6, 2020

jochem-brouwer commented Aug 6, 2020

jochem-brouwer commented Aug 6, 2020 •

edited

Loading

holgerd77 left a comment

holgerd77 Aug 10, 2020

jochem-brouwer Aug 10, 2020

holgerd77 Aug 11, 2020

holgerd77 Aug 10, 2020

holgerd77 Aug 10, 2020

jochem-brouwer Aug 10, 2020

holgerd77 Aug 10, 2020

holgerd77 Aug 10, 2020

holgerd77 Aug 10, 2020

jochem-brouwer commented Aug 12, 2020

jochem-brouwer commented Aug 12, 2020

holgerd77 commented Aug 12, 2020

holgerd77 left a comment

holgerd77 Aug 12, 2020

holgerd77 Aug 12, 2020

jochem-brouwer Aug 12, 2020

[VM] fix blockchain tests and add Homestead support #815

[VM] fix blockchain tests and add Homestead support #815

Conversation

jochem-brouwer commented Jul 15, 2020 • edited Loading

codecov bot commented Jul 15, 2020 • edited Loading

Codecov Report

jochem-brouwer commented Jul 15, 2020 • edited Loading

jochem-brouwer commented Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jochem-brouwer commented Jul 30, 2020 • edited Loading

jochem-brouwer commented Jul 30, 2020 • edited Loading

jochem-brouwer commented Jul 31, 2020 • edited Loading

Choose a reason for hiding this comment

jochem-brouwer commented Jul 31, 2020

holgerd77 commented Aug 6, 2020

jochem-brouwer commented Aug 6, 2020

jochem-brouwer commented Aug 6, 2020 • edited Loading

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jochem-brouwer commented Aug 12, 2020

jochem-brouwer commented Aug 12, 2020

holgerd77 commented Aug 12, 2020

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jochem-brouwer commented Jul 15, 2020 •

edited

Loading

codecov bot commented Jul 15, 2020 •

edited

Loading

jochem-brouwer commented Jul 15, 2020 •

edited

Loading

jochem-brouwer commented Jul 15, 2020 •

edited

Loading

jochem-brouwer commented Jul 30, 2020 •

edited

Loading

jochem-brouwer commented Jul 30, 2020 •

edited

Loading

jochem-brouwer commented Jul 31, 2020 •

edited

Loading

jochem-brouwer commented Aug 6, 2020 •

edited

Loading