Failing blockchain tests now emit a state diff

[lithp]

Intended to address #1562

- Instead of failing with "these block hashes do not match", blockchain
  tests now fail with "this part of the state tree does not match".

- The tests were inefficient, they first checked that the state hash
  matched and then checked that the states matched. It's unlikely the
  states will ever not match!

- Some validations needed to be deferred until after state-tree
  comparison, or else they would fail the test early with an unhelpful
  message.

What was wrong?

Failures used to look like this:

E       eth_utils.exceptions.ValidationError: Mismatch between block and mined block on 3 fields:
E        - header.state_root  :
E           (actual)  : b"R\xafB',,\xd3\xcb\x8b#W\x98)wL\xc4fLm\x90m\x08\xbb\xed\xc0\x03\x8e\x7f\xfcO:X"
E           (expected): b"pv\xa0\x01\xd5\xc2\xab]\x94\xdbj\xcac\x17\x84\x1fW2QZ\xf7\x1f>:\x00\xb3\x8f\xe2'Sy\xfc"
E        - header.receipt_root:
E           (actual)  : b'\n\x106\x18\xe1\xe1)0\xf1%\xef2\x94\xd9\x0fQ\xed\x12\x1d+\x19\xae\xd7\t\xcf\x92\x8c4]\xd9zy'
E           (expected): b'\x0e\xf4\xbaL\x0be\xce\xcau\xb1\xb6~i\xf5\xaf\xb1\xabd\xe7_~\x96!`m\xe5\x17M\xce\x9eD\x05'
E        - header.gas_used    :
E           (actual)  : 128058
E           (expected): 113058

And now they look like this:

E           AssertionError: State DB did not match expected state on 3 values:
E           0x1000000000000000000000000000000000000000(storage[1]) | Actual: 859287 | Expected: 849245
E            - 0x2adc25665018aa1fe0e6bc666dac8fc2697ff9ba(balance) | Actual: 2000000000000160716 | Expected: 2000000000000170758 | Delta: 10042
E            - 0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b(balance) | Actual: 999999999999739284 | Expected: 999999999999729242 | Delta: -10042

How was it fixed?

I'm running with all the hard work @veox did, this is an attempt to put it into a more-committable form.

Cute Animal Picture

[lithp]

These test failures happened because Slave 'gwXX' crashed while running, when I run each of these failing tests locally they work, which makes me think it might be something transient, maybe a timeout? I don't have permissions to rerun the build though.

EDIT: performing a rebuild-via-squash

[pipermerriam]

@lithp I believe that may be indicative of an out-of-memory crash since pytest likely has to keep around a lot of data during the test run. It may mean the tests need to be split into smaller chunks.

[carver]

It may mean the tests need to be split into smaller chunks.

Even if it's not the cause of the crash, it would be best to split the tests.

Note how we had split the state tests into 5 parts, in order to get some balance on the length of the test jobs. With this PR, all 5 of those, and the blockchain tests, are combined into a single job.

See how they were split before here:

py-evm/tox.ini

Lines 38 to 45 in f616298

	native-blockchain: pytest {posargs:tests/json-fixtures/test_blockchain.py}
	native-state-frontier: pytest {posargs:tests/json-fixtures/test_state.py --fork Frontier}
	native-state-homestead: pytest {posargs:tests/json-fixtures/test_state.py --fork Homestead}
	native-state-eip150: pytest {posargs:tests/json-fixtures/test_state.py --fork EIP150}
	native-state-eip158: pytest {posargs:tests/json-fixtures/test_state.py --fork EIP158}
	native-state-byzantium: pytest {posargs:tests/json-fixtures/test_state.py --fork Byzantium}
	native-state-constantinople: pytest {posargs:tests/json-fixtures/test_state.py --fork Constantinople}
	native-state-metropolis: pytest {posargs:tests/json-fixtures/test_state.py --fork Metropolis}

(which may or may not help)

[lithp]

Yeah, that helps! I added a commit which skipped the state tests. The blockchain tests took 30 minutes to run and still had a few crashes, it looks like some of the SLOWEST_TESTS are so slow they can't even be run in circleci?

I think the next step here is to copy over some of the tools state tests have:

• --fork is only supported by state tests, and should be copied over to the blockchain tests
• SLOWEST_TESTS and mark_statetest_fixtures have knowledge which should be given to blockchain tests
• The blockchain tests should be split into different jobs, one per fork.

Don't know what your development process looks like, I think what I'll do is turn the BlockchainTests/GeneralStateTests off again, and enable them in a different PR so this one can stay focused?

Comments on #1562 would be appreciated! I think it would make the test a lot easier to debug if state tests were dropped in favor of their BlockchainTest equivalents.

[veox]

Some of these changes - extracted from this gist, via issue #1562 - are already pending inclusion as part of cburgdorf#2.

Since you're not including the portion that enables more Constantinople tests, this change (and the other one from same file) would be unrelated to the rest of this PR.

[lithp]

You're right, removed the Constantinople parts.

[carver]

it looks like some of the SLOWEST_TESTS are so slow they can't even be run in circleci?

Yeah, or maybe too memory-intensive? Haven't had a chance to dig in.

Don't know what your development process looks like, I think what I'll do is turn the BlockchainTests/GeneralStateTests off again, and enable them in a different PR so this one can stay focused?

Yeah, that sounds right. The more focused the better. Probably just rebase away the state test changes here, and move those to another PR.

I think the next step here is to copy over some of the tools state tests have:

Yup, all three LGTM.

Comments on #1562 would be appreciated! I think it would make the test a lot easier to debug if state tests were dropped in favor of their BlockchainTest equivalents.

Which open questions on 1562 are you looking for feedback on? On my quick scan, I didn't see any glaring unsolved problems.

[veox]

it looks like some of the SLOWEST_TESTS are so slow they can't even be run in circleci?

Yeah, or maybe too memory-intensive? Haven't had a chance to dig in.

Anecdotal: a local test run (still on-going) is currently consuming 3 GiB.

[lithp]

Accidental close 😢

Which open questions on 1562 are you looking for feedback on? On my quick scan, I didn't see any glaring unsolved problems.

The one I'm concerned about is whether it's a good idea to drop the state tests and instead use the one embedded within the blockchain tests

[lithp]

The tests are all passing but:

GitHub rate limited us while adding a commit status of success: You have triggered an abuse detection mechanism. Please wait a few minutes before you try again.

[veox]

While you're at it, could you also reverse the calculation on this line?.. (I've mentioned this in passing elsewhere.)

https://github.com/ethereum/py-evm/pull/1573/files#diff-38aa0bc8e408266959b43002a08f0cd4R81

For example, as things stand, if a fixture-expected value is 1000, and the actual produced by py-evm is 500, the printed message will show a delta of 500 - that is, a net positive, whereas what truly happened is py-evm having produced a value that's too low. This is confusing: say, a miner's account balance showing "Delta: 500" makes one think that the miner received 500 wei more than expected (which is not true).

If not, I can open a separate PR.

[carver]

The tests are all passing but:

GitHub rate limited us while adding a commit status of success: You have triggered an abuse detection mechanism. Please wait a few minutes before you try again.

Yeah, that issue is a recent-ish, known, and annoying.

[veox]

Blame Microsoft.

[carver]

While you're at it, could you also reverse the calculation on this line?.. (I've mentioned this in passing elsewhere.)

https://github.com/ethereum/py-evm/pull/1573/files#diff-38aa0bc8e408266959b43002a08f0cd4R81

I was juuust about to merge this, and it would be nice to rebase #1577 on this. Maybe @lithp can sneak the change into #1577

[lithp]

While you're at it, could you also reverse the calculation on this line?

Maybe @lithp can sneak the change into #1577

I didn't want to sneak an unrelated change into #1577 but made a tiny PR for it: #1580