fix(platform): reconcile confirmed UTXO height after wallet restart#4199
Conversation
…re_transactions on load core_transactions contains authoritative confirmation metadata, while confirmation-only transaction record updates do not rewrite core_utxos. Reconcile matching UTXO heights from the transaction table during load, retaining the core_utxos fallback when no transaction record exists. Add regression coverage for mempool persistence followed by a record-only confirmation update. Related: #4178. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Codex Sol <noreply@openai.com>
…derive solely from core_transactions core_utxos.height duplicated a fact core_transactions.height already owns authoritatively, and only the latter was ever kept fresh on a confirmation-only update — the exact staleness that caused #4178. Removing the column (V007) makes core_transactions the single source for UTXO confirmation height: load_state() now derives it unconditionally from the transaction record, defaulting to unconfirmed when no matching record exists (a UTXO-only changeset is legal via apply(), so this path is real, not defensive dead code — covered by a new regression test). The write path (UPSERT_UTXO_SQL, execute_upsert_utxo) no longer touches height at all, and list_unspent_utxos/UnspentRow drop the field along with it. A schema guard (tc030_core_utxos_height_column_removed) asserts the column is actually gone via PRAGMA table_info, so this bug class can't silently resurface. Golden schema-freeze fingerprints bumped deliberately for the new migration, per this crate's own pinning-test convention. Related: #4178. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Codex Sol <noreply@openai.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
…ydration' into fix/4178-utxo-height-storage-reconcile # Conflicts: # packages/rs-platform-wallet-storage/SCHEMA.md # packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs # packages/rs-platform-wallet-storage/tests/sqlite_blob_size_gate_on_load.rs # packages/rs-platform-wallet-storage/tests/sqlite_foreign_keys.rs # packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs # packages/rs-platform-wallet-storage/tests/sqlite_money_column_overflow_on_read.rs # packages/rs-platform-wallet-storage/tests/sqlite_schema_pinning.rs
|
🕓 Ready for review — 16 ahead in queue (commit 9000d38) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The transaction-backed reconciliation correctly fixes the stale mempool-to-confirmed path when a matching transaction record exists. However, the new schema and loader silently downgrade confirmed UTXO-only changesets and migrated recordless UTXOs to unconfirmed, contradicting an existing all-features restart test and excluding those funds from final-input transaction building. The new authoritative transaction columns should also be cross-checked against their serialized records.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:340-354: Recordless confirmed UTXOs lose confirmation across restart
The single-source path fixes stale heights when `core_transactions` contains the matching record, but `apply()` still accepts a `CoreChangeSet` containing only `new_utxos`. The new writer no longer persists `utxo.height`, and these lines have no fallback when the transaction map lacks the txid, so a confirmed height-100 UTXO reloads with height 0 and `is_confirmed = false`. V009 also drops the previous fallback height from existing recordless rows. The unchanged `rt2_nonzero_balance_survives_reopen` integration test constructs exactly this legal input and, with `rehydration-apply` enabled by `--all-features`, expects the confirmed balance to remain 1,234,500; this implementation produces zero confirmed balance. This is functional, not merely cosmetic: key-wallet's transaction builder removes such UTXOs when `require_final_inputs` is enabled. Preserve confirmation in an authoritative normalized structure, retain a safe fallback, or reject and migrate/backfill confirmed UTXO-only state before removing the column.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:297-315: Validate authoritative transaction metadata against record_blob
The confirmation map is built from the typed `txid` and `height` columns, while the later transaction-record query decodes only `record_blob` and never compares the decoded record's txid or block height with those columns. A validly encoded but semantically inconsistent row can therefore mark a UTXO for the typed txid as confirmed while returning a transaction record for another txid or context. Since this PR makes these columns authoritative for UTXO confirmation and neighboring schema readers fail hard on typed-column/blob disagreement, decode and cross-check the corresponding `TransactionRecord` before accepting its typed confirmation metadata.
| let height = transaction_heights.get(&outpoint.txid).copied().flatten(); | ||
| let script = dashcore::ScriptBuf::from_bytes(script_bytes); | ||
| if let Some(owner) = owning_account_for_script(conn, wallet_id, script.as_bytes())? { | ||
| utxo_accounts.insert(outpoint, owner); | ||
| } | ||
| let address = dashcore::Address::from_script(&script, network)?; | ||
| let confirmed = height.map(|h| h > 0).unwrap_or(false); | ||
| let confirmed = height.map(|height| height > 0).unwrap_or(false); | ||
| let utxo = Utxo { | ||
| outpoint, | ||
| txout: dashcore::TxOut { | ||
| value, | ||
| script_pubkey: script, | ||
| }, | ||
| address, | ||
| height: height_u32, | ||
| height: height.unwrap_or(0), |
There was a problem hiding this comment.
🔴 Blocking: Recordless confirmed UTXOs lose confirmation across restart
The single-source path fixes stale heights when core_transactions contains the matching record, but apply() still accepts a CoreChangeSet containing only new_utxos. The new writer no longer persists utxo.height, and these lines have no fallback when the transaction map lacks the txid, so a confirmed height-100 UTXO reloads with height 0 and is_confirmed = false. V009 also drops the previous fallback height from existing recordless rows. The unchanged rt2_nonzero_balance_survives_reopen integration test constructs exactly this legal input and, with rehydration-apply enabled by --all-features, expects the confirmed balance to remain 1,234,500; this implementation produces zero confirmed balance. This is functional, not merely cosmetic: key-wallet's transaction builder removes such UTXOs when require_final_inputs is enabled. Preserve confirmation in an authoritative normalized structure, retain a safe fallback, or reject and migrate/backfill confirmed UTXO-only state before removing the column.
source: ['codex']
| let mut transaction_heights: HashMap<dashcore::Txid, Option<u32>> = HashMap::new(); | ||
| { | ||
| use dashcore::hashes::Hash; | ||
|
|
||
| let mut stmt = conn.prepare_cached( | ||
| "SELECT length(txid), txid, height FROM core_transactions WHERE wallet_id = ?1", | ||
| )?; | ||
| let mut rows = stmt.query(params![wallet_id.as_slice()])?; | ||
| while let Some(row) = rows.next()? { | ||
| blob::check_fixed_width(row.get::<_, i64>(0)?, 32, "core_transactions.txid")?; | ||
| let txid_bytes: Vec<u8> = row.get(1)?; | ||
| let txid = dashcore::Txid::from_slice(&txid_bytes)?; | ||
| let height = row | ||
| .get::<_, Option<i64>>(2)? | ||
| .map(|height| { | ||
| crate::sqlite::util::safe_cast::i64_to_u32("core_transactions.height", height) | ||
| }) | ||
| .transpose()?; | ||
| transaction_heights.insert(txid, height); |
There was a problem hiding this comment.
🟡 Suggestion: Validate authoritative transaction metadata against record_blob
The confirmation map is built from the typed txid and height columns, while the later transaction-record query decodes only record_blob and never compares the decoded record's txid or block height with those columns. A validly encoded but semantically inconsistent row can therefore mark a UTXO for the typed txid as confirmed while returning a transaction record for another txid or context. Since this PR makes these columns authoritative for UTXO confirmation and neighboring schema readers fail hard on typed-column/blob disagreement, decode and cross-check the corresponding TransactionRecord before accepting its typed confirmation metadata.
source: ['codex']
3 participants
TL;DR: Fixes identity/asset-lock funding failing with "No UTXOs available" after a wallet restart, by making UTXO confirmation height single-sourced instead of duplicated and prone to going stale.
User story
As a Dash Platform wallet user, I want a Core-funded UTXO that confirmed while the wallet was running to still be recognized as confirmed after I restart the app, so that topping up an identity or funding an asset lock doesn't spuriously fail.
Scenario
Base flow
A wallet detects an incoming transaction while it's still in the mempool, tracks it, and later sees that same transaction confirm in a block — all while the app keeps running.
Actual behavior
On the very next app restart, that same UTXO reloads from disk as if it were still unconfirmed — even though it's genuinely confirmed on-chain — because its confirmation status was stored in two separate places that don't stay in sync, and only one of them was ever kept fresh. Anything requiring a final (confirmed or instant-locked) UTXO, such as an identity top-up or an asset-lock funding, then fails with "No UTXOs available" for funds that are, in fact, spendable. See #4178.
Expected behavior
A UTXO's confirmation status survives a restart intact, sourced from exactly one place, so it can never drift out of sync with reality again.
Detailed discussion
What was done
Stacked on #3968 (
feat/platform-wallet-storage-rehydration) — this closes a gap in that PR's own rehydration path. Two commits:fix(platform-wallet-storage): reconcile confirmed UTXO height from core_transactions on load—load_state()now preferscore_transactions.height(kept fresh on every write) over the separate, staleness-pronecore_utxos.heightcolumn when reconstructing a UTXO's confirmation status.refactor(platform-wallet-storage): drop redundant core_utxos.height, derive solely from core_transactions— removescore_utxos.heightentirely (migrationV009— renumbered fromV007after merging in feat(platform-wallet-storage): embeddable SQLite persistence backend with seedless rehydration #3968's ownV007/V008migrations, which landed independently in the meantime), so the fact can no longer be stored twice.core_transactions.heightbecomes the sole source; a UTXO with no matching transaction record (a UTXO-only changeset is legal viaapply()) safely defaults to unconfirmed rather than erroring. A schema guard test (tc031_core_utxos_height_column_removed) asserts the column is actually gone, so this bug class can't silently resurface.This is the
rs-platform-wallet-storage-only portion of the fix.rs-platform-wallet'score_bridge.rsintentionally still routes confirmation-only updates throughrecordsrather thannew_utxos(by design — a confirmation doesn't change UTXO topology); that file is untouched and out of scope here.This branch also merged in the latest
feat/platform-wallet-storage-rehydration(#3968), which independently droppedcore_utxos.account_index/spent_in_txidin its ownV007migration — that combined cleanly with this PR'sV009height removal incore_state.rs.Related: #4178.
Testing
cargo test -p platform-wallet-storage --all-features— 289 passed / 0 failed, including two regression tests (load_state_reconciles_utxo_height_from_confirmed_transaction_record,load_state_defaults_utxo_without_transaction_record_to_unconfirmed) and a schema guard (tc031_core_utxos_height_column_removed).cargo clippy -p platform-wallet-storage --all-features -- -D warnings— clean.cargo fmt -p platform-wallet-storage— clean.Breaking changes
None outside this unreleased, unmerged branch —
core_utxos.heightnever shipped in any release. The schema change lands via an additive migration (V009) with a golden schema-freeze fingerprint bump, consistent with this crate's existing migration discipline.Checklist
For repository code-owners and collaborators only
Prior work
feat/platform-wallet-storage-rehydration); fixes a gap discovered in that PR's own rehydration path.Attribution
🤖 Co-authored by Claudius the Magnificent AI Agent