Skip to content

Fix/block based root window - #118

Merged
nol4lej merged 8 commits into
mainfrom
fix/block-based-root-window
Aug 5, 2026
Merged

Fix/block based root window#118
nol4lej merged 8 commits into
mainfrom
fix/block-based-root-window

Conversation

@nol4lej

@nol4lej nol4lej commented Aug 5, 2026

Copy link
Copy Markdown
Member

The bug

The historic-root window was bounded by MaxHistoricRoots (100) counted in leaf insertions, while an unsigned transaction stays valid in the pool for TX_LONGEVITY (64) blocks. Two clocks, two different units.

A private_transfer inserts up to 2 commitments, so ~50 transfers rotated the entire window. Under load an honest spend would:

  1. pass validate_unsigned and enter the pool,
  2. gossip across the network,
  3. get included in a block,
  4. and only then revert with UnknownMerkleRoot — nullifier still unspent.

No attacker was required; ordinary throughput did it. An attacker could force it cheaply by submitting valid transfers, wiping out everyone else's anchors.

The fix

Retention is now RootRetentionBlocks, a Config constant set to 300 blocks (~30 min at 6s) against a 64-block longevity. integrity_test asserts the two stay aligned, so a runtime that reintroduces the mismatch refuses to build.

MaxHistoricRoots keeps its name but changes meaning: it is now a safety cap on queue length (raised to 16384), not the window.

Two supporting changes:

  • The queue moved from a single BoundedVec to a slot-indexed StorageMap with head/tail pointers. A StorageValue is read and rewritten in full on every leaf insert; once the window holds thousands of roots that is hundreds of KiB of I/O on the hottest path in the pallet — a worse problem than the one being fixed.
  • is_known_root now accepts the active root unconditionally. Expiries are refreshed only by a leaf insert and the pallet has no hooks, so a chain idle for a full window would expire the very root every wallet proves against. private_transfer and unshield both need a known root, and only a funded shield could mint a new one — the pool would wedge.

Storage migration

MigrateToV3 (STORAGE_VERSION 2 → 3) rewrites both items. Every existing root is kept and granted a full window from the upgrade block rather than dropped: a root on chain backs proofs wallets may be about to submit.

It enumerates the old map by raw key rather than using translate, which does not delete an entry whose value fails to decode — it logs, skips, and leaves the bytes in place. With a 1-byte bool becoming a 4-byte block number that would leave unreadable, unprunable residue under live keys. Map and queue are written in the same loop so they cannot diverge.

Review notes

Two rounds of adversarial audit against this implementation found seven defects in its first version. All are fixed here, but they are the parts worth a close look:

# Defect
1 No migration at all — v2 data would have become undecodable
2 The translate misconception above
3 Map/queue divergence leaking permanently spendable roots
4 Active root expiring on an idle chain (re-introduced the original bug)
5 Live roots evicted by the cap path
6 defensive! panicking in debug builds on a reachable operational state
7 A post_upgrade that could not fail on the bug it guarded

Also in this branch

  • Weights re-benchmarked against the v3 layout (VPS, 50 steps / 20 repeats). The old file declared HistoricRootsOrder, which no longer exists, and never mentioned the new items — every insert was charged 2 storage ops while doing up to 23. The measurement exercises a queue with expired entries, so the per-insert pruning is priced rather than free.
  • Four modules split into directories (merkle, storage, validate_unsigned, types), one file per responsibility. No behaviour change — re-exports keep every path, so nothing outside the pallet was touched.
  • Custom(2)Custom(4) for amount overflow. Code 2 meant two different things depending on which validator rejected the transaction, so a Custom error: 2 in a node log was ambiguous. This is observable interface: a test now pins the values.

Verification

  • 298 pallet tests, 65 precompile tests
  • cargo check clean on runtime (WASM), try-runtime, and runtime-benchmarks
  • 78 end-to-end checks against a running dev node, on a fresh chain and on one with 1200 blocks of accumulated state: shield_batch, same-block bursts, idle chains, orphan detection in both directions, and a spend against a root the chain has churned past

The steady-state probe is the one that proves the fix: after driving past a full retention window, 30 further inserts grew the queue by 2 slots, not 30 — settling at 301 against retention = 300. Under the old window those 30 inserts would have evicted 30 roots regardless of elapsed time.

Deploy

spec_version 6 → 7 (included). transaction_version unchanged — no call signature changed. RUNTIME_VERSIONS.md previously listed 7 and 8 as separate rows; neither was deployed, so they collapse into one.

@nol4lej
nol4lej merged commit 58a2ac5 into main Aug 5, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant