[lklimek]

TL;DR

Reopening a SQLite-backed wallet restores its Core address reservations and used-key positions, so previously handed-out funding keys are not offered again.

User story

As a wallet user, I can restart the app with my saved Core balances, pending spends and address reservations intact, and receive an error when required saved data cannot be restored.

Scenario

Use funding addresses or reserve a new address, close the wallet, and reopen the same database. The wallet restores the used and reserved addresses before allocating another key. While a wallet is loading, a write from another connection is refused, so that wallet's state comes from one snapshot.

Detailed discussion

Dependencies and overlap

Stacked on #5207 (fix/sqlite-asset-lock-reconciliation), which is stacked on #5150. The history replay, pending-spend reservations, accounting repair and migrations of #5150 are reused unchanged; this PR is limited to SQLite address-pool restoration, the capability declaration, typed load errors and focused tests.

What was done?

• Restore persisted address-pool entries across managed Core account roles, including special funding keys and provider key pools. Preserve used state, reservation timestamps, derivation positions and reverse lookups.
• Preserve existing per-address accounting and reject conflicting typed keys, addresses or derivation indices.
• Restore the pools before history replay and before the stored coins mark their addresses used, so a funded address restored beyond the freshly derived window is not handed out again.
• Check every restored row against its derivation: each pool is derived from the account's public key, and a row whose key type, address, key or index contradicts the derived entry fails the wallet with a typed error. The derivation is budgeted by the pool's stored row count plus 10,000 uncovered indices, so a pool of any real depth restores in full and a damaged index cannot force unbounded work. A row the budget cannot reach is not restored and is counted as pool_row_beyond_derivation_budget; a stored reservation that no restored pool takes back is counted as unrestored_pool_reservation. Both are reported under Strict and Recovery and do not fail the wallet, except for one-time funding key accounts (next item). Hardened pools (platform-node keys) cannot be re-derived from public material and stay trusted as stored.
• Fail the wallet when a used or reserved row of a one-time funding key account (identity registration, top-up, unbound top-up, invitation, asset-lock address and shielded top-up) is not taken back by a restored pool: Strict fails the load, Recovery drops and names that wallet. Handing such a key out again would reuse a one-time funding key.
• Refuse a row filed under a pool its account does not have, instead of silently dropping the reservation it carries.
• Refill a restored pool's gap window. A pool whose stored rows already mark addresses used gets the window past them derived during load, as before pools were restored first. Without it the coin on such an address stayed in the balance, but the addresses past the saved rows were not derived, so they were unobserved and a funded one could later be handed out as fresh. Not verified: whether such a coin could be spent while its address was missing from the pool, and live SPV sync on that state.
• Count load incidents once per wallet. When startup replay repair rebuilds a wallet, the per-site degradation counts describe the wallet that is returned. Counters that predate this PR, such as unresolved_utxo_address, also stop doubling during startup repair.
• Log where a failure sits. A refused pool row logs its wallet, account type, pool type and index; under Strict the load logs the wallet id and error kind of any wallet it could not rebuild. Addresses and keys are not logged.
• Known limitation: for every other account, a reservation stored on a row that matches no restored account is reported as unrestored_pool_reservation, and a row beyond the derivation budget as pool_row_beyond_derivation_budget; neither is restored, so that address can be handed out again until a coin on it is seen.
• Known limitation: a pool row that was deleted outright cannot be detected: the row is the only record that a key was handed out.
• Known limitation: a wallet that rebuilds no account restores no pool and is not checked.
• Preserve typed storage errors through wallet reconstruction; under Recovery a dropped wallet is attributed to that error's kind instead of backend.
• Loading relies on the per-wallet BEGIN IMMEDIATE transaction introduced by fix(platform-wallet)!: restore Core spending state and repair persisted accounting #5150: a writer on another connection is refused while a wallet loads, so its records, spend claims, coins, pools and sync checkpoints come from one snapshot. The guarantee is per wallet, not per load: platform address state, the wallet id list, shielded viewing keys and the unimplemented-row count are read outside those transactions.
• Declare the existing WALLET_RESTORE capability after completing its Core-snapshot restore path. Capability meanings and bit values remain unchanged. PROVIDER_TRANSACTIONS is not newly declared.
• Wallet drain remains unsupported: conflict cleanup can remove operation evidence, so SQLite withholds ASSET_LOCK_OPERATION_IDS rather than treating an absent row as proof that a drain was never sent. Public status and drain entry points reject the operation before changing wallet state. Core restoration and asset-lock status reconciliation remain available.
• Keep the existing watch-only restore model; no secret import, new schema migration or automatic transaction resend is introduced.

WALLET_RESTORE is the existing persisted Core wallet snapshot contract. Separate token caches, DashPay profile overlays, deferred contact queues and invitation UI records are not made complete by this flag; their readback remains separate. DET's #1036 pin remains on the narrower #5207. Declaring it lets SQLite hosts pass preflights that require it, such as funding a shielded pool from an asset lock.

Out of scope

• A healthy wallet that has burned one-time funding keys reports orphaned_utxo_owner and unresolved_utxo_address on every load: the used-address projection from fix(platform-wallet)!: restore Core spending state and repair persisted accounting #5150 routes addresses of key-only accounts to the first funds account. The keys themselves are restored correctly; only the report is noisy.
• validate_account_type still admits standard for pool rows although no writer produces it; it belongs to the reader introduced with feat(platform-wallet-storage): embeddable SQLite persistence backend with seedless rehydration #3968.
• The wallet-wide audit of one-time funding key rows reads the two identity columns without the length() gate the other readers use. A mis-sized row is still refused; the cost is one transient copy bounded by the connection's 32 MiB value limit.
• ONE_TIME_FUNDING_LABELS repeats the account filter of the asset-lock writer in platform-wallet; a test fails when the two differ. A typed predicate exported from platform-wallet would replace both.

How Has This Been Tested?

• The new public-API drain regression failed before the capability correction and passes afterward; it verifies the explicit missing-capability error, no SQLite changes and no tracked asset lock.
• The full platform-wallet-storage suite passes with all features and the locked dependency graph: 1,083 tests passed, 2 preexisting ignored. Clippy with --all-targets --all-features --locked -- --no-deps -D warnings is clean for platform-wallet-storage and platform-wallet; formatting check passes.
• sqlite_wallet_restore.rs covers reopen and real manager hydration, special funding-key non-reuse, reservation timestamps, exact pending-spend and per-address accounting, malformed-key rejection before partial manager load, a typed platform-node pool failure under Strict and Recovery, a writer refused as busy while a wallet's load transaction is open, and ProReg/ProUpReg key-account history and positions. It also covers a funded address beyond the derived window, a foreign script stored beyond that window, a row filed under a missing pool, two DashPay contact pools sharing an index, and each pool-conflict guard under Strict and Recovery. It also covers the pools of a standard account registered under the legacy label, and a used one-time funding key row that no restored pool takes back. Unit tests pin that restoring pools twice leaves per-address accounting unchanged and that a row the derivation budget cannot reach is skipped and counted, with nothing derived past the budget.
• The busy-writer, typed-failure and pool-guard tests were each shown to fail when the behaviour they guard is removed.
• Not run: platform-wallet tests (its only change is two doc comments), the full workspace, live-network or user-funds testing. No Swift file differs from the base.

Breaking Changes

Rust source only: platform_wallet_storage::LoadSite gains PoolRowBeyondDerivationBudget and UnrestoredPoolReservation and becomes #[non_exhaustive]; code matching it exhaustively needs a wildcard arm. No persisted encoding, FFI layout or capability bit changes.

Checklist

• Self-review performed
• Existing PRs checked and overlapping work reused
• Reopen and real manager-hydration regression coverage
• Validation limitations documented

_{🤖 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

• Source mode: Auto
• Repository: dashpay/platform
• Branch: fix/sqlite-asset-lock-reconciliation
• File: .coderabbit.yaml
• Commit: 5a2204c
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 4940d315-05ae-44b5-99e9-3d0b30fb36a0

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

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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]

⛔ Final review complete — 4 blocking finding(s) (commit beadf9b) · triage: critical

[thepastaclaw]

Final validation — Phase 1 + Phase 2

Static verification at the exact head confirms three pool-restoration correctness gaps, plus targeted coverage and startup-cost improvements. These client-side issues do not violate consensus invariants and are classified as suggestions under the supplied severity policy. No builds or tests were run; the supplied CI snapshot shows successful completed checks but no dedicated Rust wallet-storage test result.

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

Review provenance

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

• Triage: critical by gpt-6.1-sol (effort low) — The intricate restore_pools implementation in packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs changes key handling by reconstructing typed key pools, derivation indices, used-key positions and funding-address reservations.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 14% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

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

In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:1881-1882: Apply the UTXO-derived reuse guard after restoring persisted pools
  The overlay runs after `apply_persisted_core_state` has already applied the used-address set to the reconstructed pools. A valid Standard external-pool snapshot with indices 0–39 Reserved and index 40 Available, followed by a recordless UTXO on index 40, exposes the ordering problem: initial derivation covers 0–29, and discovery stops at index 30 because no used address anchors a deeper horizon. The overlay then inserts index 40 as Available because there is no existing entry whose Used state it can preserve. Recordless UTXOs are supported by the writer, and history replay returns immediately when there are no records, so the next allocation can return the funded address. Restore pools before applying the reuse guard, or reapply the known-used address set after the overlay. Add a reopen regression with reservations beyond the initial gap and a recordless UTXO on a saved available address.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:605-615: Validate derivation for entries outside the already-generated pool
  Address/key consistency is checked only when the reconstructed pool already contains the persisted index. Special funding-key pools initially derive just their gap window and are not extended by the funds-account projection, so deeper persisted entries bypass this check. A corrupted row carrying another child's valid P2PKH script and correctly sized ECDSA key is accepted and assigned the recorded index's derivation path. The pinned key-wallet allocation methods return cached Available entries without deriving them again, while signing derives from the recorded path; the wallet can therefore return an address its corresponding signing key does not control. Pass the registered account key material into restoration and independently derive and compare every publicly derivable persisted index before insertion, including indices absent from the skeleton. Preserve explicit handling for hardened entries that cannot be regenerated from public material, and test valid-width beyond-window mismatches under Strict and Recovery.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:503-505: Reject persisted rows whose pool discriminator cannot be restored
  Selecting rows separately for each reconstructed pool means rows with an incompatible discriminator are never validated. For example, changing a reserved `identity_invitation` row from Absent (2) to External (0) passes the database CHECK constraint, but this account exposes only an Absent pool. The overlay skips the row, and the earlier used-address reader also skips it because `used = 0`. Strict load succeeds with the skeleton entry Available, silently losing the reservation and allowing the same funding key to be allocated again. Validate that saved rows for registered accounts map to an actual managed pool and return a typed error for incompatible discriminators. Add a regression proving that an impossible pool discriminator cannot discard a reservation during a successful Strict load.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:660-663: Compute the highest used index once per restored pool
  The pinned dependency stores `used_indices` in a HashSet, so `iter().max()` scans the accumulated set on every restored row, including unused rows. A pool with N used entries therefore incurs quadratic work during restoration while the connection mutex and per-wallet IMMEDIATE transaction remain held. No used index is removed here, so maintain the maximum incrementally or compute it once after the row loop.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:605-625: Exercise the new pool-conflict guards under Strict and Recovery
  The new account/key-type mismatch, derived address-or-key conflict and duplicate-address-across-indices errors have no direct regression coverage. The malformed-key test exercises a width check, and the provider wrong-curve test fails earlier in `restore_provider_platform_node_pool`, so neither verifies these overlay guards or their policy handling. Add fixtures that reach each guard: assert Strict preserves the typed storage error, and Recovery isolates the affected wallet with the correct degradation kind while retaining a healthy sibling wallet.
- [NITPICK] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:500: Reuse the identical restore statement across pools
  Every account/pool iteration prepares the same SELECT text with different bound parameters. Using `prepare_cached`, as the repeated-shape writer and ownership query already do, avoids repeated SQL preparation during wallet loading. This is a small startup optimization, separate from the per-row quadratic scan.

In `packages/rs-platform-wallet-storage/tests/sqlite_wallet_restore.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/tests/sqlite_wallet_restore.rs:160-179: Add DashPay contact-pool restore coverage
  The reopen test creates no DashPay accounts, so the new restore query's user/friend identity bindings are exercised only with zero sentinels. Existing contact tests cover writer separation and registration migration, not restoration of used and reserved pool entries for multiple contacts. Two contacts can share the same account label, numeric index and key class, making the identity pair essential to selecting the correct saved pool. Add a reopen test with two contacts at the same numeric index and distinct non-zero identity pairs, then assert their used states, reservation timestamps and addresses restore independently without cross-contact overwrite.

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

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

• Validate restored HD entries through the key-wallet owner — The confirmed beyond-window validation defect is merged into 'Validate derivation for entries outside the already-generated pool'. Requiring an upstream restoration API and dependency-pin update is not established as necessary; the essential requirement is independent validation using the account key material.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.

[thepastaclaw]

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

Verified the complete diff at 921dbb8: nine prior findings are fixed, while reservation loss for type-impossible account coordinates remains valid. Five in-scope suggestions remain, including a newly confirmed address-reuse regression; none violates the consensus-critical invariants that the supplied severity policy classifies as blocking. Scoped wallet CI passed at this exact head; this verification was static, with no local builds or tests run.

🟡 5 suggestion(s)

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

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

• Triage: critical by gpt-6.1-sol (effort low) — The intricate restore_pools and derive_towards changes in packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs directly alter key derivation, address validation, and used-key and reservation restoration that prevent funding-key reuse.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 14% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

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

In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:1874-1875: Run the initial gap refill even when restored entries are already Used
  The restore-before-projection ordering fixes saved beyond-window addresses, but it also changes the existing mark/refill loop's starting state. Consider a Standard external pool saved through index 29 with index 20 Used, plus a recordless UTXO at index 45 that has no saved pool row. After this overlay, discovery excludes index 20 because it is already present and stops at index 30 without resolving index 45. In rehydrate.rs:1298-1306, mark_used(20) returns false because the entry is already Used, and mark_used(45) returns false because it is absent, so the loop exits without refilling. The exact pinned key-wallet implementation confirms both return conditions. The base loader instead marked index 20, refilled through index 50, then marked index 45 on the next iteration. The new path can eventually derive and reserve funded index 45 as Available, and recordless history cannot repair it. Preserve the new ordering, but execute an initial bounded refill using the restored highest_used even when no state flips, then continue marking newly derived entries to a fixed point. Add a reopen regression combining saved Used entries with a recordless UTXO outside the saved pool; the existing direct wedge-zone test starts without the overlay.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:859-867: Count unrestored pool rows once across replay repair
  These counts are added immediately to the shared LoadCtx, but load_one_wallet calls load_wallet_snapshot twice when startup history replay produces repair sweeps (persister.rs:1693-1698). Both reconstructions run restore_pools with the same context, and apply_replay_sweeps does not change core_address_pool. One unmatched reservation is consequently counted twice under UnrestoredPoolReservation; an unreachable pool row is likewise counted twice under PoolRowBeyondDerivationBudget. This contradicts LoadDegradation's documented per-row/per-entry semantics under both policies. Accumulate these diagnostics once per load or deduplicate them by persisted row coordinates across repair passes. Add coverage combining an unmatched reservation with history that triggers startup repair.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:775-777: Retain row coordinates when reporting typed pool failures
  The new conflict errors preserve their type but contain only a static reason. restore_pools propagates them directly, and the Strict wallet-isolation boundary returns the original error without logging its wallet or row coordinates. A failed load therefore identifies neither the affected wallet nor the account, pool and index that need repair, although all are available at this guard. Retain the typed source and classification while adding structured coordinates to a diagnostic before propagation or to a contextual typed error. Apply the same treatment to the account/key-type and duplicate-address guards.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:847-851: Reject pool rows with impossible account discriminators
  (existing thread: https://github.com/dashpay/platform/pull/5210#discussion_r4248454211)
  The compatibility policy correctly preserves representable legacy or orphaned accounts, but it also accepts coordinates that those cases cannot produce. accounts::account_key_class always returns zero for IdentityInvitation, apply_pools uses that mapping, and V008's legacy pool conversion delegates to the canonical writer. An invitation reservation with key_class = 1 is therefore distinguishable from a legacy standard label or an absent contact account. The new should_report_a_reservation_that_no_restored_pool_takes_back regression confirms that both policies load the wallet with this previously reserved address Available; reporting the loss does not prevent allocation again. Add account-type coordinate validation before account-filtered restoration queries hide malformed rows. Reject only type-impossible sentinel combinations with a typed error, preserving report-only handling for representable unmatched coordinates. Strict should reject the malformed wallet and Recovery should isolate it.

In `packages/rs-platform-wallet-storage/src/sqlite/load_ctx.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/load_ctx.rs:77-81: Document the Rust source break from extending LoadSite
  LoadSite is publicly re-exported, and the v5.0.0-beta.2 tag contains the previous exhaustive enum without #[non_exhaustive]. Adding these variants breaks downstream exhaustive matches even though persisted encodings and FFI layouts are unchanged. The PR's blanket Breaking Changes: None statement and the current Unreleased changelog omit this Rust API change. Document the two new cases and the required downstream match updates in the compatibility discussion and release notes. This belongs to the normal crate/workspace release boundary, not PlatformVersion or PERSISTENCE_CAPABILITIES_VERSION; appending the variants or adding #[non_exhaustive] now would still break existing exhaustive matches.

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

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

• Isolate exported invitation private keys from their persisted parent xpub — The existing invitation flow exports a non-hardened funding child's private key, while account registration persists its parent extended public key and chain code. An invitation recipient who also obtains a readable database or backup can recover the invitation account private key using the BIP32 non-hardened derivation relation, then derive sibling voucher keys. The export, derivation and parent-xpub persistence mechanisms predate this diff; the SQLite restoration changes do not introduce or worsen them.
  • Follow-up: Track a separate security follow-up for hardened or independently generated voucher keys, including compatibility handling for existing invitations and a regression demonstrating sibling-key isolation.
• Centralize pool-entry integrity below persistence adapters — Out of scope: the mobile adapter's unchecked insertion path predates this diff, which changes only SQLite restoration. Moving restoration into an upstream key-wallet API and updating both adapters is a broader ownership redesign, not a prerequisite for the guarded SQLite path to work.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.

[thepastaclaw]

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

Static review of the exact head confirms that Core pool restoration now validates derived entries, restores provider pools, preserves usage state, and runs before UTXO reconciliation. Two in-scope issues remain: omitted reserved rows can be reused for one-time funding pools, and legacy standard-labelled pool rows are not selected during restoration.

🔴 1 blocking | 🟡 1 suggestion(s)

Review provenance

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

• Triage: critical by gpt-6.1-sol (effort low) — The substantial new restore_pools and derive_towards logic in packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs directly changes key derivation validation and restoration of used and reserved funding addresses, meeting both the intricacy and key-handling criteria.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 14% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

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

In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:899-922: Do not successfully restore wallets when one-time funding pool entries are omitted
  `restore_pools` treats every reservation not taken back by a reconstructed pool as a non-fatal degradation. Rows beyond the derivation budget are skipped at lines 799–803, while rows whose account coordinates match no reconstructed account are never queried; both cases are included in `stored_reservations` but not `restored_reservations`, leaving the corresponding address absent or Available. For an `IdentityInvitation` or other exported one-time funding pool, the previously exported bearer key can therefore be selected again after reopen. Because `WALLET_RESTORE` is advertised and the PR's purpose is to prevent funding-key reuse, Strict must fail closed for these pools or Recovery must quarantine the affected indices instead of returning a successfully restorable wallet.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:637-658: Restore persisted pools stored under the legacy standard account label
  The restoration queries bind only `accounts::account_type_db_label(&account_type)` (for example, `standard_bip44` or `standard_bip32`). V008 deliberately admits the historical `standard` label as compatible with either Standard variant, but a row stored under that label is never selected by these queries. Its reservation is later reported as `UnrestoredPoolReservation` and the reconstructed address can become available again. Resolve the legacy alias using unambiguous account-manifest information, or withhold `WALLET_RESTORE` for wallets containing rows that cannot be safely assigned; otherwise the advertised restore capability is not compatible with valid historical SQLite stores.

[thepastaclaw]

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

Reviewed the complete diff at ce03450 and found no remaining blocking defect; two non-blocking suggestions remain around coordinate-blob validation and shared funding-role classification. All 16 prior findings were independently revalidated: 14 are fixed, one is withdrawn, and one is intentionally deferred outside the corrected one-time funding-key case. Validation was static only; the supplied CI snapshot still has Rust workspace tests and Kotlin SDK checks queued, with the dedicated Rust wallet test job skipped.

🟡 2 suggestion(s)

Review provenance

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

• Triage: critical by gpt-6.1-sol (effort low) — The substantial restoration logic in packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs directly changes key handling by validating derived keys and restoring used positions and reservations that prevent one-time funding key reuse.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 14% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

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

In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:973-979: Check identity-blob widths before copying unmatched funding rows
  The new wallet-wide audit reads both identity columns directly into Vec<u8>, bypassing the pre-allocation checks used by load_used_addresses_with_ctx and owning_account_for_script. A reserved, unused funding row with malformed identity coordinates is excluded by the used-address reader and cannot match the per-account restoration queries, so this audit is its first validation boundary. V009 imposes no width constraint on these columns: the audit can therefore copy multi-megabyte blobs before rejecting coordinates that must contain exactly 32 bytes. The connection's 32 MiB ceiling bounds the exposure, so this is hardening rather than an unbounded or unauthenticated remote vulnerability. Select length(user_identity_id) and length(friend_identity_id), validate both with blob::check_fixed_width(..., 32, ...), and only then retrieve the vectors. Add reserved-row coverage for malformed and oversized identities, checking the typed Strict failure and Recovery isolation with a healthy sibling wallet.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:556-572: Share the one-time funding-role definition with the asset-lock writer
  ONE_TIME_FUNDING_LABELS independently reproduces the funding-account classification in wallet/asset_lock/build.rs::persist_asset_lock_account_pools. The sets currently agree, but their connection is enforced by a test that reads and parses the private writer's source syntax. This makes maintenance of the fail-closed restoration rule depend on another crate's implementation text rather than a typed wallet-domain contract. Export a shared AccountType predicate or equivalent classification API from platform-wallet's asset_lock module and consume it in both paths. Keep database labels, coordinate validation and Strict/Recovery handling in SQLite; translating the shared roles through account_type_db_label removes the parallel semantic lists without moving backend policy into the wallet layer.

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

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

• Track one-time key issuance independently of individual pool rows — The fixture should_hand_out_again_a_one_time_funding_key_whose_row_was_deleted asserts the documented residual: deleting an issued invitation key's row leaves the loader unable to distinguish that key from an unused one. This is a concrete funding-key integrity concern, but detecting missing issuance history requires additional durable metadata and a recovery policy beyond this PR's restoration of present rows.
  • Follow-up: Track a separate issuance-integrity task covering missing pool rows and explicitly distinguishing local issuance-watermark protection from stale-backup rollback protection.

[thepastaclaw]

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

Verified the combined reviewer findings and all 18 prior identities against head 52a15fa. Most restoration fixes are present, but the empty-account early return still bypasses the one-time funding-key audit; one blocking finding and five non-blocking suggestions remain. This was a static review with no builds or tests run; current CI still has Rust wallet runner selection pending and Rust workspace tests skipped.

🔴 1 blocking | 🟡 5 suggestion(s)

5 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

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

• Triage: critical by gpt-6.1-sol (effort low) — This is a large, intricate change to SQLite wallet persistence and restoration that directly affects key handling and one-time funding-address reuse, making it a critical security and funds-safety surface.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (lane failed), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

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

In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:927-943: Track pool reconstruction in its key-wallet owner
  This adapter directly maintains AddressPool entries, reverse indexes, used_indices and both high-water marks, while platform-wallet-ffi has a separate restore_address_pool implementation maintaining the same invariants. At aca020263990, which is both the pinned and supplied upstream default revision, key-wallet exposes derivation and allocation but no invariant-preserving entry-import API. The underlying restoration contract belongs in dashpay/rust-dashcore/key-wallet, leaving SQLite responsible for decoding and recovery policy. The existing upstream snapshot proposal already covers operational address-pool restoration and rebuilding its reverse indexes: https://github.com/dashpay/rust-dashcore/issues/1196. Link that ownership follow-up from this PR and explicitly associate this temporary reconstruction with it; a new broad redesign or an FFI rewrite is not required here.
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:637-640: Do not successfully restore wallets when one-time funding pool entries are omitted
  (existing thread: https://github.com/dashpay/platform/pull/5210#discussion_r4250717513)
  The six-role audit fixes unmatched funding rows only when at least one account was reconstructed. If corruption removes the account registrations while retaining valid used or reserved funding-pool rows, this early return skips both the audit and reservation diagnostics. load_wallet_snapshot explicitly constructs an empty external-signable wallet for this manifest, and apply_persisted_core_state succeeds when there are no unspent coins, so Strict and Recovery can return it without reporting the lost funding-key state. Having no pool now does not make the retained rows harmless: ensure_identity_topup_account can later recreate an account through the external signer and initialize a fresh pool without importing those rows, resetting allocation to previously consumed indices. Unlike an outright deleted pool row, the contradictory evidence still exists and can be checked. Run the wallet-wide one-time-row audit even with zero reconstructed accounts, while continuing to allow genuinely empty platform-only wallets. Add Strict failure and Recovery isolation coverage for an empty manifest with retained used and reserved funding rows.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:556-572: Share the one-time funding-role definition with the asset-lock writer
  (existing thread: https://github.com/dashpay/platform/pull/5210#discussion_r4257529154)
  ONE_TIME_FUNDING_LABELS independently duplicates the six-role filter in platform-wallet's asset_lock_account_pool_entries. The current sets agree, but their connection is a unit test that opens the writer's implementation file and parses its function name, matches! expression and flat_map boundary. That makes an otherwise semantic-preserving writer refactor part of the storage restoration contract, rather than expressing the classification through the existing Rust dependency. Export a typed funding-role predicate or equivalent classification from platform-wallet and consume it in both paths; retain SQL labels, coordinate validation and Strict/Recovery policy here. Adding public API in the owning crate is the appropriate boundary, not unrelated cleanup. If handled separately, link a tracking issue or PR; the current Out of scope entry supplies no tracked destination.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:973-979: Check identity-blob widths before copying unmatched funding rows
  (existing thread: https://github.com/dashpay/platform/pull/5210#discussion_r4257529142)
  This audit materializes user_identity_id and friend_identity_id as Vec<u8> before checking whether their coordinates can match a restored pool. An unused reserved funding row with malformed identity coordinates is excluded by the used-address reader and cannot match an account-specific restore query, so this is its first validation boundary. The 32 MiB SQLite connection limit bounds the allocation, and the wallet is subsequently refused, so this is corruption hardening rather than an unbounded or remote vulnerability. It nevertheless permits a needless multi-megabyte copy while holding the wallet transaction, unlike owning_account_for_script and load_used_addresses_with_ctx, which validate length(...) before retrieving the vectors. Select both lengths and call blob::check_fixed_width(..., 32, ...) first, with malformed reserved-row Strict/Recovery coverage. A separate cleanup needs a tracking reference rather than only the untracked deferral in the description.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:992-1005: Reject pool rows with impossible account discriminators
  (existing thread: https://github.com/dashpay/platform/pull/5210#discussion_r4248454211)
  The funding-key audit closes the original IdentityInvitation example, but impossible coordinates on other roles still discard saved reservations during a successful Strict restore. The new should_report_a_reservation_that_no_restored_pool_takes_back regression demonstrates this directly: changing provider_owner.key_class to 1 leaves its reserved entry Available after loading. The corrected historical explanation identifies no valid producer for that coordinate, so it can be distinguished from a supported historical store rather than accepted solely because reuse is considered non-funds-sensitive. Keep this non-blocking, but validate impossible account coordinates against the reconstructed manifest and reject them under Strict; Recovery can retain an explicitly documented degradation policy. Reporting the missing reservation does not preserve it, and the current deferral has no linked follow-up.

In `packages/rs-platform-wallet-storage/src/sqlite/load_ctx.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/load_ctx.rs:44-50: Document the Rust source break from extending LoadSite
  (existing thread: https://github.com/dashpay/platform/pull/5210#discussion_r4249556250)
  The PR title, Breaking Changes section and non_exhaustive annotation now disclose the Rust source break correctly. The release-note part of the earlier fix is missing at this head, however: commit 895bff9fca contains an Unreleased LoadSite migration entry, but the current CHANGELOG.md contains neither new variant name nor the wildcard-arm guidance and is unchanged from the review base. Restore the note naming PoolRowBeyondDerivationBudget, UnrestoredPoolReservation and the required downstream wildcard arm so consumers receive the migration guidance in shipped documentation. No schema, capability-bit or PlatformVersion change is required.

[thepastaclaw]

🟡 Suggestion: Track pool reconstruction in its key-wallet owner

This adapter directly maintains AddressPool entries, reverse indexes, used_indices and both high-water marks, while platform-wallet-ffi has a separate restore_address_pool implementation maintaining the same invariants. At aca020263990, which is both the pinned and supplied upstream default revision, key-wallet exposes derivation and allocation but no invariant-preserving entry-import API. The underlying restoration contract belongs in dashpay/rust-dashcore/key-wallet, leaving SQLite responsible for decoding and recovery policy. The existing upstream snapshot proposal already covers operational address-pool restoration and rebuilding its reverse indexes: dashpay/rust-dashcore#1196. Link that ownership follow-up from this PR and explicitly associate this temporary reconstruction with it; a new broad redesign or an FFI rewrite is not required here.

_{source: gpt-6.1-sol (phase2-reviewer: architecture-layering)}

[thepastaclaw]

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

The current head still has two blocking restore regressions: key-only funding pools are restored with used entries but are never gap-refilled, and stale DashPay contact rows can fail SQLite restoration before the manager’s contact-rebuild path runs. The change also retains three non-blocking documentation/ownership findings and exposes a sparse-pool compatibility gap; current CI reports the Rust workspace, Kotlin, and Swift checks passing, with wallet-specific Rust tests skipped.

🔴 2 blocking | 🟡 5 suggestion(s)

4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

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

• Triage: critical by gpt-6.1-sol (effort low) — The extensive restoration logic in packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs directly changes key handling by reconstructing derivation positions, used keys and reservations whose correctness prevents one-time funding-key reuse.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

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

In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:930-944: Refill re-derived key-only pools after restoring used rows
  The overlay restores `highest_used` and the used-index set for every managed pool, but the subsequent refill logic lives in `extend_pools_for_restored_addresses`, which is reached only through the funds-account path in `rehydrate.rs`. `all_funding_accounts_mut()` excludes identity-registration, top-up, invitation, asset-lock, and provider key-only accounts. When a key-only pool has consumed its initial generated window, restoration therefore leaves `highest_generated` at the last stored index; its later `next_unused_with_info(&KeySource::NoKeySource, ...)` call cannot derive the next key and returns `NoKeySource`. This breaks the primary one-time funding-key paths after restart. Refill every publicly re-derivable restored key-only pool, and add a reopen regression that consumes the initial window before requesting another key.
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:874-887: Do not fail Strict loads on stale DashPay pool rows after contact rotation
  The restored row is rejected whenever its address or public key differs from the current account derivation. That is correct for fixed accounts, but DashPay external accounts can legitimately retain a pool created from an older contact xpub after rotation: `apply_pools` upserts current rows without deleting higher old indices. On the next load, `restore_pools` derives those stale rows from the new xpub and returns this error before the platform-wallet manager can run `distrust_markers_of_foreign_external_pools` and rebuild the contact account. A normal contact rotation can therefore make Strict loading fail, or cause Recovery to drop the wallet. Preserve/defer stale DashPay external rows to the existing marker/rebuild path while retaining strict derivation validation for non-rotatable account types.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:549-551: Validate saved indices through upstream per-index derivation
  `derive_towards` sequentially derives every index up to the highest stored row and skips rows beyond the row-count-plus-allowance budget. The upstream `AddressPool::prune_unused` intentionally retains used and reserved entries while deleting surrounding available entries, so a valid pool can be sparse—for example, a reserved entry at a high index with only the initial window retained below it. Such a snapshot can pass `apply_pools` but lose the high reservation on reopen, allowing a non-one-time address to be handed out again. Validate each stored row with the owning key-wallet account’s per-index derivation APIs, or otherwise distinguish valid pruned reservations from damaged sparse rows before applying the bounded restoration policy.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:556-572: Share the one-time funding-role definition with the asset-lock writer
  (existing thread: https://github.com/dashpay/platform/pull/5210#discussion_r4257529154)
  `ONE_TIME_FUNDING_LABELS` duplicates the semantic account filter in `platform-wallet`'s asset-lock writer. The source-parsing test detects some present-day drift, but it couples this storage policy to another crate’s private source syntax rather than sharing the classification itself. Because this list decides whether an unrestored row is fatal, export a typed predicate or role iterator from the asset-lock domain and use it in both paths; SQLite should continue owning label conversion, row auditing, and Strict/Recovery policy.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:973-979: Check identity-blob widths before copying unmatched funding rows
  (existing thread: https://github.com/dashpay/platform/pull/5210#discussion_r4257529142)
  The wallet-wide one-time-row audit materializes both identity columns as `Vec<u8>` before checking that they are exactly 32 bytes. A malformed reserved row can reach this audit without being examined by the used-address reader or an account-scoped pool query, so the copy is the first validation boundary. The connection limit bounds the allocation and the wallet is ultimately rejected, making this corruption hardening rather than an attacker-controlled vulnerability, but the reader should follow the adjacent `length()` plus `blob::check_fixed_width` pattern before retrieving the blobs.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:927-943: Track pool reconstruction in its key-wallet owner
  (existing thread: https://github.com/dashpay/platform/pull/5210#discussion_r4341177862)
  This adapter directly maintains AddressPool entries, reverse indexes, used_indices and both high-water marks, while platform-wallet-ffi has a separate restore_address_pool implementation maintaining the same invariants. At aca020263990, which is both the pinned and supplied upstream default revision, key-wallet exposes derivation and allocation but no invariant-preserving entry-import API. The underlying restoration contract belongs in dashpay/rust-dashcore/key-wallet, leaving SQLite responsible for decoding and recovery policy. The existing upstream snapshot proposal already covers operational address-pool restoration and rebuilding its reverse indexes: https://github.com/dashpay/rust-dashcore/issues/1196. Link that ownership follow-up from this PR and explicitly associate this temporary reconstruction with it; a new broad redesign or an FFI rewrite is not required here.

In `packages/rs-platform-wallet-storage/src/sqlite/load_ctx.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/load_ctx.rs:44-50: Document the Rust source break from extending LoadSite
  (existing thread: https://github.com/dashpay/platform/pull/5210#discussion_r4249556250)
  `LoadSite` is publicly re-exported, gains two variants, and is now `#[non_exhaustive]`, so downstream exhaustive matches require a wildcard arm. The current Unreleased `CHANGELOG.md` contains no `LoadSite`, `PoolRowBeyondDerivationBudget`, or `UnrestoredPoolReservation` migration note, despite the source-breaking API change being described in the PR body. Restore a release-facing entry naming both variants and the required downstream match update.

⏸️ Needs a tracking issue before it can be deferred (1): Track pool reconstruction in its key-wallet owner. Link one in the finding's thread, or ask there and PastaClaw files it.

[thepastaclaw]

🔴 Blocking: Refill re-derived key-only pools after restoring used rows

The overlay restores highest_used and the used-index set for every managed pool, but the subsequent refill logic lives in extend_pools_for_restored_addresses, which is reached only through the funds-account path in rehydrate.rs. all_funding_accounts_mut() excludes identity-registration, top-up, invitation, asset-lock, and provider key-only accounts. When a key-only pool has consumed its initial generated window, restoration therefore leaves highest_generated at the last stored index; its later next_unused_with_info(&KeySource::NoKeySource, ...) call cannot derive the next key and returns NoKeySource. This breaks the primary one-time funding-key paths after restart. Refill every publicly re-derivable restored key-only pool, and add a reopen regression that consumes the initial window before requesting another key.

_{source: gpt-6.1-sol (phase2-reviewer: general)}

[thepastaclaw]

🔴 Blocking: Do not fail Strict loads on stale DashPay pool rows after contact rotation

The restored row is rejected whenever its address or public key differs from the current account derivation. That is correct for fixed accounts, but DashPay external accounts can legitimately retain a pool created from an older contact xpub after rotation: apply_pools upserts current rows without deleting higher old indices. On the next load, restore_pools derives those stale rows from the new xpub and returns this error before the platform-wallet manager can run distrust_markers_of_foreign_external_pools and rebuild the contact account. A normal contact rotation can therefore make Strict loading fail, or cause Recovery to drop the wallet. Preserve/defer stale DashPay external rows to the existing marker/rebuild path while retaining strict derivation validation for non-rotatable account types.

_{source: gpt-6.1-sol (phase2-reviewer: general)}

[thepastaclaw]

🟡 Suggestion: Validate saved indices through upstream per-index derivation

derive_towards sequentially derives every index up to the highest stored row and skips rows beyond the row-count-plus-allowance budget. The upstream AddressPool::prune_unused intentionally retains used and reserved entries while deleting surrounding available entries, so a valid pool can be sparse—for example, a reserved entry at a high index with only the initial window retained below it. Such a snapshot can pass apply_pools but lose the high reservation on reopen, allowing a non-one-time address to be handed out again. Validate each stored row with the owning key-wallet account’s per-index derivation APIs, or otherwise distinguish valid pruned reservations from damaged sparse rows before applying the bounded restoration policy.

_{source: gpt-6.1-sol (phase2-reviewer: architecture-layering)}

[thepastaclaw]

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

The snapshot restore work has three blocking correctness gaps: persisted PlatformPayment pools are never overlaid, restored key-only pools are not gap-refilled, and legacy hardened provider rows without typed EdDSA keys can be advertised as restored while losing ownership indexing. A stale DashPay rotation case also still fails the matched-row conflict guard, and several non-blocking ownership and validation concerns remain. The Rust workspace check is still in progress and the Rust wallet tests are skipped in the supplied CI snapshot.

🔴 4 blocking | 🟡 6 suggestion(s)

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

🔴 Blocking: Fail closed when legacy hardened pool rows have no EdDSA key
packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:115-128

V010__pool_public_key added nullable key columns after core_address_pool existed, so a valid pre-V010 platform-node row can have both public_key and key_type set to NULL. load_typed_pool_entries skips that row, but restore_pools accepts the (None, None) case and inserts the hardened entry with public_key: None because AbsentHardened bypasses derivation checks. The provider ownership index in manager/accessors.rs only indexes PublicKeyType::EdDSA entries, so the row is silently absent from ownership recovery despite the loader advertising a complete restore. Reject or quarantine such legacy rows, or perform a proven migration before treating them as restored.

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

🟡 Suggestion: Bind restored hardened provider scripts to their public keys
packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:117-163

The hardened provider path checks the EdDSA key width and parses the stored script, but never verifies that the script is the P2PKH representation of the node ID derived from that public key. restore_pools intentionally skips derivation checks for AbsentHardened, so a corrupted store can pair a valid EdDSA key with an unrelated valid script. Ownership resolution uses the script while the provider-node index uses the public-key hash, allowing one row to represent two different owners and potentially bypass the funds-account reuse guard. Reject hardened rows whose script does not match the persisted EdDSA key, or exclude them from both ownership and reuse projections.

_{source: gpt-6.1-sol (phase2-reviewer: security-auditor)}

7 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

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

• Triage: critical by gpt-6.1-sol (effort low) — This is a large, intricate change to wallet SQLite restoration that directly affects persisted storage migrations and key/address reservation handling, including preventing reuse of one-time funding keys.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

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

In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:636-641: Restore persisted PlatformPayment pools from their separate managed-account collection
  `restore_pools` obtains accounts only from `wallet_info.accounts.all_accounts_mut()`. In the pinned key-wallet implementation, `platform_payment_accounts` is a separate collection and `all_accounts_mut()` stops after the core and DashPay collections, so persisted rows labeled `platform_payment` are never overlaid with their saved used, reserved, or derivation state. The registration and wallet lifecycle code does persist these pools, but after restart the platform-address provider is rebuilt from the fresh skeleton; previously reserved or handed-out addresses can therefore be offered again and deeper persisted addresses are absent. Iterate the separate platform-payment collection through a restoration path as well, or move the common overlay operation into the key-wallet owner.
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:702-735: Refill re-derived key-only pools after restoring used rows
  (existing thread: https://github.com/dashpay/platform/pull/5210#discussion_r4360090257)
  The restoration loop derives each pool only through its highest persisted index and overlays the rows; it does not run the gap-limit refill for identity, asset-lock, provider, or other key-only pools. `extend_pools_for_restored_addresses` is reached only through `all_funding_accounts_mut()` in `rehydrate.rs`, so a key-only pool restored with a used entry at the end of its saved window returns without the lookahead entries that normal pool operation maintains. A later key allocation can then expose an address beyond the restored observation horizon, repeating the funded-beyond-window reuse risk fixed for funds accounts. Apply the bounded refill through the pool owner for every derivable restored pool and cover a used key-only entry beyond the initial window.
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:874-887: Do not fail Strict loads on stale DashPay pool rows after contact rotation
  (existing thread: https://github.com/dashpay/platform/pull/5210#discussion_r4360090265)
  The address/key conflict guard applies to every restored account, including DashPay accounts. Contact rotation removes the stale in-memory account and re-registers the same identity-pair coordinates with a new xpub, but the SQLite pool writer is upsert-only and does not remove old rows at indices no longer present in the new snapshot. If the old account had generated a deeper window, those stale rows are still selected for the new account, `derive_towards` derives the new address at the same index, and this guard fails Strict instead of treating the stale non-one-time reservation as a degradation. Scope this conflict failure to one-time funding pools or explicitly remove/quarantine stale DashPay rows, with a rotation-and-reopen regression.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:529-552: Validate saved indices through upstream per-index derivation
  (existing thread: https://github.com/dashpay/platform/pull/5210#discussion_r4360090275)
  `derive_towards` bulk-derives every index from the current pool tip through `highest_stored`, capped by `stored_rows + 10,000`, before validating any persisted row. A sparse pool with one row at a high index therefore performs thousands of derivations to validate one row, and checking is coupled to gap-filling. Use the key-wallet per-index derivation primitive, where available, to validate only persisted indices; retain a separate bounded refill for the pool horizon.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:912-934: Track pool reconstruction in its key-wallet owner
  (existing thread: https://github.com/dashpay/platform/pull/5210#discussion_r4341177862)
  This SQLite adapter now constructs `AddressInfo`, derives addresses, validates derived keys and scripts, mutates reverse indexes, and updates `highest_generated`, `highest_used`, and `used_indices` directly. Those are `AddressPool` invariants owned by key-wallet, and a future key-wallet change can make normal operation and SQLite restoration diverge. Expose a narrow validated restore/overlay API from the key-wallet pool or account owner and leave this crate responsible for SQL decoding and Strict/Recovery policy mapping.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:556-572: Share the one-time funding-role definition with the asset-lock writer
  (existing thread: https://github.com/dashpay/platform/pull/5210#discussion_r4257529154)
  `ONE_TIME_FUNDING_LABELS` duplicates the account classification in `platform-wallet`'s asset-lock writer. The parity test parses another crate's source text, so a refactor can break or bypass the test while the two semantic lists drift. Export a typed account-role predicate from the wallet owner and use it in both the writer and restore path; keep SQLite-specific label conversion and load-policy handling here.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:965-980: Check identity-blob widths before copying unmatched funding rows
  (existing thread: https://github.com/dashpay/platform/pull/5210#discussion_r4257529142)
  The wallet-wide one-time audit materializes `user_identity_id` and `friend_identity_id` as `Vec<u8>` before checking their required 32-byte widths. A malformed reserved row is excluded from the normal used-address reader and can reach this audit as its first validation boundary, causing an unnecessary large transient copy before the row is refused. Select both `length(...)` values first, apply `blob::check_fixed_width(..., 32, ...)`, and only then retrieve the blobs.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:673-701: Reject pool rows with impossible account discriminators
  (existing thread: https://github.com/dashpay/platform/pull/5210#discussion_r4248454211)
  The preflight validates only whether a stored `pool_type` exists on the reconstructed account. It does not validate the other account coordinates before the per-pool queries: for example, a registered non-one-time account can have a row with an impossible `key_class` or identity pair. That row is skipped by every restoration query, and the later reservation accounting merely reports it as `UnrestoredPoolReservation`; Strict still succeeds and the address can become available again. Validate all persisted coordinates for registered accounts against their canonical account discriminator, or quarantine such rows instead of returning a supposedly complete restore.

In `packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:115-128: Fail closed when legacy hardened pool rows have no EdDSA key
  `V010__pool_public_key` added nullable key columns after `core_address_pool` existed, so a valid pre-V010 platform-node row can have both `public_key` and `key_type` set to `NULL`. `load_typed_pool_entries` skips that row, but `restore_pools` accepts the `(None, None)` case and inserts the hardened entry with `public_key: None` because `AbsentHardened` bypasses derivation checks. The provider ownership index in `manager/accessors.rs` only indexes `PublicKeyType::EdDSA` entries, so the row is silently absent from ownership recovery despite the loader advertising a complete restore. Reject or quarantine such legacy rows, or perform a proven migration before treating them as restored.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:117-163: Bind restored hardened provider scripts to their public keys
  The hardened provider path checks the EdDSA key width and parses the stored script, but never verifies that the script is the P2PKH representation of the node ID derived from that public key. `restore_pools` intentionally skips derivation checks for `AbsentHardened`, so a corrupted store can pair a valid EdDSA key with an unrelated valid script. Ownership resolution uses the script while the provider-node index uses the public-key hash, allowing one row to represent two different owners and potentially bypass the funds-account reuse guard. Reject hardened rows whose script does not match the persisted EdDSA key, or exclude them from both ownership and reuse projections.

[thepastaclaw]

🔴 Blocking: Restore persisted PlatformPayment pools from their separate managed-account collection

restore_pools obtains accounts only from wallet_info.accounts.all_accounts_mut(). In the pinned key-wallet implementation, platform_payment_accounts is a separate collection and all_accounts_mut() stops after the core and DashPay collections, so persisted rows labeled platform_payment are never overlaid with their saved used, reserved, or derivation state. The registration and wallet lifecycle code does persist these pools, but after restart the platform-address provider is rebuilt from the fresh skeleton; previously reserved or handed-out addresses can therefore be offered again and deeper persisted addresses are absent. Iterate the separate platform-payment collection through a restoration path as well, or move the common overlay operation into the key-wallet owner.

_{source: gpt-6.1-sol (phase2-reviewer: rust-quality)}