Remove implicit approval chilling upon slash. (paritytech#12420)

* don't read slashing spans when taking election snapshot * update cargo.toml * bring back remote test * fix merge stuff * fix npos-voters function sig * remove as much redundant diff as you can * Update frame/staking/src/pallet/mod.rs Co-authored-by: Andronik <[email protected]> * fix * Update frame/staking/src/pallet/impls.rs * update lock * fix all tests * review comments * fmt * fix offence bench * clippy * ".git/.scripts/bench-bot.sh" pallet dev pallet_staking Co-authored-by: Andronik <[email protected]> Co-authored-by: Ankan <[email protected]> Co-authored-by: command-bot <>
ltfschoen · Feb 22, 2023 · b44ad46 · b44ad46
1 parent 4a525fb
commit b44ad46
Show file tree

Hide file tree

Showing 9 changed files with 526 additions and 546 deletions.
diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs
@@ -2293,6 +2293,8 @@ mod tests {
 			assert_eq!(MultiPhase::elect().unwrap_err(), ElectionError::Fallback("NoFallback."));
 			// phase is now emergency.
 			assert_eq!(MultiPhase::current_phase(), Phase::Emergency);
+			// snapshot is still there until election finalizes.
+			assert!(MultiPhase::snapshot().is_some());
 
 			assert_eq!(
 				multi_phase_events(),
@@ -2318,6 +2320,7 @@ mod tests {
 			// phase is now emergency.
 			assert_eq!(MultiPhase::current_phase(), Phase::Emergency);
 			assert!(MultiPhase::queued_solution().is_none());
+			assert!(MultiPhase::snapshot().is_some());
 
 			// no single account can trigger this
 			assert_noop!(

diff --git a/frame/offences/benchmarking/src/lib.rs b/frame/offences/benchmarking/src/lib.rs
@@ -308,17 +308,20 @@ benchmarks! {
 		let slash_amount = slash_fraction * bond_amount;
 		let reward_amount = slash_amount.saturating_mul(1 + n) / 2;
 		let reward = reward_amount / r;
+		let slash_report = |id| core::iter::once(
+			<T as StakingConfig>::RuntimeEvent::from(StakingEvent::<T>::SlashReported{ validator: id, fraction: slash_fraction, slash_era: 0})
+		);
 		let slash = |id| core::iter::once(
-			<T as StakingConfig>::RuntimeEvent::from(StakingEvent::<T>::Slashed{staker: id, amount: BalanceOf::<T>::from(slash_amount)})
+			<T as StakingConfig>::RuntimeEvent::from(StakingEvent::<T>::Slashed{ staker: id, amount: BalanceOf::<T>::from(slash_amount) })
 		);
 		let balance_slash = |id| core::iter::once(
-			<T as BalancesConfig>::RuntimeEvent::from(pallet_balances::Event::<T>::Slashed{who: id, amount: slash_amount.into()})
+			<T as BalancesConfig>::RuntimeEvent::from(pallet_balances::Event::<T>::Slashed{ who: id, amount: slash_amount.into() })
 		);
 		let chill = |id| core::iter::once(
-			<T as StakingConfig>::RuntimeEvent::from(StakingEvent::<T>::Chilled{stash: id})
+			<T as StakingConfig>::RuntimeEvent::from(StakingEvent::<T>::Chilled{ stash: id })
 		);
 		let balance_deposit = |id, amount: u32|
-			<T as BalancesConfig>::RuntimeEvent::from(pallet_balances::Event::<T>::Deposit{who: id, amount: amount.into()});
+			<T as BalancesConfig>::RuntimeEvent::from(pallet_balances::Event::<T>::Deposit{ who: id, amount: amount.into() });
 		let mut first = true;
 		let slash_events = raw_offenders.into_iter()
 			.flat_map(|offender| {
@@ -328,6 +331,7 @@ benchmarks! {
 				});
 
 				let mut events = chill(offender.stash.clone()).map(Into::into)
+					.chain(slash_report(offender.stash.clone()).map(Into::into))
 					.chain(balance_slash(offender.stash.clone()).map(Into::into))
 					.chain(slash(offender.stash).map(Into::into))
 					.chain(nom_slashes)
@@ -407,6 +411,7 @@ benchmarks! {
 			System::<T>::event_count(), 0
 			+ 1 // offence
 			+ 3 // reporter (reward + endowment)
+			+ 1 // offenders reported
 			+ 2 // offenders slashed
 			+ 1 // offenders chilled
 			+ 2 * n // nominators slashed
@@ -443,6 +448,7 @@ benchmarks! {
 			System::<T>::event_count(), 0
 			+ 1 // offence
 			+ 3 // reporter (reward + endowment)
+			+ 1 // offenders reported
 			+ 2 // offenders slashed
 			+ 1 // offenders chilled
 			+ 2 * n // nominators slashed

diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs
@@ -792,12 +792,10 @@ benchmarks! {
 	}
 
 	get_npos_voters {
-		// number of validator intention.
+		// number of validator intention. we will iterate all of them.
 		let v in (MaxValidators::<T>::get() / 2) .. MaxValidators::<T>::get();
-		// number of nominator intention.
+		// number of nominator intention. we will iterate all of them.
 		let n in (MaxNominators::<T>::get() / 2) .. MaxNominators::<T>::get();
-		// total number of slashing spans. Assigned to validators randomly.
-		let s in 1 .. 20;
 
 		let validators = create_validators_with_nominators_for_era::<T>(
 			v, n, T::MaxNominations::get() as usize, false, None
@@ -806,9 +804,8 @@ benchmarks! {
 		.map(|v| T::Lookup::lookup(v).unwrap())
 		.collect::<Vec<_>>();
 
-		(0..s).for_each(|index| {
-			add_slashing_spans::<T>(&validators[index as usize], 10);
-		});
+		assert_eq!(Validators::<T>::count(), v);
+		assert_eq!(Nominators::<T>::count(), n);
 
 		let num_voters = (v + n) as usize;
 	}: {

diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs
@@ -40,7 +40,7 @@ use sp_staking::{
 	offence::{DisableStrategy, OffenceDetails, OnOffenceHandler},
 	EraIndex, SessionIndex, Stake, StakingInterface,
 };
-use sp_std::{collections::btree_map::BTreeMap, prelude::*};
+use sp_std::prelude::*;
 
 use crate::{
 	log, slashing, weights::WeightInfo, ActiveEraInfo, BalanceOf, EraPayout, Exposure, ExposureOf,
@@ -351,6 +351,7 @@ impl<T: Config> Pallet<T> {
 		}
 	}
 
+	/// Start a new era. It does:
 	///
 	/// * Increment `active_era.index`,
 	/// * reset `active_era.start`,
@@ -704,11 +705,6 @@ impl<T: Config> Pallet<T> {
 	/// `maybe_max_len` can imposes a cap on the number of voters returned;
 	///
 	/// This function is self-weighing as [`DispatchClass::Mandatory`].
-	///
-	/// ### Slashing
-	///
-	/// All votes that have been submitted before the last non-zero slash of the corresponding
-	/// target are *auto-chilled*, but still count towards the limit imposed by `maybe_max_len`.
 	pub fn get_npos_voters(maybe_max_len: Option<usize>) -> Vec<VoterOf<Self>> {
 		let max_allowed_len = {
 			let all_voter_count = T::VoterList::count() as usize;
@@ -719,7 +715,6 @@ impl<T: Config> Pallet<T> {
 
 		// cache a few things.
 		let weight_of = Self::weight_of_fn();
-		let slashing_spans = <SlashingSpans<T>>::iter().collect::<BTreeMap<_, _>>();
 
 		let mut voters_seen = 0u32;
 		let mut validators_taken = 0u32;
@@ -737,18 +732,12 @@ impl<T: Config> Pallet<T> {
 				None => break,
 			};
 
-			if let Some(Nominations { submitted_in, mut targets, suppressed: _ }) =
-				<Nominators<T>>::get(&voter)
-			{
-				// if this voter is a nominator:
-				targets.retain(|stash| {
-					slashing_spans
-						.get(stash)
-						.map_or(true, |spans| submitted_in >= spans.last_nonzero_slash())
-				});
-				if !targets.len().is_zero() {
+			if let Some(Nominations { targets, .. }) = <Nominators<T>>::get(&voter) {
+				if !targets.is_empty() {
 					all_voters.push((voter.clone(), weight_of(&voter), targets));
 					nominators_taken.saturating_inc();
+				} else {
+					// Technically should never happen, but not much we can do about it.
 				}
 			} else if Validators::<T>::contains_key(&voter) {
 				// if this voter is a validator:
@@ -771,18 +760,14 @@ impl<T: Config> Pallet<T> {
 					warn,
 					"DEFENSIVE: invalid item in `VoterList`: {:?}, this nominator probably has too many nominations now",
 					voter
-				)
+				);
 			}
 		}
 
 		// all_voters should have not re-allocated.
 		debug_assert!(all_voters.capacity() == max_allowed_len);
 
-		Self::register_weight(T::WeightInfo::get_npos_voters(
-			validators_taken,
-			nominators_taken,
-			slashing_spans.len() as u32,
-		));
+		Self::register_weight(T::WeightInfo::get_npos_voters(validators_taken, nominators_taken));
 
 		log!(
 			info,
@@ -1285,6 +1270,12 @@ where
 				disable_strategy,
 			});
 
+			Self::deposit_event(Event::<T>::SlashReported {
+				validator: stash.clone(),
+				fraction: *slash_fraction,
+				slash_era,
+			});
+
 			if let Some(mut unapplied) = unapplied {
 				let nominators_len = unapplied.others.len() as u64;
 				let reporters_len = details.reporters.len() as u64;

diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs
@@ -517,7 +517,7 @@ pub mod pallet {
 	#[pallet::storage]
 	#[pallet::getter(fn slashing_spans)]
 	#[pallet::unbounded]
-	pub(crate) type SlashingSpans<T: Config> =
+	pub type SlashingSpans<T: Config> =
 		StorageMap<_, Twox64Concat, T::AccountId, slashing::SlashingSpans>;
 
 	/// Records information about the maximum slash of a stash within a slashing span,
@@ -671,8 +671,11 @@ pub mod pallet {
 		EraPaid { era_index: EraIndex, validator_payout: BalanceOf<T>, remainder: BalanceOf<T> },
 		/// The nominator has been rewarded by this amount.
 		Rewarded { stash: T::AccountId, amount: BalanceOf<T> },
-		/// One staker (and potentially its nominators) has been slashed by the given amount.
+		/// A staker (validator or nominator) has been slashed by the given amount.
 		Slashed { staker: T::AccountId, amount: BalanceOf<T> },
+		/// A slash for the given validator, for the given percentage of their stake, at the given
+		/// era as been reported.
+		SlashReported { validator: T::AccountId, fraction: Perbill, slash_era: EraIndex },
 		/// An old slashing report from a prior era was discarded because it could
 		/// not be processed.
 		OldSlashingReportDiscarded { session_index: SessionIndex },

diff --git a/frame/staking/src/slashing.rs b/frame/staking/src/slashing.rs
@@ -239,9 +239,9 @@ pub(crate) fn compute_slash<T: Config>(
 		return None
 	}
 
-	let (prior_slash_p, _era_slash) =
+	let prior_slash_p =
 		<Pallet<T> as Store>::ValidatorSlashInEra::get(&params.slash_era, params.stash)
-			.unwrap_or((Perbill::zero(), Zero::zero()));
+			.map_or(Zero::zero(), |(prior_slash_proportion, _)| prior_slash_proportion);
 
 	// compare slash proportions rather than slash values to avoid issues due to rounding
 	// error.
@@ -390,9 +390,7 @@ fn slash_nominators<T: Config>(
 			let mut era_slash =
 				<Pallet<T> as Store>::NominatorSlashInEra::get(&params.slash_era, stash)
 					.unwrap_or_else(Zero::zero);
-
 			era_slash += own_slash_difference;
-
 			<Pallet<T> as Store>::NominatorSlashInEra::insert(&params.slash_era, stash, &era_slash);
 
 			era_slash
@@ -411,12 +409,10 @@ fn slash_nominators<T: Config>(
 			let target_span = spans.compare_and_update_span_slash(params.slash_era, era_slash);
 
 			if target_span == Some(spans.span_index()) {
-				// End the span, but don't chill the nominator. its nomination
-				// on this validator will be ignored in the future.
+				// end the span, but don't chill the nominator.
 				spans.end_span(params.now);
 			}
 		}
-
 		nominators_slashed.push((stash.clone(), nom_slashed));
 	}