feat: add SHA-256 and SHA-512 wrapper implementations #692

Farukest · 2025-12-05T07:31:46Z

Describe your changes

Implements Hasher, HasherExt, and ElementHasher traits for SHA2 hash functions.

SHA-256 with 128-bit collision resistance
SHA-512 with 256-bit collision resistance
NIST test vectors for both algorithms
Property-based tests with proptest

Closes #689

Checklist before requesting a review

Repo forked and branch created from `next` according to naming convention.
Commit messages and codestyle follow conventions.
Relevant issues are linked in the PR description.
Tests added for new functionality.
Documentation/comments updated according to changes.

adr1anh

Looks great and thanks for taking this on. Not a full review, but I wonder if we could consolidate some code to be shared between SHA256/512 or even with other binary hash functions.

adr1anh · 2025-12-05T08:42:24Z

miden-crypto/src/hash/sha2/mod.rs

+}
+
+/// Cast the slice into contiguous bytes.
+fn prepare_merge<const N: usize, D>(args: &[D; N]) -> &[u8]


This feels like a more generally useful function that we might want to make available/reuse across different modules. Was this copied from another hash function wrapper?

Yes, this was copied from the blake and keccak modules which have identical implementations.

Can we share them instead of duplicating them?

miden-crypto/src/hash/sha2/mod.rs

Al-Kindi-0

Look good, thank you!

miden-crypto/src/hash/sha2/tests.rs

Farukest · 2025-12-09T09:48:27Z

Hi @huitseeker, I received email notifications about your review comments
( regarding #[repr(transparent)], memory layout tests, and SHA-512 truncation docs ),
but I can't see them on GitHub. I couldn't be sure if it was about github comment load issue or anything else..

In the meantime, I've addressed what I understood from the emails but did not push yet:

Added #[repr(transparent)] to digest types
Added memory layout and digests_as_bytes correctness tests
Added PropTest for merge_many
Documented SHA-512 truncation behavior (not SHA-512/256)
Added test demonstrating the difference

@bobbinth @adr1anh @Al-Kindi-0

miden-crypto/src/hash/sha2/mod.rs

huitseeker · 2025-12-09T16:13:40Z

miden-crypto/src/hash/sha2/mod.rs

+
+impl Digest for Sha512Digest {
+    fn as_bytes(&self) -> [u8; 32] {
+        // Return first 32 bytes of the 64-byte digest


This truncates SHA-512 to 32 bytes. That gives different output than the NIST SHA-512/256 standard.

For input "abc":

Truncated SHA-512 (this code): ddaf35a1...d55d39a

SHA-512/256 (NIST standard): 53048e26...07e7af23

SHA-512/256 uses different starting values, so truncating gives the wrong answer.

Please add a test showing what this actually does. Use a FIPS 140-3 test vector so it's clear this is truncated SHA-512, not SHA-512/256. Also add a comment on line 265 explaining the truncation.

If you need real SHA-512/256, the sha2 crate has Sha512_256.

Done in ae7bc92

huitseeker

@Farukest Yep, I was super unhappy with the tool that helps me add context to PR comments from my notes. So I deleted the comments, and they are now in a better shape.
The main item is being spectacularly clear in what we're doing with truncated SHA2 variants, which are not the ones that are specified in FPS-140 (those specified variants are using different IVs).

huitseeker

Actually, so sorry for the back and forth @Farukest but we won't be exposing truncated SHA512/256t, aka the truncated SHA-512 with the non-standard IVs, because it has zero usage footprint. We'd like to expose SHA-512/256 instead, which requires using the proper IVs. It's exposed in the sha2 crate as Sha512_256. Could you help make this change?

Edit: never mind, trying to figure out how to do something that makes sense here.

Ah, that makes no sense in the current shape of the PR. Never mind

huitseeker

We probably do not want to implement Digest for Sha512 for this PR. In the current shape of the trait because it forces the as_bytes function to have a 32 byte output while you're trying to expose a 64 byte hasher.

The appropriate output for the SHA-512 hasher is a 64 byte output and this forces you to use SHA-512 truncated to 256 bits with the "wrong" IVs. This is a function that has almost no usage.

What we would like to do instead is extend the Digest trait in winterfell so that it takes a const generic parameter, but providing a default for that const generic parameter to the value of 32, which will make all instances already using Digest implicitly mean Digest<32>.

In turn, this will modify the output of the as_bytes function so that the return type takes an array of bytes of a length depending on the const generic. This should let you implement Digest<64> for SHA-512. What do you think of this solution?

Farukest · 2025-12-09T18:30:14Z

In turn, this will modify the output of the as_bytes function so that the return type takes an array of bytes of a length depending on the const generic. This should let you implement Digest<64> for SHA-512. What do you think of this solution?

That sounds good to me @huitseeker . Then; I'll remove the Digest implementation for Sha512Digest
in this PR and keep it as a 64 byte hasher.
The generic Digest<N> change in Winterfell can be addressed separately ?

huitseeker · 2025-12-09T19:51:28Z

@Farukest That sounds great. The cherry on top is if you could use my comment to open an issue on Winterfell, and refer to it in a line of code comment on SHA-512 in this PR explaining why we're not implementing Digest ,

Farukest · 2025-12-09T23:28:05Z

I opened on winterfell @huitseeker and have one question that confuses my mind now.

The Hasher trait requires type Digest: Digest, so removing impl Digest for Sha512Digest also requires removing impl Hasher for Sha512. Should I remove both trait implementations
and keep only standalone methods, or keep the truncated Digest implementation until winterfell/issues/406 is resolved?

bobbinth · 2025-12-10T00:02:26Z

The Hasher trait requires type Digest: Digest, so removing impl Digest for Sha512Digest also requires removing impl Hasher for Sha512. Should I remove both trait implementations
and keep only standalone methods, or keep the truncated Digest implementation until winterfell/issues/406 is resolved?

I would remove the trait implementations and keep the standalone methods. The traits are there for hash functions that we use for STARK proof generation, and SHA512 is not one of these hash functions (at least not in the near future) - so, we don't have to implement these traits for it.

Implements Hasher, HasherExt, and ElementHasher traits for SHA2 hash functions. Includes NIST test vectors and property-based tests. Closes 0xMiden#689

- Add #[repr(transparent)] to Sha256Digest and Sha512Digest for safe pointer casting in digests_as_bytes() - Document that Sha512Digest::as_bytes() returns truncated SHA-512, NOT SHA-512/256 (different IVs per FIPS 180-4) - Add memory layout tests to verify struct size and alignment assumptions - Add digests_as_bytes correctness tests for both digest types - Add PropTest for merge_many to verify unsafe code produces correct results - Add test demonstrating SHA-512 truncation vs SHA-512/256 difference

Farukest · 2025-12-10T01:28:10Z

I would remove the trait implementations and keep the standalone methods. The traits are there for hash functions that we use for STARK proof generation, and SHA512 is not one of these hash functions (at least not in the near future) - so, we don't have to implement these traits for it.

Done @bobbinth
@huitseeker

SHA-512 produces 64-byte output which is incompatible with Winterfell's Digest trait (requires 32-byte as_bytes()). Standalone methods are kept. See facebook/winterfell#406

huitseeker

This looks great, thank you so much @Farukest !

Farukest force-pushed the issue-689-sha2-wrappers branch 2 times, most recently from 1336912 to 4273d74 Compare December 5, 2025 07:55

adr1anh reviewed Dec 5, 2025

View reviewed changes

bobbinth requested review from Al-Kindi-0 and huitseeker December 6, 2025 07:02

Al-Kindi-0 approved these changes Dec 7, 2025

View reviewed changes

miden-crypto/src/hash/sha2/tests.rs Outdated Show resolved Hide resolved

miden-crypto/src/hash/sha2/tests.rs Outdated Show resolved Hide resolved

Farukest mentioned this pull request Dec 8, 2025

Add generic Digest256 and Digest512 structs for binary hash functions #697

Open

huitseeker reviewed Dec 9, 2025

View reviewed changes

huitseeker previously requested changes Dec 9, 2025

View reviewed changes

huitseeker reviewed Dec 9, 2025

View reviewed changes

Farukest mentioned this pull request Dec 9, 2025

Make Digest trait generic over output size facebook/winterfell#406

Open

Farukest added 3 commits December 10, 2025 04:21

feat: add SHA-256 and SHA-512 wrapper implementations

3832ef9

Implements Hasher, HasherExt, and ElementHasher traits for SHA2 hash functions. Includes NIST test vectors and property-based tests. Closes 0xMiden#689

Update NIST test vector links to specific PDF documents

abf947b

Farukest force-pushed the issue-689-sha2-wrappers branch from 04224c8 to a97285a Compare December 10, 2025 01:22

Remove Digest/Hasher trait implementations for Sha512

47fd139

SHA-512 produces 64-byte output which is incompatible with Winterfell's Digest trait (requires 32-byte as_bytes()). Standalone methods are kept. See facebook/winterfell#406

Farukest force-pushed the issue-689-sha2-wrappers branch from a97285a to 47fd139 Compare December 10, 2025 11:10

huitseeker approved these changes Dec 10, 2025

View reviewed changes

huitseeker merged commit 2a24f0f into 0xMiden:next Dec 10, 2025
24 checks passed

feat: add SHA-256 and SHA-512 wrapper implementations #692

feat: add SHA-256 and SHA-512 wrapper implementations #692

Uh oh!

Conversation

Farukest commented Dec 5, 2025

Describe your changes

Checklist before requesting a review

Uh oh!

adr1anh left a comment

Choose a reason for hiding this comment

Uh oh!

adr1anh Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Farukest Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adr1anh Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Al-Kindi-0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Farukest commented Dec 9, 2025

Uh oh!

Uh oh!

Uh oh!

huitseeker Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Farukest Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

huitseeker left a comment

Choose a reason for hiding this comment

Uh oh!

huitseeker left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huitseeker left a comment

Choose a reason for hiding this comment

Uh oh!

Farukest commented Dec 9, 2025

Uh oh!

huitseeker commented Dec 9, 2025

Uh oh!

Farukest commented Dec 9, 2025

Uh oh!

bobbinth commented Dec 10, 2025

Uh oh!

Farukest commented Dec 10, 2025

Uh oh!

huitseeker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Farukest Dec 5, 2025 •

edited

Loading

huitseeker Dec 9, 2025 •

edited

Loading

huitseeker left a comment •

edited

Loading