Add special tag to exclude runtime storage items from benchmarking

[sam0x17]

Description

The Substrate runtime takes advantage of a storage cache where the first read of a storage item places an item in the cache, and subsequent reads of that item happen in memory.

There are some storage items which we know will be accessed every block, and thus we ignore reads to these items already in our benchmarking: https://github.com/paritytech/substrate/blob/master/bin/node/runtime/src/lib.rs#L1683

let whitelist: Vec<TrackedStorageKey> = vec![
    // Block Number
    hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef702a5c1b19ab7a04f536c519aca4983ac").to_vec().into(),
    // Total Issuance
    hex_literal::hex!("c2261276cc9d1f8598ea4b6a74b15c2f57c875e4cff74148e4628f264b974c80").to_vec().into(),
   ...
];

This PR adds a #[pallet::whitelist_storage] attribute macro that can be applied to storage item declarations. Doing so will result in that storage item's storage key being automatically added to the whitelist shown above, meaning they will be excluded from benchmarking calculations. The PR also replaces the existing hard-coded whitelist with #[pallet::whitelist_storage] calls that will instead generate the list organically.

This is accomplished by aggregating the #[pallet::whitelist_storage] attribute flags during pallet macro generation time, reading the storage keys for each storage item, and auto-implementing the trait WhitelistedStorageKeys which adds a function to each implementing pallet returning a Vec<TrackedStorageKey> listing the storage keys within that pallet that have been marked. This vec is then used in place of the hard-coded whitelist shown above.

Note that the actual attribute macro does nothing other than verify that it has been attached to a storage item declaration. The actual meat of this functionality all occurs in the pallet macro generation code.

A moonshot goal here that we may or may not be able to accomplish in this PR would be to panic if we can statically detect that the storage item the macro is being attached to is never read (or, even harder to detect, if it is infrequently read). This would prevent situations where the macro is applied to something it shouldn't be. That functionality probably won't be in scope for this PR, but noting here.

Notes

• this was formerly called #[benchmarking(cached)]

Follow-up Work

• the whole family of #[pallet:: attribute macros are currently parsed and have their tokens entirely removed by the main pallet proc macro. As a result there is actually no attribute macro definition for the individual variants of pallet::, making it very hard to find docs or look up these macros in docs.rs, etc. In the future we should add blank macro stubs for all the pallet:: macros that lack stubs and attach their documentation to these stubs instead of burying it in the frame_support main doc page. This could be a future PR.
• In the future we should add another macro called #[pallet::cache_storage] which will result in the storage key automatically being read at the beginning of every block. This will also automatically whitelist the storage.

Related Issues

• Fixes Add special tag to cache specific runtime storage items. #10251
• Related to META: FRAME Macro Ideas polkadot-sdk#264

[sam0x17]

I structured it so more could be added later. Right now it will panic saying “Only benchmarking(cached) is supported” or something like that if you try an argument other than cached

…

On Sep 8, 2022, at 1:12 AM, Keith Yeung ***@***.***> wrote: @KiChjang commented on this pull request. In frame/system/src/lib.rs <#12205 (comment)>: > #[pallet::unbounded] pub(super) type Events<T: Config> = StorageValue<_, Vec<Box<EventRecord<T::Event, T::Hash>>>, ValueQuery>; /// The number of events in the `Events<T>` list. #[pallet::storage] + #[benchmarking(cached)] I'm a bit ambivalent about this syntax, benchmarking(cached) seems to indicate that I can put keywords other than cached within the parenthesis. Is that true? — Reply to this email directly, view it on GitHub <#12205 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOE4LOWQWWBCDVSCVDBPATV5FYTJANCNFSM6AAAAAAQG3H3QA>. You are receiving this because you were assigned.

[sam0x17]

So my newest problem is I have my macro gen code in expand_pallet_struct() doing an impl like the following:

	let whitelisted_storage_keys_impl = quote::quote![
		impl<#type_use_gen> frame_support::traits::WhitelistedStorageKeys for #pallet_ident<#type_use_gen> {
			fn whitelisted_storage_keys() -> #frame_support::sp_std::vec::Vec<#frame_support::traits::TrackedStorageKey> {
				let info = Pallet::<T>::storage_info();
				#frame_support::sp_std::vec::Vec::new()
			}
		}
	];

I'm trying to get an access to Pallet::storage_info() to succeed without a compiler error, as I need to read the storage info at runtime to find the storage keys I believe. Problem is I keep keeting this sort of error:

error[E0599]: the function or associated item `storage_info` exists for struct `pallet::Pallet<T>`, but its trait bounds were not satisfied
   --> frame/system/src/lib.rs:197:1
    |
197 | #[frame_support::pallet]
    | ^^^^^^^^^^^^^^^^^^^^^^^^ function or associated item cannot be called on `pallet::Pallet<T>` due to unsatisfied trait bounds
...
360 |     pub struct Pallet<T>(_);
    |     ------------------------
    |     |
    |     function or associated item `storage_info` not found for this
    |     doesn't satisfy `_: frame_support::traits::StorageInfoTrait`
    |
note: trait bound `T: pallet::Config` was not satisfied
   --> frame/system/src/lib.rs:197:1
    |
197 |   #[frame_support::pallet]
    |   ^
...
358 |       #[pallet::pallet]
    |  _______________^
359 | |     #[pallet::generate_store(pub (super) trait Store)]
360 | |     pub struct Pallet<T>(_);
    | |_____________________^
    = note: this error originates in the attribute macro `frame_support::pallet` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider restricting the type parameter to satisfy the trait bound
    |
360 |     pub struct Pallet<T>(_) where T: pallet::Config;
    |                             +++++++++++++++++++++++

Anyone know how to deal with this and/or know of a better way to read the storage keys from within my impl method?

[sam0x17]

Solved using a setup like below. Turns out I was misusing #type_use_gen vs #type_impl_gen:

	let whitelisted_storage_keys_impl = quote::quote![
		use #frame_support::traits::StorageInfoTrait;
		impl<#type_impl_gen> #frame_support::traits::WhitelistedStorageKeys for #pallet_ident<#type_use_gen> {
			fn whitelisted_storage_keys() -> #frame_support::sp_std::vec::Vec<#frame_support::traits::TrackedStorageKey> {
				let info = #pallet_ident::<#type_use_gen>::storage_info();
				#frame_support::sp_std::vec::Vec::new()
			}
		}
	];

Hopefully this will let me do something useful with the StorageInfo object to get individual storage keys at runtime

[KiChjang]

Looks good after fixing my comment

[bkchr]

Looks good, just some last nitpicks.

[sam0x17]

bot merge