From 12aede0fd8ff43535067d14cab2052d89af04076 Mon Sep 17 00:00:00 2001 From: Boluwatife Bakre Date: Fri, 17 Jun 2022 04:57:29 +0100 Subject: [PATCH] Remove multiply_by_rational (#11598) * Removed multiply_by_rational Replaced with multiply_by_rational_with_rounding * fixes * Test Fixes * nightly fmt * Test Fix * Fixed fuzzer. --- primitives/arithmetic/fuzzer/Cargo.toml | 4 +- ... => multiply_by_rational_with_rounding.rs} | 16 +- primitives/arithmetic/src/fixed_point.rs | 48 ++++-- primitives/arithmetic/src/helpers_128bit.rs | 72 +-------- primitives/arithmetic/src/rational.rs | 151 ++++++++++++------ primitives/npos-elections/src/phragmen.rs | 18 ++- 6 files changed, 162 insertions(+), 147 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..474b2d363eccd 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,9 @@ 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::Down) { 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 2dfe717fe7e4f..bf3c93cdad605 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,13 @@ 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), + ) + .and_then(|value| from_i129(I129 { value, negative })) } /// Saturating multiplication for integer type `N`. Equal to `self * n`. @@ -832,10 +840,14 @@ 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), + ) + .and_then(|value| from_i129(I129 { value, negative })) + .map(Self) } } @@ -845,10 +857,14 @@ 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), + ) + .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..7938c31d1eaa6 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 @@ -247,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, @@ -256,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() { @@ -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, 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 1beafbe811614..54cabfc6214e8 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::*}; @@ -143,27 +143,38 @@ 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(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)) } } /// 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) -> 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. @@ -188,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) @@ -202,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 @@ -314,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] @@ -342,13 +357,10 @@ 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)), - Err("result cannot fit in u128"), - ); + 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)), - Ok(340282366920938463408034375210639556610), + Some(340282366920938463408034375210639556610), ); const_assert!(340282366920938463408034375210639556610 < MAX128); const_assert!(340282366920938463408034375210639556610 == MAX64 * (MAX64 - 1)); @@ -408,55 +420,87 @@ 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::Down).unwrap(), 7 * 2 / 3); + assert_eq!( + 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::Down).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::Down).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::Down).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::Down).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::Down).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::Down) + .unwrap(), + 2 * MAX64 - 1 + ); + assert_eq!( + multiply_by_rational_with_rounding(2 * MAX64 - 1, MAX64 - 1, MAX64, Rounding::Down) + .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::Down) + .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::Down + ) + .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::Down + ) + .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::Up) + .unwrap(), + 250000000 + ); assert_eq!( - multiply_by_rational( + multiply_by_rational_with_rounding( 29459999999999999988000u128, 1000000000000000000u128, - 10000000000000000000u128 + 10000000000000000000u128, + Rounding::Down ) .unwrap(), 2945999999999999998800u128 @@ -464,17 +508,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 9b50dc36e48fb..ca32780ed84b4 100644 --- a/primitives/npos-elections/src/phragmen.rs +++ b/primitives/npos-elections/src/phragmen.rs @@ -25,9 +25,9 @@ use crate::{ IdentifierT, 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::Down, ) .unwrap_or(Bounded::max_value()); let temp_d = voter.load.d(); @@ -184,9 +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(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(), + Rounding::Down, + ) + // If result cannot fit in u128. Not much we can do about it. + .unwrap_or(Bounded::max_value()); } else { edge.weight = 0 }