From a03a1edbff9ad8439f3a3e4bcb9095511fd3cd3f Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Wed, 15 Nov 2023 22:49:58 -0500 Subject: [PATCH] Remove needless transposition in coordinator --- coordinator/src/cosign_evaluator.rs | 1 - coordinator/src/main.rs | 17 ++---- coordinator/src/substrate/mod.rs | 18 ++---- coordinator/src/tests/tributary/dkg.rs | 6 +- coordinator/src/tributary/handle.rs | 76 +++++++++++++------------- coordinator/src/tributary/mod.rs | 2 +- tests/coordinator/src/tests/cosign.rs | 7 ++- 7 files changed, 56 insertions(+), 71 deletions(-) diff --git a/coordinator/src/cosign_evaluator.rs b/coordinator/src/cosign_evaluator.rs index 08719655..050a7f8a 100644 --- a/coordinator/src/cosign_evaluator.rs +++ b/coordinator/src/cosign_evaluator.rs @@ -257,7 +257,6 @@ impl CosignEvaluator { } } else { let mut latest_cosigns = self.latest_cosigns.write().await; - latest_cosigns.insert(cosign.network, (block.number(), cosign)); self.update_latest_cosign().await; } diff --git a/coordinator/src/main.rs b/coordinator/src/main.rs index ba8e9632..1e055f5a 100644 --- a/coordinator/src/main.rs +++ b/coordinator/src/main.rs @@ -431,24 +431,19 @@ async fn handle_processor_message( .i(pub_key) .expect("processor message to DKG for a session we aren't a validator in"); - // TODO: This is [receiver_share][sender_share] and is later transposed to - // [sender_share][receiver_share]. Make this [sender_share][receiver_share] from the - // start? // `tx_shares` needs to be done here as while it can be serialized from the HashMap // without further context, it can't be deserialized without context let mut tx_shares = Vec::with_capacity(shares.len()); - for i in 1 ..= spec.n() { - let i = Participant::new(i).unwrap(); - if our_i.contains(&i) { - for shares in &shares { + for shares in &mut shares { + tx_shares.push(vec![]); + for i in 1 ..= spec.n() { + let i = Participant::new(i).unwrap(); + if our_i.contains(&i) { if shares.contains_key(&i) { panic!("processor sent us our own shares"); } + continue; } - continue; - } - tx_shares.push(vec![]); - for shares in &mut shares { tx_shares.last_mut().unwrap().push( shares.remove(&i).expect("processor didn't send share for another validator"), ); diff --git a/coordinator/src/substrate/mod.rs b/coordinator/src/substrate/mod.rs index 8144d610..8d00ee43 100644 --- a/coordinator/src/substrate/mod.rs +++ b/coordinator/src/substrate/mod.rs @@ -456,15 +456,11 @@ async fn handle_new_blocks( let maximally_latent_cosign_block = skipped_block.map(|skipped_block| skipped_block + COSIGN_DISTANCE); for block in (last_intended_to_cosign_block + 1) ..= latest_number { - SeraiBlockNumber::set( - &mut txn, - serai - .block_by_number(block) - .await? - .expect("couldn't get block which should've been finalized") - .hash(), - &block, - ); + let actual_block = serai + .block_by_number(block) + .await? + .expect("couldn't get block which should've been finalized"); + SeraiBlockNumber::set(&mut txn, actual_block.hash(), &block); let mut set = false; @@ -494,10 +490,6 @@ async fn handle_new_blocks( // Get the keys as of the prior block // That means if this block is setting new keys (which won't lock in until we process this // block), we won't freeze up waiting for the yet-to-be-processed keys to sign this block - let actual_block = serai - .block_by_number(block) - .await? - .expect("couldn't get block which should've been finalized"); let serai = serai.as_of(actual_block.header().parent_hash.into()); has_no_cosigners = Some(actual_block.clone()); diff --git a/coordinator/src/tests/tributary/dkg.rs b/coordinator/src/tests/tributary/dkg.rs index 99611bbe..b96468d0 100644 --- a/coordinator/src/tests/tributary/dkg.rs +++ b/coordinator/src/tests/tributary/dkg.rs @@ -161,12 +161,12 @@ async fn dkg_test() { for (k, key) in keys.iter().enumerate() { let attempt = 0; - let mut shares = vec![]; + let mut shares = vec![vec![]]; for i in 0 .. keys.len() { if i != k { let mut share = vec![0; 256]; OsRng.fill_bytes(&mut share); - shares.push(vec![share]); + shares.last_mut().unwrap().push(share); } } @@ -225,7 +225,7 @@ async fn dkg_test() { let relative_i = i - (if i > l { 1 } else { 0 }); Some(( Participant::new((l + 1).try_into().unwrap()).unwrap(), - shares[relative_i][0].clone(), + shares[0][relative_i].clone(), )) } } else { diff --git a/coordinator/src/tributary/handle.rs b/coordinator/src/tributary/handle.rs index f0e3a22a..f94f5e84 100644 --- a/coordinator/src/tributary/handle.rs +++ b/coordinator/src/tributary/handle.rs @@ -279,38 +279,39 @@ pub(crate) async fn handle_application_tx< .expect("transaction added to tributary by signer who isn't a participant"); let sender_is_len = u16::from(sender_i.end) - u16::from(sender_i.start); - if shares.len() != (usize::from(spec.n() - sender_is_len)) { - fatal_slash::(txn, genesis, signed.signer.to_bytes(), "invalid amount of DKG shares"); + if shares.len() != usize::from(sender_is_len) { + fatal_slash::( + txn, + genesis, + signed.signer.to_bytes(), + "invalid amount of DKG shares by key shares", + ); return; } for shares in &shares { - if shares.len() != usize::from(sender_is_len) { - fatal_slash::( - txn, - genesis, - signed.signer.to_bytes(), - "invalid amount of DKG shares by key shares", - ); + if shares.len() != (usize::from(spec.n() - sender_is_len)) { + fatal_slash::(txn, genesis, signed.signer.to_bytes(), "invalid amount of DKG shares"); return; } } // Save each share as needed for blame { - let from = spec.i(signed.signer).unwrap(); - for (to, shares) in shares.iter().enumerate() { - // 0-indexed (the enumeration) to 1-indexed (Participant) - let mut to = u16::try_from(to).unwrap() + 1; - // Adjust for the omission of the sender's own shares - if to >= u16::from(from.start) { - to += u16::from(from.end) - u16::from(from.start); - } - let to = Participant::new(to).unwrap(); + let from_range = spec.i(signed.signer).unwrap(); + for (from_offset, shares) in shares.iter().enumerate() { + let from = + Participant::new(u16::from(from_range.start) + u16::try_from(from_offset).unwrap()) + .unwrap(); + + for (to_offset, share) in shares.iter().enumerate() { + // 0-indexed (the enumeration) to 1-indexed (Participant) + let mut to = u16::try_from(to_offset).unwrap() + 1; + // Adjust for the omission of the sender's own shares + if to >= u16::from(from_range.start) { + to += u16::from(from_range.end) - u16::from(from_range.start); + } + let to = Participant::new(to).unwrap(); - for (sender_share, share) in shares.iter().enumerate() { - let from = - Participant::new(u16::from(from.start) + u16::try_from(sender_share).unwrap()) - .unwrap(); TributaryDb::::save_share_for_blame(txn, &genesis, from, to, share); } } @@ -331,21 +332,17 @@ pub(crate) async fn handle_application_tx< our_i_pos -= sender_is_len; } let our_i_pos = usize::from(our_i_pos); - let shares = shares - .drain( - our_i_pos .. (our_i_pos + usize::from(u16::from(our_i.end) - u16::from(our_i.start))), - ) - .collect::>(); - - // Transpose from our shares -> sender shares -> shares to - // sender shares -> our shares -> shares - let mut transposed = vec![vec![]; shares[0].len()]; - for shares in shares { - for (sender_index, share) in shares.into_iter().enumerate() { - transposed[sender_index].push(share); - } - } - transposed + shares + .iter_mut() + .map(|shares| { + shares + .drain( + our_i_pos .. + (our_i_pos + usize::from(u16::from(our_i.end) - u16::from(our_i.start))), + ) + .collect::>() + }) + .collect() }; // Drop shares as it's been mutated into invalidity drop(shares); @@ -519,6 +516,8 @@ pub(crate) async fn handle_application_tx< }; break key_pair.0.into(); }; + let block_number = SeraiBlockNumber::get(txn, hash) + .expect("CosignSubstrateBlock yet didn't save Serai block number"); processors .send( spec.set().network, @@ -528,8 +527,7 @@ pub(crate) async fn handle_application_tx< id: SubstrateSignableId::CosigningSubstrateBlock(hash), attempt: 0, }, - block_number: SeraiBlockNumber::get(txn, hash) - .expect("CosignSubstrateBlock yet didn't save Serai block number"), + block_number, }, ) .await; diff --git a/coordinator/src/tributary/mod.rs b/coordinator/src/tributary/mod.rs index 4af7964c..20a2405a 100644 --- a/coordinator/src/tributary/mod.rs +++ b/coordinator/src/tributary/mod.rs @@ -243,7 +243,7 @@ pub enum Transaction { DkgCommitments(u32, Vec>, Signed), DkgShares { attempt: u32, - // Receiving Participant, Sending Participant, Share + // Sending Participant, Receiving Participant, Share shares: Vec>>, confirmation_nonces: [u8; 64], signed: Signed, diff --git a/tests/coordinator/src/tests/cosign.rs b/tests/coordinator/src/tests/cosign.rs index 4de3ce20..1a36ecda 100644 --- a/tests/coordinator/src/tests/cosign.rs +++ b/tests/coordinator/src/tests/cosign.rs @@ -22,7 +22,7 @@ pub async fn potentially_cosign( ) -> CoordinatorMessage { let msg = processors[primary_processor].recv_message().await; let messages::CoordinatorMessage::Coordinator( - messages::coordinator::CoordinatorMessage::CosignSubstrateBlock { id }, + messages::coordinator::CoordinatorMessage::CosignSubstrateBlock { block_number, id }, ) = msg.clone() else { return msg; @@ -85,7 +85,7 @@ pub async fn potentially_cosign( participants.insert(known_signer_i); participants } - _ => panic!("coordinator didn't send back SubstratePreprocesses"), + other => panic!("coordinator didn't send back SubstratePreprocesses: {:?}", other), }; for i in participants.clone() { @@ -152,7 +152,7 @@ pub async fn potentially_cosign( let signature = Signature( schnorrkel::keys::Keypair::from_bytes(&schnorrkel_key_pair) .unwrap() - .sign_simple(b"substrate", &cosign_block_msg(block)) + .sign_simple(b"substrate", &cosign_block_msg(block_number, block)) .to_bytes(), ); @@ -162,6 +162,7 @@ pub async fn potentially_cosign( } processor .send_message(messages::coordinator::ProcessorMessage::CosignedBlock { + block_number, block, signature: signature.0.to_vec(), })