From 75106e6d82eb8d9be5f30f944eca91f1c4b061ca Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 12 Jun 2025 11:17:32 +1000 Subject: [PATCH 1/4] Remove checked_ prefix from fee functions The `Amount` and `FeeRate` types are not simple int wrappers, as such we do not need to mimic the stdlib too closely. Having the `checked_` prefix to the functions that do fee calcualtions adds no additional meaning. Note that I'm about to change the return type to use `NumOpResult` further justifying this change. Note we leave the 'Checked foo' in the functions docs title because the functions are still checked. --- units/src/amount/tests.rs | 26 ++++++++++++------------ units/src/amount/unsigned.rs | 10 +++++----- units/src/fee.rs | 38 ++++++++++++++++++------------------ units/src/fee_rate/mod.rs | 6 +++--- units/src/weight.rs | 4 ++-- 5 files changed, 42 insertions(+), 42 deletions(-) diff --git a/units/src/amount/tests.rs b/units/src/amount/tests.rs index 021fa13bc..853370adc 100644 --- a/units/src/amount/tests.rs +++ b/units/src/amount/tests.rs @@ -268,17 +268,17 @@ fn positive_sub() { #[test] fn amount_checked_div_by_weight_ceil() { let weight = Weight::from_kwu(1).unwrap(); - let fee_rate = sat(1).checked_div_by_weight_ceil(weight).unwrap(); + let fee_rate = sat(1).div_by_weight_ceil(weight).unwrap(); // 1 sats / 1,000 wu = 1 sats/kwu assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(1).unwrap()); let weight = Weight::from_wu(381); - let fee_rate = sat(329).checked_div_by_weight_ceil(weight).unwrap(); + let fee_rate = sat(329).div_by_weight_ceil(weight).unwrap(); // 329 sats / 381 wu = 863.5 sats/kwu // round up to 864 assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(864).unwrap()); - let fee_rate = Amount::ONE_SAT.checked_div_by_weight_ceil(Weight::ZERO); + let fee_rate = Amount::ONE_SAT.div_by_weight_ceil(Weight::ZERO); assert!(fee_rate.is_none()); } @@ -286,17 +286,17 @@ fn amount_checked_div_by_weight_ceil() { #[test] fn amount_checked_div_by_weight_floor() { let weight = Weight::from_kwu(1).unwrap(); - let fee_rate = sat(1).checked_div_by_weight_floor(weight).unwrap(); + let fee_rate = sat(1).div_by_weight_floor(weight).unwrap(); // 1 sats / 1,000 wu = 1 sats/kwu assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(1).unwrap()); let weight = Weight::from_wu(381); - let fee_rate = sat(329).checked_div_by_weight_floor(weight).unwrap(); + let fee_rate = sat(329).div_by_weight_floor(weight).unwrap(); // 329 sats / 381 wu = 863.5 sats/kwu // round down to 863 assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(863).unwrap()); - let fee_rate = Amount::ONE_SAT.checked_div_by_weight_floor(Weight::ZERO); + let fee_rate = Amount::ONE_SAT.div_by_weight_floor(Weight::ZERO); assert!(fee_rate.is_none()); } @@ -307,31 +307,31 @@ fn amount_checked_div_by_fee_rate() { let fee_rate = FeeRate::from_sat_per_kwu(2).unwrap(); // Test floor division - let weight = amount.checked_div_by_fee_rate_floor(fee_rate).unwrap(); + let weight = amount.div_by_fee_rate_floor(fee_rate).unwrap(); // 1000 sats / (2 sats/kwu) = 500,000 wu assert_eq!(weight, Weight::from_wu(500_000)); // Test ceiling division - let weight = amount.checked_div_by_fee_rate_ceil(fee_rate).unwrap(); + let weight = amount.div_by_fee_rate_ceil(fee_rate).unwrap(); assert_eq!(weight, Weight::from_wu(500_000)); // Same result for exact division // Test truncation behavior let amount = sat(1000); let fee_rate = FeeRate::from_sat_per_kwu(3).unwrap(); - let floor_weight = amount.checked_div_by_fee_rate_floor(fee_rate).unwrap(); - let ceil_weight = amount.checked_div_by_fee_rate_ceil(fee_rate).unwrap(); + let floor_weight = amount.div_by_fee_rate_floor(fee_rate).unwrap(); + let ceil_weight = amount.div_by_fee_rate_ceil(fee_rate).unwrap(); assert_eq!(floor_weight, Weight::from_wu(333_333)); assert_eq!(ceil_weight, Weight::from_wu(333_334)); // Test division by zero let zero_fee_rate = FeeRate::from_sat_per_kwu(0).unwrap(); - assert!(amount.checked_div_by_fee_rate_floor(zero_fee_rate).is_none()); - assert!(amount.checked_div_by_fee_rate_ceil(zero_fee_rate).is_none()); + assert!(amount.div_by_fee_rate_floor(zero_fee_rate).is_none()); + assert!(amount.div_by_fee_rate_ceil(zero_fee_rate).is_none()); // Test with maximum amount let max_amount = Amount::MAX; let small_fee_rate = FeeRate::from_sat_per_kwu(1).unwrap(); - let weight = max_amount.checked_div_by_fee_rate_floor(small_fee_rate).unwrap(); + let weight = max_amount.div_by_fee_rate_floor(small_fee_rate).unwrap(); // 21_000_000_0000_0000 sats / (1 sat/kwu) = 2_100_000_000_000_000_000 wu assert_eq!(weight, Weight::from_wu(2_100_000_000_000_000_000)); } diff --git a/units/src/amount/unsigned.rs b/units/src/amount/unsigned.rs index bbc126f91..2e25eda84 100644 --- a/units/src/amount/unsigned.rs +++ b/units/src/amount/unsigned.rs @@ -411,7 +411,7 @@ impl Amount { /// /// Returns [`None`] if overflow occurred. #[must_use] - pub const fn checked_div_by_weight_floor(self, weight: Weight) -> Option { + pub const fn div_by_weight_floor(self, weight: Weight) -> Option { let wu = weight.to_wu(); if wu == 0 { return None; @@ -441,12 +441,12 @@ impl Amount { /// # use bitcoin_units::{amount, Amount, FeeRate, Weight}; /// let amount = Amount::from_sat(10)?; /// let weight = Weight::from_wu(300); - /// let fee_rate = amount.checked_div_by_weight_ceil(weight); + /// let fee_rate = amount.div_by_weight_ceil(weight); /// assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(34)); /// # Ok::<_, amount::OutOfRangeError>(()) /// ``` #[must_use] - pub const fn checked_div_by_weight_ceil(self, weight: Weight) -> Option { + pub const fn div_by_weight_ceil(self, weight: Weight) -> Option { let wu = weight.to_wu(); if wu == 0 { return None; @@ -471,7 +471,7 @@ impl Amount { /// /// Returns [`None`] if overflow occurred or if `fee_rate` is zero. #[must_use] - pub const fn checked_div_by_fee_rate_floor(self, fee_rate: FeeRate) -> Option { + pub const fn div_by_fee_rate_floor(self, fee_rate: FeeRate) -> Option { 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)); @@ -487,7 +487,7 @@ impl Amount { /// /// Returns [`None`] if overflow occurred or if `fee_rate` is zero. #[must_use] - pub const fn checked_div_by_fee_rate_ceil(self, fee_rate: FeeRate) -> Option { + pub const fn div_by_fee_rate_ceil(self, fee_rate: FeeRate) -> Option { // Use ceil because result is used as the divisor. let rate = fee_rate.to_sat_per_kwu_ceil(); if rate == 0 { diff --git a/units/src/fee.rs b/units/src/fee.rs index 008fd920b..b156cb694 100644 --- a/units/src/fee.rs +++ b/units/src/fee.rs @@ -8,19 +8,19 @@ //! Either the weight or fee rate can be calculated if one knows the total fee and either of the //! other values. Note however that such calculations truncate (as for integer division). //! -//! We provide `fee.checked_div_by_weight_ceil(weight)` to calculate a minimum threshold fee rate +//! We provide `fee.div_by_weight_ceil(weight)` to calculate a minimum threshold fee rate //! required to pay at least `fee` for transaction with `weight`. //! //! We support various `core::ops` traits all of which return [`NumOpResult`]. //! //! For specific methods see: //! -//! * [`Amount::checked_div_by_weight_floor`] -//! * [`Amount::checked_div_by_weight_ceil`] -//! * [`Amount::checked_div_by_fee_rate_floor`] -//! * [`Amount::checked_div_by_fee_rate_ceil`] -//! * [`Weight::checked_mul_by_fee_rate`] -//! * [`FeeRate::checked_mul_by_weight`] +//! * [`Amount::div_by_weight_floor`] +//! * [`Amount::div_by_weight_ceil`] +//! * [`Amount::div_by_fee_rate_floor`] +//! * [`Amount::div_by_fee_rate_ceil`] +//! * [`Weight::mul_by_fee_rate`] +//! * [`FeeRate::mul_by_weight`] //! * [`FeeRate::to_fee`] use core::ops; @@ -33,7 +33,7 @@ crate::internal_macros::impl_op_for_references! { impl ops::Mul for Weight { type Output = NumOpResult; fn mul(self, rhs: FeeRate) -> Self::Output { - match rhs.checked_mul_by_weight(self) { + match rhs.mul_by_weight(self) { Some(amount) => R::Valid(amount), None => R::Error(E::while_doing(MathOp::Mul)), } @@ -73,7 +73,7 @@ crate::internal_macros::impl_op_for_references! { impl ops::Mul for FeeRate { type Output = NumOpResult; fn mul(self, rhs: Weight) -> Self::Output { - match self.checked_mul_by_weight(rhs) { + match self.mul_by_weight(rhs) { Some(amount) => R::Valid(amount), None => R::Error(E::while_doing(MathOp::Mul)), } @@ -114,7 +114,7 @@ crate::internal_macros::impl_op_for_references! { type Output = NumOpResult; fn div(self, rhs: Weight) -> Self::Output { - self.checked_div_by_weight_floor(rhs).valid_or_error(MathOp::Div) + self.div_by_weight_floor(rhs).valid_or_error(MathOp::Div) } } impl ops::Div for NumOpResult { @@ -155,7 +155,7 @@ crate::internal_macros::impl_op_for_references! { type Output = NumOpResult; fn div(self, rhs: FeeRate) -> Self::Output { - self.checked_div_by_fee_rate_floor(rhs).valid_or_error(MathOp::Div) + self.div_by_fee_rate_floor(rhs).valid_or_error(MathOp::Div) } } impl ops::Div for NumOpResult { @@ -211,25 +211,25 @@ mod tests { } #[test] - fn checked_weight_mul() { + fn weight_mul() { let weight = Weight::from_vb(10).unwrap(); let fee: Amount = FeeRate::from_sat_per_vb(10) .unwrap() - .checked_mul_by_weight(weight) + .mul_by_weight(weight) .expect("expected Amount"); assert_eq!(Amount::from_sat_u32(100), fee); - let fee = FeeRate::from_sat_per_kwu(10).unwrap().checked_mul_by_weight(Weight::MAX); + let fee = FeeRate::from_sat_per_kwu(10).unwrap().mul_by_weight(Weight::MAX); assert!(fee.is_none()); let weight = Weight::from_vb(3).unwrap(); let fee_rate = FeeRate::from_sat_per_vb(3).unwrap(); - let fee = fee_rate.checked_mul_by_weight(weight).unwrap(); + let fee = fee_rate.mul_by_weight(weight).unwrap(); assert_eq!(Amount::from_sat_u32(9), fee); let weight = Weight::from_wu(381); let fee_rate = FeeRate::from_sat_per_kwu(864).unwrap(); - let fee = weight.checked_mul_by_fee_rate(fee_rate).unwrap(); + let fee = weight.mul_by_fee_rate(fee_rate).unwrap(); // 381 * 0.864 yields 329.18. // The result is then rounded up to 330. assert_eq!(fee, Amount::from_sat_u32(330)); @@ -277,12 +277,12 @@ mod tests { assert_eq!(weight, Weight::from_wu(333_333)); // Verify that ceiling division gives different result - let ceil_weight = amount.checked_div_by_fee_rate_ceil(fee_rate).unwrap(); + let ceil_weight = amount.div_by_fee_rate_ceil(fee_rate).unwrap(); assert_eq!(ceil_weight, Weight::from_wu(333_334)); // Test that division by zero returns None let zero_rate = FeeRate::from_sat_per_kwu(0).unwrap(); - assert!(amount.checked_div_by_fee_rate_floor(zero_rate).is_none()); - assert!(amount.checked_div_by_fee_rate_ceil(zero_rate).is_none()); + assert!(amount.div_by_fee_rate_floor(zero_rate).is_none()); + assert!(amount.div_by_fee_rate_ceil(zero_rate).is_none()); } } diff --git a/units/src/fee_rate/mod.rs b/units/src/fee_rate/mod.rs index aec144041..70e0e371e 100644 --- a/units/src/fee_rate/mod.rs +++ b/units/src/fee_rate/mod.rs @@ -196,7 +196,7 @@ impl FeeRate { /// wrapping. pub const fn to_fee(self, weight: Weight) -> Amount { // No `unwrap_or()` in const context. - match self.checked_mul_by_weight(weight) { + match self.mul_by_weight(weight) { Some(fee) => fee, None => Amount::MAX, } @@ -208,7 +208,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) } + pub fn fee_wu(self, weight: Weight) -> Option { self.mul_by_weight(weight) } /// Calculates the fee by multiplying this fee rate by weight, in virtual bytes, returning [`None`] /// if an overflow occurred. @@ -227,7 +227,7 @@ impl FeeRate { /// /// Returns [`None`] if overflow occurred. #[must_use] - pub const fn checked_mul_by_weight(self, weight: Weight) -> Option { + pub const fn mul_by_weight(self, weight: Weight) -> Option { let wu = weight.to_wu(); if let Some(fee_kwu) = self.to_sat_per_kwu_floor().checked_mul(wu) { // Bump by 999 to do ceil division using kwu. diff --git a/units/src/weight.rs b/units/src/weight.rs index 5bf700471..0589cb0f7 100644 --- a/units/src/weight.rs +++ b/units/src/weight.rs @@ -171,8 +171,8 @@ impl Weight { /// /// Returns [`None`] if overflow occurred. #[must_use] - pub const fn checked_mul_by_fee_rate(self, fee_rate: FeeRate) -> Option { - fee_rate.checked_mul_by_weight(self) + pub const fn mul_by_fee_rate(self, fee_rate: FeeRate) -> Option { + fee_rate.mul_by_weight(self) } } From 15065b78c782afecc86647283dbb646ddacf217e Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Sat, 14 Jun 2025 07:32:44 +1000 Subject: [PATCH 2/4] 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. --- bitcoin/src/psbt/mod.rs | 49 ++++++++++----------- units/src/amount/tests.rs | 8 ++-- units/src/amount/unsigned.rs | 82 +++++++++++++++++------------------- units/src/fee.rs | 16 +++---- units/src/fee_rate/mod.rs | 1 - 5 files changed, 75 insertions(+), 81 deletions(-) diff --git a/bitcoin/src/psbt/mod.rs b/bitcoin/src/psbt/mod.rs index 20c2c6c04..09d7f5353 100644 --- a/bitcoin/src/psbt/mod.rs +++ b/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 { 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) + Err(error_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) - ); - 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!( diff --git a/units/src/amount/tests.rs b/units/src/amount/tests.rs index 853370adc..a8b4a516f 100644 --- a/units/src/amount/tests.rs +++ b/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; diff --git a/units/src/amount/unsigned.rs b/units/src/amount/unsigned.rs index 2e25eda84..2ebc34031 100644 --- a/units/src/amount/unsigned.rs +++ b/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,33 +408,29 @@ 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 { + /// can be made. See also [`Self::div_by_weight_ceil`]. + pub const fn div_by_weight_floor(self, weight: Weight) -> NumOpResult { 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 /// @@ -441,15 +438,14 @@ impl Amount { /// # 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 { + pub const fn div_by_weight_ceil(self, weight: Weight) -> NumOpResult { 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. @@ -457,10 +453,13 @@ impl Amount { // 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. @@ -468,40 +467,37 @@ impl Amount { /// 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 { - 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 { + 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 { + pub const fn div_by_fee_rate_ceil(self, fee_rate: FeeRate) -> NumOpResult { // 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 } } diff --git a/units/src/fee.rs b/units/src/fee.rs index b156cb694..43bad6231 100644 --- a/units/src/fee.rs +++ b/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 for Weight { @@ -114,7 +114,7 @@ crate::internal_macros::impl_op_for_references! { type Output = NumOpResult; 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 for NumOpResult { @@ -155,7 +155,7 @@ crate::internal_macros::impl_op_for_references! { type Output = NumOpResult; 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 for NumOpResult { @@ -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()); } } diff --git a/units/src/fee_rate/mod.rs b/units/src/fee_rate/mod.rs index 70e0e371e..b19157cae 100644 --- a/units/src/fee_rate/mod.rs +++ b/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}; From 3b0286bd563f94caae752f1ba6968c9a66b6c85a Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Sat, 14 Jun 2025 07:44:52 +1000 Subject: [PATCH 3/4] Return NumOpResult for FeeRate and Weight As we did for `Amount` change the fee functions on `FeeRate` and `Weight` to return `NumOpResult`. This change does not add that much value but it does not make things worse and it makes the API more uniform. Also reduces lines of code. --- units/src/fee.rs | 18 ++++-------------- units/src/fee_rate/mod.rs | 15 ++++++--------- units/src/weight.rs | 7 ++----- 3 files changed, 12 insertions(+), 28 deletions(-) diff --git a/units/src/fee.rs b/units/src/fee.rs index 43bad6231..16eb0e5df 100644 --- a/units/src/fee.rs +++ b/units/src/fee.rs @@ -27,17 +27,12 @@ use core::ops; use NumOpResult as R; -use crate::{Amount, FeeRate, MathOp, NumOpError as E, NumOpResult, Weight}; +use crate::{Amount, FeeRate, NumOpResult, Weight}; crate::internal_macros::impl_op_for_references! { impl ops::Mul for Weight { type Output = NumOpResult; - fn mul(self, rhs: FeeRate) -> Self::Output { - match rhs.mul_by_weight(self) { - Some(amount) => R::Valid(amount), - None => R::Error(E::while_doing(MathOp::Mul)), - } - } + fn mul(self, rhs: FeeRate) -> Self::Output { rhs.mul_by_weight(self) } } impl ops::Mul for NumOpResult { type Output = NumOpResult; @@ -72,12 +67,7 @@ crate::internal_macros::impl_op_for_references! { impl ops::Mul for FeeRate { type Output = NumOpResult; - fn mul(self, rhs: Weight) -> Self::Output { - match self.mul_by_weight(rhs) { - Some(amount) => R::Valid(amount), - None => R::Error(E::while_doing(MathOp::Mul)), - } - } + fn mul(self, rhs: Weight) -> Self::Output { self.mul_by_weight(rhs) } } impl ops::Mul for NumOpResult { type Output = NumOpResult; @@ -218,7 +208,7 @@ mod tests { assert_eq!(Amount::from_sat_u32(100), fee); let fee = FeeRate::from_sat_per_kwu(10).unwrap().mul_by_weight(Weight::MAX); - assert!(fee.is_none()); + assert!(fee.is_error()); let weight = Weight::from_vb(3).unwrap(); let fee_rate = FeeRate::from_sat_per_vb(3).unwrap(); diff --git a/units/src/fee_rate/mod.rs b/units/src/fee_rate/mod.rs index b19157cae..ddff10fee 100644 --- a/units/src/fee_rate/mod.rs +++ b/units/src/fee_rate/mod.rs @@ -196,8 +196,8 @@ impl FeeRate { pub const fn to_fee(self, weight: Weight) -> Amount { // No `unwrap_or()` in const context. match self.mul_by_weight(weight) { - Some(fee) => fee, - None => Amount::MAX, + NumOpResult::Valid(fee) => fee, + NumOpResult::Error(_) => Amount::MAX, } } @@ -207,7 +207,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.mul_by_weight(weight) } + pub fn fee_wu(self, weight: Weight) -> Option { self.mul_by_weight(weight).ok() } /// Calculates the fee by multiplying this fee rate by weight, in virtual bytes, returning [`None`] /// if an overflow occurred. @@ -223,21 +223,18 @@ impl FeeRate { /// 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. - /// - /// Returns [`None`] if overflow occurred. - #[must_use] - pub const fn mul_by_weight(self, weight: Weight) -> Option { + pub const fn mul_by_weight(self, weight: Weight) -> NumOpResult { let wu = weight.to_wu(); if let Some(fee_kwu) = self.to_sat_per_kwu_floor().checked_mul(wu) { // Bump by 999 to do ceil division using kwu. if let Some(bump) = fee_kwu.checked_add(999) { let fee = bump / 1_000; if let Ok(fee_amount) = Amount::from_sat(fee) { - return Some(fee_amount); + return NumOpResult::Valid(fee_amount); } } } - None + NumOpResult::Error(E::while_doing(MathOp::Mul)) } } diff --git a/units/src/weight.rs b/units/src/weight.rs index 0589cb0f7..b0b93ce5a 100644 --- a/units/src/weight.rs +++ b/units/src/weight.rs @@ -10,7 +10,7 @@ use arbitrary::{Arbitrary, Unstructured}; #[cfg(feature = "serde")] use serde::{Deserialize, Deserializer, Serialize, Serializer}; -use crate::{Amount, CheckedSum, FeeRate}; +use crate::{Amount, CheckedSum, FeeRate, NumOpResult}; /// The factor that non-witness serialization data is multiplied by during weight calculation. pub const WITNESS_SCALE_FACTOR: usize = 4; @@ -168,10 +168,7 @@ impl Weight { /// Computes the absolute fee amount for a given [`FeeRate`] at this weight. 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. - /// - /// Returns [`None`] if overflow occurred. - #[must_use] - pub const fn mul_by_fee_rate(self, fee_rate: FeeRate) -> Option { + pub const fn mul_by_fee_rate(self, fee_rate: FeeRate) -> NumOpResult { fee_rate.mul_by_weight(self) } } From 0ff8d82193ef562c1aedabe98a6283d09f803c0c Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 16 Jun 2025 09:14:47 +1000 Subject: [PATCH 4/4] Make FeeRate from sat constructors infallible We now have constructors that take an arbitrary size fee rate (`Amount`). The `from_sat_per_foo` constructors can be made infallible by taking a `u32` instead of `u64`. This makes the API more ergonomic but limits the fee rate to just under 42 BTC which is plenty. Note we just delete the `from_sat_per_vb_u32` function because it is unreleased, in the past we had `from_sat_per_vb_unchecked` so we could put that back in if we wanted to be a bit more kind to downstream. Can be done later, we likely want to go over the public API before release and add a few things back in that we forgot to deprecate or could not for some reason during dev. Fuzz with a new function that consumes a `u32`. --- bitcoin/src/blockdata/script/tests.rs | 4 +- bitcoin/src/blockdata/transaction.rs | 2 +- bitcoin/src/psbt/mod.rs | 6 +- .../bitcoin/deserialize_script.rs | 4 +- fuzz/src/fuzz_utils.rs | 13 ++ units/src/amount/tests.rs | 16 +- units/src/amount/unsigned.rs | 2 +- units/src/fee.rs | 28 ++-- units/src/fee_rate/mod.rs | 137 +++++++----------- units/src/fee_rate/serde.rs | 34 +++-- units/src/result.rs | 2 +- 11 files changed, 120 insertions(+), 128 deletions(-) diff --git a/bitcoin/src/blockdata/script/tests.rs b/bitcoin/src/blockdata/script/tests.rs index 590d233d3..0268d850e 100644 --- a/bitcoin/src/blockdata/script/tests.rs +++ b/bitcoin/src/blockdata/script/tests.rs @@ -751,7 +751,7 @@ fn default_dust_value() { assert!(script_p2wpkh.is_p2wpkh()); assert_eq!(script_p2wpkh.minimal_non_dust(), Amount::from_sat_u32(294)); assert_eq!( - script_p2wpkh.minimal_non_dust_custom(FeeRate::from_sat_per_vb_u32(6)), + script_p2wpkh.minimal_non_dust_custom(FeeRate::from_sat_per_vb(6)), Some(Amount::from_sat_u32(588)) ); @@ -765,7 +765,7 @@ fn default_dust_value() { assert!(script_p2pkh.is_p2pkh()); assert_eq!(script_p2pkh.minimal_non_dust(), Amount::from_sat_u32(546)); assert_eq!( - script_p2pkh.minimal_non_dust_custom(FeeRate::from_sat_per_vb_u32(6)), + script_p2pkh.minimal_non_dust_custom(FeeRate::from_sat_per_vb(6)), Some(Amount::from_sat_u32(1092)) ); } diff --git a/bitcoin/src/blockdata/transaction.rs b/bitcoin/src/blockdata/transaction.rs index f169f4168..98369280e 100644 --- a/bitcoin/src/blockdata/transaction.rs +++ b/bitcoin/src/blockdata/transaction.rs @@ -1689,7 +1689,7 @@ mod tests { #[test] fn effective_value_happy_path() { let value = "1 cBTC".parse::().unwrap(); - let fee_rate = FeeRate::from_sat_per_kwu(10).unwrap(); + let fee_rate = FeeRate::from_sat_per_kwu(10); let effective_value = effective_value(fee_rate, InputWeightPrediction::P2WPKH_MAX, value); // 10 sat/kwu * 272 wu = 3 sats (rounding up) diff --git a/bitcoin/src/psbt/mod.rs b/bitcoin/src/psbt/mod.rs index 09d7f5353..ea1ae2d3a 100644 --- a/bitcoin/src/psbt/mod.rs +++ b/bitcoin/src/psbt/mod.rs @@ -127,7 +127,7 @@ impl Psbt { /// 1000 sats/vByte. 25k sats/vByte is obviously a mistake at this point. /// /// [`extract_tx`]: Psbt::extract_tx - pub const DEFAULT_MAX_FEE_RATE: FeeRate = FeeRate::from_sat_per_vb_u32(25_000); + pub const DEFAULT_MAX_FEE_RATE: FeeRate = FeeRate::from_sat_per_vb(25_000); /// An alias for [`extract_tx_fee_rate_limit`]. /// @@ -1437,7 +1437,7 @@ mod tests { // 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)) + .extract_tx_with_fee_rate_limit(FeeRate::from_sat_per_vb(1)) .map_err(|e| match e { ExtractTxError::AbsurdFeeRate { fee_rate, .. } => fee_rate, _ => panic!(""), @@ -1475,7 +1475,7 @@ mod tests { ExtractTxError::AbsurdFeeRate { fee_rate, .. } => fee_rate, _ => panic!(""), }), - Err(FeeRate::from_sat_per_kwu(6250003).unwrap()) // 6250000 is 25k sat/vbyte + Err(FeeRate::from_sat_per_kwu(6250003)) // 6250000 is 25k sat/vbyte ); // Lowering the input satoshis by 1 lowers the sat/kwu by 3 diff --git a/fuzz/fuzz_targets/bitcoin/deserialize_script.rs b/fuzz/fuzz_targets/bitcoin/deserialize_script.rs index c227c59b4..276da5022 100644 --- a/fuzz/fuzz_targets/bitcoin/deserialize_script.rs +++ b/fuzz/fuzz_targets/bitcoin/deserialize_script.rs @@ -2,7 +2,7 @@ use bitcoin::address::Address; use bitcoin::consensus::encode; use bitcoin::script::{self, ScriptExt as _}; use bitcoin::{FeeRate, Network}; -use bitcoin_fuzz::fuzz_utils::{consume_random_bytes, consume_u64}; +use bitcoin_fuzz::fuzz_utils::{consume_random_bytes, consume_u32}; use honggfuzz::fuzz; fn do_test(data: &[u8]) { @@ -17,7 +17,7 @@ fn do_test(data: &[u8]) { let _ = script.count_sigops_legacy(); let _ = script.minimal_non_dust(); - let fee_rate = FeeRate::from_sat_per_kwu(consume_u64(&mut new_data)).unwrap(); + let fee_rate = FeeRate::from_sat_per_kwu(consume_u32(&mut new_data)); let _ = script.minimal_non_dust_custom(fee_rate); let mut b = script::Builder::new(); diff --git a/fuzz/src/fuzz_utils.rs b/fuzz/src/fuzz_utils.rs index 8a072db98..883729c3c 100644 --- a/fuzz/src/fuzz_utils.rs +++ b/fuzz/src/fuzz_utils.rs @@ -35,3 +35,16 @@ pub fn consume_u64(data: &mut &[u8]) -> u64 { u64_bytes[7], ]) } + +#[allow(dead_code)] +pub fn consume_u32(data: &mut &[u8]) -> u32 { + // We need at least 4 bytes to read a u32 + if data.len() < 4 { + return 0; + } + + let (u32_bytes, rest) = data.split_at(4); + *data = rest; + + u32::from_le_bytes([u32_bytes[0], u32_bytes[1], u32_bytes[2], u32_bytes[3]]) +} diff --git a/units/src/amount/tests.rs b/units/src/amount/tests.rs index a8b4a516f..1eebfe6f4 100644 --- a/units/src/amount/tests.rs +++ b/units/src/amount/tests.rs @@ -270,13 +270,13 @@ fn amount_checked_div_by_weight_ceil() { let weight = Weight::from_kwu(1).unwrap(); let fee_rate = sat(1).div_by_weight_ceil(weight).unwrap(); // 1 sats / 1,000 wu = 1 sats/kwu - assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(1).unwrap()); + assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(1)); let weight = Weight::from_wu(381); let fee_rate = sat(329).div_by_weight_ceil(weight).unwrap(); // 329 sats / 381 wu = 863.5 sats/kwu // round up to 864 - assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(864).unwrap()); + assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(864)); let fee_rate = Amount::ONE_SAT.div_by_weight_ceil(Weight::ZERO); assert!(fee_rate.is_error()); @@ -288,13 +288,13 @@ fn amount_checked_div_by_weight_floor() { let weight = Weight::from_kwu(1).unwrap(); let fee_rate = sat(1).div_by_weight_floor(weight).unwrap(); // 1 sats / 1,000 wu = 1 sats/kwu - assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(1).unwrap()); + assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(1)); let weight = Weight::from_wu(381); let fee_rate = sat(329).div_by_weight_floor(weight).unwrap(); // 329 sats / 381 wu = 863.5 sats/kwu // round down to 863 - assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(863).unwrap()); + assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(863)); let fee_rate = Amount::ONE_SAT.div_by_weight_floor(Weight::ZERO); assert!(fee_rate.is_error()); @@ -304,7 +304,7 @@ fn amount_checked_div_by_weight_floor() { #[test] fn amount_checked_div_by_fee_rate() { let amount = sat(1000); - let fee_rate = FeeRate::from_sat_per_kwu(2).unwrap(); + let fee_rate = FeeRate::from_sat_per_kwu(2); // Test floor division let weight = amount.div_by_fee_rate_floor(fee_rate).unwrap(); @@ -317,20 +317,20 @@ fn amount_checked_div_by_fee_rate() { // Test truncation behavior let amount = sat(1000); - let fee_rate = FeeRate::from_sat_per_kwu(3).unwrap(); + let fee_rate = FeeRate::from_sat_per_kwu(3); let floor_weight = amount.div_by_fee_rate_floor(fee_rate).unwrap(); let ceil_weight = amount.div_by_fee_rate_ceil(fee_rate).unwrap(); assert_eq!(floor_weight, Weight::from_wu(333_333)); assert_eq!(ceil_weight, Weight::from_wu(333_334)); // Test division by zero - let zero_fee_rate = FeeRate::from_sat_per_kwu(0).unwrap(); + let zero_fee_rate = FeeRate::from_sat_per_kwu(0); 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; - let small_fee_rate = FeeRate::from_sat_per_kwu(1).unwrap(); + let small_fee_rate = FeeRate::from_sat_per_kwu(1); let weight = max_amount.div_by_fee_rate_floor(small_fee_rate).unwrap(); // 21_000_000_0000_0000 sats / (1 sat/kwu) = 2_100_000_000_000_000_000 wu assert_eq!(weight, Weight::from_wu(2_100_000_000_000_000_000)); diff --git a/units/src/amount/unsigned.rs b/units/src/amount/unsigned.rs index 2ebc34031..57e27e120 100644 --- a/units/src/amount/unsigned.rs +++ b/units/src/amount/unsigned.rs @@ -439,7 +439,7 @@ impl Amount { /// let amount = Amount::from_sat(10)?; /// let weight = Weight::from_wu(300); /// 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")); + /// assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(34)); /// # Ok::<_, amount::OutOfRangeError>(()) /// ``` pub const fn div_by_weight_ceil(self, weight: Weight) -> NumOpResult { diff --git a/units/src/fee.rs b/units/src/fee.rs index 16eb0e5df..0cd69ad8c 100644 --- a/units/src/fee.rs +++ b/units/src/fee.rs @@ -190,12 +190,12 @@ mod tests { #[test] fn fee_rate_div_by_weight() { let fee_rate = (Amount::from_sat_u32(329) / Weight::from_wu(381)).unwrap(); - assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(863).unwrap()); + assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(863)); } #[test] fn fee_wu() { - let fee_rate = FeeRate::from_sat_per_vb(2).unwrap(); + let fee_rate = FeeRate::from_sat_per_vb(2); let weight = Weight::from_vb(3).unwrap(); assert_eq!(fee_rate.to_fee(weight), Amount::from_sat_u32(6)); } @@ -204,19 +204,19 @@ mod tests { 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"); + FeeRate::from_sat_per_vb(10).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); + let fee = FeeRate::from_sat_per_kwu(10).mul_by_weight(Weight::MAX); assert!(fee.is_error()); let weight = Weight::from_vb(3).unwrap(); - let fee_rate = FeeRate::from_sat_per_vb(3).unwrap(); + let fee_rate = FeeRate::from_sat_per_vb(3); let fee = fee_rate.mul_by_weight(weight).unwrap(); assert_eq!(Amount::from_sat_u32(9), fee); let weight = Weight::from_wu(381); - let fee_rate = FeeRate::from_sat_per_kwu(864).unwrap(); + let fee_rate = FeeRate::from_sat_per_kwu(864); let fee = weight.mul_by_fee_rate(fee_rate).unwrap(); // 381 * 0.864 yields 329.18. // The result is then rounded up to 330. @@ -226,7 +226,7 @@ mod tests { #[test] #[allow(clippy::op_ref)] fn multiply() { - let two = FeeRate::from_sat_per_vb(2).unwrap(); + let two = FeeRate::from_sat_per_vb(2); let three = Weight::from_vb(3).unwrap(); let six = Amount::from_sat_u32(6); @@ -243,9 +243,9 @@ mod tests { fn amount_div_by_fee_rate() { // Test exact division let amount = Amount::from_sat_u32(1000); - let fee_rate = FeeRate::from_sat_per_kwu(2).unwrap(); - let weight = (amount / fee_rate).unwrap(); - assert_eq!(weight, Weight::from_wu(500_000)); + let fee_rate = FeeRate::from_sat_per_kwu(2); + let weight = amount / fee_rate; + assert_eq!(weight.unwrap(), Weight::from_wu(500_000)); // Test reference division let weight_ref = (&amount / fee_rate).unwrap(); @@ -257,19 +257,19 @@ mod tests { // Test truncation behavior let amount = Amount::from_sat_u32(1000); - let fee_rate = FeeRate::from_sat_per_kwu(3).unwrap(); - let weight = (amount / fee_rate).unwrap(); + let fee_rate = FeeRate::from_sat_per_kwu(3); + let weight = amount / fee_rate; // 1000 * 1000 = 1,000,000 msats // 1,000,000 / 3 = 333,333.33... wu // Should truncate down to 333,333 wu - assert_eq!(weight, Weight::from_wu(333_333)); + assert_eq!(weight.unwrap(), Weight::from_wu(333_333)); // Verify that ceiling division gives different result let ceil_weight = amount.div_by_fee_rate_ceil(fee_rate).unwrap(); assert_eq!(ceil_weight, Weight::from_wu(333_334)); // Test that division by zero returns None - let zero_rate = FeeRate::from_sat_per_kwu(0).unwrap(); + let zero_rate = FeeRate::from_sat_per_kwu(0); assert!(amount.div_by_fee_rate_floor(zero_rate).is_error()); assert!(amount.div_by_fee_rate_ceil(zero_rate).is_error()); } diff --git a/units/src/fee_rate/mod.rs b/units/src/fee_rate/mod.rs index ddff10fee..6c223f956 100644 --- a/units/src/fee_rate/mod.rs +++ b/units/src/fee_rate/mod.rs @@ -50,19 +50,15 @@ impl FeeRate { /// The minimum fee rate required to broadcast a transaction. /// /// The value matches the default Bitcoin Core policy at the time of library release. - pub const BROADCAST_MIN: FeeRate = FeeRate::from_sat_per_vb_u32(1); + pub const BROADCAST_MIN: FeeRate = FeeRate::from_sat_per_vb(1); /// The fee rate used to compute dust amount. - pub const DUST: FeeRate = FeeRate::from_sat_per_vb_u32(3); + pub const DUST: FeeRate = FeeRate::from_sat_per_vb(3); - /// Constructs a new [`FeeRate`] from satoshis per 1000 weight units, - /// returning `None` if overflow occurred. - pub const fn from_sat_per_kwu(sat_kwu: u64) -> Option { - // No `map()` in const context. - match sat_kwu.checked_mul(4_000) { - Some(fee_rate) => Some(FeeRate::from_sat_per_mvb(fee_rate)), - None => None, - } + /// Constructs a new [`FeeRate`] from satoshis per 1000 weight units. + pub const fn from_sat_per_kwu(sat_kwu: u32) -> Self { + let fee_rate = (sat_kwu as u64) * 4_000; // No `Into` in const context. + FeeRate::from_sat_per_mvb(fee_rate) } /// Constructs a new [`FeeRate`] from amount per 1000 weight units. @@ -74,14 +70,10 @@ impl FeeRate { } } - /// Constructs a new [`FeeRate`] from satoshis per virtual byte, - /// returning `None` if overflow occurred. - pub const fn from_sat_per_vb(sat_vb: u64) -> Option { - // No `map()` in const context. - match sat_vb.checked_mul(1_000_000) { - Some(fee_rate) => Some(FeeRate::from_sat_per_mvb(fee_rate)), - None => None, - } + /// Constructs a new [`FeeRate`] from satoshis per virtual byte. + pub const fn from_sat_per_vb(sat_vb: u32) -> Self { + let fee_rate = (sat_vb as u64) * 1_000_000; // No `Into` in const context. + FeeRate::from_sat_per_mvb(fee_rate) } /// Constructs a new [`FeeRate`] from amount per virtual byte. @@ -93,20 +85,10 @@ impl FeeRate { } } - /// Constructs a new [`FeeRate`] from satoshis per virtual bytes. - pub const fn from_sat_per_vb_u32(sat_vb: u32) -> Self { - let sat_vb = sat_vb as u64; // No `Into` in const context. - FeeRate::from_sat_per_mvb(sat_vb * 1_000_000) - } - - /// Constructs a new [`FeeRate`] from satoshis per kilo virtual bytes (1,000 vbytes), - /// returning `None` if overflow occurred. - pub const fn from_sat_per_kvb(sat_kvb: u64) -> Option { - // No `map()` in const context. - match sat_kvb.checked_mul(1_000) { - Some(fee_rate) => Some(FeeRate::from_sat_per_mvb(fee_rate)), - None => None, - } + /// Constructs a new [`FeeRate`] from satoshis per kilo virtual bytes (1,000 vbytes). + pub const fn from_sat_per_kvb(sat_kvb: u32) -> Self { + let fee_rate = (sat_kvb as u64) * 1_000; // No `Into` in const context. + FeeRate::from_sat_per_mvb(fee_rate) } /// Constructs a new [`FeeRate`] from satoshis per kilo virtual bytes (1,000 vbytes). @@ -301,18 +283,18 @@ mod tests { #[test] #[allow(clippy::op_ref)] fn feerate_div_nonzero() { - let rate = FeeRate::from_sat_per_kwu(200).unwrap(); + let rate = FeeRate::from_sat_per_kwu(200); let divisor = NonZeroU64::new(2).unwrap(); - assert_eq!(rate / divisor, FeeRate::from_sat_per_kwu(100).unwrap()); - assert_eq!(&rate / &divisor, FeeRate::from_sat_per_kwu(100).unwrap()); + assert_eq!(rate / divisor, FeeRate::from_sat_per_kwu(100)); + assert_eq!(&rate / &divisor, FeeRate::from_sat_per_kwu(100)); } #[test] #[allow(clippy::op_ref)] fn addition() { - let one = FeeRate::from_sat_per_kwu(1).unwrap(); - let two = FeeRate::from_sat_per_kwu(2).unwrap(); - let three = FeeRate::from_sat_per_kwu(3).unwrap(); + let one = FeeRate::from_sat_per_kwu(1); + let two = FeeRate::from_sat_per_kwu(2); + let three = FeeRate::from_sat_per_kwu(3); assert!(one + two == three); assert!(&one + two == three); @@ -323,9 +305,9 @@ mod tests { #[test] #[allow(clippy::op_ref)] fn subtract() { - let three = FeeRate::from_sat_per_kwu(3).unwrap(); - let seven = FeeRate::from_sat_per_kwu(7).unwrap(); - let ten = FeeRate::from_sat_per_kwu(10).unwrap(); + let three = FeeRate::from_sat_per_kwu(3); + let seven = FeeRate::from_sat_per_kwu(7); + let ten = FeeRate::from_sat_per_kwu(10); assert_eq!(ten - seven, three); assert_eq!(&ten - seven, three); @@ -335,44 +317,45 @@ mod tests { #[test] fn add_assign() { - let mut f = FeeRate::from_sat_per_kwu(1).unwrap(); - f += FeeRate::from_sat_per_kwu(2).unwrap(); - assert_eq!(f, FeeRate::from_sat_per_kwu(3).unwrap()); + let mut f = FeeRate::from_sat_per_kwu(1); + f += FeeRate::from_sat_per_kwu(2); + assert_eq!(f, FeeRate::from_sat_per_kwu(3)); - let mut f = FeeRate::from_sat_per_kwu(1).unwrap(); - f += &FeeRate::from_sat_per_kwu(2).unwrap(); - assert_eq!(f, FeeRate::from_sat_per_kwu(3).unwrap()); + let mut f = FeeRate::from_sat_per_kwu(1); + f += &FeeRate::from_sat_per_kwu(2); + assert_eq!(f, FeeRate::from_sat_per_kwu(3)); } #[test] fn sub_assign() { - let mut f = FeeRate::from_sat_per_kwu(3).unwrap(); - f -= FeeRate::from_sat_per_kwu(2).unwrap(); - assert_eq!(f, FeeRate::from_sat_per_kwu(1).unwrap()); + let mut f = FeeRate::from_sat_per_kwu(3); + f -= FeeRate::from_sat_per_kwu(2); + assert_eq!(f, FeeRate::from_sat_per_kwu(1)); - let mut f = FeeRate::from_sat_per_kwu(3).unwrap(); - f -= &FeeRate::from_sat_per_kwu(2).unwrap(); - assert_eq!(f, FeeRate::from_sat_per_kwu(1).unwrap()); + let mut f = FeeRate::from_sat_per_kwu(3); + f -= &FeeRate::from_sat_per_kwu(2); + assert_eq!(f, FeeRate::from_sat_per_kwu(1)); } #[test] fn checked_add() { - let one = FeeRate::from_sat_per_kwu(1).unwrap(); - let two = FeeRate::from_sat_per_kwu(2).unwrap(); - let three = FeeRate::from_sat_per_kwu(3).unwrap(); + let one = FeeRate::from_sat_per_kwu(1); + let two = FeeRate::from_sat_per_kwu(2); + let three = FeeRate::from_sat_per_kwu(3); assert_eq!(one.checked_add(two).unwrap(), three); - assert!(FeeRate::from_sat_per_kvb(u64::MAX).is_none()); // sanity check. + // Sanity check - no overflow adding one to per kvb max. + let _ = FeeRate::from_sat_per_kvb(u32::MAX).checked_add(one).unwrap(); let fee_rate = FeeRate::from_sat_per_mvb(u64::MAX).checked_add(one); assert!(fee_rate.is_none()); } #[test] fn checked_sub() { - let one = FeeRate::from_sat_per_kwu(1).unwrap(); - let two = FeeRate::from_sat_per_kwu(2).unwrap(); - let three = FeeRate::from_sat_per_kwu(3).unwrap(); + let one = FeeRate::from_sat_per_kwu(1); + let two = FeeRate::from_sat_per_kwu(2); + let three = FeeRate::from_sat_per_kwu(3); assert_eq!(three.checked_sub(two).unwrap(), one); let fee_rate = FeeRate::ZERO.checked_sub(one); @@ -390,35 +373,25 @@ mod tests { #[test] fn fee_rate_from_sat_per_vb() { - let fee_rate = FeeRate::from_sat_per_vb(10).expect("expected feerate in sat/kwu"); - assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(2500).unwrap()); + let fee_rate = FeeRate::from_sat_per_vb(10); + assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(2500)); } #[test] fn fee_rate_from_sat_per_kvb() { - let fee_rate = FeeRate::from_sat_per_kvb(11).unwrap(); + let fee_rate = FeeRate::from_sat_per_kvb(11); assert_eq!(fee_rate, FeeRate::from_sat_per_mvb(11_000)); } #[test] - fn fee_rate_from_sat_per_vb_overflow() { - let fee_rate = FeeRate::from_sat_per_vb(u64::MAX); - assert!(fee_rate.is_none()); + fn from_sat_per_vb() { + let fee_rate = FeeRate::from_sat_per_vb(10); + assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(2500)); } - #[test] - fn from_sat_per_vb_u32() { - let fee_rate = FeeRate::from_sat_per_vb_u32(10); - assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(2500).unwrap()); - } - - #[test] - #[cfg(debug_assertions)] - fn from_sat_per_vb_u32_cannot_panic() { FeeRate::from_sat_per_vb_u32(u32::MAX); } - #[test] fn raw_feerate() { - let fee_rate = FeeRate::from_sat_per_kwu(749).unwrap(); + let fee_rate = FeeRate::from_sat_per_kwu(749); assert_eq!(fee_rate.to_sat_per_kwu_floor(), 749); assert_eq!(fee_rate.to_sat_per_vb_floor(), 2); assert_eq!(fee_rate.to_sat_per_vb_ceil(), 3); @@ -427,24 +400,22 @@ mod tests { #[test] fn checked_mul() { let fee_rate = FeeRate::from_sat_per_kwu(10) - .unwrap() .checked_mul(10) .expect("expected feerate in sat/kwu"); - assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(100).unwrap()); + assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(100)); - let fee_rate = FeeRate::from_sat_per_kwu(10).unwrap().checked_mul(u64::MAX); + let fee_rate = FeeRate::from_sat_per_kwu(10).checked_mul(u64::MAX); assert!(fee_rate.is_none()); } #[test] fn checked_div() { let fee_rate = FeeRate::from_sat_per_kwu(10) - .unwrap() .checked_div(10) .expect("expected feerate in sat/kwu"); - assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(1).unwrap()); + assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(1)); - let fee_rate = FeeRate::from_sat_per_kwu(10).unwrap().checked_div(0); + let fee_rate = FeeRate::from_sat_per_kwu(10).checked_div(0); assert!(fee_rate.is_none()); } diff --git a/units/src/fee_rate/serde.rs b/units/src/fee_rate/serde.rs index 27750dce6..61390976b 100644 --- a/units/src/fee_rate/serde.rs +++ b/units/src/fee_rate/serde.rs @@ -33,15 +33,19 @@ pub mod as_sat_per_kwu_floor { use serde::{Deserialize, Deserializer, Serialize, Serializer}; - use crate::FeeRate; + use crate::{Amount, FeeRate}; pub fn serialize(f: &FeeRate, s: S) -> Result { u64::serialize(&f.to_sat_per_kwu_floor(), s) } pub fn deserialize<'d, D: Deserializer<'d>>(d: D) -> Result { - FeeRate::from_sat_per_kwu(u64::deserialize(d)?) - .ok_or_else(|| serde::de::Error::custom("overflowed sats/kwu")) + let sat = u64::deserialize(d)?; + FeeRate::from_per_kwu( + Amount::from_sat(sat).map_err(|_| serde::de::Error::custom("amount out of range"))?, + ) + .into_result() + .map_err(|_| serde::de::Error::custom("fee rate too big for sats/kwu")) } pub mod opt { @@ -100,8 +104,7 @@ pub mod as_sat_per_vb_floor { use serde::{Deserialize, Deserializer, Serialize, Serializer}; - use crate::fee_rate::serde::OverflowError; - use crate::fee_rate::FeeRate; + use crate::{Amount, FeeRate}; pub fn serialize(f: &FeeRate, s: S) -> Result { u64::serialize(&f.to_sat_per_vb_floor(), s) @@ -109,9 +112,12 @@ pub mod as_sat_per_vb_floor { // Errors on overflow. pub fn deserialize<'d, D: Deserializer<'d>>(d: D) -> Result { - FeeRate::from_sat_per_vb(u64::deserialize(d)?) - .ok_or(OverflowError) - .map_err(serde::de::Error::custom) + let sat = u64::deserialize(d)?; + FeeRate::from_per_vb( + Amount::from_sat(sat).map_err(|_| serde::de::Error::custom("amount out of range"))?, + ) + .into_result() + .map_err(|_| serde::de::Error::custom("fee rate too big for sats/vb")) } pub mod opt { @@ -171,8 +177,7 @@ pub mod as_sat_per_vb_ceil { use serde::{Deserialize, Deserializer, Serialize, Serializer}; - use crate::fee_rate::serde::OverflowError; - use crate::fee_rate::FeeRate; + use crate::{Amount, FeeRate}; pub fn serialize(f: &FeeRate, s: S) -> Result { u64::serialize(&f.to_sat_per_vb_ceil(), s) @@ -180,9 +185,12 @@ pub mod as_sat_per_vb_ceil { // Errors on overflow. pub fn deserialize<'d, D: Deserializer<'d>>(d: D) -> Result { - FeeRate::from_sat_per_vb(u64::deserialize(d)?) - .ok_or(OverflowError) - .map_err(serde::de::Error::custom) + let sat = u64::deserialize(d)?; + FeeRate::from_per_vb( + Amount::from_sat(sat).map_err(|_| serde::de::Error::custom("amount out of range"))?, + ) + .into_result() + .map_err(|_| serde::de::Error::custom("fee rate too big for sats/vb")) } pub mod opt { diff --git a/units/src/result.rs b/units/src/result.rs index a6619ac68..36f9094e0 100644 --- a/units/src/result.rs +++ b/units/src/result.rs @@ -58,7 +58,7 @@ use crate::{Amount, FeeRate, SignedAmount, Weight}; /// let a = Amount::from_sat(123).expect("valid amount"); /// let b = Amount::from_sat(467).expect("valid amount"); /// // Fee rate for transaction. -/// let fee_rate = FeeRate::from_sat_per_vb(1).unwrap(); +/// let fee_rate = FeeRate::from_sat_per_vb(1); /// /// // Somewhat contrived example to show addition operator chained with division. /// let max_fee = a + b;