debug_traceTransaction endpoint implementation #1515

rayrapetyan · 2018-11-24T18:17:33Z

What was wrong?

There was no way to trace transactions in py-evm

How was it fixed?

A basic tracing support was added.

Description

New functionality is based on a go-ethereum's debug_traceTransaction logic (https://github.com/ethereum/go-ethereum/wiki/Tracing:-Introduction). Only basic parts were ported so far (e.g. no JS tracers support). Closes issue #62.

List of open questions to discuss:

Should we drop output of computation.logger.trace as it mostly duplicates a new tracer's output?
Find a better way to turn tracer on\off (right now it's a member of VM's state class and we just check if it's "not None" from a computation instance). Another way could be passing Tracer as param all the way down in chain of calls: VM.state.apply_transaction() -> execute_transaction() -> TransactionExecutor.build_computation() -> computation.apply_computation().
Cross-compare output with go-ethereum tests. Problem is I can't find any tests related to non-JS (basic) tracer there. I've performed cross-comparison of multiple transactions and output is identical except few bugs in geth: e.g. sometimes it initializes memory with zero after push into stack, or do not populate error field in case error fired etc).
"gas" field in response represents a total block gas usage. I believe this is a bug in geth implementation and for traceTransaction should be equal to the gas used purely by a traced transaction, not the whole block.
Putting main "capture_state" into "finally" block allows us to catch both error and success cases in one place. Also it's the only way I've found to store a valid "cost" amount. In the future we shall move "cost" estimation calculations into Opcode's "gas_cost" method (see "operation's gasCost gasFunc" implementation in geth: https://github.com/ethereum/go-ethereum/blob/master/core/vm/jump_table.go).

eth/vm/state.py

eth/vm/computation.py

eth/tools/tracing.py

pipermerriam

Incomplete review, will try to get you a full review tomorrow.

eth/tools/tracing.py

pipermerriam · 2018-11-29T20:24:03Z

@rayrapetyan I started reviewing and realized that there were somewhat significant architectural changes that I wanted but needed to figure out via code.

Take a look at #1515

Wondering if you feel confident picking that up and continuing from there?

rayrapetyan · 2018-12-01T18:58:43Z

Thanks @pipermerriam, I'll continue here using your changes.

pipermerriam · 2018-12-04T19:22:47Z

@rayrapetyan 👍 let me know if you need anything from me.

pipermerriam

It'd be nice to get some additional tests added for this to demonstrate tracing situations that aren't boring simple value transfers (which don't excercise the actual tracing core logic).

pipermerriam · 2018-12-06T17:30:07Z

eth/vm/computation.py

@@ -125,7 +106,8 @@ class BaseComputation(Configurable, ABC):
    def __init__(self,
                 state: BaseState,
                 message: Message,
-                 transaction_context: BaseTransactionContext) -> None:
+                 transaction_context: BaseTransactionContext,
+                 tracer: BaseTracer = None) -> None:


I've been struggling with whether the tracer should be a required parameter and I'm leaning towards yes. My thought is that I already had a situation where my tests were failing because I thought I was using a real tracer but a code path hadn't been updated properly so it was defaulting to the NoopTracer. By making this a required argument we can't make that mistake.

@pipermerriam, I've made it required. As per tests - do you think keeping expected JSON-trace and then simply comparing it with a result will be a good test for that?

p.s. py35-lint went crazy lol - it blames to one line which passes checks in master...
It behaves really strange, e.g. we have a lot of formatted strings like f"...", which "...are only supported in Python 3.6 and greater", how does master passes py35-lint checks then?

The eth module still has to support python 3.5 so we don't use f-strings there.

My apologies if I was the one who introduced any of those 😸

@pipermerriam, so why py35-lint is launching on a trinity module then? It fails on:
__version__: str at the top of __init__.py.
It somehow passes same on master branch...

Ah! py35-lint should not be run on the trinity module. That is an error/mistake.

@pipermerriam. Weird, I don't see any diff for tox.ini in master\tracer branches, but somehow master passes all tests, and tracer fails one py35-lint test here.

pipermerriam · 2018-12-13T21:13:35Z

I took some additional liberties here: https://github.com/rayrapetyan/py-evm/pull/2

eth/vm/tracing.py

rayrapetyan · 2018-12-17T15:37:39Z

@pipermerriam, awaiting your responses in https://github.com/rayrapetyan/py-evm/pull/2. Thanks.

Piper/evm tracer

# Conflicts: # eth/vm/computation.py

rayrapetyan · 2018-12-17T22:46:07Z

@pipermerriam, please review and merge.

carver · 2018-12-18T20:09:27Z

This commit history is pretty messy. Are you up for doing rebase/squash and force push?

Also, best to remove [WIP] from the PR title if you're ready for the final review before merging. That is one signal we use for readiness.

rayrapetyan · 2018-12-18T20:14:51Z

@carver, removed WIP, please review and if everything is OK I'll squash. Thanks.

carver

Mostly minor comments.

One thing holding it back is having a test with a trace over json-rpc with at least one failing and one succeeding transaction. Some examples with other endpoints: https://github.com/ethereum/py-evm/blob/master/tests/trinity/core/json-rpc/test_ipc.py

Alternatively, you could split out the json-rpc part into a second PR to work on those tests, and I think this would be basically ready to go. (Assuming Piper has no objections, of course). I think that would be preferred, since it's already a long PR, but not a hard requirement.

eth/vm/computation.py

eth/vm/memory.py

eth/vm/stack.py

pipermerriam · 2018-12-18T23:40:46Z

@carver I'm a little too close to this issue (as I did some significant rewriting along the way) so I'm good delegating to you for final merge.

pipermerriam · 2018-12-18T23:42:39Z

trinity/rpc/modules/debug.py

+        i = 0
+        while i + 32 <= len(e.memory):
+            entry['memory'].append(remove_0x_prefix(encode_hex(e.memory[i: i + 32])))
+            i += 32


This is a section I'd like to cleanup but it doesn't have to be in this PR. I'm actually 👍 on removing all of the JSON-RPC stuff from this PR and doing it in a subsequent one (and just merging the core tracer API in this PR.

Co-Authored-By: rayrapetyan <[email protected]>

rayrapetyan · 2018-12-19T05:46:41Z

@carver, if possible - please merge on your side. I swear never merge master into dev branches anymore :)

pipermerriam · 2018-12-19T19:47:37Z

@rayrapetyan in case you aren't aware: https://help.github.com/articles/about-git-rebase/

git rebase master is what you're looking for but it takes a little bit of a learning curve to deal with merge conflicts properly.

pipermerriam

@carver I'm 👍 but I want to take this branch and pull out the trinity parts so we can iterate on those separately rather than merging them as-is.

rayrapetyan · 2018-12-19T19:57:57Z

@pipermerriam, I would be glad to merge, but most of the changes there are yours so I can miss\overlook something. As for trinity parts - I guess you mean formatting function, I would leave it as is with a TODO mark. while i + 32 <= len(e.memory): part is exactly how it was done in geth. And btw unassign me from bounty - this was primarily your job done here. Thanks.

pipermerriam · 2018-12-19T20:02:15Z

@rayrapetyan I'm fine with you collecting the bounty.

you laid the foundation for what I did
you still did a significant part of the work
I like contributors being paid though I appreciate you willingness to contribute for free.

pipermerriam · 2018-12-19T20:07:05Z

@rayrapetyan it's less about that specific line of code and more 1) wanting to give the whole implementation of the RPC endpoint another once-over and 2) wanting to convert that entire normalization function into something more inline with our code style (functional and immutable).

carver

Needs a rebase

[WIP] debug_traceTransaction endpoint implementation

c606174

rayrapetyan commented Nov 25, 2018

View reviewed changes

eth/vm/state.py Outdated Show resolved Hide resolved

rayrapetyan commented Nov 25, 2018

View reviewed changes

eth/vm/computation.py Outdated Show resolved Hide resolved

rayrapetyan commented Nov 25, 2018

View reviewed changes

eth/tools/tracing.py Outdated Show resolved Hide resolved

pipermerriam reviewed Nov 26, 2018

View reviewed changes

eth/tools/tracing.py Outdated Show resolved Hide resolved

eth/tools/tracing.py Outdated Show resolved Hide resolved

eth/tools/tracing.py Outdated Show resolved Hide resolved

pipermerriam mentioned this pull request Nov 29, 2018

[WIP] vm tracing #1531

Closed

robert and others added 2 commits November 29, 2018 13:19

[WIP] debug_traceTransaction endpoint implementation

f761881

Adjust implementation to be inline with Py-EVM architecture

bdb0d50

gitcoinbot mentioned this pull request Dec 5, 2018

Modify *trace* logging format to conform to spec #62

Closed

robert added 3 commits December 5, 2018 12:49

Merge branch 'master' into tracer

e55b1af

changes based on extension #1531

e1df86a

changes based on extension #1531

7f27c5e

pipermerriam reviewed Dec 6, 2018

View reviewed changes

robert and others added 6 commits December 7, 2018 13:24

Merge branch 'master' into tracer

239d5ba

py35 tests fixes

3a43c3a

properly return precomputations

2d494ae

Merge branch 'master' into tracer

54d587d

d

b916287

A real test for tracing a transaction

5720292

carver reviewed Dec 13, 2018

View reviewed changes

eth/vm/tracing.py Outdated Show resolved Hide resolved

pipermerriam reviewed Dec 13, 2018

View reviewed changes

eth/vm/tracing.py Outdated Show resolved Hide resolved

rayrapetyan and others added 4 commits December 17, 2018 09:17

Merge pull request #2 from pipermerriam/piper/evm-tracer

14129d1

Piper/evm tracer

fixes ref https://github.com/rayrapetyan/py-evm/pull/2

d7318cf

Merge branch 'master' into tracer

04a9481

# Conflicts: # eth/vm/computation.py

new vm tests fixes

ea889f3

rayrapetyan changed the title ~~[WIP] debug_traceTransaction endpoint implementation~~ debug_traceTransaction endpoint implementation Dec 18, 2018

carver reviewed Dec 18, 2018

View reviewed changes

pipermerriam reviewed Dec 18, 2018

View reviewed changes

carver and others added 5 commits December 18, 2018 19:59

Update eth/vm/computation.py

788a61a

Co-Authored-By: rayrapetyan <[email protected]>

Update eth/vm/computation.py

2c4f617

Co-Authored-By: rayrapetyan <[email protected]>

Update eth/vm/stack.py

eb6dc11

Co-Authored-By: rayrapetyan <[email protected]>

debug_traceTransaction endpoint implementation (13)

666d32a

debug_traceTransaction endpoint implementation (13)

53fdd02

pipermerriam approved these changes Dec 19, 2018

View reviewed changes

This was referenced Jan 9, 2019

[WIP] Add Stack and Computation Benchmark #1707

Open

[WIP] debug_traceTransaction endpoint implementation #1709

Open

Implement debug_traceTransaction RPC endpoint ethereum/trinity#124

Closed

carver requested changes May 20, 2019

View reviewed changes

pacrob deleted the branch ethereum:master December 21, 2023 02:56

pacrob closed this Dec 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

debug_traceTransaction endpoint implementation #1515

debug_traceTransaction endpoint implementation #1515

rayrapetyan commented Nov 24, 2018 •

edited

Loading

pipermerriam left a comment

pipermerriam commented Nov 29, 2018

rayrapetyan commented Dec 1, 2018

pipermerriam commented Dec 4, 2018

pipermerriam left a comment

pipermerriam Dec 6, 2018

rayrapetyan Dec 8, 2018

pipermerriam Dec 8, 2018 •

edited

Loading

rayrapetyan Dec 11, 2018 •

edited

Loading

pipermerriam Dec 11, 2018

rayrapetyan Dec 11, 2018

pipermerriam commented Dec 13, 2018

rayrapetyan commented Dec 17, 2018

rayrapetyan commented Dec 17, 2018

carver commented Dec 18, 2018

rayrapetyan commented Dec 18, 2018

carver left a comment

pipermerriam commented Dec 18, 2018

pipermerriam Dec 18, 2018

rayrapetyan commented Dec 19, 2018

pipermerriam commented Dec 19, 2018

pipermerriam left a comment

rayrapetyan commented Dec 19, 2018

pipermerriam commented Dec 19, 2018

pipermerriam commented Dec 19, 2018

carver left a comment

debug_traceTransaction endpoint implementation #1515

debug_traceTransaction endpoint implementation #1515

Conversation

rayrapetyan commented Nov 24, 2018 • edited Loading

What was wrong?

How was it fixed?

Description

pipermerriam left a comment

Choose a reason for hiding this comment

pipermerriam commented Nov 29, 2018

rayrapetyan commented Dec 1, 2018

pipermerriam commented Dec 4, 2018

pipermerriam left a comment

Choose a reason for hiding this comment

pipermerriam Dec 6, 2018

Choose a reason for hiding this comment

rayrapetyan Dec 8, 2018

Choose a reason for hiding this comment

pipermerriam Dec 8, 2018 • edited Loading

Choose a reason for hiding this comment

rayrapetyan Dec 11, 2018 • edited Loading

Choose a reason for hiding this comment

pipermerriam Dec 11, 2018

Choose a reason for hiding this comment

rayrapetyan Dec 11, 2018

Choose a reason for hiding this comment

pipermerriam commented Dec 13, 2018

rayrapetyan commented Dec 17, 2018

rayrapetyan commented Dec 17, 2018

carver commented Dec 18, 2018

rayrapetyan commented Dec 18, 2018

carver left a comment

Choose a reason for hiding this comment

pipermerriam commented Dec 18, 2018

pipermerriam Dec 18, 2018

Choose a reason for hiding this comment

rayrapetyan commented Dec 19, 2018

pipermerriam commented Dec 19, 2018

pipermerriam left a comment

Choose a reason for hiding this comment

rayrapetyan commented Dec 19, 2018

pipermerriam commented Dec 19, 2018

pipermerriam commented Dec 19, 2018

carver left a comment

Choose a reason for hiding this comment

rayrapetyan commented Nov 24, 2018 •

edited

Loading

pipermerriam Dec 8, 2018 •

edited

Loading

rayrapetyan Dec 11, 2018 •

edited

Loading