Repository navigation
fix(platform-wallet): let a shielded sync pass observe cancellation - #5733
llbartekll wants to merge 5 commits into
Conversation
A shielded sync pass had no cancellation point: only the loop's sleep between passes watched the registry token, so `stop()` returned at once but `quiesce()` waited for the whole pass (10 s, then code 27) and `destroy` for up to 37 s on measured iOS teardowns. The manager now owns a pass-cancellation token that `stop()` cancels and rotates, so passes dispatched from host threads (`sync_now(true)`, `sync_wallet`) are cancellable too. The coordinator checks it before every network step, and the note scan selects on it between batches. A cancelled scan drops its in-flight fetches and then finalizes the prefix it consumed exactly as a completed pass would (checkpoint, note saves, spend replay, watermark) and flushes that to the persisters, so the store and the host mirror stay consistent; `quiesce` keeps its barrier by waiting for that finalize. The caught-up cooldown is not stamped for a cancelled pass. Closes #5200. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…d nothing `build_per_wallet_summary` mints an all-zero `Ok` for every registered wallet, so the manager's `is_empty()` check could not tell a pass cancelled before its scan from a completed one and still stamped the timestamp and emitted a completion event. The coordinator now hands back the empty summary when a cancelled pass consumed nothing, and the stranded-release prefetch (a read-only RPC) is selected against the token so a stop that lands during it does not wait the request out. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ed pass A cancelled pass finalizes `aligned_start + scanned`, and `aligned_start` is the union MIN watermark, so a subwallet that was already further along could be written backwards. Finalize `max(prior, new)` and leave an uncovered subwallet's row alone on a cancelled pass; a completed pass keeps the unconditional write (its `ScannedThisSession` stamp feeds the restore gate). A pass cancelled before its first batch now records nothing, so the coordinator's empty-pass gate fires. Take the pass token before the `is_syncing` CAS in both entry points: a `stop()` between the CAS and `pass_token()` rotated the parent and handed the admitted pass an uncancelled child. Check cancellation ahead of the cooldown fast path, whose skip summary reported one caught-up wallet for a pass that never ran. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…shielded-sync-pass # Conflicts: # packages/rs-platform-wallet/src/manager/shielded_sync.rs
…shielded-sync-pass
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
|
⛔ Final review complete — 3 blocking finding(s) (commit c4ca8f1) · triage: normal |
|
Correctness: a cancelled partial scan can leave permanently incomplete activity history A non-empty cancelled pass still calls I reproduced this using the actual
This makes restored history depend on cancellation timing. The raw notes can remain correct while the displayed history permanently omits part of the value; an ordinary full rescan retaining the existing activity rows does not repair it. This reproducer exercises the classifier, not the full SDK/network flow. A scoped correction would be to defer creation of scan-derived activity entries until an uncancelled scan, while still finalizing and persisting the cancelled prefix's raw notes, spent marks and cursors. Please add a cancel/resume regression with owned notes split across batches sharing a proof height, and compare the resulting history with an uninterrupted scan. The existing cancellation stream tests use |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The cancellation ownership and ordered-prefix mechanics are sound, but partial finalization can incorrectly establish complete-balance provenance and permanently omit restored activity. Post-scan retry helpers also continue initiating network requests after cancellation, and the new quiesce test contains a scheduling race. Verification was static at the exact requested head; no builds or tests were run, and the current Kotlin SDK build/test check passes.
🔴 3 blocking | 🟡 3 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The cancellation and partial-scan finalization changes are intricate, but they do not themselves change consensus rules, funds movement or coin selection, cryptography, key handling, peer-facing deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 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/src/wallet/shielded/sync.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/sync.rs:783-790: Do not mark a cancelled prefix as a complete balance scan
When a cancelled pass advances an account, it calls `set_last_synced_note_index`. Both store implementations also set `last_scanned_index` and `balance_source = ScannedThisSession`, whose contract requires a complete scan. This changes availability, not just metadata: `local_balance_snapshot` accepts that source as sufficient evidence to return `Ready` for an otherwise unhydrated account. A newly registered account can therefore consume a prefix containing no owned notes, cancel before its receipts, and expose a ready zero balance. The emitted partial watermark is also interpreted as completed-scan evidence during restoration. Separate resumable progress from completed-scan provenance in the Rust store and persistence contract. Preserve note deltas and resume indices on cancellation without certifying complete coverage, and test the local snapshot both immediately after cancellation and after restoration.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1423-1431: Defer immutable activity derivation until the scan completes
Continuing all store-only processing after a partial scan also runs `derive_activity_into_changeset` unconditionally at lines 1511–1513. That derivation groups notes by their discovering proof height and skips an entire cluster if any commitment overlaps an existing activity row. On a cold restore, cancel after discovering receipt A, then resume and discover receipt B proven at the same height. The cancelled pass saves an activity row for A; the resumed cluster contains A and B, overlaps A's row, and produces only a confirmation for that existing row. B never receives an activity entry, and A's amount and commitment set are not expanded on later passes. Keep flushing raw notes, spent marks, and resume indices, but defer creation of immutable scan-derived history until completion or explicitly support extending incomplete clusters. Add a cancel/resume regression with owned receipts sharing a proof height.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1467-1470: Propagate cancellation through the post-scan retry helpers
The spend-redrive gate uses a cancellation snapshot taken immediately after scanning. If stop arrives afterward, the subwallet loop and `redrive_pending_spends` continue through every pending record without observing it. The identity-shield path checks between wallets, but its helper cannot cancel the read-only nonce fetch or observe cancellation before the subsequent broadcast and remaining records. This is not merely waiting for one already-submitted broadcast: new requests continue to start after stop, potentially consuming multiple default 10-second timeouts and retries and exhausting the quiesce budget. Thread the pass token through both retry helpers, check it between records and before each new request, and make read-only waits cancellable. Preserve conservative reservation and ambiguity handling for broadcasts already submitted, and still perform the mandatory changeset flush.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1438-1444: Exercise coordinator finalization after mid-scan cancellation
Both new coordinator cancellation tests cancel before entering the scan, so they return at the initial cancellation check. The synthetic stream tests invoke only the scan helper. Consequently, no test exercises the coordinator's partial-pass summary, persister fan-out, skipped redrive/release work, or cooldown handling after consumed batches. Add a coordinator-level mid-scan cancellation test that asserts the consumed changeset reaches the wallet persister, the summary reports useful partial work, no post-scan network request starts, and the cancelled pass does not establish a caught-up cooldown.
In `packages/rs-platform-wallet/src/manager/shielded_sync.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/shielded_sync.rs:796-798: Capture the test pass token before publishing the occupied slot
This multi-threaded test publishes `is_syncing = true` before obtaining its pass token. The parent can observe the occupied slot and call `quiesce()` while the spawned task is preempted between those statements. Stop then cancels and rotates the parent token; the spawned task subsequently obtains the fresh, uncancelled lineage and waits indefinitely, causing the two-second assertion to fail under a valid scheduling interleaving. Capture the token before claiming the slot, matching the production entry points.
In `packages/rs-platform-wallet/src/wallet/shielded/sync/cancel_tests.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/sync/cancel_tests.rs:43-71: Test cancellation with owned notes and scan-detected spends
The synthetic batches have empty ciphertexts and an empty `decrypted` vector, while their nullifiers belong to no stored note. They therefore test tree appends, watermarks, and checkpoints but not receipt persistence, outgoing-note recovery, or spent-note deltas after cancellation. These deltas are central to the partial-finalization contract because the advanced watermark prevents incoming receipts from being emitted again. Add a cancelled-prefix test with a real owned receipt and a matching scanned spend, and assert that both store state and the returned changeset contain the expected receipt and spent mark.
| let advanced = prior.max(new_index); | ||
| if cancelled && advanced == prior { | ||
| continue; | ||
| } | ||
| store | ||
| .set_last_synced_note_index(*id, new_index) | ||
| .set_last_synced_note_index(*id, advanced) | ||
| .map_err(|e| PlatformWalletError::ShieldedStoreError(e.to_string()))?; | ||
| changeset.record_synced_index(*id, new_index); | ||
| changeset.record_synced_index(*id, advanced); |
There was a problem hiding this comment.
🔴 Blocking: Do not mark a cancelled prefix as a complete balance scan
When a cancelled pass advances an account, it calls set_last_synced_note_index. Both store implementations also set last_scanned_index and balance_source = ScannedThisSession, whose contract requires a complete scan. This changes availability, not just metadata: local_balance_snapshot accepts that source as sufficient evidence to return Ready for an otherwise unhydrated account. A newly registered account can therefore consume a prefix containing no owned notes, cancel before its receipts, and expose a ready zero balance. The emitted partial watermark is also interpreted as completed-scan evidence during restoration. Separate resumable progress from completed-scan provenance in the Rust store and persistence contract. Preserve note deltas and resume indices on cancellation without certifying complete coverage, and test the local snapshot both immediately after cancellation and after restoration.
source: gpt-6.1-sol (phase2-reviewer: general)
| // Everything below that talks to the network is skipped once the | ||
| // pass is cancelled; the store-only steps and the host flush of | ||
| // what the scan consumed still run (see the method docs). | ||
| // | ||
| // Snapshotted here on purpose: the scan is the step that was or | ||
| // was not cut short, and the cooldown decision below is about | ||
| // whether that scan reached the tip. A stop that lands during the | ||
| // store-only steps after a complete scan does not change that. | ||
| let cancelled = cancel.is_cancelled(); |
There was a problem hiding this comment.
🔴 Blocking: Defer immutable activity derivation until the scan completes
Continuing all store-only processing after a partial scan also runs derive_activity_into_changeset unconditionally at lines 1511–1513. That derivation groups notes by their discovering proof height and skips an entire cluster if any commitment overlaps an existing activity row. On a cold restore, cancel after discovering receipt A, then resume and discover receipt B proven at the same height. The cancelled pass saves an activity row for A; the resumed cluster contains A and B, overlaps A's row, and produces only a confirmation for that existing row. B never receives an activity entry, and A's amount and commitment set are not expanded on later passes. Keep flushing raw notes, spent marks, and resume indices, but defer creation of immutable scan-derived history until completion or explicitly support extending incomplete clusters. Add a cancel/resume regression with owned receipts sharing a proof height.
source: gpt-6.1-sol (phase2-reviewer: general)
| // Runs before the balance read so freed notes are reflected in | ||
| // this pass's balances. | ||
| if let Some((snapshot, recorded)) = stranded_release { | ||
| // this pass's balances. Skipped on a cancelled pass: both halves | ||
| // broadcast, and a skipped release simply waits for the next pass. | ||
| if let Some((snapshot, recorded)) = stranded_release.filter(|_| !cancelled) { |
There was a problem hiding this comment.
🔴 Blocking: Propagate cancellation through the post-scan retry helpers
The spend-redrive gate uses a cancellation snapshot taken immediately after scanning. If stop arrives afterward, the subwallet loop and redrive_pending_spends continue through every pending record without observing it. The identity-shield path checks between wallets, but its helper cannot cancel the read-only nonce fetch or observe cancellation before the subsequent broadcast and remaining records. This is not merely waiting for one already-submitted broadcast: new requests continue to start after stop, potentially consuming multiple default 10-second timeouts and retries and exhausting the quiesce budget. Thread the pass token through both retry helpers, check it between records and before each new request, and make read-only waits cancellable. Preserve conservative reservation and ambiguity handling for broadcasts already submitted, and still perform the mandatory changeset flush.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general)
| wedge_sync_slot(&holder); | ||
| let _slot = SyncSlotGuard(&holder.is_syncing); | ||
| holder.pass_token().cancelled().await; |
There was a problem hiding this comment.
🟡 Suggestion: Capture the test pass token before publishing the occupied slot
This multi-threaded test publishes is_syncing = true before obtaining its pass token. The parent can observe the occupied slot and call quiesce() while the spawned task is preempted between those statements. Stop then cancels and rotates the parent token; the spawned task subsequently obtains the fresh, uncancelled lineage and waits indefinitely, causing the two-second assertion to fail under a valid scheduling interleaving. Capture the token before claiming the slot, matching the production entry points.
| wedge_sync_slot(&holder); | |
| let _slot = SyncSlotGuard(&holder.is_syncing); | |
| holder.pass_token().cancelled().await; | |
| let pass_cancel = holder.pass_token(); | |
| wedge_sync_slot(&holder); | |
| let _slot = SyncSlotGuard(&holder.is_syncing); | |
| pass_cancel.cancelled().await; |
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, rust-quality, security-auditor)
| if cancelled | ||
| && notes.total_scanned == 0 | ||
| && crate::changeset::merge::Merge::is_empty(¬es.changeset) | ||
| { | ||
| tracing::debug!("Coordinator sync cancelled before the scan consumed anything"); | ||
| return summary; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Exercise coordinator finalization after mid-scan cancellation
Both new coordinator cancellation tests cancel before entering the scan, so they return at the initial cancellation check. The synthetic stream tests invoke only the scan helper. Consequently, no test exercises the coordinator's partial-pass summary, persister fan-out, skipped redrive/release work, or cooldown handling after consumed batches. Add a coordinator-level mid-scan cancellation test that asserts the consumed changeset reaches the wallet persister, the summary reports useful partial work, no post-scan network request starts, and the cancelled pass does not establish a caught-up cooldown.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)
| /// A batch of `len` notes at `start_index`. The notes are opaque to the | ||
| /// scanner: an empty ciphertext never OVK-recovers, the single subwallet | ||
| /// is the stream driver so no IVK trial-decryption runs, and a nullifier | ||
| /// nobody owns is a no-op in the spend replay. Only the commitment is | ||
| /// load-bearing — a distinct small value is a valid Pallas element. | ||
| fn batch(start_index: u64, len: u64, total_count: u64) -> ShieldedChunkBatch { | ||
| let notes = (0..len) | ||
| .map(|i| { | ||
| let mut cmx = [0u8; 32]; | ||
| let pos = start_index + i; | ||
| cmx[0] = (pos as u8) + 1; | ||
| cmx[1] = (pos >> 8) as u8; | ||
| ShieldedEncryptedNote { | ||
| cmx: cmx.to_vec(), | ||
| nullifier: vec![0xAA; 32], | ||
| cv_net: vec![], | ||
| encrypted_note: vec![], | ||
| } | ||
| }) | ||
| .collect(); | ||
| ShieldedChunkBatch { | ||
| start_index, | ||
| notes, | ||
| decrypted: vec![], | ||
| block_height: 1_000 + start_index, | ||
| is_partial: false, | ||
| total_count, | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Test cancellation with owned notes and scan-detected spends
The synthetic batches have empty ciphertexts and an empty decrypted vector, while their nullifiers belong to no stored note. They therefore test tree appends, watermarks, and checkpoints but not receipt persistence, outgoing-note recovery, or spent-note deltas after cancellation. These deltas are central to the partial-finalization contract because the advanced watermark prevents incoming receipts from being emitted again. Add a cancelled-prefix test with a real owned receipt and a matching scanned spend, and assert that both store state and the returned changeset contain the expected receipt and spent mark.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)
|
Replaced by #5743 (same branch, now against |
Issue being fixed or feature implemented
Closes #5200.
Stacked on #5663 (base branch
fix/reopen-quiesce-gate-on-start); retarget tov5.1-devonce that merges.A shielded sync pass had no cancellation point:
ShieldedSyncManager::sync_now→NetworkShieldedCoordinator::syncran to completion, and only the loop's sleep between passes watched the registry token. Sostop()returned at once,quiesce()waited for the whole pass (10 s drain budget, thenErrorShutdownIncomplete, code 27) anddestroyjoined the loop for up to 37 s. On dashwallet-ios network switches that was 47 s of teardown spent waiting for a pass that nobody needed any more.What was done?
manager/shielded_sync.rs):pass_cancel: Mutex<CancellationToken>,pass_token()hands each pass a child.stop()cancels the current parent and rotates in a fresh one, then cancels the loop as before. Owning it in the manager (instead of reusing the loop's registry token) makes passes dispatched from host threads (sync_now(true),sync_wallet) cancellable too, and sincedrain_passcallsstop()first,quiesce,clear_shieldedandshutdownall cut a running pass short. The token is taken right after the slot CAS and before the gate check, which closes the race withdrain_pass's hold-then-stop order (commented in place).wallet/shielded/coordinator.rs):sync(force, cancel). Bails with an empty summary if cancelled before any network work; after the scan, skips the network-only steps (spend re-drives, stranded releases, identity-shield re-drives) when cancelled, but still runs the store-only steps and the persister fan-out for what the scan consumed, and does not stamp the caught-up cooldown.wallet/shielded/sync.rs):sync_notes_acrossis split into the network wrapper (builds the SDK stream) andsync_notes_from_stream(snapshot, write lock, consume, finalize). The consumer checks the token before taking the write lock andselect!s on it between batches (biased); on cancel it drops the stream (and its up-to-16 in-flight fetches) and runs the same finalize over the consumed prefix.ShieldedSyncPassSummaryis untouched. Three test callers inbalance_tests.rsand two examples pass a fresh token.Why finalizing a partial scan is safe: batches arrive as a contiguous ascending prefix (the SDK's reorder buffer only releases the batch at its watermark), the watermark is
aligned_start + total_notes_scannedfrom consumed batches, the checkpoint id is the real post-appendtree_size()and so stays strictly monotonic, and nothing after the loop assumes the tip was reached. Today's?error exits already leave appended leaves without a checkpoint or watermark; a cancelled pass is strictly better than an errored one. The fan-out is necessary, not just safe:save_noteis gated by the watermark, so an unflushed note would never be re-emitted to the host.How Has This Been Tested?
New tests (
RUST_MIN_STACK=4194304 cargo test -p platform-wallet --features shielded …):wallet::shielded::sync::cancel_tests(synthetic batch stream, file-backed store): a pass cancelled between two batches returns promptly, appends both, watermarks at 8 and has a checkpoint covering the appended leaves (witness(7)); a pre-scan cancel touches nothing and never builds the stream; a later pass re-delivering the realigned range appends only the new notes and checkpoints past the partial one.manager::shielded_sync::tests:stop()cancels the in-flight token and rotates a fresh one;quiesce()returns within 2 s when the pass observes its token, instead of waiting out the 10 s budget.wallet::shielded::coordinator::tests::should_skip_network_and_cooldown_when_cancelled_before_scan: pre-cancelled token → empty summary (the mock SDK would otherwise fail every wallet), no cooldown stamp, empty tree.Existing suites:
balance_tests(15),manager::(129),wallet::shielded::coordinator(23) pass;cargo clippy -p platform-wallet --features shielded --tests --examplesclean;platform-wallet-fficompiles withshielded.Not run on a device. The manual check is #5200's scenario: switch networks in dashwallet-ios while a shielded pass runs;
shielded_sync_stopshould return code 0 within about one batch anddestroyin milliseconds.Breaking Changes
None for hosts.
NetworkShieldedCoordinator::syncgains acancel: &CancellationTokenparameter (crate-internal callers updated).Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
Supersedes #5664, whose squash was merged into the base branch by mistake and reverted there (ba3fb97).