From d60a7f3b9f327ecd364ad474c13c6b02500fa00c Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sun, 15 Nov 2026 22:31:31 +0100 Subject: [PATCH 1/4] Split SBM and MBM check opts Signed-off-by: Oliver Tale-Yazdi --- README.md | 15 +- cli/main.rs | 15 +- cli/tests/on_runtime_upgrade.rs | 8 +- core/src/commands/on_runtime_upgrade/mbms.rs | 4 +- core/src/commands/on_runtime_upgrade/mod.rs | 162 +++++++++++++++---- 5 files changed, 164 insertions(+), 40 deletions(-) diff --git a/README.md b/README.md index 831a1c3b272..8fcadb34ace 100644 --- a/README.md +++ b/README.md @@ -136,9 +136,18 @@ after the migration. [`OnRuntimeUpgrade::pre_upgrade`] returns a [`Vec`] tha arbitrary encoded data (usually some pre-upgrade state) which will be passed to [`OnRuntimeUpgrade::pre_upgrade`] after upgrading and used for post checking. -**Note on Multi-Block Migrations (MBM):** If the runtime uses MBMs, the standard -`pre_upgrade` and `post_upgrade` checks might be skipped by the executive. To -force these hooks to run synchronously for testing, use the `--disable-mbm-checks` flag. +Use either `--checks` or both `--sbm-checks` and `--mbm-checks`; mixing modes or +providing only one new selector is rejected. All accept `none`, `pre-and-post`, +`try-state`, or `all`; a flag without a value selects `all`. + +`--checks` retains its original behavior (default: `pre-and-post`): checks run +after MBMs, or around SBMs when `--disable-mbm-checks` is set. +The paired selectors independently control checks around SBMs on original state +and the upgrade checks run again after MBMs finish. For SBM pre/post hooks and +all final invariants, use `--sbm-checks=pre-and-post --mbm-checks=all`. +Only enable SBM `try-state` checks if invariants hold before MBMs finish. +MBMs' own pre/post hooks run inside the runtime independently of these selectors. +`--mbm-checks=none` still executes MBMs; `--disable-mbm-checks` skips that execution. ### [`VersionedMigration`] diff --git a/cli/main.rs b/cli/main.rs index 45ee153b8e7..ffa13bece15 100644 --- a/cli/main.rs +++ b/cli/main.rs @@ -152,9 +152,18 @@ //! arbitrary encoded data (usually some pre-upgrade state) which will be passed to //! [`OnRuntimeUpgrade::pre_upgrade`] after upgrading and used for post checking. //! -//! **Note on Multi-Block Migrations (MBM):** If the runtime uses MBMs, the standard -//! `pre_upgrade` and `post_upgrade` checks might be skipped by the executive. To -//! force these hooks to run synchronously for testing, use the `--disable-mbm-checks` flag. +//! Use either `--checks` or both `--sbm-checks` and `--mbm-checks`; mixing modes or +//! providing only one new selector is rejected. All accept `none`, `pre-and-post`, +//! `try-state`, or `all`; a flag without a value selects `all`. +//! +//! `--checks` retains its original behavior (default: `pre-and-post`): checks run +//! after MBMs, or around SBMs when `--disable-mbm-checks` is set. +//! The paired selectors independently control checks around SBMs on original state +//! and the upgrade checks run again after MBMs finish. For SBM pre/post hooks and +//! all final invariants, use `--sbm-checks=pre-and-post --mbm-checks=all`. +//! Only enable SBM `try-state` checks if invariants hold before MBMs finish. +//! MBMs' own pre/post hooks run inside the runtime independently of these selectors. +//! `--mbm-checks=none` still executes MBMs; `--disable-mbm-checks` skips that execution. //! //! ### [`VersionedMigration`] //! diff --git a/cli/tests/on_runtime_upgrade.rs b/cli/tests/on_runtime_upgrade.rs index ec16a232df9..d13e23f3d0d 100644 --- a/cli/tests/on_runtime_upgrade.rs +++ b/cli/tests/on_runtime_upgrade.rs @@ -153,7 +153,7 @@ mod on_runtime_upgrade { async fn pre_upgrade_fail_pre_and_postfails() { run_test( TestConfig::new("rococo-people", "people_rococo_runtime_pre_upgrade_fail") - .with_sub_command_args(&["--checks=pre-and-post"]), + .with_sub_command_args(&["--sbm-checks=pre-and-post", "--mbm-checks=none"]), false, ) .await; @@ -163,7 +163,7 @@ mod on_runtime_upgrade { async fn pre_upgrade_fail_can_be_ignored() { run_test( TestConfig::new("rococo-people", "people_rococo_runtime_pre_upgrade_fail") - .with_sub_command_args(&["--checks=none"]), + .with_sub_command_args(&["--sbm-checks=none", "--mbm-checks=none"]), true, ) .await; @@ -182,7 +182,7 @@ mod on_runtime_upgrade { async fn post_upgrade_fail_pre_and_postfails() { run_test( TestConfig::new("rococo-people", "people_rococo_runtime_post_upgrade_fail") - .with_sub_command_args(&["--checks=pre-and-post"]), + .with_sub_command_args(&["--sbm-checks=pre-and-post", "--mbm-checks=none"]), false, ) .await; @@ -192,7 +192,7 @@ mod on_runtime_upgrade { async fn post_upgrade_fail_can_be_ignored() { run_test( TestConfig::new("rococo-people", "people_rococo_runtime_post_upgrade_fail") - .with_sub_command_args(&["--checks=none"]), + .with_sub_command_args(&["--sbm-checks=none", "--mbm-checks=none"]), true, ) .await; diff --git a/core/src/commands/on_runtime_upgrade/mbms.rs b/core/src/commands/on_runtime_upgrade/mbms.rs index b8710cfdda6..3e5a620b5de 100644 --- a/core/src/commands/on_runtime_upgrade/mbms.rs +++ b/core/src/commands/on_runtime_upgrade/mbms.rs @@ -47,7 +47,7 @@ where Level::Info, &format!( "🔬 Running Multi-Block-Migrations with checks: {:?}", - self.command.checks + self.command.multi_block_checks() ), ); @@ -114,7 +114,7 @@ where &mut Default::default(), &executor, "TryRuntime_on_runtime_upgrade", - self.command.checks.encode().as_ref(), + self.command.multi_block_checks().encode().as_ref(), Default::default(), // TODO None, )?; diff --git a/core/src/commands/on_runtime_upgrade/mod.rs b/core/src/commands/on_runtime_upgrade/mod.rs index 1c919dae42c..5d6ddb06339 100644 --- a/core/src/commands/on_runtime_upgrade/mod.rs +++ b/core/src/commands/on_runtime_upgrade/mod.rs @@ -52,24 +52,31 @@ pub struct Command { #[command(subcommand)] pub state: State, - /// Select which optional checks to perform. Selects all when no value is given. + /// Legacy check selection: after MBMs, or around SBMs with --disable-mbm-checks. /// - /// - `none`: Perform no checks. - /// - `all`: Perform all checks (default when --checks is present with no value). - /// - `pre-and-post`: Perform pre- and post-upgrade checks (default when the arg is not - /// present). - /// - `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 = "pre-and-post", - default_missing_value = "all", - num_args = 0..=1, - verbatim_doc_comment - )] + /// Select none, pre-and-post, try-state, or all. Bare --checks selects all. + #[clap(long, default_value = "pre-and-post", default_missing_value = "all", num_args = 0..=1, + conflicts_with_all = ["sbm_checks", "mbm_checks"])] pub checks: UpgradeCheckSelect, + /// Checks around single-block migrations on the original state. + /// + /// Select none, pre-and-post, try-state, or all. Bare --sbm-checks selects all. + /// Must be supplied together with --mbm-checks, instead of --checks. + /// Only select try-state or all if invariants hold before MBMs finish. + #[clap(long, default_missing_value = "all", num_args = 0..=1, requires = "mbm_checks")] + pub sbm_checks: Option, + + /// Checks on the migrated state after multi-block migrations finish. + /// + /// Select none, pre-and-post, try-state, or all. Bare --mbm-checks selects all. + /// These are passed to TryRuntime_on_runtime_upgrade after block execution; + /// they do not control the MBMs' own pre/post hooks inside the runtime. + /// Selecting none still executes MBMs. Use --disable-mbm-checks to skip them. + /// Must be supplied together with --sbm-checks, instead of --checks. + #[clap(long, default_missing_value = "all", num_args = 0..=1, requires = "sbm_checks")] + pub mbm_checks: Option, + /// Whether to disable weight warnings. Useful if a relay chain's runtime is being tested. #[clap(long, default_value = "false", default_missing_value = "true")] pub no_weight_warnings: bool, @@ -104,6 +111,115 @@ pub struct Command { pub blocktime: u64, } +impl Command { + fn single_block_checks(&self) -> UpgradeCheckSelect { + self.sbm_checks.unwrap_or_else(|| { + if self.disable_mbm_checks { + self.checks + } else { + UpgradeCheckSelect::None + } + }) + } + + fn multi_block_checks(&self) -> UpgradeCheckSelect { + self.mbm_checks.unwrap_or(self.checks) + } +} + +#[cfg(test)] +mod selector_tests { + use super::*; + use clap::{error::ErrorKind, Parser}; + + fn parse(args: &[&str]) -> Result { + Command::try_parse_from( + ["on-runtime-upgrade"] + .into_iter() + .chain(args.iter().copied()) + .chain(["--blocktime=6000", "snap", "--path=unused.snap"]), + ) + } + + #[test] + fn legacy_selection_preserves_scheduling() { + let command = parse(&[]).unwrap(); + assert!(matches!( + command.single_block_checks(), + UpgradeCheckSelect::None + )); + assert!(matches!( + command.multi_block_checks(), + UpgradeCheckSelect::PreAndPost + )); + let command = parse(&["--checks"]).unwrap(); + assert!(matches!( + command.single_block_checks(), + UpgradeCheckSelect::None + )); + assert!(matches!( + command.multi_block_checks(), + UpgradeCheckSelect::All + )); + let command = parse(&["--checks=all", "--disable-mbm-checks"]).unwrap(); + assert!(matches!( + command.single_block_checks(), + UpgradeCheckSelect::All + )); + } + + #[test] + fn paired_selection_is_independent() { + let command = parse(&["--sbm-checks=pre-and-post", "--mbm-checks=all"]).unwrap(); + assert!(matches!( + command.single_block_checks(), + UpgradeCheckSelect::PreAndPost + )); + assert!(matches!( + command.multi_block_checks(), + UpgradeCheckSelect::All + )); + let command = parse(&["--sbm-checks=none", "--mbm-checks=try-state"]).unwrap(); + assert!(matches!( + command.single_block_checks(), + UpgradeCheckSelect::None + )); + assert!(matches!( + command.multi_block_checks(), + UpgradeCheckSelect::TryState + )); + let command = parse(&["--sbm-checks", "--mbm-checks", "--disable-mbm-checks"]).unwrap(); + assert!(matches!( + command.single_block_checks(), + UpgradeCheckSelect::All + )); + assert!(matches!( + command.multi_block_checks(), + UpgradeCheckSelect::All + )); + } + + #[test] + fn rejects_incomplete_or_mixed_selection() { + for flag in ["--sbm-checks=all", "--mbm-checks=all"] { + assert_eq!( + parse(&[flag]).unwrap_err().kind(), + ErrorKind::MissingRequiredArgument + ); + assert_eq!( + parse(&["--checks=all", flag]).unwrap_err().kind(), + ErrorKind::ArgumentConflict, + ); + } + assert_eq!( + parse(&["--checks=all", "--sbm-checks=all", "--mbm-checks=all"]) + .unwrap_err() + .kind(), + ErrorKind::ArgumentConflict, + ); + } +} + /// Convenience struct to hold all the generic args and where clauses. pub(crate) struct CheckOnRuntimeUpgrade { pub shared: SharedParams, @@ -136,18 +252,12 @@ where .to_ext::(shared, &executor, None, runtime_checks) .await?; - let sync_checks = if command.disable_mbm_checks { - command.checks - } else { - UpgradeCheckSelect::None - }; - // Run `TryRuntime_on_runtime_upgrade` with the given checks. basti_log( Level::Info, format!( "🔬 Running TryRuntime_on_runtime_upgrade with checks: {:?}", - sync_checks + command.single_block_checks() ) .as_str(), ); @@ -159,7 +269,7 @@ where &mut overlayed_changes, &executor, "TryRuntime_on_runtime_upgrade", - sync_checks.encode().as_ref(), + command.single_block_checks().encode().as_ref(), Default::default(), // we don't really need any extensions here. shared.export_proof.clone(), )?; @@ -187,11 +297,7 @@ where if !self.command.disable_idempotency_checks { basti_log( Level::Info, - format!( - "🔬 Running TryRuntime_on_runtime_upgrade again to check idempotency: {:?}", - self.command.checks - ) - .as_str(), + "🔬 Running TryRuntime_on_runtime_upgrade again to check idempotency without optional checks.", ); let executor = build_executor(&self.shared); From d312570b2b6f8597cd8781cfce40d268cf8aef2c Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sun, 15 Nov 2026 22:34:57 +0100 Subject: [PATCH 2/4] fmt Signed-off-by: Oliver Tale-Yazdi --- core/src/commands/on_runtime_upgrade/mod.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/commands/on_runtime_upgrade/mod.rs b/core/src/commands/on_runtime_upgrade/mod.rs index 5d6ddb06339..67de32dcbd9 100644 --- a/core/src/commands/on_runtime_upgrade/mod.rs +++ b/core/src/commands/on_runtime_upgrade/mod.rs @@ -129,9 +129,10 @@ impl Command { #[cfg(test)] mod selector_tests { - use super::*; use clap::{error::ErrorKind, Parser}; + use super::*; + fn parse(args: &[&str]) -> Result { Command::try_parse_from( ["on-runtime-upgrade"] From 1318d54a42510b32384682bf2d61d48971b0208a Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sun, 15 Nov 2026 23:07:02 +0100 Subject: [PATCH 3/4] better flag handling Signed-off-by: Oliver Tale-Yazdi --- README.md | 17 ++++--- cli/main.rs | 17 ++++--- cli/tests/on_runtime_upgrade.rs | 34 +++++++++++++- core/src/commands/on_runtime_upgrade/mbms.rs | 2 +- core/src/commands/on_runtime_upgrade/mod.rs | 48 +++++++++++++++----- 5 files changed, 89 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index 8fcadb34ace..42ecc6ac329 100644 --- a/README.md +++ b/README.md @@ -136,18 +136,21 @@ after the migration. [`OnRuntimeUpgrade::pre_upgrade`] returns a [`Vec`] tha arbitrary encoded data (usually some pre-upgrade state) which will be passed to [`OnRuntimeUpgrade::pre_upgrade`] after upgrading and used for post checking. -Use either `--checks` or both `--sbm-checks` and `--mbm-checks`; mixing modes or -providing only one new selector is rejected. All accept `none`, `pre-and-post`, -`try-state`, or `all`; a flag without a value selects `all`. +Use either `--checks` or the per-phase `--sbm-checks` and `--mbm-checks`; mixing +them is rejected. All accept `none`, `pre-and-post`, `try-state`, or `all`; a flag +without a value selects `all`. `--checks` retains its original behavior (default: `pre-and-post`): checks run after MBMs, or around SBMs when `--disable-mbm-checks` is set. -The paired selectors independently control checks around SBMs on original state -and the upgrade checks run again after MBMs finish. For SBM pre/post hooks and -all final invariants, use `--sbm-checks=pre-and-post --mbm-checks=all`. + +`--sbm-checks` selects the checks around SBMs on the original state. `--mbm-checks` +selects the checks of the upgrade run after MBMs finish; its pre/post checks run the +SBM hooks again on the migrated state. Both are required, except that +`--disable-mbm-checks` skips MBMs and takes `--sbm-checks` alone. +Recommended: `--sbm-checks=pre-and-post --mbm-checks=try-state`. Only enable SBM `try-state` checks if invariants hold before MBMs finish. MBMs' own pre/post hooks run inside the runtime independently of these selectors. -`--mbm-checks=none` still executes MBMs; `--disable-mbm-checks` skips that execution. +`--mbm-checks=none` still executes MBMs. ### [`VersionedMigration`] diff --git a/cli/main.rs b/cli/main.rs index ffa13bece15..07f67f377b0 100644 --- a/cli/main.rs +++ b/cli/main.rs @@ -152,18 +152,21 @@ //! arbitrary encoded data (usually some pre-upgrade state) which will be passed to //! [`OnRuntimeUpgrade::pre_upgrade`] after upgrading and used for post checking. //! -//! Use either `--checks` or both `--sbm-checks` and `--mbm-checks`; mixing modes or -//! providing only one new selector is rejected. All accept `none`, `pre-and-post`, -//! `try-state`, or `all`; a flag without a value selects `all`. +//! Use either `--checks` or the per-phase `--sbm-checks` and `--mbm-checks`; mixing +//! them is rejected. All accept `none`, `pre-and-post`, `try-state`, or `all`; a flag +//! without a value selects `all`. //! //! `--checks` retains its original behavior (default: `pre-and-post`): checks run //! after MBMs, or around SBMs when `--disable-mbm-checks` is set. -//! The paired selectors independently control checks around SBMs on original state -//! and the upgrade checks run again after MBMs finish. For SBM pre/post hooks and -//! all final invariants, use `--sbm-checks=pre-and-post --mbm-checks=all`. +//! +//! `--sbm-checks` selects the checks around SBMs on the original state. `--mbm-checks` +//! selects the checks of the upgrade run after MBMs finish; its pre/post checks run the +//! SBM hooks again on the migrated state. Both are required, except that +//! `--disable-mbm-checks` skips MBMs and takes `--sbm-checks` alone. +//! Recommended: `--sbm-checks=pre-and-post --mbm-checks=try-state`. //! Only enable SBM `try-state` checks if invariants hold before MBMs finish. //! MBMs' own pre/post hooks run inside the runtime independently of these selectors. -//! `--mbm-checks=none` still executes MBMs; `--disable-mbm-checks` skips that execution. +//! `--mbm-checks=none` still executes MBMs. //! //! ### [`VersionedMigration`] //! diff --git a/cli/tests/on_runtime_upgrade.rs b/cli/tests/on_runtime_upgrade.rs index d13e23f3d0d..c58816257c7 100644 --- a/cli/tests/on_runtime_upgrade.rs +++ b/cli/tests/on_runtime_upgrade.rs @@ -169,6 +169,36 @@ mod on_runtime_upgrade { .await; } + #[tokio::test] + async fn pre_upgrade_fail_legacy_checks_can_be_ignored() { + run_test( + TestConfig::new("rococo-people", "people_rococo_runtime_pre_upgrade_fail") + .with_sub_command_args(&["--checks=none"]), + true, + ) + .await; + } + + #[tokio::test] + async fn pre_upgrade_fail_mbm_checks_fails() { + run_test( + TestConfig::new("rococo-people", "people_rococo_runtime_pre_upgrade_fail") + .with_sub_command_args(&["--sbm-checks=none", "--mbm-checks=pre-and-post"]), + false, + ) + .await; + } + + #[tokio::test] + async fn pre_upgrade_fail_sbm_checks_without_mbms_fails() { + run_test( + TestConfig::new("rococo-people", "people_rococo_runtime_pre_upgrade_fail") + .with_sub_command_args(&["--sbm-checks=pre-and-post", "--disable-mbm-checks"]), + false, + ) + .await; + } + #[tokio::test] async fn post_upgrade_fail_fails() { run_test( @@ -383,8 +413,8 @@ mod on_runtime_upgrade { async fn mbm_fail_ignore_mbms() { run_test( TestConfig::new("rococo-people", "people_rococo_runtime_mbm_fails") - .with_sub_command_args(&["--disable-mbms-checks"]), - false, + .with_sub_command_args(&["--disable-mbm-checks"]), + true, ) .await; } diff --git a/core/src/commands/on_runtime_upgrade/mbms.rs b/core/src/commands/on_runtime_upgrade/mbms.rs index 3e5a620b5de..d8c21a85d9c 100644 --- a/core/src/commands/on_runtime_upgrade/mbms.rs +++ b/core/src/commands/on_runtime_upgrade/mbms.rs @@ -59,7 +59,7 @@ where .await?; if core_version::(&ext, &executor)? < 5 { - return Err("Your runtime does not support Multi-Block-Migrations. Please disable the check with `--mbms false` or update your runtime.".into()); + return Err("Your runtime does not support Multi-Block-Migrations. Please disable the check with `--disable-mbm-checks` or update your runtime.".into()); } let inner_ext = Arc::new(Mutex::new(ext.inner_ext)); diff --git a/core/src/commands/on_runtime_upgrade/mod.rs b/core/src/commands/on_runtime_upgrade/mod.rs index 67de32dcbd9..fb55936d50a 100644 --- a/core/src/commands/on_runtime_upgrade/mod.rs +++ b/core/src/commands/on_runtime_upgrade/mod.rs @@ -52,29 +52,43 @@ pub struct Command { #[command(subcommand)] pub state: State, - /// Legacy check selection: after MBMs, or around SBMs with --disable-mbm-checks. + /// Select which optional checks to perform. Selects all when no value is given. /// - /// Select none, pre-and-post, try-state, or all. Bare --checks selects all. - #[clap(long, default_value = "pre-and-post", default_missing_value = "all", num_args = 0..=1, - conflicts_with_all = ["sbm_checks", "mbm_checks"])] + /// - `none`: Perform no checks. + /// - `all`: Perform all checks (default when --checks is present with no value). + /// - `pre-and-post`: Perform pre- and post-upgrade checks (default when the arg is not + /// present). + /// - `try-state`: Perform the try-state checks. + /// + /// Checks run after MBMs, or around SBMs with --disable-mbm-checks. + /// Cannot be combined with --sbm-checks or --mbm-checks. + // NOTE: The clap attributes make it backwards compatible with the previous `--checks` flag. + #[clap(long, + default_value = "pre-and-post", + default_missing_value = "all", + num_args = 0..=1, + conflicts_with_all = ["sbm_checks", "mbm_checks"], + verbatim_doc_comment + )] pub checks: UpgradeCheckSelect, /// Checks around single-block migrations on the original state. /// - /// Select none, pre-and-post, try-state, or all. Bare --sbm-checks selects all. - /// Must be supplied together with --mbm-checks, instead of --checks. + /// Accepts the same values as --checks. Bare --sbm-checks selects all. + /// Requires --mbm-checks unless --disable-mbm-checks is set. /// Only select try-state or all if invariants hold before MBMs finish. #[clap(long, default_missing_value = "all", num_args = 0..=1, requires = "mbm_checks")] pub sbm_checks: Option, /// Checks on the migrated state after multi-block migrations finish. /// - /// Select none, pre-and-post, try-state, or all. Bare --mbm-checks selects all. + /// Accepts the same values as --checks. Bare --mbm-checks selects all. /// These are passed to TryRuntime_on_runtime_upgrade after block execution; /// they do not control the MBMs' own pre/post hooks inside the runtime. /// Selecting none still executes MBMs. Use --disable-mbm-checks to skip them. - /// Must be supplied together with --sbm-checks, instead of --checks. - #[clap(long, default_missing_value = "all", num_args = 0..=1, requires = "sbm_checks")] + /// Requires --sbm-checks; conflicts with --disable-mbm-checks. + #[clap(long, default_missing_value = "all", num_args = 0..=1, requires = "sbm_checks", + conflicts_with = "disable_mbm_checks")] pub mbm_checks: Option, /// Whether to disable weight warnings. Useful if a relay chain's runtime is being tested. @@ -123,7 +137,11 @@ impl Command { } fn multi_block_checks(&self) -> UpgradeCheckSelect { - self.mbm_checks.unwrap_or(self.checks) + match (self.sbm_checks, self.mbm_checks) { + (_, Some(checks)) => checks, + (Some(_), None) => UpgradeCheckSelect::None, + (None, None) => self.checks, + } } } @@ -189,14 +207,14 @@ mod selector_tests { command.multi_block_checks(), UpgradeCheckSelect::TryState )); - let command = parse(&["--sbm-checks", "--mbm-checks", "--disable-mbm-checks"]).unwrap(); + let command = parse(&["--sbm-checks", "--disable-mbm-checks"]).unwrap(); assert!(matches!( command.single_block_checks(), UpgradeCheckSelect::All )); assert!(matches!( command.multi_block_checks(), - UpgradeCheckSelect::All + UpgradeCheckSelect::None )); } @@ -218,6 +236,12 @@ mod selector_tests { .kind(), ErrorKind::ArgumentConflict, ); + assert_eq!( + parse(&["--sbm-checks", "--mbm-checks", "--disable-mbm-checks"]) + .unwrap_err() + .kind(), + ErrorKind::ArgumentConflict, + ); } } From 67bdfed5345d7232871d7e6ba9457b3fd7cdce2f Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sun, 15 Nov 2026 23:12:53 +0100 Subject: [PATCH 4/4] deslop Signed-off-by: Oliver Tale-Yazdi --- README.md | 20 +-- cli/main.rs | 20 +-- cli/tests/on_runtime_upgrade.rs | 20 +-- core/src/commands/on_runtime_upgrade/mod.rs | 164 +++++++------------- 4 files changed, 78 insertions(+), 146 deletions(-) diff --git a/README.md b/README.md index 42ecc6ac329..c9b6b6276e3 100644 --- a/README.md +++ b/README.md @@ -136,21 +136,11 @@ after the migration. [`OnRuntimeUpgrade::pre_upgrade`] returns a [`Vec`] tha arbitrary encoded data (usually some pre-upgrade state) which will be passed to [`OnRuntimeUpgrade::pre_upgrade`] after upgrading and used for post checking. -Use either `--checks` or the per-phase `--sbm-checks` and `--mbm-checks`; mixing -them is rejected. All accept `none`, `pre-and-post`, `try-state`, or `all`; a flag -without a value selects `all`. - -`--checks` retains its original behavior (default: `pre-and-post`): checks run -after MBMs, or around SBMs when `--disable-mbm-checks` is set. - -`--sbm-checks` selects the checks around SBMs on the original state. `--mbm-checks` -selects the checks of the upgrade run after MBMs finish; its pre/post checks run the -SBM hooks again on the migrated state. Both are required, except that -`--disable-mbm-checks` skips MBMs and takes `--sbm-checks` alone. -Recommended: `--sbm-checks=pre-and-post --mbm-checks=try-state`. -Only enable SBM `try-state` checks if invariants hold before MBMs finish. -MBMs' own pre/post hooks run inside the runtime independently of these selectors. -`--mbm-checks=none` still executes MBMs. +`--checks` (default `pre-and-post`) selects the checks run after multi-block migrations (MBMs), +or around single-block migrations (SBMs) with `--disable-mbm-checks`. To select both phases +separately, use `--sbm-checks` and `--mbm-checks` instead, e.g. +`--sbm-checks=pre-and-post --mbm-checks=try-state`. `--mbm-checks=pre-and-post` runs the SBM +hooks again on the migrated state. The MBMs' own pre/post hooks always run. ### [`VersionedMigration`] diff --git a/cli/main.rs b/cli/main.rs index 07f67f377b0..d3b06672063 100644 --- a/cli/main.rs +++ b/cli/main.rs @@ -152,21 +152,11 @@ //! arbitrary encoded data (usually some pre-upgrade state) which will be passed to //! [`OnRuntimeUpgrade::pre_upgrade`] after upgrading and used for post checking. //! -//! Use either `--checks` or the per-phase `--sbm-checks` and `--mbm-checks`; mixing -//! them is rejected. All accept `none`, `pre-and-post`, `try-state`, or `all`; a flag -//! without a value selects `all`. -//! -//! `--checks` retains its original behavior (default: `pre-and-post`): checks run -//! after MBMs, or around SBMs when `--disable-mbm-checks` is set. -//! -//! `--sbm-checks` selects the checks around SBMs on the original state. `--mbm-checks` -//! selects the checks of the upgrade run after MBMs finish; its pre/post checks run the -//! SBM hooks again on the migrated state. Both are required, except that -//! `--disable-mbm-checks` skips MBMs and takes `--sbm-checks` alone. -//! Recommended: `--sbm-checks=pre-and-post --mbm-checks=try-state`. -//! Only enable SBM `try-state` checks if invariants hold before MBMs finish. -//! MBMs' own pre/post hooks run inside the runtime independently of these selectors. -//! `--mbm-checks=none` still executes MBMs. +//! `--checks` (default `pre-and-post`) selects the checks run after multi-block migrations (MBMs), +//! or around single-block migrations (SBMs) with `--disable-mbm-checks`. To select both phases +//! separately, use `--sbm-checks` and `--mbm-checks` instead, e.g. +//! `--sbm-checks=pre-and-post --mbm-checks=try-state`. `--mbm-checks=pre-and-post` runs the SBM +//! hooks again on the migrated state. The MBMs' own pre/post hooks always run. //! //! ### [`VersionedMigration`] //! diff --git a/cli/tests/on_runtime_upgrade.rs b/cli/tests/on_runtime_upgrade.rs index c58816257c7..0640de3e140 100644 --- a/cli/tests/on_runtime_upgrade.rs +++ b/cli/tests/on_runtime_upgrade.rs @@ -153,7 +153,7 @@ mod on_runtime_upgrade { async fn pre_upgrade_fail_pre_and_postfails() { run_test( TestConfig::new("rococo-people", "people_rococo_runtime_pre_upgrade_fail") - .with_sub_command_args(&["--sbm-checks=pre-and-post", "--mbm-checks=none"]), + .with_sub_command_args(&["--checks=pre-and-post"]), false, ) .await; @@ -163,18 +163,18 @@ mod on_runtime_upgrade { async fn pre_upgrade_fail_can_be_ignored() { run_test( TestConfig::new("rococo-people", "people_rococo_runtime_pre_upgrade_fail") - .with_sub_command_args(&["--sbm-checks=none", "--mbm-checks=none"]), + .with_sub_command_args(&["--checks=none"]), true, ) .await; } #[tokio::test] - async fn pre_upgrade_fail_legacy_checks_can_be_ignored() { + async fn pre_upgrade_fail_sbm_checks_fails() { run_test( TestConfig::new("rococo-people", "people_rococo_runtime_pre_upgrade_fail") - .with_sub_command_args(&["--checks=none"]), - true, + .with_sub_command_args(&["--sbm-checks=pre-and-post", "--mbm-checks=none"]), + false, ) .await; } @@ -190,11 +190,11 @@ mod on_runtime_upgrade { } #[tokio::test] - async fn pre_upgrade_fail_sbm_checks_without_mbms_fails() { + async fn pre_upgrade_fail_sbm_and_mbm_checks_can_be_ignored() { run_test( TestConfig::new("rococo-people", "people_rococo_runtime_pre_upgrade_fail") - .with_sub_command_args(&["--sbm-checks=pre-and-post", "--disable-mbm-checks"]), - false, + .with_sub_command_args(&["--sbm-checks=none", "--mbm-checks=none"]), + true, ) .await; } @@ -212,7 +212,7 @@ mod on_runtime_upgrade { async fn post_upgrade_fail_pre_and_postfails() { run_test( TestConfig::new("rococo-people", "people_rococo_runtime_post_upgrade_fail") - .with_sub_command_args(&["--sbm-checks=pre-and-post", "--mbm-checks=none"]), + .with_sub_command_args(&["--checks=pre-and-post"]), false, ) .await; @@ -222,7 +222,7 @@ mod on_runtime_upgrade { async fn post_upgrade_fail_can_be_ignored() { run_test( TestConfig::new("rococo-people", "people_rococo_runtime_post_upgrade_fail") - .with_sub_command_args(&["--sbm-checks=none", "--mbm-checks=none"]), + .with_sub_command_args(&["--checks=none"]), true, ) .await; diff --git a/core/src/commands/on_runtime_upgrade/mod.rs b/core/src/commands/on_runtime_upgrade/mod.rs index fb55936d50a..6177490dd44 100644 --- a/core/src/commands/on_runtime_upgrade/mod.rs +++ b/core/src/commands/on_runtime_upgrade/mod.rs @@ -60,33 +60,25 @@ pub struct Command { /// present). /// - `try-state`: Perform the try-state checks. /// - /// Checks run after MBMs, or around SBMs with --disable-mbm-checks. - /// Cannot be combined with --sbm-checks or --mbm-checks. + /// Runs after MBMs, or around SBMs with --disable-mbm-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 = "pre-and-post", - default_missing_value = "all", - num_args = 0..=1, - conflicts_with_all = ["sbm_checks", "mbm_checks"], - verbatim_doc_comment + default_value = "pre-and-post", + default_missing_value = "all", + num_args = 0..=1, + conflicts_with_all = ["sbm_checks", "mbm_checks"], + verbatim_doc_comment )] pub checks: UpgradeCheckSelect, - /// Checks around single-block migrations on the original state. - /// - /// Accepts the same values as --checks. Bare --sbm-checks selects all. - /// Requires --mbm-checks unless --disable-mbm-checks is set. - /// Only select try-state or all if invariants hold before MBMs finish. + /// Checks around single-block migrations, like --checks. Requires --mbm-checks unless + /// --disable-mbm-checks is set. #[clap(long, default_missing_value = "all", num_args = 0..=1, requires = "mbm_checks")] pub sbm_checks: Option, - /// Checks on the migrated state after multi-block migrations finish. - /// - /// Accepts the same values as --checks. Bare --mbm-checks selects all. - /// These are passed to TryRuntime_on_runtime_upgrade after block execution; - /// they do not control the MBMs' own pre/post hooks inside the runtime. - /// Selecting none still executes MBMs. Use --disable-mbm-checks to skip them. - /// Requires --sbm-checks; conflicts with --disable-mbm-checks. + /// Checks after multi-block migrations finish, like --checks. Does not affect the MBMs' own + /// pre/post hooks. Requires --sbm-checks. #[clap(long, default_missing_value = "all", num_args = 0..=1, requires = "sbm_checks", conflicts_with = "disable_mbm_checks")] pub mbm_checks: Option, @@ -127,13 +119,11 @@ pub struct Command { impl Command { fn single_block_checks(&self) -> UpgradeCheckSelect { - self.sbm_checks.unwrap_or_else(|| { - if self.disable_mbm_checks { - self.checks - } else { - UpgradeCheckSelect::None - } - }) + match self.sbm_checks { + Some(checks) => checks, + None if self.disable_mbm_checks => self.checks, + None => UpgradeCheckSelect::None, + } } fn multi_block_checks(&self) -> UpgradeCheckSelect { @@ -146,8 +136,9 @@ impl Command { } #[cfg(test)] -mod selector_tests { +mod tests { use clap::{error::ErrorKind, Parser}; + use UpgradeCheckSelect::{All, PreAndPost, TryState}; use super::*; @@ -161,87 +152,46 @@ mod selector_tests { } #[test] - fn legacy_selection_preserves_scheduling() { - let command = parse(&[]).unwrap(); - assert!(matches!( - command.single_block_checks(), - UpgradeCheckSelect::None - )); - assert!(matches!( - command.multi_block_checks(), - UpgradeCheckSelect::PreAndPost - )); - let command = parse(&["--checks"]).unwrap(); - assert!(matches!( - command.single_block_checks(), - UpgradeCheckSelect::None - )); - assert!(matches!( - command.multi_block_checks(), - UpgradeCheckSelect::All - )); - let command = parse(&["--checks=all", "--disable-mbm-checks"]).unwrap(); - assert!(matches!( - command.single_block_checks(), - UpgradeCheckSelect::All - )); - } - - #[test] - fn paired_selection_is_independent() { - let command = parse(&["--sbm-checks=pre-and-post", "--mbm-checks=all"]).unwrap(); - assert!(matches!( - command.single_block_checks(), - UpgradeCheckSelect::PreAndPost - )); - assert!(matches!( - command.multi_block_checks(), - UpgradeCheckSelect::All - )); - let command = parse(&["--sbm-checks=none", "--mbm-checks=try-state"]).unwrap(); - assert!(matches!( - command.single_block_checks(), - UpgradeCheckSelect::None - )); - assert!(matches!( - command.multi_block_checks(), - UpgradeCheckSelect::TryState - )); - let command = parse(&["--sbm-checks", "--disable-mbm-checks"]).unwrap(); - assert!(matches!( - command.single_block_checks(), - UpgradeCheckSelect::All - )); - assert!(matches!( - command.multi_block_checks(), - UpgradeCheckSelect::None - )); + fn check_selection() { + for (args, sbm, mbm) in [ + (&[][..], UpgradeCheckSelect::None, PreAndPost), + (&["--checks"], UpgradeCheckSelect::None, All), + (&["--checks=all", "--disable-mbm-checks"], All, All), + ( + &["--sbm-checks=pre-and-post", "--mbm-checks=try-state"], + PreAndPost, + TryState, + ), + ( + &["--sbm-checks", "--disable-mbm-checks"], + All, + UpgradeCheckSelect::None, + ), + ] { + let command = parse(args).unwrap(); + assert_eq!( + (command.single_block_checks(), command.multi_block_checks()), + (sbm, mbm) + ); + } } #[test] - fn rejects_incomplete_or_mixed_selection() { - for flag in ["--sbm-checks=all", "--mbm-checks=all"] { - assert_eq!( - parse(&[flag]).unwrap_err().kind(), - ErrorKind::MissingRequiredArgument - ); - assert_eq!( - parse(&["--checks=all", flag]).unwrap_err().kind(), + fn invalid_check_selection() { + for (args, kind) in [ + (&["--sbm-checks"][..], ErrorKind::MissingRequiredArgument), + (&["--mbm-checks"], ErrorKind::MissingRequiredArgument), + ( + &["--checks", "--sbm-checks", "--mbm-checks"], ErrorKind::ArgumentConflict, - ); + ), + ( + &["--sbm-checks", "--mbm-checks", "--disable-mbm-checks"], + ErrorKind::ArgumentConflict, + ), + ] { + assert_eq!(parse(args).unwrap_err().kind(), kind, "{args:?}"); } - assert_eq!( - parse(&["--checks=all", "--sbm-checks=all", "--mbm-checks=all"]) - .unwrap_err() - .kind(), - ErrorKind::ArgumentConflict, - ); - assert_eq!( - parse(&["--sbm-checks", "--mbm-checks", "--disable-mbm-checks"]) - .unwrap_err() - .kind(), - ErrorKind::ArgumentConflict, - ); } } @@ -277,12 +227,14 @@ where .to_ext::(shared, &executor, None, runtime_checks) .await?; + let sync_checks = command.single_block_checks(); + // Run `TryRuntime_on_runtime_upgrade` with the given checks. basti_log( Level::Info, format!( "🔬 Running TryRuntime_on_runtime_upgrade with checks: {:?}", - command.single_block_checks() + sync_checks ) .as_str(), ); @@ -294,7 +246,7 @@ where &mut overlayed_changes, &executor, "TryRuntime_on_runtime_upgrade", - command.single_block_checks().encode().as_ref(), + sync_checks.encode().as_ref(), Default::default(), // we don't really need any extensions here. shared.export_proof.clone(), )?; @@ -322,7 +274,7 @@ where if !self.command.disable_idempotency_checks { basti_log( Level::Info, - "🔬 Running TryRuntime_on_runtime_upgrade again to check idempotency without optional checks.", + "🔬 Running TryRuntime_on_runtime_upgrade again to check idempotency", ); let executor = build_executor(&self.shared);