-
Notifications
You must be signed in to change notification settings - Fork 1.2k
refactor: extract CActiveMasternodeManager from LLMQContext (2/n, CQuorumManager handler separation)
#7063
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
b8addb7 to
f950f08
Compare
|
This pull request has conflicts, please rebase. |
…ns for `quorum dkg{info,status}`, move `CDKGDebugManager` to `{Active,Observer}Context`
e98fe4f doc: add release notes for breaking changes (Kittywhiskers Van Gogh)
f5a1617 refactor: move `CDKGDebugManager` to `{Active,Observer}Context` (Kittywhiskers Van Gogh)
9d09f91 rpc: add watch-only/masternode mode limits for `quorum dkg{info,status}` (Kittywhiskers Van Gogh)
d9fe923 test: only call `quorum dkg{info,status}` on nodes that can see DKG (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Depends on #7056
* Depends on #7061
* Dependency for #7063
## Breaking Changes
See release notes.
## 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
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK e98fe4f
Tree-SHA512: c1430f8b9714ccd000552bd4116d2d339d483d6bfa206c6a01973acaf36da86639afc88342f6d1f80f738929c3f5b6068eee38d36cb5964765dcc6175a998d4d
WalkthroughThis PR refactors LLMQ/quorum handling by splitting responsibilities into three components: a new CQuorumManager (llmq/quorumsman.{h,cpp}) that manages mined quorums, a QuorumObserver (llmq/observer/quorums.{h,cpp}) that monitors chain tips and triggers data recovery/cleanup, and a QuorumParticipant (active/quorums.{h,cpp}) that participates from the active masternode side. Public headers/constants/helpers were moved into llmq/quorums.h; many includes were switched to llmq/quorumsman.h. Context and Active/Observer constructors and startup/stop lifecycles were updated and wiring changed to connect the three components. Build files added the new sources/headers. Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/active/quorums.cpp (2)
87-107: Recovery start offset computation is safe but relies on our ProTxHash being presentThe modulo is safe because
quorum.qc->validMembers.size()is always > 0 for configured LLMQs, and the sorted MN list lookup gives a deterministic per‑member offset. If ourGetProTxHash()were ever missing from the list atpIndex, this would silently fall back to offset 0, which is benign but potentially surprising.If you expect
GetProTxHash()to always be present here in masternode mode, consider adding a debug assertion or log in the “not found” path to ease future diagnosis without changing runtime behaviour.
109-167: Encrypted contribution handling mirrors prior QGETDATA/QDATA logic with clear error pathsThe unified
ProcessEncryptedContribsimplementation:
- Validates quorum membership and presence of encrypted contributions for QGETDATA, setting the appropriate
CQuorumDataRequest::Errorsand only misbehaving when the caller has exceeded the request limit.- For QDATA, requires a full quorum vvec, validates membership again, then decrypts and aggregates contributions before calling
SetSecretKeyShare, with preciseMisbehavingErrorreasons for decryption or key‑share failures.This keeps behaviour aligned with previous code while making the masternode‑only side explicit.
You could optionally route both the QDATA aggregation path and
SetQuorumSecretKeyShare()through a single helper to avoid duplication of theAggregateSecretKeys+SetSecretKeySharepattern.src/Makefile.am (1)
154-155: New active/llmq quorum sources and headers are wired correctly; add to non-backported list.The additions of
active/quorums.{h,cpp},llmq/quorumsman.{h,cpp}, andllmq/observer/quorums.{h,cpp}toBITCOIN_CORE_Handlibbitcoin_node_a_SOURCESlook consistent with how other public llmq/masternode modules are exposed.One follow-up: since these are new, non‑Bitcoin backport files, they should also be listed in
test/util/data/non-backported.txtto keep the non‑backported tracking accurate. Based on learnings, this is expected for newly added Dash‑only code.Also applies to: 281-283, 291-292, 485-486, 555-557, 562-564
src/init.cpp (1)
2209-2212: Quorum observer/participant wiring and NetInstantSend/NetSigning integration are consistent.
- The new
quorums_recovery,quorums_watch, andsync_mapvalues are computed once inAppInitMain()and passed consistently intoActiveContext(masternode) orObserverContext(watch‑only). This matches the earlier parameter‑interaction validation usingGetEnabledQuorumVvecSyncEntries(args).- Exactly one of
active_ctxorobserver_ctxis constructed (mn_activemanvsquorums_watch), soCQuorumManager::ConnectManagers()is only called from a single context.- The added
NetInstantSendandNetSigninghandlers hang offpeermanand are created afterllmq_ctxis initialized and beforepeerman->StartHandlers(), and are torn down whenpeermanis reset, so their references toisman,qman, andsigmanstay valid.- In Step 10a,
observer_ctx->Start()is invoked afterllmq_ctx->Start()and before scheduler shutdown, symmetric withPrepareShutdown()’sobserver_ctx->Stop().Overall the lifecycle and dependency ordering here looks correct.
Also applies to: 2223-2227, 2231-2233, 2341-2342
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
src/Makefile.amsrc/active/quorums.cppsrc/active/quorums.hsrc/chainlock/chainlock.cppsrc/chainlock/chainlock.hsrc/dsnotificationinterface.cppsrc/evo/assetlocktx.cppsrc/evo/cbtx.cppsrc/evo/mnhftx.cppsrc/evo/smldiff.cppsrc/evo/specialtxman.cppsrc/init.cppsrc/instantsend/net_instantsend.cppsrc/instantsend/signing.cppsrc/llmq/context.cppsrc/llmq/context.hsrc/llmq/ehf_signals.cppsrc/llmq/observer/context.cppsrc/llmq/observer/context.hsrc/llmq/observer/quorums.cppsrc/llmq/observer/quorums.hsrc/llmq/options.hsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.hsrc/llmq/signing.cppsrc/llmq/signing_shares.cppsrc/masternode/active/context.cppsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/msg_result.hsrc/net_processing.cppsrc/node/chainstate.cppsrc/node/chainstate.hsrc/rpc/quorums.cppsrc/test/util/setup_common.cpptest/lint/lint-circular-dependencies.pytest/util/data/non-backported.txt
💤 Files with no reviewable changes (1)
- src/node/chainstate.h
🧰 Additional context used
📓 Path-based instructions (10)
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/evo/assetlocktx.cppsrc/rpc/quorums.cppsrc/llmq/signing_shares.cppsrc/evo/cbtx.cppsrc/chainlock/chainlock.cppsrc/instantsend/net_instantsend.cppsrc/evo/mnhftx.cppsrc/dsnotificationinterface.cppsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/chainlock/chainlock.hsrc/msg_result.hsrc/llmq/options.hsrc/net_processing.cppsrc/test/util/setup_common.cppsrc/llmq/context.hsrc/init.cppsrc/evo/smldiff.cppsrc/llmq/observer/quorums.hsrc/instantsend/signing.cppsrc/masternode/active/context.cppsrc/llmq/signing.cppsrc/evo/specialtxman.cppsrc/llmq/ehf_signals.cppsrc/llmq/observer/context.cppsrc/llmq/quorums.cppsrc/node/chainstate.cppsrc/llmq/quorums.hsrc/active/quorums.cppsrc/active/quorums.hsrc/llmq/observer/quorums.cppsrc/llmq/context.cppsrc/llmq/observer/context.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.h
src/{masternode,evo}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/assetlocktx.cppsrc/evo/cbtx.cppsrc/evo/mnhftx.cppsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/evo/smldiff.cppsrc/masternode/active/context.cppsrc/evo/specialtxman.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/evo/assetlocktx.cppsrc/llmq/signing_shares.cppsrc/evo/cbtx.cppsrc/evo/mnhftx.cppsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/llmq/options.hsrc/llmq/context.hsrc/evo/smldiff.cppsrc/llmq/observer/quorums.hsrc/masternode/active/context.cppsrc/llmq/signing.cppsrc/evo/specialtxman.cppsrc/llmq/ehf_signals.cppsrc/llmq/observer/context.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/observer/quorums.cppsrc/llmq/context.cppsrc/llmq/observer/context.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.h
src/evo/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Special transactions use payload serialization routines defined in src/evo/specialtx.h and must include appropriate special transaction types (ProRegTx, ProUpServTx, ProUpRegTx, ProUpRevTx)
Files:
src/evo/assetlocktx.cppsrc/evo/cbtx.cppsrc/evo/mnhftx.cppsrc/evo/smldiff.cppsrc/evo/specialtxman.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/evo/assetlocktx.cppsrc/llmq/signing_shares.cppsrc/evo/cbtx.cppsrc/evo/mnhftx.cppsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/llmq/options.hsrc/llmq/context.hsrc/evo/smldiff.cppsrc/llmq/observer/quorums.hsrc/masternode/active/context.cppsrc/llmq/signing.cppsrc/evo/specialtxman.cppsrc/llmq/ehf_signals.cppsrc/llmq/observer/context.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/observer/quorums.cppsrc/llmq/context.cppsrc/llmq/observer/context.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.h
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/signing_shares.cppsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/llmq/options.hsrc/llmq/context.hsrc/llmq/observer/quorums.hsrc/masternode/active/context.cppsrc/llmq/signing.cppsrc/llmq/ehf_signals.cppsrc/llmq/observer/context.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/observer/quorums.cppsrc/llmq/context.cppsrc/llmq/observer/context.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.h
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/signing_shares.cppsrc/llmq/options.hsrc/llmq/context.hsrc/llmq/observer/quorums.hsrc/llmq/signing.cppsrc/llmq/ehf_signals.cppsrc/llmq/observer/context.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/observer/quorums.cppsrc/llmq/context.cppsrc/llmq/observer/context.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.h
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/util/setup_common.cpp
src/node/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
NodeContext must be extended with Dash-specific managers for each subsystem during initialization
Files:
src/node/chainstate.cpp
src/node/chainstate.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Chainstate initialization must be separated into dedicated src/node/chainstate.* files
Files:
src/node/chainstate.cpp
🧠 Learnings (34)
📓 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: 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: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
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: 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.
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: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
📚 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/evo/assetlocktx.cppsrc/rpc/quorums.cppsrc/llmq/signing_shares.cppsrc/evo/cbtx.cppsrc/Makefile.amsrc/chainlock/chainlock.cppsrc/instantsend/net_instantsend.cppsrc/evo/mnhftx.cppsrc/dsnotificationinterface.cppsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/chainlock/chainlock.hsrc/llmq/options.hsrc/net_processing.cppsrc/test/util/setup_common.cppsrc/llmq/context.hsrc/init.cppsrc/evo/smldiff.cppsrc/llmq/observer/quorums.hsrc/instantsend/signing.cppsrc/masternode/active/context.cppsrc/llmq/signing.cppsrc/evo/specialtxman.cppsrc/llmq/ehf_signals.cppsrc/llmq/quorums.cppsrc/node/chainstate.cppsrc/llmq/quorums.hsrc/active/quorums.cppsrc/active/quorums.hsrc/llmq/observer/quorums.cppsrc/llmq/context.cppsrc/llmq/observer/context.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.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} : LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
Applied to files:
src/evo/assetlocktx.cppsrc/rpc/quorums.cppsrc/llmq/signing_shares.cppsrc/evo/cbtx.cppsrc/Makefile.amsrc/chainlock/chainlock.cppsrc/instantsend/net_instantsend.cppsrc/evo/mnhftx.cppsrc/dsnotificationinterface.cppsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/chainlock/chainlock.hsrc/llmq/options.hsrc/net_processing.cppsrc/test/util/setup_common.cppsrc/llmq/context.hsrc/init.cppsrc/evo/smldiff.cppsrc/llmq/observer/quorums.hsrc/instantsend/signing.cppsrc/masternode/active/context.cppsrc/llmq/signing.cppsrc/evo/specialtxman.cppsrc/llmq/ehf_signals.cppsrc/llmq/observer/context.cppsrc/llmq/quorums.cppsrc/node/chainstate.cppsrc/llmq/quorums.hsrc/active/quorums.cppsrc/active/quorums.hsrc/llmq/observer/quorums.cppsrc/llmq/context.cppsrc/llmq/observer/context.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.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} : ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Applied to files:
src/evo/assetlocktx.cppsrc/rpc/quorums.cppsrc/llmq/signing_shares.cppsrc/evo/cbtx.cppsrc/Makefile.amsrc/chainlock/chainlock.cppsrc/instantsend/net_instantsend.cppsrc/evo/mnhftx.cppsrc/masternode/active/context.hsrc/chainlock/chainlock.hsrc/net_processing.cppsrc/llmq/context.hsrc/evo/smldiff.cppsrc/llmq/observer/quorums.hsrc/instantsend/signing.cppsrc/masternode/active/context.cppsrc/llmq/signing.cppsrc/evo/specialtxman.cppsrc/llmq/ehf_signals.cppsrc/llmq/quorums.cppsrc/node/chainstate.cppsrc/llmq/quorums.hsrc/active/quorums.cppsrc/active/quorums.hsrc/llmq/observer/quorums.cppsrc/llmq/context.cppsrc/llmq/observer/context.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.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/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/evo/assetlocktx.cppsrc/evo/cbtx.cppsrc/Makefile.amsrc/chainlock/chainlock.cppsrc/evo/mnhftx.cppsrc/masternode/active/notificationinterface.cppsrc/evo/smldiff.cppsrc/instantsend/signing.cppsrc/masternode/active/context.cppsrc/evo/specialtxman.cppsrc/llmq/ehf_signals.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/observer/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}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features
Applied to files:
src/evo/assetlocktx.cppsrc/rpc/quorums.cppsrc/llmq/signing_shares.cppsrc/evo/cbtx.cppsrc/Makefile.amsrc/chainlock/chainlock.cppsrc/instantsend/net_instantsend.cppsrc/evo/mnhftx.cppsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cpptest/util/data/non-backported.txtsrc/evo/smldiff.cppsrc/instantsend/signing.cppsrc/masternode/active/context.cppsrc/llmq/signing.cppsrc/evo/specialtxman.cppsrc/llmq/ehf_signals.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/active/quorums.cppsrc/active/quorums.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.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/evo/**/*.{cpp,h} : Special transactions use payload serialization routines defined in src/evo/specialtx.h and must include appropriate special transaction types (ProRegTx, ProUpServTx, ProUpRegTx, ProUpRevTx)
Applied to files:
src/evo/assetlocktx.cppsrc/evo/mnhftx.cppsrc/evo/smldiff.cppsrc/evo/specialtxman.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/{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/evo/assetlocktx.cppsrc/evo/cbtx.cppsrc/Makefile.amsrc/chainlock/chainlock.cppsrc/evo/mnhftx.cppsrc/masternode/active/notificationinterface.cpptest/util/data/non-backported.txtsrc/evo/smldiff.cppsrc/instantsend/signing.cppsrc/evo/specialtxman.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/evo/assetlocktx.cppsrc/llmq/signing_shares.cppsrc/Makefile.amsrc/masternode/active/notificationinterface.cppsrc/masternode/active/context.cppsrc/llmq/signing.cppsrc/llmq/ehf_signals.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/active/quorums.cppsrc/active/quorums.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.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/rpc/quorums.cppsrc/llmq/signing_shares.cppsrc/evo/cbtx.cppsrc/Makefile.amsrc/instantsend/net_instantsend.cppsrc/evo/mnhftx.cppsrc/dsnotificationinterface.cppsrc/masternode/active/context.hsrc/net_processing.cppsrc/llmq/context.hsrc/init.cppsrc/evo/smldiff.cppsrc/llmq/observer/quorums.hsrc/masternode/active/context.cppsrc/llmq/signing.cppsrc/evo/specialtxman.cppsrc/llmq/ehf_signals.cppsrc/llmq/observer/context.cppsrc/llmq/quorums.cppsrc/node/chainstate.cppsrc/llmq/quorums.hsrc/active/quorums.cppsrc/active/quorums.hsrc/llmq/observer/quorums.cppsrc/llmq/context.cppsrc/llmq/observer/context.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.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,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Applied to files:
src/evo/cbtx.cppsrc/Makefile.amsrc/evo/mnhftx.cppsrc/test/util/setup_common.cpptest/lint/lint-circular-dependencies.pysrc/init.cppsrc/evo/smldiff.cppsrc/llmq/quorums.cppsrc/node/chainstate.cppsrc/llmq/quorums.h
📚 Learning: 2025-08-11T17:16:36.654Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Applied to files:
src/Makefile.amtest/util/data/non-backported.txt
📚 Learning: 2025-07-23T09:30:34.631Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Applied to files:
src/Makefile.am
📚 Learning: 2025-10-05T20:38:28.457Z
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Applied to files:
src/Makefile.am
📚 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/Makefile.am
📚 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/chainlock/chainlock.cppsrc/masternode/active/notificationinterface.cppsrc/evo/smldiff.cppsrc/instantsend/signing.cppsrc/evo/specialtxman.cpp
📚 Learning: 2025-11-25T10:53:37.523Z
Learnt from: knst
Repo: dashpay/dash PR: 7008
File: src/masternode/sync.h:17-18
Timestamp: 2025-11-25T10:53:37.523Z
Learning: In Dash Core, `DEFAULT_SYNC_MEMPOOL` belongs with node synchronization logic (currently in src/masternode/sync.h) rather than with validation logic in src/validation.h, even though other mempool-related constants are in validation.h. The constant is conceptually tied to sync functionality, not validation.
Applied to files:
src/chainlock/chainlock.cppsrc/instantsend/signing.cpp
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/instantsend/net_instantsend.cppsrc/dsnotificationinterface.cppsrc/masternode/active/context.hsrc/llmq/context.hsrc/masternode/active/context.cppsrc/llmq/observer/context.cppsrc/node/chainstate.cppsrc/llmq/quorums.hsrc/llmq/context.cppsrc/llmq/observer/context.hsrc/llmq/quorumsman.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/dsnotificationinterface.cppsrc/masternode/active/context.hsrc/llmq/context.hsrc/llmq/observer/quorums.hsrc/masternode/active/context.cppsrc/llmq/observer/context.cppsrc/node/chainstate.cppsrc/llmq/quorums.hsrc/active/quorums.cppsrc/active/quorums.hsrc/llmq/context.cppsrc/llmq/observer/context.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.h
📚 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/masternode/active/context.hsrc/test/util/setup_common.cppsrc/init.cppsrc/masternode/active/context.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/masternode/active/context.hsrc/chainlock/chainlock.hsrc/msg_result.hsrc/llmq/options.hsrc/llmq/context.hsrc/llmq/observer/quorums.hsrc/llmq/quorums.hsrc/active/quorums.hsrc/llmq/observer/context.hsrc/llmq/quorumsman.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/governance/**/*.{cpp,h} : Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
Applied to files:
src/masternode/active/notificationinterface.cppsrc/instantsend/signing.cppsrc/masternode/active/context.cpp
📚 Learning: 2025-07-29T14:32:48.369Z
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.
Applied to files:
src/chainlock/chainlock.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/test/util/setup_common.cppsrc/llmq/signing.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/active/quorums.cppsrc/active/quorums.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.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/test/util/setup_common.cppsrc/llmq/quorums.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/test/util/setup_common.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/node/chainstate.{cpp,h} : Chainstate initialization must be separated into dedicated src/node/chainstate.* files
Applied to files:
src/test/util/setup_common.cppsrc/instantsend/signing.cppsrc/node/chainstate.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/node/**/*.{cpp,h} : NodeContext must be extended with Dash-specific managers for each subsystem during initialization
Applied to files:
src/test/util/setup_common.cppsrc/init.cpp
📚 Learning: 2025-12-01T18:13:21.314Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 7018
File: test/lint/lint-github-actions.py:1-9
Timestamp: 2025-12-01T18:13:21.314Z
Learning: In the Dash repository, the file test/util/data/non-backported.txt should only include C++ files (.cpp or .h) because it is used for running clang-format. Other file types (such as Python files, .ui files, etc.) should not be added to this list.
Applied to files:
test/util/data/non-backported.txt
📚 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/bench/**/*.{cpp,h} : Performance benchmarks in src/bench/ must use the nanobench library
Applied to files:
test/util/data/non-backported.txt
📚 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/{test,wallet/test}/**/*.{cpp,h} : Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Applied to files:
test/util/data/non-backported.txt
📚 Learning: 2025-08-10T13:52:46.289Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6666
File: test/functional/rpc_netinfo.py:73-81
Timestamp: 2025-08-10T13:52:46.289Z
Learning: In the Bitcoin/Dash test framework, `self.node.extra_args` contains the original startup arguments for a TestNode and is not modified by `test.restart_node()`. Each restart with new `extra_args` doesn't update the stored `self.node.extra_args`, so copying from it always provides a clean slate without accumulated modifications from previous restarts.
Applied to files:
src/init.cpp
📚 Learning: 2025-12-17T13:58:26.891Z
Learnt from: kwvg
Repo: dashpay/dash PR: 7072
File: src/qt/walletcontroller.cpp:520-528
Timestamp: 2025-12-17T13:58:26.891Z
Learning: In Dash Qt wallet code, when leveraging upstream Bitcoin Core wallet capabilities (especially for operations like rescanning), prefer using the inherited signal-based mechanisms (e.g., ShowProgress signals) over adding explicit Qt progress dialogs to minimize upstream deviation and simplify future backports.
Applied to files:
src/init.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/masternode/active/context.cpp
🧬 Code graph analysis (10)
src/masternode/active/context.h (1)
src/active/quorums.cpp (2)
QuorumParticipant(27-37)QuorumParticipant(39-39)
src/masternode/active/notificationinterface.cpp (1)
src/net_processing.cpp (1)
pindexNew(607-607)
src/test/util/setup_common.cpp (1)
src/llmq/options.h (1)
DEFAULT_BLSCHECK_THREADS(33-33)
src/llmq/observer/quorums.h (3)
src/llmq/observer/context.h (2)
llmq(22-29)llmq(34-64)src/llmq/observer/quorums.cpp (24)
QuorumObserver(24-39)QuorumObserver(41-44)Start(46-52)Start(46-46)Stop(54-59)Stop(54-54)UpdatedBlockTip(61-75)UpdatedBlockTip(61-61)SetQuorumSecretKeyShare(127-131)SetQuorumSecretKeyShare(127-127)ProcessEncryptedContribs(133-151)ProcessEncryptedContribs(133-137)IsMasternode(153-157)IsMasternode(153-153)IsWatching(159-163)IsWatching(159-159)CheckQuorumConnections(106-125)CheckQuorumConnections(106-107)TriggerQuorumDataRecoveryThreads(286-299)TriggerQuorumDataRecoveryThreads(286-286)DataRecoveryThread(165-270)DataRecoveryThread(165-166)StartCleanupOldQuorumDataThread(319-372)StartCleanupOldQuorumDataThread(319-319)src/llmq/observer/context.cpp (6)
Start(35-38)Start(35-35)Stop(40-43)Stop(40-40)UpdatedBlockTip(45-52)UpdatedBlockTip(45-45)
src/llmq/observer/context.cpp (1)
src/llmq/observer/quorums.cpp (4)
Start(46-52)Start(46-46)Stop(54-59)Stop(54-54)
src/llmq/quorums.h (4)
src/active/quorums.h (1)
llmq(30-35)src/llmq/observer/quorums.h (1)
llmq(34-37)src/llmq/context.h (1)
llmq(22-29)src/evo/mnhftx.h (1)
llmq(30-32)
src/llmq/observer/quorums.cpp (3)
src/llmq/quorums.h (1)
nDataMask(87-87)src/llmq/observer/quorums.h (1)
m_quorums_recovery(76-127)src/llmq/quorums.cpp (2)
MakeQuorumKey(18-27)MakeQuorumKey(18-18)
src/llmq/observer/context.h (2)
src/llmq/observer/quorums.cpp (8)
QuorumObserver(24-39)QuorumObserver(41-44)Start(46-52)Start(46-46)Stop(54-59)Stop(54-54)UpdatedBlockTip(61-75)UpdatedBlockTip(61-61)src/llmq/observer/context.cpp (8)
ObserverContext(13-28)ObserverContext(30-33)Start(35-38)Start(35-35)Stop(40-43)Stop(40-40)UpdatedBlockTip(45-52)UpdatedBlockTip(45-45)
src/llmq/quorumsman.cpp (4)
src/llmq/quorums.h (2)
llmq(25-30)llmq(226-226)src/active/quorums.h (1)
llmq(30-35)src/llmq/observer/context.h (2)
llmq(22-29)llmq(34-64)src/llmq/quorums.cpp (5)
DataCleanupHelper(29-74)DataCleanupHelper(29-29)batch(33-33)s(196-196)s(211-211)
src/llmq/quorumsman.h (4)
src/llmq/observer/quorums.h (1)
llmq(34-37)src/llmq/quorums.h (2)
llmq(25-30)llmq(226-226)src/llmq/quorumsman.cpp (20)
CQuorumManager(32-46)CQuorumManager(48-54)GetQuorum(368-389)GetQuorum(368-368)GetQuorum(391-407)GetQuorum(391-391)ScanQuorums(193-197)ScanQuorums(193-193)ScanQuorums(199-307)ScanQuorums(199-201)IsMasternode(309-315)IsMasternode(309-309)IsWatching(317-323)IsWatching(317-317)BuildQuorumFromCommitment(66-112)BuildQuorumFromCommitment(66-66)SelectQuorumForSigning(675-735)SelectQuorumForSigning(675-676)VerifyRecoveredSig(737-751)VerifyRecoveredSig(737-739)src/llmq/params.h (1)
LLMQType(14-125)
🪛 Cppcheck (2.19.0)
src/masternode/active/context.cpp
[error] 24-24: #error No known always_inline attribute for this platform.
(preprocessorErrorDirective)
src/llmq/observer/context.cpp
[error] 10-10: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
src/llmq/quorums.cpp
[error] 10-10: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
src/active/quorums.cpp
[error] 10-10: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
[error] 24-24: #error No known always_inline attribute for this platform.
(preprocessorErrorDirective)
src/llmq/observer/quorums.cpp
[error] 10-10: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
[error] 24-24: #error No known always_inline attribute for this platform.
(preprocessorErrorDirective)
src/llmq/context.cpp
[error] 24-24: #error No known always_inline attribute for this platform.
(preprocessorErrorDirective)
src/llmq/quorumsman.cpp
[error] 10-10: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
[error] 24-24: #error No known always_inline attribute for this platform.
(preprocessorErrorDirective)
⏰ 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). (1)
- GitHub Check: linux64_tsan-test / Test source
41b9283 refactor: inline error handler in `CQuorumManager::ProcessMessage()` (Kittywhiskers Van Gogh) caa5400 refactor: move `mapQuorumDataRequests` to `CQuorumManager`, use `Mutex` (Kittywhiskers Van Gogh) Pull request description: ## 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 `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: knst: utACK 41b9283 Tree-SHA512: 1ec93540031be1006ceefd4a2f1445f2edc089958bc8dd6dbefda689bd16ff21804b7bc1a7bbe7bae228a12824c898d43c38a8664100634469bf117349c02b05
|
This pull request has conflicts, please rebase. |
These subroutines do nothing unless we are in masternode mode so we need to extract them in order to isolate it from regular mode.
Also: - Drop avoidable `CActiveMasternodeManager&` arg by taking ref of hash we care about
Review with `git log -p -n1 --color-moved=dimmed_zebra`.
This is done to remove m_mn_activeman calls from within the worker to allow us to run it in watch-only mode.
Needed to split logic based on watch-only or masternode-mode in next commit.
Review with `git log -p -n1 --color-moved=dimmed_zebra`.
Right now we have a circular dependency between `CQuorumManager`'s TU and the handler's TU's but we also house common code in the former, we need to split it out before we can close the gap. Review with `git log -p -n1 --color-moved=dimmed_zebra`.
Handlers are initialized from contexts with complete knowledge of P2P, so we can trim down the arguments passed a fair bit. Also: - Move `UpdatedBlockTip()` to handler parent contexts and out of `CQuorumManager`
We need to split up the cache warming thread with the rest of the tasks given the thread pool used by the handlers as we start to isolate impl. details of the parent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/llmq/quorums.cpp (1)
14-32: DB key helpers centralize quorum contribution storage; minor batch nitMoving
DB_QUORUM_SK_SHARE/DB_QUORUM_QUORUM_VVECintollmqand exposingMakeQuorumKey(const CQuorum&)keeps all contribution keying logic in one place and makes it usable fromCQuorumManager/migration code. The key now hashes{llmqType, quorumHash, members[i].proTxHash}which should be stable per quorum and per node.
DataCleanupHelper()correctly walks both prefixes, skips the providedskip_list, and erases the rest in batches with an optionalCompactFull()pass for migrations. One very small improvement you might consider (not urgent):
- Clear
batchafter the finaldb.WriteBatch(batch);per prefix to avoid re‑emitting already‑applied erase ops when processing the second prefix.Functionally everything is fine; this would just trim a bit of redundant I/O.
Also applies to: 29-75
src/llmq/quorumsman.cpp (2)
484-496: The const_cast is documented but warrants attention.The TODO comment on line 486 acknowledges that
const_castonpQuorumis a code smell. While the current implementation doesn't actually write to the quorum object inProcessEncryptedContribswhen handlingQGETDATA, this design could lead to accidental modifications.Consider returning a mutable quorum reference from the cache lookup or refactoring
ProcessEncryptedContribsto take a const reference where possible.
612-673: Migration code is properly marked for removal in v23.The TODO on line 612 clearly indicates this migration code should be removed in v23. The migration:
- Checks if new DB is empty before migrating
- Copies both verification vectors and secret key shares
- Uses batch writes for efficiency
- Compacts the database after migration
- Cleans up the old EvoDB entries
Would you like me to open an issue to track the removal of this migration code in v23?
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
src/Makefile.amsrc/active/quorums.cppsrc/active/quorums.hsrc/chainlock/chainlock.cppsrc/chainlock/chainlock.hsrc/dsnotificationinterface.cppsrc/evo/assetlocktx.cppsrc/evo/cbtx.cppsrc/evo/mnhftx.cppsrc/evo/smldiff.cppsrc/evo/specialtxman.cppsrc/init.cppsrc/instantsend/net_instantsend.cppsrc/instantsend/signing.cppsrc/llmq/context.cppsrc/llmq/context.hsrc/llmq/ehf_signals.cppsrc/llmq/observer/context.cppsrc/llmq/observer/context.hsrc/llmq/observer/quorums.cppsrc/llmq/observer/quorums.hsrc/llmq/options.hsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.hsrc/llmq/signing.cppsrc/llmq/signing_shares.cppsrc/masternode/active/context.cppsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/msg_result.hsrc/net_processing.cppsrc/node/chainstate.cppsrc/node/chainstate.hsrc/rpc/quorums.cppsrc/test/util/setup_common.cpptest/lint/lint-circular-dependencies.pytest/util/data/non-backported.txt
💤 Files with no reviewable changes (1)
- src/node/chainstate.h
🚧 Files skipped from review as they are similar to previous changes (11)
- src/evo/specialtxman.cpp
- src/evo/mnhftx.cpp
- test/util/data/non-backported.txt
- src/llmq/signing_shares.cpp
- src/rpc/quorums.cpp
- src/llmq/options.h
- src/dsnotificationinterface.cpp
- src/msg_result.h
- src/instantsend/net_instantsend.cpp
- src/llmq/context.h
- src/evo/smldiff.cpp
🧰 Additional context used
📓 Path-based instructions (10)
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/instantsend/signing.cppsrc/evo/cbtx.cppsrc/evo/assetlocktx.cppsrc/llmq/observer/context.hsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/test/util/setup_common.cppsrc/chainlock/chainlock.cppsrc/llmq/context.cppsrc/masternode/active/context.cppsrc/chainlock/chainlock.hsrc/llmq/signing.cppsrc/llmq/observer/quorums.cppsrc/llmq/ehf_signals.cppsrc/llmq/observer/context.cppsrc/llmq/quorums.hsrc/llmq/quorums.cppsrc/init.cppsrc/active/quorums.hsrc/active/quorums.cppsrc/net_processing.cppsrc/node/chainstate.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.cppsrc/llmq/observer/quorums.h
src/{masternode,evo}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/cbtx.cppsrc/evo/assetlocktx.cppsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/masternode/active/context.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/evo/cbtx.cppsrc/evo/assetlocktx.cppsrc/llmq/observer/context.hsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/llmq/context.cppsrc/masternode/active/context.cppsrc/llmq/signing.cppsrc/llmq/observer/quorums.cppsrc/llmq/ehf_signals.cppsrc/llmq/observer/context.cppsrc/llmq/quorums.hsrc/llmq/quorums.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.cppsrc/llmq/observer/quorums.h
src/evo/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Special transactions use payload serialization routines defined in src/evo/specialtx.h and must include appropriate special transaction types (ProRegTx, ProUpServTx, ProUpRegTx, ProUpRevTx)
Files:
src/evo/cbtx.cppsrc/evo/assetlocktx.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/evo/cbtx.cppsrc/evo/assetlocktx.cppsrc/llmq/observer/context.hsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/llmq/context.cppsrc/masternode/active/context.cppsrc/llmq/signing.cppsrc/llmq/observer/quorums.cppsrc/llmq/ehf_signals.cppsrc/llmq/observer/context.cppsrc/llmq/quorums.hsrc/llmq/quorums.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.cppsrc/llmq/observer/quorums.h
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/observer/context.hsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/llmq/context.cppsrc/masternode/active/context.cppsrc/llmq/signing.cppsrc/llmq/observer/quorums.cppsrc/llmq/ehf_signals.cppsrc/llmq/observer/context.cppsrc/llmq/quorums.hsrc/llmq/quorums.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.cppsrc/llmq/observer/quorums.h
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/observer/context.hsrc/llmq/context.cppsrc/llmq/signing.cppsrc/llmq/observer/quorums.cppsrc/llmq/ehf_signals.cppsrc/llmq/observer/context.cppsrc/llmq/quorums.hsrc/llmq/quorums.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.cppsrc/llmq/observer/quorums.h
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/util/setup_common.cpp
src/node/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
NodeContext must be extended with Dash-specific managers for each subsystem during initialization
Files:
src/node/chainstate.cpp
src/node/chainstate.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Chainstate initialization must be separated into dedicated src/node/chainstate.* files
Files:
src/node/chainstate.cpp
🧠 Learnings (29)
📓 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: 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: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: 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: 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: 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.
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} : InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
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
📚 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/instantsend/signing.cppsrc/evo/cbtx.cppsrc/evo/assetlocktx.cppsrc/llmq/observer/context.hsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/test/util/setup_common.cppsrc/chainlock/chainlock.cppsrc/llmq/context.cppsrc/masternode/active/context.cppsrc/chainlock/chainlock.hsrc/llmq/signing.cppsrc/llmq/observer/quorums.cppsrc/llmq/ehf_signals.cppsrc/llmq/quorums.hsrc/llmq/quorums.cppsrc/active/quorums.hsrc/Makefile.amsrc/active/quorums.cppsrc/net_processing.cppsrc/node/chainstate.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.cppsrc/llmq/observer/quorums.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} : LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
Applied to files:
src/instantsend/signing.cppsrc/evo/cbtx.cppsrc/evo/assetlocktx.cppsrc/llmq/observer/context.hsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/test/util/setup_common.cppsrc/chainlock/chainlock.cppsrc/llmq/context.cppsrc/masternode/active/context.cppsrc/chainlock/chainlock.hsrc/llmq/signing.cppsrc/llmq/observer/quorums.cppsrc/llmq/ehf_signals.cppsrc/llmq/observer/context.cppsrc/llmq/quorums.hsrc/llmq/quorums.cppsrc/active/quorums.hsrc/Makefile.amsrc/active/quorums.cppsrc/net_processing.cppsrc/node/chainstate.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.cppsrc/llmq/observer/quorums.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/instantsend/signing.cppsrc/evo/cbtx.cppsrc/evo/assetlocktx.cppsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/chainlock/chainlock.cppsrc/masternode/active/context.cppsrc/llmq/signing.cppsrc/llmq/ehf_signals.cppsrc/llmq/quorums.hsrc/llmq/quorums.cppsrc/active/quorums.hsrc/Makefile.amsrc/active/quorums.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.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/instantsend/signing.cppsrc/evo/cbtx.cppsrc/evo/assetlocktx.cppsrc/llmq/observer/context.hsrc/masternode/active/context.hsrc/test/util/setup_common.cppsrc/chainlock/chainlock.cppsrc/llmq/context.cppsrc/masternode/active/context.cppsrc/chainlock/chainlock.hsrc/llmq/signing.cppsrc/llmq/observer/quorums.cppsrc/llmq/ehf_signals.cppsrc/llmq/quorums.hsrc/llmq/quorums.cppsrc/active/quorums.hsrc/Makefile.amsrc/active/quorums.cppsrc/net_processing.cppsrc/node/chainstate.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.cppsrc/llmq/observer/quorums.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/instantsend/signing.cppsrc/evo/cbtx.cppsrc/evo/assetlocktx.cppsrc/masternode/active/notificationinterface.cppsrc/chainlock/chainlock.cppsrc/Makefile.am
📚 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/instantsend/signing.cppsrc/masternode/active/notificationinterface.cppsrc/chainlock/chainlock.cpp
📚 Learning: 2025-11-25T10:53:37.523Z
Learnt from: knst
Repo: dashpay/dash PR: 7008
File: src/masternode/sync.h:17-18
Timestamp: 2025-11-25T10:53:37.523Z
Learning: In Dash Core, `DEFAULT_SYNC_MEMPOOL` belongs with node synchronization logic (currently in src/masternode/sync.h) rather than with validation logic in src/validation.h, even though other mempool-related constants are in validation.h. The constant is conceptually tied to sync functionality, not validation.
Applied to files:
src/instantsend/signing.cppsrc/chainlock/chainlock.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/instantsend/signing.cppsrc/evo/cbtx.cppsrc/evo/assetlocktx.cppsrc/masternode/active/notificationinterface.cppsrc/chainlock/chainlock.cppsrc/masternode/active/context.cppsrc/llmq/observer/quorums.cppsrc/llmq/ehf_signals.cppsrc/llmq/quorums.hsrc/llmq/quorums.cppsrc/Makefile.am
📚 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/node/chainstate.{cpp,h} : Chainstate initialization must be separated into dedicated src/node/chainstate.* files
Applied to files:
src/instantsend/signing.cppsrc/test/util/setup_common.cppsrc/node/chainstate.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/governance/**/*.{cpp,h} : Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
Applied to files:
src/instantsend/signing.cppsrc/masternode/active/notificationinterface.cppsrc/masternode/active/context.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,evo,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Applied to files:
src/evo/cbtx.cppsrc/test/util/setup_common.cpptest/lint/lint-circular-dependencies.pysrc/llmq/quorums.hsrc/llmq/quorums.cppsrc/init.cppsrc/Makefile.amsrc/node/chainstate.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/evo/cbtx.cppsrc/llmq/observer/context.hsrc/masternode/active/context.hsrc/llmq/context.cppsrc/masternode/active/context.cppsrc/llmq/signing.cppsrc/llmq/observer/quorums.cppsrc/llmq/ehf_signals.cppsrc/llmq/observer/context.cppsrc/llmq/quorums.hsrc/llmq/quorums.cppsrc/init.cppsrc/active/quorums.hsrc/Makefile.amsrc/active/quorums.cppsrc/net_processing.cppsrc/node/chainstate.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.cppsrc/llmq/observer/quorums.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/evo/**/*.{cpp,h} : Special transactions use payload serialization routines defined in src/evo/specialtx.h and must include appropriate special transaction types (ProRegTx, ProUpServTx, ProUpRegTx, ProUpRevTx)
Applied to files:
src/evo/assetlocktx.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/evo/assetlocktx.cppsrc/masternode/active/notificationinterface.cppsrc/masternode/active/context.cppsrc/llmq/signing.cppsrc/llmq/ehf_signals.cppsrc/llmq/quorums.hsrc/llmq/quorums.cppsrc/active/quorums.hsrc/Makefile.amsrc/active/quorums.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.cpp
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/llmq/observer/context.hsrc/masternode/active/context.hsrc/llmq/context.cppsrc/masternode/active/context.cppsrc/llmq/observer/context.cppsrc/llmq/quorums.hsrc/node/chainstate.cppsrc/llmq/quorumsman.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/observer/context.hsrc/masternode/active/context.hsrc/llmq/context.cppsrc/masternode/active/context.cppsrc/llmq/observer/context.cppsrc/llmq/quorums.hsrc/active/quorums.hsrc/active/quorums.cppsrc/node/chainstate.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.cppsrc/llmq/observer/quorums.h
📚 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/observer/context.hsrc/masternode/active/context.hsrc/chainlock/chainlock.hsrc/llmq/quorums.hsrc/active/quorums.hsrc/llmq/quorumsman.hsrc/llmq/observer/quorums.h
📚 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/masternode/active/context.hsrc/test/util/setup_common.cppsrc/masternode/active/context.cppsrc/init.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/test/util/setup_common.cppsrc/llmq/signing.cppsrc/llmq/quorums.hsrc/llmq/quorums.cppsrc/active/quorums.hsrc/active/quorums.cppsrc/node/chainstate.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.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:
src/test/util/setup_common.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/test/util/setup_common.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/node/**/*.{cpp,h} : NodeContext must be extended with Dash-specific managers for each subsystem during initialization
Applied to files:
src/test/util/setup_common.cppsrc/init.cpp
📚 Learning: 2025-11-25T10:53:37.523Z
Learnt from: knst
Repo: dashpay/dash PR: 7008
File: src/masternode/sync.h:17-18
Timestamp: 2025-11-25T10:53:37.523Z
Learning: The file src/masternode/sync.h containing `CMasternodeSync` is misnamed and misplaced—it has nothing to do with "masternode" functionality. It should eventually be renamed to `NodeSyncing` or `NodeSyncStatus` and moved to src/node/sync.h as a future refactoring.
Applied to files:
src/masternode/active/context.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/masternode/active/context.cpp
📚 Learning: 2025-07-29T14:32:48.369Z
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.
Applied to files:
src/chainlock/chainlock.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/llmq/quorums.cppsrc/Makefile.am
📚 Learning: 2025-08-11T17:16:36.654Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Applied to files:
src/Makefile.am
📚 Learning: 2025-07-23T09:30:34.631Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Applied to files:
src/Makefile.am
🧬 Code graph analysis (11)
src/llmq/observer/context.h (3)
src/llmq/observer/quorums.h (1)
llmq(34-37)src/llmq/observer/quorums.cpp (8)
QuorumObserver(24-39)QuorumObserver(41-44)Start(46-52)Start(46-46)Stop(54-59)Stop(54-54)UpdatedBlockTip(61-75)UpdatedBlockTip(61-61)src/llmq/observer/context.cpp (8)
ObserverContext(13-28)ObserverContext(30-33)Start(35-38)Start(35-35)Stop(40-43)Stop(40-40)UpdatedBlockTip(45-52)UpdatedBlockTip(45-45)
src/masternode/active/context.h (1)
src/active/quorums.cpp (2)
QuorumParticipant(27-37)QuorumParticipant(39-39)
src/masternode/active/notificationinterface.cpp (1)
src/net_processing.cpp (1)
pindexNew(607-607)
src/test/util/setup_common.cpp (1)
src/llmq/options.h (1)
DEFAULT_BLSCHECK_THREADS(33-33)
src/llmq/observer/context.cpp (1)
src/llmq/observer/quorums.cpp (4)
Start(46-52)Start(46-46)Stop(54-59)Stop(54-54)
src/llmq/quorums.h (8)
src/active/quorums.h (1)
llmq(30-35)src/llmq/observer/quorums.h (1)
llmq(34-37)src/llmq/context.h (1)
llmq(22-29)src/llmq/blockprocessor.h (1)
llmq(36-103)src/evo/mnhftx.h (1)
llmq(30-32)src/evo/creditpool.h (1)
llmq(33-35)src/llmq/types.h (1)
llmq(10-17)src/llmq/quorums.cpp (7)
CQuorum(101-103)MakeQuorumKey(18-27)MakeQuorumKey(18-18)DataCleanupHelper(29-74)DataCleanupHelper(29-29)SetSecretKeyShare(125-133)SetSecretKeyShare(125-125)
src/init.cpp (2)
src/init.h (1)
node(23-25)src/llmq/options.h (1)
DEFAULT_WATCH_QUORUMS(37-51)
src/active/quorums.h (5)
src/llmq/quorums.h (2)
llmq(26-31)llmq(227-227)src/llmq/observer/quorums.h (1)
llmq(34-37)src/llmq/quorums.cpp (1)
CQuorum(101-103)src/active/quorums.cpp (10)
QuorumParticipant(27-37)QuorumParticipant(39-39)IsMasternode(169-173)IsMasternode(169-169)IsWatching(175-179)IsWatching(175-175)SetQuorumSecretKeyShare(82-85)SetQuorumSecretKeyShare(82-82)ProcessEncryptedContribs(109-167)ProcessEncryptedContribs(109-113)src/llmq/observer/quorums.cpp (10)
QuorumObserver(24-39)QuorumObserver(41-44)IsMasternode(153-157)IsMasternode(153-153)IsWatching(159-163)IsWatching(159-159)SetQuorumSecretKeyShare(127-131)SetQuorumSecretKeyShare(127-127)ProcessEncryptedContribs(133-151)ProcessEncryptedContribs(133-137)
src/llmq/quorumsman.h (5)
src/llmq/observer/context.h (4)
util(30-32)llmq(22-29)llmq(34-64)final(35-63)src/llmq/quorums.h (2)
llmq(26-31)llmq(227-227)src/llmq/observer/quorums.h (1)
llmq(34-37)src/llmq/quorumsman.cpp (20)
CQuorumManager(32-46)CQuorumManager(48-54)HasQuorum(145-148)HasQuorum(145-145)GetQuorum(368-389)GetQuorum(368-368)GetQuorum(391-407)GetQuorum(391-391)ScanQuorums(193-197)ScanQuorums(193-193)ScanQuorums(199-307)ScanQuorums(199-201)IsMasternode(309-315)IsMasternode(309-309)IsWatching(317-323)IsWatching(317-317)CacheWarmingThreadMain(566-602)CacheWarmingThreadMain(566-566)MigrateOldQuorumDB(613-673)MigrateOldQuorumDB(613-613)src/llmq/params.h (1)
LLMQType(14-125)
src/llmq/quorumsman.cpp (5)
src/llmq/quorums.h (3)
nDataMask(88-88)llmq(26-31)llmq(227-227)src/llmq/observer/quorums.h (1)
llmq(34-37)src/llmq/observer/context.h (2)
llmq(22-29)llmq(34-64)src/llmq/quorums.cpp (4)
DataCleanupHelper(29-74)DataCleanupHelper(29-29)s(196-196)s(211-211)src/msg_result.h (1)
MisbehavingError(19-72)
src/llmq/observer/quorums.h (2)
src/llmq/observer/context.h (2)
llmq(22-29)llmq(34-64)src/llmq/observer/quorums.cpp (30)
QuorumObserver(24-39)QuorumObserver(41-44)Start(46-52)Start(46-46)Stop(54-59)Stop(54-54)UpdatedBlockTip(61-75)UpdatedBlockTip(61-61)SetQuorumSecretKeyShare(127-131)SetQuorumSecretKeyShare(127-127)ProcessEncryptedContribs(133-151)ProcessEncryptedContribs(133-137)IsMasternode(153-157)IsMasternode(153-153)IsWatching(159-163)IsWatching(159-159)CheckQuorumConnections(106-125)CheckQuorumConnections(106-107)TriggerQuorumDataRecoveryThreads(286-299)TriggerQuorumDataRecoveryThreads(286-286)GetQuorumsToDelete(77-104)GetQuorumsToDelete(77-78)DataRecoveryThread(165-270)DataRecoveryThread(165-166)StartVvecSyncThread(272-284)StartVvecSyncThread(272-272)TryStartVvecSyncThread(301-317)TryStartVvecSyncThread(301-302)StartCleanupOldQuorumDataThread(319-372)StartCleanupOldQuorumDataThread(319-319)
🪛 Cppcheck (2.19.0)
src/llmq/context.cpp
[error] 24-24: #error No known always_inline attribute for this platform.
(preprocessorErrorDirective)
src/masternode/active/context.cpp
[error] 24-24: #error No known always_inline attribute for this platform.
(preprocessorErrorDirective)
src/llmq/observer/quorums.cpp
[error] 10-10: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
[error] 24-24: #error No known always_inline attribute for this platform.
(preprocessorErrorDirective)
src/llmq/observer/context.cpp
[error] 10-10: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
src/llmq/quorums.cpp
[error] 10-10: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
src/active/quorums.cpp
[error] 10-10: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
[error] 24-24: #error No known always_inline attribute for this platform.
(preprocessorErrorDirective)
src/llmq/quorumsman.cpp
[error] 10-10: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
[error] 24-24: #error No known always_inline attribute for this platform.
(preprocessorErrorDirective)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (62)
src/test/util/setup_common.cpp (1)
142-152: LGTM! Good simplification of test configuration.The updated parameter list replaces explicit quorum configuration (sync maps, data recovery/watch booleans) with streamlined defaults (
llmq::DEFAULT_BLSCHECK_THREADS,llmq::DEFAULT_MAX_RECOVERED_SIGS_AGE). UsingDEFAULT_BLSCHECK_THREADS = 0is appropriate for test scenarios, enabling synchronous BLS processing and reducing threading complexity. This aligns well with the PR's refactoring objectives.src/chainlock/chainlock.h (1)
37-42: Forward declaration now pins underlying typeSpecifying
: uint8_there is fine and should match the definition inllmq/quorumsman.h; just keep these in sync if the enum ever changes.src/Makefile.am (1)
154-155: Build wiring for new quorum components looks consistentThe additions of
active/quorums.{h,cpp},llmq/quorumsman.{h,cpp}, andllmq/observer/quorums.{h,cpp}toBITCOIN_CORE_Handlibbitcoin_node_a_SOURCESmatch how other LLMQ/masternode modules are exposed; the build side looks correct.One process note: if not already done elsewhere in this PR, please also add these new non‑backported files to
test/util/data/non-backported.txtto comply with the Dash convention for newly introduced (non‑Bitcoin) sources.Also applies to: 281-283, 291-292, 485-486, 555-557, 563-563
src/init.cpp (2)
275-299: Observer context lifecycle integrates cleanly into startup/shutdownAdding
observer_ctx->Stop()inPrepareShutdown()andobserver_ctx->Start()alongsideactive_ctxkeeps the LLMQ observer lifecycle consistent with other Dash services, and the Start/Stop ordering (llmq_ctx → peerman → active/observer on start, and the reverse on shutdown) looks sound.One thing to double‑check:
PrepareShutdown()will callobserver_ctx->Stop()whenever the pointer is non‑null, even on early‑failure paths whereStart()might not have been called yet. Please ensurellmq::ObserverContext::Stop()is safe and idempotent in that scenario.Also applies to: 2338-2342
2209-2219: Quorum sync/recovery flags and handlers are wired coherentlyDeriving
quorums_recovery,quorums_watch, and the sharedllmq::QvvecSyncModeMap sync_maponce inAppInitMain()and passing them into:
ActiveContext(..., sync_map, dash_db_params, quorums_recovery, quorums_watch)whenmn_activemanis present, andObserverContext(..., sync_map, dash_db_params, quorums_recovery)for non‑MN watch‑only nodes,keeps masternode and observer modes clearly separated while sharing the same sync configuration. Moving
NetInstantSendandNetSigninghandler registration after this setup also ensures they see a fully initialized LLMQ stack.Behaviorally this matches the PR description (participant vs watch‑only split); just confirm that “masternode + watch” is intentionally handled inside
ActiveContext/QuorumParticipantand not via a separateobserver_ctxas theelse if (quorums_watch)branch implies.Also applies to: 2223-2225, 2231-2233
src/llmq/quorums.cpp (1)
125-133: Secret key share validation via proTx hash is soundUpdating
CQuorum::SetSecretKeyShareto:
- take
const uint256& protx_hash, and- verify
secretKeyShare.GetPublicKey() == GetPubKeyShare(GetMemberIndex(protx_hash))keeps the membership check local to
CQuorumand removes the dependency onCActiveMasternodeManager. GivenGetPubKeyShareguards against out‑of‑range indices, passing a non‑memberprotx_hashsimply causes the check to fail and returnsfalse, which is the desired behavior.
WriteContributions/ReadContributionsusingMakeQuorumKey(*this)and the new DB prefixes stays consistent with the helpers above, so contribution persistence remains straightforward.Also applies to: 190-205, 208-231
src/chainlock/chainlock.cpp (1)
19-21: Include switched to new quorum manager headerSwitching from
<llmq/quorums.h>to<llmq/quorumsman.h>aligns this file with the newCQuorumManager/VerifyRecoveredSighome and keeps the ChainLocks dependency surface tight. No issues spotted.src/instantsend/signing.cpp (1)
14-16: InstantSend signer now includes the correct quorum manager headerUpdating the include to
llmq/quorumsman.hmatches the new location ofCQuorumManagerandSelectQuorumForSigning. The rest ofInstantSendSignerremains unchanged, so this is a clean dependency update.src/evo/cbtx.cpp (1)
11-11: LGTM! Include path updated consistently with refactoring.The header change from
llmq/quorums.htollmq/quorumsman.hmaintains access tollmq::CQuorumBlockProcessorused throughout this file for quorum commitment processing and merkle root calculation.src/llmq/ehf_signals.cpp (1)
16-16: LGTM! Header refactor completed consistently.The include update from
llmq/quorums.htollmq/quorumsman.haligns with the PR's quorum management restructuring. The file's usage ofCQuorumManagerandSelectQuorumForSigningremains unchanged.src/evo/assetlocktx.cpp (1)
9-9: Include refactoring is correct. The change fromllmq/quorums.htollmq/quorumsman.hproperly addresses symbol requirements:assetlocktx.cppusesllmq::CQuorumManageras a const reference parameter in function signatures, which requires the full class definition available inquorumsman.h(not just the forward declaration inquorums.h).src/net_processing.cpp (1)
58-58: Include switch tollmq/quorumsman.haligns with quorum-manager usageUsing the more specific manager header here matches this file’s direct use of
m_llmq_ctx->qmanand related APIs, and keeps the dependency consistent with the new refactor. No further changes needed in this unit.src/llmq/signing.cpp (1)
9-9: LGTM!The include update from
llmq/quorums.htollmq/quorumsman.hcorrectly aligns with the PR's refactoring that movesCQuorumManagerand related APIs to the new header. The file's usage ofCQuorumManagerandIsQuorumActiveis properly supported by the new include.src/masternode/active/notificationinterface.cpp (2)
7-7: LGTM!The include addition enables access to the
QuorumParticipanttype needed for the newqman_handlerinvocation.
33-33: LGTM!The new
qman_handler->UpdatedBlockTipcall follows the established notification pattern in this file and correctly propagates block tip updates to theQuorumParticipant. This integrates the masternode-specific quorum handling into the notification chain.src/llmq/observer/context.h (5)
8-8: LGTM!The
llmq/options.hinclude is needed for theQvvecSyncModeMaptype used in the constructor signature.
16-20: LGTM!Forward declarations for
CConnmanandCMasternodeSyncare properly added to support the expanded constructor signature without introducing unnecessary header dependencies.
28-28: LGTM!Forward declaration for
QuorumObserveris correctly placed within thellmqnamespace.
43-51: LGTM!The expanded constructor signature properly accepts all dependencies needed for
QuorumObserverconstruction. TheStart()andStop()lifecycle methods follow the established pattern in the codebase for managing thread lifecycles.
60-62: LGTM!The
qman_handlermember is appropriately declared as private since it's an internal implementation detail. Usingstd::unique_ptrensures proper RAII-based lifetime management.test/lint/lint-circular-dependencies.py (3)
24-24: LGTM!The circular dependency path update correctly reflects the header rename from
llmq/quorumstollmq/quorumsman.
30-30: LGTM!Consistent with the header refactoring.
38-38: LGTM!Consistent with the header refactoring.
src/masternode/active/context.h (4)
8-8: LGTM!The
llmq/options.hinclude provides theQvvecSyncModeMaptype needed in the constructor signature.
37-37: LGTM!Forward declaration for
QuorumParticipantis correctly placed within thellmqnamespace.
56-57: LGTM!The expanded constructor signature properly includes
sync_map,db_params,quorums_recovery, andquorums_watchparameters needed forQuorumParticipantinitialization.
75-75: LGTM!The
qman_handlerpublic member follows the same pattern as other similar members (gov_signer,dkgdbgman,qdkgsman, etc.) in this context. Usingstd::unique_ptrensures proper lifetime management.src/llmq/context.cpp (3)
11-11: LGTM!The include update from
llmq/quorums.htollmq/quorumsman.haligns with the header refactoring.
16-24: LGTM!The simplified constructor correctly removes masternode-specific parameters (
mn_activeman,sync_map,quorums_recovery,quorums_watch) that are now handled byQuorumParticipantinActiveContext. TheCQuorumManagerinitialization is appropriately streamlined to only include dependencies needed for its core quorum data management functionality.
39-47: LGTM!The removal of
qman->Start()andqman->Stop()calls is correct. Lifecycle management for quorum-related threads is now handled byQuorumObserver::Start/Stop()(for observer mode) andQuorumParticipant::Start/Stop()(for masternode mode) in their respective contexts, aligning with the PR's separation of observer and participant logic.src/llmq/quorums.h (5)
14-22: LGTM!The includes are properly organized. The
<set>include was correctly added to supportstd::set<uint256>used inDataCleanupHelper. This addresses the previous review feedback.
26-31: LGTM!Forward declarations for
CQuorum,CQuorumManager,QuorumObserver, andQuorumParticipantare properly placed to minimize header dependencies.
33-38: LGTM!The public API declarations for
DB_QUORUM_SK_SHARE,DB_QUORUM_QUORUM_VVEC,MakeQuorumKey, andDataCleanupHelperare appropriately exposed in this header. These are used across observer and participant contexts for quorum data management.
162-164: LGTM!Adding
QuorumObserverandQuorumParticipantas friends ofCQuorumis appropriate since they need access to private members for quorum data recovery and management operations (e.g.,WriteContributions,ReadContributions,SetVerificationVector).
194-195: All call sites correctly updated withprotx_hashparameter.Verification confirms both
SetSecretKeySharecall sites (src/active/quorums.cpp:84 and :160) properly passprotx_hashdirectly viam_mn_activeman.GetProTxHash(). The signature decoupling fromCActiveMasternodeManageris complete and correct.src/masternode/active/context.cpp (8)
7-7: LGTM!The include for
active/quorums.hprovides theQuorumParticipantclass definition needed forqman_handler.
18-18: LGTM!The include update from
llmq/quorums.htollmq/quorumsman.haligns with the header refactoring.
26-27: LGTM!The expanded constructor parameters (
sync_map,db_params,quorums_recovery,quorums_watch) provide the dependencies needed forQuorumParticipantinitialization.
39-41: LGTM!The
QuorumParticipantis properly constructed with all required dependencies. The initialization order places it afterehf_sighandlerand beforecl_signer/is_signer, which is appropriate given it doesn't depend on those signers.
50-50: LGTM!The
ConnectManagerscall correctly registers bothqman_handler(QuorumParticipant) andqdkgsmanwith the quorum manager, enabling coordinated lifecycle and notification handling.
55-55: LGTM!The
DisconnectManagerscall properly cleans up the manager connections during destruction.
67-67: LGTM!Starting
qman_handlerfirst ensures the quorum participant's threads are running before dependent components (qdkgsman, shareman, etc.) begin their work.
82-82: LGTM!Stopping
qman_handlerlast (after qdkgsman) follows proper shutdown order—dependent components are stopped before the quorum participant threads are terminated.src/llmq/observer/context.cpp (2)
45-52: LGTM!The
UpdatedBlockTipcorrectly propagates the block tip update to bothqdkgsmanandqman_handler, maintaining the expected notification order. The early returns for IBD and no-change cases are appropriate.
30-43: No action needed—destructor follows established patterns in the codebase.The current implementation is consistent with other context destructors in the codebase (LLMQContext, ActiveContext), which call specific disconnect/cleanup methods rather than their full Stop() methods during destruction. The caller is responsible for invoking Stop() if needed before the destructor runs. No changes required.
Likely an incorrect or invalid review comment.
src/node/chainstate.cpp (2)
91-93: LGTM!The simplified call to
DashChainstateSetupcorrectly removes thesync_map,quorums_recovery, andquorums_watchparameters, aligning with the refactoring that moves these concerns to the observer/participant layer.
244-246: LGTM!The streamlined
LLMQContextconstruction with reduced parameters is cleaner and matches the updated function signature inDashChainstateSetup.src/llmq/observer/quorums.cpp (3)
165-270: DataRecoveryThread is well-structured with proper interrupt handling.The thread correctly:
- Waits for blockchain sync before proceeding
- Uses
quorumThreadInterruptfor graceful shutdown- Applies load balancing via
start_offset- Handles request timeouts and retries
- Uses atomic flag
fQuorumDataRecoveryThreadRunningto prevent duplicate threadsThe logic for requesting data from quorum members and handling various response statuses is sound.
319-372: Cleanup thread runs at specific block heights to avoid DKG interference.The choice of
height % 576 == 58is well-documented in the comments. The implementation correctly:
- Uses a timer for performance monitoring
- Runs cleanup in the worker pool to avoid blocking
- Respects thread interrupts
- Uses proper locking for the cleanup cache
46-59: The original review comment's concern about guarding against double-start or stop-before-start is incorrect. Thectpl::thread_pool::stop(true)method has built-in guards that make it safe to call multiple times:
- Internal idempotency check:
ctpl::thread_pool::stop(true)checksif (this->isDone || this->isStop) return;before proceeding, preventing re-entry on subsequent calls.- Safe on unstarted pool: If
Start()was never called,workerPool.resize()simply resizes to 0 threads, andstop(true)safely handles an empty thread list via a guardedforloop withjoinable()checks.- Consistent with codebase patterns: Other components (e.g.,
CSigSharesManager) use identical patterns without additional guards, relying on ctpl's internal safety mechanisms.No changes needed to the code.
src/active/quorums.h (1)
37-81: Well-designed class hierarchy with clear separation of concerns.
QuorumParticipantcorrectly:
- Derives from
QuorumObserverwithfinalspecifier- Overrides the necessary virtual methods for masternode participation
- Keeps masternode-specific state (
m_bls_worker,m_mn_activeman,m_quorums_watch) as private members- Documents the purpose of
GetQuorumRecoveryStartOffsetin the commentThe deleted default and copy constructors enforce proper initialization.
src/active/quorums.cpp (3)
41-80: CheckQuorumConnections correctly adds inter-quorum connections for InstantSend.The logic for
watchOtherISQuorumsensures that masternodes participating in InstantSend quorums maintain connections to other IS quorums they're not members of. This is important for efficient message relay. The use ofCalcDeterministicWatchConnectionsprovides deterministic peer selection.
109-167: ProcessEncryptedContribs correctly handles encryption/decryption for masternodes.For
QGETDATA:
- Validates membership before providing encrypted contributions
- Properly retrieves and serializes encrypted contributions
For
QDATA:
- Validates verification vector availability
- Decrypts contributions using the active masternode's keys
- Aggregates secret keys and sets the quorum's secret key share
The error handling with
MisbehavingErrorscoring is appropriate.
196-230: TriggerQuorumDataRecoveryThreads has proper recovery logic for masternodes.The method correctly:
- Distinguishes between quorums where the masternode is a valid member vs. observer
- Requests both verification vector and encrypted contributions for member quorums
- Falls back to
TryStartVvecSyncThreadfor non-member quorumsNote: The
std::move(pQuorum)on lines 220 and 226 is intentional to transfer ownership to the spawned threads, and the loop variable is not used after the move.src/llmq/observer/quorums.h (2)
46-64: QuorumHandlerParent provides a clean abstraction for handler callbacks.The interface correctly declares pure virtual methods that
CQuorumManagerneeds to call on the handler. This enables the observer/participant pattern while maintaining loose coupling.
66-127: QuorumObserver base class is well-designed for the observer pattern.Key design decisions:
- Protected members allow derived classes direct access to dependencies
- Mutable members (
cs_cleanup,cleanupQuorumsCache,workerPool,quorumThreadInterrupt) correctly support const methods that modify cache/thread state- Virtual methods at appropriate visibility levels (public for API, protected for customization)
- Thread pool and interrupt mechanism enable background processing
The
m_sync_mapis stored by value (copy) which is appropriate since it's configuration that shouldn't change.src/llmq/quorumsman.h (3)
64-68: Friend declarations enable tight integration with observer/participant.The
friend classdeclarations forQuorumObserverandQuorumParticipantallow these classes to access private members likedband caches. While this creates coupling, it's intentional for this refactoring where the observer/participant are tightly integrated components of the quorum management system.
86-103: Multiple mutexes with clear thread-safety annotations.The class uses fine-grained locking with separate mutexes for different concerns:
cs_dbfor database accesscs_data_requestsfor request trackingcs_map_quorumsfor quorum cachecs_scan_quorumsfor scan cachecs_quorumBaseBlockIndexCachefor block index cachem_cache_csfor warming queueThe TODO on line 89 about merging
cs_map_quorumsandcs_scan_quorumsis noted for future cleanup.
113-125: ConnectManagers/DisconnectManagers provide safe lifecycle management.The
ConnectManagersmethod uses assertions to prevent double initialization, which is appropriate for catching programmer errors during development. Based on learnings, this fail-fast approach is preferred for invariant enforcement.src/llmq/quorumsman.cpp (3)
31-46: Constructor properly initializes cache, starts warming thread, and migrates legacy data.The initialization sequence is correct:
- Initialize member references and create database
- Initialize quorums cache
- Reset interrupt and start cache warming thread
- Migrate old quorum data from EvoDB
The use of
util::TraceThreadenables proper thread naming for debugging.
48-54: Destructor correctly joins the cache warming thread.The
joinable()check beforejoin()is good practice, and callingm_cache_interrupt()before join ensures the thread exits promptly.
675-751: Quorum selection and signature verification functions are correctly implemented.
SelectQuorumForSigning:
- Handles both rotation and non-rotation quorum types
- Uses deterministic selection based on
selectionHash- Properly handles the sign offset for chain tip verification
VerifyRecoveredSig:
- Correctly constructs the sign hash
- Uses
VerifyInsecurewhich is appropriate for recovered signatures where the public key is already validated by the quorum
Additional Information
Depends on refactor(rpc): add watch-only/masternode mode restrictions for
quorum dkg{info,status}, moveCDKGDebugManagerto{Active,Observer}Context#7062Dependency for refactor: extract
CActiveMasternodeManagerfromLLMQContext(3/n, DKG session isolation,ActiveContextconsolidation) #7065Reviewers are encouraged to scrutinise the
CheckQuorumConnectionsMn()andCheckQuorumConnectionsWatchOnly()split as there is a divergence in behaviour beyond simple access to masternode parameters. So far testing has indicated it's fine but additional consideration may be warranted.ProcessEncryptedContribs()has the argumentvStream, which is written to inQGETDATAand read from inQDATA. As the function signature is non-constbut theQDATAis a read-only branch, we have to useconst_castto be able to callProcessEncryptedContribs(). It has been noted as a code comment.To enforce the split between masternode mode (which can participate in quorums and seek quorum data) and watch-only mode (which can only seek quorum data), threading logic is split between
StartDataRecoveryThread()andStartVvecSyncThread(), they both call the same underlyingDataRecoveryThread()but one has access to masternode-specific parameters and the other does not.This becomes relevant as the entities are split out and the access to specific parameters are enforced by the relevant class members outright not existing.
TryStartVvecSyncThread()as it will not start threads if no data is actually needed, callingStartVvecSyncThread()directly bypasses this check.CQuorumManagerexposes bothIsWatching()andIsMasternode()to allow P2P code and interfaces to query the node's state (this is most relevant inPeerManagerwhich can trivially detect masternode mode but not watch-only status).The
CQuorumManagercache warmer was one of the tasks allocated to the common worker pool, as the rest of the activities are managed by the observer context, rather than keeping the worker pool in the quorum manager and then exposing it through the interface for the sake of one task, the worker pool has been moved to the observer context and we have a regular thread for the quorum manager instead.Breaking Changes
None expected.
Checklist