Properly defer slashes

frame/staking/src/mock.rs

@@ -549,6 +549,7 @@ impl ExtBuilder {
 		ext
 	}
 	pub fn build_and_execute(self, test: impl FnOnce() -> ()) {
+		sp_tracing::try_init_simple();
 		let mut ext = self.build();
 		ext.execute_with(test);
 		ext.execute_with(post_conditions);
@@ -884,6 +885,20 @@ pub(crate) fn staking_events() -> Vec<crate::Event<Test>> {
 		.collect()
 }
 
+parameter_types! {
+	static StakingEventsIndex: usize = 0;
+}
+
+pub(crate) fn staking_events_since_last_call() -> Vec<crate::Event<Test>> {
+	let all: Vec<_> = System::events()
+		.into_iter()
+		.filter_map(|r| if let Event::Staking(inner) = r.event { Some(inner) } else { None })
+		.collect();
+	let seen = StakingEventsIndex::get();
+	StakingEventsIndex::set(all.len());
+	all.into_iter().skip(seen).collect()
+}
+
 pub(crate) fn balances(who: &AccountId) -> (Balance, Balance) {
 	(Balances::free_balance(who), Balances::reserved_balance(who))
 }

frame/staking/src/pallet/impls.rs

@@ -599,20 +599,19 @@ impl<T: Config> Pallet<T> {
 
 	/// Apply previously-unapplied slashes on the beginning of a new era, after a delay.
 	fn apply_unapplied_slashes(active_era: EraIndex) {
-		let slash_defer_duration = T::SlashDeferDuration::get();
-		<Self as Store>::EarliestUnappliedSlash::mutate(|earliest| {
-			if let Some(ref mut earliest) = earliest {
-				let keep_from = active_era.saturating_sub(slash_defer_duration);
-				for era in (*earliest)..keep_from {
-					let era_slashes = <Self as Store>::UnappliedSlashes::take(&era);
-					for slash in era_slashes {
-						slashing::apply_slash::<T>(slash, era);
-					}
-				}
-
-				*earliest = (*earliest).max(keep_from)
+		if <Self as Store>::UnappliedSlashes::contains_key(&active_era) {
+			let era_slashes = <Self as Store>::UnappliedSlashes::take(&active_era);
+			log!(
+				debug,
+				"found {} slashes scheduled to be executed in era {}",
+				active_era,
+				era_slashes.len()
+			);
+			for slash in era_slashes {
+				let slash_era = active_era.saturating_sub(T::SlashDeferDuration::get());
+				slashing::apply_slash::<T>(slash, slash_era);
 			}
-		})
+		}
 	}
 
 	/// Add reward points to validators using their stash account ID.
@@ -1209,11 +1208,6 @@ where
 			}
 		};
 
-		<Self as Store>::EarliestUnappliedSlash::mutate(|earliest| {
-			if earliest.is_none() {
-				*earliest = Some(active_era)
-			}
-		});
 		add_db_reads_writes(1, 1);
 
 		let slash_defer_duration = T::SlashDeferDuration::get();
@@ -1263,9 +1257,18 @@ where
 					}
 				} else {
 					// Defer to end of some `slash_defer_duration` from now.
-					<Self as Store>::UnappliedSlashes::mutate(active_era, move |for_later| {
-						for_later.push(unapplied)
-					});
+					log!(
+						debug,
+						"deferring slash of {:?}% happened in {:?} (reported in {:?}) to {:?}",
+						slash_fraction,
+						slash_era,
+						active_era,
+						slash_era + slash_defer_duration + 1,
+					);
+					<Self as Store>::UnappliedSlashes::mutate(
+						slash_era + slash_defer_duration + 1,
+						move |for_later| for_later.push(unapplied),
+					);
 					add_db_reads_writes(1, 1);
 				}
 			} else {

frame/staking/src/tests.rs

@@ -2778,12 +2778,91 @@ fn deferred_slashes_are_deferred() {
 		assert_eq!(Balances::free_balance(11), 1000);
 		assert_eq!(Balances::free_balance(101), 2000);
 
+		let _ = staking_events_since_last_call();
+
 		// at the start of era 4, slashes from era 1 are processed,
 		// after being deferred for at least 2 full eras.
 		mock::start_active_era(4);
 
 		assert_eq!(Balances::free_balance(11), 900);
 		assert_eq!(Balances::free_balance(101), 2000 - (nominated_value / 10));
+
+		assert_eq!(
+			staking_events_since_last_call(),
+			vec![
+				Event::StakersElected,
+				Event::EraPaid(3, 11075, 33225),
+				Event::Slashed(11, 100),
+				Event::Slashed(101, 12)
+			]
+		);
+	})
+}
+
+#[test]
+fn retroactive_deferred_slashes_two_eras_before_() {
+	ExtBuilder::default().slash_defer_duration(2).build_and_execute(|| {
+		assert_eq!(BondingDuration::get(), 3);
+
+		mock::start_active_era(1);
+		let exposure_11_at_era1 = Staking::eras_stakers(active_era(), 11);
+
+		mock::start_active_era(3);
+		on_offence_in_era(
+			&[OffenceDetails { offender: (11, exposure_11_at_era1), reporters: vec![] }],
+			&[Perbill::from_percent(10)],
+			1, // should be deferred for two full eras, and applied at the beginning of era 4.
+			DisableStrategy::Never,
+		);
+		let _ = staking_events_since_last_call();
+
+		mock::start_active_era(4);
+
+		assert_eq!(
+			staking_events_since_last_call(),
+			vec![
+				Event::StakersElected,
+				Event::EraPaid(3, 7100, 21300),
+				Event::Slashed(11, 100),
+				Event::Slashed(101, 12)
+			]
+		);
+	})
+}
+
+#[test]
+fn retroactive_deferred_slashes_one_before() {
+	ExtBuilder::default().slash_defer_duration(2).build_and_execute(|| {
+		assert_eq!(BondingDuration::get(), 3);
+
+		mock::start_active_era(1);
+		let exposure_11_at_era1 = Staking::eras_stakers(active_era(), 11);
+
+		mock::start_active_era(3);
+		on_offence_in_era(
+			&[OffenceDetails { offender: (11, exposure_11_at_era1), reporters: vec![] }],
+			&[Perbill::from_percent(10)],
+			2, // should be deferred for two full eras, and applied at the beginning of era 5.
+			DisableStrategy::Never,
+		);
+		let _ = staking_events_since_last_call();
+
+		mock::start_active_era(4);
+		assert_eq!(
+			staking_events_since_last_call(),
+			vec![Event::StakersElected, Event::EraPaid(3, 7100, 21300)]
+		);
+
+		mock::start_active_era(5);
+		assert_eq!(
+			staking_events_since_last_call(),
+			vec![
+				Event::StakersElected,
+				Event::EraPaid(4, 11075, 33225),
+				Event::Slashed(11, 100),
+				Event::Slashed(101, 12)
+			]
+		);
 	})
 }
 
@@ -2872,6 +2951,7 @@ fn remove_deferred() {
 		assert_eq!(Balances::free_balance(101), 2000);
 		let nominated_value = exposure.others.iter().find(|o| o.who == 101).unwrap().value;
 
+		// deferred to start of era 4.
 		on_offence_now(
 			&[OffenceDetails { offender: (11, exposure.clone()), reporters: vec![] }],
 			&[Perbill::from_percent(10)],
@@ -2882,6 +2962,7 @@ fn remove_deferred() {
 
 		mock::start_active_era(2);
 
+		// reported later, but deferred to start of era 4 as well.
 		on_offence_in_era(
 			&[OffenceDetails { offender: (11, exposure.clone()), reporters: vec![] }],
 			&[Perbill::from_percent(15)],
@@ -2895,7 +2976,8 @@ fn remove_deferred() {
 			Error::<Test>::EmptyTargets
 		);
 
-		assert_ok!(Staking::cancel_deferred_slash(Origin::root(), 1, vec![0]));
+		// cancel one of them.
+		assert_ok!(Staking::cancel_deferred_slash(Origin::root(), 4, vec![0]));
 
 		assert_eq!(Balances::free_balance(11), 1000);
 		assert_eq!(Balances::free_balance(101), 2000);
@@ -2907,13 +2989,19 @@ fn remove_deferred() {
 
 		// at the start of era 4, slashes from era 1 are processed,
 		// after being deferred for at least 2 full eras.
+		let _ = staking_events_since_last_call();
 		mock::start_active_era(4);
 
-		// the first slash for 10% was cancelled, so no effect.
-		assert_eq!(Balances::free_balance(11), 1000);
-		assert_eq!(Balances::free_balance(101), 2000);
-
-		mock::start_active_era(5);
+		// the first slash for 10% was cancelled, but the 15% one
+		assert_eq!(
+			staking_events_since_last_call(),
+			vec![
+				Event::StakersElected,
+				Event::EraPaid(3, 11075, 33225),
+				Event::Slashed(11, 50),
+				Event::Slashed(101, 7)
+			]
+		);
 
 		let slash_10 = Perbill::from_percent(10);
 		let slash_15 = Perbill::from_percent(15);
@@ -2966,7 +3054,7 @@ fn remove_multi_deferred() {
 			&[Perbill::from_percent(25)],
 		);
 
-		assert_eq!(<Staking as Store>::UnappliedSlashes::get(&1).len(), 5);
+		assert_eq!(<Staking as Store>::UnappliedSlashes::get(&4).len(), 5);
 
 		// fails if list is not sorted
 		assert_noop!(
@@ -2984,9 +3072,9 @@ fn remove_multi_deferred() {
 			Error::<Test>::InvalidSlashIndex
 		);
 
-		assert_ok!(Staking::cancel_deferred_slash(Origin::root(), 1, vec![0, 2, 4]));
+		assert_ok!(Staking::cancel_deferred_slash(Origin::root(), 4, vec![0, 2, 4]));
 
-		let slashes = <Staking as Store>::UnappliedSlashes::get(&1);
+		let slashes = <Staking as Store>::UnappliedSlashes::get(&4);
 		assert_eq!(slashes.len(), 2);
 		assert_eq!(slashes[0].validator, 21);
 		assert_eq!(slashes[1].validator, 42);