diff --git a/Cargo.lock b/Cargo.lock index 2a4d02efc31..cfd788c4a6c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11262,7 +11262,7 @@ checksum = "3528ecfd12c466c6f163363caf2d02a71161dd5e1cc6ae7b34207ea2d42d81ed" [[package]] name = "try-runtime-cli" -version = "0.3.5" +version = "0.4.0" dependencies = [ "clap", "env_logger", @@ -11314,7 +11314,7 @@ dependencies = [ [[package]] name = "try-runtime-core" -version = "0.3.5" +version = "0.4.0" dependencies = [ "assert_cmd", "async-trait", diff --git a/Cargo.toml b/Cargo.toml index 70d1f1bc9c1..772eccdd395 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -7,7 +7,7 @@ members = [ ] [workspace.package] -version = "0.3.5" +version = "0.4.0" authors = ["Parity Technologies "] description = "Substrate's programmatic testing framework." edition = "2021" diff --git a/core/src/commands/execute_block.rs b/core/src/commands/execute_block.rs index d9dc17184b4..987a6260baa 100644 --- a/core/src/commands/execute_block.rs +++ b/core/src/commands/execute_block.rs @@ -141,6 +141,7 @@ where let _ = state_machine_call_with_proof::( &ext, + &mut Default::default(), &executor, "TryRuntime_execute_block", &payload, diff --git a/core/src/commands/fast_forward.rs b/core/src/commands/fast_forward.rs index 3140e892f64..a5e1e9ad390 100644 --- a/core/src/commands/fast_forward.rs +++ b/core/src/commands/fast_forward.rs @@ -247,6 +247,7 @@ where log::info!("Running migrations..."); state_machine_call_with_proof::( &ext, + &mut Default::default(), &executor, "TryRuntime_on_runtime_upgrade", command.try_state.encode().as_ref(), diff --git a/core/src/commands/follow_chain.rs b/core/src/commands/follow_chain.rs index f6ac38e9677..09375ad0156 100644 --- a/core/src/commands/follow_chain.rs +++ b/core/src/commands/follow_chain.rs @@ -158,8 +158,10 @@ where .as_mut() .expect("state_ext either existed or was just created"); + let mut overlayed_changes = Default::default(); let result = state_machine_call_with_proof::( state_ext, + &mut overlayed_changes, &executor, "TryRuntime_execute_block", ( @@ -187,9 +189,7 @@ where continue; } - let (mut changes, _, _) = result.expect("checked to be Ok; qed"); - - let storage_changes = changes + let storage_changes = overlayed_changes .drain_storage_changes( &state_ext.backend, // Note that in case a block contains a runtime upgrade, state version could diff --git a/core/src/commands/on_runtime_upgrade.rs b/core/src/commands/on_runtime_upgrade.rs index 5787745858c..d88ed677eba 100644 --- a/core/src/commands/on_runtime_upgrade.rs +++ b/core/src/commands/on_runtime_upgrade.rs @@ -57,11 +57,13 @@ pub struct Command { )] pub checks: UpgradeCheckSelect, - /// Whether to assume that the runtime is a relay chain runtime. - /// - /// This is used to adjust the behavior of weight measurement warnings. + /// Whether to disable weight warnings, useful if the runtime is for a relay chain. #[clap(long, default_value = "false", default_missing_value = "true")] pub no_weight_warnings: bool, + + /// Whether to disable migration idempotency checks + #[clap(long, default_value = "false", default_missing_value = "true")] + pub no_idempotency_checks: bool, } // Runs the `on-runtime-upgrade` command. @@ -99,15 +101,18 @@ where "🔬 Running TryRuntime_on_runtime_upgrade with checks: {:?}", command.checks ); - let (_, proof, encoded_result) = state_machine_call_with_proof::( + // Save the overlayed changes from the first run, so we can use them later for idempotency + // checks. + let mut overlayed_changes = Default::default(); + let (proof, encoded_result) = state_machine_call_with_proof::( &ext, + &mut overlayed_changes, &executor, "TryRuntime_on_runtime_upgrade", command.checks.encode().as_ref(), Default::default(), // we don't really need any extensions here. shared.export_proof.clone(), )?; - let ref_time_results = encoded_result.try_into()?; // If the above call ran with checks then we need to run the call again without checks to @@ -120,43 +125,93 @@ where log::info!( "🔬 TryRuntime_on_runtime_upgrade succeeded! Running it again without checks for weight measurements." ); - let (_, proof, encoded_result) = state_machine_call_with_proof::( + let (proof, encoded_result) = state_machine_call_with_proof::( &ext, + &mut Default::default(), &executor, "TryRuntime_on_runtime_upgrade", UpgradeCheckSelect::None.encode().as_ref(), Default::default(), // we don't really need any extensions here. - shared.export_proof, + shared.export_proof.clone(), )?; let ref_time_results = encoded_result.try_into()?; (proof, ref_time_results) } }; + // Check idempotency + let idempotency_ok = match command.no_idempotency_checks { + true => { + log::info!("ℹ Skipping idempotency check"); + true + } + false => { + log::info!( + "🔬 Running TryRuntime_on_runtime_upgrade again to check idempotency: {:?}", + command.checks + ); + let (oc_pre_root, _) = overlayed_changes.storage_root(&ext.backend, ext.state_version); + match state_machine_call_with_proof::( + &ext, + &mut overlayed_changes, + &executor, + "TryRuntime_on_runtime_upgrade", + command.checks.encode().as_ref(), + Default::default(), // we don't really need any extensions here. + shared.export_proof.clone(), + ) { + Ok(_) => { + // Execution was OK, check if the storage root changed. + let (oc_post_root, _) = + overlayed_changes.storage_root(&ext.backend, ext.state_version); + if oc_pre_root != oc_post_root { + log::error!("❌ Migrations are not idempotent. Selectively remove migrations from Executive until you find the culprit."); + false + } else { + // Exeuction was OK and state root didn't change + true + } + } + Err(e) => { + log::error!( + "❌ Migrations are not idempotent, they failed during the second execution.", + ); + log::debug!("{:?}", e); + false + } + } + } + }; + + // Check weight safety let pre_root = ext.backend.root(); let pov_safety = analyse_pov::>(proof, *pre_root, command.no_weight_warnings); let ref_time_safety = analyse_ref_time(ref_time_results, command.no_weight_warnings); - - match (pov_safety, ref_time_safety, command.no_weight_warnings) { + let weight_ok = match (pov_safety, ref_time_safety, command.no_weight_warnings) { (_, _, true) => { - log::info!("✅ TryRuntime_on_runtime_upgrade executed without errors") + log::info!("ℹ Skipped checking weight safety"); + true } (WeightSafety::ProbablySafe, WeightSafety::ProbablySafe, _) => { log::info!( target: LOG_TARGET, - "✅ TryRuntime_on_runtime_upgrade executed without errors or weight safety \ - warnings. Please note this does not guarantee a successful runtime upgrade. \ + "✅ No weight safety issues detected. \ + Please note this does not guarantee a successful runtime upgrade. \ Always test your runtime upgrade with recent state, and ensure that the weight usage \ of your migrations will not drastically differ between testing and actual on-chain \ execution." ); + true } _ => { - log::warn!(target: LOG_TARGET, "⚠️ TryRuntime_on_runtime_upgrade executed \ - successfully but with weight safety warnings."); - // Exit with a non-zero exit code to indicate that the runtime upgrade may not be safe. - std::process::exit(1); + log::error!(target: LOG_TARGET, "❌ Weight safety issues detected."); + false } + }; + + if !weight_ok || !idempotency_ok { + log::error!("❌ Issues detected, exiting non-zero. See logs."); + std::process::exit(1); } Ok(()) diff --git a/core/src/lib.rs b/core/src/lib.rs index d1ba19735c3..06e7db0816b 100644 --- a/core/src/lib.rs +++ b/core/src/lib.rs @@ -152,23 +152,22 @@ impl TryFrom> for RefTimeInfo { /// Make sure [`LOG_TARGET`] is enabled in logging. pub(crate) fn state_machine_call_with_proof( ext: &TestExternalities>, + storage_overlay: &mut OverlayedChanges>, executor: &WasmExecutor, method: &'static str, data: &[u8], mut extensions: Extensions, maybe_export_proof: Option, -) -> sc_cli::Result<(OverlayedChanges>, StorageProof, Vec)> { - let mut changes = Default::default(); - let backend = ext.backend.clone(); - let runtime_code_backend = sp_state_machine::backend::BackendRuntimeCode::new(&backend); - let proving_backend = TrieBackendBuilder::wrap(&backend) +) -> sc_cli::Result<(StorageProof, Vec)> { + let runtime_code_backend = sp_state_machine::backend::BackendRuntimeCode::new(&ext.backend); + let proving_backend = TrieBackendBuilder::wrap(&ext.backend) .with_recorder(Default::default()) .build(); let runtime_code = runtime_code_backend.runtime_code()?; let encoded_result = StateMachine::new( &proving_backend, - &mut changes, + storage_overlay, executor, method, data, @@ -210,7 +209,7 @@ pub(crate) fn state_machine_call_with_proof tokio::process::Child { + Command::new(cargo_bin("try-runtime")) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .arg(format!("--runtime={}", runtime_path)) + .arg("on-runtime-upgrade") + .args(extra_args) + .args(["snap", format!("--path={}", snap_path).as_str()]) + .kill_on_drop(true) + .spawn() + .unwrap() + } + + #[tokio::test] + async fn ok_works() { + common::run_with_timeout(Duration::from_secs(300), async move { + let project_root = env!("CARGO_MANIFEST_DIR"); + let snap_path = format!("{}/tests/snaps/rococo-bridge-hub.snap", project_root); + let runtime_path = format!( + "{}/tests/runtimes/bridge_hub_rococo_runtime_ok.compact.compressed.wasm", + project_root + ); + dbg!(&runtime_path, &snap_path); + let child = on_runtime_upgrade(snap_path.as_str(), runtime_path.as_str(), &[]); + let out = child.wait_with_output().await.unwrap(); + dbg!(&out); + assert!(out.status.success()); + }) + .await; + } + + #[tokio::test] + async fn weight_issue_fails() { + common::run_with_timeout(Duration::from_secs(300), async move { + let project_root = env!("CARGO_MANIFEST_DIR"); + let snap_path = format!("{}/tests/snaps/rococo-bridge-hub.snap", project_root); + let runtime_path = format!( + "{}/tests/runtimes/bridge_hub_rococo_runtime_WEIGHT_ISSUE.compact.compressed.wasm", + project_root + ); + let child = on_runtime_upgrade(snap_path.as_str(), runtime_path.as_str(), &[]); + let out = child.wait_with_output().await.unwrap(); + assert!(!out.status.success()); + }) + .await; + } + + #[tokio::test] + async fn weight_issue_can_be_ignored() { + common::run_with_timeout(Duration::from_secs(300), async move { + let project_root = env!("CARGO_MANIFEST_DIR"); + let snap_path = format!("{}/tests/snaps/rococo-bridge-hub.snap", project_root); + let runtime_path = format!( + "{}/tests/runtimes/bridge_hub_rococo_runtime_WEIGHT_ISSUE.compact.compressed.wasm", + project_root + ); + + let child = on_runtime_upgrade( + snap_path.as_str(), + runtime_path.as_str(), + &["--no-weight-warnings"], + ); + let out = child.wait_with_output().await.unwrap(); + assert!(out.status.success()); + }) + .await; + } + + #[tokio::test] + async fn not_idempotent_execution_fails() { + common::run_with_timeout(Duration::from_secs(300), async move { + let project_root = env!("CARGO_MANIFEST_DIR"); + let snap_path = format!("{}/tests/snaps/rococo-bridge-hub.snap", project_root); + let runtime_path = format!( + "{}/tests/runtimes/bridge_hub_rococo_runtime_not_idempotent_execution.compact.compressed.wasm", + project_root + ); + + let child = on_runtime_upgrade(snap_path.as_str(), runtime_path.as_str(), &[]); + let out = child.wait_with_output().await.unwrap(); + assert!(!out.status.success()); + }) + .await; + } + + #[tokio::test] + async fn not_idempotent_execution_issue_can_be_ignored() { + common::run_with_timeout(Duration::from_secs(300), async move { + let project_root = env!("CARGO_MANIFEST_DIR"); + let snap_path = format!("{}/tests/snaps/rococo-bridge-hub.snap", project_root); + let runtime_path = format!( + "{}/tests/runtimes/bridge_hub_rococo_runtime_not_idempotent_execution.compact.compressed.wasm", + project_root + ); + + let child = on_runtime_upgrade( + snap_path.as_str(), + runtime_path.as_str(), + &["--no-idempotency-checks"], + ); + let out = child.wait_with_output().await.unwrap(); + assert!(out.status.success()); + }) + .await; + } + + #[tokio::test] + async fn not_idempotent_state_root_fails() { + common::run_with_timeout(Duration::from_secs(300), async move { + let project_root = env!("CARGO_MANIFEST_DIR"); + let snap_path = format!("{}/tests/snaps/rococo-bridge-hub.snap", project_root); + let runtime_path = format!( + "{}/tests/runtimes/bridge_hub_rococo_runtime_not_idempotent_state_root.compact.compressed.wasm", + project_root + ); + + let child = on_runtime_upgrade(snap_path.as_str(), runtime_path.as_str(), &[]); + let out = child.wait_with_output().await.unwrap(); + assert!(!out.status.success()); + }) + .await; + } + + #[tokio::test] + async fn not_idempotent_state_root_issue_can_be_ignored() { + common::run_with_timeout(Duration::from_secs(300), async move { + let project_root = env!("CARGO_MANIFEST_DIR"); + let snap_path = format!("{}/tests/snaps/rococo-bridge-hub.snap", project_root); + let runtime_path = format!( + "{}/tests/runtimes/bridge_hub_rococo_runtime_not_idempotent_state_root.compact.compressed.wasm", + project_root + ); + let child = on_runtime_upgrade( + snap_path.as_str(), + runtime_path.as_str(), + &["--no-idempotency-checks"], + ); + let out = child.wait_with_output().await.unwrap(); + assert!(out.status.success()); + }) + .await; + } +} diff --git a/core/tests/readme.md b/core/tests/readme.md new file mode 100644 index 00000000000..3377d0ae2a1 --- /dev/null +++ b/core/tests/readme.md @@ -0,0 +1,11 @@ + +# tests + +## ./runtimes and ./snaps + +A state snapshot is included in ./snaps, and some runtimes in ./runtimes for use in tests. + +- `bridge_hub_rococo_runtime_OK.compact.compressed.wasm` a runtime with correctly configured migrations +- `bridge_hub_rococo_runtime_WEIGHT_ISSUE.compact.compressed.wasm` a runtime with migrations that would exceed sensible values for a parachain +- `bridge_hub_rococo_runtime_NOT_IDEMPOTENT_EXECUTION.compact.compressed.wasm` a runtime where `try_on_runtime_upgrade` if migrations are executed for a second time +- `bridge_hub_rococo_runtime_NOT_IDEMPOTENT_STATE_ROOT.compact.compressed.wasm` a runtime which will succeed when migrations are executed for a second time, but the state changes are not idempotent diff --git a/core/tests/runtimes/bridge_hub_rococo_runtime_not_idempotent_execution.compact.compressed.wasm b/core/tests/runtimes/bridge_hub_rococo_runtime_not_idempotent_execution.compact.compressed.wasm new file mode 100644 index 00000000000..835cccc0701 Binary files /dev/null and b/core/tests/runtimes/bridge_hub_rococo_runtime_not_idempotent_execution.compact.compressed.wasm differ diff --git a/core/tests/runtimes/bridge_hub_rococo_runtime_not_idempotent_state_root.compact.compressed.wasm b/core/tests/runtimes/bridge_hub_rococo_runtime_not_idempotent_state_root.compact.compressed.wasm new file mode 100644 index 00000000000..c965be9b033 Binary files /dev/null and b/core/tests/runtimes/bridge_hub_rococo_runtime_not_idempotent_state_root.compact.compressed.wasm differ diff --git a/core/tests/runtimes/bridge_hub_rococo_runtime_ok.compact.compressed.wasm b/core/tests/runtimes/bridge_hub_rococo_runtime_ok.compact.compressed.wasm new file mode 100644 index 00000000000..4ff6c81fdb0 Binary files /dev/null and b/core/tests/runtimes/bridge_hub_rococo_runtime_ok.compact.compressed.wasm differ diff --git a/core/tests/runtimes/bridge_hub_rococo_runtime_weight_issue.compact.compressed.wasm b/core/tests/runtimes/bridge_hub_rococo_runtime_weight_issue.compact.compressed.wasm new file mode 100644 index 00000000000..fc03a10bb52 Binary files /dev/null and b/core/tests/runtimes/bridge_hub_rococo_runtime_weight_issue.compact.compressed.wasm differ diff --git a/core/tests/snaps/rococo-bridge-hub.snap b/core/tests/snaps/rococo-bridge-hub.snap new file mode 100644 index 00000000000..bb49d8f068b Binary files /dev/null and b/core/tests/snaps/rococo-bridge-hub.snap differ