Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce storage attr macro #[disable_try_decode_storage] and set it on System::Events and ParachainSystem::HostConfiguration #3454

Merged
merged 18 commits into from
Feb 28, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions prdoc/pr_3454.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://github.com/raw/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: "Introduce storage attr macro #[disable_try_decode_storage] and set it on System::Events"

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.

crates:
- name: frame-support-procedural
- name: frame-system

19 changes: 19 additions & 0 deletions substrate/frame/support/procedural/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
/// ```
liamaharon marked this conversation as resolved.
Show resolved Hide resolved
///
/// 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -834,7 +834,7 @@ pub fn expand_storages(def: &mut Def) -> proc_macro2::TokenStream {
.storages
.iter()
.filter_map(|storage| {
if storage.cfg_attrs.is_empty() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kianenigma do you remember why we check if storage.cfg_attrs.is_empty() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems it's to avoid this compile error https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5317738

I wonder if there's a better way to handle that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea we should probably re-expand the attributes here.
It seems to be only used only for testing.

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> ))
Expand Down
24 changes: 22 additions & 2 deletions substrate/frame/support/procedural/src/pallet/parse/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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 {
Expand All @@ -53,6 +56,7 @@ impl PalletStorageAttr {
Self::StorageName(_, span) |
Self::Unbounded(span) |
Self::WhitelistStorage(span) => *span,
Self::DisableTryDecodeStorage(span) => *span,
}
}
}
Expand Down Expand Up @@ -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())
}
Expand All @@ -104,6 +111,7 @@ struct PalletStorageAttrInfo {
rename_as: Option<syn::LitStr>,
unbounded: bool,
whitelisted: bool,
try_decode: bool,
}

impl PalletStorageAttrInfo {
Expand All @@ -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(),
Expand All @@ -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,
})
}
}

Expand Down Expand Up @@ -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,
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -921,6 +940,7 @@ impl StorageDef {
named_generics,
unbounded,
whitelisted,
try_decode,
use_default_hasher,
})
}
Expand Down
14 changes: 12 additions & 2 deletions substrate/frame/support/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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.
///
Expand Down Expand Up @@ -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, origin, pallet_section,
storage_prefix, storage_version, type_value, unbounded, validate_unsigned, weight,
whitelist_storage,
Expand Down
Original file line number Diff line number Diff line change
@@ -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)]
Expand Down
1 change: 1 addition & 0 deletions substrate/frame/system/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -892,6 +892,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>;
Expand Down
Loading