Improve Staking Limits (#9193)

* only allow `chill_other` near threshold.

* improve test

* skip limit check for existing validators / nominators

* add `ChillThreshold`

* rename to `set` for consistent api

* more tests

* fix some line width

substrate/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::<T>::max_value(),
 		BalanceOf::<T>::max_value(),
 		Some(u32::MAX),
-		Some(u32::MAX)
+		Some(u32::MAX),
+		Some(Percent::max_value())
 	) verify {
 		assert_eq!(MinNominatorBond::<T>::get(), BalanceOf::<T>::max_value());
 		assert_eq!(MinValidatorBond::<T>::get(), BalanceOf::<T>::max_value());
 		assert_eq!(MaxNominatorsCount::<T>::get(), Some(u32::MAX));
 		assert_eq!(MaxValidatorsCount::<T>::get(), Some(u32::MAX));
+		assert_eq!(ChillThreshold::<T>::get(), Some(Percent::from_percent(100)));
 	}
 
 	chill_other {
 		let (_, controller) = create_stash_controller::<T>(USER_SEED, 100, Default::default())?;
 		Staking::<T>::validate(RawOrigin::Signed(controller.clone()).into(), ValidatorPrefs::default())?;
-		Staking::<T>::update_staking_limits(
-			RawOrigin::Root.into(), BalanceOf::<T>::max_value(), BalanceOf::<T>::max_value(), None, None,
+		Staking::<T>::set_staking_limits(
+			RawOrigin::Root.into(),
+			BalanceOf::<T>::max_value(),
+			BalanceOf::<T>::max_value(),
+			Some(0),
+			Some(0),
+			Some(Percent::from_percent(0))
 		)?;
 		let caller = whitelisted_caller();
 	}: _(RawOrigin::Signed(caller), controller.clone())

substrate/frame/staking/src/lib.rs

@@ -1216,6 +1216,12 @@ pub mod pallet {
 	#[pallet::storage]
 	pub(crate) type StorageVersion<T: Config> = 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<T: Config> = StorageValue<_, Percent, OptionQuery>;
+
 	#[pallet::genesis_config]
 	pub struct GenesisConfig<T: Config> {
 		pub history_depth: u32,
@@ -1714,16 +1720,19 @@ pub mod pallet {
 		pub fn validate(origin: OriginFor<T>, 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::<T>::get() {
-				ensure!(CounterForValidators::<T>::get() < max_validators, Error::<T>::TooManyValidators);
-			}
-
 			let ledger = Self::ledger(&controller).ok_or(Error::<T>::NotController)?;
 			ensure!(ledger.active >= MinValidatorBond::<T>::get(), Error::<T>::InsufficientBond);
-
 			let stash = &ledger.stash;
+
+			// Only check limits if they are not already a validator.
+			if !Validators::<T>::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::<T>::get() {
+					ensure!(CounterForValidators::<T>::get() < max_validators, Error::<T>::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::<T>::get() {
-				ensure!(CounterForNominators::<T>::get() < max_nominators, Error::<T>::TooManyNominators);
-			}
-
 			let ledger = Self::ledger(&controller).ok_or(Error::<T>::NotController)?;
 			ensure!(ledger.active >= MinNominatorBond::<T>::get(), Error::<T>::InsufficientBond);
-
 			let stash = &ledger.stash;
+
+			// Only check limits if they are not already a nominator.
+			if !Nominators::<T>::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::<T>::get() {
+					ensure!(CounterForNominators::<T>::get() < max_nominators, Error::<T>::TooManyNominators);
+				}
+			}
+
 			ensure!(!targets.is_empty(), Error::<T>::EmptyTargets);
 			ensure!(targets.len() <= T::MAX_NOMINATIONS as usize, Error::<T>::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<T>,
 			min_nominator_bond: BalanceOf<T>,
 			min_validator_bond: BalanceOf<T>,
 			max_nominator_count: Option<u32>,
 			max_validator_count: Option<u32>,
+			threshold: Option<Percent>,
 		) -> DispatchResult {
 			ensure_root(origin)?;
 			MinNominatorBond::<T>::set(min_nominator_bond);
 			MinValidatorBond::<T>::set(min_validator_bond);
 			MaxNominatorsCount::<T>::set(max_nominator_count);
 			MaxValidatorsCount::<T>::set(max_validator_count);
+			ChillThreshold::<T>::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::<T>::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::<T>::get().ok_or(Error::<T>::CannotChillOther)?;
 				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();
+					ensure!(threshold * max_nominator_count < current_nominator_count, Error::<T>::CannotChillOther);
 					MinNominatorBond::<T>::get()
 				} 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();
+					ensure!(threshold * max_validator_count < current_validator_count, Error::<T>::CannotChillOther);
 					MinValidatorBond::<T>::get()
 				} else {
 					Zero::zero()

substrate/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::<Test>::InsufficientBond);
-				assert_noop!(Staking::validate(Origin::signed(4), ValidatorPrefs::default()), Error::<Test>::InsufficientBond);
+				assert_noop!(
+					Staking::validate(Origin::signed(4), ValidatorPrefs::default()),
+					Error::<Test>::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::<Test>::InsufficientBond);
+				assert_noop!(
+					Staking::validate(Origin::signed(4), ValidatorPrefs::default()),
+					Error::<Test>::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);
 
-				// Validator
-				assert_ok!(Staking::bond(Origin::signed(3), 4, 1500, RewardDestination::Controller));
-				assert_ok!(Staking::validate(Origin::signed(4), ValidatorPrefs::default()));
+					// 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()));
+				}
+
+				// 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::<Test>::CannotChillOther);
-				assert_noop!(Staking::chill_other(Origin::signed(1), 4), Error::<Test>::CannotChillOther);
+				assert_noop!(Staking::chill_other(Origin::signed(1337), 1), Error::<Test>::CannotChillOther);
+				assert_noop!(Staking::chill_other(Origin::signed(1337), 3), Error::<Test>::CannotChillOther);
 
-				// Change the minimum bond
-				assert_ok!(Staking::update_staking_limits(Origin::root(), 1_500, 2_000, None, None));
+				// Change the minimum bond... but no limits.
+				assert_ok!(Staking::set_staking_limits(Origin::root(), 1_500, 2_000, None, None, 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), Error::<Test>::CannotChillOther);
+				assert_noop!(Staking::chill_other(Origin::signed(1337), 3), Error::<Test>::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::<Test>::CannotChillOther);
+				assert_noop!(Staking::chill_other(Origin::signed(1337), 3), Error::<Test>::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::<Test>::CannotChillOther);
+				assert_noop!(Staking::chill_other(Origin::signed(1337), 3), Error::<Test>::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::<Test>::get(), 16);
+				assert_eq!(CounterForValidators::<Test>::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), Error::<Test>::CannotChillOther);
+				assert_noop!(Staking::chill_other(Origin::signed(1337), 3), Error::<Test>::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::<Test>(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::<Test>(
+					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::<Test>(
 				1337, 100, RewardDestination::Controller,
 			).unwrap();
+
 			assert_noop!(
 				Staking::validate(Origin::signed(last_validator), ValidatorPrefs::default()),
 				Error::<Test>::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::<Test>(
-					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::<Test>(
-				20_000_000, 100, RewardDestination::Controller,
+				30_000_000, 100, RewardDestination::Controller,
 			).unwrap();
 			assert_noop!(Staking::nominate(Origin::signed(last_nominator), vec![1]), Error::<Test>::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()));
 		})

substrate/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<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 			.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))
 	}