From cdd486a8d6fb6c7ea3ee0c141089787e4e973a9f Mon Sep 17 00:00:00 2001 From: valued mammal Date: Fri, 25 Apr 2025 09:48:55 -0400 Subject: [PATCH 1/4] ci: fix typo in rust.yml --- .github/workflows/rust.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 4df18dc..061719a 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -21,7 +21,7 @@ jobs: - name: Install Rust uses: dtolnay/rust-toolchain@v1 with: - toolchain: ${{ matrix.rust.toolchain }} + toolchain: ${{ matrix.rust.version }} - name: Pin dependencies for MSRV if: matrix.rust.version == '1.63.0' run: ./ci/pin-msrv.sh From c6da294a13292ef9b2fe454a8890e4bc8ca626ab Mon Sep 17 00:00:00 2001 From: valued mammal Date: Sat, 19 Apr 2025 07:44:33 -0400 Subject: [PATCH 2/4] feat!: Improve error handling Change functions that return Option to return a Result instead which makes for better error handling. Added - `FromPsbtInputError` - `CoinbaseMismatch` - `CannotMeetTarget` - `SelectorError` - `GetForeignUnspentError` - `ExtractReplacementsError` --- examples/common.rs | 6 +- examples/synopsis.rs | 18 ++-- src/canonical_unspents.rs | 167 +++++++++++++++++++++++++++++++------- src/input.rs | 158 ++++++++++++++++++++++++++++-------- src/input_candidates.rs | 69 ++++++++-------- src/output.rs | 8 +- src/rbf.rs | 9 +- src/selection.rs | 14 ++-- src/selector.rs | 75 +++++++++++++---- 9 files changed, 382 insertions(+), 142 deletions(-) diff --git a/examples/common.rs b/examples/common.rs index 8e7c035..eb9356a 100644 --- a/examples/common.rs +++ b/examples/common.rs @@ -1,5 +1,3 @@ -#![allow(unused)] - use std::sync::Arc; use bdk_bitcoind_rpc::Emitter; @@ -152,9 +150,7 @@ impl Wallet { let mut canon_utxos = CanonicalUnspents::new(self.canonical_txs()); // Exclude txs that reside-in `rbf_set`. - let rbf_set = canon_utxos - .extract_replacements(replace) - .ok_or(anyhow::anyhow!("cannot replace given txs"))?; + let rbf_set = canon_utxos.extract_replacements(replace)?; // TODO: We should really be returning an error if we fail to select an input of a tx we // are intending to replace. let must_select = rbf_set diff --git a/examples/synopsis.rs b/examples/synopsis.rs index 8f12d80..7a8ba86 100644 --- a/examples/synopsis.rs +++ b/examples/synopsis.rs @@ -28,14 +28,16 @@ fn main() -> anyhow::Result<()> { let addr = wallet.next_address().expect("must derive address"); - env.send(&addr, Amount::ONE_BTC)?; + let txid = env.send(&addr, Amount::ONE_BTC)?; env.mine_blocks(1, None)?; wallet.sync(&env)?; - println!("balance: {}", wallet.balance()); + println!("Received {}", txid); + println!("Balance (confirmed): {}", wallet.balance()); - env.send(&addr, Amount::ONE_BTC)?; + let txid = env.send(&addr, Amount::ONE_BTC)?; wallet.sync(&env)?; - println!("balance: {}", wallet.balance()); + println!("Received {txid}"); + println!("Balance (pending): {}", wallet.balance()); let (tip_height, tip_time) = wallet.tip_info(env.rpc_client())?; let longterm_feerate = FeeRate::from_sat_per_vb_unchecked(1); @@ -45,7 +47,7 @@ fn main() -> anyhow::Result<()> { .get_new_address(None, None)? .assume_checked(); - // okay now create tx. + // Okay now create tx. let selection = wallet .all_candidates() .regroup(group_by_spk()) @@ -88,7 +90,7 @@ fn main() -> anyhow::Result<()> { let txid = env.rpc_client().send_raw_transaction(&tx)?; println!("tx broadcasted: {}", txid); wallet.sync(&env)?; - println!("balance: {}", wallet.balance()); + println!("Balance (send tx): {}", wallet.balance()); // Try cancel a tx. // We follow all the rules as specified by @@ -140,7 +142,7 @@ fn main() -> anyhow::Result<()> { )?; let mut psbt = selection.create_psbt(PsbtParams { - // Not strictly necessary, but it may help us replace this replacement faster. + // Not strictly necessary, but it may help us replace the tx faster. fallback_sequence: Sequence::ENABLE_RBF_NO_LOCKTIME, ..Default::default() })?; @@ -172,7 +174,7 @@ fn main() -> anyhow::Result<()> { let txid = env.rpc_client().send_raw_transaction(&tx)?; println!("tx broadcasted: {}", txid); wallet.sync(&env)?; - println!("balance: {}", wallet.balance()); + println!("Balance (RBF): {}", wallet.balance()); } Ok(()) diff --git a/src/canonical_unspents.rs b/src/canonical_unspents.rs index 3bd3779..3c80b27 100644 --- a/src/canonical_unspents.rs +++ b/src/canonical_unspents.rs @@ -1,11 +1,13 @@ -use alloc::vec::Vec; - use alloc::sync::Arc; +use alloc::vec::Vec; +use core::fmt; use bitcoin::{psbt, OutPoint, Sequence, Transaction, TxOut, Txid}; use miniscript::{bitcoin, plan::Plan}; -use crate::{collections::HashMap, Input, InputStatus, RbfSet}; +use crate::{ + collections::HashMap, input::CoinbaseMismatch, FromPsbtInputError, Input, InputStatus, RbfSet, +}; /// Tx with confirmation status. pub type TxWithStatus = (T, Option); @@ -19,7 +21,7 @@ pub struct CanonicalUnspents { } impl CanonicalUnspents { - /// Construct. + /// Construct [`CanonicalUnspents`] from an iterator of txs with confirmation status. pub fn new(canonical_txs: impl IntoIterator>) -> Self where T: Into>, @@ -43,16 +45,28 @@ impl CanonicalUnspents { } } - /// TODO: This should return a descriptive error on why it failed. - /// TODO: Error if trying to replace coinbase. + /// Extract txs in the set of `replace` from the canonical view of unspents. + /// + /// Returns the [`RbfSet`] if the replacements are valid and succesfully extracted. + /// Errors if the replacements cannot be extracted (e.g. due to missing data). pub fn extract_replacements( &mut self, replace: impl IntoIterator, - ) -> Option { + ) -> Result { let mut rbf_txs = replace .into_iter() - .map(|txid| self.txs.get(&txid).cloned().map(|tx| (txid, tx))) - .collect::>>()?; + .map(|txid| -> Result<(Txid, Arc), _> { + let tx = self + .txs + .get(&txid) + .cloned() + .ok_or(ExtractReplacementsError::TransactionNotFound(txid))?; + if tx.is_coinbase() { + return Err(ExtractReplacementsError::CannotReplaceCoinbase); + } + Ok((tx.compute_txid(), tx)) + }) + .collect::, _>>()?; // Remove txs in this set which have ancestors of other members of this set. let mut to_remove_from_rbf_txs = Vec::::new(); @@ -79,25 +93,25 @@ impl CanonicalUnspents { } // Find prev outputs of all txs in the set. - // Fail when on prev output is not found. We need to use the prevouts to determine fee fr - // rbf! + // Fail when a prev output is not found. We need to use the prevouts to determine fee for RBF! let prev_txouts = rbf_txs .values() .flat_map(|tx| &tx.input) .map(|txin| txin.previous_output) - .map(|op| -> Option<(OutPoint, TxOut)> { + .map(|op| -> Result<(OutPoint, TxOut), _> { let txout = self .txs .get(&op.txid) .and_then(|tx| tx.output.get(op.vout as usize)) - .cloned()?; - Some((op, txout)) + .cloned() + .ok_or(ExtractReplacementsError::PreviousOutputNotFound(op))?; + Ok((op, txout)) }) - .collect::>>()?; + .collect::, _>>()?; - // Remove rbf txs (and their descendants) from canoncial unspents. - let to_remove_from_canoncial_unspents = rbf_txs.keys().chain(&to_remove_from_rbf_txs); - for txid in to_remove_from_canoncial_unspents { + // Remove rbf txs (and their descendants) from canonical unspents. + let to_remove_from_canonical_unspents = rbf_txs.keys().chain(&to_remove_from_rbf_txs); + for txid in to_remove_from_canonical_unspents { if let Some(tx) = self.txs.remove(txid) { self.statuses.remove(txid); for txin in &tx.input { @@ -106,7 +120,10 @@ impl CanonicalUnspents { } } - RbfSet::new(rbf_txs.into_values(), prev_txouts) + Ok( + RbfSet::new(rbf_txs.into_values(), prev_txouts) + .expect("must not have missing prevouts"), + ) } /// Whether outpoint is a leaf (unspent). @@ -149,23 +166,115 @@ impl CanonicalUnspents { .filter_map(|(op, plan)| self.try_get_unspent(op, plan)) } - /// Try get foreign leaf. - /// TODO: Check psbt_input data with our own prev tx data. - /// TODO: Create `try_get_foreign_leaves` method. + /// Try get foreign leaf (unspent). pub fn try_get_foreign_unspent( &self, outpoint: OutPoint, sequence: Sequence, psbt_input: psbt::Input, satisfaction_weight: usize, - ) -> Option { - if self.spends.contains_key(&outpoint) { - return None; + is_coinbase: bool, + ) -> Result { + if !self.is_unspent(outpoint) { + return Err(GetForeignUnspentError::OutputIsAlreadySpent(outpoint)); + } + if let Some(prev_tx) = self.txs.get(&outpoint.txid) { + let non_witness_utxo = psbt_input.non_witness_utxo.as_ref(); + if non_witness_utxo.is_some() && non_witness_utxo != Some(prev_tx) { + return Err(GetForeignUnspentError::UtxoMismatch(outpoint)); + } + let witness_utxo = psbt_input.witness_utxo.as_ref(); + if witness_utxo.is_some() + && psbt_input.witness_utxo.as_ref() != prev_tx.output.get(outpoint.vout as usize) + { + return Err(GetForeignUnspentError::UtxoMismatch(outpoint)); + } + if is_coinbase != prev_tx.is_coinbase() { + return Err(GetForeignUnspentError::Coinbase(CoinbaseMismatch { + txid: outpoint.txid, + expected: is_coinbase, + got: prev_tx.is_coinbase(), + })); + } } - let prev_tx = Arc::clone(self.txs.get(&outpoint.txid)?); - let output_index: usize = outpoint.vout.try_into().expect("vout must fit into usize"); - let _txout = prev_tx.output.get(output_index)?; let status = self.statuses.get(&outpoint.txid).cloned(); - Input::from_psbt_input(outpoint, sequence, psbt_input, satisfaction_weight, status) + Input::from_psbt_input( + outpoint, + sequence, + psbt_input, + satisfaction_weight, + status, + is_coinbase, + ) + .map_err(GetForeignUnspentError::FromPsbtInput) + } + + /// Try get foreign leaves (unspent). + pub fn try_get_foreign_unspents<'a, O>( + &'a self, + outpoints: O, + ) -> impl Iterator> + 'a + where + O: IntoIterator, + O::IntoIter: 'a, + { + outpoints + .into_iter() + .map(|(op, seq, input, sat_wu, is_coinbase)| { + self.try_get_foreign_unspent(op, seq, input, sat_wu, is_coinbase) + }) } } + +/// Canonical unspents error +#[derive(Debug)] +pub enum GetForeignUnspentError { + /// Invalid parameter for `is_coinbase` + Coinbase(CoinbaseMismatch), + /// Error creating an input from a PSBT input + FromPsbtInput(FromPsbtInputError), + /// Cannot get unspent input from output that is already spent + OutputIsAlreadySpent(OutPoint), + /// The witness or non-witness UTXO in the PSBT input does not match the expected outpoint + UtxoMismatch(OutPoint), +} + +impl fmt::Display for GetForeignUnspentError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Coinbase(err) => write!(f, "{}", err), + Self::FromPsbtInput(err) => write!(f, "{}", err), + Self::OutputIsAlreadySpent(op) => { + write!(f, "outpoint is already spent: {}", op) + } + Self::UtxoMismatch(op) => write!(f, "UTXO mismatch: {}", op), + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for GetForeignUnspentError {} + +/// Error when attempting to do [`extract_replacements`](CanonicalUnspents::extract_replacements). +#[derive(Debug)] +pub enum ExtractReplacementsError { + /// Transaction not found in canonical unspents + TransactionNotFound(Txid), + /// Cannot replace a coinbase transaction + CannotReplaceCoinbase, + /// Previous output not found for input + PreviousOutputNotFound(OutPoint), +} + +impl fmt::Display for ExtractReplacementsError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::TransactionNotFound(txid) => write!(f, "transaction not found: {}", txid), + Self::CannotReplaceCoinbase => write!(f, "cannot replace a coinbase transaction"), + Self::PreviousOutputNotFound(op) => write!(f, "previous output not found: {}", op), + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for ExtractReplacementsError {} diff --git a/src/input.rs b/src/input.rs index 50abd2d..15e6b2a 100644 --- a/src/input.rs +++ b/src/input.rs @@ -1,13 +1,15 @@ -use std::{sync::Arc, vec::Vec}; +use alloc::sync::Arc; +use alloc::vec::Vec; +use core::fmt; use bitcoin::constants::COINBASE_MATURITY; use bitcoin::transaction::OutputsIndexError; -use bitcoin::{absolute, psbt, relative, Amount, Sequence}; +use bitcoin::{absolute, psbt, relative, Amount, Sequence, Txid}; use miniscript::bitcoin; use miniscript::bitcoin::{OutPoint, Transaction, TxOut}; use miniscript::plan::Plan; -/// Confirmation status of a tx. +/// Confirmation status of a tx input. #[derive(Debug, Clone, Copy)] pub struct InputStatus { /// Confirmation block height. @@ -20,10 +22,11 @@ pub struct InputStatus { } impl InputStatus { - /// Helper method. + /// From consensus `height` and `time`. pub fn new(height: u32, time: u64) -> Result { Ok(Self { height: absolute::Height::from_consensus(height)?, + // TODO: handle `.try_into::()` time: absolute::Time::from_consensus(time as _)?, }) } @@ -33,29 +36,27 @@ impl InputStatus { enum PlanOrPsbtInput { Plan(Plan), PsbtInput { - psbt_input: bitcoin::psbt::Input, - sequence: bitcoin::Sequence, + psbt_input: psbt::Input, + sequence: Sequence, absolute_timelock: absolute::LockTime, satisfaction_weight: usize, }, } impl PlanOrPsbtInput { - /// Returns `None` if input index does not exist or input is not finalized. - /// - /// TODO: Check whether satisfaction_weight calculations are correct. - /// TODO: Return error type: invalid outpoint, no `witness_utxo` or `non_witness_utxo`, etc. - /// - /// # WHy do we only support finalized psbt inputs? + /// From [`psbt::Input`]. /// - /// There is no mulit-party tx building protocol that requires choosing from foreign, - /// non-finalized PSBT inputs. + /// Errors if neither the witness- or non-witness UTXO are present in `psbt_input`. fn from_psbt_input( sequence: Sequence, psbt_input: psbt::Input, satisfaction_weight: usize, - ) -> Option { - Some(Self::PsbtInput { + ) -> Result { + // We require at least one of the witness or non-witness utxo + if psbt_input.witness_utxo.is_none() && psbt_input.non_witness_utxo.is_none() { + return Err(FromPsbtInputError::UtxoCheck); + } + Ok(Self::PsbtInput { psbt_input, sequence, absolute_timelock: absolute::LockTime::ZERO, @@ -127,6 +128,61 @@ impl PlanOrPsbtInput { } } +/// Mismatch between the expected and actual value of [`Transaction::is_coinbase`]. +#[derive(Debug, Clone)] +pub struct CoinbaseMismatch { + /// txid + pub txid: Txid, + /// expected value of whether a tx is coinbase + pub expected: bool, + /// whether the actual tx is coinbase + pub got: bool, +} + +impl fmt::Display for CoinbaseMismatch { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "invalid coinbase parameter for txid {}; expected `is_coinbase`: {}, found: {}", + self.txid, self.expected, self.got + ) + } +} + +#[cfg(feature = "std")] +impl std::error::Error for CoinbaseMismatch {} + +/// Error creating [`Input`] from a PSBT input +#[derive(Debug, Clone)] +pub enum FromPsbtInputError { + /// Invalid `is_coinbase` parameter + Coinbase(CoinbaseMismatch), + /// Invalid outpoint + InvalidOutPoint(OutPoint), + /// The input's UTXO is missing or invalid + UtxoCheck, +} + +impl fmt::Display for FromPsbtInputError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Coinbase(err) => write!(f, "{}", err), + Self::InvalidOutPoint(op) => { + write!(f, "invalid outpoint: {}", op) + } + Self::UtxoCheck => { + write!( + f, + "one of the witness or non-witness utxo is missing or invalid" + ) + } + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for FromPsbtInputError {} + /// Single-input plan. #[derive(Debug, Clone)] pub struct Input { @@ -139,9 +195,12 @@ pub struct Input { } impl Input { - /// Create + /// Create [`Input`] from a previous transaction. + /// + /// # Errors /// - /// Returns `None` if `prev_output_index` does not exist in `prev_tx`. + /// Returns `OutputsIndexError` if the previous txout is not found in `prev_tx` + /// at `output_index`. pub fn from_prev_tx( plan: Plan, prev_tx: T, @@ -163,7 +222,7 @@ impl Input { }) } - /// Create + /// Create [`Input`] from a previous txout and plan. pub fn from_prev_txout( plan: Plan, prev_outpoint: OutPoint, @@ -181,33 +240,65 @@ impl Input { } } - /// Create + /// Create [`Input`] from a [`psbt::Input`]. /// - /// TODO: Return error type: out of bounds, etc. + /// # Errors + /// + /// - If neither the witness or non-witness utxo are present in `psbt_input`. + /// - If `prev_outpoint` doesn't agree with the previous transaction. + /// - If the previous transaction is known but doesn't match the provided `is_coinbase` + /// parameter. pub fn from_psbt_input( prev_outpoint: OutPoint, sequence: Sequence, psbt_input: psbt::Input, satisfaction_weight: usize, status: Option, - ) -> Option { - let prev_txout = psbt_input.witness_utxo.clone().or(psbt_input - .non_witness_utxo - .clone() - .and_then(|tx| { - tx.output - .get(prev_outpoint.vout.try_into().unwrap_or(usize::MAX)) + is_coinbase: bool, + ) -> Result { + let outpoint = prev_outpoint; + let prev_txout = match ( + psbt_input.non_witness_utxo.as_ref(), + psbt_input.witness_utxo.as_ref(), + ) { + (Some(prev_tx), witness_utxo) => { + // The outpoint must be valid + if prev_tx.compute_txid() != outpoint.txid { + return Err(FromPsbtInputError::InvalidOutPoint(outpoint)); + } + let prev_txout = prev_tx + .output + .get(outpoint.vout as usize) .cloned() - }))?; + .ok_or(FromPsbtInputError::InvalidOutPoint(outpoint))?; + // In case the witness-utxo is present, the txout must match + if let Some(txout) = witness_utxo { + if txout != &prev_txout { + return Err(FromPsbtInputError::UtxoCheck); + } + } + // The value of `is_coinbase` must match that of the previous tx + if is_coinbase != prev_tx.is_coinbase() { + return Err(FromPsbtInputError::Coinbase(CoinbaseMismatch { + txid: outpoint.txid, + expected: is_coinbase, + got: prev_tx.is_coinbase(), + })); + } + prev_txout + } + (_, Some(txout)) => txout.clone(), + _ => return Err(FromPsbtInputError::UtxoCheck), + }; let prev_tx = psbt_input.non_witness_utxo.clone().map(Arc::new); let plan = PlanOrPsbtInput::from_psbt_input(sequence, psbt_input, satisfaction_weight)?; - Some(Self { + Ok(Self { prev_outpoint, prev_txout, prev_tx, plan, status, - is_coinbase: false, + is_coinbase, }) } @@ -332,9 +423,10 @@ impl Input { self.plan.sequence() } - /// In weight units. + /// The weight in witness units needed for satisfying the [`Input`]. /// - /// TODO: Describe what fields are actually included in this calculation. + /// The satisfaction weight is the combined size of the fully satisfied input's witness + /// and scriptSig expressed in weight units. See . pub fn satisfaction_weight(&self) -> u64 { self.plan .satisfaction_weight() diff --git a/src/input_candidates.rs b/src/input_candidates.rs index 3d9b9b4..9e257b0 100644 --- a/src/input_candidates.rs +++ b/src/input_candidates.rs @@ -1,18 +1,16 @@ -use core::{ - fmt::{Debug, Display}, - ops::Deref, -}; - -use crate::{ - collections::BTreeMap, collections::HashSet, cs_feerate, Input, Selection, Selector, - SelectorParams, -}; use alloc::vec::Vec; +use core::fmt; +use core::ops::Deref; + use bdk_coin_select::{metrics::LowestFee, Candidate, NoBnbSolution}; use bitcoin::{absolute, FeeRate, OutPoint}; use miniscript::bitcoin; -use crate::InputGroup; +use crate::collections::{BTreeMap, HashSet}; +use crate::{ + cs_feerate, CannotMeetTarget, Input, InputGroup, Selection, Selector, SelectorError, + SelectorParams, +}; /// Input candidates. #[must_use] @@ -34,7 +32,9 @@ fn cs_candidate_from_group(group: &InputGroup) -> Candidate { } impl InputCandidates { - /// Construct + /// Construct [`InputCandidates`] with a list of inputs that must be selected as well as + /// those that may additionally be selected. If the same outpoint occurs in both `must_select` and + /// `can_select`, the one in `must_select` is retained. pub fn new(must_select: A, can_select: B) -> Self where A: IntoIterator, @@ -192,7 +192,8 @@ impl InputCandidates { self } - /// Into selection + /// Attempt to convert the input candidates into a valid [`Selection`] with a given + /// `algorithm` and selector `params`. pub fn into_selection( self, mut algorithm: A, @@ -201,12 +202,11 @@ impl InputCandidates { where A: FnMut(&mut Selector) -> Result<(), E>, { - let mut selector = - Selector::new(&self, params).map_err(IntoSelectionError::InvalidChangePolicy)?; + let mut selector = Selector::new(&self, params).map_err(IntoSelectionError::Selector)?; algorithm(&mut selector).map_err(IntoSelectionError::SelectionAlgorithm)?; let selection = selector .try_finalize() - .ok_or(IntoSelectionError::CannotMeetTarget)?; + .ok_or(IntoSelectionError::CannotMeetTarget(CannotMeetTarget))?; Ok(selection) } } @@ -214,32 +214,32 @@ impl InputCandidates { /// Occurs when we cannot find a solution for selection. #[derive(Debug)] pub enum IntoSelectionError { - /// Parameters provided created an invalid change policy. - InvalidChangePolicy(miniscript::Error), + /// Coin selector returned an error + Selector(SelectorError), /// Selection algorithm failed. SelectionAlgorithm(E), - /// Cannot meet target. - CannotMeetTarget, + /// The target cannot be met + CannotMeetTarget(CannotMeetTarget), } -impl Display for IntoSelectionError { - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { +impl fmt::Display for IntoSelectionError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - IntoSelectionError::InvalidChangePolicy(error) => { - write!(f, "invalid change policy: {}", error) + IntoSelectionError::Selector(error) => { + write!(f, "{}", error) } IntoSelectionError::SelectionAlgorithm(error) => { write!(f, "selection algorithm failed: {}", error) } - IntoSelectionError::CannotMeetTarget => write!(f, "cannot meet target"), + IntoSelectionError::CannotMeetTarget(error) => write!(f, "{}", error), } } } #[cfg(feature = "std")] -impl std::error::Error for IntoSelectionError {} +impl std::error::Error for IntoSelectionError {} -/// Occurs when we are missing ouputs. +/// Occurs when we are missing outputs. #[derive(Debug)] pub struct MissingOutputs(HashSet); @@ -251,8 +251,9 @@ impl Deref for MissingOutputs { } } -impl Display for MissingOutputs { - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { +impl fmt::Display for MissingOutputs { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + // TODO: should not use fmt::Debug on Display write!(f, "missing outputs: {:?}", self.0) } } @@ -269,19 +270,19 @@ pub enum PolicyFailure { PolicyFailure(PF), } -impl Display for PolicyFailure { - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { +impl fmt::Display for PolicyFailure { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - PolicyFailure::MissingOutputs(missing_outputs) => Debug::fmt(missing_outputs, f), - PolicyFailure::PolicyFailure(failure) => { - write!(f, "policy failure: {:?}", failure) + PolicyFailure::MissingOutputs(err) => write!(f, "{}", err), + PolicyFailure::PolicyFailure(err) => { + write!(f, "policy failure: {}", err) } } } } #[cfg(feature = "std")] -impl std::error::Error for PolicyFailure {} +impl std::error::Error for PolicyFailure {} /// Select for lowest fee with bnb pub fn selection_algorithm_lowest_fee_bnb( diff --git a/src/output.rs b/src/output.rs index adc991a..92e972a 100644 --- a/src/output.rs +++ b/src/output.rs @@ -3,12 +3,12 @@ use miniscript::bitcoin; use crate::DefiniteDescriptor; -/// Can get script pubkey from this. +/// Source of the output script pubkey #[derive(Debug, Clone)] pub enum ScriptSource { - /// From ScriptBuf. + /// bitcoin script Script(ScriptBuf), - /// From definite descriptor. + /// definite descriptor Descriptor(DefiniteDescriptor), } @@ -38,7 +38,7 @@ impl ScriptSource { /// To ScriptBuf pub fn script(&self) -> ScriptBuf { match self { - ScriptSource::Script(script_buf) => script_buf.clone(), + ScriptSource::Script(spk) => spk.clone(), ScriptSource::Descriptor(descriptor) => descriptor.script_pubkey(), } } diff --git a/src/rbf.rs b/src/rbf.rs index ae68498..edb2683 100644 --- a/src/rbf.rs +++ b/src/rbf.rs @@ -1,7 +1,6 @@ use alloc::sync::Arc; use core::fmt::Display; -use alloc::vec::Vec; use bitcoin::{absolute, Amount, OutPoint, Transaction, TxOut, Txid}; use miniscript::bitcoin; @@ -87,12 +86,7 @@ impl RbfSet { let prev_spends = self .txs .values() - .flat_map(|tx| { - tx.input - .iter() - .map(|txin| txin.previous_output) - .collect::>() - }) + .flat_map(|tx| tx.input.iter().map(|txin| txin.previous_output)) .collect::>(); move |input| { prev_spends.contains(&input.prev_outpoint()) || input.confirmations(tip_height) > 0 @@ -152,6 +146,7 @@ impl RbfSet { .value }) .sum(); + // TODO: is it safe to do unchecked subtraction? input_sum - output_sum } diff --git a/src/selection.rs b/src/selection.rs index 4196a9a..d0c8d1a 100644 --- a/src/selection.rs +++ b/src/selection.rs @@ -2,7 +2,7 @@ use core::fmt::{Debug, Display}; use std::vec::Vec; use bdk_coin_select::FeeRate; -use bitcoin::{absolute, transaction}; +use bitcoin::{absolute, transaction, Sequence}; use miniscript::bitcoin; use miniscript::psbt::PsbtExt; @@ -36,10 +36,13 @@ pub struct PsbtParams { /// It is best practive to set this to the latest block height to avoid fee sniping. pub fallback_locktime: absolute::LockTime, - /// Yes. - pub fallback_sequence: bitcoin::Sequence, + /// [`Sequence`] value to use by default if not provided by the input. + pub fallback_sequence: Sequence, - /// Recommended. + /// Whether to require the full tx, aka [`non_witness_utxo`] for segwit v0 inputs, + /// default is `true`. + /// + /// [`non_witness_utxo`]: bitcoin::psbt::Input::non_witness_utxo pub mandate_full_tx_for_segwit_v0: bool, } @@ -97,7 +100,7 @@ impl std::error::Error for CreatePsbtError {} impl Selection { /// Returns none if there is a mismatch of units in `locktimes`. /// - /// As according to BIP-64... + // TODO: As according to BIP-64... ? fn _accumulate_max_locktime( locktimes: impl IntoIterator, fallback: absolute::LockTime, @@ -138,7 +141,6 @@ impl Selection { .iter() .map(|input| bitcoin::TxIn { previous_output: input.prev_outpoint(), - // TODO: Custom fallback sequence? sequence: input.sequence().unwrap_or(params.fallback_sequence), ..Default::default() }) diff --git a/src/selector.rs b/src/selector.rs index 0e6b836..97092da 100644 --- a/src/selector.rs +++ b/src/selector.rs @@ -7,8 +7,9 @@ use miniscript::bitcoin; use crate::{cs_feerate, DefiniteDescriptor, InputCandidates, InputGroup, Output, Selection}; use alloc::vec::Vec; +use core::fmt; -/// You seeing this? +/// A coin selector #[derive(Debug, Clone)] pub struct Selector<'c> { candidates: &'c InputCandidates, @@ -82,7 +83,8 @@ pub struct RbfParams { pub incremental_relay_feerate: FeeRate, } -/// TODO: Make this more flexible. +/// Change policy type +// TODO: Make this more flexible. #[derive(Debug, Clone, Copy)] pub enum ChangePolicyType { /// Avoid creating dust change output. @@ -95,13 +97,9 @@ pub enum ChangePolicyType { } impl OriginalTxStats { - /// Feerate. - /// - /// TODO: Make sure this is correct with the rounding. + /// Return the [`FeeRate`] of the original tx. pub fn feerate(&self) -> FeeRate { - FeeRate::from_sat_per_vb_unchecked( - ((self.fee.to_sat() as f32) / (self.weight.to_vbytes_ceil() as f32)) as _, - ) + self.fee / self.weight } } @@ -120,11 +118,10 @@ impl RbfParams { /// To coin select `Replace` params. pub fn to_cs_replace(&self) -> Replace { - let replace = Replace { + Replace { fee: self.original_txs.iter().map(|otx| otx.fee.to_sat()).sum(), incremental_relay_feerate: cs_feerate(self.incremental_relay_feerate), - }; - replace + } } /// Max feerate of all the original txs. @@ -134,7 +131,7 @@ impl RbfParams { self.original_txs .iter() .map(|otx| otx.feerate()) - .min() + .max() .unwrap_or(FeeRate::ZERO) } } @@ -219,19 +216,63 @@ impl SelectorParams { } } +/// Error when the selection is impossible with the input candidates +#[derive(Debug)] +pub struct CannotMeetTarget; + +impl fmt::Display for CannotMeetTarget { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "meeting the target is not possible with the input candidates" + ) + } +} + +#[cfg(feature = "std")] +impl std::error::Error for CannotMeetTarget {} + +/// Selector error +#[derive(Debug)] +pub enum SelectorError { + /// miniscript error + Miniscript(miniscript::Error), + /// meeting the target is not possible + CannotMeetTarget(CannotMeetTarget), +} + +impl fmt::Display for SelectorError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Miniscript(err) => write!(f, "{}", err), + Self::CannotMeetTarget(err) => write!(f, "{}", err), + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for SelectorError {} + impl<'c> Selector<'c> { /// Create new input selector. /// - /// TODO: Have this return custom error with more check: - /// * Whether selection is even possible. + /// # Errors + /// + /// - If we are unable to create a change policy from the `params`. + /// - If the target is unreachable given the total input value. pub fn new( candidates: &'c InputCandidates, params: SelectorParams, - ) -> Result { + ) -> Result { let target = params.to_cs_target(); - let change_policy = params.to_cs_change_policy()?; + let change_policy = params + .to_cs_change_policy() + .map_err(SelectorError::Miniscript)?; let target_outputs = params.target_outputs; let change_descriptor = params.change_descriptor; + if target.value() > candidates.groups().map(|grp| grp.value().to_sat()).sum() { + return Err(SelectorError::CannotMeetTarget(CannotMeetTarget)); + } let mut inner = bdk_coin_select::CoinSelector::new(candidates.coin_select_candidates()); if candidates.must_select().is_some() { inner.select_next(); @@ -266,6 +307,8 @@ impl<'c> Selector<'c> { self.change_policy } + // TODO: Remove this and have `select_with_algorithm` where algorithm is a closure + // see bdk-tx#1 comment /// Do branch-and-bound selection with `LowestFee` metric. pub fn select_for_lowest_fee( &mut self, From f4ac1b1eb1e59b03de5c00a14ec88335175a74d4 Mon Sep 17 00:00:00 2001 From: valued mammal Date: Fri, 25 Apr 2025 11:30:12 -0400 Subject: [PATCH 3/4] feat: Add `Selector::select_with_algorithm` --- src/input_candidates.rs | 6 ++++-- src/selector.rs | 25 +++++++------------------ 2 files changed, 11 insertions(+), 20 deletions(-) diff --git a/src/input_candidates.rs b/src/input_candidates.rs index 9e257b0..6603f1e 100644 --- a/src/input_candidates.rs +++ b/src/input_candidates.rs @@ -196,14 +196,16 @@ impl InputCandidates { /// `algorithm` and selector `params`. pub fn into_selection( self, - mut algorithm: A, + algorithm: A, params: SelectorParams, ) -> Result> where A: FnMut(&mut Selector) -> Result<(), E>, { let mut selector = Selector::new(&self, params).map_err(IntoSelectionError::Selector)?; - algorithm(&mut selector).map_err(IntoSelectionError::SelectionAlgorithm)?; + selector + .select_with_algorithm(algorithm) + .map_err(IntoSelectionError::SelectionAlgorithm)?; let selection = selector .try_finalize() .ok_or(IntoSelectionError::CannotMeetTarget(CannotMeetTarget))?; diff --git a/src/selector.rs b/src/selector.rs index 97092da..3063e0f 100644 --- a/src/selector.rs +++ b/src/selector.rs @@ -1,6 +1,5 @@ use bdk_coin_select::{ - float::Ordf32, metrics::LowestFee, ChangePolicy, DrainWeights, InsufficientFunds, - NoBnbSolution, Replace, Target, TargetFee, TargetOutputs, + ChangePolicy, DrainWeights, InsufficientFunds, Replace, Target, TargetFee, TargetOutputs, }; use bitcoin::{Amount, FeeRate, Transaction, TxOut, Weight}; use miniscript::bitcoin; @@ -307,22 +306,12 @@ impl<'c> Selector<'c> { self.change_policy } - // TODO: Remove this and have `select_with_algorithm` where algorithm is a closure - // see bdk-tx#1 comment - /// Do branch-and-bound selection with `LowestFee` metric. - pub fn select_for_lowest_fee( - &mut self, - longterm_feerate: FeeRate, - max_bnb_rounds: usize, - ) -> Result { - self.inner.run_bnb( - LowestFee { - target: self.target, - long_term_feerate: cs_feerate(longterm_feerate), - change_policy: self.change_policy, - }, - max_bnb_rounds, - ) + /// Select with the provided `algorithm`. + pub fn select_with_algorithm(&mut self, mut algorithm: F) -> Result<(), E> + where + F: FnMut(&mut Selector) -> Result<(), E>, + { + algorithm(self) } /// Select all. From 5bc533da6e251300249d37656fda1e276ee857bf Mon Sep 17 00:00:00 2001 From: valued mammal Date: Sun, 27 Apr 2025 17:17:39 -0400 Subject: [PATCH 4/4] fix(finalizer): return true if already finalized in `finalize_input` Include the check for `final_script_sig` when evaluating whether the input is finalized. --- src/finalizer.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/finalizer.rs b/src/finalizer.rs index d88bda7..5ffbd61 100644 --- a/src/finalizer.rs +++ b/src/finalizer.rs @@ -35,9 +35,7 @@ impl Finalizer { // return true if already finalized. { let psbt_input = &psbt.inputs[input_index]; - if psbt_input.final_script_witness.is_some() - || psbt_input.final_script_witness.is_some() - { + if psbt_input.final_script_sig.is_some() || psbt_input.final_script_witness.is_some() { return Ok(true); } }