feat(tests): complete EIP-7981 coverage and testing checklist - #3537
danceratopz wants to merge 13 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #3537 +/- ##
================================================
Coverage 94.44% 94.44%
================================================
Files 624 624
Lines 36928 36928
Branches 3326 3326
================================================
Hits 34875 34875
Misses 1450 1450
Partials 603 603
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:
|
fb73a23 to
906c947
Compare
b9126b3 to
4b26471
Compare
a214f03 to
5cb8faa
Compare
Expand test_access_list_data_cost_with_execution with calldata-floor and execution-cost dominance for zero and nonzero calldata, retaining transaction types 1 and 2. These cases catch a surcharge omitted from either side of the gas-used maximum. Derive the surcharge independently of the access-list calculator and provide surplus gas so an out-of-gas transaction cannot satisfy the receipt assertion simply by consuming the entire gas limit. Validation: all 24 focused EELS fill cases and just static pass against the unrefactored specification on forks/amsterdam.
Add test_execution_gas_after_access_list_surcharge. The existing OOG checklist markers only cover transactions rejected before execution for insufficient intrinsic or floor gas. They do not prove that EIP-7981's surcharge is deducted before the EVM runs. Add an exact execution-gas boundary around a warm SSTORE for transaction types 1 and 2. Assert receipt status, gas used and storage rollback with one gas too little, exact gas and one extra gas. This supplies actual execution-OOG evidence for the gas-cost checklist item. Validation: focused EELS fill and just static pass before this commit.
Build the transaction gas limit from the gas the transaction is expected to use, including the top-frame authorization gas of type 4 transactions, and attach an expected receipt with success status and that exact gas to every valid fixture-built transaction. Until now the fixture limit stopped at the intrinsic cost, so type 4 transactions ran out of gas while processing their authorization, and the gas-measurement tests only checked acceptance. Add a gas surplus fixture, on by default, so a receipt that equals the gas limit cannot pass by consuming everything. The exact gas limit test sets it to zero. Annotate the authorization fixtures with whether they create an account so the top-frame gas is modeled. Remove the manual receipt in the calldata floor test, which the fixture now supersedes.
The shared error fixture returned the intrinsic gas exception for every negative gas delta. When the calldata floor exceeds the intrinsic cost, the gas limit sits between the two and the specification rejects on the floor check, so record that exception. This changes the floor validation test; the intrinsic gas cases keep their exception. Nimbus reports the intrinsic exception for these blocks because it rejects a block with transactions and zero gas used before validating any transaction. That needs invalid-transaction blocks to carry a nonzero header gas used, which is a separate change.
Add test_access_list_floor_with_exact_balance. The existing exact-balance scenario only rejected a gas limit midway through the surcharge gap. The data-floor checklist requires both valid and invalid transactions when the floor exceeds intrinsic gas. Use floor minus one, exact floor and floor plus one limits. Assert receipt status, gas used and the sender's final balance for valid cases, with full upfront funding for both transaction types 1 and 2. Correct the marker from execution OOG to data-floor intrinsic validity. Validation: focused EELS fill and just static pass before this commit.
Map gas-limit and data-floor rejection to intrinsic validity rather than execution OOG. Credit exact valid limits and contract creation with their specific checklist items; preserve actual gas measurements. Validation: module EELS fill and just static pass before this commit.
Both gas-transition tests already assert fork-specific billed gas through receipts or sender balances. Add the gas-measurement mapping alongside their existing before/after activation decorators. The validity-transition test already covers all four required outcomes. Validation: module EELS fill and just static pass before this commit.
Strengthen test_duplicate_access_list_entries. The existing duplicate-address-entry case does not detect a client that deduplicates storage keys within each entry. Add that distinct shape to the measured duplicate test without multiplying unrelated parameters. This is an optional improvement beyond checklist closure. Validation: focused EELS fill and just static pass before this commit.
Add test_access_list_data_cost_at_crossover. The existing surcharge regression tests well-separated floor and execution dominance. Add the exact crossover and adjacent execution gas values for zero and nonzero calldata, retaining the existing cases. Use surplus gas and exact receipts to detect a misplaced max operation. This is an independent improvement beyond checklist closure. Validation: focused EELS fill and just static pass before this commit.
Add test_access_list_surcharge_with_refund. EIP-7981 preserves refund rules, so the new-refund checklist section is not applicable. Add independent interaction coverage: clear a warm slot, optionally revert, and assert storage, receipt status and gas after refunds with and without calldata-floor dominance. This checks that the surcharged floor still binds and revert discards the refund. Validation: focused EELS fill and just static pass before this commit.
Add test_access_list_surcharge_with_recipient_costs. The surcharge must survive Amsterdam's different intrinsic bases for self-transfers and value-bearing transfers. The previous cases only used non-self recipients with zero value. Add focused combinations of self/other recipients and zero/nonzero value with and without floor dominance. Derive the expected surcharge separately from calculators without an access list, and assert success and billed gas with surplus gas. Use an existing funded recipient so account creation cannot obscure the repricing being measured. This is an optional interaction regression beyond checklist closure. Validation: focused EELS fill and just static pass before this commit.
Replace the blanket transaction-type exclusion with exclusions for the rules EIP-7981 leaves unchanged, keeping the exact and insufficient gas limit, floor validity and contract creation items for the repriced transaction types. Record the coverage evidence as self-contained statements and mark the optional second-client instrumentation as not collected rather than as validation.
Since ethereum#3223 the pin has been the hash of the EIPs commit that moved the EIP to Review, so the version check never matched a blob. Use the blob hash of eip-7981.md in that same commit. The pinned content is unchanged. Relative to the blob pinned before ethereum#3223 (954963f), the only difference is the status change from Draft to Review.
5cb8faa to
e0e8200
Compare
LouisTsai-Csie
left a comment
There was a problem hiding this comment.
Thanks for the checklist PR. A few ideas:
- Two-dimensional gas: execution cost with access list dominates header cost.
- Combine
test_access_list_data_cost_with_executionandtest_access_list_data_cost_at_crossoveras they have similar structure.
| [pytest.param("eoa", id="")], | ||
| indirect=True, | ||
| ) | ||
| def test_access_list_floor_cost_with_calldata( |
There was a problem hiding this comment.
Test name and docstring indicate calldata floor domination, but pinning the delegated recipe in conftest means we can't guarantee that anymore. Like in this test, Intrinsic cost outperforms the floor.
Not a coverage gap though; at_crossover and with_refund already cover the floor.
| top_frame_gas = fork.transaction_top_frame_gas_calculator()( | ||
| contract_creation=contract_creating_tx, | ||
| authorizations=authorization_list or [], | ||
| ) |
There was a problem hiding this comment.
Should this be transaction_top_frame_execution_gas instead of transaction_top_frame_gas_calculator? As the comparison is max(intrinsic + execution, floor). It is not breaking any tests since which no state gas is charged within the test suite.
| GAS_SURPLUS = 1000 | ||
| """ | ||
| Gas added to the limit of valid fixture-built transactions. | ||
|
|
||
| With the exact expected gas as the limit, a receipt equal to the limit | ||
| cannot tell exact billing from consuming everything that was available. | ||
| The surplus makes the receipt show that unused gas is returned as well. | ||
| Boundary tests override it with zero through the `tx_gas_surplus` fixture. | ||
| """ |
There was a problem hiding this comment.
This is only used by tx_gas_surplus, I think we could remove this.
| @pytest.fixture | ||
| def authorization_refund() -> bool: | ||
| """ | ||
| Return whether the transaction has an existing authority in the | ||
| authorization list. | ||
| """ | ||
| return False |
There was a problem hiding this comment.
This is parameterized but always False. Is there missing coverage here?
Second, EIP2780.refund_types() filters out AUTHORIZATION_EXISTING_AUTHORITY, so in Amsterdam there is no such refunds. The flag now only decides whether the authority account pre-exists (hence its new creates_account wiring). So should we rename authorization_refund here?
| if not hasattr(request, "param"): | ||
| if tx_type == 4: | ||
| return [ | ||
| AuthorizationTuple( | ||
| signer=pre.fund_eoa(1 if authorization_refund else 0), | ||
| address=Address(1), | ||
| creates_account=not authorization_refund, | ||
| ) | ||
| ] | ||
| return None | ||
| if request.param is None: | ||
| if tx_type == 4: | ||
| return [ | ||
| AuthorizationTuple( | ||
| signer=pre.fund_eoa(1 if authorization_refund else 0), | ||
| address=Address(1), | ||
| creates_account=not authorization_refund, | ||
| ) | ||
| ] | ||
| return None |
There was a problem hiding this comment.
We could fold these two statements into:
param = getattr(request, "param", None)
if param is None:
if tx_type == 4:
pass| transaction_type/test/intrinsic_validity/nonce_minus_one = EIP-7981 reprices existing access-list transactions; it does not introduce a transaction type or change this rule. | ||
| transaction_type/test/intrinsic_validity/nonce_plus_one = EIP-7981 reprices existing access-list transactions; it does not introduce a transaction type or change this rule. | ||
| transaction_type/test/intrinsic_validity/nonce_exact = EIP-7981 reprices existing access-list transactions; it does not introduce a transaction type or change this rule. | ||
| transaction_type/test/intrinsic_validity/to = EIP-7981 reprices existing access-list transactions; it does not introduce a transaction type or change this rule. |
There was a problem hiding this comment.
I think this is covered by test_contract_creation_with_access_list?
| transaction_type/test/intrinsic_validity/value_non_zero_insufficient_balance = EIP-7981 reprices existing access-list transactions; it does not introduce a transaction type or change this rule. | ||
| transaction_type/test/intrinsic_validity/value_non_zero_sufficient_balance = EIP-7981 reprices existing access-list transactions; it does not introduce a transaction type or change this rule. | ||
| transaction_type/test/intrinsic_validity/value_zero_insufficient_balance = EIP-7981 reprices existing access-list transactions; it does not introduce a transaction type or change this rule. | ||
| transaction_type/test/intrinsic_validity/value_zero_sufficient_balance = EIP-7981 reprices existing access-list transactions; it does not introduce a transaction type or change this rule. |
There was a problem hiding this comment.
I think we should still test out these cases!
- value_non_zero_insufficient_balance
- value_non_zero_sufficient_balance
- value_zero_insufficient_balance
- value_zero_sufficient_balance
|
Follow up review PR: danceratopz#61 Fire away! :) |
Description
Strengthen EIP-7981 tests to check successful execution and exact gas charges.
Nimbus rejects a block that has transactions and a header gasUsed of zero before it validates any transaction, so it reports the intrinsic gas exception for the below-floor cases of the floor validation and exact-balance tests. Those cases pass on Nimbus once invalid-transaction blocks carry a nonzero header gasUsed, which is the subject of the follow-up #3599. This is kinda low prio. Currently exception mapping is disabled for nimbus (in the hive sim Dockerfiles), see #3603.
Validation at the current head:
just staticpasses, and all 731 EIP-7981 fill cases pass with--until Amsterdam --generate-all-formats, including 184 EngineX fixtures and the activation transitions. The regenerated checklist covers all 15 applicable items with 254 items marked N/A and no unmarked tests.consume enginexagainst go-ethereum, Nethermind, ethrex, Erigon, Besu and Reth passes all 186 cases each; Nimbus passes 174 and fails the 12 below-floor cases named above until the follow-up lands. Devnet execution andconsume engineremain open.Related Issues or PRs
tests@v21.0.0(Glamsterdam Testnet) #3217Checklist
just static<type>(<area>): <title>, where<type>and<area>come from an appropriateC-<type>, respectivelyA-<area>, label. The title should match the target squash commit message.Cute Animal Picture