try-state: should indicate which pallet is failing

[liamaharon]

Description

try_state error handling more or less seems to follow this pattern:

ensure!(
    invariant_holds,
    "some explanation of the invariant not holding"
)

If !invariant_holds, the try_state call will return a DispatchError::Other(message) to Executive, who calls it like this:

		// run the try-state checks of all pallets, ensuring they don't alter any state.
		let _guard = frame_support::StorageNoopGuard::default();
		<AllPalletsWithSystem as frame_support::traits::TryState<
			BlockNumberFor<System>,
		>>::try_state(*header.number(), select)
		.map_err(|e| {
			frame_support::log::error!(target: LOG_TARGET, "failure: {:?}", e);
			e
		})?;
		drop(_guard);

The issue here is that Executive does not log which pallet the error emitted from, which is vital information when there are multiple instances of the same pallet in the runtime.

I encountered this issue when investigating a collective pallet error in polkadot which has two instances. I needed to add additional logging to try_state and replace the ensure! with a conditional to learn which instance the error is coming from:

        fn do_try_state() -> Result<(), TryRuntimeError> {
+               use frame_support::traits::PalletInfo;
+               let pallet_error_log = "try-state invariant failed for pallet ".to_owned() +
+                       T::PalletInfo::name::<Pallet<T, I>>().unwrap_or("unknown_pallet") +
+                       ".";
+
                Self::proposals()
                        .into_iter()
                        .try_for_each(|proposal| -> Result<(), TryRuntimeError> {
@@ -1041,10 +1046,10 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
                        "The member count is greater than `MaxMembers`."
                );

-               ensure!(
-                       Self::members().windows(2).all(|members| members[0] <= members[1]),
-                       "The members are not sorted by value."
-               );
+               if !Self::members().windows(2).all(|members| members[0] <= members[1]) {
+                       log::error!("{}", &pallet_error_log);
+                       return Err("The members are not sorted by value.".into())
+               };		

Solutions?

Replace ensure with conditionals and additional logging

The simplest way to resolve this is likely to simply replace all the ensure usage with conditionals, and an extra log containing the pallet name (optionally also index) like I did when investigating the collectives pallet issue.

This solution kind of sucks, making the code verbose.

Change try_state to return a new error type

I may be wrong but I'm not sure if it even makes sense for try_state to return a DispatchError, because it only ever checks storage and never actually dispatches anything?

Something very roughly like

// If you want P to be something that can be converted into a String or has some relation to String
#[cfg(feature = "try-runtime")]
#[derive(Eq, Clone, Copy, Encode, Debug, PartialEq)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
pub struct TryStateError<P: PalletInfo>(pub P, pub &'static str);

This way, Executive will have access to P as well as the error message and be able to log which pallet it came from.

[liamaharon]

@kianenigma I think you are likely best qualified to provide valuable insights about this :)