From cf7dbe8151ffb9642e1334d4e5500e0d8eb8d76b Mon Sep 17 00:00:00 2001
From: thiolliere <gui.thiolliere@gmail.com>
Date: Mon, 24 Jun 2019 13:23:54 +0200
Subject: [PATCH] Staking module: approximate fraction into perbill to avoid
overflow (#2889)
* approximate fraction into perbill
* test
* fix comment
* line width
* bump impl version
* rename test for better naming
* test overflow
* Apply suggestions from code review
Co-Authored-By: Niklas Adolfsson <niklasadolfsson1@gmail.com>
---
substrate/core/sr-primitives/src/lib.rs | 66 ++++++++++++++++++++-----
substrate/node/runtime/src/lib.rs | 2 +-
substrate/srml/staking/src/lib.rs | 16 +++---
substrate/srml/staking/src/tests.rs | 53 ++++++++++++++++++--
4 files changed, 112 insertions(+), 25 deletions(-)
diff --git a/substrate/core/sr-primitives/src/lib.rs b/substrate/core/sr-primitives/src/lib.rs
index 1ef8bc22757..007010bc53d 100644
--- a/substrate/core/sr-primitives/src/lib.rs
+++ b/substrate/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 fraction `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 fraction `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]
diff --git a/substrate/node/runtime/src/lib.rs b/substrate/node/runtime/src/lib.rs
index d7530628295..cff2e39b4cd 100644
--- a/substrate/node/runtime/src/lib.rs
+++ b/substrate/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: 97,
- impl_version: 98,
+ impl_version: 99,
apis: RUNTIME_API_VERSIONS,
};
diff --git a/substrate/srml/staking/src/lib.rs b/substrate/srml/staking/src/lib.rs
index 086f78bd533..c1b42a8b3bc 100644
--- a/substrate/srml/staking/src/lib.rs
+++ b/substrate/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);
diff --git a/substrate/srml/staking/src/tests.rs b/substrate/srml/staking/src/tests.rs
index 180ce130ae4..0643cd15cf5 100644
--- a/substrate/srml/staking/src/tests.rs
+++ b/substrate/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);
+ })
+}
--
GitLab