From 33c01bbee900b0d93a5e8406da0f862392562f0a Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 2 Jan 2023 21:27:57 +0100 Subject: [PATCH 1/8] Make try-runtime checks selectable Signed-off-by: Oliver Tale-Yazdi --- bin/node-template/runtime/src/lib.rs | 2 +- bin/node/runtime/src/lib.rs | 2 +- frame/executive/src/lib.rs | 10 +++-- 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 | 15 ++++--- 7 files changed, 62 insertions(+), 15 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 0c946fa180f20..0edf0d5b61179 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -2197,7 +2197,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/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..314cd560a049e 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)] +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..6f755244748df 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,16 @@ 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: /// - /// This will perform more checks, but it will also makes the reported PoV/Weight be - /// inaccurate. - #[clap(long)] - pub checks: bool, + /// - `all`: Perform all checks (default). + /// - `pre-and-post`: Perform pre- and post-upgrade checks. + /// - `try-state`: Perform the try-state checks. + /// - `none`: Perform no checks. + /// + /// Performing any checks will potentially invalidate the measured PoV/Weight. + #[clap(long, default_value = "All", verbatim_doc_comment)] + pub checks: UpgradeCheckSelect, } pub(crate) async fn on_runtime_upgrade( From 1f9eb3a0e07bb7569f97cb1660c271526b95b177 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 3 Jan 2023 12:49:09 +0100 Subject: [PATCH 2/8] Update frame/support/src/traits/try_runtime.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- frame/support/src/traits/try_runtime.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/support/src/traits/try_runtime.rs b/frame/support/src/traits/try_runtime.rs index 314cd560a049e..273af09a145e5 100644 --- a/frame/support/src/traits/try_runtime.rs +++ b/frame/support/src/traits/try_runtime.rs @@ -82,7 +82,7 @@ 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)] +#[derive(codec::Encode, codec::Decode, Clone, Debug, Copy)] pub enum UpgradeCheckSelect { /// Run no checks. None, From e29538c1a79d1711b43addc9400d871f6aa32844 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 3 Jan 2023 12:50:34 +0100 Subject: [PATCH 3/8] Add Clap wrapper for enum UpgradeCheckSelect Signed-off-by: Oliver Tale-Yazdi --- frame/support/src/traits/try_runtime.rs | 15 ------- .../cli/src/commands/on_runtime_upgrade.rs | 39 ++++++++++++++----- 2 files changed, 30 insertions(+), 24 deletions(-) diff --git a/frame/support/src/traits/try_runtime.rs b/frame/support/src/traits/try_runtime.rs index 273af09a145e5..29bce49762be5 100644 --- a/frame/support/src/traits/try_runtime.rs +++ b/frame/support/src/traits/try_runtime.rs @@ -106,21 +106,6 @@ impl UpgradeCheckSelect { } } -#[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/utils/frame/try-runtime/cli/src/commands/on_runtime_upgrade.rs b/utils/frame/try-runtime/cli/src/commands/on_runtime_upgrade.rs index 6f755244748df..38e4b8c23f6b7 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,7 +16,6 @@ // 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}; @@ -30,18 +29,39 @@ pub struct OnRuntimeUpgradeCmd { #[command(subcommand)] pub state: State, - /// Select which optional checks to perform: - /// - /// - `all`: Perform all checks (default). - /// - `pre-and-post`: Perform pre- and post-upgrade checks. - /// - `try-state`: Perform the try-state checks. - /// - `none`: Perform no checks. + /// Select which optional checks to perform. /// /// Performing any checks will potentially invalidate the measured PoV/Weight. - #[clap(long, default_value = "All", verbatim_doc_comment)] + #[clap(long, value_enum, default_value_t = UpgradeCheckSelect::All)] pub checks: UpgradeCheckSelect, } +/// This is an adapter for [`frame_try_runtime::UpgradeCheckSelect`] since that does not implement +/// `clap::ValueEnum`. +#[derive(clap::ValueEnum, Debug, Clone, Copy)] +#[value(rename_all = "kebab-case")] +pub enum UpgradeCheckSelect { + /// Perform no checks. + None, + /// Perform all checks. + All, + /// Perform pre- and post-upgrade checks. + PreAndPost, + /// Perform the try-state checks. + TryState, +} + +impl From for frame_try_runtime::UpgradeCheckSelect { + fn from(x: UpgradeCheckSelect) -> Self { + match x { + UpgradeCheckSelect::None => Self::None, + UpgradeCheckSelect::All => Self::All, + UpgradeCheckSelect::PreAndPost => Self::PreAndPost, + UpgradeCheckSelect::TryState => Self::TryState, + } + } +} + pub(crate) async fn on_runtime_upgrade( shared: SharedParams, command: OnRuntimeUpgradeCmd, @@ -57,12 +77,13 @@ where { let executor = build_executor(&shared); let ext = command.state.into_ext::(&shared, &executor, None).await?; + let checks: frame_try_runtime::UpgradeCheckSelect = command.checks.into(); let (_, encoded_result) = state_machine_call_with_proof::( &ext, &executor, "TryRuntime_on_runtime_upgrade", - command.checks.encode().as_ref(), + checks.encode().as_ref(), Default::default(), // we don't really need any extensions here. shared.export_proof, )?; From fbd67246a1d8e1a0b5d4db0798af4a755901fca3 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 3 Jan 2023 14:41:47 +0100 Subject: [PATCH 4/8] Revert "Add Clap wrapper for enum UpgradeCheckSelect" This reverts commit e29538c1a79d1711b43addc9400d871f6aa32844. --- frame/support/src/traits/try_runtime.rs | 15 +++++++ .../cli/src/commands/on_runtime_upgrade.rs | 39 +++++-------------- 2 files changed, 24 insertions(+), 30 deletions(-) diff --git a/frame/support/src/traits/try_runtime.rs b/frame/support/src/traits/try_runtime.rs index 29bce49762be5..273af09a145e5 100644 --- a/frame/support/src/traits/try_runtime.rs +++ b/frame/support/src/traits/try_runtime.rs @@ -106,6 +106,21 @@ impl UpgradeCheckSelect { } } +#[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/utils/frame/try-runtime/cli/src/commands/on_runtime_upgrade.rs b/utils/frame/try-runtime/cli/src/commands/on_runtime_upgrade.rs index 38e4b8c23f6b7..6f755244748df 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,39 +30,18 @@ pub struct OnRuntimeUpgradeCmd { #[command(subcommand)] pub state: State, - /// Select which optional checks to perform. + /// Select which optional checks to perform: + /// + /// - `all`: Perform all checks (default). + /// - `pre-and-post`: Perform pre- and post-upgrade checks. + /// - `try-state`: Perform the try-state checks. + /// - `none`: Perform no checks. /// /// Performing any checks will potentially invalidate the measured PoV/Weight. - #[clap(long, value_enum, default_value_t = UpgradeCheckSelect::All)] + #[clap(long, default_value = "All", verbatim_doc_comment)] pub checks: UpgradeCheckSelect, } -/// This is an adapter for [`frame_try_runtime::UpgradeCheckSelect`] since that does not implement -/// `clap::ValueEnum`. -#[derive(clap::ValueEnum, Debug, Clone, Copy)] -#[value(rename_all = "kebab-case")] -pub enum UpgradeCheckSelect { - /// Perform no checks. - None, - /// Perform all checks. - All, - /// Perform pre- and post-upgrade checks. - PreAndPost, - /// Perform the try-state checks. - TryState, -} - -impl From for frame_try_runtime::UpgradeCheckSelect { - fn from(x: UpgradeCheckSelect) -> Self { - match x { - UpgradeCheckSelect::None => Self::None, - UpgradeCheckSelect::All => Self::All, - UpgradeCheckSelect::PreAndPost => Self::PreAndPost, - UpgradeCheckSelect::TryState => Self::TryState, - } - } -} - pub(crate) async fn on_runtime_upgrade( shared: SharedParams, command: OnRuntimeUpgradeCmd, @@ -77,13 +57,12 @@ where { let executor = build_executor(&shared); let ext = command.state.into_ext::(&shared, &executor, None).await?; - let checks: frame_try_runtime::UpgradeCheckSelect = command.checks.into(); let (_, encoded_result) = state_machine_call_with_proof::( &ext, &executor, "TryRuntime_on_runtime_upgrade", - checks.encode().as_ref(), + command.checks.encode().as_ref(), Default::default(), // we don't really need any extensions here. shared.export_proof, )?; From d32ae6150d69e9ac8e7a90bb40346d4b063dc40d Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 3 Jan 2023 14:43:09 +0100 Subject: [PATCH 5/8] fix pools sanity check --- frame/nomination-pools/src/lib.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) 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(); From d6b2f4d378cace76e20504d8e6d490bd4940ecf6 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 3 Jan 2023 14:43:41 +0100 Subject: [PATCH 6/8] Set default for --checks to None Signed-off-by: Oliver Tale-Yazdi --- .../try-runtime/cli/src/commands/on_runtime_upgrade.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 6f755244748df..9a0381d87ad56 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 @@ -32,13 +32,13 @@ pub struct OnRuntimeUpgradeCmd { /// Select which optional checks to perform: /// - /// - `all`: Perform all checks (default). + /// - `none`: Perform no checks (default). + /// - `all`: Perform all checks. /// - `pre-and-post`: Perform pre- and post-upgrade checks. /// - `try-state`: Perform the try-state checks. - /// - `none`: Perform no checks. /// /// Performing any checks will potentially invalidate the measured PoV/Weight. - #[clap(long, default_value = "All", verbatim_doc_comment)] + #[clap(long, default_value = "None", verbatim_doc_comment)] pub checks: UpgradeCheckSelect, } From ca27a6ab9639a13d919c6683856ab1e8d589d28b Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 3 Jan 2023 17:34:37 +0100 Subject: [PATCH 7/8] Make --checks backwards comp Signed-off-by: Oliver Tale-Yazdi --- .../cli/src/commands/on_runtime_upgrade.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) 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 9a0381d87ad56..92bb6846d6c4c 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 @@ -30,15 +30,20 @@ pub struct OnRuntimeUpgradeCmd { #[command(subcommand)] pub state: State, - /// Select which optional checks to perform: + /// Select which optional checks to perform. Selects all when no value is given. /// - /// - `none`: Perform no checks (default). - /// - `all`: Perform all checks. + /// - `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. - #[clap(long, default_value = "None", verbatim_doc_comment)] + #[clap(long, + default_value = "None", + default_missing_value = "All", + num_args = 0..=1, + require_equals = true, + verbatim_doc_comment)] pub checks: UpgradeCheckSelect, } From 7269f21a11fe842d7bc8fa49d65bc8cd55bca768 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 4 Jan 2023 13:16:36 +0100 Subject: [PATCH 8/8] Add clap attr comment Signed-off-by: Oliver Tale-Yazdi --- utils/frame/try-runtime/cli/src/commands/on_runtime_upgrade.rs | 1 + 1 file changed, 1 insertion(+) 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 92bb6846d6c4c..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 @@ -38,6 +38,7 @@ pub struct OnRuntimeUpgradeCmd { /// - `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",