Skip to content

feat(tests,specs): eip7928 extended coverage - #3566

Merged
spencer-tb merged 13 commits into
ethereum:forks/amsterdamfrom
fselmo:feat/eip7928-extended-coverage
Sep 17, 2026
Merged

spencer-tb merged 13 commits into
ethereum:forks/amsterdamfrom
fselmo:feat/eip7928-extended-coverage

Conversation

@fselmo

@fselmo fselmo commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Description

Closes out the EIP-7928 section of the Amsterdam test-release tracker. This doesn't claim to exhaust all possible BALs cases but should get us in great shape for the #3217 scope.

One spec fix, flagged by @gurukamath: the BAL builder diffed each merge unit against the running block state, so two pre-execution system calls at index 0 that undid each other recorded a (0, 0) storage change instead of a read. It now diffs against the state at the start of the block access index. This doesn't affect any existing test, nor does it affect any behavior on mainnet today, but is an inconsistency in the spec implementation and something that now has coverage for any pre-execution contracts that interact with each other in this way. The t8n also ran the two pre-execution calls in the opposite order from apply_body, which this PR aligns and pins down with a test.

Increasing coverage on the gaps a coverage diff and a few CC w/ Fable iterations turned up:

  • netting at both boundary indices, and a failing pre-execution system call with its "invalid block" counterpart
  • value calls that fail (the besu#11124 / erigon#23670 shape), on both credit paths of generic_call()
  • addresses that must appear exactly once (sender == coinbase) or not at all (EXTCODE* on a delegation, BLOCKHASH against the EIP-2935 contract, a precompile named in a tx access list but never actually called)
  • burned blob fees leaving the sender and reaching no account
  • value reaching a precompile by CALL and CALLCODE, not only as a transaction's recipient
  • the first BAL-asserting deposit request
  • six intra-account ordering negatives, with the reverse_* modifiers behind them (more invalid tests)
  • adds mainnet tests (test_eip_mainnet.py) for tracker EIP-7928 Implementation Tracker: Block-Level Access Lists #1878

Where an existing test already ran a relevant scenario, a BAL expectation was added to pin the Amsterdam behavior, rather than re-writing that same coverage under Amsterdam. Each new claim was broken / mutated once in what it pins to confirm it doesn't silently still fill.

Related Issues or PRs

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

20240903_125929

@fselmo fselmo changed the title Feat/eip7928 extended coverage feat(tests,specs): eip7928 extended coverage Sep 11, 2026
@codecov

codecov Bot commented Sep 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.47%. Comparing base (b20964f) to head (0509b19).
⚠️ Report is 10 commits behind head on forks/amsterdam.

Additional details and impacted files
@@                 Coverage Diff                 @@
##           forks/amsterdam    #3566      +/-   ##
===================================================
+ Coverage            94.44%   94.47%   +0.03%     
===================================================
  Files                  624      624              
  Lines                36929    36969      +40     
  Branches              3326     3334       +8     
===================================================
+ Hits                 34876    34925      +49     
+ Misses                1450     1443       -7     
+ Partials               603      601       -2     
Flag Coverage Δ
unittests 94.47% <100.00%> (+0.03%) ⬆️

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.

@spencer-tb
spencer-tb self-requested a review September 14, 2026 14:05
@spencer-tb spencer-tb self-assigned this Sep 14, 2026
@fselmo
fselmo force-pushed the feat/eip7928-extended-coverage branch from d394d5e to 303e2fc Compare September 14, 2026 18:49
@fselmo
fselmo force-pushed the feat/eip7928-extended-coverage branch from 303e2fc to eb7e98c Compare September 14, 2026 19:23
@fselmo
fselmo marked this pull request as ready for review September 14, 2026 22:38
spencer-tb and others added 2 commits September 15, 2026 09:38
* chore(tests): pin EIP-7928 to its merged head d2a64c2d

`f834f000…` no longer exists in ethereum/EIPs: it was a pre-force-push
commit of EIPs#12277, which squash-merged on 2026-09-09 as
d2a64c2d4cc44f2f507577d0ebfb110dcc21d358 (rationale-only wording change).
The stateful benchmark refresh copied the dead hash and the compute
benchmark test still carried aca88aa0. PR CI only collects the version
checks, so none of the three were flagged.

* chore(tests): annotate precompile parameters as Address

The `with_all_precompiles` marker yields an `Address`, which is what
`test_bal_precompile_funded` already declares; match it in the two other
tests that consume the same parameter.

* chore(tests): attribute the coinbase BAL entry to the fee credit

The coinbase lands in the BAL because the priority-fee credit touches it
even when the fee is zero (`create_ether(coinbase, transaction_fee)` in
`process_transaction`), not because EIP-3651 warms it: warmth is a gas
property and records no access. Reword both gas-limit boundary comments.

* fix(test-client-clis): map reth's BAL decode failure to INVALID_BLOCK_ACCESS_LIST

reth rejects a non-minimally encoded BAL with "failed to decode block
access list: leading zero". The mapper had no pattern for it, so six
`test_bal_invalid_non_minimal_scalar_encoding` engine fixtures failed as
"Undefined exception message" although reth rejects the block.

* chore(tests): accept the header-hash verdict for undecodable BAL payloads

A client that hashes the payload's BAL bytes into its reconstructed
header before validating the encoding (reth) answers INVALID_BLOCK_HASH
where the fixtures only accepted INVALID_BLOCK_ACCESS_LIST. EIP-7928
fixes neither order, and both verdicts reject the block, so
`test_bal_invalid_engine_payload_encoding` and
`test_bal_invalid_non_minimal_scalar_encoding` now accept either. Nine
reth consume-engine failures on the 2026-09-14 glamsterdam run.

* fix(tests): keep test_tx_gas_limit below the EIP-7928 item cap

With a 21000 block gas limit the BAL item cap is 10, but an empty
Amsterdam block already carries 25 items of system-contract reads and
writes, so the engine payload is invalid twice over. besu, geth, reth
and nimbus report the cap before the transaction's gas allowance
(ethereum#3439) while EELS reports the allowance first. A 100000 limit keeps the
transaction the only defect; every fork still fills with
GAS_ALLOWANCE_EXCEEDED.

* test(bal): reject malformed encodings with matching header commitments

* test(bal): require empty change lists for zero-value recipients

* fix(test-fill): isolate pre-fork BAL hash rejection from API validation
@fselmo

fselmo commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

@spencer-tb, merged your PR and did a touch of cleaning up on top. Ready for re-review on your end.

fselmo added a commit to lambdaclass/execution-specs that referenced this pull request Sep 15, 2026
Byte-identical to the forks/amsterdam hunk so the next focil rebase resolves cleanly; fixture output is unchanged.
fselmo added a commit that referenced this pull request Sep 15, 2026
…ration (#3471)

* fix(tests): clear rlp_modifier when building inclusion-list variants (#3406)

The inclusion-list variant moves the last transaction of the last block
out of the block body and into the inclusion list, then clears the
expectations derived from it: header_verify, expected_gas_used and
expected_block_access_list. rlp_modifier was left in place.

rlp_modifier force-writes header fields onto the built block after the
transition tool has run, and tests compute it from the pre-move
transaction list. The moved transaction therefore keeps contributing to
the header even though it no longer executes in the block, producing
fixtures whose header contradicts their own body.

Co-authored-by: Marc Harvey-Hill <10379486+Marchhill@users.noreply.github.com>

* fix(focil): omit inclusion list result for invalid payloads

* fix(tests): give test_tx_gas_limit a block gas limit a valid block fits in

The test pinned the block gas limit to 21000 with a 21001-gas transaction to
trigger GAS_ALLOWANCE_EXCEEDED. On forks with the EIP-7928 block-access-list
item budget (gas_limit // 2000) a 21000-gas-limit block allows 10 items, which
is below the protocol-level writes of even an empty block, so no block in this
environment can be valid. The test itself never noticed - its block is expected
invalid - but its auto-generated inclusion-list variant moves the failing
transaction into the inclusion list and expects the emptied block to be VALID,
producing a fixture that the spec's own state transition rejects
(tests-focil-devnet@v0.2.0, blockchain_test_engine_inclusion_list, the one
non-blob entry in the 127 broken engine fixtures).

Scale the numbers to 100_000/100_001: the allowance check still fires the same
way, and the item budget (50) now accommodates an empty block, so the
inclusion-list variant fills to a genuinely valid block.

* fix(test-specs): splat the metadata regression block from untyped kwargs

The regression test from #3445 constructs a BuiltBlock through
model_construct with object() sentinels for fields that
get_fixture_engine_new_payload only forwards, which the typechecker rejects
for missing and mistyped named arguments. model_construct skips validation
and the sentinels are never read, so splat them from a dict[str, Any]
instead of passing them as checked named arguments.

* fix(tests): align test_tx_gas_limit with its #3566 counterpart

Byte-identical to the forks/amsterdam hunk so the next focil rebase resolves cleanly; fixture output is unchanged.

---------

Co-authored-by: Marc <Marchhill@users.noreply.github.com>
Co-authored-by: Marc Harvey-Hill <10379486+Marchhill@users.noreply.github.com>
Co-authored-by: chugarchugarr <josephlerma19@gmail.com>
Co-authored-by: Felipe Selmo <fselmo2@gmail.com>
@fselmo

fselmo commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

Actually, you can review but please don't merge yet. There's a quirk about using pre-execution contacts since those EIPs don't require the client to make a system call. It says they may update the state directly. I want to make sure we are writing the best cases around this. Will update here tomorrow with more details.

fselmo added a commit that referenced this pull request Sep 16, 2026
…es to eip-7805 (#3595)

* fix(github): EIP-7805 Devnet config

Backported without the whitespace-only runs-on reformatting.

(cherry picked from commit 5f46130)

* fix(consume): require null inclusionListSatisfied on non-VALID payloads (#3470)

Port the engine simulator's inclusionListSatisfied guard from #3410 to the
devnets/focil/0 simulator. The check previously asserted a non-null response
field whenever the fixture stamped one, regardless of the payload's expected
status; execution-apis bogota.md requires the field to be null for any payload
not deemed VALID, so the check demanded a spec violation. tests-focil-devnet
v0.2.0 stamps the field on 5,489 expected-INVALID engine fixtures, which fail
for every client on the Hive focil board with "expected
inclusion_list_satisfied in response".

The ported guard is verbatim from #3410: a payload not deemed VALID must
report null, and only VALID payloads have the fixture's expected verdict
enforced.

Fixes #3436 for the consume path independently of a fixture refill.

(cherry picked from commit 67a9345)

* fix(tests,test-specs): fix invalid inclusion-list engine fixture generation (#3471)

* fix(tests): clear rlp_modifier when building inclusion-list variants (#3406)

The inclusion-list variant moves the last transaction of the last block
out of the block body and into the inclusion list, then clears the
expectations derived from it: header_verify, expected_gas_used and
expected_block_access_list. rlp_modifier was left in place.

rlp_modifier force-writes header fields onto the built block after the
transition tool has run, and tests compute it from the pre-move
transaction list. The moved transaction therefore keeps contributing to
the header even though it no longer executes in the block, producing
fixtures whose header contradicts their own body.

Co-authored-by: Marc Harvey-Hill <10379486+Marchhill@users.noreply.github.com>

* fix(focil): omit inclusion list result for invalid payloads

* fix(tests): give test_tx_gas_limit a block gas limit a valid block fits in

The test pinned the block gas limit to 21000 with a 21001-gas transaction to
trigger GAS_ALLOWANCE_EXCEEDED. On forks with the EIP-7928 block-access-list
item budget (gas_limit // 2000) a 21000-gas-limit block allows 10 items, which
is below the protocol-level writes of even an empty block, so no block in this
environment can be valid. The test itself never noticed - its block is expected
invalid - but its auto-generated inclusion-list variant moves the failing
transaction into the inclusion list and expects the emptied block to be VALID,
producing a fixture that the spec's own state transition rejects
(tests-focil-devnet@v0.2.0, blockchain_test_engine_inclusion_list, the one
non-blob entry in the 127 broken engine fixtures).

Scale the numbers to 100_000/100_001: the allowance check still fires the same
way, and the item budget (50) now accommodates an empty block, so the
inclusion-list variant fills to a genuinely valid block.

* fix(test-specs): splat the metadata regression block from untyped kwargs

The regression test from #3445 constructs a BuiltBlock through
model_construct with object() sentinels for fields that
get_fixture_engine_new_payload only forwards, which the typechecker rejects
for missing and mistyped named arguments. model_construct skips validation
and the sentinels are never read, so splat them from a dict[str, Any]
instead of passing them as checked named arguments.

* fix(tests): align test_tx_gas_limit with its #3566 counterpart

Byte-identical to the forks/amsterdam hunk so the next focil rebase resolves cleanly; fixture output is unchanged.

---------

Co-authored-by: Marc <Marchhill@users.noreply.github.com>
Co-authored-by: Marc Harvey-Hill <10379486+Marchhill@users.noreply.github.com>
Co-authored-by: chugarchugarr <josephlerma19@gmail.com>
Co-authored-by: Felipe Selmo <fselmo2@gmail.com>
(cherry picked from commit 65f4222)

---------

Co-authored-by: marioevz <marioevz@gmail.com>
Co-authored-by: Ivan Litteri <67517699+ilitteri@users.noreply.github.com>
Co-authored-by: Marc <Marchhill@users.noreply.github.com>
Co-authored-by: Marc Harvey-Hill <10379486+Marchhill@users.noreply.github.com>
Co-authored-by: chugarchugarr <josephlerma19@gmail.com>
@fselmo

fselmo commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

Good for review + merge again! Reconciled the system contract call tests by checking same-value slot replacement in the pre-exec contract test and moving the cross-system-contract interaction test to the post-execution system calls. This ends up working well imo.

@spencer-tb spencer-tb left a comment

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.

LGTM!

@spencer-tb
spencer-tb merged commit a9792ab into ethereum:forks/amsterdam Sep 17, 2026
28 checks passed
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