Merge #518: Make comparison functions stable

9850550 Move AsRef impl block next to Index (Tobin C. Harding)
4d42e8e Derive Copy and Clone (Tobin C. Harding)
b38ae97 Implement stable comparison functionality (Tobin C. Harding)
630fc1f Remove len and is_empty from impl_array_newtype macros (Tobin C. Harding)
9788b6d Remove leading colons from impl_array_newtype methods (Tobin C. Harding)
2bb08c2 Remove as_[mut_]ptr from impl_array_newtype macros (Tobin C. Harding)
3e28070 Duplicate impl_array_newtype (Tobin C. Harding)
6358903 Add newline to end of file (Tobin C. Harding)

Pull request description:

  Supersedes #515

  The primary aim of this PR is to fix the fact that we currently implement various comparison traits (`Ord`, `PartialEq`) by comparing the inner byte arrays. These bytes are meant to be opaque and are not guaranteed across versions of `libsecp256k1`.

  This PR is a bit involved because I had to detangle all the various types (across both `secp256k1` and `secp256k1-sys`) that use the `impl_array_newtype` macro.

  - Patch 1: is trivial cleanup
  - Patch 2: Step one of the PR is duplicating the macro into both crates so we can tease them apart.
  - Patch 3-5: Are cleanup of the now duplicated `impl_array_newtype` macros
  - Patch 6: Is the meat and potatoes
  - Patch 7,8: Further minor clean ups to the macro

  I had a lot of fun with this PR, I hope you enjoy it too.

  Fix: #463

ACKs for top commit:
  apoelstra:
    ACK 9850550

Tree-SHA512: 160381e53972ff882ceb1d2d47bac56a7301a2d13bfe75d3f6ff658ab2c6fbe516ad856587c4d23f98524205918ca1a5f9b737e35c23c7a01b476c92d8d1792f

secp256k1-sys/src/lib.rs

@@ -133,6 +133,7 @@ impl SchnorrSigExtraParams {
 
 /// Library-internal representation of a Secp256k1 public key
 #[repr(C)]
+#[derive(Copy, Clone)]
 pub struct PublicKey([c_uchar; 64]);
 impl_array_newtype!(PublicKey, c_uchar, 64);
 impl_raw_debug!(PublicKey);
@@ -169,10 +170,64 @@ 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
 #[repr(C)]
+#[derive(Copy, Clone)]
 pub struct Signature([c_uchar; 64]);
 impl_array_newtype!(Signature, c_uchar, 64);
 impl_raw_debug!(Signature);
@@ -209,9 +264,58 @@ 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)]
+#[derive(Copy, Clone)]
 pub struct XOnlyPublicKey([c_uchar; 64]);
 impl_array_newtype!(XOnlyPublicKey, c_uchar, 64);
 impl_raw_debug!(XOnlyPublicKey);
@@ -248,9 +352,59 @@ 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)]
+#[derive(Copy, Clone)]
 pub struct KeyPair([c_uchar; 96]);
 impl_array_newtype!(KeyPair, c_uchar, 96);
 impl_raw_debug!(KeyPair);
@@ -287,6 +441,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

@@ -17,75 +17,76 @@
 #[macro_export]
 macro_rules! impl_array_newtype {
     ($thing:ident, $ty:ty, $len:expr) => {
-        impl Copy for $thing {}
-
         impl $thing {
-            /// Converts the object to a raw pointer for FFI interfacing.
-            #[inline]
-            pub fn as_ptr(&self) -> *const $ty {
-                let &$thing(ref dat) = self;
-                dat.as_ptr()
+            /// 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[..])
             }
 
-            /// Converts the object to a mutable raw pointer for FFI interfacing.
-            #[inline]
-            pub fn as_mut_ptr(&mut self) -> *mut $ty {
-                let &mut $thing(ref mut dat) = self;
-                dat.as_mut_ptr()
+            /// 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[..])
             }
-
-            /// Returns the length of the object as an array.
-            #[inline]
-            pub fn len(&self) -> usize { $len }
-
-            /// Returns whether the object as an array is empty.
-            #[inline]
-            pub fn is_empty(&self) -> bool { false }
         }
 
-        impl AsRef<[$ty; $len]> for $thing {
-            #[inline]
-            /// Gets a reference to the underlying array
-            fn as_ref(&self) -> &[$ty; $len] {
-                let &$thing(ref dat) = self;
-                dat
-            }
-        }
+        // 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 {
                 &self[..] == &other[..]
             }
         }
 
+        #[cfg(fuzzing)]
         impl Eq for $thing {}
 
-        impl ::core::hash::Hash for $thing {
-            fn hash<H: ::core::hash::Hasher>(&self, state: &mut H) {
+        #[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> {
                 self[..].partial_cmp(&other[..])
             }
         }
 
+        #[cfg(fuzzing)]
         impl Ord for $thing {
             #[inline]
             fn cmp(&self, other: &$thing) -> core::cmp::Ordering {
                 self[..].cmp(&other[..])
             }
         }
 
-        impl Clone for $thing {
+        impl AsRef<[$ty; $len]> for $thing {
             #[inline]
-            fn clone(&self) -> $thing {
+            /// Gets a reference to the underlying array
+            fn as_ref(&self) -> &[$ty; $len] {
                 let &$thing(ref dat) = self;
-                $thing(dat.clone())
+                dat
             }
         }
 
@@ -101,20 +102,15 @@ macro_rules! impl_array_newtype {
 
         impl $crate::CPtr for $thing {
             type Target = $ty;
+
             fn as_c_ptr(&self) -> *const Self::Target {
-                if self.is_empty() {
-                    core::ptr::null()
-                } else {
-                    self.as_ptr()
-                }
+                let &$thing(ref dat) = self;
+                dat.as_ptr()
             }
 
             fn as_mut_c_ptr(&mut self) -> *mut Self::Target {
-                if self.is_empty() {
-                    core::ptr::null::<Self::Target>() as *mut _
-                } else {
-                    self.as_mut_ptr()
-                }
+                let &mut $thing(ref mut dat) = self;
+                dat.as_mut_ptr()
             }
         }
     }