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

feat(fw): Call evmone-eofparse on EOF tests filling #519

Merged
merged 9 commits into from
May 1, 2024

Conversation

marioevz
Copy link
Member

@marioevz marioevz commented Apr 22, 2024

🗒️ Description

Adds EOF exception checking using evmone-eofparse tool. Prints a nice message now if expected exception does not match returned by evmone or if valid eof test fails on evmone and so on.

🔗 Related Issues

✅ Checklist

  • All: Set appropriate labels for the changes.
  • All: Considered squashing commits to improve commit history.
  • All: Added an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • Tests: All converted JSON/YML tests from ethereum/tests have been added to converted-ethereum-tests.txt.
  • Tests: Included the type and version of evm t8n tool used to locally execute test cases: e.g., ref with commit hash or geth 1.13.1-stable-3f40e65.
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

@winsvega winsvega force-pushed the call-evmone-eofparse-during-fill branch from b34d2ac to a53e0f4 Compare April 23, 2024 10:06
@winsvega
Copy link
Collaborator

this PR consumes #516 that introduced this feature example use

@winsvega winsvega mentioned this pull request Apr 23, 2024
7 tasks
@winsvega winsvega added scope:pytest Scope: Pytest plugins type:feat type: Feature labels Apr 23, 2024
@winsvega winsvega force-pushed the call-evmone-eofparse-during-fill branch from a53e0f4 to c29c9c0 Compare April 23, 2024 10:23
@winsvega winsvega self-assigned this Apr 23, 2024
@winsvega winsvega marked this pull request as ready for review April 23, 2024 10:27
@shemnon
Copy link
Collaborator

shemnon commented Apr 24, 2024

It would be nice if this could be made generic in a manner like TransitionTool is made generic, I expect each client to expose their own tool.

Also, I would like a format that accepts required arguments. Besu's equivelant is evmtool code-validation and that won't pass the binary string matching task ask it is a binary plus an argument.

Combine this with the fork being validated possibly being another argument.

@marioevz
Copy link
Member Author

It would be nice if this could be made generic in a manner like TransitionTool is made generic, I expect each client to expose their own tool.

Also, I would like a format that accepts required arguments. Besu's equivelant is evmtool code-validation and that won't pass the binary string matching task ask it is a binary plus an argument.

Combine this with the fork being validated possibly being another argument.

I think @danceratopz might know a bit better than I do how to implement something along these lines.

We might need to create another package like we currently have evm_transition_tool, maybe a eof_tool with all different flavors of EOF tools implemented.

We might also need to implement new parameters to configure it in the pytest_plugins/test_filler.

My main ask when we implement this is that the normal test filling process does not fail if there no eof tool installed, only fails if we are trying to fill an EOF test.

@winsvega winsvega force-pushed the call-evmone-eofparse-during-fill branch from c29c9c0 to 59f57a0 Compare April 30, 2024 08:09
@winsvega
Copy link
Collaborator

winsvega commented Apr 30, 2024

we stick to evmone-eofparse for verification for now till later when we refactor tooling capture and expection verification

Copy link
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

Very nice.

Really like how/where eofparse is called, I had a mental block on this one and wanted to add it to the FixtureCollector - it's much more logical in the fixture generator for this use case!

Love the exceptions raised by verify_fixtures() - really clear.

After aligning with @winsvega, I've refactored the code a bit in #538, which addresses the small concerns below. Please let me know what you think.

From the above comment from @shemnon:

It would be nice if this could be made generic in a manner like TransitionTool is made generic, I expect each client to expose their own tool.

We will definitely do this! But will add this work in a separate PR as it requires quite a bit of code organization. Don't want to block this helpful addition though, so we should get this merged first.

src/ethereum_test_tools/exceptions/evmone_exceptions.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/exceptions/evmone_exceptions.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/exceptions/evmone_exceptions.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/spec/eof/eof_test.py Outdated Show resolved Hide resolved
* refactor(fw): define and use exception classes for eofparse errors

Additionally, truncate code output in the terminal if it's longer than 120 characters.

* refactor(fw): avoid importing generically named 'run' into module namespace

* feat(fw): check that eofparse exits with reasonble exit code

* refactor(fw): use bidict to map eof exceptions & evmone error messages

* feat(fw): better fail message if an exception message is missing

* refactor(fw): rename EvmoneExceptionParser to EvmoneExceptionMapper

* chore(fw): add todo; don't forget to improve undefined exception handling
@danceratopz
Copy link
Member

My main ask when we implement this is that the normal test filling process does not fail if there no eof tool installed, only fails if we are trying to fill an EOF test.

@marioevz, Regarding this, we warn at the end of the test session if evmone-eofparse was not found:
image

@danceratopz danceratopz merged commit aa61d73 into main May 1, 2024
9 checks passed
shemnon pushed a commit to shemnon/execution-spec-tests that referenced this pull request May 13, 2024
…thereum#519)

Co-authored-by: Dimitry Kh <dimitry@ethereum.org>
Co-authored-by: danceratopz <danceratopz@gmail.com>
marioevz added a commit that referenced this pull request May 28, 2024
* new(test): add tests for EOF/EIP-663 DUPN SWAPN

* improve code generation

* chore(ci): Update workflow actions to use Node.js 20 versions (#527)

* chore(ci): Update workflow actions to use Node.js 20 versions.

* chore: Add changelog.

* Add --traces support to besu (#511)

Add support for adding traces to output when using Besu.

Signed-off-by: Danno Ferrin <danno@numisight.com>

* feat(fw): call `evmone-eofparse` on generated EOF fixtures in fill (#519)

Co-authored-by: Dimitry Kh <dimitry@ethereum.org>
Co-authored-by: danceratopz <danceratopz@gmail.com>

* docs(fix): small fix to tracing report in readme cf #511 (#539)

* fix EOF return stack tests

The tests were previously corrected against a bug in Besu,

Signed-off-by: Danno Ferrin <danno@numisight.com>

* EXCHANGE

Exercise exchange operation

Signed-off-by: Danno Ferrin <danno@numisight.com>

* speling

Signed-off-by: Danno Ferrin <danno@numisight.com>

* move

Signed-off-by: Danno Ferrin <danno@numisight.com>

* feat(fw): Add EXCHANGE encoder

* new(tests): EOF - EIP-663: Invalid container due to invalid exchange

---------

Signed-off-by: Danno Ferrin <danno@numisight.com>
Co-authored-by: Paweł Bylica <pawel@ethereum.org>
Co-authored-by: spencer <spencer.taylor-brown@ethereum.org>
Co-authored-by: Mario Vega <marioevz@gmail.com>
Co-authored-by: Dimitry Kh <dimitry@ethereum.org>
Co-authored-by: danceratopz <danceratopz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:pytest Scope: Pytest plugins type:feat type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants