From 64098e4578c93a3c2bbf337abf0fc19f12cf860d Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 21 May 2025 05:24:34 +1000 Subject: [PATCH 01/15] Remove encapsulate module from fee rate This module is a PITA to work on, just remove it until the dust settles on fee rate. While we are at it make the rustdocs on the getter more terse. --- units/src/fee_rate/mod.rs | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/units/src/fee_rate/mod.rs b/units/src/fee_rate/mod.rs index 672fd3fbb..dd2cbb007 100644 --- a/units/src/fee_rate/mod.rs +++ b/units/src/fee_rate/mod.rs @@ -11,26 +11,12 @@ use core::ops; #[cfg(feature = "arbitrary")] use arbitrary::{Arbitrary, Unstructured}; -mod encapsulate { - /// Fee rate. - /// - /// This is an integer newtype representing fee rate in `sat/kwu`. It provides protection - /// against mixing up the types as well as basic formatting features. - #[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] - pub struct FeeRate(u64); - - impl FeeRate { - /// Constructs a new [`FeeRate`] from satoshis per 1000 weight units. - pub const fn from_sat_per_kwu(sat_kwu: u64) -> Self { FeeRate(sat_kwu) } - - /// Returns raw fee rate. - /// - /// Can be used instead of `into()` to avoid inference issues. - pub const fn to_sat_per_kwu(self) -> u64 { self.0 } - } -} -#[doc(inline)] -pub use encapsulate::FeeRate; +/// Fee rate. +/// +/// This is an integer newtype representing fee rate in `sat/kwu`. It provides protection +/// against mixing up the types as well as basic formatting features. +#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] +pub struct FeeRate(u64); impl FeeRate { /// 0 sat/kwu. @@ -54,6 +40,9 @@ impl FeeRate { /// Fee rate used to compute dust amount. pub const DUST: FeeRate = FeeRate::from_sat_per_vb_u32(3); + /// Constructs a new [`FeeRate`] from satoshis per 1000 weight units. + pub const fn from_sat_per_kwu(sat_kwu: u64) -> Self { FeeRate(sat_kwu) } + /// Constructs a new [`FeeRate`] from satoshis per virtual bytes. /// /// # Errors @@ -75,6 +64,9 @@ impl FeeRate { /// Constructs a new [`FeeRate`] from satoshis per kilo virtual bytes (1,000 vbytes). pub const fn from_sat_per_kvb(sat_kvb: u64) -> Self { FeeRate::from_sat_per_kwu(sat_kvb / 4) } + /// Returns raw fee rate. + pub const fn to_sat_per_kwu(self) -> u64 { self.0 } + /// Converts to sat/vB rounding down. pub const fn to_sat_per_vb_floor(self) -> u64 { self.to_sat_per_kwu() / (1000 / 4) } From b929022d56d1441f842d61d06858255857975093 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 21 May 2025 05:29:25 +1000 Subject: [PATCH 02/15] Add floor/ceil versions of to_sat_per_kwu In preparation for changing the inner representation of `FeeRate` add floor and ceil versions of the getter function `to_sat_per_kwu`. For now both functions return the same thing but still call the correct one so that when we change the representation we do not need to re-visit them. --- bitcoin/src/blockdata/script/borrowed.rs | 2 +- bitcoin/src/psbt/mod.rs | 7 ++-- units/src/fee.rs | 7 ++-- units/src/fee_rate/mod.rs | 41 +++++++++++++----------- units/src/fee_rate/serde.rs | 4 +-- 5 files changed, 34 insertions(+), 27 deletions(-) diff --git a/bitcoin/src/blockdata/script/borrowed.rs b/bitcoin/src/blockdata/script/borrowed.rs index 7cc5fca7d..41a89e4a7 100644 --- a/bitcoin/src/blockdata/script/borrowed.rs +++ b/bitcoin/src/blockdata/script/borrowed.rs @@ -290,7 +290,7 @@ crate::internal_macros::define_extension_trait! { /// /// [`minimal_non_dust`]: Script::minimal_non_dust fn minimal_non_dust_custom(&self, dust_relay_fee: FeeRate) -> Option { - self.minimal_non_dust_internal(dust_relay_fee.to_sat_per_kwu() * 4) + self.minimal_non_dust_internal(dust_relay_fee.to_sat_per_kwu_ceil() * 4) } /// Counts the sigops for this Script using accurate counting. diff --git a/bitcoin/src/psbt/mod.rs b/bitcoin/src/psbt/mod.rs index 3e5c9c2da..b8f518898 100644 --- a/bitcoin/src/psbt/mod.rs +++ b/bitcoin/src/psbt/mod.rs @@ -1178,8 +1178,11 @@ impl fmt::Display for ExtractTxError { use ExtractTxError::*; match *self { - AbsurdFeeRate { fee_rate, .. } => - write!(f, "an absurdly high fee rate of {} sat/kwu", fee_rate.to_sat_per_kwu()), + AbsurdFeeRate { fee_rate, .. } => write!( + f, + "an absurdly high fee rate of {} sat/kwu", + fee_rate.to_sat_per_kwu_floor() + ), MissingInputValue { .. } => write!( f, "one of the inputs lacked value information (witness_utxo or non_witness_utxo)" diff --git a/units/src/fee.rs b/units/src/fee.rs index 4dc6c4b56..418db80ce 100644 --- a/units/src/fee.rs +++ b/units/src/fee.rs @@ -80,7 +80,7 @@ impl Amount { #[must_use] pub const fn checked_div_by_fee_rate_floor(self, fee_rate: FeeRate) -> Option { match self.to_sat().checked_mul(1000) { - Some(amount_msats) => match amount_msats.checked_div(fee_rate.to_sat_per_kwu()) { + Some(amount_msats) => match amount_msats.checked_div(fee_rate.to_sat_per_kwu_ceil()) { Some(wu) => Some(Weight::from_wu(wu)), None => None, }, @@ -96,7 +96,8 @@ 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 { - let rate = fee_rate.to_sat_per_kwu(); + // Use ceil because result is used as the divisor. + let rate = fee_rate.to_sat_per_kwu_ceil(); match self.to_sat().checked_mul(1000) { Some(amount_msats) => match rate.checked_sub(1) { Some(rate_minus_one) => match amount_msats.checked_add(rate_minus_one) { @@ -151,7 +152,7 @@ impl FeeRate { /// /// Returns [`NumOpResult::Error`] if overflow occurred. pub const fn checked_mul_by_weight(self, weight: Weight) -> NumOpResult { - if let Some(fee) = self.to_sat_per_kwu().checked_mul(weight.to_wu()) { + 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); diff --git a/units/src/fee_rate/mod.rs b/units/src/fee_rate/mod.rs index dd2cbb007..31fae382d 100644 --- a/units/src/fee_rate/mod.rs +++ b/units/src/fee_rate/mod.rs @@ -64,15 +64,18 @@ impl FeeRate { /// Constructs a new [`FeeRate`] from satoshis per kilo virtual bytes (1,000 vbytes). pub const fn from_sat_per_kvb(sat_kvb: u64) -> Self { FeeRate::from_sat_per_kwu(sat_kvb / 4) } - /// Returns raw fee rate. - pub const fn to_sat_per_kwu(self) -> u64 { self.0 } + /// Converts to sat/kwu rounding down. + pub const fn to_sat_per_kwu_floor(self) -> u64 { self.0 } + + /// Converts to sat/kwu rounding up. + pub const fn to_sat_per_kwu_ceil(self) -> u64 { self.0 } /// Converts to sat/vB rounding down. - pub const fn to_sat_per_vb_floor(self) -> u64 { self.to_sat_per_kwu() / (1000 / 4) } + pub const fn to_sat_per_vb_floor(self) -> u64 { self.to_sat_per_kwu_floor() / (1000 / 4) } /// Converts to sat/vB rounding up. pub const fn to_sat_per_vb_ceil(self) -> u64 { - (self.to_sat_per_kwu() + (1000 / 4 - 1)) / (1000 / 4) + (self.to_sat_per_kwu_floor() + (1000 / 4 - 1)) / (1000 / 4) } /// Checked multiplication. @@ -81,7 +84,7 @@ impl FeeRate { #[must_use] pub const fn checked_mul(self, rhs: u64) -> Option { // No `map()` in const context. - match self.to_sat_per_kwu().checked_mul(rhs) { + match self.to_sat_per_kwu_floor().checked_mul(rhs) { Some(res) => Some(Self::from_sat_per_kwu(res)), None => None, } @@ -93,7 +96,7 @@ impl FeeRate { #[must_use] pub const fn checked_div(self, rhs: u64) -> Option { // No `map()` in const context. - match self.to_sat_per_kwu().checked_div(rhs) { + match self.to_sat_per_kwu_floor().checked_div(rhs) { Some(res) => Some(Self::from_sat_per_kwu(res)), None => None, } @@ -105,7 +108,7 @@ impl FeeRate { #[must_use] pub const fn checked_add(self, rhs: FeeRate) -> Option { // No `map()` in const context. - match self.to_sat_per_kwu().checked_add(rhs.to_sat_per_kwu()) { + match self.to_sat_per_kwu_floor().checked_add(rhs.to_sat_per_kwu_floor()) { Some(res) => Some(Self::from_sat_per_kwu(res)), None => None, } @@ -117,7 +120,7 @@ impl FeeRate { #[must_use] pub const fn checked_sub(self, rhs: FeeRate) -> Option { // No `map()` in const context. - match self.to_sat_per_kwu().checked_sub(rhs.to_sat_per_kwu()) { + match self.to_sat_per_kwu_floor().checked_sub(rhs.to_sat_per_kwu_floor()) { Some(res) => Some(Self::from_sat_per_kwu(res)), None => None, } @@ -128,19 +131,19 @@ crate::internal_macros::impl_op_for_references! { impl ops::Add for FeeRate { type Output = FeeRate; - fn add(self, rhs: FeeRate) -> Self::Output { FeeRate::from_sat_per_kwu(self.to_sat_per_kwu() + rhs.to_sat_per_kwu()) } + fn add(self, rhs: FeeRate) -> Self::Output { FeeRate::from_sat_per_kwu(self.to_sat_per_kwu_floor() + rhs.to_sat_per_kwu_floor()) } } impl ops::Sub for FeeRate { type Output = FeeRate; - fn sub(self, rhs: FeeRate) -> Self::Output { FeeRate::from_sat_per_kwu(self.to_sat_per_kwu() - rhs.to_sat_per_kwu()) } + fn sub(self, rhs: FeeRate) -> Self::Output { FeeRate::from_sat_per_kwu(self.to_sat_per_kwu_floor() - rhs.to_sat_per_kwu_floor()) } } impl ops::Div for FeeRate { type Output = FeeRate; - fn div(self, rhs: NonZeroU64) -> Self::Output{ Self::from_sat_per_kwu(self.to_sat_per_kwu() / rhs.get()) } + fn div(self, rhs: NonZeroU64) -> Self::Output{ Self::from_sat_per_kwu(self.to_sat_per_kwu_floor() / rhs.get()) } } } crate::internal_macros::impl_add_assign!(FeeRate); @@ -151,7 +154,7 @@ impl core::iter::Sum for FeeRate { where I: Iterator, { - FeeRate::from_sat_per_kwu(iter.map(FeeRate::to_sat_per_kwu).sum()) + FeeRate::from_sat_per_kwu(iter.map(FeeRate::to_sat_per_kwu_floor).sum()) } } @@ -160,7 +163,7 @@ impl<'a> core::iter::Sum<&'a FeeRate> for FeeRate { where I: Iterator, { - FeeRate::from_sat_per_kwu(iter.map(|f| FeeRate::to_sat_per_kwu(*f)).sum()) + FeeRate::from_sat_per_kwu(iter.map(|f| FeeRate::to_sat_per_kwu_floor(*f)).sum()) } } @@ -266,11 +269,11 @@ mod tests { #[test] fn fee_rate_const() { - assert_eq!(FeeRate::ZERO.to_sat_per_kwu(), 0); - assert_eq!(FeeRate::MIN.to_sat_per_kwu(), u64::MIN); - assert_eq!(FeeRate::MAX.to_sat_per_kwu(), u64::MAX); - assert_eq!(FeeRate::BROADCAST_MIN.to_sat_per_kwu(), 250); - assert_eq!(FeeRate::DUST.to_sat_per_kwu(), 750); + assert_eq!(FeeRate::ZERO.to_sat_per_kwu_floor(), 0); + assert_eq!(FeeRate::MIN.to_sat_per_kwu_floor(), u64::MIN); + assert_eq!(FeeRate::MAX.to_sat_per_kwu_floor(), u64::MAX); + assert_eq!(FeeRate::BROADCAST_MIN.to_sat_per_kwu_floor(), 250); + assert_eq!(FeeRate::DUST.to_sat_per_kwu_floor(), 750); } #[test] @@ -304,7 +307,7 @@ mod tests { #[test] fn raw_feerate() { let fee_rate = FeeRate::from_sat_per_kwu(749); - assert_eq!(fee_rate.to_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); } diff --git a/units/src/fee_rate/serde.rs b/units/src/fee_rate/serde.rs index ae2e2fb8f..473d1ba15 100644 --- a/units/src/fee_rate/serde.rs +++ b/units/src/fee_rate/serde.rs @@ -36,7 +36,7 @@ pub mod as_sat_per_kwu { use crate::FeeRate; pub fn serialize(f: &FeeRate, s: S) -> Result { - u64::serialize(&f.to_sat_per_kwu(), s) + u64::serialize(&f.to_sat_per_kwu_floor(), s) } pub fn deserialize<'d, D: Deserializer<'d>>(d: D) -> Result { @@ -57,7 +57,7 @@ pub mod as_sat_per_kwu { #[allow(clippy::ref_option)] // API forced by serde. pub fn serialize(f: &Option, s: S) -> Result { match *f { - Some(f) => s.serialize_some(&f.to_sat_per_kwu()), + Some(f) => s.serialize_some(&f.to_sat_per_kwu_floor()), None => s.serialize_none(), } } From fe0a448e78d77af2e2cdb43f8e116297a0e7311f Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 21 May 2025 05:46:28 +1000 Subject: [PATCH 03/15] Temporarily remove const from fee calc function Code that works with `const` is annoying to use and hard to reason about. Just remove all the consts for now so we can hack on `FeeRate`. Introduces two lint warnings about manual implementation of `map` but they will go away later. --- units/src/fee.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/units/src/fee.rs b/units/src/fee.rs index 418db80ce..2db3803ba 100644 --- a/units/src/fee.rs +++ b/units/src/fee.rs @@ -55,7 +55,7 @@ impl Amount { /// # Ok::<_, amount::OutOfRangeError>(()) /// ``` #[must_use] - pub const fn checked_div_by_weight_ceil(self, weight: Weight) -> Option { + pub fn checked_div_by_weight_ceil(self, weight: Weight) -> Option { let wu = weight.to_wu(); // No `?` operator in const context. if let Some(sats) = self.to_sat().checked_mul(1_000) { @@ -78,7 +78,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 fn checked_div_by_fee_rate_floor(self, fee_rate: FeeRate) -> Option { match self.to_sat().checked_mul(1000) { Some(amount_msats) => match amount_msats.checked_div(fee_rate.to_sat_per_kwu_ceil()) { Some(wu) => Some(Weight::from_wu(wu)), @@ -95,7 +95,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 fn checked_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(); match self.to_sat().checked_mul(1000) { @@ -119,7 +119,7 @@ impl FeeRate { /// [`NumOpResult::Error`] if an overflow occurred. /// /// This is equivalent to `Self::checked_mul_by_weight()`. - pub const fn to_fee(self, weight: Weight) -> NumOpResult { + pub fn to_fee(self, weight: Weight) -> NumOpResult { self.checked_mul_by_weight(weight) } @@ -151,7 +151,7 @@ impl FeeRate { /// enough instead of falling short if rounded down. /// /// Returns [`NumOpResult::Error`] if overflow occurred. - pub const fn checked_mul_by_weight(self, weight: Weight) -> NumOpResult { + 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) { @@ -329,7 +329,7 @@ impl Weight { /// enough instead of falling short if rounded down. /// /// Returns [`None`] if overflow occurred. - pub const fn checked_mul_by_fee_rate(self, fee_rate: FeeRate) -> NumOpResult { + pub fn checked_mul_by_fee_rate(self, fee_rate: FeeRate) -> NumOpResult { fee_rate.checked_mul_by_weight(self) } } From 64ac33754f5b961704f25f919d4b93a1ffd243f8 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 26 May 2025 13:53:10 +1000 Subject: [PATCH 04/15] Add missing argument docs We document the other to arguments already, add the missing one. --- bitcoin/src/blockdata/transaction.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/bitcoin/src/blockdata/transaction.rs b/bitcoin/src/blockdata/transaction.rs index 0e64bdd3c..bea8e5bdc 100644 --- a/bitcoin/src/blockdata/transaction.rs +++ b/bitcoin/src/blockdata/transaction.rs @@ -776,6 +776,7 @@ 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 /// From d174c06a4ac776fa2cbb4a24a17a6b21c4863b85 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 21 May 2025 08:45:31 +1000 Subject: [PATCH 05/15] 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 { From 399bca531cbf2003f4ac8d880729408cf3969b13 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 21 May 2025 08:57:15 +1000 Subject: [PATCH 06/15] Reduce the FeeRate::MAX value In preparation for changing the internal representation of `FeeRate` to use MvB reduce the max value by 4_000. Done separately to make the change explicit. --- bitcoin/src/blockdata/transaction.rs | 3 ++- units/src/fee_rate/mod.rs | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/bitcoin/src/blockdata/transaction.rs b/bitcoin/src/blockdata/transaction.rs index 678ac8d21..2a3da7b46 100644 --- a/bitcoin/src/blockdata/transaction.rs +++ b/bitcoin/src/blockdata/transaction.rs @@ -1700,7 +1700,8 @@ mod tests { fn effective_value_fee_rate_does_not_overflow() { let eff_value = effective_value(FeeRate::MAX, InputWeightPrediction::P2WPKH_MAX, Amount::ZERO); - assert_eq!(eff_value, SignedAmount::MIN) + let want = SignedAmount::from_sat(-1254378597012250).unwrap(); // U64::MAX / 4_000 because of FeeRate::MAX + assert_eq!(eff_value, want) } #[test] diff --git a/units/src/fee_rate/mod.rs b/units/src/fee_rate/mod.rs index 31fae382d..84e8d9f86 100644 --- a/units/src/fee_rate/mod.rs +++ b/units/src/fee_rate/mod.rs @@ -30,7 +30,7 @@ impl FeeRate { pub const MIN: FeeRate = FeeRate::ZERO; /// Maximum possible value. - pub const MAX: FeeRate = FeeRate::from_sat_per_kwu(u64::MAX); + pub const MAX: FeeRate = FeeRate::from_sat_per_kwu(u64::MAX / 4_000); /// Minimum fee rate required to broadcast a transaction. /// @@ -271,7 +271,7 @@ mod tests { fn fee_rate_const() { assert_eq!(FeeRate::ZERO.to_sat_per_kwu_floor(), 0); assert_eq!(FeeRate::MIN.to_sat_per_kwu_floor(), u64::MIN); - assert_eq!(FeeRate::MAX.to_sat_per_kwu_floor(), u64::MAX); + assert_eq!(FeeRate::MAX.to_sat_per_kwu_floor(), u64::MAX / 4_000); assert_eq!(FeeRate::BROADCAST_MIN.to_sat_per_kwu_floor(), 250); assert_eq!(FeeRate::DUST.to_sat_per_kwu_floor(), 750); } From 2e0b88ba76ebb5543c49d4184badaa1fe46fff39 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 21 May 2025 11:09:06 +1000 Subject: [PATCH 07/15] bitcoin: Fix dust 'fee' identifiers Currently we get the fee_rate per kwu then multiply it by 4. Instead lets add a per_kvb function to `FeeRate`. We are about to change the internal representation of `FeeRate` to use MvB so for now just panic on ovelflow. Also these are fee _rates_ - we already have suboptimal names from Core in the consts in `policy`, no need to let them infect other identifiers. --- bitcoin/src/blockdata/script/borrowed.rs | 8 ++++---- units/src/fee_rate/mod.rs | 3 +++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/bitcoin/src/blockdata/script/borrowed.rs b/bitcoin/src/blockdata/script/borrowed.rs index 41a89e4a7..97c294b33 100644 --- a/bitcoin/src/blockdata/script/borrowed.rs +++ b/bitcoin/src/blockdata/script/borrowed.rs @@ -289,8 +289,8 @@ crate::internal_macros::define_extension_trait! { /// To use the default Bitcoin Core value, use [`minimal_non_dust`]. /// /// [`minimal_non_dust`]: Script::minimal_non_dust - fn minimal_non_dust_custom(&self, dust_relay_fee: FeeRate) -> Option { - self.minimal_non_dust_internal(dust_relay_fee.to_sat_per_kwu_ceil() * 4) + fn minimal_non_dust_custom(&self, dust_relay: FeeRate) -> Option { + self.minimal_non_dust_internal(dust_relay.to_sat_per_kvb()) } /// Counts the sigops for this Script using accurate counting. @@ -407,10 +407,10 @@ mod sealed { crate::internal_macros::define_extension_trait! { pub(crate) trait ScriptExtPriv impl for Script { - fn minimal_non_dust_internal(&self, dust_relay_fee: u64) -> Option { + fn minimal_non_dust_internal(&self, dust_relay_fee_rate_per_kvb: u64) -> Option { // This must never be lower than Bitcoin Core's GetDustThreshold() (as of v0.21) as it may // otherwise allow users to create transactions which likely can never be broadcast/confirmed. - let sats = dust_relay_fee + let sats = dust_relay_fee_rate_per_kvb .checked_mul(if self.is_op_return() { 0 } else if self.is_witness_program() { diff --git a/units/src/fee_rate/mod.rs b/units/src/fee_rate/mod.rs index 84e8d9f86..f2f73e162 100644 --- a/units/src/fee_rate/mod.rs +++ b/units/src/fee_rate/mod.rs @@ -78,6 +78,9 @@ impl FeeRate { (self.to_sat_per_kwu_floor() + (1000 / 4 - 1)) / (1000 / 4) } + /// Converts to sat/kvb. + pub const fn to_sat_per_kvb(self) -> u64 { self.to_sat_per_kwu_floor() * 4 } + /// Checked multiplication. /// /// Computes `self * rhs` returning [`None`] if overflow occurred. From b27d8e581933aab5d7af5b034f9a422bc41c9bb5 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 21 May 2025 11:18:15 +1000 Subject: [PATCH 08/15] Change the internal representation of FeeRate To get more precision use sats per million virtual bytes. To make review easier keep most calls in tests using `FeeRate::from_sats_per_kwu` and just unwrap. These can likely be cleaned up later on if we want to. For `serde` just change the module to `_floor` and leave it at that. The serde stuff likely needs re-visiting before release anyways. --- bitcoin/src/blockdata/script/borrowed.rs | 2 +- bitcoin/src/blockdata/transaction.rs | 2 +- bitcoin/src/psbt/mod.rs | 14 +- .../bitcoin/deserialize_script.rs | 2 +- units/src/amount/tests.rs | 16 +- units/src/fee.rs | 46 ++--- units/src/fee_rate/mod.rs | 192 ++++++++++-------- units/src/fee_rate/serde.rs | 7 +- units/tests/serde.rs | 4 +- 9 files changed, 161 insertions(+), 124 deletions(-) diff --git a/bitcoin/src/blockdata/script/borrowed.rs b/bitcoin/src/blockdata/script/borrowed.rs index 97c294b33..a6b36b198 100644 --- a/bitcoin/src/blockdata/script/borrowed.rs +++ b/bitcoin/src/blockdata/script/borrowed.rs @@ -290,7 +290,7 @@ crate::internal_macros::define_extension_trait! { /// /// [`minimal_non_dust`]: Script::minimal_non_dust fn minimal_non_dust_custom(&self, dust_relay: FeeRate) -> Option { - self.minimal_non_dust_internal(dust_relay.to_sat_per_kvb()) + self.minimal_non_dust_internal(dust_relay.to_sat_per_kvb_ceil()) } /// Counts the sigops for this Script using accurate counting. diff --git a/bitcoin/src/blockdata/transaction.rs b/bitcoin/src/blockdata/transaction.rs index 2a3da7b46..4fcff528c 100644 --- a/bitcoin/src/blockdata/transaction.rs +++ b/bitcoin/src/blockdata/transaction.rs @@ -1687,7 +1687,7 @@ mod tests { #[test] fn effective_value_happy_path() { let value = "1 cBTC".parse::().unwrap(); - let fee_rate = FeeRate::from_sat_per_kwu(10); + let fee_rate = FeeRate::from_sat_per_kwu(10).unwrap(); let effective_value = effective_value(fee_rate, InputWeightPrediction::P2WPKH_MAX, value); // 10 sat/kwu * 272 wu = 4 sats (rounding up) diff --git a/bitcoin/src/psbt/mod.rs b/bitcoin/src/psbt/mod.rs index b8f518898..07b2d8f41 100644 --- a/bitcoin/src/psbt/mod.rs +++ b/bitcoin/src/psbt/mod.rs @@ -1356,9 +1356,15 @@ mod tests { 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 = FeeRate::from_sat_per_kwu(15_060_240_960_843); - /// Fee rate which is just below absurd threshold (1 sat/kwu less) - const JUST_BELOW_ABSURD_FEE_RATE: FeeRate = FeeRate::from_sat_per_kwu(15_060_240_960_842); + 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 { @@ -1475,7 +1481,7 @@ mod tests { ExtractTxError::AbsurdFeeRate { fee_rate, .. } => fee_rate, _ => panic!(""), }), - Err(FeeRate::from_sat_per_kwu(6250003)) // 6250000 is 25k sat/vbyte + Err(FeeRate::from_sat_per_kwu(6250003).unwrap()) // 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 41ef80493..c227c59b4 100644 --- a/fuzz/fuzz_targets/bitcoin/deserialize_script.rs +++ b/fuzz/fuzz_targets/bitcoin/deserialize_script.rs @@ -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)); + let fee_rate = FeeRate::from_sat_per_kwu(consume_u64(&mut new_data)).unwrap(); let _ = script.minimal_non_dust_custom(fee_rate); let mut b = script::Builder::new(); diff --git a/units/src/amount/tests.rs b/units/src/amount/tests.rs index ebff58af5..021fa13bc 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).checked_div_by_weight_ceil(weight).unwrap(); // 1 sats / 1,000 wu = 1 sats/kwu - assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(1)); + 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(); // 329 sats / 381 wu = 863.5 sats/kwu // round up to 864 - assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(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); assert!(fee_rate.is_none()); @@ -288,13 +288,13 @@ 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(); // 1 sats / 1,000 wu = 1 sats/kwu - assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(1)); + 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(); // 329 sats / 381 wu = 863.5 sats/kwu // round down to 863 - assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(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); assert!(fee_rate.is_none()); @@ -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); + 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(); @@ -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); + 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(); 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); + 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()); // Test with maximum amount let max_amount = Amount::MAX; - let small_fee_rate = FeeRate::from_sat_per_kwu(1); + 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(); // 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/fee.rs b/units/src/fee.rs index 596b5f99d..73b4919aa 100644 --- a/units/src/fee.rs +++ b/units/src/fee.rs @@ -26,14 +26,15 @@ impl Amount { /// Returns [`None`] if overflow occurred. #[must_use] pub const fn checked_div_by_weight_floor(self, weight: Weight) -> Option { - // No `?` operator in const context. - match self.to_sat().checked_mul(1_000) { - Some(res) => match res.checked_div(weight.to_wu()) { - Some(fee_rate) => Some(FeeRate::from_sat_per_kwu(fee_rate)), - None => None, - }, - None => None, + let wu = weight.to_wu(); + if wu == 0 { + return None; } + + let sats = self.to_sat() * 1_000; // Because we use per/kwu. + let fee_rate = sats / wu; + + FeeRate::from_sat_per_kwu(fee_rate) } /// Checked weight ceiling division. @@ -51,23 +52,20 @@ impl Amount { /// let amount = Amount::from_sat(10)?; /// let weight = Weight::from_wu(300); /// let fee_rate = amount.checked_div_by_weight_ceil(weight); - /// assert_eq!(fee_rate, Some(FeeRate::from_sat_per_kwu(34))); + /// assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(34)); /// # Ok::<_, amount::OutOfRangeError>(()) /// ``` #[must_use] pub fn checked_div_by_weight_ceil(self, weight: Weight) -> Option { let wu = weight.to_wu(); - // No `?` operator in const context. - if let Some(sats) = self.to_sat().checked_mul(1_000) { - if let Some(wu_minus_one) = wu.checked_sub(1) { - if let Some(sats_plus_wu_minus_one) = sats.checked_add(wu_minus_one) { - if let Some(fee_rate) = sats_plus_wu_minus_one.checked_div(wu) { - return Some(FeeRate::from_sat_per_kwu(fee_rate)); - } - } - } + if wu == 0 { + return None; } - None + + let sats = self.to_sat() * 1_000; // Because we use per/kwu. + let fee_rate = (sats + wu - 1) / wu; + + FeeRate::from_sat_per_kwu(fee_rate) } /// Checked fee rate floor division. @@ -346,7 +344,7 @@ 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)); + assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(863).unwrap()); } #[test] @@ -365,7 +363,7 @@ mod tests { .expect("expected Amount"); assert_eq!(Amount::from_sat_u32(100), fee); - let fee = FeeRate::from_sat_per_kwu(10).checked_mul_by_weight(Weight::MAX); + let fee = FeeRate::from_sat_per_kwu(10).unwrap().checked_mul_by_weight(Weight::MAX); assert!(fee.is_none()); let weight = Weight::from_vb(3).unwrap(); @@ -374,7 +372,7 @@ mod tests { assert_eq!(Amount::from_sat_u32(9), fee); let weight = Weight::from_wu(381); - let fee_rate = FeeRate::from_sat_per_kwu(864); + let fee_rate = FeeRate::from_sat_per_kwu(864).unwrap(); let fee = weight.checked_mul_by_fee_rate(fee_rate).unwrap(); // 381 * 0.864 yields 329.18. // The result is then rounded up to 330. @@ -401,7 +399,7 @@ 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); + let fee_rate = FeeRate::from_sat_per_kwu(2).unwrap(); let weight = (amount / fee_rate).unwrap(); assert_eq!(weight, Weight::from_wu(500_000)); @@ -415,7 +413,7 @@ mod tests { // Test truncation behavior let amount = Amount::from_sat_u32(1000); - let fee_rate = FeeRate::from_sat_per_kwu(3); + let fee_rate = FeeRate::from_sat_per_kwu(3).unwrap(); let weight = (amount / fee_rate).unwrap(); // 1000 * 1000 = 1,000,000 msats // 1,000,000 / 3 = 333,333.33... wu @@ -427,7 +425,7 @@ mod tests { 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); + 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()); } diff --git a/units/src/fee_rate/mod.rs b/units/src/fee_rate/mod.rs index f2f73e162..87e89c50b 100644 --- a/units/src/fee_rate/mod.rs +++ b/units/src/fee_rate/mod.rs @@ -22,7 +22,7 @@ impl FeeRate { /// 0 sat/kwu. /// /// Equivalent to [`MIN`](Self::MIN), may better express intent in some contexts. - pub const ZERO: FeeRate = FeeRate::from_sat_per_kwu(0); + pub const ZERO: FeeRate = FeeRate::from_sat_per_mvb(0); /// Minimum possible value (0 sat/kwu). /// @@ -30,7 +30,7 @@ impl FeeRate { pub const MIN: FeeRate = FeeRate::ZERO; /// Maximum possible value. - pub const MAX: FeeRate = FeeRate::from_sat_per_kwu(u64::MAX / 4_000); + pub const MAX: FeeRate = FeeRate::from_sat_per_mvb(u64::MAX); /// Minimum fee rate required to broadcast a transaction. /// @@ -40,46 +40,66 @@ impl FeeRate { /// Fee rate used to compute dust amount. pub const DUST: FeeRate = FeeRate::from_sat_per_vb_u32(3); + /// Constructs a new [`FeeRate`] from satoshis per 1,000,000 virtual bytes. + pub(crate) const fn from_sat_per_mvb(sat_mvb: u64) -> Self { Self(sat_mvb) } + /// Constructs a new [`FeeRate`] from satoshis per 1000 weight units. - pub const fn from_sat_per_kwu(sat_kwu: u64) -> Self { FeeRate(sat_kwu) } + 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 virtual bytes. /// /// # Errors /// /// Returns [`None`] on arithmetic overflow. - pub fn from_sat_per_vb(sat_vb: u64) -> Option { - // 1 vb == 4 wu - // 1 sat/vb == 1/4 sat/wu - // sat/vb * 1000 / 4 == sat/kwu - Some(FeeRate::from_sat_per_kwu(sat_vb.checked_mul(1000 / 4)?)) + 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 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_kwu(sat_vb * (1000 / 4)) + FeeRate::from_sat_per_mvb(sat_vb * 1_000_000) } /// Constructs a new [`FeeRate`] from satoshis per kilo virtual bytes (1,000 vbytes). - pub const fn from_sat_per_kvb(sat_kvb: u64) -> Self { FeeRate::from_sat_per_kwu(sat_kvb / 4) } - - /// Converts to sat/kwu rounding down. - pub const fn to_sat_per_kwu_floor(self) -> u64 { self.0 } - - /// Converts to sat/kwu rounding up. - pub const fn to_sat_per_kwu_ceil(self) -> u64 { self.0 } - - /// Converts to sat/vB rounding down. - pub const fn to_sat_per_vb_floor(self) -> u64 { self.to_sat_per_kwu_floor() / (1000 / 4) } - - /// Converts to sat/vB rounding up. - pub const fn to_sat_per_vb_ceil(self) -> u64 { - (self.to_sat_per_kwu_floor() + (1000 / 4 - 1)) / (1000 / 4) + 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, + } } - /// Converts to sat/kvb. - pub const fn to_sat_per_kvb(self) -> u64 { self.to_sat_per_kwu_floor() * 4 } + /// Converts to sat/MvB. + pub(crate) const fn to_sat_per_mvb(self) -> u64 { self.0 } + + /// Converts to sat/kwu rounding down. + pub const fn to_sat_per_kwu_floor(self) -> u64 { self.to_sat_per_mvb() / 4_000 } + + /// Converts to sat/kwu rounding up. + pub const fn to_sat_per_kwu_ceil(self) -> u64 { (self.to_sat_per_mvb() + 3_999) / 4_000 } + + /// Converts to sat/vB rounding down. + pub const fn to_sat_per_vb_floor(self) -> u64 { self.to_sat_per_mvb() / 1_000_000 } + + /// Converts to sat/vB rounding up. + pub const fn to_sat_per_vb_ceil(self) -> u64 { (self.to_sat_per_mvb() + 999_999) / 1_000_000 } + + /// Converts to sat/kvb rounding down. + pub const fn to_sat_per_kvb_floor(self) -> u64 { self.to_sat_per_mvb() / 1_000 } + + /// Converts to sat/kvb rounding up. + pub const fn to_sat_per_kvb_ceil(self) -> u64 { (self.to_sat_per_mvb() + 999) / 1_000 } /// Checked multiplication. /// @@ -87,8 +107,8 @@ impl FeeRate { #[must_use] pub const fn checked_mul(self, rhs: u64) -> Option { // No `map()` in const context. - match self.to_sat_per_kwu_floor().checked_mul(rhs) { - Some(res) => Some(Self::from_sat_per_kwu(res)), + match self.to_sat_per_mvb().checked_mul(rhs) { + Some(res) => Some(Self::from_sat_per_mvb(res)), None => None, } } @@ -99,8 +119,8 @@ impl FeeRate { #[must_use] pub const fn checked_div(self, rhs: u64) -> Option { // No `map()` in const context. - match self.to_sat_per_kwu_floor().checked_div(rhs) { - Some(res) => Some(Self::from_sat_per_kwu(res)), + match self.to_sat_per_mvb().checked_div(rhs) { + Some(res) => Some(Self::from_sat_per_mvb(res)), None => None, } } @@ -111,8 +131,8 @@ impl FeeRate { #[must_use] pub const fn checked_add(self, rhs: FeeRate) -> Option { // No `map()` in const context. - match self.to_sat_per_kwu_floor().checked_add(rhs.to_sat_per_kwu_floor()) { - Some(res) => Some(Self::from_sat_per_kwu(res)), + match self.to_sat_per_mvb().checked_add(rhs.to_sat_per_mvb()) { + Some(res) => Some(Self::from_sat_per_mvb(res)), None => None, } } @@ -123,8 +143,8 @@ impl FeeRate { #[must_use] pub const fn checked_sub(self, rhs: FeeRate) -> Option { // No `map()` in const context. - match self.to_sat_per_kwu_floor().checked_sub(rhs.to_sat_per_kwu_floor()) { - Some(res) => Some(Self::from_sat_per_kwu(res)), + match self.to_sat_per_mvb().checked_sub(rhs.to_sat_per_mvb()) { + Some(res) => Some(Self::from_sat_per_mvb(res)), None => None, } } @@ -134,19 +154,19 @@ crate::internal_macros::impl_op_for_references! { impl ops::Add for FeeRate { type Output = FeeRate; - fn add(self, rhs: FeeRate) -> Self::Output { FeeRate::from_sat_per_kwu(self.to_sat_per_kwu_floor() + rhs.to_sat_per_kwu_floor()) } + fn add(self, rhs: FeeRate) -> Self::Output { FeeRate::from_sat_per_mvb(self.to_sat_per_mvb() + rhs.to_sat_per_mvb()) } } impl ops::Sub for FeeRate { type Output = FeeRate; - fn sub(self, rhs: FeeRate) -> Self::Output { FeeRate::from_sat_per_kwu(self.to_sat_per_kwu_floor() - rhs.to_sat_per_kwu_floor()) } + fn sub(self, rhs: FeeRate) -> Self::Output { FeeRate::from_sat_per_mvb(self.to_sat_per_mvb() - rhs.to_sat_per_mvb()) } } impl ops::Div for FeeRate { type Output = FeeRate; - fn div(self, rhs: NonZeroU64) -> Self::Output{ Self::from_sat_per_kwu(self.to_sat_per_kwu_floor() / rhs.get()) } + fn div(self, rhs: NonZeroU64) -> Self::Output{ Self::from_sat_per_mvb(self.to_sat_per_mvb() / rhs.get()) } } } crate::internal_macros::impl_add_assign!(FeeRate); @@ -157,7 +177,7 @@ impl core::iter::Sum for FeeRate { where I: Iterator, { - FeeRate::from_sat_per_kwu(iter.map(FeeRate::to_sat_per_kwu_floor).sum()) + FeeRate::from_sat_per_mvb(iter.map(FeeRate::to_sat_per_mvb).sum()) } } @@ -166,7 +186,7 @@ impl<'a> core::iter::Sum<&'a FeeRate> for FeeRate { where I: Iterator, { - FeeRate::from_sat_per_kwu(iter.map(|f| FeeRate::to_sat_per_kwu_floor(*f)).sum()) + FeeRate::from_sat_per_mvb(iter.map(|f| FeeRate::to_sat_per_mvb(*f)).sum()) } } @@ -179,7 +199,7 @@ impl<'a> Arbitrary<'a> for FeeRate { 1 => Ok(FeeRate::BROADCAST_MIN), 2 => Ok(FeeRate::DUST), 3 => Ok(FeeRate::MAX), - _ => Ok(FeeRate::from_sat_per_kwu(u64::arbitrary(u)?)), + _ => Ok(FeeRate::from_sat_per_mvb(u64::arbitrary(u)?)), } } } @@ -193,18 +213,18 @@ mod tests { #[test] #[allow(clippy::op_ref)] fn feerate_div_nonzero() { - let rate = FeeRate::from_sat_per_kwu(200); + let rate = FeeRate::from_sat_per_kwu(200).unwrap(); let divisor = NonZeroU64::new(2).unwrap(); - assert_eq!(rate / divisor, FeeRate::from_sat_per_kwu(100)); - assert_eq!(&rate / &divisor, FeeRate::from_sat_per_kwu(100)); + assert_eq!(rate / divisor, FeeRate::from_sat_per_kwu(100).unwrap()); + assert_eq!(&rate / &divisor, FeeRate::from_sat_per_kwu(100).unwrap()); } #[test] #[allow(clippy::op_ref)] fn addition() { - let one = FeeRate::from_sat_per_kwu(1); - let two = FeeRate::from_sat_per_kwu(2); - let three = FeeRate::from_sat_per_kwu(3); + 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(); assert!(one + two == three); assert!(&one + two == three); @@ -215,9 +235,9 @@ mod tests { #[test] #[allow(clippy::op_ref)] fn subtract() { - let three = FeeRate::from_sat_per_kwu(3); - let seven = FeeRate::from_sat_per_kwu(7); - let ten = FeeRate::from_sat_per_kwu(10); + 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(); assert_eq!(ten - seven, three); assert_eq!(&ten - seven, three); @@ -227,43 +247,44 @@ mod tests { #[test] fn add_assign() { - 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)); + 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()); } #[test] fn sub_assign() { - 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)); + 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()); } #[test] fn checked_add() { - let one = FeeRate::from_sat_per_kwu(1); - let two = FeeRate::from_sat_per_kwu(2); - let three = FeeRate::from_sat_per_kwu(3); + 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(); assert_eq!(one.checked_add(two).unwrap(), three); - let fee_rate = FeeRate::from_sat_per_kwu(u64::MAX).checked_add(one); + assert!(FeeRate::from_sat_per_kvb(u64::MAX).is_none()); // sanity check. + 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); - let two = FeeRate::from_sat_per_kwu(2); - let three = FeeRate::from_sat_per_kwu(3); + 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(); assert_eq!(three.checked_sub(two).unwrap(), one); let fee_rate = FeeRate::ZERO.checked_sub(one); @@ -282,13 +303,13 @@ 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)); + assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(2500).unwrap()); } #[test] fn fee_rate_from_sat_per_kvb() { - let fee_rate = FeeRate::from_sat_per_kvb(11); - assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(2)); + let fee_rate = FeeRate::from_sat_per_kvb(11).unwrap(); + assert_eq!(fee_rate, FeeRate::from_sat_per_mvb(11_000)); } #[test] @@ -300,7 +321,7 @@ mod tests { #[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)); + assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(2500).unwrap()); } #[test] @@ -309,7 +330,7 @@ mod tests { #[test] fn raw_feerate() { - let fee_rate = FeeRate::from_sat_per_kwu(749); + let fee_rate = FeeRate::from_sat_per_kwu(749).unwrap(); 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); @@ -317,21 +338,32 @@ mod tests { #[test] fn checked_mul() { - let fee_rate = - FeeRate::from_sat_per_kwu(10).checked_mul(10).expect("expected feerate in sat/kwu"); - assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(100)); + 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()); - let fee_rate = FeeRate::from_sat_per_kwu(10).checked_mul(u64::MAX); + let fee_rate = FeeRate::from_sat_per_kwu(10).unwrap().checked_mul(u64::MAX); assert!(fee_rate.is_none()); } #[test] fn checked_div() { - let fee_rate = - FeeRate::from_sat_per_kwu(10).checked_div(10).expect("expected feerate in sat/kwu"); - assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(1)); + 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()); - let fee_rate = FeeRate::from_sat_per_kwu(10).checked_div(0); + let fee_rate = FeeRate::from_sat_per_kwu(10).unwrap().checked_div(0); assert!(fee_rate.is_none()); } + + #[test] + fn mvb() { + let fee_rate = FeeRate::from_sat_per_mvb(1_234_567); + let got = fee_rate.to_sat_per_mvb(); + assert_eq!(got, 1_234_567); + } } diff --git a/units/src/fee_rate/serde.rs b/units/src/fee_rate/serde.rs index 473d1ba15..27750dce6 100644 --- a/units/src/fee_rate/serde.rs +++ b/units/src/fee_rate/serde.rs @@ -18,7 +18,7 @@ //! //! #[derive(Serialize, Deserialize)] //! pub struct Foo { -//! #[serde(with = "fee_rate::serde::as_sat_per_kwu")] +//! #[serde(with = "fee_rate::serde::as_sat_per_kwu_floor")] //! pub fee_rate: FeeRate, //! } //! ``` @@ -26,7 +26,7 @@ use core::convert::Infallible; use core::fmt; -pub mod as_sat_per_kwu { +pub mod as_sat_per_kwu_floor { //! Serialize and deserialize [`FeeRate`] denominated in satoshis per 1000 weight units. //! //! Use with `#[serde(with = "fee_rate::serde::as_sat_per_kwu")]`. @@ -40,7 +40,8 @@ pub mod as_sat_per_kwu { } pub fn deserialize<'d, D: Deserializer<'d>>(d: D) -> Result { - Ok(FeeRate::from_sat_per_kwu(u64::deserialize(d)?)) + FeeRate::from_sat_per_kwu(u64::deserialize(d)?) + .ok_or_else(|| serde::de::Error::custom("overflowed sats/kwu")) } pub mod opt { diff --git a/units/tests/serde.rs b/units/tests/serde.rs index 4c93da1f0..ca3b88e89 100644 --- a/units/tests/serde.rs +++ b/units/tests/serde.rs @@ -37,13 +37,13 @@ struct Serde { vb_floor: FeeRate, #[serde(with = "fee_rate::serde::as_sat_per_vb_ceil")] vb_ceil: FeeRate, - #[serde(with = "fee_rate::serde::as_sat_per_kwu")] + #[serde(with = "fee_rate::serde::as_sat_per_kwu_floor")] kwu: FeeRate, #[serde(with = "fee_rate::serde::as_sat_per_vb_floor::opt")] opt_vb_floor: Option, #[serde(with = "fee_rate::serde::as_sat_per_vb_ceil::opt")] opt_vb_ceil: Option, - #[serde(with = "fee_rate::serde::as_sat_per_kwu::opt")] + #[serde(with = "fee_rate::serde::as_sat_per_kwu_floor::opt")] opt_kwu: Option, a: BlockHeight, From 1bd1e89458fe5ddca8c5c199bd3859a09640c636 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 21 May 2025 11:23:48 +1000 Subject: [PATCH 09/15] Re-introduce FeeRate encapsulate module Now we have the `fee_rate` module clened up re-introduce the `encapsulate` module using MvB. --- units/src/fee_rate/mod.rs | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/units/src/fee_rate/mod.rs b/units/src/fee_rate/mod.rs index 87e89c50b..ceaf239f4 100644 --- a/units/src/fee_rate/mod.rs +++ b/units/src/fee_rate/mod.rs @@ -11,12 +11,24 @@ use core::ops; #[cfg(feature = "arbitrary")] use arbitrary::{Arbitrary, Unstructured}; -/// Fee rate. -/// -/// This is an integer newtype representing fee rate in `sat/kwu`. It provides protection -/// against mixing up the types as well as basic formatting features. -#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] -pub struct FeeRate(u64); +mod encapsulate { + /// Fee rate. + /// + /// This is an integer newtype representing fee rate. It provides protection + /// against mixing up the types, conversion functions, and basic formatting. + #[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] + pub struct FeeRate(u64); + + impl FeeRate { + /// Constructs a new [`FeeRate`] from satoshis per 1,000,000 virtual bytes. + pub(crate) const fn from_sat_per_mvb(sat_mvb: u64) -> Self { Self(sat_mvb) } + + /// Converts to sat/MvB. + pub(crate) const fn to_sat_per_mvb(self) -> u64 { self.0 } + } +} +#[doc(inline)] +pub use encapsulate::FeeRate; impl FeeRate { /// 0 sat/kwu. @@ -40,9 +52,6 @@ impl FeeRate { /// Fee rate used to compute dust amount. pub const DUST: FeeRate = FeeRate::from_sat_per_vb_u32(3); - /// Constructs a new [`FeeRate`] from satoshis per 1,000,000 virtual bytes. - pub(crate) const fn from_sat_per_mvb(sat_mvb: u64) -> Self { Self(sat_mvb) } - /// Constructs a new [`FeeRate`] from satoshis per 1000 weight units. pub const fn from_sat_per_kwu(sat_kwu: u64) -> Option { // No `map()` in const context. @@ -80,9 +89,6 @@ impl FeeRate { } } - /// Converts to sat/MvB. - pub(crate) const fn to_sat_per_mvb(self) -> u64 { self.0 } - /// Converts to sat/kwu rounding down. pub const fn to_sat_per_kwu_floor(self) -> u64 { self.to_sat_per_mvb() / 4_000 } From 9b2fc021b21c44828e362b99fedf5654b2eeb785 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 26 May 2025 14:49:58 +1000 Subject: [PATCH 10/15] Improve rustdocs on FeeRate Remove the unit from associated consts and then make all the rustdocs on the various consts use 'The'. --- units/src/fee_rate/mod.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/units/src/fee_rate/mod.rs b/units/src/fee_rate/mod.rs index ceaf239f4..c2ee6f4b9 100644 --- a/units/src/fee_rate/mod.rs +++ b/units/src/fee_rate/mod.rs @@ -31,25 +31,25 @@ mod encapsulate { pub use encapsulate::FeeRate; impl FeeRate { - /// 0 sat/kwu. + /// The zero fee rate. /// /// Equivalent to [`MIN`](Self::MIN), may better express intent in some contexts. pub const ZERO: FeeRate = FeeRate::from_sat_per_mvb(0); - /// Minimum possible value (0 sat/kwu). + /// The minimum possible value. /// /// Equivalent to [`ZERO`](Self::ZERO), may better express intent in some contexts. pub const MIN: FeeRate = FeeRate::ZERO; - /// Maximum possible value. + /// The maximum possible value. pub const MAX: FeeRate = FeeRate::from_sat_per_mvb(u64::MAX); - /// Minimum fee rate required to broadcast a transaction. + /// 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); - /// Fee rate used to compute dust amount. + /// The fee rate used to compute dust amount. pub const DUST: FeeRate = FeeRate::from_sat_per_vb_u32(3); /// Constructs a new [`FeeRate`] from satoshis per 1000 weight units. From b65860067f7da1f4e665eb14d77adc6db9a3ba59 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 26 May 2025 15:23:44 +1000 Subject: [PATCH 11/15] Make fee functions const We temporarily removed `const` in the `fee` module to make patching and reviewing easier. Now add it back in. --- units/src/fee.rs | 46 ++++++++++++++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 16 deletions(-) diff --git a/units/src/fee.rs b/units/src/fee.rs index 73b4919aa..f8a693351 100644 --- a/units/src/fee.rs +++ b/units/src/fee.rs @@ -56,16 +56,21 @@ impl Amount { /// # Ok::<_, amount::OutOfRangeError>(()) /// ``` #[must_use] - pub fn checked_div_by_weight_ceil(self, weight: Weight) -> Option { + pub const fn checked_div_by_weight_ceil(self, weight: Weight) -> Option { let wu = weight.to_wu(); if wu == 0 { return None; } - let sats = self.to_sat() * 1_000; // Because we use per/kwu. - let fee_rate = (sats + wu - 1) / wu; - - FeeRate::from_sat_per_kwu(fee_rate) + // 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); + } + } + None } /// Checked fee rate floor division. @@ -76,7 +81,7 @@ impl Amount { /// /// Returns [`None`] if overflow occurred or if `fee_rate` is zero. #[must_use] - pub fn checked_div_by_fee_rate_floor(self, fee_rate: FeeRate) -> Option { + pub const fn checked_div_by_fee_rate_floor(self, fee_rate: FeeRate) -> Option { match self.to_sat().checked_mul(1000) { Some(amount_msats) => match amount_msats.checked_div(fee_rate.to_sat_per_kwu_ceil()) { Some(wu) => Some(Weight::from_wu(wu)), @@ -93,7 +98,7 @@ impl Amount { /// /// Returns [`None`] if overflow occurred or if `fee_rate` is zero. #[must_use] - pub fn checked_div_by_fee_rate_ceil(self, fee_rate: FeeRate) -> Option { + pub const fn checked_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(); match self.to_sat().checked_mul(1000) { @@ -122,8 +127,12 @@ impl FeeRate { /// 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) + pub const fn to_fee(self, weight: Weight) -> Amount { + // No `unwrap_or()` in const context. + match self.checked_mul_by_weight(weight) { + Some(fee) => fee, + None => Amount::MAX, + } } /// Calculates the fee by multiplying this fee rate by weight, in weight units, returning [`None`] @@ -150,13 +159,18 @@ impl FeeRate { /// enough instead of falling short if rounded down. /// /// Returns [`None`] if overflow occurred. - pub fn checked_mul_by_weight(self, weight: Weight) -> Option { + pub const 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() + 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); + } + } + } + None } } @@ -332,7 +346,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) -> Option { + pub const fn checked_mul_by_fee_rate(self, fee_rate: FeeRate) -> Option { fee_rate.checked_mul_by_weight(self) } } From bf0776e3dd8f457ec5c4b368d05ffb09d2cda20d Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 26 May 2025 07:37:51 +0100 Subject: [PATCH 12/15] Remove panic using checked arithmetic During dev I introduced a pancic, remove it. --- units/src/fee.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/units/src/fee.rs b/units/src/fee.rs index f8a693351..63a75cbfb 100644 --- a/units/src/fee.rs +++ b/units/src/fee.rs @@ -31,10 +31,14 @@ impl Amount { return None; } - let sats = self.to_sat() * 1_000; // Because we use per/kwu. - let fee_rate = sats / wu; - - FeeRate::from_sat_per_kwu(fee_rate) + // 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) + } + None => None, + } } /// Checked weight ceiling division. From 7c186e6081fbc6af1b7907cc06e769e3b174be69 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 26 May 2025 07:45:20 +0100 Subject: [PATCH 13/15] Refactor fee functions The `fee` functions are a bit convoluted because of the usage of `const`. Refactor a couple of them to be easier to read. Internal change only. --- units/src/fee.rs | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/units/src/fee.rs b/units/src/fee.rs index 63a75cbfb..09bccb741 100644 --- a/units/src/fee.rs +++ b/units/src/fee.rs @@ -86,13 +86,12 @@ 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 { - match self.to_sat().checked_mul(1000) { - Some(amount_msats) => match amount_msats.checked_div(fee_rate.to_sat_per_kwu_ceil()) { - Some(wu) => Some(Weight::from_wu(wu)), - None => None, - }, - None => None, + 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)); + } } + None } /// Checked fee rate ceiling division. @@ -105,19 +104,18 @@ impl Amount { pub const fn checked_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(); - match self.to_sat().checked_mul(1000) { - Some(amount_msats) => match rate.checked_sub(1) { - Some(rate_minus_one) => match amount_msats.checked_add(rate_minus_one) { - Some(rounded_msats) => match rounded_msats.checked_div(rate) { - Some(wu) => Some(Weight::from_wu(wu)), - None => None, - }, - None => None, - }, - None => None, - }, - None => None, + if rate == 0 { + return None; } + + 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) { + let wu = bump / rate; + return Some(Weight::from_wu(wu)); + } + } + None } } From 8cf1dc39b59cc6a6e98fdd50029e1b721536b66d Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 26 May 2025 15:30:54 +1000 Subject: [PATCH 14/15] Fix incorrect code comment --- bitcoin/src/blockdata/transaction.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bitcoin/src/blockdata/transaction.rs b/bitcoin/src/blockdata/transaction.rs index 4fcff528c..fc88001b1 100644 --- a/bitcoin/src/blockdata/transaction.rs +++ b/bitcoin/src/blockdata/transaction.rs @@ -1690,7 +1690,7 @@ mod tests { let fee_rate = FeeRate::from_sat_per_kwu(10).unwrap(); let effective_value = effective_value(fee_rate, InputWeightPrediction::P2WPKH_MAX, value); - // 10 sat/kwu * 272 wu = 4 sats (rounding up) + // 10 sat/kwu * 272 wu = 3 sats (rounding up) let expected_fee = "3 sats".parse::().unwrap(); let expected_effective_value = (value.to_signed() - expected_fee).unwrap(); assert_eq!(effective_value, expected_effective_value); From 56516757ad8874d8121dba468946546bb8fd7d4e Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 29 May 2025 05:08:34 +0100 Subject: [PATCH 15/15] Add code comment to amount calculation Its not immediately obvious that x - y cannot overflow. Add a code comment to explain it. --- bitcoin/src/blockdata/transaction.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bitcoin/src/blockdata/transaction.rs b/bitcoin/src/blockdata/transaction.rs index fc88001b1..6182308f5 100644 --- a/bitcoin/src/blockdata/transaction.rs +++ b/bitcoin/src/blockdata/transaction.rs @@ -784,6 +784,8 @@ pub fn effective_value( let weight = input_weight_prediction.total_weight(); let fee = fee_rate.to_fee(weight); + // Cannot overflow because after conversion to signed Amount::MIN - Amount::MAX + // still fits in SignedAmount::MAX (0 - MAX = -MAX). (value.to_signed() - fee.to_signed()).expect("cannot overflow") }