Allow to specify some max number of values for storages in pallet macro. #8735

gui1117 · 2021-05-05T11:33:18Z

DONE:

This PR allows to specify some max number of values for storages as a new optional generic.
And allows macros to generate storages info with an opt-in attribute.

Both macro use this information to implement a new trait on pallet: StoragesInfo:

/// Some info about a storage in a pallet.
#[derive(codec::Encode, codec::Decode, crate::RuntimeDebug, Eq, PartialEq, Clone)]
pub struct StorageInfo {
	/// The prefix of the storage. All keys after the prefix are considered part of the storage
	pub prefix: [u8; 32],
	/// The maximum number of values in the storage, or none if no maximum specified.
	pub max_values: Option<u32>,
	/// The maximum size of key/values in the storage, or none if no maximum specified.
	pub max_size: Option<u32>,
}

/// A trait to give information about storages.
///
/// It can be used to calculate PoV worst case size.
pub trait StoragesInfo {
	fn storages_info() -> Vec<StorageInfo>;
}

// So that AllPalletWithSystem::storages_info contains all informations
#[impl_trait_for_tuples::impl_for_tuples(30)]
impl StoragesInfo for Tuple {
	fn storages_info() -> Vec<StorageInfo> {
		let mut res = vec![];
		for_tuples!( #( res.extend_from_slice(&Tuple::storages_info()); )* );
		res
	}
}

/// A trait for types which contains storage info.
pub trait StorageInfoTrait {
	fn storage_info() -> StorageInfo;
}

So every storage implement StorageInfoTrait (on condition that value and keys implement MaxEncodedLen). Then macro calls it if the opt-in attribute is set.

For decl_storage the attribute is just a generate_storages_info in the front.
For pallet it is #[pallet::generate_storages_info] on top of pallet struct.

For decl_storage a new attribute max_values(..) is available to set this new value. I might make more sense to put after the map keyword though. I can do if requested.

(EDIT: in the past I mentionned an idea to not have to opt-in but it couldn't work because storages can be private).

The runtime can get all storages info with AllPalletWithSystem::storages_info().

NOTE: the usage of an additional generic in storages should be ease with this PR: #8751

TODO:

maybe some ui test for decl_storage
discuss the Vec<StorageInfo>, I was thinking it could have more information like frame_system could declare its usage of the :code: key. Like we would allow 2 kind of description:
- description from prefix: (for storages), the number of expected values after the prefix, and the size of these values
- description of specific key: just the size of the key and the key itself.
  but this can be done in follow-up PR

At the end I imagine we want to give this information to the client thourght Benchmarking runtime API so that client can then return the PoV worst case cost.

gui1117 · 2021-05-07T08:14:55Z

I marked a in-progress because I didn't take a last look to polish, add doc, add some more tests, and double check implementation. But the implementation should be quite good.

cla-bot-2021 · 2021-05-07T08:14:58Z

@thiolliere No commands could be detected from your comment.

frame/support/procedural/src/pallet/parse/pallet_struct.rs

shawntabrizi · 2021-05-12T15:06:02Z

frame/support/procedural/src/storage/storage_struct.rs

+									max_values: #max_values,
+									max_size: Some(max_size),


Not sure we have the right granularity here.

The impact on the child trie for a map will be some multiple of the value size * max_values

Since max_size encodes both the key and value, i dont think we can do the right math here to get the right PoV impact

@cheme

"child trie" ? I guess you mean the branched portion of the trie containing the Map.

The fact that max_size encodes both key and value is correct for iterable maps I think (IIRC key is stored with value as trie node value), and not for non iterable one I think.

So from 'max_values' you can estimate the number of branches in PoV. Size of branch also could be estimate since the trie key are fix length hash.
Depending on the number of value accessed (and kind of access), you will can estimate the number of shared branch too.

and not for non iterable one I think.

Ah you are right it is only the hasher which concatenate the key.

Should we include the hash part when computing the max_size for a storage key/value ?

like if a storagemap with hashertwox64concat has only one value, then the node contains the full key and the value, so the size include the hash part. (so 8 bytes + key::max_encoded_len + value::max_encoded_len).

What do you think ?
implemented in 999640b

I would share the 8bytes amongst branches size evaluation and then do (key::max_encoded_len + value::max_encoded_len) as you suggest (as if all the encoded key are in the leaf nodes (worst case scenario)).

Looking again at what is hasertwox64, (twox64(key), key) would make unhashed part highly improbably to be in a branch so it is no really a worst case scenario.
Putting key with value seems correct. (and twox part shared amongst branches).

Looking again at what is hasertwox64, (twox64(key), key) would make unhashed part highly improbably to be in a branch so it is no really a worst case scenario.

Yeah I agree, I'm ok to remove the hash part, if it is considered important we can improve on it later

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

frame/support/procedural/src/storage/storage_struct.rs

coriolinus · 2021-05-14T11:32:49Z

frame/support/src/lib.rs

-		traits::{Get, Hooks, IsType, GetPalletVersion, EnsureOrigin, PalletInfoAccess},
+		traits::{
+			Get, Hooks, IsType, GetPalletVersion, EnsureOrigin, PalletInfoAccess, StoragesInfo,
+			ConstU32, GetDefault,


Looks like ConstU32 isn't actually used in this file?

this is the prelude that people ususally import when declaring their pallet.

ConstU32 can be used to declare MaxValues easily:

type Foo<T> = StorageMap<_, Twox64Concat, u32, u32, OptionQuery, GetDefault, ConstU32<100_000>>

coriolinus · 2021-05-14T11:34:58Z

frame/support/src/traits.rs

@@ -50,7 +50,7 @@ mod misc;
 pub use misc::{
 	Len, Get, GetDefault, HandleLifetime, TryDrop, Time, UnixTime, IsType, IsSubType, ExecuteBlock,
 	SameOrOther, OnNewAccount, OnKilledAccount, OffchainWorker, GetBacking, Backing, ExtrinsicCall,
-	EnsureInherentsAreFirst,
+	EnsureInherentsAreFirst, ConstU32,


Likewise, ConstU32 here appears not to be used in the rest of the file.

I feel like ppl prefer to reexport everything in frame/support/src/traits instead of getting them from their own module. Though I agree we can remove this one, and it is in the pallet_prelude anyway.

frame/support/procedural/src/pallet/parse/pallet_struct.rs

coriolinus

This looks good to me! I have a few nits, but nothing that I think really needs to change.

I think it is more future proof. In the future some storage could make use of multiple prefix. Like one to store how much value has been inserted, etc...

Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>

hasher now expose `max_len` which allows to computes their maximum len. For hasher without concatenation, it is the size of the hash part, for hasher with concatenation, it is the size of the hash part + max encoded len of the key.

…values-macro

shawntabrizi

Ive been using this in my follow up PR and it is looking good.

I think it is ready to merge, and can be iterated on as we continue down the path.

gui1117 · 2021-05-17T13:44:14Z

I think it is ready to merge, and can be iterated on as we continue down the path.

Yes we should make sure the PoV benchmark is resolved before considering this API stable.

gui1117 · 2021-05-17T13:44:20Z

bot merge

ghost · 2021-05-17T13:44:24Z

Trying merge.

…ro. (paritytech#8735) * implement max_values + storages info * some formatting + doc * rename StoragesInfo -> PalletStorageInfo * merge both StorageInfoTrait and PalletStorageInfo I think it is more future proof. In the future some storage could make use of multiple prefix. Like one to store how much value has been inserted, etc... * Update frame/support/procedural/src/storage/parse.rs Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com> * Update frame/support/procedural/src/storage/storage_struct.rs Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com> * Fix max_size using hasher information hasher now expose `max_len` which allows to computes their maximum len. For hasher without concatenation, it is the size of the hash part, for hasher with concatenation, it is the size of the hash part + max encoded len of the key. * fix tests * fix ui tests Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>

github-actions bot added the A0-please_review Pull request needs code review. label May 5, 2021

gui1117 added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels May 5, 2021

gui1117 marked this pull request as draft May 5, 2021 13:03

implement max_values + storages info

0de4775

gui1117 force-pushed the gui-max_values-macro branch from 4d469eb to 0de4775 Compare May 6, 2021 17:28

gui1117 marked this pull request as ready for review May 6, 2021 17:37

github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels May 6, 2021

gui1117 added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels May 6, 2021

gui1117 mentioned this pull request May 6, 2021

Allow to name the generic for storages in #[pallet::storage] #8751

Merged

coriolinus mentioned this pull request May 7, 2021

MaxEncodedLen tracking issue #8719

Closed

20 tasks

gui1117 added 2 commits May 12, 2021 12:21

Merge remote-tracking branch 'origin/master' into gui-max_values-macro

e7c14fb

some formatting + doc

42c4f48

gui1117 requested review from shawntabrizi, kianenigma and coriolinus May 12, 2021 11:54

shawntabrizi reviewed May 12, 2021

View reviewed changes

frame/support/procedural/src/pallet/parse/pallet_struct.rs Outdated Show resolved Hide resolved

shawntabrizi reviewed May 12, 2021

View reviewed changes

frame/support/procedural/src/pallet/parse/pallet_struct.rs Outdated Show resolved Hide resolved

shawntabrizi reviewed May 12, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into gui-max_values-macro

0ca09ee

rename StoragesInfo -> PalletStorageInfo

ada719b

coriolinus reviewed May 14, 2021

View reviewed changes

coriolinus approved these changes May 14, 2021

View reviewed changes

merge both StorageInfoTrait and PalletStorageInfo

341d3f5

I think it is more future proof. In the future some storage could make use of multiple prefix. Like one to store how much value has been inserted, etc...

gui1117 force-pushed the gui-max_values-macro branch from e8f65d3 to 341d3f5 Compare May 14, 2021 12:09

gui1117 and others added 7 commits May 14, 2021 14:09

Update frame/support/procedural/src/storage/parse.rs

5521953

Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>

Update frame/support/procedural/src/storage/storage_struct.rs

02b0cbb

Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>

Fix max_size using hasher information

999640b

hasher now expose `max_len` which allows to computes their maximum len. For hasher without concatenation, it is the size of the hash part, for hasher with concatenation, it is the size of the hash part + max encoded len of the key.

fix tests

a7909d7

fix ui tests

e5964c0

Merge commit '8d02bb0bfc6136f6a3c805db19f51e43090a7cd4' into gui-max_…

15918ff

…values-macro

Merge remote-tracking branch 'origin/master' into gui-max_values-macro

4fc7829

shawntabrizi approved these changes May 17, 2021

View reviewed changes

ghost merged commit 6d78dbd into master May 17, 2021

ghost deleted the gui-max_values-macro branch May 17, 2021 13:44

shawntabrizi mentioned this pull request Jun 21, 2021

PoV Benchmarking Tracking Issue paritytech/polkadot-sdk#398

Open

9 tasks

ghzlatarev mentioned this pull request Feb 15, 2022

Implement MaxEncodedLen wherever necessary Manta-Network/Manta#387

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to specify some max number of values for storages in pallet macro. #8735

Allow to specify some max number of values for storages in pallet macro. #8735

gui1117 commented May 5, 2021 •

edited

Loading

gui1117 commented May 7, 2021

cla-bot-2021 bot commented May 7, 2021

shawntabrizi May 12, 2021

cheme May 12, 2021

gui1117 May 12, 2021

gui1117 May 14, 2021 •

edited

Loading

cheme May 14, 2021 •

edited

Loading

cheme May 14, 2021

gui1117 May 17, 2021 •

edited

Loading

coriolinus May 14, 2021

gui1117 May 14, 2021

coriolinus May 14, 2021

gui1117 May 14, 2021

coriolinus left a comment

shawntabrizi left a comment

gui1117 commented May 17, 2021

gui1117 commented May 17, 2021

ghost commented May 17, 2021

Allow to specify some max number of values for storages in pallet macro. #8735

Allow to specify some max number of values for storages in pallet macro. #8735

Conversation

gui1117 commented May 5, 2021 • edited Loading

DONE:

TODO:

gui1117 commented May 7, 2021

cla-bot-2021 bot commented May 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gui1117 May 14, 2021 • edited Loading

Choose a reason for hiding this comment

cheme May 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gui1117 May 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coriolinus left a comment

Choose a reason for hiding this comment

shawntabrizi left a comment

Choose a reason for hiding this comment

gui1117 commented May 17, 2021

gui1117 commented May 17, 2021

ghost commented May 17, 2021

gui1117 commented May 5, 2021 •

edited

Loading

gui1117 May 14, 2021 •

edited

Loading

cheme May 14, 2021 •

edited

Loading

gui1117 May 17, 2021 •

edited

Loading