test: run the Amsterdam spec suite for the implemented EIP set - #2524
Closed
brett-monad wants to merge 5 commits into
Closed
brett-monad wants to merge 5 commits into
brett-monad wants to merge 5 commits into
Conversation
Add opcode 0x4B (SLOTNUM) feature gated under eip_7843_active(). Bump MONAD_NEXT to MONAD_ETH_AMSTERDAM. SLOTNUM returns the slot number for the current block via evmc_tx_context.block_round in both the interpreter and the x86 compiler. Add all-zero block_access_list_hash block header field; BAL (EIP-7928) is not planned for Monad but proceeds slot_number in RLP encoding. There's a corresponding eip_7928_active() feature flag set to false for Monad revisions, to stub out any future implementation. RLP-encode/decode BlockHeader::slot_number as a trailing optional field after requests_hash and block_access_list_hash. propose_block only populates it when eip_7843_active() is true. Update MONAD_NEXT test-fixture exclusions for SLOTNUM and drop the full exclusion in favor of remaining fixtures. This change will be followed shortly by a PR in monad-bft adding slot_number and the all-zero block_access_list_hash to consensus headers: category-labs/monad-bft#3185 Bump MonadSpecTestFixtures 1.1.1 -> 1.2.0 BumpMonadAmsterdamSpecTestFixtures 0.2.0 -> 0.4.0
Add the DUPN (0xE6), SWAPN (0xE7) and EXCHANGE (0xE8) stack-manipulation opcodes to both execution engines (interpreter and x86 JIT), backward-compatible and active from MONAD_NEXT. The opcode and instruction tables gate on MonadTraits::eip_8024_active() via the when()/avail() availability predicates. The operand-dependent stack effect (a function of the decoded immediate) is computed by a shared eip8024_stack_effect helper, consumed by the basic-block scanner (decode_eip8024) and the evm-as validator; the interpreter charges gas and validates via a dynamic check_requirements_eip8024. DUPN/SWAPN lower to the existing dup()/swap() emitter paths; the new emitter surface is Emitter::exchange and the Stack::exchange virtual-stack bookkeeping it drives, with Stack::swap reimplemented in terms of Stack::exchange. The immediate is encoded so it can never be a JUMPDEST or PUSH byte, preserving legacy JUMPDEST scanning. check_requirements_eip8024 validates the immediate before deducting min_gas: a disallowed encoding makes the instruction invalid irrespective of the gas available, so it exits Error rather than OutOfGas. That matches the interpreter's handling of every other invalid instruction -- `invalid` charges nothing -- and the compiler, where decode_eip8024 turns a disallowed immediate into Terminator::InvalidInstruction at analysis time, so the opcode never contributes its gas to the block. eip8024_stack_effect dispatches on DUPN and SWAPN and then falls through to the pair-decoding EXCHANGE path, so it asserts opcode == EXCHANGE there rather than leaving the shared helper's contract implicit. The preconditions these paths rely on use MONAD_ASSERT rather than MONAD_DEBUG_ASSERT, so they survive NDEBUG. The nine in opcodes.hpp are the five EIP-8024 encode/decode guards and the four get_*_opcode_index guards they were modelled on. Where the caller tests the same predicate immediately before calling, they are free: check_requirements_eip8024 evaluates `disallowed` on the line above, and Context::exit is [[noreturn]], so the optimizer proves the condition and deletes it -- per-symbol instruction counts in execute.cpp and basic_blocks.cpp are unchanged in both release builds. That is the hot path, once per instruction executed. The x86 backend's DupN/SwapN/Exchange cases decode without a local test, because decode_eip8024 filtered disallowed immediates earlier during analysis, so those guards survive: x86.cpp grows from 38077 to 38461 instructions at -O3, across 63 symbols. Immaterial, since it runs once per instruction compiled and is amortised over cached compiled code. What this closes is a silent failure rather than a loud one: a disallowed immediate in [91,127] decodes to a plausible-looking index, and at n == 0 DUPN evaluates *(stack_top + 1), reading the stale slot above the stack top -- a wrong result with no fault, the worst class of bug for a consensus VM. The stack_indices_ erase/insert asserts in Stack::pop, Stack::push and Stack::exchange are a different trade: nothing upstream constrains what a set erase or insert returns, so the optimizer cannot fold them away. exchange therefore pays a compare-and-branch, judged worth it because it runs once per SWAP/EXCHANGE compiled rather than once executed, and because a violated index invariant means the emitter has lost track of which element owns which stack index and will emit code reading the wrong slot -- a silently miscompiled contract is worse than an abort. pop/push cost nothing under gcc, which uses the fact to delete the rem != 1 / !ins handling and outline the failure path. eip8024_decode_single returns uint8_t, matching eip8024_decode_pair and sitting next to the mask that makes the narrowing value-preserving; the backend's static_casts before Emitter::dup/swap and the interpreter's widening both disappear. Verified codegen-neutral by per-function instruction counts on execute.cpp and x86.cpp under gcc-15 and clang-19. Eip8024Operands keeps ptrdiff_t fields, since its values are consumed purely as offsets from stack_top. The opcode_table entries for 0xE6-0xE8 carry fictional absolute stack values -- only the net delta survives, and it is that delta the table states correctly. This is safe because no consumer reads the absolutes: scan_from returns through decode_eip8024 before the generic path, Block::stack_deltas reads the per-instruction values decode_eip8024 computed, the evm-as validator derives them from the operand, and check_requirements is never instantiated for these opcodes. Relatedly, num_args is documented by its actual property -- only PUSHN immediates are consumed through it -- since the JUMPDEST scan in intercode.cpp and show_opcodes in parser.cpp both skip immediates by opcode range instead. show_opcodes names opcodes from MONAD_ETH_MAX_REVISION rather than MONAD_ETH_LATEST_STABLE_REVISION -- a disassembler should label bytes that only became instructions in a not-yet-stable fork -- so the EIP-8024 immediate byte is consumed instead of being misread as a following instruction. This also names CLZ and SLOTNUM, which were previously printing as UNKNOWN. find_opcode and compile_tokens stay on the latest stable revision. This commit is based on main, whose Amsterdam exclusion list already covers */eip8024_dupn_swapn_exchange/*, so EIP-8024's own fixtures do not run yet. Enabling them needs a bundle regenerated against a spec that has 8024, and is deferred to a follow-up. One exclusion is added here, and it is not under the EIP's own directory so the existing wildcard does not reach it: frontier/opcodes/all_opcodes/all_opcodes.json asserts the behaviour of every opcode including 0xE6/0xE7/0xE8, which it expects to be undefined -- implementing them at Amsterdam invalidates it. It comes back out with the same regenerated bundle. Measured with it in place: 552 fixtures run, 548 pass, 4 skip, none fail. Immediate validity goes through a single eip8024_immediate_valid(opcode, imm) rather than each call site choosing between the pair and single rule itself. Six sites made that choice independently, and EXCHANGE rejects a wider range than the single form, so a site reaching for the wrong rule would have silently misjudged a band of immediates -- and in a release build the decoder's assert would not catch it. The two range predicates are now referenced only from within opcodes.hpp; callers that already hold a valid immediate, such as the x86 emitter, still decode directly. Review follow-ups folded in: - Refuse the EIP-8024 opcodes in EvmBuilder::ins(). ins() assembles nullary opcodes, and the table entry for DUPN is not unknown once 8024 is active, so a bare one fell through to a single-byte PlainI and swallowed the next instruction's first byte as its immediate. It now yields InvalidI, matching the existing unknown-opcode branch, with a test pinning the bytecode. - Print the operand in IR dumps. DupN/SwapN/Exchange fell through to the bare-opcode branch of the Instruction formatter, so DUPN 17 and DUPN 235 were indistinguishable. index() holds the raw encoded immediate for these, so the new branch decodes it rather than printing it -- and emits the same mnemonic evm-as does. - Keep the MONAD_DEBUG_ASSERT promotions, and say why, since a reviewer read them as unrelated scope. They answer an earlier review comment on this branch asking that preconditions be enforced in release rather than only in debug, and each was measured before being taken: the opcodes.hpp preconditions are free -- the optimizer deletes them because every call site tests the same predicate immediately before calling and Context::exit is [[noreturn]] -- while Stack::exchange costs +41 gcc / +37 clang and Stack::pop / Stack::push are -36 gcc / +18 clang. The Stack ones were taken despite the cost because they guard the virtual stack's own index bookkeeping, which no caller checks: corrupt Stack state means the emitter has lost track of which element owns which index, and a silently miscompiled contract is worse than an abort. The cost is per-compilation rather than per-execution -- virtual_stack.hpp is reached only from the x86 emitter. Coverage for blocks entered by a jump: Every existing EIP-8024 execution test is a single straight-line block that builds its own stack, so the block's min_delta never drops far and the compiler's block_prologue stack-size check only ever runs against a stack the block itself created. These four enter the block by a real JUMP with a deep live stack instead, which puts the operands in the block's negative stack indices -- loaded from the runtime stack -- and drives block_prologue's `cmp size_mem, -min_delta; jb error` at a min_delta down to -236. DeepDupnInJumpedToBlock and DeepSwapnInJumpedToBlock use DUPN/SWAPN 235, the deepest single-operand reach; DeepExchangeInJumpedToBlock uses EXCHANGE 1,29, the deepest pair. DeepDupnInJumpedToBlockUnderflows enters the same block with too few items, so the compiler has to reject it in block_prologue against the runtime stack size rather than through the per-instruction check the interpreter uses. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n Amsterdam EIP-8246 removes the two paths by which SELFDESTRUCT still destroys ETH. It is a prerequisite of EIP-7708, whose current text requires it and no longer specifies a burn, so this completes 7708 rather than adopting an extra EIP. That is an ordering constraint rather than a code one: this commit is based on main and independent of the SLOTNUM, EIP-8024 and EIP-7708 branches, but it has to merge before 7708 or that EIP would claim complete native-value logging while two burn paths went unlogged. Rule 1: a same-transaction account destructing to itself keeps its balance. The pre-8246 guard admits the debit with no matching credit when the beneficiary is the account itself and the account is the current incarnation; that arm is the burn, so at 8246-active revisions the guard collapses to a plain test for value having somewhere else to go. Rule 2: at finalization a destructed account is preserved rather than deleted when it holds a balance -- nonce reset, code and storage cleared, balance untouched. A zero balance still deletes, which is EIP-161 unchanged. Rule 2 also covers the EIP's second burn, value sent to an account already marked for selfdestruction, because it keys on the balance rather than on who the beneficiary was. Gated on a new eip_8246_active() predicate: pre-Amsterdam revisions must keep burning. Implementation notes worth carrying forward. The preserved account is mutated in place so its incarnation survives. That is the storage-generation key the commit builders compare against the pre-block account to decide whether the old storage subtree is rebuilt, and assigning a fresh Account would default it -- which is itself a legal incarnation, not a safe sentinel. Storage is cleared by zeroing the original map's key set, which covers the current map's because the current map's keys are always a subset of it. In consensus the two sets are equal; they differ only when something stamps the current incarnation onto a pre-existing contract, which set_to_state_incarnation does for the RPC full-state-override path, so iterating the original map is what keeps eth_call correct. Three invariants are recorded at the head of destruct_suicides because nothing else states them. destruct_touched_dead is an untemplated second deletion pass, so it cannot be made revision-aware, and it spares a preserved account only because the balance test and is_dead are the same EIP-161 emptiness test. BlockState::can_merge runs before execute_final, which is what stops relaxed merge from zeroing the balance and committing an empty account. The reserve-balance check runs at depth zero before finalization, so rule 2 is invisible to it and only rule 1 is, which removes a debit. Testing. Nine Amsterdam fixtures encode the pre-8246 deletion and are excluded here: four under cancun/eip6780_selfdestruct, two tangerine_whistle, and one each in frontier/create, paris/security and monad_nine/mip4_checkreservebalance. They fail on a postState that omits the balance-only account 8246 now preserves. Measured: 553 fixtures run, 540 pass, 4 skip, and reverting the nine entries reproduces exactly those nine failures. They come back out when a bundle generated against a spec that really implements 8246 is pinned -- upstream carries an EIP8246 fork class and a selfdestruct_no_burn test directory, but the class is a stub and no fixtures are generated from it, so no such bundle exists yet. The mip4_checkreservebalance entry is coarser than the rest: gtest filters per file, and that file's cases are mostly selfdestruct_False ones 8246 does not touch, so they are suppressed as collateral rather than because they encode pre-8246 behaviour. Because those nine are excluded, the Amsterdam job cannot exercise 8246's own behaviour and the unit tests are the real gate. Accordingly, every assertion was checked by deleting the code it covers and confirming the test fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n Amsterdam Emit a standardised log for every native ETH movement, so indexers can track native value the way they already track ERC-20 transfers. `Transfer(address,address,uint256)` on ordinary value transfers, at the four sites Monad already wired: top-level tx transfer, value-carrying CALL, SELFDESTRUCT, and CREATE/CREATE2. That is the only log the EIP defines. There is deliberately no Burn log. An earlier draft of 7708 also specified `Burn(address,uint256)` for ETH destroyed by SELFDESTRUCT, and this commit used to implement it; the draft then took EIP-8246 as a prerequisite and dropped the log, because after 8246 there is no burn left to report. EIP-8246 is a separate branch based on main rather than a commit beneath this one, but it merges first, so by the time 7708 is live the burn never happens and the log would have no subject. geth and erigon are in the same configuration. Consensus logs use SYSTEM_ADDRESS. eth_simulate's traceTransfers keeps the ERC-7528 native-token address and is emitted alongside the consensus log rather than replacing it, second, so discarding it leaves the sequence a real block produces. Also in this commit: - Leave the spec-test configuration alone. This commit is based on the SLOTNUM branch, whose exclusion list already covers */eip7708_eth_transfer_logs/*, so 7708's own fixtures do not run yet and the Amsterdam job passes unchanged. Enabling them needs a bundle regenerated with 7708 active and is deferred to a follow-up once the whole set has landed. The base is a fixture constraint rather than a code dependency: nothing here needs anything from SLOTNUM, and this commit builds and unit-tests cleanly on main. But main pins the Amsterdam bundle at v0.2.0, which predates 7708, so every fixture that moves ETH expects no Transfer log and 110 of them break. The SLOTNUM branch carries the bump to v0.5.0, whose fixtures were generated with 7708 active. - Make the simulate_v1 trace expectations revision-aware. Four tests asserted log counts and contents that predate 7708; two stopped compiling once the rule was active and two failed at runtime. They had gone unnoticed because MONAD_NEXT only rejoined the typed-test matrices with the fork bump in the SLOTNUM commit beneath this one, so they had never run at a revision where 7708 is active. The doubling is now expressed by helpers derived from the emitter rather than from observed output. - Run the new tests over an explicit two-element type list naming both revisions where 7708 is active: MonadRevisionConstant<MONAD_NEXT> and EvmRevisionConstant<MONAD_ETH_AMSTERDAM>. That covers the configuration that ships as well as the plain-EVM one, and names the revisions rather than deriving them from LATEST_SUPPORTED_EVM_FORK, so the suite does not depend on that constant having been advanced to Amsterdam. A TypesSince form would be more durable once it has been, and is worth revisiting then. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The MONAD_NEXT Amsterdam job has been switched off or narrowed on every branch that implements one of these EIPs, because each one moves ahead of the pinned fixture bundle in a different way. With SLOTNUM, EIP-8024, EIP-8246 and EIP-7708 all present, the suite can run: 641 fixtures, 633 pass, 8 skip, none fail. Retire the MonadEip7981SpecTestFixtures bundle. It is pinned at tests-monad_eip7981@v0.1.0, the only version ever published, and it predates both EIP-7708 and the SLOTNUM header field -- so all 555 of its fixtures fail, its own 17 EIP-7981 ones included: they expect no Transfer log in the bloom, and their genesis headers carry no slotNumber, which the loader hashes. Excluding them by name would be the same act written out 555 times. The whole ExternalProject goes rather than just its path in the job, since a declared bundle still downloads 195MB on every configure whether or not anything reads it. EIP-7981 coverage is expected to return through the Amsterdam bundle, which is where the rest of the fork's fixtures already live; if upstream keeps it separate instead, the ExternalProject comes back with a regenerated pin. Exclude 17 fixtures from tests-monad_amsterdam@v0.5.0, in two groups with different causes. Eight are gas: v0.5.0 was generated without EIP-7981, which main implements, so its intrinsic-gas budgets predate the access-list cost increase and the fixtures fail with "intrinsic gas greater than limit". Nine are selfdestruct: v0.5.0 implements EIP-7708 but not EIP-8246, so it expects a Transfer log where rule 1 means nothing moves, an account deleted that rule 2 preserves, and a balance burned that rule 2 keeps. Both groups come out when upstream publishes a bundle generated against a spec with EIP-7981, EIP-8246 and EIP-7708 all active. No such release exists: v0.5.0 has 7708, SLOTNUM and 8024 but not 7981, and eip7981@v0.1.0 has 7981 but predates the others. When it lands the whole change here is two deletions from the exclusion list -- but note that EIP-8246 has never been exercised by any fixture, so those nine are the first real test of it rather than a confirmation. The 8 skips are the for_monad_tentomonad_nextattime15k fork-transition fixtures, whose network has no revision_map entry -- including EIP-7708's own two, which pin when emission starts. That harness gap predates this change and a new bundle will not close it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
brett-monad
force-pushed
the
amsterdam-spec-tests
branch
2 times, most recently
from
September 8, 2026 21:27
91905ec to
0282223
Compare
Contributor
|
Superseded: #2537 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft, and deliberately not mergeable as-is. It also does not yet include EIP-7997 (#2499), the fifth Amsterdam EIP, so this is not the complete fork set.
Draft note: The four EIP commits below it are cherry-picks of #2439, #2407, #2512 and #2469, so this branch will conflict with them on merge. It exists to prove the end state and to hold the spec-test work while those four land; once they have, it gets rebuilt by rebasing onto
mainand reduces to its own top commit.Every branch implementing an Amsterdam EIP currently has the
MONAD_NEXTAmsterdam spec job switched off or narrowed, because each one moves ahead of the pinned fixture bundle in a different way. #2439 and #2469 blanket it via theWILL_FAILguard; #2407 excludes one fixture; #2512 excludes nine. With all four present the suite can actually run.Result: 641 fixtures run, 633 pass, 8 skip, none fail.
What it takes
Retire the
MonadEip7981SpecTestFixturesbundle. Pinned attests-monad_eip7981@v0.1.0, the only version ever published, and it predates both EIP-7708 and the SLOTNUM header field — so all 555 of its fixtures fail, including its own 17 EIP-7981 ones: they expect noTransferlog in the bloom, and their genesis headers carry noslotNumber, which the loader hashes. The wholeExternalProjectgoes rather than just its path in the job, since a declared bundle still downloads 195 MB on every configure whether anything reads it or not. EIP-7981 coverage is expected to return through the Amsterdam bundle; if upstream keeps it separate, theExternalProjectcomes back with a regenerated pin.Exclude 17 fixtures from
tests-monad_amsterdam@v0.5.0, in two groups with different causes:mainimplements, so its intrinsic-gas budgets predate the access-list cost increase and these fail withintrinsic gas greater than limit.Transferlog where rule 1 means nothing moves, an account deleted that rule 2 preserves, and a balance burned that rule 2 keeps. Each fixture reports the bloom mismatch first and the post-state size and balance mismatches underneath — reading only the first line makes this look like a 7708 emission bug, which it is not.When the exclusions come out
Both groups clear when upstream publishes a bundle generated against a spec with EIP-7981, EIP-8246 and EIP-7708 all active. No such release exists today: v0.5.0 has 7708, SLOTNUM and 8024 but not 7981, and
eip7981@v0.1.0has 7981 but predates the others. At that point the change here is two deletions from the exclusion list — no bundle to re-add, no path to re-wire.One thing worth not assuming away: EIP-8246 has never been exercised by any spec fixture. It has no upstream coverage at all — the
EIP8246fork class is a stub and itsselfdestruct_no_burntest directory generates nothing. So those nine are the first real test of that implementation rather than a confirmation of it, and a residual failure there should be read as information about our code before it is read as a stale fixture.Fixtures that pass without an implementation
amsterdam/eip7954_increase_max_contract_sizeruns green even though there is noeip_7954_activepredicate and no implementation anywhere in the tree. That iscorrect, not an accident: Monad's existing max code size already subsumes the
limit EIP-7954 raises Ethereum's to, so there is nothing to gate.
Not covered
The 8 skips are the
for_monad_tentomonad_nextattime15kfork-transition fixtures, whose network has norevision_map.hppentry — including EIP-7708's own two, which pin when emission starts. That harness gap predates this change and a new bundle will not close it.