Skip to content

feat(forks,ci,tox): Create future feature #1385

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

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

feat(forks,ci,tox): Create future feature #1385

wants to merge 5 commits into from

Conversation

marioevz
Copy link
Member

@marioevz marioevz commented Apr 2, 2025

🗒️ Description

Renames eip7692 to future, which will now onwards point to the current mainnet deployed hardfork + 2.

  • stable: Current mainnet deployed hardfork (Currently Cancun)
  • develop: Current mainnet deployed hardfork + 1 (Currently Prague)
  • future: Current mainnet deployed hardfork + 2 (Currently Osaka)

🔗 Related Issues

None

✅ 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.

@marioevz marioevz added scope:forks Scope: Changes ethereum_test_forks package type:chore Type: Chore scope:fw Scope: Framework (evm|tools|forks|pytest) labels Apr 2, 2025
Copy link
Contributor

@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.

LGTM! Just some small comments. Feel free to use :)

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.

Nice suggestion and, in general, I'm really happy to improve our fixture release structure and think this goes in the right direction, but:

  1. I wouldn't make this change so shortly before a hard-fork, I think this causes unnecessary changes for downstream teams. I would make this change post Prague. Edit: As-is, this PR doesn't break anything, got corrected by Spencer below 🙂
  2. I find the label future misleading; before reading the PR, I assumed that other future EVM features would be included in this release, even for fork N+2. I don't consider this to be coherent naming, e.g., how can we explain to teams that, yes, stateless tests are for a future upgrade, but they're not part of the future release yet.
  3. This configures all Osaka tests to be filled by evmone, not eels, which is not the direction we're aiming for. It might be a technicality, but I think we should only make this change once EELS has full EOF support and we fill EOF tests with EELS. If we assume that's not going to be anytime soon, we might find ourselves having to unwind this change in order to fill EOF with evmone and other Osaka SFI'd EIPs.

About 2., I suggest naming the releases explicitly:

  • cancun
  • prague
  • osaka

I also think it would be cleaner to separate the fork classes change to a separate PR as they're not strictly tied to this packaging change.

@danceratopz
Copy link
Member

One more comment, I think we should also quickly consider how releases look after the ethereum/tests yaml/json files have been moved to eest. We we still be able to deliver a single tarball of all tests for Cancun for example. I think it should be reasonable, but let's check real quick.

@danceratopz danceratopz changed the title feat(forks,github,tox): Create future feature feat(forks,ci,tox): Create future feature Apr 3, 2025
@danceratopz danceratopz added the scope:ci Scope: Continuous Integration label Apr 3, 2025
@spencer-tb
Copy link
Contributor

Just to push back a little, as I prefer the generality of the naming :)

  1. I wouldn't make this change so shortly before a hard-fork, I think this causes unnecessary changes for downstream teams. I would make this change post Prague.

From my side I don't think this will affect Prague at all, only Osaka

  1. I find the label future misleading; before reading the PR, I assumed that other future EVM features would be included in this release, even for fork N+2. I don't consider this to be coherent naming, e.g., how can we explain to teams that, yes, stateless tests are for a future upgrade, but they're not part of the future release yet.

I tend to agree with this! What about upcoming, to me this implies the fork after next?

About 2., I suggest naming the releases explicitly:

  • cancun
  • prague
  • osaka

I'm also open to this, verbosity is nice. If we choose this option then we should indeed wait until after the Prague fork. I do prefer stable/develop/upcoming as it means we have minimal changes for client CIs. They only need to point there client to the latest stable release. If we go with fork names explicitly they have to change there CI more (not necessarily a bad thing).

@danceratopz
Copy link
Member

Just to push back a little, as I prefer the generality of the naming :)

  1. I wouldn't make this change so shortly before a hard-fork, I think this causes unnecessary changes for downstream teams. I would make this change post Prague.

From my side I don't think this will affect Prague at all, only Osaka

Thanks @spencer-tb, this is correct, I updated the comment above to reflect this 🙏

Copy link
Contributor

@winsvega winsvega left a comment

Choose a reason for hiding this comment

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

Also the get_all_forks return sequence of forks. and some of them are not supposed to be used in test generation. like glaciers.

@spencer-tb spencer-tb added the needs-discussion Needs discussion before proceeding label Apr 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-discussion Needs discussion before proceeding scope:ci Scope: Continuous Integration scope:forks Scope: Changes ethereum_test_forks package scope:fw Scope: Framework (evm|tools|forks|pytest) type:chore Type: Chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants