feat(consume): check inclusionListSatisfied semantics and probe engine_getInclusionListV1 - #3410
spencer-tb wants to merge 29 commits into
Conversation
…um#2643) * feat: continue focil implementation * feat: tests added
* feat: Bogota fork + EIP-7805 engine_newPayloadV6 bump The first tests-focil release fixtures were filling FOCIL tests with `newPayloadVersion: 5` because no EIP class declared the version bump and there was no fork class to host EIP-7805. As a result `consume engine` appended `inclusionListTransactions` as a 5th positional arg to V5, which isn't a wire format defined by any Engine API spec — all clients reject it. This adds: - EIP7805 mixin under eips/bogota/ with engine_new_payload_version_bump and engine_forkchoice_updated_version_bump - Bogota fork class (Amsterdam + EIP7805), reusing Amsterdam's t8n - Switches FOCIL tests' valid_from marker to Bogota After refilling, fixtures now use engine_newPayloadV6 / forkchoiceUpdatedV5, matching the canonical Engine API PR (ethereum/execution-apis#626). * Address review: BogotaEIPs auto-discovery + feature.yaml --from=Bogota - Add BogotaEIPs sentinel that auto-discovers eip_*.py modules under eips/bogota/, mirroring AmsterdamEIPs. Bogota fork class now inherits BogotaEIPs instead of referencing eips.EIP7805 explicitly. - Switch focil entry in .github/configs/feature.yaml from --until=Amsterdam to --from=Bogota so the FOCIL job fills against the Bogota fork rather than ending at Amsterdam. --------- Co-authored-by: Marc Harvey-Hill <10379486+Marchhill@users.noreply.github.com>
…msterdam (ethereum#3370) * fix(tests,tools): make the EIP-7805 FOCIL tests fill on the current Amsterdam 21 of the 24 FOCIL tests do not fill on this branch. Three independent causes, none of them in the tests' assertions. t8n passed decoded transactions where raw ones are expected. check_inclusion_list_transactions takes Tuple[LegacyTransaction | Bytes, ...] and calls get_transaction_hash, which asserts isinstance(tx, (LegacyTransaction, Bytes)); the caller passed convert_transaction(...), which returns a decoded fork transaction, so every typed-transaction case died on AssertionError. Both tuples now go through encode_transaction. The scenarios left no gas budget for the block's own access list. They size block_gas_limit to their transactions alone, but the EIP-7928 budget is block_gas_limit // GAS_BLOCK_ACCESS_LIST_ITEM, and the system-contract predeploys touched every block plus senders, recipients and coinbase come to about 30 items against a limit of about 22. Raising the limit alone would change what the scenarios assert, because the gas a pending inclusion-list transaction is allowed comes from block_gas_limit minus the gas the included transactions use. So build_block raises the limit and spends exactly the same amount on ballast transactions, leaving remaining_gas -- and therefore every "fits" and "does not fit" boundary -- unchanged. The ballast is a whole number of empty transfers so the amount is always representable on the fork's calldata-cost lattice. One funding transfer predated value-carrying transactions costing extra intrinsic gas. test_unsatisfied_when_block_tx_funds_pending_il_sender sizes and funds alice's transfer with calc(), but it carries value, so it needs calc(sends_value=True) and was failing INTRINSIC_GAS_BELOW_FLOOR_GAS_COST. Assertions are untouched: of the changed lines in test_focil.py, none touch expected_status, pytest.param, assertions, sender balances, nonces or inclusion-list composition. Scenario ids are unchanged and the reference implementation validates all 24 expected outcomes during fill. * Apply suggestions from code review Co-authored-by: Mario Vega <marioevz@gmail.com> * fix: lint --------- Co-authored-by: Mario Vega <marioevz@gmail.com>
… existing tests (ethereum#3373) * feat(test-specs): Add inclusion list satisfied verification to block * fix(test-specs): fix framework on missing inclusion list * refactor(tests): refactor existing focil tests * nit(test-specs): Unnecessary comment * Apply suggestions from code review Co-authored-by: spencer <spencer.tb@ethereum.org> Co-authored-by: Mario Vega <marioevz@gmail.com> --------- Co-authored-by: spencer <spencer.tb@ethereum.org>
…inclusion-list fixtures
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## eips/amsterdam/eip-7805 #3410 +/- ##
==========================================================
Coverage ? 81.95%
==========================================================
Files ? 624
Lines ? 37099
Branches ? 3397
==========================================================
Hits ? 30405
Misses ? 6274
Partials ? 420
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:
|
|
Reminder to check whether we have in this branch all of the commits mentioned in this comment. |
|
Some ideas for deterministic testing:
Note that the second and third can return either True or False for inclusionListSatisfied depending on other IL transactions. I'd also want to mention that this can be expanded to not IL-eligible Frame transactions. I'd expect this be useful for FOCIL and Frames testing. |
…ds (#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.
974e582 to
204067d
Compare
…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>
5b33750 to
f791277
Compare
…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>
Description
Follow up to #3401. Adds
inclusionListSatisfiedchecks to the engine and enginex simulators and probesengine_getInclusionListV1after importing each inclusion list fixture.Related Issues or PRs
Follow up to #3401.
Checklist
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