Merge pull request #253 from apoelstra/2020-11--schnorrsig-followup

apoelstra · web-flow · commit 11e9641d21c8 · 2020-12-09T20:31:07.000Z
BIP 0340 followups
diff --git a/secp256k1-sys/src/lib.rs b/secp256k1-sys/src/lib.rs
@@ -106,13 +106,32 @@ impl_array_newtype!(PublicKey, c_uchar, 64);
 impl_raw_debug!(PublicKey);
 
 impl PublicKey {
-    /// Create a new (zeroed) public key usable for the FFI interface
-    pub fn new() -> PublicKey { PublicKey([0; 64]) }
-}
+    /// Creates an "uninitialized" FFI public key which is zeroed out
+    ///
+    /// If you pass this to any FFI functions, except as an out-pointer,
+    /// the result is likely to be an assertation failure and process
+    /// termination.
+    pub unsafe fn new() -> Self {
+        Self::from_array_unchecked([0; 64])
+    }
+
+    /// Create a new public key usable for the FFI interface from raw bytes
+    ///
+    /// Does not check the validity of the underlying representation. If it is
+    /// invalid the result may be assertation failures (and process aborts) from
+    /// the underlying library. You should not use this method except with data
+    /// that you obtained from the FFI interface of the same version of this
+    /// library.
+    pub unsafe fn from_array_unchecked(data: [c_uchar; 64]) -> Self {
+        PublicKey(data)
+    }
 
-impl Default for PublicKey {
-    fn default() -> Self {
-        PublicKey::new()
+    /// Returns the underlying FFI opaque representation of the public key
+    ///
+    /// You should not use this unless you really know what you are doing. It is
+    /// essentially only useful for extending the FFI interface itself.
+    pub fn underlying_bytes(self) -> [c_uchar; 64] {
+        self.0
     }
 }
 
@@ -129,13 +148,32 @@ impl_array_newtype!(Signature, c_uchar, 64);
 impl_raw_debug!(Signature);
 
 impl Signature {
-    /// Create a new (zeroed) signature usable for the FFI interface
-    pub fn new() -> Signature { Signature([0; 64]) }
-}
+    /// Creates an "uninitialized" FFI signature which is zeroed out
+    ///
+    /// If you pass this to any FFI functions, except as an out-pointer,
+    /// the result is likely to be an assertation failure and process
+    /// termination.
+    pub unsafe fn new() -> Self {
+        Self::from_array_unchecked([0; 64])
+    }
 
-impl Default for Signature {
-    fn default() -> Self {
-        Signature::new()
+    /// Create a new signature usable for the FFI interface from raw bytes
+    ///
+    /// Does not check the validity of the underlying representation. If it is
+    /// invalid the result may be assertation failures (and process aborts) from
+    /// the underlying library. You should not use this method except with data
+    /// that you obtained from the FFI interface of the same version of this
+    /// library.
+    pub unsafe fn from_array_unchecked(data: [c_uchar; 64]) -> Self {
+        Signature(data)
+    }
+
+    /// Returns the underlying FFI opaque representation of the signature
+    ///
+    /// You should not use this unless you really know what you are doing. It is
+    /// essentially only useful for extending the FFI interface itself.
+    pub fn underlying_bytes(self) -> [c_uchar; 64] {
+        self.0
     }
 }
 
@@ -145,11 +183,33 @@ impl_array_newtype!(XOnlyPublicKey, c_uchar, 64);
 impl_raw_debug!(XOnlyPublicKey);
 
 impl XOnlyPublicKey {
-    /// Create a new (zeroed) x-only public key usable for the FFI interface
-    pub fn new() -> XOnlyPublicKey { XOnlyPublicKey([0; 64]) }
-    pub fn from_array(data: [c_uchar; 64]) -> XOnlyPublicKey {
+    /// Creates an "uninitialized" FFI x-only public key which is zeroed out
+    ///
+    /// If you pass this to any FFI functions, except as an out-pointer,
+    /// the result is likely to be an assertation failure and process
+    /// termination.
+    pub unsafe fn new() -> Self {
+        Self::from_array_unchecked([0; 64])
+    }
+
+    /// Create a new x-only public key usable for the FFI interface from raw bytes
+    ///
+    /// Does not check the validity of the underlying representation. If it is
+    /// invalid the result may be assertation failures (and process aborts) from
+    /// the underlying library. You should not use this method except with data
+    /// that you obtained from the FFI interface of the same version of this
+    /// library.
+    pub unsafe fn from_array_unchecked(data: [c_uchar; 64]) -> Self {
         XOnlyPublicKey(data)
     }
+
+    /// Returns the underlying FFI opaque representation of the x-only public key
+    ///
+    /// You should not use this unless you really know what you are doing. It is
+    /// essentially only useful for extending the FFI interface itself.
+    pub fn underlying_bytes(self) -> [c_uchar; 64] {
+        self.0
+    }
 }
 
 impl hash::Hash for XOnlyPublicKey {
@@ -158,23 +218,39 @@ impl hash::Hash for XOnlyPublicKey {
     }
 }
 
-impl Default for XOnlyPublicKey {
-    fn default() -> Self {
-        XOnlyPublicKey::new()
-    }
-}
-
 #[repr(C)]
 pub struct KeyPair([c_uchar; 96]);
 impl_array_newtype!(KeyPair, c_uchar, 96);
 impl_raw_debug!(KeyPair);
 
 impl KeyPair {
-    /// Create a new (zeroed) key pair usable for the FFI interface
-    pub fn new() -> KeyPair { KeyPair([0; 96]) }
-    pub fn from_array(data: [c_uchar; 96]) -> KeyPair {
+    /// Creates an "uninitialized" FFI keypair which is zeroed out
+    ///
+    /// If you pass this to any FFI functions, except as an out-pointer,
+    /// the result is likely to be an assertation failure and process
+    /// termination.
+    pub unsafe fn new() -> Self {
+        Self::from_array_unchecked([0; 96])
+    }
+
+    /// Create a new keypair usable for the FFI interface from raw bytes
+    ///
+    /// Does not check the validity of the underlying representation. If it is
+    /// invalid the result may be assertation failures (and process aborts) from
+    /// the underlying library. You should not use this method except with data
+    /// that you obtained from the FFI interface of the same version of this
+    /// library.
+    pub unsafe fn from_array_unchecked(data: [c_uchar; 96]) -> Self {
         KeyPair(data)
     }
+
+    /// Returns the underlying FFI opaque representation of the x-only public key
+    ///
+    /// You should not use this unless you really know what you are doing. It is
+    /// essentially only useful for extending the FFI interface itself.
+    pub fn underlying_bytes(self) -> [c_uchar; 96] {
+        self.0
+    }
 }
 
 impl hash::Hash for KeyPair {
@@ -183,12 +259,6 @@ impl hash::Hash for KeyPair {
     }
 }
 
-impl Default for KeyPair {
-    fn default() -> Self {
-        KeyPair::new()
-    }
-}
-
 #[cfg(not(feature = "fuzztarget"))]
 extern "C" {
     /// Default ECDH hash function
@@ -591,7 +661,7 @@ impl<T> CPtr for [T] {
 
     fn as_mut_c_ptr(&mut self) -> *mut Self::Target {
         if self.is_empty() {
-            ptr::null::<Self::Target>() as *mut _
+            ptr::null_mut::<Self::Target>()
         } else {
             self.as_mut_ptr()
         }
diff --git a/src/key.rs b/src/key.rs
@@ -219,37 +219,37 @@ impl PublicKey {
     /// Obtains a raw const pointer suitable for use with FFI functions
     #[inline]
     pub fn as_ptr(&self) -> *const ffi::PublicKey {
-        &self.0 as *const _
+        &self.0
     }
 
     /// Obtains a raw mutable pointer suitable for use with FFI functions
     #[inline]
     pub fn as_mut_ptr(&mut self) -> *mut ffi::PublicKey {
-        &mut self.0 as *mut _
+        &mut self.0
     }
 
     /// Creates a new public key from a secret key.
     #[inline]
     pub fn from_secret_key<C: Signing>(secp: &Secp256k1<C>,
                            sk: &SecretKey)
                            -> PublicKey {
-        let mut pk = ffi::PublicKey::new();
         unsafe {
+            let mut pk = ffi::PublicKey::new();
             // We can assume the return value because it's not possible to construct
             // an invalid `SecretKey` without transmute trickery or something
             let res = ffi::secp256k1_ec_pubkey_create(secp.ctx, &mut pk, sk.as_c_ptr());
             debug_assert_eq!(res, 1);
+            PublicKey(pk)
         }
-        PublicKey(pk)
     }
 
     /// Creates a public key directly from a slice
     #[inline]
     pub fn from_slice(data: &[u8]) -> Result<PublicKey, Error> {
         if data.is_empty() {return Err(Error::InvalidPublicKey);}
 
-        let mut pk = ffi::PublicKey::new();
         unsafe {
+            let mut pk = ffi::PublicKey::new();
             if ffi::secp256k1_ec_pubkey_parse(
                 ffi::secp256k1_context_no_precomp,
                 &mut pk,
@@ -313,7 +313,7 @@ impl PublicKey {
         secp: &Secp256k1<C>
     ) {
         unsafe {
-            let res = ffi::secp256k1_ec_pubkey_negate(secp.ctx, &mut self.0 as *mut _);
+            let res = ffi::secp256k1_ec_pubkey_negate(secp.ctx, &mut self.0);
             debug_assert_eq!(res, 1);
         }
     }
@@ -331,8 +331,7 @@ impl PublicKey {
             return Err(Error::InvalidTweak);
         }
         unsafe {
-            if ffi::secp256k1_ec_pubkey_tweak_add(secp.ctx, &mut self.0 as *mut _,
-                                                  other.as_c_ptr()) == 1 {
+            if ffi::secp256k1_ec_pubkey_tweak_add(secp.ctx, &mut self.0, other.as_c_ptr()) == 1 {
                 Ok(())
             } else {
                 Err(Error::InvalidTweak)
@@ -353,8 +352,7 @@ impl PublicKey {
             return Err(Error::InvalidTweak);
         }
         unsafe {
-            if ffi::secp256k1_ec_pubkey_tweak_mul(secp.ctx, &mut self.0 as *mut _,
-                                                  other.as_c_ptr()) == 1 {
+            if ffi::secp256k1_ec_pubkey_tweak_mul(secp.ctx, &mut self.0, other.as_c_ptr()) == 1 {
                 Ok(())
             } else {
                 Err(Error::InvalidTweak)
diff --git a/src/lib.rs b/src/lib.rs
@@ -269,9 +269,8 @@ impl Signature {
     pub fn from_der(data: &[u8]) -> Result<Signature, Error> {
         if data.is_empty() {return Err(Error::InvalidSignature);}
 
-        let mut ret = ffi::Signature::new();
-
         unsafe {
+            let mut ret = ffi::Signature::new();
             if ffi::secp256k1_ecdsa_signature_parse_der(
                 ffi::secp256k1_context_no_precomp,
                 &mut ret,
@@ -288,12 +287,12 @@ impl Signature {
 
     /// Converts a 64-byte compact-encoded byte slice to a signature
     pub fn from_compact(data: &[u8]) -> Result<Signature, Error> {
-        let mut ret = ffi::Signature::new();
         if data.len() != 64 {
             return Err(Error::InvalidSignature)
         }
 
         unsafe {
+            let mut ret = ffi::Signature::new();
             if ffi::secp256k1_ecdsa_signature_parse_compact(
                 ffi::secp256k1_context_no_precomp,
                 &mut ret,
@@ -362,13 +361,13 @@ impl Signature {
     /// Obtains a raw pointer suitable for use with FFI functions
     #[inline]
     pub fn as_ptr(&self) -> *const ffi::Signature {
-        &self.0 as *const _
+        &self.0
     }
 
     /// Obtains a raw mutable pointer suitable for use with FFI functions
     #[inline]
     pub fn as_mut_ptr(&mut self) -> *mut ffi::Signature {
-        &mut self.0 as *mut _
+        &mut self.0
     }
 
     #[inline]
@@ -522,6 +521,8 @@ pub enum Error {
     InvalidRecoveryId,
     /// Invalid tweak for add_*_assign or mul_*_assign
     InvalidTweak,
+    /// `tweak_add_check` failed on an xonly public key
+    TweakCheckFailed,
     /// Didn't pass enough memory to context creation with preallocated memory
     NotEnoughMemory,
 }
@@ -536,6 +537,7 @@ impl Error {
             Error::InvalidSecretKey => "secp: malformed or out-of-range secret key",
             Error::InvalidRecoveryId => "secp: bad recovery id",
             Error::InvalidTweak => "secp: bad tweak",
+            Error::TweakCheckFailed => "secp: xonly_pubkey_tewak_add_check failed",
             Error::NotEnoughMemory => "secp: not enough memory allocated",
         }
     }
@@ -661,16 +663,15 @@ impl<C: Signing> Secp256k1<C> {
     pub fn sign(&self, msg: &Message, sk: &key::SecretKey)
                 -> Signature {
 
-        let mut ret = ffi::Signature::new();
         unsafe {
+            let mut ret = ffi::Signature::new();
             // We can assume the return value because it's not possible to construct
             // an invalid signature from a valid `Message` and `SecretKey`
             assert_eq!(ffi::secp256k1_ecdsa_sign(self.ctx, &mut ret, msg.as_c_ptr(),
                                                  sk.as_c_ptr(), ffi::secp256k1_nonce_function_rfc6979,
                                                  ptr::null()), 1);
+            Signature::from(ret)
         }
-
-        Signature::from(ret)
     }
 
     /// Generates a random keypair. Convenience function for `key::SecretKey::new`
diff --git a/src/recovery.rs b/src/recovery.rs
@@ -82,13 +82,13 @@ impl RecoverableSignature {
     /// Obtains a raw pointer suitable for use with FFI functions
     #[inline]
     pub fn as_ptr(&self) -> *const ffi::RecoverableSignature {
-        &self.0 as *const _
+        &self.0
     }
 
     /// Obtains a raw mutable pointer suitable for use with FFI functions
     #[inline]
     pub fn as_mut_ptr(&mut self) -> *mut ffi::RecoverableSignature {
-        &mut self.0 as *mut _
+        &mut self.0
     }
 
     #[inline]
@@ -112,16 +112,16 @@ impl RecoverableSignature {
     /// for verification
     #[inline]
     pub fn to_standard(&self) -> Signature {
-        let mut ret = super_ffi::Signature::new();
         unsafe {
+            let mut ret = super_ffi::Signature::new();
             let err = ffi::secp256k1_ecdsa_recoverable_signature_convert(
                 super_ffi::secp256k1_context_no_precomp,
                 &mut ret,
                 self.as_c_ptr(),
             );
             assert!(err == 1);
+            Signature(ret)
         }
-        Signature(ret)
     }
 }
 
@@ -178,15 +178,14 @@ impl<C: Verification> Secp256k1<C> {
     pub fn recover(&self, msg: &Message, sig: &RecoverableSignature)
                    -> Result<key::PublicKey, Error> {
 
-        let mut pk = super_ffi::PublicKey::new();
-
         unsafe {
+            let mut pk = super_ffi::PublicKey::new();
             if ffi::secp256k1_ecdsa_recover(self.ctx, &mut pk,
                                             sig.as_c_ptr(), msg.as_c_ptr()) != 1 {
                 return Err(Error::InvalidSignature);
             }
-        };
-        Ok(key::PublicKey::from(pk))
+            Ok(key::PublicKey::from(pk))
+        }
     }
 }
 
diff --git a/src/schnorrsig.rs b/src/schnorrsig.rs
