[#3662] Stop endowing the handler at genesis, and report the value a self-destruct destroys - #43
Conversation
…wment The Supra EVM cannot mint, so native SUPRA on the EVM side is becoming a mirror of value escrowed on the Move side, arriving only by crossing. Endowing the handler at genesis has no place in that model, and the handler's own test suite already asserts the invariant an endowment breaks: its balance equals ERC20Supra's total supply, which holds only while every unit of native it holds was deposited in exchange for tokens. The initial_native_token configuration field and the parameter chain that carried it to the proxy deployment are removed rather than set to zero, so the concept cannot return by accident. The proxy now deploys through GenesisTransaction::create, which is byte-identical apart from carrying no value, and check_multisig_setup asserts the deployed handler holds nothing. Refs Entropy-Foundation/smr-moonshot#3662, Entropy-Foundation/smr-moonshot#3474
…stroys A `SELFDESTRUCT` that names the destroyed account as its own beneficiary zeroes the balance without crediting anyone, so that value leaves circulation. Supra's EVM native balance mirrors value escrowed outside the EVM and cannot be minted, so the host needs the exact amount in order to account for it; `SelfDestructResult::had_value` only says whether the balance was non-zero. Accumulate the destroyed amount on `JournalInner` at the point where the journal decides to zero the balance, and subtract it again whenever the journal entry that recorded it is reverted, so a reverted frame or a discarded transaction reports nothing. The total is not cleared at the transaction boundary, so it is still readable after execution; the host drains it with `take_selfdestruct_burn` once per transaction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed The invisible burn
Recognised at the branch that causes it, not reconstructed from a state diff — a diff cannot distinguish a burn from a transfer without re-deriving that condition, and would be wrong the moment it changed. The pre-Cancun self-target case comes free, since it takes the same branch. Revert safety is structural, not bookkeepingThis was the property that mattered most: an over-reported burn would let the node account for value that was never destroyed. A burn is only ever recorded alongside an Three tests cover it, and each of the three decision points was mutation-tested: dropping the Deliberately not done
Baseline, and a correction to how it has been describedTwo pre-existing hard compile errors sit in lib-test targets — Worth recording for anyone measuring this repo again: an unqualified clippy run aborts at Two things left open rather than papered overA lifetime judgement call for review. The accumulator is not cleared in An unexplained build result. The very first baseline Also untested: all nine tests drive
|
isaacdoidge
left a comment
There was a problem hiding this comment.
Reviewed both commits, with the accumulator's placement question examined explicitly.
Is JournalInner the right place? Yes — and here is the reasoning worth recording.
Everything else block-cumulative in this stack is aggregated outside revm, in the executor, over per-transaction outputs: receipts, cumulative gas, the settlement records, and now the fee side of the destroyed total all live in EvmExecutionAggregatedResults. Upstream revm keeps the same discipline — the journal's per-tx metadata is cleared by commit_tx/discard_tx, and the only thing that outlives a transaction is state itself. So this field is genuinely the first of its kind, and the burden of proof is on it.
It carries that burden for two reasons:
- The fact exists only at journal-entry granularity. A self-targeting destruction removes the account from the final state, and
ExecutionResultcarries no balance delta, so a host-side aggregation has nothing to aggregate from. Something inside revm must surface it; the only alternatives are a new field threaded throughResultAndState/handler/inspector types (a much larger upstream-merge surface, per-tx attribution we don't need) or this. - The number must obey frame-revert semantics exactly, and the implementation makes that coupling structural rather than reconstructed: the revert paths re-derive the reverted amount from the very entries being reverted (
entry.selfdestruct_burn()), so the counter and the state cannot disagree by construction. The nested-revert and discard tests pin the property.
Two contracts keep it sound, both worth keeping visible: the drain discipline (take between transactions only — the saturating_sub in the revert paths silently tolerates a mid-tx drain rather than failing it, so the field doc's "drain between transactions" is load-bearing), and the lifetime coupling — the block total falls out of Supra's one-journal-per-block usage; an embedder that finalizes per transaction gets per-tx totals instead, which still composes via take, so nothing breaks, but the "block" in the doc is a property of the caller, not of the journal.
Correctness
The branch analysis is exact: only the destroying branch with a self-targeting beneficiary counts (a distinct beneficiary transfers, the post-Cancun not-created-locally self-target is a no-op that writes no entry and counts nothing), a repeated destruction sees a zero balance and cannot double-count, and the pre-Cancun behaviour matches the destroying branch's actual semantics. commit_tx/discard_tx/finalize all destructure the field explicitly — the existing destructuring pattern did its job of forcing every lifecycle path to take a position. Test coverage hits every case I could construct, including the cross-tx no-op and finalize survival.
Nits
JournalEntryTr::selfdestruct_burnis a required method with no default. AU256::ZEROdefault impl would let otherENTRYimplementors (and future upstream syncs) compile without caring, at no cost toJournalEntry.- The genesis commit (deploy the handler unendowed, drop
initial_native_tokenend to end) pairs correctly with the smr-side removal and thevalue() == 0assertion; the config-field removal is a breaking config change, fine under the pre-production policy.
…rain contract `JournalEntryTr::selfdestruct_burn` reports a Supra-local concept, so give it a zero default: an entry type with no notion of a destroyed balance, or one arriving from an upstream sync, then needs no change. `JournalEntry` overrides it, so nothing is lost. Document the two contracts the accumulator places on its caller rather than on the journal. The revert paths subtract with `saturating_sub`, which makes a mid-transaction drain quiet instead of rejected, so the field is only correct if it is drained between transactions; say so where a caller will read it. And the journal has no notion of a block, so record that the total covers whatever span the caller chooses to drain over, which for Supra is a block because it runs one journal per block. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…urn total A revert that tries to take back more than the accumulator holds can only mean the total was drained while the destruction it covers was still revertible, so the saturation is itself the symptom. Assert against it at both revert sites, which turns the drain contract from a documented requirement into one that fails loudly wherever an embedder would introduce the violation. Release behaviour is unchanged: the subtraction still saturates, because on a consensus-critical path a caller that breaks the contract should cost accounting accuracy rather than halt the node. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The framing correction is worth recording, because I had it wrong too"The counter and the state cannot disagree by construction" holds inside the drain discipline, not at its boundary. A caller who drains mid-transaction leaves a later revert with nothing to take back, so the drained total keeps a burn that never happened, and Most of that contract is now construction againA mid-transaction drain is detectable at the moment it bites: a revert path trying to subtract more than the accumulator holds is the symptom. Both subtraction sites now carry a The spurious-fire question was worked before adding it, and the enumeration is the reason to trust it: every burn is accumulated in the same branch that pushes its "Release behaviour unchanged" was evidenced rather than asserted: the release binary reports 9 burn tests against debug's 10, which is the Two doc contracts, both caller obligationsDrain between transactions only — with the reason stated, that the saturation is what makes a violation quiet, so the discipline is a requirement and not advice. Mirrored onto Scope is whatever the caller drains — the journal has no notion of a block. The block total falls out of Supra running one journal per block; an embedder finalizing per transaction gets per-transaction totals from the same field, and they still compose, because each drain covers exactly the destructions since the last. The end-to-end wiring is covered, contrary to my earlier noteI recorded a residual that nothing downstream read the accumulator and that no test drove real That second half is the one this design needs: without the drain, block one's endowment would be counted again and the escrow asked to retire it twice. |
Two changes to the Supra revm fork, both required by the supply-mirroring work in Entropy-Foundation/smr-moonshot#3662: the EVM must stop being endowed with value it cannot mint, and every path that destroys native value must be observable so the node can account for it.
Part of Entropy-Foundation/smr-moonshot#3662. No
Closeskeyword: GitHub only auto-closes issues in the same repository, so a keyword here would not fire and would misrepresent what merging this does. The issue is closed by the smr-moonshot half, which merges last.1. The handler deploys without a genesis endowment
Today's design endows
ERC20SupraHandlerwith the entire SUPRA supply cap at genesis. Under the new model EVM native supply starts at zero and every unit arrives by crossing from the Move side, so there is nothing for genesis to endow.Two independent reasons to remove it rather than shrink it:
test/ERC20SupraHandler.t.solassertsaddress(erc20SupraHandler).balance == token.totalSupply()in three places. That holds only while every unit of native the handler holds was deposited in exchange for tokens. A genesis endowment violates it in real genesis state; the tests pass only because they deploy the handler unendowed.ERC20Supra.initializenever mints, so total supply at genesis is zero; the handler is the only authorised minter; its only mint path requires sending native in; andwithdrawrequires burning tokens you already hold. The endowed native can only be reached by exercising the owner's unbounded mint authority or a UUPS upgrade — the powers smr-moonshot#3661 removes.initial_native_tokenand the whole parameter chain that carried it are removed rather than set to zero, so the concept cannot return by accident. The proxy deploys viaGenesisTransaction::create— byte-identical, same constructor args, same precomputed address, same nonce accounting, no value — andcheck_multisig_setupasserts the deployed handler holds nothing, kept deliberately as a regression guard.2. The value an EIP-6780 self-destruct destroys is now reported
JournalInner::selfdestructhas a balance-transfer block gated onaddress != target, so its first branch zeroes the balance with no credit anywhere exactly when a contract created in the same transaction self-destructs to itself. That is a genuine burn of native value, and revm reported onlyhad_value: bool; the destroyed account simply vanished from the final state. A node that cannot see it cannot account for it.Recognised at the branch that causes it, not reconstructed from a state diff — a diff cannot distinguish a burn from a transfer without re-deriving that condition, and would be wrong the moment it changed. The pre-Cancun self-target case comes free, since it takes the same branch.
Revert safety is structural. The amount is read from the
AccountDestroyedjournal entry itself, and bothcheckpoint_revertanddiscard_txsubtract what they drain. So a reverted frame un-reports its burn, an outer frame reverting a nested one un-reports the nested burn too, and a discarded transaction reports zero. The amount cannot survive the entry that records the destruction — a stronger guarantee than two counters kept in step. This mattered most of anything here: an over-reported burn would let the node retire escrow for value that was never destroyed.SelfDestructResultis untouched — it is upstream surface, and the amount has to be summed and revert-adjusted after the opcode returns, which a per-call return value cannot do. Nothing was added toJournalTr;Journalderefs toJournalInnerand the consumer names the concrete type, so the workspace's otherJournalTrimpl is unaffected.JournalEntryTrdid need a method, to read the concrete entry from the generic revert loops.One lifetime property the consumer must honour: the accumulator is not cleared in
commit_tx, which is what makes it readable aftertransact(). A host that never drains will attribute an earlier transaction's burn to a later one. The per-batch total is correct regardless; smr-moonshot drains once per block and tests that a second block does not inherit the first, so this is enforced at the consumer rather than by moving the total into the execution result and touching output surface.Verification
smr-moonshot pins this branch directly (
issue-3662). The parallelissue-3662-rc4branch, which carried the same payload against the older release candidate while smr-moonshot#3674 blocked a tag, has been deleted now that bluealloy#3674 is resolved and this branch has been merged up tofeature/evm_automation. This must become a released tag before the smr-moonshot half can merge.cargo build --workspaceand the crate tests pass;revm-contextgoes from 46 to 55 tests. Each of the three burn decision points was mutation-tested and turns a named test red: theaddress == targetcondition, thecheckpoint_revertsubtraction, and thediscard_txsubtraction.Two conditions inherited rather than introduced, both confirmed by stashing and re-running: two pre-existing hard compile errors in lib-test targets (
precompile_provider.rsE0277,eip3155.rsE0283 — awinnow-versus-coreAsRef<[u8]>ambiguity), and a red clippy baseline of 14 unique diagnostics, identical before and after, so this adds none. Note that an unqualified clippy run aborts atrevm-handlerand never lintssupra-extensionat all;--keep-goingis needed to see the real set.Untested: all burn tests drive
JournalInnerdirectly, so nothing runs realSELFDESTRUCTbytecode throughtransact()and reads the total off the host. The opcode-to-journal wiring is unchanged, but that path is verified from the node side rather than here. There is also norust-toolchain.tomlin this repo (smr-moonshot#3675), socargo +1.97.1was used to match CI's currentstable.