diff --git a/substrate/Cargo.lock b/substrate/Cargo.lock index a645ecec60d86e18c4f755666d6fa8dd9e2e4909..33bca4005483cc566c7cfd9568e2c42f926709f6 100644 --- a/substrate/Cargo.lock +++ b/substrate/Cargo.lock @@ -220,9 +220,9 @@ dependencies = [ [[package]] name = "arbitrary" -version = "1.2.3" +version = "1.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3e90af4de65aa7b293ef2d09daff88501eb254f58edde2e1ac02c82d873eadad" +checksum = "e2d098ff73c1ca148721f37baad5ea6a465a13f9573aba8641fbbbae8164a54e" [[package]] name = "arc-swap" @@ -2224,6 +2224,16 @@ dependencies = [ "percent-encoding", ] +[[package]] +name = "fraction" +version = "0.13.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3027ae1df8d41b4bed2241c8fdad4acc1e7af60c8e17743534b545e77182d678" +dependencies = [ + "lazy_static", + "num", +] + [[package]] name = "fragile" version = "2.0.0" @@ -5286,6 +5296,20 @@ dependencies = [ "winapi", ] +[[package]] +name = "num" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "43db66d1170d347f9a065114077f7dccb00c1b9478c89384490a3425279a4606" +dependencies = [ + "num-bigint", + "num-complex", + "num-integer", + "num-iter", + "num-rational", + "num-traits", +] + [[package]] name = "num-bigint" version = "0.4.3" @@ -5326,6 +5350,17 @@ dependencies = [ "num-traits", ] +[[package]] +name = "num-iter" +version = "0.1.43" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7d03e6c028c5dc5cac6e2dec0efda81fc887605bb3d884578bb6d6bf7514e252" +dependencies = [ + "autocfg", + "num-integer", + "num-traits", +] + [[package]] name = "num-rational" version = "0.4.1" @@ -10143,8 +10178,11 @@ dependencies = [ name = "sp-arithmetic-fuzzer" version = "2.0.0" dependencies = [ + "arbitrary", + "fraction", "honggfuzz", "num-bigint", + "num-traits", "primitive-types", "sp-arithmetic", ] diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 36cafcc1854db0f33f81bbc2cc69bcd46901e90b..d97eb3ef89cabbbce78a0f22908bbccf56ddac00 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -5461,7 +5461,7 @@ fn proportional_ledger_slash_works() { ledger.active = unit; ledger.total = unit * 4 + value; // When - assert_eq!(ledger.slash(slash, 0, 0), slash - 5); + assert_eq!(ledger.slash(slash, 0, 0), slash); // Then // The amount slashed out of `unit` let affected_balance = value + unit * 4; @@ -5477,12 +5477,12 @@ fn proportional_ledger_slash_works() { value - value_slash }; assert_eq!(ledger.active, unit_slashed); - assert_eq!(ledger.unlocking, vec![c(5, value_slashed)]); - assert_eq!(ledger.total, value_slashed); + assert_eq!(ledger.unlocking, vec![c(5, value_slashed), c(7, 32)]); + assert_eq!(ledger.total, value_slashed + 32); assert_eq!(LedgerSlashPerEra::get().0, 0); assert_eq!( LedgerSlashPerEra::get().1, - BTreeMap::from([(4, 0), (5, value_slashed), (6, 0), (7, 0)]) + BTreeMap::from([(4, 0), (5, value_slashed), (6, 0), (7, 32)]) ); } diff --git a/substrate/primitives/arithmetic/fuzzer/Cargo.toml b/substrate/primitives/arithmetic/fuzzer/Cargo.toml index 7be800a2e966c55d33343fc5f6c5c3ae2675ec2e..99dbdf748732073456a1bad9f46ca9b4c01ba657 100644 --- a/substrate/primitives/arithmetic/fuzzer/Cargo.toml +++ b/substrate/primitives/arithmetic/fuzzer/Cargo.toml @@ -14,8 +14,11 @@ publish = false targets = ["x86_64-unknown-linux-gnu"] [dependencies] +arbitrary = "1.3.0" +fraction = "0.13.1" honggfuzz = "0.5.49" num-bigint = "0.4.3" +num-traits = "0.2.15" primitive-types = "0.12.0" sp-arithmetic = { version = "6.0.0", path = ".." } @@ -28,8 +31,12 @@ name = "normalize" path = "src/normalize.rs" [[bin]] -name = "per_thing_rational" -path = "src/per_thing_rational.rs" +name = "per_thing_from_rational" +path = "src/per_thing_from_rational.rs" + +[[bin]] +name = "per_thing_mult_fraction" +path = "src/per_thing_mult_fraction.rs" [[bin]] name = "multiply_by_rational_with_rounding" diff --git a/substrate/primitives/arithmetic/fuzzer/src/multiply_by_rational_with_rounding.rs b/substrate/primitives/arithmetic/fuzzer/src/multiply_by_rational_with_rounding.rs index 13c9d022be6bdf45c3e7a2ad9c7147a5b0686781..e9a3208a738efdee8a7333380658954b3ffd9f87 100644 --- a/substrate/primitives/arithmetic/fuzzer/src/multiply_by_rational_with_rounding.rs +++ b/substrate/primitives/arithmetic/fuzzer/src/multiply_by_rational_with_rounding.rs @@ -29,52 +29,79 @@ //! More information about `honggfuzz` can be found //! [here](https://docs.rs/honggfuzz/). +use fraction::prelude::BigFraction as Fraction; use honggfuzz::fuzz; -use sp_arithmetic::{helpers_128bit::multiply_by_rational_with_rounding, traits::Zero, Rounding}; +use sp_arithmetic::{MultiplyRational, Rounding, Rounding::*}; +/// Tries to demonstrate that `multiply_by_rational_with_rounding` is incorrect. fn main() { loop { - fuzz!(|data: ([u8; 16], [u8; 16], [u8; 16])| { - let (a_bytes, b_bytes, c_bytes) = data; - let (a, b, c) = ( - u128::from_be_bytes(a_bytes), - u128::from_be_bytes(b_bytes), - u128::from_be_bytes(c_bytes), - ); + fuzz!(|data: (u128, u128, u128, ArbitraryRounding)| { + let (f, n, d, r) = (data.0, data.1, data.2, data.3 .0); - println!("++ Equation: {} * {} / {}", 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 { - println!("++ Expected {}", truth); - println!("+++++++ Got {}", result); - panic!(); - } - } + check::<u8>(f as u8, n as u8, d as u8, r); + check::<u16>(f as u16, n as u16, d as u16, r); + check::<u32>(f as u32, n as u32, d as u32, r); + check::<u64>(f as u64, n as u64, d as u64, r); + check::<u128>(f, n, d, r); }) } } -fn mul_div(a: u128, b: u128, c: u128) -> u128 { - use primitive_types::U256; - if a.is_zero() { - return Zero::zero() - } - let c = c.max(1); +fn check<N>(f: N, n: N, d: N, r: Rounding) +where + N: MultiplyRational + Into<u128> + Copy + core::fmt::Debug, +{ + let Some(got) = f.multiply_rational(n, d, r) else { + return; + }; + + let (ae, be, ce) = + (Fraction::from(f.into()), Fraction::from(n.into()), Fraction::from(d.into())); + let want = round(ae * be / ce, r); - // e for extended - let ae: U256 = a.into(); - let be: U256 = b.into(); - let ce: U256 = c.into(); + assert_eq!( + Fraction::from(got.into()), + want, + "{:?} * {:?} / {:?} = {:?} != {:?}", + f, + n, + d, + got, + want + ); +} + +/// Round a `Fraction` according to the given mode. +fn round(f: Fraction, r: Rounding) -> Fraction { + match r { + Up => f.ceil(), + NearestPrefUp => + if f.fract() < Fraction::from(0.5) { + f.floor() + } else { + f.ceil() + }, + Down => f.floor(), + NearestPrefDown => + if f.fract() > Fraction::from(0.5) { + f.ceil() + } else { + f.floor() + }, + } +} - let r = ae * be / ce; - if r > u128::MAX.into() { - a - } else { - r.as_u128() +/// An [`arbitrary::Arbitrary`] [`Rounding`] mode. +struct ArbitraryRounding(Rounding); +impl arbitrary::Arbitrary<'_> for ArbitraryRounding { + fn arbitrary(u: &mut arbitrary::Unstructured<'_>) -> arbitrary::Result<Self> { + Ok(Self(match u.int_in_range(0..=3).unwrap() { + 0 => Up, + 1 => NearestPrefUp, + 2 => Down, + 3 => NearestPrefDown, + _ => unreachable!(), + })) } } diff --git a/substrate/primitives/arithmetic/fuzzer/src/per_thing_from_rational.rs b/substrate/primitives/arithmetic/fuzzer/src/per_thing_from_rational.rs new file mode 100644 index 0000000000000000000000000000000000000000..93af4df9e8e55d9190a0926467b5e72e7f423fbf --- /dev/null +++ b/substrate/primitives/arithmetic/fuzzer/src/per_thing_from_rational.rs @@ -0,0 +1,105 @@ +// This file is part of Substrate. + +// Copyright (C) Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! # Running +//! Running this fuzzer can be done with `cargo hfuzz run per_thing_from_rational`. `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 per_thing_from_rational hfuzz_workspace/per_thing_from_rational/*.fuzz`. + +use fraction::prelude::BigFraction as Fraction; +use honggfuzz::fuzz; +use sp_arithmetic::{ + traits::SaturatedConversion, PerThing, Perbill, Percent, Perquintill, Rounding::*, *, +}; + +/// Tries to demonstrate that `from_rational` is incorrect for any rounding modes. +/// +/// NOTE: This `Fraction` library is really slow. Using f128/f256 does not work for the large +/// numbers. But an optimization could be done do use either floats or Fraction depending on the +/// size of the inputs. +fn main() { + loop { + fuzz!(|data: (u128, u128, ArbitraryRounding)| { + let (n, d, r) = (data.0.min(data.1), data.0.max(data.1).max(1), data.2); + + check::<PerU16>(n, d, r.0); + check::<Percent>(n, d, r.0); + check::<Permill>(n, d, r.0); + check::<Perbill>(n, d, r.0); + check::<Perquintill>(n, d, r.0); + }) + } +} + +/// Assert that the parts of `from_rational` are correct for the given rounding mode. +fn check<Per: PerThing>(a: u128, b: u128, r: Rounding) +where + Per::Inner: Into<u128>, +{ + let approx_ratio = Per::from_rational_with_rounding(a, b, r).unwrap(); + let approx_parts = Fraction::from(approx_ratio.deconstruct().saturated_into::<u128>()); + + let perfect_ratio = if a == 0 && b == 0 { + Fraction::from(1) + } else { + Fraction::from(a) / Fraction::from(b.max(1)) + }; + let perfect_parts = round(perfect_ratio * Fraction::from(Per::ACCURACY.into()), r); + + assert_eq!( + approx_parts, perfect_parts, + "approx_parts: {}, perfect_parts: {}, a: {}, b: {}", + approx_parts, perfect_parts, a, b + ); +} + +/// Round a `Fraction` according to the given mode. +fn round(f: Fraction, r: Rounding) -> Fraction { + match r { + Up => f.ceil(), + NearestPrefUp => + if f.fract() < Fraction::from(0.5) { + f.floor() + } else { + f.ceil() + }, + Down => f.floor(), + NearestPrefDown => + if f.fract() > Fraction::from(0.5) { + f.ceil() + } else { + f.floor() + }, + } +} + +/// An [`arbitrary::Arbitrary`] [`Rounding`] mode. +struct ArbitraryRounding(Rounding); +impl arbitrary::Arbitrary<'_> for ArbitraryRounding { + fn arbitrary(u: &mut arbitrary::Unstructured<'_>) -> arbitrary::Result<Self> { + Ok(Self(match u.int_in_range(0..=3).unwrap() { + 0 => Up, + 1 => NearestPrefUp, + 2 => Down, + 3 => NearestPrefDown, + _ => unreachable!(), + })) + } +} diff --git a/substrate/primitives/arithmetic/fuzzer/src/per_thing_mult_fraction.rs b/substrate/primitives/arithmetic/fuzzer/src/per_thing_mult_fraction.rs new file mode 100644 index 0000000000000000000000000000000000000000..9cfe28a7800b0dd995009ccf780be43302c2ebb6 --- /dev/null +++ b/substrate/primitives/arithmetic/fuzzer/src/per_thing_mult_fraction.rs @@ -0,0 +1,69 @@ +// This file is part of Substrate. + +// Copyright (C) Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! # Running +//! Running this fuzzer can be done with `cargo hfuzz run per_thing_mult_fraction`. `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 per_thing_mult_fraction hfuzz_workspace/per_thing_mult_fraction/*.fuzz`. + +use honggfuzz::fuzz; +use sp_arithmetic::{PerThing, Perbill, Percent, Perquintill, *}; + +/// Tries to disprove `(n / d) * d <= n` for any `PerThing`s. +fn main() { + loop { + fuzz!(|data: (u128, u128)| { + let (n, d) = (data.0.min(data.1), data.0.max(data.1).max(1)); + + check_mul::<PerU16>(n, d); + check_mul::<Percent>(n, d); + check_mul::<Perbill>(n, d); + check_mul::<Perquintill>(n, d); + + check_reciprocal_mul::<PerU16>(n, d); + check_reciprocal_mul::<Percent>(n, d); + check_reciprocal_mul::<Perbill>(n, d); + check_reciprocal_mul::<Perquintill>(n, d); + }) + } +} + +/// Checks that `(n / d) * d <= n`. +fn check_mul<P: PerThing>(n: u128, d: u128) +where + P: PerThing + core::ops::Mul<u128, Output = u128>, +{ + let q = P::from_rational_with_rounding(n, d, Rounding::Down).unwrap(); + assert!(q * d <= n, "{:?} * {:?} <= {:?}", q, d, n); +} + +/// Checks that `n / (n / d) >= d`. +fn check_reciprocal_mul<P: PerThing>(n: u128, d: u128) +where + P: PerThing + core::ops::Mul<u128, Output = u128>, +{ + let q = P::from_rational_with_rounding(n, d, Rounding::Down).unwrap(); + if q.is_zero() { + return + } + + let r = q.saturating_reciprocal_mul_floor(n); + assert!(r >= d, "{} / ({} / {}) != {} but {}", n, n, d, d, r); +} diff --git a/substrate/primitives/arithmetic/fuzzer/src/per_thing_rational.rs b/substrate/primitives/arithmetic/fuzzer/src/per_thing_rational.rs deleted file mode 100644 index c9c5146565cc7896bf651038af9765d4fe183e39..0000000000000000000000000000000000000000 --- a/substrate/primitives/arithmetic/fuzzer/src/per_thing_rational.rs +++ /dev/null @@ -1,116 +0,0 @@ -// This file is part of Substrate. - -// Copyright (C) Parity Technologies (UK) Ltd. -// SPDX-License-Identifier: Apache-2.0 - -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -//! # Running -//! Running this fuzzer can be done with `cargo hfuzz run per_thing_rational`. `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 per_thing_rational hfuzz_workspace/per_thing_rational/*.fuzz`. - -use honggfuzz::fuzz; -use sp_arithmetic::{traits::SaturatedConversion, PerThing, PerU16, Perbill, Percent, Perquintill}; - -fn main() { - loop { - fuzz!(|data: ((u16, u16), (u32, u32), (u64, u64))| { - let (u16_pair, u32_pair, u64_pair) = data; - - // peru16 - let (smaller, bigger) = (u16_pair.0.min(u16_pair.1), u16_pair.0.max(u16_pair.1)); - let ratio = PerU16::from_rational(smaller, bigger); - assert_per_thing_equal_error( - ratio, - PerU16::from_float(smaller as f64 / bigger.max(1) as f64), - 1, - ); - let (smaller, bigger) = (u32_pair.0.min(u32_pair.1), u32_pair.0.max(u32_pair.1)); - let ratio = PerU16::from_rational(smaller, bigger); - assert_per_thing_equal_error( - ratio, - PerU16::from_float(smaller as f64 / bigger.max(1) as f64), - 1, - ); - let (smaller, bigger) = (u64_pair.0.min(u64_pair.1), u64_pair.0.max(u64_pair.1)); - let ratio = PerU16::from_rational(smaller, bigger); - assert_per_thing_equal_error( - ratio, - PerU16::from_float(smaller as f64 / bigger.max(1) as f64), - 1, - ); - - // percent - let (smaller, bigger) = (u16_pair.0.min(u16_pair.1), u16_pair.0.max(u16_pair.1)); - let ratio = Percent::from_rational(smaller, bigger); - assert_per_thing_equal_error( - ratio, - Percent::from_float(smaller as f64 / bigger.max(1) as f64), - 1, - ); - - let (smaller, bigger) = (u32_pair.0.min(u32_pair.1), u32_pair.0.max(u32_pair.1)); - let ratio = Percent::from_rational(smaller, bigger); - assert_per_thing_equal_error( - ratio, - Percent::from_float(smaller as f64 / bigger.max(1) as f64), - 1, - ); - - let (smaller, bigger) = (u64_pair.0.min(u64_pair.1), u64_pair.0.max(u64_pair.1)); - let ratio = Percent::from_rational(smaller, bigger); - assert_per_thing_equal_error( - ratio, - Percent::from_float(smaller as f64 / bigger.max(1) as f64), - 1, - ); - - // perbill - let (smaller, bigger) = (u32_pair.0.min(u32_pair.1), u32_pair.0.max(u32_pair.1)); - let ratio = Perbill::from_rational(smaller, bigger); - assert_per_thing_equal_error( - ratio, - Perbill::from_float(smaller as f64 / bigger.max(1) as f64), - 100, - ); - - let (smaller, bigger) = (u64_pair.0.min(u64_pair.1), u64_pair.0.max(u64_pair.1)); - let ratio = Perbill::from_rational(smaller, bigger); - assert_per_thing_equal_error( - ratio, - Perbill::from_float(smaller as f64 / bigger.max(1) as f64), - 100, - ); - - // perquintillion - let (smaller, bigger) = (u64_pair.0.min(u64_pair.1), u64_pair.0.max(u64_pair.1)); - let ratio = Perquintill::from_rational(smaller, bigger); - assert_per_thing_equal_error( - ratio, - Perquintill::from_float(smaller as f64 / bigger.max(1) as f64), - 1000, - ); - }) - } -} - -fn assert_per_thing_equal_error<P: PerThing>(a: P, b: P, err: u128) { - let a_abs = a.deconstruct().saturated_into::<u128>(); - let b_abs = b.deconstruct().saturated_into::<u128>(); - let diff = a_abs.max(b_abs) - a_abs.min(b_abs); - assert!(diff <= err, "{:?} !~ {:?}", a, b); -} diff --git a/substrate/primitives/arithmetic/src/helpers_128bit.rs b/substrate/primitives/arithmetic/src/helpers_128bit.rs index 56432d15a726009e13609bc663566b783be24909..9b9c74ba55774984f3891968861317208b4bb5d2 100644 --- a/substrate/primitives/arithmetic/src/helpers_128bit.rs +++ b/substrate/primitives/arithmetic/src/helpers_128bit.rs @@ -182,8 +182,8 @@ mod double128 { } } -/// Returns `a * b / c` and `(a * b) % c` (wrapping to 128 bits) or `None` in the case of -/// overflow and c = 0. +/// Returns `a * b / c` (wrapping to 128 bits) or `None` in the case of +/// overflow. pub const fn multiply_by_rational_with_rounding( a: u128, b: u128, diff --git a/substrate/primitives/arithmetic/src/lib.rs b/substrate/primitives/arithmetic/src/lib.rs index 581206b6d4460bc863c470e4688c430e7acde07a..e7ba08a5329098c93f5cd1980057a5f2f51a1409 100644 --- a/substrate/primitives/arithmetic/src/lib.rs +++ b/substrate/primitives/arithmetic/src/lib.rs @@ -45,7 +45,7 @@ pub use per_things::{ InnerOf, MultiplyArg, PerThing, PerU16, Perbill, Percent, Permill, Perquintill, RationalArg, ReciprocalArg, Rounding, SignedRounding, UpperOf, }; -pub use rational::{Rational128, RationalInfinite}; +pub use rational::{MultiplyRational, Rational128, RationalInfinite}; use sp_std::{cmp::Ordering, fmt::Debug, prelude::*}; use traits::{BaseArithmetic, One, SaturatedConversion, Unsigned, Zero}; diff --git a/substrate/primitives/arithmetic/src/per_things.rs b/substrate/primitives/arithmetic/src/per_things.rs index 068bdb4e17b0dfac69769d022998c5dc7d5ee77c..c4cb5196602ea38135c79af2f583ad1524050925 100644 --- a/substrate/primitives/arithmetic/src/per_things.rs +++ b/substrate/primitives/arithmetic/src/per_things.rs @@ -26,7 +26,7 @@ use codec::{CompactAs, Encode}; use num_traits::{Pow, SaturatingAdd, SaturatingSub}; use sp_std::{ fmt, ops, - ops::{Add, AddAssign, Div, Rem, Sub}, + ops::{Add, Sub}, prelude::*, }; @@ -46,6 +46,7 @@ pub trait RationalArg: + Unsigned + Zero + One + + crate::MultiplyRational { } @@ -58,7 +59,8 @@ impl< + ops::AddAssign<Self> + Unsigned + Zero - + One, + + One + + crate::MultiplyRational, > RationalArg for T { } @@ -105,7 +107,7 @@ pub trait PerThing: + Pow<usize, Output = Self> { /// The data type used to build this per-thingy. - type Inner: BaseArithmetic + Unsigned + Copy + Into<u128> + fmt::Debug; + type Inner: BaseArithmetic + Unsigned + Copy + Into<u128> + fmt::Debug + crate::MultiplyRational; /// A data type larger than `Self::Inner`, used to avoid overflow in some computations. /// It must be able to compute `ACCURACY^2`. @@ -115,7 +117,8 @@ pub trait PerThing: + TryInto<Self::Inner> + UniqueSaturatedInto<Self::Inner> + Unsigned - + fmt::Debug; + + fmt::Debug + + crate::MultiplyRational; /// The accuracy of this type. const ACCURACY: Self::Inner; @@ -538,39 +541,6 @@ where rem_mul_div_inner.into() } -/// Just a simple generic integer divide with custom rounding. -fn div_rounded<N>(n: N, d: N, r: Rounding) -> N -where - N: Clone - + Eq - + Ord - + Zero - + One - + AddAssign - + Add<Output = N> - + Rem<Output = N> - + Div<Output = N>, -{ - let mut o = n.clone() / d.clone(); - use Rounding::*; - let two = || N::one() + N::one(); - if match r { - Up => !((n % d).is_zero()), - NearestPrefDown => { - let rem = n % d.clone(); - rem > d / two() - }, - NearestPrefUp => { - let rem = n % d.clone(); - rem >= d.clone() / two() + d % two() - }, - Down => false, - } { - o += N::one() - } - o -} - macro_rules! implement_per_thing { ( $name:ident, @@ -687,7 +657,8 @@ macro_rules! implement_per_thing { + ops::AddAssign<N> + Unsigned + Zero - + One, + + One + + $crate::MultiplyRational, Self::Inner: Into<N> { // q cannot be zero. @@ -695,30 +666,8 @@ macro_rules! implement_per_thing { // p should not be bigger than q. if p > q { return Err(()) } - let factor = div_rounded::<N>(q.clone(), $max.into(), Rounding::Up).max(One::one()); - - // q cannot overflow: (q / (q/$max)) < $max. p < q hence p also cannot overflow. - let q_reduce: $type = div_rounded(q, factor.clone(), r) - .try_into() - .map_err(|_| "Failed to convert") - .expect( - "`q / ceil(q/$max) < $max`; macro prevents any type being created that \ - does not satisfy this; qed" - ); - let p_reduce: $type = div_rounded(p, factor, r) - .try_into() - .map_err(|_| "Failed to convert") - .expect( - "`p / ceil(p/$max) < $max`; macro prevents any type being created that \ - does not satisfy this; qed" - ); - - // `p_reduced` and `q_reduced` are within `Self::Inner`. Multiplication by another - // `$max` will always fit in `$upper_type`. This is guaranteed by the macro tests. - let n = p_reduce as $upper_type * <$upper_type>::from($max); - let d = q_reduce as $upper_type; - let part = div_rounded(n, d, r); - Ok($name(part as Self::Inner)) + let max: N = $max.into(); + max.multiply_rational(p, q, r).ok_or(())?.try_into().map(|x| $name(x)).map_err(|_| ()) } } @@ -1862,6 +1811,22 @@ fn from_rational_with_rounding_works_in_extreme_case() { } } +#[test] +fn from_rational_with_rounding_breakage() { + let n = 372633774963620730670986667244911905u128; + let d = 512593663333074177468745541591173060u128; + let q = Perquintill::from_rational_with_rounding(n, d, Rounding::Down).unwrap(); + assert!(q * d <= n); +} + +#[test] +fn from_rational_with_rounding_breakage_2() { + let n = 36893488147419103230u128; + let d = 36893488147419103630u128; + let q = Perquintill::from_rational_with_rounding(n, d, Rounding::Up).unwrap(); + assert!(q * d >= n); +} + implement_per_thing!(Percent, test_per_cent, [u32, u64, u128], 100u8, u8, u16, "_Percent_",); implement_per_thing_with_perthousand!( PerU16, diff --git a/substrate/primitives/arithmetic/src/rational.rs b/substrate/primitives/arithmetic/src/rational.rs index 0fcb7d4d9462a1579d0b618a8346cc8c81e19413..ebd89c615a38b3efc717fbb7051fe12ccfb22ea1 100644 --- a/substrate/primitives/arithmetic/src/rational.rs +++ b/substrate/primitives/arithmetic/src/rational.rs @@ -284,6 +284,54 @@ impl PartialEq for Rational128 { } } +pub trait MultiplyRational: Sized { + fn multiply_rational(self, n: Self, d: Self, r: Rounding) -> Option<Self>; +} + +macro_rules! impl_rrm { + ($ulow:ty, $uhi:ty) => { + impl MultiplyRational for $ulow { + fn multiply_rational(self, n: Self, d: Self, r: Rounding) -> Option<Self> { + if d.is_zero() { + return None + } + + let sn = (self as $uhi) * (n as $uhi); + let mut result = sn / (d as $uhi); + let remainder = (sn % (d as $uhi)) as $ulow; + if match r { + Rounding::Up => remainder > 0, + // cannot be `(d + 1) / 2` since `d` might be `max_value` and overflow. + Rounding::NearestPrefUp => remainder >= d / 2 + d % 2, + Rounding::NearestPrefDown => remainder > d / 2, + Rounding::Down => false, + } { + result = match result.checked_add(1) { + Some(v) => v, + None => return None, + }; + } + if result > (<$ulow>::max_value() as $uhi) { + None + } else { + Some(result as $ulow) + } + } + } + }; +} + +impl_rrm!(u8, u16); +impl_rrm!(u16, u32); +impl_rrm!(u32, u64); +impl_rrm!(u64, u128); + +impl MultiplyRational for u128 { + fn multiply_rational(self, n: Self, d: Self, r: Rounding) -> Option<Self> { + crate::helpers_128bit::multiply_by_rational_with_rounding(self, n, d, r) + } +} + #[cfg(test)] mod tests { use super::{helpers_128bit::*, *}; diff --git a/substrate/primitives/arithmetic/src/traits.rs b/substrate/primitives/arithmetic/src/traits.rs index 33b4e376aaf9d3d81b31458b7a04ed94f6a21da5..061b11b3e9c722cd2925ab6643c04c22bad8da0e 100644 --- a/substrate/primitives/arithmetic/src/traits.rs +++ b/substrate/primitives/arithmetic/src/traits.rs @@ -32,6 +32,8 @@ use sp_std::ops::{ Add, AddAssign, Div, DivAssign, Mul, MulAssign, Rem, RemAssign, Shl, Shr, Sub, SubAssign, }; +use crate::MultiplyRational; + /// A meta trait for arithmetic type operations, regardless of any limitation on size. pub trait BaseArithmetic: From<u8> @@ -186,9 +188,9 @@ pub trait AtLeast32Bit: BaseArithmetic + From<u16> + From<u32> {} impl<T: BaseArithmetic + From<u16> + From<u32>> AtLeast32Bit for T {} /// A meta trait for arithmetic. Same as [`AtLeast32Bit `], but also bounded to be unsigned. -pub trait AtLeast32BitUnsigned: AtLeast32Bit + Unsigned {} +pub trait AtLeast32BitUnsigned: AtLeast32Bit + Unsigned + MultiplyRational {} -impl<T: AtLeast32Bit + Unsigned> AtLeast32BitUnsigned for T {} +impl<T: AtLeast32Bit + Unsigned + MultiplyRational> AtLeast32BitUnsigned for T {} /// Just like `From` except that if the source value is too big to fit into the destination type /// then it'll saturate the destination.