From 3555ef425d5b221251b4f1370ed9277f05f8787f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Silva?= <andre.beat@gmail.com>
Date: Thu, 10 Jan 2019 15:03:15 +0000
Subject: [PATCH] Improve offline slashing calculation (#1371)
* srml: apply slashing for all offline reports at once
* srml: add regression test for slashing overflow
---
substrate/srml/staking/src/lib.rs | 87 +++++++++++++++++------------
substrate/srml/staking/src/tests.rs | 39 +++++++++++++
2 files changed, 89 insertions(+), 37 deletions(-)
diff --git a/substrate/srml/staking/src/lib.rs b/substrate/srml/staking/src/lib.rs
index 527c916008a..70f71de1527 100644
--- a/substrate/srml/staking/src/lib.rs
+++ b/substrate/srml/staking/src/lib.rs
@@ -508,55 +508,68 @@ impl<T: Trait> Module<T> {
/// Call when a validator is determined to be offline. `count` is the
/// number of offences the validator has committed.
pub fn on_offline_validator(v: T::AccountId, count: usize) {
- use primitives::traits::CheckedShl;
+ use primitives::traits::{CheckedAdd, CheckedShl};
// Early exit if validator is invulnerable.
if Self::invulnerables().contains(&v) {
return
}
- for _ in 0..count {
- let slash_count = Self::slash_count(&v);
- <SlashCount<T>>::insert(v.clone(), slash_count + 1);
- let grace = Self::offline_slash_grace();
+ let slash_count = Self::slash_count(&v);
+ let new_slash_count = slash_count + count as u32;
+ <SlashCount<T>>::insert(v.clone(), new_slash_count);
+ let grace = Self::offline_slash_grace();
- let event = if slash_count >= grace {
+ let event = if new_slash_count > grace {
+ let slash = {
+ let base_slash = Self::current_offline_slash();
let instances = slash_count - grace;
- let base_slash = Self::current_offline_slash();
- let slash = match base_slash.checked_shl(instances) {
- Some(slash) => slash,
- None => {
- // freeze at last maximum valid slash if this starts
- // to overflow.
- <SlashCount<T>>::insert(v.clone(), slash_count);
- base_slash.checked_shl(instances - 1)
- .expect("slash count no longer incremented after overflow; \
- prior check only fails with instances >= 1; \
- thus instances - 1 always works and is a valid amount of bits; qed")
+ let mut total_slash = T::Balance::default();
+ for i in instances..(instances + count as u32) {
+ if let Some(total) = base_slash.checked_shl(i)
+ .and_then(|slash| total_slash.checked_add(&slash)) {
+ total_slash = total;
+ } else {
+ // reset slash count only up to the current
+ // instance. the total slash overflows the unit for
+ // balance in the system therefore we can slash all
+ // the slashable balance for the account
+ <SlashCount<T>>::insert(v.clone(), slash_count + i);
+ total_slash = Self::slashable_balance(&v);
+ break;
}
- };
-
- let next_slash = slash << 1;
-
- let _ = Self::slash_validator(&v, slash);
- if instances >= Self::validator_preferences(&v).unstake_threshold
- || Self::slashable_balance(&v) < next_slash
- {
- if let Some(pos) = Self::intentions().into_iter().position(|x| &x == &v) {
- Self::apply_unstake(&v, pos)
- .expect("pos derived correctly from Self::intentions(); \
- apply_unstake can only fail if pos wrong; \
- Self::intentions() doesn't change; qed");
- }
- let _ = Self::apply_force_new_era(false);
}
- RawEvent::OfflineSlash(v.clone(), slash)
- } else {
- RawEvent::OfflineWarning(v.clone(), slash_count)
+
+ total_slash
};
- Self::deposit_event(event);
- }
+
+ let _ = Self::slash_validator(&v, slash);
+
+ let next_slash = match slash.checked_shl(1) {
+ Some(slash) => slash,
+ None => Self::slashable_balance(&v),
+ };
+
+ let instances = new_slash_count - grace;
+ if instances > Self::validator_preferences(&v).unstake_threshold
+ || Self::slashable_balance(&v) < next_slash
+ || next_slash <= slash
+ {
+ if let Some(pos) = Self::intentions().into_iter().position(|x| &x == &v) {
+ Self::apply_unstake(&v, pos)
+ .expect("pos derived correctly from Self::intentions(); \
+ apply_unstake can only fail if pos wrong; \
+ Self::intentions() doesn't change; qed");
+ }
+ let _ = Self::apply_force_new_era(false);
+ }
+ RawEvent::OfflineSlash(v.clone(), slash)
+ } else {
+ RawEvent::OfflineWarning(v.clone(), slash_count)
+ };
+
+ Self::deposit_event(event);
}
}
diff --git a/substrate/srml/staking/src/tests.rs b/substrate/srml/staking/src/tests.rs
index 801c15d3e12..fc3209bae34 100644
--- a/substrate/srml/staking/src/tests.rs
+++ b/substrate/srml/staking/src/tests.rs
@@ -557,3 +557,42 @@ fn slash_value_calculation_does_not_overflow() {
Staking::on_offline_validator(10, 100);
});
}
+
+#[test]
+fn next_slash_value_calculation_does_not_overflow() {
+ with_externalities(&mut new_test_ext(0, 3, 3, 0, true, 10), || {
+ assert_eq!(Staking::era_length(), 9);
+ assert_eq!(Staking::sessions_per_era(), 3);
+ assert_eq!(Staking::last_era_length_change(), 0);
+ assert_eq!(Staking::current_era(), 0);
+ assert_eq!(Session::current_index(), 0);
+ assert_eq!(Balances::total_balance(&10), 1);
+ assert_eq!(Staking::intentions(), vec![10, 20]);
+ assert_eq!(Staking::offline_slash_grace(), 0);
+
+ // set validator preferences so the validator doesn't back down after
+ // slashing.
+ <ValidatorPreferences<Test>>::insert(10, ValidatorPrefs {
+ unstake_threshold: u32::max_value(),
+ validator_payment: 0,
+ });
+
+ // we have enough balance to cover the last slash before overflow
+ Balances::set_free_balance(&10, u64::max_value());
+ assert_eq!(Balances::total_balance(&10), u64::max_value());
+
+ // the balance type is u64, so after slashing 64 times,
+ // the slash value should have overflowed. add a couple extra for
+ // good measure with the slash grace.
+ trait TypeEq {}
+ impl<A> TypeEq for (A, A) {}
+ fn assert_type_eq<A: TypeEq>() {}
+ assert_type_eq::<(u64, <Test as balances::Trait>::Balance)>();
+
+ // the total slash value should overflow the balance type
+ // therefore the total validator balance should be slashed
+ Staking::on_offline_validator(10, 100);
+
+ assert_eq!(Balances::total_balance(&10), 0);
+ });
+}
--
GitLab