chore(tests): reduce Amsterdam ported_static skip list (480 to 258) - #2996
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #2996 +/- ##
===================================================
+ Coverage 92.82% 93.19% +0.36%
===================================================
Files 620 624 +4
Lines 36719 36926 +207
Branches 3342 3377 +35
===================================================
+ Hits 34086 34413 +327
+ Misses 1773 1716 -57
+ Partials 860 797 -63
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
c7dea6c to
ed4cb40
Compare
leolara
changed the title
leolara
changed the title
leolara
changed the title
leolara
changed the title
Progress summary (updated)This PR is now scoped to the changes that are independent of the exact EIP-8037 gas constants. Each one is either an unskip or an incidental- Cleared, by approach
Split out to a follow-upThe gas-measurement, Still skipped (258)Dominated by:
Reviving these needs a gas-budget change (not the same scenario), a per- |
leolara
changed the title
PR ethereum#2969 made the transaction gas limit automatic. On EIP-8037 forks the implicit limit now carries a state-gas reservoir, so tests that leave the transaction gas limit unset receive enough headroom and stop running out of gas under the two-dimensional gas model. Filling tests/ported_static/ for Amsterdam without the skip list shows 33 of these entries pass across all fixture variants. Remove them from amsterdam_skip_list.txt and recompute the per-section counts and the total, from 480 down to 447. Two sections become empty and are dropped, stCallDelegateCodesHomestead and stRecursiveCreate. Most of the cleared cases are the _suicide_end family. They set only the environment gas limit and rely on the implicit transaction gas limit, so the reservoir restores their pre-Amsterdam behaviour.
These 10 entries match no test collected on Amsterdam, so the conftest
never skips anything for them. They are dead weight in the list.
Eight are size-limit tests under stCodeSizeLimit and
stEIP3860_limitmeterinitcode. They carry valid_before("EIP7954"), and
Amsterdam includes EIP-7954, so pytest deselects them and they never
reach the skip step. Both sections empty out and are dropped.
Two are stCreateTest/test_create_address_warm_after_fail cases pinned
to a create-code-too-big-v1 parametrization that no longer exists after
the test was reparametrized.
Recompute the section counts and the total, from 447 down to 437.
PR ethereum#2969 computes the transaction gas limit automatically and adds a state-gas reservoir on EIP-8037 forks. These 69 files set an explicit transaction gas_limit that was incidental: it sufficed on older forks, but on Amsterdam the state-gas component pushed the transaction over its budget and broke the test. Leaving the gas limit unset gives each transaction the implicit limit plus reservoir, which restores the recorded full-execution behaviour. Drop the transaction gas_limit from these files, together with the helpers it orphaned (tx_gas lists, an intrinsic-gas computation) and three now-unused fork-conditional bumps that the unset limit supersedes. Filling them across every valid fork passes all variants with no residual failures, so remove their 116 entries from amsterdam_skip_list.txt. Total 437 down to 321.
test_no_src_account_create and its 1559 variant assert an INSUFFICIENT_ACCOUNT_FUNDS rejection at gas index g1 (210000). On Amsterdam EIP-8037 raises the creation intrinsic gas above that budget, so the transaction is rejected for gas before the funds check and the assertion fails. Make the g1 budget fork-conditional: leave the gas limit unset on EIP-8037 forks, where the implicit limit plus reservoir clears the intrinsic and the intended funds rejection fires again. Earlier forks keep the original 210000. Filling across every valid fork passes all variants, so remove the 16 g1 entries from amsterdam_skip_list.txt. Total 321 down to 305.
Extend the per-parametrization fix from no_src_account_create to eight more mixed files. Each sets the skip-listed gas slot to None on EIP-8037, so that parametrization gets the implicit limit plus reservoir, while the other slots and earlier forks keep their explicit gas. These slots recorded a full-execution result that broke on Amsterdam when the state-gas component pushed the transaction over its budget. The stZeroKnowledge point_mul tests run out of gas because they SSTORE the precompile result, which EIP-8037 makes more expensive; the precompile call itself is unchanged, so unsetting restores the recorded success state. Filling across every valid fork passes all variants with no failures, so remove the 47 entries from amsterdam_skip_list.txt. Total 305 down to 258.
8209510 to
924c30f
Compare
leolara
changed the title
leolara
changed the title
leolara
changed the title
leolara
changed the title
marioevz
left a comment
marioevz
left a comment
There was a problem hiding this comment.
I think this PR does a good job in pointing out which tests try to store gas at a certain point during execution, but the proposed solution is to simply introduce a new constant instead of fixing the issue and making it future proof.
I think the steps forward should be:
- We keep the changes in this PR that simply remove the
gas_limitfrom the transaction, since those effectively remove constants and make the tests more resilient. - Move the changes that introduce new constants into a new branch/issue so we can start tackling them one by one via a proper update to the test.
For (2) I think we need to create a claude skill and this should be an agentic process.
| 4: 1, | ||
| 5: 1, | ||
| 12: 32028, | ||
| 12: (7028 if fork.is_eip_enabled(8037) else 32028), |
There was a problem hiding this comment.
This doesn't look like a permanent solution IMO – we are going have to come back on the next gas repricing to re-disable it.
I would rather only re-enable these tests if we are doing dynamic recalculation of these values via a rewrite.
There was a problem hiding this comment.
I understand, I will change this to only send the tests that don't need specific repricing.
Then, perhaps I can think of a way to abstract this out, like to say declaratively what we want to happen with the gas and that gets automatically calculated, I don't know how difficult is that.
823587c to
5927cdb
Compare
leolara
changed the title
marioevz
left a comment
marioevz
left a comment
There was a problem hiding this comment.
LGTM, I ported the failing tests to the newer python ECADD+ECMUL framework, so we can proceed to merge. Thanks!
2 participants
🗒️ Description
Reduce the Amsterdam
tests/ported_static/skip list from 480 to 258entries. PR #2969 made the transaction gas limit automatic, adding a
state-gas reservoir on EIP-8037 forks, which fixes a large class of
these tests directly.
Every change here is independent of the exact EIP-8037 gas constants. It
either removes a skip entry or unsets an incidental transaction
gas_limit, so the test rides the implicit limit and the reservoir from#2969. The reduction comes in four parts:
Unskip cases that now pass (33). After feat(test-specs): Allow automatic transaction gas-limit #2969, tests that leave the
transaction gas limit unset get the implicit limit plus reservoir.
Drop obsolete entries (10). Size-limit tests deselected by
EIP-7954, and stale parametrization ids.
Unset incidental tx gas_limit (116). An explicit gas_limit that
was incidental: enough on older forks, short on Amsterdam.
Fork-conditional per-slot unset (63). Mixed files where only some
gas parametrizations broke; unset the affected slot on EIP-8037 only.
The gas-measurement,
warm_after_fail, and refund re-pins, which pinvalues derived from the current EIP-8037 constants, are split out to
leolara/fix-ported-static-amsterdam-repinsfor a follow-up PR once theconstants settle. Keeping them out of this PR makes it stable against
another recalibration.
Every change was filled across all valid forks and passes strict mypy
and ruff.
🔗 Related Issues or PRs
Follows #2969.
✅ Checklist
just statictype(scope):.mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.@ported_frommarker.Cute Animal Picture