Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs`:
- Around line 235-244: The current migrate branch uses peek_schema_version which
swallows all errors into None causing applied to be unreliable; change
peek_schema_version to return Result<Option<i64>, E> (i.e., Result<Option<i64>,
Box<dyn Error> or a concrete error type) and update the call site in the
Cmd::Migrate block: keep using the old behavior for pre_version (call
peek_schema_version before opening the DB if you must tolerate missing table),
but after SqlitePersister::open(config.clone()) call the new peek_schema_version
and propagate or error out on Err so the post_version probe cannot be silently
treated as 0; compute applied only when post_version is Ok(Some/_), and map
errors to the CLI error flow (use map_open_err_for_cli or similar) instead of
unwrap_or(0) so the printed "applied: N" is trustworthy.
- Around line 288-300: The CLI-level pre-check args.out.is_file() in run_backup
is redundant and does not prevent the TOCTOU race because backup_to() itself
checks exists() and ultimately calls run_to() -> Connection::open(dest) which
can be raced; either remove the args.out.is_file() guard from run_backup, or
(preferred) harden the persistence path by modifying backup_to()/run_to() to
perform atomic file creation (e.g., use OpenOptions::create_new() or equivalent)
when opening the destination instead of relying on exists()/Connection::open,
ensuring the creation fails if the file was created concurrently.

In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- Around line 517-520: The code that rebuilds state in load() uses
schema::platform_addrs::count_per_wallet and then inserts addrs into
state.platform_addresses only if count > 0 || addrs.sync_height > 0 ||
addrs.sync_timestamp > 0, which drops wallets whose only persisted platform
state is addrs.last_known_recent_block; update the reconstruction condition to
also check addrs.last_known_recent_block (e.g., include ||
addrs.last_known_recent_block > 0) so that entries with only
last_known_recent_block are preserved when inserting into
state.platform_addresses.
- Around line 412-468: flush_inner currently calls self.buffer.drain(wallet_id)
and discards the changeset (cs) before opening the DB transaction, so any
failure after draining (e.g., during schema::...::apply or tx.commit()) loses
the buffered changes; change the logic so the buffer is only removed after
tx.commit() succeeds: either fetch/clone/peek the changeset (use a
non-destructive read API instead of buffer.drain) into cs, execute the schema
apply calls and tx.commit(), and only then call buffer.drain(wallet_id) or
buffer.remove(wallet_id) to clear it; if you must drain early, ensure every
error path re-inserts/requeues the drained cs back into the buffer (e.g.,
buffer.insert(wallet_id, cs)) before returning the PersistenceError, referencing
flush_inner, self.buffer.drain(wallet_id), cs, and tx.commit() to locate the
edits.
- Around line 269-326: Before checking existence/backing up/deleting in
delete_wallet_inner, reconcile in-memory buffered writes for the target wallet:
call the component that persists or discards pending buffer entries (e.g.,
flush/commit_writes or discard_buffered_writes) for wallet_id before the initial
conn() existence check and before run_auto_backup; propagate any error from that
operation so the delete aborts on flush failures. Locate delete_wallet_inner and
insert the flush/discard call at the top (prior to the SELECT 1 and
run_auto_backup usages) and ensure the rest of the logic (PER_WALLET_TABLES
counting, schema::wallet_meta::delete, tx.commit) operates on the now-consistent
persisted state.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs`:
- Around line 26-28: The decode function currently ignores the bytes-consumed
result from bincode::serde::decode_from_slice, allowing trailing garbage to pass
as valid; update the blob::decode implementation (the decode function that calls
bincode::serde::decode_from_slice) to check the second tuple element
(bytes_consumed) against blob.len() and return a WalletStorageError (e.g.,
Err(WalletStorageError::CorruptedBlob) or similar existing error variant) when
bytes_consumed != blob.len(), otherwise return the decoded value; ensure the
error path uses the same error type returned on bincode failures to keep the
function signature consistent.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- Around line 108-136: The upsert_utxo function currently writes account_index
as 0 into core_utxos causing list_unspent_utxos to misassign UTXOs; fix by
populating account_index before inserting: either (a) perform a lookup/join
against core_derived_addresses inside upsert_utxo (match wallet_id +
script/address) to derive the correct account_index and use that value instead
of 0, or (b) modify the Utxo/CoreChangeSet pipeline so the writer that calls
upsert_utxo receives and passes the owning account_index, or (c) add a
deterministic backfill writer that runs after writes but before any
list_unspent_utxos calls to update core_utxos.account_index from
core_derived_addresses; update upsert_utxo (and any callers of
upsert_utxo/CoreChangeSet) to ensure account_index is never left as the
hardcoded 0.
- Around line 138-174: The read in upsert_sync_state currently uses lp.map(|x| x
as u32) / sy.map(|x| x as u32) which silently truncates out-of-range i64 values;
change the rusqlite row mapping to return Option<i64> for both heights (keep the
closure in query_row returning (Option<i64>, Option<i64>)), then after the
query_row.optional()? unwrap_or((None,None)) convert each Option<i64> to
Option<u32> using a checked helper (reuse i64_to_u64 + u32::try_from or add
i64_to_u32) and map conversion errors to WalletStorageError::IntegerOverflow so
the function returns an error rather than silently wrapping before using lp and
sy in the INSERT/ON CONFLICT params.
- Around line 27-45: The code iterates cs.spent_utxos and calls upsert_utxo(tx,
wallet_id, utxo, true) when the outpoint is missing, which will materialize a
synthetic UTXO with account_index = 0; document this intent at the call site or
add an explicit guard flag to make the exceptional behavior obvious. Update the
block in the loop that handles cs.spent_utxos to either (a) add a clear comment
above the else branch referencing upsert_utxo and why creating a spent-only
placeholder with account_index = 0 is safe and will be corrected later, or (b)
wrap the else branch in a condition/flag (e.g., only_insert_spent_placeholders)
and pass that flag through to upsert_utxo so callers must opt into creating
synthetic rows; reference cs.spent_utxos, upsert_utxo, and the
core_utxos/account_index = 0 behavior in the comment or the new guard.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- Around line 42-64: fetch currently only selects entry_blob and discards the
tombstoned column referenced in the doc; change fetch to SELECT entry_blob,
tombstoned from identities and return the tombstone flag alongside the decoded
IdentityEntry (e.g. update the signature to Result<Option<(IdentityEntry,
bool)>, WalletStorageError> or a small wrapper type), decode payload with
blob::decode for the entry, map the tombstoned SQL value to a bool, and update
the doc comment to reflect the new return shape (ensure you still use
rusqlite::OptionalExtension and propagate errors as before).

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:
- Around line 72-89: The upsert loop writes typed key columns from the loop key
((identity_id, key_id), wallet_id) but serializes the payload from entry (via
IdentityKeyWire::from_entry), which can produce mismatches; ensure the
serialized wire and the SQL key are consistent by either normalizing the wire
values to the loop key or rejecting mismatches before persistence: construct or
overwrite IdentityKeyWire fields using the loop's wallet_id/identity_id/key_id
(from cs.upserts key and wallet_id variable) prior to blob::encode, or validate
that entry.identity_id, entry.key_id and entry.wallet_id exactly match the loop
key and return an error if they differ, then proceed to tx.execute.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rs`:
- Around line 19-37: The loop over cs.addresses currently ignores each entry's
own wallet_id and always writes using the outer wallet_id; add a fast-fail check
at the top of the loop that compares entry.wallet_id to the outer wallet_id (the
variables named entry and wallet_id in the cs.addresses loop) and return an
error if they differ (include a descriptive message mentioning the mismatched
wallet ids and the PlatformAddressBalanceEntry), before executing the INSERT;
this prevents silently writing mixed-wallet entries.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/wallet_meta.rs`:
- Around line 45-51: The row mapper in the wallet_metadata reader must reject
malformed rows instead of coercing them: in the stmt.query_map closure that
reads wallet_id (currently into bytes and wid) and birth_height (converted with
as u32), validate that wallet_id.bytes.len() == 32 and that birth_height is
within u32 bounds before converting; if either check fails return a real error
(e.g., a StorageError::CorruptedRow or map a descriptive
rusqlite::Error::InvalidParameterName/Other) from the closure so the query fails
instead of producing a zeroed wallet id or truncated height. Locate the closure
that does row.get(0) / copy_from_slice and the code that casts birth_height to
u32 and replace the coercion with explicit checks and an early Err(...) return
with a typed, descriptive error.

In `@packages/rs-platform-wallet-storage/tests/sqlite_auto_backup.rs`:
- Around line 87-106: The test's assertion that delete_wallet returns
AutoBackupDirUnwritable is flaky when run as root because chmod 0o500 doesn't
block root; modify the test in sqlite_auto_backup.rs (the block that creates
unwritable dir, calls SqlitePersisterConfig::with_auto_backup_dir,
SqlitePersister::open, and persister.delete_wallet) to detect running as root
(e.g., check geteuid()==0 on Unix) and in that case either skip the
exact-variant assertion or assert only that an error occurred, otherwise keep
the existing matches!(... AutoBackupDirUnwritable { .. }) check; this ensures
deterministic behavior without changing SqlitePersister/delete_wallet logic.

---

Nitpick comments:
In `@packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:
- Around line 185-191: The comment mentions layering constraints/indexes "via
`inject_custom`" but the code builds raw DDL by concatenating into `tail` after
calling `m.make::<Sqlite>()`; either (A) change the implementation to use
Barrel's `inject_custom` API to append the constraints/indexes instead of manual
string concatenation (replace the `tail` concatenation and use
`m.inject_custom(...)` at the appropriate spot), or (B) update the comment to
accurately describe the current behavior (mention that raw DDL is appended to
`tail` after `m.make::<Sqlite>()` and that `inject_custom` is not used) so
future maintainers are not misled; locate references to `tail`,
`m.make::<Sqlite>()`, and any subsequent emission of the SQL to modify
accordingly.
- Around line 281-326: The closure parent_check currently accepts an unused
parameter _cols: &[&str]; either remove that parameter from parent_check and all
its callers (keep it as parent_check(child: &str) and continue using wallet_id
inside the generated trigger SQL), or update parent_check to accept cols:
&[&str] and use cols[0] (or join cols) in place of the hard-coded wallet_id
everywhere (including the WHEN clauses and DELETE WHERE clause) so the same
helper can generate triggers for identities, identity_keys, dashpay_profiles,
etc.; ensure you update the for loop callers accordingly to pass either no cols
(if removed) or the appropriate slice like &["wallet_id"] or &["identity_id"]
where needed.

In `@packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs`:
- Around line 188-231: The migrate handling is split across two places; fold the
two separate `if let Cmd::Migrate` blocks into one contiguous block so all
migrate-specific validation and invocation live together. Locate the existing
`if let Cmd::Migrate(m) = &cli.cmd {` usage, move the checks that validate
`auto_backup_dir` and the `m.no_auto_backup` mutation of `config` (the use of
`SqlitePersisterConfig::new(&db)` and `config.with_auto_backup_dir(...)`) into
the same block where the migrate command is executed (or replace both with a
single `run_migrate` helper that accepts `db`, `m`, `config`, and
`auto_backup_dir`), then perform the `run_migrate` call and return its Result
immediately; finally remove the now-unnecessary second `if let Cmd::Migrate` and
the `unreachable!()` arm.
- Around line 122-134: In parse_wallet_id, stop echoing the raw input in the
length error and validate/normalize hex consistently: when s.len() != 64 return
an Err that only mentions the expected length (do not include `s`), and replace
relying solely on hex::decode with an explicit check that all chars are
lowercase hex (0-9,a-f) so uppercase is rejected; keep the existing "wallet id
is not valid hex" error for decoding failures but ensure messages follow the
same non-leaking style.

In `@packages/rs-platform-wallet-storage/src/sqlite/config.rs`:
- Around line 111-118: The function default_auto_backup_dir contains an overly
defensive check when computing parent from db_path; simplify by using
db_path.parent().unwrap_or_else(|| Path::new(".")) to get a &Path fallback and
then join "backups"/"auto" off that parent. Update the local variable used
(currently named parent) to hold the &Path from parent() rather than mapping to
a PathBuf, and then call parent.join("backups").join("auto") to produce the
final PathBuf.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs`:
- Around line 102-107: The IntegerOverflow error currently reports
SafeCastTarget::U64 for an i64→u32 conversion in asset_locks.rs (see the
u32::try_from usage producing WalletStorageError::IntegerOverflow), which is
misleading; add a U32 variant to crate::sqlite::util::safe_cast::SafeCastTarget
and implement a dedicated i64_to_u32 helper in the safe_cast module that
performs the conversion and returns WalletStorageError::IntegerOverflow with
target = SafeCastTarget::U32 on failure, then replace the manual u32::try_from
usages (e.g., the account_index conversion in asset_locks.rs and the similar
conversions in core_state.rs for core_utxos.height and core_utxos.account_index)
to call the new helper so errors accurately report the intended target type.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- Around line 72-102: ensure_exists currently performs an immediate INSERT OR
IGNORE on a plain &Connection, which bypasses the in-memory merge buffer and
per-flush transaction used by apply and can cause durability/ordering
mismatches; update ensure_exists to participate in the same flush boundary by
either accepting and using the existing merge buffer/flush transaction or a
transactional handle (e.g., a &Transaction or buffer API used by apply) and
executing the INSERT within that transaction, or if intentionally intended only
for tests, add a clear doc comment on ensure_exists describing that it writes
immediately to the DB and must not be used by production code that relies on the
merge-buffer + per-flush transaction model (reference ensure_exists, apply, and
the in-memory merge buffer/flush transaction behavior in your comment).

In `@packages/rs-platform-wallet-storage/src/sqlite/util/safe_cast.rs`:
- Around line 20-26: The SafeCastTarget enum currently only lists I64 and U64
but callers need a U32 target; add a new variant SafeCastTarget::U32 and
implement a small helper function i64_to_u32 that mirrors existing
i64_to_u64/i64_to_i64 behavior (validate range, return Result<u32,
SafeCastError> and report SafeCastTarget::U32 on failure). Update any match/log
paths in this module that construct errors to use the new U32 variant so callers
like asset_locks::list_active and core_state::list_unspent_utxos can call the
helper and produce the correct error shape/message.

In `@packages/rs-platform-wallet-storage/tests/common/mod.rs`:
- Around line 47-56: The helper ensure_wallet_meta currently hardcodes network =
'testnet', which can collide with tests expecting other networks; change the
signature of ensure_wallet_meta(persister: &SqlitePersister, wallet_id:
&WalletId) to accept a network parameter (e.g., network: &str) with callers
passing "testnet" where appropriate, update the INSERT SQL call in
ensure_wallet_meta to bind the network parameter instead of the literal
'testnet', and update all test call sites (or add a convenience wrapper) to pass
the correct network values (e.g., "mainnet" or "testnet") so tests see
consistent wallet_metadata rows.

In `@packages/rs-platform-wallet-storage/tests/sqlite_buffer_semantics.rs`:
- Around line 273-278: Remove the redundant dead-code shim `_unused_btreemap`
and its explanatory comment: delete the `#[allow(dead_code)] fn
_unused_btreemap() -> BTreeMap<u32, u32> { BTreeMap::new() }` block since
`BTreeMap` is now actually used in the test
`tc023_one_flush_is_one_transaction`, making the shim unnecessary; ensure
imports remain and run tests to confirm nothing else depends on that helper.
- Around line 154-180: Test currently calls fresh_persister() inside the
proptest closure causing a new on-disk SQLite DB per case; move persister
creation out of the proptest! Create the persister once before proptest (call
fresh_persister() once to get persister/_tmp/_path), then inside the proptest
closure use a new wallet id (wid) per case or clear/reset the core_sync_state
row (use ensure_wallet_meta and persister.store with core_with_height) so each
case reuses the same DB connection and only varies the wallet or row content;
update references to fresh_persister, wid, ensure_wallet_meta and persister
accordingly.

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

- [BLOCKING] In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:125-199: Carried-forward prior finding (STILL VALID): `restore_from` still does not serialize against concurrent SQLite writers
  `restore_from()` still documents the `_lock` as the mechanism that prevents a racing opener from recreating `-wal`/`-shm` during restore, but the implementation only takes an advisory `fs2::FileExt::try_lock_exclusive()` lock on the database file. Ordinary SQLite readers and writers do not participate in that flock, so they can continue using the database while this code removes siblings and atomically swaps the main file. The fallback branch at lines 192-198 also downgrades any non-`WouldBlock` lock failure to a warning and proceeds unlocked. In other words, the restore path still cannot enforce the serialization invariant it claims, so a concurrent process can race the restore and observe or create inconsistent wallet state.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:297-430: Carried-forward prior finding (STILL VALID): destructive auto-backups are still taken before any cross-process exclusion
  Both destructive rollback snapshots are still created before anything excludes other SQLite processes. `restore_from_inner()` takes the pre-restore backup at lines 297-313 and only then calls `backup::restore_from()`, while `delete_wallet_inner()` takes the pre-delete backup at lines 420-428 before starting the delete transaction at line 430. `run_auto_backup()` delegates to `backup::run_to()`, whose own contract says it is an online page-stepping backup that does not block writers (`backup.rs:51-53`). A concurrent process can therefore commit rows after the snapshot is taken but before the restore/delete lands, which means the advertised rollback backup can already be stale when the destructive operation succeeds.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/error.rs`:170-185: Carried-forward prior finding (STILL VALID): `ErrorNoSelectableInputs` is still unreachable from real auto-selection failures
  This PR now maps `PlatformWalletError::{NoSpendableInputs, OnlyOutputAddressesFunded, OnlyDustInputs}` to `ErrorNoSelectableInputs`, but those wallet error variants are still never constructed anywhere in the tree. The only in-repo references are the enum definition and the new FFI tests. The real platform-address auto-selection failures still return generic `AddressOperation` errors in `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs:235-238` and `packages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rs:209-231`, so FFI and Swift callers still fall through to `ErrorUnknown` for the exact cases this new ABI surface is meant to distinguish.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:1284-1303: Carried-forward prior finding (STILL VALID): `load()` still accepts a host-owned wallet-list allocation without the paired free callback
  `FFIPersister::load()` still only requires `on_load_wallet_list_fn`; it constructs `LoadGuard` with `self.callbacks.on_load_wallet_list_free_fn` even when that free callback is `None`. If the host callback returns a non-null `entries_ptr`/`count` but the paired free callback was not configured, Rust proceeds to read the host-owned allocation and the guard has no way to return it. That violates the callback ownership contract and leaks/strands the wallet-list allocation and nested buffers on every successful load.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:1572-1605: Carried-forward prior finding (STILL VALID): `on_get_core_tx_record_fn` can still return owned tx bytes without the paired free callback
  The tx-record path still accepts `tx_bytes_ptr` from `on_get_core_tx_record_fn` and builds `TxBytesGuard` with `self.callbacks.on_get_core_tx_record_free_fn`, but that field remains optional. When the host supplies a non-null tx-bytes buffer and forgets the matching free callback, the guard's `Drop` branch cannot call anything, so the allocation is never returned to the host allocator. The FFI contract should reject this configuration before accepting host-owned bytes, or otherwise require the paired free callback whenever the producer callback is installed.

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/backup.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/backup.rs:350-356: PruneReport.kept undercounts files that remain after failed removals
  `PruneReport.kept` is documented in `persister.rs:35` as 'Number of files that remain in the directory after pruning', but the loop only increments `kept` when the retention policy passes. When `remove_file()` fails the file remains on disk and is pushed onto `failed_removals`, but `kept` is not bumped. As a result `kept + removed.len() + failed_removals.len()` can exceed the documented invariant in the success direction (a file shows up in both surviving on disk and being missing from `kept`), so callers and tests that use `kept` to assert post-prune retention will misread the filesystem state under partial failure. Either bump `kept` when `remove_file` fails or amend the rustdoc to say `kept` is files retained by policy, distinct from files surviving due to deletion failure.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/backup.rs:181-203: restore_from advisory lock does not exclude concurrent SQLite writers; fallback proceeds unlocked
  `restore_from()` relies on `fs2::FileExt::try_lock_exclusive()` as the guard that makes WAL/SHM cleanup and atomic rename safe. That lock only serializes peers that voluntarily take the same fs2 lock — ordinary SQLite clients (other platform-wallet processes, external sqlite3 tools) use POSIX byte-range / OFD locks on their own connections and do not participate in fs2's flock, so they can keep writing through the restore window. Additionally, lines 192-199 downgrade non-WouldBlock lock-acquire errors to a tracing::warn! and proceed with `_lock = None`, so on filesystems where flock returns EINVAL/ENOTSUP (some NFS/FUSE backends) the restore runs entirely unguarded. The current head explicitly documents this trade-off in the comment at lines 168-180, which is why this is a suggestion and not a blocker: either (a) use SQLite's own exclusive locking (BEGIN EXCLUSIVE on the destination held across the rename, or rusqlite's backup API directly into a fresh connection that holds the write lock), or (b) expose a typed `WalletStorageError::AdvisoryLockUnavailable` and require callers to opt in to an unlocked restore via an explicit sibling (mirroring `delete_wallet_skip_backup`) so the safety bypass is at the call site, not buried in a warn line.

In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:297-430: Pre-restore and pre-delete auto-backups are taken before any cross-process exclusion
  Both destructive rollback snapshots are produced via `run_auto_backup` → `rusqlite::backup::Backup::run_to_completion`, which is a page-stepping online backup that does not block writers. In `restore_from_inner` (lines 297-315) the pre-restore backup is taken before `backup::restore_from` even attempts its advisory lock. In `delete_wallet_inner` (lines 420-429) the pre-delete backup is taken under the in-process connection mutex, but that mutex only excludes other threads inside this `SqlitePersister` instance — a second process opening the same `.db` file can commit a transaction between the backup's last copied page and the start of the delete transaction at line 430. The 'rollback safety net' that destructive operations advertise therefore does not necessarily capture state at destructive-op start time, only at backup-start time. Either acquire a SQLite-level exclusive lock (BEGIN EXCLUSIVE on the same connection) before invoking `run_auto_backup` for destructive ops, or tighten the rustdoc to state explicitly that the rollback snapshot reflects state at backup-start, not state at the moment the destructive op's transaction begins, and that single-process access is a precondition of the rollback guarantee.

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/persister.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:271-296: Carried-forward (STILL VALID): pre-restore auto-backup is taken before any SQLite-native exclusion on the destination
  `restore_from_inner` opens the destination read-only and page-streams the `PreRestore` auto-backup at persister.rs:283-293 before `backup::restore_from` acquires its destination `BEGIN EXCLUSIVE` (backup.rs:244-270). A cross-process peer (another rusqlite Connection / sibling `SqlitePersister`) can commit between the backup's last copied page and the EXCLUSIVE acquisition; those bytes appear in neither the rollback snapshot nor the post-restore state, and the rename silently drops them. The delete path already closes the equivalent gap with a pre/post-EXCLUSIVE `wallet_footprint` re-check that surfaces `ConcurrentMutationDuringDelete`. Restore has no analogous guard — the asymmetry is not load-bearing. Either snapshot under EXCLUSIVE (mirror the delete pattern: open a short-lived writer conn, `BEGIN EXCLUSIVE`, then drive the rusqlite Backup API while holding the lock) or expand the rustdoc to say plainly that the pre-restore snapshot reflects state at backup-start, not at restore-tx-start, and that callers must quiesce peers. Note: the lock-release-before-rename window further down is a separate, deliberately documented trade-off (see out-of-scope) — this finding is specifically about the backup-before-EXCLUSIVE gap.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:458-514: New latest-delta finding: post-EXCLUSIVE wallet-footprint re-check compares only row counts, so in-place UPDATE by a peer evades it
  The new guard at persister.rs:458-514 snapshots `wallet_footprint` before `run_auto_backup`, then re-snapshots under `BEGIN EXCLUSIVE` and aborts with `ConcurrentMutationDuringDelete` on mismatch. But `wallet_footprint` (persister.rs:1288-1305) and `wallet_footprint_tx` (:1309-1326) only collect `(table, row_count)` via `count_rows_for_wallet_sql`. Several per-wallet tables are single-row or upsert-style (`wallet_metadata`, `core_sync_state`, `platform_address_sync`), so a peer `UPDATE` that mutates existing-row contents without changing counts passes the check. In that case the cascade proceeds, but the pre-delete backup the operator is told they can roll back to is already stale relative to what was destroyed — defeating the new safe-by-default contract. Tighten the footprint to also fingerprint row contents (e.g., `SELECT COUNT(*), COALESCE(SUM(LENGTH(...)), 0)` or a per-table content hash) so an UPDATE-only peer also surfaces `ConcurrentMutationDuringDelete`.

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/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/error.rs:66-99: Dead error variants: NoSpendableInputs / OnlyOutputAddressesFunded / OnlyDustInputs have no production producer
  Carried-forward prior finding (STILL VALID at 016987c7). Repo-wide grep for these three variants returns only two files: the definition in packages/rs-platform-wallet/src/error.rs and the FFI classifier in packages/rs-platform-wallet-ffi/src/error.rs (match arm + its own unit tests). The actual auto-selection failure sites — wallet/platform_addresses/transfer.rs, wallet/platform_addresses/withdrawal.rs, wallet/identity/network/payments.rs — still collapse selection failures into stringly-typed AddressOperation(...) / TransactionBuild(...) errors. The FFI carves out a distinct ErrorNoSelectableInputs code that only its own tests can ever trip, which undermines the exhaustiveness contract the classifier advertises. Either wire the real selection sites to construct these variants (the shapes — funded_outputs, sub_min_count, sub_min_aggregate, min_input_amount — are already correct), or delete the variants and the FFI arm until a real producer exists.

In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:458-514: delete_wallet concurrent-mutation guard compares only row counts, not content
  Carried-forward prior finding (STILL VALID at 016987c7). wallet_footprint() / wallet_footprint_tx() (persister.rs:1284-1326) reduce each PER_WALLET_TABLES entry to (table_name, row_count). An in-place UPDATE that touches single-row or upsert-style tables — wallet_metadata, core_sync_state, platform_address_sync — will not change the row count, so the pre-backup and post-EXCLUSIVE snapshots compare equal even when the underlying content was overwritten by a racing writer. The cascade then proceeds and the advertised rollback backup is already stale relative to the state being destroyed. Strengthening the fingerprint to include a content hash (or a per-row monotonically updating revision column) closes the gap; alternatively, accept the limitation in rustdoc so callers understand the guarantee is row-count not row-content.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:277-295: Pre-restore auto-backup snapshot is taken before destination EXCLUSIVE lock
  Carried-forward prior finding (STILL VALID at 016987c7). restore_from_inner opens the destination read-only and runs run_auto_backup() to page-stream a snapshot to auto_backup_dir BEFORE backup::restore_from acquires the destination-side BEGIN EXCLUSIVE. A cross-process writer that commits during the page-step window contributes to neither the rollback snapshot nor the post-restore database. The delete path already defends the analogous gap with a footprint snapshot + post-lock re-check; restore has no equivalent guard. Either move the pre-restore snapshot inside the EXCLUSIVE-locked window, or document the restore-snapshot precondition explicitly in rustdoc so operators understand 'rollback to pre-restore snapshot' means 'rollback to the point the snapshot completed' rather than 'rollback to the instant restore was invoked'.

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/persister.rs`:
- [SUGGESTION] lines 458-514: delete_wallet concurrent-mutation guard compares only row counts, not content
  Carried-forward prior finding (STILL VALID at 016987c7). wallet_footprint() / wallet_footprint_tx() (persister.rs:1284-1326) reduce each PER_WALLET_TABLES entry to (table_name, row_count). An in-place UPDATE that touches single-row or upsert-style tables — wallet_metadata, core_sync_state, platform_address_sync — will not change the row count, so the pre-backup and post-EXCLUSIVE snapshots compare equal even when the underlying content was overwritten by a racing writer. The cascade then proceeds and the advertised rollback backup is already stale relative to the state being destroyed. Strengthening the fingerprint to include a content hash (or a per-row monotonically updating revision column) closes the gap; alternatively, accept the limitation in rustdoc so callers understand the guarantee is row-count not row-content.

In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- [SUGGESTION] lines 271-296: Pre-restore auto-backup snapshot is taken before the destination EXCLUSIVE lock
  `restore_from_inner` opens the destination read-only and runs `run_auto_backup()` to page-stream a snapshot to `auto_backup_dir` BEFORE `backup::restore_from` acquires the destination-side `BEGIN EXCLUSIVE`. A cross-process writer that commits during the page-step window contributes to neither the rollback snapshot (already drifting) nor the post-restore database (about to be wholesale replaced). The delete path already defends the analogous gap with a footprint snapshot + post-lock re-check (lines 458-514); restore has no equivalent guard. Either move the pre-restore snapshot inside the EXCLUSIVE-locked window, or document the precondition explicitly in rustdoc on `restore_from_backup` so operators understand 'rollback to pre-restore snapshot' means 'rollback to the point the snapshot completed' rather than 'rollback to the instant restore was invoked'.

In `packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs`:
- [NITPICK] lines 257-282: peek_schema_version uses rusqlite::Connection::open and can create a zero-byte file at the user-supplied path
  `peek_schema_version` opens the destination through `rusqlite::Connection::open(db)?` (line 259), which silently creates a zero-byte SQLite file at `db` if the path does not yet exist. It runs BEFORE `SqlitePersister::open` in the migrate arm (line 215). For the typical `migrate --db /new/path/wallets.db` case this is benign because the persister immediately migrates the freshly-created file. But (a) it bypasses the parent-directory existence guard the persister provides (NFR-6 / `WalletStorageError::Io(NotFound)`) and surfaces a raw rusqlite `CANTOPEN` instead, and (b) if `SqlitePersister::open` subsequently errors (e.g. `apply_pragmas` rejects journal_mode, or the auto-backup-disabled gate fires), an empty pre-migration `.db` file is left behind with default-umask permissions — `apply_secure_permissions` (chmod 600) is never reached on this path. Routing the pre-version probe through `crate::sqlite::conn::open_conn(_, Access::ReadOnly)` opens with `SQLITE_OPEN_READ_ONLY` and will not create the file.

In `packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:
- [NITPICK] lines 140-148: identity_keys.derivation_blob column is declared but never populated
  identity_keys.derivation_blob BLOB is declared in V001__initial.rs but the writer in schema/identity_keys.rs::apply always inserts NULL for that column and stuffs the entire IdentityKeyWire (including derivation_indices) into public_key_blob; decode_entry reads the wire from public_key_blob only. The column is dead schema. Not a correctness bug, but future readers may select the wrong column. Either populate the column or drop it via a follow-up migration (under append-only NFR-8 it would remain a documented dead column).

In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:
- [NITPICK] lines 311-348: restore_from unlinks WAL/SHM siblings while the destination lock connection is still alive
  The WAL/SHM sibling unlink at backup.rs:311-322 runs BEFORE `dest_lock_conn` is dropped at lines 342-348. On Unix the unlinked inodes remain reachable through the still-open fds, so the open `BEGIN EXCLUSIVE` tx (and the subsequent ROLLBACK) operate against orphaned inodes — functionally fine, but leaves SQLite holding handles for unlinked files. On Windows, `remove_file` against a file held open by another process returns `PermissionDenied`, which would surface as `WalletStorageError::Io`. The comment at lines 336-341 documents that lock release must happen before `persist` for inode-handle correctness — moving the sibling unlink to between `drop(lock_conn)` and `persist()` keeps both invariants. Low confidence because the current ordering may have been deliberate so step-5 sibling cleanup is atomic with the staged-file validation gate; flagging in case the trade-off was for the LIVE conn's WAL handles rather than the lock conn's.