From b5228e89cf9dabbb5119e66f7dc6b28a5b7f9644 Mon Sep 17 00:00:00 2001 From: Joey Yandle Date: Fri, 13 Dec 2024 10:11:22 -0500 Subject: [PATCH] move public shares check to common code, and call it from everywhere we check public shares add test where one signer has incorrect threshold use common code to check public shares --- src/common.rs | 5 ++ src/state_machine/coordinator/fire.rs | 123 +++++++++++++++++++++++++- src/state_machine/signer/mod.rs | 4 +- src/v1.rs | 9 +- src/v2.rs | 6 +- 5 files changed, 135 insertions(+), 12 deletions(-) diff --git a/src/common.rs b/src/common.rs index 89e35702..3f962340 100644 --- a/src/common.rs +++ b/src/common.rs @@ -214,6 +214,11 @@ impl TupleProof { } } +/// Check that the PolyCommitment is properly signed and has the correct degree polynomial +pub fn check_public_shares(poly_comm: &PolyCommitment, threshold: usize) -> bool { + poly_comm.verify() && poly_comm.poly.len() == threshold +} + /// An implementation of p256k1's MultiMult trait that allows fast checking of DKG private shares /// We convert a set of checked polynomial evaluations into a single giant multimult /// These evaluations take the form of s * G == \Sum{k=0}{T+1}(a_k * x^k) where the a vals are the coeffs of the polys diff --git a/src/state_machine/coordinator/fire.rs b/src/state_machine/coordinator/fire.rs index c427c89c..c3708a4d 100644 --- a/src/state_machine/coordinator/fire.rs +++ b/src/state_machine/coordinator/fire.rs @@ -3,7 +3,7 @@ use std::{collections::BTreeMap, time::Instant}; use tracing::{debug, error, info, warn}; use crate::{ - common::{PolyCommitment, PublicNonce, Signature, SignatureShare}, + common::{check_public_shares, PolyCommitment, PublicNonce, Signature, SignatureShare}, compute, curve::{ point::{Point, G}, @@ -568,7 +568,7 @@ impl Coordinator { } let mut dkg_failures = HashMap::new(); - + let threshold: usize = self.config.threshold.try_into().unwrap(); if self.dkg_wait_signer_ids.is_empty() { // if there are any errors, mark signers malicious and retry for (signer_id, dkg_end) in &self.dkg_end_messages { @@ -585,7 +585,7 @@ impl Coordinator { let dkg_public_shares = &self.dkg_public_shares[bad_signer_id]; let mut bad_party_ids = Vec::new(); for (party_id, comm) in &dkg_public_shares.comms { - if !comm.verify() { + if !check_public_shares(comm, threshold) { bad_party_ids.push(party_id); } } @@ -597,6 +597,9 @@ impl Coordinator { } else { warn!("Signer {} reported BadPublicShares from {}, mark {} as malicious", signer_id, bad_signer_id, bad_signer_id); self.malicious_dkg_signer_ids.insert(*bad_signer_id); + + // save legitimate failures to return to caller + dkg_failures.insert(*signer_id, dkg_failure.clone()); } } } @@ -677,12 +680,14 @@ impl Coordinator { } else { warn!("Signer {} reported BadPrivateShare from {}, mark {} as malicious", signer_id, bad_signer_id, bad_signer_id); self.malicious_dkg_signer_ids.insert(*bad_signer_id); + + // save legitimate failures to return to caller + dkg_failures.insert(*signer_id, dkg_failure.clone()); } } } _ => (), } - dkg_failures.insert(*signer_id, dkg_failure.clone()); } } if dkg_failures.is_empty() { @@ -2822,4 +2827,114 @@ pub mod test { fn gen_nonces_v2() { gen_nonces::, v2::Signer>(5, 1); } + + #[test] + fn one_signer_bad_threshold_v1() { + one_signer_bad_threshold::(); + } + + #[test] + fn one_signer_bad_threshold_v2() { + one_signer_bad_threshold::(); + } + + fn one_signer_bad_threshold() { + let mut rng = create_rng(); + let (mut coordinators, mut signers) = + setup::, SignerType>(10, 1); + + // persist one signer, change the threshold, reset polys + let mut state = signers[0].save(); + + state.threshold -= 1; + state.signer.threshold -= 1; + + signers[0] = Signer::::load(&state); + + signers[0].signer.reset_polys(&mut rng); + + // 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_eq!(operation_results.len(), 0); + assert_eq!(outbound_messages.len(), 1); + match &outbound_messages[0].msg { + Message::DkgEndBegin(_) => {} + _ => { + panic!("Expected DkgEndBegin message"); + } + } + + // 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, &outbound_messages); + assert_eq!(outbound_messages.len(), 0); + assert_eq!(operation_results.len(), 1); + match &operation_results[0] { + OperationResult::DkgError(DkgError::DkgEndFailure(failure_map)) => { + for i in 1..10 { + match failure_map.get(&i) { + Some(DkgFailure::BadPublicShares(set)) => { + if set.len() != 1 { + panic!( + "signer {} should have reported a single BadPublicShares", + i + ); + } else if !set.contains(&0) { + panic!( + "signer {} should have reported BadPublicShares from signer 0", + i + ); + } + } + Some(failure) => { + panic!("signer {} should have reported BadPublicShares, instead reported {:?}", i, failure); + } + None => { + panic!("signer {} should have reported BadPublicShares", i); + } + } + } + + match failure_map.get(&0) { + Some(failure) => { + panic!("Coordinator should not have passed along incorrect failure {:?} from signer 0", failure); + } + None => {} + } + } + result => panic!( + "Expected OperationResult::DkgError(DkgError::DkgEndFailure), got {:?}", + &result + ), + } + } } diff --git a/src/state_machine/signer/mod.rs b/src/state_machine/signer/mod.rs index 214d45e2..02772f75 100644 --- a/src/state_machine/signer/mod.rs +++ b/src/state_machine/signer/mod.rs @@ -4,7 +4,7 @@ use std::collections::BTreeMap; use tracing::{debug, info, trace, warn}; use crate::{ - common::{PolyCommitment, PublicNonce, TupleProof}, + common::{check_public_shares, PolyCommitment, PublicNonce, TupleProof}, curve::{ point::{Compressed, Point, G}, scalar::Scalar, @@ -357,7 +357,7 @@ impl Signer { for signer_id in &dkg_end_begin.signer_ids { if let Some(shares) = self.dkg_public_shares.get(signer_id) { for (party_id, comm) in shares.comms.iter() { - if comm.poly.len() != threshold || !comm.verify() { + if !check_public_shares(comm, threshold) { bad_public_shares.insert(*signer_id); } else { self.commitments.insert(*party_id, comm.clone()); diff --git a/src/v1.rs b/src/v1.rs index fa1a66ac..0633c45f 100644 --- a/src/v1.rs +++ b/src/v1.rs @@ -5,7 +5,10 @@ use rand_core::{CryptoRng, RngCore}; use tracing::warn; use crate::{ - common::{CheckPrivateShares, Nonce, PolyCommitment, PublicNonce, Signature, SignatureShare}, + common::{ + check_public_shares, CheckPrivateShares, Nonce, PolyCommitment, PublicNonce, Signature, + SignatureShare, + }, compute, curve::{ point::{Point, G}, @@ -139,7 +142,7 @@ impl Party { let threshold: usize = self.threshold.try_into()?; let mut bad_ids = Vec::new(); //: Vec = polys for (i, comm) in public_shares.iter() { - if comm.poly.len() != threshold || !comm.verify() { + if !check_public_shares(comm, threshold) { bad_ids.push(*i); } else { self.group_key += comm.poly[0]; @@ -420,7 +423,7 @@ impl traits::Aggregator for Aggregator { let threshold = self.threshold.try_into()?; let mut bad_poly_commitments = Vec::new(); for (_id, comm) in comms { - if comm.poly.len() != threshold || !comm.verify() { + if !check_public_shares(comm, threshold) { bad_poly_commitments.push(comm.id.id); } } diff --git a/src/v2.rs b/src/v2.rs index a24e2354..2faf5a29 100644 --- a/src/v2.rs +++ b/src/v2.rs @@ -5,7 +5,7 @@ use rand_core::{CryptoRng, RngCore}; use tracing::warn; use crate::{ - common::{Nonce, PolyCommitment, PublicNonce, Signature, SignatureShare}, + common::{check_public_shares, Nonce, PolyCommitment, PublicNonce, Signature, SignatureShare}, compute, curve::{ point::{Point, G}, @@ -112,7 +112,7 @@ impl Party { let mut bad_ids = Vec::new(); for (i, comm) in public_shares.iter() { - if comm.poly.len() != threshold || !comm.verify() { + if !check_public_shares(comm, threshold) { bad_ids.push(*i); } else { self.group_key += comm.poly[0]; @@ -411,7 +411,7 @@ impl traits::Aggregator for Aggregator { let threshold: usize = self.threshold.try_into()?; let mut bad_poly_commitments = Vec::new(); for (_id, comm) in comms { - if comm.poly.len() != threshold || !comm.verify() { + if !check_public_shares(comm, threshold) { bad_poly_commitments.push(comm.id.id); } }