Merge rust-bitcoin/rust-bitcoin#1503: Fix wrong newtype APIs

64f7d2549e Fix wrong newtype APIs (Martin Habovstiak)

Pull request description:

  This fixes several API bugs:

  * Use `TryFrom` instead of `From` for fallible conversions
  * Move byte conversion methods from `impl_array_newtype` to `impl_bytes_newtype`
  * Add missing trait impls like `AsRef`, `Borrow`, their mutable versions and infallible conversions from arrays

  Closes #1336

  Notably this change is much less bad than even I expected and some places actually get cleaner than before.

ACKs for top commit:
  tcharding:
    ACK 64f7d2549e
  sanket1729:
    code review ACK 64f7d2549e
  apoelstra:
    ACK 64f7d2549e

Tree-SHA512: 0c5ab19aa1a01ccf34ad1c2f3f66bd003a3142a840cb7cf57e1449e721f6eed1523555c62f0e3391012708767ff86315ff7edce9afd85017a67c61cfe57dddbc

bitcoin/src/bip32.rs

@@ -33,6 +33,12 @@ pub struct ChainCode([u8; 32]);
 impl_array_newtype!(ChainCode, u8, 32);
 impl_bytes_newtype!(ChainCode, 32);
 
+impl ChainCode {
+    fn from_hmac(hmac: Hmac<sha512::Hash>) -> Self {
+        hmac[32..].try_into().expect("half of hmac is guaranteed to be 32 bytes")
+    }
+}
+
 /// A fingerprint
 #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Default)]
 pub struct Fingerprint([u8; 4]);
@@ -523,7 +529,7 @@ impl ExtendedPrivKey {
             parent_fingerprint: Default::default(),
             child_number: ChildNumber::from_normal_idx(0)?,
             private_key: secp256k1::SecretKey::from_slice(&hmac_result[..32])?,
-            chain_code: ChainCode::from(&hmac_result[32..]),
+            chain_code: ChainCode::from_hmac(hmac_result),
         })
     }
 
@@ -588,7 +594,7 @@ impl ExtendedPrivKey {
             parent_fingerprint: self.fingerprint(secp),
             child_number: i,
             private_key: tweaked,
-            chain_code: ChainCode::from(&hmac_result[32..]),
+            chain_code: ChainCode::from_hmac(hmac_result),
         })
     }
 
@@ -611,9 +617,9 @@ impl ExtendedPrivKey {
         Ok(ExtendedPrivKey {
             network,
             depth: data[4],
-            parent_fingerprint: Fingerprint::from(&data[5..9]),
+            parent_fingerprint: data[5..9].try_into().expect("9 - 5 == 4, which is the Fingerprint length"),
             child_number: u32::from_be_bytes(data[9..13].try_into().expect("4 byte slice")).into(),
-            chain_code: ChainCode::from(&data[13..45]),
+            chain_code: data[13..45].try_into().expect("45 - 13 == 32, which is the ChainCode length"),
             private_key: secp256k1::SecretKey::from_slice(&data[46..78])?,
         })
     }
@@ -643,7 +649,7 @@ impl ExtendedPrivKey {
 
     /// Returns the first four bytes of the identifier
     pub fn fingerprint<C: secp256k1::Signing>(&self, secp: &Secp256k1<C>) -> Fingerprint {
-        Fingerprint::from(&self.identifier(secp)[0..4])
+        self.identifier(secp)[0..4].try_into().expect("4 is the fingerprint length")
     }
 }
 
@@ -701,7 +707,7 @@ impl ExtendedPubKey {
                 let hmac_result: Hmac<sha512::Hash> = Hmac::from_engine(hmac_engine);
 
                 let private_key = secp256k1::SecretKey::from_slice(&hmac_result[..32])?;
-                let chain_code = ChainCode::from(&hmac_result[32..]);
+                let chain_code = ChainCode::from_hmac(hmac_result);
                 Ok((private_key, chain_code))
             }
         }
@@ -743,9 +749,9 @@ impl ExtendedPubKey {
                 return Err(Error::UnknownVersion(ver));
             },
             depth: data[4],
-            parent_fingerprint: Fingerprint::from(&data[5..9]),
+            parent_fingerprint: data[5..9].try_into().expect("9 - 5 == 4, which is the Fingerprint length"),
             child_number: u32::from_be_bytes(data[9..13].try_into().expect("4 byte slice")).into(),
-            chain_code: ChainCode::from(&data[13..45]),
+            chain_code: data[13..45].try_into().expect("45 - 13 == 32, which is the ChainCode length"),
             public_key: secp256k1::PublicKey::from_slice(&data[45..78])?,
         })
     }
@@ -775,7 +781,7 @@ impl ExtendedPubKey {
     }
 
     /// Returns the first four bytes of the identifier
-    pub fn fingerprint(&self) -> Fingerprint { Fingerprint::from(&self.identifier()[0..4]) }
+    pub fn fingerprint(&self) -> Fingerprint { self.identifier()[0..4].try_into().expect("4 is the fingerprint length") }
 }
 
 impl fmt::Display for ExtendedPrivKey {
@@ -1108,10 +1114,10 @@ mod tests {
     #[cfg(feature = "serde")]
     pub fn encode_fingerprint_chaincode() {
         use serde_json;
-        let fp = Fingerprint::from(&[1u8, 2, 3, 42][..]);
+        let fp = Fingerprint::from([1u8, 2, 3, 42]);
         #[rustfmt::skip]
         let cc = ChainCode::from(
-            &[1u8,2,3,4,5,6,7,8,9,0,1,2,3,4,5,6,7,8,9,0,1,2,3,4,5,6,7,8,9,0,1,2][..]
+            [1u8,2,3,4,5,6,7,8,9,0,1,2,3,4,5,6,7,8,9,0,1,2,3,4,5,6,7,8,9,0,1,2]
         );
 
         serde_round_trip!(fp);
@@ -1156,7 +1162,7 @@ mod tests {
             parent_fingerprint: Default::default(),
             child_number: ChildNumber::Normal { index: 0 },
             private_key: sk,
-            chain_code: ChainCode::from(&[0u8; 32][..])
+            chain_code: ChainCode::from([0u8; 32])
         };
 
         println!("{}", xpriv);

bitcoin/src/internal_macros.rs

@@ -115,6 +115,23 @@ mod test_macros {
 /// - hashes::hex::FromHex
 macro_rules! impl_bytes_newtype {
     ($t:ident, $len:literal) => {
+        impl $t {
+            /// Returns a reference the underlying bytes.
+            #[inline]
+            pub fn as_bytes(&self) -> &[u8; $len] { &self.0 }
+
+            /// Returns the underlying bytes.
+            #[inline]
+            pub fn to_bytes(self) -> [u8; $len] {
+                // We rely on `Copy` being implemented for $t so conversion
+                // methods use the correct Rust naming conventions.
+                fn check_copy<T: Copy>() {}
+                check_copy::<$t>();
+
+                self.0
+            }
+        }
+
         impl core::fmt::LowerHex for $t {
             fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result {
                 for &ch in self.0.iter() {

bitcoin/src/psbt/map/global.rs

@@ -149,7 +149,7 @@ impl PartiallySignedTransaction {
                                 }
                                 let derivation = DerivationPath::from(path);
                                 // Keys, according to BIP-174, must be unique
-                                if xpub_map.insert(xpub, (Fingerprint::from(&fingerprint[..]), derivation)).is_some() {
+                                if xpub_map.insert(xpub, (Fingerprint::from(fingerprint), derivation)).is_some() {
                                     return Err(encode::Error::ParseFailed("Repeated global xpub key"))
                                 }
                             } else {

bitcoin/src/psbt/serialize.rs

@@ -6,6 +6,8 @@
 //! bytes from/as PSBT key-value pairs.
 //!
 
+use core::convert::TryInto;
+
 use crate::prelude::*;
 
 use crate::io;
@@ -150,7 +152,7 @@ impl Deserialize for KeySource {
             return Err(io::Error::from(io::ErrorKind::UnexpectedEof).into())
         }
 
-        let fprint: Fingerprint = Fingerprint::from(&bytes[0..4]);
+        let fprint: Fingerprint = bytes[0..4].try_into().expect("4 is the fingerprint length");
         let mut dpath: Vec<ChildNumber> = Default::default();
 
         let mut d = &bytes[4..];

internals/src/macros.rs

@@ -27,29 +27,76 @@ macro_rules! impl_array_newtype {
             /// Returns whether the object, as an array, is empty. Always false.
             #[inline]
             pub fn is_empty(&self) -> bool { false }
+        }
 
-            /// Returns a reference the underlying bytes.
-            #[inline]
-            pub fn as_bytes(&self) -> &[$ty; $len] { &self.0 }
-
-            /// Returns the underlying bytes.
-            #[inline]
-            pub fn to_bytes(self) -> [$ty; $len] {
-                // We rely on `Copy` being implemented for $thing so conversion
-                // methods use the correct Rust naming conventions.
-                fn check_copy<T: Copy>() {}
-                check_copy::<$thing>();
-
-                self.0
+        impl<'a> core::convert::From<[$ty; $len]> for $thing {
+            fn from(data: [$ty; $len]) -> Self {
+                $thing(data)
             }
         }
 
-        impl<'a> core::convert::From<&'a [$ty]> for $thing {
-            fn from(data: &'a [$ty]) -> $thing {
-                assert_eq!(data.len(), $len);
-                let mut ret = [0; $len];
-                ret.copy_from_slice(&data[..]);
-                $thing(ret)
+        impl<'a> core::convert::From<&'a [$ty; $len]> for $thing {
+            fn from(data: &'a [$ty; $len]) -> Self {
+                $thing(*data)
+            }
+        }
+
+        impl<'a> core::convert::TryFrom<&'a [$ty]> for $thing {
+            type Error = core::array::TryFromSliceError;
+
+            fn try_from(data: &'a [$ty]) -> Result<Self, Self::Error> {
+                use core::convert::TryInto;
+
+                Ok($thing(data.try_into()?))
+            }
+        }
+
+        impl AsRef<[$ty; $len]> for $thing {
+            fn as_ref(&self) -> &[$ty; $len] {
+                &self.0
+            }
+        }
+
+        impl AsMut<[$ty; $len]> for $thing {
+            fn as_mut(&mut self) -> &mut [$ty; $len] {
+                &mut self.0
+            }
+        }
+
+        impl AsRef<[$ty]> for $thing {
+            fn as_ref(&self) -> &[$ty] {
+                &self.0
+            }
+        }
+
+        impl AsMut<[$ty]> for $thing {
+            fn as_mut(&mut self) -> &mut [$ty] {
+                &mut self.0
+            }
+        }
+
+        impl core::borrow::Borrow<[$ty; $len]> for $thing {
+            fn borrow(&self) -> &[$ty; $len] {
+                &self.0
+            }
+        }
+
+        impl core::borrow::BorrowMut<[$ty; $len]> for $thing {
+            fn borrow_mut(&mut self) -> &mut [$ty; $len] {
+                &mut self.0
+            }
+        }
+
+        // The following two are valid because `[T; N]: Borrow<[T]>`
+        impl core::borrow::Borrow<[$ty]> for $thing {
+            fn borrow(&self) -> &[$ty] {
+                &self.0
+            }
+        }
+
+        impl core::borrow::BorrowMut<[$ty]> for $thing {
+            fn borrow_mut(&mut self) -> &mut [$ty] {
+                &mut self.0
             }
         }