gurukamath
commented
10 months ago Currently implemented the trace only for Shanghai. Will be ported to the other forks after review.
Closed gurukamath closed 9 months ago
gurukamath
commented
10 months ago Currently implemented the trace only for Shanghai. Will be ported to the other forks after review.
codecov-commenter
commented
10 months ago Patch coverage: 74.03% and project coverage change: +0.03% :tada:
Comparison is base (
adc6acf) 74.06% compared to head (753416e) 74.09%. Report is 8 commits behind head on master.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
gurukamath
commented
10 months ago 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.
gurukamath
commented
10 months ago Probably still here because you've only done Shanghai so far, but just in case, you'll want to remove this as well:
Yes. This will also go after porting the changes to the other forks.
gurukamath
commented
10 months ago @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
commented
10 months ago @gurukamath Yes, if I apply #823 to this branch, it now works as expected with https://github.com/ethereum/execution-spec-tests/pull/289 as-is, thanks!
gurukamath
commented
9 months ago @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
commented
9 months ago 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 (753416e7a70c43bc00c032e3e58ebd9570b7192c).
The traces are almost identical :partying_face: and only differ upon an error in execution. They fall into 2 categories.
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":""}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":""}
- 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
gasCostupon an exception caused by initcode that is over the allowed limit when callingCREATEorCREATE2, the gas cost for the reverted transaction is, however, identical:
test_create_opcode_initcode_fork_Shanghai_create2_over_limit_ones/1/trace-0-0xff8b41168839e22964df27b1810063b71235dd344b1473025c2c9473f380cd02.jsonl23c23 < {"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
commented
9 months ago 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/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
(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