feat: add generic Digest256 and Digest512 structs - #777
Conversation
Introduce reusable generic digest types for binary hash functions to reduce code duplication. - Add digest.rs with Digest256 (32-byte) and Digest512 (64-byte) types - Refactor Sha256Digest and Sha512Digest to type aliases - Remove ~130 lines of duplicate struct implementations - Update tests to use new constructor syntax Closes 0xMiden#697
- Remove square bracket links for Digest256 and Digest512 in module docs to fix rustdoc broken-intra-doc-links errors - Add PR 0xMiden#777 entry to CHANGELOG.md
| /// # Safety | ||
| /// This function uses unsafe code to reinterpret the slice of digests as bytes. | ||
| /// This is safe because `Digest256` is `#[repr(transparent)]` over `[u8; 32]`. |
There was a problem hiding this comment.
nit: the safety argument is an implementation detail, and is redundant with the existing one on line 58, so I would just remove it
| #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] | ||
| #[cfg_attr(feature = "serde", serde(into = "String", try_from = "&str"))] | ||
| #[repr(transparent)] | ||
| pub struct Digest256([u8; DIGEST256_BYTES]); |
There was a problem hiding this comment.
We could instead have just a single const-generic Digest struct defined as
// defaults to 32 bytes
pub struct Digest<const N: usize = 32>([u8; N]);
// example usage
pub type Sha256Digest = Digest;
pub type Sha512Digest = Digest<64>;I believe this is also what was alluded to in #692 (review).
Address review feedback: - Replace separate Digest256 and Digest512 structs with single const-generic Digest<N> - Add Digest256 and Digest512 as type aliases for backward compatibility - Remove redundant safety documentation comment - Update CHANGELOG entry
Thank you for detaileed guide @plafer . All done for now. Kindly needs check. |
plafer
left a comment
There was a problem hiding this comment.
LGTM (modulo small suggestions in this review), thank you!
Will still wait for @huitseeker to take a look before we merge though
|
Done @plafer 👍 |
huitseeker
left a comment
There was a problem hiding this comment.
This is generally in good shape, but leaves a few simplifications on the table, see comments.
| } | ||
|
|
||
| /// Converts a slice of digests into a contiguous byte slice. | ||
| pub fn digests_as_bytes(digests: &[Digest<N>]) -> &[u8] { |
There was a problem hiding this comment.
This prepare_merge function is duplicated in sha2, keccak, and blake modules. Consider moving it here and exporting it so all three can share it.
| // ================================================================================================ | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Nit: I'd add a test_memory_layout_assumptions test here to verify size_of and align_of match the inner array. The unsafe code in digests_as_bytes and prepare_merge relies on this.
| } | ||
| /// | ||
| /// This is a type alias to the generic [`Digest256`] type. | ||
| pub type Sha256Digest = Digest256; |
There was a problem hiding this comment.
The same pattern works for Keccak256Digest. You can make it a type alias to Digest256 and remove ~80 lines of duplicate code from keccak/mod.rs.
…ests - Move prepare_merge helper function from sha2, blake, and keccak modules to the shared digest module - Add test_memory_layout_assumptions test to verify Digest<N> has same size and alignment as [u8; N] - Update keccak module to use Digest256 type alias instead of custom Keccak256Digest struct - Remove redundant DIGEST_BYTES constant in favor of DIGEST256_BYTES
All done @huitseeker . kindly need your check again 👍 |
plafer
left a comment
There was a problem hiding this comment.
LGTM after latest changes, thank you!
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a few comments inline.
| #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] | ||
| #[cfg_attr(feature = "serde", serde(into = "String", try_from = "&str"))] | ||
| #[repr(transparent)] | ||
| pub struct Blake3Digest<const N: usize>([u8; N]); |
There was a problem hiding this comment.
Maybe for another PR, but would be good to use the Digest struct for BLAKE3 digest as well. For this, we may need to add the 24-byte variant there.
- Move changelog entry to 0.22.0 section - Replace prepare_merge function with Digest::digests_as_bytes() - Make digest module crate-private (pub(crate)) - Use type-specific digests_as_bytes in merge implementations
|
@bobbinth thanks. handled all but last, for another PR. |
Remove brackets from doc links to `Digest256` and `Digest512` since these types are not public, causing rustdoc to fail with `rustdoc::private-intra-doc-links` errors.
bobbinth
left a comment
There was a problem hiding this comment.
All looks good! Thank you!
Introduces generic
Digest256andDigest512structs to reduce code duplication across hash function implementations.Changes:
digest.rsmodule: Contains reusableDigest256(32-byte) andDigest512(64-byte) digest types with full trait implementations (Default,Deref,From/Into,Serializable,Deserializable, serde support, hex conversion)Sha256DigestandSha512Digestare now type aliases to the generic types, removing ~130 lines of duplicate code::from()constructor syntaxThis lays the groundwork for other hash functions (Blake3, Keccak) to potentially reuse these generic digest types in the future.
Closes #697
Checklist before requesting a review
nextaccording to naming convention.