diff --git a/substrate/frame/election-provider-multi-phase/src/benchmarking.rs b/substrate/frame/election-provider-multi-phase/src/benchmarking.rs index 1284d0b873..e9a4583b57 100644 --- a/substrate/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/substrate/frame/election-provider-multi-phase/src/benchmarking.rs @@ -169,10 +169,12 @@ fn set_up_data_provider(v: u32, t: u32) { let mut targets = (0..t) .map(|i| { let target = frame_benchmarking::account::("Target", i, SEED); + T::DataProvider::add_target(target.clone()); target }) .collect::>(); + // we should always have enough voters to fill. assert!( targets.len() > ::MaxVotesPerVoter::get() as usize @@ -276,7 +278,7 @@ frame_benchmarking::benchmarks! { >::create_snapshot_internal(targets, voters, desired_targets) } verify { assert!(>::snapshot().is_some()); - assert_eq!(>::snapshot_metadata().ok_or("metadata missing")?.voters, v + t); + assert_eq!(>::snapshot_metadata().ok_or("metadata missing")?.voters, v); assert_eq!(>::snapshot_metadata().ok_or("metadata missing")?.targets, t); } @@ -450,7 +452,7 @@ frame_benchmarking::benchmarks! { >::create_snapshot().map_err(|_| "could not create snapshot")?; } verify { assert!(>::snapshot().is_some()); - assert_eq!(>::snapshot_metadata().ok_or("snapshot missing")?.voters, v + t); + assert_eq!(>::snapshot_metadata().ok_or("snapshot missing")?.voters, v); assert_eq!(>::snapshot_metadata().ok_or("snapshot missing")?.targets, t); } diff --git a/substrate/frame/election-provider-multi-phase/src/mock.rs b/substrate/frame/election-provider-multi-phase/src/mock.rs index 911e93d358..c5133d89e4 100644 --- a/substrate/frame/election-provider-multi-phase/src/mock.rs +++ b/substrate/frame/election-provider-multi-phase/src/mock.rs @@ -501,12 +501,6 @@ impl ElectionDataProvider for StakingMock { let mut current = Targets::get(); current.push(target); Targets::set(current); - - // to be on-par with staking, we add a self vote as well. the stake is really not that - // important. - let mut current = Voters::get(); - current.push((target, ExistentialDeposit::get() as u64, bounded_vec![target])); - Voters::set(current); } } diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index 99622a64f8..8f1513f506 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -159,7 +159,7 @@ //! ``` //! use pallet_staking::{self as staking}; //! -//! #[frame_support::pallet] +//! #[frame_support::pallet(dev_mode)] //! pub mod pallet { //! use super::*; //! use frame_support::pallet_prelude::*; diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index f2aa48e331..0e8c98cab2 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -789,8 +789,14 @@ impl Pallet { None => break, }; + let voter_weight = weight_of(&voter); + // if voter weight is zero, do not consider this voter for the snapshot. + if voter_weight.is_zero() { + log!(debug, "voter's active balance is 0. skip this voter."); + continue + } + if let Some(Nominations { targets, .. }) = >::get(&voter) { - let voter_weight = weight_of(&voter); if !targets.is_empty() { all_voters.push((voter.clone(), voter_weight, targets)); nominators_taken.saturating_inc(); @@ -803,7 +809,7 @@ impl Pallet { // if this voter is a validator: let self_vote = ( voter.clone(), - weight_of(&voter), + voter_weight, vec![voter.clone()] .try_into() .expect("`MaxVotesPerVoter` must be greater than or equal to 1"), @@ -830,7 +836,7 @@ impl Pallet { Self::register_weight(T::WeightInfo::get_npos_voters(validators_taken, nominators_taken)); let min_active_stake: T::CurrencyBalance = - if all_voters.len() == 0 { 0u64.into() } else { min_active_stake.into() }; + if all_voters.is_empty() { Zero::zero() } else { min_active_stake.into() }; MinimumActiveStake::::put(min_active_stake); diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index e3ee4cd1a8..124e1c3fda 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -4519,11 +4519,75 @@ mod election_data_provider { } #[test] - fn set_minimum_active_stake_zero_correct() { + fn set_minimum_active_stake_lower_bond_works() { + // if there are no voters, minimum active stake is zero (should not happen). ExtBuilder::default().has_stakers(false).build_and_execute(|| { + assert_eq!(::VoterList::count(), 0); assert_ok!(::electing_voters(None)); assert_eq!(MinimumActiveStake::::get(), 0); }); + + // lower non-zero active stake below `MinNominatorBond` is the minimum active stake if + // it is selected as part of the npos voters. + ExtBuilder::default().has_stakers(true).nominate(true).build_and_execute(|| { + assert_eq!(MinNominatorBond::::get(), 1); + assert_eq!(::VoterList::count(), 4); + + assert_ok!(Staking::bond(RuntimeOrigin::signed(4), 5, Default::default(),)); + assert_ok!(Staking::nominate(RuntimeOrigin::signed(4), vec![1])); + assert_eq!(::VoterList::count(), 5); + + let voters_before = ::electing_voters(None).unwrap(); + assert_eq!(MinimumActiveStake::::get(), 5); + + // update minimum nominator bond. + MinNominatorBond::::set(10); + assert_eq!(MinNominatorBond::::get(), 10); + // voter list still considers nominator 4 for voting, even though its active stake is + // lower than `MinNominatorBond`. + assert_eq!(::VoterList::count(), 5); + + let voters = ::electing_voters(None).unwrap(); + assert_eq!(voters_before, voters); + + // minimum active stake is lower than `MinNominatorBond`. + assert_eq!(MinimumActiveStake::::get(), 5); + }); + } + + #[test] + fn set_minimum_active_bond_corrupt_state() { + ExtBuilder::default() + .has_stakers(true) + .nominate(true) + .add_staker(61, 61, 2_000, StakerStatus::::Nominator(vec![21])) + .build_and_execute(|| { + assert_eq!(Staking::weight_of(&101), 500); + let voters = ::electing_voters(None).unwrap(); + assert_eq!(voters.len(), 5); + assert_eq!(MinimumActiveStake::::get(), 500); + + assert_ok!(Staking::unbond(RuntimeOrigin::signed(101), 200)); + start_active_era(10); + assert_ok!(Staking::unbond(RuntimeOrigin::signed(101), 100)); + start_active_era(20); + + // corrupt ledger state by lowering max unlocking chunks bounds. + MaxUnlockingChunks::set(1); + + let voters = ::electing_voters(None).unwrap(); + // number of returned voters decreases since ledger entry of stash 101 is now + // corrupt. + assert_eq!(voters.len(), 4); + // minimum active stake does not take into consideration the corrupt entry. + assert_eq!(MinimumActiveStake::::get(), 2_000); + + // voter weight of corrupted ledger entry is 0. + assert_eq!(Staking::weight_of(&101), 0); + + // reset max unlocking chunks for try_state to pass. + MaxUnlockingChunks::set(32); + }) } #[test]