feat: enhance CQuorumManager with active chain view and ancestor caching #6962
Conversation
- Introduced ActiveChainView structure to minimize cs_main contention. - Added ComputeAncestorCacheMaxSize method to determine cache size limits. - Implemented FindAncestorFast for efficient ancestor lookups. - Integrated ChainViewSubscriber for block tip updates and cache management. - Updated relevant methods to utilize the new caching mechanism for improved performance.
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThe changes add an ActiveChainView snapshot and an LRU ancestor cache to CQuorumManager, with ComputeAncestorCacheMaxSize(), GetActiveChainView(), and FindAncestorFast(...) helpers. A ChainViewSubscriber (CValidationInterface) keeps the snapshot in sync with tip updates and clears or updates the ancestor cache on reorgs/extensions. The constructor/bootstrap initialize the cache and register the subscriber. ScanQuorums, SelectQuorumForSigning, and related flows prefer the active view and FindAncestorFast with safe fallbacks to prior cs_main-based GetAncestor logic. Sequence Diagram(s)sequenceDiagram
actor Init as Initialization
participant QM as CQuorumManager
participant CSV as ChainViewSubscriber
participant VI as ValidationInterface
participant Chain as Blockchain
Init->>QM: Constructor
QM->>QM: m_ancestor_lru ← ComputeAncestorCacheMaxSize()
QM->>QM: Bootstrap ActiveChainView(current tip) (under lock)
QM->>CSV: Create ChainViewSubscriber
QM->>VI: Register ChainViewSubscriber
VI->>CSV: Notify tip updates
rect rgba(59,130,246,0.08)
Note over QM: Ancestor lookup using active snapshot
QM->>QM: GetActiveChainView()
QM->>QM: FindAncestorFast(view, height)
alt Cache Hit
QM-->>Caller: Return cached CBlockIndex
else Cache Miss
QM->>Chain: GetAncestor(height) (fallback)
Chain-->>QM: CBlockIndex
QM->>QM: Update m_ancestor_lru
QM-->>Caller: Return CBlockIndex
end
end
rect rgba(16,185,129,0.06)
Note over CSV,QM: On chain update notifications
Chain->>CSV: UpdatedBlockTip(new_tip, fork_point)
alt Reorg (fork point changed)
CSV->>QM: Clear m_ancestor_lru
CSV->>QM: Set ActiveChainView(new_tip)
else Extension
CSV->>QM: Set ActiveChainView(new_tip)
Note right of QM: Cache may remain partially valid
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to focus review on:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/llmq/quorums.h (1)
1-1: Fix clang-format issues.The CI pipeline detected formatting differences. Run the formatting script as indicated in the pipeline failure.
src/llmq/quorums.cpp (1)
1-1: Fix clang-format issues.The CI pipeline detected formatting differences in this file. Run the formatting script as indicated in the pipeline failure.
🧹 Nitpick comments (1)
src/llmq/quorums.cpp (1)
1318-1342: Fallback pattern is correct, but const_cast is concerning.The new path using
GetActiveChainView()andFindAncestorFast()properly reduces cs_main contention. However, theconst_cast<CBlockIndex*>at line 1331 is a code smell. While it appears safe here (the pointer isn't modified), consider ifpindexStartcould be declared asconst CBlockIndex*to avoid the cast.If
pindexStartcan be const-qualified throughout the downstream call chain, remove the const_cast:- CBlockIndex* pindexStart; + const CBlockIndex* pindexStart; auto view = qman.GetActiveChainView(); if (view.tip) { // ... const CBlockIndex* ancestor = qman.FindAncestorFast(view, startBlockHeight); if (!ancestor) { return {}; } - pindexStart = const_cast<CBlockIndex*>(ancestor); + pindexStart = ancestor;This would require checking if downstream consumers (like
ScanQuorums) can accept const pointers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
-
src/llmq/quorums.cpp(6 hunks) -
src/llmq/quorums.h(4 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/quorums.hsrc/llmq/quorums.cpp
🧠 Learnings (7)
📓 Common learnings
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.
📚 Learning: 2025-01-14T09:06:19.717Z
Learnt from: kwvg
Repo: dashpay/dash 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.
Applied to files:
src/llmq/quorums.h
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/llmq/quorums.hsrc/llmq/quorums.cpp
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
Repo: dashpay/dash 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.
Applied to files:
src/llmq/quorums.hsrc/llmq/quorums.cpp
📚 Learning: 2025-10-21T11:09:34.688Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In the Dash Core codebase, `CacheMap` (defined in src/cachemap.h) is internally thread-safe and uses its own `mutable CCriticalSection cs` to protect access to its members. Methods like `GetSize()`, `Insert()`, `Get()`, `HasKey()`, etc., can be called without holding external locks.
Applied to files:
src/llmq/quorums.h
📚 Learning: 2025-05-05T12:45:44.781Z
Learnt from: knst
Repo: dashpay/dash PR: 6658
File: src/evo/creditpool.cpp:177-185
Timestamp: 2025-05-05T12:45:44.781Z
Learning: The GetAncestor() function in CBlockIndex safely handles negative heights by returning nullptr rather than asserting, making it safe to call with potentially negative values.
Applied to files:
src/llmq/quorums.cpp
📚 Learning: 2025-07-15T14:53:04.819Z
Learnt from: knst
Repo: dashpay/dash 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.
Applied to files:
src/llmq/quorums.cpp
🧬 Code graph analysis (2)
src/llmq/quorums.h (1)
src/llmq/quorums.cpp (18)
ComputeAncestorCacheMaxSize(212-221)ComputeAncestorCacheMaxSize(212-212)UpdatedBlockTip(264-286)UpdatedBlockTip(264-264)UpdatedBlockTip(402-425)UpdatedBlockTip(402-402)CQuorumManager(288-325)CQuorumManager(327-333)ScanQuorums(622-627)ScanQuorums(622-622)ScanQuorums(629-741)ScanQuorums(629-629)GetActiveChainView(223-227)GetActiveChainView(223-223)FindAncestorFast(229-262)FindAncestorFast(229-229)CheckQuorumConnections(427-495)CheckQuorumConnections(427-428)
src/llmq/quorums.cpp (3)
src/net_processing.cpp (8)
LOCK(323-329)LOCK(336-340)LOCK(341-345)UpdatedBlockTip(2125-2160)UpdatedBlockTip(2125-2125)pindexNew(603-603)pindex(607-607)pindex(849-849)src/evo/deterministicmns.cpp (2)
UpdatedBlockTip(765-770)UpdatedBlockTip(765-765)src/llmq/utils.cpp (8)
InitQuorumsCache(962-968)InitQuorumsCache(962-962)InitQuorumsCache(969-970)InitQuorumsCache(971-972)InitQuorumsCache(973-978)InitQuorumsCache(979-980)InitQuorumsCache(981-982)InitQuorumsCache(983-984)
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/quorums.h
[error] 1-1: Clang-format differences detected in file src/llmq/quorums.h. Run the formatting script to fix: 'git diff -U0 origin/develop --
src/llmq/quorums.cpp
[error] 1-1: Clang-format differences detected in file src/llmq/quorums.cpp. Run the formatting script to fix: 'git diff -U0 origin/develop --
⏰ 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: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-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_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (13)
src/llmq/quorums.h (5)
20-20: LGTM: Includes are appropriate.The new includes support the ChainViewSubscriber (CValidationInterface) and related functionality.
Also applies to: 25-25
272-277: LGTM: Ancestor cache structure is well-designed.The use of
unordered_lru_cachealigns with Dash coding guidelines. The tip tracking variables are appropriately initialized (note thatm_lru_tip_hashdefault-constructs to zero).Based on coding guidelines
280-288: LGTM: ValidationInterface subscriber is properly structured.The nested ChainViewSubscriber class follows the established pattern for ValidationInterface observers (similar to CDeterministicMNManager's approach). The use of
std::unique_ptrand reference to parent is appropriate.
263-270: Initializeheightmember to -1 in ActiveChainView.The
ActiveChainViewstruct should default-initializeheightto -1 to ensure consistency with the pattern used elsewhere (e.g.,m_lru_tip_height{-1}at line 276).Apply this diff:
struct ActiveChainView { const CBlockIndex* tip{nullptr}; - int height; + int height{-1}; uint256 hash; };⛔ Skipped due to learnings
Learnt from: knst Repo: dashpay/dash PR: 6658 File: src/evo/creditpool.cpp:177-185 Timestamp: 2025-05-05T12:45:44.781Z Learning: The GetAncestor() function in CBlockIndex safely handles negative heights by returning nullptr rather than asserting, making it safe to call with potentially negative values.
308-308: Based on my verification of the lock acquisition patterns and ValidationInterface callback context, the code shows:
- Lock usage pattern: UpdatedBlockTip callbacks are enqueued while holding cs_main, but are executed asynchronously by the validation system
- New locks are isolated: The new
cs_active_chain_viewandcs_ancestor_cachelocks are only acquired in theChainViewSubscriber::UpdatedBlockTipcallback (lines 270, 278 in quorums.cpp)- No circular dependencies found: The code establishes a clear lock order during initialization (
cs_main→cs_active_chain_view) and the callback doesn't attempt to re-acquirecs_mainwhile holding the new locks- Public methods are independent: Methods requiring
!cs_active_chain_viewand!cs_ancestor_cacheare not called from within the callback; the callback only performs snapshot updatesLock ordering is correctly implemented with no deadlock risks.
src/llmq/quorums.cpp (8)
28-28: LGTM: Includes are necessary.The new includes support ValidationInterface registration and the
std::clamp/std::maxusage in cache size computation.Also applies to: 30-30
212-221: LGTM: Cache size computation is reasonable.The formula accounts for all LLMQ types and clamps to sensible bounds. The +128 buffer and [512, 4096] limits provide safety margins without excessive memory usage.
223-227: LGTM: Snapshot accessor is correct.Returning a copy of
ActiveChainViewallows lock-free access by callers, which is the intended design.
264-286: LGTM: UpdatedBlockTip correctly handles reorgs and extensions.The reorg detection (
pindexFork != pindexNew->pprev) properly distinguishes between chain extensions and reorgs. Clearing the cache on reorg prevents cross-fork pollution. The use of separate lock scopes avoids lock contention.
327-333: LGTM: Destructor properly cleans up subscriber.Unregistering the ValidationInterface subscriber prevents use-after-free issues. The check for
m_chain_view_subscriberis defensive programming (though it should always be non-null after construction).
622-627: LGTM: ScanQuorums uses new cache with proper fallback.The pattern of checking
view.tipand falling back to the original cs_main-based path ensures correctness during initialization or edge cases. The fallback paths maintain the original behavior.Also applies to: 645-659
229-262: Approved; cache invalidation logic and usage patterns are safe.The snapshot from
GetActiveChainView()is taken atomically under lock, and all callers immediately pass it toFindAncestorFast()without gap, eliminating transient inconsistency. The extension vs. reorg detection correctly clears the cache on reorg, and theGetAncestor()fallback ensures correctness regardless of cache state. No changes needed.
304-304: Lock ordering is consistent and safe. No issues found.The verification confirms that
cs_active_chain_viewacquisitions follow a consistent lock order:
- Constructor (lines 312-315): Acquires
cs_main→cs_active_chain_viewfor bootstrap initialization- UpdatedBlockTip callback (line 270): Called by ValidationInterface while
cs_mainis held by validation.cpp, then acquirescs_active_chain_view—matching the constructor's order- GetActiveChainView() (line 225): Acquires only
cs_active_chain_viewindependentlyNo code path acquires
cs_active_chain_viewbeforecs_main, eliminating reverse-order deadlock risk.
Fix regression introduced in 3db85e2 that caused incorrect quorum information to be returned during chain reorgs and block invalidations. Changes: - ScanQuorums(pindexStart): Compute ancestors strictly from pindexStart's chain context instead of mixing with global ActiveChainView to avoid cross-fork contamination - SelectQuorumForSigning: Add proper fallback to cs_main path when cached view cannot satisfy requested height or cache misses occur This ensures correctness during invalidate/reconsider scenarios while maintaining cs_main-free performance in busy paths when the cached view is sufficient.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/llmq/quorums.cpp (2)
212-221: Consider documenting the +128 safety margin.The cache size computation is reasonable, but the
+ 128safety margin on line 216 lacks explanation. Consider adding a comment to clarify why this specific value was chosen.
1313-1366: Suggest refactoring to reduce complexity and duplication.This function has significant code duplication in the cs_main fallback paths (lines 1333-1341, 1344-1351, 1357-1365). Consider extracting a helper function for the cs_main-based path to improve maintainability.
Additionally, line 1330 uses
const_cast<CBlockIndex*>(ancestor)to remove const. IfIsQuorumRotationEnabledandqman.ScanQuorumscan acceptconst CBlockIndex*, the const_cast is unnecessary and the variable could be declared asconst CBlockIndex* pindexStartor the function signatures could be updated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/llmq/quorums.cpp(6 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/quorums.cpp
🧠 Learnings (5)
📓 Common learnings
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.
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
Repo: dashpay/dash 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.
Applied to files:
src/llmq/quorums.cpp
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/llmq/quorums.cpp
📚 Learning: 2025-05-05T12:45:44.781Z
Learnt from: knst
Repo: dashpay/dash PR: 6658
File: src/evo/creditpool.cpp:177-185
Timestamp: 2025-05-05T12:45:44.781Z
Learning: The GetAncestor() function in CBlockIndex safely handles negative heights by returning nullptr rather than asserting, making it safe to call with potentially negative values.
Applied to files:
src/llmq/quorums.cpp
📚 Learning: 2025-07-15T14:53:04.819Z
Learnt from: knst
Repo: dashpay/dash 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.
Applied to files:
src/llmq/quorums.cpp
🧬 Code graph analysis (1)
src/llmq/quorums.cpp (3)
src/net_processing.cpp (8)
LOCK(323-329)LOCK(336-340)LOCK(341-345)UpdatedBlockTip(2125-2160)UpdatedBlockTip(2125-2125)pindexNew(603-603)pindex(607-607)pindex(849-849)src/llmq/dkgsessionmgr.cpp (2)
UpdatedBlockTip(79-93)UpdatedBlockTip(79-79)src/llmq/utils.cpp (8)
InitQuorumsCache(962-968)InitQuorumsCache(962-962)InitQuorumsCache(969-970)InitQuorumsCache(971-972)InitQuorumsCache(973-978)InitQuorumsCache(979-980)InitQuorumsCache(981-982)InitQuorumsCache(983-984)
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/quorums.cpp
[error] 261-261: Clang-format differences found in quorums.cpp. Apply clang-format to fix formatting differences.
⏰ 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: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (7)
src/llmq/quorums.cpp (7)
28-28: LGTM: Includes added for new functionality.The new includes support the ChainViewSubscriber (CValidationInterface) and utility functions (std::clamp, std::max).
Also applies to: 30-30
223-227: LGTM: Thread-safe getter for active chain view.The function properly locks cs_active_chain_view and returns by value to ensure thread safety.
229-262: Verify thread safety of CBlockIndex::GetAncestor() without cs_main.Lines 242 and 257 call
view.tip->GetAncestor()without holding cs_main. While the CBlockIndex tree structure is generally immutable and this should be safe, please confirm that accessing GetAncestor() without cs_main doesn't introduce race conditions, especially during chain reorganizations.The cache invalidation logic correctly distinguishes between chain extensions (where the cache remains valid) and reorgs (where the cache is cleared).
264-286: LGTM: Proper chain tip update handling.The subscriber correctly updates the active chain view snapshot and manages the ancestor cache, clearing it on reorgs and preserving it on chain extensions. The reorg detection logic at line 279 is correct.
288-325: LGTM: Proper initialization of chain view and cache.The constructor correctly:
- Initializes the ancestor cache with computed size
- Bootstraps the active chain view from the current tip under cs_main
- Registers the validation interface subscriber
The lock ordering (cs_main → cs_active_chain_view) established here should be maintained consistently throughout the codebase.
327-333: LGTM: Proper cleanup in destructor.The destructor correctly unregisters the validation interface subscriber with appropriate null checking.
622-660: LGTM: Appropriate use of active chain view with correct fallback.The changes correctly use the active chain view when available to reduce cs_main contention. Lines 649-653 and 657-659 appropriately use
pindexStart->GetAncestor()directly rather than the cached view to avoid cross-fork contamination, as explained in the comment.
| if (out) { | ||
| m_ancestor_lru.insert(target_height, out); | ||
| } | ||
| return out; |
There was a problem hiding this comment.
Fix clang-format issue.
The CI pipeline reports a clang-format difference at this line. Please run clang-format to fix the formatting.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 261-261: Clang-format differences found in quorums.cpp. Apply clang-format to fix formatting differences.
🤖 Prompt for AI Agents
In src/llmq/quorums.cpp around line 261, the file's formatting at the return
statement does not match clang-format expectations; run clang-format (using the
repo's configured style) on this file or adjust whitespace so the line reads
exactly as formatted by clang-format (e.g., ensure correct indentation and
spacing around the return statement) and commit the resulting change.
…tive chain tip when view lags This addresses rotation test flakiness by avoiding returning quorum sets based on a stale ActiveChainView during rapid tip changes. Default path remains cs_main-free; we only fall back to cs_main when the cached view height is behind the active chain.
… provided Ensure verification paths (e.g. ChainLocks, rotation) derive pindexStart from the authoritative active chain when a specific height is given. Keep cs_main-free fast path only for callers without an explicit height. This avoids mismatches during rapid tip changes and prevents missed ChainLocks in feature_llmq_rotation.py.
…der cs_main Always derive pindexStart from the authoritative active chain (with SIGN_HEIGHT_OFFSET) for both signing and verification. This removes cached-view variability that led to missed ChainLocks in rotation tests, while keeping other paths cs_main-free.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/llmq/quorums.cpp (1)
229-262: Fix clang-format issue (duplicate concern).The cache invalidation logic correctly distinguishes chain extensions from reorgs and maintains thread safety with cs_ancestor_cache. However, a past review flagged a clang-format difference at line 261.
Please run clang-format on this file to address the formatting issue at the return statement (line 261).
🧹 Nitpick comments (1)
src/llmq/quorums.cpp (1)
624-635: Consider simplifying the fallback logic.The current implementation correctly uses the cached chain view with fallbacks to cs_main when needed. However, the double cs_main acquisition (lines 627 and 630-632) could be consolidated for clarity and potential performance improvement.
Consider this refactor:
- auto view = GetActiveChainView(); - const CBlockIndex* pindex = view.tip; - if (pindex == nullptr) { - pindex = WITH_LOCK(::cs_main, return m_chainstate.m_chain.Tip()); - } else { - // If the cached view lags behind the active chain, prefer the up-to-date tip for correctness. - const int active_height = WITH_LOCK(::cs_main, return m_chainstate.m_chain.Height()); - if (view.height < active_height) { - pindex = WITH_LOCK(::cs_main, return m_chainstate.m_chain.Tip()); - } - } + auto view = GetActiveChainView(); + const CBlockIndex* pindex = [&]() { + if (view.tip != nullptr) { + LOCK(::cs_main); + // Use cached view only if it matches current tip height + if (view.height == m_chainstate.m_chain.Height()) { + return view.tip; + } + } + // Fallback to authoritative tip + LOCK(::cs_main); + return m_chainstate.m_chain.Tip(); + }();This consolidates the cs_main acquisitions and makes the intent clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/llmq/quorums.cpp(6 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/quorums.cpp
🧠 Learnings (8)
📓 Common learnings
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.
📚 Learning: 2025-02-14T15:15:58.165Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/rpc/governance.cpp:1074-1089
Timestamp: 2025-02-14T15:15:58.165Z
Learning: Code blocks marked with `// clang-format off` and `// clang-format on` directives should be excluded from clang-format suggestions as they are intentionally formatted manually for better readability.
Applied to files:
src/llmq/quorums.cpp
📚 Learning: 2025-07-09T15:05:36.250Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6729
File: src/rpc/evo.cpp:1273-1273
Timestamp: 2025-07-09T15:05:36.250Z
Learning: When clang-format suggestions significantly harm readability (like splitting logical parameter groups across multiple lines), it's acceptable to use `// clang-format off` and `// clang-format on` directives to exclude the problematic section from automatic formatting, prioritizing code readability over strict line length compliance.
Applied to files:
src/llmq/quorums.cpp
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/llmq/quorums.cpp
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
Repo: dashpay/dash 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.
Applied to files:
src/llmq/quorums.cpp
📚 Learning: 2025-05-05T12:45:44.781Z
Learnt from: knst
Repo: dashpay/dash PR: 6658
File: src/evo/creditpool.cpp:177-185
Timestamp: 2025-05-05T12:45:44.781Z
Learning: The GetAncestor() function in CBlockIndex safely handles negative heights by returning nullptr rather than asserting, making it safe to call with potentially negative values.
Applied to files:
src/llmq/quorums.cpp
📚 Learning: 2025-07-15T14:53:04.819Z
Learnt from: knst
Repo: dashpay/dash 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.
Applied to files:
src/llmq/quorums.cpp
📚 Learning: 2025-08-19T15:08:00.835Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/commitment.cpp:54-60
Timestamp: 2025-08-19T15:08:00.835Z
Learning: In Dash Core, llmq_params.size == 1 is used only for regtest environments, not on public networks, which significantly reduces the risk profile of code paths specific to single-member quorums.
Applied to files:
src/llmq/quorums.cpp
🧬 Code graph analysis (1)
src/llmq/quorums.cpp (2)
src/net_processing.cpp (8)
LOCK(323-329)LOCK(336-340)LOCK(341-345)UpdatedBlockTip(2125-2160)UpdatedBlockTip(2125-2125)pindexNew(603-603)pindex(607-607)pindex(849-849)src/llmq/utils.cpp (8)
InitQuorumsCache(962-968)InitQuorumsCache(962-962)InitQuorumsCache(969-970)InitQuorumsCache(971-972)InitQuorumsCache(973-978)InitQuorumsCache(979-980)InitQuorumsCache(981-982)InitQuorumsCache(983-984)
🔇 Additional comments (8)
src/llmq/quorums.cpp (8)
28-28: LGTM: Required includes for new functionality.The new includes support the ChainViewSubscriber (validationinterface.h) and utility functions like std::clamp and std::max (algorithm).
Also applies to: 30-30
212-221: LGTM: Well-designed cache sizing heuristic.The formula accounts for the number of cycles and mining window size, with a reasonable buffer (+128) and safety bounds [512, 4096]. This aligns with the goal of reducing cs_main contention while controlling memory usage.
223-227: LGTM: Thread-safe accessor for chain view snapshot.The function correctly locks cs_active_chain_view and returns a copy of the snapshot, ensuring thread-safe access to the active chain view.
264-286: LGTM: Correct chain view synchronization and cache management.The UpdatedBlockTip handler properly distinguishes reorgs from chain extensions (line 279) and clears the ancestor cache only on reorgs, which is the correct optimization. Thread safety is ensured with appropriate locks for both the view snapshot and the cache.
303-325: LGTM: Proper initialization and subscriber registration.The constructor correctly:
- Initializes the ancestor cache with an appropriate size
- Bootstraps the chain view snapshot under cs_main for consistency
- Registers the ValidationInterface subscriber to keep the view synchronized
The lock ordering (cs_main → cs_active_chain_view) is safe and the null check for tip (line 314) is good defensive programming.
330-332: LGTM: Clean subscriber unregistration.The destructor properly unregisters the ValidationInterface subscriber before destruction, with appropriate null checking.
658-668: LGTM: Correct ancestor resolution to avoid cross-fork issues.The code properly uses
pindexStart->GetAncestor()directly to compute ancestors strictly within pindexStart's chain context, preventing cross-fork contamination. The inline comments clearly document this important invariant, and null checks guard against invalid ancestors.
1328-1338: LGTM: Thread-safe quorum selection with proper bounds checking.The function now acquires cs_main upfront and resolves all heights strictly relative to active_chain, preventing inconsistencies during rapid tip changes or rotations. The bounds checking at line 1335 correctly guards against invalid height access.
1 participant
Issue being fixed or feature implemented
based on testing in #6953; we saw significant cs_main contention reduction.
However; cs_main at llmq/quorums.cpp:1214 and llmq/quorums.cpp:526 now became the main (relatively easy to reduce) cs_main contention see below, and are part of the hot loop for islock creation. We should consider trying to reduce: net_processing.cpp:5462 but it seems non-trivial.
What was done?
How Has This Been Tested?
Builds locally; 1 functional test passes.
Breaking Changes
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.