chore: remove unused hash variants from proof of work hashing sources #6765
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
This pull request has conflicts, please rebase. |
WalkthroughThis set of changes removes support for all 224-, 256-, and 384-bit variants of the X11 hash function components, leaving only the 512-bit variants in each algorithm. The updates affect both implementation (.c) and header (.h) files for BLAKE, BMW, CubeHash, ECHO, Groestl, JH, Keccak, Luffa, SHAvite, SIMD, and Skein. All code, context structures, macros, constants, and public API functions related to the smaller output sizes are deleted. The AES helper code is also simplified to remove big-endian support, retaining only little-endian processing. Additionally, extensive documentation and some type definitions are removed from the core types header. Estimated code review effort5 (~150 minutes) ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/crypto/x11/blake.c (1)
578-628: Theout_size_w64parameter is always 8 for BLAKE-512Since all other BLAKE variants have been removed, the
out_size_w64parameter inblake64_closewill always be 8 (for 512-bit output). The conditional checks on lines 609 and 620 are now redundant.Consider simplifying the code by removing the conditional checks:
- if (out_size_w64 == 8) - u.buf[111] |= 1; + u.buf[111] |= 1;And similarly for line 620:
- if (out_size_w64 == 8) - u.buf[111] = 1; + u.buf[111] = 1;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
src/crypto/x11/aes_helper.c(1 hunks)src/crypto/x11/blake.c(1 hunks)src/crypto/x11/bmw.c(0 hunks)src/crypto/x11/cubehash.c(0 hunks)src/crypto/x11/echo.c(0 hunks)src/crypto/x11/groestl.c(0 hunks)src/crypto/x11/jh.c(0 hunks)src/crypto/x11/keccak.c(0 hunks)src/crypto/x11/luffa.c(1 hunks)src/crypto/x11/shavite.c(0 hunks)src/crypto/x11/simd.c(0 hunks)src/crypto/x11/skein.c(0 hunks)src/crypto/x11/sph_blake.h(0 hunks)src/crypto/x11/sph_bmw.h(0 hunks)src/crypto/x11/sph_cubehash.h(0 hunks)src/crypto/x11/sph_echo.h(0 hunks)src/crypto/x11/sph_groestl.h(0 hunks)src/crypto/x11/sph_jh.h(0 hunks)src/crypto/x11/sph_keccak.h(0 hunks)src/crypto/x11/sph_luffa.h(0 hunks)src/crypto/x11/sph_shavite.h(0 hunks)src/crypto/x11/sph_simd.h(0 hunks)src/crypto/x11/sph_skein.h(0 hunks)src/crypto/x11/sph_types.h(0 hunks)
🧠 Learnings (2)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: knst
PR: dashpay/dash#6533
File: test/functional/feature_llmq_singlenode.py:98-106
Timestamp: 2025-01-22T08:33:31.405Z
Learning: When reviewing PRs, ensure that suggestions are directly related to the PR's primary objectives rather than general code improvements that could be addressed separately.
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:15-250
Timestamp: 2025-07-23T09:28:32.758Z
Learning: In refactoring PRs like #6761, kwvg prefers to defer code formatting fixes to separate follow-up PRs when formatting is not the primary objective, to maintain focus on the structural changes and avoid scope creep.
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.592Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Learnt from: kwvg
PR: dashpay/dash#6752
File: src/wallet/load.cpp:164-164
Timestamp: 2025-07-17T15:48:29.418Z
Learning: In Dash Core, kwvg prefers using assert() statements to document expected behavior in code paths, especially when the called function also has internal asserts. This is used as a defensive programming pattern rather than requiring conditional error handling.
src/crypto/x11/aes_helper.c (1)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/crypto/{ctaes,x11}/** : Do not make changes under any circumstances to vendored dependencies in src/crypto/ctaes and src/crypto/x11
💤 Files with no reviewable changes (21)
- src/crypto/x11/cubehash.c
- src/crypto/x11/jh.c
- src/crypto/x11/sph_echo.h
- src/crypto/x11/sph_groestl.h
- src/crypto/x11/sph_jh.h
- src/crypto/x11/sph_cubehash.h
- src/crypto/x11/sph_simd.h
- src/crypto/x11/sph_skein.h
- src/crypto/x11/keccak.c
- src/crypto/x11/sph_types.h
- src/crypto/x11/sph_keccak.h
- src/crypto/x11/echo.c
- src/crypto/x11/sph_luffa.h
- src/crypto/x11/sph_bmw.h
- src/crypto/x11/sph_shavite.h
- src/crypto/x11/groestl.c
- src/crypto/x11/sph_blake.h
- src/crypto/x11/simd.c
- src/crypto/x11/skein.c
- src/crypto/x11/bmw.c
- src/crypto/x11/shavite.c
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: knst
PR: dashpay/dash#6533
File: test/functional/feature_llmq_singlenode.py:98-106
Timestamp: 2025-01-22T08:33:31.405Z
Learning: When reviewing PRs, ensure that suggestions are directly related to the PR's primary objectives rather than general code improvements that could be addressed separately.
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:15-250
Timestamp: 2025-07-23T09:28:32.758Z
Learning: In refactoring PRs like #6761, kwvg prefers to defer code formatting fixes to separate follow-up PRs when formatting is not the primary objective, to maintain focus on the structural changes and avoid scope creep.
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.592Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Learnt from: kwvg
PR: dashpay/dash#6752
File: src/wallet/load.cpp:164-164
Timestamp: 2025-07-17T15:48:29.418Z
Learning: In Dash Core, kwvg prefers using assert() statements to document expected behavior in code paths, especially when the called function also has internal asserts. This is used as a defensive programming pattern rather than requiring conditional error handling.
src/crypto/x11/aes_helper.c (1)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/crypto/{ctaes,x11}/** : Do not make changes under any circumstances to vendored dependencies in src/crypto/ctaes and src/crypto/x11
🔇 Additional comments (5)
src/crypto/x11/luffa.c (2)
244-256: LGTM!The parallel mode word mixing implementation correctly handles 64-bit operations by splitting into 32-bit parts and applying the specified rotations.
260-289: LGTM!The non-parallel mode implementations of SUB_CRUMB and MIX_WORD correctly implement the Luffa algorithm's substitution and mixing operations.
src/crypto/x11/blake.c (1)
1-668: LGTM! Clean removal of unused BLAKE variantsThe removal of BLAKE-224, BLAKE-256, and BLAKE-384 variants is complete and thorough. The remaining BLAKE-512 implementation is intact and functional. This aligns well with the PR objective to optimize X11 proof of work by removing unused hash variants.
src/crypto/x11/aes_helper.c (2)
327-348: AES round macros correctly implement little-endian processing.The
AES_ROUND_LEandAES_ROUND_NOKEY_LEmacros are technically sound:
- Proper bit shifting and masking for AES table lookups
- Standard AES round transformation using the 4 lookup tables
- Correct parameter handling for state and round keys
The unconditional definition (removing big-endian variants) aligns with the PR's goal of simplifying unused code paths.
51-51: Confirm modifications to vendored dependency insrc/crypto/x11/aes_helper.cThe change unconditionally defines
#define AESx(x) SPH_C32(x)and removes all big-endian support, which aligns with the PR’s goal of trimming unused variants. However, according to project guidelines, files under
src/crypto/x11are vendored and typically shouldn’t be altered in place.Please verify that this edit is intentional and properly documented:
- Confirm that you intend to maintain a forked copy of the X11 AES helper rather than preserving upstream compatibility.
- Ensure no other X11 source files or build rules depend on the removed big-endian logic.
- Consider adding a note in the top-of-file header or the PR description explaining why vendored code was modified.
4 participants
Motivation
Dash utilizes a daisy-chain of 11 hash algorithms for its proof of work termed X11. The library that provided the implementation of the underlying hash algorithms is
sphlibby Thomas Pornin (source, Internet Archive). The library has been a part of Dash Core since inception (f164aea) and does what it says on the tin quite well.Though, it's been a solid decade since and performance profiling has shown that proof of work hashing takes up a not-insignificant amount of time. As an alternative to (or alongside) dash#6610, we intend to work on improving the performance of X11 while maintaining readability and auditability.
To begin with that, we are removing variants that Dash doesn't use, namely, non 512-bit variants of the constituent algorithms used and subsequent pull requests will be integrating the library's contents with primitives available in Dash Core to allow for attributable and reasonable performance improvements.
Breaking Changes
None expected.
Checklist