feat(platform-wallet): persistence readers + seedless load() wiring (split from #3692)

[Claudius-Maginificent]

Why this PR exists

• Problem: feat(platform-wallet): watch-only rehydration from persistor (seedless load) #3692's load_from_persistor() has nothing to read from — the persister's load() returns an empty start-state. This PR is the storage-crate implementation that reconstructs persisted rows into the keyless ClientWalletStartState.
• Independence: rebuilt to stand alone on v3.1-dev. It carries only the storage work plus the single shared type (ClientWalletStartState, byte-identical to feat(platform-wallet): watch-only rehydration from persistor (seedless load) #3692). Because reshaping that struct removes wallet/wallet_info, two base consumers are stubbed with todo!() (manager/load.rs whole-body, ffi/persistence.rs tail-only) — both resolved by feat(platform-wallet): watch-only rehydration from persistor (seedless load) #3692 in the dash-evo-tool integration. The cross-crate manager-apply e2e tests are gated behind an off-by-default rehydration-apply feature (enabled in the integrated stack).

What was done (rs-platform-wallet-storage)

• Seedless SqlitePersister::load() wiring + per-area readers (accounts, core_state, identities, asset_locks unconsumed-only, contacts, identity_keys).
• refactor(platform-wallet)!: retire in-band pool snapshot; hardcode core UTXO account_index=0 #3828 schema rework — V001 edited in place (on-disk format unshipped) — plus the last_applied_chain_lock column.
• Secrets subsystem hardening + Tier-2 password-envelope format (golden vectors, fail-closed NeedsPassword, size caps, fd-lock pin).

Testing

cargo fmt --all --check; cargo clippy --workspace --all-targets -- -D warnings; cargo test -p platform-wallet-storage (222 passed; the gated cross-crate e2e runs in the integrated stack and passes there).

Breaking changes

None against v3.1-dev.

Checklist

• Self-review performed
• Tests added/updated
• Docs updated (SCHEMA.md / SECRETS.md / README)

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

Summary by CodeRabbit

• New Features

  • Added broader support for wallet rehydration data, including contacts, identity keys, account registrations, and asset-lock filtering.
  • Introduced stronger secret storage options, including protected and unprotected vault modes.

• Bug Fixes

  • Tightened validation for wallet storage reads to reject malformed, oversized, or mismatched data more reliably.
  • Improved handling of backups, restores, deletes, and schema checks to better preserve wallet state.

• Documentation

  • Updated storage, secrets, and schema docs to reflect the current wallet database layout and security behavior.

[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: 82640efe-eca1-446a-873f-4a4293333912

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

📝 Walkthrough

Walkthrough

This PR introduces a Tier-2 bincode secret-envelope wire format (scheme-0 plaintext / scheme-1 Argon2id+AEAD) under secrets/wire/, hardens EncryptedFileStore with blank-passphrase rejection, size caps, and fsync/permission fixes, and refactors SecretStore public API. In parallel, it renames the SQLite root table from wallet_metadata to wallets, promotes per-area schema readers (accounts, identities, identity_keys, contacts, asset_locks, core_state) from test-only to production, rewires SqlitePersister::load() to emit fully keyless ClientWalletStartState objects, and stubs load_from_persistor in the manager pending issue #3692.

Changes

Secret Store: Tier-2 Envelope Wire Format and EncryptedFileStore Hardening

Layer / File(s)	Summary
Wire format: Envelope, KDF, AAD, config src/secrets/wire/mod.rs, src/secrets/wire/config.rs, src/secrets/wire/kdf.rs, src/secrets/wire/aad.rs, src/secrets/wire/envelope.rs	New secrets/wire/ module with WIRE_CONFIG, domain-separation constants, KdfParamsEncoded, three AAD structs (Tier2Aad, EntryAad, VerifyAad), and the Envelope/Payload model with wrap/unwrap, decode-budget limiting, and a golden-vector + fuzz test suite.
SecretStoreError taxonomy src/secrets/error.rs	Adds ExpectedProtectedButUnsealed, NeedsPassword, WrongPassword, BlankPassphrase, UnsupportedEnvelopeVersion, NoEntry; routes tier-2 states into recoverable KeyringError::NoStorageAccess; extends tests for secret-free display and downcast recovery.
KDF/AEAD hardening and vault format AAD rebinding src/secrets/file/crypto.rs, src/secrets/file/format.rs	derive_key salt changed to &[u8; SALT_LEN]; AEAD construction failures remapped to Encrypt; aad()/verify_aad() now bincode-encode typed EntryAad/VerifyAad binding salt and KDF params; fuzz-resistance tests for deserialize.
EncryptedFileStore: passphrase/size/fsync/permission hardening src/secrets/file/mod.rs	open/rekey reject blank passphrases; put enforces MAX_SECRET_LEN; do_write_vault_at rejects oversized vaults; post-persist parent-fsync failures are logged not propagated; Drop re-asserts permissions best-effort; VaultLock adds unsafe Box reclaim; tests for plaintext-at-rest, crash-safety, size caps, and permission hardening.
SecretStore public API, SecretString/SecretBytes, keyring docs src/secrets/store.rs, src/secrets/secret.rs, src/secrets/mod.rs, src/secrets/keyring.rs	Adds file_unprotected; set delegates through set_secret → envelope::wrap → put_raw; get_secret does strict fail-closed unwrap; reprotect is read-then-rewrap; delete returns Ok(bool); OS get_raw caps blob size; SecretString gains is_blank(); mod.rs exports MAX_SECRET_LEN, MIN_PASSPHRASE_LEN, MAX_PLAINTEXT_LEN; strict-read quadrant, scheme-flip, password-lifecycle, and crash-safety tests.
Feature flags, audit.toml, SECRETS.md docs, compile tests Cargo.toml, .cargo/audit.toml, SECRETS.md, tests/secrets_*.rs	Adds secret-serde, secret-schemars, rehydration-apply features; pins fd-lock; ignores RUSTSEC-2025-0141 with rationale; SECRETS.md documents two-tier model, envelope format, strict-read table, and OS metadata enumerability; compile tests verify constant re-exports.

SQLite Wallets Rename, Keyless Rehydration, and Persister Hardening

Layer / File(s)	Summary
ClientWalletStartState keyless shape and load_from_persistor stub packages/rs-platform-wallet/src/changeset/client_wallet_start_state.rs, src/manager/load.rs, rs-platform-wallet-ffi/src/persistence.rs, src/lib.rs	Removes wallet/wallet_info fields; adds network, birth_height, account_manifest, core_state, contacts, identity_keys; load_from_persistor and FFI restore path become todo!(); pub const SIZE_LIMIT_BYTES added.
V001 migration wallets rename, blob sealed trait, safe_cast, schema/mod migrations/V001__initial.rs, src/sqlite/schema/mod.rs, src/sqlite/schema/wallets.rs, src/sqlite/schema/blob.rs, src/sqlite/util/safe_cast.rs	All FKs redirected from wallet_metadata to wallets; identity_keys gains wallet_id PK column; cascade trigger fires on wallets; PRAGMA application_id stamped; PersistableBlob sealed trait gates encode(); i64_to_u32 helper added.
Per-area load_state readers src/sqlite/schema/accounts.rs, src/sqlite/schema/identities.rs, src/sqlite/schema/identity_keys.rs, src/sqlite/schema/contacts.rs, src/sqlite/schema/asset_locks.rs, src/sqlite/schema/core_state.rs, src/sqlite/schema/platform_addrs.rs, src/sqlite/schema/dashpay.rs, src/sqlite/schema/token_balances.rs, src/kv.rs	accounts::load_state cross-checks typed columns vs blob; identities::load_state ungated; identity_keys::load_state added; contacts::load_changeset added; asset_locks::load_unconsumed filters consumed locks with status integrity check; core_state persists last_applied_chain_lock and uses hardcoded account index 0; platform_addrs counts only reconstructible addresses.
SqlitePersister open/load/delete/backup hardening src/sqlite/persister.rs, src/sqlite/migrations.rs, src/sqlite/error.rs, src/sqlite/conn.rs, src/sqlite/backup.rs, src/sqlite/config.rs, src/bin/platform-wallet-storage.rs	Open-path registry (AlreadyOpen guard); open validates application_id and schema-history; journal_mode readback validation; load() fully populates keyless wallets; delete_wallet uses wallets table; backup run_to fsyncs parent; restore validates application_id; prune uses within_floor; AccountRegistrationEntryMismatch variant added; UtxoAddressNotDerived removed.
Test suite: schema rename, new readers, persister integration tests/*	All test files updated for wallet_metadata→wallets; new modules for accounts reader, asset-lock filtering, contacts+keys rehydration, core-state reader, DashPay overlay, delete partial commit, FK ordering, identity keys reader, load wiring, money overflow, identity tombstone, second-open guard, wallet-DB identity, UTXO account-zero attribution, and aggressive prune timestamp ordering.
SCHEMA.md and README.md SCHEMA.md, README.md	All diagrams and FK descriptions redirected to wallets anchor; 21-table count; identity_keys two-FK exception documented; consumed asset-lock SQL filtering; load() narrative updated to keyless rehydration with wallets_pending_rehydration=0.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related issues

• register_wallet: partial-commit on restore-phase failure (persisted record orphaned) #3736: load_from_persistor restore-path stub and the overall wallet-load rehydration work are directly related to this issue.
• platform-wallet: orphan identities cannot be loaded — persistor writes them, but no consumer-side reader #3745: The identity load_state reader promotion, wallet-scoped tombstone logic, and identity-keys reader are directly related to supporting identity state in the keyless rehydration pipeline.

Possibly related PRs

• dashpay/platform#3634: Directly related — modifies the same persistence.rs restore path in rs-platform-wallet-ffi (building identity_manager/unused_asset_locks and assembling ClientWalletStartState), which this PR now stubs out.

Suggested labels

ready for final review

Suggested reviewers

• shumkov
• QuantumExplorer
• thepastaclaw

Poem

🐰 Hop hop, the wallet sheds its keys at rest,
A wallets table crowns the DB nest,
Bincode envelopes sealed with Argon2 might,
Consumed locks stay put, but filtered from sight,
todo! marks the path not yet complete—
The rabbit keeps building, one layer at a time, neat! 🔐

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: seedless persistence readers and load() wiring for platform-wallet, split from #3692.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/platform-wallet-storage-rehydration

---

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

[Claudius-Maginificent]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[thepastaclaw]

✅ Review complete (commit fa6a031)

[thepastaclaw]

Code Review

The PR adds the storage-side keyless load readers, but it also replaces two externally reachable restore paths with unconditional panics. The new rehydration readers are mostly wired, but several fail-hard corruption checks are missing where typed SQLite columns can disagree with decoded blobs.

🔴 2 blocking | 🟡 6 suggestion(s)

Findings not posted inline (2)

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

• [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:143-150: Identity reader trusts blob identity over the row key — load_state() selects identity_id but discards it, then decodes entry_blob and routes the restored identity using entry.id. The writer rejects IdentityEntry values whose blob ID disagrees with the typed column, but a restored or corrupted SQLite row can bypass the writer. The reader shou...
• [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:245-266: Contact reader does not validate request IDs against row keys — The contacts reader keys pending rows from (owner_id, contact_id) but stores the decoded ContactRequest without checking its sender and recipient IDs. During apply, sent requests are inserted under entry.request.recipient_id and incoming requests under entry.request.sender_id, so a row wh...

🤖 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/load.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/load.rs:13-15: Public manager restore API now panics
  `load_from_persistor()` is a public restore entry point returning `Result<(), PlatformWalletError>`, but this PR replaces the previous implementation with `todo!()`. The exported C ABI function `platform_wallet_manager_load_from_persistor` calls this method directly, and the Swift `loadFromPersistor()` wrapper calls that exported function, so any app invoking persisted wallet restore aborts instead of receiving a typed error. If this branch intentionally defers keyless manager rehydration to #3692, the public API still needs to fail closed with an error rather than panic across the FFI boundary.

In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:3389-3390: FFI persister load panics after receiving restore rows
  `FFIPersister::load()` calls `build_wallet_start_state()` for every wallet returned by the Swift `on_load_wallet_list_fn` callback, and this function now reaches an unconditional `todo!()` after partially reconstructing the entry. This path is externally reachable through restore and shielded binding flows that call `persister.load()`. A panic here can unwind toward `extern "C"` callers and abort the process instead of returning the existing `PersistenceError`/`PlatformWalletFFIResult` failure path.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:143-150: Identity reader trusts blob identity over the row key
  `load_state()` selects `identity_id` but discards it, then decodes `entry_blob` and routes the restored identity using `entry.id`. The writer rejects `IdentityEntry` values whose blob ID disagrees with the typed column, but a restored or corrupted SQLite row can bypass the writer. The reader should enforce the same column-vs-blob check, including wallet scope when `entry.wallet_id` is set, so semantic corruption fails the load instead of hydrating the wrong identity.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:143-150: Identity reader trusts blob identity over the row key
  `load_state()` selects `identity_id` but discards it, then decodes `entry_blob` and routes the restored identity using `entry.id`. The writer rejects `IdentityEntry` values whose blob ID disagrees with the typed column, but a restored or corrupted SQLite row can bypass the writer. The reader should enforce the same column-vs-blob check, including wallet scope when `entry.wallet_id` is set, so semantic corruption fails the load instead of hydrating the wrong identity.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:168-169: Identity-key reader does not verify decoded entries match row columns
  `load_state()` reconstructs `(identity_id, key_id)` from the SQL row, decodes `public_key_blob`, and inserts the decoded entry without checking that the blob carries the same identity, key id, wallet id, or public-key hash. The apply path later ignores the changeset map key and routes by fields from the decoded `IdentityKeyEntry`, so a semantically inconsistent row can attach a public key to the wrong identity or carry a hash that disagrees with the indexed column. Mirror the writer-side consistency checks on read before inserting into the changeset.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:245-266: Contact reader does not validate request IDs against row keys
  The contacts reader keys pending rows from `(owner_id, contact_id)` but stores the decoded `ContactRequest` without checking its sender and recipient IDs. During apply, sent requests are inserted under `entry.request.recipient_id` and incoming requests under `entry.request.sender_id`, so a row whose blob disagrees with the typed columns rehydrates under a different counterparty and later tombstones for the row key will not clear it. Established rows should also verify their outgoing and incoming requests match the same `(owner, contact)` relationship before accepting the row.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:245-266: Contact reader does not validate request IDs against row keys
  The contacts reader keys pending rows from `(owner_id, contact_id)` but stores the decoded `ContactRequest` without checking its sender and recipient IDs. During apply, sent requests are inserted under `entry.request.recipient_id` and incoming requests under `entry.request.sender_id`, so a row whose blob disagrees with the typed columns rehydrates under a different counterparty and later tombstones for the row key will not clear it. Established rows should also verify their outgoing and incoming requests match the same `(owner, contact)` relationship before accepting the row.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:316-325: Oversized BLOB rows are materialized before the size cap runs
  The new load readers fetch BLOB columns directly into `Vec<u8>` and only then call `blob::decode()`, whose 16 MiB cap runs after rusqlite has already allocated and copied the value. A restored or locally modified SQLite DB can therefore store a huge `record_blob` or other `*_blob` value that passes SQLite integrity checks and forces large process allocations on startup before returning `BlobTooLarge`. Use a shared bounded read helper or select `length(blob_column)` first, as the KV path already does, before materializing BLOB contents.

[coderabbitai]

Actionable comments posted: 16

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (7)

packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs (1)

165-180: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Count identity_keys by wallet_id now that the table is wallet-scoped.

identity_keys moved onto (wallet_id, identity_id, key_id), but this smoke test still routes it through the via_identity path. That means the assertion would still pass if the row were written with the wrong wallet_id as long as identity_id matched, so the new schema contract is not actually being exercised here.

Suggested fix

     let via_identity = [
-        "identity_keys",
         "token_balances",
         "dashpay_profiles",
         "dashpay_payments_overlay",
     ];

🤖 Prompt for AI Agents

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

In `@packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs` around lines
165 - 180, The smoke test still treats identity_keys as identity-scoped, but the
schema now scopes it by wallet_id. Update the test logic in sqlite_migrations.rs
so identity_keys uses the wallet_id COUNT query path instead of the via_identity
branch, while keeping the other tables that still depend on identities routed
through identity_id. Use the existing via_identity handling in the loop over
cases to locate and adjust the count_sql selection.

packages/rs-platform-wallet-storage/SCHEMA.md (1)

507-513: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

The soft-cascade note overstates cleanup for identity-scoped metadata.

meta_identity and meta_token do not carry wallet_id, so a wallet delete only reaches them through existing identities rows. If metadata was written before an identities row ever existed, that cleanup path never fires; the orphan-metadata section above already documents exactly that case.

Suggested wording

-`wallets` row fires a wallet-rooted `AFTER DELETE` trigger that
-brooms the wallet-scoped tables (`meta_wallet`, `meta_contact`,
-`meta_platform_address`) by `wallet_id`, and the FK cascade through
-`identities` fires a per-identity trigger that brooms `meta_identity` +
-`meta_token` by `identity_id`. Both legs key on the id alone, so a wallet
-delete cleans its metadata transitively whether or not the typed parent
-was ever written and regardless of any contact's lifecycle state.
+`wallets` row fires a wallet-rooted `AFTER DELETE` trigger that
+brooms the wallet-scoped tables (`meta_wallet`, `meta_contact`,
+`meta_platform_address`) by `wallet_id`, and the FK cascade through
+existing `identities` rows fires a per-identity trigger that brooms
+`meta_identity` + `meta_token` by `identity_id`. That means wallet-scoped
+metadata is cleaned regardless of typed-parent existence, while
+identity-scoped metadata still requires an `identities` row to exist.

🤖 Prompt for AI Agents

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

In `@packages/rs-platform-wallet-storage/SCHEMA.md` around lines 507 - 513, The
soft-cascade description in SCHEMA.md overstates what a wallet delete cleans up
for identity-scoped metadata. Update the note near the wallet/identity trigger
flow to say that `wallets` deletion only reaches `meta_identity` and
`meta_token` through existing `identities` rows and that orphan metadata written
before an `identities` row exists is not covered; align the wording with the
existing orphan-metadata section and reference the `wallets` trigger and the
`identities` FK cascade path.

packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs (1)

27-36: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Fail closed on corrupted platform-payment registration rows.

This helper trusts the typed account_index column but never verifies that the decoded AccountRegistrationEntry is actually a PlatformPayment entry for that same index. all_platform_payment_registrations() feeds platform_addrs::load_all(), so a tampered row will currently rehydrate under the typed index with the blob's xpub instead of tripping AccountRegistrationEntryMismatch.

Suggested fix

 fn decode_platform_payment_row(
     account_index: i64,
     xpub_bytes: &[u8],
 ) -> Result<PlatformPaymentRegistration, WalletStorageError> {
     let account_index = crate::sqlite::util::safe_cast::i64_to_u32(
         "account_registrations.account_index",
         account_index,
     )?;
     let entry: AccountRegistrationEntry = blob::decode(xpub_bytes)?;
+    if account_type_db_label(&entry.account_type) != "platform_payment"
+        || account_index(&entry.account_type) != account_index
+    {
+        return Err(WalletStorageError::AccountRegistrationEntryMismatch);
+    }
     Ok((account_index, entry.account_xpub))
 }

🤖 Prompt for AI Agents

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

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs` around
lines 27 - 36, `decode_platform_payment_row` currently decodes the blob and
returns the typed `account_index` without checking that the
`AccountRegistrationEntry` is a `PlatformPayment` for that same index. Update
this helper to validate the decoded `AccountRegistrationEntry` matches the
expected `PlatformPayment` variant and index, and return
`AccountRegistrationEntryMismatch` if it does not. Keep the existing
`safe_cast::i64_to_u32` conversion, but make
`all_platform_payment_registrations()` fail closed by rejecting any corrupted or
mismatched row instead of rehydrating it.

packages/rs-platform-wallet-storage/src/sqlite/backup.rs (2)

243-263: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift

Do not delete WAL/SHM before the replacement is guaranteed.

If sibling removal succeeds and tmp.persist(dest_db_path) then fails, the original main DB remains but its WAL/SHM may be gone, losing committed WAL-mode state. The restore path needs a rollback-safe swap strategy or a SQLite-native restore that does not destructively unlink siblings before the main replacement succeeds.

🤖 Prompt for AI Agents

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

In `@packages/rs-platform-wallet-storage/src/sqlite/backup.rs` around lines 243 -
263, The restore flow in `backup.rs` removes `-wal`/`-shm` siblings before
`tmp.persist(dest_db_path)`, which can leave the original DB intact but its
WAL-mode state lost if persist fails. Change the `restore` logic to use a
rollback-safe replacement strategy: do not unlink siblings until the destination
swap is guaranteed, or replace the whole SQLite set atomically via a
SQLite-native restore path. Keep the fix localized around the sibling cleanup
and `tmp.persist` sequence so the operation remains all-or-nothing.

---

361-374: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Apply keep_last_n as a floor, not a ceiling.

With both keep_last_n and max_age set, line 373 still requires pass_count, so backups beyond the newest N are deleted even when they are within max_age. That contradicts the new floor semantics.

Proposed fix

-        let pass_count = match policy.keep_last_n {
-            Some(n) => idx < n,
-            None => true,
-        };
         let pass_age = match policy.max_age {
             Some(max) => now.duration_since(ts).map(|d| d <= max).unwrap_or(true),
-            None => true,
+            None => policy.keep_last_n.is_none(),
         };
-        if within_floor || (pass_count && pass_age) {
+        if within_floor || pass_age {

🤖 Prompt for AI Agents

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

In `@packages/rs-platform-wallet-storage/src/sqlite/backup.rs` around lines 361 -
374, In backup pruning logic in the `retain_backups` flow, `keep_last_n` is
still being treated like a ceiling because the deletion condition requires
`pass_count` even when `max_age` is also set. Update the condition around
`within_floor`, `pass_count`, and `pass_age` so that the newest N backups are
always kept as a floor and any backup within the age limit is also retained,
using the existing `policy.keep_last_n` and `policy.max_age` checks in this
block.

packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs (1)

143-150: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Validate typed identity columns against the blob during load.

load_state ignores the selected identity_id, so a corrupted row whose typed column and entry_blob.id diverge is silently rehydrated under the blob value. Also reject a blob wallet_id that disagrees with the scoped wallet.

Proposed fix

-        let _identity_id: Vec<u8> = row.get(0)?;
+        let identity_id: Vec<u8> = row.get(0)?;
         let payload: Vec<u8> = row.get(1)?;
         let tombstoned: i64 = row.get(2)?;
         if tombstoned != 0 {
             continue;
         }
+        let typed_id = <[u8; 32]>::try_from(identity_id.as_slice())
+            .map_err(|_| WalletStorageError::blob_decode("identities.identity_id is not 32 bytes"))?;
         let entry: IdentityEntry = blob::decode(&payload)?;
+        if entry.id.as_bytes() != &typed_id {
+            return Err(WalletStorageError::IdentityEntryIdMismatch);
+        }
+        if let Some(entry_wallet_id) = entry.wallet_id {
+            if entry_wallet_id != *wallet_id {
+                return Err(WalletStorageError::WalletIdMismatch {
+                    expected: *wallet_id,
+                    found: entry_wallet_id,
+                });
+            }
+        }
         let managed = managed_identity_from_entry(&entry, wallet_id);

🤖 Prompt for AI Agents

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

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs` around
lines 143 - 150, The load path in load_state is trusting the blob too much and
currently ignores the selected identity_id, so mismatched typed columns can be
silently rehydrated under the blob value. Update the row handling in load_state
to validate that the typed identity_id matches entry_blob.id before decoding
into IdentityEntry, and also verify the blob wallet_id matches the wallet_id
scope passed into managed_identity_from_entry. If either check fails, reject the
row instead of continuing.

packages/rs-platform-wallet-storage/src/sqlite/persister.rs (1)

299-326: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Enforce the open-path registry before restore.

restore_from_inner can replace dest_db_path while a live SqlitePersister in this process still owns the same DB. Check the registry up front and return AlreadyOpen; otherwise the live handle/buffer can diverge from the restored file.

Proposed fix outline

+        let registered_path = dest_db_path
+            .canonicalize()
+            .unwrap_or_else(|_| dest_db_path.to_path_buf());
+        if open_path_registry()
+            .lock()
+            .unwrap_or_else(|p| p.into_inner())
+            .contains(&registered_path)
+        {
+            return Err(WalletStorageError::AlreadyOpen {
+                path: registered_path,
+            });
+        }
+
         if !skip_backup && dest_db_path.exists() {

🤖 Prompt for AI Agents

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

In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs` around lines 299
- 326, restore_from_inner currently restores the database without checking
whether the destination path is already owned by a live SqlitePersister, which
can leave an in-memory handle out of sync with the replaced file. Add an upfront
registry lookup in restore_from_inner for dest_db_path and return
WalletStorageError::AlreadyOpen when the path is already registered, before any
backup or restore work begins. Keep the change localized around
restore_from_inner and the open-path registry used by SqlitePersister so
existing live handles are protected from restore-time replacement.

🧹 Nitpick comments (4)

packages/rs-platform-wallet-storage/tests/sqlite_load_wiring.rs (1)

91-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert synced_height as well as last_processed_height.

This test writes both fields, but only validates one of them. If load() stops wiring synced_height, the round-trip still passes.

Suggested assertion

     assert_eq!(slice.core_state.new_utxos.len(), 1);
     assert_eq!(slice.core_state.new_utxos[0].value(), 777_000);
+    assert_eq!(slice.core_state.synced_height, Some(50));
     assert_eq!(slice.core_state.last_processed_height, Some(50));

🤖 Prompt for AI Agents

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

In `@packages/rs-platform-wallet-storage/tests/sqlite_load_wiring.rs` around lines
91 - 127, The round-trip test in `sqlite_load_wiring.rs` only verifies
`last_processed_height` from `state.wallets.get(&w).core_state` even though
`synced_height` is also written into `CoreChangeSet`; update the existing load
assertions to check both fields after `p2.load()` so `load()` wiring regressions
for `synced_height` are caught alongside `last_processed_height`.

packages/rs-platform-wallet-storage/tests/sqlite_dashpay_overlay_contract.rs (1)

93-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert that the overlay stays out of the rehydrated identity.

This currently proves only that load() still returns the wallet's core state. If a regression starts merging dashpay_profiles into the loaded identity payload, this test still passes. Please also assert that the seeded identity is present after load() and that its DashPay profile remains absent for the overlay-only write case.

🤖 Prompt for AI Agents

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

In `@packages/rs-platform-wallet-storage/tests/sqlite_dashpay_overlay_contract.rs`
around lines 93 - 108, The current test around persister.load() only verifies
wallet.core_state, so it can miss regressions where dashpay_profiles gets merged
into the rehydrated identity. Update the sqlite_dashpay_overlay_contract test to
also inspect the loaded identity payload for the seeded wallet after load() and
assert that the identity is still present while its DashPay profile remains
absent in this overlay-only write scenario. Use the existing persister.load(),
wallets.get(&w), and any identity fields already available in the loaded state
to make the check explicit.

packages/rs-platform-wallet-storage/tests/sqlite_delete_real_apply_failure.rs (1)

67-72: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Also assert that the failed pre-flush left nothing durable.

Restoring the buffer is only half of the contract here. If apply_changeset_to_tx ever leaks the wallets insert before the core_sync_state failure, this test still passes and leaves duplicate-on-retry state behind.

Suggested assertion block

     assert!(
         persister.buffer_has_changeset_for_test(&w),
         "buffered changeset must be restored after a real pre-flush apply failure"
     );
+
+    let conn = persister.lock_conn_for_test();
+    let wallets_rows: i64 = conn
+        .query_row(
+            "SELECT COUNT(*) FROM wallets WHERE wallet_id = ?1",
+            rusqlite::params![w.as_slice()],
+            |row| row.get(0),
+        )
+        .unwrap();
+    let core_rows: i64 = conn
+        .query_row(
+            "SELECT COUNT(*) FROM core_sync_state WHERE wallet_id = ?1",
+            rusqlite::params![w.as_slice()],
+            |row| row.get(0),
+        )
+        .unwrap();
+    assert_eq!(wallets_rows, 0, "failed pre-flush must not durably create the wallet row");
+    assert_eq!(core_rows, 0, "failed pre-flush must not durably create child rows");

🤖 Prompt for AI Agents

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

In
`@packages/rs-platform-wallet-storage/tests/sqlite_delete_real_apply_failure.rs`
around lines 67 - 72, The test currently only verifies the buffered changeset is
restored, but it should also verify that a failed pre-flush did not persist any
durable state. In sqlite_delete_real_apply_failure.rs, extend the existing
scenario around the failed delete so it checks the database/transaction state
after the apply failure and confirms no `wallets` insert or other durable side
effects remain from `apply_changeset_to_tx`. Keep the existing
`persister.buffer_has_changeset_for_test(&w)` assertion, and add a second
assertion in the same test that validates the storage is clean after the failure
so retry does not see duplicate-on-retry state.

packages/rs-platform-wallet-storage/src/sqlite/persister.rs (1)

813-814: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Fix the query-budget documentation.

load() currently performs multiple reader calls inside the for wallet_id in wallet_ids loop, so the query count grows with wallet count. Reword this to avoid promising constant query budget.

🤖 Prompt for AI Agents

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

In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs` around lines 813
- 814, Update the query-budget comment in the load path so it no longer claims
constant cost with wallet count; the current load() flow iterates over
wallet_ids and performs multiple reader calls per wallet, so reword the
documentation to describe that it has per-wallet read/query work rather than a
fixed query budget. Keep the note near the wallet_ids loop/load() implementation
and make sure the wording matches the actual behavior of the reader calls.

🤖 Prompt for all review comments with AI agents

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-ffi/src/persistence.rs`:
- Around line 3389-3390: The temporary restore stub in the persistence restore
flow should not panic via todo!(); replace it with a recoverable typed error so
callers receive a PersistenceError instead of crashing. Update the restore-path
branch that currently ignores identity_manager and unused_asset_locks to return
an appropriate PersistenceError variant (or equivalent error conversion) from
the same function/method, keeping the signature consistent and preserving the
existing error handling path.

In `@packages/rs-platform-wallet-storage/README.md`:
- Around line 165-168: The README wording around the manager-side rehydration
flow is too strong for this PR because the manager/FFI load path is still
stubbed. Update the description near the watch-only rebuild note to clearly mark
the manager-side `load_from_persistor`/`Wallet::new_watch_only` application as
pending or follow-up work, and keep the current text scoped to the storage-side
behavior only.

In `@packages/rs-platform-wallet-storage/src/kv.rs`:
- Around line 62-65: The key-length validation in validate_key currently assumes
Rust chars().count() matches SQLite length() for all strings, but embedded NULs
break that equivalence. Update the key precheck to explicitly reject keys
containing \0 before comparing length, or adjust the validation/comment so it no
longer claims the same key set; keep the logic aligned with the SQL CHECK
constraint in kv.rs.

In `@packages/rs-platform-wallet-storage/src/secrets/error.rs`:
- Around line 3-5: The file-level non-leakage docs in error.rs are too broad for
the current Io behavior: they claim variants never carry a stringified source,
but Io::fmt/rendering still exposes the underlying source text. Update the docs
to carve out the Io exception, or change Io’s display implementation/tests so it
no longer includes the source string, keeping the wording aligned with the
actual Error and Io rendering behavior.
- Around line 88-91: The UnsupportedEnvelopeVersion error currently truncates
the envelope version to u8, so update the error variant in error.rs to store the
full u32 version value instead. Then adjust the envelope parsing call site that
constructs UnsupportedEnvelopeVersion to pass the original Envelope.version
without narrowing, keeping the reported version accurate in the error message.

In `@packages/rs-platform-wallet-storage/src/secrets/file/format.rs`:
- Around line 21-22: The docs for the nested BTreeMap format currently imply
duplicate (wallet_id, label) pairs are prevented entirely, but the read path
still accepts duplicate JSON keys and serde collapses them. Update the
documentation near the format description to state that uniqueness is only
guaranteed by serialization, or change the deserialization logic in the file
format/parser code to explicitly reject duplicate keys, and make the behavior
match the tests and the intended API.

In `@packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- Around line 628-654: The post-persist Unix handling in the vault write path is
swallowing parent-directory fsync failures and returning success, which makes
`put`/`delete`/`rekey` report a durable commit when only the rename succeeded.
Update the flow around the `persist()`/`sync_all()` block to surface a distinct
“committed but not durable” result or otherwise keep the in-memory commit behind
the durability boundary, and make sure the caller can tell when
`fs::File::open(parent)` or `sync_all()` fails instead of only logging via
`tracing::warn!`.

In `@packages/rs-platform-wallet-storage/src/secrets/store.rs`:
- Around line 255-266: The reprotect method in SecretStore currently does a
non-atomic read-then-write using get_secret followed by set_secret, which can
overwrite concurrent updates with stale plaintext. Update reprotect to use an
atomic backend-specific reprotect/CAS path, or add a version check so the write
only succeeds if the entry has not changed since get_secret; reference
SecretStore::reprotect, get_secret, and set_secret when wiring the fix.

In `@packages/rs-platform-wallet-storage/src/secrets/wire/envelope.rs`:
- Around line 136-141: The scheme-0 plaintext path in the envelope handling
still leaves temporary Vec<u8> buffers unwiped, including the
Unprotected(plaintext.to_vec()) branch and the ExpectedProtectedButUnsealed arm.
Update the envelope logic in the encode/decode flow around the Envelope and
Payload handling to use zeroizing storage for these plaintext temporaries or
explicitly wipe them before drop, while keeping SecretBytes::new only for the
final encoded blob.

In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- Around line 179-199: `persist`/`open` currently treats `has_schema_history()`
as the only brand-new-vs-existing check, so a pre-existing non-wallet SQLite
file with no `refinery_schema_history` can still be migrated. Add an explicit
guard in the `had_schema_history` decision path to reject existing SQLite files
that lack wallet schema history, using the same `conn`/`has_schema_history` flow
and returning a typed wallet storage error before any backup, integrity check,
or `migrations::run()` work begins.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- Around line 143-154: The sync-state write path in core_state should treat
last_applied_chain_lock monotonically, not as a blind overwrite. Update the
CoreChangeSet-to-DB flow around upsert_sync_state so the stored chain-lock is
max-merged with the existing row (using the same chain-lock height comparison
logic as the height watermarks) before persisting. Apply this behavior wherever
last_applied_chain_lock is written in the affected core_state update functions
so the persisted chain-lock cannot regress.
- Around line 40-41: The `decode_from_slice` handling in
`last_applied_chain_lock` is too permissive because it accepts a valid prefix
and ignores any appended data. Update this decoding path in `core_state.rs` to
mirror the other blob decoders: after calling `bincode::decode_from_slice` for
`ChainLock`, verify the returned consumed length matches `bytes.len()` and treat
any mismatch as corruption by returning `None` instead of loading the state.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:
- Around line 151-169: Mirror the writer-side validation in load_state by
checking that each decoded public_key_blob matches the row’s typed columns
before inserting into cs.upserts. After decode_entry(&payload), verify the
entry’s identity_id, key_id, wallet_id, and public_key_hash against the values
from the identity_keys query, and return a WalletStorageError if any mismatch is
found. Keep the checks local to load_state and use the existing decode_entry,
Identifier::from, and KeyID::try_from flow so inconsistent rows are rejected
instead of loaded silently.

In `@packages/rs-platform-wallet-storage/tests/sqlite_accounts_reader.rs`:
- Around line 46-82: The sqlite_accounts_reader test is too weak because both
AccountRegistrationEntry fixtures use the same xpub and the assertions only
check set membership, so row reordering or xpub/row mixups can still pass.
Update the test to use distinct xpub fixtures for each entry and assert the
loaded manifest in the expected order, using the accounts::load_state result and
the existing AccountType variants to verify each row maps to the correct xpub.

In `@packages/rs-platform-wallet/src/changeset/client_wallet_start_state.rs`:
- Line 33: The doc comment on the wallet start state field still references the
old wallet_metadata table. Update the comment in client_wallet_start_state.rs to
point to the renamed wallets table instead, keeping the wording aligned with the
field’s source of truth and using the existing comment near the network field to
locate it.

In `@packages/rs-platform-wallet/src/manager/load.rs`:
- Around line 8-14: The public rehydration entry point
PlatformWalletManager::load_from_persistor currently panics via todo!, which
turns a caller error into a runtime abort. Replace the todo! with a recoverable
Result path by returning an explicit PlatformWalletError for the unsupported
stub state, or otherwise gate/remove this API until keyless rehydration in
PlatformWalletManager is implemented. Ensure callers receive an error instead of
a panic.

---

Outside diff comments:
In `@packages/rs-platform-wallet-storage/SCHEMA.md`:
- Around line 507-513: The soft-cascade description in SCHEMA.md overstates what
a wallet delete cleans up for identity-scoped metadata. Update the note near the
wallet/identity trigger flow to say that `wallets` deletion only reaches
`meta_identity` and `meta_token` through existing `identities` rows and that
orphan metadata written before an `identities` row exists is not covered; align
the wording with the existing orphan-metadata section and reference the
`wallets` trigger and the `identities` FK cascade path.

In `@packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:
- Around line 243-263: The restore flow in `backup.rs` removes `-wal`/`-shm`
siblings before `tmp.persist(dest_db_path)`, which can leave the original DB
intact but its WAL-mode state lost if persist fails. Change the `restore` logic
to use a rollback-safe replacement strategy: do not unlink siblings until the
destination swap is guaranteed, or replace the whole SQLite set atomically via a
SQLite-native restore path. Keep the fix localized around the sibling cleanup
and `tmp.persist` sequence so the operation remains all-or-nothing.
- Around line 361-374: In backup pruning logic in the `retain_backups` flow,
`keep_last_n` is still being treated like a ceiling because the deletion
condition requires `pass_count` even when `max_age` is also set. Update the
condition around `within_floor`, `pass_count`, and `pass_age` so that the newest
N backups are always kept as a floor and any backup within the age limit is also
retained, using the existing `policy.keep_last_n` and `policy.max_age` checks in
this block.

In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- Around line 299-326: restore_from_inner currently restores the database
without checking whether the destination path is already owned by a live
SqlitePersister, which can leave an in-memory handle out of sync with the
replaced file. Add an upfront registry lookup in restore_from_inner for
dest_db_path and return WalletStorageError::AlreadyOpen when the path is already
registered, before any backup or restore work begins. Keep the change localized
around restore_from_inner and the open-path registry used by SqlitePersister so
existing live handles are protected from restore-time replacement.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs`:
- Around line 27-36: `decode_platform_payment_row` currently decodes the blob
and returns the typed `account_index` without checking that the
`AccountRegistrationEntry` is a `PlatformPayment` for that same index. Update
this helper to validate the decoded `AccountRegistrationEntry` matches the
expected `PlatformPayment` variant and index, and return
`AccountRegistrationEntryMismatch` if it does not. Keep the existing
`safe_cast::i64_to_u32` conversion, but make
`all_platform_payment_registrations()` fail closed by rejecting any corrupted or
mismatched row instead of rehydrating it.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- Around line 143-150: The load path in load_state is trusting the blob too much
and currently ignores the selected identity_id, so mismatched typed columns can
be silently rehydrated under the blob value. Update the row handling in
load_state to validate that the typed identity_id matches entry_blob.id before
decoding into IdentityEntry, and also verify the blob wallet_id matches the
wallet_id scope passed into managed_identity_from_entry. If either check fails,
reject the row instead of continuing.

In `@packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs`:
- Around line 165-180: The smoke test still treats identity_keys as
identity-scoped, but the schema now scopes it by wallet_id. Update the test
logic in sqlite_migrations.rs so identity_keys uses the wallet_id COUNT query
path instead of the via_identity branch, while keeping the other tables that
still depend on identities routed through identity_id. Use the existing
via_identity handling in the loop over cases to locate and adjust the count_sql
selection.

---

Nitpick comments:
In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- Around line 813-814: Update the query-budget comment in the load path so it no
longer claims constant cost with wallet count; the current load() flow iterates
over wallet_ids and performs multiple reader calls per wallet, so reword the
documentation to describe that it has per-wallet read/query work rather than a
fixed query budget. Keep the note near the wallet_ids loop/load() implementation
and make sure the wording matches the actual behavior of the reader calls.

In
`@packages/rs-platform-wallet-storage/tests/sqlite_dashpay_overlay_contract.rs`:
- Around line 93-108: The current test around persister.load() only verifies
wallet.core_state, so it can miss regressions where dashpay_profiles gets merged
into the rehydrated identity. Update the sqlite_dashpay_overlay_contract test to
also inspect the loaded identity payload for the seeded wallet after load() and
assert that the identity is still present while its DashPay profile remains
absent in this overlay-only write scenario. Use the existing persister.load(),
wallets.get(&w), and any identity fields already available in the loaded state
to make the check explicit.

In
`@packages/rs-platform-wallet-storage/tests/sqlite_delete_real_apply_failure.rs`:
- Around line 67-72: The test currently only verifies the buffered changeset is
restored, but it should also verify that a failed pre-flush did not persist any
durable state. In sqlite_delete_real_apply_failure.rs, extend the existing
scenario around the failed delete so it checks the database/transaction state
after the apply failure and confirms no `wallets` insert or other durable side
effects remain from `apply_changeset_to_tx`. Keep the existing
`persister.buffer_has_changeset_for_test(&w)` assertion, and add a second
assertion in the same test that validates the storage is clean after the failure
so retry does not see duplicate-on-retry state.

In `@packages/rs-platform-wallet-storage/tests/sqlite_load_wiring.rs`:
- Around line 91-127: The round-trip test in `sqlite_load_wiring.rs` only
verifies `last_processed_height` from `state.wallets.get(&w).core_state` even
though `synced_height` is also written into `CoreChangeSet`; update the existing
load assertions to check both fields after `p2.load()` so `load()` wiring
regressions for `synced_height` are caught alongside `last_processed_height`.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: edc85543-e83f-4a54-88ef-17800859c720

📥 Commits

Reviewing files that changed from the base of the PR and between 83f7d4f and 2f2a74a.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (79)

• packages/rs-platform-wallet-ffi/src/persistence.rs
• packages/rs-platform-wallet-storage/.cargo/audit.toml
• packages/rs-platform-wallet-storage/Cargo.toml
• packages/rs-platform-wallet-storage/README.md
• packages/rs-platform-wallet-storage/SCHEMA.md
• packages/rs-platform-wallet-storage/SECRETS.md
• packages/rs-platform-wallet-storage/migrations/V001__initial.rs
• packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs
• packages/rs-platform-wallet-storage/src/kv.rs
• packages/rs-platform-wallet-storage/src/lib.rs
• packages/rs-platform-wallet-storage/src/secrets/error.rs
• packages/rs-platform-wallet-storage/src/secrets/file/crypto.rs
• packages/rs-platform-wallet-storage/src/secrets/file/format.rs
• packages/rs-platform-wallet-storage/src/secrets/file/mod.rs
• packages/rs-platform-wallet-storage/src/secrets/keyring.rs
• packages/rs-platform-wallet-storage/src/secrets/mod.rs
• packages/rs-platform-wallet-storage/src/secrets/secret.rs
• packages/rs-platform-wallet-storage/src/secrets/store.rs
• packages/rs-platform-wallet-storage/src/secrets/wire/aad.rs
• packages/rs-platform-wallet-storage/src/secrets/wire/config.rs
• packages/rs-platform-wallet-storage/src/secrets/wire/envelope.rs
• packages/rs-platform-wallet-storage/src/secrets/wire/kdf.rs
• packages/rs-platform-wallet-storage/src/secrets/wire/mod.rs
• packages/rs-platform-wallet-storage/src/sqlite/backup.rs
• packages/rs-platform-wallet-storage/src/sqlite/config.rs
• packages/rs-platform-wallet-storage/src/sqlite/conn.rs
• packages/rs-platform-wallet-storage/src/sqlite/error.rs
• packages/rs-platform-wallet-storage/src/sqlite/kv.rs
• packages/rs-platform-wallet-storage/src/sqlite/migrations.rs
• packages/rs-platform-wallet-storage/src/sqlite/persister.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/dashpay.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/mod.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/token_balances.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/wallets.rs
• packages/rs-platform-wallet-storage/src/sqlite/util/safe_cast.rs
• packages/rs-platform-wallet-storage/tests/common/mod.rs
• packages/rs-platform-wallet-storage/tests/secrets_api.rs
• packages/rs-platform-wallet-storage/tests/secrets_default_on_compiles.rs
• packages/rs-platform-wallet-storage/tests/secrets_scan.rs
• packages/rs-platform-wallet-storage/tests/sqlite_account_zero_attribution.rs
• packages/rs-platform-wallet-storage/tests/sqlite_accounts_reader.rs
• packages/rs-platform-wallet-storage/tests/sqlite_asset_locks_filter.rs
• packages/rs-platform-wallet-storage/tests/sqlite_auto_backup.rs
• packages/rs-platform-wallet-storage/tests/sqlite_check_constraints.rs
• packages/rs-platform-wallet-storage/tests/sqlite_commit_writes_lock_poison_shortcircuit.rs
• packages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs
• packages/rs-platform-wallet-storage/tests/sqlite_contacts_keys_rehydration.rs
• packages/rs-platform-wallet-storage/tests/sqlite_core_state_reader.rs
• packages/rs-platform-wallet-storage/tests/sqlite_dashpay_overlay_contract.rs
• packages/rs-platform-wallet-storage/tests/sqlite_delete_buffer_reconcile.rs
• packages/rs-platform-wallet-storage/tests/sqlite_delete_cross_process_exclusion.rs
• packages/rs-platform-wallet-storage/tests/sqlite_delete_partial_commit_window.rs
• packages/rs-platform-wallet-storage/tests/sqlite_delete_real_apply_failure.rs
• packages/rs-platform-wallet-storage/tests/sqlite_delete_wallet.rs
• packages/rs-platform-wallet-storage/tests/sqlite_error_classification.rs
• packages/rs-platform-wallet-storage/tests/sqlite_fk_changeset_ordering.rs
• packages/rs-platform-wallet-storage/tests/sqlite_foreign_keys.rs
• packages/rs-platform-wallet-storage/tests/sqlite_identity_keys_reader.rs
• packages/rs-platform-wallet-storage/tests/sqlite_load_reconstruction.rs
• packages/rs-platform-wallet-storage/tests/sqlite_load_wiring.rs
• packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs
• packages/rs-platform-wallet-storage/tests/sqlite_money_column_overflow_on_read.rs
• packages/rs-platform-wallet-storage/tests/sqlite_object_metadata.rs
• packages/rs-platform-wallet-storage/tests/sqlite_open_integrity_check.rs
• packages/rs-platform-wallet-storage/tests/sqlite_persist_roundtrip.rs
• packages/rs-platform-wallet-storage/tests/sqlite_qa_identity_tombstone.rs
• packages/rs-platform-wallet-storage/tests/sqlite_second_open_guard.rs
• packages/rs-platform-wallet-storage/tests/sqlite_structural_hardening.rs
• packages/rs-platform-wallet-storage/tests/sqlite_wallet_db_identity.rs
• packages/rs-platform-wallet/src/changeset/client_wallet_start_state.rs
• packages/rs-platform-wallet/src/manager/load.rs

[thepastaclaw]

Code Review

This cumulative pass confirms the three prior findings on the storage BLOB-decode discipline (chain-lock cap, platform-address fixed-width, nested identity-key bincode) remain FIXED at 12979b5 — the delta since 53b7d26 does not touch rs-platform-wallet-storage. One new blocking finding emerged: load_used_addresses (added in this PR at commit 6365be7 as the rehydration address-reuse guard) reads core_utxos.script into a Vec without the length()+blob::check_size gate that every sibling reader in the same file applies, re-opening exactly the class of hazard the earlier rehydration-reader hardening closed. Remaining findings are lower-severity: a doc/impl mismatch in load_and_apply_persisted (only re-hydrates platform addresses despite the doc implying full replay for late-registered accounts), a wallet_id/bucket migration gap on the update branch of apply_identity_entry, a handful of ungated/partially-gated BLOB reads on adjacent readers, an FFI helper missing the isize::MAX guard its sibling has, and a test-fixture coverage gap for the new rehydration path.

🔴 1 blocking | 🟡 6 suggestion(s) | 💬 3 nitpick(s)

Findings not posted inline (10)

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

• [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:421-439: load_used_addresses reads core_utxos.script with no BLOB size gate — Every other BLOB read in this file gates the column with a SELECT length(col), ... + blob::check_size() pre-read before materializing the Vec, per the file's own stated discipline (comment at lines 299-302: "Pre-read length() gates ... before materializing the Vec so tampered oversize values...
• [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1028-1064: load_and_apply_persisted discards reloaded core/identity state despite doc claiming full late-account replay — The doc comment states this is "the recommended entry point for startup hydration after late-registered accounts (e.g. DashPay contact accounts that bootstrap_dashpay_contact_accounts adds) have landed" and that "a second call after account bootstrap picks up the rest without regressing anyth...
• [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs:42-69: apply_identity_entry never migrates wallet_id/bucket on the update branch — On the existing-identity branch (lines 48-69), scalar fields are updated in place but existing.wallet_id is never reassigned and no bucket move between out_of_wallet_identities and wallet_identities[wallet_id] (with location_index update) is performed — unlike the fresh-insert path (lines...
• [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs:184-204: load_unconsumed materializes outpoint before any size gate — load_unconsumed gates lifecycle_blob with blob::check_size(row.get::<_, i64>(2)?)? (line 197) before reading it into a Vec, but op_bytes (the outpoint column, line 195) is read into a Vec with no length check — the same gap exists in the sibling load_state reader a few lines above (li...
• [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:362-380: core_instant_locks.txid materialized with no length/fixed-width gate — In the core_instant_locks block of load_state, islock_blob is gated with blob::check_size(row.get::<_, i64>(1)?)? (line 372) before materialization, but txid_bytes (column 0) is read directly into a Vec (line 371) with no length gate before dashcore::Txid::from_slice(&txid_bytes) is c...
• [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:4019-4025: slice_from_raw lacks the isize::MAX overflow guard its sibling helper has — decode_cmx_array (lines 2180-2190+) explicitly guards len against isize::MAX before calling from_raw_parts, per from_raw_parts's documented safety requirement. slice_from_raw calls slice::from_raw_parts(ptr, len) directly with only a null/zero check, no upper-bound check on len. `...
• [SUGGESTION] packages/rs-platform-wallet/tests/rehydration_load.rs:57-356: Rehydration integration tests never exercise identity_manager/contacts/identity_keys/unused_asset_locks fixtures — Fixtures in this file build ClientWalletStartState values but leave identity_manager, contacts, identity_keys, and unused_asset_locks at their default/empty values throughout. Given this PR wires IdentityManager::apply_contacts_and_keys and asset-lock flattening into `load_from_persis...
• [NITPICK] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:87-95: all_platform_payment_registrations open-codes the blob-size gate instead of using blob::check_size — Every other reader in this PR routes length(<col>) through the shared blob::check_size helper (e.g. identity_keys.rs:159, accounts.rs:179, core_state.rs:311/315/355). This one reader open-codes the same logic (usize::try_from(...).unwrap_or(usize::MAX) + inline BlobTooLarge construction)....
• [NITPICK] packages/rs-platform-wallet/src/manager/load.rs:153-157: Flattening unused_asset_locks silently drops attribution on outpoint collisions — tracked_asset_locks.extend(account_locks) overwrites the outer key when the inner outpoint collides across accounts. In practice outpoints are globally unique and TrackedAssetLock.account_index is already denormalized inside the value, so no data is actually lost — but the collision is silent...
• [NITPICK] packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:165-169: key_id cast error uses SafeCastTarget::U64 label but doesn't use the safe_cast helper — KeyID::try_from(key_id) (i64 → KeyID) is done inline with a manually constructed WalletStorageError::IntegerOverflow { ..., target: SafeCastTarget::U64, ... } rather than going through the crate::sqlite::util::safe_cast helpers used elsewhere in this file and sibling files. The U64 label...

🤖 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/schema/core_state.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:421-439: load_used_addresses reads core_utxos.script with no BLOB size gate
  Every other BLOB read in this file gates the column with a `SELECT length(col), ...` + `blob::check_size()` pre-read before materializing the Vec, per the file's own stated discipline (comment at lines 299-302: "Pre-read length() gates ... before materializing the Vec so tampered oversize values are caught before heap allocation"). `load_used_addresses` — added by this PR (commit 6365be79) as the rehydration address-reuse guard — breaks that pattern: the prepared SQL is `SELECT DISTINCT script FROM core_utxos ...`, executed via `query_map`, with `row.get::<_, Vec<u8>>(0)` called directly with zero size check. A tampered/corrupted SQLite file with an oversized `script` blob forces an unbounded heap allocation per row before any validation runs — the same hazard class as the three previously-fixed findings in this exact file. Since this reader is invoked from `persister.rs:948` on every rehydration load, it re-opens a hole the earlier hardening commits (`pre-read BLOB size-gate on rehydration readers`) explicitly closed on the sibling readers.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:362-380: core_instant_locks.txid materialized with no length/fixed-width gate
  In the `core_instant_locks` block of `load_state`, `islock_blob` is gated with `blob::check_size(row.get::<_, i64>(1)?)?` (line 372) before materialization, but `txid_bytes` (column 0) is read directly into a Vec (line 371) with no length gate before `dashcore::Txid::from_slice(&txid_bytes)` is called. `Txid` is a fixed 32-byte hash, so a tampered oversized `txid` column would still force an unbounded allocation before `from_slice` rejects the length. Add a `length(txid)` or `blob::check_fixed_width` gate before materializing `txid_bytes`, consistent with the fixed-width pattern used for other 32-byte identifiers in this PR (platform_addrs.rs).

In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1028-1064: load_and_apply_persisted discards reloaded core/identity state despite doc claiming full late-account replay
  The doc comment states this is "the recommended entry point for startup hydration *after* late-registered accounts (e.g. DashPay contact accounts that `bootstrap_dashpay_contact_accounts` adds) have landed" and that "a second call after account bootstrap picks up the rest without regressing anything." The implementation destructures `ClientStartState` and immediately discards `wallets: _` (which carries `core_state`, `identity_manager`, `contacts`, `identity_keys`, `unused_asset_locks` per `ClientWalletStartState`), only re-applying `platform_addresses`. If the intent is that late-registered accounts' core/identity state should be re-hydrated on this second call, that never happens — only platform-address state is reloaded. `ClientStartState`/`ClientWalletStartState` are new types introduced by this PR, so the doc/impl divergence originates here.

In `packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs:42-69: apply_identity_entry never migrates wallet_id/bucket on the update branch
  On the existing-identity branch (lines 48-69), scalar fields are updated in place but `existing.wallet_id` is never reassigned and no bucket move between `out_of_wallet_identities` and `wallet_identities[wallet_id]` (with `location_index` update) is performed — unlike the fresh-insert path (lines 84-140), which correctly sets `wallet_id` and calls `location_index_insert`. `IdentityChangeSet::merge` (changeset.rs:465) does set `existing.wallet_id = entry.wallet_id` on merge, and `managed.wallet_id` is mutated on already-managed identities in discovery.rs:275, registration.rs:296, and loading.rs:251. An identity discovered out-of-wallet and later associated with a wallet can end up with a changeset `wallet_id` that never gets replayed into the correct bucket on restart, leaving it stuck in `out_of_wallet_identities` after a reload even though its live in-memory state (before persistence) had it correctly bucketed. Either narrow the doc comment to explicitly scope out wallet_id/bucket migration on this branch, or add the migration logic mirroring the fresh-insert path.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs:184-204: load_unconsumed materializes outpoint before any size gate
  `load_unconsumed` gates `lifecycle_blob` with `blob::check_size(row.get::<_, i64>(2)?)?` (line 197) before reading it into a Vec, but `op_bytes` (the `outpoint` column, line 195) is read into a Vec with no length check — the same gap exists in the sibling `load_state` reader a few lines above (line 167). Outpoints are fixed-width (36 bytes), so this is a smaller allocation-size hazard than the blocking finding above, but it breaks the file's stated pre-read length-gate discipline and a tampered row could still force a larger-than-expected allocation before `decode_row` rejects it. Same class of fix as the blocking finding: add a `length(outpoint)` gate (or a fixed-width check).

In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:4019-4025: slice_from_raw lacks the isize::MAX overflow guard its sibling helper has
  `decode_cmx_array` (lines 2180-2190+) explicitly guards `len` against `isize::MAX` before calling `from_raw_parts`, per `from_raw_parts`'s documented safety requirement. `slice_from_raw` calls `slice::from_raw_parts(ptr, len)` directly with only a null/zero check, no upper-bound check on `len`. `len` is host-supplied (Swift-side), so this is lower risk than a fully adversarial-DB path, but it's an inconsistency with the hardening applied to the sibling helper in the same file. Add the same `isize::MAX` bound check.

In `packages/rs-platform-wallet/tests/rehydration_load.rs`:
- [SUGGESTION] packages/rs-platform-wallet/tests/rehydration_load.rs:57-356: Rehydration integration tests never exercise identity_manager/contacts/identity_keys/unused_asset_locks fixtures
  Fixtures in this file build `ClientWalletStartState` values but leave `identity_manager`, `contacts`, `identity_keys`, and `unused_asset_locks` at their default/empty values throughout. Given this PR wires `IdentityManager::apply_contacts_and_keys` and asset-lock flattening into `load_from_persistor` (load.rs:167-177, 154-157), there's no integration-level coverage confirming identities, contacts, keys, or asset locks actually survive a full `load_from_persistor` round trip — only the lower-level unit tests in rehydrate.rs and apply.rs cover these paths in isolation. Add at least one integration fixture that populates these fields and asserts they're correctly present after `load_from_persistor`.

Note: GitHub refused the PR diff for inline mapping (PullRequest.diff too_large), so this review is posted body-only while preserving the verified findings.

[thepastaclaw]

Code Review

Cumulative pass at fa6a031. Carried-forward prior findings: all 10 findings from 12979b5 are STILL VALID by direct re-inspection and are included below; none of the files carrying them were touched by the latest delta. New latest-delta finding: one nitpick in rehydrate.rs where a doc comment still describes CoinJoin as single-pool even though this delta's own test asserts CoinJoin now carries both External and Internal pools.

🔴 1 blocking | 🟡 6 suggestion(s) | 💬 4 nitpick(s)

Verified findings (11)

These findings are body-only because GitHub refuses the PR diff (PullRequest.diff too_large). Prior findings marked STILL VALID were re-checked against fa6a031c and intentionally carried forward.

• [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:421-439: load_used_addresses reads core_utxos.script with no BLOB size gate — Verified STILL VALID at fa6a031 — file untouched by this delta. Every other BLOB reader in this same file pre-reads length(col) and calls blob::check_size() before materializing the Vec (see the utxo/record/islock/chain_lock readers at lines 305–316, 355, 372, 396), and the file's own comment at lines 299–302 states this discipline explicitly. load_used_addresses breaks it: SELECT DISTINCT script FROM core_utxos ... executed via query_map calls row.get::<_, Vec<u8>>(0) directly with zero size check. Since this reader is invoked from persister.rs on every rehydration load, a tampered/corrupted SQLite file with an oversized script blob forces an unbounded heap allocation per row before any validation runs — re-opening the exact hazard class the sibling readers were previously hardened against.
• [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1028-1064: load_and_apply_persisted discards reloaded core/identity state despite doc claiming full late-account replay — Verified STILL VALID at fa6a031 — file untouched by this delta. The doc comment (lines 1037–1044) advertises this as the entry point for hydration "after late-registered accounts ... have landed" where "a second call after account bootstrap picks up the rest without regressing anything." The implementation destructures ClientStartState and immediately discards wallets: _ at line 1050 — the field that carries core_state, identity_manager, contacts, identity_keys, and unused_asset_locks — only re-applying platform_addresses. Either narrow the doc comment to platform-address re-hydration only, or extend the function to also apply the reloaded wallets state for the current wallet_id.
• [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs:42-69: apply_identity_entry never migrates wallet_id/bucket on the update branch — Verified STILL VALID at fa6a031 — file untouched by this delta. The existing-identity branch (lines 48–69) updates scalar fields in place but never reassigns existing.wallet_id and never moves the identity between out_of_wallet_identities and wallet_identities[wallet_id] (with location_index update), unlike the fresh-insert path at lines 84–140. IdentityChangeSet::merge does set existing.wallet_id = entry.wallet_id on merge, and managed.wallet_id is mutated on already-managed identities in discovery.rs / registration.rs / loading.rs. An identity discovered out-of-wallet and later associated with a wallet can therefore end up with a changeset wallet_id that never gets replayed into the correct bucket on restart.
• [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs:184-204: load_unconsumed materializes outpoint before any size gate — Verified STILL VALID at fa6a031 — file untouched by this delta. lifecycle_blob is gated with blob::check_size(row.get::<_, i64>(2)?)? at line 197, but op_bytes (the outpoint column, line 195) is materialized into a Vec with no length check. The sibling load_state reader (line 167) has the same gap. Outpoints are fixed-width (36 bytes), so the allocation-size hazard is smaller than the blocking finding above, but it still breaks the file's stated pre-read length-gate discipline. Add a length(outpoint) gate or a fixed-width check before materializing.
• [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:362-380: core_instant_locks.txid materialized with no length/fixed-width gate — Verified STILL VALID at fa6a031 — file untouched by this delta. islock_blob is gated with blob::check_size at line 372, but txid_bytes (column 0, line 371) is read directly into a Vec with no length gate before dashcore::Txid::from_slice(&txid_bytes) is called. Txid is a fixed 32-byte hash, so a tampered oversized txid column would still force an unbounded allocation before from_slice rejects the length. Add a length(txid) or fixed-width gate consistent with the pattern used elsewhere in the PR.
• [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:4019-4025: slice_from_raw lacks the isize::MAX overflow guard its sibling helper has — Verified STILL VALID at fa6a031 — file untouched by this delta. slice_from_raw calls slice::from_raw_parts(ptr, len) with only a null/zero check. The sibling decode_cmx_array in the same file explicitly guards len against isize::MAX per from_raw_parts's documented safety requirement. len is host-supplied (Swift-side over FFI), so exploitability requires a malicious/buggy host rather than adversarial DB, but the asymmetry with the sibling helper at the same FFI boundary should be closed.
• [SUGGESTION] packages/rs-platform-wallet/tests/rehydration_load.rs:57-356: Rehydration integration tests never exercise identity_manager/contacts/identity_keys/unused_asset_locks fixtures — Verified STILL VALID at fa6a031 — file untouched by this delta. Every ClientWalletStartState fixture (lines 73–76, 133–136, 284–287, 351–354) leaves identity_manager, contacts, identity_keys, and unused_asset_locks at Default::default(). Given this PR wires IdentityManager::apply_contacts_and_keys and asset-lock flattening into load_from_persistor (load.rs:167–177, 154–157), there's no integration-level coverage confirming identities, contacts, keys, or asset locks survive a full load_from_persistor round trip. Add at least one integration fixture that populates these fields and asserts they're correctly present after load_from_persistor.
• [NITPICK] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:87-95: all_platform_payment_registrations open-codes the blob-size gate instead of using blob::check_size — Verified STILL VALID at fa6a031 — file untouched by this delta. This reader open-codes usize::try_from(...).unwrap_or(usize::MAX) + inline BlobTooLarge construction, while every other reader in the PR routes through blob::check_size (identity_keys.rs:159, accounts.rs:179, core_state.rs:311/315/355). Functionally equivalent today, but any future tweak to the size-gate semantics has to touch two paths. Route through the shared helper for consistency.
• [NITPICK] packages/rs-platform-wallet/src/manager/load.rs:153-157: Flattening unused_asset_locks silently drops attribution on outpoint collisions — Verified STILL VALID at fa6a031 — file untouched by this delta. tracked_asset_locks.extend(account_locks) overwrites the outer key on outpoint collisions across accounts. Outpoints are globally unique in practice and TrackedAssetLock.account_index is denormalized inside the value, so no real data loss — but the collision is silent. A debug_assert! on duplicate insertion would keep the invariant honest without changing release behavior.
• [NITPICK] packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:165-169: key_id cast error uses SafeCastTarget::U64 label but doesn't use the safe_cast helper — Verified STILL VALID at fa6a031 — file untouched by this delta. KeyID::try_from(key_id) (i64 → KeyID) is done inline with a manually constructed WalletStorageError::IntegerOverflow { ..., target: SafeCastTarget::U64, ... } rather than going through the crate::sqlite::util::safe_cast helpers used elsewhere in this file and sibling files. The U64 label is also questionable if KeyID is narrower than u64. Cosmetic/consistency issue only — KeyID::try_from itself still bounds-checks.
• [NITPICK] packages/rs-platform-wallet/src/manager/rehydrate.rs:301-308: extend_pools_for_restored_addresses doc contradicts this delta's own CoinJoin test on pool topology — New in this delta. The rust-dashcore 0.45 bump changed CoinJoin's account topology: the delta's own test at rehydrate.rs:912–917 states "CoinJoin accounts carry both an External and an Internal pool (mirroring Standard)" and the fixture at line 962 / 1032 unwraps an External pool from a CoinJoin account. But the production doc comment on extend_pools_for_restored_addresses (line 302) still reads "CoinJoin topology (single External pool)". The function's logic itself is topology-agnostic (fully positional, fail-closed on pool_type mismatch), so there's no functional bug — but the doc is now stale and can mislead a future reader about CoinJoin's actual pool count.

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

- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:421-439: load_used_addresses reads core_utxos.script with no BLOB size gate
  Verified STILL VALID at fa6a031c — file untouched by this delta. Every other BLOB reader in this same file pre-reads `length(col)` and calls `blob::check_size()` before materializing the Vec (see the utxo/record/islock/chain_lock readers at lines 305–316, 355, 372, 396), and the file's own comment at lines 299–302 states this discipline explicitly. `load_used_addresses` breaks it: `SELECT DISTINCT script FROM core_utxos ...` executed via `query_map` calls `row.get::<_, Vec<u8>>(0)` directly with zero size check. Since this reader is invoked from `persister.rs` on every rehydration load, a tampered/corrupted SQLite file with an oversized `script` blob forces an unbounded heap allocation per row before any validation runs — re-opening the exact hazard class the sibling readers were previously hardened against.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1028-1064: load_and_apply_persisted discards reloaded core/identity state despite doc claiming full late-account replay
  Verified STILL VALID at fa6a031c — file untouched by this delta. The doc comment (lines 1037–1044) advertises this as the entry point for hydration "*after* late-registered accounts ... have landed" where "a second call after account bootstrap picks up the rest without regressing anything." The implementation destructures `ClientStartState` and immediately discards `wallets: _` at line 1050 — the field that carries `core_state`, `identity_manager`, `contacts`, `identity_keys`, and `unused_asset_locks` — only re-applying `platform_addresses`. Either narrow the doc comment to platform-address re-hydration only, or extend the function to also apply the reloaded `wallets` state for the current `wallet_id`.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs:42-69: apply_identity_entry never migrates wallet_id/bucket on the update branch
  Verified STILL VALID at fa6a031c — file untouched by this delta. The existing-identity branch (lines 48–69) updates scalar fields in place but never reassigns `existing.wallet_id` and never moves the identity between `out_of_wallet_identities` and `wallet_identities[wallet_id]` (with `location_index` update), unlike the fresh-insert path at lines 84–140. `IdentityChangeSet::merge` does set `existing.wallet_id = entry.wallet_id` on merge, and `managed.wallet_id` is mutated on already-managed identities in discovery.rs / registration.rs / loading.rs. An identity discovered out-of-wallet and later associated with a wallet can therefore end up with a changeset `wallet_id` that never gets replayed into the correct bucket on restart.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs:184-204: load_unconsumed materializes outpoint before any size gate
  Verified STILL VALID at fa6a031c — file untouched by this delta. `lifecycle_blob` is gated with `blob::check_size(row.get::<_, i64>(2)?)?` at line 197, but `op_bytes` (the outpoint column, line 195) is materialized into a Vec with no length check. The sibling `load_state` reader (line 167) has the same gap. Outpoints are fixed-width (36 bytes), so the allocation-size hazard is smaller than the blocking finding above, but it still breaks the file's stated pre-read length-gate discipline. Add a `length(outpoint)` gate or a fixed-width check before materializing.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:362-380: core_instant_locks.txid materialized with no length/fixed-width gate
  Verified STILL VALID at fa6a031c — file untouched by this delta. `islock_blob` is gated with `blob::check_size` at line 372, but `txid_bytes` (column 0, line 371) is read directly into a Vec with no length gate before `dashcore::Txid::from_slice(&txid_bytes)` is called. `Txid` is a fixed 32-byte hash, so a tampered oversized `txid` column would still force an unbounded allocation before `from_slice` rejects the length. Add a `length(txid)` or fixed-width gate consistent with the pattern used elsewhere in the PR.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:4019-4025: slice_from_raw lacks the isize::MAX overflow guard its sibling helper has
  Verified STILL VALID at fa6a031c — file untouched by this delta. `slice_from_raw` calls `slice::from_raw_parts(ptr, len)` with only a null/zero check. The sibling `decode_cmx_array` in the same file explicitly guards `len` against `isize::MAX` per `from_raw_parts`'s documented safety requirement. `len` is host-supplied (Swift-side over FFI), so exploitability requires a malicious/buggy host rather than adversarial DB, but the asymmetry with the sibling helper at the same FFI boundary should be closed.
- [SUGGESTION] packages/rs-platform-wallet/tests/rehydration_load.rs:57-356: Rehydration integration tests never exercise identity_manager/contacts/identity_keys/unused_asset_locks fixtures
  Verified STILL VALID at fa6a031c — file untouched by this delta. Every `ClientWalletStartState` fixture (lines 73–76, 133–136, 284–287, 351–354) leaves `identity_manager`, `contacts`, `identity_keys`, and `unused_asset_locks` at `Default::default()`. Given this PR wires `IdentityManager::apply_contacts_and_keys` and asset-lock flattening into `load_from_persistor` (load.rs:167–177, 154–157), there's no integration-level coverage confirming identities, contacts, keys, or asset locks survive a full `load_from_persistor` round trip. Add at least one integration fixture that populates these fields and asserts they're correctly present after `load_from_persistor`.
- [NITPICK] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:87-95: all_platform_payment_registrations open-codes the blob-size gate instead of using blob::check_size
  Verified STILL VALID at fa6a031c — file untouched by this delta. This reader open-codes `usize::try_from(...).unwrap_or(usize::MAX)` + inline `BlobTooLarge` construction, while every other reader in the PR routes through `blob::check_size` (identity_keys.rs:159, accounts.rs:179, core_state.rs:311/315/355). Functionally equivalent today, but any future tweak to the size-gate semantics has to touch two paths. Route through the shared helper for consistency.
- [NITPICK] packages/rs-platform-wallet/src/manager/load.rs:153-157: Flattening unused_asset_locks silently drops attribution on outpoint collisions
  Verified STILL VALID at fa6a031c — file untouched by this delta. `tracked_asset_locks.extend(account_locks)` overwrites the outer key on outpoint collisions across accounts. Outpoints are globally unique in practice and `TrackedAssetLock.account_index` is denormalized inside the value, so no real data loss — but the collision is silent. A `debug_assert!` on duplicate insertion would keep the invariant honest without changing release behavior.
- [NITPICK] packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:165-169: key_id cast error uses SafeCastTarget::U64 label but doesn't use the safe_cast helper
  Verified STILL VALID at fa6a031c — file untouched by this delta. `KeyID::try_from(key_id)` (i64 → KeyID) is done inline with a manually constructed `WalletStorageError::IntegerOverflow { ..., target: SafeCastTarget::U64, ... }` rather than going through the `crate::sqlite::util::safe_cast` helpers used elsewhere in this file and sibling files. The `U64` label is also questionable if `KeyID` is narrower than u64. Cosmetic/consistency issue only — `KeyID::try_from` itself still bounds-checks.
- [NITPICK] packages/rs-platform-wallet/src/manager/rehydrate.rs:301-308: extend_pools_for_restored_addresses doc contradicts this delta's own CoinJoin test on pool topology
  New in this delta. The rust-dashcore 0.45 bump changed CoinJoin's account topology: the delta's own test at rehydrate.rs:912–917 states "CoinJoin accounts carry both an External and an Internal pool (mirroring `Standard`)" and the fixture at line 962 / 1032 unwraps an `External` pool from a CoinJoin account. But the production doc comment on `extend_pools_for_restored_addresses` (line 302) still reads "CoinJoin topology (single External pool)". The function's logic itself is topology-agnostic (fully positional, fail-closed on `pool_type` mismatch), so there's no functional bug — but the doc is now stale and can mislead a future reader about CoinJoin's actual pool count.

Note: Normal review_poster dry-run failed because GitHub refused the PR diff (PullRequest.diff too_large), so this is posted as a top-level exact-SHA review body.

Reviewed commit: fa6a031