Merge rust-bitcoin#4616: units: Improve the fee calculation functions

0ff8d82 Make FeeRate from sat constructors infallible (Tobin C. Harding)
3b0286b Return NumOpResult for FeeRate and Weight (Tobin C. Harding)
15065b7 Return NumOpResult when calculating fee on Amount (Tobin C. Harding)
75106e6 Remove checked_ prefix from fee functions (Tobin C. Harding)

Pull request description:

  This was rust-bitcoin@14dc950 from rust-bitcoin#4610.

  Remove the `checked_` prefix from `fee` functions then make them all return `NumOpResult`.

ACKs for top commit:
  apoelstra:
    ACK 0ff8d82; successfully ran local tests; nice!

Tree-SHA512: 62f2af6701f2a0e17b9f0a0ee132ca4a9289fe00e22c94f746602ed3c1f3758bb22323b224362cb659432ff5297391f3106ee460e9a8f65a39f904d72bc98aeb

Author: apoelstra

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

bitcoin/src/blockdata/transaction.rs

@@ -1685,7 +1685,7 @@ mod tests {
     #[test]
     fn effective_value_happy_path() {
         let value = "1 cBTC".parse::<Amount>().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)

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`].
     ///
@@ -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<Psbt, crate::psbt::error::Error> {
         let r = Vec::from_hex(s);
@@ -1442,39 +1431,51 @@ 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(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)
-        );
-        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)
+            Err(error_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!(
             psbt_with_values(2076001, 1000).extract_tx().map_err(|e| match e {
                 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

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();

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]])
+}

units/src/amount/tests.rs

@@ -268,70 +268,70 @@ 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());
+    assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(1));
 
     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());
+    assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(864));
 
-    let fee_rate = Amount::ONE_SAT.checked_div_by_weight_ceil(Weight::ZERO);
-    assert!(fee_rate.is_none());
+    let fee_rate = Amount::ONE_SAT.div_by_weight_ceil(Weight::ZERO);
+    assert!(fee_rate.is_error());
 }
 
 #[cfg(feature = "alloc")]
 #[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());
+    assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(1));
 
     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());
+    assert_eq!(fee_rate, FeeRate::from_sat_per_kwu(863));
 
-    let fee_rate = Amount::ONE_SAT.checked_div_by_weight_floor(Weight::ZERO);
-    assert!(fee_rate.is_none());
+    let fee_rate = Amount::ONE_SAT.div_by_weight_floor(Weight::ZERO);
+    assert!(fee_rate.is_error());
 }
 
 #[cfg(feature = "alloc")]
 #[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.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 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();
-    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());
+    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 weight = max_amount.checked_div_by_fee_rate_floor(small_fee_rate).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));
 }