Refreshed Identifier newtype of Scalar with traits (#114)

* use Identifier instead of index

* remove pub(crate) from the Identifier index

* Refreshed Identifier newtype of Scalar with traits

* Remove commented out lines

* add test vectors with indices larger than 1 byte

* add little_endian_serialize to implement Ord for Identifier

Co-authored-by: Conrado Gouvea <conradoplg@gmail.com>

frost-core/src/frost.rs

@@ -146,8 +146,6 @@ fn derive_lagrange_coeff<C: Ciphersuite>(
     signer_id: &Identifier<C>,
     signing_package: &SigningPackage<C>,
 ) -> Result<<<C::Group as Group>::Field as Field>::Scalar, &'static str> {
-    let signer_id_scalar = signer_id.to_scalar()?;
-
     let zero = <<C::Group as Group>::Field as Field>::zero();
 
     let mut num = <<C::Group as Group>::Field as Field>::one();
@@ -161,10 +159,9 @@ fn derive_lagrange_coeff<C: Ciphersuite>(
             continue;
         }
 
-        let commitment_id_scalar = commitment.identifier.to_scalar()?;
+        num *= commitment.identifier;
 
-        num = num * commitment_id_scalar;
-        den = den * (commitment_id_scalar - signer_id_scalar);
+        den *= commitment.identifier - *signer_id;
     }
 
     if den == zero {
@@ -244,13 +241,7 @@ where
                 let mut rho_input = vec![];
 
                 rho_input.extend_from_slice(&rho_input_prefix);
-                rho_input.extend_from_slice(
-                    <<C::Group as Group>::Field as Field>::serialize(
-                        // unwrap() is OK because this will become infallible after refactoring (#102)
-                        &c.identifier.to_scalar().unwrap(),
-                    )
-                    .as_ref(),
-                );
+                rho_input.extend_from_slice(c.identifier.serialize().as_ref());
                 (c.identifier, rho_input)
             })
             .collect()

frost-core/src/frost/identifier.rs

@@ -3,7 +3,6 @@
 use std::{
     fmt::{self, Debug},
     hash::{Hash, Hasher},
-    marker::PhantomData,
 };
 
 use crate::{Ciphersuite, Error, Field, Group, Scalar};
@@ -13,83 +12,100 @@ use crate::{Ciphersuite, Error, Field, Group, Scalar};
 /// The identifier is a field element in the scalar field that the secret polynomial is defined
 /// over, corresponding to some x-coordinate for a polynomial f(x) = y.  MUST NOT be zero in the
 /// field, as f(0) = the shared secret.
-#[derive(Copy, Clone)]
-pub struct Identifier<C>(u16, PhantomData<C>);
+#[derive(Copy, Clone, PartialEq)]
+pub struct Identifier<C: Ciphersuite>(Scalar<C>);
 
 impl<C> Identifier<C>
 where
     C: Ciphersuite,
 {
-    // Convert the identifier to a Scalar.
-    //
-    // Ideally this would be a From<Identifier<C>> for Scalar<C> impl, but rustc
-    // doesn't like that
-    pub(crate) fn to_scalar(self) -> Result<Scalar<C>, &'static str> {
-        // This should never happen since we check it when building Identifier,
-        // but we check again out of abundance of caution.
-        if self.0 == 0 {
-            return Err("Identifier must not be zero");
-        }
-        // Classic left-to-right double-and-add algorithm that skips the first bit 1 (since
-        // identifiers are never zero, there is always a bit 1), thus `sum` starts with 1 too.
-        let one = <<C::Group as Group>::Field as Field>::one();
-        let mut sum = <<C::Group as Group>::Field as Field>::one();
-
-        let bits = (self.0.to_be_bytes().len() as u32) * 8;
-        for i in (0..(bits - self.0.leading_zeros() - 1)).rev() {
-            sum = sum + sum;
-            if self.0 & (1 << i) != 0 {
-                sum = sum + one;
-            }
-        }
-        Ok(sum)
+    // Serialize the underlying scalar.
+    pub(crate) fn serialize(&self) -> <<C::Group as Group>::Field as Field>::Serialization {
+        <<C::Group as Group>::Field as Field>::serialize(&self.0)
     }
 }
 
-impl<C> PartialEq for Identifier<C> {
-    fn eq(&self, other: &Self) -> bool {
-        self.0 == other.0
-    }
-}
-
-impl<C> Eq for Identifier<C> {}
-
-impl<C> PartialOrd for Identifier<C> {
-    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
-        self.0.partial_cmp(&other.0)
-    }
-}
-
-impl<C> Ord for Identifier<C> {
-    fn cmp(&self, other: &Self) -> std::cmp::Ordering {
-        self.0.cmp(&other.0)
-    }
-}
+impl<C> Eq for Identifier<C> where C: Ciphersuite {}
 
 impl<C> Debug for Identifier<C>
 where
     C: Ciphersuite,
 {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        f.debug_tuple("Identifier").field(&self.0).finish()
+        f.debug_tuple("Identifier")
+            .field(&<<C::Group as Group>::Field as Field>::serialize(&self.0).as_ref())
+            .finish()
     }
 }
 
+#[allow(clippy::derive_hash_xor_eq)]
 impl<C> Hash for Identifier<C>
 where
     C: Ciphersuite,
 {
     fn hash<H: Hasher>(&self, state: &mut H) {
-        self.0.hash(state)
+        <<C::Group as Group>::Field as Field>::serialize(&self.0)
+            .as_ref()
+            .hash(state)
     }
 }
 
-impl<C> From<Identifier<C>> for u16
+impl<C> Ord for Identifier<C>
 where
     C: Ciphersuite,
 {
-    fn from(identifier: Identifier<C>) -> Self {
-        identifier.0
+    fn cmp(&self, other: &Self) -> std::cmp::Ordering {
+        let serialized_self =
+            <<C::Group as Group>::Field as Field>::little_endian_serialize(&self.0);
+        let serialized_other =
+            <<C::Group as Group>::Field as Field>::little_endian_serialize(&other.0);
+        serialized_self.as_ref().cmp(serialized_other.as_ref())
+    }
+}
+
+impl<C> PartialOrd for Identifier<C>
+where
+    C: Ciphersuite,
+{
+    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+        let serialized_self =
+            <<C::Group as Group>::Field as Field>::little_endian_serialize(&self.0);
+        let serialized_other =
+            <<C::Group as Group>::Field as Field>::little_endian_serialize(&other.0);
+        serialized_self
+            .as_ref()
+            .partial_cmp(serialized_other.as_ref())
+    }
+}
+
+impl<C> std::ops::Mul<Scalar<C>> for Identifier<C>
+where
+    C: Ciphersuite,
+{
+    type Output = Scalar<C>;
+
+    fn mul(self, scalar: Scalar<C>) -> Scalar<C> {
+        self.0 * scalar
+    }
+}
+
+impl<C> std::ops::MulAssign<Identifier<C>> for Scalar<C>
+where
+    C: Ciphersuite,
+{
+    fn mul_assign(&mut self, identifier: Identifier<C>) {
+        *self = *self * identifier.0
+    }
+}
+
+impl<C> std::ops::Sub for Identifier<C>
+where
+    C: Ciphersuite,
+{
+    type Output = Self;
+
+    fn sub(self, rhs: Identifier<C>) -> Self::Output {
+        Self(self.0 - rhs.0)
     }
 }
 
@@ -103,7 +119,19 @@ where
         if n == 0 {
             Err(Self::Error::InvalidZeroScalar)
         } else {
-            Ok(Self(n, Default::default()))
+            // Classic left-to-right double-and-add algorithm that skips the first bit 1 (since
+            // identifiers are never zero, there is always a bit 1), thus `sum` starts with 1 too.
+            let one = <<C::Group as Group>::Field as Field>::one();
+            let mut sum = <<C::Group as Group>::Field as Field>::one();
+
+            let bits = (n.to_be_bytes().len() as u32) * 8;
+            for i in (0..(bits - n.leading_zeros() - 1)).rev() {
+                sum = sum + sum;
+                if n & (1 << i) != 0 {
+                    sum = sum + one;
+                }
+            }
+            Ok(Self(sum))
         }
     }
 }

frost-core/src/frost/keys.rs

@@ -349,10 +349,10 @@ fn evaluate_polynomial<C: Ciphersuite>(
     coefficients: &[Scalar<C>],
 ) -> Result<<<<C as Ciphersuite>::Group as Group>::Field as Field>::Scalar, &'static str> {
     let mut value = <<C::Group as Group>::Field as Field>::zero();
-    let ell_scalar = identifier.to_scalar()?;
+    let ell_scalar = identifier;
     for coeff in coefficients.iter().skip(1).rev() {
         value = value + *coeff;
-        value = ell_scalar * value;
+        value *= ell_scalar;
     }
     value = value + coefficients[0];
     Ok(value)
@@ -365,7 +365,7 @@ fn evaluate_vss<C: Ciphersuite>(
     commitment: &VerifiableSecretSharingCommitment<C>,
     identifier: Identifier<C>,
 ) -> Result<<<C as Ciphersuite>::Group as Group>::Element, &'static str> {
-    let i = identifier.to_scalar()?;
+    let i = identifier;
 
     let (_, result) = commitment.0.iter().fold(
         (
@@ -566,19 +566,17 @@ pub fn reconstruct_secret<C: Ciphersuite>(
     for (i, secret_share) in secret_share_map.clone() {
         let mut num = <<C::Group as Group>::Field as Field>::one();
         let mut den = <<C::Group as Group>::Field as Field>::one();
-        let i_scalar = i.to_scalar()?;
 
         for j in secret_share_map.clone().into_keys() {
             if j == i {
                 continue;
             }
-            let j_scalar = j.to_scalar()?;
 
             // numerator *= j
-            num = num * j_scalar;
+            num *= j;
 
             // denominator *= j - i
-            den = den * (j_scalar - i_scalar);
+            den *= j - i;
         }
 
         // If at this step, the denominator is zero in the scalar field, there must be a duplicate

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

@@ -144,12 +144,7 @@ where
 {
     let mut preimage = vec![];
 
-    let i_scalar = identifier
-        .to_scalar()
-        .expect("this will never fail after identifier is defined as scalar");
-
-    preimage
-        .extend_from_slice(<<C::Group as Group>::Field as Field>::serialize(&i_scalar).as_ref());
+    preimage.extend_from_slice(identifier.serialize().as_ref());
     preimage.extend_from_slice(<C::Group as Group>::serialize(R).as_ref());
     preimage.extend_from_slice(<C::Group as Group>::serialize(verifying_key).as_ref());
 

frost-core/src/frost/round1.rs

@@ -281,11 +281,7 @@ pub(super) fn encode_group_commitments<C: Ciphersuite>(
     let mut bytes = vec![];
 
     for item in sorted_signing_commitments {
-        bytes.extend_from_slice(
-            // unwrap() is OK because this will become infallible after refactoring (#102)
-            <<C::Group as Group>::Field as Field>::serialize(&item.identifier.to_scalar().unwrap())
-                .as_ref(),
-        );
+        bytes.extend_from_slice(item.identifier.serialize().as_ref());
         bytes.extend_from_slice(<C::Group as Group>::serialize(&item.hiding.0).as_ref());
         bytes.extend_from_slice(<C::Group as Group>::serialize(&item.binding.0).as_ref());
     }

frost-core/src/lib.rs

@@ -46,8 +46,6 @@ pub trait Field: Copy + Clone {
         + Sub<Output = Self::Scalar>;
 
     /// A unique byte array buf of fixed length N.
-    ///
-    /// Little-endian!
     type Serialization: AsRef<[u8]> + Debug + Default + TryFrom<Vec<u8>>;
 
     /// Returns the zero element of the field, the additive identity.
@@ -76,6 +74,12 @@ pub trait Field: Copy + Clone {
     /// <https://www.ietf.org/archive/id/draft-irtf-cfrg-frost-11.html#section-3.1-3.8>
     fn serialize(scalar: &Self::Scalar) -> Self::Serialization;
 
+    /// A member function of a [`Field`] that maps a [`Scalar`] to a unique byte array buf of
+    /// fixed length Ne, in little-endian order.
+    ///
+    /// This is used internally.
+    fn little_endian_serialize(scalar: &Self::Scalar) -> Self::Serialization;
+
     /// A member function of a [`Field`] that attempts to map a byte array `buf` to a [`Scalar`].
     ///
     /// Fails if the input is not a valid byte representation of an [`Scalar`] of the

frost-p256/src/lib.rs

@@ -74,6 +74,12 @@ impl Field for P256ScalarField {
             None => Err(Error::MalformedScalar),
         }
     }
+
+    fn little_endian_serialize(scalar: &Self::Scalar) -> Self::Serialization {
+        let mut array = Self::serialize(scalar);
+        array.reverse();
+        array
+    }
 }
 
 #[derive(Clone, Copy, PartialEq, Eq)]

frost-ristretto255/src/lib.rs

@@ -70,6 +70,10 @@ impl Field for RistrettoScalarField {
             None => Err(Error::MalformedScalar),
         }
     }
+
+    fn little_endian_serialize(scalar: &Self::Scalar) -> Self::Serialization {
+        Self::serialize(scalar)
+    }
 }
 
 #[derive(Clone, Copy, PartialEq, Eq)]