diff --git a/pallets/collator-selection/src/benchmarking.rs b/pallets/collator-selection/src/benchmarking.rs index c5103eb9b..35a0da26c 100644 --- a/pallets/collator-selection/src/benchmarking.rs +++ b/pallets/collator-selection/src/benchmarking.rs @@ -208,10 +208,10 @@ benchmarks! { assert_ok!(CollatorSelection::::leave_intent(RawOrigin::Signed(leaving.clone()).into())); let session_length = ::NextSessionRotation::average_session_length(); session::Pallet::::on_initialize(session_length); - assert_eq!(>::get(&leaving), (1u32, T::Currency::minimum_balance())); + assert_eq!(>::get(&leaving), Some((1u32, T::Currency::minimum_balance()))); }: _(RawOrigin::Signed(leaving.clone())) verify { - assert_eq!(>::get(&leaving), (0u32, BalanceOf::::default())); + assert_eq!(>::get(&leaving), None); } // worse case is paying a non-existing candidate account. diff --git a/pallets/collator-selection/src/lib.rs b/pallets/collator-selection/src/lib.rs index ad181c9d0..0077c5e45 100644 --- a/pallets/collator-selection/src/lib.rs +++ b/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 = - StorageMap<_, Twox64Concat, T::AccountId, (SessionIndex, BalanceOf), ValueQuery>; + StorageMap<_, Twox64Concat, T::AccountId, (SessionIndex, BalanceOf), 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 = StorageValue<_, ::AccountId>; + pub type SlashDestination = 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, + destination: Option, + ) -> DispatchResult { + T::UpdateOrigin::ensure_origin(origin)?; + match destination { + Some(account) => >::put(account), + None => >::kill(), + } + Ok(()) + } } impl Pallet { @@ -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::::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::::get(&who) { + if T::ValidatorSet::session_index() > locked_until { + // bond is already unlocked + >::remove(who); + } else { + // slash un-bonding candidate + Self::slash_non_candidate(&who); + } } } ( diff --git a/pallets/collator-selection/src/tests.rs b/pallets/collator-selection/src/tests.rs index fe064c2e3..1e4338b63 100644 --- a/pallets/collator-selection/src/tests.rs +++ b/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::::get(3), 10); // 10 unbonding from session 1 - assert_eq!(NonCandidates::::get(3), (1, 10)); + assert_eq!(NonCandidates::::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::::get(3), (0, 0)); + assert_eq!(NonCandidates::::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 >::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::::get(3), 10); + assert_eq!(LastAuthoredBlock::::get(4), 10); + + assert_ok!(CollatorSelection::leave_intent(RuntimeOrigin::signed(3))); + // can withdraw on next session + assert_eq!(NonCandidates::::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::::get(3), 10); + assert_eq!(LastAuthoredBlock::::get(4), 10); + assert_eq!(NonCandidates::::get(3), Some((1, 10))); + assert_eq!(Balances::free_balance(3), 90); + + initialize_to_block(20); + assert_eq!(SessionChangeBlock::get(), 20); + assert!(!LastAuthoredBlock::::contains_key(3)); + assert_eq!(LastAuthoredBlock::::get(4), 20); + + assert_eq!(NonCandidates::::get(3), Some((1, 10))); + assert_eq!(Balances::free_balance(3), 90); + + assert_ok!(CollatorSelection::withdraw_bond(RuntimeOrigin::signed(3))); + assert_eq!(NonCandidates::::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::::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::::get(), Some(1)); + + // remove destination + assert_ok!(CollatorSelection::set_slash_destination( + RuntimeOrigin::signed(RootAccount::get()), + None, + )); + assert_eq!(SlashDestination::::get(), None); + }); +}