feat: Improve error handling

.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

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

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(())

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> = (T, Option<InputStatus>);
@@ -19,7 +21,7 @@ pub struct CanonicalUnspents {
 }
 
 impl CanonicalUnspents {
-    /// Construct.
+    /// Construct [`CanonicalUnspents`] from an iterator of txs with confirmation status.
     pub fn new<T>(canonical_txs: impl IntoIterator<Item = TxWithStatus<T>>) -> Self
     where
         T: Into<Arc<Transaction>>,
@@ -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<Item = Txid>,
-    ) -> Option<RbfSet> {
+    ) -> Result<RbfSet, ExtractReplacementsError> {
         let mut rbf_txs = replace
             .into_iter()
-            .map(|txid| self.txs.get(&txid).cloned().map(|tx| (txid, tx)))
-            .collect::<Option<HashMap<Txid, _>>>()?;
+            .map(|txid| -> Result<(Txid, Arc<Transaction>), _> {
+                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::<Result<HashMap<_, _>, _>>()?;
 
         // Remove txs in this set which have ancestors of other members of this set.
         let mut to_remove_from_rbf_txs = Vec::<Txid>::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::<Option<HashMap<_, _>>>()?;
+            .collect::<Result<HashMap<_, _>, _>>()?;
 
-        // 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<Input> {
-        if self.spends.contains_key(&outpoint) {
-            return None;
+        is_coinbase: bool,
+    ) -> Result<Input, GetForeignUnspentError> {
+        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<Item = Result<Input, GetForeignUnspentError>> + 'a
+    where
+        O: IntoIterator<Item = (OutPoint, Sequence, psbt::Input, usize, bool)>,
+        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 {}

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);
             }
         }