From 7ce9faeb2a3bacedc27b076c3f150c131ebb4b5c Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Sun, 5 Jul 2026 18:35:40 +0700 Subject: [PATCH 1/3] fix(platform-wallet): data-integrity follow-ups from the #3990 sync review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the CodeRabbit data-integrity findings on the v4.0-dev→v4.1-dev sync (#3990). #3991 tried to fix them but was orphaned when its base (the sync branch) merged, and its diff no longer applied — the reconcile seam grew a `credited_outputs` arg (ADDR-09, #4004/#4005) since. Each fix here is re-derived against current v4.1-dev. - file_store purge: `purge_wallet` / `purge_all_subwallets` now delete the durable `shielded_pending_spends` rows too (they were leaving stale redrive rows that rehydrate ghost reservations after a Clear / unregister). `reset_commitment_tree` deliberately does NOT touch them — a redrive is broadcast state, not tree state. - `SubwalletState::mark_spent` now resolves the reservation + redrive whenever the nullifier is KNOWN, not only on the first unspent→spent transition, so a note restored already-spent still clears its rehydrated redrive. Returns `MarkSpentOutcome { newly_spent, dropped_redrives }` so the file store mirrors the exact SQLite deletion even on the already-spent path (targeted by activity id — zero SQLite work on the common no-redrive path). - `reconcile_address_infos` gains a `_with_persistence` variant reporting whether the changeset was durably stored; `fund_from_asset_lock` gates `consume_asset_lock` on it, so an irreversible Consumed lock is never paired with balance rows that failed to persist (which would under-budget the next spend after a restart). The public signature is unchanged (thin discarding wrapper) so the other seven callers don't churn. - `commit_reconciliation` index conflict: a CREDIT whose derivation index is already paired to a different address is now dropped outright (committing it would evict the pairing or persist a non-round-trippable seed); a REMOVAL still zeroes `found` and is emitted (so the durable zero-out lands and a stale balance can't resurrect) but skips the bijection merge. - `reset_sync_state` holds the provider write lock across BOTH the provider reset and the managed-account balance clear (wallet-manager write nested inside, matching the seam's lock order), so the reset is atomic against a concurrent sync / reconciliation instead of leaving a window between two separate lock scopes. - `register_from_addresses` treats the post-acceptance local `add_identity` as best-effort (warn, don't propagate), so a local persistence failure no longer suppresses the `(identity, address_infos)` return the composite needs to reconcile spent funding addresses. - Swift Clear races: `PlatformBalanceSyncService` gains a `balanceSnapshotGeneration` token (a snapshot in flight when a Clear runs is dropped instead of republishing pre-clear balances) and an `isClearing` flag; `CoreContentView` gates Clear + Sync Now on it. Verified: `cargo test -p platform-wallet` 244 (default) + 362 (shielded, incl. 5 new regression tests: purge-clears-redrive, mark_spent-on-restored-spent-note, index-conflict removal-emit + credit-drop); `cargo fmt --all --check` and `cargo clippy` clean. The Swift changes are app-level (no FFI signature change) and are compiled by the Swift SDK CI check. Original review by CodeRabbit on #3990; fix approach cross-referenced with @thepastaclaw's #3991. Drops the anchor-probe skip-vs-break finding (an earlier adversarial verify on #3977 refuted the harmful variant) and its collateral FFI error code 20. Co-Authored-By: Claude Fable 5 --- .../network/register_from_addresses.rs | 50 +++-- .../fund_from_asset_lock.rs | 33 +++- .../src/wallet/platform_addresses/provider.rs | 184 +++++++++++++++-- .../src/wallet/platform_addresses/wallet.rs | 70 +++++-- .../src/wallet/shielded/file_store.rs | 186 ++++++++++++++++-- .../src/wallet/shielded/store.rs | 56 ++++-- .../Services/PlatformBalanceSyncService.swift | 35 ++++ .../Core/Views/CoreContentView.swift | 14 +- 8 files changed, 548 insertions(+), 80 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/register_from_addresses.rs b/packages/rs-platform-wallet/src/wallet/identity/network/register_from_addresses.rs index 74867714726..710ce8566ff 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/register_from_addresses.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/register_from_addresses.rs @@ -132,21 +132,45 @@ impl IdentityWallet { } let identity = registered_identity; - // Step 3: Add the identity to the local manager (with its HD - // index) so subsequent operations route through it. + // Step 3 (best-effort): add the identity to the local manager + // (with its HD index) so subsequent operations route through it. + // + // Platform has ALREADY accepted the registration, so a local + // persistence failure must NOT propagate as `Err` — that would + // suppress the `(identity, address_infos)` return the caller + // needs to reconcile the spent funding-address balances, leaving + // them stale even though the identity exists on chain. A missed + // local add self-heals on the next identity re-sync. This mirrors + // the sibling `transfer_credits_to_addresses` / top-up flows, + // which likewise treat post-acceptance local bookkeeping as + // best-effort. { let mut wm = self.wallet_manager.write().await; - let info = wm.get_wallet_info_mut(&self.wallet_id).ok_or_else(|| { - crate::error::PlatformWalletError::WalletNotFound( - "Wallet info not found in wallet manager".to_string(), - ) - })?; - info.identity_manager.add_identity( - identity.clone(), - identity_index, - self.wallet_id, - &self.persister, - )?; + match wm.get_wallet_info_mut(&self.wallet_id) { + Some(info) => { + if let Err(e) = info.identity_manager.add_identity( + identity.clone(), + identity_index, + self.wallet_id, + &self.persister, + ) { + tracing::warn!( + error = %e, + identity_id = %identity.id(), + "register_from_addresses: identity registered on Platform but \ + local add_identity failed; returning the registered identity \ + anyway so address balances can reconcile" + ); + } + } + None => { + tracing::warn!( + identity_id = %identity.id(), + "register_from_addresses: identity registered on Platform but wallet \ + info was not found locally; skipping local persistence" + ); + } + } } // The spent platform-address balances are reconciled by the diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs index d33b1690c26..b5c943f8021 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs @@ -302,11 +302,40 @@ impl PlatformAddressWallet { // forcing the next pass to full-scan-reconcile — the automated // equivalent of the manual Sync-tab "Clear" + "Sync Now". let credited_outputs = super::credited_outputs_set(addresses.keys()); - let cs = self - .reconcile_address_infos(&address_infos, &credited_outputs, "fund from asset lock") + // Use the persistence-reporting variant: marking the lock + // `Consumed` below is irreversible, so it MUST be gated on the + // reconciled balances actually reaching disk. `persisted` is + // false ONLY when the in-memory balances were updated but the + // durable write failed — exactly the case where a Consumed lock + // would pair with stale rows and under-budget the next spend + // after a restart. + let (cs, persisted) = self + .reconcile_address_infos_with_persistence( + &address_infos, + &credited_outputs, + "fund from asset lock", + ) .await; if let Some(out_point) = tracked_out_point { + if !persisted { + // The proof-attested balances were applied in memory but + // did not reach disk. Leave the lock non-Consumed: it + // stays in the Resumable Funding list, and a user Resume + // gets Platform's deterministic "lock already consumed" + // rejection — the same benign recovery path as a failed + // consume below — while the next platform-address sync + // repairs the stale rows. Consuming here would strand the + // lock as Consumed over durable balances that under-report + // the credit. + tracing::error!( + outpoint = %out_point, + "skipping consume_asset_lock: the reconciled balance changeset \ + was not durably stored; the lock stays non-Consumed (Resumable) \ + rather than pairing a Consumed lock with stale balance rows on disk" + ); + return Ok(cs); + } // Platform DID accept the top-up — propagating an Err // here would misreport the protocol outcome, since the // caller's recipient(s) already have credits attested diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs index 0b51ed5c2ff..2fa773f08d7 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs @@ -1061,29 +1061,63 @@ impl PlatformPaymentAddressProvider { } } } + // Derivation-index conflict: `entry.address` isn't yet in the + // bijection, but its `address_index` already maps to a + // DIFFERENT address. Detected BEFORE the `found` mutation + // because a conflicting credit must not be half-applied. + let index_conflict = state.addresses.get_by_right(&entry.address).is_none() + && state.addresses.contains_left(&entry.address_index); + + if index_conflict && !is_removal { + // A credit under a conflicting index: dropping it outright + // is the only safe response. Inserting the pairing would + // evict the existing one (`BiBTreeMap::insert` drops + // conflicting pairs, orphaning the other address's `found` + // entry); NOT inserting it would commit a `found` balance + // downstream can't pair with a derivation index, so + // `current_balances` couldn't round-trip the committed + // seed. The address stays unresolved until `initialize` / + // `add_provider` re-snapshots the account set. + tracing::error!( + account_index = entry.account_index, + address_index = entry.address_index, + address = %entry.address, + "commit_reconciliation: derivation index already maps to a \ + different address — dropping the credit reconciliation entry \ + to avoid corrupting the bijection" + ); + continue; + } + if is_removal { state.found.remove(&entry.address); } else { state.found.insert(entry.address, entry.funds); } - // Merge pool-resolved addresses into the bijection so - // `current_balances` can pair the fresh funds with a - // derivation index. Never overwrite an existing pairing — - // `BiBTreeMap::insert` evicts conflicting pairs, which - // would orphan another address's `found` entry. - if state.addresses.get_by_right(&entry.address).is_none() { - if state.addresses.contains_left(&entry.address_index) { - tracing::error!( - account_index = entry.account_index, - address_index = entry.address_index, - address = %entry.address, - "commit_reconciliation: derivation index already \ - maps to a different address — state drift; \ - leaving the bijection untouched" - ); - } else { - state.addresses.insert(entry.address_index, entry.address); - } + + if index_conflict { + // A removal under a conflicting index: the credit case + // already `continue`d above, so this is a zero-out. It + // MUST still zero `found` (done) and be emitted (below) so + // the durable persister writes the zero — otherwise a + // stale persisted balance for this address resurrects + // after restart. Only the bijection merge is skipped, so + // the pre-existing `(index -> other address)` pairing + // survives. + tracing::warn!( + account_index = entry.account_index, + address_index = entry.address_index, + address = %entry.address, + "commit_reconciliation: derivation index already maps to a \ + different address — applying the removal without touching \ + the bijection so a stale persisted balance can't resurrect" + ); + } else if state.addresses.get_by_right(&entry.address).is_none() { + // Merge pool-resolved addresses into the bijection so + // `current_balances` can pair the fresh funds with a + // derivation index. The conflict guard above ruled out an + // eviction, so this insert is always a fresh pairing. + state.addresses.insert(entry.address_index, entry.address); } outcome.entries.push(entry); } @@ -1858,6 +1892,120 @@ mod tests { assert_eq!(seed[0].2, funds(700, 5)); } + /// A zero-funds removal that pool-resolves to an `address_index` + /// already paired to a DIFFERENT address in the bijection must still + /// zero the in-memory `found` row AND be emitted downstream — + /// otherwise a durable persister row for the removed address would + /// resurrect after restart. The bijection stays untouched so the + /// pre-existing `(index -> other addr)` pairing isn't evicted. + #[test] + fn commit_reconciliation_index_conflict_still_emits_removal() { + let owned = p2pkh(0x11); + let conflicting = p2pkh(0x77); + let mut provider = provider_with_one_funded_address(owned, funds(700, 3)); + { + let state = provider + .per_wallet + .get_mut(&WALLET) + .and_then(|s| s.get_mut(&ACCOUNT)) + .expect("account state present"); + // Pin `conflicting` at index 5 with a balance we must protect. + state.insert_persisted_entry(5, conflicting, funds(200, 1)); + // Stale `found` row for the address that will be removed, + // seeded at the SAME index 5 to force the conflict. + state.found.insert(p2pkh(0x22), funds(999, 4)); + } + + // The removed address pool-resolves to index 5 — a DIFFERENT + // address than the bijection holds there. + let removed = p2pkh(0x22); + let removed_addr = PlatformAddress::P2pkh([0x22; 20]); + let mut pool_indexes = BTreeMap::new(); + pool_indexes.insert(removed, (ACCOUNT, 5u32)); + + let mut address_infos = AddressInfos::new(); + // Fully-consumed input: Drive elides the info → removal entry. + address_infos.insert(removed_addr, None); + + let outcome = provider.commit_reconciliation(&WALLET, &address_infos, &pool_indexes); + + // The removal is emitted so the durable persister writes the zero. + assert_eq!(outcome.entries.len(), 1, "removal survives the guard"); + assert_eq!(outcome.entries[0].address, removed); + assert_eq!(outcome.entries[0].funds, funds(0, 0)); + + let state = provider + .per_wallet + .get(&WALLET) + .and_then(|s| s.get(&ACCOUNT)) + .expect("account state present"); + // In-memory `found` for the removed address is dropped. + assert!(!state.found.contains_key(&removed)); + // The bijection is unchanged — pre-existing pairing survives. + assert_eq!(state.addresses.get_by_left(&5u32).copied(), Some(conflicting)); + assert!(state.addresses.get_by_right(&removed).is_none()); + // The protected address's balance is untouched. + assert_eq!(state.found.get(&conflicting).copied(), Some(funds(200, 1))); + } + + /// A CREDIT (non-zero funds) that pool-resolves to an already-taken + /// derivation index must be dropped outright: neither applied to + /// `found` nor emitted, and the bijection untouched. Committing it + /// would either evict the existing pairing or persist a seed + /// `current_balances` can't round-trip. + #[test] + fn commit_reconciliation_index_conflict_drops_credit() { + use dash_sdk::query_types::AddressInfo; + + let owned = p2pkh(0x11); + let conflicting = p2pkh(0x77); + let mut provider = provider_with_one_funded_address(owned, funds(700, 3)); + { + let state = provider + .per_wallet + .get_mut(&WALLET) + .and_then(|s| s.get_mut(&ACCOUNT)) + .expect("account state present"); + state.insert_persisted_entry(5, conflicting, funds(200, 1)); + } + + // A credit for a fresh address that pool-resolves to the taken + // index 5. + let credited = p2pkh(0x33); + let credited_addr = PlatformAddress::P2pkh([0x33; 20]); + let mut pool_indexes = BTreeMap::new(); + pool_indexes.insert(credited, (ACCOUNT, 5u32)); + + let mut address_infos = AddressInfos::new(); + address_infos.insert( + credited_addr, + Some(AddressInfo { + address: credited_addr, + nonce: 2, + balance: 5_000, + }), + ); + + let outcome = provider.commit_reconciliation(&WALLET, &address_infos, &pool_indexes); + + assert!( + outcome.entries.is_empty(), + "a credit under a conflicting index is dropped, not emitted" + ); + let state = provider + .per_wallet + .get(&WALLET) + .and_then(|s| s.get(&ACCOUNT)) + .expect("account state present"); + // `found` never gained the conflicting credit. + assert!(!state.found.contains_key(&credited)); + // Bijection untouched: index 5 still → conflicting, and the + // credited address was not inserted. + assert_eq!(state.addresses.get_by_left(&5u32).copied(), Some(conflicting)); + assert!(state.addresses.get_by_right(&credited).is_none()); + assert_eq!(state.found.get(&conflicting).copied(), Some(funds(200, 1))); + } + /// An entry identical to the committed seed is a no-op and is dropped /// to avoid persister churn. #[test] diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs index dbff6ba6b2e..420688cf6ad 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs @@ -242,8 +242,33 @@ impl PlatformAddressWallet { credited_outputs: &BTreeSet, context: &'static str, ) -> crate::PlatformAddressChangeSet { + self.reconcile_address_infos_with_persistence(address_infos, credited_outputs, context) + .await + .0 + } + + /// Like [`Self::reconcile_address_infos`], but also reports whether the + /// reconciled balance changeset was **durably persisted**. + /// + /// `persisted == false` means (and only means) that the in-memory + /// managed-account balances WERE updated to the proof-attested values + /// but the durable `persister.store(...)` write failed — so a restart + /// would reseed from the stale rows. Every early-return path (no + /// provider, nothing resolved, nothing changed) leaves memory + /// untouched, so there is no memory-vs-disk divergence and + /// `persisted` is `true` there. Callers that pair reconciliation with + /// a one-shot side effect — notably `fund_from_asset_lock` marking an + /// asset lock `Consumed` — must gate that side effect on + /// `persisted == true`, or they risk pairing an irreversible + /// commitment with durable balances that under-report the spend. + pub(super) async fn reconcile_address_infos_with_persistence( + &self, + address_infos: &AddressInfos, + credited_outputs: &BTreeSet, + context: &'static str, + ) -> (crate::PlatformAddressChangeSet, bool) { if address_infos.is_empty() { - return crate::PlatformAddressChangeSet::default(); + return (crate::PlatformAddressChangeSet::default(), true); } let mut guard = self.provider.write().await; @@ -255,7 +280,7 @@ impl PlatformAddressWallet { provider for this wallet; local balances stay stale \ until the next platform-address sync" ); - return crate::PlatformAddressChangeSet::default(); + return (crate::PlatformAddressChangeSet::default(), true); }; if provider.per_wallet_state(&self.wallet_id).is_none() { tracing::warn!( @@ -265,7 +290,7 @@ impl PlatformAddressWallet { provider state for this wallet; local balances stay \ stale until the next platform-address sync" ); - return crate::PlatformAddressChangeSet::default(); + return (crate::PlatformAddressChangeSet::default(), true); } // Live-pool fallback indexes for addresses derived since the last @@ -306,7 +331,7 @@ impl PlatformAddressWallet { address belongs to a third party; otherwise local \ balances stay stale until the next platform-address sync" ); - return crate::PlatformAddressChangeSet::default(); + return (crate::PlatformAddressChangeSet::default(), true); } if outcome.stale_skipped > 0 || outcome.unchanged_skipped > 0 { tracing::debug!( @@ -319,7 +344,7 @@ impl PlatformAddressWallet { ); } if outcome.entries.is_empty() { - return crate::PlatformAddressChangeSet::default(); + return (crate::PlatformAddressChangeSet::default(), true); } // Apply the proof-attested balances to the managed accounts while @@ -384,17 +409,21 @@ impl PlatformAddressWallet { provider.invalidate_sync_watermark(); } - if let Err(e) = self.persister.store(cs.clone().into()) { - tracing::error!( - context, - error = %e, - "Failed to persist platform-address reconciliation; \ - in-memory balances are updated but durable rows stay stale \ - until the next platform-address sync" - ); - } + let persisted = match self.persister.store(cs.clone().into()) { + Ok(()) => true, + Err(e) => { + tracing::error!( + context, + error = %e, + "Failed to persist platform-address reconciliation; \ + in-memory balances are updated but durable rows stay stale \ + until the next platform-address sync" + ); + false + } + }; drop(guard); - cs + (cs, persisted) } /// Get the network from the SDK. @@ -541,6 +570,16 @@ impl PlatformAddressWallet { /// nested-lock hazard; this mirrors the ordering rationale in /// [`initialize_from_persisted`]. pub async fn reset_sync_state(&self) { + // Hold the provider write lock across BOTH the managed-account + // balance clear AND the provider reset, so the whole reset is + // atomic: without it, a sync pass or a reconciliation could + // interleave between the two steps and repopulate the managed + // balances (or the provider seed) we just cleared, leaving the + // "Clear" half-applied. The wallet-manager write is nested + // INSIDE the provider write — the same provider → wallet-manager + // lock order `reconcile_address_infos` uses — so the two paths + // can't deadlock. + let mut guard = self.provider.write().await; { let mut wm = self.wallet_manager.write().await; if let Some(info) = wm.get_wallet_info_mut(&self.wallet_id) { @@ -549,7 +588,6 @@ impl PlatformAddressWallet { } } } - let mut guard = self.provider.write().await; if let Some(provider) = guard.as_mut() { provider.reset_sync_state(); } diff --git a/packages/rs-platform-wallet/src/wallet/shielded/file_store.rs b/packages/rs-platform-wallet/src/wallet/shielded/file_store.rs index 1ea30ea9104..a696739db97 100644 --- a/packages/rs-platform-wallet/src/wallet/shielded/file_store.rs +++ b/packages/rs-platform-wallet/src/wallet/shielded/file_store.rs @@ -225,10 +225,34 @@ impl FileBackedShieldedStore { Ok(conn) } + /// Delete the single persisted redrive row for `id` keyed by + /// `activity_id`. Used to mirror the exact in-memory drops + /// [`SubwalletState::mark_spent`] reports, avoiding the + /// scan-every-row cost of [`Self::delete_redrive_rows_containing`] + /// on the common path where the resolved note had no armed redrive. + fn delete_redrive_row( + &self, + id: SubwalletId, + activity_id: &[u8; 32], + ) -> Result<(), FileShieldedStoreError> { + let conn = self.pending_conn.lock().expect("pending_conn mutex"); + conn.execute( + "DELETE FROM shielded_pending_spends \ + WHERE wallet_id = ?1 AND account_index = ?2 AND activity_id = ?3", + rusqlite::params![ + id.wallet_id.as_slice(), + id.account_index, + activity_id.as_slice(), + ], + ) + .map_err(|e| FileShieldedStoreError(format!("delete redrive row by activity: {e}")))?; + Ok(()) + } + /// Mirror to SQLite the redrive deletions [`SubwalletState`] performs - /// in memory when a nullifier resolves (`mark_spent` / - /// `clear_pending`): delete every persisted row for `id` whose - /// nullifier blob contains `nullifier`. + /// in memory when a nullifier resolves via `clear_pending`: delete + /// every persisted row for `id` whose nullifier blob contains + /// `nullifier`. fn delete_redrive_rows_containing( &self, id: SubwalletId, @@ -295,16 +319,19 @@ impl ShieldedStore for FileBackedShieldedStore { let Some(sw) = self.subwallets.get_mut(&id) else { return Ok(false); }; - let marked = sw.mark_spent(nullifier); - if marked { - // `SubwalletState::mark_spent` dropped any redrive carrying - // this nullifier from memory; mirror the deletions. The - // in-memory transition already happened, so a SQLite failure - // must not abort the call — log it and keep the trait - // behavior consistent. A surviving stale row rehydrates a - // reservation on the next open, which the reconcile / prune - // passes then clear. - if let Err(e) = self.delete_redrive_rows_containing(id, nullifier) { + let outcome = sw.mark_spent(nullifier); + // Mirror the durable deletion whenever the in-memory drop + // happened — keyed on the returned activity ids, NOT on + // `newly_spent`. A note restored already-spent still resolves a + // rehydrated redrive here (`newly_spent == false`), and leaving + // the SQLite row would resurrect the reservation on the next + // open. Targeting the exact activity ids means the common case + // (no armed redrive for this note) issues zero SQLite work. The + // in-memory transition already happened, so a SQLite failure + // only warns — a surviving row rehydrates and self-heals via the + // reconcile / prune passes. + for activity_id in &outcome.dropped_redrives { + if let Err(e) = self.delete_redrive_row(id, activity_id) { tracing::warn!( error = %e, "redrive row deletion failed after mark_spent; a stale row may \ @@ -312,7 +339,7 @@ impl ShieldedStore for FileBackedShieldedStore { ); } } - Ok(marked) + Ok(outcome.newly_spent) } fn mark_pending(&mut self, id: SubwalletId, nullifier: &[u8; 32]) -> Result { @@ -613,11 +640,27 @@ impl ShieldedStore for FileBackedShieldedStore { // in-memory only (`subwallets`); the commitment tree in // SQLite is chain-wide and intentionally left intact. self.subwallets.retain(|id, _| id.wallet_id != wallet_id); + // The redrive table IS durable (unlike the rest of subwallet + // state), so purging the in-memory map alone would leave this + // wallet's rows to rehydrate stale reservations / rebroadcast + // state on the next open. Delete them here too, scoped by + // wallet_id. + let conn = self.pending_conn.lock().expect("pending_conn mutex"); + conn.execute( + "DELETE FROM shielded_pending_spends WHERE wallet_id = ?1", + rusqlite::params![wallet_id.as_slice()], + ) + .map_err(|e| FileShieldedStoreError(format!("purge pending spends for wallet: {e}")))?; Ok(()) } fn purge_all_subwallets(&mut self) -> Result<(), Self::Error> { self.subwallets.clear(); + // Durable redrive rows for every wallet go with the in-memory + // purge (see `purge_wallet`). + let conn = self.pending_conn.lock().expect("pending_conn mutex"); + conn.execute("DELETE FROM shielded_pending_spends", []) + .map_err(|e| FileShieldedStoreError(format!("purge all pending spends: {e}")))?; Ok(()) } @@ -728,6 +771,121 @@ mod tests { let _ = std::fs::remove_file(&path); } + /// Purging a wallet (or all subwallets) must also delete its durable + /// redrive rows — otherwise a Clear / unregister leaves stale rows + /// that rehydrate ghost reservations on the next open. And + /// `reset_commitment_tree` must NOT touch them: a redrive is + /// broadcast state, not tree state, and a tree resync doesn't + /// invalidate an in-flight transition. + #[test] + fn purge_clears_durable_redrive_rows_but_tree_reset_does_not() { + let path = temp_tree_path("purge_redrive"); + let id_a = SubwalletId::new([0xA1; 32], 0); + let id_b = SubwalletId::new([0xB2; 32], 0); + let redrive = |activity: u8, nf: u8| PendingRedrive { + activity_id: [activity; 32], + anchor: [0x22; 32], + nullifiers: vec![[nf; 32]], + st_bytes: vec![0xCD; 32], + attempts: 0, + }; + + // purge_wallet is scoped: it drops A's rows, keeps B's. + { + let mut store = FileBackedShieldedStore::open_path(&path, 100).expect("open"); + store.arm_redrive(id_a, redrive(0x01, 0x0A)).expect("arm a"); + store.arm_redrive(id_b, redrive(0x02, 0x0B)).expect("arm b"); + + // reset_commitment_tree leaves BOTH redrives intact. + store.reset_commitment_tree().expect("reset tree"); + assert_eq!(store.pending_redrives(id_a).expect("a").len(), 1); + assert_eq!(store.pending_redrives(id_b).expect("b").len(), 1); + + store.purge_wallet(id_a.wallet_id).expect("purge a"); + assert!( + store.pending_redrives(id_a).expect("a").is_empty(), + "purge_wallet dropped A's durable redrive rows" + ); + assert_eq!( + store.pending_redrives(id_b).expect("b").len(), + 1, + "purge_wallet is scoped — B's rows survive" + ); + } + // The deletion is durable across reopen; B still rehydrates. + { + let store = FileBackedShieldedStore::open_path(&path, 100).expect("reopen"); + assert!(store.pending_redrives(id_a).expect("a").is_empty()); + assert_eq!(store.pending_redrives(id_b).expect("b").len(), 1); + } + // purge_all_subwallets drops everything, durably. + { + let mut store = FileBackedShieldedStore::open_path(&path, 100).expect("reopen 2"); + store.purge_all_subwallets().expect("purge all"); + assert!(store.pending_redrives(id_b).expect("b").is_empty()); + } + { + let store = FileBackedShieldedStore::open_path(&path, 100).expect("reopen 3"); + assert!(store.pending_redrives(id_b).expect("b").is_empty()); + } + let _ = std::fs::remove_file(&path); + } + + /// `mark_spent` must resolve a rehydrated redrive even for a note + /// that is restored ALREADY spent (its transition landed in a prior + /// session) — the durable SQLite row must be deleted too, not just + /// the in-memory record, or it resurrects the reservation on the + /// next open. + #[test] + fn mark_spent_on_restored_spent_note_clears_durable_redrive() { + let path = temp_tree_path("mark_spent_idempotent"); + let id = SubwalletId::new([0x9; 32], 0); + let nf = [0x3A; 32]; + let note = ShieldedNote { + position: 0, + cmx: [0x1; 32], + nullifier: nf, + block_height: 10, + // Restored from disk ALREADY spent. + is_spent: true, + value: 500, + note_data: vec![0u8; 115], + }; + { + let mut store = FileBackedShieldedStore::open_path(&path, 100).expect("open"); + store.save_note(id, ¬e).expect("save"); + store + .arm_redrive( + id, + PendingRedrive { + activity_id: [0x7; 32], + anchor: [0x22; 32], + nullifiers: vec![nf], + st_bytes: vec![0xEF; 32], + attempts: 0, + }, + ) + .expect("arm"); + + // Already-spent → returns false, but STILL resolves the redrive. + let newly = store.mark_spent(id, &nf).expect("mark_spent"); + assert!(!newly, "note was already spent, so not newly spent"); + assert!( + store.pending_redrives(id).expect("redrives").is_empty(), + "the redrive must be dropped from memory even on the already-spent path" + ); + } + { + // And the durable row was deleted — nothing rehydrates. + let store = FileBackedShieldedStore::open_path(&path, 100).expect("reopen"); + assert!( + store.pending_redrives(id).expect("redrives").is_empty(), + "the SQLite redrive row was mirrored-deleted, not left to rehydrate" + ); + } + let _ = std::fs::remove_file(&path); + } + /// Regression test for the "Shielded Merkle witness /// unavailable" spend failure (multi-wallet shared-tree bug). /// diff --git a/packages/rs-platform-wallet/src/wallet/shielded/store.rs b/packages/rs-platform-wallet/src/wallet/shielded/store.rs index ac47ddc423f..bfb738a3417 100644 --- a/packages/rs-platform-wallet/src/wallet/shielded/store.rs +++ b/packages/rs-platform-wallet/src/wallet/shielded/store.rs @@ -152,6 +152,20 @@ pub struct PendingRedrive { pub attempts: u32, } +/// The result of [`SubwalletState::mark_spent`]. +/// +/// `newly_spent` preserves the historical `bool` return (the +/// unspent→spent transition, which the durable store keys its +/// note-row write on). `dropped_redrives` carries the activity ids of +/// any redrive records resolved by this nullifier so the durable store +/// can mirror the SQLite deletion — even on the already-spent path, +/// where the in-memory drop happens but `newly_spent` is false. +#[derive(Debug, Default, Clone, PartialEq, Eq)] +pub(super) struct MarkSpentOutcome { + pub newly_spent: bool, + pub dropped_redrives: Vec<[u8; 32]>, +} + /// Storage abstraction for shielded wallet state. /// /// Consumers implement this for their persistence layer. The @@ -534,22 +548,29 @@ impl SubwalletState { self.notes.clone() } - pub(super) fn mark_spent(&mut self, nullifier: &[u8; 32]) -> bool { - if let Some(&idx) = self.nullifier_index.get(nullifier) { - if !self.notes[idx].is_spent { - self.notes[idx].is_spent = true; - // Promotion implies the spend confirmed; drop any - // matching pending reservation. Idempotent — the - // common path already cleared pending in the - // spend-flow finalizer. - self.pending_nullifiers.remove(nullifier); - // The spend landed — its redrive record (if armed) is - // resolved for every nullifier it carries. - self.drop_redrives_containing(nullifier); - return true; - } + pub(super) fn mark_spent(&mut self, nullifier: &[u8; 32]) -> MarkSpentOutcome { + let Some(&idx) = self.nullifier_index.get(nullifier) else { + return MarkSpentOutcome::default(); + }; + let newly_spent = !self.notes[idx].is_spent; + self.notes[idx].is_spent = true; + // Resolve the reservation + redrive record whenever the + // nullifier is KNOWN, not only on the first unspent→spent + // transition. A note restored from disk already `is_spent` + // (its owning transition landed in a prior session) paired with + // a rehydrated redrive row would otherwise keep a ghost + // reservation alive and re-broadcast a transition that already + // executed. Removing a pending reservation on a spent note is + // always safe — a spent note can't be re-spent. `dropped` is + // returned so the durable store can mirror the deletion even + // when `newly_spent` is false (the in-memory drop still + // happened here). + self.pending_nullifiers.remove(nullifier); + let dropped = self.drop_redrives_containing(nullifier); + MarkSpentOutcome { + newly_spent, + dropped_redrives: dropped, } - false } /// Drop every redrive record that carries `nullifier`, returning @@ -774,10 +795,13 @@ impl ShieldedStore for InMemoryShieldedStore { } fn mark_spent(&mut self, id: SubwalletId, nullifier: &[u8; 32]) -> Result { + // In-memory store: the redrive map lives in the same + // `SubwalletState`, so `mark_spent` already dropped any resolved + // redrives — nothing durable to mirror. Surface `newly_spent`. Ok(self .subwallets .get_mut(&id) - .map(|sw| sw.mark_spent(nullifier)) + .map(|sw| sw.mark_spent(nullifier).newly_spent) .unwrap_or(false)) } diff --git a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swift b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swift index 142bde94029..4547252d1ed 100644 --- a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swift +++ b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swift @@ -23,6 +23,18 @@ class PlatformBalanceSyncService: ObservableObject { /// Whether a sync is currently in progress. @Published var isSyncing = false + /// Whether a `clearLocalState` is currently in progress. Gates the + /// Clear / Sync Now controls so a second Clear or a Sync Now can't + /// interleave with the Rust reset + SwiftData wipe sequence. + @Published var isClearing = false + + /// Monotonic token bumped at the start of every `clearLocalState`. A + /// `refreshBalanceSnapshot` captures it before its `await` and + /// re-checks it before publishing, so a snapshot that was already in + /// flight when a Clear ran can't republish pre-clear balances over + /// the cleared UI. `@MainActor` serializes the bump and the check. + private var balanceSnapshotGeneration: UInt64 = 0 + /// Last successful sync time (local clock). @Published var lastSyncTime: Date? @@ -227,6 +239,15 @@ class PlatformBalanceSyncService: ObservableObject { network: Network, walletIdsOnNetwork: Set ) async { + // Invalidate any in-flight `refreshBalanceSnapshot`: a snapshot + // queued by a sync that completed just before this Clear must not + // land its (pre-clear) balances over the cleared UI. It captured + // the old generation and will drop its publish once we bump here. + balanceSnapshotGeneration &+= 1 + // Gate the Clear / Sync Now controls for the whole sequence. + isClearing = true + defer { isClearing = false } + // 1) Reset the Rust-owned state BEFORE touching disk. Without // this the in-memory watermark survives and the next "Sync // Now" resumes incrementally (fast) instead of doing a full @@ -368,6 +389,11 @@ class PlatformBalanceSyncService: ObservableObject { private func refreshBalanceSnapshot() async { guard let wallet = platformAddressWallet else { return } + // Capture the generation before the off-actor read. If a + // `clearLocalState` bumps it while we await, the balances below + // are pre-clear and must NOT be published over the cleared UI. + let generation = balanceSnapshotGeneration + do { let (balances, credits) = try await Task.detached { [wallet] in let balances = try wallet.addressesWithBalances() @@ -375,6 +401,15 @@ class PlatformBalanceSyncService: ObservableObject { return (balances, credits) }.value + // A Clear ran while this snapshot was in flight — drop it. + guard generation == balanceSnapshotGeneration else { + SDKLogger.log( + "BLAST snapshot dropped: superseded by a Clear", + minimumLevel: .medium + ) + return + } + var newBalances: [String: UInt64] = [:] var total: UInt64 = 0 var nonZero = 0 diff --git a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift index c2550bec5ad..23797ffc69e 100644 --- a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift +++ b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift @@ -369,7 +369,13 @@ var body: some View { .buttonStyle(.borderedProminent) .tint(.blue) .controlSize(.mini) - .disabled(platformBalanceSyncService.isSyncing) + // Also block Sync Now while a Clear is running so + // it can't interleave with the Rust reset + + // SwiftData wipe. + .disabled( + platformBalanceSyncService.isSyncing + || platformBalanceSyncService.isClearing + ) Button { Task { @@ -387,6 +393,12 @@ var body: some View { .buttonStyle(.borderedProminent) .tint(.red) .controlSize(.mini) + // Clear is fire-and-forget; gate it against a + // concurrent sync and against a second Clear. + .disabled( + platformBalanceSyncService.isSyncing + || platformBalanceSyncService.isClearing + ) } } .padding(.vertical, 4) From abf952eacefe0a1225550de5f0469d2f5e7175c5 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Sun, 5 Jul 2026 18:48:17 +0700 Subject: [PATCH 2/3] chore: cargo fmt Co-Authored-By: Claude Fable 5 --- .../src/wallet/platform_addresses/provider.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs index 2fa773f08d7..8b1df4d02b9 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs @@ -1942,7 +1942,10 @@ mod tests { // In-memory `found` for the removed address is dropped. assert!(!state.found.contains_key(&removed)); // The bijection is unchanged — pre-existing pairing survives. - assert_eq!(state.addresses.get_by_left(&5u32).copied(), Some(conflicting)); + assert_eq!( + state.addresses.get_by_left(&5u32).copied(), + Some(conflicting) + ); assert!(state.addresses.get_by_right(&removed).is_none()); // The protected address's balance is untouched. assert_eq!(state.found.get(&conflicting).copied(), Some(funds(200, 1))); @@ -2001,7 +2004,10 @@ mod tests { assert!(!state.found.contains_key(&credited)); // Bijection untouched: index 5 still → conflicting, and the // credited address was not inserted. - assert_eq!(state.addresses.get_by_left(&5u32).copied(), Some(conflicting)); + assert_eq!( + state.addresses.get_by_left(&5u32).copied(), + Some(conflicting) + ); assert!(state.addresses.get_by_right(&credited).is_none()); assert_eq!(state.found.get(&conflicting).copied(), Some(funds(200, 1))); } From 153e51c603dbb1d0dc7b16f1eb54e858a20cdf28 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Sun, 5 Jul 2026 19:06:48 +0700 Subject: [PATCH 3/3] fix(platform-wallet): post-review hardening from self-review + thepastaclaw MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - purge_wallet/purge_all_subwallets: run the SQLite redrive DELETE BEFORE the in-memory purge, so an Err leaves both stores untouched (fail-atomic) instead of a memory purge the caller can't observe. - clear_redrive: delegate the durable delete to delete_redrive_row instead of carrying a second copy of the same SQL. - reset_sync_state: fix the stale doc paragraph that still claimed the two locks are taken sequentially — the wallet-manager write is now nested inside the provider write. - PlatformBalanceSyncService: bump balanceSnapshotGeneration in reset()/configure() too — a snapshot in flight across a wallet or network switch strongly captured the old wallet handle and would publish its balances over the new configuration. - performSync(): guard on isClearing at the service level — pull-to- refresh and the post-submit resyncs bypass CoreContentView's button gating and could start a sync mid-Clear. Co-Authored-By: Claude Fable 5 --- .../src/wallet/platform_addresses/wallet.rs | 8 +-- .../src/wallet/shielded/file_store.rs | 52 +++++++++---------- .../Services/PlatformBalanceSyncService.swift | 14 +++++ 3 files changed, 42 insertions(+), 32 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs index 420688cf6ad..f2d0fae4bf8 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs @@ -565,10 +565,10 @@ impl PlatformAddressWallet { /// /// Does NOT route through [`apply_sync_state`] — that helper's /// all-None early-return guard is meant for persisted-state replay - /// and is irrelevant here. The two locks are taken sequentially - /// (one released before the next is acquired), so there is no - /// nested-lock hazard; this mirrors the ordering rationale in - /// [`initialize_from_persisted`]. + /// and is irrelevant here. The wallet-manager write is nested + /// INSIDE the provider write (provider → wallet-manager, the same + /// order `reconcile_address_infos` uses) so the two clears are + /// atomic with respect to a concurrent sync or reconciliation. pub async fn reset_sync_state(&self) { // Hold the provider write lock across BOTH the managed-account // balance clear AND the provider reset, so the whole reset is diff --git a/packages/rs-platform-wallet/src/wallet/shielded/file_store.rs b/packages/rs-platform-wallet/src/wallet/shielded/file_store.rs index a696739db97..d9cc3443633 100644 --- a/packages/rs-platform-wallet/src/wallet/shielded/file_store.rs +++ b/packages/rs-platform-wallet/src/wallet/shielded/file_store.rs @@ -471,18 +471,7 @@ impl ShieldedStore for FileBackedShieldedStore { if let Some(sw) = self.subwallets.get_mut(&id) { sw.clear_redrive(activity_id); } - let conn = self.pending_conn.lock().expect("pending_conn mutex"); - conn.execute( - "DELETE FROM shielded_pending_spends \ - WHERE wallet_id = ?1 AND account_index = ?2 AND activity_id = ?3", - rusqlite::params![ - id.wallet_id.as_slice(), - id.account_index, - activity_id.as_slice(), - ], - ) - .map_err(|e| FileShieldedStoreError(format!("clear redrive: {e}")))?; - Ok(()) + self.delete_redrive_row(id, activity_id) } fn record_outgoing_note( @@ -636,31 +625,38 @@ impl ShieldedStore for FileBackedShieldedStore { } fn purge_wallet(&mut self, wallet_id: WalletId) -> Result<(), Self::Error> { + // The redrive table IS durable (unlike the rest of subwallet + // state), so purging the in-memory map alone would leave this + // wallet's rows to rehydrate stale reservations / rebroadcast + // state on the next open. Delete them first, scoped by + // wallet_id — SQL before memory, so an Err return means neither + // store was touched (fail-atomic) rather than a memory purge + // the caller can't distinguish from a no-op. + { + let conn = self.pending_conn.lock().expect("pending_conn mutex"); + conn.execute( + "DELETE FROM shielded_pending_spends WHERE wallet_id = ?1", + rusqlite::params![wallet_id.as_slice()], + ) + .map_err(|e| FileShieldedStoreError(format!("purge pending spends for wallet: {e}")))?; + } // Per-subwallet note / watermark / checkpoint state is // in-memory only (`subwallets`); the commitment tree in // SQLite is chain-wide and intentionally left intact. self.subwallets.retain(|id, _| id.wallet_id != wallet_id); - // The redrive table IS durable (unlike the rest of subwallet - // state), so purging the in-memory map alone would leave this - // wallet's rows to rehydrate stale reservations / rebroadcast - // state on the next open. Delete them here too, scoped by - // wallet_id. - let conn = self.pending_conn.lock().expect("pending_conn mutex"); - conn.execute( - "DELETE FROM shielded_pending_spends WHERE wallet_id = ?1", - rusqlite::params![wallet_id.as_slice()], - ) - .map_err(|e| FileShieldedStoreError(format!("purge pending spends for wallet: {e}")))?; Ok(()) } fn purge_all_subwallets(&mut self) -> Result<(), Self::Error> { - self.subwallets.clear(); // Durable redrive rows for every wallet go with the in-memory - // purge (see `purge_wallet`). - let conn = self.pending_conn.lock().expect("pending_conn mutex"); - conn.execute("DELETE FROM shielded_pending_spends", []) - .map_err(|e| FileShieldedStoreError(format!("purge all pending spends: {e}")))?; + // purge; SQL first for the same fail-atomic reason as + // `purge_wallet`. + { + let conn = self.pending_conn.lock().expect("pending_conn mutex"); + conn.execute("DELETE FROM shielded_pending_spends", []) + .map_err(|e| FileShieldedStoreError(format!("purge all pending spends: {e}")))?; + } + self.subwallets.clear(); Ok(()) } diff --git a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swift b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swift index 4547252d1ed..872db6b1a9a 100644 --- a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swift +++ b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swift @@ -109,6 +109,9 @@ class PlatformBalanceSyncService: ObservableObject { persistenceHandler: PlatformWalletPersistenceHandler? = nil, walletId: Data? = nil ) { + // Same invalidation as `reset()`: a snapshot still in flight for + // the previous wallet must not publish over this configuration. + balanceSnapshotGeneration &+= 1 self.platformAddressWallet = platformAddressWallet self.walletManager = walletManager self.persistenceHandler = persistenceHandler @@ -195,6 +198,11 @@ class PlatformBalanceSyncService: ObservableObject { /// Use for wallet deletion or network switch. Caller must re-configure /// before the next sync. func reset() { + // Invalidate any in-flight `refreshBalanceSnapshot` — its + // detached task captured the OLD wallet handle strongly, so + // without this bump a snapshot racing a wallet/network switch + // would publish the old wallet's balances over the reset UI. + balanceSnapshotGeneration &+= 1 clearDisplay() platformAddressWallet = nil walletManager = nil @@ -329,6 +337,12 @@ class PlatformBalanceSyncService: ObservableObject { /// reset locally regardless of outcome. func performSync() async { guard !isSyncing else { return } + // The Clear/Sync mutual exclusion must live here, not only on + // CoreContentView's buttons: pull-to-refresh (Wallets/Identities + // tabs) and the post-submit resyncs (Transfer/Withdraw views) + // call performSync directly and would otherwise start a sync + // between clearLocalState's Rust reset and its SwiftData wipe. + guard !isClearing else { return } guard let walletManager = walletManager else { lastError = "Platform address wallet not configured" return