Repository navigation
perf(platform-wallet): don't hold the shielded store lock across sync network I/O - #5277
PastaPastaPasta wants to merge 7 commits into
Conversation
|
Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
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 ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughShielded sync now releases the store write lock during network streaming and appends commitments in slices. Store epochs detect conflicting lifecycle changes. The coordinator retries superseded passes, and checkpoint handling avoids adding non-increasing checkpoint IDs. ChangesShielded sync consistency and lock contention
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Sequence Diagram(s)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
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Shielded sync no longer holds the store lock while it waits on the network, so sends no longer stall behind a running sync. Wallet changes made during a sync now cancel and retry the affected pass safely. No concrete merge-blocking issue was identified. The timing-based tests may be sensitive to CI load. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to 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 Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
|
✅ Final review complete — no blockers (commit 9b7b561) · triage: critical |
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
|
@thepastaclaw review |
…er of the sync consumer Split `sync_notes_across` into the SDK-facing wrapper, which builds the `sync_shielded_notes_stream` (and its progress-clamping config), and a consumer generic over the batch source. Production behaviour is unchanged; tests can now drive a sync pass with a hand-fed stream and hold it mid-download. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… network I/O A shielded sync pass took the store write lock before opening the SDK note stream and held it until the pass committed, across every `stream.next().await` (network round trips, server proof generation), trial decryption and tree appends. A send needs that lock up to five times (reserve notes, anchor/witness probe, pending activity, release arming, post-result), and tokio's RwLock is fair, so every send waited for the whole in-flight pass: up to minutes during catch-up. The pass now never holds the lock across an await on the network: - Each batch is parsed, trial-decrypted and OVK-recovered with no lock held. Its new commitments are appended in 256-leaf write-lock holds. - The checkpoint, notes, outgoing notes, scan-detected spends and watermarks are still committed together in one write-lock hold after the stream ends, so readers see them atomically per pass, as before. - Spends can't observe the uncheckpointed leaves appended between holds: they witness only at checkpoints, and shardtree computes a checkpoint-depth witness as of that checkpoint's position. Because lifecycle operations can now land mid-pass, every write the pass makes first re-validates its snapshot: - `StoreEpoch` keeps one generation per wallet and one for the tree. Unregister, account-dropping re-binds and snapshot restores bump their wallet's generation; Clear bumps the tree's. Each bump happens under the store write guard. - The pass watches the generations before it snapshots the registry. - The tree's leaf count must be exactly the snapshot size plus the pass's own appends. If anything moved, the pass is abandoned (`Superseded`) having written only uncheckpointed leaves, the state an interrupted pass already leaves. `sync()` then retries up to twice from a fresh registry snapshot. Changes to a wallet the pass isn't scanning don't supersede it, so binding a second wallet mid-pass keeps the scan. Related changes: - Passes on a coordinator are serialized by a `sync_pass` mutex, which keeps the pass the tree's only appender. - The commit checkpoints whenever the tree is non-empty, not only when the pass appended. Otherwise notes saved in leaves an interrupted pass left behind (superseded, stream error, crash) had no checkpoint and were unspendable until the chain grew. A checkpoint at an unchanged size is a no-op; this rule is now documented on `ShieldedStore::checkpoint_tree` and followed by the in-memory store. - The tree-progress callback no longer runs under the store lock. In the regression test, a write-lock wait behind a pass stalled on the network dropped from 1.51 s (the full stall) to about 2 µs. While the pass appends an 8192-leaf batch, concurrent waits are 5-10 ms median and under 60 ms max. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…nd tests Simplification pass over the previous commit, behaviour unchanged: - Restores bump the wallet generation directly instead of through a once-only flag. Only equality of generations is observable, so the extra bumps make no difference. - Drop the IVK clone and the `new_tree_size` alias in the sync pass. - Remove an unused derive. - Collapse comments that restated the `StoreEpoch` docs. - Tests: add a `TempTree` drop guard so the SQLite file is removed even when a test panics. Add `expect_superseded` and a shared host-snapshot builder. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
66d1cdd to
6b4f9a4
Compare
|
Bots are done — your move: post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
🟡 2 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-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. - 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 below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); 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`:
- [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.
A sync pass no longer holds the store lock across its download, so a send can confirm a note spent (`mark_notes_spent`) mid-pass. When the pass re-scans that note (e.g. a restored notes-only snapshot left the watermark at 0) from scan data older than the spend, `save_note` replaced the stored note wholesale with `is_spent: false`, and the consumed note became selectable again. The commit now carries the stored spent flag over to re-saved receipts and the emitted changeset. The append-contention test could pass without any probe overlapping the ingest: its prober had no readiness handshake, and a tokio task that yields is starved by the pass's uncontended slice acquisitions. The prober now runs on its own thread, and the test requires probes at 8+ distinct mid-ingest tree sizes, so a whole-batch (or few-slice) hold fails it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
🟡 2 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 9: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-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. - 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 below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer - 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.
| /// Generations a sync pass re-checks before every store write; | ||
| /// lifecycle mutations that conflict with a pass's commit bump them | ||
| /// under their store write guard, and [`sync`](Self::sync) retries a | ||
| /// superseded pass from a fresh registry snapshot. See [`StoreEpoch`]. | ||
| store_epoch: StoreEpoch, |
There was a problem hiding this comment.
🟡 Suggestion: 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.
source: gpt-6.1-sol (phase2-reviewer: architecture-layering)
There was a problem hiding this comment.
Not changing this in this PR. Nothing in production can reach it:
- Every in-tree purge goes through the coordinator's lifecycle methods (
coordinator.rs:799,:921), which bump the epoch under their store guard. - The in-crate callers of
store()(the send and balance paths inplatform_wallet.rs) never purge. - FFI hosts only see exported functions.
I tried narrowing store() to pub(crate), but platform-wallet-ffi's contention tests (shielded_sync.rs:883, :961) need the raw lock from outside the crate to model a pass owning it. A test-only escape hatch would reopen the same hole.
The clean fix is your first option: have the store's purge methods bump the generations themselves, so invalidation can't be skipped at any boundary. That moves StoreEpoch into the store and touches every lifecycle path, so it's better as a follow-up than tacked onto this perf change.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Deferred (re-reviewed at 3b47cd06): Your caller tracing checks out: current production purges go through epoch-bumping lifecycle methods, while the FFI raw-lock accesses are test-only. The publicly exported native handle still permits bypassing invalidation, so I am retaining your store-owned generation proposal as a concrete follow-up rather than requiring that ownership refactor here.
… mid-pass The stalled-stream tests take the store lock after a pass's first batch, before feeding it another batch or ending the stream. If sync regressed to holding its guard across `stream.next()`, the pass would wait for the test's next batch while the test waited for the pass's guard, and the test would hang instead of reporting the regression. Those mid-pass acquisitions now go through a timeout. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
🟡 2 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-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. - 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 below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); 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/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.
…ullifier Carrying a stored spent flag over to a re-saved receipt read `get_all_notes`, which deep-copies the subwallet's whole note history (payloads included) under the final store write guard on every pass that saves a receipt. `ShieldedStore::is_note_spent` answers from the nullifier index instead. Its default body scans `get_all_notes`, so other implementations keep compiling. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
If sync regressed to holding the store writer across its first `stream.next()` and won the lock first, the prober would block, the test would wait on the prober forever, and sync would wait on a batch the test never sent. A timeout turns that hang into a failure. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-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. - 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 below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
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.
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
This branch has not been deployed
2 participants
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, which can be minutes during catch-up. That wait appeared to the user as "generating proof".
Part of a series cutting Dash shielded send latency. Related: dashpay/halo2#1 and dashpay/orchard#12 (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;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 ·
9b7b561/self-reviewedrs-platform-wallet-ffi(packages/rs-platform-wallet-ffi/src/shielded_sync.rs) — HashEngineering or ZocoLini or llbartekll or romchornyirs-platform-wallet(packages/rs-platform-wallet/src/wallet/platform_wallet.rs,packages/rs-platform-wallet/src/wallet/shielded/balance_tests.rs,packages/rs-platform-wallet/src/wallet/shielded/coordinator.rsand 4 more) — HashEngineering or ZocoLini or 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