Fix/block based root window - #118
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
The historic-root window was bounded by
MaxHistoricRoots(100) counted in leaf insertions, while an unsigned transaction stays valid in the pool forTX_LONGEVITY(64) blocks. Two clocks, two different units.A
private_transferinserts up to 2 commitments, so ~50 transfers rotated the entire window. Under load an honest spend would:validate_unsignedand enter the pool,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_testasserts the two stay aligned, so a runtime that reintroduces the mismatch refuses to build.MaxHistoricRootskeeps its name but changes meaning: it is now a safety cap on queue length (raised to 16384), not the window.Two supporting changes:
BoundedVecto a slot-indexedStorageMapwith head/tail pointers. AStorageValueis 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_rootnow 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_transferandunshieldboth need a known root, and only a fundedshieldcould mint a new one — the pool would wedge.Storage migration
MigrateToV3(STORAGE_VERSION2 → 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-byteboolbecoming 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:
translatemisconception abovedefensive!panicking in debug builds on a reachable operational statepost_upgradethat could not fail on the bug it guardedAlso in this branch
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.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 aCustom error: 2in a node log was ambiguous. This is observable interface: a test now pins the values.Verification
cargo checkclean on runtime (WASM),try-runtime, andruntime-benchmarksshield_batch, same-block bursts, idle chains, orphan detection in both directions, and a spend against a root the chain has churned pastThe 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_version6 → 7 (included).transaction_versionunchanged — no call signature changed.RUNTIME_VERSIONS.mdpreviously listed 7 and 8 as separate rows; neither was deployed, so they collapse into one.