staking: only disable slashed validators and keep them disabled for whole era (#9448)

* session: remove disabled validators threshold logic

* staking: add logic to track offending validators

* staking: disable validators for the whole era

* frame: fix tests

* staking: add tests for disabling validators handling

* staking: fix adding offending validator when already slashed in era

* address review comments

* session, staking: add comments about sorted vecs

Co-authored-by: Andronik Ordian <write@reusable.software>

substrate/frame/staking/src/lib.rs

@@ -619,12 +619,9 @@ pub struct UnappliedSlash<AccountId, Balance: HasCompact> {
 ///
 /// This is needed because `Staking` sets the `ValidatorIdOf` of the `pallet_session::Config`
 pub trait SessionInterface<AccountId>: frame_system::Config {
-	/// Disable a given validator by stash ID.
-	///
-	/// Returns `true` if new era should be forced at the end of this session.
-	/// This allows preventing a situation where there is too many validators
-	/// disabled and block production stalls.
-	fn disable_validator(validator: &AccountId) -> Result<bool, ()>;
+	/// Disable the validator at the given index, returns `false` if the validator was already
+	/// disabled or the index is out of bounds.
+	fn disable_validator(validator_index: u32) -> bool;
 	/// Get the validators from session.
 	fn validators() -> Vec<AccountId>;
 	/// Prune historical session tries up to but not including the given index.
@@ -645,8 +642,8 @@ where
 		Option<<T as frame_system::Config>::AccountId>,
 	>,
 {
-	fn disable_validator(validator: &<T as frame_system::Config>::AccountId) -> Result<bool, ()> {
-		<pallet_session::Pallet<T>>::disable(validator)
+	fn disable_validator(validator_index: u32) -> bool {
+		<pallet_session::Pallet<T>>::disable_index(validator_index)
 	}
 
 	fn validators() -> Vec<<T as frame_system::Config>::AccountId> {

substrate/frame/staking/src/mock.rs

@@ -34,7 +34,7 @@ use sp_runtime::{
 	traits::{IdentityLookup, Zero},
 };
 use sp_staking::offence::{OffenceDetails, OnOffenceHandler};
-use std::{cell::RefCell, collections::HashSet};
+use std::cell::RefCell;
 
 pub const INIT_TIMESTAMP: u64 = 30_000;
 pub const BLOCK_TIME: u64 = 1000;
@@ -45,10 +45,6 @@ pub(crate) type AccountIndex = u64;
 pub(crate) type BlockNumber = u64;
 pub(crate) type Balance = u128;
 
-thread_local! {
-	static SESSION: RefCell<(Vec<AccountId>, HashSet<AccountId>)> = RefCell::new(Default::default());
-}
-
 /// Another session handler struct to test on_disabled.
 pub struct OtherSessionHandler;
 impl OneSessionHandler<AccountId> for OtherSessionHandler {
@@ -61,23 +57,14 @@ impl OneSessionHandler<AccountId> for OtherSessionHandler {
 	{
 	}
 
-	fn on_new_session<'a, I: 'a>(_: bool, validators: I, _: I)
+	fn on_new_session<'a, I: 'a>(_: bool, _: I, _: I)
 	where
 		I: Iterator<Item = (&'a AccountId, Self::Key)>,
 		AccountId: 'a,
 	{
-		SESSION.with(|x| {
-			*x.borrow_mut() = (validators.map(|x| x.0.clone()).collect(), HashSet::new())
-		});
 	}
 
-	fn on_disabled(validator_index: usize) {
-		SESSION.with(|d| {
-			let mut d = d.borrow_mut();
-			let value = d.0[validator_index];
-			d.1.insert(value);
-		})
-	}
+	fn on_disabled(_validator_index: u32) {}
 }
 
 impl sp_runtime::BoundToRuntimeAppPublic for OtherSessionHandler {
@@ -86,7 +73,12 @@ impl sp_runtime::BoundToRuntimeAppPublic for OtherSessionHandler {
 
 pub fn is_disabled(controller: AccountId) -> bool {
 	let stash = Staking::ledger(&controller).unwrap().stash;
-	SESSION.with(|d| d.borrow().1.contains(&stash))
+	let validator_index = match Session::validators().iter().position(|v| *v == stash) {
+		Some(index) => index as u32,
+		None => return false,
+	};
+
+	Session::disabled_validators().contains(&validator_index)
 }
 
 type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic<Test>;
@@ -171,7 +163,6 @@ impl pallet_balances::Config for Test {
 }
 parameter_types! {
 	pub const UncleGenerations: u64 = 0;
-	pub const DisabledValidatorsThreshold: Perbill = Perbill::from_percent(25);
 }
 sp_runtime::impl_opaque_keys! {
 	pub struct SessionKeys {
@@ -186,7 +177,6 @@ impl pallet_session::Config for Test {
 	type Event = Event;
 	type ValidatorId = AccountId;
 	type ValidatorIdOf = crate::StashOf<Test>;
-	type DisabledValidatorsThreshold = DisabledValidatorsThreshold;
 	type NextSessionRotation = pallet_session::PeriodicSessions<Period, Offset>;
 	type WeightInfo = ();
 }
@@ -224,6 +214,7 @@ parameter_types! {
 	pub const BondingDuration: EraIndex = 3;
 	pub const RewardCurve: &'static PiecewiseLinear<'static> = &I_NPOS;
 	pub const MaxNominatorRewardedPerValidator: u32 = 64;
+	pub const OffendingValidatorsThreshold: Perbill = Perbill::from_percent(75);
 }
 
 thread_local! {
@@ -277,6 +268,7 @@ impl crate::pallet::pallet::Config for Test {
 	type EraPayout = ConvertCurve<RewardCurve>;
 	type NextNewSession = Session;
 	type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator;
+	type OffendingValidatorsThreshold = OffendingValidatorsThreshold;
 	type ElectionProvider = onchain::OnChainSequentialPhragmen<Self>;
 	type GenesisElectionProvider = Self::ElectionProvider;
 	type WeightInfo = ();
@@ -510,10 +502,6 @@ impl ExtBuilder {
 		.assimilate_storage(&mut storage);
 
 		let mut ext = sp_io::TestExternalities::from(storage);
-		ext.execute_with(|| {
-			let validators = Session::validators();
-			SESSION.with(|x| *x.borrow_mut() = (validators.clone(), HashSet::new()));
-		});
 
 		if self.initialize_first_session {
 			// We consider all test to start after timestamp is initialized This must be ensured by

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

@@ -302,6 +302,13 @@ impl<T: Config> Pallet<T> {
 				Self::start_era(start_session);
 			}
 		}
+
+		// 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);
+			}
+		}
 	}
 
 	/// End a session potentially ending an era.
@@ -374,6 +381,9 @@ impl<T: Config> Pallet<T> {
 			// Set ending era reward.
 			<ErasValidatorReward<T>>::insert(&active_era.index, validator_payout);
 			T::RewardRemainder::on_unbalanced(T::Currency::issue(rest));
+
+			// Clear offending validators.
+			<OffendingValidators<T>>::kill();
 		}
 	}
 

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

@@ -141,6 +141,10 @@ pub mod pallet {
 		#[pallet::constant]
 		type MaxNominatorRewardedPerValidator: 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 can provide a sorted list of voters in a somewhat sorted way. The
 		/// original use case for this was designed with [`pallet_bags_list::Pallet`] in mind. If
 		/// the bags-list is not desired, [`impls::UseNominatorsMap`] is likely the desired option.
@@ -437,6 +441,19 @@ 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.
+	///
+	/// 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.
+	#[pallet::storage]
+	#[pallet::getter(fn offending_validators)]
+	pub type OffendingValidators<T: Config> = StorageValue<_, Vec<(u32, bool)>, ValueQuery>;
+
 	/// True if network has been upgraded to this version.
 	/// Storage version of the pallet.
 	///

substrate/frame/staking/src/slashing.rs

@@ -56,7 +56,7 @@ use crate::{
 use codec::{Decode, Encode};
 use frame_support::{
 	ensure,
-	traits::{Currency, Imbalance, OnUnbalanced},
+	traits::{Currency, Get, Imbalance, OnUnbalanced},
 };
 use scale_info::TypeInfo;
 use sp_runtime::{
@@ -278,15 +278,13 @@ pub(crate) fn compute_slash<T: Config>(
 			// not continue in the next election. also end the slashing span.
 			spans.end_span(now);
 			<Pallet<T>>::chill_stash(stash);
-
-			// make sure to disable validator till the end of this session
-			if T::SessionInterface::disable_validator(stash).unwrap_or(false) {
-				// force a new era, to select a new validator set
-				<Pallet<T>>::ensure_new_era()
-			}
 		}
 	}
 
+	// 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 mut nominators_slashed = Vec::new();
 	reward_payout += slash_nominators::<T>(params, prior_slash_p, &mut nominators_slashed);
 
@@ -316,13 +314,53 @@ 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()) {
 		spans.end_span(params.now);
 		<Pallet<T>>::chill_stash(params.stash);
-
-		// make sure to disable validator till the end of this session
-		if T::SessionInterface::disable_validator(params.stash).unwrap_or(false) {
-			// force a new era, to select a new validator set
-			<Pallet<T>>::ensure_new_era()
-		}
 	}
+
+	// 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);
+}
+
+/// 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) {
+	<Pallet<T> as Store>::OffendingValidators::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);
+				}
+			},
+		}
+	});
 }
 
 /// Slash nominators. Accepts general parameters and the prior slash percentage of the validator.

substrate/frame/staking/src/tests.rs

@@ -2318,10 +2318,11 @@ fn slash_in_old_span_does_not_deselect() {
 			1,
 		);
 
-		// not forcing for zero-slash and previous span.
-		assert_eq!(Staking::force_era(), Forcing::NotForcing);
-		assert!(<Validators<Test>>::contains_key(11));
-		assert!(Session::validators().contains(&11));
+		// the validator doesn't get chilled again
+		assert!(<Staking as Store>::Validators::iter().find(|(stash, _)| *stash == 11).is_some());
+
+		// but we are still forcing a new era
+		assert_eq!(Staking::force_era(), Forcing::ForceNew);
 
 		on_offence_in_era(
 			&[OffenceDetails {
@@ -2333,10 +2334,13 @@ fn slash_in_old_span_does_not_deselect() {
 			1,
 		);
 
-		// or non-zero.
-		assert_eq!(Staking::force_era(), Forcing::NotForcing);
-		assert!(<Validators<Test>>::contains_key(11));
-		assert!(Session::validators().contains(&11));
+		// the validator doesn't get chilled again
+		assert!(<Staking as Store>::Validators::iter().find(|(stash, _)| *stash == 11).is_some());
+
+		// but it's disabled
+		assert!(is_disabled(10));
+		// and we are still forcing a new era
+		assert_eq!(Staking::force_era(), Forcing::ForceNew);
 	});
 }
 
@@ -2967,6 +2971,132 @@ fn slash_kicks_validators_not_nominators_and_disables_nominator_for_kicked_valid
 	});
 }
 
+#[test]
+fn non_slashable_offence_doesnt_disable_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);
+
+		// offence with no slash associated
+		on_offence_now(
+			&[OffenceDetails { offender: (11, exposure_11.clone()), reporters: vec![] }],
+			&[Perbill::zero()],
+		);
+
+		// offence that slashes 25% of the bond
+		on_offence_now(
+			&[OffenceDetails { offender: (21, exposure_21.clone()), reporters: vec![] }],
+			&[Perbill::from_percent(25)],
+		);
+
+		// the offence for validator 10 wasn't slashable so it wasn't disabled
+		assert!(!is_disabled(10));
+		// whereas validator 20 gets disabled
+		assert!(is_disabled(20));
+	});
+}
+
+#[test]
+fn offence_threshold_triggers_new_era() {
+	ExtBuilder::default()
+		.validator_count(4)
+		.set_status(41, StakerStatus::Validator)
+		.build_and_execute(|| {
+			mock::start_active_era(1);
+			assert_eq_uvec!(Session::validators(), vec![11, 21, 31, 41]);
+
+			assert_eq!(
+				<Test as Config>::OffendingValidatorsThreshold::get(),
+				Perbill::from_percent(75),
+			);
+
+			// we have 4 validators and an offending validator threshold of 75%,
+			// once the third validator commits an offence a new era should be forced
+
+			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 exposure_31 = Staking::eras_stakers(Staking::active_era().unwrap().index, &31);
+
+			on_offence_now(
+				&[OffenceDetails { offender: (11, exposure_11.clone()), reporters: vec![] }],
+				&[Perbill::zero()],
+			);
+
+			assert_eq!(ForceEra::<Test>::get(), Forcing::NotForcing);
+
+			on_offence_now(
+				&[OffenceDetails { offender: (21, exposure_21.clone()), reporters: vec![] }],
+				&[Perbill::zero()],
+			);
+
+			assert_eq!(ForceEra::<Test>::get(), Forcing::NotForcing);
+
+			on_offence_now(
+				&[OffenceDetails { offender: (31, exposure_31.clone()), reporters: vec![] }],
+				&[Perbill::zero()],
+			);
+
+			assert_eq!(ForceEra::<Test>::get(), Forcing::ForceNew);
+		});
+}
+
+#[test]
+fn disabled_validators_are_kept_disabled_for_whole_era() {
+	ExtBuilder::default()
+		.validator_count(4)
+		.set_status(41, StakerStatus::Validator)
+		.build_and_execute(|| {
+			mock::start_active_era(1);
+			assert_eq_uvec!(Session::validators(), vec![11, 21, 31, 41]);
+			assert_eq!(<Test as Config>::SessionsPerEra::get(), 3);
+
+			let exposure_11 = Staking::eras_stakers(Staking::active_era().unwrap().index, &11);
+			let exposure_21 = Staking::eras_stakers(Staking::active_era().unwrap().index, &21);
+
+			on_offence_now(
+				&[OffenceDetails { offender: (11, exposure_11.clone()), reporters: vec![] }],
+				&[Perbill::zero()],
+			);
+
+			on_offence_now(
+				&[OffenceDetails { offender: (21, exposure_21.clone()), reporters: vec![] }],
+				&[Perbill::from_percent(25)],
+			);
+
+			// validator 10 should not be disabled since the offence wasn't slashable
+			assert!(!is_disabled(10));
+			// validator 20 gets disabled since it got slashed
+			assert!(is_disabled(20));
+
+			advance_session();
+
+			// disabled validators should carry-on through all sessions in the era
+			assert!(!is_disabled(10));
+			assert!(is_disabled(20));
+
+			// validator 10 should now get disabled
+			on_offence_now(
+				&[OffenceDetails { offender: (11, exposure_11.clone()), reporters: vec![] }],
+				&[Perbill::from_percent(25)],
+			);
+
+			advance_session();
+
+			// and both are disabled in the last session of the era
+			assert!(is_disabled(10));
+			assert!(is_disabled(20));
+
+			mock::start_active_era(2);
+
+			// when a new era starts disabled validators get cleared
+			assert!(!is_disabled(10));
+			assert!(!is_disabled(20));
+		});
+}
+
 #[test]
 fn claim_reward_at_the_last_era_and_no_double_claim_and_invalid_claim() {
 	// should check that: