fix non-candidate slashing (#1334)

* fix non-candidate slashing

* add call to update slash destination

* remove non-candidate from next session check

* additional check

* update storage query

* fix benchmark

pallets/collator-selection/src/benchmarking.rs

@@ -208,10 +208,10 @@ benchmarks! {
         assert_ok!(CollatorSelection::<T>::leave_intent(RawOrigin::Signed(leaving.clone()).into()));
         let session_length = <T as session::Config>::NextSessionRotation::average_session_length();
         session::Pallet::<T>::on_initialize(session_length);
-        assert_eq!(<NonCandidates<T>>::get(&leaving), (1u32, T::Currency::minimum_balance()));
+        assert_eq!(<NonCandidates<T>>::get(&leaving), Some((1u32, T::Currency::minimum_balance())));
     }: _(RawOrigin::Signed(leaving.clone()))
     verify {
-        assert_eq!(<NonCandidates<T>>::get(&leaving), (0u32, BalanceOf::<T>::default()));
+        assert_eq!(<NonCandidates<T>>::get(&leaving), None);
     }
 
     // worse case is paying a non-existing candidate account.

pallets/collator-selection/src/lib.rs

@@ -198,7 +198,7 @@ pub mod pallet {
     /// Candidates who initiated leave intent or kicked.
     #[pallet::storage]
     pub type NonCandidates<T: Config> =
-        StorageMap<_, Twox64Concat, T::AccountId, (SessionIndex, BalanceOf<T>), ValueQuery>;
+        StorageMap<_, Twox64Concat, T::AccountId, (SessionIndex, BalanceOf<T>), OptionQuery>;
 
     /// Last block authored by collator.
     #[pallet::storage]
@@ -219,7 +219,7 @@ pub mod pallet {
 
     /// Destination account for slashed amount.
     #[pallet::storage]
-    pub type SlashDestination<T> = StorageValue<_, <T as frame_system::Config>::AccountId>;
+    pub type SlashDestination<T: Config> = StorageValue<_, T::AccountId, OptionQuery>;
 
     #[pallet::genesis_config]
     #[derive(DefaultNoBound)]
@@ -480,6 +480,22 @@ pub mod pallet {
 
             Ok(())
         }
+
+        /// Set slash destination.
+        /// Use `Some` to deposit slashed balance into destination or `None` to burn it.
+        #[pallet::call_index(6)]
+        #[pallet::weight(T::DbWeight::get().reads_writes(1, 1))]
+        pub fn set_slash_destination(
+            origin: OriginFor<T>,
+            destination: Option<T::AccountId>,
+        ) -> DispatchResult {
+            T::UpdateOrigin::ensure_origin(origin)?;
+            match destination {
+                Some(account) => <SlashDestination<T>>::put(account),
+                None => <SlashDestination<T>>::kill(),
+            }
+            Ok(())
+        }
     }
 
     impl<T: Config> Pallet<T> {
@@ -545,16 +561,21 @@ pub mod pallet {
                 if now.saturating_sub(last_authored) < kick_threshold {
                     continue;
                 }
-                // still candidate, kick and slash
+                // stale candidate, kick and slash
                 if Self::is_account_candidate(&who) {
                     if Candidates::<T>::get().len() > T::MinCandidates::get() as usize {
                         // no error, who is a candidate
                         let _ = Self::try_remove_candidate(&who);
                         Self::slash_non_candidate(&who);
                     }
-                } else {
-                    // slash un-bonding candidate
-                    Self::slash_non_candidate(&who);
+                } else if let Some((locked_until, _)) = NonCandidates::<T>::get(&who) {
+                    if T::ValidatorSet::session_index() > locked_until {
+                        // bond is already unlocked
+                        <LastAuthoredBlock<T>>::remove(who);
+                    } else {
+                        // slash un-bonding candidate
+                        Self::slash_non_candidate(&who);
+                    }
                 }
             }
             (

pallets/collator-selection/src/tests.rs

@@ -18,7 +18,7 @@
 use crate as collator_selection;
 use crate::{
     mock::*, CandidacyBond, CandidateInfo, Candidates, DesiredCandidates, Error, Invulnerables,
-    LastAuthoredBlock, NonCandidates,
+    LastAuthoredBlock, NonCandidates, SlashDestination,
 };
 use frame_support::{
     assert_noop, assert_ok,
@@ -282,7 +282,7 @@ fn leave_intent() {
         assert_eq!(Balances::reserved_balance(3), 10);
         assert_eq!(LastAuthoredBlock::<Test>::get(3), 10);
         // 10 unbonding from session 1
-        assert_eq!(NonCandidates::<Test>::get(3), (1, 10));
+        assert_eq!(NonCandidates::<Test>::get(3), Some((1, 10)));
     });
 }
 
@@ -313,7 +313,7 @@ fn withdraw_unbond() {
 
         initialize_to_block(10);
         assert_ok!(CollatorSelection::withdraw_bond(RuntimeOrigin::signed(3)));
-        assert_eq!(NonCandidates::<Test>::get(3), (0, 0));
+        assert_eq!(NonCandidates::<Test>::get(3), None);
         assert_eq!(Balances::free_balance(3), 100);
         assert_eq!(Balances::reserved_balance(3), 0);
 
@@ -493,7 +493,7 @@ fn kick_and_slash_mechanism() {
 }
 
 #[test]
-fn slash_mechanism_for_unbonding_candidates() {
+fn slash_mechanism_for_unbonding_candidates_who_missed_block() {
     new_test_ext().execute_with(|| {
         // Define slash destination account
         <crate::SlashDestination<Test>>::put(5);
@@ -538,6 +538,45 @@ fn slash_mechanism_for_unbonding_candidates() {
     });
 }
 
+#[test]
+fn should_not_slash_unbonding_candidates() {
+    new_test_ext().execute_with(|| {
+        // add a new collator
+        assert_ok!(CollatorSelection::register_as_candidate(
+            RuntimeOrigin::signed(3)
+        ));
+        assert_ok!(CollatorSelection::register_as_candidate(
+            RuntimeOrigin::signed(4)
+        ));
+        assert_eq!(LastAuthoredBlock::<Test>::get(3), 10);
+        assert_eq!(LastAuthoredBlock::<Test>::get(4), 10);
+
+        assert_ok!(CollatorSelection::leave_intent(RuntimeOrigin::signed(3)));
+        // can withdraw on next session
+        assert_eq!(NonCandidates::<Test>::get(3), Some((1, 10)));
+
+        initialize_to_block(10);
+        // not included next session and doesn't withdraw bond
+        assert_eq!(NextSessionCollators::get(), vec![1, 2, 4]);
+        assert_eq!(LastAuthoredBlock::<Test>::get(3), 10);
+        assert_eq!(LastAuthoredBlock::<Test>::get(4), 10);
+        assert_eq!(NonCandidates::<Test>::get(3), Some((1, 10)));
+        assert_eq!(Balances::free_balance(3), 90);
+
+        initialize_to_block(20);
+        assert_eq!(SessionChangeBlock::get(), 20);
+        assert!(!LastAuthoredBlock::<Test>::contains_key(3));
+        assert_eq!(LastAuthoredBlock::<Test>::get(4), 20);
+
+        assert_eq!(NonCandidates::<Test>::get(3), Some((1, 10)));
+        assert_eq!(Balances::free_balance(3), 90);
+
+        assert_ok!(CollatorSelection::withdraw_bond(RuntimeOrigin::signed(3)));
+        assert_eq!(NonCandidates::<Test>::get(3), None);
+        assert_eq!(Balances::free_balance(3), 100);
+    });
+}
+
 #[test]
 fn should_not_kick_mechanism_too_few() {
     new_test_ext().execute_with(|| {
@@ -589,3 +628,30 @@ fn cannot_set_genesis_value_twice() {
     // collator selection must be initialized before session.
     collator_selection.assimilate_storage(&mut t).unwrap();
 }
+
+#[test]
+fn set_slash_destination() {
+    new_test_ext().execute_with(|| {
+        assert_eq!(SlashDestination::<Test>::get(), None);
+
+        // only UpdateOrigin can update
+        assert_noop!(
+            CollatorSelection::set_slash_destination(RuntimeOrigin::signed(1), Some(1)),
+            sp_runtime::DispatchError::BadOrigin
+        );
+
+        // set destination
+        assert_ok!(CollatorSelection::set_slash_destination(
+            RuntimeOrigin::signed(RootAccount::get()),
+            Some(1),
+        ));
+        assert_eq!(SlashDestination::<Test>::get(), Some(1));
+
+        // remove destination
+        assert_ok!(CollatorSelection::set_slash_destination(
+            RuntimeOrigin::signed(RootAccount::get()),
+            None,
+        ));
+        assert_eq!(SlashDestination::<Test>::get(), None);
+    });
+}