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

<img width="1185" alt="Screenshot 2023-10-25 at 15 44 44"
src="https://github.com/paritytech/polkadot-sdk/assets/16665596/b75588a2-1c64-45df-bbc8-bcb8bf8b0fe0">

### 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

<img width="1185" alt="Screenshot 2023-10-25 at 15 39 01"
src="https://github.com/paritytech/polkadot-sdk/assets/16665596/e3429cb1-489e-430a-9716-77c052e5dae6">
 

## 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

@@ -349,23 +349,12 @@ where
 		Ok(frame_system::Pallet::<System>::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
+	/// The `checks` param determines whether to execute `pre/post_upgrade` and `try_state` hooks.
-	/// `true`. Also, if set to `true`, it runs the `pre_upgrade` and `post_upgrade` hooks.
 	pub fn try_runtime_upgrade(
 		checks: frame_try_runtime::UpgradeCheckSelect,
 	) -> Result<Weight, TryRuntimeError> {
-		if checks.try_state() {
-			let _guard = frame_support::StorageNoopGuard::default();
-			<AllPalletsWithSystem as frame_support::traits::TryState<
-				BlockNumberFor<System>,
-			>>::try_state(
-				frame_system::Pallet::<System>::block_number(),
-				frame_try_runtime::TryStateSelect::All,
-			)?;
-		}
-
 		let weight =
 			<(COnRuntimeUpgrade, AllPalletsWithSystem) as OnRuntimeUpgrade>::try_on_runtime_upgrade(
 				checks.pre_and_post(),

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 = <
-			<T as #frame_system::Config>::PalletInfo
-			as
-			#frame_support::traits::PalletInfo
-		>::name::<Self>().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::<T>,
 				_s: #frame_support::traits::TryStateSelect
 			) -> Result<(), #frame_support::sp_runtime::TryRuntimeError> {
-				#log_try_state
+				let pallet_name = <
+					<T as #frame_system::Config>::PalletInfo
+					as
+					#frame_support::traits::PalletInfo
+				>::name::<Self>().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::<T>
 					>
-				>::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
+				})
 			}
 		}
 	)

substrate/frame/support/src/traits/try_runtime.rs

@@ -144,9 +144,27 @@ impl<BlockNumber: Clone + sp_std::fmt::Debug + AtLeast32BitUnsigned> TryState<Bl
 		match targets {
 			Select::None => Ok(()),
 			Select::All => {
-				let mut result = Ok(());
+				let mut error_count = 0;
-				for_tuples!( #( result = result.and(Tuple::try_state(n.clone(), targets.clone())); )* );
+				for_tuples!(#(
-				result
+					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>] =