Merge rust-bitcoin/rust-secp256k1#507: Minimise FFI in the public API

68c73850d8 Minimise FFI in the public API (Tobin C. Harding)

Pull request description:

  Normal users should never need to directly interact with the FFI layer.

  Audit and reduce the use of `ffi` types in the public API of various types. Leave only the implementation of `CPtr`, and document this clearly as not required by normal users. Done for:

  - PublicKey
  - XOnlyPublicKey
  - KeyPair
  - ecdsa::Signature
  - ecdsa::RecoverableSignature

ACKs for top commit:
  apoelstra:
    ACK 68c73850d8

Tree-SHA512: 8242527837872f9aba2aab19b02c2280ca1eb1dfd33c8ca619726d981811d72de3e5a57cbde2fbe621eb8e50e43f488804cd51d27949459da1c0ceb03fca35e3

secp256k1-sys/src/lib.rs

@@ -691,10 +691,12 @@ unsafe fn strlen(mut str_ptr: *const c_char) -> usize {
 }
 
 
-/// A trait for producing pointers that will always be valid in C. (assuming NULL pointer is a valid no-op)
+/// A trait for producing pointers that will always be valid in C (assuming NULL pointer is a valid
-/// Rust doesn't promise what pointers does it give to ZST (<https://doc.rust-lang.org/nomicon/exotic-sizes.html#zero-sized-types-zsts>)
+/// no-op).
-/// In case the type is empty this trait will give a NULL pointer, which should be handled in C.
 ///
+/// Rust does not guarantee pointers to Zero Sized Types
+/// (<https://doc.rust-lang.org/nomicon/exotic-sizes.html#zero-sized-types-zsts>). In case the type
+/// is empty this trait will return a NULL pointer, which should be handled in C.
 pub trait CPtr {
     type Target;
     fn as_c_ptr(&self) -> *const Self::Target;

src/ecdh.rs

@@ -150,7 +150,7 @@ pub fn shared_secret_point(point: &PublicKey, scalar: &SecretKey) -> [u8; 64] {
         ffi::secp256k1_ecdh(
             ffi::secp256k1_context_no_precomp,
             xy.as_mut_ptr(),
-            point.as_ptr(),
+            point.as_c_ptr(),
             scalar.as_ptr(),
             Some(c_callback),
             ptr::null_mut(),

src/ecdsa/mod.rs

@@ -144,14 +144,16 @@ impl Signature {
 
     /// Obtains a raw pointer suitable for use with FFI functions
     #[inline]
+    #[deprecated(since = "0.25.0", note = "Use Self::as_c_ptr if you need to access the FFI layer")]
     pub fn as_ptr(&self) -> *const ffi::Signature {
-        &self.0
+        self.as_c_ptr()
     }
 
     /// Obtains a raw mutable pointer suitable for use with FFI functions
     #[inline]
+    #[deprecated(since = "0.25.0", note = "Use Self::as_mut_c_ptr if you need to access the FFI layer")]
     pub fn as_mut_ptr(&mut self) -> *mut ffi::Signature {
-        &mut self.0
+        self.as_mut_c_ptr()
     }
 
     #[inline]
@@ -199,11 +201,11 @@ impl CPtr for Signature {
     type Target = ffi::Signature;
 
     fn as_c_ptr(&self) -> *const Self::Target {
-        self.as_ptr()
+        &self.0
     }
 
     fn as_mut_c_ptr(&mut self) -> *mut Self::Target {
-        self.as_mut_ptr()
+        &mut self.0
     }
 }
 
@@ -307,7 +309,7 @@ impl<C: Signing> Secp256k1<C> {
 
                     counter += 1;
                     extra_entropy[..4].copy_from_slice(&counter.to_le_bytes());
-                    entropy_p = extra_entropy.as_ptr().cast::<ffi::types::c_void>();
+                    entropy_p = extra_entropy.as_c_ptr().cast::<ffi::types::c_void>();
 
                     // When fuzzing, these checks will usually spinloop forever, so just short-circuit them.
                     #[cfg(fuzzing)]

src/ecdsa/recovery.rs

@@ -76,14 +76,16 @@ impl RecoverableSignature {
 
     /// Obtains a raw pointer suitable for use with FFI functions.
     #[inline]
+    #[deprecated(since = "0.25.0", note = "Use Self::as_c_ptr if you need to access the FFI layer")]
     pub fn as_ptr(&self) -> *const ffi::RecoverableSignature {
-        &self.0
+        self.as_c_ptr()
     }
 
     /// Obtains a raw mutable pointer suitable for use with FFI functions.
     #[inline]
+    #[deprecated(since = "0.25.0", note = "Use Self::as_mut_c_ptr if you need to access the FFI layer")]
     pub fn as_mut_ptr(&mut self) -> *mut ffi::RecoverableSignature {
-        &mut self.0
+        self.as_mut_c_ptr()
     }
 
     #[inline]
@@ -133,11 +135,11 @@ impl RecoverableSignature {
 impl CPtr for RecoverableSignature {
     type Target = ffi::RecoverableSignature;
     fn as_c_ptr(&self) -> *const Self::Target {
-        self.as_ptr()
+        &self.0
     }
 
     fn as_mut_c_ptr(&mut self) -> *mut Self::Target {
-        self.as_mut_ptr()
+        &mut self.0
     }
 }
 

src/key.rs

@@ -221,7 +221,7 @@ impl SecretKey {
             let ret = ffi::secp256k1_keypair_sec(
                 ffi::secp256k1_context_no_precomp,
                 sk.as_mut_c_ptr(),
-                keypair.as_ptr()
+                keypair.as_c_ptr()
             );
             debug_assert_eq!(ret, 1);
         }
@@ -395,14 +395,16 @@ impl<'de> serde::Deserialize<'de> for SecretKey {
 impl PublicKey {
     /// Obtains a raw const pointer suitable for use with FFI functions.
     #[inline]
+    #[deprecated(since = "0.25.0", note = "Use Self::as_c_ptr if you need to access the FFI layer")]
     pub fn as_ptr(&self) -> *const ffi::PublicKey {
-        &self.0
+        self.as_c_ptr()
     }
 
     /// Obtains a raw mutable pointer suitable for use with FFI functions.
     #[inline]
+    #[deprecated(since = "0.25.0", note = "Use Self::as_mut_c_ptr if you need to access the FFI layer")]
     pub fn as_mut_ptr(&mut self) -> *mut ffi::PublicKey {
-        &mut self.0
+        self.as_mut_c_ptr()
     }
 
     /// Creates a new public key from a [`SecretKey`].
@@ -479,7 +481,7 @@ impl PublicKey {
             let ret = ffi::secp256k1_keypair_pub(
                 ffi::secp256k1_context_no_precomp,
                 &mut pk,
-                keypair.as_ptr()
+                keypair.as_c_ptr()
             );
             debug_assert_eq!(ret, 1);
             PublicKey(pk)
@@ -666,7 +668,7 @@ impl PublicKey {
                 ffi::secp256k1_context_no_precomp,
                 &mut xonly_pk,
                 &mut pk_parity,
-                self.as_ptr(),
+                self.as_c_ptr(),
             );
             debug_assert_eq!(ret, 1);
             let parity = Parity::from_i32(pk_parity).expect("should not panic, pk_parity is 0 or 1");
@@ -676,19 +678,26 @@ impl PublicKey {
     }
 }
 
+/// This trait enables interaction with the FFI layer and even though it is part of the public API
+/// normal users should never need to directly interact with FFI types.
 impl CPtr for PublicKey {
     type Target = ffi::PublicKey;
+
+    /// Obtains a const pointer suitable for use with FFI functions.
     fn as_c_ptr(&self) -> *const Self::Target {
-        self.as_ptr()
+        &self.0
     }
 
+    /// Obtains a mutable pointer suitable for use with FFI functions.
     fn as_mut_c_ptr(&mut self) -> *mut Self::Target {
-        self.as_mut_ptr()
+        &mut self.0
     }
 }
 
 
-/// Creates a new public key from a FFI public key
+/// Creates a new public key from a FFI public key.
+///
+/// Note, normal users should never need to interact directly with FFI types.
 impl From<ffi::PublicKey> for PublicKey {
     #[inline]
     fn from(pk: ffi::PublicKey) -> PublicKey {
@@ -796,14 +805,16 @@ impl_display_secret!(KeyPair);
 impl KeyPair {
     /// Obtains a raw const pointer suitable for use with FFI functions.
     #[inline]
+    #[deprecated(since = "0.25.0", note = "Use Self::as_c_ptr if you need to access the FFI layer")]
     pub fn as_ptr(&self) -> *const ffi::KeyPair {
-        &self.0
+        self.as_c_ptr()
     }
 
     /// Obtains a raw mutable pointer suitable for use with FFI functions.
     #[inline]
+    #[deprecated(since = "0.25.0", note = "Use Self::as_mut_c_ptr if you need to access the FFI layer")]
     pub fn as_mut_ptr(&mut self) -> *mut ffi::KeyPair {
-        &mut self.0
+        self.as_mut_c_ptr()
     }
 
     /// Creates a [`KeyPair`] directly from a Secp256k1 secret key.
@@ -1090,6 +1101,17 @@ impl<'de> serde::Deserialize<'de> for KeyPair {
     }
 }
 
+impl CPtr for KeyPair {
+    type Target = ffi::KeyPair;
+    fn as_c_ptr(&self) -> *const Self::Target {
+        &self.0
+    }
+
+    fn as_mut_c_ptr(&mut self) -> *mut Self::Target {
+        &mut self.0
+    }
+}
+
 /// An x-only public key, used for verification of Schnorr signatures and serialized according to BIP-340.
 ///
 /// # Serde support
@@ -1148,14 +1170,16 @@ impl str::FromStr for XOnlyPublicKey {
 impl XOnlyPublicKey {
     /// Obtains a raw const pointer suitable for use with FFI functions.
     #[inline]
+    #[deprecated(since = "0.25.0", note = "Use Self::as_c_ptr if you need to access the FFI layer")]
     pub fn as_ptr(&self) -> *const ffi::XOnlyPublicKey {
-        &self.0
+        self.as_c_ptr()
     }
 
     /// Obtains a raw mutable pointer suitable for use with FFI functions.
     #[inline]
+    #[deprecated(since = "0.25.0", note = "Use Self::as_mut_c_ptr if you need to access the FFI layer")]
     pub fn as_mut_ptr(&mut self) -> *mut ffi::XOnlyPublicKey {
-        &mut self.0
+        self.as_mut_c_ptr()
     }
 
     /// Returns the [`XOnlyPublicKey`] (and it's [`Parity`]) for `keypair`.
@@ -1168,7 +1192,7 @@ impl XOnlyPublicKey {
                 ffi::secp256k1_context_no_precomp,
                 &mut xonly_pk,
                 &mut pk_parity,
-                keypair.as_ptr(),
+                keypair.as_c_ptr(),
             );
             debug_assert_eq!(ret, 1);
             let parity = Parity::from_i32(pk_parity).expect("should not panic, pk_parity is 0 or 1");
@@ -1496,11 +1520,11 @@ impl<'de> serde::Deserialize<'de> for Parity {
 impl CPtr for XOnlyPublicKey {
     type Target = ffi::XOnlyPublicKey;
     fn as_c_ptr(&self) -> *const Self::Target {
-        self.as_ptr()
+        &self.0
     }
 
     fn as_mut_c_ptr(&mut self) -> *mut Self::Target {
-        self.as_mut_ptr()
+        &mut self.0
     }
 }
 

src/schnorr.rs

@@ -112,7 +112,7 @@ impl<C: Signing> Secp256k1<C> {
                     self.ctx,
                     sig.as_mut_c_ptr(),
                     msg.as_c_ptr(),
-                    keypair.as_ptr(),
+                    keypair.as_c_ptr(),
                     nonce_data,
                 )
             );