Merge rust-bitcoin/rust-secp256k1#382: Fix the mess around Parity

6fad20ef0c Fix the mess around Parity (Tobin Harding)

Pull request description:

  Recently we made a wee mess with the `Parity` opaque type. Let's fix it
  up by doing:

  - Use an enum with variants `Even` and `Odd`.
  - Add explicit conversion methods to/from u8 and i32
  - Implement `BitXor`

  This PR attempts to follow the plan laid out in the issue but:
  - I don't get why`to_u8` is requested, I added it anyways.
  - `to_i32` is not mentioned but we need it if we are going to pass the parity back to FFI code after receiving it _without_ having to care about the value. And that is the whole point of this `Parity` type in the first place.
  - I don't get why `from_xxx` is fallible, when is an integer not even or odd?

  Please note: This patch is an API breaking change that does _not_ follow the deprecation guidelines. Rust does not allow deprecating `From` impl blocks AFAICT.

  Fixes: #370

ACKs for top commit:
  apoelstra:
    ACK 6fad20ef0c

Tree-SHA512: 810d5028f02fa608eab48721d32dca58be472080fcbac7a0ee79b5211b229112a756c48233e8597fb6c137690b2565431f410e9e97d6940ed389a4501bb015a0

src/key.rs

@@ -18,6 +18,7 @@
 #[cfg(any(test, feature = "rand"))] use rand::Rng;
 
 use core::{fmt, ptr, str};
+use core::ops::BitXor;
 
 use super::{from_hex, Secp256k1};
 use super::Error::{self, InvalidPublicKey, InvalidPublicKeySum, InvalidSecretKey};
@@ -890,7 +891,7 @@ impl XOnlyPublicKey {
                 return Err(Error::InvalidPublicKey);
             }
 
-            Ok(parity.into())
+            Parity::from_i32(parity)
         }
     }
 
@@ -918,7 +919,7 @@ impl XOnlyPublicKey {
             let err = ffi::secp256k1_xonly_pubkey_tweak_add_check(
                 secp.ctx,
                 tweaked_ser.as_c_ptr(),
-                tweaked_parity.into(),
+                tweaked_parity.to_i32(),
                 &self.0,
                 tweak.as_c_ptr(),
             );
@@ -928,19 +929,69 @@ impl XOnlyPublicKey {
     }
 }
 
-/// Opaque type used to hold the parity passed between FFI function calls.
+/// Represents the parity passed between FFI function calls.
 #[derive(Copy, Clone, PartialEq, Eq, Debug, PartialOrd, Ord, Hash)]
-pub struct Parity(i32);
+pub enum Parity {
+    /// Even parity.
+    Even = 0,
+    /// Odd parity.
+    Odd = 1,
+}
+
+impl Parity {
+    /// Converts parity into a integer (byte) value.
+    pub fn to_u8(self) -> u8 {
+        self as u8
+    }
+
+    /// Converts parity into a integer value.
+    pub fn to_i32(self) -> i32 {
+        self as i32
+    }
+
+    /// Constructs a [`Parity`] from a byte.
+    pub fn from_u8(parity: u8) -> Result<Parity, Error> {
+        Parity::from_i32(parity as i32)
+    }
+
+    /// Constructs a [`Parity`] from a signed integer.
+    pub fn from_i32(parity: i32) -> Result<Parity, Error> {
+        match parity {
+            0 => Ok(Parity::Even),
+            1 => Ok(Parity::Odd),
+            _ => Err(Error::InvalidParityValue),
+        }
+    }
+}
 
 impl From<i32> for Parity {
+    /// Please note, this method is deprecated and will be removed in an upcoming release, it
+    /// is not equivalent to `from_u32()`, it is better to use `Parity::from_u32`.
     fn from(parity: i32) -> Parity {
-        Parity(parity)
+        if parity % 2 == 0 {
+            Parity::Even
+        } else {
+            Parity::Odd
+        }
     }
 }
 
 impl From<Parity> for i32 {
     fn from(parity: Parity) -> i32 {
-        parity.0
+        parity.to_i32()
+    }
+}
+
+impl BitXor for Parity {
+    type Output = Parity;
+
+    fn bitxor(self, rhs: Parity) -> Self::Output {
+        // This works because Parity has only two values (i.e. only 1 bit of information).
+        if self == rhs {
+            Parity::Even        // 1^1==0 and 0^0==0
+        } else {
+            Parity::Odd         // 1^0==1 and 0^1==1
+        }
     }
 }
 
@@ -948,7 +999,7 @@ impl From<Parity> for i32 {
 #[cfg_attr(docsrs, doc(cfg(feature = "serde")))]
 impl ::serde::Serialize for Parity {
     fn serialize<S: ::serde::Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
-        s.serialize_i32(self.0)
+        s.serialize_i32(self.to_i32())
     }
 }
 
@@ -969,7 +1020,7 @@ impl<'de> ::serde::Deserialize<'de> for Parity {
             fn visit_i32<E>(self, v: i32) -> Result<Self::Value, E>
                 where E: ::serde::de::Error
             {
-                Ok(Parity::from(v))
+                Parity::from_i32(v).map_err(E::custom)
             }
         }
 

src/lib.rs

@@ -322,6 +322,8 @@ pub enum Error {
     NotEnoughMemory,
     /// Bad set of public keys
     InvalidPublicKeySum,
+    /// The only valid parity values are 0 or 1.
+    InvalidParityValue,
 }
 
 impl Error {
@@ -336,6 +338,7 @@ impl Error {
             Error::InvalidTweak => "secp: bad tweak",
             Error::NotEnoughMemory => "secp: not enough memory allocated",
             Error::InvalidPublicKeySum => "secp: the sum of public keys was invalid or the input vector lengths was less than 1",
+            Error::InvalidParityValue => "The only valid parity values are 0 or 1",
         }
     }
 }