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.
paritytech · Jun 17, 2022 · 61f99d9 · 61f99d9
1 parent 6a59cc3
commit 61f99d9
Show file tree

Hide file tree

Showing 6 changed files with 162 additions and 147 deletions.
diff --git 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/...hmetic/fuzzer/src/multiply_by_rational.rs → ...src/multiply_by_rational_with_rounding.rs b/...hmetic/fuzzer/src/multiply_by_rational.rs → ...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
@@ -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
@@ -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<u128, &'static str> {
-	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<u128> {
 	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);