refactor: split off governance masternode-only logic to GovernanceSigner, drop Relay()s and use periodic relay instead, minor cleanup
#6838
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughRefactors governance signing and relay flows: removes Sign/Relay methods from CoinJoin queue/broadcast, governance objects, and votes; introduces GovernanceSigner and GovernanceSignerParent and adds governance/signing.{h,cpp}. Adds CActiveMasternodeManager::SignBasic and switches callers to assign vchSig directly. CGovernanceManager gains scheduling, RelayObject/RelayVote, postponed object handling, AddInvalidVote, GetApprovedProposals, and a simplified UpdatedBlockTip; many ProcessMessage/AddGovernanceObject/ProcessVoteAndRelay signatures drop PeerManager. CDSNotificationInterface and ActiveContext constructors updated; GovernanceSigner is wired into ActiveContext and notification paths. Net/RPC/node/interface/init/test files updated accordingly; Makefile exports updated; lint expectations and some includes adjusted. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120–180 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
src/chainlock/chainlock.cpp (4)
65-81: Snapshot atomic signer before use in the scheduler.If
m_signerbecomes atomic, load once into a local before calls to prevent TOCTOU/UAF:- if (m_signer) { - m_signer->Start(); - } + if (auto* signer = m_signer.load(std::memory_order_acquire)) { + signer->Start(); + } ... - if (m_signer) { - m_signer->TrySignChainTip(isman); - } + if (auto* signer = m_signer.load(std::memory_order_acquire)) { + signer->TrySignChainTip(isman); + }
218-230: Same atomic snapshot pattern for UpdatedBlockTip task.- if (m_signer) { - m_signer->TrySignChainTip(isman); - } + if (auto* signer = m_signer.load(std::memory_order_acquire)) { + signer->TrySignChainTip(isman); + }
276-287: Use atomic snapshot in BlockConnected/Disconnected.- if (m_signer) { - m_signer->UpdateBlockHashTxidMap(pindex->GetBlockHash(), pblock->vtx); - } + if (auto* signer = m_signer.load(std::memory_order_acquire)) { + signer->UpdateBlockHashTxidMap(pindex->GetBlockHash(), pblock->vtx); + } ... - if (m_signer) { - m_signer->EraseFromBlockHashTxidMap(pindexDisconnected->GetBlockHash()); - } + if (auto* signer = m_signer.load(std::memory_order_acquire)) { + signer->EraseFromBlockHashTxidMap(pindexDisconnected->GetBlockHash()); + }
454-462: Atomic snapshot around Cleanup() as well.- if (m_signer) { - const auto cleanup_txes{m_signer->Cleanup()}; + if (auto* signer = m_signer.load(std::memory_order_acquire)) { + const auto cleanup_txes{signer->Cleanup()};src/instantsend/instantsend.h (1)
15-19: Missing include for gsl::not_null in this header.Unlike chainlock.h, this file doesn’t include
<gsl/pointers.h>. Avoid relying on transitive includes.#include <instantsend/signing.h> #include <unordered_lru_cache.h> +#include <gsl/pointers.h>src/instantsend/instantsend.cpp (1)
69-81: Potential race: start signer before worker thread.
workThreadmay enterWorkThreadMainand callm_signer->ProcessPendingRetryLockTxs()beforem_signer->Start()runs. Start the signer first to avoid TOCTOU between internal queues and thread startup.void CInstantSendManager::Start(PeerManager& peerman) { // can't start new thread if we have one running already if (workThread.joinable()) { assert(false); } - workThread = std::thread(&util::TraceThread, "isman", [this, &peerman] { WorkThreadMain(peerman); }); - - if (m_signer) { - m_signer->Start(); - } + if (m_signer) { + m_signer->Start(); + } + workThread = std::thread(&util::TraceThread, "isman", [this, &peerman] { WorkThreadMain(peerman); }); }src/coinjoin/server.h (1)
10-26: Missing forward declaration for llmq::CInstantSendManager.This header uses llmq::CInstantSendManager by reference (Line 41) but does not forward-declare it and does not include a header that guarantees its declaration. Add a forward decl to keep this header self-sufficient.
class PeerManager; +namespace llmq { +class CInstantSendManager; +} // namespace llmq + class UniValue;src/coinjoin/server.cpp (1)
450-459: Don’t call RelayTransaction under cs_main.Relay under cs_main increases lock-order inversion risk (cs_main → net). Capture the hash, drop cs_main, then relay.
void CCoinJoinServer::ConsumeCollateral(const CTransactionRef& txref) const { - LOCK(::cs_main); - if (!ATMPIfSaneFee(m_chainman, txref)) { - LogPrint(BCLog::COINJOIN, "%s -- ATMPIfSaneFee failed\n", __func__); - } else { - m_peerman.RelayTransaction(txref->GetHash()); - LogPrint(BCLog::COINJOIN, "%s -- Collateral was consumed\n", __func__); - } + bool accepted{false}; + uint256 txhash = txref->GetHash(); + { + LOCK(::cs_main); + accepted = ATMPIfSaneFee(m_chainman, txref); + } + if (!accepted) { + LogPrint(BCLog::COINJOIN, "%s -- ATMPIfSaneFee failed\n", __func__); + return; + } + m_peerman.RelayTransaction(txhash); + LogPrint(BCLog::COINJOIN, "%s -- Collateral was consumed\n", __func__); }src/net_processing.cpp (2)
2886-2894: Same null-deref risk when fetching DSQ for GETDATA.Guard cj_server.
- auto opt_dsq = m_active_ctx ? m_active_ctx->cj_server->GetQueueFromHash(inv.hash) : std::nullopt; + auto opt_dsq = (m_active_ctx && m_active_ctx->cj_server) + ? m_active_ctx->cj_server->GetQueueFromHash(inv.hash) + : std::nullopt;
1959-1970: Update all PeerManager::make invocations with the new context parameters
- src/init.cpp (L2150)
- src/test/net_peer_connection_tests.cpp (L88)
- src/test/denialofservice_tests.cpp (L151, L255, L322, L427)
- src/test/util/setup_common.cpp (L335)
🧹 Nitpick comments (32)
test/util/data/non-backported.txt (1)
1-2: Add src/active/ to non-backported set — LGTM*Patterns look correct and consistent with existing per-extension globs for other dirs.
If desired, add a short comment above explaining that ActiveContext/ActiveNotificationInterface are new-only and intentionally excluded from backports.
src/llmq/ehf_signals.cpp (1)
34-47: Drop masternode guard: safe after verification
- All UpdatedBlockTip call sites use the single-parameter signature and CEHFSignalsHandler is only instantiated in ActiveNotificationInterface.
- Optional refactor: add a nullptr check at the top of UpdatedBlockTip:
void CEHFSignalsHandler::UpdatedBlockTip(const CBlockIndex* const pindexNew) { + if (!pindexNew) return; if (!DeploymentActiveAfter(pindexNew, Params().GetConsensus(), Consensus::DEPLOYMENT_V20)) return; … }src/coinjoin/coinjoin.h (1)
213-215: Doc nit: “valid Masternode address” → “valid signature”CheckSignature verifies the BLS signature against the provided MN pubkey, not an address.
- /// Check if we have a valid Masternode address + /// Verify masternode BLS signature against the provided public key [[nodiscard]] bool CheckSignature(const CBLSPublicKey& blsPubKey) const;src/net.h (2)
1261-1261: Accessor should be [[nodiscard]]Small API nit to discourage ignored calls.
- bool IsActiveMasternode() const { return m_active_masternode; } + [[nodiscard]] bool IsActiveMasternode() const { return m_active_masternode; }
1846-1848: Thread-safety notem_active_masternode is written in Init() and then read concurrently. That’s fine if Init() completes before worker threads start. Please confirm call order; otherwise consider std::atomic.
src/chainlock/chainlock.h (2)
66-67: Chrono literal in a header leaks dependencies.
lastCleanupTime{0s};in a header relies onstd::chrono_literalsbeing in scope transitively. Use explicit construction:- std::atomic<std::chrono::seconds> lastCleanupTime{0s}; + std::atomic<std::chrono::seconds> lastCleanupTime{std::chrono::seconds{0}};
90-92: Avoid passing trivial types by const-ref.
const int64_t& timeshould be by value.- void UpdateTxFirstSeenMap(const std::unordered_set<uint256, StaticSaltedHasher>& tx, const int64_t& time) override + void UpdateTxFirstSeenMap(const std::unordered_set<uint256, StaticSaltedHasher>& tx, int64_t time) overridesrc/chainlock/chainlock.cpp (1)
35-39: Chrono literals withoutstd::chrono_literalsin scope may not compile.Prefer explicit types to avoid reliance on a using-directive:
-static constexpr auto CLEANUP_INTERVAL{30s}; -static constexpr auto CLEANUP_SEEN_TIMEOUT{24h}; -static constexpr auto WAIT_FOR_ISLOCK_TIMEOUT{10min}; +static constexpr auto CLEANUP_INTERVAL{std::chrono::seconds{30}}; +static constexpr auto CLEANUP_SEEN_TIMEOUT{std::chrono::hours{24}}; +static constexpr auto WAIT_FOR_ISLOCK_TIMEOUT{std::chrono::minutes{10}};test/functional/test_framework/p2p.py (1)
148-156: Mapping Dash/q-prefixed messages to None is fine; avoids unknown-message errors in tests.Good defensive expansion of
MESSAGEMAP. Consider a brief comment noting these are placeholders intentionally ignored by the framework to reduce test flakiness.b"clsig": msg_clsig, +b# The following Dash/q-prefixed commands are recognized but intentionally ignored +b# by the framework unless a specific test needs handlers. b"dsa": None,Also applies to: 159-166, 168-188
src/test/util/setup_common.cpp (1)
335-339: Construct CJContext before PeerManager::make
In src/test/util/setup_common.cpp (lines 335–339),m_node.peerman = PeerManager::make(…, m_node.cj_ctx, …)is called beforem_node.cj_ctx = std::make_unique<CJContext>(…)(line 346), resulting in a nullm_cj_ctxinsidePeerManagerImpl. Move the CJContext initialization above the PeerManager::make call, or add null-safe guards aroundm_cj_ctxusage.test/functional/feature_governance.py (1)
366-379: Fix potential late-binding closure on sb_block_height (Ruff B023).Bind the loop variable into the lambda default to avoid late-binding flakiness.
- sb_block_height = 180 + (i + 1) * sb_cycle - self.wait_until(lambda: have_trigger_for_height(self.nodes, sb_block_height)) + sb_block_height = 180 + (i + 1) * sb_cycle + self.wait_until(lambda h=sb_block_height: have_trigger_for_height(self.nodes, h))src/test/denialofservice_tests.cpp (1)
151-155: New PeerManager::make params—sanity check ordering.Calls pass
/*banman=*/nullptrand/*active_ctx=*/nullptrin the new slots; looks correct. If available, consider passing a realbanmanin the first two tests for stronger coverage (optional).Also applies to: 255-259, 324-326
src/governance/object.cpp (1)
247-250: Consider basic validation on signature bytes before use.
SetSignatureblindly stores bytes;CheckSignaturelater callssig.SetBytes(...)without verifying size/validity. Suggest early checks to avoid parsing invalid blobs and clearer error paths:void CGovernanceObject::SetSignature(const std::vector<uint8_t>& sig) { - m_obj.vchSig = sig; + m_obj.vchSig = sig; // store as-is } bool CGovernanceObject::CheckSignature(const CBLSPublicKey& pubKey) const { CBLSSignature sig; - sig.SetBytes(m_obj.vchSig, false); + sig.SetBytes(m_obj.vchSig, false); + // Optionally: if available, assert validity/size of the signature blob. + // if (!sig.IsValid()) return false; if (!sig.VerifyInsecure(pubKey, GetSignatureHash(), false)) { LogPrintf("CGovernanceObject::CheckSignature -- VerifyInsecure() failed\n"); return false; } return true; }src/masternode/node.h (3)
55-56: Document call context for UpdatedBlockTipSince this is no longer an override, briefly document expected caller/thread (ActiveNotificationInterface) and lock expectations (already encoded by EXCLUSIVE_LOCKS_REQUIRED(!cs)) to prevent misuse.
69-70: Prefer fixed-size return type for BLS signature bytesstd::vector<uint8_t> hides the 96-byte invariant. Consider std::array<uint8_t, 96> for type safety and zero-allocation.
- [[nodiscard]] std::vector<uint8_t> SignBasic(const uint256& hash) const EXCLUSIVE_LOCKS_REQUIRED(!cs); + [[nodiscard]] std::array<uint8_t, 96> SignBasic(const uint256& hash) const EXCLUSIVE_LOCKS_REQUIRED(!cs);
30-31: Trim unused ValidationInterface include; registration wiring verified
ActiveNotificationInterfaceis correctly registered and unregistered ininit.cpp(around lines 2174–2175 and 325–327).- Remove
#include <validationinterface.h>fromsrc/masternode/node.hif that header is no longer needed.src/active/notificationinterface.h (2)
8-9: Include for std::unique_ptr in headerAvoid relying on transitive includes.
#include <validationinterface.h> +#include <memory>
23-28: Clarify lifetime/ownership expectationsThis class stores references to ActiveContext and CActiveMasternodeManager. Add a brief comment noting they must outlive g_active_notification_interface and that registration with the ValidationInterface occurs after construction and before destruction.
src/node/interfaces.cpp (2)
150-157: processVoteAndRelay path looks good; consider clearer error on missing connman.If connman is null, the error reads “Governance manager not available,” which can mislead. Prefer a distinct message for missing connman.
- error = "Governance manager not available"; + error = context().govman == nullptr ? "Governance manager not available" : "Connection manager not available";
240-241: AddGovernanceObject no longer relays immediately—document asynchronous relay.Since Relay() is gone, RPC clients may observe a slight delay before network propagation. Consider a brief comment or RPC help note to set expectations.
src/active/context.cpp (1)
39-43: Destructor symmetry looks good; consider idempotence.Disconnects are symmetrical. If future code allows multiple Connect/Disconnect cycles, make sure handlers are idempotent or guard against double-disconnect.
src/active/context.h (3)
41-47: Document lifetime and (dis)connection invariants.Please add a brief comment stating that cl_signer/is_signer are constructed, connected in the ctor, and always disconnected in the dtor, and that they must outlive any LLMQ-registered callbacks. This will help future refactors avoid accidental reorderings.
49-55: Explicitly delete copy/move assignment.Given const unique_ptr members, the generated assignment operators are implicitly deleted. Make this explicit for clarity.
ActiveContext() = delete; ActiveContext(const ActiveContext&) = delete; + ActiveContext& operator=(const ActiveContext&) = delete; + ActiveContext(ActiveContext&&) = delete; + ActiveContext& operator=(ActiveContext&&) = delete;
62-65: Consider switching public owning pointers to references once wiring stabilizes.cj_server, gov_signer, and ehf_sighandler are exposed as const unique_ptr. If ActiveContext is the owner and these are always present, prefer references to make ownership/lifetime explicit (aligns with the TODO on Line 38).
src/coinjoin/server.h (1)
32-42: Ref-to-non-owned members: document lifetime expectations.m_peerman and m_mn_activeman are now non-owning references. Please document, in-class or in the constructor comment, that the referenced objects must outlive CCoinJoinServer. This prevents accidental use-after-lifetime later.
Also applies to: 95-112
src/coinjoin/server.cpp (3)
496-503: Relay DSQ after updating vecCoinJoinQueue.Push to vecCoinJoinQueue under lock first, then relay. This ensures local state reflects what was announced.
- dsq.vchSig = m_mn_activeman.SignBasic(dsq.GetSignatureHash()); - m_peerman.RelayDSQ(dsq); - WITH_LOCK(cs_vecqueue, vecCoinJoinQueue.push_back(dsq)); + dsq.vchSig = m_mn_activeman.SignBasic(dsq.GetSignatureHash()); + WITH_LOCK(cs_vecqueue, vecCoinJoinQueue.push_back(dsq)); + m_peerman.RelayDSQ(dsq);
705-714: Same ordering: enqueue then relay.Mirror the ordering fix in CreateNewSession.
- dsq.vchSig = m_mn_activeman.SignBasic(dsq.GetSignatureHash()); - m_peerman.RelayDSQ(dsq); - LOCK(cs_vecqueue); - vecCoinJoinQueue.push_back(dsq); + dsq.vchSig = m_mn_activeman.SignBasic(dsq.GetSignatureHash()); + { + LOCK(cs_vecqueue); + vecCoinJoinQueue.push_back(dsq); + } + m_peerman.RelayDSQ(dsq);
181-185: Avoid holding cs_vecqueue while calling RelayDSQ
Release cs_vecqueue before m_peerman.RelayDSQ(dsq) to eliminate lock-order inversion risk with PeerManagerImpl’s internal locks.File: src/coinjoin/server.cpp (181–185)
- TRY_LOCK(cs_vecqueue, lockRecv); - if (!lockRecv) return ret; - vecCoinJoinQueue.push_back(dsq); - m_peerman.RelayDSQ(dsq); + { + TRY_LOCK(cs_vecqueue, lockRecv); + if (!lockRecv) return ret; + vecCoinJoinQueue.push_back(dsq); + } + m_peerman.RelayDSQ(dsq);PeerManagerImpl::RelayDSQ is annotated EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex) and does not acquire cs_vecqueue, so moving it outside the lock is safe.
src/net_processing.cpp (1)
2279-2290: Optional helper to centralize CJ server access.To avoid repeating null checks, add a small accessor (e.g., GetCJServer()) returning a raw ptr or nullptr and reuse it at these sites.
Also applies to: 2886-2896, 5281-5287
src/governance/signing.h (1)
47-49: Avoid const-qualified T in std::optional.std::optional hampers assignment/moves and provides no benefit for return-by-value. Prefer std::optional.
- std::optional<const CGovernanceObject> CreateGovernanceTrigger(const std::optional<const CSuperblock>& sb_opt); - std::optional<const CSuperblock> CreateSuperblockCandidate(int nHeight) const; + std::optional<CGovernanceObject> CreateGovernanceTrigger(const std::optional<CSuperblock>& sb_opt); + std::optional<CSuperblock> CreateSuperblockCandidate(int nHeight) const;src/governance/signing.cpp (1)
298-306: Fix clang-format diff failures.CI reports clang-format differences in this region. Please run:
./contrib/devtools/clang-format-diff.py -p1src/governance/governance.h (1)
392-405: Friendship to GovernanceSigner is justified but minimize exposure.Access is only for cached height/read-only bits; keep it limited and documented to avoid future overreach.
Add a short comment near the friend declaration explaining why it’s needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (55)
src/Makefile.am(4 hunks)src/active/context.cpp(1 hunks)src/active/context.h(1 hunks)src/active/notificationinterface.cpp(1 hunks)src/active/notificationinterface.h(1 hunks)src/chainlock/chainlock.cpp(1 hunks)src/chainlock/chainlock.h(2 hunks)src/coinjoin/coinjoin.cpp(0 hunks)src/coinjoin/coinjoin.h(1 hunks)src/coinjoin/context.cpp(1 hunks)src/coinjoin/context.h(1 hunks)src/coinjoin/server.cpp(9 hunks)src/coinjoin/server.h(2 hunks)src/dsnotificationinterface.cpp(1 hunks)src/dsnotificationinterface.h(0 hunks)src/governance/governance.cpp(25 hunks)src/governance/governance.h(9 hunks)src/governance/object.cpp(1 hunks)src/governance/object.h(1 hunks)src/governance/signing.cpp(1 hunks)src/governance/signing.h(1 hunks)src/governance/vote.cpp(0 hunks)src/governance/vote.h(0 hunks)src/init.cpp(8 hunks)src/instantsend/instantsend.cpp(1 hunks)src/instantsend/instantsend.h(2 hunks)src/llmq/context.cpp(1 hunks)src/llmq/context.h(0 hunks)src/llmq/ehf_signals.cpp(1 hunks)src/llmq/ehf_signals.h(2 hunks)src/masternode/node.cpp(1 hunks)src/masternode/node.h(3 hunks)src/masternode/sync.cpp(2 hunks)src/net.cpp(2 hunks)src/net.h(4 hunks)src/net_processing.cpp(9 hunks)src/net_processing.h(2 hunks)src/node/context.cpp(1 hunks)src/node/context.h(2 hunks)src/node/interfaces.cpp(2 hunks)src/rpc/coinjoin.cpp(2 hunks)src/rpc/governance.cpp(3 hunks)src/test/coinjoin_queue_tests.cpp(1 hunks)src/test/denialofservice_tests.cpp(5 hunks)src/test/net_peer_connection_tests.cpp(2 hunks)src/test/util/setup_common.cpp(2 hunks)src/util/system.cpp(0 hunks)src/util/system.h(0 hunks)src/version.h(0 hunks)src/wallet/coinjoin.cpp(0 hunks)test/functional/feature_governance.py(2 hunks)test/functional/feature_governance_cl.py(2 hunks)test/functional/test_framework/p2p.py(1 hunks)test/lint/lint-circular-dependencies.py(1 hunks)test/util/data/non-backported.txt(1 hunks)
💤 Files with no reviewable changes (9)
- src/util/system.h
- src/util/system.cpp
- src/wallet/coinjoin.cpp
- src/version.h
- src/governance/vote.cpp
- src/llmq/context.h
- src/coinjoin/coinjoin.cpp
- src/dsnotificationinterface.h
- src/governance/vote.h
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be placed in src/bench/ and use nanobench
Applied to files:
test/util/data/non-backported.txt
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
PR: dashpay/dash#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/net_processing.hsrc/instantsend/instantsend.h
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
PR: dashpay/dash#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/ehf_signals.h
📚 Learning: 2025-07-23T09:30:34.631Z
Learnt from: kwvg
PR: dashpay/dash#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-07-29T14:32:48.369Z
Learnt from: kwvg
PR: dashpay/dash#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
🪛 Ruff (0.12.2)
test/functional/feature_governance.py
373-373: Function definition does not bind loop variable sb_block_height
(B023)
🪛 GitHub Actions: Clang Diff Format Check
src/governance/object.cpp
[error] 244-250: Clang format differences found. Run './contrib/devtools/clang-format-diff.py -p1' to apply formatting changes.
src/governance/signing.cpp
[error] 298-306: Clang format differences found. Run './contrib/devtools/clang-format-diff.py -p1' to apply formatting changes.
⏰ 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). (6)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (93)
test/functional/feature_governance_cl.py (1)
112-114: Make waits bounded and align mocktime bump with governance relay tick
At lines 112–114 and 124–126, add an explicittimeout(e.g. 15 s) to yourwait_untilcalls and replace the hard-codedbump_mocktime(10)with the governance relay thread’s scheduling interval. I couldn’t find the relay tick constant in the code—please verify its definition and update these calls accordingly.test/lint/lint-circular-dependencies.py (1)
25-27: Install missing dependency or update cycle-check script
The cycle-detection script errors with “No module named ‘multiprocess’”. To validate current circular dependencies, either:
- Run
pip3 install multiprocessbefore rerunning the script.- Or edit
contrib/devtools/circular-dependencies.pyto use the standard library:# replace this line from multiprocess import Pool # with this from multiprocessing import PoolThen re-run:
cd src python3 ../contrib/devtools/circular-dependencies.py $(git ls-files '*.h' '*.cpp') | sed 's/^Circular dependency: //'src/net.h (1)
1197-1198: Plumbed m_active_masternode into Options/Init — LGTMInitialization copies the flag; usage can gate MN-only behavior elsewhere.
Also applies to: 1235-1236
src/chainlock/chainlock.h (1)
69-71: All CChainLocksHandler constructor call sites updated.src/chainlock/chainlock.cpp (1)
46-48: All CChainLocksHandler constructor calls updated to new signature Verified that the only instantiation in llmq/context.cpp now matches the updated parameters (chainstate, qman, sigman, sporkman, mempool, mn_sync) and no callers pass_sharemanoris_masternodeanymore.src/instantsend/instantsend.h (1)
92-95: Confirm CInstantSendManager constructor call sites updated
Removed CSigSharesManager& and is_masternode; verify all instantiations (global definitions and anynewcalls) use the updated signature.src/instantsend/instantsend.cpp (3)
62-63: LGTM: initializer list update.
m_mn_sync{mn_sync}matches the new ownership model.
52-55: Constructor signature updated—verify all instantiation sites
Definitions in instantsend.h/.cpp now accept only CSigningManager and drop _shareman/is_masternode, but no instantiations of CInstantSendManager were found by grep—manually confirm all call sites (e.g. in active/context.cpp) are updated to the new signature.
919-953: Signer lifecycle correctly ordered; no gating required
ConnectSigneris invoked inActiveContextbefore anyStart()calls (LLMQContext::Start), andDisconnectSigneroccurs inActiveContextdestructor afterLLMQContext::Stop; the original suggestion is not needed.Likely an incorrect or invalid review comment.
test/functional/test_framework/p2p.py (1)
157-157: Approve v2 headers handler wiring
Mappings forgetheaders2/headers2/sendheaders2correctly align withon_getheaders2/on_headers2/on_sendheaders2and their message classes.src/node/context.cpp (1)
7-7: Include of ActiveContext is appropriate.Matches the header’s new
std::unique_ptr<ActiveContext>member usage.src/net.cpp (2)
1974-1978: Update tests to use m_active_masternode Replace anyfMasternodeModetoggles in tests with toggles ofCConnman::m_active_masternode(or its new accessor) to preserve inbound‐gating behavior coverage.
1804-1821: m_active_masternode initialization verified: Options::m_active_masternode defaults to false (net.h 1197), is set from node.mn_activeman in init.cpp 2444, and propagated to CConnman::m_active_masternode in Init (net.h 1235); there are no other assignments or dynamic updates, so eviction protection semantics align with the legacy fMasternodeMode.src/rpc/coinjoin.cpp (1)
5-5: New include dependency is fine.No issues.
src/masternode/node.cpp (1)
279-285: SignBasic helper is a clean addition.Good: lock assertions, non-legacy scheme, and returning serialized bytes.
src/test/net_peer_connection_tests.cpp (1)
88-92: PeerManager::make call updated with active_ctx parameter.Signature update is correct; passing nullptr here is expected in unit tests.
src/test/coinjoin_queue_tests.cpp (1)
36-39: Test updated to use CActiveMasternodeManager::SignBasic.Matches the new API; verification via CheckSignature(pub) is intact.
src/net_processing.h (2)
28-31: Forward-declare ActiveContext: OK.No header include needed; forward decl suffices.
58-67: PeerManager::make signature update validated
All call sites (src/init.cpp and all tests) pass the newactive_ctxargument, and no unguardedactive_ctx->dereferences were found in net_processing.cpp.src/test/util/setup_common.cpp (1)
346-349: CJContext ctor change (added relay_txes): test wiring OK.Passing true makes sense for tests.
src/llmq/ehf_signals.h (2)
16-16: Namespace formatting change only.No action needed.
44-45: All CEHFSignalsHandler::UpdatedBlockTip invocations updated
No remaining call site passes the removed is_masternode flag.src/node/context.h (2)
34-34: Forward declaration looks correct.
struct ActiveContext;here avoids a heavy include in the header. Good.
80-95: Verify destruction order and inclusion for incomplete type.Because
active_ctxis aunique_ptrto an incomplete type in this header, ensure the out-of-line~NodeContext()is defined in a TU that includesactive/context.h. Also confirm thatactive_ctxbeing destroyed before managers is intended (members destroy in reverse order of declaration).src/masternode/sync.cpp (2)
127-132: Behavior tweak on inactivity reset—confirm intent.Reset now only when not a masternode (
!connman.IsActiveMasternode()). This mirrors!fMasternodeMode, but please confirm we don’t want to reset when we are an active MN that slept >1h.
160-160: Inbound skip guard switched to IsActiveMasternode().Condition now skips inbound when we’re an active MN. Looks right; just verify no unintended side effects for non-MN nodes.
test/functional/feature_governance.py (2)
320-322: Log expectation updated—double-check exact message.Make sure the governance code actually emits "VoteGovernanceTriggers --" on this path; otherwise this assertion will flake.
354-357: Mocktime bumps increased—OK.Increasing to 10s aligns with new timing; no issues.
Also applies to: 361-362, 369-370, 375-376
src/Makefile.am (1)
138-140: Build wiring for ActiveContext/Notification/GovernanceSigner looks consistent.Headers and sources are added in the right places. Please confirm new headers use the standard BITCOIN_*-style include guards and have the MIT header.
Also applies to: 213-216, 456-458, 497-497
src/test/denialofservice_tests.cpp (1)
23-24: Include ActiveContext header—good.Needed for the extended PeerManager::make signature.
src/active/notificationinterface.cpp (1)
21-24: Remove the suggested null checks—these pointers can’t be null
Bothehf_sighandlerandgov_signerare declared asconst std::unique_ptr<…>inActiveContext(context.h) and are unconditionally initialized viastd::make_uniquein its constructor (src/active/context.cpp:31–32), so neither can ever be null at runtime.Likely an incorrect or invalid review comment.
src/governance/object.cpp (2)
244-250: Formatting nits—CI flagged clang-format drift.CI reports clang-format differences around this hunk. Please run
./contrib/devtools/clang-format-diff.py -p1.
5-22: No lingering CGovernanceObject::Sign/Relay references. Searches confirmed zero occurrences; old API hooks have been fully removed.src/rpc/governance.cpp (3)
456-459: Good: dropped PeerManager from ProcessVoteAndRelay callPassing only (vote, exception, connman) aligns with the refactor and simplifies RPC paths.
399-399: No action needed: AddGovernanceObject always enqueues for relay and returns void — it never returns a status but, after validity checks, unconditionally appends to m_relay_invs (src/governance/governance.cpp ln 385).
960-966: ProcessVoteAndRelay behavior verified
ProcessVoteAndRelay only uses connman to request missing objects in the orphan-vote path, and since ProcessVote returns false for already seen votes, each vote is enqueued exactly once for relay.src/active/notificationinterface.h (1)
30-31: g_active_notification_interface lifecycle ordering is correct
Constructed in init.cpp (lines 2169–2175) before its RegisterValidationInterface call and unregistered/reset in init.cpp (lines 325–327) prior to UnregisterAllValidationInterfaces (line 394).src/llmq/context.cpp (1)
37-40: Verified constructor parameters and usages match updated signatures. Context.cpp’s CChainLocksHandler and CInstantSendManager instantiations align with their headers, and no obsolete call sites were found.src/governance/object.h (1)
217-219: SetSignature API: avoid copies and enforce type/size
- Change
SetSignatureto accept a dedicated BLS signature type or aSpan<const uint8_t>instead ofstd::vector<uint8_t>to enforce compile-time type and size guarantees.- Inside
SetSignature, acquire the class mutex (cs) and perform a cheap runtime check thatsig.size()matches the expected BLS signature length.Diff options:
- void SetSignature(const std::vector<uint8_t>& sig); +class CBLSSignature; + void SetSignature(const CBLSSignature& sig);or
- void SetSignature(const std::vector<uint8_t>& sig); +#include <span.h> + void SetSignature(Span<const uint8_t> sig);Current
rgsearch forCGovernanceObject::(Sign|Relay)returned no matches—please manually verify that no callers remain before removing those methods.src/coinjoin/context.cpp (2)
10-10: Include looks appropriate.coinjoin/coinjoin.h is a reasonable dependency here and matches the new wiring.
22-23: LGTM: dstxman ownership and construction are clear.src/active/context.cpp (2)
35-37: Balanced ConnectSigner calls — good.Connects CL and IS signers after successful construction of all members; avoids partial-init pitfalls.
24-28: No change needed—ctor signature includesclhandleras second parameter.
TheInstantSendSignerconstructor is declared insrc/instantsend/signing.h(lines 72–75) asInstantSendSigner(CChainState& chainstate, llmq::CChainLocksHandler& clhandler, InstantSendSignerParent& isman, llmq::CSigningManager& sigman, llmq::CSigSharesManager& shareman, llmq::CQuorumManager& qman, CSporkManager& sporkman, CTxMemPool& mempool, const CMasternodeSync& mn_sync);so passing
*llmq_ctx.clhandlerin that position is correct.Likely an incorrect or invalid review comment.
src/dsnotificationinterface.cpp (1)
93-93: Governance UpdatedBlockTip decoupling LGTM.Calling m_govman.UpdatedBlockTip(pindexNew) aligns with moving relay into an internal periodic thread and drops peerman/mn_activeman coupling.
src/coinjoin/server.cpp (2)
337-345: LGTM: switch to SignBasic and reference-based activeman.The DSTX construction and signature assignment via SignBasic look correct, and the relay path now uses PeerManager by reference.
685-701: LGTM: guard against starting a new session when one is active.The early return on non-zero nSessionID is a good correctness check.
src/coinjoin/context.h (2)
36-44: Ensure full types are included in the TU where the dtor is defined
The out-of-lineCJContext::~CJContext()definition (context.cpp:25) is correct; confirm thatcontext.cppincludes the headers forCoinJoinWalletManager,CCoinJoinClientQueueManager(underENABLE_WALLET), andCDSTXManagerso theunique_ptrmembers refer to complete types at destruction.
33-35: All CJContext call sites use the new signature and no CConnman/PeerManager references remain.src/net_processing.cpp (1)
590-603: Ensure ActiveContext reference lifetimePeerManagerImpl binds a
const std::unique_ptr<ActiveContext>& m_active_ctxtonode.active_ctx. Verify that init.cpp wiring:
- Constructs
node.active_ctxbefore any PeerManagerImpl methods that dereference it (Step 7d runs afterPeerManager::make).- Never calls into ActiveContext after
node.active_ctx.reset()—shutdown resetsnode.active_ctxbeforenode.peerman.reset(), so confirm PeerManagerImpl’s destructor and scheduled tasks don’t use it post-reset.Also apply the same check for
CJContextandLLMQContext(lines 792–799).src/governance/signing.h (2)
36-41: API surface looks good; keep private helpers private.Minimal public API (UpdatedBlockTip) and friend CGovernanceManager align with goals. No issues.
Also applies to: 44-53
40-41: No action required: destructor is defined in signing.cpp (L36).src/governance/signing.cpp (1)
21-21: Fix chrono literal:2hrequires C++14 chrono literals or a safer construction.
constexpr std::chrono::seconds GOVERNANCE_FUDGE_WINDOW{2h};won’t compile unless chrono literals are brought into scope. Prefer explicit construction to avoid globalusing:Apply:
-constexpr std::chrono::seconds GOVERNANCE_FUDGE_WINDOW{2h}; +constexpr std::chrono::seconds GOVERNANCE_FUDGE_WINDOW{std::chrono::hours{2}};Also ensure
<chrono>is included (see include comment below).⛔ Skipped due to learnings
Learnt from: kwvg PR: dashpay/dash#6815 File: src/chainlock/chainlock.h:68-69 Timestamp: 2025-08-13T16:10:32.972Z Learning: The Dash codebase extensively uses std::chrono_literals throughout, including in header files. Chrono literals like 0s, 1min, etc. are acceptable and preferred over explicit constructors like std::chrono::seconds::zero().Learnt from: kwvg PR: dashpay/dash#6815 File: src/chainlock/chainlock.h:68-69 Timestamp: 2025-08-13T16:10:32.972Z Learning: The Dash codebase extensively uses std::chrono_literals throughout, including in header files. The pattern using namespace std::chrono_literals; is used globally in src/util/time.h, and chrono literals like 0s are commonly used for atomic member initialization in headers (e.g., src/net.h). Chrono literals like 0s, 1min, etc. are preferred over explicit constructors.src/governance/governance.h (4)
247-260: Relay-thread fields look good; ensure lifecycle is safe.New
cs_relay,m_relay_interrupt,m_relay_thread, andm_relay_invsare fine. Please verifyStop()interrupts and joinsm_relay_thread, and destructor does not rely on implicit join.Would you like me to scan governance.cpp for
Start/Stopto confirm properjoin()and no lock inversions withcs/cs_relay?
267-269: Explicit Start/Stop API: good separation.Starting/stopping the internal relay thread via
CGovernanceManager::Start/Stopis a clean decoupling fromPeerManager.
284-286: Thread-safety annotation change: ok, but check call sites.Marking methods
EXCLUSIVE_LOCKS_REQUIRED(!cs_relay)is sensible. Please ensure no call sites holdcs_relaywhen calling these; otherwise TSAN will complain.I can generate a repo-wide grep to spot any calls from inside
cs_relaycritical sections if helpful.
329-333: ProcessVoteAndRelay/CheckPostponedObjects gating on!cs_relay: consistent.API simplification (dropping PeerManager param) is fine and lock annotations are coherent.
src/init.cpp (8)
271-272: Stopping governance on shutdown: good.Calling
node.govman->Stop()inPrepareShutdownensures the relay thread is joined before destruction.
325-333: Orderly teardown of ActiveContext/notifications: good.Unregister then reset is correct; avoids callbacks into freed objects.
1707-1709: Creation point of CActiveMasternodeManager: OK; initialization deferred.Instantiation before ThreadImport is fine; just ensure
Init()is invoked later (it is in load thread).
2149-2154: PeerManager::make now takes ActiveContext: verify nullability.At this point
node.active_ctxis still null (created in Step 7d). EnsurePeerManager::maketreats it as optional and does not dereference it.Do you want a quick scan of
net_processing.{h,cpp}to confirm it’s guarded or uses a weak/optional pointer?
2167-2176: ActiveContext/ActiveNotificationInterface creation and registration: solid.Asserts and registration order look right.
2282-2284: Governance relay thread start timing: good.
govman->Start(*peerman)only when valid and after peerman is ready — correct.
2288-2289: CoinJoin server maintenance scheduling: ensureactive_ctxnon-null.Guarded by
if (node.mn_activeman);active_ctxexists in the same branch — good.
2444-2445: Propagate active-masternode flag to CConnman: good.This helps downstream feature gating.
src/governance/governance.cpp (29)
27-27: LGTM! Good addition of thread utilities.The inclusion of
util/thread.his appropriate given the new relay thread implementation.
34-37: LGTM! Proper use of chrono-based constants.The replacement of raw integer constants with proper chrono types improves type safety and code clarity. The values and naming are appropriate for their usage in governance timing logic.
57-59: LGTM! Standard defaulted constructors.The defaulted constructor and destructor are appropriate for the GovernanceStore class.
69-69: LGTM! Constructor parameter update is consistent.The change from reference to unique_ptr parameter is consistent with the AI summary and maintains proper ownership semantics.
79-98: LGTM! Well-implemented relay thread initialization.The Start method properly initializes the governance relay thread with:
- Proper thread safety with interrupt mechanism
- Appropriate sync checks before relaying
- Clean queue processing with contention reduction
- Named thread for debugging ("govrelay")
100-106: LGTM! Clean shutdown implementation.The Stop method properly handles thread cleanup with interrupt and join semantics.
161-166: LGTM! Proper thread-safe postponed object handling.The method correctly adds objects to both the postponed objects map and the relay queue while maintaining thread safety with proper lock assertions.
168-169: LGTM! Simplified ProcessMessage signature.The updated signature with 4 parameters is cleaner and consistent with the new relay model described in the AI summary.
171-171: Good defensive programming with lock assertion.The lock assertion ensures thread safety for the relay mechanism.
263-263: LGTM! Consistent parameter update.The addition of the peer parameter to AddGovernanceObject is consistent with the new signature.
297-297: LGTM! Proper inventory handling.Using emplace_back with the inventory vector is more efficient than previous approaches.
312-314: LGTM! Proper lock assertion for thread safety.The lock assertion ensures the relay queue is properly protected.
330-330: LGTM! Consistent relay queue usage.The vote relay is properly queued using the new relay mechanism.
339-341: LGTM! Updated method signature with proper lock assertion.The method signature update and lock assertion are consistent with the new relay model.
385-385: LGTM! Governance object relay queuing.The governance object is properly added to the relay queue using the new mechanism.
394-394: LGTM! Orphan vote processing.The CheckOrphanVotes call is consistent with the updated method signature.
454-454: LGTM! Proper use of chrono-based timing.Using
count_seconds(GOVERNANCE_DELETION_DELAY)is consistent with the new chrono-based constants.
478-479: LGTM! Consistent chrono usage in expiration calculation.The use of
count_seconds(GOVERNANCE_DELETION_DELAY)maintains consistency with the new timing constants.
658-658: LGTM! Proper use of chrono constant.Using
RELIABLE_PROPAGATION_TIMEwith the chrono-based time calculation is consistent with the refactoring.
784-784: LGTM! Consistent chrono usage in rate checking.The use of
count_seconds()with the chrono constants maintains consistency throughout the timing logic.
825-825: LGTM! Future deviation check with chrono.The future timestamp validation using
count_seconds(MAX_TIME_FUTURE_DEVIATION)is consistent with the new timing approach.
862-870: LGTM! Proper relay integration in vote processing.The ProcessVoteAndRelay method correctly:
- Asserts lock state for thread safety
- Processes the vote first
- Queues for relay only on success
- Maintains the existing return semantics
899-900: LGTM! Consistent chrono usage in orphan vote timing.The use of
count_seconds()withGOVERNANCE_ORPHAN_EXPIRATION_TIMEis consistent with the chrono refactoring.
926-928: LGTM! Proper lock assertion in postponed object checking.The method signature and lock assertion are consistent with the thread-safe relay model.
944-944: LGTM! Consistent method call.The AddGovernanceObject call without peer parameter is appropriate for internal usage.
971-979: LGTM! Proper chrono usage in timing validation.The timing calculations using
count_seconds()with the chrono constants are consistent and correct for the additional relay logic.
1112-1112: LGTM! Proper masternode connection filtering.The use of
connman.IsActiveMasternode()instead of the previous flag-based approach is consistent with the refactoring mentioned in the AI summary.
1273-1275: LGTM! Updated method signature with proper lock assertion.The UpdatedBlockTip method signature and lock assertion are consistent with the new relay model.
1292-1292: LGTM! Consistent method call.The CheckPostponedObjects call is consistent with the updated method signature.
|
This pull request has conflicts, please rebase. |
kwvg
changed the title
GovernanceSigner, drop Relay()s and use periodic relay thread instead, minor cleanupGovernanceSigner, drop Relay()s and use periodic relay instead, minor cleanup
kwvg
force-pushed
the
gov_split
branch
3 times, most recently
from
f6db09b to
27bd78a
Compare
…er pointers, check before `queueman` access, update P2P message map in tests 24f9357 fix: update P2P message map in functional test framework (Kittywhiskers Van Gogh) a432a95 fix: check if `queueman` is initialized before accessing it (Kittywhiskers Van Gogh) 0444e59 trivial: use `std::atomic` to protect connected manager pointers (Kittywhiskers Van Gogh) b7700b3 trivial: use `std::atomic` to protect connected signer pointers (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependency for #6838 This pull request contains the following: * Minor follow-up to [dash#6828](#6828) based on feedback received during review also extended to similar connections introduced in [dash#5980](#5980) ([commit](a5be37c#diff-c065d4cd2398ad0dbcef393c5dfc53f465bf44723348892395fffd2fb3bac522R350-R355)) and [dash#6030](#6030) ([commit](805537e#diff-59f8e9f1b35c1428332caab753a818e3b2146e73bb6c998a2aed5f7eddbc6fa1R357-R363)) * A bugfix to avoid potential crash caused by missing `nullptr` check after `CCoinJoinClientQueueManager` was conditionally initialized in [dash#5163](#5163) ([commit](2d2814e#diff-b1e19192258d83199d8adaa5ac31f067af98f63554bfdd679bd8e8073815e69dR2308-R2310)) * Updating the Python mapping of Dash-specific P2P messages to avoid unexpected test failures ([build](https://github.com/dashpay/dash/actions/runs/17707917238/job/50323243018#step:6:328)), observed when working on [dash#6838](#6838) ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 24f9357 Tree-SHA512: 90b0b2536a7704e3f3da4ece05b6ad09c393a4348aaff87682b7547f6a7d6ffede504176fa1f63bd9ad88961fb1e3b113aae5df357c0dfb70df2fc55500c2b5f
kwvg
force-pushed
the
gov_split
branch
2 times, most recently
from
4b2c1a5 to
61fdd18
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/net_processing.cpp (1)
2886-2894: Fix potential null dereference: guard cj_server in DSQ GetData path.Same issue here when fetching DSQ by hash.
Apply this diff:
- auto opt_dsq = m_active_ctx ? m_active_ctx->cj_server->GetQueueFromHash(inv.hash) : std::nullopt; + auto opt_dsq = (m_active_ctx && m_active_ctx->cj_server) + ? m_active_ctx->cj_server->GetQueueFromHash(inv.hash) + : std::optional<CCoinJoinQueue>{std::nullopt};
🧹 Nitpick comments (4)
src/governance/signing.h (2)
71-73: Avoid const-qualified optionals to prevent unnecessary copies and assignment issues.Use std::optional instead of std::optional. This simplifies usage and avoids awkward move/assignment constraints.
Apply this diff:
- std::optional<const CGovernanceObject> CreateGovernanceTrigger(const std::optional<const CSuperblock>& sb_opt); - std::optional<const CSuperblock> CreateSuperblockCandidate(int nHeight) const; + std::optional<CGovernanceObject> CreateGovernanceTrigger(const std::optional<CSuperblock>& sb_opt); + std::optional<CSuperblock> CreateSuperblockCandidate(int nHeight) const;Note: Update the corresponding implementations and call sites in signing.cpp accordingly.
47-56: Make GovernanceSigner non-copyable.The class holds references to external managers; copying would be unsafe. Explicitly delete copy/move.
Apply this diff:
class GovernanceSigner { private: + GovernanceSigner(const GovernanceSigner&) = delete; + GovernanceSigner& operator=(const GovernanceSigner&) = delete; + GovernanceSigner(GovernanceSigner&&) = delete; + GovernanceSigner& operator=(GovernanceSigner&&) = delete;src/governance/object.h (1)
217-219: Unify signature byte container type across governance types.
CGovernanceObject::SetSignaturetakesstd::vector<uint8_t>, whileCGovernanceVote::SetSignatureusesstd::vector<unsigned char>. This inconsistency causes needless conversions/friction.- void SetSignature(const std::vector<uint8_t>& sig); + void SetSignature(const std::vector<unsigned char>& sig);Additionally update the corresponding implementation in object.cpp to match.
test/functional/feature_governance.py (1)
320-320: Make the log assertion less brittle.Match just "VoteGovernanceTriggers" to avoid test failures if the log suffix or spacing changes.
File: test/functional/feature_governance.py:320
- with self.nodes[1].assert_debug_log(["VoteGovernanceTriggers --"]): + with self.nodes[1].assert_debug_log(["VoteGovernanceTriggers"]):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
src/Makefile.am(2 hunks)src/coinjoin/coinjoin.cpp(0 hunks)src/coinjoin/coinjoin.h(1 hunks)src/coinjoin/server.cpp(3 hunks)src/dsnotificationinterface.cpp(1 hunks)src/dsnotificationinterface.h(0 hunks)src/governance/governance.cpp(35 hunks)src/governance/governance.h(7 hunks)src/governance/object.cpp(1 hunks)src/governance/object.h(1 hunks)src/governance/signing.cpp(1 hunks)src/governance/signing.h(1 hunks)src/governance/vote.cpp(0 hunks)src/governance/vote.h(0 hunks)src/init.cpp(3 hunks)src/masternode/active/context.cpp(2 hunks)src/masternode/active/context.h(3 hunks)src/masternode/active/notificationinterface.cpp(2 hunks)src/masternode/node.cpp(1 hunks)src/masternode/node.h(1 hunks)src/net_processing.cpp(1 hunks)src/node/interfaces.cpp(3 hunks)src/qt/governancelist.h(2 hunks)src/rpc/governance.cpp(3 hunks)src/test/coinjoin_queue_tests.cpp(1 hunks)src/version.h(0 hunks)src/wallet/interfaces.cpp(0 hunks)test/functional/feature_governance.py(1 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
💤 Files with no reviewable changes (6)
- src/wallet/interfaces.cpp
- src/dsnotificationinterface.h
- src/governance/vote.cpp
- src/coinjoin/coinjoin.cpp
- src/governance/vote.h
- src/version.h
🚧 Files skipped from review as they are similar to previous changes (8)
- src/coinjoin/coinjoin.h
- src/masternode/node.cpp
- src/coinjoin/server.cpp
- src/test/coinjoin_queue_tests.cpp
- test/lint/lint-circular-dependencies.py
- src/rpc/governance.cpp
- src/governance/object.cpp
- src/masternode/node.h
🧰 Additional context used
📓 Path-based instructions (3)
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/feature_governance.py
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/masternode/active/notificationinterface.cppsrc/masternode/active/context.hsrc/init.cppsrc/node/interfaces.cppsrc/governance/signing.cppsrc/qt/governancelist.hsrc/masternode/active/context.cppsrc/governance/object.hsrc/governance/signing.hsrc/governance/governance.hsrc/governance/governance.cppsrc/net_processing.cppsrc/dsnotificationinterface.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 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/masternode/active/context.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#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.
🧬 Code graph analysis (9)
test/functional/feature_governance.py (1)
test/functional/test_framework/test_node.py (1)
assert_debug_log(444-472)
src/masternode/active/context.h (2)
src/governance/governance.cpp (2)
CGovernanceManager(71-87)CGovernanceManager(89-93)src/governance/signing.cpp (2)
GovernanceSigner(23-33)GovernanceSigner(35-35)
src/governance/signing.cpp (6)
src/node/interfaces.cpp (17)
chainman(568-570)chainman(1053-1055)nAmount(375-378)nAmount(375-375)nAmount(391-394)nAmount(391-391)LOCK(533-537)LOCK(543-550)LOCK(551-558)LOCK(820-829)LOCK(863-867)LOCK(1047-1051)tip(97-105)tip(538-542)tip(559-567)vote(149-161)vote(149-149)src/governance/classes.cpp (1)
payment(210-210)src/util/time.h (1)
count_seconds(56-56)src/governance/governance.cpp (2)
UpdatedBlockTip(1322-1344)UpdatedBlockTip(1322-1322)src/dsnotificationinterface.cpp (2)
UpdatedBlockTip(68-95)UpdatedBlockTip(68-68)src/masternode/active/notificationinterface.cpp (2)
UpdatedBlockTip(18-27)UpdatedBlockTip(18-19)
src/qt/governancelist.h (1)
src/evo/deterministicmns.h (1)
CDeterministicMNList(127-188)
src/governance/object.h (2)
src/governance/object.cpp (2)
SetSignature(247-250)SetSignature(247-247)src/governance/vote.h (1)
SetSignature(97-97)
src/governance/signing.h (3)
src/governance/governance.cpp (20)
GetBestSuperblock(1628-1634)GetBestSuperblock(1628-1629)MasternodeRateCheck(836-840)MasternodeRateCheck(836-836)MasternodeRateCheck(842-904)MasternodeRateCheck(842-842)ProcessVoteAndRelay(906-914)ProcessVoteAndRelay(906-906)FindGovernanceObject(573-578)FindGovernanceObject(573-573)FindGovernanceObjectByDataHash(587-595)FindGovernanceObjectByDataHash(587-587)GetActiveTriggers(1553-1558)GetActiveTriggers(1553-1553)GetApprovedProposals(1742-1773)GetApprovedProposals(1742-1743)AddGovernanceObject(382-441)AddGovernanceObject(382-382)UpdatedBlockTip(1322-1344)UpdatedBlockTip(1322-1322)src/governance/signing.cpp (16)
GovernanceSigner(23-33)GovernanceSigner(35-35)UpdatedBlockTip(280-289)UpdatedBlockTip(280-280)HasAlreadyVotedFundingTrigger(270-273)HasAlreadyVotedFundingTrigger(270-270)VoteFundingTrigger(254-268)VoteFundingTrigger(254-254)CreateGovernanceTrigger(120-166)CreateGovernanceTrigger(120-120)CreateSuperblockCandidate(37-118)CreateSuperblockCandidate(37-37)ResetVotedFundingTrigger(275-278)ResetVotedFundingTrigger(275-275)VoteGovernanceTriggers(168-252)VoteGovernanceTriggers(168-168)src/masternode/active/notificationinterface.cpp (2)
UpdatedBlockTip(18-27)UpdatedBlockTip(18-19)
src/governance/governance.h (5)
src/governance/governance.cpp (64)
CGovernanceManager(71-87)CGovernanceManager(89-93)Schedule(95-122)Schedule(95-95)RelayMessage(135-144)RelayMessage(135-135)RelayMessage(146-161)RelayMessage(146-146)ProcessMessage(211-353)ProcessMessage(211-212)FindConstGovernanceObject(563-571)FindConstGovernanceObject(563-563)FindGovernanceObject(573-578)FindGovernanceObject(573-573)FindGovernanceObjectByDataHash(587-595)FindGovernanceObjectByDataHash(587-587)DeleteGovernanceObject(597-604)DeleteGovernanceObject(597-597)GetCurrentVotes(606-644)GetCurrentVotes(606-606)GetAllNewerThan(646-659)GetAllNewerThan(646-646)AddGovernanceObject(382-441)AddGovernanceObject(382-382)CheckAndRemove(443-561)CheckAndRemove(443-443)ToJson(1290-1320)ToJson(1290-1290)UpdatedBlockTip(1322-1344)UpdatedBlockTip(1322-1322)AddPostponedObject(205-209)AddPostponedObject(205-205)MasternodeRateUpdate(814-834)MasternodeRateUpdate(814-814)MasternodeRateCheck(836-840)MasternodeRateCheck(836-836)MasternodeRateCheck(842-904)MasternodeRateCheck(842-842)ProcessVoteAndRelay(906-914)ProcessVoteAndRelay(906-906)CheckPostponedObjects(970-1036)CheckPostponedObjects(970-970)GetActiveTriggers(1553-1558)GetActiveTriggers(1553-1553)GetBestSuperblock(1628-1634)GetBestSuperblock(1628-1629)GetApprovedProposals(1742-1773)GetApprovedProposals(1742-1743)InternalFindGovernanceObject(580-585)InternalFindGovernanceObject(580-580)InternalGetActiveTriggers(1560-1574)InternalGetActiveTriggers(1560-1560)InternalGetBestSuperblock(1636-1667)InternalGetBestSuperblock(1636-1637)ExecuteBestSuperblock(1730-1740)ExecuteBestSuperblock(1730-1730)RequestGovernanceObject(1038-1067)RequestGovernanceObject(1038-1038)AddInvalidVote(1069-1072)AddInvalidVote(1069-1069)ProcessVote(916-968)ProcessVote(916-916)CheckOrphanVotes(355-380)CheckOrphanVotes(355-355)src/governance/object.cpp (8)
CGovernanceObject(25-29)CGovernanceObject(31-38)CGovernanceObject(40-56)obj(270-270)ToJson(352-355)ToJson(352-352)ProcessVote(58-167)ProcessVote(58-59)src/governance/vote.cpp (3)
CGovernanceVote(84-93)IsValid(155-185)IsValid(155-155)src/governance/signing.cpp (4)
GovernanceSigner(23-33)GovernanceSigner(35-35)UpdatedBlockTip(280-289)UpdatedBlockTip(280-280)src/net_processing.cpp (24)
scheduler(624-624)scheduler(675-675)ProcessMessage(3527-5315)ProcessMessage(3527-3532)peer(680-680)peer(810-810)peer(915-915)peer(916-916)peer(1023-1023)peer(1031-1031)pfrom(618-618)pfrom(639-640)pfrom(734-736)pfrom(742-742)pfrom(750-750)pfrom(753-753)pfrom(755-755)pfrom(757-757)pfrom(851-851)pfrom(1033-1033)UpdatedBlockTip(2122-2157)UpdatedBlockTip(2122-2122)pindex(613-613)pindex(849-849)
src/governance/governance.cpp (3)
src/net_processing.cpp (21)
scheduler(624-624)scheduler(675-675)inv(632-632)inv(633-633)inv(652-652)inv(658-658)inv(941-941)inv(2320-2320)ProcessMessage(3527-5315)ProcessMessage(3527-3532)pfrom(618-618)pfrom(639-640)pfrom(734-736)pfrom(742-742)pfrom(750-750)pfrom(753-753)pfrom(755-755)pfrom(757-757)pfrom(851-851)pfrom(1033-1033)ret(666-666)src/governance/governance.h (1)
cs_relay(270-270)src/util/time.h (1)
count_seconds(56-56)
src/dsnotificationinterface.cpp (1)
src/net_processing.cpp (1)
pindexNew(609-609)
🪛 GitHub Actions: Clang Diff Format Check
src/governance/signing.cpp
[error] 267-267: Clang format differences found. Run the formatting script to fix code style issues.
src/governance/governance.cpp
[error] 1066-1066: Clang format differences found. Run the formatting script to fix code style issues.
🔇 Additional comments (28)
src/net_processing.cpp (3)
5283-5286: Guard optional CJ server before dereference.m_active_ctx may exist while cj_server is null; this can crash.
Apply this diff:
- if (m_active_ctx) { - PostProcessMessage(m_active_ctx->cj_server->ProcessMessage(pfrom, msg_type, vRecv), pfrom.GetId()); - } + if (m_active_ctx && m_active_ctx->cj_server) { + PostProcessMessage(m_active_ctx->cj_server->ProcessMessage(pfrom, msg_type, vRecv), pfrom.GetId()); + }
2279-2284: Fix potential null dereference: guard cj_server in DSQ AlreadyHave path.Current code dereferences cj_server without checking it.
Apply this diff:
case MSG_DSQ: return #ifdef ENABLE_WALLET (m_cj_ctx->queueman && m_cj_ctx->queueman->HasQueue(inv.hash)) || #endif // ENABLE_WALLET - (m_active_ctx && m_active_ctx->cj_server->HasQueue(inv.hash)); + (m_active_ctx && m_active_ctx->cj_server && m_active_ctx->cj_server->HasQueue(inv.hash));
5288-5289: LGTM: governance message processing integrated via PostProcessMessage.The new call aligns with the refactored governance pipeline.
src/masternode/active/notificationinterface.cpp (1)
7-7: LGTM: correct include for new signer.
#include <governance/signing.h>is appropriate for invoking signer hooks.src/dsnotificationinterface.cpp (1)
93-94: LGTM: governance tip update routed via CGovernanceManager.Simplified
UpdatedBlockTip(pindexNew)aligns with centralized relay/scheduler changes.src/node/interfaces.cpp (2)
151-154: API update: PeerManager removed from vote relay path — good.Call now matches
CGovernanceManager::ProcessVoteAndRelay(vote, exception, connman).
241-243: Resolved — scheduler uses 1s on mockable chains and RelayMessage already usedCGovernanceManager::Schedule uses Params().IsMockableChain() ? std::chrono::seconds{1} : std::chrono::seconds{5} and CGovernanceManager::AddGovernanceObject calls RelayMessage(govobj); interfaces.cpp already calls RelayMessage for postponed objects — no change required.
src/qt/governancelist.h (1)
37-39: No change needed — forward-declared enum matches its definition.
Definition found at src/governance/vote.h:22 asenum vote_outcome_enum_t : int, so the forward declaration is correct.src/Makefile.am (1)
210-214: Confirm governance/signing.h exists and public exposure is intentional.governance/signing.cpp is added to sources (src/Makefile.am:496); governance/signing.h is listed in BITCOIN_CORE_H (src/Makefile.am:210-214) but the header file wasn't found in the repo — add the header if it should be public or remove it from BITCOIN_CORE_H.
src/masternode/active/context.h (1)
16-17: Constructor/API updates — verified: destructor visibility, callsites, and gov_signer usage are OK.
- ~ActiveContext is defined in src/masternode/active/context.cpp which includes <governance/signing.h> — destructor visibility satisfied.
- The ctor now takes CGovernanceManager&; the ActiveContext construction in src/init.cpp passes *node.govman (no other ctor callsites found).
- gov_signer is constructed unconditionally in src/masternode/active/context.cpp, so the dereference in src/masternode/active/notificationinterface.cpp is safe.
src/masternode/active/context.cpp (1)
10-11: LGTM: GovernanceSigner wiring added correctlyIncludes and GovernanceSigner construction look consistent with the broader refactor.
Also applies to: 31-33
src/init.cpp (3)
2155-2157: Verify DSNotificationInterface arg types and init orderYou pass node.llmq_ctx and node.cj_ctx before cj_ctx is created (created at Line 2162). If CDSNotificationInterface stores references to the unique_ptrs, this is fine; if it snapshots raw pointers, UpdatedBlockTip may hit the assert (m_cj_ctx && m_llmq_ctx). Confirm the constructor expects references to unique_ptrs (or a lazily-populated pointer) and not dereferenced objects.
2170-2173: LGTM: ActiveContext ctor updatesActiveContext construction matches the new signature including govman, and the arg order looks correct.
2281-2283: LGTM: Governance schedulingScheduling governance via CGovernanceManager::Schedule gated on IsValid() is appropriate.
src/governance/governance.cpp (3)
702-709: Type-safety note: m_requested_hash_time value typeGood use of std::chrono::seconds for validity. No action needed.
1066-1066: Run clang-format to satisfy CIClang format differences are reported for this file. Please run the repo’s formatting script.
27-40: Fix chrono literal usage and missing header10min/1h/1min require std::chrono literals or explicit types. Add and use std::chrono::minutes/hours to avoid namespace pollution.
Apply:
+#include <chrono> @@ -constexpr std::chrono::seconds GOVERNANCE_DELETION_DELAY{10min}; -constexpr std::chrono::seconds GOVERNANCE_ORPHAN_EXPIRATION_TIME{10min}; -constexpr std::chrono::seconds MAX_TIME_FUTURE_DEVIATION{1h}; -constexpr std::chrono::seconds RELIABLE_PROPAGATION_TIME{1min}; +constexpr std::chrono::seconds GOVERNANCE_DELETION_DELAY{std::chrono::minutes{10}}; +constexpr std::chrono::seconds GOVERNANCE_ORPHAN_EXPIRATION_TIME{std::chrono::minutes{10}}; +constexpr std::chrono::seconds MAX_TIME_FUTURE_DEVIATION{std::chrono::hours{1}}; +constexpr std::chrono::seconds RELIABLE_PROPAGATION_TIME{std::chrono::minutes{1}};⛔ Skipped due to learnings
Learnt from: kwvg PR: dashpay/dash#6815 File: src/chainlock/chainlock.h:68-69 Timestamp: 2025-08-13T16:10:32.972Z Learning: The Dash codebase extensively uses std::chrono_literals throughout, including in header files. The pattern using namespace std::chrono_literals; is used globally in src/util/time.h, and chrono literals like 0s are commonly used for atomic member initialization in headers (e.g., src/net.h). Chrono literals like 0s, 1min, etc. are preferred over explicit constructors.Learnt from: kwvg PR: dashpay/dash#6815 File: src/chainlock/chainlock.h:68-69 Timestamp: 2025-08-13T16:10:32.972Z Learning: The Dash codebase extensively uses std::chrono_literals throughout, including in header files. Chrono literals like 0s, 1min, etc. are acceptable and preferred over explicit constructors like std::chrono::seconds::zero().src/governance/signing.cpp (10)
267-267: Run clang-format to satisfy CIClang format differences are reported for this file. Please run the repo’s formatting script.
5-13: Add required standard headersstd::sort/std::ranges and std::chrono usage require and .
Apply:
#include <governance/signing.h> +#include <algorithm> +#include <chrono>
19-21: Use explicit chrono type, not bare 2h literalDefine GOVERNANCE_FUDGE_WINDOW using std::chrono::hours for portability.
Apply:
-constexpr std::chrono::seconds GOVERNANCE_FUDGE_WINDOW{2h}; +constexpr std::chrono::seconds GOVERNANCE_FUDGE_WINDOW{std::chrono::hours{2}};
56-60: Replace magic per-block time with consensus spacingAvoid 2.62*60 float math; use consensus nPowTargetSpacing.
Apply:
- auto SBEpochTime = static_cast<int64_t>(GetTime<std::chrono::seconds>().count() + - (nNextSuperblock - nHeight) * 2.62 * 60); + const int64_t now = GetTime(); + const int64_t spacing = Params().GetConsensus().nPowTargetSpacing; // seconds per block + const int64_t SBEpochTime = now + (nNextSuperblock - nHeight) * spacing;
84-86: Fix UniValue gettersUse get_int64() instead of non-existent getInt<int64_t>().
Apply:
- int64_t windowStart = jproposal["start_epoch"].getInt<int64_t>() - count_seconds(GOVERNANCE_FUDGE_WINDOW); - int64_t windowEnd = jproposal["end_epoch"].getInt<int64_t>() + count_seconds(GOVERNANCE_FUDGE_WINDOW); + int64_t windowStart = jproposal["start_epoch"].get_int64() - count_seconds(GOVERNANCE_FUDGE_WINDOW); + int64_t windowEnd = jproposal["end_epoch"].get_int64() + count_seconds(GOVERNANCE_FUDGE_WINDOW);
89-95: Correct 64-bit logging format specifiersUse %lld and cast to long long for int64_t values (or PRId64 with <inttypes.h>).
Apply:
- LogPrint(BCLog::GOBJECT, "%s -- nHeight:%d SB:%d windowStart:%d\n", __func__, nHeight, SBEpochTime, - windowStart); + LogPrint(BCLog::GOBJECT, "%s -- nHeight:%d SB:%lld windowStart:%lld\n", __func__, nHeight, + (long long)SBEpochTime, (long long)windowStart); @@ - LogPrint(BCLog::GOBJECT, "%s -- nHeight:%d SB:%d windowEnd:%d\n", __func__, nHeight, SBEpochTime, windowEnd); + LogPrint(BCLog::GOBJECT, "%s -- nHeight:%d SB:%lld windowEnd:%lld\n", __func__, nHeight, + (long long)SBEpochTime, (long long)windowEnd);
126-137: Avoid double-locking cs_mainAlready holding ::cs_main; remove the nested WITH_LOCK.
Apply:
- LOCK(::cs_main); + LOCK(::cs_main); @@ - const CBlockIndex* tip = WITH_LOCK(::cs_main, return m_chainman.ActiveChain().Tip()); + const CBlockIndex* tip = m_chainman.ActiveChain().Tip();
184-197: Fix ranges algorithm qualifierUse std::ranges::any_of (C++20) or std::any_of; current ranges::any_of is invalid and will not compile.
Apply:
- voted_already = ranges::any_of(voteRecord.mapInstances, [&](const auto& voteInstancePair) { + voted_already = std::ranges::any_of(voteRecord.mapInstances, [&](const auto& voteInstancePair) {
216-218: Null-deref risk on govobjCheck govobj for nullptr before dereferencing.
Apply:
- const auto govobj = m_govman.FindGovernanceObject(trigger->GetGovernanceObjHash()); - const uint256 trigger_hash = govobj->GetHash(); + const auto govobj = m_govman.FindGovernanceObject(trigger->GetGovernanceObjHash()); + if (!govobj) { + LogPrint(BCLog::GOBJECT, "%s -- Skipping trigger with unknown govobj:%s\n", + __func__, trigger->GetGovernanceObjHash().ToString()); + continue; + } + const uint256 trigger_hash = govobj->GetHash();
232-242: Fix second ranges callSame as above: std::ranges::any_of (or std::any_of with iterators).
Apply:
- if (ranges::any_of(voteRecord.mapInstances, [&](const auto& voteInstancePair) { + if (std::ranges::any_of(voteRecord.mapInstances, [&](const auto& voteInstancePair) { if (voteInstancePair.first == VOTE_SIGNAL_FUNDING) { LogPrint(BCLog::GOBJECT, /* Continued */ "%s -- Not voting NO-FUNDING for trigger:%s, we voted %s for it already\n", strFunc, trigger_hash.ToString(), CGovernanceVoting::ConvertOutcomeToString(voteInstancePair.second.eOutcome)); return true; } return false; - })) { + })) {src/governance/governance.h (1)
269-271: LGTM: Relay queue guarded by its own mutexDedicated cs_relay and guarded vector is a clean separation to avoid contention with main cs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/masternode/active/context.cpp (1)
31-31: GovernanceSigner lifecycle/registration.gov_signer is instantiated but there’s no explicit Connect/Disconnect like cl_signer/is_signer. If GovernanceSigner doesn’t self-register and unregister with CGovernanceManager, consider mirroring the explicit pattern to avoid dangling callbacks.
If explicit wiring is desired and API exists, one approach:
class ActiveContext { - gov_signer{std::make_unique<GovernanceSigner>(connman, dmnman, govman, mn_activeman, chainman, mn_sync)}, + gov_signer{std::make_unique<GovernanceSigner>(connman, dmnman, govman, mn_activeman, chainman, mn_sync)}, @@ { m_llmq_ctx.clhandler->ConnectSigner(cl_signer.get()); m_llmq_ctx.isman->ConnectSigner(is_signer.get()); + // If available: + // govman.ConnectSigner(gov_signer.get()); } ActiveContext::~ActiveContext() { m_llmq_ctx.clhandler->DisconnectSigner(); m_llmq_ctx.isman->DisconnectSigner(); + // If available: + // govman.DisconnectSigner(); }If the Connect/Disconnect API is not available, please confirm GovernanceSigner self‑registers and cleans up on destruction. Based on learnings.
src/init.cpp (1)
2282-2283: Good: governance relay schedulinggovman->Schedule(scheduler, connman, peerman) centralizes relay. Consider logging a one‑time message when scheduling to aid debugging network propagation.
src/governance/signing.h (1)
71-75: Avoid optional in public APIstd::optional<const CGovernanceObject/CSuperblock> complicates use (move/assignment). Prefer std::optional and std::optional.
- std::optional<const CGovernanceObject> CreateGovernanceTrigger(const std::optional<const CSuperblock>& sb_opt); - std::optional<const CSuperblock> CreateSuperblockCandidate(int nHeight) const; + std::optional<CGovernanceObject> CreateGovernanceTrigger(const std::optional<CSuperblock>& sb_opt); + std::optional<CSuperblock> CreateSuperblockCandidate(int nHeight) const;src/governance/governance.cpp (4)
36-40: Chrono literals require chrono_literalsYou use 10min/1h/1min. Ensure a using directive exists in this TU, or switch to explicit minutes/hours to avoid build issues across compilers.
-constexpr std::chrono::seconds GOVERNANCE_DELETION_DELAY{10min}; -constexpr std::chrono::seconds GOVERNANCE_ORPHAN_EXPIRATION_TIME{10min}; -constexpr std::chrono::seconds MAX_TIME_FUTURE_DEVIATION{1h}; -constexpr std::chrono::seconds RELIABLE_PROPAGATION_TIME{1min}; +using namespace std::chrono_literals; +constexpr std::chrono::seconds GOVERNANCE_DELETION_DELAY{10min}; +constexpr std::chrono::seconds GOVERNANCE_ORPHAN_EXPIRATION_TIME{10min}; +constexpr std::chrono::seconds MAX_TIME_FUTURE_DEVIATION{1h}; +constexpr std::chrono::seconds RELIABLE_PROPAGATION_TIME{1min};(Or replace literals with std::chrono::minutes{...}/hours{...})
135-161: RelayMessage enqueues invs: good guardDeferring until synced and using cs_relay is sound. Consider batching distinct hashes per type to avoid duplicates before flushing.
702-713: Reliability window uses std::chrono::seconds in map—niceMinor: consider erasing stale entries lazily to cap map size growth under flood.
1742-1773: GetApprovedProposals: include for std::sortFile doesn’t include . Add it to avoid transitive include reliance.
#include <util/ranges.h> +#include <algorithm> // std::sort
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
src/Makefile.am(2 hunks)src/coinjoin/coinjoin.cpp(0 hunks)src/coinjoin/coinjoin.h(1 hunks)src/coinjoin/server.cpp(3 hunks)src/dsnotificationinterface.cpp(1 hunks)src/dsnotificationinterface.h(0 hunks)src/governance/governance.cpp(35 hunks)src/governance/governance.h(7 hunks)src/governance/object.cpp(1 hunks)src/governance/object.h(1 hunks)src/governance/signing.cpp(1 hunks)src/governance/signing.h(1 hunks)src/governance/vote.cpp(0 hunks)src/governance/vote.h(0 hunks)src/init.cpp(3 hunks)src/masternode/active/context.cpp(2 hunks)src/masternode/active/context.h(3 hunks)src/masternode/active/notificationinterface.cpp(2 hunks)src/masternode/node.cpp(1 hunks)src/masternode/node.h(1 hunks)src/net_processing.cpp(1 hunks)src/node/interfaces.cpp(3 hunks)src/qt/governancelist.h(2 hunks)src/rpc/governance.cpp(3 hunks)src/test/coinjoin_queue_tests.cpp(1 hunks)src/version.h(0 hunks)src/wallet/interfaces.cpp(0 hunks)test/functional/feature_governance.py(1 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
💤 Files with no reviewable changes (6)
- src/governance/vote.cpp
- src/version.h
- src/coinjoin/coinjoin.cpp
- src/wallet/interfaces.cpp
- src/governance/vote.h
- src/dsnotificationinterface.h
🚧 Files skipped from review as they are similar to previous changes (8)
- src/masternode/node.h
- src/Makefile.am
- src/coinjoin/coinjoin.h
- src/masternode/node.cpp
- src/test/coinjoin_queue_tests.cpp
- src/governance/object.h
- src/dsnotificationinterface.cpp
- src/node/interfaces.cpp
🧰 Additional context used
📓 Path-based instructions (3)
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/feature_governance.py
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/masternode/active/context.hsrc/qt/governancelist.hsrc/coinjoin/server.cppsrc/masternode/active/context.cppsrc/masternode/active/notificationinterface.cppsrc/net_processing.cppsrc/rpc/governance.cppsrc/governance/signing.cppsrc/governance/signing.hsrc/init.cppsrc/governance/governance.hsrc/governance/object.cppsrc/governance/governance.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/masternode/active/context.hsrc/masternode/active/context.cppsrc/masternode/active/notificationinterface.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
PR: dashpay/dash#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/masternode/active/notificationinterface.cpp
📚 Learning: 2025-07-29T14:32:48.369Z
Learnt from: kwvg
PR: dashpay/dash#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/masternode/active/notificationinterface.cpp
🧬 Code graph analysis (9)
test/functional/feature_governance.py (1)
test/functional/test_framework/test_node.py (1)
assert_debug_log(444-472)
src/masternode/active/context.h (2)
src/governance/governance.cpp (2)
CGovernanceManager(71-87)CGovernanceManager(89-93)src/governance/signing.cpp (2)
GovernanceSigner(23-33)GovernanceSigner(35-35)
src/qt/governancelist.h (1)
src/evo/deterministicmns.h (1)
CDeterministicMNList(128-189)
src/rpc/governance.cpp (1)
src/node/interfaces.cpp (2)
vote(149-161)vote(149-149)
src/governance/signing.cpp (6)
src/node/interfaces.cpp (17)
chainman(576-578)chainman(1061-1063)nAmount(375-378)nAmount(375-375)nAmount(391-394)nAmount(391-391)LOCK(541-545)LOCK(551-558)LOCK(559-566)LOCK(828-837)LOCK(871-875)LOCK(1055-1059)tip(97-105)tip(546-550)tip(567-575)vote(149-161)vote(149-149)src/governance/classes.cpp (1)
payment(210-210)src/util/time.h (1)
count_seconds(56-56)src/governance/governance.cpp (2)
UpdatedBlockTip(1322-1344)UpdatedBlockTip(1322-1322)src/dsnotificationinterface.cpp (2)
UpdatedBlockTip(68-95)UpdatedBlockTip(68-68)src/masternode/active/notificationinterface.cpp (2)
UpdatedBlockTip(18-27)UpdatedBlockTip(18-19)
src/governance/signing.h (3)
src/governance/governance.cpp (20)
GetBestSuperblock(1628-1634)GetBestSuperblock(1628-1629)MasternodeRateCheck(836-840)MasternodeRateCheck(836-836)MasternodeRateCheck(842-904)MasternodeRateCheck(842-842)ProcessVoteAndRelay(906-914)ProcessVoteAndRelay(906-906)FindGovernanceObject(573-578)FindGovernanceObject(573-573)FindGovernanceObjectByDataHash(587-595)FindGovernanceObjectByDataHash(587-587)GetActiveTriggers(1553-1558)GetActiveTriggers(1553-1553)GetApprovedProposals(1742-1773)GetApprovedProposals(1742-1743)AddGovernanceObject(382-441)AddGovernanceObject(382-382)UpdatedBlockTip(1322-1344)UpdatedBlockTip(1322-1322)src/governance/signing.cpp (16)
GovernanceSigner(23-33)GovernanceSigner(35-35)UpdatedBlockTip(280-289)UpdatedBlockTip(280-280)HasAlreadyVotedFundingTrigger(270-273)HasAlreadyVotedFundingTrigger(270-270)VoteFundingTrigger(254-268)VoteFundingTrigger(254-254)CreateGovernanceTrigger(120-166)CreateGovernanceTrigger(120-120)CreateSuperblockCandidate(37-118)CreateSuperblockCandidate(37-37)ResetVotedFundingTrigger(275-278)ResetVotedFundingTrigger(275-275)VoteGovernanceTriggers(168-252)VoteGovernanceTriggers(168-168)src/masternode/active/notificationinterface.cpp (2)
UpdatedBlockTip(18-27)UpdatedBlockTip(18-19)
src/governance/governance.h (1)
src/governance/governance.cpp (62)
CGovernanceManager(71-87)CGovernanceManager(89-93)Schedule(95-122)Schedule(95-95)RelayMessage(135-144)RelayMessage(135-135)RelayMessage(146-161)RelayMessage(146-146)ProcessMessage(211-353)ProcessMessage(211-212)FindConstGovernanceObject(563-571)FindConstGovernanceObject(563-563)FindGovernanceObject(573-578)FindGovernanceObject(573-573)FindGovernanceObjectByDataHash(587-595)FindGovernanceObjectByDataHash(587-587)DeleteGovernanceObject(597-604)DeleteGovernanceObject(597-597)AddGovernanceObject(382-441)AddGovernanceObject(382-382)CheckAndRemove(443-561)CheckAndRemove(443-443)ToJson(1290-1320)ToJson(1290-1290)UpdatedBlockTip(1322-1344)UpdatedBlockTip(1322-1322)AddPostponedObject(205-209)AddPostponedObject(205-205)MasternodeRateUpdate(814-834)MasternodeRateUpdate(814-814)MasternodeRateCheck(836-840)MasternodeRateCheck(836-836)MasternodeRateCheck(842-904)MasternodeRateCheck(842-842)ProcessVoteAndRelay(906-914)ProcessVoteAndRelay(906-906)CheckPostponedObjects(970-1036)CheckPostponedObjects(970-970)GetActiveTriggers(1553-1558)GetActiveTriggers(1553-1553)GetBestSuperblock(1628-1634)GetBestSuperblock(1628-1629)GetApprovedProposals(1742-1773)GetApprovedProposals(1742-1743)InternalFindGovernanceObject(580-585)InternalFindGovernanceObject(580-580)InternalGetActiveTriggers(1560-1574)InternalGetActiveTriggers(1560-1560)InternalGetBestSuperblock(1636-1667)InternalGetBestSuperblock(1636-1637)ExecuteBestSuperblock(1730-1740)ExecuteBestSuperblock(1730-1730)RequestGovernanceObject(1038-1067)RequestGovernanceObject(1038-1038)AddInvalidVote(1069-1072)AddInvalidVote(1069-1069)ProcessVote(916-968)ProcessVote(916-916)AcceptMessage(1191-1202)AcceptMessage(1191-1191)CheckOrphanVotes(355-380)CheckOrphanVotes(355-355)
src/governance/object.cpp (1)
src/governance/vote.h (1)
SetSignature(97-97)
src/governance/governance.cpp (5)
src/net_processing.cpp (29)
inv(632-632)inv(633-633)inv(652-652)inv(658-658)inv(941-941)inv(2320-2320)ProcessMessage(3527-5315)ProcessMessage(3527-3532)peer(680-680)peer(810-810)peer(915-915)peer(916-916)peer(1023-1023)peer(1031-1031)pfrom(618-618)pfrom(639-640)pfrom(734-736)pfrom(742-742)pfrom(750-750)pfrom(753-753)pfrom(755-755)pfrom(757-757)pfrom(851-851)pfrom(1033-1033)UpdatedBlockTip(2122-2157)UpdatedBlockTip(2122-2122)pindex(613-613)pindex(849-849)ret(666-666)src/governance/vote.cpp (2)
IsValid(155-185)IsValid(155-155)src/governance/object.cpp (3)
obj(270-270)ProcessVote(58-167)ProcessVote(58-59)src/util/time.h (1)
count_seconds(56-56)src/dsnotificationinterface.cpp (2)
UpdatedBlockTip(68-95)UpdatedBlockTip(68-68)
⏰ 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: Build slim container / Build container
🔇 Additional comments (50)
src/coinjoin/server.cpp (3)
500-500: Same signing pattern as line 341.This line follows the same refactored signing pattern as line 341. The same error handling considerations apply - if
SignBasiccan fail, the DSQ would be relayed with an invalid signature. See the comment on line 341 for verification steps.
710-710: Consistent refactoring across all signing paths.The signing pattern is consistently applied across all three changed locations (lines 341, 500, 710). The refactoring successfully centralizes signing logic in
SignBasicand removes direct object-based Sign methods.However, ensure that error handling for
SignBasicfailures is appropriate across all call sites. See verification script in the comment on line 341.
341-341: No error handling needed - SignBasic includes validation.The
SignBasicmethod includesassert(sig.IsValid())at line 283 ofsrc/masternode/node.cpp, which ensures that only valid signatures are returned. This assert will catch initialization errors in debug builds. Additionally, all signed objects (DSTX, CCoinJoinQueue, governance votes) are verified viaCheckSignature()methods when received by peers, providing defense-in-depth. This pattern is consistent across the entire codebase—7 call sites use the same approach without explicit error checking.Likely an incorrect or invalid review comment.
test/lint/lint-circular-dependencies.py (1)
50-54: LGTM! Expected circular dependencies correctly documented.The added circular dependency paths accurately reflect the architectural changes from splitting governance signing logic into
GovernanceSignerand introducing relay-queue patterns throughActiveContext.src/qt/governancelist.h (2)
9-9: LGTM! Required include for CKeyID usage.The
pubkey.hinclude is necessary for theCKeyIDtype used in thevotableMasternodesmember at line 63.
14-14: LGTM! Good practice reducing header dependencies.The forward declaration of
vote_outcome_enum_twith explicit underlying type avoids including the fullgovernance/vote.hheader, reducing compilation dependencies. The include reordering and preservedCDeterministicMNListforward declaration maintain proper header hygiene.Also applies to: 37-38
src/governance/object.cpp (1)
247-250: LGTM! Clean separation of signing and storage.Replacing
Sign()withSetSignature()successfully decouples signing logic (now inGovernanceSigner) from signature storage. The implementation correctly stores the provided signature without performing validation, which is handled separately byCheckSignature()(lines 252-261). This aligns with the PR objective of extracting masternode-only logic.src/governance/governance.h (12)
25-52: LGTM!Forward declarations and type aliases are correctly added to support the new relay infrastructure and governance signer integration.
242-242: LGTM!The multiple inheritance pattern correctly establishes
CGovernanceManageras both a data store (GovernanceStore) and a parent interface for governance signing operations (GovernanceSignerParent). This enables the newGovernanceSignerto interact with the manager via the interface.
269-270: LGTM!The relay queue infrastructure (
cs_relaymutex andm_relay_invsqueue) correctly implements the new centralized relay pattern described in the PR objectives. The use ofMutex(non-recursive) is appropriate for this queue protection pattern.
278-278: LGTM!The
Schedulemethod signature correctly establishes the periodic governance tasks infrastructure. The implementation (seen in governance.cpp) appropriately schedules orphan cleanup, relay queue processing, and object maintenance at suitable intervals.
282-286: LGTM!The
IsValid()override andRelayMessageoverloads are correctly declared. TheEXCLUSIVE_LOCKS_REQUIRED(!cs_relay)annotations properly document that callers must not hold the relay lock, which the implementations respect by acquiring it internally.
297-298: LGTM!The
ProcessMessagesignature correctly enforces lock ordering withEXCLUSIVE_LOCKS_REQUIRED(!cs_relay)and uses[[nodiscard]]to ensure error handling. This aligns with the relay-aware processing model.
301-344: LGTM!The method signature updates consistently apply the
overridekeyword andEXCLUSIVE_LOCKS_REQUIREDannotations to support the new relay-aware governance architecture. The lock annotations correctly enforce lock ordering:
- Methods that may relay require
!cs_relay(caller must not hold it)- Methods that query state require
!cs(caller must not hold it)
363-391: LGTM!The trigger and proposal query methods correctly use
overrideandEXCLUSIVE_LOCKS_REQUIRED(!cs)annotations. The implementations properly delegate to internal helpers that hold the lock, and returnshared_ptrfor thread-safe access to governance objects.
395-398: LGTM!The internal helper methods correctly document with
EXCLUSIVE_LOCKS_REQUIRED(cs)that they expect the caller to hold the lock. This is the proper pattern for separating public (locking) from internal (locked) implementations.
404-404: LGTM!The
AddInvalidVotemethod correctly has no lock annotation becausecmapInvalidVotesis aCacheMapwith internal synchronization.
411-411: LGTM!The
EXCLUSIVE_LOCKS_REQUIRED(!cs_relay)annotation onCheckOrphanVotesis correct because the method callsRelayMessage, which also requires the caller not holdcs_relay.
11-21: Add missing standard headers.The header uses
std::optional(line 266) andstd::chrono::seconds(line 264) but does not include<optional>and<chrono>. This can cause compilation failures depending on transitive includes.Apply this diff to add the required headers:
#include <protocol.h> #include <sync.h> #include <governance/signing.h> +#include <chrono> #include <limits> #include <map> #include <memory> +#include <optional> #include <set> #include <string> #include <string_view> #include <vector>src/masternode/active/context.h (3)
16-22: LGTM!Forward declarations are correctly used for
CGovernanceManagerandGovernanceSignerto minimize header dependencies.
62-62: LGTM!The
gov_signermember usesconst std::unique_ptr<GovernanceSigner>which correctly establishes single ownership and prevents pointer reassignment after construction, consistent with other members likecj_serverandehf_sighandler.
50-53: LGTM! Constructor correctly initializes governance signerThe constructor signature properly adds
CGovernanceManager& govman, and the implementation correctly uses it at line 31 ofcontext.cppto initializegov_signerwith all required parameters.src/masternode/active/notificationinterface.cpp (2)
7-7: LGTM!The include for
governance/signing.his correctly added to support thegov_signer->UpdatedBlockTipcall.
26-26: Guard against nullgov_signer.If
gov_signeris not initialized (e.g., in non-masternode mode), this will cause a segmentation fault. Add a nullptr check or ensuregov_signeris always initialized inActiveContext.Apply this diff to add a null check:
m_mn_activeman.UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); m_active_ctx.ehf_sighandler->UpdatedBlockTip(pindexNew); - m_active_ctx.gov_signer->UpdatedBlockTip(pindexNew); + if (m_active_ctx.gov_signer) { + m_active_ctx.gov_signer->UpdatedBlockTip(pindexNew); + }Based on learnings: kwvg may defer this to follow-up consistent with avoiding scope creep in refactoring PRs.
src/rpc/governance.cpp (4)
396-401: LGTM!The
gobject_submithandler correctly uses the new relay API:
RelayMessage(govobj)for postponed objectsAddGovernanceObject(govobj)for validated objects (which internally callsRelayMessage)This eliminates the need for
PeerManagerin this code path, consistent with the PR objectives.
458-458: LGTM!The
ProcessVoteAndRelaycall correctly uses the new signature withoutPeerManager, relying on the internal relay queue mechanism.
964-964: LGTM!The
voterawhandler correctly adopts the newProcessVoteAndRelaysignature withoutPeerManager.
396-964: Governance RPC refactoring successfully removes PeerManager dependency.All governance RPC handlers (
gobject_submit,VoteWithMasternodes,voteraw) correctly adopt the new relay API that eliminates directPeerManagerusage. The relay responsibility is now centralized inCGovernanceManagerviaRelayMessageand the periodic relay queue, as intended by the PR objectives.test/functional/feature_governance.py (1)
320-320: Verify the log message matches the implementation.The test expects
"VoteGovernanceTriggers --"in the debug log. TheGovernanceSigner::VoteGovernanceTriggersmethod usesLogPrint(BCLog::GOBJECT, "%s -- ...", __func__, ...)throughout its implementation, where__func__expands to"VoteGovernanceTriggers". The test framework'sassert_debug_logperforms substring matching, so it will correctly match any log line containing"VoteGovernanceTriggers --"from this function.src/net_processing.cpp (2)
5288-5289: Callsite update matches new CGovernanceManager::ProcessMessage signature.Looks good; relays will flow via PostProcessMessage as intended by the periodic relay refactor.
Please confirm that governance scheduled tasks are started once at init (e.g., CGovernanceManager::StartScheduledTasks or equivalent), so the relay queue is processed on mainnet/test cadence.
5283-5286: Optional guard/assert for cj_server access.If cj_server is always constructed with ActiveContext, no change needed. Otherwise guard or assert to prevent accidental null deref.
- if (m_active_ctx) { - PostProcessMessage(m_active_ctx->cj_server->ProcessMessage(pfrom, msg_type, vRecv), pfrom.GetId()); - } + if (m_active_ctx && m_active_ctx->cj_server) { + PostProcessMessage(m_active_ctx->cj_server->ProcessMessage(pfrom, msg_type, vRecv), pfrom.GetId()); + }src/masternode/active/context.cpp (2)
10-11: Includes for governance look correct.
19-22: Call site already updated correctly.The single
ActiveContextinstantiation atsrc/init.cpp:2170-2172already matches the new 13-parameter constructor signature with all arguments in the correct order. No further updates required.src/init.cpp (2)
2170-2173: ActiveContext wiring LGTMActiveContext now includes govman and updated deps; ordering and derefs look consistent.
2155-2157: Constructor signature verified - code is correctThe constructor expects
const std::unique_ptr<LLMQContext>&andconst std::unique_ptr<CJContext>&, and the call site correctly passesnode.llmq_ctxandnode.cj_ctxby reference without.get(). Whilenode.cj_ctxis null at construction (line 2155) and initialized later (line 2162), this is safe:RegisterValidationInterfaceonly registers the callback without invoking it, andInitializeCurrentBlockTip(which triggers methods that assert both contexts are non-null) is called at line 2353, well aftercj_ctxinitialization.src/governance/signing.cpp (9)
5-13: Missing headers for used symbols (sort, ranges, chrono logs)Include the right headers to avoid transitive‑include breaks.
#include <governance/signing.h> +#include <algorithm> // std::sort +#include <chrono> // chrono literals if used +#include <inttypes.h> // PRId64 if using printf macros +#include <util/ranges.h> // ranges::any_of alias
56-60: Replace magic 2.62*60 with consensus spacing (integer math)Avoid floating rounding and encode network target spacing.
- auto SBEpochTime = static_cast<int64_t>(GetTime<std::chrono::seconds>().count() + - (nNextSuperblock - nHeight) * 2.62 * 60); + const int64_t now = GetTime(); + const int64_t spacing = Params().GetConsensus().nPowTargetSpacing; // seconds + const int64_t SBEpochTime = now + (nNextSuperblock - nHeight) * spacing;
89-95: Fix int64 logging specifiersSBEpochTime/windowStart/windowEnd are int64_t; "%d" is wrong. Use %lld with casts or PRId64.
- LogPrint(BCLog::GOBJECT, "%s -- nHeight:%d SB:%d windowStart:%d\n", __func__, nHeight, SBEpochTime, - windowStart); + LogPrint(BCLog::GOBJECT, "%s -- nHeight:%d SB:%lld windowStart:%lld\n", __func__, nHeight, + (long long)SBEpochTime, (long long)windowStart); ... - LogPrint(BCLog::GOBJECT, "%s -- nHeight:%d SB:%d windowEnd:%d\n", __func__, nHeight, SBEpochTime, windowEnd); + LogPrint(BCLog::GOBJECT, "%s -- nHeight:%d SB:%lld windowEnd:%lld\n", __func__, nHeight, + (long long)SBEpochTime, (long long)windowEnd);
230-242: Same issue: ranges::any_of without headerAs above; include util/ranges.h and keep ranges::any_of, or use std::ranges::any_of with .
84-86: UniValue getter misuse: getInt<int64_t>() is not validUse get_int64().
- int64_t windowStart = jproposal["start_epoch"].getInt<int64_t>() - count_seconds(GOVERNANCE_FUDGE_WINDOW); - int64_t windowEnd = jproposal["end_epoch"].getInt<int64_t>() + count_seconds(GOVERNANCE_FUDGE_WINDOW); + int64_t windowStart = jproposal["start_epoch"].get_int64() - count_seconds(GOVERNANCE_FUDGE_WINDOW); + int64_t windowEnd = jproposal["end_epoch"].get_int64() + count_seconds(GOVERNANCE_FUDGE_WINDOW);
126-137: Double-locking cs_mainYou LOCK(::cs_main) then call WITH_LOCK(::cs_main, ...) causing re-lock on non‑recursive mutex. Get tip directly.
- LOCK(::cs_main); ... - const CBlockIndex* tip = WITH_LOCK(::cs_main, return m_chainman.ActiveChain().Tip()); + LOCK(::cs_main); + const CBlockIndex* tip = m_chainman.ActiveChain().Tip();
216-218: Null deref risk: govobj may be nullptrFindGovernanceObject can return nullptr. Guard before dereference.
- const auto govobj = m_govman.FindGovernanceObject(trigger->GetGovernanceObjHash()); - const uint256 trigger_hash = govobj->GetHash(); + const auto govobj = m_govman.FindGovernanceObject(trigger->GetGovernanceObjHash()); + if (!govobj) { + LogPrint(BCLog::GOBJECT, "%s -- Skipping trigger with unknown govobj:%s\n", + __func__, trigger->GetGovernanceObjHash().ToString()); + continue; + } + const uint256 trigger_hash = govobj->GetHash();
184-197: ranges::any_of without ranges headerEither include util/ranges.h and keep ranges::any_of, or switch to std::ranges::any_of with . Given other files use util/ranges.h, include it here.
- voted_already = ranges::any_of(voteRecord.mapInstances, [&](const auto& voteInstancePair) { + voted_already = ranges::any_of(voteRecord.mapInstances, [&](const auto& voteInstancePair) { ... });(Add #include <util/ranges.h> as above.)
19-21: Chrono literal 2h may not compile without chrono_literalsEither add using namespace std::chrono_literals; in this TU, or use explicit hours.
-constexpr std::chrono::seconds GOVERNANCE_FUDGE_WINDOW{2h}; +using namespace std::chrono_literals; +constexpr std::chrono::seconds GOVERNANCE_FUDGE_WINDOW{2h};Or:
-constexpr std::chrono::seconds GOVERNANCE_FUDGE_WINDOW{2h}; +constexpr std::chrono::seconds GOVERNANCE_FUDGE_WINDOW{std::chrono::hours{2}};⛔ Skipped due to learnings
Learnt from: kwvg PR: dashpay/dash#6815 File: src/chainlock/chainlock.h:68-69 Timestamp: 2025-08-13T16:10:32.972Z Learning: The Dash codebase extensively uses std::chrono_literals throughout, including in header files. The pattern using namespace std::chrono_literals; is used globally in src/util/time.h, and chrono literals like 0s are commonly used for atomic member initialization in headers (e.g., src/net.h). Chrono literals like 0s, 1min, etc. are preferred over explicit constructors.Learnt from: kwvg PR: dashpay/dash#6815 File: src/chainlock/chainlock.h:68-69 Timestamp: 2025-08-13T16:10:32.972Z Learning: The Dash codebase extensively uses std::chrono_literals throughout, including in header files. Chrono literals like 0s, 1min, etc. are acceptable and preferred over explicit constructors like std::chrono::seconds::zero().src/governance/governance.cpp (6)
95-122: Scheduler closures LGTMPeriodic maintenance and relay flushing look correct; intervals match PR intent (1s test, 5s main).
205-210: Postponed map guarded by cs: LGTM
306-307: Relay on accepted objectSwitch to RelayMessage(obj) aligns with centralized relay.
355-374: Orphan vote processing relay path updatedRelaying via queue is consistent; good.
1015-1024: Additional relay timing guards are clearThe valid/ready checks with MAX_TIME_FUTURE_DEVIATION and RELIABLE_PROPAGATION_TIME are reasonable.
1322-1344: UpdatedBlockTip pathwaySetting nCachedBlockHeight before CheckPostponedObjects/ExecuteBestSuperblock is correct ordering.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/governance/signing.h (1)
33-45: Align pointer types with CGovernanceManager (use alias/const)GovernanceManager APIs use CSuperblock_sptr; here you use std::shared_ptr. To avoid override mismatches and enforce const-correctness, change to the same alias/type (likely shared_ptr) for:
- GetBestSuperblock(..., CSuperblock_sptr& ...)
- GetActiveTriggers() -> std::vector<CSuperblock_sptr>
This keeps interfaces consistent across modules.
src/governance/signing.cpp (1)
59-61: Avoid magic constant for per-block timeUse consensus target spacing to compute SB ETA; avoids float rounding and encodes network policy.
- auto SBEpochTime = static_cast<int64_t>(GetTime<std::chrono::seconds>().count() + - (nNextSuperblock - nHeight) * 2.62 * 60); + const int64_t now = GetTime(); + const int64_t spacing = Params().GetConsensus().nPowTargetSpacing; // seconds per block + const int64_t SBEpochTime = now + (nNextSuperblock - nHeight) * spacing;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
src/Makefile.am(2 hunks)src/coinjoin/coinjoin.cpp(0 hunks)src/coinjoin/coinjoin.h(1 hunks)src/coinjoin/server.cpp(3 hunks)src/dsnotificationinterface.cpp(1 hunks)src/dsnotificationinterface.h(0 hunks)src/governance/governance.cpp(35 hunks)src/governance/governance.h(7 hunks)src/governance/object.cpp(1 hunks)src/governance/object.h(1 hunks)src/governance/signing.cpp(1 hunks)src/governance/signing.h(1 hunks)src/governance/vote.cpp(0 hunks)src/governance/vote.h(0 hunks)src/init.cpp(3 hunks)src/masternode/active/context.cpp(2 hunks)src/masternode/active/context.h(3 hunks)src/masternode/active/notificationinterface.cpp(2 hunks)src/masternode/node.cpp(1 hunks)src/masternode/node.h(1 hunks)src/net_processing.cpp(1 hunks)src/node/interfaces.cpp(3 hunks)src/qt/governancelist.h(2 hunks)src/rpc/governance.cpp(3 hunks)src/test/coinjoin_queue_tests.cpp(1 hunks)src/version.h(0 hunks)src/wallet/interfaces.cpp(0 hunks)test/functional/feature_governance.py(1 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
💤 Files with no reviewable changes (6)
- src/governance/vote.h
- src/dsnotificationinterface.h
- src/coinjoin/coinjoin.cpp
- src/wallet/interfaces.cpp
- src/governance/vote.cpp
- src/version.h
🚧 Files skipped from review as they are similar to previous changes (7)
- src/net_processing.cpp
- src/masternode/active/context.h
- src/rpc/governance.cpp
- src/Makefile.am
- src/coinjoin/server.cpp
- src/dsnotificationinterface.cpp
- src/masternode/node.h
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/masternode/active/notificationinterface.cppsrc/coinjoin/coinjoin.hsrc/qt/governancelist.hsrc/node/interfaces.cppsrc/masternode/node.cppsrc/governance/signing.cppsrc/governance/signing.hsrc/init.cppsrc/governance/object.hsrc/test/coinjoin_queue_tests.cppsrc/governance/governance.cppsrc/governance/governance.hsrc/masternode/active/context.cppsrc/governance/object.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/masternode/active/notificationinterface.cppsrc/masternode/node.cppsrc/masternode/active/context.cpp
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/feature_governance.py
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/coinjoin_queue_tests.cpp
🧠 Learnings (8)
📚 Learning: 2025-10-03T11:29:36.332Z
Learnt from: kwvg
PR: dashpay/dash#6838
File: src/governance/signing.cpp:84-85
Timestamp: 2025-10-03T11:29:36.332Z
Learning: After bitcoin#25153 (included in dash#6775), UniValue uses the templated getInt<T>() API instead of the legacy get_int() and get_int64() methods. For example, use jproposal["start_epoch"].getInt<int64_t>() to retrieve int64_t values from UniValue objects.
Applied to files:
src/governance/signing.cpp
📚 Learning: 2025-10-03T11:30:10.665Z
Learnt from: kwvg
PR: dashpay/dash#6838
File: src/governance/signing.cpp:232-242
Timestamp: 2025-10-03T11:30:10.665Z
Learning: The Dash codebase dropped C++17 support in PR #6380 and now requires C++20 or later, as configured in configure.ac. C++20 features, including std::ranges algorithms like std::ranges::any_of, are appropriate and preferred where they improve code clarity.
Applied to files:
src/governance/signing.cpp
📚 Learning: 2025-08-13T16:10:32.972Z
Learnt from: kwvg
PR: dashpay/dash#6815
File: src/chainlock/chainlock.h:68-69
Timestamp: 2025-08-13T16:10:32.972Z
Learning: The Dash codebase extensively uses std::chrono_literals throughout, including in header files. The pattern using namespace std::chrono_literals; is used globally in src/util/time.h, and chrono literals like 0s are commonly used for atomic member initialization in headers (e.g., src/net.h). Chrono literals like 0s, 1min, etc. are preferred over explicit constructors.
Applied to files:
src/governance/signing.cpp
📚 Learning: 2025-08-13T16:10:32.972Z
Learnt from: kwvg
PR: dashpay/dash#6815
File: src/chainlock/chainlock.h:68-69
Timestamp: 2025-08-13T16:10:32.972Z
Learning: The Dash codebase extensively uses std::chrono_literals throughout, including in header files. Chrono literals like 0s, 1min, etc. are acceptable and preferred over explicit constructors like std::chrono::seconds::zero().
Applied to files:
src/governance/signing.cpp
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
PR: dashpay/dash#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/governance/signing.cpp
📚 Learning: 2025-07-29T14:33:01.040Z
Learnt from: kwvg
PR: dashpay/dash#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/governance/signing.cpp
📚 Learning: 2025-10-03T11:27:54.131Z
Learnt from: kwvg
PR: dashpay/dash#6838
File: src/governance/signing.cpp:89-95
Timestamp: 2025-10-03T11:27:54.131Z
Learning: In the Dash codebase, `LogPrint` uses tinyformat internally, which is a type-safe formatting library that handles format specifiers and type conversions automatically. Unlike traditional printf-style functions, tinyformat does not require matching format specifiers (like %lld for int64_t) with argument types, making manual casting unnecessary.
Applied to files:
src/governance/signing.cpp
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/**/*.{cpp,h,cc,cxx,hpp} : Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Applied to files:
src/governance/signing.cpp
🧬 Code graph analysis (10)
src/qt/governancelist.h (1)
src/evo/deterministicmns.h (1)
CDeterministicMNList(129-190)
src/node/interfaces.cpp (2)
src/rest.cpp (1)
context(977-977)src/rpc/governance.cpp (4)
vote(951-951)govobj(112-112)govobj(174-174)govobj(352-352)
src/masternode/node.cpp (2)
src/netbase.h (1)
nodiscard(131-136)src/masternode/node.h (1)
cs(45-80)
src/governance/signing.cpp (6)
src/node/interfaces.cpp (15)
chainman(576-578)chainman(1061-1063)nAmount(375-378)nAmount(375-375)nAmount(391-394)nAmount(391-391)LOCK(541-545)LOCK(551-558)LOCK(559-566)LOCK(828-837)LOCK(871-875)LOCK(1055-1059)tip(97-105)tip(546-550)tip(567-575)src/governance/classes.cpp (1)
payment(210-210)src/util/time.h (1)
count_seconds(56-56)src/governance/governance.cpp (2)
UpdatedBlockTip(1322-1344)UpdatedBlockTip(1322-1322)src/dsnotificationinterface.cpp (2)
UpdatedBlockTip(68-95)UpdatedBlockTip(68-68)src/masternode/active/notificationinterface.cpp (2)
UpdatedBlockTip(18-27)UpdatedBlockTip(18-19)
src/governance/signing.h (2)
src/governance/governance.cpp (20)
GetBestSuperblock(1628-1634)GetBestSuperblock(1628-1629)MasternodeRateCheck(836-840)MasternodeRateCheck(836-836)MasternodeRateCheck(842-904)MasternodeRateCheck(842-842)ProcessVoteAndRelay(906-914)ProcessVoteAndRelay(906-906)FindGovernanceObject(573-578)FindGovernanceObject(573-573)FindGovernanceObjectByDataHash(587-595)FindGovernanceObjectByDataHash(587-587)GetActiveTriggers(1553-1558)GetActiveTriggers(1553-1553)GetApprovedProposals(1742-1773)GetApprovedProposals(1742-1743)AddGovernanceObject(382-441)AddGovernanceObject(382-382)UpdatedBlockTip(1322-1344)UpdatedBlockTip(1322-1322)src/governance/signing.cpp (16)
GovernanceSigner(25-35)GovernanceSigner(37-37)UpdatedBlockTip(288-297)UpdatedBlockTip(288-288)HasAlreadyVotedFundingTrigger(278-281)HasAlreadyVotedFundingTrigger(278-278)VoteFundingTrigger(262-276)VoteFundingTrigger(262-262)CreateGovernanceTrigger(122-168)CreateGovernanceTrigger(122-122)CreateSuperblockCandidate(39-120)CreateSuperblockCandidate(39-39)ResetVotedFundingTrigger(283-286)ResetVotedFundingTrigger(283-283)VoteGovernanceTriggers(170-260)VoteGovernanceTriggers(170-170)
test/functional/feature_governance.py (1)
test/functional/test_framework/test_node.py (1)
assert_debug_log(444-472)
src/governance/object.h (2)
src/governance/object.cpp (2)
SetSignature(247-250)SetSignature(247-247)src/governance/vote.h (1)
SetSignature(97-97)
src/governance/governance.cpp (4)
src/net_processing.cpp (17)
inv(633-633)inv(634-634)inv(653-653)inv(659-659)inv(945-945)inv(2326-2326)peer(681-681)peer(814-814)peer(919-919)peer(920-920)peer(1027-1027)peer(1035-1035)UpdatedBlockTip(2126-2161)UpdatedBlockTip(2126-2126)pindex(614-614)pindex(853-853)ret(667-667)src/governance/governance.h (1)
cs_relay(272-272)src/util/time.h (1)
count_seconds(56-56)src/dsnotificationinterface.cpp (2)
UpdatedBlockTip(68-95)UpdatedBlockTip(68-68)
src/governance/governance.h (3)
src/governance/governance.cpp (64)
CGovernanceManager(71-87)CGovernanceManager(89-93)Schedule(95-122)Schedule(95-95)RelayMessage(135-144)RelayMessage(135-135)RelayMessage(146-161)RelayMessage(146-146)ProcessMessage(211-353)ProcessMessage(211-212)FindConstGovernanceObject(563-571)FindConstGovernanceObject(563-563)FindGovernanceObject(573-578)FindGovernanceObject(573-573)FindGovernanceObjectByDataHash(587-595)FindGovernanceObjectByDataHash(587-587)DeleteGovernanceObject(597-604)DeleteGovernanceObject(597-597)GetCurrentVotes(606-644)GetCurrentVotes(606-606)AddGovernanceObject(382-441)AddGovernanceObject(382-382)CheckAndRemove(443-561)CheckAndRemove(443-443)ToJson(1290-1320)ToJson(1290-1290)UpdatedBlockTip(1322-1344)UpdatedBlockTip(1322-1322)AddPostponedObject(205-209)AddPostponedObject(205-205)MasternodeRateUpdate(814-834)MasternodeRateUpdate(814-814)MasternodeRateCheck(836-840)MasternodeRateCheck(836-836)MasternodeRateCheck(842-904)MasternodeRateCheck(842-842)ProcessVoteAndRelay(906-914)ProcessVoteAndRelay(906-906)CheckPostponedObjects(970-1036)CheckPostponedObjects(970-970)GetActiveTriggers(1553-1558)GetActiveTriggers(1553-1553)GetBestSuperblock(1628-1634)GetBestSuperblock(1628-1629)GetApprovedProposals(1742-1773)GetApprovedProposals(1742-1743)InternalFindGovernanceObject(580-585)InternalFindGovernanceObject(580-580)InternalGetActiveTriggers(1560-1574)InternalGetActiveTriggers(1560-1560)InternalGetBestSuperblock(1636-1667)InternalGetBestSuperblock(1636-1637)ExecuteBestSuperblock(1730-1740)ExecuteBestSuperblock(1730-1730)RequestGovernanceObject(1038-1067)RequestGovernanceObject(1038-1038)AddInvalidVote(1069-1072)AddInvalidVote(1069-1069)ProcessVote(916-968)ProcessVote(916-916)AcceptMessage(1191-1202)AcceptMessage(1191-1191)CheckOrphanVotes(355-380)CheckOrphanVotes(355-355)src/governance/signing.cpp (4)
GovernanceSigner(25-35)GovernanceSigner(37-37)UpdatedBlockTip(288-297)UpdatedBlockTip(288-288)src/net_processing.cpp (24)
scheduler(625-625)scheduler(676-676)ProcessMessage(3617-5405)ProcessMessage(3617-3622)peer(681-681)peer(814-814)peer(919-919)peer(920-920)peer(1027-1027)peer(1035-1035)pfrom(619-619)pfrom(640-641)pfrom(735-737)pfrom(746-746)pfrom(754-754)pfrom(757-757)pfrom(759-759)pfrom(761-761)pfrom(855-855)pfrom(1037-1037)UpdatedBlockTip(2126-2161)UpdatedBlockTip(2126-2126)pindex(614-614)pindex(853-853)
src/governance/object.cpp (1)
src/governance/vote.h (1)
SetSignature(97-97)
🪛 GitHub Actions: Clang Diff Format Check
src/governance/signing.cpp
[error] 1-1: Clang format differences found in signing.cpp. Run clang-format to fix formatting issues.
src/governance/governance.cpp
[error] 1-1: Clang format differences found in governance.cpp. Run clang-format to fix formatting issues.
src/governance/object.cpp
[error] 1-1: Clang format differences found in object.cpp. Run clang-format to fix formatting issues.
⏰ 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: Build container / Build container
🔇 Additional comments (41)
test/lint/lint-circular-dependencies.py (1)
50-54: LGTM! Circular dependency expectations updated correctly.The updated circular dependency paths accurately reflect the governance refactor:
- Two expanded paths for governance/classes involving the new governance/signing module
- New path through masternode/active/context that results from integrating GovernanceSigner
These are documented as expected circular dependencies arising from the architectural changes.
src/coinjoin/coinjoin.h (1)
212-212: LGTM! Sign() method removed as part of signature refactor.The removal of the
Sign()method fromCCoinJoinQueuealigns with the broader refactor where signatures are now provisioned externally viavchSigusingCActiveMasternodeManager::SignBasic(), rather than being generated inline by the object itself.This is consistent with similar changes to
CCoinJoinBroadcastTxand governance objects.test/functional/feature_governance.py (1)
320-320: LGTM! Test updated to match new log format.The expected debug log message has been updated from
CGovernanceManager::VoteGovernanceTriggerstoVoteGovernanceTriggers --to align with the new logging format introduced by the governance refactor.The test logic remains unchanged; only the log message assertion was updated.
src/test/coinjoin_queue_tests.cpp (1)
37-37: LGTM! Test updated to use SignBasic() for signature provisioning.The test now correctly uses
mn_activeman.SignBasic(q.GetSignatureHash())to provision the signature and assigns it directly toq.vchSig, replacing the removedq.Sign(mn_activeman)method.This change aligns with the broader refactor where signatures are embedded directly via the
vchSigfield rather than generated inline by the object.src/masternode/node.cpp (1)
279-285: LGTM! SignBasic() implementation is correct.The new
SignBasic()method provides a clean, non-legacy signing interface that:
- Correctly delegates to
Sign()withis_legacy=false- Maintains proper lock discipline with
AssertLockNotHeld(cs)- Asserts signature validity (appropriate since invalid signatures indicate a critical error)
- Returns the signature as a byte vector for external consumption
This serves as the unified signing pathway for governance and coinjoin components, replacing inline
Sign()methods.src/masternode/active/notificationinterface.cpp (2)
7-7: LGTM! Governance signing header included.The inclusion of
governance/signing.his necessary to support the newgov_signer->UpdatedBlockTip()call added at line 26.
26-26: LGTM! Governance signer integrated into block tip updates.The call to
m_active_ctx.gov_signer->UpdatedBlockTip(pindexNew)correctly integrates governance signing with block tip updates. As confirmed in previous review discussions,gov_signeris always initialized in theActiveContextconstructor, so no null check is needed.The call is appropriately guarded by the same conditions as other updates (not in IBD, new block).
src/qt/governancelist.h (2)
9-14: LGTM! Header organization improved.The changes improve header organization by:
- Explicitly including
pubkey.h(needed forCKeyIDinvotableMasternodes)- Reordering
governance/object.hto follow other includes- Maintaining proper include structure
These changes reduce header coupling while preserving functionality.
37-38: LGTM! Forward declarations reduce header dependencies.Adding forward declarations for
CDeterministicMNListandvote_outcome_enum_treduces compilation dependencies without affecting the public API.This is a good practice that improves build times and reduces coupling.
src/governance/object.h (1)
217-217: LGTM! Signature API simplified and improved.The change from
Sign()toSetSignature(const std::vector<uint8_t>& sig)is a good design improvement that:
- Separates signature production from signature storage
- Allows external callers to use
CActiveMasternodeManager::SignBasic()for signing- Simplifies the governance object API by removing the signing responsibility
- Aligns with similar changes across coinjoin components
This is consistent with the broader refactor toward centralized signing and relay mechanisms.
src/node/interfaces.cpp (2)
151-158: ProcessVoteAndRelay peerman removal looks correctCalling govman->ProcessVoteAndRelay(vote, exception, *connman) matches updated API and centralizes relay.
Also applies to: 160-161
239-243: Good switch to queued relay flowUsing AddPostponedObject + RelayMessage on missing confirmations and AddGovernanceObject otherwise aligns with periodic relay design.
src/governance/object.cpp (1)
1-1: Run clang-format to fix style diffsCI reports clang-format differences for this file. Please run clang-format on src/governance/object.cpp.
src/masternode/active/context.cpp (2)
10-12: Governance dependencies correctly introducedIncluding governance headers here is appropriate for the new signer wiring.
19-23: ActiveContext now owns GovernanceSigner — wiring looks goodConstructor signature and gov_signer initialization match GovernanceSigner(CConnman, DmnMan, GovMan, ActiveMN, Chainman, MnSync).
Also applies to: 31-33
src/init.cpp (3)
2137-2139: Updated CDSNotificationInterface wiring is consistentConstructor args updated to remove peerman/activemn and include govman/chainman; matches new interfaces.
2151-2154: ActiveContext construction updated correctlyPassing govman into ActiveContext and expanded dependency order match the new constructor.
2263-2264: Governance scheduling change LGTMUsing govman->Schedule(scheduler, connman, peerman) meets the periodic relay design and is gated by IsValid().
src/governance/signing.cpp (1)
1-1: Run clang-format to satisfy CICI reports clang-format differences for this file. Please run clang-format on src/governance/signing.cpp.
src/governance/governance.cpp (15)
135-161: LGTM! Relay message queuing is correctly implemented.The RelayMessage overloads properly check sync status, validate masternode existence for votes, and safely queue relay messages using the cs_relay mutex.
205-209: LGTM! AddPostponedObject implementation is correct.The method properly locks the mutex and inserts the governance object into the postponed objects map.
211-353: LGTM! ProcessMessage updated for relay queue model.The method signature correctly removes the PeerManager parameter and uses the new RelayMessage approach for queuing relays instead of direct relay calls.
355-380: LGTM! CheckOrphanVotes updated correctly.The method signature properly removes the PeerManager parameter and uses RelayMessage for vote relay.
382-441: LGTM! AddGovernanceObject updated for relay queue model.The method signature correctly changes to take a nullable peer pointer and uses RelayMessage for object relay instead of direct relay calls.
497-497: LGTM! count_seconds usage is correct.The conversion from chrono::seconds to int64_t using count_seconds() is appropriate for time comparisons.
Also applies to: 521-522
573-585: LGTM! Lock separation pattern is well implemented.The separation of FindGovernanceObject (public, acquires lock) and InternalFindGovernanceObject (internal, requires lock) is a good pattern that clarifies lock requirements.
587-595: LGTM! Lock acquisition is correctly added.The method properly asserts lock is not held and acquires it before accessing mapObjects.
702-702: LGTM! Chrono-based time calculations are correct.All usages of count_seconds() properly convert chrono::seconds to int64_t for time comparisons and calculations.
Also applies to: 828-828, 869-869, 943-944, 1015-1018, 1023-1023
906-914: LGTM! ProcessVoteAndRelay updated correctly.The method properly asserts lock state and uses RelayMessage for vote relay.
970-1036: LGTM! CheckPostponedObjects updated correctly.The method properly asserts lock state and uses the updated AddGovernanceObject and RelayMessage APIs.
1069-1072: LGTM! AddInvalidVote implementation is correct.The method properly inserts the invalid vote into the cache.
1322-1344: LGTM! UpdatedBlockTip signature simplified correctly.The method signature properly removes unnecessary parameters and uses the updated CheckPostponedObjects API.
1458-1458: LGTM! Internal methods and GetApprovedProposals are well implemented.The internal methods pattern is consistently applied throughout. GetApprovedProposals correctly filters and sorts approved proposals by vote threshold and yes votes, with proper tie-breaking logic.
Also applies to: 1500-1500, 1554-1773
95-122: Guarantee valid lifetimes forconnmanandpeermanin scheduled lambdas. Lambdas in CGovernanceManager::Schedule capture these by reference—ensure they outlive all scheduled callbacks or switch to owning captures (e.g.std::shared_ptr) and stop the scheduler before destroying them.src/governance/governance.h (7)
10-10: LGTM! Required headers are now included.The necessary headers
<chrono>and<optional>are now included, addressing previous review feedback. The inclusion of<governance/signing.h>and other standard headers is appropriate for the declarations in this file.Also applies to: 13-13, 15-23
27-27: LGTM! Forward declarations and typedefs are well organized.The forward declarations appropriately reduce header dependencies, and the typedefs provide clear aliases for complex types.
Also applies to: 29-29, 33-34, 39-39, 47-54
244-244: LGTM! Multiple inheritance is appropriate here.Inheriting from both
GovernanceStoreandGovernanceSignerParentaligns with the PR's goal of separating governance signing responsibilities.
271-272: LGTM! Relay infrastructure is well designed.The cs_relay mutex and m_relay_invs vector are properly annotated with thread safety attributes. RelayMessage methods correctly require the relay lock is NOT held to prevent deadlocks.
Also applies to: 280-280, 284-284, 286-287
299-300: LGTM! API signatures are correctly updated.All method signatures properly reflect the new relay queue model, removing direct PeerManager usage. Lock annotations and override keywords are appropriate.
Also applies to: 303-304, 311-312, 318-318, 322-322
335-335: LGTM! Additional API methods are correctly annotated.All method declarations have appropriate lock annotations and override keywords where needed.
Also applies to: 339-339, 343-344, 346-346, 365-365, 389-393
396-400: LGTM! Internal methods are well designed.The separation of public methods (that acquire locks) and internal methods (that require locks to be held) is a clear and maintainable pattern. Lock annotations properly document threading requirements.
Also applies to: 406-406, 413-413
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/governance/signing.cpp (1)
287-296: Potentially stale YES-vote tracking (behavior unchanged).votedFundingYesTriggerHash is only cleared when GetBestSuperblock() succeeds. If that trigger disappears, new trigger creation may be blocked until next success. This mirrors develop; consider a follow-up to clear on missing/invalid trigger.
🧹 Nitpick comments (2)
src/governance/signing.cpp (2)
58-61: Replace magic block-time factor with consensus target spacing.Avoid 2.62*60. Use nPowTargetSpacing to compute SB ETA deterministically.
Apply:
- auto SBEpochTime = static_cast<int64_t>(GetTime<std::chrono::seconds>().count() + - (nNextSuperblock - nHeight) * 2.62 * 60); + const int64_t now = GetTime<std::chrono::seconds>().count(); + const int64_t spacing = Params().GetConsensus().nPowTargetSpacing; // seconds per block + const int64_t SBEpochTime = now + (nNextSuperblock - nHeight) * spacing;
127-139: Avoid re-locking cs_main inside an active cs_main lock.WITH_LOCK(::cs_main, ...) while holding LOCK(::cs_main) is safe (RecursiveMutex) but unnecessary. Use the existing lock for clarity.
Apply:
- LOCK(::cs_main); + LOCK(::cs_main); @@ - const CBlockIndex* tip = WITH_LOCK(::cs_main, return m_chainman.ActiveChain().Tip()); + const CBlockIndex* tip = m_chainman.ActiveChain().Tip();Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
src/Makefile.am(2 hunks)src/coinjoin/coinjoin.cpp(0 hunks)src/coinjoin/coinjoin.h(1 hunks)src/coinjoin/server.cpp(3 hunks)src/dsnotificationinterface.cpp(1 hunks)src/dsnotificationinterface.h(0 hunks)src/governance/governance.cpp(39 hunks)src/governance/governance.h(7 hunks)src/governance/object.cpp(2 hunks)src/governance/object.h(2 hunks)src/governance/signing.cpp(1 hunks)src/governance/signing.h(1 hunks)src/governance/vote.cpp(1 hunks)src/governance/vote.h(0 hunks)src/init.cpp(3 hunks)src/masternode/active/context.cpp(2 hunks)src/masternode/active/context.h(3 hunks)src/masternode/active/notificationinterface.cpp(2 hunks)src/masternode/node.cpp(1 hunks)src/masternode/node.h(1 hunks)src/net_processing.cpp(1 hunks)src/node/interfaces.cpp(3 hunks)src/qt/governancelist.h(2 hunks)src/rpc/governance.cpp(3 hunks)src/test/coinjoin_queue_tests.cpp(1 hunks)src/wallet/interfaces.cpp(0 hunks)test/functional/feature_governance.py(1 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
💤 Files with no reviewable changes (4)
- src/coinjoin/coinjoin.cpp
- src/dsnotificationinterface.h
- src/governance/vote.h
- src/wallet/interfaces.cpp
🚧 Files skipped from review as they are similar to previous changes (7)
- src/masternode/active/notificationinterface.cpp
- src/net_processing.cpp
- test/functional/feature_governance.py
- src/rpc/governance.cpp
- src/Makefile.am
- src/qt/governancelist.h
- test/lint/lint-circular-dependencies.py
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/masternode/node.hsrc/test/coinjoin_queue_tests.cppsrc/masternode/active/context.hsrc/node/interfaces.cppsrc/coinjoin/coinjoin.hsrc/governance/object.hsrc/masternode/node.cppsrc/governance/signing.hsrc/dsnotificationinterface.cppsrc/governance/object.cppsrc/governance/governance.cppsrc/governance/vote.cppsrc/init.cppsrc/governance/signing.cppsrc/coinjoin/server.cppsrc/masternode/active/context.cppsrc/governance/governance.h
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/masternode/node.hsrc/masternode/active/context.hsrc/masternode/node.cppsrc/masternode/active/context.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/coinjoin_queue_tests.cpp
🧠 Learnings (9)
📚 Learning: 2025-10-03T11:29:36.358Z
Learnt from: kwvg
PR: dashpay/dash#6838
File: src/governance/signing.cpp:84-85
Timestamp: 2025-10-03T11:29:36.358Z
Learning: After bitcoin#25153 (included in dash#6775), UniValue uses the templated getInt<T>() API instead of the legacy get_int() and get_int64() methods. For example, use jproposal["start_epoch"].getInt<int64_t>() to retrieve int64_t values from UniValue objects.
Applied to files:
src/governance/signing.cpp
📚 Learning: 2025-10-03T11:30:10.766Z
Learnt from: kwvg
PR: dashpay/dash#6838
File: src/governance/signing.cpp:232-242
Timestamp: 2025-10-03T11:30:10.766Z
Learning: The Dash codebase dropped C++17 support in PR #6380 and now requires C++20 or later, as configured in configure.ac. C++20 features, including std::ranges algorithms like std::ranges::any_of, are appropriate and preferred where they improve code clarity.
Applied to files:
src/governance/signing.cpp
📚 Learning: 2025-08-13T16:10:32.972Z
Learnt from: kwvg
PR: dashpay/dash#6815
File: src/chainlock/chainlock.h:68-69
Timestamp: 2025-08-13T16:10:32.972Z
Learning: The Dash codebase extensively uses std::chrono_literals throughout, including in header files. The pattern using namespace std::chrono_literals; is used globally in src/util/time.h, and chrono literals like 0s are commonly used for atomic member initialization in headers (e.g., src/net.h). Chrono literals like 0s, 1min, etc. are preferred over explicit constructors.
Applied to files:
src/governance/signing.cpp
📚 Learning: 2025-08-13T16:10:32.972Z
Learnt from: kwvg
PR: dashpay/dash#6815
File: src/chainlock/chainlock.h:68-69
Timestamp: 2025-08-13T16:10:32.972Z
Learning: The Dash codebase extensively uses std::chrono_literals throughout, including in header files. Chrono literals like 0s, 1min, etc. are acceptable and preferred over explicit constructors like std::chrono::seconds::zero().
Applied to files:
src/governance/signing.cpp
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
PR: dashpay/dash#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/governance/signing.cpp
📚 Learning: 2025-07-29T14:33:01.040Z
Learnt from: kwvg
PR: dashpay/dash#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/governance/signing.cpp
📚 Learning: 2025-10-03T11:27:54.178Z
Learnt from: kwvg
PR: dashpay/dash#6838
File: src/governance/signing.cpp:89-95
Timestamp: 2025-10-03T11:27:54.178Z
Learning: In the Dash codebase, `LogPrint` uses tinyformat internally, which is a type-safe formatting library that handles format specifiers and type conversions automatically. Unlike traditional printf-style functions, tinyformat does not require matching format specifiers (like %lld for int64_t) with argument types, making manual casting unnecessary.
Applied to files:
src/governance/signing.cpp
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/**/*.{cpp,h,cc,cxx,hpp} : Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Applied to files:
src/governance/signing.cpp
📚 Learning: 2025-07-17T15:48:29.418Z
Learnt from: kwvg
PR: dashpay/dash#6752
File: src/wallet/load.cpp:164-164
Timestamp: 2025-07-17T15:48:29.418Z
Learning: In Dash Core, kwvg prefers using assert() statements to document expected behavior in code paths, especially when the called function also has internal asserts. This is used as a defensive programming pattern rather than requiring conditional error handling.
Applied to files:
src/coinjoin/server.cpp
🔇 Additional comments (47)
src/test/coinjoin_queue_tests.cpp (1)
37-37: LGTM! Test correctly updated for new signing API.The test now uses
SignBasicto generate the signature and directly assigns it tovchSig, which aligns with the new signing pattern introduced across the codebase.src/masternode/node.cpp (1)
279-285: LGTM! SignBasic implementation is correct.The method correctly delegates to
Signwith the non-legacy flag and asserts signature validity before returning the byte vector. The locking pattern is consistent with other methods in the class.src/masternode/node.h (1)
69-69: LGTM! SignBasic declaration is correct.The method signature is consistent with the implementation and follows the same locking pattern as the existing
Signmethod.src/coinjoin/coinjoin.h (1)
210-210: LGTM! Sign method removal aligns with refactoring.The removal of
Signmethods fromCCoinJoinQueueandCCoinJoinBroadcastTxis intentional, as signing is now performed externally usingSignBasicand the signature is directly assigned tovchSig.src/masternode/active/context.h (3)
16-16: LGTM! Forward declarations added correctly.The forward declarations for
CGovernanceManagerandGovernanceSignerare properly placed and support the new governance signing integration.Also applies to: 22-22
50-53: LGTM! Constructor signature updated for governance wiring.The constructor now accepts
CGovernanceManager& govmanwhich enables governance signing functionality to be properly wired into the active masternode context.
62-62: LGTM! GovernanceSigner member added correctly.The
gov_signermember follows the same pattern as other masternode-only components likecj_serverand is appropriately managed as aunique_ptr.src/governance/vote.cpp (1)
12-14: LGTM! Import changes align with method removals.The include updates correctly reflect the removal of
RelayandSignmethods. The removal ofmasternode/node.handnet_processing.his appropriate since those dependencies are no longer needed. The addition oflogging.hsupports logging functionality used elsewhere in the file.src/coinjoin/server.cpp (1)
348-348: LGTM! Signing updated to use SignBasic correctly.All three signing operations now use
SignBasicto generate signatures and directly assign them tovchSig. This is the correct pattern, and theassert(sig.IsValid())inSignBasicprovides adequate protection since assertions are always enabled in Dash Core. Based on learnings.Also applies to: 507-507, 717-717
src/governance/signing.h (3)
1-16: LGTM! Header structure is well-organized.The includes and forward declarations are appropriate. The inclusion of
governance/object.hat line 9 ensuresCGovernanceObjectis a complete type forstd::optional<const CGovernanceObject>usage, addressing the previously identified compilation issue.
29-46: LGTM! GovernanceSignerParent interface is well-designed.The abstract interface provides clean separation between the governance manager and the signer. The virtual methods cover all necessary operations for governance signing and validation:
- State validation and caching
- Superblock and trigger management
- Vote processing
- Object lookup and addition
The interface design follows SOLID principles and enables proper dependency inversion.
48-76: LGTM! GovernanceSigner class is properly structured.The class design is sound:
- Stores references to all necessary managers
- Uses
std::optional<uint256>for state tracking (votedFundingYesTriggerHash)- Constructor signature accepts all dependencies by reference
- Public
UpdatedBlockTipprovides blockchain update notifications- Private methods encapsulate trigger creation and voting logic
The encapsulation and dependency management follow best practices.
src/governance/governance.cpp (19)
26-38: LGTM!The refactoring of timing constants to
constexpr std::chrono::secondsin an anonymous namespace improves type safety and follows modern C++ best practices.
94-121: LGTM!The scheduled relay implementation correctly batches governance object and vote announcements, with appropriately tighter timing for tests (1s) versus production (5s). The brief lock duration and sync checks are appropriate.
134-162: LGTM!The relay methods correctly queue inventory messages with appropriate validation:
- Sync state checks prevent premature relay
- RelayVote validates masternode existence before queuing
- Lock annotations ensure correct lock ordering
206-210: LGTM!Correctly uses
emplace()to avoid constructing a temporary pair, as discussed in previous reviews.
212-214: LGTM!The updated signature correctly removes the
PeerManagerparameter, aligning with the centralized relay architecture. Lock annotations are consistent.
307-307: LGTM!The relay calls are correctly placed after validation and use the new centralized relay methods consistently.
Also applies to: 341-341, 374-374, 429-429
473-563: LGTM!The chrono refactoring improves type safety. The mixed usage of
.count()at line 544 vs. direct comparison at line 554 is intentional and correct given the different value types inmapErasedGovernanceObjects(int64_t) vs.m_requested_hash_time(std::chrono::seconds).
575-587: LGTM!The split into public/internal variants with appropriate lock annotations is a good pattern that allows callers already holding the lock to avoid deadlock.
589-597: LGTM!Lock annotations added for consistency and correctness.
704-714: LGTM!Type-safe chrono duration arithmetic.
830-833: LGTM!Correctly uses
count_seconds()helper to convert chrono durations to int64_t for comparison with timestamp values.
871-875: LGTM!Consistent chrono conversion pattern.
908-916: LGTM!Correctly implements the vote-then-relay pattern with appropriate lock annotations.
945-946: LGTM!Correctly converts chrono duration to int64_t for storage in the orphan votes map.
972-1038: LGTM!Correctly processes postponed objects and additional relay objects with appropriate lock annotations and chrono usage.
1071-1074: LGTM!Simple and correct implementation for tracking invalid votes.
1324-1346: LGTM!Simplified signature correctly aligns with the new architecture. Lock annotations are consistent with the method's calls to CheckPostponedObjects.
1556-1576: LGTM!The public/internal split pattern is consistently applied for lock granularity, allowing callers that already hold the lock to avoid deadlock.
1744-1775: LGTM!The
GetApprovedProposalsimplementation correctly:
- Filters proposals by type and approval threshold
- Sorts by yes votes with deterministic tiebreaker
- Returns immutable shared_ptr to prevent caller mutation
The use of
shared_ptris appropriate for this API surface despite the reference counting overhead.src/governance/governance.h (10)
10-23: LGTM!Headers are complete and correctly organized. Previously flagged missing
<optional>and<chrono>includes have been added.
25-54: LGTM!Forward declarations and type aliases are complete and improve code readability.
244-244: LGTM!Inheritance from
GovernanceSignerParentcorrectly integrates the governance signing framework as described in the PR objectives.
271-272: LGTM!The relay queue is correctly protected with a mutex and proper guard annotations.
280-287: LGTM!Method signatures correctly declare lock requirements to prevent deadlock.
IsValid()override is correct.
299-304: LGTM!Signatures correctly match the implementation with appropriate override specifiers and lock annotations.
311-318: LGTM!Signatures correctly simplified and annotated with appropriate overrides and lock requirements.
322-322: LGTM!Override specifiers and lock annotations are consistent with the parent interface and implementation.
Also applies to: 335-335, 339-339, 343-346
365-365: LGTM!Public API correctly declares overrides with appropriate lock annotations matching the implementation.
Also applies to: 389-393
396-406: LGTM!Internal helper methods are correctly declared with appropriate lock requirements, enabling proper lock granularity patterns.
Also applies to: 413-413
src/node/interfaces.cpp (1)
151-161: PeerManager decoupling looks good; confirm schedule-driven relays are active.
- processVoteAndRelay now uses govman+connman only; OK.
- submitProposal: AddPostponedObject + RelayObject/AddGovernanceObject aligns with queued/scheduled relays.
Please confirm CGovernanceManager::Schedule(...) is invoked during init on valid nodes so queued relays are processed (timers differ by network).
Also applies to: 239-243
src/governance/object.h (1)
14-15: Span-based SetSignature API is a solid improvement.Taking Span avoids copies and is flexible; implementation matches usage elsewhere. No issues.
Also applies to: 216-218
src/dsnotificationinterface.cpp (1)
92-94: UpdatedBlockTip integration matches new CGovernanceManager API.Direct call with pindexNew is tidy and removes peerman dependency. Looks good.
src/governance/object.cpp (1)
247-250: Correct Span-to-vector copy.assign(sig.begin(), sig.end()) is the right pattern. Matches the header’s Span API.
src/masternode/active/context.cpp (1)
18-23: GovernanceSigner wiring is sensible; ensure it’s driven by notifications.gov_signer is constructed with the right deps. Please confirm it’s invoked via the active notification interface or other hooks (e.g., UpdatedBlockTip) so it participates in the flow.
Also applies to: 31-34
src/init.cpp (1)
2143-2144: Init wiring aligns with refactor.
- CDSNotificationInterface: peerman/mn_activeman removed; OK.
- ActiveContext: govman added; OK.
- Governance Schedule(...) called when valid; OK.
Please verify functional tests covering governance relays/voting pass with the schedule-based relayer.
Also applies to: 2157-2159, 2264-2266
|
This pull request has conflicts, please rebase. |
We are discarding the value of `ObjName::Sign()` so we are assuming it is always valid, let's codify that assumption with an assert, something we generate should hold valid to ourselves.
Review with `git log -p -n1 --color-moved=dimmed_zebra`.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/governance/governance.cpp (1)
977-1003: Deadlock risk: re-locking cs in AddGovernanceObject from CheckPostponedObjectsYou hold LOCK2(::cs_main, cs) and then call AddGovernanceObject(), which acquires LOCK2(::cs_main, cs) again. cs is a non-recursive Mutex, so this can deadlock.
Proposed fix: collect ready objects while holding locks, release locks, then call AddGovernanceObject().
- LOCK2(::cs_main, cs); - - // Check postponed proposals - for (auto it = mapPostponedObjects.begin(); it != mapPostponedObjects.end();) { + std::vector<CGovernanceObject> ready; + { + LOCK2(::cs_main, cs); + // Check postponed proposals + for (auto it = mapPostponedObjects.begin(); it != mapPostponedObjects.end();) { @@ - if (govobj.IsValidLocally(Assert(m_dmnman)->GetListAtChainTip(), m_chainman, strError, false)) { - AddGovernanceObject(govobj); - } else { + if (govobj.IsValidLocally(Assert(m_dmnman)->GetListAtChainTip(), m_chainman, strError, false)) { + ready.push_back(govobj); + } else { @@ - // remove processed or invalid object from the queue - mapPostponedObjects.erase(it++); - } + // remove processed or invalid object from the queue + mapPostponedObjects.erase(it++); + } + } // release cs_main/cs before calling AddGovernanceObject + for (auto& obj : ready) { + AddGovernanceObject(obj); + }Also applies to: 1006-1038
♻️ Duplicate comments (1)
src/coinjoin/server.cpp (1)
348-349: LGTM: migrated to SignBasic() with direct signature assignmentConsistent with removal of Sign(...) methods and the project’s assert policy for signature validity.
Also applies to: 507-508, 717-718
🧹 Nitpick comments (10)
test/lint/lint-circular-dependencies.py (1)
52-55: The governance module's circular dependencies are expanding significantly.The refactoring replaces simpler cycles with more complex multi-hop paths:
- Line 52: Adds
governance/classesto thegovernance/object↔governance/governancecycle- Line 53: Introduces a new cycle through
governance/signing- Line 55: Adds a cycle via
masternode/active/contextWhile these may be necessary for the current refactoring, the proliferation of circular dependencies within the governance subsystem suggests opportunities for future architectural improvements. Consider whether dependency inversion or clearer module boundaries could reduce coupling.
Note: As this PR is already addressing technical debt by centralizing relay logic and extracting signing concerns, addressing these architectural patterns could be deferred to future work.
src/masternode/active/notificationinterface.cpp (1)
26-26: Document invariant with assertSince gov_signer is always constructed, add an assert to document the invariant.
m_mn_activeman.UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); m_active_ctx.ehf_sighandler->UpdatedBlockTip(pindexNew); + assert(m_active_ctx.gov_signer); m_active_ctx.gov_signer->UpdatedBlockTip(pindexNew);Based on learnings
src/coinjoin/coinjoin.h (1)
185-186: Consider adding SetSignature() helpers for API consistencyExpose small setters to assign signatures (mirrors governance objects), enabling future invariants without external churn.
class CCoinJoinQueue { ... - std::vector<unsigned char> vchSig; + std::vector<unsigned char> vchSig; + void SetSignature(const std::vector<unsigned char>& sig) { vchSig = sig; } // memory only bool fTried{false};class CCoinJoinBroadcastTx { ... - std::vector<unsigned char> vchSig; + std::vector<unsigned char> vchSig; + void SetSignature(const std::vector<unsigned char>& sig) { vchSig = sig; } int64_t sigTime{0};Also applies to: 238-240
src/governance/signing.h (2)
72-76: Avoid optional for heavy types; prefer optional or pointer/reference wrappersoptional/optional is unusual and can cause unnecessary copies and awkward const semantics. Use optional (const-qualified at the call site) or optional<std::reference_wrapper>/shared_ptr for non-owning.
Apply:
- std::optional<const CGGovernanceObject> CreateGovernanceTrigger(const std::optional<const CSuperblock>& sb_opt); - std::optional<const CSuperblock> CreateSuperblockCandidate(int nHeight) const; + std::optional<CGovernanceObject> CreateGovernanceTrigger(const std::optional<CSuperblock>& sb_opt); + std::optional<CSuperblock> CreateSuperblockCandidate(int nHeight) const; @@ - void VoteGovernanceTriggers(const std::optional<const CGovernanceObject>& trigger_opt); + void VoteGovernanceTriggers(const std::optional<CGovernanceObject>& trigger_opt);
40-46: Unify ownership semantics (raw pointers vs shared_ptr)Returning raw CGovernanceObject* while other APIs return shared_ptr is inconsistent and can confuse ownership. Prefer either non-owning references consistently (const CGovernanceObject*) or shared_ptr for all returning APIs.
src/governance/object.h (1)
215-217: Ensure SetSignature copies from span and does not retain referencesVerify SetSignature copies sig bytes into owned storage (e.g., std::vector<uint8_t>) and does not store a view to avoid dangling references. Consider documenting this or returning bool if validation/size checks are performed.
src/governance/governance.cpp (2)
79-85: Unused member: votedFundingYesTriggerHash on CGovernanceManagerThis state now lives in GovernanceSigner. Keeping it here is confusing and unused in this TU. Remove to avoid drift.
582-587: Avoid double lookup and operator[] in FindGovernanceObjectInternalUse find to avoid two lookups and accidental insertion patterns.
-CGovernanceObject* CGovernanceManager::FindGovernanceObjectInternal(const uint256& nHash) -{ - AssertLockHeld(cs); - if (mapObjects.count(nHash)) return &mapObjects[nHash]; - return nullptr; -} +CGovernanceObject* CGovernanceManager::FindGovernanceObjectInternal(const uint256& nHash) +{ + AssertLockHeld(cs); + auto it = mapObjects.find(nHash); + return it != mapObjects.end() ? &it->second : nullptr; +}src/governance/governance.h (2)
299-301: Annotation style: prefer explicit exclusion for readabilityIf the codebase supports it, consider using LOCKS_EXCLUDED(cs_relay) instead of EXCLUSIVE_LOCKS_REQUIRED(!cs_relay) for functions that acquire cs_relay internally (e.g., ProcessMessage). Improves clarity and aligns with common thread-safety annotation patterns.
Would you like a follow-up sweep to normalize these annotations across governance/?
389-394: Consider [[nodiscard]] on query methodsGetBestSuperblock and GetApprovedProposals results are easy to ignore accidentally. Marking them [[nodiscard]] can prevent bugs.
Example:
- bool GetBestSuperblock(... ) override EXCLUSIVE_LOCKS_REQUIRED(!cs); + [[nodiscard]] bool GetBestSuperblock(... ) override EXCLUSIVE_LOCKS_REQUIRED(!cs); - std::vector<std::shared_ptr<const CGovernanceObject>> GetApprovedProposals(...) override EXCLUSIVE_LOCKS_REQUIRED(!cs); + [[nodiscard]] std::vector<std::shared_ptr<const CGovernanceObject>> GetApprovedProposals(...) override EXCLUSIVE_LOCKS_REQUIRED(!cs);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
src/Makefile.am(2 hunks)src/coinjoin/coinjoin.cpp(0 hunks)src/coinjoin/coinjoin.h(1 hunks)src/coinjoin/server.cpp(3 hunks)src/dsnotificationinterface.cpp(1 hunks)src/dsnotificationinterface.h(0 hunks)src/governance/governance.cpp(39 hunks)src/governance/governance.h(7 hunks)src/governance/object.cpp(2 hunks)src/governance/object.h(2 hunks)src/governance/signing.cpp(1 hunks)src/governance/signing.h(1 hunks)src/governance/vote.cpp(1 hunks)src/governance/vote.h(0 hunks)src/init.cpp(3 hunks)src/masternode/active/context.cpp(2 hunks)src/masternode/active/context.h(3 hunks)src/masternode/active/notificationinterface.cpp(2 hunks)src/masternode/node.cpp(1 hunks)src/masternode/node.h(1 hunks)src/net_processing.cpp(1 hunks)src/node/interfaces.cpp(3 hunks)src/qt/governancelist.h(2 hunks)src/rpc/governance.cpp(3 hunks)src/test/coinjoin_queue_tests.cpp(1 hunks)src/wallet/interfaces.cpp(0 hunks)test/functional/feature_governance.py(1 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
💤 Files with no reviewable changes (4)
- src/coinjoin/coinjoin.cpp
- src/dsnotificationinterface.h
- src/wallet/interfaces.cpp
- src/governance/vote.h
🚧 Files skipped from review as they are similar to previous changes (4)
- src/test/coinjoin_queue_tests.cpp
- test/functional/feature_governance.py
- src/masternode/node.cpp
- src/masternode/node.h
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/masternode/active/notificationinterface.cppsrc/masternode/active/context.hsrc/coinjoin/coinjoin.hsrc/governance/object.hsrc/rpc/governance.cppsrc/governance/object.cppsrc/governance/vote.cppsrc/dsnotificationinterface.cppsrc/masternode/active/context.cppsrc/governance/signing.cppsrc/node/interfaces.cppsrc/init.cppsrc/governance/signing.hsrc/qt/governancelist.hsrc/net_processing.cppsrc/governance/governance.cppsrc/governance/governance.hsrc/coinjoin/server.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 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/masternode/active/context.cpp
🧠 Learnings (11)
📚 Learning: 2025-10-03T11:29:36.358Z
Learnt from: kwvg
PR: dashpay/dash#6838
File: src/governance/signing.cpp:84-85
Timestamp: 2025-10-03T11:29:36.358Z
Learning: After bitcoin#25153 (included in dash#6775), UniValue uses the templated getInt<T>() API instead of the legacy get_int() and get_int64() methods. For example, use jproposal["start_epoch"].getInt<int64_t>() to retrieve int64_t values from UniValue objects.
Applied to files:
src/governance/signing.cpp
📚 Learning: 2025-10-03T11:30:10.766Z
Learnt from: kwvg
PR: dashpay/dash#6838
File: src/governance/signing.cpp:232-242
Timestamp: 2025-10-03T11:30:10.766Z
Learning: The Dash codebase dropped C++17 support in PR #6380 and now requires C++20 or later, as configured in configure.ac. C++20 features, including std::ranges algorithms like std::ranges::any_of, are appropriate and preferred where they improve code clarity.
Applied to files:
src/governance/signing.cpp
📚 Learning: 2025-08-13T16:10:32.972Z
Learnt from: kwvg
PR: dashpay/dash#6815
File: src/chainlock/chainlock.h:68-69
Timestamp: 2025-08-13T16:10:32.972Z
Learning: The Dash codebase extensively uses std::chrono_literals throughout, including in header files. The pattern using namespace std::chrono_literals; is used globally in src/util/time.h, and chrono literals like 0s are commonly used for atomic member initialization in headers (e.g., src/net.h). Chrono literals like 0s, 1min, etc. are preferred over explicit constructors.
Applied to files:
src/governance/signing.cpp
📚 Learning: 2025-08-13T16:10:32.972Z
Learnt from: kwvg
PR: dashpay/dash#6815
File: src/chainlock/chainlock.h:68-69
Timestamp: 2025-08-13T16:10:32.972Z
Learning: The Dash codebase extensively uses std::chrono_literals throughout, including in header files. Chrono literals like 0s, 1min, etc. are acceptable and preferred over explicit constructors like std::chrono::seconds::zero().
Applied to files:
src/governance/signing.cpp
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
PR: dashpay/dash#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/governance/signing.cpp
📚 Learning: 2025-07-29T14:33:01.040Z
Learnt from: kwvg
PR: dashpay/dash#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/governance/signing.cpp
📚 Learning: 2025-10-03T11:27:54.178Z
Learnt from: kwvg
PR: dashpay/dash#6838
File: src/governance/signing.cpp:89-95
Timestamp: 2025-10-03T11:27:54.178Z
Learning: In the Dash codebase, `LogPrint` uses tinyformat internally, which is a type-safe formatting library that handles format specifiers and type conversions automatically. Unlike traditional printf-style functions, tinyformat does not require matching format specifiers (like %lld for int64_t) with argument types, making manual casting unnecessary.
Applied to files:
src/governance/signing.cpp
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/**/*.{cpp,h,cc,cxx,hpp} : Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Applied to files:
src/governance/signing.cpp
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
PR: dashpay/dash#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/net_processing.cpp
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
PR: dashpay/dash#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/net_processing.cpp
📚 Learning: 2025-07-17T15:48:29.418Z
Learnt from: kwvg
PR: dashpay/dash#6752
File: src/wallet/load.cpp:164-164
Timestamp: 2025-07-17T15:48:29.418Z
Learning: In Dash Core, kwvg prefers using assert() statements to document expected behavior in code paths, especially when the called function also has internal asserts. This is used as a defensive programming pattern rather than requiring conditional error handling.
Applied to files:
src/coinjoin/server.cpp
🔇 Additional comments (24)
test/lint/lint-circular-dependencies.py (1)
43-44: Verify that the newcore_iocircular dependencies throughgovernance/signingare unavoidable.The introduction of two new circular dependency paths from
core_iothrough the newgovernance/signingmodule back tocore_iosuggests architectural coupling that may warrant scrutiny. While the PR's refactoring goals are clear, consider whether these cycles could be broken by:
- Using dependency injection or interfaces to decouple modules
- Extracting shared types/data structures into a separate module
- Restructuring the call graph to avoid bidirectional dependencies
Run the following script to examine the actual dependencies and understand why these cycles exist:
src/qt/governancelist.h (2)
9-9: LGTM on include adjustmentsIncluding pubkey.h is appropriate for CKeyID usage; moving governance/object.h down reduces coupling.
Also applies to: 14-15
37-39: Confirm enum forward declaration matches definitionForward-declaring an unscoped enum requires specifying the exact underlying type. Please confirm governance/vote.h defines vote_outcome_enum_t with “: int”.
src/Makefile.am (1)
227-231: LGTM: wiring GovernanceSigner into buildgovernance/signing.{h,cpp} correctly added to exports and sources; reordering of governance/governance.h is benign.
Also applies to: 514-515
src/masternode/active/context.h (1)
51-54: LGTM: ActiveContext now carries GovernanceSignerConstructor wiring and member addition look correct; forward declarations keep the header light.
Also applies to: 62-62
src/governance/vote.cpp (1)
13-15: LGTM on include updateschainparams/logging are appropriate here given network checks and LogPrint usage; dropped unused headers.
src/governance/signing.h (1)
59-68: Validate thread-safety of internal state accessed by UpdatedBlockTipvotedFundingYesTriggerHash is mutated/read without synchronization. If UpdatedBlockTip can be called from validation interface threads (and any other path), this risks a data race. Confirm single-threaded access or guard with a mutex/atomic.
src/node/interfaces.cpp (2)
151-154: LGTM: dropped peerman dependency in vote relayprocessVoteAndRelay now uses govman + connman only; null checks and error propagation look correct.
239-243: Confirm RelayObject semantics with 0-conf collateralOn fMissingConfirmations, calling RelayObject(govobj) could relay 0-conf proposals. Ensure RelayObject only enqueues for periodic relay and respects min relay confirmations/rate limits to avoid premature propagation.
src/rpc/governance.cpp (3)
398-401: Confirm governance relay queue behavior for postponed objectsFor fMissingConfirmations, you AddPostponedObject then RelayObject. Please verify RelayObject respects min relay confirmations, rate checks, and only queues for scheduled relay to avoid flooding peers with 0-conf objects.
458-459: LGTM: updated ProcessVoteAndRelay signaturePassing only connman (no peerman) aligns with centralized relay.
965-965: LGTM: voteraw uses new ProcessVoteAndRelay overloadSignature update looks correct.
src/governance/object.h (1)
14-15: LGTM: include span for SetSignatureIncluding <span.h> is appropriate for the new API.
src/net_processing.cpp (1)
5383-5383: Governance handler signature update looks correctCall site matches the new ProcessMessage(pfrom, m_connman, msg_type, vRecv) API and integrates with PostProcessMessage as before. No ordering concerns spotted.
src/dsnotificationinterface.cpp (1)
92-94: UpdatedBlockTip governance call aligned with new APISwitch to m_govman.UpdatedBlockTip(pindexNew) under IsValid() is consistent with removing peerman/mn_activeman deps. Early IBD return remains intact.
src/governance/object.cpp (1)
247-250: SetSignature via span is clean and flexibleAccepting Span and assigning into vchSig is correct and avoids unnecessary copies from container conversions.
src/init.cpp (3)
2143-2144: CDSNotificationInterface wiring matches new signatureArgument order and types align with the updated constructor, keeping dependencies minimal at init.
2157-2159: ActiveContext now includes govman and gov_signerConstructor update and dependency ordering look correct.
2265-2266: Governance scheduling integratedUsing Schedule(*scheduler, *connman, *peerman) fits the new periodic relay design; shutdown order (scheduler->stop before connman/peerman teardown) keeps captures safe.
src/masternode/active/context.cpp (1)
18-33: GovernanceSigner integration looks correctConstructor signature change and gov_signer initialization are consistent; no lifetime issues with captured references.
src/governance/signing.cpp (2)
121-167: Trigger creation and validation flow is sound
- Avoids duplicates via DataHash check.
- Restricts creation to current payee.
- Signs via SignBasic and validates locally with rate check before submission.
Looks good.
169-258: Voting logic avoids duplicates and handles NO votes for othersCorrectly tracks YES state and skips outdated/duplicate votes. Use of ranges::any_of is fine with C++20.
src/governance/governance.h (2)
10-23: Includes look correct for new typesGovernance signing header and std headers cover std::chrono::seconds and std::optional usage. LGTM.
47-54: Prune unused forwards/aliases (if unused here)If GovernanceSigner and CDeterministicMNListPtr aren’t referenced in this header, drop these declarations to keep the header minimal.
kwvg
requested review from
PastaPastaPasta,
UdjinM6 and
knst
and removed request for
PastaPastaPasta,
UdjinM6 and
knst
4 participants
Additional Information
Depends on refactor: drop
fMasternodeModeglobal, create new context for masternode mode-only logic (ActiveContext), move spun-off signers and EHF signals handler #6828Depends on fix: use
std::atomicto protect connected manager/signer pointers, check beforequeuemanaccess, update P2P message map in tests #6842Depends on chore!: merge bitcoin#28753, partial bitcoin#28195 (bump
MIN_PEER_PROTO_VERSIONto70221) #6877Dependency for refactor: update lock annotations for
GovernanceStore,CGovernanceManagerandCGovernanceStore, move away fromRecursiveMutexes #6849To reduce the proliferation of
PeerManagerin governance logic, relaying has been centralized toCGovernanceManagerand now done through a scheduled task that processes a queue of requests. This reduces the amount of code that needs to fetch a reference toPeerManagerin order to queue a message for relay and brings us closer to dropping it as a member in other objects.Likewise, to reduce the references to
CActiveMasternodeManager, signing logic is consolidated toSignBasic()with the objects using simple getter/setters (or direct assignment) where applicable.Breaking Changes
None expected.
Checklist