Skip to content

Commit

Permalink
Merge rust-bitcoin#3222: Siphash24 cleanup
Browse files Browse the repository at this point in the history 
6e5bd47 Improve siphash's `as_u64` -> `to_u64` rename (Martin Habovstiak)
1a91492 Clean up the siphash mess (Martin Habovstiak)

Pull request description:

  Never break SSOT. Never break SSOT. Never break SSOT. Never break SSOT. Never break SSOT. Never break SSOT. Never break SSOT. ...

  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`.

  FTR both of our recent bugs were caused by breaking SSOT. It confirms again my life experience that breaking it will almost certainly bite one in the ass. If there is the most important principle in programming it's quite likely this one.

  I was trying to be more "friendly" towards contributors breaking it but I no longer feel that this is a good choice, especially for complicated PRs. Thus I plan to be more aggressive here and NACK pretty much all breaks of SSOT that aren't provably unsolvable.

ACKs for top commit:
  apoelstra:
    ACK 6e5bd47 successfully ran local tests

Tree-SHA512: 03f413f302a41d07b52ac4645552231bb136aad9eb15010758fbad935ac2a686287d1adab7637372dd2629c37434b6ff9a085fba6792b8b6ec6c2a357e99538e
  • Loading branch information
@apoelstra
apoelstra committed Aug 23, 2024
2 parents b4bda00 + 6e5bd47 commit 5651d8e
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 100 deletions.
57 changes: 32 additions & 25 deletions hashes/src/internal_macros.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) }
}

Expand Down Expand Up @@ -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 { <Self as crate::GeneralHash>::hash(data) }

/// Hashes all the byte slices retrieved from the iterator together.
pub fn hash_byte_chunks<B, I>(byte_slices: I) -> Self
where
B: AsRef<[u8]>,
I: IntoIterator<Item = B>,
{
<Self as crate::GeneralHash>::hash_byte_chunks(byte_slices)
}

/// Hashes the entire contents of the `reader`.
#[cfg(feature = "bitcoin-io")]
pub fn hash_reader<R: io::BufRead>(reader: &mut R) -> Result<Self, io::Error> {
<Self as crate::GeneralHash>::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)]
Expand All @@ -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) }

Expand All @@ -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 { <Self as crate::GeneralHash>::hash(data) }

/// Hashes all the byte slices retrieved from the iterator together.
pub fn hash_byte_chunks<B, I>(byte_slices: I) -> Self
where
B: AsRef<[u8]>,
I: IntoIterator<Item = B>,
{
<Self as crate::GeneralHash>::hash_byte_chunks(byte_slices)
}

/// Hashes the entire contents of the `reader`.
#[cfg(feature = "bitcoin-io")]
pub fn hash_reader<R: io::BufRead>(reader: &mut R) -> Result<Self, io::Error> {
<Self as crate::GeneralHash>::hash_reader(reader)
}

/// Returns the underlying byte array.
pub const fn to_byte_array(self) -> [u8; $bits / 8] { self.0 }

Expand Down Expand Up @@ -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;
84 changes: 9 additions & 75 deletions hashes/src/siphash24.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = <String>::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))]
Expand Down Expand Up @@ -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);
Expand All @@ -224,61 +203,16 @@ 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()) }

/// 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<Self, crate::FromSliceError> {
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, crate::FromSliceError> { 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<I: SliceIndex<[u8]>> Index<I> 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<Self, Self::Err> {
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`.
Expand Down

0 comments on commit 5651d8e

Please sign in to comment.