Offence implementations can disable offenders independently from slashing (#10201)

* Offence implementations can disable offenders independently from slashing

* Fix build on CI

* Run cargo fmt

* Fixes based on review comments

* Make parameter naming consistent

* Fix migration and some English

* Fix migration - again

* cargo fmt

* Cover 2 new cases with a test

substrate/frame/staking/src/mock.rs

@@ -33,7 +33,7 @@ use sp_runtime::{
 	testing::{Header, TestXt, UintAuthorityId},
 	traits::{IdentityLookup, Zero},
 };
-use sp_staking::offence::{OffenceDetails, OnOffenceHandler};
+use sp_staking::offence::{DisableStrategy, OffenceDetails, OnOffenceHandler};
 use std::cell::RefCell;
 
 pub const INIT_TIMESTAMP: u64 = 30_000;
@@ -765,11 +765,12 @@ pub(crate) fn on_offence_in_era(
 	>],
 	slash_fraction: &[Perbill],
 	era: EraIndex,
+	disable_strategy: DisableStrategy,
 ) {
 	let bonded_eras = crate::BondedEras::<Test>::get();
 	for &(bonded_era, start_session) in bonded_eras.iter() {
 		if bonded_era == era {
-			let _ = Staking::on_offence(offenders, slash_fraction, start_session);
+			let _ = Staking::on_offence(offenders, slash_fraction, start_session, disable_strategy);
 			return
 		} else if bonded_era > era {
 			break
@@ -781,6 +782,7 @@ pub(crate) fn on_offence_in_era(
 			offenders,
 			slash_fraction,
 			Staking::eras_start_session_index(era).unwrap(),
+			disable_strategy,
 		);
 	} else {
 		panic!("cannot slash in era {}", era);
@@ -795,7 +797,7 @@ pub(crate) fn on_offence_now(
 	slash_fraction: &[Perbill],
 ) {
 	let now = Staking::active_era().unwrap().index;
-	on_offence_in_era(offenders, slash_fraction, now)
+	on_offence_in_era(offenders, slash_fraction, now, DisableStrategy::WhenSlashed)
 }
 
 pub(crate) fn add_slash(who: &AccountId) {

substrate/frame/staking/src/pallet/impls.rs

@@ -36,7 +36,7 @@ use sp_runtime::{
 	Perbill,
 };
 use sp_staking::{
-	offence::{OffenceDetails, OnOffenceHandler},
+	offence::{DisableStrategy, OffenceDetails, OnOffenceHandler},
 	SessionIndex,
 };
 use sp_std::{collections::btree_map::BTreeMap, prelude::*};
@@ -1137,6 +1137,7 @@ where
 		>],
 		slash_fraction: &[Perbill],
 		slash_session: SessionIndex,
+		disable_strategy: DisableStrategy,
 	) -> Weight {
 		let reward_proportion = SlashRewardFraction::<T>::get();
 		let mut consumed_weight: Weight = 0;
@@ -1206,6 +1207,7 @@ where
 				window_start,
 				now: active_era,
 				reward_proportion,
+				disable_strategy,
 			});
 
 			if let Some(mut unapplied) = unapplied {

substrate/frame/staking/src/slashing.rs

@@ -63,6 +63,7 @@ use sp_runtime::{
 	traits::{Saturating, Zero},
 	DispatchResult, RuntimeDebug,
 };
+use sp_staking::offence::DisableStrategy;
 use sp_std::vec::Vec;
 
 /// The proportion of the slashing reward to be paid out on the first slashing detection.
@@ -213,6 +214,8 @@ pub(crate) struct SlashParams<'a, T: 'a + Config> {
 	/// The maximum percentage of a slash that ever gets paid out.
 	/// This is f_inf in the paper.
 	pub(crate) reward_proportion: Perbill,
+	/// When to disable offenders.
+	pub(crate) disable_strategy: DisableStrategy,
 }
 
 /// Computes a slash of a validator and nominators. It returns an unapplied
@@ -224,15 +227,12 @@ pub(crate) struct SlashParams<'a, T: 'a + Config> {
 pub(crate) fn compute_slash<T: Config>(
 	params: SlashParams<T>,
 ) -> Option<UnappliedSlash<T::AccountId, BalanceOf<T>>> {
-	let SlashParams { stash, slash, exposure, slash_era, window_start, now, reward_proportion } =
-		params.clone();
-
 	let mut reward_payout = Zero::zero();
 	let mut val_slashed = Zero::zero();
 
 	// is the slash amount here a maximum for the era?
-	let own_slash = slash * exposure.own;
-	if slash * exposure.total == Zero::zero() {
+	let own_slash = params.slash * params.exposure.own;
+	if params.slash * params.exposure.total == Zero::zero() {
 		// kick out the validator even if they won't be slashed,
 		// as long as the misbehavior is from their most recent slashing span.
 		kick_out_if_recent::<T>(params);
@@ -240,13 +240,17 @@ pub(crate) fn compute_slash<T: Config>(
 	}
 
 	let (prior_slash_p, _era_slash) =
-		<Pallet<T> as Store>::ValidatorSlashInEra::get(&slash_era, stash)
+		<Pallet<T> as Store>::ValidatorSlashInEra::get(&params.slash_era, params.stash)
 			.unwrap_or((Perbill::zero(), Zero::zero()));
 
 	// compare slash proportions rather than slash values to avoid issues due to rounding
 	// error.
-	if slash.deconstruct() > prior_slash_p.deconstruct() {
-		<Pallet<T> as Store>::ValidatorSlashInEra::insert(&slash_era, stash, &(slash, own_slash));
+	if params.slash.deconstruct() > prior_slash_p.deconstruct() {
+		<Pallet<T> as Store>::ValidatorSlashInEra::insert(
+			&params.slash_era,
+			params.stash,
+			&(params.slash, own_slash),
+		);
 	} else {
 		// we slash based on the max in era - this new event is not the max,
 		// so neither the validator or any nominators will need an update.
@@ -261,14 +265,14 @@ pub(crate) fn compute_slash<T: Config>(
 	// apply slash to validator.
 	{
 		let mut spans = fetch_spans::<T>(
-			stash,
-			window_start,
+			params.stash,
+			params.window_start,
 			&mut reward_payout,
 			&mut val_slashed,
-			reward_proportion,
+			params.reward_proportion,
 		);
 
-		let target_span = spans.compare_and_update_span_slash(slash_era, own_slash);
+		let target_span = spans.compare_and_update_span_slash(params.slash_era, own_slash);
 
 		if target_span == Some(spans.span_index()) {
 			// misbehavior occurred within the current slashing span - take appropriate
@@ -276,20 +280,19 @@ pub(crate) fn compute_slash<T: Config>(
 
 			// chill the validator - it misbehaved in the current span and should
 			// not continue in the next election. also end the slashing span.
-			spans.end_span(now);
-			<Pallet<T>>::chill_stash(stash);
+			spans.end_span(params.now);
+			<Pallet<T>>::chill_stash(params.stash);
 		}
 	}
 
-	// add the validator to the offenders list and make sure it is disabled for
-	// the duration of the era
-	add_offending_validator::<T>(params.stash, true);
+	let disable_when_slashed = params.disable_strategy != DisableStrategy::Never;
+	add_offending_validator::<T>(params.stash, disable_when_slashed);
 
 	let mut nominators_slashed = Vec::new();
-	reward_payout += slash_nominators::<T>(params, prior_slash_p, &mut nominators_slashed);
+	reward_payout += slash_nominators::<T>(params.clone(), prior_slash_p, &mut nominators_slashed);
 
 	Some(UnappliedSlash {
-		validator: stash.clone(),
+		validator: params.stash.clone(),
 		own: val_slashed,
 		others: nominators_slashed,
 		reporters: Vec::new(),
@@ -316,9 +319,8 @@ fn kick_out_if_recent<T: Config>(params: SlashParams<T>) {
 		<Pallet<T>>::chill_stash(params.stash);
 	}
 
-	// add the validator to the offenders list but since there's no slash being
-	// applied there's no need to disable the validator
-	add_offending_validator::<T>(params.stash, false);
+	let disable_without_slash = params.disable_strategy == DisableStrategy::Always;
+	add_offending_validator::<T>(params.stash, disable_without_slash);
 }
 
 /// Add the given validator to the offenders list and optionally disable it.
@@ -371,13 +373,10 @@ fn slash_nominators<T: Config>(
 	prior_slash_p: Perbill,
 	nominators_slashed: &mut Vec<(T::AccountId, BalanceOf<T>)>,
 ) -> BalanceOf<T> {
-	let SlashParams { stash: _, slash, exposure, slash_era, window_start, now, reward_proportion } =
-		params;
-
 	let mut reward_payout = Zero::zero();
 
-	nominators_slashed.reserve(exposure.others.len());
-	for nominator in &exposure.others {
+	nominators_slashed.reserve(params.exposure.others.len());
+	for nominator in &params.exposure.others {
 		let stash = &nominator.who;
 		let mut nom_slashed = Zero::zero();
 
@@ -385,15 +384,16 @@ fn slash_nominators<T: Config>(
 		// had a new max slash for the era.
 		let era_slash = {
 			let own_slash_prior = prior_slash_p * nominator.value;
-			let own_slash_by_validator = slash * nominator.value;
+			let own_slash_by_validator = params.slash * nominator.value;
 			let own_slash_difference = own_slash_by_validator.saturating_sub(own_slash_prior);
 
-			let mut era_slash = <Pallet<T> as Store>::NominatorSlashInEra::get(&slash_era, stash)
-				.unwrap_or_else(|| Zero::zero());
+			let mut era_slash =
+				<Pallet<T> as Store>::NominatorSlashInEra::get(&params.slash_era, stash)
+					.unwrap_or_else(|| Zero::zero());
 
 			era_slash += own_slash_difference;
 
-			<Pallet<T> as Store>::NominatorSlashInEra::insert(&slash_era, stash, &era_slash);
+			<Pallet<T> as Store>::NominatorSlashInEra::insert(&params.slash_era, stash, &era_slash);
 
 			era_slash
 		};
@@ -402,18 +402,18 @@ fn slash_nominators<T: Config>(
 		{
 			let mut spans = fetch_spans::<T>(
 				stash,
-				window_start,
+				params.window_start,
 				&mut reward_payout,
 				&mut nom_slashed,
-				reward_proportion,
+				params.reward_proportion,
 			);
 
-			let target_span = spans.compare_and_update_span_slash(slash_era, era_slash);
+			let target_span = spans.compare_and_update_span_slash(params.slash_era, era_slash);
 
 			if target_span == Some(spans.span_index()) {
 				// End the span, but don't chill the nominator. its nomination
 				// on this validator will be ignored in the future.
-				spans.end_span(now);
+				spans.end_span(params.now);
 			}
 		}
 

substrate/frame/staking/src/tests.rs

@@ -34,7 +34,7 @@ use sp_runtime::{
 	Perbill, Percent,
 };
 use sp_staking::{
-	offence::{OffenceDetails, OnOffenceHandler},
+	offence::{DisableStrategy, OffenceDetails, OnOffenceHandler},
 	SessionIndex,
 };
 use sp_std::prelude::*;
@@ -2250,6 +2250,7 @@ fn slash_in_old_span_does_not_deselect() {
 			}],
 			&[Perbill::from_percent(0)],
 			1,
+			DisableStrategy::WhenSlashed,
 		);
 
 		// the validator doesn't get chilled again
@@ -2266,6 +2267,7 @@ fn slash_in_old_span_does_not_deselect() {
 			// NOTE: A 100% slash here would clean up the account, causing de-registration.
 			&[Perbill::from_percent(95)],
 			1,
+			DisableStrategy::WhenSlashed,
 		);
 
 		// the validator doesn't get chilled again
@@ -2562,6 +2564,7 @@ fn slashing_nominators_by_span_max() {
 			}],
 			&[Perbill::from_percent(10)],
 			2,
+			DisableStrategy::WhenSlashed,
 		);
 
 		assert_eq!(Balances::free_balance(11), 900);
@@ -2588,6 +2591,7 @@ fn slashing_nominators_by_span_max() {
 			}],
 			&[Perbill::from_percent(30)],
 			3,
+			DisableStrategy::WhenSlashed,
 		);
 
 		// 11 was not further slashed, but 21 and 101 were.
@@ -2609,6 +2613,7 @@ fn slashing_nominators_by_span_max() {
 			}],
 			&[Perbill::from_percent(20)],
 			2,
+			DisableStrategy::WhenSlashed,
 		);
 
 		// 11 was further slashed, but 21 and 101 were not.
@@ -2744,6 +2749,7 @@ fn remove_deferred() {
 			&[OffenceDetails { offender: (11, exposure.clone()), reporters: vec![] }],
 			&[Perbill::from_percent(15)],
 			1,
+			DisableStrategy::WhenSlashed,
 		);
 
 		// fails if empty
@@ -2933,6 +2939,40 @@ fn non_slashable_offence_doesnt_disable_validator() {
 	});
 }
 
+#[test]
+fn slashing_independent_of_disabling_validator() {
+	ExtBuilder::default().build_and_execute(|| {
+		mock::start_active_era(1);
+		assert_eq_uvec!(Session::validators(), vec![11, 21]);
+
+		let exposure_11 = Staking::eras_stakers(Staking::active_era().unwrap().index, &11);
+		let exposure_21 = Staking::eras_stakers(Staking::active_era().unwrap().index, &21);
+
+		let now = Staking::active_era().unwrap().index;
+
+		// offence with no slash associated, BUT disabling
+		on_offence_in_era(
+			&[OffenceDetails { offender: (11, exposure_11.clone()), reporters: vec![] }],
+			&[Perbill::zero()],
+			now,
+			DisableStrategy::Always,
+		);
+
+		// offence that slashes 25% of the bond, BUT not disabling
+		on_offence_in_era(
+			&[OffenceDetails { offender: (21, exposure_21.clone()), reporters: vec![] }],
+			&[Perbill::from_percent(25)],
+			now,
+			DisableStrategy::Never,
+		);
+
+		// the offence for validator 10 was explicitly disabled
+		assert!(is_disabled(10));
+		// whereas validator 20 is explicitly not disabled
+		assert!(!is_disabled(20));
+	});
+}
+
 #[test]
 fn offence_threshold_triggers_new_era() {
 	ExtBuilder::default()
@@ -3595,7 +3635,7 @@ fn offences_weight_calculated_correctly() {
 	ExtBuilder::default().nominate(true).build_and_execute(|| {
 		// On offence with zero offenders: 4 Reads, 1 Write
 		let zero_offence_weight = <Test as frame_system::Config>::DbWeight::get().reads_writes(4, 1);
-		assert_eq!(Staking::on_offence(&[], &[Perbill::from_percent(50)], 0), zero_offence_weight);
+		assert_eq!(Staking::on_offence(&[], &[Perbill::from_percent(50)], 0, DisableStrategy::WhenSlashed), zero_offence_weight);
 
 		// On Offence with N offenders, Unapplied: 4 Reads, 1 Write + 4 Reads, 5 Writes
 		let n_offence_unapplied_weight = <Test as frame_system::Config>::DbWeight::get().reads_writes(4, 1)
@@ -3608,7 +3648,7 @@ fn offences_weight_calculated_correctly() {
 					reporters: vec![],
 				}
 			).collect();
-		assert_eq!(Staking::on_offence(&offenders, &[Perbill::from_percent(50)], 0), n_offence_unapplied_weight);
+		assert_eq!(Staking::on_offence(&offenders, &[Perbill::from_percent(50)], 0, DisableStrategy::WhenSlashed), n_offence_unapplied_weight);
 
 		// On Offence with one offenders, Applied
 		let one_offender = [
@@ -3629,7 +3669,7 @@ fn offences_weight_calculated_correctly() {
 			// `reward_cost` * reporters (1)
 			+ <Test as frame_system::Config>::DbWeight::get().reads_writes(2, 2);
 
-		assert_eq!(Staking::on_offence(&one_offender, &[Perbill::from_percent(50)], 0), one_offence_unapplied_weight);
+		assert_eq!(Staking::on_offence(&one_offender, &[Perbill::from_percent(50)], 0, DisableStrategy::WhenSlashed), one_offence_unapplied_weight);
 	});
 }