Selectable on-runtime-upgrade checks (#13045)

* Make try-runtime checks selectable

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Update frame/support/src/traits/try_runtime.rs

Co-authored-by: Bastian Köcher <git@kchr.de>

* Add Clap wrapper for enum UpgradeCheckSelect

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Revert "Add Clap wrapper for enum UpgradeCheckSelect"

This reverts commit e29538c1a79d1711b43addc9400d871f6aa32844.

* fix pools sanity check

* Set default for --checks to None

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Make --checks backwards comp

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add clap attr comment

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: kianenigma <kian@parity.io>

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

@@ -539,7 +539,7 @@ impl_runtime_apis! {
 
 	#[cfg(feature = "try-runtime")]
 	impl frame_try_runtime::TryRuntime<Block> for Runtime {
-		fn on_runtime_upgrade(checks: bool) -> (Weight, Weight) {
+		fn on_runtime_upgrade(checks: frame_try_runtime::UpgradeCheckSelect) -> (Weight, Weight) {
 			// NOTE: intentional unwrap: we don't want to propagate the error backwards, and want to
 			// have a backtrace here. If any of the pre/post migration checks fail, we shall stop
 			// right here and right now.

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

@@ -2196,7 +2196,7 @@ impl_runtime_apis! {
 
 	#[cfg(feature = "try-runtime")]
 	impl frame_try_runtime::TryRuntime<Block> for Runtime {
-		fn on_runtime_upgrade(checks: bool) -> (Weight, Weight) {
+		fn on_runtime_upgrade(checks: frame_try_runtime::UpgradeCheckSelect) -> (Weight, Weight) {
 			// NOTE: intentional unwrap: we don't want to propagate the error backwards, and want to
 			// have a backtrace here. If any of the pre/post migration checks fail, we shall stop
 			// right here and right now.

substrate/frame/executive/src/lib.rs

@@ -333,8 +333,10 @@ where
 	///
 	/// Runs the try-state code both before and after the migration function if `checks` is set to
 	/// `true`. Also, if set to `true`, it runs the `pre_upgrade` and `post_upgrade` hooks.
-	pub fn try_runtime_upgrade(checks: bool) -> Result<Weight, &'static str> {
-		if checks {
+	pub fn try_runtime_upgrade(
+		checks: frame_try_runtime::UpgradeCheckSelect,
+	) -> Result<Weight, &'static str> {
+		if checks.try_state() {
 			let _guard = frame_support::StorageNoopGuard::default();
 			<AllPalletsWithSystem as frame_support::traits::TryState<System::BlockNumber>>::try_state(
 				frame_system::Pallet::<System>::block_number(),
@@ -344,10 +346,10 @@ where
 
 		let weight =
 			<(COnRuntimeUpgrade, AllPalletsWithSystem) as OnRuntimeUpgrade>::try_on_runtime_upgrade(
-				checks,
+				checks.pre_and_post(),
 			)?;
 
-		if checks {
+		if checks.try_state() {
 			let _guard = frame_support::StorageNoopGuard::default();
 			<AllPalletsWithSystem as frame_support::traits::TryState<System::BlockNumber>>::try_state(
 				frame_system::Pallet::<System>::block_number(),

substrate/frame/nomination-pools/src/lib.rs

@@ -2470,12 +2470,17 @@ impl<T: Config> Pallet<T> {
 
 		for id in reward_pools {
 			let account = Self::create_reward_account(id);
-			assert!(
-				T::Currency::free_balance(&account) >= T::Currency::minimum_balance(),
-				"reward pool of {id}: {:?} (ed = {:?})",
-				T::Currency::free_balance(&account),
-				T::Currency::minimum_balance()
-			);
+			if T::Currency::free_balance(&account) < T::Currency::minimum_balance() {
+				log!(
+					warn,
+					"reward pool of {:?}: {:?} (ed = {:?}), should only happen because ED has \
+					changed recently. Pool operators should be notified to top up the reward \
+					account",
+					id,
+					T::Currency::free_balance(&account),
+					T::Currency::minimum_balance(),
+				)
+			}
 		}
 
 		let mut pools_members = BTreeMap::<PoolId, u32>::new();

substrate/frame/support/src/traits.rs

@@ -121,4 +121,4 @@ pub use messages::{
 #[cfg(feature = "try-runtime")]
 mod try_runtime;
 #[cfg(feature = "try-runtime")]
-pub use try_runtime::{Select as TryStateSelect, TryState};
+pub use try_runtime::{Select as TryStateSelect, TryState, UpgradeCheckSelect};

substrate/frame/support/src/traits/try_runtime.rs

@@ -21,7 +21,7 @@ use impl_trait_for_tuples::impl_for_tuples;
 use sp_arithmetic::traits::AtLeast32BitUnsigned;
 use sp_std::prelude::*;
 
-// Which state tests to execute.
+/// Which state tests to execute.
 #[derive(codec::Encode, codec::Decode, Clone)]
 pub enum Select {
 	/// None of them.
@@ -81,6 +81,46 @@ impl sp_std::str::FromStr for Select {
 	}
 }
 
+/// Select which checks should be run when trying a runtime upgrade upgrade.
+#[derive(codec::Encode, codec::Decode, Clone, Debug, Copy)]
+pub enum UpgradeCheckSelect {
+	/// Run no checks.
+	None,
+	/// Run the `try_state`, `pre_upgrade` and `post_upgrade` checks.
+	All,
+	/// Run the `pre_upgrade` and `post_upgrade` checks.
+	PreAndPost,
+	/// Run the `try_state` checks.
+	TryState,
+}
+
+impl UpgradeCheckSelect {
+	/// Whether the pre- and post-upgrade checks are selected.
+	pub fn pre_and_post(&self) -> bool {
+		matches!(self, Self::All | Self::PreAndPost)
+	}
+
+	/// Whether the try-state checks are selected.
+	pub fn try_state(&self) -> bool {
+		matches!(self, Self::All | Self::TryState)
+	}
+}
+
+#[cfg(feature = "std")]
+impl core::str::FromStr for UpgradeCheckSelect {
+	type Err = &'static str;
+
+	fn from_str(s: &str) -> Result<Self, Self::Err> {
+		match s.to_lowercase().as_str() {
+			"none" => Ok(Self::None),
+			"all" => Ok(Self::All),
+			"pre-and-post" => Ok(Self::PreAndPost),
+			"try-state" => Ok(Self::TryState),
+			_ => Err("Invalid CheckSelector"),
+		}
+	}
+}
+
 /// Execute some checks to ensure the internal state of a pallet is consistent.
 ///
 /// Usually, these checks should check all of the invariants that are expected to be held on all of

substrate/frame/try-runtime/src/lib.rs

@@ -20,7 +20,7 @@
 #![cfg_attr(not(feature = "std"), no_std)]
 #![cfg(feature = "try-runtime")]
 
-pub use frame_support::traits::TryStateSelect;
+pub use frame_support::traits::{TryStateSelect, UpgradeCheckSelect};
 use frame_support::weights::Weight;
 
 sp_api::decl_runtime_apis! {
@@ -37,7 +37,7 @@ sp_api::decl_runtime_apis! {
 		/// If `checks` is `true`, `pre_migrate` and `post_migrate` of each migration and
 		/// `try_state` of all pallets will be executed. Else, no. If checks are executed, the PoV
 		/// tracking is likely inaccurate.
-		fn on_runtime_upgrade(checks: bool) -> (Weight, Weight);
+		fn on_runtime_upgrade(checks: UpgradeCheckSelect) -> (Weight, Weight);
 
 		/// Execute the given block, but optionally disable state-root and signature checks.
 		///

substrate/utils/frame/try-runtime/cli/src/commands/on_runtime_upgrade.rs

@@ -16,6 +16,7 @@
 // limitations under the License.
 
 use crate::{build_executor, state_machine_call_with_proof, SharedParams, State, LOG_TARGET};
+use frame_try_runtime::UpgradeCheckSelect;
 use parity_scale_codec::{Decode, Encode};
 use sc_executor::sp_wasm_interface::HostFunctions;
 use sp_runtime::traits::{Block as BlockT, NumberFor};
@@ -29,12 +30,22 @@ pub struct OnRuntimeUpgradeCmd {
 	#[command(subcommand)]
 	pub state: State,
 
-	/// Execute `try_state`, `pre_upgrade` and `post_upgrade` checks as well.
+	/// Select which optional checks to perform. Selects all when no value is given.
 	///
-	/// This will perform more checks, but it will also makes the reported PoV/Weight be
-	/// inaccurate.
-	#[clap(long)]
-	pub checks: bool,
+	/// - `none`: Perform no checks (default when the arg is not present).
+	/// - `all`: Perform all checks (default when the arg is present).
+	/// - `pre-and-post`: Perform pre- and post-upgrade checks.
+	/// - `try-state`: Perform the try-state checks.
+	///
+	/// Performing any checks will potentially invalidate the measured PoV/Weight.
+	// NOTE: The clap attributes make it backwards compatible with the previous `--checks` flag.
+	#[clap(long,
+		default_value = "None",
+		default_missing_value = "All",
+		num_args = 0..=1,
+		require_equals = true,
+		verbatim_doc_comment)]
+	pub checks: UpgradeCheckSelect,
 }
 
 pub(crate) async fn on_runtime_upgrade<Block, HostFns>(