[❄] PairedSecretShare construction safety

Pair the secret shares using the shared key so you know it's correct.

Also cleaned up the accessing the public key in order to do the implementation.

..and some other misc changes 🤗

schnorr_fun/src/frost/mod.rs

@@ -452,7 +452,7 @@ impl<H: Digest<OutputSize = U32> + Clone, NG: NonceGen> Frost<H, NG> {
         session_id: &[u8],
     ) -> R {
         let sid_len = (session_id.len() as u64).to_be_bytes();
-        let pk_bytes = paired_secret_share.shared_key().to_xonly_bytes();
+        let pk_bytes = paired_secret_share.public_key().to_xonly_bytes();
 
         let rng: R = derive_nonce_rng!(
             nonce_gen => self.nonce_gen(),
@@ -620,6 +620,7 @@ impl<H: Digest<OutputSize = U32> + Clone, NG> Frost<H, NG> {
                 .map(|coef| coef.normalize())
                 .collect(),
         )
+        .non_zero()
         .ok_or(NewKeyGenError::ZeroFrostKey)?;
 
         Ok(KeyGen {
@@ -698,7 +699,8 @@ impl<H: Digest<OutputSize = U32> + Clone, NG> Frost<H, NG> {
             share: total_secret_share,
         };
 
-        let secret_share_with_image = PairedSecretShare::new(secret_share, keygen.frost_poly.key());
+        let secret_share_with_image =
+            PairedSecretShare::new(secret_share, keygen.frost_poly.public_key());
 
         Ok((secret_share_with_image, keygen.frost_poly))
     }
@@ -757,12 +759,12 @@ impl<H: Digest<OutputSize = U32> + Clone, NG> Frost<H, NG> {
 
         let agg_binonce = binonce::Nonce::aggregate(nonces.values().cloned());
 
-        let binding_coeff = self.binding_coefficient(shared_key.key(), agg_binonce, message);
+        let binding_coeff = self.binding_coefficient(shared_key.public_key(), agg_binonce, message);
         let (final_nonce, binonce_needs_negation) = agg_binonce.bind(binding_coeff);
 
         let challenge = self
             .schnorr
-            .challenge(&final_nonce, &shared_key.key(), message);
+            .challenge(&final_nonce, &shared_key.public_key(), message);
 
         for nonce in nonces.values_mut() {
             nonce.conditional_negate(binonce_needs_negation);
@@ -810,7 +812,7 @@ impl<H: Digest<OutputSize = U32> + Clone, NG> Frost<H, NG> {
         secret_share: &PairedSecretShare<EvenY>,
         secret_nonce: NonceKeyPair,
     ) -> Scalar<Public, Zero> {
-        if session.public_key != secret_share.shared_key() {
+        if session.public_key != secret_share.public_key() {
             panic!("the share's shared key is not the same as the shared key of the session");
         }
         let secret_share = secret_share.secret_share();

schnorr_fun/src/frost/share.rs

@@ -20,8 +20,7 @@ use secp256kfun::{poly, prelude::*};
 /// shares that way. Share identification can help for keeping track of them and distinguishing shares
 /// when there are only a small number of them.
 ///
-/// The backup format is enabled with the `share_backup` feature and accessed with the `FromStr`
-/// and `Display`.
+/// The backup format is enabled with the `share_backup` feature and accessed with the feature enabled methods.
 ///
 /// ### Index in human readable part
 ///
@@ -132,9 +131,9 @@ secp256kfun::impl_display_debug_serialize! {
 ///
 /// This is useful so you can keep track of tweaks to the secret value and tweaks to the shared key
 /// in tandem.
-pub struct PairedSecretShare<T: Normalized, Z = NonZero> {
+pub struct PairedSecretShare<T: PointType, Z = NonZero> {
     secret_share: SecretShare,
-    shared_key: Point<T, Public, Z>,
+    public_key: Point<T, Public, Z>,
 }
 
 impl<T: Normalized, Z: ZeroChoice> PairedSecretShare<T, Z> {
@@ -148,9 +147,9 @@ impl<T: Normalized, Z: ZeroChoice> PairedSecretShare<T, Z> {
         self.secret_share.share
     }
 
-    /// The shared key this secret is for
-    pub fn shared_key(&self) -> Point<T, Public, Z> {
-        self.shared_key
+    /// The public key that this secert share is a part of
+    pub fn public_key(&self) -> Point<T, Public, Z> {
+        self.public_key
     }
 
     /// The inner un-paired secret share.
@@ -162,12 +161,16 @@ impl<T: Normalized, Z: ZeroChoice> PairedSecretShare<T, Z> {
     }
 }
 
-impl<Z: ZeroChoice, T: Normalized> PairedSecretShare<T, Z> {
-    /// Pair a secret share to a shared key
-    pub fn new(secret_share: SecretShare, shared_key: Point<T, Public, Z>) -> Self {
+impl<Z: ZeroChoice, T: PointType> PairedSecretShare<T, Z> {
+    /// Pair a secret share to a shared key.
+    ///
+    /// Users are meant to use [`pair_secret_share`] to create this
+    ///
+    /// [`pair_secret_share`]: SharedKey::pair_secret_share
+    pub(crate) fn new(secret_share: SecretShare, public_key: Point<T, Public, Z>) -> Self {
         Self {
             secret_share,
-            shared_key,
+            public_key,
         }
     }
 
@@ -195,12 +198,12 @@ impl<Z: ZeroChoice, T: Normalized> PairedSecretShare<T, Z> {
     ) -> PairedSecretShare<Normal, Zero> {
         let PairedSecretShare {
             mut secret_share,
-            shared_key,
+            public_key: shared_key,
         } = self;
         let shared_key = g!(shared_key + tweak * G).normalize();
         secret_share.share = s!(secret_share.share + tweak);
         PairedSecretShare {
-            shared_key,
+            public_key: shared_key,
             secret_share,
         }
     }
@@ -209,15 +212,15 @@ impl<Z: ZeroChoice, T: Normalized> PairedSecretShare<T, Z> {
     #[must_use]
     pub fn homomorphic_mul(self, tweak: Scalar<impl Secrecy>) -> PairedSecretShare<Normal, Z> {
         let PairedSecretShare {
-            shared_key,
+            public_key: shared_key,
             mut secret_share,
         } = self;
 
         let shared_key = g!(tweak * shared_key).normalize();
         secret_share.share = s!(tweak * self.secret_share.share);
         PairedSecretShare {
             secret_share,
-            shared_key,
+            public_key: shared_key,
         }
     }
 
@@ -230,26 +233,26 @@ impl<Z: ZeroChoice, T: Normalized> PairedSecretShare<T, Z> {
     pub fn non_zero(self) -> Option<PairedSecretShare<T, NonZero>> {
         Some(PairedSecretShare {
             secret_share: self.secret_share,
-            shared_key: self.shared_key.non_zero()?,
+            public_key: self.public_key.non_zero()?,
         })
     }
 
     /// Is the key this is a share of zero
     pub fn is_zero(&self) -> bool {
-        self.shared_key.is_zero()
+        self.public_key.is_zero()
     }
 }
 
 impl PairedSecretShare<Normal, NonZero> {
     /// Create an XOnly secert share where the paired image is always an `EvenY` point.
     #[must_use]
     pub fn into_xonly(mut self) -> PairedSecretShare<EvenY> {
-        let (shared_key, needs_negation) = self.shared_key.into_point_with_even_y();
+        let (shared_key, needs_negation) = self.public_key.into_point_with_even_y();
         self.secret_share.share.conditional_negate(needs_negation);
 
         PairedSecretShare {
             secret_share: self.secret_share,
-            shared_key,
+            public_key: shared_key,
         }
     }
 }
@@ -488,7 +491,7 @@ mod test {
             let chosen = shares.choose_multiple(&mut rng, threshold).cloned()
                 .map(|paired_share| paired_share.secret_share).collect::<Vec<_>>();
             let secret = SecretShare::recover_secret(&chosen);
-            prop_assert_eq!(g!(secret * G), frost_poly.key());
+            prop_assert_eq!(g!(secret * G), frost_poly.public_key());
         }
     }
 }

schnorr_fun/src/frost/shared_key.rs

@@ -3,7 +3,7 @@ use core::{marker::PhantomData, ops::Deref};
 use alloc::vec::Vec;
 use secp256kfun::{poly, prelude::*};
 
-use super::PartyIndex;
+use super::{PairedSecretShare, PartyIndex, SecretShare};
 /// A polynomial
 #[derive(Clone, Debug, PartialEq, Eq)]
 #[cfg_attr(
@@ -31,20 +31,33 @@ impl<T: PointType, Z: ZeroChoice> SharedKey<T, Z> {
         poly::point::eval(&self.point_polynomial, index)
     }
 
+    /// "pair" a secret share that belongs to this shared key so you can keep track of tweaks to the
+    /// public key and the secret share together.
+    ///
+    /// Returns `None` if the secret share is not a valid share of this key.
+    pub fn pair_secret_share(&self, secret_share: SecretShare) -> Option<PairedSecretShare<T, Z>> {
+        if self.verification_share(secret_share.index) != g!(secret_share.share * G) {
+            return None;
+        }
+
+        Some(PairedSecretShare::new(secret_share, self.public_key()))
+    }
+
     /// The threshold number of participants required in a signing coalition to produce a valid signature.
     pub fn threshold(&self) -> usize {
         self.point_polynomial.len()
     }
 
-    /// The public image of the key's polynomial on the elliptic curve.
+    /// The internal public polynomial coefficients that defines the public key and the share structure.
     ///
-    /// Note: the first coefficient (index `0`) is guaranteed to be non-zero but the coefficients
-    /// may be.
+    /// To get the first coefficient of the polynomial typed correctly call [`public_key`].
+    ///
+    /// [`public_key`]: Self::public_key
     pub fn point_polynomial(&self) -> Vec<Point<Normal, Public, Zero>> {
         self.point_polynomial.clone()
     }
 
-    /// Type unsafe: you have to make sure the polynomial fits the type parameters
+    /// ☠ Type unsafe: you have to make sure the polynomial fits the type parameters
     fn from_inner(point_polynomial: Vec<Point<Normal, Public, Zero>>) -> Self {
         SharedKey {
             point_polynomial,
@@ -134,6 +147,19 @@ impl<T: PointType, Z: ZeroChoice> SharedKey<T, Z> {
         let poly = poly::point::normalize(poly);
         SharedKey::from_inner(poly.collect())
     }
+
+    /// The public key that has been shared.
+    ///
+    /// This is using *public key* in a rather loose sense. Unless it's a `SharedKey<EvenY>` then it
+    /// won't be usable as an actual Schnorr [BIP340] public key.
+    ///
+    /// [BIP340]: https://bips.xyz/340
+    pub fn public_key(&self) -> Point<T, Public, Z> {
+        // SAFETY: we hold the first coefficient to match the type parameters always
+        let public_key = Z::cast_point(self.point_polynomial[0]).expect("invariant");
+        let public_key = T::cast_point(public_key).expect("invariant");
+        public_key
+    }
 }
 
 impl SharedKey<Normal> {
@@ -143,49 +169,31 @@ impl SharedKey<Normal> {
     ///
     /// [BIP340]: https://bips.xyz/340
     pub fn into_xonly(mut self) -> SharedKey<EvenY> {
-        let needs_negation = !self.key().is_y_even();
+        let needs_negation = !self.public_key().is_y_even();
         if needs_negation {
             self = self.homomorphic_negate();
-            debug_assert!(self.key().is_y_even());
+            debug_assert!(self.public_key().is_y_even());
         }
 
         SharedKey::from_inner(self.point_polynomial)
     }
 }
 
-impl<Z: ZeroChoice> SharedKey<Normal, Z> {
-    /// The key that was shared with this polynomial defining the sharing.
-    ///
-    /// This is the first coefficient of the polynomial.
-    pub fn key(&self) -> Point<Normal, Public, Z> {
-        Z::cast_point(self.point_polynomial[0]).expect("invariant")
-    }
-    /// Constructor to create a from a vector of points where each item represent a polynomial
+impl SharedKey<Normal, Zero> {
+    /// Constructor to create a shared key from a vector of points where each item represent a polynomial
     /// coefficient.
     ///
-    /// Returns `None` if the first coefficient is [`Point::zero`].
-    pub fn from_poly(poly: Vec<Point<Normal, Public, Zero>>) -> Option<Self> {
+    /// The resulting shared key will be `SharedKey<Normal, Zero>`. It's up to the caller to do the zero check with [`non_zero`]
+    ///
+    /// [`non_zero`]: Self::non_zero
+    pub fn from_poly(poly: Vec<Point<Normal, Public, Zero>>) -> Self {
         if poly.is_empty() {
-            return None;
+            // an empty polynomial is represented as a vector with a single zero item to avoid
+            // panics
+            return Self::from_poly(vec![Point::zero()]);
         }
 
-        if poly[0].is_zero() && !Z::is_zero() {
-            return None;
-        }
-
-        Some(SharedKey::from_inner(poly))
-    }
-}
-
-impl SharedKey<EvenY> {
-    /// The public key that would have signatures verified against for this shared key.
-    pub fn key(&self) -> Point<EvenY> {
-        let (even_y_point, _needs_negation) = self.point_polynomial[0]
-            .non_zero()
-            .expect("invariant")
-            .into_point_with_even_y();
-        assert!(!_needs_negation);
-        even_y_point
+        SharedKey::from_inner(poly)
     }
 }
 

schnorr_fun/tests/frost_prop.rs

@@ -64,7 +64,7 @@ proptest! {
         }
 
         for secret_share in &xonly_secret_shares {
-            assert_eq!(secret_share.shared_key(), xonly_shared_key.key(), "shared key doesn't match");
+            assert_eq!(secret_share.public_key(), xonly_shared_key.public_key(), "shared key doesn't match");
         }
 
         // use a boolean mask for which t participants are signers
@@ -98,7 +98,7 @@ proptest! {
         );
 
         let party_signing_session = proto.party_sign_session(
-            xonly_shared_key.key(),
+            xonly_shared_key.public_key(),
             coord_signing_session.parties(),
             coord_signing_session.agg_binonce(),
             message,
@@ -126,7 +126,7 @@ proptest! {
 
         assert_eq!(proto.verify_and_combine_signature_shares(&xonly_shared_key, &coord_signing_session, signatures), Ok(combined_sig.clone()));
         assert!(proto.schnorr.verify(
-            &xonly_shared_key.key(),
+            &xonly_shared_key.public_key(),
             message,
             &combined_sig
         ));