fix(platform-wallet): emit the real locking script for spent UTXOs

[Claudius-Maginificent]

TL;DR: When the wallet records a coin it has spent, it stored a blank placeholder where the coin's real locking script belongs. Storage backends write that placeholder to disk, and reading it back later can fail in a way that rejects an entire wallet file — in one real database, a single such record made twelve wallets impossible to open. This change records the genuine script, which the wallet already had in hand.

User story

As a Dash Platform wallet developer, I want the wallet's saved record of a spent coin to reflect what was actually on-chain, so that one unreadable record cannot stop my users' whole wallet file from opening.

Scenario

Base flow

A wallet spends some of its coins. It records what was spent, so that spend history and address-reuse tracking survive a restart.

Actual behavior

The record of a spent coin carries a blank placeholder instead of the coin's real locking script — even though the wallet already knows the address that owned it. A blank script is not a marker meaning "unknown"; it is a legitimate value in its own right, so nothing downstream can tell the placeholder apart from a genuine observation. A storage backend that keeps these records writes the blank value to disk. Reading it back fails, and that failure is not confined to the offending record: it rejects every wallet in the file. One such record in a real database made twelve wallets unopenable.

Expected behavior

The record carries the coin's genuine locking script, rebuilt exactly from the address the wallet already holds. Records written from now on read back cleanly, and address-reuse tracking receives the address it exists to recover.

Detailed discussion

Issue being fixed

derive_spent_utxos built every spent-UTXO record with script_pubkey: ScriptBuf::default() — an empty script the wallet never observed. TxOut belongs to rust-dashcore, which documents the field as "The script which must be satisfied for the output to be spent": a total field with no encoding for absence. In Bitcoin/Dash an empty script is a legitimate on-chain locking script, indistinguishable from "unknown", so the record misrepresented itself to every consumer.

The correct value was available all along. InputDetail.address is non-optional and is lifted from a UTXO already in the wallet's set; the function cloned it into Utxo.address two lines below the fabrication.

SqlitePersister does not discard the field: where no row exists it inserts a synthetic one, committing the empty script (schema/core_state.rs:143-160). load_used_addresses later decodes every stored script — spent and unspent — through Address::from_script(..)?; an empty script returns UnrecognizedScript and the ? rejects the entire store, not the offending row. One such row made twelve wallets unloadable in a real database. The synthetic row exists solely to feed the address-reuse guard, which recovers the address from the script — so the placeholder destroyed precisely what the row is for.

The prior doc comment justified the placeholder on the grounds that "the persister deletes by outpoint so the missing fields are informational only". No in-tree persister deletes by outpoint, so that licence did not hold; the comment is replaced rather than merely amended, since it is what allowed the fabrication to look reasonable.

What was done

One value changed: script_pubkey: ScriptBuf::default() → script_pubkey: detail.address.script_pubkey(), plus the now-unused ScriptBuf import and a rewritten doc comment stating why the reconstruction is exact.

The reconstruction is exact. Every production path derives a UTXO's address from its script, gating on that decode succeeding — chiefly the on-chain ingestion path at key-wallet .../managed_core_funds_account.rs:244 — so a script that cannot decode never enters a UTXO set, and Address::from_script → script_pubkey() round-trips byte-for-byte for P2PKH, P2SH and witness programs (all strict canonical templates). That consistency is call-site behaviour, not something Utxo enforces: Utxo::new takes txout and address as independent parameters and validates neither, and all fields are pub. The added test pins the property that licenses this fix, so a future path that sets the two independently fails loudly instead of silently invalidating it.

Two limits worth stating plainly: this corrects new writes only — rows already on disk keep their empty scripts — and it changes persisted bytes, so fixtures asserting the old empty value need updating.

Known and deliberately not addressed here: the loss originates upstream. InputDetail is a lossy projection built from a full Utxo whose txout it discards, and key_wallet::Utxo is documented "Unspent Transaction Output" — no field of it has a slot for absence, which is why is_coinbase / is_confirmed / is_trusted are asserted rather than known. Carrying the script in InputDetail would remove the need for downstream reconstruction entirely, but that is an upstream change with a far wider blast radius (every InputDetail consumer, and any serialized form) and does not belong in this fix.

How Has This Been Tested?

• New regression test spent_utxos_carry_the_real_script_of_the_address_they_spend covers P2PKH and P2SH, asserting three properties per input: the script is non-empty, it equals the address's own locking script, and Address::from_script on the emitted script returns the input's own address. Proven to fail before the fix for the correct reason — panicking on its own assertion ("a spent UTXO must never carry a fabricated empty script"), not on unrelated setup.
• cargo test -p platform-wallet — 503 passed / 0 failed / 1 ignored across 7 binaries; the lib target is 494 (493 baseline + 1 new).
• cargo fmt -p platform-wallet clean.
• cargo clippy -p platform-wallet --all-targets -- -D warnings fails on this base, before and after this change — 3 errors, none citing core_bridge.rs, reproduced identically on the stashed unmodified tree: wallet/identity/network/withdrawal.rs:229 and :268 (clippy::useless_vec), wallet/asset_lock/sync/recovery.rs:585 (clippy::await_holding_lock). Left alone deliberately rather than widening this PR's scope. Worth noting that the third is the same lint already fixed on feat/platform-wallet-storage-rehydration — v4.2-dev is carrying a lint failure that branch has resolved, so it needs either its own fix or that merge.

The round-trip argument above was verified against rust-dashcore 70d4bf8e, the revision this branch's Cargo.toml actually pins — not the newer revision pinned by #3968.

Breaking changes

None to any public signature. The change is behavioural: spent-UTXO records now carry a real locking script where they previously carried an empty one. Consumers treating the empty value as meaningful — including test fixtures asserting it — need updating; no such consumer exists in-tree.

Related

The consumer-side hardening that stops one undecodable row from rejecting an entire wallet file is separate, and lands in #3968. Neither change subsumes the other, and the layering is deliberate: this PR stops bad rows being produced; #3968 stops existing ones being fatal.

Once this lands, #3968's script_pubkey.is_empty() skip stops being taken in normal operation — the script is never empty. It does not become dead code: it still guards rows already written to disk before this fix, and any future producer that regresses. Belt and braces, in that order — the storage layer must not depend on the producer being correct.

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

Summary by CodeRabbit

• Bug Fixes

  • Corrected spent transaction output reconstruction to preserve the appropriate locking scripts.
  • Improved handling for P2PKH, P2SH, and witness-program wallet addresses, ensuring transaction details remain accurate.
  • Preserved expected default status information for reconstructed spent outputs.

• Tests

  • Added regression coverage confirming locking scripts round-trip correctly for supported wallet address types.
  • Added integration coverage validating reconstructed scripts against the original funding output.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b3c16711-b7b4-42ba-97f2-3bfe8bd97138

📥 Commits

Reviewing files that changed from the base of the PR and between f9d152c and 3ef85a7.

📒 Files selected for processing (1)

• packages/rs-platform-wallet/src/changeset/core_bridge.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• packages/rs-platform-wallet/src/changeset/core_bridge.rs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Spent UTXO conversion now reconstructs locking scripts from wallet addresses. Tests verify funding-output equality and P2PKH and P2SH script round trips.

Changes

Spent UTXO script reconstruction

Layer / File(s)	Summary
Address-derived locking scripts and regression coverage packages/rs-platform-wallet/src/changeset/core_bridge.rs	Spent UTXOs use detail.address.script_pubkey() instead of an empty script. Documentation records the supported address forms and default height and confirmation metadata. Tests verify that derived scripts match funding outputs and round-trip for P2PKH and P2SH addresses.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 3ef85

This localized change records the correct locking script for spent coins, with no actionable merge-blocking risk remaining after normal checks and review.

Suggested reviewers: lklimek, llbartekll, quantumexplorer

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: emitting the real locking script for spent UTXOs.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/platform-wallet-spent-utxo-real-script

---

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 — no blockers (commit 3ef85a7)

[thepastaclaw]

Final validation — Codex + Sonnet

This PR fixes a real data-integrity bug: derive_spent_utxos previously wrote ScriptBuf::default() for spent UTXOs' script_pubkey while an authoritative address was available two lines below, producing unusable/misleading persisted rows. The fix reconstructs the script from InputDetail.address, and I verified against the exact pinned rust-dashcore rev (70d4bf8e) that Address::script_pubkey() reconstructs the original bytes for the canonical forms in play. No blocking issues found. Two accurate, non-blocking findings from Codex remain: the new regression test constructs InputDetail directly from the same address it later checks against, so it doesn't exercise the real ingestion path where the address/script pairing could actually diverge; and the new doc comment incorrectly claims Address::from_script only accepts P2PKH/P2SH when the pinned dependency also handles witness programs.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed)
• Verifier: claude-sonnet-5 — final-verifier
• Sonnet reviewers: claude-sonnet-5 — general (failed), claude-sonnet-5 — general (completed)

🟡 1 suggestion(s) | 💬 1 nitpick(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/src/changeset/core_bridge.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:1010-1032: Regression test doesn't exercise the real address/script pairing it's meant to guard
  The new test builds each InputDetail directly with the same Address object it later asserts against, so it only proves derive_spent_utxos calls Address::script_pubkey() — it never routes through key_wallet's actual funding-then-spend path (check_core_transaction), where InputDetail.address is independently derived via Address::from_script on the real previous TxOut. The doc comment explicitly calls out that the address/script pairing is a caller convention, not a type invariant (Utxo::new validates neither), which is exactly the scenario this test should catch if it ever breaks. The file already has a working pattern for this (spent_input_address_is_captured_after_utxo_removal, lines 858-927) that funds a real UTXO and spends it; extending that flow to also assert the derived spent UTXO's script_pubkey matches the original funding output's script_pubkey would close the gap and test the invariant end-to-end rather than tautologically.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[thepastaclaw]

Final validation — Codex/Sol only (Phase 2 disabled)

The script reconstruction and new funding-then-spend regression test are correct, and both prior findings are fixed. One in-scope documentation nit remains: the comment promises that default metadata re-warms on the next sync even though the spent-record path provides no such refresh.
Source: reviewer backends gpt-5.6-sol (general and rust-quality); final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.

Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — rust-quality (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
• Secondary pass: disabled (temporary_phase2_sonnet_disable)

💬 1 nitpick(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/src/changeset/core_bridge.rs`:
- [NITPICK] packages/rs-platform-wallet/src/changeset/core_bridge.rs:1090-1093: Comment promises a metadata refresh that the spent path does not perform
  The statement that these default values are interpreted as unknown and "re-warm on the next sync" is inaccurate. `Utxo` represents the flags as ordinary booleans rather than optional unknown values, and this spent record has no refresh path. The SQLite persister either marks an existing outpoint as spent without updating its metadata or inserts a synthetic spent row, which is then excluded from `list_unspent_utxos`; a routine subsequent sync does not revisit the previous funding transaction. Describe the defaults as metadata unavailable to this projection rather than promising that they will be recovered.

[thepastaclaw]

Final validation — Codex/Sol only (Phase 2 disabled)

The change correctly reconstructs spent-UTXO locking scripts from the retained address, and the funding-then-spend test verifies the result through the real wallet ingestion path. The prior metadata-refresh claim is fixed; only a stale unit-test comment remains because it credits the direct-construction test with enforcing an invariant that the integration test above actually covers. Source: reviewers gpt-5.6-sol (general and rust-quality); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.

Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — rust-quality (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
• Secondary pass: disabled (temporary_phase2_sonnet_disable)

💬 1 nitpick(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/src/changeset/core_bridge.rs`:
- [NITPICK] packages/rs-platform-wallet/src/changeset/core_bridge.rs:1848-1853: Unit-test comment credits the wrong test with enforcing the caller invariant
  The comment says `InputDetail.address` was cloned from a wallet UTXO and that this test keeps the address/script pairing true, but this unit test constructs every `InputDetail` directly from the same hand-built address it later checks. It therefore verifies only the local P2PKH/P2SH reconstruction. The funding-then-spend test at lines 1781-1834 is what exercises the production caller convention by deriving the input address through the wallet path and comparing the result with the original funding output.

[thepastaclaw]

💬 Nitpick: Unit-test comment credits the wrong test with enforcing the caller invariant

The comment says InputDetail.address was cloned from a wallet UTXO and that this test keeps the address/script pairing true, but this unit test constructs every InputDetail directly from the same hand-built address it later checks. It therefore verifies only the local P2PKH/P2SH reconstruction. The funding-then-spend test at lines 1781-1834 is what exercises the production caller convention by deriving the input address through the wallet path and comparing the result with the original funding output.

Suggested change

	/// The reconstruction is exact: `InputDetail.address` is cloned from the
	/// wallet's own `Utxo.address`, which key-wallet derived from that
	/// output's script via `Address::from_script`, and `is_p2pkh`/`is_p2sh`
	/// accept only the canonical form — so `script_pubkey()` rebuilds the same
	/// bytes. This test is what keeps that true, since `Utxo::new` does not
	/// enforce the address/script pairing.
	/// This unit test covers the local reconstruction for canonical P2PKH and
	/// P2SH addresses. The funding-then-spend test above covers the caller
	/// convention that `InputDetail.address` matches the original UTXO script,
	/// since `Utxo::new` does not enforce that pairing.

_{source: ['codex']}