From bff8df5fb854a73756eebae6c991fb2f8fd16475 Mon Sep 17 00:00:00 2001 From: gpestana Date: Thu, 8 Dec 2022 23:39:47 +0100 Subject: [PATCH 1/3] Staking: store last min-active-bond on-chain Storing the `min-active-bond` onchain helps the UIs with minimal on-chain costs. Closes https://github.com/paritytech/substrate/issues/12746 --- frame/staking/src/pallet/impls.rs | 8 ++++++++ frame/staking/src/pallet/mod.rs | 4 ++++ frame/staking/src/tests.rs | 18 ++++++++++++++++++ 3 files changed, 30 insertions(+) diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 34e12fbcf6adf..c3a2480da0d03 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -704,6 +704,8 @@ impl Pallet { /// /// `maybe_max_len` can imposes a cap on the number of voters returned; /// + /// Sets `LastMinimumActiveBond` to the minimum active bond in the returned set of nominators. + /// /// This function is self-weighing as [`DispatchClass::Mandatory`]. /// /// ### Slashing @@ -793,6 +795,12 @@ impl Pallet { nominators_taken ); + // `all_voters` is sorted by vote weight, so we use the last element to set the current + // minimum active bond. Note that depending on the `T::VoterList` list, the sorting may be + // inconsistent as the sorting may be lazy. + let min_active_bond: T::CurrencyBalance = all_voters.last().map_or(0, |v| v.1).into(); + LastMinimumActiveBond::::put(min_active_bond); + all_voters } diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index fd0c494fa6723..be497bb6153f3 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -295,6 +295,10 @@ pub mod pallet { #[pallet::storage] pub type MinValidatorBond = StorageValue<_, BalanceOf, ValueQuery>; + /// The minimum active bond of the last successful election. + #[pallet::storage] + pub type LastMinimumActiveBond = StorageValue<_, BalanceOf, ValueQuery>; + /// The minimum amount of commission that validators can set. /// /// If set to `0`, no limit exists. diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 78429122d00f1..05281af9e2c4d 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -4398,6 +4398,24 @@ mod election_data_provider { ); } + #[test] + fn set_last_minimum_nominator_bond_is_correct() { + ExtBuilder::default() + .nominate(false) + .add_staker(61, 60, 2_000, StakerStatus::::Nominator(vec![21])) + .add_staker(71, 70, 10, StakerStatus::::Nominator(vec![21])) + .add_staker(81, 80, 50, StakerStatus::::Nominator(vec![21])) + .build_and_execute(|| { + assert_ok!(::electing_voters(None)); + assert_eq!(LastMinimumActiveBond::::get(), 10); + + // remove staker with lower bond by limiting the number of voters and check + // `LastMinimumActiveBond` again after electing voters. + assert_ok!(::electing_voters(Some(5))); + assert_eq!(LastMinimumActiveBond::::get(), 50); + }); + } + #[test] fn voters_include_self_vote() { ExtBuilder::default().nominate(false).build_and_execute(|| { From a727d63c77aaef01567093bb2443a15ef2f6c107 Mon Sep 17 00:00:00 2001 From: gpestana Date: Mon, 12 Dec 2022 11:58:14 +0100 Subject: [PATCH 2/3] Avoid relying on sorting to set the --- frame/staking/src/pallet/impls.rs | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index c3a2480da0d03..cd15b2bb96836 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -729,6 +729,7 @@ impl Pallet { let mut nominators_taken = 0u32; let mut sorted_voters = T::VoterList::iter(); + let mut min_active_bond = None; while all_voters.len() < max_allowed_len && voters_seen < (NPOS_MAX_ITERATIONS_COEFFICIENT * max_allowed_len as u32) { @@ -749,10 +750,19 @@ impl Pallet { .get(stash) .map_or(true, |spans| submitted_in >= spans.last_nonzero_slash()) }); + let voter_weight = weight_of(&voter); if !targets.len().is_zero() { - all_voters.push((voter.clone(), weight_of(&voter), targets)); + all_voters.push((voter.clone(), voter_weight, targets)); nominators_taken.saturating_inc(); } + + min_active_bond = min_active_bond.map_or(Some(voter_weight), |current_min| { + if voter_weight < current_min { + Some(voter_weight) + } else { + Some(current_min) + } + }); } else if Validators::::contains_key(&voter) { // if this voter is a validator: let self_vote = ( @@ -787,6 +797,10 @@ impl Pallet { slashing_spans.len() as u32, )); + LastMinimumActiveBond::::put::( + min_active_bond.map_or(0, |v| v).into(), + ); + log!( info, "generated {} npos voters, {} from validators and {} nominators", @@ -795,12 +809,6 @@ impl Pallet { nominators_taken ); - // `all_voters` is sorted by vote weight, so we use the last element to set the current - // minimum active bond. Note that depending on the `T::VoterList` list, the sorting may be - // inconsistent as the sorting may be lazy. - let min_active_bond: T::CurrencyBalance = all_voters.last().map_or(0, |v| v.1).into(); - LastMinimumActiveBond::::put(min_active_bond); - all_voters } From 9ac31d467b77080c9c44fe6a4a9e7114f9719060 Mon Sep 17 00:00:00 2001 From: gpestana Date: Tue, 13 Dec 2022 09:24:19 +0100 Subject: [PATCH 3/3] Addresses PR comments --- frame/staking/src/pallet/impls.rs | 21 +++++++++------------ frame/staking/src/pallet/mod.rs | 4 ++-- frame/staking/src/tests.rs | 16 ++++++++++++---- 3 files changed, 23 insertions(+), 18 deletions(-) diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index cd15b2bb96836..99bc98f9a14e5 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -704,7 +704,8 @@ impl Pallet { /// /// `maybe_max_len` can imposes a cap on the number of voters returned; /// - /// Sets `LastMinimumActiveBond` to the minimum active bond in the returned set of nominators. + /// Sets `MinimumActiveStake` to the minimum active nominator stake in the returned set of + /// nominators. /// /// This function is self-weighing as [`DispatchClass::Mandatory`]. /// @@ -727,9 +728,9 @@ impl Pallet { let mut voters_seen = 0u32; let mut validators_taken = 0u32; let mut nominators_taken = 0u32; + let mut min_active_stake = u64::MAX; let mut sorted_voters = T::VoterList::iter(); - let mut min_active_bond = None; while all_voters.len() < max_allowed_len && voters_seen < (NPOS_MAX_ITERATIONS_COEFFICIENT * max_allowed_len as u32) { @@ -756,13 +757,8 @@ impl Pallet { nominators_taken.saturating_inc(); } - min_active_bond = min_active_bond.map_or(Some(voter_weight), |current_min| { - if voter_weight < current_min { - Some(voter_weight) - } else { - Some(current_min) - } - }); + min_active_stake = + if voter_weight < min_active_stake { voter_weight } else { min_active_stake }; } else if Validators::::contains_key(&voter) { // if this voter is a validator: let self_vote = ( @@ -797,9 +793,10 @@ impl Pallet { slashing_spans.len() as u32, )); - LastMinimumActiveBond::::put::( - min_active_bond.map_or(0, |v| v).into(), - ); + let min_active_stake: T::CurrencyBalance = + if all_voters.len() == 0 { 0u64.into() } else { min_active_stake.into() }; + + MinimumActiveStake::::put(min_active_stake); log!( info, diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index be497bb6153f3..4b1622b2e6b34 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -295,9 +295,9 @@ pub mod pallet { #[pallet::storage] pub type MinValidatorBond = StorageValue<_, BalanceOf, ValueQuery>; - /// The minimum active bond of the last successful election. + /// The minimum active nominator stake of the last successful election. #[pallet::storage] - pub type LastMinimumActiveBond = StorageValue<_, BalanceOf, ValueQuery>; + pub type MinimumActiveStake = StorageValue<_, BalanceOf, ValueQuery>; /// The minimum amount of commission that validators can set. /// diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 05281af9e2c4d..b4560df69d4f7 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -4399,7 +4399,7 @@ mod election_data_provider { } #[test] - fn set_last_minimum_nominator_bond_is_correct() { + fn set_minimum_active_stake_is_correct() { ExtBuilder::default() .nominate(false) .add_staker(61, 60, 2_000, StakerStatus::::Nominator(vec![21])) @@ -4407,15 +4407,23 @@ mod election_data_provider { .add_staker(81, 80, 50, StakerStatus::::Nominator(vec![21])) .build_and_execute(|| { assert_ok!(::electing_voters(None)); - assert_eq!(LastMinimumActiveBond::::get(), 10); + assert_eq!(MinimumActiveStake::::get(), 10); // remove staker with lower bond by limiting the number of voters and check - // `LastMinimumActiveBond` again after electing voters. + // `MinimumActiveStake` again after electing voters. assert_ok!(::electing_voters(Some(5))); - assert_eq!(LastMinimumActiveBond::::get(), 50); + assert_eq!(MinimumActiveStake::::get(), 50); }); } + #[test] + fn set_minimum_active_stake_zero_correct() { + ExtBuilder::default().has_stakers(false).build_and_execute(|| { + assert_ok!(::electing_voters(None)); + assert_eq!(MinimumActiveStake::::get(), 0); + }); + } + #[test] fn voters_include_self_vote() { ExtBuilder::default().nominate(false).build_and_execute(|| {