diff --git a/src/common.rs b/src/common.rs index 89e35702..fad2216a 100644 --- a/src/common.rs +++ b/src/common.rs @@ -321,7 +321,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 c427c89c..eeb35a47 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 @@ -1270,7 +1287,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::{ @@ -1524,17 +1541,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); @@ -1547,6 +1574,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 @@ -1605,17 +1633,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); @@ -1628,6 +1667,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 @@ -1760,15 +1800,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; @@ -1781,6 +1833,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 @@ -2209,20 +2262,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 @@ -2293,20 +2361,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"); @@ -2380,15 +2463,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) = @@ -2400,6 +2495,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..a5831fdc 100644 --- a/src/state_machine/coordinator/frost.rs +++ b/src/state_machine/coordinator/frost.rs @@ -229,10 +229,18 @@ impl Coordinator { dkg_id = %self.current_dkg_id, "Starting Private Share Distribution" ); + let dkg_public_shares = if self.config.embed_public_private_shares { + (0..self.config.num_signers) + .map(|id| (id, self.dkg_public_shares[&id].clone())) + .collect() + } 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 +259,18 @@ impl Coordinator { dkg_id = %self.current_dkg_id, "Starting DKG End Distribution" ); + let dkg_private_shares = if self.config.embed_public_private_shares { + (0..self.config.num_signers) + .map(|id| (id, self.dkg_private_shares[&id].clone())) + .collect() + } 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 +803,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 11f1b8d4..a68c210f 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, @@ -89,6 +89,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 +100,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 +133,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 +158,7 @@ impl Config { sign_timeout: None, signer_key_ids: Default::default(), signer_public_keys: Default::default(), + embed_public_private_shares: false, } } @@ -165,6 +177,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 +192,7 @@ impl Config { sign_timeout, signer_key_ids, signer_public_keys, + embed_public_private_shares, } } } @@ -438,9 +452,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 +466,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 +523,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 +544,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 214d45e2..51766a73 100644 --- a/src/state_machine/signer/mod.rs +++ b/src/state_machine/signer/mod.rs @@ -1,3 +1,5 @@ +use aes_gcm::Error as AesGcmError; +use core::num::TryFromIntError; use hashbrown::{HashMap, HashSet}; use rand_core::{CryptoRng, RngCore}; use std::collections::BTreeMap; @@ -61,6 +63,24 @@ pub enum Error { /// An encryption error occurred #[error("Encryption error: {0}")] Encryption(#[from] EncryptionError), + /// An AES-GCM error occurred + #[error("AES-GCM: {0}")] + AesGcm(AesGcmError), + #[error("integer conversion error")] + /// An error during integer conversion operations + TryFromInt, +} + +impl From for Error { + fn from(err: AesGcmError) -> Self { + Error::AesGcm(err) + } +} + +impl From for Error { + fn from(_e: TryFromIntError) -> Self { + Self::TryFromInt + } } /// The saved state required to reconstruct a signer @@ -110,6 +130,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 +181,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 +196,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 +234,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 +261,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 +288,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 +341,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 +390,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) { @@ -694,6 +732,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)?; @@ -742,11 +784,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, @@ -887,7 +937,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}, @@ -922,6 +974,7 @@ pub mod test { vec![1], Default::default(), Default::default(), + true, &mut rng, ); let public_share = DkgPublicShares { @@ -959,6 +1012,7 @@ pub mod test { vec![1], Default::default(), Default::default(), + true, &mut rng, ); // publich_shares_done starts out as false @@ -997,8 +1051,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()); @@ -1010,10 +1073,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) @@ -1021,13 +1091,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 @@ -1050,6 +1127,7 @@ pub mod test { .with(fmt::layer()) .with(EnvFilter::from_default_env()) .init();*/ + let mut rng = create_rng(); let mut signer = Signer::::new( 1, @@ -1059,6 +1137,7 @@ pub mod test { vec![1], Default::default(), Default::default(), + true, &mut rng, );