fix: avoid possible nullptr for unknown hash of qc after LookupBlockIndex #6789
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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.
src/init.cpp
Outdated
| 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.
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
fix-blockprocessor-lookup
branch
from
b455148 to
2636327
Compare
knst
force-pushed
the
fix-blockprocessor-lookup
branch
from
2636327 to
8018a92
Compare
knst
force-pushed
the
fix-blockprocessor-lookup
branch
from
8018a92 to
3b9e061
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/llmq/dkgsessionhandler.cpp (1)
540-546: Refine quorum base block validation with active‐chain membership and diagnostic logsTo prevent initializing a DKG session on a stale or reorged branch, verify that the quorum base block is on the active chain via
ActiveChain().Contains(). Emitting aLogPrintin each failure path will aid post-mortem debugging. For example:- const CBlockIndex* pQuorumBaseBlockIndex = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(curQuorumHash)); - - if (!pQuorumBaseBlockIndex || !InitNewQuorum(pQuorumBaseBlockIndex)) { - // should actually never happen - WaitForNewQuorum(curQuorumHash); - throw AbortPhaseException(); - } + // Lookup and ensure the block is on the active chain + const CBlockIndex* pQuorumBaseBlockIndex = WITH_LOCK(::cs_main, { + const auto* pidx = m_chainstate.m_blockman.LookupBlockIndex(curQuorumHash); + return (pidx && m_chainstate.m_chainman.ActiveChain().Contains(pidx)) ? pidx : nullptr; + }); + + if (!pQuorumBaseBlockIndex) { + LogPrint(BCLog::LLMQ_DKG, + "CDKGSessionHandler::%s -- %s qi[%d] - quorum base block %s not on active chain; waiting\n", + __func__, params.name, quorumIndex, curQuorumHash.ToString()); + WaitForNewQuorum(curQuorumHash); + throw AbortPhaseException(); + } + + if (!InitNewQuorum(pQuorumBaseBlockIndex)) { + LogPrint(BCLog::LLMQ_DKG, + "CDKGSessionHandler::%s -- %s qi[%d] - InitNewQuorum failed at height=%d for hash=%s; waiting\n", + __func__, params.name, quorumIndex, pQuorumBaseBlockIndex->nHeight, curQuorumHash.ToString()); + WaitForNewQuorum(curQuorumHash); + throw AbortPhaseException(); + }Key changes:
- Use
m_chainstate.m_chainman.ActiveChain().Contains(pidx)instead of directly accessing a non‐existentm_chain.- Split the null and init‐failure checks to provide specific log messages for each case.
- Retain the existing
WaitForNewQuorum()and exception throw behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/llmq/blockprocessor.cpp(2 hunks)src/llmq/dkgsessionhandler.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/llmq/blockprocessor.cpp
🧰 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/dkgsessionhandler.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: knst
PR: dashpay/dash#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
PR: dashpay/dash#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.
⏰ 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_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: Lint / Run linters
Issue being fixed or feature implemented
Follow-up for #6692; discovered possible nullptr usage after looking for non-existing block
What was done?
Added extra check for nullptr in 3 missing places in dash specific code:
How Has This Been Tested?
N/A
Breaking Changes
N/A
Checklist: