feat: complete vm and statediff tracers #3529

nhtyy · 2023-07-01T21:09:28Z

addresses issue #2577

todo! i need to get all the bytecodes from the db
will likely need to use an auxillary struct or add fields to the builder so i can do a dfs on the calls to track the addresses, since they arent kept around the in VmTrace

i would say this pr is at ~90% of the way done, it just needs tests and i need to fill in the (base?) gas cost of the opcode

…he builder

nhtyy · 2023-07-02T17:18:11Z

crates/revm/revm-inspectors/src/tracing/builder/parity.rs

i still need to respect the config here, but the rest of the file doesnt seem to use the config yet + logging

gakonst

good start - supportive of this. thank you for taking the time. anything else you'd like to add before us taking a deeper look on this?

crates/revm/revm-inspectors/src/tracing/builder/parity.rs

mattsse

tysm for taking this on, very much appreciated.

I can follow how this works but I have a few requests.

crates/revm/revm-inspectors/src/tracing/builder/mod.rs

crates/revm/revm-inspectors/src/tracing/builder/parity.rs

mattsse · 2023-07-03T11:47:07Z

crates/revm/revm-inspectors/src/tracing/builder/walker.rs

+        holder.push(idx);
+        for child_idx in nodes[idx].children.iter() {
+            Self::get_all_children(nodes, holder, *child_idx);
+        }


please no recursive calls, this can be written iteratively

yup - context is these overflow for deep traces

got it....

I also use recursion to create the VmTrace, I should also try to change that?

but for now will work on making the CallTraceNodeWalker iterative :)

nhtyy · 2023-07-05T04:28:25Z

https://github.com/bluealloy/revm/blob/10f81ba1b3cd6171c25d3346f1800bbb04f5ead1/crates/interpreter/src/gas/constants.rs does this work?

@gakonst i think may need to export this actually https://github.com/bluealloy/revm/blob/10f81ba1b3cd6171c25d3346f1800bbb04f5ead1/crates/interpreter/src/instructions/opcode.rs#L833C1-L833C1

mattsse

great progress, left a few suggestions

btw spec_opcode_gas is public and you should be able to import it

crates/revm/revm-inspectors/src/tracing/builder/parity.rs

crates/revm/revm-inspectors/src/tracing/builder/walker.rs

Co-authored-by: Matthias Seitz <[email protected]>

…trace

nhtyy · 2023-07-05T17:46:26Z

sweeet so i was able to make everything iterative it seems!

I think is ready for more review now while i can start on testing soon (or vice versa)

also on the Walker types over CallTraceNodes that i made, i only ended up using the breadth first one but theres depth first one also, so not sure if thats something we want to keep around for whatever reasons, i just commented it out for now

also on opcode thing i dont think i can get it because its not used here and no pub mod
https://github.com/bluealloy/revm/blob/10f81ba1b3cd6171c25d3346f1800bbb04f5ead1/crates/interpreter/src/lib.rs#L20

mattsse

this looks pretty good already.

left a few nits

there are a few chunks that are commented out,
what is missing here?

I agree we can start testing this

mattsse · 2023-07-07T10:09:21Z

crates/revm/revm-inspectors/src/tracing/builder/walker.rs

+                for child_idx in curr.children.iter() {
+                    self.queue.push_back(*child_idx);
+                }


can use .extend here

oooo nice thx fixed

mattsse · 2023-07-07T10:09:35Z

crates/revm/revm-inspectors/src/tracing/builder/walker.rs

+};
+use revm::interpreter::opcode;
+use std::collections::VecDeque;
+


Suggested change

mattsse · 2023-07-07T10:12:27Z

crates/revm/revm-inspectors/src/tracing/builder/parity.rs

+        let instructions = loop {
+            match current.children.get(child_idx) {
+                Some(child) => {
+                    child_idx_stack.push(child_idx + 1);


why the +1 here?

ah to read the next child when it gets popped from the stack, i could pop from the stack then increment, not sure which u prefer

mattsse · 2023-07-07T10:13:38Z

crates/revm/revm-inspectors/src/tracing/builder/parity.rs

+    breadth_first_addresses: I,
+    trace: &mut VmTrace,


nit: swap the order here, it's better to have iter types as last argument

mattsse · 2023-07-07T10:15:43Z

crates/revm/revm-inspectors/src/tracing/builder/parity.rs

+                curr_ref.code = match db_acc.code {
+                    Some(code) => code.bytecode.into(),
+                    None => Default::default(),
+                };


I think we need to use code_by_hash here if the code_hash is not KECCAK_EMPTY and code is none

because code is stored separately

mattsse · 2023-07-07T10:16:05Z

crates/revm/revm-inspectors/src/tracing/types.rs

+                opcode::CREATE
+                | opcode::CREATE2
+                | opcode::DELEGATECALL
+                | opcode::CALL
+                | opcode::STATICCALL
+                | opcode::CALLCODE => {


I'd like to make this a helper function

i think this comment was to be on the function vm_trace in src/tracing/builder/parity? sorry i realize my fmt touched this when it shouldnt have

if u can confirm i can fix

…trace

nhtyy · 2023-07-11T00:25:12Z

also on opcode thing i dont think i can get it because its not used here and no pub mod
https://github.com/bluealloy/revm/blob/10f81ba1b3cd6171c25d3346f1800bbb04f5ead1/crates/interpreter/src/lib.rs#L20

getting the spec gas costs from revm ^, and there is aTracingInspectorConfig that is passed to the parity builder that has fields that maybe impact some of this behavior, but its not ever read or respected by other parts of the file it seems (like getting state diff)

reth/crates/revm/revm-inspectors/src/tracing/builder/parity.rs

Line 24 in 08d8d40

_config: TracingInspectorConfig,

thats it!

mattsse

sorry for the delay, lost the notification -.-

again, tysm for doing this!!

this is awesome.

let's try this and then run a few more checks/diffs

mattsse · 2023-07-12T17:45:07Z

crates/revm/revm-inspectors/src/tracing/builder/parity.rs

+
+        VmInstruction {
+            pc: step.pc,
+            cost: 0, // todo::n


what to do here?

this is the gas cost of the opcode?
we should be able to get this via spec_opcode_gas

I think we need to get the spec id from the env during tracing.

I can add this in a follow up

this is the gas cost of the opcode?

yessir

codecov · 2023-07-12T18:17:22Z

Codecov Report

Merging #3529 (38c1483) into main (9924090) will decrease coverage by 21.36%.
The diff coverage is 0.00%.

Impacted Files	Coverage Δ
...revm/revm-inspectors/src/tracing/builder/parity.rs	`0.00% <0.00%> (ø)`
...revm/revm-inspectors/src/tracing/builder/walker.rs	`0.00% <0.00%> (ø)`
crates/revm/revm-inspectors/src/tracing/mod.rs	`0.00% <0.00%> (ø)`
crates/revm/revm-inspectors/src/tracing/types.rs	`0.00% <ø> (ø)`

... and 251 files with indirect coverage changes

Flag	Coverage Δ
integration-tests	`13.47% <0.00%> (-2.34%)`	⬇️
unit-tests	`43.86% <0.00%> (-20.53%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
reth binary	`17.80% <ø> (-8.49%)`	⬇️
blockchain tree	`29.30% <ø> (-53.41%)`	⬇️
pipeline	`66.86% <ø> (-20.27%)`	⬇️
storage (db)	`51.47% <ø> (-22.72%)`	⬇️
trie	`58.69% <ø> (-35.97%)`	⬇️
txpool	`35.62% <ø> (-13.17%)`	⬇️
networking	`53.51% <ø> (-24.32%)`	⬇️
rpc	`41.99% <ø> (-16.45%)`	⬇️
consensus	`42.72% <ø> (-22.15%)`	⬇️
revm	`21.31% <0.00%> (-13.51%)`	⬇️
payload builder	`6.83% <ø> (ø)`
primitives	`63.77% <ø> (-24.59%)`	⬇️

nhtyy · 2023-07-12T18:21:20Z

yayy! great experience hope to be back!

Co-authored-by: N <[email protected]> Co-authored-by: Matthias Seitz <[email protected]>

N added 18 commits July 1, 2023 16:46

missing stack logic

772d9c1

track new stack in trace step, use it in vm trace conversion

892340f

comment

9eb1935

add todo

86c4cfa

fix

2ff63de

introduce graphwalker logic, move somethings to asscioated types on t…

b60a612

…he builder

helper function to get all the address from a DF walk

db0b459

code layout

19f3545

commnets + todos

672acec

use ref of vec rather than vec of regs

816c707

forgot to push the current calltracenode to the count

c443edb

isgts

01dcd43

trait bound gn

6017856

fill in code

7cc6c66

merge main

1416c66

remove get_code function

9eb8012

from -> new, comments removed

8aa012c

trait wasnt doing anything

2a2fb85

nhtyy marked this pull request as ready for review July 2, 2023 17:14

nhtyy requested a review from mattsse as a code owner July 2, 2023 17:14

nhtyy commented Jul 2, 2023

View reviewed changes

N added 3 commits July 2, 2023 13:30

comments

4fb2b3a

comments + some tests

3f5db80

assert stuff in test, layout

3035c34

gakonst reviewed Jul 3, 2023

View reviewed changes

crates/revm/revm-inspectors/src/tracing/builder/parity.rs Outdated Show resolved Hide resolved

mattsse requested changes Jul 3, 2023

View reviewed changes

mattsse added the A-rpc Related to the RPC implementation label Jul 3, 2023

use trait bounds instead of dyn dispatch, comments, naming

971f80d

onbjerg changed the title ~~fix: issue #2577~~ feat: complete vm and statediff tracers Jul 3, 2023

onbjerg added the C-enhancement New feature or request label Jul 3, 2023

mattsse requested changes Jul 5, 2023

View reviewed changes

nhtyy and others added 5 commits July 5, 2023 12:18

Update crates/revm/revm-inspectors/src/tracing/builder/walker.rs

5838f2e

Co-authored-by: Matthias Seitz <[email protected]>

iterative byte code filling

699c93d

Merge branch 'n/vm-trace' of https://github.com/nhtyy/reth into n/vm-…

c00ed89

…trace

comments

3b69972

comments

9f9d2c4

N added 2 commits July 5, 2023 13:47

comment unused types

7fac645

better expects

849577c

mattsse requested changes Jul 7, 2023

View reviewed changes

mattsse mentioned this pull request Jul 7, 2023

Tracking: RPC methods #3661

Closed

89 tasks

N added 4 commits July 10, 2023 15:32

extend, remove comments

21d2872

ordering of iterator in params

6bb08dd

use code_by_hash

175bbec

Merge branch 'main' of https://github.com/paradigmxyz/reth into n/vm-…

08d8d40

…trace

gakonst requested a review from mattsse July 10, 2023 22:33

Merge branch 'main' into n/vm-trace

77acc0a

mattsse approved these changes Jul 12, 2023

View reviewed changes

mattsse enabled auto-merge July 12, 2023 17:47

nits

38c1483

mattsse force-pushed the n/vm-trace branch from bd0d7a1 to 38c1483 Compare July 12, 2023 17:55

mattsse added this pull request to the merge queue Jul 12, 2023

Merged via the queue into paradigmxyz:main with commit f0cf93e Jul 12, 2023

merklefruit pushed a commit to op-rs/op-reth that referenced this pull request Jul 18, 2023

feat: complete vm and statediff tracers (paradigmxyz#3529)

a091355

Co-authored-by: N <[email protected]> Co-authored-by: Matthias Seitz <[email protected]>

feat: complete vm and statediff tracers #3529

feat: complete vm and statediff tracers #3529

Conversation

nhtyy commented Jul 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nhtyy Jul 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gakonst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nhtyy Jul 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nhtyy Jul 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nhtyy commented Jul 5, 2023

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nhtyy commented Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nhtyy Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nhtyy commented Jul 11, 2023

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nhtyy commented Jul 1, 2023 •

edited

Loading

nhtyy Jul 2, 2023 •

edited

Loading

nhtyy Jul 3, 2023 •

edited

Loading

nhtyy Jul 3, 2023 •

edited

Loading

nhtyy commented Jul 5, 2023 •

edited

Loading

nhtyy Jul 10, 2023 •

edited

Loading

codecov bot commented Jul 12, 2023 •

edited

Loading