keep nominations after getting kicked with zero slash (#4681)

* keep nominations after getting kicked with zero slash

* rename next_key to maybe_next_key

Co-Authored-By: Gavin Wood <gavin@parity.io>

Co-authored-by: Gavin Wood <github@gavwood.com>

substrate/frame/staking/src/lib.rs

@@ -1482,11 +1482,11 @@ impl<T: Trait> Module<T> {
 			let Nominations { submitted_in, mut targets, suppressed: _ } = nominations;
 
 			// Filter out nomination targets which were nominated before the most recent
-			// slashing span.
+			// non-zero slash.
 			targets.retain(|stash| {
 				<Self as Store>::SlashingSpans::get(&stash).map_or(
 					true,
-					|spans| submitted_in >= spans.last_start(),
+					|spans| submitted_in >= spans.last_nonzero_slash(),
 				)
 			});
 

substrate/frame/staking/src/migration.rs

@@ -20,12 +20,14 @@
 pub type VersionNumber = u32;
 
 // the current expected version of the storage
-pub const CURRENT_VERSION: VersionNumber = 1;
+pub const CURRENT_VERSION: VersionNumber = 2;
 
+/// The inner logic of migrations.
 #[cfg(any(test, feature = "migrate"))]
-mod inner {
+pub mod inner {
 	use crate::{Store, Module, Trait};
-	use frame_support::{StorageLinkedMap, StorageValue};
+	use frame_support::{StorageLinkedMap, StoragePrefixedMap, StorageValue};
+	use codec::{Encode, Decode};
 	use sp_std::vec::Vec;
 	use super::{CURRENT_VERSION, VersionNumber};
 
@@ -60,6 +62,55 @@ mod inner {
 		frame_support::print("Finished migrating Staking storage to v1.");
 	}
 
+	// migrate storage from v1 to v2: adds another field to the `SlashingSpans`
+	// struct.
+	pub fn to_v2<T: Trait>(version: &mut VersionNumber) {
+		use crate::{EraIndex, slashing::SpanIndex};
+		#[derive(Decode)]
+		struct V1SlashingSpans {
+			span_index: SpanIndex,
+			last_start: EraIndex,
+			prior: Vec<EraIndex>,
+		}
+
+		#[derive(Encode)]
+		struct V2SlashingSpans {
+			span_index: SpanIndex,
+			last_start: EraIndex,
+			last_nonzero_slash: EraIndex,
+			prior: Vec<EraIndex>,
+		}
+
+		if *version != 1 { return }
+		*version += 1;
+
+		let prefix = <Module<T> as Store>::SlashingSpans::final_prefix();
+		let mut current_key = prefix.to_vec();
+		loop {
+			let maybe_next_key = sp_io::storage::next_key(&current_key[..])
+				.filter(|v| v.starts_with(&prefix[..]));
+
+			match maybe_next_key {
+				Some(next_key) => {
+					let maybe_spans = sp_io::storage::get(&next_key[..])
+						.and_then(|v| V1SlashingSpans::decode(&mut &v[..]).ok());
+					if let Some(spans) = maybe_spans {
+						let new_val = V2SlashingSpans {
+							span_index: spans.span_index,
+							last_start: spans.last_start,
+							last_nonzero_slash: spans.last_start,
+							prior: spans.prior,
+						}.encode();
+
+						sp_io::storage::set(&next_key[..], &new_val[..]);
+					}
+					current_key = next_key;
+				}
+				None => break,
+			}
+		}
+	}
+
 	pub(super) fn perform_migrations<T: Trait>() {
 		<Module<T> as Store>::StorageVersion::mutate(|version| {
 			if *version < MIN_SUPPORTED_VERSION {
@@ -72,6 +123,7 @@ mod inner {
 			if *version == CURRENT_VERSION { return }
 
 			to_v1::<T>(version);
+			to_v2::<T>(version);
 		});
 	}
 }

substrate/frame/staking/src/slashing.rs

@@ -90,7 +90,9 @@ pub struct SlashingSpans {
 	span_index: SpanIndex,
 	// the start era of the most recent (ongoing) slashing span.
 	last_start: EraIndex,
-	// all prior slashing spans start indices, in reverse order (most recent first)
+	// the last era at which a non-zero slash occurred.
+	last_nonzero_slash: EraIndex,
+	// all prior slashing spans' start indices, in reverse order (most recent first)
 	// encoded as offsets relative to the slashing span after it.
 	prior: Vec<EraIndex>,
 }
@@ -102,6 +104,10 @@ impl SlashingSpans {
 		SlashingSpans {
 			span_index: 0,
 			last_start: window_start,
+			// initialize to zero, as this structure is lazily created until
+			// the first slash is applied. setting equal to `window_start` would
+			// put a time limit on nominations.
+			last_nonzero_slash: 0,
 			prior: Vec::new(),
 		}
 	}
@@ -136,9 +142,9 @@ impl SlashingSpans {
 		sp_std::iter::once(last).chain(prior)
 	}
 
-	/// Yields the era index where the last (current) slashing span started.
+	/// Yields the era index where the most recent non-zero slash occurred.
-	pub(crate) fn last_start(&self) -> EraIndex {
+	pub(crate) fn last_nonzero_slash(&self) -> EraIndex {
-		self.last_start
+		self.last_nonzero_slash
 	}
 
 	// prune the slashing spans against a window, whose start era index is given.
@@ -457,8 +463,12 @@ impl<'a, T: 'a + Trait> InspectingSpans<'a, T> {
 		self.dirty = self.spans.end_span(now) || self.dirty;
 	}
 
-	fn add_slash(&mut self, amount: BalanceOf<T>) {
+	// add some value to the slash of the staker.
+	// invariant: the staker is being slashed for non-zero value here
+	// although `amount` may be zero, as it is only a difference.
+	fn add_slash(&mut self, amount: BalanceOf<T>, slash_era: EraIndex) {
 		*self.slash_of += amount;
+		self.spans.last_nonzero_slash = sp_std::cmp::max(self.spans.last_nonzero_slash, slash_era);
 	}
 
 	// find the span index of the given era, if covered.
@@ -489,7 +499,7 @@ impl<'a, T: 'a + Trait> InspectingSpans<'a, T> {
 			let reward = REWARD_F1
 				* (self.reward_proportion * slash).saturating_sub(span_record.paid_out);
 
-			self.add_slash(difference);
+			self.add_slash(difference, slash_era);
 			changed = true;
 
 			reward
@@ -681,6 +691,7 @@ mod tests {
 		let spans = SlashingSpans {
 			span_index: 0,
 			last_start: 1000,
+			last_nonzero_slash: 0,
 			prior: Vec::new(),
 		};
 
@@ -695,6 +706,7 @@ mod tests {
 		let spans = SlashingSpans {
 			span_index: 10,
 			last_start: 1000,
+			last_nonzero_slash: 0,
 			prior: vec![10, 9, 8, 10],
 		};
 
@@ -715,6 +727,7 @@ mod tests {
 		let mut spans = SlashingSpans {
 			span_index: 10,
 			last_start: 1000,
+			last_nonzero_slash: 0,
 			prior: vec![10, 9, 8, 10],
 		};
 
@@ -768,6 +781,7 @@ mod tests {
 		let mut spans = SlashingSpans {
 			span_index: 10,
 			last_start: 1000,
+			last_nonzero_slash: 0,
 			prior: vec![10, 9, 8, 10],
 		};
 		assert_eq!(spans.prune(2000), Some((6, 10)));
@@ -784,6 +798,7 @@ mod tests {
 		let mut spans = SlashingSpans {
 			span_index: 1,
 			last_start: 10,
+			last_nonzero_slash: 0,
 			prior: Vec::new(),
 		};
 

substrate/frame/staking/src/tests.rs

@@ -18,12 +18,13 @@
 
 use super::*;
 use mock::*;
+use codec::Encode;
 use sp_runtime::{assert_eq_error_rate, traits::{OnInitialize, BadOrigin}};
 use sp_staking::offence::OffenceDetails;
 use frame_support::{
 	assert_ok, assert_noop,
 	traits::{Currency, ReservableCurrency},
-	dispatch::DispatchError,
+	dispatch::DispatchError, StorageMap,
 };
 use substrate_test_utils::assert_eq_uvec;
 
@@ -2710,7 +2711,95 @@ fn slash_kicks_validators_not_nominators() {
 
 		// and make sure that the vote will be ignored even if the validator
 		// re-registers.
-		let last_slash = <Staking as Store>::SlashingSpans::get(&11).unwrap().last_start();
+		let last_slash = <Staking as Store>::SlashingSpans::get(&11).unwrap().last_nonzero_slash();
 		assert!(nominations.submitted_in < last_slash);
 	});
 }
+
+#[test]
+fn migration_v2() {
+	ExtBuilder::default().build().execute_with(|| {
+		use crate::{EraIndex, slashing::SpanIndex};
+
+		#[derive(Encode)]
+		struct V1SlashingSpans {
+			span_index: SpanIndex,
+			last_start: EraIndex,
+			prior: Vec<EraIndex>,
+		}
+
+		// inject old-style values directly into storage.
+		let set = |stash, spans: V1SlashingSpans| {
+			let key = <Staking as Store>::SlashingSpans::hashed_key_for(stash);
+			sp_io::storage::set(&key, &spans.encode());
+		};
+
+		let spans_11 = V1SlashingSpans {
+			span_index: 10,
+			last_start: 1,
+			prior: vec![0],
+		};
+
+		let spans_21 = V1SlashingSpans {
+			span_index: 1,
+			last_start: 5,
+			prior: vec![],
+		};
+
+		set(11, spans_11);
+		set(21, spans_21);
+
+		<Staking as Store>::StorageVersion::put(1);
+
+		// perform migration.
+		crate::migration::inner::to_v2::<Test>(&mut 1);
+
+		assert_eq!(
+			<Staking as Store>::SlashingSpans::get(&11).unwrap().last_nonzero_slash(),
+			1,
+		);
+
+		assert_eq!(
+			<Staking as Store>::SlashingSpans::get(&21).unwrap().last_nonzero_slash(),
+			5,
+		);
+	});
+}
+
+#[test]
+fn zero_slash_keeps_nominators() {
+	ExtBuilder::default().build().execute_with(|| {
+		start_era(1);
+
+		assert_eq!(Balances::free_balance(&11), 1000);
+
+		let exposure = Staking::stakers(&11);
+		assert_eq!(Balances::free_balance(&101), 2000);
+
+		on_offence_now(
+			&[
+				OffenceDetails {
+					offender: (11, exposure.clone()),
+					reporters: vec![],
+				},
+			],
+			&[Perbill::from_percent(0)],
+		);
+
+		assert_eq!(Balances::free_balance(&11), 1000);
+		assert_eq!(Balances::free_balance(&101), 2000);
+
+		// This is the best way to check that the validator was chilled; `get` will
+		// return default value.
+		for (stash, _) in <Staking as Store>::Validators::enumerate() {
+			assert!(stash != 11);
+		}
+
+		let nominations = <Staking as Store>::Nominators::get(&101).unwrap();
+
+		// and make sure that the vote will not be ignored, because the slash was
+		// zero.
+		let last_slash = <Staking as Store>::SlashingSpans::get(&11).unwrap().last_nonzero_slash();
+		assert!(nominations.submitted_in >= last_slash);
+	});
+}