diff --git a/key-wallet-ffi/src/transaction.rs b/key-wallet-ffi/src/transaction.rs index eb09a0201..1622fa010 100644 --- a/key-wallet-ffi/src/transaction.rs +++ b/key-wallet-ffi/src/transaction.rs @@ -744,12 +744,33 @@ impl From for AssetLockFundingType { } } +/// The funding sources an asset lock pools, in order — the FIRST source +/// (BIP44) supplies the change address, so change from a pooled asset lock +/// always returns to the transparent primary account. +/// +/// CoinJoin is deliberately absent (spending mixed outputs alongside +/// transparent ones links them and undoes the mixing — the same reasoning as +/// `AccountTypePreference::DEFAULT`), and so are a contact's watch-only +/// `DashpayExternalAccount` coins, which `AllDashpayReceivingFunds` excludes by +/// construction (it selects only the receiving side the local seed can sign). +const ASSET_LOCK_FUNDING_SOURCES: [AccountTypePreference; 3] = [ + AccountTypePreference::BIP44, + AccountTypePreference::BIP32, + AccountTypePreference::AllDashpayReceivingFunds, +]; + /// Build and sign an asset lock transaction for Core to Platform transfers. /// /// Creates a special transaction (type 8) with `AssetLockPayload` that locks /// Dash for Platform credits. Derives one unique private key per credit output /// from the specified funding account types. /// +/// Funding is POOLED across [`ASSET_LOCK_FUNDING_SOURCES`]: coin selection +/// draws from the union of the BIP44, BIP32 and DashPay contact-receiving +/// accounts the wallet has, so a lock no longer needs the whole amount sitting +/// in one account (and change returns to BIP44). `account_index` addresses the +/// standard families; DashPay accounts span their own indices. +/// /// # Parameters /// /// - `funding_types`: Array of `credit_outputs_count` funding account types, @@ -840,9 +861,8 @@ pub unsafe extern "C" fn wallet_build_and_sign_asset_lock_transaction( let result = unwrap_or_return!(managed_wallet.build_asset_lock( wallet_ref.inner(), - key_wallet::wallet::managed_wallet_info::asset_lock_builder::AssetLockFundingAccount::Bip44 { - account_index, - }, + &ASSET_LOCK_FUNDING_SOURCES, + account_index, fundings, fee_per_kb, false, 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 6aca94b34..4c5f0dc60 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 @@ -9,16 +9,22 @@ use dashcore::{OutPoint, Transaction, TxOut}; use secp256k1::PublicKey; use std::fmt; +use crate::account::AccountType; use crate::managed_account::managed_account_trait::ManagedAccountTrait; +use crate::managed_account::reservation::ReservationSet; use crate::managed_account::{ManagedCoreKeysAccount, ReservationToken}; 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_building::{ + AccountTypePreference, PooledFunding, +}; use crate::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; use crate::wallet::managed_wallet_info::ManagedWalletInfo; use crate::wallet::Wallet; use crate::DerivationPath; +use std::collections::HashSet; /// Which funding account to derive the one-time key from. #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] @@ -38,14 +44,19 @@ pub enum AssetLockFundingType { AssetLockShieldedAddressTopUp, } -/// Which wallet account supplies the funding UTXOs (and signs the inputs) -/// of an asset lock transaction. +/// A single wallet account that supplies the funding UTXOs (and signs the +/// inputs) of a whole-balance **drain** asset lock. /// -/// `Bip44` is the standard spendable balance — the historical behavior of -/// the builders below. `CoinJoin` lets mixed coins fund an asset lock -/// directly, without first sweeping them through a transparent BIP44 -/// address (which would link the mixed UTXOs to a reusable transparent -/// address for an extra hop). +/// The asset-lock builders take a *list* of [`AccountTypePreference`] sources +/// and pool them; this is the narrower vocabulary of the drain flows, which +/// name exactly one account by construction (a drain has no change output, so +/// "which account supplies change" — the thing a pooled list decides — does not +/// arise). `Bip44` is the standard spendable balance; `CoinJoin` lets mixed +/// coins fund an asset lock directly, without first sweeping them through a +/// transparent BIP44 address (which would link the mixed UTXOs to a reusable +/// transparent address for an extra hop). +/// +/// Convert with [`AccountTypePreference::from`] to hand one to a builder. #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum AssetLockFundingAccount { @@ -75,6 +86,19 @@ impl AssetLockFundingAccount { } } +impl From for AccountTypePreference { + fn from(account: AssetLockFundingAccount) -> Self { + match account { + AssetLockFundingAccount::Bip44 { + .. + } => Self::BIP44, + AssetLockFundingAccount::CoinJoin { + .. + } => Self::CoinJoin, + } + } +} + /// Per-credit-output funding specification. pub struct CreditOutputFunding { /// The credit output (script + amount). @@ -111,17 +135,29 @@ pub struct AssetLockResult { /// ordering and variant semantics. pub keys: AssetLockCreditKeys, /// Owner token for the reservation this build took on the funding inputs, - /// or `None` if the funding account carried no reservation set. + /// or `None` if no funding account carried a reservation set. /// /// The caller broadcasts `transaction` and, on a rejected broadcast, must /// release the reserved inputs with /// [`ManagedCoreFundsAccount::release_reservation_if_owner`] passing this - /// token — never the unconditional `release_reservation`. See + /// token — never the unconditional `release_reservation` — on **every** + /// account in [`Self::funding_accounts`]. See /// `ReservationSet::release_if_owner` for why owner-guarded release is /// required here (`dashpay/platform#4185`). /// /// [`ManagedCoreFundsAccount::release_reservation_if_owner`]: crate::managed_account::ManagedCoreFundsAccount::release_reservation_if_owner pub reservation_token: Option, + /// The accounts that actually contributed inputs to `transaction`, and so + /// the accounts holding a share of this build's reservation — a pooled + /// build reserves in each contributing account's own set, all under the one + /// [`Self::reservation_token`]. + /// + /// This is the *contributor* list, not everything the source list offered: + /// coin selection routinely takes nothing from most offered accounts, and a + /// list naming every DashPay contact would make the caller's release and + /// bookkeeping scale with the address book while claiming contributions + /// that never happened. + pub funding_accounts: Vec, } /// Errors specific to asset lock transaction building. @@ -143,8 +179,6 @@ pub enum AssetLockError { SigningFailed(String), /// The wallet does not have a private key (watch-only). WatchOnlyWallet, - /// The specified funding account (BIP44 or CoinJoin, by index) was not found. - AccountNotFound(u32), /// No change address available. NoChangeAddress, /// Underlying transaction builder error. @@ -164,7 +198,6 @@ impl fmt::Display for AssetLockError { Self::Signer(msg) => write!(f, "Signer error: {msg}"), Self::SigningFailed(msg) => write!(f, "Signing failed: {msg}"), Self::WatchOnlyWallet => write!(f, "Cannot sign with watch-only wallet"), - Self::AccountNotFound(idx) => write!(f, "funding account {} not found", idx), Self::NoChangeAddress => write!(f, "No change address available"), Self::Builder(e) => write!(f, "Transaction builder error: {e}"), } @@ -218,12 +251,17 @@ fn resolve_funding_account( } } -/// Shared guard for both asset-lock builders: a drain rewrites exactly one -/// credit output, and CoinJoin accounts have no change-address pool semantics -/// for asset locks (change would need re-denomination), so they only support -/// the whole-balance drain. -fn validate_drain_funding( - funding_account: AssetLockFundingAccount, +/// Shared guard for both asset-lock builders, run before any wallet state is +/// touched. +/// +/// * A drain rewrites exactly one credit output, so it requires exactly one. +/// * CoinJoin funding is drain-only and must be the *sole* source. CoinJoin +/// accounts have no change-address pool semantics for asset locks (change +/// would need re-denomination), and pooling mixed coins with transparent ones +/// in a single transaction links them and undoes the mixing — the same +/// reasoning that keeps CoinJoin out of [`AccountTypePreference::DEFAULT`]. +fn validate_funding_sources( + sources: &[AccountTypePreference], credit_output_count: usize, drain: bool, ) -> Result<(), AssetLockError> { @@ -232,14 +270,46 @@ fn validate_drain_funding( "drain asset lock requires exactly one credit output".into(), ))); } - if matches!(funding_account, AssetLockFundingAccount::CoinJoin { .. }) && !drain { + let has_coinjoin = sources.contains(&AccountTypePreference::CoinJoin); + if has_coinjoin && !drain { return Err(AssetLockError::Builder(BuilderError::InvalidData( "CoinJoin-funded asset locks support drain mode only".into(), ))); } + if has_coinjoin && sources.len() > 1 { + return Err(AssetLockError::Builder(BuilderError::InvalidData( + "CoinJoin funding cannot be pooled with other sources: spending mixed outputs \ + alongside transparent ones in one transaction links them and undoes the mixing" + .into(), + ))); + } Ok(()) } +/// The accounts among `offered` that contributed an input to `transaction`. +/// +/// Only these hold a share of the build's reservation, so this is what the +/// caller must reconcile on a rejected broadcast. An outpoint is attributed to +/// an account when that account still holds it as a UTXO — a build reserves its +/// inputs but does not remove them, so this is exact right after the build. +fn contributing_accounts( + accounts: &crate::account::ManagedAccountCollection, + offered: &[AccountType], + transaction: &Transaction, +) -> Vec { + let spent: HashSet = + transaction.input.iter().map(|input| input.previous_output).collect(); + offered + .iter() + .copied() + .filter(|account_type| { + accounts.funds_account(account_type).is_some_and(|account| { + account.utxos.keys().any(|outpoint| spent.contains(outpoint)) + }) + }) + .collect() +} + impl ManagedWalletInfo { /// Build and sign an asset lock transaction. /// @@ -249,16 +319,26 @@ impl ManagedWalletInfo { /// The transaction is built first, and keys are only derived after a successful /// build — so no addresses are consumed if the build fails. /// - /// `funding_account` picks which account family supplies (and signs) the - /// funding UTXOs — see [`AssetLockFundingAccount`]. `drain` locks the - /// account's whole spendable balance: every final UTXO is consumed and - /// the single credit output's value is rewritten to `Σ inputs − fee` - /// (the caller's credit-output value is ignored; exactly one credit - /// output is required). + /// `funding_sources` picks which account families supply (and sign) the + /// funding UTXOs; coin selection draws from the union of their UTXOs and + /// the first source supplies the change address. A single source is an + /// explicit request for that one account and errors if it is absent; a + /// pooled list skips the sources this wallet has nothing for. `source_index` + /// addresses the standard families (BIP44/BIP32/CoinJoin); DashPay set + /// selectors span their own indices. See + /// [`ManagedWalletInfo::build_and_sign_transaction`] for the shared + /// source-list semantics. + /// + /// `drain` locks the sourced accounts' whole spendable balance: every final + /// UTXO is consumed and the single credit output's value is rewritten to + /// `Σ inputs − fee` (the caller's credit-output value is ignored; exactly + /// one credit output is required). CoinJoin funding is drain-only and + /// cannot be pooled — see [`validate_funding_sources`]. pub async fn build_asset_lock( &mut self, wallet: &Wallet, - funding_account: AssetLockFundingAccount, + funding_sources: &[AccountTypePreference], + source_index: u32, credit_output_fundings: Vec, fee_per_kb: u64, drain: bool, @@ -271,38 +351,7 @@ impl ManagedWalletInfo { let network = self.network; let height = self.last_processed_height(); - let account_index = funding_account.account_index(); - let acc = match funding_account { - AssetLockFundingAccount::Bip44 { - .. - } => wallet - .get_bip44_account(account_index) - .ok_or(AssetLockError::AccountNotFound(account_index))?, - AssetLockFundingAccount::CoinJoin { - .. - } => wallet - .get_coinjoin_account(account_index) - .ok_or(AssetLockError::AccountNotFound(account_index))?, - }; - - let funds_acc = match funding_account { - AssetLockFundingAccount::Bip44 { - .. - } => self - .accounts - .standard_bip44_accounts - .get_mut(&account_index) - .ok_or(AssetLockError::AccountNotFound(account_index))?, - AssetLockFundingAccount::CoinJoin { - .. - } => self - .accounts - .coinjoin_accounts - .get_mut(&account_index) - .ok_or(AssetLockError::AccountNotFound(account_index))?, - }; - - validate_drain_funding(funding_account, credit_output_fundings.len(), drain)?; + validate_funding_sources(funding_sources, credit_output_fundings.len(), drain)?; let credit_outputs: Vec = credit_output_fundings.iter().map(|f| f.output.clone()).collect(); @@ -314,27 +363,46 @@ impl ManagedWalletInfo { .set_current_height(height) .set_special_payload(TransactionPayload::AssetLockPayloadType(AssetLockPayload::new( credit_outputs, - ))); + ))) + .require_final_inputs(); if drain { builder = builder.set_selection_strategy(SelectionStrategy::All); } - let (transaction, fee, reservation_token) = builder - .add_funding(funds_acc, acc) - .require_final_inputs() - .build_signed_reserved(wallet, |addr| funds_acc.address_derivation_path(&addr)) - .await?; - - // The build above reserved the funding inputs. Clone the reservation - // handle (a shared `Arc` view of the same set) now, before the loop - // below re-borrows `self.accounts` — a mid-loop failure can no longer - // reach `funds_acc` to release, and the caller never received the token - // to release it either, so a leaked reservation would strand the - // already-signed inputs until the 24-block TTL sweep. Owner-guarded - // release only (see `ReservationSet::release_if_owner`, - // `dashpay/platform#4185`). - let reservations = funds_acc.reservations().clone(); + let PooledFunding { + builder, + paths, + accounts: offered, + } = self.fund(wallet, funding_sources, source_index, builder)?; + + // The build below reserves the funding inputs in each contributing + // account's own set. Clone every offered account's reservation handle + // (a shared `Arc` view of the same set) now, before the loop further + // down re-borrows `self.accounts`: a mid-loop failure can no longer + // reach those accounts to release, and the caller never received the + // token to release with either, so a leaked reservation would strand + // the already-signed inputs until the 24-block TTL sweep. Offered + // rather than contributing, because the set is captured before the + // build tells us who contributed; releasing against an account that + // reserved nothing is a no-op. Owner-guarded release only (see + // `ReservationSet::release_if_owner`, `dashpay/platform#4185`). + let reservations: Vec = offered + .iter() + .filter_map(|account_type| self.accounts.funds_account(account_type)) + .map(|account| account.reservations().clone()) + .collect(); + + let (transaction, fee, reservation_token) = + builder.build_signed_reserved(wallet, move |addr| paths.get(&addr).cloned()).await?; + let reserved: Vec = transaction.input.iter().map(|input| input.previous_output).collect(); + let release_reservations = || { + if let Some(token) = reservation_token { + for set in &reservations { + set.release_if_owner(&reserved, token); + } + } + }; // Derive one private key per credit output. On any failure, release // this build's own reservation before returning. @@ -355,18 +423,18 @@ impl ManagedWalletInfo { })() { Ok(keys) => keys, Err(e) => { - if let Some(token) = reservation_token { - reservations.release_if_owner(&reserved, token); - } + release_reservations(); return Err(e); } }; + let funding_accounts = contributing_accounts(&self.accounts, &offered, &transaction); Ok(AssetLockResult { transaction, fee, keys: AssetLockCreditKeys::Private(keys), reservation_token, + funding_accounts, }) } @@ -386,11 +454,14 @@ impl ManagedWalletInfo { /// request signatures from the same signer when later consuming the /// credits on Platform. /// - /// `funding_account` / `drain` — see [`Self::build_asset_lock`]. + /// `funding_sources` / `source_index` / `drain` — see + /// [`Self::build_asset_lock`]. + #[allow(clippy::too_many_arguments)] pub async fn build_asset_lock_with_signer( &mut self, wallet: &Wallet, - funding_account: AssetLockFundingAccount, + funding_sources: &[AccountTypePreference], + source_index: u32, credit_output_fundings: Vec, fee_per_kb: u64, drain: bool, @@ -398,40 +469,7 @@ impl ManagedWalletInfo { ) -> Result { let height = self.last_processed_height(); - let account_index = funding_account.account_index(); - let acc = match funding_account { - AssetLockFundingAccount::Bip44 { - .. - } => wallet - .get_bip44_account(account_index) - .ok_or(AssetLockError::AccountNotFound(account_index))? - .clone(), - AssetLockFundingAccount::CoinJoin { - .. - } => wallet - .get_coinjoin_account(account_index) - .ok_or(AssetLockError::AccountNotFound(account_index))? - .clone(), - }; - - let funds_acc = match funding_account { - AssetLockFundingAccount::Bip44 { - .. - } => self - .accounts - .standard_bip44_accounts - .get_mut(&account_index) - .ok_or(AssetLockError::AccountNotFound(account_index))?, - AssetLockFundingAccount::CoinJoin { - .. - } => self - .accounts - .coinjoin_accounts - .get_mut(&account_index) - .ok_or(AssetLockError::AccountNotFound(account_index))?, - }; - - validate_drain_funding(funding_account, credit_output_fundings.len(), drain)?; + validate_funding_sources(funding_sources, credit_output_fundings.len(), drain)?; let credit_outputs: Vec = credit_output_fundings.iter().map(|f| f.output.clone()).collect(); @@ -441,26 +479,43 @@ impl ManagedWalletInfo { .set_current_height(height) .set_special_payload(TransactionPayload::AssetLockPayloadType(AssetLockPayload::new( credit_outputs, - ))); + ))) + .require_final_inputs(); if drain { builder = builder.set_selection_strategy(SelectionStrategy::All); } - let (transaction, fee, reservation_token) = builder - .add_funding(funds_acc, &acc) - .require_final_inputs() - .build_signed_reserved(signer, |addr| funds_acc.address_derivation_path(&addr)) - .await?; - - // The build above reserved the funding inputs. Clone the reservation - // handle (a shared `Arc` view of the same set) before the bookkeeping - // loop below re-borrows `self.accounts`, so a failure during Phase 1–3 - // — which runs after the transaction is already signed — can still + let PooledFunding { + builder, + paths, + accounts: offered, + } = self.fund(wallet, funding_sources, source_index, builder)?; + + // The build below reserves the funding inputs in each contributing + // account's own set. Clone every offered account's reservation handle + // (a shared `Arc` view of the same set) before the bookkeeping loop + // further down re-borrows `self.accounts`, so a failure during Phase + // 1–3 — which runs after the transaction is already signed — can still // release THIS build's reservation instead of stranding the signed // inputs until the 24-block TTL sweep. Owner-guarded release only (see // `ReservationSet::release_if_owner`, `dashpay/platform#4185`). - let reservations = funds_acc.reservations().clone(); + let reservations: Vec = offered + .iter() + .filter_map(|account_type| self.accounts.funds_account(account_type)) + .map(|account| account.reservations().clone()) + .collect(); + + let (transaction, fee, reservation_token) = + builder.build_signed_reserved(signer, move |addr| paths.get(&addr).cloned()).await?; + let reserved: Vec = transaction.input.iter().map(|input| input.previous_output).collect(); + let release_reservations = || { + if let Some(token) = reservation_token { + for set in &reservations { + set.release_if_owner(&reserved, token); + } + } + }; // Credit-output bookkeeping: for each funding, peek the next unused // path on its account, ask the signer for the matching pubkey, and @@ -516,18 +571,18 @@ impl ManagedWalletInfo { { Ok(keys) => keys, Err(e) => { - if let Some(token) = reservation_token { - reservations.release_if_owner(&reserved, token); - } + release_reservations(); return Err(e); } }; + let funding_accounts = contributing_accounts(&self.accounts, &offered, &transaction); Ok(AssetLockResult { transaction, fee, keys: AssetLockCreditKeys::Public(credit_output_keys), reservation_token, + funding_accounts, }) } } @@ -540,6 +595,7 @@ mod tests { use crate::{Network, Utxo}; use dashcore::{OutPoint, ScriptBuf, Txid}; use dashcore_hashes::Hash; + use test_case::test_case; fn test_credit_outputs(amounts: &[u64]) -> Vec { amounts @@ -659,9 +715,8 @@ mod tests { let result = info .build_asset_lock( &wallet, - AssetLockFundingAccount::CoinJoin { - account_index: 0, - }, + &[AccountTypePreference::CoinJoin], + 0, test_credit_outputs(&[0]), 1000, true, @@ -701,9 +756,8 @@ mod tests { let result = info .build_asset_lock( &wallet, - AssetLockFundingAccount::CoinJoin { - account_index: 0, - }, + &[AccountTypePreference::CoinJoin], + 0, test_credit_outputs(&[0, 0]), 1000, true, @@ -728,9 +782,8 @@ mod tests { let result = info .build_asset_lock( &wallet, - AssetLockFundingAccount::CoinJoin { - account_index: 0, - }, + &[AccountTypePreference::CoinJoin], + 0, test_credit_outputs(&[200_000]), 1000, false, @@ -751,7 +804,6 @@ mod tests { AssetLockError::WatchOnlyWallet.to_string(), "Cannot sign with watch-only wallet" ); - assert_eq!(AssetLockError::AccountNotFound(5).to_string(), "funding account 5 not found"); assert_eq!(AssetLockError::NoChangeAddress.to_string(), "No change address available"); } @@ -768,15 +820,7 @@ mod tests { async fn test_empty_credit_outputs_rejected() { let (wallet, mut info) = test_wallet_and_info(); let result = info - .build_asset_lock( - &wallet, - AssetLockFundingAccount::Bip44 { - account_index: 0, - }, - vec![], - 1000, - false, - ) + .build_asset_lock(&wallet, &[AccountTypePreference::BIP44], 0, vec![], 1000, false) .await; assert!(matches!(result, Err(AssetLockError::Builder(BuilderError::NoOutputs)))); } @@ -787,15 +831,17 @@ mod tests { let result = info .build_asset_lock( &wallet, - AssetLockFundingAccount::Bip44 { - account_index: 99, - }, + &[AccountTypePreference::BIP44], + 99, test_credit_outputs(&[100_000]), 1000, false, ) .await; - assert!(matches!(result, Err(AssetLockError::AccountNotFound(99)))); + assert!( + matches!(result, Err(AssetLockError::Builder(BuilderError::AccountNotFound(_)))), + "a single named source is strict: the absent account must be an error" + ); } #[tokio::test] @@ -805,9 +851,8 @@ mod tests { let result = info .build_asset_lock( &wallet, - AssetLockFundingAccount::Bip44 { - account_index: 0, - }, + &[AccountTypePreference::BIP44], + 0, test_credit_outputs(&[500_000]), 1000, false, @@ -833,9 +878,8 @@ mod tests { let result = info .build_asset_lock( &wallet, - AssetLockFundingAccount::Bip44 { - account_index: 0, - }, + &[AccountTypePreference::BIP44], + 0, test_credit_outputs(&[200_000]), 1000, false, @@ -860,9 +904,8 @@ mod tests { let result = info .build_asset_lock( &wallet, - AssetLockFundingAccount::Bip44 { - account_index: 0, - }, + &[AccountTypePreference::BIP44], + 0, test_credit_outputs(&[200_000]), 1000, false, @@ -987,9 +1030,8 @@ mod tests { let result = info .build_asset_lock_with_signer( &wallet, - AssetLockFundingAccount::Bip44 { - account_index: 0, - }, + &[AccountTypePreference::BIP44], + 0, vec![], 1000, false, @@ -1016,16 +1058,18 @@ mod tests { let result = info .build_asset_lock_with_signer( &wallet, - AssetLockFundingAccount::Bip44 { - account_index: 99, - }, + &[AccountTypePreference::BIP44], + 99, test_credit_outputs(&[100_000]), 1000, false, &signer, ) .await; - assert!(matches!(result, Err(AssetLockError::AccountNotFound(99)))); + assert!( + matches!(result, Err(AssetLockError::Builder(BuilderError::AccountNotFound(_)))), + "a single named source is strict: the absent account must be an error" + ); } #[tokio::test] @@ -1056,9 +1100,8 @@ mod tests { let result = info .build_asset_lock_with_signer( &wallet, - AssetLockFundingAccount::Bip44 { - account_index: 0, - }, + &[AccountTypePreference::BIP44], + 0, test_credit_outputs(&[100_000]), 1000, false, @@ -1096,9 +1139,8 @@ mod tests { let result = info .build_asset_lock_with_signer( &wallet, - AssetLockFundingAccount::Bip44 { - account_index: 0, - }, + &[AccountTypePreference::BIP44], + 0, fundings, 1000, false, @@ -1158,9 +1200,8 @@ mod tests { let result = info .build_asset_lock_with_signer( &wallet, - AssetLockFundingAccount::Bip44 { - account_index: 0, - }, + &[AccountTypePreference::BIP44], + 0, test_credit_outputs(&[500_000]), 1000, false, @@ -1173,4 +1214,353 @@ mod tests { result.err() ); } + + // -- Pooled funding -------------------------------------------------- + // + // Asset locks fund from a LIST of sources. These pin the three things the + // pooling has to get right: it really spans accounts, it tolerates the + // sources a wallet does not have, and every failure path after the build + // gives back the reservations it took — in *each* contributing account, + // since a pooled build reserves per account under one owner token. + + /// The default pooled set, mirroring platform's `ASSET_LOCK_FUNDING_SOURCES`. + const POOLED: [AccountTypePreference; 3] = [ + AccountTypePreference::BIP44, + AccountTypePreference::BIP32, + AccountTypePreference::AllDashpayReceivingFunds, + ]; + + /// Fund the BIP32 account at index 0 with a confirmed UTXO. + fn insert_funded_bip32_utxo( + info: &mut ManagedWalletInfo, + wallet: &Wallet, + txid_byte: u8, + value: u64, + ) -> OutPoint { + let account_xpub = wallet + .accounts + .standard_bip32_accounts + .get(&0) + .expect("default wallet has BIP32 account 0") + .account_xpub; + let account = info.accounts.standard_bip32_accounts.get_mut(&0).unwrap(); + let funding_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, + script_pubkey: funding_address.script_pubkey(), + }, + address: funding_address, + height: 1000, + is_coinbase: false, + is_confirmed: true, + is_instantlocked: false, + is_locked: false, + is_trusted: false, + }, + ); + outpoint + } + + fn bip44_0() -> AccountType { + AccountType::Standard { + index: 0, + standard_account_type: crate::account::StandardAccountType::BIP44Account, + } + } + + fn bip32_0() -> AccountType { + AccountType::Standard { + index: 0, + standard_account_type: crate::account::StandardAccountType::BIP32Account, + } + } + + /// A credit output whose one-time key comes from an identity top-up + /// account that does not exist, so credit-key derivation fails *after* the + /// transaction is built and signed — the window in which a pooled build + /// holds reservations it must give back. + fn credit_output_with_missing_key_account() -> Vec { + let mut fundings = test_credit_outputs(&[400_000]); + fundings[0].funding_type = AssetLockFundingType::IdentityTopUp; + fundings[0].identity_index = 7; + fundings + } + + /// Reserved outpoints across the two standard accounts at height 1100. + fn reserved_outpoints(info: &ManagedWalletInfo) -> HashSet { + [bip44_0(), bip32_0()] + .iter() + .filter_map(|at| info.accounts.funds_account(at)) + .flat_map(|account| account.reservations().reserved(1100)) + .collect() + } + + /// Neither standard account covers the lock on its own, so the build only + /// succeeds by pooling both — which also proves the derivation paths were + /// collected across accounts, since every input had to be signed. Change + /// goes back to BIP44 (the first source), each account reserves what it + /// contributed in its own set, and both are reported as funding accounts. + #[tokio::test] + async fn pooled_asset_lock_spans_the_standard_accounts() { + let (wallet, mut info) = test_wallet_and_info(); + let bip44 = insert_funded_utxo(&mut info, &wallet, 0x11, 300_000, true); + let bip32 = insert_funded_bip32_utxo(&mut info, &wallet, 0x22, 300_000); + info.update_last_processed_height(1100); + + let result = info + .build_asset_lock(&wallet, &POOLED, 0, test_credit_outputs(&[500_000]), 1000, false) + .await + .expect("a 500k lock funded by two 300k accounts"); + + let spent: HashSet = + result.transaction.input.iter().map(|txin| txin.previous_output).collect(); + assert_eq!(spent, HashSet::from([bip44, bip32]), "the lock must pool both accounts"); + for (i, txin) in result.transaction.input.iter().enumerate() { + assert!(!txin.script_sig.is_empty(), "input {i} not signed"); + } + + // Change returns to the FIRST source, not to whichever account happened + // to be selected from last. + let change = result + .transaction + .output + .iter() + .find(|out| !out.script_pubkey.is_op_return()) + .expect("600k in against a 500k lock leaves change"); + let bip44_account = info.accounts.standard_bip44_accounts.get(&0).unwrap(); + assert!( + bip44_account + .managed_account_type() + .all_script_pubkeys() + .contains(&change.script_pubkey), + "change must return to the BIP44 account" + ); + + // One token, but the reservation lives in each contributing account's + // own set — that set is the one its next coin selection consults. + assert!(result.reservation_token.is_some()); + assert_eq!(bip44_account.reservations().reserved(1100), HashSet::from([bip44])); + assert_eq!( + info.accounts.standard_bip32_accounts.get(&0).unwrap().reservations().reserved(1100), + HashSet::from([bip32]) + ); + assert_eq!( + result.funding_accounts.iter().copied().collect::>(), + HashSet::from([bip44_0(), bip32_0()]) + ); + } + + /// A pooled list names sources this wallet may have nothing for. Skipping + /// them is the point: a wallet with no DashPay contacts still funds an + /// asset lock, and only the accounts that contributed are reported. + #[tokio::test] + async fn pooled_sources_skip_what_the_wallet_does_not_have() { + let (wallet, mut info) = test_wallet_and_info(); + let bip44 = insert_funded_utxo(&mut info, &wallet, 0x11, 900_000, true); + info.update_last_processed_height(1100); + + let result = info + .build_asset_lock(&wallet, &POOLED, 0, test_credit_outputs(&[500_000]), 1000, false) + .await + .expect("no contacts and an empty BIP32 account must not block the build"); + + let spent: Vec = + result.transaction.input.iter().map(|txin| txin.previous_output).collect(); + assert_eq!(spent, vec![bip44]); + assert_eq!( + result.funding_accounts, + vec![bip44_0()], + "an account that contributed nothing is not a funding account" + ); + } + + /// A pooled list that funds nothing at all is still an error — leniency + /// skips absent sources, it does not invent funds. + #[tokio::test] + async fn pooled_sources_that_resolve_to_nothing_are_an_error() { + let (wallet, mut info) = test_wallet_and_info(); + info.update_last_processed_height(1100); + + let result = info + .build_asset_lock(&wallet, &POOLED, 99, test_credit_outputs(&[500_000]), 1000, false) + .await; + + assert!( + matches!(result, Err(AssetLockError::Builder(BuilderError::AccountNotFound(_)))), + "no account of any named source at index 99" + ); + } + + /// Mixed coins must never ride alongside transparent ones: pooling would + /// link them in a single transaction and undo the mixing. Rejected before + /// any wallet state is touched, drain or not. + #[test_case(true ; "drain")] + #[test_case(false ; "exact amount")] + #[tokio::test] + async fn coinjoin_cannot_be_pooled_with_transparent_sources(drain: bool) { + let (wallet, mut info) = test_wallet_and_info(); + insert_funded_coinjoin_utxo(&mut info, &wallet, 0x41, 900_000, true); + insert_funded_utxo(&mut info, &wallet, 0x11, 900_000, true); + info.update_last_processed_height(1100); + + let result = info + .build_asset_lock( + &wallet, + &[AccountTypePreference::CoinJoin, AccountTypePreference::BIP44], + 0, + test_credit_outputs(&[500_000]), + 1000, + drain, + ) + .await; + + assert!(matches!(result, Err(AssetLockError::Builder(BuilderError::InvalidData(_))))); + assert!( + reserved_outpoints(&info).is_empty() + && info + .accounts + .coinjoin_accounts + .get(&0) + .unwrap() + .reservations() + .reserved(1100) + .is_empty(), + "a rejected source list must not have reserved anything" + ); + } + + /// The failure window a pooled build opens: the transaction is already + /// built, signed and reserved when credit-key derivation fails. The caller + /// never receives the token, so nothing else can release those inputs — + /// they must be freed here, in EVERY contributing account, or the funds + /// stay stranded until the 24-block TTL sweep. + #[tokio::test] + async fn credit_key_failure_releases_the_reservation_in_every_pooled_account() { + let (wallet, mut info) = test_wallet_and_info(); + insert_funded_utxo(&mut info, &wallet, 0x11, 300_000, true); + insert_funded_bip32_utxo(&mut info, &wallet, 0x22, 300_000); + info.update_last_processed_height(1100); + + let result = info + .build_asset_lock( + &wallet, + &POOLED, + 0, + credit_output_with_missing_key_account(), + 1000, + false, + ) + .await; + + assert!( + matches!(result, Err(AssetLockError::FundingAccountNotFound(_))), + "the absent identity top-up account must fail credit-key derivation" + ); + assert!( + reserved_outpoints(&info).is_empty(), + "both pooled accounts must have released this build's reservation" + ); + } + + /// [`credit_key_failure_releases_the_reservation_in_every_pooled_account`] + /// for the signer-driven builder, whose bookkeeping loop runs after an + /// `.await` and so had the same stranding window. + #[tokio::test] + async fn signer_credit_key_failure_releases_the_reservation_in_every_pooled_account() { + let (wallet, mut info) = test_wallet_and_info(); + insert_funded_utxo(&mut info, &wallet, 0x11, 300_000, true); + insert_funded_bip32_utxo(&mut info, &wallet, 0x22, 300_000); + info.update_last_processed_height(1100); + let root = match &wallet.wallet_type { + crate::wallet::WalletType::Mnemonic { + root_extended_private_key, + .. + } => root_extended_private_key.clone(), + _ => unreachable!("test_wallet_and_info produces a mnemonic wallet"), + }; + let signer = InMemorySigner { + root, + network: Network::Testnet, + }; + + let result = info + .build_asset_lock_with_signer( + &wallet, + &POOLED, + 0, + credit_output_with_missing_key_account(), + 1000, + false, + &signer, + ) + .await; + + assert!( + matches!(result, Err(AssetLockError::FundingAccountNotFound(_))), + "the absent identity top-up account must fail credit-key bookkeeping" + ); + assert!( + reserved_outpoints(&info).is_empty(), + "both pooled accounts must have released this build's reservation" + ); + } + + /// A signing failure is handled one layer down, by the builder itself — + /// which must also reach every funding account, not just the first. + #[tokio::test] + async fn signing_failure_releases_the_reservation_in_every_pooled_account() { + struct FailingSigner; + + #[async_trait::async_trait] + impl Signer for FailingSigner { + type Error = String; + + fn supported_methods(&self) -> &[SignerMethod] { + IN_MEMORY_METHODS + } + + async fn sign_ecdsa( + &self, + _path: &DerivationPath, + _sighash: [u8; 32], + ) -> Result<(secp256k1::ecdsa::Signature, PublicKey), Self::Error> { + Err("signing device unavailable".to_string()) + } + + async fn public_key(&self, _path: &DerivationPath) -> Result { + Err("signing device unavailable".to_string()) + } + } + + let (wallet, mut info) = test_wallet_and_info(); + insert_funded_utxo(&mut info, &wallet, 0x11, 300_000, true); + insert_funded_bip32_utxo(&mut info, &wallet, 0x22, 300_000); + info.update_last_processed_height(1100); + + let result = info + .build_asset_lock_with_signer( + &wallet, + &POOLED, + 0, + test_credit_outputs(&[500_000]), + 1000, + false, + &FailingSigner, + ) + .await; + + assert!(result.is_err(), "a failing signer must not produce a transaction"); + assert!( + reserved_outpoints(&info).is_empty(), + "both pooled accounts must have released this build's reservation" + ); + } } 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 50aa36277..3d1ec4f44 100644 --- a/key-wallet/src/wallet/managed_wallet_info/transaction_building.rs +++ b/key-wallet/src/wallet/managed_wallet_info/transaction_building.rs @@ -90,15 +90,36 @@ impl fmt::Display for AccountTypePreference { } } +/// A builder seeded with the funding of a resolved source list, plus what it +/// took to seed it: the derivation path of every candidate input address +/// (inputs can come from different accounts, so one account's resolver is not +/// enough) and the accounts whose UTXOs were offered to selection, in funding +/// order. +/// +/// The offered accounts are not the accounts that end up *contributing* inputs +/// — selection routinely takes nothing from most of them — but they are the +/// accounts holding this build's reservations, so they are what a failure path +/// must reconcile. +pub(super) struct PooledFunding { + /// The builder, with one `add_funding` call per resolved account. + pub builder: TransactionBuilder, + /// Address → derivation path for every UTXO offered to selection. + pub paths: HashMap, + /// The accounts funded, in funding order; the first supplied the change + /// address. + pub accounts: Vec, +} + 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. + /// [`AccountTypePreference::DEFAULT`]. A *single* source is an explicit + /// request for that one account and errors if it is absent; a *pooled* + /// (multi-source) list skips the sources this wallet has nothing for and + /// errors only when none of them funds anything. pub async fn build_and_sign_transaction( &mut self, wallet: &Wallet, @@ -193,7 +214,11 @@ impl ManagedWalletInfo { .set_selection_strategy(strategy) .set_current_height(height); - let (mut builder, paths) = self.fund(wallet, sources, source_index, builder)?; + let PooledFunding { + mut builder, + paths, + accounts: _, + } = self.fund(wallet, sources, source_index, builder)?; for (address, value) in outputs { builder = builder.add_output(&address, value); @@ -238,26 +263,35 @@ impl ManagedWalletInfo { /// Seed `builder` with the UTXOs of every funding account named by /// `sources`, returning it alongside the derivation path of each candidate /// input address, since the inputs can come from different accounts. - fn fund( + /// + /// A single-source list is *strict*: it names one account and a caller that + /// asked for exactly those funds must not silently be given others', so a + /// missing account is an error. A pooled list (two or more sources, or the + /// empty list standing for [`AccountTypePreference::DEFAULT`]) is *lenient*: + /// a wallet with no BIP32 account and no DashPay contacts still funds from + /// the sources it does have, and only a list that funds nothing at all is an + /// error. + pub(super) 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 + ) -> Result { + let preferences = if sources.is_empty() { + &AccountTypePreference::DEFAULT[..] } else { - &AccountTypePreference::DEFAULT + sources }; + let strict = preferences.len() == 1; let mut paths = HashMap::new(); + let mut accounts: Vec = Vec::new(); let mut funded: HashSet = HashSet::new(); for &preference in preferences { let account_types = self.account_types_for(preference, source_index); - if account_types.is_empty() && named_explicitly { + if account_types.is_empty() && strict { return Err(BuilderError::AccountNotFound(format!("account {preference}"))); } @@ -274,7 +308,7 @@ impl ManagedWalletInfo { let managed_account = self.accounts.funds_account_mut(&account_type); let (Some(account), Some(managed_account)) = (account, managed_account) else { - if named_explicitly { + if strict { return Err(BuilderError::AccountNotFound(format!( "account {account_type}" ))); @@ -289,16 +323,21 @@ impl ManagedWalletInfo { } builder = builder.add_funding(managed_account, account); funded.insert(account_type); + accounts.push(account_type); } } - if funded.is_empty() { + if accounts.is_empty() { return Err(BuilderError::AccountNotFound(format!( - "no funding account of any type at index {source_index}" + "no funding account of any named source at index {source_index}" ))); } - Ok((builder, paths)) + Ok(PooledFunding { + builder, + paths, + accounts, + }) } } #[cfg(test)] @@ -1022,6 +1061,54 @@ mod tests { ); } + /// A pooled list names sources a wallet may have nothing for — the default + /// send set names every DashPay contact, and most wallets have none. Those + /// are skipped, not fatal; a SINGLE named source stays strict, because a + /// caller asking for exactly one account's funds must not silently be given + /// another's. + #[tokio::test] + async fn a_pooled_list_skips_absent_sources_where_a_single_one_is_strict() { + let (wallet, mut info) = test_wallet_and_info(); + let bip44 = fund(&wallet, &mut info, AccountTypePreference::BIP44, 0, 0x11); + info.update_last_processed_height(1100); + + // Pooled: no contacts exist, so `AllDashpayReceivingFunds` resolves to + // nothing and the send still goes out of BIP44. + let (tx, _fee) = info + .build_and_sign_transaction( + &wallet, + &[ + AccountTypePreference::BIP44, + AccountTypePreference::BIP32, + AccountTypePreference::AllDashpayReceivingFunds, + ], + 0, + dest_outputs(200_000), + FeeRate::normal(), + SelectionStrategy::BranchAndBound, + ) + .await + .expect("a wallet with no contacts still sends from its standard accounts"); + let spent: Vec = tx.input.iter().map(|txin| txin.previous_output).collect(); + assert_eq!(spent, vec![bip44]); + + // Strict: that same absent source, named alone, is an error. + let result = info + .build_and_sign_transaction( + &wallet, + &[AccountTypePreference::AllDashpayReceivingFunds], + 0, + dest_outputs(200_000), + FeeRate::normal(), + SelectionStrategy::BranchAndBound, + ) + .await; + assert!( + matches!(result, Err(BuilderError::AccountNotFound(_))), + "a single named source must not fall back to other accounts" + ); + } + /// 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.