Skip to content

fix(test-forks): treat transition fork variants as equal to canonical#2782

Merged
danceratopz merged 3 commits intoethereum:forks/amsterdamfrom
danceratopz:fix-phase-1
Apr 30, 2026
Merged

fix(test-forks): treat transition fork variants as equal to canonical#2782
danceratopz merged 3 commits intoethereum:forks/amsterdamfrom
danceratopz:fix-phase-1

Conversation

@danceratopz
Copy link
Copy Markdown
Member

@danceratopz danceratopz commented Apr 30, 2026

Description

TransitionBaseClass.with_env_gas_limit produced fresh class objects that compared unequal via the default type.__eq__, breaking pre-alloc group reuse during fill --generate-pre-alloc-groups and tripping the add_test_pre assertion (Incompatible fork: CancunToPragueAtTime15k!=CancunToPragueAtTime15k).

Mirror BaseForkMeta by adding __eq__/__hash__ on TransitionBaseMetaClass keyed on the canonical identity, set _base_fork on the variant inside with_env_gas_limit, and widen _identity/_maybe_transitioned signatures to type so the metaclass can reuse them. Adds a regression test covering transition fork variant equality.

Related Issues or PRs

Bug introduced by #2690 (feat(test-forks): Gas-limit-aware Fork, fork-aware Environment).

Checklist

  • All: Ran fast static checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:
    just static
  • All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
  • All: Considered updating the online docs in the ./docs/ directory.
  • All: Set appropriate labels for the changes (only maintainers can apply labels).

@danceratopz danceratopz added C-bug Category: this is a bug, deviation, or other problem A-test-forks Area: execution_testing.forks labels Apr 30, 2026
Cover the case where `with_env_gas_limit` variants of a transition fork
compare unequal to their canonical class and to each other, breaking
pre-alloc group reuse during fill.
`TransitionBaseClass.with_env_gas_limit` produced new class objects that
compared unequal via the default `type.__eq__`, breaking pre-alloc group
reuse during fill (assertion in `add_test_pre`).

Mirror `BaseForkMeta` by adding `__eq__`/`__hash__` on
`TransitionBaseMetaClass` keyed on the canonical identity, and set
`_base_fork` on the variant inside `with_env_gas_limit`. Widen
`_identity`/`_maybe_transitioned` signatures to `type` so the metaclass
can reuse them.
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 30, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.17%. Comparing base (bf9dbb4) to head (ea84993).

Additional details and impacted files
@@               Coverage Diff                @@
##           forks/amsterdam    #2782   +/-   ##
================================================
  Coverage            88.17%   88.17%           
================================================
  Files                  577      577           
  Lines                35659    35659           
  Branches              3490     3490           
================================================
  Hits                 31442    31442           
  Misses                3654     3654           
  Partials               563      563           
Flag Coverage Δ
unittests 88.17% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@felix314159 felix314159 left a comment

Choose a reason for hiding this comment

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

lgtm, fill with --generate-all-formats passed for bal and the unit test passed too.

but we also have to be a bit careful because a footgun for e.g. transition forks would be sth like:

from execution_testing.forks import Amsterdam

# transition from bpo2 to amsterdam to get post with gas limit 120 mil
if post == Amsterdam:
    post = Amsterdam  # replaces variant with canonical Amsterdam (`_env_gas_limit=0`)

basically just cuz you have the same canonical fork identity it does not mean u have the same variant state

@danceratopz
Copy link
Copy Markdown
Member Author

lgtm, fill with --generate-all-formats passed for bal and the unit test passed too.

👍

but we also have to be a bit careful because a footgun for e.g. transition forks would be sth like:

from execution_testing.forks import Amsterdam

# transition from bpo2 to amsterdam to get post with gas limit 120 mil
if post == Amsterdam:
    post = Amsterdam  # replaces variant with canonical Amsterdam (`_env_gas_limit=0`)

basically just cuz you have the same canonical fork identity it does not mean u have the same variant state

Agreed. If this becomes commonplace, we could look into protecting ourselves against this (something like wrapping (fork_cls, env_gas_limit) in a small @dataclass(frozen=True). I think it's ok for now; but yeah anyone using this has to be aware of the potential consequences.

@danceratopz danceratopz merged commit 9d18733 into ethereum:forks/amsterdam Apr 30, 2026
25 checks passed
marioevz added a commit to marioevz/execution-specs that referenced this pull request May 8, 2026
…ent`

Revert "fix(test-forks): treat transition fork variants as equal to canonical (ethereum#2782)"

This reverts commit 9d18733.

This reverts commit c3462e0.
marioevz added a commit that referenced this pull request May 8, 2026
…ent` (#2762)

Revert "fix(test-forks): treat transition fork variants as equal to canonical (#2782)"

This reverts commit 9d18733.

This reverts commit c3462e0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-test-forks Area: execution_testing.forks C-bug Category: this is a bug, deviation, or other problem

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants