Update the flow for completed signing processes

Now, an on-chain transaction exists. This resolves some ambiguities and
provides greater coordination.

coordinator/src/main.rs

@@ -190,7 +190,6 @@ pub async fn scan_tributaries<D: Db, Pro: Processors, P: P2p>(
   }
 }
 
-#[allow(clippy::type_complexity)]
 pub async fn heartbeat_tributaries<D: Db, P: P2p>(
   p2p: P,
   tributaries: Arc<RwLock<Tributaries<D, P>>>,
@@ -217,7 +216,6 @@ pub async fn heartbeat_tributaries<D: Db, P: P2p>(
   }
 }
 
-#[allow(clippy::type_complexity)]
 pub async fn handle_p2p<D: Db, P: P2p>(
   our_key: <Ristretto as Ciphersuite>::G,
   p2p: P,
@@ -364,7 +362,6 @@ pub async fn publish_transaction<D: Db, P: P2p>(
   }
 }
 
-#[allow(clippy::type_complexity)]
 pub async fn handle_processors<D: Db, Pro: Processors, P: P2p>(
   mut db: D,
   key: Zeroizing<<Ristretto as Ciphersuite>::F>,

coordinator/src/tributary/mod.rs

@@ -232,6 +232,7 @@ pub enum Transaction {
 
   SignPreprocess(SignData),
   SignShare(SignData),
+  SignCompleted([u8; 32], Vec<u8>, Signed),
 }
 
 impl ReadWrite for Transaction {
@@ -304,6 +305,19 @@ impl ReadWrite for Transaction {
       6 => SignData::read(reader).map(Transaction::SignPreprocess),
       7 => SignData::read(reader).map(Transaction::SignShare),
 
+      8 => {
+        let mut plan = [0; 32];
+        reader.read_exact(&mut plan)?;
+
+        let mut tx_len = [0];
+        reader.read_exact(&mut tx_len)?;
+        let mut tx = vec![0; usize::from(tx_len[0])];
+        reader.read_exact(&mut tx)?;
+
+        let signed = Signed::read(reader)?;
+        Ok(Transaction::SignCompleted(plan, tx, signed))
+      }
+
       _ => Err(io::Error::new(io::ErrorKind::Other, "invalid transaction type")),
     }
   }
@@ -376,6 +390,13 @@ impl ReadWrite for Transaction {
         writer.write_all(&[7])?;
         data.write(writer)
       }
+      Transaction::SignCompleted(plan, tx, signed) => {
+        writer.write_all(&[8])?;
+        writer.write_all(plan)?;
+        writer.write_all(&[u8::try_from(tx.len()).expect("tx hash length exceed 255 bytes")])?;
+        writer.write_all(tx)?;
+        signed.write(writer)
+      }
     }
   }
 }
@@ -394,6 +415,7 @@ impl TransactionTrait for Transaction {
 
       Transaction::SignPreprocess(data) => TransactionKind::Signed(&data.signed),
       Transaction::SignShare(data) => TransactionKind::Signed(&data.signed),
+      Transaction::SignCompleted(_, _, signed) => TransactionKind::Signed(signed),
     }
   }
 
@@ -451,6 +473,7 @@ impl Transaction {
 
         Transaction::SignPreprocess(ref mut data) => &mut data.signed,
         Transaction::SignShare(ref mut data) => &mut data.signed,
+        Transaction::SignCompleted(_, _, ref mut signed) => signed,
       }
     }
 

coordinator/src/tributary/scanner.rs

@@ -297,6 +297,7 @@ async fn handle_block<D: Db, Pro: Processors>(
               .await;
           }
         }
+        Transaction::SignCompleted(_, _, _) => todo!(),
       }
 
       TributaryDb::<D>::handle_event(&mut txn, hash, event_id);

coordinator/tributary/src/transaction.rs

@@ -1,11 +1,15 @@
 use core::fmt::Debug;
 use std::{io, collections::HashMap};
 
+use zeroize::Zeroize;
 use thiserror::Error;
 
 use blake2::{Digest, Blake2b512};
 
-use ciphersuite::{group::GroupEncoding, Ciphersuite, Ristretto};
+use ciphersuite::{
+  group::{Group, GroupEncoding},
+  Ciphersuite, Ristretto,
+};
 use schnorr::SchnorrSignature;
 
 use crate::{TRANSACTION_SIZE_LIMIT, ReadWrite};
@@ -48,12 +52,23 @@ impl ReadWrite for Signed {
       Err(io::Error::new(io::ErrorKind::Other, "nonce exceeded limit"))?;
     }
 
-    let signature = SchnorrSignature::<Ristretto>::read(reader)?;
+    let mut signature = SchnorrSignature::<Ristretto>::read(reader)?;
+    if signature.R.is_identity().into() {
+      // Anyone malicious could remove this and try to find zero signatures
+      // We should never produce zero signatures though meaning this should never come up
+      // If it does somehow come up, this is a decent courtesy
+      signature.zeroize();
+      Err(io::Error::new(io::ErrorKind::Other, "signature nonce was identity"))?;
+    }
 
     Ok(Signed { signer, nonce, signature })
   }
 
   fn write<W: io::Write>(&self, writer: &mut W) -> io::Result<()> {
+    // This is either an invalid signature or a private key leak
+    if self.signature.R.is_identity().into() {
+      Err(io::Error::new(io::ErrorKind::Other, "signature nonce was identity"))?;
+    }
     writer.write_all(&self.signer.to_bytes())?;
     writer.write_all(&self.nonce.to_le_bytes())?;
     self.signature.write(writer)

docs/coordinator/Coordinator.md

@@ -36,5 +36,10 @@ transaction containing the signed batch to the Serai blockchain.
 
 # Sign Completed
 
-On `sign::ProcessorMessage::Completed`, the coordinator broadcasts the
-contained information to all validators.
+On `sign::ProcessorMessage::Completed`, the coordinator makes a tributary
+transaction containing the transaction hash the signing process was supposedly
+completed with.
+
+Due to rushing adversaries, the actual transaction completing the plan may be
+distinct on-chain. These messages solely exist to coordinate the signing
+process, not to determine chain state.

processor/messages/src/lib.rs

@@ -98,7 +98,6 @@ pub mod sign {
     // Signed share for the specified signing protocol.
     Share { id: SignId, share: Vec<u8> },
     // Completed a signing protocol already.
-    // TODO: Move this to SignId
     Completed { key: Vec<u8>, id: [u8; 32], tx: Vec<u8> },
   }
 }
@@ -246,9 +245,13 @@ impl CoordinatorMessage {
           sign::CoordinatorMessage::Preprocesses { id, .. } => (0, bincode::serialize(id).unwrap()),
           sign::CoordinatorMessage::Shares { id, .. } => (1, bincode::serialize(id).unwrap()),
           sign::CoordinatorMessage::Reattempt { id } => (2, bincode::serialize(id).unwrap()),
-          // TODO: This doesn't embed the attempt. Accordingly, multiple distinct completions will
-          // be flattened. This isn't acceptable.
-          sign::CoordinatorMessage::Completed { id, .. } => (3, id.to_vec()),
+          // The coordinator should report all reported completions to the processor
+          // Accordingly, the intent is a combination of plan ID and actual TX
+          // While transaction alone may suffice, that doesn't cover cross-chain TX ID conflicts,
+          // which are possible
+          sign::CoordinatorMessage::Completed { id, tx, .. } => {
+            (3, bincode::serialize(&(id, tx)).unwrap())
+          }
         };
 
         let mut res = vec![COORDINATOR_UID, TYPE_SIGN_UID, sub];