diff --git a/dash-spv/tests/dashd_sync/tests_transaction.rs b/dash-spv/tests/dashd_sync/tests_transaction.rs index 0dfb5d7aa..f0f005225 100644 --- a/dash-spv/tests/dashd_sync/tests_transaction.rs +++ b/dash-spv/tests/dashd_sync/tests_transaction.rs @@ -370,7 +370,7 @@ async fn build_and_sign( TransactionBuilder::new() .set_current_height(height) - .set_funding(funds_account, &account) + .add_funding(funds_account, &account) .add_output(&dest, amount) .build_signed(w, |a| funds_account.address_derivation_path(&a)) .await @@ -605,7 +605,7 @@ async fn test_drain_account_into_another() { let (w, info) = lock.get_wallet_and_info_mut(&wallet_id).expect("wallet"); info.build_and_sign_transaction( w, - AccountTypePreference::BIP32, + &[AccountTypePreference::BIP32], 0, vec![(dest.as_unchecked().clone(), 0)], FeeRate::normal(), diff --git a/key-wallet-ffi/src/transaction.rs b/key-wallet-ffi/src/transaction.rs index 700d2c192..eb09a0201 100644 --- a/key-wallet-ffi/src/transaction.rs +++ b/key-wallet-ffi/src/transaction.rs @@ -137,7 +137,7 @@ pub unsafe extern "C" fn wallet_build_and_sign_transaction( manager .build_and_sign_transaction( &wallet_id, - AccountTypePreference::BIP44, + &[AccountTypePreference::BIP44], account_index, outputs, FeeRate::new(fee_per_kb), diff --git a/key-wallet-manager/src/lib.rs b/key-wallet-manager/src/lib.rs index b3337e7a3..9680eaba5 100644 --- a/key-wallet-manager/src/lib.rs +++ b/key-wallet-manager/src/lib.rs @@ -588,7 +588,7 @@ impl WalletManager { pub async fn build_and_sign_transaction( &mut self, wallet_id: &WalletId, - source: AccountTypePreference, + sources: &[AccountTypePreference], source_index: u32, outputs: Vec<(Address, u64)>, fee_rate: FeeRate, @@ -600,7 +600,7 @@ impl WalletManager { .ok_or(WalletError::WalletNotFound(*wallet_id))?; managed_wallet - .build_and_sign_transaction(wallet, source, source_index, outputs, fee_rate, strategy) + .build_and_sign_transaction(wallet, sources, source_index, outputs, fee_rate, strategy) .await .map_err(|e| WalletError::TransactionBuild(e.to_string())) } diff --git a/key-wallet/src/managed_account/reservation.rs b/key-wallet/src/managed_account/reservation.rs index 1715c5fd4..d24eba69b 100644 --- a/key-wallet/src/managed_account/reservation.rs +++ b/key-wallet/src/managed_account/reservation.rs @@ -24,6 +24,7 @@ //! why the check must be atomic under this set's mutex (`dashpay/platform#4185`). use std::collections::{HashMap, HashSet}; +use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::{Arc, Mutex, MutexGuard}; use dashcore::blockdata::transaction::OutPoint; @@ -41,20 +42,21 @@ use dashcore::blockdata::transaction::OutPoint; /// the very double-spend this guards against. const RESERVATION_TTL_BLOCKS: u32 = 24; -/// Opaque, per-`reserve`-call identity stamped onto every outpoint that call -/// reserves. +/// Opaque, per-build identity stamped onto every outpoint that build reserves. /// /// A token is the *proof of ownership* a build presents to the reservation /// set's `release_if_owner` so a release only removes the build's own -/// reservation. Each `reserve` call mints a fresh token from a monotonic -/// counter, so two reservations never share one — not even two reservations -/// taken at the same block height, which is why the height (which collides -/// freely) cannot serve as the identity. +/// reservation. Tokens are minted from a process-wide counter, so two builds +/// never share one — not even two reservations taken at the same block height, +/// which is why the height (which collides freely) cannot serve as the +/// identity, and not even across two accounts' sets, which is what lets one +/// build funded from several accounts reserve in each of them under a single +/// token. /// -/// The inner counter is private and there is no public constructor: a token can -/// only originate from a real `reserve` call. That is deliberate — it prevents a -/// caller from forging a token that happens to match another build's ownership -/// and releasing inputs out from under it. +/// The inner counter is private and minting is crate-private: a token can only +/// originate from this crate. That is deliberate — it prevents a caller from +/// forging a token that happens to match another build's ownership and +/// releasing inputs out from under it. /// /// Copy semantics let a build hold its token cheaply across an `.await` (e.g. /// the platform broadcast path in `dashpay/platform#4185`) and present it again @@ -62,6 +64,17 @@ const RESERVATION_TTL_BLOCKS: u32 = 24; #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub struct ReservationToken(u64); +impl ReservationToken { + /// Mint a token, unique within this process. + /// + /// (Wraparound would need ~2^64 mints in one process lifetime, which is + /// unreachable in practice.) + pub(crate) fn next() -> Self { + static NEXT_TOKEN: AtomicU64 = AtomicU64::new(0); + Self(NEXT_TOKEN.fetch_add(1, Ordering::Relaxed)) + } +} + /// A single reserved outpoint: when it was reserved (for the TTL backstop) and /// which build owns it (for owner-guarded release). #[derive(Debug, Clone, Copy)] @@ -73,16 +86,10 @@ struct Reservation { owner: ReservationToken, } -/// Mutex-guarded interior: the reservations keyed by outpoint plus the counter -/// that mints the next [`ReservationToken`]. +/// Mutex-guarded interior: the reservations keyed by outpoint. #[derive(Debug, Default)] struct Reserved { entries: HashMap, - /// Monotonically increasing source of unique tokens. Never persisted and - /// only ever incremented, so within a process every issued token is unique. - /// (Wraparound would need ~2^64 reserves in one process lifetime, which is - /// unreachable in practice.) - next_token: u64, } /// Ephemeral, in-memory set of reserved outpoints. Cloning shares the @@ -95,7 +102,7 @@ pub(crate) struct ReservationSet { impl ReservationSet { /// Recovers from a poisoned mutex rather than panicking: the guarded data is - /// a plain map plus a counter with no invariant a partial write could break, + /// a plain map 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<'_, Reserved> { @@ -115,25 +122,26 @@ impl ReservationSet { }); } - /// Reserve `outpoints` as of `current_height`, dropping expired entries - /// first, and return the [`ReservationToken`] stamped onto all of them. + /// Reserve `outpoints` as of `current_height` on behalf of `owner`, + /// dropping expired entries first. /// - /// Every outpoint in a single call shares the one returned token: the caller - /// keeps it and later presents it to [`Self::release_if_owner`] to release - /// only what this call reserved. Re-reserving an outpoint (a later `reserve` - /// naming it again) refreshes its height *and* transfers ownership to the new - /// token — the previous owner's [`Self::release_if_owner`] then becomes a - /// no-op for it, which is precisely the behavior that closes the + /// The owner is the token the reserving build was minted, and it is what the + /// build later presents to [`Self::release_if_owner`] to release only what it + /// reserved — here and, for a build funded from several accounts, in every + /// other account's set it reserved in. Re-reserving an outpoint (a later + /// `reserve` naming it again) refreshes its height *and* transfers ownership + /// to the new owner — the previous owner's [`Self::release_if_owner`] then + /// becomes a no-op for it, which is precisely the behavior that closes the /// release/re-reserve race described in the module docs (`platform#4185`). - pub(crate) fn reserve(&self, outpoints: &[OutPoint], current_height: u32) -> ReservationToken { + pub(crate) fn reserve( + &self, + outpoints: &[OutPoint], + current_height: u32, + owner: ReservationToken, + ) { let mut reserved = self.lock(); Self::sweep(&mut reserved, current_height); - let owner = ReservationToken(reserved.next_token); - // Increment even on an empty `outpoints` slice so a token is never - // reissued; wrapping is documented-unreachable but avoids a debug panic. - reserved.next_token = reserved.next_token.wrapping_add(1); - for outpoint in outpoints { reserved.entries.insert( *outpoint, @@ -143,7 +151,6 @@ impl ReservationSet { }, ); } - owner } /// Return the currently reserved outpoints, dropping expired entries first. @@ -224,7 +231,7 @@ mod tests { let a = outpoint(0x01, 0); let b = outpoint(0x02, 1); - set.reserve(&[a], 100); + set.reserve(&[a], 100, ReservationToken::next()); assert!(set.reserved(100).contains(&a)); assert!(!set.reserved(100).contains(&b)); assert_eq!(set.reserved(100), HashSet::from([a])); @@ -239,7 +246,7 @@ mod tests { let a = outpoint(0x03, 0); // Releasing an unreserved outpoint is a no-op. set.release([&a]); - set.reserve(&[a], 10); + set.reserve(&[a], 10, ReservationToken::next()); set.release([&a]); set.release([&a]); assert!(!set.reserved(10).contains(&a)); @@ -249,7 +256,7 @@ mod tests { fn ttl_reclaims_stale_reservation_by_height() { let set = ReservationSet::default(); let a = outpoint(0x04, 0); - set.reserve(&[a], 100); + set.reserve(&[a], 100, ReservationToken::next()); // The boundary is exclusive, so the entry survives until exactly // `reserved_at + RESERVATION_TTL_BLOCKS`. @@ -262,14 +269,14 @@ mod tests { fn zero_height_disables_sweep() { let set = ReservationSet::default(); let a = outpoint(0x07, 0); - set.reserve(&[a], 0); + set.reserve(&[a], 0, ReservationToken::next()); // 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); + set.reserve(&[a], 1, ReservationToken::next()); assert!(set.reserved(1 + RESERVATION_TTL_BLOCKS).is_empty()); } @@ -277,10 +284,10 @@ mod tests { fn re_reserving_refreshes_the_ttl_height() { let set = ReservationSet::default(); let a = outpoint(0x06, 0); - set.reserve(&[a], 100); + set.reserve(&[a], 100, ReservationToken::next()); // A later reserve replaces the stored height, so the TTL is measured // from the most recent reservation, not the first. - set.reserve(&[a], 150); + set.reserve(&[a], 150, ReservationToken::next()); assert_eq!(set.reserved(150 + RESERVATION_TTL_BLOCKS - 1), HashSet::from([a])); assert!(set.reserved(150 + RESERVATION_TTL_BLOCKS).is_empty()); @@ -293,18 +300,16 @@ mod tests { 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); + set.reserve(&[a], 1, ReservationToken::next()); assert!(clone.reserved(1).contains(&a)); } #[test] - fn each_reserve_call_mints_a_distinct_token() { - let set = ReservationSet::default(); - // Two reserves at the SAME height must still get different tokens — the - // whole point of not keying ownership on height, which collides. - let token_a = set.reserve(&[outpoint(0x10, 0)], 100); - let token_b = set.reserve(&[outpoint(0x11, 0)], 100); - assert_ne!(token_a, token_b); + fn each_build_mints_a_distinct_token() { + // Two builds at the SAME height must still get different tokens — the + // whole point of not keying ownership on height, which collides. The + // counter is process-wide, so this holds across accounts' sets too. + assert_ne!(ReservationToken::next(), ReservationToken::next()); } #[test] @@ -313,8 +318,9 @@ mod tests { let mine = outpoint(0x20, 0); let theirs = outpoint(0x21, 0); - let my_token = set.reserve(&[mine], 100); - let _their_token = set.reserve(&[theirs], 100); + let my_token = ReservationToken::next(); + set.reserve(&[mine], 100, my_token); + set.reserve(&[theirs], 100, ReservationToken::next()); // Releasing with my token frees only my outpoint; theirs is untouched. set.release_if_owner(&[mine, theirs], my_token); @@ -326,11 +332,12 @@ mod tests { fn release_if_owner_is_a_noop_for_unreserved_or_wrong_token() { let set = ReservationSet::default(); let a = outpoint(0x22, 0); - let stale_token = set.reserve(&[a], 100); + let stale_token = ReservationToken::next(); + set.reserve(&[a], 100, stale_token); // Simulate the outpoint being released and re-reserved by someone else. set.release([&a]); - let _new_token = set.reserve(&[a], 100); + set.reserve(&[a], 100, ReservationToken::next()); // The stale token no longer owns `a`, so its release must not remove it. set.release_if_owner(&[a], stale_token); @@ -354,14 +361,16 @@ mod tests { let x = outpoint(0x30, 0); // Build A reserves X. - let token_a = set.reserve(&[x], 100); + let token_a = ReservationToken::next(); + set.reserve(&[x], 100, token_a); assert!(set.reserved(100).contains(&x)); // TTL sweep reclaims A's reservation mid-await (modeled by advancing the // height past the TTL so the next reserve's sweep drops A's entry)... let swept_height = 100 + RESERVATION_TTL_BLOCKS; // ...and build B re-reserves the very same outpoint under a new token. - let token_b = set.reserve(&[x], swept_height); + let token_b = ReservationToken::next(); + set.reserve(&[x], swept_height, token_b); assert_ne!(token_a, token_b); assert!(set.reserved(swept_height).contains(&x)); diff --git a/key-wallet/src/tests/spent_outpoints_tests.rs b/key-wallet/src/tests/spent_outpoints_tests.rs index 6e80f18fd..88eadc42f 100644 --- a/key-wallet/src/tests/spent_outpoints_tests.rs +++ b/key-wallet/src/tests/spent_outpoints_tests.rs @@ -6,6 +6,7 @@ use dashcore::{BlockHash, TxIn, Txid}; use crate::account::{AccountType, StandardAccountType, TransactionRecord}; use crate::managed_account::managed_account_trait::ManagedAccountTrait; +use crate::managed_account::reservation::ReservationToken; use crate::managed_account::transaction_record::TransactionDirection; use crate::managed_account::ManagedCoreFundsAccount; use crate::test_utils::TestWalletContext; @@ -77,7 +78,7 @@ fn reservations_are_not_persisted() { let account = ManagedCoreFundsAccount::dummy_bip44(); let outpoint = OutPoint::new(Txid::from([0x42; 32]), 0); - account.reservations().reserve(&[outpoint], 0); + account.reservations().reserve(&[outpoint], 0, ReservationToken::next()); assert!(account.reservations().reserved(0).contains(&outpoint)); let json = serde_json::to_string(&account).unwrap(); @@ -96,7 +97,7 @@ async fn processing_a_spend_releases_its_reservation() { let account = ctx.managed_wallet.first_bip44_managed_account_mut().expect("BIP44 account"); assert!(account.utxos.contains_key(&funded)); - account.reservations().reserve(&[funded], 0); + account.reservations().reserve(&[funded], 0, ReservationToken::next()); assert!(account.reservations().reserved(0).contains(&funded)); let spend = spending_tx(&[funded]); @@ -114,7 +115,7 @@ async fn processing_a_spend_releases_its_reservation() { 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); + account.reservations().reserve(&[second_funded], 0, ReservationToken::next()); assert!(account.reservations().reserved(0).contains(&second_funded)); let block_hash = BlockHash::from_slice(&[7u8; 32]).expect("hash"); diff --git a/key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs b/key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs index 26595961d..6aca94b34 100644 --- a/key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs +++ b/key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs @@ -319,7 +319,7 @@ impl ManagedWalletInfo { builder = builder.set_selection_strategy(SelectionStrategy::All); } let (transaction, fee, reservation_token) = builder - .set_funding(funds_acc, acc) + .add_funding(funds_acc, acc) .require_final_inputs() .build_signed_reserved(wallet, |addr| funds_acc.address_derivation_path(&addr)) .await?; @@ -446,7 +446,7 @@ impl ManagedWalletInfo { builder = builder.set_selection_strategy(SelectionStrategy::All); } let (transaction, fee, reservation_token) = builder - .set_funding(funds_acc, &acc) + .add_funding(funds_acc, &acc) .require_final_inputs() .build_signed_reserved(signer, |addr| funds_acc.address_derivation_path(&addr)) .await?; 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 076581ed6..a687b05b9 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,7 @@ use dashcore_hashes::Hash; use secp256k1::ecdsa::Signature; use secp256k1::{Message, PublicKey, Secp256k1}; use std::cmp::Ordering; +use std::collections::HashSet; /// A transaction with more inputs would exceed the relay standard-size cap (~100 KB at ~148 /// bytes/signed input) and be rejected by the network @@ -51,10 +52,11 @@ pub struct TransactionBuilder { require_final_inputs: bool, /// 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, + /// Reservation set of each funding account paired with the outpoints that + /// account contributed, captured by `add_funding`. Reservations live on the + /// account that holds the UTXO, so each account reserves its own share of + /// the chosen inputs — all under the one token this build is stamped with. + funding: Vec<(ReservationSet, HashSet)>, } impl Default for TransactionBuilder { @@ -75,7 +77,7 @@ impl TransactionBuilder { selection_strategy: SelectionStrategy::BranchAndBound, require_final_inputs: false, special_payload: None, - reservations: None, + funding: Vec::new(), } } @@ -100,26 +102,36 @@ impl TransactionBuilder { self } - /// Seed the builder with the funding account's spendable UTXOs, skipping any - /// already reserved by another in-flight build. + /// Add a funding account's spendable UTXOs to the candidate input set, + /// skipping any already reserved by another in-flight build. + /// + /// Call it once per funding account: coin selection then draws from the + /// union of their UTXOs, and the first account supplies the change + /// address. /// /// 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 + /// must therefore not be held across an `await` between `add_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 { + pub fn add_funding(mut self, funds_acc: &mut ManagedCoreFundsAccount, acc: &Account) -> Self { let reserved = funds_acc.reservations().reserved(self.current_height); - self.inputs = funds_acc + let candidates: Vec = 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.funding.push(( + funds_acc.reservations().clone(), + candidates.iter().map(|utxo| utxo.outpoint).collect(), + )); + self.inputs.extend(candidates); + if self.change_addr.is_none() { + self.change_addr = funds_acc.next_change_address(Some(&acc.account_xpub), true).ok(); + } self } @@ -284,13 +296,13 @@ impl TransactionBuilder { /// Select inputs, build the unsigned transaction, and reserve the chosen /// inputs. The optional [`ReservationToken`] is `Some` exactly when a - /// reservation set was attached (via [`set_funding`]) and identifies the + /// reservation set was attached (via [`add_funding`]) and identifies the /// reservation this build just took, so a caller that later abandons the /// build can release *only* its own inputs via /// [`ReservationSet::release_if_owner`]. It is `None` for builds with no /// reservation set (nothing was reserved, nothing to release). /// - /// [`set_funding`]: Self::set_funding + /// [`add_funding`]: Self::add_funding fn assemble_unsigned( mut self, ) -> Result<(Transaction, Vec, Option), BuilderError> { @@ -435,13 +447,22 @@ impl TransactionBuilder { // 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. Keep the - // stamped owner token so the caller can release only this reservation - // if the build is later abandoned (see `release_if_owner`). - let reservation_token = self.reservations.as_ref().map(|reservations| { - let outpoints: Vec = - selected_inputs.iter().map(|utxo| utxo.outpoint).collect(); - reservations.reserve(&outpoints, self.current_height) + // releases the reservation) or the TTL backstop reclaims it. Each + // account reserves the inputs it contributed, since its own set is the + // one its coin selection consults. Keep the stamped owner token so the + // caller can release only this reservation if the build is later + // abandoned (see `release_if_owner`). + let reservation_token = (!self.funding.is_empty()).then(|| { + let owner = ReservationToken::next(); + for (reservations, candidates) in &self.funding { + let outpoints: Vec = selected_inputs + .iter() + .map(|utxo| utxo.outpoint) + .filter(|outpoint| candidates.contains(outpoint)) + .collect(); + reservations.reserve(&outpoints, self.current_height, owner); + } + owner }); return Ok((transaction, selected_inputs, reservation_token)); @@ -480,7 +501,9 @@ impl TransactionBuilder { /// `.await` that releases the wallet lock — most importantly the platform /// broadcast path, which reserves inputs, awaits the broadcast, and on /// rejection must release them — and release with - /// [`ManagedCoreFundsAccount::release_reservation_if_owner`]. See + /// [`ManagedCoreFundsAccount::release_reservation_if_owner`] on *every* + /// funding account passed to [`Self::add_funding`]: each account reserves + /// only the inputs it contributed, in its own set. See /// `ReservationSet::release_if_owner` for why owner-guarded release is /// required (`dashpay/platform#4185`). pub fn build_unsigned_reserved( @@ -527,7 +550,7 @@ impl TransactionBuilder { S: TransactionSigner + ?Sized + Sync, P: Fn(Address) -> Option + Send, { - let reservations = self.reservations.clone(); + let funding = self.funding.clone(); let (tx, inputs, reservation) = self.assemble_unsigned()?; let total_input: u64 = inputs.iter().map(|utxo| utxo.value()).sum(); @@ -546,8 +569,10 @@ impl TransactionBuilder { let tx = match signer.sign_tx(tx, inputs, path_resolver).await { Ok(tx) => tx, Err(err) => { - if let (Some(reservations), Some(token)) = (&reservations, reservation) { - reservations.release_if_owner(&reserved, token); + if let Some(token) = reservation { + for (reservations, _) in &funding { + reservations.release_if_owner(&reserved, token); + } } return Err(err); } @@ -1248,10 +1273,10 @@ mod tests { funds.utxos.insert(reserved.outpoint, reserved.clone()); funds.utxos.insert(free.outpoint, free.clone()); - funds.reservations().reserve(&[reserved.outpoint], 200); + funds.reservations().reserve(&[reserved.outpoint], 200, ReservationToken::next()); let builder = - TransactionBuilder::new().set_current_height(200).set_funding(&mut funds, &account); + TransactionBuilder::new().set_current_height(200).add_funding(&mut funds, &account); let candidates: Vec = builder.inputs.iter().map(|utxo| utxo.outpoint).collect(); assert!(!candidates.contains(&reserved.outpoint)); @@ -1272,7 +1297,7 @@ mod tests { let builder = TransactionBuilder::new() .set_current_height(200) .set_fee_rate(FeeRate::normal()) - .set_funding(&mut funds, &account) + .add_funding(&mut funds, &account) .set_change_address(Address::dummy(Network::Testnet, 1)) .add_output(&destination, 500_000); @@ -1300,7 +1325,7 @@ mod tests { let (tx, _, _) = TransactionBuilder::new() .set_current_height(200) .set_fee_rate(FeeRate::normal()) - .set_funding(&mut funds, &account) + .add_funding(&mut funds, &account) .set_change_address(Address::dummy(Network::Testnet, 1)) .add_output(&destination, 500_000) .build_unsigned_reserved() diff --git a/key-wallet/src/wallet/managed_wallet_info/transaction_building.rs b/key-wallet/src/wallet/managed_wallet_info/transaction_building.rs index 425d36ac7..63d4f0259 100644 --- a/key-wallet/src/wallet/managed_wallet_info/transaction_building.rs +++ b/key-wallet/src/wallet/managed_wallet_info/transaction_building.rs @@ -4,122 +4,209 @@ use crate::managed_account::managed_account_trait::ManagedAccountTrait; use crate::signer::Signer; use crate::wallet::managed_wallet_info::coin_selection::SelectionStrategy; use crate::wallet::managed_wallet_info::fee::FeeRate; -use crate::wallet::managed_wallet_info::transaction_builder::{BuilderError, TransactionBuilder}; +use crate::wallet::managed_wallet_info::transaction_builder::{ + BuilderError, TransactionBuilder, TransactionSigner, +}; use crate::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; use crate::wallet::ManagedWalletInfo; -use crate::Wallet; +use crate::{DerivationPath, Wallet}; use dashcore::address::NetworkUnchecked; use dashcore::{Address, Transaction}; +use std::collections::HashMap; /// Account type preference for transaction building -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum AccountTypePreference { BIP44, BIP32, CoinJoin, } +impl AccountTypePreference { + /// The account types an empty source list draws from, in order. + /// + /// CoinJoin is deliberately absent: spending mixed outputs alongside + /// transparent ones in the same transaction links them and undoes the + /// mixing + pub const DEFAULT: [Self; 2] = [Self::BIP44, Self::BIP32]; +} + impl ManagedWalletInfo { + /// Build and sign a transaction funded from the given account types at + /// `source_index`, signing with the wallet's own keys. + /// + /// Coin selection draws from the union of those accounts' UTXOs, and the + /// first of them supplies the change address. An empty `sources` means + /// [`AccountTypePreference::DEFAULT`], skipping the types absent at the + /// index; a non-empty one is taken literally and every account named must + /// exist. pub async fn build_and_sign_transaction( &mut self, wallet: &Wallet, - source: AccountTypePreference, + sources: &[AccountTypePreference], source_index: u32, outputs: Vec<(Address, u64)>, fee_rate: FeeRate, strategy: SelectionStrategy, ) -> Result<(Transaction, u64), BuilderError> { - let height = self.last_processed_height(); - - let managed_account = match source { - AccountTypePreference::BIP44 => { - self.accounts.standard_bip44_accounts.get_mut(&source_index) - } - AccountTypePreference::BIP32 => { - self.accounts.standard_bip32_accounts.get_mut(&source_index) - } - AccountTypePreference::CoinJoin => { - self.accounts.coinjoin_accounts.get_mut(&source_index) - } - } - .ok_or_else(|| { - BuilderError::AccountNotFound(format!("managed account {source:?} #{source_index}")) - })?; - let account = match source { - AccountTypePreference::BIP44 => wallet.get_bip44_account(source_index), - AccountTypePreference::BIP32 => wallet.get_bip32_account(source_index), - AccountTypePreference::CoinJoin => wallet.get_coinjoin_account(source_index), - } - .ok_or_else(|| { - BuilderError::AccountNotFound(format!("wallet account {source:?} #{source_index}")) - })?; - - let mut tx_builder = TransactionBuilder::new() - .set_fee_rate(fee_rate) - .set_selection_strategy(strategy) - .set_current_height(height) - .set_funding(managed_account, account); + self.build_and_sign(wallet, sources, source_index, outputs, fee_rate, strategy, wallet) + .await + } - for (address, value) in outputs { - let checked = address.require_network(wallet.network).map_err(|e| { - BuilderError::InvalidData(format!("Output address network mismatch: {}", e)) - })?; - tx_builder = tx_builder.add_output(&checked, value); - } + /// [`Self::build_and_sign_transaction`] with signing delegated to an + /// external [`Signer`], so the private keys never have to be held by this + /// process. + #[allow(clippy::too_many_arguments)] + pub async fn build_and_sign_transaction_with_signer( + &mut self, + wallet: &Wallet, + sources: &[AccountTypePreference], + source_index: u32, + outputs: Vec<(Address, u64)>, + fee_rate: FeeRate, + strategy: SelectionStrategy, + signer: &S, + ) -> Result<(Transaction, u64), BuilderError> { + self.build_and_sign(wallet, sources, source_index, outputs, fee_rate, strategy, signer) + .await + } - tx_builder.build_signed(wallet, |addr| managed_account.address_derivation_path(&addr)).await + /// [`Self::build_and_sign_transaction`] without the signing step, for + /// callers that sign the transaction themselves. Its inputs are reserved + /// just the same, so a build that is never broadcast holds them until the + /// reservation's TTL reclaims them. + pub fn build_unsigned_transaction( + &mut self, + wallet: &Wallet, + sources: &[AccountTypePreference], + source_index: u32, + outputs: Vec<(Address, u64)>, + fee_rate: FeeRate, + strategy: SelectionStrategy, + ) -> Result<(Transaction, u64), BuilderError> { + let (builder, _paths) = + self.funded_builder(wallet, sources, source_index, outputs, fee_rate, strategy)?; + let (transaction, fee, _reservation) = builder.build_unsigned_reserved()?; + Ok((transaction, fee)) } #[allow(clippy::too_many_arguments)] - pub async fn build_and_sign_transaction_with_signer( + async fn build_and_sign( &mut self, wallet: &Wallet, - source: AccountTypePreference, + sources: &[AccountTypePreference], source_index: u32, outputs: Vec<(Address, u64)>, fee_rate: FeeRate, strategy: SelectionStrategy, signer: &S, ) -> Result<(Transaction, u64), BuilderError> { - let height = self.last_processed_height(); + let (builder, paths) = + self.funded_builder(wallet, sources, source_index, outputs, fee_rate, strategy)?; + builder.build_signed(signer, move |addr| paths.get(&addr).cloned()).await + } - let managed_account = match source { - AccountTypePreference::BIP44 => { - self.accounts.standard_bip44_accounts.get_mut(&source_index) - } - AccountTypePreference::BIP32 => { - self.accounts.standard_bip32_accounts.get_mut(&source_index) - } - AccountTypePreference::CoinJoin => { - self.accounts.coinjoin_accounts.get_mut(&source_index) - } - } - .ok_or_else(|| { - BuilderError::AccountNotFound(format!("managed account {source:?} #{source_index}")) - })?; - let account = match source { - AccountTypePreference::BIP44 => wallet.get_bip44_account(source_index), - AccountTypePreference::BIP32 => wallet.get_bip32_account(source_index), - AccountTypePreference::CoinJoin => wallet.get_coinjoin_account(source_index), - } - .ok_or_else(|| { - BuilderError::AccountNotFound(format!("wallet account {source:?} #{source_index}")) - })?; + /// A builder carrying the outputs and the funding of `sources`, ready to be + /// built with or without signing. + fn funded_builder( + &mut self, + wallet: &Wallet, + sources: &[AccountTypePreference], + source_index: u32, + outputs: Vec<(Address, u64)>, + fee_rate: FeeRate, + strategy: SelectionStrategy, + ) -> Result<(TransactionBuilder, HashMap), BuilderError> { + let outputs = outputs + .into_iter() + .map(|(address, value)| { + address.require_network(wallet.network).map(|checked| (checked, value)).map_err( + |e| { + BuilderError::InvalidData(format!("Output address network mismatch: {}", e)) + }, + ) + }) + .collect::, _>>()?; - let mut tx_builder = TransactionBuilder::new() + let height = self.last_processed_height(); + let builder = TransactionBuilder::new() .set_fee_rate(fee_rate) .set_selection_strategy(strategy) - .set_current_height(height) - .set_funding(managed_account, account); + .set_current_height(height); + + let (mut builder, paths) = self.fund(wallet, sources, source_index, builder)?; for (address, value) in outputs { - let checked = address.require_network(wallet.network).map_err(|e| { - BuilderError::InvalidData(format!("Output address network mismatch: {}", e)) - })?; - tx_builder = tx_builder.add_output(&checked, value); + builder = builder.add_output(&address, value); } - tx_builder.build_signed(signer, |addr| managed_account.address_derivation_path(&addr)).await + Ok((builder, paths)) + } + + /// Seed `builder` with the UTXOs of every funding account named by + /// `sources` at `source_index`, returning it alongside the derivation path + /// of each candidate input address, since the inputs can come from + /// different accounts. + fn fund( + &mut self, + wallet: &Wallet, + sources: &[AccountTypePreference], + source_index: u32, + mut builder: TransactionBuilder, + ) -> Result<(TransactionBuilder, HashMap), BuilderError> { + let named_explicitly = !sources.is_empty(); + let preferences = if named_explicitly { + sources + } else { + &AccountTypePreference::DEFAULT + }; + + let mut paths = HashMap::new(); + let mut funded = 0usize; + + for &preference in preferences { + let account = match preference { + AccountTypePreference::BIP44 => wallet.get_bip44_account(source_index), + AccountTypePreference::BIP32 => wallet.get_bip32_account(source_index), + AccountTypePreference::CoinJoin => wallet.get_coinjoin_account(source_index), + }; + let managed_account = match preference { + AccountTypePreference::BIP44 => { + self.accounts.standard_bip44_accounts.get_mut(&source_index) + } + AccountTypePreference::BIP32 => { + self.accounts.standard_bip32_accounts.get_mut(&source_index) + } + AccountTypePreference::CoinJoin => { + self.accounts.coinjoin_accounts.get_mut(&source_index) + } + }; + + let (Some(account), Some(managed_account)) = (account, managed_account) else { + if named_explicitly { + return Err(BuilderError::AccountNotFound(format!( + "account {preference:?} #{source_index}" + ))); + } + continue; + }; + + for utxo in managed_account.utxos.values() { + if let Some(path) = managed_account.address_derivation_path(&utxo.address) { + paths.insert(utxo.address.clone(), path); + } + } + builder = builder.add_funding(managed_account, account); + funded += 1; + } + + if funded == 0 { + return Err(BuilderError::AccountNotFound(format!( + "no funding account of any type at index {source_index}" + ))); + } + + Ok((builder, paths)) } } #[cfg(test)] @@ -539,7 +626,7 @@ mod tests { let result = info .build_and_sign_transaction_with_signer( &wallet, - AccountTypePreference::BIP44, + &[AccountTypePreference::BIP44], 99, dest_outputs(100_000), FeeRate::normal(), @@ -578,7 +665,7 @@ mod tests { let result = info .build_and_sign_transaction_with_signer( &wallet, - AccountTypePreference::BIP44, + &[AccountTypePreference::BIP44], 0, dest_outputs(100_000), FeeRate::normal(), @@ -644,7 +731,7 @@ mod tests { let (tx, fee) = info .build_and_sign_transaction_with_signer( &wallet, - AccountTypePreference::BIP44, + &[AccountTypePreference::BIP44], 0, dest_outputs(send_amount), FeeRate::normal(), @@ -685,7 +772,7 @@ mod tests { let result = info .build_and_sign_transaction_with_signer( &wallet, - AccountTypePreference::BIP44, + &[AccountTypePreference::BIP44], 0, dest_outputs(500_000), FeeRate::normal(), @@ -736,7 +823,7 @@ mod tests { let result = info .build_and_sign_transaction_with_signer( &wallet, - AccountTypePreference::BIP44, + &[AccountTypePreference::BIP44], 0, outputs, FeeRate::normal(), @@ -750,4 +837,88 @@ mod tests { result.err() ); } + + // -- Multi-account funding -- + + use dashcore::{OutPoint, TxOut}; + use std::collections::HashSet; + use test_case::test_case; + + /// Put a confirmed 300k UTXO on a fresh receive address of the account at + /// index 0 and return its outpoint. + fn fund( + wallet: &Wallet, + info: &mut ManagedWalletInfo, + preference: AccountTypePreference, + txid_byte: u8, + ) -> OutPoint { + let (account_xpub, account) = match preference { + AccountTypePreference::BIP32 => ( + wallet.get_bip32_account(0).unwrap().account_xpub, + info.accounts.standard_bip32_accounts.get_mut(&0).unwrap(), + ), + _ => ( + wallet.get_bip44_account(0).unwrap().account_xpub, + info.accounts.standard_bip44_accounts.get_mut(&0).unwrap(), + ), + }; + let address = account.next_receive_address(Some(&account_xpub), true).unwrap(); + let outpoint = OutPoint { + txid: Txid::from_byte_array([txid_byte; 32]), + vout: 0, + }; + account.utxos.insert( + outpoint, + Utxo { + outpoint, + txout: TxOut { + value: 300_000, + script_pubkey: address.script_pubkey(), + }, + address, + height: 1000, + is_coinbase: false, + is_confirmed: true, + is_instantlocked: false, + is_locked: false, + is_trusted: false, + }, + ); + outpoint + } + + /// Neither account covers the 500k target on its own, so the build only + /// succeeds by pooling both — and signing them proves the derivation paths + /// were collected across both accounts. + #[test_case(&[AccountTypePreference::BIP44, AccountTypePreference::BIP32] ; "named explicitly")] + #[test_case(&[] ; "empty list draws from every type")] + #[tokio::test] + async fn funding_pools_across_account_types(sources: &[AccountTypePreference]) { + let (wallet, mut info) = test_wallet_and_info(); + let bip44 = fund(&wallet, &mut info, AccountTypePreference::BIP44, 0x11); + let bip32 = fund(&wallet, &mut info, AccountTypePreference::BIP32, 0x22); + info.update_last_processed_height(1100); + + let (tx, _fee) = info + .build_and_sign_transaction( + &wallet, + sources, + 0, + dest_outputs(500_000), + FeeRate::normal(), + SelectionStrategy::BranchAndBound, + ) + .await + .expect("a 500k send funded by two 300k accounts"); + + let spent: HashSet = tx.input.iter().map(|txin| txin.previous_output).collect(); + assert_eq!(spent, HashSet::from([bip44, bip32])); + + // Each account reserves what it contributed, in its own set, so a build + // funded from only one of them still skips the spent-to-be UTXO. + let bip44_account = info.accounts.standard_bip44_accounts.get(&0).unwrap(); + let bip32_account = info.accounts.standard_bip32_accounts.get(&0).unwrap(); + assert_eq!(bip44_account.reservations().reserved(1100), HashSet::from([bip44])); + assert_eq!(bip32_account.reservations().reserved(1100), HashSet::from([bip32])); + } }