Skip to content

feat(tests): close out eip-7954 checklist - #3546

Open
LouisTsai-Csie wants to merge 4 commits into
ethereum:forks/amsterdamfrom
LouisTsai-Csie:tests/amsterdam/eip-7954-closeout
Open

LouisTsai-Csie wants to merge 4 commits into
ethereum:forks/amsterdamfrom
LouisTsai-Csie:tests/amsterdam/eip-7954-closeout

Conversation

@LouisTsai-Csie

@LouisTsai-Csie LouisTsai-Csie commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Description

Fills the remaining coverage gaps, brings the EIP checklist to 100%, hardens assertions that could pass vacuously, and completes the fork transition matrix.

Related Issues or PRs

issue #3217

Checklist

  • Ran fast static checks to avoid CI fails, see Code Standards & Verifying Changes: just static
  • PR title has the form <type>(<area>): <title>, where <type> and <area> come from an appropriate C-<type>, respectively A-<area>, label. The title should match the target squash commit message.

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@LouisTsai-Csie LouisTsai-Csie added C-feat Category: an improvement or new feature A-tests Area: Consensus tests. labels Sep 8, 2026
@LouisTsai-Csie LouisTsai-Csie self-assigned this Sep 8, 2026
@LouisTsai-Csie LouisTsai-Csie changed the title tests: close out eip-7954 checklist feat(tests): close out eip-7954 checklist Sep 8, 2026
@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.44%. Comparing base (4963382) to head (6a828f9).
⚠️ Report is 38 commits behind head on forks/amsterdam.

Additional details and impacted files
@@                 Coverage Diff                 @@
##           forks/amsterdam    #3546      +/-   ##
===================================================
+ Coverage            94.01%   94.44%   +0.42%     
===================================================
  Files                  624      624              
  Lines                36927    36928       +1     
  Branches              3326     3326              
===================================================
+ Hits                 34717    34875     +158     
+ Misses                1533     1450      -83     
+ Partials               677      603      -74     
Flag Coverage Δ
unittests 94.44% <ø> (+0.42%) ⬆️

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

☔ View full report in Codecov by Harness.
📢 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.

The max-initcode gas boundary took its gas limit from
`transaction_intrinsic_cost_calculator()`, which returns the greater of the
standard intrinsic cost and the EIP-7976 calldata floor. At
`MAX_INITCODE_SIZE` the floor is 8,412,608 gas against a standard cost of
556,588, so the case walked the floor's boundary while declaring
`INTRINSIC_GAS_TOO_LOW`. EELS maps both rejections to a single error, so it
filled clean, but geth's mapper separates them.

Take the threshold from `transaction_data_floor_cost_calculator()` directly,
expect `INTRINSIC_GAS_BELOW_FLOOR_GAS_COST`, and assert the floor is the
greater of the two so that a fork which reverses that cannot move the test
onto the other rule unnoticed.

Add an `above_floor` case. At the boundary the gas charged equals the gas
limit, so a receipt pin there holds under either rule; with slack above the
floor the receipt matches only if the floor is what is priced. The
deployment itself spends 741,748 gas, so the pinned figure is one that only
the floor produces.

Rename to `test_max_initcode_size_calldata_floor` and update `test_cases.md`
to match.
…uously

- `test_max_code_size_deposit_gas`: pin the receipt. The exact fit states
  the 128 KiB total outright rather than through EIP-8037 grant mechanics,
  and the short case pins the gas cap, proving the deposit failed as an
  exceptional halt and not a revert, which post-state alone cannot tell.
- Jump tests: pre-set a sentinel in the caller. A stored zero matched both
  "the CALL returned failure" and "the caller never ran"; now it can only
  mean the CALL executed and failed.
- `test_max_initcode_size_via_create[over_max]`: pin the block access list.
  The initcode size is rejected before the child address is computed, so
  the address must be absent and the factory untouched.
- `eip_checklist_external_coverage.txt`: state the mechanism behind the
  `test_coverage` claim (positive sibling one byte smaller, pre-set
  sentinels) instead of asserting no case is vacuous.
- `test_cases.md`: the invalid jump target is a `STOP`, not a `PUSH1`.
@fselmo

fselmo commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Nice work! No real comments, just some enhancements to your changes here. I did want to hold off on a merge though bc I investigated a bit an existing test case that a Claude review flagged. Since I ended up investigating and fixing some issues with it, I PR'd it to your branch here at LouisTsai-Csie#19.

Nothing else really stands in the way here. I just made a second commit to that PR adding extra pins to some tests since I've been trying to think of ways to make sure our tests don't drift from original intent. Feel free to comment on that PR and continue the conversation! I will happily approve this once we've resolved at least that broken test (first commit in the PR) so lmk what you think of the changes there.

@fselmo
fselmo force-pushed the tests/amsterdam/eip-7954-closeout branch 2 times, most recently from 2cd3d71 to e2ffc8c Compare September 10, 2026 13:10
@fselmo

fselmo commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Ignore my push here! I think it essentially didn't do anything. I ended up accidentally rebasing this branch on something and then reset it. Sorry for the trouble 🫣

@spencer-tb spencer-tb self-assigned this Sep 14, 2026
fix(tests): pin the EIP-7954 max-initcode boundary to the calldata floor
@spencer-tb

Copy link
Copy Markdown
Contributor

Created a PR to yours here: LouisTsai-Csie#20

Feel free to add, some tweaks and extra coverage! :)

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

Labels

A-tests Area: Consensus tests. C-feat Category: an improvement or new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants