V12 dilithium

[illuzen]

Summary

Remediates a series of correctness, resource-bounding, and DoS-isolation findings in the vendored sc-transaction-pool, and wires --pool-type through the Quantus node so SingleState vs ForkAware can be A/B tested without a rebuild.

Default pool type remains SingleState (per #479: ForkAware mempool retention until finality saturated the pool under stress on our ~99-block PoW finality lag). Quantus pool sizing from #141 is preserved as CLI defaults (--pool-limit 36772, --pool-kbytes 262144).

Transaction-pool remediations

Resource bounds / DoS

• Cap submit_at batches at pool capacity before validation
• Cap maintenance tree-route length for same-height and backward reorgs
• Bound view status streams; keep external watchers open through transient backlog
• Keep import-notification subscribers alive through transient channel backlog
• Bound revalidation queues: bounded view channel + latest-wins mempool slot (no unbounded RevalidateMempool backlog during finalization bursts)
• Fix unbounded event-metrics map growth for rejected transactions
• Prefer Maintained validation lane with biased tokio::select! so Submitted floods cannot starve maintain/block-production validation

Correctness

• Isolate view and mempool revalidation onto separate workers so maintain’s finish_revalidation cannot stall behind uncancellable mempool batches
• Re-mark future deps when ready providers are removed
• Drop stale unlock edges after partial ready replace
• Honor ban expiry in is_banned; drop expired bans on rotator clone
• Evict lowest-priority future transactions first on limit enforcement
• Reclaim submit_and_watch watchers after eventless import failures
• Reclaim dropped-watcher view-map entries for dropped/invalid/usurped txs
• Return one Err per input from extend_unwatched_sync when the mempool sync bridge is dead (avoids submit_local panic on .remove(0))
• Report the evicted block hash (not the block being pruned) on finality-timeout event-handler notifications

Ops / A/B

• Node uses Configuration::transaction_pool instead of hardcoding SingleState
• CLI defaults: SingleState + Quantus stress-test sizing; pass --pool-type fork-aware to exercise ForkAware

SingleState vs ForkAware (context)

	SingleState (default)	ForkAware
Capacity	Frees at best-block inclusion	Holds txs until finality (~20 min at 99-block lag)
Stress history	Survived #479 stress tests	Hit 40k ceiling → ImmediatelyDropped (RPC 1016)
Reorgs	Prune + resubmit (fine for shallow reorgs)	Per-tip views; better for fork-heavy RPC correctness
Grafana (2026-07-31)	Planck / Heisenberg	Dirac already exports ForkAware metrics (active_views)

Recommendation: keep SingleState for miners/validators; A/B ForkAware on RPC nodes (and optionally Planck stress) with larger sizing if needed, e.g. --pool-type fork-aware --pool-limit 52000 --pool-kbytes 409600.

Test plan

• cargo test -p sc-transaction-pool --lib
• Confirm node starts with default SingleState: log Creating transaction pool txpool_type=SingleState
• Confirm --pool-type fork-aware selects ForkAware and exports substrate_sub_txpool_active_views
• Smoke: submit + watch a transfer under both pool types
• (Optional) Stress A/B on Planck: flood submissions; watch RPC 1016 rate, substrate_ready_transactions_number / substrate_sub_txpool_unwatched_txs, maintain duration p95, CPU during fork bursts

---

Note

High Risk
Touches core mempool maintain/validation paths, PQ signing and key handling, and on-chain cancel/weight semantics—bugs could affect liveness, memory, or fee/cancel correctness.

Overview
Hardens the vendored transaction pool against DoS and correctness bugs: caps batch admission and maintenance tree routes, bounds status/import/revalidation channels (separate view vs mempool workers, latest-wins mempool jobs), prefers the Maintained validation lane, and fixes leaks in watchers, dropped-view maps, and event metrics. Graph-layer fixes cover future dependency tracking after ready removals, priority-based future eviction, ban expiry on clone/check, and related submit_and_watch cleanup.

Node ops: pool sizing and type come from CLI (--pool-type, --pool-limit, --pool-kbytes) via Configuration::transaction_pool instead of hardcoded SingleState limits; defaults stay Quantus-oriented (SingleState, ~256 MiB / large PQ tx count).

Dilithium / CLI: adds fallible try_sign, secret zeroization, faithful HD seed from mnemonics; CLI sign/verify rejects oversized messages before signing panics.

Runtime / RPC: reversible-transfers freezes cancel authority in pending.guardian and charges recover_funds per distinct scheduler agenda bucket; fee-query RPC rejects extrinsics above block-length before SCALE decode.

^{Reviewed by Cursor Bugbot for commit 43d2761. Configure here.}

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

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

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

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

^{Reviewed by Cursor Bugbot for commit 43d2761. Configure here.}

[n13]

AI Review

Verdict: Approve — with one should-fix and a benchmark re-run before release

Reviewed all 36 files against the base sources, not just the hunks. Every remediation claimed in the description checks out as a real pre-existing bug with a correct fix and a test pinning the behavior: the base_pool false-promotion hole (future tx with a satisfied-then-removed dep), the stale unlock edges in ready.rs, the submit_local panic on .remove(0) when the mempool sync bridge dies, the dropped-watcher view-map leak, the event-metrics leak for rejected txs, the watcher-fire finality-timeout hash inconsistency, and the permanently-killed import-notification subscriber on transient overflow were all confirmed present on main. Locking discipline in the new async paths is sound (guards dropped before every await, leaf mutexes with try_send under them), the --pool-type wiring is complete end-to-end with CLI defaults byte-identical to the previously hardcoded Quantus sizing, the reversible-transfers guardian freeze is invariant-safe with no storage migration needed, and the commit factoring made this reviewable. CI is green on both matrices.

Should fix (before A/B-ing ForkAware on RPC nodes)

AddViewStream rides the drop-on-full controller lane — multi_view_listener.rs, send_controller_command. If ≥1024 non-final commands (realistically a Broadcast flood from gossip) queue between listener-task polls, a new view's AddViewStream is silently discarded: that view's stream never enters aggregated_streams_map, and every watcher misses all Ready/InBlock/Retracted events from that view for its lifetime (terminal events still arrive on the finals lane; it self-heals at the next view). AddViewStream/RemoveViewStream are intrinsically bounded (~1–2 per block) — route them over the loss-less finals lane instead. Low probability, trivial fix, and it's the one hole left in exactly the flood scenario this PR hardens against. Only reachable with --pool-type fork-aware, hence not merge-blocking for the SingleState default.

Weights: re-run the recover_funds benchmark

• The per-n proof size charges only the Agenda MEL (12,493) but the function's own storage comments declare PendingTransfers (2,640) + Lookup (2,528) + Agenda (12,493) reads and Retries (2,515) writes per n — FRAME-generated weights sum these (~20,176/n), so a full 16-transfer recovery under-declares proof size by ~120 KB. The new test pins the wrong constant (step.proof_size() == 12493).
• The per-n ref-time slope (56,328,935) is carried over from the pre-fix benchmark where up to 8 transfers shared one agenda bucket; with one bucket per transfer each cancel does strictly more agenda decode/write work. The code comment acknowledges this — please add the re-run to the test plan rather than shipping the stale slope.

Worth addressing (minors)

• rotator.rs is_banned TOCTOU: after the read lock sees an expired entry, the unconditional write().remove(hash) can delete a fresh re-ban that landed in between (e.g. from remove_invalid after failed revalidation). Consequence is wasted revalidation, not inclusion of invalid txs — re-check expiry under the write lock.
• submit_at cap uses static total capacity, not remaining, and the rejected tail is dropped even when earlier batch entries fail validation and would have left room — a near-capacity batch with an invalid prefix now loses valid tail txs the old path would have imported. Fine as a DoS tradeoff, but it's a semantics change worth a sentence in the PR body.
• Terminal event on a full per-tx channel: try_send_external_watcher_command removes the watcher controller, so the external stream ends after the consumer drains its buffer without ever delivering Finalized/Invalid. Documented in-code, but it's API-visible to RPC clients — call it out in the description.
• revalidation_worker.rs view-queue-full fallback runs revalidation inline on the maintain path — the exact long-blocking behavior this PR removes elsewhere. Awaiting queue capacity (backpressure) would preserve maintain latency; pathological-only as-is.
• Dropped-status backpressure vs dropped_watcher fallback: if a Ready registration for view B is dropped under backlog, a later Dropped from view A finds no tracked views and evicts a tx that B still holds ready. Accepted degradation, but the two sites should reference each other in comments.
• Dilithium zeroization is only as complete as the upstream crate: try_sign/from_seed/from_raw materialize an external ml_dsa_87::Keypair copy of the secret (create_keypair per sign call) that drops outside the new ZeroizeOnDrop coverage. If qp-rusty-crystals-dilithium 3.0.1 doesn't zeroize internally, every signature still leaves secret bytes in freed memory — worth verifying upstream or noting the residual gap.
• from_phrase now returns the HD-derived secret as the displayed seed. Derivation itself is unchanged (pinned by test_from_phrase_matches_hd_derivation), but previously displayed "secret seed" values for mnemonic accounts never reconstructed the real account and still won't — this deserves a release note telling users to re-derive backups from the mnemonic.
• recover_funds vs frozen guardian asymmetry: cancel authority is now frozen in pending.guardian (new test enforces NotOwner for the live guardian on pre-enrollment one-time transfers), but recover_funds still seizes those same held funds for the live guardian, fee applied. Presumably intended — recovery is seize-the-account by design — but worth an explicit comment so the cancel test isn't read as a stronger guarantee than it is.

Nits

• extend_unwatched_sync reports a dead sync bridge as Error::InvalidBlockId; Unactionable fits better.
• MAX_ENCODED_EXTRINSIC_LEN in transaction-payment-rpc hardcodes the 5 MiB operational-class block length; a governance change wouldn't propagate. Fine as a loose DoS bound.
• The hex path in message_params.rs caps the hex text, so hex-encoded messages max out at ~32 MiB vs 64 MiB raw — asymmetry worth a doc line.
• sign<P> discards P and hardcodes DilithiumPair. Safe today (with_crypto_scheme! only wires Dilithium), but if a second scheme is ever added, sign silently keeps signing with Dilithium — a static assertion tying the macro to the hardcode would future-proof it.
• view.rs droppable_events_in_flight check-then-act (load + fetch_add) can overshoot the cap by the number of concurrent senders; fetch_update would be exact.
• submit_at computes hash_and_length per accepted tx just for the length, and the hash is recomputed during validation — duplicated blake2 work, negligible at Dilithium tx sizes.
• Utility subcommands (import-blocks etc.) now get default pool options instead of the previously hardcoded Quantus sizing since their CliConfiguration impls don't override transaction_pool() — harmless (they don't exercise the pool), just noting the behavior change.

[n13]

Requesting to reverse the CLI changes

[n13]

Requesting changes

[n13]

GPT-5.6 review

Verdict: Request changes.

I reviewed current head f509d286 against main. The PR contains several valuable correctness fixes and all five CI checks are green, but the current head still has merge-blocking liveness/resource-accounting problems:

1. The biased multi-view listener loop can starve the terminal-status lane under a continuously ready view stream, allowing the supposedly lossless unbounded lane to grow and leaving watchers without finals.
2. View dependency restoration is quadratic in removed ready tags times future entries on a pool write path; the configured limits make a nine-figure scan feasible during a large subtree removal.
3. AddViewStream/RemoveViewStream are structural lifecycle commands but still use the drop-on-full lane, so one overflow can detach a view for its lifetime or retain stale state.
4. recover_funds changes the benchmark's worst-case scheduler access pattern but deliberately retains the old measured CPU slope and manually edits generated weights. Runtime weights need to be regenerated from the corrected benchmark.

Please also resolve the existing CLI genericity regression (sign<P> ignores P and hardcodes ML-DSA-87) and re-check expiry under the rotator write lock so an expired-ban cleanup cannot delete a concurrent fresh re-ban. The current per-watcher full-buffer behavior also closes the stream without delivering its terminal status; if that API degradation is intentional, it needs explicit release/API documentation and a test for a final arriving while the per-watcher queue is still full.

The current tests cover a full controller before the listener starts, but not a permanently-ready aggregated stream competing with finals, which is the starvation case above.

— Reviewed by GPT-5.6

[n13]

GPT-5.6 re-review

Verdict: Request changes — one new blocking correctness case remains.

I re-reviewed head 428d1b1e against the previously reviewed f509d286. The reported starvation, lifecycle-delivery, complexity, generated-weight, CLI-genericity, and ban-expiry race issues are materially fixed:

• the lossless lane is polled before hot view streams and now carries AddViewStream/RemoveViewStream;
• lost tags are hashed and each future transaction is scanned once;
• recover_funds uses newly generated ref-time, DB, and proof terms from the distinct-Agenda benchmark;
• the hard-coded Dilithium CLI signing change was reverted;
• is_banned re-checks expiry under the write lock.

Focused validation on this head: cargo test -p sc-transaction-pool --lib passed 91/91, and cargo test -p pallet-reversible-transfers passed 54/54. Format, config, and Clippy/doc checks are green; the two full build/test matrices are still pending at review time.

The remaining blocker is in the replacement/promotion interaction described inline: a future transaction can leave self.future before a replacement removes one of its previously satisfied requirements, so the repair scan misses it and it is admitted to Ready incompletely. A focused regression test reproduces the failure on this head.

Non-blocking housekeeping: the PR description still says this change adds try_sign and CLI message-size rejection, but those changes were reverted.

— Reviewed by GPT-5.6

[n13]

GPT-5.6 final re-review

Verdict: Approve.

Reviewed current head 021529ef against the previously reviewed 428d1b1e and against the current main merge result.

The last blocker is fixed correctly: queued promotions are now reconciled against tags lost during replacement, incomplete candidates are returned to Future, and the focused regression test covers the exact R {conflict, lost} / F requires {lost, unlock} / A {conflict, unlock} construction that failed on the prior head. The earlier starvation, lifecycle-delivery, complexity, generated-weight, CLI-genericity, and ban-expiry findings remain resolved.

Validation:

• All five GitHub checks are green, including both full build/test matrices.
• PR head: cargo test -p sc-transaction-pool --lib passed 92/92.
• Clean local merge onto current main (e362a877): transaction-pool 92/92, reversible-transfers 46/46, transaction-payment RPC 3/3, and Dilithium crypto 12/12 passed.
• The merged quantus-node native compile check passed.

No blocking findings remain.

Non-blocking housekeeping: the PR description still says the change adds try_sign and CLI message-size rejection, but commit a875c098 reverted that work.

— Reviewed by GPT-5.6