refactor: extract CActiveMasternodeManager from LLMQContext (4/n, followups)
#7066
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
kwvg
force-pushed
the
active_p5
branch
2 times, most recently
from
3ebc918 to
869f847
Compare
|
This pull request has conflicts, please rebase. |
8e44990 chore!: enforce ban for re-propagation of stale `QFCOMMIT`s (Kittywhiskers Van Gogh) Pull request description: ## Additional Information Spun off from [dash#7066](#7066) due to breaking change, enforces policy introduced in [dash#5145](#5145). ## Breaking changes Broadcasting stale `QFCOMMIT`s will result in the offending node getting banned. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 8e44990; since this is protocol version guarded; not a breaking change. UdjinM6: utACK 8e44990 Tree-SHA512: 659e43fba896ac3046dfc7aa197ab0a2207e1648488fdcd545902a90d4efa4f25d74f4a92b99eb2e68cf96b30a78a99114d9487c1abf0b8954dc7a585131d88b
…ontext` (3/n, DKG session isolation, `ActiveContext` consolidation) de4d0ea chore: update lint circular dependencies allowlist (Kittywhiskers Van Gogh) a93f8ae refactor: keep base DKG handler lightweight, follow IWYU (Kittywhiskers Van Gogh) 70e6372 refactor: `InitNewQuorum()` is the only place where real `curSession`s are created (Kittywhiskers Van Gogh) d115e0e refactor: consolidate masternode mode logic to `src/active/` (Kittywhiskers Van Gogh) 19b2ee5 refactor: merge `ActiveNotificationInterface` into `ActiveContext` (Kittywhiskers Van Gogh) f0468e6 refactor: merge `CActiveMasternodeInfo` into `CActiveMasternodeManager` (Kittywhiskers Van Gogh) e9f8294 refactor: move `CActiveMasternodeManager` to `ActiveContext` (Kittywhiskers Van Gogh) 004b3b7 refactor: drop `CActiveMasternodeManager` from chainstate init (Kittywhiskers Van Gogh) 01c0b94 refactor: separate active session routines into dedicated class (Kittywhiskers Van Gogh) bb90306 move-only: move DKG session participant logic to `llmq/active` (Kittywhiskers Van Gogh) b59baac refactor: separate active session handler routines into dedicated class (Kittywhiskers Van Gogh) 1bdf836 move-only: move DKG session handler participant logic to `llmq/active` (Kittywhiskers Van Gogh) 831dbfe refactor: move DKG session handler init out of ctor (Kittywhiskers Van Gogh) 394c66b refactor: use `unique_ptr` in DKG session handler map (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #7063 * Dependency for #7066 * As `ActiveNotificationInterface` exclusively interacts with `ActiveContext` members, we can squash them together like `ObserverContext`. ## Breaking Changes None expected. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK de4d0ea UdjinM6: utACK de4d0ea Tree-SHA512: 79d92003b672cd9309fe241610544ebd32caecd2984a60e31673e11a922d1e4e3cc4a189b2345ce71e9a9793d4364a07ac5f5b49d40bcf0aa0d3b379b8a6d8a5
|
This pull request has conflicts, please rebase. |
Resolves a TODO
Most usage is not ordering-sensitive, can reduce lookup complexity.
Also: - Don't start DKG logger if we expect to bail right after in ActiveSession
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR refactors LLMQ (Long-Living Masternode Quorum) infrastructure across multiple components. Key changes include: replacing std::set containers with Uint256HashSet and std::unordered_set for performance optimization; restructuring CDKGDebugManager to use dependency injection (dmnman, qsnapman, chainman) instead of default construction; introducing a ReceiveMessagePreamble template method to centralize message handling logic for Contribution, Complaint, and Justification message types; consolidating CQuorumManager mutexes (cs_map_quorums and cs_scan_quorums into m_cs_maps); converting CDKGDebugStatus from a class to a struct with simplified interfaces; adding thread-safe lazy initialization for qwatch connection seed; and updating RPC parameter parsing from ParseInt64V/ParseInt32V to direct UniValue.getInt<>() calls. The changes span protocol handling, quorum management, RPC interfaces, and data structure containers. Sequence Diagram(s)sequenceDiagram
participant Client as CDKGSession
participant Preamble as ReceiveMessagePreamble<br/>(Template)
participant Container as Message Container<br/>(Phase Map)
participant Debug as CDKGLogger
participant Processor as Per-Type Handler<br/>(VerifyAndJustify, etc.)
Client->>Preamble: ReceiveMessagePreamble<CDKGContribution>(msg, phase)
Preamble->>Container: Select container by phase
Preamble->>Container: Insert message into phase map
Preamble->>Debug: Update debug status
Preamble->>Preamble: Check membership & accumulate hashes
Preamble-->>Client: Return ReceiveMessageState (member, hash, inv, should_process)
alt should_process == true
Client->>Processor: Call VerifyAndJustify or equivalent
Processor-->>Client: Process result
else should_process == false
Note over Client: Skip full processing
end
Note over Preamble: Relay logic executes before<br/>full verification regardless
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/llmq/signing.cpp (1)
182-189: HardenwriteTimedeserialization to avoid crashes on corrupted/unexpected DB data (keep happy-path behavior unchanged).
db->ReadDataStream(k2, writeTimeDs)succeeding doesn’t guarantee the stream contains at least 4 bytes (or is in the expected format).writeTimeDs >> writeTime;without a size/exception guard can crash cleanup/removal on DB corruption.Proposed fix (minimal, behavior-preserving for valid entries)
if (deleteTimeKey) { CDataStream writeTimeDs(SER_DISK, CLIENT_VERSION); if (db->ReadDataStream(k2, writeTimeDs)) { - uint32_t writeTime; - writeTimeDs >> writeTime; - auto k5 = std::make_tuple(std::string("rs_t"), (uint32_t) htobe32_internal(writeTime), recSig.getLlmqType(), recSig.getId()); - batch.Erase(k5); + // Defensive: DB value may be corrupted or from an unexpected legacy format + if (writeTimeDs.size() >= sizeof(uint32_t)) { + try { + uint32_t writeTime; + writeTimeDs >> writeTime; + auto k5 = std::make_tuple(std::string("rs_t"), (uint32_t)htobe32_internal(writeTime), recSig.getLlmqType(), recSig.getId()); + batch.Erase(k5); + } catch (const std::exception&) { + // ignore; leave rs_t cleanup to CleanupOldRecoveredSigs() + } + } } }src/llmq/dkgsession.cpp (1)
1-1: Fix clang-format issues before merging.The CI pipeline reports clang-format differences. Run
clang-format-diff.pyto fix formatting in this file.
🤖 Fix all issues with AI agents
In @src/llmq/utils.h:
- Around line 65-67: The header declares std::unordered_set in the public API
(CalcDeterministicWatchConnections) but does not include <unordered_set>; add an
explicit #include <unordered_set> to the top of the header (next to the other
standard includes) so the declaration does not rely on transitive includes and
consumers can compile independently.
🧹 Nitpick comments (4)
src/llmq/ehf_signals.h (1)
8-13: Include addition is fine; consider dropping<set>if it’s now unused in this header.src/llmq/utils.cpp (1)
761-768: Thread-safe lazy initialization for qwatch connection seed is functionally correct but could be improved.The current pattern checks the atomic flag outside the lock but doesn't re-check inside. Multiple threads could sequentially generate seeds. While this is acceptable for random seed generation (any random value is valid), a cleaner double-checked locking pattern would be:
if (!qwatchConnectionSeedGenerated) { LOCK(qwatchConnectionSeedCs); if (!qwatchConnectionSeedGenerated) { // Re-check after acquiring lock qwatchConnectionSeed = GetRandHash(); qwatchConnectionSeedGenerated = true; } }This is a minor improvement and the current code is functionally correct for its purpose. Based on learnings, this refinement could be deferred to a follow-up.
src/llmq/dkgsession.cpp (2)
620-636: Consider adding a defensivedefaultcase inside the switch.The switch statement covers all current
MsgPhasevalues, and theassert(false)after the lambda provides a runtime check. However, for improved maintainability and compiler diagnostics, consider either:
- Adding
default: assert(false);inside the switch, or- Using
std::unreachable()(C++23) or compiler-specific__builtin_unreachable()to help the compiler with control flow analysisThis would allow compilers to warn if a new enum value is added but not handled.
♻️ Optional improvement
auto& member_set = [&]() -> Uint256HashSet& { switch (phase) { case MsgPhase::Contribution: inv_type = MSG_QUORUM_CONTRIB; msg_name = "contribution"; return member->contributions; case MsgPhase::Complaint: inv_type = MSG_QUORUM_COMPLAINT; msg_name = "complaint"; return member->complaints; case MsgPhase::Justification: inv_type = MSG_QUORUM_JUSTIFICATION; msg_name = "justification"; return member->justifications; + default: + assert(false); } - assert(false); }();
640-671: Logic flow is correct but comment could be clearer.The two size checks serve different purposes:
- Line 640:
size >= 2- Rejects the 3rd+ message entirely (before emplacing)- Line 665:
size > 1- After emplacing, if size is now >1, marks member as bad but still relaysThis correctly allows up to 2 messages to be relayed while marking the sender as bad after the 2nd. The comment on line 641 mentions "justifications" but this template handles all three message types. Consider updating the comment to be generic.
📝 Minor comment fix
if (member_set.size() >= 2) { - // only relay up to 2 justifications, that's enough to let the other members know about his bad behavior + // only relay up to 2 messages per member, that's enough to let the other members know about their bad behavior return std::nullopt; }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
src/active/context.cppsrc/active/dkgsession.cppsrc/active/dkgsession.hsrc/active/dkgsessionhandler.cppsrc/llmq/core_write.cppsrc/llmq/debug.cppsrc/llmq/debug.hsrc/llmq/dkgsession.cppsrc/llmq/dkgsession.hsrc/llmq/dkgsessionhandler.hsrc/llmq/ehf_signals.hsrc/llmq/observer/context.cppsrc/llmq/observer/quorums.cppsrc/llmq/observer/quorums.hsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.hsrc/llmq/signing.cppsrc/llmq/utils.cppsrc/llmq/utils.hsrc/net.cppsrc/net.hsrc/rpc/client.cppsrc/rpc/evo.cppsrc/rpc/governance.cppsrc/rpc/masternode.cppsrc/rpc/quorums.cppsrc/rpc/util.cppsrc/rpc/util.hsrc/test/rpc_tests.cpptest/functional/feature_llmq_dkgerrors.py
💤 Files with no reviewable changes (2)
- src/rpc/util.cpp
- src/rpc/util.h
🧰 Additional context used
📓 Path-based instructions (7)
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/dkgsessionhandler.hsrc/rpc/masternode.cppsrc/net.cppsrc/rpc/evo.cppsrc/active/dkgsession.hsrc/llmq/quorums.cppsrc/llmq/observer/quorums.cppsrc/net.hsrc/llmq/quorumsman.cppsrc/rpc/client.cppsrc/llmq/observer/quorums.hsrc/llmq/signing.cppsrc/llmq/quorumsman.hsrc/llmq/dkgsession.hsrc/rpc/governance.cppsrc/llmq/dkgsession.cppsrc/llmq/quorums.hsrc/active/dkgsession.cppsrc/llmq/observer/context.cppsrc/llmq/utils.cppsrc/llmq/utils.hsrc/active/context.cppsrc/test/rpc_tests.cppsrc/llmq/ehf_signals.hsrc/rpc/quorums.cppsrc/llmq/debug.hsrc/llmq/debug.cppsrc/active/dkgsessionhandler.cppsrc/llmq/core_write.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/dkgsessionhandler.hsrc/llmq/quorums.cppsrc/llmq/observer/quorums.cppsrc/llmq/quorumsman.cppsrc/llmq/observer/quorums.hsrc/llmq/signing.cppsrc/llmq/quorumsman.hsrc/llmq/dkgsession.hsrc/llmq/dkgsession.cppsrc/llmq/quorums.hsrc/llmq/observer/context.cppsrc/llmq/utils.cppsrc/llmq/utils.hsrc/llmq/ehf_signals.hsrc/llmq/debug.hsrc/llmq/debug.cppsrc/llmq/core_write.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/dkgsessionhandler.hsrc/llmq/quorums.cppsrc/llmq/observer/quorums.cppsrc/llmq/quorumsman.cppsrc/llmq/observer/quorums.hsrc/llmq/signing.cppsrc/llmq/quorumsman.hsrc/llmq/dkgsession.hsrc/llmq/dkgsession.cppsrc/llmq/quorums.hsrc/llmq/observer/context.cppsrc/llmq/utils.cppsrc/llmq/utils.hsrc/llmq/ehf_signals.hsrc/llmq/debug.hsrc/llmq/debug.cppsrc/llmq/core_write.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/dkgsessionhandler.hsrc/llmq/quorums.cppsrc/llmq/observer/quorums.cppsrc/llmq/quorumsman.cppsrc/llmq/observer/quorums.hsrc/llmq/signing.cppsrc/llmq/quorumsman.hsrc/llmq/dkgsession.hsrc/llmq/dkgsession.cppsrc/llmq/quorums.hsrc/llmq/observer/context.cppsrc/llmq/utils.cppsrc/llmq/utils.hsrc/llmq/ehf_signals.hsrc/llmq/debug.hsrc/llmq/debug.cppsrc/llmq/core_write.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/dkgsessionhandler.hsrc/llmq/quorums.cppsrc/llmq/observer/quorums.cppsrc/llmq/quorumsman.cppsrc/llmq/observer/quorums.hsrc/llmq/signing.cppsrc/llmq/quorumsman.hsrc/llmq/dkgsession.hsrc/llmq/dkgsession.cppsrc/llmq/quorums.hsrc/llmq/observer/context.cppsrc/llmq/utils.cppsrc/llmq/utils.hsrc/llmq/ehf_signals.hsrc/llmq/debug.hsrc/llmq/debug.cppsrc/llmq/core_write.cpp
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests in test/functional/ must be written in Python (minimum version specified in .python-version) and depend on dashd and dash-node
Files:
test/functional/feature_llmq_dkgerrors.py
src/{test,wallet/test}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Files:
src/test/rpc_tests.cpp
🧠 Learnings (31)
📓 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.
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/wallet/wallet.cpp:0-0
Timestamp: 2025-07-29T14:33:01.040Z
Learning: In refactoring PRs like #6761, kwvg acknowledges code safety improvements (like null pointer checks and unused parameter warnings) but prefers to defer them to follow-up PRs to maintain focus on the primary refactoring objectives, avoiding scope creep.
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: kwvg
Repo: dashpay/dash PR: 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: 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)
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
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.
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
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
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.
📚 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/dkgsessionhandler.hsrc/active/dkgsession.hsrc/net.hsrc/llmq/observer/quorums.hsrc/llmq/quorumsman.hsrc/llmq/dkgsession.hsrc/llmq/quorums.hsrc/llmq/utils.hsrc/llmq/ehf_signals.hsrc/llmq/debug.h
📚 Learning: 2025-10-03T11:29:36.358Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6838
File: src/governance/signing.cpp:84-85
Timestamp: 2025-10-03T11:29:36.358Z
Learning: After bitcoin#25153 (included in dash#6775), UniValue uses the templated getInt<T>() API instead of the legacy get_int() and get_int64() methods. For example, use jproposal["start_epoch"].getInt<int64_t>() to retrieve int64_t values from UniValue objects.
Applied to files:
src/rpc/masternode.cppsrc/rpc/evo.cppsrc/rpc/governance.cpp
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/rpc/evo.cppsrc/rpc/client.cppsrc/test/rpc_tests.cppsrc/rpc/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/rpc/evo.cppsrc/llmq/observer/quorums.cppsrc/llmq/quorumsman.cppsrc/llmq/dkgsession.h
📚 Learning: 2025-12-02T12:59:28.247Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 7026
File: src/rpc/evo.cpp:1713-1799
Timestamp: 2025-12-02T12:59:28.247Z
Learning: In the Dash codebase, CChain::operator[] has built-in safe bounds checking. Accessing chainman.ActiveChain()[height] is safe even if height exceeds the current chain height, as the operator will handle bounds checking internally. There is no need to manually check chain height before using the [] operator on CChain.
Applied to files:
src/rpc/evo.cppsrc/rpc/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} : 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.cppsrc/llmq/observer/quorums.cppsrc/net.hsrc/llmq/quorumsman.cppsrc/llmq/observer/quorums.hsrc/llmq/quorumsman.hsrc/llmq/dkgsession.hsrc/llmq/dkgsession.cppsrc/llmq/quorums.hsrc/llmq/utils.cppsrc/llmq/utils.hsrc/rpc/quorums.cppsrc/llmq/debug.h
📚 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.cppsrc/llmq/observer/quorums.cppsrc/net.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.hsrc/llmq/dkgsession.hsrc/llmq/dkgsession.cppsrc/llmq/utils.cppsrc/llmq/utils.hsrc/llmq/ehf_signals.hsrc/rpc/quorums.cppsrc/llmq/debug.h
📚 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/{masternode,llmq}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features
Applied to files:
src/net.hsrc/llmq/quorumsman.hsrc/llmq/dkgsession.hsrc/llmq/ehf_signals.h
📚 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/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
Applied to files:
src/net.h
📚 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/{validation,consensus,net_processing}/**/*.{cpp,h} : ValidationInterface callbacks must be used for event-driven architecture to coordinate subsystems during block/transaction processing
Applied to files:
src/net.h
📚 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/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
src/net.hsrc/llmq/dkgsession.hsrc/active/dkgsession.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/net.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.hsrc/llmq/dkgsession.hsrc/llmq/dkgsession.cppsrc/llmq/utils.cppsrc/llmq/utils.hsrc/llmq/debug.h
📚 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/net.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.h
📚 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/net.hsrc/llmq/quorumsman.h
📚 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.
Applied to files:
src/net.h
📚 Learning: 2025-09-09T21:36:58.969Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:223-232
Timestamp: 2025-09-09T21:36:58.969Z
Learning: In RawSender class (src/stats/rawsender.cpp), socket operations are properly synchronized using the cs_net mutex. The m_sock and m_server members are GUARDED_BY(cs_net), and methods like Connect(), SendDirectly(), and ReconnectThread() use appropriate locking with cs_net to prevent race conditions on socket access.
Applied to files:
src/net.h
📚 Learning: 2025-09-09T21:36:31.504Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:31.504Z
Learning: In RawSender class (src/stats/rawsender.cpp), m_sock access is protected by the cs_net mutex. This mutex coordinates between the TCP send path in SendDirectly() and the reconnection operations in Connect()/Reconnect() methods, ensuring proper synchronization of socket state.
Applied to files:
src/net.h
📚 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/{masternode,evo}/**/*.{cpp,h} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/net.hsrc/llmq/dkgsession.h
📚 Learning: 2025-09-09T21:36:31.504Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:31.504Z
Learning: In RawSender class (src/stats/rawsender.cpp), m_sock access requires synchronization using the cs_net mutex. The race condition exists between SendDirectly() (callable from any thread) and Connect() (called by ReconnectThread), where both read and write m_sock concurrently. The cs_net mutex properly coordinates these network operations.
Applied to files:
src/net.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/quorumsman.cppsrc/llmq/quorumsman.hsrc/llmq/dkgsession.cppsrc/llmq/observer/context.cppsrc/llmq/utils.cppsrc/active/context.cppsrc/rpc/quorums.cppsrc/llmq/debug.h
📚 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/quorumsman.cppsrc/llmq/observer/context.cpp
📚 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:
test/functional/feature_llmq_dkgerrors.pysrc/rpc/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/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h} : Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
Applied to files:
src/llmq/quorumsman.hsrc/llmq/dkgsession.hsrc/llmq/utils.h
📚 Learning: 2025-11-13T20:02:55.480Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6969
File: src/evo/deterministicmns.h:441-479
Timestamp: 2025-11-13T20:02:55.480Z
Learning: In `src/evo/deterministicmns.h`, the `internalId` field in `CDeterministicMN` and the `mnInternalIdMap` in `CDeterministicMNList` are non-deterministic and used only for internal bookkeeping and efficient lookups. Different nodes can assign different internalIds to the same masternode depending on their sync history. Methods like `IsEqual()` intentionally ignore internalId mappings and only compare consensus-critical deterministic fields (proTxHash, collateral, state, etc.).
Applied to files:
src/llmq/dkgsession.hsrc/llmq/utils.cppsrc/llmq/utils.hsrc/llmq/debug.h
📚 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/utils.cppsrc/llmq/utils.hsrc/rpc/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: In src/net_processing.cpp, if ActiveContext (m_active_ctx) is non-null, its members (including cj_server) are guaranteed to be initialized; call sites can safely dereference m_active_ctx->cj_server without an additional null-check.
Applied to files:
src/active/context.cpp
📚 Learning: 2025-10-03T11:20:37.475Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6838
File: src/active/context.cpp:29-33
Timestamp: 2025-10-03T11:20:37.475Z
Learning: In Dash codebase, NodeContext (src/node/context.h) serves only as a container with trivial c/d-tors; member lifetime is explicitly managed via reset() calls in the shutdown sequence (src/init.cpp), not by declaration order. For example, active_ctx.reset() is called before DashChainstateSetupClose handles llmq_ctx teardown, ensuring proper destruction order regardless of declaration order in the struct.
Applied to files:
src/active/context.cpp
📚 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/ehf_signals.h
📚 Learning: 2025-11-04T18:24:27.241Z
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.
Applied to files:
src/rpc/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/rpc/quorums.cpp
🧬 Code graph analysis (14)
src/rpc/evo.cpp (1)
src/evo/providertx.h (1)
nReason(258-300)
src/net.h (1)
src/net.cpp (2)
AddPendingProbeConnections(4458-4462)AddPendingProbeConnections(4458-4458)
test/functional/feature_llmq_dkgerrors.py (1)
test/functional/test_framework/test_framework.py (2)
get_node(1223-1228)mine_quorum(2108-2196)
src/llmq/observer/quorums.h (1)
src/llmq/quorumsman.cpp (2)
CleanupOldQuorumData(362-366)CleanupOldQuorumData(362-362)
src/llmq/quorumsman.h (1)
src/llmq/quorumsman.cpp (10)
ScanQuorums(193-197)ScanQuorums(193-193)ScanQuorums(199-307)ScanQuorums(199-201)CleanupOldQuorumData(362-366)CleanupOldQuorumData(362-362)GetQuorum(368-389)GetQuorum(368-368)GetQuorum(391-407)GetQuorum(391-391)
src/llmq/dkgsession.h (2)
src/llmq/dkgsession.cpp (5)
ReceiveMessagePreamble(612-675)ReceiveMessagePreamble(612-612)ReceiveMessagePreamble(677-677)ReceiveMessagePreamble(678-678)ReceiveMessagePreamble(679-679)src/active/dkgsessionhandler.cpp (1)
logger(336-336)
src/rpc/governance.cpp (4)
src/qt/rpcconsole.cpp (2)
request(421-475)request(421-421)src/rpc/request.cpp (1)
request(31-31)src/rpc/server.cpp (1)
request(552-552)src/governance/vote.h (1)
nTime(69-69)
src/llmq/dkgsession.cpp (2)
src/llmq/debug.h (1)
phase(60-60)src/llmq/commitment.h (1)
quorumIndex(56-56)
src/llmq/quorums.h (1)
src/llmq/quorums.cpp (2)
DataCleanupHelper(29-74)DataCleanupHelper(29-29)
src/active/dkgsession.cpp (1)
src/active/dkgsessionhandler.cpp (1)
logger(336-336)
src/llmq/utils.h (2)
src/llmq/utils.cpp (2)
CalcDeterministicWatchConnections(757-777)CalcDeterministicWatchConnections(757-759)src/llmq/params.h (1)
LLMQType(14-125)
src/test/rpc_tests.cpp (5)
src/rpc/quorums.cpp (1)
result(475-475)src/rpc/mining.cpp (1)
result(872-872)src/rpc/blockchain.cpp (3)
result(139-139)result(277-277)result(564-564)src/bitcoin-cli.cpp (4)
result(329-329)result(340-340)result(474-474)result(715-715)src/rpc/node.cpp (7)
result(429-429)result(503-503)result(590-590)result(678-678)result(743-743)result(1015-1015)result(1094-1094)
src/llmq/debug.h (2)
src/llmq/debug.cpp (4)
CDKGDebugManager(110-116)CDKGDebugManager(118-118)GetSessionCount(120-123)GetSessionCount(120-120)src/llmq/core_write.cpp (14)
GetJsonHelp(20-78)GetJsonHelp(20-20)GetJsonHelp(81-95)GetJsonHelp(81-81)GetJsonHelp(97-114)GetJsonHelp(97-97)GetJsonHelp(134-142)GetJsonHelp(134-134)GetJsonHelp(153-178)GetJsonHelp(153-153)GetJsonHelp(222-234)GetJsonHelp(222-222)GetJsonHelp(255-266)GetJsonHelp(255-255)
src/llmq/debug.cpp (1)
src/llmq/debug.h (1)
CDKGDebugManager(91-124)
🪛 Cppcheck (2.19.0)
src/llmq/core_write.cpp
[error] 80-80: Uninitialized variable
(legacyUninitvar)
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/dkgsession.h
[error] 1-1: Clang format differences detected. Run clang-format-diff.py to fix formatting in this file.
src/llmq/dkgsession.cpp
[error] 1-1: Clang format differences detected. Run clang-format-diff.py to fix formatting in this file.
src/rpc/quorums.cpp
[error] 1-1: Clang format differences detected. Run clang-format-diff.py to fix formatting in this file.
src/llmq/debug.h
[error] 1-1: Clang format differences detected. Run clang-format-diff.py to fix formatting in this file.
src/active/dkgsessionhandler.cpp
[error] 1-1: Clang format differences detected. Run clang-format-diff.py to fix formatting in this file.
🔇 Additional comments (59)
src/rpc/client.cpp (2)
258-258: LGTM: Clear documentation of index offset.The comment accurately explains that compound RPC parameter indices are offset by one to account for the subcommand occupying position 0 in the parameter array.
259-313: All parameter indices in the RPC conversion entries are correct and match the actual RPC method signatures. No changes needed.src/rpc/governance.cpp (4)
179-180: LGTM - Consistent use of getInt() API.The migration from
ParseInt32V/ParseInt64VtogetInt<int>()andgetInt<int64_t>()aligns with the UniValue templated API introduced in bitcoin#25153 (dash#6775). Based on learnings.
228-228: LGTM - Type consistency verified.The
collateralIndexis correctly parsed asintand subsequently cast touint32_ton line 232, matching the existing pattern.
281-281: LGTM - Proper handling of optional parameter.The ternary correctly handles the empty params case with a default of 10, and the negative check on line 282 provides appropriate validation.
364-365: LGTM - Consistent with gobject_prepare parsing.The
nRevisionandnTimeparsing matches the pattern used ingobject_prepare(lines 179-180), maintaining consistency across related RPC handlers.src/rpc/evo.cpp (4)
716-716: LGTM - Proper negative value validation before cast.The pattern of parsing as
int, validatingcollateralIndex < 0, then casting touint32_ton line 721 is correct and safe.
1269-1274: LGTM - Proper bounds validation for reason code.The
nReasonis correctly validated against both lower bound (< 0) and upper bound (CProUpRevTx::REASON_LAST) before casting touint16_t. This matches theuint16_t nReasonfield inCProUpRevTxas shown in the relevant code snippets fromsrc/evo/providertx.h.
1455-1455: LGTM - Consistent height parsing with validation.The height is properly validated on line 1456 against chain bounds before use.
1488-1488: LGTM - Consistent pattern with wallet list variant.This matches the same parsing and validation pattern used on lines 1455-1458 for the wallet case, maintaining consistency within the function.
src/net.h (3)
36-38: LGTM - Required includes for type migration.The
saltedhasher.hinclude provides theUint256HashSettype definition needed for the container type changes below.
1469-1469: LGTM - Consistent container type migration.The parameter type change from
std::set<uint256>toUint256HashSetaligns with the member variable change on line 1786 and the implementation innet.cppthat uses range insert.
1786-1786: LGTM - Performance optimization for pending probes.Changing from
std::set<uint256>toUint256HashSetprovides O(1) average-case lookup/insert complexity instead of O(log n). Since probe connections don't require ordering, an unordered set is appropriate here. This aligns with the broader PR migration pattern across DKG and quorum-related components.src/active/context.cpp (1)
38-41: DI construction forCDKGDebugManagerlooks consistent with the refactor.No concerns with the new constructor wiring; it also keeps
dkgdbgmaninitialized beforeqdkgsman/handlers that may use it.src/llmq/ehf_signals.h (1)
32-37:Uint256HashSetforidslooks appropriate for the described “filter” usage.Assuming callers don’t require ordered iteration, this is a good perf-oriented refactor.
src/test/rpc_tests.cpp (1)
604-616: Test expectation update togetInt<int>()is aligned with typed RPC conversion.src/net.cpp (1)
4458-4462: Self-aliasing risk is theoretical but extremely unlikely; iterator invalidation concern is incorrect.The function is called only once in the codebase (src/llmq/utils.cpp:873) with a locally-constructed
Uint256HashSet probeConnections, making self-aliasing withmasternodePendingProbesimpossible in practice.More importantly, the iterator invalidation risk described is not present: the code iterates over the source range (
proTxHashes) while inserting into the destination (masternodePendingProbes). Rehashing of the destination set does not invalidate iterators to the source, so there is no UB here. Iterator invalidation would only occur if you iterated over the same unordered_set being rehashed during insertion.The proposed hardening with
reserve()and the self-aliasing check are defensive in the wrong direction—if self-aliasing did occur, you would still need to copy the source before inserting to avoid modifying the container mid-iteration. Given the single, fixed call site with a local variable, this mitigation adds complexity without addressing a realistic threat.src/llmq/quorums.h (1)
38-38: LGTM! Performance optimization with hash-based container.The migration from
std::set<uint256>toUint256HashSetis well-suited for the skip list use case, providing O(1) membership checks instead of O(log n). The implementation insrc/llmq/quorums.cppconfirms this is used exclusively for lookups without requiring ordering.src/rpc/masternode.cpp (1)
385-385: LGTM! Type-safe parameter extraction.The migration to
getInt<int64_t>()provides type-safe extraction with proper null-check handling via the ternary operator. This aligns with the modern UniValue API introduced in bitcoin#25153.src/llmq/observer/quorums.cpp (1)
337-363: LGTM! Consistent container migration for performance.The migration to
Uint256HashSetfor bothdbKeysToSkipandquorum_keysis appropriate. The containers are used exclusively for membership tracking and merging, where hash-based operations provide O(1) performance vs O(log n) for ordered sets. Themerge()operation is fully supported in C++17+ forstd::unordered_set.src/llmq/observer/context.cpp (1)
22-22: LGTM! Dependency injection for improved testability.The change from default construction to parameterized construction
CDKGDebugManager(dmnman, qsnapman, chainman)makes dependencies explicit and improves testability. The required references are already available from the constructor parameter list.src/llmq/observer/quorums.h (1)
62-62: LGTM! Interface updated for container migration.The pure virtual method signature correctly updated to match the
Uint256HashSetmigration. The implementation insrc/llmq/quorumsman.cpp:361confirms the interface change is consistently applied across all implementations.test/functional/feature_llmq_dkgerrors.py (1)
40-40: LGTM! Numeric argument type alignment.The conversion from string literals to integer literals for quorum command numeric arguments is consistent across all test cases and aligns with the RPC parameter parsing refactor (switching from ParseInt32V/ParseInt64V to direct
.getInt<>()calls).Also applies to: 45-46, 51-51, 59-60, 65-67, 72-73, 78-79
src/llmq/dkgsessionhandler.h (1)
70-70: LGTM! Performance optimization via hash-based container.Replacing
std::set<uint256>withUint256HashSetprovides O(1) lookup instead of O(log n) for message deduplication, which is appropriate since message ordering is not required for tracking seen messages.src/llmq/quorums.cpp (1)
29-29: LGTM! Container type migration for DataCleanupHelper.The parameter type change to
Uint256HashSetis compatible with the existing usage at line 50 (skip_list.find(...)) and aligns with the broader performance optimization effort.src/llmq/quorumsman.cpp (2)
85-85: LGTM! Mutex consolidation simplifies quorum cache locking.The consolidation of
cs_map_quorumsandcs_scan_quorumsinto a singlem_cs_mapsmutex simplifies the locking logic and ensures consistent protection for bothmapQuorumsCacheandscanQuorumsCache.Note: This consolidation may increase lock contention if map and scan operations were previously parallelizable, but it eliminates potential lock-ordering issues and reduces complexity.
Also applies to: 109-109, 233-233, 296-296, 402-402, 533-533
362-362: LGTM! Parameter type change aligns with container migration.The signature change to accept
const Uint256HashSet&instead ofconst std::set<uint256>&is consistent with the broader performance optimization effort across LLMQ code.src/active/dkgsession.h (1)
46-46: LGTM! Consistent parameter type migration.The
SendJustificationparameter type change fromconst std::set<uint256>&toconst Uint256HashSet&aligns with the broader container migration for performance optimization. All call-sites have been properly updated to passUint256HashSet, as confirmed by the implementation atsrc/active/dkgsession.cpp:307-336where thejustifyForvariable is correctly declared asUint256HashSetand passed to the method.src/active/dkgsession.cpp (3)
46-47: LGTM: Logger initialization moved after membership check.The logger is now created after the
AreWeMember()check and threshold assertion, avoiding unnecessary construction when the early return paths are taken.
307-307: LGTM: Container type update to Uint256HashSet.The change from
std::set<uint256>toUint256HashSetimproves lookup performance for member justification tracking. The usage withemplace()andcount()remains semantically equivalent.
340-341: LGTM: SendJustification signature updated consistently.The parameter type change to
const Uint256HashSet&aligns with the caller inVerifyAndJustifyand the header declaration.src/active/dkgsessionhandler.cpp (2)
253-260: LGTM: Container type changes for batch signature verification.The function signature and internal containers are updated from ordered to unordered sets. This is appropriate since ordering is not required for tracking bad nodes or message hashes, and hash-based containers provide better average-case lookup performance.
266-266: LGTM: messageHashesSet uses Uint256HashSet.Consistent with the PR-wide migration to hash-based containers for uint256 collections.
src/llmq/dkgsession.h (4)
9-9: LGTM: Added protocol.h include.Required for
CInvtype used in the newReceiveMessageStatestruct.
218-224: LGTM: CDKGMember container types updated to Uint256HashSet.All member tracking containers (
contributions,complaints,justifications,prematureCommitments,badMemberVotes,complaintsFromOthers) now use hash-based sets. This aligns with the coding guidelines for using efficient containers in LLMQ data structures.
284-296: LGTM: Internal helper types for message reception.The
MsgPhaseenum andReceiveMessageStatestruct support the newReceiveMessagePreambletemplate, centralizing common message handling logic and reducing code duplication acrossReceiveMessageoverloads.
417-420: LGTM: ReceiveMessagePreamble template declaration.The template with
EXCLUSIVE_LOCKS_REQUIRED(invCs)annotation correctly documents the locking requirements for the centralized message preamble processing.src/llmq/core_write.cpp (1)
80-81: LGTM: GetJsonHelp moved to CDKGDebugManager.The function scope change from
CDKGDebugStatustoCDKGDebugManageraligns with the refactor consolidating debug status reporting. The addedinner_optionalparameter provides flexibility for optional inner field documentation.Note: The Cppcheck "uninitialized variable" warning at line 80 appears to be a false positive—this is a static method that returns a fully constructed
RPCResultobject.src/llmq/utils.cpp (2)
757-770: LGTM: Return type and container updated to unordered_set.The function returns connection indices where ordering is not required, making
std::unordered_set<size_t>appropriate.
845-845: LGTM: probeConnections uses Uint256HashSet.Consistent with PR-wide container type migration.
src/rpc/quorums.cpp (5)
72-72: LGTM: Direct UniValue parameter parsing.Using
request.params[0].getInt<int>()instead ofParseInt32V()is cleaner and leverages UniValue's built-in type conversion with proper error handling.
277-277: LGTM: llmqType parsing updated consistently.The pattern
static_cast<Consensus::LLMQType>(request.params[0].getInt<int>())is used consistently across all RPC methods that accept llmqType.
300-300: LGTM: Updated to CDKGDebugManager::GetJsonHelp.Reflects the API consolidation where JSON help generation moved to the debug manager.
349-358: LGTM: Debug status JSON generation refactored.The refactor from constructing
CDKGDebugStatusseparately to usingdebugman->ToJson(detailLevel)simplifies the code and centralizes the JSON generation logic in the debug manager.
1006-1006: LGTM: Using GetSessionCount() accessor.The new accessor method properly encapsulates the session count retrieval with appropriate locking.
src/llmq/debug.h (4)
48-48: LGTM: complaintsFromMembers uses std::unordered_set.The container change from
std::set<uint16_t>tostd::unordered_set<uint16_t>is appropriate since member indices don't require ordering for complaint tracking.
86-89: LGTM: CDKGDebugStatus simplified to struct.Changing from class to struct with only data members is appropriate since all methods were moved to
CDKGDebugManager. The simpler data structure clearly conveys its role as a plain data container.
93-106: LGTM: CDKGDebugManager constructor with dependency injection.The refactor introduces proper dependency injection, storing references to
CDeterministicMNManager,CQuorumSnapshotManager, andChainstateManager. This aligns with the broader refactoring goal of extracting components fromLLMQContextand follows the project's initialization patterns seen in other files.
119-123: LGTM: New accessor methods with proper lock annotations.
GetSessionCount()provides encapsulated access to session countGetJsonHelp()is appropriately static since it doesn't depend on instance stateToJson()has the correctEXCLUSIVE_LOCKS_REQUIRED(!cs_lockStatus)annotation indicating it acquires the lock internallyThe API is clean and well-documented.
src/llmq/quorumsman.h (3)
87-91: Mutex consolidation looks good.Consolidating
cs_map_quorumsandcs_scan_quorumsinto a singlem_cs_mapsmutex simplifies the locking strategy. Looking at the relevant snippets fromquorumsman.cpp, the ScanQuorums method accesses both caches within the same logical operation (lines 231-262 and 296-302), so having a single mutex is appropriate and reduces the risk of lock ordering issues.
161-161: Signature change toUint256HashSetaligns with guidelines.Switching from
std::set<uint256>toUint256HashSetprovides O(1) lookup complexity, which is appropriate for membership checks during cleanup operations. As per coding guidelines, this follows the preference for unordered containers in Dash-specific data structures.
15-15: Include addition is necessary.The
saltedhasher.hinclude is required forUint256HashSettype used in theCleanupOldQuorumDatasignature.src/llmq/dkgsession.cpp (3)
206-221: Clean extraction of common logic to preamble.The refactoring properly delegates pre-processing (hash computation, relay decision, status updates) to
ReceiveMessagePreamble, and the contribution-specific logic (vvec storage, decryption, verification) remains inReceiveMessage. The timert2is correctly placed to measure only the contribution-specific processing.
361-361: Good performance improvement.Switching from
std::set<size_t>tostd::unordered_set<size_t>improves the duplicate detection check from O(log n) to O(1), which is appropriate for this DoS protection code path.
677-679: Explicit instantiations are complete.The three explicit template instantiations correctly cover all message types that use
ReceiveMessagePreamble.CDKGPrematureCommitmentintentionally has different handling and doesn't use this template.src/llmq/debug.cpp (4)
110-116: Dependency injection improves design.The new constructor that accepts
CDeterministicMNManager&,CQuorumSnapshotManager&, andconst ChainstateManager&improves testability and makes dependencies explicit. This aligns with the broader refactoring goals of the PR.
120-123: Simple and thread-safe accessor.The
GetSessionCount()method correctly usesWITH_LOCKto accesslocalStatus.sessions.size()under the mutex.
135-146: Session iteration correctly uses internal state and injected dependencies.The loop properly iterates over
localStatus.sessionsand passes the stored member references (m_dmnman,m_qsnapman,m_chainman) toCDKGDebugSessionStatus::ToJson. This ensures consistent behavior regardless of whereToJsonis called from.
125-150: Lock ordering is consistent and safe. TheToJsonmethod acquirescs_lockStatus, then callsCDKGDebugSessionStatus::ToJsonwhich usesWITH_LOCKto acquirecs_main. This establishes a consistent one-way lock order (cs_lockStatus → cs_main) with no reverse dependency. Public methods are properly annotated withEXCLUSIVE_LOCKS_REQUIRED(!cs_lockStatus), preventing misuse. No deadlock risk exists.
| std::unordered_set<size_t> CalcDeterministicWatchConnections(Consensus::LLMQType llmqType, | ||
| gsl::not_null<const CBlockIndex*> pQuorumBaseBlockIndex, | ||
| size_t memberCount, size_t connectionCount); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read its content around lines 65-67
head -n 80 src/llmq/utils.h | tail -n 30Repository: dashpay/dash
Length of output: 1616
🏁 Script executed:
# Check the full include section at the top of the file
head -n 20 src/llmq/utils.hRepository: dashpay/dash
Length of output: 527
🏁 Script executed:
# Find all call sites of CalcDeterministicWatchConnections
rg -n 'CalcDeterministicWatchConnections\s*\(' --type=cppRepository: dashpay/dash
Length of output: 606
🏁 Script executed:
# Check the call site at src/llmq/utils.cpp:805
sed -n '800,815p' src/llmq/utils.cppRepository: dashpay/dash
Length of output: 916
🏁 Script executed:
# Check the call site at src/active/quorums.cpp:60
sed -n '55,75p' src/active/quorums.cppRepository: dashpay/dash
Length of output: 2055
Add explicit #include <unordered_set> to src/llmq/utils.h.
The function declaration at lines 65–67 returns std::unordered_set<size_t>, but the header lacks an explicit include for it. Call sites (src/llmq/utils.cpp:805 and src/active/quorums.cpp:60) iterate through the result to index into arrays, so they don't depend on ordering. However, relying on transitive includes for a type used in the public API is fragile; add the include directly.
Proposed fix
#include <gsl/pointers.h>
+#include <unordered_set>
#include <set>
#include <vector>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/llmq/utils.h around lines 65 - 67, The header declares
std::unordered_set in the public API (CalcDeterministicWatchConnections) but
does not include <unordered_set>; add an explicit #include <unordered_set> to
the top of the header (next to the other standard includes) so the declaration
does not rely on transitive includes and consumers can compile independently.
1 participant
Additional Information
CActiveMasternodeManagerfromLLMQContext(3/n, DKG session isolation,ActiveContextconsolidation) #7065Breaking Changes
Work in progress.
Checklist