TryDecodeEntireState check for storage types and pallets

prdoc/pr_1805.prdoc

@@ -0,0 +1,19 @@
+title: Introduce state decoding check after runtime upgrades.
+
+doc:
+  - audience: Core Dev
+    description: |
+      Adds a check to the try-runtime logic that will verify that all pallet on-chain storage still decodes. This can help to spot missing migrations before they become a problem. The check is enabled as soon as the `--checks` option of the `try-runtime` CLI is not `None`.
+
+migrations:
+  db: []
+
+  runtime: []
+
+crates: 
+  - name: frame-support
+    semver: minor
+  - name: frame-support-procedural
+    semver: minor
+
+host_functions: []

substrate/bin/node-template/runtime/src/lib.rs

@@ -313,6 +313,12 @@ pub type SignedExtra = (
 	pallet_transaction_payment::ChargeTransactionPayment<Runtime>,
 );
 
+/// All migrations of the runtime, aside from the ones declared in the pallets.
+///
+/// This can be a tuple of types, each implementing `OnRuntimeUpgrade`.
+#[allow(unused_parens)]
+type Migrations = ();
+
 /// Unchecked extrinsic type as expected by this runtime.
 pub type UncheckedExtrinsic =
 	generic::UncheckedExtrinsic<Address, RuntimeCall, Signature, SignedExtra>;
@@ -325,6 +331,7 @@ pub type Executive = frame_executive::Executive<
 	frame_system::ChainContext<Runtime>,
 	Runtime,
 	AllPalletsWithSystem,
+	Migrations,
 >;
 
 #[cfg(feature = "runtime-benchmarks")]

substrate/frame/executive/src/lib.rs

@@ -139,9 +139,15 @@ use sp_runtime::{
 use sp_std::{marker::PhantomData, prelude::*};
 
 #[cfg(feature = "try-runtime")]
-use log;
-#[cfg(feature = "try-runtime")]
-use sp_runtime::TryRuntimeError;
+use ::{
+	frame_support::{
+		traits::{TryDecodeEntireStorage, TryDecodeEntireStorageError, TryState},
+		StorageNoopGuard,
+	},
+	frame_try_runtime::{TryStateSelect, UpgradeCheckSelect},
+	log,
+	sp_runtime::TryRuntimeError,
+};
 
 #[allow(dead_code)]
 const LOG_TARGET: &str = "runtime::executive";
@@ -229,7 +235,8 @@ impl<
 			+ OnIdle<BlockNumberFor<System>>
 			+ OnFinalize<BlockNumberFor<System>>
 			+ OffchainWorker<BlockNumberFor<System>>
-			+ frame_support::traits::TryState<BlockNumberFor<System>>,
+			+ TryState<BlockNumberFor<System>>
+			+ TryDecodeEntireStorage,
 		COnRuntimeUpgrade: OnRuntimeUpgrade,
 	> Executive<System, Block, Context, UnsignedValidator, AllPalletsWithSystem, COnRuntimeUpgrade>
 where
@@ -308,11 +315,15 @@ where
 		let _guard = frame_support::StorageNoopGuard::default();
 		<AllPalletsWithSystem as frame_support::traits::TryState<
 			BlockNumberFor<System>,
-		>>::try_state(*header.number(), select)
+		>>::try_state(*header.number(), select.clone())
 		.map_err(|e| {
 			log::error!(target: LOG_TARGET, "failure: {:?}", e);
 			e
 		})?;
+		if select.any() {
+			let res = AllPalletsWithSystem::try_decode_entire_state();
+			Self::log_decode_result(res)?;
+		}
 		drop(_guard);
 
 		// do some of the checks that would normally happen in `final_checks`, but perhaps skip
@@ -352,26 +363,61 @@ where
 	/// Execute all `OnRuntimeUpgrade` of this runtime.
 	///
 	/// 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<Weight, TryRuntimeError> {
+	pub fn try_runtime_upgrade(checks: UpgradeCheckSelect) -> Result<Weight, TryRuntimeError> {
 		let weight =
 			<(COnRuntimeUpgrade, AllPalletsWithSystem) as OnRuntimeUpgrade>::try_on_runtime_upgrade(
 				checks.pre_and_post(),
 			)?;
+		// Nothing should modify the state after the migrations ran:
+		let _guard = StorageNoopGuard::default();
 
+		// The state must be decodable:
+		if checks.any() {
+			let res = AllPalletsWithSystem::try_decode_entire_state();
+			Self::log_decode_result(res)?;
+		}
+
+		// Check all storage invariants:
 		if checks.try_state() {
-			let _guard = frame_support::StorageNoopGuard::default();
-			<AllPalletsWithSystem as frame_support::traits::TryState<
-				BlockNumberFor<System>,
-			>>::try_state(
+			AllPalletsWithSystem::try_state(
 				frame_system::Pallet::<System>::block_number(),
-				frame_try_runtime::TryStateSelect::All,
+				TryStateSelect::All,
 			)?;
 		}
 
 		Ok(weight)
 	}
+
+	/// Logs the result of trying to decode the entire state.
+	fn log_decode_result(
+		res: Result<usize, Vec<TryDecodeEntireStorageError>>,
+	) -> Result<(), TryRuntimeError> {
+		match res {
+			Ok(bytes) => {
+				log::debug!(
+					target: LOG_TARGET,
+					"decoded the entire state ({bytes} bytes)",
+				);
+
+				Ok(())
+			},
+			Err(errors) => {
+				log::error!(
+					target: LOG_TARGET,
+					"`try_decode_entire_state` failed with {} errors",
+					errors.len(),
+				);
+
+				for (i, err) in errors.iter().enumerate() {
+					// We log the short version to `error` and then the full debug info to `debug`:
+					log::error!(target: LOG_TARGET, "- {i}. error: {err}");
+					log::debug!(target: LOG_TARGET, "- {i}. error: {err:?}");
+				}
+
+				Err("`try_decode_entire_state` failed".into())
+			},
+		}
+	}
 }
 
 impl<

substrate/frame/glutton/src/lib.rs

@@ -21,9 +21,8 @@
 //!
 //! # Glutton Pallet
 //!
-//! Pallet that consumes `ref_time` and `proof_size` of a block. Based on the
-//! `Compute` and `Storage` parameters the pallet consumes the adequate amount
-//! of weight.
+//! Pallet that consumes `ref_time` and `proof_size` of a block. Based on the `Compute` and
+//! `Storage` parameters the pallet consumes the adequate amount of weight.
 
 #![deny(missing_docs)]
 #![cfg_attr(not(feature = "std"), no_std)]

substrate/frame/support/Cargo.toml

@@ -13,6 +13,7 @@ readme = "README.md"
 targets = ["x86_64-unknown-linux-gnu"]
 
 [dependencies]
+array-bytes = { version = "6.1", default-features = false }
 serde = { version = "1.0.188", default-features = false, features = ["alloc", "derive"] }
 codec = { package = "parity-scale-codec", version = "3.6.1", default-features = false, features = ["derive", "max-encoded-len"] }
 scale-info = { version = "2.10.0", default-features = false, features = ["derive"] }
@@ -52,7 +53,6 @@ aquamarine = { version = "0.3.2" }
 assert_matches = "1.3.0"
 pretty_assertions = "1.2.1"
 frame-system = { path = "../system" }
-array-bytes = "6.1"
 
 [features]
 default = [ "std" ]

substrate/frame/support/procedural/src/pallet/expand/event.rs

@@ -127,11 +127,12 @@ pub fn expand_event(def: &mut Def) -> proc_macro2::TokenStream {
 		let trait_use_gen = &def.trait_use_generics(event.attr_span);
 		let type_impl_gen = &def.type_impl_generics(event.attr_span);
 		let type_use_gen = &def.type_use_generics(event.attr_span);
+		let pallet_ident = &def.pallet_struct.pallet;
 
 		let PalletEventDepositAttr { fn_vis, fn_span, .. } = deposit_event;
 
 		quote::quote_spanned!(*fn_span =>
-			impl<#type_impl_gen> Pallet<#type_use_gen> #completed_where_clause {
+			impl<#type_impl_gen> #pallet_ident<#type_use_gen> #completed_where_clause {
 				#fn_vis fn deposit_event(event: Event<#event_use_gen>) {
 					let event = <
 						<T as Config #trait_use_gen>::RuntimeEvent as

substrate/frame/support/procedural/src/pallet/expand/storage.rs

@@ -822,12 +822,69 @@ pub fn expand_storages(def: &mut Def) -> proc_macro2::TokenStream {
 		)
 	});
 
+	// aggregated where clause of all storage types and the whole pallet.
 	let mut where_clauses = vec![&def.config.where_clause];
 	where_clauses.extend(def.storages.iter().map(|storage| &storage.where_clause));
 	let completed_where_clause = super::merge_where_clauses(&where_clauses);
 	let type_impl_gen = &def.type_impl_generics(proc_macro2::Span::call_site());
 	let type_use_gen = &def.type_use_generics(proc_macro2::Span::call_site());
 
+	let try_decode_entire_state = {
+		let mut storage_names = def
+			.storages
+			.iter()
+			.filter_map(|storage| {
+				if storage.cfg_attrs.is_empty() {
+					let ident = &storage.ident;
+					let gen = &def.type_use_generics(storage.attr_span);
+					Some(quote::quote_spanned!(storage.attr_span => #ident<#gen> ))
+				} else {
+					None
+				}
+			})
+			.collect::<Vec<_>>();
+		storage_names.sort_by_cached_key(|ident| ident.to_string());
+
+		quote::quote!(
+			#[cfg(feature = "try-runtime")]
+			impl<#type_impl_gen> #frame_support::traits::TryDecodeEntireStorage
+			for #pallet_ident<#type_use_gen> #completed_where_clause
+			{
+				fn try_decode_entire_state() -> Result<usize, #frame_support::__private::sp_std::vec::Vec<#frame_support::traits::TryDecodeEntireStorageError>> {
+					let pallet_name = <<T as #frame_system::Config>::PalletInfo	as frame_support::traits::PalletInfo>
+						::name::<#pallet_ident<#type_use_gen>>()
+						.expect("Every active pallet has a name in the runtime; qed");
+
+					#frame_support::__private::log::debug!(target: "runtime::try-decode-state", "trying to decode pallet: {pallet_name}");
+
+					// NOTE: for now, we have to exclude storage items that are feature gated.
+					let mut errors = #frame_support::__private::sp_std::vec::Vec::new();
+					let mut decoded = 0usize;
+
+					#(
+						#frame_support::__private::log::debug!(target: "runtime::try-decode-state", "trying to decode storage: \
+						{pallet_name}::{}", stringify!(#storage_names));
+
+						match <#storage_names as #frame_support::traits::TryDecodeEntireStorage>::try_decode_entire_state() {
+							Ok(count) => {
+								decoded += count;
+							},
+							Err(err) => {
+								errors.extend(err);
+							},
+						}
+					)*
+
+					if errors.is_empty() {
+						Ok(decoded)
+					} else {
+						Err(errors)
+					}
+				}
+			}
+		)
+	};
+
 	quote::quote!(
 		impl<#type_impl_gen> #pallet_ident<#type_use_gen>
 			#completed_where_clause
@@ -853,5 +910,7 @@ pub fn expand_storages(def: &mut Def) -> proc_macro2::TokenStream {
 		#( #getters )*
 		#( #prefix_structs )*
 		#( #on_empty_structs )*
+
+		#try_decode_entire_state
 	)
 }

substrate/frame/support/procedural/src/pallet/parse/storage.rs

@@ -151,7 +151,7 @@ pub enum QueryKind {
 /// `type MyStorage = StorageValue<MyStorageP, u32>`
 /// The keys and values types are parsed in order to get metadata
 pub struct StorageDef {
-	/// The index of error item in pallet module.
+	/// The index of storage item in pallet module.
 	pub index: usize,
 	/// Visibility of the storage type.
 	pub vis: syn::Visibility,

substrate/frame/support/src/storage/generator/mod.rs

@@ -24,10 +24,10 @@
 //!
 //! This is internal api and is subject to change.
 
-mod double_map;
+pub(crate) mod double_map;
 pub(crate) mod map;
-mod nmap;
-mod value;
+pub(crate) mod nmap;
+pub(crate) mod value;
 
 pub use double_map::StorageDoubleMap;
 pub use map::StorageMap;

substrate/frame/support/src/storage/types/counted_map.rs

@@ -119,7 +119,11 @@ impl<P: CountedStorageMapInstance, H, K, V, Q, O, M> MapWrapper
 	type Map = StorageMap<P, H, K, V, Q, O, M>;
 }
 
-type CounterFor<P> = StorageValue<<P as CountedStorageMapInstance>::CounterPrefix, u32, ValueQuery>;
+/// The numeric counter type.
+pub type Counter = u32;
+
+type CounterFor<P> =
+	StorageValue<<P as CountedStorageMapInstance>::CounterPrefix, Counter, ValueQuery>;
 
 /// On removal logic for updating counter while draining upon some prefix with
 /// [`crate::storage::PrefixIterator`].
@@ -423,14 +427,14 @@ where
 	/// can be very heavy, so use with caution.
 	///
 	/// Returns the number of items in the map which is used to set the counter.
-	pub fn initialize_counter() -> u32 {
-		let count = Self::iter_values().count() as u32;
+	pub fn initialize_counter() -> Counter {
+		let count = Self::iter_values().count() as Counter;
 		CounterFor::<Prefix>::set(count);
 		count
 	}
 
 	/// Return the count.
-	pub fn count() -> u32 {
+	pub fn count() -> Counter {
 		CounterFor::<Prefix>::get()
 	}
 }
@@ -1207,7 +1211,7 @@ mod test {
 				StorageEntryMetadataIR {
 					name: "counter_for_foo",
 					modifier: StorageEntryModifierIR::Default,
-					ty: StorageEntryTypeIR::Plain(scale_info::meta_type::<u32>()),
+					ty: StorageEntryTypeIR::Plain(scale_info::meta_type::<Counter>()),
 					default: vec![0, 0, 0, 0],
 					docs: if cfg!(feature = "no-metadata-docs") {
 						vec![]

substrate/frame/support/src/storage/types/counted_nmap.rs

@@ -114,8 +114,10 @@ impl<P: CountedStorageNMapInstance, K, V, Q, O, M> MapWrapper
 	type Map = StorageNMap<P, K, V, Q, O, M>;
 }
 
+type Counter = super::counted_map::Counter;
+
 type CounterFor<P> =
-	StorageValue<<P as CountedStorageNMapInstance>::CounterPrefix, u32, ValueQuery>;
+	StorageValue<<P as CountedStorageNMapInstance>::CounterPrefix, Counter, ValueQuery>;
 
 /// On removal logic for updating counter while draining upon some prefix with
 /// [`crate::storage::PrefixIterator`].
@@ -472,7 +474,7 @@ where
 	}
 
 	/// Return the count.
-	pub fn count() -> u32 {
+	pub fn count() -> Counter {
 		CounterFor::<Prefix>::get()
 	}
 }

substrate/frame/support/src/storage/types/mod.rs

@@ -30,7 +30,7 @@ mod map;
 mod nmap;
 mod value;
 
-pub use counted_map::{CountedStorageMap, CountedStorageMapInstance};
+pub use counted_map::{CountedStorageMap, CountedStorageMapInstance, Counter};
 pub use counted_nmap::{CountedStorageNMap, CountedStorageNMapInstance};
 pub use double_map::StorageDoubleMap;
 pub use key::{

substrate/frame/support/src/storage/types/value.rs

@@ -23,7 +23,7 @@ use crate::{
 		types::{OptionQuery, QueryKindTrait, StorageEntryMetadataBuilder},
 		StorageAppend, StorageDecodeLength, StorageTryAppend,
 	},
-	traits::{GetDefault, StorageInfo, StorageInstance},
+	traits::{Get, GetDefault, StorageInfo, StorageInstance},
 };
 use codec::{Decode, Encode, EncodeLike, FullCodec, MaxEncodedLen};
 use frame_support::storage::StorageDecodeNonDedupLength;
@@ -72,7 +72,7 @@ where
 	Prefix: StorageInstance,
 	Value: FullCodec,
 	QueryKind: QueryKindTrait<Value, OnEmpty>,
-	OnEmpty: crate::traits::Get<QueryKind::Query> + 'static,
+	OnEmpty: Get<QueryKind::Query> + 'static,
 {
 	type Query = QueryKind::Query;
 	fn pallet_prefix() -> &'static [u8] {

substrate/frame/support/src/traits.rs

@@ -126,4 +126,7 @@ pub use tx_pause::{TransactionPause, TransactionPauseError};
 #[cfg(feature = "try-runtime")]
 mod try_runtime;
 #[cfg(feature = "try-runtime")]
-pub use try_runtime::{Select as TryStateSelect, TryState, UpgradeCheckSelect};
+pub use try_runtime::{
+	Select as TryStateSelect, TryDecodeEntireStorage, TryDecodeEntireStorageError, TryState,
+	UpgradeCheckSelect,
+};