Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SHA256 #1285

Merged
merged 58 commits into from
Jan 17, 2024
Merged

SHA256 #1285

merged 58 commits into from
Jan 17, 2024

Conversation

Trivo25
Copy link
Member

@Trivo25 Trivo25 commented Dec 1, 2023

Implementation of static sized SHA2-256 https://csrc.nist.gov/pubs/fips/180-4/upd1/final

@Trivo25 Trivo25 changed the title sha256 :) sha256 Dec 1, 2023
Base automatically changed from feature/gadgets-uint to main December 19, 2023 10:27
@Trivo25 Trivo25 marked this pull request as ready for review December 19, 2023 14:22
@Trivo25 Trivo25 requested review from mitschabaude and a team as code owners December 19, 2023 14:22
Copy link
Member

@mitschabaude mitschabaude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All my comments are addressed now!

Copy link
Contributor

@rbonichon rbonichon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to re-read the sha256 implementation. I'm assuming it's correct or tests would break. But I'll need to verify.

It does seem strange to use little-endian conversions and always have to reverse them. What would be the reason?

src/lib/gadgets/bit-slices.ts Show resolved Hide resolved
src/lib/gadgets/bit-slices.ts Show resolved Hide resolved
src/lib/gadgets/bit-slices.ts Show resolved Hide resolved
src/lib/gadgets/bit-slices.ts Show resolved Hide resolved
src/lib/gadgets/sha256.ts Outdated Show resolved Hide resolved
src/lib/gadgets/sha256.ts Show resolved Hide resolved
src/lib/gadgets/sha256.ts Show resolved Hide resolved
src/lib/gadgets/sha256.ts Show resolved Hide resolved
@mitschabaude
Copy link
Member

mitschabaude commented Jan 16, 2024

It does seem strange to use little-endian conversions and always have to reverse them. What would be the reason?

I can answer that! bytesToWord and wordsToBytes are not introduced in this PR, but existed earlier, for use in the Keccak gadget, where we used their little-endian versions.

Also, a general comment from me is that little-endian bytes layout is by far the more common overall, and also more logical IMO, or in other words, SHA256 is weird here.

So, I don't think we need to add a "le" suffix to methods that use little-endian convention (we have many places where that would need to be added..). I would treat little-endian as the general default and instead use a "be" suffix for methods that use big-endian convention

@rbonichon
Copy link
Contributor

rbonichon commented Jan 16, 2024

Also, a general comment from me is that little-endian bytes layout is by far the more common overall, and also more logical IMO, or in other words, SHA256 is weird here.

It is the most common these days in hardware architecture but most network protocols are big-endian (very recent example being the transfer protocol in the Optimism Cannon VM). Human text in latin languages or numbers are big-endian too. All this is just a matter of convention. I would not treat either of these as being "obviously" the one used in any setting.

So, I don't think we need to add a "le" suffix to methods that use little-endian convention (we have many places where that would need to be added..). I would treat little-endian as the general default and instead use a "be" suffix for methods that use big-endian convention

My suggestion to add endianness in the name was more akin to following the principle of least astonishment. Rust or OCaml, for example, do that in their integer to bytes converter. I understand it might not be done in this PR but you could consider to have both le and be converters it in a follow-up issue. And that would avoid the inelegant (and not super-efficient) reverse call above.

Copy link
Contributor

@rbonichon rbonichon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation looks correct.

src/lib/gadgets/sha256.ts Show resolved Hide resolved
src/lib/gadgets/sha256.ts Outdated Show resolved Hide resolved
@Trivo25 Trivo25 merged commit bc945a0 into main Jan 17, 2024
13 checks passed
@Trivo25 Trivo25 deleted the dog-food-sha256 branch January 17, 2024 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants