From f3dcb60cbb96e79c0601128e0c1adf6651d27b4a 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 | 241 ++++++++----------------- crates/wallet/tests/wallet.rs | 53 ++++-- 3 files changed, 115 insertions(+), 187 deletions(-) diff --git a/crates/wallet/src/wallet/mod.rs b/crates/wallet/src/wallet/mod.rs index 61ec5893b..fdacf5232 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; @@ -1357,12 +1357,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, } } @@ -1748,7 +1747,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(); @@ -1872,7 +1871,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 28b70a8bc..024c37278 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 } @@ -556,15 +538,11 @@ impl<'a, Cs, Ctx> TxBuilder<'a, Cs, Ctx> { /// Overrides the [`DefaultCoinSelectionAlgorithm`]. /// /// 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,106 +590,7 @@ impl<'a, Cs, Ctx> TxBuilder<'a, Cs, Ctx> { self.params.allow_dust = allow_dust; self } -} - -impl<'a, Cs: CoinSelectionAlgorithm, Ctx> TxBuilder<'a, Cs, Ctx> { - /// Finish building the transaction. - /// - /// Returns a new [`Psbt`] per [`BIP174`]. - /// - /// [`BIP174`]: https://github.com/bitcoin/bips/blob/master/bip-0174.mediawiki - pub fn finish(self) -> Result { - self.wallet - .borrow_mut() - .create_tx(self.coin_selection, self.params) - } -} - -#[derive(Debug)] -/// Error returned from [`TxBuilder::add_utxo`] and [`TxBuilder::add_utxos`] -pub enum AddUtxoError { - /// Happens when trying to spend an UTXO that is not in the internal database - UnknownUtxo(OutPoint), -} - -impl fmt::Display for AddUtxoError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::UnknownUtxo(outpoint) => write!( - f, - "UTXO not found in the internal database for txid: {} with vout: {}", - outpoint.txid, outpoint.vout - ), - } - } -} - -#[cfg(feature = "std")] -impl std::error::Error for AddUtxoError {} - -#[derive(Debug)] -/// Error returned from [`TxBuilder::add_foreign_utxo`]. -pub enum AddForeignUtxoError { - /// Foreign utxo outpoint txid does not match PSBT input txid - InvalidTxid { - /// PSBT input txid - input_txid: Txid, - /// Foreign UTXO outpoint - foreign_utxo: OutPoint, - }, - /// Requested outpoint doesn't exist in the tx (vout greater than available outputs) - InvalidOutpoint(OutPoint), - /// Foreign utxo missing witness_utxo or non_witness_utxo - MissingUtxo, -} - -impl fmt::Display for AddForeignUtxoError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::InvalidTxid { - input_txid, - foreign_utxo, - } => write!( - f, - "Foreign UTXO outpoint txid: {} does not match PSBT input txid: {}", - foreign_utxo.txid, input_txid, - ), - Self::InvalidOutpoint(outpoint) => write!( - f, - "Requested outpoint doesn't exist for txid: {} with vout: {}", - outpoint.txid, outpoint.vout, - ), - Self::MissingUtxo => write!(f, "Foreign utxo missing witness_utxo or non_witness_utxo"), - } - } -} - -#[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 @@ -745,11 +624,8 @@ impl<'a, Cs: CoinSelectionAlgorithm> TxBuilder<'a, Cs, CreateTx> { /// 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. + /// If you choose not to set any recipients, you should provide the utxos that the + /// transaction should spend via [`add_utxos`]. /// /// # Example /// @@ -762,7 +638,6 @@ impl<'a, Cs: CoinSelectionAlgorithm> TxBuilder<'a, Cs, CreateTx> { /// # 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 = @@ -783,7 +658,6 @@ impl<'a, Cs: CoinSelectionAlgorithm> TxBuilder<'a, Cs, CreateTx> { /// # Ok::<(), anyhow::Error>(()) /// ``` /// - /// [`allow_shrinking`]: Self::allow_shrinking /// [`add_recipient`]: Self::add_recipient /// [`add_utxos`]: Self::add_utxos /// [`drain_wallet`]: Self::drain_wallet @@ -793,38 +667,81 @@ impl<'a, Cs: CoinSelectionAlgorithm> TxBuilder<'a, Cs, CreateTx> { } } -// 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. +impl<'a, Cs: CoinSelectionAlgorithm> TxBuilder<'a, Cs> { + /// Finish building the transaction. /// - /// **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 a new [`Psbt`] per [`BIP174`]. /// - /// 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)), + /// [`BIP174`]: https://github.com/bitcoin/bips/blob/master/bip-0174.mediawiki + pub fn finish(self) -> Result { + self.wallet + .borrow_mut() + .create_tx(self.coin_selection, self.params) + } +} + +#[derive(Debug)] +/// Error returned from [`TxBuilder::add_utxo`] and [`TxBuilder::add_utxos`] +pub enum AddUtxoError { + /// Happens when trying to spend an UTXO that is not in the internal database + UnknownUtxo(OutPoint), +} + +impl fmt::Display for AddUtxoError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::UnknownUtxo(outpoint) => write!( + f, + "UTXO not found in the internal database for txid: {} with vout: {}", + outpoint.txid, outpoint.vout + ), + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for AddUtxoError {} + +#[derive(Debug)] +/// Error returned from [`TxBuilder::add_foreign_utxo`]. +pub enum AddForeignUtxoError { + /// Foreign utxo outpoint txid does not match PSBT input txid + InvalidTxid { + /// PSBT input txid + input_txid: Txid, + /// Foreign UTXO outpoint + foreign_utxo: OutPoint, + }, + /// Requested outpoint doesn't exist in the tx (vout greater than available outputs) + InvalidOutpoint(OutPoint), + /// Foreign utxo missing witness_utxo or non_witness_utxo + MissingUtxo, +} + +impl fmt::Display for AddForeignUtxoError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::InvalidTxid { + input_txid, + foreign_utxo, + } => write!( + f, + "Foreign UTXO outpoint txid: {} does not match PSBT input txid: {}", + foreign_utxo.txid, input_txid, + ), + Self::InvalidOutpoint(outpoint) => write!( + f, + "Requested outpoint doesn't exist for txid: {} with vout: {}", + outpoint.txid, outpoint.vout, + ), + Self::MissingUtxo => write!(f, "Foreign utxo missing witness_utxo or non_witness_utxo"), } } } +#[cfg(feature = "std")] +impl std::error::Error for AddForeignUtxoError {} + /// 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 58b25061d..f72c90417 100644 --- a/crates/wallet/tests/wallet.rs +++ b/crates/wallet/tests/wallet.rs @@ -744,7 +744,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); @@ -764,7 +764,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); @@ -785,7 +785,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(); } @@ -1623,7 +1623,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(); } @@ -1645,7 +1645,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(); } @@ -1707,7 +1707,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 = @@ -1772,8 +1772,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")); @@ -1814,9 +1818,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); @@ -1888,8 +1896,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")); @@ -1905,7 +1911,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, @@ -2057,7 +2063,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")); @@ -2122,8 +2128,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(); @@ -2334,7 +2340,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")); @@ -2443,8 +2452,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(); }