Repository navigation
fix(platform-wallet): stop concurrent shielded syncs from duplicating commitments - #5564
Claudius-Maginificent wants to merge 2 commits into
Conversation
…ncurrent shielded syncs Two syncs over the same shielded store snapshot the tree size under a read lock and append under a later write lock, so both append the same commitments (8 leaves instead of 4 cold, 6 instead of 4 resumed). Observed on testnet as witness roots matching no Platform-recorded anchor. Co-Authored-By: Codex <noreply@openai.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
…hotting sync gates Hold the write guard from the tree-size/watermark snapshot through the streamed append, so a concurrent sync cannot append between them and make this pass re-append commitments already in the tree. The stream already ran under this guard; SDK fetches never touch the store. Co-Authored-By: Codex <noreply@openai.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
|
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:
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 |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
✅ Final review complete — no blockers (commit ac9693e) · triage: normal |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The fix correctly serializes the snapshot, streamed consumption, tree appends, and watermark updates under the existing store write lock, addressing the concurrent commitment-duplication race. The wallet test check passed; the unrelated Swift SDK build was still in progress at the captured CI snapshot. I found one misleading regression-test comment and one minor test-fixture coupling issue.
🟡 1 suggestion(s) | 💬 1 nitpick(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) — This is a contained concurrency fix in shielded wallet synchronization that changes locking and persistence ordering, so it requires careful review but does not itself alter consensus, cryptography, or a critical protocol surface. - 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/concurrent_sync_tests.rs`:
- [NITPICK] packages/rs-platform-wallet/src/wallet/shielded/sync/concurrent_sync_tests.rs:90-93: Concurrency test comment describes the pre-fix lock behavior as current behavior
The comment says that releasing the writer grants both queued readers permits, but the current implementation queues both syncs on `store.write()` and serializes them before either can snapshot. That reader behavior describes the bug being reproduced before this fix, not the behavior of the code under test. Make the historical pre-fix behavior explicit so the rendezvous remains understandable.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/sync/concurrent_sync_tests.rs:24-25: Name the mock stream window to document its coupling to sync configuration
The mock provisions exactly 16 chunks while `sync_notes_across` configures `max_concurrent` to 16. If either value changes, the fixture can under-provision the speculative window and leave a fetch expectation pending, producing a confusing test failure. A named constant and an explicit coupling comment make the required synchronization visible.
| // Queue both at their first store lock, without sleeps or scheduler races. | ||
| // Releasing the writer grants both queued readers their permits: each can | ||
| // snapshot before either acquires the append lock. An atomic write-locked | ||
| // snapshot instead serializes these same two queued syncs. |
There was a problem hiding this comment.
💬 Nitpick: Concurrency test comment describes the pre-fix lock behavior as current behavior
The comment says that releasing the writer grants both queued readers permits, but the current implementation queues both syncs on store.write() and serializes them before either can snapshot. That reader behavior describes the bug being reproduced before this fix, not the behavior of the code under test. Make the historical pre-fix behavior explicit so the rendezvous remains understandable.
| // Queue both at their first store lock, without sleeps or scheduler races. | |
| // Releasing the writer grants both queued readers their permits: each can | |
| // snapshot before either acquires the append lock. An atomic write-locked | |
| // snapshot instead serializes these same two queued syncs. | |
| // Before this fix, both passes reached a shared read lock here; releasing | |
| // the writer let both snapshot before either acquired the append lock. | |
| // The write-locked snapshot now serializes the queued syncs instead. |
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)
| // Cover the stream's speculative window; only the first chunk has notes. | ||
| for chunk in 0..16 { |
There was a problem hiding this comment.
🟡 Suggestion: Name the mock stream window to document its coupling to sync configuration
The mock provisions exactly 16 chunks while sync_notes_across configures max_concurrent to 16. If either value changes, the fixture can under-provision the speculative window and leave a fetch expectation pending, producing a confusing test failure. A named constant and an explicit coupling comment make the required synchronization visible.
| // Cover the stream's speculative window; only the first chunk has notes. | |
| for chunk in 0..16 { | |
| const MOCK_SPECULATIVE_WINDOW_CHUNKS: u64 = 16; | |
| // Keep this aligned with `max_concurrent` in sync.rs. | |
| for chunk in 0..MOCK_SPECULATIVE_WINDOW_CHUNKS { |
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)
github-actions
Bot
added
bot-review-skipped
Basic explanation
What this does: Prevents overlapping shielded syncs from duplicating local note-tree entries and breaking later spends.
Value: Wallets sharing a coordinator retain consistent scan progress and a usable commitment tree.
Risks: Low; the store lock already covered network streaming. This extends it over the preceding snapshot, with no protocol or consensus changes. Existing corrupted stores still require a coordinated clear and rescan.
Issue being fixed or feature implemented
sync_notes_acrossread tree size and subwallet watermarks before obtaining its append lock. Concurrent passes could therefore append the same positions using stale snapshots.Testnet evidence: four shielded e2e cases ran in parallel; two passes started at 5,321 leaves and both scanned through 5,323, after which the local tree reported 5,325. Later spends reported
ShieldedNoRecordedAnchor, and inflated progress was associated with missed incoming notes. Observed on 2026-10-23 in the platform-wallet e2e suite (#3549, testnet,--test-threads=4): 10 shielded cases failed this way, while the same cases passed when they did not overlap.What was done?
platform-wallet::wallet::shielded::sync; no SDK, Drive, consensus, or public API changes.No automatic corruption detection or repair is added. Sync trusts the local leaf count; spend-time witness/anchor checks can expose symptoms but cannot identify duplicate leaves. Existing clear/reset functionality must be coordinated with host persistence before a rescan.
How Has This Been Tested?
Offline only, with
CARGO_BUILD_JOBS=4andCARGO_NET_OFFLINE=true:should_append_commitments_once_across_concurrent_cold_syncsandshould_append_commitments_once_across_concurrent_resumed_syncs. Reproduced 8 versus 4 and 6 versus 4 leaves before the fix.cargo test -p platform-wallet --all-features --lib wallet::shielded::sync -- --nocapture: all 17 passed, including both regressions, spend detection, and IVK/OVK/memo round trips.cargo clippy -p platform-wallet --all-targets --all-features -- --no-deps -D warnings: passed.cargo fmt --allandgit diff --check: passed.No live testnet rerun.
Breaking Changes
None.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent
🤖 Generated with Claude Code
https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
PR Hygiene ·
ac9693ers-platform-wallet(packages/rs-platform-wallet/src/wallet/shielded/sync.rs,packages/rs-platform-wallet/src/wallet/shielded/sync/concurrent_sync_tests.rs) — 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.