BlockchainTests format changed

[MrChico]

With f386934 (and the corresponding deletion f11c474#diff-1a427fc3f925eef4e65fb0650dc712f7 ) the format of the transactions in BlockchainTests changed, from signed to unsigned.

Was this breaking change was intended? It has rendered both https://github.com/kframework/evm-semantics and https://github.com/dapphub/dapptools/tree/master/src/hevm incapable of running these tests.

[dvdplm]

Parity also expect r, s, v to be present, and considers secretKey optional. See https://github.com/paritytech/parity-ethereum/blob/d311bebaeee3793dc2aa433dbe060fe4c873367f/json/src/transaction.rs#L38-L43

We can work around that particular quirk by changing the parsing code, but there seems to be other problems with the recent updates.

[dvdplm]

Another issue is that some tests seem to have change the format of postState to be a hash (?) rather than a map, e.g. this and this. Not sure where to look for documentation on that, if it's a mistake that needs fixing or us that need to update our parsing code.

[holiman]

Another issue is that some tests seem to have change the format of postState to be a hash (?) rather than a map

👍 on that -- If poststates are too large, then it would be better to add a new field postStateHash instead of changing a map to a string. However, it seems kind of moot, since the postStateHash obivously is identical to the stateRoot of the last successfull block. E.g.

tests/BlockchainTests/GeneralStateTests/stRecursiveCreate/recursiveCreateReturnValue.json

Line 66 in f386934

"stateRoot" : "0xd6fef701f6ca86d54a6756a69d5f69ab4ad53fd6c75b4d3134e5d0500d9ef6c2",

. So I don't really see the point in having it there... ?

If it's not there, then it's just a matter for following lastblockhash, find that block, check stateRoot.

[holiman]

Anyway, the postState for blockchain tests are moot in general, and can be ignored. They only serve to validate that the test itself is correctly generated (has expected stuff).

A client actually running the test need only verify the lastBlockhash after importing the rlp:ed blocks.

[FrankSzendzielarz]

Another issue is that some tests seem to have change the format of postState to be a hash (?) rather than a map, e.g. this and this. Not sure where to look for documentation on that, if it's a mistake that needs fixing or us that need to update our parsing code.

Had the same issue in Hive. Patched by ignoring poststate. IMO this is an error in the data format that needs correcting as the tests now adhere to differing types of metadata.

[dvdplm]

Debugging failing tests is a significant time sink so I think keeping the data format as simple as possible to parse is valuable.

[holiman]

Geth also chokes on the change:
https://travis-ci.org/ethereum/go-ethereum/jobs/586074502#L587

[winsvega]

You don't need to parse the transaction in blockchain tests. That field is for humans. All you do is import block rlps one by one and compare the lastblockhash.

All other fields are just supplementary. Huge post state was replaced by hash because validation/retrival of it over RPC during test generation takes way too much time

[FrankSzendzielarz]

@winsvega Actually the problem is that the poststate for most tests was not replaced, resulting in an inconsistent data format. In the Blockchain tests folder only about 5 or 6 files exhibit the new format.

[winsvega]

For the debugging the idea is to use retesteth + new rpc method of evm trace.
Using 2 clients retesteth could detect a particular byte instruction on which clients go different paths.

[winsvega]

Only huge post states are affected. Smaller post states kept in the tests for debug.

[FrankSzendzielarz]

Sure but now we have two incompatible data types. It's patched in Hive by ignoring poststate completely, but now we have a test suite with tests that have differing formats.

[winsvega]

I think the test repo needs master branch of stable consensus test format.

[FrankSzendzielarz]

Ah I see. It's a Develop branch.