Implementation of the new validator disabling strategy (#2226)

Closes https://github.com/paritytech/polkadot-sdk/issues/1966,
https://github.com/paritytech/polkadot-sdk/issues/1963 and
https://github.com/paritytech/polkadot-sdk/issues/1962.

Disabling strategy specification
[here](https://github.com/paritytech/polkadot-sdk/pull/2955). (Updated
13/02/2024)

Implements:
* validator disabling for a whole era instead of just a session
* no more than 1/3 of the validators in the active set are disabled
Removes:
* `DisableStrategy` enum - now each validator committing an offence is
disabled.
* New era is not forced if too many validators are disabled.

Before this PR not all offenders were disabled. A decision was made
based on [`enum
DisableStrategy`](https://github.com/paritytech/polkadot-sdk/blob/bbb6631641f9adba30c0ee6f4d11023a424dd362/substrate/primitives/staking/src/offence.rs#L54).
Some offenders were disabled for a whole era, some just for a session,
some were not disabled at all.

This PR changes the disabling behaviour. Now a validator committing an
offense is disabled immediately till the end of the current era.

Some implementation notes:
* `OffendingValidators` in pallet session keeps all offenders (this is
not changed). However its type is changed from `Vec<(u32, bool)>` to
`Vec<u32>`. The reason is simple - each offender is getting disabled so
the bool doesn't make sense anymore.
* When a validator is disabled it is first added to
`OffendingValidators` and then to `DisabledValidators`. This is done in
[`add_offending_validator`](https://github.com/paritytech/polkadot-sdk/blob/bbb6631641f9adba30c0ee6f4d11023a424dd362/substrate/frame/staking/src/slashing.rs#L325)
from staking pallet.
* In
[`rotate_session`](https://github.com/paritytech/polkadot-sdk/blob/bdbe98297032e21a553bf191c530690b1d591405/substrate/frame/session/src/lib.rs#L623)
the `end_session` also calls
[`end_era`](https://github.com/paritytech/polkadot-sdk/blob/bbb6631641f9adba30c0ee6f4d11023a424dd362/substrate/frame/staking/src/pallet/impls.rs#L490)
when an era ends. In this case `OffendingValidators` are cleared
**(1)**.
* Then in
[`rotate_session`](https://github.com/paritytech/polkadot-sdk/blob/bdbe98297032e21a553bf191c530690b1d591405/substrate/frame/session/src/lib.rs#L623)
`DisabledValidators` are cleared **(2)**
* And finally (still in `rotate_session`) a call to
[`start_session`](https://github.com/paritytech/polkadot-sdk/blob/bbb6631641f9adba30c0ee6f4d11023a424dd362/substrate/frame/staking/src/pallet/impls.rs#L430)
repopulates the disabled validators **(3)**.
* The reason for this complication is that session pallet knows nothing
abut eras. To overcome this on each new session the disabled list is
repopulated (points 2 and 3). Staking pallet knows when a new era starts
so with point 1 it ensures that the offenders list is cleared.

---------

Co-authored-by: ordian <noreply@reusable.software>
Co-authored-by: ordian <write@reusable.software>
Co-authored-by: Maciej <maciej.zyszkiewicz@parity.io>
Co-authored-by: Gonçalo Pestana <g6pestana@gmail.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: command-bot <>
Co-authored-by: Ankan <10196091+Ank4n@users.noreply.github.com>

substrate/frame/staking/CHANGELOG.md

@@ -7,6 +7,25 @@ on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). We maintain a
 single integer version number for staking pallet to keep track of all storage
 migrations.
 
+## [v15]
+
+### Added
+
+- New trait `DisablingStrategy` which is responsible for making a decision which offenders should be
+  disabled on new offence.
+- Default implementation of `DisablingStrategy` - `UpToLimitDisablingStrategy`. It
+  disables each new offender up to a threshold (1/3 by default). Offenders are not runtime disabled for
+  offences in previous era(s). But they will be low-priority node-side disabled for dispute initiation.
+- `OffendingValidators` storage item is replaced with `DisabledValidators`. The former keeps all
+  offenders and if they are disabled or not. The latter just keeps a list of all offenders as they
+  are disabled by default.
+
+### Deprecated
+
+- `enum DisableStrategy` is no longer needed because disabling is not related to the type of the
+  offence anymore. A decision if a offender is disabled or not is made by a `DisablingStrategy`
+  implementation.
+
 ## [v14]
 
 ### Added

substrate/frame/staking/src/lib.rs

@@ -1239,3 +1239,79 @@ impl BenchmarkingConfig for TestBenchmarkingConfig {
 	type MaxValidators = frame_support::traits::ConstU32<100>;
 	type MaxNominators = frame_support::traits::ConstU32<100>;
 }
+
+/// Controls validator disabling
+pub trait DisablingStrategy<T: Config> {
+	/// Make a disabling decision. Returns the index of the validator to disable or `None` if no new
+	/// validator should be disabled.
+	fn decision(
+		offender_stash: &T::AccountId,
+		slash_era: EraIndex,
+		currently_disabled: &Vec<u32>,
+	) -> Option<u32>;
+}
+
+/// Implementation of [`DisablingStrategy`] which disables validators from the active set up to a
+/// threshold. `DISABLING_LIMIT_FACTOR` is the factor of the maximum disabled validators in the
+/// active set. E.g. setting this value to `3` means no more than 1/3 of the validators in the
+/// active set can be disabled in an era.
+/// By default a factor of 3 is used which is the byzantine threshold.
+pub struct UpToLimitDisablingStrategy<const DISABLING_LIMIT_FACTOR: usize = 3>;
+
+impl<const DISABLING_LIMIT_FACTOR: usize> UpToLimitDisablingStrategy<DISABLING_LIMIT_FACTOR> {
+	/// Disabling limit calculated from the total number of validators in the active set. When
+	/// reached no more validators will be disabled.
+	pub fn disable_limit(validators_len: usize) -> usize {
+		validators_len
+			.saturating_sub(1)
+			.checked_div(DISABLING_LIMIT_FACTOR)
+			.unwrap_or_else(|| {
+				defensive!("DISABLING_LIMIT_FACTOR should not be 0");
+				0
+			})
+	}
+}
+
+impl<T: Config, const DISABLING_LIMIT_FACTOR: usize> DisablingStrategy<T>
+	for UpToLimitDisablingStrategy<DISABLING_LIMIT_FACTOR>
+{
+	fn decision(
+		offender_stash: &T::AccountId,
+		slash_era: EraIndex,
+		currently_disabled: &Vec<u32>,
+	) -> Option<u32> {
+		let active_set = T::SessionInterface::validators();
+
+		// We don't disable more than the limit
+		if currently_disabled.len() >= Self::disable_limit(active_set.len()) {
+			log!(
+				debug,
+				"Won't disable: reached disabling limit {:?}",
+				Self::disable_limit(active_set.len())
+			);
+			return None
+		}
+
+		// We don't disable for offences in previous eras
+		if ActiveEra::<T>::get().map(|e| e.index).unwrap_or_default() > slash_era {
+			log!(
+				debug,
+				"Won't disable: current_era {:?} > slash_era {:?}",
+				Pallet::<T>::current_era().unwrap_or_default(),
+				slash_era
+			);
+			return None
+		}
+
+		let offender_idx = if let Some(idx) = active_set.iter().position(|i| i == offender_stash) {
+			idx as u32
+		} else {
+			log!(debug, "Won't disable: offender not in active set",);
+			return None
+		};
+
+		log!(debug, "Will disable {:?}", offender_idx);
+
+		Some(offender_idx)
+	}
+}

substrate/frame/staking/src/migrations.rs

@@ -20,9 +20,10 @@
 use super::*;
 use frame_election_provider_support::SortedListProvider;
 use frame_support::{
+	migrations::VersionedMigration,
 	pallet_prelude::ValueQuery,
 	storage_alias,
-	traits::{GetStorageVersion, OnRuntimeUpgrade},
+	traits::{GetStorageVersion, OnRuntimeUpgrade, UncheckedOnRuntimeUpgrade},
 };
 
 #[cfg(feature = "try-runtime")]
@@ -59,11 +60,61 @@ impl Default for ObsoleteReleases {
 #[storage_alias]
 type StorageVersion<T: Config> = StorageValue<Pallet<T>, ObsoleteReleases, ValueQuery>;
 
+/// Migrating `OffendingValidators` from `Vec<(u32, bool)>` to `Vec<u32>`
+pub mod v15 {
+	use super::*;
+
+	// The disabling strategy used by staking pallet
+	type DefaultDisablingStrategy = UpToLimitDisablingStrategy;
+
+	pub struct VersionUncheckedMigrateV14ToV15<T>(sp_std::marker::PhantomData<T>);
+	impl<T: Config> UncheckedOnRuntimeUpgrade for VersionUncheckedMigrateV14ToV15<T> {
+		fn on_runtime_upgrade() -> Weight {
+			let mut migrated = v14::OffendingValidators::<T>::take()
+				.into_iter()
+				.filter(|p| p.1) // take only disabled validators
+				.map(|p| p.0)
+				.collect::<Vec<_>>();
+
+			// Respect disabling limit
+			migrated.truncate(DefaultDisablingStrategy::disable_limit(
+				T::SessionInterface::validators().len(),
+			));
+
+			DisabledValidators::<T>::set(migrated);
+
+			log!(info, "v15 applied successfully.");
+			T::DbWeight::get().reads_writes(1, 1)
+		}
+
+		#[cfg(feature = "try-runtime")]
+		fn post_upgrade(_state: Vec<u8>) -> Result<(), TryRuntimeError> {
+			frame_support::ensure!(
+				v14::OffendingValidators::<T>::decode_len().is_none(),
+				"OffendingValidators is not empty after the migration"
+			);
+			Ok(())
+		}
+	}
+
+	pub type MigrateV14ToV15<T> = VersionedMigration<
+		14,
+		15,
+		VersionUncheckedMigrateV14ToV15<T>,
+		Pallet<T>,
+		<T as frame_system::Config>::DbWeight,
+	>;
+}
+
 /// Migration of era exposure storage items to paged exposures.
 /// Changelog: [v14.](https://github.com/paritytech/substrate/blob/ankan/paged-rewards-rebased2/frame/staking/CHANGELOG.md#14)
 pub mod v14 {
 	use super::*;
 
+	#[frame_support::storage_alias]
+	pub(crate) type OffendingValidators<T: Config> =
+		StorageValue<Pallet<T>, Vec<(u32, bool)>, ValueQuery>;
+
 	pub struct MigrateToV14<T>(core::marker::PhantomData<T>);
 	impl<T: Config> OnRuntimeUpgrade for MigrateToV14<T> {
 		fn on_runtime_upgrade() -> Weight {
@@ -73,10 +124,10 @@ pub mod v14 {
 			if in_code == 14 && on_chain == 13 {
 				in_code.put::<Pallet<T>>();
 
-				log!(info, "v14 applied successfully.");
+				log!(info, "staking v14 applied successfully.");
 				T::DbWeight::get().reads_writes(1, 1)
 			} else {
-				log!(warn, "v14 not applied.");
+				log!(warn, "staking v14 not applied.");
 				T::DbWeight::get().reads(1)
 			}
 		}

substrate/frame/staking/src/mock.rs

@@ -34,7 +34,7 @@ use frame_system::{EnsureRoot, EnsureSignedBy};
 use sp_io;
 use sp_runtime::{curve::PiecewiseLinear, testing::UintAuthorityId, traits::Zero, BuildStorage};
 use sp_staking::{
-	offence::{DisableStrategy, OffenceDetails, OnOffenceHandler},
+	offence::{OffenceDetails, OnOffenceHandler},
 	OnStakingUpdate,
 };
 
@@ -186,7 +186,6 @@ pallet_staking_reward_curve::build! {
 parameter_types! {
 	pub const BondingDuration: EraIndex = 3;
 	pub const RewardCurve: &'static PiecewiseLinear<'static> = &I_NPOS;
-	pub const OffendingValidatorsThreshold: Perbill = Perbill::from_percent(75);
 }
 
 parameter_types! {
@@ -267,6 +266,9 @@ impl OnStakingUpdate<AccountId, Balance> for EventListenerMock {
 	}
 }
 
+// Disabling threshold for `UpToLimitDisablingStrategy`
+pub(crate) const DISABLING_LIMIT_FACTOR: usize = 3;
+
 impl crate::pallet::pallet::Config for Test {
 	type Currency = Balances;
 	type CurrencyBalance = <Self as pallet_balances::Config>::Balance;
@@ -284,7 +286,6 @@ impl crate::pallet::pallet::Config for Test {
 	type EraPayout = ConvertCurve<RewardCurve>;
 	type NextNewSession = Session;
 	type MaxExposurePageSize = MaxExposurePageSize;
-	type OffendingValidatorsThreshold = OffendingValidatorsThreshold;
 	type ElectionProvider = onchain::OnChainExecution<OnChainSeqPhragmen>;
 	type GenesisElectionProvider = Self::ElectionProvider;
 	// NOTE: consider a macro and use `UseNominatorsAndValidatorsMap<Self>` as well.
@@ -297,6 +298,7 @@ impl crate::pallet::pallet::Config for Test {
 	type EventListeners = EventListenerMock;
 	type BenchmarkingConfig = TestBenchmarkingConfig;
 	type WeightInfo = ();
+	type DisablingStrategy = pallet_staking::UpToLimitDisablingStrategy<DISABLING_LIMIT_FACTOR>;
 }
 
 pub struct WeightedNominationsQuota<const MAX: u32>;
@@ -461,6 +463,8 @@ impl ExtBuilder {
 				(31, self.balance_factor * 2000),
 				(41, self.balance_factor * 2000),
 				(51, self.balance_factor * 2000),
+				(201, self.balance_factor * 2000),
+				(202, self.balance_factor * 2000),
 				// optional nominator
 				(100, self.balance_factor * 2000),
 				(101, self.balance_factor * 2000),
@@ -488,8 +492,10 @@ impl ExtBuilder {
 				(31, 31, self.balance_factor * 500, StakerStatus::<AccountId>::Validator),
 				// an idle validator
 				(41, 41, self.balance_factor * 1000, StakerStatus::<AccountId>::Idle),
-			];
-			// optionally add a nominator
+				(51, 51, self.balance_factor * 1000, StakerStatus::<AccountId>::Idle),
+				(201, 201, self.balance_factor * 1000, StakerStatus::<AccountId>::Idle),
+				(202, 202, self.balance_factor * 1000, StakerStatus::<AccountId>::Idle),
+			]; // optionally add a nominator
 			if self.nominate {
 				stakers.push((
 					101,
@@ -728,12 +734,11 @@ 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, disable_strategy);
+			let _ = Staking::on_offence(offenders, slash_fraction, start_session);
 			return
 		} else if bonded_era > era {
 			break
@@ -745,7 +750,6 @@ 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);
@@ -760,7 +764,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, DisableStrategy::WhenSlashed)
+	on_offence_in_era(offenders, slash_fraction, now)
 }
 
 pub(crate) fn add_slash(who: &AccountId) {

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

@@ -43,7 +43,7 @@ use sp_runtime::{
 };
 use sp_staking::{
 	currency_to_vote::CurrencyToVote,
-	offence::{DisableStrategy, OffenceDetails, OnOffenceHandler},
+	offence::{OffenceDetails, OnOffenceHandler},
 	EraIndex, OnStakingUpdate, Page, SessionIndex, Stake,
 	StakingAccount::{self, Controller, Stash},
 	StakingInterface,
@@ -505,10 +505,8 @@ impl<T: Config> Pallet<T> {
 		}
 
 		// disable all offending validators that have been disabled for the whole era
-		for (index, disabled) in <OffendingValidators<T>>::get() {
-			if disabled {
-				T::SessionInterface::disable_validator(index);
-			}
+		for index in <DisabledValidators<T>>::get() {
+			T::SessionInterface::disable_validator(index);
 		}
 	}
 
@@ -598,8 +596,8 @@ impl<T: Config> Pallet<T> {
 			<ErasValidatorReward<T>>::insert(&active_era.index, validator_payout);
 			T::RewardRemainder::on_unbalanced(T::Currency::issue(remainder));
 
-			// Clear offending validators.
-			<OffendingValidators<T>>::kill();
+			// Clear disabled validators.
+			<DisabledValidators<T>>::kill();
 		}
 	}
 
@@ -868,14 +866,6 @@ impl<T: Config> Pallet<T> {
 		Self::deposit_event(Event::<T>::ForceEra { mode });
 	}
 
-	/// Ensures that at the end of the current session there will be a new era.
-	pub(crate) fn ensure_new_era() {
-		match ForceEra::<T>::get() {
-			Forcing::ForceAlways | Forcing::ForceNew => (),
-			_ => Self::set_force_era(Forcing::ForceNew),
-		}
-	}
-
 	#[cfg(feature = "runtime-benchmarks")]
 	pub fn add_era_stakers(
 		current_era: EraIndex,
@@ -1447,7 +1437,6 @@ where
 		>],
 		slash_fraction: &[Perbill],
 		slash_session: SessionIndex,
-		disable_strategy: DisableStrategy,
 	) -> Weight {
 		let reward_proportion = SlashRewardFraction::<T>::get();
 		let mut consumed_weight = Weight::from_parts(0, 0);
@@ -1512,7 +1501,6 @@ where
 				window_start,
 				now: active_era,
 				reward_proportion,
-				disable_strategy,
 			});
 
 			Self::deposit_event(Event::<T>::SlashReported {
@@ -1986,7 +1974,8 @@ impl<T: Config> Pallet<T> {
 		Self::check_nominators()?;
 		Self::check_exposures()?;
 		Self::check_paged_exposures()?;
-		Self::check_count()
+		Self::check_count()?;
+		Self::ensure_disabled_validators_sorted()
 	}
 
 	/// Invariants:
@@ -2300,4 +2289,12 @@ impl<T: Config> Pallet<T> {
 
 		Ok(())
 	}
+
+	fn ensure_disabled_validators_sorted() -> Result<(), TryRuntimeError> {
+		ensure!(
+			DisabledValidators::<T>::get().windows(2).all(|pair| pair[0] <= pair[1]),
+			"DisabledValidators is not sorted"
+		);
+		Ok(())
+	}
 }

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

@@ -47,10 +47,11 @@ mod impls;
 pub use impls::*;
 
 use crate::{
-	slashing, weights::WeightInfo, AccountIdLookupOf, ActiveEraInfo, BalanceOf, EraPayout,
-	EraRewardPoints, Exposure, ExposurePage, Forcing, LedgerIntegrityState, MaxNominationsOf,
-	NegativeImbalanceOf, Nominations, NominationsQuota, PositiveImbalanceOf, RewardDestination,
-	SessionInterface, StakingLedger, UnappliedSlash, UnlockChunk, ValidatorPrefs,
+	slashing, weights::WeightInfo, AccountIdLookupOf, ActiveEraInfo, BalanceOf, DisablingStrategy,
+	EraPayout, EraRewardPoints, Exposure, ExposurePage, Forcing, LedgerIntegrityState,
+	MaxNominationsOf, NegativeImbalanceOf, Nominations, NominationsQuota, PositiveImbalanceOf,
+	RewardDestination, SessionInterface, StakingLedger, UnappliedSlash, UnlockChunk,
+	ValidatorPrefs,
 };
 
 // The speculative number of spans are used as an input of the weight annotation of
@@ -67,7 +68,7 @@ pub mod pallet {
 	use super::*;
 
 	/// The in-code storage version.
-	const STORAGE_VERSION: StorageVersion = StorageVersion::new(14);
+	const STORAGE_VERSION: StorageVersion = StorageVersion::new(15);
 
 	#[pallet::pallet]
 	#[pallet::storage_version(STORAGE_VERSION)]
@@ -217,10 +218,6 @@ pub mod pallet {
 		#[pallet::constant]
 		type MaxExposurePageSize: Get<u32>;
 
-		/// The fraction of the validator set that is safe to be offending.
-		/// After the threshold is reached a new era will be forced.
-		type OffendingValidatorsThreshold: Get<Perbill>;
-
 		/// Something that provides a best-effort sorted list of voters aka electing nominators,
 		/// used for NPoS election.
 		///
@@ -278,6 +275,9 @@ pub mod pallet {
 		/// WARNING: this only reports slashing and withdraw events for the time being.
 		type EventListeners: sp_staking::OnStakingUpdate<Self::AccountId, BalanceOf<Self>>;
 
+		// `DisablingStragegy` controls how validators are disabled
+		type DisablingStrategy: DisablingStrategy<Self>;
+
 		/// Some parameters of the benchmarking.
 		type BenchmarkingConfig: BenchmarkingConfig;
 
@@ -654,19 +654,16 @@ pub mod pallet {
 	#[pallet::getter(fn current_planned_session)]
 	pub type CurrentPlannedSession<T> = StorageValue<_, SessionIndex, ValueQuery>;
 
-	/// Indices of validators that have offended in the active era and whether they are currently
-	/// disabled.
+	/// Indices of validators that have offended in the active era. The offenders are disabled for a
+	/// whole era. For this reason they are kept here - only staking pallet knows about eras. The
+	/// implementor of [`DisablingStrategy`] defines if a validator should be disabled which
+	/// implicitly means that the implementor also controls the max number of disabled validators.
 	///
-	/// This value should be a superset of disabled validators since not all offences lead to the
-	/// validator being disabled (if there was no slash). This is needed to track the percentage of
-	/// validators that have offended in the current era, ensuring a new era is forced if
-	/// `OffendingValidatorsThreshold` is reached. The vec is always kept sorted so that we can find
-	/// whether a given validator has previously offended using binary search. It gets cleared when
-	/// the era ends.
+	/// The vec is always kept sorted so that we can find whether a given validator has previously
+	/// offended using binary search.
 	#[pallet::storage]
 	#[pallet::unbounded]
-	#[pallet::getter(fn offending_validators)]
-	pub type OffendingValidators<T: Config> = StorageValue<_, Vec<(u32, bool)>, ValueQuery>;
+	pub type DisabledValidators<T: Config> = StorageValue<_, Vec<u32>, ValueQuery>;
 
 	/// The threshold for when users can start calling `chill_other` for other validators /
 	/// nominators. The threshold is compared to the actual number of validators / nominators

substrate/frame/staking/src/slashing.rs

@@ -50,21 +50,21 @@
 //! Based on research at <https://research.web3.foundation/en/latest/polkadot/slashing/npos.html>
 
 use crate::{
-	BalanceOf, Config, Error, Exposure, NegativeImbalanceOf, NominatorSlashInEra,
-	OffendingValidators, Pallet, Perbill, SessionInterface, SpanSlash, UnappliedSlash,
+	BalanceOf, Config, DisabledValidators, DisablingStrategy, Error, Exposure, NegativeImbalanceOf,
+	NominatorSlashInEra, Pallet, Perbill, SessionInterface, SpanSlash, UnappliedSlash,
 	ValidatorSlashInEra,
 };
 use codec::{Decode, Encode, MaxEncodedLen};
 use frame_support::{
 	ensure,
-	traits::{Currency, Defensive, DefensiveSaturating, Get, Imbalance, OnUnbalanced},
+	traits::{Currency, Defensive, DefensiveSaturating, Imbalance, OnUnbalanced},
 };
 use scale_info::TypeInfo;
 use sp_runtime::{
 	traits::{Saturating, Zero},
 	DispatchResult, RuntimeDebug,
 };
-use sp_staking::{offence::DisableStrategy, EraIndex};
+use sp_staking::EraIndex;
 use sp_std::vec::Vec;
 
 /// The proportion of the slashing reward to be paid out on the first slashing detection.
@@ -220,8 +220,6 @@ 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
@@ -280,18 +278,13 @@ pub(crate) fn compute_slash<T: Config>(
 		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
-			// actions.
-
-			// chill the validator - it misbehaved in the current span and should
-			// not continue in the next election. also end the slashing span.
+			// misbehavior occurred within the current slashing span - end current span.
+			// Check <https://github.com/paritytech/polkadot-sdk/issues/2650> for details.
 			spans.end_span(params.now);
-			<Pallet<T>>::chill_stash(params.stash);
 		}
 	}
 
-	let disable_when_slashed = params.disable_strategy != DisableStrategy::Never;
-	add_offending_validator::<T>(params.stash, disable_when_slashed);
+	add_offending_validator::<T>(&params);
 
 	let mut nominators_slashed = Vec::new();
 	reward_payout += slash_nominators::<T>(params.clone(), prior_slash_p, &mut nominators_slashed);
@@ -320,54 +313,31 @@ fn kick_out_if_recent<T: Config>(params: SlashParams<T>) {
 	);
 
 	if spans.era_span(params.slash_era).map(|s| s.index) == Some(spans.span_index()) {
+		// Check https://github.com/paritytech/polkadot-sdk/issues/2650 for details
 		spans.end_span(params.now);
-		<Pallet<T>>::chill_stash(params.stash);
 	}
 
-	let disable_without_slash = params.disable_strategy == DisableStrategy::Always;
-	add_offending_validator::<T>(params.stash, disable_without_slash);
+	add_offending_validator::<T>(&params);
 }
 
-/// Add the given validator to the offenders list and optionally disable it.
-/// If after adding the validator `OffendingValidatorsThreshold` is reached
-/// a new era will be forced.
-fn add_offending_validator<T: Config>(stash: &T::AccountId, disable: bool) {
-	OffendingValidators::<T>::mutate(|offending| {
-		let validators = T::SessionInterface::validators();
-		let validator_index = match validators.iter().position(|i| i == stash) {
-			Some(index) => index,
-			None => return,
-		};
-
-		let validator_index_u32 = validator_index as u32;
-
-		match offending.binary_search_by_key(&validator_index_u32, |(index, _)| *index) {
-			// this is a new offending validator
-			Err(index) => {
-				offending.insert(index, (validator_index_u32, disable));
-
-				let offending_threshold =
-					T::OffendingValidatorsThreshold::get() * validators.len() as u32;
-
-				if offending.len() >= offending_threshold as usize {
-					// force a new era, to select a new validator set
-					<Pallet<T>>::ensure_new_era()
-				}
-
-				if disable {
-					T::SessionInterface::disable_validator(validator_index_u32);
-				}
-			},
-			Ok(index) => {
-				if disable && !offending[index].1 {
-					// the validator had previously offended without being disabled,
-					// let's make sure we disable it now
-					offending[index].1 = true;
-					T::SessionInterface::disable_validator(validator_index_u32);
-				}
-			},
+/// Inform the [`DisablingStrategy`] implementation about the new offender and disable the list of
+/// validators provided by [`make_disabling_decision`].
+fn add_offending_validator<T: Config>(params: &SlashParams<T>) {
+	DisabledValidators::<T>::mutate(|disabled| {
+		if let Some(offender) =
+			T::DisablingStrategy::decision(params.stash, params.slash_era, &disabled)
+		{
+			// Add the validator to `DisabledValidators` and disable it. Do nothing if it is
+			// already disabled.
+			if let Err(index) = disabled.binary_search_by_key(&offender, |index| *index) {
+				disabled.insert(index, offender);
+				T::SessionInterface::disable_validator(offender);
+			}
 		}
 	});
+
+	// `DisabledValidators` should be kept sorted
+	debug_assert!(DisabledValidators::<T>::get().windows(2).all(|pair| pair[0] < pair[1]));
 }
 
 /// Slash nominators. Accepts general parameters and the prior slash percentage of the validator.