Validator Re-Enabling

polkadot/runtime/test-runtime/src/lib.rs

@@ -369,7 +369,7 @@ impl pallet_staking::Config for Runtime {
 	type BenchmarkingConfig = polkadot_runtime_common::StakingBenchmarkingConfig;
 	type EventListeners = ();
 	type WeightInfo = ();
-	type DisablingStrategy = pallet_staking::UpToLimitDisablingStrategy;
+	type DisablingStrategy = pallet_staking::UpToLimitWithReEnablingDisablingStrategy;
 }
 
 parameter_types! {

polkadot/runtime/westend/src/lib.rs

@@ -162,7 +162,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	spec_name: create_runtime_str!("westend"),
 	impl_name: create_runtime_str!("parity-westend"),
 	authoring_version: 2,
-	spec_version: 1_015_000,
+	spec_version: 1_016_002,
 	impl_version: 0,
 	apis: RUNTIME_API_VERSIONS,
 	transaction_version: 26,
@@ -747,7 +747,7 @@ impl pallet_staking::Config for Runtime {
 	type BenchmarkingConfig = polkadot_runtime_common::StakingBenchmarkingConfig;
 	type EventListeners = (NominationPools, DelegatedStaking);
 	type WeightInfo = weights::pallet_staking::WeightInfo<Runtime>;
-	type DisablingStrategy = pallet_staking::UpToLimitDisablingStrategy;
+	type DisablingStrategy = pallet_staking::UpToLimitWithReEnablingDisablingStrategy;
 }
 
 impl pallet_fast_unstake::Config for Runtime {
@@ -1799,6 +1799,7 @@ pub mod migrations {
 			MaxPoolsToMigrate,
 		>,
 		pallet_staking::migrations::v15::MigrateV14ToV15<Runtime>,
+		pallet_staking::migrations::v16::MigrateV15ToV16<Runtime>,
 	);
 }
 

prdoc/pr_5724.prdoc

@@ -0,0 +1,37 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: Validator Re-Enabling (master PR)
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      Implementation of the Stage 3 for the New Disabling Strategy: https://github.com/paritytech/polkadot-sdk/issues/4359
+
+      This PR changes when an active validator node gets disabled for comitting offences.
+      When Byzantine Threshold Validators (1/3) are already disabled instead of no longer
+      disabling the highest offenders will be disabled potentially re-enabling low offenders.
+
+  - audience: Node Operator
+    description: |
+      Implementation of the Stage 3 for the New Disabling Strategy: https://github.com/paritytech/polkadot-sdk/issues/4359
+
+      This PR changes when an active validator node gets disabled within parachain consensus (reduced responsibilities and
+      reduced rewards) for comitting offences. This should not affect active validators on a day-to-day basis and will only
+      be relevant when the network is under attack or there is a wide spread malfunction causing slashes. In that case
+      lowest offenders might get eventually re-enabled (back to normal responsibilities and normal rewards).
+
+migrations:
+  db: []
+  runtime:
+    - reference: pallet-staking
+      description: |
+        Migrating `DisabledValidators` from `Vec<u32>` to `Vec<(u32, PerBill)>` where the PerBill represents the severity
+        of the offence in terms of the % slash.
+
+crates:
+  - name: pallet-staking
+    bump: minor
+
+  - name: pallet-session
+    bump: minor

substrate/bin/node/runtime/src/lib.rs

@@ -698,7 +698,7 @@ impl pallet_staking::Config for Runtime {
 	type EventListeners = NominationPools;
 	type WeightInfo = pallet_staking::weights::SubstrateWeight<Runtime>;
 	type BenchmarkingConfig = StakingBenchmarkingConfig;
-	type DisablingStrategy = pallet_staking::UpToLimitDisablingStrategy;
+	type DisablingStrategy = pallet_staking::UpToLimitWithReEnablingDisablingStrategy;
 }
 
 impl pallet_fast_unstake::Config for Runtime {

substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs

@@ -156,11 +156,9 @@ fn mass_slash_doesnt_enter_emergency_phase() {
 		assert_eq!(active_set_size_before_slash, active_set_size_after_slash);
 
 		// Slashed validators are disabled up to a limit
-		slashed.truncate(
-			pallet_staking::UpToLimitDisablingStrategy::<SLASHING_DISABLING_FACTOR>::disable_limit(
-				active_set_size_after_slash,
-			),
-		);
+		slashed.truncate(pallet_staking::UpToLimitWithReEnablingDisablingStrategy::<
+			SLASHING_DISABLING_FACTOR,
+		>::disable_limit(active_set_size_after_slash));
 
 		// Find the indices of the disabled validators
 		let active_set = Session::validators();

substrate/frame/election-provider-multi-phase/test-staking-e2e/src/mock.rs

@@ -304,7 +304,8 @@ impl pallet_staking::Config for Runtime {
 	type MaxUnlockingChunks = MaxUnlockingChunks;
 	type EventListeners = Pools;
 	type WeightInfo = pallet_staking::weights::SubstrateWeight<Runtime>;
-	type DisablingStrategy = pallet_staking::UpToLimitDisablingStrategy<SLASHING_DISABLING_FACTOR>;
+	type DisablingStrategy =
+		pallet_staking::UpToLimitWithReEnablingDisablingStrategy<SLASHING_DISABLING_FACTOR>;
 	type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig;
 }
 

substrate/frame/session/src/lib.rs

@@ -735,6 +735,23 @@ impl<T: Config> Pallet<T> {
 		})
 	}
 
+	/// Re-enable the validator of index `i`, returns `false` if the validator was already enabled.
+	pub fn enable_index(i: u32) -> bool {
+		if i >= Validators::<T>::decode_len().unwrap_or(0) as u32 {
+			return false
+		}
+
+		// If the validator is not disabled, return false.
+		DisabledValidators::<T>::mutate(|disabled| {
+			if let Ok(index) = disabled.binary_search(&i) {
+				disabled.remove(index);
+				true
+			} else {
+				false
+			}
+		})
+	}
+
 	/// Disable the validator identified by `c`. (If using with the staking pallet,
 	/// this would be their *stash* account.)
 	///

substrate/frame/staking/CHANGELOG.md

@@ -7,6 +7,14 @@ 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.
 
+## [v16]
+
+
+### Added
+
+- New default implementation of `DisablingStrategy` - `UpToLimitWithReEnablingDisablingStrategy`. Same as `UpToLimitDisablingStrategy` except when a limit (1/3 default) is reached. When limit is reached the offender is only disabled if his offence is greater or equal than some other already disabled offender. The smallest possible offender is re-enabled to make space for the new greater offender. A limit should thus always be respected.
+- `DisabledValidators` changed format to include severity of the offence.
+
 ## [v15]
 
 ### Added

substrate/frame/staking/src/lib.rs

@@ -323,7 +323,7 @@ use sp_runtime::{
 	Perbill, Perquintill, Rounding, RuntimeDebug, Saturating,
 };
 use sp_staking::{
-	offence::{Offence, OffenceError, ReportOffence},
+	offence::{Offence, OffenceError, OffenceSeverity, ReportOffence},
 	EraIndex, ExposurePage, OnStakingUpdate, Page, PagedExposureMetadata, SessionIndex,
 	StakingAccount,
 };
@@ -848,6 +848,9 @@ pub trait SessionInterface<AccountId> {
 	/// 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;
+	/// Re-enable a validator that was previously disabled. Returns `false` if the validator was
+	/// already enabled or the index is out of bounds.
+	fn enable_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.
@@ -872,6 +875,10 @@ where
 		<pallet_session::Pallet<T>>::disable_index(validator_index)
 	}
 
+	fn enable_validator(validator_index: u32) -> bool {
+		<pallet_session::Pallet<T>>::enable_index(validator_index)
+	}
+
 	fn validators() -> Vec<<T as frame_system::Config>::AccountId> {
 		<pallet_session::Pallet<T>>::validators()
 	}
@@ -885,6 +892,9 @@ impl<AccountId> SessionInterface<AccountId> for () {
 	fn disable_validator(_: u32) -> bool {
 		true
 	}
+	fn enable_validator(_: u32) -> bool {
+		true
+	}
 	fn validators() -> Vec<AccountId> {
 		Vec::new()
 	}
@@ -1270,33 +1280,54 @@ impl BenchmarkingConfig for TestBenchmarkingConfig {
 
 /// 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.
+	/// Make a disabling decision. Returning a [`DisablingDecision`]
 	fn decision(
 		offender_stash: &T::AccountId,
+		offender_slash_severity: OffenceSeverity,
 		slash_era: EraIndex,
-		currently_disabled: &Vec<u32>,
-	) -> Option<u32>;
+		currently_disabled: &Vec<(u32, OffenceSeverity)>,
+	) -> DisablingDecision;
 }
 
-/// Implementation of [`DisablingStrategy`] which disables validators from the active set up to a
+/// Helper struct representing a decision coming from a given [`DisablingStrategy`] implementing
+/// `decision`
+///
+/// `disable` is the index of the validator to disable,
+/// `reenable` is the index of the validator to re-enable.
+#[derive(Debug)]
+pub struct DisablingDecision {
+	pub disable: Option<u32>,
+	pub reenable: Option<u32>,
+}
+
+/// Calculate the disabling limit based on the number of validators and the disabling limit factor.
+/// 
+/// This is a sensible default implementation for the disabling limit factor for most disabling strategies.
+/// 
+/// Disabling limit factor n=2 -> 1/n = 1/2 = 50% of validators can be disabled
+fn factor_based_disable_limit(validators_len: usize, disabling_limit_factor: usize) -> usize {
+    validators_len
+        .saturating_sub(1)
+        .checked_div(disabling_limit_factor)
+        .unwrap_or_else(|| {
+            defensive!("DISABLING_LIMIT_FACTOR should not be 0");
+            0
+        })
+}
+
+/// Implementation of [`DisablingStrategy`] using factor_based_disable_limit 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
-			})
+		factor_based_disable_limit(validators_len, DISABLING_LIMIT_FACTOR)
 	}
 }
 
@@ -1305,9 +1336,10 @@ impl<T: Config, const DISABLING_LIMIT_FACTOR: usize> DisablingStrategy<T>
 {
 	fn decision(
 		offender_stash: &T::AccountId,
+		_offender_slash_severity: OffenceSeverity,
 		slash_era: EraIndex,
-		currently_disabled: &Vec<u32>,
-	) -> Option<u32> {
+		currently_disabled: &Vec<(u32, OffenceSeverity)>,
+	) -> DisablingDecision {
 		let active_set = T::SessionInterface::validators();
 
 		// We don't disable more than the limit
@@ -1317,7 +1349,7 @@ impl<T: Config, const DISABLING_LIMIT_FACTOR: usize> DisablingStrategy<T>
 				"Won't disable: reached disabling limit {:?}",
 				Self::disable_limit(active_set.len())
 			);
-			return None
+			return DisablingDecision { disable: None, reenable: None }
 		}
 
 		// We don't disable for offences in previous eras
@@ -1328,18 +1360,115 @@ impl<T: Config, const DISABLING_LIMIT_FACTOR: usize> DisablingStrategy<T>
 				Pallet::<T>::current_era().unwrap_or_default(),
 				slash_era
 			);
-			return None
+			return DisablingDecision { disable: None, reenable: 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
+			return DisablingDecision { disable: None, reenable: None }
 		};
 
 		log!(debug, "Will disable {:?}", offender_idx);
 
-		Some(offender_idx)
+		DisablingDecision { disable: Some(offender_idx), reenable: None }
+	}
+}
+
+/// Implementation of [`DisablingStrategy`] which disables validators from the active set up to a
+/// limit (factor_based_disable_limit) and if the limit is reached and the new offender is higher (bigger punishment/severity)
+/// then it re-enables to lowest offender to free up space for the new offender.
+/// 
+/// This strategy is not based on cumulative severity of offences but only on the severity of the highest offence.
+/// Offender first committing a 25% offence and then a 50% offence will be treated the same as an offender committing 50%
+/// offence.
+///
+/// An extension of [`UpToLimitDisablingStrategy`].
+pub struct UpToLimitWithReEnablingDisablingStrategy<const DISABLING_LIMIT_FACTOR: usize = 3>;
+
+impl<const DISABLING_LIMIT_FACTOR: usize>
+	UpToLimitWithReEnablingDisablingStrategy<DISABLING_LIMIT_FACTOR>
+{
+	/// Disabling limit calculated from the total number of validators in the active set. When
+	/// reached re-enabling logic might kick in.
+	pub fn disable_limit(validators_len: usize) -> usize {
+		factor_based_disable_limit(validators_len, DISABLING_LIMIT_FACTOR)
+	}
+}
+
+impl<T: Config, const DISABLING_LIMIT_FACTOR: usize> DisablingStrategy<T>
+	for UpToLimitWithReEnablingDisablingStrategy<DISABLING_LIMIT_FACTOR>
+{
+	fn decision(
+		offender_stash: &T::AccountId,
+		offender_slash_severity: OffenceSeverity,
+		slash_era: EraIndex,
+		currently_disabled: &Vec<(u32, OffenceSeverity)>,
+	) -> DisablingDecision {
+		let active_set = T::SessionInterface::validators();
+
+		// 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 DisablingDecision { disable: None, reenable: None }
+		}
+
+		// We don't disable validators that are not in the active set
+		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 DisablingDecision { disable: None, reenable: None }
+		};
+
+		// Check if offender is already disabled
+		if let Some((_, old_severity)) =
+			currently_disabled.iter().find(|(idx, _)| *idx == offender_idx)
+		{
+			if offender_slash_severity > *old_severity {
+				log!(debug, "Offender already disabled but with lower severity, will disable again to refresh severity of {:?}", offender_idx);
+				return DisablingDecision { disable: Some(offender_idx), reenable: None };
+			} else {
+				log!(debug, "Offender already disabled with higher or equal severity");
+				return DisablingDecision { disable: None, reenable: None };
+			}
+		}
+
+		// We don't disable more than the limit (but we can re-enable a smaller offender to make
+		// space)
+		if currently_disabled.len() >= Self::disable_limit(active_set.len()) {
+			log!(
+				debug,
+				"Reached disabling limit {:?}, checking for re-enabling",
+				Self::disable_limit(active_set.len())
+			);
+
+			// Find the smallest offender to re-enable that is not higher than
+			// offender_slash_severity
+			if let Some((smallest_idx, _)) = currently_disabled
+				.iter()
+				.filter(|(_, severity)| *severity <= offender_slash_severity)
+				.min_by_key(|(_, severity)| *severity)
+			{
+				log!(debug, "Will disable {:?} and re-enable {:?}", offender_idx, smallest_idx);
+				return DisablingDecision {
+					disable: Some(offender_idx),
+					reenable: Some(*smallest_idx),
+				}
+			} else {
+				log!(debug, "No smaller offender found to re-enable");
+				return DisablingDecision { disable: None, reenable: None }
+			}
+		} else {
+			// If we are not at the limit, just disable the new offender and dont re-enable anyone
+			log!(debug, "Will disable {:?}", offender_idx);
+			return DisablingDecision { disable: Some(offender_idx), reenable: None }
+		}
 	}
 }