fix(platform-wallet)!: preserve Core wallet snapshots on restart

[lklimek]

Reopening a SQLite wallet preserves its Core spending state, so repeated transaction delivery cannot make spent funds available again. Older databases resynchronize Core while retaining accounts, addresses, and Platform data.

User story

As a wallet user, I want my saved wallet to retain the correct balance after reopening and continuing synchronization.

Scenario

1. Receive funds and spend them while the wallet is saved locally.
2. Close and reopen the wallet.
3. Resume synchronization and receive the original funding transaction again.

Previously, incomplete restoration could make the spent coin available again. The restored wallet now retains the live engine's spend and finality state.

Detailed discussion

• Capture complete ManagedWalletInfo snapshots at a coherent event-queue boundary under the manager read lock. Release the lock before reconstruction operations that require write access.
• Opt SQLite into this capability and persist snapshots atomically with Core SQL projections. Buffered writes retain the newest snapshot; subsequent Core changes without one invalidate it. Faulted synchronization cannot advance through a snapshot.
• Add migration V020 with versioned, size-bounded snapshots and an upstream serialization-layout marker. Validate wallet ID, network, and embedded InstantLock transaction ownership on write and load; reject corrupt or incompatible snapshots instead of falling back silently.
• Read all wallet data in one SQLite transaction. Restore address pools monotonically over snapshots, preserving deep unused, used, reserved, provider, and hardened EdDSA addresses. Detect account registrations missing from a stale snapshot and restart Core synchronization.
• For databases without snapshots, initialize Core at birth_height.saturating_sub(1). Retain old SQL projections, accounts, addresses, and Platform data; do not treat incomplete financial rows or old checkpoints as a live Core state.
• Preserve account-local transaction SQL projections and engine UTXO credit verdicts. Index account slices once per changeset so batched persistence avoids repeated full scans.

The rust-dashcore dependency remains at the base branch's official revision e4208c90786a6854bd498315bcb571ef24182c15. This PR does not depend on rust-dashcore #1028 or a fork.

Compatibility

Older databases require Core resynchronization on first load. Platform data and saved account/address state remain intact. The partial apply_persisted_core_state helper is replaced by full snapshot restoration and address-only legacy loading.

Validation

• The SQLite load-query budget regression passes with default features and with --all-features; all 26 load-reconstruction tests pass with all features. The shared budget includes the optional shielded viewing-key scan and is measured independently for an empty database.
• Complete storage package suite with --all-features: 1002 passed, 3 ignored. Includes snapshot corruption/rollback, legacy rescan, address preservation, buffered invalidation, and concurrent readers/writers.
• Two embedded InstantLock mismatch regressions were confirmed failing before the fix and passing afterward. The snapshot/conflict targets pass all 11 tests under both default features and platform-wallet/keep-finalized-transactions; conflict cases persist late funding after the first reopen and verify a second reopen.
• 117 platform-wallet changeset/bridge tests passed, including the complete event-queue boundary and write-lock reconstruction regression.
• 24 Core lifecycle/storage tests passed with platform-wallet/keep-finalized-transactions enabled, as well as with the default feature configuration.
• cargo clippy -p platform-wallet-storage --all-targets --all-features --locked -- --no-deps -D warnings.
• cargo fmt -p platform-wallet -p platform-wallet-storage -- --check and git diff --check.
• No full-workspace or live-network tests were run.

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

[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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 2c0e4ad4-1151-4d85-9d43-16f8fafcf88e

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

---

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]

⚠️ DEGRADED — Final review complete — no blockers (commit bdd8771) · triage: critical · stand-in models (primary models out of quota)

[thepastaclaw]

Final validation — Phase 1 + Phase 2

The atomic restore design is directionally correct, but the SQLite adapter still loses account attribution and converts rejected transaction outputs into permanent spend claims. Targeted restoration tests pass, while focused probes reproduce both balance/state divergences, along with an InstantSend flag regression.

🔴 2 blocking | 🟡 4 suggestion(s)

2 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Restore InstantSend coin flags before the duplicate-lock guard
packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:429-436

load_state initializes every restored UTXO with is_instantlocked: false. When a persisted record has InstantSend context, restore_persisted_state already inserts its txid into key-wallet's instant_send_locks; the subsequent mark_instant_send_utxos call therefore returns immediately because the txid is already present and never marks the restored coins. The wallet remembers the lock but places the coin in the unconfirmed balance bucket. Set is_instantlocked on persisted UTXOs whose txids have restored locks before calling restore_persisted_state, or add an upstream restore operation that reconciles the lock set with coin flags.

    for (_, utxo) in &mut persisted.utxos {
        if core
            .instant_locks_for_non_final_records
            .contains_key(&utxo.outpoint.txid)
        {
            utxo.is_instantlocked = true;
        }
    }
    wallet_info.restore_persisted_state(persisted)?;

_{source: gpt-6-astra (phase2-reviewer: general, architecture-layering, rust-quality, security-auditor)}

🟡 Suggestion: Update load-policy documentation for fail-closed ownership restoration
packages/rs-platform-wallet-storage/src/sqlite/persister.rs:1458-1462

These comments still say unresolved restored addresses degrade under both policies, re-warm on the next sync, and preserve an exact total. The new restore path instead rejects unresolved unspent coins through restore_persisted_state; strict loading fails atomically and recovery isolates the wallet. The corresponding fallback wording also remains in schema/core_state.rs around lines 914-920. Update both documentation sites to describe the fail-closed ownership requirement and the need to restore the relevant address-pool range before retrying.

_{source: muse-spark-1.3-contributor (phase1-reviewer: general, architecture-layering, rust-quality, security-auditor)}

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

• Triage: critical by gpt-6-astra (effort low) — The large, intricate diff changes persistent wallet-state restoration and UTXO spendability/coin-selection behavior in packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs and schema/core_state.rs, directly affecting funds movement and storage-backed wallet state.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — architecture-layering (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — security-auditor (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 11% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh verifier: gpt-6-astra — final-verifier; agent astra-verifier
• Phase 2 reviewers: gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort xhigh); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:335-338: Restore account-scoped transaction records instead of folded wallet records
  `core.records` contains one wallet-level record per transaction, produced by folding the account-local slices, but `restore_persisted_state` installs each record into the single account named by `record.account_type`. For a transaction paying 5,000 duffs to BIP44 and 7,000 to CoinJoin, the folded 12,000-duff record is restored only into the funding account. When confirmation is later delivered, key-wallet creates the missing sibling-account record, and the persistence bridge folds the restored 12,000 record together with the new 7,000 record, producing 19,000 instead of the uninterrupted wallet's 12,000. `CoreChangeSet.account_records` preserves the required slices, but SQLite currently discards them during `load_state`. Persist and restore the account-local slices, or reconstruct them before calling the upstream restore API.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:429-436: Restore InstantSend coin flags before the duplicate-lock guard
  `load_state` initializes every restored UTXO with `is_instantlocked: false`. When a persisted record has InstantSend context, `restore_persisted_state` already inserts its txid into key-wallet's `instant_send_locks`; the subsequent `mark_instant_send_utxos` call therefore returns immediately because the txid is already present and never marks the restored coins. The wallet remembers the lock but places the coin in the unconfirmed balance bucket. Set `is_instantlocked` on persisted UTXOs whose txids have restored locks before calling `restore_persisted_state`, or add an upstream restore operation that reconciles the lock set with coin flags.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:160-162: Do not turn doomed outputs into durable unattributed spend claims
  `UtxoCreditVerdict::Doomed` means the creating mempool transaction was rejected because one of its inputs was already observed spent; it does not prove that the transaction's own outputs were consumed. This branch writes every doomed output as `spent = 1` without a height. `load_state` then exports that row as supplemental evidence with `None`, and `restore_persisted_state` places it in `unattributed_spent_outpoints`. Because no record input claims the output, a later authoritative delivery of the formerly conflicting funding transaction is suppressed after restart. A focused round-trip reproduced the divergence: the uninterrupted wallet credited the output after chain-locked confirmation, while the restored wallet remained at zero. Preserve doomed/rejected-creation state separately from evidence that an output was spent, so later authoritative confirmation can reinstate the coin.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:135-137: Preserve the greatest observed spend height
  The `ObservedSpent` path unconditionally overwrites `winner_mined_height`. A stale or reordered verdict with a lower height can therefore replace a newer spend height. Since tombstone collection deletes rows once `winner_mined_height` reaches the chainlock/sync boundary, regressing this stamp can make spend evidence collectible too early and allow a later funding rescan to lose the only durable protection. Update the column monotonically, matching the height-watermark handling elsewhere in the persistence layer.

In `packages/rs-platform-wallet-storage/tests/sqlite_core_state_reader.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/tests/sqlite_core_state_reader.rs:386-390: Actually vary the record order supplied to restoration
  `records_spend_first` changes the vector passed to `store`, but `load_state` reads `core_transactions` without an `ORDER BY`, so the test does not guarantee that the spend record is first when restoration runs. Instrumentation showed both order variants pass the same `[funding, spend]` sequence to `apply_persisted_core_state`. Make the loaded `core` mutable and swap the records immediately before restoration, then assert the first record's txid, so the advertised order-independence test exercises both paths.

In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:1458-1462: Update load-policy documentation for fail-closed ownership restoration
  These comments still say unresolved restored addresses degrade under both policies, re-warm on the next sync, and preserve an exact total. The new restore path instead rejects unresolved unspent coins through `restore_persisted_state`; strict loading fails atomically and recovery isolates the wallet. The corresponding fallback wording also remains in `schema/core_state.rs` around lines 914-920. Update both documentation sites to describe the fail-closed ownership requirement and the need to restore the relevant address-pool range before retrying.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

All six prior findings are fixed at the reviewed head. Two independently reproduced restoration defects remain: continued synchronization can make a previously loadable wallet fail on its next reopen, and an embedded InstantLock for another transaction is accepted and used to select the wrong conflict winner. The complete storage suite finished with 996 passed, 1 failed, and 3 ignored; the failure is the prepared-statement guard, and temporary verification probes were removed, leaving the worktree unchanged.

🔴 2 blocking | 🟡 2 suggestion(s)

Review provenance

Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: architecture-layering); reviewer 3: gemini-3.8-flash-high (agent: phase1-reviewer, role: platform-versioning); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

• Triage: critical by gpt-6-astra (effort low) — The intricate restoration changes in packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs and schema/core_state.rs directly determine spendable coins through restored spend evidence, ownership and lock-conflict reconciliation, while V019__core_account_records.rs changes the persisted storage schema.
• Phase 1 reviewers: gemini-3.8-flash-high — general (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — architecture-layering (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — platform-versioning (completed, effort high); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — security-auditor (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (lane failed), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh verifier: gpt-6-astra — final-verifier; agent astra-verifier
• Phase 2 reviewers: gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort xhigh); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:560-568: Reconcile locked conflicts before validating subsequent snapshots
  The conflict sweep repairs only the in-memory wallet, discards its removal results, and runs after snapshot validation. I reproduced a load → sync → store → load failure with a persisted mempool loser spending X and F:0, a winner spending only X, and a separately persisted winner lock. The first restoration correctly removes the loser. Synchronization then discovers F and correctly credits and persists F:0, but the next restoration returns CoreStateRestore(SpentUtxo(F:0)): the stale losing record remains in SQLite and claims that coin before the sweep can run. Strict loading fails; Recovery omits the affected wallet. Reconcile the winning conflict set, coins, and spend evidence before validation, or keep the durable snapshot synchronized with the restoration sweep. Add a regression that continues synchronization, persists the resulting funding delivery, and reopens again.
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:569-570: Reject embedded InstantLocks belonging to another transaction
  The new core_instant_locks row check does not cover locks embedded in TransactionRecord.context. The pinned key-wallet validate_persisted_state checks the InstantSend lifecycle flag but never checks that the embedded lock.txid equals record.txid. I reproduced this through SQLite with B's lock embedded in conflicting transaction A's record and no separate lock-table row: strict restoration succeeded, treated A as locked, retained A's balance, and swept B. This violates the fail-closed restoration contract and lets malformed persisted state choose the wrong conflict winner. Validate the embedded lock-to-record association in key-wallet's shared persistence validator before installing state, update the dependency pin, and add owner-level and SQLite regressions. Keep the existing separate-row identity check as well.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:305: Keep the new pool reader compatible with the prepared-statement guard
  The new conn.prepare call is absent from READ_ONLY_PREPARE_ALLOWED, so tc_p1_003_prepare_cached_in_writers fails and identifies this line as its only offender. I confirmed this both independently and in the complete storage suite. This is a read-only query, not an uncached writer, so it can either receive a precise read-only exemption or use prepare_cached. Using prepare_cached also avoids preparing the same SQL repeatedly while restore_provider_key_pools iterates through provider accounts and their pools.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:130-134: Index account slices once per changeset
  This filter scans every account record for each wallet-level record. A changeset containing N transactions with one account slice each therefore performs N² txid comparisons while holding the SQLite write transaction. Such batches occur in synchronization and manual flushes, where changesets are merged before persistence. Build a txid-to-slices index once before the record loop and look up only the matching slices, using borrowed records so the index introduces no additional record clones. The existing per-account merge and sibling-preservation behavior can remain unchanged.

Out-of-scope follow-up suggestions (3)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Move the alternate FFI loader onto shared snapshot restoration — The unchanged loader in packages/rs-platform-wallet-ffi/src/persistence.rs inserts UTXOs directly and selectively restores asset-lock and provider transaction records rather than complete spending history. It does not inherit this PR's SQLite restoration guarantees. This is a concrete persistence gap worth tracking separately, but the PR explicitly targets SQLite-backed wallets and does not modify that loader.
  • Follow-up: Track a separate change to carry complete account-local records and spend evidence through the FFI persistence boundary and install them through the shared validated restoration API.
• Provider platform node pool reservations are not restored — Outside this PR's scope: restore_provider_platform_node_pool already documents the omitted reservation timestamps in TODO(#4188), and this PR does not introduce or worsen that limitation. Routine adjacent follow-up omitted.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
• Orphaned empty wallet manifests lack lifecycle eviction or recovery — Outside this PR's scope and not an established defect: load_one_wallet explicitly permits legitimate platform-only wallets with empty account manifests and identifies orphan lifecycle handling as an unresolved product decision. No new failure is demonstrated.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.

[thepastaclaw]

⚠️ DEGRADED — Re-review — Final validation — Phase 1 + Phase 2

⚠️ DEGRADED review. The primary review models were unavailable (gpt-6-astra unavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache), so this review ran on stand-in models: gpt-5.6-luna → muse-spark-1.3-contributor, gpt-5.6-sol → muse-spark-1.3-contributor, gpt-5.6-terra → muse-spark-1.3-contributor, gpt-6-astra → muse-spark-1.3-contributor. Both review phases and the independent verifiers still ran, but on weaker models, with Phase 1 capped at high effort. Treat the verdict as provisional; a full-strength re-review will run on the next push once the primary models are back.

Snapshot-based Core restore at this head is sound: full ManagedWalletInfo snapshots are captured at a coherent queue boundary, validated on write and load, and persisted atomically with their projections, and all ten prior findings verify as fixed. Remaining observations are non-blocking robustness gaps: an unbounded snapshot drain, over-broad invalidation, missing slice backfill, an oversize-snapshot failure mode, a narrow layout canary, a public helper with an unnameable parameter, unchecked pool arithmetic, and one duplicated predicate.

🟡 7 suggestion(s) | 💬 1 nitpick(s)

Review provenance

Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 5: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 6: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: architecture-layering); reviewer 7: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: rust-quality); reviewer 8: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: security-auditor); reviewer 9: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 10: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: architecture-layering); reviewer 11: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: rust-quality); reviewer 12: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: security-auditor); final verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: astra-verifier, role: final-verifier)

• Degraded mode: gpt-6-astra unavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache (detected by probe, since 2026-09-18T05:22:01Z); stand-ins gpt-5.6-luna → muse-spark-1.3-contributor, gpt-5.6-sol → muse-spark-1.3-contributor, gpt-5.6-terra → muse-spark-1.3-contributor, gpt-6-astra → muse-spark-1.3-contributor; Phase 1 effort capped at high
• Triage: critical by muse-spark-1.3-contributor (standing in for gpt-6-astra) (effort low) — Large +4217/-2650 change adds storage migration V020__core_wallet_snapshots.rs and restores Core spend/UTXO state to prevent double-spend on reopen.
• Phase 1 reviewers: gemini-3.8-flash-high — general (completed, effort high); agent phase1-reviewer, muse-spark-1.3-contributor — architecture-layering (completed, effort high); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort high); agent phase1-reviewer, muse-spark-1.3-contributor — security-auditor (completed, effort high); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (lane failed), glm-5.3-flash (zai below 15% reserve: 5h 99% left, weekly 13% left)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) — final-verifier; agent astra-verifier
• Phase 2 reviewers: muse-spark-1.3-contributor (standing in for gpt-6-astra) — general (completed, effort xhigh); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — architecture-layering (completed, effort xhigh); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — rust-quality (completed, effort xhigh); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — security-auditor (completed, effort xhigh); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — general (completed, effort xhigh); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — architecture-layering (completed, effort xhigh); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — rust-quality (completed, effort xhigh); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — security-auditor (completed, effort xhigh); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet/src/changeset/core_bridge.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:414-435: Snapshot path drains the unbounded backlog with no batch cap
  When CORE_WALLET_SNAPSHOTS is attested, `while snapshots_enabled || events.len() < ADAPTER_STORE_BATCH_LIMIT` drains until TryRecvError::Empty, so one round folds the entire unbounded mpsc backlog into a single changeset and a single SQLite transaction. The comment at lines 417-418 states the reason (a capped prefix would leave the snapshot ahead of its rows), but per-chunk snapshots would preserve that invariant too: each stored snapshot only needs to cover its own stored rows. The read guard is dropped before projection and commit, yet the drain plus one full ManagedWalletInfo clone per wallet still runs under it, and the resulting single changeset/transaction reintroduces the oversized-round and cancellation-latency problem the 512-event cap existed to bound. Drain in bounded chunks where each chunk captures its own snapshot, so every commit stays snapshot-consistent without one unbounded round.
- [NITPICK] packages/rs-platform-wallet/src/changeset/core_bridge.rs:505-513: Store-eligibility triple duplicated in batch filter and commit skip
  The `!core.is_empty_no_records() || !asset_locks.is_empty() || snapshot.is_some()` condition appears both in the batch_wallet_ids filter (lines 505-513) and in commit_wallet's early-return skip (lines 748-751), with the snapshot disjunct added in both spots by this PR. A future change to what counts as store-worthy must edit both in lockstep with no compiler help. Add a small WalletBatch::should_store() helper and call it from both places.

In `packages/rs-platform-wallet/src/changeset/changeset.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/changeset.rs:2253-2259: Snapshot invalidation fires on non-durable Core geometry the store never persists
  PlatformWalletChangeSet::merge (and symmetrically apply_changeset_to_tx, which invalidates on `!core.is_empty()`) treats any non-empty CoreChangeSet as invalidating the snapshot. But addresses_derived, addresses_marked_used, and account_highest_used have no storage writer — core_state::apply explicitly does not persist addresses_derived and derives ownership from account_address_pools instead. A changeset carrying only such geometry with no snapshot therefore deletes the stored snapshot while persisting nothing that replaces it, forcing a needless legacy rescan. This normally needs a fault/sweep-guard strip or Manual-mode buffer merge to trigger, since drains carry snapshots. Narrow the invalidation predicate to durable Core fields (records, account_records, utxos, sweeps, locks, heights, verdicts) or persist the geometry it treats as invalidating.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:118-149: Pre-V019 rows never backfill account_records_blob even when complete slices arrive
  When the prior row has a record but account_records_blob IS NULL, legacy_row is set and account_payload is forced to None, so COALESCE keeps NULL forever. Later changesets carrying the full per-account slices for that txid still merge them in memory (lines 134-143) and then discard them at line 147. Per-account history for migrated txids therefore stays NULL permanently instead of backfilling on the first post-migration re-emission. If the guard exists to avoid storing partial drains as complete, backfill only when the incoming slice set is non-empty and keyed consistently, or document that migrated rows intentionally never gain slices.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_wallet_snapshots.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_wallet_snapshots.rs:26-35: Oversize snapshot fails the whole store with no fallback, freezing sync
  apply encodes the full ManagedWalletInfo and enforces the shared 16 MiB blob cap before writing. A wallet whose live state exceeds the cap (plausible with long history under keep-finalized-transactions) fails every snapshot-carrying store, and commit_wallet then faults and freezes the durable watermark, so sync cannot advance past that point via this backend. Since snapshots are an optimization over the delta rows riding in the same atomic transaction, store the deltas without the snapshot (treating it as an invalidation) rather than failing the round, or add a snapshot-specific bound with an explicit oversize metric.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_wallet_snapshots.rs:112-126: Layout marker probes a single account type, may miss layout drift elsewhere
  layout_marker encodes one ManagedCoreKeysAccount (IdentityRegistration) as the upstream serde-layout canary. The snapshot contains other account shapes (standard, coinjoin, provider BLS/EdDSA, platform payment) whose serde layouts can change independently without altering this marker, leaving detection to bincode decode failing loudly rather than being rejected up front. Since bincode serde payloads are not self-describing, probe one exemplar per account family (or an explicit upstream layout version) so the fail-closed marker covers the whole snapshot schema.

In `packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:304-313: Public helper exposes crate-private manifest type
  pub fn restore_core_address_pools takes &AccountManifest from schema::accounts, but sqlite::schema is pub(crate) in shipping builds (public only under test/__test-helpers), so a downstream embedder cannot name the parameter type and cannot call the function. The only production caller is persister::load_one_wallet in the same crate. Make the helper pub(crate) and cover the direct-call cases through persister-level store/load tests or in-crate unit tests, or deliberately re-export the manifest type alongside OwningAccount if external callers are intended.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:873-880: Unchecked h + 1 on snapshot-controlled highest_generated can panic on corrupt DB
  ensure_derived computes pool.highest_generated.map(|h| h + 1) where highest_generated comes from the deserialized ManagedWalletInfo snapshot — untrusted local-DB bytes validated only for size, wallet id, network, and lock ownership. A corrupt or crafted snapshot with highest_generated == u32::MAX panics on overflow in debug builds and wraps to 0 in release builds, turning load into a crash or a bogus massive derivation instead of a typed error the LoadCtx recovery path can tolerate. The rest of this crate treats corrupt persisted values as data errors, so arithmetic over them must be checked.

Out-of-scope follow-up suggestions (1)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• FFI hosts retain pre-snapshot Core restoration — FFIPersister does not attest CORE_WALLET_SNAPSHOTS and keeps the projection-only restore path, so non-SQLite (e.g. mobile) hosts do not get this PR's restart guarantee. Wiring ManagedWalletInfo across the C ABI is a separate transport feature, not a defect in this PR's SQLite snapshot design.
  • Follow-up: Create a separate issue for an FFI snapshot slot (or equivalent) so FFI hosts can opt into the same authoritative Core restore.

[thepastaclaw]

🟡 Suggestion: Snapshot path drains the unbounded backlog with no batch cap

When CORE_WALLET_SNAPSHOTS is attested, while snapshots_enabled || events.len() < ADAPTER_STORE_BATCH_LIMIT drains until TryRecvError::Empty, so one round folds the entire unbounded mpsc backlog into a single changeset and a single SQLite transaction. The comment at lines 417-418 states the reason (a capped prefix would leave the snapshot ahead of its rows), but per-chunk snapshots would preserve that invariant too: each stored snapshot only needs to cover its own stored rows. The read guard is dropped before projection and commit, yet the drain plus one full ManagedWalletInfo clone per wallet still runs under it, and the resulting single changeset/transaction reintroduces the oversized-round and cancellation-latency problem the 512-event cap existed to bound. Drain in bounded chunks where each chunk captures its own snapshot, so every commit stays snapshot-consistent without one unbounded round.

_{source: muse-spark-1.3-contributor (phase2-reviewer: general, architecture-layering, rust-quality, security-auditor)}

[thepastaclaw]

🟡 Suggestion: Snapshot invalidation fires on non-durable Core geometry the store never persists

PlatformWalletChangeSet::merge (and symmetrically apply_changeset_to_tx, which invalidates on !core.is_empty()) treats any non-empty CoreChangeSet as invalidating the snapshot. But addresses_derived, addresses_marked_used, and account_highest_used have no storage writer — core_state::apply explicitly does not persist addresses_derived and derives ownership from account_address_pools instead. A changeset carrying only such geometry with no snapshot therefore deletes the stored snapshot while persisting nothing that replaces it, forcing a needless legacy rescan. This normally needs a fault/sweep-guard strip or Manual-mode buffer merge to trigger, since drains carry snapshots. Narrow the invalidation predicate to durable Core fields (records, account_records, utxos, sweeps, locks, heights, verdicts) or persist the geometry it treats as invalidating.

_{source: muse-spark-1.3-contributor (phase2-reviewer: general)}

[thepastaclaw]

🟡 Suggestion: Pre-V019 rows never backfill account_records_blob even when complete slices arrive

When the prior row has a record but account_records_blob IS NULL, legacy_row is set and account_payload is forced to None, so COALESCE keeps NULL forever. Later changesets carrying the full per-account slices for that txid still merge them in memory (lines 134-143) and then discard them at line 147. Per-account history for migrated txids therefore stays NULL permanently instead of backfilling on the first post-migration re-emission. If the guard exists to avoid storing partial drains as complete, backfill only when the incoming slice set is non-empty and keyed consistently, or document that migrated rows intentionally never gain slices.

_{source: muse-spark-1.3-contributor (phase2-reviewer: general)}

[thepastaclaw]

🟡 Suggestion: Oversize snapshot fails the whole store with no fallback, freezing sync

apply encodes the full ManagedWalletInfo and enforces the shared 16 MiB blob cap before writing. A wallet whose live state exceeds the cap (plausible with long history under keep-finalized-transactions) fails every snapshot-carrying store, and commit_wallet then faults and freezes the durable watermark, so sync cannot advance past that point via this backend. Since snapshots are an optimization over the delta rows riding in the same atomic transaction, store the deltas without the snapshot (treating it as an invalidation) rather than failing the round, or add a snapshot-specific bound with an explicit oversize metric.

_{source: muse-spark-1.3-contributor (phase2-reviewer: general)}

[thepastaclaw]

🟡 Suggestion: Layout marker probes a single account type, may miss layout drift elsewhere

layout_marker encodes one ManagedCoreKeysAccount (IdentityRegistration) as the upstream serde-layout canary. The snapshot contains other account shapes (standard, coinjoin, provider BLS/EdDSA, platform payment) whose serde layouts can change independently without altering this marker, leaving detection to bincode decode failing loudly rather than being rejected up front. Since bincode serde payloads are not self-describing, probe one exemplar per account family (or an explicit upstream layout version) so the fail-closed marker covers the whole snapshot schema.

_{source: muse-spark-1.3-contributor (phase2-reviewer: general)}

[thepastaclaw]

🟡 Suggestion: Public helper exposes crate-private manifest type

pub fn restore_core_address_pools takes &AccountManifest from schema::accounts, but sqlite::schema is pub(crate) in shipping builds (public only under test/__test-helpers), so a downstream embedder cannot name the parameter type and cannot call the function. The only production caller is persister::load_one_wallet in the same crate. Make the helper pub(crate) and cover the direct-call cases through persister-level store/load tests or in-crate unit tests, or deliberately re-export the manifest type alongside OwningAccount if external callers are intended.

_{source: muse-spark-1.3-contributor (phase2-reviewer: architecture-layering)}

[thepastaclaw]

🟡 Suggestion: Unchecked h + 1 on snapshot-controlled highest_generated can panic on corrupt DB

ensure_derived computes pool.highest_generated.map(|h| h + 1) where highest_generated comes from the deserialized ManagedWalletInfo snapshot — untrusted local-DB bytes validated only for size, wallet id, network, and lock ownership. A corrupt or crafted snapshot with highest_generated == u32::MAX panics on overflow in debug builds and wraps to 0 in release builds, turning load into a crash or a bogus massive derivation instead of a typed error the LoadCtx recovery path can tolerate. The rest of this crate treats corrupt persisted values as data errors, so arithmetic over them must be checked.

Suggested change

	pool.generate_addresses(index - start + 1, key_source, true)
	.ok()?;
	let start = pool.highest_generated.map(|h| h.saturating_add(1)).unwrap_or(0);

_{source: muse-spark-1.3-contributor (phase2-reviewer: rust-quality)}

[thepastaclaw]

💬 Nitpick: Store-eligibility triple duplicated in batch filter and commit skip

The !core.is_empty_no_records() || !asset_locks.is_empty() || snapshot.is_some() condition appears both in the batch_wallet_ids filter (lines 505-513) and in commit_wallet's early-return skip (lines 748-751), with the snapshot disjunct added in both spots by this PR. A future change to what counts as store-worthy must edit both in lockstep with no compiler help. Add a small WalletBatch::should_store() helper and call it from both places.

_{source: muse-spark-1.3-contributor (phase2-reviewer: rust-quality)}

[github-actions]

PR Hygiene

State: waiting-bots · commit bdd87712513e59f36524a6e3ff41ab0fb21cfc74

• Bot review threads remain unresolved
• Proceeded without coderabbitai: no review within the configured window

Self-review is an author attestation that you have read the diff:
/self-reviewed — covers everything pushed so far; post it again after a new push.
/skip-bots — proceed without the bots that have not reported; anyone with write access may, and the report says who did.

This check passes when the policy is satisfied; the repository decides whether merging requires it.