Repository: rust-bitcoin/rust-bitcoin
Pull request #3587 (2024-11-06 17:15:20)

Split checked_div_by_weight into floor and ceiling version
closes https://github.com/rust-bitcoin/rust-bitcoin/issues/3563

github-actions[bot]: :rotating_light: API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.
coveralls: ## Pull Request Test Coverage Report for [Build 11708619686](https://coveralls.io/builds/70737540)


### Details

* **26** of **26**   **(100.0%)**  changed or added relevant lines in **2** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.01%**) to **82.399%**

---



|  Totals | [![Coverage Status](https://coveralls.io/builds/70737540/badge)](https://coveralls.io/builds/70737540) |
| :-- | --: |
| Change from base [Build 11707380848](https://coveralls.io/builds/70735815): |  0.01% |
| Covered Lines: | 19714 |
| Relevant Lines: | 23925 |

---
##### 💛  - [Coveralls](https://coveralls.io)

yancyribbens: `checked_mul_by_weight` was added recently enough I think that depreciation isn't required.
github-actions[bot]: :rotating_light: API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.
github-actions[bot]: :rotating_light: API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.
apoelstra: Afraid not, it's present in 0.32: https://docs.rs/bitcoin/latest/bitcoin/struct.FeeRate.html#method.checked_mul_by_weight

So we do need to deprecate.
github-actions[bot]: :rotating_light: API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.
github-actions[bot]: :rotating_light: API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.
yancyribbens: > Afraid not, it's present in 0.32: https://docs.rs/bitcoin/latest/bitcoin/struct.FeeRate.html#method.checked_mul_by_weight

Added back the original method and test with a deprecation notice.
apoelstra: >Added back the original method and test with a deprecation notice.

This method does not need to be deprecated. It does not appear in 0.32.
yancyribbens: > This method does not need to be deprecated. It does not appear in 0.32.

Oh I see what happened you had pointed to checked_mul_by_weight which I had accidentally titled this PR.  I'l undo that, sorry.

Patch:
diff --git a/bitcoin/src/blockdata/block.rs b/bitcoin/src/blockdata/block.rs
index 391b0992..e4614ae3 100644
--- a/bitcoin/src/blockdata/block.rs
+++ b/bitcoin/src/blockdata/block.rs
@@ -249,14 +249,20 @@ pub fn bip34_block_height(&self) -> Result<u64, Bip34Error> {
 
         let cb = self.coinbase().ok_or(Bip34Error::NotPresent)?;
         let input = cb.input.first().ok_or(Bip34Error::NotPresent)?;
-        let push = input.script_sig.instructions_minimal()
-            .next()
-            .ok_or(Bip34Error::NotPresent)?
-            .map_err(to_bip34_error)?;
-        match (push.script_num(), push.push_bytes().map(|b| b.read_scriptint())) {
-            (Some(num), Some(Ok(_)) | None) => Ok(num.try_into().map_err(|_| Bip34Error::NegativeHeight)?),
-            (_, Some(Err(err))) => Err(to_bip34_error(err)),
-            (None, _) => Err(Bip34Error::NotPresent),
+        let push = input.script_sig.instructions_minimal().next().ok_or(Bip34Error::NotPresent)?;
+        match push.map_err(|_| Bip34Error::NotPresent)? {
+            script::Instruction::PushBytes(b) => {
+                // Check that the number is encoded in the minimal way.
+                let h = b
+                    .read_scriptint()
+                    .map_err(|_e| Bip34Error::UnexpectedPush(b.as_bytes().to_vec()))?;
+                if h < 0 {
+                    Err(Bip34Error::NegativeHeight)
+                } else {
+                    Ok(h as u64)
+                }
+            }
+            _ => Err(Bip34Error::NotPresent),
         }
     }
 }
@@ -277,8 +283,8 @@ pub enum Bip34Error {
     Unsupported,
     /// No push was present where the BIP34 push was expected.
     NotPresent,
-    /// The BIP34 push was not minimally encoded.
-    NonMinimalPush,
+    /// The BIP34 push was larger than 8 bytes.
+    UnexpectedPush(Vec<u8>),
     /// The BIP34 push was negative.
     NegativeHeight,
 }
@@ -292,7 +298,9 @@ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match *self {
             Unsupported => write!(f, "block doesn't support BIP34"),
             NotPresent => write!(f, "BIP34 push not present in block's coinbase"),
-            NonMinimalPush => write!(f, "byte push not minimally encoded"),
+            UnexpectedPush(ref p) => {
+                write!(f, "unexpected byte push of > 8 bytes: {:?}", p)
+            }
             NegativeHeight => write!(f, "negative BIP34 height"),
         }
     }
@@ -304,19 +312,11 @@ fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
         use Bip34Error::*;
 
         match *self {
-            Unsupported | NotPresent | NonMinimalPush | NegativeHeight => None,
+            Unsupported | NotPresent | UnexpectedPush(_) | NegativeHeight => None,
         }
     }
 }
 
-#[inline]
-fn to_bip34_error(err: script::Error) -> Bip34Error {
-    match err {
-        script::Error::NonMinimalPush => Bip34Error::NonMinimalPush,
-        _ => Bip34Error::NotPresent,
-    }
-}
-
 /// A block validation error.
 #[derive(Debug, Clone, PartialEq, Eq)]
 #[non_exhaustive]
@@ -360,7 +360,7 @@ fn arbitrary(u: &mut Unstructured<'a>) -> arbitrary::Result<Self> {
 
 #[cfg(test)]
 mod tests {
-    use hex::test_hex_unwrap as hex;
+    use hex::{test_hex_unwrap as hex, FromHex};
     use internals::ToU64 as _;
 
     use super::*;
@@ -378,36 +378,12 @@ fn test_coinbase_and_bip34() {
 
         assert_eq!(block.bip34_block_height(), Ok(100_000));
 
-        // block with 3-byte bip34 push for height 0x03010000 (non-minimal 1)
-        const BAD_HEX: &str = "0200000035ab154183570282ce9afc0b494c9fc6a3cfea05aa8c1add2ecc56490000000038ba3d78e4500a5a7570dbe61960398add4410d278b21cd9708e6d9743f374d544fc055227f1001c29c1ea3b0101000000010000000000000000000000000000000000000000000000000000000000000000ffffffff3703010000000427f1001c046a510100522cfabe6d6d0000000000000000000068692066726f6d20706f6f6c7365727665726aac1eeeed88ffffffff0100f2052a010000001976a914912e2b234f941f30b18afbb4fa46171214bf66c888ac00000000";
+        // block with 9-byte bip34 push
+        const BAD_HEX: &str = "0200000035ab154183570282ce9afc0b494c9fc6a3cfea05aa8c1add2ecc56490000000038ba3d78e4500a5a7570dbe61960398add4410d278b21cd9708e6d9743f374d544fc055227f1001c29c1ea3b0101000000010000000000000000000000000000000000000000000000000000000000000000ffffffff3d09a08601112233445566000427f1001c046a510100522cfabe6d6d0000000000000000000068692066726f6d20706f6f6c7365727665726aac1eeeed88ffffffff0100f2052a010000001976a914912e2b234f941f30b18afbb4fa46171214bf66c888ac00000000";
         let bad: Block = deserialize(&hex!(BAD_HEX)).unwrap();
 
-        assert_eq!(bad.bip34_block_height(), Err(super::Bip34Error::NonMinimalPush));
-
-        // Block 15 on Testnet4 has height of 0x5f (15 PUSHNUM)
-        const BLOCK_HEX_SMALL_HEIGHT_15: &str = "000000200fd8c4c1e88f313b561b2724542ff9be1bc54a7dab8db8ef6359d48a00000000705bf9145e6d3c413702cc61f32e4e7bfe3117b1eb928071a59adcf75694a3fb07d83866ffff001dcf4c5e8401010000000001010000000000000000000000000000000000000000000000000000000000000000ffffffff095f00062f4077697a2fffffffff0200f2052a010000001976a9140a59837ccd4df25adc31cdad39be6a8d97557ed688ac0000000000000000266a24aa21a9ede2f61c3f71d1defd3fa999dfa36953755c690689799962b48bebd836974e8cf90120000000000000000000000000000000000000000000000000000000000000000000000000";
-        let block: Block = deserialize(&hex!(BLOCK_HEX_SMALL_HEIGHT_15)).unwrap();
-
-        assert_eq!(block.bip34_block_height(), Ok(15));
-
-        // Block 42 on Testnet4 has height of 0x012a (42)
-        const BLOCK_HEX_SMALL_HEIGHT_42: &str = "000000202803addb5a3f42f3e8d6c8536598b2d872b04f3b4f0698c26afdb17300000000463dd9a37a5d3d5c05f9c80a1485b41f1f513dee00338bbc33f5a6e836fce0345dda3866ffff001d872b9def01010000000001010000000000000000000000000000000000000000000000000000000000000000ffffffff09012a062f4077697a2fffffffff0200f2052a010000001976a9140a59837ccd4df25adc31cdad39be6a8d97557ed688ac0000000000000000266a24aa21a9ede2f61c3f71d1defd3fa999dfa36953755c690689799962b48bebd836974e8cf90120000000000000000000000000000000000000000000000000000000000000000000000000";
-        let block: Block = deserialize(&hex!(BLOCK_HEX_SMALL_HEIGHT_42)).unwrap();
-
-        assert_eq!(block.bip34_block_height(), Ok(42));
-
-        // Block 42 on Testnet4 using OP_PUSHDATA1 0x4c012a (42) instead of 0x012a (42)
-        const BLOCK_HEX_SMALL_HEIGHT_42_WRONG: &str = "000000202803addb5a3f42f3e8d6c8536598b2d872b04f3b4f0698c26afdb17300000000463dd9a37a5d3d5c05f9c80a1485b41f1f513dee00338bbc33f5a6e836fce0345dda3866ffff001d872b9def01010000000001010000000000000000000000000000000000000000000000000000000000000000ffffffff0a4c012a062f4077697a2fffffffff0200f2052a010000001976a9140a59837ccd4df25adc31cdad39be6a8d97557ed688ac0000000000000000266a24aa21a9ede2f61c3f71d1defd3fa999dfa36953755c690689799962b48bebd836974e8cf90120000000000000000000000000000000000000000000000000000000000000000000000000";
-        let block: Block = deserialize(&hex!(BLOCK_HEX_SMALL_HEIGHT_42_WRONG)).unwrap();
-
-        assert_eq!(block.bip34_block_height(), Err(super::Bip34Error::NonMinimalPush));
-
-        // Block with a 5 byte height properly minimally encoded
-        // this is an overflow for ScriptNum (i32) parsing
-        const BLOCK_HEX_5_BYTE_HEIGHT: &str = "000000202803addb5a3f42f3e8d6c8536598b2d872b04f3b4f0698c26afdb17300000000463dd9a37a5d3d5c05f9c80a1485b41f1f513dee00338bbc33f5a6e836fce0345dda3866ffff001d872b9def01010000000001010000000000000000000000000000000000000000000000000000000000000000ffffffff0d052a2a2a2a2a062f4077697a2fffffffff0200f2052a010000001976a9140a59837ccd4df25adc31cdad39be6a8d97557ed688ac0000000000000000266a24aa21a9ede2f61c3f71d1defd3fa999dfa36953755c690689799962b48bebd836974e8cf90120000000000000000000000000000000000000000000000000000000000000000000000000";
-        let block: Block = deserialize(&hex!(BLOCK_HEX_5_BYTE_HEIGHT)).unwrap();
-
-        assert_eq!(block.bip34_block_height(), Err(super::Bip34Error::NotPresent));
+        let push = Vec::<u8>::from_hex("a08601112233445566").unwrap();
+        assert_eq!(bad.bip34_block_height(), Err(super::Bip34Error::UnexpectedPush(push)));
     }
 
     #[test]
diff --git a/nightly-version b/nightly-version
index 176d23d4..ddde6232 100644
--- a/nightly-version
+++ b/nightly-version
@@ -1,1 +1,1 @@
-nightly-2024-11-06
+nightly-2024-11-03
diff --git a/units/src/amount/unsigned.rs b/units/src/amount/unsigned.rs
index 74628e31..28cbf235 100644
--- a/units/src/amount/unsigned.rs
+++ b/units/src/amount/unsigned.rs
@@ -231,22 +231,34 @@ pub fn checked_mul(self, rhs: u64) -> Option<Amount> { self.0.checked_mul(rhs).m
     /// Returns [`None`] if overflow occurred.
     pub fn checked_div(self, rhs: u64) -> Option<Amount> { self.0.checked_div(rhs).map(Amount) }
 
-    /// Checked weight division.
+    /// 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. If you wish to round down use `amount / weight`.
+    /// sufficient.  See also [`Amount::checked_div_by_weight_floor`].
     ///
     /// Returns [`None`] if overflow occurred.
     #[cfg(feature = "alloc")]
-    pub fn checked_div_by_weight(self, rhs: Weight) -> Option<FeeRate> {
-        let sats = self.0.checked_mul(1000)?;
+    pub fn checked_div_by_weight_ceil(self, rhs: Weight) -> Option<FeeRate> {
+        let sats = self.0.checked_mul(1_000)?;
         let wu = rhs.to_wu();
 
         let fee_rate = sats.checked_add(wu.checked_sub(1)?)?.checked_div(wu)?;
         Some(FeeRate::from_sat_per_kwu(fee_rate))
     }
 
+    /// Checked weight floor division.
+    ///
+    /// Be aware that integer division loses the remainder if no exact division
+    /// can be made.  See also [`Amount::checked_div_by_weight_ceil`].
+    ///
+    /// Returns [`None`] if overflow occurred.
+    #[cfg(feature = "alloc")]
+    pub fn checked_div_by_weight_floor(self, rhs: Weight) -> Option<FeeRate> {
+        let fee_rate = self.0.checked_mul(1_000)?.checked_div(rhs.to_wu())?;
+        Some(FeeRate::from_sat_per_kwu(fee_rate))
+    }
+
     /// Checked remainder.
     ///
     /// Returns [`None`] if overflow occurred.


Test patch:
diff --git a/units/src/amount/tests.rs b/units/src/amount/tests.rs
index 9c3e6bf9..e695a443 100644
--- a/units/src/amount/tests.rs
+++ b/units/src/amount/tests.rs
@@ -140,22 +140,43 @@ fn checked_arithmetic() {
 
 #[cfg(feature = "alloc")]
 #[test]
-fn amount_checked_div_by_weight() {
+fn amount_checked_div_by_weight_ceil() {
     let weight = Weight::from_kwu(1).unwrap();
-    let fee_rate = Amount::from_sat(1).checked_div_by_weight(weight).unwrap();
+    let fee_rate = Amount::from_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));
 
     let weight = Weight::from_wu(381);
-    let fee_rate = Amount::from_sat(329).checked_div_by_weight(weight).unwrap();
+    let fee_rate = Amount::from_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));
 
-    let fee_rate = Amount::MAX.checked_div_by_weight(weight);
+    let fee_rate = Amount::MAX.checked_div_by_weight_ceil(weight);
     assert!(fee_rate.is_none());
 
-    let fee_rate = Amount::ONE_SAT.checked_div_by_weight(Weight::ZERO);
+    let fee_rate = Amount::ONE_SAT.checked_div_by_weight_ceil(Weight::ZERO);
+    assert!(fee_rate.is_none());
+}
+
+#[cfg(feature = "alloc")]
+#[test]
+fn amount_checked_div_by_weight_floor() {
+    let weight = Weight::from_kwu(1).unwrap();
+    let fee_rate = Amount::from_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));
+
+    let weight = Weight::from_wu(381);
+    let fee_rate = Amount::from_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));
+
+    let fee_rate = Amount::MAX.checked_div_by_weight_floor(weight);
+    assert!(fee_rate.is_none());
+
+    let fee_rate = Amount::ONE_SAT.checked_div_by_weight_floor(Weight::ZERO);
     assert!(fee_rate.is_none());
 }