Merge rust-bitcoin/rust-secp256k1#344: Improve handling of parity integer

ede114fb1a Improve docs on tweak_add_check method (Tobin Harding)
fbc64c7725 Add opaque parity type (Tobin Harding)
1b768b2749 Make tweak_add_assign return statements uniform (Tobin Harding)
edafb88f8c Move key unit tests to key module (Tobin Harding)
e3d21a3d87 Clean up test imports with key module (Tobin Harding)

Pull request description:

  Two functions in the FFI secp code return and accept a parity integer.

  Currently we are manually converting this to a bool. Doing so forces readers of the code to think what the bool means even though understanding this value is not needed since in is just passed back down to the FFI code.

  We initially tried to solve this issue by adding an enum, discussion below refers to that version. Instead of an enum we can solve this issue by adding an opaque type that holds the parity value returned by the FFI function call and then just pass it back down to FFI code without devs needing to know what the value should be. This fully abstracts the value away and removes the boolean conversion code which must otherwise be read by each dev.

  - Patch 1 and 2 improve unit tests that test the code path modified by this PR
  - Patch 3 trivially changes code to be uniform between two similar methods (`tweak_add_assign`)
  - Patch 4 is the meat and potatoes (main part of PR :)
  - Patch 5 is docs improvements to code in the area of this PR

ACKs for top commit:
  apoelstra:
    ACK ede114fb1a

Tree-SHA512: 37843e066d9006c5daa30dece9f7eb7a802864b85606e43ed2651c6d55938c4f884cc4abab81eccb69685f6eda918a9b9ba57bf1a4efec41e89239b99ae2b726

src/key.rs

@@ -636,12 +636,11 @@ impl KeyPair {
                 &mut self.0,
                 tweak.as_c_ptr(),
             );
-
+            if err != 1 {
-            if err == 1 {
+                return Err(Error::InvalidTweak);
-                Ok(())
-            } else {
-                Err(Error::InvalidTweak)
             }
+
+            Ok(())
         }
     }
 }
@@ -825,15 +824,18 @@ impl XOnlyPublicKey {
 
     /// Tweak an x-only PublicKey by adding the generator multiplied with the given tweak to it.
     ///
-    /// Returns a boolean representing the parity of the tweaked key, which can be provided to
+    /// # Return
+    /// An opaque type representing the parity of the tweaked key, this should be provided to
     /// `tweak_add_check` which can be used to verify a tweak more efficiently than regenerating
-    /// it and checking equality. Will return an error if the resulting key would be invalid or
+    /// it and checking equality.
-    /// if the tweak was not a 32-byte length slice.
+    ///
+    /// # Error
+    /// If the resulting key would be invalid or if the tweak was not a 32-byte length slice.
     pub fn tweak_add_assign<V: Verification>(
         &mut self,
         secp: &Secp256k1<V>,
         tweak: &[u8],
-    ) -> Result<bool, Error> {
+    ) -> Result<Parity, Error> {
         if tweak.len() != 32 {
             return Err(Error::InvalidTweak);
         }
@@ -846,7 +848,6 @@ impl XOnlyPublicKey {
                 self.as_c_ptr(),
                 tweak.as_c_ptr(),
             );
-
             if err != 1 {
                 return Err(Error::InvalidTweak);
             }
@@ -858,16 +859,15 @@ impl XOnlyPublicKey {
                 &mut parity,
                 &pubkey,
             );
-
             if err == 0 {
-                Err(Error::InvalidPublicKey)
+                return Err(Error::InvalidPublicKey);
-            } else {
-                Ok(parity != 0)
             }
+
+            Ok(parity.into())
         }
     }
 
-    /// Verify that a tweak produced by `tweak_add_assign` was computed correctly
+    /// Verify that a tweak produced by `tweak_add_assign` was computed correctly.
     ///
     /// Should be called on the original untweaked key. Takes the tweaked key and
     /// output parity from `tweak_add_assign` as input.
@@ -876,11 +876,14 @@ impl XOnlyPublicKey {
     /// and checking equality. However, in future this API will support batch
     /// verification, which is significantly faster, so it is wise to design
     /// protocols with this in mind.
+    ///
+    /// # Return
+    /// True if tweak and check is successful, false otherwise.
     pub fn tweak_add_check<V: Verification>(
         &self,
         secp: &Secp256k1<V>,
         tweaked_key: &Self,
-        tweaked_parity: bool,
+        tweaked_parity: Parity,
         tweak: [u8; 32],
     ) -> bool {
         let tweaked_ser = tweaked_key.serialize();
@@ -888,7 +891,7 @@ impl XOnlyPublicKey {
             let err = ffi::secp256k1_xonly_pubkey_tweak_add_check(
                 secp.ctx,
                 tweaked_ser.as_c_ptr(),
-                if tweaked_parity { 1 } else { 0 },
+                tweaked_parity.into(),
                 &self.0,
                 tweak.as_c_ptr(),
             );
@@ -898,6 +901,21 @@ impl XOnlyPublicKey {
     }
 }
 
+/// Opaque type used to hold the parity passed between FFI function calls.
+pub struct Parity(i32);
+
+impl From<i32> for Parity {
+    fn from(parity: i32) -> Parity {
+        Parity(parity)
+    }
+}
+
+impl From<Parity> for i32 {
+    fn from(parity: Parity) -> i32 {
+        parity.0
+    }
+}
+
 impl CPtr for XOnlyPublicKey {
     type Target = ffi::XOnlyPublicKey;
     fn as_c_ptr(&self) -> *const Self::Target {
@@ -964,16 +982,16 @@ impl<'de> ::serde::Deserialize<'de> for XOnlyPublicKey {
 
 #[cfg(test)]
 mod test {
-    use Secp256k1;
+    use super::*;
-    use {from_hex, to_hex};
+
-    use super::super::Error::{InvalidPublicKey, InvalidSecretKey};
+    use std::iter;
-    use super::{PublicKey, SecretKey};
+    use std::str::FromStr;
-    use super::super::constants;
 
     use rand::{Error, ErrorKind, RngCore, thread_rng};
     use rand_core::impls;
-    use std::iter;
+
-    use std::str::FromStr;
+    use {to_hex, constants};
+    use Error::{InvalidPublicKey, InvalidSecretKey};
 
     #[cfg(target_arch = "wasm32")]
     use wasm_bindgen_test::wasm_bindgen_test as test;
@@ -1476,4 +1494,38 @@ mod test {
         assert_tokens(&pk.readable(), &[Token::String(PK_STR)]);
 
     }
+
+    #[test]
+    fn test_tweak_add_assign_then_tweak_add_check() {
+        let s = Secp256k1::new();
+
+        for _ in 0..10 {
+            let mut tweak = [0u8; 32];
+            thread_rng().fill_bytes(&mut tweak);
+            let (mut kp, mut pk) = s.generate_schnorrsig_keypair(&mut thread_rng());
+            let orig_pk = pk;
+            kp.tweak_add_assign(&s, &tweak).expect("Tweak error");
+            let parity = pk.tweak_add_assign(&s, &tweak).expect("Tweak error");
+            assert_eq!(XOnlyPublicKey::from_keypair(&kp), pk);
+            assert!(orig_pk.tweak_add_check(&s, &pk, parity, tweak));
+        }
+    }
+
+    #[test]
+    fn test_from_key_pubkey() {
+        let kpk1 = PublicKey::from_str(
+            "02e6642fd69bd211f93f7f1f36ca51a26a5290eb2dd1b0d8279a87bb0d480c8443",
+        )
+        .unwrap();
+        let kpk2 = PublicKey::from_str(
+            "0384526253c27c7aef56c7b71a5cd25bebb66dddda437826defc5b2568bde81f07",
+        )
+        .unwrap();
+
+        let pk1 = XOnlyPublicKey::from(kpk1);
+        let pk2 = XOnlyPublicKey::from(kpk2);
+
+        assert_eq!(pk1.serialize()[..], kpk1.serialize()[1..]);
+        assert_eq!(pk2.serialize()[..], kpk2.serialize()[1..]);
+    }
 }

src/schnorr.rs

@@ -565,37 +565,4 @@ mod tests {
         assert_tokens(&pk.readable(), &[Token::Str(PK_STR)]);
         assert_tokens(&pk.readable(), &[Token::String(PK_STR)]);
     }
-    #[test]
-    fn test_addition() {
-        let s = Secp256k1::new();
-
-        for _ in 0..10 {
-            let mut tweak = [0u8; 32];
-            thread_rng().fill_bytes(&mut tweak);
-            let (mut kp, mut pk) = s.generate_schnorrsig_keypair(&mut thread_rng());
-            let orig_pk = pk;
-            kp.tweak_add_assign(&s, &tweak).expect("Tweak error");
-            let parity = pk.tweak_add_assign(&s, &tweak).expect("Tweak error");
-            assert_eq!(XOnlyPublicKey::from_keypair(&kp), pk);
-            assert!(orig_pk.tweak_add_check(&s, &pk, parity, tweak));
-        }
-    }
-
-    #[test]
-    fn test_from_key_pubkey() {
-        let kpk1 = ::key::PublicKey::from_str(
-            "02e6642fd69bd211f93f7f1f36ca51a26a5290eb2dd1b0d8279a87bb0d480c8443",
-        )
-        .unwrap();
-        let kpk2 = ::key::PublicKey::from_str(
-            "0384526253c27c7aef56c7b71a5cd25bebb66dddda437826defc5b2568bde81f07",
-        )
-        .unwrap();
-
-        let pk1 = XOnlyPublicKey::from(kpk1);
-        let pk2 = XOnlyPublicKey::from(kpk2);
-
-        assert_eq!(pk1.serialize()[..], kpk1.serialize()[1..]);
-        assert_eq!(pk2.serialize()[..], kpk2.serialize()[1..]);
-    }
 }