[WIP] debug_traceTransaction endpoint implementation #1709
Conversation
There was a problem hiding this comment.
Making note that the structure of EVM trace output is not exactly standardized and it'd be ideal if we went ahead an authored an EIP which defined the schema for this data. Worth also linking to ethereum/EIPs#1474 with aims to standardize the eth_ namespaced RPC commands.
| ) | ||
|
|
||
| try: | ||
| opcode_fn(computation=computation) | ||
| with computation.tracer.capture(computation, opcode_fn): |
There was a problem hiding this comment.
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.
| @@ -1,6 +1,6 @@ | |||
| from abc import ( | |||
| ABC, | |||
| abstractmethod | |||
| abstractmethod, | |||
There was a problem hiding this comment.
Did you catch this manually or you have a tool you run to manage your imports?
| transaction: BaseTransaction | ||
| ) -> Tuple[BlockHeader, Receipt, BaseComputation]: | ||
| transaction: BaseTransaction, | ||
| *, |
There was a problem hiding this comment.
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.
| from eth.vm.stack import Stack | ||
| from eth.vm.state import BaseState | ||
| from eth.vm.tracing import ( | ||
| BaseTracer, |
There was a problem hiding this comment.
Why leave this one expanded if it's also importing just a single name?
|
|
||
| @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 |
There was a problem hiding this comment.
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_gasI suppose the same argument applies to the above change. Slightly safer would be:
if self.error:
raise self.error| @@ -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 | |||
There was a problem hiding this comment.
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.
| @@ -35,6 +36,18 @@ def __init__(self) -> None: | |||
| def __len__(self) -> int: | |||
| return len(self.values) | |||
|
|
|||
| def __iter__(self) -> Iterable[int]: | |||
There was a problem hiding this comment.
Here's the hint which should be changed to Iterator[int]
There was a problem hiding this comment.
Also, why convert everything to int instead of returning Iterator[Union[int, bytes]]? It doesn't look like tracer relies on receiving just ints.
| computation = self.vm_state.get_computation( | ||
| message, | ||
| transaction_context).apply_message() | ||
| transaction_context).apply_message() |
There was a problem hiding this comment.
nitpick: This closing paren should be on its own line
| return None | ||
|
|
||
| @error.setter | ||
| def error(self, value: VMError) -> None: |
There was a problem hiding this comment.
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
| # | ||
| @contextlib.contextmanager | ||
| def trace(self, tracer: BaseTracer) -> Iterator[None]: |
There was a problem hiding this comment.
I think the idea behind this context manager is that BaseState instances will outlive transactions, so tracer should only be set while it's running a transaction. But I think that's really a sign that tracer shouldn't be an attribute of BaseState at all.
Using a context manager for this seems error-prone and unnecessary. It's error-prone because you have to remember to use this whenever you're calling a method which might need the tracer. If tracer was passed in as a parameter then the mistake of not passing one would be much harder to make.
It's unnecessary because this code seems to take two approaches simultaneously. It both adds the tracer to vm_state, making it available nearly everywhere, and it also manually threads tracer through all the calls. Because the threading has already been done, it's simpler to use that tracer everywhere.
| @@ -299,7 +316,7 @@ def __init__(self, vm_state: BaseState) -> None: | |||
| def __call__(self, transaction: BaseOrSpoofTransaction) -> 'BaseComputation': | |||
| valid_transaction = self.validate_transaction(transaction) | |||
| message = self.build_evm_message(valid_transaction) | |||
| computation = self.build_computation(message, valid_transaction) | |||
| computation = self.build_computation(message, valid_transaction, self.vm_state.tracer) | |||
There was a problem hiding this comment.
Here's somewhere it'd be difficult to thread the tracer too, this might be the origin of the context manager? I've exhausted my timebox for looking into this PR so I don't have an answer but I expect that getting a tracer here isn't insurmountable.
Looking at this now, BaseState is long-lived but Message isn't, maybe it's a better place to attach the tracer?
| computation = self.vm_state.get_computation( | ||
| message, | ||
| transaction_context).apply_message() | ||
| transaction_context).apply_message() | ||
|
|
||
| return computation |
There was a problem hiding this comment.
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.
| computation = self.vm_state.get_computation( | ||
| message, | ||
| transaction_context, | ||
| ).apply_create_message() |
There was a problem hiding this comment.
An alternative might be to pass the tracer into get_computation or apply_create_message
| except Halt: | ||
| break | ||
|
|
||
| # Allow the tracer to record final values. | ||
| computation.tracer.finalize(computation) |
There was a problem hiding this comment.
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.
Replaces: #1515 , removing all of the
trinitycomponents which will be added in a separate PR to thetrinityrepo once this is merged and released.What was wrong?
https://github.com/ethereum/go-ethereum/wiki/Tracing:-Introduction
Adds support for tracing EVM transactions.
How was it fixed?
This adds a new API that can be used to trace transactions by supplying a
tracertoVM.apply_transactionCute Animal Picture