From 89a081176521ec93fa36718c618b79d26a054943 Mon Sep 17 00:00:00 2001 From: Joey Yandle Date: Fri, 22 Nov 2024 16:12:48 -0500 Subject: [PATCH] embed public and private shares into DkgPrivateBegin and DkgEndBegin messages make embedding of public and private shares configurable reenable start_private_shares test now that embedding is configurable inc major semver for backwards incompatible API changes test modules do not need to be public unless someone is using them externally allow a static mut ref to an atomic variable used for test log initialization; allow many arguments to Signer::new allow many arguments to setup_with_timeouts fmt and code change for rng PR rebase rebase fixes add TryFromInt to signer and coordinator errors so we can remove more unwrap calls remove unwrap in signer state machine remove unnecessary AesGcm error since we now have EncryptionError; remove bracket access from frost coordinator add MissingPrivateShares error; remove bracket access in frost Coordinator --- src/common.rs | 2 +- src/net.rs | 13 +++ src/state_machine/coordinator/fire.rs | 142 +++++++++++++++++++++---- src/state_machine/coordinator/frost.rs | 36 ++++++- src/state_machine/coordinator/mod.rs | 28 ++++- src/state_machine/signer/mod.rs | 87 +++++++++++++-- 6 files changed, 273 insertions(+), 35 deletions(-) diff --git a/src/common.rs b/src/common.rs index 3f962340..87ccf9d2 100644 --- a/src/common.rs +++ b/src/common.rs @@ -326,7 +326,7 @@ pub mod test_helpers { #[cfg(test)] /// Test module for common functionality -pub mod test { +mod test { use super::*; use crate::util::create_rng; diff --git a/src/net.rs b/src/net.rs index 2d305ff8..464a39b0 100644 --- a/src/net.rs +++ b/src/net.rs @@ -168,6 +168,9 @@ pub struct DkgPrivateBegin { pub signer_ids: Vec, /// Key IDs who responded in time for this DKG round pub key_ids: Vec, + /// Include DkgPublicShares to avoid p2p related message delivery + /// order issues when signers communicate directly with each other + pub dkg_public_shares: HashMap, } impl Signable for DkgPrivateBegin { @@ -179,6 +182,9 @@ impl Signable for DkgPrivateBegin { } for signer_id in &self.signer_ids { hasher.update(signer_id.to_be_bytes()); + if let Some(shares) = self.dkg_public_shares.get(signer_id) { + shares.hash(hasher); + } } } } @@ -228,6 +234,9 @@ pub struct DkgEndBegin { pub signer_ids: Vec, /// Key IDs who responded in time for this DKG round pub key_ids: Vec, + /// Include DkgPrivateShares to avoid p2p related message delivery + /// order issues when signers communicate directly with each other + pub dkg_private_shares: HashMap, } impl Signable for DkgEndBegin { @@ -239,6 +248,9 @@ impl Signable for DkgEndBegin { } for signer_id in &self.signer_ids { hasher.update(signer_id.to_be_bytes()); + if let Some(shares) = self.dkg_private_shares.get(signer_id) { + shares.hash(hasher); + } } } } @@ -660,6 +672,7 @@ mod test { dkg_id: 0, key_ids: Default::default(), signer_ids: Default::default(), + dkg_public_shares: Default::default(), }; let msg = Message::DkgBegin(dkg_begin.clone()); let coordinator_packet_dkg_begin = Packet { diff --git a/src/state_machine/coordinator/fire.rs b/src/state_machine/coordinator/fire.rs index 87c0d7e1..5e79bf00 100644 --- a/src/state_machine/coordinator/fire.rs +++ b/src/state_machine/coordinator/fire.rs @@ -424,11 +424,19 @@ impl Coordinator { .keys() .flat_map(|signer_id| self.config.signer_key_ids[signer_id].clone()) .collect::>(); - + let dkg_public_shares = if self.config.embed_public_private_shares { + self.dkg_public_shares + .iter() + .map(|(id, share)| (*id, share.clone())) + .collect() + } else { + Default::default() + }; let dkg_begin = DkgPrivateBegin { dkg_id: self.current_dkg_id, key_ids: active_key_ids, signer_ids: self.dkg_public_shares.keys().cloned().collect(), + dkg_public_shares, }; let dkg_private_begin_msg = Packet { sig: dkg_begin @@ -458,11 +466,20 @@ impl Coordinator { .keys() .flat_map(|signer_id| self.config.signer_key_ids[signer_id].clone()) .collect::>(); + let dkg_private_shares = if self.config.embed_public_private_shares { + self.dkg_private_shares + .iter() + .map(|(id, share)| (*id, share.clone())) + .collect() + } else { + Default::default() + }; let dkg_end_begin = DkgEndBegin { dkg_id: self.current_dkg_id, key_ids: active_key_ids, signer_ids: self.dkg_private_shares.keys().cloned().collect(), + dkg_private_shares, }; let dkg_end_begin_msg = Packet { sig: dkg_end_begin @@ -1275,7 +1292,7 @@ impl CoordinatorTrait for Coordinator { #[cfg(test)] /// Test module for coordinator functionality -pub mod test { +mod test { use crate::{ curve::{point::Point, scalar::Scalar}, net::{ @@ -1530,17 +1547,27 @@ pub mod test { #[test] fn missing_public_keys_dkg_v1() { - missing_public_keys_dkg::(10, 1); + missing_public_keys_dkg::(10, 1, false); + } + #[test] + fn missing_public_keys_dkg_v1_embed() { + missing_public_keys_dkg::(10, 1, true); } #[test] fn missing_public_keys_dkg_v2() { - missing_public_keys_dkg::(10, 1); + missing_public_keys_dkg::(10, 1, false); + } + + #[test] + fn missing_public_keys_dkg_v2_embed() { + missing_public_keys_dkg::(10, 1, true); } fn missing_public_keys_dkg( num_signers: u32, keys_per_signer: u32, + embed_public_private_shares: bool, ) -> (Vec>, Vec>) { let timeout = Duration::from_millis(1024); let expire = Duration::from_millis(1280); @@ -1553,6 +1580,7 @@ pub mod test { Some(timeout), Some(timeout), Some(timeout), + embed_public_private_shares, ); // Start a DKG round where we will not allow all signers to recv DkgBegin, so they will not respond with DkgPublicShares @@ -1611,17 +1639,28 @@ pub mod test { #[test] fn minimum_signers_dkg_v1() { - minimum_signers_dkg::(10, 2); + minimum_signers_dkg::(10, 2, false); } #[test] fn minimum_signers_dkg_v2() { - minimum_signers_dkg::(10, 2); + minimum_signers_dkg::(10, 2, false); + } + + #[test] + fn minimum_signers_dkg_v1_embed() { + minimum_signers_dkg::(10, 2, true); + } + + #[test] + fn minimum_signers_dkg_v2_embed() { + minimum_signers_dkg::(10, 2, true); } fn minimum_signers_dkg( num_signers: u32, keys_per_signer: u32, + embed_public_private_shares: bool, ) -> (Vec>, Vec>) { let timeout = Duration::from_millis(1024); let expire = Duration::from_millis(1280); @@ -1634,6 +1673,7 @@ pub mod test { Some(timeout), Some(timeout), Some(timeout), + embed_public_private_shares, ); // Start a DKG round where we will not allow all signers to recv DkgBegin, so they will not respond with DkgPublicShares @@ -1766,15 +1806,27 @@ pub mod test { #[test] fn insufficient_signers_dkg_v1() { - insufficient_signers_dkg::(); + insufficient_signers_dkg::(false); } #[test] fn insufficient_signers_dkg_v2() { - insufficient_signers_dkg::(); + insufficient_signers_dkg::(false); + } + + #[test] + fn insufficient_signers_dkg_v1_embed() { + insufficient_signers_dkg::(true); + } + + #[test] + fn insufficient_signers_dkg_v2_embed() { + insufficient_signers_dkg::(true); } - fn insufficient_signers_dkg() { + fn insufficient_signers_dkg( + embed_public_private_shares: bool, + ) { let timeout = Duration::from_millis(1024); let expire = Duration::from_millis(1280); let num_signers = 10; @@ -1787,6 +1839,7 @@ pub mod test { Some(timeout), Some(timeout), Some(timeout), + embed_public_private_shares, ); // Start a DKG round where we will not allow all signers to recv DkgBegin, so they will not respond with DkgPublicShares @@ -2215,20 +2268,35 @@ pub mod test { #[test] fn minimum_signers_sign_v1() { - minimum_signers_sign::(); + minimum_signers_sign::(false); } #[test] fn minimum_signers_sign_v2() { - minimum_signers_sign::(); + minimum_signers_sign::(false); } - fn minimum_signers_sign() { + #[test] + fn minimum_signers_sign_v1_embed() { + minimum_signers_sign::(true); + } + + #[test] + fn minimum_signers_sign_v2_embed() { + minimum_signers_sign::(true); + } + + fn minimum_signers_sign( + embed_public_private_shares: bool, + ) { let num_signers = 10; let keys_per_signer = 2; - let (mut coordinators, mut signers) = - minimum_signers_dkg::(num_signers, keys_per_signer); + let (mut coordinators, mut signers) = minimum_signers_dkg::( + num_signers, + keys_per_signer, + embed_public_private_shares, + ); let config = coordinators.first().unwrap().get_config(); // Figure out how many signers we can remove and still be above the threshold @@ -2299,20 +2367,35 @@ pub mod test { #[test] fn missing_public_keys_sign_v1() { - missing_public_keys_sign::(); + missing_public_keys_sign::(false); + } + + #[test] + fn missing_public_keys_sign_v2() { + missing_public_keys_sign::(false); } #[test] - fn minimum_missing_public_keys_sign_v2() { - missing_public_keys_sign::(); + fn missing_public_keys_sign_v1_embed() { + missing_public_keys_sign::(true); } - fn missing_public_keys_sign() { + #[test] + fn missing_public_keys_sign_v2_embed() { + missing_public_keys_sign::(true); + } + + fn missing_public_keys_sign( + embed_public_private_shares: bool, + ) { let num_signers = 10; let keys_per_signer = 2; - let (mut coordinators, mut signers) = - minimum_signers_dkg::(num_signers, keys_per_signer); + let (mut coordinators, mut signers) = minimum_signers_dkg::( + num_signers, + keys_per_signer, + embed_public_private_shares, + ); // Let us also remove that signers public key from the config including all of its key ids let mut removed_signer = signers.pop().expect("Failed to pop signer"); @@ -2386,15 +2469,27 @@ pub mod test { #[test] fn insufficient_signers_sign_v1() { - insufficient_signers_sign::(); + insufficient_signers_sign::(false); } #[test] fn insufficient_signers_sign_v2() { - insufficient_signers_sign::(); + insufficient_signers_sign::(false); + } + + #[test] + fn insufficient_signers_sign_v1_embed() { + insufficient_signers_sign::(true); + } + + #[test] + fn insufficient_signers_sign_v2_embed() { + insufficient_signers_sign::(true); } - fn insufficient_signers_sign() { + fn insufficient_signers_sign( + embed_public_private_shares: bool, + ) { let num_signers = 5; let keys_per_signer = 2; let (mut coordinators, mut signers) = @@ -2406,6 +2501,7 @@ pub mod test { None, Some(Duration::from_millis(128)), Some(Duration::from_millis(128)), + embed_public_private_shares, ); let config = coordinators.first().unwrap().get_config(); diff --git a/src/state_machine/coordinator/frost.rs b/src/state_machine/coordinator/frost.rs index c4b6145c..a850b5f0 100644 --- a/src/state_machine/coordinator/frost.rs +++ b/src/state_machine/coordinator/frost.rs @@ -229,10 +229,27 @@ impl Coordinator { dkg_id = %self.current_dkg_id, "Starting Private Share Distribution" ); + let dkg_public_shares = if self.config.embed_public_private_shares { + let mut public_shares = HashMap::new(); + for id in 0..self.config.num_signers { + match self.dkg_public_shares.get(&id) { + Some(shares) => { + public_shares.insert(id, shares.clone()); + } + None => { + return Err(Error::MissingPublicShares(id)); + } + } + } + public_shares + } else { + Default::default() + }; let dkg_begin = DkgPrivateBegin { dkg_id: self.current_dkg_id, key_ids: (1..self.config.num_keys + 1).collect(), signer_ids: (0..self.config.num_signers).collect(), + dkg_public_shares, }; let dkg_private_begin_msg = Packet { sig: dkg_begin @@ -251,10 +268,27 @@ impl Coordinator { dkg_id = %self.current_dkg_id, "Starting DKG End Distribution" ); + let dkg_private_shares = if self.config.embed_public_private_shares { + let mut private_shares = HashMap::new(); + for id in 0..self.config.num_signers { + match self.dkg_private_shares.get(&id) { + Some(shares) => { + private_shares.insert(id, shares.clone()); + } + None => { + return Err(Error::MissingPrivateShares(id)); + } + } + } + private_shares + } else { + Default::default() + }; let dkg_begin = DkgEndBegin { dkg_id: self.current_dkg_id, key_ids: (0..self.config.num_keys).collect(), signer_ids: (0..self.config.num_signers).collect(), + dkg_private_shares, }; let dkg_end_begin_msg = Packet { sig: dkg_begin.sign(&self.config.message_private_key).expect(""), @@ -787,7 +821,7 @@ impl CoordinatorTrait for Coordinator { #[cfg(test)] /// Test module for coordinator functionality -pub mod test { +mod test { use crate::{ curve::scalar::Scalar, net::{DkgBegin, Message, NonceRequest, Packet, SignatureType}, diff --git a/src/state_machine/coordinator/mod.rs b/src/state_machine/coordinator/mod.rs index 8d6bdfb6..2bdf3cd4 100644 --- a/src/state_machine/coordinator/mod.rs +++ b/src/state_machine/coordinator/mod.rs @@ -6,7 +6,7 @@ use crate::{ state_machine::{DkgFailure, OperationResult}, taproot::SchnorrProof, }; -use core::{cmp::PartialEq, fmt::Debug}; +use core::{cmp::PartialEq, fmt::Debug, num::TryFromIntError}; use hashbrown::{HashMap, HashSet}; use std::{ collections::BTreeMap, @@ -78,6 +78,12 @@ pub enum Error { /// Missing message response information for a signing round #[error("Missing message nonce information")] MissingMessageNonceInfo, + /// Missing DkgPublicShares for a signer_id + #[error("Missing DkgPublicShares for signer_id {0}")] + MissingPublicShares(u32), + /// Missing DkgPrivateShares for a signer_id + #[error("Missing DkgPrivateShares for signer_id {0}")] + MissingPrivateShares(u32), /// DKG failure from signers #[error("DKG failure from signers")] DkgFailure(HashMap), @@ -89,6 +95,9 @@ pub enum Error { /// Supplied party polynomial contained duplicate party IDs #[error("Supplied party polynomials contained a duplicate party ID")] DuplicatePartyId, + #[error("integer conversion error")] + /// An error during integer conversion operations + TryFromInt, } impl From for Error { @@ -97,6 +106,12 @@ impl From for Error { } } +impl From for Error { + fn from(_e: TryFromIntError) -> Self { + Self::TryFromInt + } +} + /// Config fields common to all Coordinators #[derive(Default, Clone, Debug, PartialEq)] pub struct Config { @@ -124,6 +139,8 @@ pub struct Config { pub signer_key_ids: HashMap>, /// ECDSA public keys as Point objects indexed by signer_id pub signer_public_keys: HashMap, + /// Embed public and private shares into DkgBegin messages + pub embed_public_private_shares: bool, } impl Config { @@ -147,6 +164,7 @@ impl Config { sign_timeout: None, signer_key_ids: Default::default(), signer_public_keys: Default::default(), + embed_public_private_shares: false, } } @@ -165,6 +183,7 @@ impl Config { sign_timeout: Option, signer_key_ids: HashMap>, signer_public_keys: HashMap, + embed_public_private_shares: bool, ) -> Self { Config { num_signers, @@ -179,6 +198,7 @@ impl Config { sign_timeout, signer_key_ids, signer_public_keys, + embed_public_private_shares, } } } @@ -438,9 +458,12 @@ pub mod test { None, None, None, + false, ) } + #[allow(static_mut_refs)] + #[allow(clippy::too_many_arguments)] pub fn setup_with_timeouts( num_signers: u32, keys_per_signer: u32, @@ -449,6 +472,7 @@ pub mod test { dkg_end_timeout: Option, nonce_timeout: Option, sign_timeout: Option, + embed_public_private_shares: bool, ) -> (Vec, Vec>) { INIT.call_once(|| { tracing_subscriber::registry() @@ -505,6 +529,7 @@ pub mod test { signer_key_ids[&(signer_id as u32)].clone(), *private_key, public_keys.clone(), + embed_public_private_shares, &mut rng, ) }) @@ -525,6 +550,7 @@ pub mod test { sign_timeout, signer_key_ids_set.clone(), signer_public_keys.clone(), + embed_public_private_shares, ); Coordinator::new(config) }) diff --git a/src/state_machine/signer/mod.rs b/src/state_machine/signer/mod.rs index e45128d2..d636634b 100644 --- a/src/state_machine/signer/mod.rs +++ b/src/state_machine/signer/mod.rs @@ -1,3 +1,4 @@ +use core::num::TryFromIntError; use hashbrown::{HashMap, HashSet}; use rand_core::{CryptoRng, RngCore}; use std::collections::{BTreeMap, BTreeSet}; @@ -61,6 +62,15 @@ pub enum Error { /// An encryption error occurred #[error("Encryption error: {0}")] Encryption(#[from] EncryptionError), + #[error("integer conversion error")] + /// An error during integer conversion operations + TryFromInt, +} + +impl From for Error { + fn from(_e: TryFromIntError) -> Self { + Self::TryFromInt + } } /// The saved state required to reconstruct a signer @@ -110,6 +120,8 @@ pub struct SavedState { pub dkg_private_begin_msg: Option, /// the DKG end begin message received in this round pub dkg_end_begin_msg: Option, + /// Embed public and private shares into DkgBegin messages + pub embed_public_private_shares: bool, } /// A state machine for a signing round @@ -159,6 +171,8 @@ pub struct Signer { pub dkg_private_begin_msg: Option, /// the DKG end begin message received in this round pub dkg_end_begin_msg: Option, + /// Embed public and private shares into DkgBegin messages + pub embed_public_private_shares: bool, } impl Signer { @@ -172,6 +186,7 @@ impl Signer { key_ids: Vec, network_private_key: Scalar, public_keys: PublicKeys, + embed_public_private_shares: bool, rng: &mut R, ) -> Self { assert!(threshold <= total_keys); @@ -209,6 +224,7 @@ impl Signer { dkg_private_shares: Default::default(), dkg_private_begin_msg: Default::default(), dkg_end_begin_msg: Default::default(), + embed_public_private_shares, } } @@ -235,6 +251,7 @@ impl Signer { dkg_private_shares: state.dkg_private_shares.clone(), dkg_private_begin_msg: state.dkg_private_begin_msg.clone(), dkg_end_begin_msg: state.dkg_end_begin_msg.clone(), + embed_public_private_shares: state.embed_public_private_shares, } } @@ -261,6 +278,7 @@ impl Signer { dkg_private_shares: self.dkg_private_shares.clone(), dkg_private_begin_msg: self.dkg_private_begin_msg.clone(), dkg_end_begin_msg: self.dkg_end_begin_msg.clone(), + embed_public_private_shares: self.embed_public_private_shares, } } @@ -313,10 +331,20 @@ impl Signer { Message::DkgPrivateBegin(dkg_private_begin) => { self.dkg_private_begin(dkg_private_begin, rng) } - Message::DkgEndBegin(dkg_end_begin) => self.dkg_end_begin(dkg_end_begin), - Message::DkgPublicShares(dkg_public_shares) => self.dkg_public_share(dkg_public_shares), + Message::DkgEndBegin(dkg_end_begin) => self.dkg_end_begin(dkg_end_begin, rng), + Message::DkgPublicShares(dkg_public_shares) => { + if !self.embed_public_private_shares { + self.dkg_public_share(dkg_public_shares) + } else { + Ok(vec![]) + } + } Message::DkgPrivateShares(dkg_private_shares) => { - self.dkg_private_shares(dkg_private_shares, rng) + if !self.embed_public_private_shares { + self.dkg_private_shares(dkg_private_shares, rng) + } else { + Ok(vec![]) + } } Message::SignatureShareRequest(sign_share_request) => { self.sign_share_request(sign_share_request, rng) @@ -352,7 +380,7 @@ impl Signer { let mut missing_public_shares = HashSet::new(); let mut missing_private_shares = HashSet::new(); let mut bad_public_shares = HashSet::new(); - let threshold: usize = self.threshold.try_into().unwrap(); + let threshold: usize = self.threshold.try_into()?; if let Some(dkg_end_begin) = &self.dkg_end_begin_msg { for signer_id in &dkg_end_begin.signer_ids { if let Some(shares) = self.dkg_public_shares.get(signer_id) { @@ -712,6 +740,10 @@ impl Signer { .cloned() .collect::>(); + for (_, shares) in &dkg_private_begin.dkg_public_shares { + let _ = self.dkg_public_share(shares)?; + } + self.dkg_private_begin_msg = Some(dkg_private_begin.clone()); self.move_to(State::DkgPrivateDistribute)?; @@ -760,11 +792,19 @@ impl Signer { } /// handle incoming DkgEndBegin - pub fn dkg_end_begin(&mut self, dkg_end_begin: &DkgEndBegin) -> Result, Error> { + pub fn dkg_end_begin( + &mut self, + dkg_end_begin: &DkgEndBegin, + rng: &mut R, + ) -> Result, Error> { let msgs = vec![]; self.dkg_end_begin_msg = Some(dkg_end_begin.clone()); + for (_, shares) in &dkg_end_begin.dkg_private_shares { + let _ = self.dkg_private_shares(shares, rng)?; + } + info!( signer_id = %self.signer_id, dkg_id = %self.dkg_id, @@ -905,7 +945,9 @@ impl StateMachine for Signer } #[cfg(test)] /// Test module for signer functionality -pub mod test { +mod test { + use hashbrown::HashMap; + use crate::{ common::PolyCommitment, curve::{ecdsa, scalar::Scalar}, @@ -940,6 +982,7 @@ pub mod test { vec![1], Default::default(), Default::default(), + true, &mut rng, ); let public_share = DkgPublicShares { @@ -977,6 +1020,7 @@ pub mod test { vec![1], Default::default(), Default::default(), + true, &mut rng, ); // publich_shares_done starts out as false @@ -1015,8 +1059,17 @@ pub mod test { public_keys.signers.insert(0, public_key.clone()); public_keys.key_ids.insert(1, public_key.clone()); - let mut signer = - Signer::::new(1, 1, 1, 0, vec![1], private_key, public_keys, &mut rng); + let mut signer = Signer::::new( + 1, + 1, + 1, + 0, + vec![1], + private_key, + public_keys, + true, + &mut rng, + ); // can_dkg_end starts out as false assert!(!signer.can_dkg_end()); @@ -1028,10 +1081,17 @@ pub mod test { let _ = signer .process(&dkg_public_shares[0], &mut rng) .expect("failed to process DkgPublicShares"); + let mut public_shares = HashMap::new(); + if let Message::DkgPublicShares(shares) = &dkg_public_shares[0] { + public_shares.insert(0, shares.clone()); + } else { + panic!(""); + } let dkg_private_begin = Message::DkgPrivateBegin(DkgPrivateBegin { dkg_id: 1, signer_ids: vec![0], key_ids: vec![1], + dkg_public_shares: public_shares, }); let dkg_private_shares = signer .process(&dkg_private_begin, &mut rng) @@ -1039,13 +1099,20 @@ pub mod test { let _ = signer .process(&dkg_private_shares[0], &mut rng) .expect("failed to process DkgPrivateShares"); + let mut private_shares = HashMap::new(); + if let Message::DkgPrivateShares(shares) = &dkg_private_shares[0] { + private_shares.insert(0u32, shares.clone()); + } else { + panic!(""); + } let dkg_end_begin = DkgEndBegin { dkg_id: 1, signer_ids: vec![0], key_ids: vec![1], + dkg_private_shares: private_shares, }; let _ = signer - .dkg_end_begin(&dkg_end_begin) + .dkg_end_begin(&dkg_end_begin, &mut rng) .expect("failed to process DkgPrivateShares"); // can_dkg_end should be true @@ -1068,6 +1135,7 @@ pub mod test { .with(fmt::layer()) .with(EnvFilter::from_default_env()) .init();*/ + let mut rng = create_rng(); let mut signer = Signer::::new( 1, @@ -1077,6 +1145,7 @@ pub mod test { vec![1], Default::default(), Default::default(), + true, &mut rng, );