Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Used CountedStorageMap in pallet-staking #10233

Merged
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
662fffe
Removed counters and used CountedStorageMap instead.
ayevbeosa Nov 11, 2021
48a4825
Merge branch 'master' of https://github.com/paritytech/substrate into…
ayevbeosa Nov 11, 2021
da2d7ec
Little refactoring
ayevbeosa Nov 11, 2021
31f7bd7
Update frame/staking/src/migrations.rs
ayevbeosa Nov 11, 2021
a8c2f16
Removed redundant code to update counter for validator & nominator.
ayevbeosa Nov 11, 2021
01a605d
Removed redundant code to update counter for validator & nominator.
ayevbeosa Nov 11, 2021
2c192d8
Removed unreachable code to inject the hashed prefix for nominator & …
ayevbeosa Nov 11, 2021
c2f7577
Removed redundant check for nominator & validator count.
ayevbeosa Nov 15, 2021
b332858
Merge branch 'master' of https://github.com/paritytech/substrate into…
ayevbeosa Nov 16, 2021
d771e4a
Merge branch 'master' of https://github.com/paritytech/substrate into…
ayevbeosa Nov 19, 2021
e449fe5
Generated `fn prefix_hash` for `CountedStorageMap`.
ayevbeosa Nov 19, 2021
65d7d02
Merge branch 'master' of https://github.com/paritytech/substrate into…
ayevbeosa Nov 19, 2021
b9d0a88
Applied changes from `cargo fmt`
ayevbeosa Nov 19, 2021
9b21bef
Merge branch 'master' of https://github.com/paritytech/substrate into…
ayevbeosa Nov 19, 2021
d79d380
Possible correct implementation of migration code
ayevbeosa Nov 20, 2021
16d6a44
Merge branch 'master' of https://github.com/paritytech/substrate into…
ayevbeosa Nov 24, 2021
190ec8e
Implemented fn module_prefix, storage_prefix and prefix_hash.
ayevbeosa Nov 24, 2021
41f8dae
Merge branch 'master' of https://github.com/paritytech/substrate into…
ayevbeosa Nov 24, 2021
3a0500f
Removed counted_map.rs
ayevbeosa Nov 29, 2021
fac8882
Merge branch 'master' of https://github.com/paritytech/substrate into…
ayevbeosa Nov 29, 2021
bf1bc0a
Renamed `fn storage_prefix` to `storage_counter_prefix`.
ayevbeosa Nov 30, 2021
36115cb
Merge branch 'master' of https://github.com/paritytech/substrate into…
ayevbeosa Nov 30, 2021
de685ff
Update frame/support/src/storage/types/counted_map.rs
gui1117 Dec 1, 2021
d07414f
Update frame/bags-list/remote-tests/src/snapshot.rs
gui1117 Dec 1, 2021
256ac95
Update frame/support/src/storage/types/counted_map.rs
gui1117 Dec 1, 2021
fa56998
Merge branch 'master' into ayevbeosa-employ-counted-map
ayevbeosa Dec 4, 2021
a437c17
Fixed errors.
ayevbeosa Dec 4, 2021
b7e8047
Merge branch 'master' of https://github.com/paritytech/substrate into…
ayevbeosa Dec 4, 2021
da0f59f
Merge remote-tracking branch 'origin/master' into 10233
gui1117 Dec 7, 2021
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
3 changes: 1 addition & 2 deletions frame/bags-list/remote-tests/src/snapshot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ pub async fn execute<Runtime: crate::RuntimeT, Block: BlockT + DeserializeOwned>
ws_url: String,
) {
use frame_support::storage::generator::StorageMap;

let mut ext = Builder::<Block>::new()
.mode(Mode::Online(OnlineConfig {
transport: ws_url.to_string().into(),
Expand All @@ -40,8 +41,6 @@ pub async fn execute<Runtime: crate::RuntimeT, Block: BlockT + DeserializeOwned>
.inject_hashed_prefix(&<pallet_staking::Ledger<Runtime>>::prefix_hash())
.inject_hashed_prefix(&<pallet_staking::Validators<Runtime>>::prefix_hash())
.inject_hashed_prefix(&<pallet_staking::Nominators<Runtime>>::prefix_hash())
gui1117 marked this conversation as resolved.
Show resolved Hide resolved
.inject_hashed_key(&<pallet_staking::CounterForNominators<Runtime>>::hashed_key())
gui1117 marked this conversation as resolved.
Show resolved Hide resolved
.inject_hashed_key(&<pallet_staking::CounterForValidators<Runtime>>::hashed_key())
.build()
.await
.unwrap();
Expand Down
8 changes: 4 additions & 4 deletions frame/staking/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,8 @@ pub fn create_validator_with_nominators<T: Config>(

assert_eq!(new_validators.len(), 1);
assert_eq!(new_validators[0], v_stash, "Our validator was not selected!");
assert_ne!(CounterForValidators::<T>::get(), 0);
assert_ne!(CounterForNominators::<T>::get(), 0);
assert_ne!(Validators::<T>::count(), 0);
assert_ne!(Nominators::<T>::count(), 0);

// Give Era Points
let reward = EraRewardPoints::<T::AccountId> {
Expand Down Expand Up @@ -919,8 +919,8 @@ mod tests {
let count_validators = Validators::<Test>::iter().count();
let count_nominators = Nominators::<Test>::iter().count();

assert_eq!(count_validators, CounterForValidators::<Test>::get() as usize);
assert_eq!(count_nominators, CounterForNominators::<Test>::get() as usize);
assert_eq!(count_validators, Validators::<Test>::count() as usize);
assert_eq!(count_nominators, Nominators::<Test>::count() as usize);

assert_eq!(count_validators, v as usize);
assert_eq!(count_nominators, n as usize);
Expand Down
20 changes: 16 additions & 4 deletions frame/staking/src/migrations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,22 @@ pub mod v8 {

pub mod v7 {
use super::*;
use frame_support::generate_storage_alias;

generate_storage_alias!(Staking, CounterForValidators => Value<u32>);
gui1117 marked this conversation as resolved.
Show resolved Hide resolved
generate_storage_alias!(Staking, CounterForNominators => Value<u32>);

pub fn pre_migrate<T: Config>() -> Result<(), &'static str> {
assert!(CounterForValidators::<T>::get().is_zero(), "CounterForValidators already set.");
assert!(CounterForNominators::<T>::get().is_zero(), "CounterForNominators already set.");
assert!(
CounterForValidators::get().unwrap().is_zero(),
"CounterForValidators already set."
);
assert!(
CounterForNominators::get().unwrap().is_zero(),
"CounterForNominators already set."
);
assert!(Validators::<T>::count().is_zero(), "Validators already set.");
assert!(Nominators::<T>::count().is_zero(), "Nominators already set.");
ayevbeosa marked this conversation as resolved.
Show resolved Hide resolved
assert!(StorageVersion::<T>::get() == Releases::V6_0_0);
Ok(())
}
Expand All @@ -83,8 +95,8 @@ 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this question for me or @kianenigma

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

CounterForNominators::<T>::put(nominator_count);
CounterForValidators::put(validator_count);
CounterForNominators::put(nominator_count);

StorageVersion::<T>::put(Releases::V7_0_0);
log!(info, "Completed staking migration to Releases::V7_0_0");
Expand Down
4 changes: 2 additions & 2 deletions frame/staking/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -536,8 +536,8 @@ fn post_conditions() {
fn check_count() {
let nominator_count = Nominators::<Test>::iter().count() as u32;
let validator_count = Validators::<Test>::iter().count() as u32;
assert_eq!(nominator_count, CounterForNominators::<Test>::get());
assert_eq!(validator_count, CounterForValidators::<Test>::get());
assert_eq!(nominator_count, Nominators::<Test>::count());
assert_eq!(validator_count, Validators::<Test>::count());

// the voters that the `SortedListProvider` list is storing for us.
let external_voters = <Test as Config>::SortedListProvider::count();
Expand Down
55 changes: 16 additions & 39 deletions frame/staking/src/pallet/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -665,8 +665,8 @@ impl<T: Config> Pallet<T> {
maybe_max_len: Option<usize>,
) -> Vec<(T::AccountId, VoteWeight, Vec<T::AccountId>)> {
let max_allowed_len = {
let nominator_count = CounterForNominators::<T>::get() as usize;
let validator_count = CounterForValidators::<T>::get() as usize;
let nominator_count = Nominators::<T>::count() as usize;
let validator_count = Validators::<T>::count() as usize;
let all_voter_count = validator_count.saturating_add(nominator_count);
maybe_max_len.unwrap_or(all_voter_count).min(all_voter_count)
};
Expand Down Expand Up @@ -765,18 +765,15 @@ impl<T: Config> Pallet<T> {
}

/// This function will add a nominator to the `Nominators` storage map,
/// [`SortedListProvider`] and keep track of the `CounterForNominators`.
/// and [`SortedListProvider`].
///
/// If the nominator already exists, their nominations will be updated.
///
/// NOTE: you must ALWAYS use this function to add nominator or update their targets. Any access
/// to `Nominators`, its counter, or `VoterList` outside of this function is almost certainly
/// to `Nominators` or `VoterList` outside of this function is almost certainly
/// wrong.
pub fn do_add_nominator(who: &T::AccountId, nominations: Nominations<T::AccountId>) {
if !Nominators::<T>::contains_key(who) {
// maybe update the counter.
CounterForNominators::<T>::mutate(|x| x.saturating_inc());

// maybe update sorted list. Error checking is defensive-only - this should never fail.
if T::SortedListProvider::on_insert(who.clone(), Self::weight_of(who)).is_err() {
log!(warn, "attempt to insert duplicate nominator ({:#?})", who);
Expand All @@ -790,53 +787,46 @@ impl<T: Config> Pallet<T> {
}

/// This function will remove a nominator from the `Nominators` storage map,
/// [`SortedListProvider`] and keep track of the `CounterForNominators`.
/// and [`SortedListProvider`].
///
/// Returns true if `who` was removed from `Nominators`, otherwise false.
///
/// NOTE: you must ALWAYS use this function to remove a nominator from the system. Any access to
/// `Nominators`, its counter, or `VoterList` outside of this function is almost certainly
/// `Nominators` or `VoterList` outside of this function is almost certainly
/// wrong.
pub fn do_remove_nominator(who: &T::AccountId) -> bool {
if Nominators::<T>::contains_key(who) {
Nominators::<T>::remove(who);
CounterForNominators::<T>::mutate(|x| x.saturating_dec());
T::SortedListProvider::on_remove(who);
debug_assert_eq!(T::SortedListProvider::sanity_check(), Ok(()));
debug_assert_eq!(CounterForNominators::<T>::get(), T::SortedListProvider::count());
debug_assert_eq!(Nominators::<T>::count(), T::SortedListProvider::count());
true
} else {
false
}
}

/// This function will add a validator to the `Validators` storage map, and keep track of the
/// `CounterForValidators`.
/// This function will add a validator to the `Validators` storage map.
///
/// If the validator already exists, their preferences will be updated.
///
/// NOTE: you must ALWAYS use this function to add a validator to the system. Any access to
/// `Validators`, its counter, or `VoterList` outside of this function is almost certainly
/// `Validators` or `VoterList` outside of this function is almost certainly
/// wrong.
pub fn do_add_validator(who: &T::AccountId, prefs: ValidatorPrefs) {
if !Validators::<T>::contains_key(who) {
CounterForValidators::<T>::mutate(|x| x.saturating_inc())
}
Validators::<T>::insert(who, prefs);
}

/// This function will remove a validator from the `Validators` storage map,
/// and keep track of the `CounterForValidators`.
/// This function will remove a validator from the `Validators` storage map.
///
/// Returns true if `who` was removed from `Validators`, otherwise false.
///
/// NOTE: you must ALWAYS use this function to remove a validator from the system. Any access to
/// `Validators`, its counter, or `VoterList` outside of this function is almost certainly
/// `Validators` or `VoterList` outside of this function is almost certainly
/// wrong.
pub fn do_remove_validator(who: &T::AccountId) -> bool {
if Validators::<T>::contains_key(who) {
Validators::<T>::remove(who);
CounterForValidators::<T>::mutate(|x| x.saturating_dec());
true
} else {
false
Expand Down Expand Up @@ -865,14 +855,6 @@ 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_eq!(
CounterForNominators::<T>::get(),
T::SortedListProvider::count(),
"voter_count must be accurate",
);

// This can never fail -- if `maybe_max_len` is `Some(_)` we handle it.
let voters = Self::get_npos_voters(maybe_max_len);
debug_assert!(maybe_max_len.map_or(true, |max| voters.len() <= max));
Expand All @@ -881,7 +863,7 @@ impl<T: Config> ElectionDataProvider<T::AccountId, BlockNumberFor<T>> for Pallet
}

fn targets(maybe_max_len: Option<usize>) -> data_provider::Result<Vec<T::AccountId>> {
let target_count = CounterForValidators::<T>::get();
let target_count = Validators::<T>::count();

// We can't handle this case yet -- return an error.
if maybe_max_len.map_or(false, |max_len| target_count > max_len as u32) {
Expand Down Expand Up @@ -969,8 +951,6 @@ impl<T: Config> ElectionDataProvider<T::AccountId, BlockNumberFor<T>> for Pallet
<Ledger<T>>::remove_all(None);
<Validators<T>>::remove_all(None);
<Nominators<T>>::remove_all(None);
<CounterForNominators<T>>::kill();
<CounterForValidators<T>>::kill();
let _ = T::SortedListProvider::clear(None);
}

Expand Down Expand Up @@ -1284,7 +1264,7 @@ impl<T: Config> SortedListProvider<T::AccountId> for UseNominatorsMap<T> {
Box::new(Nominators::<T>::iter().map(|(n, _)| n))
}
fn count() -> u32 {
CounterForNominators::<T>::get()
Nominators::<T>::count()
}
fn contains(id: &T::AccountId) -> bool {
Nominators::<T>::contains_key(id)
Expand All @@ -1310,12 +1290,9 @@ impl<T: Config> SortedListProvider<T::AccountId> for UseNominatorsMap<T> {
Ok(())
}
fn clear(maybe_count: Option<u32>) -> u32 {
let count_before = Nominators::<T>::count();
Nominators::<T>::remove_all(maybe_count);
if let Some(count) = maybe_count {
CounterForNominators::<T>::mutate(|noms| *noms - count);
count
} else {
CounterForNominators::<T>::take()
}
let count_after = Nominators::<T>::count();
count_before.saturating_sub(count_after)
shawntabrizi marked this conversation as resolved.
Show resolved Hide resolved
}
}
26 changes: 7 additions & 19 deletions frame/staking/src/pallet/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -228,16 +228,10 @@ pub mod pallet {
StorageMap<_, Twox64Concat, T::AccountId, RewardDestination<T::AccountId>, ValueQuery>;

/// The map from (wannabe) validator stash key to the preferences of that validator.
///
/// When updating this storage item, you must also update the `CounterForValidators`.
#[pallet::storage]
#[pallet::getter(fn validators)]
pub type Validators<T: Config> =
StorageMap<_, Twox64Concat, T::AccountId, ValidatorPrefs, ValueQuery>;

/// A tracker to keep count of the number of items in the `Validators` map.
#[pallet::storage]
pub type CounterForValidators<T> = StorageValue<_, u32, ValueQuery>;
CountedStorageMap<_, Twox64Concat, T::AccountId, ValidatorPrefs, ValueQuery>;

/// The maximum validator count before we stop allowing new validators to join.
///
Expand All @@ -246,16 +240,10 @@ pub mod pallet {
pub type MaxValidatorsCount<T> = StorageValue<_, u32, OptionQuery>;

/// The map from nominator stash key to the set of stash keys of all validators to nominate.
///
/// When updating this storage item, you must also update the `CounterForNominators`.
#[pallet::storage]
#[pallet::getter(fn nominators)]
pub type Nominators<T: Config> =
StorageMap<_, Twox64Concat, T::AccountId, Nominations<T::AccountId>>;

/// A tracker to keep count of the number of items in the `Nominators` map.
#[pallet::storage]
pub type CounterForNominators<T> = StorageValue<_, u32, ValueQuery>;
CountedStorageMap<_, Twox64Concat, T::AccountId, Nominations<T::AccountId>>;

/// The maximum nominator count before we stop allowing new validators to join.
///
Expand Down Expand Up @@ -554,7 +542,7 @@ pub mod pallet {
// all voters are reported to the `SortedListProvider`.
assert_eq!(
T::SortedListProvider::count(),
CounterForNominators::<T>::get(),
Nominators::<T>::count(),
"not all genesis stakers were inserted into sorted list provider, something is wrong."
);
}
Expand Down Expand Up @@ -965,7 +953,7 @@ pub mod pallet {
// the runtime.
if let Some(max_validators) = MaxValidatorsCount::<T>::get() {
ensure!(
CounterForValidators::<T>::get() < max_validators,
Validators::<T>::count() < max_validators,
Error::<T>::TooManyValidators
);
}
Expand Down Expand Up @@ -1005,7 +993,7 @@ pub mod pallet {
// the runtime.
if let Some(max_nominators) = MaxNominatorsCount::<T>::get() {
ensure!(
CounterForNominators::<T>::get() < max_nominators,
Nominators::<T>::count() < max_nominators,
Error::<T>::TooManyNominators
);
}
Expand Down Expand Up @@ -1582,7 +1570,7 @@ pub mod pallet {
let min_active_bond = if Nominators::<T>::contains_key(&stash) {
let max_nominator_count =
MaxNominatorsCount::<T>::get().ok_or(Error::<T>::CannotChillOther)?;
let current_nominator_count = CounterForNominators::<T>::get();
let current_nominator_count = Nominators::<T>::count();
ensure!(
threshold * max_nominator_count < current_nominator_count,
Error::<T>::CannotChillOther
Expand All @@ -1591,7 +1579,7 @@ pub mod pallet {
} else if Validators::<T>::contains_key(&stash) {
let max_validator_count =
MaxValidatorsCount::<T>::get().ok_or(Error::<T>::CannotChillOther)?;
let current_validator_count = CounterForValidators::<T>::get();
let current_validator_count = Validators::<T>::count();
ensure!(
threshold * max_validator_count < current_validator_count,
Error::<T>::CannotChillOther
Expand Down
3 changes: 1 addition & 2 deletions frame/staking/src/testing_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,10 @@ const SEED: u32 = 0;
/// This function removes all validators and nominators from storage.
pub fn clear_validators_and_nominators<T: Config>() {
Validators::<T>::remove_all(None);
CounterForValidators::<T>::kill();

// whenever we touch nominators counter we should update `T::SortedListProvider` as well.
Nominators::<T>::remove_all(None);
CounterForNominators::<T>::kill();

let _ = T::SortedListProvider::clear(None);
}

Expand Down
16 changes: 8 additions & 8 deletions frame/staking/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4300,8 +4300,8 @@ fn chill_other_works() {
.min_nominator_bond(1_000)
.min_validator_bond(1_500)
.build_and_execute(|| {
let initial_validators = CounterForValidators::<Test>::get();
let initial_nominators = CounterForNominators::<Test>::get();
let initial_validators = Validators::<Test>::count();
let initial_nominators = Nominators::<Test>::count();
for i in 0..15 {
let a = 4 * i;
let b = 4 * i + 1;
Expand Down Expand Up @@ -4413,8 +4413,8 @@ fn chill_other_works() {
));

// 16 people total because tests start with 2 active one
assert_eq!(CounterForNominators::<Test>::get(), 15 + initial_nominators);
assert_eq!(CounterForValidators::<Test>::get(), 15 + initial_validators);
assert_eq!(Nominators::<Test>::count(), 15 + initial_nominators);
assert_eq!(Validators::<Test>::count(), 15 + initial_validators);

// Users can now be chilled down to 7 people, so we try to remove 9 of them (starting
// with 16)
Expand All @@ -4426,23 +4426,23 @@ fn chill_other_works() {
}

// chill a nominator. Limit is not reached, not chill-able
assert_eq!(CounterForNominators::<Test>::get(), 7);
assert_eq!(Nominators::<Test>::count(), 7);
assert_noop!(
Staking::chill_other(Origin::signed(1337), 1),
Error::<Test>::CannotChillOther
);
// chill a validator. Limit is reached, chill-able.
assert_eq!(CounterForValidators::<Test>::get(), 9);
assert_eq!(Validators::<Test>::count(), 9);
assert_ok!(Staking::chill_other(Origin::signed(1337), 3));
})
}

#[test]
fn capped_stakers_works() {
ExtBuilder::default().build_and_execute(|| {
let validator_count = CounterForValidators::<Test>::get();
let validator_count = Validators::<Test>::count();
assert_eq!(validator_count, 3);
let nominator_count = CounterForNominators::<Test>::get();
let nominator_count = Nominators::<Test>::count();
assert_eq!(nominator_count, 1);

// Change the maximums
Expand Down
Loading