[lklimek]

TL;DR: Repair Core transaction history that omits spent inputs and prevent already-spent outputs from becoming selectable after a SQLite wallet restart.

Scope: persisted accounting and safe SQLite restart, including authorization checks before restoring identifiable pending spend reservations. Live Core mempool admission and reservation expiry remain tracked in #5225.

Targets v5.1-dev after the typed Core cryptography prerequisite #5514 was merged. The branch includes rust-dashcore dev aca020263990a86a15cc1d97ae2cbf965d488db5 (workspace version 0.46.0); the published v0.45.0 release is a separate line. The wallet change builds on the earlier merged #5307 accounting baseline.

Issue being fixed or feature implemented

Part of #5126: this PR repairs the Rust accounting and the SQLite store; the SwiftData store follows in #5417. As a wallet user, I want history to reflect what each transaction actually spent and received, including when funding is discovered late or an asset lock is restored after restart.

What was done?

• Before constructing the SQLite wallet projection, validate identifiable pending wallet input authorizations using the existing Dash P2PKH/SIGHASH_ALL signing contract. Atomically remove unauthorized records and their unconfirmed descendants, release only their claims, and remove their outputs before replay. Preserve authorized pending spends and authenticated final claims. The shared wallet helper reuses rust-dashcore cryptography and normalizes high-S signatures through its existing API. No new dependency, migration or FFI change is required.
• Bind startup ChainLock promotion to the exact height and block hash; a saved height alone cannot authorize destructive conflict settlement.

• Reject stored transaction records whose embedded transaction body hashes to a different txid before any replay or conflict cleanup. Strict reads fail; recovery isolates the affected wallet without modifying its stored history.
• Replay SQLite history in dependency order during load, settling conflicts using persisted ChainLock and InstantSend finality. Repair losing history and non-consumed asset-lock lifecycle entries atomically; recovery loads roll back both. Validate replay arithmetic and restore persisted spent claims, including recordless and unknown-owner claims.
• Add SQLite migration V020 (V019 is asset_lock_operation_id from fix(platform-wallet)!: drain Core balance with restart-safe shielded recovery #5396 on the base), which indexes raw transaction inputs and rebuilds Core history by rescanning it: it drops each wallet's mined Core history and lowers its filter checkpoint to just below its birth height, so the next SPV start rescans from birth and records history through the current accounting. Spend evidence a block rescan cannot rebuild is kept: unconfirmed transactions with their instant locks and outputs, and spent rows that no mined spend backs. Their mined funding rows become height-only rows that the rescan fills in, and their claimed inputs are indexed; load-time authorization still authenticates every kept claim and releases forged ones. The SPV header/filter store, address pools, asset locks, identities, DashPay and metadata are untouched.
• Keep an authorized unconfirmed record with out-of-range amounts in conflict settlement: it stays in sweep planning at load and in stored history, so a final competing spend removes it and releases its other inputs whether that spend was persisted before the restart or arrives after it. A record whose owned outputs alone exceed i64 goes back into in-memory history without its output details, and one whose input amounts exceed i64 goes back without its input details; the store keeps both, so a final competitor that leaves no record of its own still removes it and releases what only it claimed. A record left out of the load replay for unsafe amounts is restored into every funding account that owns one of its addresses, so a final competitor arriving after the restart removes its coins and descendants in all of them.
• Ask the conflict sweep at load only about final records that share an input with an unconfirmed record, using an index built once, so peer-supplied mempool inputs no longer multiply startup work by the number of final records. load_spent_claims gates blob lengths inside its reading statement and reports an oversized claim as BlobTooLarge; the scan for surviving stored claimants checks the stored lengths of both of its columns before copying or decoding them, and rejects an oversized value as BlobTooLarge; SQLite still loads such a value first, as in the crate's other readers. The contact-ownership and prior spend-state probes use the statement cache.
• Replay InstantSend-locked records before plain mempool ones at load, and a final record's stored ancestors with it, including ancestors whose stored record still says mempool, so unrelated mempool records never enter history ahead of a final record.
• Plan conflict sweeps at load in a scratch wallet without coins, handing each sweep only the competitors of its winner and their unconfirmed descendants; the planned sweeps are unchanged.
• Record the surviving stored spender, not the conflict's winner, as the claimant of an input a sweep holds back, so removing that spender after a restart releases the coin.
• When load settlement removes a transaction, a DashPay payment the wallet sent with it, whether pending or confirmed, is marked failed in the same atomic repair, exactly as the live sweep marks it; received payments are left alone, and a recovery load rolls the verdict back with the rest of the repair.
• Preserve known ownership through partial replays, isolate accounting by wallet, exclude contact watch-only outputs, and retain spendability for ambiguous credit verdicts. Project corrected transaction topology and coalesce repeated account snapshots before calculating totals.
• Share Rust ownership, direction and net-amount rules between live projection and runtime SQLite repair. Add typed storage errors for transaction-body conflicts, unknown wallet networks and amount overflow.
• The SwiftData host's own accounting reconcile, wallet-scoped amounts, schema migration and example-app views belong to fix(swift-sdk): reconcile SwiftData transaction accounting per wallet #5417, stacked on this PR. Nothing under packages/swift-sdk changes here.

This PR uses rust-dashcore dev aca020263990a86a15cc1d97ae2cbf965d488db5 (workspace version 0.46.0), inherited from merged #5514. SpvRuntime::is_running follows #1177, which made dash-spv's is_running synchronous. The PR uses its spent-claim restoration API: claims are held once per wallet and guard every funding account, including one registered after the load. Core spending does not imply Platform asset-lock consumption. No consensus-versioned behavior changes are introduced in this diff.

Known limitations and deferred work

• Live Core mempool processing still accepts peer observations without checking spending signatures (platform-wallet: unconfirmed spend reservations from unauthenticated mempool transactions never expire #5225). SQLite load now revalidates identifiable pending wallet spends before replay. This establishes wallet authorization, not complete transaction validity or finality. Unsupported authorized signature modes or funding scripts fail restoration without mutating the store; anonymous and recordless final holds remain protected.
• When the store holds no record of a surviving spender of a held input, the sweep still names its winner as claimant, so that coin stays reserved until a rescan. An unknown claim on a held input that the sweep was not asked to release is still replaced by the sweep's winner.
• An authorized record restored outside the replay marks nothing spent by itself: a coin only it spends is credited if its funding first arrives after the restart. Without a final competitor such a record, like any other unconfirmed reservation, stays until expiry is implemented in platform-wallet: unconfirmed spend reservations from unauthenticated mempool transactions never expire #5225.
• Upstream sums the amounts a late funding attributes to a record without checking, for any record; checked accounting belongs in key-wallet.
• Each spent coin queues its funding record for repair again, so spending N outputs of one transaction in separate rounds costs N² ownership probes in total; one round is bounded by the outputs of that transaction.
• A record restored outside the replay appears in per-account history once per owning account, each copy with the wallet-level amounts. A coin that load parks in the first account because no address pool knows its script is not reached when the stored record names another account.
• Full chain-truncation persistence remains in fix(platform-wallet)!: persist chain truncations so a stored wallet follows a reorg #5384. SQLite load does not infer ancestor branch membership from height: promotion using a saved ChainLock requires the exact block height and hash, and replay does not apply that checkpoint globally to stored history. Stale branch records therefore cannot gain ChainLocked priority solely from their height.
• An InstantSend-locked record that a plain block conflicts with is re-examined once per such block at load; deciding that precedence stays upstream.
• Planning and replay at load are linear in the inputs of stored unconfirmed records, which nothing limits or expires (platform-wallet: unconfirmed spend reservations from unauthenticated mempool transactions never expire #5225).
• load holds the persister's single connection for the whole replay, as it did for the whole load before this change, so a store or flush issued during startup waits for it. Each wallet's repair stays atomic with its reads.
• The upstream conflict sweep scans the whole account history per call: quadratic in the wallet's own history at load, and one scan per final transaction while running. Bounding it needs an indexed sweep in key-wallet.
• If the key-wallet transaction checker ever suspended during load replay, the wallet would load without replayed history and finality (durable spend claims are still restored); a test fails CI if a dependency change makes that reachable.
• SQLite wallet drain and operation-status queries remain unsupported: conflict settlement can delete operation evidence, so the backend does not attest ASSET_LOCK_OPERATION_IDS. It also does not attest WALLET_RESTORE in this PR. Public drain/status calls fail at capability admission before they can interpret missing evidence as NotSent or create another funding transaction. Enabling drain requires durable evidence that survives cleanup.
• The migration was renumbered from V021 to V020 so wallet-storage versions stay contiguous (guarded by test(platform-wallet-storage): require unique, contiguous migration versions #5553). A development database that applied V021 from an earlier revision of this branch fails to open (MissingVersion) and must be recreated. Released paths (V018 → V019 → V020) are unaffected.
• Unmined records kept by V020 retain their pre-V020 accounting (input details, net, direction), which may be wrong, until the wallet records them again — at the latest when they mine. SQL cannot recompute it; preserving the spend reservation outweighs a temporarily wrong amount on a pending record. A kept record whose body cannot be decoded fails a Strict load, as it did before V020; Recovery skips that wallet.
• Unconfirmed reservation expiry (platform-wallet: unconfirmed spend reservations from unauthenticated mempool transactions never expire #5225), releasing repair spends after reorg, and unifying record coalescing remain deferred.
• The SwiftData store is repaired in fix(swift-sdk): reconcile SwiftData transaction accounting per wallet #5417; Rust-authoritative Swift accounting and a persisted backfill marker belong to platform-wallet: make Rust the single source of Core transaction accounting (drop Swift re-derivation) #5226.
• fix(platform-wallet): require only atomic tracked-lock writes for reconciliation #5207 separates SQLite asset-lock reconciliation from WALLET_RESTORE; feat(wallet-storage)!: restore complete Core wallet snapshots from SQLite #5210 adds complete SQLite snapshots; fix(platform-wallet)!: replay recorded transaction history on load for every persister #5220 moves history replay to the shared load boundary, including FFI/SwiftData. fix(platform-wallet)!: preserve Core wallet snapshots on restart #4777 remains separate.

How Has This Been Tested?

• Startup security follow-up: both blocking regressions reproduced before implementation. The fixed tree passed 581 SQLite integration tests (one preexisting ignored test), 44 rehydration tests, and scoped CI Clippy for platform-wallet and platform-wallet-storage with all targets/features, --locked -- --no-deps -D warnings. Tests cover forged claims, valid pending/final competing spenders, descendant cleanup, atomic rollback and read-only recovery, and a stale branch with a competing InstantSend transaction across repeated loads. All 8 owner-signature tests and the final 29-test storage subset passed. The broad 581-test run preceded the final deterministic survivor selection and additional raw-flag test assertions; the focused final repeats and Clippy validate those final changes. The 29-test subset is not an additional distinct test count.

• Integrity fix at f7500221fa: reproduced a one-field corrupted-body case that previously let strict load delete an unrelated pending transaction. The regression now passes in Strict and Recovery, including point reads and zero database mutations. All 1,052 active storage tests pass (disjoint runs of 1,008 non-sweep tests and 44 sweep tests; two preexisting ignored tests). Storage Clippy with all targets/features, --locked -- --no-deps -D warnings, formatting and whitespace checks passed. An independent security follow-up verified all four stored-record readers; the three-agent review found this one actionable issue, now fixed.

• Drain capability follow-up: 56 targeted storage tests passed, including cleanup → reopen → public status/drain rejection with no writes. The regression failed before withdrawing the unsupported operation-ID capability. Scoped all-targets/all-features storage Clippy, formatting and whitespace checks passed.

• Current merged head c653e46d9f: 361 focused wallet/storage tests pass with all features and the locked dependency graph. Coverage includes conflict replay, accounting, V020 rescan, migration execution and fingerprints, clean restart, recovery mode, transaction sweeps, asset-lock filtering and SPV readiness. 2,281 unrelated tests were filtered out. cargo clippy -p platform-wallet -p platform-wallet-storage --all-targets --all-features --locked -- --no-deps -D warnings, scoped formatting and whitespace checks passed.
• Prior validation before the current base/dependency refresh: the complete wallet/storage suites passed 2,539 tests, including the SQLite query-caching gate; scoped CI Clippy passed.
• sqlite_core_rescan.rs drives a wiped wallet through block processing, persistence and reload: the rescan rebuilds input details, net amounts and directions (including an asset lock without change), and unconsumed asset locks load and survive the rescan unchanged. A second scenario upgrades a V018 store holding a checkpoint and a ChainLock above that history: after V020 the rescan runs through the ChainLocked path and rebuilds the same accounting, also after another reload. A third scenario upgrades a store holding confirmed funding, an InstantSend-locked unmined spend, a signed pending send and a forged claim, then rescans blocks only, without mempool or InstantSend redelivery: both reservations hold, the forged claim is released, the funding is recorded as confirmed again and the kept spends re-record correctly once mined. A migration test pins each V020 keep/drop rule, schedules the rescan from birth and checks that every other table is identical. A bounded-work test covers spend authorization over inputs naming absent parent outputs (100k×100k).
• Regression tests cover the claimant recorded for a new unconfirmed spend, a wallet load holding an unconfirmed transaction with out-of-range amounts, the release of an input freed by a persisted conflict after restart, settlement of an out-of-range unconfirmed record against a final spend before and after restart (including one whose owned outputs exceed i64, and the stored outputs it keeps when written again), the same settlement for a record paying two funding accounts and for its child, a record with out-of-range input amounts restored without its input details and removed by a final competitor that leaves no record (also when that competitor spends one of the wallet's own coins), the load planning the same sweeps with and without the shared-input filter over generated histories, an oversized spent claim rejected before it is read, migration from V018 with such a record, the release of a held input once its surviving spender is removed after restart, and the failed verdict of a pending or confirmed sent DashPay payment whose transaction load settlement removes (received payments untouched, rolled back by a recovery load).
• On the merged base (v5.1-dev) with rust-dashcore aca0202: cargo check --workspace --all-targets --locked passes, and the platform-wallet-storage and platform-wallet suites (2,539 tests, including persisted_core_transactions and sqlite_spent_rehydration) pass, Clippy-clean with -D warnings.
• Earlier branch validation covered wallet/accounting, encryption, DPP signing/BLS and RPC compatibility; migration-specific validation is documented in chore(platform)!: update rust-dashcore (incl secp256k1 0.33) #5307.
• Live-network end-to-end tests and the full workspace runtime suite were not run here.

Breaking Changes

WalletStorageError becomes #[non_exhaustive] and gains TransactionBodyConflict, UnknownWalletNetwork and NetAmountOverflow; downstream exhaustive matches need a wildcard arm. SQLite adds V020, which drops stored mined Core history (keeping unmined spend evidence) and rescans each wallet from its birth height on first open. Development databases that applied this branch's earlier V021 must be recreated because refinery rejects divergent migrations. V020's SQL body and pinned schema fingerprint were also changed in place during review (now 7680eaf99ca79ed8929ed953dd99aef14ea242af4cf2c9d8b5b4992a8ad5dfe7); this is acceptable only because V020 is unreleased, and development databases that applied an earlier V020 must be recreated as well. V020 decodes V019 records with the live TransactionRecord codec; immutable V019 byte fixtures make any incompatible serde change fail CI when key-wallet/dashcore are bumped.

The latest typed Core cryptography API migration was merged in #5514. Earlier Node.js >=20 and upstream development-database compatibility requirements belong to #5307. The FFI ABI is unchanged.

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
• If I added or changed GroveDB structure, I described it in the area's structure.rs, regenerated grovedb-structure.json, and checked the structure viewer link posted on this pull request

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone

Owner-side checked record accounting and a supported safe restoration API are tracked in rust-dashcore#1213. The in-memory accounting sanitization is temporary; it does not fix native key-wallet late-attribution arithmetic.

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

PR Hygiene · f72313e

• Bots — coderabbitai skipped after the window · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it
• Self-review — address llbartekll requested changes, then post /self-reviewed
• Build green
• Approvals
  • files with no dedicated owner (CHANGELOG.md) — QuantumExplorer or shumkov
  • wallet-storage — you own it
  • rs-platform-wallet (packages/rs-platform-wallet/src/changeset/changeset.rs, packages/rs-platform-wallet/src/changeset/core_bridge.rs, packages/rs-platform-wallet/src/changeset/mod.rs and 4 more) — HashEngineering or ZocoLini or llbartekll or romchornyi

When every merge requirement is met, the PR Hygiene check passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.

[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: v5.1-dev
• File: .coderabbit.yaml
• Commit: 132ae31
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 11004ac7-44c7-472a-a207-ba3f964a5003

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 — 1 blocking finding(s) (commit f72313e) · triage: critical

[thepastaclaw]

Preliminary review — Phase 1 blocker gate

The SQLite repair logic addresses the stated restart and historical-accounting failures, but the confirmed-spend replay is implemented only in the SQLite persister while the FFI/SwiftData restore path does not rebuild the corresponding spend guards. Several additional correctness and scalability issues remain in the new history-repair and Swift reconciliation code.

Validated blockers were found by the Phase-1 review and confirmed by a fresh verifier. Phase 2 is deferred until a fresh same-head revalidation clears the blocker gate.

🔴 1 blocking | 🟡 7 suggestion(s)

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

🟡 Suggestion: History replay holds the SQLite connection lock for the full confirmed history
packages/rs-platform-wallet-storage/src/sqlite/persister.rs

load_one_wallet invokes restore_confirmed_transactions while load holds the connection mutex. The replay performs an async block_on round trip and runs check_core_transaction once per confirmed record, even though the replay itself does not access SQLite. Large histories therefore block concurrent store and flush operations for the duration of the replay. Snapshot the required records and release the connection lock before replaying, or document and bound this startup write-stall behavior.

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

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6-astra (agent: astra-gate-verifier, role: verifier)

• Triage: critical by gpt-6-astra (effort low) — The diff combines intricate persisted-accounting repair and a new SQLite migration with changes to restore_confirmed_transactions in packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs that directly determine spendable UTXOs through transaction replay, spent-output exclusion, account deduplication, and finality restoration.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — architecture-layering (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 13% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh verifier: gpt-6-astra — verifier; agent astra-gate-verifier
• Phase 2 reviewers: not run (deferred by blocker gate)

🤖 Prompt for all review comments with AI agents

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

In `packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:423-505: Confirmed-history replay is implemented only for SQLite
  The restart invariant is that every persister must hand WalletManager a spend projection containing confirmed spent outpoints. This change enforces that invariant only in SQLite by replaying persisted records through `check_core_transaction` inside `restore_confirmed_transactions`. The FFI restore path still reconstructs spendability from persisted TXO rows, and the Swift reconciliation pass only rewrites `netAmount` and `direction`; it does not rebuild Rust's `spent_outpoints` or remove confirmed inputs from the in-memory UTXO set. Consequently, a wallet restored through the FFI/SwiftData path can still rehydrate a confirmed-spent output as selectable, while SQLite behaves differently. Move the confirmed replay to the shared async `load_from_persistor` boundary, or provide equivalent replay semantics for every persister before the manager consumes the start state.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:6811-6863: SwiftData independently re-derives Rust-owned accounting
  The SwiftData repair path recomputes ownership, amounts, and direction from decoded prevouts, while SQLite's `repair_record` derives the same values from persisted UTXO state and the Rust wallet projection. These implementations already have materially different rules: Swift requires every input prevout to resolve, passes `allOutputsOwned = false` to its helper, and re-encodes contact exclusion using a raw account-type tag, whereas the Rust repair can recover ownership from the UTXO table and distinguishes internal transactions from external outputs. Future checker or contact-ownership changes can therefore make the two persistence backends repair the same transaction differently. Centralize this accounting projection in Rust and have Swift persist the returned verdict rather than reimplementing it.
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:6915-6934: Load-time repair scans the entire store and fails all wallets on one bad row
  `loadWalletList` fetches every `PersistentTransaction` and every `PersistentTxo`, then filters transactions in memory for the wallets being loaded. This makes startup work proportional to the entire SwiftData store even when loading one wallet. The same `do` block also treats any reconciliation or fetch error as a failure for the complete wallet list, so one unreadable transaction or TXO hides otherwise healthy wallets. Scope both fetches to the loaded wallet IDs, or batch per wallet and isolate per-wallet reconciliation failures.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_history.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_history.rs:32-36: Same-txid redelivery with different witness bytes aborts persistence
  `preserve_known_details` rejects a persisted transaction whenever `previous.transaction != incoming.transaction`. A transaction's txid excludes witness data, so legitimate observations of the same txid can differ only in witness bytes. That causes the entire changeset to fail even though the row is already keyed by txid and the ownership details below could be merged. Accept same-txid redeliveries whose non-witness transaction identity is unchanged, or compare an appropriate txid-level identity rather than full transaction equality. If a mismatch must remain fatal, include the txid in the typed error so the failure is diagnosable.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_history.rs:150-157: Contact filtering hardcodes a database label
  `contact_only_script` embeds `account_type != 'dashpay_external'` directly in SQL, while the canonical mapping is `accounts::account_type_db_label`. If the persisted label changes, the writer and this repair query can silently diverge and contact-only outputs can be treated as wallet-owned during repair. Use a shared constant or construct the query with the canonical label and add a test tying the SQL value to `account_type_db_label`.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_history.rs:292-313: Migration performs redundant decoding and wallet lookups per record
  During migration, each row is decoded by `get_tx_record`, then `repair_record` calls `get_tx_record` again for the same txid. The loop also calls `network(tx, &wallet_id)` once per transaction, which repeats the wallets-table lookup for every record. Because this runs inside the migration transaction, large histories hold the migration lock while doing twice the decoding and redundant queries. Cache the network per wallet and pass the already-decoded record into the repair function.

In `packages/rs-platform-wallet/src/changeset/changeset.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/changeset.rs:628-643: Account coalescing mixes finality from one observation with body data from another
  When a later record has lower context rank, the code keeps the later record's amounts, inputs, and outputs but replaces only its context with the earlier record's more-confirmed context. That produces a synthetic record combining the stale observation's body with finality it did not carry. If corrected-then-stale ordering is reachable, the higher-ranked observation should win wholesale; if it is impossible, this branch is dead and should be removed. Add a regression test for corrected-then-stale ordering and replace the record wholesale based on context rank.

In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/persister.rs: History replay holds the SQLite connection lock for the full confirmed history
  `load_one_wallet` invokes `restore_confirmed_transactions` while `load` holds the connection mutex. The replay performs an async `block_on` round trip and runs `check_core_transaction` once per confirmed record, even though the replay itself does not access SQLite. Large histories therefore block concurrent store and flush operations for the duration of the replay. Snapshot the required records and release the connection lock before replaying, or document and bound this startup write-stall behavior.

[github-actions]

Waiting for bot review — coderabbitai not yet · thepastaclaw 2 threads unresolved — resolve them. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[github-actions]

Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[github-actions]

Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[github-actions]

Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 3 threads unresolved — resolve them. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[thepastaclaw]

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

The PR fixes most previously identified Core accounting and replay issues, but three in-scope blockers remain. V020 can still fail before its Recovery policy handles unreadable kept records, its typed migration depends on the mutable runtime TransactionRecord codec, and Recovery authorization cleanup deletes by the blob txid rather than the physical SQLite row key when column drift is tolerated. The Rust workspace test job is still in progress; other package checks are skipped in the supplied CI snapshot.

🔴 3 blocking | 🟡 7 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); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: critical by gpt-6.1-sol (effort low) — The large, intricate changes in sqlite/rehydrate.rs, sqlite/schema/core_state.rs, wallet/core/spend_authorization.rs and migration V020 directly alter persisted spend claims, input authorization, selectable funds and wallet storage migration behavior.
• 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 — 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/migrations/v020.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/migrations/v020.rs:37-46: Gate V020 kept-record blobs before SQLite materializes them
  The Recovery path promises to skip kept records whose bodies cannot be read, but the query selects `wallet_id`, `txid`, `length(record_blob)`, and `record_blob` together. SQLite steps the complete row before Rust reaches `blob::check_size`, so a record above SQLite's configured maximum can make `rows.next()` return a raw SQLite size error and abort the migration instead of being skipped. Values above the crate limit are also materialized before the policy gate. Use a length-only read followed by a payload read, or a lazy `CASE` expression for the payload, so Recovery can isolate the unreadable wallet as intended.
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/migrations/v020.rs:9-17: Freeze the TransactionRecord codec used by V020
  V020 is a one-shot upgrade for persisted V019 databases, but its typed hook calls the live `core_state::decode_transaction_record` codec. Refinery freezes the SQL migration checksum, not this Rust decoder. A later key-wallet or dashcore serialization change can therefore make a valid V019 database fail during V020 or change which inputs are indexed. The current migration tests generate records with the current encoder, so they do not establish compatibility with immutable V019 bytes. Use a frozen historical decoder or an explicit codec-version adapter and test it with immutable V019 fixtures.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/migrations/v020.rs:37-46: Gate V019 identifier blobs before materializing migration key scans
  The renamed V020 typed scan still selects `wallet_id` and `txid` before applying their typed checks. A malformed pre-existing database with an oversized key can therefore materialize the key or return SQLite's allocation/size error before the intended validation runs. Keep these identifiers behind length-only queries or lazy payload expressions so migration corruption is classified deterministically and the Recovery policy can handle it consistently.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_spend_authorization.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_spend_authorization.rs:192-197: Recovery authorization cleanup deletes by blob txid instead of the persisted row key
  Recovery tolerates a `CoreTransactionColumnDrift` where the transaction body and `TransactionRecord::txid` are valid but the physical `core_transactions.txid` key differs. Authorization repair then adds the blob txid to `rejected` and deletes `core_transactions`, locks, outputs, and related state using that blob txid. For a row keyed by K whose valid pending record is R, no transaction row is deleted; a subsequent snapshot still returns R and the loader can restore its unauthorized pending reservation while the corrupt row remains stored. Preserve the physical SQLite key alongside each loaded record and use it for cleanup, or make txid drift fatal before authorization repair.

In `packages/rs-platform-wallet-storage/tests/integration/sqlite_migrations.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/tests/integration/sqlite_migrations.rs:602-603: Freeze historical transaction blobs in the V019 migration regression
  The current migration regression still creates its historical `TransactionRecord` with the live encoder and derives expected values through the same codec. A future codec or dependency change could update both the inserted bytes and expectations together while the compatibility test remains green. Add an immutable blob fixture originating from the pre-V020 writer while retaining the generated case for behavioral coverage.

In `packages/rs-platform-wallet-storage/migrations/V020__core_transaction_accounting.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/migrations/V020__core_transaction_accounting.rs:92-96: Validate the stored birth height before subtracting the rescan offset
  V020 computes `w.birth_height - 1` directly in SQL before any typed range validation. The schema does not constrain `birth_height`, so a corrupt negative or oversized value can produce an invalid rescan watermark or SQLite numeric conversion behavior after the migration has already deleted mined Core history. Read and validate every birth height before destructive cleanup, then bind a checked or saturating `u32`-domain rewind value into the update.

In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:1524-1577: History replay holds the SQLite connection lock for the full confirmed history
  (existing thread: https://github.com/dashpay/platform/pull/5150#discussion_r4264068577)
  `load()` acquires the persister's only connection before the wallet loop and retains it through snapshot reads, in-memory replay planning, checker polling, conflict repair, and the second snapshot. The new replay work therefore blocks `store`, `flush`, and `delete_wallet` for the full persisted-history duration. The existing atomicity rationale is valid, but the critical section should eventually be split or moved to a dedicated connection with an explicit consistency protocol.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_history.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_history.rs:200-203: Reuse stored bytes and decoded records during runtime repair
  (existing thread: https://github.com/dashpay/platform/pull/5150#discussion_r4260719018)
  `repair_record` reads and decodes the stored record, then immediately re-encodes it as `original` before applying the repair. The stored payload was already materialized and can serve as the comparison baseline, avoiding a full serialization and allocation for every repaired record while the SQLite transaction is held. Carry the validated payload with the decoded record through the repair path.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_history.rs:102-123: Avoid re-repairing unchanged funding records on every spend
  (existing thread: https://github.com/dashpay/platform/pull/5150#discussion_r4259614874)
  `apply()` adds every funding transaction referenced by `new_utxos` and `spent_utxos` to `affected`, including the ordinary path where an existing funding row only changes its spent or claimant state. `repair_record` then repeats all ownership and output probes. Spending N outputs from one funding transaction in separate rounds consequently retains the documented quadratic aggregate work. Distinguish newly materialized or newly attributed records from plain spent-state updates and add a query-count regression.

In `packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:522-552: Do not report a successful load when replay suspends
  (existing thread: https://github.com/dashpay/platform/pull/5150#discussion_r4252020712)
  If the synchronous checker poll returns `Pending`, this branch restores the pre-replay snapshot and returns `Ok(Vec::new())`. Durable claims are restored later, but the returned wallet lacks replayed history, finality promotion, and conflict settlement while load is reported as successful. The current pinned checker completes on its first poll and the regression detects a dependency change, but the production fallback should return a typed incomplete-replay error or move this work to an asynchronous load boundary.

[thepastaclaw]

🔴 Blocking: Freeze the TransactionRecord codec used by V020

V020 is a one-shot upgrade for persisted V019 databases, but its typed hook calls the live core_state::decode_transaction_record codec. Refinery freezes the SQL migration checksum, not this Rust decoder. A later key-wallet or dashcore serialization change can therefore make a valid V019 database fail during V020 or change which inputs are indexed. The current migration tests generate records with the current encoder, so they do not establish compatibility with immutable V019 bytes. Use a frozen historical decoder or an explicit codec-version adapter and test it with immutable V019 fixtures.

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

[Claudius-Maginificent]

Per the owner decision, V020 uses the live core_state::decode_transaction_record codec shipped in the same release; a frozen decoder is warranted when a future serde change breaks compatibility, rather than adding one now. The immutable V019 bytes are checked by should_decode_immutable_unmined_contexts_and_leaf_values_with_live_codec, should_decode_all_immutable_v019_transaction_fixtures_with_live_codec, and should_index_immutable_v019_record_bytes in src/sqlite/migrations/v020.rs, plus should_decode_and_re_encode_every_stored_transaction_row_unchanged in tests/integration/persisted_core_transactions.rs, so an incompatible change to the represented formats fails CI at dependency-bump time. The separate codec crate is removed: after dependency revisions diverged, its separately pinned dashcore/key-wallet copy would also be compiled into mobile FFI builds.

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

[thepastaclaw]

Still applies (re-reviewed at f72313e4): You added immutable compatibility fixtures, but V020 still executes the live transaction codec, so future codec changes can still alter or break historical migration behavior.

[github-actions]

Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[thepastaclaw]

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

The PR fixes the persisted SQLite accounting and restart-safety defects identified in the prior review. One blocking issue remains: V020 still decodes historical V019 records with the live transaction codec, so future dependency changes can alter or break a one-shot migration despite immutable fixture tests. The Rust workspace test check is still in progress and policy checks remain pending; no failed code check is reported.

🔴 1 blocking | 🟡 1 suggestion(s)

2 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: 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: 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: rust-quality); 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 funds movement and coin-selection accounting, cryptographic signature authorization, and SQLite migration/rehydration logic, including V020 persisted-state transformation.
• 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/migrations/v020.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/migrations/v020.rs:59-69: Freeze the TransactionRecord codec used by V020
  (existing thread: https://github.com/dashpay/platform/pull/5150#discussion_r4363087135)
  V020 is a one-shot compatibility migration, but it still calls the live `core_state::decode_transaction_record` decoder and `core_history::index_record` writer for already-shipped V019 rows. The immutable fixtures verify selected historical bytes against the current implementation and will detect some future incompatibilities in CI, but they do not make the migration behavior independent of later codec changes or guarantee that every valid V019 representation will retain the same interpretation and indexed input set. A future dependency update can therefore strand an existing wallet during upgrade or change which durable spend evidence is reconstructed. Use a frozen historical decoder/input adapter for V019, while retaining the immutable fixtures as regression coverage.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_history.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_history.rs:102-123: Avoid re-repairing unchanged funding records on every spend
  (existing thread: https://github.com/dashpay/platform/pull/5150#discussion_r4259614874)
  `apply` adds every spent outpoint's funding transaction to `affected`, including the ordinary materialized-row path where only the spent flag and claimant change. `repair_record` then decodes that funding record and repeats the ownership, contact, and output probes even though its accounting evidence did not change. Spending N outputs of one funding transaction in separate persistence rounds therefore repeats repair work approximately quadratically while the SQLite transaction is held. Distinguish newly materialized or accounting-changing rows from pure spent-state updates, or coalesce funding repairs before invoking `repair_record`.

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.

• Shared confirmed-history replay remains deferred for non-SQLite persisters — The current change reconstructs confirmed spend state in SQLite, while the shared FFI and SwiftData restore boundary remains dependent on the stacked replay work. This is a concrete wallet-safety follow-up, but it is not a new regression in this diff and is explicitly tracked.
  • Follow-up: Complete #5220 before relying on non-SQLite restore paths for the same confirmed-spend invariant.
• Live peer mempool reservation admission remains unauthenticated — SQLite load now authenticates identifiable pending claims, but live peer admission and reservation expiry still allow the pre-existing unauthenticated reservation behavior. This is explicitly outside the PR's persisted SQLite scope and is tracked separately.
  • Follow-up: Complete #5225 for provenance-aware admission, expiry, and safe release.

[github-actions]

Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.