feat(platform-wallet): actively re-drive unconfirmed shielded spends by QuantumExplorer · Pull Request #3988 · dashpay/platform

QuantumExplorer · 2026-07-02T21:21:14Z

Rebased onto v4.0-dev after #3982 squash-merged. The prune backstop this PR falls back to after exhausted attempts is #3982's release pass.

Issue being fixed or feature implemented

A shielded spend whose broadcast is accepted but whose result wait fails ambiguously (ShieldedSpendUnconfirmed — the TestFlight "may have gone through, do not retry" case) previously sat frozen until its anchor aged out of Platform's ~1000-block retention window (#3982's prune backstop), and after an app restart its persisted activity row could never resolve at all.

This implements the active-resolution design: on timeout, trigger a shielded sync to see if it landed; if it didn't, re-broadcast; repeat up to 3 times; only then fall back to the prune backstop.

What was done?

Persist the transition on the ambiguous outcome. All four spend flows (unshield / transfer / withdraw / identity-create-from-pool) serialize the signed state transition and arm a PendingRedrive — new shielded_pending_spends SQLite table carrying (activity_id, anchor, nullifiers, st_bytes, attempts). On store open the records rehydrate both the redrive and the note reservations, so an unconfirmed spend keeps its notes reserved across restarts and its activity row can still flip — closing the restart gap noted in fix(platform-wallet): auto-release a stranded shielded-spend reservation on sync #3982's review.
Immediate first re-check. The FFI spend wrappers fire a fire-and-forget forced shielded sync when the ambiguous outcome surfaces, so the first nullifier re-check happens in seconds instead of at the next background tick.
Sync-time re-drive pass (runs after the scan's spent-note reconcile, before the prune-backstop release, judged against the same pre-scan recorded-anchor set): a landing was already detected by the reconcile (the mark_spent hook drops the record); otherwise, while the anchor is still recorded and attempts < 3, re-broadcast the byte-identical transition — fund-safe, identical nullifiers cannot double-spend — and classify:
- accepted / inconclusive → count the attempt, let the next scan detect the landing;
- NullifierAlreadySpent → the original executed; success signal, touch nothing (this check deliberately runs before the generic consensus-rejection arm);
- any other consensus verdict → provably dead now: release the notes and flip the activity row to Failed, hours before the prune would;
- after 3 silent attempts → the fix(platform-wallet): auto-release a stranded shielded-spend reservation on sync #3982 prune backstop owns it, unchanged.
Store-level consistency: resolving any of a redrive's nullifiers (mark_spent / clear_pending) drops the whole record durably — a transition lands or dies atomically for all its nullifiers.

How Has This Been Tested?

cargo test -p platform-wallet --lib --features shielded → 320 passed (316 base + 4 new).
New coverage: SQLite roundtrip + rehydration (record, attempt counter, and reservations survive reopen; resolution deletions are durable); redrive decision paths against a mock SDK with no broadcast expectation — proving pruned-anchor and exhausted records are left untouched and a corrupt record is dropped, all without touching the network; NullifierAlreadySpent classification incl. the ordering constraint vs carries_consensus_rejection.
cargo fmt --all -- --check and cargo clippy -p platform-wallet --features shielded clean; platform-wallet-ffi and default-features checks pass.

Not covered (network-gated): a live ambiguous timeout → re-broadcast → landing on testnet; best confirmed on a device once this ships in a build.

Breaking Changes

None externally. ShieldedStore gains four required methods (arm_redrive / pending_redrives / bump_redrive_attempts / clear_redrive); both in-tree implementations updated. New SQLite table is additive (pre-release; no migration concerns).

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
I have made corresponding changes to the documentation if needed

🤖 Generated with Claude Code

coderabbitai · 2026-07-02T21:21:21Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5a153d41-2937-471b-a19b-0825e83bbf1a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feat/shielded-spend-redrive

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

thepastaclaw · 2026-07-02T21:24:05Z

✅ Review complete (commit 997d9e7)

A broadcast-accepted spend whose result wait fails ambiguously (ShieldedSpendUnconfirmed) previously sat frozen until its anchor aged out of Platform's ~1000-block retention window. Resolve the ambiguity actively instead: - On the ambiguous outcome, all four spend flows persist a PendingRedrive (new shielded_pending_spends SQLite table): the signed transition bytes, anchor, nullifiers, activity id, attempt counter. Rehydrated on store open — the note reservations and the re-drive survive restarts, which also closes the restart gap where a never-landing spend's activity row stayed Pending forever. - The FFI spend wrappers kick an immediate forced shielded sync on the ambiguous outcome, so the first re-check happens in seconds. - Each sync pass, after the spent-note reconcile (a landing drops the record via the mark_spent hook): while the anchor is still recorded and attempts < 3, re-broadcast the byte-identical transition (fund-safe — identical nullifiers cannot double-spend) and classify: accepted/inconclusive counts an attempt; NullifierAlreadySpent means the original executed (next scan confirms — touch nothing); any other consensus verdict is provably dead NOW: release the notes and flip the activity row to Failed hours before the prune would. - After 3 silent attempts the existing anchor-prune release backstop owns the reservation, unchanged. platform-wallet shielded suite: 320 passed. New coverage: store roundtrip + rehydration + durable resolution, redrive decision paths (pruned/exhausted/corrupt) against a mock SDK proving no network touch, NullifierAlreadySpent classification ordering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

thepastaclaw

Code Review

Sound active-redrive lifecycle with fund-safe byte-identical rebroadcast, correct NullifierAlreadySpent-first classification ordering, and coherent SQLite/rehydrate persistence. The main defect is the new FFI-triggered poke_sync_on_unconfirmed bypasses the ShieldedSyncManager is_syncing/quiescing gate that the rest of the codebase relies on as a hard invariant, allowing concurrent sync passes to race the periodic loop and undermine quiesce()'s drain barrier. Secondary concerns cluster around SQLite consistency in file_store.rs (memory-before-persist ordering in bump_redrive_attempts, missing busy_timeout on the second writer connection, ?-abort desync in mark_spent/clear_pending), plus an unbounded NullifierAlreadySpent success arm and a network-blocking persisters read-lock scope.

Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general, failed_or_unparseable), opus (rust-quality), claude-sonnet-5 (rust-quality), gpt-5.5[high] (rust-quality, failed_or_unparseable), opus (ffi-engineer), claude-sonnet-5 (ffi-engineer), gpt-5.5[high] (ffi-engineer, failed_or_unparseable); verifier: opus; specialists: rust-quality, ffi-engineer

🔴 1 blocking | 🟡 6 suggestion(s) | 💬 2 nitpick(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/shielded_send.rs:441-457: poke_sync_on_unconfirmed bypasses the manager's is_syncing/quiescing gate
  `poke_sync_on_unconfirmed` spawns `coordinator.sync(true).await` directly against the same `Arc<NetworkShieldedCoordinator>` used by the periodic loop and `sync_wallet`/`sync_now`. Every other in-tree caller routes through `ShieldedSyncManager::sync_now` (packages/rs-platform-wallet/src/manager/shielded_sync.rs:336-395), which CAS-guards on `is_syncing` and checks `quiescing` before invoking `coordinator.sync()`. `NetworkShieldedCoordinator::sync()` itself (coordinator.rs:557) has no internal reentrancy guard — the `is_syncing` atomic is the only serializer — and `force=true` also skips the cooldown check.

  Two concrete consequences: (1) a background pass already in flight when a spend goes ambiguous will run a second, fully concurrent `sync()` on the same store; both take the same pre-scan `stale_pending_spends`+`recorded` snapshot and both call `redrive_pending_spends` on the same subwallet, letting each rebroadcast the stored transition and independently bump `attempts`, so one ambiguous event can burn 2 of the 3 redrive attempts in a single race. (2) It breaks the documented invariant of `WalletManager::quiesce()` (shielded_sync.rs:299-323): because this spawn never sets `is_syncing`, the `while self.is_syncing.load(...) {}` drain loop will not wait for it, so a Clear/unregister/rebind that follows `quiesce()` can still race a resolving sync pass triggered by this poke. Fund-safety is preserved (identical nullifiers can't double-spend and the duplicate hits `NullifierAlreadySpent`), but the retry-budget intent and the teardown barrier are not. Route this through the manager's gated `sync_now`, or add an in-flight mutex/atomic inside `NetworkShieldedCoordinator::sync` now that it has two independent callers.

In `packages/rs-platform-wallet/src/wallet/shielded/file_store.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:377-401: bump_redrive_attempts mutates memory before persisting, so a SQLite failure diverges the two views
  `FileBackedShieldedStore::bump_redrive_attempts` calls `sw.bump_redrive_attempts(activity_id)` — which unconditionally increments in-memory `SubwalletState::redrives[activity_id].attempts` — before issuing the `UPDATE shielded_pending_spends SET attempts = ?4`. If the UPDATE fails the function returns `Err`, but the in-memory counter has already advanced. On the next reopen `rehydrate_pending_spends` rebuilds from the stale SQLite value, so the visible attempt count silently rewinds. The caller in `redrive_pending_spends` (operations.rs:2156-2160 and 2203-2207) additionally does `.unwrap_or(0)`, folding the persistence failure into an `attempts=0` log line and continuing. `arm_redrive` already orders SQLite before memory — mirror that here (read/UPDATE against SQLite first, then mirror the resulting count to memory) so the two views cannot diverge across restarts.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:105-220: Second SQLite connection on the same file lacks a busy_timeout
  The PR opens a second `rusqlite::Connection` (`pending_conn`) via `open_tuned_connection` against the same file as `tree`. Under WAL, readers do not block writers, but two writer connections still serialize on the write lock and a concurrent writer receives `SQLITE_BUSY` immediately unless `busy_timeout` (or `PRAGMA busy_timeout=...`) is configured. `open_tuned_connection` sets `journal_mode=WAL`, `synchronous=NORMAL`, `temp_store=MEMORY`, but installs no busy handler. On iOS under concurrent activity (a `mark_spent`/tree write while `arm_redrive` or `bump_redrive_attempts` writes to `pending_conn`) an unlucky ordering surfaces as `FileShieldedStoreError("persist redrive: database is locked")`, either propagating up as a hard `arm_redrive` failure (silently disabling redrive persistence for that spend so only the prune backstop rescues it) or aborting the operation entirely. Set a modest `busy_timeout` (a few seconds) on both connections so SQLite retries transparently.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:288-322: delete_redrive_rows_containing error can abort mark_spent/clear_pending after the in-memory drop already happened
  Both `mark_spent` (line 296) and `clear_pending` (line 319) call `self.delete_redrive_rows_containing(id, nullifier)?` after the in-memory `SubwalletState::mark_spent`/`clear_pending` mutation already ran and already dropped the in-memory `redrives` entry. If the SQL delete fails (WAL contention between the two connections on the same file, disk error), the function returns `Err` even though the in-memory state has already changed — the persisted `shielded_pending_spends` row is left behind, desynced from memory.

  Concretely: `mark_spent` is called from `apply_scanned_nullifier_spends` as `store.mark_spent(*id, nf)?`, and this `?` now aborts detection of all remaining scanned nullifier spends across every subwallet for the pass — a new failure mode, since pre-PR `mark_spent` only touched the in-memory map and could not fail in practice. On `clear_pending` SQL failure the in-memory reservation is already freed, but the stale SQLite redrive row survives; on restart `rehydrate_pending_spends` resurrects it and re-reserves the (legitimately released) nullifier, locking the note back up until a later pass notices. Perform the SQL delete before mutating memory, or catch the persistence error and log without propagating so memory and SQLite cannot desync mid-call.

In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:2167-2172: NullifierAlreadySpent arm does not bound retries or clear the record
  The `Err(e) if is_nullifier_already_spent(&e)` arm in `redrive_pending_spends` only logs — it does not `bump_redrive_attempts` and does not `clear_redrive`. The comment ("the next scan confirms") assumes the following scan will land the nullifier via `apply_scanned_nullifier_spends` → `mark_spent`, which drops the record. That is the common path, but the arm is otherwise unbounded: if the scan repeatedly fails to cover the block that landed the nullifier (transport hiccup during a chunk, a subwallet whose store lost the note, delivery lag past cooldowns), every subsequent sync will re-broadcast the same transition and receive `NullifierAlreadySpent` again indefinitely. Since this arm is already a definitive success signal (`NullifierAlreadySpent` proves the transition executed), the record could safely be cleared here — leaving the reservation in place for the next scan's `mark_spent` to release. At minimum, count the attempt against `MAX_REDRIVE_ATTEMPTS` so the retry loop is bounded even in the pathological scan-gap case.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:2156-2207: bump_redrive_attempts persistence errors silently discarded at both call sites
  Both `bump_redrive_attempts` invocations in `redrive_pending_spends` (the `Ok(())` arm at 2156-2160 and the inconclusive-error arm at 2203-2207) do `.unwrap_or(0)` on the Result, swallowing any store-persistence error entirely — no `warn!`/`debug!` is logged. Every other store-call failure in the same function (`pending_redrives`, `clear_redrive`, `clear_pending`) logs a `warn!` on error; these two are the odd ones out. Combined with the memory-before-SQLite ordering in the file store, this converts a SQLite failure into a silent stale-count on restart with no diagnostic trail. Log the error the same way the sibling arms do.

In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:657-673: persisters read lock held across the entire network-calling redrive loop
  The new redrive block acquires `self.persisters.read().await` and holds it for the full `for (id, _) in &subwallets` loop, and each iteration awaits `redrive_pending_spends`, which performs up to `MAX_REDRIVE_ATTEMPTS` real `st.broadcast(sdk, None).await` round-trips per subwallet. A `tokio::sync::RwLock` read guard blocks any writer for its full lifetime, so `register_wallet`/`unregister_wallet`/`clear` (all of which take `persisters.write().await`) will stall for the duration of this pass any time a host tries to add/remove a wallet while a redrive-heavy sync is running. The pre-existing `release_stranded_spends` uses the same locking pattern but only does local store work; extending that pattern across network I/O changes the assumption. Snapshot the needed `WalletPersister` clones under the lock, drop the guard, then run the network-calling loop.

thepastaclaw

Code Review

Pure rebase onto v4.0-dev — the five shielded files under review (shielded_send.rs, coordinator.rs, file_store.rs, operations.rs, store.rs) are byte-identical to prior reviewed SHA da6655b (git diff confirms zero changes). None of the 9 prior findings have been addressed; all are carried forward as STILL VALID. The blocking concern remains that poke_sync_on_unconfirmed bypasses the ShieldedSyncManager is_syncing/quiescing gate that every other in-tree sync trigger relies on.

Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general, failed_or_unparseable), opus (rust-quality), claude-sonnet-5 (rust-quality), gpt-5.5[high] (rust-quality, failed_or_unparseable), opus (ffi-engineer), claude-sonnet-5 (ffi-engineer), gpt-5.5[high] (ffi-engineer, failed_or_unparseable); verifier: opus; specialists: rust-quality, ffi-engineer

🔴 1 blocking | 🟡 6 suggestion(s) | 💬 2 nitpick(s)

Carried-forward findings already raised (9)

These findings were not re-posted as new inline comments because an existing review thread already covers them.

[BLOCKING] (deduped existing open thread) packages/rs-platform-wallet-ffi/src/shielded_send.rs:441-457: poke_sync_on_unconfirmed bypasses the manager's is_syncing/quiescing gate — Carried forward from prior review (prior-1); verified byte-identical at head 5a9c704. poke_sync_on_unconfirmed spawns coordinator.sync(true).await directly against the shared Arc<NetworkShieldedCoordinator>, bypassing ShieldedSyncManager::sync_now (packages/rs-platform-wallet/src/manag...
[SUGGESTION] (deduped existing open thread) packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:377-402: bump_redrive_attempts mutates memory before persisting, so a SQLite failure diverges the two views — Carried forward from prior review (prior-2); code unchanged at head 5a9c704. FileBackedShieldedStore::bump_redrive_attempts calls sw.bump_redrive_attempts(activity_id) — which unconditionally increments the in-memory SubwalletState::redrives[activity_id].attempts — before issuing `UPDATE...
[SUGGESTION] (deduped existing open thread) packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:205-220: Second SQLite connection on the same file lacks a busy_timeout — Carried forward from prior review (prior-3); code unchanged at head. The PR opens a second rusqlite::Connection (pending_conn) via open_tuned_connection against the same file as tree. open_tuned_connection sets journal_mode=WAL, synchronous=NORMAL, temp_store=MEMORY, but installs...
[SUGGESTION] (deduped existing open thread) packages/rs-platform-wallet/src/wallet/shielded/operations.rs:2167-2172: NullifierAlreadySpent arm does not bound retries or clear the record — Carried forward from prior review (prior-4); code unchanged at head. The Err(e) if is_nullifier_already_spent(&e) arm in redrive_pending_spends only logs — it does not bump_redrive_attempts and does not clear_redrive. The comment ("the next scan confirms") assumes the following scan will...
[SUGGESTION] (deduped existing open thread) packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:288-322: delete_redrive_rows_containing error can abort mark_spent/clear_pending after the in-memory drop already happened — Carried forward from prior review (prior-5); code unchanged at head. Both mark_spent (line 296) and clear_pending (line 319) call self.delete_redrive_rows_containing(id, nullifier)? after the in-memory SubwalletState::mark_spent/clear_pending mutation already ran and already dropped the...
[SUGGESTION] (deduped existing open thread) packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:657-673: persisters read lock held across the entire network-calling redrive loop — Carried forward from prior review (prior-6); code unchanged at head. The new redrive block acquires self.persisters.read().await and holds it for the full for (id, _) in &subwallets loop, and each iteration awaits redrive_pending_spends, which performs up to MAX_REDRIVE_ATTEMPTS real `st....
[SUGGESTION] (deduped existing open thread) packages/rs-platform-wallet/src/wallet/shielded/operations.rs:2156-2207: bump_redrive_attempts persistence errors silently discarded at both call sites — Carried forward from prior review (prior-7); code unchanged at head. Both bump_redrive_attempts invocations in redrive_pending_spends (the Ok(()) arm at 2156-2160 and the inconclusive-error arm at 2203-2207) do .unwrap_or(0) on the Result, swallowing any store-persistence error entirely w...
[NITPICK] (deduped existing open thread) packages/rs-platform-wallet-ffi/src/shielded_send.rs:454-456: poke_sync_on_unconfirmed silently discards the forced-sync result — Carried forward from prior review (prior-8); code unchanged at head. let _ = coordinator.sync(true).await; throws away both errors and any panic captured in the (immediately dropped) JoinHandle. Discarding the typed spend result is fine — the ambiguity is already decided — but discarding the...
[NITPICK] (deduped existing open thread) packages/rs-platform-wallet/src/wallet/shielded/operations.rs:2114-2216: redrive_pending_spends' actual broadcast-response classification is untested — Carried forward from prior review (prior-9); code unchanged at head. The only test exercising redrive_pending_spends (redrive_skips_pruned_and_exhausted_and_drops_garbage) deliberately never reaches st.broadcast(sdk, None).await — its three records are pruned-anchor, exhausted-attempts, and...

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/shielded_send.rs:441-457: poke_sync_on_unconfirmed bypasses the manager's is_syncing/quiescing gate
  Carried forward from prior review (prior-1); verified byte-identical at head 5a9c704358. `poke_sync_on_unconfirmed` spawns `coordinator.sync(true).await` directly against the shared `Arc<NetworkShieldedCoordinator>`, bypassing `ShieldedSyncManager::sync_now` (packages/rs-platform-wallet/src/manager/shielded_sync.rs:336-395) — the only place that CAS-guards on `is_syncing` and checks `quiescing`. `NetworkShieldedCoordinator::sync()` has no internal reentrancy guard, and `force=true` also skips the cooldown check.

  Two consequences:
  1. A background pass already in flight when a spend goes ambiguous will run a second, fully concurrent `sync()` against the same store; both take the same pre-scan `stale_pending_spends`+`recorded` snapshot and both call `redrive_pending_spends` on the same subwallet. One ambiguous event can burn 2 of the 3 redrive attempts in a single race, and independently bump `attempts` twice.
  2. It breaks `WalletManager::quiesce()`'s drain invariant (shielded_sync.rs:299-323): since this spawned task never sets `is_syncing`, the `while self.is_syncing.load(...) {}` drain loop will not wait for it. A Clear/unregister/rebind following `quiesce()` can still race a resolving sync pass triggered by this poke — holding coordinator persisters/store RwLocks after the host was told teardown was safe.

  Also worth noting: because `on_shielded_sync_completed` is dispatched by the manager's `sync_now` (shielded_sync.rs:393), not by `coordinator.sync`, this manually-triggered pass never emits the completion event that hosts (Swift `handleShieldedSyncCompleted`) rely on to reflect the resolved re-check.

  Fix: route the FFI poke through a `manager.poke_sync_now(true)` helper that runs `sync_now(true)` under the same CAS + quiescing check, or add an in-flight guard inside `NetworkShieldedCoordinator::sync` now that it has two independent callers. Fund-safety is preserved (duplicate nullifiers hit `NullifierAlreadySpent`) but the retry-budget intent and the teardown barrier are not.

In `packages/rs-platform-wallet/src/wallet/shielded/file_store.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:377-402: bump_redrive_attempts mutates memory before persisting, so a SQLite failure diverges the two views
  Carried forward from prior review (prior-2); code unchanged at head 5a9c704358. `FileBackedShieldedStore::bump_redrive_attempts` calls `sw.bump_redrive_attempts(activity_id)` — which unconditionally increments the in-memory `SubwalletState::redrives[activity_id].attempts` — before issuing `UPDATE shielded_pending_spends SET attempts = ?4`. If the UPDATE fails the function returns `Err`, but the in-memory counter has already advanced. On next reopen `rehydrate_pending_spends` rebuilds from the stale SQLite value, silently rewinding the visible attempt count.

  Compounded by the caller in `redrive_pending_spends` (operations.rs:2156-2160 and 2203-2207) doing `.unwrap_or(0)`, which folds the persistence failure into an `attempts=0` log line and continues. `arm_redrive` already orders SQLite before memory — mirror that here (UPDATE against SQLite first, then mirror the resulting count to memory) so the two views cannot diverge across restarts.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:205-220: Second SQLite connection on the same file lacks a busy_timeout
  Carried forward from prior review (prior-3); code unchanged at head. The PR opens a second `rusqlite::Connection` (`pending_conn`) via `open_tuned_connection` against the same file as `tree`. `open_tuned_connection` sets `journal_mode=WAL`, `synchronous=NORMAL`, `temp_store=MEMORY`, but installs no `busy_timeout`. Under WAL, readers do not block writers, but two writer connections still serialize on the write lock and a concurrent writer receives `SQLITE_BUSY` immediately without a busy handler.

  On iOS under concurrent activity — a `mark_spent`/tree write racing an `arm_redrive` or `bump_redrive_attempts` write on `pending_conn` — this surfaces as `FileShieldedStoreError("persist redrive: database is locked")`, either propagating up as a hard `arm_redrive` failure (silently disabling redrive persistence for that spend so only the prune backstop rescues it) or aborting the operation entirely. Set a modest `busy_timeout` (a few seconds) on both connections so SQLite retries transparently.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:288-322: delete_redrive_rows_containing error can abort mark_spent/clear_pending after the in-memory drop already happened
  Carried forward from prior review (prior-5); code unchanged at head. Both `mark_spent` (line 296) and `clear_pending` (line 319) call `self.delete_redrive_rows_containing(id, nullifier)?` after the in-memory `SubwalletState::mark_spent`/`clear_pending` mutation already ran and already dropped the in-memory `redrives` entry. If the SQL delete fails (WAL contention between the two connections on the same file, disk error), the function returns `Err` even though the in-memory state has already changed — the persisted `shielded_pending_spends` row is left behind, desynced from memory.

  `mark_spent` is called from `apply_scanned_nullifier_spends` as `store.mark_spent(*id, nf)?`, so this `?` now aborts detection of all remaining scanned nullifier spends across every subwallet for the pass — a new failure mode, since pre-PR `mark_spent` only touched the in-memory map and could not fail. On `clear_pending` SQL failure the in-memory reservation is already freed but the stale SQLite redrive row survives; on restart `rehydrate_pending_spends` resurrects it and re-reserves the (legitimately released) nullifier, locking the note back up until a later pass notices. Perform the SQL delete before mutating memory, or catch the persistence error and log without propagating.

In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:2167-2172: NullifierAlreadySpent arm does not bound retries or clear the record
  Carried forward from prior review (prior-4); code unchanged at head. The `Err(e) if is_nullifier_already_spent(&e)` arm in `redrive_pending_spends` only logs — it does not `bump_redrive_attempts` and does not `clear_redrive`. The comment ("the next scan confirms") assumes the following scan will land the nullifier via `apply_scanned_nullifier_spends` → `mark_spent`, which drops the record.

  That is the common path but the arm is otherwise unbounded: if the scan repeatedly fails to cover the block that landed the nullifier (transport hiccup during a chunk, a subwallet whose store lost the note, delivery lag past cooldowns), every subsequent sync re-broadcasts and receives `NullifierAlreadySpent` again indefinitely. Since this arm is already a definitive success signal (`NullifierAlreadySpent` proves the transition executed), clear the record here — leaving the reservation in place for the next scan's `mark_spent` to release. At minimum, count the attempt against `MAX_REDRIVE_ATTEMPTS` so the retry loop is bounded even in the pathological scan-gap case.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:2156-2207: bump_redrive_attempts persistence errors silently discarded at both call sites
  Carried forward from prior review (prior-7); code unchanged at head. Both `bump_redrive_attempts` invocations in `redrive_pending_spends` (the `Ok(())` arm at 2156-2160 and the inconclusive-error arm at 2203-2207) do `.unwrap_or(0)` on the Result, swallowing any store-persistence error entirely with no `warn!`/`debug!`. Every other store-call failure in the same function (`pending_redrives`, `clear_redrive`, `clear_pending`) logs a `warn!` on error; these two are the odd ones out. Combined with the memory-before-SQLite ordering in the file store (prior-2), this converts a SQLite failure into a silent stale-count on restart with no diagnostic trail.

In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:657-673: persisters read lock held across the entire network-calling redrive loop
  Carried forward from prior review (prior-6); code unchanged at head. The new redrive block acquires `self.persisters.read().await` and holds it for the full `for (id, _) in &subwallets` loop, and each iteration awaits `redrive_pending_spends`, which performs up to `MAX_REDRIVE_ATTEMPTS` real `st.broadcast(sdk, None).await` round-trips per subwallet. A `tokio::sync::RwLock` read guard blocks any writer for its full lifetime, so `register_wallet`/`unregister_wallet`/`clear` (all needing `persisters.write().await`) stall for the duration of this pass any time a host adds/removes a wallet while a redrive-heavy sync is running.

  The pre-existing `release_stranded_spends` uses the same locking pattern but only does local store work; extending that pattern across network I/O changes the assumption. Snapshot the needed `WalletPersister` clones under the lock, drop the guard, then run the network-calling loop.

- Route the post-unconfirmed sync poke through ShieldedSyncManager::sync_now instead of coordinator.sync directly, so it respects the manager's is_syncing CAS + quiescing drain barrier (force only bypasses the caught-up cooldown, never serialization); an in-flight pass makes the poke a no-op and the next tick redrives. The pass outcome is logged rather than discarded. - bump_redrive_attempts persists FIRST and mutates memory only on success, so a SQLite failure can no longer diverge the two views. - busy_timeout(5s) on the store's tuned connections: two writer connections share the file (tree + pending), and WAL rejects a colliding write immediately without one. - The NullifierAlreadySpent success arm now consumes an attempt so a lagging scan can't induce unbounded per-pass re-broadcasts. - mark_spent / clear_pending log-and-continue when mirroring redrive row deletions to SQLite (the in-memory transition already happened; a stale row rehydrates and self-heals via reconcile/prune). - Per-wallet persisters are snapshotted before the redrive loop so the read lock is not held across network broadcasts. - Attempt-counter persistence failures are logged (the attempt slot is retried next pass), not silently zeroed. - The broadcast-outcome classification is extracted into the pure classify_redrive_broadcast with a matrix test pinning the arm order (AlreadyExecuted must beat the generic consensus-rejection arm). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

thepastaclaw

Code Review

Delta commit 997d9e7 ("address review findings on the re-drive pass") resolves all 9 prior findings from 5a9c704: the blocking poke_sync_on_unconfirmed bypass now routes through ShieldedSyncManager::sync_now (respecting the is_syncing CAS + quiescing drain barrier), bump_redrive_attempts is now persist-first, busy_timeout(5s) is installed on the shared SQLite writer, the AlreadyExecuted arm now consumes an attempt (bounding retries), mark_spent/clear_pending log-and-continue on SQL delete failures, the persisters read guard is snapshotted before the network-calling redrive loop, a new bump_redrive_attempts_logged helper logs persistence errors, the poke's summary is inspected/logged, and a new redrive_broadcast_classification_matrix test pins the AlreadyExecuted-before-DefinitiveRejection arm order. The only new observation is a cosmetic whitespace/log-wording nitpick in the poke's own debug line.

Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general, failed_or_unparseable), opus (rust-quality), claude-sonnet-5 (rust-quality), gpt-5.5[high] (rust-quality, failed_or_unparseable), opus (ffi-engineer), claude-sonnet-5 (ffi-engineer), gpt-5.5[high] (ffi-engineer, failed_or_unparseable); verifier: opus; specialists: rust-quality, ffi-engineer

💬 1 nitpick(s)

thepastaclaw · 2026-07-03T04:07:00Z

+        if summary.sync_unix_seconds == 0 {
+            tracing::debug!(
+                "post-unconfirmed shielded sync poke skipped (a pass was already in flight                  or shielded is unconfigured); the next pass owns the re-drive"
+            );
+        } else {
+            tracing::debug!(
+                wallets = summary.wallet_results.len(),
+                "post-unconfirmed shielded sync pass completed"
+            );
+        }


💬 Nitpick: Stray run of spaces in the poke-skipped debug log, and the message slightly mischaracterizes the sync_unix_seconds == 0 branch

Two small issues in the new poke_sync_on_unconfirmed debug log:

Line 472 has ~18 literal spaces baked into the string between "in flight" and "or shielded is unconfigured" — an editor-unwrap artifact that rustfmt won't catch inside a string literal. It will appear verbatim in operator logs and become a grep landmine.

The message says sync_unix_seconds == 0 covers both "a pass was already in flight" and "shielded is unconfigured". Looking at ShieldedSyncManager::sync_now (packages/rs-platform-wallet/src/manager/shielded_sync.rs:336-398): only the CAS-lost and quiescing short-circuits leave sync_unix_seconds == 0. The no-coordinator path returns a default summary and then falls through to the summary.sync_unix_seconds = now stamp + completion-event dispatch, so "unconfigured" actually lands in the "...pass completed" branch (with wallets = 0). The unconfigured case is also practically unreachable here since the spend that produced the ambiguous result required a configured coordinator. Purely cosmetic — no runtime impact.

Suggested change

if summary.sync_unix_seconds == 0 {

tracing::debug!(

"post-unconfirmed shielded sync poke skipped (a pass was already in flight or shielded is unconfigured); the next pass owns the re-drive"

);

} else {

tracing::debug!(

wallets = summary.wallet_results.len(),

"post-unconfirmed shielded sync pass completed"

);

}

tracing::debug!(

"post-unconfirmed shielded sync poke skipped (a pass was already in flight or shielded is unconfigured); the next pass owns the re-drive"

);

_{source: ['claude']}

QuantumExplorer requested review from ZocoLini, lklimek, llbartekll and shumkov as code owners July 2, 2026 21:21

Base automatically changed from fix/shielded-spend-reservation-auto-release to v4.0-dev July 2, 2026 21:39

github-actions Bot added this to the v4.0.0 milestone Jul 2, 2026

QuantumExplorer force-pushed the feat/shielded-spend-redrive branch from da6655b to 5a9c704 Compare July 2, 2026 21:55

thepastaclaw suggested changes Jul 2, 2026

View reviewed changes

thepastaclaw approved these changes Jul 3, 2026

View reviewed changes

QuantumExplorer merged commit 9431319 into v4.0-dev Jul 3, 2026
16 of 17 checks passed

QuantumExplorer deleted the feat/shielded-spend-redrive branch July 3, 2026 04:45

This was referenced Jul 3, 2026

chore: sync v4.1-dev with v4.0-dev #3990

Merged

fix(platform-wallet): data-integrity follow-ups from the #3990 sync review #4008

Merged

Claudius-Maginificent mentioned this pull request Jul 16, 2026

fix(rs-dapi,rs-sdk): generalize retryable post-broadcast result-wait errors #4137

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(platform-wallet): actively re-drive unconfirmed shielded spends#3988

feat(platform-wallet): actively re-drive unconfirmed shielded spends#3988
QuantumExplorer merged 2 commits into
v4.0-devfrom
feat/shielded-spend-redrive

QuantumExplorer commented Jul 2, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 2, 2026 •

edited

Loading

Review skipped

Uh oh!

thepastaclaw commented Jul 2, 2026 •

edited

Loading

Uh oh!

thepastaclaw left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thepastaclaw left a comment

Uh oh!

thepastaclaw left a comment

Uh oh!

thepastaclaw Jul 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

QuantumExplorer commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

coderabbitai Bot commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

thepastaclaw commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

thepastaclaw Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

QuantumExplorer commented Jul 2, 2026 •

edited

Loading

coderabbitai Bot commented Jul 2, 2026 •

edited

Loading

thepastaclaw commented Jul 2, 2026 •

edited

Loading