Implement stable comparison functionality

Currently we rely on the inner bytes with types that are passed across
the FFI boundry when implementing comparison functions (e.g. `Ord`,
`PartialEq`), this is incorrect because the bytes are opaque, meaning
the byte layout is not guaranteed across versions of `libsecp26k1`.

Implement stable comparison functionality by doing:

- Implement `core::cmp` traits by first coercing the data into a stable
  form e.g., by serializing it.
- Add fast comparison methods to `secp256k1-sys` types that wrap types
  from libsecp, add similar methods to types in `secp256k1` that wrap
  `secp256k1-sys` types (just call through to inner type).
- In `secp256k1-sys` feature gate the new `core::cmp` impls on
  `not(fuzzing)`, when fuzzing just derive the impls instead.

Any additional methods added to `secp256k1-sys` types are private,
justified by the fact the -sys is meant to be just a thin wrapper around
libsecp256k1, we don't want to commit to supporting additional API
functions.

Please note, the solution presented in this patch is already present for
`secp256k1::PublicKey`, this PR removes that code in favour of deriving
traits that then call down to the same logic in `secp256k1-sys`.

secp256k1-sys/src/lib.rs

@@ -169,6 +169,59 @@ impl PublicKey {
     pub fn underlying_bytes(self) -> [c_uchar; 64] {
         self.0
     }
+
+    /// Serializes this public key as a byte-encoded pair of values, in compressed form.
+    fn serialize(&self) -> [u8; 33] {
+        let mut buf = [0u8; 33];
+        let mut len = 33;
+        unsafe {
+            let ret = secp256k1_ec_pubkey_serialize(
+                secp256k1_context_no_precomp,
+                buf.as_mut_c_ptr(),
+                &mut len,
+                self,
+                SECP256K1_SER_COMPRESSED,
+            );
+            debug_assert_eq!(ret, 1);
+            debug_assert_eq!(len, 33);
+        };
+        buf
+    }
+}
+
+#[cfg(not(fuzzing))]
+impl PartialOrd for PublicKey {
+    fn partial_cmp(&self, other: &PublicKey) -> Option<core::cmp::Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+#[cfg(not(fuzzing))]
+impl Ord for PublicKey {
+    fn cmp(&self, other: &PublicKey) -> core::cmp::Ordering {
+        let ret = unsafe {
+            secp256k1_ec_pubkey_cmp(secp256k1_context_no_precomp, self, other)
+        };
+        ret.cmp(&0i32)
+    }
+}
+
+#[cfg(not(fuzzing))]
+impl PartialEq for PublicKey {
+    fn eq(&self, other: &Self) -> bool {
+        self.cmp(other) == core::cmp::Ordering::Equal
+    }
+}
+
+#[cfg(not(fuzzing))]
+impl Eq for PublicKey {}
+
+#[cfg(not(fuzzing))]
+impl core::hash::Hash for PublicKey {
+    fn hash<H: core::hash::Hasher>(&self, state: &mut H) {
+        let ser = self.serialize();
+        ser.hash(state);
+    }
 }
 
 /// Library-internal representation of a Secp256k1 signature
@@ -209,6 +262,54 @@ impl Signature {
     pub fn underlying_bytes(self) -> [c_uchar; 64] {
         self.0
     }
+
+    /// Serializes the signature in compact format.
+    fn serialize(&self) -> [u8; 64] {
+        let mut buf = [0u8; 64];
+        unsafe {
+            let ret = secp256k1_ecdsa_signature_serialize_compact(
+                secp256k1_context_no_precomp,
+                buf.as_mut_c_ptr(),
+                self,
+            );
+            debug_assert!(ret == 1);
+        }
+        buf
+    }
+}
+
+#[cfg(not(fuzzing))]
+impl PartialOrd for Signature {
+    fn partial_cmp(&self, other: &Signature) -> Option<core::cmp::Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+#[cfg(not(fuzzing))]
+impl Ord for Signature {
+    fn cmp(&self, other: &Signature) -> core::cmp::Ordering {
+        let this = self.serialize();
+        let that = other.serialize();
+        this.cmp(&that)
+    }
+}
+
+#[cfg(not(fuzzing))]
+impl PartialEq for Signature {
+    fn eq(&self, other: &Self) -> bool {
+        self.cmp(other) == core::cmp::Ordering::Equal
+    }
+}
+
+#[cfg(not(fuzzing))]
+impl Eq for Signature {}
+
+#[cfg(not(fuzzing))]
+impl core::hash::Hash for Signature {
+    fn hash<H: core::hash::Hasher>(&self, state: &mut H) {
+        let ser = self.serialize();
+        ser.hash(state);
+    }
 }
 
 #[repr(C)]
@@ -248,6 +349,55 @@ impl XOnlyPublicKey {
     pub fn underlying_bytes(self) -> [c_uchar; 64] {
         self.0
     }
+
+    /// Serializes this key as a byte-encoded x coordinate value (32 bytes).
+    fn serialize(&self) -> [u8; 32] {
+        let mut buf = [0u8; 32];
+        unsafe {
+            let ret = secp256k1_xonly_pubkey_serialize(
+                secp256k1_context_no_precomp,
+                buf.as_mut_c_ptr(),
+                self,
+            );
+            assert_eq!(ret, 1);
+        };
+        buf
+    }
+}
+
+#[cfg(not(fuzzing))]
+impl PartialOrd for XOnlyPublicKey {
+    fn partial_cmp(&self, other: &XOnlyPublicKey) -> Option<core::cmp::Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+#[cfg(not(fuzzing))]
+impl Ord for XOnlyPublicKey {
+    fn cmp(&self, other: &XOnlyPublicKey) -> core::cmp::Ordering {
+        let ret = unsafe {
+            secp256k1_xonly_pubkey_cmp(secp256k1_context_no_precomp, self, other)
+        };
+        ret.cmp(&0i32)
+    }
+}
+
+#[cfg(not(fuzzing))]
+impl PartialEq for XOnlyPublicKey {
+    fn eq(&self, other: &Self) -> bool {
+        self.cmp(other) == core::cmp::Ordering::Equal
+    }
+}
+
+#[cfg(not(fuzzing))]
+impl Eq for XOnlyPublicKey {}
+
+#[cfg(not(fuzzing))]
+impl core::hash::Hash for XOnlyPublicKey {
+    fn hash<H: core::hash::Hasher>(&self, state: &mut H) {
+        let ser = self.serialize();
+        ser.hash(state);
+    }
 }
 
 #[repr(C)]
@@ -287,6 +437,58 @@ impl KeyPair {
     pub fn underlying_bytes(self) -> [c_uchar; 96] {
         self.0
     }
+
+    /// Creates a new compressed public key from this key pair.
+    fn public_key(&self) -> PublicKey {
+        unsafe {
+            let mut pk = PublicKey::new();
+            let ret = secp256k1_keypair_pub(
+                secp256k1_context_no_precomp,
+                &mut pk,
+                self,
+            );
+            debug_assert_eq!(ret, 1);
+            pk
+        }
+    }
+}
+
+#[cfg(not(fuzzing))]
+impl PartialOrd for KeyPair {
+    fn partial_cmp(&self, other: &KeyPair) -> Option<core::cmp::Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+#[cfg(not(fuzzing))]
+impl Ord for KeyPair {
+    fn cmp(&self, other: &KeyPair) -> core::cmp::Ordering {
+        let this = self.public_key();
+        let that = other.public_key();
+        this.cmp(&that)
+    }
+}
+
+#[cfg(not(fuzzing))]
+impl PartialEq for KeyPair {
+    fn eq(&self, other: &Self) -> bool {
+        self.cmp(other) == core::cmp::Ordering::Equal
+    }
+}
+
+#[cfg(not(fuzzing))]
+impl Eq for KeyPair {}
+
+#[cfg(not(fuzzing))]
+impl core::hash::Hash for KeyPair {
+    fn hash<H: core::hash::Hasher>(&self, state: &mut H) {
+        // To hash the key pair we just hash the serialized public key. Since any change to the
+        // secret key would also be a change to the public key this is a valid one way function from
+        // the key pair to the digest.
+        let pk = self.public_key();
+        let ser = pk.serialize();
+        ser.hash(state);
+    }
 }
 
 extern "C" {

secp256k1-sys/src/macros.rs

@@ -19,6 +19,34 @@ macro_rules! impl_array_newtype {
     ($thing:ident, $ty:ty, $len:expr) => {
         impl Copy for $thing {}
 
+        impl $thing {
+            /// Like `cmp::Ord` but faster and with no guarantees across library versions.
+            ///
+            /// The inner byte array of `Self` is passed across the FFI boundry, as such there are
+            /// no guarantees on its layout and it is subject to change across library versions,
+            /// even minor versions. For this reason comparison function implementations (e.g.
+            /// `Ord`, `PartialEq`) take measures to ensure the data will remain constant (e.g., by
+            /// serializing it to a guaranteed format). This means they may be slow, this function
+            /// provides a faster comparison if you know that your types come from the same library
+            /// version.
+            pub fn cmp_fast_unstable(&self, other: &Self) -> core::cmp::Ordering {
+                self[..].cmp(&other[..])
+            }
+
+            /// Like `cmp::Eq` but faster and with no guarantees across library versions.
+            ///
+            /// The inner byte array of `Self` is passed across the FFI boundry, as such there are
+            /// no guarantees on its layout and it is subject to change across library versions,
+            /// even minor versions. For this reason comparison function implementations (e.g.
+            /// `Ord`, `PartialEq`) take measures to ensure the data will remain constant (e.g., by
+            /// serializing it to a guaranteed format). This means they may be slow, this function
+            /// provides a faster equality check if you know that your types come from the same
+            /// library version.
+            pub fn eq_fast_unstable(&self, other: &Self) -> bool {
+                self[..].eq(&other[..])
+            }
+        }
+
         impl AsRef<[$ty; $len]> for $thing {
             #[inline]
             /// Gets a reference to the underlying array
@@ -28,6 +56,9 @@ macro_rules! impl_array_newtype {
             }
         }
 
+        // We cannot derive these traits because Rust 1.41.1 requires `std::array::LengthAtMost32`.
+
+       #[cfg(fuzzing)]
         impl PartialEq for $thing {
             #[inline]
             fn eq(&self, other: &$thing) -> bool {
@@ -35,14 +66,17 @@ macro_rules! impl_array_newtype {
             }
         }
 
+        #[cfg(fuzzing)]
         impl Eq for $thing {}
 
+        #[cfg(fuzzing)]
         impl core::hash::Hash for $thing {
             fn hash<H: core::hash::Hasher>(&self, state: &mut H) {
                 (&self[..]).hash(state)
             }
         }
 
+        #[cfg(fuzzing)]
         impl PartialOrd for $thing {
             #[inline]
             fn partial_cmp(&self, other: &$thing) -> Option<core::cmp::Ordering> {
@@ -50,6 +84,7 @@ macro_rules! impl_array_newtype {
             }
         }
 
+        #[cfg(fuzzing)]
         impl Ord for $thing {
             #[inline]
             fn cmp(&self, other: &$thing) -> core::cmp::Ordering {

secp256k1-sys/src/recovery.rs

@@ -15,7 +15,7 @@
 
 //! # FFI of the recovery module
 
-use crate::{Context, Signature, NonceFn, PublicKey, CPtr, impl_array_newtype};
+use crate::{Context, Signature, NonceFn, PublicKey, CPtr, impl_array_newtype, secp256k1_context_no_precomp};
 use crate::types::*;
 use core::fmt;
 
@@ -27,6 +27,23 @@ impl_array_newtype!(RecoverableSignature, c_uchar, 65);
 impl RecoverableSignature {
     /// Create a new (zeroed) signature usable for the FFI interface
     pub fn new() -> RecoverableSignature { RecoverableSignature([0; 65]) }
+
+    /// Serializes the signature in compact format.
+    fn serialize(&self) -> [u8; 65] {
+        let mut buf = [0u8; 65];
+        let mut recid = 0;
+        unsafe {
+            let ret = secp256k1_ecdsa_recoverable_signature_serialize_compact(
+                secp256k1_context_no_precomp,
+                buf.as_mut_c_ptr(),
+                &mut recid,
+                self,
+            );
+            debug_assert!(ret == 1);
+        }
+        buf[64] = (recid & 0xFF) as u8;
+        buf
+    }
 }
 
 impl Default for RecoverableSignature {
@@ -59,6 +76,40 @@ impl fmt::Debug for RecoverableSignature {
     }
 }
 
+#[cfg(not(fuzzing))]
+impl PartialOrd for RecoverableSignature {
+    fn partial_cmp(&self, other: &RecoverableSignature) -> Option<core::cmp::Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+#[cfg(not(fuzzing))]
+impl Ord for RecoverableSignature {
+    fn cmp(&self, other: &RecoverableSignature) -> core::cmp::Ordering {
+        let this = self.serialize();
+        let that = other.serialize();
+        this.cmp(&that)
+    }
+}
+
+#[cfg(not(fuzzing))]
+impl PartialEq for RecoverableSignature {
+    fn eq(&self, other: &Self) -> bool {
+        self.cmp(other) == core::cmp::Ordering::Equal
+    }
+}
+
+#[cfg(not(fuzzing))]
+impl Eq for RecoverableSignature {}
+
+#[cfg(not(fuzzing))]
+impl core::hash::Hash for RecoverableSignature {
+    fn hash<H: core::hash::Hasher>(&self, state: &mut H) {
+        let ser = self.serialize();
+        ser.hash(state);
+    }
+}
+
 extern "C" {
     #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_6_1_ecdsa_recoverable_signature_parse_compact")]
     pub fn secp256k1_ecdsa_recoverable_signature_parse_compact(cx: *const Context, sig: *mut RecoverableSignature,

src/ecdsa/mod.rs

@@ -18,8 +18,9 @@ use crate::{
 };
 
 /// An ECDSA signature
-#[derive(Copy, Clone, PartialEq, Eq, Hash)]
+#[derive(Copy, Clone, PartialOrd, Ord, PartialEq, Eq, Hash)]
 pub struct Signature(pub(crate) ffi::Signature);
+impl_fast_comparisons!(Signature);
 
 impl fmt::Debug for Signature {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Display::fmt(self, f) }

src/key.rs

@@ -94,10 +94,10 @@ impl str::FromStr for SecretKey {
 /// ```
 /// [`bincode`]: https://docs.rs/bincode
 /// [`cbor`]: https://docs.rs/cbor
-#[derive(Copy, Clone, Debug)]
+#[derive(Copy, Clone, Debug, PartialOrd, Ord, PartialEq, Eq, Hash)]
-#[cfg_attr(fuzzing, derive(PartialOrd, Ord, PartialEq, Eq, Hash))]
 #[repr(transparent)]
 pub struct PublicKey(ffi::PublicKey);
+impl_fast_comparisons!(PublicKey);
 
 impl fmt::LowerHex for PublicKey {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
@@ -718,43 +718,6 @@ impl<'de> serde::Deserialize<'de> for PublicKey {
     }
 }
 
-#[cfg(not(fuzzing))]
-impl PartialOrd for PublicKey {
-    fn partial_cmp(&self, other: &PublicKey) -> Option<core::cmp::Ordering> {
-        Some(self.cmp(other))
-    }
-}
-
-#[cfg(not(fuzzing))]
-impl Ord for PublicKey {
-    fn cmp(&self, other: &PublicKey) -> core::cmp::Ordering {
-        let ret = unsafe {
-            ffi::secp256k1_ec_pubkey_cmp(
-                ffi::secp256k1_context_no_precomp,
-                self.as_c_ptr(),
-                other.as_c_ptr(),
-            )
-        };
-        ret.cmp(&0i32)
-    }
-}
-
-#[cfg(not(fuzzing))]
-impl PartialEq for PublicKey {
-    fn eq(&self, other: &Self) -> bool { self.cmp(other) == core::cmp::Ordering::Equal }
-}
-
-#[cfg(not(fuzzing))]
-impl Eq for PublicKey {}
-
-#[cfg(not(fuzzing))]
-impl core::hash::Hash for PublicKey {
-    fn hash<H: core::hash::Hasher>(&self, state: &mut H) {
-        let ser = self.serialize();
-        ser.hash(state);
-    }
-}
-
 /// Opaque data structure that holds a keypair consisting of a secret and a public key.
 ///
 /// # Serde support
@@ -779,9 +742,10 @@ impl core::hash::Hash for PublicKey {
 /// ```
 /// [`bincode`]: https://docs.rs/bincode
 /// [`cbor`]: https://docs.rs/cbor
-#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
+#[derive(Copy, Clone, PartialOrd, Ord, PartialEq, Eq, Hash)]
 pub struct KeyPair(ffi::KeyPair);
 impl_display_secret!(KeyPair);
+impl_fast_comparisons!(KeyPair);
 
 impl KeyPair {
     /// Obtains a raw const pointer suitable for use with FFI functions.
@@ -1091,8 +1055,9 @@ impl CPtr for KeyPair {
 /// ```
 /// [`bincode`]: https://docs.rs/bincode
 /// [`cbor`]: https://docs.rs/cbor
-#[derive(Copy, Clone, PartialEq, Eq, Debug, PartialOrd, Ord, Hash)]
+#[derive(Copy, Clone, Debug, PartialOrd, Ord, PartialEq, Eq, Hash)]
 pub struct XOnlyPublicKey(ffi::XOnlyPublicKey);
+impl_fast_comparisons!(XOnlyPublicKey);
 
 impl fmt::LowerHex for XOnlyPublicKey {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {

src/macros.rs

@@ -28,6 +28,8 @@ macro_rules! impl_array_newtype {
             }
         }
 
+        // We cannot derive these traits because Rust 1.41.1 requires `std::array::LengthAtMost32`.
+
         impl PartialEq for $thing {
             #[inline]
             fn eq(&self, other: &$thing) -> bool {
@@ -123,3 +125,28 @@ macro_rules! write_err {
         }
     }
 }
+
+/// Implements fast unstable comparison methods for `$ty`.
+macro_rules! impl_fast_comparisons {
+    ($ty:ident) => {
+        impl $ty {
+            /// Like `cmp::Cmp` but faster and with no guarantees across library versions.
+            ///
+            /// The `Cmp` implementation for FFI types is stable but slow because it first
+            /// serializes `self` and `other` before comparing them. This function provides a faster
+            /// comparison if you know that your types come from the same library version.
+            pub fn cmp_fast_unstable(&self, other: &Self) -> core::cmp::Ordering {
+                self.0.cmp_fast_unstable(&other.0)
+            }
+
+            /// Like `cmp::Eq` but faster and with no guarantees across library versions.
+            ///
+            /// The `Eq` implementation for FFI types is stable but slow because it first serializes
+            /// `self` and `other` before comparing them. This function provides a faster equality
+            /// check if you know that your types come from the same library version.
+            pub fn eq_fast_unstable(&self, other: &Self) -> bool {
+                self.0.eq_fast_unstable(&other.0)
+            }
+        }
+    }
+}