From 1a914922046bbc0df42163e25a39a97fadfa3b7c Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Thu, 22 Aug 2024 22:24:29 +0200 Subject: [PATCH 1/2] Clean up the siphash mess Previously we had removed `Default` impl on `siphash24::HashEngine` by reimplementing the type manually. This was a really bad idea as it inevitably led to API differences that broke the build which we didn't notice because of unrelated bug. It should've just split the macro from the start as was suggested but it was claimed to be difficult, I don't think was the case as can be seen by this PR. This commit does what the previous one should've done: it renames the macro to have `_no_default` suffix, creates another one of the original name that calls into `_no_default` one and moves anything related to `Default`. This cleanly ensures all previous hashes stay the same while siphash gets `Default` removed. This also removes all now-conflicting impls from `siphash24` module which makes the module almost identical to what it looked like before the change. The only differences are removed `Default`/`new`, fixes in tests and recent rename of `as_u64` to `to_u64`. --- hashes/src/internal_macros.rs | 57 ++++++++++++++----------- hashes/src/siphash24.rs | 78 ++--------------------------------- 2 files changed, 36 insertions(+), 99 deletions(-) diff --git a/hashes/src/internal_macros.rs b/hashes/src/internal_macros.rs index 0b66e41854..7eda9dc00a 100644 --- a/hashes/src/internal_macros.rs +++ b/hashes/src/internal_macros.rs @@ -102,8 +102,6 @@ macro_rules! hash_trait_impls { impl<$($gen: $gent),*> crate::GeneralHash for Hash<$($gen),*> { type Engine = HashEngine; - fn engine() -> HashEngine { Self::engine() } - fn from_engine(e: HashEngine) -> Hash<$($gen),*> { Self::from_engine(e) } } @@ -142,6 +140,37 @@ pub(crate) use hash_trait_impls; /// The `from_engine` free-standing function is still required with this macro. See the doc of /// [`hash_trait_impls`]. macro_rules! hash_type { + ($bits:expr, $reverse:expr, $doc:literal) => { + $crate::internal_macros::hash_type_no_default!($bits, $reverse, $doc); + + impl Hash { + /// Constructs a new engine. + pub fn engine() -> HashEngine { Default::default() } + + /// Hashes some bytes. + #[allow(clippy::self_named_constructors)] // Hash is a noun and a verb. + pub fn hash(data: &[u8]) -> Self { ::hash(data) } + + /// Hashes all the byte slices retrieved from the iterator together. + pub fn hash_byte_chunks(byte_slices: I) -> Self + where + B: AsRef<[u8]>, + I: IntoIterator, + { + ::hash_byte_chunks(byte_slices) + } + + /// Hashes the entire contents of the `reader`. + #[cfg(feature = "bitcoin-io")] + pub fn hash_reader(reader: &mut R) -> Result { + ::hash_reader(reader) + } + } + } +} +pub(crate) use hash_type; + +macro_rules! hash_type_no_default { ($bits:expr, $reverse:expr, $doc:literal) => { #[doc = $doc] #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] @@ -165,9 +194,6 @@ macro_rules! hash_type { unsafe { &mut *(bytes as *mut _ as *mut Self) } } - /// Constructs a new engine. - pub fn engine() -> HashEngine { Default::default() } - /// Produces a hash from the current state of a given engine. pub fn from_engine(e: HashEngine) -> Hash { from_engine(e) } @@ -184,25 +210,6 @@ macro_rules! hash_type { } } - /// Hashes some bytes. - #[allow(clippy::self_named_constructors)] // Hash is a noun and a verb. - pub fn hash(data: &[u8]) -> Self { ::hash(data) } - - /// Hashes all the byte slices retrieved from the iterator together. - pub fn hash_byte_chunks(byte_slices: I) -> Self - where - B: AsRef<[u8]>, - I: IntoIterator, - { - ::hash_byte_chunks(byte_slices) - } - - /// Hashes the entire contents of the `reader`. - #[cfg(feature = "bitcoin-io")] - pub fn hash_reader(reader: &mut R) -> Result { - ::hash_reader(reader) - } - /// Returns the underlying byte array. pub const fn to_byte_array(self) -> [u8; $bits / 8] { self.0 } @@ -234,4 +241,4 @@ macro_rules! hash_type { crate::internal_macros::hash_trait_impls!($bits, $reverse); }; } -pub(crate) use hash_type; +pub(crate) use hash_type_no_default; diff --git a/hashes/src/siphash24.rs b/hashes/src/siphash24.rs index 6aaed346a4..30d2c844eb 100644 --- a/hashes/src/siphash24.rs +++ b/hashes/src/siphash24.rs @@ -4,32 +4,14 @@ use core::ops::Index; use core::slice::SliceIndex; -use core::str::FromStr; use core::{cmp, mem, ptr}; -use hex::FromHex; - -use crate::internal_macros::arr_newtype_fmt_impl; use crate::HashEngine as _; -#[doc = "Output of the SipHash24 hash function."] -#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] -#[repr(transparent)] -pub struct Hash([u8; 8]); - -#[cfg(feature = "schemars")] -impl schemars::JsonSchema for Hash { - fn schema_name() -> String { "Hash".to_owned() } - - fn json_schema(gen: &mut schemars::gen::SchemaGenerator) -> schemars::schema::Schema { - let mut schema: schemars::schema::SchemaObject = ::json_schema(gen).into(); - schema.string = Some(Box::new(schemars::schema::StringValidation { - max_length: Some(8 * 2), - min_length: Some(8 * 2), - pattern: Some("[0-9a-fA-F]+".to_owned()), - })); - schema.into() - } +crate::internal_macros::hash_type_no_default! { + 64, + false, + "Output of the SipHash24 hash function." } #[cfg(not(hashes_fuzz))] @@ -196,9 +178,6 @@ impl Hash { Hash::from_engine(engine) } - /// Produces a hash from the current state of a given engine. - pub fn from_engine(e: HashEngine) -> Hash { from_engine(e) } - /// Hashes the given data directly to u64 with an engine with the provided keys. pub fn hash_to_u64_with_keys(k0: u64, k1: u64, data: &[u8]) -> u64 { let mut engine = HashEngine::with_keys(k0, k1); @@ -228,57 +207,8 @@ impl Hash { /// Creates a hash from its (little endian) 64-bit integer representation. pub fn from_u64(hash: u64) -> Hash { Hash(hash.to_le_bytes()) } - - /// Returns the underlying byte array. - pub const fn to_byte_array(self) -> [u8; 8] { self.0 } - - /// Returns a reference to the underlying byte array. - pub const fn as_byte_array(&self) -> &[u8; 8] { &self.0 } - - /// Constructs a hash from the underlying byte array. - pub const fn from_byte_array(bytes: [u8; 8]) -> Self { Self(bytes) } - - fn from_slice(sl: &[u8]) -> Result { - let mut ret = [0; 8]; - ret.copy_from_slice(sl); - Ok(Hash(ret)) - } } -impl crate::Hash for Hash { - type Bytes = [u8; 8]; - - const LEN: usize = 8; - const DISPLAY_BACKWARD: bool = false; - - fn from_slice(sl: &[u8]) -> Result { Self::from_slice(sl) } - - fn to_byte_array(self) -> Self::Bytes { self.to_byte_array() } - - fn as_byte_array(&self) -> &Self::Bytes { self.as_byte_array() } - - fn from_byte_array(bytes: Self::Bytes) -> Self { Self::from_byte_array(bytes) } -} - -impl> Index for Hash { - type Output = I::Output; - - #[inline] - fn index(&self, index: I) -> &Self::Output { &self.0[index] } -} - -impl FromStr for Hash { - type Err = hex::HexToArrayError; - fn from_str(s: &str) -> Result { - let bytes = <[u8; 8]>::from_hex(s)?; - Ok(Self::from_byte_array(bytes)) - } -} - -arr_newtype_fmt_impl!(Hash, 8); -serde_impl!(Hash, 8); -borrow_slice_impl!(Hash); - /// Load an u64 using up to 7 bytes of a byte slice. /// /// Unsafe because: unchecked indexing at `start..start+len`. From 6e5bd473a67d5a3ba701dd41de0a75e74991be57 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Thu, 22 Aug 2024 22:35:26 +0200 Subject: [PATCH 2/2] Improve siphash's `as_u64` -> `to_u64` rename The previous change was just a dumb rename with no deprecation and it also kept the `self` type which should be taken by value since the hash is `Copy`. This improves on it by adding a deprecated method of the original name and changing the type to be `self` instead of `&self`. --- hashes/src/siphash24.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/hashes/src/siphash24.rs b/hashes/src/siphash24.rs index 30d2c844eb..5ffd8a89f4 100644 --- a/hashes/src/siphash24.rs +++ b/hashes/src/siphash24.rs @@ -203,7 +203,11 @@ impl Hash { } /// Returns the (little endian) 64-bit integer representation of the hash value. - pub fn to_u64(&self) -> u64 { u64::from_le_bytes(self.0) } + #[deprecated(since = "TBD", note = "use `to_u64` instead")] + pub fn as_u64(&self) -> u64 { self.to_u64() } + + /// Returns the (little endian) 64-bit integer representation of the hash value. + pub fn to_u64(self) -> u64 { u64::from_le_bytes(self.0) } /// Creates a hash from its (little endian) 64-bit integer representation. pub fn from_u64(hash: u64) -> Hash { Hash(hash.to_le_bytes()) }