diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index f7545b07c90a8..ff7be272eec81 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -601,26 +601,33 @@ benchmarks! { assert_eq!(targets.len() as u32, v); } - update_staking_limits { + set_staking_limits { // This function always does the same thing... just write to 4 storage items. }: _( RawOrigin::Root, BalanceOf::::max_value(), BalanceOf::::max_value(), Some(u32::MAX), - Some(u32::MAX) + Some(u32::MAX), + Some(Percent::max_value()) ) verify { assert_eq!(MinNominatorBond::::get(), BalanceOf::::max_value()); assert_eq!(MinValidatorBond::::get(), BalanceOf::::max_value()); assert_eq!(MaxNominatorsCount::::get(), Some(u32::MAX)); assert_eq!(MaxValidatorsCount::::get(), Some(u32::MAX)); + assert_eq!(ChillThreshold::::get(), Some(Percent::from_percent(100))); } chill_other { let (_, controller) = create_stash_controller::(USER_SEED, 100, Default::default())?; Staking::::validate(RawOrigin::Signed(controller.clone()).into(), ValidatorPrefs::default())?; - Staking::::update_staking_limits( - RawOrigin::Root.into(), BalanceOf::::max_value(), BalanceOf::::max_value(), None, None, + Staking::::set_staking_limits( + RawOrigin::Root.into(), + BalanceOf::::max_value(), + BalanceOf::::max_value(), + Some(0), + Some(0), + Some(Percent::from_percent(0)) )?; let caller = whitelisted_caller(); }: _(RawOrigin::Signed(caller), controller.clone()) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index ce1f5afc64c1d..ec7da1be18714 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -1216,6 +1216,12 @@ pub mod pallet { #[pallet::storage] pub(crate) 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(crate) type ChillThreshold = StorageValue<_, Percent, OptionQuery>; + #[pallet::genesis_config] pub struct GenesisConfig { pub history_depth: u32, @@ -1714,16 +1720,19 @@ pub mod pallet { 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) = MaxValidatorsCount::::get() { - ensure!(CounterForValidators::::get() < max_validators, Error::::TooManyValidators); - } - let ledger = Self::ledger(&controller).ok_or(Error::::NotController)?; ensure!(ledger.active >= MinValidatorBond::::get(), Error::::InsufficientBond); - let stash = &ledger.stash; + + // Only check limits if they are not already a validator. + if !Validators::::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) = MaxValidatorsCount::::get() { + ensure!(CounterForValidators::::get() < max_validators, Error::::TooManyValidators); + } + } + Self::do_remove_nominator(stash); Self::do_add_validator(stash, prefs); Ok(()) @@ -1755,16 +1764,19 @@ pub mod pallet { ) -> 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) = MaxNominatorsCount::::get() { - ensure!(CounterForNominators::::get() < max_nominators, Error::::TooManyNominators); - } - let ledger = Self::ledger(&controller).ok_or(Error::::NotController)?; ensure!(ledger.active >= MinNominatorBond::::get(), Error::::InsufficientBond); - let stash = &ledger.stash; + + // Only check limits if they are not already a nominator. + if !Nominators::::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) = MaxNominatorsCount::::get() { + ensure!(CounterForNominators::::get() < max_nominators, Error::::TooManyNominators); + } + } + ensure!(!targets.is_empty(), Error::::EmptyTargets); ensure!(targets.len() <= T::MAX_NOMINATIONS as usize, Error::::TooManyTargets); @@ -2266,31 +2278,42 @@ 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: BalanceOf, min_validator_bond: BalanceOf, max_nominator_count: Option, max_validator_count: Option, + threshold: Option, ) -> DispatchResult { ensure_root(origin)?; MinNominatorBond::::set(min_nominator_bond); MinValidatorBond::::set(min_validator_bond); MaxNominatorsCount::::set(max_nominator_count); MaxValidatorsCount::::set(max_validator_count); + ChillThreshold::::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. @@ -2307,14 +2330,27 @@ pub mod pallet { let ledger = Self::ledger(&controller).ok_or(Error::::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 = ChillThreshold::::get().ok_or(Error::::CannotChillOther)?; let min_active_bond = if Nominators::::contains_key(&stash) { + let max_nominator_count = MaxNominatorsCount::::get().ok_or(Error::::CannotChillOther)?; + let current_nominator_count = CounterForNominators::::get(); + ensure!(threshold * max_nominator_count < current_nominator_count, Error::::CannotChillOther); MinNominatorBond::::get() } else if Validators::::contains_key(&stash) { + let max_validator_count = MaxValidatorsCount::::get().ok_or(Error::::CannotChillOther)?; + let current_validator_count = CounterForValidators::::get(); + ensure!(threshold * max_validator_count < current_validator_count, Error::::CannotChillOther); MinValidatorBond::::get() } else { Zero::zero() diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index e314a70399fdd..bbb0d5522fcc6 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -4050,12 +4050,18 @@ mod election_data_provider { // 500 is not enough for any role assert_ok!(Staking::bond(Origin::signed(3), 4, 500, RewardDestination::Controller)); assert_noop!(Staking::nominate(Origin::signed(4), vec![1]), Error::::InsufficientBond); - assert_noop!(Staking::validate(Origin::signed(4), ValidatorPrefs::default()), Error::::InsufficientBond); + assert_noop!( + Staking::validate(Origin::signed(4), ValidatorPrefs::default()), + Error::::InsufficientBond, + ); // 1000 is enough for nominator assert_ok!(Staking::bond_extra(Origin::signed(3), 500)); assert_ok!(Staking::nominate(Origin::signed(4), vec![1])); - assert_noop!(Staking::validate(Origin::signed(4), ValidatorPrefs::default()), Error::::InsufficientBond); + assert_noop!( + Staking::validate(Origin::signed(4), ValidatorPrefs::default()), + Error::::InsufficientBond, + ); // 1500 is enough for validator assert_ok!(Staking::bond_extra(Origin::signed(3), 500)); @@ -4083,24 +4089,80 @@ 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, 1000, RewardDestination::Controller)); - 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; + Balances::make_free_balance_be(&a, 100_000); + Balances::make_free_balance_be(&b, 100_000); + Balances::make_free_balance_be(&c, 100_000); + Balances::make_free_balance_be(&d, 100_000); + + // Nominator + assert_ok!(Staking::bond(Origin::signed(a), b, 1000, RewardDestination::Controller)); + assert_ok!(Staking::nominate(Origin::signed(b), vec![1])); + + // Validator + assert_ok!(Staking::bond(Origin::signed(c), d, 1500, RewardDestination::Controller)); + assert_ok!(Staking::validate(Origin::signed(d), ValidatorPrefs::default())); + } - // Validator - assert_ok!(Staking::bond(Origin::signed(3), 4, 1500, RewardDestination::Controller)); - 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), Error::::CannotChillOther); - assert_noop!(Staking::chill_other(Origin::signed(1), 4), Error::::CannotChillOther); - - // Change the minimum bond - assert_ok!(Staking::update_staking_limits(Origin::root(), 1_500, 2_000, None, None)); + assert_noop!(Staking::chill_other(Origin::signed(1337), 1), Error::::CannotChillOther); + assert_noop!(Staking::chill_other(Origin::signed(1337), 3), Error::::CannotChillOther); + + // 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), Error::::CannotChillOther); + assert_noop!(Staking::chill_other(Origin::signed(1337), 3), Error::::CannotChillOther); + + // Add limits, but no threshold + assert_ok!(Staking::set_staking_limits(Origin::root(), 1_500, 2_000, Some(10), Some(10), None)); + + // Still can't chill these users + assert_noop!(Staking::chill_other(Origin::signed(1337), 1), Error::::CannotChillOther); + assert_noop!(Staking::chill_other(Origin::signed(1337), 3), Error::::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), Error::::CannotChillOther); + assert_noop!(Staking::chill_other(Origin::signed(1337), 3), Error::::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!(CounterForNominators::::get(), 16); + assert_eq!(CounterForValidators::::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)); + } - // Users can now be chilled - assert_ok!(Staking::chill_other(Origin::signed(1), 2)); - assert_ok!(Staking::chill_other(Origin::signed(1), 4)); + // Cant go lower. + assert_noop!(Staking::chill_other(Origin::signed(1337), 1), Error::::CannotChillOther); + assert_noop!(Staking::chill_other(Origin::signed(1337), 3), Error::::CannotChillOther); }) } @@ -4114,36 +4176,53 @@ mod election_data_provider { // Change the maximums let max = 10; - assert_ok!(Staking::update_staking_limits(Origin::root(), 10, 10, Some(max), Some(max))); + assert_ok!(Staking::set_staking_limits( + Origin::root(), 10, 10, Some(max), Some(max), Some(Percent::from_percent(0)) + )); // can create `max - validator_count` validators - assert_ok!(testing_utils::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) = testing_utils::create_stash_controller::( 1337, 100, RewardDestination::Controller, ).unwrap(); + assert_noop!( Staking::validate(Origin::signed(last_validator), ValidatorPrefs::default()), Error::::TooManyValidators, ); // same with nominators + let mut some_existing_nominator = AccountId::default(); for i in 0 .. max - nominator_count { let (_, controller) = testing_utils::create_stash_controller::( - i + 10_000_000, 100, RewardDestination::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) = testing_utils::create_stash_controller::( - 20_000_000, 100, RewardDestination::Controller, + 30_000_000, 100, RewardDestination::Controller, ).unwrap(); assert_noop!(Staking::nominate(Origin::signed(last_nominator), vec![1]), Error::::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(Origin::root(), 10, 10, None, None)); + assert_ok!(Staking::set_staking_limits(Origin::root(), 10, 10, None, None, None)); assert_ok!(Staking::nominate(Origin::signed(last_nominator), vec![1])); assert_ok!(Staking::validate(Origin::signed(last_validator), ValidatorPrefs::default())); }) diff --git a/frame/staking/src/weights.rs b/frame/staking/src/weights.rs index dbf5f3fc82bf9..cf14e8b22362f 100644 --- a/frame/staking/src/weights.rs +++ b/frame/staking/src/weights.rs @@ -70,7 +70,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; } @@ -252,7 +252,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 { (5_028_000 as Weight) .saturating_add(T::DbWeight::get().writes(4 as Weight)) } @@ -440,7 +440,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 { (5_028_000 as Weight) .saturating_add(RocksDbWeight::get().writes(4 as Weight)) }