Merge pull request #2023 from fselmo/erase-return-data-on-appropriate-exceptions

Erase return data on appropriate exceptions

Author: carver

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

@@ -154,13 +154,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)
+            computation.return_data = b''
+            if insufficient_funds:
+                err_msg = f"Insufficient Funds: {storage_address_balance} < {stack_data.endowment}"
+            elif stack_too_deep:
+                err_msg = "Stack limit reached"
+            self.logger.debug2("%s failure: %s", self.mnemonic, err_msg,)
             return
 
         call_data = computation.memory_read_bytes(

newsfragments/2023.bugfix.rst

@@ -0,0 +1 @@
+Erase return data for exceptions with `erases_return_data` flag set to True and for CREATE / CREATE2 computations with insufficient funds

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,94 @@
+# 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 (
+    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=40000,
+        gas_price=1,
+    )
+
+    if computation.is_error:
+        assert 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:
+        # We provide 40000 gas and a simple create uses 32000 gas. This test doesn't particularly
+        # care (and isn't testing for) the exact gas, we just want to make sure not all the gas
+        # is burned since if, say, a VMError were to be raised it would burn all the gas
+        # (burns_gas = True).
+        assert 34000 < computation.get_gas_used() < 39000
+        assert computation.return_data == b''

tests/core/vm/test_london.py

@@ -14,7 +14,6 @@
 from eth.tools.factories.transaction import (
     new_dynamic_fee_transaction, new_transaction,
 )
-from eth.vm.message import Message
 
 FOUR_TXN_GAS_LIMIT = 21000 * 4
 
@@ -57,17 +56,17 @@
 )
 
 
-def _configure_mining_chain(name, request):
+def _configure_mining_chain(name, genesis_vm, vm_under_test):
     return MiningChain.configure(
         __name__=name,
         vm_configuration=(
             (
                 constants.GENESIS_BLOCK_NUMBER,
-                BerlinVM.configure(consensus_class=NoProofConsensus),
+                genesis_vm.configure(consensus_class=NoProofConsensus),
             ),
             (
                 constants.GENESIS_BLOCK_NUMBER + 1,
-                request.param.configure(consensus_class=NoProofConsensus),
+                vm_under_test.configure(consensus_class=NoProofConsensus),
             ),
         ),
         chain_id=1337,
@@ -77,7 +76,9 @@ def _configure_mining_chain(name, request):
 # VMs starting at London
 @pytest.fixture(params=MAINNET_VMS[9:])
 def london_plus_miner(request, base_db, genesis_state):
-    klass = _configure_mining_chain('LondonAt1', request)
+    vm_under_test = request.param
+
+    klass = _configure_mining_chain('LondonAt1', BerlinVM, vm_under_test)
     header_fields = dict(
         difficulty=1,
         gas_limit=21000 * 2,  # block limit is hit with two transactions
@@ -90,7 +91,9 @@ def london_plus_miner(request, base_db, genesis_state):
 # VMs up to, but not including, London
 @pytest.fixture(params=MAINNET_VMS[0:9])
 def pre_london_miner(request, base_db, genesis_state):
-    klass = _configure_mining_chain('EndsBeforeLondon', request)
+    vm_under_test = request.param
+
+    klass = _configure_mining_chain('EndsBeforeLondon', MAINNET_VMS[0], vm_under_test)
     header_fields = dict(
         difficulty=1,
         gas_limit=100000,  # arbitrary, just enough for testing
@@ -145,41 +148,37 @@ def test_base_fee_evolution(
     EIP_3541_CREATE_AND_CREATE2_REVERT_TEST_CASES
 )
 def test_revert_on_reserved_0xEF_byte_for_CREATE_and_CREATE2_post_london(
-    london_plus_miner, transaction_context, funded_address, code, data,
+    london_plus_miner, funded_address, code, data,
 ):
     chain = london_plus_miner
-    state = chain.get_vm().state
+    vm = chain.get_vm()
 
     # test positive case from https://eips.ethereum.org/EIPS/eip-3541#test-cases
-    create_message = Message(
+    successful_create_computation = vm.execute_bytecode(
+        origin=funded_address,
         to=funded_address,
         sender=funded_address,
         value=0,
         code=code,
         data=decode_hex("0x60fe60005360016000f3"),
         gas=400000,
-    )
-
-    successful_create_computation = state.computation_class.apply_create_message(
-        state, create_message, transaction_context
+        gas_price=1,
     )
 
     assert successful_create_computation.is_success
-    # gas used varies between 32261 and 32270 depending on test case
+    # assert only the appropriate gas is consumed, not all the gas. This falls within a range
     assert 32261 <= successful_create_computation.get_gas_used() <= 32270
 
     # test parameterized negative cases
-    reverted_create_message = Message(
+    revert_create_computation = vm.execute_bytecode(
+        origin=funded_address,
         to=funded_address,
         sender=funded_address,
         value=0,
         code=code,
         data=data,
         gas=40000,
-    )
-
-    revert_create_computation = state.computation_class.apply_create_message(
-        state, reverted_create_message, transaction_context
+        gas_price=1,
     )
 
     assert revert_create_computation.is_error
@@ -275,32 +274,30 @@ def test_state_revert_on_reserved_0xEF_byte_for_create_transaction_post_london(
         assert "0xef" in repr(reverted_computation.error).lower()
 
         assert reverted_computation.get_nonce(funded_address) == 1  # assert nonce is still 1
-        # reverted txn only consumes gas:
-        assert end_balance == new_balance - mined_header.gas_used
+        # reverted txn consumes the gas:
+        assert mined_header.gas_used == 60000
+        assert end_balance == new_balance - 60000
 
 
 @pytest.mark.parametrize(
     "code, data",
     EIP_3541_CREATE_AND_CREATE2_REVERT_TEST_CASES
 )
 def test_state_does_not_revert_on_reserved_0xEF_byte_for_CREATE_and_CREATE2_pre_london(
-    pre_london_miner, transaction_context, funded_address, code, data,
+    pre_london_miner, funded_address, code, data,
 ):
     chain = pre_london_miner
     vm = chain.get_vm()
-    state = vm.state
 
-    create_message = Message(
+    computation = vm.execute_bytecode(
+        origin=funded_address,
         to=funded_address,
         sender=funded_address,
         value=0,
         code=code,
         data=data,
         gas=40000,
-    )
-
-    computation = state.computation_class.apply_create_message(
-        state, create_message, transaction_context
+        gas_price=1,
     )
 
     if computation.is_error:
@@ -311,7 +308,7 @@ def test_state_does_not_revert_on_reserved_0xEF_byte_for_CREATE_and_CREATE2_pre_
 
     else:
         assert computation.is_success
-        # gas used varies between 32670 and 38470 depending on test case
+        # assert only the appropriate gas is consumed, not all the gas. This falls within a range
         assert 32261 <= computation.get_gas_used() <= 38470
         assert computation.get_gas_refund() == 0