feat(platform-wallet-storage): snapshot readers + unified V002 schema migration

[Claudius-Maginificent]

Issue being fixed or feature implemented

Redirects #3968 (stacked on its head branch): the persisted store cannot yet serve a verbatim snapshot at load. Core address pools have no per-index rows (used_core_addresses is re-derived at read time), every UTXO is attributed to hardcoded account_index 0, and there is no per-domain versioning or store-generation identity — blocking verbatim rehydration (the live e2e deep-derivation-window restore blocker) and any client cache-invalidation strategy.

What was done?

One additive V002 migration (V001 untouched — refinery checksums make it immutable) plus writers and snapshot readers:

• core_address_pool: per-index pool rows with monotonic used flags. Primary key is (wallet_id, account_type, account_index, key_class, pool_type, address_index) — account_type is included so distinct account kinds that share the same numeric (account_index, key_class) (e.g. IdentityRegistration, ProviderVotingKeys, or Standard/CoinJoin both at index 0) never collide; pool_type keeps External/Internal from colliding; a script column supports verbatim reads and UTXO attribution. Persisted idempotently per changeset.
• Real account_index: per-UTXO account attribution via pool-row script match (deterministic tie-break: ORDER BY account_type, account_index, key_class, pool_type, address_index when multiple rows share a script); hardcoded CORE_UTXO_ACCOUNT_INDEX = 0 deleted.
• meta_data_versions (wallet_id, domain, seq): saturating per-domain bump executed inside the single flush transaction — atomic with the data it versions. Exhaustive changeset destructure (re-armed via a pass-through shielded feature) makes adding a changeset field without wiring its domain a compile error.
• meta_store_generation: 16-byte store-generation token. Rotation is folded into the pre-swap staged file during restore, so the atomic rename swaps in the restored bytes and the fresh token together — no window where restored content is observable under a stale token.
• Snapshot reader: used_core_addresses read verbatim from pool rows, UNIONED with the UTXO-derived set (mixed/pre-pool stores keep historical used addresses). Both core_pool and core_state variable-width blob readers gate on a cheap pre-read length check before materializing, bounding allocation on a corrupt/oversize column.
• Schema freeze guards: content-level SQL fingerprint (the built-in refinery fingerprint is content-blind) + retired-name grep guard.
• Migration safety: pre-migration auto-backup (verified byte-faithful and restorable); forward-version rejection (SchemaVersionUnsupported); interrupted-migration recovery proven against a committed populated-V001 fixture.

Deferred by decision (design preserved in the WS-B plan §7): manifest MAC/authentication — the trust boundary stays open and tracked as the headline follow-up.

How Has This Been Tested?

• Migration-execution suite against a populated V001 fixture: data preservation, backup restorability, deterministic re-migration, forward-version rejection, idempotent re-entry, interrupted-migration recovery (real rolled-back partial-DDL artifact, byte-equal reconvergence), empty-store path.
• Cross-wallet cascade/isolation tests for all new V002 tables; mixed-store union-reader test; two-wallet reader isolation.
• Cross-account-type collision regression: two distinct AccountTypes sharing (account_index, key_class) now persist as independent pool rows (covers both the identity/provider-key family and Standard/CoinJoin at index 0).
• Blob-size-gate regression tests for both load_used_addresses implementations (core_pool, core_state).
• Store-generation atomicity regression: restore leaves no stale -wal/-shm siblings and the rotated token is observable immediately after the swap.
• Full platform-wallet-storage suite: 60 test binaries, 0 failures; platform-wallet lib 216/216; platform-wallet-ffi suite green; fmt + clippy -D warnings clean across all three crates.
• Independent adversarial QA: two review cycles, all findings (4 MEDIUM in the first pass, 4 further findings — one CRITICAL PK-collision bug plus 3 MEDIUM — in the second) fixed and re-verified; final verdict PASS, 0 findings remaining.

Breaking Changes

None. V001 is byte-identical; V002 is additive; stores migrate forward automatically with a pre-migration backup. No FFI or SkipReason/CorruptKind shape changes.

Checklist:

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

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone

Attribution

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

[coderabbitai]

Important

Review skipped

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

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

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 29f8d691-bf0e-4255-8fec-d2986c5dbc65

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

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/3968-snapshot-redirect

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[thepastaclaw]

✅ Review complete (commit c0f17da)

[thepastaclaw]

Code Review

V002 migration is generally well-engineered, but multiple reviewers independently identified a blocking correctness bug: the new core_address_pool primary key drops the account_type discriminant that the sibling account_registrations table already carries, causing silent row overwrites between distinct AccountType variants that collapse to (account_index=0, key_class=0) (e.g. default Standard vs CoinJoin, and every degenerate identity/provider variant). Additional suggestions cluster around the restore_from generation-rotation window (four reviewers), non-deterministic account_index_for_script lookup, and an inconsistent blob-length gate in the new pool reader.

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

1 additional finding(s) omitted (not in diff).

🤖 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/migrations/V002__unified.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/migrations/V002__unified.rs:30-40: core_address_pool PK omits account_type — silent cross-account row collisions in the common case
  The `core_address_pool` PK is `(wallet_id, account_index, key_class, pool_type, address_index)`. Verified against `accounts.rs:273-303`:

  - `account_key_class()` returns the real key class only for `PlatformPayment` — every other variant maps to the sentinel `0`.
  - `account_index()` collapses at least nine distinct variants (`IdentityRegistration`, `IdentityTopUpNotBoundToIdentity`, `IdentityInvitation`, `AssetLockAddressTopUp`, `AssetLockShieldedAddressTopUp`, all four `Provider*Keys`) to `0`.

  So the following distinct accounts, all created concurrently in a normal wallet, upsert onto the same PK tuple in `core_address_pool`:

  - `Standard{index:0, BIP44}` and `CoinJoin{index:0}` collide on `(wallet_id, 0, 0, {0,1}, address_index)` (both derive External/Internal pools).
  - `IdentityRegistration`, `IdentityInvitation`, `ProviderVotingKeys`, `ProviderOwnerKeys`, `ProviderOperatorKeys`, etc. — all `AddressPoolType::Absent` at `address_index=0` — collide on `(wallet_id, 0, 0, 2, 0)`.

  `UPSERT_POOL_SQL` at `core_pool.rs:31-36` (`ON CONFLICT ... DO UPDATE SET script = excluded.script, used = MAX(used, excluded.used)`) silently overwrites the earlier account's `script` with the last one applied and merges `used` flags from unrelated addresses. This breaks both the "verbatim snapshot" guarantee this PR advertises and the address-reuse guard (a `used` bit can bleed onto an unrelated address once a colliding pool row's script wins the last-writer race).

  This is a direct regression relative to `account_registrations`, whose PK explicitly widens with `account_type` TEXT specifically to disambiguate these variants (see the `apply_registrations` insert at `accounts.rs:122-127` and the comment at 117-120: "distinct accounts that share `(account_type, account_index)` don't overwrite each other"). The pattern for correct disambiguation was already in the codebase but wasn't applied to the new table.

  Test gap: none of the new tests (`sqlite_core_pool_writer.rs`, `sqlite_pool_reader.rs`, `sqlite_v002_isolation.rs`, `sqlite_version_bump.rs`) exercise more than one account type sharing `(account_index, key_class)` in the same wallet — only `Standard` and `PlatformPayment` are used, and never together — so the collision is invisible to CI.

  Fix: widen the PK with an `account_type` discriminator (mirroring `account_registrations`), and add a regression test that flushes two different AccountType variants sharing `(0, 0)` (e.g. `IdentityRegistration` + `ProviderVotingKeys`, and `Standard{0}` + `CoinJoin{0}`) into the same wallet and asserts both survive with correct scripts and `used` flags.

In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/backup.rs:278-288: restore_from rotates the store-generation token AFTER the atomic swap — two coupled hazards
  Step 7 (`tmp.persist(dest_db_path)`) is the commit point: once it succeeds the destination inode holds the restored bytes. Steps 8–11 then run on a fresh RW connection outside the step-6 EXCLUSIVE lock, culminating in `regenerate_generation` at step 11. Two coupled hazards fall out of this ordering:

  1. **Semantics of `Err` after the swap.** Any failure in steps 8–11 (`fsync_parent_dir`, `apply_secure_permissions`, `open_conn`, or the `UPDATE meta_store_generation`) bubbles up as `Err(WalletStorageError::…)`. A caller that treats `Err` as "restore did not happen" (a reasonable read of a function that documents its swap as atomic) will act on a stale mental model — the destination has in fact been replaced with the source bytes, minus the token rotation.

  2. **Weakened cache-invalidation guarantee.** Between steps 7 and 11 the on-disk file carries the SOURCE's un-rotated generation token on top of the RESTORED bytes. A concurrent peer opening the DB in that window (contract violation per the module doc, but also a WAL/crash case where the UPDATE never fsyncs) sees restored content under a generation value its cache already has memoised — defeating the invalidation this token exists to guarantee (see `schema/versions.rs:227-239`).

  Both hazards close by folding the regeneration into the staged temp file before step 7, so the atomic rename is the single commit point that swaps in both restored bytes and a rotated token together. Alternatively, perform the rotation on the still-held EXCLUSIVE-lock connection before dropping it, and follow with `PRAGMA wal_checkpoint(FULL)` + parent-dir fsync so the token can't be lost to a crash between step 11 and the next fsync.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:82-88: account_index_for_script uses LIMIT 1 with no ORDER BY — non-deterministic on any script collision
  `SELECT account_index FROM core_address_pool WHERE wallet_id = ?1 AND script = ?2 LIMIT 1` has no `ORDER BY`, so if two rows ever share a script for one wallet (a subtly-introduced schema evolution, a wallet-import bug, or — most immediately — the PK-collision fix above landing a schema where the same script can appear under multiple `(account_index, key_class, ...)` tuples), SQLite is free to return either row. UTXO attribution then depends on incidental row-insertion or query-plan order and can flip across restarts.

  Make the tie-break explicit — e.g. `ORDER BY account_index, key_class, pool_type, address_index ASC LIMIT 1` — so the attribution is auditable and reproducible.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:107-123: load_used_addresses skips the pre-read length() gate used elsewhere in this PR
  Every other reader touching a variable-width BLOB in this crate (`core_state::load_state`, `core_state::load_used_addresses`, `accounts::load_state`) pre-reads `length(<col>)` and calls `blob::check_size` before materialising the `Vec<u8>`, so a tampered/corrupt DB with an oversize column can't force a large allocation before validation. The new `core_pool::load_used_addresses` reads `script` directly via `row.get::<_, Vec<u8>>(0)` with no such gate.

  The connection-wide `SQLITE_LIMIT_LENGTH` backstop (32 MiB) applies, but that's 2× looser than the crate's typed per-column cap (`BLOB_SIZE_LIMIT_BYTES`, 16 MiB) used everywhere else, and this query has no `LIMIT` and iterates `DISTINCT script` for every used address — so a corrupt store can force a larger aggregate allocation than the codebase's own stated per-column contract. Adding the same `length(script)` + `blob::check_size` gate keeps this reader consistent with the pattern the file's own doc already references (`load_used_addresses` doc comment cites the sibling in `core_state.rs`).

[thepastaclaw]

🔴 Blocking: core_address_pool PK omits account_type — silent cross-account row collisions in the common case

The core_address_pool PK is (wallet_id, account_index, key_class, pool_type, address_index). Verified against accounts.rs:273-303:

• account_key_class() returns the real key class only for PlatformPayment — every other variant maps to the sentinel 0.
• account_index() collapses at least nine distinct variants (IdentityRegistration, IdentityTopUpNotBoundToIdentity, IdentityInvitation, AssetLockAddressTopUp, AssetLockShieldedAddressTopUp, all four Provider*Keys) to 0.

So the following distinct accounts, all created concurrently in a normal wallet, upsert onto the same PK tuple in core_address_pool:

• Standard{index:0, BIP44} and CoinJoin{index:0} collide on (wallet_id, 0, 0, {0,1}, address_index) (both derive External/Internal pools).
• IdentityRegistration, IdentityInvitation, ProviderVotingKeys, ProviderOwnerKeys, ProviderOperatorKeys, etc. — all AddressPoolType::Absent at address_index=0 — collide on (wallet_id, 0, 0, 2, 0).

UPSERT_POOL_SQL at core_pool.rs:31-36 (ON CONFLICT ... DO UPDATE SET script = excluded.script, used = MAX(used, excluded.used)) silently overwrites the earlier account's script with the last one applied and merges used flags from unrelated addresses. This breaks both the "verbatim snapshot" guarantee this PR advertises and the address-reuse guard (a used bit can bleed onto an unrelated address once a colliding pool row's script wins the last-writer race).

This is a direct regression relative to account_registrations, whose PK explicitly widens with account_type TEXT specifically to disambiguate these variants (see the apply_registrations insert at accounts.rs:122-127 and the comment at 117-120: "distinct accounts that share (account_type, account_index) don't overwrite each other"). The pattern for correct disambiguation was already in the codebase but wasn't applied to the new table.

Test gap: none of the new tests (sqlite_core_pool_writer.rs, sqlite_pool_reader.rs, sqlite_v002_isolation.rs, sqlite_version_bump.rs) exercise more than one account type sharing (account_index, key_class) in the same wallet — only Standard and PlatformPayment are used, and never together — so the collision is invisible to CI.

Fix: widen the PK with an account_type discriminator (mirroring account_registrations), and add a regression test that flushes two different AccountType variants sharing (0, 0) (e.g. IdentityRegistration + ProviderVotingKeys, and Standard{0} + CoinJoin{0}) into the same wallet and asserts both survive with correct scripts and used flags.

_{source: ['claude']}

[Claudius-Maginificent]

Confirmed and agreed — blocking. Verified account_index/account_key_class collapse ≥9 AccountType variants to (0, 0), and UPSERT_POOL_SQL's ON CONFLICT genuinely overwrites script/merges used across unrelated accounts. Also checked: this does NOT recur elsewhere in the schema — account_registrations already disambiguates correctly via account_type in its PK; core_utxos/asset_locks/platform_addresses key on real UTXO/address identity with account_index as attribute-only. Isolated to this one new table. Needs a fix before merge — not yet fixed.

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

[thepastaclaw]

Correction: my automated reconciler posted an erroneous "resolved" reply here while processing the 0614fc4 review. This finding is still valid at 0614fc4 and was carried forward in the current review.

The misleading resolved text came from using the live PR diff after the branch had already advanced to a newer commit. Please ignore the earlier resolved wording in this reply thread.

[thepastaclaw]

🟡 Suggestion: restore_from rotates the store-generation token AFTER the atomic swap — two coupled hazards

Step 7 (tmp.persist(dest_db_path)) is the commit point: once it succeeds the destination inode holds the restored bytes. Steps 8–11 then run on a fresh RW connection outside the step-6 EXCLUSIVE lock, culminating in regenerate_generation at step 11. Two coupled hazards fall out of this ordering:

1. Semantics of Err after the swap. Any failure in steps 8–11 (fsync_parent_dir, apply_secure_permissions, open_conn, or the UPDATE meta_store_generation) bubbles up as Err(WalletStorageError::…). A caller that treats Err as "restore did not happen" (a reasonable read of a function that documents its swap as atomic) will act on a stale mental model — the destination has in fact been replaced with the source bytes, minus the token rotation.

2. Weakened cache-invalidation guarantee. Between steps 7 and 11 the on-disk file carries the SOURCE's un-rotated generation token on top of the RESTORED bytes. A concurrent peer opening the DB in that window (contract violation per the module doc, but also a WAL/crash case where the UPDATE never fsyncs) sees restored content under a generation value its cache already has memoised — defeating the invalidation this token exists to guarantee (see schema/versions.rs:227-239).

Both hazards close by folding the regeneration into the staged temp file before step 7, so the atomic rename is the single commit point that swaps in both restored bytes and a rotated token together. Alternatively, perform the rotation on the still-held EXCLUSIVE-lock connection before dropping it, and follow with PRAGMA wal_checkpoint(FULL) + parent-dir fsync so the token can't be lost to a crash between step 11 and the next fsync.

_{source: ['claude']}

[Claudius-Maginificent]

Confirmed — steps 8-11 (WAL cleanup, fsync, perms, generation regen) run after the step-7 atomic commit, outside the lock, so an Err there is ambiguous about whether the restore actually happened. Valid, not blocking. Not yet fixed.

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

[thepastaclaw]

Correction: my automated reconciler posted an erroneous "resolved" reply here while processing the 0614fc4 review. This finding is still valid at 0614fc4 and was carried forward in the current review.

The misleading resolved text came from using the live PR diff after the branch had already advanced to a newer commit. Please ignore the earlier resolved wording in this reply thread.

[thepastaclaw]

🟡 Suggestion: account_index_for_script uses LIMIT 1 with no ORDER BY — non-deterministic on any script collision

SELECT account_index FROM core_address_pool WHERE wallet_id = ?1 AND script = ?2 LIMIT 1 has no ORDER BY, so if two rows ever share a script for one wallet (a subtly-introduced schema evolution, a wallet-import bug, or — most immediately — the PK-collision fix above landing a schema where the same script can appear under multiple (account_index, key_class, ...) tuples), SQLite is free to return either row. UTXO attribution then depends on incidental row-insertion or query-plan order and can flip across restarts.

Make the tie-break explicit — e.g. ORDER BY account_index, key_class, pool_type, address_index ASC LIMIT 1 — so the attribution is auditable and reproducible.

Suggested change

	let idx: Option<i64> = tx
	.prepare_cached(
	"SELECT account_index FROM core_address_pool \
	WHERE wallet_id = ?1 AND script = ?2 LIMIT 1",
	)?
	.query_row(params![wallet_id.as_slice(), script], |row| row.get(0))
	.optional()?;
	pub fn account_index_for_script(
	tx: &Transaction<'_>,
	wallet_id: &WalletId,
	script: &[u8],
	) -> Result<Option<u32>, WalletStorageError> {
	let idx: Option<i64> = tx
	.prepare_cached(
	"SELECT account_index FROM core_address_pool \
	WHERE wallet_id = ?1 AND script = ?2 \
	ORDER BY account_index, key_class, pool_type, address_index ASC LIMIT 1",
	)?
	.query_row(params![wallet_id.as_slice(), script], |row| row.get(0))
	.optional()?;
	idx.map(|v| crate::sqlite::util::safe_cast::i64_to_u32("core_address_pool.account_index", v))
	.transpose()
	}

_{source: ['claude']}

[Claudius-Maginificent]

Confirmed real, and it gets more likely once finding #1 (PK missing account_type) is fixed — widening the PK increases the chance of two rows legitimately sharing a script across accounts (e.g. address reuse across account families). Fix in the same pass as #1. Not yet fixed.

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

[thepastaclaw]

Correction: my automated reconciler posted an erroneous "resolved" reply here while processing the 0614fc4 review. This finding is still valid at 0614fc4 and was carried forward in the current review.

The misleading resolved text came from using the live PR diff after the branch had already advanced to a newer commit. Please ignore the earlier resolved wording in this reply thread.

[thepastaclaw]

🟡 Suggestion: load_used_addresses skips the pre-read length() gate used elsewhere in this PR

Every other reader touching a variable-width BLOB in this crate (core_state::load_state, core_state::load_used_addresses, accounts::load_state) pre-reads length(<col>) and calls blob::check_size before materialising the Vec<u8>, so a tampered/corrupt DB with an oversize column can't force a large allocation before validation. The new core_pool::load_used_addresses reads script directly via row.get::<_, Vec<u8>>(0) with no such gate.

The connection-wide SQLITE_LIMIT_LENGTH backstop (32 MiB) applies, but that's 2× looser than the crate's typed per-column cap (BLOB_SIZE_LIMIT_BYTES, 16 MiB) used everywhere else, and this query has no LIMIT and iterates DISTINCT script for every used address — so a corrupt store can force a larger aggregate allocation than the codebase's own stated per-column contract. Adding the same length(script) + blob::check_size gate keeps this reader consistent with the pattern the file's own doc already references (load_used_addresses doc comment cites the sibling in core_state.rs).

Suggested change

	let mut stmt = conn.prepare(
	"SELECT DISTINCT script FROM core_address_pool \
	WHERE wallet_id = ?1 AND used = 1 ORDER BY script",
	)?;
	let rows = stmt.query_map(params![wallet_id.as_slice()], |row| {
	row.get::<_, Vec<u8>>(0)
	})?;
	let mut out = Vec::new();
	for r in rows {
	let script = dashcore::ScriptBuf::from_bytes(r?);
	let address = dashcore::Address::from_script(&script, network).map_err(|_| {
	WalletStorageError::blob_decode("core_address_pool.script not an address")
	})?;
	out.push(address);
	}
	Ok(out)
	}
	let mut stmt = conn.prepare(
	"SELECT DISTINCT length(script), script FROM core_address_pool \
	WHERE wallet_id = ?1 AND used = 1 ORDER BY script",
	)?;
	let mut rows = stmt.query(params![wallet_id.as_slice()])?;
	let mut out = Vec::new();
	while let Some(row) = rows.next()? {
	crate::sqlite::schema::blob::check_size(row.get::<_, i64>(0)?)?;
	let script_bytes: Vec<u8> = row.get(1)?;
	let script = dashcore::ScriptBuf::from_bytes(script_bytes);
	let address = dashcore::Address::from_script(&script, network).map_err(|_| {
	WalletStorageError::blob_decode("core_address_pool.script not an address")
	})?;
	out.push(address);
	}
	Ok(out)

_{source: ['claude']}

[Claudius-Maginificent]

Confirmed real — and one correction to the comment's own citation: core_state::load_used_addresses is NOT actually gated (verified directly — it does row.get::<_, Vec<u8>>(0) on core_utxos.script with no length()/check_size precheck), so it's not a safe example to mirror, it's a second live instance of the same gap. platform_addrs.rs's two functions and accounts::load_state genuinely are gated correctly. Recommend widening this fix to cover core_state::load_used_addresses too. Not yet fixed.

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

[thepastaclaw]

Correction: my automated reconciler posted an erroneous "resolved" reply here while processing the 0614fc4 review. This finding is still valid at 0614fc4 and was carried forward in the current review.

The misleading resolved text came from using the live PR diff after the branch had already advanced to a newer commit. Please ignore the earlier resolved wording in this reply thread.

[thepastaclaw]

Code Review

Delta a511bde..0614fc4 is a merge commit that pulls sibling FFI/Swift work into the branch; nothing under packages/rs-platform-wallet-storage/** changed, so all five prior findings carry forward as still valid at HEAD. The blocking issue is the core_address_pool primary key omitting an account_type discriminator: accounts::account_index collapses nine AccountType variants (IdentityRegistration, IdentityTopUpNotBoundToIdentity, IdentityInvitation, AssetLockAddressTopUp/ShieldedAddressTopUp, all four Provider*Keys) plus Standard{0}/CoinJoin{0} to (account_index=0, key_class=0), so their UPSERT rows collide silently and the last writer's script/used flag wins — feeding non-deterministic UTXO attribution through account_index_for_script and bleeding the used bit onto unrelated addresses. Four supporting suggestions/nitpicks around restore ordering, LIMIT-1-without-ORDER-BY, and missing blob length-gate remain unaddressed.

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

Source: reviewers: opus general/security-auditor/rust-quality/ffi-engineer ok; claude-sonnet-5 general/security-auditor/rust-quality/ffi-engineer ok; gpt-5.5[high] general/security-auditor/rust-quality/ffi-engineer failed; verifier: opus; specialists: security-auditor, rust-quality, ffi-engineer.

Reconciliation

• Carried-forward prior findings: all 5 prior findings from a511bdec are STILL VALID at 0614fc4c.
• New findings in latest delta: none. The a511bdec..0614fc4c delta is a merge of sibling FFI/Swift work and does not change the wallet-storage files that own these findings.
• Canonical review action: REQUEST_CHANGES (GitHub rendered this submitted review as COMMENT because duplicate inline findings were suppressed).

🤖 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/migrations/V002__unified.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/migrations/V002__unified.rs:30-40: core_address_pool PK omits account_type/DashPay-id — silent cross-account row collisions and used-bit bleed
  The `core_address_pool` PK at line 38 is `(wallet_id, account_index, key_class, pool_type, address_index)`. There is no `account_type` (nor DashPay identity) discriminator, unlike the sibling `account_registrations` table in V001__initial.rs:75-88, whose PK is `(wallet_id, account_type, account_index, key_class, user_identity_id, friend_identity_id)` precisely to disambiguate variants sharing `(account_index, key_class)`.
  
  Verified against src/sqlite/schema/accounts.rs:273-303: `account_index()` returns the sentinel `0` for nine distinct `AccountType` variants (`IdentityRegistration`, `IdentityTopUpNotBoundToIdentity`, `IdentityInvitation`, `AssetLockAddressTopUp`, `AssetLockShieldedAddressTopUp`, `ProviderVotingKeys`, `ProviderOwnerKeys`, `ProviderOperatorKeys`, `ProviderPlatformKeys`), and `account_key_class()` returns the sentinel `0` for every variant except `PlatformPayment`. Additionally, `Standard{index:0, BIP44}` and `CoinJoin{index:0}` both collapse to `(0, 0)`, and `DashpayReceivingFunds{index:0, (a,b)}` collides across differing DashPay identity pairs (that axis isn't in the PK at all).
  
  `UPSERT_POOL_SQL` at core_pool.rs:31-36 does `ON CONFLICT ... DO UPDATE SET script = excluded.script, used = MAX(used, excluded.used)` — the losing account's `script` is silently overwritten with the last-applied one, and the `used` bit is merged monotonically across unrelated addresses. Consequences at a normal multi-account wallet:
  
  1. Verbatim-snapshot invariant broken: `core_pool::load_used_addresses` (lines 107-123) surfaces a script the losing account never derived.
  2. Reuse-guard weakened: a used address whose row is overwritten drops out of the pool set; the `core_utxos` fallback UNION only rescues addresses whose UTXOs are still tracked, so used-then-fully-spent addresses on the losing side can be silently missing from the guard.
  3. UTXO attribution wrong: `core_state::resolve_account_index` calls `account_index_for_script` to bucket every incoming UTXO — funds from account A can be attributed to account B.
  
  None of the new tests (`sqlite_core_pool_writer.rs`, `sqlite_pool_reader.rs`, `sqlite_v002_isolation.rs`) exercise two AccountType variants sharing `(account_index, key_class)` in the same wallet — only distinct `Standard` indices and `PlatformPayment` appear, never together — so CI does not catch the collision.
  
  Fix: widen the `core_address_pool` PK with an `account_type` discriminator (and DashPay identity pair, matching `account_registrations`), and mirror the change in `UPSERT_POOL_SQL`, `account_index_for_script`, and readers. Add a regression test flushing two colliding variants (e.g. `Standard{0}` + `CoinJoin{0}`, and `IdentityRegistration` + `ProviderVotingKeys`) into the same wallet and asserting both rows survive with correct scripts, `used` flags, and attribution.

In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/backup.rs:246-289: restore_from rotates the store-generation token AFTER the atomic swap — stale generation on partial failure or crash
  `tmp.persist(dest_db_path)` at line 251 is the documented commit point — once it succeeds, the destination inode holds the restored bytes. Steps 8-11 (WAL/SHM cleanup, `fsync_parent_dir`, `apply_secure_permissions`, `open_conn` at 285 + `regenerate_generation` at 286) then run on a fresh RW connection outside the step-6 EXCLUSIVE lock. Two coupled hazards fall out of that ordering:
  
  1. **Ambiguous `Err` after the swap.** Any failure in steps 8-11 bubbles up as `Err(WalletStorageError::…)` after the destination has already been replaced. A caller reading the doc-comment as an atomic swap will act on a stale mental model — the destination is in fact restored, just missing the token rotation and durable sibling cleanup.
  2. **Weakened cache-invalidation guarantee.** Between step 7 (line 251) and step 11 (line 286), the on-disk file carries the SOURCE's un-rotated generation token on top of the RESTORED bytes. A crash between the two (or a concurrent peer opening the DB in that window) sees restored content under an un-rotated generation value a client cache may already have memoised, defeating the invalidation the token exists to enforce (see schema/versions.rs:227-239).
  
  Fold the regeneration into the staged temp file before step 7: open the staged temp RW, call `regenerate_generation` there, close, then `persist`. The atomic rename then becomes the single commit point that swaps in both the restored bytes and the rotated token together.
- [NITPICK] packages/rs-platform-wallet-storage/src/sqlite/backup.rs:175-197: restore_from skips the EXCLUSIVE lock when the destination doesn't yet exist
  When `dest_db_path.exists()` is false at line 175, `dest_lock_conn` stays `None` and no `BEGIN EXCLUSIVE` is taken. The module-level comment already documents `Callers must serialize restore intent across processes`, but a concurrent peer that creates `dest_db_path` between the existence check and step 7 `tmp.persist(dest_db_path)` (line 251) would have its file silently overwritten by the atomic rename — `persist`, not `persist_noclobber`.
  
  Note: the prior finding cited `persister.rs:175-197`; the canonical location is `backup::restore_from`. If defence-in-depth against a racing peer is intended on the fresh-destination path, switch to `persist_noclobber` and map `AlreadyExists` to a typed `RestoreDestinationRacedIntoExistence` error, mirroring the pattern `backup::run_to` already uses.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:82-88: account_index_for_script uses LIMIT 1 with no ORDER BY — non-deterministic UTXO attribution on any script collision
  `SELECT account_index FROM core_address_pool WHERE wallet_id = ?1 AND script = ?2 LIMIT 1` has no `ORDER BY`, so if two rows ever share a script for one wallet — realistically caused by the PK collision above, or by wallet-import / schema-evolution scenarios in the future — SQLite is free to return either row, and which one wins depends on incidental row-insertion or query-plan order. `core_state::resolve_account_index` feeds the result directly into every UTXO's persisted `account_index`, so balance-by-account reporting can flip across restarts for the same wallet state.
  
  Add an explicit tie-break so attribution is auditable and reproducible.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:102-123: load_used_addresses skips the pre-read length() gate used by every other BLOB reader in this crate
  Every other variable-width BLOB reader in this crate (`core_state::load_state`, `core_state::load_used_addresses`, `accounts::load_state`) pre-reads `length(<col>)` and calls `blob::check_size` (per-column cap `BLOB_SIZE_LIMIT_BYTES` = 16 MiB) before materialising the `Vec<u8>`, so a tampered/corrupt DB can't force a large allocation before validation. The new `core_pool::load_used_addresses` reads `script` directly via `row.get::<_, Vec<u8>>(0)` at line 112 with no such gate.
  
  The connection-wide `SQLITE_LIMIT_LENGTH` backstop (32 MiB) applies, but that's 2× looser than the crate's typed per-column cap, and this query has no `LIMIT` and iterates `DISTINCT script` for every used address — a corrupt store can force a larger aggregate allocation than the crate's own stated per-column contract. The reader's own doc-comment (line 101) already cites `core_state::load_used_addresses` as the pattern to follow, and that sibling gates. Adding the same gate keeps the invariant crate-wide.

[thepastaclaw]

Code Review

Cumulative review at c0f17da reconciling all 5 prior findings from 0614fc4. Prior-2, prior-3, and prior-4 are cleanly FIXED with matching regression tests (generation-rotate-before-swap under DELETE journal mode + sync_all, PK-ordered ORDER BY tie-break, and MAX(length(script)) blob-size gate). Prior-1 is PARTIALLY_FIXED — the account_type discriminator that closes the IdentityRegistration/ProviderVotingKeys and Standard{0}/CoinJoin{0} collisions landed with regression tests, but the DashPay (user_identity_id, friend_identity_id) axis that account_registrations discriminates on was not wired into core_address_pool. Verified in-tree: register_contact_account (contacts.rs:100) does not currently emit an AccountAddressPoolEntry to the persister, so DashPay pool rows are not being written to core_address_pool today, which reduces this from Sonnet's blocking to a schema-forethought suggestion on this additive-only migration. Prior-5 (EXCLUSIVE lock skipped when destination file does not exist) remains a caller-serialization nitpick, unchanged in this delta. No new blocking regressions from the c0f17da delta.

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

1 additional finding(s) omitted (not in diff).

🤖 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/migrations/V002__unified.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/migrations/V002__unified.rs:34-45: core_address_pool PK omits DashPay (user_identity_id, friend_identity_id) — schema forethought while the migration is still landing
  The widened PK (wallet_id, account_type, account_index, key_class, pool_type, address_index) correctly closes the account_type-variant collisions the prior review flagged, verified by distinct_account_types_sharing_index_zero_do_not_collide and standard_and_coinjoin_index_zero_do_not_collide in sqlite_core_pool_writer.rs.

  However, account_type_db_label collapses every DashpayReceivingFunds{index, user_identity_id, friend_identity_id} to the fixed string "dashpay_receiving" (accounts.rs:265) and account_index() returns just the inner index field (accounts.rs:288). send_contact_request (contact_requests.rs:162) hardcodes account_index = 0 for every DashPay contact, so two contacts on the same wallet would collapse onto the identical PK tuple (wallet_id, 'dashpay_receiving', 0, 0, pool_type, address_index) and silently overwrite each other via ON CONFLICT ... DO UPDATE SET script = excluded.script (core_pool.rs:36-38).

  Mitigation for THIS PR: verified in-tree that register_contact_account (contacts.rs:100-156) only inserts the managed account into in-memory info.core_wallet.accounts and never emits an AccountAddressPoolEntry to the persister — DashPay pool rows are not currently written to core_address_pool in production, so no live collision. The sibling account_registrations table (V001) does carry (user_identity_id, friend_identity_id) in its PK for exactly this reason, with a passing distinct_dashpay_friends_do_not_collide test, and account_dashpay_ids() (accounts.rs:309) already exposes the pair.

  Since V002 is additive-only, widening the PK later requires another migration. If DashPay pool persistence is on the near-term roadmap for this workstream, mirror account_registrations' PK now (add user_identity_id/friend_identity_id columns, thread account_dashpay_ids through apply_pools/UPSERT_POOL_SQL/account_index_for_script, add a regression test with two DashpayReceivingFunds entries sharing (account_type, account_index, key_class) but differing only by contact identity). Otherwise, add a brief note to the V002 migration doc-comment explicitly deferring the DashPay identity axis (mirroring the manifest-MAC deferral note) and track it as a follow-up.

Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general, failed_or_unparseable), opus (security-auditor), claude-sonnet-5 (security-auditor), gpt-5.5[high] (security-auditor, failed_or_unparseable), opus (rust-quality), claude-sonnet-5 (rust-quality), gpt-5.5[high] (rust-quality, failed_or_unparseable), opus (ffi-engineer), claude-sonnet-5 (ffi-engineer), gpt-5.5[high] (ffi-engineer, failed_or_unparseable); verifier: opus; specialists: security-auditor, rust-quality, ffi-engineer

[thepastaclaw]

🟡 Suggestion: core_address_pool PK omits DashPay (user_identity_id, friend_identity_id) — schema forethought while the migration is still landing

The widened PK (wallet_id, account_type, account_index, key_class, pool_type, address_index) correctly closes the account_type-variant collisions the prior review flagged, verified by distinct_account_types_sharing_index_zero_do_not_collide and standard_and_coinjoin_index_zero_do_not_collide in sqlite_core_pool_writer.rs.

However, account_type_db_label collapses every DashpayReceivingFunds{index, user_identity_id, friend_identity_id} to the fixed string "dashpay_receiving" (accounts.rs:265) and account_index() returns just the inner index field (accounts.rs:288). send_contact_request (contact_requests.rs:162) hardcodes account_index = 0 for every DashPay contact, so two contacts on the same wallet would collapse onto the identical PK tuple (wallet_id, 'dashpay_receiving', 0, 0, pool_type, address_index) and silently overwrite each other via ON CONFLICT ... DO UPDATE SET script = excluded.script (core_pool.rs:36-38).

Mitigation for THIS PR: verified in-tree that register_contact_account (contacts.rs:100-156) only inserts the managed account into in-memory info.core_wallet.accounts and never emits an AccountAddressPoolEntry to the persister — DashPay pool rows are not currently written to core_address_pool in production, so no live collision. The sibling account_registrations table (V001) does carry (user_identity_id, friend_identity_id) in its PK for exactly this reason, with a passing distinct_dashpay_friends_do_not_collide test, and account_dashpay_ids() (accounts.rs:309) already exposes the pair.

Since V002 is additive-only, widening the PK later requires another migration. If DashPay pool persistence is on the near-term roadmap for this workstream, mirror account_registrations' PK now (add user_identity_id/friend_identity_id columns, thread account_dashpay_ids through apply_pools/UPSERT_POOL_SQL/account_index_for_script, add a regression test with two DashpayReceivingFunds entries sharing (account_type, account_index, key_class) but differing only by contact identity). Otherwise, add a brief note to the V002 migration doc-comment explicitly deferring the DashPay identity axis (mirroring the manifest-MAC deferral note) and track it as a follow-up.

_{source: ['claude']}