Test and refactor EvenY (renamed from PositiveY) (#94)

* rename PositiveY into EvenY; refactor it

change reexposed FROST functions to already ensure even Y keys

* add comment

src/frost/redpallas.rs

@@ -1,4 +1,10 @@
 //! Rerandomized FROST with Pallas curve.
+//!
+//! This also re-exposes the FROST functions already parametrized with the
+//! Pallas curve. Note that if you use the generic frost-core functions instead,
+//! you will not get public keys with guaranteed even Y coordinate, and will
+//! need to convert them using the [`EvenY`] trait; see its documentation for
+//! details.
 #![allow(non_snake_case)]
 #![deny(missing_docs)]
 
@@ -207,7 +213,7 @@ pub type Identifier = frost::Identifier<P>;
 
 /// FROST(Pallas, BLAKE2b-512) keys, key generation, key shares.
 pub mod keys {
-    use alloc::collections::BTreeMap;
+    use alloc::{collections::BTreeMap, vec::Vec};
 
     use super::*;
 
@@ -222,7 +228,12 @@ pub mod keys {
         identifiers: IdentifierList,
         mut rng: RNG,
     ) -> Result<(BTreeMap<Identifier, SecretShare>, PublicKeyPackage), Error> {
-        frost::keys::generate_with_dealer(max_signers, min_signers, identifiers, &mut rng)
+        Ok(into_even_y(frost::keys::generate_with_dealer(
+            max_signers,
+            min_signers,
+            identifiers,
+            &mut rng,
+        )?))
     }
 
     /// Splits an existing key into FROST shares.
@@ -238,7 +249,13 @@ pub mod keys {
         identifiers: IdentifierList,
         rng: &mut R,
     ) -> Result<(BTreeMap<Identifier, SecretShare>, PublicKeyPackage), Error> {
-        frost::keys::split(key, max_signers, min_signers, identifiers, rng)
+        Ok(into_even_y(frost::keys::split(
+            key,
+            max_signers,
+            min_signers,
+            identifiers,
+            rng,
+        )?))
     }
 
     /// Secret and public key material generated by a dealer performing
@@ -284,19 +301,47 @@ pub mod keys {
     /// ensure that they received the correct (and same) value.
     pub type VerifiableSecretSharingCommitment = frost::keys::VerifiableSecretSharingCommitment<P>;
 
-    /// Trait for ensuring the group public key has a positive Y coordinate.
-    pub trait PositiveY {
-        /// Convert the given package to make sure the group public key has
-        /// a positive Y coordinate.
-        fn into_positive_y(self) -> Self;
+    /// Trait for ensuring the group public key has an even Y coordinate.
+    ///
+    /// In the [Zcash spec][1], Orchard spend authorizing keys (which are then
+    /// ones where FROST applies) are generated so that their matching public
+    /// keys have a even Y coordinate.
+    ///
+    /// This trait is used to enable this procedure, by changing the private and
+    /// public keys to ensure that the public key has a even Y coordinate. This
+    /// is done by simply negating both keys if Y is even (in a field, negating
+    /// is equivalent to computing p - x where p is the prime modulus. Since p
+    /// is odd, if x is odd then the result will be even). Fortunately this
+    /// works even after Shamir secret sharing, in the individual signing and
+    /// verifying shares, since it's linear.
+    ///
+    /// [1]: https://zips.z.cash/protocol/protocol.pdf#orchardkeycomponents
+    pub trait EvenY {
+        /// Return if the given type has a group public key with an even Y
+        /// coordinate.
+        fn has_even_y(&self) -> bool;
+
+        /// Convert the given type to make sure the group public key has an even
+        /// Y coordinate. `is_even` can be specified if evenness was already
+        /// determined beforehand. Returns a boolean indicating if the original
+        /// type had an even Y, and a (possibly converted) value with even Y.
+        fn into_even_y(self, is_even: Option<bool>) -> Self;
     }
 
-    impl PositiveY for PublicKeyPackage {
-        fn into_positive_y(self) -> Self {
-            let pubkey = self.verifying_key();
-            let pubkey_serialized = pubkey.serialize();
-            if pubkey_serialized[31] & 0x80 != 0 {
-                let pubkey = VerifyingKey::new(-pubkey.to_element());
+    impl EvenY for PublicKeyPackage {
+        fn has_even_y(&self) -> bool {
+            let verifying_key = self.verifying_key();
+            let verifying_key_serialized = verifying_key.serialize();
+            verifying_key_serialized[31] & 0x80 == 0
+        }
+
+        fn into_even_y(self, is_even: Option<bool>) -> Self {
+            let is_even = is_even.unwrap_or_else(|| self.has_even_y());
+            if !is_even {
+                // Negate verifying key
+                let verifying_key = VerifyingKey::new(-self.verifying_key().to_element());
+                // Recreate verifying share map with negated VerifyingShares
+                // values.
                 let verifying_shares: BTreeMap<_, _> = self
                     .verifying_shares()
                     .iter()
@@ -305,26 +350,64 @@ pub mod keys {
                         (*i, vs)
                     })
                     .collect();
-                PublicKeyPackage::new(verifying_shares, pubkey)
+                PublicKeyPackage::new(verifying_shares, verifying_key)
             } else {
                 self
             }
         }
     }
 
-    impl PositiveY for KeyPackage {
-        fn into_positive_y(self) -> Self {
+    impl EvenY for SecretShare {
+        fn has_even_y(&self) -> bool {
+            let key_package: KeyPackage = self
+                .clone()
+                .try_into()
+                .expect("Should work; expected to be called in freshly generated SecretShares");
+            key_package.has_even_y()
+        }
+
+        fn into_even_y(self, is_even: Option<bool>) -> Self {
+            let is_even = is_even.unwrap_or_else(|| self.has_even_y());
+            if !is_even {
+                // Negate SigningShare
+                let signing_share = SigningShare::new(-self.signing_share().to_scalar());
+                // Negate VerifiableSecretSharingCommitment by negating each
+                // coefficient in it. TODO: remove serialization roundtrip
+                // workaround after required functions are added to frost-core
+                let coefficients: Vec<_> = self
+                    .commitment()
+                    .coefficients()
+                    .iter()
+                    .map(|e| <PallasBlake2b512 as Ciphersuite>::Group::serialize(&-e.value()))
+                    .collect();
+                let commitments = VerifiableSecretSharingCommitment::deserialize(coefficients)
+                    .expect("Should work since they were just serialized");
+                SecretShare::new(*self.identifier(), signing_share, commitments)
+            } else {
+                self
+            }
+        }
+    }
+
+    impl EvenY for KeyPackage {
+        fn has_even_y(&self) -> bool {
             let pubkey = self.verifying_key();
             let pubkey_serialized = pubkey.serialize();
-            if pubkey_serialized[31] & 0x80 != 0 {
-                let pubkey = VerifyingKey::new(-pubkey.to_element());
+            pubkey_serialized[31] & 0x80 == 0
+        }
+
+        fn into_even_y(self, is_even: Option<bool>) -> Self {
+            let is_even = is_even.unwrap_or_else(|| self.has_even_y());
+            if !is_even {
+                // Negate all components
+                let verifying_key = VerifyingKey::new(-self.verifying_key().to_element());
                 let signing_share = SigningShare::new(-self.signing_share().to_scalar());
                 let verifying_share = VerifyingShare::new(-self.verifying_share().to_element());
                 KeyPackage::new(
                     *self.identifier(),
                     signing_share,
                     verifying_share,
-                    pubkey,
+                    verifying_key,
                     *self.min_signers(),
                 )
             } else {
@@ -333,6 +416,20 @@ pub mod keys {
         }
     }
 
+    // Helper function which calls into_even_y() on the return values of
+    // keygen/split functions.
+    fn into_even_y(
+        (secret_shares, public_key_package): (BTreeMap<Identifier, SecretShare>, PublicKeyPackage),
+    ) -> (BTreeMap<Identifier, SecretShare>, PublicKeyPackage) {
+        let is_even = public_key_package.has_even_y();
+        let public_key_package = public_key_package.into_even_y(Some(is_even));
+        let secret_shares = secret_shares
+            .iter()
+            .map(|(i, s)| (*i, s.clone().into_even_y(Some(is_even))))
+            .collect();
+        (secret_shares, public_key_package)
+    }
+
     pub mod dkg;
     pub mod repairable;
 }

src/frost/redpallas/keys/dkg.rs

@@ -83,5 +83,11 @@ pub fn part3(
     round1_packages: &BTreeMap<Identifier, round1::Package>,
     round2_packages: &BTreeMap<Identifier, round2::Package>,
 ) -> Result<(KeyPackage, PublicKeyPackage), Error> {
-    frost::keys::dkg::part3(round2_secret_package, round1_packages, round2_packages)
+    let (key_package, public_key_package) =
+        frost::keys::dkg::part3(round2_secret_package, round1_packages, round2_packages)?;
+    let is_even = public_key_package.has_even_y();
+    Ok((
+        key_package.into_even_y(Some(is_even)),
+        public_key_package.into_even_y(Some(is_even)),
+    ))
 }

tests/frost_redpallas.rs

@@ -1,18 +1,20 @@
 #![cfg(feature = "frost")]
 
-use frost_rerandomized::frost_core::{Ciphersuite, Group, GroupError};
+use std::collections::BTreeMap;
+
+use frost_rerandomized::frost_core::{self as frost, Ciphersuite, Group, GroupError};
 use rand::thread_rng;
 
-use reddsa::{frost::redpallas::PallasBlake2b512, orchard};
+use reddsa::{
+    frost::redpallas::{keys::EvenY, PallasBlake2b512},
+    orchard,
+};
 
 #[test]
 fn check_sign_with_dealer() {
     let rng = thread_rng();
 
-    frost_rerandomized::frost_core::tests::ciphersuite_generic::check_sign_with_dealer::<
-        PallasBlake2b512,
-        _,
-    >(rng);
+    frost::tests::ciphersuite_generic::check_sign_with_dealer::<PallasBlake2b512, _>(rng);
 }
 
 #[test]
@@ -46,10 +48,7 @@ fn check_randomized_sign_with_dealer() {
 fn check_sign_with_dkg() {
     let rng = thread_rng();
 
-    frost_rerandomized::frost_core::tests::ciphersuite_generic::check_sign_with_dkg::<
-        PallasBlake2b512,
-        _,
-    >(rng);
+    frost::tests::ciphersuite_generic::check_sign_with_dkg::<PallasBlake2b512, _>(rng);
 }
 
 #[test]
@@ -78,3 +77,112 @@ fn check_deserialize_non_canonical() {
     let r = <PallasBlake2b512 as Ciphersuite>::Group::deserialize(&encoded_point);
     assert_eq!(r, Err(GroupError::MalformedElement));
 }
+
+#[test]
+fn check_even_y_frost_core() {
+    let mut rng = thread_rng();
+
+    // Since there is a 50% chance of the public key having an odd Y (which
+    // we need to actually test), loop until we get an odd Y.
+    loop {
+        let max_signers = 5;
+        let min_signers = 3;
+        // Generate keys with frost-core function, which doesn't ensure even Y
+        let (shares, public_key_package) =
+            frost::keys::generate_with_dealer::<PallasBlake2b512, _>(
+                max_signers,
+                min_signers,
+                frost::keys::IdentifierList::Default,
+                &mut rng,
+            )
+            .unwrap();
+
+        if !public_key_package.has_even_y() {
+            // Test consistency of into_even_y() for PublicKeyPackage
+            let even_public_key_package_is_even_none = public_key_package.clone().into_even_y(None);
+            let even_public_key_package_is_even_false =
+                public_key_package.clone().into_even_y(Some(false));
+            assert_eq!(
+                even_public_key_package_is_even_false,
+                even_public_key_package_is_even_none
+            );
+            assert_ne!(public_key_package, even_public_key_package_is_even_false);
+            assert_ne!(public_key_package, even_public_key_package_is_even_none);
+
+            // Test consistency of into_even_y() for SecretShare (arbitrarily on
+            // the first secret share)
+            let secret_share = shares.first_key_value().unwrap().1.clone();
+            let even_secret_share_is_even_none = secret_share.clone().into_even_y(None);
+            let even_secret_share_is_even_false = secret_share.clone().into_even_y(Some(false));
+            assert_eq!(
+                even_secret_share_is_even_false,
+                even_secret_share_is_even_none
+            );
+            assert_ne!(secret_share, even_secret_share_is_even_false);
+            assert_ne!(secret_share, even_secret_share_is_even_none);
+
+            // Make secret shares even, then convert into KeyPackages
+            let key_packages_evened_before: BTreeMap<_, _> = shares
+                .clone()
+                .into_iter()
+                .map(|(identifier, share)| {
+                    Ok((
+                        identifier,
+                        frost::keys::KeyPackage::try_from(share.into_even_y(None))?,
+                    ))
+                })
+                .collect::<Result<_, frost::Error<PallasBlake2b512>>>()
+                .unwrap();
+            // Convert into KeyPackages, then make them even
+            let key_packages_evened_after: BTreeMap<_, _> = shares
+                .into_iter()
+                .map(|(identifier, share)| {
+                    Ok((
+                        identifier,
+                        frost::keys::KeyPackage::try_from(share)?.into_even_y(None),
+                    ))
+                })
+                .collect::<Result<_, frost::Error<PallasBlake2b512>>>()
+                .unwrap();
+            // Make sure they are equal
+            assert_eq!(key_packages_evened_after, key_packages_evened_before);
+
+            // Check if signing works with evened keys
+            frost::tests::ciphersuite_generic::check_sign(
+                min_signers,
+                key_packages_evened_after,
+                &mut rng,
+                even_public_key_package_is_even_none,
+            )
+            .unwrap();
+
+            // We managed to test it; break the loop and return
+            break;
+        }
+    }
+}
+
+#[test]
+fn check_even_y_reddsa() {
+    let mut rng = thread_rng();
+
+    // Since there is a ~50% chance of having a odd Y internally, to make sure
+    // that odd Ys are converted to even, we test multiple times to increase
+    // the chance of an odd Y being generated internally
+    for _ in 0..16 {
+        let max_signers = 5;
+        let min_signers = 3;
+        // Generate keys with reexposed reddsa function, which ensures even Y
+        let (shares, public_key_package) =
+            reddsa::frost::redpallas::keys::generate_with_dealer::<_>(
+                max_signers,
+                min_signers,
+                frost::keys::IdentifierList::Default,
+                &mut rng,
+            )
+            .unwrap();
+
+        assert!(public_key_package.has_even_y());
+        assert!(shares.values().all(|s| s.has_even_y()));
+    }
+}