backport: merge bitcoin#25058, #25107, #26481, #27598, #29180, #29407, #29528, #28893, #29834, partial bitcoin#26158, #25200, #24322 (auxiliary backports: part 25) #6752
Conversation
kwvg
changed the title
kwvg
force-pushed
the
auxports25
branch
2 times, most recently
from
c80c07c to
aa0d871
Compare
WalkthroughThis set of changes primarily restructures build system logic, enhances the benchmarking framework, and reorganizes RPC command handling. The build configuration and Makefiles are updated to remove all assembly-related options, migrate from static archives to libtool convenience libraries, and enforce static linking for several internal libraries. Optimized SHA256 implementation selection is centralized and made more configurable through a new enum and runtime parameter, with associated code guarded by a new macro. The benchmarking framework is extended with priority levels, improved argument parsing, and a "sanity check" mode. Benchmarks are refactored for explicit CPU variant testing and clearer naming. RPC commands related to address and descriptor handling are moved to a new file, and the registration process is reorganized accordingly. Minor functional changes include thread-safe cancellation in BLS benchmarks, ECC context management in ECDSA benchmarks, initialization of the stats client in a block check benchmark, and guarded calls to the coinjoin loader in wallet logic. Documentation is updated to reflect these build and feature changes. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)src/**/*.{cpp,h,cc,cxx,hpp}Instructions used from: Sources:
🧠 Learnings (2)📓 Common learningssrc/rpc/output_script.cpp (1)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (5)
✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/bench/bls.cpp (1)
316-324: Fix clang-format issues.The pipeline indicates formatting issues in this range. Please run clang-format to fix the formatting.
src/rpc/output_script.cpp (1)
67-67: Fix minor syntax errorThere's a double semicolon in the scriptPubKey assignment.
Apply this diff to fix the syntax error:
- ret.pushKV("scriptPubKey", HexStr(scriptPubKey));; + ret.pushKV("scriptPubKey", HexStr(scriptPubKey));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
configure.ac(1 hunks)doc/developer-notes.md(0 hunks)doc/fuzzing.md(1 hunks)src/Makefile.am(8 hunks)src/Makefile.bench.include(1 hunks)src/Makefile.crc32c.include(2 hunks)src/Makefile.leveldb.include(2 hunks)src/Makefile.qt.include(1 hunks)src/Makefile.qttest.include(1 hunks)src/Makefile.test.include(3 hunks)src/Makefile.test_fuzz.include(0 hunks)src/Makefile.test_util.include(0 hunks)src/bench/bench.cpp(3 hunks)src/bench/bench.h(1 hunks)src/bench/bench_bitcoin.cpp(5 hunks)src/bench/bls.cpp(3 hunks)src/bench/checkblock.cpp(2 hunks)src/bench/crypto_hash.cpp(2 hunks)src/bench/ecdsa.cpp(4 hunks)src/crypto/sha256.cpp(7 hunks)src/crypto/sha256.h(1 hunks)src/crypto/sha256_x86_shani.cpp(1 hunks)src/rpc/node.cpp(1 hunks)src/rpc/output_script.cpp(1 hunks)src/rpc/register.h(2 hunks)src/wallet/wallet.cpp(1 hunks)
💤 Files with no reviewable changes (3)
- doc/developer-notes.md
- src/Makefile.test_util.include
- src/Makefile.test_fuzz.include
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
src/bench/**/*.{cpp,h,cc,cxx,hpp}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
doc/**
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 Learnings (21)
📓 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: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Favor extension over modification of the Bitcoin Core foundation
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue}/** : Do not make changes under any circumstances to vendored dependencies in src/crc32c, src/dashbls, src/gsl, src/immer, src/leveldb, src/minisketch, src/secp256k1, src/univalue
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests should be implemented in src/test/, src/wallet/test/, and src/qt/test/ using Boost::Test or Qt 5 for GUI tests
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be implemented in src/bench/ using nanobench
src/Makefile.qttest.include (2)
Learnt from: kwvg
PR: dashpay/dash#6516
File: depends/patches/gmp/include_ldflags_in_configure.patch:557-621
Timestamp: 2025-01-06T09:51:03.167Z
Learning: The `GMP_GCC_ARM_UMODSI` macro checks only the compiler version, and `GMP_GCC_MIPS_O32` relies on the `-mabi=32` flag. Therefore, `$LDFLAGS` is irrelevant to these tests.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests should be implemented in src/test/, src/wallet/test/, and src/qt/test/ using Boost::Test or Qt 5 for GUI tests
src/bench/ecdsa.cpp (1)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be implemented in src/bench/ using nanobench
src/bench/bls.cpp (1)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be implemented in src/bench/ using nanobench
src/Makefile.test.include (5)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/fuzz/**/*.{cpp,h,cc,cxx,hpp} : Fuzzing harnesses should be implemented in src/fuzz/
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be implemented in src/bench/ using nanobench
Learnt from: kwvg
PR: dashpay/dash#6516
File: depends/patches/gmp/include_ldflags_in_configure.patch:557-621
Timestamp: 2025-01-06T09:51:03.167Z
Learning: The `GMP_GCC_ARM_UMODSI` macro checks only the compiler version, and `GMP_GCC_MIPS_O32` relies on the `-mabi=32` flag. Therefore, `$LDFLAGS` is irrelevant to these tests.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests should be implemented in src/test/, src/wallet/test/, and src/qt/test/ using Boost::Test or Qt 5 for GUI tests
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/**/*.{cpp,h,cc,cxx,hpp} : Dash Core codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
src/bench/checkblock.cpp (1)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be implemented in src/bench/ using nanobench
src/Makefile.bench.include (3)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be implemented in src/bench/ using nanobench
Learnt from: kwvg
PR: dashpay/dash#6516
File: depends/patches/gmp/include_ldflags_in_configure.patch:557-621
Timestamp: 2025-01-06T09:51:03.167Z
Learning: The `GMP_GCC_ARM_UMODSI` macro checks only the compiler version, and `GMP_GCC_MIPS_O32` relies on the `-mabi=32` flag. Therefore, `$LDFLAGS` is irrelevant to these tests.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests should be implemented in src/test/, src/wallet/test/, and src/qt/test/ using Boost::Test or Qt 5 for GUI tests
doc/fuzzing.md (2)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/fuzz/**/*.{cpp,h,cc,cxx,hpp} : Fuzzing harnesses should be implemented in src/fuzz/
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/**/*.{cpp,h,cc,cxx,hpp} : Dash Core codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
src/wallet/wallet.cpp (3)
undefined
<retrieved_learning>
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
</retrieved_learning>
<retrieved_learning>
Learnt from: kwvg
PR: #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.
</retrieved_learning>
<retrieved_learning>
Learnt from: kwvg
PR: #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.
</retrieved_learning>
src/Makefile.qt.include (4)
Learnt from: kwvg
PR: dashpay/dash#6516
File: depends/patches/gmp/include_ldflags_in_configure.patch:557-621
Timestamp: 2025-01-06T09:51:03.167Z
Learning: The `GMP_GCC_ARM_UMODSI` macro checks only the compiler version, and `GMP_GCC_MIPS_O32` relies on the `-mabi=32` flag. Therefore, `$LDFLAGS` is irrelevant to these tests.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue}/** : Do not make changes under any circumstances to vendored dependencies in src/crc32c, src/dashbls, src/gsl, src/immer, src/leveldb, src/minisketch, src/secp256k1, src/univalue
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests should be implemented in src/test/, src/wallet/test/, and src/qt/test/ using Boost::Test or Qt 5 for GUI tests
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.
src/Makefile.leveldb.include (1)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue}/** : Do not make changes under any circumstances to vendored dependencies in src/crc32c, src/dashbls, src/gsl, src/immer, src/leveldb, src/minisketch, src/secp256k1, src/univalue
configure.ac (1)
Learnt from: kwvg
PR: dashpay/dash#6516
File: depends/patches/gmp/include_ldflags_in_configure.patch:557-621
Timestamp: 2025-01-06T09:51:03.167Z
Learning: The `GMP_GCC_ARM_UMODSI` macro checks only the compiler version, and `GMP_GCC_MIPS_O32` relies on the `-mabi=32` flag. Therefore, `$LDFLAGS` is irrelevant to these tests.
src/bench/bench.h (1)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be implemented in src/bench/ using nanobench
src/bench/bench_bitcoin.cpp (3)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be implemented in src/bench/ using nanobench
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: 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.
src/rpc/node.cpp (3)
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
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.
src/crypto/sha256.cpp (2)
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: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue}/** : Do not make changes under any circumstances to vendored dependencies in src/crc32c, src/dashbls, src/gsl, src/immer, src/leveldb, src/minisketch, src/secp256k1, src/univalue
src/rpc/output_script.cpp (1)
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
src/Makefile.crc32c.include (2)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue}/** : Do not make changes under any circumstances to vendored dependencies in src/crc32c, src/dashbls, src/gsl, src/immer, src/leveldb, src/minisketch, src/secp256k1, src/univalue
Learnt from: kwvg
PR: dashpay/dash#6516
File: depends/patches/gmp/include_ldflags_in_configure.patch:557-621
Timestamp: 2025-01-06T09:51:03.167Z
Learning: The `GMP_GCC_ARM_UMODSI` macro checks only the compiler version, and `GMP_GCC_MIPS_O32` relies on the `-mabi=32` flag. Therefore, `$LDFLAGS` is irrelevant to these tests.
src/Makefile.am (4)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue}/** : Do not make changes under any circumstances to vendored dependencies in src/crc32c, src/dashbls, src/gsl, src/immer, src/leveldb, src/minisketch, src/secp256k1, src/univalue
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
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
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests should be implemented in src/test/, src/wallet/test/, and src/qt/test/ using Boost::Test or Qt 5 for GUI tests
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/**/*.{cpp,h,cc,cxx,hpp} : Dash uses unordered_lru_cache for efficient caching with LRU eviction
src/bench/bench.cpp (1)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be implemented in src/bench/ using nanobench
src/bench/crypto_hash.cpp (3)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be implemented in src/bench/ using nanobench
Learnt from: knst
PR: dashpay/dash#6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions `MigrateDBIfNeeded` and `MigrateDBIfNeeded2` in `src/evo/deterministicmns.cpp` are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Learnt from: kwvg
PR: dashpay/dash#6532
File: src/test/fuzz/netaddress.cpp:83-84
Timestamp: 2025-01-14T09:06:19.717Z
Learning: In fuzzer harness tests, CServiceHash can be used with both default constructor (CServiceHash()) and parameterized constructor (CServiceHash(salt_k0, salt_k1)) to test different variants. The usage pattern CServiceHash()(service) and CServiceHash(0, 0)(service) is valid and intentional in such tests.
🧬 Code Graph Analysis (2)
src/bench/bench_bitcoin.cpp (1)
src/bench/bench.cpp (6)
ListPriorities(55-61)ListPriorities(55-55)StringToPriority(63-68)StringToPriority(63-63)RunAll(81-138)RunAll(81-81)
src/bench/bench.cpp (1)
src/bench/bench.h (2)
BenchRunner(72-72)args(74-74)
🪛 GitHub Actions: Clang Diff Format Check
src/bench/bls.cpp
[error] 316-324: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix formatting issues.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: linux64_multiprocess-test / Test source
- GitHub Check: linux64_tsan-test / Test source
🔇 Additional comments (58)
src/Makefile.qttest.include (1)
62-62: LGTM - Consistent with build system cleanupThe removal of
$(LIBLEVELDB_SSE42)from the Qt test linker flags is consistent with the broader build system modernization described in the PR objectives, transitioning from static archives to libtool convenience libraries.src/Makefile.qt.include (1)
411-411: LGTM - Consistent build system cleanupThe removal of
$(LIBLEVELDB_SSE42)from the Qt GUI linker flags aligns with the build system modernization efforts described in the PR objectives, removing assembly-related configuration options.src/bench/ecdsa.cpp (6)
11-11: LGTM - Proper ECC context initializationThe addition of
ECC_Start()ensures the ECC context is properly initialized before the benchmark runs.
30-30: LGTM - Proper ECC context cleanupThe addition of
ECC_Stop()ensures proper cleanup of the ECC context after the benchmark completes.
35-35: LGTM - Consistent ECC context initializationProper ECC context initialization for the ECDSAVerify benchmark.
57-57: LGTM - Consistent ECC context cleanupProper ECC context cleanup for the ECDSAVerify benchmark.
62-62: LGTM - Consistent ECC context initializationProper ECC context initialization for the ECDSAVerify_LargeBlock benchmark.
84-84: LGTM - Consistent ECC context cleanupProper ECC context cleanup for the ECDSAVerify_LargeBlock benchmark. This ensures all three benchmark functions follow the same resource management pattern.
src/Makefile.bench.include (1)
64-64: LGTM - Improved linker dependency orderingMoving
$(LIBTEST_UTIL)to the beginning of the linker flags list and removing$(LIBLEVELDB_SSE42)aligns with the build system cleanup and fixes linker dependency ordering issues described in the PR objectives.src/crypto/sha256_x86_shani.cpp (1)
9-9: LGTM - Proper dependency guard for SHA-NI implementationThe change from
#ifdef ENABLE_X86_SHANIto#if defined(ENABLE_SSE41) && defined(ENABLE_X86_SHANI)correctly ensures that the SHA-NI implementation is only compiled when both SSE4.1 and SHA-NI instruction sets are available. This aligns with the centralized SHA256 implementation selection described in the AI summary.src/bench/checkblock.cpp (2)
10-10: LGTM! Appropriate include for the stats client functionality.The include is necessary for the
InitStatsClientfunction used below.
40-42: LGTM! Proper initialization of the stats client for CheckBlock.This manual initialization is necessary because
CheckBlockcallsg_stats_clientinternally, but the benchmark environment doesn't automatically provide it. The comment clearly explains the reasoning.doc/fuzzing.md (1)
137-137: LGTM! Removal of discontinued configure option.This change aligns with the PR objective of removing all assembly-related configuration options from the build system. The
--disable-asmflag is no longer available.src/bench/bls.cpp (3)
10-10: LGTM! Appropriate include for atomic operations.Required for the
std::atomic<bool>type used in the cancellation mechanism.
318-321: LGTM! Proper thread-safe cancellation mechanism.The change from
volatile booltostd::atomic<bool>correctly fixes the data race detected by the sanity checks. The explicitload()call ensures thread-safe access to the cancellation flag.
352-352: LGTM! Thread-safe cancellation assignment.Using
store(true)ensures atomic write operation for the cancellation flag.src/Makefile.test.include (3)
43-44: LGTM! Proper library ordering for linking.Moving
LIBTEST_FUZZandLIBTEST_UTILto the beginning of the linking order helps resolve linker dependency issues mentioned in the PR objectives.
243-243: LGTM! Removal of SSE42-specific LevelDB library.This change aligns with the broader build system updates that remove SSE42-specific library variants and move to libtool convenience libraries.
401-402: LGTM! Enabling benchmark sanity checks.This change implements the PR objective of reintroducing sanity checks into the
make checkprocess. The-sanity-checkand-priority-level=highflags limit overhead by running only high-priority benchmarks with single iterations.src/crypto/sha256.h (2)
29-39: LGTM! Well-designed SHA256 implementation selection API.The namespace and enum provide a clean interface for selecting specific SHA256 CPU optimizations. The use of bit flags allows flexible combinations of implementations, and the predefined combinations (USE_SSE4_AND_AVX2, USE_ALL) provide convenient defaults.
44-44: LGTM! Backward-compatible function signature enhancement.The optional parameter with default value
USE_ALLmaintains backward compatibility while allowing callers to specify which implementations to consider during autodetection. This centralizes SHA256 implementation selection as intended by the PR objectives.src/rpc/node.cpp (1)
1153-1153: LGTM!The rename from
RegisterMiscRPCCommandstoRegisterNodeRPCCommandsbetter reflects that this file now contains node-specific RPC commands after moving address/descriptor-related commands tooutput_script.cpp.configure.ac (1)
605-608: LGTM!The SSE4.1 intrinsics test correctly uses
_mm_blend_epi16, which is a proper SSE4.1-specific intrinsic. The test pattern is valid and will correctly detect SSE4.1 support.src/Makefile.leveldb.include (1)
41-46: Transition to libtool convenience libraries looks good.The explicit
-staticflags in bothCXXFLAGSandLDFLAGScorrectly ensure static linking, which is necessary due to the lack of DLL exports on Windows. The comment clearly explains the rationale.src/rpc/register.h (2)
20-22: LGTM: Clean RPC command modularizationThe function declarations properly reflect the reorganization of RPC commands from the previous
RegisterMiscRPCCommandsinto more specific registration functions, improving code organization and maintainability.
46-48: LGTM: Proper registration sequenceThe new registration calls maintain the correct order and properly replace the removed
RegisterMiscRPCCommandscall, ensuring all RPC commands are registered appropriately.src/bench/bench_bitcoin.cpp (4)
21-22: LGTM: Appropriate default priority configurationThe
DEFAULT_PRIORITYconstant with value "all" provides sensible default behavior for running benchmarks at all priority levels.
31-36: LGTM: Improved argument naming and new featuresThe migration from underscore-separated arguments (e.g.,
-min_time) to hyphen-separated (e.g.,-min-time) follows standard CLI conventions. The new-sanity-checkand-priority-leveloptions properly integrate with the enhanced benchmark framework.
52-58: LGTM: Well-implemented priority parsingThe
parsePriorityLevelfunction correctly parses comma-separated priority level strings and converts them to a bitmask using thebenchmark::StringToPriorityfunction. The implementation handles multiple priority levels appropriately.
121-138: LGTM: Robust exception handling and argument parsingThe try-catch block properly handles exceptions during argument parsing and provides informative error messages. The argument parsing correctly maps CLI options to the
benchmark::Argsstructure using the new helper functions.src/bench/bench.h (6)
44-48: LGTM: Well-designed priority level systemThe
PriorityLevelenum uses bit flags (LOW = 1 << 0, HIGH = 1 << 2) which allows for efficient bitwise operations and future extensibility. The gap between LOW and HIGH values leaves room for additional priority levels.
50-52: LGTM: Proper utility function declarationsThe
ListPriorities()andStringToPriority()functions provide necessary utilities for converting between string representations and priority values, supporting the CLI integration.
56-56: LGTM: Appropriate Args struct extensionsThe addition of
sanity_checkboolean andpriorityuint8_t fields properly supports the new benchmark features while maintaining backward compatibility with existing code.Also applies to: 62-62
67-68: LGTM: Enhanced benchmark registrationThe updated
BenchmarkMaptypedef correctly maps benchmark names to pairs of functions and priority levels, enabling priority-based filtering during benchmark execution.
72-72: LGTM: Updated constructor signatureThe
BenchRunnerconstructor now properly accepts aPriorityLevelparameter, enabling priority assignment during benchmark registration.
79-80: LGTM: Sensible default priorityThe updated
BENCHMARKmacro assignsHIGHpriority by default, ensuring that existing benchmarks maintain high priority execution unless explicitly specified otherwise.src/crypto/sha256.cpp (5)
11-11: LGTM: Clean compile-time guard implementationThe
DISABLE_OPTIMIZED_SHA256macro provides a clean way to conditionally exclude all optimized SHA256 implementations at compile time, improving code organization and reducing binary size when optimizations are not needed.Also applies to: 60-60, 570-570, 580-580, 593-593, 651-651, 685-685
584-592: LGTM: Enhanced function signature with proper initializationThe updated
SHA256AutoDetectfunction signature accepts ause_implementationparameter with a sensible default, enabling runtime control over optimization selection. The explicit initialization of function pointers to standard implementations ensures proper fallback behavior.
604-620: LGTM: Proper runtime optimization controlThe CPU feature detection logic correctly checks the
use_implementationparameter before enabling specific optimizations, allowing selective disabling of CPU features even when hardware support is available.
622-650: LGTM: Systematic conditional compilationThe optimized implementations are properly guarded by their respective feature macros (
ENABLE_SSE41,ENABLE_AVX2,ENABLE_X86_SHANI), ensuring compilation only occurs when the build system enables these features.
655-684: LGTM: Platform-specific ARM SHANI detectionThe ARM SHANI detection properly handles different platforms (Linux, macOS) and architectures (32-bit, 64-bit) with appropriate runtime checks, maintaining compatibility across various ARM environments.
src/rpc/output_script.cpp (5)
31-78: LGTM: Well-implemented address validationThe
validateaddressRPC command properly validates Dash addresses usingDecodeDestination, handles both valid and invalid cases appropriately, and returns comprehensive information including scriptPubKey and address details. The error handling correctly usesCHECK_NONFATALfor consistency validation.
80-137: LGTM: Robust multisig creationThe
createmultisigRPC command properly validates hex-encoded public keys with correct length checks (66 or 130 characters), creates multisig destinations usingAddAndGetMultisigDestination, and returns comprehensive results including address, redeem script, and descriptor. The error handling provides clear feedback for invalid keys.
139-185: LGTM: Comprehensive descriptor analysisThe
getdescriptorinfoRPC command properly parses descriptors using theParsefunction, validates input withRPCTypeCheck, and returns detailed information about the descriptor including canonical form, checksum, and various boolean flags. The error handling correctly propagates parsing errors.
187-270: LGTM: Thorough address derivationThe
deriveaddressesRPC command implements proper descriptor parsing with checksum validation, range parameter validation for ranged vs non-ranged descriptors, and comprehensive address derivation with proper error handling. The logic correctly handles both single and ranged descriptor derivation.
272-283: LGTM: Proper RPC command registrationThe
RegisterOutputScriptRPCCommandsfunction correctly registers all four RPC commands under the "util" category, following the standard RPC registration pattern used throughout the codebase.src/bench/bench.cpp (3)
48-68: LGTM! Clean implementation of priority management.The priority mapping system is well-designed with clear string-to-bitmask associations. The
ListPriorities()function correctly sorts by priority value for consistent output.
70-79: Correct implementation of priority storage.The updated constructor properly stores both the benchmark function and its priority level, matching the updated map structure.
81-141: Well-implemented priority filtering and sanity check mode.The changes correctly implement:
- Priority-based filtering using bitwise operations
- Sanity check mode that runs a single iteration without output
- Proper destructuring of the benchmark map to access both function and priority
The implementation maintains backward compatibility while adding the new features cleanly.
src/Makefile.crc32c.include (2)
5-8: Correct transition to libtool convenience libraries.The change from
EXTRA_LIBRARIEStonoinst_LTLIBRARIESand.ato.laproperly converts the crc32c library to a libtool convenience library. This is build configuration, not modification of the vendored crc32c source code.
38-44: Proper enforcement of static linking.The addition of
-staticto both CXXFLAGS and LDFLAGS correctly ensures only static versions are built. The comment clearly explains the rationale.src/Makefile.am (4)
21-28: Good practice: Initialize build variables explicitly.Adding these empty variable declarations improves clarity and prevents potential issues with undefined variables in the build system.
75-92: Correct crypto library organization.The changes properly:
- Transition to libtool convenience libraries
- Nest X86_SHANI inside the SSE41 conditional (architecturally correct as SHANI requires SSE4.1)
- Aggregate all crypto libraries into
LIBBITCOIN_CRYPTO
548-550: Good refactoring: Split RPC commands into specific files.Replacing
rpc/misc.cppwithrpc/node.cppandrpc/output_script.cppimproves code organization by grouping related RPC commands together.
999-1003: Appropriate consensus library configuration.Using
crypto_libbitcoin_crypto_base_la_SOURCESand adding-DDISABLE_OPTIMIZED_SHA256ensures the consensus library uses only standard implementations, which is crucial for deterministic consensus behavior.src/bench/crypto_hash.cpp (3)
42-51: Clever implementation testing pattern for SHA256 variants.The pattern of calling
SHA256AutoDetect()twice (before to set implementation and after to restore auto-detection) ensures each benchmark tests the specific CPU implementation while maintaining system state. The dynamic naming clearly indicates which implementation is being tested.Also applies to: 53-62, 64-73, 75-84
287-433: Excellent organization of Dash-specific benchmarks.The clear separation of Dash-specific tests improves code organization. All benchmarks consistently use the standardized batch setup with proper byte unit specification.
86-93: SHA3_256::Write(in) is valid via SpanThe
Writemethod is defined asSHA3_256& Write(Span<const unsigned char> data);and our Span implementation has an implicit constructor fromstd::vector<uint8_t>. Passingindirectly to.Write(in)is intentional and correct—no change needed.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
configure.ac(1 hunks)doc/developer-notes.md(0 hunks)doc/fuzzing.md(1 hunks)src/Makefile.am(8 hunks)src/Makefile.bench.include(1 hunks)src/Makefile.crc32c.include(2 hunks)src/Makefile.leveldb.include(2 hunks)src/Makefile.qt.include(1 hunks)src/Makefile.qttest.include(1 hunks)src/Makefile.test.include(3 hunks)src/Makefile.test_fuzz.include(0 hunks)src/Makefile.test_util.include(0 hunks)src/bench/bench.cpp(3 hunks)src/bench/bench.h(1 hunks)src/bench/bench_bitcoin.cpp(5 hunks)src/bench/bls.cpp(3 hunks)src/bench/checkblock.cpp(2 hunks)src/bench/crypto_hash.cpp(2 hunks)src/bench/ecdsa.cpp(4 hunks)src/crypto/sha256.cpp(7 hunks)src/crypto/sha256.h(1 hunks)src/crypto/sha256_x86_shani.cpp(1 hunks)src/wallet/load.cpp(1 hunks)src/wallet/wallet.cpp(2 hunks)
💤 Files with no reviewable changes (3)
- src/Makefile.test_util.include
- src/Makefile.test_fuzz.include
- doc/developer-notes.md
✅ Files skipped from review due to trivial changes (4)
- doc/fuzzing.md
- src/bench/checkblock.cpp
- src/Makefile.bench.include
- src/Makefile.qttest.include
🚧 Files skipped from review as they are similar to previous changes (14)
- src/Makefile.qt.include
- src/bench/ecdsa.cpp
- src/crypto/sha256_x86_shani.cpp
- src/bench/bls.cpp
- src/Makefile.test.include
- src/wallet/wallet.cpp
- src/crypto/sha256.h
- src/crypto/sha256.cpp
- src/bench/bench.h
- configure.ac
- src/Makefile.leveldb.include
- src/bench/bench.cpp
- src/bench/bench_bitcoin.cpp
- src/Makefile.crc32c.include
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
src/bench/**/*.{cpp,h,cc,cxx,hpp}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 Learnings (4)
📓 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: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Favor extension over modification of the Bitcoin Core foundation
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue}/** : Do not make changes under any circumstances to vendored dependencies in src/crc32c, src/dashbls, src/gsl, src/immer, src/leveldb, src/minisketch, src/secp256k1, src/univalue
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests should be implemented in src/test/, src/wallet/test/, and src/qt/test/ using Boost::Test or Qt 5 for GUI tests
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be implemented in src/bench/ using nanobench
src/wallet/load.cpp (1)
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
src/Makefile.am (4)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue}/** : Do not make changes under any circumstances to vendored dependencies in src/crc32c, src/dashbls, src/gsl, src/immer, src/leveldb, src/minisketch, src/secp256k1, src/univalue
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
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
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests should be implemented in src/test/, src/wallet/test/, and src/qt/test/ using Boost::Test or Qt 5 for GUI tests
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/**/*.{cpp,h,cc,cxx,hpp} : Dash uses unordered_lru_cache for efficient caching with LRU eviction
src/bench/crypto_hash.cpp (3)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be implemented in src/bench/ using nanobench
Learnt from: knst
PR: dashpay/dash#6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions `MigrateDBIfNeeded` and `MigrateDBIfNeeded2` in `src/evo/deterministicmns.cpp` are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Learnt from: kwvg
PR: dashpay/dash#6532
File: src/test/fuzz/netaddress.cpp:83-84
Timestamp: 2025-01-14T09:06:19.717Z
Learning: In fuzzer harness tests, CServiceHash can be used with both default constructor (CServiceHash()) and parameterized constructor (CServiceHash(salt_k0, salt_k1)) to test different variants. The usage pattern CServiceHash()(service) and CServiceHash(0, 0)(service) is valid and intentional in such tests.
🧬 Code Graph Analysis (1)
src/bench/crypto_hash.cpp (6)
src/crypto/sha256.h (4)
hash(25-25)SHA256AutoDetect(44-44)CSHA256(23-23)SHA256D64(51-51)src/crypto/ripemd160.h (1)
CRIPEMD160(22-22)src/crypto/sha1.h (1)
CSHA1(22-22)src/crypto/sha256.cpp (5)
SHA256AutoDetect(584-689)SHA256AutoDetect(584-584)CSHA256(693-696)SHA256D64(748-780)SHA256D64(748-748)src/crypto/sha3.h (1)
SHA3_256(35-35)src/crypto/sha512.h (1)
CSHA512(22-22)
🔇 Additional comments (16)
src/bench/crypto_hash.cpp (5)
18-18: Include addition looks correct.The
tinyformat.hinclude is appropriately added to support thestrprintffunction used in benchmark naming.
42-51: SHA256 implementation selection pattern is well-implemented.The pattern of calling
SHA256AutoDetect()with specific implementation parameters, performing the benchmark, then restoring defaults is correct and consistent.
28-28: Consistent benchmarking pattern adopted.All benchmark functions now use the standardized
.batch(in.size()).unit("byte").run([&] { ... });pattern, which is excellent for consistent measurement and reporting.Also applies to: 37-37, 47-47, 58-58, 69-69, 80-80, 90-90, 99-99, 111-111, 123-123, 135-135, 147-147, 157-157, 167-167, 177-177, 187-187
289-433: Well-organized Dash-specific benchmarks section.The Dash-specific benchmarks are properly separated and follow consistent naming conventions. The variety of input sizes (32b, 80b, 128b, 512b, 1024b, 2048b) provides good coverage for performance testing of both DSHA256 and X11 hash functions.
261-285: Benchmark registration macros updated correctly.All benchmark registration macros have been updated to match the new function names, maintaining consistency between function definitions and registrations.
src/Makefile.am (11)
21-29: LGTM: Standardized build variable declarations.The addition of empty variable declarations for
lib_LTLIBRARIES,noinst_LTLIBRARIES,bin_PROGRAMS,noinst_PROGRAMS,check_PROGRAMS,TESTS, andBENCHMARKSfollows good Makefile practices by establishing these variables early in the file.
59-59: LGTM: Crypto library migration to libtool convenience library.The change from
LIBBITCOIN_CRYPTO_BASE=crypto/libbitcoin_crypto_base.ato.lais part of the systematic migration to libtool convenience libraries, which is consistent with the build system modernization objectives.
75-92: LGTM: Consistent crypto library variable updates.The systematic conversion of all crypto library variables from
.ato.laformat is well-executed:
LIBBITCOIN_CRYPTO_SSE41,LIBBITCOIN_CRYPTO_AVX2,LIBBITCOIN_CRYPTO_X86_SHANI,LIBBITCOIN_CRYPTO_ARM_SHANIall converted consistently- The conditional logic structure is preserved
- Moving from
EXTRA_LIBRARIEStonoinst_LTLIBRARIESis appropriate for convenience libraries
548-550: LGTM: RPC file reorganization aligns with refactoring objectives.The replacement of
rpc/misc.cppwithrpc/node.cppandrpc/output_script.cppreflects the RPC command reorganization mentioned in the PR objectives, where address validation and descriptor handling commands are being moved to dedicated files.
644-656: LGTM: Well-documented crypto base library configuration.The conversion to libtool convenience library format with appropriate flags:
- Clear documentation explaining the
-staticflag rationale- Proper PIC flags for shared library compatibility
- Consistent naming convention with
_la_suffixes
691-708: LGTM: Consistent SSE4.1 and AVX2 crypto library configuration.Both libraries follow the same pattern:
- Proper
-staticflags with clear documentation- Appropriate feature flags (
-DENABLE_SSE41,-DENABLE_AVX2)- Consistent build flag structure
710-735: LGTM: X11 crypto sources properly integrated.The X11 algorithm sources are correctly added to the base crypto library, maintaining the existing structure while adapting to the new
.laformat.
736-753: LGTM: SHANI crypto libraries properly configured.Both x86 and ARM SHANI libraries follow the established pattern:
- Consistent
-staticflags and documentation- Proper architecture-specific flags
- Correct feature detection macros (
-DENABLE_X86_SHANI,-DENABLE_ARM_SHANI)
999-999: LGTM: Consensus library updated to use new crypto base sources.The change to use
$(crypto_libbitcoin_crypto_base_la_SOURCES)maintains consistency with the migration to libtool convenience libraries.
1003-1003: LGTM: Appropriate optimization flag for consensus library.The addition of
-DDISABLE_OPTIMIZED_SHA256flag is consistent with the build configuration changes mentioned in the PR objectives, where assembly-related options were removed from configure.ac.
683-683: sha256_sse4.cpp correctly belongs in the base crypto libraryI’ve verified that:
crypto_libbitcoin_crypto_base_la_SOURCESunconditionally includescrypto/sha256_sse4.cpp.- The SSE4.1‐optimized code lives in its own library (
crypto_libbitcoin_crypto_sse41_la_SOURCES = crypto/sha256_sse41.cpp), with-DENABLE_SSE41and$(SSE41_CXXFLAGS).- There are no conflicting symbols or missing guards—each static library is distinct and only pulled in when explicitly requested.
No changes required.
… misc.cpp Review with `git log -p -n1 --color-moved=dimmed_zebra`.
* ECDSA benchmarks require ECC initialization, which was missing * CheckBlock() internally calls `g_stats_client` and without a testing setup, it'll be a nullptr. * Wallet benchmarks assume that TestingSetup will setup the CoinJoin loader but that is not guaranteed. We should check that the loader is available before attempting registration with it.
… comments excludes: - bf209ac
Also drops code comments not present upstream due to change in sorting Review with `git log -p -n1 --color-moved=dimmed_zebra`.
Also upstream uses `unit("byte")` for `HASH_SHA256_32b()`, so let's
realign that as well.
We'll also do the same for Dash-specific tests to adopt the same style but this means our upstream portions now mirror 1:1.
Linker reads binaries from top to bottom, they need to be placed earlier to avoid linker problems.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/bench/crypto_hash.cpp (1)
143-151: Critical: SHA256D64 in-place operation safety issue persists.This is identical to the previous review comment. The SHA256D64 function is called with the same buffer for input and output (
SHA256D64(in.data(), in.data(), 1024)), which is unsafe as the function header insrc/crypto/sha256.hdoes not guarantee support for overlapping buffers.Apply this fix to create separate output buffers:
static void SHA256D64_1024_STANDARD(benchmark::Bench& bench) { bench.name(strprintf("%s using the '%s' SHA256 implementation", __func__, SHA256AutoDetect(sha256_implementation::STANDARD))); std::vector<uint8_t> in(64 * 1024, 0); + std::vector<uint8_t> out(32 * 1024, 0); bench.batch(in.size()).unit("byte").run([&] { - SHA256D64(in.data(), in.data(), 1024); + SHA256D64(out.data(), in.data(), 1024); }); SHA256AutoDetect(); }Apply the same fix to
SHA256D64_1024_SSE4,SHA256D64_1024_AVX2, andSHA256D64_1024_SHANIfunctions.Also applies to: 153-161, 163-171, 173-181
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
configure.ac(1 hunks)doc/developer-notes.md(0 hunks)doc/fuzzing.md(1 hunks)src/Makefile.am(8 hunks)src/Makefile.bench.include(1 hunks)src/Makefile.crc32c.include(2 hunks)src/Makefile.leveldb.include(2 hunks)src/Makefile.qt.include(1 hunks)src/Makefile.qttest.include(1 hunks)src/Makefile.test.include(3 hunks)src/Makefile.test_fuzz.include(0 hunks)src/Makefile.test_util.include(0 hunks)src/bench/bench.cpp(3 hunks)src/bench/bench.h(1 hunks)src/bench/bench_bitcoin.cpp(5 hunks)src/bench/bls.cpp(3 hunks)src/bench/checkblock.cpp(2 hunks)src/bench/crypto_hash.cpp(2 hunks)src/bench/ecdsa.cpp(4 hunks)src/crypto/sha256.cpp(7 hunks)src/crypto/sha256.h(1 hunks)src/crypto/sha256_x86_shani.cpp(1 hunks)src/rpc/node.cpp(1 hunks)src/rpc/output_script.cpp(1 hunks)src/rpc/register.h(2 hunks)src/wallet/load.cpp(1 hunks)src/wallet/wallet.cpp(2 hunks)
💤 Files with no reviewable changes (3)
- doc/developer-notes.md
- src/Makefile.test_util.include
- src/Makefile.test_fuzz.include
✅ Files skipped from review due to trivial changes (3)
- src/bench/checkblock.cpp
- src/Makefile.qttest.include
- doc/fuzzing.md
🚧 Files skipped from review as they are similar to previous changes (19)
- src/bench/ecdsa.cpp
- src/wallet/load.cpp
- src/Makefile.qt.include
- src/crypto/sha256_x86_shani.cpp
- src/bench/bls.cpp
- src/Makefile.test.include
- src/Makefile.bench.include
- src/crypto/sha256.h
- src/crypto/sha256.cpp
- src/rpc/node.cpp
- configure.ac
- src/bench/bench_bitcoin.cpp
- src/bench/bench.h
- src/Makefile.leveldb.include
- src/rpc/register.h
- src/bench/bench.cpp
- src/wallet/wallet.cpp
- src/Makefile.am
- src/Makefile.crc32c.include
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
src/bench/**/*.{cpp,h,cc,cxx,hpp}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 Learnings (3)
📓 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: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Favor extension over modification of the Bitcoin Core foundation
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue}/** : Do not make changes under any circumstances to vendored dependencies in src/crc32c, src/dashbls, src/gsl, src/immer, src/leveldb, src/minisketch, src/secp256k1, src/univalue
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be implemented in src/bench/ using nanobench
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests should be implemented in src/test/, src/wallet/test/, and src/qt/test/ using Boost::Test or Qt 5 for GUI tests
src/rpc/output_script.cpp (1)
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
src/bench/crypto_hash.cpp (3)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be implemented in src/bench/ using nanobench
Learnt from: knst
PR: dashpay/dash#6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions `MigrateDBIfNeeded` and `MigrateDBIfNeeded2` in `src/evo/deterministicmns.cpp` are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Learnt from: kwvg
PR: dashpay/dash#6532
File: src/test/fuzz/netaddress.cpp:83-84
Timestamp: 2025-01-14T09:06:19.717Z
Learning: In fuzzer harness tests, CServiceHash can be used with both default constructor (CServiceHash()) and parameterized constructor (CServiceHash(salt_k0, salt_k1)) to test different variants. The usage pattern CServiceHash()(service) and CServiceHash(0, 0)(service) is valid and intentional in such tests.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: Lint / Run linters
🔇 Additional comments (11)
src/bench/crypto_hash.cpp (5)
18-18: LGTM: Required header for string formatting.The
tinyformat.hinclude is needed for thestrprintffunction used in dynamic benchmark naming.
24-31: LGTM: Consistent benchmark pattern adopted.The benchmark now uses the standardized
.batch(in.size()).unit("byte").run([&] {...})pattern, which provides better performance metrics.
42-51: LGTM: Excellent dynamic naming for SHA256 variants.The implementation correctly uses
strprintfto create descriptive benchmark names that include the specific SHA256 implementation being tested, and properly resets the implementation afterward.
287-432: LGTM: Well-organized Dash-specific benchmarks.The Dash-specific benchmarks are properly separated with clear comments and follow the same consistent pattern as the upstream benchmarks. The comprehensive testing of different input sizes for both DSHA256 and X11 is valuable for performance analysis.
261-285: LGTM: Comprehensive benchmark registration.All the renamed and new benchmark functions are properly registered with the BENCHMARK macro, ensuring they'll be included in the benchmark suite.
src/rpc/output_script.cpp (6)
1-30: LGTM! Header includes and namespace usage are appropriate.The copyright headers, includes, and namespace declarations follow proper Dash Core conventions. The included headers cover all necessary functionality for the RPC commands implemented in this file.
31-78: Address validation logic is correct.The
validateaddressfunction properly validates Dash addresses and returns comprehensive information including validity, normalized address, scriptPubKey, and detailed address information. The error handling is appropriate with proper JSON-RPC error responses.
80-137: Multisig creation implementation is sound.The
createmultisigfunction correctly validates public keys (checking for hex format and proper length of 66 or 130 characters), creates P2SH multisig addresses, and returns the complete information including address, redeem script, and descriptor. The error handling for invalid public keys is appropriate.
139-185: Descriptor analysis implementation is correct.The
getdescriptorinfofunction properly parses and analyzes output descriptors, returning canonical form, checksum, and various boolean flags. The error handling for invalid descriptors is appropriate.
187-270: Address derivation logic is well-implemented.The
deriveaddressesfunction correctly handles both ranged and non-ranged descriptors, validates range parameters, and derives addresses appropriately. The error handling covers various edge cases including missing private keys and descriptors without corresponding addresses.
272-283: RPC command registration follows standard patterns.The
RegisterOutputScriptRPCCommandsfunction properly registers all four RPC commands under the "util" category, following the standard Dash Core RPC registration patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/bench/bls_dkg.cpp (1)
116-119: Performance optimization for sanity checks looks good.The conditional logic correctly reduces workload during dry-run/sanity-check modes by setting both
epoch_itersandquorumSizeto 1. This aligns with the PR objective of reintroducing sanity checks with minimal overhead.However, there's a clang-format issue that needs to be addressed.
#!/bin/bash # Format the specific lines that have clang-format issues clang-format -i src/bench/bls_dkg.cpp
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/bench/bls.cpp(9 hunks)src/bench/bls_dkg.cpp(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bench/bls.cpp
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
src/bench/**/*.{cpp,h,cc,cxx,hpp}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 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: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Favor extension over modification of the Bitcoin Core foundation
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue}/** : Do not make changes under any circumstances to vendored dependencies in src/crc32c, src/dashbls, src/gsl, src/immer, src/leveldb, src/minisketch, src/secp256k1, src/univalue
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be implemented in src/bench/ using nanobench
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests should be implemented in src/test/, src/wallet/test/, and src/qt/test/ using Boost::Test or Qt 5 for GUI tests
src/bench/bls_dkg.cpp (2)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be implemented in src/bench/ using nanobench
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.
🪛 GitHub Actions: Clang Diff Format Check
src/bench/bls_dkg.cpp
[error] 156-189: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (3)
src/bench/bls_dkg.cpp (3)
143-146: Consistent performance optimization applied.The same conditional logic pattern is correctly applied to the
BLSDKG_InitDKGfunction, maintaining consistency across the codebase.
162-167: Macro optimization follows the same pattern effectively.The conditional logic in the
BENCH_BuildQuorumVerificationVectorsmacro correctly creates a minimal DKG instance with size 1 and returns early when output is disabled. This provides significant performance benefits during sanity checks.
177-185: Final macro optimization completes the consistent pattern.The
BENCH_VerifyContributionSharesmacro correctly implements the same optimization pattern, ensuring all BLS DKG benchmarks benefit from reduced workload during sanity checks.
…on for Echo512 and Shavite512, improve benchmark stability 005967b crypto: drop naive AES backends (Kittywhiskers Van Gogh) d131fcc crypto: implement Shavite512's full `Compress()` routine (Kittywhiskers Van Gogh) 31f93ad crypto: unroll Echo512's `FullStateRound()` (Kittywhiskers Van Gogh) fa68c70 crypto: implement ARM NEON backend for Echo512's `ShiftAndMix()` (Kittywhiskers Van Gogh) 963215b crypto: implement ARM AES backend for Shavite512's `CompressElement()` (Kittywhiskers Van Gogh) 959c9ee crypto: implement ARM AES backend for Echo512's `FullStateRound()` (Kittywhiskers Van Gogh) 76bd236 crypto: implement naive ARM AES backend for simple rounds (Kittywhiskers Van Gogh) f2ececc crypto: implement AES-NI backend for Shavite512's `CompressElement()` (Kittywhiskers Van Gogh) e1bbec4 crypto: avoid extra load/store in Echo512's `ShiftAndMix()` (Kittywhiskers Van Gogh) 250dcce crypto: combine Echo512's Shift and Mix operations (Kittywhiskers Van Gogh) 71d6ef9 crypto: implement SSSE3 backend for Echo512's `ShiftRows()` (Kittywhiskers Van Gogh) da38871 crypto: implement SSSE3 backend for Echo512's `MixColumns()` (Kittywhiskers Van Gogh) 31a6732 crypto: implement AES-NI backend for Echo512's `FullStateRound()` (Kittywhiskers Van Gogh) 1f2eb21 crypto: implement naive AES-NI backend for simple rounds (Kittywhiskers Van Gogh) cba39c5 const: use function pointer to allow for switching implementation (Kittywhiskers Van Gogh) d6d3518 crypto: replace hardcoded AES transform tables with constexpr tables (Kittywhiskers Van Gogh) 20fc998 refactor: move software AES round to header (Kittywhiskers Van Gogh) 386742b refactor: remove large footprint Shavite512 impl, switch to C++ (Kittywhiskers Van Gogh) 7e8607e refactor: remove large footprint Echo512 impl, switch to C++ (Kittywhiskers Van Gogh) c4e7a40 fix: suppress unaligned memory UBSan warnings if supported by arch (Kittywhiskers Van Gogh) 830928c bench: set minimum epoch iterations to improve `pow_hash` stability (Kittywhiskers Van Gogh) Pull request description: ## Additional Information ### Important * Due to the build system changes done in this PR, switching between commits in this branch or between this branch and any other branch (like `develop`) will likely require `git -dfx src/bench src/crypto src/primitives` and re-running `./autogen.sh` and `./configure`. * As the dispatcher doesn't kick in until _after_ genesis block validation, the correctness of the implementation can be validated by applying a commit that would retrigger the validation in the benchmark ([commit](23d15b2)). This is not included in this PR as it negatively impacts benchmark results in some builds ([comment](#6796 (comment))). * While working on the ARMv7/ARMv8 backends, it was noticed that the `pow_hash` benchmark's error rate on Apple Silicon can go as high as ~15%, even on repeated runs, which reduces the reliability of benchmark results and hinders decision-making. To mitigate this, `minEpochIterations` has been reintroduced and set to 20 (up from pre-[dash#6752](#6752 10). Baseline measurements were taken against this change _before_ large footprint removal. ### Misc. * `libdashconsensus` is built with hardware optimizations disabled, which is currently set with `DISABLE_OPTIMIZED_SHA256` (introduced in [bitcoin#29180](bitcoin#29180)). To align with upstream behavior, our platform-specific code is disabled in the library as well using the macro to mean "disable optimizations". * Despite not being specified in Apple's documentation ([source](https://developer.apple.com/documentation/kernel/1387446-sysctlbyname/determining_instruction_set_characteristics)), some versions of macOS report NEON (a.k.a. advanced SIMD) using `hw.optional.arm.AdvSIMD` instead of `hw.optional.AdvSIMD`, so we check for that `sysctl` as well (see [google/cpufeatures#390](google/cpu_features#390)) * Unaligned memory accesses are hardware-supported by platforms like x86_64 and sphlib utilizes them to improve performance ([source](https://github.com/dashpay/dash/blob/893b46a000c5088ce92f8625e74d7e3c126e0cdb/src/crypto/x11/sph_types.h#L117-L132)). When switching to the small footprint implementation of Shavite512, this triggers a UBSan error ([build](https://github.com/dashpay/dash/actions/runs/17896732544/job/50884688479#step:10:292)). As this is intentional behavior, we have opted to suppress the error by conditionally suppressing alignment sanitization _if_ the target platform supports it and it has been enabled (i.e. `SPH_UPTR` is set, which it is by default). ## Benchmarks ### Apple M1, macOS Sequoia 15.7 * **Build Information:** Apple Clang 17 (clang-1700.3.19.1), Xcode 26.0 (17A324) * **Depends Config:** `MULTIPROCESS=1 CC=clang CXX=clang++ HOST=aarch64-apple-darwin` * **Build Config:** `CC=clang CXX=clang++ CFLAGS="-O2 -g" LDFLAGS="-Wl,-O2" ./configure --prefix=$(pwd)/depends/aarch64-apple-darwin --enable-reduce-exports --without-gui --disable-fuzz-binary --disable-maintainer-mode --disable-dependency-tracking --disable-ccache` **Baseline** * <details> <summary>Echo512:</summary> ``` | ns/byte | byte/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 39.68 | 25,201,342.83 | 0.2% | 0.01 | `Pow_Echo512_0032b` | 16.09 | 62,143,122.31 | 1.2% | 0.01 | `Pow_Echo512_0080b` | 19.63 | 50,943,039.85 | 0.2% | 0.01 | `Pow_Echo512_0128b` | 12.24 | 81,674,975.07 | 0.9% | 0.01 | `Pow_Echo512_0512b` | 10.94 | 91,442,178.90 | 0.4% | 0.01 | `Pow_Echo512_1024b` | 10.30 | 97,063,958.41 | 0.3% | 0.01 | `Pow_Echo512_2048b` | 9.72 | 102,906,236.78 | 0.4% | 0.11 | `Pow_Echo512_1M` ``` </details> * <details> <summary>Shavite512:</summary> ``` | ns/byte | byte/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 15.18 | 65,869,311.23 | 0.5% | 0.01 | `Pow_Shavite512_0032b` | 6.08 | 164,380,032.21 | 0.5% | 0.01 | `Pow_Shavite512_0080b` | 7.50 | 133,291,221.55 | 0.4% | 0.01 | `Pow_Shavite512_0128b` | 4.69 | 213,338,740.34 | 0.6% | 0.01 | `Pow_Shavite512_0512b` | 4.25 | 235,339,069.77 | 0.8% | 0.01 | `Pow_Shavite512_1024b` | 4.02 | 248,784,999.42 | 0.7% | 0.01 | `Pow_Shavite512_2048b` | 3.75 | 266,841,572.42 | 0.4% | 0.04 | `Pow_Shavite512_1M` ``` </details> * <details> <summary>X11:</summary> ``` | ns/byte | byte/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 249.10 | 4,014,450.39 | 0.2% | 0.01 | `Pow_X11_0032b` | 100.76 | 9,924,687.87 | 0.7% | 0.01 | `Pow_X11_0080b` | 63.72 | 15,694,749.39 | 0.4% | 0.01 | `Pow_X11_0128b` | 16.99 | 58,861,757.72 | 0.3% | 0.01 | `Pow_X11_0512b` | 9.27 | 107,817,565.81 | 0.2% | 0.01 | `Pow_X11_1024b` | 5.28 | 189,479,026.11 | 0.8% | 0.01 | `Pow_X11_2048b` | 1.37 | 731,462,009.69 | 0.7% | 0.01 | `Pow_X11_1M` ``` </details> **Optimized** * <details> <summary>Echo512:</summary> ``` | ns/byte | byte/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 14.34 | 69,741,448.00 | 0.8% | 0.01 | `Pow_Echo512_0032b` | 5.68 | 176,124,721.60 | 0.0% | 0.01 | `Pow_Echo512_0080b` | 7.32 | 136,556,992.52 | 1.5% | 0.01 | `Pow_Echo512_0128b` | 4.31 | 232,035,714.32 | 0.1% | 0.01 | `Pow_Echo512_0512b` | 3.97 | 251,961,418.41 | 0.8% | 0.01 | `Pow_Echo512_1024b` | 3.62 | 276,147,242.57 | 0.1% | 0.01 | `Pow_Echo512_2048b` | 3.43 | 291,830,314.57 | 0.4% | 0.82 | `Pow_Echo512_1M` ``` </details> * <details> <summary>Shavite512:</summary> ``` | ns/byte | byte/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 17.16 | 58,273,414.75 | 0.9% | 0.01 | `Pow_Shavite512_0032b` | 6.81 | 146,755,834.13 | 0.2% | 0.01 | `Pow_Shavite512_0080b` | 8.34 | 119,878,248.65 | 1.2% | 0.01 | `Pow_Shavite512_0128b` | 5.05 | 197,863,716.10 | 0.7% | 0.01 | `Pow_Shavite512_0512b` | 4.55 | 219,843,493.16 | 0.8% | 0.01 | `Pow_Shavite512_1024b` | 4.40 | 227,519,688.25 | 0.5% | 0.01 | `Pow_Shavite512_2048b` | 3.79 | 263,911,928.82 | 0.1% | 0.91 | `Pow_Shavite512_1M` ``` </details> * <details> <summary>X11:</summary> ``` | ns/byte | byte/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 224.98 | 4,444,913.98 | 1.1% | 0.01 | `Pow_X11_0032b` | 90.76 | 11,017,721.52 | 1.7% | 0.01 | `Pow_X11_0080b` | 57.66 | 17,343,058.47 | 0.6% | 0.01 | `Pow_X11_0128b` | 15.79 | 63,347,591.25 | 0.3% | 0.01 | `Pow_X11_0512b` | 8.39 | 119,204,486.51 | 0.7% | 0.01 | `Pow_X11_1024b` | 4.98 | 200,909,248.89 | 1.1% | 0.01 | `Pow_X11_2048b` | 1.33 | 751,189,042.44 | 0.2% | 0.32 | `Pow_X11_1M` ``` </details> ### AMD Ryzen 5 5600G, Ubuntu 24.04 (Noble) * **Build Information:** Ubuntu Clang 18.1.8 * **Depends Config:** `MULTIPROCESS=1 CC=clang-18 CXX=clang++-18 HOST=x86_64-pc-linux-gnu` * **Build Config:** `CC=clang-18 CXX=clang++-18 CFLAGS="-O2 -g" LDFLAGS="-Wl,--as-needed -Wl,-O2" ./configure --prefix=$(pwd)/depends/x86_64-pc-linux-gnu --enable-reduce-exports --without-gui --disable-fuzz-binary --disable-maintainer-mode --disable-dependency-tracking --disable-ccache` **Baseline** * <details> <summary>Echo512:</summary> ``` | ns/byte | byte/s | err% | ins/byte | cyc/byte | IPC | bra/byte | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 51.81 | 19,300,083.40 | 0.2% | 916.31 | 230.13 | 3.982 | 6.19 | 0.5% | 0.01 | `Pow_Echo512_0032b` | 20.72 | 48,266,658.57 | 0.1% | 366.56 | 92.01 | 3.984 | 2.49 | 0.5% | 0.01 | `Pow_Echo512_0080b` | 25.67 | 38,953,174.42 | 0.1% | 456.97 | 113.97 | 4.009 | 2.95 | 0.5% | 0.01 | `Pow_Echo512_0128b` | 16.00 | 62,491,477.70 | 0.1% | 285.25 | 71.06 | 4.014 | 1.81 | 0.6% | 0.01 | `Pow_Echo512_0512b` | 14.38 | 69,562,745.47 | 0.1% | 256.63 | 63.83 | 4.020 | 1.63 | 0.6% | 0.01 | `Pow_Echo512_1024b` | 13.65 | 73,272,852.80 | 0.1% | 242.32 | 60.27 | 4.020 | 1.53 | 0.6% | 0.01 | `Pow_Echo512_2048b` | 12.84 | 77,877,131.11 | 0.0% | 228.02 | 56.70 | 4.022 | 1.44 | 0.6% | 3.07 | `Pow_Echo512_1M` ``` </details> * <details> <summary>Shavite512:</summary> ``` | ns/byte | byte/s | err% | ins/byte | cyc/byte | IPC | bra/byte | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 26.75 | 37,379,255.99 | 0.0% | 423.31 | 118.82 | 3.563 | 1.41 | 0.0% | 0.01 | `Pow_Shavite512_0032b` | 10.71 | 93,360,115.43 | 0.0% | 169.35 | 47.57 | 3.560 | 0.58 | 0.0% | 0.01 | `Pow_Shavite512_0080b` | 13.26 | 75,436,781.19 | 0.0% | 210.04 | 58.87 | 3.568 | 0.45 | 0.0% | 0.01 | `Pow_Shavite512_0128b` | 8.26 | 121,051,101.55 | 0.0% | 130.99 | 36.69 | 3.570 | 0.27 | 0.0% | 0.01 | `Pow_Shavite512_0512b` | 7.44 | 134,444,187.57 | 0.0% | 117.82 | 33.04 | 3.566 | 0.24 | 0.4% | 0.01 | `Pow_Shavite512_1024b` | 7.06 | 141,686,987.23 | 0.2% | 111.23 | 31.18 | 3.568 | 0.23 | 0.2% | 0.01 | `Pow_Shavite512_2048b` | 6.65 | 150,409,844.26 | 0.1% | 104.65 | 29.37 | 3.563 | 0.21 | 0.0% | 1.60 | `Pow_Shavite512_1M` ``` </details> * <details> <summary>X11:</summary> ``` | ns/byte | byte/s | err% | ins/byte | cyc/byte | IPC | bra/byte | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 326.13 | 3,066,305.67 | 0.0% | 5,979.73 | 1,448.47 | 4.128 | 41.00 | 0.1% | 0.01 | `Pow_X11_0032b` | 130.47 | 7,664,483.17 | 0.0% | 2,391.79 | 579.50 | 4.127 | 16.38 | 0.1% | 0.01 | `Pow_X11_0080b` | 83.00 | 12,048,837.15 | 0.0% | 1,516.78 | 368.62 | 4.115 | 10.30 | 0.1% | 0.01 | `Pow_X11_0128b` | 21.78 | 45,920,017.94 | 0.0% | 395.29 | 96.72 | 4.087 | 2.64 | 0.1% | 0.01 | `Pow_X11_0512b` | 11.50 | 86,977,849.93 | 0.0% | 208.38 | 51.07 | 4.081 | 1.36 | 0.1% | 0.01 | `Pow_X11_1024b` | 6.38 | 156,826,545.04 | 0.0% | 114.92 | 28.16 | 4.081 | 0.72 | 0.1% | 0.01 | `Pow_X11_2048b` | 1.21 | 827,082,388.48 | 0.1% | 21.65 | 5.34 | 4.052 | 0.09 | 0.0% | 0.29 | `Pow_X11_1M` ``` </details> **Optimized** * <details> <summary>Echo512:</summary> ``` | ns/byte | byte/s | err% | ins/byte | cyc/byte | IPC | bra/byte | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 17.52 | 57,085,952.31 | 0.0% | 166.19 | 77.77 | 2.137 | 7.88 | 0.0% | 0.01 | `Pow_Echo512_0032b` | 7.01 | 142,732,714.40 | 0.1% | 66.51 | 31.10 | 2.139 | 3.16 | 0.0% | 0.01 | `Pow_Echo512_0080b` | 8.69 | 115,084,427.41 | 0.1% | 81.58 | 38.56 | 2.116 | 3.76 | 0.0% | 0.01 | `Pow_Echo512_0128b` | 5.36 | 186,499,101.72 | 0.1% | 50.51 | 23.79 | 2.123 | 2.31 | 0.1% | 0.01 | `Pow_Echo512_0512b` | 4.79 | 208,614,198.78 | 0.1% | 45.33 | 21.29 | 2.129 | 2.07 | 0.0% | 0.01 | `Pow_Echo512_1024b` | 4.51 | 221,584,256.28 | 0.0% | 42.74 | 20.04 | 2.133 | 1.95 | 0.0% | 0.01 | `Pow_Echo512_2048b` | 4.27 | 234,154,680.31 | 0.0% | 40.15 | 18.95 | 2.119 | 1.83 | 0.0% | 1.02 | `Pow_Echo512_1M` ``` </details> * <details> <summary>Shavite512:</summary> ``` | ns/byte | byte/s | err% | ins/byte | cyc/byte | IPC | bra/byte | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 10.12 | 98,853,454.39 | 0.5% | 69.47 | 44.93 | 1.546 | 8.38 | 0.0% | 0.01 | `Pow_Shavite512_0032b` | 3.98 | 251,044,756.51 | 0.1% | 27.81 | 17.69 | 1.572 | 3.36 | 0.0% | 0.01 | `Pow_Shavite512_0080b` | 4.89 | 204,696,173.21 | 0.1% | 32.78 | 21.69 | 1.511 | 3.90 | 0.0% | 0.01 | `Pow_Shavite512_0128b` | 3.01 | 332,315,588.24 | 0.1% | 20.08 | 13.37 | 1.502 | 2.42 | 0.1% | 0.01 | `Pow_Shavite512_0512b` | 2.70 | 369,999,483.82 | 0.2% | 17.96 | 12.00 | 1.496 | 2.17 | 0.0% | 0.01 | `Pow_Shavite512_1024b` | 2.55 | 392,427,831.38 | 0.0% | 16.90 | 11.32 | 1.493 | 2.05 | 0.0% | 0.01 | `Pow_Shavite512_2048b` | 2.39 | 418,541,658.56 | 0.0% | 15.84 | 10.60 | 1.494 | 1.92 | 0.0% | 0.57 | `Pow_Shavite512_1M` ``` </details> * <details> <summary>X11:</summary> ``` | ns/byte | byte/s | err% | ins/byte | cyc/byte | IPC | bra/byte | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 271.74 | 3,679,981.60 | 0.1% | 4,875.78 | 1,206.89 | 4.040 | 49.66 | 0.0% | 0.01 | `Pow_X11_0032b` | 108.70 | 9,199,814.12 | 0.2% | 1,950.20 | 482.28 | 4.044 | 19.84 | 0.0% | 0.01 | `Pow_X11_0080b` | 69.40 | 14,408,573.10 | 0.2% | 1,240.79 | 308.15 | 4.027 | 12.47 | 0.0% | 0.01 | `Pow_X11_0128b` | 18.33 | 54,546,702.44 | 0.1% | 326.29 | 81.43 | 4.007 | 3.18 | 0.0% | 0.01 | `Pow_X11_0512b` | 9.78 | 102,262,967.62 | 0.2% | 173.88 | 43.39 | 4.007 | 1.63 | 0.0% | 0.01 | `Pow_X11_1024b` | 5.48 | 182,512,076.76 | 0.1% | 97.67 | 24.33 | 4.015 | 0.86 | 0.0% | 0.01 | `Pow_X11_2048b` | 1.20 | 835,292,126.52 | 0.0% | 21.62 | 5.31 | 4.068 | 0.09 | 0.0% | 0.29 | `Pow_X11_1M` ``` </details> ## Breaking Changes Platforms that partially support the set of extensions for x86_64 (SSSE3, SSE4.1 and AES-NI) or ARMv7/ARMv8 (NEON and crypto extensions for AES) may experience different performance characteristics. The following cases may experience performance degradation due to moving to a small-footprint variant and the overhead (and lost optimization opportunity) incurred by runtime dispatching. * Platforms other than x86_64 or ARMv7 and above (there are no backends that implement optimized routines for them) * x86_64 or ARMv7/ARMv8, without extensions (i.e. older chips, OS/Hypervisor-level disablement of extensions) * Operating systems other than Windows, macOS, Linux and FreeBSD (there are no dispatch routines for other platforms) * Using `libdashconsensus` (optimizations are disabled wholesale for the library following upstream behavior) ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK/light-ACK 005967b Tree-SHA512: 8b23f8e4e591faa4fbdb75ef85a64fed5f0429c804e650e8389789468d7e594998b11228794d650adfaeaf65085d157f98533983a7893d8db5ba587341c86e44
3 participants
Additional Information
make checkupstream includes a dry run of the benchmarking suite, this behavior was introduced in bitcoin#14092 (a557022) but was disabled for Dash Core as our benchmarking suite is substantially heavier. Unfortunately, as a side effect of that, failures in the benchmark suite go unnoticed by CI (see e507a51 for one such fix).With the introduction of bitcoin#25107 and bitcoin#26158, the sanity checks require significantly less overhead and should be suitable for reintroduction into the
make checkrotation.The sanity check caught failures in the test suite (build) due to missing initialization or insufficient checking and they have been remedied in this pull request. An example is available below.
Debugger stacktrace
The benchmark
BLS_Verify_BatchedParallelalso has a data race that was spotted courtesy ofmake check(build) that has been resolved by using an atomic.To allow backporting bitcoin#27598, the naming scheme changes in
src/bench/crypto_hash.cppintroduced by dash#1925 have been effectively reverted and Dash-specific benchmarks have been moved downward to mitigate merge conflict issues.The order of libraries for the linker had to be rearranged as the linker reads dependencies from left to right (i.e. top to bottom), which makes them ordering sensitive. Without this, the linker complains visibly (see below).
Linker errors:
Breaking Changes
None expected.
Checklist