execution,vm: implement EIP-8246 (remove SELFDESTRUCT burn), active on Amsterdam - #2512
execution,vm: implement EIP-8246 (remove SELFDESTRUCT burn), active on Amsterdam#2512brett-monad wants to merge 1 commit into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the EIP-8246 implementation covering State::selfdestruct, State::destruct_suicides, the trait flag, the state-diff tracer comment, and the coinbase-award comment in execute_transaction.cpp.
The selfdestruct reshuffle is correct: the new eip_8246_active arm collapses to a no-op when address == beneficiary (no burn) and to a transfer otherwise, and the pre-Cancun and Cancun-only arms are unchanged. The destruct_suicides refactor preserves prior semantics for < Cancun (unconditional reset), for post-Cancun with mismatched incarnation (skip), and for post-Cancun with matching incarnation under !eip_8246_active (reset). The new preservation path only fires when the account was created this tx, has non-zero balance, and mutates in place so that the incarnation drives the reincarnation flag through commit — I verified Account::operator== includes incarnation and that create_contract stamps it. The storage-zeroing iterates the original map, which is the correct superset given set_storage/get_storage both populate the original map before or in place of current. MONAD_ASSERT(orig != original_.end()) holds because the only path that inserts into current_ is current_account_state, which first goes through original_account_state. The destruct_touched_dead interaction is sound: a preserved account has balance > 0, so is_dead is false and the untemplated pass spares it. can_merge/relaxed-merge ordering (before execute_final) leaves the preservation unaffected. The tests are thorough — the preserved-storage, credit-after-destruct, touched-dead, merge-through-trie, state-root-parity, and same-block-recreation cases each pin a distinct invariant.
Verdict: CORRECT
🤖 Generated with Claude Code
c357443 to
3832d8f
Compare
4b05bfa to
f927f4e
Compare
There was a problem hiding this comment.
Pull request overview
Implements EIP-8246, removing SELFDESTRUCT balance burns and preserving funded accounts from Amsterdam onward.
Changes:
- Adds Amsterdam-gated EIP-8246 support.
- Updates transfer and finalization behavior.
- Expands state, storage, merge, and tracing tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Summary |
|---|---|
category/vm/evm/traits.hpp |
Adds the EIP-8246 activation predicate. |
category/execution/ethereum/trace/state_tracer.cpp |
Documents preserved-account tracing behavior. |
category/execution/ethereum/test/test_call_trace.cpp |
Verifies SELFDESTRUCT trace values. |
category/execution/ethereum/state3/state.cpp |
Implements non-burning and account-preservation logic. |
category/execution/ethereum/state2/test/test_state.cpp |
Tests preservation, deletion, storage clearing, and commits. |
category/execution/ethereum/execute_transaction.cpp |
Documents finalization ordering. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
f927f4e to
b89479a
Compare
3832d8f to
d3feea3
Compare
ff1dfbe to
e1b9d1a
Compare
d3feea3 to
207d60e
Compare
584bd9e to
ca8f8dc
Compare
ca8f8dc to
d002b44
Compare
…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>
d002b44 to
e2cf84d
Compare
Removes the two paths by which
SELFDESTRUCTstill destroys ETH. This is a prerequisite of EIP-7708 rather than an adjacent EIP — 7708's current text listsrequires: 8246and no longer specifies a burn — which is what lets #2469 stop introducing aBurnlog at all. That is an ordering constraint, not a code one: this PR is based onmainand independent of #2439, #2407 and #2469, but it must merge before #2469, or 7708 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 holding a balance is preserved — nonce reset, code and storage cleared, balance untouched — rather than deleted. 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(), which is>= MONAD_ETH_AMSTERDAMin both trait families; pre-Amsterdam revisions must keep burning.Reserve balance is deliberately unchanged
A preserved account is a funded, nonce-0, codeless address, and no predicate distinguishes it from an ordinary pre-funded address — three
Accountfields match by construction, and the fourth,incarnation, is drawn from the same source for both and is absent from the merkle account. So it takes the existing MIP-4 floor exactly as a pre-funded address does today. Exempting it would mean making the storage-generation key carry account provenance, and that equivalence is the reason not to.Spec tests
Nine fixtures in the Amsterdam suite encode the pre-8246 deletion, and this PR
excludes them in
exclude/MONAD_NEXT_amsterdam.cmake— four undercancun/eip6780_selfdestruct, twotangerine_whistle, and one each infrontier/create,paris/securityandmonad_nine/mip4_checkreservebalance.They fail on a
postStatethat omits the balance-only account 8246 nowpreserves. 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
EIP8246fork class and aselfdestruct_no_burntest directory, but the class is a stub and no fixturesare generated from it, so no such bundle exists yet.
The
mip4_checkreservebalanceentry is coarser than the rest: gtest filters perfile, and that file's cases are mostly
selfdestruct_Falseones 8246 does nottouch, so they are suppressed as collateral rather than because they encode
pre-8246 behaviour.
The nine exclusions mean the Amsterdam job cannot exercise 8246's own behaviour, so the unit tests are the real gate. Accordingly, every assertion was checked by deleting the code it covers and confirming the test fails.
Invariants recorded at the site
Three, because nothing else states them:
destruct_touched_deadis an untemplated second deletion pass and spares a preserved account only because itsis_deadtest and the balance test are the same EIP-161 emptiness test;BlockState::can_mergeruns beforeexecute_final, which is what stops relaxed merge zeroing the balance and committing an empty account; and the reserve-balance check runs at depth zero before finalization, so rule 2 is invisible to it.Testing
Full
MONAD_COMPILER_TESTING=ONbuild clean.ctestclean apart from the knownBacktraceTest.works. All spec-test jobs pass.clang-formatandlicense.shclean.Related: #2508 (exec-event schema cannot express a code-cleared surviving account).
🤖 Generated with Claude Code
https://claude.ai/code/session_01T2YcdwU1mj3Lqunp9Y7pqa