ARM SHA256 intrinsics take state in natural order: ABCD + EFGH (hash_abcd/hash_efgh). Documented here
The code already uses that layout, only the ABEF_SAVE/CDGH_SAVE names were wrong.
Rename to ABCD_SAVE/EFGH_SAVE. No logic change.
Fix in original C code (Jeffrey): https://github.com/noloader/SHA-Intrinsics/pull/14
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | l0rinc |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Concept ACK.
Based on the description and the code path, this looks correct: STATE0 maps to s[0..3] (vld1q_u32(ptr) loads 4 consecutive lanes according to the docs) (and STATE1 is similar), so ABCD_SAVE / EFGH_SAVE is semantically aligned.
I checked other projects (e.g. https://botan.randombit.net/doxygen/sha1__armv8_8cpp_source.html#l00037) and did a compared the local binary and this appears to be a correct rename-only clarity fix with no logic change.
Consider keeping the scripted rename split into two explicit substitutions to make review simpler, e.g.
perl -pi -e 's/\bABEF_SAVE([AB])?\b/ABCD_SAVE$1/g' src/crypto/sha256_arm_shani.cppperl -pi -e 's/\bCDGH_SAVE([AB])?\b/EFGH_SAVE$1/g' src/crypto/sha256_arm_shani.cpp
ARM SHA256 intrinsics take state in natural order: ABCD + EFGH
(hash_abcd/hash_efgh). The code already uses that layout, only the
ABEF_SAVE/CDGH_SAVE names were wrong. Rename to ABCD_SAVE/EFGH_SAVE.
No logic change.
Docs: https://developer.arm.com/architectures/instruction-sets/intrinsics/#f:@navigationhierarchiesinstructiongroup=[Cryptography,SHA256]
Fix in original C code (Jeffrey): https://github.com/noloader/SHA-Intrinsics/pull/14
-BEGIN VERIFY SCRIPT-
perl -pi -e 's/\bABEF_SAVE([AB])?\b/ABCD_SAVE$1/g' src/crypto/sha256_arm_shani.cpp
perl -pi -e 's/\bCDGH_SAVE([AB])?\b/EFGH_SAVE$1/g' src/crypto/sha256_arm_shani.cpp
-END VERIFY SCRIPT-
sorry, missed that. redone now as a scripted-diff. not sure tho if CI failing is related to this PR ?
also, I noticed that in several places we use ARM_SHANI. this might be a bit misleading, since as far as I know SHA-NI is Intel/x86 related and has nothing to do with ARM. x86_shani makes sense, but I’m not sure arm_shani is the right term here.
since the term is already used in many places, it may not be worth changing right now, but happy to follow up with a PR if we want to clean that up.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task i686, no IPC: https://github.com/bitcoin/bitcoin/actions/runs/25274080662/job/74101068747</sub>
<sub>LLM reason (✨ experimental): CI failed because the sock_tests unit tests crashed with a fatal assertion (s != static_cast<SOCKET>(-1)), causing 6 test failures in the Bitcoin Core test suite.</sub>
<details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
ACK 86718e4589682529932050dc08998fbb7bd86cef
Compared to previous push it's been rebased but the modified file is the same, only the commit message was adjusted.
It's a pure refactor, before/after produces the exact same assembly, see https://godbolt.org/z/jz351hWcb
The variable rename looks correct independently of the broader ARM_SHANI naming.
The naming concern about ARM_SHANI seems valid, SHA-NI appears to be Intel terminology - should be done in a separate PR (not sure anyone will review it, though).
The CI failure is unrelated, should be fixed after https://github.com/bitcoin/bitcoin/pull/35202
SHA-NI is actually not a thing at all. The name was chosen analoguously to the AES-NI instructions on x86, but that is not how they're called there either.
If we'd want proper naming, maybe "sha_native" or so, which could be used on both x86 and ARM. Not urgent, and out of scope for this PR, I think.
SHA-NI appears to be Intel terminology
SHA-NI is actually not a thing at all
My understanding is that the NI is simply referring to the family of New Instructions (i.e. https://www.intel.com/content/www/us/en/developer/articles/technical/intel-sha-extensions.html) - but I couldn't find the same naming for other platforms.