fix(platform-wallet-storage)!: harden wallet history restore after #5150 review

[lklimek]

TL;DR: Follow-up fixes from the multi-model review of #5150: unconfirmed spends survive restart, history repair is reversible and recoverable, Swift per-wallet amounts are correct, CoreHistoryReplay is removed, and history integrity failures get precise error variants.

Stacked on #5150 (fix/pr-5126).

Siblings on #5150: #5207 (asset-lock reconciliation without WALLET_RESTORE); #5210 (full Core wallet snapshot restore, stacked on #5207) already includes this PR at 338067889e.

Issue being fixed or feature implemented

Addresses the remaining review findings on #5150 (#5126). The three HIGH findings (duplicate UTXO after replay, load-time reconcile inside an open changeset, contact-only outputs loaded as spendable) are fixed directly in #5150.

What was done?

• Unconfirmed spends restored on load: load also replays mempool and InstantSend records, in dependency order (parents first, otherwise chain order), so redelivering a funding transaction after restart can no longer release outputs reserved by a pending spend.

• Reversible history repair (V019, unreleased, edited in place): V019 adds core_transaction_record_originals (append-only), which stores each record's pre-repair blob. Every repair-set spent flag records spent_in_txid.

• Frozen V019: the migration's repair logic lives in migrations/legacy_v019.rs, so later changes to the shared repair cannot change what V019 did. A V018→V019 pinning test locks the result.

• Corrupt records:

  • Runtime repair stays strict: a corrupt prior blob is an error.
  • V019 drops an undecodable block-confirmed record (and its input index) and lowers that wallet's synced_height to birth_height - 1, forcing an SPV rescan that re-delivers it.
  • An undecodable unconfirmed record, which a rescan cannot restore, still fails the migration atomically.

• No async bridge: the transaction checker completes on its first poll, so replay polls it once instead of calling dash_async::block_on. This removes WalletStorageError::CoreHistoryReplay and the dash-async dependency. A unit test fails if the checker ever stops completing on first poll.

• One direction rule: Rust's direction rule is extracted as repaired_direction, and Swift uses the same rule. Both sides share one case table in tests. Upstream rust-dashcore keeps its own rule, which is out of scope here.

• Swift accounting:

  • netAmount(for:) checks pendingInputs before the single-wallet fast path.
  • Outputs matched only by address count only toward wallets that fund the transaction.
  • A stored asset-lock debit is preserved when no input is linked yet.

• Swift load: a load-time accounting failure rolls back only its own pass, logs a distinct event, and no longer blocks wallet restore. Address lookups are batched.

• Low-severity cleanups:

  • Unresolved amounts render as unavailable.
  • Reconcile failure has its own log event instead of save_failed.
  • Swift magic numbers replaced with named constants.
  • Tests for uncredited outputs and V019 spent marking.
  • Hand-written ## Unreleased section removed from CHANGELOG.md.

• Second review round (grumpy review of fix(platform-wallet)!: restore Core spending state and repair persisted accounting #5150):

  • Storage: use imports at the top instead of inline paths; long SQL literals split so rustfmt formats the calls; one DASHPAY_EXTERNAL_LABEL shared by the account label and the contact-only query.
  • New tests: a replay credit for a contact-only script never survives load; same-height records without a block position give the same result in either order.
  • Swift: the load-time TXO map no longer traps on a duplicate outpoint. Accounting accessors, the amount formatter (no Int64.min trap) and CoreDirectionCode are shared. isOpReturn replaces 0x6a. The list row and the detail view use one direction style, so an internal transfer is now red in both and CoinJoin shows the shuffle icon.
  • Precise history errors: WalletStorageError gains TransactionBodyConflict (Constraint), UnknownWalletNetwork (Fatal) and NetAmountOverflow (Constraint) instead of reusing BlobDecode. The enum becomes #[non_exhaustive].
  • Merged the updated fix(platform-wallet)!: restore Core spending state and repair persisted accounting #5150 base (v4.3-dev, PV15 query-version fix).

Changelog entry (moved from CHANGELOG.md): platform-wallet-storage: Restore confirmed Core spend and finality state on SQLite load so old funding transactions cannot make already-spent outputs selectable again.

Bot review fixes:

• Replay restores InstantSend context from persisted IS locks. A mempool record whose lock arrived later now replays as InstantSend.
• replay_order is a topological sort over all records, so a stale mempool parent replays before its confirmed child. Independent records keep chain order.
• V019 read_record checks the blob length before fetching the payload.

Deferred (marked with TODO(...) in code)

• bound-load-history-replay: replay only records above the last chain lock. This is performance only and touches finality/guard correctness.
• release-repair-spends-after-reorg: release rows whose spent_in_txid spender left the chain. Needs verification of how upstream downgrades a stored context on reorg.
• unify-record-coalescing: coalesce_newest_wins vs the second coalesce helper. This changes Merge semantics and predates fix(platform-wallet)!: restore Core spending state and repair persisted accounting #5150.
• test-round-accounting-failure: fault-injection test for a round-end reconcile failure. The handling is already in place; the test needs a Swift toolchain to pin which fetch fails.
• persist-accounting-backfill-marker: the Swift full-history pass runs on every launch. A marker needs a SwiftData schema version (V4) or a UserDefaults side channel, which is a product decision.

How Has This Been Tested?

• cargo nextest run -p platform-wallet-storage: 1011 passed, 2 skipped (after the second round); clippy --all-targets -D warnings clean; cargo fmt --check clean.
• New regression tests failed before their fix, including:
  • unconfirmed spend reservation with funding redelivery after restart;
  • unconfirmed funding confirming after reload;
  • corrupt confirmed record dropped with rescan;
  • corrupt unconfirmed record failing the migration;
  • strict runtime rejection;
  • V019 pinning;
  • first-poll checker;
  • direction case table.
• Swift tests were added for each Swift fix. Not run: Swift compilation and tests (no Apple tooling on Linux).
• CI and live-network testing were not run locally.

Breaking Changes

WalletStorageError (public, platform-wallet-storage) is now #[non_exhaustive] and gains TransactionBodyConflict, UnknownWalletNetwork and NetAmountOverflow. Downstream exhaustive matches need a wildcard arm. Errors previously reported as BlobDecode for these three cases now carry their own variant and metric tag. This PR also removes the unreleased CoreHistoryReplay variant added by #5150. No FFI changes. V019 is unreleased and is edited in place instead of adding a new migration. It gains the core_transaction_record_originals table, which the V019 repair itself writes to, so the table cannot move to a later migration. As a result, a development database that already ran #5150's V019 fails refinery's divergent-migration check and must be recreated.

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

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

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

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

⚙️ Run configuration

Configuration used: Repository: dashpay/platform/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 2163a5a6-0810-4b03-9603-cb26a217c7b3

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

---

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]

🕓 Queued for automated review — 3rd in line, estimated start in ~45 min (commit 2a98ef8)
Estimated review time once started: ~1.6 h (two-phase automated review; median of recent runs).

• Request priority review — click to move this review to the front of the queue.

[thepastaclaw]

Final validation — Phase 1 + Phase 2

The PR improves wallet history restoration, but three blocking defects remain in the new recovery paths: persisted InstantSend locks are applied before transaction records exist, corrupt confirmed records can be deleted without scheduling a rescan, and legacy height 0 records are misclassified as confirmed. The replay ordering and migration blob-size handling also need hardening to preserve spend reservations and bounded allocation behavior.

🔴 3 blocking | 🟡 3 suggestion(s)

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: ffi-engineer); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: platform-versioning); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

• Triage: critical by gpt-6-astra (effort low) — The large, intricate diff changes critical wallet storage migrations and rehydration/accounting logic that affects spend-state restoration, funds movement, and coin selection.
• 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 — ffi-engineer (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — platform-versioning (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — security-auditor (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 — final-verifier; agent astra-verifier
• Phase 2 reviewers: gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — 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/rehydrate.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:413-458: Replay does not restore InstantSend context from persisted locks
  The persisted InstantSend locks are applied at lines 413-415 before `restore_recorded_transactions` replays any transaction records. `mark_instant_send_utxos` can only update a record that already exists; at this point it merely records the txid in the lock set. The replay then passes each stored record's unchanged `record.context` to `check_core_transaction`, so a record originally stored as `Mempool` remains a mempool record rather than becoming `InstantSend`. Because the lock txid is already present in the lock set, a later call to `mark_instant_send_utxos` is deduplicated and does not repair the record context or run the InstantSend conflict sweep. After restart, the transaction can therefore remain evictable and conflicting outputs can remain incorrectly credited. Apply the locks after records have been replayed, or make the replay resolve each record's context from the persisted lock map before checking it.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:526-555: Replay all records in dependency order, not confirmed records before pending records
  `replay_order` always emits every record with `block_info()` before any pending record, and only topologically sorts the pending partition. A confirmed child can therefore be replayed before a stale mempool parent. This state is reachable because a height-only confirmation update intentionally does not replace an existing blob-bearing mempool record (`core_state::apply` has `WHERE core_transactions.record_blob IS NULL`), while the child can have a confirmed record. The child is checked before its parent output exists, so its input is not attributed or reserved; replaying the parent later as mempool does not perform a final spend reservation. Use a dependency-aware ordering over the complete record set, while retaining deterministic block-position ordering where there is no dependency.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:528-555: Add direct tests for replay ordering invariants
  `replay_order` now encodes the spend-reservation ordering invariant, but the tests only exercise selected end-to-end cases. There is no direct regression test for a child-first unconfirmed chain, a confirmed-position tie-break, or the cycle fallback. A focused deterministic unit test would make a future refactor less likely to reintroduce an ordering that loses reservations during load.

In `packages/rs-platform-wallet-storage/src/sqlite/migrations/legacy_v019.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/migrations/legacy_v019.rs:83-109: Create a sync-state row when forcing a rescan
  `drop_for_resync` deletes the corrupt confirmed record and then uses an `UPDATE ... WHERE wallet_id = ?1` to lower `synced_height`. SQLite reports success when that UPDATE matches zero rows, so a wallet without a `core_sync_state` row proceeds as though the rescan was scheduled even though no checkpoint was created. Such wallets are possible: wallet creation does not create a sync-state row, and `core_state::apply` only upserts one when a changeset carries a height or chain lock, while transaction records may be stored without those fields. The migration can therefore delete the only record that can restore the transaction and return success without arranging for Core to redeliver it. Use an upsert that inserts the wallet's rescan checkpoint when the row is absent.
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/migrations/legacy_v019.rs:68-90: Do not treat legacy height 0 as a confirmed record
  V019 currently treats any `Some(height)` as block-confirmed. However, the V014 migration explicitly documents that pre-V014 writers encoded unconfirmed records with `height = 0`; only positive heights identify a confirmation, while post-V014 unconfirmed rows use NULL. A corrupt pending record from a pre-V014 database therefore enters `drop_for_resync`, gets deleted along with its input index, and the migration succeeds even though an SPV rescan cannot redeliver a mempool transaction. Its reservation can then be lost and the input can become selectable for a conflicting spend. The rescan path must require a strictly positive height.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/migrations/legacy_v019.rs:30-47: Check the record length before fetching the BLOB
  `read_record` selects both `length(record_blob)` and `record_blob`, and the query closure materializes the payload with `row.get(1)?` before `blob::check_size(len)` runs. An oversized or corrupt record is therefore copied into a `Vec<u8>` before the migration rejects it. This bypasses the pre-materialization size guard used by the normal SQLite readers and permits an allocation larger than the 16 MiB blob budget, subject only to the connection's SQLite limit. Read and validate the length first, then fetch the payload in a second query.

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

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.

• Release repair spend claims after reorg — Repaired confirmed history sets spent_in_txid, but the current design does not release that claim when the spender later leaves the active chain. This can strand a legitimately spendable UTXO after a reorg; the PR already marks this as deferred and it requires verifying upstream downgrade semantics.
  • Follow-up: Track a separate reorg-reconciliation issue before enabling release of repaired spend claims.
• Bound startup replay of persisted history — The new load path replays all persisted records on every startup, so a large amount of wallet-relevant history can make startup work grow without bound. The PR explicitly defers this because pruning records must not remove finality or spend guards.
  • Follow-up: Design a checkpointed or chain-lock-bounded replay strategy that preserves guards for older records.
• Swift redefines the wallet direction classifier — Out of scope — the PR deliberately keeps the Swift and SQLite persistence rules separate, adds mirrored case-table tests, and explicitly excludes an upstream shared-classifier change.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
• Core transaction replay is owned by the SQLite adapter — Out of scope architecture redesign — this PR's SQLite persister is the backend that decodes persisted records and replays them, while the separate FFI startup path has different inputs; no incorrect result was demonstrated for the current supported backends.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
• Upstream sync checker API lives in rust-dashcore/key-wallet — Out of scope — this is an upstream API improvement rather than a defect in the PR, and the current checker is intentionally verified to complete on its first poll.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.