VM: address consensus issue: tx goes OOG but refunds get applied anyways

[jochem-brouwer]

Closes #1601, closes #1604

[codecov]

Codecov Report

Merging #1603 (8147f25) into master (bffbc03) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Flag	Coverage Δ
block	84.96% <ø> (ø)
blockchain	85.08% <ø> (ø)
client	70.89% <ø> (ø)
common	100.00% <ø> (ø)
devp2p	83.17% <ø> (+0.13%)	⬆️
ethash	∅ <ø> (∅)
trie	86.09% <ø> (ø)
tx	88.62% <ø> (ø)
util	91.81% <ø> (ø)
vm	79.79% <100.00%> (+0.08%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

[jochem-brouwer]

This doesn't work, will stop here for now. The problem seems to be that we revert to old refund here, but this does not catch all errors which are not caught in this scope; if anything happens which makes a message scope revert then the entire refund should be forfeited.

[jochem-brouwer]

CC @winsvega we have encountered an consensus bug in this PR which is not covered in ethereum/tests. I will write up a more detailed explanation later on, if I forget please remind me.

[winsvega]

Hm, should be covered, lets check

[jochem-brouwer]

Here's the problem.

We check if there's an execution error in runInterpreter and if thats the case we reset the refund counter to the previous refund counter here.

This is all fine if we don't do additional error checks afterwards. For call frames, we dont do this, however, for create frames we do this and here lies the problem. In case that we throw an error over there, then we don't reset the refunds. Note that any error down the runInterpreter should be caught.

NOTE: there might be an uncovered edge case as well, in run interpreter in case of an error we also reset;

      // Clear the result on error
      result = {
        ...result,
        logs: [],
        selfdestruct: {},
      }

We might have to check what happens in case that in case we dont have enough gas to pay for code deposit, if we also revert the selfdestruct / logs correctly.

[acolytec3]

Looks good to me. One nigly change in the wording for the test to clarify what's happening that you can take or leave but awesome work getting a fix so quickly!

[gabrocheleau]

Looks good to me.

[jochem-brouwer]

ethereum/tests#1003 has found that the possible selfdestruct bug as described in #1605 is also present, will fix here. We probably also have the issue in #1605 with logs.

[acolytec3]

@jochem-brouwer So do we need to add some additional tests for these other cases regard logs/self-destruct?

[jochem-brouwer]

I could add tests, but since these test cases which I want are covered in the ethereum-tests PR linked above, I'd say that I run that branch locally, verify that the relevant tests pass, and then once there's a new ethereum-tests release we integrate it, which then implicitly means that the relevant tests which were failing before this PR then pass. The relevant selfdestruct test passes.

[jochem-brouwer]

Okay, ethereum/tests#1003 now has tests for both selfdestructs and logs into the contract creation context for cases where (0) contract gets deployed succesfully, (1) contract goes OOG due to invalid opcode, and (2) contract goes OOG because execution finishes but there is not enough gas left to deploy the code.

Tests now pass after 2f33d13. We had the selfdestruct vulnerability, we did not have the log vulnerability.

[ryanio]

amazing speed and execution on this, thank you everyone! lgtm