From 635890322a4196d889c47c8a12e8f53ec36e322f Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 18 Nov 2022 07:56:54 +1100 Subject: [PATCH 1/8] Add newline to end of file Inline with UNIX convention add a trailing newline to file. --- secp256k1-sys/src/recovery.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/secp256k1-sys/src/recovery.rs b/secp256k1-sys/src/recovery.rs index c4cf20a0e..a5c7fa411 100644 --- a/secp256k1-sys/src/recovery.rs +++ b/secp256k1-sys/src/recovery.rs @@ -169,4 +169,4 @@ mod fuzz_dummy { } #[cfg(fuzzing)] -pub use self::fuzz_dummy::*; \ No newline at end of file +pub use self::fuzz_dummy::*; From 3e2807018764740d273befc81f53d18228cb39dd Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 17 Nov 2022 10:33:38 +1100 Subject: [PATCH 2/8] Duplicate impl_array_newtype The two crates `secp256k1` and `secp256k1-sys` serve very different purposes, having a macro defined in one that is used in both makes it hard to get nuanced things correct in the macro, for example the comparison implementations (`Ord`, `PartialEq` etc.) are semantically different in each crate. In an effort to decouple `secp256k1` and `secp256k1-sys` duplicate the `impl_array_newtype` macro. --- src/key.rs | 4 +- src/lib.rs | 2 +- src/macros.rs | 107 +++++++++++++++++++++++++++++++++++++++++++++++++ src/schnorr.rs | 4 +- 4 files changed, 112 insertions(+), 5 deletions(-) diff --git a/src/key.rs b/src/key.rs index 8832c9968..6b67c678b 100644 --- a/src/key.rs +++ b/src/key.rs @@ -24,11 +24,11 @@ use core::{fmt, ptr, str}; use serde::ser::SerializeTuple; use crate::ffi::types::c_uint; -use crate::ffi::{self, impl_array_newtype, CPtr}; +use crate::ffi::{self, CPtr}; #[cfg(all(feature = "global-context", feature = "rand-std"))] use crate::schnorr; use crate::Error::{self, InvalidPublicKey, InvalidPublicKeySum, InvalidSecretKey}; -use crate::{constants, from_hex, Scalar, Secp256k1, Signing, Verification}; +use crate::{constants, from_hex, impl_array_newtype, Scalar, Secp256k1, Signing, Verification}; #[cfg(feature = "global-context")] use crate::{ecdsa, Message, SECP256K1}; #[cfg(feature = "bitcoin-hashes")] diff --git a/src/lib.rs b/src/lib.rs index db3faa3ac..6a334d53e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -200,7 +200,7 @@ pub use serde; pub use crate::context::*; use crate::ffi::types::AlignedType; -use crate::ffi::{impl_array_newtype, CPtr}; +use crate::ffi::CPtr; #[cfg(feature = "bitcoin-hashes")] use crate::hashes::Hash; pub use crate::key::{PublicKey, SecretKey, *}; diff --git a/src/macros.rs b/src/macros.rs index d2da41bde..fbeb6b78b 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -13,6 +13,113 @@ // If not, see . // +/// Implement methods and traits for types that contain an inner array. +#[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() + } + + /// 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() + } + + /// 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 + } + } + + impl PartialEq for $thing { + #[inline] + fn eq(&self, other: &$thing) -> bool { + &self[..] == &other[..] + } + } + + impl Eq for $thing {} + + impl ::core::hash::Hash for $thing { + fn hash(&self, state: &mut H) { + (&self[..]).hash(state) + } + } + + impl PartialOrd for $thing { + #[inline] + fn partial_cmp(&self, other: &$thing) -> Option { + self[..].partial_cmp(&other[..]) + } + } + + impl Ord for $thing { + #[inline] + fn cmp(&self, other: &$thing) -> core::cmp::Ordering { + self[..].cmp(&other[..]) + } + } + + impl Clone for $thing { + #[inline] + fn clone(&self) -> $thing { + let &$thing(ref dat) = self; + $thing(dat.clone()) + } + } + + impl core::ops::Index for $thing + where + [$ty]: core::ops::Index, + { + type Output = <[$ty] as core::ops::Index>::Output; + + #[inline] + fn index(&self, index: I) -> &Self::Output { &self.0[index] } + } + + 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() + } + } + + fn as_mut_c_ptr(&mut self) -> *mut Self::Target { + if self.is_empty() { + core::ptr::null::() as *mut _ + } else { + self.as_mut_ptr() + } + } + } + } +} + macro_rules! impl_pretty_debug { ($thing:ident) => { impl core::fmt::Debug for $thing { diff --git a/src/schnorr.rs b/src/schnorr.rs index 0d001e7e8..2d33ce902 100644 --- a/src/schnorr.rs +++ b/src/schnorr.rs @@ -7,11 +7,11 @@ use core::{fmt, ptr, str}; #[cfg(any(test, feature = "rand"))] use rand::{CryptoRng, Rng}; -use crate::ffi::{self, impl_array_newtype, CPtr}; +use crate::ffi::{self, CPtr}; use crate::key::{KeyPair, XOnlyPublicKey}; #[cfg(all(feature = "global-context", feature = "rand-std"))] use crate::SECP256K1; -use crate::{constants, from_hex, Error, Message, Secp256k1, Signing, Verification}; +use crate::{constants, from_hex, impl_array_newtype, Error, Message, Secp256k1, Signing, Verification}; /// Represents a Schnorr signature. pub struct Signature([u8; constants::SCHNORR_SIGNATURE_SIZE]); From 2bb08c21e5b8f360929ac6d16401285ac1aa26a9 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 17 Nov 2022 10:52:36 +1100 Subject: [PATCH 3/8] Remove as_[mut_]ptr from impl_array_newtype macros For interfacing with the FFI layer we implement `ffi::CPtr`, there is not need to provide methods `as_ptr` and `as_mut_ptr` as well. --- secp256k1-sys/src/macros.rs | 29 +++++------------------------ src/ecdh.rs | 2 +- src/macros.rs | 31 ++++++------------------------- 3 files changed, 12 insertions(+), 50 deletions(-) diff --git a/secp256k1-sys/src/macros.rs b/secp256k1-sys/src/macros.rs index e3850f01e..6d2a0c30c 100644 --- a/secp256k1-sys/src/macros.rs +++ b/secp256k1-sys/src/macros.rs @@ -20,20 +20,6 @@ macro_rules! impl_array_newtype { 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() - } - - /// 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() - } - /// Returns the length of the object as an array. #[inline] pub fn len(&self) -> usize { $len } @@ -101,20 +87,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::() as *mut _ - } else { - self.as_mut_ptr() - } + let &mut $thing(ref mut dat) = self; + dat.as_mut_ptr() } } } diff --git a/src/ecdh.rs b/src/ecdh.rs index c93a5e641..31e703938 100644 --- a/src/ecdh.rs +++ b/src/ecdh.rs @@ -146,7 +146,7 @@ pub fn shared_secret_point(point: &PublicKey, scalar: &SecretKey) -> [u8; 64] { ffi::secp256k1_context_no_precomp, xy.as_mut_ptr(), point.as_c_ptr(), - scalar.as_ptr(), + scalar.as_c_ptr(), Some(c_callback), ptr::null_mut(), ) diff --git a/src/macros.rs b/src/macros.rs index fbeb6b78b..c028e2362 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -20,20 +20,6 @@ macro_rules! impl_array_newtype { 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() - } - - /// 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() - } - /// Returns the length of the object as an array. #[inline] pub fn len(&self) -> usize { $len } @@ -99,22 +85,17 @@ macro_rules! impl_array_newtype { fn index(&self, index: I) -> &Self::Output { &self.0[index] } } - impl $crate::CPtr for $thing { + impl $crate::ffi::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::() as *mut _ - } else { - self.as_mut_ptr() - } + let &mut $thing(ref mut dat) = self; + dat.as_mut_ptr() } } } From 9788b6df880cc86787122a54fa3a546a05cb6d82 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 17 Nov 2022 10:54:06 +1100 Subject: [PATCH 4/8] Remove leading colons from impl_array_newtype methods The leading colons are an artifact of Rust 1.29, remove them. --- secp256k1-sys/src/macros.rs | 4 ++-- src/macros.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/secp256k1-sys/src/macros.rs b/secp256k1-sys/src/macros.rs index 6d2a0c30c..a3dd1b191 100644 --- a/secp256k1-sys/src/macros.rs +++ b/secp256k1-sys/src/macros.rs @@ -47,8 +47,8 @@ macro_rules! impl_array_newtype { impl Eq for $thing {} - impl ::core::hash::Hash for $thing { - fn hash(&self, state: &mut H) { + impl core::hash::Hash for $thing { + fn hash(&self, state: &mut H) { (&self[..]).hash(state) } } diff --git a/src/macros.rs b/src/macros.rs index c028e2362..8245cda1d 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -47,8 +47,8 @@ macro_rules! impl_array_newtype { impl Eq for $thing {} - impl ::core::hash::Hash for $thing { - fn hash(&self, state: &mut H) { + impl core::hash::Hash for $thing { + fn hash(&self, state: &mut H) { (&self[..]).hash(state) } } From 630fc1fcb6d8dd9e3f454859ba971fad5cfd2e79 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 17 Nov 2022 11:00:03 +1100 Subject: [PATCH 5/8] Remove len and is_empty from impl_array_newtype macros An array in Rust has no concept of length, it is a fixed size data type. Equally an array cannot be "empty", again since it is a fixed size data type. These are methods/concepts seen in slices and vectors. Remove the `len` and `is_empty` methods. --- secp256k1-sys/src/macros.rs | 10 ---------- src/macros.rs | 10 ---------- 2 files changed, 20 deletions(-) diff --git a/secp256k1-sys/src/macros.rs b/secp256k1-sys/src/macros.rs index a3dd1b191..761ab940e 100644 --- a/secp256k1-sys/src/macros.rs +++ b/secp256k1-sys/src/macros.rs @@ -19,16 +19,6 @@ macro_rules! impl_array_newtype { ($thing:ident, $ty:ty, $len:expr) => { impl Copy for $thing {} - impl $thing { - /// 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 diff --git a/src/macros.rs b/src/macros.rs index 8245cda1d..b457c0246 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -19,16 +19,6 @@ macro_rules! impl_array_newtype { ($thing:ident, $ty:ty, $len:expr) => { impl Copy for $thing {} - impl $thing { - /// 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 From b38ae97eafe8d07612bb35e59bbc4eeafea834ec Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 17 Nov 2022 15:56:35 +1100 Subject: [PATCH 6/8] 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 | 202 ++++++++++++++++++++++++++++++++++ secp256k1-sys/src/macros.rs | 35 ++++++ secp256k1-sys/src/recovery.rs | 53 ++++++++- src/ecdsa/mod.rs | 3 +- src/key.rs | 47 +------- src/macros.rs | 27 +++++ 6 files changed, 324 insertions(+), 43 deletions(-) diff --git a/secp256k1-sys/src/lib.rs b/secp256k1-sys/src/lib.rs index c5fc38348..f92535e13 100644 --- a/secp256k1-sys/src/lib.rs +++ b/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 { + 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(&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 { + 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(&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 { + 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(&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 { + 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(&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" { diff --git a/secp256k1-sys/src/macros.rs b/secp256k1-sys/src/macros.rs index 761ab940e..247ee1200 100644 --- a/secp256k1-sys/src/macros.rs +++ b/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(&self, state: &mut H) { (&self[..]).hash(state) } } + #[cfg(fuzzing)] impl PartialOrd for $thing { #[inline] fn partial_cmp(&self, other: &$thing) -> Option { @@ -50,6 +84,7 @@ macro_rules! impl_array_newtype { } } + #[cfg(fuzzing)] impl Ord for $thing { #[inline] fn cmp(&self, other: &$thing) -> core::cmp::Ordering { diff --git a/secp256k1-sys/src/recovery.rs b/secp256k1-sys/src/recovery.rs index a5c7fa411..7a8911254 100644 --- a/secp256k1-sys/src/recovery.rs +++ b/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 { + 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(&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, diff --git a/src/ecdsa/mod.rs b/src/ecdsa/mod.rs index 6e3256ff0..d44126a5e 100644 --- a/src/ecdsa/mod.rs +++ b/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) } diff --git a/src/key.rs b/src/key.rs index 6b67c678b..910385bb6 100644 --- a/src/key.rs +++ b/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)] -#[cfg_attr(fuzzing, derive(PartialOrd, Ord, PartialEq, Eq, Hash))] +#[derive(Copy, Clone, Debug, 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 { - 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(&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 { diff --git a/src/macros.rs b/src/macros.rs index b457c0246..11ce90e91 100644 --- a/src/macros.rs +++ b/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) + } + } + } +} From 4d42e8e906a853c0254c02dc163c1437593eda30 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 18 Nov 2022 10:56:24 +1100 Subject: [PATCH 7/8] Derive Copy and Clone There is no obvious reason why not to derive `Copy` and `Clone` for types that use the `impl_newtype_macro`. Derives are less surprising so deriving makes the code marginally easier to read. --- secp256k1-sys/src/lib.rs | 4 ++++ secp256k1-sys/src/macros.rs | 10 ---------- secp256k1-sys/src/recovery.rs | 1 + src/key.rs | 1 + src/lib.rs | 1 + src/macros.rs | 9 --------- src/schnorr.rs | 1 + 7 files changed, 8 insertions(+), 19 deletions(-) diff --git a/secp256k1-sys/src/lib.rs b/secp256k1-sys/src/lib.rs index f92535e13..6239d0be1 100644 --- a/secp256k1-sys/src/lib.rs +++ b/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); @@ -226,6 +227,7 @@ impl core::hash::Hash for PublicKey { /// 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); @@ -313,6 +315,7 @@ impl core::hash::Hash for Signature { } #[repr(C)] +#[derive(Copy, Clone)] pub struct XOnlyPublicKey([c_uchar; 64]); impl_array_newtype!(XOnlyPublicKey, c_uchar, 64); impl_raw_debug!(XOnlyPublicKey); @@ -401,6 +404,7 @@ impl core::hash::Hash for XOnlyPublicKey { } #[repr(C)] +#[derive(Copy, Clone)] pub struct KeyPair([c_uchar; 96]); impl_array_newtype!(KeyPair, c_uchar, 96); impl_raw_debug!(KeyPair); diff --git a/secp256k1-sys/src/macros.rs b/secp256k1-sys/src/macros.rs index 247ee1200..98499ff76 100644 --- a/secp256k1-sys/src/macros.rs +++ b/secp256k1-sys/src/macros.rs @@ -17,8 +17,6 @@ #[macro_export] 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. /// @@ -92,14 +90,6 @@ macro_rules! impl_array_newtype { } } - impl Clone for $thing { - #[inline] - fn clone(&self) -> $thing { - let &$thing(ref dat) = self; - $thing(dat.clone()) - } - } - impl core::ops::Index for $thing where [$ty]: core::ops::Index, diff --git a/secp256k1-sys/src/recovery.rs b/secp256k1-sys/src/recovery.rs index 7a8911254..76f0180ad 100644 --- a/secp256k1-sys/src/recovery.rs +++ b/secp256k1-sys/src/recovery.rs @@ -21,6 +21,7 @@ use core::fmt; /// Library-internal representation of a Secp256k1 signature + recovery ID #[repr(C)] +#[derive(Copy, Clone)] pub struct RecoverableSignature([c_uchar; 65]); impl_array_newtype!(RecoverableSignature, c_uchar, 65); diff --git a/src/key.rs b/src/key.rs index 910385bb6..8c3fae5e2 100644 --- a/src/key.rs +++ b/src/key.rs @@ -56,6 +56,7 @@ use crate::{hashes, ThirtyTwoByteHash}; /// ``` /// [`bincode`]: https://docs.rs/bincode /// [`cbor`]: https://docs.rs/cbor +#[derive(Copy, Clone)] pub struct SecretKey([u8; constants::SECRET_KEY_SIZE]); impl_array_newtype!(SecretKey, u8, constants::SECRET_KEY_SIZE); impl_display_secret!(SecretKey); diff --git a/src/lib.rs b/src/lib.rs index 6a334d53e..d8c78626a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -233,6 +233,7 @@ impl ThirtyTwoByteHash for hashes::sha256t::Hash { } /// A (hashed) message input to an ECDSA signature. +#[derive(Copy, Clone)] pub struct Message([u8; constants::MESSAGE_SIZE]); impl_array_newtype!(Message, u8, constants::MESSAGE_SIZE); impl_pretty_debug!(Message); diff --git a/src/macros.rs b/src/macros.rs index 11ce90e91..1ed2bd135 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -17,7 +17,6 @@ #[macro_export] macro_rules! impl_array_newtype { ($thing:ident, $ty:ty, $len:expr) => { - impl Copy for $thing {} impl AsRef<[$ty; $len]> for $thing { #[inline] @@ -59,14 +58,6 @@ macro_rules! impl_array_newtype { } } - impl Clone for $thing { - #[inline] - fn clone(&self) -> $thing { - let &$thing(ref dat) = self; - $thing(dat.clone()) - } - } - impl core::ops::Index for $thing where [$ty]: core::ops::Index, diff --git a/src/schnorr.rs b/src/schnorr.rs index 2d33ce902..4c208a046 100644 --- a/src/schnorr.rs +++ b/src/schnorr.rs @@ -14,6 +14,7 @@ use crate::SECP256K1; use crate::{constants, from_hex, impl_array_newtype, Error, Message, Secp256k1, Signing, Verification}; /// Represents a Schnorr signature. +#[derive(Copy, Clone)] pub struct Signature([u8; constants::SCHNORR_SIGNATURE_SIZE]); impl_array_newtype!(Signature, u8, constants::SCHNORR_SIGNATURE_SIZE); impl_pretty_debug!(Signature); From 9850550734af82b31523c8d9d73190d14ab63fda Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 18 Nov 2022 10:57:32 +1100 Subject: [PATCH 8/8] Move AsRef impl block next to Index These two traits are related, in as much as they both give access to the inner byte array. Put them next to each other to assist clarity. --- secp256k1-sys/src/macros.rs | 18 +++++++++--------- src/macros.rs | 18 +++++++++--------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/secp256k1-sys/src/macros.rs b/secp256k1-sys/src/macros.rs index 98499ff76..35734fcce 100644 --- a/secp256k1-sys/src/macros.rs +++ b/secp256k1-sys/src/macros.rs @@ -45,15 +45,6 @@ macro_rules! impl_array_newtype { } } - 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)] @@ -90,6 +81,15 @@ macro_rules! impl_array_newtype { } } + 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 + } + } + impl core::ops::Index for $thing where [$ty]: core::ops::Index, diff --git a/src/macros.rs b/src/macros.rs index 1ed2bd135..e89e8d07d 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -18,15 +18,6 @@ macro_rules! impl_array_newtype { ($thing:ident, $ty:ty, $len:expr) => { - 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`. impl PartialEq for $thing { @@ -58,6 +49,15 @@ macro_rules! impl_array_newtype { } } + 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 + } + } + impl core::ops::Index for $thing where [$ty]: core::ops::Index,