From 64f7d2549ed29bd290a8d7a3aeb4a3b2c51f2a50 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Fri, 23 Dec 2022 01:28:17 +0100 Subject: [PATCH] Fix wrong newtype APIs 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 --- bitcoin/src/bip32.rs | 30 +++++++----- bitcoin/src/internal_macros.rs | 17 +++++++ bitcoin/src/psbt/map/global.rs | 2 +- bitcoin/src/psbt/serialize.rs | 4 +- internals/src/macros.rs | 85 ++++++++++++++++++++++++++-------- 5 files changed, 105 insertions(+), 33 deletions(-) diff --git a/bitcoin/src/bip32.rs b/bitcoin/src/bip32.rs index f5f5f59d..777eae77 100644 --- a/bitcoin/src/bip32.rs +++ b/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) -> 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(&self, secp: &Secp256k1) -> 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 = 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); diff --git a/bitcoin/src/internal_macros.rs b/bitcoin/src/internal_macros.rs index 1ad769f4..ffa5f7a0 100644 --- a/bitcoin/src/internal_macros.rs +++ b/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() {} + 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() { diff --git a/bitcoin/src/psbt/map/global.rs b/bitcoin/src/psbt/map/global.rs index a337e7c6..eb317718 100644 --- a/bitcoin/src/psbt/map/global.rs +++ b/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 { diff --git a/bitcoin/src/psbt/serialize.rs b/bitcoin/src/psbt/serialize.rs index 0fd909a7..7b4ff52f 100644 --- a/bitcoin/src/psbt/serialize.rs +++ b/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 = Default::default(); let mut d = &bytes[4..]; diff --git a/internals/src/macros.rs b/internals/src/macros.rs index d000943e..67de76c3 100644 --- a/internals/src/macros.rs +++ b/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() {} - 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 { + 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 } }