From 6819591d84438b94fa58e077d0bceb4e2eaeff6d Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Mon, 30 Oct 2023 19:43:32 +1100 Subject: [PATCH] Improve `try-state` developer experience & fix bug (#2019) Making some devex improvements as I audit our chains adherence to try-state invariants, in preparation for automated try-state checks and alerting. Note to reviewer: while you're here, if you have time would be great to get your eyes on /~https://github.com/paritytech/polkadot-sdk/pull/1297 also since it touches a similar file and I'd like to avoid merge conflicts :P ## Devex Improvements - Changes the log level of logs informing the user that try-state checks are being run for a pallet from debug to info - Improves how errors are communicated - Errors are logged when they are encountered, rather than after everything has been executed - Exact pallet the error originated from is included with the error log - Clearly see all errors and how many there are, rather than only one - Closes #136 ### Example of new logs Screenshot 2023-10-25 at 15 44 44 ### Same but with old logs (run with RUST_LOG=debug) Notice only informed of one of the errors, and it's unclear which pallet it originated Screenshot 2023-10-25 at 15 39 01 ## Bug fix When dry-running migrations and `checks.try_state()` is `true`, only run `try_state` checks after migrations have been executed. Otherwise, `try_state` checks that expect state to be in at a HIGHER storage version than is on-chain could incorrectly fail. --------- Co-authored-by: command-bot <> --- substrate/frame/executive/src/lib.rs | 15 ++------ .../procedural/src/pallet/expand/hooks.rs | 36 +++++++++++-------- .../frame/support/src/traits/try_runtime.rs | 24 +++++++++++-- 3 files changed, 44 insertions(+), 31 deletions(-) diff --git a/substrate/frame/executive/src/lib.rs b/substrate/frame/executive/src/lib.rs index a4b12c6d31dbd..1ca9629fd4208 100644 --- a/substrate/frame/executive/src/lib.rs +++ b/substrate/frame/executive/src/lib.rs @@ -349,23 +349,12 @@ where Ok(frame_system::Pallet::::block_weight().total()) } - /// Execute all `OnRuntimeUpgrade` of this runtime, including the pre and post migration checks. + /// Execute all `OnRuntimeUpgrade` of this runtime. /// - /// 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. + /// The `checks` param determines whether to execute `pre/post_upgrade` and `try_state` hooks. 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(), - frame_try_runtime::TryStateSelect::All, - )?; - } - let weight = <(COnRuntimeUpgrade, AllPalletsWithSystem) as OnRuntimeUpgrade>::try_on_runtime_upgrade( checks.pre_and_post(), diff --git a/substrate/frame/support/procedural/src/pallet/expand/hooks.rs b/substrate/frame/support/procedural/src/pallet/expand/hooks.rs index 2825756f270f0..aaad4dd2be0e6 100644 --- a/substrate/frame/support/procedural/src/pallet/expand/hooks.rs +++ b/substrate/frame/support/procedural/src/pallet/expand/hooks.rs @@ -58,19 +58,6 @@ pub fn expand_hooks(def: &mut Def) -> proc_macro2::TokenStream { } }; - let log_try_state = quote::quote! { - let pallet_name = < - ::PalletInfo - as - #frame_support::traits::PalletInfo - >::name::().expect("No name found for the pallet! This usually means that the pallet wasn't added to `construct_runtime!`."); - #frame_support::__private::log::debug!( - target: #frame_support::LOG_TARGET, - "🩺 try-state pallet {:?}", - pallet_name, - ); - }; - let hooks_impl = if def.hooks.is_none() { let frame_system = &def.frame_system; quote::quote! { @@ -271,12 +258,31 @@ pub fn expand_hooks(def: &mut Def) -> proc_macro2::TokenStream { n: #frame_system::pallet_prelude::BlockNumberFor::, _s: #frame_support::traits::TryStateSelect ) -> Result<(), #frame_support::sp_runtime::TryRuntimeError> { - #log_try_state + let pallet_name = < + ::PalletInfo + as + #frame_support::traits::PalletInfo + >::name::().expect("No name found for the pallet! This usually means that the pallet wasn't added to `construct_runtime!`."); + + #frame_support::__private::log::info!( + target: #frame_support::LOG_TARGET, + "🩺 Running {:?} try-state checks", + pallet_name, + ); < Self as #frame_support::traits::Hooks< #frame_system::pallet_prelude::BlockNumberFor:: > - >::try_state(n) + >::try_state(n).map_err(|err| { + #frame_support::__private::log::error!( + target: #frame_support::LOG_TARGET, + "❌ {:?} try_state checks failed: {:?}", + pallet_name, + err + ); + + err + }) } } ) diff --git a/substrate/frame/support/src/traits/try_runtime.rs b/substrate/frame/support/src/traits/try_runtime.rs index 31aebeeb4d99b..e7a1fe109fc22 100644 --- a/substrate/frame/support/src/traits/try_runtime.rs +++ b/substrate/frame/support/src/traits/try_runtime.rs @@ -144,9 +144,27 @@ impl TryState Ok(()), Select::All => { - let mut result = Ok(()); - for_tuples!( #( result = result.and(Tuple::try_state(n.clone(), targets.clone())); )* ); - result + let mut error_count = 0; + for_tuples!(#( + if let Err(_) = Tuple::try_state(n.clone(), targets.clone()) { + error_count += 1; + } + )*); + + if error_count > 0 { + log::error!( + target: "try-runtime", + "{} pallets exited with errors while executing try_state checks.", + error_count + ); + + return Err( + "Detected errors while executing try_state checks. See logs for more info." + .into(), + ) + } + + Ok(()) }, Select::RoundRobin(len) => { let functions: &[fn(BlockNumber, Select) -> Result<(), TryRuntimeError>] =