Use SEC1 for the encoding of secq256k1 points, like secp256k1 does

crypto/evrf/embedwards25519/src/lib.rs

@@ -49,7 +49,7 @@ impl ShortWeierstrass for Embedwards25519 {
   type Repr = [u8; 32];
   // Use an all-zero encoding for the identity as `0` isn't the `x` coordinate of an on-curve point
   const IDENTITY: [u8; 32] = [0; 32];
-  fn compress(x: Self::FieldElement, odd_y: Choice) -> Self::Repr {
+  fn encode_compressed(x: Self::FieldElement, odd_y: Choice) -> Self::Repr {
     // The LE `x` coordinate, with if `y` is odd in the unused 256th bit
     let mut res = [0; 32];
     res.as_mut().copy_from_slice(x.to_repr().as_ref());

crypto/evrf/secq256k1/src/lib.rs

@@ -19,6 +19,7 @@ use k256::elliptic_curve::{
     ff::{PrimeField, FromUniformBytes},
     Group,
   },
+  sec1::Tag,
 };
 
 prime_field::odd_prime_field!(
@@ -71,13 +72,12 @@ impl ShortWeierstrass for Secq256k1 {
   type Scalar = Scalar;
 
   type Repr = GenericArray<u8, U33>;
-  // Use an all-zero encoding for the identity as `0` isn't the `x` coordinate of an on-curve point
+  /// Use the SEC1-encoded identity point, which happens to be all zeroes
-  // This uses `unsafe` to construct a `GenericArray` at compile-time as ``
   const IDENTITY: Self::Repr = GenericArray::from_array([0; 33]);
-  fn compress(x: Self::FieldElement, odd_y: Choice) -> Self::Repr {
+  fn encode_compressed(x: Self::FieldElement, odd_y: Choice) -> Self::Repr {
-    // If `y` is odd, followed by the big-endian `x` coordinate
     let mut res = GenericArray::default();
-    res[0] = odd_y.unwrap_u8();
+    res[0] =
+      <_>::conditional_select(&(Tag::CompressedEvenY as u8), &(Tag::CompressedOddY as u8), odd_y);
     {
       let res: &mut [u8] = res.as_mut();
       res[1 ..].copy_from_slice(x.to_repr().as_ref());
@@ -86,9 +86,11 @@ impl ShortWeierstrass for Secq256k1 {
   }
   fn decode_compressed(bytes: &Self::Repr) -> (<Self::FieldElement as PrimeField>::Repr, Choice) {
     // Parse out if `y` is odd
-    let odd_y = Choice::from(bytes[0] & 1);
+    let odd_y = bytes[0].ct_eq(&(Tag::CompressedOddY as u8));
-    // Check if the extra byte was malleated
+    // Check if the tag was malleated
-    let invalid = !bytes[0].ct_eq(&odd_y.unwrap_u8());
+    let expected_tag =
+      <_>::conditional_select(&(Tag::CompressedEvenY as u8), &(Tag::CompressedOddY as u8), odd_y);
+    let invalid = !bytes[0].ct_eq(&expected_tag);
 
     // Copy the alleged `x` coordinate, overwriting with `0xffffff...` if the sign byte was
     // malleated (causing the `x` coordinate to be invalid)
@@ -164,7 +166,7 @@ fn generator() {
   assert_eq!(
     Point::generator(),
     Point::from_bytes(GenericArray::from_slice(&hex_literal::hex!(
-      "000000000000000000000000000000000000000000000000000000000000000001"
+      "020000000000000000000000000000000000000000000000000000000000000001"
     )))
     .unwrap()
   );

crypto/ff-group-tests/src/group.rs

@@ -158,16 +158,20 @@ pub fn test_encoding<G: PrimeGroup>() {
     let bytes = point.to_bytes();
     let mut repr = G::Repr::default();
     repr.as_mut().copy_from_slice(bytes.as_ref());
-    let decoded = G::from_bytes(&repr).unwrap();
+    let decoded = G::from_bytes(&repr).expect("couldn't decode encoded point");
     assert_eq!(point, decoded, "{msg} couldn't be encoded and decoded");
     assert_eq!(
       point,
-      G::from_bytes_unchecked(&repr).unwrap(),
+      G::from_bytes_unchecked(&repr)
-      "{msg} couldn't be encoded and decoded",
+        .expect("couldn't decode encoded point with `from_bytes_unchecked`"),
+      "{msg} couldn't be encoded and decoded with `from_bytes_unchecked`",
     );
     decoded
   };
-  assert!(bool::from(test(G::identity(), "identity").is_identity()));
+  assert!(
+    bool::from(test(G::identity(), "identity").is_identity()),
+    "decoded identity was no longer the identity"
+  );
   test(G::generator(), "generator");
   test(G::generator() + G::generator(), "(generator * 2)");
 }

crypto/short-weierstrass/src/affine.rs

@@ -89,8 +89,9 @@ impl<C: ShortWeierstrass> Affine<C> {
   pub fn decompress(x: C::FieldElement, odd_y: Choice) -> CtOption<Self> {
     let y_square = ((x.square() + C::A) * x) + C::B;
     y_square.sqrt().and_then(|mut y| {
-      y = <_>::conditional_select(&y, &-y, odd_y.ct_ne(&y.is_odd()));
+      y = <_>::conditional_select(&y, &-y, y.is_odd().ct_ne(&odd_y));
-      CtOption::new(Self { x, y }, 1.into())
+      // Handles the exceptional case of `y = 0, odd_y = 1`
+      CtOption::new(Self { x, y }, y.is_odd().ct_eq(&odd_y))
     })
   }
 

crypto/short-weierstrass/src/lib.rs

@@ -35,13 +35,13 @@ pub trait ShortWeierstrass: 'static + Sized + Debug {
   type Repr: 'static + Send + Sync + Copy + Default + AsRef<[u8]> + AsMut<[u8]>;
   /// The representation of the identity point.
   const IDENTITY: Self::Repr;
-  /// Compress an affine point its byte encoding.
+  /// Encode a compresed, on-curve point to its byte encoding.
   ///
   /// The space of potential outputs MUST exclude `Self::IDENTITY`.
-  fn compress(x: Self::FieldElement, odd_y: Choice) -> Self::Repr;
+  fn encode_compressed(x: Self::FieldElement, odd_y: Choice) -> Self::Repr;
-  /// Decode a compressed point.
+  /// Decode the `x` coordinate and if the `y` coordinate is odd from a compressed representation.
   ///
-  /// This is expected to return the `x` coordinate and if the `y` coordinate is odd.
+  /// This MAY return any value if the bytes represent the identity.
   fn decode_compressed(bytes: &Self::Repr) -> (<Self::FieldElement as PrimeField>::Repr, Choice);
 
   /// If the point is outside the largest prime-order subgroup and isn't the identity point.

crypto/short-weierstrass/src/projective.rs

@@ -369,13 +369,11 @@ impl<C: ShortWeierstrass> GroupEncoding for Projective<C> {
 
     let (x, odd_y) = C::decode_compressed(bytes);
 
-    let result = C::FieldElement::from_repr(x).and_then(|x| {
+    let result = C::FieldElement::from_repr(x)
-      // Parse x and recover y
+      .and_then(|x| Affine::decompress(x, odd_y).map(Projective::from));
-      let non_identity_on_curve_point = Affine::decompress(x, odd_y).map(Projective::from);
     // Set the identity, if the identity
     let identity = CtOption::new(Projective::IDENTITY, identity);
-      non_identity_on_curve_point.or_else(|| identity)
+    let result = result.or_else(|| identity);
-    });
 
     let mut result_is_valid = result.is_some();
     let result = result.unwrap_or(Projective::IDENTITY);
@@ -394,7 +392,7 @@ impl<C: ShortWeierstrass> GroupEncoding for Projective<C> {
     let compressed_if_not_identity = {
       let affine_on_curve = affine_on_curve.unwrap_or(C::GENERATOR);
       let (x, y) = affine_on_curve.coordinates();
-      C::compress(x, y.is_odd())
+      C::encode_compressed(x, y.is_odd())
     };
 
     let mut res = C::Repr::default();