fix(platform-wallet): make shielded re-bind non-destructive so it cannot wipe an in-flight sync pass#4237
Merged
Merged
Conversation
…not wipe an in-flight sync pass
A host re-bind (iOS fires bind twice at launch, plus on Sync Now and
wallet navigation) ran unregister -> purge -> restore on every call.
Against an in-flight sync pass the purge/restore lock acquisitions
queue behind the pass's store write guard and land the moment it
finishes, wiping the pass's freshly discovered notes and watermark and
restoring a persister snapshot loaded before the pass ran. Observed on
testnet as "note discovered by sync is not spendable until app restart"
(ShieldedNoUnspentNotes) and as every subsequent pass rescanning the
full tree from index 0.
- install_shielded_views: idempotent re-bind fast path — when the
wallet is already registered with the same account -> FVK map, just
refresh the registration and skip purge/restore (the in-memory store
is strictly fresher than any persister snapshot).
- restore_for_wallet: advance-only watermark, and never overwrite a
note whose nullifier the store already tracks (a stale unspent copy
would re-offer a spent note to selection).
- register_wallet: insert the persister before the accounts so a pass
starting between the two inserts can no longer drop its changeset
("no persister registered").
- sync: drop the stale "next_start_index is 0 — next sync will rescan
from the beginning" warning; that value is diagnostic-only since the
streaming refactor and fired spuriously on every single-partial-batch
cold scan.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
July 27, 2026 07:32
Contributor
|
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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
Collaborator
|
🕓 Ready for review — 2 ahead in queue (commit 390f091) |
Addresses three code-review findings on the re-bind race fix: 1. Account-set changes no longer reproduce the destructive race. install_shielded_views no longer calls unregister_wallet at all; register_wallet now purges only the subwallets the new registration drops or re-keys (new ShieldedStore::purge_subwallet) and never removes the persister. Retained accounts keep their in-memory notes and watermark across any re-bind, so a registration replacement racing an in-flight sync pass cannot wipe the pass's results, and a pass finishing mid-bind always finds a persister for its changeset. 2. accounts/persisters mutations are atomic across lifecycle operations. A coordinator-level lifecycle mutex serializes register_wallet, unregister_wallet, and clear's registry phase, closing the interleaving (insert persister -> remove accounts -> insert accounts -> remove persister) that could leave visible accounts without a persister. 3. A matching registration no longer implies hydration. The coordinator tracks per-wallet hydration (set only after a successful load + restore for the current registration shape; cleared on shape change, unregister, and clear), and the idempotent fast path requires registration match AND hydration — a transient persistence failure on the first bind is now retried by the next re-bind instead of being fast-pathed over. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
suggested changes
Jul 27, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The latest commit fixes both previously reported defects: identical re-binds no longer take a destructive unregister path, and the coordinator lifecycle mutex preserves the accounts/persister registration invariant. Three blocking issues remain: the full bind lifecycle is not serialized, a changed FVK is rehydrated from durable state belonging to the old key, and an in-flight old-key sync can persist its changeset after the re-key.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking
1 additional finding(s) omitted (not in diff).
🤖 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/platform_wallet.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:637-698: Serialize the complete shielded bind lifecycle
The coordinator lifecycle mutex protects only `register_wallet`; the wallet-local key publication, registration comparison, restore, and hydration commit remain separate operations. Starting from hydrated registration A, bind A can write local keys A and observe `identical = true`; bind B can then write local keys B and register B; bind A can subsequently register A. The coordinator ends on A while `self.shielded_keys` remains B, so sync decrypts with A while address, balance, and spend paths use B. The same gap affects removal and clear: the FFI bind function retains a wallet `Arc` after releasing manager lookup protection, so a bind can register a wallet after `remove_wallet` has completed. In addition, `restore_for_wallet` snapshots registered IDs before taking the store lock, allowing unregister or clear to purge state before a delayed restore repopulates it and calls `mark_hydrated(true)`. Serialize the entire per-wallet install transaction and coordinate it with unregister/clear, or use a registration generation that is checked while committing restore and hydration.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:662-698: Do not restore old-key state after re-keying a subwallet
For the changed-FVK path introduced here, `register_wallet` correctly identifies the existing `SubwalletId` as stale and purges its old-key in-memory notes and watermark. The bind tail then loads and restores that subwallet's durable snapshot unconditionally. Durable notes, activity, and sync state are keyed only by `(wallet_id, account_index)`; the viewing key is stored separately, and a seed-backed bind upserts the FVK without deleting the old subwallet rows. `restore_for_wallet` checks only the registered `SubwalletId`, not which FVK produced the snapshot, so it immediately restores old-key notes and the old high watermark under the new registration and marks it hydrated. This can expose notes the new spend key cannot spend and skip historical notes belonging to the new key. Reject an FVK change, or atomically invalidate the durable subwallet snapshot and exclude it from restoration before committing the new registration.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:967-983: Discard old-key sync changesets after a re-key
A sync pass snapshots the old account/FVK map at lines 843-846 and builds an old-key changeset while holding the store write lock. A concurrent re-key publishes the new registration, waits for that store lock, and purges the old-key in-memory state at lines 418-429. After releasing the lock, however, the old pass still reaches this fan-out and sends its retained changeset through the current wallet persister. The host persistence callbacks receive only the wallet/account IDs and note or watermark data, with no FVK or registration generation that could reject the stale pass. Old-key notes and a high old-key watermark can therefore be persisted after the account was re-keyed, defeating any durable purge and causing a restart to restore the wrong state. Tag sync snapshots with a registration generation or FVK and discard affected subwallet changesets when the current registration no longer matches, or drain the complete pass through persistence before replacing the key.
Comment on lines
+638
to
+698
| *slot = Some(account_views.clone()); | ||
| drop(slot); | ||
|
|
||
| // Rebind is replace-not-merge (the doc contract above). | ||
| // `register_wallet` replaces the coordinator's `accounts` | ||
| // entries for this wallet, but it does NOT touch the | ||
| // store's per-`SubwalletId` state — so a same-process | ||
| // rebind would otherwise leave stale watermarks, orphaned | ||
| // accounts dropped from the new bind set, and abandoned | ||
| // `pending_nullifiers` reservations behind (the latter can | ||
| // make note selection skip spendable notes). Unregister | ||
| // first to purge that state; it's a no-op on first bind. | ||
| coordinator.unregister_wallet(self.wallet_id).await; | ||
|
|
||
| // Register on the coordinator BEFORE restoring so the | ||
| // restore path's "is this account registered?" gate | ||
| // sees this wallet's subwallets. | ||
| // Compute idempotence BEFORE registering — after | ||
| // register_wallet the registration always matches. | ||
| let identical = coordinator | ||
| .wallet_registration_matches(self.wallet_id, &account_views) | ||
| .await; | ||
|
|
||
| // (Re-)register on the coordinator. This is non-destructive | ||
| // by construction: the persister handle is replaced, never | ||
| // removed (a sync pass finishing mid-bind always finds one), | ||
| // and per-subwallet store state is purged only for accounts | ||
| // this registration DROPS or re-keys — accounts that remain | ||
| // bound with the same viewing key keep their in-memory notes | ||
| // and watermark. A re-bind racing an in-flight sync pass can | ||
| // therefore no longer wipe the pass's results (the former | ||
| // unregister-then-register cycle here purged the whole | ||
| // wallet behind the pass's store lock and then restored a | ||
| // pre-pass snapshot — the "note discovered by sync is | ||
| // unspendable until app restart" / "every pass rescans from | ||
| // 0" failure). Registration also runs BEFORE the restore so | ||
| // the restore path's "is this account registered?" gate sees | ||
| // this wallet's subwallets. | ||
| coordinator | ||
| .register_wallet(self.wallet_id, account_views, self.persister.clone()) | ||
| .await; | ||
|
|
||
| // Idempotent re-bind fast path: hosts re-run bind liberally | ||
| // (launch fires it twice — a direct call plus the wallet-set | ||
| // observer — and again on Sync Now / wallet navigation). When | ||
| // the registration is unchanged AND a prior hydration | ||
| // succeeded, the coordinator's in-memory state is strictly | ||
| // fresher than any persister snapshot (the snapshot's rows | ||
| // were produced FROM it), so re-running the restore could | ||
| // only re-apply older data — skip it. The hydration flag is | ||
| // load-bearing: a matching registration alone doesn't prove | ||
| // the store was ever hydrated (the first bind's load/restore | ||
| // may have failed transiently and is only logged), and | ||
| // skipping on registration match alone would leave notes and | ||
| // the watermark absent until a full rescan or restart. | ||
| if identical && coordinator.is_hydrated(self.wallet_id).await { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| // Rehydrate per-subwallet notes / sync watermarks from | ||
| // the persister's start state if any are present for | ||
| // this wallet. The lookup is cheap: load() is the | ||
| // boot-time snapshot, indexed by SubwalletId. Errors are | ||
| // logged but not fatal — first-launch wallets simply | ||
| // see no persisted state. | ||
| // this wallet. The restore is additive and monotonic | ||
| // (`restore_for_wallet` never rewinds a watermark or | ||
| // overwrites a known note), so applying a snapshot on top | ||
| // of retained live state is safe. Errors are logged but | ||
| // not fatal — first-launch wallets simply see no persisted | ||
| // state; the hydration flag stays unset on failure so the | ||
| // next re-bind retries the restore instead of fast-pathing | ||
| // over an unhydrated store. | ||
| match preloaded.map(Ok).unwrap_or_else(|| self.persister.load()) { | ||
| Ok(start) => { | ||
| if let Err(e) = coordinator | ||
| .restore_for_wallet(self.wallet_id, &start.shielded) | ||
| .await | ||
| { | ||
| Ok(start) => match coordinator | ||
| .restore_for_wallet(self.wallet_id, &start.shielded) | ||
| .await | ||
| { | ||
| Ok(()) => coordinator.mark_hydrated(self.wallet_id, true).await, |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Serialize the complete shielded bind lifecycle
The coordinator lifecycle mutex protects only register_wallet; the wallet-local key publication, registration comparison, restore, and hydration commit remain separate operations. Starting from hydrated registration A, bind A can write local keys A and observe identical = true; bind B can then write local keys B and register B; bind A can subsequently register A. The coordinator ends on A while self.shielded_keys remains B, so sync decrypts with A while address, balance, and spend paths use B. The same gap affects removal and clear: the FFI bind function retains a wallet Arc after releasing manager lookup protection, so a bind can register a wallet after remove_wallet has completed. In addition, restore_for_wallet snapshots registered IDs before taking the store lock, allowing unregister or clear to purge state before a delayed restore repopulates it and calls mark_hydrated(true). Serialize the entire per-wallet install transaction and coordinate it with unregister/clear, or use a registration generation that is checked while committing restore and hydration.
source: ['codex']
Collaborator
There was a problem hiding this comment.
Confirmed and fixed in 7959e154ea.
The lifecycle mutex now covers the whole install transaction, not one registry write: NetworkShieldedCoordinator::begin_install returns a guard that holds it across registration comparison → register → restore → hydration commit, and every single-step public method wraps itself in a one-step transaction so external callers stay serialized. clear() takes the mutex before the store reset (lock order is lifecycle → store everywhere), and remove_wallet now marks the handle detached before unregistering, which the install refuses on — so a bind holding a stale Arc<PlatformWallet> past the removal can no longer re-register the wallet.
Went with the "serialize the transaction" remedy rather than a registration generation, since the generation is redundant once the transaction is atomic.
Regression test: with the transaction released early (the old shape), a second bind publishes its registration inside the first one's transaction — a_second_bind_cannot_commit_inside_another_binds_transaction fails left: 2, right: 1 before the fix, passes after. install_transaction_blocks_unregister_until_it_commits and bind_after_wallet_removal_is_refused pin the other two halves.
Comment on lines
+662
to
+698
| coordinator | ||
| .register_wallet(self.wallet_id, account_views, self.persister.clone()) | ||
| .await; | ||
|
|
||
| // Idempotent re-bind fast path: hosts re-run bind liberally | ||
| // (launch fires it twice — a direct call plus the wallet-set | ||
| // observer — and again on Sync Now / wallet navigation). When | ||
| // the registration is unchanged AND a prior hydration | ||
| // succeeded, the coordinator's in-memory state is strictly | ||
| // fresher than any persister snapshot (the snapshot's rows | ||
| // were produced FROM it), so re-running the restore could | ||
| // only re-apply older data — skip it. The hydration flag is | ||
| // load-bearing: a matching registration alone doesn't prove | ||
| // the store was ever hydrated (the first bind's load/restore | ||
| // may have failed transiently and is only logged), and | ||
| // skipping on registration match alone would leave notes and | ||
| // the watermark absent until a full rescan or restart. | ||
| if identical && coordinator.is_hydrated(self.wallet_id).await { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| // Rehydrate per-subwallet notes / sync watermarks from | ||
| // the persister's start state if any are present for | ||
| // this wallet. The lookup is cheap: load() is the | ||
| // boot-time snapshot, indexed by SubwalletId. Errors are | ||
| // logged but not fatal — first-launch wallets simply | ||
| // see no persisted state. | ||
| // this wallet. The restore is additive and monotonic | ||
| // (`restore_for_wallet` never rewinds a watermark or | ||
| // overwrites a known note), so applying a snapshot on top | ||
| // of retained live state is safe. Errors are logged but | ||
| // not fatal — first-launch wallets simply see no persisted | ||
| // state; the hydration flag stays unset on failure so the | ||
| // next re-bind retries the restore instead of fast-pathing | ||
| // over an unhydrated store. | ||
| match preloaded.map(Ok).unwrap_or_else(|| self.persister.load()) { | ||
| Ok(start) => { | ||
| if let Err(e) = coordinator | ||
| .restore_for_wallet(self.wallet_id, &start.shielded) | ||
| .await | ||
| { | ||
| Ok(start) => match coordinator | ||
| .restore_for_wallet(self.wallet_id, &start.shielded) | ||
| .await | ||
| { | ||
| Ok(()) => coordinator.mark_hydrated(self.wallet_id, true).await, |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Do not restore old-key state after re-keying a subwallet
For the changed-FVK path introduced here, register_wallet correctly identifies the existing SubwalletId as stale and purges its old-key in-memory notes and watermark. The bind tail then loads and restores that subwallet's durable snapshot unconditionally. Durable notes, activity, and sync state are keyed only by (wallet_id, account_index); the viewing key is stored separately, and a seed-backed bind upserts the FVK without deleting the old subwallet rows. restore_for_wallet checks only the registered SubwalletId, not which FVK produced the snapshot, so it immediately restores old-key notes and the old high watermark under the new registration and marks it hydrated. This can expose notes the new spend key cannot spend and skip historical notes belonging to the new key. Reject an FVK change, or atomically invalidate the durable subwallet snapshot and exclude it from restoration before committing the new registration.
source: ['codex']
Collaborator
There was a problem hiding this comment.
Confirmed and fixed in 7959e154ea.
Took the "reject an FVK change" branch, since durable rows are keyed (wallet_id, account_index) with no key attribution — the schema can't express a coherent partial invalidation, and ShieldedChangeSet::viewing_keys already documents that an account's FVK never legitimately changes. bind_shielded now compares each derived FVK against the persisted row before upserting it, and the install compares against the current registration; both fail closed with an error pointing at Clear as the recovery.
As defense in depth for direct callers, restore_for_wallet also skips any subwallet whose snapshot carries a viewing key other than the registered one (snapshots predating the FVK rows restore unchanged).
Regression tests: restore_skips_a_snapshot_produced_under_a_different_viewing_key fails on the old restore (old-key notes and the 5000 watermark land) and passes now; bind_rejects_a_viewing_key_change_for_an_already_persisted_account fails on the old bind (it succeeded) and passes now.
One behavior note: the seed bind now hard-fails if persister.load() fails, since it can't tell a re-key from a first bind without it — the seedless path it sits behind in the FFI already did.
…e re-keys Two blocking review findings on the shielded re-bind path. 1. Serialize the complete bind lifecycle. A bind was five separate critical sections — publish the wallet's viewing-key slot, compare the registration, replace it, restore the host snapshot, commit the hydration flag — with the coordinator's lifecycle mutex taken only inside `register_wallet`. Three interleavings followed: - Two binds of one wallet could commit their registrations in the opposite order to their key-slot writes, leaving the coordinator trial-decrypting under one bind's keys while addresses, balances and spends used the other's. - `restore_for_wallet` snapshotted the registered subwallets before taking the store lock, so an `unregister_wallet` / `clear` could purge in between; the restore then repopulated the purged state and marked the removed wallet hydrated. - `remove_wallet` drops the manager's entry, but callers hold an `Arc<PlatformWallet>` across a bind that may resolve a mnemonic through the host, so a bind could re-register a removed wallet — the next sync pass would then re-fetch and re-persist shielded history the host believes it deleted. Widen the lifecycle mutex from "one registry mutation" to "one install transaction": `begin_install` hands out a guard that owns the mutex and exposes the steps, and every single-step public method wraps itself in a one-step transaction so outside callers stay serialized too. `clear` now takes the mutex before the store reset (keeping the lifecycle → store order), and `remove_wallet` marks the handle detached before unregistering, which the install refuses on. 2. Do not restore old-key state after re-keying a subwallet. `register_wallet` purges a re-keyed subwallet's store state, but the bind tail then restored that subwallet's durable snapshot straight back: notes, activity and watermarks are keyed by `(wallet_id, account_index)` alone, and a seed bind upserts the FVK without deleting the old rows, so the purge was undone from a snapshot the new key did not produce — surfacing unspendable notes and a watermark that hides the new key's own history. Refuse the re-key instead, which is what the durable schema can actually support: `bind_shielded` compares each derived FVK against the persisted row before upserting anything, and the install compares against the current registration. As defense in depth for direct callers, `restore_for_wallet` now skips any subwallet whose snapshot carries a viewing key other than the registered one. The seed path loads the start state once and hands it to the install, so it no longer double-loads; a snapshot that cannot be read is now fatal there, matching the seedless path (and unreachable behind it in the FFI flow, which tries the seedless bind first). Tests would have caught these in CI (each red against the pre-fix logic, green after): - a_second_bind_cannot_commit_inside_another_binds_transaction ✖ left: 2, right: 1 → ✔ - restore_skips_a_snapshot_produced_under_a_different_viewing_key ✖ notes restored → ✔ - bind_rejects_a_viewing_key_change_for_an_already_persisted_account ✖ bind succeeded → ✔ plus install_transaction_blocks_unregister_until_it_commits and bind_after_wallet_removal_is_refused pinning the new contracts. 634 lib tests green; clippy clean on the changed files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e install transaction Serializing the install transaction did not cover the part of a bind that cannot run inside it. Both bind paths must read the host's persisted snapshot BEFORE opening the transaction — the seedless path reconstructs its viewing keys from that snapshot, the seed-backed path checks its derived keys against it, and neither may run a host callback under the coordinator's lifecycle mutex. - A Clear landing in that window was silently undone. The bind then restored pre-Clear notes and the pre-Clear watermark into the store the Clear had just purged — after the host, which wipes its own rows only once clear() returns, had dropped everything. Clear appeared to do nothing, and the restored watermark reported caught-up so the promised cold rebuild from index 0 never ran. quiesce() does not help: it drains sync passes, not binds. Adds a clear generation the bind samples before the load and the install re-reads while holding the mutex; a snapshot that predates a Clear is registered but not restored, with hydration left unset so the next bind hydrates from the host's post-Clear rows. - The detached-wallet refusal ran two host round-trips after bind_shielded had already persisted the viewing-key rows, and hosts delete their wallet data after remove_wallet returns (Swift's deleteWallet calls remove_wallet first). A bind resuming from the mnemonic resolver therefore left a full viewing key on disk for a deleted wallet whose mnemonic was already gone — an FVK discloses every incoming and outgoing note of its account. The check now runs at the top of both bind paths, before any write. - shielded_add_account is the other writer of that row and had no comparison at all. Overwriting an account's key there is undetectable afterwards: the next seedless bind derives its keys FROM the row that was overwritten, so both the registration check and the restore-time key filter compare a key against itself. It now makes the same refusal. Also fixes a latency regression introduced with the transaction: the install holds the lifecycle mutex while taking the wallet's key slot, and the shield path held that slot's read guard across a Halo 2 proof and broadcast, so a shield could stall wallet removal and Clear for every wallet on the network. The shield path now clones the keyset and releases the guard before proving. ShieldedInstall and begin_install become crate-internal: holding a guard across any other lifecycle entry point self-deadlocks, and Rust's temporary scopes make that easy to write by accident. Tests would have caught these in CI (red against the unfixed logic, green after): bind_does_not_restore_a_snapshot_that_predates_a_clear ✖ notes restored → ✔, and add_account_rejects_a_viewing_key_change_for_a_persisted_account ✖ the add succeeded → ✔. bind_after_wallet_removal_is_refused now also asserts that nothing was persisted. The Clear test uses a load-entry handshake rather than a sleep, so it cannot pass by winning a race. 636 lib + 9 integration tests green; clippy and fmt clean; builds with and without the shielded feature. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…cally The mutual-exclusion tests asserted on a 200 ms wall-clock sleep, which measures elapsed time, not exclusion. A peer bind must run two ZIP-32 derivations and round-trip the persister before it even reaches the contended mutex, so on a loaded machine the assertion was satisfied by that peer merely not having been scheduled yet. Measured against a build with the serialization removed: 56% false passes running the test alone under load, 25% running the full suite — a regression test that silently goes green is worse than none. Run them on a paused clock instead (tokio's test-util, already a dev-dependency). `sleep` then fires only once nothing else is runnable, so the assertion reads "the runtime had no work left and the peer still had not committed" — which is the property being claimed. A peer that is merely slow keeps the clock pinned; a peer genuinely parked on the mutex lets it advance. Against the same lock-removed build the tests now catch it 15/15 under load average 62-129, and the suite runs faster (0.44s for the three, no wall-clock waiting). Also adds the coverage the previous commit's `clear()` change had none of: `clear_waits_for_an_in_flight_install_transaction` pins that Clear waits for an open install AND that the store purge is inside the mutex — the purge landing mid-install is precisely what lets a bind restore its snapshot back over a completed Clear. Narrows the concurrent-bind test's doc claim to what it asserts: both binds derive from one seed, so it pins the account-set half of the handle/registration agreement, not the key bytes. 637 lib + 9 integration tests green; fmt and clippy clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ep host I/O out of the key slot Two defects a cross-model review pass found in the preceding commits. A Clear that fails partway had already destroyed restorable state. The subwallet purge runs before the tree reset, so a later failure returns Err with the notes and watermarks gone — while the hydration flag stayed set and the snapshot generation unbumped. The host, told the Clear failed, keeps its own rows; the next identical re-bind then took the idempotent fast path over the emptied store and never restored them, so balances and history stayed missing until a restart. Hydration and the generation are now invalidated whenever the purge ran, on both paths. The registries still survive a failed Clear, deliberately: dropping them would make the coordinator forget every bound wallet while the host keeps its state. `shielded_add_account` held the key slot's write guard across its persister load and store. A host callback invoked under that guard deadlocks against a concurrent bind, which takes the coordinator's lifecycle mutex and then the same slot: the callback re-enters the FFI and waits on the mutex, while the bind holding it waits on the slot. It also froze every address and balance read for the duration of unbounded host I/O. All host calls now happen outside the slot, which is taken twice — once to check, once to insert — with a detach re-check before the mutation and an idempotent insert, since a bind may add the same account in between. Tests would have caught these in CI (red against the unfixed logic, green after): clear_failure_still_invalidates_hydration_and_snapshots ✖ hydration survived → ✔, and add_account_does_not_hold_the_key_slot_across_host_persistence ✖ the slot read timed out → ✔. 639 lib + 9 integration tests green; fmt and clippy clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Issue being fixed or feature implemented
On iOS (rc.2, testnet), a shielded note discovered by a sync pass was not spendable until the app restarted —
IdentityCreateFromShieldedPool/ unshield immediately after a shield failed withShieldedNoUnspentNoteseven though the host's persisted note row existed and was unspent. A second defect in the same area: after a from-scratch rescan, the sync watermark never advanced in-session, so every subsequent pass rescanned the entire tree until restart.Both have one root cause. Hosts re-run
bind_shieldedliberally (the example app fires it twice at launch — a directrebindWalletScopedServices()call plus the wallet-setonChangeobserver — and again on Sync Now and wallet navigation).PlatformWallet::install_shielded_viewsran unregister → purge → restore on every bind. Against an in-flightsync_notes_acrosspass (which holds the store write lock for the whole streamed pass, 45–90 s on testnet), the purge/restore lock acquisitions queue behind the pass and land the moment it finishes — wiping the pass's freshly discovered notes and watermark from the coordinator's in-memory store and restoring a persister snapshot loaded before the pass ran. The host's SwiftData rows survive (the pass's changeset persists synchronously first), which is why an app restart always healed it.Log fingerprint from the failing sessions: Core SPV auto-start (main-actor-queued behind the second bind) begins <0.5 s after pass 1 ends; the next pass reads the pre-pass watermark and re-discovers the same note (
new_notes_total=1twice for one on-chain note). A related interleaving also producedWARN Shielded sync changeset dropped: no persister registered—register_walletinsertedaccountsandpersistersunder two separate lock acquisitions, so a pass starting between them dropped its whole changeset.What was done?
install_shielded_viewsno longer callsunregister_walletat all.register_walletnow purges only the subwallets the new registration drops or re-keys (via the newShieldedStore::purge_subwallet; an account whose FVK changed is treated as dropped — its stored notes belong to the old key) and it replaces, never removes, the persister handle. Retained accounts keep their in-memory notes and watermark across any re-bind — identical or account-set-changing — so a registration replacement racing an in-flight sync pass cannot wipe the pass's results, and a pass finishing mid-bind always finds a persister for its changeset.NetworkShieldedCoordinator::wallet_registration_matches, compared by the canonical 96-byte FVK encoding) and its hydration previously succeeded, the bind skips reloading/re-applying the persister snapshot entirely and never touches the store lock. Side effect: the second launch-time bind no longer blocks the main actor behind the in-flight pass (~50 s launch freeze gone).restore_for_walletis monotonic: the watermark only advances (max), and a snapshot note whose nullifier the store already tracks is skipped —save_noteis overwrite-by-nullifier, so a staleis_spent = falsecopy would otherwise re-offer a spent note to selection (a double-spend attempt at broadcast).register_wallet/unregister_wallet/clear's registry phase, andregister_walletinserts the persister before the accounts. Together these make "accounts visible ⇒ persister visible" hold at every interleaving — including concurrent register + unregister — so a pass can no longer silently drop its changeset (the only remaining accounts-without-persister window is a wallet genuinely being removed mid-pass, where dropping its changeset is intended).next_start_index is 0 after scanning N notes — next sync will rescan from the beginning: since the streaming refactor that value is diagnostic-only (the watermark isaligned_start + total_notes_scanned; the partial buffer chunk is re-covered by chunk-boundary realignment on the next pass), and it fired spuriously on every single-partial-batch cold scan — it pointed the field investigation of this bug at the wrong mechanism.How Has This Been Tested?
wallet_registration_matchesaccepts only the identical wallet + account set + FVK and rejects unknown wallets, changed account sets, and changed keys; a changed-set re-register keeps retained-account state (notes + watermark + persister) while purging only the dropped subwallet; a re-keyed account's old-key notes are purged; and the hydration flag follows the registration lifecycle (preserved on identical re-register, cleared on shape change / unregister).cargo test -p platform-wallet --features shielded --lib: 629 passed, 0 failed.cargo check -p platform-wallet --all-targets --features shieldedandcargo check -p platform-wallet-ffi --features shieldedclean;cargo fmtapplied.End-to-end verification on the iOS simulator (this build)
Rebuilt the sim xcframework + SwiftExampleApp from this branch (
build_info.txtin the session log confirms commitda4e45a3, clean tree) and re-ran the originally failing shield-then-spend flow on testnet, all in one app session with no restart:new_notes_total=1, watermark → 2274)IdentityCreateFromShieldedPool(denomination 3B credits): selection succeeds instantly —inputs=1 total_input=4787148800, i.e. exactly the note discovered 3½ min earlierHVvuKeae6bpLsBNUZK9gN8rYEK7vWxa4Vr7r4pVWD6f1The 3B-credit denomination could only be covered by the fresh note (the only other unspent note was 1.787B), so the spend provably selected the note discovered mid-session — the exact case that returned
ShieldedNoUnspentNotesbefore this fix.Both defects confirmed fixed in the session logs:
Breaking Changes
None.
wallet_registration_matchesis a new public method; existing APIs and persisted formats are unchanged.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code