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

Improve Staking Limits #9193

Merged
8 commits merged into from
Jun 28, 2021
+172 −50
Merged

Improve Staking Limits #9193

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4ad91dd
only allow `chill_other` near threshold.
shawntabrizi Jun 24, 2021
5b5375e
improve test
shawntabrizi Jun 24, 2021
9d137e8
skip limit check for existing validators / nominators
shawntabrizi Jun 24, 2021
f7c1698
add `ChillThreshold`
shawntabrizi Jun 27, 2021
4723d5a
rename to `set` for consistent api
shawntabrizi Jun 27, 2021
1778970
more tests
shawntabrizi Jun 27, 2021
0a15b45
Merge branch 'master' into shawntabrizi-improve-staking-limits
shawntabrizi Jun 27, 2021
aa5fb4e
fix some line width
shawntabrizi Jun 28, 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
15 changes: 11 additions & 4 deletions frame/staking/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
shawntabrizi marked this conversation as resolved.
Show resolved Hide resolved
Some(Percent::from_percent(0))
)?;
let caller = whitelisted_caller();
}: _(RawOrigin::Signed(caller), controller.clone())
Expand Down
80 changes: 58 additions & 22 deletions frame/staking/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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(())
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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.
Expand All @@ -2307,14 +2330,27 @@ pub mod pallet {
let ledger = Self::ledger(&controller).ok_or(Error::<T>::NotController)?;
Copy link
Contributor

@emostov emostov Jun 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Should the origin param for chill_other be configurable for the pallet? (I believe @shawntabrizi mentioned this idea already somewhere)

The current, permission nature of this call is useful for scenarios where we want a network to be continuously "self adjusting". But I think we can imagine scenarios where this makes more sense as a governance only call (that then can be turned into a permission-less call with a runtime upgrade).

While I don't see how it would improve security properties, I think having a call like this go through governance could benefit the community

Copy link
Contributor

Choose a reason for hiding this comment

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

Tagential, is it possible to make a call origin configurable without a runtime upgrade?

Copy link
Contributor

Choose a reason for hiding this comment

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

Tagential, is it possible to make a call origin configurable without a runtime upgrade?

Nope, that's logic change and needs altering :code.

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()
Expand Down
121 changes: 100 additions & 21 deletions frame/staking/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down Expand Up @@ -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::<Test>::CannotChillOther);
assert_noop!(Staking::chill_other(Origin::signed(1), 4), Error::<Test>::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::<Test>::CannotChillOther);
assert_noop!(Staking::chill_other(Origin::signed(1337), 3), Error::<Test>::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::<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));
}

// 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::<Test>::CannotChillOther);
assert_noop!(Staking::chill_other(Origin::signed(1337), 3), Error::<Test>::CannotChillOther);
})
}

Expand All @@ -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()));
})
Expand Down
6 changes: 3 additions & 3 deletions frame/staking/src/weights.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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))
}
Expand Down Expand Up @@ -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))
}
Expand Down