From d174c06a4ac776fa2cbb4a24a17a6b21c4863b85 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 21 May 2025 08:45:31 +1000 Subject: [PATCH] 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 | 16 +++----- units/src/fee.rs | 60 ++++++++++++++-------------- units/src/result.rs | 17 -------- 3 files changed, 36 insertions(+), 57 deletions(-) diff --git a/bitcoin/src/blockdata/transaction.rs b/bitcoin/src/blockdata/transaction.rs index bea8e5bdc..678ac8d21 100644 --- a/bitcoin/src/blockdata/transaction.rs +++ b/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 { 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::().unwrap(); let fee_rate = FeeRate::from_sat_per_kwu(10); - let effective_value = - effective_value(fee_rate, InputWeightPrediction::P2WPKH_MAX, value).unwrap(); + let effective_value = effective_value(fee_rate, InputWeightPrediction::P2WPKH_MAX, value); // 10 sat/kwu * 272 wu = 4 sats (rounding up) let expected_fee = "3 sats".parse::().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] diff --git a/units/src/fee.rs b/units/src/fee.rs index 2db3803ba..596b5f99d 100644 --- a/units/src/fee.rs +++ b/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 - /// [`NumOpResult::Error`] if an overflow occurred. + /// Calculates the fee by multiplying this fee rate by weight. /// - /// This is equivalent to `Self::checked_mul_by_weight()`. - pub fn to_fee(self, weight: Weight) -> NumOpResult { - self.checked_mul_by_weight(weight) + /// Computes the absolute fee amount for a given [`Weight`] at this fee rate. When the resulting + /// 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. + /// + /// 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 { - self.checked_mul_by_weight(weight).ok() - } + pub fn fee_wu(self, weight: Weight) -> Option { self.checked_mul_by_weight(weight) } /// 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 { - Weight::from_vb(vb).and_then(|w| self.to_fee(w).ok()) - } + pub fn fee_vb(self, vb: u64) -> Option { Weight::from_vb(vb).map(|w| self.to_fee(w)) } /// 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. - pub fn checked_mul_by_weight(self, weight: Weight) -> NumOpResult { - if let Some(fee) = self.to_sat_per_kwu_floor().checked_mul(weight.to_wu()) { - if let Some(round_up) = fee.checked_add(999) { - if let Ok(ret) = Amount::from_sat(round_up / 1_000) { - return NumOpResult::Valid(ret); - } - } - } - NumOpResult::Error(E::while_doing(MathOp::Mul)) + /// Returns [`None`] if overflow occurred. + pub fn checked_mul_by_weight(self, weight: Weight) -> Option { + let wu = weight.to_wu(); + let fee_kwu = self.to_sat_per_kwu_floor().checked_mul(wu)?; + let bump = fee_kwu.checked_add(999)?; // We do ceil division. + let fee = bump / 1_000; + + Amount::from_sat(fee).ok() } } @@ -167,7 +166,10 @@ crate::internal_macros::impl_op_for_references! { impl ops::Mul for Weight { type Output = NumOpResult; 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 for NumOpResult { @@ -204,7 +206,10 @@ crate::internal_macros::impl_op_for_references! { impl ops::Mul for FeeRate { type Output = NumOpResult; 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 for NumOpResult { @@ -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 { + pub fn checked_mul_by_fee_rate(self, fee_rate: FeeRate) -> Option { 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(); diff --git a/units/src/result.rs b/units/src/result.rs index 7de341f60..c64ab5cbe 100644 --- a/units/src/result.rs +++ b/units/src/result.rs @@ -90,23 +90,6 @@ pub enum NumOpResult { impl NumOpResult { /// Maps a `NumOpResult` to `NumOpResult` 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>(self, op: F) -> NumOpResult { match self {