Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd: refactor evm tool #30633

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

lightclient
Copy link
Member

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.

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, 2). 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.

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

If these changes make sense, I also want to do some work on cmd/evm to improve the interface for logging, running batches of tests, and validating tests against the stateless runner.

--

@holiman
Copy link
Contributor

holiman commented Oct 20, 2024

  • 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
Copy link
Contributor

holiman commented Oct 20, 2024

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.

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

@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
Copy link
Member Author

lightclient commented Oct 23, 2024

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
Copy link
Member

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

ahh great!

@lightclient
Copy link
Member Author

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

@@ -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)
Copy link
Contributor

@mdehoog mdehoog Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants