Eip 8246 remove selfdestruct burn - #2511
Closed
brett-monad wants to merge 3 commits into
Closed
brett-monad wants to merge 3 commits into
brett-monad wants to merge 3 commits into
Conversation
brett-monad
force-pushed
the
eip-8246-remove-selfdestruct-burn
branch
from
August 21, 2026 17:21
e5f79a1 to
7b7af35
Compare
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. The EIP-8024 amsterdam spec-test fixtures run and pass: 56 fixtures (53 EIP-8024 + 3 EIP-7843), with the allowlist in MONAD_NEXT_amsterdam.cmake opening to eip8024_dupn_swapn_exchange on top of the eip7843_slotnum entry the base carries. The tests-monad_amsterdam@v0.4.0 bundle emits an all-zero blockAccessListHash alongside slotNumber in its genesis header, matching the atomic blockAccessListHash/slot_number pair that encode_block_header and decode_block_header require, so the genesis commit succeeds. 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. Measurements and the repo-wide policy this is a piece of are recorded in assert-policy-future-work.md. 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. 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. No spec-test fixtures execute at this commit: the Amsterdam suite is allowlisted to the SLOTNUM and EIP-8024 directories, so nothing this change touches runs. The excluded selfdestruct fixtures fail here as they do at the parent, but not in the same way -- the pinned bundle predates 8246, so on top of the existing logs-bloom failures this commit adds post-state mismatches, where a fixture's postState omits the created-and-destructed account that 8246 now preserves. Allowlisting the 7708 directory therefore needs a bundle generated against a spec that has 8246; without one, those post-state failures read as an 8246 bug rather than as a stale expectation. Unit tests are the whole gate here, so 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>
brett-monad
force-pushed
the
eip-8246-remove-selfdestruct-burn
branch
from
August 21, 2026 18:50
7b7af35 to
4b05bfa
Compare
Contributor
Author
|
Duplicate of #2512, which has the correct base ( |
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
2 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.
No description provided.