test_blockchain_fixtures() works poorly

[veox]

• py-evm Version: current master (commit b6865d0)
• OS: Arch Linux
• Python Version (python --version): 3.7.1
• Environment (output of pip freeze): MISSING (sorry!..)

What is wrong?

In testing for PR #1181 - which spawned PR #1560, and cburgdorf#2 in particular - I've done the following (quote from gitter):

(...) there's a very handy verify_account_db() invocation in test_blockchain_fixtures() of tests/json-fixtures/test_blockchain.py, which - with a little bit of commenting-out of asserts and checks through a couple files - can be used to easily compare the difference between the upstream test and py-evm's result.

Normally, GeneralStateTests are skipped in these test runs, to avoid duplication with tests/json-fixtures/test_state.py. However, the latter tests are less detailed in their JSON specification - so the comparison trick can't be used.

Using this, [along with increasing the logging level to 5 (trace), ] helps circumvent some guesswork [in debugging externally-provided tests].

The test function, however, works "poorly" in general, since it applies verify_account_db() unconditionally, and only after it has already applied the blocks (specified in the fixture) that have produced this account-DB-to-be-checked, and has compared the produced blocks (with those in the fixture).

In other words:

(...) there's no sense in doing these fine-grained state comparisons when [the state root in] the block header already matches. It just slows down the test even more.

Also, the assert in

py-evm/tests/json-fixtures/test_blockchain.py

Lines 123 to 125 in b6865d0

	if should_be_good_block:
	(block, mined_block, block_rlp) = apply_fixture_block_to_chain(block_fixture, chain)
	assert_mined_block_unchanged(block, mined_block)

is never hit, because apply_fixture_block_to_chain() has a very similar nested validity check that gets hit in the same conditions - shadowing the assert, and terrifying the poor sob debugging this with a single cytoolz-/functools-obfuscated generic function (aren't these supposed to go on forever, all the way down?..).

How can it be fixed

Not sure about the latter issue, and what "fixing" it might break. (Didn't do a git blame.)

Regarding verify_account_db(): don't do this in a "works as expected" run, but fall back on it if the {block header,state root} check fails. This will become possible when the above is addressed, or if the chain is grown without checks against the fixture.

Perhaps then it would make sense to run all tests from /BlockchainTests and not /GeneralStateTests... (Not sure.)

[veox]

For an example of how this can be useful, see #1181 (comment).

In short: it shows exactly what differs between py-evm and the externally-provided fixture, which helps narrow down the cause of the failure.

[veox]

Here's a gist that demonstrates how I slap this together currently (via git stash):

https://gist.github.com/veox/5874314b11cb731e69256c0bd451c88c

(Note that some changes are part of cburgdorf#2.)

[lithp]

This is awesome! I played with the gist and ended up with #1573

I think it makes sense to only run BlockchainTests and drop GeneralStateTests; they're perfect duplicates except one is easier to debug. They test more and are consequently a little slower, unfortunately.

[carver]

Another possibility for why we were using the global state tests is that we were running state tests before we even had code that was able to import blocks. ie~ EVM execution was being tested before chain structure, which makes sense to me from an ordering point of view.

If that were true, then we only needed the global state tests for bootstrapping, so we could switch to the blockchain state tests.

In general, I can't think of a reason why we can't use the ones inside blockchain tests right now. (Only that a block import failure would cause "false test failures" in state tests, which is a pretty minor problem I think)

---

Mostly unrelated: I can't think of a reason that ethereum/tests couldn't copy the postState into the other state test jsons, as well. That would help the next brand new client to get started, I expect.

[veox]

I think this can be closed as fixed by PR #1573.

Respond if I'm missing something, will reopen.