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

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

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