Remove sender_i from DkgShares

It was a piece of duplicated data used to achieve context-less
de)serialization. This new Vec code is a bit tricker to first read, yet overall
clean and removes a potential fault.

Saves 2 bytes from DkgShares messages.

coordinator/src/main.rs

@@ -17,6 +17,7 @@ use ciphersuite::{
   Ciphersuite, Ristretto,
 };
 use schnorr::SchnorrSignature;
+use frost::Participant;
 
 use serai_db::{DbTxn, Db};
 use serai_env as env;
@@ -508,7 +509,7 @@ pub async fn handle_processors<D: Db, Pro: Processors, P: P2p>(
         key_gen::ProcessorMessage::Commitments { id, commitments } => {
           Some(Transaction::DkgCommitments(id.attempt, commitments, Transaction::empty_signed()))
         }
-        key_gen::ProcessorMessage::Shares { id, shares } => {
+        key_gen::ProcessorMessage::Shares { id, mut shares } => {
           // Create a MuSig-based machine to inform Substrate of this key generation
           // DkgConfirmer has a TODO noting it's only secure for a single usage, yet this ensures
           // the TODO is resolved before unsafe usage
@@ -516,10 +517,20 @@ pub async fn handle_processors<D: Db, Pro: Processors, P: P2p>(
             panic!("attempt wasn't 0");
           }
           let nonces = crate::tributary::dkg_confirmation_nonces(&key, &spec);
+
+          let mut tx_shares = Vec::with_capacity(shares.len());
+          for i in 1 ..= spec.n() {
+            let i = Participant::new(i).unwrap();
+            if i == my_i {
+              continue;
+            }
+            tx_shares
+              .push(shares.remove(&i).expect("processor didn't send share for another validator"));
+          }
+
           Some(Transaction::DkgShares {
             attempt: id.attempt,
-            sender_i: my_i,
+            shares: tx_shares,
-            shares,
             confirmation_nonces: nonces,
             signed: Transaction::empty_signed(),
           })

coordinator/src/tests/tributary/dkg.rs

@@ -160,18 +160,17 @@ async fn dkg_test() {
   for (k, key) in keys.iter().enumerate() {
     let attempt = 0;
 
-    let mut shares = HashMap::new();
+    let mut shares = vec![];
     for i in 0 .. keys.len() {
       if i != k {
         let mut share = vec![0; 256];
         OsRng.fill_bytes(&mut share);
-        shares.insert(Participant::new((i + 1).try_into().unwrap()).unwrap(), share);
+        shares.push(share);
       }
     }
 
     let mut tx = Transaction::DkgShares {
       attempt,
-      sender_i: Participant::new((k + 1).try_into().unwrap()).unwrap(),
       shares,
       confirmation_nonces: crate::tributary::dkg_confirmation_nonces(key, &spec),
       signed: Transaction::empty_signed(),
@@ -219,10 +218,15 @@ async fn dkg_test() {
         .enumerate()
         .filter_map(|(l, tx)| {
           if let Transaction::DkgShares { shares, .. } = tx {
-            shares
+            if i == l {
-              .get(&Participant::new((i + 1).try_into().unwrap()).unwrap())
+              None
-              .cloned()
+            } else {
-              .map(|share| (Participant::new((l + 1).try_into().unwrap()).unwrap(), share))
+              let relative_i = i - (if i > l { 1 } else { 0 });
+              Some((
+                Participant::new((l + 1).try_into().unwrap()).unwrap(),
+                shares[relative_i].clone(),
+              ))
+            }
           } else {
             panic!("txs had non-shares");
           }

coordinator/src/tests/tributary/mod.rs

@@ -1,10 +1,7 @@
 use core::fmt::Debug;
-use std::collections::HashMap;
 
 use rand_core::{RngCore, OsRng};
 
-use frost::Participant;
-
 use tributary::{ReadWrite, tests::random_signed};
 
 use crate::tributary::{SignData, Transaction};
@@ -20,10 +17,6 @@ mod dkg;
 mod handle_p2p;
 mod sync;
 
-fn random_u16<R: RngCore>(rng: &mut R) -> u16 {
-  u16::try_from(rng.next_u64() >> 48).unwrap()
-}
-
 fn random_u32<R: RngCore>(rng: &mut R) -> u32 {
   u32::try_from(rng.next_u64() >> 32).unwrap()
 }
@@ -70,18 +63,17 @@ fn serialize_transaction() {
     // This supports a variable share length, yet share length is expected to be constant among
     // shares
     let share_len = usize::try_from(OsRng.next_u64() % 512).unwrap();
-    // Create a valid map of shares
+    // Create a valid vec of shares
-    let mut shares = HashMap::new();
+    let mut shares = vec![];
     // Create up to 512 participants
     for i in 0 .. (OsRng.next_u64() % 512) {
       let mut share = vec![0; share_len];
       OsRng.fill_bytes(&mut share);
-      shares.insert(Participant::new(u16::try_from(i + 1).unwrap()).unwrap(), share);
+      shares.push(share);
     }
 
     test_read_write(Transaction::DkgShares {
       attempt: random_u32(&mut OsRng),
-      sender_i: Participant::new(random_u16(&mut OsRng).saturating_add(1)).unwrap(),
       shares,
       confirmation_nonces: {
         let mut nonces = [0; 64];

coordinator/src/tributary/handle.rs

@@ -340,28 +340,31 @@ pub async fn handle_application_tx<
       }
     }
 
-    Transaction::DkgShares { attempt, sender_i, mut shares, confirmation_nonces, signed } => {
+    Transaction::DkgShares { attempt, mut shares, confirmation_nonces, signed } => {
-      if sender_i !=
-        spec
-          .i(signed.signer)
-          .expect("transaction added to tributary by signer who isn't a participant")
-      {
-        // TODO: Full slash
-        todo!();
-      }
-
       if shares.len() != (usize::from(spec.n()) - 1) {
         // TODO: Full slash
         todo!();
       }
 
+      let sender_i = spec
+        .i(signed.signer)
+        .expect("transaction added to tributary by signer who isn't a participant");
+
       // Only save our share's bytes
       let our_i = spec
         .i(Ristretto::generator() * key.deref())
         .expect("in a tributary we're not a validator for");
-      // This unwrap is safe since the length of shares is checked, the the only missing key
+
-      // within the valid range will be the sender's i
+      let bytes = if sender_i == our_i {
-      let bytes = if sender_i == our_i { vec![] } else { shares.remove(&our_i).unwrap() };
+        vec![]
+      } else {
+        // 1-indexed to 0-indexed, handling the omission of the sender's own data
+        let relative_i = usize::from(u16::from(our_i) - 1) -
+          (if u16::from(our_i) > u16::from(sender_i) { 1 } else { 0 });
+        // Safe since we length-checked shares
+        shares.swap_remove(relative_i)
+      };
+      drop(shares);
 
       let confirmation_nonces = handle(
         txn,

coordinator/src/tributary/mod.rs

@@ -1,8 +1,5 @@
 use core::ops::Deref;
-use std::{
+use std::io::{self, Read, Write};
-  io::{self, Read, Write},
-  collections::HashMap,
-};
 
 use zeroize::Zeroizing;
 use rand_core::{RngCore, CryptoRng};
@@ -224,8 +221,7 @@ pub enum Transaction {
   DkgCommitments(u32, Vec<u8>, Signed),
   DkgShares {
     attempt: u32,
-    sender_i: Participant,
+    shares: Vec<Vec<u8>>,
-    shares: HashMap<Participant, Vec<u8>>,
     confirmation_nonces: [u8; 64],
     signed: Signed,
   },
@@ -289,10 +285,6 @@ impl ReadWrite for Transaction {
         reader.read_exact(&mut attempt)?;
         let attempt = u32::from_le_bytes(attempt);
 
-        let mut sender_i = [0; 2];
-        reader.read_exact(&mut sender_i)?;
-        let sender_i = u16::from_le_bytes(sender_i);
-
         let shares = {
           let mut share_quantity = [0; 2];
           reader.read_exact(&mut share_quantity)?;
@@ -301,15 +293,11 @@ impl ReadWrite for Transaction {
           reader.read_exact(&mut share_len)?;
           let share_len = usize::from(u16::from_le_bytes(share_len));
 
-          let mut shares = HashMap::new();
+          let mut shares = vec![];
           for i in 0 .. u16::from_le_bytes(share_quantity) {
-            let mut participant = Participant::new(i + 1).unwrap();
-            if u16::from(participant) >= sender_i {
-              participant = Participant::new(u16::from(participant) + 1).unwrap();
-            }
             let mut share = vec![0; share_len];
             reader.read_exact(&mut share)?;
-            shares.insert(participant, share);
+            shares.push(share);
           }
           shares
         };
@@ -319,14 +307,7 @@ impl ReadWrite for Transaction {
 
         let signed = Signed::read(reader)?;
 
-        Ok(Transaction::DkgShares {
+        Ok(Transaction::DkgShares { attempt, shares, confirmation_nonces, signed })
-          attempt,
-          sender_i: Participant::new(sender_i)
-            .ok_or_else(|| io::Error::new(io::ErrorKind::Other, "invalid sender participant"))?,
-          shares,
-          confirmation_nonces,
-          signed,
-        })
       }
 
       2 => {
@@ -395,46 +376,30 @@ impl ReadWrite for Transaction {
         signed.write(writer)
       }
 
-      Transaction::DkgShares { attempt, sender_i, shares, confirmation_nonces, signed } => {
+      Transaction::DkgShares { attempt, shares, confirmation_nonces, signed } => {
         writer.write_all(&[1])?;
         writer.write_all(&attempt.to_le_bytes())?;
 
-        // It's unfortunate to have this so duplicated, yet it avoids needing to pass a Spec to
+        // `shares` is a Vec which maps to a HashMap<Pariticpant, Vec<u8>> for any legitimate
-        // read in order to create a valid DkgShares
+        // `DkgShares`. Since Participant has a range of 1 ..= u16::MAX, the length must be <
-        // TODO: Transform DkgShares to having a Vec of shares, with post-expansion to the proper
+        // u16::MAX. The only way for this to not be true if we were malicious, or if we read a
-        // HashMap
+        // `DkgShares` with a `shares.len() > u16::MAX`. The former is assumed untrue. The latter
-        writer.write_all(&u16::from(*sender_i).to_le_bytes())?;
+        // is impossible since we'll only read up to u16::MAX items.
-
-        // Shares are indexed by non-zero u16s (Participants), so this can't fail
         writer.write_all(&u16::try_from(shares.len()).unwrap().to_le_bytes())?;
 
-        let mut share_len = None;
+        let share_len = shares.get(0).map(|share| share.len()).unwrap_or(0);
-        let mut found_our_share = false;
+        // For BLS12-381 G2, this would be:
-        for participant in 1 ..= (shares.len() + 1) {
+        // - A 32-byte share
-          let Some(share) =
+        // - A 96-byte ephemeral key
-            &shares.get(&Participant::new(u16::try_from(participant).unwrap()).unwrap())
+        // - A 128-byte signature
-          else {
+        // Hence why this has to be u16
-            assert!(!found_our_share);
+        writer.write_all(&u16::try_from(share_len).unwrap().to_le_bytes())?;
-            found_our_share = true;
-            continue;
-          };
-
-          if let Some(share_len) = share_len {
-            if share.len() != share_len {
-              panic!("variable length shares");
-            }
-          } else {
-            // For BLS12-381 G2, this would be:
-            // - A 32-byte share
-            // - A 96-byte ephemeral key
-            // - A 128-byte signature
-            // Hence why this has to be u16
-            writer.write_all(&u16::try_from(share.len()).unwrap().to_le_bytes())?;
-            share_len = Some(share.len());
-          }
 
+        for share in shares {
+          assert_eq!(share.len(), share_len, "shares were of variable length");
           writer.write_all(share)?;
         }
+
         writer.write_all(confirmation_nonces)?;
         signed.write(writer)
       }