fix: avoid possible nullptr for unknown hash of qc after LookupBlockIndex #6789
Conversation
It introduces new commandline argument -parbls to set up amount of parallel threads for BLS validation New parallel BlsChecker validates asynchonously quorumSig and membersSig in Quorum Commitment
Co-authored-by: UdjinM6 <[email protected]>
|
WalkthroughThis change introduces asynchronous and parallelized BLS (Boneh–Lynn–Shacham) signature verification for quorum final commitments during block processing. A new command-line argument, Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes This review involves changes to core block processing logic, multi-threading, new configuration options, refactoring of cryptographic verification, and the introduction of new utility structures. The changes span several files and require careful attention to concurrency, correctness of signature verification, and integration with existing block validation flows. Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/init.cpp(1 hunks)src/llmq/blockprocessor.cpp(5 hunks)src/llmq/blockprocessor.h(5 hunks)src/llmq/commitment.cpp(3 hunks)src/llmq/commitment.h(2 hunks)src/llmq/options.h(1 hunks)src/llmq/utils.cpp(1 hunks)src/llmq/utils.h(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/llmq/blockprocessor.hsrc/llmq/options.hsrc/init.cppsrc/llmq/commitment.cppsrc/llmq/utils.cppsrc/llmq/commitment.hsrc/llmq/blockprocessor.cppsrc/llmq/utils.h
🧠 Learnings (6)
📓 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: 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#6761
File: src/chainlock/signing.cpp:15-250
Timestamp: 2025-07-23T09:28:32.783Z
Learning: In refactoring PRs like #6761, kwvg prefers to defer code formatting fixes to separate follow-up PRs when formatting is not the primary objective, to maintain focus on the structural changes and avoid scope creep.
Learnt from: knst
PR: dashpay/dash#6533
File: test/functional/feature_llmq_singlenode.py:98-106
Timestamp: 2025-01-22T08:33:31.405Z
Learning: When reviewing PRs, ensure that suggestions are directly related to the PR's primary objectives rather than general code improvements that could be addressed separately.
src/llmq/blockprocessor.h (4)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/**/*.{cpp,h,cc,cxx,hpp} : Dash uses unordered_lru_cache for efficient caching with LRU eviction
Learnt from: kwvg
PR: #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.
Learnt from: kwvg
PR: #6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The CQuorumManager is fully initialized by LLMQContext, addressing any concerns about the manager’s initialization sequence.
Learnt from: knst
PR: #6691
File: src/test/llmq_params_tests.cpp:148-151
Timestamp: 2025-07-15T14:53:04.819Z
Learning: In the Dash Core LLMQ implementation, signingActiveQuorumCount is never 0 in the actual parameters defined in params.h, making division by zero scenarios unrealistic in the max_cycles() function.
src/llmq/commitment.cpp (4)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h
Learnt from: kwvg
PR: #6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/is_basic_scheme_active=/true, /is_extended_addr=/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Learnt from: knst
PR: #6691
File: src/test/llmq_params_tests.cpp:148-151
Timestamp: 2025-07-15T14:53:04.819Z
Learning: In the Dash Core LLMQ implementation, signingActiveQuorumCount is never 0 in the actual parameters defined in params.h, making division by zero scenarios unrealistic in the max_cycles() function.
src/llmq/commitment.h (3)
Learnt from: kwvg
PR: #6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the CInstantSendManager. Previously, this was handled globally; now it's handled as a class member in LLMQContext, but the concurrency control remains consistent.
Learnt from: kwvg
PR: #6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The CQuorumManager is fully initialized by LLMQContext, addressing any concerns about the manager’s initialization sequence.
Learnt from: knst
PR: #6691
File: src/test/llmq_params_tests.cpp:148-151
Timestamp: 2025-07-15T14:53:04.819Z
Learning: In the Dash Core LLMQ implementation, signingActiveQuorumCount is never 0 in the actual parameters defined in params.h, making division by zero scenarios unrealistic in the max_cycles() function.
src/llmq/blockprocessor.cpp (1)
Learnt from: knst
PR: #6691
File: src/test/llmq_params_tests.cpp:148-151
Timestamp: 2025-07-15T14:53:04.819Z
Learning: In the Dash Core LLMQ implementation, signingActiveQuorumCount is never 0 in the actual parameters defined in params.h, making division by zero scenarios unrealistic in the max_cycles() function.
src/llmq/utils.h (3)
Learnt from: kwvg
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.
Learnt from: kwvg
PR: #6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The CQuorumManager is fully initialized by LLMQContext, addressing any concerns about the manager’s initialization sequence.
Learnt from: knst
PR: #6691
File: src/test/llmq_params_tests.cpp:148-151
Timestamp: 2025-07-15T14:53:04.819Z
Learning: In the Dash Core LLMQ implementation, signingActiveQuorumCount is never 0 in the actual parameters defined in params.h, making division by zero scenarios unrealistic in the max_cycles() function.
🧬 Code Graph Analysis (3)
src/init.cpp (1)
src/util/system.cpp (2)
GetNumCores(1450-1453)GetNumCores(1450-1450)
src/llmq/blockprocessor.cpp (1)
src/llmq/blockprocessor.h (1)
CQuorumBlockProcessor(42-91)
src/llmq/utils.h (1)
src/llmq/utils.cpp (8)
m_pubkeys(943-960)InitQuorumsCache(963-969)InitQuorumsCache(963-963)InitQuorumsCache(970-970)InitQuorumsCache(971-971)InitQuorumsCache(972-972)InitQuorumsCache(973-973)InitQuorumsCache(974-974)
🔇 Additional comments (10)
src/llmq/options.h (1)
27-30: LGTM! Well-defined BLS thread configuration constants.The constants are appropriately named, documented, and follow good practices:
MAX_BLSCHECK_THREADS = 31provides a reasonable upper bound to prevent resource exhaustionDEFAULT_BLSCHECK_THREADS = 0enables automatic configuration by default- Clear comments explain the purpose of each constant
src/llmq/utils.cpp (1)
943-960: LGTM! Well-implemented BLS signature verification logic.The
operator()correctly handles different signature verification scenarios:
- Multiple public keys: Uses
VerifySecureAggregatedfor proper aggregated signature verification- Single public key: Uses
VerifyInsecurefor individual signature verification- Zero public keys: Properly rejects with clear error message
- Appropriate error logging on verification failures
The implementation follows BLS library best practices and provides good error handling.
src/llmq/blockprocessor.h (1)
10-13: Inconsistency: Changes don't match PR objectivesThese changes introduce parallel BLS signature verification infrastructure, which doesn't align with the PR objective of "fix: avoid possible nullptr for unknown hash of qc after LookupBlockIndex". The PR claims to add a nullptr check, but these modifications implement a completely different feature.
Also applies to: 50-51, 62-63, 86-87
Likely an incorrect or invalid review comment.
src/llmq/utils.h (1)
8-8: Inconsistency: BlsCheck struct unrelated to PR objectivesThe addition of the
BlsCheckstruct is for parallel BLS signature verification, which is unrelated to the stated PR objective of fixing a nullptr issue afterLookupBlockIndex.Also applies to: 60-86
Likely an incorrect or invalid review comment.
src/llmq/commitment.h (1)
28-29: Inconsistency: Asynchronous verification unrelated to PR objectivesThe addition of
VerifySignatureAsyncand related declarations supports parallel BLS verification, which doesn't address the stated goal of fixing a nullptr dereference afterLookupBlockIndex.Also applies to: 35-37, 75-77
Likely an incorrect or invalid review comment.
src/llmq/commitment.cpp (1)
31-95: Implementation doesn't address PR objectivesThis new
VerifySignatureAsyncmethod and the modification toVerifyimplement parallel BLS signature verification. There are no changes here that fix a nullptr issue afterLookupBlockIndexas stated in the PR objectives.Also applies to: 177-177
Likely an incorrect or invalid review comment.
src/llmq/blockprocessor.cpp (4)
57-71: Unrelated change: BLS thread pool initializationThis code initializes a thread pool for BLS signature verification, which is unrelated to the PR's stated objective of fixing a nullptr issue.
Likely an incorrect or invalid review comment.
215-231: Unrelated change: Parallel BLS verificationThis asynchronous BLS signature verification logic is not mentioned in the PR objectives and appears to be a separate feature implementation.
Likely an incorrect or invalid review comment.
310-314: This is the actual fix mentioned in PR objectivesThis nullptr check after
LookupBlockIndexis the only change that matches the PR description. IfpQuorumBaseBlockIndexis null (meaning the block hash wasn't found), the function now returns false instead of potentially dereferencing a nullptr.
258-259: Signature change unrelated to nullptr fixThe removal of the
fBLSChecksparameter fromProcessCommitmentand hardcodingcheckSigs=falsein theVerifycall are part of the BLS parallelization refactoring, not the nullptr fix.Also applies to: 317-317
Likely an incorrect or invalid review comment.
| argsman.AddArg("-parbls=<n>", strprintf("Set the number of BLS verification threads (%u to %d, 0 = auto, <0 = leave that many cores free, default: %d)", | ||
| -GetNumCores(), llmq::MAX_BLSCHECK_THREADS, llmq::DEFAULT_BLSCHECK_THREADS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure runtime handling of the new -parbls argument mirrors -par semantics
The CLI flag is registered correctly, but I don’t see corresponding logic in this file that:
- Parses the user-supplied value (incl. negative “leave N cores free” and
0 = autocases). - Caps the effective thread count to
llmq::MAX_BLSCHECK_THREADS. - Spawns the thread pool (similar to the
-parcode around lines 1494-1511).
Please double-check that this is implemented in the LLMQ block-processor path; otherwise the option will be a no-op or may allow out-of-range values.
🏁 Script executed:
#!/bin/bash
# Locate all usages of the new -parbls argument
rg --line-number --fixed-strings "-parbls"Length of output: 439
🏁 Script executed:
#!/bin/bash
# Search for literal "-parbls" registration/usage
rg -n --fixed-strings "-parbls" .
# Search for any references to "parbls" (without the dash) in the code
rg -n "parbls" .Length of output: 275083
Missing runtime handling for -parbls in src/init.cpp
The new -parbls flag is only registered; there’s no code to:
- Read the user‐supplied value (
gArgs.GetArg("-parbls", …)) - Interpret
0 = autoor<0 = leave N cores free - Clamp between
llmq::DEFAULT_BLSCHECK_THREADSandllmq::MAX_BLSCHECK_THREADS - Pass the resulting thread count into the LLMQ BLS batch‐verifier or block‐processor
Locations needing updates:
- src/init.cpp at the call to
ArgsManager::AddArg("-parbls=…")(around line 533): add aGetArgcall, normalize the value, and store it. - LLMQ block‐processor setup (e.g. where the BLS verifier or block‐processor is initialized): invoke the BLS thread pool creation using the parsed & clamped
nBlsThreads.
Please mirror the existing -par logic (around lines 1494–1511 in init.cpp) to implement full support for -parbls.
🤖 Prompt for AI Agents
In src/init.cpp around lines 533 to 534, the new -parbls argument is registered
but lacks runtime handling. To fix this, add code to read the user-supplied
value using gArgs.GetArg("-parbls", default), interpret 0 as auto and negative
values as leaving that many cores free, clamp the value between
llmq::DEFAULT_BLSCHECK_THREADS and llmq::MAX_BLSCHECK_THREADS, and store the
normalized thread count. Then, locate the LLMQ block-processor or BLS verifier
initialization (around lines 1494–1511) and update it to use the parsed and
clamped nBlsThreads value for thread pool creation, mirroring the existing -par
logic implementation.
Depends on #6692
Issue being fixed or feature implemented
Follow-up for #6692; discovered possible nullptr usage after looking for non-existing block
What was done?
Extra check for nullptr
How Has This Been Tested?
N/A
Breaking Changes
N/A
Checklist: