fix(platform-wallet): data-integrity follow-ups from the #3990 sync review

[QuantumExplorer]

Issue being fixed or feature implemented

Addresses the CodeRabbit data-integrity findings raised on the v4.0-dev → v4.1-dev sync (#3990). Those findings are against content that is now on v4.1-dev; they were never fixed — #3991 prepared fixes but was orphaned/closed when its base (the sync branch) merged, and its diff no longer applies because the reconcile seam grew a credited_outputs argument in the ADDR-09 work (#4004/#4005) since. Each fix here is re-derived against current v4.1-dev and independently verified, not cherry-picked.

What was done?

Shielded store (durable redrive lifecycle — gaps from #3988):

• purge_wallet / purge_all_subwallets now also delete the durable shielded_pending_spends rows. They were only clearing in-memory state, so a Clear / unregister left stale redrive rows that rehydrate ghost reservations on the next open. reset_commitment_tree deliberately does not touch them — a redrive is broadcast state, not tree state.
• SubwalletState::mark_spent 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. It returns MarkSpentOutcome { newly_spent, dropped_redrives } so the file store mirrors the exact SQLite deletion on the already-spent path too (targeted by activity id, so the common no-redrive path issues zero SQLite work).

Address reconciliation seam:

• reconcile_address_infos gains a _with_persistence variant that reports whether the changeset was durably stored. fund_from_asset_lock gates consume_asset_lock on it: 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 already maps to a different address is dropped outright (committing it would evict the pairing via BiBTreeMap::insert, orphaning another address's found, or persist a seed current_balances can't round-trip); 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 provider→wallet-manager lock order), so the reset is atomic against a concurrent sync / reconciliation rather than leaving a window between two separate lock scopes.

Identity registration:

• register_from_addresses treats the post-acceptance local add_identity as best-effort (warn, don't propagate) — mirroring the transfer / top-up flows — so a local persistence failure no longer suppresses the (identity, address_infos) return the composite needs to reconcile spent funding addresses.

SwiftExampleApp Clear races:

• PlatformBalanceSyncService gains a balanceSnapshotGeneration token: a refreshBalanceSnapshot that was in flight when a Clear runs is dropped instead of republishing pre-clear balances over the cleared UI. It also gains an isClearing flag, and CoreContentView gates both Clear and Sync Now on it (Clear is fire-and-forget, so it could otherwise interleave with the Rust reset + SwiftData wipe).

How Has This Been Tested?

• cargo test -p platform-wallet --lib → 244 passed (default features); --features shielded → 362 passed.
• 5 new regression tests: purge_clears_durable_redrive_rows_but_tree_reset_does_not, mark_spent_on_restored_spent_note_clears_durable_redrive, commit_reconciliation_index_conflict_still_emits_removal, commit_reconciliation_index_conflict_drops_credit.
• cargo fmt --all -- --check clean; cargo clippy -p platform-wallet --features shielded clean (only pre-existing workspace-profile notes).
• The two Swift changes are app-level (no FFI signature change) and are compiled by the repo's Swift SDK build + tests CI check.

Not covered (deferred / reasoned): the lock-ordering (reset_sync_state) and best-effort (register_from_addresses) fixes are correctness-by-construction and covered by the existing integration suites rather than dedicated unit tests; the asset-lock consume gate's persist-failure branch needs an erroring-persister harness and is covered by reasoning + the plain-path integration test.

Breaking Changes

None. MarkSpentOutcome is a pub(super) internal type; the public reconcile_address_infos signature is unchanged.

Notes for reviewers

Original findings by CodeRabbit on #3990; fix approach cross-referenced with @thepastaclaw's #3991. I dropped the anchor-probe "skip-vs-break" finding — an earlier adversarial verification on #3977 refuted the harmful variant (deeper probes fail identically; the "fix" would make every routine mid-block spend probe all 100 depths) — and with it the collateral ErrorShieldedMerkleWitnessUnavailable = 20 FFI code, since not adding an ABI code we don't need is strictly better.

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
• I have made corresponding changes to the documentation if needed

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

@QuantumExplorer, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 8 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e523d92b-9f64-428e-9bba-e8c9ec3d316b

📥 Commits

Reviewing files that changed from the base of the PR and between e8b9da2 and 153e51c.

📒 Files selected for processing (8)

• packages/rs-platform-wallet/src/wallet/identity/network/register_from_addresses.rs
• packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs
• packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs
• packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs
• packages/rs-platform-wallet/src/wallet/shielded/file_store.rs
• packages/rs-platform-wallet/src/wallet/shielded/store.rs
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/post-sync-review-followups

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[thepastaclaw]

✅ Review complete (commit 153e51c)

[thepastaclaw]

Code Review

PR delivers narrow data-integrity follow-ups (durable redrive lifecycle, persistence-gated asset-lock consume, address-reconciliation index-conflict handling, Swift Clear/Sync race gating). Verification against 7ce9fae confirms invariants match the extensive rationale comments; no blocking issues. Two suggestion-level polish items on the shielded-store purge ordering and the fund_from_asset_lock non-persisted return signal are worth flagging.

🟡 2 suggestion(s)

Source: reviewers claude-general/security-auditor/rust-quality/ffi-engineer (opus); codex-general/security-auditor/rust-quality/ffi-engineer (gpt-5.5[high], failed/unparseable); verifier claude (opus).

🤖 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/file_store.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:638-665: Durable redrive DELETE runs after the in-memory purge, so a SQL failure leaves memory and disk inconsistent
  `purge_wallet` and `purge_all_subwallets` clear `self.subwallets` before running `DELETE FROM shielded_pending_spends`. If the SQL DELETE fails, both functions return `Err`, but the in-memory purge has already committed — the exact memory/disk divergence this PR is fixing on the `mark_spent` path. Two consistent options: (a) run the SQL DELETE first, then purge memory, so an `Err` return leaves both stores unchanged (fail-atomic); or (b) match the `mark_spent` / `clear_pending` error posture in this file — `warn!` the SQL failure and return `Ok(())` because the in-memory purge is already authoritative. Either is preferable to the current shape, where callers see `Err` and cannot tell whether the memory purge landed.

In `packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs:320-338: `fund_from_asset_lock` returns `Ok(cs)` identically on the durable-success and skip-consume paths
  When `persisted == false`, the function returns `Ok(cs)` — the same shape as the fully-successful path — after logging at `error!` and skipping `consume_asset_lock`. The rationale (Platform will deterministically reject a Resume with 'lock already consumed') is correct, but the return signal collapses two very different outcomes: 'top-up landed and is durable' vs. 'top-up applied in memory only, lock stays Resumable, balance may resurface stale after restart'. Downstream UI aggregation and telemetry can't distinguish the two. A distinct variant or a boolean on the returned changeset would let callers surface the discrepancy rather than treat it as full success. Non-blocking — current behavior is correct, just opaque.

[thepastaclaw]

Code Review

Latest push (7ce9fae → abf952e) is a pure cargo fmt on two test assertions in platform_addresses/provider.rs — no behavioral change. Two prior suggestion-level findings from the shielded-store purge and fund_from_asset_lock persistence-gated paths still apply at the current head. No new findings from the delta; no blocking issues.

🟡 2 carried-forward suggestion(s) | 0 new latest-delta findings

Prior finding reconciliation

• STILL VALID: packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:638-665 — purge_wallet / purge_all_subwallets mutate memory before the durable DELETE, so a SQL failure returns Err with memory already purged
• STILL VALID: packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs:320-338 — fund_from_asset_lock returns Ok(cs) identically on the durable-success and skip-consume paths

Latest delta
7ce9faeb..abf952ea only changes packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs formatting/test assertion layout. No new latest-delta findings.

Source: reviewers claude-general/security-auditor/rust-quality/ffi-engineer (opus, ok); codex-general/security-auditor/rust-quality/ffi-engineer (gpt-5.5[high], failed/unparseable: ACP advertises gpt-5.5 not gpt-5.5[high]); verifier claude (opus).

🤖 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.

Carried-forward prior findings still valid at abf952ea:

In `packages/rs-platform-wallet/src/wallet/shielded/file_store.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:638-665: purge_wallet / purge_all_subwallets mutate memory before the durable DELETE, so a SQL failure returns Err with memory already purged
  `purge_wallet` clears `self.subwallets` at line 642 before executing `DELETE FROM shielded_pending_spends WHERE wallet_id = ?1` at line 649; `purge_all_subwallets` does the same at line 658 before the unscoped DELETE at line 662. If the SQL execute fails, both functions map the error and return `Err(...)` — but the in-memory purge has already committed. Callers cannot distinguish 'nothing happened' from 'memory purged, disk unchanged', which is exactly the memory/disk divergence this PR is otherwise fixing on the mark_spent path. Two consistent fixes: (a) run the SQL DELETE first, then purge memory only on success (fail-atomic); or (b) match the log-don't-abort posture used earlier in this same file for `mark_spent` and `clear_pending` — `warn!` the SQL failure and return `Ok(())` because the in-memory purge is already authoritative and the durable rows will be reconciled on the next open.

In `packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs:320-338: fund_from_asset_lock returns Ok(cs) identically on the durable-success and skip-consume paths
  When `persisted == false`, the function returns `Ok(cs)` at line 337 after logging at `error!` and skipping `consume_asset_lock` — the same return shape as the fully-successful path further down. The persistence-gate itself is the correct invariant (a Consumed lock over non-durable balances would under-budget after restart), and Platform will deterministically reject a Resume with 'lock already consumed', but the collapsed return signal makes 'top-up landed and is durable' indistinguishable from 'top-up applied in memory only, lock stays Resumable, balance may resurface stale after restart'. Downstream UI aggregation and telemetry cannot detect the degraded outcome. A distinct variant on the returned changeset — or a `persisted: bool` companion mirroring the `_with_persistence` reconciliation seam this already calls — would let callers surface the discrepancy rather than treat it as full success. Non-blocking; behavior is correct, just opaque.

[QuantumExplorer]

Self-review results (8 finder angles × adversarial verification)

Ran a full multi-angle review of this PR. Fixed in 153e51c603 (plus abf952eace fmt):

• purge ordering — SQLite DELETE now runs before the in-memory purge (fail-atomic); also flagged by thepastaclaw.
• performSync() gate — the Clear/Sync mutual exclusion now lives in the service (guard !isClearing), not just on CoreContentView's buttons; pull-to-refresh (Wallets/Identities) and the post-submit resyncs (Transfer/Withdraw) called performSync directly and could start a sync mid-Clear.
• wallet-switch generation gap — reset()/configure() now bump balanceSnapshotGeneration; an in-flight snapshot strongly captures the old wallet handle and would otherwise publish the old wallet's balances over the new configuration.
• stale doc comment — reset_sync_state's outer doc still claimed the two locks are taken sequentially; rewritten to describe the nested provider → wallet-manager order.
• SQL dedup — clear_redrive now delegates its durable delete to delete_redrive_row.

Deferred (tracked separately, both pre-existing):

1. Index-conflicting removals are still emitted carrying the conflicting address_index; the Swift persister writes it onto the zero-balance row, so two durable rows can claim one index and the restore-time bimap insert may evict the protected pairing. Needs a persister-contract fix (sentinel or skip-index-on-zero-balance) — deeper than this PR.
2. registration.rs (register_identity_with_funding) still propagates post-acceptance add_identity failure with ? and skips consume_asset_lock, the exact hazard class this PR fixes in register_from_addresses.rs.

Verified-refuted (for the record): the vacuous persisted=true early returns are safe (credits are proof-attested on-chain; consuming is truthful), best-effort add_identity cannot cause index reuse (Swift derives the next index and the discovery rescan self-heals), and the rehydrated-ghost-redrive scenario self-heals because the shielded watermark is in-memory (every reopen rescans from 0 and re-encounters the nullifier). The index-conflict credit drop self-heals within one sync round via prepare_for_sync's bijection rebuild, so the drop-don't-apply choice stands.

🤖 Generated with Claude Code

[thepastaclaw]

Code Review

Latest push (abf952ea → 153e51c6) fixes one of the two carried-forward findings and leaves the other still valid. purge_wallet / purge_all_subwallets now delete durable redrive rows before mutating memory, so the prior fail-atomicity concern is fixed. fund_from_asset_lock still returns the same Ok(cs) for durable success and the persistence-failure skip-consume path, so that suggestion carries forward. No new latest-delta findings and no blockers.

🟡 1 carried-forward suggestion(s) | 0 new latest-delta findings

Prior finding reconciliation

• FIXED: packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:638-665 — purge ordering now runs SQL before memory in both purge paths.
• STILL VALID: packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs:320-338 — fund_from_asset_lock still returns Ok(cs) identically on durable-success and skip-consume paths.

Latest delta
abf952ea..153e51c6 touches wallet.rs, shielded/file_store.rs, and PlatformBalanceSyncService.swift. It fixes the shielded purge ordering, updates the reset-sync lock-order doc, and adds Swift-side snapshot/clear-sync hardening. No new latest-delta findings.

Source: reviewers claude-general (opus, ok); codex-general (gpt-5.5, ok); claude-security-auditor (opus, ok); codex-security-auditor (gpt-5.5, ok); claude-rust-quality (opus, ok); codex-rust-quality (gpt-5.5, ok); claude-ffi-engineer (opus, ok); codex-ffi-engineer (gpt-5.5, ok); verifier claude (opus).

🤖 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.

Carried-forward prior findings still valid at 153e51c6:

In `packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs:320-338: fund_from_asset_lock returns Ok(cs) identically on the durable-success and skip-consume paths
  Carried forward from prior-2; unchanged at 153e51c6 (git diff of this file across abf952ea..HEAD is empty). When `persisted == false`, the function logs at `error!`, skips `consume_asset_lock`, and returns `Ok(cs)` at line 337 — the same return shape as the fully-successful path further down. The persistence-gate itself is the correct invariant (a Consumed lock over non-durable balances would under-budget after restart, and Platform will deterministically reject a Resume with 'lock already consumed'), but the collapsed return signal makes 'top-up landed and is durable' indistinguishable from 'top-up applied in memory only, lock stays Resumable, balance may resurface stale after restart'. The signal loss propagates through the C FFI (`rs-platform-wallet-ffi/src/platform_addresses/fund_from_asset_lock.rs:116-119` converts to `PlatformWalletFFIResult::ok()`) and through Swift (`ManagedPlatformAddressWallet.swift:614-617` decodes as a successful `[UpdatedBalance]`), so downstream UI aggregation and telemetry cannot detect the degraded outcome. A distinct variant on the returned changeset — or a `persisted: bool` companion mirroring the `_with_persistence` reconciliation seam this already calls — would let callers surface the discrepancy rather than treat it as full success. Consistent with the fail-atomic reordering just applied to `purge_wallet` / `purge_all_subwallets`, this is the last place in the data-integrity series where the caller can't distinguish partial-success from full-success. Non-blocking; behavior is correct, just opaque.