Validator disabling in session enhancements

prdoc/pr_7663.prdoc

@@ -0,0 +1,21 @@
+# 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 disabling in session enhancements
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      This PR introduces changes to the pallet-session interface. Disabled validators can
+      still be disabled with just the index but it will default to highest possible severity.
+      pallet-session also additionally exposes DisabledValidators with their severities.
+      The staking primitive OffenceSeverity received min, max and default implementations
+      for ease of use.
+
+crates:
+  - name: pallet-staking
+    bump: minor
+  - name: pallet-session
+    bump: minor
+  - name: sp-staking
+    bump: minor

substrate/frame/session/src/lib.rs

@@ -138,13 +138,16 @@ use frame_support::{
 use frame_system::pallet_prelude::BlockNumberFor;
 use sp_runtime::{
 	traits::{AtLeast32BitUnsigned, Convert, Member, One, OpaqueKeys, Zero},
-	ConsensusEngineId, DispatchError, KeyTypeId, Perbill, Permill, RuntimeAppPublic,
+	ConsensusEngineId, DispatchError, KeyTypeId, Permill, RuntimeAppPublic,
 };
 use sp_staking::{offence::OffenceSeverity, SessionIndex};
 
 pub use pallet::*;
 pub use weights::WeightInfo;
 
+#[cfg(any(feature = "try-runtime"))]
+use sp_runtime::TryRuntimeError;
+
 pub(crate) const LOG_TARGET: &str = "runtime::session";
 
 // syntactic sugar for logging.
@@ -590,6 +593,11 @@ pub mod pallet {
 				Weight::zero()
 			}
 		}
+
+		#[cfg(feature = "try-runtime")]
+		fn try_state(_n: BlockNumberFor<T>) -> Result<(), TryRuntimeError> {
+			Self::do_try_state()
+		}
 	}
 
 	#[pallet::call]
@@ -675,6 +683,7 @@ impl<T: Config> Pallet<T> {
 		Validators::<T>::put(&validators);
 
 		if changed {
+			log!(trace, "resetting disabled validators");
 			// reset disabled validators if active set was changed
 			DisabledValidators::<T>::take();
 		}
@@ -683,12 +692,12 @@ impl<T: Config> Pallet<T> {
 		let session_index = session_index + 1;
 		CurrentIndex::<T>::put(session_index);
 		T::SessionManager::start_session(session_index);
-		log::trace!(target: "runtime::session", "starting_session {:?}", session_index);
+		log!(trace, "starting_session {:?}", session_index);
 
 		// Get next validator set.
 		let maybe_next_validators = T::SessionManager::new_session(session_index + 1);
-		log::trace!(
-			target: "runtime::session",
+		log!(
+			trace,
 			"planning_session {:?} with {:?} validators",
 			session_index + 1,
 			maybe_next_validators.as_ref().map(|v| v.len())
@@ -745,38 +754,6 @@ impl<T: Config> Pallet<T> {
 		T::SessionHandler::on_new_session::<T::Keys>(changed, &session_keys, &queued_amalgamated);
 	}
 
-	/// Disable the validator of index `i`, returns `false` if the validator was already disabled.
-	///
-	/// Note: This sets the OffenceSeverity to the lowest value.
-	pub fn disable_index(i: u32) -> bool {
-		if i >= Validators::<T>::decode_len().defensive_unwrap_or(0) as u32 {
-			return false
-		}
-
-		DisabledValidators::<T>::mutate(|disabled| {
-			if let Err(index) = disabled.binary_search_by_key(&i, |(index, _)| *index) {
-				disabled.insert(index, (i, OffenceSeverity(Perbill::zero())));
-				T::SessionHandler::on_disabled(i);
-				return true
-			}
-
-			false
-		})
-	}
-
-	/// Disable the validator identified by `c`. (If using with the staking pallet,
-	/// this would be their *stash* account.)
-	///
-	/// Returns `false` either if the validator could not be found or it was already
-	/// disabled.
-	pub fn disable(c: &T::ValidatorId) -> bool {
-		Validators::<T>::get()
-			.iter()
-			.position(|i| i == c)
-			.map(|i| Self::disable_index(i as u32))
-			.unwrap_or(false)
-	}
-
 	/// Upgrade the key type from some old type to a new type. Supports adding
 	/// and removing key types.
 	///
@@ -928,45 +905,118 @@ impl<T: Config> Pallet<T> {
 		KeyOwner::<T>::remove((id, key_data));
 	}
 
-	pub fn report_offence(validator: T::ValidatorId, severity: OffenceSeverity) {
+	/// Disable the validator of index `i` with a specified severity,
+	/// returns `false` if the validator is not found.
+	///
+	/// Note: If validator is already disabled, the severity will
+	/// be updated if the new one is higher.
+	pub fn disable_index_with_severity(i: u32, severity: OffenceSeverity) -> bool {
+		if i >= Validators::<T>::decode_len().defensive_unwrap_or(0) as u32 {
+			return false;
+		}
+
 		DisabledValidators::<T>::mutate(|disabled| {
-			let decision = T::DisablingStrategy::decision(&validator, severity, &disabled);
-
-			if let Some(offender_idx) = decision.disable {
-				// Check if the offender is already disabled
-				match disabled.binary_search_by_key(&offender_idx, |(index, _)| *index) {
-					// Offender is already disabled, update severity if the new one is higher
-					Ok(index) => {
-						let (_, old_severity) = &mut disabled[index];
-						if severity > *old_severity {
-							*old_severity = severity;
-						}
-					},
-					Err(index) => {
-						// Offender is not disabled, add to `DisabledValidators` and disable it
-						disabled.insert(index, (offender_idx, severity));
-						// let the session handlers know that a validator got disabled
-						T::SessionHandler::on_disabled(offender_idx);
-
-						// Emit event that a validator got disabled
-						Self::deposit_event(Event::ValidatorDisabled {
-							validator: validator.clone(),
-						});
-					},
-				}
+			match disabled.binary_search_by_key(&i, |(index, _)| *index) {
+				// Validator is already disabled, update severity if the new one is higher
+				Ok(index) => {
+					let current_severity = &mut disabled[index].1;
+					if severity > *current_severity {
+						log!(
+							trace,
+							"updating disablement severity of validator {:?} from {:?} to {:?}",
+							i,
+							*current_severity,
+							severity
+						);
+						*current_severity = severity;
+					}
+					true
+				},
+				// Validator is not disabled, add to `DisabledValidators` and disable it
+				Err(index) => {
+					log!(trace, "disabling validator {:?}", i);
+					Self::deposit_event(Event::ValidatorDisabled {
+						validator: Validators::<T>::get()[index as usize].clone(),
+					});
+					disabled.insert(index, (i, severity));
+					T::SessionHandler::on_disabled(i);
+					true
+				},
 			}
+		})
+	}
 
-			if let Some(reenable_idx) = decision.reenable {
-				// Remove the validator from `DisabledValidators` and re-enable it.
-				if let Ok(index) = disabled.binary_search_by_key(&reenable_idx, |(index, _)| *index)
-				{
-					disabled.remove(index);
-					// Emit event that a validator got re-enabled
-					let reenabled_stash = Validators::<T>::get()[reenable_idx as usize].clone();
-					Self::deposit_event(Event::ValidatorReenabled { validator: reenabled_stash });
-				}
+	/// Disable the validator of index `i` with a default severity (defaults to most severe),
+	/// returns `false` if the validator is not found.
+	pub fn disable_index(i: u32) -> bool {
+		let default_severity = OffenceSeverity::default();
+		Self::disable_index_with_severity(i, default_severity)
+	}
+
+	/// Disable the validator identified by `c`. (If using with the staking pallet,
+	/// this would be their *stash* account.)
+	///
+	/// Returns `false` either if the validator could not be found or it was already
+	/// disabled.
+	pub fn disable(c: &T::ValidatorId) -> bool {
+		Validators::<T>::get()
+			.iter()
+			.position(|i| i == c)
+			.map(|i| Self::disable_index(i as u32))
+			.unwrap_or(false)
+	}
+
+	/// Re-enable the validator of index `i`, returns `false` if the validator was not disabled.
+	pub fn reenable_index(i: u32) -> bool {
+		if i >= Validators::<T>::decode_len().defensive_unwrap_or(0) as u32 {
+			return false;
+		}
+
+		DisabledValidators::<T>::mutate(|disabled| {
+			if let Ok(index) = disabled.binary_search_by_key(&i, |(index, _)| *index) {
+				log!(trace, "reenabling validator {:?}", i);
+				Self::deposit_event(Event::ValidatorReenabled {
+					validator: Validators::<T>::get()[index as usize].clone(),
+				});
+				disabled.remove(index);
+				return true;
 			}
-		});
+			false
+		})
+	}
+
+	/// Convert a validator ID to an index.
+	/// (If using with the staking pallet, this would be their *stash* account.)
+	pub fn validator_id_to_index(id: &T::ValidatorId) -> Option<u32> {
+		Validators::<T>::get().iter().position(|i| i == id).map(|i| i as u32)
+	}
+
+	/// Report an offence for the given validator and let disabling strategy decide
+	/// what changes to disabled validators should be made.
+	pub fn report_offence(validator: T::ValidatorId, severity: OffenceSeverity) {
+		log!(trace, "reporting offence for {:?} with {:?}", validator, severity);
+		let decision =
+			T::DisablingStrategy::decision(&validator, severity, &DisabledValidators::<T>::get());
+
+		// Disable
+		if let Some(offender_idx) = decision.disable {
+			Self::disable_index_with_severity(offender_idx, severity);
+		}
+
+		// Re-enable
+		if let Some(reenable_idx) = decision.reenable {
+			Self::reenable_index(reenable_idx);
+		}
+	}
+
+	#[cfg(any(test, feature = "try-runtime"))]
+	pub fn do_try_state() -> Result<(), sp_runtime::TryRuntimeError> {
+		// Ensure that the validators are sorted
+		ensure!(
+			DisabledValidators::<T>::get().windows(2).all(|pair| pair[0].0 <= pair[1].0),
+			"DisabledValidators is not sorted"
+		);
+		Ok(())
 	}
 }
 

substrate/frame/session/src/migrations/v1.rs

@@ -15,7 +15,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use crate::{Config, DisabledValidators as NewDisabledValidators, Pallet, Perbill, Vec};
+use crate::{Config, DisabledValidators as NewDisabledValidators, Pallet, Vec};
 use frame_support::{
 	pallet_prelude::{Get, ValueQuery, Weight},
 	traits::UncheckedOnRuntimeUpgrade,
@@ -49,14 +49,14 @@ impl<T: Config> MigrateDisabledValidators for InitOffenceSeverity<T> {
 	fn peek_disabled() -> Vec<(u32, OffenceSeverity)> {
 		DisabledValidators::<T>::get()
 			.iter()
-			.map(|v| (*v, OffenceSeverity(Perbill::zero())))
+			.map(|v| (*v, OffenceSeverity::max_severity()))
 			.collect::<Vec<_>>()
 	}
 
 	fn take_disabled() -> Vec<(u32, OffenceSeverity)> {
 		DisabledValidators::<T>::take()
 			.iter()
-			.map(|v| (*v, OffenceSeverity(Perbill::zero())))
+			.map(|v| (*v, OffenceSeverity::max_severity()))
 			.collect::<Vec<_>>()
 	}
 }