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(unit-tests): verify transition tool support #1263

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

winsvega
Copy link
Contributor

@winsvega winsvega commented Feb 25, 2025

🗒️ Description

Despite checks, and unit test pass, evmone t8n input is still inconsistent
geth t8n requires 0x20000 format for hex input and support decimals
evmone t8n requires 0x020000 format for hex input and does not support decimals
eest t8n supports all

first I introduce a unit test CI to verify that test filling works correctly on t8n's
TODO
design a simple test that triggers interesting t8n input on all forks. (with blockheader history and all fork fields in env) to run as a reference

🔗 Related Issues

Current coverage script is broken because of t8n format inconsistency with evmone after changing fields from int to hex in:

✅ 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: A PR with removal of converted JSON/YML tests from ethereum/tests have been opened.
  • 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 fillci branch 2 times, most recently from fd1abe3 to 116eb5d Compare February 25, 2025 21:34
@winsvega winsvega added type:bug Something isn't working scope:ci Scope: Continuous Integration scope:tooling Scope: Python tools (uv, ruff, tox,...) scope:fill Scope: fill command labels Feb 26, 2025
@winsvega winsvega force-pushed the fillci branch 2 times, most recently from 37a1c4b to 2913584 Compare February 26, 2025 11:28
@winsvega winsvega requested a review from danceratopz February 26, 2025 11:43
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.

Thanks @winsvega for identifying and fixing the bug. I'm not sure about the approach here for testing the t8n tool (more below), so I moved your fix to a separate PR #1268 to get that merged more quickly.

I agree that it's really important to add better testing for our t8n support/compatibility. I would suggest however, to move all these tests to our unit tests under ./src/ and improve the unit testing flow so that it can run with any t8n tool.

Then, we add jobs to the existing tox_verify.yaml that runs these unit tests with every t8n tool we support.

@winsvega
Copy link
Contributor Author

The issue is that it requires to build geth, evmone, ... t8ns.
I don't think unit tests are building the t8ns

@winsvega winsvega changed the title workflow to verify t8n filling support feat(unit-tests): verify transition tool support Mar 14, 2025
@winsvega
Copy link
Contributor Author

I update the PR so now it is a unit test. but it will require t8n binaries to be installed before the run.

@winsvega winsvega requested a review from danceratopz March 14, 2025 11:42
@spencer-tb
Copy link
Collaborator

This will be a nice addition for the future. Note it would have caught this issue: #1276

Some thoughts below:

  1. I think we could add a new tox step in tox.ini for this (that only runs when specified locally - not by default). We will likely want to skip running these tests within pytest-framework.ini.
  2. Once added to tox.ini we should add this as step to the tox_verify.yaml workflow, but only if we have a docker image churning daily for each client t8n tool. This would be to make sure CI remains fast and we don't have to wait to long for the t8n's to build. Keeping the test run on the PR level.
  3. If we don't create docker images, then I think we should instead create a new workflow tox_t8n_verify.yaml that only runs the t8n tests in CI when PRs are merged to main, i.e only on main. I don't mind slow CI on main, but on PRs directly I'd like to keep it fast! Personally I prefer 2).

@marioevz marioevz mentioned this pull request Mar 17, 2025
@marioevz
Copy link
Member

Pushed a fix for transaction type 4 not working in Geth's t8n.

@marioevz
Copy link
Member

Still failing for some reason, and I didn't have time to debug it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:ci Scope: Continuous Integration scope:fill Scope: fill command scope:tooling Scope: Python tools (uv, ruff, tox,...) type:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants