From 32f5dc3cf730e8d713912c7a32418e9465e83927 Mon Sep 17 00:00:00 2001 From: valued mammal Date: Sun, 27 Oct 2024 09:31:02 -0400 Subject: [PATCH 1/5] example_cli: fix collecting `Assets` Previously creating a PSBT was limited to certain descriptor types that excluded e.g. `wsh`. That is fixed by using `for_each_key` which visits every pubkey in the descriptor. --- example-crates/example_cli/src/lib.rs | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/example-crates/example_cli/src/lib.rs b/example-crates/example_cli/src/lib.rs index 3a700db3a..0d2d2bff6 100644 --- a/example-crates/example_cli/src/lib.rs +++ b/example-crates/example_cli/src/lib.rs @@ -17,7 +17,7 @@ use bdk_chain::miniscript::{ descriptor::{DescriptorSecretKey, SinglePubKey}, plan::{Assets, Plan}, psbt::PsbtExt, - Descriptor, DescriptorPublicKey, + Descriptor, DescriptorPublicKey, ForEachKey, }; use bdk_chain::ConfirmationBlockTime; use bdk_chain::{ @@ -596,24 +596,20 @@ pub fn handle_commands( let chain = chain.lock().unwrap(); // collect assets we can sign for - let mut assets = Assets::new(); + let mut pks = vec![]; + graph.index.keychains().for_each(|(_, desc)| { + desc.for_each_key(|k| { + pks.push(k.clone()); + true + }); + }); + let mut assets = Assets::new().add(pks); if let Some(n) = after { assets = assets.after(absolute::LockTime::from_consensus(n)); } if let Some(n) = older { assets = assets.older(relative::LockTime::from_consensus(n)?); } - for (_, desc) in graph.index.keychains() { - match desc { - Descriptor::Wpkh(wpkh) => { - assets = assets.add(wpkh.clone().into_inner()); - } - Descriptor::Tr(tr) => { - assets = assets.add(tr.internal_key().clone()); - } - _ => bail!("unsupported descriptor type"), - } - } create_tx(&mut graph, &*chain, &assets, coin_select, address, value)? }; From c07cc1519ab5cfe3626083b0cc9e1837cc7ab330 Mon Sep 17 00:00:00 2001 From: valued mammal Date: Sat, 30 Nov 2024 15:53:06 -0500 Subject: [PATCH 2/5] ref(wallet): move getting local utxos to `create_tx` We can't easily plan the utxos before all of the assets are known, so having `TxBuilder` try to create `WeightedUtxo`s creates a chicken and egg problem that we fix by moving some logic to `Wallet::create_tx`. `TxParams::utxos` field is thus split into two: `utxos` is a list of outpoints strictly to be used for getting locally owned outputs, and `foreign_utxos` handles adding `WeightedUtxos` coming from `add_foreign_utxo`. These are later combined with the manually selected coins to form the list of utxos that must be spent. --- crates/wallet/src/wallet/mod.rs | 129 +++++++++++++------------ crates/wallet/src/wallet/tx_builder.rs | 32 ++---- 2 files changed, 79 insertions(+), 82 deletions(-) diff --git a/crates/wallet/src/wallet/mod.rs b/crates/wallet/src/wallet/mod.rs index d64f4b8e2..c624e3948 100644 --- a/crates/wallet/src/wallet/mod.rs +++ b/crates/wallet/src/wallet/mod.rs @@ -890,6 +890,22 @@ impl Wallet { .next() } + /// Get a local output. A local output is an output owned by this wallet and + /// indexed internally. This can be used for retrieving an output that may be + /// spent but is eligible to be used in a replacement tx. + fn get_output(&self, outpoint: OutPoint) -> Option { + let ((keychain, index), _) = self.indexed_graph.index.txout(outpoint)?; + self.indexed_graph + .graph() + .filter_chain_txouts( + &self.chain, + self.chain.tip().block_id(), + core::iter::once(((), outpoint)), + ) + .map(|(_, full_txo)| new_local_utxo(keychain, index, full_txo)) + .next() + } + /// Inserts a [`TxOut`] at [`OutPoint`] into the wallet's transaction graph. /// /// This is used for providing a previous output's value so that we can use [`calculate_fee`] @@ -1425,8 +1441,28 @@ impl Wallet { fee_amount += fee_rate * tx.weight(); + let must_spend: Vec<_> = params + .utxos + .iter() + .flat_map(|outpoint| { + self.get_utxo(*outpoint) + .or_else(|| self.get_output(*outpoint)) + .map(|output| { + let desc = self.public_descriptor(output.keychain); + let satisfaction_weight = desc + .max_weight_to_satisfy() + .expect("descriptor should be satisfiable"); + WeightedUtxo { + utxo: Utxo::Local(output), + satisfaction_weight, + } + }) + }) + .chain(params.foreign_utxos.clone()) + .collect(); + let (required_utxos, optional_utxos) = - self.preselect_utxos(¶ms, Some(current_height.to_consensus_u32())); + self.preselect_utxos(must_spend, ¶ms, Some(current_height.to_consensus_u32())); // get drain script let mut drain_index = Option::<(KeychainKind, u32)>::None; @@ -1636,65 +1672,39 @@ impl Wallet { .map_err(|_| BuildFeeBumpError::FeeRateUnavailable)?; // remove the inputs from the tx and process them - let original_txin = tx.input.drain(..).collect::>(); - let original_utxos = original_txin - .iter() - .map(|txin| -> Result<_, BuildFeeBumpError> { - let prev_tx = graph - .get_tx(txin.previous_output.txid) - .ok_or(BuildFeeBumpError::UnknownUtxo(txin.previous_output))?; - let txout = &prev_tx.output[txin.previous_output.vout as usize]; - - let chain_position = chain_positions - .get(&txin.previous_output.txid) - .cloned() - .ok_or(BuildFeeBumpError::UnknownUtxo(txin.previous_output))?; - - let weighted_utxo = match txout_index.index_of_spk(txout.script_pubkey.clone()) { - Some(&(keychain, derivation_index)) => { - let satisfaction_weight = self - .public_descriptor(keychain) - .max_weight_to_satisfy() - .unwrap(); - WeightedUtxo { - utxo: Utxo::Local(LocalOutput { - outpoint: txin.previous_output, - txout: txout.clone(), - keychain, - is_spent: true, - derivation_index, - chain_position, - }), - satisfaction_weight, - } - } - None => { - let satisfaction_weight = Weight::from_wu_usize( - serialize(&txin.script_sig).len() * 4 + serialize(&txin.witness).len(), - ); - WeightedUtxo { - utxo: Utxo::Foreign { - outpoint: txin.previous_output, - sequence: txin.sequence, - psbt_input: Box::new(psbt::Input { - witness_utxo: Some(txout.clone()), - non_witness_utxo: Some(prev_tx.as_ref().clone()), - ..Default::default() - }), - }, - satisfaction_weight, - } - } - }; - - Ok(weighted_utxo) - }) - .collect::, _>>()?; + let mut utxos = vec![]; + let mut foreign_utxos = vec![]; + + for txin in tx.input.drain(..) { + let prev_tx = graph + .get_tx(txin.previous_output.txid) + .ok_or(BuildFeeBumpError::UnknownUtxo(txin.previous_output))?; + let txout = &prev_tx.output[txin.previous_output.vout as usize]; + if self.is_mine(txout.script_pubkey.clone()) { + utxos.push(txin.previous_output); + continue; + } + let satisfaction_weight = Weight::from_wu_usize( + serialize(&txin.script_sig).len() * 4 + serialize(&txin.witness).len(), + ); + foreign_utxos.push(WeightedUtxo { + utxo: Utxo::Foreign { + outpoint: txin.previous_output, + sequence: txin.sequence, + psbt_input: Box::new(psbt::Input { + witness_utxo: Some(txout.clone()), + non_witness_utxo: Some(prev_tx.as_ref().clone()), + ..Default::default() + }), + }, + satisfaction_weight, + }); + } if tx.output.len() > 1 { let mut change_index = None; for (index, txout) in tx.output.iter().enumerate() { - let change_keychain = KeychainKind::Internal; + let change_keychain = self.map_keychain(KeychainKind::Internal); match txout_index.index_of_spk(txout.script_pubkey.clone()) { Some((keychain, _)) if *keychain == change_keychain => { change_index = Some(index) @@ -1702,21 +1712,20 @@ impl Wallet { _ => {} } } - if let Some(change_index) = change_index { tx.output.remove(change_index); } } let params = TxParams { - // TODO: figure out what rbf option should be? version: Some(tx_builder::Version(tx.version.0)), recipients: tx .output .into_iter() .map(|txout| (txout.script_pubkey, txout.value)) .collect(), - utxos: original_utxos, + utxos, + foreign_utxos, bumping_fee: Some(tx_builder::PreviousFee { absolute: fee, rate: fee_rate, @@ -2008,13 +2017,13 @@ impl Wallet { /// transaction and any further that may be used if needed. fn preselect_utxos( &self, + utxos: Vec, params: &TxParams, current_height: Option, ) -> (Vec, Vec) { let TxParams { change_policy, unspendable, - utxos, drain_wallet, manually_selected_only, bumping_fee, diff --git a/crates/wallet/src/wallet/tx_builder.rs b/crates/wallet/src/wallet/tx_builder.rs index 868d51dfa..396b694f0 100644 --- a/crates/wallet/src/wallet/tx_builder.rs +++ b/crates/wallet/src/wallet/tx_builder.rs @@ -125,7 +125,8 @@ pub(crate) struct TxParams { pub(crate) fee_policy: Option, pub(crate) internal_policy_path: Option>>, pub(crate) external_policy_path: Option>>, - pub(crate) utxos: Vec, + pub(crate) utxos: Vec, + pub(crate) foreign_utxos: Vec, pub(crate) unspendable: HashSet, pub(crate) manually_selected_only: bool, pub(crate) sighash: Option, @@ -274,27 +275,14 @@ impl<'a, Cs> TxBuilder<'a, Cs> { /// These have priority over the "unspendable" utxos, meaning that if a utxo is present both in /// the "utxos" and the "unspendable" list, it will be spent. pub fn add_utxos(&mut self, outpoints: &[OutPoint]) -> Result<&mut Self, AddUtxoError> { - { - let wallet = &mut self.wallet; - let utxos = outpoints - .iter() - .map(|outpoint| { - wallet - .get_utxo(*outpoint) - .ok_or(AddUtxoError::UnknownUtxo(*outpoint)) - }) - .collect::, _>>()?; - - for utxo in utxos { - let descriptor = wallet.public_descriptor(utxo.keychain); - let satisfaction_weight = descriptor.max_weight_to_satisfy().unwrap(); - self.params.utxos.push(WeightedUtxo { - satisfaction_weight, - utxo: Utxo::Local(utxo), - }); - } + // first check that we can get the utxo from the wallet + for outpoint in outpoints { + let _utxo = self + .wallet + .get_utxo(*outpoint) + .ok_or(AddUtxoError::UnknownUtxo(*outpoint))?; } - + self.params.utxos.extend(outpoints); Ok(self) } @@ -393,7 +381,7 @@ impl<'a, Cs> TxBuilder<'a, Cs> { } } - self.params.utxos.push(WeightedUtxo { + self.params.foreign_utxos.push(WeightedUtxo { satisfaction_weight, utxo: Utxo::Foreign { outpoint, From 65810e44b0b3888dbd9efa2572015a2b6cf21015 Mon Sep 17 00:00:00 2001 From: valued mammal Date: Sun, 1 Dec 2024 19:31:34 -0500 Subject: [PATCH 3/5] feat(wallet)!: use miniscript `plan` in place of old policy mod The internal behavior is preserved insofar as the policy path can influence the outcome of tx building. The difference is instead of encode the path as nodes in a policy tree, the plan is determined by the `Assets` available. Note that creating a `Plan` is a pre-requisite to coin selection, so if a UTXO in the wallet can't be planned, it won't be selected. The wallet attempts to create a plan for each UTXO based on the available information. The user may provide additional assets or specify a particular set of assets in order to complete a plan. New APIs include `TxBuilder::add_assets`, `set_assets` and `Wallet::try_plan`. To prevent confusion due to overlapping functionality, the `TxBuilder` methods `policy_path` and `current_height` are removed. --- crates/wallet/src/descriptor/policy.rs | 15 +- crates/wallet/src/test_utils.rs | 5 + crates/wallet/src/types.rs | 97 ++++++- crates/wallet/src/wallet/error.rs | 45 ++-- crates/wallet/src/wallet/mod.rs | 359 +++++++++++++------------ crates/wallet/src/wallet/tx_builder.rs | 193 +++++++------ crates/wallet/src/wallet/utils.rs | 71 ++++- crates/wallet/tests/create_tx_plan.rs | 203 ++++++++++++++ crates/wallet/tests/wallet.rs | 204 ++++++-------- 9 files changed, 749 insertions(+), 443 deletions(-) create mode 100644 crates/wallet/tests/create_tx_plan.rs diff --git a/crates/wallet/src/descriptor/policy.rs b/crates/wallet/src/descriptor/policy.rs index 6fb4013f6..df2fd25c8 100644 --- a/crates/wallet/src/descriptor/policy.rs +++ b/crates/wallet/src/descriptor/policy.rs @@ -63,6 +63,7 @@ use crate::descriptor::ExtractPolicy; use crate::keys::ExtScriptContext; use crate::wallet::signer::{SignerId, SignersContainer}; use crate::wallet::utils::{After, Older, SecpCtx}; +use crate::Condition; use super::checksum::calc_checksum; use super::error::Error; @@ -444,18 +445,6 @@ pub struct Policy { pub contribution: Satisfaction, } -/// An extra condition that must be satisfied but that is out of control of the user -/// TODO: use `bitcoin::LockTime` and `bitcoin::Sequence` -#[derive(Hash, Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Default, Serialize)] -pub struct Condition { - /// Optional CheckSequenceVerify condition - #[serde(skip_serializing_if = "Option::is_none")] - pub csv: Option, - /// Optional timelock condition - #[serde(skip_serializing_if = "Option::is_none")] - pub timelock: Option, -} - impl Condition { fn merge_nlocktime( a: absolute::LockTime, @@ -478,7 +467,7 @@ impl Condition { } } - pub(crate) fn merge(mut self, other: &Condition) -> Result { + fn merge(mut self, other: &Condition) -> Result { match (self.csv, other.csv) { (Some(a), Some(b)) => self.csv = Some(Self::merge_nsequence(a, b)?), (None, any) => self.csv = any, diff --git a/crates/wallet/src/test_utils.rs b/crates/wallet/src/test_utils.rs index 7ad93e0c3..01399560f 100644 --- a/crates/wallet/src/test_utils.rs +++ b/crates/wallet/src/test_utils.rs @@ -160,6 +160,11 @@ pub fn get_test_single_sig_cltv() -> &'static str { "wsh(and_v(v:pk(cVpPVruEDdmutPzisEsYvtST1usBR3ntr8pXSyt6D2YYqXRyPcFW),after(100000)))" } +/// `wsh` descriptor with policy `and(pk(A),after(500000001))` +pub fn get_test_single_sig_cltv_timestamp() -> &'static str { + "wsh(and_v(v:pk(cVpPVruEDdmutPzisEsYvtST1usBR3ntr8pXSyt6D2YYqXRyPcFW),after(500000001)))" +} + /// taproot single key descriptor pub fn get_test_tr_single_sig() -> &'static str { "tr(cNJmN3fH9DDbDt131fQNkVakkpzawJBSeybCUNmP1BovpmGQ45xG)" diff --git a/crates/wallet/src/types.rs b/crates/wallet/src/types.rs index 54ddfa261..d8a325daa 100644 --- a/crates/wallet/src/types.rs +++ b/crates/wallet/src/types.rs @@ -10,13 +10,16 @@ // licenses. use alloc::boxed::Box; -use chain::{ChainPosition, ConfirmationBlockTime}; use core::convert::AsRef; +use serde::{Deserialize, Serialize}; use bitcoin::transaction::{OutPoint, Sequence, TxOut}; -use bitcoin::{psbt, Weight}; +use bitcoin::{absolute, psbt, relative, Weight}; +use chain::{ChainPosition, ConfirmationBlockTime}; +use miniscript::plan::Plan; -use serde::{Deserialize, Serialize}; +use crate::error::PlanError; +use crate::utils::merge_nsequence; /// Types of keychains #[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, Hash, Ord, PartialOrd)] @@ -77,6 +80,31 @@ pub struct WeightedUtxo { pub utxo: Utxo, } +/// A [`Utxo`] and accompanying [`Plan`] +#[derive(Debug, Clone)] +pub struct PlannedUtxo { + /// plan + pub plan: Plan, + /// utxo + pub utxo: Utxo, +} + +impl PlannedUtxo { + /// Create new [`PlannedUtxo`] + pub fn new(plan: Plan, utxo: Utxo) -> Self { + Self { plan, utxo } + } + + /// Get a weighted utxo + pub fn weighted_utxo(&self) -> WeightedUtxo { + let wu = self.plan.satisfaction_weight(); + WeightedUtxo { + satisfaction_weight: Weight::from_wu_usize(wu), + utxo: self.utxo.clone(), + } + } +} + #[derive(Debug, Clone, PartialEq, Eq)] /// An unspent transaction output (UTXO). pub enum Utxo { @@ -133,3 +161,66 @@ impl Utxo { } } } + +/// Represents a condition that must be satisfied +#[derive(Hash, Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Default, serde::Serialize)] +pub struct Condition { + /// sequence value used as the argument to `OP_CHECKSEQUENCEVERIFY` + #[serde(skip_serializing_if = "Option::is_none")] + pub csv: Option, + /// absolute timelock value used as the argument to `OP_CHECKLOCKTIMEVERIFY` + #[serde(skip_serializing_if = "Option::is_none")] + pub timelock: Option, +} + +impl Condition { + /// Merges two absolute locktimes. Errors if `a` and `b` do not have the same unit. + pub(crate) fn merge_abs_locktime( + a: Option, + b: Option, + ) -> Result, PlanError> { + match (a, b) { + (None, b) => Ok(b), + (a, None) => Ok(a), + (Some(a), Some(b)) => { + if a.is_block_height() != b.is_block_height() { + Err(PlanError::MixedTimelockUnits) + } else if b > a { + Ok(Some(b)) + } else { + Ok(Some(a)) + } + } + } + } + + /// Merges two relative locktimes. Errors if `a` and `b` do not have the same unit. + pub(crate) fn merge_rel_locktime( + a: Option, + b: Option, + ) -> Result, PlanError> { + match (a, b) { + (a, None) => Ok(a), + (None, b) => Ok(b), + (Some(a), Some(b)) => { + let seq = merge_nsequence(a.to_sequence(), b.to_sequence())?; + Ok(seq.to_relative_lock_time()) + } + } + } + + /// Merges the conditions of `other` with `self` keeping the greater of the two + /// for each individual condition. Locktime types must not be mixed or else a + /// [`PlanError`] is returned. + pub(crate) fn merge_condition(mut self, other: Condition) -> Result { + self.timelock = Self::merge_abs_locktime(self.timelock, other.timelock)?; + + match (self.csv, other.csv) { + (Some(a), Some(b)) => self.csv = Some(merge_nsequence(a, b)?), + (None, b) => self.csv = b, + _ => {} + } + + Ok(self) + } +} diff --git a/crates/wallet/src/wallet/error.rs b/crates/wallet/src/wallet/error.rs index adce5b188..07dac5bdd 100644 --- a/crates/wallet/src/wallet/error.rs +++ b/crates/wallet/src/wallet/error.rs @@ -11,13 +11,12 @@ //! Errors that can be thrown by the [`Wallet`](crate::wallet::Wallet) -use crate::descriptor::policy::PolicyError; -use crate::descriptor::DescriptorError; +use crate::descriptor::{self, DescriptorError}; use crate::wallet::coin_selection; -use crate::{descriptor, KeychainKind}; use alloc::string::String; use bitcoin::{absolute, psbt, Amount, OutPoint, Sequence, Txid}; use core::fmt; +use miniscript::{DefiniteDescriptorKey, Descriptor}; /// Errors returned by miniscript when updating inconsistent PSBTs #[derive(Debug, Clone)] @@ -43,6 +42,27 @@ impl fmt::Display for MiniscriptPsbtError { #[cfg(feature = "std")] impl std::error::Error for MiniscriptPsbtError {} +/// Error when preparing the conditions of a spending plan +#[derive(Debug, Clone)] +pub enum PlanError { + /// Attempted to mix height- and time-based locktimes + MixedTimelockUnits, + /// Error creating a spend [`Plan`](miniscript::plan::Plan) + Plan(Descriptor), +} + +impl fmt::Display for PlanError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::MixedTimelockUnits => write!(f, "cannot mix locktime units"), + Self::Plan(d) => write!(f, "failed to create plan for descriptor {}", d), + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for PlanError {} + #[derive(Debug)] /// Error returned from [`TxBuilder::finish`] /// @@ -50,10 +70,6 @@ impl std::error::Error for MiniscriptPsbtError {} pub enum CreateTxError { /// There was a problem with the descriptors passed in Descriptor(DescriptorError), - /// There was a problem while extracting and manipulating policies - Policy(PolicyError), - /// Spending policy is not compatible with this [`KeychainKind`] - SpendingPolicyRequired(KeychainKind), /// Requested invalid transaction version '0' Version0, /// Requested transaction version `1`, but at least `2` is needed to use OP_CSV @@ -104,16 +120,14 @@ pub enum CreateTxError { MissingNonWitnessUtxo(OutPoint), /// Miniscript PSBT error MiniscriptPsbt(MiniscriptPsbtError), + /// Error creating a spending plan + Plan(PlanError), } impl fmt::Display for CreateTxError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Self::Descriptor(e) => e.fmt(f), - Self::Policy(e) => e.fmt(f), - CreateTxError::SpendingPolicyRequired(keychain_kind) => { - write!(f, "Spending policy required: {:?}", keychain_kind) - } CreateTxError::Version0 => { write!(f, "Invalid version `0`") } @@ -171,6 +185,9 @@ impl fmt::Display for CreateTxError { CreateTxError::MiniscriptPsbt(err) => { write!(f, "Miniscript PSBT error: {}", err) } + CreateTxError::Plan(e) => { + write!(f, "{}", e) + } } } } @@ -181,12 +198,6 @@ impl From for CreateTxError { } } -impl From for CreateTxError { - fn from(err: PolicyError) -> Self { - CreateTxError::Policy(err) - } -} - impl From for CreateTxError { fn from(err: MiniscriptPsbtError) -> Self { CreateTxError::MiniscriptPsbt(err) diff --git a/crates/wallet/src/wallet/mod.rs b/crates/wallet/src/wallet/mod.rs index c624e3948..0cb324e53 100644 --- a/crates/wallet/src/wallet/mod.rs +++ b/crates/wallet/src/wallet/mod.rs @@ -38,7 +38,7 @@ use bitcoin::{ absolute, consensus::encode::serialize, constants::{genesis_block, COINBASE_MATURITY}, - psbt, + psbt, relative, secp256k1::Secp256k1, sighash::{EcdsaSighashType, TapSighashType}, transaction, Address, Amount, Block, BlockHash, FeeRate, Network, OutPoint, Psbt, ScriptBuf, @@ -46,7 +46,9 @@ use bitcoin::{ }; use miniscript::{ descriptor::KeyMap, + plan::Assets, psbt::{PsbtExt, PsbtInputExt, PsbtInputSatisfier}, + ForEachKey, }; use rand_core::RngCore; @@ -62,18 +64,17 @@ pub(crate) mod utils; use crate::collections::{BTreeMap, HashMap, HashSet}; use crate::descriptor::{ - check_wallet_descriptor, error::Error as DescriptorError, policy::BuildSatisfaction, - DerivedDescriptor, DescriptorMeta, ExtendedDescriptor, ExtractPolicy, IntoWalletDescriptor, - Policy, XKeyUtils, + check_wallet_descriptor, error::Error as DescriptorError, DerivedDescriptor, DescriptorMeta, + ExtendedDescriptor, IntoWalletDescriptor, Policy, XKeyUtils, }; use crate::psbt::PsbtUtils; use crate::types::*; use crate::wallet::{ coin_selection::{DefaultCoinSelectionAlgorithm, Excess, InsufficientFunds}, - error::{BuildFeeBumpError, CreateTxError, MiniscriptPsbtError}, + error::{BuildFeeBumpError, CreateTxError, MiniscriptPsbtError, PlanError}, signer::{SignOptions, SignerError, SignerOrdering, SignersContainer, TransactionSigner}, tx_builder::{FeePolicy, TxBuilder, TxParams}, - utils::{check_nsequence_rbf, After, Older, SecpCtx}, + utils::{check_nsequence_rbf, AssetsExt, SecpCtx}, }; // re-exports @@ -1240,65 +1241,58 @@ impl Wallet { params: TxParams, rng: &mut impl RngCore, ) -> Result { - let keychains: BTreeMap<_, _> = self.indexed_graph.index.keychains().collect(); - let external_descriptor = keychains.get(&KeychainKind::External).expect("must exist"); - let internal_descriptor = keychains.get(&KeychainKind::Internal); + // get spend assets + let assets = match params.assets { + None => self.assets(), + Some(ref params) => { + let mut assets = Assets::new(); + assets + .extend(params) + .expect("extending a new Assets should not fail"); + assets + } + }; - let external_policy = external_descriptor - .extract_policy(&self.signers, BuildSatisfaction::None, &self.secp)? - .unwrap(); - let internal_policy = internal_descriptor - .map(|desc| { - Ok::<_, CreateTxError>( - desc.extract_policy(&self.change_signers, BuildSatisfaction::None, &self.secp)? - .unwrap(), - ) + // get planned utxos + let manually_selected: HashSet = params.utxos.clone().into_iter().collect(); + let picked = manually_selected + .iter() + .flat_map(|outpoint| { + self.get_utxo(*outpoint) + .or_else(|| self.get_output(*outpoint)) + .map(|output| self.try_plan(output, &assets)) }) - .transpose()?; - - // The policy allows spending external outputs, but it requires a policy path that hasn't been - // provided - if params.change_policy != tx_builder::ChangeSpendPolicy::OnlyChange - && external_policy.requires_path() - && params.external_policy_path.is_none() - { - return Err(CreateTxError::SpendingPolicyRequired( - KeychainKind::External, - )); - }; - // Same for the internal_policy path - if let Some(internal_policy) = &internal_policy { - if params.change_policy != tx_builder::ChangeSpendPolicy::ChangeForbidden - && internal_policy.requires_path() - && params.internal_policy_path.is_none() - { - return Err(CreateTxError::SpendingPolicyRequired( - KeychainKind::Internal, - )); - }; + .collect::>>(); + + let avail = self + .list_unspent() + .filter(|utxo| !manually_selected.contains(&utxo.outpoint)) + .map(|utxo| self.try_plan(utxo, &assets)) + .collect::>>(); + + // We may have coins to spend but were unable to create any plans. This + // can happen if the known or provided assets are insufficient. + if !picked.iter().chain(&avail).any(|res| res.is_ok()) { + while let Some(res) = picked.iter().chain(&avail).next().cloned() { + let _ = res.map_err(CreateTxError::Plan)?; + } } + let must_spend: Vec = picked.into_iter().flat_map(|res| res.ok()).collect(); + let may_spend: Vec = avail.into_iter().flat_map(|res| res.ok()).collect(); - let external_requirements = external_policy.get_condition( - params - .external_policy_path - .as_ref() - .unwrap_or(&BTreeMap::new()), - )?; - let internal_requirements = internal_policy - .map(|policy| { - Ok::<_, CreateTxError>( - policy.get_condition( - params - .internal_policy_path - .as_ref() - .unwrap_or(&BTreeMap::new()), - )?, - ) + // accumulate the max required locktimes + let requirements = must_spend + .iter() + .chain(&may_spend) + .try_fold(Condition::default(), |req, plan_utxo| { + let PlannedUtxo { plan, .. } = plan_utxo; + let cond = Condition { + timelock: plan.absolute_timelock, + csv: plan.relative_timelock.map(|lt| lt.to_sequence()), + }; + req.merge_condition(cond) }) - .transpose()?; - - let requirements = - external_requirements.merge(&internal_requirements.unwrap_or_default())?; + .map_err(CreateTxError::Plan)?; let version = match params.version { Some(tx_builder::Version(0)) => return Err(CreateTxError::Version0), @@ -1310,15 +1304,12 @@ impl Wallet { None => 1, }; - // We use a match here instead of a unwrap_or_else as it's way more readable :) - let current_height = match params.current_height { - // If they didn't tell us the current height, we assume it's the latest sync height. - None => { - let tip_height = self.chain.tip().height(); - absolute::LockTime::from_height(tip_height).expect("invalid height") - } - Some(h) => h, - }; + let current_height: absolute::LockTime = + assets + .absolute_timelock + .unwrap_or(absolute::LockTime::from_consensus( + self.latest_checkpoint().height(), + )); let lock_time = match params.locktime { // When no nLockTime is specified, we try to prevent fee sniping, if possible @@ -1441,28 +1432,39 @@ impl Wallet { fee_amount += fee_rate * tx.weight(); - let must_spend: Vec<_> = params - .utxos - .iter() - .flat_map(|outpoint| { - self.get_utxo(*outpoint) - .or_else(|| self.get_output(*outpoint)) - .map(|output| { - let desc = self.public_descriptor(output.keychain); - let satisfaction_weight = desc - .max_weight_to_satisfy() - .expect("descriptor should be satisfiable"); - WeightedUtxo { - utxo: Utxo::Local(output), - satisfaction_weight, - } - }) + // prepare candidates for selection + let must_spend: Vec = must_spend + .into_iter() + .map(|p| p.weighted_utxo()) + .chain( + params + .foreign_utxos + .clone() + .into_iter() + .filter(|u| !manually_selected.contains(&u.utxo.outpoint())), + ) + .collect(); + + let may_spend: Vec<(LocalOutput, Weight)> = may_spend + .into_iter() + .flat_map(|p| { + let weighted_utxo = p.weighted_utxo(); + match weighted_utxo.utxo { + Utxo::Local(output) => Some((output, weighted_utxo.satisfaction_weight)), + _ => None, + } }) - .chain(params.foreign_utxos.clone()) .collect(); + let (required_utxos, optional_utxos) = self.preselect_utxos( + must_spend, + may_spend, + ¶ms, + current_height.to_consensus_u32(), + ); + let (required_utxos, optional_utxos) = - self.preselect_utxos(must_spend, ¶ms, Some(current_height.to_consensus_u32())); + coin_selection::filter_duplicates(required_utxos, optional_utxos); // get drain script let mut drain_index = Option::<(KeychainKind, u32)>::None; @@ -1491,9 +1493,6 @@ impl Wallet { } }; - let (required_utxos, optional_utxos) = - coin_selection::filter_duplicates(required_utxos, optional_utxos); - let coin_selection = coin_selection .coin_select( required_utxos, @@ -1575,13 +1574,12 @@ impl Wallet { // recording changes to the change keychain if let (Excess::Change { .. }, Some((keychain, index))) = (excess, drain_index) { - let (_, index_changeset) = self - .indexed_graph - .index - .reveal_to_target(keychain, index) - .expect("must not be None"); - self.stage.merge(index_changeset.into()); - self.mark_used(keychain, index); + if let Some((_, index_changeset)) = + self.indexed_graph.index.reveal_to_target(keychain, index) + { + self.stage.merge(index_changeset.into()); + self.mark_used(keychain, index); + } } Ok(psbt) @@ -1818,6 +1816,8 @@ impl Wallet { /// Return the spending policies for the wallet's descriptor pub fn policies(&self, keychain: KeychainKind) -> Result, DescriptorError> { + use crate::descriptor::{policy::BuildSatisfaction, ExtractPolicy}; + let signers = match keychain { KeychainKind::External => &self.signers, KeychainKind::Internal => &self.change_signers, @@ -1854,36 +1854,12 @@ impl Wallet { pub fn finalize_psbt( &self, psbt: &mut Psbt, - sign_options: SignOptions, + _sign_options: SignOptions, ) -> Result { let tx = &psbt.unsigned_tx; - let chain_tip = self.chain.tip().block_id(); - let prev_txids = tx - .input - .iter() - .map(|txin| txin.previous_output.txid) - .collect::>(); - let confirmation_heights = self - .indexed_graph - .graph() - .list_canonical_txs(&self.chain, chain_tip) - .filter(|canon_tx| prev_txids.contains(&canon_tx.tx_node.txid)) - // This is for a small performance gain. Although `.filter` filters out excess txs, it - // will still consume the internal `CanonicalIter` entirely. Having a `.take` here - // allows us to stop further unnecessary canonicalization. - .take(prev_txids.len()) - .map(|canon_tx| { - let txid = canon_tx.tx_node.txid; - match canon_tx.chain_position { - ChainPosition::Confirmed { anchor, .. } => (txid, anchor.block_id.height), - ChainPosition::Unconfirmed { .. } => (txid, u32::MAX), - } - }) - .collect::>(); - let mut finished = true; - for (n, input) in tx.input.iter().enumerate() { + for (n, _input) in tx.input.iter().enumerate() { let psbt_input = &psbt .inputs .get(n) @@ -1891,12 +1867,6 @@ impl Wallet { if psbt_input.final_script_sig.is_some() || psbt_input.final_script_witness.is_some() { continue; } - let confirmation_height = confirmation_heights - .get(&input.previous_output.txid) - .copied(); - let current_height = sign_options - .assume_height - .unwrap_or_else(|| self.chain.tip().height()); // - Try to derive the descriptor by looking at the txout. If it's in our database, we // know exactly which `keychain` to use, and which derivation index it is @@ -1916,14 +1886,7 @@ impl Wallet { match desc { Some(desc) => { let mut tmp_input = bitcoin::TxIn::default(); - match desc.satisfy( - &mut tmp_input, - ( - PsbtInputSatisfier::new(psbt, n), - After::new(Some(current_height), false), - Older::new(Some(current_height), confirmation_height, false), - ), - ) { + match desc.satisfy(&mut tmp_input, PsbtInputSatisfier::new(psbt, n)) { Ok(_) => { // Set the UTXO fields, final script_sig and witness // and clear everything else. @@ -2000,26 +1963,63 @@ impl Wallet { descriptor.at_derivation_index(child).ok() } - fn get_available_utxos(&self) -> Vec<(LocalOutput, Weight)> { - self.list_unspent() - .map(|utxo| { - let keychain = utxo.keychain; - (utxo, { - self.public_descriptor(keychain) - .max_weight_to_satisfy() - .unwrap() - }) - }) - .collect() + /// Try to create a [`Plan`] for the given `output`. + /// + /// We want to afford the UTXO with as many assets as we can, so when looking for + /// absolute and relative timelocks, if we can't get the value from the given + /// `spend_assets` we set it based on the current local height and the age of the + /// coin respectively. + /// + /// # Errors + /// + /// If the spend assets are insufficient to create a plan then a [`PlanError`] is + /// returned. + /// + /// [`Plan`]: miniscript::plan::Plan + pub fn try_plan( + &self, + output: LocalOutput, + spend_assets: &Assets, + ) -> Result { + let cur_height = self.latest_checkpoint().height(); + let abs_locktime = spend_assets + .absolute_timelock + .unwrap_or(absolute::LockTime::from_consensus(cur_height)); + let desc = self.public_descriptor(output.keychain); + let definite_desc = desc + .at_derivation_index(output.derivation_index) + .expect("valid derivation index"); + + let rel_locktime = spend_assets.relative_timelock.unwrap_or_else(|| { + let age = match output.chain_position { + ChainPosition::Confirmed { anchor, .. } => (cur_height - anchor.block_id.height) + .try_into() + .unwrap_or(u16::MAX), + ChainPosition::Unconfirmed { .. } => 0, + }; + relative::LockTime::from_height(age) + }); + let mut assets = Assets::new(); + assets.extend(spend_assets)?; + assets = assets.after(abs_locktime); + assets = assets.older(rel_locktime); + + let plan = definite_desc.plan(&assets).map_err(PlanError::Plan)?; + + Ok(PlannedUtxo { + plan, + utxo: Utxo::Local(output), + }) } /// Given the options returns the list of utxos that must be used to form the /// transaction and any further that may be used if needed. fn preselect_utxos( &self, - utxos: Vec, + mut must_spend: Vec, + mut may_spend: Vec<(LocalOutput, Weight)>, params: &TxParams, - current_height: Option, + current_height: u32, ) -> (Vec, Vec) { let TxParams { change_policy, @@ -2030,22 +2030,10 @@ impl Wallet { .. } = params; - let manually_selected = utxos.clone(); // we mandate confirmed transactions if we're bumping the fee let must_only_use_confirmed_tx = bumping_fee.is_some(); let must_use_all_available = *drain_wallet; - // must_spend <- manually selected utxos - // may_spend <- all other available utxos - let mut may_spend = self.get_available_utxos(); - - may_spend.retain(|may_spend| { - !manually_selected - .iter() - .any(|manually_selected| manually_selected.utxo.outpoint() == may_spend.0.outpoint) - }); - let mut must_spend = manually_selected; - // NOTE: we are intentionally ignoring `unspendable` here. i.e manual // selection overrides unspendable. if *manually_selected_only { @@ -2054,8 +2042,8 @@ impl Wallet { let satisfies_confirmed = may_spend .iter() - .map(|u| -> bool { - let txid = u.0.outpoint.txid; + .map(|(utxo, _)| -> bool { + let txid = utxo.outpoint.txid; let tx = match self.indexed_graph.graph().get_tx(txid) { Some(tx) => tx, None => return false, @@ -2063,7 +2051,7 @@ impl Wallet { // Whether the UTXO is mature and, if needed, confirmed let mut spendable = true; - let chain_position = u.0.chain_position; + let chain_position = utxo.chain_position; if must_only_use_confirmed_tx && !chain_position.is_confirmed() { return false; } @@ -2072,26 +2060,25 @@ impl Wallet { chain_position.is_confirmed(), "coinbase must always be confirmed" ); - if let Some(current_height) = current_height { - match chain_position { - ChainPosition::Confirmed { anchor, .. } => { - // https://github.com/bitcoin/bitcoin/blob/c5e67be03bb06a5d7885c55db1f016fbf2333fe3/src/validation.cpp#L373-L375 - spendable &= (current_height - .saturating_sub(anchor.block_id.height)) - >= COINBASE_MATURITY; - } - ChainPosition::Unconfirmed { .. } => spendable = false, + match chain_position { + ChainPosition::Confirmed { anchor, .. } => { + // https://github.com/bitcoin/bitcoin/blob/c5e67be03bb06a5d7885c55db1f016fbf2333fe3/src/validation.cpp#L373-L375 + spendable &= (current_height.saturating_sub(anchor.block_id.height)) + >= COINBASE_MATURITY; } + ChainPosition::Unconfirmed { .. } => spendable = false, } } spendable }) .collect::>(); + // keep available coins that meet the change policy, were not specifically + // marked unspendable, and for which `satisfies_confirmed` is true. let mut i = 0; - may_spend.retain(|u| { - let retain = (self.keychains().count() == 1 || change_policy.is_satisfied_by(&u.0)) - && !unspendable.contains(&u.0.outpoint) + may_spend.retain(|(utxo, _)| { + let retain = (self.keychains().count() == 1 || change_policy.is_satisfied_by(utxo)) + && !unspendable.contains(&utxo.outpoint) && satisfies_confirmed[i]; i += 1; retain @@ -2462,6 +2449,22 @@ impl Wallet { keychain } } + + /// Returns the key [`Assets`] by collecting as many pubkeys as appear in the wallet's + /// descriptors. + /// + /// This is useful in cases where we have an unambiguous spending path (e.g. single-sig) + /// and therefore don't require the user to provide it. + fn assets(&self) -> Assets { + let mut pks = vec![]; + for (_, desc) in self.keychains() { + desc.for_each_key(|k| { + pks.push(k.clone()); + true + }); + } + Assets::new().add(pks) + } } /// Methods to construct sync/full-scan requests for spk-based chain sources. @@ -2594,7 +2597,7 @@ macro_rules! floating_rate { /// Macro for getting a wallet for use in a doctest macro_rules! doctest_wallet { () => {{ - use $crate::bitcoin::{BlockHash, Transaction, absolute, TxOut, Network, hashes::Hash}; + use $crate::bitcoin::{transaction, Amount, BlockHash, Transaction, absolute, TxOut, Network, hashes::Hash}; use $crate::chain::{ConfirmationBlockTime, BlockId, TxGraph, tx_graph}; use $crate::{Update, KeychainKind, Wallet}; use $crate::test_utils::*; diff --git a/crates/wallet/src/wallet/tx_builder.rs b/crates/wallet/src/wallet/tx_builder.rs index 396b694f0..5d7bd583e 100644 --- a/crates/wallet/src/wallet/tx_builder.rs +++ b/crates/wallet/src/wallet/tx_builder.rs @@ -36,8 +36,9 @@ //! # Ok::<(), anyhow::Error>(()) //! ``` -use alloc::{boxed::Box, string::String, vec::Vec}; +use alloc::{boxed::Box, vec::Vec}; use core::fmt; +use miniscript::plan::Assets; use alloc::sync::Arc; @@ -50,9 +51,10 @@ use bitcoin::{ use rand_core::RngCore; use super::coin_selection::CoinSelectionAlgorithm; -use super::utils::shuffle_slice; +use super::error::PlanError; +use super::utils::{shuffle_slice, AssetsExt}; use super::{CreateTxError, Wallet}; -use crate::collections::{BTreeMap, HashSet}; +use crate::collections::HashSet; use crate::{KeychainKind, LocalOutput, Utxo, WeightedUtxo}; /// A transaction builder @@ -117,14 +119,13 @@ pub struct TxBuilder<'a, Cs> { /// The parameters for transaction creation sans coin selection algorithm. //TODO: TxParams should eventually be exposed publicly. -#[derive(Default, Debug, Clone)] +#[derive(Default, Debug)] pub(crate) struct TxParams { pub(crate) recipients: Vec<(ScriptBuf, Amount)>, pub(crate) drain_wallet: bool, pub(crate) drain_to: Option, pub(crate) fee_policy: Option, - pub(crate) internal_policy_path: Option>>, - pub(crate) external_policy_path: Option>>, + pub(crate) assets: Option, pub(crate) utxos: Vec, pub(crate) foreign_utxos: Vec, pub(crate) unspendable: HashSet, @@ -139,7 +140,6 @@ pub(crate) struct TxParams { pub(crate) add_global_xpubs: bool, pub(crate) include_output_redeem_witness_script: bool, pub(crate) bumping_fee: Option, - pub(crate) current_height: Option, pub(crate) allow_dust: bool, } @@ -192,82 +192,6 @@ impl<'a, Cs> TxBuilder<'a, Cs> { self } - /// Set the policy path to use while creating the transaction for a given keychain. - /// - /// This method accepts a map where the key is the policy node id (see - /// [`Policy::id`](crate::descriptor::Policy::id)) and the value is the list of the indexes of - /// the items that are intended to be satisfied from the policy node (see - /// [`SatisfiableItem::Thresh::items`](crate::descriptor::policy::SatisfiableItem::Thresh::items)). - /// - /// ## Example - /// - /// An example of when the policy path is needed is the following descriptor: - /// `wsh(thresh(2,pk(A),sj:and_v(v:pk(B),n:older(6)),snj:and_v(v:pk(C),after(630000))))`, - /// derived from the miniscript policy `thresh(2,pk(A),and(pk(B),older(6)),and(pk(C),after(630000)))`. - /// It declares three descriptor fragments, and at the top level it uses `thresh()` to - /// ensure that at least two of them are satisfied. The individual fragments are: - /// - /// 1. `pk(A)` - /// 2. `and(pk(B),older(6))` - /// 3. `and(pk(C),after(630000))` - /// - /// When those conditions are combined in pairs, it's clear that the transaction needs to be created - /// differently depending on how the user intends to satisfy the policy afterwards: - /// - /// * If fragments `1` and `2` are used, the transaction will need to use a specific - /// `n_sequence` in order to spend an `OP_CSV` branch. - /// * If fragments `1` and `3` are used, the transaction will need to use a specific `locktime` - /// in order to spend an `OP_CLTV` branch. - /// * If fragments `2` and `3` are used, the transaction will need both. - /// - /// When the spending policy is represented as a tree (see - /// [`Wallet::policies`](super::Wallet::policies)), every node - /// is assigned a unique identifier that can be used in the policy path to specify which of - /// the node's children the user intends to satisfy: for instance, assuming the `thresh()` - /// root node of this example has an id of `aabbccdd`, the policy path map would look like: - /// - /// `{ "aabbccdd" => [0, 1] }` - /// - /// where the key is the node's id, and the value is a list of the children that should be - /// used, in no particular order. - /// - /// If a particularly complex descriptor has multiple ambiguous thresholds in its structure, - /// multiple entries can be added to the map, one for each node that requires an explicit path. - /// - /// ``` - /// # use std::str::FromStr; - /// # use std::collections::BTreeMap; - /// # use bitcoin::*; - /// # use bdk_wallet::*; - /// # let to_address = - /// Address::from_str("2N4eQYCbKUHCCTUjBJeHcJp9ok6J2GZsTDt") - /// .unwrap() - /// .assume_checked(); - /// # let mut wallet = doctest_wallet!(); - /// let mut path = BTreeMap::new(); - /// path.insert("aabbccdd".to_string(), vec![0, 1]); - /// - /// let builder = wallet - /// .build_tx() - /// .add_recipient(to_address.script_pubkey(), Amount::from_sat(50_000)) - /// .policy_path(path, KeychainKind::External); - /// - /// # Ok::<(), anyhow::Error>(()) - /// ``` - pub fn policy_path( - &mut self, - policy_path: BTreeMap>, - keychain: KeychainKind, - ) -> &mut Self { - let to_update = match keychain { - KeychainKind::Internal => &mut self.params.internal_policy_path, - KeychainKind::External => &mut self.params.external_policy_path, - }; - - *to_update = Some(policy_path); - self - } - /// Add the list of outpoints to the internal list of UTXOs that **must** be spent. /// /// If an error occurs while adding any of the UTXOs then none of them are added and the error is returned. @@ -393,6 +317,92 @@ impl<'a, Cs> TxBuilder<'a, Cs> { Ok(self) } + /// Add [`Assets`] to the current collection of wallet assets. + /// + /// Note that the resulting assets will include all of the wallet's "key" assets, + /// which are the ones that it can trivially infer by scanning the pubkeys of a + /// descriptor. Therefore this method is available for providing additional assets + /// that are required to complete a spending plan. + /// + /// See [`set_assets`] to specify an exact set of assets. + /// + /// # Example + /// + /// We have a descriptor defined by the policy `and(pk(A),older(144))`. This means + /// that a UTXO owned by the wallet needs to be at least 144 blocks old before it can + /// be spent. However if we want to create a transaction that spends an output before it + /// is technically mature, we can convince the wallet to consider the condition satisfied + /// and then broadcast it later when the timelock is met. + /// + /// ```rust,no_run + /// # use bitcoin::relative; + /// # use bdk_wallet::*; + /// # use miniscript::plan::Assets; + /// # let mut wallet = doctest_wallet!(); + /// let rel_locktime = relative::LockTime::from_height(144); + /// + /// let mut builder = wallet.build_tx(); + /// builder + /// .add_assets(Assets::new().older(rel_locktime)) + /// .expect("first addition should always work"); + /// ``` + /// + /// # Errors + /// + /// A [`PlanError`] is returned if attempting to combine timelocks with incompatible + /// types, for example adding a time-based relative locktime to a height-based relative + /// locktime. + /// + /// [`set_assets`]: Self::set_assets + pub fn add_assets(&mut self, assets: Assets) -> Result<&mut Self, PlanError> { + let mut new = match self.params.assets { + None => self.wallet.assets(), + Some(ref existing) => { + let mut new = Assets::new(); + new.extend(existing)?; + new + } + }; + new.extend(&assets)?; + self.params.assets = Some(new); + Ok(self) + } + + /// Set the spend [`Assets`]. + /// + /// This should be used to specify a particular spending path where multiple paths + /// may exist, in which case all of the assets required to complete a spending plan + /// must be known at this point. See also [`add_assets`](Self::add_assets). + /// + /// # Example + /// + /// We have a descriptor defined by the policy `or(pk(A),and(pk(B),after(1732934156)))` + /// and we want to create a transaction using the second spending path, that is we intend + /// to produce a signature for `B` and build a transaction that will be valid after + /// the time `1732934156`. + /// + /// ```rust,no_run + /// # use std::str::FromStr; + /// # use bitcoin::{absolute, bip32::*}; + /// # use bdk_wallet::*; + /// # use miniscript::plan::*; + /// # let mut wallet = doctest_wallet!(); + /// # let mut builder = wallet.build_tx(); + /// let fingerprint = Fingerprint::from_hex("f6a5cb8b").unwrap(); + /// let derivation_path = DerivationPath::from_str("86h/0h/0h/0").unwrap(); + /// let origin = (fingerprint, derivation_path); + /// let abs_locktime = absolute::LockTime::from_consensus(1732934156); + /// + /// let mut assets = Assets::new(); + /// assets.keys.insert((origin, CanSign::default())); + /// assets = assets.after(abs_locktime); + /// builder.set_assets(assets); + /// ``` + pub fn set_assets(&mut self, assets: Assets) -> &mut Self { + self.params.assets = Some(assets); + self + } + /// Only spend utxos added by [`add_utxo`]. /// /// The wallet will **not** add additional utxos to the transaction even if they are needed to @@ -538,23 +548,6 @@ impl<'a, Cs> TxBuilder<'a, Cs> { self } - /// Set the current blockchain height. - /// - /// This will be used to: - /// 1. Set the nLockTime for preventing fee sniping. - /// **Note**: This will be ignored if you manually specify a nlocktime using [`TxBuilder::nlocktime`]. - /// 2. Decide whether coinbase outputs are mature or not. If the coinbase outputs are not - /// mature at `current_height`, we ignore them in the coin selection. - /// If you want to create a transaction that spends immature coinbase inputs, manually - /// add them using [`TxBuilder::add_utxos`]. - /// - /// In both cases, if you don't provide a current height, we use the last sync height. - pub fn current_height(&mut self, height: u32) -> &mut Self { - self.params.current_height = - Some(absolute::LockTime::from_height(height).expect("Invalid height")); - self - } - /// Set whether or not the dust limit is checked. /// /// **Note**: by avoiding a dust limit check you may end up with a transaction that is non-standard. diff --git a/crates/wallet/src/wallet/utils.rs b/crates/wallet/src/wallet/utils.rs index 76d0aaa75..751912692 100644 --- a/crates/wallet/src/wallet/utils.rs +++ b/crates/wallet/src/wallet/utils.rs @@ -11,11 +11,16 @@ use bitcoin::secp256k1::{All, Secp256k1}; use bitcoin::{absolute, relative, Amount, Script, Sequence}; - +use miniscript::plan::Assets; use miniscript::{MiniscriptKey, Satisfier, ToPublicKey}; - use rand_core::RngCore; +use crate::error::PlanError; +use crate::Condition; + +/// `Secp256k1` context with `All` capabilities +pub(crate) type SecpCtx = Secp256k1; + /// Trait to check if a value is below the dust limit. /// We are performing dust value calculation for a given script public key using rust-bitcoin to /// keep it compatible with network dust rate @@ -72,6 +77,14 @@ pub(crate) fn check_nsequence_rbf(sequence: Sequence, csv: Sequence) -> bool { true } +pub fn merge_nsequence(a: Sequence, b: Sequence) -> Result { + if a.is_time_locked() != b.is_time_locked() { + Err(PlanError::MixedTimelockUnits) + } else { + Ok(a.max(b)) + } +} + impl Satisfier for After { fn check_after(&self, n: absolute::LockTime) -> bool { if let Some(current_height) = self.current_height { @@ -118,7 +131,7 @@ impl Satisfier for Older { } } -// The Knuth shuffling algorithm based on the original [Fisher-Yates method](https://en.wikipedia.org/wiki/Fisher%E2%80%93Yates_shuffle) +/// The Knuth shuffling algorithm based on the original [Fisher-Yates method](https://en.wikipedia.org/wiki/Fisher%E2%80%93Yates_shuffle) pub(crate) fn shuffle_slice(list: &mut [T], rng: &mut impl RngCore) { if list.is_empty() { return; @@ -131,7 +144,31 @@ pub(crate) fn shuffle_slice(list: &mut [T], rng: &mut impl RngCore) { } } -pub(crate) type SecpCtx = Secp256k1; +/// Implemented for [`Assets`] to allow extending one instance with a reference of the same type. +pub(crate) trait AssetsExt { + /// Extend `self` with the contents of `other`. + fn extend(&mut self, other: &Self) -> Result<(), PlanError>; +} + +impl AssetsExt for Assets { + fn extend(&mut self, other: &Assets) -> Result<(), PlanError> { + self.keys.extend(other.keys.clone()); + self.sha256_preimages.extend(other.sha256_preimages.clone()); + self.hash256_preimages + .extend(other.hash256_preimages.clone()); + self.ripemd160_preimages + .extend(other.ripemd160_preimages.clone()); + self.hash160_preimages + .extend(other.hash160_preimages.clone()); + + self.absolute_timelock = + Condition::merge_abs_locktime(self.absolute_timelock, other.absolute_timelock)?; + self.relative_timelock = + Condition::merge_rel_locktime(self.relative_timelock, other.relative_timelock)?; + + Ok(()) + } +} #[cfg(test)] mod test { @@ -139,8 +176,8 @@ mod test { // otherwise it's time-based pub(crate) const SEQUENCE_LOCKTIME_TYPE_FLAG: u32 = 1 << 22; - use super::{check_nsequence_rbf, shuffle_slice, IsDust}; - use crate::bitcoin::{Address, Network, Sequence}; + use super::{check_nsequence_rbf, shuffle_slice, AssetsExt, IsDust}; + use crate::bitcoin::{absolute, relative, Address, Network, Sequence}; use alloc::vec::Vec; use core::str::FromStr; use rand::{rngs::StdRng, thread_rng, SeedableRng}; @@ -247,4 +284,26 @@ mod test { shuffle_slice(&mut test, &mut rng); assert_eq!(test, &[0, 4, 1, 2, 5]); } + + #[test] + fn cannot_extend_assets_with_different_lock_type() { + use miniscript::plan::Assets; + let mut assets = Assets::new(); + let height_lock = absolute::LockTime::from_consensus(100_000); + assert!(height_lock.is_block_height()); + let time_lock = absolute::LockTime::from_consensus(500_000_001); + assert!(time_lock.is_block_time()); + + assets = assets.after(height_lock); + assets.extend(&Assets::new().after(time_lock)).unwrap_err(); + + let mut assets = Assets::new(); + let height_lock = relative::LockTime::from_consensus(6).unwrap(); + assert!(height_lock.is_block_height()); + let time_lock = relative::LockTime::from_seconds_floor(42 * 512).unwrap(); + assert!(time_lock.is_block_time()); + + assets = assets.older(height_lock); + assets.extend(&Assets::new().older(time_lock)).unwrap_err(); + } } diff --git a/crates/wallet/tests/create_tx_plan.rs b/crates/wallet/tests/create_tx_plan.rs new file mode 100644 index 000000000..aa3ed88c4 --- /dev/null +++ b/crates/wallet/tests/create_tx_plan.rs @@ -0,0 +1,203 @@ +use bitcoin::{absolute, relative, secp256k1::Secp256k1, Amount, ScriptBuf, Sequence}; +use miniscript::{ + descriptor::{DescriptorPublicKey, KeyMap}, + plan::Assets, + Descriptor, ForEachKey, +}; + +use bdk_wallet::error::{CreateTxError, PlanError}; +use bdk_wallet::miniscript; +use bdk_wallet::test_utils::*; + +fn parse_descriptor(s: &str) -> (Descriptor, KeyMap) { + >::parse_descriptor(&Secp256k1::new(), s) + .expect("failed to parse descriptor") +} + +#[test] +fn construct_plan_from_assets() { + // technically this is tested in rust-miniscript and is only + // here for demonstration + let (desc, _) = parse_descriptor(get_test_single_sig_cltv()); + + let mut pk = vec![]; + desc.for_each_key(|k| { + pk.push(k.clone()); + true + }); + + // locktime not met + let lt = absolute::LockTime::from_consensus(99_999); + let assets = Assets::new().add(pk.clone()).after(lt); + let definite_desc = desc.at_derivation_index(0).unwrap(); + definite_desc.plan(&assets).unwrap_err(); + + // no keys + let lt = absolute::LockTime::from_consensus(100_000); + let assets = Assets::new().after(lt); + let definite_desc = desc.at_derivation_index(0).unwrap(); + definite_desc.plan(&assets).unwrap_err(); + + // assets are sufficient + let lt = absolute::LockTime::from_consensus(100_000); + let assets = Assets::new().add(pk).after(lt); + let definite_desc = desc.at_derivation_index(0).unwrap(); + definite_desc.plan(&assets).unwrap(); +} + +#[test] +fn create_tx_assets() -> anyhow::Result<()> { + let abs_locktime = absolute::LockTime::from_consensus(100_000); + let abs_locktime_t = absolute::LockTime::from_consensus(500_000_001); + let rel_locktime = relative::LockTime::from_consensus(6).unwrap(); + let rel_locktime_144 = relative::LockTime::from_consensus(144).unwrap(); + + let default_locktime = absolute::LockTime::from_consensus(2000); + let default_sequence = Sequence::ENABLE_RBF_NO_LOCKTIME; + + // Test that the assets we pass in are enough to spend outputs defined by a descriptor + struct TestCase { + name: &'static str, + desc: &'static str, + assets: Option, + exp_cltv: absolute::LockTime, + exp_sequence: Sequence, + // whether the result of `finish` is Ok + exp_result: bool, + } + let cases = vec![ + TestCase { + name: "single sig no assets ok", + desc: get_test_tr_single_sig(), + assets: None, + exp_cltv: default_locktime, + exp_sequence: default_sequence, + exp_result: true, + }, + TestCase { + name: "single sig + cltv", + desc: get_test_single_sig_cltv(), + assets: Some(Assets::new().after(abs_locktime)), + exp_cltv: abs_locktime, + exp_sequence: default_sequence, + exp_result: true, + }, + TestCase { + name: "single sig + cltv timestamp", + desc: get_test_single_sig_cltv_timestamp(), + assets: Some(Assets::new().after(abs_locktime_t)), + exp_cltv: abs_locktime_t, + exp_sequence: default_sequence, + exp_result: true, + }, + TestCase { + name: "single sig + csv", + desc: get_test_single_sig_csv(), + assets: Some(Assets::new().older(rel_locktime)), + exp_cltv: default_locktime, + exp_sequence: Sequence(6), + exp_result: true, + }, + TestCase { + name: "optional csv, no assets ok", + desc: get_test_a_or_b_plus_csv(), + assets: None, + exp_cltv: default_locktime, + exp_sequence: default_sequence, + exp_result: true, + }, + TestCase { + name: "optional csv, use csv", + desc: get_test_a_or_b_plus_csv(), + assets: { + // 2 possible spend paths. we only include the key we + // intend to sign with + let (_, keymap) = parse_descriptor(get_test_a_or_b_plus_csv()); + let pk = keymap + .keys() + .find(|k| k.master_fingerprint().to_string() == "9c5eab64") + .unwrap(); + let assets = Assets::new().add(pk.clone()).older(rel_locktime_144); + Some(assets) + }, + exp_cltv: default_locktime, + exp_sequence: Sequence(144), + exp_result: true, + }, + TestCase { + name: "insufficient assets cltv", + desc: get_test_single_sig_cltv(), + assets: Some(Assets::new().after(absolute::LockTime::from_consensus(99_999))), + exp_cltv: default_locktime, + exp_sequence: default_sequence, + exp_result: false, + }, + TestCase { + name: "insufficient assets csv", + desc: get_test_single_sig_csv(), + assets: Some(Assets::new().older(relative::LockTime::from_height(5))), + exp_cltv: default_locktime, + exp_sequence: default_sequence, + exp_result: false, + }, + TestCase { + name: "insufficient assets (no assets)", + desc: get_test_single_sig_cltv(), + assets: None, + exp_cltv: default_locktime, + exp_sequence: default_sequence, + exp_result: false, + }, + TestCase { + name: "wrong locktime (after)", + desc: get_test_single_sig_csv(), + assets: Some(Assets::new().after(abs_locktime)), + exp_cltv: default_locktime, + exp_sequence: default_sequence, + exp_result: false, + }, + TestCase { + name: "wrong locktime (older)", + desc: get_test_single_sig_cltv(), + assets: Some(Assets::new().older(rel_locktime)), + exp_cltv: default_locktime, + exp_sequence: default_sequence, + exp_result: false, + }, + ]; + + let recip = ScriptBuf::from_hex("0014446906a6560d8ad760db3156706e72e171f3a2aa")?; + + for test in cases { + let (mut wallet, _) = get_funded_wallet_single(test.desc); + assert_eq!(wallet.latest_checkpoint().height(), 2_000); + let mut builder = wallet.build_tx(); + if let Some(assets) = test.assets { + if assets.keys.is_empty() { + // add to existing + builder.add_assets(assets).unwrap(); + } else { + // only use the given assets + builder.set_assets(assets); + } + } + builder.add_recipient(recip.clone(), Amount::from_sat(10_000)); + if test.exp_result { + let psbt = builder.finish().expect("tx builder should not fail"); + assert_eq!(psbt.unsigned_tx.lock_time, test.exp_cltv, "{}", test.name); + assert_eq!( + psbt.unsigned_tx.input[0].sequence, test.exp_sequence, + "{}", + test.name + ); + } else { + let err = builder.finish().expect_err("expected create tx fail"); + assert!( + matches!(err, CreateTxError::Plan(PlanError::Plan(_))), + "{}", + test.name + ); + } + } + Ok(()) +} diff --git a/crates/wallet/tests/wallet.rs b/crates/wallet/tests/wallet.rs index 7ca60022e..764c804cc 100644 --- a/crates/wallet/tests/wallet.rs +++ b/crates/wallet/tests/wallet.rs @@ -17,15 +17,14 @@ use bdk_wallet::{KeychainKind, LoadError, LoadMismatch, LoadWithPersistError}; use bitcoin::constants::{ChainHash, COINBASE_MATURITY}; use bitcoin::hashes::Hash; use bitcoin::key::Secp256k1; -use bitcoin::psbt; use bitcoin::script::PushBytesBuf; use bitcoin::sighash::{EcdsaSighashType, TapSighashType}; use bitcoin::taproot::TapNodeHash; use bitcoin::{ - absolute, transaction, Address, Amount, BlockHash, FeeRate, Network, OutPoint, ScriptBuf, - Sequence, Transaction, TxIn, TxOut, Txid, Weight, + absolute, psbt, relative, transaction, Address, Amount, BlockHash, FeeRate, Network, OutPoint, + ScriptBuf, Sequence, Transaction, TxIn, TxOut, Txid, }; -use miniscript::{descriptor::KeyMap, Descriptor, DescriptorPublicKey}; +use miniscript::{descriptor::KeyMap, plan::Assets, Descriptor, DescriptorPublicKey}; use rand::rngs::StdRng; use rand::SeedableRng; @@ -550,6 +549,8 @@ fn test_create_tx_version_1_csv() { let addr = wallet.next_unused_address(KeychainKind::External); let mut builder = wallet.build_tx(); builder + .add_assets(Assets::new().older(relative::LockTime::from_height(6))) + .unwrap() .add_recipient(addr.script_pubkey(), Amount::from_sat(25_000)) .version(1); assert!(matches!(builder.finish(), Err(CreateTxError::Version1Csv))); @@ -601,6 +602,14 @@ fn test_create_tx_fee_sniping_locktime_last_sync() { #[test] fn test_create_tx_default_locktime_cltv() { let (mut wallet, _) = get_funded_wallet_single(get_test_single_sig_cltv()); + // wallet can satisfy its own timelock if the local height is great enough + insert_checkpoint( + &mut wallet, + BlockId { + height: 100_000, + hash: BlockHash::all_zeros(), + }, + ); let addr = wallet.next_unused_address(KeychainKind::External); let mut builder = wallet.build_tx(); builder.add_recipient(addr.script_pubkey(), Amount::from_sat(25_000)); @@ -612,11 +621,17 @@ fn test_create_tx_default_locktime_cltv() { #[test] fn test_create_tx_custom_locktime() { let (mut wallet, _) = get_funded_wallet_wpkh(); + insert_checkpoint( + &mut wallet, + BlockId { + height: 630_001, + hash: BlockHash::all_zeros(), + }, + ); let addr = wallet.next_unused_address(KeychainKind::External); let mut builder = wallet.build_tx(); builder .add_recipient(addr.script_pubkey(), Amount::from_sat(25_000)) - .current_height(630_001) .nlocktime(absolute::LockTime::from_height(630_000).unwrap()); let psbt = builder.finish().unwrap(); @@ -632,6 +647,8 @@ fn test_create_tx_custom_locktime_compatible_with_cltv() { let addr = wallet.next_unused_address(KeychainKind::External); let mut builder = wallet.build_tx(); builder + .add_assets(Assets::new().after(absolute::LockTime::from_consensus(100_000))) + .unwrap() .add_recipient(addr.script_pubkey(), Amount::from_sat(25_000)) .nlocktime(absolute::LockTime::from_height(630_000).unwrap()); let psbt = builder.finish().unwrap(); @@ -645,6 +662,8 @@ fn test_create_tx_custom_locktime_incompatible_with_cltv() { let addr = wallet.next_unused_address(KeychainKind::External); let mut builder = wallet.build_tx(); builder + .add_assets(Assets::new().after(absolute::LockTime::from_consensus(100_000))) + .unwrap() .add_recipient(addr.script_pubkey(), Amount::from_sat(25_000)) .nlocktime(absolute::LockTime::from_height(50000).unwrap()); assert!(matches!(builder.finish(), @@ -659,6 +678,8 @@ fn test_create_tx_custom_csv() { let addr = wallet.next_unused_address(KeychainKind::External); let mut builder = wallet.build_tx(); builder + .add_assets(Assets::new().older(relative::LockTime::from_height(6))) + .unwrap() .set_exact_sequence(Sequence(42)) .add_recipient(addr.script_pubkey(), Amount::from_sat(25_000)); let psbt = builder.finish().unwrap(); @@ -671,7 +692,10 @@ fn test_create_tx_no_rbf_csv() { let (mut wallet, _) = get_funded_wallet_single(get_test_single_sig_csv()); let addr = wallet.next_unused_address(KeychainKind::External); let mut builder = wallet.build_tx(); - builder.add_recipient(addr.script_pubkey(), Amount::from_sat(25_000)); + builder + .add_assets(Assets::new().older(relative::LockTime::from_height(6))) + .unwrap() + .add_recipient(addr.script_pubkey(), Amount::from_sat(25_000)); let psbt = builder.finish().unwrap(); assert_eq!(psbt.unsigned_tx.input[0].sequence, Sequence(6)); @@ -683,6 +707,8 @@ fn test_create_tx_incompatible_csv() { let addr = wallet.next_unused_address(KeychainKind::External); let mut builder = wallet.build_tx(); builder + .add_assets(Assets::new().older(relative::LockTime::from_height(6))) + .unwrap() .add_recipient(addr.script_pubkey(), Amount::from_sat(25_000)) .set_exact_sequence(Sequence(3)); assert!(matches!(builder.finish(), @@ -695,7 +721,10 @@ fn test_create_tx_with_default_rbf_csv() { let (mut wallet, _) = get_funded_wallet_single(get_test_single_sig_csv()); let addr = wallet.next_unused_address(KeychainKind::External); let mut builder = wallet.build_tx(); - builder.add_recipient(addr.script_pubkey(), Amount::from_sat(25_000)); + builder + .add_assets(Assets::new().older(relative::LockTime::from_height(6))) + .unwrap() + .add_recipient(addr.script_pubkey(), Amount::from_sat(25_000)); let psbt = builder.finish().unwrap(); // When CSV is enabled it takes precedence over the rbf value (unless forced by the user). // It will be set to the OP_CSV value, in this case 6 @@ -707,8 +736,11 @@ fn test_create_tx_no_rbf_cltv() { let (mut wallet, _) = get_funded_wallet_single(get_test_single_sig_cltv()); let addr = wallet.next_unused_address(KeychainKind::External); let mut builder = wallet.build_tx(); - builder.add_recipient(addr.script_pubkey(), Amount::from_sat(25_000)); - builder.set_exact_sequence(Sequence(0xFFFFFFFE)); + builder + .add_assets(Assets::new().after(absolute::LockTime::from_consensus(100_000))) + .unwrap() + .add_recipient(addr.script_pubkey(), Amount::from_sat(25_000)) + .set_exact_sequence(Sequence(0xFFFFFFFE)); let psbt = builder.finish().unwrap(); assert_eq!(psbt.unsigned_tx.input[0].sequence, Sequence(0xFFFFFFFE)); @@ -1288,96 +1320,30 @@ fn test_create_tx_manually_selected_insufficient() { } #[test] -#[should_panic(expected = "SpendingPolicyRequired(External)")] -fn test_create_tx_policy_path_required() { - let (mut wallet, _) = get_funded_wallet_single(get_test_a_or_b_plus_csv()); +fn test_create_tx_spend_assets_required() { + use bdk_wallet::error::PlanError; + let (mut wallet, _) = get_funded_wallet_single(get_test_single_sig_csv()); let addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX") .unwrap() .assume_checked(); let mut builder = wallet.build_tx(); builder.add_recipient(addr.script_pubkey(), Amount::from_sat(10_000)); - builder.finish().unwrap(); -} -#[test] -fn test_create_tx_policy_path_no_csv() { - let (descriptor, change_descriptor) = get_test_wpkh_and_change_desc(); - let mut wallet = Wallet::create(descriptor, change_descriptor) - .network(Network::Regtest) - .create_wallet_no_persist() - .expect("wallet"); + // the wallet's only utxo has not reached the required age of 6 blocks + let res = builder.finish(); + assert!(matches!(res, Err(CreateTxError::Plan(PlanError::Plan(_))))); - let tx = Transaction { - version: transaction::Version::non_standard(0), - lock_time: absolute::LockTime::ZERO, - input: vec![], - output: vec![TxOut { - script_pubkey: wallet - .next_unused_address(KeychainKind::External) - .script_pubkey(), - value: Amount::from_sat(50_000), - }], + // now advance the local chain by 6 blocks + let local_tip = wallet.latest_checkpoint().height(); + let new_tip = BlockId { + height: local_tip + 6, + hash: BlockHash::all_zeros(), }; - insert_tx(&mut wallet, tx); - - let external_policy = wallet.policies(KeychainKind::External).unwrap().unwrap(); - let root_id = external_policy.id; - // child #0 is just the key "A" - let path = vec![(root_id, vec![0])].into_iter().collect(); - - let addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX") - .unwrap() - .assume_checked(); + insert_checkpoint(&mut wallet, new_tip); let mut builder = wallet.build_tx(); - builder - .add_recipient(addr.script_pubkey(), Amount::from_sat(30_000)) - .policy_path(path, KeychainKind::External); - let psbt = builder.finish().unwrap(); - - assert_eq!(psbt.unsigned_tx.input[0].sequence, Sequence(0xFFFFFFFD)); -} - -#[test] -fn test_create_tx_policy_path_use_csv() { - let (mut wallet, _) = get_funded_wallet_single(get_test_a_or_b_plus_csv()); - - let external_policy = wallet.policies(KeychainKind::External).unwrap().unwrap(); - let root_id = external_policy.id; - // child #1 is or(pk(B),older(144)) - let path = vec![(root_id, vec![1])].into_iter().collect(); - - let addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX") - .unwrap() - .assume_checked(); - let mut builder = wallet.build_tx(); - builder - .add_recipient(addr.script_pubkey(), Amount::from_sat(30_000)) - .policy_path(path, KeychainKind::External); - let psbt = builder.finish().unwrap(); - - assert_eq!(psbt.unsigned_tx.input[0].sequence, Sequence(144)); -} - -#[test] -fn test_create_tx_policy_path_ignored_subtree_with_csv() { - let (mut wallet, _) = get_funded_wallet_single("wsh(or_d(pk(cRjo6jqfVNP33HhSS76UhXETZsGTZYx8FMFvR9kpbtCSV1PmdZdu),or_i(and_v(v:pkh(cVpPVruEDdmutPzisEsYvtST1usBR3ntr8pXSyt6D2YYqXRyPcFW),older(30)),and_v(v:pkh(cMnkdebixpXMPfkcNEjjGin7s94hiehAH4mLbYkZoh9KSiNNmqC8),older(90)))))"); - - let external_policy = wallet.policies(KeychainKind::External).unwrap().unwrap(); - let root_id = external_policy.id; - // child #0 is pk(cRjo6jqfVNP33HhSS76UhXETZsGTZYx8FMFvR9kpbtCSV1PmdZdu) - let path = vec![(root_id, vec![0])].into_iter().collect(); - - let addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX") - .unwrap() - .assume_checked(); - let mut builder = wallet.build_tx(); - builder - .add_recipient(addr.script_pubkey(), Amount::from_sat(30_000)) - .policy_path(path, KeychainKind::External); - let psbt = builder.finish().unwrap(); - - assert_eq!(psbt.unsigned_tx.input[0].sequence, Sequence(0xFFFFFFFD)); + builder.add_recipient(addr.script_pubkey(), Amount::from_sat(10_000)); + let _psbt = builder.finish().unwrap(); } #[test] @@ -2140,8 +2106,8 @@ fn test_bump_fee_drain_wallet() { } #[test] -#[should_panic(expected = "InsufficientFunds")] fn test_bump_fee_remove_output_manually_selected_only() { + use bdk_wallet::coin_selection::InsufficientFunds; let (mut wallet, _) = get_funded_wallet_wpkh(); // receive an extra tx so that our wallet has two utxos. then we manually pick only one of // them, and make sure that `bump_fee` doesn't try to add more. This fails because we've @@ -2194,7 +2160,11 @@ fn test_bump_fee_remove_output_manually_selected_only() { builder .manually_selected_only() .fee_rate(FeeRate::from_sat_per_vb_unchecked(255)); - builder.finish().unwrap(); + let res = builder.finish(); + assert!(matches!( + res, + Err(CreateTxError::CoinSelection(InsufficientFunds { .. })) + )); } #[test] @@ -2266,7 +2236,7 @@ fn test_bump_fee_add_input() { sent_received.1 ); - assert_fee_rate!(psbt, fee.unwrap_or(Amount::ZERO), FeeRate::from_sat_per_vb_unchecked(50), @add_signature); + assert_fee_rate!(psbt, fee.unwrap_or(Amount::ZERO), feerate_unchecked(50.2), @add_signature); } #[test] @@ -2386,7 +2356,7 @@ fn test_bump_fee_no_change_add_input_and_change() { Amount::from_sat(75_000) - original_send_all_amount - fee.unwrap_or(Amount::ZERO) ); - assert_fee_rate!(psbt, fee.unwrap_or(Amount::ZERO), FeeRate::from_sat_per_vb_unchecked(50), @add_signature); + assert_fee_rate!(psbt, fee.unwrap_or(Amount::ZERO), feerate_unchecked(50.2), @add_signature); } #[test] @@ -2407,7 +2377,6 @@ fn test_bump_fee_add_input_change_dust() { for txin in &mut tx.input { txin.witness.push([0x00; P2WPKH_FAKE_WITNESS_SIZE]); // to get realistic weight } - let original_tx_weight = tx.weight(); assert_eq!(tx.input.len(), 1); assert_eq!(tx.output.len(), 2); let txid = tx.compute_txid(); @@ -2417,18 +2386,9 @@ fn test_bump_fee_add_input_change_dust() { // We set a fee high enough that during rbf we are forced to add // a new input and also that we have to remove the change // that we had previously + // fee_absolute = 50_000 + 25_000 - 45_000 - // We calculate the new weight as: - // original weight - // + extra input weight: 160 WU = (32 (prevout) + 4 (vout) + 4 (nsequence)) * 4 - // + input satisfaction weight: 112 WU = 106 (witness) + 2 (witness len) + (1 (script len)) * 4 - // - change output weight: 124 WU = (8 (value) + 1 (script len) + 22 (script)) * 4 - let new_tx_weight = - original_tx_weight + Weight::from_wu(160) + Weight::from_wu(112) - Weight::from_wu(124); - // two inputs (50k, 25k) and one output (45k) - epsilon - // We use epsilon here to avoid asking for a slightly too high feerate - let fee_abs = 50_000 + 25_000 - 45_000 - 10; - builder.fee_rate(Amount::from_sat(fee_abs) / new_tx_weight); + builder.fee_absolute(Amount::from_sat(30_000)); let psbt = builder.finish().unwrap(); let sent_received = wallet.sent_and_received(&psbt.clone().extract_tx().expect("failed to extract tx")); @@ -3309,15 +3269,8 @@ fn test_taproot_psbt_populate_tap_key_origins_repeated_key() { let (mut wallet, _) = get_funded_wallet(get_test_tr_repeated_key(), get_test_tr_single_sig()); let addr = wallet.reveal_next_address(KeychainKind::External); - let path = vec![("rn4nre9c".to_string(), vec![0])] - .into_iter() - .collect(); - let mut builder = wallet.build_tx(); - builder - .drain_to(addr.script_pubkey()) - .drain_wallet() - .policy_path(path, KeychainKind::External); + builder.drain_to(addr.script_pubkey()).drain_wallet(); let psbt = builder.finish().unwrap(); let mut input_key_origins = psbt.inputs[0] @@ -3879,9 +3832,7 @@ fn test_spend_coinbase() { .unwrap() .assume_checked(); let mut builder = wallet.build_tx(); - builder - .add_recipient(addr.script_pubkey(), balance.immature / 2) - .current_height(confirmation_height); + builder.add_recipient(addr.script_pubkey(), balance.immature / 2); assert!(matches!( builder.finish(), Err(CreateTxError::CoinSelection( @@ -3893,10 +3844,15 @@ fn test_spend_coinbase() { )); // Still unspendable... + insert_checkpoint( + &mut wallet, + BlockId { + height: not_yet_mature_time, + hash: BlockHash::all_zeros(), + }, + ); let mut builder = wallet.build_tx(); - builder - .add_recipient(addr.script_pubkey(), balance.immature / 2) - .current_height(not_yet_mature_time); + builder.add_recipient(addr.script_pubkey(), balance.immature / 2); assert_matches!( builder.finish(), Err(CreateTxError::CoinSelection( @@ -3907,6 +3863,7 @@ fn test_spend_coinbase() { )) ); + // Finally the coinbase matures insert_checkpoint( &mut wallet, BlockId { @@ -3925,20 +3882,16 @@ fn test_spend_coinbase() { } ); let mut builder = wallet.build_tx(); - builder - .add_recipient(addr.script_pubkey(), balance.confirmed / 2) - .current_height(maturity_time); + builder.add_recipient(addr.script_pubkey(), balance.confirmed / 2); builder.finish().unwrap(); } #[test] fn test_allow_dust_limit() { - let (mut wallet, _) = get_funded_wallet_single(get_test_single_sig_cltv()); - + let (mut wallet, _) = get_funded_wallet_wpkh(); let addr = wallet.next_unused_address(KeychainKind::External); let mut builder = wallet.build_tx(); - builder.add_recipient(addr.script_pubkey(), Amount::ZERO); assert_matches!( @@ -3947,7 +3900,6 @@ fn test_allow_dust_limit() { ); let mut builder = wallet.build_tx(); - builder .allow_dust(true) .add_recipient(addr.script_pubkey(), Amount::ZERO); From 69cd5386edf411021027fcdf0b473a46f3b672aa Mon Sep 17 00:00:00 2001 From: valued mammal Date: Thu, 12 Dec 2024 08:36:01 -0500 Subject: [PATCH 4/5] test: add test `spend_path_from_assets` --- crates/wallet/tests/create_tx_plan.rs | 45 +++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/crates/wallet/tests/create_tx_plan.rs b/crates/wallet/tests/create_tx_plan.rs index aa3ed88c4..da29da37a 100644 --- a/crates/wallet/tests/create_tx_plan.rs +++ b/crates/wallet/tests/create_tx_plan.rs @@ -14,6 +14,51 @@ fn parse_descriptor(s: &str) -> (Descriptor, KeyMap) { .expect("failed to parse descriptor") } +#[test] +fn spend_path_from_assets() { + use std::str::FromStr; + + // Test the plan result for each spending path given the policy + // thresh(2,pk(A),and(pk(B),older(6)),and(pk(C),after(630000))) + + let a = "020ffa2c93a3eeed29768a338694da24ad60aa18e06eaf193e7945ad097f21d953"; + let pka = DescriptorPublicKey::from_str(a).unwrap(); + let b = "03f7781a611d88a5f98301ca3de3b33f6c856e4572cea2fefa8b274fe2fc516d3e"; + let pkb = DescriptorPublicKey::from_str(b).unwrap(); + let c = "02bc273a1aca4c50c43572dce6d4857523915bdf09d5f5f4dcedea7c1f9cc41099"; + let pkc = DescriptorPublicKey::from_str(c).unwrap(); + + let (desc, _) = parse_descriptor("wsh(thresh(2,pk(020ffa2c93a3eeed29768a338694da24ad60aa18e06eaf193e7945ad097f21d953),snj:and_v(v:pk(03f7781a611d88a5f98301ca3de3b33f6c856e4572cea2fefa8b274fe2fc516d3e),older(6)),snj:and_v(v:pk(02bc273a1aca4c50c43572dce6d4857523915bdf09d5f5f4dcedea7c1f9cc41099),after(850000))))#09tf9qx0"); + + // A + B + let def = desc.at_derivation_index(0).unwrap(); + let assets = Assets::new() + .add(vec![pka.clone(), pkb.clone()]) + .older(relative::LockTime::from_height(6)); + let plan = def.plan(&assets).unwrap(); + assert!(plan.absolute_timelock.is_none()); + assert_eq!(plan.relative_timelock.unwrap().to_sequence(), Sequence(6)); + + // A + C + let def = desc.at_derivation_index(0).unwrap(); + let assets = Assets::new() + .add(vec![pka.clone(), pkc.clone()]) + .after(absolute::LockTime::from_consensus(850_000)); + let plan = def.plan(&assets).unwrap(); + assert!(plan.relative_timelock.is_none()); + assert_eq!(plan.absolute_timelock.unwrap().to_consensus_u32(), 850_000); + + // B + C + let def = desc.at_derivation_index(0).unwrap(); + let assets = Assets::new() + .add(vec![pkb, pkc]) + .older(relative::LockTime::from_height(6)) + .after(absolute::LockTime::from_consensus(850_000)); + let plan = def.plan(&assets).unwrap(); + assert_eq!(plan.relative_timelock.unwrap().to_sequence(), Sequence(6)); + assert_eq!(plan.absolute_timelock.unwrap().to_consensus_u32(), 850_000); +} + #[test] fn construct_plan_from_assets() { // technically this is tested in rust-miniscript and is only From 17cd5286035f35adeb5fe4d3125671f131bb5cf8 Mon Sep 17 00:00:00 2001 From: valued mammal Date: Sat, 30 Nov 2024 17:28:50 -0500 Subject: [PATCH 5/5] fix(wallet)!: remove `sign_options` param from `finalize_psbt` - clean up unused `fee_amount` in `create_tx` --- crates/wallet/src/wallet/mod.rs | 41 +++++++++++---------------------- 1 file changed, 13 insertions(+), 28 deletions(-) diff --git a/crates/wallet/src/wallet/mod.rs b/crates/wallet/src/wallet/mod.rs index 0cb324e53..562aa4225 100644 --- a/crates/wallet/src/wallet/mod.rs +++ b/crates/wallet/src/wallet/mod.rs @@ -1503,7 +1503,6 @@ impl Wallet { rng, ) .map_err(CreateTxError::CoinSelection)?; - fee_amount += coin_selection.fee_amount; let excess = &coin_selection.excess; tx.input = coin_selection @@ -1544,28 +1543,18 @@ impl Wallet { } } - match excess { - Excess::NoChange { - remaining_amount, .. - } => fee_amount += *remaining_amount, - Excess::Change { amount, fee } => { - if self.is_mine(drain_script.clone()) { - received += *amount; - } - fee_amount += *fee; - - // create drain output - let drain_output = TxOut { - value: *amount, - script_pubkey: drain_script, - }; + if let Excess::Change { amount, .. } = excess { + // create drain output + let drain_output = TxOut { + value: *amount, + script_pubkey: drain_script, + }; - // TODO: We should pay attention when adding a new output: this might increase - // the length of the "number of vouts" parameter by 2 bytes, potentially making - // our feerate too low - tx.output.push(drain_output); - } - }; + // TODO: We should pay attention when adding a new output: this might increase + // the length of the "number of vouts" parameter by 2 bytes, potentially making + // our feerate too low + tx.output.push(drain_output); + } // sort input/outputs according to the chosen algorithm params.ordering.sort_tx_with_aux_rand(&mut tx, rng); @@ -1808,7 +1797,7 @@ impl Wallet { // attempt to finalize if sign_options.try_finalize { - self.finalize_psbt(psbt, sign_options) + self.finalize_psbt(psbt) } else { Ok(false) } @@ -1851,11 +1840,7 @@ impl Wallet { /// Returns `true` if the PSBT could be finalized, and `false` otherwise. /// /// The [`SignOptions`] can be used to tweak the behavior of the finalizer. - pub fn finalize_psbt( - &self, - psbt: &mut Psbt, - _sign_options: SignOptions, - ) -> Result { + pub fn finalize_psbt(&self, psbt: &mut Psbt) -> Result { let tx = &psbt.unsigned_tx; let mut finished = true;