fix: keyholder check should use HistoricalActiveEpochs

This is more reliable than using HistoricalAuthorities because the latter is not cleared when an epoch expires.

Preivously is was possible for a validator to be considered active for an expired epoch if the epochs were expired out-of-order (see updated test case).

state-chain/pallets/cf-funding/src/lib.rs

@@ -270,7 +270,7 @@ pub mod pallet {
 		/// The account is already bound to an executor address.
 		ExecutorAddressAlreadyBound,
 
-		/// The account cannot be reaped before it is unregstered.
+		/// The account cannot be reaped before it is unregistered.
 		AccountMustBeUnregistered,
 	}
 

state-chain/pallets/cf-validator/src/lib.rs

@@ -751,11 +751,7 @@ pub mod pallet {
 				<T as frame_system::Config>::AccountId,
 			>>::from_ref(&account_id);
 
-			ensure!(
-				(LastExpiredEpoch::<T>::get() + 1..=CurrentEpoch::<T>::get())
-					.all(|epoch| !HistoricalAuthorities::<T>::get(epoch).contains(validator_id)),
-				Error::<T>::StillKeyHolder
-			);
+			ensure!(!EpochHistory::<T>::is_keyholder(validator_id), Error::<T>::StillKeyHolder);
 
 			// This can only error if the validator didn't register any keys, in which case we want
 			// to continue with the deregistration anyway.

state-chain/pallets/cf-validator/src/tests.rs

@@ -1232,16 +1232,20 @@ fn validator_deregistration_after_expired_epoch() {
 	new_test_ext().execute_with(|| {
 		assert_ok!(ValidatorPallet::register_as_validator(RuntimeOrigin::signed(ALICE),));
 		ValidatorPallet::transition_to_next_epoch(vec![1, ALICE], 100);
+		let first_epoch_to_expire = ValidatorPallet::current_epoch();
 
 		// Can't deregister
 		assert_noop!(
 			ValidatorPallet::deregister_as_validator(RuntimeOrigin::signed(ALICE),),
 			Error::<Test>::StillKeyHolder
 		);
 
-		let epoch_to_expire = ValidatorPallet::current_epoch();
+		ValidatorPallet::transition_to_next_epoch(vec![1, ALICE], 100);
+		let second_epoch_to_expire = ValidatorPallet::current_epoch();
 		ValidatorPallet::transition_to_next_epoch(vec![1, 2], 100);
-		ValidatorPallet::expire_epoch(epoch_to_expire);
+
+		ValidatorPallet::expire_epoch(second_epoch_to_expire);
+		ValidatorPallet::expire_epoch(first_epoch_to_expire);
 
 		// Now you can deregister
 		assert_ok!(ValidatorPallet::deregister_as_validator(RuntimeOrigin::signed(ALICE),));

state-chain/traits/src/lib.rs

@@ -676,6 +676,10 @@ pub trait HistoricalEpoch {
 	fn active_bond(authority: &Self::ValidatorId) -> Self::Amount;
 	/// Returns the number of active epochs a authority is still active in
 	fn number_of_active_epochs_for_authority(id: &Self::ValidatorId) -> u32;
+	/// Is the validator a keyholder for an active epoch?
+	fn is_keyholder(id: &Self::ValidatorId) -> bool {
+		Self::number_of_active_epochs_for_authority(id) > 0
+	}
 }
 
 /// Handles the bonding logic