You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
perf(platform-wallet): don't hold the shielded store lock across sync network I/O - #5277
Shielded send started while a sync pass is running: it waited for the rest of the pass before it could reserve notes and start proving. Worst case (send at the start of the pass): 0.30 s median (0.23–1.6 s) behind an incremental pass, 1.2 s behind a full testnet catch-up, and 226 s behind one catch-up that stalled. Now ≤ 15 ms, and ≤ 0.1 ms during an incremental pass.
How often: passes are short today, so a send overlaps one in roughly 1% of sends (the 60 s loop's ~0.3 s pass), more for a second send right after the first (forced readback passes at 0, +4, +12 and +24 s) and right after foregrounding. The tail is unbounded: a slow or stalled pass held every send for its whole length.
Measured on an Apple M4 Max desktop, release build, against testnet (4,879-note pool), host load average 44–320 from other builds. 5 interleaved runs per side (base, PR, base, PR, ...), each one cold catch-up plus 4 incremental passes through NetworkShieldedCoordinator::sync(true). The "send" is a probe that takes the store write lock (as reserve_notes does) every 20 ms; "before" numbers are its wait when it starts at the beginning of the pass, i.e. the worst case. Desktop network only; phone numbers not measured.
Issue being fixed or feature implemented
A shielded sync pass held the shielded store's write lock for the entire note download:
every stream.next().await (network round trips and server proof generation);
trial decryption;
tree appends.
A shielded send needs that same lock up to five times: reserve notes, the anchor/witness probe, pending activity, release arming, and post-result. tokio's RwLock is fair, so every send queued behind the whole in-flight pass.
Passes run often:
the Rust loop runs every 60 s;
the iOS app adds a 15 s watchdog / 90 s staleness pass;
the app also forces passes on foreground (if 30 s have passed since the last scan), every 5 s on the Receive screen while it waits for a payment, and after each send (plus reconciliation passes at +4, +12 and +24 s).
So a send could wait anywhere from 0 s to a full sync pass. That wait appeared to the user as "generating proof".
No lock across network I/O. Each streamed batch is parsed, trial-decrypted and OVK-recovered with no lock held. New commitments are appended in 256-leaf write-lock holds.
Same per-pass atomicity for readers. After the stream ends, the checkpoint, saved notes, outgoing notes, scan-detected spends and watermarks are still committed together in one write-lock hold.
Spends are unaffected by mid-pass appends. The spend path witnesses only at checkpoints, and shardtree (0.6.2, checked) computes a checkpoint-depth witness as of that checkpoint's position. Leaves appended between holds change no witness or anchor until the pass checkpoints them together with their notes. Sends also copy their witnesses out under the read lock.
Concurrent lifecycle changes are detected (StoreEpoch).
Unregister, account-dropping re-bind and host snapshot restore bump their wallet's generation. Clear bumps the tree generation. All bumps happen under the store write guard.
The pass records the generations before it snapshots the registry. Every write it makes first re-checks them, plus the tree's exact leaf count.
If anything moved, the pass is Superseded. It has written only uncheckpointed leaves, and sync() retries up to twice from a fresh registry snapshot.
Changes to wallets the pass isn't scanning don't supersede it.
One pass at a time. A per-coordinator sync_pass mutex serializes sync() passes, keeping the pass the tree's only appender.
Checkpoint fix. The commit now checkpoints whenever the tree is non-empty, not only when the pass appended. Notes in leaves left by an interrupted pass (superseded, stream error, crash) were otherwise unwitnessable until the chain grew. A checkpoint at an unchanged size is a no-op; this is now documented on ShieldedStore::checkpoint_tree and followed by InMemoryShieldedStore.
Testability and side effects.
The sync consumer takes its batch stream as a parameter (sync_notes_from_source), so tests can pause a pass mid-download.
The tree-progress callback no longer runs under the store lock.
The FFI local-balance snapshot (100 ms budget) now succeeds during a sync instead of timing out.
How Has This Been Tested?
New sync/lock_contention_tests.rs drives a pass with a hand-fed stream:
Scenario
Before
After
Send write-lock wait while the pass is stalled on the network (regression test)
1.514 s (whole stall; test fails on the old code)
0.5–3.9 µs
Send read-lock wait (anchor/witness probe), same situation
whole stall
0.25–1.3 µs
Lock waits while the pass appends an 8,192-leaf batch
entire pass
median 4–10 ms, max 38–155 ms (debug build, shared host)
Plus:
spend witnesses are stable mid-pass;
supersession on wallet removal, on Clear right before commit, and on a foreign tree write;
no supersession for another wallet's change;
the retry checkpoints leaves an abandoned pass left behind (this test fails with the old checkpoint condition);
coordinator-level tests over the mock SDK that inject lifecycle changes mid-pass: retry without bringing back a removed wallet, ShieldedSyncFailed after repeated supersession, and a mid-pass bind of another wallet does not interrupt the scan;
which lifecycle operations bump the generations.
Testnet measurement (see "Observable value"), store write-lock wait seen by the probe, per pass:
Pass
Pass length (median, range)
Before
After
Cold catch-up, 4,879 notes (5 per side)
1.2 s (1.2–226 s base, 1.2–300 s PR)
1.22 s (1.17–225.9 s)
4.0 ms (2.8–15.2 ms)
Incremental (20 per side)
0.31 s base / 0.38 s PR (0.24–1.8 s)
0.30 s (0.23–1.62 s)
0.001 ms (≤ 0.07 ms)
The two slow catch-ups (226 s, 300 s) coincided with a load spike on the host; pass length is otherwise unchanged by this PR.
Commands:
cargo test -p platform-wallet --features shielded: 1396 lib tests plus integration tests pass.
cargo test -p platform-wallet-ffi --features shielded: 426 pass.
cargo clippy ... --all-targets -D warnings (both crates) and cargo fmt --all --check are clean.
Two review rounds and a simplification pass were applied before opening.
Breaking Changes
None. A pass superseded three times in a row reports ShieldedSyncFailed for the wallets it was scanning; the next scheduled pass runs normally.
Known trade-offs:
An abandoned pass discards its download; the retry refetches from the old watermark, so the host's "Downloaded" progress can jump back.
The final commit still replays scanned nullifiers under the lock. That takes microseconds normally, and up to about a second at the end of a cold full catch-up, as before.
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
If I added or changed GroveDB structure, I described it in the area's structure.rs, regenerated grovedb-structure.json, and checked the structure viewer link posted on this pull request
rs-platform-wallet-ffi (packages/rs-platform-wallet-ffi/src/persistence.rs, packages/rs-platform-wallet-ffi/src/shielded_persistence.rs, packages/rs-platform-wallet-ffi/src/shielded_sync.rs) — HashEngineering or ZocoLini or llbartekll or romchornyi
rs-platform-wallet (packages/rs-platform-wallet/src/changeset/shielded_changeset.rs, packages/rs-platform-wallet/src/wallet/platform_wallet.rs, packages/rs-platform-wallet/src/wallet/shielded/balance_tests.rs and 8 more) — HashEngineering or ZocoLini or llbartekll or romchornyi
swift-sdk (packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentShieldedNote.swift, packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedLocalBalance.swift, packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift and 1 more) — llbartekll or romchornyi
When every merge requirement is met, the PR Hygiene check passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.
Summary by CodeRabbit
Performance
Shielded sync no longer holds the store’s write lock while waiting for network data, helping balance reads and other store operations stay responsive. Brief delays may still occur during tree updates, final commits, restore, or Clear.
Reliability
Sync detects wallet or tree changes during a scan and retries when possible, avoiding commits based on outdated state.
Results for removed wallets are excluded, while unrelated wallet changes do not unnecessarily invalidate a scan.
Checkpoints are recorded only when their identifiers advance, and interrupted sync progress can be checkpointed on a later pass.
Rescanned notes retain their recorded spent status, and shielded stores can report whether a note is spent.
Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.
It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.
No actionable comments were generated in the recent review.
Walkthrough
Shielded sync now releases the store write lock during network streaming, appends commitments in slices, and checks store epochs before committing. The coordinator retries superseded passes. Store changes preserve spent-note status and prevent non-increasing checkpoint IDs.
Changes: Shielded sync consistency and lock contention
Layer / File(s)
Summary
Lifecycle epochs and retry orchestration packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs, packages/rs-platform-wallet/src/wallet/shielded/sync.rs, packages/rs-platform-wallet/src/wallet/platform_wallet.rs, packages/rs-platform-wallet/src/wallet/shielded/balance_tests.rs, packages/rs-platform-wallet/src/changeset/shielded_changeset.rs
The coordinator tracks relevant wallet and tree changes with epochs. It snapshots registered subwallets and retries superseded sync passes up to two times. Changesets exclude subwallet state that was purged. Tests cover lifecycle changes during a pass.
Streamed appends and final commit packages/rs-platform-wallet/src/wallet/shielded/sync.rs, packages/rs-platform-wallet/src/wallet/shielded/store.rs, packages/rs-platform-wallet/src/wallet/shielded/file_store.rs, packages/rs-platform-wallet-ffi/src/shielded_sync.rs, packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedLocalBalance.swift
Sync parses batches before appending commitments in slices of up to 256. It checks the epoch and expected tree size before writes, then validates the snapshot before committing sync results. Store lookups report spent status, and checkpoint IDs must increase strictly. Documentation describes the lock periods during sync and lifecycle operations.
Concurrency and lifecycle validation packages/rs-platform-wallet/src/wallet/shielded/sync/lock_contention_tests.rs
Tests cover store-lock access during stalled streams, incremental appends, supersession and retries, witness roots, unrelated wallet changes, and preservation of spent status during rescans.
sequenceDiagram
participant Coordinator
participant SyncPass
participant SDK
participant ShieldedStore
Coordinator->>SyncPass: Start with epoch watch and subwallet snapshot
SyncPass->>SDK: Open note stream
SDK-->>SyncPass: Stream note batches
SyncPass->>ShieldedStore: Append commitments in slices
SyncPass->>ShieldedStore: Validate snapshot and commit sync results
Coordinator->>SyncPass: Lifecycle change bumps relevant epoch
SyncPass-->>Coordinator: Return superseded outcome
Coordinator->>SyncPass: Retry with refreshed snapshot
Loading
Merge Risk: Low · previous review fc90f
Mergeable with awareness that a wallet change during sync can delay other wallets’ results until a retry or later pass.
Security architecture risk: Low · up to 66d1c
The synchronization change retains existing validation controls and limits conflicting wallet updates. No new security exposure was established, but persistence and failure recovery were not fully verified.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — A commitment-tree consistency failure could affect spending availability across wallets registered on the same network coordinator. Wallet note ownership remains separately scoped; the inspected change does not establish new remote lifecycle authority, cross-environment access, or access to spending keys.
Trust Boundaries and Controls
observed — Production batches use the existing SDK proof-processing path: shielded-note queries reject unproved requests, fetched responses pass through proof parsing and verification, and malformed fixed-width nullifiers or new commitments fail the consumer rather than being silently accepted.
observed — Spend construction holds one read guard while selecting mutually consistent checkpoint-depth witnesses and requires their root to belong to Platform's recorded anchor set. The added real-tree test asserts that uncheckpointed mid-pass appends leave the previous checkpoint root unchanged; this test was inspected but not run.
inferred — Generation validation ends at the protected store commit, while subsequent changeset publication looks up the current persister by WalletId. A concurrent local rebind can therefore create a stale-publication window. The same post-commit ordering and routing existed at the PR base; no worsened exposure was demonstrated, and the host backend's treatment of old-key account data remains unverified.
Resilience and Maintainability Implications
inferred — Supersession, stream errors, or cancellation before the final commit can leave appended leaves without new wallet notes or watermark advances. A later successful pass skips already-present positions and checkpoints the nonempty tree even when it appends nothing. The added retry test checks leaf deduplication, restored witness availability, and repeated-checkpoint idempotency.
Hardening Proposals
proposed — Consider binding queued changesets and completion results to the wallet registration generation, or coordinating publication with lifecycle cleanup. This would strengthen the pre-existing post-commit ownership boundary; it is a hardening proposal, not a finding introduced by this PR.
Pre-merge checks: 5
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title clearly and concisely describes the primary change: avoiding the shielded store lock during sync network I/O.
Docstring Coverage
Docstring coverage is inconclusive: files from earlier reviews still need to be rechecked within the check's analysis limits. Not blocking.
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Finishing Touches
Fix failing CI checks
Commit to this branch
Create a new PR
Generate docstrings
Commit to this branch
Create a new PR
🧪 Generate unit tests (beta)
Commit to this branch
Create a new PR
Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Final validation — Phase 1 + Phase 2
The unlocked scan can overwrite a spend confirmed mid-pass when an existing note is re-scanned, and the append-contention test can pass without an overlapping lock probe. Both are in-scope, non-consensus suggestions under the supplied severity policy. Verification was static; the supplied exact-head CI snapshot reports successful Rust wallet, Swift, and Kotlin checks, with Rust workspace tests skipped and PR Hygiene pending.
Triage: critical by gpt-6.1-sol (effort low) — The intricate changes in sync_notes_from_source in packages/rs-platform-wallet/src/wallet/shielded/sync.rs alter commitment-tree checkpointing and the atomic visibility of spendable notes and detected spends, directly affecting coin selection and cryptographic witness availability.
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`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/sync.rs:757-759: Preserve spend confirmations made during the unlocked scan
The final validation checks lifecycle generations and tree size, but ordinary spend finalization changes neither. Below this guard, rediscovered receipts are constructed with `is_spent: false` and passed to `save_note`; both store implementations delegate to `SubwalletState::save_note`, which replaces an existing note wholesale.
This can undo a confirmation that lands during the newly unlocked download. For example, `restore_locked` accepts a notes-only snapshot with watermark 0, so an existing unspent note can be re-scanned. If the cached tree already has a usable checkpoint covering that note, a concurrent send can spend it and call `finalize_pending` → `mark_notes_spent` while the pass is downloading. When the downloaded scan data predates that spend, its nullifiers cannot repair the subsequent receipt overwrite. The epoch check succeeds, the confirmed spent flag becomes false, and `mark_spent` has already removed the reservation, making the consumed note selectable again and inflating the balance. Previously, the pass-wide write lock ordered a confirmation occurring during download after receipt saving.
Merge rediscovered receipts with the current stored spent state under the final write guard, or skip already-known receipts, and ensure the emitted changeset also preserves that state. Add a hand-fed-stream regression that confirms an existing note at or above the snapshot watermark during the stall, then verifies that committing older scan data preserves its spent status, excludes it from selection, and does not emit an unspent replacement.
In `packages/rs-platform-wallet/src/wallet/shielded/sync/lock_contention_tests.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/sync/lock_contention_tests.rs:241-245: Require lock probes to overlap the append batch
The spawned prober has no readiness handshake, and the test never checks that a lock acquisition occurred during ingestion. If the prober first runs after `done` becomes true, it returns an empty vector; `unwrap_or_default()` then makes `max_wait` zero and the regression assertion passes. Samples taken only before or after ingestion can also pass without exercising contention, so this test does not reliably distinguish slice-sized lock holds from a whole-batch hold.
Coordinate the prober with ingestion and assert that it acquires the store at an intermediate tree size strictly between 0 and 8192. A test-store hook or explicit channels can establish that interleaving deterministically. Keep the timing measurements as supplementary evidence rather than the sole regression invariant.
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete diff at 1828d4b and confirmed that both prior findings are fixed. Two non-blocking suggestions remain: direct mutations through the public store handle bypass lifecycle invalidation, and several stalled-stream tests can deadlock on a lock-scope regression. Validation was static; current CI shows Rust wallet, Swift SDK, and Kotlin SDK checks passing, with PR Hygiene pending.
Triage: critical by gpt-6.1-sol (effort low) — The intricate concurrency rewrite in sync_notes_from_source changes shielded commitment-tree checkpointing and spent-note reconciliation, directly affecting spend witnesses and which notes remain eligible for spending.
Model comparison: every Phase-2 reviewer also ran on gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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/coordinator.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:268-272: Enforce invalidation at the exposed store mutation boundary
The coordinator's lifecycle methods bump this epoch correctly, but the public `store()` accessor still exposes `Arc<RwLock<FileBackedShieldedStore>>`. A native caller can acquire that handle during a stalled download and invoke `ShieldedStore::purge_wallet` or `purge_subwallet` without invalidating the pass. Those methods leave the shared tree unchanged, so the leaf-count check also succeeds. The final commit can then recreate the purged watermark and save receipts accumulated before the purge. Previously, the pass-wide write lock ordered a purge requested during download after receipt saving.
The accessor is documented as supporting tests and migration scaffolding, and existing in-tree lifecycle paths are protected, so this is a non-blocking public-boundary concern rather than a defect in those paths. Make conflicting mutations participate in invalidation at the store boundary, or prevent them through the exposed handle and provide coordinator-controlled operations. Add coverage through that public boundary without manually bumping the epoch in the test.
In `packages/rs-platform-wallet/src/wallet/shielded/sync/lock_contention_tests.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/sync/lock_contention_tests.rs:363: Bound lock acquisition in the stalled-stream tests
After receiving the first progress notification, this test waits for the store writer before sending another batch or dropping the stream sender. If sync regresses to holding its write guard across `stream.next().await`, the pass waits for the sender while the test waits for the pass's guard. Neither can advance, so the test hangs instead of reporting the regression. The same circular wait exists in the mid-download witness, reset, foreign-writer, unrelated-wallet, retry, and spend-confirmation tests.
Bound these mid-pass lock acquisitions with `tokio::time::timeout`, or bound the entire interaction. Unlike the independently scheduled stream release in `should_not_hold_store_lock_while_sync_waits_for_network`, these tests currently have no escape from that circular wait.
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
At exact head 3b47cd0, the complete diff removes store guards from network waits and decryption while retaining guarded snapshot validation and a serialized final commit; no blocking defects were verified. Two non-blocking suggestions remain, three prior findings are fixed, and the public-store invalidation concern remains intentionally deferred. This was static verification only: the supplied head-matched CI snapshot reports successful wallet, Swift, and Kotlin checks, with Rust workspace tests skipped and PR Hygiene pending.
Triage: critical by gpt-6.1-sol (effort low) — The intricate refactor of shielded/sync.rs::sync_notes_across changes spend-detection and note-state commit semantics while allowing concurrent sends and lifecycle mutations, directly affecting funds availability and spend correctness.
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/lock_contention_tests.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/sync/lock_contention_tests.rs:263: Bound the prober readiness handshake
The pass is spawned before the prober, and the first batch is sent only after this unbounded readiness wait. If sync regresses to holding the store writer across its initial stream.next().await and acquires the writer first, the prober blocks in blocking_write(), this recv() waits for the prober, and sync waits for the batch that the test has not sent. That scheduling order hangs the regression test instead of reporting a failure. Use recv_timeout with MID_PASS_LOCK_BOUND; unwinding then drops the sender and StopOnDrop so the stalled interaction can terminate.
In `packages/rs-platform-wallet/src/wallet/shielded/sync.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/sync.rs:819-827: Avoid cloning complete note history to preserve spent flags
Both store implementations delegate get_all_notes to SubwalletState::all_notes, which clones the complete notes vector, including each note's owned note_data buffer. This condition also matches an ordinary new receipt, so saving even one new note deep-copies the subwallet's entire history and builds a spent-nullifier set under the final exclusive store guard. The spent-state reconciliation is necessary, but this implementation adds history-proportional allocations to the lock hold even when only a small number of receipts need reconciliation. Expose a per-nullifier spent-state lookup backed by the existing nullifier_index, or a store-level receipt merge that returns the merged note for the changeset, while keeping reconciliation under the final guard.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
Store-owned invalidation for public native mutations — The public coordinator.store() handle still permits native callers to purge notes and watermarks without changing the coordinator-private generations. Because those purges preserve tree size, an unlocked scan can subsequently recreate the purged state. Current in-tree production purges use epoch-bumping coordinator lifecycle methods, and the cross-crate FFI raw-lock accesses are tests; the author explicitly deferred this concrete public-API ownership gap to a separate follow-up.
Follow-up: Track store-owned invalidation or a restricted production mutation interface separately, with a stalled-stream purge regression through the public boundary that does not manually bump StoreEpoch.
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of the complete diff at 9b7b561 found no remaining in-scope findings requiring changes. Five prior findings are fixed; the public native store-invalidation boundary remains a concrete, non-blocking follow-up rather than a completed fix. No builds or tests were run: the supplied CI snapshot from 2026-10-06T20:29:51Z shows wallet tests queued, Swift and Kotlin checks running, and PR Hygiene pending.
Triage: critical by gpt-6.1-sol (effort low) — The intricate concurrency refactor in shielded/sync.rs changes spend-state preservation and commitment-tree checkpoint handling used for spend witnesses, directly affecting funds movement correctness.
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
Make conflicting mutations invalidate sync through the public native store boundary — The public manager API exposes the coordinator, whose store() accessor returns a writable FileBackedShieldedStore. Its purge methods remove subwallet history without changing the coordinator-owned epoch or shared tree size, so a native caller purging during a download can let the old pass restore an advanced watermark while retaining only its old scan range, causing subsequent scans to skip deleted history. The in-tree lifecycle purge paths participate in invalidation, and the external FFI raw-lock uses are tests. Centralizing invalidation remains an explicitly deferred ownership change, recorded separately rather than requested again in this PR.
Follow-up: In a separate maintainer-requested change, move conflicting-mutation generations to the store boundary or restrict raw mutation access. Cover a purge through the public handle during a scan starting from a positive watermark, without manually bumping StoreEpoch, and verify complete historical recovery.
`persistShieldedNotes` read the existing row with `try?`, so a failed
read took the insert branch and wrote the incoming copy, unspent, over a
row it couldn't see, and the callback still reported success. The read
is what keeps a spent flag, so it now goes through the handler's
`ModelFetching` seam and a failure fails the round.
Also state the keep-the-spent-flag rule where a custom host reads it: on
`ShieldedNoteFFI`, `on_persist_shielded_notes_fn` and Kotlin's
`NativePersistenceBridge.onPersistShieldedNote`.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
Updated after discussion: 1 finding(s) of this review were settled in the inline threads; no blocking finding remains.
⏭️ Deferred (maintainer decision): 🔴 Preserve rejected receipt rounds before publishing later watermarks (tracked in #5695)
Verified the complete diff at 274ee9f and confirmed one blocking persistence-failure path: a rejected receipt round is discarded, allowing a later durable watermark to hide missing notes after restart. All 22 prior findings were independently revalidated: 17 are fixed and five remain covered by concrete tracked follow-ups. Validation was static only; current Kotlin checks passed, Swift build was skipped, and Rust workspace tests failed during checkout because of corrupt runner Git objects, before compilation.
Triage: critical by gpt-6.1-sol (effort low) — The intricate concurrency refactor directly changes spend-state handling in operations.rs::mark_notes_spent and store.rs::SubwalletState::save_note, affecting whether spent funds can become selectable again while sync and sends overlap.
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/coordinator.rs`:
- [BLOCKING → DEFERRED by a maintainer; not blocking, do not fix in this PR] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1700-1706: Preserve rejected receipt rounds before publishing later watermarks
Swift's newly throwing note lookup can reject a receipt round, which FFIPersister rolls back without retaining its changeset. This delivery path only logs the error, although sync_notes_from_source has already saved the receipts and advanced the in-memory watermark. If a pass discovers an unspent note and its host lookup fails once, the next successful pass skips that receipt because its position is below the in-memory watermark (sync.rs:836–839), but still publishes the watermark (sync.rs:938–943). After restart, restore accepts that durable watermark without the missing receipt, and ordinary sync continues skipping the note, omitting its balance and receipt history until an explicit rescan. The correction round does not repair an unspent note. Keep failed publication state recoverable in the Rust persistence/coordinator layer and prevent later durable watermarks from overtaking unpublished receipts. Respect the backend's retry classification rather than blindly retrying a fatal or potentially partially applied round; returning a sync error alone is insufficient while subsequent passes retain the advanced cursor. Add a regression covering receipt-round rejection, a subsequent successful pass, and restoration from the resulting host state.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
Post-scan redrive and activity work still needs lifecycle fencing — The post-scan helpers can retain an obsolete registration across awaits and later broadcast, mutate store state, or notify persistence after removal or replacement. Direct comparison confirms that the same unfenced helper sequence exists on the base revision; the scan-lock refactor does not newly introduce this post-commit window. The concrete follow-up is already tracked in #5675.
Follow-up: Address #5675 separately in rs-platform-wallet, checking lifecycle validity at post-scan mutation, broadcast, and publication boundaries.
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.
Resolves conflicts with #5743 (shielded sync cancellation), which was
written against the old design that held the store write lock across the
streamed download:
- sync_notes_from_stream keeps this branch's lock-free scan and gains the
token. A stop is observed before the scan, on the pre-scan read lock,
between batches and while waiting for an append slice, and returns the
new NoteSyncOutcome::Cancelled. It checkpoints the appended leaves only
when the store is free and the snapshot current, so a stop never queues
behind a writer; the next completed pass checkpoints anything left.
- The coordinator's sync_pass wait is cancellable, a stop after a
supersession is reported as cancelled rather than retried or failed,
and #5743's cooldown and post-scan network-step rules are kept.
- Watermarks never rewind, as in #5743.
- The pass's correction round skips notes the same round already marks
spent, so the host no longer gets a redundant second copy.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merged the latest v5.1-dev (5b3a113) to clear the conflicts. The main overlap was #5743 (sync cancellation), which was written for the old design that held the store write lock across the download. How the two fit together:
Lock-free scan.sync_notes_from_stream keeps this PR's scan and takes the token. A stop is observed before the scan, on the pre-scan read lock, between batches, and while waiting for an append slice. It returns a new NoteSyncOutcome::Cancelled. A stopped pass checkpoints its appended leaves only when try_write succeeds and the snapshot is still current, so a stop never queues behind a writer. Anything it leaves uncheckpointed is picked up by the next completed pass, the same as after a supersession.
Coordinator. The sync_pass wait is cancellable, and the token is re-checked after it. A stop that lands after a supersession returns the empty summary; it is not retried and doesn't fall into the retries-exhausted failure. fix(platform-wallet): let a shielded sync pass observe cancellation #5743's cooldown rules and post-scan network-step skipping are unchanged.
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 2 only (queue backlog)
Static verification of head 5b3a113 found one non-blocking performance issue in the new publication filter and no blocking findings. All 23 prior findings were revalidated: 17 are fixed and six remain tracked deferrals. Current CI reports successful Rust workspace, Swift, and Kotlin checks; PR Hygiene remains pending, and no local builds or tests were run.
Triage: critical by gpt-6.1-sol (effort low) — The intricate synchronization refactor changes spend-state handling and persistence in operations.rs::mark_notes_spent and shielded/sync.rs, making concurrent note availability and spent-state correctness directly relevant to funds movement.
Phase 1 reviewers: not run (skipped for throughput: 11 PRs queued, above the 10 limit)
Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
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/coordinator.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1734-1740: Index same-pass spent nullifiers before filtering receipts
`nullifiers_spent` stores a Vec, so testing `contains` for each saved unspent receipt makes this filter O(receipts × spends) per subwallet. Cold catch-up exercises this path: sync records receipt copies in `notes_saved` before replaying historical spends, so those copies can still have `is_spent == false` even though the same changeset marks their nullifiers spent. This adds quadratic work before publication while `fan_out` is held, delaying delivery and lifecycle operations waiting to purge state. Build a nullifier set once per subwallet and use indexed membership.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
Recover rejected receipt publications before advancing durable watermarks — The coordinator still logs rejected persistence rounds after committing receipts and watermarks in memory. A later successful watermark publication can therefore leave restored host state beyond receipts it never received. The base revision already has this recovery gap, and the previously settled follow-up is tracked in #5695.
Follow-up: Address #5695 separately with recoverable publication state, backend-aware failure handling, and a regression covering rejection, a later successful pass, and restart.
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of the complete diff at c93ca64 found no new actionable defects. All 24 prior findings were revalidated: 18 are fixed and six retain their settled, issue-tracked deferrals, with no reintroduction found. No builds or tests were run locally; current CI reports Rust workspace tests, Kotlin build/tests, and Swift build/tests pending, while the Swift frozen-schema check has passed and no code check is currently failing.
Triage: critical by gpt-6.1-sol (effort low) — The intricate synchronization rewrite directly changes spendable-note state and spend persistence in shielded/store.rs and operations.rs::mark_notes_spent, making concurrent funds-movement correctness part of the diff itself.
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
Out-of-scope follow-up suggestions (2)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
Recover rejected shielded persistence rounds before advancing durable watermarks — The coordinator still logs failed delivery after committing receipts and watermarks in memory. A later successful publication can persist a watermark beyond the missing receipts, leaving those notes absent after restoration. The base revision has the same commit-before-publication and log-only failure handling; this remains the existing recovery gap tracked in #5695.
Follow-up: Address #5695 separately, preserving recoverable publication state and respecting backend retry classifications.
Extend lifecycle fencing to post-scan redrive and activity work — Post-scan helpers continue using the captured subwallet list across awaits without fencing every later mutation, broadcast decision, or direct persistence notification. The same unfenced helper sequence exists on the base revision; the scan and final-publication fences do not resolve this separate lifecycle window.
Follow-up: Complete #5675 with lifecycle checks at helper mutation and notification boundaries and targeted removal/re-key regressions.
Bots are done — your move: address romchornyi left a review thread unresolved, then post /self-reviewed.
Full checklist in the description.
This branch has not been deployed
No deployments
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
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.
github-actions
Bot
added
the
waiting-bots
thepastaclaw
left a comment
Observable value
Shielded send started while a sync pass is running: it waited for the rest of the pass before it could reserve notes and start proving. Worst case (send at the start of the pass): 0.30 s median (0.23–1.6 s) behind an incremental pass, 1.2 s behind a full testnet catch-up, and 226 s behind one catch-up that stalled. Now ≤ 15 ms, and ≤ 0.1 ms during an incremental pass.
How often: passes are short today, so a send overlaps one in roughly 1% of sends (the 60 s loop's ~0.3 s pass), more for a second send right after the first (forced readback passes at 0, +4, +12 and +24 s) and right after foregrounding. The tail is unbounded: a slow or stalled pass held every send for its whole length.
Measured on an Apple M4 Max desktop, release build, against testnet (4,879-note pool), host load average 44–320 from other builds. 5 interleaved runs per side (base, PR, base, PR, ...), each one cold catch-up plus 4 incremental passes through
NetworkShieldedCoordinator::sync(true). The "send" is a probe that takes the store write lock (asreserve_notesdoes) every 20 ms; "before" numbers are its wait when it starts at the beginning of the pass, i.e. the worst case. Desktop network only; phone numbers not measured.Issue being fixed or feature implemented
A shielded sync pass held the shielded store's write lock for the entire note download:
stream.next().await(network round trips and server proof generation);A shielded send needs that same lock up to five times: reserve notes, the anchor/witness probe, pending activity, release arming, and post-result. tokio's
RwLockis fair, so every send queued behind the whole in-flight pass.Passes run often:
So a send could wait anywhere from 0 s to a full sync pass. That wait appeared to the user as "generating proof".
Part of a series cutting Dash shielded send latency. Related: #5280, #5282, dashpay/halo2#1–#3 and dashpay/orchard#12–#13 (prover), dashpay/dashwallet-ios#1183 (prover warm-up).
What was done?
shardtree(0.6.2, checked) computes a checkpoint-depth witness as of that checkpoint's position. Leaves appended between holds change no witness or anchor until the pass checkpoints them together with their notes. Sends also copy their witnesses out under the read lock.StoreEpoch).Superseded. It has written only uncheckpointed leaves, andsync()retries up to twice from a fresh registry snapshot.sync_passmutex serializessync()passes, keeping the pass the tree's only appender.ShieldedStore::checkpoint_treeand followed byInMemoryShieldedStore.sync_notes_from_source), so tests can pause a pass mid-download.How Has This Been Tested?
New
sync/lock_contention_tests.rsdrives a pass with a hand-fed stream:Plus:
ShieldedSyncFailedafter repeated supersession, and a mid-pass bind of another wallet does not interrupt the scan;Testnet measurement (see "Observable value"), store write-lock wait seen by the probe, per pass:
The two slow catch-ups (226 s, 300 s) coincided with a load spike on the host; pass length is otherwise unchanged by this PR.
Commands:
cargo test -p platform-wallet --features shielded: 1396 lib tests plus integration tests pass.cargo test -p platform-wallet-ffi --features shielded: 426 pass.cargo clippy ... --all-targets -D warnings(both crates) andcargo fmt --all --checkare clean.Two review rounds and a simplification pass were applied before opening.
Breaking Changes
None. A pass superseded three times in a row reports
ShieldedSyncFailedfor the wallets it was scanning; the next scheduled pass runs normally.Known trade-offs:
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
PR Hygiene ·
c93ca64/self-reviewedkotlin-sdk(packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/NativePersistenceBridge.kt,packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt,packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.kt) — HashEngineeringrs-platform-wallet-ffi(packages/rs-platform-wallet-ffi/src/persistence.rs,packages/rs-platform-wallet-ffi/src/shielded_persistence.rs,packages/rs-platform-wallet-ffi/src/shielded_sync.rs) — HashEngineering or ZocoLini or llbartekll or romchornyirs-platform-wallet(packages/rs-platform-wallet/src/changeset/shielded_changeset.rs,packages/rs-platform-wallet/src/wallet/platform_wallet.rs,packages/rs-platform-wallet/src/wallet/shielded/balance_tests.rsand 8 more) — HashEngineering or ZocoLini or llbartekll or romchornyiswift-sdk(packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentShieldedNote.swift,packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedLocalBalance.swift,packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftand 1 more) — llbartekll or romchornyiWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.Summary by CodeRabbit