Update logic to cancel_deferred_slash (#5186)

* Update logic to cancel_deferred_slash

* More idiomatic

* bump spec

Co-authored-by: Gavin Wood <gavin@parity.io>

substrate/frame/staking/src/lib.rs

@@ -928,6 +928,8 @@ decl_error! {
 		InvalidEraToReward,
 		/// Invalid number of nominations.
 		InvalidNumberOfNominations,
+		/// Items are not sorted and unique.
+		NotSortedAndUnique,
 	}
 }
 
@@ -1327,21 +1329,15 @@ decl_module! {
 				.map(|_| ())
 				.or_else(ensure_root)?;
 
-			let mut slash_indices = slash_indices;
-			slash_indices.sort_unstable();
+			ensure!(!slash_indices.is_empty(), Error::<T>::EmptyTargets);
+			ensure!(Self::is_sorted_and_unique(&slash_indices), Error::<T>::NotSortedAndUnique);
+
 			let mut unapplied = <Self as Store>::UnappliedSlashes::get(&era);
+			let last_item = slash_indices[slash_indices.len() - 1];
+			ensure!((last_item as usize) < unapplied.len(), Error::<T>::InvalidSlashIndex);
 
 			for (removed, index) in slash_indices.into_iter().enumerate() {
-				let index = index as usize;
-
-				// if `index` is not duplicate, `removed` must be <= index.
-				ensure!(removed <= index, Error::<T>::DuplicateIndex);
-
-				// all prior removals were from before this index, since the
-				// list is sorted.
-				let index = index - removed;
-				ensure!(index < unapplied.len(), Error::<T>::InvalidSlashIndex);
-
+				let index = (index as usize) - removed;
 				unapplied.remove(index);
 			}
 
@@ -1411,7 +1407,7 @@ decl_module! {
 			let controller = ensure_signed(origin)?;
 			let ledger = Self::ledger(&controller).ok_or(Error::<T>::NotController)?;
 			ensure!(
-				ledger.unlocking.len() > 0,
+				!ledger.unlocking.is_empty(),
 				Error::<T>::NoUnlockChunk,
 			);
 
@@ -1460,6 +1456,11 @@ impl<T: Trait> Module<T> {
 		Self::bonded(stash).and_then(Self::ledger).map(|l| l.active).unwrap_or_default()
 	}
 
+	/// Check that list is sorted and has no duplicates.
+	fn is_sorted_and_unique(list: &Vec<u32>) -> bool {
+		list.windows(2).all(|w| w[0] < w[1])
+	}
+
 	// MUTABLES (DANGEROUS)
 
 	fn do_payout_nominator(who: T::AccountId, era: EraIndex, validators: Vec<(T::AccountId, u32)>)

substrate/frame/staking/src/tests.rs

@@ -2616,7 +2616,13 @@ fn remove_deferred() {
 			1,
 		);
 
-		Staking::cancel_deferred_slash(Origin::ROOT, 1, vec![0]).unwrap();
+		// fails if empty
+		assert_noop!(
+			Staking::cancel_deferred_slash(Origin::ROOT, 1, vec![]),
+			Error::<Test>::EmptyTargets
+		);
+
+		assert_ok!(Staking::cancel_deferred_slash(Origin::ROOT, 1, vec![0]));
 
 		assert_eq!(Balances::free_balance(11), 1000);
 		assert_eq!(Balances::free_balance(101), 2000);
@@ -2689,12 +2695,51 @@ fn remove_multi_deferred() {
 			&[Perbill::from_percent(25)],
 		);
 
-		assert_eq!(<Staking as Store>::UnappliedSlashes::get(&1).len(), 3);
-		Staking::cancel_deferred_slash(Origin::ROOT, 1, vec![0, 2]).unwrap();
+		on_offence_now(
+			&[
+				OffenceDetails {
+					offender: (42, exposure.clone()),
+					reporters: vec![],
+				},
+			],
+			&[Perbill::from_percent(25)],
+		);
+
+		on_offence_now(
+			&[
+				OffenceDetails {
+					offender: (69, exposure.clone()),
+					reporters: vec![],
+				},
+			],
+			&[Perbill::from_percent(25)],
+		);
+
+		assert_eq!(<Staking as Store>::UnappliedSlashes::get(&1).len(), 5);
+
+		// fails if list is not sorted
+		assert_noop!(
+			Staking::cancel_deferred_slash(Origin::ROOT, 1, vec![2, 0, 4]),
+			Error::<Test>::NotSortedAndUnique
+		);
+		// fails if list is not unique
+		assert_noop!(
+			Staking::cancel_deferred_slash(Origin::ROOT, 1, vec![0, 2, 2]),
+			Error::<Test>::NotSortedAndUnique
+		);
+		// fails if bad index
+		assert_noop!(
+			Staking::cancel_deferred_slash(Origin::ROOT, 1, vec![1, 2, 3, 4, 5]),
+			Error::<Test>::InvalidSlashIndex
+		);
+
+		assert_ok!(Staking::cancel_deferred_slash(Origin::ROOT, 1, vec![0, 2, 4]));
 
 		let slashes = <Staking as Store>::UnappliedSlashes::get(&1);
-		assert_eq!(slashes.len(), 1);
+		assert_eq!(slashes.len(), 2);
+		println!("Slashes: {:?}", slashes);
 		assert_eq!(slashes[0].validator, 21);
+		assert_eq!(slashes[1].validator, 42);
 	})
 }
 
@@ -2994,4 +3039,3 @@ fn set_history_depth_works() {
 		assert!(!<Staking as Store>::ErasTotalStake::contains_key(10 - 5));
 	});
 }
-