Staking module: approximate fraction into perbill to avoid overflow

core/sr-primitives/src/lib.rs

@@ -162,6 +162,21 @@ impl Permill {
 	/// Converts a fraction into `Permill`.
 	#[cfg(feature = "std")]
 	pub fn from_fraction(x: f64) -> Self { Self((x * 1_000_000.0) as u32) }
+
+	/// Approximate the fracion `p/q` into a per million fraction
+	pub fn from_rational_approximation<N>(p: N, q: N) -> Self
+		where N: traits::SimpleArithmetic + Clone
+	{
+		let p = p.min(q.clone());
+		let factor = (q.clone() / 1_000_000u32.into()).max(1u32.into());
+
+		// Conversion can't overflow as p < q so ( p / (q/million)) < million
+		let p_reduce: u32 = (p / factor.clone()).try_into().unwrap_or_else(|_| panic!());
+		let q_reduce: u32 = (q / factor.clone()).try_into().unwrap_or_else(|_| panic!());
+		let part = p_reduce as u64 * 1_000_000u64 / q_reduce as u64;
+
+		Permill(part as u32)
+	}
 }
 
 impl<N> ops::Mul<N> for Permill
@@ -254,6 +269,21 @@ impl Perbill {
 	#[cfg(feature = "std")]
 	/// Construct new instance whose value is equal to `x` (between 0 and 1).
 	pub fn from_fraction(x: f64) -> Self { Self((x.max(0.0).min(1.0) * 1_000_000_000.0) as u32) }
+
+	/// Approximate the fracion `p/q` into a per billion fraction
+	pub fn from_rational_approximation<N>(p: N, q: N) -> Self
+		where N: traits::SimpleArithmetic + Clone
+	{
+		let p = p.min(q.clone());
+		let factor = (q.clone() / 1_000_000_000u32.into()).max(1u32.into());
+
+		// Conversion can't overflow as p < q so ( p / (q/billion)) < billion
+		let p_reduce: u32 = (p / factor.clone()).try_into().unwrap_or_else(|_| panic!());
+		let q_reduce: u32 = (q / factor.clone()).try_into().unwrap_or_else(|_| panic!());
+		let part = p_reduce as u64 * 1_000_000_000u64 / q_reduce as u64;
+
+		Perbill(part as u32)
+	}
 }
 
 impl<N> ops::Mul<N> for Perbill
@@ -318,8 +348,7 @@ impl From<codec::Compact<Perbill>> for Perbill {
 	}
 }
 
-/// PerU128 is parts-per-u128-max-value. It stores a value between 0 and 1 in fixed point and
-/// provides a means to multiply some other value by that.
+/// PerU128 is parts-per-u128-max-value. It stores a value between 0 and 1 in fixed point.
 #[cfg_attr(feature = "std", derive(Serialize, Deserialize, Debug))]
 #[derive(Encode, Decode, Default, Copy, Clone, PartialEq, Eq)]
 pub struct PerU128(u128);
@@ -646,9 +675,9 @@ impl traits::Extrinsic for OpaqueExtrinsic {
 mod tests {
 	use crate::codec::{Encode, Decode};
 
-	macro_rules! per_thing_mul_upper_test {
+	macro_rules! per_thing_upper_test {
 		($num_type:tt, $per:tt) => {
-			// all sort of from_percent
+			// multiplication from all sort of from_percent
 			assert_eq!($per::from_percent(100) * $num_type::max_value(), $num_type::max_value());
 			assert_eq!(
 				$per::from_percent(99) * $num_type::max_value(),
@@ -658,9 +687,23 @@ mod tests {
 			assert_eq!($per::from_percent(1) * $num_type::max_value(), $num_type::max_value() / 100);
 			assert_eq!($per::from_percent(0) * $num_type::max_value(), 0);
 
-			// bounds
+			// multiplication with bounds
 			assert_eq!($per::one() * $num_type::max_value(), $num_type::max_value());
 			assert_eq!($per::zero() * $num_type::max_value(), 0);
+
+			// from_rational_approximation
+			assert_eq!(
+				$per::from_rational_approximation(u128::max_value() - 1, u128::max_value()),
+				$per::one(),
+			);
+			assert_eq!(
+				$per::from_rational_approximation(u128::max_value()/3, u128::max_value()),
+				$per::from_parts($per::one().0/3),
+			);
+			assert_eq!(
+				$per::from_rational_approximation(1, u128::max_value()),
+				$per::zero(),
+			);
 		}
 	}
 
@@ -714,13 +757,14 @@ mod tests {
 		use super::{Perbill, Permill};
 		use primitive_types::U256;
 
-		per_thing_mul_upper_test!(u32, Perbill);
-		per_thing_mul_upper_test!(u64, Perbill);
-		per_thing_mul_upper_test!(u128, Perbill);
+		per_thing_upper_test!(u32, Perbill);
+		per_thing_upper_test!(u64, Perbill);
+		per_thing_upper_test!(u128, Perbill);
+
+		per_thing_upper_test!(u32, Permill);
+		per_thing_upper_test!(u64, Permill);
+		per_thing_upper_test!(u128, Permill);
 
-		per_thing_mul_upper_test!(u32, Permill);
-		per_thing_mul_upper_test!(u64, Permill);
-		per_thing_mul_upper_test!(u128, Permill);
 	}
 
 	#[test]

node/runtime/src/lib.rs

@@ -59,7 +59,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	impl_name: create_runtime_str!("substrate-node"),
 	authoring_version: 10,
 	spec_version: 96,
-	impl_version: 96,
+	impl_version: 97,
 	apis: RUNTIME_API_VERSIONS,
 };
 

srml/staking/src/lib.rs

@@ -940,11 +940,10 @@ impl<T: Trait> Module<T> {
 			// The total to be slashed from the nominators.
 			let total = exposure.total - exposure.own;
 			if !total.is_zero() {
-				// FIXME #1572 avoid overflow
-				let safe_mul_rational = |b| b * rest_slash / total;
 				for i in exposure.others.iter() {
+					let per_u64 = Perbill::from_rational_approximation(i.value, total);
 					// best effort - not much that can be done on fail.
-					imbalance.subsume(T::Currency::slash(&i.who, safe_mul_rational(i.value)).0)
+					imbalance.subsume(T::Currency::slash(&i.who, per_u64 * rest_slash).0)
 				}
 			}
 		}
@@ -986,13 +985,14 @@ impl<T: Trait> Module<T> {
 		} else {
 			let exposure = Self::stakers(stash);
 			let total = exposure.total.max(One::one());
-			// FIXME #1572:  avoid overflow
-			let safe_mul_rational = |b| b * reward / total;
+
 			for i in &exposure.others {
-				let nom_payout = safe_mul_rational(i.value);
-				imbalance.maybe_subsume(Self::make_payout(&i.who, nom_payout));
+				let per_u64 = Perbill::from_rational_approximation(i.value, total);
+				imbalance.maybe_subsume(Self::make_payout(&i.who, per_u64 * reward));
 			}
-			safe_mul_rational(exposure.own)
+
+			let per_u64 = Perbill::from_rational_approximation(exposure.own, total);
+			per_u64 * reward
 		};
 		imbalance.maybe_subsume(Self::make_payout(stash, validator_cut + off_the_table));
 		T::Reward::on_unbalanced(imbalance);

srml/staking/src/tests.rs

@@ -665,13 +665,21 @@ fn nominating_and_rewards_should_work() {
 		// nothing else will happen, era ends and rewards are paid again,
 		// it is expected that nominators will also be paid. See below
 
+		// Approximation resulting from Perbill conversion
+		let approximation = 1;
 		// Nominator 2: has [400/1800 ~ 2/9 from 10] + [600/2200 ~ 3/11 from 20]'s reward. ==> 2/9 + 3/11
-		assert_eq!(Balances::total_balance(&2), initial_balance + (2*new_session_reward/9 + 3*new_session_reward/11) - 1);
+		assert_eq!(
+			Balances::total_balance(&2),
+			initial_balance + (2*new_session_reward/9 + 3*new_session_reward/11) - 1 - approximation
+		);
 		// Nominator 4: has [400/1800 ~ 2/9 from 10] + [600/2200 ~ 3/11 from 20]'s reward. ==> 2/9 + 3/11
-		assert_eq!(Balances::total_balance(&4), initial_balance + (2*new_session_reward/9 + 3*new_session_reward/11) - 1);
+		assert_eq!(
+			Balances::total_balance(&4),
+			initial_balance + (2*new_session_reward/9 + 3*new_session_reward/11) - 1 - approximation
+		);
 
 		// 10 got 800 / 1800 external stake => 8/18 =? 4/9 => Validator's share = 5/9
-		assert_eq!(Balances::total_balance(&10), initial_balance + 5*new_session_reward/9);
+		assert_eq!(Balances::total_balance(&10), initial_balance + 5*new_session_reward/9 - approximation);
 		// 10 got 1200 / 2200 external stake => 12/22 =? 6/11 => Validator's share = 5/11
 		assert_eq!(Balances::total_balance(&20), initial_balance + 5*new_session_reward/11+ 2);
 
@@ -1655,11 +1663,13 @@ fn bond_with_no_staked_value() {
 
 		start_era(3);
 
+		// Approximation resulting from Perbill conversion
+		let approximation = 1;
 		let reward = Staking::current_session_reward() * 3;
 		// 2 will not get a reward of only 1
 		// 4 will get the rest
-		assert_eq!(Balances::free_balance(&2), initial_balance_2 + 3);
-		assert_eq!(Balances::free_balance(&4), initial_balance_4 + reward - 3);
+		assert_eq!(Balances::free_balance(&2), initial_balance_2 + 3 - approximation);
+		assert_eq!(Balances::free_balance(&4), initial_balance_4 + reward - 3 - approximation);
 	});
 }
 
@@ -1975,3 +1985,36 @@ fn phragmen_large_scale_test_2() {
 		assert_total_expo(5, nom_budget / 2 + c_budget);
 	})
 }
+
+#[test]
+fn reward_validator_slashing_validator_doesnt_overflow() {
+	with_externalities(&mut ExtBuilder::default()
+		.build(),
+	|| {
+		let stake = u32::max_value() as u64 * 2;
+		let reward_slash = u32::max_value() as u64 * 2;
+
+		// Assert multiplication overflows in balance arithmetic.
+		assert!(stake.checked_mul(reward_slash).is_none());
+
+		// Set staker
+		let _ = Balances::make_free_balance_be(&11, stake);
+		<Stakers<Test>>::insert(&11, Exposure { total: stake, own: stake, others: vec![] });
+
+		// Check reward
+		Staking::reward_validator(&11, reward_slash);
+		assert_eq!(Balances::total_balance(&11), stake * 2);
+
+		// Set staker
+		let _ = Balances::make_free_balance_be(&11, stake);
+		let _ = Balances::make_free_balance_be(&2, stake);
+		<Stakers<Test>>::insert(&11, Exposure { total: stake, own: 1, others: vec![
+			IndividualExposure { who: 2, value: stake - 1 }
+		]});
+
+		// Check slashing
+		Staking::slash_validator(&11, reward_slash);
+		assert_eq!(Balances::total_balance(&11), stake - 1);
+		assert_eq!(Balances::total_balance(&2), 1);
+	})
+}