vm: implement EIP-8024 (backward-compatible DUPN/SWAPN/EXCHANGE) - #2407
vm: implement EIP-8024 (backward-compatible DUPN/SWAPN/EXCHANGE)#2407brett-monad wants to merge 1 commit into
Conversation
|
@claude review this PR |
There was a problem hiding this comment.
Pull request overview
Implements draft EIP-8024 stack-manipulation opcodes (DUPN/SWAPN/EXCHANGE) across the interpreter, x86 JIT compiler pipeline, and the evm-as fluent assembler, gated behind eip_8024_active() and staged on the MONAD_ETH_EXPERIMENTAL EVM revision.
Changes:
- Adds EIP-8024 opcode definitions plus immediate encode/decode + disallowed-domain logic to preserve legacy JUMPDEST scanning behavior.
- Wires EIP-8024 execution into both engines (interpreter dispatch + handlers; compiler decode + x86 emitter/virtual stack support) behind a feature predicate.
- Extends
evm-aswith an EXPERIMENTAL builder factory, EIP-8024 instruction variant support, validation logic, and unit tests.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/vm/unit/evm-as_tests.cpp | Adds evm-as emission and gating tests for the new EIP-8024 builder helpers. |
| test/vm/unit/eip8024_tests.cpp | New EXPERIMENTAL-only interpreter-vs-compiler execution tests plus codec/jumpdest/dormancy coverage. |
| test/vm/unit/CMakeLists.txt | Registers the new EIP-8024 unit test source under MONAD_COMPILER_TESTING. |
| category/vm/utils/evm-as/validator.hpp | Adds dynamic stack-effect validation for EIP-8024 instructions. |
| category/vm/utils/evm-as/resolver.hpp | Teaches the resolver that EIP-8024 instructions are 2 bytes (opcode + imm). |
| category/vm/utils/evm-as/instruction.hpp | Introduces the Eip8024I instruction variant and helpers to identify/access it. |
| category/vm/utils/evm-as/compiler.hpp | Emits EIP-8024 instructions to bytecode and prints EIP-8024 mnemonics in mcompile(). |
| category/vm/utils/evm-as/builder.hpp | Adds fluent builder methods dupn/swapn/exchange with operand validation and revision gating. |
| category/vm/utils/evm-as.hpp | Adds evm_as::experimental() builder factory for MONAD_ETH_EXPERIMENTAL. |
| category/vm/runtime/storage_costs.hpp | Defines storage cost table inheritance for MONAD_ETH_EXPERIMENTAL. |
| category/vm/interpreter/intercode.cpp | Documents why jumpdest scanning intentionally does not skip EIP-8024 immediates. |
| category/vm/interpreter/instructions_fwd.hpp | Declares interpreter instruction entrypoints for dupn/swapn/exchange. |
| category/vm/interpreter/instruction_table.hpp | Adds interpreter gating + handlers for EIP-8024, and a 2-byte-step MONAD_VM_NEXT_IMM macro. |
| category/vm/evm/traits.hpp | Introduces eip_8024_active() (true for EXPERIMENTAL EvmTraits; false for MonadTraits). |
| category/vm/evm/switch_traits.hpp | Extends the EVM-traits switch to handle MONAD_ETH_EXPERIMENTAL. |
| category/vm/evm/opcodes.hpp | Adds EIP-8024 opcodes to EvmOpCode, opcode-table wiring for EXPERIMENTAL, and codec helpers. |
| category/vm/evm/explicit_traits.hpp | Adds explicit template instantiations for EvmTraits<MONAD_ETH_EXPERIMENTAL>. |
| category/vm/compiler/ir/x86/virtual_stack.hpp | Declares a generalized Stack::exchange() to support EXCHANGE. |
| category/vm/compiler/ir/x86/virtual_stack.cpp | Implements Stack::exchange() with alias-safe bookkeeping. |
| category/vm/compiler/ir/x86/emitter.hpp | Declares Emitter::exchange() for codegen. |
| category/vm/compiler/ir/x86/emitter.cpp | Implements Emitter::exchange() using the virtual stack’s generalized swap. |
| category/vm/compiler/ir/x86.cpp | Adds codegen cases to decode EIP-8024 immediates and emit dup/swap/exchange operations. |
| category/vm/compiler/ir/instruction.hpp | Adds IR opcode tags and stringification for the EIP-8024 operations. |
| category/vm/compiler/ir/basic_blocks.hpp | Adds compiler-side decode/validation for EIP-8024 immediates and dynamic stack effects. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Reviewed the EIP-8024 wiring. The implementation looks sound; noteworthy checks:
- The immediate codec's disallowed ranges ([91,127] for single, [82,127] for pair) are correctly stricter than the raw JUMPDEST/PUSH-avoidance minimum, so encoded bytes fall in [0,90] ∪ [128,255] and never overlap those opcodes. The exhaustive round-trip and decode-domain sweep tests confirm this.
- The interpreter handlers charge gas first, validate the disallowed range, then bound the dynamic operand against the current stack size before touching memory — including the tight
stack_size < n(DUPN),< n+1(SWAPN), and< m+1(EXCHANGE) checks that keep*(stack_top - k)in bounds even at the deepest reach. - Not special-casing DUPN/SWAPN/EXCHANGE in
find_jumpdestsis correct given the encoding: valid immediates can never be0x5B, and theEip8024Jumpdest.DisallowedImmediateByteRemainsAJumpdesttest guards the case where a disallowed0x5Bremains a live jumpdest. Stack::exchangemirrorsStack::swap's erase-before-insert bookkeeping, which theExchangeAliasedDuplicatedValuetest exercises via a DUP+EXCHANGE pattern.- Staging on the
EXPERIMENTALslot (noMonadTraitsmaps to it,to_evmc_revisionasserts<= OSAKA, chain revisions never return it) correctly keeps the draft opcodes inert outside the explicit-EvmTraits tests.
One P3 finding inline about a stale test coverage roster now that EvmTraits<MONAD_ETH_EXPERIMENTAL> is a real instantiation rather than a sentinel.
Verdict: CORRECT
🤖 Generated with Claude Code
8012824 to
73a2379
Compare
|
@claude review this PR |
andreaslyn
left a comment
There was a problem hiding this comment.
I suppose we will want some emitter unit tests for the new instructions.
73a2379 to
a7e7b5c
Compare
a7e7b5c to
d776dc7
Compare
d776dc7 to
a6a28ef
Compare
Review summaryReviewed the EIP-8024 implementation across the interpreter, x86 JIT, evm-as assembler, and tests. The design is coherent — the Bounds checks look right end-to-end:
Test coverage is thorough: codec round-trips, full-domain decode range checks, jumpdest analysis on both engines, full DUPN/SWAPN operand sweeps, the exhaustive EXCHANGE Only one non-blocking style nit — see the inline comment on Verdict: CORRECT 🤖 Generated with Claude Code |
a6a28ef to
83a0de0
Compare
941953f to
3ac6d81
Compare
66461b1 to
cc9f672
Compare
3ac6d81 to
6c9757c
Compare
cc9f672 to
e69ad7c
Compare
6c9757c to
2f2cced
Compare
21f5f35 to
f987246
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.
Suppressed comments (3)
test/vm/unit/eip8024_tests.cpp:214
- [P2] Avoid constructing a vector from null EVMC output pointers. Error results explicitly use
output_data == nullptrwithoutput_size == 0(category/vm/runtime/context.cpp:158-163), and this helper reaches that path in every failure test; evaluatingoutput_data + 0and using the null pointer range is undefined behavior. Build an empty vector when the size is zero.
std::vector<uint8_t>(
result_.output_data,
result_.output_data + result_.output_size)};
category/vm/interpreter/intercode.cpp:65
- [P3] Qualify this as a valid immediate. A disallowed immediate can be
0x5B—the newDisallowedImmediateByteRemainsAJumpdesttest relies on exactly that—so the current absolute statement is false. The legacy scan remains correct because valid immediates exclude JUMPDEST while disallowed bytes remain standalone instructions.
// The EIP-8024 opcodes (DUPN/SWAPN/EXCHANGE) are intentionally NOT
// special-cased: their immediate byte can never be 0x5B (JUMPDEST), so
// this pre-8024 scan already yields the correct valid-JUMPDEST set.
category/vm/utils/parser.cpp:217
- [P2] Render a disallowed EIP-8024 encoding as INVALID. The code leaves the immediate unconsumed, but it has already printed the opcode-table name (
DUPN,SWAPN, orEXCHANGE) on line 204, contradicting both the execution semantics and this comment. Check validity before emitting the name, printINVALIDfor the opcode byte, then let the next iteration disassemble the immediate separately.
else if (is_eip8024_opcode(c) && i + 1 < opcodes.size()) {
// Consume the immediate like PUSH data, but only for an allowed
// encoding: a disallowed immediate makes the opcode INVALID and
// leaves the byte a separate instruction, possibly a JUMPDEST.
// Not revision-gated; 0xE6-0xE8 are unassigned earlier.
auto const imm = opcodes[i + 1];
if (eip8024_immediate_valid(c, imm)) {
2f2cced to
c268d52
Compare
f987246 to
5493881
Compare
c268d52 to
f6c4c7d
Compare
5493881 to
ea14054
Compare
f6c4c7d to
5b6157a
Compare
ea14054 to
c357443
Compare
1ed9e29 to
8a42a59
Compare
c357443 to
3832d8f
Compare
3832d8f to
d3feea3
Compare
dhil
left a comment
There was a problem hiding this comment.
One thing that seems to be missing from this patch is an extension to the microbenchmark suite with the new opcodes (or well atleast confirming that it isn't broken in some way).
acbdae8 to
24b9149
Compare
d3feea3 to
207d60e
Compare
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>
Implement EIP-8024: backward-compatible DUPN/SWAPN/EXCHANGE
EIP-8024 adds three stack-manipulation opcodes that take an immediate operand selecting which stack item(s) to act on, lifting the fixed-depth limit of the legacy DUP1–16 / SWAP1–16 family:
It is backward-compatible because the immediate byte is encoded so it can never be JUMPDEST (0x5B) or a PUSH byte (0x60–0x7F): legacy jumpdest analysis is unaffected, and any disallowed immediate behaves as INVALID.
This PR implements all three opcodes across both execution engines (interpreter and x86 JIT) and the evm-as assembler, with unit tests. EIP-8024 is still a draft under peer review, so it is gated behind an eip_8024_active() predicate and staged on the unshipped AMSTERDAM revision — no production traits map to AMSTERDAM, so the feature is inert on-chain and is exercised only by dedicated unit tests.
Spec tests
One exclusion, in
exclude/MONAD_NEXT_amsterdam.cmake:frontier/opcodes/all_opcodes/all_opcodes.json. That fixture asserts everyopcode's behaviour including
0xE6/0xE7/0xE8, which it expects to beundefined — so implementing them at Amsterdam invalidates it. Unlike the
existing
*/eip8024_dupn_swapn_exchange/*wildcard it is not under this EIP'sown directory, so it needs naming separately. It comes back out when a bundle
generated against a spec that has 8024 is pinned.
Measured with it in place: 552 fixtures run, 548 pass, 4 skip, none fail.
This PR is based on
mainand independent of #2439, #2512 and #2469.