Remove VM tests with CALL, CREATE, SELFDESTRUCT and the like, translating them into GeneralStateTests

[pirapira]

[ehildenb]

@pirapira any recent thoughts on this? I don't think we should do away with VMTests, but perhaps could limit their scope. It doesn't feel good to have KEVM have two versions of several opcodes, one just for VMTests.

Perhaps the VMTests that only deal with VM-related stuff could remain (eg. arithmetic, local memory ops, program counter ops), and anything that deals with Ethereum-specific state could be translated into GeneralStateTests instead?

I've become interested in this because I'm in the middle of refactoring the KEVM control-flow to make it more uniform to make doing proofs with KEVM easier (and also to support more uniform typed exceptions), but there are several spots where special cases are made for VMTests mode which are causing me headache.

[ehildenb]

Note that I do like having the VMTests around as an early target for implementers who have only made it through implementing the basic VM functionality. Similarly, someone might implement a wasm VM without making it extend to the ewasm, and would want to be able to run the wasm test-set without worrying about the extra host-functions added for ethereum.

[pirapira]

@ehildenb I have exactly the same reasons for keeping VMtests. I changed the title of this issue, to remove only those problematic opcodes from VMtests.

[ehildenb]

Ok, then I guess we should discuss which tests should specifically be removed from the test-suite. At least the ones dealing with CALL*, CREATE, and SELFDESTRUCT I think. I will try to run the VMTests in non-VMTEST mode on KEVM and get a list of tests that break then post them here.

[ehildenb]

Here is a list of tests that fail when I disable all VMTESTS specific stuff: disable-vmtests-mode-initial.txt

There are 40 total (including BLOCKHASH tests as well).

[ehildenb]

Full list of "problematic" opcodes: BALANCE, EXTCODESIZE, EXTCODECOPY, CALL*, CREATE. Actually doesn't seem to be any problem with SELFDESTRUCT.

[ehildenb]

So I have removed most of the dependencies of KEVM on VMTESTS mode (and disabled the relevant tests, which is coming to about 70ish now). One final issue remains, which is that the executing account's balance is asserted in the post-state of the VMTests. Because there is no transaction, there isn't a gasPrice to use, so a default gasPrice = 1 is assumed.

I think that the post-state of the account should not be asserted in VMTests (or anything related to network-state/non-vm execution). I do think that the VMTests should be making assertions about the gas itself, but that is already happening separately over the gas component of state directly. For now I will disable the post-state balance assertions in KEVM to completely remove the semantics dependence on VMTESTS mode.

This makes me wonder about post-state storage assertions though. When talking with @winsvega at EthCC, it seemed that the storage was used to make assertions about computation by doing the computation then asserting that the resulting storage had some form, and certainly the tests are written that way. If we're taking the "separate the VM from the network state" seriously, I think even SLOAD and SSTORE don't belong there. In KEVM it is easy to also make assertions about things like the local memory and stack, but I get the feeling it won't be super easy in other clients because they are probably already set up to be able to do state-dumps of network state, but maybe not VM execution state.

[winsvega]

Just to note.
Storage is used in state tests as a mean to check the execution result.
In VMTests gasUsed is the indicator of bytecode execution results.
The storage check was added later and is used when generating the tests.

[pirapira]

I picked the first one

VMTests/vmBlockInfoTest/blockhash258Block.json

I'll see if this is worth translating to GeneralStateTest, and work accordingly. With the feedback on this one, I can continue with the others.

EDIT: well, this one looks like just testing the blockhash mock for the VM tests. I'm going to remove these, no translations.

[pirapira]

@ehildenb why is VMTests/vmRandomTest/201503120909PYTHON.json listed? Is it because of CALLVALUE and CALLDATALOAD?

[ehildenb]

Commit which drops VMTESTS mode from KEVM semantics: runtimeverification/evm-semantics@902a7ed

[pirapira]

I tried the branch, and I can see some of the listed VM tests can stay, like pirapira/evm-semantics@e350e75