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};