fix: bls benchmarks crash when ran independently #6977
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThis pull request updates BLS-related bench code. In src/bench/bls.cpp, hardcoded Sequence Diagram(s)(Skipped — changes are simple parameter substitutions and an initialization call; no new control flow worth diagramming.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~5–15 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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). (10)
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. Comment |
|
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 a3896db |
|
@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. |
|
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); |
|
Another thing I just noticed is that there is no |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 18951 🏁 Script executed: Length of output: 6776 🏁 Script executed: Length of output: 1646 @UdjinM6 No, it's not okay. The missing Here's why:Based on my analysis: What
Why it's missing:
Why it matters:
Recommendation: #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 |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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.
| SetupBenchArgs(argsman); | ||
| SapphireAutoDetect(); | ||
| SHA256AutoDetect(); | ||
| BLSInit(); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify error handling and cleanup requirements for BLSInit().
The BLSInit() call correctly addresses the reported crash and is properly placed before argument parsing, consistent with other crypto initialization calls (SapphireAutoDetect, SHA256AutoDetect). However, ensure that:
- BLSInit() doesn't require error handling (return value checking)
- No cleanup function is needed at exit (or document why it's acceptable to skip for benchmarks)
- The initialization approach is consistent with how other Dash binaries (dashd, dash-cli) initialize BLS
Run the following script to verify BLSInit() usage patterns across the codebase:
🏁 Script executed:
#!/bin/bash
# Description: Check BLSInit() signature, return type, and usage patterns in other binaries
# Find BLSInit() declaration to check signature and return type
echo "=== BLSInit() declaration ==="
rg -n "BLSInit\s*\(" --type=cpp --type=h -A 2
echo -e "\n=== BLSInit() usage in main() functions ==="
# Find how other binaries initialize BLS
rg -n "int main\(" --type=cpp -A 20 | rg -B 5 -A 2 "BLSInit"
echo -e "\n=== Check for BLS cleanup functions ==="
# Look for potential cleanup functions
rg -n "BLS.*(?:Cleanup|Shutdown|Deinit|Destroy)" --type=cpp --type=hLength of output: 1253
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine BLSInit() implementation to understand failure modes
echo "=== BLSInit() implementation ==="
sed -n '428,480p' src/bls/bls.cpp
echo -e "\n=== Check context around test setup BLSInit() call ==="
sed -n '180,195p' src/test/util/setup_common.cpp
echo -e "\n=== Check context around init/common.cpp BLSInit() call ==="
sed -n '45,60p' src/init/common.cpp
Length of output: 1261
Check BLSInit() return value in benchmark binary.
BLSInit() returns bool and is designed to indicate initialization success/failure. The established pattern in src/init/common.cpp:49-51 shows proper error handling with if (!BLSInit()) { return false; }. The benchmark binary at line 69 must follow the same defensive coding practice instead of ignoring the return value.
Change:
BLSInit();To:
if (!BLSInit()) {
return false; // or appropriate error handling for benchmarks
}🤖 Prompt for AI Agents
In src/bench/bench_bitcoin.cpp around line 69, BLSInit() is called without
checking its boolean return; update this to check the return value and handle
failure—if BLSInit() returns false, immediately return false from the enclosing
function (or call exit(1)/return a non-zero status if this is main) so the
benchmark binary fails fast on initialization error.
| } | ||
| Shuffle(invalid.begin(), invalid.end(), FastRandomContext()); | ||
|
|
||
| const bool scheme = bls::bls_legacy_scheme.load(); |
There was a problem hiding this comment.
I believe by default it will be true, but we shoulld benchmark non-legacy bls, should we?
3 participants
Issue being fixed or feature implemented
Crashes like below happen when BLS bench's are ran w/o BLS_DKG bench's
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
xin all the boxes that apply.