Implement Evm Trace #828

gurukamath · 2023-08-31T13:23:47Z

(closes #624 )

What was wrong?

The specs don't currently implement the full evm trace.

Related to Issue #624

How was it fixed?

Implemented full evm trace and integrated it fully with t8n.

Cute Animal Picture

gurukamath · 2023-08-31T13:25:10Z

Currently implemented the trace only for Shanghai. Will be ported to the other forks after review.

codecov-commenter · 2023-08-31T15:36:00Z

Codecov Report

Patch coverage: 74.03% and project coverage change: +0.03% 🎉

Comparison is base (adc6acf) 74.06% compared to head (753416e) 74.09%.
Report is 8 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #828      +/-   ##
==========================================
+ Coverage   74.06%   74.09%   +0.03%     
==========================================
  Files         571      572       +1     
  Lines       31735    32018     +283     
==========================================
+ Hits        23503    23725     +222     
- Misses       8232     8293      +61

Flag	Coverage Δ
unittests	`74.09% <74.03%> (+0.03%)`	⬆️

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

Files Changed	Coverage Δ
src/ethereum/__init__.py	`100.00% <ø> (ø)`
src/ethereum/arrow_glacier/fork.py	`0.00% <ø> (ø)`
src/ethereum/arrow_glacier/utils/message.py	`0.00% <ø> (ø)`
src/ethereum/arrow_glacier/vm/__init__.py	`0.00% <0.00%> (ø)`
src/ethereum/arrow_glacier/vm/gas.py	`0.00% <0.00%> (ø)`
.../ethereum/arrow_glacier/vm/instructions/storage.py	`0.00% <0.00%> (ø)`
...c/ethereum/arrow_glacier/vm/instructions/system.py	`0.00% <0.00%> (ø)`
src/ethereum/arrow_glacier/vm/interpreter.py	`0.00% <0.00%> (ø)`
src/ethereum/berlin/fork.py	`93.85% <ø> (+0.02%)`	⬆️
src/ethereum/berlin/utils/message.py	`96.00% <ø> (ø)`
... and 97 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gurukamath · 2023-09-01T09:03:25Z

Since the evm tests fail for the forks before Shanghai, I have added a temporary if condition. This is just so we see all the tests passing in the CI. Once the Shanghai changes have been reviewed and are ported over to the other forks, this if condition goes away before merging all the changes with master.

SamWilsn

Probably still here because you've only done Shanghai so far, but just in case, you'll want to remove this as well:

execution-specs/src/ethereum/__init__.py

Lines 31 to 37 in fade6e9

    
           def evm_trace(evm: Any, op: Any) -> None: 
        
               """ 
        
               autoapi_noshow 
        
               Placeholder for an evm trace function. The spec does not trace evm by 
        
               default. EVM tracing will be injected if the user requests it. 
        
               """ 
        
               pass

src/ethereum/trace.py

tests/conftest.py

src/ethereum_spec_tools/evm_trace.py

gurukamath · 2023-09-04T12:02:05Z

Probably still here because you've only done Shanghai so far, but just in case, you'll want to remove this as well:

execution-specs/src/ethereum/__init__.py

Lines 31 to 37 in fade6e9

def evm_trace(evm: Any, op: Any) -> None:

"""

autoapi_noshow

Placeholder for an evm trace function. The spec does not trace evm by

default. EVM tracing will be injected if the user requests it.

"""

pass

Yes. This will also go after porting the changes to the other forks.

petertdavies

I have one further niggle. I think Sam has covered the rest.

src/ethereum_spec_tools/evm_trace.py

danceratopz

Really looking forward to using this!

I quickly tested the --trace flag (combined with the changes required from #823) against ethereum/execution-spec-tests#289; also slightly modified due to differences in filename (more on that below):

diff --git a/src/evm_transition_tool/transition_tool.py b/src/evm_transition_tool/transition_tool.py
index ae24b319..14c1ee2b 100644
--- a/src/evm_transition_tool/transition_tool.py
+++ b/src/evm_transition_tool/transition_tool.py
@@ -242,6 +242,7 @@ class TransitionTool:
         traces: List[List[Dict]] = []
         for i, r in enumerate(receipts):
             trace_file_name = f"trace-{i}-{r['transactionHash']}.jsonl"
+            # trace_file_name = f"spec-trace-{r['transactionHash']}.json"
             if debug_output_path:
                 shutil.copy(
                     os.path.join(temp_dir.name, trace_file_name),

Here's the execution-spec-tests command:

fill --evm-bin=/path/to/ethereum/execution-specs/venv/bin/ethereum-spec-evm --t8n-dump-dir=/tmp/ethereum-spec-evm/ --fork=Shanghai -v --traces

Some background: The interface used in execution-spec-tests to execute the ethereum-spec-evm t8n tool is shared with geth's evm t8n and evmone-t8n, so any differences are quickly highlighted. Would it be possible to change the filename as below, to make them consistent across these tools? The main effort will be adding the index of the transaction processed by the t8n tool.

Further help on using execution-spec-tests with execution-spec-evm here.

gurukamath · 2023-09-07T09:53:13Z

@SamWilsn @petertdavies Have updated the relevant bits based on your comments.

@danceratopz Have updated the tool to reflect specific output file names. Please verify.

danceratopz · 2023-09-07T20:04:42Z

@gurukamath Yes, if I apply #823 to this branch, it now works as expected with ethereum/execution-spec-tests#289 as-is, thanks!

danceratopz

Ah, one last niggle/discrepancy, sorry! I just saw that the following flags are also available:

  --trace.memory TRACE_MEMORY
  --trace.nomemory TRACE_NOMEMORY
  --trace.noreturndata TRACE_NORETURNDATA
  --trace.nostack  TRACE_NOSTACK 
  --trace.returndata TRACE_RETURNDATA

which require an argument. To make the behavior exactly as geth's evm t8n command, these would be switches that do not take/require an argument.

Of course, we can abstract this away in our interface, but I hope you appreciate it's nice to keep these tools as consistent as possible.

src/ethereum_spec_tools/evm_tools/t8n/__init__.py

gurukamath · 2023-09-16T12:06:44Z

@SamWilsn @petertdavies Have ported the changes over to the older forks.

The main update from the earlier commits in this PR is that the refund calculation for SELFDESTRUCT is moved from the interpreter module into the opcode itself (mainly relevant to Berlin and before). This requires making the accounts_to_delete from the parent evm available to the child evm.

danceratopz · 2023-09-19T11:12:01Z

Hi @gurukamath, I took a deeper look at the traces generated from the current execution-spec-tests and diff'd the traces generated from geth 1.13.1-stable-3f40e65c and the ethereum-spec-evm from this branch (753416e).

The traces are almost identical 🥳 and only differ upon an error in execution. They fall into 2 categories.

Missing exception message upon error. Here are examples of the 4 missing message types (here they are in ethereum/go-ethereum/core/vm/errors.go#L24@90d5bd85, I have no idea if they are standardized):

test_push0_stack_overflow_fork_Shanghai/1/trace-0-0xf8616cc40f214a4729758189c77720a6b672f29dcb62cd206a49ebacaf00e96f.jsonl

1028,1029c1028,1029
< {"pc":1028,"op":95,"gas":"0xd63f","gasCost":"0x2","memSize":0,"stack":["0x0", <full stack ommitted>, "0x0"],"depth":1,"refund":0,"opName":"PUSH0","error":"stack limit reached 1024 (1023)"}
< {"output":"","gasUsed":"0x13498","error":"stack limit reached 1024 (1023)"}
---
> {"pc":1028,"op":95,"gas":"0xd63f","gasCost":"0x2","memSize":0,"stack":["0x0", <full stack ommitted>, "0x0"],"depth":1,"refund":0,"opName":"PUSH0","error":""}
> {"output":"","gasUsed":"0x13498","error":""}

test_create_opcode_initcode_fork_Shanghai_create2_over_limit_ones/1/trace-0-0xff8b41168839e22964df27b1810063b71235dd344b1473025c2c9473f380cd02.jsonl

23c23
< {"pc":17,"op":245,"gas":"0x4c151c","gasCost":"0x7d00","memSize":49184,"stack":["0x4c1527","0xdeadbeef","0xc001","0x0","0x0"],"depth":2,"refund":0,"opName":"CREATE2","error":"out of gas"}
---
> {"pc":17,"op":245,"gas":"0x4c151c","gasCost":"0xad08","memSize":49184,"stack":["0x4c1527","0xdeadbeef","0xc001","0x0","0x0"],"depth":2,"refund":0,"opName":"CREATE2","error":""}

test_gas_usage_fork_Shanghai_too_little_execution_gas_49121_bytes/1/trace-0-0x1e4296b38ebdc740cd34f5b63602660f115815a352821261416a379a03054832.jsonl
```
8c8
< {"output":"00","gasUsed":"0xdf","error":"contract creation code storage out of gas"}
---
> {"output":"","gasUsed":"0xdf","error":""}
```
test_withdrawing_to_precompiles_fork_Shanghai_precompile_9_amount_1/2/trace-0-0x3e9f556a7efc77edc7e00178f4c3d226633320d06a8289cce10d363f713f50a9.jsonl
```
1c1
< {"output":"","gasUsed":"0x13498","error":"invalid input length"}
---
> {"output":"","gasUsed":"0x13498","error":""}
```

Different reported gasCost upon an exception caused by initcode that is over the allowed limit when calling CREATE or CREATE2, the gas cost for the reverted transaction is, however, identical:

test_create_opcode_initcode_fork_Shanghai_create2_over_limit_ones/1/trace-0-0xff8b41168839e22964df27b1810063b71235dd344b1473025c2c9473f380cd02.jsonl

23c23
< {"pc":17,"op":245,"gas":"0x4c151c","gasCost":"0x7d00","memSize":49184,"stack":["0x4c1527","0xdeadbeef","0xc001","0x0","0x0"],"depth":2,"refund":0,"opName":"CREATE2","error":"out of gas"}
---
> {"pc":17,"op":245,"gas":"0x4c151c","gasCost":"0xad08","memSize":49184,"stack":["0x4c1527","0xdeadbeef","0xc001","0x0","0x0"],"depth":2,"refund":0,"opName":"CREATE2","error":""}

gurukamath · 2023-09-19T15:49:15Z

Missing exception message upon error. Here are examples of the 4 missing message types (here they are in ethereum/go-ethereum/core/vm/errors.go#L24@90d5bd85, I have no idea if they are standardized):

The specs do not currently emit comprehensive error messages. We are working on it and have an active issue ( see #795 )

Different reported gasCost upon an exception caused by initcode that is over the allowed limit when calling CREATE or CREATE2, the gas cost for the reverted transaction is, however, identical:
test_create_opcode_initcode_fork_Shanghai_create2_over_limit_ones/1/trace-0-0xff8b41168839e22964df27b1810063b71235dd344b1473025c2c9473f380cd02.jsonl
23c23
< {"pc":17,"op":245,"gas":"0x4c151c","gasCost":"0x7d00","memSize":49184,"stack":["0x4c1527","0xdeadbeef","0xc001","0x0","0x0"],"depth":2,"refund":0,"opName":"CREATE2","error":"out of gas"}
---
> {"pc":17,"op":245,"gas":"0x4c151c","gasCost":"0xad08","memSize":49184,"stack":["0x4c1527","0xdeadbeef","0xc001","0x0","0x0"],"depth":2,"refund":0,"opName":"CREATE2","error":""}

We are aware that in some cases where the opcode runs out of gas, geth and specs might emit different gas costs. In almost all such cases, I have found that geth emits only the static part of the gas cost. See this geth issue for more details.

However, I would also like to take a deeper look at this particular case just to make sure that it is a similar case. Would it be possible for you to provide the input json files for running this? Or alternatively, point me to the fixture in the tests repo that has this case?

danceratopz · 2023-09-20T04:54:12Z

Thanks for info and links.

However, I would also like to take a deeper look at this particular case just to make sure that it is a similar case. Would it be possible for you to provide the input json files for running this? Or alternatively, point me to the fixture in the tests repo that has this

Github wouldn't let me upload json or tgz - I'll send you the files another way.

In general, you could run this test case from ethereum/execution-spec-tests in isolation with:

fill tests/shanghai/eip3860_initcode/test_initcode.py::TestCreateInitcode::test_create_opcode_initcode[fork=Shanghai-create2-over_limit_ones] --evm-bin=/path/to/execution-specs/venv/bin/ethereum-spec-evm--output=output/

gurukamath · 2023-09-20T09:24:35Z

In general, you could run this test case from ethereum/execution-spec-tests in isolation with:

fill tests/shanghai/eip3860_initcode/test_initcode.py::TestCreateInitcode::test_create_opcode_initcode[fork=Shanghai-create2-over_limit_ones] --evm-bin=/path/to/execution-specs/venv/bin/ethereum-spec-evm--output=output/

@danceratopz I did look into this case and is similar to the scenario that I pointed out earlier. Geth figures out that the opcode is going to run out of gas due to large init code. For efficiency reasons, it then does not even bother to calculate the dynamic gas because it makes no difference. So it just emits the static part in the trace.

The specs on the other hand calculate all the components of the gas (static + dynamic) and emits that in the trace. I am not sure if there is a standard behaviour that is agreed upon in this case. At least, I couldn't find anything in EIP-3155. For greater context on such issues you can follow the issue on the geth repo that linked earlier. See here

petertdavies

LGTM

implement evm trace

f588b28

gurukamath force-pushed the evm-trace-final branch from 037e6a0 to f588b28 Compare September 1, 2023 09:00

gurukamath requested review from SamWilsn and petertdavies September 1, 2023 11:48

SamWilsn reviewed Sep 1, 2023

View reviewed changes

petertdavies requested changes Sep 5, 2023

View reviewed changes

src/ethereum_spec_tools/evm_trace.py Outdated Show resolved Hide resolved

danceratopz mentioned this pull request Sep 6, 2023

feat(evm): enable tracing support for ethereum-spec-evm ethereum/execution-spec-tests#289

Merged

danceratopz reviewed Sep 6, 2023

View reviewed changes

src/ethereum_spec_tools/evm_trace.py Outdated Show resolved Hide resolved

danceratopz reviewed Sep 6, 2023

View reviewed changes

src/ethereum_spec_tools/evm_trace.py Outdated Show resolved Hide resolved

danceratopz suggested changes Sep 6, 2023

View reviewed changes

review feedback

f57ca39

SamWilsn approved these changes Sep 7, 2023

View reviewed changes

danceratopz reviewed Sep 7, 2023

View reviewed changes

src/ethereum_spec_tools/evm_tools/t8n/__init__.py Outdated Show resolved Hide resolved

gurukamath mentioned this pull request Sep 12, 2023

Implement the different trace flags #835

Closed

modify type hints

f756a07

gurukamath added 2 commits September 18, 2023 14:47

minor clean-up shanghai

5e5fb5b

port changes to the other forks

753416e

gurukamath force-pushed the evm-trace-final branch from f239e23 to 753416e Compare September 18, 2023 13:54

petertdavies approved these changes Sep 26, 2023

View reviewed changes

petertdavies merged commit b2ddda3 into ethereum:master Sep 26, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Evm Trace #828

Implement Evm Trace #828

gurukamath commented Aug 31, 2023

gurukamath commented Aug 31, 2023

codecov-commenter commented Aug 31, 2023 •

edited

gurukamath commented Sep 1, 2023

SamWilsn left a comment

gurukamath commented Sep 4, 2023

petertdavies left a comment

danceratopz left a comment •

edited

gurukamath commented Sep 7, 2023

danceratopz commented Sep 7, 2023

danceratopz left a comment •

edited

gurukamath commented Sep 16, 2023

danceratopz commented Sep 19, 2023

gurukamath commented Sep 19, 2023

danceratopz commented Sep 20, 2023

gurukamath commented Sep 20, 2023

petertdavies left a comment

	def evm_trace(evm: Any, op: Any) -> None:
	"""
	autoapi_noshow
	Placeholder for an evm trace function. The spec does not trace evm by
	default. EVM tracing will be injected if the user requests it.
	"""
	pass

Implement Evm Trace #828

Implement Evm Trace #828

Conversation

gurukamath commented Aug 31, 2023

What was wrong?

How was it fixed?

Cute Animal Picture

gurukamath commented Aug 31, 2023

codecov-commenter commented Aug 31, 2023 • edited

Codecov Report

gurukamath commented Sep 1, 2023

SamWilsn left a comment

Choose a reason for hiding this comment

gurukamath commented Sep 4, 2023

petertdavies left a comment

Choose a reason for hiding this comment

danceratopz left a comment • edited

Choose a reason for hiding this comment

gurukamath commented Sep 7, 2023

danceratopz commented Sep 7, 2023

danceratopz left a comment • edited

Choose a reason for hiding this comment

gurukamath commented Sep 16, 2023

danceratopz commented Sep 19, 2023

gurukamath commented Sep 19, 2023

danceratopz commented Sep 20, 2023

gurukamath commented Sep 20, 2023

petertdavies left a comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 31, 2023 •

edited

danceratopz left a comment •

edited

danceratopz left a comment •

edited