Remove contract specific commit

[mattdean-digicatapult]

This is part of the broader discussion in #268 and #309 so readers should probably start there.

This PR simplifies the stateManager interface by removing the two-phase commit that is exposed by stateManager. In the old setup a contract account goes through an extra phase of commit/revert so as to cope with the consensus bug after EIP-161 (https://github.com/ethereum/EIPs/blob/master/EIPS/eip-161.md). The aim here was to reproduce a behaviour whereby a precompiled contract is considered touched even in the case where an OOG error occurs. The new implementation introduces an explicit touch of a precompiled contract in the case of an OOG error in runCall and more closely matches the implementation used in other evm implementations. It also simplifies the interface of stateManager such that this behaviour does not have to be replicated in multiple places should other implementations be created as intended.

This PR also includes removing concept that a contract exists, relying instead on the concept of the account being empty. I believe in the current EVM empty-ness is the only concept that matters and so this change is broadly speaking just cleanup.

Ideally this latter change would have been done as a separate PR, but it turns out that separating this from the first change is problematic due to what I believe is a bug in the handling of touched contracts. Essentially when an error occurs during a CALL we completely reset the touched account set to the empty set. This means that a previously touched account, perhaps from a previous CALL, would now not be considered touched and would not be cleaned up if empty. The new implementation introduces a touched stack so as to handle reverting the touched set as required. Worryingly state tests pass for both the old implementation and the new one, thus a new test case should probably be created to resolve the correct behaviour.

[holgerd77]

Hi @mattdean-digicatapult, sorry, I am delayed on reviews due to personal reasons. I also think on this one, @jwasinger, @axic or @cdetrio would be more into the code base to do a review, maybe one of them is available?

I would otherwise try to look into this asap, but also have some other time-critical things on the plate.

[mattdean-digicatapult]

np @holgerd77, I really only added you as a reviewer as I wasn't sure who would be best suited to look at this. I'm not going to be available to do further work on this next week anyway, so it can wait a bit. This change does highlight a potential bug in how we handle touched accounts though, so it would be good to work out which interpretation is correct and also whether a new test case would be appropriate.

[holgerd77]

Hi @jwasinger, do you want to have a look if you can create such a new test case as described above (see Matthews introductory text)?

[holgerd77]

Hi @mattdean-digicatapult, just to bring you up-to-date: just did a first review and had a look into the various code parts. I think I now have a rough understanding what was intended and what is going on, nevertheless I don't understand all the details yet and so don't dare yet to do a proper review, will continue on this in the upcoming days.

Nevertheless my confidence in the code changes being consistent and generally making sense grew. 😄 I also ran the blockchain tests on this, outcome is the same (66 failing, far too much on a side note) as a test run (see #341) without the changes.

[coveralls]

Coverage increased (+0.1%) to 86.123% when pulling da81615 on remove-contract-specific-commit into 3431a07 on master.

[jwasinger]

Good find @mattdean-digicatapult . If you want I can show you how to write test cases (we are looking for people to help with testing for the next hard fork).

[jwasinger]

I can also look into seeing if we can get a bounty for this state test case once I confirm the bug from my side.

[mattdean-digicatapult]

@jwasinger, more than happy to learn how to write a test case for something like this. Some pointers on how to get started would be greatly appreciated

[jwasinger]

Check out this guide: https://github.com/ewasm/tests/tree/wasm-tests/src/GeneralStateTestsFiller/stEWASMTests . Although it's for ewasm tests, the normal tests are developed the exact same way. Just omit compiling Hera and remove --vm /path/to/libhera.so from the command you run.

You will also need to change the code section. You can write code for tests using LLL, raw bytecode (hex string) or solidity assembly. I usually use LLL because that's what I'm familiar with.

You will want a test with two pre-existing accounts: A, B. The test sends a transaction to A which calls B (the inner call returns successfully). then after, A traps. The expect section of the test should ensure that B still exists.

[jwasinger]

After, check to make sure the new test case fails on master (and passes on this PR).

[holgerd77]

Wouldn't this make sense to integrate this in the general tests documentation?

[jwasinger]

Yes

[jwasinger]

@mattdean-digicatapult @holgerd77 I'm actually surprised that this is an undefined area of test coverage. If you read, https://eips.ethereum.org/EIPS/eip-161 , scroll to the bottom to find this:

Geth was failing to revert empty account deletions when the transaction causing the deletions of empty accounts ended with an an out-of-gas exception. An additional issue was found in Parity, where the Parity client incorrectly failed to revert empty account deletions in a more limited set of contexts involving out-of-gas calls to precompiled contracts; the new Geth behavior matches Parity’s, and empty accounts will cease to be a source of concern in general in about one week once the state clearing process finishes.

So perhaps this was fixed in both clients but no one wrote tests for it?

[mattdean-digicatapult]

Thanks @jwasinger, give it a look. I'm not sure the logic of the test you describe quite covers the issue and I'm pretty sure I came across a test like that whilst working on this PR. I think the logic needs to be something like:

Two pre-existing accounts A and B. Transaction to A. First, call B which succeeds. Next, call B which fails. At the end of the transaction B should not exist.

My belief is that on master B will still exist as the touched set will have been emptied when the second call to B fails. I think the behaviour in this PR is the correct one but like I say, I'll give this a go and let you know if I run into any issues.

[holgerd77]

Maybe this is covered in the blockchain tests and not the state tests?

[jwasinger]

@holgerd77 could be. At this point, the state tests are a subset of blockchain tests (as state tests are converted to blockchain tests when a PR is made).

[jwasinger]

@mattdean-digicatapult Ah okay. Will be interesting to see what the result is. Feel free to ping me over gitter if you run into any problems with the testing tooling.

[mattdean-digicatapult]

OK, I've had some success defining a test case. You'll find the new test in my fork of the ethereum/tests repo https://github.com/mattdean-digicatapult/tests/blob/add-new-touched-cleanup-case/src/GeneralStateTestsFiller/stEIP158Specific/callToEmptyThenCallErrorFiller.json. As expected this fails against ethereumjs-vm master but passes after these changes.

I'm assuming the correct thing to do is to PR that new test prior to this being merged in?

[jwasinger]

Wonderful! Thanks so much @mattdean-digicatapult

[mattdean-digicatapult]

Now the test has been merged is there anything else to do prior to this being merged?

[holgerd77]

Hi @mattdean-digicatapult, cool, that you took on this! Will have a look during the week, I think this should be now ready for merge on first sight.

We are not using the tests directly but through snapshots in the https://github.com/ethereumjs/ethereumjs-testing repository, so it's a bit off since this code will be run directly on the new tests, we are currently working to fix somewhat recent tests here (but not yet including your new test cases).

We can nevertheless test-run/use your new cases manually, see the testing section in the README for instructions (in case you haven't already).

Cheers
Holger

[holgerd77]

If you run/ran your new tests on this, can you post your test results here? Then we have this explicitly documented.

[mattdean-digicatapult]

@holgerd77, I'm not sure precisely what you're after but I'll give it a go. To test I forked the testing repo and then updated the relevant references. When I then run the following:

node ./tests/tester.js -s --file=callToEmptyThenCallError --jsontrace

On master I get

TAP version 13
# GeneralStateTests
# file: callToEmptyThenCallError test: callToEmptyThenCallError
# {"pc":0,"op":96,"gas":"0x8d5b8","gasCost":"0x3","stack":[],"depth":0,"opName":"PUSH1"}
# {"pc":2,"op":96,"gas":"0x8d5b5","gasCost":"0x3","stack":["0x0"],"depth":0,"opName":"PUSH1"}
# {"pc":4,"op":96,"gas":"0x8d5b2","gasCost":"0x3","stack":["0x0","0x0"],"depth":0,"opName":"PUSH1"}
# {"pc":6,"op":96,"gas":"0x8d5af","gasCost":"0x3","stack":["0x0","0x0","0x0"],"depth":0,"opName":"PUSH1"}
# {"pc":8,"op":96,"gas":"0x8d5ac","gasCost":"0x3","stack":["0x0","0x0","0x0","0x0"],"depth":0,"opName":"PUSH1"}
# {"pc":10,"op":115,"gas":"0x8d5a9","gasCost":"0x3","stack":["0x0","0x0","0x0","0x0","0x0"],"depth":0,"opName":"PUSH20"}
# {"pc":31,"op":96,"gas":"0x8d5a6","gasCost":"0x3","stack":["0x0","0x0","0x0","0x0","0x0","0xee098e6c2a43d9e2c04f08f0c3a87b0ba59079d4"],"depth":0,"opName":"PUSH1"}
# {"pc":33,"op":241,"gas":"0x8d5a3","gasCost":"0x2bc","stack":["0x0","0x0","0x0","0x0","0x0","0xee098e6c2a43d9e2c04f08f0c3a87b0ba59079d4","0x0"],"depth":0,"opName":"CALL"}
# {"pc":34,"op":80,"gas":"0x8d2e7","gasCost":"0x2","stack":["0x1"],"depth":0,"opName":"POP"}
# {"pc":35,"op":96,"gas":"0x8d2e5","gasCost":"0x3","stack":[],"depth":0,"opName":"PUSH1"}
# {"pc":37,"op":96,"gas":"0x8d2e2","gasCost":"0x3","stack":["0x0"],"depth":0,"opName":"PUSH1"}
# {"pc":39,"op":96,"gas":"0x8d2df","gasCost":"0x3","stack":["0x0","0x0"],"depth":0,"opName":"PUSH1"}
# {"pc":41,"op":96,"gas":"0x8d2dc","gasCost":"0x3","stack":["0x0","0x0","0x0"],"depth":0,"opName":"PUSH1"}
# {"pc":43,"op":96,"gas":"0x8d2d9","gasCost":"0x3","stack":["0x0","0x0","0x0","0x0"],"depth":0,"opName":"PUSH1"}
# {"pc":45,"op":115,"gas":"0x8d2d6","gasCost":"0x3","stack":["0x0","0x0","0x0","0x0","0x0"],"depth":0,"opName":"PUSH20"}
# {"pc":66,"op":96,"gas":"0x8d2d3","gasCost":"0x3","stack":["0x0","0x0","0x0","0x0","0x0","0xc94f5374fce5edbc8e2a8697c15331677e6ebf0b"],"depth":0,"opName":"PUSH1"}
# {"pc":68,"op":241,"gas":"0x8d2d0","gasCost":"0x2bc","stack":["0x0","0x0","0x0","0x0","0x0","0xc94f5374fce5edbc8e2a8697c15331677e6ebf0b","0x0"],"depth":0,"opName":"CALL"}
# {"pc":0,"op":90,"gas":"0x0","gasCost":"0x2","stack":[],"depth":1,"opName":"GAS"}
# {"stateRoot":"6e2b8a992e7b1ffcf7cd1a3b5ede458ffb894dce2fd3a6c41e4b2df6553124aa"}
not ok 1 the state roots should match
  ---
    operator: equal
    expected: |-
      'f341372fc018357dda85c74aaae4029b5196a4a67035584f33a05382ae9b1099'
    actual: |-
      '6e2b8a992e7b1ffcf7cd1a3b5ede458ffb894dce2fd3a6c41e4b2df6553124aa'
    at: replenish (/Users/mdean/git/ethereumjs-vm/node_modules/async/dist/async.js:998:17)
  ...

1..1
# tests 1
# pass  0
# fail  1

Whilst on my feature branch I get:

TAP version 13
# GeneralStateTests
# file: callToEmptyThenCallError test: callToEmptyThenCallError
# {"pc":0,"op":96,"gas":"0x8d5b8","gasCost":"0x3","stack":[],"depth":0,"opName":"PUSH1"}
# {"pc":2,"op":96,"gas":"0x8d5b5","gasCost":"0x3","stack":["0x0"],"depth":0,"opName":"PUSH1"}
# {"pc":4,"op":96,"gas":"0x8d5b2","gasCost":"0x3","stack":["0x0","0x0"],"depth":0,"opName":"PUSH1"}
# {"pc":6,"op":96,"gas":"0x8d5af","gasCost":"0x3","stack":["0x0","0x0","0x0"],"depth":0,"opName":"PUSH1"}
# {"pc":8,"op":96,"gas":"0x8d5ac","gasCost":"0x3","stack":["0x0","0x0","0x0","0x0"],"depth":0,"opName":"PUSH1"}
# {"pc":10,"op":115,"gas":"0x8d5a9","gasCost":"0x3","stack":["0x0","0x0","0x0","0x0","0x0"],"depth":0,"opName":"PUSH20"}
# {"pc":31,"op":96,"gas":"0x8d5a6","gasCost":"0x3","stack":["0x0","0x0","0x0","0x0","0x0","0xee098e6c2a43d9e2c04f08f0c3a87b0ba59079d4"],"depth":0,"opName":"PUSH1"}
# {"pc":33,"op":241,"gas":"0x8d5a3","gasCost":"0x2bc","stack":["0x0","0x0","0x0","0x0","0x0","0xee098e6c2a43d9e2c04f08f0c3a87b0ba59079d4","0x0"],"depth":0,"opName":"CALL"}
# {"pc":34,"op":80,"gas":"0x8d2e7","gasCost":"0x2","stack":["0x1"],"depth":0,"opName":"POP"}
# {"pc":35,"op":96,"gas":"0x8d2e5","gasCost":"0x3","stack":[],"depth":0,"opName":"PUSH1"}
# {"pc":37,"op":96,"gas":"0x8d2e2","gasCost":"0x3","stack":["0x0"],"depth":0,"opName":"PUSH1"}
# {"pc":39,"op":96,"gas":"0x8d2df","gasCost":"0x3","stack":["0x0","0x0"],"depth":0,"opName":"PUSH1"}
# {"pc":41,"op":96,"gas":"0x8d2dc","gasCost":"0x3","stack":["0x0","0x0","0x0"],"depth":0,"opName":"PUSH1"}
# {"pc":43,"op":96,"gas":"0x8d2d9","gasCost":"0x3","stack":["0x0","0x0","0x0","0x0"],"depth":0,"opName":"PUSH1"}
# {"pc":45,"op":115,"gas":"0x8d2d6","gasCost":"0x3","stack":["0x0","0x0","0x0","0x0","0x0"],"depth":0,"opName":"PUSH20"}
# {"pc":66,"op":96,"gas":"0x8d2d3","gasCost":"0x3","stack":["0x0","0x0","0x0","0x0","0x0","0xc94f5374fce5edbc8e2a8697c15331677e6ebf0b"],"depth":0,"opName":"PUSH1"}
# {"pc":68,"op":241,"gas":"0x8d2d0","gasCost":"0x2bc","stack":["0x0","0x0","0x0","0x0","0x0","0xc94f5374fce5edbc8e2a8697c15331677e6ebf0b","0x0"],"depth":0,"opName":"CALL"}
# {"pc":0,"op":90,"gas":"0x0","gasCost":"0x2","stack":[],"depth":1,"opName":"GAS"}
# {"stateRoot":"f341372fc018357dda85c74aaae4029b5196a4a67035584f33a05382ae9b1099"}
ok 1 the state roots should match

1..1
# tests 1
# pass  1

# ok

Essentially the execution is identical; the only difference being that with this change the first called account is considered touched at the end of the transaction and is therefore cleaned up. On master this does not occur as the touched set is reset by the error in the second call.

[holgerd77]

Hi Matthew, ah yes, that's what I wanted to have documented here for future lookups of this PR, that the cases from the new test case are actually fixed with this PR, hope this didn't cause too much extra inconvenience.

[mattdean-digicatapult]

np, I already had it set up to do this from when I developed the test case

[holgerd77]

This looks good, will merge this after 24 hours wait period if no objection is coming from other reviewers or people watching this.

[holgerd77]

Hi @mattdean-digicatapult, thanks again for keeping this up so long, I really have a bad conscience here. I was somewhat completely away in the last weeks due to family reasons, but I am slowly getting back to work now and will take on being more responsive for stuff/work from you and give responses and reviews in a more timely fashion.