Saturate to_fee to Amount::MAX

The `FeeRate::checked_mul_by_weight` function currently returns a
`NumOpResult` but all other `checked_` functions return an `Option`.
This is surprising and adds no additional information.

Change `checked_mul_by_weight` to return `None` on overflow. But in
`to_fee` saturate to `Amount::MAX` because doing so makes a few APIs
better without any risk since a fee must be checked anyway so
`Amount::MAX` as a fee is equally descriptive in the error case.

This leads to removing the `NumOpResult` from `effective_value` also.

Note that sadly we remove the very nice docs on `NumOpResult::map`
because they no longer work.

Fix: #4497

bitcoin/src/blockdata/transaction.rs

@@ -18,7 +18,6 @@ use hashes::sha256d;
 use internals::{compact_size, write_err, ToU64};
 use io::{BufRead, Write};
 use primitives::Sequence;
-use units::NumOpResult;
 
 use super::Weight;
 use crate::consensus::{self, encode, Decodable, Encodable};
@@ -777,19 +776,15 @@ impl Decodable for Transaction {
 /// * `fee_rate` - the fee rate of the transaction being created.
 /// * `input_weight_prediction` - the predicted input weight.
 /// * `value` - The value of the output we are spending.
-///
-/// # Returns
-///
-/// This will return [`NumOpResult::Error`] if the fee calculation (fee_rate * weight) overflows.
-/// Otherwise, [`NumOpResult::Valid`] will wrap the successful calculation.
 pub fn effective_value(
     fee_rate: FeeRate,
     input_weight_prediction: InputWeightPrediction,
     value: Amount,
-) -> NumOpResult<SignedAmount> {
+) -> SignedAmount {
     let weight = input_weight_prediction.total_weight();
+    let fee = fee_rate.to_fee(weight);
 
-    fee_rate.to_fee(weight).map(Amount::to_signed).and_then(|fee| value.to_signed() - fee) // Cannot overflow.
+    (value.to_signed() - fee.to_signed()).expect("cannot overflow")
 }
 
 /// Predicts the weight of a to-be-constructed transaction.
@@ -1693,8 +1688,7 @@ mod tests {
     fn effective_value_happy_path() {
         let value = "1 cBTC".parse::<Amount>().unwrap();
         let fee_rate = FeeRate::from_sat_per_kwu(10);
-        let effective_value =
+        let effective_value = effective_value(fee_rate, InputWeightPrediction::P2WPKH_MAX, value);
-            effective_value(fee_rate, InputWeightPrediction::P2WPKH_MAX, value).unwrap();
 
         // 10 sat/kwu * 272 wu = 4 sats (rounding up)
         let expected_fee = "3 sats".parse::<SignedAmount>().unwrap();
@@ -1706,7 +1700,7 @@ mod tests {
     fn effective_value_fee_rate_does_not_overflow() {
         let eff_value =
             effective_value(FeeRate::MAX, InputWeightPrediction::P2WPKH_MAX, Amount::ZERO);
-        assert!(eff_value.is_error());
+        assert_eq!(eff_value, SignedAmount::MIN)
     }
 
     #[test]

units/src/fee.rs

@@ -115,12 +115,17 @@ impl Amount {
 }
 
 impl FeeRate {
-    /// Calculates the fee by multiplying this fee rate by weight, in weight units, returning
+    /// Calculates the fee by multiplying this fee rate by weight.
-    /// [`NumOpResult::Error`] if an overflow occurred.
     ///
-    /// This is equivalent to `Self::checked_mul_by_weight()`.
+    /// Computes the absolute fee amount for a given [`Weight`] at this fee rate. When the resulting
-    pub fn to_fee(self, weight: Weight) -> NumOpResult<Amount> {
+    /// fee is a non-integer amount, the amount is rounded up, ensuring that the transaction fee is
-        self.checked_mul_by_weight(weight)
+    /// enough instead of falling short if rounded down.
+    ///
+    /// If the calculation would overflow we saturate to [`Amount::MAX`]. Since such a fee can never
+    /// be paid this is meaningful as an error case while still removing the possibility of silently
+    /// wrapping.
+    pub fn to_fee(self, weight: Weight) -> Amount {
+        self.checked_mul_by_weight(weight).unwrap_or(Amount::MAX)
     }
 
     /// Calculates the fee by multiplying this fee rate by weight, in weight units, returning [`None`]
@@ -129,9 +134,7 @@ impl FeeRate {
     /// This is equivalent to `Self::checked_mul_by_weight()`.
     #[must_use]
     #[deprecated(since = "TBD", note = "use `to_fee()` instead")]
-    pub fn fee_wu(self, weight: Weight) -> Option<Amount> {
+    pub fn fee_wu(self, weight: Weight) -> Option<Amount> { self.checked_mul_by_weight(weight) }
-        self.checked_mul_by_weight(weight).ok()
-    }
 
     /// Calculates the fee by multiplying this fee rate by weight, in virtual bytes, returning [`None`]
     /// if an overflow occurred.
@@ -140,9 +143,7 @@ impl FeeRate {
     /// `Self::fee_wu(weight)`.
     #[must_use]
     #[deprecated(since = "TBD", note = "use Weight::from_vb and then `to_fee()` instead")]
-    pub fn fee_vb(self, vb: u64) -> Option<Amount> {
+    pub fn fee_vb(self, vb: u64) -> Option<Amount> { Weight::from_vb(vb).map(|w| self.to_fee(w)) }
-        Weight::from_vb(vb).and_then(|w| self.to_fee(w).ok())
-    }
 
     /// Checked weight multiplication.
     ///
@@ -150,16 +151,14 @@ impl FeeRate {
     /// fee is a non-integer amount, the amount is rounded up, ensuring that the transaction fee is
     /// enough instead of falling short if rounded down.
     ///
-    /// Returns [`NumOpResult::Error`] if overflow occurred.
+    /// Returns [`None`] if overflow occurred.
-    pub fn checked_mul_by_weight(self, weight: Weight) -> NumOpResult<Amount> {
+    pub fn checked_mul_by_weight(self, weight: Weight) -> Option<Amount> {
-        if let Some(fee) = self.to_sat_per_kwu_floor().checked_mul(weight.to_wu()) {
+        let wu = weight.to_wu();
-            if let Some(round_up) = fee.checked_add(999) {
+        let fee_kwu = self.to_sat_per_kwu_floor().checked_mul(wu)?;
-                if let Ok(ret) = Amount::from_sat(round_up / 1_000) {
+        let bump = fee_kwu.checked_add(999)?; // We do ceil division.
-                    return NumOpResult::Valid(ret);
+        let fee = bump / 1_000;
-                }
+
-            }
+        Amount::from_sat(fee).ok()
-        }
-        NumOpResult::Error(E::while_doing(MathOp::Mul))
     }
 }
 
@@ -167,7 +166,10 @@ crate::internal_macros::impl_op_for_references! {
     impl ops::Mul<FeeRate> for Weight {
         type Output = NumOpResult<Amount>;
         fn mul(self, rhs: FeeRate) -> Self::Output {
-            rhs.checked_mul_by_weight(self)
+            match rhs.checked_mul_by_weight(self) {
+                Some(amount) => R::Valid(amount),
+                None => R::Error(E::while_doing(MathOp::Mul)),
+            }
         }
     }
     impl ops::Mul<FeeRate> for NumOpResult<Weight> {
@@ -204,7 +206,10 @@ crate::internal_macros::impl_op_for_references! {
     impl ops::Mul<Weight> for FeeRate {
         type Output = NumOpResult<Amount>;
         fn mul(self, rhs: Weight) -> Self::Output {
-            self.checked_mul_by_weight(rhs)
+            match self.checked_mul_by_weight(rhs) {
+                Some(amount) => R::Valid(amount),
+                None => R::Error(E::while_doing(MathOp::Mul)),
+            }
         }
     }
     impl ops::Mul<Weight> for NumOpResult<FeeRate> {
@@ -329,7 +334,7 @@ impl Weight {
     /// enough instead of falling short if rounded down.
     ///
     /// Returns [`None`] if overflow occurred.
-    pub fn checked_mul_by_fee_rate(self, fee_rate: FeeRate) -> NumOpResult<Amount> {
+    pub fn checked_mul_by_fee_rate(self, fee_rate: FeeRate) -> Option<Amount> {
         fee_rate.checked_mul_by_weight(self)
     }
 }
@@ -346,12 +351,9 @@ mod tests {
 
     #[test]
     fn fee_wu() {
-        let operation = FeeRate::from_sat_per_kwu(10).to_fee(Weight::MAX).unwrap_err().operation();
-        assert!(operation.is_multiplication());
-
         let fee_rate = FeeRate::from_sat_per_vb(2).unwrap();
         let weight = Weight::from_vb(3).unwrap();
-        assert_eq!(fee_rate.to_fee(weight).unwrap(), Amount::from_sat_u32(6));
+        assert_eq!(fee_rate.to_fee(weight), Amount::from_sat_u32(6));
     }
 
     #[test]
@@ -364,7 +366,7 @@ mod tests {
         assert_eq!(Amount::from_sat_u32(100), fee);
 
         let fee = FeeRate::from_sat_per_kwu(10).checked_mul_by_weight(Weight::MAX);
-        assert!(fee.is_error());
+        assert!(fee.is_none());
 
         let weight = Weight::from_vb(3).unwrap();
         let fee_rate = FeeRate::from_sat_per_vb(3).unwrap();

units/src/result.rs

@@ -90,23 +90,6 @@ pub enum NumOpResult<T> {
 impl<T> NumOpResult<T> {
     /// Maps a `NumOpResult<T>` to `NumOpResult<U>` by applying a function to a
     /// contained [`NumOpResult::Valid`] value, leaving a [`NumOpResult::Error`] value untouched.
-    ///
-    /// # Examples
-    ///
-    /// ```
-    /// use bitcoin_units::{FeeRate, Amount, Weight, SignedAmount};
-    ///
-    /// let fee_rate = FeeRate::from_sat_per_vb(1).unwrap();
-    /// let weight = Weight::from_wu(1000);
-    /// let amount = Amount::from_sat_u32(1_000_000);
-    ///
-    /// let amount_after_fee = fee_rate
-    ///     .to_fee(weight) // (1 sat/ 4 wu) * (1000 wu) = 250 sat fee
-    ///     .map(|fee| fee.to_signed())
-    ///     .and_then(|fee| amount.to_signed() - fee);
-    ///
-    /// assert_eq!(amount_after_fee.unwrap(), SignedAmount::from_sat_i32(999_750))
-    /// ```
     #[inline]
     pub fn map<U, F: FnOnce(T) -> U>(self, op: F) -> NumOpResult<U> {
         match self {