Skip to content

[WIP] debug_traceTransaction endpoint implementation #1709

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[WIP] debug_traceTransaction endpoint implementation #1709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
843314f
[WIP] debug_traceTransaction endpoint implementation
Nov 24, 2018
1bcbdca
eliminate use of classmethod on instance of computation
pipermerriam Jan 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 28 additions & 12 deletions eth/vm/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,15 @@
validate_length_lte,
validate_gas_limit,
)
from eth.vm.computation import BaseComputation
from eth.vm.message import (
Message,
)
from eth.vm.state import BaseState
from eth.vm.computation import BaseComputation
from eth.vm.tracing import (
BaseTracer,
NoopTracer,
)


class BaseVM(Configurable, ABC):
Expand Down Expand Up @@ -111,8 +115,9 @@ def logger(self) -> logging.Logger:
@abstractmethod
def apply_transaction(self,
header: BlockHeader,
transaction: BaseTransaction
) -> Tuple[BlockHeader, Receipt, BaseComputation]:
transaction: BaseTransaction,
*,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of my python experience is with 2.7. I'm still a little bitter about reduce and 3113 but wow, this is a nice feature.

tracer: BaseTracer=None) -> Tuple[BlockHeader, Receipt, BaseComputation]:
raise NotImplementedError("VM classes must implement this method")

@abstractmethod
Expand Down Expand Up @@ -397,17 +402,22 @@ def logger(self) -> logging.Logger:
#
def apply_transaction(self,
header: BlockHeader,
transaction: BaseTransaction
) -> Tuple[BlockHeader, Receipt, BaseComputation]:
transaction: BaseTransaction,
*,
tracer: BaseTracer=None) -> Tuple[BlockHeader, Receipt, BaseComputation]:
"""
Apply the transaction to the current block. This is a wrapper around
:func:`~eth.vm.state.State.apply_transaction` with some extra orchestration logic.

:param header: header of the block before application
:param transaction: to apply
"""
if tracer is None:
tracer = NoopTracer()

self.validate_transaction_against_header(header, transaction)
state_root, computation = self.state.apply_transaction(transaction)
with self.state.trace(tracer):
state_root, computation = self.state.apply_transaction(transaction)
receipt = self.make_receipt(header, transaction, computation, self.state)
self.validate_receipt(receipt)

Expand All @@ -429,14 +439,18 @@ def execute_bytecode(self,
data: bytes,
code: bytes,
code_address: Address=None,
) -> BaseComputation:
*,
tracer: BaseTracer=None) -> BaseComputation:
"""
Execute raw bytecode in the context of the current state of
the virtual machine.
"""
if origin is None:
origin = sender

if tracer is None:
tracer = NoopTracer()

# Construct a message
message = Message(
gas=gas,
Expand All @@ -455,11 +469,13 @@ def execute_bytecode(self,
)

# Execute it in the VM
return self.state.get_computation(message, transaction_context).apply_computation(
self.state,
message,
transaction_context,
)
with self.state.trace(tracer):
Copy link
Contributor

@lithp lithp Jan 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this call to trace is unnecessary. Neither get_computation_class nor apply_computation use state's tracer attribute.

If it is necessary, it feels ugly to have to pass tracer twice, once here and once in the call to apply_computation.

return self.state.get_computation_class().apply_computation(
self.state,
message,
transaction_context,
tracer,
)

def apply_all_transactions(
self,
Expand Down
99 changes: 62 additions & 37 deletions eth/vm/computation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from abc import (
ABC,
abstractmethod
abstractmethod,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you catch this manually or you have a tool you run to manage your imports?

)
import itertools
import logging
Expand All @@ -10,6 +10,7 @@
cast,
Dict,
List,
Optional,
Tuple,
Union,
)
Expand All @@ -20,7 +21,6 @@
from eth_utils import (
encode_hex,
)

from eth.constants import (
GAS_MEMORY,
GAS_MEMORY_QUADRATIC_DENOMINATOR,
Expand All @@ -46,33 +46,18 @@
validate_is_bytes,
validate_uint256,
)
from eth.vm.code_stream import (
CodeStream,
)
from eth.vm.gas_meter import (
GasMeter,
)
from eth.vm.logic.invalid import (
InvalidOpcode,
)
from eth.vm.memory import (
Memory,
)
from eth.vm.message import (
Message,
)
from eth.vm.opcode import ( # noqa: F401
Opcode
)
from eth.vm.stack import (
Stack,
)
from eth.vm.state import (
BaseState,
)
from eth.vm.transaction_context import (
BaseTransactionContext
from eth.vm.code_stream import CodeStream
from eth.vm.gas_meter import GasMeter
from eth.vm.logic.invalid import InvalidOpcode
from eth.vm.memory import Memory
from eth.vm.message import Message
from eth.vm.opcode import Opcode
from eth.vm.stack import Stack
from eth.vm.state import BaseState
from eth.vm.tracing import (
BaseTracer,
Copy link
Contributor

@lithp lithp Jan 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why leave this one expanded if it's also importing just a single name?

)
from eth.vm.transaction_context import BaseTransactionContext


def memory_gas_cost(size_in_bytes: int) -> int:
Expand Down Expand Up @@ -125,7 +110,8 @@ class BaseComputation(Configurable, ABC):
def __init__(self,
state: BaseState,
message: Message,
transaction_context: BaseTransactionContext) -> None:
transaction_context: BaseTransactionContext,
tracer: BaseTracer) -> None:

self.state = state
self.msg = message
Expand All @@ -142,6 +128,8 @@ def __init__(self,
code = message.code
self.code = CodeStream(code)

self.tracer = tracer

#
# Convenience
#
Expand All @@ -152,9 +140,31 @@ def is_origin_computation(self) -> bool:
"""
return self.msg.sender == self.transaction_context.origin

#
# Program Counter
#
def get_pc(self) -> int:
return max(0, self.code.pc - 1)

#
# Error handling
#
@property
def error(self) -> Optional[VMError]:
if self.is_error:
return self._error
else:
return None

@error.setter
def error(self, value: VMError) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eth/vm/forks/frontier/state.py:build_computation() also sets _error, and should probably now use error

https://github.com/ethereum/py-evm/pull/1709/files#diff-f2f498681e795533042b5ff0de3783b7L121

if not isinstance(value, VMError):
raise TypeError(
"Computation.error can only be set to a VMError subclass. Got: "
"{0}".format(value)
)
self._error = value

@property
def is_success(self) -> bool:
"""
Expand All @@ -175,15 +185,15 @@ def raise_if_error(self) -> None:

:raise VMError:
"""
if self._error is not None:
raise self._error
if self.is_error:
raise self.error

@property
def should_burn_gas(self) -> bool:
"""
Return ``True`` if the remaining gas should be burned.
"""
return self.is_error and self._error.burns_gas
return self.is_error and self.error.burns_gas
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: self.is_error indirectly does a check that error exists but I can see that accidentally breaking in a refactor (the computation hasn't run between __init__ and apply_message so some future soul might have is_success only return true once the computation has finished. If that change were to be made then before the call to apply_message: is_success would be False but error would also be None)

This would be slightly safer if it was instead:

return self.error and self.error.burns_gas

I suppose the same argument applies to the above change. Slightly safer would be:

if self.error:
  raise self.error


@property
def should_return_gas(self) -> bool:
Expand Down Expand Up @@ -258,6 +268,9 @@ def memory_read_bytes(self, start_position: int, size: int) -> bytes:
"""
return self._memory.read_bytes(start_position, size)

def dump_memory(self) -> bytes:
return self._memory.read_bytes(0, len(self._memory))

#
# Gas Consumption
#
Expand Down Expand Up @@ -341,6 +354,9 @@ def stack_dup(self, position: int) -> None:
"""
return self._stack.dup(position)

def dump_stack(self) -> Tuple[int, ...]:
return tuple(self._stack) # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: the # type: ignore here is hiding what I think is a real problem. Stack.__iter__() returns a generator which is an Iterator, not an Iterable, so the type annotation there is wrong. When that annotation is corrected this ignore is not longer necessary.


#
# Computation result
#
Expand Down Expand Up @@ -402,12 +418,14 @@ def generate_child_computation(self, child_msg: Message) -> 'BaseComputation':
self.state,
child_msg,
self.transaction_context,
self.tracer,
).apply_create_message()
else:
child_computation = self.__class__(
self.state,
child_msg,
self.transaction_context,
self.tracer,
).apply_message()
return child_computation

Expand Down Expand Up @@ -511,7 +529,7 @@ def __exit__(self, exc_type: None, exc_value: None, traceback: None) -> None:
"y" if self.msg.is_static else "n",
exc_value,
)
self._error = exc_value
self.error = exc_value
if self.should_burn_gas:
self.consume_gas(
self._gas_meter.gas_remaining,
Expand Down Expand Up @@ -559,30 +577,37 @@ def apply_create_message(self) -> 'BaseComputation':
def apply_computation(cls,
state: BaseState,
message: Message,
transaction_context: BaseTransactionContext) -> 'BaseComputation':
transaction_context: BaseTransactionContext,
tracer: BaseTracer) -> 'BaseComputation':
"""
Perform the computation that would be triggered by the VM message.
"""
with cls(state, message, transaction_context) as computation:
with cls(state, message, transaction_context, tracer) as computation:
# Early exit on pre-compiles
if message.code_address in computation.precompiles:
computation.precompiles[message.code_address](computation)
return computation

for opcode in computation.code:
opcode_fn = computation.get_opcode_fn(opcode)
pc = computation.get_pc()

computation.logger.debug2(
"OPCODE: 0x%x (%s) | pc: %s",
opcode,
opcode_fn.mnemonic,
max(0, computation.code.pc - 1),
pc,
)

try:
opcode_fn(computation=computation)
with computation.tracer.capture(computation, opcode_fn):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not thrilled with the overhead this incurs for EVM execution but I'm inclined to leave this be and optimize later when we get around to actual EVM optimizations.

opcode_fn(computation=computation)
except Halt:
break

# Allow the tracer to record final values.
computation.tracer.finalize(computation)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm exhausted my timebox but I was planning at looking at this next. I think this fails if there are any child computations? When the child's apply_computation finishes the tracer is finalized. It then returns to the parent which attempts to continue tracing and the tracer will complain that it's already been finalized.


return computation

#
Expand Down
1 change: 1 addition & 0 deletions eth/vm/forks/frontier/computation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ def apply_message(self) -> BaseComputation:
self.state,
self.msg,
self.transaction_context,
self.tracer,
)

if computation.is_error:
Expand Down
24 changes: 17 additions & 7 deletions eth/vm/forks/frontier/state.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@
BaseState,
BaseTransactionExecutor,
)
from eth.vm.tracing import (
BaseTracer,
NoopTracer,
)


from .computation import FrontierComputation
Expand Down Expand Up @@ -106,8 +110,12 @@ def build_evm_message(self, transaction: BaseOrSpoofTransaction) -> Message:

def build_computation(self,
message: Message,
transaction: BaseOrSpoofTransaction) -> BaseComputation:
transaction: BaseOrSpoofTransaction,
tracer: BaseTracer=None) -> BaseComputation:
"""Apply the message to the VM."""
if tracer is None:
tracer = NoopTracer()

transaction_context = self.vm_state.get_transaction_context(transaction)
if message.is_create:
is_collision = self.vm_state.account_db.account_has_code_or_nonce(
Expand All @@ -128,14 +136,16 @@ def build_computation(self,
encode_hex(message.storage_address),
)
else:
with self.vm_state.trace(tracer):
computation = self.vm_state.get_computation(
message,
transaction_context,
).apply_create_message()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative might be to pass the tracer into get_computation or apply_create_message

else:
with self.vm_state.trace(tracer):
computation = self.vm_state.get_computation(
message,
transaction_context,
).apply_create_message()
else:
computation = self.vm_state.get_computation(
message,
transaction_context).apply_message()
transaction_context).apply_message()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: This closing paren should be on its own line


return computation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apply_message() calls apply_computation which throws an exception if the tracer does not exist. This function returns BaseComputation's which have an apply_message method but if that message is ever called an exception is thrown. The caller of this method will probably never call apply_message but this is still a smell.


Expand Down
Loading