Move SignCompleted to Unsigned to cause de-duplication amongst honest validators

coordinator/src/main.rs

@@ -12,7 +12,11 @@ use std::{
 use zeroize::{Zeroize, Zeroizing};
 use rand_core::OsRng;
 
-use ciphersuite::{group::ff::PrimeField, Ciphersuite, Ristretto};
+use ciphersuite::{
+  group::ff::{Field, PrimeField},
+  Ciphersuite, Ristretto,
+};
+use schnorr::SchnorrSignature;
 
 use serai_db::{DbTxn, Db};
 use serai_env as env;
@@ -476,6 +480,10 @@ pub async fn handle_processors<D: Db, Pro: Processors, P: P2p>(
   loop {
     // TODO: Dispatch this message to a task dedicated to handling this processor, preventing one
     // processor from holding up all the others
+    // In order to do so, we need to locally track if a message was handled, even if its ID is
+    // greater than what the message-queue tracks as ack'd
+    // TODO: Shouldn't the processor also perform such checks? That its ack doesn't exceed the
+    // queue's?
     let msg = processors.recv().await;
 
     // TODO2: This is slow, and only works as long as a network only has a single Tributary
@@ -565,7 +573,23 @@ pub async fn handle_processors<D: Db, Pro: Processors, P: P2p>(
           signed: Transaction::empty_signed(),
         })),
         sign::ProcessorMessage::Completed { key: _, id, tx } => {
-          Some(Transaction::SignCompleted(id, tx, Transaction::empty_signed()))
+          let r = Zeroizing::new(<Ristretto as Ciphersuite>::F::random(&mut OsRng));
+          #[allow(non_snake_case)]
+          let R = <Ristretto as Ciphersuite>::generator() * r.deref();
+          let mut tx = Transaction::SignCompleted {
+            plan: id,
+            tx_hash: tx,
+            first_signer: pub_key,
+            signature: SchnorrSignature { R, s: <Ristretto as Ciphersuite>::F::ZERO },
+          };
+          let signed = SchnorrSignature::sign(&key, r, tx.sign_completed_challenge());
+          match &mut tx {
+            Transaction::SignCompleted { signature, .. } => {
+              *signature = signed;
+            }
+            _ => unreachable!(),
+          }
+          Some(tx)
         }
       },
       ProcessorMessage::Coordinator(inner_msg) => match inner_msg {

coordinator/src/substrate/mod.rs

@@ -101,7 +101,6 @@ async fn handle_key_gen<Pro: Processors>(
   key_pair: KeyPair,
 ) -> Result<(), SeraiError> {
   if in_set(key, serai, set).await?.expect("KeyGen occurred for a set which doesn't exist") {
-    // TODO: Check how the processor handles this being fired multiple times
     processors
       .send(
         set.network,
@@ -196,7 +195,6 @@ async fn handle_batch_and_burns<Pro: Processors>(
         .expect("network had a batch/burn yet never set a latest block")
     };
 
-    // TODO: Check how the processor handles this being fired multiple times
     processors
       .send(
         network,

coordinator/src/tests/tributary/mod.rs

@@ -116,4 +116,17 @@ fn serialize_transaction() {
 
   test_read_write(Transaction::SignPreprocess(random_sign_data(&mut OsRng)));
   test_read_write(Transaction::SignShare(random_sign_data(&mut OsRng)));
+
+  {
+    let mut plan = [0; 32];
+    OsRng.fill_bytes(&mut plan);
+    let mut tx_hash = vec![0; (OsRng.next_u64() % 64).try_into().unwrap()];
+    OsRng.fill_bytes(&mut tx_hash);
+    test_read_write(Transaction::SignCompleted {
+      plan,
+      tx_hash,
+      first_signer: random_signed(&mut OsRng).signer,
+      signature: random_signed(&mut OsRng).signature,
+    });
+  }
 }

coordinator/src/tributary/handle.rs

@@ -585,8 +585,8 @@ pub async fn handle_application_tx<
         None => {}
       }
     }
-    Transaction::SignCompleted(id, tx, signed) => {
-      // TODO: Confirm this is a valid ID
+    Transaction::SignCompleted { plan, tx_hash, .. } => {
+      // TODO: Confirm this is a valid plan ID
       // TODO: Confirm this signer hasn't prior published a completion
       let Some(key_pair) = TributaryDb::<D>::key_pair(txn, spec.set()) else { todo!() };
       processors
@@ -594,8 +594,8 @@ pub async fn handle_application_tx<
           spec.set().network,
           CoordinatorMessage::Sign(sign::CoordinatorMessage::Completed {
             key: key_pair.1.to_vec(),
-            id,
-            tx,
+            id: plan,
+            tx: tx_hash,
           }),
         )
         .await;

coordinator/src/tributary/mod.rs

@@ -232,6 +232,10 @@ pub enum Transaction {
   DkgConfirmed(u32, [u8; 32], Signed),
 
   // When we have synchrony on a batch, we can allow signing it
+  // TODO (never?): This is less efficient compared to an ExternalBlock provided transaction,
+  // which would be binding over the block hash and automatically achieve synchrony on all
+  // relevant batches. ExternalBlock was removed for this due to complexity around the pipeline
+  // with the current processor, yet it would still be an improvement.
   Batch([u8; 32], [u8; 32]),
   // When a Serai block is finalized, with the contained batches, we can allow the associated plan
   // IDs
@@ -242,10 +246,18 @@ pub enum Transaction {
 
   SignPreprocess(SignData),
   SignShare(SignData),
-  // TODO: We can't make this an Unsigned as we need to prevent spam, which requires a max of 1
-  // claim per sender
-  // Can we de-duplicate across senders though, if they claim the same hash completes?
-  SignCompleted([u8; 32], Vec<u8>, Signed),
+  // This is defined as an Unsigned transaction in order to de-duplicate SignCompleted amongst
+  // reporters (who should all report the same thing)
+  // We do still track the signer in order to prevent a single signer from publishing arbitrarily
+  // many TXs without penalty
+  // Here, they're denoted as the first_signer, as only the signer of the first TX to be included
+  // with this pairing will be remembered on-chain
+  SignCompleted {
+    plan: [u8; 32],
+    tx_hash: Vec<u8>,
+    first_signer: <Ristretto as Ciphersuite>::G,
+    signature: SchnorrSignature<Ristretto>,
+  },
 }
 
 impl ReadWrite for Transaction {
@@ -354,13 +366,15 @@ impl ReadWrite for Transaction {
         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 mut tx_hash_len = [0];
+        reader.read_exact(&mut tx_hash_len)?;
+        let mut tx_hash = vec![0; usize::from(tx_hash_len[0])];
+        reader.read_exact(&mut tx_hash)?;
 
-        let signed = Signed::read(reader)?;
-        Ok(Transaction::SignCompleted(plan, tx, signed))
+        let first_signer = Ristretto::read_G(reader)?;
+        let signature = SchnorrSignature::<Ristretto>::read(reader)?;
+
+        Ok(Transaction::SignCompleted { plan, tx_hash, first_signer, signature })
       }
 
       _ => Err(io::Error::new(io::ErrorKind::Other, "invalid transaction type")),
@@ -460,12 +474,14 @@ impl ReadWrite for Transaction {
         writer.write_all(&[8])?;
         data.write(writer)
       }
-      Transaction::SignCompleted(plan, tx, signed) => {
+      Transaction::SignCompleted { plan, tx_hash, first_signer, signature } => {
         writer.write_all(&[9])?;
         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)
+        writer
+          .write_all(&[u8::try_from(tx_hash.len()).expect("tx hash length exceed 255 bytes")])?;
+        writer.write_all(tx_hash)?;
+        writer.write_all(&first_signer.to_bytes())?;
+        signature.write(writer)
       }
     }
   }
@@ -486,7 +502,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),
+      Transaction::SignCompleted { .. } => TransactionKind::Unsigned,
     }
   }
 
@@ -506,6 +522,12 @@ impl TransactionTrait for Transaction {
       }
     }
 
+    if let Transaction::SignCompleted { plan, tx_hash, first_signer, signature } = self {
+      if !signature.verify(*first_signer, self.sign_completed_challenge()) {
+        Err(TransactionError::InvalidContent)?;
+      }
+    }
+
     Ok(())
   }
 }
@@ -545,7 +567,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,
+        Transaction::SignCompleted { .. } => panic!("signing SignCompleted"),
       }
     }
 
@@ -558,4 +580,18 @@ impl Transaction {
     let sig_hash = self.sig_hash(genesis);
     signed(self).signature = SchnorrSignature::<Ristretto>::sign(key, sig_nonce, sig_hash);
   }
+
+  pub fn sign_completed_challenge(&self) -> <Ristretto as Ciphersuite>::F {
+    if let Transaction::SignCompleted { plan, tx_hash, first_signer, signature } = self {
+      let mut transcript =
+        RecommendedTranscript::new(b"Coordinator Tributary Transaction SignCompleted");
+      transcript.append_message(b"plan", plan);
+      transcript.append_message(b"tx_hash", tx_hash);
+      transcript.append_message(b"signer", first_signer.to_bytes());
+      transcript.append_message(b"nonce", signature.R.to_bytes());
+      Ristretto::hash_to_F(b"SignCompleted signature", &transcript.challenge(b"challenge"))
+    } else {
+      panic!("sign_completed_challenge called on transaction which wasn't SignCompleted")
+    }
+  }
 }

coordinator/src/tributary/scanner.rs

@@ -76,7 +76,7 @@ async fn handle_block<
         // mark the node as fatally slashed
         TributaryDb::<D>::set_fatally_slashed(&mut txn, genesis, msgs.0.msg.sender);
 
-        // TODO: disconnect the node from network/ban from further participation in Tributary
+        // TODO2: disconnect the node from network/ban from further participation in Tributary
       }
       TributaryTransaction::Application(tx) => {
         handle_application_tx::<D, _, _, _, _, _>(
@@ -99,7 +99,7 @@ async fn handle_block<
     event_id += 1;
   }
 
-  // TODO: Trigger any necessary re-attempts
+  // TODO2: Trigger any necessary re-attempts
 }
 
 pub async fn handle_new_blocks<