Jesse removal miss block

pallets/collator-selection/src/benchmarking.rs

@@ -27,6 +27,7 @@ use frame_support::{
 	traits::{Currency, Get, EnsureOrigin},
 };
 use pallet_authorship::EventHandler;
+use pallet_session::SessionManager;
 
 pub type BalanceOf<T> =
 	<<T as Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance;
@@ -75,7 +76,7 @@ benchmarks! {
 		assert_last_event::<T>(Event::NewInvulnerables(new_invulnerables).into());
 	}
 
-	set_max_candidates {
+	set_desired_candidates {
 		let max: u32 = 999;
 		let origin = T::UpdateOrigin::successful_origin();
 	}: {
@@ -133,6 +134,11 @@ benchmarks! {
 
 	// worse case is paying a non-existing candidate account.
 	note_author {
+		let c in 1 .. T::MaxCandidates::get();
+		<CandidacyBond<T>>::put(T::Currency::minimum_balance());
+		<DesiredCandidates<T>>::put(c);
+		register_candidates::<T>(c);
+
 		T::Currency::make_free_balance_be(
 			&<CollatorSelection<T>>::account_id(),
 			T::Currency::minimum_balance() * 2u32.into(),
@@ -144,6 +150,38 @@ benchmarks! {
 	} verify {
 		assert!(T::Currency::free_balance(&author) > 0u32.into());
 	}
+
+	// worse case is on new session.
+	// TODO review this benchmark
+	new_session {
+		let r in 1 .. T::MaxCandidates::get();
+		let c in 1 .. T::MaxCandidates::get();
+
+		<CandidacyBond<T>>::put(T::Currency::minimum_balance());
+		<DesiredCandidates<T>>::put(c);
+		frame_system::Pallet::<T>::set_block_number(0u32.into());
+		register_candidates::<T>(c);
+
+		let new_block: T::BlockNumber = 20u32.into();
+
+		let mut candidates = <Candidates<T>>::get();
+		let non_removals = if c > r { c - r } else { 0 };
+
+		for i in 0..non_removals {
+			candidates[i as usize].last_block = new_block;
+		}
+		<Candidates<T>>::put(candidates.clone());
+
+		let pre_length = <Candidates<T>>::get().len();
+		frame_system::Pallet::<T>::set_block_number(new_block.clone());
+
+		assert!(<Candidates<T>>::get().len() == c as usize);
+
+	}: {
+		<CollatorSelection<T> as SessionManager<_>>::new_session(0)
+	} verify {
+		assert!(<Candidates<T>>::get().len() <= pre_length);
+	}
 }
 
 impl_benchmark_test_suite!(CollatorSelection, crate::mock::new_test_ext(), crate::mock::Test,);

pallets/collator-selection/src/lib.rs

@@ -130,6 +130,9 @@ pub mod pallet {
 		/// Used only for benchmarking.
 		type MaxInvulnerables: Get<u32>;
 
+		// Will be kicked if block is not produced in threshold.
+		type KickThreshold: Get<Self::BlockNumber>;
+
 		/// The weight information of this pallet.
 		type WeightInfo: WeightInfo;
 	}
@@ -142,7 +145,7 @@ pub mod pallet {
 		/// Reserved deposit.
 		pub deposit: Balance,
 		/// Last block at which they authored a block.
-		pub last_block: Option<BlockNumber>,
+		pub last_block: BlockNumber,
 	}
 
 	#[pallet::pallet]
@@ -175,6 +178,7 @@ pub mod pallet {
 	#[pallet::getter(fn candidacy_bond)]
 	pub type CandidacyBond<T> = StorageValue<_, BalanceOf<T>, ValueQuery>;
 
+
 	#[pallet::genesis_config]
 	pub struct GenesisConfig<T: Config> {
 		pub invulnerables: Vec<T::AccountId>,
@@ -292,7 +296,7 @@ pub mod pallet {
 			ensure!(!Self::invulnerables().contains(&who), Error::<T>::AlreadyInvulnerable);
 
 			let deposit = Self::candidacy_bond();
-			let incoming = CandidateInfo { who: who.clone(), deposit, last_block: None };
+			let incoming = CandidateInfo { who: who.clone(), deposit, last_block: frame_system::Pallet::<T>::block_number() };
 
 			let current_count =
 				<Candidates<T>>::try_mutate(|candidates| -> Result<usize, DispatchError> {
@@ -313,14 +317,8 @@ pub mod pallet {
 		pub fn leave_intent(origin: OriginFor<T>) -> DispatchResultWithPostInfo {
 			let who = ensure_signed(origin)?;
 
-			let current_count = <Candidates<T>>::try_mutate(|candidates| -> Result<usize, DispatchError> {
-				let index = candidates.iter().position(|candidate| candidate.who == who).ok_or(Error::<T>::NotCandidate)?;
-				T::Currency::unreserve(&who, candidates[index].deposit);
-				candidates.remove(index);
-				Ok(candidates.len())
-			})?;
+			let current_count = Self::try_remove_candidate(&who)?;
 
-			Self::deposit_event(Event::CandidateRemoved(who));
 			Ok(Some(T::WeightInfo::leave_intent(current_count as u32)).into())
 		}
 	}
@@ -330,17 +328,47 @@ pub mod pallet {
 		pub fn account_id() -> T::AccountId {
 			T::PotId::get().into_account()
 		}
+		/// Removes a candidate if they exist and sends them back their deposit
+		fn try_remove_candidate(who: &T::AccountId) -> Result<usize, DispatchError> {
+			let current_count = <Candidates<T>>::try_mutate(|candidates| -> Result<usize, DispatchError> {
+				let index = candidates.iter().position(|candidate| candidate.who == *who).ok_or(Error::<T>::NotCandidate)?;
+				T::Currency::unreserve(&who, candidates[index].deposit);
+				candidates.remove(index);
+				Ok(candidates.len())
+			});
+			Self::deposit_event(Event::CandidateRemoved(who.clone()));
+			current_count
+		}
 
 		/// Assemble the current set of candidates and invulnerables into the next collator set.
 		///
 		/// This is done on the fly, as frequent as we are told to do so, as the session manager.
-		pub fn assemble_collators() -> Vec<T::AccountId> {
+		pub fn assemble_collators(candidates: Vec<T::AccountId>) -> Vec<T::AccountId> {
 			let mut collators = Self::invulnerables();
 			collators.extend(
-				Self::candidates().into_iter().map(|c| c.who).collect::<Vec<_>>(),
+				candidates.into_iter().collect::<Vec<_>>(),
 			);
 			collators
 		}
+		/// Kicks out and candidates that did not produce a block in the kick threshold.
+		pub fn kick_stale_candidates(candidates: Vec<CandidateInfo<T::AccountId, BalanceOf<T>, T::BlockNumber>>) -> Vec<T::AccountId> {
+			let now = frame_system::Pallet::<T>::block_number();
+			let kick_threshold = T::KickThreshold::get();
+			let new_candidates = candidates.into_iter().filter_map(|c| {
+				let since_last = now - c.last_block;
+				if since_last < kick_threshold {
+					Some(c.who)
+				} else {
+					let outcome = Self::try_remove_candidate(&c.who);
+					if let Err(why) = outcome {
+						log::warn!("Failed to remove candidate {:?}", why);
+						debug_assert!(false, "failed to remove candidate {:?}", why);
+					}
+					None
+				}
+			}).collect::<Vec<_>>();
+			new_candidates
+		}
 	}
 
 	/// Keep track of number of authored blocks per authority, uncles are counted as well since
@@ -355,9 +383,14 @@ pub mod pallet {
 			// `reward` is half of treasury account, this should never fail.
 			let _success = T::Currency::transfer(&treasury, &author, reward, KeepAlive);
 			debug_assert!(_success.is_ok());
-
+			let candidates_len = <Candidates<T>>::mutate(|candidates| -> usize {
+				if let Some(found) = candidates.iter_mut().find(|candidate| candidate.who == author) {
+					found.last_block = frame_system::Pallet::<T>::block_number();
+				}
+				candidates.len()
+			});
 			frame_system::Pallet::<T>::register_extra_weight_unchecked(
-				T::WeightInfo::note_author(),
+				T::WeightInfo::note_author(candidates_len as u32),
 				DispatchClass::Mandatory,
 			);
 		}
@@ -375,7 +408,19 @@ pub mod pallet {
 				index,
 				<frame_system::Pallet<T>>::block_number(),
 			);
-			Some(Self::assemble_collators())
+
+			let candidates = Self::candidates();
+			let candidates_len_before = candidates.len();
+			let active_candidates = Self::kick_stale_candidates(candidates);
+			let active_candidates_len = active_candidates.len();
+			let result = Self::assemble_collators(active_candidates);
+			let removed = candidates_len_before - active_candidates_len;
+
+			frame_system::Pallet::<T>::register_extra_weight_unchecked(
+				T::WeightInfo::new_session(candidates_len_before as u32, removed as u32),
+				DispatchClass::Mandatory,
+			);
+			Some(result)
 		}
 		fn start_session(_: SessionIndex) {
 			// we don't care.

pallets/collator-selection/src/mock.rs

@@ -194,6 +194,7 @@ impl Config for Test {
 	type PotId = PotId;
 	type MaxCandidates = MaxCandidates;
 	type MaxInvulnerables = MaxInvulnerables;
+	type KickThreshold = Period;
 	type WeightInfo = ();
 }
 
@@ -234,7 +235,6 @@ pub fn new_test_ext() -> sp_io::TestExternalities {
 
 pub fn initialize_to_block(n: u64) {
 	for i in System::block_number()+1..=n {
-		println!("init {}", i);
 		System::set_block_number(i);
 		<AllPallets as frame_support::traits::OnInitialize<u64>>::on_initialize(i);
 	}

pallets/collator-selection/src/tests.rs

@@ -124,7 +124,7 @@ fn cannot_register_dupe_candidate() {
 	new_test_ext().execute_with(|| {
 		// can add 3 as candidate
 		assert_ok!(CollatorSelection::register_as_candidate(Origin::signed(3)));
-		let addition = CandidateInfo { who: 3, deposit: 10, last_block: None };
+		let addition = CandidateInfo { who: 3, deposit: 10, last_block: 0 };
 		assert_eq!(CollatorSelection::candidates(), vec![addition]);
 		assert_eq!(Balances::free_balance(3), 90);
 
@@ -203,12 +203,21 @@ fn authorship_event_handler() {
 
 		// 4 is the default author.
 		assert_eq!(Balances::free_balance(4), 100);
-
+		assert_ok!(CollatorSelection::register_as_candidate(Origin::signed(4)));
 		// triggers `note_author`
 		Authorship::on_initialize(1);
 
+
+		let collator = CandidateInfo {
+			who: 4,
+			deposit: 10,
+			last_block: 0
+		};
+
+		assert_eq!(CollatorSelection::candidates(), vec![collator]);
+
 		// half of the pot goes to the collator who's the author (4 in tests).
-		assert_eq!(Balances::free_balance(4), 150);
+		assert_eq!(Balances::free_balance(4), 140);
 		// half stays.
 		assert_eq!(Balances::free_balance(CollatorSelection::account_id()), 50);
 	});
@@ -251,6 +260,29 @@ fn session_management_works() {
 	});
 }
 
+#[test]
+fn kick_mechanism() {
+	new_test_ext().execute_with(|| {
+		// add a new collator
+		assert_ok!(CollatorSelection::register_as_candidate(Origin::signed(3)));
+		assert_ok!(CollatorSelection::register_as_candidate(Origin::signed(4)));
+		assert_eq!(CollatorSelection::candidates().len(), 2);
+		initialize_to_block(21);
+		assert_eq!(SessionChangeBlock::get(), 20);
+		// 4 authored this block, gets to stay 3 was kicked
+		assert_eq!(CollatorSelection::candidates().len(), 1);
+		assert_eq!(SessionHandlerCollators::get(), vec![1, 2, 4]);
+		let collator = CandidateInfo {
+			who: 4,
+			deposit: 10,
+			last_block: 21
+		};
+		assert_eq!(CollatorSelection::candidates(), vec![collator]);
+		// kicked collator gets funds back
+		assert_eq!(Balances::free_balance(3), 100);
+	});
+}
+
 
 #[test]
 #[should_panic = "duplicate invulnerables in genesis."]

pallets/collator-selection/src/weights.rs

@@ -28,7 +28,8 @@ pub trait WeightInfo {
 	fn set_candidacy_bond() -> Weight;
 	fn register_as_candidate(_c: u32) -> Weight;
 	fn leave_intent(_c: u32) -> Weight;
-	fn note_author() -> Weight;
+	fn note_author(_c: u32) -> Weight;
+	fn new_session(_c: u32, _r: u32) -> Weight;
 }
 
 /// Weights for pallet_collator_selection using the Substrate node and recommended hardware.
@@ -62,10 +63,21 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 			.saturating_add(T::DbWeight::get().reads(1 as Weight))
 			.saturating_add(T::DbWeight::get().writes(1 as Weight))
 	}
-	fn note_author() -> Weight {
-		(97_000_000 as Weight)
+	fn note_author(c: u32, ) -> Weight {
+		(108_730_000 as Weight)
+			// Standard Error: 3_000
+			.saturating_add((286_000 as Weight).saturating_mul(c as Weight))
+			.saturating_add(T::DbWeight::get().reads(4 as Weight))
+			.saturating_add(T::DbWeight::get().writes(4 as Weight))
+	}
+	fn new_session(r: u32, c: u32, ) -> Weight {
+		(50_005_000 as Weight)
+			// Standard Error: 2_000
+			.saturating_add((8_000 as Weight).saturating_mul(r as Weight))
+			// Standard Error: 2_000
+			.saturating_add((291_000 as Weight).saturating_mul(c as Weight))
 			.saturating_add(T::DbWeight::get().reads(3 as Weight))
-			.saturating_add(T::DbWeight::get().writes(3 as Weight))
+			.saturating_add(T::DbWeight::get().writes(1 as Weight))
 	}
 }
 
@@ -100,9 +112,20 @@ impl WeightInfo for () {
 			.saturating_add(RocksDbWeight::get().reads(1 as Weight))
 			.saturating_add(RocksDbWeight::get().writes(1 as Weight))
 	}
-	fn note_author() -> Weight {
-		(97_000_000 as Weight)
+	fn note_author(c: u32, ) -> Weight {
+		(108_730_000 as Weight)
+			// Standard Error: 3_000
+			.saturating_add((286_000 as Weight).saturating_mul(c as Weight))
+			.saturating_add(RocksDbWeight::get().reads(4 as Weight))
+			.saturating_add(RocksDbWeight::get().writes(4 as Weight))
+	}
+	fn new_session(r: u32, c: u32, ) -> Weight {
+		(50_005_000 as Weight)
+			// Standard Error: 2_000
+			.saturating_add((8_000 as Weight).saturating_mul(r as Weight))
+			// Standard Error: 2_000
+			.saturating_add((291_000 as Weight).saturating_mul(c as Weight))
 			.saturating_add(RocksDbWeight::get().reads(3 as Weight))
-			.saturating_add(RocksDbWeight::get().writes(3 as Weight))
+			.saturating_add(RocksDbWeight::get().writes(1 as Weight))
 	}
 }

runtime/statemine/src/lib.rs

@@ -661,6 +661,8 @@ impl pallet_collator_selection::Config for Runtime {
 	type PotId = PotId;
 	type MaxCandidates = MaxCandidates;
 	type MaxInvulnerables = MaxInvulnerables;
+	// should be a multiple of session or things will get inconsistent
+	type KickThreshold = Period;
 	type WeightInfo = weights::pallet_collator_selection::WeightInfo<Runtime>;
 }
 
@@ -736,15 +738,15 @@ pub type Executive = frame_executive::Executive<
 >;
 
 impl_runtime_apis! {
-	// impl sp_consensus_aura::AuraApi<Block, AuraId> for Runtime {
-	// 	fn slot_duration() -> sp_consensus_aura::SlotDuration {
-	// 		sp_consensus_aura::SlotDuration::from_millis(Aura::slot_duration())
-	// 	}
-
-	// 	fn authorities() -> Vec<AuraId> {
-	// 		CollatorSelection::Collators()
-	// 	}
-	// }
+	impl sp_consensus_aura::AuraApi<Block, AuraId> for Runtime {
+		fn slot_duration() -> sp_consensus_aura::SlotDuration {
+			sp_consensus_aura::SlotDuration::from_millis(Aura::slot_duration())
+		}
+
+		fn authorities() -> Vec<AuraId> {
+			Aura::authorities()
+		}
+	}
 
 	impl sp_api::Core<Block> for Runtime {
 		fn version() -> RuntimeVersion {