From 096b8ef781b21e7da690b9ca2e4b326880d6ac0b Mon Sep 17 00:00:00 2001 From: Steve Myers Date: Thu, 16 May 2024 21:47:56 -0500 Subject: [PATCH] fix(wallet): remove TxBuilder::allow_shrinking function and TxBuilderContext --- crates/wallet/src/wallet/mod.rs | 8 +- crates/wallet/src/wallet/tx_builder.rs | 253 +++++++++---------------- crates/wallet/tests/wallet.rs | 53 ++++-- 3 files changed, 121 insertions(+), 193 deletions(-) diff --git a/crates/wallet/src/wallet/mod.rs b/crates/wallet/src/wallet/mod.rs index 0c37b55b..e80584dc 100644 --- a/crates/wallet/src/wallet/mod.rs +++ b/crates/wallet/src/wallet/mod.rs @@ -59,7 +59,7 @@ pub use utils::IsDust; use coin_selection::DefaultCoinSelectionAlgorithm; use signer::{SignOptions, SignerOrdering, SignersContainer, TransactionSigner}; -use tx_builder::{BumpFee, CreateTx, FeePolicy, TxBuilder, TxParams}; +use tx_builder::{FeePolicy, TxBuilder, TxParams}; use utils::{check_nsequence_rbf, After, Older, SecpCtx}; use crate::descriptor::policy::BuildSatisfaction; @@ -1293,12 +1293,11 @@ impl Wallet { /// ``` /// /// [`TxBuilder`]: crate::TxBuilder - pub fn build_tx(&mut self) -> TxBuilder<'_, DefaultCoinSelectionAlgorithm, CreateTx> { + pub fn build_tx(&mut self) -> TxBuilder<'_, DefaultCoinSelectionAlgorithm> { TxBuilder { wallet: alloc::rc::Rc::new(core::cell::RefCell::new(self)), params: TxParams::default(), coin_selection: DefaultCoinSelectionAlgorithm::default(), - phantom: core::marker::PhantomData, } } @@ -1684,7 +1683,7 @@ impl Wallet { pub fn build_fee_bump( &mut self, txid: Txid, - ) -> Result, BuildFeeBumpError> { + ) -> Result, BuildFeeBumpError> { let graph = self.indexed_graph.graph(); let txout_index = &self.indexed_graph.index; let chain_tip = self.chain.tip().block_id(); @@ -1808,7 +1807,6 @@ impl Wallet { wallet: alloc::rc::Rc::new(core::cell::RefCell::new(self)), params, coin_selection: DefaultCoinSelectionAlgorithm::default(), - phantom: core::marker::PhantomData, }) } diff --git a/crates/wallet/src/wallet/tx_builder.rs b/crates/wallet/src/wallet/tx_builder.rs index 28b70a8b..7b952201 100644 --- a/crates/wallet/src/wallet/tx_builder.rs +++ b/crates/wallet/src/wallet/tx_builder.rs @@ -19,7 +19,6 @@ //! # use bdk_wallet::*; //! # use bdk_wallet::wallet::ChangeSet; //! # use bdk_wallet::wallet::error::CreateTxError; -//! # use bdk_wallet::wallet::tx_builder::CreateTx; //! # use bdk_persist::PersistBackend; //! # use anyhow::Error; //! # let to_address = Address::from_str("2N4eQYCbKUHCCTUjBJeHcJp9ok6J2GZsTDt").unwrap().assume_checked(); @@ -43,31 +42,16 @@ use alloc::{boxed::Box, rc::Rc, string::String, vec::Vec}; use core::cell::RefCell; use core::fmt; -use core::marker::PhantomData; use bitcoin::psbt::{self, Psbt}; use bitcoin::script::PushBytes; use bitcoin::{absolute, Amount, FeeRate, OutPoint, ScriptBuf, Sequence, Transaction, Txid}; -use super::coin_selection::{CoinSelectionAlgorithm, DefaultCoinSelectionAlgorithm}; +use super::coin_selection::CoinSelectionAlgorithm; use super::{CreateTxError, Wallet}; use crate::collections::{BTreeMap, HashSet}; use crate::{KeychainKind, LocalOutput, Utxo, WeightedUtxo}; -/// Context in which the [`TxBuilder`] is valid -pub trait TxBuilderContext: core::fmt::Debug + Default + Clone {} - -/// Marker type to indicate the [`TxBuilder`] is being used to create a new transaction (as opposed -/// to bumping the fee of an existing one). -#[derive(Debug, Default, Clone)] -pub struct CreateTx; -impl TxBuilderContext for CreateTx {} - -/// Marker type to indicate the [`TxBuilder`] is being used to bump the fee of an existing transaction. -#[derive(Debug, Default, Clone)] -pub struct BumpFee; -impl TxBuilderContext for BumpFee {} - /// A transaction builder /// /// A `TxBuilder` is created by calling [`build_tx`] or [`build_fee_bump`] on a wallet. After @@ -123,11 +107,10 @@ impl TxBuilderContext for BumpFee {} /// [`finish`]: Self::finish /// [`coin_selection`]: Self::coin_selection #[derive(Debug)] -pub struct TxBuilder<'a, Cs, Ctx> { +pub struct TxBuilder<'a, Cs> { pub(crate) wallet: Rc>, pub(crate) params: TxParams, pub(crate) coin_selection: Cs, - pub(crate) phantom: PhantomData, } /// The parameters for transaction creation sans coin selection algorithm. @@ -175,19 +158,18 @@ impl Default for FeePolicy { } } -impl<'a, Cs: Clone, Ctx> Clone for TxBuilder<'a, Cs, Ctx> { +impl<'a, Cs: Clone> Clone for TxBuilder<'a, Cs> { fn clone(&self) -> Self { TxBuilder { wallet: self.wallet.clone(), params: self.params.clone(), coin_selection: self.coin_selection.clone(), - phantom: PhantomData, } } } -// methods supported by both contexts, for any CoinSelectionAlgorithm -impl<'a, Cs, Ctx> TxBuilder<'a, Cs, Ctx> { +// Methods supported for any CoinSelectionAlgorithm. +impl<'a, Cs> TxBuilder<'a, Cs> { /// Set a custom fee rate. /// /// This method sets the mining fee paid by the transaction as a rate on its size. @@ -212,8 +194,8 @@ impl<'a, Cs, Ctx> TxBuilder<'a, Cs, Ctx> { /// Note that this is really a minimum absolute fee -- it's possible to /// overshoot it slightly since adding a change output to drain the remaining /// excess might not be viable. - pub fn fee_absolute(&mut self, fee_amount: u64) -> &mut Self { - self.params.fee_policy = Some(FeePolicy::FeeAmount(fee_amount)); + pub fn fee_absolute(&mut self, fee_amount: Amount) -> &mut Self { + self.params.fee_policy = Some(FeePolicy::FeeAmount(fee_amount.to_sat())); self } @@ -553,18 +535,14 @@ impl<'a, Cs, Ctx> TxBuilder<'a, Cs, Ctx> { /// Choose the coin selection algorithm /// - /// Overrides the [`DefaultCoinSelectionAlgorithm`]. + /// Overrides the [`CoinSelectionAlgorithm`]. /// /// Note that this function consumes the builder and returns it so it is usually best to put this as the first call on the builder. - pub fn coin_selection( - self, - coin_selection: P, - ) -> TxBuilder<'a, P, Ctx> { + pub fn coin_selection(self, coin_selection: P) -> TxBuilder<'a, P> { TxBuilder { wallet: self.wallet, params: self.params, coin_selection, - phantom: PhantomData, } } @@ -612,9 +590,84 @@ impl<'a, Cs, Ctx> TxBuilder<'a, Cs, Ctx> { self.params.allow_dust = allow_dust; self } + + /// Replace the recipients already added with a new list + pub fn set_recipients(&mut self, recipients: Vec<(ScriptBuf, Amount)>) -> &mut Self { + self.params.recipients = recipients + .into_iter() + .map(|(script, amount)| (script, amount.to_sat())) + .collect(); + self + } + + /// Add a recipient to the internal list + pub fn add_recipient(&mut self, script_pubkey: ScriptBuf, amount: Amount) -> &mut Self { + self.params + .recipients + .push((script_pubkey, amount.to_sat())); + self + } + + /// Add data as an output, using OP_RETURN + pub fn add_data>(&mut self, data: &T) -> &mut Self { + let script = ScriptBuf::new_op_return(data); + self.add_recipient(script, Amount::ZERO); + self + } + + /// Sets the address to *drain* excess coins to. + /// + /// Usually, when there are excess coins they are sent to a change address generated by the + /// wallet. This option replaces the usual change address with an arbitrary `script_pubkey` of + /// your choosing. Just as with a change output, if the drain output is not needed (the excess + /// coins are too small) it will not be included in the resulting transaction. The only + /// difference is that it is valid to use `drain_to` without setting any ordinary recipients + /// with [`add_recipient`] (but it is perfectly fine to add recipients as well). + /// + /// If you choose not to set any recipients, you should provide the utxos that the + /// transaction should spend via [`add_utxos`]. + /// + /// # Example + /// + /// `drain_to` is very useful for draining all the coins in a wallet with [`drain_wallet`] to a + /// single address. + /// + /// ``` + /// # use std::str::FromStr; + /// # use bitcoin::*; + /// # use bdk_wallet::*; + /// # use bdk_wallet::wallet::ChangeSet; + /// # use bdk_wallet::wallet::error::CreateTxError; + /// # use bdk_persist::PersistBackend; + /// # use anyhow::Error; + /// # let to_address = + /// Address::from_str("2N4eQYCbKUHCCTUjBJeHcJp9ok6J2GZsTDt") + /// .unwrap() + /// .assume_checked(); + /// # let mut wallet = doctest_wallet!(); + /// let mut tx_builder = wallet.build_tx(); + /// + /// tx_builder + /// // Spend all outputs in this wallet. + /// .drain_wallet() + /// // Send the excess (which is all the coins minus the fee) to this address. + /// .drain_to(to_address.script_pubkey()) + /// .fee_rate(FeeRate::from_sat_per_vb(5).expect("valid feerate")) + /// .enable_rbf(); + /// let psbt = tx_builder.finish()?; + /// # Ok::<(), anyhow::Error>(()) + /// ``` + /// + /// [`add_recipient`]: Self::add_recipient + /// [`add_utxos`]: Self::add_utxos + /// [`drain_wallet`]: Self::drain_wallet + pub fn drain_to(&mut self, script_pubkey: ScriptBuf) -> &mut Self { + self.params.drain_to = Some(script_pubkey); + self + } } -impl<'a, Cs: CoinSelectionAlgorithm, Ctx> TxBuilder<'a, Cs, Ctx> { +impl<'a, Cs: CoinSelectionAlgorithm> TxBuilder<'a, Cs> { /// Finish building the transaction. /// /// Returns a new [`Psbt`] per [`BIP174`]. @@ -689,142 +742,6 @@ impl fmt::Display for AddForeignUtxoError { #[cfg(feature = "std")] impl std::error::Error for AddForeignUtxoError {} -#[derive(Debug)] -/// Error returned from [`TxBuilder::allow_shrinking`] -pub enum AllowShrinkingError { - /// Script/PubKey was not in the original transaction - MissingScriptPubKey(ScriptBuf), -} - -impl fmt::Display for AllowShrinkingError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::MissingScriptPubKey(script_buf) => write!( - f, - "Script/PubKey was not in the original transaction: {}", - script_buf, - ), - } - } -} - -#[cfg(feature = "std")] -impl std::error::Error for AllowShrinkingError {} - -impl<'a, Cs: CoinSelectionAlgorithm> TxBuilder<'a, Cs, CreateTx> { - /// Replace the recipients already added with a new list - pub fn set_recipients(&mut self, recipients: Vec<(ScriptBuf, Amount)>) -> &mut Self { - self.params.recipients = recipients - .into_iter() - .map(|(script, amount)| (script, amount.to_sat())) - .collect(); - self - } - - /// Add a recipient to the internal list - pub fn add_recipient(&mut self, script_pubkey: ScriptBuf, amount: Amount) -> &mut Self { - self.params - .recipients - .push((script_pubkey, amount.to_sat())); - self - } - - /// Add data as an output, using OP_RETURN - pub fn add_data>(&mut self, data: &T) -> &mut Self { - let script = ScriptBuf::new_op_return(data); - self.add_recipient(script, Amount::ZERO); - self - } - - /// Sets the address to *drain* excess coins to. - /// - /// Usually, when there are excess coins they are sent to a change address generated by the - /// wallet. This option replaces the usual change address with an arbitrary `script_pubkey` of - /// your choosing. Just as with a change output, if the drain output is not needed (the excess - /// coins are too small) it will not be included in the resulting transaction. The only - /// difference is that it is valid to use `drain_to` without setting any ordinary recipients - /// with [`add_recipient`] (but it is perfectly fine to add recipients as well). - /// - /// If you choose not to set any recipients, you should either provide the utxos that the - /// transaction should spend via [`add_utxos`], or set [`drain_wallet`] to spend all of them. - /// - /// When bumping the fees of a transaction made with this option, you probably want to - /// use [`allow_shrinking`] to allow this output to be reduced to pay for the extra fees. - /// - /// # Example - /// - /// `drain_to` is very useful for draining all the coins in a wallet with [`drain_wallet`] to a - /// single address. - /// - /// ``` - /// # use std::str::FromStr; - /// # use bitcoin::*; - /// # use bdk_wallet::*; - /// # use bdk_wallet::wallet::ChangeSet; - /// # use bdk_wallet::wallet::error::CreateTxError; - /// # use bdk_wallet::wallet::tx_builder::CreateTx; - /// # use bdk_persist::PersistBackend; - /// # use anyhow::Error; - /// # let to_address = - /// Address::from_str("2N4eQYCbKUHCCTUjBJeHcJp9ok6J2GZsTDt") - /// .unwrap() - /// .assume_checked(); - /// # let mut wallet = doctest_wallet!(); - /// let mut tx_builder = wallet.build_tx(); - /// - /// tx_builder - /// // Spend all outputs in this wallet. - /// .drain_wallet() - /// // Send the excess (which is all the coins minus the fee) to this address. - /// .drain_to(to_address.script_pubkey()) - /// .fee_rate(FeeRate::from_sat_per_vb(5).expect("valid feerate")) - /// .enable_rbf(); - /// let psbt = tx_builder.finish()?; - /// # Ok::<(), anyhow::Error>(()) - /// ``` - /// - /// [`allow_shrinking`]: Self::allow_shrinking - /// [`add_recipient`]: Self::add_recipient - /// [`add_utxos`]: Self::add_utxos - /// [`drain_wallet`]: Self::drain_wallet - pub fn drain_to(&mut self, script_pubkey: ScriptBuf) -> &mut Self { - self.params.drain_to = Some(script_pubkey); - self - } -} - -// methods supported only by bump_fee -impl<'a> TxBuilder<'a, DefaultCoinSelectionAlgorithm, BumpFee> { - /// Explicitly tells the wallet that it is allowed to reduce the amount of the output matching this - /// `script_pubkey` in order to bump the transaction fee. Without specifying this the wallet - /// will attempt to find a change output to shrink instead. - /// - /// **Note** that the output may shrink to below the dust limit and therefore be removed. If it is - /// preserved then it is currently not guaranteed to be in the same position as it was - /// originally. - /// - /// Returns an `Err` if `script_pubkey` can't be found among the recipients of the - /// transaction we are bumping. - pub fn allow_shrinking( - &mut self, - script_pubkey: ScriptBuf, - ) -> Result<&mut Self, AllowShrinkingError> { - match self - .params - .recipients - .iter() - .position(|(recipient_script, _)| *recipient_script == script_pubkey) - { - Some(position) => { - self.params.recipients.remove(position); - self.params.drain_to = Some(script_pubkey); - Ok(self) - } - None => Err(AllowShrinkingError::MissingScriptPubKey(script_pubkey)), - } - } -} - /// Ordering of the transaction's inputs and outputs #[derive(Default, Debug, Ord, PartialOrd, Eq, PartialEq, Hash, Clone, Copy)] pub enum TxOrdering { diff --git a/crates/wallet/tests/wallet.rs b/crates/wallet/tests/wallet.rs index 4269dd62..9b27a279 100644 --- a/crates/wallet/tests/wallet.rs +++ b/crates/wallet/tests/wallet.rs @@ -779,7 +779,7 @@ fn test_create_tx_absolute_fee() { builder .drain_to(addr.script_pubkey()) .drain_wallet() - .fee_absolute(100); + .fee_absolute(Amount::from_sat(100)); let psbt = builder.finish().unwrap(); let fee = check_fee!(wallet, psbt); @@ -799,7 +799,7 @@ fn test_create_tx_absolute_zero_fee() { builder .drain_to(addr.script_pubkey()) .drain_wallet() - .fee_absolute(0); + .fee_absolute(Amount::ZERO); let psbt = builder.finish().unwrap(); let fee = check_fee!(wallet, psbt); @@ -820,7 +820,7 @@ fn test_create_tx_absolute_high_fee() { builder .drain_to(addr.script_pubkey()) .drain_wallet() - .fee_absolute(60_000); + .fee_absolute(Amount::from_sat(60_000)); let _ = builder.finish().unwrap(); } @@ -1658,7 +1658,7 @@ fn test_bump_fee_low_abs() { .unwrap(); let mut builder = wallet.build_fee_bump(txid).unwrap(); - builder.fee_absolute(10); + builder.fee_absolute(Amount::from_sat(10)); builder.finish().unwrap(); } @@ -1680,7 +1680,7 @@ fn test_bump_fee_zero_abs() { .unwrap(); let mut builder = wallet.build_fee_bump(txid).unwrap(); - builder.fee_absolute(0); + builder.fee_absolute(Amount::ZERO); builder.finish().unwrap(); } @@ -1742,7 +1742,7 @@ fn test_bump_fee_reduce_change() { assert_fee_rate!(psbt, fee.unwrap_or(0), feerate, @add_signature); let mut builder = wallet.build_fee_bump(txid).unwrap(); - builder.fee_absolute(200); + builder.fee_absolute(Amount::from_sat(200)); builder.enable_rbf(); let psbt = builder.finish().unwrap(); let sent_received = @@ -1807,8 +1807,12 @@ fn test_bump_fee_reduce_single_recipient() { let mut builder = wallet.build_fee_bump(txid).unwrap(); builder .fee_rate(feerate) - .allow_shrinking(addr.script_pubkey()) - .unwrap(); + // remove original tx drain_to address and amount + .set_recipients(Vec::new()) + // set back original drain_to address + .drain_to(addr.script_pubkey()) + // drain wallet output amount will be re-calculated with new fee rate + .drain_wallet(); let psbt = builder.finish().unwrap(); let sent_received = wallet.sent_and_received(&psbt.clone().extract_tx().expect("failed to extract tx")); @@ -1849,9 +1853,13 @@ fn test_bump_fee_absolute_reduce_single_recipient() { let mut builder = wallet.build_fee_bump(txid).unwrap(); builder - .allow_shrinking(addr.script_pubkey()) - .unwrap() - .fee_absolute(300); + .fee_absolute(Amount::from_sat(300)) + // remove original tx drain_to address and amount + .set_recipients(Vec::new()) + // set back original drain_to address + .drain_to(addr.script_pubkey()) + // drain wallet output amount will be re-calculated with new fee rate + .drain_wallet(); let psbt = builder.finish().unwrap(); let tx = &psbt.unsigned_tx; let sent_received = wallet.sent_and_received(tx); @@ -1923,8 +1931,6 @@ fn test_bump_fee_drain_wallet() { let mut builder = wallet.build_fee_bump(txid).unwrap(); builder .drain_wallet() - .allow_shrinking(addr.script_pubkey()) - .unwrap() .fee_rate(FeeRate::from_sat_per_vb_unchecked(5)); let psbt = builder.finish().unwrap(); let sent_received = wallet.sent_and_received(&psbt.extract_tx().expect("failed to extract tx")); @@ -1940,7 +1946,7 @@ fn test_bump_fee_remove_output_manually_selected_only() { // them, and make sure that `bump_fee` doesn't try to add more. This fails because we've // told the wallet it's not allowed to add more inputs AND it can't reduce the value of the // existing output. In other words, bump_fee + manually_selected_only is always an error - // unless you've also set "allow_shrinking" OR there is a change output. + // unless there is a change output. let init_tx = Transaction { version: transaction::Version::ONE, lock_time: absolute::LockTime::ZERO, @@ -2092,7 +2098,7 @@ fn test_bump_fee_absolute_add_input() { .unwrap(); let mut builder = wallet.build_fee_bump(txid).unwrap(); - builder.fee_absolute(6_000); + builder.fee_absolute(Amount::from_sat(6_000)); let psbt = builder.finish().unwrap(); let sent_received = wallet.sent_and_received(&psbt.clone().extract_tx().expect("failed to extract tx")); @@ -2157,8 +2163,8 @@ fn test_bump_fee_no_change_add_input_and_change() { .insert_tx(tx, ConfirmationTime::Unconfirmed { last_seen: 0 }) .unwrap(); - // now bump the fees without using `allow_shrinking`. the wallet should add an - // extra input and a change output, and leave the original output untouched + // Now bump the fees, the wallet should add an extra input and a change output, and leave + // the original output untouched. let mut builder = wallet.build_fee_bump(txid).unwrap(); builder.fee_rate(FeeRate::from_sat_per_vb_unchecked(50)); let psbt = builder.finish().unwrap(); @@ -2369,7 +2375,10 @@ fn test_bump_fee_absolute_force_add_input() { // the new fee_rate is low enough that just reducing the change would be fine, but we force // the addition of an extra input with `add_utxo()` let mut builder = wallet.build_fee_bump(txid).unwrap(); - builder.add_utxo(incoming_op).unwrap().fee_absolute(250); + builder + .add_utxo(incoming_op) + .unwrap() + .fee_absolute(Amount::from_sat(250)); let psbt = builder.finish().unwrap(); let sent_received = wallet.sent_and_received(&psbt.clone().extract_tx().expect("failed to extract tx")); @@ -2478,8 +2487,12 @@ fn test_bump_fee_unconfirmed_input() { let mut builder = wallet.build_fee_bump(txid).unwrap(); builder .fee_rate(FeeRate::from_sat_per_vb_unchecked(15)) - .allow_shrinking(addr.script_pubkey()) - .unwrap(); + // remove original tx drain_to address and amount + .set_recipients(Vec::new()) + // set back original drain_to address + .drain_to(addr.script_pubkey()) + // drain wallet output amount will be re-calculated with new fee rate + .drain_wallet(); builder.finish().unwrap(); }