Correct detection of handover completion

substrate/validator-sets/pallet/src/lib.rs

@@ -643,8 +643,9 @@ pub mod pallet {
     // Checks if this session has completed the handover from the prior session.
     fn handover_completed(network: NetworkId, session: Session) -> bool {
       let Some(current_session) = Self::session(network) else { return false };
-      // No handover occurs on genesis
-      if current_session.0 == 0 {
+
+      // If the session we've been queried about is old, it must have completed its handover
+      if current_session.0 > session.0 {
         return true;
       }
       // If the session we've been queried about has yet to start, it can't have completed its
@@ -652,19 +653,21 @@ pub mod pallet {
       if current_session.0 < session.0 {
         return false;
       }
-      if current_session.0 == session.0 {
-        // Handover is automatically complete for Serai as it doesn't have a handover protocol
-        // If not Serai, check the prior session had its keys cleared, which happens once its
-        // retired
-        return (network == NetworkId::Serai) ||
-          (!Keys::<T>::contains_key(ValidatorSet {
-            network,
-            session: Session(current_session.0 - 1),
-          }));
+
+      // Handover is automatically complete for Serai as it doesn't have a handover protocol
+      if network == NetworkId::Serai {
+        return true;
       }
-      // We're currently in a future session, meaning this session definitely performed itself
-      // handover
-      true
+
+      // The current session must have set keys for its handover to be completed
+      if !Keys::<T>::contains_key(ValidatorSet { network, session }) {
+        return false;
+      }
+
+      // This must be the first session (which has set keys) OR the prior session must have been
+      // retired (signified by its keys no longer being present)
+      (session.0 == 0) ||
+        (!Keys::<T>::contains_key(ValidatorSet { network, session: Session(session.0 - 1) }))
     }
 
     fn new_session() {
@@ -682,6 +685,8 @@ pub mod pallet {
       }
     }
 
+    // TODO: This is called retire_set, yet just starts retiring the set
+    // Update the nomenclature within this function
     pub fn retire_set(set: ValidatorSet) {
       // If the prior prior set didn't report, emit they're retired now
       if PendingSlashReport::<T>::get(set.network).is_some() {

tests/coordinator/src/tests/key_gen.rs

@@ -10,7 +10,6 @@ use ciphersuite::{
   group::{ff::Field, GroupEncoding},
   Ciphersuite, Ristretto, Secp256k1,
 };
-use dkg::ThresholdParams;
 
 use serai_client::{
   primitives::NetworkId,
@@ -41,8 +40,8 @@ pub async fn key_gen<C: Ciphersuite>(
           shares,
         }) => {
           assert_eq!(id, *this_id);
-          assert_eq!(params.t(), u16::try_from(((coordinators * 2) / 3) + 1).unwrap(),);
-          assert_eq!(params.n(), u16::try_from(coordinators).unwrap(),);
+          assert_eq!(params.t(), u16::try_from(((coordinators * 2) / 3) + 1).unwrap());
+          assert_eq!(params.n(), u16::try_from(coordinators).unwrap());
           assert_eq!(*shares, 1);
           participant_is.push(params.i());
           break;

tests/coordinator/src/tests/rotation.rs

@@ -83,46 +83,45 @@ async fn deallocate_stake(
   publish_tx(serai, &tx).await
 }
 
-async fn wait_till_next_epoch(serai: &Serai, current_epoch: u32) -> Session {
-  let mut session = Session(current_epoch);
-  while session.0 < current_epoch + 1 {
+async fn get_session(serai: &Serai, network: NetworkId) -> Session {
+  serai
+    .as_of_latest_finalized_block()
+    .await
+    .unwrap()
+    .validator_sets()
+    .session(network)
+    .await
+    .unwrap()
+    .unwrap()
+}
+
+async fn wait_till_next_epoch(serai: &Serai) -> Session {
+  let starting_session = get_session(serai, NetworkId::Serai).await;
+
+  let mut session = starting_session;
+  while session == starting_session {
     sleep(Duration::from_secs(6)).await;
-    session = serai
-      .as_of_latest_finalized_block()
-      .await
-      .unwrap()
-      .validator_sets()
-      .session(NetworkId::Serai)
-      .await
-      .unwrap()
-      .unwrap();
+    session = get_session(serai, NetworkId::Serai).await;
   }
   session
 }
 
-async fn get_session(serai: &Serai, block: [u8; 32], network: NetworkId) -> Session {
-  serai.as_of(block).validator_sets().session(network).await.unwrap().unwrap()
-}
-
-async fn new_set_events(
-  serai: &Serai,
-  session: Session,
-  network: NetworkId,
-) -> Vec<ValidatorSetsEvent> {
+async fn most_recent_new_set_event(serai: &Serai, network: NetworkId) -> ValidatorSetsEvent {
   let mut current_block = serai.latest_finalized_block().await.unwrap();
-  let mut current_session = get_session(serai, current_block.hash(), network).await;
-
-  while current_session == session {
+  loop {
     let events = serai.as_of(current_block.hash()).validator_sets().new_set_events().await.unwrap();
-    if !events.is_empty() {
-      return events;
+    for event in events {
+      match event {
+        ValidatorSetsEvent::NewSet { set } => {
+          if set.network == network {
+            return event;
+          }
+        }
+        _ => panic!("new_set_events gave non-NewSet event: {event:?}"),
+      }
     }
-
     current_block = serai.block(current_block.header.parent_hash.0).await.unwrap().unwrap();
-    current_session = get_session(serai, current_block.hash(), network).await;
   }
-
-  panic!("can't find the new set events for session: {} ", session.0);
 }
 
 #[tokio::test]
@@ -133,30 +132,29 @@ async fn set_rotation_test() {
       let excluded = processors.pop().unwrap();
       assert_eq!(processors.len(), COORDINATORS);
 
-      // genesis keygen
-      let _ = key_gen::<Secp256k1>(&mut processors, Session(0)).await;
-
       let pair5 = insecure_pair_from_name("Eve");
       let network = NetworkId::Bitcoin;
       let amount = Amount(1_000_000 * 10_u64.pow(8));
       let serai = processors[0].serai().await;
 
       // add the last participant into validator set for btc network
-      let block = allocate_stake(&serai, network, amount, &pair5, 0).await;
+      allocate_stake(&serai, network, amount, &pair5, 0).await;
+
+      // genesis keygen
+      let _ = key_gen::<Secp256k1>(&mut processors, Session(0)).await;
 
       // wait until next session to see the effect on coordinator
-      let current_epoch = get_session(&serai, block, NetworkId::Serai).await;
-      let session = wait_till_next_epoch(&serai, current_epoch.0).await;
+      wait_till_next_epoch(&serai).await;
 
       // verfiy that coordinator received new_set
-      let events = new_set_events(&serai, session, network).await;
-      assert!(
-        events.contains(&ValidatorSetsEvent::NewSet { set: ValidatorSet { session, network } })
+      assert_eq!(
+        most_recent_new_set_event(&serai, network).await,
+        ValidatorSetsEvent::NewSet { set: ValidatorSet { session: Session(1), network } },
       );
 
       // add the last participant & do the keygen
       processors.push(excluded);
-      let _ = key_gen::<Secp256k1>(&mut processors, session).await;
+      let _ = key_gen::<Secp256k1>(&mut processors, Session(1)).await;
     },
     true,
   )