refactor Lagrange coefficient computation (#436)

* refactor Lagrange coefficient computation

* A line

* Apply suggestions from code review

Co-authored-by: Deirdre Connolly <durumcrustulum@gmail.com>

* address review comments; make compute_lagrange_coefficients() not pub by default

---------

Co-authored-by: Deirdre Connolly <durumcrustulum@gmail.com>

frost-core/src/error.rs

@@ -24,8 +24,14 @@ pub enum Error<C: Ciphersuite> {
     #[error("Malformed identifier is unserializable.")]
     MalformedIdentifier,
     /// This identifier is duplicated.
-    #[error("Duplicated identifier.")]
-    DuplicatedIdentifier,
+    #[error("Duplicated identifiers.")]
+    DuplicatedIdentifiers,
+    /// This identifier does not belong to a participant in the signing process.
+    #[error("Unknown identifier.")]
+    UnknownIdentifier,
+    /// Incorrect number of identifiers.
+    #[error("Incorrect number of identifiers.")]
+    IncorrectNumberOfIdentifiers,
     /// The encoding of a signing key was malformed.
     #[error("Malformed signing key encoding.")]
     MalformedSigningKey,
@@ -125,8 +131,10 @@ where
             | Error::DKGNotSupported
             | Error::FieldError(_)
             | Error::GroupError(_)
-            | Error::DuplicatedIdentifier
+            | Error::DuplicatedIdentifiers
             | Error::InvalidCoefficient
+            | Error::UnknownIdentifier
+            | Error::IncorrectNumberOfIdentifiers
             | Error::IdentifierDerivationNotSupported => None,
         }
     }

frost-core/src/frost.rs

@@ -11,7 +11,7 @@
 //! Sharing, where shares are generated using Shamir Secret Sharing.
 
 use std::{
-    collections::{BTreeMap, HashMap},
+    collections::{BTreeMap, BTreeSet, HashMap},
     fmt::{self, Debug},
     ops::Index,
 };
@@ -148,34 +148,69 @@ where
     }
 }
 
-/// Generates the lagrange coefficient for the i'th participant.
+/// Generates a lagrange coefficient.
+///
+/// The Lagrange polynomial for a set of points (x_j, y_j) for 0 <= j <= k
+/// is ∑_{i=0}^k y_i.ℓ_i(x), where ℓ_i(x) is the Lagrange basis polynomial:
+///
+/// ℓ_i(x) = ∏_{0≤j≤k; j≠i} (x - x_j) / (x_i - x_j).
+///
+/// This computes ℓ_j(x) for the set of points `xs` and for the j corresponding
+/// to the given xj.
+///
+/// If `x` is None, it uses 0 for it (since Identifiers can't be 0)
+#[cfg_attr(feature = "internals", visibility::make(pub))]
+fn compute_lagrange_coefficient<C: Ciphersuite>(
+    x_set: &BTreeSet<Identifier<C>>,
+    x: Option<Identifier<C>>,
+    x_i: Identifier<C>,
+) -> Result<Scalar<C>, Error<C>> {
+    if x_set.is_empty() {
+        return Err(Error::IncorrectNumberOfIdentifiers);
+    }
+    let mut num = <<C::Group as Group>::Field>::one();
+    let mut den = <<C::Group as Group>::Field>::one();
+
+    let mut x_i_found = false;
+
+    for x_j in x_set.iter() {
+        if x_i == *x_j {
+            x_i_found = true;
+            continue;
+        }
+
+        if let Some(x) = x {
+            num *= x - *x_j;
+            den *= x_i - *x_j;
+        } else {
+            // Both signs inverted just to avoid requiring Neg (-*xj)
+            num *= *x_j;
+            den *= *x_j - x_i;
+        }
+    }
+    if !x_i_found {
+        return Err(Error::UnknownIdentifier);
+    }
+
+    Ok(num
+        * <<C::Group as Group>::Field>::invert(&den).map_err(|_| Error::DuplicatedIdentifiers)?)
+}
+
+/// Generates the lagrange coefficient for the i'th participant (for `signer_id`).
 #[cfg_attr(feature = "internals", visibility::make(pub))]
 fn derive_interpolating_value<C: Ciphersuite>(
     signer_id: &Identifier<C>,
     signing_package: &SigningPackage<C>,
 ) -> Result<Scalar<C>, Error<C>> {
-    let zero = <<C::Group as Group>::Field>::zero();
-
-    let mut num = <<C::Group as Group>::Field>::one();
-    let mut den = <<C::Group as Group>::Field>::one();
-
-    for commitment_identifier in signing_package.signing_commitments().keys() {
-        if *commitment_identifier == *signer_id {
-            continue;
-        }
-
-        num *= *commitment_identifier;
-
-        den *= *commitment_identifier - *signer_id;
-    }
-
-    if den == zero {
-        return Err(Error::DuplicatedShares);
-    }
-
-    let lagrange_coeff = num * <<C::Group as Group>::Field>::invert(&den).unwrap();
-
-    Ok(lagrange_coeff)
+    compute_lagrange_coefficient(
+        &signing_package
+            .signing_commitments()
+            .keys()
+            .cloned()
+            .collect(),
+        None,
+        *signer_id,
+    )
 }
 
 /// Generated by the coordinator of the signing operation and distributed to

frost-core/src/frost/keys.rs

@@ -2,7 +2,7 @@
 #![allow(clippy::type_complexity)]
 
 use std::{
-    collections::{HashMap, HashSet},
+    collections::{BTreeSet, HashMap, HashSet},
     convert::TryFrom,
     default::Default,
     fmt::{self, Debug},
@@ -23,6 +23,8 @@ use crate::{
 #[cfg(feature = "serde")]
 use crate::{ElementSerialization, ScalarSerialization};
 
+use super::compute_lagrange_coefficient;
+
 pub mod dkg;
 pub mod repairable;
 
@@ -716,7 +718,7 @@ pub(crate) fn generate_secret_shares<C: Ciphersuite>(
 
     let identifiers_set: HashSet<_> = identifiers.iter().collect();
     if identifiers_set.len() != identifiers.len() {
-        return Err(Error::DuplicatedIdentifier);
+        return Err(Error::DuplicatedIdentifiers);
     }
 
     for id in identifiers {
@@ -754,39 +756,20 @@ pub fn reconstruct<C: Ciphersuite>(
 
     let mut secret = <<C::Group as Group>::Field>::zero();
 
-    // Compute the Lagrange coefficients
-    for (i_idx, i, secret_share) in secret_shares
+    let identifiers: BTreeSet<_> = secret_shares
         .iter()
-        .enumerate()
-        .map(|(idx, s)| (idx, s.identifier, s))
-    {
-        let mut num = <<C::Group as Group>::Field>::one();
-        let mut den = <<C::Group as Group>::Field>::one();
+        .map(|s| s.identifier())
+        .cloned()
+        .collect();
 
-        for (j_idx, j) in secret_shares
-            .iter()
-            .enumerate()
-            .map(|(idx, s)| (idx, s.identifier))
-        {
-            if j_idx == i_idx {
-                continue;
-            }
+    if identifiers.len() != secret_shares.len() {
+        return Err(Error::DuplicatedIdentifiers);
+    }
 
-            // numerator *= j
-            num *= j;
-
-            // denominator *= j - i
-            den *= j - i;
-        }
-
-        // If at this step, the denominator is zero in the scalar field, there must be a duplicate
-        // secret share.
-        if den == <<C::Group as Group>::Field>::zero() {
-            return Err(Error::DuplicatedShares);
-        }
-
-        // Save numerator * 1/denominator in the scalar field
-        let lagrange_coefficient = num * <<C::Group as Group>::Field>::invert(&den).unwrap();
+    // Compute the Lagrange coefficients
+    for secret_share in secret_shares.iter() {
+        let lagrange_coefficient =
+            compute_lagrange_coefficient(&identifiers, None, secret_share.identifier)?;
 
         // Compute y = f(0) via polynomial interpolation of these t-of-n solutions ('points) of f
         secret = secret + (lagrange_coefficient * secret_share.value.0);

frost-core/src/frost/keys/repairable.rs

@@ -4,9 +4,12 @@
 //! The RTS is used to help a signer (participant) repair their lost share. This is achieved
 //! using a subset of the other signers know here as `helpers`.
 
-use std::collections::HashMap;
+use std::collections::{BTreeSet, HashMap};
 
-use crate::{frost::Identifier, Ciphersuite, CryptoRng, Field, Group, RngCore, Scalar};
+use crate::{
+    frost::{compute_lagrange_coefficient, Identifier},
+    Ciphersuite, CryptoRng, Error, Field, Group, RngCore, Scalar,
+};
 
 use super::{generate_coefficients, SecretShare, SigningShare, VerifiableSecretSharingCommitment};
 
@@ -22,10 +25,18 @@ pub fn repair_share_step_1<C: Ciphersuite, R: RngCore + CryptoRng>(
     share_i: &SecretShare<C>,
     rng: &mut R,
     participant: Identifier<C>,
-) -> HashMap<Identifier<C>, Scalar<C>> {
+) -> Result<HashMap<Identifier<C>, Scalar<C>>, Error<C>> {
+    if helpers.is_empty() {
+        return Err(Error::IncorrectNumberOfIdentifiers);
+    }
+    let xset: BTreeSet<_> = helpers.iter().cloned().collect();
+    if xset.len() != helpers.len() {
+        return Err(Error::DuplicatedIdentifiers);
+    }
+
     let rand_val: Vec<Scalar<C>> = generate_coefficients::<C, R>(helpers.len() - 1, rng);
 
-    compute_last_random_value(helpers, share_i, &rand_val, participant)
+    compute_last_random_value(&xset, share_i, &rand_val, participant)
 }
 
 /// Compute the last delta value given the (generated uniformly at random) remaining ones
@@ -33,13 +44,13 @@ pub fn repair_share_step_1<C: Ciphersuite, R: RngCore + CryptoRng>(
 ///
 /// Returns a HashMap mapping which value should be sent to which participant.
 fn compute_last_random_value<C: Ciphersuite>(
-    helpers: &[Identifier<C>],
+    helpers: &BTreeSet<Identifier<C>>,
     share_i: &SecretShare<C>,
     random_values: &Vec<Scalar<C>>,
     participant: Identifier<C>,
-) -> HashMap<Identifier<C>, Scalar<C>> {
+) -> Result<HashMap<Identifier<C>, Scalar<C>>, Error<C>> {
     // Calculate Lagrange Coefficient for helper_i
-    let zeta_i = compute_lagrange_coefficient(helpers, participant, share_i.identifier);
+    let zeta_i = compute_lagrange_coefficient(helpers, Some(participant), share_i.identifier)?;
 
     let lhs = zeta_i * share_i.value.0;
 
@@ -55,32 +66,9 @@ fn compute_last_random_value<C: Ciphersuite>(
         sum_i_deltas = sum_i_deltas + *v;
     }
 
-    out.insert(helpers[helpers.len() - 1], lhs - sum_i_deltas);
+    out.insert(*helpers.last().unwrap(), lhs - sum_i_deltas);
 
-    out
-}
-
-/// Compute the i-th Lagrange coefficient evaluated at `participant`, i.e.
-/// computes `zeta_i` such that f(participant) is the sum of all `zeta_i * share_i`
-/// for each `i` in `helpers`.
-pub fn compute_lagrange_coefficient<C: Ciphersuite>(
-    helpers: &[Identifier<C>],
-    participant: Identifier<C>,
-    helper_i: Identifier<C>,
-) -> Scalar<C> {
-    let mut num = <<C::Group as Group>::Field>::one();
-    let mut den = <<C::Group as Group>::Field>::one();
-
-    for j in helpers.iter() {
-        if helper_i == *j {
-            continue;
-        }
-
-        num *= participant - *j;
-        den *= helper_i - *j;
-    }
-
-    num * <<C::Group as Group>::Field>::invert(&den).unwrap()
+    Ok(out)
 }
 
 /// Communication round

frost-core/src/tests/ciphersuite_generic.rs

@@ -55,7 +55,7 @@ pub fn check_share_generation<C: Ciphersuite, R: RngCore + CryptoRng>(mut rng: R
 
     assert_eq!(
         frost::keys::reconstruct::<C>(&secret_shares).unwrap_err(),
-        Error::DuplicatedShares
+        Error::DuplicatedIdentifiers
     );
 }
 
@@ -372,7 +372,7 @@ pub fn check_sign_with_dealer_and_identifiers<C: Ciphersuite, R: RngCore + Crypt
         &mut rng,
     )
     .unwrap_err();
-    assert_eq!(err, Error::DuplicatedIdentifier);
+    assert_eq!(err, Error::DuplicatedIdentifiers);
 
     // Check correct case
 

frost-core/src/tests/repairable.rs

@@ -8,12 +8,9 @@ use serde_json::Value;
 
 use crate::{
     frost::{
-        self,
+        self, compute_lagrange_coefficient,
         keys::{
-            repairable::{
-                compute_lagrange_coefficient, repair_share_step_1, repair_share_step_2,
-                repair_share_step_3,
-            },
+            repairable::{repair_share_step_1, repair_share_step_2, repair_share_step_3},
             PublicKeyPackage, SecretShare, SigningShare,
         },
         Identifier,
@@ -58,9 +55,12 @@ pub fn check_rts<C: Ciphersuite, R: RngCore + CryptoRng>(mut rng: R) {
 
     // Each helper generates random values for each helper
 
-    let helper_1_deltas = repair_share_step_1(&helpers, helper_1, &mut rng, participant.identifier);
-    let helper_4_deltas = repair_share_step_1(&helpers, helper_4, &mut rng, participant.identifier);
-    let helper_5_deltas = repair_share_step_1(&helpers, helper_5, &mut rng, participant.identifier);
+    let helper_1_deltas =
+        repair_share_step_1(&helpers, helper_1, &mut rng, participant.identifier).unwrap();
+    let helper_4_deltas =
+        repair_share_step_1(&helpers, helper_4, &mut rng, participant.identifier).unwrap();
+    let helper_5_deltas =
+        repair_share_step_1(&helpers, helper_5, &mut rng, participant.identifier).unwrap();
 
     // Each helper calculates their sigma from the random values received from the other helpers
 
@@ -130,10 +130,14 @@ pub fn check_repair_share_step_1<C: Ciphersuite, R: RngCore + CryptoRng>(mut rng
     ];
 
     // Generate deltas for helper 4
-    let deltas = repair_share_step_1(&helpers, helper_4, &mut rng, participant.identifier);
+    let deltas = repair_share_step_1(&helpers, helper_4, &mut rng, participant.identifier).unwrap();
 
-    let lagrange_coefficient =
-        compute_lagrange_coefficient(&helpers, participant.identifier, helpers[1]);
+    let lagrange_coefficient = compute_lagrange_coefficient(
+        &helpers.iter().cloned().collect(),
+        Some(participant.identifier),
+        helpers[1],
+    )
+    .unwrap();
 
     let mut rhs = <<C::Group as Group>::Field>::zero();
     for (_k, v) in deltas {

frost-ed25519/src/keys/repairable.rs

@@ -9,8 +9,8 @@ use std::collections::HashMap;
 // This is imported separately to make `gencode` work.
 // (if it were below, the position of the import would vary between ciphersuites
 //  after `cargo fmt`)
-use crate::Ed25519Sha512;
 use crate::{frost, Ciphersuite, CryptoRng, Identifier, RngCore, Scalar};
+use crate::{Ed25519Sha512, Error};
 
 use super::{SecretShare, VerifiableSecretSharingCommitment};
 
@@ -26,7 +26,7 @@ pub fn repair_share_step_1<C: Ciphersuite, R: RngCore + CryptoRng>(
     share_i: &SecretShare,
     rng: &mut R,
     participant: Identifier,
-) -> HashMap<Identifier, Scalar> {
+) -> Result<HashMap<Identifier, Scalar>, Error> {
     frost::keys::repairable::repair_share_step_1(helpers, share_i, rng, participant)
 }
 

frost-ed448/src/keys/repairable.rs

@@ -9,8 +9,8 @@ use std::collections::HashMap;
 // This is imported separately to make `gencode` work.
 // (if it were below, the position of the import would vary between ciphersuites
 //  after `cargo fmt`)
-use crate::Ed448Shake256;
 use crate::{frost, Ciphersuite, CryptoRng, Identifier, RngCore, Scalar};
+use crate::{Ed448Shake256, Error};
 
 use super::{SecretShare, VerifiableSecretSharingCommitment};
 
@@ -26,7 +26,7 @@ pub fn repair_share_step_1<C: Ciphersuite, R: RngCore + CryptoRng>(
     share_i: &SecretShare,
     rng: &mut R,
     participant: Identifier,
-) -> HashMap<Identifier, Scalar> {
+) -> Result<HashMap<Identifier, Scalar>, Error> {
     frost::keys::repairable::repair_share_step_1(helpers, share_i, rng, participant)
 }
 

frost-p256/src/keys/repairable.rs

@@ -9,8 +9,8 @@ use std::collections::HashMap;
 // This is imported separately to make `gencode` work.
 // (if it were below, the position of the import would vary between ciphersuites
 //  after `cargo fmt`)
-use crate::P256Sha256;
 use crate::{frost, Ciphersuite, CryptoRng, Identifier, RngCore, Scalar};
+use crate::{Error, P256Sha256};
 
 use super::{SecretShare, VerifiableSecretSharingCommitment};
 
@@ -26,7 +26,7 @@ pub fn repair_share_step_1<C: Ciphersuite, R: RngCore + CryptoRng>(
     share_i: &SecretShare,
     rng: &mut R,
     participant: Identifier,
-) -> HashMap<Identifier, Scalar> {
+) -> Result<HashMap<Identifier, Scalar>, Error> {
     frost::keys::repairable::repair_share_step_1(helpers, share_i, rng, participant)
 }
 

frost-ristretto255/src/keys/repairable.rs

@@ -9,8 +9,8 @@ use std::collections::HashMap;
 // This is imported separately to make `gencode` work.
 // (if it were below, the position of the import would vary between ciphersuites
 //  after `cargo fmt`)
-use crate::Ristretto255Sha512;
 use crate::{frost, Ciphersuite, CryptoRng, Identifier, RngCore, Scalar};
+use crate::{Error, Ristretto255Sha512};
 
 use super::{SecretShare, VerifiableSecretSharingCommitment};
 
@@ -26,7 +26,7 @@ pub fn repair_share_step_1<C: Ciphersuite, R: RngCore + CryptoRng>(
     share_i: &SecretShare,
     rng: &mut R,
     participant: Identifier,
-) -> HashMap<Identifier, Scalar> {
+) -> Result<HashMap<Identifier, Scalar>, Error> {
     frost::keys::repairable::repair_share_step_1(helpers, share_i, rng, participant)
 }
 

frost-secp256k1/src/keys/repairable.rs

@@ -9,8 +9,8 @@ use std::collections::HashMap;
 // This is imported separately to make `gencode` work.
 // (if it were below, the position of the import would vary between ciphersuites
 //  after `cargo fmt`)
-use crate::Secp256K1Sha256;
 use crate::{frost, Ciphersuite, CryptoRng, Identifier, RngCore, Scalar};
+use crate::{Error, Secp256K1Sha256};
 
 use super::{SecretShare, VerifiableSecretSharingCommitment};
 
@@ -26,7 +26,7 @@ pub fn repair_share_step_1<C: Ciphersuite, R: RngCore + CryptoRng>(
     share_i: &SecretShare,
     rng: &mut R,
     participant: Identifier,
-) -> HashMap<Identifier, Scalar> {
+) -> Result<HashMap<Identifier, Scalar>, Error> {
     frost::keys::repairable::repair_share_step_1(helpers, share_i, rng, participant)
 }