Merge pull request #1921 from carver/static-apply-message

carver · web-flow · commit 001399767a0e · 2020-04-09T16:38:51.000-07:00
Convert apply_message to a classmethod
diff --git a/eth/abc.py b/eth/abc.py
@@ -1542,17 +1542,28 @@ def get_log_entries(self) -> Tuple[Tuple[bytes, Tuple[int, ...], bytes], ...]:
     #
     # State Transition
     #
+    @classmethod
     @abstractmethod
-    def apply_message(self) -> 'ComputationAPI':
+    def apply_message(
+            cls,
+            state: 'StateAPI',
+            message: MessageAPI,
+            transaction_context: TransactionContextAPI) -> 'ComputationAPI':
         """
-        Execution of a VM message.
+        Execute a VM message. This is where the VM-specific call logic exists.
         """
         ...
 
+    @classmethod
     @abstractmethod
-    def apply_create_message(self) -> 'ComputationAPI':
+    def apply_create_message(
+            cls,
+            state: 'StateAPI',
+            message: MessageAPI,
+            transaction_context: TransactionContextAPI) -> 'ComputationAPI':
         """
-        Execution of a VM message to create a new contract.
+        Execute a VM message to create a new contract. This is where the VM-specific
+        create logic exists.
         """
         ...
 
@@ -1563,7 +1574,14 @@ def apply_computation(cls,
                           message: MessageAPI,
                           transaction_context: TransactionContextAPI) -> 'ComputationAPI':
         """
-        Perform the computation that would be triggered by the VM message.
+        Execute the logic within the message: Either run the precompile, or
+        step through each opcode.  Generally, the only VM-specific logic is for
+        each opcode as it executes.
+
+        This should rarely be called directly, because it will skip over other important
+        VM-specific logic that happens before or after the execution.
+
+        Instead, prefer :meth:`~apply_message` or :meth:`~apply_create_message`.
         """
         ...
 
@@ -2499,7 +2517,17 @@ def execute_bytecode(self,
                          code_address: Address = None) -> ComputationAPI:
         """
         Execute raw bytecode in the context of the current state of
-        the virtual machine.
+        the virtual machine. Note that this skips over some of the logic
+        that would normally happen during a call. Watch out for:
+
+            - value (ether) is *not* transferred
+            - state is *not* rolled back in case of an error
+            - The target account is *not* necessarily created
+            - others...
+
+        For other potential surprises, check the implementation differences
+        between :meth:`ComputationAPI.apply_computation` and
+        :meth:`ComputationAPI.apply_message`. (depending on the VM fork)
         """
         ...
 
diff --git a/eth/vm/base.py b/eth/vm/base.py
@@ -216,7 +216,7 @@ def execute_bytecode(self,
         )
 
         # Execute it in the VM
-        return self.state.get_computation(message, transaction_context).apply_computation(
+        return self.state.computation_class.apply_computation(
             self.state,
             message,
             transaction_context,
diff --git a/eth/vm/computation.py b/eth/vm/computation.py
@@ -1,6 +1,3 @@
-from abc import (
-    abstractmethod,
-)
 import itertools
 from types import TracebackType
 from typing import (
@@ -369,17 +366,17 @@ def apply_child_computation(self, child_msg: MessageAPI) -> ComputationAPI:
 
     def generate_child_computation(self, child_msg: MessageAPI) -> ComputationAPI:
         if child_msg.is_create:
-            child_computation = self.__class__(
+            child_computation = self.apply_create_message(
                 self.state,
                 child_msg,
                 self.transaction_context,
-            ).apply_create_message()
+            )
         else:
-            child_computation = self.__class__(
+            child_computation = self.apply_message(
                 self.state,
                 child_msg,
                 self.transaction_context,
-            ).apply_message()
+            )
         return child_computation
 
     def add_child_computation(self, child_computation: ComputationAPI) -> None:
@@ -513,14 +510,6 @@ def __exit__(self,
     #
     # State Transition
     #
-    @abstractmethod
-    def apply_message(self) -> ComputationAPI:
-        raise NotImplementedError("Must be implemented by subclasses")
-
-    @abstractmethod
-    def apply_create_message(self) -> ComputationAPI:
-        raise NotImplementedError("Must be implemented by subclasses")
-
     @classmethod
     def apply_computation(cls,
                           state: StateAPI,
diff --git a/eth/vm/forks/frontier/computation.py b/eth/vm/forks/frontier/computation.py
@@ -15,6 +15,9 @@
 )
 from eth.abc import (
     ComputationAPI,
+    MessageAPI,
+    StateAPI,
+    TransactionContextAPI,
 )
 from eth.exceptions import (
     OutOfGas,
@@ -45,47 +48,59 @@ class FrontierComputation(BaseComputation):
     opcodes = FRONTIER_OPCODES
     _precompiles = FRONTIER_PRECOMPILES     # type: ignore # https://github.com/python/mypy/issues/708 # noqa: E501
 
-    def apply_message(self) -> ComputationAPI:
-        snapshot = self.state.snapshot()
+    @classmethod
+    def apply_message(
+            cls,
+            state: StateAPI,
+            message: MessageAPI,
+            transaction_context: TransactionContextAPI) -> ComputationAPI:
 
-        if self.msg.depth > STACK_DEPTH_LIMIT:
+        snapshot = state.snapshot()
+
+        if message.depth > STACK_DEPTH_LIMIT:
             raise StackDepthLimit("Stack depth limit reached")
 
-        if self.msg.should_transfer_value and self.msg.value:
-            sender_balance = self.state.get_balance(self.msg.sender)
+        if message.should_transfer_value and message.value:
+            sender_balance = state.get_balance(message.sender)
 
-            if sender_balance < self.msg.value:
+            if sender_balance < message.value:
                 raise InsufficientFunds(
-                    f"Insufficient funds: {sender_balance} < {self.msg.value}"
+                    f"Insufficient funds: {sender_balance} < {message.value}"
                 )
 
-            self.state.delta_balance(self.msg.sender, -1 * self.msg.value)
-            self.state.delta_balance(self.msg.storage_address, self.msg.value)
+            state.delta_balance(message.sender, -1 * message.value)
+            state.delta_balance(message.storage_address, message.value)
 
-            self.logger.debug2(
+            cls.logger.debug2(
                 "TRANSFERRED: %s from %s -> %s",
-                self.msg.value,
-                encode_hex(self.msg.sender),
-                encode_hex(self.msg.storage_address),
+                message.value,
+                encode_hex(message.sender),
+                encode_hex(message.storage_address),
             )
 
-        self.state.touch_account(self.msg.storage_address)
+        state.touch_account(message.storage_address)
 
-        computation = self.apply_computation(
-            self.state,
-            self.msg,
-            self.transaction_context,
+        computation = cls.apply_computation(
+            state,
+            message,
+            transaction_context,
         )
 
         if computation.is_error:
-            self.state.revert(snapshot)
+            state.revert(snapshot)
         else:
-            self.state.commit(snapshot)
+            state.commit(snapshot)
 
         return computation
 
-    def apply_create_message(self) -> ComputationAPI:
-        computation = self.apply_message()
+    @classmethod
+    def apply_create_message(
+            cls,
+            state: StateAPI,
+            message: MessageAPI,
+            transaction_context: TransactionContextAPI) -> ComputationAPI:
+
+        computation = cls.apply_message(state, message, transaction_context)
 
         if computation.is_error:
             return computation
@@ -102,11 +117,11 @@ def apply_create_message(self) -> ComputationAPI:
                 except OutOfGas:
                     computation.output = b''
                 else:
-                    self.logger.debug2(
+                    cls.logger.debug2(
                         "SETTING CODE: %s -> length: %s | hash: %s",
-                        encode_hex(self.msg.storage_address),
+                        encode_hex(message.storage_address),
                         len(contract_code),
                         encode_hex(keccak(contract_code))
                     )
-                    self.state.set_code(self.msg.storage_address, contract_code)
+                    state.set_code(message.storage_address, contract_code)
             return computation
diff --git a/eth/vm/forks/frontier/state.py b/eth/vm/forks/frontier/state.py
@@ -123,14 +123,17 @@ def build_computation(self,
                     encode_hex(message.storage_address),
                 )
             else:
-                computation = self.vm_state.get_computation(
+                computation = self.vm_state.computation_class.apply_create_message(
+                    self.vm_state,
                     message,
                     transaction_context,
-                ).apply_create_message()
+                )
         else:
-            computation = self.vm_state.get_computation(
+            computation = self.vm_state.computation_class.apply_message(
+                self.vm_state,
                 message,
-                transaction_context).apply_message()
+                transaction_context,
+            )
 
         return computation
 
diff --git a/eth/vm/forks/homestead/computation.py b/eth/vm/forks/homestead/computation.py
@@ -8,7 +8,12 @@
     encode_hex,
 )
 
-from eth.abc import ComputationAPI
+from eth.abc import (
+    ComputationAPI,
+    MessageAPI,
+    StateAPI,
+    TransactionContextAPI,
+)
 from eth.vm.forks.frontier.computation import (
     FrontierComputation,
 )
@@ -24,13 +29,18 @@ class HomesteadComputation(FrontierComputation):
     # Override
     opcodes = HOMESTEAD_OPCODES
 
-    def apply_create_message(self) -> ComputationAPI:
-        snapshot = self.state.snapshot()
+    @classmethod
+    def apply_create_message(
+            cls,
+            state: StateAPI,
+            message: MessageAPI,
+            transaction_context: TransactionContextAPI) -> ComputationAPI:
+        snapshot = state.snapshot()
 
-        computation = self.apply_message()
+        computation = cls.apply_message(state, message, transaction_context)
 
         if computation.is_error:
-            self.state.revert(snapshot)
+            state.revert(snapshot)
             return computation
         else:
             contract_code = computation.output
@@ -46,18 +56,18 @@ def apply_create_message(self) -> ComputationAPI:
                     # Different from Frontier: reverts state on gas failure while
                     # writing contract code.
                     computation.error = err
-                    self.state.revert(snapshot)
+                    state.revert(snapshot)
                 else:
-                    if self.logger:
-                        self.logger.debug2(
+                    if cls.logger:
+                        cls.logger.debug2(
                             "SETTING CODE: %s -> length: %s | hash: %s",
-                            encode_hex(self.msg.storage_address),
+                            encode_hex(message.storage_address),
                             len(contract_code),
                             encode_hex(keccak(contract_code))
                         )
 
-                    self.state.set_code(self.msg.storage_address, contract_code)
-                    self.state.commit(snapshot)
+                    state.set_code(message.storage_address, contract_code)
+                    state.commit(snapshot)
             else:
-                self.state.commit(snapshot)
+                state.commit(snapshot)
             return computation
diff --git a/eth/vm/forks/spurious_dragon/computation.py b/eth/vm/forks/spurious_dragon/computation.py
@@ -4,7 +4,12 @@
 )
 
 from eth import constants
-from eth.abc import ComputationAPI
+from eth.abc import (
+    ComputationAPI,
+    MessageAPI,
+    StateAPI,
+    TransactionContextAPI,
+)
 from eth.exceptions import (
     OutOfGas,
 )
@@ -24,16 +29,22 @@ class SpuriousDragonComputation(HomesteadComputation):
     # Override
     opcodes = SPURIOUS_DRAGON_OPCODES
 
-    def apply_create_message(self) -> ComputationAPI:
-        snapshot = self.state.snapshot()
+    @classmethod
+    def apply_create_message(
+            cls,
+            state: StateAPI,
+            message: MessageAPI,
+            transaction_context: TransactionContextAPI) -> ComputationAPI:
+
+        snapshot = state.snapshot()
 
         # EIP161 nonce incrementation
-        self.state.increment_nonce(self.msg.storage_address)
+        state.increment_nonce(message.storage_address)
 
-        computation = self.apply_message()
+        computation = cls.apply_message(state, message, transaction_context)
 
         if computation.is_error:
-            self.state.revert(snapshot)
+            state.revert(snapshot)
             return computation
         else:
             contract_code = computation.output
@@ -43,7 +54,7 @@ def apply_create_message(self) -> ComputationAPI:
                     f"Contract code size exceeds EIP170 limit of {EIP170_CODE_SIZE_LIMIT}."
                     f"  Got code of size: {len(contract_code)}"
                 )
-                self.state.revert(snapshot)
+                state.revert(snapshot)
             elif contract_code:
                 contract_code_gas_cost = len(contract_code) * constants.GAS_CODEDEPOSIT
                 try:
@@ -55,18 +66,18 @@ def apply_create_message(self) -> ComputationAPI:
                     # Different from Frontier: reverts state on gas failure while
                     # writing contract code.
                     computation.error = err
-                    self.state.revert(snapshot)
+                    state.revert(snapshot)
                 else:
-                    if self.logger:
-                        self.logger.debug2(
+                    if cls.logger:
+                        cls.logger.debug2(
                             "SETTING CODE: %s -> length: %s | hash: %s",
-                            encode_hex(self.msg.storage_address),
+                            encode_hex(message.storage_address),
                             len(contract_code),
                             encode_hex(keccak(contract_code))
                         )
 
-                    self.state.set_code(self.msg.storage_address, contract_code)
-                    self.state.commit(snapshot)
+                    state.set_code(message.storage_address, contract_code)
+                    state.commit(snapshot)
             else:
-                self.state.commit(snapshot)
+                state.commit(snapshot)
             return computation
diff --git a/newsfragments/1921.internal.rst b/newsfragments/1921.internal.rst
@@ -0,0 +1,5 @@
+Fix for creating a duplicate "ghost" Computation that was never used. It didn't
+break anything, but was ineligant and surprising to get extra objects created
+that were mostly useless. This was achieved by changing
+:meth:`ComputationAPI.apply_message` and
+:meth:`ComputationAPI.apply_create_message` to be class methods.
diff --git a/pytest.ini b/pytest.ini
diff --git a/tests/core/opcodes/test_opcodes.py b/tests/core/opcodes/test_opcodes.py
diff --git a/tests/core/vm/test_base_computation.py b/tests/core/vm/test_base_computation.py
diff --git a/tests/json-fixtures/test_virtual_machine.py b/tests/json-fixtures/test_virtual_machine.py

Original file line number	Diff line number	Diff line change
`@@ -216,7 +216,7 @@ def execute_bytecode(self,`
`216`	`216`	`)`
`217`	`217`
`218`	`218`	`# Execute it in the VM`
`219`		`- return self.state.get_computation(message, transaction_context).apply_computation(`
	`219`	`+ return self.state.computation_class.apply_computation(`
`220`	`220`	`self.state,`
`221`	`221`	`message,`
`222`	`222`	`transaction_context,`