fix(platform-wallet): emit the real locking script for spent UTXOs - #4257
fix(platform-wallet): emit the real locking script for spent UTXOs#4257Claudius-Maginificent wants to merge 1 commit into
Conversation
`derive_spent_utxos` filled `TxOut.script_pubkey` with `ScriptBuf::default()` while cloning the authoritative address two lines below. `TxOut.script_pubkey` is documented upstream as "The script which must be satisfied for the output to be spent" — a total field with no encoding for absence — so a default there is a claim about the chain that was never observed. Rebuild it from the address the function already holds. The reconstruction is exact, not a guess. `InputDetail.address` is cloned from the wallet's own `Utxo.address` (managed_core_funds_account.rs:398), which key-wallet derived from the spent output's `script_pubkey` via `Address::from_script` on the receive path. `is_p2pkh` and `is_p2sh` accept only the canonical form — fixed length with every structural opcode pinned, the 20-byte hash the sole free field, copied verbatim in both directions — so re-encoding the address reproduces the original bytes. Verified against rust-dashcore rev 70d4bf8e, the rev this branch pins. The address/script pairing is a caller convention, not a type invariant: `Utxo::new` takes both as independent parameters and validates neither. The new test is what keeps the convention honest. Behavioural consequence, intended and reviewed: spent rows now carry a resolvable script, so consumers that read stored scripts — notably the address-reuse guard being added in #3968 — gain entries that never existed before. A previously-used address whose funds were since spent is no longer re-issued as a fresh receive address. This is an expansion of reuse-guard coverage and is why it lands here on v4.2-dev as its own change rather than inside that PR's merge window. Height and the confirmation flags stay defaulted; unlike the script they are genuinely unrecoverable here and are read as "not yet known". Test: spent_utxos_carry_the_real_script_of_the_address_they_spend. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSpent UTXO conversion now reconstructs locking scripts from wallet addresses. Regression coverage verifies P2PKH and P2SH scripts. ChangesSpent UTXO script reconstruction
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
|
✅ Final review complete — no blockers (commit 9d77f41) |
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
| let spent = super::derive_spent_utxos(&record); | ||
| assert_eq!(spent.len(), addresses.len()); | ||
| for (utxo, address) in spent.iter().zip(addresses.iter()) { | ||
| assert!( | ||
| !utxo.txout.script_pubkey.is_empty(), | ||
| "a spent UTXO must never carry a fabricated empty script" | ||
| ); | ||
| assert_eq!( | ||
| utxo.txout.script_pubkey, | ||
| address.script_pubkey(), | ||
| "the script must be the address's own locking script" | ||
| ); | ||
| assert_eq!( | ||
| dashcore::Address::from_script( | ||
| &utxo.txout.script_pubkey, | ||
| dashcore::Network::Testnet | ||
| ) | ||
| .expect("the emitted script must decode as an address"), | ||
| *address, | ||
| "the script must round-trip back to the input's own address" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: 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.
source: ['codex']
| /// The script is an exact reconstruction, not a guess. `InputDetail.address` | ||
| /// is cloned from the wallet's own `Utxo.address`, which key-wallet derived | ||
| /// from the spent output's `script_pubkey` via `Address::from_script`; that | ||
| /// decoder accepts only canonical P2PKH/P2SH forms, so re-encoding the | ||
| /// address reproduces the original bytes. Note the pairing is a caller | ||
| /// convention rather than a type invariant — `Utxo::new` takes the script | ||
| /// and the address as independent parameters and validates neither. |
There was a problem hiding this comment.
💬 Nitpick: Doc comment overstates the decoder's format restriction
The comment states Address::from_script's decoder "accepts only canonical P2PKH/P2SH forms." I checked the pinned rust-dashcore rev (70d4bf8e, dash/src/address.rs Payload::from_script) directly, and it also matches script.is_witness_program() and constructs Payload::WitnessProgram, which Address::script_pubkey() round-trips just like P2PKH/P2SH. The reconstruction argument in this comment still holds for witness outputs, so this is purely a factual accuracy issue in the doc, not a correctness bug — but since the PR's whole justification for the fix leans on precise round-trip reasoning about this decoder, the comment should describe the decoder accurately.
| /// The script is an exact reconstruction, not a guess. `InputDetail.address` | |
| /// is cloned from the wallet's own `Utxo.address`, which key-wallet derived | |
| /// from the spent output's `script_pubkey` via `Address::from_script`; that | |
| /// decoder accepts only canonical P2PKH/P2SH forms, so re-encoding the | |
| /// address reproduces the original bytes. Note the pairing is a caller | |
| /// convention rather than a type invariant — `Utxo::new` takes the script | |
| /// and the address as independent parameters and validates neither. | |
| /// decoder accepts canonical P2PKH, P2SH, and witness-program forms, so | |
| /// re-encoding the address reproduces the original bytes. Note the pairing | |
| /// is a caller |
source: ['codex']
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_utxosbuilt every spent-UTXO record withscript_pubkey: ScriptBuf::default()— an empty script the wallet never observed.TxOutbelongs 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.addressis non-optional and is lifted from a UTXO already in the wallet's set; the function cloned it intoUtxo.addresstwo lines below the fabrication.SqlitePersisterdoes 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_addresseslater decodes every stored script — spent and unspent — throughAddress::from_script(..)?; an empty script returnsUnrecognizedScriptand 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
outpointso 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-unusedScriptBufimport 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, andAddress::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 somethingUtxoenforces:Utxo::newtakestxoutandaddressas independent parameters and validates neither, and all fields arepub. 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.
InputDetailis a lossy projection built from a fullUtxowhosetxoutit discards, andkey_wallet::Utxois documented "Unspent Transaction Output" — no field of it has a slot for absence, which is whyis_coinbase/is_confirmed/is_trustedare asserted rather than known. Carrying the script inInputDetailwould remove the need for downstream reconstruction entirely, but that is an upstream change with a far wider blast radius (everyInputDetailconsumer, and any serialized form) and does not belong in this fix.How Has This Been Tested?
spent_utxos_carry_the_real_script_of_the_address_they_spendcovers P2PKH and P2SH, asserting three properties per input: the script is non-empty, it equals the address's own locking script, andAddress::from_scripton 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-walletclean.cargo clippy -p platform-wallet --all-targets -- -D warningsfails on this base, before and after this change — 3 errors, none citingcore_bridge.rs, reproduced identically on the stashed unmodified tree:wallet/identity/network/withdrawal.rs:229and: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 onfeat/platform-wallet-storage-rehydration—v4.2-devis 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-dashcore70d4bf8e, the revision this branch'sCargo.tomlactually 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
Tests