Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Selectable on-runtime-upgrade checks #13045

Merged
merged 10 commits into from
Jan 4, 2023
2 changes: 1 addition & 1 deletion bin/node-template/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2197,7 +2197,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.
Expand Down
10 changes: 6 additions & 4 deletions frame/executive/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -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(),
Expand Down
17 changes: 11 additions & 6 deletions frame/nomination-pools/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2470,12 +2470,17 @@ impl<T: Config> Pallet<T> {

for id in reward_pools {
let account = Self::create_reward_account(id);
assert!(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you removed the assert?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did, it does not hold anymore because we changed ED in Kusama :(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
Expand Down
2 changes: 1 addition & 1 deletion frame/support/src/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
42 changes: 41 additions & 1 deletion frame/support/src/traits/try_runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions frame/try-runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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! {
Expand All @@ -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.
///
Expand Down
20 changes: 15 additions & 5 deletions utils/frame/try-runtime/cli/src/commands/on_runtime_upgrade.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -29,12 +30,21 @@ 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.
#[clap(long,
default_value = "None",
default_missing_value = "All",
num_args = 0..=1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this or? default_value should indicate this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could also use some context on what these flags do, but I believe they help us remain backwards compat.

Copy link
Member Author

@ggwpez ggwpez Jan 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is needed since otherwise it does not work as flag without arg. The difference in the help print:
--checks=<CHECKS> (without num_args)
vs
--checks[=<CHECKS>] (with num_args)

I added a comment.

require_equals = true,
verbatim_doc_comment)]
pub checks: UpgradeCheckSelect,
}

pub(crate) async fn on_runtime_upgrade<Block, HostFns>(
Expand Down