From 95da6583602691fd0c8403e9eda26db1e10dafc4 Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Wed, 28 Feb 2024 13:13:09 +1100 Subject: [PATCH] Introduce storage attr macro `#[disable_try_decode_storage]` and set it on `System::Events` and `ParachainSystem::HostConfiguration` (#3454) Closes https://github.com/paritytech/polkadot-sdk/issues/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 | 1 + prdoc/pr_3454.prdoc | 19 +++++++++++++++ substrate/frame/support/procedural/src/lib.rs | 19 +++++++++++++++ .../procedural/src/pallet/expand/storage.rs | 5 +++- .../procedural/src/pallet/parse/storage.rs | 24 +++++++++++++++++-- substrate/frame/support/src/lib.rs | 17 +++++++++++-- .../storage_invalid_attribute.stderr | 2 +- substrate/frame/system/src/lib.rs | 1 + 8 files changed, 82 insertions(+), 6 deletions(-) create mode 100644 prdoc/pr_3454.prdoc diff --git a/cumulus/pallets/parachain-system/src/lib.rs b/cumulus/pallets/parachain-system/src/lib.rs index d86a67e58f44..3439227bc5b0 100644 --- a/cumulus/pallets/parachain-system/src/lib.rs +++ b/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 = StorageValue<_, AbridgedHostConfiguration>; diff --git a/prdoc/pr_3454.prdoc b/prdoc/pr_3454.prdoc new file mode 100644 index 000000000000..7f564258f23a --- /dev/null +++ b/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 + diff --git a/substrate/frame/support/procedural/src/lib.rs b/substrate/frame/support/procedural/src/lib.rs index 441b21e26ff2..78729ee3dc3f 100644 --- a/substrate/frame/support/procedural/src/lib.rs +++ b/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 = StorageValue<_, Vec>>, 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 diff --git a/substrate/frame/support/procedural/src/pallet/expand/storage.rs b/substrate/frame/support/procedural/src/pallet/expand/storage.rs index 96c2c8e3120b..937b068cfabd 100644 --- a/substrate/frame/support/procedural/src/pallet/expand/storage.rs +++ b/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> )) diff --git a/substrate/frame/support/procedural/src/pallet/parse/storage.rs b/substrate/frame/support/procedural/src/pallet/parse/storage.rs index d1c7ba2e5e3c..9d96a18b5694 100644 --- a/substrate/frame/support/procedural/src/pallet/parse/storage.rs +++ b/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::()?; Ok(Self::WhitelistStorage(attr_span)) + } else if lookahead.peek(keyword::disable_try_decode_storage) { + content.parse::()?; + Ok(Self::DisableTryDecodeStorage(attr_span)) } else { Err(lookahead.error()) } @@ -104,6 +111,7 @@ struct PalletStorageAttrInfo { rename_as: Option, unbounded: bool, whitelisted: bool, + try_decode: bool, } impl PalletStorageAttrInfo { @@ -112,6 +120,7 @@ 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), @@ -119,6 +128,8 @@ impl PalletStorageAttrInfo { 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 = 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, }) } diff --git a/substrate/frame/support/src/lib.rs b/substrate/frame/support/src/lib.rs index 26fc1fe42c11..3e97a3842756 100644 --- a/substrate/frame/support/src/lib.rs +++ b/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 = StorageValue<_, u32, ValueQuery>; /// } /// ``` diff --git a/substrate/frame/support/test/tests/pallet_ui/storage_invalid_attribute.stderr b/substrate/frame/support/test/tests/pallet_ui/storage_invalid_attribute.stderr index 7f125526edf2..519fadaa6049 100644 --- a/substrate/frame/support/test/tests/pallet_ui/storage_invalid_attribute.stderr +++ b/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)] diff --git a/substrate/frame/system/src/lib.rs b/substrate/frame/system/src/lib.rs index aa6841c008a9..c527a483d880 100644 --- a/substrate/frame/system/src/lib.rs +++ b/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 = StorageValue<_, Vec>>, ValueQuery>;