From f14d4d853c9b15fbb7301570838ad883efa76262 Mon Sep 17 00:00:00 2001 From: Xavier Lau Date: Sun, 5 Dec 2021 14:52:04 +0800 Subject: [PATCH] Companion for Substrate#9193 --- frame/staking/src/lib.rs | 100 +++++++++---- frame/staking/src/substrate_tests.rs | 215 ++++++++++++++++++++++----- frame/staking/src/weights.rs | 6 +- 3 files changed, 249 insertions(+), 72 deletions(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 98a095b46a..963fa3e5bc 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -891,6 +891,12 @@ pub mod pallet { #[pallet::storage] pub type StorageVersion = StorageValue<_, Releases, ValueQuery>; + /// The threshold for when users can start calling `chill_other` for other validators / nominators. + /// The threshold is compared to the actual number of validators / nominators (`CountFor*`) in + /// the system compared to the configured max (`Max*Count`). + #[pallet::storage] + pub type ChillThreshold = StorageValue<_, Percent, OptionQuery>; + /// The chain's running time form genesis in milliseconds, /// use for calculate darwinia era payout #[pallet::storage] @@ -1624,16 +1630,6 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::validate())] pub fn validate(origin: OriginFor, prefs: ValidatorPrefs) -> DispatchResult { let controller = ensure_signed(origin)?; - - // If this error is reached, we need to adjust the `MinValidatorBond` and start calling `chill_other`. - // Until then, we explicitly block new validators to protect the runtime. - if let Some(max_validators) = >::get() { - ensure!( - >::get() < max_validators, - >::TooManyValidators - ); - } - let ledger = Self::ledger(&controller).ok_or(>::NotController)?; ensure!( @@ -1643,6 +1639,18 @@ pub mod pallet { let stash = &ledger.stash; + // Only check limits if they are not already a validator. + if !>::contains_key(stash) { + // If this error is reached, we need to adjust the `MinValidatorBond` and start calling `chill_other`. + // Until then, we explicitly block new validators to protect the runtime. + if let Some(max_validators) = >::get() { + ensure!( + >::get() < max_validators, + >::TooManyValidators + ); + } + } + Self::do_remove_nominator(stash); Self::do_add_validator(stash, prefs); @@ -1674,16 +1682,6 @@ pub mod pallet { targets: Vec<::Source>, ) -> DispatchResult { let controller = ensure_signed(origin)?; - - // If this error is reached, we need to adjust the `MinNominatorBond` and start calling `chill_other`. - // Until then, we explicitly block new nominators to protect the runtime. - if let Some(max_nominators) = >::get() { - ensure!( - >::get() < max_nominators, - >::TooManyNominators - ); - } - let ledger = Self::ledger(&controller).ok_or(>::NotController)?; ensure!( @@ -1693,6 +1691,18 @@ pub mod pallet { let stash = &ledger.stash; + // Only check limits if they are not already a nominator. + if !>::contains_key(stash) { + // If this error is reached, we need to adjust the `MinNominatorBond` and start calling `chill_other`. + // Until then, we explicitly block new nominators to protect the runtime. + if let Some(max_nominators) = >::get() { + ensure!( + >::get() < max_nominators, + >::TooManyNominators + ); + } + } + ensure!(!targets.is_empty(), >::EmptyTargets); ensure!( targets.len() <= T::MAX_NOMINATIONS as usize, @@ -2288,13 +2298,14 @@ pub mod pallet { /// /// NOTE: Existing nominators and validators will not be affected by this update. /// to kick people under the new limits, `chill_other` should be called. - #[pallet::weight(T::WeightInfo::update_staking_limits())] - pub fn update_staking_limits( + #[pallet::weight(T::WeightInfo::set_staking_limits())] + pub fn set_staking_limits( origin: OriginFor, min_nominator_bond: RingBalance, min_validator_bond: RingBalance, max_nominator_count: Option, max_validator_count: Option, + threshold: Option, ) -> DispatchResult { ensure_root(origin)?; @@ -2302,19 +2313,29 @@ pub mod pallet { >::set(min_validator_bond); >::set(max_nominator_count); >::set(max_validator_count); + >::set(threshold); Ok(()) } - /// Declare a `controller` as having no desire to either validator or nominate. + /// Declare a `controller` to stop participating as either a validator or nominator. /// /// Effects will be felt at the beginning of the next era. /// /// The dispatch origin for this call must be _Signed_, but can be called by anyone. /// - /// If the caller is the same as the controller being targeted, then no further checks - /// are enforced. However, this call can also be made by an third party user who witnesses - /// that this controller does not satisfy the minimum bond requirements to be in their role. + /// If the caller is the same as the controller being targeted, then no further checks are + /// enforced, and this function behaves just like `chill`. + /// + /// If the caller is different than the controller being targeted, the following conditions + /// must be met: + /// * A `ChillThreshold` must be set and checked which defines how close to the max + /// nominators or validators we must reach before users can start chilling one-another. + /// * A `MaxNominatorCount` and `MaxValidatorCount` must be set which is used to determine + /// how close we are to the threshold. + /// * A `MinNominatorBond` and `MinValidatorBond` must be set and checked, which determines + /// if this is a person that should be chilled because they have not met the threshold + /// bond required. /// /// This can be helpful if bond requirements are updated, and we need to remove old users /// who do not satisfy these requirements. @@ -2328,14 +2349,37 @@ pub mod pallet { let ledger = Self::ledger(&controller).ok_or(>::NotController)?; let stash = ledger.stash; - // If the caller is not the controller, we want to check that the minimum bond - // requirements are not satisfied, and thus we have reason to chill this user. + // In order for one user to chill another user, the following conditions must be met: + // * A `ChillThreshold` is set which defines how close to the max nominators or + // validators we must reach before users can start chilling one-another. + // * A `MaxNominatorCount` and `MaxValidatorCount` which is used to determine how close + // we are to the threshold. + // * A `MinNominatorBond` and `MinValidatorBond` which is the final condition checked to + // determine this is a person that should be chilled because they have not met the + // threshold bond required. // // Otherwise, if caller is the same as the controller, this is just like `chill`. if caller != controller { + let threshold = >::get().ok_or(>::CannotChillOther)?; let min_active_bond = if >::contains_key(&stash) { + let max_nominator_count = + >::get().ok_or(>::CannotChillOther)?; + let current_nominator_count = >::get(); + ensure!( + threshold * max_nominator_count < current_nominator_count, + >::CannotChillOther + ); + >::get() } else if >::contains_key(&stash) { + let max_validator_count = + >::get().ok_or(>::CannotChillOther)?; + let current_validator_count = >::get(); + ensure!( + threshold * max_validator_count < current_validator_count, + >::CannotChillOther + ); + >::get() } else { Zero::zero() diff --git a/frame/staking/src/substrate_tests.rs b/frame/staking/src/substrate_tests.rs index fbb6647180..c97d9dbfef 100644 --- a/frame/staking/src/substrate_tests.rs +++ b/frame/staking/src/substrate_tests.rs @@ -40,7 +40,7 @@ use frame_support::{ use sp_runtime::{ assert_eq_error_rate, traits::{BadOrigin, Dispatchable, Zero}, - Perbill, + Perbill, Percent, }; use sp_staking::{ offence::{OffenceDetails, OnOffenceHandler}, @@ -50,7 +50,6 @@ use substrate_test_utils::assert_eq_uvec; // --- darwinia-network --- use crate::{ mock::{AccountId, Balance, *}, - testing_utils::*, weights::SubstrateWeight, Event, *, }; @@ -4837,51 +4836,149 @@ mod election_data_provider { .min_nominator_bond(1_000) .min_validator_bond(1_500) .build_and_execute(|| { - // Nominator - assert_ok!(Staking::bond( - Origin::signed(1), - 2, - StakingBalance::RingBalance(1000), - RewardDestination::Controller, - 0 - )); - assert_ok!(Staking::nominate(Origin::signed(2), vec![1])); + for i in 0..15 { + let a = 4 * i; + let b = 4 * i + 1; + let c = 4 * i + 2; + let d = 4 * i + 3; + Ring::make_free_balance_be(&a, 100_000); + Ring::make_free_balance_be(&b, 100_000); + Ring::make_free_balance_be(&c, 100_000); + Ring::make_free_balance_be(&d, 100_000); + + // Nominator + assert_ok!(Staking::bond( + Origin::signed(a), + b, + StakingBalance::RingBalance(1000), + RewardDestination::Controller, + 0 + )); + assert_ok!(Staking::nominate(Origin::signed(b), vec![1])); + + // Validator + assert_ok!(Staking::bond( + Origin::signed(c), + d, + StakingBalance::RingBalance(1500), + RewardDestination::Controller, + 0 + )); + assert_ok!(Staking::validate( + Origin::signed(d), + ValidatorPrefs::default() + )); + } - // Validator - assert_ok!(Staking::bond( - Origin::signed(3), - 4, - StakingBalance::RingBalance(1500), - RewardDestination::Controller, - 0 - )); - assert_ok!(Staking::validate( - Origin::signed(4), - ValidatorPrefs::default() - )); + // To chill other users, we need to: + // * Set a minimum bond amount + // * Set a limit + // * Set a threshold + // + // If any of these are missing, we do not have enough information to allow the + // `chill_other` to succeed from one user to another. // Can't chill these users assert_noop!( - Staking::chill_other(Origin::signed(1), 2), + Staking::chill_other(Origin::signed(1337), 1), >::CannotChillOther ); assert_noop!( - Staking::chill_other(Origin::signed(1), 4), + Staking::chill_other(Origin::signed(1337), 3), >::CannotChillOther ); - // Change the minimum bond - assert_ok!(Staking::update_staking_limits( + // Change the minimum bond... but no limits. + assert_ok!(Staking::set_staking_limits( Origin::root(), 1_500, 2_000, None, + None, + None + )); + + // Still can't chill these users + assert_noop!( + Staking::chill_other(Origin::signed(1337), 1), + >::CannotChillOther + ); + assert_noop!( + Staking::chill_other(Origin::signed(1337), 3), + >::CannotChillOther + ); + + // Add limits, but no threshold + assert_ok!(Staking::set_staking_limits( + Origin::root(), + 1_500, + 2_000, + Some(10), + Some(10), None )); - // Users can now be chilled - assert_ok!(Staking::chill_other(Origin::signed(1), 2)); - assert_ok!(Staking::chill_other(Origin::signed(1), 4)); + // Still can't chill these users + assert_noop!( + Staking::chill_other(Origin::signed(1337), 1), + >::CannotChillOther + ); + assert_noop!( + Staking::chill_other(Origin::signed(1337), 3), + >::CannotChillOther + ); + + // Add threshold, but no limits + assert_ok!(Staking::set_staking_limits( + Origin::root(), + 1_500, + 2_000, + None, + None, + Some(Percent::from_percent(0)) + )); + + // Still can't chill these users + assert_noop!( + Staking::chill_other(Origin::signed(1337), 1), + >::CannotChillOther + ); + assert_noop!( + Staking::chill_other(Origin::signed(1337), 3), + >::CannotChillOther + ); + + // Add threshold and limits + assert_ok!(Staking::set_staking_limits( + Origin::root(), + 1_500, + 2_000, + Some(10), + Some(10), + Some(Percent::from_percent(75)) + )); + + // 16 people total because tests start with 1 active one + assert_eq!(>::get(), 16); + assert_eq!(>::get(), 16); + + // Users can now be chilled down to 7 people, so we try to remove 9 of them (starting with 16) + for i in 6..15 { + let b = 4 * i + 1; + let d = 4 * i + 3; + assert_ok!(Staking::chill_other(Origin::signed(1337), b)); + assert_ok!(Staking::chill_other(Origin::signed(1337), d)); + } + + // Cant go lower. + assert_noop!( + Staking::chill_other(Origin::signed(1337), 1), + >::CannotChillOther + ); + assert_noop!( + Staking::chill_other(Origin::signed(1337), 3), + >::CannotChillOther + ); }) } @@ -4895,51 +4992,87 @@ mod election_data_provider { // Change the maximums let max = 10; - assert_ok!(Staking::update_staking_limits( + assert_ok!(Staking::set_staking_limits( Origin::root(), 10, 10, Some(max), - Some(max) + Some(max), + Some(Percent::from_percent(0)) )); // can create `max - validator_count` validators - assert_ok!(create_validators::(max - validator_count, 100)); + let mut some_existing_validator = AccountId::default(); + for i in 0..max - validator_count { + let (_, controller) = testing_utils::create_stash_controller::( + i + 10_000_000, + 100, + RewardDestination::Controller, + ) + .unwrap(); + assert_ok!(Staking::validate( + Origin::signed(controller), + ValidatorPrefs::default() + )); + some_existing_validator = controller; + } // but no more - let (_, last_validator) = - create_stash_controller::(1337, 100, RewardDestination::Controller).unwrap(); + let (_, last_validator) = testing_utils::create_stash_controller::( + 1337, + 100, + RewardDestination::Controller, + ) + .unwrap(); + assert_noop!( Staking::validate(Origin::signed(last_validator), ValidatorPrefs::default()), >::TooManyValidators, ); // same with nominators + let mut some_existing_nominator = AccountId::default(); for i in 0..max - nominator_count { - let (_, controller) = create_stash_controller::( - i + 10_000_000, + let (_, controller) = testing_utils::create_stash_controller::( + i + 20_000_000, 100, RewardDestination::Controller, ) .unwrap(); assert_ok!(Staking::nominate(Origin::signed(controller), vec![1])); + some_existing_nominator = controller; } // one more is too many - let (_, last_nominator) = - create_stash_controller::(20_000_000, 100, RewardDestination::Controller) - .unwrap(); + let (_, last_nominator) = testing_utils::create_stash_controller::( + 30_000_000, + 100, + RewardDestination::Controller, + ) + .unwrap(); assert_noop!( Staking::nominate(Origin::signed(last_nominator), vec![1]), >::TooManyNominators ); + // Re-nominate works fine + assert_ok!(Staking::nominate( + Origin::signed(some_existing_nominator), + vec![1] + )); + // Re-validate works fine + assert_ok!(Staking::validate( + Origin::signed(some_existing_validator), + ValidatorPrefs::default() + )); + // No problem when we set to `None` again - assert_ok!(Staking::update_staking_limits( + assert_ok!(Staking::set_staking_limits( Origin::root(), 10, 10, None, + None, None )); assert_ok!(Staking::nominate(Origin::signed(last_nominator), vec![1])); diff --git a/frame/staking/src/weights.rs b/frame/staking/src/weights.rs index 4380a63bb3..610802ed53 100644 --- a/frame/staking/src/weights.rs +++ b/frame/staking/src/weights.rs @@ -56,7 +56,7 @@ pub trait WeightInfo { fn new_era(v: u32, n: u32) -> Weight; fn get_npos_voters(v: u32, n: u32, s: u32) -> Weight; fn get_npos_targets(v: u32) -> Weight; - fn update_staking_limits() -> Weight; + fn set_staking_limits() -> Weight; fn chill_other() -> Weight; } @@ -233,7 +233,7 @@ impl WeightInfo for SubstrateWeight { .saturating_add(T::DbWeight::get().reads(1 as Weight)) .saturating_add(T::DbWeight::get().reads((1 as Weight).saturating_mul(v as Weight))) } - fn update_staking_limits() -> Weight { + fn set_staking_limits() -> Weight { (6_398_000 as Weight).saturating_add(T::DbWeight::get().writes(4 as Weight)) } fn chill_other() -> Weight { @@ -415,7 +415,7 @@ impl WeightInfo for () { .saturating_add(RocksDbWeight::get().reads(1 as Weight)) .saturating_add(RocksDbWeight::get().reads((1 as Weight).saturating_mul(v as Weight))) } - fn update_staking_limits() -> Weight { + fn set_staking_limits() -> Weight { (6_398_000 as Weight).saturating_add(RocksDbWeight::get().writes(4 as Weight)) } fn chill_other() -> Weight {