fix(platform-wallet): preserve persisted wallet state through seedless rehydration

[QuantumExplorer]

Issue being fixed or feature implemented

Suggested fixes for the findings of a code review of #3692 (see the review comment there for the full findings list with verification details). The confirmed defects all share one root cause: the keyless projection in the FFI persister flattens state the store already holds exactly — per-account UTXO attribution, pool contents with per-index used flags, tx records — and the manager-side replay cannot reconstruct what the flattening discarded. Concretely:

1. Address-reuse wedge (rehydrate.rs): mark_used ran once before maintain_gap_limit, so a previously-used address past the discovery horizon but within gap-refill reach (e.g. used idx 45 with in-window used idx 20, gap 30) was derived after the mark pass and came back used = false — pool-visible as a fresh receive address, with the stale flag then persisted back over the store's is_used = true.
2. Attribution loss: every restored UTXO was routed to the first funds-bearing account, zeroing non-first-account (CoinJoin/BIP32/DashPay) balances and leaving their addresses unresolvable from the wrong xpub — although the FFI decode loop had already routed each UTXO to its exact account.
3. Watch-set shrinkage: only used addresses crossed the boundary, so derived-but-unused (handed-out) addresses and everything past the rebuilt depth fell out of the SPV watch set until a full rescan.
4. Swift discarded the outcome: a Rust-skipped corrupt wallet still went through the get_wallet loop, landing a spurious NotFound in the @Published lastError, and the user was never told a wallet was skipped.

What was done?

• Carry the snapshot instead of projecting it: ClientWalletStartState gains core_wallet_info: Option<Box<ManagedWalletInfo>> — still keyless by type (no Wallet, no seed; ManagedWalletInfo holds balances/pools/UTXOs only). The FFI persister hands its fully-restored wallet_info across; the manager validates wallet_id/network against the row (mismatch ⇒ per-row corrupt skip) and consumes it directly. This fixes attribution, watch-set coverage, and used-flag fidelity in one move, restores the asset-lock funding-tx records to the in-memory map, deletes the projection block from the FFI crate (per packages/swift-sdk/CLAUDE.md, stitching belongs in the library), and removes the second eager gap-window derivation from the launch path. core_state/used_core_addresses remain as the documented fallback for persisters that can't build a snapshot (SQLite until feat(platform-wallet): persistence readers + seedless load() wiring (split from #3692) #3968).
• Mark↔refill fixpoint in extend_pools_for_restored_addresses (still used by the fallback path): re-mark after every gap refill until nothing new resolves, so wedge-zone used addresses come back used.
• Idempotency check hoisted to the top of the load loop (cheap read-lock get_wallet), so a repeat load_from_persistor skips per-wallet reconstruction instead of discarding it at the WalletExists arm (which stays as a race backstop).
• Swift consumes LoadOutcomeFFI: skipped wallets are logged with a decoded reason, surfaced via a new @Published lastLoadSkippedWallets: [SkippedWalletOnLoad], excluded from the get_wallet loop (no more lastError pollution), and the outcome is freed via platform_wallet_load_outcome_free (deferred ahead of the throwing check()).
• Architecture record: packages/rs-platform-wallet/README.md gains a normative "Persistence architecture" section codifying the agreed model (single writer, store-as-read-model, versioned schema contract, verbatim load(), hard persist errors, seedless load) — see the execution-plan comment on feat(platform-wallet): watch-only rehydration from persistor (seedless load) #3692.
• Cleanups: RehydrateRowError (variant-for-variant mirror of CorruptKind + identity From) deleted — build_watch_only_wallet returns CorruptKind directly; the FFI skipped-array allocation reuses the crate's vec_to_ptr helper.

How Has This Been Tested?

• cargo test -p platform-wallet — 217 lib + 8 + 7 integration, all passing. New tests:
  • rehydration_wedge_zone_used_address_marked_after_refill (fails without the fixpoint fix),
  • rt_snapshot_preserves_attribution_and_pools (CoinJoin-account UTXO stays attributed; derived-but-unused deep address survives reload),
  • rt_snapshot_wallet_id_mismatch_is_skipped (snapshot/row mismatch ⇒ per-row skip, not a load).
• cargo test -p platform-wallet-ffi --features shielded — 111 + 26 + 5, all passing.
• cargo clippy -p platform-wallet -p platform-wallet-ffi --all-targets -- -D warnings clean; cargo fmt --all --check clean; cargo check --all-targets clean on the downstream platform-wallet-storage and rs-unified-sdk-ffi crates.
• Swift file changed in lockstep with the FFI signature; like the base PR, it still needs an iOS build check (the xcframework isn't built in this environment).

Breaking Changes

None beyond the base PR's own unreleased shapes (ClientWalletStartState gains a field; build_watch_only_wallet's error type changes — both introduced by #3692 and unreleased).

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

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f2720b80-1de9-4e4e-a2eb-f0f0f83d5ec7

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fable/3692-review-fixes

---

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 51cd9c9)

[thepastaclaw]

Code Review

PR #3980 cleanly addresses the four seedless-rehydration defects from #3692 (attribution loss, watch-set shrinkage, wedge-zone address reuse, Swift discarding LoadOutcome). Snapshot path is guarded by wallet_id/network fail-closed, mark↔refill fixpoint is correctly bounded on a finite restored-address set, FFI ownership/free contract is symmetric with a zero-init on early returns, and Swift now consumes and frees the outcome. No blockers; a handful of non-blocking suggestions and nitpicks — mostly about type-safety of the snapshot-vs-projection union, the DecodeError reuse for identity-mismatch, and Swift/Rust constant coupling.

🟡 5 suggestion(s) | 💬 3 nitpick(s)

🤖 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/changeset/client_wallet_start_state.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/client_wallet_start_state.rs:41-66: Snapshot vs projection fields form an implicit-invariant union
  `ClientWalletStartState` now carries three overlapping sources of truth for core state: `core_wallet_info: Option<Box<ManagedWalletInfo>>`, `core_state: CoreChangeSet`, and `used_core_addresses: Vec<Address>`. The rule "if `core_wallet_info` is `Some`, ignore the other two" lives only in the load-loop match arm and a doc comment; a persister that populates both a snapshot and a projection would silently discard the projection with no compile-time signal. A tagged enum such as `enum CoreLoadState { Snapshot(Box<ManagedWalletInfo>), Projection { core_state: CoreChangeSet, used_core_addresses: Vec<Address> } }` would make the mutual exclusion type-level and remove the FFI persister's need to `Default::default()` the unused fields. Not a bug today but locks in the invariant the docs are pleading for, right before #3968 adds a second persister backend.

In `packages/rs-platform-wallet/src/manager/load.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/load.rs:162-220: Snapshot path validates wallet_id/network but not manifest ↔ snapshot account consistency
  When `core_wallet_info: Some(info)` is consumed, the manager stitches a watch-only `Wallet` built from `account_manifest` together with `ManagedWalletInfo.accounts` from the snapshot, but only `wallet_id`/`network` are cross-checked. If a future persister populates the manifest and the snapshot from divergent sources (extra CoinJoin account only in one, mis-ordered types, etc.) `Wallet.accounts` and `wallet_info.accounts` disagree silently: one drives scanning/derivation while the other drives balances/UTXOs. The current FFI persister derives both from the same in-memory state so this isn't user-triggerable today, but `PlatformWalletPersistence` is a public trait and #3968 will add a SQLite backend. Even a cheap structural check (assert the account-type sets match) — or at minimum a doc line making the assumption explicit — would extend the existing wallet_id/network fail-closed pattern to the account collection.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/load.rs:175-190: Snapshot wallet_id/network mismatch reuses CorruptKind::DecodeError, obscuring the failure mode
  A wallet_id/network mismatch on the persisted `core_wallet_info` snapshot is reported via `CorruptKind::DecodeError`, which the FFI maps to `LOAD_SKIP_REASON_DECODE_ERROR` (102) — the same code as a genuine bincode failure. These are semantically distinct: one means the persisted bytes are unreadable, the other means a persister returned a snapshot for the wrong row (a stronger data-integrity signal, e.g. persister row mixup). `CorruptKind` is `#[non_exhaustive]`, so a dedicated `SnapshotIdentityMismatch` variant (plus a new `LOAD_SKIP_REASON_*` code) is a low-cost, backward-compatible change that would make debug logs and Swift-side triage clearer.

In `packages/rs-platform-wallet/src/manager/rehydrate.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/rehydrate.rs:462-478: New RehydrationPoolTypeMismatch runtime guard is not exercised by tests
  The `debug_assert!(pool.pool_type == probe.pool_type)` was promoted to a runtime fail-closed `Err(RehydrationPoolTypeMismatch { .. })` in release builds — good hardening — but no test verifies that a mis-ordered probe/pool pair actually produces this error. Because `probes` is built from `address_pools()` and the real pools come from `address_pools_mut()` on the same account, the mismatch is only reachable via divergence between the two `key_wallet` accessors. A small structural-invariant test (or a `debug_assert_eq!` alongside the `Err`) would catch a future upstream regression. Same applies to the pre-existing `RehydrationPoolMismatch` length check.

In `packages/rs-platform-wallet/tests/rehydration_load.rs`:
- [SUGGESTION] packages/rs-platform-wallet/tests/rehydration_load.rs:1: No test combines snapshot-mismatch skip with a co-occurring successful load
  `rt_snapshot_wallet_id_mismatch_is_skipped` verifies the mismatched row is skipped in isolation, and `rt_corrupt_row_skipped_and_other_loads` verifies a corrupt-manifest skip alongside a successful load in the same batch. There's no equivalent combined-batch test for the new snapshot-mismatch skip path (skipped wallet + a second healthy wallet loading successfully in the same `load_from_persistor` call). Extending the existing pattern would confirm the mismatch skip doesn't poison siblings in the batch.

[thepastaclaw]

Code Review

Latest push (e66fbf5..51cd9c9) is documentation-only — a 93-line 'Persistence architecture' section added to packages/rs-platform-wallet/README.md, with no changes to Rust or Swift source. All 8 prior findings remain valid at unchanged line ranges. One new suggestion from codex-general (projection-fallback path routes used addresses to only the first funds account) is included. No in-scope blockers.

🟡 8 suggestion(s) | 💬 5 nitpick(s)

Findings not posted inline (4)

These findings could not be anchored to the current diff, but they are still part of this review.

• [SUGGESTION] packages/rs-platform-wallet/src/manager/rehydrate.rs:223-264: Projection fallback only restores used flags on the first funds account — used_pool_addresses is documented as a flattened snapshot across all accounts/pools, but both replay branches at lines 223-249 and 250-263 route the entire set through wallet_info.accounts.all_funding_accounts_mut().into_iter().next() and call extend_pools_for_restored_addresses on only tha...
• [SUGGESTION] packages/rs-platform-wallet/src/manager/rehydrate.rs:462-478: New RehydrationPoolTypeMismatch runtime guard is not exercised by tests — STILL VALID (prior-4, code unchanged). The debug_assert!(pool.pool_type == probe.pool_type) was promoted to a runtime fail-closed Err(PlatformWalletError::RehydrationPoolTypeMismatch { .. }) in release builds (good hardening), but no test verifies that a mis-ordered probe/pool pair actually p...
• [NITPICK] packages/rs-platform-wallet-ffi/src/manager.rs:384-407: Document that non-null out_outcome must be zero-initialised or previously freed — STILL VALID (prior-6, code unchanged). std::ptr::write(out_outcome, LoadOutcomeFFI { .., skipped: null_mut() }) unconditionally overwrites the destination without inspecting or freeing any prior skipped array — ptr::write does not run Drop. If a non-Swift caller reused the same `LoadOutcome...
• [NITPICK] packages/rs-platform-wallet-ffi/src/manager.rs:463-474: platform_wallet_load_outcome_free leaves loaded_count non-zero after freeing — STILL VALID (prior-8, code unchanged). The free function clears skipped_count/skipped for idempotence but leaves loaded_count untouched. The doc string says the free is idempotent and nulls the pointer, so callers who inspect the struct post-free see a stale loaded_count. No memory-safety...

Carried-forward findings already raised (4)

These findings were not re-posted as new inline comments because an existing review thread already covers them.

• [SUGGESTION] (deduped existing open thread) packages/rs-platform-wallet/src/changeset/client_wallet_start_state.rs:41-97: Snapshot vs projection fields form an implicit-invariant union — STILL VALID (prior-1, code unchanged). ClientWalletStartState carries three overlapping sources of truth for core state: core_wallet_info: Option<Box<ManagedWalletInfo>> (snapshot) and core_state: CoreChangeSet + used_core_addresses: Vec<Address> (projection). The 'if core_wallet_info i...
• [SUGGESTION] (deduped existing open thread) packages/rs-platform-wallet/src/manager/load.rs:162-220: Snapshot path validates wallet_id/network but not manifest↔snapshot account consistency — STILL VALID (prior-2, code unchanged). When core_wallet_info: Some(info) is consumed, the manager stitches a watch-only Wallet built from account_manifest together with ManagedWalletInfo.accounts from the snapshot, but only wallet_id/network are cross-checked. If a future persister po...
• [SUGGESTION] (deduped existing open thread) packages/rs-platform-wallet/src/manager/load.rs:175-190: Snapshot wallet_id/network mismatch reuses CorruptKind::DecodeError, obscuring the failure mode — STILL VALID (prior-3, code unchanged). A wallet_id/network mismatch on the persisted core_wallet_info snapshot is reported via CorruptKind::DecodeError(format!(...)), which the FFI maps to LOAD_SKIP_REASON_DECODE_ERROR (102) — the same code as a genuine bincode failure. These are semantical...
• [NITPICK] (deduped existing open thread) packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:340-349: Swift reasonDescription hardcodes wire numbers instead of importing LOAD_SKIP_REASON_ constants* — STILL VALID (prior-7, code unchanged). Rust exports LOAD_SKIP_REASON_MISSING_MANIFEST (100), LOAD_SKIP_REASON_MALFORMED_XPUB (101), LOAD_SKIP_REASON_DECODE_ERROR (102), LOAD_SKIP_REASON_CORRUPT_OTHER (199), LOAD_SKIP_REASON_OTHER (200) as pub const u32 specifically so consumers can re...

🤖 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/manager/rehydrate.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/rehydrate.rs:223-264: Projection fallback only restores used flags on the first funds account
  `used_pool_addresses` is documented as a flattened snapshot across all accounts/pools, but both replay branches at lines 223-249 and 250-263 route the entire set through `wallet_info.accounts.all_funding_accounts_mut().into_iter().next()` and call `extend_pools_for_restored_addresses` on only that first funds account. A fallback persister that cannot provide `core_wallet_info` but does supply used addresses for a non-first account (e.g. CoinJoin, DashPay) will never probe those addresses against the matching account xpub, so they remain `used = false` and can be handed out again after seedless load — defeating the exact address-reuse guard this PR added via `used_pool_addresses`. Not user-triggerable today because the in-tree FFI persister always sets `core_wallet_info` and SQLite does not hydrate wallets yet (dashpay/platform#3968), but the public fallback contract is incorrect. Fix by grouping `addresses_to_mark` by their owning account (or dispatching per pool) before calling `extend_pools_for_restored_addresses`.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/rehydrate.rs:462-478: New RehydrationPoolTypeMismatch runtime guard is not exercised by tests
  STILL VALID (prior-4, code unchanged). The `debug_assert!(pool.pool_type == probe.pool_type)` was promoted to a runtime fail-closed `Err(PlatformWalletError::RehydrationPoolTypeMismatch { .. })` in release builds (good hardening), but no test verifies that a mis-ordered probe/pool pair actually produces this error. Because `probes` is built from `address_pools()` and the real pools come from `address_pools_mut()` on the same account, the mismatch is only reachable via divergence between the two `key_wallet` accessors. A small structural-invariant test would catch a future upstream regression. Same applies to the pre-existing `RehydrationPoolMismatch` length check at lines 456-461.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/rehydrate.rs:223-264: Projection fallback only restores used flags on the first funds account
  `used_pool_addresses` is documented as a flattened snapshot across all accounts/pools, but both replay branches at lines 223-249 and 250-263 route the entire set through `wallet_info.accounts.all_funding_accounts_mut().into_iter().next()` and call `extend_pools_for_restored_addresses` on only that first funds account. A fallback persister that cannot provide `core_wallet_info` but does supply used addresses for a non-first account (e.g. CoinJoin, DashPay) will never probe those addresses against the matching account xpub, so they remain `used = false` and can be handed out again after seedless load — defeating the exact address-reuse guard this PR added via `used_pool_addresses`. Not user-triggerable today because the in-tree FFI persister always sets `core_wallet_info` and SQLite does not hydrate wallets yet (dashpay/platform#3968), but the public fallback contract is incorrect. Fix by grouping `addresses_to_mark` by their owning account (or dispatching per pool) before calling `extend_pools_for_restored_addresses`.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/rehydrate.rs:462-478: New RehydrationPoolTypeMismatch runtime guard is not exercised by tests
  STILL VALID (prior-4, code unchanged). The `debug_assert!(pool.pool_type == probe.pool_type)` was promoted to a runtime fail-closed `Err(PlatformWalletError::RehydrationPoolTypeMismatch { .. })` in release builds (good hardening), but no test verifies that a mis-ordered probe/pool pair actually produces this error. Because `probes` is built from `address_pools()` and the real pools come from `address_pools_mut()` on the same account, the mismatch is only reachable via divergence between the two `key_wallet` accessors. A small structural-invariant test would catch a future upstream regression. Same applies to the pre-existing `RehydrationPoolMismatch` length check at lines 456-461.

In `packages/rs-platform-wallet/tests/rehydration_load.rs`:
- [SUGGESTION] packages/rs-platform-wallet/tests/rehydration_load.rs:546-587: No test combines snapshot-mismatch skip with a co-occurring successful load
  STILL VALID (prior-5, code unchanged). `rt_snapshot_wallet_id_mismatch_is_skipped` verifies the mismatched row is skipped in isolation, and `rt_corrupt_row_skipped_and_other_loads` verifies a corrupt-manifest skip alongside a successful load in the same batch. There's no equivalent combined-batch test for the new snapshot-mismatch skip path (skipped wallet + a second healthy wallet loading successfully in the same `load_from_persistor` call). Extending the existing pattern would confirm the mismatch skip doesn't poison siblings in the batch.

In `packages/rs-platform-wallet/src/changeset/client_wallet_start_state.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/client_wallet_start_state.rs:41-97: Snapshot vs projection fields form an implicit-invariant union
  STILL VALID (prior-1, code unchanged). `ClientWalletStartState` carries three overlapping sources of truth for core state: `core_wallet_info: Option<Box<ManagedWalletInfo>>` (snapshot) and `core_state: CoreChangeSet` + `used_core_addresses: Vec<Address>` (projection). The 'if `core_wallet_info` is Some, ignore the other two' rule lives only in a match arm in `manager/load.rs` and doc comments — a persister that populates both would silently discard the projection with no compile-time signal. The new README section documents these projection fields as 'a transitional fallback … slated for removal once every in-tree persister produces snapshots', which strengthens the case for a tagged enum `enum CoreLoadState { Snapshot(Box<ManagedWalletInfo>), Projection { core_state, used_core_addresses } }`: it would (a) make mutual exclusion type-level, (b) remove the FFI persister's need to `Default::default()` unused fields, and (c) shrink the deletion when #3968 lands to removing one variant.

In `packages/rs-platform-wallet/src/manager/load.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/load.rs:162-220: Snapshot path validates wallet_id/network but not manifest↔snapshot account consistency
  STILL VALID (prior-2, code unchanged). When `core_wallet_info: Some(info)` is consumed, the manager stitches a watch-only `Wallet` built from `account_manifest` together with `ManagedWalletInfo.accounts` from the snapshot, but only `wallet_id`/`network` are cross-checked. If a future persister populates the manifest and snapshot from divergent sources (extra CoinJoin account only in one, mis-ordered types, etc.), `Wallet.accounts` and `wallet_info.accounts` disagree silently: one drives scanning/derivation while the other drives balances/UTXOs. Not user-triggerable today since the FFI persister derives both from the same in-memory state, but `PlatformWalletPersistence` is a public trait and #3968 will add a SQLite backend. A cheap structural check comparing account-type sets — or at minimum an explicit doc line — would extend the wallet_id/network fail-closed pattern to the account collection.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/load.rs:175-190: Snapshot wallet_id/network mismatch reuses CorruptKind::DecodeError, obscuring the failure mode
  STILL VALID (prior-3, code unchanged). A wallet_id/network mismatch on the persisted `core_wallet_info` snapshot is reported via `CorruptKind::DecodeError(format!(...))`, which the FFI maps to `LOAD_SKIP_REASON_DECODE_ERROR` (102) — the same code as a genuine bincode failure. These are semantically distinct: unreadable bytes vs a persister returning a snapshot for the wrong row (a stronger data-integrity signal, e.g. persister row mixup, especially relevant once SQLite lands per #3968). `CorruptKind` is `#[non_exhaustive]`, so a dedicated `SnapshotIdentityMismatch` variant plus a new `LOAD_SKIP_REASON_*` code is a low-cost backward-compatible improvement for debug logs and Swift-side triage.