Skip to content

Commit

Permalink
Remove multiply_by_rational (paritytech#11598)
Browse files Browse the repository at this point in the history 
* Removed multiply_by_rational
Replaced with multiply_by_rational_with_rounding

* fixes

* Test Fixes

* nightly fmt

* Test Fix

* Fixed fuzzer.
  • Loading branch information
@tifecool @godcodehunter
tifecool authored and godcodehunter committed Jun 22, 2022
1 parent 6b5a89d commit 12aede0
Show file tree
Hide file tree
Showing 6 changed files with 162 additions and 147 deletions.
4 changes: 2 additions & 2 deletions primitives/arithmetic/fuzzer/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
16 changes: 9 additions & 7 deletions ...hmetic/fuzzer/src/multiply_by_rational.rs → ...src/multiply_by_rational_with_rounding.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
48 changes: 32 additions & 16 deletions primitives/arithmetic/src/fixed_point.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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`.
Expand All @@ -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`.
Expand Down Expand Up @@ -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)
}
}

Expand All @@ -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)
}
}

Expand Down
72 changes: 4 additions & 68 deletions primitives/arithmetic/src/helpers_128bit.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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,
Expand All @@ -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() {
Expand Down Expand Up @@ -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);
Expand Down
Loading

0 comments on commit 12aede0

Please sign in to comment.