Skip to content

fix(ported_static): fork-specific Amsterdam balance for OoG refund tests#2790

Open
leolara wants to merge 1 commit intoethereum:devnets/snøbal/4from
leolara:wt-snobal-4-amsterdam-oog-balances
Open

fix(ported_static): fork-specific Amsterdam balance for OoG refund tests#2790
leolara wants to merge 1 commit intoethereum:devnets/snøbal/4from
leolara:wt-snobal-4-amsterdam-oog-balances

Conversation

@leolara
Copy link
Copy Markdown
Member

@leolara leolara commented May 1, 2026

🗒️ Description

Add Amsterdam-specific post-state balance overrides for the OoG-refund parametrizations of test_create_oog_from_call_refunds.py and test_create2_oog_from_call_refunds.py, then drop the 36 corresponding entries from amsterdam_skip_list.txt.

EIP-8037's two-dimensional gas model changes the refund arithmetic on OoG paths in these tests. The sender ends up with a non-zero residue where Cancun/Prague/Osaka leave 0:

OoG path family Sender residue on Amsterdam
SStore / SelfDestruct / LogOp OoG (data ∈ {1,2,4,5,7,8,10,11,13,14,16,17}) 0x19CBC0 (1 690 560 wei)
SStore + Create / Create2 OoG (data ∈ {19,20,22,23}) 0x284E5C (2 641 500 wei)

For each of the five OoG expect_entries_ blocks per file, a clone with network: [">=Amsterdam"] and the residue value above is inserted before the existing network: [">=Cancun"] entry, so resolve_expect_post's first-match rule picks the Amsterdam-specific entry on Amsterdam and falls through to the original on earlier forks.

Both files now carry an @manually-enhanced marker so the change survives future scripts/filler_to_python regeneration.

Caveat — values are empirical, not derived

The constants 0x19CBC0 and 0x284E5C are the observed got balances from the failing fill output on snøbal/4 — clustered by parametrization. They have not been derived from EIP-8037's specification text. A reviewer who knows EIP-8037's refund accounting should confirm these are the right targets (and ideally that they should be exactly these values on Amsterdam).

Verification

  • --fork Amsterdam on the two files: 144 passed, 0 failed (24 parametrizations × 3 fixture variants × 2 files).
  • --fork Cancun on the two files: 144 passed, 0 failed (sanity check that the >=Cancun fall-through still works).

🔗 Related Issues or PRs

✅ 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).
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.
  • Tests: For PRs implementing a missed test case, update the post-mortem document to add an entry the list.
  • Ported Tests: All converted JSON/YML tests from ethereum/tests or tests/static have been assigned @ported_from marker.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 1, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (devnets/snøbal/4@befad1c). Learn more about missing BASE report.

Additional details and impacted files
@@                 Coverage Diff                 @@
##             devnets/snøbal/4    #2790   +/-   ##
===================================================
  Coverage                    ?   86.68%           
===================================================
  Files                       ?      599           
  Lines                       ?    37848           
  Branches                    ?     3821           
===================================================
  Hits                        ?    32807           
  Misses                      ?     4503           
  Partials                    ?      538           
Flag Coverage Δ
unittests 86.68% <ø> (?)

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.

@leolara leolara marked this pull request as ready for review May 1, 2026 09:09
@leolara leolara marked this pull request as draft May 1, 2026 09:10
EIP-8037's two-dimensional gas model changes the refund arithmetic on
OoG paths in test_create_oog_from_call_refunds and
test_create2_oog_from_call_refunds. The sender ends up with a non-zero
residue where Cancun/Prague/Osaka leave 0 — 0x19CBC0 wei for
SStore/SelfDestruct/LogOp OoG paths and 0x284E5C wei for the SStore +
CREATE/CREATE2 paths.

Add per-fork overrides for the five OoG `expect_entries_` blocks (data
indexes [1,2,4,5,7,8,10,11], [13,14], [16,17], [19,20], [22,23]) so
Amsterdam matches the new balance via resolve_expect_post's first-match
rule.  Other forks keep the original `balance=0` post-state.

Drop the 36 corresponding entries from amsterdam_skip_list.txt and mark
both test files `@manually-enhanced` to keep these overrides immune to
future regeneration.

Note: the residue values are observed empirically from the failing fill
output on snøbal/4, not derived from the EIP-8037 specification text.
A reviewer who knows EIP-8037 should confirm these are the right
targets.

Verified: --fork Amsterdam on the two test files -> 144 passed (24
parametrizations x 3 fixture variants x 2 files), 0 failed.
@leolara leolara force-pushed the wt-snobal-4-amsterdam-oog-balances branch from 53a1301 to 2f57424 Compare May 1, 2026 09:14
@leolara leolara marked this pull request as ready for review May 1, 2026 09:18
Copy link
Copy Markdown
Contributor

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

LGTM! The 0x19CBC0 and 0x284E5C values seem right to me. I left a comment about deriving the value instead of hard-coding, but feel free to get ignore if it doesn't make sense!

},
"network": [">=Amsterdam"],
"result": {
sender: Account(balance=0x19CBC0, nonce=2),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since this is manually-enhanced anyway, I wonder if it's worth making a couple helpers to do this calculation instead of hard coding so that when cpsb inevitably changes, this test (and the create test below) will automatically pick it up. I think we also won't need this >=Amsterdam branch because these helpers will return 0 before Amsterdam. Maybe something like:

  residue_sstore = (
      fork.create_state_gas(code_size=0)
      + fork.sstore_state_gas()
  ) * gas_price

Then this line becomes something like:

Suggested change
sender: Account(balance=0x19CBC0, nonce=2),
sender: Account(balance=residue_sstore, nonce=2),

"indexes": {"data": [19, 20], "gas": -1, "value": -1},
"network": [">=Amsterdam"],
"result": {
sender: Account(balance=0x284E5C, nonce=2),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similarly, you could have another helper like:

  residue_sstore_create = (
      fork.create_state_gas(code_size=0)
      + fork.create_state_gas(code_size=1)
  ) * gas_price

and then this line becomes:

Suggested change
sender: Account(balance=0x284E5C, nonce=2),
sender: Account(balance=residue_sstore_create, nonce=2),

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.

2 participants