fix: bls benchmarks crash when ran independently

[PastaPastaPasta]

Issue being fixed or feature implemented

Crashes like below happen when BLS bench's are ran w/o BLS_DKG bench's

> $ ./src/bench/bench_dash --filter="BLS_.*"                                                                     [±ci/run-bench ●]

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|           70,625.77 |           14,159.14 |    0.5% |      0.08 | `BLS_PubKeyAggregate_Batch_100`
|          143,101.94 |            6,988.03 |    0.6% |      0.17 | `BLS_PubKeyAggregate_Batch_200`
|           16,714.92 |           59,826.77 |    1.4% |      0.02 | `BLS_PubKeyAggregate_Batch_25`
|            2,733.86 |          365,783.71 |    0.9% |      0.01 | `BLS_PubKeyAggregate_Batch_5`
|           34,708.34 |           28,811.52 |    0.7% |      0.04 | `BLS_PubKeyAggregate_Batch_50`
|           77,659.39 |           12,876.74 |    0.5% |      0.09 | `BLS_PubKeyAggregate_Iterative_100`
|          155,927.75 |            6,413.23 |    0.8% |      0.19 | `BLS_PubKeyAggregate_Iterative_200`
|           18,291.67 |           54,669.70 |    1.3% |      0.02 | `BLS_PubKeyAggregate_Iterative_25`
|            2,830.29 |          353,320.46 |    0.5% |      0.01 | `BLS_PubKeyAggregate_Iterative_5`
|           37,932.04 |           26,362.94 |    0.9% |      0.05 | `BLS_PubKeyAggregate_Iterative_50`
|              777.73 |        1,285,787.58 |    1.5% |      0.01 | `BLS_PubKeyAggregate_Normal`
|              390.42 |        2,561,315.62 |    1.2% |      0.01 | `BLS_SecKeyAggregate_Normal`
|          516,138.10 |            1,937.47 |    0.3% |      0.62 | `BLS_Sign_Normal`
|            1,463.50 |          683,294.10 |    0.8% |      0.01 | `BLS_SignatureAggregate_Normal`
expected valid but it is invalid
Assertion failure:
  assertion: false
  file: bls.cpp, line: 301
  function: operator()
   0#: (0x103732300) stacktraces.cpp:640   - __assert_rtn
   1#: (0x102AB46CC) basic_ostream.h       - BLS_Verify_Batched(ankerl::nanobench::Bench&)::$_0::operator()() const
   2#: (0x102AB46CC) nanobench.h:1221      - ankerl::nanobench::Bench& ankerl::nanobench::Bench::run<BLS_Verify_Batched(ankerl::nanobench::Bench&)::$_0>(BLS_Verify_Batched(ankerl::nanobench::Bench&)::$_0&&)
   3#: (0x102AAFBE4) vector:1933           - std::__1::vector<bool, std::__1::allocator<bool>>::__destroy_vector::operator()[abi:ne190102]()
   4#: (0x102AAFBE4) vector:1942           - std::__1::vector<bool, std::__1::allocator<bool>>::~vector[abi:ne190102]()
   5#: (0x102AAFBE4) vector:1942           - std::__1::vector<bool, std::__1::allocator<bool>>::~vector[abi:ne190102]()
   6#: (0x102AAFBE4) bls.cpp:305           - BLS_Verify_Batched(ankerl::nanobench::Bench&)
   7#: (0x102A9605C) function.h            - std::__1::__function::__value_func<void (ankerl::nanobench::Bench&)>::operator()[abi:ne190102](ankerl::nanobench::Bench&) const
   8#: (0x102A9605C) function.h:989        - std::__1::function<void (ankerl::nanobench::Bench&)>::operator()(ankerl::nanobench::Bench&) const
   9#: (0x102A9605C) bench.cpp:120         - benchmark::BenchRunner::RunAll(benchmark::Args const&)
  10#: (0x102AA8EE4) string:1881           - std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::__is_long[abi:ne190102]() const
  11#: (0x102AA8EE4) string:1212           - std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::~basic_string()
  12#: (0x102AA8EE4) string:1210           - std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::~basic_string()
  13#: (0x102AA8EE4) bench.h:54            - benchmark::Args::~Args()
  14#: (0x102AA8EE4) bench.h:54            - benchmark::Args::~Args()
  15#: (0x102AA8EE4) bench_bitcoin.cpp:137 - main
  16#: (0x1919C0274) <unknown-file>        - ???
Assertion failed: (false), function operator(), file bls.cpp, line 301.
[1]    20836 abort      ./src/bench/bench_dash --filter="BLS_.*"

What was done?

Don't assume scheme

How Has This Been Tested?

Bench ran

Breaking Changes

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[coderabbitai]

Walkthrough

The changes add BLS (Boneh-Lynn-Shacham) signature scheme support to the Bitcoin benchmark file. This includes importing the BLS header, initializing BLS during setup via BLSInit(), and explicitly disabling the legacy BLS scheme by setting bls::bls_legacy_scheme to false. These modifications enable the benchmark to test with the non-legacy BLS implementation.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

• Simple header inclusion and initialization setup in a benchmark file
• No complex logic or control flow changes
• No impact on public APIs or exported entities
• Straightforward configuration flag modification

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: fixing BLS benchmarks that crash when run independently, which is exactly what the changeset addresses.
Description check	✅ Passed	The description explains the crash issue, provides reproduction steps, identifies the root cause, and describes the fix, which is directly related to the changeset modifying BLS initialization.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f6bae67cc12beecc2c995c627543ec84c7dedc17 and f961e04.

📒 Files selected for processing (1)

• src/bench/bench_bitcoin.cpp (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/bench/bench_bitcoin.cpp

⏰ 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). (3)

• GitHub Check: Lint / Run linters
• GitHub Check: x86_64-pc-linux-gnu / Build depends
• GitHub Check: arm-linux-gnueabihf / Build depends

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[UdjinM6]

I guess with BLSDKG_ we are benchmarking basic bls scheme in BLS_ ones somehow so there are no crashes. With this patch and no BLSDKG_ we'll benchmark legacy scheme so results would be kind of useless/confusing. How about forcing basic scheme here instead? see a3896db2b2

[PastaPastaPasta]

@UdjinM6 why do you suggest storing / resetting the atomic; why not just enforce it to false at the beginning of each bench?

[UdjinM6]

@UdjinM6 why do you suggest storing / resetting the atomic; why not just enforce it to false at the beginning of each bench?

This could work too I guess, I just tried to avoid any potential side effects for other benchmarks.

[UdjinM6]

Actually... we could probably just set it to false for everyone.

diff --git a/src/bench/bench_bitcoin.cpp b/src/bench/bench_bitcoin.cpp
index c5fe3fbdfd..77d5232377 100644
--- a/src/bench/bench_bitcoin.cpp
+++ b/src/bench/bench_bitcoin.cpp
@@ -11,6 +11,8 @@
 #include <util/strencodings.h>
 #include <util/system.h>
 
+#include <bls/bls.h>
+
 #include <chrono>
 #include <cstdint>
 #include <iostream>
@@ -64,6 +66,7 @@ int main(int argc, char** argv)
     SetupBenchArgs(argsman);
     SapphireAutoDetect();
     SHA256AutoDetect();
+    bls::bls_legacy_scheme.store(false);
     std::string error;
     if (!argsman.ParseParameters(argc, argv, error)) {
         tfm::format(std::cerr, "Error parsing command line arguments: %s\n", error);

[UdjinM6]

Another thing I just noticed is that there is no BLSInit(); in main() in bench_bitcoin.cpp. Is it ok @coderabbitai?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# Find and examine the BLSInit() function definition
echo "=== Finding BLSInit() definition ==="
ast-grep --pattern 'BLSInit() {
  $$$
}'

echo -e "\n=== Searching for BLSInit declaration/definition ==="
rg -nP --type=cpp -A10 'bool\s+BLSInit\(\)|void\s+BLSInit\(\)'

echo -e "\n=== Looking at bench_bitcoin.cpp main function ==="
ast-grep --pattern 'int main($$$) {
  $$$
}' src/bench/bench_bitcoin.cpp

echo -e "\n=== Finding all calls to BLSInit ==="
rg -nP --type=cpp 'BLSInit\(\)' -C2

echo -e "\n=== Examining bls.h header for initialization requirements ==="
fd -t f '^bls\.h$' --exec cat {}

Length of output: 18951

---

🏁 Script executed:

#!/bin/bash

# Get the bench_bitcoin.cpp file to see the main function
echo "=== Checking bench_bitcoin.cpp main function ==="
cat src/bench/bench_bitcoin.cpp

echo -e "\n=== Checking if BUILD_BITCOIN_INTERNAL is defined for benchmarks ==="
rg -nP 'BUILD_BITCOIN_INTERNAL' --type=cpp src/bench/

Length of output: 6776

---

🏁 Script executed:

#!/bin/bash

# Check how BUILD_BITCOIN_INTERNAL is used in the build system
echo "=== Checking BUILD_BITCOIN_INTERNAL definition in build files ==="
rg -nP 'BUILD_BITCOIN_INTERNAL' --type=make

echo -e "\n=== Checking CMakeLists or configure files ==="
fd -t f 'CMakeLists.txt|configure.ac|Makefile.am' --exec rg -l 'BUILD_BITCOIN_INTERNAL' {}

echo -e "\n=== Checking for references in any build-related files ==="
rg -nP 'BUILD_BITCOIN_INTERNAL' -g '*.am' -g '*.ac' -g 'CMakeLists.txt' -C2

echo -e "\n=== Checking where secure_allocate and secure_free are defined ==="
rg -nP 'secure_allocate|secure_free' --type=cpp -C2 | head -50

Length of output: 1646

---

@UdjinM6 No, it's not okay. The missing BLSInit(); call in bench_bitcoin.cpp is a problem that should be fixed.

Here's why:Based on my analysis:

What BLSInit() does:

• When BUILD_BITCOIN_INTERNAL is NOT defined (which is the case for benchmarks), it calls bls::BLS::SetSecureAllocator(secure_allocate, secure_free) to configure secure memory allocation for BLS cryptographic operations
• The secure_allocate and secure_free functions use Dash's secure allocator to protect sensitive cryptographic material in memory

Why it's missing:

• bench_bitcoin.cpp calls SapphireAutoDetect() and SHA256AutoDetect() but does NOT call BLSInit()
• Both the test framework (src/test/util/setup_common.cpp:187) and the main application (src/init/common.cpp:49) properly call BLSInit() during initialization

Why it matters:

• Without BLSInit(), the BLS library won't have secure memory allocators configured
• This can lead to undefined behavior or crashes when BLS operations try to allocate memory for private keys and other sensitive data
• The crash you're seeing in the PR could be partially related to this missing initialization

Recommendation:
You should add BLSInit(); to bench_bitcoin.cpp's main() function, similar to how it's done for SHA256AutoDetect() and SapphireAutoDetect(). Here's the suggested change:

#include <bench/bench.h>

#include <bls/bls.h>  // Add this include
#include <clientversion.h>
#include <crypto/sha256.h>
#include <crypto/x11/dispatch.h>
#include <fs.h>
#include <util/strencodings.h>
#include <util/system.h>

// ... rest of code ...

int main(int argc, char** argv)
{
    ArgsManager argsman;
    SetupBenchArgs(argsman);
    SapphireAutoDetect();
    SHA256AutoDetect();
    BLSInit();  // Add this line
    std::string error;
    // ... rest of main function ...
}

This ensures proper initialization of the BLS library's secure memory allocators before any benchmarks run.

ฅ^•ﻌ•^ฅ

---

🧠 Learnings used

Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.

---

Learnt from: kwvg
Repo: dashpay/dash 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.

---

Learnt from: kwvg
Repo: dashpay/dash 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.

---

Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e428db1c77c1640263267f911fb6af9d4308327a and 576c11a3a433242e07f0a419e3a6e714f64aa52f.

📒 Files selected for processing (1)

• src/bench/bench_bitcoin.cpp (3 hunks)

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.

Learnt from: kwvg
Repo: dashpay/dash 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.

Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.

Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.

Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.

📚 Learning: 2025-07-23T09:30:34.631Z

Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
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.

Applied to files:

• src/bench/bench_bitcoin.cpp

⏰ 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: arm-linux-build / Build source
• GitHub Check: linux64_sqlite-build / Build source
• GitHub Check: linux64_fuzz-build / Build source
• GitHub Check: linux64_ubsan-build / Build source
• GitHub Check: linux64-build / Build source
• GitHub Check: mac-build / Build source
• GitHub Check: win64-build / Build source
• GitHub Check: linux64_tsan-build / Build source
• GitHub Check: linux64_nowallet-build / Build source
• GitHub Check: Lint / Run linters

🔇 Additional comments (1)

src/bench/bench_bitcoin.cpp (1)

7-7: LGTM: BLS header inclusion is correct.

The include statement is properly positioned and necessary for accessing the BLSInit() function.

[knst]

I believe by default it will be true, but we shoulld benchmark non-legacy bls, should we?

[PastaPastaPasta]

Are there changes remaining @knst @UdjinM6 ?

[UdjinM6]

Are there changes remaining @knst @UdjinM6 ?

should drop original commits and do #6977 (comment) instead or enforce basic bls scheme in any other way

[knst]

LGTM f961e04

[UdjinM6]

ACK f961e04