Clean up error handling in compute_secrets (#95)

* remove NotEnoughShares DkgError and instead specify for/from in MissingPrivateShares DkgError; add TryFromInt DkgError and remove unwrap call in compute_secret; clean up tests

* remove unwrap in v1 compute_secret

* add TryFromInt to AggregatorError; remove moar unwraps from v1 and v2

* remove unused AggregatorError::BadPolyCommitmentLen since we're making API breaking changes anyway

* do full missing_private_shares check in signer state machine so we don't have to worry about getting anything other than bad private shares from compute secrets

* use get rather than indexing into hashmaps to be extra safe, even though we checked above that the keys should be present

* use fold to sum private shares since we don't impl Sum for Scalar

* use Sum from p256k1 7.2; impl PartialEq for PartyState and SignerState

Cargo.toml

@@ -23,7 +23,7 @@ num-traits = "0.2"
 polynomial = { version = "0.2.5", features = ["serde"] }
 primitive-types = "0.12"
 rand_core = "0.6"
-p256k1 = { version = "7.1", default-features = false }
+p256k1 = { version = "7.2", default-features = false }
 serde = { version = "1.0", features = ["derive"] }
 sha2 = "0.10"
 thiserror = "1.0"

src/errors.rs

@@ -1,3 +1,4 @@
+use core::num::TryFromIntError;
 use serde::{Deserialize, Serialize};
 use thiserror::Error;
 
@@ -9,21 +10,21 @@ pub enum DkgError {
     #[error("missing public shares from {0:?}")]
     /// The public shares which were missing
     MissingPublicShares(Vec<u32>),
-    #[error("missing private shares from {0:?}")]
+    #[error("missing private shares for/from {0:?}")]
     /// The private shares which were missing
-    MissingPrivateShares(Vec<u32>),
+    MissingPrivateShares(Vec<(u32, u32)>),
     #[error("bad public shares {0:?}")]
     /// The public shares that failed to verify or were the wrong size
     BadPublicShares(Vec<u32>),
-    #[error("not enough shares {0:?}")]
-    /// Not enough shares to complete DKG
-    NotEnoughShares(Vec<u32>),
     #[error("bad private shares {0:?}")]
     /// The private shares which failed to verify
     BadPrivateShares(Vec<u32>),
     #[error("point error {0:?}")]
     /// An error during point operations
     Point(PointError),
+    #[error("integer conversion error")]
+    /// An error during integer conversion operations
+    TryFromInt,
 }
 
 impl From<PointError> for DkgError {
@@ -32,12 +33,15 @@ impl From<PointError> for DkgError {
     }
 }
 
+impl From<TryFromIntError> for DkgError {
+    fn from(_e: TryFromIntError) -> Self {
+        Self::TryFromInt
+    }
+}
+
 #[derive(Error, Debug, Clone, Serialize, Deserialize, PartialEq)]
 /// Errors which can happen during signature aggregation
 pub enum AggregatorError {
-    #[error("bad poly commitment length (expected {0} got {1})")]
-    /// The number of polynomial commitments was wrong (no longer used)
-    BadPolyCommitmentLen(usize, usize),
     #[error("bad poly commitments {0:?}")]
     /// The polynomial commitments which failed verification or were the wrong size
     BadPolyCommitments(Vec<Scalar>),
@@ -53,4 +57,13 @@ pub enum AggregatorError {
     #[error("bad group sig")]
     /// The aggregate group signature failed to verify
     BadGroupSig,
+    #[error("integer conversion error")]
+    /// An error during integer conversion operations
+    TryFromInt,
+}
+
+impl From<TryFromIntError> for AggregatorError {
+    fn from(_e: TryFromIntError) -> Self {
+        Self::TryFromInt
+    }
 }

src/state_machine/signer/mod.rs

@@ -363,8 +363,15 @@ impl<SignerType: SignerTrait> Signer<SignerType> {
                 } else {
                     missing_public_shares.insert(*signer_id);
                 }
-                if let Some(_shares) = self.dkg_private_shares.get(signer_id) {
-                    // TODO: consider move private shares checks here
+                if let Some(shares) = self.dkg_private_shares.get(signer_id) {
+                    // signer_id sent shares, but make sure that it sent shares for every one of this signer's key_ids
+                    for dst_key_id in self.signer.get_key_ids() {
+                        for (_src_key_id, shares) in &shares.shares {
+                            if shares.get(&dst_key_id).is_none() {
+                                missing_private_shares.insert(*signer_id);
+                            }
+                        }
+                    }
                 } else {
                     missing_private_shares.insert(*signer_id);
                 }

src/traits.rs

@@ -11,7 +11,7 @@ use crate::{
     taproot::SchnorrProof,
 };
 
-#[derive(Clone, Debug, Deserialize, Serialize)]
+#[derive(Clone, Debug, Deserialize, Serialize, PartialEq)]
 /// The saved state required to reconstruct a party
 pub struct PartyState {
     /// The party's private polynomial
@@ -22,7 +22,7 @@ pub struct PartyState {
     pub nonce: Nonce,
 }
 
-#[derive(Clone, Debug, Deserialize, Serialize)]
+#[derive(Clone, Debug, Deserialize, Serialize, PartialEq)]
 /// The saved state required to reconstruct a signer
 pub struct SignerState {
     /// The signer ID
@@ -209,7 +209,7 @@ pub mod test_helpers {
     }
 
     /// Remove the provided key ids from the list of private shares and execute compute secrets
-    fn compute_secrets_not_enough_shares<RNG: RngCore + CryptoRng, Signer: super::Signer>(
+    fn compute_secrets_missing_private_shares<RNG: RngCore + CryptoRng, Signer: super::Signer>(
         signers: &mut [Signer],
         rng: &mut RNG,
         missing_key_ids: &[u32],
@@ -251,8 +251,8 @@ pub mod test_helpers {
     }
 
     #[allow(non_snake_case)]
-    /// Run compute secrets test to trigger NotEnoughShares code path
-    pub fn run_compute_secrets_not_enough_shares<Signer: super::Signer>() {
+    /// Run compute secrets test to trigger MissingPrivateShares code path
+    pub fn run_compute_secrets_missing_private_shares<Signer: super::Signer>() {
         let Nk: u32 = 10;
         let Np: u32 = 4;
         let T: u32 = 7;
@@ -265,11 +265,11 @@ pub mod test_helpers {
             .map(|(id, ids)| Signer::new(id.try_into().unwrap(), ids, Nk, Np, T, &mut rng))
             .collect();
 
-        match compute_secrets_not_enough_shares(&mut signers, &mut rng, &missing_key_ids) {
+        match compute_secrets_missing_private_shares(&mut signers, &mut rng, &missing_key_ids) {
             Ok(polys) => panic!("Got a result with missing public shares: {polys:?}"),
             Err(secret_errors) => {
                 for (_, error) in secret_errors {
-                    assert!(matches!(error, DkgError::NotEnoughShares(_)));
+                    assert!(matches!(error, DkgError::MissingPrivateShares(_)));
                 }
             }
         }

src/v1.rs

@@ -130,28 +130,15 @@ impl Party {
     /// Compute this party's share of the group secret key
     pub fn compute_secret(
         &mut self,
-        shares: HashMap<u32, Scalar>,
-        polys: &HashMap<u32, PolyCommitment>,
+        private_shares: HashMap<u32, Scalar>,
+        public_shares: &HashMap<u32, PolyCommitment>,
     ) -> Result<(), DkgError> {
-        if shares.len() != polys.len() {
-            return Err(DkgError::NotEnoughShares(vec![self.id]));
-        }
-        let mut missing_shares = Vec::new();
-        for i in polys.keys() {
-            if shares.get(i).is_none() {
-                missing_shares.push(*i);
-            }
-        }
-        if !missing_shares.is_empty() {
-            return Err(DkgError::MissingPublicShares(missing_shares));
-        }
-
         self.private_key = Scalar::zero();
         self.group_key = Point::zero();
 
-        let threshold: usize = self.threshold.try_into().unwrap();
+        let threshold: usize = self.threshold.try_into()?;
         let mut bad_ids = Vec::new(); //: Vec<u32> = polys
-        for (i, comm) in polys.iter() {
+        for (i, comm) in public_shares.iter() {
             if comm.poly.len() != threshold || !comm.verify() {
                 bad_ids.push(*i);
             } else {
@@ -162,27 +149,39 @@ impl Party {
             return Err(DkgError::BadPublicShares(bad_ids));
         }
 
+        let mut missing_shares = Vec::new();
+        for i in public_shares.keys() {
+            if private_shares.get(i).is_none() {
+                missing_shares.push((self.id, *i));
+            }
+        }
+        if !missing_shares.is_empty() {
+            return Err(DkgError::MissingPrivateShares(missing_shares));
+        }
+
         // let's optimize for the case where all shares are good, and test them as a batch
 
         // building a vector of scalars and points from public poly evaluations and expected values takes too much memory
         // instead make an object which implements p256k1 MultiMult trait, using the existing powers of x and shares
-        let mut check_shares = CheckPrivateShares::new(self.id(), &shares, polys.clone());
+        let mut check_shares =
+            CheckPrivateShares::new(self.id(), &private_shares, public_shares.clone());
 
         // if the batch verify fails then check them one by one and find the bad ones
         if Point::multimult_trait(&mut check_shares)? != Point::zero() {
             let mut bad_shares = Vec::new();
-            for (i, s) in shares.iter() {
-                let comm = &polys[i];
-                if s * G != compute::poly(&self.id(), &comm.poly)? {
-                    bad_shares.push(*i);
+            for (i, s) in private_shares.iter() {
+                if let Some(comm) = public_shares.get(i) {
+                    if s * G != compute::poly(&self.id(), &comm.poly)? {
+                        bad_shares.push(*i);
+                    }
+                } else {
+                    warn!("unable to check private share from {}: no corresponding public share, even though we checked for it above", i);
                 }
             }
             return Err(DkgError::BadPrivateShares(bad_shares));
         }
 
-        for (_i, s) in shares.iter() {
-            self.private_key += s;
-        }
+        self.private_key = private_shares.values().sum();
         self.public_key = self.private_key * G;
 
         Ok(())
@@ -418,7 +417,7 @@ impl traits::Aggregator for Aggregator {
 
     /// Initialize the Aggregator polynomial
     fn init(&mut self, comms: &HashMap<u32, PolyCommitment>) -> Result<(), AggregatorError> {
-        let threshold = self.threshold.try_into().unwrap();
+        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() {
@@ -788,7 +787,7 @@ pub mod test_helpers {
 #[cfg(test)]
 mod tests {
     use crate::traits;
-    use crate::traits::test_helpers::run_compute_secrets_not_enough_shares;
+    use crate::traits::test_helpers::run_compute_secrets_missing_private_shares;
     use crate::traits::{Aggregator, Signer};
     use crate::v1;
 
@@ -916,7 +915,7 @@ mod tests {
     #[test]
     /// Run a distributed key generation round with not enough shares
     pub fn run_compute_secrets_missing_shares() {
-        run_compute_secrets_not_enough_shares::<v1::Signer>()
+        run_compute_secrets_missing_private_shares::<v1::Signer>()
     }
 
     #[allow(non_snake_case)]