Properly define the on-chain handover protocol

The new key publishing `Batch`s is more than sufficient.

Also uses the correct key to verify the published `Batch`s authenticity.

coordinator/src/main.rs

@@ -898,6 +898,8 @@ async fn handle_processor_messages<D: Db, Pro: Processors, P: P2p>(
               if id.attempt == 0 {
                 MainDb::<D>::save_first_preprocess(&mut txn, spec.set().network, id.id, preprocess);
 
+                // TODO: If this is the new key's first Batch, only create this TX once we verify
+                // all prior published `Batch`s
                 Some(Transaction::Batch(block.0, id.id))
               } else {
                 Some(Transaction::BatchPreprocess(SignData {

docs/processor/Multisig Rotation.md

@@ -122,9 +122,9 @@ The following timeline is established:
    Once all the 6 hour period has expired, no `Eventuality`s remain, and all
    outputs are forwarded, the multisig publishes a final `Batch` of the first
    block, plus `CONFIRMATIONS`, which met these conditions, regardless of if it
-   would've otherwise had a `Batch`. Then, it reports to Substrate has closed.
-   No further actions by it, nor its validators, are expected (unless those
-   validators remain present in the new multisig).
+   would've otherwise had a `Batch`. No further actions by it, nor its
+   validators, are expected (unless, of course, those validators remain present
+   in the new multisig).
 
 7) The new multisig confirms all transactions from all prior multisigs were made
    as expected, including the reported `Batch`s.
@@ -151,8 +151,8 @@ The following timeline is established:
    actually be achieved (at least, not without a ZK proof the published `Batch`s
    were correct).
 
-8) The new multisig reports a successful close of the prior multisig, and
-   becomes the sole multisig with full responsibilities.
+8) The new multisig publishes the next `Batch`, signifying the accepting of full
+   responsibilities and a successful close of the prior multisig.
 
 ### Latency and Fees
 

substrate/in-instructions/pallet/Cargo.toml

@@ -50,5 +50,6 @@ std = [
   "in-instructions-primitives/std",
 
   "tokens-pallet/std",
+  "validator-sets-pallet/std",
 ]
 default = ["std"]

substrate/in-instructions/pallet/src/lib.rs

@@ -80,29 +80,21 @@ pub mod pallet {
     }
   }
 
-  fn key_for_network<T: Config>(network: NetworkId) -> Result<Public, InvalidTransaction> {
-    // TODO: Get the latest session
-    let session = Session(0);
-
+  fn key_for_network<T: Config>(network: NetworkId) -> Result<(Session, Option<Public>, Option<Public>), InvalidTransaction> {
+    let session = ValidatorSets::<T>::session(network);
     let mut set = ValidatorSet { session, network };
-    // TODO: If this session just set their keys, it'll invalidate any batches in the mempool
-    // Should there be a transitory period/future-set cut off?
-    if let Some(keys) = ValidatorSets::<T>::keys(set) {
-      Ok(keys.0)
+    let latest = ValidatorSets::<T>::keys(set).map(|keys| keys.0);
+    let prior = if set.session.0 != 0 {
+      set.session.0 -= 1;
+      ValidatorSets::<T>::keys(set).map(|keys| keys.0)
     } else {
-      // If this set hasn't set their keys yet, use the previous set's
-      if set.session.0 == 0 {
-        // Since there haven't been any keys set, no signature can legitimately exist
+      None
+    };
+    // If there's no keys set, then this must be an invalid signature
+    if prior.is_none() && latest.is_none() {
       Err(InvalidTransaction::BadProof)?;
     }
-      set.session.0 -= 1;
-
-      if let Some(keys) = ValidatorSets::<T>::keys(set) {
-        Ok(keys.0)
-      } else {
-        Err(InvalidTransaction::BadProof)?
-      }
-    }
+    Ok((session, prior, latest))
   }
 
   #[pallet::call]
@@ -155,7 +147,7 @@ pub mod pallet {
       };
 
       let network = batch.batch.network;
-      let key = key_for_network::<T>(network)?;
+      let (current_session, prior, current) = key_for_network::<T>(network)?;
 
       // verify the batch size
       // TODO: Merge this encode with the one done by batch_message
@@ -164,10 +156,26 @@ pub mod pallet {
       }
 
       // verify the signature
-      if !key.verify(&batch_message(&batch.batch), &batch.signature) {
+      let batch_message = batch_message(&batch.batch);
+      // Check the prior key first since only a single `Batch` (the last one) will be when prior is
+      // Some yet prior wasn't the signing key
+      let valid_by_prior = if let Some(key) = prior {
+        key.verify(&batch_message, &batch.signature)
+      } else { false };
+      let valid = valid_by_prior || (if let Some(key) = current {
+        key.verify(&batch_message, &batch.signature)
+      } else { false });
+      if !valid {
         Err(InvalidTransaction::BadProof)?;
       }
 
+      // If it wasn't valid by the prior key, meaning it was valid by the current key, the current
+      // key is publishing `Batch`s. This should only happen once the current key has verified all
+      // `Batch`s published by the prior key, meaning they are accepting the hand-over.
+      if prior.is_some() && (!valid_by_prior) {
+        ValidatorSets::<T>::retire_session(network, Session(current_session.0 - 1));
+      }
+
       // check that this validator set isn't publishing a batch more than once per block
       let current_block = <frame_system::Pallet<T>>::block_number();
       let last_block = LastBatchBlock::<T>::get(network).unwrap_or(Zero::zero());

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

@@ -58,7 +58,7 @@ pub mod pallet {
   #[pallet::storage]
   pub type CurrentSession<T: Config> = StorageMap<_, Identity, NetworkId, Session, OptionQuery>;
   impl<T: Config> Pallet<T> {
-    fn session(network: NetworkId) -> Session {
+    pub fn session(network: NetworkId) -> Session {
       if network == NetworkId::Serai {
         Session(pallet_session::Pallet::<T>::current_index())
       } else {
@@ -206,12 +206,6 @@ pub mod pallet {
       let set = ValidatorSet { network, session };
       Pallet::<T>::deposit_event(Event::NewSet { set });
       if network != NetworkId::Serai {
-        // Remove the keys for the set prior to the one now rotating out
-        if session.0 >= 2 {
-          let prior_to_now_rotating = ValidatorSet { network, session: Session(session.0 - 2) };
-          MuSigKeys::<T>::remove(prior_to_now_rotating);
-          Keys::<T>::remove(prior_to_now_rotating);
-        }
         MuSigKeys::<T>::set(set, Some(musig_key(set, &participants)));
       }
       Participants::<T>::set(network, participants.try_into().unwrap());
@@ -398,8 +392,16 @@ pub mod pallet {
         // Handover is automatically complete for Serai as it doesn't have a handover protocol
         // TODO: Update how handover completed is determined. It's not on set keys. It's on new
         // set accepting responsibility
-        let handover_completed = (network == NetworkId::Serai) ||
-          Keys::<T>::contains_key(ValidatorSet { network, session: Self::session(network) });
+        let handover_completed = (network == NetworkId::Serai) || {
+          let current_session = Self::session(network);
+          // This function shouldn't be used on genesis
+          debug_assert!(current_session != Session(0));
+          // Check the prior session had its keys cleared, which happens once its retired
+          !Keys::<T>::contains_key(ValidatorSet {
+            network,
+            session: Session(current_session.0 - 1),
+          })
+        };
         // Only spawn a NewSet if the current set was actually established with a completed
         // handover protocol
         if handover_completed {
@@ -411,6 +413,12 @@ pub mod pallet {
     pub fn validators(network: NetworkId) -> Vec<Public> {
       Self::participants(network).into()
     }
+
+    pub fn retire_session(network: NetworkId, session: Session) {
+      let set = ValidatorSet { network, session };
+      MuSigKeys::<T>::remove(set);
+      Keys::<T>::remove(set);
+    }
   }
 }