reset return data when VMError raised

VMError has a `erases_return_data` property but this never guaranteed anything. The BaseCall class somewhat accounted for this in its `__call__` method but this is sometimes not called e.g. Create(Opcode). We account for the `burns_gas` flag for the VMError on the `__exit__` call of a computation and so these changes make sure to account for the erasing of the return data for errors with the `erases_return_data` flag within this `__exit__` method as well.

Author: fselmo

eth/vm/computation.py

@@ -489,6 +489,10 @@ def __exit__(self,
                     )),
                 )
 
+            # When we raise an exception that erases return data, erase the return data.
+            if self.should_erase_return_data:
+                self.return_data = b''
+
             # suppress VM exceptions
             return True
         elif exc_type is None and self.logger.show_debug2:

eth/vm/logic/system.py

@@ -7,7 +7,9 @@
 from eth import constants
 from eth.exceptions import (
     Halt,
+    InsufficientFunds,
     Revert,
+    StackDepthLimit,
     WriteProtection,
 )
 
@@ -154,14 +156,19 @@ def __call__(self, computation: ComputationAPI) -> None:
 
         computation.extend_memory(stack_data.memory_start, stack_data.memory_length)
 
-        insufficient_funds = computation.state.get_balance(
-            computation.msg.storage_address
-        ) < stack_data.endowment
+        storage_address_balance = computation.state.get_balance(computation.msg.storage_address)
+
+        insufficient_funds = storage_address_balance < stack_data.endowment
         stack_too_deep = computation.msg.depth + 1 > constants.STACK_DEPTH_LIMIT
 
         if insufficient_funds or stack_too_deep:
             computation.stack_push_int(0)
-            return
+            if insufficient_funds:
+                raise InsufficientFunds(
+                    f"Insufficient funds: {storage_address_balance} < {stack_data.endowment}"
+                )
+            elif stack_too_deep:
+                raise StackDepthLimit("Stack depth limit reached")
 
         call_data = computation.memory_read_bytes(
             stack_data.memory_start, stack_data.memory_length

newsfragments/2023.bugfix.rst

@@ -0,0 +1 @@
+Erase return data for exceptions with `erases_return_data` flag set to True

tests/core/vm/test_base_computation.py

@@ -346,31 +346,39 @@ def test_get_gas_used_with_revert(computation):
 
 def test_should_burn_gas_with_vm_error(computation):
     assert computation.get_gas_remaining() == 100
-    # Trigger an out of gas error causing get gas remaining to be 0
+    # Trigger an out of gas error causing remaining gas to be 0
     with computation:
         raise VMError('Triggered VMError for tests')
     assert computation.should_burn_gas
+    assert computation.get_gas_used() == 100
+    assert computation.get_gas_remaining() == 0
 
 
 def test_should_burn_gas_with_revert(computation):
     assert computation.get_gas_remaining() == 100
-    # Trigger an out of gas error causing get gas remaining to be 0
+    # Trigger a Revert which should not burn gas
     with computation:
         raise Revert('Triggered VMError for tests')
     assert not computation.should_burn_gas
+    assert computation.get_gas_used() == 0
+    assert computation.get_gas_remaining() == 100
 
 
 def test_should_erase_return_data_with_vm_error(computation):
     assert computation.get_gas_remaining() == 100
-    # Trigger an out of gas error causing get gas remaining to be 0
+    computation.return_data = b'\x1337'
+    # Trigger a VMError which should erase the return data
     with computation:
         raise VMError('Triggered VMError for tests')
     assert computation.should_erase_return_data
+    assert computation.return_data == b''
 
 
 def test_should_erase_return_data_with_revert(computation):
     assert computation.get_gas_remaining() == 100
-    # Trigger an out of gas error causing get gas remaining to be 0
+    computation.return_data = b'\x1337'
+    # Trigger a Revert which should not erase return data
     with computation:
         raise Revert('Triggered VMError for tests')
     assert not computation.should_erase_return_data
+    assert computation.return_data == b'\x1337'

tests/core/vm/test_computation.py

@@ -0,0 +1,95 @@
+# test computation class behavior across VMs
+import pytest
+from eth_typing import Address
+
+from eth_utils import decode_hex
+
+from eth.chains.base import MiningChain
+from eth.chains.mainnet import MAINNET_VMS
+
+from eth import constants
+from eth.consensus import NoProofConsensus
+from eth.exceptions import (
+    InsufficientFunds,
+    InvalidInstruction,
+)
+
+
+def _configure_mining_chain(starting_vm, vm_under_test):
+    return MiningChain.configure(
+        __name__="AllVMs",
+        vm_configuration=(
+            (
+                constants.GENESIS_BLOCK_NUMBER,
+                starting_vm.configure(consensus_class=NoProofConsensus),
+            ),
+            (
+                constants.GENESIS_BLOCK_NUMBER + 1,
+                vm_under_test.configure(consensus_class=NoProofConsensus),
+            ),
+        ),
+        chain_id=1337,
+    )
+
+
+# CREATE, RETURNDATASIZE, and RETURNDATACOPY opcodes not added until Byzantium
+@pytest.fixture(params=MAINNET_VMS[4:])
+def byzantium_plus_miner(request, base_db, genesis_state):
+    byzantium_vm = MAINNET_VMS[4]
+    vm_under_test = request.param
+
+    klass = _configure_mining_chain(byzantium_vm, vm_under_test)
+    header_fields = dict(
+        difficulty=1,
+        gas_limit=100000,  # arbitrary, just enough for testing
+    )
+    return klass.from_genesis(base_db, header_fields, genesis_state)
+
+
+@pytest.mark.parametrize(
+    "code",
+    (
+        # generate some return data, then call CREATE (third-to-last opcode - 0xf0)
+        decode_hex('0x5b595958333d5859858585858585858585f195858585858585858485858585f195858585858585f1f03d30'),  # noqa: E501
+        # generate some return data, then call CREATE2 (third-to-last opcode - 0xf5)
+        decode_hex('0x5b595958333d5859858585858585858585f195858585858585858485858585f195858585858585f1f53d30'),  # noqa: E501
+    )
+)
+def test_CREATE_and_CREATE2_resets_return_data_if_account_has_insufficient_funds(
+    byzantium_plus_miner,
+    canonical_address_a,
+    transaction_context,
+    code,
+):
+    chain = byzantium_plus_miner
+    vm = chain.get_vm()
+    state = vm.state
+
+    assert state.get_balance(canonical_address_a) == 0
+
+    computation = vm.execute_bytecode(
+        origin=canonical_address_a,
+        to=canonical_address_a,
+        sender=Address(
+            b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x12\x34'
+        ),
+        value=0,
+        code=code,
+        data=code,
+        gas=400000,
+        gas_price=1,
+    )
+
+    assert computation.is_error
+
+    if isinstance(computation.error, InvalidInstruction):
+        # only test CREATE case for byzantium as the CREATE2 opcode (0xf5) was not yet introduced
+        assert vm.fork == "byzantium"
+        assert "0xf5" in repr(computation.error).lower()
+
+    else:
+        assert isinstance(computation.error, InsufficientFunds)
+
+        assert computation.get_gas_used() == 400000
+        assert computation.get_gas_refund() == 0
+        assert computation.return_data == b''