From 2cc944578d3939d6dbb58babccdd93d1c722c971 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 18 Sep 2024 10:53:47 +1000 Subject: [PATCH] Fully deprecate Hash::from_slice We had an initial go at this but we didn't do the `Hash` trait method. In order to do so we need to hack the serde code a fair bit, note the public visitor types. --- bitcoin/src/address/script_pubkey.rs | 4 +- bitcoin/src/blockdata/block.rs | 6 +-- bitcoin/src/crypto/sighash.rs | 5 +- bitcoin/src/merkle_tree/block.rs | 2 +- bitcoin/src/psbt/error.rs | 6 +-- bitcoin/src/psbt/macros.rs | 4 +- bitcoin/src/psbt/mod.rs | 2 +- bitcoin/src/taproot/merkle_branch.rs | 4 +- bitcoin/src/taproot/mod.rs | 7 +-- hashes/src/hmac.rs | 3 +- hashes/src/internal_macros.rs | 3 +- hashes/src/lib.rs | 1 + hashes/src/serde_macros.rs | 80 +++++++++++----------------- hashes/src/util.rs | 5 +- 14 files changed, 60 insertions(+), 72 deletions(-) diff --git a/bitcoin/src/address/script_pubkey.rs b/bitcoin/src/address/script_pubkey.rs index edd74ee01..fac45467e 100644 --- a/bitcoin/src/address/script_pubkey.rs +++ b/bitcoin/src/address/script_pubkey.rs @@ -70,8 +70,8 @@ define_extension_trait! { fn p2wpkh_script_code(&self) -> Option { if self.is_p2wpkh() { // The `self` script is 0x00, 0x14, - let bytes = &self.as_bytes()[2..]; - let wpkh = WPubkeyHash::from_slice(bytes).expect("length checked in is_p2wpkh()"); + let bytes = <[u8; 20]>::try_from(&self.as_bytes()[2..]).expect("length checked in is_p2wpkh()"); + let wpkh = WPubkeyHash::from_byte_array(bytes); Some(script::p2wpkh_script_code(wpkh)) } else { None diff --git a/bitcoin/src/blockdata/block.rs b/bitcoin/src/blockdata/block.rs index 3d90d7e6b..7d6a9897e 100644 --- a/bitcoin/src/blockdata/block.rs +++ b/bitcoin/src/blockdata/block.rs @@ -190,10 +190,8 @@ impl Block { .iter() .rposition(|o| o.script_pubkey.len() >= 38 && o.script_pubkey.as_bytes()[0..6] == MAGIC) { - let commitment = WitnessCommitment::from_slice( - &coinbase.output[pos].script_pubkey.as_bytes()[6..38], - ) - .unwrap(); + let bytes = <[u8; 32]>::try_from(&coinbase.output[pos].script_pubkey.as_bytes()[6..38]).unwrap(); + let commitment = WitnessCommitment::from_byte_array(bytes); // Witness reserved value is in coinbase input witness. let witness_vec: Vec<_> = coinbase.input[0].witness.iter().collect(); if witness_vec.len() == 1 && witness_vec[0].len() == 32 { diff --git a/bitcoin/src/crypto/sighash.rs b/bitcoin/src/crypto/sighash.rs index f1faa5efd..45a9b4d93 100644 --- a/bitcoin/src/crypto/sighash.rs +++ b/bitcoin/src/crypto/sighash.rs @@ -1527,7 +1527,7 @@ mod tests { let cache = SighashCache::new(&tx); let got = cache.legacy_signature_hash(1, &script, SIGHASH_SINGLE).expect("sighash"); - let want = LegacySighash::from_slice(&UINT256_ONE).unwrap(); + let want = LegacySighash::from_byte_array(UINT256_ONE); assert_eq!(got, want) } @@ -1550,7 +1550,8 @@ mod tests { let script = ScriptBuf::from(Vec::from_hex(script).unwrap()); let mut raw_expected = Vec::from_hex(expected_result).unwrap(); raw_expected.reverse(); - let want = LegacySighash::from_slice(&raw_expected[..]).unwrap(); + let bytes = <[u8; 32]>::try_from(&raw_expected[..]).unwrap(); + let want = LegacySighash::from_byte_array(bytes); let cache = SighashCache::new(&tx); let got = cache.legacy_signature_hash(input_index, &script, hash_type as u32).unwrap(); diff --git a/bitcoin/src/merkle_tree/block.rs b/bitcoin/src/merkle_tree/block.rs index 4147d84a3..1b96efe12 100644 --- a/bitcoin/src/merkle_tree/block.rs +++ b/bitcoin/src/merkle_tree/block.rs @@ -724,7 +724,7 @@ mod tests { let hashes = &mut self.hashes; let mut hash = hashes[n].to_byte_array(); hash[(bit >> 3) as usize] ^= 1 << (bit & 7); - hashes[n] = TxMerkleNode::from_slice(&hash).unwrap(); + hashes[n] = TxMerkleNode::from_byte_array(hash); } } diff --git a/bitcoin/src/psbt/error.rs b/bitcoin/src/psbt/error.rs index 9a2fdf5f4..1804459e4 100644 --- a/bitcoin/src/psbt/error.rs +++ b/bitcoin/src/psbt/error.rs @@ -56,7 +56,7 @@ pub enum Error { /// Unable to parse as a standard sighash type. NonStandardSighashType(u32), /// Invalid hash when parsing slice. - InvalidHash(hashes::FromSliceError), + InvalidHash(core::array::TryFromSliceError), /// The pre-image must hash to the corresponding psbt hash InvalidPreimageHashPair { /// Hash-type @@ -203,8 +203,8 @@ impl std::error::Error for Error { } } -impl From for Error { - fn from(e: hashes::FromSliceError) -> Error { Error::InvalidHash(e) } +impl From for Error { + fn from(e: core::array::TryFromSliceError) -> Error { Error::InvalidHash(e) } } impl From for Error { diff --git a/bitcoin/src/psbt/macros.rs b/bitcoin/src/psbt/macros.rs index d0c8d7956..f5134946b 100644 --- a/bitcoin/src/psbt/macros.rs +++ b/bitcoin/src/psbt/macros.rs @@ -173,7 +173,9 @@ macro_rules! impl_psbt_hash_deserialize { ($hash_type:ty) => { impl $crate::psbt::serialize::Deserialize for $hash_type { fn deserialize(bytes: &[u8]) -> core::result::Result { - <$hash_type>::from_slice(&bytes[..]).map_err(|e| $crate::psbt::Error::from(e)) + const LEN: usize = <$hash_type as hashes::Hash>::LEN; + let bytes = <[u8; LEN]>::try_from(bytes).map_err(|e| $crate::psbt::Error::from(e))?; + Ok(<$hash_type>::from_byte_array(bytes)) } } }; diff --git a/bitcoin/src/psbt/mod.rs b/bitcoin/src/psbt/mod.rs index 77eb01e56..d934f21e0 100644 --- a/bitcoin/src/psbt/mod.rs +++ b/bitcoin/src/psbt/mod.rs @@ -1886,7 +1886,7 @@ mod tests { #[cfg(not(feature = "std"))] assert_eq!( err.to_string(), - "invalid hash when parsing slice: invalid slice length 33 (expected 32)" + "invalid hash when parsing slice: could not convert slice to array" ); let err = hex_psbt("70736274ff01005e02000000019bd48765230bf9a72e662001f972556e54f0c6f97feb56bcb5600d817f6995260100000000ffffffff0148e6052a01000000225120030da4fce4f7db28c2cb2951631e003713856597fe963882cb500e68112cca63000000000001012b00f2052a01000000225120c2247efbfd92ac47f6f40b8d42d169175a19fa9fa10e4a25d7f35eb4dd85b69241142cb13ac68248de806aa6a3659cf3c03eb6821d09c8114a4e868febde865bb6d2cd970e15f53fc0c82f950fd560ffa919b76172be017368a89913af074f400b094289756aa3739ccc689ec0fcf3a360be32cc0b59b16e93a1e8bb4605726b2ca7a3ff706c4176649632b2cc68e1f912b8a578e3719ce7710885c7a966f49bcd43cb01010000").unwrap_err(); #[cfg(feature = "std")] diff --git a/bitcoin/src/taproot/merkle_branch.rs b/bitcoin/src/taproot/merkle_branch.rs index e48648ad3..a28f5b0ff 100644 --- a/bitcoin/src/taproot/merkle_branch.rs +++ b/bitcoin/src/taproot/merkle_branch.rs @@ -53,8 +53,8 @@ impl TaprootMerkleBranch { let inner = sl .chunks_exact(TAPROOT_CONTROL_NODE_SIZE) .map(|chunk| { - TapNodeHash::from_slice(chunk) - .expect("chunks_exact always returns the correct size") + let bytes = <[u8; 32]>::try_from(chunk).expect("chunks_exact always returns the correct size"); + TapNodeHash::from_byte_array(bytes) }) .collect(); diff --git a/bitcoin/src/taproot/mod.rs b/bitcoin/src/taproot/mod.rs index 31de88a20..5be2d3073 100644 --- a/bitcoin/src/taproot/mod.rs +++ b/bitcoin/src/taproot/mod.rs @@ -1560,7 +1560,6 @@ mod test { #[cfg(feature = "serde")] use { - hex::test_hex_unwrap as hex, serde_test::Configure, serde_test::{assert_tokens, Token}, }; @@ -1855,10 +1854,8 @@ mod test { #[test] #[cfg(feature = "serde")] fn test_merkle_branch_serde() { - let dummy_hash = hex!("03ba2a4dcd914fed29a1c630c7e811271b081a0e2f2f52cf1c197583dfd46c1b"); - let hash1 = TapNodeHash::from_slice(&dummy_hash).unwrap(); - let dummy_hash = hex!("8d79dedc2fa0b55167b5d28c61dbad9ce1191a433f3a1a6c8ee291631b2c94c9"); - let hash2 = TapNodeHash::from_slice(&dummy_hash).unwrap(); + let hash1 = TapNodeHash("03ba2a4dcd914fed29a1c630c7e811271b081a0e2f2f52cf1c197583dfd46c1b".parse::>().unwrap()); + let hash2 = TapNodeHash("8d79dedc2fa0b55167b5d28c61dbad9ce1191a433f3a1a6c8ee291631b2c94c9".parse::>().unwrap()); let merkle_branch = TaprootMerkleBranch::from([hash1, hash2]); // use serde_test to test serialization and deserialization serde_test::assert_tokens( diff --git a/hashes/src/hmac.rs b/hashes/src/hmac.rs index a38ce7531..ecd5bc673 100644 --- a/hashes/src/hmac.rs +++ b/hashes/src/hmac.rs @@ -127,6 +127,7 @@ impl GeneralHash for Hmac { impl Hash for Hmac { type Bytes = T::Bytes; + #[allow(deprecated_in_future)] fn from_slice(sl: &[u8]) -> Result, FromSliceError> { T::from_slice(sl).map(Hmac) } fn to_byte_array(self) -> Self::Bytes { self.0.to_byte_array() } @@ -307,7 +308,7 @@ mod tests { 0x0b, 0x2d, 0x8a, 0x60, 0x0b, 0xdf, 0x4c, 0x0c, ]; - let hash = Hmac::::from_slice(&HASH_BYTES).expect("right number of bytes"); + let hash = Hmac::::from_byte_array(HASH_BYTES); assert_tokens(&hash.compact(), &[Token::BorrowedBytes(&HASH_BYTES[..])]); assert_tokens( &hash.readable(), diff --git a/hashes/src/internal_macros.rs b/hashes/src/internal_macros.rs index 9379bc7fa..320b29d7d 100644 --- a/hashes/src/internal_macros.rs +++ b/hashes/src/internal_macros.rs @@ -77,7 +77,7 @@ macro_rules! hash_trait_impls { } $crate::internal_macros::arr_newtype_fmt_impl!(Hash, $bits / 8 $(, $gen: $gent)*); - serde_impl!(Hash, $bits / 8 $(, $gen: $gent)*); + serde_impl!(Hash, { $bits / 8 } $(, $gen: $gent)*); borrow_slice_impl!(Hash $(, $gen: $gent)*); impl<$($gen: $gent),*> $crate::_export::_core::convert::AsRef<[u8; $bits / 8]> for Hash<$($gen),*> { @@ -97,6 +97,7 @@ macro_rules! hash_trait_impls { const DISPLAY_BACKWARD: bool = $reverse; + #[allow(deprecated_in_future)] fn from_slice(sl: &[u8]) -> $crate::_export::_core::result::Result, $crate::FromSliceError> { Self::from_slice(sl) } diff --git a/hashes/src/lib.rs b/hashes/src/lib.rs index 68c87032d..49fb05f47 100644 --- a/hashes/src/lib.rs +++ b/hashes/src/lib.rs @@ -265,6 +265,7 @@ pub trait Hash: const LEN: usize = Self::Bytes::LEN; /// Copies a byte slice into a hash object. + #[deprecated(since = "TBD", note = "use `from_byte_array` instead")] fn from_slice(sl: &[u8]) -> Result; /// Flag indicating whether user-visible serializations of this hash diff --git a/hashes/src/serde_macros.rs b/hashes/src/serde_macros.rs index ff542d29a..073ca1a0a 100644 --- a/hashes/src/serde_macros.rs +++ b/hashes/src/serde_macros.rs @@ -9,9 +9,14 @@ pub mod serde_details { use core::str::FromStr; use core::{fmt, str}; - use crate::FromSliceError; - struct HexVisitor(PhantomData); - use serde::{de, Deserializer, Serializer}; + use serde::de; + + /// Type used to implement serde traits for hashes as hex strings. + pub struct HexVisitor(PhantomData); + + impl Default for HexVisitor { + fn default() -> Self { Self(PhantomData) } + } impl<'de, ValueT> de::Visitor<'de> for HexVisitor where @@ -43,12 +48,17 @@ pub mod serde_details { } } - struct BytesVisitor(PhantomData); + /// Type used to implement serde traits for hashes as bytes. + pub struct BytesVisitor(PhantomData); - impl<'de, ValueT> de::Visitor<'de> for BytesVisitor + impl Default for BytesVisitor { + fn default() -> Self { Self(PhantomData) } + } + + impl<'de, ValueT, const N: usize> de::Visitor<'de> for BytesVisitor where - ValueT: SerdeHash, - ::Err: fmt::Display, + ValueT: crate::Hash, + ValueT: crate::Hash, { type Value = ValueT; @@ -60,41 +70,12 @@ pub mod serde_details { where E: de::Error, { - SerdeHash::from_slice_delegated(v).map_err(|_| { + let bytes = <[u8; N]>::try_from(v).map_err(|_| { // from_slice only errors on incorrect length E::invalid_length(v.len(), &stringify!(N)) - }) - } - } + })?; - /// Default serialization/deserialization methods. - pub trait SerdeHash - where - Self: Sized + FromStr + fmt::Display + crate::Hash, - ::Err: fmt::Display, - { - /// Size, in bits, of the hash. - const N: usize; - - /// Helper function to turn a deserialized slice into the correct hash type. - fn from_slice_delegated(sl: &[u8]) -> core::result::Result; - - /// Do serde serialization. - fn serialize(&self, s: S) -> core::result::Result { - if s.is_human_readable() { - s.collect_str(self) - } else { - s.serialize_bytes(::as_byte_array(self).as_ref()) - } - } - - /// Do serde deserialization. - fn deserialize<'de, D: Deserializer<'de>>(d: D) -> core::result::Result { - if d.is_human_readable() { - d.deserialize_str(HexVisitor::(PhantomData)) - } else { - d.deserialize_bytes(BytesVisitor::(PhantomData)) - } + Ok(::from_byte_array(bytes)) } } } @@ -105,22 +86,25 @@ pub mod serde_details { #[cfg(feature = "serde")] macro_rules! serde_impl( ($t:ident, $len:expr $(, $gen:ident: $gent:ident)*) => ( - impl<$($gen: $gent),*> $crate::serde_macros::serde_details::SerdeHash for $t<$($gen),*> { - const N : usize = $len; - fn from_slice_delegated(sl: &[u8]) -> core::result::Result { - <$t<$($gen),*> as $crate::Hash>::from_slice(sl) - } - } - impl<$($gen: $gent),*> $crate::serde::Serialize for $t<$($gen),*> { fn serialize(&self, s: S) -> core::result::Result { - $crate::serde_macros::serde_details::SerdeHash::serialize(self, s) + if s.is_human_readable() { + s.collect_str(self) + } else { + s.serialize_bytes(::as_byte_array(self)) + } } } impl<'de $(, $gen: $gent)*> $crate::serde::Deserialize<'de> for $t<$($gen),*> { fn deserialize>(d: D) -> core::result::Result<$t<$($gen),*>, D::Error> { - $crate::serde_macros::serde_details::SerdeHash::deserialize(d) + use $crate::serde_macros::serde_details::{BytesVisitor, HexVisitor}; + + if d.is_human_readable() { + d.deserialize_str(HexVisitor::::default()) + } else { + d.deserialize_bytes(BytesVisitor::::default()) + } } } )); diff --git a/hashes/src/util.rs b/hashes/src/util.rs index 935c40276..80ca72af7 100644 --- a/hashes/src/util.rs +++ b/hashes/src/util.rs @@ -190,12 +190,14 @@ macro_rules! hash_newtype { } $crate::hex_fmt_impl!(<$newtype as $crate::Hash>::DISPLAY_BACKWARD, <$newtype as $crate::Hash>::LEN, $newtype); - $crate::serde_impl!($newtype, <$newtype as $crate::Hash>::LEN); + $crate::serde_impl!($newtype, { <$newtype as $crate::Hash>::LEN }); $crate::borrow_slice_impl!($newtype); #[allow(unused)] // Private wrapper types may not need all functions. impl $newtype { /// Copies a byte slice into a hash object. + #[deprecated(since = "TBD", note = "use `from_byte_array` instead")] + #[allow(deprecated_in_future)] pub fn from_slice(sl: &[u8]) -> $crate::_export::_core::result::Result<$newtype, $crate::FromSliceError> { Ok($newtype(<$hash as $crate::Hash>::from_slice(sl)?)) } @@ -235,6 +237,7 @@ macro_rules! hash_newtype { const DISPLAY_BACKWARD: bool = $crate::hash_newtype_get_direction!($hash, $(#[$($type_attrs)*])*); #[inline] + #[allow(deprecated_in_future)] fn from_slice(sl: &[u8]) -> $crate::_export::_core::result::Result<$newtype, $crate::FromSliceError> { Self::from_slice(sl) }