Used CountedStorageMap in pallet-staking #10233

ayevbeosa · 2021-11-11T09:53:05Z

As raised in issue #10180, CountedStorageMap has been used for the Validators & Nominators type and its corresponding counters has also been removed.

Polkadot address: 12dVgRw6ntsMQiETAoCBzu6MA7LPWagvU4Y1rH9jGPDHQvPX

… ayevbeosa-employ-counted-map

frame/staking/src/migrations.rs

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

frame/staking/src/pallet/impls.rs

shawntabrizi · 2021-11-11T14:04:08Z

Looking good otherwise, thank you.

Check the CI for some compiler issues and things you still need to fix.

shawntabrizi · 2021-11-11T14:04:26Z

/tip medium

substrate-tip-bot · 2021-11-11T14:04:27Z

Please fix the following problems before calling the tip bot again:

Contributor did not properly post their Polkadot or Kusama address. Make sure the pull request has: "{network} address: {address}".

ayevbeosa · 2021-11-11T14:33:02Z

/tip medium

Thanks for the tip 😀
I have added my wallet address now

…validator.

shawntabrizi · 2021-11-11T15:39:08Z

/tip medium

substrate-tip-bot · 2021-11-11T15:39:20Z

A medium tip was successfully submitted for ayevbeosa (12dVgRw6ntsMQiETAoCBzu6MA7LPWagvU4Y1rH9jGPDHQvPX on polkadot).

https://polkadot.js.org/apps/#/treasury/tips

kianenigma · 2021-11-15T15:32:54Z

frame/staking/src/pallet/impls.rs

@@ -865,10 +855,10 @@ impl<T: Config> ElectionDataProvider<T::AccountId, BlockNumberFor<T>> for Pallet
 	fn voters(
 		maybe_max_len: Option<usize>,
 	) -> data_provider::Result<Vec<(T::AccountId, VoteWeight, Vec<T::AccountId>)>> {
-		debug_assert!(<Nominators<T>>::iter().count() as u32 == CounterForNominators::<T>::get());
-		debug_assert!(<Validators<T>>::iter().count() as u32 == CounterForValidators::<T>::get());
+		debug_assert!(<Nominators<T>>::iter().count() as u32 == Nominators::<T>::count());


This is kinda pointless now to have. We already check these in fn check_count as well, which is called after every test.

Okay that makes sense, I will have them removed.

kianenigma · 2021-11-15T15:37:50Z

frame/staking/src/migrations.rs

@@ -83,9 +83,6 @@ pub mod v7 {
 		let validator_count = Validators::<T>::iter().count() as u32;
 		let nominator_count = Nominators::<T>::iter().count() as u32;

-		CounterForValidators::<T>::put(validator_count);


This is technically making this migration code WRONG. Either remove it completely, or make sure it is correct.

we don't enforce this now, but a correct way would be to make sure this migration code stays correct over time. That is, it should have its own storage items and not depend on the pallet types. In this case, you need to create (mock) storage types via generate_storage_alias and use them to set a correct value for the counters.

Or, we need some special set_counter to set the counters upon migration, but I don't think we have this for now.

If all of this is too much, with despair and sadness, I am also fine with just removing the migration code altogether. But the long term correct thing to do is as I said above.

Could you help with the correct implementation details on how to write the migration code, I am still new to the Substrate repo.

you can look into other use cases of generate_storage_alias and its documentation. This allows you to generate a storage value type that mimics the one that is being removed. In this case, you can create a

generate_storage_value!(CounterForValidators => Value<u32>);

and now you have CounterForValidators back in scope, and you can use it.

is staking pallet using the pallet macro in v7 ?
If so then the generate_storage_value can use a wrong pallet prefix.

Is this question for me or @kianenigma

yeah it was already migrated. Then the very pedantic course of action would be to accept the prefix as the argument of the migration function, but I will be fine with skipping it since the migration code is already at risk to becoming outdated (since it depends on T: Config). So the assumption is that whoever uses this migration code should test it, and we're good.

frame/bags-list/remote-tests/src/snapshot.rs

kianenigma

Thanks! have some minor complains.

… ayevbeosa-employ-counted-map

gui1117

this is what I have in mind WDYT ?

frame/support/src/storage/types/counted_map.rs

frame/bags-list/remote-tests/src/snapshot.rs

frame/support/src/storage/types/counted_map.rs

gui1117

maybe we can have a test that the counter is correctly stored at twox128(pallet prefix)++twox128("CounterForNominators").
We can just do some assertion to asset that Nominators::counter_storage_final_key is equal to twox128(b"Staking") ++ twox128(b"CounterForNominators").

But otherwise looks good to me

gui1117 · 2021-12-02T15:35:12Z

Will depend on merging first this other PR Fix pallet bags list and doc #10231 (which should be ready, just need to update master)

kianenigma · 2021-12-03T23:07:19Z

frame/support/src/storage/types/counted_map.rs

@@ -99,6 +99,17 @@ where
 	OnEmpty: Get<QueryKind::Query> + 'static,
 	MaxValues: Get<Option<u32>>,
 {
+	/// The key used to store the counter of the map.


very good 👍

kianenigma

LGTM

… ayevbeosa-employ-counted-map

gui1117 · 2021-12-07T02:08:31Z

bot merge

paritytech-processbot · 2021-12-07T02:08:40Z

Waiting for commit status.

paritytech-processbot · 2021-12-07T02:41:08Z

Merge cancelled due to error. Error: Checks failed for da0f59f

gui1117 · 2021-12-07T09:15:09Z

bot merge

paritytech-processbot · 2021-12-07T09:15:18Z

Waiting for commit status.

* Removed counters and used CountedStorageMap instead. * Little refactoring * Update frame/staking/src/migrations.rs Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com> * Removed redundant code to update counter for validator & nominator. * Removed redundant code to update counter for validator & nominator. * Removed unreachable code to inject the hashed prefix for nominator & validator. * Removed redundant check for nominator & validator count. * Generated `fn prefix_hash` for `CountedStorageMap`. * Applied changes from `cargo fmt` * Possible correct implementation of migration code * Implemented fn module_prefix, storage_prefix and prefix_hash. * Removed counted_map.rs * Renamed `fn storage_prefix` to `storage_counter_prefix`. * Update frame/support/src/storage/types/counted_map.rs * Update frame/bags-list/remote-tests/src/snapshot.rs * Update frame/support/src/storage/types/counted_map.rs * Fixed errors. Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com> Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

ayevbeosa added 2 commits November 11, 2021 10:44

Removed counters and used CountedStorageMap instead.

662fffe

Merge branch 'master' of https://github.com/paritytech/substrate into…

48a4825

… ayevbeosa-employ-counted-map

ayevbeosa requested a review from kianenigma as a code owner November 11, 2021 09:53

Little refactoring

da2d7ec

shawntabrizi mentioned this pull request Nov 11, 2021

moved pallet staking counters to counted storage map #10234

Closed

shawntabrizi reviewed Nov 11, 2021

View reviewed changes

frame/staking/src/migrations.rs Show resolved Hide resolved

Update frame/staking/src/migrations.rs

31f7bd7

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

shawntabrizi reviewed Nov 11, 2021

View reviewed changes

frame/staking/src/pallet/impls.rs Outdated Show resolved Hide resolved

shawntabrizi approved these changes Nov 11, 2021

View reviewed changes

shawntabrizi added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Nov 11, 2021

shawntabrizi added D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. and removed D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Nov 11, 2021

Removed redundant code to update counter for validator & nominator.

a8c2f16

ayevbeosa added 2 commits November 11, 2021 15:36

Removed redundant code to update counter for validator & nominator.

01a605d

Removed unreachable code to inject the hashed prefix for nominator & …

2c192d8

…validator.

kianenigma reviewed Nov 15, 2021

View reviewed changes

frame/bags-list/remote-tests/src/snapshot.rs Show resolved Hide resolved

kianenigma suggested changes Nov 15, 2021

View reviewed changes

Removed redundant check for nominator & validator count.

c2f7577

shawntabrizi requested a review from kianenigma November 30, 2021 17:43

ayevbeosa added 2 commits November 30, 2021 19:27

Renamed fn storage_prefix to storage_counter_prefix.

bf1bc0a

Merge branch 'master' of https://github.com/paritytech/substrate into…

36115cb

… ayevbeosa-employ-counted-map

gui1117 reviewed Dec 1, 2021

View reviewed changes

frame/support/src/storage/types/counted_map.rs Outdated Show resolved Hide resolved

frame/bags-list/remote-tests/src/snapshot.rs Outdated Show resolved Hide resolved

gui1117 added 2 commits December 1, 2021 16:37

Update frame/support/src/storage/types/counted_map.rs

de685ff

Update frame/bags-list/remote-tests/src/snapshot.rs

d07414f

gui1117 reviewed Dec 1, 2021

View reviewed changes

frame/support/src/storage/types/counted_map.rs Outdated Show resolved Hide resolved

Update frame/support/src/storage/types/counted_map.rs

256ac95

gui1117 approved these changes Dec 1, 2021

View reviewed changes

kianenigma reviewed Dec 3, 2021

View reviewed changes

kianenigma approved these changes Dec 3, 2021

View reviewed changes

kianenigma added the A8-mergeoncegreen label Dec 3, 2021

ayevbeosa and others added 4 commits December 4, 2021 06:37

Merge branch 'master' into ayevbeosa-employ-counted-map

fa56998

Fixed errors.

a437c17

Merge branch 'master' of https://github.com/paritytech/substrate into…

b7e8047

… ayevbeosa-employ-counted-map

Merge remote-tracking branch 'origin/master' into 10233

da0f59f

paritytech-processbot bot merged commit 6079539 into paritytech:master Dec 7, 2021

KiChjang mentioned this pull request Dec 8, 2021

Move pallet-staking's counters to CountedMap #10180

Closed

AurevoirXavier added a commit to darwinia-network/darwinia-common that referenced this pull request Sep 8, 2022

Companion of paritytech/substrate#10231, paritytech/substrate#10233

ecb22b3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Used CountedStorageMap in pallet-staking #10233

Used CountedStorageMap in pallet-staking #10233

ayevbeosa commented Nov 11, 2021 •

edited

Loading

shawntabrizi commented Nov 11, 2021 •

edited

Loading

shawntabrizi commented Nov 11, 2021

substrate-tip-bot bot commented Nov 11, 2021

ayevbeosa commented Nov 11, 2021 •

edited

Loading

shawntabrizi commented Nov 11, 2021

substrate-tip-bot bot commented Nov 11, 2021

kianenigma Nov 15, 2021

ayevbeosa Nov 15, 2021

kianenigma Nov 15, 2021

ayevbeosa Nov 17, 2021

kianenigma Nov 18, 2021

gui1117 Nov 23, 2021

ayevbeosa Dec 1, 2021

kianenigma Dec 3, 2021

kianenigma left a comment

gui1117 left a comment

gui1117 left a comment

gui1117 commented Dec 2, 2021 •

edited

Loading

kianenigma Dec 3, 2021

kianenigma left a comment

gui1117 commented Dec 7, 2021

paritytech-processbot bot commented Dec 7, 2021

paritytech-processbot bot commented Dec 7, 2021

gui1117 commented Dec 7, 2021

paritytech-processbot bot commented Dec 7, 2021

Used CountedStorageMap in pallet-staking #10233

Used CountedStorageMap in pallet-staking #10233

Conversation

ayevbeosa commented Nov 11, 2021 • edited Loading

shawntabrizi commented Nov 11, 2021 • edited Loading

shawntabrizi commented Nov 11, 2021

substrate-tip-bot bot commented Nov 11, 2021

ayevbeosa commented Nov 11, 2021 • edited Loading

shawntabrizi commented Nov 11, 2021

substrate-tip-bot bot commented Nov 11, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

gui1117 left a comment

Choose a reason for hiding this comment

gui1117 left a comment

Choose a reason for hiding this comment

gui1117 commented Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

gui1117 commented Dec 7, 2021

paritytech-processbot bot commented Dec 7, 2021

paritytech-processbot bot commented Dec 7, 2021

gui1117 commented Dec 7, 2021

paritytech-processbot bot commented Dec 7, 2021

ayevbeosa commented Nov 11, 2021 •

edited

Loading

shawntabrizi commented Nov 11, 2021 •

edited

Loading

ayevbeosa commented Nov 11, 2021 •

edited

Loading

gui1117 commented Dec 2, 2021 •

edited

Loading