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..eb4f9dd98 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,31 @@ 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 + } + + /// 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 @@ -208,7 +238,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 +760,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..db6fd9adf --- /dev/null +++ b/key-wallet/src/managed_account/reservation.rs @@ -0,0 +1,178 @@ +//! 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. 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 { + /// 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. 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; + } + 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..6e80f18fd 100644 --- a/key-wallet/src/tests/spent_outpoints_tests.rs +++ b/key-wallet/src/tests/spent_outpoints_tests.rs @@ -1,13 +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::transaction_checking::{TransactionContext, TransactionType}; +use crate::test_utils::TestWalletContext; +use crate::transaction_checking::{BlockInfo, TransactionContext, TransactionType}; /// Create a transaction that spends the given outpoints. fn spending_tx(spent: &[OutPoint]) -> Transaction { @@ -70,6 +72,63 @@ 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)); + + // 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] 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..9b8f8ca79 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; @@ -20,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 { @@ -44,6 +54,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 +77,7 @@ impl TransactionBuilder { current_height: 0, selection_strategy: SelectionStrategy::BranchAndBound, special_payload: None, + reservations: None, } } @@ -76,8 +91,25 @@ 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. 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 { - 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 +356,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 @@ -349,13 +390,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)) } @@ -373,14 +420,33 @@ 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?; - - let mut tx_bytes = Vec::new(); - tx.consensus_encode(&mut tx_bytes).unwrap(); + // 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 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)) } @@ -549,6 +615,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 +1032,88 @@ 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)); + } + + // 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()); + } }