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

tools: add t8n.sh to the t8n-dump-dir for evmone, besu & ethereum-specs-evm (and various t8n tool clean-up) #269

Conversation

danceratopz
Copy link
Member

@danceratopz danceratopz commented Aug 23, 2023

Implements the t8n.sh script added in #261 for the remaining supported t8n tools (evmone-t8n, besu t8n-server, ethereum-specs-evm t8n). The script gets created to the --t8n-dump-dir if specified on the command-line, e.g.,

fill --fork=Shanghai --traces --t8n-dump-dir=/tmp/geth-t8n

This PR also contains quite a lot of other small improvements and clean-up across the transition tools. Here's a quick overview:

  1. Adds file extensions to all the files created in the t8n dump dir.
  2. Writes input and output files to input/output subdirectories.
  3. The result ofoutput.body is now also written to txs.rlp in the dump dir. Except ethereum-spec-evm cf Add output.body/txs rlp support for ethereum-spec-evm  #268.
  4. The default evaluate() method (used by geth, nimbus, ethereum-specs-evm) no longer creates a temporary directory (unless tracing is enabled). All input/output is handled by stdin/stdout, so the directory is not required.
  5. A default collect_traces() method has been added.
  6. Besu t8n-server:
    i. Print the request's text.response in the test failure (by raising an exception in evaluate()) if the response is invalid.
    ii. Add more information about the request to the t8n dump dir (response.text, time elapsed, status code).
  7. ethereum-spec-evm t8n: Required a few changes to get it running, see associated commit messages which reference the issues found; most of these are very small problems that should be easy to solve, but are out of the scope of this PR.
  8. Add a small section to the docs describing --t8n-dump-dir and t8n.sh, preview:
    https://danceratopz.github.io/execution-spec-tests/main/getting_started/debugging_t8n_tools/

@danceratopz danceratopz force-pushed the tools/add-t8n-script-to-t8n-dump-dir-to-all-t8n-commands branch from a21c70a to 65d68b7 Compare August 24, 2023 13:23
@danceratopz
Copy link
Member Author

Force pushed to include #271.

@danceratopz danceratopz marked this pull request as draft August 24, 2023 13:25
@danceratopz
Copy link
Member Author

danceratopz commented Aug 24, 2023

Marked as draft: It's probably worth waiting for ethereum/execution-specs#823 as this will remove the additional workaround logic added in 32ac3bf when the transition tool is ethereum-specs-evm.

Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

Looks good overall, just a few comments 👍

docs/getting_started/debugging_t8n_tools.md Outdated Show resolved Hide resolved
docs/getting_started/debugging_t8n_tools.md Outdated Show resolved Hide resolved
docs/getting_started/debugging_t8n_tools.md Outdated Show resolved Hide resolved
src/evm_transition_tool/transition_tool.py Outdated Show resolved Hide resolved
src/evm_transition_tool/transition_tool.py Outdated Show resolved Hide resolved
src/evm_transition_tool/evmone.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@spencer-tb spencer-tb left a comment

Choose a reason for hiding this comment

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

Super awesome addition!

Tried with all the evm t8n tools (minus nimbus):

Happy for you to merge when the extra changes for evmone/execution-specs are in!

docs/getting_started/debugging_t8n_tools.md Outdated Show resolved Hide resolved
src/evm_transition_tool/evmone.py Show resolved Hide resolved
rodiazet and others added 5 commits August 29, 2023 10:58
* evmone: Fix paths' values for t8n

* style: fix tox linting complaints

---------

Co-authored-by: danceratopz <danceratopz@gmail.com>
Co-authored-by: Mario Vega <marioevz@gmail.com>
Co-authored-by: spencer <spencer.taylor-brown@ethereum.org>
@danceratopz danceratopz marked this pull request as ready for review August 29, 2023 11:02
@danceratopz
Copy link
Member Author

danceratopz commented Aug 29, 2023

@spencer-tb @marioevz thanks a lot for your fixes and suggestions.

Regarding execution-specs-evm: I reverted 32ac3bf(to reduce complexity) in 504b7e4, I wouldn't wait for ethereum/execution-specs#823 in order to merge this PR, I'm sure 823 (or a different fix) will get merged in due course.

@marioevz if you're happy with the changes regarding rm -rf in the t8n.sh script, see 3070d94 (and my comments on your review), I think we can merge.

#!/bin/bash
rm -rf {debug_output_path}/t8n.sh.out # hard-coded to avoid surprises
mkdir {debug_output_path}/t8n.sh.out # unused if tracing is not enabled
{t8n_call} < {debug_output_path}/stdin.txt
"""

@danceratopz danceratopz dismissed marioevz’s stale review August 29, 2023 11:15

All points were addressed; re-requested review.

Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

LGTM :)

@marioevz marioevz merged commit dcc3a3f into ethereum:main Aug 29, 2023
5 checks passed
@danceratopz
Copy link
Member Author

@rodiazet this is now merged into main, thanks for your contribution!

@danceratopz danceratopz deleted the tools/add-t8n-script-to-t8n-dump-dir-to-all-t8n-commands branch September 28, 2023 08:22
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.

4 participants