From fc51d06c04e87c31674d03778c6f3439814ba25d Mon Sep 17 00:00:00 2001 From: xdustinface Date: Thu, 18 Jun 2026 13:53:30 +1000 Subject: [PATCH 1/6] fix(key-wallet): reserve selected UTXOs to prevent concurrent double-spend A freshly built transaction is not reflected in the UTXO set until its broadcast is processed back into the wallet, so a second build in that window can select the same coins and produce a double-spend the network rejects. - Add an ephemeral `ReservationSet` on `ManagedCoreFundsAccount`: `set_funding` skips reserved outpoints during coin selection and `assemble_unsigned` reserves the inputs it selects. - Release the reservation when the spend is processed, and on a failed sign, so coin selection is unaffected by a build that never reaches the network. - Reclaim stale reservations via a roughly one-hour (24-block) TTL backstop, skipping the sweep at height 0 where elapsed time is unknown. - Share the set via `Arc>` so a reservation outlives the wallet lock, and never persist it: after a restart, chain and mempool sync are the source of truth. - Cover the read, write, release, TTL, and height-0 paths with unit tests, plus a `dashd` integration test asserting two pre-broadcast builds select disjoint inputs. --- .../tests/dashd_sync/tests_transaction.rs | 67 +++++++ .../managed_core_funds_account.rs | 28 ++- key-wallet/src/managed_account/mod.rs | 1 + key-wallet/src/managed_account/reservation.rs | 177 ++++++++++++++++++ key-wallet/src/tests/spent_outpoints_tests.rs | 35 ++++ .../transaction_builder.rs | 129 ++++++++++++- 6 files changed, 433 insertions(+), 4 deletions(-) create mode 100644 key-wallet/src/managed_account/reservation.rs diff --git a/dash-spv/tests/dashd_sync/tests_transaction.rs b/dash-spv/tests/dashd_sync/tests_transaction.rs index ec3b66957..6255a720b 100644 --- a/dash-spv/tests/dashd_sync/tests_transaction.rs +++ b/dash-spv/tests/dashd_sync/tests_transaction.rs @@ -1,5 +1,6 @@ use dash_spv::sync::ProgressPercentage; use dashcore::{Address, Amount, Network}; +use std::collections::HashSet; use std::sync::Arc; use std::time::Duration; use tokio::sync::RwLock; @@ -345,6 +346,72 @@ async fn test_spend_change_balance() { client_handle.stop().await; } +/// Two transactions built from the same wallet before either is broadcast must +/// not select the same UTXO. The first build reserves its inputs so the second +/// build's coin selection skips them, preventing a double-spend the network +/// would reject. Without the reservation both builds pick the same UTXO and the +/// `tx_a`/`tx_b` inputs overlap, so the disjoint-input assertion below fails. +/// +/// The two builds run sequentially: this guards that the first build's +/// reservation outlives its wallet lock and is observed by the second build, +/// not that the two builds are scheduled concurrently. +#[tokio::test] +async fn test_concurrent_builds_do_not_double_spend() { + let Some(ctx) = TestContext::new(TestChain::Minimal).await else { + return; + }; + if !ctx.dashd.supports_mining { + eprintln!("Skipping test (dashd RPC miner not available)"); + return; + } + + let (wallet, wallet_id) = create_test_wallet(EMPTY_MNEMONIC, Network::Regtest); + let mut client_handle = create_and_start_client(&ctx.client_config, Arc::clone(&wallet)).await; + wait_for_sync(&mut client_handle.progress_receiver, ctx.dashd.initial_height).await; + + // Fund the wallet with two separate, equal UTXOs at the same receive + // address. Either one alone covers a single send, so naive coin selection + // would pick the same UTXO for both builds. + let receive_address = reserve_first_address(EMPTY_MNEMONIC).await; + let utxo_amount = Amount::from_sat(200_000_000); + ctx.dashd.node.send_to_address(&receive_address, utxo_amount); + ctx.dashd.node.send_to_address(&receive_address, utxo_amount); + + let miner_address = ctx.dashd.node.get_new_address_from_wallet("default"); + ctx.dashd.node.generate_blocks(1, &miner_address); + let funded_height = ctx.dashd.initial_height + 1; + wait_for_sync(&mut client_handle.progress_receiver, funded_height).await; + wait_for_wallet_synced(&wallet, &wallet_id, funded_height).await; + + // Build both transactions before broadcasting either, so neither spend has + // been processed back into the wallet when the second build selects inputs. + let dest_a = Address::dummy(Network::Regtest, 1); + let dest_b = Address::dummy(Network::Regtest, 2); + let (tx_a, _) = + build_and_sign(&wallet, &wallet_id, &dest_a, 50_000_000).await.expect("build tx_a"); + let (tx_b, _) = + build_and_sign(&wallet, &wallet_id, &dest_b, 50_000_000).await.expect("build tx_b"); + + let inputs_a: HashSet<_> = tx_a.input.iter().map(|i| i.previous_output).collect(); + let overlap = tx_b.input.iter().any(|i| inputs_a.contains(&i.previous_output)); + assert!(!overlap, "tx_a and tx_b must not share an input (double-spend)"); + + // Both broadcasts succeed and both transactions reach the mempool: a + // double-spend would have the second rejected by the network. + client_handle.client.broadcast_transaction(&tx_a).await.expect("broadcast tx_a"); + let detected_a = wait_for_mempool_tx(&mut client_handle.wallet_event_receiver, MEMPOOL_TIMEOUT) + .await + .expect("detect tx_a"); + assert_eq!(detected_a, tx_a.txid()); + client_handle.client.broadcast_transaction(&tx_b).await.expect("broadcast tx_b"); + let detected_b = wait_for_mempool_tx(&mut client_handle.wallet_event_receiver, MEMPOOL_TIMEOUT) + .await + .expect("detect tx_b"); + assert_eq!(detected_b, tx_b.txid()); + + client_handle.stop().await; +} + /// Spend an incoming mempool UTXO (we own the output, none of the inputs) /// before it confirms. #[tokio::test] diff --git a/key-wallet/src/managed_account/managed_core_funds_account.rs b/key-wallet/src/managed_account/managed_core_funds_account.rs index cf95d78d1..986775a95 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -19,6 +19,7 @@ use crate::managed_account::address_pool; use crate::managed_account::managed_account_trait::ManagedAccountTrait; use crate::managed_account::managed_account_type::ManagedAccountType; use crate::managed_account::managed_core_keys_account::ManagedCoreKeysAccount; +use crate::managed_account::reservation::ReservationSet; use crate::managed_account::transaction_record::{ InputDetail, OutputDetail, OutputRole, TransactionDirection, }; @@ -61,6 +62,12 @@ pub struct ManagedCoreFundsAccount { /// Rebuilt from `transactions` during deserialization. #[cfg_attr(feature = "serde", serde(skip_serializing))] spent_outpoints: HashSet, + /// Outpoints reserved by in-flight transaction builds so concurrent builds + /// do not select the same UTXO before the first build's transaction is + /// processed. Empty after a restart, where chain and mempool sync + /// re-establish which coins are spent. + #[cfg_attr(feature = "serde", serde(skip))] + reservations: ReservationSet, } impl ManagedCoreFundsAccount { @@ -71,6 +78,7 @@ impl ManagedCoreFundsAccount { balance: WalletCoreBalance::default(), utxos: BTreeMap::new(), spent_outpoints: HashSet::new(), + reservations: ReservationSet::default(), } } @@ -97,9 +105,22 @@ impl ManagedCoreFundsAccount { balance: WalletCoreBalance::default(), utxos: BTreeMap::new(), spent_outpoints: HashSet::new(), + reservations: ReservationSet::default(), } } + /// Reservation set tracking outpoints chosen by in-flight transaction + /// builds, consulted by coin selection so concurrent builds do not pick the + /// same UTXO. + /// + /// The reservation is only effective when each build runs `set_funding` + /// through `assemble_unsigned` under a single uninterrupted hold of the + /// wallet lock. If two builds interleave between observing the UTXO set and + /// reserving their inputs, both can see the same UTXO as free and select it. + pub(crate) fn reservations(&self) -> &ReservationSet { + &self.reservations + } + /// Get a reference to the inner keys-account state. pub fn keys(&self) -> &ManagedCoreKeysAccount { &self.keys @@ -208,7 +229,11 @@ impl ManagedCoreFundsAccount { } } - // Remove UTXOs spent by this transaction and track spent outpoints + // Remove UTXOs spent by this transaction and track spent outpoints. + // Processing the spend also hands the inputs off from the + // ephemeral build reservation to the durable spent set, so the + // reservation taken when this transaction was built is released. + self.reservations.release(tx.input.iter().map(|input| &input.previous_output)); for input in &tx.input { self.spent_outpoints.insert(input.previous_output); @@ -726,6 +751,7 @@ impl<'de> Deserialize<'de> for ManagedCoreFundsAccount { balance: helper.balance, utxos: helper.utxos, spent_outpoints, + reservations: ReservationSet::default(), }) } } diff --git a/key-wallet/src/managed_account/mod.rs b/key-wallet/src/managed_account/mod.rs index 25b483624..5d1766c5f 100644 --- a/key-wallet/src/managed_account/mod.rs +++ b/key-wallet/src/managed_account/mod.rs @@ -21,6 +21,7 @@ pub mod managed_core_funds_account; pub mod managed_core_keys_account; pub mod managed_platform_account; pub mod platform_address; +pub(crate) mod reservation; pub mod transaction_record; pub use managed_account_ref::{ManagedAccountRef, ManagedAccountRefMut, OwnedManagedCoreAccount}; diff --git a/key-wallet/src/managed_account/reservation.rs b/key-wallet/src/managed_account/reservation.rs new file mode 100644 index 000000000..64b02ffd0 --- /dev/null +++ b/key-wallet/src/managed_account/reservation.rs @@ -0,0 +1,177 @@ +//! Ephemeral reservation of UTXOs selected by in-flight transaction builds. +//! +//! Coin selection reads the UTXO set, but a freshly built transaction is not +//! reflected in that set until the broadcast transaction is processed back into +//! the wallet (its inputs leave `utxos`). In the window between selecting inputs +//! and that processing, a second build sees the same UTXOs and can pick them +//! again, producing a double-spend the network rejects. +//! +//! A [`ReservationSet`] bridges exactly that window: selected inputs are +//! reserved at build time and skipped by subsequent coin selection. The set is +//! shared via `Arc>` so a reservation taken while the wallet lock is +//! held survives the lock being released and is observed by the next build. It +//! is never persisted: after a restart it is empty, where chain and mempool +//! sync are the source of truth for which coins are already spent. + +use std::collections::{HashMap, HashSet}; +use std::sync::{Arc, Mutex, MutexGuard}; + +use dashcore::blockdata::transaction::OutPoint; + +/// Number of blocks after which a stale reservation is reclaimed, about one +/// hour at the mainnet 2.5-minute block target. +/// +/// A reservation is normally released the moment its broadcast transaction is +/// processed (the input leaves the UTXO set), and a restart drops all +/// reservations, so this backstop only matters for a reservation whose +/// transaction was built but never processed in a long-running process, e.g. a +/// silently failed broadcast. The value is kept comfortably above the real +/// selection-to-processing latency (seconds): reclaiming a still-in-flight +/// reservation too early would let coin selection re-pick the input and rebuild +/// the very double-spend this guards against. +const RESERVATION_TTL_BLOCKS: u32 = 24; + +/// Ephemeral, in-memory set of reserved outpoints keyed by the block height at +/// which each was reserved (used for the TTL backstop). Cloning shares the +/// underlying state, which is what lets a build's reservation outlive the +/// wallet lock and be seen by the next build. +#[derive(Debug, Clone, Default)] +pub(crate) struct ReservationSet { + inner: Arc>>, +} + +impl ReservationSet { + /// Lock the inner map, recovering from a poisoned mutex instead of + /// panicking. The guarded data is a plain `HashMap` with no invariant a + /// partial write could break, and panicking here would brick all later coin + /// selection in a long-running node. + fn lock(&self) -> MutexGuard<'_, HashMap> { + self.inner.lock().unwrap_or_else(|poisoned| poisoned.into_inner()) + } + + fn sweep(reserved: &mut HashMap, current_height: u32) { + // Height 0 means the wallet has no processed height yet, so the elapsed + // span is unknown and no entry can be reliably judged stale. + if current_height == 0 { + return; + } + reserved.retain(|_, reserved_at| { + current_height.saturating_sub(*reserved_at) < RESERVATION_TTL_BLOCKS + }); + } + + /// Reserve `outpoints` as of `current_height`, dropping expired entries + /// first. Re-reserving an outpoint refreshes its height. + pub(crate) fn reserve(&self, outpoints: &[OutPoint], current_height: u32) { + let mut reserved = self.lock(); + Self::sweep(&mut reserved, current_height); + for outpoint in outpoints { + reserved.insert(*outpoint, current_height); + } + } + + /// Return the currently reserved outpoints, dropping expired entries first. + pub(crate) fn reserved(&self, current_height: u32) -> HashSet { + let mut reserved = self.lock(); + Self::sweep(&mut reserved, current_height); + reserved.keys().copied().collect() + } + + /// Release the given outpoints. Idempotent: releasing an outpoint that is + /// not reserved does nothing. + pub(crate) fn release<'a>(&self, outpoints: impl IntoIterator) { + let mut reserved = self.lock(); + for outpoint in outpoints { + reserved.remove(outpoint); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use dashcore::Txid; + + fn outpoint(byte: u8, vout: u32) -> OutPoint { + OutPoint::new(Txid::from([byte; 32]), vout) + } + + #[test] + fn reserve_then_skip_then_release() { + let set = ReservationSet::default(); + let a = outpoint(0x01, 0); + let b = outpoint(0x02, 1); + + set.reserve(&[a], 100); + assert!(set.reserved(100).contains(&a)); + assert!(!set.reserved(100).contains(&b)); + assert_eq!(set.reserved(100), HashSet::from([a])); + + set.release([&a]); + assert!(set.reserved(100).is_empty()); + } + + #[test] + fn release_is_idempotent() { + let set = ReservationSet::default(); + let a = outpoint(0x03, 0); + // Releasing an unreserved outpoint is a no-op. + set.release([&a]); + set.reserve(&[a], 10); + set.release([&a]); + set.release([&a]); + assert!(!set.reserved(10).contains(&a)); + } + + #[test] + fn ttl_reclaims_stale_reservation_by_height() { + let set = ReservationSet::default(); + let a = outpoint(0x04, 0); + set.reserve(&[a], 100); + + // The boundary is exclusive, so the entry survives until exactly + // `reserved_at + RESERVATION_TTL_BLOCKS`. + assert_eq!(set.reserved(100 + RESERVATION_TTL_BLOCKS - 1), HashSet::from([a])); + // At that height the sweep's strict less-than drops the entry. + assert!(set.reserved(100 + RESERVATION_TTL_BLOCKS).is_empty()); + } + + #[test] + fn zero_height_disables_sweep() { + let set = ReservationSet::default(); + let a = outpoint(0x07, 0); + set.reserve(&[a], 0); + + // Height 0 means the wallet has no processed height yet, so the elapsed + // span is unknown and the sweep is suppressed: the reservation stands. + assert!(set.reserved(0).contains(&a)); + + // Once a real height is known the TTL backstop applies again. + set.reserve(&[a], 1); + assert!(set.reserved(1 + RESERVATION_TTL_BLOCKS).is_empty()); + } + + #[test] + fn re_reserving_refreshes_the_ttl_height() { + let set = ReservationSet::default(); + let a = outpoint(0x06, 0); + set.reserve(&[a], 100); + // A later reserve replaces the stored height, so the TTL is measured + // from the most recent reservation, not the first. + set.reserve(&[a], 150); + + assert_eq!(set.reserved(150 + RESERVATION_TTL_BLOCKS - 1), HashSet::from([a])); + assert!(set.reserved(150 + RESERVATION_TTL_BLOCKS).is_empty()); + } + + #[test] + fn clone_shares_state() { + let set = ReservationSet::default(); + let clone = set.clone(); + let a = outpoint(0x05, 0); + // A reservation taken on one handle is visible through the other, which + // is what lets a build's reservation outlive the wallet lock. + set.reserve(&[a], 1); + assert!(clone.reserved(1).contains(&a)); + } +} diff --git a/key-wallet/src/tests/spent_outpoints_tests.rs b/key-wallet/src/tests/spent_outpoints_tests.rs index cf8215f58..6b70ab6e4 100644 --- a/key-wallet/src/tests/spent_outpoints_tests.rs +++ b/key-wallet/src/tests/spent_outpoints_tests.rs @@ -7,6 +7,7 @@ use crate::account::{AccountType, StandardAccountType, TransactionRecord}; use crate::managed_account::managed_account_trait::ManagedAccountTrait; use crate::managed_account::transaction_record::TransactionDirection; use crate::managed_account::ManagedCoreFundsAccount; +use crate::test_utils::TestWalletContext; use crate::transaction_checking::{TransactionContext, TransactionType}; /// Create a transaction that spends the given outpoints. @@ -70,6 +71,40 @@ fn fresh_account_has_empty_spent_outpoints() { let _ = probe; // used only for clarity of intent } +#[test] +fn reservations_are_not_persisted() { + let account = ManagedCoreFundsAccount::dummy_bip44(); + let outpoint = OutPoint::new(Txid::from([0x42; 32]), 0); + + account.reservations().reserve(&[outpoint], 0); + assert!(account.reservations().reserved(0).contains(&outpoint)); + + let json = serde_json::to_string(&account).unwrap(); + assert!(!json.contains("reservations")); + + // After a round-trip the reservation set is empty: it is ephemeral, and on + // restart chain/mempool sync is the source of truth for spent coins. + let deserialized: ManagedCoreFundsAccount = serde_json::from_str(&json).unwrap(); + assert!(!deserialized.reservations().reserved(0).contains(&outpoint)); +} + +#[tokio::test] +async fn processing_a_spend_releases_its_reservation() { + let (mut ctx, funding_tx) = TestWalletContext::new_random().with_mempool_funding(150_000).await; + let funded = OutPoint::new(funding_tx.txid(), 0); + + let account = ctx.managed_wallet.first_bip44_managed_account_mut().expect("BIP44 account"); + assert!(account.utxos.contains_key(&funded)); + account.reservations().reserve(&[funded], 0); + assert!(account.reservations().reserved(0).contains(&funded)); + + let spend = spending_tx(&[funded]); + ctx.check_transaction(&spend, TransactionContext::Mempool).await; + + let account = ctx.managed_wallet.first_bip44_managed_account().expect("BIP44 account"); + assert!(!account.reservations().reserved(0).contains(&funded)); +} + #[test] fn serde_round_trip_rebuilds_spent_outpoints() { let mut account = ManagedCoreFundsAccount::dummy_bip44(); diff --git a/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs b/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs index b87a0a913..96d361984 100644 --- a/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs +++ b/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs @@ -3,6 +3,7 @@ //! This module provides high-level transaction building functionality //! using types from the dashcore crate. +use crate::managed_account::reservation::ReservationSet; use crate::managed_account::ManagedCoreFundsAccount; use crate::wallet::managed_wallet_info::coin_selection::{CoinSelector, SelectionStrategy}; use crate::wallet::managed_wallet_info::fee::FeeRate; @@ -10,7 +11,7 @@ use crate::{Account, DerivationPath, Signer, Utxo, Wallet}; use core::fmt; use dashcore::blockdata::script::{Builder, PushBytes, ScriptBuf}; use dashcore::blockdata::transaction::special_transaction::TransactionPayload; -use dashcore::blockdata::transaction::Transaction; +use dashcore::blockdata::transaction::{OutPoint, Transaction}; use dashcore::consensus::Encodable; use dashcore::sighash::{EcdsaSighashType, LegacySighash, SighashCache}; use dashcore::Address; @@ -44,6 +45,10 @@ pub struct TransactionBuilder { selection_strategy: SelectionStrategy, /// Special transaction payload for Dash-specific transactions special_payload: Option, + /// Reservation set of the funding account, captured by `set_funding`. The + /// inputs chosen during assembly are reserved here so a concurrent build + /// skips them until the broadcast transaction is processed. + reservations: Option, } impl Default for TransactionBuilder { @@ -63,6 +68,7 @@ impl TransactionBuilder { current_height: 0, selection_strategy: SelectionStrategy::BranchAndBound, special_payload: None, + reservations: None, } } @@ -76,8 +82,22 @@ impl TransactionBuilder { self } + /// Seed the builder with the funding account's spendable UTXOs, skipping any + /// already reserved by another in-flight build. + /// + /// This call and the later `assemble_unsigned` that reserves the chosen + /// inputs must run under one uninterrupted hold of the wallet lock. If two + /// builds interleave between here and their reservation, both can observe the + /// same UTXO as free and select it, defeating the reservation. pub fn set_funding(mut self, funds_acc: &mut ManagedCoreFundsAccount, acc: &Account) -> Self { - self.inputs = funds_acc.utxos.values().cloned().collect(); + let reserved = funds_acc.reservations().reserved(self.current_height); + self.inputs = funds_acc + .utxos + .values() + .filter(|utxo| !reserved.contains(&utxo.outpoint)) + .cloned() + .collect(); + self.reservations = Some(funds_acc.reservations().clone()); self.change_addr = funds_acc.next_change_address(Some(&acc.account_xpub), true).ok(); self } @@ -324,6 +344,15 @@ impl TransactionBuilder { special_transaction_payload: self.special_payload, }; + // Reserve the chosen inputs so a concurrent build skips them until the + // broadcast transaction is processed back into the wallet (which + // releases the reservation) or the TTL backstop reclaims it. + if let Some(reservations) = &self.reservations { + let outpoints: Vec = + selected_inputs.iter().map(|utxo| utxo.outpoint).collect(); + reservations.reserve(&outpoints, self.current_height); + } + return Ok((transaction, selected_inputs)); // BIP-69: Sort outputs by amount first, then by scriptPubKey @@ -373,9 +402,23 @@ impl TransactionBuilder { P: Fn(Address) -> Option + Send, { let fee_rate = self.fee_rate; + let reservations = self.reservations.clone(); let (tx, inputs) = self.assemble_unsigned()?; - let tx = signer.sign_tx(tx, inputs, path_resolver).await?; + // Signing never reaches the network for a local key, but an external + // signer can fail. A failed sign means the reserved inputs are still + // spendable, so release them now instead of stranding the funds until + // the TTL backstop reclaims them. + let reserved: Vec = inputs.iter().map(|utxo| utxo.outpoint).collect(); + let tx = match signer.sign_tx(tx, inputs, path_resolver).await { + Ok(tx) => tx, + Err(err) => { + if let Some(reservations) = &reservations { + reservations.release(reserved.iter()); + } + return Err(err); + } + }; let mut tx_bytes = Vec::new(); tx.consensus_encode(&mut tx_bytes).unwrap(); @@ -549,6 +592,7 @@ impl std::error::Error for BuilderError {} #[cfg(test)] mod tests { use super::*; + use crate::test_utils::TestWalletContext; use crate::Network; use dashcore::blockdata::transaction::special_transaction::asset_lock::AssetLockPayload; use dashcore::{OutPoint, Txid}; @@ -965,4 +1009,83 @@ mod tests { "Should select multiple inputs to cover fees for special payload" ); } + + #[test] + fn set_funding_skips_reserved_utxos() { + let ctx = TestWalletContext::new_random(); + let account = + ctx.wallet.accounts.standard_bip44_accounts.get(&0).expect("BIP44 account").clone(); + + let mut funds = ManagedCoreFundsAccount::dummy_bip44(); + let reserved = Utxo::dummy(0x01, 100_000, 100, false, true); + let free = Utxo::dummy(0x02, 200_000, 100, false, true); + funds.utxos.insert(reserved.outpoint, reserved.clone()); + funds.utxos.insert(free.outpoint, free.clone()); + + funds.reservations().reserve(&[reserved.outpoint], 200); + + let builder = + TransactionBuilder::new().set_current_height(200).set_funding(&mut funds, &account); + + let candidates: Vec = builder.inputs.iter().map(|utxo| utxo.outpoint).collect(); + assert!(!candidates.contains(&reserved.outpoint)); + assert!(candidates.contains(&free.outpoint)); + } + + #[tokio::test] + async fn build_signed_releases_reservation_on_signing_failure() { + let ctx = TestWalletContext::new_random(); + let account = + ctx.wallet.accounts.standard_bip44_accounts.get(&0).expect("BIP44 account").clone(); + + let mut funds = ManagedCoreFundsAccount::dummy_bip44(); + let utxo = Utxo::dummy(0x01, 1_000_000, 100, false, true); + funds.utxos.insert(utxo.outpoint, utxo.clone()); + + let destination = Address::dummy(Network::Testnet, 0); + let builder = TransactionBuilder::new() + .set_current_height(200) + .set_fee_rate(FeeRate::normal()) + .set_funding(&mut funds, &account) + .set_change_address(Address::dummy(Network::Testnet, 1)) + .add_output(&destination, 500_000); + + // A resolver that never finds a derivation path forces signing to fail + // after the inputs have already been reserved by `assemble_unsigned`. + let result = builder.build_signed(&ctx.wallet, |_addr| None).await; + assert!(result.is_err()); + + // The failed sign must leave no reservation behind, so the UTXO stays + // selectable instead of being stranded until the TTL backstop. + assert!(funds.reservations().reserved(200).is_empty()); + } + + #[test] + fn build_unsigned_reserves_selected_inputs() { + let ctx = TestWalletContext::new_random(); + let account = + ctx.wallet.accounts.standard_bip44_accounts.get(&0).expect("BIP44 account").clone(); + + let mut funds = ManagedCoreFundsAccount::dummy_bip44(); + let utxo = Utxo::dummy(0x01, 1_000_000, 100, false, true); + funds.utxos.insert(utxo.outpoint, utxo.clone()); + + let destination = Address::dummy(Network::Testnet, 0); + let (tx, _) = TransactionBuilder::new() + .set_current_height(200) + .set_fee_rate(FeeRate::normal()) + .set_funding(&mut funds, &account) + .set_change_address(Address::dummy(Network::Testnet, 1)) + .add_output(&destination, 500_000) + .build_unsigned() + .expect("build unsigned"); + + // Every input the build selected is reserved, so a later build observes + // them as taken and skips them. + let reserved = funds.reservations().reserved(200); + assert!(!reserved.is_empty()); + for input in &tx.input { + assert!(reserved.contains(&input.previous_output)); + } + } } From d28a42400e1ccfb4d7417c43a8cca92aab6cdebf Mon Sep 17 00:00:00 2001 From: xdustinface Date: Thu, 18 Jun 2026 20:43:08 +1000 Subject: [PATCH 2/6] docs: trim and clarify `ReservationSet` doc comments Drop the data-shape restatement from the `ReservationSet` struct doc and lead the `lock` method doc with its rationale rather than the method name. Make the height-0 sweep comment explicit that a height-0 entry relies on a later non-zero height to ever be reclaimed. --- key-wallet/src/managed_account/reservation.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/key-wallet/src/managed_account/reservation.rs b/key-wallet/src/managed_account/reservation.rs index 64b02ffd0..db6fd9adf 100644 --- a/key-wallet/src/managed_account/reservation.rs +++ b/key-wallet/src/managed_account/reservation.rs @@ -31,8 +31,7 @@ use dashcore::blockdata::transaction::OutPoint; /// the very double-spend this guards against. const RESERVATION_TTL_BLOCKS: u32 = 24; -/// Ephemeral, in-memory set of reserved outpoints keyed by the block height at -/// which each was reserved (used for the TTL backstop). Cloning shares the +/// Ephemeral, in-memory set of reserved outpoints. Cloning shares the /// underlying state, which is what lets a build's reservation outlive the /// wallet lock and be seen by the next build. #[derive(Debug, Clone, Default)] @@ -41,17 +40,19 @@ pub(crate) struct ReservationSet { } impl ReservationSet { - /// Lock the inner map, recovering from a poisoned mutex instead of - /// panicking. The guarded data is a plain `HashMap` with no invariant a - /// partial write could break, and panicking here would brick all later coin - /// selection in a long-running node. + /// Recovers from a poisoned mutex rather than panicking: the guarded data is + /// a plain `HashMap` with no invariant a partial write could break, and + /// panicking here would strand all later coin selection in a long-running + /// node. fn lock(&self) -> MutexGuard<'_, HashMap> { self.inner.lock().unwrap_or_else(|poisoned| poisoned.into_inner()) } fn sweep(reserved: &mut HashMap, current_height: u32) { // Height 0 means the wallet has no processed height yet, so the elapsed - // span is unknown and no entry can be reliably judged stale. + // span is unknown and no entry can be reliably judged stale. An entry + // stamped at height 0 therefore relies on a later non-zero height being + // presented before it can ever be reclaimed by the TTL backstop. if current_height == 0 { return; } From fada0848b624e1e58ad9e127ba6dd8b60f3f078b Mon Sep 17 00:00:00 2001 From: xdustinface Date: Thu, 18 Jun 2026 20:44:40 +1000 Subject: [PATCH 3/6] test: cover reservation release on the confirmed spend path `processing_a_spend_releases_its_reservation` only exercised the `Mempool` context. Extend it so a second funded outpoint is reserved and then spent under `TransactionContext::InBlock`, asserting the reservation is released on the confirmed path as well. --- key-wallet/src/tests/spent_outpoints_tests.rs | 28 +++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/key-wallet/src/tests/spent_outpoints_tests.rs b/key-wallet/src/tests/spent_outpoints_tests.rs index 6b70ab6e4..6e80f18fd 100644 --- a/key-wallet/src/tests/spent_outpoints_tests.rs +++ b/key-wallet/src/tests/spent_outpoints_tests.rs @@ -1,14 +1,15 @@ //! Tests for spent_outpoints deserialization and tracking. use dashcore::blockdata::transaction::{OutPoint, Transaction}; -use dashcore::{TxIn, Txid}; +use dashcore::hashes::Hash; +use dashcore::{BlockHash, TxIn, Txid}; use crate::account::{AccountType, StandardAccountType, TransactionRecord}; use crate::managed_account::managed_account_trait::ManagedAccountTrait; use crate::managed_account::transaction_record::TransactionDirection; use crate::managed_account::ManagedCoreFundsAccount; use crate::test_utils::TestWalletContext; -use crate::transaction_checking::{TransactionContext, TransactionType}; +use crate::transaction_checking::{BlockInfo, TransactionContext, TransactionType}; /// Create a transaction that spends the given outpoints. fn spending_tx(spent: &[OutPoint]) -> Transaction { @@ -103,6 +104,29 @@ async fn processing_a_spend_releases_its_reservation() { let account = ctx.managed_wallet.first_bip44_managed_account().expect("BIP44 account"); assert!(!account.reservations().reserved(0).contains(&funded)); + + // The confirmed path releases reservations too: a separate funding tx with + // a distinct input range yields a second outpoint that is reserved and then + // spent in a block. + let second_funding = Transaction::dummy(&ctx.receive_address, 1..2, &[120_000]); + ctx.check_transaction(&second_funding, TransactionContext::Mempool).await; + let second_funded = OutPoint::new(second_funding.txid(), 0); + + let account = ctx.managed_wallet.first_bip44_managed_account_mut().expect("BIP44 account"); + assert!(account.utxos.contains_key(&second_funded)); + account.reservations().reserve(&[second_funded], 0); + assert!(account.reservations().reserved(0).contains(&second_funded)); + + let block_hash = BlockHash::from_slice(&[7u8; 32]).expect("hash"); + let confirmed_spend = spending_tx(&[second_funded]); + ctx.check_transaction( + &confirmed_spend, + TransactionContext::InBlock(BlockInfo::new(100, block_hash, 1_700_000_000)), + ) + .await; + + let account = ctx.managed_wallet.first_bip44_managed_account().expect("BIP44 account"); + assert!(!account.reservations().reserved(0).contains(&second_funded)); } #[test] From af0030f0588737d2623554424cbf43c5176c17a2 Mon Sep 17 00:00:00 2001 From: xdustinface Date: Thu, 18 Jun 2026 20:46:19 +1000 Subject: [PATCH 4/6] fix(key-wallet): release reservations when fee encoding fails `build_unsigned` reserved the selected inputs in `assemble_unsigned` and then encoded the transaction with `unwrap`, leaking the reservation until the TTL backstop if the encode ever failed. Route both builders through a fallible `encoded_size` helper that returns a `BuilderError` instead of panicking, and release the reserved inputs on that failure path so `build_unsigned` matches the existing release behavior in `build_signed`. --- .../transaction_builder.rs | 38 ++++++++++++++----- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs b/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs index 96d361984..e2d8d6094 100644 --- a/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs +++ b/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs @@ -21,6 +21,15 @@ use secp256k1::ecdsa::Signature; use secp256k1::{Message, PublicKey, Secp256k1}; use std::cmp::Ordering; +/// Consensus-encoded byte length of `tx`, used to compute the fee from the real +/// serialized size. Surfaces an encode error instead of panicking so the caller +/// can release any reservations it took rather than stranding the inputs. +fn encoded_size(tx: &Transaction) -> Result { + let mut bytes = Vec::new(); + tx.consensus_encode(&mut bytes) + .map_err(|err| BuilderError::InvalidData(format!("failed to encode transaction: {}", err))) +} + /// Calculate varint size for a given number fn varint_size(n: usize) -> usize { match n { @@ -378,13 +387,19 @@ impl TransactionBuilder { pub fn build_unsigned(self) -> Result<(Transaction, u64), BuilderError> { let fee_rate = self.fee_rate; + let reservations = self.reservations.clone(); - let (tx, _) = self.assemble_unsigned()?; - - let mut tx_bytes = Vec::new(); - tx.consensus_encode(&mut tx_bytes).unwrap(); + let (tx, inputs) = self.assemble_unsigned()?; - let fee = fee_rate.calculate_fee(tx_bytes.len()); + let fee = match encoded_size(&tx) { + Ok(size) => fee_rate.calculate_fee(size), + Err(err) => { + if let Some(reservations) = &reservations { + reservations.release(inputs.iter().map(|utxo| &utxo.outpoint)); + } + return Err(err); + } + }; Ok((tx, fee)) } @@ -420,10 +435,15 @@ impl TransactionBuilder { } }; - let mut tx_bytes = Vec::new(); - tx.consensus_encode(&mut tx_bytes).unwrap(); - - let fee = fee_rate.calculate_fee(tx_bytes.len()); + let fee = match encoded_size(&tx) { + Ok(size) => fee_rate.calculate_fee(size), + Err(err) => { + if let Some(reservations) = &reservations { + reservations.release(reserved.iter()); + } + return Err(err); + } + }; Ok((tx, fee)) } From 6416bea0e1bb13d822b77121928015296456817c Mon Sep 17 00:00:00 2001 From: xdustinface Date: Thu, 18 Jun 2026 20:46:36 +1000 Subject: [PATCH 5/6] docs: note the builder must not be held across an await in `set_funding` Strengthen the `set_funding` lock invariant to state that the builder must not be held across an `await` before `build_signed` or `assemble_unsigned`, since suspending there reopens the read-then-reserve window for a concurrent build. --- .../src/wallet/managed_wallet_info/transaction_builder.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs b/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs index e2d8d6094..c40754418 100644 --- a/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs +++ b/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs @@ -97,7 +97,10 @@ impl TransactionBuilder { /// This call and the later `assemble_unsigned` that reserves the chosen /// inputs must run under one uninterrupted hold of the wallet lock. If two /// builds interleave between here and their reservation, both can observe the - /// same UTXO as free and select it, defeating the reservation. + /// same UTXO as free and select it, defeating the reservation. The builder + /// must therefore not be held across an `await` between `set_funding` and + /// `build_signed` or `assemble_unsigned`, since suspending there reopens the + /// read-then-reserve window for a concurrent build. pub fn set_funding(mut self, funds_acc: &mut ManagedCoreFundsAccount, acc: &Account) -> Self { let reserved = funds_acc.reservations().reserved(self.current_height); self.inputs = funds_acc From e9ff904e051e6792f5360560e87b1111db170766 Mon Sep 17 00:00:00 2001 From: xdustinface Date: Fri, 19 Jun 2026 14:45:14 +1000 Subject: [PATCH 6/6] feat(key-wallet): release UTXO reservations for abandoned builds Add `ManagedCoreFundsAccount::release_reservation` so a caller can release the reservations held for a built transaction's inputs when that transaction will not be broadcast (e.g. the user cancelled). Without this the inputs stay reserved until the TTL backstop reclaims them, making the funds appear unspendable for about an hour. Broadcast transactions still release on their own once the spend is processed back into the wallet, so this is only for abandoned builds. --- .../src/managed_account/managed_core_funds_account.rs | 9 +++++++++ .../wallet/managed_wallet_info/transaction_builder.rs | 5 +++++ 2 files changed, 14 insertions(+) diff --git a/key-wallet/src/managed_account/managed_core_funds_account.rs b/key-wallet/src/managed_account/managed_core_funds_account.rs index 986775a95..eb4f9dd98 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -121,6 +121,15 @@ impl ManagedCoreFundsAccount { &self.reservations } + /// Release the reservations held for `tx`'s inputs. Call this when a built + /// transaction will not be broadcast, e.g. the user cancelled, so its inputs + /// become selectable again immediately instead of waiting out the TTL + /// backstop. Broadcast transactions release on their own once the spend is + /// processed back into the wallet, so this is only for abandoned builds. + pub fn release_reservation(&self, tx: &Transaction) { + self.reservations.release(tx.input.iter().map(|input| &input.previous_output)); + } + /// Get a reference to the inner keys-account state. pub fn keys(&self) -> &ManagedCoreKeysAccount { &self.keys diff --git a/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs b/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs index c40754418..9b8f8ca79 100644 --- a/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs +++ b/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs @@ -1110,5 +1110,10 @@ mod tests { for input in &tx.input { assert!(reserved.contains(&input.previous_output)); } + + // Abandoning the build releases its inputs immediately rather than + // stranding them until the TTL backstop reclaims them. + funds.release_reservation(&tx); + assert!(funds.reservations().reserved(200).is_empty()); } }