From 1a1263a27fa96f7dbf9b283f6fc14101149ab5fd Mon Sep 17 00:00:00 2001 From: bfoss765 Date: Sun, 9 Aug 2026 15:30:17 -0400 Subject: [PATCH] feat(key-wallet): pool BIP44 + BIP32 + DashPay receiving funds on the asset-lock path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An asset lock could only ever be funded from ONE account. A wallet holding its balance across the standard families and its DashPay contact-receiving accounts had to sweep them into BIP44 first and lock out of that — an extra on-chain hop, an extra fee, and a transparent address reused for the privilege. The send path stopped needing that in dashpay/platform#4329; this is the same change for asset locks. Both builders now take a LIST of `AccountTypePreference` sources plus a `source_index` instead of a single `AssetLockFundingAccount`, and fold them through the same `transaction_building::fund` the send path uses: coin selection draws from the union, the first source supplies the change address, overlapping sources fund each account once (#931's dedup is what makes the repeated `add_funding` safe), and derivation paths are collected across every contributing account so the inputs can be signed. Reservation bookkeeping is the part that had to change shape. A pooled build reserves in EACH contributing account's own set under the one owner token, so the post-build failure paths — credit-key derivation on the soft-wallet builder, the peek/sign/commit loop on the signer builder, both running after the transaction is already signed — now release across every funded account instead of just the one. Releasing a single account's set would have stranded the rest of the inputs until the 24-block TTL sweep. `AssetLockResult` carries the contributing accounts so the caller's rejected-broadcast release can reach them all; it is the contributor list, not everything the sources offered, so a wallet's address book does not inflate the caller's bookkeeping. `fund`'s strictness rule now matches platform's: a SINGLE named source is strict (a caller asking for exactly one account's funds must not silently be given another's), while a pooled list skips the sources this wallet has nothing for — no BIP32 account, no contacts — and errors only when none of them funds anything. Without that, the default pooled set would fail on the very wallets it is meant to serve. CoinJoin funding is unchanged and stays excluded from pooling: it remains drain-only, and it must now be the sole source, because spending mixed outputs alongside transparent ones in one transaction links them and undoes the mixing. The `AssetLockFundingAccount::CoinJoin` + `drain: true` flow that dashpay/platform#4327 ships on converts to a single-element source list and behaves exactly as before. `AssetLockError::AccountNotFound(u32)` is removed — account resolution is now the builder's, and it reports `BuilderError::AccountNotFound` with the source that failed. `AssetLockFundingAccount` remains as the drain flows' single-account vocabulary, with a `From` conversion into the source list. --- key-wallet-ffi/src/transaction.rs | 26 +- .../managed_wallet_info/asset_lock_builder.rs | 742 +++++++++++++----- .../transaction_building.rs | 117 ++- 3 files changed, 691 insertions(+), 194 deletions(-) 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.