cmd/evm: improve block/state test runner #30633

lightclient · 2024-10-19T15:00:05Z

Had the idea for a while to refactor cmd/evm. It feels like a lot of it has grown organically to meet demands -> test running, test fill, eof validation, etc.

original motivation

Unfortunately this had led to several subcommands with little relation beyond a general relationship to the EVM. This makes using the `evm` command confusing. Both `evm run` and `evm t8n` specify their own set of tracing flags ([1][runner-trace], [2][t8n-trace]). Flags like `--input` are reused for different things between the test runners and disassemblers. `eofparse` adds `--hex` which is basically `--input` but for it's own purposes.

There isn't a coherent story for the set of tools. I think we should either spend a fair bit of work ensuring the flags are meaningful across all subcommands and avoid duplicate behavior, or some of the subcommands should live in their own package. My preference is the latter, because it more accurately represents today's maintenance: t8n is the highest prio and has many external contributions from the testing team. Enforcing and ensuring their changes make sense across evm will slow down everyone involved. The interface for t8n is the most robust and it makes sense for it to be it's own package. eofparse is much newer, but similarly seems like an unrelated project that can live on it's own. Which leaves us with the runner commands and the compilation tools. I opted to delete the compilation tools since they are used much any more and it was suggested we delete them last year.

Updated this PR to focus mostly on harmonizing the staterunner / blockrunner while also tidying up in cmd/evm/main.go where I could.

In summary:

~~move t8n, t9n, and b11r to their own command cmd/t8ntool~~
delete evm compile and evm disasm
~~move evm eofparse to cmd/eofparse~~
Added some nice stuff in c594c87:
unify staterunner and blockrunner CLI flags, especially around tracing
added support for struct logger or json logging (although having issue insertChain panics when struct tracer is set #30658)
new --cross-check flag to validate the stateless witness collection / execution matches stateful
adds support for tracing the stateless execution when a tracer is set (to more easily debug differences)
--human for more readable test summary
directory or file input, so if you pass tests/spec-tests/fixtures/blockchain_tests it will execute all blockchain tests

--

example

$ go run ./cmd/evm --verbosity=0 blocktest --human --run="zero_inputs" ./tests/spec-tests/fixtures/blockchain_tests/cancun
[PASS] eip5656_mcopy test_valid_mcopy_operations, param=zero_inputs
--
1 tests passed, 0 tests failed.

--

holiman · 2024-10-20T15:27:11Z

move t8n, t9n, and b11r to their own command cmd/t8ntool

move evm eofparse to cmd/eofparse

Both t8n and eofparse were their own cmds. But @fjl preferred to have it inside evm. As for me, I don't really have an opinion which is preferrable.

holiman · 2024-10-20T15:28:25Z

One upside in having it inside evm is that it's always present whenever evm is. We don't need to fiddle with build scripts and makefiles to add or remove targets.

MariusVanDerWijden

Code changes LGTM

I personally prefer to have these as separate commands and I don't think we should be concerned about the build scripts etc. Eofdump has as very small audience (Martin and I), same with t8ntool (Mario and Matt) so the only really user-facing function is evm state-test and evm blocktest that are used by hive, testing and fuzzing. Everything else is functionality that we use internally that is not really intended for a wider audience imo

lightclient · 2024-10-21T16:22:53Z

@fjl wdyt? I don't see much advantage of only needing one binary everywhere. There aren't many consumers and those consumers are somewhat independent (as Marius says). The advantage is that the tools can be simpler, less thought needed in organization since each tool only does a few related things, and different people can own different packages.

lightclient · 2024-10-23T20:07:04Z

Added some nice stuff in c594c87:

unify staterunner and blockrunner CLI flags, especially around tracing
added support for struct logger or json logging (although having issue insertChain panics when struct tracer is set #30658)
new --cross-check flag to validate the stateless witness collection / execution matches stateful
adds support for tracing the stateless execution when a tracer is set (to more easily debug differences)
--human for more readable test summary
directory or file input, so if you pass tests/spec-tests/fixtures/blockchain_tests it will execute all blockchain tests

--

example

$ go run ./cmd/evm --verbosity=0 blocktest --human --run="zero_inputs" ./tests/spec-tests/fixtures/blockchain_tests/cancun
[PASS] eip5656_mcopy test_valid_mcopy_operations, param=zero_inputs
--
1 tests passed, 0 tests failed.

MariusVanDerWijden · 2024-10-24T03:01:35Z

directory or file input, so if you pass tests/spec-tests/fixtures/blockchain_tests it will execute all blockchain tests

ahh great!

lightclient · 2024-10-29T14:28:24Z

From triage: seems like there is desire to not split the evm command. I'll combine them again with the other refactors I did.

mdehoog · 2024-11-01T19:29:25Z

eth/catalyst/api.go

@@ -995,7 +996,7 @@ func (api *ConsensusAPI) executeStatelessPayload(params engine.ExecutableData, v
 	api.lastNewPayloadLock.Unlock()

 	log.Trace("Executing block statelessly", "number", block.Number(), "hash", params.BlockHash)
-	stateRoot, receiptRoot, err := core.ExecuteStateless(api.eth.BlockChain().Config(), block, witness)
+	stateRoot, receiptRoot, err := core.ExecuteStateless(api.eth.BlockChain().Config(), vm.Config{}, block, witness)


any reason not to pass *api.eth.BlockChain().GetVMConfig() here? or at least the live logger?

…o-ethereum/pull/30633)

lightclient · 2024-11-19T13:32:00Z

I'll combine them again with the other refactors I did.

still haven't done this, will get to it soon

lightclient · 2024-11-21T07:17:05Z

Okay this should be good to go. Have reverted the refactoring out the subcommands, but retained the other improvements. PTAL!

…utput, stateless cross-checking option

cmd/evm/blockrunner.go

holiman · 2024-11-21T09:08:57Z

cmd/evm/staterunner.go

+				if state.StateDB != nil {
+					root = state.StateDB.IntermediateRoot(false)
+					result.Root = &root
+					// Dump any state to aid debugging.


Did you just drop the fmt.Fprintf(os.Stderr - output of the stateRoot?

The state root is recorded in the result and output via report(..).

cmd/evm/main.go

holiman · 2024-11-21T09:29:32Z

Some diffs when I run the reference tests in goevmlab. github.com/holiman/goevmlab/evms/testdata, using the following changed params:

diff --git a/evms/testdata/run.sh b/evms/testdata/run.sh
index 4c38810..eee91c2 100755
--- a/evms/testdata/run.sh
+++ b/evms/testdata/run.sh
@@ -15,7 +15,8 @@ if [[ -n "$evm" ]]; then
     cd ./cases
     # The traces
     for i in *.json; do
-        $evm --json --nomemory --noreturndata statetest $i \
+#        $evm --json --nomemory --noreturndata statetest $i \
+        $evm statetest --trace --trace.format=json --trace.nomemory --trace.noreturndata $i \
          2>../traces/$i.geth.stderr.txt \
          1>../traces/$i.geth.stdout.txt
     done

lightclient · 2024-11-21T10:58:38Z

I'm not sure why output step might be missing. That should come from the tracer and the tracer is configured the same before and after this PR?

I can add the Fprintln back in if it's important, but I was thinking it would be good to move all this data into a full json object so we can more easily interpret and access it.

You can see it is written to stdout after the execution completes:

[
  {
    "name": "tests/prague/eip7702_set_code_tx/test_set_code_txs.py::test_set_code_call_set_code[fork_Prague-call_opcode_CALL-evm_code_type_LEGACY-state_test-value_1]",
    "pass": true,
    "stateRoot": "0xa489cbd2d4e37e8b6b27fa5a66242d3732fbe346aea1aa011d3aa4545765ec13",
    "fork": "Prague"
  }
]

When there is a full dump, it is also there:

[
  {
    "name": "tests/prague/eip7702_set_code_tx/test_set_code_txs.py::test_set_code_call_set_code[fork_Prague-call_opcode_CALL-evm_code_type_LEGACY-state_test-value_0]",
    "pass": true,
    "stateRoot": "0x21331f3c18a3cf737d68e396e32bfef23a8f7cec32971f0265850453b82afe9f",
    "fork": "Prague",
    "state": {
      "root": "21331f3c18a3cf737d68e396e32bfef23a8f7cec32971f0265850453b82afe9f",
      "accounts": {
        "0x0000000000000000000000000000000000001000": {
          "balance": "0",
          "nonce": 1,
          "root": "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421",
          "codeHash": "0xe90b835ed0c9ae43182bcfe017e4e9776a804d20e0bf7061ae73758bcc5e9cde",
          "code": "0x60006000600060006000738a0a19589531694250d570040a0c4b74576919b85af1600055600160015500",
          "address": "0x0000000000000000000000000000000000001000",
          "key": "0x1d7dcb6a0ce5227c5379fc5b0e004561d7833b063355f69bfea3178f08fbaab4"
        }
      }
    }
  }
]

holiman · 2024-11-21T12:40:01Z

You can see it is written to stdout after the execution completes:

Ah, but I'm reading jsonl items from stderr, so that's where I need it

cmd/evm/blockrunner.go

lightclient · 2024-11-21T14:25:00Z

Added back the fprint for the state root 👍

holiman · 2024-11-24T19:22:58Z

Still missing the output elements

holiman · 2024-11-24T19:27:30Z

Ah, wait, it's broken on master too, not your fault.. Bisecting

holiman · 2024-11-24T19:36:16Z

fa581766f5b14f6fad9f2c7a4aa7e7ac826a8de2 is the first bad commit
commit fa581766f5b14f6fad9f2c7a4aa7e7ac826a8de2
Author: Sina M <[email protected]>
Date:   Thu May 23 10:55:54 2024 +0200

    eth/tracers: fix json logger for evm blocktest (#29795)

For some reason, this PR makes the output-output go missing: #29795. @s1na any ideas?

holiman · 2024-11-24T19:42:13Z

doing a $ bash transition-test.sh | tee foo.txt and comparing foo.txt with README.md shows that t8n is still fine. So as far as the output of evm statetest and t8n, seems fine to me so far. Will review more in depth tomorrow.

holiman · 2024-11-24T19:44:07Z

Found the bug, OnExit != OnEnd

holiman · 2024-11-25T07:49:23Z

The struct logger doesn't actually ever output anything:

[user@work testdata]$ yes "./cases/00000006-naivefuzz-0.json" | head -n2  | /home/user/go/src/github.com/ethereum/go-ethereum/evm2  statetest --trace  --trace.nomemory  --trace.format=struct
{"stateRoot": "0xad1024c87b5548e77c937aa50f72b6cb620d278f4dd79bae7f78f71ff75af458"}
[
  {
    "name": "00000006-naivefuzz-0",
    "pass": false,
    "stateRoot": "0xad1024c87b5548e77c937aa50f72b6cb620d278f4dd79bae7f78f71ff75af458",
    "fork": "London",
    "error": "post state root mismatch: got ad1024c87b5548e77c937aa50f72b6cb620d278f4dd79bae7f78f71ff75af458, want 0000000000000000000000000000000000000000000000000000000000000000"
  }
]
{"stateRoot": "0xad1024c87b5548e77c937aa50f72b6cb620d278f4dd79bae7f78f71ff75af458"}
[
  {
    "name": "00000006-naivefuzz-0",
    "pass": false,
    "stateRoot": "0xad1024c87b5548e77c937aa50f72b6cb620d278f4dd79bae7f78f71ff75af458",
    "fork": "London",
    "error": "post state root mismatch: got ad1024c87b5548e77c937aa50f72b6cb620d278f4dd79bae7f78f71ff75af458, want 0000000000000000000000000000000000000000000000000000000000000000"
  }
]

As opposed to:

[user@work testdata]$ yes "./cases/00000006-naivefuzz-0.json" | head -n2  | /home/user/go/src/github.com/ethereum/go-ethereum/evm2  statetest --trace  --trace.nomemory  --trace.format=json
{"pc":0,"op":96,"gas":"0xb4213","gasCost":"0x3","memSize":0,"stack":[],"depth":1,"refund":0,"opName":"PUSH1"}
{"pc":2,"op":96,"gas":"0xb4210","gasCost":"0x3","memSize":0,"stack":["0x2"],"depth":1,"refund":0,"opName":"PUSH1"}
{"pc":4,"op":85,"gas":"0xb420d","gasCost":"0x5654","memSize":0,"stack":["0x2","0x3"],"depth":1,"refund":0,"opName":"SSTORE"}
{"pc":5,"op":96,"gas":"0xaebb9","gasCost":"0x3","memSize":0,"stack":[],"depth":1,"refund":0,"opName":"PUSH1"}

~~Reason being that it only emits traces if it's configured for Debug, but afaict there's no code which enables that~~

// OnExit is called a call frame finishes processing.
func (l *StructLogger) OnExit(depth int, output []byte, gasUsed uint64, err error, reverted bool) {
	if depth != 0 {
		return
	}
	l.output = output
	l.err = err
	if l.cfg.Debug {
		fmt.Printf("%#x\n", output)
		if err != nil {
			fmt.Printf(" error: %v\n", err)
		}
	}
}

EDIT: No, althought Debug might be never set, that's a different error.

holiman · 2024-11-25T08:06:44Z

So, as far as I can tell, the only way to get struct output is to use the evm run command:

$ evm run --debug   "0x6040"  

#### TRACE ####
PUSH1           pc=00000000 gas=10000000000 cost=3

STOP            pc=00000002 gas=9999999997 cost=0
Stack:
00000000  0x40

It can never be output from the statetest command.

These emit no output:

$ evm run    "0x6040"  --trace --trace.format=json

$ evm run    --trace --trace.format=json "0x6040"

$ evm run    --trace --trace.format=struct "0x6040"

$ evm run  "0x6040" --dump

In order to make evm run spit out json, one needs to do :

$ evm run   --json --trace --trace.format=json 6040 
{"pc":0,"op":96,"gas":"0x2540be400","gasCost":"0x3","memSize":0,"stack":[],"depth":1,"refund":0,"opName":"PUSH1"}
{"pc":2,"op":0,"gas":"0x2540be3fd","gasCost":"0x0","memSize":0,"stack":["0x40"],"depth":1,"refund":0,"opName":"STOP"}
{"output":"","gasUsed":"0x3"}

Putting the --trace on the wrong level (which is a super-common user-mistake to do), has unintended sideeffects. Apparently
it conflicts with a global tracing function:

$ evm     --trace --trace.format=struct run  "0x6040"
INFO [11-25|08:58:02.680] Go tracing started                       dump="--trace.format=struct"

INFO [11-25|08:58:02.683] Done writing Go trace                    dump="--trace.format=struct"
$ evm     --trace --trace.format=json run  "0x6040"
INFO [11-25|08:58:08.381] Go tracing started                       dump="--trace.format=json"

INFO [11-25|08:58:08.383] Done writing Go trace                    dump="--trace.format=json"

See internal/debug/trace.go

// StartGoTrace turns on tracing, writing to the given file.
func (h *HandlerT) StartGoTrace(file string) error {

If user puts --debug in the sweet-spot, then evm run will grace him with the output:

$ evm run --debug  --trace --trace.format=json 6040

#### TRACE ####
PUSH1           pc=00000000 gas=10000000000 cost=3

STOP            pc=00000002 gas=9999999997 cost=0
Stack:
00000000  0x40

#### LOGS ####

IMO, we should

Make the behaviour with
- 1. How to configure the outputs identical (--trace, --trace.format, drop --debug as the enabler for struct output).
- 1. Make format json default, over struct.
Avoid conflict between trace: global tracing enabled XOR trace-execution.

holiman · 2024-11-25T08:23:15Z

We should also make the markdown logger usable, and perhaps even default.
Example if I hack-enable it:

		//tracer = logger.NewJSONLogger(logconfig, os.Stdout)
		tracer = logger.NewMarkdownLogger(logconfig, os.Stdout).Hooks()

(Note: strange assymetry there, where the NewJSONLogger returns *tracing.Hooks, but NewMarkdownLogger does not.)

[user@work testdata]$ /home/user/go/src/github.com/ethereum/go-ethereum/evm run   --json --trace --trace.format=json 6040 
From: `0x000000000000000000000000000073656E646572`
To: `0x0000000000000000000000007265636569766572`
Data: ``
Gas: `10000000000`
Value `0` wei

|  Pc   |      Op     | Cost |   Stack   |   RStack  |  Refund |
|-------|-------------|------|-----------|-----------|---------|
|    0  |      PUSH1  |    3 |        [] |         0 |
|    2  |       STOP  |    0 |    [0x40] |         0 |

Output: ``
Consumed gas: `3`
Error: `<nil>`

Or, without quoting:

[user@work testdata]$ /home/user/go/src/github.com/ethereum/go-ethereum/evm run --json --trace --trace.format=json 6040
From: 0x000000000000000000000000000073656E646572
To: 0x0000000000000000000000007265636569766572
Data: ``
Gas: 10000000000
Value `0` wei

Pc	Op	Cost	Stack	RStack	Refund
0	PUSH1	3	[]	0
2	STOP	0	[0x40]	0

Output: ``
Consumed gas: 3
Error: ``

holiman · 2024-11-25T08:30:29Z

All users of NewMarkdownLogger instantly invoke Hooks, e.g. logger.NewMarkdownLogger(nil, os.Stdout).Hooks() so please change it to use that.

Also, the struct logger is a non-streaming logger. It was originally used in RPC, where we need to store the logs in-memory before we marshal to json. For evm, we can IMO always stream the output. That would make the tracer outputters more similar, and we wouldn't need to invoke a logger.WriteTrace. This is the reason why json vs struct behave differently in evm run vs e.g. evm statetest.

So I think we should align them, behaviourally (json, struct and markdown), and if we need a special type of non-streaming tracer just for the rpc, it can be a separate one for that purpose. Or we put a collector on the outside of a streaming outputter, for that purpose.

Fixes a flaw introduced in #29795 , discovered while reviewing #30633 .

holiman · 2024-11-25T10:27:06Z

The StructLogger has a slot-dirty tracking within OnOpCode

// OnOpcode also tracks SLOAD/SSTORE ops to track storage change.

However, said dirty-tracking does not handle reverted scopes, so it's pretty random wether it will present accurate or false information. IMO we should drop that. Dunno if it needs to go into this PR, just noting it.

lightclient requested review from holiman and MariusVanDerWijden as code owners October 19, 2024 15:00

MariusVanDerWijden approved these changes Oct 21, 2024

View reviewed changes

lightclient force-pushed the cmd-evm2 branch from d45d989 to c594c87 Compare October 23, 2024 19:56

lightclient requested review from gballet, karalabe and rjl493456442 as code owners October 23, 2024 19:56

lightclient added the status:triage label Oct 29, 2024

lightclient requested review from fjl and zsfelfoldi as code owners October 29, 2024 14:59

lightclient force-pushed the cmd-evm2 branch 3 times, most recently from 20ad594 to b37356f Compare October 29, 2024 20:45

lightclient mentioned this pull request Nov 1, 2024

Support passing vm.Config to core.ExecuteStateless #30710

Closed

mdehoog reviewed Nov 1, 2024

View reviewed changes

mdehoog added a commit to mdehoog/op-geth that referenced this pull request Nov 1, 2024

ExecuteStateless should accept a vm.Config (see github.com/ethereum/g…

8f32d72

…o-ethereum/pull/30633)

fjl removed the status:triage label Nov 5, 2024

lightclient force-pushed the cmd-evm2 branch 4 times, most recently from 0cb4c48 to 270925f Compare November 21, 2024 07:15

cmd/evm: unify staterunner and blockrunner more, add human-readable o…

7c34200

…utput, stateless cross-checking option

lightclient force-pushed the cmd-evm2 branch from 270925f to 7c34200 Compare November 21, 2024 08:48

holiman reviewed Nov 21, 2024

View reviewed changes

cmd/evm/blockrunner.go Show resolved Hide resolved

holiman reviewed Nov 21, 2024

View reviewed changes

cmd/evm/main.go Outdated Show resolved Hide resolved

cmd/evm: review fixes

092b99f

holiman reviewed Nov 21, 2024

View reviewed changes

cmd/evm/blockrunner.go Show resolved Hide resolved

cmd/evm: print state root for jsonl format

2ae47cf

holiman mentioned this pull request Nov 25, 2024

eth/tracers/logger: fix json-logger output missing #30804

Merged

holiman added a commit that referenced this pull request Nov 25, 2024

eth/tracers/logger: fix json-logger output missing (#30804)

ab4a1cc

Fixes a flaw introduced in #29795 , discovered while reviewing #30633 .

fjl changed the title ~~cmd: refactor evm tool~~ cmd/evm: improve block/state test runner Nov 26, 2024

fjl added this to the 1.14.13 milestone Nov 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/evm: improve block/state test runner #30633

cmd/evm: improve block/state test runner #30633

lightclient commented Oct 19, 2024 •

edited

Loading

holiman commented Oct 20, 2024

holiman commented Oct 20, 2024

MariusVanDerWijden left a comment

lightclient commented Oct 21, 2024

lightclient commented Oct 23, 2024 •

edited

Loading

MariusVanDerWijden commented Oct 24, 2024

lightclient commented Oct 29, 2024

mdehoog Nov 1, 2024 •

edited

Loading

lightclient commented Nov 19, 2024

lightclient commented Nov 21, 2024

holiman Nov 21, 2024

lightclient Nov 21, 2024

holiman commented Nov 21, 2024

lightclient commented Nov 21, 2024

holiman commented Nov 21, 2024

lightclient commented Nov 21, 2024

holiman commented Nov 24, 2024

holiman commented Nov 24, 2024

holiman commented Nov 24, 2024

holiman commented Nov 24, 2024

holiman commented Nov 24, 2024

holiman commented Nov 25, 2024 •

edited

Loading

holiman commented Nov 25, 2024 •

edited

Loading

holiman commented Nov 25, 2024

holiman commented Nov 25, 2024

holiman commented Nov 25, 2024

cmd/evm: improve block/state test runner #30633

Are you sure you want to change the base?

cmd/evm: improve block/state test runner #30633

Conversation

lightclient commented Oct 19, 2024 • edited Loading

holiman commented Oct 20, 2024

holiman commented Oct 20, 2024

MariusVanDerWijden left a comment

Choose a reason for hiding this comment

lightclient commented Oct 21, 2024

lightclient commented Oct 23, 2024 • edited Loading

MariusVanDerWijden commented Oct 24, 2024

lightclient commented Oct 29, 2024

mdehoog Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

lightclient commented Nov 19, 2024

lightclient commented Nov 21, 2024

holiman Nov 21, 2024

Choose a reason for hiding this comment

lightclient Nov 21, 2024

Choose a reason for hiding this comment

holiman commented Nov 21, 2024

lightclient commented Nov 21, 2024

holiman commented Nov 21, 2024

lightclient commented Nov 21, 2024

holiman commented Nov 24, 2024

holiman commented Nov 24, 2024

holiman commented Nov 24, 2024

holiman commented Nov 24, 2024

holiman commented Nov 24, 2024

holiman commented Nov 25, 2024 • edited Loading

holiman commented Nov 25, 2024 • edited Loading

holiman commented Nov 25, 2024

holiman commented Nov 25, 2024

holiman commented Nov 25, 2024

lightclient commented Oct 19, 2024 •

edited

Loading

lightclient commented Oct 23, 2024 •

edited

Loading

mdehoog Nov 1, 2024 •

edited

Loading

holiman commented Nov 25, 2024 •

edited

Loading

holiman commented Nov 25, 2024 •

edited

Loading