Introduce storage attr macro #[disable_try_decode_storage] and set …

…it on `System::Events` and `ParachainSystem::HostConfiguration` (#3454)

Closes #2560

Allows marking storage items with `#[disable_try_decode_storage]`, and
uses it with `System::Events`.

Question: what's the recommended way to write a test for this? I
couldn't find a test for similar existing macro `#[whitelist_storage]`.

cumulus/pallets/parachain-system/src/lib.rs

@@ -840,6 +840,7 @@ pub mod pallet {
 	///
 	/// This data is also absent from the genesis.
 	#[pallet::storage]
+	#[pallet::disable_try_decode_storage]
 	#[pallet::getter(fn host_configuration)]
 	pub(super) type HostConfiguration<T: Config> = StorageValue<_, AbridgedHostConfiguration>;
 

prdoc/pr_3454.prdoc

@@ -0,0 +1,19 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: "Introduce storage attr macro #[disable_try_decode_storage] and set it on System::Events and ParachainSystem::HostConfiguration"
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      Allows marking storage items with \#[disable_try_decode_storage], which disables that storage item from being decoded
+      during try_decode_entire_state calls.
+
+      Applied the attribute to System::Events to close https://github.com/paritytech/polkadot-sdk/issues/2560.
+      Applied the attribute to ParachainSystem::HostConfiguration to resolve periodic issues with it.
+
+crates:
+  - name: frame-support-procedural
+  - name: frame-system
+  - name: cumulus-pallet-parachain-system
+

substrate/frame/support/procedural/src/lib.rs

@@ -1371,6 +1371,25 @@ pub fn whitelist_storage(_: TokenStream, _: TokenStream) -> TokenStream {
 	pallet_macro_stub()
 }
 
+/// The optional attribute `#[pallet::disable_try_decode_storage]` will declare the
+/// storage as whitelisted from decoding during try-runtime checks. This should only be
+/// attached to transient storage which cannot be migrated during runtime upgrades.
+///
+/// ### Example
+/// ```ignore
+/// 	#[pallet::storage]
+/// 	#[pallet::disable_try_decode_storage]
+/// 	pub(super) type Events<T: Config> = StorageValue<_, Vec<Box<EventRecord<T::RuntimeEvent, T::Hash>>>, ValueQuery>;
+/// ```
+///
+/// NOTE: As with all `pallet::*` attributes, this one _must_ be written as
+/// `#[pallet::disable_try_decode_storage]` and can only be placed inside a `pallet` module in order
+/// for it to work properly.
+#[proc_macro_attribute]
+pub fn disable_try_decode_storage(_: TokenStream, _: TokenStream) -> TokenStream {
+	pallet_macro_stub()
+}
+
 /// The `#[pallet::type_value]` attribute lets you define a struct implementing the `Get` trait
 /// to ease the use of storage types. This attribute is meant to be used alongside
 /// [`#[pallet::storage]`](`macro@storage`) to define a storage's default value. This attribute

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

@@ -834,7 +834,10 @@ pub fn expand_storages(def: &mut Def) -> proc_macro2::TokenStream {
 			.storages
 			.iter()
 			.filter_map(|storage| {
-				if storage.cfg_attrs.is_empty() {
+				// A little hacky; don't generate for cfg gated storages to not get compile errors
+				// when building "frame-feature-testing" gated storages in the "frame-support-test"
+				// crate.
+				if storage.try_decode && 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> ))

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

@@ -29,6 +29,7 @@ mod keyword {
 	syn::custom_keyword!(storage_prefix);
 	syn::custom_keyword!(unbounded);
 	syn::custom_keyword!(whitelist_storage);
+	syn::custom_keyword!(disable_try_decode_storage);
 	syn::custom_keyword!(OptionQuery);
 	syn::custom_keyword!(ResultQuery);
 	syn::custom_keyword!(ValueQuery);
@@ -39,11 +40,13 @@ mod keyword {
 /// * `#[pallet::storage_prefix = "CustomName"]`
 /// * `#[pallet::unbounded]`
 /// * `#[pallet::whitelist_storage]
+/// * `#[pallet::disable_try_decode_storage]`
 pub enum PalletStorageAttr {
 	Getter(syn::Ident, proc_macro2::Span),
 	StorageName(syn::LitStr, proc_macro2::Span),
 	Unbounded(proc_macro2::Span),
 	WhitelistStorage(proc_macro2::Span),
+	DisableTryDecodeStorage(proc_macro2::Span),
 }
 
 impl PalletStorageAttr {
@@ -53,6 +56,7 @@ impl PalletStorageAttr {
 			Self::StorageName(_, span) |
 			Self::Unbounded(span) |
 			Self::WhitelistStorage(span) => *span,
+			Self::DisableTryDecodeStorage(span) => *span,
 		}
 	}
 }
@@ -93,6 +97,9 @@ impl syn::parse::Parse for PalletStorageAttr {
 		} else if lookahead.peek(keyword::whitelist_storage) {
 			content.parse::<keyword::whitelist_storage>()?;
 			Ok(Self::WhitelistStorage(attr_span))
+		} else if lookahead.peek(keyword::disable_try_decode_storage) {
+			content.parse::<keyword::disable_try_decode_storage>()?;
+			Ok(Self::DisableTryDecodeStorage(attr_span))
 		} else {
 			Err(lookahead.error())
 		}
@@ -104,6 +111,7 @@ struct PalletStorageAttrInfo {
 	rename_as: Option<syn::LitStr>,
 	unbounded: bool,
 	whitelisted: bool,
+	try_decode: bool,
 }
 
 impl PalletStorageAttrInfo {
@@ -112,13 +120,16 @@ impl PalletStorageAttrInfo {
 		let mut rename_as = None;
 		let mut unbounded = false;
 		let mut whitelisted = false;
+		let mut disable_try_decode_storage = false;
 		for attr in attrs {
 			match attr {
 				PalletStorageAttr::Getter(ident, ..) if getter.is_none() => getter = Some(ident),
 				PalletStorageAttr::StorageName(name, ..) if rename_as.is_none() =>
 					rename_as = Some(name),
 				PalletStorageAttr::Unbounded(..) if !unbounded => unbounded = true,
 				PalletStorageAttr::WhitelistStorage(..) if !whitelisted => whitelisted = true,
+				PalletStorageAttr::DisableTryDecodeStorage(..) if !disable_try_decode_storage =>
+					disable_try_decode_storage = true,
 				attr =>
 					return Err(syn::Error::new(
 						attr.attr_span(),
@@ -127,7 +138,13 @@ impl PalletStorageAttrInfo {
 			}
 		}
 
-		Ok(PalletStorageAttrInfo { getter, rename_as, unbounded, whitelisted })
+		Ok(PalletStorageAttrInfo {
+			getter,
+			rename_as,
+			unbounded,
+			whitelisted,
+			try_decode: !disable_try_decode_storage,
+		})
 	}
 }
 
@@ -186,6 +203,8 @@ pub struct StorageDef {
 	pub unbounded: bool,
 	/// Whether or not reads to this storage key will be ignored by benchmarking
 	pub whitelisted: bool,
+	/// Whether or not to try to decode the storage key when running try-runtime checks.
+	pub try_decode: bool,
 	/// Whether or not a default hasher is allowed to replace `_`
 	pub use_default_hasher: bool,
 }
@@ -775,7 +794,7 @@ impl StorageDef {
 		};
 
 		let attrs: Vec<PalletStorageAttr> = helper::take_item_pallet_attrs(&mut item.attrs)?;
-		let PalletStorageAttrInfo { getter, rename_as, mut unbounded, whitelisted } =
+		let PalletStorageAttrInfo { getter, rename_as, mut unbounded, whitelisted, try_decode } =
 			PalletStorageAttrInfo::from_attrs(attrs)?;
 
 		// set all storages to be unbounded if dev_mode is enabled
@@ -921,6 +940,7 @@ impl StorageDef {
 			named_generics,
 			unbounded,
 			whitelisted,
+			try_decode,
 			use_default_hasher,
 		})
 	}

substrate/frame/support/src/lib.rs

@@ -982,6 +982,7 @@ pub mod pallet_prelude {
 /// * [`pallet::storage_prefix = "SomeName"`](#palletstorage_prefix--somename-optional)
 /// * [`pallet::unbounded`](#palletunbounded-optional)
 /// * [`pallet::whitelist_storage`](#palletwhitelist_storage-optional)
+/// * [`pallet::disable_try_decode_storage`](#palletdisable_try_decode_storage-optional)
 /// * [`cfg(..)`](#cfg-for-storage) (on storage items)
 /// * [`pallet::type_value`](#type-value-pallettype_value-optional)
 /// * [`pallet::genesis_config`](#genesis-config-palletgenesis_config-optional)
@@ -1498,6 +1499,15 @@ pub mod pallet_prelude {
 /// [`pallet::whitelist_storage`](frame_support::pallet_macros::whitelist_storage)
 /// for more info.
 ///
+/// ## `#[pallet::disable_try_decode_storage]` (optional)
+///
+/// The optional attribute `#[pallet::disable_try_decode_storage]` will declare the storage as
+/// whitelisted state decoding during try-runtime logic.
+///
+/// See
+/// [`pallet::disable_try_decode_storage`](frame_support::pallet_macros::disable_try_decode_storage)
+/// for more info.
+///
 ///	## `#[cfg(..)]` (for storage)
 /// The optional attributes `#[cfg(..)]` allow conditional compilation for the storage.
 ///
@@ -2272,8 +2282,8 @@ pub use frame_support_procedural::pallet;
 /// Contains macro stubs for all of the pallet:: macros
 pub mod pallet_macros {
 	pub use frame_support_procedural::{
-		composite_enum, config, disable_frame_system_supertrait_check, error, event,
-		extra_constants, feeless_if, generate_deposit, generate_store, getter, hooks,
+		composite_enum, config, disable_frame_system_supertrait_check, disable_try_decode_storage,
+		error, event, extra_constants, feeless_if, generate_deposit, generate_store, getter, hooks,
 		import_section, inherent, no_default, no_default_bounds, pallet_section, storage_prefix,
 		storage_version, type_value, unbounded, validate_unsigned, weight, whitelist_storage,
 	};
@@ -2698,6 +2708,8 @@ pub mod pallet_macros {
 	/// * [`macro@getter`]: Creates a custom getter function.
 	/// * [`macro@storage_prefix`]: Overrides the default prefix of the storage item.
 	/// * [`macro@unbounded`]: Declares the storage item as unbounded.
+	/// * [`macro@disable_try_decode_storage`]: Declares that try-runtime checks should not
+	///   attempt to decode the storage item.
 	///
 	/// #### Example
 	/// ```
@@ -2713,6 +2725,7 @@ pub mod pallet_macros {
 	/// 	#[pallet::getter(fn foo)]
 	/// 	#[pallet::storage_prefix = "OtherFoo"]
 	/// 	#[pallet::unbounded]
+	/// 	#[pallet::disable_try_decode_storage]
 	///     pub type Foo<T> = StorageValue<_, u32, ValueQuery>;
 	/// }
 	/// ```

substrate/frame/support/test/tests/pallet_ui/storage_invalid_attribute.stderr

@@ -1,4 +1,4 @@
-error: expected one of: `getter`, `storage_prefix`, `unbounded`, `whitelist_storage`
+error: expected one of: `getter`, `storage_prefix`, `unbounded`, `whitelist_storage`, `disable_try_decode_storage`
   --> tests/pallet_ui/storage_invalid_attribute.rs:33:12
    |
 33 |     #[pallet::generate_store(pub trait Store)]

substrate/frame/system/src/lib.rs

@@ -894,6 +894,7 @@ pub mod pallet {
 	/// just in case someone still reads them from within the runtime.
 	#[pallet::storage]
 	#[pallet::whitelist_storage]
+	#[pallet::disable_try_decode_storage]
 	#[pallet::unbounded]
 	pub(super) type Events<T: Config> =
 		StorageValue<_, Vec<Box<EventRecord<T::RuntimeEvent, T::Hash>>>, ValueQuery>;