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,10 @@ publish = false
 targets = ["x86_64-unknown-linux-gnu"]
 
 [dependencies]
+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 +30,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/per_thing_from_rational.rs

@@ -0,0 +1,68 @@
+// 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 fraction::prelude::BigFraction as Fraction;
+use honggfuzz::fuzz;
+use sp_arithmetic::{traits::SaturatedConversion, PerThing, Perbill, Percent, Perquintill, *};
+
+/// Tries to demonstrate that `from_rational` is incorrect.
+///
+/// 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)| {
+			let (n, d) = (data.0.min(data.1), data.0.max(data.1).max(1));
+
+			check::<PerU16>(n, d);
+			check::<Percent>(n, d);
+			check::<Perbill>(n, d);
+			check::<Perquintill>(n, d);
+		})
+	}
+}
+
+/// Assert that the parts of `from_rational` are correct.
+fn check<Per: PerThing>(a: u128, b: u128)
+where
+	Per::Inner: Into<u128>,
+{
+	let approx_ratio = Per::from_rational(a, b); // This rounds down.
+	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 = (perfect_ratio * Fraction::from(Per::ACCURACY.into())).floor();
+
+	assert_eq!(
+		approx_parts, perfect_parts,
+		"approx_parts: {}, perfect_parts: {}, a: {}, b: {}",
+		approx_parts, perfect_parts, a, b
+	);
+}

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_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::{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`. FAIL-CI is this intended?
+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(n);
+	assert!(r >= d, "{} / ({} / {}) != {} but {}", n, n, d, d, r);
+}

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,

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