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.

substrate/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"

substrate/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 {

substrate/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)
 			}
 		}
 

substrate/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);

substrate/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<Self, &'static str> {
+	pub fn to_den(self, den: u128) -> Option<Self> {
 		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<u128, &'static str> {
+	pub fn lcm(&self, other: &Self) -> Option<u128> {
 		// 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<Self, &'static str> {
-		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<Self, &'static str> {
-		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)
 		);
 	}
 }