Use sensible maths for from_rational

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)])
 	);
 }
 

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"

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!(),
+		}))
 	}
 }

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!(),
+		}))
+	}
+}