feat(tx_graph)!: change TxGraph::calculate_fee to return Result<u64,CalculateFeeError>

added
- tx_graph::CalculateFeeError enum

BREAKING CHANGES:

changed
- TxGraph::calculate_fee function to return Result<u64,CalculateFeeError> instead of Option<i64>

Author: notmandatory

crates/bdk/src/error.rs

@@ -9,10 +9,6 @@
 // You may not use this file except in accordance with one or both of these
 // licenses.
 
-//! Errors
-//!
-//! This module defines the errors that can be thrown by [`crate`] functions.
-
 use crate::bitcoin::Network;
 use crate::{descriptor, wallet};
 use alloc::{string::String, vec::Vec};
@@ -93,17 +89,7 @@ pub enum Error {
     Psbt(bitcoin::psbt::Error),
 }
 
-/// Errors returned by `Wallet::calculate_fee`.
-#[derive(Debug)]
-pub enum CalculateFeeError {
-    /// Missing `TxOut` for one of the inputs of the tx
-    MissingTxOut,
-    /// When the transaction is invalid according to the graph it has a negative fee
-    NegativeFee(i64),
-}
-
 /// Errors returned by miniscript when updating inconsistent PSBTs
-#[allow(missing_docs)] // TODO add docs
 #[derive(Debug, Clone)]
 pub enum MiniscriptPsbtError {
     Conversion(miniscript::descriptor::ConversionError),

crates/bdk/src/lib.rs

@@ -29,7 +29,7 @@ extern crate bip39;
 
 #[allow(unused_imports)]
 #[macro_use]
-pub mod error;
+pub(crate) mod error;
 pub mod descriptor;
 pub mod keys;
 pub mod psbt;

crates/bdk/src/wallet/mod.rs

@@ -40,6 +40,7 @@ use core::fmt;
 use core::ops::Deref;
 use miniscript::psbt::{PsbtExt, PsbtInputExt, PsbtInputSatisfier};
 
+use bdk_chain::tx_graph::CalculateFeeError;
 #[allow(unused_imports)]
 use log::{debug, error, info, trace};
 
@@ -66,7 +67,7 @@ use crate::descriptor::{
     calc_checksum, into_wallet_descriptor_checked, DerivedDescriptor, DescriptorMeta,
     ExtendedDescriptor, ExtractPolicy, IntoWalletDescriptor, Policy, XKeyUtils,
 };
-use crate::error::{CalculateFeeError, Error, MiniscriptPsbtError};
+use crate::error::{Error, MiniscriptPsbtError};
 use crate::psbt::PsbtUtils;
 use crate::signer::SignerError;
 use crate::types::*;
@@ -434,11 +435,7 @@ impl<D> Wallet<D> {
     ///
     /// Note `tx` does not have to be in the graph for this to work.
     pub fn calculate_fee(&self, tx: &Transaction) -> Result<u64, CalculateFeeError> {
-        match self.indexed_graph.graph().calculate_fee(tx) {
-            None => Err(CalculateFeeError::MissingTxOut),
-            Some(fee) if fee < 0 => Err(CalculateFeeError::NegativeFee(fee)),
-            Some(fee) => Ok(u64::try_from(fee).unwrap()),
-        }
+        self.indexed_graph.graph().calculate_fee(tx)
     }
 
     /// Calculate the `FeeRate` for a given transaction.
@@ -1072,13 +1069,12 @@ impl<D> Wallet<D> {
             return Err(Error::IrreplaceableTransaction);
         }
 
-        let fee = graph.calculate_fee(&tx).ok_or(Error::FeeRateUnavailable)?;
-        if fee < 0 {
-            // It's available but it's wrong so let's say it's unavailable
-            return Err(Error::FeeRateUnavailable)?;
-        }
-        let fee = fee as u64;
-        let feerate = FeeRate::from_wu(fee, tx.weight());
+        let fee = self
+            .calculate_fee(&tx)
+            .map_err(|_| Error::FeeRateUnavailable)?;
+        let fee_rate = self
+            .calculate_fee_rate(&tx)
+            .map_err(|_| Error::FeeRateUnavailable)?;
 
         // remove the inputs from the tx and process them
         let original_txin = tx.input.drain(..).collect::<Vec<_>>();
@@ -1162,7 +1158,7 @@ impl<D> Wallet<D> {
             utxos: original_utxos,
             bumping_fee: Some(tx_builder::PreviousFee {
                 absolute: fee,
-                rate: feerate.as_sat_per_vb(),
+                rate: fee_rate.as_sat_per_vb(),
             }),
             ..Default::default()
         };

crates/bdk/tests/wallet.rs

@@ -6,6 +6,7 @@ use bdk::wallet::coin_selection::LargestFirstCoinSelection;
 use bdk::wallet::AddressIndex::*;
 use bdk::wallet::{AddressIndex, AddressInfo, Balance, Wallet};
 use bdk::{Error, FeeRate, KeychainKind};
+use bdk_chain::tx_graph::CalculateFeeError;
 use bdk_chain::COINBASE_MATURITY;
 use bdk_chain::{BlockId, ConfirmationTime};
 use bitcoin::hashes::Hash;
@@ -104,7 +105,7 @@ fn test_get_funded_wallet_sent_and_received() {
 fn test_get_funded_wallet_tx_fees() {
     let (wallet, _) = get_funded_wallet(get_test_wpkh());
     assert_eq!(wallet.get_balance().confirmed, 50000);
-    let mut tx_fee_amounts: Vec<(Txid, Result<u64, bdk::error::CalculateFeeError>)> = wallet
+    let mut tx_fee_amounts: Vec<(Txid, Result<u64, CalculateFeeError>)> = wallet
         .transactions()
         .map(|ct| {
             let fee = wallet.calculate_fee(ct.node.tx);
@@ -116,7 +117,7 @@ fn test_get_funded_wallet_tx_fees() {
     assert_eq!(tx_fee_amounts.len(), 2);
     assert_matches!(
         tx_fee_amounts.get(1),
-        Some((_, Err(bdk::error::CalculateFeeError::MissingTxOut)))
+        Some((_, Err(CalculateFeeError::MissingTxOut(_))))
     );
     assert_matches!(tx_fee_amounts.get(0), Some((_, Ok(1000))))
 }
@@ -125,7 +126,7 @@ fn test_get_funded_wallet_tx_fees() {
 fn test_get_funded_wallet_tx_fee_rate() {
     let (wallet, _) = get_funded_wallet(get_test_wpkh());
     assert_eq!(wallet.get_balance().confirmed, 50000);
-    let mut tx_fee_rates: Vec<(Txid, Result<FeeRate, bdk::error::CalculateFeeError>)> = wallet
+    let mut tx_fee_rates: Vec<(Txid, Result<FeeRate, CalculateFeeError>)> = wallet
         .transactions()
         .map(|ct| {
             let fee_rate = wallet.calculate_fee_rate(ct.node.tx);
@@ -137,7 +138,7 @@ fn test_get_funded_wallet_tx_fee_rate() {
     assert_eq!(tx_fee_rates.len(), 2);
     assert_matches!(
         tx_fee_rates.get(1),
-        Some((_, Err(bdk::error::CalculateFeeError::MissingTxOut)))
+        Some((_, Err(CalculateFeeError::MissingTxOut(_))))
     );
     assert_matches!(tx_fee_rates.get(0), Some((_, Ok(_))))
 }

crates/chain/src/tx_graph.rs

@@ -135,6 +135,15 @@ pub struct CanonicalTx<'a, T, A> {
     pub tx_node: TxNode<'a, T, A>,
 }
 
+/// Errors returned by `TxGraph::calculate_fee`.
+#[derive(Debug, PartialEq, Eq)]
+pub enum CalculateFeeError {
+    /// Missing `TxOut` for one or more of the inputs of the tx
+    MissingTxOut(Vec<OutPoint>),
+    /// When the transaction is invalid according to the graph it has a negative fee
+    NegativeFee(i64),
+}
+
 impl<A> TxGraph<A> {
     /// Iterate over all tx outputs known by [`TxGraph`].
     ///
@@ -236,33 +245,46 @@ impl<A> TxGraph<A> {
     }
 
     /// Calculates the fee of a given transaction. Returns 0 if `tx` is a coinbase transaction.
-    /// Returns `Some(_)` if we have all the `TxOut`s being spent by `tx` in the graph (either as
-    /// the full transactions or individual txouts). If the returned value is negative, then the
-    /// transaction is invalid according to the graph.
-    ///
-    /// Returns `None` if we're missing an input for the tx in the graph.
+    /// Returns `OK(_)` if we have all the `TxOut`s being spent by `tx` in the graph (either as
+    /// the full transactions or individual txouts).
     ///
     /// Note `tx` does not have to be in the graph for this to work.
-    pub fn calculate_fee(&self, tx: &Transaction) -> Option<i64> {
+    pub fn calculate_fee(&self, tx: &Transaction) -> Result<u64, CalculateFeeError> {
         if tx.is_coin_base() {
-            return Some(0);
+            return Ok(0);
         }
-        let inputs_sum = tx
-            .input
-            .iter()
-            .map(|txin| {
-                self.get_txout(txin.previous_output)
-                    .map(|txout| txout.value as i64)
-            })
-            .sum::<Option<i64>>()?;
+        let inputs_sum = tx.input.iter().fold(
+            (0_u64, Vec::new()),
+            |(mut sum, mut missing_outpoints), txin| match self.get_txout(txin.previous_output) {
+                None => {
+                    missing_outpoints.push(txin.previous_output);
+                    (sum, missing_outpoints)
+                }
+                Some(txout) => {
+                    sum += txout.value;
+                    (sum, missing_outpoints)
+                }
+            },
+        );
+
+        let inputs_sum = if inputs_sum.1.is_empty() {
+            Ok(inputs_sum.0 as i64)
+        } else {
+            Err(CalculateFeeError::MissingTxOut(inputs_sum.1))
+        }?;
 
         let outputs_sum = tx
             .output
             .iter()
             .map(|txout| txout.value as i64)
             .sum::<i64>();
 
-        Some(inputs_sum - outputs_sum)
+        let fee = inputs_sum - outputs_sum;
+        if fee < 0 {
+            Err(CalculateFeeError::NegativeFee(fee))
+        } else {
+            Ok(fee as u64)
+        }
     }
 
     /// The transactions spending from this output.

crates/chain/tests/test_tx_graph.rs

@@ -1,5 +1,6 @@
 #[macro_use]
 mod common;
+use bdk_chain::tx_graph::CalculateFeeError;
 use bdk_chain::{
     collections::*,
     local_chain::LocalChain,
@@ -453,22 +454,29 @@ fn test_calculate_fee() {
         }],
     };
 
-    assert_eq!(graph.calculate_fee(&tx), Some(100));
+    assert_eq!(graph.calculate_fee(&tx), Ok(100));
 
     tx.input.remove(2);
 
-    // fee would be negative
-    assert_eq!(graph.calculate_fee(&tx), Some(-200));
+    // fee would be negative, should return CalculateFeeError::NegativeFee
+    assert_eq!(
+        graph.calculate_fee(&tx),
+        Err(CalculateFeeError::NegativeFee(-200))
+    );
 
-    // If we have an unknown outpoint, fee should return None.
+    // If we have an unknown outpoint, fee should return CalculateFeeError::MissingTxOut.
+    let outpoint = OutPoint {
+        txid: h!("unknown_txid"),
+        vout: 0,
+    };
     tx.input.push(TxIn {
-        previous_output: OutPoint {
-            txid: h!("unknown_txid"),
-            vout: 0,
-        },
+        previous_output: outpoint,
         ..Default::default()
     });
-    assert_eq!(graph.calculate_fee(&tx), None);
+    assert_eq!(
+        graph.calculate_fee(&tx),
+        Err(CalculateFeeError::MissingTxOut(vec!(outpoint)))
+    );
 }
 
 #[test]
@@ -485,7 +493,7 @@ fn test_calculate_fee_on_coinbase() {
 
     let graph = TxGraph::<()>::default();
 
-    assert_eq!(graph.calculate_fee(&tx), Some(0));
+    assert_eq!(graph.calculate_fee(&tx), Ok(0));
 }
 
 #[test]