From 0b5e912ebec53987b089896cd8f472b2f5c205de Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 4 Jan 2023 13:44:41 +0100 Subject: [PATCH] Selectable on-runtime-upgrade checks (#13045) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Make try-runtime checks selectable Signed-off-by: Oliver Tale-Yazdi * Update frame/support/src/traits/try_runtime.rs Co-authored-by: Bastian Köcher * Add Clap wrapper for enum UpgradeCheckSelect Signed-off-by: Oliver Tale-Yazdi * 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 * Make --checks backwards comp Signed-off-by: Oliver Tale-Yazdi * Add clap attr comment Signed-off-by: Oliver Tale-Yazdi Signed-off-by: Oliver Tale-Yazdi Co-authored-by: Bastian Köcher Co-authored-by: kianenigma --- bin/node-template/runtime/src/lib.rs | 2 +- bin/node/runtime/src/lib.rs | 2 +- frame/executive/src/lib.rs | 10 +++-- frame/nomination-pools/src/lib.rs | 17 +++++--- frame/support/src/traits.rs | 2 +- frame/support/src/traits/try_runtime.rs | 42 ++++++++++++++++++- frame/try-runtime/src/lib.rs | 4 +- .../cli/src/commands/on_runtime_upgrade.rs | 21 +++++++--- 8 files changed, 79 insertions(+), 21 deletions(-) diff --git a/bin/node-template/runtime/src/lib.rs b/bin/node-template/runtime/src/lib.rs index f76b2c449ee4a..f4372af525da5 100644 --- a/bin/node-template/runtime/src/lib.rs +++ b/bin/node-template/runtime/src/lib.rs @@ -539,7 +539,7 @@ impl_runtime_apis! { #[cfg(feature = "try-runtime")] impl frame_try_runtime::TryRuntime 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. diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 90a52ab697dee..a9e93a16f0713 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -2196,7 +2196,7 @@ impl_runtime_apis! { #[cfg(feature = "try-runtime")] impl frame_try_runtime::TryRuntime 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. diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index 1f20e93f43c30..b454d1e276cc8 100644 --- a/frame/executive/src/lib.rs +++ b/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 { - if checks { + pub fn try_runtime_upgrade( + checks: frame_try_runtime::UpgradeCheckSelect, + ) -> Result { + if checks.try_state() { let _guard = frame_support::StorageNoopGuard::default(); >::try_state( frame_system::Pallet::::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(); >::try_state( frame_system::Pallet::::block_number(), diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 5e5385b62acd3..99bfc8b8c36a5 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -2470,12 +2470,17 @@ impl Pallet { 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::::new(); diff --git a/frame/support/src/traits.rs b/frame/support/src/traits.rs index 63c86c1f68459..f7b57f446fee8 100644 --- a/frame/support/src/traits.rs +++ b/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}; diff --git a/frame/support/src/traits/try_runtime.rs b/frame/support/src/traits/try_runtime.rs index f741ca56a56fc..273af09a145e5 100644 --- a/frame/support/src/traits/try_runtime.rs +++ b/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 { + 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 diff --git a/frame/try-runtime/src/lib.rs b/frame/try-runtime/src/lib.rs index 99c68d4dc65b8..d8b117d1ff059 100644 --- a/frame/try-runtime/src/lib.rs +++ b/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. /// diff --git a/utils/frame/try-runtime/cli/src/commands/on_runtime_upgrade.rs b/utils/frame/try-runtime/cli/src/commands/on_runtime_upgrade.rs index 2b2800d505864..81f81d6a551b9 100644 --- a/utils/frame/try-runtime/cli/src/commands/on_runtime_upgrade.rs +++ b/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(