Implement turnstile and migration

[illuzen]

Wormhole soundness turnstile

Summary

Adds a Zcash-style turnstile to the zk-wormhole so the chain can never mint more
value out of the wormhole than could possibly have been deposited into it. The
invariant enforced on every exit is:

TotalWormholeExits <= PotentialWormholeBalance

If a ZK soundness bug ever let a forged proof through, this backstop caps the
damage at the currently-available margin and rejects the exit instead of minting
unbacked tokens.

How it works

• PotentialWormholeBalance — running total of value held by ambiguous
  addresses (accounts with nonce == 0, i.e. that have never signed a
  transaction). These are the addresses that could be wormhole deposits.
• TotalWormholeExits — cumulative value minted via wormhole exits.
• Deposit tracking: record_transfer adds to the pool whenever a native
  transfer lands in an ambiguous address (covers ordinary transfers, genesis
  endowments, mining rewards, and exits that flow back into the wormhole).
• Reveal: the moment an account first signs a transaction it is no longer
  ambiguous, so WormholeProofRecorderExtension::validate() subtracts its
  balance from the pool.
• Enforcement: verify_aggregated_proof checks exits_after <= potential_balance
  before minting any exit output, and rejects with SoundnessInvariantViolation
  otherwise.

A shared is_ambiguous_account() helper is the single source of truth for the
nonce == 0 heuristic, used by both the deposit-tracking and reveal paths.

Seeding an already-running chain

On a chain that predates this tracking, wormhole deposits made earlier aren't
reflected in PotentialWormholeBalance (it would default to 0), so the first
legitimate exit would trip the invariant and brick the wormhole.

A one-shot versioned migration (migrations::v1::InitSoundnessCounters, wired as
MigrateV0ToV1) seeds:

PotentialWormholeBalance = total_issuance()

Every balance in an ambiguous address is backed by issued tokens, so total
issuance is a safe upper bound on what could legitimately be exited — seeding to
it can never block a valid exit. The pool then tightens toward the true ambiguous
sum as accounts reveal themselves. (We intentionally avoid an extra configurable
buffer: it could only loosen the bound, never make it safer.) On a fresh chain
the migration is skipped and the pool is seeded by the block-1 genesis-endowment
record_transfer calls instead.

Changes

• pallets/wormhole/src/lib.rs — PotentialWormholeBalance / TotalWormholeExits
  storage, SoundnessInvariantViolation error, STORAGE_VERSION = 1,
  deposit tracking in record_transfer, invariant enforcement in
  verify_aggregated_proof, and the is_ambiguous_account() helper.
• pallets/wormhole/src/migrations.rs — v0 -> v1 seeding migration (with
  try-runtime pre/post checks).
• runtime/src/lib.rs — wire MigrateV0ToV1 into Executive and bump
  spec_version 131 -> 132.
• runtime/src/transaction_extensions.rs — reveal logic in the proof-recorder
  extension's validate(), weight accounting, and extensive integration tests.
• Cargo.tomls — try-runtime feature plumbing.
• docs/wormhole-soundness-detection-plan.md — design write-up.

Testing

pallet-wormhole (25 pass) and the runtime transaction_extensions suite
(27 pass) are green. Coverage includes:

• Deposits to ambiguous vs. revealed addresses.
• The full sender/recipient nonce matrix through a complete transaction
  lifecycle (validate -> prepare -> dispatch -> post_dispatch), including batch
  transfers (sender revealed once, each ambiguous recipient counted).
• Mining rewards: a mined block's full emission flows into the pool (miner and
  treasury are both ambiguous).
• Migration seeds the pool to total issuance.
• Soundness enforcement: an exit is rejected with
  SoundnessInvariantViolation when the margin is exhausted (P == E), even when
  the exit targets an ambiguous address (exiting into another wormhole is valid,
  but must still respect the margin), plus the cold-start P == 0 case.

Notes

• The nonce == 0 heuristic is a deliberate over-approximation of "wormhole
  deposit address": unused accounts are also counted, which only makes the bound
  looser (safe). Revealing tightens it back down.
• Exiting into another wormhole address (ambiguous) is a supported operation; the
  exit output legitimately re-counts toward the pool because it's still inside the
  wormhole system. Margin is only consumed when value leaves to a revealed account.

---

Note

High Risk
Changes wormhole exit minting and supply accounting with a runtime migration; a bug in counters or reveal timing could block legitimate exits or weaken the backstop.

Overview
Adds an on-chain wormhole soundness turnstile so cumulative ZK exits cannot exceed value tracked as possibly wormhole-backed. The enforced invariant is TotalWormholeExits <= PotentialWormholeBalance.

PotentialWormholeBalance grows when native transfers (via record_transfer) land on ambiguous accounts (nonce == 0, via is_ambiguous_account). TotalWormholeExits increases only after successful aggregated exits. verify_aggregated_proof checks the invariant before minting and fails with SoundnessInvariantViolation otherwise.

First-time signers reveal as non-wormhole: WormholeProofRecorderExtension::validate subtracts the signer’s free balance from the pool (with updated weight accounting). Pallet storage version bumps to v1; runtime spec_version 132 wires MigrateV0ToV1, which one-shot seeds PotentialWormholeBalance = total_issuance() so pre-tracking deposits are not bricked on live chains.

Includes design doc, try-runtime feature plumbing, and broad pallet/runtime tests (deposit/reveal matrix, mining rewards, margin-exhausted exits).

^{Reviewed by Cursor Bugbot for commit 5b4e863. Configure here.}

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 5b4e863. Configure here.}

[cursor]

Migration omits historical exit total

High Severity

The v0→v1 migration seeds PotentialWormholeBalance from total_issuance() but leaves TotalWormholeExits at its default of zero. On a chain that already processed wormhole exits, the turnstile treats cumulative exits as starting at upgrade, so exits_after <= potential_balance can allow roughly another full issuance worth of exits before rejection, undermining the stated lifetime cap.

 

^{Reviewed by Cursor Bugbot for commit 5b4e863. Configure here.}

[n13]

Review: Wormhole soundness turnstile

Reviewed the full diff plus surrounding code (pallets/wormhole/src/lib.rs, migrations.rs, runtime/src/transaction_extensions.rs, the fee/mining-reward path in pallet-mining-rewards, and the runtime wiring). Static review only (didn't compile — ZK deps make a build slow).

Verdict

The core idea is sound and the implementation is careful and well-tested. The invariant TotalWormholeExits <= PotentialWormholeBalance is algebraically just conservation of the ambiguous pool (cumulative outflows <= cumulative inflows), so the accounting is coherent and the failure direction is safe — a tripped invariant rejects an exit, it never over-mints. Mergeable as a backstop, but there's one design limitation that meaningfully weakens its security value over time, plus an encapsulation issue worth fixing. None are hard blockers.

Strengths

• is_ambiguous_account() as the single nonce == 0 source of truth — good DRY.
• VersionedMigration<0,1,…> is the right pattern; fresh chains skip it and seed via the block-1 on_initialize genesis-endowment calls, existing chains seed once.
• Seed = total_issuance() is genuinely a safe upper bound — can't block a valid exit.
• Enforcement is check-before-mint, and TotalWormholeExits is committed only after every mint succeeds.
• Reveal timing reasoning is correct: all extensions' validate run before any prepare, and CheckNonce increments in prepare, so nonce == 0 in validate reliably means "first signature."
• try-runtime feature plumbing is valid — pallet-balances/pallet-zk-tree are real (non-dev) deps, so the new entries compile.

Findings

1. The backstop loosens monotonically — keyless accounts never "reveal" (most important)

The PR claims the pool "tightens toward the true ambiguous sum as accounts reveal themselves." That's only true for accounts that can sign. Keyless/derived accounts never sign, so their inflows enter PotentialWormholeBalance and are never released.

Concrete, every block: pallet-mining-rewards::on_finalize mints treasury_reward to the treasury account and calls record_transfer_proof. The treasury is a PalletId-derived account with nonce == 0 forever -> ambiguous -> P += treasury_reward. When the treasury later disburses, the reveal path never fires (treasury never signs), so P is not decremented. Same for multisig accounts, pure proxies, and sovereign/pallet accounts that move funds without their own nonce incrementing.

Net effect: P accumulates a permanent, monotonically growing term, so the margin P - E grows without bound from treasury emission alone. Safe for liveness (only makes false positives less likely), but it steadily inflates the headroom a forged exit could consume before tripping — i.e. it erodes the very backstop this PR exists to provide. The added test counter_mining_rewards_increase_potential_balance even bakes this in.

Suggestion: decrement P when value leaves known keyless accounts (treasury payouts), or exclude the treasury portion from the pool, or at minimum document that the bound is loose-and-growing rather than tightening.

2. Add-side and subtract-side use different sources of truth

Deposits add the transferred amount (via record_transfer), but reveal subtracts Currency::free_balance(signer). Different code paths in different crates, not guaranteed to net to zero per account:

• Untracked credits inflate free_balance without incrementing P, so the eventual reveal over-subtracts (pushes P down — the dangerous direction for liveness). Clearest example: force_set_balance is counted in count_transfers() (weight) but emits BalanceSet, which record_proofs_from_events_since does not scan (it only matches Balances::Transfer and Balances::Minted). So a sudo balance bump isn't tracked, yet a later reveal subtracts the full balance.
• Reserved balance is excluded by free_balance, so an account holding a reserve at reveal time subtracts less than was added.

The huge seed + saturating_sub absorb this, so unlikely to bite, but the coupling is fragile. Worth reconciling (track per-account contribution, or subtract total balance and ensure every credit path the reveal will later subtract is actually tracked).

3. Counter is mutated from two crates (encapsulation / DRY)

The deposit increment lives in the pallet (record_transfer), but the reveal decrement reaches directly into pallet storage from the runtime:

pallet_wormhole::PotentialWormholeBalance::<Runtime>::mutate(|total| {
    *total = total.saturating_sub(balance);
});

The counter's invariant is split across the crate boundary. Cleaner to expose one pallet method that encapsulates read-balance-and-subtract, e.g. pallet_wormhole::Pallet::<T>::on_account_revealed(&who), so the extension just calls it. Keeps all mutations of the counter (and the ambiguity check) in one place, consistent with the deposit side.

Nits

• Enforcement is placed after UsedNullifiers are marked. Correct because ensure! -> Err rolls back the whole dispatch (transactional-by-default, which existing minting/? paths already rely on), so a rejected exit doesn't burn its nullifiers. Slightly cleaner to check the invariant before marking nullifiers. The new tests assert end-state counters but don't independently assert nullifiers are not consumed on the rejection path — adding that assertion would lock in the rollback guarantee the safety story depends on.
• Mixed token APIs: seed/deposit use the fungible traits (total_issuance, NativeBalance) while reveal uses legacy Currency::free_balance. Works for native, but inconsistent.
• Duplicate branch: record_transfer now has two adjacent if asset_id == T::AssetId::default() blocks (new tracking + existing event). Merge them.
• Reveal weight (reads_writes(2,1)) is added unconditionally to every extrinsic, including unsigned and already-revealed signers where the real work is one nonce read. Conservative/safe, just slightly over-charged.
• False-positive edge case: if a genuine deposit address ever signs (reveals) before its ZK exit, the reveal subtracts its balance and the later legit exit could trip SoundnessInvariantViolation until other deposits grow P. Unusual for the privacy model, and only a contingent block (not a loss), but worth a docs note.

[n13]

Review: Wormhole soundness turnstile — verdict

Reviewed the full diff plus surrounding code (pallets/wormhole/src/lib.rs, migrations.rs, runtime/src/transaction_extensions.rs, the fee/mining-reward path in pallet-mining-rewards, and the runtime wiring). Static review only — didn't compile (ZK deps make a build slow).

Verdict

The design is sound and safe in the failure direction: the invariant TotalWormholeExits <= PotentialWormholeBalance is just conservation of the ambiguous pool (cumulative outflows ≤ cumulative inflows), and a tripped invariant rejects an exit — it never over-mints. The implementation is careful and well-tested. Mergeable as a backstop, but it ships with one security limitation that grows over time plus a couple of correctness/encapsulation issues worth addressing first. None are hard blockers.

On the existing reviews

• Bugbot ("Migration omits historical exit total", High): the symptom is real but the framing is off. TotalWormholeExits never existed pre-upgrade, so "seed E too" is impossible — you can't recover historical cumulative exits. The real issue is that seeding P = total_issuance() with E = 0 makes the post-migration cap roughly a full issuance of headroom. The fix is a tighter P seed (below), not seeding E. I'd treat this as Medium, not High: it only loosens the bound, it never blocks a valid exit.
• @n13's review: accurate; I independently confirmed every substantive point against the code (see findings).

Findings

1. Backstop loosens monotonically — keyless accounts never reveal (most important).
pallet-mining-rewards::on_finalize mints treasury_reward to the treasury (a PalletId-derived, never-signing account → ambiguous forever) and the miner, then calls record_transfer_proof. Both land in P every block, and the treasury portion is never decremented (treasury never signs → reveal never fires). Same for multisig/pure-proxy/sovereign accounts that move funds without their own nonce incrementing. P therefore accrues a permanent, monotonically growing term, inflating the margin P − E from emission alone and steadily eroding the very backstop this PR provides. The counter_mining_rewards_increase_potential_balance test bakes this in. The PR text claims the pool "tightens toward the true ambiguous sum"; that is not true for keyless accounts. Suggestion: decrement P on known keyless payouts (treasury disbursements), exclude the treasury portion, or at minimum document that the bound is loose-and-growing rather than tightening.

2. Add-side and subtract-side use different sources of truth.
Deposits add the transferred amount (via record_transfer); reveal subtracts Currency::free_balance(signer). These aren't guaranteed to net to zero per account. record_proofs_from_events_since only matches Balances::Transfer/Minted and Assets::Transferred/Issued — not Balances::BalanceSet. Yet count_transfers counts force_set_balance (for weight). So a sudo force_set_balance raises a balance that was never added to P, but a later reveal subtracts the full balance → over-subtraction (the liveness-dangerous direction). Reserved balance is likewise excluded by free_balance. The huge migration seed + saturating_sub hide this on an existing chain, but on a fresh chain (tight seed) it could wrongly trip SoundnessInvariantViolation and block a legit exit. Root-gated, so low probability — but it's a silent over-subtraction. Suggestion: track per-account contribution, or ensure every credit path the reveal later subtracts is actually tracked.

3. Storage mutation inside validate() (new — not raised yet).
The reveal performs a storage write in validate():

if let Ok(signer) = ensure_signed(origin.clone()) {
    if pallet_wormhole::Pallet::<Runtime>::is_ambiguous_account(&signer) {
        let balance = <Balances as Currency<AccountId>>::free_balance(&signer);
        if balance > 0 {
            pallet_wormhole::PotentialWormholeBalance::<Runtime>::mutate(|t| *t = t.saturating_sub(balance));
        }
    }
}

validate() is the pool-validation entry point and is intended to be side-effect-free (it can be called repeatedly against discarded overlays). The idiomatic place for committed pre-dispatch state changes is prepare(). It happens to work today (pool runs are discarded; the committed write occurs once during block execution), but it's fragile and inconsistent with the rest of this very extension, which mutates in post_dispatch. Cleaner: detect ambiguity + capture the balance in validate, return it via Val, and apply the subtraction in prepare.

Related: the new integration tests are good but they call validate → prepare → dispatch → post_dispatch directly, not through frame_executive::apply_extrinsic. So the two things the safety story leans on — that validate-phase writes commit in the real pipeline, and that all extensions' validate run before any prepare (relative to CheckNonce) — are assumed by construction, not exercised end-to-end.

4. Counter is mutated from two crates (encapsulation / DRY).
The deposit increment lives in the pallet (record_transfer), but the reveal decrement reaches into pallet storage from the runtime (pallet_wormhole::PotentialWormholeBalance::<Runtime>::mutate(...)). The counter's invariant is split across the crate boundary. Cleaner to expose one pallet method, e.g. Pallet::<T>::on_account_revealed(&who), so the extension just calls it — keeping all mutations (and the ambiguity check) in one place, consistent with the deposit side.

Concrete suggestions (ranked)

1. Tighten the migration seed. Seed P to the sum of balances over nonce == 0 accounts instead of total_issuance(). That's the true current ambiguous pool, makes E = 0 actually correct, gives a meaningful cap immediately, and resolves the Bugbot concern. Cost is account iteration — fine for a young chain, or use a multi-block migration. If you keep total_issuance(), document that the post-migration cap is ~full issuance.
2. Address the keyless/treasury monotonic growth (finding 1).
3. Encapsulate the counter via on_account_revealed, and move the mutation into prepare() (findings 3 + 4).
4. Reconcile add vs subtract sources (finding 2), notably force_set_balance/BalanceSet and reserved balance.

Nits

• Duplicate adjacent if asset_id == T::AssetId::default() blocks in record_transfer — merge them.
• Soundness check is placed after UsedNullifiers are marked. Correct only because a returned Err rolls back the whole transactional dispatch; slightly cleaner to check before marking. The rejection-path tests should also assert nullifiers are not consumed, to lock in the rollback guarantee the safety story relies on.
• Reveal weight (reads_writes(2,1)) is added unconditionally to every extrinsic, including unsigned/already-revealed — conservative but over-charged.
• Mixed token APIs: migration/deposit use the fungible traits; reveal uses legacy Currency::free_balance. Works for native, but inconsistent.
• False-positive edge case: if a genuine deposit address ever signs (reveals) before its ZK exit, the reveal subtracts its balance and the later legit exit could trip SoundnessInvariantViolation until other deposits grow P. Contingent block, not a loss — worth a docs note.

Verified correct

• VersionedMigration<0,1,…>, STORAGE_VERSION = 1; fresh chains skip the migration (genesis writes version 1) and seed via block-1 record_transfer calls; existing chains seed once.
• Check-before-mint; TotalWormholeExits committed only after all mints succeed.
• Exiting into an ambiguous address re-adds to P and is balanced by the reveal subtraction — traced the full path; it does not allow unbounded extraction to revealed accounts (margin is correctly paid back on reveal).
• try-runtime plumbing valid: pallet-balances and pallet-zk-tree are real (non-dev) deps of pallet-wormhole.

[n13]

If I read this correctly

• We're missing treasury events - we need to "decrement P when value leaves a known keyless account" or "exclude the treasury portion"
• Missing force_set_balance (sudo, I don't think we should call this, but still, should also not screw up the turnstile)
• validate() is the wrong place to modify the counter... see (3) above, we should fix this

[n13]

Re-review (commits 27010d28, 2c902192, d94f2f14)

Thanks for the fast turnaround — all three findings from the first pass are addressed, and the validate side-effect concern was fixed properly too. Re-reviewed statically against the rest of the runtime (didn't rebuild).

Verdict: LGTM / approve

The turnstile is now materially tighter and the bookkeeping is cleanly encapsulated. Two low-severity, non-blocking follow-ups below.

Findings addressed

• Keyless accounts loosening the backstop — fixed via NonWormholeAccounts: Contains<AccountId> (excludes registered multisigs, the configured treasury, and the keyless PalletId/sentinel accounts), so their balances no longer inflate the pool. The mining test now asserts only the miner's ambiguous portion enters the pool and that it's strictly less than the full emission — exactly the treasury case I flagged.
• Multisig pre-funding gap (the subtle one this surfaces) — handled correctly: a pre-computed multisig address looks ambiguous, so a pre-fund is counted, and create_multisig calls reveal_address after registering the address, deducting its balance so it nets zero, and is_multisig excludes it thereafter. Nice catch and good test (counter_multisig_creation_reveals_prefunded_address). Ordering (insert → reveal) is correct.
• free vs total balance — now subtracts Currency::total_balance (free + reserved); the "ambiguous accounts can't spend, so total == sum of credits" justification is sound. force_set_balance removed from count_transfers (it emits BalanceSet, never recorded) and the over-subtraction caveat is documented on reduce_potential_balance.
• Encapsulation / DRY — reveal_account / reduce_potential_balance pallet methods + the TransferProofRecorder::reveal_address trait hook mean the runtime and multisig pallet no longer poke pallet storage directly.
• Bonus: moving the mutation out of validate (capture balance in Val, commit in prepare) is exactly right — validate is now side-effect free, and capturing the pre-tx balance keeps the subtraction correct regardless of fee-charge ordering.

Remaining (non-blocking)

1. The total_balance reveal over-subtracts for any untracked credit path, not just force_set_balance. The invariant the reveal relies on is "every credit the account received was added via record_transfer." That also breaks for governance-scheduled transfers (the scheduler is EnsureRoot, dispatches in on_initialize with no transaction extension, so the Transfer/Minted event is never scanned → pool not incremented), and for any future pallet-internal credit. All such paths are privileged today, so the risk is bounded and in the safe-ish (liveness, not safety) direction, mitigated by the conservative seed. Worth either generalizing the caveat doc beyond force_set_balance, or — for true robustness — tracking a per-account contributed amount and subtracting that instead of total_balance.

2. Weight under-counts the new storage reads. is_ambiguous_account now also reads Multisigs (is_multisig) and TreasuryAccount (treasury_account()) on every call — i.e. once per recorded transfer (deposit side) and once per signed tx (reveal detection). reveal_weight = reads_writes(2, 1) and the per-transfer count_transfers weight don't include these extra ~2 reads (nor the conditional pool write in record_transfer). Minor, but worth tightening so block weight stays a true upper bound.

Nits

• NonWormholeAccounts::contains rebuilds the 5-element keyless array (with into_account_truncating hashing) on every call; could hoist to a parameter_types!/const.
• The treasury is excluded twice — once via the configured treasury_account() getter (the real one) and once via TreasuryPalletId::…into_account_truncating() (a different, likely-unused derived account). Harmless, but the comment calling the latter "the treasury" is slightly misleading since the treasury account is configurable in this fork.