Validator Re-Enabling

prdoc/pr_5724.prdoc

@@ -0,0 +1,36 @@
+# 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
+      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 dato-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/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/src/lib.rs

@@ -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,13 +1280,29 @@ 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. Potentially returns 2 validators indices.
+	/// First index is to be disabled, second is to be re-enabled.
+	/// 
+	/// Options:
+	/// (None, None) no changes needed
+	/// (Some(x), None) just disable x
+	/// (Some(x), Some(y)) disable x instead of y
+	/// (None, Some(y)) unreachable
 	fn decision(
 		offender_stash: &T::AccountId,
+		offender_slash_severity: Perbill,
 		slash_era: EraIndex,
-		currently_disabled: &Vec<u32>,
-	) -> Option<u32>;
+		currently_disabled: &Vec<(u32, Perbill)>,
+	) -> DisablingDecision;
+}
+
+/// Helper struct representing a decision coming from a given [`DisablingStrategy`] implementing `decision`
+/// 
+/// Currently supports at most 1 disable + 1 re-enable per decision
+#[derive(Debug)]
+pub struct DisablingDecision {
+	pub disable: Option<u32>,
+	pub reenable: Option<u32>,
 }
 
 /// Implementation of [`DisablingStrategy`] which disables validators from the active set up to a
@@ -1305,9 +1331,10 @@ impl<T: Config, const DISABLING_LIMIT_FACTOR: usize> DisablingStrategy<T>
 {
 	fn decision(
 		offender_stash: &T::AccountId,
+		_offender_slash_severity: Perbill,
 		slash_era: EraIndex,
-		currently_disabled: &Vec<u32>,
-	) -> Option<u32> {
+		currently_disabled: &Vec<(u32, Perbill)>,
+	) -> DisablingDecision {
 		let active_set = T::SessionInterface::validators();
 
 		// We don't disable more than the limit
@@ -1317,7 +1344,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 +1355,97 @@ 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 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.
+/// 
+/// 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 {
+		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 UpToLimitWithReEnablingDisablingStrategy<DISABLING_LIMIT_FACTOR>
+{
+	fn decision(
+		offender_stash: &T::AccountId,
+		offender_slash_severity: Perbill,
+		slash_era: EraIndex,
+		currently_disabled: &Vec<(u32, Perbill)>,
+	) -> 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 }
+		};
+
+		// 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(|(_, perbill)| *perbill <= offender_slash_severity)
+				.min_by_key(|(_, perbill)| *perbill) 
+			{
+				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 }
+			}
+		}
+
+		// If we are not at the limit, just disable the new offender and dont re-enable anyone
+		log!(debug, "Will disable {:?}", offender_idx);
+		DisablingDecision { disable: Some(offender_idx), reenable: None }
 	}
 }

substrate/frame/staking/src/migrations.rs

@@ -60,13 +60,45 @@ impl Default for ObsoleteReleases {
 #[storage_alias]
 type StorageVersion<T: Config> = StorageValue<Pallet<T>, ObsoleteReleases, ValueQuery>;
 
+/// Migrating `DisabledValidators` from `Vec<u32>` to `Vec<(u32, PerBill)>` to track offense severity for re-enabling purposes.
+pub mod v16 {
+	use super::*;
+
+	pub struct VersionUncheckedMigrateV15ToV16<T>(core::marker::PhantomData<T>);
+	impl<T: Config> UncheckedOnRuntimeUpgrade for VersionUncheckedMigrateV15ToV16<T> {
+		fn on_runtime_upgrade() -> Weight {
+			// Migrating `DisabledValidators` from `Vec<u32>` to `Vec<(u32, PerBill)>`.
+			// Using max severity (PerBill) for the migration which effectively makes it so offenders before the migration will never be re-enabled.
+			let max_perbill = Perbill::from_percent(100);
+			// Inject severity
+			let migrated = v15::DisabledValidators::<T>::take().into_iter().map(|v| (v, max_perbill)).collect::<Vec<_>>();
+
+			DisabledValidators::<T>::set(migrated);
+
+			log!(info, "v16 applied successfully.");
+			T::DbWeight::get().reads_writes(1, 1)
+		}
+	}
+
+	pub type MigrateV14ToV15<T> = VersionedMigration<
+		15,
+		16,
+		VersionUncheckedMigrateV15ToV16<T>,
+		Pallet<T>,
+		<T as frame_system::Config>::DbWeight,
+	>;
+}
+
 /// Migrating `OffendingValidators` from `Vec<(u32, bool)>` to `Vec<u32>`
 pub mod v15 {
 	use super::*;
 
 	// The disabling strategy used by staking pallet
 	type DefaultDisablingStrategy = UpToLimitDisablingStrategy;
 
+	#[storage_alias]
+	pub(crate) type DisabledValidators<T: Config> = StorageValue<Pallet<T>, Vec<u32>, ValueQuery>;
+
 	pub struct VersionUncheckedMigrateV14ToV15<T>(core::marker::PhantomData<T>);
 	impl<T: Config> UncheckedOnRuntimeUpgrade for VersionUncheckedMigrateV14ToV15<T> {
 		fn on_runtime_upgrade() -> Weight {

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

@@ -508,7 +508,7 @@ impl<T: Config> Pallet<T> {
 		}
 
 		// disable all offending validators that have been disabled for the whole era
-		for index in <DisabledValidators<T>>::get() {
+		for (index, _) in <DisabledValidators<T>>::get() {
 			T::SessionInterface::disable_validator(index);
 		}
 	}
@@ -2297,9 +2297,10 @@ impl<T: Config> Pallet<T> {
 		Ok(())
 	}
 
+	// Sorted by index 
 	fn ensure_disabled_validators_sorted() -> Result<(), TryRuntimeError> {
 		ensure!(
-			DisabledValidators::<T>::get().windows(2).all(|pair| pair[0] <= pair[1]),
+			DisabledValidators::<T>::get().windows(2).all(|pair| pair[0].0 <= pair[1].0),
 			"DisabledValidators is not sorted"
 		);
 		Ok(())

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

@@ -727,7 +727,7 @@ pub mod pallet {
 	/// offended using binary search.
 	#[pallet::storage]
 	#[pallet::unbounded]
-	pub type DisabledValidators<T: Config> = StorageValue<_, Vec<u32>, ValueQuery>;
+	pub type DisabledValidators<T: Config> = StorageValue<_, Vec<(u32, Perbill)>, 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