Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from all 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
6 changes: 3 additions & 3 deletions bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ use pallet_grandpa::{
};
use pallet_im_online::sr25519::AuthorityId as ImOnlineId;
use pallet_session::historical as pallet_session_historical;
use pallet_staking::UseValidatorsMap;
pub use pallet_transaction_payment::{CurrencyAdapter, Multiplier, TargetedFeeAdjustment};
use pallet_transaction_payment::{FeeDetails, RuntimeDispatchInfo};
use sp_api::impl_runtime_apis;
Expand Down Expand Up @@ -555,9 +556,8 @@ impl pallet_staking::Config for Runtime {
type OffendingValidatorsThreshold = OffendingValidatorsThreshold;
type ElectionProvider = ElectionProviderMultiPhase;
type GenesisElectionProvider = onchain::OnChainSequentialPhragmen<Self>;
// Alternatively, use pallet_staking::UseNominatorsMap<Runtime> to just use the nominators map.
// Note that the aforementioned does not scale to a very large number of nominators.
type SortedListProvider = BagsList;
type VoterList = BagsList;
type TargetList = UseValidatorsMap<Self>;
type WeightInfo = pallet_staking::weights::SubstrateWeight<Runtime>;
type BenchmarkingConfig = StakingBenchmarkingConfig;
}
Expand Down
11 changes: 5 additions & 6 deletions frame/multisig/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,11 @@
//! ## Overview
//!
//! This pallet contains functionality for multi-signature dispatch, a (potentially) stateful
//! operation, allowing multiple signed
//! origins (accounts) to coordinate and dispatch a call from a well-known origin, derivable
//! deterministically from the set of account IDs and the threshold number of accounts from the
//! set that must approve it. In the case that the threshold is just one then this is a stateless
//! operation. This is useful for multisig wallets where cryptographic threshold signatures are
//! not available or desired.
//! operation, allowing multiple signed origins (accounts) to coordinate and dispatch a call from a
//! well-known origin, derivable deterministically from the set of account IDs and the threshold
//! number of accounts from the set that must approve it. In the case that the threshold is just one
//! then this is a stateless operation. This is useful for multisig wallets where cryptographic
//! threshold signatures are not available or desired.
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like just a line re-wrap? IMO it would be preferable to keep this out since it seems totally unrelated

//!
//! ## Interface
//!
Expand Down
32 changes: 16 additions & 16 deletions frame/staking/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ impl<T: Config> ListScenario<T> {

// find a destination weight that will trigger the worst case scenario
let dest_weight_as_vote =
T::SortedListProvider::weight_update_worst_case(&origin_stash1, is_increase);
T::VoterList::weight_update_worst_case(&origin_stash1, is_increase);

let total_issuance = T::Currency::total_issuance();

Expand Down Expand Up @@ -316,7 +316,7 @@ benchmarks! {
let scenario = ListScenario::<T>::new(origin_weight, true)?;
let controller = scenario.origin_controller1.clone();
let stash = scenario.origin_stash1.clone();
assert!(T::SortedListProvider::contains(&stash));
assert!(T::VoterList::contains(&stash));

let ed = T::Currency::minimum_balance();
let mut ledger = Ledger::<T>::get(&controller).unwrap();
Expand All @@ -328,7 +328,7 @@ benchmarks! {
}: withdraw_unbonded(RawOrigin::Signed(controller.clone()), s)
verify {
assert!(!Ledger::<T>::contains_key(controller));
assert!(!T::SortedListProvider::contains(&stash));
assert!(!T::VoterList::contains(&stash));
}

validate {
Expand All @@ -338,14 +338,14 @@ benchmarks! {
Default::default(),
)?;
// because it is chilled.
assert!(!T::SortedListProvider::contains(&stash));
assert!(!T::VoterList::contains(&stash));

let prefs = ValidatorPrefs::default();
whitelist_account!(controller);
}: _(RawOrigin::Signed(controller), prefs)
verify {
assert!(Validators::<T>::contains_key(&stash));
assert!(T::SortedListProvider::contains(&stash));
assert!(T::VoterList::contains(&stash));
}

kick {
Expand Down Expand Up @@ -430,14 +430,14 @@ benchmarks! {
).unwrap();

assert!(!Nominators::<T>::contains_key(&stash));
assert!(!T::SortedListProvider::contains(&stash));
assert!(!T::VoterList::contains(&stash));

let validators = create_validators::<T>(n, 100).unwrap();
whitelist_account!(controller);
}: _(RawOrigin::Signed(controller), validators)
verify {
assert!(Nominators::<T>::contains_key(&stash));
assert!(T::SortedListProvider::contains(&stash))
assert!(T::VoterList::contains(&stash))
}

chill {
Expand All @@ -451,12 +451,12 @@ benchmarks! {
let scenario = ListScenario::<T>::new(origin_weight, true)?;
let controller = scenario.origin_controller1.clone();
let stash = scenario.origin_stash1.clone();
assert!(T::SortedListProvider::contains(&stash));
assert!(T::VoterList::contains(&stash));

whitelist_account!(controller);
}: _(RawOrigin::Signed(controller))
verify {
assert!(!T::SortedListProvider::contains(&stash));
assert!(!T::VoterList::contains(&stash));
}

set_payee {
Expand Down Expand Up @@ -519,13 +519,13 @@ benchmarks! {
let scenario = ListScenario::<T>::new(origin_weight, true)?;
let controller = scenario.origin_controller1.clone();
let stash = scenario.origin_stash1.clone();
assert!(T::SortedListProvider::contains(&stash));
assert!(T::VoterList::contains(&stash));
add_slashing_spans::<T>(&stash, s);

}: _(RawOrigin::Root, stash.clone(), s)
verify {
assert!(!Ledger::<T>::contains_key(&controller));
assert!(!T::SortedListProvider::contains(&stash));
assert!(!T::VoterList::contains(&stash));
}

cancel_deferred_slash {
Expand Down Expand Up @@ -704,13 +704,13 @@ benchmarks! {
Ledger::<T>::insert(&controller, l);

assert!(Bonded::<T>::contains_key(&stash));
assert!(T::SortedListProvider::contains(&stash));
assert!(T::VoterList::contains(&stash));

whitelist_account!(controller);
}: _(RawOrigin::Signed(controller), stash.clone(), s)
verify {
assert!(!Bonded::<T>::contains_key(&stash));
assert!(!T::SortedListProvider::contains(&stash));
assert!(!T::VoterList::contains(&stash));
}

new_era {
Expand Down Expand Up @@ -844,7 +844,7 @@ benchmarks! {
v, n, T::MaxNominations::get() as usize, false, None
)?;
}: {
let targets = <Staking<T>>::get_npos_targets();
let targets = <Staking<T>>::get_npos_targets(None);
assert_eq!(targets.len() as u32, v);
}

Expand Down Expand Up @@ -878,7 +878,7 @@ benchmarks! {
let scenario = ListScenario::<T>::new(origin_weight, true)?;
let controller = scenario.origin_controller1.clone();
let stash = scenario.origin_stash1.clone();
assert!(T::SortedListProvider::contains(&stash));
assert!(T::VoterList::contains(&stash));

Staking::<T>::set_staking_configs(
RawOrigin::Root.into(),
Expand All @@ -893,7 +893,7 @@ benchmarks! {
let caller = whitelisted_caller();
}: _(RawOrigin::Signed(caller), controller.clone())
verify {
assert!(!T::SortedListProvider::contains(&stash));
assert!(!T::VoterList::contains(&stash));
}

force_apply_min_commission {
Expand Down
11 changes: 6 additions & 5 deletions frame/staking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -767,11 +767,12 @@ enum Releases {
V2_0_0,
V3_0_0,
V4_0_0,
V5_0_0, // blockable validators.
V6_0_0, // removal of all storage associated with offchain phragmen.
V7_0_0, // keep track of number of nominators / validators in map
V8_0_0, // populate `SortedListProvider`.
V9_0_0, // inject validators into `SortedListProvider` as well.
V5_0_0, // blockable validators.
V6_0_0, // removal of all storage associated with offchain phragmen.
V7_0_0, // keep track of number of nominators / validators in map
V8_0_0, // populate `VoterList`.
V9_0_0, // inject validators into `NPoSVoteProvider` as well.
V10_0_0, // inject validator into `NPoSTargetList`.
Comment on lines +774 to +775
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
V9_0_0, // inject validators into `NPoSVoteProvider` as well.
V10_0_0, // inject validator into `NPoSTargetList`.
V9_0_0, // inject validators into `VoterList` as well.
V10_0_0, // inject validator into `TargetList`.

}

impl Default for Releases {
Expand Down
67 changes: 50 additions & 17 deletions frame/staking/src/migrations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,45 +22,79 @@ use frame_support::traits::OnRuntimeUpgrade;

/// Migration implementation that injects all validators into sorted list.
///
/// This is only useful for chains that started their `SortedListProvider` just based on nominators.
pub struct InjectValidatorsIntoSortedListProvider<T>(sp_std::marker::PhantomData<T>);
impl<T: Config> OnRuntimeUpgrade for InjectValidatorsIntoSortedListProvider<T> {
/// This is only useful for chains that started their `VoterList` just based on nominators.
pub struct InjectValidatorsIntoVoterList<T>(sp_std::marker::PhantomData<T>);
impl<T: Config> OnRuntimeUpgrade for InjectValidatorsIntoVoterList<T> {
fn on_runtime_upgrade() -> Weight {
if StorageVersion::<T>::get() == Releases::V8_0_0 {
for (v, _) in Validators::<T>::iter() {
let weight = Pallet::<T>::vote_weight(&v);
let _ = T::SortedListProvider::on_insert(v.clone(), weight).map_err(|err| {
log!(warn, "failed to insert {:?} into SortedListProvider: {:?}", v, err)
let _ = T::VoterList::on_insert(v.clone(), weight).map_err(|err| {
log!(warn, "failed to insert {:?} into VoterList: {:?}", v, err)
});
}

StorageVersion::<T>::put(Releases::V9_0_0);
T::BlockWeights::get().max_block
} else {
log!(warn, "InjectValidatorsIntoSortedListProvider being executed on the wrong storage version, expected Releases::V8_0_0");
log!(warn, "InjectValidatorsIntoVoterList being executed on the wrong storage version, expected Releases::V8_0_0");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log!(warn, "InjectValidatorsIntoVoterList being executed on the wrong storage version, expected Releases::V8_0_0");
log!(warn, "InjectValidatorsIntoVoterList being attempted on the wrong storage version, expected Releases::V8_0_0");

maybe? Executed makes it sound like we did the migration anyways

T::DbWeight::get().reads(1)
}
}

#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<(), &'static str> {
use frame_support::traits::OnRuntimeUpgradeHelpersExt;
frame_support::ensure!(
StorageVersion::<T>::get() == crate::Releases::V8_0_0,
"must upgrade linearly"
);
ensure!(StorageVersion::<T>::get() == crate::Releases::V8_0_0, "must upgrade linearly");

let prev_count = T::SortedListProvider::count();
let prev_count = T::VoterList::count();
Self::set_temp_storage(prev_count, "prev");
Ok(())
}

#[cfg(feature = "try-runtime")]
fn post_upgrade() -> Result<(), &'static str> {
use frame_support::traits::OnRuntimeUpgradeHelpersExt;
let post_count = T::SortedListProvider::count();
let post_count = T::VoterList::count();
let prev_count = Self::get_temp_storage::<u32>("prev").unwrap();
let validators = Validators::<T>::count();
assert!(post_count == prev_count + validators);
ensure!(post_count == prev_count + validators, "incorrect count");
ensure!(StorageVersion::<T>::get(), Releases::V9_0_0, "version not set");
Ok(())
}
}

/// Migration implementation that injects all validators into sorted list.
///
/// This is only useful for chains that started their `VoterList` just based on nominators.
pub struct InjectValidatorsIntoTargetList<T>(sp_std::marker::PhantomData<T>);
impl<T: Config> OnRuntimeUpgrade for InjectValidatorsIntoTargetList<T> {
fn on_runtime_upgrade() -> Weight {
if StorageVersion::<T>::get() == Releases::V9_0_0 {
for (v, _) in Validators::<T>::iter() {
let weight = Pallet::<T>::vote_weight(&v);
let _ = T::TargetList::on_insert(v.clone(), weight).map_err(|err| {
log!(warn, "failed to insert {:?} into TargetList: {:?}", v, err)
});
}
Comment on lines +74 to +79
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid duplicate code we can instead use:

TargetList::unsafe_regenerate(				
    Validators::<T>::iter().map(|(id, _)| id),
	Pallet::<T>::weight_of_fn(),
)

There isn't any performance benefit though (originally there was, but that never made it into master because we refactored insert_many to just use the normal insert)


StorageVersion::<T>::put(Releases::V10_0_0);
T::BlockWeights::get().max_block
} else {
log!(warn, "InjectValidatorsIntoTargetList being executed on the wrong storage version, expected Releases::V9_0_0");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log!(warn, "InjectValidatorsIntoTargetList being executed on the wrong storage version, expected Releases::V9_0_0");
log!(warn, "InjectValidatorsIntoTargetList being attempted on the wrong storage version, expected Releases::V9_0_0");

Same rationale as above - although not really a big deal

T::DbWeight::get().reads(1)
}
}

#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<(), &'static str> {
ensure!(StorageVersion::<T>::get() == Releases::V9_0_0, "must upgrade linearly");
Ok(())
}

#[cfg(feature = "try-runtime")]
fn post_upgrade() -> Result<(), &'static str> {
ensure!(StorageVersion::<T>::get(), Releases::V10_0_0, "must upgrade linearly");
Copy link
Contributor

Choose a reason for hiding this comment

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

We should ensure!(Validators::count() == TargetList::count(), "each validator must be in the target list")

Copy link
Contributor

@emostov emostov Mar 1, 2022

Choose a reason for hiding this comment

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

Also T::TargetList::sanity_check().map_err(|_| "TargetList is not in a sane state.")?;

Ok(())
}
}
Expand All @@ -87,11 +121,11 @@ pub mod v8 {
if StorageVersion::<T>::get() == crate::Releases::V7_0_0 {
crate::log!(info, "migrating staking to Releases::V8_0_0");

let migrated = T::SortedListProvider::unsafe_regenerate(
let migrated = T::VoterList::unsafe_regenerate(
Nominators::<T>::iter().map(|(id, _)| id),
Pallet::<T>::weight_of_fn(),
);
debug_assert_eq!(T::SortedListProvider::sanity_check(), Ok(()));
debug_assert_eq!(T::VoterList::sanity_check(), Ok(()));

StorageVersion::<T>::put(crate::Releases::V8_0_0);
crate::log!(
Expand All @@ -108,8 +142,7 @@ pub mod v8 {

#[cfg(feature = "try-runtime")]
pub fn post_migrate<T: Config>() -> Result<(), &'static str> {
T::SortedListProvider::sanity_check()
.map_err(|_| "SortedListProvider is not in a sane state.")?;
T::VoterList::sanity_check().map_err(|_| "VoterList is not in a sane state.")?;
crate::log!(info, "👜 staking bags-list migration passes POST migrate checks ✅",);
Ok(())
}
Expand Down
7 changes: 4 additions & 3 deletions frame/staking/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,8 @@ impl crate::pallet::pallet::Config for Test {
type ElectionProvider = onchain::OnChainSequentialPhragmen<Self>;
type GenesisElectionProvider = Self::ElectionProvider;
// NOTE: consider a macro and use `UseNominatorsAndValidatorsMap<Self>` as well.
type SortedListProvider = BagsList;
type VoterList = BagsList;
type TargetList = UseValidatorsMap<Self>;
type BenchmarkingConfig = TestBenchmarkingConfig;
type WeightInfo = ();
}
Expand Down Expand Up @@ -539,8 +540,8 @@ fn check_count() {
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();
// the voters that the `VoterList` list is storing for us.
let external_voters = <Test as Config>::VoterList::count();
assert_eq!(external_voters, nominator_count + validator_count);
}

Expand Down
Loading