feat: special transactions - #5
Merged
Merged
Conversation
…shcore into feat/specialTransactions
shumkov
reviewed
Aug 8, 2022
shumkov
reviewed
Aug 8, 2022
shumkov
reviewed
Aug 8, 2022
| /// A Dash devnet | ||
| Devnet <-> "devnet", | ||
| /// Dash's regtest | ||
| Regtest <-> "regtest" |
Collaborator
There was a problem hiding this comment.
What means this syntax, can't find anything similar https://doc.rust-lang.org/reference/items/enumerations.html
Contributor
There was a problem hiding this comment.
Hm, that's an interesting thing indeed. Probably some deprecated syntax? This code was originally written for the very first version of the compiler
Contributor
There was a problem hiding this comment.
I think this syntax is not part of the language but has been introduced by wrapping macro user_enum!
QuantumExplorer
marked this pull request as ready for review
August 9, 2022 15:59
antouhou
reviewed
Aug 9, 2022
| use hashes::sha256d; | ||
| use blockdata::opcodes; | ||
| use blockdata::script; | ||
| use blockdata::transaction::{OutPoint, Transaction, TxOut, TxIn}; |
Contributor
There was a problem hiding this comment.
What was the problem with use blockdata::transaction::{OutPoint, Transaction, TxOut, TxIn};?
Member
Author
There was a problem hiding this comment.
They got moved to their own files.
antouhou
reviewed
Aug 9, 2022
| assert_eq!(out_point.vout, 0); | ||
| assert_eq!(out_point.txid, tx.txid()); | ||
| } | ||
| } No newline at end of file |
Contributor
There was a problem hiding this comment.
Please don't forget about the newlines
antouhou
reviewed
Aug 9, 2022
| use blockdata::block::Block; | ||
| use blockdata::script::Script; | ||
| use blockdata::transaction::OutPoint; | ||
| use blockdata::transaction::outpoint::OutPoint; |
Contributor
There was a problem hiding this comment.
What do you thing about re-exporting the OutPoint, so the lib external interface stays the same? I.e. pub use outpoint::OutPoint
shumkov
reviewed
Aug 10, 2022
| } | ||
|
|
||
| #[cfg(test)] | ||
| #[cfg(feature="signer")] |
Member
Author
There was a problem hiding this comment.
Yes, because the test requires signing.
romchornyi
pushed a commit
that referenced
this pull request
Aug 13, 2026
…spent-marks **#6 — abandon could delete network-settled money.** `transaction_is_settled` and the cascade walk both gated on `is_confirmed()`, which excludes `InstantSend`. An IS lock is final against a double spend under DIP-10, so an IS-locked root passed the guard and IS-locked descendants were followed; their inputs were then released and a rescan could re-credit coins the network has irreversibly moved. Both now treat a lock as settled. **#5 — a plain block deleted an IS-locked record.** The sweep's filter admitted any `!is_confirmed()` record, and its gate accepts a non-chainlocked `InBlock`, so an ordinary tip delivery unrecoverably dropped a record the network had locked — with no reorg recovery anywhere in the stack. Precedence is now explicit: a chainlock overrides anything, and an IS-locked loser may only be evicted by a chainlocked arrival. **#4 — the walk was not wallet-wide despite saying so.** It iterated funding accounts only, skipping keys-only accounts. An asset-lock funding transaction is recorded in *both* its funding account and the identity account it pays, so abandoning it left the keys-account copy behind — which makes `is_new` false on re-sighting, so the funds account never re-records it and never re-marks its input spent. The walk and the removal now span every account that holds records. **#7 — the rebuild erased chainlock-pruned marks.** This PR dropped the `cfg` gate on `rebuild_spent_outpoints` and added runtime callers, but it derives only from live records — and under the default `keep-finalized-transactions = off` a chainlocked spend keeps only its txid, so its inputs live solely as marks already in the set. A wholesale reassignment discarded them, letting a later backfill re-credit coins spent on chain. Replaced with a targeted retain over only the outpoints the removed records contributed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
QuantumExplorer
pushed a commit
that referenced
this pull request
Aug 14, 2026
…ney that does not exist (#961) * fix(key-wallet): resolve trusted self-sends across the whole wallet `update_utxos` decided "are all these inputs ours and final" by looking only at `self.utxos` — the UTXOs of the single account being updated. Pooled funding breaks that assumption: an asset lock draws inputs from BIP44, BIP32 and the DashPay contact-receiving accounts at once, so the account holding the change routinely cannot see the other inputs' parents. It then denied trust to the wallet's own transfer and filed the change under `unconfirmed`, where nothing later corrects it. Assemble the parent view at the wallet level instead. The checker unions each funds account's final parents for the transaction before any account is borrowed mutably — and before `update_utxos` starts removing spent parents — and threads the set down through `record_transaction` / `confirm_transaction`. The per-account lookup is unchanged and still runs first; the set only supplies parents this account cannot see, so callers driving a single account directly pass an empty set and keep today's behavior. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(key-wallet): drop the outputs of a transaction that lost its inputs A self-originated transaction that is beaten to its inputs can never confirm, but the wallet kept crediting the change it contributed. Nothing removed it: the loser is in no block, so no block processing revisits it, and dash-spv's mempool expiry only drops its own tracking without telling the wallet. The change sat in the `unconfirmed` bucket permanently — money the wallet displays and does not have. When a transaction arrives with a final context, every input it spends is settled under consensus, so any other recorded unconfirmed transaction spending the same outpoint is provably dead. Drop that transaction's outputs and its record. Deliberately narrow, on two counts. It fires only on proof of a conflicting final spend, never on a timeout: the p2p network has no negative signal since BIP61 `reject` was removed, so a transaction that merely went quiet may still be live in a miner's mempool, and un-applying it would re-expose its inputs to coin selection and invite a double-spend. And it reverts only the loser's outputs, which needs no recovery of discarded state — its inputs are already correctly accounted for by the transaction that actually spent them. Reverting a transaction of unknown fate would additionally require restoring the spent parents, whose `Utxo` values are not retained anywhere; that case needs an explicit abandon primitive driven by the layer that owns broadcast policy, not this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(key-wallet): abandon a dead transaction and everything built on it A transaction the network never accepted still mutated the wallet: its outputs were credited and its inputs marked spent. Nothing reverses that — it is in no block, so no block processing revisits it — and further transactions get built on its change, each inheriting the same fiction. A testnet device carried three such transactions chained together, 1.57 DASH of outputs the network had never seen, permanently in `unconfirmed`. `abandon_transaction` takes a root txid, walks the recorded spenders transitively, and drops the whole chain: outputs, records, and the reservations they held. The walk is wallet-wide because pooled funding spreads a transaction's inputs across account families, so a descendant's change can land in an account holding none of the root. Confirmed and finalized transactions are never followed — they are settled, so what they spent was real. The coins the chain consumed are released from `spent_outpoints` rather than re-credited. The `Utxo` removed for a spent parent is discarded by `update_utxos` and `InputDetail` keeps only index/value/address, so the flags deciding a restored coin's balance bucket survive nowhere; inventing them would be a guess. What those coins actually are is unspent on chain, so the rescan that the release enables is the honest source. The call asserts the root is dead, it does not establish it — silence is not proof on a network with no reject message. The judgement stays with the layer that owns broadcast policy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(key-wallet): let the abandon cascade follow an external spend view The descendant walk reads recorded transactions, which is enough while the wallet is live but not after a restore that brings back UTXOs without their creating transactions. On a testnet device the walk stopped at the root and left two descendants credited — their records were never in the map to be found. `abandon_transaction_with_spends` takes an outpoint-to-spender map from the caller's persistence mirror and follows both views. The no-argument form is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(key-wallet): fix the two broken intra-doc links on abandon_transaction `apply_abandon` is private, so linking to it from a public item fails the docs build; state the reasoning inline instead. `update_balance` comes from `WalletInfoInterface`, not from `Self`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(key-wallet): address the review findings on the abandon path `AbandonOutcome::is_empty` could never return true: `abandoned` always holds the root, whether or not the wallet had anything recorded for it, so a caller guarding on `!is_empty()` acted on every call — including one that removed nothing. Count what was actually dropped instead, and report records alongside UTXOs. `apply_abandon` removed each abandoned record's inputs from `spent_outpoints` one record at a time, which un-marks an outpoint that a *surviving* transaction also spends — precisely the double-spend shape this work exists for, where the loser and the winner share an input. Re-derive the set from the surviving records instead; only they can say which outpoints are still spent. `rebuild_spent_outpoints` loses its serde/test cfg gate accordingly. `final_parents_of` and the cascade walk now iterate `all_funding_accounts` rather than filtering `all_accounts`. Simpler, and it drops DashPay *external* watch-only accounts from the scan — a coin the wallet cannot spend was never an input to a transaction the wallet built, so it has no business granting trust. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(key-wallet): close four gaps in the conflict sweep and the cascade Review findings, all four reachable: **The sweep skipped the InstantSend transition.** When the winner was already recorded — both spends sitting in the mempool, then an IS lock arrives — the update-in-place branch marks UTXOs and returns without reaching `update_utxos`, so the sweep it carries never ran and the loser's outputs stayed credited. An IS lock settles the inputs exactly as a block does; run the sweep there too. Regression test included, verified to fail without the fix. **The sweep did not cascade.** It dropped the direct loser only, so a further unconfirmed transaction spending the loser's change kept its own outputs — the parent gone, the child still credited, which is the same phantom-balance shape this PR removes. Walk the unconfirmed descendant closure instead; confirmed records are never followed. **`spent_outpoints` kept the loser's inputs.** Deleting the record left them marked, so an input the winner does not spend could not be rediscovered until a restart rebuilt the set. Re-derive from the surviving records, as `apply_abandon` already does — a loser spending A+B against a winner spending only A must leave A marked and free B. **The external cascade could delete settled state.** `external_spends` carries no confirmation state, so a stale mirror row naming a confirmed transaction would have had its record and UTXOs removed. Check finality against the wallet before following a spender, and refuse a settled root outright. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(key-wallet): three deeper review findings on the sweep and abandon **A loser arriving after its winner re-created the phantom, spendably.** The sweep only fires when the *arriving* transaction is final, and nothing checked an arriving transaction's inputs at all — the two guards in `update_utxos` test its own output outpoints. So the reverse order (winner confirms, loser delivered afterwards as mempool) credited the loser's outputs with nothing left to remove them, and `is_spendable` gates only on `is_locked` and maturity, so coin selection could spend them. Refuse to credit a non-final transaction whose input a block has already spent. The record still stands, so history keeps the attempt. **A swept loser's non-overlapping input silently vanished.** The doc claimed reverting only the outputs needs no recovery, which holds only when the winner spends every input the loser did. It often does not: a loser spending A+B against a winner spending only A leaves B freed from `spent_outpoints` but with no `Utxo` — discarded when the loser was recorded, and `InputDetail` cannot rebuild it. The release is what makes B recoverable by a rescan; the doc now says so, including that a chainlock-finalized funding record needs a deeper rescan. Regression test covers the round trip. **`apply_abandon` used the release `ReservationSet` forbids here.** Its doc reserves the unconditional form for coins *known spent*, and requires `release_if_owner` from a caller abandoning an in-flight build — exactly this one — so it cannot free a reservation a newer build has taken over. There is nothing of this build's left to release anyway: recording the transaction already handed its inputs to `spent_outpoints`. Drop the call. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(key-wallet): restore the doc block, and make three assertions load-bearing **#15** `sweep_conflicts_for` was inserted between `mark_utxos_instant_send`'s doc block and its signature, so it rendered with that method's docs while `mark_utxos_instant_send` was left with none. Moved back. Two stale claims alongside it. The phantom outputs do not "sit in the `unconfirmed` bucket" — the trusted-self-send rule this PR widens files them as *confirmed* and therefore spendable, which understated the bug. And `rebuild_spent_outpoints` still claimed only `Deserialize` and the test reload reach it, the assumption that made its records-only derivation look safe now that there are runtime callers. **#14** Both new balance assertions were vacuous: trusted self-send change is bucketed as confirmed, so `unconfirmed() == 0` held before the cascade as well as after, and deleting the cascade left them passing. Assert the confirmed total instead — verified to fail with the removal neutered. Also every transaction in these tests had exactly one wallet-owned output, so the per-txid removal loops were never exercised against more than one and a first-only filter would have passed. The cascade's tip now pays us twice; confirmed that a `seen`-guarded first-only filter fails the test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(key-wallet): InstantSend finality, wallet-wide abandon, targeted spent-marks **#6 — abandon could delete network-settled money.** `transaction_is_settled` and the cascade walk both gated on `is_confirmed()`, which excludes `InstantSend`. An IS lock is final against a double spend under DIP-10, so an IS-locked root passed the guard and IS-locked descendants were followed; their inputs were then released and a rescan could re-credit coins the network has irreversibly moved. Both now treat a lock as settled. **#5 — a plain block deleted an IS-locked record.** The sweep's filter admitted any `!is_confirmed()` record, and its gate accepts a non-chainlocked `InBlock`, so an ordinary tip delivery unrecoverably dropped a record the network had locked — with no reorg recovery anywhere in the stack. Precedence is now explicit: a chainlock overrides anything, and an IS-locked loser may only be evicted by a chainlocked arrival. **#4 — the walk was not wallet-wide despite saying so.** It iterated funding accounts only, skipping keys-only accounts. An asset-lock funding transaction is recorded in *both* its funding account and the identity account it pays, so abandoning it left the keys-account copy behind — which makes `is_new` false on re-sighting, so the funds account never re-records it and never re-marks its input spent. The walk and the removal now span every account that holds records. **#7 — the rebuild erased chainlock-pruned marks.** This PR dropped the `cfg` gate on `rebuild_spent_outpoints` and added runtime callers, but it derives only from live records — and under the default `keep-finalized-transactions = off` a chainlocked spend keeps only its txid, so its inputs live solely as marks already in the set. A wholesale reassignment discarded them, letting a later backfill re-credit coins spent on chain. Replaced with a targeted retain over only the outpoints the removed records contributed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(key-wallet): make the conflict sweep wallet-wide and ungated Three findings, one design gap: the sweep was attached to *account* processing, but "a competing spend just became provably dead" is a wallet-wide fact the moment any account observes a final spend. **#8 — the sweep never ran when the winner looked irrelevant.** `check_core_transaction` returns before touching any account, and relevance is computed from matching outputs and from inputs still in `utxos` — but a recorded loser already removed the shared input. So a winner that spends our coin and pays only external addresses matches nothing, and the loser stayed credited. Worse, as trusted self-send change it counts as *confirmed* and is spendable. The sweep now runs before that gate, next to `record_observed_spends`, which is unconditional for the same reason. **#13 — a loser in a sibling account survived.** The per-account sweep only visited `result.affected_accounts`, i.e. the winner's. Pooled funding routinely puts the loser's change elsewhere — the shape this PR's own commits call normal for asset locks. `ManagedWalletInfo:: sweep_conflicts` now asks every funds account. **#9 — the IS-lock sweep was dead code.** The live pipeline reaches `process_instant_send_lock` → `mark_instant_send_utxos`, which marks UTXOs and rewrites context and had no sweep at all; the branch I had added in `check_core_transaction` is only reachable on a first sighting that already carries the lock. The sweep now hangs off `mark_instant_send_utxos`, and the superseded per-account entry point is gone. Both new tests were confirmed to fail with the wallet-level sweep disabled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(key-wallet-manager): expose abandon, and pin the rescan-recovery limit **#10's mitigation.** `d3af15ed` widens trusted-self-send resolution to the whole wallet, which files a never-broadcast phantom under *confirmed* rather than unconfirmed. The sweep cannot clear that shape — it needs a competing final spend to prove the loser dead, and a transaction nobody ever saw has no competitor — so `abandon_transaction` is the only path that reaches it. Left as a `ManagedWalletInfo` method it was reachable only by a caller already holding the info; `WalletManager:: abandon_transaction` makes it a first-class entry point for the layer that owns broadcast policy, alongside the existing per-wallet operations. The underlying gap — key-wallet has no `fInMempool` equivalent, so trust is a structural check with no acceptance signal — is pre-existing and wants its own design pass. This does not close it; it makes the one remedy for its worst outcome callable. **#11.** `apply_abandon`'s "a rescan can rediscover them" holds only while the funding record is live. A chainlock-finalized funding transaction keeps just its txid, so `has_transaction` stays true, `is_new` stays false, and `confirm_transaction` returns before `update_utxos` — the only production insert site. The coin does not come back. Documented at the promise, and pinned by a test so the boundary cannot drift silently. Fixing it needs a rescan deep enough to re-fetch the block, which is above this layer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(key-wallet-manager): report swept transactions so mirrors can delete them **#12.** The sweep corrected wallet state in memory and told nobody. `TransactionCheckResult` and all five `WalletEvent` variants are purely additive, yet that bus is the documented persistence channel — so a consumer mirroring wallet state had no way to learn a row was gone. It kept the dead transaction, replayed it on the next load, and re-created the balance the wallet had already corrected. Observed on a testnet device: the correction landed every launch and never stuck. `drop_conflicted_transactions` now returns the txids it removed rather than a bool, `sweep_conflicts` unions them across accounts (one transaction can be recorded in several), and they surface as `TransactionCheckResult::swept_transactions` → `CheckTransactionsResult::per_wallet_swept` → a new `WalletEvent::TransactionsSwept` carrying the removed txids, the transaction that superseded them, and the post-removal balances. Gathered outside the relevance branch, because a sweep can fire for a transaction the wallet finds irrelevant — the shared input is already gone from `utxos` and the winner may pay only external addresses — and the removal still has to reach the consumer. Emitted before the additive events on both paths, so a consumer applying them in order deletes the dead rows before writing anything that replaces them. `dash-spv-ffi` has no C callback for the variant; it logs, with a `TODO(sweep-ffi)` naming the gap, rather than inventing ABI surface here. Also two review points from the same pass: `mark_instant_send_utxos` returned `false` when the sweep alone changed state, so the caller skipped the balance refresh and never emitted `TransactionInstantLocked` — it now reports a context rewrite and a sweep as changes. And a test comment named the wrong chain link as the multi-output one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(key-wallet): stop the sweep freeing the outpoint the winner spends `freed` collected every input of every removed loser — including the shared one the winner itself consumed. `release_spent_marks` keeps a mark only while a live record claims it, and on the checker path the sweep runs *before* the winner is recorded, so nothing did: the outpoint was released, and a later rescan could re-insert a coin that is spent on chain. Coin selection would then build a guaranteed double spend — the mirror image of the bug this PR exists to fix. Two arrival paths hid it. A block winner is already in `observed_spent_outpoints`, so the #649 guard blocks re-insertion; a relevant winner re-marks the outpoint moments later when it is recorded. An InstantSend winner has neither — the context carries no block info, so no observed spend is recorded, and an irrelevant one is never recorded at all. Reachable through `process_mempool_transaction`, which builds an `InstantSend` context for a mempool transaction arriving with its lock. Retain only the loser's *extra* inputs, independent of record ordering. `test_the_sweep_never_frees_the_winners_own_input` reproduces the InstantSend shape and was verified to fail without the retain. Two coverage gaps closed alongside, both for code this PR added: `abandon_transaction_with_spends`' external view — a descendant reachable only through the map, a stale row naming a settled transaction that must not be followed, and a settled root refused — and the reverse arrival order that `doomed_by_a_settled_spend` guards, where the winner confirms first and the loser turns up afterwards. Both verified to fail with their respective fix disabled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(dash-spv-ffi): expose the sweep as a C callback Replaces the `TODO(sweep-ffi)` placeholder, per review. Leaving it as a log line meant every C consumer silently kept mirroring transactions the wallet had already dropped — which is the same class of bug the sweep exists to fix, just one layer out. `on_transactions_swept` delivers the removed txids, the transaction whose arrival settled the inputs, and the post-removal balances. Its doc says plainly that this is the only removal-shaped wallet callback and that a consumer mirroring state must act on it, since every other one is additive. An unset callback now warns rather than logging at info: a consumer that never wires it up is in exactly the state this event exists to prevent, and that should be loud. The bundled CLI consumes it, so the shape has at least one caller in-tree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Roman <51091564+jeanpierreroma@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.