EIP-7708 (ETH transfer logs) - #2469
brett-monad wants to merge 1 commit into
Conversation
5b7256d to
db72acf
Compare
db72acf to
ac3bce4
Compare
ac3bce4 to
23cef36
Compare
23cef36 to
7a9f21c
Compare
why is there a Monad-specific extension needed? |
7a9f21c to
3ae693f
Compare
3ae693f to
0d6fe6c
Compare
|
@claude review this PR |
There was a problem hiding this comment.
Pull request overview
Implements EIP-7708-style observability for native ETH movement by emitting synthetic ERC-20-like Transfer logs (and a Monad-specific Burn log for same-tx created+destroyed accounts) from SYSTEM_ADDRESS, wiring emission through the consensus execution path and updating tests/fixtures accordingly.
Changes:
- Add EIP-7708 feature-gate (
eip_7708_active) to the traits interface and trait implementations. - Emit consensus
Transfer(from,to,value)logs for non-zero native value movement (plus optional eth_simulate synthetic logs), and emitBurn(address,uint256)logs for destroyed residual balances (inline + finalization). - Change
State::destruct_suicidesto return sorted(address, balance)residuals for deterministic burn-log emission ordering; expand/adjust tests and Amsterdam fixture allowlist.
Verdict: NEEDS CHANGES
🤖 Generated with Claude Code
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/ethereum_test/exclude/MONAD_NEXT_amsterdam.cmake | Opens MONAD_NEXT+Amsterdam fixture allowlist to the full suite with updated rationale. |
| category/vm/evm/traits.hpp | Adds eip_7708_active() to the Traits contract and implementations. |
| category/rpc/monad_executor_test.cpp | Updates eth_simulate expectations to include consensus Transfer logs when 7708 is active. |
| category/execution/ethereum/test/test_call_trace.cpp | Updates call-trace tests and adds pinned Amsterdam fixtures for 7708 Transfer/Burn behavior. |
| category/execution/ethereum/state3/state.hpp | Updates destruct_suicides API to return burned residual balances for 7708. |
| category/execution/ethereum/state3/state.cpp | Captures and sorts burned residual balances during suicide destruction for deterministic Burn emission. |
| category/execution/ethereum/state2/test/test_state.cpp | Adds pinned Amsterdam test asserting burned residuals are returned sorted by address. |
| category/execution/ethereum/process_requests.cpp | Removes local SYSTEM_ADDRESS spelling in favor of shared constant. |
| category/execution/ethereum/execute_transaction.cpp | Emits finalization Burn logs based on destruct_suicides return value when 7708 is active. |
| category/execution/ethereum/execute_transaction_test.cpp | Adds pinned Amsterdam test validating finalization-site Burn log behavior. |
| category/execution/ethereum/evmc_host.hpp | Promotes SYSTEM_ADDRESS and SIMULATE_NATIVE_TOKEN_LOG_ADDRESS, adds Burn-log builder, and emits 7708 Transfer/Burn logs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds EIP-7708 Transfer log emission from SYSTEM_ADDRESS on every value-carrying transfer (top-level tx, value-carrying CALL, SELFDESTRUCT, CREATE/CREATE2), plus a monad-specific Burn log at the two sites where an EIP-6780 same-tx-created-and-destroyed account loses ETH: inline at selfdestruct-to-self, and at finalization for residual balance. The value > 0 && from != to predicate is shared with the existing eth_simulate traceTransfers machinery; both consensus and ERC-7528 synthetic emissions coexist rather than one replacing the other, matching geth. SYSTEM_ADDRESS is promoted from a function-local constant in process_requests.cpp to a shared inline constexpr in evmc_host.hpp. Active on MonadTraits<MONAD_NEXT> (which maps to MONAD_ETH_AMSTERDAM) and on EvmTraits<MONAD_ETH_AMSTERDAM> and later.
I traced the following and found nothing wrong:
- Refactored
emit_native_transfer_event: for every combination ofeip_7708_activeandlog_native_transfers_the pre-activation truth table matches the old single-branch check; post-activation the consensus log is emitted unconditionally on the shared predicate and the synthetic still fires only under the flag. EvmcHost::selfdestructinline burn:is_current_incarnation(address)is captured beforestate_.selfdestruct(correct — the selfdestruct doesn't touch incarnation, but this reads consistently even if it did). The burn is gated oncreated_this_tx && beneficiary == address && transferred_balance > 0, and the pairedemit_native_transfer_eventself-skips onfrom == to, so no double emission. The comment "store_log without a matching on_log" documents the intentional trace/receipt asymmetry.- Finalization path:
destruct_suicidesruns after the beneficiary award, captures(address, balance)inside the CANCUN+ branch (guarded bystatic_assert(!eip_7708_active() || evm_rev() >= CANCUN)— nice), and sorts by address.execute_finalthen emits Burn logs from the returned pairs. Ordering matters and is called out in the comment. - Revert safety:
state_.store_logis versioned, so the inline burn rolls back withlogs_.pop_rejectif the emitting frame reverts. Finalization burns are at version 0 and don't need revert semantics. [[maybe_unused]] auto const burned = state.destruct_suicides<traits>();—burnedis only touched insideif constexpr (eip_7708_active()), so the attribute is necessary to silence the unused-variable warning on the discarded-branch path; NRVO keeps the returned empty vector free of overhead pre-activation.- Trait wiring: the
eip_7708_active()requirement is added to theTraitsconcept, and bothEvmTraitsandMonadTraitsadd matchingconstevalpredicates keyed onMONAD_ETH_AMSTERDAM.
Test coverage is thorough — top-level Transfer log, consensus + synthetic pairing, inline burn, finalization burn (via beneficiary award landing on a selfdestructed account), address ordering of the burned set, and the burn-out-of-call-trace invariant. All are pinned to EvmTraits<MONAD_ETH_AMSTERDAM> since no monad revision reaches AMSTERDAM in the shared matrix yet, with the !eip_7708_active() static_asserts on the two touched pre-activation tests making the intent explicit.
One meta note that doesn't block: the PR title still says "held inert on MONAD_NEXT", but the code and the commit message activate on MONAD_NEXT (via MonadTraits<MONAD_NEXT>::evm_rev() == MONAD_ETH_AMSTERDAM), and the MONAD_NEXT_amsterdam.cmake allowlist opens to "*" — consistent with activation, not inertness. Worth updating the title before merge.
All CI green (clang-format, clang-tidy, trait instantiation, Debug+ASAN, RelWithDebInfo, RISC-V, VM tests, CodeQL).
Verdict: CORRECT
🤖 Generated with Claude Code
8f8fdc6 to
c708971
Compare
699bca9 to
e8a609b
Compare
e8a609b to
34bafb7
Compare
34bafb7 to
68a419c
Compare
| // Not every ETH movement emits. The EIP excludes withdrawals (not attached | ||
| // to a transaction, so no natural emission point), priority fees and the | ||
| // base-fee burn (derivable from the header). Beyond the EIP, monad's | ||
| // staking contract moves, mints and burns ETH by direct balance mutation | ||
| // rather than a value transfer, so none of that emits either. |
There was a problem hiding this comment.
Did we explicitly settle the staking contract choice when building eth_simulateV1 @dhil? It seems analogous to withdrawals etc. from Ethereum, but I wanted to check that explicitly.
There was a problem hiding this comment.
No we did not. What semantics do we want here?
There was a problem hiding this comment.
Well - the argument here for Ethereum is that there is no transaction associated with withdrawals etc., so there's nowhere to hang logs from (and the transfers are derivable from the headers there anyway). I think this is different in Monad, because staking operations happen via system calls? If there is a transaction "in hand" when we do staking-related balance transfers, then it would make sense to emit the logs for them, even if it's a system transaction.
There was a problem hiding this comment.
I agree. I am inclined to believe that right now balance transfers done via system calls are not observable in the eth_simulateV1 transfer log. I will test it out, and draft a patch to fix the semantics if needed.
There was a problem hiding this comment.
I guess that property is sort of inert at present though, because we don't allow simulation of system calls? It becomes relevant here because they're consensus artifacts
There was a problem hiding this comment.
Protocol-wise, I am inclined to believe that we should also emit a transfer log. Then I agree with your other comment about hoisting out the emission logic, because it should be used to emit transfer events inside staking too.
There was a problem hiding this comment.
Yep, agreed - thanks for the sense check
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) is emitted on ordinary value transfers, at the four sites Monad already wired: top-level tx transfer, value-carrying CALL, SELFDESTRUCT, and CREATE/CREATE2. EIP-7708's earlier Burn(address,uint256) log is deliberately not implemented. It reported ETH destroyed by a same-tx-created account self-destructing to itself, or left on such an account at finalization. EIP-8246, a prerequisite of the current EIP-7708 text on the same fork, removes both burns at the source -- the balance stays with the account -- so there is nothing left to report, and EIP-7708 drops the case for exactly that reason. destruct_suicides therefore keeps its void signature and no Burn helper or emit site exists. Consensus logs use ETH_SYSTEM_ADDRESS. eth_simulate's traceTransfers keeps the ERC-7528 native-token address and is emitted alongside the consensus log rather than replacing it, and ahead of it: geth's simulate tracer records the synthetic on frame entry, before the consensus log, for calls and creates. Its SELFDESTRUCT differs and no spec fixes either order, so the comments cite it as observed behaviour only. Only the eth_simulateV1 path sets the flag; with it clear a single log exists and consensus never sees the order. Where it is set, the order fixes each log's position and logIndex in the simulate output. Also in this commit: - Make the simulate_v1 trace expectations revision-aware. Four tests asserted log counts and contents that predate 7708. Two carried static_assert(!eip_7708_active()) and failed to compile; two had no guard and failed at runtime. None had been noticed because MONAD_NEXT was filtered out of the typed-test matrices, so they never ran at the only revision where 7708 is active. They now run there, with the doubling expressed by helpers derived from the emitter rather than from observed output. - Assert the receipt side of the transfer logs (State::store_log) from the shared TraitsTest matrix, at every revision, instead of a fixture pinned to Amsterdam: for a top-level transfer the receipt's logs are the call frame's logs in the same order, so one helper checks that in execute_success and simulate_v1_trace and the pinned fixture is gone. Co-Authored-By: Bruce Collie <brucecollie82@gmail.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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)(topic00xddf252ad…) on every non-zero value transfer to a different account: the top-level tx transfer, value-carryingCALL,SELFDESTRUCT, andCREATE/CREATE2. This reuses the existingeth_simulatenative-transfer machinery, re-gated to fire on the consensus path. Two emitter addresses are in play and 7708 does not replace one with the other: the consensus log fromETH_SYSTEM_ADDRESSfires whenever 7708 is active, andeth_simulate's synthetic from the ERC-7528 native-token address0xeeee…is emitted alongside it whentraceTransfersis set — geth returns both. Note thatlogIndexcounts both, so a consumer dropping the0xeeee…entries sees only odd indices.The address was already a function-local constant in
process_requests.cpp, the sender the EIP-7002/7251 system calls execute as. 7708 needs the same address, so it is promoted to a sharedETH_SYSTEM_ADDRESSrather than leaving two spellings of one consensus constant that can silently diverge.There is deliberately no Burn log
An earlier draft of EIP-7708 also specified
Burn(address,uint256)for ETH destroyed bySELFDESTRUCT. That draft then took EIP-8246 as a prerequisite and dropped the log, because after 8246 there is no burn left to report. geth and erigon are in the same configuration.Active on MONAD_NEXT
eip_7708_active()is>= MONAD_ETH_AMSTERDAMin both trait families, andMonadTraits<MONAD_NEXT>::evm_rev()is Amsterdam, so the rule is live on the configuration that ships. The tests run over an explicit two-element type list —MonadRevisionConstant<MONAD_NEXT>andEvmRevisionConstant<MONAD_ETH_AMSTERDAM>— naming both revisions where 7708 is active. They are named rather than derived fromLATEST_SUPPORTED_EVM_FORKso the suite does not depend on that constant having been advanced.Spec tests
No change to the Amsterdam exclusion list. The Amsterdam suite is disabled at
this commit and cannot open here: the fixtures are generated with the whole
fork active, so any Amsterdam EIP not yet in the tree fails its own fixtures —
at this commit the
eip8024_dupn_swapn_exchangeandfrontier/opcodes/all_opcodescases, 47 of 710. It opens once every AmsterdamEIP is in the tree.
Unit tests are unaffected and are the real gate here:
test_state379 andtest_call_trace234 passing, both builds clean.Prerequisite
EIP-7708 lists
requires: 8246, which must be live before this is. Without ittwo
SELFDESTRUCTpaths still destroy ETH with no recipient — aTransferlogcannot express that — so 7708 would claim complete native-value logging while
those burns went unlogged.
🤖 Generated with Claude Code
https://claude.ai/code/session_01T2YcdwU1mj3Lqunp9Y7pqa