diff --git a/coordinator/src/main.rs b/coordinator/src/main.rs index ca150656..e1dae98e 100644 --- a/coordinator/src/main.rs +++ b/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( 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( 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, + shares: tx_shares, confirmation_nonces: nonces, signed: Transaction::empty_signed(), }) diff --git a/coordinator/src/tests/tributary/dkg.rs b/coordinator/src/tests/tributary/dkg.rs index 7da9fdf2..8ade2e09 100644 --- a/coordinator/src/tests/tributary/dkg.rs +++ b/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 - .get(&Participant::new((i + 1).try_into().unwrap()).unwrap()) - .cloned() - .map(|share| (Participant::new((l + 1).try_into().unwrap()).unwrap(), share)) + if i == l { + None + } else { + 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"); } diff --git a/coordinator/src/tests/tributary/mod.rs b/coordinator/src/tests/tributary/mod.rs index 449fc3bc..fd198b8e 100644 --- a/coordinator/src/tests/tributary/mod.rs +++ b/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(rng: &mut R) -> u16 { - u16::try_from(rng.next_u64() >> 48).unwrap() -} - fn random_u32(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 - let mut shares = HashMap::new(); + // Create a valid vec of shares + 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]; diff --git a/coordinator/src/tributary/handle.rs b/coordinator/src/tributary/handle.rs index 1fd60b05..98aff266 100644 --- a/coordinator/src/tributary/handle.rs +++ b/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 } => { - if sender_i != - spec - .i(signed.signer) - .expect("transaction added to tributary by signer who isn't a participant") - { - // TODO: Full slash - todo!(); - } - + Transaction::DkgShares { attempt, mut shares, confirmation_nonces, signed } => { 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 { vec![] } else { shares.remove(&our_i).unwrap() }; + + let bytes = if sender_i == our_i { + 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, diff --git a/coordinator/src/tributary/mod.rs b/coordinator/src/tributary/mod.rs index ae7a6f7b..c97107db 100644 --- a/coordinator/src/tributary/mod.rs +++ b/coordinator/src/tributary/mod.rs @@ -1,8 +1,5 @@ use core::ops::Deref; -use std::{ - io::{self, Read, Write}, - collections::HashMap, -}; +use std::io::{self, Read, Write}; use zeroize::Zeroizing; use rand_core::{RngCore, CryptoRng}; @@ -224,8 +221,7 @@ pub enum Transaction { DkgCommitments(u32, Vec, Signed), DkgShares { attempt: u32, - sender_i: Participant, - shares: HashMap>, + shares: Vec>, 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 { - attempt, - sender_i: Participant::new(sender_i) - .ok_or_else(|| io::Error::new(io::ErrorKind::Other, "invalid sender participant"))?, - shares, - confirmation_nonces, - signed, - }) + Ok(Transaction::DkgShares { attempt, 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 - // read in order to create a valid DkgShares - // TODO: Transform DkgShares to having a Vec of shares, with post-expansion to the proper - // HashMap - writer.write_all(&u16::from(*sender_i).to_le_bytes())?; - - // Shares are indexed by non-zero u16s (Participants), so this can't fail + // `shares` is a Vec which maps to a HashMap> for any legitimate + // `DkgShares`. Since Participant has a range of 1 ..= u16::MAX, the length must be < + // u16::MAX. The only way for this to not be true if we were malicious, or if we read a + // `DkgShares` with a `shares.len() > u16::MAX`. The former is assumed untrue. The latter + // is impossible since we'll only read up to u16::MAX items. writer.write_all(&u16::try_from(shares.len()).unwrap().to_le_bytes())?; - let mut share_len = None; - let mut found_our_share = false; - for participant in 1 ..= (shares.len() + 1) { - let Some(share) = - &shares.get(&Participant::new(u16::try_from(participant).unwrap()).unwrap()) - else { - assert!(!found_our_share); - 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()); - } + let share_len = shares.get(0).map(|share| share.len()).unwrap_or(0); + // 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())?; + 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) }