feat: minimal improvements to llmq/quorums #7086
Conversation
`mapQuorumDataRequests` isn't used outside `CQuorumManager` and a `RecursiveMutex` is unnecessary for the usage pattern
|
WalkthroughThe changes refactor quorum data request handling by moving synchronization primitives from global file scope to class members within Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (5)src/**/*.{cpp,h,hpp,cc}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/{masternode,llmq}/**/*.{cpp,h}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/llmq/**/*.{cpp,h}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (10)📓 Common learnings📚 Learning: 2025-11-24T16:41:22.457ZApplied to files:
📚 Learning: 2025-11-24T16:41:22.457ZApplied to files:
📚 Learning: 2025-11-24T16:41:22.457ZApplied to files:
📚 Learning: 2025-10-21T11:09:34.688ZApplied to files:
📚 Learning: 2025-09-09T21:36:11.833ZApplied to files:
📚 Learning: 2025-10-02T18:29:54.756ZApplied to files:
📚 Learning: 2025-11-24T16:41:22.457ZApplied to files:
📚 Learning: 2025-12-22T15:42:48.595ZApplied to files:
📚 Learning: 2025-06-09T16:43:20.996ZApplied to files:
🧬 Code graph analysis (1)src/llmq/quorums.h (1)
🪛 GitHub Actions: Clang Diff Format Checksrc/llmq/quorums.cpp[error] 822-856: Clang format differences found by diff step. Command failed: git diff -U0 origin/develop -- 🔇 Additional comments (7)
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 (1)
src/llmq/quorums.cpp (1)
822-858: Fix clang-format issues flagged by CI.The pipeline reports formatting differences on lines 822-856. Run clang-format on the affected lines to resolve:
clang-format -i src/llmq/quorums.cppOr apply formatting only to the changed lines using
clang-format-diff.py.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/llmq/quorums.cppsrc/llmq/quorums.h
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/llmq/quorums.hsrc/llmq/quorums.cpp
src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Files:
src/llmq/quorums.hsrc/llmq/quorums.cpp
src/{masternode,llmq}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
BLS integration must be used for cryptographic foundation of advanced masternode features
Files:
src/llmq/quorums.hsrc/llmq/quorums.cpp
src/llmq/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
src/llmq/**/*.{cpp,h}: LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Files:
src/llmq/quorums.hsrc/llmq/quorums.cpp
src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
Files:
src/llmq/quorums.hsrc/llmq/quorums.cpp
🧠 Learnings (10)
📓 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: 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: 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.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
Applied to files:
src/llmq/quorums.hsrc/llmq/quorums.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
Applied to files:
src/llmq/quorums.hsrc/llmq/quorums.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
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.hsrc/llmq/quorums.cpp
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
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: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/evodb/**/*.{cpp,h} : Evolution Database (CEvoDb) must handle masternode snapshots, quorum state, governance objects with efficient differential updates for masternode lists
Applied to files:
src/llmq/quorums.hsrc/llmq/quorums.cpp
📚 Learning: 2025-12-22T15:42:48.595Z
Learnt from: kwvg
Repo: dashpay/dash PR: 7068
File: src/qt/guiutil_font.h:68-77
Timestamp: 2025-12-22T15:42:48.595Z
Learning: In C++/Qt codebases, use fail-fast asserts in setters to enforce invariants (e.g., ensuring internal maps contain necessary keys). Prefer assert() for programmer errors that should be caught in development and debugging, rather than defensive runtime checks with fallbacks. This helps catch invariant violations early during development. Apply to header and source files where invariant-driven state mutations occur, especially in setters like SetWeightBold/SetWeightNormal that assume established relationships or preconditions.
Applied to files:
src/llmq/quorums.h
📚 Learning: 2025-06-09T16:43:20.996Z
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.
Applied to files:
src/llmq/quorums.cpp
🧬 Code graph analysis (1)
src/llmq/quorums.h (1)
src/llmq/quorums.cpp (2)
UpdatedBlockTip(301-325)UpdatedBlockTip(301-301)
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/quorums.cpp
[error] 822-856: Clang format differences found by diff step. Command failed: git diff -U0 origin/develop --
🔇 Additional comments (7)
src/llmq/quorums.h (2)
254-256: LGTM: Data request state moved into class scope with proper synchronization.The refactoring moves
mapQuorumDataRequestsfrom file scope intoCQuorumManageras a private member, improving encapsulation. Themutablequalifier is appropriate since the map is modified inconstmethods. The change fromRecursiveMutextoMutexis valid since the map access patterns don't require recursive locking.
300-313: Lock annotations correctly updated for new mutex.The
EXCLUSIVE_LOCKS_REQUIREDannotations are properly extended to include!cs_data_requestsfor all methods that accessmapQuorumDataRequests. This enables compile-time thread safety analysis.src/llmq/quorums.cpp (5)
706-709: LGTM: Direct misbehavior return replaces indirect error handler.The inline
MisbehavingErrorreturn is cleaner than routing through a lambda. The error message clearly describes the validation failure.
714-737: LGTM: Penalty logic correctly differentiated by error type.The
sendQDATAlambda appropriately penalizes request limit violations only when the error is not due to locally missing data (QUORUM_VERIFICATION_VECTOR_MISSING,ENCRYPTED_CONTRIBUTIONS_MISSING). This prevents punishing peers for our own data unavailability.
800-827: LGTM: Error handling correctly distinguishes solicited vs unsolicited failures.The refactored error handling appropriately:
- Penalizes peers for sending unrequested, duplicate, or malformed data (lines 813, 816, 819)
- Logs without penalty when the peer responds to our request but conditions aren't met (line 825-826)
This prevents incorrectly punishing peers for responding to our own data requests with valid error conditions.
873-882: LGTM: Correct penalty for cryptographic validation failures.Penalizing peers for decryption failures (line 875) and invalid secret key shares (line 881) is appropriate since these indicate the peer sent corrupted or malicious cryptographic data rather than a benign error condition.
739-751: Lock ordering is consistent and safe.The
LOCK2(::cs_main, cs_data_requests)pattern at lines 741 and 809 maintains consistent lock ordering (cs_main before cs_data_requests), preventing deadlock scenarios. This follows the established Dash Core locking conventions.
|
|
||
| if (request.GetError() != CQuorumDataRequest::Errors::NONE) { | ||
| return errorHandler(strprintf("Error %d (%s)", request.GetError(), request.GetErrorString()), 0); | ||
| LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- %s: %s, from peer=%d\n", __func__, msg_type, strprintf("Error %d (%s)", request.GetError(), request.GetErrorString()), pfrom.GetId()); |
There was a problem hiding this comment.
nit: inline strprintf
NOT_TESTED but probably it should be:
LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- %s: failed %d(%s), from peer=%d\n", __func__, msg_type, request.GetError(), request.GetErrorString(), pfrom.GetId());
3 participants
Issue being fixed or feature implemented
What was done?
extracted two commits from: #7063
How Has This Been Tested?
Breaking Changes
None
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.