refactor: split off governance masternode-only logic to GovernanceSigner, drop Relay()s and use periodic relay instead, minor cleanup
#6838
Conversation
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds a GovernanceSigner subsystem (governance/signing.{h,cpp}) and exports its header. Introduces CActiveMasternodeManager::SignBasic and replaces in-place Sign/Relay flows: CGovernanceObject/CGovernanceVote no longer sign/relay themselves but expose SetSignature/vchSig and rely on external signing and new relay APIs. CGovernanceManager gains Schedule, RelayObject, RelayVote, AddPostponedObject, AddInvalidVote, GetApprovedProposals, a relay queue and chrono-based timing; several methods drop PeerManager/CActiveMasternodeManager parameters and change locking semantics. ActiveContext and notification paths are wired to the new signer; coinjoin, RPC, tests, and build exports updated; GOVSCRIPT_PROTO_VERSION removed. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
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`.
Support for P2SH proposals was introduced in Dash Core 0.18, we don't have to worry about antagonizing those peers as they're multiple hard forks behind us.
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
| m_mn_sync{mn_sync}, | ||
| nTimeLastDiff(0), | ||
| nCachedBlockHeight(0), | ||
| mapPostponedObjects(), |
There was a problem hiding this comment.
@coderabbitai is this line needed? shouldn't a map like this be automatically initialized properly?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the declaration of mapPostponedObjects in the governance header/implementation files
rg -n "mapPostponedObjects" --type=cpp --type=h -C2 src/governance/
Length of output: 3326
There was a problem hiding this comment.
This change was made while trying to trim the header count in governance/governance.h by moving the constructor's definition into the source file so we don't have to include heavier headers and can make do with a forward declaration.
It could probably be reversed but no strong feelings either way.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| auto dmn = tip_mn_list.GetMNByCollateral(vote.GetMasternodeOutpoint()); | ||
| if (!dmn) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
should we do a misbehaving here? Currently someone could spam these messages with invalid master node right?
There was a problem hiding this comment.
The check is technically redundant as all the non-interface callers for RelayMessage() first call ProcessVote()
dash/src/governance/governance.cpp
Lines 337 to 340 in 6898eff
dash/src/governance/governance.cpp
Lines 372 to 373 in 6898eff
dash/src/governance/governance.cpp
Lines 909 to 911 in 6898eff
and ProcessVote() makes that check
dash/src/governance/object.cpp
Lines 74 to 80 in 6898eff
but it has been retained as a nice-to-have
|
|
||
| void CGovernanceManager::AddInvalidVote(const CGovernanceVote& vote) | ||
| { | ||
| cmapInvalidVotes.Insert(vote.GetHash(), vote); |
There was a problem hiding this comment.
note: it appears we never use this as a map; only as a set. We should consider refactoring this.
| } | ||
|
|
||
| bool CGovernanceObject::Sign(const CActiveMasternodeManager& mn_activeman) | ||
| void CGovernanceObject::SetSignature(const std::vector<uint8_t>& sig) |
There was a problem hiding this comment.
nit: take in span, or a Bytes obj
There was a problem hiding this comment.
Doesn't work with assignment operator
no operator "=" matches these operands C/C++(349)
object.cpp(249, 18): operand types are: std::vector<unsigned char, std::allocator<unsigned char>> = Span<const unsigned char>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/governance/governance.cpp (1)
27-27: Run clang-format to fix formatting issues.The pipeline failure indicates clang-format differences persist in this file. Please run clang-format to ensure consistent code formatting.
Also applies to: 36-40
🧹 Nitpick comments (1)
src/governance/signing.cpp (1)
138-138: Redundant lock: cs_main already held.
cs_mainis already held byLOCK2at line 128. TheWITH_LOCKwrapper is redundant (though safe due to the recursive mutex). For clarity and minor performance improvement, directly accessm_chainman.ActiveChain().Tip().Apply this diff:
- const CBlockIndex* tip = WITH_LOCK(::cs_main, return m_chainman.ActiveChain().Tip()); + const CBlockIndex* tip = m_chainman.ActiveChain().Tip(); // cs_main already held by LOCK2 above
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
-
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.h(1 hunks) -
src/governance/signing.cpp(1 hunks) -
src/governance/signing.h(1 hunks) -
src/init.cpp(3 hunks) -
src/masternode/active/context.cpp(2 hunks) -
src/net_processing.cpp(1 hunks) -
src/node/interfaces.cpp(3 hunks) -
src/rpc/governance.cpp(3 hunks) -
test/lint/lint-circular-dependencies.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/dsnotificationinterface.h
🚧 Files skipped from review as they are similar to previous changes (1)
- test/lint/lint-circular-dependencies.py
🧰 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/node/interfaces.cppsrc/masternode/active/context.cppsrc/net_processing.cppsrc/governance/signing.hsrc/rpc/governance.cppsrc/governance/governance.cppsrc/governance/object.hsrc/governance/signing.cppsrc/init.cppsrc/dsnotificationinterface.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/active/context.cpp
🧠 Learnings (10)
📚 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-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
🧬 Code graph analysis (7)
src/governance/signing.h (3)
src/governance/governance.cpp (20)
GetBestSuperblock(1631-1637)GetBestSuperblock(1631-1632)MasternodeRateCheck(839-843)MasternodeRateCheck(839-839)MasternodeRateCheck(845-907)MasternodeRateCheck(845-845)ProcessVoteAndRelay(909-917)ProcessVoteAndRelay(909-909)FindGovernanceObject(576-581)FindGovernanceObject(576-576)FindGovernanceObjectByDataHash(590-598)FindGovernanceObjectByDataHash(590-590)GetActiveTriggers(1556-1561)GetActiveTriggers(1556-1556)GetApprovedProposals(1745-1776)GetApprovedProposals(1745-1746)AddGovernanceObject(384-443)AddGovernanceObject(384-384)UpdatedBlockTip(1325-1347)UpdatedBlockTip(1325-1325)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)src/masternode/active/notificationinterface.cpp (2)
UpdatedBlockTip(18-27)UpdatedBlockTip(18-19)
src/rpc/governance.cpp (2)
src/interfaces/node.h (2)
node(41-43)node(385-385)src/node/interfaces.cpp (2)
vote(149-161)vote(149-149)
src/governance/governance.cpp (2)
src/net_processing.cpp (9)
scheduler(625-625)scheduler(676-676)inv(633-633)inv(634-634)inv(653-653)inv(659-659)inv(945-945)inv(2326-2326)ret(667-667)src/util/time.h (1)
count_seconds(56-56)
src/governance/object.h (2)
src/governance/vote.h (1)
SetSignature(97-97)src/governance/object.cpp (2)
SetSignature(247-250)SetSignature(247-247)
src/governance/signing.cpp (5)
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/dsnotificationinterface.cpp (2)
UpdatedBlockTip(68-95)UpdatedBlockTip(68-68)src/masternode/active/notificationinterface.cpp (2)
UpdatedBlockTip(18-27)UpdatedBlockTip(18-19)
src/dsnotificationinterface.cpp (1)
src/net_processing.cpp (1)
pindexNew(610-610)
src/governance/governance.h (1)
src/governance/governance.cpp (60)
CGovernanceManager(71-87)CGovernanceManager(89-93)Schedule(95-122)Schedule(95-95)LoadCache(124-133)LoadCache(124-124)RelayObject(135-145)RelayObject(135-135)RelayVote(147-163)RelayVote(147-147)ProcessMessage(213-355)ProcessMessage(213-214)FindConstGovernanceObject(566-574)FindConstGovernanceObject(566-566)FindGovernanceObject(576-581)FindGovernanceObject(576-576)AddGovernanceObject(384-443)AddGovernanceObject(384-384)CheckAndRemove(445-564)CheckAndRemove(445-445)ToJson(1293-1323)ToJson(1293-1293)UpdatedBlockTip(1325-1347)UpdatedBlockTip(1325-1325)AddPostponedObject(207-211)AddPostponedObject(207-207)MasternodeRateUpdate(817-837)MasternodeRateUpdate(817-817)MasternodeRateCheck(839-843)MasternodeRateCheck(839-839)MasternodeRateCheck(845-907)MasternodeRateCheck(845-845)ProcessVoteAndRelay(909-917)ProcessVoteAndRelay(909-909)CheckPostponedObjects(973-1039)CheckPostponedObjects(973-973)GetActiveTriggers(1556-1561)GetActiveTriggers(1556-1556)GetBestSuperblock(1631-1637)GetBestSuperblock(1631-1632)GetApprovedProposals(1745-1776)GetApprovedProposals(1745-1746)InternalFindGovernanceObject(583-588)InternalFindGovernanceObject(583-583)InternalGetActiveTriggers(1563-1577)InternalGetActiveTriggers(1563-1563)InternalGetBestSuperblock(1639-1670)InternalGetBestSuperblock(1639-1640)ExecuteBestSuperblock(1733-1743)ExecuteBestSuperblock(1733-1733)RequestGovernanceObject(1041-1070)RequestGovernanceObject(1041-1041)AddInvalidVote(1072-1075)AddInvalidVote(1072-1072)ProcessVote(919-971)ProcessVote(919-919)AcceptMessage(1194-1205)AcceptMessage(1194-1194)CheckOrphanVotes(357-382)CheckOrphanVotes(357-357)
🪛 GitHub Actions: Clang Diff Format Check
src/governance/governance.cpp
[error] 1-1: Clang-format differences detected in governance.cpp. Command failed: git diff -U0 origin/develop --
src/governance/signing.cpp
[error] 1-1: Clang-format differences detected in signing.cpp. Command failed: git diff -U0 origin/develop --
⏰ 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). (4)
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
🔇 Additional comments (16)
src/governance/signing.cpp (1)
25-297: LGTM: Solid governance signing implementation.The GovernanceSigner class properly extracts masternode-only governance logic, consolidating signing workflows and eliminating direct PeerManager usage. The implementation:
- Correctly uses
getInt<int64_t>()for UniValue (post-bitcoin#25153)- Appropriately employs C++20
std::ranges::any_of- Includes proper null checks (e.g., line 219)
- Follows safe locking patterns with recursive mutexes
- Integrates cleanly with the relay queue architecture
The code is well-structured and aligns with the PR objectives to reduce PeerManager proliferation.
Based on learnings.
src/governance/governance.h (1)
1-428: LGTM: Well-structured governance manager interface.The header changes cleanly support the governance signing refactoring:
- Proper includes for
<chrono>,<optional>, and reordered standard headers- Forward declarations minimize header dependencies
GovernanceSignerParentinheritance enables centralized signing- New relay queue architecture with
cs_relaymutex and explicitRelayObject/RelayVotemethods- Comprehensive locking annotations (
EXCLUSIVE_LOCKS_REQUIRED)- Internal helper methods (
InternalFind*,InternalGet*) enable lock-aware implementationsSchedulemethod supports periodic governance maintenance and relay processingThe interface is consistent, well-annotated, and aligns with the PR's goal to reduce PeerManager proliferation.
src/governance/governance.cpp (14)
95-122: LGTM! Well-structured scheduling implementation.The periodic task setup is well-designed:
- Proper separation of concerns (maintenance vs relay)
- Appropriate timing adjustments for test vs production environments
- Correct locking and synchronization for the relay queue
- Clean lambda captures for the required dependencies
This aligns well with the PR's goal of centralizing relay logic.
135-163: LGTM! Proper relay queueing with good validation.Both relay methods are well-implemented:
- Correct lock ordering (AssertLockNotHeld at entry, LOCK when accessing shared state)
- Appropriate sync checks to prevent premature relays
- RelayVote includes masternode validation as defense-in-depth (lines 155-159)
The centralized relay queue approach aligns with the PR's refactoring goals.
207-211: LGTM! Efficient implementation with emplace.The use of
emplaceavoids creating a temporary pair object, as suggested in previous reviews.
213-215: LGTM! Signature simplification aligns with PR goals.Removing the
PeerManager¶meter reduces coupling and aligns with the PR's objective to minimize PeerManager proliferation. Relay operations are now handled through the centralized relay queue.
384-386: LGTM! Consistent refactoring across related methods.The signature changes for
AddGovernanceObjectandCheckOrphanVotesconsistently remove PeerManager dependencies and use the new relay queue mechanism (lines 430, 375). All call sites are properly updated.Also applies to: 430-430, 439-439
474-555: LGTM! Idiomatic std::chrono usage throughout.The time handling has been properly refactored to use
std::chronotypes:
- Keeps time values as
std::chrono::seconds(line 474)- Performs duration arithmetic with chrono types (lines 493, 523-525)
- Direct duration comparisons without manual conversions (line 499)
- Only converts to
count()when interfacing with legacy int64_t APIs (lines 535, 545)This follows best practices for type-safe time handling, as requested in previous reviews.
578-598: LGTM! Good internal/external method separation pattern.The introduction of
InternalFindGovernanceObjectand similar methods provides:
- Clear separation between locking and pre-locked code paths
- Proper lock assertions to catch errors (lines 578, 585, 592)
- Ability for internal methods to call these without double-locking
- Reduced lock contention
This is a good pattern for managing lock hierarchies.
705-705: LGTM! Consistent chrono constant usage.The time handling correctly uses:
- Chrono types where possible (line 705)
- Explicit
count_seconds()helper for interfacing with legacy int64_t timestamps (lines 831, 872)The conversions are type-safe and follow the codebase's time handling conventions.
Also applies to: 831-831, 872-872
909-917: LGTM! Simplified interface with queue-based relay.The signature change removes the
CConnman¶meter as relay operations are now handled through the centralized queue. TheAssertLockNotHeld(cs_relay)ensures proper lock ordering.
946-947: LGTM! Proper chrono usage and consistent refactoring.The changes show:
- Correct chrono arithmetic for orphan vote expiration (lines 946-947)
- Proper lock assertions in
CheckPostponedObjects(line 975)- Consistent use of the new relay mechanism (line 1026)
- Appropriate signature updates for method calls (line 991)
- Type-safe time comparisons (lines 1018-1021)
Also applies to: 973-1039
1072-1075: LGTM! Clean API addition.The
AddInvalidVotemethod provides a simple, well-encapsulated way to record invalid votes in the cache.
1325-1327: LGTM! Simplified signature with proper lock assertions.The signature change removes unnecessary parameters, and the
AssertLockNotHeld(cs_relay)ensures correct lock ordering. The call toCheckPostponedObjects(line 1344) is appropriately placed.Also applies to: 1344-1344
1557-1577: LGTM! Consistent internal/external pattern for trigger methods.The refactoring of
GetActiveTriggersandGetBestSuperblockfollows the same good pattern:
- Public methods acquire lock and call internal versions
- Proper lock assertions throughout (lines 1558, 1565, 1634-1635, 1646)
- Internal methods can call other internal methods safely (lines 1647, 1655)
- Call sites correctly use internal versions when lock is already held (lines 1680, 1726, 1738)
This maintains consistent lock management across the codebase.
Also applies to: 1633-1670
1745-1776: LGTM! Well-implemented proposal filtering and sorting.The
GetApprovedProposalsmethod correctly:
- Calculates the passing threshold using weighted MN count (lines 1755-1756)
- Filters proposals by type and vote count (lines 1759-1766)
- Sorts by absolute Yes votes descending with hash tiebreaker for determinism (lines 1769-1773)
- Uses
shared_ptr<const>for efficient, immutable access (line 1751)The sorting logic matches the existing
sortProposalsByVotespattern (lines 668-673), ensuring consistency.
| // Copyright (c) 2014-2025 The Dash Core developers | ||
| // Distributed under the MIT software license, see the accompanying | ||
| // file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
|
|
||
| #include <governance/signing.h> | ||
|
|
||
| #include <evo/deterministicmns.h> | ||
| #include <governance/classes.h> | ||
| #include <masternode/node.h> | ||
| #include <masternode/sync.h> | ||
|
|
||
| #include <chainparams.h> | ||
| #include <logging.h> | ||
| #include <timedata.h> | ||
| #include <util/check.h> | ||
| #include <util/time.h> | ||
| #include <validation.h> | ||
|
|
||
| #include <algorithm> | ||
|
|
||
| namespace { | ||
| constexpr std::chrono::seconds GOVERNANCE_FUDGE_WINDOW{2h}; | ||
| } // anonymous namespace | ||
|
|
||
| GovernanceSigner::GovernanceSigner(CConnman& connman, CDeterministicMNManager& dmnman, GovernanceSignerParent& govman, | ||
| const CActiveMasternodeManager& mn_activeman, const ChainstateManager& chainman, | ||
| const CMasternodeSync& mn_sync) : | ||
| m_connman{connman}, | ||
| m_dmnman{dmnman}, | ||
| m_govman{govman}, | ||
| m_mn_activeman{mn_activeman}, | ||
| m_chainman{chainman}, | ||
| m_mn_sync{mn_sync} | ||
| { | ||
| } | ||
|
|
||
| GovernanceSigner::~GovernanceSigner() = default; | ||
|
|
||
| std::optional<const CSuperblock> GovernanceSigner::CreateSuperblockCandidate(int nHeight) const | ||
| { | ||
| if (!m_govman.IsValid()) return std::nullopt; | ||
| if (!m_mn_sync.IsSynced()) return std::nullopt; | ||
| if (nHeight % Params().GetConsensus().nSuperblockCycle < | ||
| Params().GetConsensus().nSuperblockCycle - Params().GetConsensus().nSuperblockMaturityWindow) | ||
| return std::nullopt; | ||
| if (HasAlreadyVotedFundingTrigger()) return std::nullopt; | ||
|
|
||
| const auto approvedProposals = m_govman.GetApprovedProposals(m_dmnman.GetListAtChainTip()); | ||
| if (approvedProposals.empty()) { | ||
| LogPrint(BCLog::GOBJECT, "%s -- nHeight:%d empty approvedProposals\n", __func__, nHeight); | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| std::vector<CGovernancePayment> payments; | ||
| int nLastSuperblock; | ||
| int nNextSuperblock; | ||
|
|
||
| CSuperblock::GetNearestSuperblocksHeights(nHeight, nLastSuperblock, nNextSuperblock); | ||
| auto SBEpochTime = static_cast<int64_t>(GetTime<std::chrono::seconds>().count() + | ||
| (nNextSuperblock - nHeight) * 2.62 * 60); | ||
| auto governanceBudget = CSuperblock::GetPaymentsLimit(m_chainman.ActiveChain(), nNextSuperblock); | ||
|
|
||
| CAmount budgetAllocated{}; | ||
| for (const auto& proposal : approvedProposals) { | ||
| // Extract payment address and amount from proposal | ||
| UniValue jproposal = proposal->GetJSONObject(); | ||
|
|
||
| CTxDestination dest = DecodeDestination(jproposal["payment_address"].getValStr()); | ||
| if (!IsValidDestination(dest)) continue; | ||
|
|
||
| CAmount nAmount{}; | ||
| try { | ||
| nAmount = ParsePaymentAmount(jproposal["payment_amount"].getValStr()); | ||
| } catch (const std::runtime_error& e) { | ||
| LogPrint(BCLog::GOBJECT, "%s -- nHeight:%d Skipping payment exception:%s\n", __func__, nHeight, e.what()); | ||
| continue; | ||
| } | ||
|
|
||
| // Construct CGovernancePayment object and make sure it is valid | ||
| CGovernancePayment payment(dest, nAmount, proposal->GetHash()); | ||
| if (!payment.IsValid()) continue; | ||
|
|
||
| // Skip proposals that are too expensive | ||
| if (budgetAllocated + payment.nAmount > governanceBudget) continue; | ||
|
|
||
| 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); | ||
|
|
||
| // Skip proposals if the SB isn't within the proposal time window | ||
| if (SBEpochTime < windowStart) { | ||
| LogPrint(BCLog::GOBJECT, "%s -- nHeight:%d SB:%d windowStart:%d\n", __func__, nHeight, SBEpochTime, | ||
| windowStart); | ||
| continue; | ||
| } | ||
| if (SBEpochTime > windowEnd) { | ||
| LogPrint(BCLog::GOBJECT, "%s -- nHeight:%d SB:%d windowEnd:%d\n", __func__, nHeight, SBEpochTime, windowEnd); | ||
| continue; | ||
| } | ||
|
|
||
| // Keep track of total budget allocation | ||
| budgetAllocated += payment.nAmount; | ||
|
|
||
| // Add the payment | ||
| payments.push_back(payment); | ||
| } | ||
|
|
||
| // No proposals made the cut | ||
| if (payments.empty()) { | ||
| LogPrint(BCLog::GOBJECT, "%s -- CreateSuperblockCandidate nHeight:%d empty payments\n", __func__, nHeight); | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| // Sort by proposal hash descending | ||
| std::sort(payments.begin(), payments.end(), [](const CGovernancePayment& a, const CGovernancePayment& b) { | ||
| return UintToArith256(a.proposalHash) > UintToArith256(b.proposalHash); | ||
| }); | ||
|
|
||
| // Create Superblock | ||
| return CSuperblock(nNextSuperblock, std::move(payments)); | ||
| } | ||
|
|
||
| std::optional<const CGovernanceObject> GovernanceSigner::CreateGovernanceTrigger(const std::optional<const CSuperblock>& sb_opt) | ||
| { | ||
| // no sb_opt, no trigger | ||
| if (!sb_opt.has_value()) return std::nullopt; | ||
|
|
||
| // TODO: Check if nHashParentIn, nRevision and nCollateralHashIn are correct | ||
| LOCK(::cs_main); | ||
|
|
||
| // Check if identical trigger (equal DataHash()) is already created (signed by other masternode) | ||
| CGovernanceObject gov_sb(uint256(), 1, GetAdjustedTime(), uint256(), sb_opt.value().GetHexStrData()); | ||
| if (auto identical_sb = m_govman.FindGovernanceObjectByDataHash(gov_sb.GetDataHash())) { | ||
| // Somebody submitted a trigger with the same data, support it instead of submitting a duplicate | ||
| return std::make_optional<CGovernanceObject>(*identical_sb); | ||
| } | ||
|
|
||
| // Nobody submitted a trigger we'd like to see, so let's do it but only if we are the payee | ||
| const CBlockIndex* tip = WITH_LOCK(::cs_main, return m_chainman.ActiveChain().Tip()); | ||
| const auto mnList = m_dmnman.GetListForBlock(tip); | ||
| const auto mn_payees = mnList.GetProjectedMNPayees(tip); | ||
|
|
||
| if (mn_payees.empty()) { | ||
| LogPrint(BCLog::GOBJECT, "%s -- payee list is empty\n", __func__); | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| if (mn_payees.front()->proTxHash != m_mn_activeman.GetProTxHash()) { | ||
| LogPrint(BCLog::GOBJECT, "%s -- we are not the payee, skipping\n", __func__); | ||
| return std::nullopt; | ||
| } | ||
| gov_sb.SetMasternodeOutpoint(m_mn_activeman.GetOutPoint()); | ||
| gov_sb.SetSignature(m_mn_activeman.SignBasic(gov_sb.GetSignatureHash())); | ||
|
|
||
| if (std::string strError; !gov_sb.IsValidLocally(m_dmnman.GetListAtChainTip(), m_chainman, strError, true)) { | ||
| LogPrint(BCLog::GOBJECT, "%s -- Created trigger is invalid:%s\n", __func__, strError); | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| if (!m_govman.MasternodeRateCheck(gov_sb)) { | ||
| LogPrint(BCLog::GOBJECT, "%s -- Trigger rejected because of rate check failure hash(%s)\n", __func__, | ||
| gov_sb.GetHash().ToString()); | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| // The trigger we just created looks good, submit it | ||
| m_govman.AddGovernanceObject(gov_sb); | ||
| return std::make_optional<CGovernanceObject>(gov_sb); | ||
| } | ||
|
|
||
| void GovernanceSigner::VoteGovernanceTriggers(const std::optional<const CGovernanceObject>& trigger_opt) | ||
| { | ||
| // only active masternodes can vote on triggers | ||
| if (m_mn_activeman.GetProTxHash().IsNull()) return; | ||
|
|
||
| LOCK(::cs_main); | ||
|
|
||
| if (trigger_opt.has_value()) { | ||
| // We should never vote "yes" on another trigger or the same trigger twice | ||
| assert(!votedFundingYesTriggerHash.has_value()); | ||
| // Vote YES-FUNDING for the trigger we like, unless we already did | ||
| const uint256 gov_sb_hash = trigger_opt.value().GetHash(); | ||
| bool voted_already{false}; | ||
| if (vote_rec_t voteRecord; trigger_opt.value().GetCurrentMNVotes(m_mn_activeman.GetOutPoint(), voteRecord)) { | ||
| const auto& strFunc = __func__; | ||
| // Let's see if there is a VOTE_SIGNAL_FUNDING vote from us already | ||
| voted_already = ranges::any_of(voteRecord.mapInstances, [&](const auto& voteInstancePair) { | ||
| if (voteInstancePair.first == VOTE_SIGNAL_FUNDING) { | ||
| if (voteInstancePair.second.eOutcome == VOTE_OUTCOME_YES) { | ||
| votedFundingYesTriggerHash = gov_sb_hash; | ||
| } | ||
| LogPrint(BCLog::GOBJECT, /* Continued */ | ||
| "%s -- Not voting YES-FUNDING for trigger:%s, we voted %s for it already\n", strFunc, | ||
| gov_sb_hash.ToString(), | ||
| CGovernanceVoting::ConvertOutcomeToString(voteInstancePair.second.eOutcome)); | ||
| return true; | ||
| } | ||
| return false; | ||
| }); | ||
| } | ||
| if (!voted_already) { | ||
| // No previous VOTE_SIGNAL_FUNDING was found, vote now | ||
| if (VoteFundingTrigger(gov_sb_hash, VOTE_OUTCOME_YES)) { | ||
| LogPrint(BCLog::GOBJECT, "%s -- Voting YES-FUNDING for new trigger:%s success\n", __func__, | ||
| gov_sb_hash.ToString()); | ||
| votedFundingYesTriggerHash = gov_sb_hash; | ||
| } else { | ||
| LogPrint(BCLog::GOBJECT, "%s -- Voting YES-FUNDING for new trigger:%s failed\n", __func__, | ||
| gov_sb_hash.ToString()); | ||
| // this should never happen, bail out | ||
| return; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Vote NO-FUNDING for the rest of the active triggers | ||
| const auto activeTriggers = m_govman.GetActiveTriggers(); | ||
| for (const auto& trigger : activeTriggers) { | ||
| const auto govobj = m_govman.FindGovernanceObject(trigger->GetGovernanceObjHash()); | ||
| if (!govobj) { | ||
| LogPrint(BCLog::GOBJECT, "%s -- Not voting NO-FUNDING for unknown trigger %s\n", __func__, | ||
| trigger->GetGovernanceObjHash().ToString()); | ||
| continue; | ||
| } | ||
|
|
||
| const uint256 trigger_hash = govobj->GetHash(); | ||
| if (trigger->GetBlockHeight() <= m_govman.GetCachedBlockHeight()) { | ||
| // ignore triggers from the past | ||
| LogPrint(BCLog::GOBJECT, "%s -- Not voting NO-FUNDING for outdated trigger:%s\n", __func__, | ||
| trigger_hash.ToString()); | ||
| continue; | ||
| } | ||
| if (trigger_hash == votedFundingYesTriggerHash) { | ||
| // Skip actual trigger | ||
| LogPrint(BCLog::GOBJECT, "%s -- Not voting NO-FUNDING for trigger:%s, we voted yes for it already\n", | ||
| __func__, trigger_hash.ToString()); | ||
| continue; | ||
| } | ||
| if (vote_rec_t voteRecord; govobj->GetCurrentMNVotes(m_mn_activeman.GetOutPoint(), voteRecord)) { | ||
| const auto& strFunc = __func__; | ||
| if (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; | ||
| })) { | ||
| continue; | ||
| } | ||
| } | ||
| if (!VoteFundingTrigger(trigger_hash, VOTE_OUTCOME_NO)) { | ||
| LogPrint(BCLog::GOBJECT, "%s -- Voting NO-FUNDING for trigger:%s failed\n", __func__, trigger_hash.ToString()); | ||
| // failing here is ok-ish | ||
| continue; | ||
| } | ||
| LogPrint(BCLog::GOBJECT, "%s -- Voting NO-FUNDING for trigger:%s success\n", __func__, trigger_hash.ToString()); | ||
| } | ||
| } | ||
|
|
||
| bool GovernanceSigner::VoteFundingTrigger(const uint256& nHash, const vote_outcome_enum_t outcome) | ||
| { | ||
| CGovernanceVote vote(m_mn_activeman.GetOutPoint(), nHash, VOTE_SIGNAL_FUNDING, outcome); | ||
| vote.SetTime(GetAdjustedTime()); | ||
| vote.SetSignature(m_mn_activeman.SignBasic(vote.GetSignatureHash())); | ||
|
|
||
| CGovernanceException exception; | ||
| if (!m_govman.ProcessVoteAndRelay(vote, exception, m_connman)) { | ||
| LogPrint(BCLog::GOBJECT, "%s -- Vote FUNDING %d for trigger:%s failed:%s\n", __func__, outcome, | ||
| nHash.ToString(), exception.what()); | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| bool GovernanceSigner::HasAlreadyVotedFundingTrigger() const | ||
| { | ||
| return votedFundingYesTriggerHash.has_value(); | ||
| } | ||
|
|
||
| void GovernanceSigner::ResetVotedFundingTrigger() | ||
| { | ||
| votedFundingYesTriggerHash = std::nullopt; | ||
| } | ||
|
|
||
| void GovernanceSigner::UpdatedBlockTip(const CBlockIndex* pindex) | ||
| { | ||
| const auto sb_opt = CreateSuperblockCandidate(pindex->nHeight); | ||
| const auto trigger_opt = CreateGovernanceTrigger(sb_opt); | ||
| VoteGovernanceTriggers(trigger_opt); | ||
| CSuperblock_sptr pSuperblock; | ||
| if (m_govman.GetBestSuperblock(m_dmnman.GetListAtChainTip(), pSuperblock, pindex->nHeight)) { | ||
| ResetVotedFundingTrigger(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix clang-format violations before merging.
The CI pipeline reports clang-format differences in this file. Run ./contrib/devtools/clang-format-diff.py on your changes and commit the formatting fixes.
#!/bin/bash
# Check the specific formatting differences
git diff -U0 origin/develop -- src/governance/signing.cpp | ./contrib/devtools/clang-format-diff.py -p1🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 1-1: Clang-format differences detected in signing.cpp. Command failed: git diff -U0 origin/develop --
🤖 Prompt for AI Agents
In src/governance/signing.cpp lines 1-297: the CI flagged clang-format style
violations; run the provided clang-format-diff.py against your branch changes
(e.g. git diff -U0 origin/develop -- src/governance/signing.cpp |
./contrib/devtools/clang-format-diff.py -p1), apply the suggested formatting
fixes (or run clang-format over the file), verify the diff is clean, and commit
the formatting-only changes before pushing.
| #include <memory> | ||
| #include <optional> | ||
| #include <vector> | ||
|
|
||
| class CActiveMasternodeManager; | ||
| class CBlockIndex; | ||
| class CConnman; | ||
| class CDeterministicMNList; | ||
| class CDeterministicMNManager; | ||
| class CGovernanceException; | ||
| class CGovernanceObject; | ||
| class CGovernanceVote; | ||
| class ChainstateManager; | ||
| class CMasternodeSync; | ||
| class CNode; | ||
| class CSuperblock; | ||
| enum vote_outcome_enum_t : int; | ||
|
|
||
| class GovernanceSignerParent | ||
| { | ||
| public: | ||
| virtual ~GovernanceSignerParent() = default; | ||
|
|
||
| virtual bool IsValid() const = 0; | ||
| virtual bool GetBestSuperblock(const CDeterministicMNList& tip_mn_list, | ||
| std::shared_ptr<CSuperblock>& pSuperblockRet, int nBlockHeight) = 0; | ||
| virtual bool MasternodeRateCheck(const CGovernanceObject& govobj, bool fUpdateFailStatus = false) = 0; | ||
| virtual bool ProcessVoteAndRelay(const CGovernanceVote& vote, CGovernanceException& exception, CConnman& connman) = 0; | ||
| virtual int GetCachedBlockHeight() const = 0; | ||
| virtual CGovernanceObject* FindGovernanceObject(const uint256& nHash) = 0; | ||
| virtual CGovernanceObject* FindGovernanceObjectByDataHash(const uint256& nDataHash) = 0; | ||
| virtual std::vector<std::shared_ptr<CSuperblock>> GetActiveTriggers() const = 0; | ||
| virtual std::vector<std::shared_ptr<const CGovernanceObject>> GetApprovedProposals( | ||
| const CDeterministicMNList& tip_mn_list) = 0; | ||
| virtual void AddGovernanceObject(CGovernanceObject& govobj, const CNode* pfrom = nullptr) = 0; | ||
| }; | ||
|
|
||
| class GovernanceSigner | ||
| { | ||
| private: | ||
| CConnman& m_connman; | ||
| CDeterministicMNManager& m_dmnman; | ||
| GovernanceSignerParent& m_govman; | ||
| const CActiveMasternodeManager& m_mn_activeman; | ||
| const ChainstateManager& m_chainman; | ||
| const CMasternodeSync& m_mn_sync; | ||
|
|
||
| private: | ||
| std::optional<uint256> votedFundingYesTriggerHash{std::nullopt}; | ||
|
|
||
| public: | ||
| explicit GovernanceSigner(CConnman& connman, CDeterministicMNManager& dmnman, GovernanceSignerParent& govman, | ||
| const CActiveMasternodeManager& mn_activeman, const ChainstateManager& chainman, | ||
| const CMasternodeSync& mn_sync); | ||
| ~GovernanceSigner(); | ||
|
|
||
| void UpdatedBlockTip(const CBlockIndex* pindex); | ||
|
|
||
| private: | ||
| bool HasAlreadyVotedFundingTrigger() const; | ||
| bool VoteFundingTrigger(const uint256& nHash, const vote_outcome_enum_t outcome); | ||
| std::optional<const CGovernanceObject> CreateGovernanceTrigger(const std::optional<const CSuperblock>& sb_opt); | ||
| std::optional<const CSuperblock> CreateSuperblockCandidate(int nHeight) const; | ||
| void ResetVotedFundingTrigger(); | ||
| void VoteGovernanceTriggers(const std::optional<const CGovernanceObject>& trigger_opt); | ||
| }; |
There was a problem hiding this comment.
Include the full CGovernanceObject definition
std::optional<const CGovernanceObject> requires a complete type, but this header only forward-declares CGovernanceObject. Any TU that includes governance/signing.h before governance/object.h will fail to compile. Please include the appropriate header (e.g. #include <governance/object.h>) here so the type is complete at the point of instantiation.
🤖 Prompt for AI Agents
In src/governance/signing.h around lines 10 to 75, the header only
forward-declares CGovernanceObject but uses std::optional<const
CGovernanceObject>, which requires a complete type and will break compilation if
this header is included before the CGovernanceObject definition; fix it by
adding the appropriate include (for example #include "governance/object.h") near
the top with the other includes so the full CGovernanceObject definition is
available when std::optional<const CGovernanceObject> is instantiated.
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 #6842Dependency for refactor: update lock annotations for
GovernanceStore,CGovernanceManagerandCGovernanceStore, move away fromRecursiveMutexes #6849We are dropping compatibility code introduced as part of dash#4672 aimed at avoiding bans from pre-v0.18 nodes. As we are multiple major versions and hard forks ahead of v0.18, we can drop these checks and simplify relay logic.
To 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