-
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
WalkthroughSplits and reorganizes LLMQ/quorum responsibilities: introduces llmq::CQuorumManager (llmq/quorumsman.{h,cpp}) for mined-quorum lifecycle, caching, data exchange and signing; adds llmq::QuorumObserver (llmq/observer/quorums.{h,cpp}) for block-tip monitoring, connection maintenance, data-recovery and cleanup; and adds llmq::QuorumParticipant (active/quorums.{h,cpp}) for active masternode participation and contribution handling. Moves quorum helpers/constants into llmq/quorums.h, updates many includes to llmq/quorumsman.h, and adjusts constructor signatures and startup/shutdown wiring across ActiveContext, ObserverContext and LLMQContext. Build exports updated for new headers/sources. Sequence Diagram(s)mermaid Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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. |
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
| // file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
|
|
||
| #include <llmq/quorums.h> | ||
| #include <active/quorums.h> |
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.
please rename to activemn or something that indicates what is active
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.
As discussed earlier, the suggestion of active_mn or something to that effect can be considered down the road but IMO shouldn't be a blocker now.
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`.
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
♻️ Duplicate comments (1)
src/active/quorums.cpp (1)
1-5: Consider renaming directory to clarify masternode context.A previous reviewer suggested renaming from
activetoactivemnor similar to indicate that this relates to active masternode functionality, not just generic "active" behavior.
🧹 Nitpick comments (2)
src/init.cpp (1)
1415-1422: Early validation and warning for-llmq-qvvec-syncvs-llmq-data-recoveryis well scopedCatching
std::invalid_argumentfromGetEnabledQuorumVvecSyncEntries(args)and converting it toInitError, plus warning when qvvec sync is configured but recovery is disabled, gives clear feedback while leaving behavior unchanged if the user insists on this combo. One small optional refinement is that-llmq-qvvec-syncis parsed twice (here and when buildingsync_mapinAppInitMain); if this ever becomes more complex, you could cache the parsed map or thread it viaNodeContext, but current cost is negligible.src/llmq/observer/quorums.cpp (1)
318-371: Cleanup thread scheduling avoids DKG windows.The logic at line 327 (
pIndex->nHeight % 576 != 58) is designed to avoid CPU-heavy cleanup during DKG mining windows. The comment explains the rationale well. This is a thoughtful optimization.Note: The magic number
576appears to be derived from quorum cycle calculations (24 blocks × 24 cycles). Consider defining this as a named constant for clarity in future refactoring.
📜 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 due to trivial changes (1)
- src/evo/mnhftx.cpp
🚧 Files skipped from review as they are similar to previous changes (13)
- src/chainlock/chainlock.cpp
- src/chainlock/chainlock.h
- src/test/util/setup_common.cpp
- src/instantsend/signing.cpp
- src/evo/cbtx.cpp
- test/util/data/non-backported.txt
- src/instantsend/net_instantsend.cpp
- src/Makefile.am
- src/node/chainstate.cpp
- test/lint/lint-circular-dependencies.py
- src/evo/assetlocktx.cpp
- src/net_processing.cpp
- src/evo/specialtxman.cpp
🧰 Additional context used
📓 Path-based instructions (7)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/masternode/active/notificationinterface.cppsrc/dsnotificationinterface.cppsrc/init.cppsrc/msg_result.hsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/masternode/active/context.hsrc/rpc/quorums.cppsrc/llmq/signing_shares.cppsrc/active/quorums.cppsrc/llmq/observer/context.hsrc/llmq/signing.cppsrc/llmq/observer/quorums.cppsrc/llmq/context.hsrc/llmq/ehf_signals.cppsrc/evo/smldiff.cppsrc/active/quorums.hsrc/masternode/active/context.cppsrc/llmq/observer/context.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.cppsrc/llmq/options.hsrc/llmq/observer/quorums.hsrc/llmq/context.cpp
src/{masternode,evo}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/masternode/active/notificationinterface.cppsrc/masternode/active/context.hsrc/evo/smldiff.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/masternode/active/notificationinterface.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/masternode/active/context.hsrc/llmq/signing_shares.cppsrc/llmq/observer/context.hsrc/llmq/signing.cppsrc/llmq/observer/quorums.cppsrc/llmq/context.hsrc/llmq/ehf_signals.cppsrc/evo/smldiff.cppsrc/masternode/active/context.cppsrc/llmq/observer/context.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.cppsrc/llmq/options.hsrc/llmq/observer/quorums.hsrc/llmq/context.cpp
src/{masternode,llmq}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
BLS integration must be used for cryptographic foundation of advanced masternode features
Files:
src/masternode/active/notificationinterface.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/masternode/active/context.hsrc/llmq/signing_shares.cppsrc/llmq/observer/context.hsrc/llmq/signing.cppsrc/llmq/observer/quorums.cppsrc/llmq/context.hsrc/llmq/ehf_signals.cppsrc/masternode/active/context.cppsrc/llmq/observer/context.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.cppsrc/llmq/options.hsrc/llmq/observer/quorums.hsrc/llmq/context.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/masternode/active/notificationinterface.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/masternode/active/context.hsrc/llmq/signing_shares.cppsrc/llmq/observer/context.hsrc/llmq/signing.cppsrc/llmq/observer/quorums.cppsrc/llmq/context.hsrc/llmq/ehf_signals.cppsrc/evo/smldiff.cppsrc/masternode/active/context.cppsrc/llmq/observer/context.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.cppsrc/llmq/options.hsrc/llmq/observer/quorums.hsrc/llmq/context.cpp
src/llmq/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
src/llmq/**/*.{cpp,h}: LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Files:
src/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/signing_shares.cppsrc/llmq/observer/context.hsrc/llmq/signing.cppsrc/llmq/observer/quorums.cppsrc/llmq/context.hsrc/llmq/ehf_signals.cppsrc/llmq/observer/context.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.cppsrc/llmq/options.hsrc/llmq/observer/quorums.hsrc/llmq/context.cpp
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/smldiff.cpp
🧠 Learnings (33)
📓 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/llmq/**/*.{cpp,h} : LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
Learnt from: kwvg
Repo: dashpay/dash PR: 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} : ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/evodb/**/*.{cpp,h} : Evolution Database (CEvoDb) must handle masternode snapshots, quorum state, governance objects with efficient differential updates for masternode lists
Learnt from: 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
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/{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/masternode/active/notificationinterface.cppsrc/evo/smldiff.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/masternode/active/notificationinterface.cppsrc/active/quorums.cppsrc/evo/smldiff.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/masternode/active/notificationinterface.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/masternode/active/context.hsrc/rpc/quorums.cppsrc/llmq/signing_shares.cppsrc/active/quorums.cppsrc/llmq/signing.cppsrc/llmq/ehf_signals.cppsrc/evo/smldiff.cppsrc/active/quorums.hsrc/masternode/active/context.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} : InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
Applied to files:
src/masternode/active/notificationinterface.cppsrc/dsnotificationinterface.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/masternode/active/context.hsrc/rpc/quorums.cppsrc/llmq/signing_shares.cppsrc/active/quorums.cppsrc/llmq/observer/context.hsrc/llmq/signing.cppsrc/llmq/observer/quorums.cppsrc/llmq/context.hsrc/llmq/ehf_signals.cppsrc/evo/smldiff.cppsrc/active/quorums.hsrc/masternode/active/context.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.cppsrc/llmq/options.hsrc/llmq/observer/quorums.hsrc/llmq/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/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/masternode/active/notificationinterface.cppsrc/dsnotificationinterface.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/masternode/active/context.hsrc/rpc/quorums.cppsrc/llmq/signing_shares.cppsrc/active/quorums.cppsrc/llmq/observer/context.hsrc/llmq/signing.cppsrc/llmq/observer/quorums.cppsrc/llmq/context.hsrc/llmq/ehf_signals.cppsrc/evo/smldiff.cppsrc/active/quorums.hsrc/masternode/active/context.cppsrc/llmq/observer/context.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.cppsrc/llmq/options.hsrc/llmq/observer/quorums.hsrc/llmq/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/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/masternode/active/notificationinterface.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/ehf_signals.cppsrc/evo/smldiff.cppsrc/masternode/active/context.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/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/masternode/active/context.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/masternode/active/notificationinterface.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/signing_shares.cppsrc/active/quorums.cppsrc/llmq/signing.cppsrc/llmq/ehf_signals.cppsrc/active/quorums.hsrc/masternode/active/context.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.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/dsnotificationinterface.cppsrc/init.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/masternode/active/context.hsrc/rpc/quorums.cppsrc/llmq/signing_shares.cppsrc/active/quorums.cppsrc/llmq/observer/context.hsrc/llmq/signing.cppsrc/llmq/observer/quorums.cppsrc/llmq/context.hsrc/llmq/ehf_signals.cppsrc/evo/smldiff.cppsrc/active/quorums.hsrc/masternode/active/context.cppsrc/llmq/observer/context.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.cppsrc/llmq/observer/quorums.hsrc/llmq/context.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/dsnotificationinterface.cppsrc/llmq/quorums.hsrc/masternode/active/context.hsrc/llmq/observer/context.hsrc/llmq/context.hsrc/masternode/active/context.cppsrc/llmq/observer/context.cppsrc/llmq/quorumsman.hsrc/llmq/context.cpp
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
Applied to files:
src/dsnotificationinterface.cppsrc/llmq/quorums.hsrc/masternode/active/context.hsrc/active/quorums.cppsrc/llmq/observer/context.hsrc/llmq/context.hsrc/active/quorums.hsrc/masternode/active/context.cppsrc/llmq/observer/context.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.cppsrc/llmq/observer/quorums.hsrc/llmq/context.cpp
📚 Learning: 2025-10-03T11:20:37.475Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6838
File: src/active/context.cpp:29-33
Timestamp: 2025-10-03T11:20:37.475Z
Learning: In Dash codebase, NodeContext (src/node/context.h) serves only as a container with trivial c/d-tors; member lifetime is explicitly managed via reset() calls in the shutdown sequence (src/init.cpp), not by declaration order. For example, active_ctx.reset() is called before DashChainstateSetupClose handles llmq_ctx teardown, ensuring proper destruction order regardless of declaration order in the struct.
Applied to files:
src/init.cppsrc/masternode/active/context.hsrc/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/init.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/evo/smldiff.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/init.cpp
📚 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-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/msg_result.hsrc/llmq/quorums.hsrc/masternode/active/context.hsrc/llmq/observer/context.hsrc/llmq/context.hsrc/active/quorums.hsrc/llmq/quorumsman.hsrc/llmq/options.hsrc/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} : ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Applied to files:
src/llmq/quorums.cppsrc/llmq/quorums.hsrc/masternode/active/context.hsrc/rpc/quorums.cppsrc/llmq/signing_shares.cppsrc/active/quorums.cppsrc/llmq/observer/context.hsrc/llmq/signing.cppsrc/llmq/observer/quorums.cppsrc/llmq/context.hsrc/llmq/ehf_signals.cppsrc/evo/smldiff.cppsrc/active/quorums.hsrc/masternode/active/context.cppsrc/llmq/quorumsman.hsrc/llmq/quorumsman.cppsrc/llmq/observer/quorums.hsrc/llmq/context.cpp
📚 Learning: 2025-08-19T15:08:00.835Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/commitment.cpp:54-60
Timestamp: 2025-08-19T15:08:00.835Z
Learning: In Dash Core, llmq_params.size == 1 is used only for regtest environments, not on public networks, which significantly reduces the risk profile of code paths specific to single-member quorums.
Applied to files:
src/llmq/quorums.cppsrc/llmq/quorums.hsrc/active/quorums.cppsrc/llmq/signing.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/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
src/llmq/quorums.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/llmq/quorums.hsrc/active/quorums.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/llmq/quorums.hsrc/active/quorums.cpp
📚 Learning: 2025-11-04T18:24:27.241Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Applied to files:
src/llmq/quorums.hsrc/active/quorums.cpp
📚 Learning: 2025-07-29T14:33:01.040Z
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.
Applied to files:
src/llmq/quorums.h
📚 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/llmq/quorums.hsrc/active/quorums.cpp
📚 Learning: 2025-02-06T14:34:30.466Z
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.
Applied to files:
src/llmq/quorums.h
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/llmq/quorums.hsrc/active/quorums.cpp
📚 Learning: 2025-10-13T12:37:12.357Z
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Applied to files:
src/llmq/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,evo,coinjoin,governance}/**/*.{cpp,h} : Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
Applied to files:
src/active/quorums.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/**/*.{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/smldiff.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
🧬 Code graph analysis (10)
src/masternode/active/notificationinterface.cpp (1)
src/net_processing.cpp (1)
pindexNew(607-607)
src/init.cpp (4)
src/interfaces/node.h (2)
node(48-50)node(481-481)src/init.h (1)
node(23-25)src/llmq/options.h (1)
DEFAULT_WATCH_QUORUMS(37-51)src/llmq/options.cpp (2)
GetEnabledQuorumVvecSyncEntries(59-104)GetEnabledQuorumVvecSyncEntries(59-59)
src/llmq/quorums.h (5)
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/types.h (1)
llmq(10-17)
src/masternode/active/context.h (1)
src/active/quorums.cpp (2)
QuorumParticipant(27-37)QuorumParticipant(39-39)
src/active/quorums.cpp (3)
src/llmq/dkgsessionmgr.h (1)
m_quorums_watch(60-126)src/llmq/quorumsman.cpp (4)
IsMasternode(309-315)IsMasternode(309-309)IsWatching(317-323)IsWatching(317-317)src/llmq/observer/quorums.h (1)
m_quorums_recovery(76-127)
src/llmq/observer/context.h (4)
src/llmq/observer/quorums.h (1)
llmq(34-37)src/net_processing.h (2)
llmq(34-36)bool(93-93)src/node/context.h (1)
llmq(38-40)src/llmq/observer/context.cpp (2)
ObserverContext(13-28)ObserverContext(30-33)
src/llmq/observer/quorums.cpp (4)
src/llmq/quorumsman.cpp (4)
IsMasternode(309-315)IsMasternode(309-309)IsWatching(317-323)IsWatching(317-317)src/llmq/quorums.h (1)
nDataMask(88-88)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.cpp (3)
src/llmq/observer/quorums.cpp (4)
Start(46-52)Start(46-46)Stop(54-59)Stop(54-54)src/llmq/context.cpp (4)
Start(39-42)Start(39-39)Stop(44-47)Stop(44-44)src/net_processing.cpp (1)
pindexNew(607-607)
src/llmq/quorumsman.h (4)
src/llmq/observer/quorums.h (1)
llmq(34-37)src/llmq/quorumsman.cpp (8)
CQuorumManager(32-46)CQuorumManager(48-54)HasQuorum(145-148)HasQuorum(145-145)SelectQuorumForSigning(670-730)SelectQuorumForSigning(670-671)VerifyRecoveredSig(732-746)VerifyRecoveredSig(732-734)src/llmq/params.h (1)
LLMQType(14-125)src/llmq/quorums.cpp (1)
CQuorum(101-103)
src/llmq/observer/quorums.h (3)
src/llmq/observer/context.h (2)
llmq(22-29)llmq(34-64)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 (6)
Start(35-38)Start(35-35)Stop(40-43)Stop(40-40)UpdatedBlockTip(45-52)UpdatedBlockTip(45-45)
🪛 Cppcheck (2.19.0)
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/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/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)
src/llmq/context.cpp
[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: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (55)
src/llmq/options.h (1)
24-28: Explicitint8_tunderlying type forQvvecSyncModeis safe and clearerEnumerators stay within [-1,1], and existing parsing/usage via
int32_tcasts remains valid, so this narrows the type without changing behavior while making ABI/size explicit.src/llmq/context.h (1)
39-64: Constructor reshaping keeps LLMQContext invariants intactThe reduced, ordered parameter list still provides everything needed to initialize
bls_worker,qman, and the other managers, preserving the “all members valid when LLMQContext is non-null” guarantee relied on elsewhere (e.g.net_processing). Based on learnings, this fits the existing initialization model without introducing new lifetime risks.src/init.cpp (2)
295-299: ObserverContext shutdown/teardown order looks correctStopping
observer_ctxalongsideactive_ctxandllmq_ctx, unregistering it from validation, and then resetting both contexts beforeDashChainstateSetupClosereceivesnode.llmq_ctxkeeps the lifetimes aligned with NodeContext teardown patterns from earlier PRs, avoiding use-after-free on LLMQ components. Based on learnings, this matches the intended shutdown sequencing.Also applies to: 352-373, 398-410
2209-2212: Newsync_map/ (active vs observer) wiring is consistent with the masternode/watch-only splitDeriving
quorums_recovery,quorums_watch, and theQvvecSyncModeMap sync_maponce and passing them intoActiveContext(for masternode mode) orllmq::ObserverContext(for watch-only-watchquorumsnodes) cleanly centralizes quorum vvec/recovery configuration. Startingllmq_ctx, then peerman handlers, thenactive_ctx(if present), and finallyobserver_ctxin Step 10a, with matchingStop()/reset()paths inPrepareShutdown, keeps lifetimes and thread startup/shutdown aligned and avoids extra quorum threads on nodes that neither run a masternode nor watch quorums.Also applies to: 2218-2227, 2231-2233, 2338-2342
src/llmq/quorums.h (1)
14-22: Quorum header surface refactor (helpers, friends, andSetSecretKeyShare) is coherentLocalizing the core includes plus
<set>/<string>/<vector>, adding theDB_QUORUM_SK_SHARE/DB_QUORUM_QUORUM_VVECexterns, and exposingMakeQuorumKey/DataCleanupHelper(CDBWrapper&, const std::set<uint256>&, ...)gives a clean, minimal API and avoids the previous by-valuestd::setcopies. ExtendingCQuorum’s friends toQuorumObserverandQuorumParticipantand adding theprotx_hashparameter toSetSecretKeySharefits the new observer/participant split; assuming the definition and all call sites inllmq/quorums.cppand the new managers were updated accordingly, there are no correctness or consensus-behavior concerns here. Based on learnings, this stays within refactor scope.Also applies to: 26-39, 161-164, 194-195
src/rpc/quorums.cpp (1)
26-26: Includingllmq/quorumsman.hhere matches the new CQuorumManager APISwitching the include from
llmq/quorums.htollmq/quorumsman.his appropriate given this file’s heavy use ofllmq_ctx.qman. As long asquorumsman.h(directly or transitively) provides the fullllmq::CQuorumdefinition needed byBuildQuorumInfoand related RPCs, this keeps the dependency surface aligned with the refactor without altering behavior.src/llmq/signing_shares.cpp (1)
9-9: LGTM! Clean header migration.The include change from
llmq/quorums.htollmq/quorumsman.haligns with the PR's broader refactoring to introduce the new quorum management architecture. No functional changes in this file.src/llmq/ehf_signals.cpp (1)
16-16: LGTM! Header migration is consistent.The switch to
llmq/quorumsman.hmaintains compatibility with the existingCQuorumManagerusage throughout this file.src/evo/smldiff.cpp (1)
16-16: LGTM! Straightforward header update.The header migration maintains compatibility with the
CQuorumManagerusage in this file's quorum diff building logic.src/llmq/signing.cpp (1)
9-9: LGTM! Header migration complete.The switch to
llmq/quorumsman.his consistent with the PR's architectural refactoring while maintaining all existingCQuorumManagerfunctionality used throughout this file.src/dsnotificationinterface.cpp (1)
20-20: Include change is correct; UpdatedBlockTip handling properly delegated to observer/handler pathway.The include migration to
llmq/quorumsman.his appropriate. TheUpdatedBlockTipfunctionality previously called directly onqmanis now properly routed through theqman_handler(QuorumObserver) via theObserverContext::UpdatedBlockTipmethod. The observer implementation handles all necessary quorum operations: checking quorum connections, cleaning up expired data requests, triggering quorum data recovery, and managing old quorum data cleanup. This architectural refactoring successfully delegates the responsibility without losing functionality.src/msg_result.h (1)
70-71: LGTM!The
empty()method is well-implemented, correctly checking all relevant state members. The naming follows STL conventions and the const-correctness is appropriate.src/masternode/active/notificationinterface.cpp (2)
7-7: LGTM!Include added correctly for the new
QuorumParticipanthandler.
33-33: LGTM!The
qman_handler->UpdatedBlockTip()call follows the same pattern as other handlers in this method and correctly forwards the block tip update to the new quorum participant component.src/llmq/observer/context.h (3)
8-9: LGTM!Include for
llmq/options.his needed for theQvvecSyncModeMaptype used in the constructor.
43-51: LGTM!The expanded constructor signature and added
Start()/Stop()lifecycle methods align with the component lifecycle patterns used elsewhere in the codebase. The dependency injection approach is clean and testable.
60-62: LGTM!Good encapsulation moving
qman_handlerto the private section while keepingdkgdbgmanandqdkgsmanpublic for external access. This appropriately hides the observer implementation details.src/masternode/active/context.h (3)
8-9: LGTM!Include for
llmq/options.hprovides theQvvecSyncModeMaptype needed in the constructor signature.
52-57: LGTM!The constructor extension with
sync_map,db_params,quorums_recovery, andquorums_watchparameters properly wires the dependencies for the newQuorumParticipantcomponent.
75-75: LGTM!The
qman_handlermember follows the same pattern as other public handlers (gov_signer,qdkgsman,shareman,ehf_sighandler) withconst std::unique_ptrfor clear ownership semantics.src/llmq/context.cpp (3)
11-11: LGTM!Header change from
llmq/quorums.htollmq/quorumsman.hreflects the architectural split whereCQuorumManageris now defined in the new header.
16-24: LGTM!The simplified constructor reflects the observer/watch-only mode: masternode-specific parameters (
mn_activeman,sync_map,quorums_recovery,quorums_watch) have been moved toActiveContextandObserverContext. The coreLLMQContextnow only handles shared infrastructure components.
39-47: LGTM!
Start()andStop()now only manageclhandlerlifecycle. The quorum manager start/stop is appropriately delegated to the observer/participant handlers that own those responsibilities.src/masternode/active/context.cpp (6)
7-7: LGTM!Include for
active/quorums.hprovides theQuorumParticipantclass definition.
39-41: LGTM!The
QuorumParticipantinitialization properly wires all required dependencies, passing through the sync mode map, recovery flag, and watch flag to configure masternode quorum participation behavior.
48-50: LGTM!The
ConnectManagers()call correctly registers both theqman_handler(QuorumParticipant) andqdkgsmanwith the quorum manager, establishing the multi-manager architecture described in the PR objectives.
53-58: LGTM!The destructor correctly disconnects managers before disconnecting signers, maintaining proper teardown order.
75-83: LGTM!The
Stop()method correctly stopsqman_handlerlast (Line 82), maintaining symmetric ordering withStart(). This ensures dependent components are stopped before the underlying quorum infrastructure.
65-72: The code correctly maintains symmetric start/stop ordering.qman_handler->Start()is called first inStart()andqman_handler->Stop()is called last inStop(). All other components (qdkgsman, shareman, and recovery interfaces) follow proper LIFO cleanup order.src/llmq/quorums.cpp (4)
14-16: LGTM!The database key constants are now properly scoped within the
llmqnamespace, making them accessible for the new modular quorum architecture while maintaining clear ownership.
18-27: LGTM!The
MakeQuorumKeyfunction is now properly scoped in thellmqnamespace and takes aconst CQuorum&reference. The key construction includes quorum type, hash, and all member proTxHashes, providing a deterministic and unique key.
29-74: LGTM!The
DataCleanupHelperfunction is well-implemented:
- Iterates both prefixes (vvec, sk_share) for comprehensive cleanup
- Uses batch operations with 16MB threshold to avoid memory pressure
- Properly resets cursor before optional compaction
- Logging correctly identifies the calling context
One minor note: the function handles an empty
skip_listcorrectly—it will simply delete all entries for the prefixes, which appears to be the intended behavior for full cleanup scenarios.
125-133: LGTM!The updated
SetSecretKeySharesignature with theprotx_hashparameter strengthens validation by ensuring:
- The proTxHash is not null
- The secret key is valid
- The derived public key matches the expected pubkey share for the member index
This prevents storing invalid or mismatched secret key shares.
src/llmq/observer/context.cpp (2)
13-28: LGTM! Constructor wiring is consistent with the multi-manager pattern.The constructor properly initializes the
QuorumObserverand wires it to theCQuorumManagerviaConnectManagers. The parameter forwarding and initialization order are correct.
35-43: Lifecycle delegation looks correct.
Start()andStop()properly delegate toqman_handler, which manages the worker pool lifecycle. The destructor callingDisconnectManagers()ensures proper cleanup ordering.src/llmq/observer/quorums.cpp (4)
41-44: Destructor callsStop()which may be called twice.If
Stop()is called explicitly before destruction, the destructor will call it again. Verify thatStop()is idempotent. Looking at the implementation,workerPool.stop(true)should be safe to call multiple times, but consider adding a guard or documenting this expectation.
46-52: Thread pool sizing is reasonable.Worker count is clamped to
[1, 4]based on half of hardware concurrency. This is a sensible default for background data recovery work that shouldn't monopolize CPU resources.
77-104:GetQuorumsToDeletecorrectly preserves in-progress DKG quorums.The logic properly handles both rotated and non-rotated quorum types, ensuring that quorums involved in ongoing DKG rounds are not deleted. The logging provides good visibility into which quorums are being preserved.
164-269: DataRecoveryThread implementation is thorough but complex.The recovery logic handles:
- Blockchain sync waiting
- Member list building and sorting for deterministic order
- Timeout-based retry with member rotation
- Proper interrupt handling
The complexity is warranted for robust data recovery. The
printLoglambda provides consistent logging throughout.src/active/quorums.cpp (3)
41-80:CheckQuorumConnectionscorrectly handles masternode-specific connection logic.The implementation properly:
- Uses the masternode's own
proTxHashfor connection decisions- Handles inter-quorum connections for InstantSend when the node is a member of related quorums
- Maintains connections for quorums where the node is a participant
The
watchOtherISQuorumslogic ensures masternodes in InstantSend quorums maintain connections to other IS quorums for signing coordination.
87-107: Load balancing offset calculation is deterministic and fair.
GetQuorumRecoveryStartOffsetuses the sorted masternode list to calculate a consistent offset, ensuring that recovery requests are distributed across quorum members evenly. The modulo operation at line 106 ensures the offset stays within valid member bounds.
135-169: Encrypted contribution processing handles decryption securely.The implementation:
- Validates quorum verification vector size before processing
- Checks member index validity
- Decrypts each contribution using the active masternode's keys
- Aggregates secret keys before setting the share
The misbehavior scores (10) for failures are appropriate for protocol violations.
src/active/quorums.h (1)
38-81: Well-structured class hierarchy with clear separation of concerns.
QuorumParticipantproperly extendsQuorumObserverwith masternode-specific behavior:
finalkeyword prevents further derivation- Protected overrides for customizable behavior
- Private helpers for internal implementation details
[[nodiscard]]on message processing methods enforces result handlingThe BLS worker and active masternode manager references are correctly stored for cryptographic operations.
src/llmq/quorumsman.cpp (5)
31-46: Constructor properly initializes caches and starts background thread.The initialization sequence:
- Initializes quorum caches based on consensus parameters
- Resets interrupt flag
- Starts cache warming thread with trace naming
- Migrates legacy DB data
This follows RAII principles with the thread managed by the class lifetime.
48-54: Destructor properly handles thread cleanup.The pattern of checking
joinable(), calling interrupt, thenjoin()is correct for thread shutdown. This prevents hanging on destruction and ensures clean termination.
409-493: QGETDATA message processing is comprehensive and secure.Key security checks:
- Validates sender is masternode or qwatch connection (line 412)
- Rate limiting with misbehavior scoring (lines 444-455)
- Validates LLMQ type exists (line 458)
- Validates block index exists (line 462-464)
- Validates quorum exists (line 467-470)
- Delegates encrypted contribution handling to handler
The
sendQDATAlambda cleanly handles response generation with appropriate error codes.
496-556: QDATA message processing validates request authenticity.The implementation:
- Validates the request was actually sent by this node (line 508-509)
- Prevents duplicate processing (line 511-513)
- Validates response matches request (line 514-516)
- Marks request as processed before handling data
- Validates verification vector before accepting (line 540-544)
- Persists contributions to DB after successful processing
The misbehavior scores are appropriately applied for protocol violations.
670-730: Quorum selection for signing handles rotation and non-rotation correctly.For rotated quorums:
- Uses log2-based selection from
signingActiveQuorumCount- Extracts selection bits from the selection hash
- Finds matching quorum by index
For non-rotated quorums:
- Uses hash-based scoring for deterministic selection
- Sorts by score and returns the best match
Both paths handle empty quorum sets gracefully.
src/llmq/quorumsman.h (4)
64-77: Class design properly separates concerns with handler pattern.The
CQuorumManageruses raw pointers form_handlerandm_qdkgsmanto support late binding viaConnectManagers. Thefrienddeclarations forQuorumObserverandQuorumParticipantprovide controlled access to internals while maintaining encapsulation for other consumers.
113-125:ConnectManagers/DisconnectManagersuse fail-fast assertions.The
assert(m_handler == nullptr)pattern prevents double initialization, catching programmer errors early during development. This is consistent with the codebase's approach to enforcing invariants via assertions.
131-133: Lock annotations document threading contracts clearly.The
EXCLUSIVE_LOCKS_REQUIREDannotations onProcessMessageexplicitly document which locks must not be held by the caller, enabling Clang Thread Safety Analysis to catch potential issues at compile time.
181-192: Free functions and constant provide clean signing API.
SIGN_HEIGHT_OFFSET(8 blocks) ensures signatures remain verifiable by nodes that aren't exactly at chain tip. The free functionsSelectQuorumForSigningandVerifyRecoveredSigprovide a stateless interface for signing operations.src/llmq/observer/quorums.h (3)
46-64:QuorumHandlerParentinterface enables clean dependency inversion.The abstract interface defines the contract that
CQuorumManagerimplements, allowingQuorumObserverto work with the manager without tight coupling. The pure virtual methods cover:
- Data retrieval (
GetEncryptedContributions,ScanQuorums)- Request lifecycle (
IsDataRequestPending,RequestQuorumData,GetDataRequestStatus)- Cleanup operations (
CleanupExpiredDataRequests,CleanupOldQuorumData)This design supports the separation of observer and participant logic cleanly.
66-94:QuorumObserverbase class provides extensible foundation.Protected members give derived classes (
QuorumParticipant) access to essential dependencies while encapsulating implementation details. Themutablequalifiers on thread pool and interrupt are appropriate since they represent internal concurrency control that doesn't affect logical constness.
101-109: Virtual methods provide clear extension points.The virtual methods (
SetQuorumSecretKeyShare,ProcessContribQGETDATA,ProcessContribQDATA,IsMasternode,IsWatching) define the behavioral differences between watch-only and masternode modes. The[[nodiscard]]attribute on processing methods ensures error handling isn't ignored.
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.
concept ACK; overall all changes [except re-ordering initialization in init.cpp] make sense and reasonable for me.
Though, I would disagree with some naming of classes and files as misleading or confusing.
See also some nits.
src/llmq/quorums.cpp
Outdated
| if (auto request_err = request.GetError(); | ||
| request_err != CQuorumDataRequest::Errors::NONE && request_err != CQuorumDataRequest::Errors::UNDEFINED) { | ||
| auto qdata_ret = sendQDATA(request_err, request_limit_exceeded); | ||
| return ret.empty() ? qdata_ret : ret; |
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.
ret.empty() logically is not the same thing as there is no error happen.
prefer to do direct check for any error, rather that empty(); remove empty() from MessageProcessingResult
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.
Error processing is done by sendQDATA itself and checking if ret is empty is simply to decide between whether to choose the earlier result (ret) or the later result (qdata_ret, with the side effect of pushing messages), to know if we have an earlier result, we check if ret is empty, hence the check.
| libbitcoin_node_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BOOST_CPPFLAGS) $(MINIUPNPC_CPPFLAGS) $(NATPMP_CPPFLAGS) $(EVENT_CFLAGS) $(EVENT_PTHREADS_CFLAGS) | ||
| libbitcoin_node_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) | ||
| libbitcoin_node_a_SOURCES = \ | ||
| active/quorums.cpp \ |
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.
-
I'd agree with pasta;
active/is quite unclear naming because it is not clear meaning without knowing details of implementation. Could it be in directorymasternode/active/quorums.cppwhich already exist, or justmasternode/quorums.cpp? -
quorums.cpp now presented in llmq/ and in active/. Humans are not machines, better to rename it to something unique. masternode/qman.cpp ? masternode/quorum_manager.cpp ? llmq/quorum_manager.cpp ? llmq/active_quorums.cpp ?
I think llmq/quorum_manager.cpp or llmq/qman.cpp is a best option, @coderabbitai, any suggestions?
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.
@knst, excellent points on naming clarity! Having multiple quorums.cpp files is indeed confusing. Here are my suggestions:
For the directory structure:
Since there's already an llmq/observer/quorums.cpp containing QuorumObserver, the most consistent approach would be:
llmq/participant/quorums.cpp(or even better,llmq/participant/participant.cpp) for theQuorumParticipantclass
This mirrors the observer/participant pattern cleanly and keeps related LLMQ logic together.
Alternative options:
masternode/active/quorums.cpp- works sinceQuorumParticipantis masternode-specific, and this directory already exists. However, this might blur the LLMQ architectural boundaries.llmq/active/quorums.cpp- clearer than top-levelactive/, but "active" is still somewhat ambiguous.
For filename uniqueness:
To avoid confusion with llmq/quorums.cpp, consider:
participant.cpp- clearest option, especially if placed inllmq/participant/qparticipant.cpp- follows the class name more closelyactive_quorums.cpp- distinguishes it, but less precise than "participant"
My recommendation:
Move active/quorums.{cpp,h} to llmq/participant/participant.{cpp,h}
This provides:
✅ Clear semantic meaning (participant vs observer)
✅ Consistent with existing llmq/observer/ structure
✅ No filename collision
✅ Keeps LLMQ architecture cohesive
Regarding your preference for llmq/quorum_manager.cpp or llmq/qman.cpp - note that there's already llmq/quorumsman.{cpp,h} for CQuorumManager, so the main naming issue is specifically for the active/quorums.cpp file containing QuorumParticipant.
What do you think about the llmq/participant/participant.{cpp,h} approach?
🧠 Learnings used
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features
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
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: 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.
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.
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.
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.
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.
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.
participant is not a good idea, because DIP says nothing about participants, but uses terminology member: https://github.com/dashpay/dips/blob/master/dip-0006.md
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.
@knst, excellent point about aligning with DIP-0006 terminology! You're absolutely right that we should use the official specification's language.
Given that consideration, here are refined suggestions:
Option 1: masternode/active/quorums.cpp ✅ (Recommended)
- Uses the existing
masternode/active/directory structure - Semantically correct: this is active masternode functionality (requires masternode keys to participate in quorums)
- The
masternode/active/context.{cpp,h}already exists in this directory, making this a natural home - Clear distinction from observer (watch-only) functionality
Option 2: llmq/active/quorums.cpp
- Keeps LLMQ logic together under
llmq/ - "Active" distinguishes from observer without terminology conflicts
- Creates new subdirectory structure
For the filename itself:
Since masternode/active/ would be the directory, having quorums.cpp there is actually fine because:
- Full path
masternode/active/quorums.cppis distinct fromllmq/quorums.cpp - The directory context provides the differentiation
- Describes what the file manages (quorum participation for active masternodes)
My recommendation: Keep it as masternode/active/quorums.{cpp,h} since that directory already exists and semantically this IS active masternode functionality for quorum participation. The full path provides clarity without conflicting with llmq/quorums.cpp.
Alternatively, if you prefer it under llmq/, then llmq/active/quorums.{cpp,h} would work.
What's your preference between these two options?
🧠 Learnings used
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features
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
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: 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: 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: 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: 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.
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
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.
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.
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.
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 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.
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.
quorums.cpp there is actually fine because:
no, because gdb command break quorums.cpp:XXX will not be determined anymore; using IDE / file search is also will be less conventient because two files have same name.
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.
@knst, you're absolutely right—ambiguous filenames create real friction with debugging tools and IDE navigation. That's a valid practical concern that trumps "full path provides context" arguments.
Given that we need:
- Unique filename (for tooling)
- DIP-aligned terminology (uses "member" not "participant")
- Clear purpose (active masternode quorum functionality)
Here are concrete suggestions with unique names:
Recommended option:
masternode/active/quorum_member.{cpp,h}
Rationale:
- ✅ Unique filename (no conflicts)
- ✅ Uses DIP-0006 terminology ("member")
- ✅ Natural home (existing
masternode/active/directory) - ✅ Clear purpose: active masternode acting as quorum member
- ✅ Unambiguous for
break quorum_member.cpp:XXX
Alternative options:
masternode/active/member.{cpp,h}- shortest, though slightly less explicitllmq/active_member.{cpp,h}- if you prefer keeping all LLMQ logic underllmq/llmq/member.{cpp,h}- even shorter if context is clear
The class name QuorumParticipant would remain unchanged (it's descriptive and internal), but the file would use DIP-aligned terminology.
My recommendation: Go with masternode/active/quorum_member.{cpp,h} since it's unambiguous, uses proper terminology, and fits the existing structure.
What do you think?
🧠 Learnings used
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features
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
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: 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: 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: 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: 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.
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.
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.
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 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.
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.
-
Please see dash#7066, the eventual structure will fold
masternode/activeintoactive(source).It was decided against placing quorum logic into
masternode/activeas it is decidedly LLMQ code but there isn't enough of it to justifyllmq/active, so everything, regardless of subsystem is put intoactiveas they all share two things in common- They're only used in masternode-mode
- They will be excluded from
dash-chainstate
-
Participation != membership, the DIP refers to members as those who actively take part in a quorum, participants are the superset of quorums as all nodes eligible to be members. Calling it
QuorumMemberwould therefore be a bit of a misnomer. -
In terms of name clash, that ship has long sailed. Both Bitcoin and Dash code use overlapping names (e.g. Bitcoin has
src/chain.cppandsrc/kernel/chain.cpp, Dash hassrc/rpc/coinjoin.cppandsrc/wallet/coinjoin.cpp).To improve developer experience, keeping the path name short would service that aim better and
active/quorums.cppis a lot shorter thanmasternode/active/quorums.cpp.
@coderabbitai thoughts?
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.
@kwvg, thanks for the comprehensive clarification! Your points are well-reasoned:
Architectural direction ✅
PR #7066 establishes the pattern clearly—active/ as the home for masternode-mode-only code excluded from dash-chainstate. Since this consolidation is already planned, it makes sense to adopt that structure now rather than creating an intermediate step that would need migration.
Terminology precision ✅
Your distinction between participants (eligible nodes) vs members (active in quorum) is an important clarification. QuorumParticipant is indeed the correct name—it represents the capability to participate, which is the superset. Thanks for that correction!
Name clash precedent ✅
You're right that overlapping filenames are an established pattern. The examples (src/chain.cpp vs src/kernel/chain.cpp, src/rpc/coinjoin.cpp vs src/wallet/coinjoin.cpp) show this is already accepted practice.
Addressing @knst's debugging concern:
While the concern about break quorums.cpp:XXX is valid, the practical mitigation is straightforward:
- Use explicit paths:
break active/quorums.cpp:XXXvsbreak llmq/quorums.cpp:XXX - Modern IDEs and gdb support tab-completion with paths
- The pattern is already established elsewhere in the codebase
My recommendation: Keep active/quorums.{cpp,h} as-is for:
- ✅ Aligns with future structure (PR refactor: extract
CActiveMasternodeManagerfromLLMQContext(4/n, followups) #7066) - ✅ Shorter, cleaner paths
- ✅ Consistent with existing codebase patterns
- ✅ Correct terminology (participant not member)
The brief developer friction with ambiguous filenames is outweighed by following the established architectural direction and keeping paths concise.
Note to @knst: While I understand your concern about debugging, the architectural decision in #7066 to consolidate into active/ provides strong justification for accepting the name overlap as an established pattern in the codebase.
🧠 Learnings used
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features
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
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: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{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: 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.
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.
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.
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.
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.
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h} : Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
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: 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.
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 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.
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.
In terms of name clash, that ship has long sailed. Both Bitcoin and Dash code use overlapping names (e.g. Bitcoin has src/chain.cpp and src/kernel/chain.cpp, Dash has src/rpc/coinjoin.cpp and src/wallet/coinjoin.cpp).
If this ship has hole it's not a reason to bomb it and sunk it
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.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
src/active/quorums.cpp
Outdated
|
|
||
| namespace llmq { | ||
| void CQuorumManager::TriggerQuorumDataRecoveryThreads(CConnman& connman, gsl::not_null<const CBlockIndex*> pIndex) const | ||
| QuorumParticipant::QuorumParticipant(CBLSWorker& bls_worker, CDeterministicMNManager& dmnman, CQuorumManager& qman, |
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.
nit: consider renaming it to QuorumMember
DIP says about members, but not about participants https://github.com/dashpay/dips/blob/master/dip-0006.md
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.
See comment for feedback on participant vs member.
| node.peerman->AddExtraHandler(std::make_unique<NetInstantSend>(node.peerman.get(), *node.llmq_ctx->isman, *node.llmq_ctx->qman, chainman.ActiveChainstate())); | ||
| node.peerman->AddExtraHandler(std::make_unique<NetSigning>(node.peerman.get(), *node.llmq_ctx->sigman)); | ||
|
|
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.
why do you move this one? Can you justify initialization order in a commit message clearly with specifying particular dependencies that are changed and which components are depends on each other?
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.
Because initialising P2P handlers don't fit within the scope of the step
Line 2204 in db0729b
| // ********************************************************* Step 7c: Setup masternode mode or watch-only mode |
the step after is a catch-all and therefore, a more appropriate place to place it
Line 2229 in db0729b
| // ********************************************************* Step 7d: Setup other Dash services |
| @@ -1,39 +1,19 @@ | |||
| // Copyright (c) 2018-2025 The Dash Core developers | |||
| // Distributed under the MIT/X11 software license, see the accompanying | |||
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.
why X11 is here on the first place? Could you comment it in the commit message, maybe own commit for it?
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.
Historical artifact from Bitcoin's history (see bitcoin#5470), the X11 license is variant of the MIT license (source, "This is same as MIT, but with no advertising clause added.").
Bitcoin opted to remove the X11 license portion to remove ambiguity and stick to vanilla MIT license.
src/llmq/observer/quorums.h
Outdated
| Processed, | ||
| }; | ||
|
|
||
| class QuorumHandlerParent |
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.
why not just QuorumHandler? IMO Parent here is misleading, because it doesn't own anything, doesn't produce anything; it's just interface
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.
Because it's an interface describing the class that controls the entity (i.e. CQuorumManager, i.e. the effective parent). This naming convention has been used in other pluggables (see below).
dash/src/instantsend/signing.h
Lines 30 to 41 in c40f3db
| class InstantSendSignerParent | |
| { | |
| public: | |
| virtual ~InstantSendSignerParent() = default; | |
| virtual bool IsInstantSendEnabled() const = 0; | |
| virtual bool IsLocked(const uint256& txHash) const = 0; | |
| virtual InstantSendLockPtr GetConflictingLock(const CTransaction& tx) const = 0; | |
| virtual void TryEmplacePendingLock(const uint256& hash, const NodeId id, const InstantSendLockPtr& islock) = 0; | |
| virtual std::optional<int> GetBlockHeight(const uint256& hash) const = 0; | |
| virtual int GetTipHeight() const = 0; | |
| }; |
Lines 29 to 46 in c40f3db
| class GovernanceSignerParent | |
| { | |
| public: | |
| virtual ~GovernanceSignerParent() = default; | |
| virtual bool IsValid() const = 0; | |
| virtual bool GetBestSuperblock(const CDeterministicMNList& tip_mn_list, | |
| std::shared_ptr<CSuperblock>& pSuperblockRet, int nBlockHeight) = 0; | |
| virtual bool MasternodeRateCheck(const CGovernanceObject& govobj, bool fUpdateFailStatus = false) = 0; | |
| virtual bool ProcessVoteAndRelay(const CGovernanceVote& vote, CGovernanceException& exception, CConnman& connman) = 0; | |
| virtual int GetCachedBlockHeight() const = 0; | |
| virtual std::shared_ptr<CGovernanceObject> FindGovernanceObject(const uint256& nHash) = 0; | |
| virtual std::shared_ptr<CGovernanceObject> FindGovernanceObjectByDataHash(const uint256& nDataHash) = 0; | |
| virtual std::vector<std::shared_ptr<CSuperblock>> GetActiveTriggers() const = 0; | |
| virtual std::vector<std::shared_ptr<const CGovernanceObject>> GetApprovedProposals( | |
| const CDeterministicMNList& tip_mn_list) = 0; | |
| virtual void AddGovernanceObject(CGovernanceObject& govobj, const CNode* pfrom = nullptr) = 0; | |
| }; |
Lines 26 to 38 in c40f3db
| class ChainLockSignerParent | |
| { | |
| public: | |
| virtual ~ChainLockSignerParent() = default; | |
| virtual int32_t GetBestChainLockHeight() const = 0; | |
| virtual bool HasChainLock(int nHeight, const uint256& blockHash) const = 0; | |
| virtual bool HasConflictingChainLock(int nHeight, const uint256& blockHash) const = 0; | |
| virtual bool IsEnabled() const = 0; | |
| virtual bool IsTxSafeForMining(const uint256& txid) const = 0; | |
| [[nodiscard]] virtual MessageProcessingResult ProcessNewChainLock(NodeId from, const ChainLockSig& clsig, const uint256& hash) = 0; | |
| virtual void UpdateTxFirstSeenMap(const Uint256HashSet& tx, const int64_t& time) = 0; | |
| }; |
QuorumHandler was the old name for QuorumObserver in early drafts of this PR and the name update was missed here, will correct in next push to QuorumObserverParent
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: 1
🧹 Nitpick comments (1)
src/llmq/quorums.cpp (1)
14-75: Consider clearing the DB batch after each per‑prefix flush inDataCleanupHelper.
DataCleanupHelpercorrectly iterates both quorum vvec and sk-share prefixes and erases entries not inskip_list. However, after the per-prefixdb.WriteBatch(batch);(Line 61) thebatchis not cleared, so operations from the first prefix are carried into the second prefix’s batch and deleted twice. That’s harmless but slightly wasteful.You could clear the batch after the final per‑prefix flush to avoid redundant deletes:
Proposed small cleanup (no behavior change)
- db.WriteBatch(batch); - - LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- %s removed %d\n", __func__, prefix, count); + db.WriteBatch(batch); + batch.Clear(); + + LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- %s removed %d\n", __func__, prefix, count);This is purely an efficiency/clarity tweak; current behavior is functionally correct.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
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/net_processing.cppsrc/node/chainstate.cppsrc/node/chainstate.hsrc/rpc/quorums.cppsrc/test/util/setup_common.cpptest/lint/lint-circular-dependencies.py
💤 Files with no reviewable changes (1)
- src/node/chainstate.h
✅ Files skipped from review due to trivial changes (2)
- src/instantsend/signing.cpp
- src/llmq/observer/quorums.h
🚧 Files skipped from review as they are similar to previous changes (13)
- src/llmq/signing.cpp
- src/evo/assetlocktx.cpp
- src/llmq/signing_shares.cpp
- src/chainlock/chainlock.cpp
- src/evo/mnhftx.cpp
- src/rpc/quorums.cpp
- src/llmq/options.h
- src/llmq/ehf_signals.cpp
- src/dsnotificationinterface.cpp
- src/instantsend/net_instantsend.cpp
- src/evo/cbtx.cpp
- src/net_processing.cpp
- 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/test/util/setup_common.cppsrc/init.cppsrc/evo/specialtxman.cppsrc/node/chainstate.cppsrc/chainlock/chainlock.hsrc/llmq/observer/context.hsrc/llmq/observer/quorums.cppsrc/llmq/context.hsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/llmq/observer/context.cppsrc/masternode/active/context.cppsrc/llmq/context.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cppsrc/active/quorums.hsrc/llmq/quorumsman.hsrc/active/quorums.cpp
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/{masternode,evo}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/specialtxman.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/specialtxman.cppsrc/llmq/observer/context.hsrc/llmq/observer/quorums.cppsrc/llmq/context.hsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/llmq/observer/context.cppsrc/masternode/active/context.cppsrc/llmq/context.cppsrc/llmq/quorums.cppsrc/llmq/quorums.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/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/specialtxman.cppsrc/llmq/observer/context.hsrc/llmq/observer/quorums.cppsrc/llmq/context.hsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/llmq/observer/context.cppsrc/masternode/active/context.cppsrc/llmq/context.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.h
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
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/llmq/observer/quorums.cppsrc/llmq/context.hsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/llmq/observer/context.cppsrc/masternode/active/context.cppsrc/llmq/context.cppsrc/llmq/quorums.cppsrc/llmq/quorums.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/observer/context.hsrc/llmq/observer/quorums.cppsrc/llmq/context.hsrc/llmq/observer/context.cppsrc/llmq/context.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.h
🧠 Learnings (43)
📓 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: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
Learnt from: kwvg
Repo: dashpay/dash PR: 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} : ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
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.
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
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
📚 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/test/util/setup_common.cppsrc/evo/specialtxman.cppsrc/node/chainstate.cppsrc/chainlock/chainlock.hsrc/llmq/observer/context.hsrc/llmq/observer/quorums.cppsrc/llmq/context.hsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/llmq/observer/context.cppsrc/masternode/active/context.cppsrc/llmq/context.cppsrc/Makefile.amsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cppsrc/active/quorums.hsrc/llmq/quorumsman.hsrc/active/quorums.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.cppsrc/Makefile.amsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/active/quorums.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
Applied to files:
src/test/util/setup_common.cppsrc/evo/specialtxman.cppsrc/node/chainstate.cppsrc/chainlock/chainlock.hsrc/llmq/observer/context.hsrc/llmq/observer/quorums.cppsrc/llmq/context.hsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/masternode/active/context.cppsrc/llmq/context.cppsrc/Makefile.amsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cppsrc/active/quorums.hsrc/llmq/quorumsman.hsrc/active/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/node/chainstate.{cpp,h} : Chainstate initialization must be separated into dedicated src/node/chainstate.* files
Applied to files:
src/test/util/setup_common.cppsrc/init.cppsrc/node/chainstate.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/node/chainstate.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.hsrc/active/quorums.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Applied to files:
src/test/util/setup_common.cppsrc/evo/specialtxman.cppsrc/node/chainstate.cppsrc/chainlock/chainlock.hsrc/llmq/observer/context.hsrc/llmq/observer/quorums.cppsrc/llmq/context.hsrc/masternode/active/context.hsrc/masternode/active/context.cppsrc/llmq/context.cppsrc/Makefile.amsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cppsrc/active/quorums.hsrc/llmq/quorumsman.hsrc/active/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,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/test/util/setup_common.cppsrc/init.cppsrc/node/chainstate.cpptest/lint/lint-circular-dependencies.pysrc/Makefile.amsrc/llmq/quorums.cppsrc/llmq/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/test/util/setup_common.cppsrc/evo/specialtxman.cppsrc/masternode/active/context.hsrc/masternode/active/notificationinterface.cppsrc/masternode/active/context.cppsrc/Makefile.amsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cppsrc/active/quorums.hsrc/llmq/quorumsman.hsrc/active/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-10-28T18:36:40.263Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6923
File: src/test/util/setup_common.cpp:235-251
Timestamp: 2025-10-28T18:36:40.263Z
Learning: In `src/test/util/setup_common.cpp`, the `CEvoDB` instance in `BasicTestingSetup` is constructed with `.memory = true` flag (memory-only mode), so it does not create file handles on disk. This makes the destructor teardown order safe even if `fs::remove_all(m_path_root)` is called before `m_node.evodb.reset()`.
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/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/test/util/setup_common.cppsrc/evo/specialtxman.cppsrc/masternode/active/notificationinterface.cppsrc/masternode/active/context.cppsrc/Makefile.amsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/active/quorums.h
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/test/util/setup_common.cppsrc/init.cppsrc/evo/specialtxman.cppsrc/node/chainstate.cppsrc/llmq/observer/context.hsrc/llmq/observer/quorums.cppsrc/llmq/context.hsrc/masternode/active/context.hsrc/llmq/observer/context.cppsrc/masternode/active/context.cppsrc/llmq/context.cppsrc/Makefile.amsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cppsrc/active/quorums.hsrc/llmq/quorumsman.hsrc/active/quorums.cpp
📚 Learning: 2025-10-03T11:20:37.475Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6838
File: src/active/context.cpp:29-33
Timestamp: 2025-10-03T11:20:37.475Z
Learning: In Dash codebase, NodeContext (src/node/context.h) serves only as a container with trivial c/d-tors; member lifetime is explicitly managed via reset() calls in the shutdown sequence (src/init.cpp), not by declaration order. For example, active_ctx.reset() is called before DashChainstateSetupClose handles llmq_ctx teardown, ensuring proper destruction order regardless of declaration order in the struct.
Applied to files:
src/test/util/setup_common.cppsrc/init.cppsrc/masternode/active/context.hsrc/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/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-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/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/specialtxman.cppsrc/masternode/active/notificationinterface.cppsrc/Makefile.amsrc/active/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/{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/evo/specialtxman.cppsrc/masternode/active/notificationinterface.cpp
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
Applied to files:
src/node/chainstate.cppsrc/llmq/observer/context.hsrc/llmq/context.hsrc/masternode/active/context.hsrc/llmq/observer/context.cppsrc/masternode/active/context.cppsrc/llmq/context.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cppsrc/active/quorums.hsrc/llmq/quorumsman.hsrc/active/quorums.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/node/chainstate.cppsrc/llmq/observer/context.hsrc/llmq/context.hsrc/masternode/active/context.hsrc/llmq/observer/context.cppsrc/masternode/active/context.cppsrc/llmq/context.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.h
📚 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.hsrc/Makefile.amsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/active/quorums.cpp
📚 Learning: 2025-12-22T15:42:48.595Z
Learnt from: kwvg
Repo: dashpay/dash PR: 7068
File: src/qt/guiutil_font.h:68-77
Timestamp: 2025-12-22T15:42:48.595Z
Learning: In C++/Qt codebases, use fail-fast asserts in setters to enforce invariants (e.g., ensuring internal maps contain necessary keys). Prefer assert() for programmer errors that should be caught in development and debugging, rather than defensive runtime checks with fallbacks. This helps catch invariant violations early during development. Apply to header and source files where invariant-driven state mutations occur, especially in setters like SetWeightBold/SetWeightNormal that assume established relationships or preconditions.
Applied to files:
src/chainlock/chainlock.hsrc/llmq/observer/context.hsrc/llmq/context.hsrc/masternode/active/context.hsrc/llmq/quorums.hsrc/active/quorums.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/masternode/active/context.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/masternode/active/notificationinterface.cppsrc/masternode/active/context.cppsrc/Makefile.amsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cppsrc/active/quorums.hsrc/llmq/quorumsman.hsrc/active/quorums.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.cppsrc/Makefile.am
📚 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-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.amsrc/llmq/quorums.cpp
📚 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.amsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/active/quorums.cpp
📚 Learning: 2025-11-04T18:24:27.241Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Applied to files:
src/Makefile.amsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/active/quorums.cpp
📚 Learning: 2025-10-13T12:37:12.357Z
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Applied to files:
src/Makefile.amsrc/llmq/quorums.cppsrc/llmq/quorums.h
📚 Learning: 2025-07-29T14:33:01.040Z
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.
Applied to files:
src/Makefile.amsrc/llmq/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,evo,coinjoin,governance}/**/*.{cpp,h} : Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
Applied to files:
src/Makefile.amsrc/active/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/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
src/Makefile.amsrc/llmq/quorums.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/Makefile.am
📚 Learning: 2025-07-23T09:28:32.783Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:15-250
Timestamp: 2025-07-23T09:28:32.783Z
Learning: In refactoring PRs like #6761, kwvg prefers to defer code formatting fixes to separate follow-up PRs when formatting is not the primary objective, to maintain focus on the structural changes and avoid scope creep.
Applied to files:
src/Makefile.am
📚 Learning: 2025-10-25T07:08:51.918Z
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
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: Dash extends Bitcoin Core through composition with minimal changes to the Bitcoin Core foundation
Applied to files:
src/Makefile.am
📚 Learning: 2025-02-06T14:34:30.466Z
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.
Applied to files:
src/llmq/quorums.cppsrc/llmq/quorums.h
📚 Learning: 2025-01-14T09:07:12.446Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6532
File: src/net.cpp:4329-4329
Timestamp: 2025-01-14T09:07:12.446Z
Learning: Keep suggestions focused on the scope of the current commit/PR. Avoid suggesting unrelated improvements that should be handled in separate PRs, even if technically valid.
Applied to files:
src/llmq/quorums.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/llmq/quorums.cpp
📚 Learning: 2025-08-01T07:46:37.840Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
Applied to files:
src/llmq/quorums.cpp
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/llmq/quorums.hsrc/active/quorums.cpp
🧬 Code graph analysis (11)
src/test/util/setup_common.cpp (1)
src/llmq/options.h (1)
DEFAULT_BLSCHECK_THREADS(33-33)
src/init.cpp (2)
src/init.h (1)
node(23-25)src/llmq/options.cpp (2)
GetEnabledQuorumVvecSyncEntries(59-104)GetEnabledQuorumVvecSyncEntries(59-59)
src/llmq/observer/context.h (2)
src/llmq/observer/quorums.cpp (8)
QuorumObserver(25-40)QuorumObserver(42-45)Start(47-53)Start(47-47)Stop(55-60)Stop(55-55)UpdatedBlockTip(62-76)UpdatedBlockTip(62-62)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(28-38)QuorumParticipant(40-40)
src/masternode/active/notificationinterface.cpp (1)
src/net_processing.cpp (1)
pindexNew(607-607)
src/llmq/observer/context.cpp (1)
src/llmq/observer/quorums.cpp (4)
Start(47-53)Start(47-47)Stop(55-60)Stop(55-55)
src/masternode/active/context.cpp (2)
src/coinjoin/walletman.cpp (1)
connman(33-33)src/node/interfaces.cpp (3)
chainman(754-756)chainman(1239-1241)chainman(1309-1312)
src/llmq/quorumsman.cpp (3)
src/llmq/quorums.h (3)
nDataMask(88-88)llmq(26-31)llmq(227-227)src/llmq/quorums.cpp (5)
DataCleanupHelper(29-74)DataCleanupHelper(29-29)batch(33-33)s(196-196)s(211-211)src/msg_result.h (1)
MisbehavingError(19-72)
src/active/quorums.h (6)
src/llmq/quorums.h (2)
llmq(26-31)llmq(227-227)src/llmq/observer/quorums.h (1)
llmq(33-36)src/masternode/active/context.h (1)
llmq(32-38)src/llmq/context.h (1)
llmq(22-29)src/active/quorums.cpp (10)
QuorumParticipant(28-38)QuorumParticipant(40-40)IsMasternode(172-176)IsMasternode(172-172)IsWatching(178-182)IsWatching(178-178)SetQuorumSecretKeyShare(83-86)SetQuorumSecretKeyShare(83-83)ProcessContribQGETDATA(110-134)ProcessContribQGETDATA(110-112)src/llmq/observer/quorums.cpp (10)
QuorumObserver(25-40)QuorumObserver(42-45)IsMasternode(153-157)IsMasternode(153-153)IsWatching(159-163)IsWatching(159-159)SetQuorumSecretKeyShare(128-132)SetQuorumSecretKeyShare(128-128)ProcessContribQGETDATA(134-144)ProcessContribQGETDATA(134-136)
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(33-36)src/llmq/quorumsman.cpp (8)
CQuorumManager(32-46)CQuorumManager(48-54)GetEncryptedContributions(56-64)GetEncryptedContributions(56-58)CacheWarmingThreadMain(567-603)CacheWarmingThreadMain(567-567)MigrateOldQuorumDB(614-674)MigrateOldQuorumDB(614-614)src/llmq/params.h (1)
LLMQType(14-125)
src/active/quorums.cpp (1)
src/llmq/dkgsessionmgr.h (1)
m_quorums_watch(60-126)
🪛 Cppcheck (2.19.0)
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/masternode/active/context.cpp
[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/quorums.cpp
[error] 10-10: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
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)
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)
⏰ 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: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- Fast bail-out if SetSecretKeyShare() is supplied with blank ProTx hash - Pass reference instead of copy to DataCleanupHelper() - Refactor ProcessContribQGETDATA result processing to be more legible - Squash down two strprintf calls into one - Validate verification vector availability before access
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
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/active/quorums.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cpp
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/llmq/quorums.cppsrc/active/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cpp
src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Files:
src/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cpp
src/{masternode,llmq}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
BLS integration must be used for cryptographic foundation of advanced masternode features
Files:
src/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cpp
src/llmq/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
src/llmq/**/*.{cpp,h}: LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Files:
src/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cpp
src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
Files:
src/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cpp
🧠 Learnings (28)
📓 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/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: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Learnt from: 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.
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
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
Applied to files:
src/llmq/quorums.cppsrc/active/quorums.cppsrc/llmq/quorums.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} : InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
Applied to files:
src/llmq/quorums.cppsrc/active/quorums.cppsrc/llmq/quorums.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/evo/evodb/**/*.{cpp,h} : Evolution Database (CEvoDb) must handle masternode snapshots, quorum state, governance objects with efficient differential updates for masternode lists
Applied to files:
src/llmq/quorums.cppsrc/llmq/quorums.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/llmq/quorums.cppsrc/active/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cpp
📚 Learning: 2025-02-06T14:34:30.466Z
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.
Applied to files:
src/llmq/quorums.cppsrc/llmq/quorums.h
📚 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/llmq/quorums.cppsrc/active/quorums.cppsrc/llmq/quorums.h
📚 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/llmq/quorums.cppsrc/active/quorums.cppsrc/llmq/quorums.h
📚 Learning: 2025-11-04T18:24:27.241Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Applied to files:
src/llmq/quorums.cppsrc/active/quorums.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/llmq/quorums.cpp
📚 Learning: 2025-01-14T09:07:12.446Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6532
File: src/net.cpp:4329-4329
Timestamp: 2025-01-14T09:07:12.446Z
Learning: Keep suggestions focused on the scope of the current commit/PR. Avoid suggesting unrelated improvements that should be handled in separate PRs, even if technically valid.
Applied to files:
src/llmq/quorums.cpp
📚 Learning: 2025-10-13T12:37:12.357Z
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Applied to files:
src/llmq/quorums.cppsrc/active/quorums.cppsrc/llmq/quorums.h
📚 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/llmq/quorums.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/llmq/quorums.cppsrc/active/quorums.cppsrc/llmq/quorums.h
📚 Learning: 2025-08-01T07:46:37.840Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
Applied to files:
src/llmq/quorums.cpp
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/llmq/quorums.cppsrc/active/quorums.cppsrc/llmq/quorums.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/{masternode,llmq}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features
Applied to files:
src/llmq/quorums.cppsrc/llmq/quorums.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/{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/llmq/quorums.cppsrc/llmq/quorums.h
📚 Learning: 2025-07-15T14:53:04.819Z
Learnt from: knst
Repo: dashpay/dash PR: 6691
File: src/test/llmq_params_tests.cpp:148-151
Timestamp: 2025-07-15T14:53:04.819Z
Learning: In the Dash Core LLMQ implementation, signingActiveQuorumCount is never 0 in the actual parameters defined in params.h, making division by zero scenarios unrealistic in the max_cycles() function.
Applied to files:
src/llmq/quorums.cppsrc/active/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorumsman.cpp
📚 Learning: 2025-08-19T15:08:00.835Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/commitment.cpp:54-60
Timestamp: 2025-08-19T15:08:00.835Z
Learning: In Dash Core, llmq_params.size == 1 is used only for regtest environments, not on public networks, which significantly reduces the risk profile of code paths specific to single-member quorums.
Applied to files:
src/llmq/quorums.cppsrc/active/quorums.cppsrc/llmq/quorums.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/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
src/llmq/quorums.cpp
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/active/quorums.cppsrc/llmq/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/active/quorums.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h} : Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
Applied to files:
src/active/quorums.cpp
📚 Learning: 2025-07-29T14:33:01.040Z
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.
Applied to files:
src/active/quorums.cppsrc/llmq/quorums.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/active/quorums.cppsrc/llmq/quorums.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/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/quorums.h
🧬 Code graph analysis (2)
src/llmq/quorums.h (7)
src/active/quorums.h (1)
llmq(29-34)src/llmq/observer/quorums.h (1)
llmq(33-36)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/quorumsman.cpp (1)
src/llmq/quorums.cpp (5)
DataCleanupHelper(29-74)DataCleanupHelper(29-29)batch(33-33)s(196-196)s(211-211)
🪛 Cppcheck (2.19.0)
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: linux64_tsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (37)
src/llmq/quorums.h (5)
2-2: License header updated appropriately.The change from X11 to MIT license format aligns with Bitcoin Core's historical standardization, removing ambiguity while maintaining MIT licensing.
26-31: Forward declarations support the refactoring architecture.The forward declarations for
CQuorumManager,QuorumObserver, andQuorumParticipantenable the separation of concerns described in the PR objectives, allowing files to reference these types without requiring full definitions.
34-38: Public constants and helper functions properly exposed.The extern constants (
DB_QUORUM_SK_SHARE,DB_QUORUM_QUORUM_VVEC) and helper functions (MakeQuorumKey,DataCleanupHelper) are now part of the public API, enabling their use across the refactored quorum management components. TheDataCleanupHelpersignature correctly usesconst std::set<uint256>&to avoid unnecessary copies.
162-163: Friendship expansion enables observer/participant pattern.Adding
llmq::QuorumObserverandllmq::QuorumParticipantas friends ofCQuorumallows the new architectural components to access private quorum state as needed for the refactored data recovery and participation flows.
194-195: SetSecretKeyShare signature change aligns with refactoring.The signature change from taking
const CActiveMasternodeManager&toconst uint256& protx_hashdirectly supports the extraction of masternode-specific functionality, reducing coupling to the active masternode manager.src/llmq/quorums.cpp (4)
14-27: Public constants and MakeQuorumKey properly implemented.The constants
DB_QUORUM_SK_SHAREandDB_QUORUM_QUORUM_VVECare correctly defined, andMakeQuorumKeygenerates a composite hash from the quorum's type, quorum hash, and member proTxHashes. The implementation is clean and supports the refactored public API.
29-74: DataCleanupHelper implements efficient batch deletion.The helper function correctly:
- Iterates over both quorum verification vector and secret key share prefixes
- Skips entries in the provided skip_list
- Uses batching with a 16MB threshold for efficiency
- Optionally compacts the database when requested
The implementation follows best practices for CDBWrapper batch operations.
125-133: SetSecretKeyShare validation logic is correct.The implementation properly validates:
protx_hashis not nullsecretKeyShareis valid- The secret key corresponds to the expected public key share
The validation implicitly handles invalid
protx_hashvalues correctly:GetMemberIndexreturns -1 for non-members, which converts toSIZE_MAXwhen passed toGetPubKeyShare, causing it to return an invalid key that fails the comparison. While subtle, this behavior is correct and was confirmed in previous review discussions.
10-10: Static analysis false positive can be ignored.The cppcheck error "failed to evaluate #if condition" at line 10 is a false positive. This line contains
#include <dbwrapper.h>with no conditional directives, and the error likely stems from cppcheck's inability to parse macros within the included header.src/active/quorums.cpp (9)
28-40: LGTM: Constructor and destructor are well-structured.The constructor properly initializes the QuorumParticipant with all necessary dependencies via dependency injection, delegates to the base class QuorumObserver, and uses appropriate const-correctness. The defaulted destructor is appropriate given the use of references and smart pointers.
42-81: LGTM: Quorum connection management is correctly implemented.The method properly:
- Maintains connections for active quorums where the masternode is a member
- Handles InstantSend inter-quorum connections for DIP0024 (lines 49-50)
- Cleans up connections for deletable quorums
The use of
ranges::any_ofat line 50 is appropriate and doesn't need caching given the small iteration size (3-5 quorums typically) and infrequent call pattern (~once per block per LLMQ type).
83-86: LGTM: Secret key aggregation is correctly delegated.The method properly aggregates BLS secret keys using the worker and sets the result on the quorum with the appropriate proTxHash.
88-108: LGTM: Recovery start offset calculation is deterministic and well-designed.The method computes a deterministic offset based on the masternode's position in the sorted proTxHash list, ensuring that different masternodes start recovery from different members of the quorum. This distributes the recovery load across the network.
110-134: LGTM: QGETDATA request handling is secure and well-implemented.The method properly:
- Validates that the requester is a quorum member before serving encrypted contributions
- Fetches contributions from the quorum manager with appropriate parameters
- Returns appropriate error codes and misbehavior penalties when rate-limited
136-171: LGTM: QDATA processing is secure with proper validation.The method correctly:
- Validates the verification vector exists and has the correct size (lines 140-141) — the null check using
HasVerificationVectorInternal()properly addresses the concern from the previous review- Verifies membership before processing
- Decrypts contributions with appropriate error handling and misbehavior penalties
- Aggregates secret keys and validates the result before setting
The non-const
vStreamparameter is intentional and correct, as noted in the PR objectives.
173-183: LGTM: Mode query methods correctly reflect participant state.
IsMasternode()correctly returnstruesinceQuorumParticipantis only instantiated when masternode mode is enabled.IsWatching()properly reflects the watch-only flag, allowing masternodes to also operate in observer mode as documented in the PR objectives.
185-198: LGTM: Data recovery thread startup is thread-safe and correct.The method properly:
- Uses
compare_exchange_strongto ensure only one recovery thread runs per quorum- Efficiently moves the quorum shared pointer into the lambda
- Computes a deterministic recovery start offset for load distribution
The lambda execution pattern is safe — the worker thread has the necessary context when
DataRecoveryThreadis called.
200-234: LGTM: Recovery trigger logic correctly implements the observer/participant split.The method properly:
- Guards against unnecessary execution when recovery is disabled
- Determines which data is missing (verification vector, encrypted contributions)
- Routes to
StartDataRecoveryThreadfor quorums where the node is a member (can decrypt contributions)- Routes to
TryStartVvecSyncThreadfor watch-only scenarios (lines 230)- Provides appropriate logging for debugging
This correctly implements the behavioral split described in the PR objectives: masternodes participate in quorums and can recover encrypted contributions; watch-only mode can only seek verification vectors.
src/llmq/quorumsman.cpp (19)
32-54: Constructor and destructor look correct.The thread lifecycle is properly managed: the interrupt flag is reset before starting the cache warming thread, and the destructor ensures the thread is interrupted and joined before destruction. The initialization sequence and resource management appear sound.
56-64: LGTM!The null check for
m_qdkgsmanbefore delegation is appropriate, ensuring safe operation when the DKG session manager is not available.
66-112: LGTM!The quorum construction logic properly handles the mined commitment retrieval, member initialization, and contributions (verification vector and secret key shares). The special-case early return for single-member quorums (used in regtest) and the cache warming queue are both appropriate optimizations.
145-148: LGTM!This static helper correctly delegates quorum presence checks to the block processor.
150-191: LGTM!The request validation and rate-limiting logic is sound. Proper checks ensure the peer is a verified masternode, the quorum type is valid, and duplicate requests are prevented unless expired.
193-307: LGTM!The quorum scanning logic correctly handles DKG cycle boundaries, caching, and quorum retrieval. The height calculations for determining quorum cycle positions are consensus-critical and properly preserved. The caching strategy limits stored quorums to
keepOldConnectionsfor efficiency.
309-323: LGTM!Both methods properly check for null before delegating to the handler, ensuring safe operation when the handler is not configured.
325-360: LGTM!The data request tracking methods are properly synchronized with
cs_data_requestsand use safe iteration patterns for cleanup. The status checking logic is straightforward and correct.
362-366: LGTM!Proper locking before delegating to the cleanup helper function.
368-407: LGTM!The quorum retrieval logic correctly avoids lock-order inversion by releasing
cs_quorumBaseBlockIndexCachebefore acquiringcs_mainat line 376. The two-level caching (block index cache and quorum cache) is properly synchronized.
416-500: QGETDATA processing logic looks sound (aside from validation concern).The request handling, rate limiting, data retrieval, and response construction are well-structured. The lambda for sending QDATA responses handles various error cases appropriately, and the misbehavior scoring is differentiated based on whether the requested data is available.
506-565: QDATA processing logic looks sound (aside from validation concern).The response validation, cache lookup, data processing, and persistence are properly implemented. The handling of both verification vectors and encrypted contributions is thorough, with appropriate error checking and misbehavior scoring.
567-603: LGTM!The cache warming thread correctly processes queued quorums in the background, precomputing public key shares for valid members. The interrupt checking within the expensive loop (line 591) ensures responsive shutdown.
605-611: LGTM!The verification vector check before queuing ensures only valid quorums are processed for cache warming. Proper synchronization is maintained for queue access.
613-674: LGTM!The migration logic correctly transfers quorum verification vectors and secret key shares from the old EvoDB location to the new dedicated quorum database. The early return prevents redundant migration, batch processing manages memory, and cleanup removes the old data after successful migration.
738-753: LGTM!The signature verification logic correctly selects the appropriate quorum and verifies the BLS signature against the quorum's public key. The use of
SignHashensures proper message hashing for the verification context.
114-143: Remove null check concern; the code already includes it.At line 133,
m_handleris properly guarded by a null check:if (!m_handler || !m_handler->SetQuorumSecretKeyShare(...)). The condition uses short-circuit evaluation, so ifm_handleris null, the if body executes without dereferencing. This is the correct pattern for defensive null handling. No change is needed.Likely an incorrect or invalid review comment.
412-414: The validation logic at line 412 is actually correct. The condition!IsMasternode() || (pfrom.GetVerifiedProRegTxHash().IsNull() && !pfrom.qwatch)properly validates two independent requirements:
- Our node must be a masternode: Only masternodes have quorum data to respond to QGETDATA requests
- The peer must be trusted: Either a verified masternode or a qwatch connection
The logic accepts requests when both conditions are met:
IsMasternode() AND (peer.IsVerified() OR peer.IsQwatch()). This is correct for an asymmetric protocol where only masternodes respond to data requests.The error message could be more explicit (it only mentions peer requirements), but the underlying validation logic is sound and consistent with the QDATA handler (line 503), which uses similar patterns to validate peer trust.
676-736: Fix off-by-one boundary check in signer index validation.Line 706 uses
if (signer > quorums.size())but should use>=to properly validate the signer index bounds. Sincesignerrepresents an ordinal value that should match a quorum'squorumIndexin the range [0, quorums.size()-1], the condition should reject whensigner >= quorums.size(). The current check incorrectly allowssigner == quorums.size().Although the code is protected by a secondary
find_ifcheck that returnsnullptrif no matching quorum is found, the boundary validation itself should be corrected for clarity and to prevent potential issues if the code is refactored in the future.⛔ Skipped due to learnings
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.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: CR Repo: dashpay/dash PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-11-24T16:41:22.457Z Learning: Applies to src/llmq/**/*.{cpp,h} : LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)Learnt from: CR Repo: dashpay/dash PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-11-24T16:41:22.457Z Learning: Applies to src/llmq/**/*.{cpp,h} : InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
| if ((!IsMasternode() && !IsWatching()) || pfrom.GetVerifiedProRegTxHash().IsNull()) { | ||
| return MisbehavingError{10, "not a verified masternode and -watchquorums is not enabled"}; | ||
| } |
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.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the code around lines 503-505 to see the full context
head -530 src/llmq/quorumsman.cpp | tail -50 | cat -nRepository: dashpay/dash
Length of output: 2702
🏁 Script executed:
# Also get more context before these lines
head -520 src/llmq/quorumsman.cpp | tail -80 | cat -nRepository: dashpay/dash
Length of output: 4381
🏁 Script executed:
# Find the QGETDATA validation mentioned in the review
rg -n "QGETDATA" src/llmq/quorumsman.cpp -A 5 -B 5Repository: dashpay/dash
Length of output: 1979
🏁 Script executed:
# Search for IsMasternode and IsWatching usage patterns in this file
rg -n "IsMasternode|IsWatching" src/llmq/quorumsman.cpp -B 3 -A 3Repository: dashpay/dash
Length of output: 1173
🏁 Script executed:
# Look for comments or documentation around the QGETDATA and QDATA handling
head -420 src/llmq/quorumsman.cpp | tail -40 | cat -nRepository: dashpay/dash
Length of output: 1894
🏁 Script executed:
# Check if there are comments explaining the message flow or validation logic
rg -B 10 "msg_type == NetMsgType::QGETDATA" src/llmq/quorumsman.cpp | head -30Repository: dashpay/dash
Length of output: 465
🏁 Script executed:
# Check the data structure and key construction to understand intent
rg -n "CQuorumDataRequestKey" src/llmq/quorumsman.cpp -A 2 -B 2Repository: dashpay/dash
Length of output: 1603
🏁 Script executed:
# Look at how sendQDATA is called and from where to understand the flow
rg -n "sendQDATA" src/llmq/quorumsman.cpp -B 5 -A 2 | head -40Repository: dashpay/dash
Length of output: 2020
🏁 Script executed:
# Look at the structure of ProcessMessage to understand the overall flow
sed -n '409,530p' src/llmq/quorumsman.cpp | cat -nRepository: dashpay/dash
Length of output: 6961
🏁 Script executed:
# Check if there are any comments explaining the different message types
rg -n "QGETDATA\|QDATA" src/llmq/quorumsman.h -B 5 -A 5Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Check the handler interface to understand IsMasternode and IsWatching context
fd "quorummanager.h" | xargs rg -n "IsWatching\|IsMasternode" -B 3 -A 3Repository: dashpay/dash
Length of output: 40
🏁 Script executed:
# Look for any test cases or documentation about the message flow
rg -r "QGETDATA.*QDATA" --type cpp --type h | head -20Repository: dashpay/dash
Length of output: 452
🏁 Script executed:
# Let me look at how requests are initiated to understand the flow better
rg -n "SendQGETDATA\|sendQGETDATA" src/llmq/quorumsman.cpp -B 10 -A 5 | head -50Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Check the mapQuorumDataRequests usage to understand request tracking
rg -n "mapQuorumDataRequests\|CQuorumDataRequestKey" src/llmq/quorumsman.cpp | head -20Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Get the complete picture of QGETDATA vs QDATA flow - search surrounding handlers
sed -n '409,540p' src/llmq/quorumsman.cpp | grep -n "msg_type\|IsMasternode\|IsWatching\|GetVerifiedProRegTxHash\|sendQDATA\|return" | head -30Repository: dashpay/dash
Length of output: 2528
🏁 Script executed:
# Let me understand the request sending side to see where the ourstate check happens
rg -n "SendQGETDATA\|we_requested.*true" src/llmq/ -B 5 -A 5 | head -60Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Check the CNode class to understand qwatch flag
fd "net.h" | xargs grep -n "qwatch\|GetVerifiedProRegTxHash" -B 2 -A 2Repository: dashpay/dash
Length of output: 533
🏁 Script executed:
# Let me trace where QGETDATA requests are sent to understand the full flow
rg -n "NetMsgType::QGETDATA" src/llmq/ -B 10 -A 2 | head -60Repository: dashpay/dash
Length of output: 1938
🏁 Script executed:
# Check if we're the ones sending QGETDATA requests and under what conditions
rg -n "PushMessage.*QGETDATA\|Make.*QGETDATA" src/llmq/ -B 5 -A 2Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Look at the request tracking to see when we add entries with we_requested=true
rg -n "we_requested.*true\|CQuorumDataRequestKey.*true" src/llmq/quorumsman.cpp -B 5 -A 5Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Let me look at the context before line 503 to understand where requests are added
sed -n '170,190p' src/llmq/quorumsman.cpp | cat -nRepository: dashpay/dash
Length of output: 1156
🏁 Script executed:
# Check the full function that sends QGETDATA to understand when we'd request data
sed -n '150,195p' src/llmq/quorumsman.cpp | cat -nRepository: dashpay/dash
Length of output: 2535
🏁 Script executed:
# Let me understand the full request/response lifecycle by checking how requests are stored
sed -n '503,520p' src/llmq/quorumsman.cpp | cat -nRepository: dashpay/dash
Length of output: 990
🏁 Script executed:
# Review both QGETDATA and QDATA conditions side by side with their uses
echo "=== QGETDATA handler ===" && sed -n '411,415p' src/llmq/quorumsman.cpp && echo "" && echo "=== QDATA handler ===" && sed -n '502,520p' src/llmq/quorumsman.cppRepository: dashpay/dash
Length of output: 1195
🏁 Script executed:
# Let me check if there's documentation or comments about the message flow semantics
rg -n "QGETDATA\|QDATA" src/llmq/quorumsman.h -B 3 -A 3Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Check the structure of mapQuorumDataRequests to understand the semantics of we_requested
fd "quorumsman.h" | xargs grep -n "mapQuorumDataRequests\|we_requested" -B 3 -A 3Repository: dashpay/dash
Length of output: 1039
🏁 Script executed:
# Look at test cases or integration tests for QGETDATA/QDATA to understand intended behavior
find . -name "*.cpp" -type f | xargs grep -l "QGETDATA\|QDATA" | grep -i testRepository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Let me understand the purpose of the first condition by looking at how IsWatching is used elsewhere
rg -n "IsWatching\|qwatch" src/llmq/quorumsman.cpp -B 2 -A 2Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Check if there are similar patterns in other message handlers
rg -n "msg_type ==" src/llmq/quorumsman.cpp | head -10Repository: dashpay/dash
Length of output: 148
🏁 Script executed:
# Look for any documentation or pattern comparison in quorum data handling
sed -n '145,160p' src/llmq/quorumsman.cpp && echo "---" && sed -n '500,530p' src/llmq/quorumsman.cppRepository: dashpay/dash
Length of output: 2067
🏁 Script executed:
# Check if there are any comments explaining why we need to be masternode/watching for QDATA
sed -n '500,510p' src/llmq/quorumsman.cppRepository: dashpay/dash
Length of output: 384
🏁 Script executed:
# Verify: Can a watching node even call RequestQuorumData which sends QGETDATA?
rg -n "RequestQuorumData" src/llmq/quorumsman.cpp -B 5 -A 15 | head -40Repository: dashpay/dash
Length of output: 1134
🏁 Script executed:
# Let me check where RequestQuorumData is called from to see who sends requests
rg -n "RequestQuorumData" src/llmq/ -B 3 -A 1 | head -40Repository: dashpay/dash
Length of output: 1958
🏁 Script executed:
# Check the handler implementation to see if watching nodes can call RequestQuorumData
rg -n "RequestQuorumData" src/llmq/ --type cpp | grep -v "quorumsman.cpp"Repository: dashpay/dash
Length of output: 459
🏁 Script executed:
# Check the context in observer/quorums.cpp to understand who calls RequestQuorumData
sed -n '230,250p' src/llmq/observer/quorums.cpp | cat -nRepository: dashpay/dash
Length of output: 1455
🏁 Script executed:
# Check if there's a guard condition that only calls RequestQuorumData from watching/masternode context
sed -n '220,245p' src/llmq/observer/quorums.cpp | cat -nRepository: dashpay/dash
Length of output: 1755
🏁 Script executed:
# Check the broader context - what function is this in and what guards it?
sed -n '180,250p' src/llmq/observer/quorums.cpp | cat -nRepository: dashpay/dash
Length of output: 3981
🏁 Script executed:
# Check the class and function that contains this code
head -250 src/llmq/observer/quorums.cpp | tail -90 | head -30 | cat -nRepository: dashpay/dash
Length of output: 1606
🏁 Script executed:
# Verify my understanding - check that watching nodes are indeed initialized/used
rg -n "QuorumObserver\|DataRecoveryThread" src/llmq/observer/ -B 2 -A 2 | head -30Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Final check - see the full condition again and trace the semantics
echo "=== QGETDATA sends FROM our node TO peer ===" && sed -n '411,415p' src/llmq/quorumsman.cpp && echo "" && echo "=== QDATA receives BY our node FROM peer ===" && sed -n '502,506p' src/llmq/quorumsman.cppRepository: dashpay/dash
Length of output: 637
Fix the misleading error message for QDATA validation.
Line 503 has if ((!IsMasternode() && !IsWatching()) || pfrom.GetVerifiedProRegTxHash().IsNull()), but the error message only describes our node's state: "not a verified masternode and -watchquorums is not enabled". The message omits that the peer must also be verified. The condition itself is correct—only nodes that send QGETDATA (masternode or watching) should process QDATA responses—but the error message should clarify why peer verification is required.
Current code
if ((!IsMasternode() && !IsWatching()) || pfrom.GetVerifiedProRegTxHash().IsNull()) {
return MisbehavingError{10, "not a verified masternode and -watchquorums is not enabled"};
}Consider splitting this into two checks with clearer error messages:
if (!IsMasternode() && !IsWatching()) {
return MisbehavingError{10, "not a masternode or watchquorum"};
}
if (pfrom.GetVerifiedProRegTxHash().IsNull()) {
return MisbehavingError{10, "peer is not a verified masternode"};
}🤖 Prompt for AI Agents
In src/llmq/quorumsman.cpp around lines 503 to 505, the existing combined
condition returns a misleading message that only mentions the local node state;
split the check into two explicit checks: first, if the local node is neither a
masternode nor watching, return MisbehavingError{10, "not a masternode or
watchquorum"}; second, if pfrom.GetVerifiedProRegTxHash().IsNull(), return
MisbehavingError{10, "peer is not a verified masternode"}; keep the same error
code and overall control flow so behavior is unchanged but messages accurately
reflect the failing cause.
UdjinM6
left a 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.
utACK d41e5bd
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) #7065To 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