Return NumOpResult when calculating fee on Amount

Currently we call the `Amount` fee calculation functions `div_by_foo`
and return an `Option` to indicate error. We have the `NumOpResult`
type that better indicates the error case.

Includes a bunch of changes to the `psbt` tests because extracting the
transaction from a PSBT has a bunch of overflow paths that need testing
caused by fee calculation.

Author: tcharding

bitcoin/src/psbt/mod.rs

@@ -1348,17 +1348,6 @@ mod tests {
     use crate::witness::Witness;
     use crate::Sequence;
 
-    /// Fee rate in sat/kwu for a high-fee PSBT with an input=5_000_000_000_000, output=1000
-    const ABSURD_FEE_RATE: FeeRate = match FeeRate::from_sat_per_kwu(15_060_240_960_843) {
-        Some(fee_rate) => fee_rate,
-        None => panic!("unreachable - no unwrap in Rust 1.63 in const"),
-    };
-    const JUST_BELOW_ABSURD_FEE_RATE: FeeRate = match FeeRate::from_sat_per_kwu(15_060_240_960_842)
-    {
-        Some(fee_rate) => fee_rate,
-        None => panic!("unreachable - no unwrap in Rust 1.63 in const"),
-    };
-
     #[track_caller]
     pub fn hex_psbt(s: &str) -> Result<Psbt, crate::psbt::error::Error> {
         let r = Vec::from_hex(s);
@@ -1442,31 +1431,43 @@ mod tests {
 
     #[test]
     fn psbt_high_fee_checks() {
-        let psbt = psbt_with_values(5_000_000_000_000, 1000);
+        let psbt = psbt_with_values(Amount::MAX.to_sat(), 1000);
+
+        // We cannot create an expected fee rate to test against because `FeeRate::from_sat_per_mvb` is private.
+        // Large fee rate errors if we pass in 1 sat/vb so just use this to get the error fee rate returned.
+        let error_fee_rate = psbt
+            .clone()
+            .extract_tx_with_fee_rate_limit(FeeRate::from_sat_per_vb_u32(1))
+            .map_err(|e| match e {
+                ExtractTxError::AbsurdFeeRate { fee_rate, .. } => fee_rate,
+                _ => panic!(""),
+            })
+            .unwrap_err();
+
+        // In `internal_extract_tx_with_fee_rate_limit` when we do fee / weight
+        // we manually saturate to `FeeRate::MAX`.
+        assert!(psbt.clone().extract_tx_with_fee_rate_limit(FeeRate::MAX).is_ok());
+
+        // These error because the fee rate is above the limit as expected.
         assert_eq!(
             psbt.clone().extract_tx().map_err(|e| match e {
                 ExtractTxError::AbsurdFeeRate { fee_rate, .. } => fee_rate,
                 _ => panic!(""),
             }),
-            Err(ABSURD_FEE_RATE)
+            Err(error_fee_rate)
         );
         assert_eq!(
             psbt.clone().extract_tx_fee_rate_limit().map_err(|e| match e {
                 ExtractTxError::AbsurdFeeRate { fee_rate, .. } => fee_rate,
                 _ => panic!(""),
             }),
-            Err(ABSURD_FEE_RATE)
-        );
-        assert_eq!(
-            psbt.clone().extract_tx_with_fee_rate_limit(JUST_BELOW_ABSURD_FEE_RATE).map_err(|e| {
-                match e {
-                    ExtractTxError::AbsurdFeeRate { fee_rate, .. } => fee_rate,
-                    _ => panic!(""),
-                }
-            }),
-            Err(ABSURD_FEE_RATE)
+            Err(error_fee_rate)
         );
-        assert!(psbt.extract_tx_with_fee_rate_limit(ABSURD_FEE_RATE).is_ok());
+
+        // No one is using an ~50 BTC fee so if we can handle this
+        // then the `FeeRate` restrictions are fine for PSBT usage.
+        let psbt = psbt_with_values(Amount::from_btc_u16(50).to_sat(), 1000); // fee = 50 BTC - 1000 sats
+        assert!(psbt.extract_tx_with_fee_rate_limit(FeeRate::MAX).is_ok());
 
         // Testing that extract_tx will error at 25k sat/vbyte (6250000 sat/kwu)
         assert_eq!(

units/src/amount/tests.rs

@@ -279,7 +279,7 @@ fn amount_checked_div_by_weight_ceil() {
     assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(864).unwrap());
 
     let fee_rate = Amount::ONE_SAT.div_by_weight_ceil(Weight::ZERO);
-    assert!(fee_rate.is_none());
+    assert!(fee_rate.is_error());
 }
 
 #[cfg(feature = "alloc")]
@@ -297,7 +297,7 @@ fn amount_checked_div_by_weight_floor() {
     assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(863).unwrap());
 
     let fee_rate = Amount::ONE_SAT.div_by_weight_floor(Weight::ZERO);
-    assert!(fee_rate.is_none());
+    assert!(fee_rate.is_error());
 }
 
 #[cfg(feature = "alloc")]
@@ -325,8 +325,8 @@ fn amount_checked_div_by_fee_rate() {
 
     // Test division by zero
     let zero_fee_rate = FeeRate::from_sat_per_kwu(0).unwrap();
-    assert!(amount.div_by_fee_rate_floor(zero_fee_rate).is_none());
-    assert!(amount.div_by_fee_rate_ceil(zero_fee_rate).is_none());
+    assert!(amount.div_by_fee_rate_floor(zero_fee_rate).is_error());
+    assert!(amount.div_by_fee_rate_ceil(zero_fee_rate).is_error());
 
     // Test with maximum amount
     let max_amount = Amount::MAX;

units/src/amount/unsigned.rs

@@ -9,13 +9,14 @@ use core::{default, fmt};
 
 #[cfg(feature = "arbitrary")]
 use arbitrary::{Arbitrary, Unstructured};
+use NumOpResult as R;
 
 use super::error::{ParseAmountErrorInner, ParseErrorInner};
 use super::{
     parse_signed_to_satoshi, split_amount_and_denomination, Denomination, Display, DisplayStyle,
     OutOfRangeError, ParseAmountError, ParseError, SignedAmount,
 };
-use crate::{FeeRate, Weight};
+use crate::{FeeRate, MathOp, NumOpError as E, NumOpResult, Weight};
 
 mod encapsulate {
     use super::OutOfRangeError;
@@ -407,101 +408,96 @@ impl Amount {
     /// Checked weight floor division.
     ///
     /// Be aware that integer division loses the remainder if no exact division
-    /// can be made. See also [`Self::checked_div_by_weight_ceil`].
-    ///
-    /// Returns [`None`] if overflow occurred.
-    #[must_use]
-    pub const fn div_by_weight_floor(self, weight: Weight) -> Option<FeeRate> {
+    /// can be made. See also [`Self::div_by_weight_ceil`].
+    pub const fn div_by_weight_floor(self, weight: Weight) -> NumOpResult<FeeRate> {
         let wu = weight.to_wu();
-        if wu == 0 {
-            return None;
-        }
 
         // Mul by 1,000 because we use per/kwu.
-        match self.to_sat().checked_mul(1_000) {
-            Some(sats) => {
-                let fee_rate = sats / wu;
-                FeeRate::from_sat_per_kwu(fee_rate)
+        if let Some(sats) = self.to_sat().checked_mul(1_000) {
+            match sats.checked_div(wu) {
+                Some(fee_rate) =>
+                    if let Ok(amount) = Amount::from_sat(fee_rate) {
+                        return FeeRate::from_per_kwu(amount);
+                    },
+                None => return R::Error(E::while_doing(MathOp::Div)),
             }
-            None => None,
         }
+        // Use `MathOp::Mul` because `Div` implies div by zero.
+        R::Error(E::while_doing(MathOp::Mul))
     }
 
     /// Checked weight ceiling division.
     ///
     /// Be aware that integer division loses the remainder if no exact division
     /// can be made. This method rounds up ensuring the transaction fee rate is
-    /// sufficient. See also [`Self::checked_div_by_weight_floor`].
-    ///
-    /// Returns [`None`] if overflow occurred.
+    /// sufficient. See also [`Self::div_by_weight_floor`].
     ///
     /// # Examples
     ///
     /// ```
     /// # use bitcoin_units::{amount, Amount, FeeRate, Weight};
     /// let amount = Amount::from_sat(10)?;
     /// let weight = Weight::from_wu(300);
-    /// let fee_rate = amount.div_by_weight_ceil(weight);
-    /// assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(34));
+    /// let fee_rate = amount.div_by_weight_ceil(weight).expect("valid fee rate");
+    /// assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(34).expect("valid fee rate"));
     /// # Ok::<_, amount::OutOfRangeError>(())
     /// ```
-    #[must_use]
-    pub const fn div_by_weight_ceil(self, weight: Weight) -> Option<FeeRate> {
+    pub const fn div_by_weight_ceil(self, weight: Weight) -> NumOpResult<FeeRate> {
         let wu = weight.to_wu();
         if wu == 0 {
-            return None;
+            return R::Error(E::while_doing(MathOp::Div));
         }
 
         // Mul by 1,000 because we use per/kwu.
         if let Some(sats) = self.to_sat().checked_mul(1_000) {
             // No need to used checked arithmetic because wu is non-zero.
             if let Some(bump) = sats.checked_add(wu - 1) {
                 let fee_rate = bump / wu;
-                return FeeRate::from_sat_per_kwu(fee_rate);
+                if let Ok(amount) = Amount::from_sat(fee_rate) {
+                    return FeeRate::from_per_kwu(amount);
+                }
             }
         }
-        None
+        // Use `MathOp::Mul` because `Div` implies div by zero.
+        R::Error(E::while_doing(MathOp::Mul))
     }
 
     /// Checked fee rate floor division.
     ///
     /// Computes the maximum weight that would result in a fee less than or equal to this amount
     /// at the given `fee_rate`. Uses floor division to ensure the resulting weight doesn't cause
     /// the fee to exceed the amount.
-    ///
-    /// Returns [`None`] if overflow occurred or if `fee_rate` is zero.
-    #[must_use]
-    pub const fn div_by_fee_rate_floor(self, fee_rate: FeeRate) -> Option<Weight> {
-        if let Some(msats) = self.to_sat().checked_mul(1000) {
-            if let Some(wu) = msats.checked_div(fee_rate.to_sat_per_kwu_ceil()) {
-                return Some(Weight::from_wu(wu));
-            }
+    pub const fn div_by_fee_rate_floor(self, fee_rate: FeeRate) -> NumOpResult<Weight> {
+        debug_assert!(Amount::MAX.to_sat().checked_mul(1_000).is_some());
+        let msats = self.to_sat() * 1_000;
+        match msats.checked_div(fee_rate.to_sat_per_kwu_ceil()) {
+            Some(wu) => R::Valid(Weight::from_wu(wu)),
+            None => R::Error(E::while_doing(MathOp::Div)),
         }
-        None
     }
 
     /// Checked fee rate ceiling division.
     ///
     /// Computes the minimum weight that would result in a fee greater than or equal to this amount
     /// at the given `fee_rate`. Uses ceiling division to ensure the resulting weight is sufficient.
-    ///
-    /// Returns [`None`] if overflow occurred or if `fee_rate` is zero.
-    #[must_use]
-    pub const fn div_by_fee_rate_ceil(self, fee_rate: FeeRate) -> Option<Weight> {
+    pub const fn div_by_fee_rate_ceil(self, fee_rate: FeeRate) -> NumOpResult<Weight> {
         // Use ceil because result is used as the divisor.
         let rate = fee_rate.to_sat_per_kwu_ceil();
+        // Early return so we do not have to use checked arithmetic below.
         if rate == 0 {
-            return None;
+            return R::Error(E::while_doing(MathOp::Div));
         }
 
-        if let Some(msats) = self.to_sat().checked_mul(1000) {
-            // No need to used checked arithmetic because rate is non-zero.
-            if let Some(bump) = msats.checked_add(rate - 1) {
+        debug_assert!(Amount::MAX.to_sat().checked_mul(1_000).is_some());
+        let msats = self.to_sat() * 1_000;
+        match msats.checked_add(rate - 1) {
+            Some(bump) => {
                 let wu = bump / rate;
-                return Some(Weight::from_wu(wu));
+                NumOpResult::Valid(Weight::from_wu(wu))
             }
+            // Use `MathOp::Add` because `Div` implies div by zero.
+            None => R::Error(E::while_doing(MathOp::Add)),
         }
-        None
     }
 }
 

units/src/fee.rs

@@ -27,7 +27,7 @@ use core::ops;
 
 use NumOpResult as R;
 
-use crate::{Amount, FeeRate, MathOp, NumOpError as E, NumOpResult, OptionExt, Weight};
+use crate::{Amount, FeeRate, MathOp, NumOpError as E, NumOpResult, Weight};
 
 crate::internal_macros::impl_op_for_references! {
     impl ops::Mul<FeeRate> for Weight {
@@ -114,7 +114,7 @@ crate::internal_macros::impl_op_for_references! {
         type Output = NumOpResult<FeeRate>;
 
         fn div(self, rhs: Weight) -> Self::Output {
-            self.div_by_weight_floor(rhs).valid_or_error(MathOp::Div)
+            self.div_by_weight_floor(rhs)
         }
     }
     impl ops::Div<Weight> for NumOpResult<Amount> {
@@ -155,7 +155,7 @@ crate::internal_macros::impl_op_for_references! {
         type Output = NumOpResult<Weight>;
 
         fn div(self, rhs: FeeRate) -> Self::Output {
-            self.div_by_fee_rate_floor(rhs).valid_or_error(MathOp::Div)
+            self.div_by_fee_rate_floor(rhs)
         }
     }
     impl ops::Div<FeeRate> for NumOpResult<Amount> {
@@ -213,10 +213,8 @@ mod tests {
     #[test]
     fn weight_mul() {
         let weight = Weight::from_vb(10).unwrap();
-        let fee: Amount = FeeRate::from_sat_per_vb(10)
-            .unwrap()
-            .mul_by_weight(weight)
-            .expect("expected Amount");
+        let fee: Amount =
+            FeeRate::from_sat_per_vb(10).unwrap().mul_by_weight(weight).expect("expected Amount");
         assert_eq!(Amount::from_sat_u32(100), fee);
 
         let fee = FeeRate::from_sat_per_kwu(10).unwrap().mul_by_weight(Weight::MAX);
@@ -282,7 +280,7 @@ mod tests {
 
         // Test that division by zero returns None
         let zero_rate = FeeRate::from_sat_per_kwu(0).unwrap();
-        assert!(amount.div_by_fee_rate_floor(zero_rate).is_none());
-        assert!(amount.div_by_fee_rate_ceil(zero_rate).is_none());
+        assert!(amount.div_by_fee_rate_floor(zero_rate).is_error());
+        assert!(amount.div_by_fee_rate_ceil(zero_rate).is_error());
     }
 }

units/src/fee_rate/mod.rs

@@ -10,7 +10,6 @@ use core::ops;
 
 #[cfg(feature = "arbitrary")]
 use arbitrary::{Arbitrary, Unstructured};
-
 use NumOpResult as R;
 
 use crate::{Amount, MathOp, NumOpError as E, NumOpResult, Weight};