From 95da6583602691fd0c8403e9eda26db1e10dafc4 Mon Sep 17 00:00:00 2001
From: Liam Aharon <liam.aharon@hotmail.com>
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 d86a67e58f4..3439227bc5b 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<T: Config> = StorageValue<_, AbridgedHostConfiguration>;
diff --git a/prdoc/pr_3454.prdoc b/prdoc/pr_3454.prdoc
new file mode 100644
index 00000000000..7f564258f23
--- /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 441b21e26ff..78729ee3dc3 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<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
diff --git a/substrate/frame/support/procedural/src/pallet/expand/storage.rs b/substrate/frame/support/procedural/src/pallet/expand/storage.rs
index 96c2c8e3120..937b068cfab 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 d1c7ba2e5e3..9d96a18b569 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::<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,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<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,
})
}
diff --git a/substrate/frame/support/src/lib.rs b/substrate/frame/support/src/lib.rs
index 26fc1fe42c1..3e97a384275 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<T> = 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 7f125526edf..519fadaa604 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 aa6841c008a..c527a483d88 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<T: Config> =
StorageValue<_, Vec<Box<EventRecord<T::RuntimeEvent, T::Hash>>>, ValueQuery>;
--
GitLab