revert #4838 and #4847 using div_ceil
What the developer wrote
Authored by Andrew Poelstra
revert #4838 and #4847 using div_ceil
In #4838 we used saturating addition to simulate `div_ceil`, which gives
incorrect results on extreme values but at least doesn't panic. Similar
in the followup PR #4847. (These aren't detected by clippy, but I remember
them.)
What changed, and why it matters
This commit fixes a subtle arithmetic bug in how Bitcoin fee rates and transaction weights are rounded up. The previous code tried to avoid crashes on extremely large numbers by using 'saturating addition,' but that produced slightly wrong rounded-up results for the maximum possible values. The patch replaces that workaround with Rust's proper div_ceil function, which gives the mathematically correct ceiling division even at the extremes. In practice, this mostly affects edge-case calculations with maximum values and is unlikely to be directly exploitable for theft, but it removes a source of incorrect fee/weight estimates.
Review any downstream code that relies on to_sat_per_kwu_ceil, to_sat_per_vb_ceil, to_sat_per_kvb_ceil, Weight::to_kwu_ceil, or Weight::to_vbytes_ceil with maximum or near-maximum inputs. Update to this commit or a later release to ensure correct rounding. Consider adding property-based tests for u64 boundary values in these conversion functions.
Security signals we found
Incorrect ceiling division on extreme values due to saturating_add clamping
Reversion of prior defensive fix (#4838/#4847) that traded correctness for panic avoidance
Unit tests updated to assert corrected max-value behavior
No explicit security advisory or CVE referenced in commit
Evidence from the diff
The commit reverts two earlier PRs (#4838 and #4847) that implemented ceiling division via saturating_add(denominator - 1) / denominator. That idiom is incorrect when the numerator is near u64::MAX because saturating_add clamps to u64::MAX instead of overflowing, so the result is floor(max/denom) rather than ceil(max/denom). The patch uses Rust’s div_ceil, which correctly computes the ceiling for all u64 inputs. Updated unit tests confirm the corrected behavior for FeeRate::MAX and Weight::MAX. The change is in the units crate (fee_rate/mod.rs and weight.rs).
Changed components
units/src/fee_rate/mod.rsunits/src/weight.rsInspect captured patch +10 / −10
diff --git a/units/src/fee_rate/mod.rs b/units/src/fee_rate/mod.rs
index bcf690b7..5e202839 100644
--- a/units/src/fee_rate/mod.rs
+++ b/units/src/fee_rate/mod.rs
@@ -107,7 +107,7 @@ impl FeeRate {
/// Converts to sat/kwu rounding up.
pub const fn to_sat_per_kwu_ceil(self) -> u64 {
- self.to_sat_per_mvb().saturating_add(3_999) / 4_000
+ self.to_sat_per_mvb().div_ceil(4_000)
}
/// Converts to sat/vB rounding down.
@@ -115,7 +115,7 @@ impl FeeRate {
/// Converts to sat/vB rounding up.
pub const fn to_sat_per_vb_ceil(self) -> u64 {
- self.to_sat_per_mvb().saturating_add(999_999) / 1_000_000
+ self.to_sat_per_mvb().div_ceil(1_000_000)
}
/// Converts to sat/kvb rounding down.
@@ -123,7 +123,7 @@ impl FeeRate {
/// Converts to sat/kvb rounding up.
pub const fn to_sat_per_kvb_ceil(self) -> u64 {
- self.to_sat_per_mvb().saturating_add(999) / 1_000
+ self.to_sat_per_mvb().div_ceil(1_000)
}
/// Checked multiplication.
@@ -408,9 +408,9 @@ mod tests {
assert_eq!(fee_rate.to_sat_per_kvb_ceil(), 2_001);
let max = FeeRate::MAX;
- assert_eq!(max.to_sat_per_kwu_ceil(), u64::MAX / 4_000);
- assert_eq!(max.to_sat_per_vb_ceil(), u64::MAX / 1_000_000);
- assert_eq!(max.to_sat_per_kvb_ceil(), u64::MAX / 1_000);
+ assert_eq!(max.to_sat_per_kwu_ceil(), u64::MAX / 4_000 + 1);
+ assert_eq!(max.to_sat_per_vb_ceil(), u64::MAX / 1_000_000 + 1);
+ assert_eq!(max.to_sat_per_kvb_ceil(), u64::MAX / 1_000 + 1);
}
#[test]
diff --git a/units/src/weight.rs b/units/src/weight.rs
index 2c23c73e..14bf5e9a 100644
--- a/units/src/weight.rs
+++ b/units/src/weight.rs
@@ -107,14 +107,14 @@ impl Weight {
pub const fn to_kwu_floor(self) -> u64 { self.to_wu() / 1000 }
/// Converts to kilo weight units rounding up.
- pub const fn to_kwu_ceil(self) -> u64 { self.to_wu().saturating_add(999) / 1000 }
+ pub const fn to_kwu_ceil(self) -> u64 { self.to_wu().div_ceil(1_000) }
/// Converts to vB rounding down.
pub const fn to_vbytes_floor(self) -> u64 { self.to_wu() / Self::WITNESS_SCALE_FACTOR }
/// Converts to vB rounding up.
pub const fn to_vbytes_ceil(self) -> u64 {
- self.to_wu().saturating_add(Self::WITNESS_SCALE_FACTOR - 1) / Self::WITNESS_SCALE_FACTOR
+ self.to_wu().div_ceil(Self::WITNESS_SCALE_FACTOR)
}
/// Checked addition.
@@ -389,7 +389,7 @@ mod tests {
fn to_kwu_ceil() {
assert_eq!(Weight::from_wu(1_000).to_kwu_ceil(), 1);
assert_eq!(Weight::from_wu(1_001).to_kwu_ceil(), 2);
- assert_eq!(Weight::MAX.to_kwu_ceil(), u64::MAX / 1_000);
+ assert_eq!(Weight::MAX.to_kwu_ceil(), u64::MAX / 1_000 + 1);
}
#[test]
@@ -402,7 +402,7 @@ mod tests {
fn to_vb_ceil() {
assert_eq!(Weight::from_wu(4).to_vbytes_ceil(), 1);
assert_eq!(Weight::from_wu(5).to_vbytes_ceil(), 2);
- assert_eq!(Weight::MAX.to_vbytes_ceil(), u64::MAX / Weight::WITNESS_SCALE_FACTOR);
+ assert_eq!(Weight::MAX.to_vbytes_ceil(), u64::MAX / Weight::WITNESS_SCALE_FACTOR + 1);
}
#[test]
Why this scored 48/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.