From d443fe7f6613776a3dce00def0b655c4b2b36728 Mon Sep 17 00:00:00 2001 From: Steve Myers Date: Tue, 1 Aug 2023 12:42:37 -0500 Subject: [PATCH] feat(tx_graph)!: change TxGraph::calculate_fee to return Result added - tx_graph::CalculateFeeError enum BREAKING CHANGES: changed - TxGraph::calculate_fee function to return Result instead of Option --- crates/bdk/src/error.rs | 14 -------- crates/bdk/src/lib.rs | 2 +- crates/bdk/src/wallet/mod.rs | 24 ++++++------- crates/bdk/tests/wallet.rs | 9 ++--- crates/chain/src/tx_graph.rs | 54 ++++++++++++++++++++--------- crates/chain/tests/test_tx_graph.rs | 28 +++++++++------ 6 files changed, 72 insertions(+), 59 deletions(-) diff --git a/crates/bdk/src/error.rs b/crates/bdk/src/error.rs index f0e33fea..fcb5a6f7 100644 --- a/crates/bdk/src/error.rs +++ b/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), diff --git a/crates/bdk/src/lib.rs b/crates/bdk/src/lib.rs index 93ed400b..012a868a 100644 --- a/crates/bdk/src/lib.rs +++ b/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; diff --git a/crates/bdk/src/wallet/mod.rs b/crates/bdk/src/wallet/mod.rs index c939db1c..e5095a1b 100644 --- a/crates/bdk/src/wallet/mod.rs +++ b/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 Wallet { /// /// Note `tx` does not have to be in the graph for this to work. pub fn calculate_fee(&self, tx: &Transaction) -> Result { - 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 Wallet { 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::>(); @@ -1162,7 +1158,7 @@ impl Wallet { 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() }; diff --git a/crates/bdk/tests/wallet.rs b/crates/bdk/tests/wallet.rs index 8ef921e1..906900e4 100644 --- a/crates/bdk/tests/wallet.rs +++ b/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)> = wallet + let mut tx_fee_amounts: Vec<(Txid, Result)> = 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)> = wallet + let mut tx_fee_rates: Vec<(Txid, Result)> = 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(_)))) } diff --git a/crates/chain/src/tx_graph.rs b/crates/chain/src/tx_graph.rs index adb84ca2..1572cd9a 100644 --- a/crates/chain/src/tx_graph.rs +++ b/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), + /// When the transaction is invalid according to the graph it has a negative fee + NegativeFee(i64), +} + impl TxGraph { /// Iterate over all tx outputs known by [`TxGraph`]. /// @@ -236,25 +245,33 @@ impl TxGraph { } /// 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 { + pub fn calculate_fee(&self, tx: &Transaction) -> Result { 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::>()?; + 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 @@ -262,7 +279,12 @@ impl TxGraph { .map(|txout| txout.value as i64) .sum::(); - 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. diff --git a/crates/chain/tests/test_tx_graph.rs b/crates/chain/tests/test_tx_graph.rs index 26475f76..4c68f510 100644 --- a/crates/chain/tests/test_tx_graph.rs +++ b/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]