Add coordinator rotation test (#535)

* add node side unit test

* complete rotation test for all networks

* set up the fast-epoch docker file

* fix pr comments

* add coordinator side rotation test

* bug fixes

* Remove EPOCH_INTERVAL

* Minor nits

* Add note on origin of publish_tx function in tests/coordinator

* Correct ThresholdParams assert_eq

* fmt

* Correct detection of handover completion

* Restore key gen message match from develop

It was modified in response to the handover completion bug, which has now been
resolved.

* bug fixes

* Correct invalid constant

* Typo fixes

* remove selecting participant to remove at random

---------

Co-authored-by: Luke Parker <lukeparker5132@gmail.com>

substrate/abi/src/grandpa.rs

@@ -1,4 +1,3 @@
-use sp_core::{ConstU32, bounded::BoundedVec};
 use sp_consensus_grandpa::EquivocationProof;
 
 use serai_primitives::{BlockNumber, SeraiAddress};
@@ -19,7 +18,7 @@ pub enum Call {
 #[cfg_attr(feature = "serde", derive(serde::Serialize))]
 #[cfg_attr(all(feature = "std", feature = "serde"), derive(serde::Deserialize))]
 pub enum Event {
-  NewAuthorities { authority_set: BoundedVec<(SeraiAddress, u64), ConstU32<0>> },
+  NewAuthorities { authority_set: alloc::vec::Vec<(SeraiAddress, u64)> },
   // TODO: Remove these
   Paused,
   Resumed,

substrate/client/src/serai/mod.rs

@@ -195,10 +195,10 @@ impl Serai {
   }
 
   async fn active_network_validators(&self, network: NetworkId) -> Result<Vec<Public>, SeraiError> {
-    let hash: String = self
+    let validators: String = self
       .call("state_call", ["SeraiRuntimeApi_validators".to_string(), hex::encode(network.encode())])
       .await?;
-    let bytes = Self::hex_decode(hash)?;
+    let bytes = Self::hex_decode(validators)?;
     let r = Vec::<Public>::decode(&mut bytes.as_slice())
       .map_err(|e| SeraiError::ErrorInResponse(e.to_string()))?;
     Ok(r)

substrate/client/tests/common/in_instructions.rs

@@ -31,7 +31,7 @@ pub async fn provide_batch(serai: &Serai, batch: Batch) -> [u8; 32] {
     keys
   } else {
     let keys = KeyPair(pair.public(), vec![].try_into().unwrap());
-    set_keys(serai, set, keys.clone()).await;
+    set_keys(serai, set, keys.clone(), &[insecure_pair_from_name("Alice")]).await;
     keys
   };
   assert_eq!(keys.0, pair.public());

substrate/client/tests/common/validator_sets.rs

@@ -14,7 +14,6 @@ use frost::dkg::musig::musig;
 use schnorrkel::Schnorrkel;
 
 use serai_client::{
-  primitives::insecure_pair_from_name,
   validator_sets::{
     primitives::{ValidatorSet, KeyPair, musig_context, set_keys_message},
     ValidatorSetsEvent,
@@ -25,26 +24,40 @@ use serai_client::{
 use crate::common::tx::publish_tx;
 
 #[allow(dead_code)]
-pub async fn set_keys(serai: &Serai, set: ValidatorSet, key_pair: KeyPair) -> [u8; 32] {
-  let pair = insecure_pair_from_name("Alice");
-  let public = pair.public();
+pub async fn set_keys(
+  serai: &Serai,
+  set: ValidatorSet,
+  key_pair: KeyPair,
+  pairs: &[Pair],
+) -> [u8; 32] {
+  let mut pub_keys = vec![];
+  for pair in pairs {
+    let public_key =
+      <Ristretto as Ciphersuite>::read_G::<&[u8]>(&mut pair.public().0.as_ref()).unwrap();
+    pub_keys.push(public_key);
+  }
 
-  let public_key = <Ristretto as Ciphersuite>::read_G::<&[u8]>(&mut public.0.as_ref()).unwrap();
-  let secret_key = <Ristretto as Ciphersuite>::read_F::<&[u8]>(
-    &mut pair.as_ref().secret.to_bytes()[.. 32].as_ref(),
-  )
-  .unwrap();
-  assert_eq!(Ristretto::generator() * secret_key, public_key);
-  let threshold_keys =
-    musig::<Ristretto>(&musig_context(set), &Zeroizing::new(secret_key), &[public_key]).unwrap();
+  let mut threshold_keys = vec![];
+  for i in 0 .. pairs.len() {
+    let secret_key = <Ristretto as Ciphersuite>::read_F::<&[u8]>(
+      &mut pairs[i].as_ref().secret.to_bytes()[.. 32].as_ref(),
+    )
+    .unwrap();
+    assert_eq!(Ristretto::generator() * secret_key, pub_keys[i]);
+
+    threshold_keys.push(
+      musig::<Ristretto>(&musig_context(set), &Zeroizing::new(secret_key), &pub_keys).unwrap(),
+    );
+  }
+
+  let mut musig_keys = HashMap::new();
+  for tk in threshold_keys {
+    musig_keys.insert(tk.params().i(), tk.into());
+  }
 
   let sig = frost::tests::sign_without_caching(
     &mut OsRng,
-    frost::tests::algorithm_machines(
-      &mut OsRng,
-      &Schnorrkel::new(b"substrate"),
-      &HashMap::from([(threshold_keys.params().i(), threshold_keys.into())]),
-    ),
+    frost::tests::algorithm_machines(&mut OsRng, &Schnorrkel::new(b"substrate"), &musig_keys),
     &set_keys_message(&set, &[], &key_pair),
   );
 

substrate/client/tests/validator_sets.rs

@@ -1,36 +1,71 @@
 use rand_core::{RngCore, OsRng};
 
-use sp_core::{sr25519::Public, Pair};
+use sp_core::{
+  sr25519::{Public, Pair},
+  Pair as PairTrait,
+};
 
 use serai_client::{
-  primitives::{NETWORKS, NetworkId, insecure_pair_from_name},
+  primitives::{NETWORKS, NetworkId, BlockHash, insecure_pair_from_name},
   validator_sets::{
     primitives::{Session, ValidatorSet, KeyPair},
     ValidatorSetsEvent,
   },
+  in_instructions::{
+    primitives::{Batch, SignedBatch, batch_message},
+    SeraiInInstructions,
+  },
   Amount, Serai,
 };
 
 mod common;
-use common::validator_sets::{set_keys, allocate_stake, deallocate_stake};
+use common::{
+  tx::publish_tx,
+  validator_sets::{allocate_stake, deallocate_stake, set_keys},
+};
 
-const EPOCH_INTERVAL: u64 = 5;
+fn get_random_key_pair() -> KeyPair {
+  let mut ristretto_key = [0; 32];
+  OsRng.fill_bytes(&mut ristretto_key);
+  let mut external_key = vec![0; 33];
+  OsRng.fill_bytes(&mut external_key);
+  KeyPair(Public(ristretto_key), external_key.try_into().unwrap())
+}
+
+async fn get_ordered_keys(serai: &Serai, network: NetworkId, accounts: &[Pair]) -> Vec<Pair> {
+  // retrieve the current session validators so that we know the order of the keys
+  // that is necessary for the correct musig signature.
+  let validators = serai
+    .as_of_latest_finalized_block()
+    .await
+    .unwrap()
+    .validator_sets()
+    .active_network_validators(network)
+    .await
+    .unwrap();
+
+  // collect the pairs of the validators
+  let mut pairs = vec![];
+  for v in validators {
+    let p = accounts.iter().find(|pair| pair.public() == v).unwrap().clone();
+    pairs.push(p);
+  }
+
+  pairs
+}
 
 serai_test!(
   set_keys_test: (|serai: Serai| async move {
     let network = NetworkId::Bitcoin;
     let set = ValidatorSet { session: Session(0), network };
 
-    let public = insecure_pair_from_name("Alice").public();
+    let pair = insecure_pair_from_name("Alice");
+    let public = pair.public();
 
     // Neither of these keys are validated
     // The external key is infeasible to validate on-chain, the Ristretto key is feasible
     // TODO: Should the Ristretto key be validated?
-    let mut ristretto_key = [0; 32];
-    OsRng.fill_bytes(&mut ristretto_key);
-    let mut external_key = vec![0; 33];
-    OsRng.fill_bytes(&mut external_key);
-    let key_pair = KeyPair(Public(ristretto_key), external_key.try_into().unwrap());
+    let key_pair = get_random_key_pair();
 
     // Make sure the genesis is as expected
     assert_eq!(
@@ -62,7 +97,7 @@ serai_test!(
       assert_eq!(participants_ref, [public].as_ref());
     }
 
-    let block = set_keys(&serai, set, key_pair.clone()).await;
+    let block = set_keys(&serai, set, key_pair.clone(), &[pair]).await;
 
     // While the set_keys function should handle this, it's beneficial to
     // independently test it
@@ -149,11 +184,13 @@ async fn validator_set_rotation() {
       );
 
       // genesis accounts
-      let pair1 = insecure_pair_from_name("Alice");
-      let pair2 = insecure_pair_from_name("Bob");
-      let pair3 = insecure_pair_from_name("Charlie");
-      let pair4 = insecure_pair_from_name("Dave");
-      let pair5 = insecure_pair_from_name("Eve");
+      let accounts = vec![
+        insecure_pair_from_name("Alice"),
+        insecure_pair_from_name("Bob"),
+        insecure_pair_from_name("Charlie"),
+        insecure_pair_from_name("Dave"),
+        insecure_pair_from_name("Eve"),
+      ];
 
       // amounts for single key share per network
       let key_shares = HashMap::from([
@@ -164,8 +201,9 @@ async fn validator_set_rotation() {
       ]);
 
       // genesis participants per network
+      #[allow(clippy::redundant_closure_for_method_calls)]
       let default_participants =
-        vec![pair1.public(), pair2.public(), pair3.public(), pair4.public()];
+        accounts[.. 4].to_vec().iter().map(|pair| pair.public()).collect::<Vec<_>>();
       let mut participants = HashMap::from([
         (NetworkId::Serai, default_participants.clone()),
         (NetworkId::Bitcoin, default_participants.clone()),
@@ -181,28 +219,83 @@ async fn validator_set_rotation() {
         participants.sort();
         verify_session_and_active_validators(&serai, network, 0, participants).await;
 
-        // add 1 participant & verify
-        let hash =
-          allocate_stake(&serai, network, key_shares[&network], &pair5, i.try_into().unwrap())
-            .await;
-        participants.push(pair5.public());
-        participants.sort();
-        verify_session_and_active_validators(
+        // add 1 participant
+        let last_participant = accounts[4].clone();
+        let hash = allocate_stake(
           &serai,
           network,
-          get_active_session(&serai, network, hash).await,
-          participants,
+          key_shares[&network],
+          &last_participant,
+          i.try_into().unwrap(),
         )
         .await;
+        participants.push(last_participant.public());
+        // the session at which set changes becomes active
+        let activation_session = get_session_at_which_changes_activate(&serai, network, hash).await;
 
-        // remove 1 participant & verify
-        let hash =
-          deallocate_stake(&serai, network, key_shares[&network], &pair2, i.try_into().unwrap())
-            .await;
-        participants.swap_remove(participants.iter().position(|k| *k == pair2.public()).unwrap());
-        let active_session = get_active_session(&serai, network, hash).await;
+        // set the keys if it is an external set
+        if network != NetworkId::Serai {
+          let set = ValidatorSet { session: Session(0), network };
+          let key_pair = get_random_key_pair();
+          let pairs = get_ordered_keys(&serai, network, &accounts).await;
+          set_keys(&serai, set, key_pair, &pairs).await;
+        }
+
+        // verify
         participants.sort();
-        verify_session_and_active_validators(&serai, network, active_session, participants).await;
+        verify_session_and_active_validators(&serai, network, activation_session, participants)
+          .await;
+
+        // remove 1 participant
+        let participant_to_remove = accounts[1].clone();
+        let hash = deallocate_stake(
+          &serai,
+          network,
+          key_shares[&network],
+          &participant_to_remove,
+          i.try_into().unwrap(),
+        )
+        .await;
+        participants.swap_remove(
+          participants.iter().position(|k| *k == participant_to_remove.public()).unwrap(),
+        );
+        let activation_session = get_session_at_which_changes_activate(&serai, network, hash).await;
+
+        if network != NetworkId::Serai {
+          // set the keys if it is an external set
+          let set = ValidatorSet { session: Session(1), network };
+
+          // we need the whole substrate key pair to sign the batch
+          let (substrate_pair, key_pair) = {
+            let pair = insecure_pair_from_name("session-1-key-pair");
+            let public = pair.public();
+
+            let mut external_key = vec![0; 33];
+            OsRng.fill_bytes(&mut external_key);
+
+            (pair, KeyPair(public, external_key.try_into().unwrap()))
+          };
+          let pairs = get_ordered_keys(&serai, network, &accounts).await;
+          set_keys(&serai, set, key_pair, &pairs).await;
+
+          // provide a batch to complete the handover and retire the previous set
+          let mut block_hash = BlockHash([0; 32]);
+          OsRng.fill_bytes(&mut block_hash.0);
+          let batch = Batch { network, id: 0, block: block_hash, instructions: vec![] };
+          publish_tx(
+            &serai,
+            &SeraiInInstructions::execute_batch(SignedBatch {
+              batch: batch.clone(),
+              signature: substrate_pair.sign(&batch_message(&batch)),
+            }),
+          )
+          .await;
+        }
+
+        // verify
+        participants.sort();
+        verify_session_and_active_validators(&serai, network, activation_session, participants)
+          .await;
 
         // check pending deallocations
         let pending = serai
@@ -212,8 +305,8 @@ async fn validator_set_rotation() {
           .validator_sets()
           .pending_deallocations(
             network,
-            pair2.public(),
-            Session(u32::try_from(active_session + 1).unwrap()),
+            participant_to_remove.public(),
+            Session(activation_session + 1),
           )
           .await
           .unwrap();
@@ -223,24 +316,39 @@ async fn validator_set_rotation() {
     .await;
 }
 
+async fn session_for_block(serai: &Serai, block: [u8; 32], network: NetworkId) -> u32 {
+  serai.as_of(block).validator_sets().session(network).await.unwrap().unwrap().0
+}
+
 async fn verify_session_and_active_validators(
   serai: &Serai,
   network: NetworkId,
-  session: u64,
+  session: u32,
   participants: &[Public],
 ) {
-  // wait untill the epoch block finalized
-  let epoch_block = (session * EPOCH_INTERVAL) + 1;
-  while serai.finalized_block_by_number(epoch_block).await.unwrap().is_none() {
-    // sleep 1 block
-    tokio::time::sleep(tokio::time::Duration::from_secs(6)).await;
-  }
-  let serai_for_block =
-    serai.as_of(serai.finalized_block_by_number(epoch_block).await.unwrap().unwrap().hash());
+  // wait until the active session. This wait should be max 30 secs since the epoch time.
+  let block = tokio::time::timeout(core::time::Duration::from_secs(2 * 60), async move {
+    loop {
+      let mut block = serai.latest_finalized_block_hash().await.unwrap();
+      if session_for_block(serai, block, network).await < session {
+        // Sleep a block
+        tokio::time::sleep(core::time::Duration::from_secs(6)).await;
+        continue;
+      }
+      while session_for_block(serai, block, network).await > session {
+        block = serai.block(block).await.unwrap().unwrap().header.parent_hash.0;
+      }
+      assert_eq!(session_for_block(serai, block, network).await, session);
+      break block;
+    }
+  })
+  .await
+  .unwrap();
+  let serai_for_block = serai.as_of(block);
 
   // verify session
   let s = serai_for_block.validator_sets().session(network).await.unwrap().unwrap();
-  assert_eq!(u64::from(s.0), session);
+  assert_eq!(s.0, session);
 
   // verify participants
   let mut validators =
@@ -249,10 +357,11 @@ async fn verify_session_and_active_validators(
   assert_eq!(validators, participants);
 
   // make sure finalization continues as usual after the changes
-  tokio::time::timeout(tokio::time::Duration::from_secs(60), async move {
+  let current_finalized_block = serai.latest_finalized_block().await.unwrap().header.number;
+  tokio::time::timeout(core::time::Duration::from_secs(60), async move {
     let mut finalized_block = serai.latest_finalized_block().await.unwrap().header.number;
-    while finalized_block <= epoch_block + 2 {
-      tokio::time::sleep(tokio::time::Duration::from_secs(6)).await;
+    while finalized_block <= current_finalized_block + 2 {
+      tokio::time::sleep(core::time::Duration::from_secs(6)).await;
       finalized_block = serai.latest_finalized_block().await.unwrap().header.number;
     }
   })
@@ -262,15 +371,18 @@ async fn verify_session_and_active_validators(
   // TODO: verify key shares as well?
 }
 
-async fn get_active_session(serai: &Serai, network: NetworkId, hash: [u8; 32]) -> u64 {
-  let block_number = serai.block(hash).await.unwrap().unwrap().header.number;
-  let epoch = block_number / EPOCH_INTERVAL;
+async fn get_session_at_which_changes_activate(
+  serai: &Serai,
+  network: NetworkId,
+  hash: [u8; 32],
+) -> u32 {
+  let session = session_for_block(serai, hash, network).await;
 
   // changes should be active in the next session
   if network == NetworkId::Serai {
     // it takes 1 extra session for serai net to make the changes active.
-    epoch + 2
+    session + 2
   } else {
-    epoch + 1
+    session + 1
   }
 }

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() {