From b7b641317c2e91b62fbe499f0706962c6a1e7492 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Sun, 26 Nov 2023 13:08:10 +0100 Subject: [PATCH 1/2] Refactors target and voters list to include idle stakers --- substrate/frame/staking/src/mock.rs | 7 +- substrate/frame/staking/src/pallet/impls.rs | 159 +++++++++++++----- substrate/frame/staking/src/pallet/mod.rs | 5 +- substrate/frame/staking/src/tests.rs | 91 +++++++--- .../frame/staking/stake-tracker/src/lib.rs | 79 ++++++--- .../frame/staking/stake-tracker/src/tests.rs | 36 +++- substrate/primitives/staking/src/lib.rs | 10 ++ 7 files changed, 288 insertions(+), 99 deletions(-) diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index 50baa12afca5..475a61b68816 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -602,7 +602,7 @@ impl ExtBuilder { let mut ext = self.build(); ext.execute_with(test); ext.execute_with(|| { - Staking::do_try_state(System::block_number()) + let _ = Staking::do_try_state(System::block_number()) .map_err(|err| println!(" 🕵️‍♂️ try_state failure: {:?}", err)) .unwrap(); }); @@ -881,7 +881,10 @@ pub(crate) fn balances(who: &AccountId) -> (Balance, Balance) { pub(crate) fn stake_tracker_sanity_tests() -> Result<(), &'static str> { use sp_staking::StakingInterface; - assert_eq!(Nominators::::count() + Validators::::count(), VoterBagsList::count()); + assert_eq!( + Nominators::::count() + Validators::::count(), + VoterBagsList::iter().filter(|v| Staking::status(&v) != Ok(StakerStatus::Idle)).count() as u32, + ); // recalculate the target's stake based on voter's nominations and compare with the score in the // target list. diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 35368c24b476..7286f37b05c7 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -318,9 +318,10 @@ impl Pallet { /// Chill a stash account. pub(crate) fn chill_stash(stash: &T::AccountId) { - let chilled_as_validator = Self::do_remove_validator(stash); - let chilled_as_nominator = Self::do_remove_nominator(stash); + let chilled_as_validator = Self::do_chill_validator(stash); + let chilled_as_nominator = Self::do_chill_nominator(stash); if chilled_as_validator || chilled_as_nominator { + debug_assert_eq!(Self::status(stash), Ok(StakerStatus::Idle)); Self::deposit_event(Event::::Chilled { stash: stash.clone() }); } } @@ -832,7 +833,9 @@ impl Pallet { let mut nominators_taken = 0u32; let mut min_active_stake = u64::MAX; - let mut sorted_voters = T::VoterList::iter(); + // voter list also contains chilled/idle voters, filter those. + let mut sorted_voters = T::VoterList::iter() + .filter(|v| Self::status(&v) != Ok(StakerStatus::Idle)); while all_voters.len() < final_predicted_len as usize && voters_seen < (NPOS_MAX_ITERATIONS_COEFFICIENT * final_predicted_len as u32) { @@ -940,7 +943,9 @@ impl Pallet { let mut all_targets = Vec::::with_capacity(final_predicted_len as usize); let mut targets_seen = 0; - let mut targets_iter = T::TargetList::iter(); + // target list also contains chilled/idle validators, filter those. + let mut targets_iter = T::TargetList::iter() + .filter(|t| Self::status(&t) != Ok(StakerStatus::Idle)); while all_targets.len() < final_predicted_len as usize && targets_seen < (NPOS_MAX_ITERATIONS_COEFFICIENT * final_predicted_len as u32) { @@ -980,28 +985,51 @@ impl Pallet { /// to `Nominators` or `VoterList` outside of this function is almost certainly /// wrong. pub fn do_add_nominator(who: &T::AccountId, nominations: Nominations) { - if !Nominators::::contains_key(who) { - // new nominator. - Nominators::::insert(who, nominations); - >>::on_nominator_add( - who, - ); - } else { - // update nominations. - let prev_nominations = Self::nominations(who).unwrap_or_default(); - Nominators::::insert(who, nominations); - >>::on_nominator_update( - who, - prev_nominations, - ); - } + + match (Nominators::::contains_key(who), T::VoterList::contains(who)) { + (false, false) => { + // new nomination + Nominators::::insert(who, nominations); + >>::on_nominator_add( + who, + ); + }, + (true, true) | (false, true) => { + // update nominations or un-chill nominator. + let prev_nominations = Self::nominations(who).unwrap_or_default(); + Nominators::::insert(who, nominations); + >>::on_nominator_update( + who, + prev_nominations, + ); + }, + (true, false) => { + defensive!("unexpected state."); + } + }; debug_assert_eq!( Nominators::::count() + Validators::::count(), - T::VoterList::count() + T::VoterList::iter().filter(|v| Self::status(&v) != Ok(StakerStatus::Idle)).count() as u32, ); } + pub(crate) fn do_chill_nominator(who: &T::AccountId) -> bool { + let outcome = if Nominators::::contains_key(who) { + >>::on_nominator_idle( + who, + Self::nominations(who).unwrap_or_default(), + ); + + Nominators::::remove(who); + true + } else { + false + }; + + outcome + } + /// This function will remove a nominator from the `Nominators` storage map, /// and `VoterList`. /// @@ -1011,20 +1039,40 @@ impl Pallet { /// `Nominators` or `VoterList` outside of this function is almost certainly /// wrong. pub fn do_remove_nominator(who: &T::AccountId) -> bool { - let outcome = if Nominators::::contains_key(who) { - >>::on_nominator_remove( - who, - Self::nominations(who).unwrap_or_default(), - ); - Nominators::::remove(who); - true - } else { - false + let nominations = Self::nominations(who).unwrap_or_default(); + + let outcome = match (Nominators::::contains_key(who), T::VoterList::contains(who)) { + // ensure that the voter is idle before removing it. + (true, true) => { + // first chill nominator. + Self::do_chill_nominator(who); + + >>::on_nominator_remove( + who, + nominations, + ); + Nominators::::remove(who); + true + }, + // nominator is idle already, remove it. + (false, true) => { + >>::on_nominator_remove( + who, + nominations, + ); + true + }, + (true, false) => { + defensive!("inconsistent state: staker is in nominators list but not in voter list"); + false + } + // nominator has been already removed. + (false, false) => false, }; debug_assert_eq!( Nominators::::count() + Validators::::count(), - T::VoterList::count() + T::VoterList::iter().filter(|v| Self::status(&v) != Ok(StakerStatus::Idle)).count() as u32, ); outcome @@ -1049,10 +1097,19 @@ impl Pallet { debug_assert_eq!( Nominators::::count() + Validators::::count(), - T::VoterList::count() + T::VoterList::iter().filter(|v| Self::status(&v) != Ok(StakerStatus::Idle)).count() as u32, ); } + pub(crate) fn do_chill_validator(who: &T::AccountId) -> bool { + if Validators::::contains_key(who) { + Validators::::remove(who); + true + } else { + false + } + } + /// This function will remove a validator from the `Validators` storage map. /// /// Returns true if `who` was removed from `Validators`, otherwise false. @@ -1061,19 +1118,35 @@ impl Pallet { /// `Validators` or `VoterList` outside of this function is almost certainly /// wrong. pub fn do_remove_validator(who: &T::AccountId) -> bool { - let outcome = if Validators::::contains_key(who) { - Validators::::remove(who); - >>::on_validator_remove( - who, - ); - true - } else { - false + let outcome = match (Validators::::contains_key(who), T::TargetList::contains(who)) { + // ensure the validator is idle before removing it. + (true, true) => { + Self::do_chill_validator(who); + + >>::on_validator_remove( + who, + ); + Validators::::remove(who); + true + }, + // validator is idle, remove it. + (false, true) => { + >>::on_validator_remove( + who, + ); + true + } + (true, false) => { + defensive!("inconsistent state: staker is in validators list but not in targets list"); + false + }, + // validator has been already removed. + (false, false) => false, }; debug_assert_eq!( Nominators::::count() + Validators::::count(), - T::VoterList::count() + T::VoterList::iter().filter(|v| Self::status(&v) != Ok(StakerStatus::Idle)).count() as u32, ); outcome @@ -1825,7 +1898,7 @@ impl Pallet { pub(crate) fn do_try_state(_: BlockNumberFor) -> Result<(), TryRuntimeError> { ensure!( T::VoterList::iter() - .all(|x| >::contains_key(&x) || >::contains_key(&x)), + .all(|x| >::contains_key(&x) || >::contains_key(&x) || Self::status(&x) == Ok(StakerStatus::Idle)), "VoterList contains non-staker" ); @@ -1837,12 +1910,12 @@ impl Pallet { fn check_count() -> Result<(), TryRuntimeError> { ensure!( - ::VoterList::count() == + ::VoterList::iter().filter(|v| Self::status(&v) != Ok(StakerStatus::Idle)).count() as u32 == Nominators::::count() + Validators::::count(), "wrong external count (VoterList.count != Nominators.count + Validators.count)" ); ensure!( - ::TargetList::count() == Validators::::count(), + ::TargetList::iter().filter(|t| Self::status(&t) != Ok(StakerStatus::Idle)).count() as u32 == Validators::::count(), "wrong external count (TargetList.count != Validators.count)" ); ensure!( diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index c17e9eb124a7..56bc6b82b9f4 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -1151,7 +1151,7 @@ pub mod pallet { } } - Self::do_remove_nominator(stash); + Self::do_chill_nominator(stash); Self::do_add_validator(stash, prefs.clone()); Self::deposit_event(Event::::ValidatorPrefsSet { stash: ledger.stash, prefs }); @@ -1225,7 +1225,7 @@ pub mod pallet { suppressed: false, }; - Self::do_remove_validator(stash); + Self::do_chill_validator(stash); Self::do_add_nominator(stash, nominations); Ok(()) @@ -1646,6 +1646,7 @@ pub mod pallet { if let Some(ref mut nom) = maybe_nom { if let Some(pos) = nom.targets.iter().position(|v| v == stash) { nom.targets.swap_remove(pos); + // update nominations and trickle down to target list score. Self::do_add_nominator(&nom_stash, nom.clone()); diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 3093d6c499df..63814e3ea08b 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -520,6 +520,7 @@ fn blocking_and_kicking_works() { RuntimeOrigin::signed(11), ValidatorPrefs { blocked: true, ..Default::default() } )); + // attempt to nominate from 100/101... assert_ok!(Staking::nominate(RuntimeOrigin::signed(101), vec![11])); // should have worked since we're already nominated them @@ -5030,6 +5031,7 @@ fn on_finalize_weight_is_nonzero() { mod sorted_list_provider_integration { use super::*; use frame_election_provider_support::ScoreProvider; + use sp_staking::StakingInterface; #[test] fn nominator_bond_unbond_chill_works() { @@ -5069,11 +5071,32 @@ mod sorted_list_provider_integration { assert_eq!(TargetBagsList::score(&11), 1520); assert_eq!(TargetBagsList::score(&21), 1520); - // finally, stash 42 chills and unbond completely. + // stash 42 chills, but remains bonded. Thus it is in idle state. assert_ok!(Staking::chill(RuntimeOrigin::signed(42))); - assert_eq!(VoterBagsList::contains(&42), false); + assert_eq!(VoterBagsList::contains(&42), true); + assert!(>::get(&42).is_some()); + assert_ok!(>::get(StakingAccount::Stash(42))); + assert_eq!(Staking::status(&42), Ok(StakerStatus::Idle)); + + // scores of previously nominated validators were updated. assert_eq!(TargetBagsList::score(&11), 1500); assert_eq!(TargetBagsList::score(&21), 1500); + + // stash 42 unbonds completely by requesting all unbonding rewards and thus its ledger + // is killed. + assert_ok!(Staking::unbond(RuntimeOrigin::signed(42), 20)); + // active bonded is 0, ledger will be killed when all the unlocking funds are withdrawn. + assert_eq!(>::get(StakingAccount::Stash(42)).unwrap().active, 0); + + // roll to block where stash can successfully withdraw unbonding chunks. + start_active_era(current_era() + BondingDuration::get()); + + // after withdrawing, the ledger is killed. + assert_ok!(Staking::withdraw_unbonded(RuntimeOrigin::signed(42), 0)); + assert!(>::get(&42).is_none()); + assert!(>::get(StakingAccount::Stash(42)).is_err()); + assert!(Staking::status(&42).is_err()); + assert_eq!(VoterBagsList::contains(&42), false); }) } @@ -5103,10 +5126,18 @@ mod sorted_list_provider_integration { assert_eq!(VoterBagsList::score(&42), 20); assert_eq!(TargetBagsList::score(&42), 20); - // finally, stash 42 chills and unbond completely. + // stash 42 chills, thus it should be part of the voter and target bags list but with + // `Idle` status. assert_ok!(Staking::chill(RuntimeOrigin::signed(42))); + assert_eq!(VoterBagsList::contains(&42), true); + assert_eq!(TargetBagsList::contains(&42), true); + assert_eq!(Staking::status(&42), Ok(StakerStatus::Idle)); + + // finally, remove the validator (similar to withdraw all and subsequent ledger kill). + assert_ok!(Staking::kill_stash(&42, 0)); assert_eq!(VoterBagsList::contains(&42), false); assert_eq!(TargetBagsList::contains(&42), false); + assert!(Staking::status(&42).is_err()); }) } } @@ -7272,7 +7303,6 @@ mod stake_tracker { // * Call::validate() // * Call::nominate() // * Call::chill() - // * Call::kick() ExtBuilder::default().build_and_execute(|| { // bond extra to rebag nominated validator. assert_ok!(Staking::bond_extra(RuntimeOrigin::signed(101), 600)); @@ -7320,25 +7350,33 @@ mod stake_tracker { // let's chill a validator now. assert_ok!(Staking::chill(RuntimeOrigin::signed(11))); - // the chilled validator score is updated to 0 and 11 is not part of the targets list - // anymore. + // the chilled validator score remains the same, 11 is still part of the targets list + // but its staker status is Idle and it was removed from the nominator sand validators + // list. assert_eq!(Staking::status(&11), Ok(StakerStatus::Idle)); - assert_eq!(>::score(&11), 0); - assert_eq!(voters_and_targets().1, [(21, 1000), (31, 500)]); + assert_eq!(>::score(&11), 1000); + assert_eq!(voters_and_targets().1, [(11, 1000), (21, 1000), (31, 500)]); + assert!(!Nominators::::contains_key(&11)); + assert!(!Validators::::contains_key(&11)); - // now, let's have 101 re-nominate 21 and kick him. Note that 101 also nominates 11 but - // that's a noop since 11 is Idle at the moment. + // now, let's have 101 re-nominate 21. Note that 101 also nominates 11: even + // though 11 is chilled at the moment. assert_ok!(Staking::nominate(RuntimeOrigin::signed(101), vec![21, 11])); - assert_eq!(voters_and_targets().1, [(21, 2100), (31, 500)]); + assert_eq!(voters_and_targets().1, [(21, 2100), (11, 2100), (31, 500)]); - // score of 21 and rebag hapened due to nomination. + // score update and rebag hapened to 21 and 11 (idle) due to nomination of 101. assert_eq!( target_bags_events(), [ BagsEvent::Rebagged { who: 21, from: 1000, to: 10000 }, - BagsEvent::ScoreUpdated { who: 21, new_score: 2100 } + BagsEvent::ScoreUpdated { who: 21, new_score: 2100 }, + BagsEvent::Rebagged { who: 11, from: 1000, to: 10000 }, + BagsEvent::ScoreUpdated { who: 11, new_score: 2100 }, ] ); + + assert_eq!(Staking::status(&21).unwrap(), StakerStatus::Validator); + assert_eq!(Staking::status(&11).unwrap(), StakerStatus::Idle); }) } @@ -7492,15 +7530,19 @@ mod stake_tracker { // we immediately slash 11 with a 50% slash. let exposure = Staking::eras_stakers(active_era(), &11); let slash_percent = Perbill::from_percent(50); + let total_stake_to_slash = slash_percent * exposure.total; on_offence_now( &[OffenceDetails { offender: (11, exposure.clone()), reporters: vec![] }], &[slash_percent], ); - // 11 has been chilled and not part of the targets list anymore (using - // `DisablementStrategy::WhenSlashed`). - assert!(!>::contains(&11)); - assert_eq!(>::score(&11), 0); + // 11 has been chilled but it is still part of the targets list and it is in `Idle` + // state. + assert!(>::contains(&11)); + assert_eq!(Staking::status(&11), Ok(StakerStatus::Idle)); + // and its balance has been updated based on the slash applied. + let score_11_after = >::score(&11); + assert_eq!(score_11_after, score_11_before - total_stake_to_slash); // self-stake of 11 has decreased by 50% due to slash. assert_eq!( @@ -7525,15 +7567,26 @@ mod stake_tracker { ); // the target list has been updated accordingly and an indirect rebag of 21 happened. - assert_eq!(voters_and_targets().1, [(21, score_21_after), (31, 500)]); + // 11, althoug chilled, is still part of the target list. + assert_eq!(voters_and_targets().1, [(11, score_11_after), (21, score_21_after), (31, 500)]); assert_eq!( target_bags_events(), [ + BagsEvent::Rebagged { who: 11, from: 10000, to: 2000 }, + BagsEvent::ScoreUpdated { who: 11, new_score: 1550 }, + BagsEvent::ScoreUpdated { who: 11, new_score: 1465 }, BagsEvent::Rebagged { who: 21, from: 10000, to: 2000 }, BagsEvent::ScoreUpdated { who: 21, new_score: 1965 }, - BagsEvent::ScoreUpdated { who: 21, new_score: 1872 } + BagsEvent::ScoreUpdated { who: 21, new_score: 1872 }, + BagsEvent::ScoreUpdated { who: 11, new_score: 1372 }, ] ); + + // fetching targets sorted and filtered by status works. + assert_eq!( + TargetBagsList::iter().filter(|t| Staking::status(&t).unwrap() != StakerStatus::Idle).collect::>(), + [21, 31], + ); }) } } diff --git a/substrate/frame/staking/stake-tracker/src/lib.rs b/substrate/frame/staking/stake-tracker/src/lib.rs index 0992e6c5f505..1f0db3fa93bb 100644 --- a/substrate/frame/staking/stake-tracker/src/lib.rs +++ b/substrate/frame/staking/stake-tracker/src/lib.rs @@ -135,7 +135,7 @@ pub mod pallet { #[pallet::hooks] impl Hooks> for Pallet { #[cfg(feature = "try-runtime")] - fn try_state(_n: BlockNumberFor) -> Result<(), sp_runtime::TryRuntimeError> { + fn try_state(_n: BlockNumberFor) -> Result<(), TryRuntimeError> { Self::do_try_state() } } @@ -305,6 +305,7 @@ impl OnStakingUpdate> for Pallet { // // Note: it is assumed that who's staking state is updated *before* this method is called. fn on_stake_update(who: &T::AccountId, prev_stake: Option>>) { + // closure to calculate the stake imbalance of a staker. let stake_imbalance_of = |prev_stake: Option>>, voter_weight: ExtendedBalance| { @@ -352,11 +353,7 @@ impl OnStakingUpdate> for Pallet { // updates vote weight of nominated targets accordingly. Note: this will update // the score of up to `T::MaxNominations` validators. for target in nominations.into_iter() { - // target may be chilling due to a recent slash, verify if it is active - // before updating the score. - if ::is_validator(&target) { - Self::update_score::(&target, stake_imbalance); - } + Self::update_score::(&target, stake_imbalance); } }, StakerStatus::Validator => { @@ -380,9 +377,13 @@ impl OnStakingUpdate> for Pallet { fn on_nominator_add(who: &T::AccountId) { let nominator_vote = Self::weight_of(Self::active_vote_of(who)); - let _ = T::VoterList::on_insert(who.clone(), nominator_vote).defensive_proof( - "staker should not exist in VoterList, as per the contract with staking.", - ); + // voter may exist in the list in case of re-enabling a chilled nominator; + if T::VoterList::contains(who) { + return + } + + let _ = T::VoterList::on_insert(who.clone(), nominator_vote) + .defensive_proof("staker does not exist in the list as per check above; qed."); // If who is a nominator, update the vote weight of the nominations if they exist. Note: // this will update the score of up to `T::MaxNominations` validators. @@ -407,9 +408,12 @@ impl OnStakingUpdate> for Pallet { // // Note: it is assumed that who's staking state is updated *before* calling this method. fn on_validator_add(who: &T::AccountId) { - let _ = T::TargetList::on_insert(who.clone(), Self::active_vote_of(who)).defensive_proof( - "staker should not exist in TargetList, as per the contract with staking.", - ); + + // target may exist in the list in case of re-enabling a chilled validator; + if !T::TargetList::contains(who) { + let _ = T::TargetList::on_insert(who.clone(), Self::active_vote_of(who)) + .expect("staker does not exist in the list as per check above; qed."); + } log!(debug, "on_validator_add: {:?}. role: {:?}", who, T::Staking::status(who),); @@ -417,17 +421,17 @@ impl OnStakingUpdate> for Pallet { Self::on_nominator_add(who) } - // Fired when someone removes their intention to nominate, either due to chill or validating. - // - // Note: it is assumed that who's staking state is updated *before* the caller calling into - // this method. Thus, the nominations before the nominator has been removed from staking are - // passed in, so that the target list can be updated accordingly. - fn on_nominator_remove(who: &T::AccountId, nominations: Vec) { + /// Fired when some nominator becomes idle and stops nominating without being removed from the + /// staking state. + /// + /// Note: it is assumed that `who`'s staking ledger and `nominations` are up to date before + /// calling this method. + fn on_nominator_idle(who: &T::AccountId, nominations: Vec) { let nominator_vote = Self::weight_of(Self::active_vote_of(who)); log!( debug, - "on_nominator_remove: {:?} with {:?}. impacting {:?}", + "remove nominations from {:?} with {:?} weight. impacting {:?}.", who, nominator_vote, nominations, @@ -438,13 +442,27 @@ impl OnStakingUpdate> for Pallet { for t in nominations.iter() { Self::update_score::(&t, StakeImbalance::Negative(nominator_vote.into())) } + } + + // Fired when someone removes their intention to nominate and is completely removed from the + // staking state. + fn on_nominator_remove(who: &T::AccountId, nominations: Vec) { + log!(debug, "on_nominator_remove: {:?}, impacting {:?}", who, nominations); + + if T::Staking::status(who).is_ok() { + //debug_assert!(!T::VoterList::contains(who)); + + // nominator must be idle before removing completely. + Self::on_nominator_idle(who, nominations); + } let _ = T::VoterList::on_remove(&who).defensive_proof( "the nominator exists in the list as per the contract with staking; qed.", ); } - // Fired when someone removes their intention to validate, either due to chill or nominating. + // Fired when someone removes their intention to validate and is completely removed from the + // staking state. fn on_validator_remove(who: &T::AccountId) { log!(debug, "on_validator_remove: {:?}", who,); @@ -495,15 +513,24 @@ impl OnStakingUpdate> for Pallet { } } - // noop: the updates to target and voter lists when applying a slash are performed - // through [`Self::on_nominator_remove`] and [`Self::on_validator_remove`] when the stakers are - // chilled. When the slash is applied, the ledger is updated of the affected stashes is, thus - // the stake is propagated through the [`Self::update::`]. + /// Fired when a slash happens. + /// + /// In practice, this only updates the score of the slashed validators, since the score of the + /// nominators and corresponding scores are updated through the `ledger.update` calls following + /// the slash. fn on_slash( - _stash: &T::AccountId, + stash: &T::AccountId, _slashed_active: BalanceOf, _slashed_unlocking: &BTreeMap>, - _slashed_total: BalanceOf, + slashed_total: BalanceOf, ) { + let stake_imbalance = StakeImbalance::Negative(Self::to_vote_extended(slashed_total)); + + match T::Staking::status(stash).defensive_proof("called on_slash on a unbonded stash") { + Ok(StakerStatus::Idle) | Ok(StakerStatus::Validator) => Self::update_score::(stash, stake_imbalance), + // score of target impacted by nominators will be updated through ledger.update. + Ok(StakerStatus::Nominator(_)) => (), + Err(_) => (), // nothing to see here. + } } } diff --git a/substrate/frame/staking/stake-tracker/src/tests.rs b/substrate/frame/staking/stake-tracker/src/tests.rs index 8246af32e9bc..1dd17f96d341 100644 --- a/substrate/frame/staking/stake-tracker/src/tests.rs +++ b/substrate/frame/staking/stake-tracker/src/tests.rs @@ -19,7 +19,7 @@ use crate::{mock::*, StakeImbalance}; -use frame_election_provider_support::SortedListProvider; +use frame_election_provider_support::{SortedListProvider, ScoreProvider}; use frame_support::assert_ok; use sp_staking::{OnStakingUpdate, Stake, StakingInterface}; @@ -292,22 +292,44 @@ fn on_validator_add_works() { } #[test] -#[should_panic = "Defensive failure has been triggered!: Duplicate: \"staker should not exist in VoterList, as per the contract with staking.\""] -fn on_nominator_add_already_exists_defensive_works() { +fn on_nominator_add_already_exists_works() { ExtBuilder::default().populate_lists().build_and_execute(|| { - // voter already exists in the list, trying to emit `on_add_nominator` again will fail. assert!(VoterBagsList::contains(&1)); + assert_eq!(VoterBagsList::count(), 4); + assert_eq!(>::score(&1), 100); + + let voter_list_before = VoterBagsList::iter().collect::>(); + let target_list_before = TargetBagsList::iter().collect::>(); + + // noop. >::on_nominator_add(&1); + assert!(VoterBagsList::contains(&1)); + assert_eq!(VoterBagsList::count(), 4); + assert_eq!(>::score(&1), 100); + + assert_eq!(VoterBagsList::iter().collect::>(), voter_list_before); + assert_eq!(TargetBagsList::iter().collect::>(), target_list_before); }); } #[test] -#[should_panic = "Defensive failure has been triggered!: Duplicate: \"staker should not exist in TargetList, as per the contract with staking.\""] -fn on_validator_add_already_exists_defensive_works() { +fn on_validator_add_already_exists_works() { ExtBuilder::default().populate_lists().build_and_execute(|| { - // validator already exists in the list, trying to emit `on_add_validator` again will fail. assert!(TargetBagsList::contains(&10)); + assert_eq!(TargetBagsList::count(), 2); + assert_eq!(>::score(&10), 300); + + let voter_list_before = VoterBagsList::iter().collect::>(); + let target_list_before = TargetBagsList::iter().collect::>(); + + // noop >::on_validator_add(&10); + assert!(TargetBagsList::contains(&10)); + assert_eq!(TargetBagsList::count(), 2); + assert_eq!(>::score(&10), 300); + + assert_eq!(VoterBagsList::iter().collect::>(), voter_list_before); + assert_eq!(TargetBagsList::iter().collect::>(), target_list_before); }); } diff --git a/substrate/primitives/staking/src/lib.rs b/substrate/primitives/staking/src/lib.rs index c2ac5ae004b1..40ac62d674de 100644 --- a/substrate/primitives/staking/src/lib.rs +++ b/substrate/primitives/staking/src/lib.rs @@ -114,6 +114,11 @@ pub trait OnStakingUpdate { /// nominator. fn on_nominator_update(_who: &AccountId, _prev_nominations: Vec) {} + /// Fired when an existng nominator becomes idle. + /// + /// An idle nominator stops nominating but its stake state should not be removed. + fn on_nominator_idle(_who: &AccountId, _prev_nominations: Vec) {} + /// Fired when someone removes their intention to nominate, either due to chill or validating. /// /// The set of nominations at the time of removal is provided as it can no longer be fetched in @@ -130,6 +135,11 @@ pub trait OnStakingUpdate { /// Note validator preference changes are not communicated, but could be added if needed. fn on_validator_update(_who: &AccountId) {} + /// Fired when an existing validator becomes idle. + /// + /// An idle validator stops validating but its stake state should not be removed. + fn on_validator_idle(_who: &AccountId) {} + /// Fired when someone removes their intention to validate, either due to chill or nominating. fn on_validator_remove(_who: &AccountId) {} From 9f5efa14b33ac84c350129f7c137afd64978e3d1 Mon Sep 17 00:00:00 2001 From: command-bot <> Date: Sun, 26 Nov 2023 12:13:32 +0000 Subject: [PATCH 2/2] ".git/.scripts/commands/fmt/fmt.sh" --- substrate/frame/staking/src/mock.rs | 4 +- substrate/frame/staking/src/pallet/impls.rs | 53 ++++++++++++------- substrate/frame/staking/src/tests.rs | 9 +++- .../frame/staking/stake-tracker/src/lib.rs | 5 +- .../frame/staking/stake-tracker/src/tests.rs | 2 +- 5 files changed, 47 insertions(+), 26 deletions(-) diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index 475a61b68816..9c7839da10ef 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -883,7 +883,9 @@ pub(crate) fn stake_tracker_sanity_tests() -> Result<(), &'static str> { assert_eq!( Nominators::::count() + Validators::::count(), - VoterBagsList::iter().filter(|v| Staking::status(&v) != Ok(StakerStatus::Idle)).count() as u32, + VoterBagsList::iter() + .filter(|v| Staking::status(&v) != Ok(StakerStatus::Idle)) + .count() as u32, ); // recalculate the target's stake based on voter's nominations and compare with the score in the diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 7286f37b05c7..9be8acfc1520 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -834,8 +834,8 @@ impl Pallet { let mut min_active_stake = u64::MAX; // voter list also contains chilled/idle voters, filter those. - let mut sorted_voters = T::VoterList::iter() - .filter(|v| Self::status(&v) != Ok(StakerStatus::Idle)); + let mut sorted_voters = + T::VoterList::iter().filter(|v| Self::status(&v) != Ok(StakerStatus::Idle)); while all_voters.len() < final_predicted_len as usize && voters_seen < (NPOS_MAX_ITERATIONS_COEFFICIENT * final_predicted_len as u32) { @@ -944,8 +944,8 @@ impl Pallet { let mut targets_seen = 0; // target list also contains chilled/idle validators, filter those. - let mut targets_iter = T::TargetList::iter() - .filter(|t| Self::status(&t) != Ok(StakerStatus::Idle)); + let mut targets_iter = + T::TargetList::iter().filter(|t| Self::status(&t) != Ok(StakerStatus::Idle)); while all_targets.len() < final_predicted_len as usize && targets_seen < (NPOS_MAX_ITERATIONS_COEFFICIENT * final_predicted_len as u32) { @@ -985,7 +985,6 @@ impl Pallet { /// to `Nominators` or `VoterList` outside of this function is almost certainly /// wrong. pub fn do_add_nominator(who: &T::AccountId, nominations: Nominations) { - match (Nominators::::contains_key(who), T::VoterList::contains(who)) { (false, false) => { // new nomination @@ -1005,12 +1004,14 @@ impl Pallet { }, (true, false) => { defensive!("unexpected state."); - } + }, }; debug_assert_eq!( Nominators::::count() + Validators::::count(), - T::VoterList::iter().filter(|v| Self::status(&v) != Ok(StakerStatus::Idle)).count() as u32, + T::VoterList::iter() + .filter(|v| Self::status(&v) != Ok(StakerStatus::Idle)) + .count() as u32, ); } @@ -1063,16 +1064,20 @@ impl Pallet { true }, (true, false) => { - defensive!("inconsistent state: staker is in nominators list but not in voter list"); + defensive!( + "inconsistent state: staker is in nominators list but not in voter list" + ); false - } + }, // nominator has been already removed. (false, false) => false, }; debug_assert_eq!( Nominators::::count() + Validators::::count(), - T::VoterList::iter().filter(|v| Self::status(&v) != Ok(StakerStatus::Idle)).count() as u32, + T::VoterList::iter() + .filter(|v| Self::status(&v) != Ok(StakerStatus::Idle)) + .count() as u32, ); outcome @@ -1097,7 +1102,9 @@ impl Pallet { debug_assert_eq!( Nominators::::count() + Validators::::count(), - T::VoterList::iter().filter(|v| Self::status(&v) != Ok(StakerStatus::Idle)).count() as u32, + T::VoterList::iter() + .filter(|v| Self::status(&v) != Ok(StakerStatus::Idle)) + .count() as u32, ); } @@ -1135,9 +1142,11 @@ impl Pallet { who, ); true - } + }, (true, false) => { - defensive!("inconsistent state: staker is in validators list but not in targets list"); + defensive!( + "inconsistent state: staker is in validators list but not in targets list" + ); false }, // validator has been already removed. @@ -1146,7 +1155,9 @@ impl Pallet { debug_assert_eq!( Nominators::::count() + Validators::::count(), - T::VoterList::iter().filter(|v| Self::status(&v) != Ok(StakerStatus::Idle)).count() as u32, + T::VoterList::iter() + .filter(|v| Self::status(&v) != Ok(StakerStatus::Idle)) + .count() as u32, ); outcome @@ -1897,8 +1908,9 @@ impl StakingInterface for Pallet { impl Pallet { pub(crate) fn do_try_state(_: BlockNumberFor) -> Result<(), TryRuntimeError> { ensure!( - T::VoterList::iter() - .all(|x| >::contains_key(&x) || >::contains_key(&x) || Self::status(&x) == Ok(StakerStatus::Idle)), + T::VoterList::iter().all(|x| >::contains_key(&x) || + >::contains_key(&x) || + Self::status(&x) == Ok(StakerStatus::Idle)), "VoterList contains non-staker" ); @@ -1910,12 +1922,15 @@ impl Pallet { fn check_count() -> Result<(), TryRuntimeError> { ensure!( - ::VoterList::iter().filter(|v| Self::status(&v) != Ok(StakerStatus::Idle)).count() as u32 == - Nominators::::count() + Validators::::count(), + ::VoterList::iter() + .filter(|v| Self::status(&v) != Ok(StakerStatus::Idle)) + .count() as u32 == Nominators::::count() + Validators::::count(), "wrong external count (VoterList.count != Nominators.count + Validators.count)" ); ensure!( - ::TargetList::iter().filter(|t| Self::status(&t) != Ok(StakerStatus::Idle)).count() as u32 == Validators::::count(), + ::TargetList::iter() + .filter(|t| Self::status(&t) != Ok(StakerStatus::Idle)) + .count() as u32 == Validators::::count(), "wrong external count (TargetList.count != Validators.count)" ); ensure!( diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 63814e3ea08b..f6cc84b931bd 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -7568,7 +7568,10 @@ mod stake_tracker { // the target list has been updated accordingly and an indirect rebag of 21 happened. // 11, althoug chilled, is still part of the target list. - assert_eq!(voters_and_targets().1, [(11, score_11_after), (21, score_21_after), (31, 500)]); + assert_eq!( + voters_and_targets().1, + [(11, score_11_after), (21, score_21_after), (31, 500)] + ); assert_eq!( target_bags_events(), [ @@ -7584,7 +7587,9 @@ mod stake_tracker { // fetching targets sorted and filtered by status works. assert_eq!( - TargetBagsList::iter().filter(|t| Staking::status(&t).unwrap() != StakerStatus::Idle).collect::>(), + TargetBagsList::iter() + .filter(|t| Staking::status(&t).unwrap() != StakerStatus::Idle) + .collect::>(), [21, 31], ); }) diff --git a/substrate/frame/staking/stake-tracker/src/lib.rs b/substrate/frame/staking/stake-tracker/src/lib.rs index 1f0db3fa93bb..5159e4f963d0 100644 --- a/substrate/frame/staking/stake-tracker/src/lib.rs +++ b/substrate/frame/staking/stake-tracker/src/lib.rs @@ -305,7 +305,6 @@ impl OnStakingUpdate> for Pallet { // // Note: it is assumed that who's staking state is updated *before* this method is called. fn on_stake_update(who: &T::AccountId, prev_stake: Option>>) { - // closure to calculate the stake imbalance of a staker. let stake_imbalance_of = |prev_stake: Option>>, voter_weight: ExtendedBalance| { @@ -408,7 +407,6 @@ impl OnStakingUpdate> for Pallet { // // Note: it is assumed that who's staking state is updated *before* calling this method. fn on_validator_add(who: &T::AccountId) { - // target may exist in the list in case of re-enabling a chilled validator; if !T::TargetList::contains(who) { let _ = T::TargetList::on_insert(who.clone(), Self::active_vote_of(who)) @@ -527,7 +525,8 @@ impl OnStakingUpdate> for Pallet { let stake_imbalance = StakeImbalance::Negative(Self::to_vote_extended(slashed_total)); match T::Staking::status(stash).defensive_proof("called on_slash on a unbonded stash") { - Ok(StakerStatus::Idle) | Ok(StakerStatus::Validator) => Self::update_score::(stash, stake_imbalance), + Ok(StakerStatus::Idle) | Ok(StakerStatus::Validator) => + Self::update_score::(stash, stake_imbalance), // score of target impacted by nominators will be updated through ledger.update. Ok(StakerStatus::Nominator(_)) => (), Err(_) => (), // nothing to see here. diff --git a/substrate/frame/staking/stake-tracker/src/tests.rs b/substrate/frame/staking/stake-tracker/src/tests.rs index 1dd17f96d341..f427e7f3ad9a 100644 --- a/substrate/frame/staking/stake-tracker/src/tests.rs +++ b/substrate/frame/staking/stake-tracker/src/tests.rs @@ -19,7 +19,7 @@ use crate::{mock::*, StakeImbalance}; -use frame_election_provider_support::{SortedListProvider, ScoreProvider}; +use frame_election_provider_support::{ScoreProvider, SortedListProvider}; use frame_support::assert_ok; use sp_staking::{OnStakingUpdate, Stake, StakingInterface};