Skip to content

Commit b38ae97

Browse files
tchardingtcharding
committed
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`.
1 parent 630fc1f commit b38ae97

File tree

6 files changed

+324
-43
lines changed

6 files changed

+324
-43
lines changed

‎secp256k1-sys/src/lib.rs

Lines changed: 202 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,59 @@ impl PublicKey {
169169
pub fn underlying_bytes(self) -> [c_uchar; 64] {
170170
self.0
171171
}
172+
173+
/// Serializes this public key as a byte-encoded pair of values, in compressed form.
174+
fn serialize(&self) -> [u8; 33] {
175+
let mut buf = [0u8; 33];
176+
let mut len = 33;
177+
unsafe {
178+
let ret = secp256k1_ec_pubkey_serialize(
179+
secp256k1_context_no_precomp,
180+
buf.as_mut_c_ptr(),
181+
&mut len,
182+
self,
183+
SECP256K1_SER_COMPRESSED,
184+
);
185+
debug_assert_eq!(ret, 1);
186+
debug_assert_eq!(len, 33);
187+
};
188+
buf
189+
}
190+
}
191+
192+
#[cfg(not(fuzzing))]
193+
impl PartialOrd for PublicKey {
194+
fn partial_cmp(&self, other: &PublicKey) -> Option<core::cmp::Ordering> {
195+
Some(self.cmp(other))
196+
}
197+
}
198+
199+
#[cfg(not(fuzzing))]
200+
impl Ord for PublicKey {
201+
fn cmp(&self, other: &PublicKey) -> core::cmp::Ordering {
202+
let ret = unsafe {
203+
secp256k1_ec_pubkey_cmp(secp256k1_context_no_precomp, self, other)
204+
};
205+
ret.cmp(&0i32)
206+
}
207+
}
208+
209+
#[cfg(not(fuzzing))]
210+
impl PartialEq for PublicKey {
211+
fn eq(&self, other: &Self) -> bool {
212+
self.cmp(other) == core::cmp::Ordering::Equal
213+
}
214+
}
215+
216+
#[cfg(not(fuzzing))]
217+
impl Eq for PublicKey {}
218+
219+
#[cfg(not(fuzzing))]
220+
impl core::hash::Hash for PublicKey {
221+
fn hash<H: core::hash::Hasher>(&self, state: &mut H) {
222+
let ser = self.serialize();
223+
ser.hash(state);
224+
}
172225
}
173226

174227
/// Library-internal representation of a Secp256k1 signature
@@ -209,6 +262,54 @@ impl Signature {
209262
pub fn underlying_bytes(self) -> [c_uchar; 64] {
210263
self.0
211264
}
265+
266+
/// Serializes the signature in compact format.
267+
fn serialize(&self) -> [u8; 64] {
268+
let mut buf = [0u8; 64];
269+
unsafe {
270+
let ret = secp256k1_ecdsa_signature_serialize_compact(
271+
secp256k1_context_no_precomp,
272+
buf.as_mut_c_ptr(),
273+
self,
274+
);
275+
debug_assert!(ret == 1);
276+
}
277+
buf
278+
}
279+
}
280+
281+
#[cfg(not(fuzzing))]
282+
impl PartialOrd for Signature {
283+
fn partial_cmp(&self, other: &Signature) -> Option<core::cmp::Ordering> {
284+
Some(self.cmp(other))
285+
}
286+
}
287+
288+
#[cfg(not(fuzzing))]
289+
impl Ord for Signature {
290+
fn cmp(&self, other: &Signature) -> core::cmp::Ordering {
291+
let this = self.serialize();
292+
let that = other.serialize();
293+
this.cmp(&that)
294+
}
295+
}
296+
297+
#[cfg(not(fuzzing))]
298+
impl PartialEq for Signature {
299+
fn eq(&self, other: &Self) -> bool {
300+
self.cmp(other) == core::cmp::Ordering::Equal
301+
}
302+
}
303+
304+
#[cfg(not(fuzzing))]
305+
impl Eq for Signature {}
306+
307+
#[cfg(not(fuzzing))]
308+
impl core::hash::Hash for Signature {
309+
fn hash<H: core::hash::Hasher>(&self, state: &mut H) {
310+
let ser = self.serialize();
311+
ser.hash(state);
312+
}
212313
}
213314

214315
#[repr(C)]
@@ -248,6 +349,55 @@ impl XOnlyPublicKey {
248349
pub fn underlying_bytes(self) -> [c_uchar; 64] {
249350
self.0
250351
}
352+
353+
/// Serializes this key as a byte-encoded x coordinate value (32 bytes).
354+
fn serialize(&self) -> [u8; 32] {
355+
let mut buf = [0u8; 32];
356+
unsafe {
357+
let ret = secp256k1_xonly_pubkey_serialize(
358+
secp256k1_context_no_precomp,
359+
buf.as_mut_c_ptr(),
360+
self,
361+
);
362+
assert_eq!(ret, 1);
363+
};
364+
buf
365+
}
366+
}
367+
368+
#[cfg(not(fuzzing))]
369+
impl PartialOrd for XOnlyPublicKey {
370+
fn partial_cmp(&self, other: &XOnlyPublicKey) -> Option<core::cmp::Ordering> {
371+
Some(self.cmp(other))
372+
}
373+
}
374+
375+
#[cfg(not(fuzzing))]
376+
impl Ord for XOnlyPublicKey {
377+
fn cmp(&self, other: &XOnlyPublicKey) -> core::cmp::Ordering {
378+
let ret = unsafe {
379+
secp256k1_xonly_pubkey_cmp(secp256k1_context_no_precomp, self, other)
380+
};
381+
ret.cmp(&0i32)
382+
}
383+
}
384+
385+
#[cfg(not(fuzzing))]
386+
impl PartialEq for XOnlyPublicKey {
387+
fn eq(&self, other: &Self) -> bool {
388+
self.cmp(other) == core::cmp::Ordering::Equal
389+
}
390+
}
391+
392+
#[cfg(not(fuzzing))]
393+
impl Eq for XOnlyPublicKey {}
394+
395+
#[cfg(not(fuzzing))]
396+
impl core::hash::Hash for XOnlyPublicKey {
397+
fn hash<H: core::hash::Hasher>(&self, state: &mut H) {
398+
let ser = self.serialize();
399+
ser.hash(state);
400+
}
251401
}
252402

253403
#[repr(C)]
@@ -287,6 +437,58 @@ impl KeyPair {
287437
pub fn underlying_bytes(self) -> [c_uchar; 96] {
288438
self.0
289439
}
440+
441+
/// Creates a new compressed public key from this key pair.
442+
fn public_key(&self) -> PublicKey {
443+
unsafe {
444+
let mut pk = PublicKey::new();
445+
let ret = secp256k1_keypair_pub(
446+
secp256k1_context_no_precomp,
447+
&mut pk,
448+
self,
449+
);
450+
debug_assert_eq!(ret, 1);
451+
pk
452+
}
453+
}
454+
}
455+
456+
#[cfg(not(fuzzing))]
457+
impl PartialOrd for KeyPair {
458+
fn partial_cmp(&self, other: &KeyPair) -> Option<core::cmp::Ordering> {
459+
Some(self.cmp(other))
460+
}
461+
}
462+
463+
#[cfg(not(fuzzing))]
464+
impl Ord for KeyPair {
465+
fn cmp(&self, other: &KeyPair) -> core::cmp::Ordering {
466+
let this = self.public_key();
467+
let that = other.public_key();
468+
this.cmp(&that)
469+
}
470+
}
471+
472+
#[cfg(not(fuzzing))]
473+
impl PartialEq for KeyPair {
474+
fn eq(&self, other: &Self) -> bool {
475+
self.cmp(other) == core::cmp::Ordering::Equal
476+
}
477+
}
478+
479+
#[cfg(not(fuzzing))]
480+
impl Eq for KeyPair {}
481+
482+
#[cfg(not(fuzzing))]
483+
impl core::hash::Hash for KeyPair {
484+
fn hash<H: core::hash::Hasher>(&self, state: &mut H) {
485+
// To hash the key pair we just hash the serialized public key. Since any change to the
486+
// secret key would also be a change to the public key this is a valid one way function from
487+
// the key pair to the digest.
488+
let pk = self.public_key();
489+
let ser = pk.serialize();
490+
ser.hash(state);
491+
}
290492
}
291493

292494
extern "C" {

‎secp256k1-sys/src/macros.rs

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,34 @@ macro_rules! impl_array_newtype {
1919
($thing:ident, $ty:ty, $len:expr) => {
2020
impl Copy for $thing {}
2121

22+
impl $thing {
23+
/// Like `cmp::Ord` but faster and with no guarantees across library versions.
24+
///
25+
/// The inner byte array of `Self` is passed across the FFI boundry, as such there are
26+
/// no guarantees on its layout and it is subject to change across library versions,
27+
/// even minor versions. For this reason comparison function implementations (e.g.
28+
/// `Ord`, `PartialEq`) take measures to ensure the data will remain constant (e.g., by
29+
/// serializing it to a guaranteed format). This means they may be slow, this function
30+
/// provides a faster comparison if you know that your types come from the same library
31+
/// version.
32+
pub fn cmp_fast_unstable(&self, other: &Self) -> core::cmp::Ordering {
33+
self[..].cmp(&other[..])
34+
}
35+
36+
/// Like `cmp::Eq` but faster and with no guarantees across library versions.
37+
///
38+
/// The inner byte array of `Self` is passed across the FFI boundry, as such there are
39+
/// no guarantees on its layout and it is subject to change across library versions,
40+
/// even minor versions. For this reason comparison function implementations (e.g.
41+
/// `Ord`, `PartialEq`) take measures to ensure the data will remain constant (e.g., by
42+
/// serializing it to a guaranteed format). This means they may be slow, this function
43+
/// provides a faster equality check if you know that your types come from the same
44+
/// library version.
45+
pub fn eq_fast_unstable(&self, other: &Self) -> bool {
46+
self[..].eq(&other[..])
47+
}
48+
}
49+
2250
impl AsRef<[$ty; $len]> for $thing {
2351
#[inline]
2452
/// Gets a reference to the underlying array
@@ -28,28 +56,35 @@ macro_rules! impl_array_newtype {
2856
}
2957
}
3058

59+
// We cannot derive these traits because Rust 1.41.1 requires `std::array::LengthAtMost32`.
60+
61+
#[cfg(fuzzing)]
3162
impl PartialEq for $thing {
3263
#[inline]
3364
fn eq(&self, other: &$thing) -> bool {
3465
&self[..] == &other[..]
3566
}
3667
}
3768

69+
#[cfg(fuzzing)]
3870
impl Eq for $thing {}
3971

72+
#[cfg(fuzzing)]
4073
impl core::hash::Hash for $thing {
4174
fn hash<H: core::hash::Hasher>(&self, state: &mut H) {
4275
(&self[..]).hash(state)
4376
}
4477
}
4578

79+
#[cfg(fuzzing)]
4680
impl PartialOrd for $thing {
4781
#[inline]
4882
fn partial_cmp(&self, other: &$thing) -> Option<core::cmp::Ordering> {
4983
self[..].partial_cmp(&other[..])
5084
}
5185
}
5286

87+
#[cfg(fuzzing)]
5388
impl Ord for $thing {
5489
#[inline]
5590
fn cmp(&self, other: &$thing) -> core::cmp::Ordering {

‎secp256k1-sys/src/recovery.rs

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515

1616
//! # FFI of the recovery module
1717
18-
use crate::{Context, Signature, NonceFn, PublicKey, CPtr, impl_array_newtype};
18+
use crate::{Context, Signature, NonceFn, PublicKey, CPtr, impl_array_newtype, secp256k1_context_no_precomp};
1919
use crate::types::*;
2020
use core::fmt;
2121

@@ -27,6 +27,23 @@ impl_array_newtype!(RecoverableSignature, c_uchar, 65);
2727
impl RecoverableSignature {
2828
/// Create a new (zeroed) signature usable for the FFI interface
2929
pub fn new() -> RecoverableSignature { RecoverableSignature([0; 65]) }
30+
31+
/// Serializes the signature in compact format.
32+
fn serialize(&self) -> [u8; 65] {
33+
let mut buf = [0u8; 65];
34+
let mut recid = 0;
35+
unsafe {
36+
let ret = secp256k1_ecdsa_recoverable_signature_serialize_compact(
37+
secp256k1_context_no_precomp,
38+
buf.as_mut_c_ptr(),
39+
&mut recid,
40+
self,
41+
);
42+
debug_assert!(ret == 1);
43+
}
44+
buf[64] = (recid & 0xFF) as u8;
45+
buf
46+
}
3047
}
3148

3249
impl Default for RecoverableSignature {
@@ -59,6 +76,40 @@ impl fmt::Debug for RecoverableSignature {
5976
}
6077
}
6178

79+
#[cfg(not(fuzzing))]
80+
impl PartialOrd for RecoverableSignature {
81+
fn partial_cmp(&self, other: &RecoverableSignature) -> Option<core::cmp::Ordering> {
82+
Some(self.cmp(other))
83+
}
84+
}
85+
86+
#[cfg(not(fuzzing))]
87+
impl Ord for RecoverableSignature {
88+
fn cmp(&self, other: &RecoverableSignature) -> core::cmp::Ordering {
89+
let this = self.serialize();
90+
let that = other.serialize();
91+
this.cmp(&that)
92+
}
93+
}
94+
95+
#[cfg(not(fuzzing))]
96+
impl PartialEq for RecoverableSignature {
97+
fn eq(&self, other: &Self) -> bool {
98+
self.cmp(other) == core::cmp::Ordering::Equal
99+
}
100+
}
101+
102+
#[cfg(not(fuzzing))]
103+
impl Eq for RecoverableSignature {}
104+
105+
#[cfg(not(fuzzing))]
106+
impl core::hash::Hash for RecoverableSignature {
107+
fn hash<H: core::hash::Hasher>(&self, state: &mut H) {
108+
let ser = self.serialize();
109+
ser.hash(state);
110+
}
111+
}
112+
62113
extern "C" {
63114
#[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_6_1_ecdsa_recoverable_signature_parse_compact")]
64115
pub fn secp256k1_ecdsa_recoverable_signature_parse_compact(cx: *const Context, sig: *mut RecoverableSignature,

‎src/ecdsa/mod.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,9 @@ use crate::{
1818
};
1919

2020
/// An ECDSA signature
21-
#[derive(Copy, Clone, PartialEq, Eq, Hash)]
21+
#[derive(Copy, Clone, PartialOrd, Ord, PartialEq, Eq, Hash)]
2222
pub struct Signature(pub(crate) ffi::Signature);
23+
impl_fast_comparisons!(Signature);
2324

2425
impl fmt::Debug for Signature {
2526
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Display::fmt(self, f) }

0 commit comments

Comments
 (0)