Remove requirement that bitcoin key shares need to be normalized

Signed-off-by: Denis Varlakov <[email protected]>

.gitignore

@@ -2,3 +2,4 @@
 /wasm/no_std/target
 
 /.helix
+.cspell.config.yaml

givre/src/ciphersuite.rs

@@ -7,8 +7,6 @@
 //! * [Ed25519], requires `ciphersuite-ed25519` feature
 //! * [Bitcoin], requires `ciphersuite-bitcoin` feature
 
-use alloc::vec::Vec;
-
 use generic_ec::{
     errors::{InvalidPoint, InvalidScalar},
     Curve, NonZero, Point, Scalar, SecretScalar,
@@ -47,14 +45,14 @@ pub trait Ciphersuite: Sized + Clone + Copy + core::fmt::Debug {
 
     /// Preferred [multiscalar multiplication](generic_ec::multiscalar) algorithm
     ///
-    /// Multiscalar multiplication optimization greatly improves performace of FROST protocol.
+    /// Multiscalar multiplication optimization greatly improves performance of FROST protocol.
     /// By default, we set it to [`generic_ec::multiscalar::Default`] which uses the fastest
     /// algorithm available in [`generic_ec`] crate.
     type MultiscalarMul: generic_ec::multiscalar::MultiscalarMul<Self::Curve>;
 
     /// `H1` hash function as defined in the draft
     ///
-    /// Accepts a list of bytestring, that'll be contatenated before hashing.
+    /// Accepts a list of bytestring, that'll be concatenated before hashing.
     /// Returns `H1(data[0] || data[1] || ... || data[data.len() - 1])`.
     fn h1(msg: &[&[u8]]) -> Scalar<Self::Curve>;
     /// Computes the challenge according to Schnorr scheme
@@ -67,18 +65,18 @@ pub trait Ciphersuite: Sized + Clone + Copy + core::fmt::Debug {
     ) -> Scalar<Self::Curve>;
     /// `H3` hash function as defined in the draft
     ///
-    /// Accepts a list of bytestring, that'll be contatenated before hashing.
+    /// Accepts a list of bytestring, that'll be concatenated before hashing.
     /// Returns `H3(data[0] || data[1] || ... || data[data.len() - 1])`.
     fn h3(msg: &[&[u8]]) -> Scalar<Self::Curve>;
 
     /// `H4` hash function as defined in the draft
     ///
-    /// Accepts a list of bytestring, that'll be contatenated before hashing.
+    /// Accepts a list of bytestring, that'll be concatenated before hashing.
     /// Returns `H4(data[0] || data[1] || ... || data[data.len() - 1])`.
     fn h4() -> Self::Digest;
     /// `H5` hash function as defined in the draft
     ///
-    /// Accepts a list of bytestring, that'll be contatenated before hashing.
+    /// Accepts a list of bytestring, that'll be concatenated before hashing.
     /// Returns `H5(data[0] || data[1] || ... || data[data.len() - 1])`.
     fn h5() -> Self::Digest;
 
@@ -115,9 +113,9 @@ pub trait Ciphersuite: Sized + Clone + Copy + core::fmt::Debug {
     ///
     /// Our implementation of FROST requires that if point $X$ isn't normalized, then $-X$ is normalized. Zero point
     /// (aka point at infinity) is always normalized. Note that certain parts of the protocol may enforce this property
-    /// via debug assertations.
+    /// via debug assertions.
     ///
-    /// The protocol always outputs sigantures with normalized R-component. We also require that public key is
+    /// The protocol always outputs signatures with normalized R-component. We also require that public key is
     /// normalized. If it isn't, signing fails. You can use [`normalize_key_share`] function to normalize any key.
     ///
     /// If Schnorr scheme doesn't have a notion of normalized points, this function should always return `true`.
@@ -176,7 +174,7 @@ pub trait AdditionalEntropy<C: Ciphersuite> {
     where
         Self: 'b;
 
-    /// Returns bytes representation of the entropy encoded in complience with [`C`](Ciphersuite)
+    /// Returns bytes representation of the entropy encoded in compliance with [`C`](Ciphersuite)
     fn to_bytes(&self) -> Self::Bytes<'_>;
 }
 
@@ -231,7 +229,7 @@ impl<C: Ciphersuite, T: AdditionalEntropy<C>> AdditionalEntropy<C> for &T {
 pub struct NormalizedPoint<C, P>(P, core::marker::PhantomData<C>);
 
 impl<C: Ciphersuite, P: AsRef<Point<C::Curve>>> NormalizedPoint<C, P> {
-    /// Serialzies the normalized point in a space-efficient manner
+    /// Serializes the normalized point in a space-efficient manner
     ///
     /// Alias to [`Ciphersuite::serialize_normalized_point`]
     pub fn to_bytes(&self) -> C::NormalizedPointBytes {
@@ -310,49 +308,6 @@ where
     }
 }
 
-/// Normalizes the key share
-///
-/// Some Schnorr signing schemes require public key to be [normalized](Ciphersuite::is_normalized).
-/// Generally, DKG protocols output arbitrary public keys which are not necessarily normalized. If
-/// you want to use this key in the Schnorr scheme that requires public keys to be normalized, each
-/// key share needs to be normalized using this function.
-///
-/// ## Error
-/// Returns an error if output key share is inconsistent. That should never happen in practice. If
-/// it happened, it's probably a bug.
-pub fn normalize_key_share<C: Ciphersuite>(
-    key_share: crate::key_share::KeyShare<C::Curve>,
-) -> Result<crate::key_share::KeyShare<C::Curve>, crate::key_share::InvalidKeyShare> {
-    use crate::key_share::Validate;
-
-    let Err(new_pk) = NormalizedPoint::<C, _>::try_normalize(key_share.shared_public_key) else {
-        // Public key is already normalized
-        return Ok(key_share);
-    };
-    let key_share = key_share.into_inner();
-
-    // PK is negated. Now we need to negate the key share, and each public key share
-    let new_sk = -key_share.x;
-    let public_shares = key_share
-        .key_info
-        .public_shares
-        .iter()
-        .map(|p| -p)
-        .collect::<Vec<_>>();
-
-    crate::key_share::DirtyKeyShare {
-        i: key_share.i,
-        key_info: crate::key_share::DirtyKeyInfo {
-            shared_public_key: *new_pk,
-            public_shares,
-            ..key_share.key_info
-        },
-        x: new_sk,
-    }
-    .validate()
-    .map_err(|e| e.into_error())
-}
-
 /// Checks whether `key_share` is normalized
 ///
 /// `key_share` must be normalized, otherwise the signing will return error. See [`Ciphersuite::is_normalized`]

givre/src/ciphersuite/bitcoin.rs

@@ -3,7 +3,7 @@ use sha2::Digest;
 
 use super::{Ciphersuite, Secp256k1};
 
-/// FROST ciphersuite that outputs [BIP-340] compliant sigantures
+/// FROST ciphersuite that outputs [BIP-340] compliant signatures
 ///
 /// # Normalized public keys
 /// BIP-340 requires that public keys are normalized, meaning that they must have

givre/src/lib.rs

@@ -6,7 +6,7 @@
 //! This crate provides:
 //! * Distributed Key Generation (DKG) \
 //!   FROST does not define DKG protocol to be used. We simply re-export DKG based on [CGGMP21] implementation
-//!   when `cggmp21-keygen` feature is enabled, which is a fairly reasonalbe choice as it's proven to be UC-secure.
+//!   when `cggmp21-keygen` feature is enabled, which is a fairly reasonable choice as it's proven to be UC-secure.
 //!   Alternatively, you can use any other UC-secure DKG protocol.
 //! * FROST Signing \
 //!   We provide API for both manual signing execution (for better flexibility and efficiency) and interactive protocol
@@ -23,7 +23,7 @@
 //!
 //! ## Distributed Key Generation (DKG)
 //! First of all, you need to generate a key. For that purpose, you can use any secure
-//! (preferrably, UC-secure) DKG protocol. FROST IETF Draft does not define any DKG
+//! (preferably, UC-secure) DKG protocol. FROST IETF Draft does not define any DKG
 //! protocol or requirements it needs to meet, so the choice is up to you. This library
 //! re-exports CGGMP21 DKG from [`cggmp21-keygen`] crate when `cggmp21-keygen` feature
 //! is enabled which is proven to be UC-secure and should be a reasonable default.
@@ -39,7 +39,7 @@
 //!
 //! where:
 //! * `Msg` is a protocol message (e.g., [`keygen::msg::threshold::Msg`])
-//! * [`round_based::Incoming`] and [`round_based::Outgoing`] wrap `Msg` and provide additional data (e.g., sender/recepient)
+//! * [`round_based::Incoming`] and [`round_based::Outgoing`] wrap `Msg` and provide additional data (e.g., sender/recipient)
 //! * [`futures::Stream`] and [`futures::Sink`] are well-known async primitives.
 //!
 //! [`futures::Stream`]: https://docs.rs/futures/latest/futures/stream/trait.Stream.html
@@ -262,7 +262,7 @@ pub type SignerIndex = u16;
 ///
 /// This can be less efficient than doing signing manually, when you can commit nonces before
 /// a message to be signed is known, but more secure, as using this function ensures that
-/// protocol isn't misused (e.g. that nonce is never resused).
+/// protocol isn't misused (e.g. that nonce is never reused).
 ///
 /// ## Inputs
 /// * Signer index in *signing protocol* $0 \le i < \\text{min\\_signers}$

givre/src/signing/aggregate.rs

@@ -100,11 +100,23 @@ pub fn aggregate<C: Ciphersuite>(
     msg: &[u8],
 ) -> Result<Signature<C>, AggregateError> {
     // --- Retrieve and Validate Data
+    let crate::key_share::DirtyKeyInfo {
+        shared_public_key: pk,
+        vss_setup,
+        ..
+    } = &**key_info;
+    // Make sure we never use (potentially non-normalized) `key_info` anywhere
+    // below by shadowing the variable.
+    #[allow(unused_variables)]
+    let key_info = ();
+
+    // Normalize the public key
+    let pk = C::normalize_point(*pk);
+
     let mut comm_list = signers
         .iter()
         .map(|(j, comm, _sig_share)| {
-            key_info
-                .share_preimage(*j)
+            utils::share_preimage(vss_setup, *j)
                 .map(|id| (id, *comm))
                 .ok_or(Reason::UnknownSigner(*j))
         })
@@ -122,8 +134,7 @@ pub fn aggregate<C: Ciphersuite>(
     }
 
     // --- The Aggregation
-    let binding_factor_list =
-        utils::compute_binding_factors::<C>(key_info.shared_public_key, &comm_list, msg);
+    let binding_factor_list = utils::compute_binding_factors::<C>(*pk, &comm_list, msg);
     let group_commitment = utils::compute_group_commitment::<C>(&comm_list, &binding_factor_list);
     let z = signers
         .iter()
@@ -134,8 +145,7 @@ pub fn aggregate<C: Ciphersuite>(
         r: C::normalize_point(group_commitment),
         z,
     };
-    sig.verify(&C::normalize_point(key_info.shared_public_key), msg)
-        .map_err(|_| Reason::InvalidSig)?;
+    sig.verify(&pk, msg).map_err(|_| Reason::InvalidSig)?;
 
     Ok(sig)
 }

givre/src/signing/round2.rs

@@ -6,10 +6,10 @@
 //!
 //! [Section 5.2]: https://www.ietf.org/archive/id/draft-irtf-cfrg-frost-15.html#name-round-two-signature-share-g
 
-use alloc::vec::Vec;
+use alloc::{borrow::Cow, vec::Vec};
 use core::{fmt, iter};
 
-use generic_ec::{Curve, NonZero, Scalar};
+use generic_ec::{Curve, NonZero, Scalar, SecretScalar};
 
 use crate::{
     ciphersuite::{Ciphersuite, NormalizedPoint},
@@ -49,27 +49,54 @@ pub fn sign<C: Ciphersuite>(
     signers: &[(SignerIndex, PublicCommitments<C::Curve>)],
 ) -> Result<SigShare<C::Curve>, SigningError> {
     // --- Retrieve and Validate Data
-    let pk = NormalizedPoint::<C, _>::try_normalize(key_share.shared_public_key)
-        .map_err(|_| Reason::ShareNotNormalized)?;
-    if signers.len() < usize::from(key_share.min_signers()) {
+    let t = key_share.min_signers();
+    let crate::key_share::DirtyKeyShare {
+        i,
+        key_info:
+            crate::key_share::DirtyKeyInfo {
+                shared_public_key: pk,
+                vss_setup,
+                ..
+            },
+        x,
+    } = &**key_share;
+    // Make sure we never use (potentially non-normalized) `key_share` anywhere
+    // below by shadowing the variable.
+    #[allow(unused_variables)]
+    let key_share = ();
+
+    // Normalize the key share
+    let (x, pk): (Cow<NonZero<SecretScalar<C::Curve>>>, NormalizedPoint<C, _>) =
+        match NormalizedPoint::<C, _>::try_normalize(*pk) {
+            Ok(pk) => {
+                // public key is already normalized, there's nothing to do
+                (Cow::Borrowed(&x), pk)
+            }
+
+            Err(neg_pk) => {
+                // public key was not normalized. we need to negate the key share.
+                // note that we do not negate `public_shares` as they are not used
+                // anywhere in this function.
+                (Cow::Owned(-x), neg_pk)
+            }
+        };
+
+    if signers.len() < usize::from(t) {
         return Err(Reason::TooFewSigners {
-            min_signers: key_share.min_signers(),
+            min_signers: t,
             n: signers.len(),
         }
         .into());
     }
-    let signer_id = key_share
-        .share_preimage(key_share.i)
-        .ok_or(Bug::RetrieveOwnShareId)?;
+    let signer_id = utils::share_preimage(vss_setup, *i).ok_or(Bug::RetrieveOwnShareId)?;
     let mut comm_list = signers
         .iter()
         .map(|(j, comm)| {
-            if key_share.i == *j && nonce.public_commitments() != *comm {
+            if i == j && nonce.public_commitments() != *comm {
                 // Commitments don't match provided nonces - invalid inputs
                 Err(Reason::NoncesDontMatchComm)
             } else {
-                key_share
-                    .share_preimage(*j)
+                utils::share_preimage(vss_setup, *j)
                     .map(|id| (id, *comm))
                     .ok_or(Reason::UnknownSigner(*j))
             }
@@ -102,8 +129,7 @@ pub fn sign<C: Ciphersuite>(
     };
 
     // --- The Signing
-    let binding_factor_list =
-        utils::compute_binding_factors::<C>(key_share.shared_public_key, &comm_list, msg);
+    let binding_factor_list = utils::compute_binding_factors::<C>(*pk, &comm_list, msg);
     let binding_factor = binding_factor_list.get(i).ok_or(Bug::OwnBindingFactor)?.1;
     debug_assert_eq!(binding_factor_list[i].0, signer_id);
 
@@ -123,7 +149,7 @@ pub fn sign<C: Ciphersuite>(
     };
 
     let signers_list = comm_list.iter().map(|(i, _)| *i).collect::<Vec<_>>();
-    let lambda_i = if key_share.vss_setup.is_some() {
+    let lambda_i = if vss_setup.is_some() {
         derive_interpolating_value(&signers_list, &signer_id)
             .ok_or(Reason::DeriveInterpolationValue)?
     } else {
@@ -132,9 +158,7 @@ pub fn sign<C: Ciphersuite>(
 
     let challenge = C::compute_challenge(&group_commitment, &pk, msg);
 
-    Ok(SigShare(
-        nonce_share + (lambda_i * &key_share.x * challenge),
-    ))
+    Ok(SigShare(nonce_share + (lambda_i * &*x * challenge)))
 }
 
 /// Computes an interpolation value as described in [Section 4.2]
@@ -189,7 +213,6 @@ pub struct SigningError(Reason);
 
 #[derive(Debug)]
 enum Reason {
-    ShareNotNormalized,
     TooFewSigners { min_signers: u16, n: usize },
     UnknownSigner(SignerIndex),
     SameSignerTwice,
@@ -208,7 +231,6 @@ enum Bug {
 impl fmt::Display for SigningError {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self.0 {
-            Reason::ShareNotNormalized => f.write_str("key share is not normalized, see the docs"),
             Reason::TooFewSigners { min_signers, n } => write!(
                 f,
                 "signers list contains {n} singners, although at \
@@ -244,8 +266,7 @@ impl fmt::Display for Bug {
 impl std::error::Error for SigningError {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
         match &self.0 {
-            Reason::ShareNotNormalized
-            | Reason::TooFewSigners { .. }
+            Reason::TooFewSigners { .. }
             | Reason::UnknownSigner(_)
             | Reason::NoncesDontMatchComm
             | Reason::DeriveInterpolationValue

givre/src/signing/utils.rs

@@ -109,3 +109,22 @@ where
 {
     slice.windows(2).all(|win| f(&win[0]) <= f(&win[1]))
 }
+
+/// Returns share preimage associated with j-th signer
+///
+/// * For additive shares, share preimage is defined as `j+1`
+/// * For VSS-shares, share preimage is scalar $I_j$ such that $x_j = F(I_j)$ where
+///   $F(x)$ is polynomial co-shared by the signers and $x_j$ is secret share of j-th
+///   signer
+pub fn share_preimage<E: generic_ec::Curve>(
+    vss_setup: &Option<crate::key_share::VssSetup<E>>,
+    j: u16,
+) -> Option<NonZero<Scalar<E>>> {
+    match vss_setup {
+        Some(v) => v.I.get(usize::from(j)).copied(),
+        None => Some(
+            #[allow(clippy::expect_used)]
+            NonZero::from_scalar(Scalar::from(j + 1)).expect("j+1 is guaranteed to be non-zero"),
+        ),
+    }
+}