-
Notifications
You must be signed in to change notification settings - Fork 133
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
SHA256 #1285
Conversation
Speed up rotation of 3 values in SHA2
Express Maj with 2 XORs instead of 5
…mmon, and gadgets out of int.ts
There was a problem hiding this 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!
There was a problem hiding this 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?
I can answer that! 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 |
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.
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. |
There was a problem hiding this 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.
Implementation of static sized SHA2-256 https://csrc.nist.gov/pubs/fips/180-4/upd1/final