cherry-pick dkg_threshold fix into hotfix branch (#11)

* Check DKG threshold in Signer state machine (#9)

src/common.rs

@@ -320,7 +320,7 @@ pub mod test_helpers {
 }
 
 #[cfg(test)]
-pub mod test {
+mod test {
     use rand_core::OsRng;
 
     use crate::{

src/net.rs

@@ -57,6 +57,8 @@ pub struct BadPrivateShare {
 #[derive(Clone, Serialize, Deserialize, Debug, PartialEq)]
 /// Final DKG status after receiving public and private shares
 pub enum DkgFailure {
+    /// DKG threshold not met
+    Threshold,
     /// Signer was in the wrong internal state to complete DKG
     BadState,
     /// DKG public shares were missing from these signer_ids
@@ -625,12 +627,17 @@ mod test {
             let signer_private_key = Scalar::random(&mut rng);
             let signer_public_key = ecdsa::PublicKey::new(&signer_private_key).unwrap();
             let mut signer_ids_map = HashMap::new();
+            let mut signer_key_ids = HashMap::new();
             let mut key_ids_map = HashMap::new();
+            let mut key_ids_set = HashSet::new();
             signer_ids_map.insert(0, signer_public_key);
             key_ids_map.insert(1, signer_public_key);
+            key_ids_set.insert(1);
+            signer_key_ids.insert(0, key_ids_set);
             let public_keys = PublicKeys {
                 signers: signer_ids_map,
                 key_ids: key_ids_map,
+                signer_key_ids,
             };
             let coordinator_private_key = Scalar::random(&mut rng);
             let coordinator_public_key = ecdsa::PublicKey::new(&coordinator_private_key).unwrap();

src/state_machine/coordinator/fire.rs

@@ -419,16 +419,11 @@ impl<Aggregator: AggregatorTrait> Coordinator<Aggregator> {
             "DKG Round {}: Starting Private Share Distribution",
             self.current_dkg_id
         );
-        let active_key_ids = self
-            .dkg_public_shares
-            .keys()
-            .flat_map(|signer_id| self.config.signer_key_ids[signer_id].clone())
-            .collect::<Vec<u32>>();
 
         let dkg_begin = DkgPrivateBegin {
             dkg_id: self.current_dkg_id,
-            key_ids: active_key_ids,
             signer_ids: self.dkg_public_shares.keys().cloned().collect(),
+            key_ids: vec![],
         };
         let dkg_private_begin_msg = Packet {
             sig: dkg_begin
@@ -453,16 +448,11 @@ impl<Aggregator: AggregatorTrait> Coordinator<Aggregator> {
             "DKG Round {}: Starting DkgEnd Distribution",
             self.current_dkg_id
         );
-        let active_key_ids = self
-            .dkg_private_shares
-            .keys()
-            .flat_map(|signer_id| self.config.signer_key_ids[signer_id].clone())
-            .collect::<Vec<u32>>();
 
         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(),
+            key_ids: vec![],
         };
         let dkg_end_begin_msg = Packet {
             sig: dkg_end_begin
@@ -568,10 +558,15 @@ impl<Aggregator: AggregatorTrait> Coordinator<Aggregator> {
             // if there are any errors, mark signers malicious and retry
             for (signer_id, dkg_end) in &self.dkg_end_messages {
                 if let DkgStatus::Failure(dkg_failure) = &dkg_end.status {
+                    warn!(%signer_id, ?dkg_failure, "DkgEnd failure");
                     match dkg_failure {
                         DkgFailure::BadState => {
                             // signer should not be in a bad state so treat as malicious
                             self.malicious_dkg_signer_ids.insert(*signer_id);
+                            dkg_failures.insert(*signer_id, dkg_failure.clone());
+                        }
+                        DkgFailure::Threshold => {
+                            dkg_failures.insert(*signer_id, dkg_failure.clone());
                         }
                         DkgFailure::BadPublicShares(bad_shares) => {
                             // bad_shares is a set of signer_ids
@@ -675,15 +670,22 @@ impl<Aggregator: AggregatorTrait> Coordinator<Aggregator> {
                                 }
                             }
                         }
-                        _ => (),
+                        DkgFailure::MissingPublicShares(_) => {
+                            dkg_failures.insert(*signer_id, dkg_failure.clone());
+                        }
+                        DkgFailure::MissingPrivateShares(_) => {
+                            dkg_failures.insert(*signer_id, dkg_failure.clone());
+                        }
                     }
                     dkg_failures.insert(*signer_id, dkg_failure.clone());
                 }
             }
             if dkg_failures.is_empty() {
+                warn!("no dkg failures");
                 self.dkg_end_gathered()?;
             } else {
                 // TODO: see if we have sufficient non-malicious signers to continue
+                warn!("got dkg failures");
                 return Err(Error::DkgFailure(dkg_failures));
             }
         }
@@ -1261,7 +1263,7 @@ impl<Aggregator: AggregatorTrait> CoordinatorTrait for Coordinator<Aggregator> {
 }
 
 #[cfg(test)]
-pub mod test {
+mod test {
     use crate::{
         curve::{point::Point, scalar::Scalar},
         net::{
@@ -1486,7 +1488,7 @@ pub mod test {
         // Send the DKG Private Begin message to all signers and share their responses with the coordinators and signers
         let (outbound_messages, operation_results) =
             feedback_messages(&mut coordinators, &mut signers, &outbound_messages);
-        assert_eq!(operation_results.len(), 0);
+        assert!(operation_results.is_empty());
         assert_eq!(outbound_messages.len(), 1);
         match &outbound_messages[0].msg {
             Message::DkgEndBegin(_) => {}
@@ -1498,7 +1500,7 @@ pub mod test {
         // Send the DkgEndBegin message to all signers and share their responses with the coordinators and signers
         let (outbound_messages, operation_results) =
             feedback_messages(&mut coordinators, &mut signers, &outbound_messages);
-        assert_eq!(outbound_messages.len(), 0);
+        assert!(outbound_messages.is_empty());
         assert_eq!(operation_results.len(), 1);
         match operation_results[0] {
             OperationResult::Dkg(point) => {
@@ -1572,8 +1574,8 @@ pub mod test {
             &[message.clone()],
         );
 
-        assert_eq!(outbound_messages.len(), 0);
-        assert_eq!(operation_results.len(), 0);
+        assert!(outbound_messages.is_empty());
+        assert!(operation_results.is_empty());
         assert_eq!(coordinators.first().unwrap().state, State::DkgPublicGather,);
 
         // Sleep long enough to hit the timeout
@@ -1586,7 +1588,7 @@ pub mod test {
             .unwrap();
 
         assert_eq!(outbound_messages.len(), 1);
-        assert_eq!(operation_results.len(), 0);
+        assert!(operation_results.is_empty());
         assert_eq!(
             minimum_coordinators.first().unwrap().state,
             State::DkgPrivateGather,
@@ -1643,8 +1645,8 @@ pub mod test {
             &[message.clone()],
         );
 
-        assert_eq!(outbound_messages.len(), 0);
-        assert_eq!(operation_results.len(), 0);
+        assert!(outbound_messages.is_empty());
+        assert!(operation_results.is_empty());
         assert_eq!(
             minimum_coordinators.first().unwrap().state,
             State::DkgPublicGather,
@@ -1660,7 +1662,7 @@ pub mod test {
             .unwrap();
 
         assert_eq!(outbound_messages.len(), 1);
-        assert_eq!(operation_results.len(), 0);
+        assert!(operation_results.is_empty());
         assert_eq!(
             minimum_coordinators.first().unwrap().state,
             State::DkgPrivateGather,
@@ -1699,7 +1701,7 @@ pub mod test {
             &outbound_messages,
         );
         assert!(outbound_messages.is_empty());
-        assert_eq!(operation_results.len(), 0);
+        assert!(operation_results.is_empty());
         assert_eq!(
             minimum_coordinator.first().unwrap().state,
             State::DkgPrivateGather,
@@ -1715,7 +1717,7 @@ pub mod test {
             .unwrap();
 
         assert_eq!(outbound_messages.len(), 1);
-        assert_eq!(operation_results.len(), 0);
+        assert!(operation_results.is_empty());
         match &outbound_messages[0].msg {
             Message::DkgEndBegin(_) => {}
             _ => {
@@ -1733,7 +1735,7 @@ pub mod test {
             &mut minimum_signers,
             &outbound_messages,
         );
-        assert_eq!(outbound_messages.len(), 0);
+        assert!(outbound_messages.is_empty());
         assert_eq!(operation_results.len(), 1);
         match operation_results[0] {
             OperationResult::Dkg(point) => {
@@ -1797,8 +1799,8 @@ pub mod test {
         );
 
         // Failed to get an aggregate public key
-        assert_eq!(outbound_messages.len(), 0);
-        assert_eq!(operation_results.len(), 0);
+        assert!(outbound_messages.is_empty());
+        assert!(operation_results.is_empty());
         for coordinator in &insufficient_coordinators {
             assert_eq!(coordinator.state, State::DkgPublicGather);
         }
@@ -1863,7 +1865,7 @@ pub mod test {
             &outbound_messages,
         );
         assert!(outbound_messages.is_empty());
-        assert_eq!(operation_results.len(), 0);
+        assert!(operation_results.is_empty());
         assert_eq!(
             insufficient_coordinator.first().unwrap().state,
             State::DkgPrivateGather,
@@ -1977,7 +1979,7 @@ pub mod test {
                 }
             },
         );
-        assert_eq!(operation_results.len(), 0);
+        assert!(operation_results.is_empty());
         assert_eq!(outbound_messages.len(), 1);
         match &outbound_messages[0].msg {
             Message::DkgEndBegin(_) => {}
@@ -1989,7 +1991,7 @@ pub mod test {
         // Send the DkgEndBegin message to all signers and share their responses with the coordinators and signers
         let (outbound_messages, operation_results) =
             feedback_messages(&mut coordinators, &mut signers, &outbound_messages);
-        assert_eq!(outbound_messages.len(), 0);
+        assert!(outbound_messages.is_empty());
         assert_eq!(operation_results.len(), 1);
         match &operation_results[0] {
             OperationResult::DkgError(dkg_error) => {
@@ -2094,7 +2096,7 @@ pub mod test {
 
         let (outbound_messages, operation_results) =
             feedback_messages(&mut coordinators, &mut signers, &outbound_messages);
-        assert_eq!(operation_results.len(), 0);
+        assert!(operation_results.is_empty());
         assert_eq!(outbound_messages.len(), 1);
         match &outbound_messages[0].msg {
             Message::DkgEndBegin(_) => {}
@@ -2106,7 +2108,7 @@ pub mod test {
         // Send the DkgEndBegin message to all signers and share their responses with the coordinators and signers
         let (outbound_messages, operation_results) =
             feedback_messages(&mut coordinators, &mut signers, &outbound_messages);
-        assert_eq!(outbound_messages.len(), 0);
+        assert!(outbound_messages.is_empty());
         assert_eq!(operation_results.len(), 1);
         match &operation_results[0] {
             OperationResult::DkgError(dkg_error) => {
@@ -2418,7 +2420,7 @@ pub mod test {
         // Send the DKG Private Begin message to all signers and share their responses with the coordinators and signers
         let (outbound_messages, operation_results) =
             feedback_messages(&mut coordinators, &mut signers, &outbound_messages);
-        assert_eq!(operation_results.len(), 0);
+        assert!(operation_results.is_empty());
         assert_eq!(outbound_messages.len(), 1);
         match &outbound_messages[0].msg {
             Message::DkgEndBegin(_) => {}
@@ -2561,7 +2563,7 @@ pub mod test {
             .unwrap();
 
         assert_eq!(outbound_messages.len(), 1);
-        assert_eq!(operation_results.len(), 0);
+        assert!(operation_results.is_empty());
         assert_eq!(
             insufficient_coordinators.first().unwrap().state,
             State::NonceGather(signature_type)
@@ -2579,7 +2581,7 @@ pub mod test {
             &outbound_messages,
         );
         assert_eq!(outbound_messages.len(), 1);
-        assert_eq!(operation_results.len(), 0);
+        assert!(operation_results.is_empty());
 
         for coordinator in &insufficient_coordinators {
             assert_eq!(coordinator.state, State::SigShareGather(signature_type));
@@ -2612,7 +2614,7 @@ pub mod test {
             .process_inbound_messages(&[])
             .unwrap();
 
-        assert_eq!(outbound_messages.len(), 0);
+        assert!(outbound_messages.is_empty());
         assert_eq!(operation_results.len(), 1);
         assert_eq!(
             insufficient_coordinators.first_mut().unwrap().state,
@@ -2803,4 +2805,83 @@ pub mod test {
             assert_eq!(coordinator.current_sign_id, id);
         }
     }
+
+    #[test]
+    fn bad_dkg_threshold_v1() {
+        bad_dkg_threshold::<v1::Aggregator, v1::Signer>();
+    }
+
+    #[test]
+    fn bad_dkg_threshold_v2() {
+        bad_dkg_threshold::<v2::Aggregator, v2::Signer>();
+    }
+
+    fn bad_dkg_threshold<Aggregator: AggregatorTrait, SignerType: SignerTrait>() {
+        let (mut coordinators, mut signers) =
+            setup::<FireCoordinator<Aggregator>, SignerType>(10, 1);
+
+        // We have started a dkg round
+        let message = coordinators.first_mut().unwrap().start_dkg_round().unwrap();
+        assert!(coordinators
+            .first_mut()
+            .unwrap()
+            .get_aggregate_public_key()
+            .is_none());
+        assert_eq!(
+            coordinators.first_mut().unwrap().get_state(),
+            State::DkgPublicGather
+        );
+
+        // Send the DKG Begin message to all signers and gather responses by sharing with all other signers and coordinator
+        let (outbound_messages, operation_results) =
+            feedback_messages(&mut coordinators, &mut signers, &[message]);
+        assert!(operation_results.is_empty());
+        for coordinator in coordinators.iter() {
+            assert_eq!(coordinator.get_state(), State::DkgPrivateGather);
+        }
+
+        assert_eq!(outbound_messages.len(), 1);
+        match &outbound_messages[0].msg {
+            Message::DkgPrivateBegin(_) => {}
+            _ => {
+                panic!("Expected DkgPrivateBegin message");
+            }
+        }
+
+        // Send the DKG Private Begin message to all signers and share their responses with the coordinator and signers
+        let (outbound_messages, operation_results) =
+            feedback_messages(&mut coordinators, &mut signers, &outbound_messages);
+        assert!(operation_results.is_empty());
+        assert_eq!(outbound_messages.len(), 1);
+        match &outbound_messages[0].msg {
+            Message::DkgEndBegin(_) => {}
+            _ => {
+                panic!("Expected DkgEndBegin message");
+            }
+        }
+
+        // alter the DkgEndBegin message
+        let mut packet = outbound_messages[0].clone();
+        if let Message::DkgEndBegin(ref mut dkg_end_begin) = packet.msg {
+            dkg_end_begin.signer_ids = vec![0u32];
+        }
+
+        // Send the DkgEndBegin message to all signers and share their responses with the coordinator and signers
+        let (outbound_messages, operation_results) =
+            feedback_messages(&mut coordinators, &mut signers, &[packet]);
+        assert!(outbound_messages.is_empty());
+        assert_eq!(operation_results.len(), 1);
+        match &operation_results[0] {
+            OperationResult::DkgError(DkgError::DkgEndFailure(failure_map)) => {
+                for (signer_id, failure) in failure_map {
+                    if !matches!(failure, DkgFailure::Threshold) {
+                        panic!("{signer_id} had wrong failure {:?}", failure);
+                    }
+                }
+            }
+            result => {
+                panic!("Expected DkgEndFailure got {:?}", result);
+            }
+        }
+    }
 }

src/state_machine/coordinator/frost.rs

@@ -761,7 +761,7 @@ impl<Aggregator: AggregatorTrait> CoordinatorTrait for Coordinator<Aggregator> {
 }
 
 #[cfg(test)]
-pub mod test {
+mod test {
     use crate::{
         curve::scalar::Scalar,
         net::{DkgBegin, Message, NonceRequest, Packet, SignatureType},