From bafff0bedbcf53a90f5585761432271adffad27f Mon Sep 17 00:00:00 2001 From: Tife Date: Sun, 5 Jun 2022 20:20:54 +0100 Subject: [PATCH 1/6] Removed multiply_by_rational Replaced with multiply_by_rational_with_rounding --- primitives/arithmetic/fuzzer/Cargo.toml | 4 +- ... => multiply_by_rational_with_rounding.rs} | 18 ++- primitives/arithmetic/src/fixed_point.rs | 51 ++++--- primitives/arithmetic/src/helpers_128bit.rs | 68 +-------- primitives/arithmetic/src/rational.rs | 129 ++++++++++++++---- primitives/npos-elections/src/phragmen.rs | 14 +- 6 files changed, 161 insertions(+), 123 deletions(-) rename primitives/arithmetic/fuzzer/src/{multiply_by_rational.rs => multiply_by_rational_with_rounding.rs} (77%) diff --git a/primitives/arithmetic/fuzzer/Cargo.toml b/primitives/arithmetic/fuzzer/Cargo.toml index 33bf313766545..990106f990323 100644 --- a/primitives/arithmetic/fuzzer/Cargo.toml +++ b/primitives/arithmetic/fuzzer/Cargo.toml @@ -32,8 +32,8 @@ name = "per_thing_rational" path = "src/per_thing_rational.rs" [[bin]] -name = "multiply_by_rational" -path = "src/multiply_by_rational.rs" +name = "multiply_by_rational_with_rounding" +path = "src/multiply_by_rational_with_rounding.rs" [[bin]] name = "fixed_point" diff --git a/primitives/arithmetic/fuzzer/src/multiply_by_rational.rs b/primitives/arithmetic/fuzzer/src/multiply_by_rational_with_rounding.rs similarity index 77% rename from primitives/arithmetic/fuzzer/src/multiply_by_rational.rs rename to primitives/arithmetic/fuzzer/src/multiply_by_rational_with_rounding.rs index 019cf0ec39b7d..fbb490997da62 100644 --- a/primitives/arithmetic/fuzzer/src/multiply_by_rational.rs +++ b/primitives/arithmetic/fuzzer/src/multiply_by_rational_with_rounding.rs @@ -16,19 +16,21 @@ // limitations under the License. //! # Running -//! Running this fuzzer can be done with `cargo hfuzz run multiply_by_rational`. `honggfuzz` CLI -//! options can be used by setting `HFUZZ_RUN_ARGS`, such as `-n 4` to use 4 threads. +//! Running this fuzzer can be done with `cargo hfuzz run multiply_by_rational_with_rounding`. +//! `honggfuzz` CLI options can be used by setting `HFUZZ_RUN_ARGS`, such as `-n 4` to use 4 +//! threads. //! //! # Debugging a panic //! Once a panic is found, it can be debugged with -//! `cargo hfuzz run-debug multiply_by_rational hfuzz_workspace/multiply_by_rational/*.fuzz`. +//! `cargo hfuzz run-debug multiply_by_rational_with_rounding +//! hfuzz_workspace/multiply_by_rational_with_rounding/*.fuzz`. //! //! # More information //! More information about `honggfuzz` can be found //! [here](https://docs.rs/honggfuzz/). use honggfuzz::fuzz; -use sp_arithmetic::{helpers_128bit::multiply_by_rational, traits::Zero}; +use sp_arithmetic::{helpers_128bit::multiply_by_rational_with_rounding, traits::Zero, Rounding}; fn main() { loop { @@ -42,9 +44,11 @@ fn main() { println!("++ Equation: {} * {} / {}", a, b, c); - // The point of this fuzzing is to make sure that `multiply_by_rational` is 100% - // accurate as long as the value fits in a u128. - if let Ok(result) = multiply_by_rational(a, b, c) { + // The point of this fuzzing is to make sure that `multiply_by_rational_with_rounding` + // is 100% accurate as long as the value fits in a u128. + if let Some(result) = + multiply_by_rational_with_rounding(a, b, c, Rounding::NearestPrefDown) + { let truth = mul_div(a, b, c); if result != truth && result != truth + 1 { diff --git a/primitives/arithmetic/src/fixed_point.rs b/primitives/arithmetic/src/fixed_point.rs index f4ed70ee8b5dc..9fdb7625710b1 100644 --- a/primitives/arithmetic/src/fixed_point.rs +++ b/primitives/arithmetic/src/fixed_point.rs @@ -18,7 +18,7 @@ //! Decimal Fixed Point implementations for Substrate runtime. use crate::{ - helpers_128bit::{multiply_by_rational, multiply_by_rational_with_rounding, sqrt}, + helpers_128bit::{multiply_by_rational_with_rounding, sqrt}, traits::{ Bounded, CheckedAdd, CheckedDiv, CheckedMul, CheckedNeg, CheckedSub, One, SaturatedConversion, Saturating, UniqueSaturatedInto, Zero, @@ -151,10 +151,14 @@ pub trait FixedPointNumber: let d: I129 = d.into(); let negative = n.negative != d.negative; - multiply_by_rational(n.value, Self::DIV.unique_saturated_into(), d.value) - .ok() - .and_then(|value| from_i129(I129 { value, negative })) - .map(Self::from_inner) + multiply_by_rational_with_rounding( + n.value, + Self::DIV.unique_saturated_into(), + d.value, + Rounding::from_signed(SignedRounding::Minor, negative), + ) + .and_then(|value| from_i129(I129 { value, negative })) + .map(Self::from_inner) } /// Checked multiplication for integer type `N`. Equal to `self * n`. @@ -165,9 +169,14 @@ pub trait FixedPointNumber: let rhs: I129 = n.into(); let negative = lhs.negative != rhs.negative; - multiply_by_rational(lhs.value, rhs.value, Self::DIV.unique_saturated_into()) - .ok() - .and_then(|value| from_i129(I129 { value, negative })) + multiply_by_rational_with_rounding( + lhs.value, + rhs.value, + Self::DIV.unique_saturated_into(), + Rounding::from_signed(SignedRounding::Minor, negative), + ) + .ok() + .and_then(|value| from_i129(I129 { value, negative })) } /// Saturating multiplication for integer type `N`. Equal to `self * n`. @@ -832,10 +841,15 @@ macro_rules! implement_fixed { // is equivalent to the `SignedRounding::NearestPrefMinor`. This means it is // expected to give exactly the same result as `const_checked_div` when the result // is positive and a result up to one epsilon greater when it is negative. - multiply_by_rational(lhs.value, Self::DIV as u128, rhs.value) - .ok() - .and_then(|value| from_i129(I129 { value, negative })) - .map(Self) + multiply_by_rational_with_rounding( + lhs.value, + Self::DIV as u128, + rhs.value, + Rounding::from_signed(SignedRounding::Minor, negative), + ) + .ok() + .and_then(|value| from_i129(I129 { value, negative })) + .map(Self) } } @@ -845,10 +859,15 @@ macro_rules! implement_fixed { let rhs: I129 = other.0.into(); let negative = lhs.negative != rhs.negative; - multiply_by_rational(lhs.value, rhs.value, Self::DIV as u128) - .ok() - .and_then(|value| from_i129(I129 { value, negative })) - .map(Self) + multiply_by_rational_with_rounding( + lhs.value, + rhs.value, + Self::DIV as u128, + Rounding::from_signed(SignedRounding::Minor, negative), + ) + .ok() + .and_then(|value| from_i129(I129 { value, negative })) + .map(Self) } } diff --git a/primitives/arithmetic/src/helpers_128bit.rs b/primitives/arithmetic/src/helpers_128bit.rs index 11df8bd53153e..fcecc0e8478d0 100644 --- a/primitives/arithmetic/src/helpers_128bit.rs +++ b/primitives/arithmetic/src/helpers_128bit.rs @@ -22,12 +22,7 @@ //! multiplication implementation provided there. use crate::{biguint, Rounding}; -use num_traits::Zero; -use sp_std::{ - cmp::{max, min}, - convert::TryInto, - mem, -}; +use sp_std::cmp::{max, min}; /// Helper gcd function used in Rational128 implementation. pub fn gcd(a: u128, b: u128) -> u128 { @@ -61,65 +56,6 @@ pub fn to_big_uint(x: u128) -> biguint::BigUint { n } -/// Safely and accurately compute `a * b / c`. The approach is: -/// - Simply try `a * b / c`. -/// - Else, convert them both into big numbers and re-try. `Err` is returned if the result cannot -/// be safely casted back to u128. -/// -/// Invariant: c must be greater than or equal to 1. -pub fn multiply_by_rational(mut a: u128, mut b: u128, mut c: u128) -> Result { - if a.is_zero() || b.is_zero() { - return Ok(Zero::zero()) - } - c = c.max(1); - - // a and b are interchangeable by definition in this function. It always helps to assume the - // bigger of which is being multiplied by a `0 < b/c < 1`. Hence, a should be the bigger and - // b the smaller one. - if b > a { - mem::swap(&mut a, &mut b); - } - - // Attempt to perform the division first - if a % c == 0 { - a /= c; - c = 1; - } else if b % c == 0 { - b /= c; - c = 1; - } - - if let Some(x) = a.checked_mul(b) { - // This is the safest way to go. Try it. - Ok(x / c) - } else { - let a_num = to_big_uint(a); - let b_num = to_big_uint(b); - let c_num = to_big_uint(c); - - let mut ab = a_num * b_num; - ab.lstrip(); - let mut q = if c_num.len() == 1 { - // PROOF: if `c_num.len() == 1` then `c` fits in one limb. - ab.div_unit(c as biguint::Single) - } else { - // PROOF: both `ab` and `c` cannot have leading zero limbs; if length of `c` is 1, - // the previous branch would handle. Also, if ab for sure has a bigger size than - // c, because `a.checked_mul(b)` has failed, hence ab must be at least one limb - // bigger than c. In this case, returning zero is defensive-only and div should - // always return Some. - let (mut q, r) = ab.div(&c_num, true).unwrap_or((Zero::zero(), Zero::zero())); - let r: u128 = r.try_into().expect("reminder of div by c is always less than c; qed"); - if r > (c / 2) { - q = q.add(&to_big_uint(1)); - } - q - }; - q.lstrip(); - q.try_into().map_err(|_| "result cannot fit in u128") - } -} - mod double128 { // Inspired by: https://medium.com/wicketh/mathemagic-512-bit-division-in-solidity-afa55870a65 @@ -361,7 +297,7 @@ mod tests { let b = random_u128(i + (1 << 30)); let c = random_u128(i + (1 << 31)); let x = mulrat(a, b, c, NearestPrefDown); - let y = multiply_by_rational(a, b, c).ok(); + let y = multiply_by_rational_with_rounding(a, b, c).ok(); assert_eq!(x.is_some(), y.is_some()); let x = x.unwrap_or(0); let y = y.unwrap_or(0); diff --git a/primitives/arithmetic/src/rational.rs b/primitives/arithmetic/src/rational.rs index 1beafbe811614..d4a56fa7c8a12 100644 --- a/primitives/arithmetic/src/rational.rs +++ b/primitives/arithmetic/src/rational.rs @@ -15,7 +15,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use crate::{biguint::BigUint, helpers_128bit}; +use crate::{biguint::BigUint, helpers_128bit, Rounding}; use num_traits::{Bounded, One, Zero}; use sp_std::{cmp::Ordering, prelude::*}; @@ -149,7 +149,13 @@ impl Rational128 { if den == self.1 { Ok(self) } else { - helpers_128bit::multiply_by_rational(self.0, den, self.1).map(|n| Self(n, den)) + helpers_128bit::multiply_by_rational_with_rounding( + self.0, + den, + self.1, + Rounding::NearestPrefDown, + ) + .map(|n| Self(n, den)) } } @@ -157,13 +163,18 @@ impl Rational128 { /// /// This only returns if the result is accurate. `Err` is returned if the result cannot be /// accurately calculated. - pub fn lcm(&self, other: &Self) -> Result { + pub fn lcm(&self, other: &Self) -> Option { // this should be tested better: two large numbers that are almost the same. if self.1 == other.1 { - return Ok(self.1) + return Some(self.1) } let g = helpers_128bit::gcd(self.1, other.1); - helpers_128bit::multiply_by_rational(self.1, other.1, g) + helpers_128bit::multiply_by_rational_with_rounding( + self.1, + other.1, + g, + Rounding::NearestPrefDown, + ) } /// A saturating add that assumes `self` and `other` have the same denominator. @@ -408,52 +419,107 @@ mod tests { } #[test] - fn multiply_by_rational_works() { - assert_eq!(multiply_by_rational(7, 2, 3).unwrap(), 7 * 2 / 3); - assert_eq!(multiply_by_rational(7, 20, 30).unwrap(), 7 * 2 / 3); - assert_eq!(multiply_by_rational(20, 7, 30).unwrap(), 7 * 2 / 3); + fn multiply_by_rational_with_rounding_works() { + assert_eq!( + multiply_by_rational_with_rounding(7, 2, 3, Rounding::NearestPrefDown).unwrap(), + 7 * 2 / 3 + ); + assert_eq!( + multiply_by_rational_with_rounding(7, 20, 30, Rounding::NearestPrefDown).unwrap(), + 7 * 2 / 3 + ); + assert_eq!( + multiply_by_rational_with_rounding(20, 7, 30, Rounding::NearestPrefDown).unwrap(), + 7 * 2 / 3 + ); assert_eq!( // MAX128 % 3 == 0 - multiply_by_rational(MAX128, 2, 3).unwrap(), + multiply_by_rational_with_rounding(MAX128, 2, 3, Rounding::NearestPrefDown).unwrap(), MAX128 / 3 * 2, ); assert_eq!( // MAX128 % 7 == 3 - multiply_by_rational(MAX128, 5, 7).unwrap(), + multiply_by_rational_with_rounding(MAX128, 5, 7, Rounding::NearestPrefDown).unwrap(), (MAX128 / 7 * 5) + (3 * 5 / 7), ); assert_eq!( // MAX128 % 7 == 3 - multiply_by_rational(MAX128, 11, 13).unwrap(), + multiply_by_rational_with_rounding(MAX128, 11, 13, Rounding::NearestPrefDown).unwrap(), (MAX128 / 13 * 11) + (8 * 11 / 13), ); assert_eq!( // MAX128 % 1000 == 455 - multiply_by_rational(MAX128, 555, 1000).unwrap(), + multiply_by_rational_with_rounding(MAX128, 555, 1000, Rounding::NearestPrefDown) + .unwrap(), (MAX128 / 1000 * 555) + (455 * 555 / 1000), ); - assert_eq!(multiply_by_rational(2 * MAX64 - 1, MAX64, MAX64).unwrap(), 2 * MAX64 - 1); - assert_eq!(multiply_by_rational(2 * MAX64 - 1, MAX64 - 1, MAX64).unwrap(), 2 * MAX64 - 3); + assert_eq!( + multiply_by_rational_with_rounding( + 2 * MAX64 - 1, + MAX64, + MAX64, + Rounding::NearestPrefDown + ) + .unwrap(), + 2 * MAX64 - 1 + ); + assert_eq!( + multiply_by_rational_with_rounding( + 2 * MAX64 - 1, + MAX64 - 1, + MAX64, + Rounding::NearestPrefDown + ) + .unwrap(), + 2 * MAX64 - 3 + ); assert_eq!( - multiply_by_rational(MAX64 + 100, MAX64_2, MAX64_2 / 2).unwrap(), + multiply_by_rational_with_rounding( + MAX64 + 100, + MAX64_2, + MAX64_2 / 2, + Rounding::NearestPrefDown + ) + .unwrap(), (MAX64 + 100) * 2, ); assert_eq!( - multiply_by_rational(MAX64 + 100, MAX64_2 / 100, MAX64_2 / 200).unwrap(), + multiply_by_rational_with_rounding( + MAX64 + 100, + MAX64_2 / 100, + MAX64_2 / 200, + Rounding::NearestPrefDown + ) + .unwrap(), (MAX64 + 100) * 2, ); assert_eq!( - multiply_by_rational(2u128.pow(66) - 1, 2u128.pow(65) - 1, 2u128.pow(65)).unwrap(), + multiply_by_rational_with_rounding( + 2u128.pow(66) - 1, + 2u128.pow(65) - 1, + 2u128.pow(65), + Rounding::NearestPrefDown + ) + .unwrap(), 73786976294838206461, ); - assert_eq!(multiply_by_rational(1_000_000_000, MAX128 / 8, MAX128 / 2).unwrap(), 250000000); + assert_eq!( + multiply_by_rational_with_rounding( + 1_000_000_000, + MAX128 / 8, + MAX128 / 2, + Rounding::NearestPrefDown + ) + .unwrap(), + 250000000 + ); assert_eq!( - multiply_by_rational( + multiply_by_rational_with_rounding( 29459999999999999988000u128, 1000000000000000000u128, 10000000000000000000u128 @@ -464,17 +530,28 @@ mod tests { } #[test] - fn multiply_by_rational_a_b_are_interchangeable() { - assert_eq!(multiply_by_rational(10, MAX128, MAX128 / 2), Ok(20)); - assert_eq!(multiply_by_rational(MAX128, 10, MAX128 / 2), Ok(20)); + fn multiply_by_rational_with_rounding_a_b_are_interchangeable() { + assert_eq!( + multiply_by_rational_with_rounding(10, MAX128, MAX128 / 2, Rounding::NearestPrefDown), + Some(20) + ); + assert_eq!( + multiply_by_rational_with_rounding(MAX128, 10, MAX128 / 2, Rounding::NearestPrefDown), + Some(20) + ); } #[test] #[ignore] - fn multiply_by_rational_fuzzed_equation() { + fn multiply_by_rational_with_rounding_fuzzed_equation() { assert_eq!( - multiply_by_rational(154742576605164960401588224, 9223376310179529214, 549756068598), - Ok(2596149632101417846585204209223679) + multiply_by_rational_with_rounding( + 154742576605164960401588224, + 9223376310179529214, + 549756068598, + Rounding::NearestPrefDown + ), + Some(2596149632101417846585204209223679) ); } } diff --git a/primitives/npos-elections/src/phragmen.rs b/primitives/npos-elections/src/phragmen.rs index 9b0bfa42215c3..632d75d3b136b 100644 --- a/primitives/npos-elections/src/phragmen.rs +++ b/primitives/npos-elections/src/phragmen.rs @@ -25,9 +25,9 @@ use crate::{ PerThing128, VoteWeight, Voter, }; use sp_arithmetic::{ - helpers_128bit::multiply_by_rational, + helpers_128bit::multiply_by_rational_with_rounding, traits::{Bounded, Zero}, - Rational128, + Rational128, Rounding, }; use sp_std::prelude::*; @@ -143,10 +143,11 @@ pub fn seq_phragmen_core( for edge in &voter.edges { let mut candidate = edge.candidate.borrow_mut(); if !candidate.elected && !candidate.approval_stake.is_zero() { - let temp_n = multiply_by_rational( + let temp_n = multiply_by_rational_with_rounding( voter.load.n(), voter.budget, candidate.approval_stake, + Rounding::NearestPrefDown, ) .unwrap_or(Bounded::max_value()); let temp_d = voter.load.d(); @@ -184,9 +185,10 @@ pub fn seq_phragmen_core( for edge in &mut voter.edges { if edge.candidate.borrow().elected { // update internal state. - edge.weight = multiply_by_rational(voter.budget, edge.load.n(), voter.load.n()) - // If result cannot fit in u128. Not much we can do about it. - .unwrap_or(Bounded::max_value()); + edge.weight = + multiply_by_rational_with_rounding(voter.budget, edge.load.n(), voter.load.n()) + // If result cannot fit in u128. Not much we can do about it. + .unwrap_or(Bounded::max_value()); } else { edge.weight = 0 } From 86e61694f8e97725cf16ed81bde073804d244ca6 Mon Sep 17 00:00:00 2001 From: Tife Date: Sun, 5 Jun 2022 20:55:08 +0100 Subject: [PATCH 2/6] fixes --- primitives/arithmetic/src/fixed_point.rs | 3 --- primitives/arithmetic/src/rational.rs | 27 ++++++++++++++---------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/primitives/arithmetic/src/fixed_point.rs b/primitives/arithmetic/src/fixed_point.rs index 9fdb7625710b1..7f588e95c6733 100644 --- a/primitives/arithmetic/src/fixed_point.rs +++ b/primitives/arithmetic/src/fixed_point.rs @@ -175,7 +175,6 @@ pub trait FixedPointNumber: Self::DIV.unique_saturated_into(), Rounding::from_signed(SignedRounding::Minor, negative), ) - .ok() .and_then(|value| from_i129(I129 { value, negative })) } @@ -847,7 +846,6 @@ macro_rules! implement_fixed { rhs.value, Rounding::from_signed(SignedRounding::Minor, negative), ) - .ok() .and_then(|value| from_i129(I129 { value, negative })) .map(Self) } @@ -865,7 +863,6 @@ macro_rules! implement_fixed { Self::DIV as u128, Rounding::from_signed(SignedRounding::Minor, negative), ) - .ok() .and_then(|value| from_i129(I129 { value, negative })) .map(Self) } diff --git a/primitives/arithmetic/src/rational.rs b/primitives/arithmetic/src/rational.rs index d4a56fa7c8a12..d8946681d8d17 100644 --- a/primitives/arithmetic/src/rational.rs +++ b/primitives/arithmetic/src/rational.rs @@ -143,11 +143,11 @@ impl Rational128 { /// Convert `self` to a similar rational number where denominator is the given `den`. // - /// This only returns if the result is accurate. `Err` is returned if the result cannot be + /// This only returns if the result is accurate. `None` is returned if the result cannot be /// accurately calculated. - pub fn to_den(self, den: u128) -> Result { + pub fn to_den(self, den: u128) -> Option { if den == self.1 { - Ok(self) + Some(self) } else { helpers_128bit::multiply_by_rational_with_rounding( self.0, @@ -161,7 +161,7 @@ impl Rational128 { /// Get the least common divisor of `self` and `other`. /// - /// This only returns if the result is accurate. `Err` is returned if the result cannot be + /// This only returns if the result is accurate. `None` is returned if the result cannot be /// accurately calculated. pub fn lcm(&self, other: &Self) -> Option { // this should be tested better: two large numbers that are almost the same. @@ -199,9 +199,11 @@ impl Rational128 { /// /// Overflow might happen during any of the steps. Error is returned in such cases. pub fn checked_add(self, other: Self) -> Result { - let lcm = self.lcm(&other).map_err(|_| "failed to scale to denominator")?; - let self_scaled = self.to_den(lcm).map_err(|_| "failed to scale to denominator")?; - let other_scaled = other.to_den(lcm).map_err(|_| "failed to scale to denominator")?; + let lcm = self.lcm(&other).ok_or(0).map_err(|_| "failed to scale to denominator")?; + let self_scaled = + self.to_den(lcm).ok_or(0).map_err(|_| "failed to scale to denominator")?; + let other_scaled = + other.to_den(lcm).ok_or(0).map_err(|_| "failed to scale to denominator")?; let n = self_scaled .0 .checked_add(other_scaled.0) @@ -213,9 +215,11 @@ impl Rational128 { /// /// Overflow might happen during any of the steps. None is returned in such cases. pub fn checked_sub(self, other: Self) -> Result { - let lcm = self.lcm(&other).map_err(|_| "failed to scale to denominator")?; - let self_scaled = self.to_den(lcm).map_err(|_| "failed to scale to denominator")?; - let other_scaled = other.to_den(lcm).map_err(|_| "failed to scale to denominator")?; + let lcm = self.lcm(&other).ok_or(0).map_err(|_| "failed to scale to denominator")?; + let self_scaled = + self.to_den(lcm).ok_or(0).map_err(|_| "failed to scale to denominator")?; + let other_scaled = + other.to_den(lcm).ok_or(0).map_err(|_| "failed to scale to denominator")?; let n = self_scaled .0 @@ -522,7 +526,8 @@ mod tests { multiply_by_rational_with_rounding( 29459999999999999988000u128, 1000000000000000000u128, - 10000000000000000000u128 + 10000000000000000000u128, + Rounding::NearestPrefDown ) .unwrap(), 2945999999999999998800u128 From 12e7d23d138a7c2e1db74906a3276a5a0603f2e5 Mon Sep 17 00:00:00 2001 From: Tife Date: Sun, 5 Jun 2022 21:27:55 +0100 Subject: [PATCH 3/6] Test Fixes --- primitives/arithmetic/src/helpers_128bit.rs | 2 +- primitives/arithmetic/src/rational.rs | 44 ++++++++++----------- primitives/npos-elections/src/phragmen.rs | 2 +- 3 files changed, 24 insertions(+), 24 deletions(-) diff --git a/primitives/arithmetic/src/helpers_128bit.rs b/primitives/arithmetic/src/helpers_128bit.rs index fcecc0e8478d0..cc2bff8c0b51e 100644 --- a/primitives/arithmetic/src/helpers_128bit.rs +++ b/primitives/arithmetic/src/helpers_128bit.rs @@ -297,7 +297,7 @@ mod tests { let b = random_u128(i + (1 << 30)); let c = random_u128(i + (1 << 31)); let x = mulrat(a, b, c, NearestPrefDown); - let y = multiply_by_rational_with_rounding(a, b, c).ok(); + let y = multiply_by_rational_with_rounding(a, b, c, Rounding::NearestPrefDown); assert_eq!(x.is_some(), y.is_some()); let x = x.unwrap_or(0); let y = y.unwrap_or(0); diff --git a/primitives/arithmetic/src/rational.rs b/primitives/arithmetic/src/rational.rs index d8946681d8d17..e1f9959ae3558 100644 --- a/primitives/arithmetic/src/rational.rs +++ b/primitives/arithmetic/src/rational.rs @@ -329,18 +329,18 @@ mod tests { #[test] fn to_denom_works() { // simple up and down - assert_eq!(r(1, 5).to_den(10), Ok(r(2, 10))); - assert_eq!(r(4, 10).to_den(5), Ok(r(2, 5))); + assert_eq!(r(1, 5).to_den(10), Some(r(2, 10))); + assert_eq!(r(4, 10).to_den(5), Some(r(2, 5))); // up and down with large numbers - assert_eq!(r(MAX128 - 10, MAX128).to_den(10), Ok(r(10, 10))); - assert_eq!(r(MAX128 / 2, MAX128).to_den(10), Ok(r(5, 10))); + assert_eq!(r(MAX128 - 10, MAX128).to_den(10), Some(r(10, 10))); + assert_eq!(r(MAX128 / 2, MAX128).to_den(10), Some(r(5, 10))); // large to perbill. This is very well needed for npos-elections. - assert_eq!(r(MAX128 / 2, MAX128).to_den(1000_000_000), Ok(r(500_000_000, 1000_000_000))); + assert_eq!(r(MAX128 / 2, MAX128).to_den(1000_000_000), Some(r(500_000_000, 1000_000_000))); // large to large - assert_eq!(r(MAX128 / 2, MAX128).to_den(MAX128 / 2), Ok(r(MAX128 / 4, MAX128 / 2))); + assert_eq!(r(MAX128 / 2, MAX128).to_den(MAX128 / 2), Some(r(MAX128 / 4, MAX128 / 2))); } #[test] @@ -359,11 +359,11 @@ mod tests { // large numbers assert_eq!( r(1_000_000_000, MAX128).lcm(&r(7_000_000_000, MAX128 - 1)), - Err("result cannot fit in u128"), + None, ); assert_eq!( r(1_000_000_000, MAX64).lcm(&r(7_000_000_000, MAX64 - 1)), - Ok(340282366920938463408034375210639556610), + Some(340282366920938463408034375210639556610), ); const_assert!(340282366920938463408034375210639556610 < MAX128); const_assert!(340282366920938463408034375210639556610 == MAX64 * (MAX64 - 1)); @@ -425,36 +425,36 @@ mod tests { #[test] fn multiply_by_rational_with_rounding_works() { assert_eq!( - multiply_by_rational_with_rounding(7, 2, 3, Rounding::NearestPrefDown).unwrap(), + multiply_by_rational_with_rounding(7, 2, 3, Rounding::Down).unwrap(), 7 * 2 / 3 ); assert_eq!( - multiply_by_rational_with_rounding(7, 20, 30, Rounding::NearestPrefDown).unwrap(), + multiply_by_rational_with_rounding(7, 20, 30, Rounding::Down).unwrap(), 7 * 2 / 3 ); assert_eq!( - multiply_by_rational_with_rounding(20, 7, 30, Rounding::NearestPrefDown).unwrap(), + multiply_by_rational_with_rounding(20, 7, 30, Rounding::Down).unwrap(), 7 * 2 / 3 ); assert_eq!( // MAX128 % 3 == 0 - multiply_by_rational_with_rounding(MAX128, 2, 3, Rounding::NearestPrefDown).unwrap(), + multiply_by_rational_with_rounding(MAX128, 2, 3, Rounding::Down).unwrap(), MAX128 / 3 * 2, ); assert_eq!( // MAX128 % 7 == 3 - multiply_by_rational_with_rounding(MAX128, 5, 7, Rounding::NearestPrefDown).unwrap(), + multiply_by_rational_with_rounding(MAX128, 5, 7, Rounding::Down).unwrap(), (MAX128 / 7 * 5) + (3 * 5 / 7), ); assert_eq!( // MAX128 % 7 == 3 - multiply_by_rational_with_rounding(MAX128, 11, 13, Rounding::NearestPrefDown).unwrap(), + multiply_by_rational_with_rounding(MAX128, 11, 13, Rounding::Down).unwrap(), (MAX128 / 13 * 11) + (8 * 11 / 13), ); assert_eq!( // MAX128 % 1000 == 455 - multiply_by_rational_with_rounding(MAX128, 555, 1000, Rounding::NearestPrefDown) + multiply_by_rational_with_rounding(MAX128, 555, 1000, Rounding::Down) .unwrap(), (MAX128 / 1000 * 555) + (455 * 555 / 1000), ); @@ -464,7 +464,7 @@ mod tests { 2 * MAX64 - 1, MAX64, MAX64, - Rounding::NearestPrefDown + Rounding::Down ) .unwrap(), 2 * MAX64 - 1 @@ -474,7 +474,7 @@ mod tests { 2 * MAX64 - 1, MAX64 - 1, MAX64, - Rounding::NearestPrefDown + Rounding::Down ) .unwrap(), 2 * MAX64 - 3 @@ -485,7 +485,7 @@ mod tests { MAX64 + 100, MAX64_2, MAX64_2 / 2, - Rounding::NearestPrefDown + Rounding::Down ) .unwrap(), (MAX64 + 100) * 2, @@ -495,7 +495,7 @@ mod tests { MAX64 + 100, MAX64_2 / 100, MAX64_2 / 200, - Rounding::NearestPrefDown + Rounding::Down ) .unwrap(), (MAX64 + 100) * 2, @@ -506,7 +506,7 @@ mod tests { 2u128.pow(66) - 1, 2u128.pow(65) - 1, 2u128.pow(65), - Rounding::NearestPrefDown + Rounding::Down ) .unwrap(), 73786976294838206461, @@ -516,7 +516,7 @@ mod tests { 1_000_000_000, MAX128 / 8, MAX128 / 2, - Rounding::NearestPrefDown + Rounding::Up ) .unwrap(), 250000000 @@ -527,7 +527,7 @@ mod tests { 29459999999999999988000u128, 1000000000000000000u128, 10000000000000000000u128, - Rounding::NearestPrefDown + Rounding::Down ) .unwrap(), 2945999999999999998800u128 diff --git a/primitives/npos-elections/src/phragmen.rs b/primitives/npos-elections/src/phragmen.rs index 632d75d3b136b..bd5829dc935e5 100644 --- a/primitives/npos-elections/src/phragmen.rs +++ b/primitives/npos-elections/src/phragmen.rs @@ -186,7 +186,7 @@ pub fn seq_phragmen_core( if edge.candidate.borrow().elected { // update internal state. edge.weight = - multiply_by_rational_with_rounding(voter.budget, edge.load.n(), voter.load.n()) + multiply_by_rational_with_rounding(voter.budget, edge.load.n(), voter.load.n(), Rounding::Down) // If result cannot fit in u128. Not much we can do about it. .unwrap_or(Bounded::max_value()); } else { From ddcf12bc28c50399cea86b45f54cfd4b83999d7a Mon Sep 17 00:00:00 2001 From: Tife Date: Sun, 5 Jun 2022 21:35:38 +0100 Subject: [PATCH 4/6] nightly fmt --- primitives/arithmetic/src/rational.rs | 49 +++++------------------ primitives/npos-elections/src/phragmen.rs | 12 ++++-- 2 files changed, 19 insertions(+), 42 deletions(-) diff --git a/primitives/arithmetic/src/rational.rs b/primitives/arithmetic/src/rational.rs index e1f9959ae3558..54cabfc6214e8 100644 --- a/primitives/arithmetic/src/rational.rs +++ b/primitives/arithmetic/src/rational.rs @@ -357,10 +357,7 @@ mod tests { assert_eq!(r(5, 30).lcm(&r(1, 10)).unwrap(), 30); // large numbers - assert_eq!( - r(1_000_000_000, MAX128).lcm(&r(7_000_000_000, MAX128 - 1)), - None, - ); + assert_eq!(r(1_000_000_000, MAX128).lcm(&r(7_000_000_000, MAX128 - 1)), None,); assert_eq!( r(1_000_000_000, MAX64).lcm(&r(7_000_000_000, MAX64 - 1)), Some(340282366920938463408034375210639556610), @@ -424,10 +421,7 @@ mod tests { #[test] fn multiply_by_rational_with_rounding_works() { - assert_eq!( - multiply_by_rational_with_rounding(7, 2, 3, Rounding::Down).unwrap(), - 7 * 2 / 3 - ); + assert_eq!(multiply_by_rational_with_rounding(7, 2, 3, Rounding::Down).unwrap(), 7 * 2 / 3); assert_eq!( multiply_by_rational_with_rounding(7, 20, 30, Rounding::Down).unwrap(), 7 * 2 / 3 @@ -454,40 +448,24 @@ mod tests { ); assert_eq!( // MAX128 % 1000 == 455 - multiply_by_rational_with_rounding(MAX128, 555, 1000, Rounding::Down) - .unwrap(), + multiply_by_rational_with_rounding(MAX128, 555, 1000, Rounding::Down).unwrap(), (MAX128 / 1000 * 555) + (455 * 555 / 1000), ); assert_eq!( - multiply_by_rational_with_rounding( - 2 * MAX64 - 1, - MAX64, - MAX64, - Rounding::Down - ) - .unwrap(), + multiply_by_rational_with_rounding(2 * MAX64 - 1, MAX64, MAX64, Rounding::Down) + .unwrap(), 2 * MAX64 - 1 ); assert_eq!( - multiply_by_rational_with_rounding( - 2 * MAX64 - 1, - MAX64 - 1, - MAX64, - Rounding::Down - ) - .unwrap(), + multiply_by_rational_with_rounding(2 * MAX64 - 1, MAX64 - 1, MAX64, Rounding::Down) + .unwrap(), 2 * MAX64 - 3 ); assert_eq!( - multiply_by_rational_with_rounding( - MAX64 + 100, - MAX64_2, - MAX64_2 / 2, - Rounding::Down - ) - .unwrap(), + multiply_by_rational_with_rounding(MAX64 + 100, MAX64_2, MAX64_2 / 2, Rounding::Down) + .unwrap(), (MAX64 + 100) * 2, ); assert_eq!( @@ -512,13 +490,8 @@ mod tests { 73786976294838206461, ); assert_eq!( - multiply_by_rational_with_rounding( - 1_000_000_000, - MAX128 / 8, - MAX128 / 2, - Rounding::Up - ) - .unwrap(), + multiply_by_rational_with_rounding(1_000_000_000, MAX128 / 8, MAX128 / 2, Rounding::Up) + .unwrap(), 250000000 ); diff --git a/primitives/npos-elections/src/phragmen.rs b/primitives/npos-elections/src/phragmen.rs index bd5829dc935e5..db9ee19bd772e 100644 --- a/primitives/npos-elections/src/phragmen.rs +++ b/primitives/npos-elections/src/phragmen.rs @@ -185,10 +185,14 @@ pub fn seq_phragmen_core( for edge in &mut voter.edges { if edge.candidate.borrow().elected { // update internal state. - edge.weight = - multiply_by_rational_with_rounding(voter.budget, edge.load.n(), voter.load.n(), Rounding::Down) - // If result cannot fit in u128. Not much we can do about it. - .unwrap_or(Bounded::max_value()); + edge.weight = multiply_by_rational_with_rounding( + voter.budget, + edge.load.n(), + voter.load.n(), + Rounding::Down, + ) + // If result cannot fit in u128. Not much we can do about it. + .unwrap_or(Bounded::max_value()); } else { edge.weight = 0 } From 79ad2a63400a4a6a325efe2bb8cc52fbc587786e Mon Sep 17 00:00:00 2001 From: Tife Date: Mon, 6 Jun 2022 07:13:27 +0100 Subject: [PATCH 5/6] Test Fix --- primitives/npos-elections/src/phragmen.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/npos-elections/src/phragmen.rs b/primitives/npos-elections/src/phragmen.rs index db9ee19bd772e..637c896469111 100644 --- a/primitives/npos-elections/src/phragmen.rs +++ b/primitives/npos-elections/src/phragmen.rs @@ -147,7 +147,7 @@ pub fn seq_phragmen_core( voter.load.n(), voter.budget, candidate.approval_stake, - Rounding::NearestPrefDown, + Rounding::Down, ) .unwrap_or(Bounded::max_value()); let temp_d = voter.load.d(); From 7502333f0c2f2158c61aa6a715b9debfce78b466 Mon Sep 17 00:00:00 2001 From: Tife Date: Tue, 14 Jun 2022 21:24:53 +0100 Subject: [PATCH 6/6] Fixed fuzzer. --- .../fuzzer/src/multiply_by_rational_with_rounding.rs | 4 +--- primitives/arithmetic/src/helpers_128bit.rs | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/primitives/arithmetic/fuzzer/src/multiply_by_rational_with_rounding.rs b/primitives/arithmetic/fuzzer/src/multiply_by_rational_with_rounding.rs index fbb490997da62..474b2d363eccd 100644 --- a/primitives/arithmetic/fuzzer/src/multiply_by_rational_with_rounding.rs +++ b/primitives/arithmetic/fuzzer/src/multiply_by_rational_with_rounding.rs @@ -46,9 +46,7 @@ fn main() { // The point of this fuzzing is to make sure that `multiply_by_rational_with_rounding` // is 100% accurate as long as the value fits in a u128. - if let Some(result) = - multiply_by_rational_with_rounding(a, b, c, Rounding::NearestPrefDown) - { + if let Some(result) = multiply_by_rational_with_rounding(a, b, c, Rounding::Down) { let truth = mul_div(a, b, c); if result != truth && result != truth + 1 { diff --git a/primitives/arithmetic/src/helpers_128bit.rs b/primitives/arithmetic/src/helpers_128bit.rs index cc2bff8c0b51e..7938c31d1eaa6 100644 --- a/primitives/arithmetic/src/helpers_128bit.rs +++ b/primitives/arithmetic/src/helpers_128bit.rs @@ -183,7 +183,7 @@ mod double128 { } /// Returns `a * b / c` and `(a * b) % c` (wrapping to 128 bits) or `None` in the case of -/// overflow. +/// overflow and c = 0. pub const fn multiply_by_rational_with_rounding( a: u128, b: u128, @@ -192,7 +192,7 @@ pub const fn multiply_by_rational_with_rounding( ) -> Option { use double128::Double128; if c == 0 { - panic!("attempt to divide by zero") + return None } let (result, remainder) = Double128::product_of(a, b).div(c); let mut result: u128 = match result.try_into_u128() {