refactor: drop fMasternodeMode global, create new context for masternode mode-only logic (ActiveContext), move spun-off signers and EHF signals handler
#6828
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
kwvg
force-pushed
the
active_ctx
branch
3 times, most recently
from
e2ff3c6 to
08db0fd
Compare
WalkthroughIntroduces a new masternode ActiveContext and ActiveNotificationInterface, exports their headers, and wires them into init, net_processing, RPC, and tests. Refactors ChainLocks and InstantSend to externally managed signer attachment (ConnectSigner/DisconnectSigner) and removes CSigSharesManager/is_masternode constructor params. Converts several pointer-based members to references (e.g., CCoinJoinServer deps), removes global fMasternodeMode in favor of CConnman::m_active_masternode with IsActiveMasternode(). Simplifies CJContext constructor (drops connman/peerman). Removes LLMQ EHF handler from LLMQContext; adds standalone EHF integration via ActiveContext and its notification interface. Adjusts governance, sync, wallet logic, and updates circular dependency expectations. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/chainlock/chainlock.cpp (1)
454-463: Potential nullptr deref when processing cleanup_txes; guard per prior issue.Given the known null-check gap noted previously in ChainLockSigner::Cleanup() (per PR #6761 follow-up), this loop should tolerate nullptr entries to avoid crashes.
- if (m_signer) { - const auto cleanup_txes{m_signer->Cleanup()}; - LOCK(cs); - for (const auto& tx : cleanup_txes) { - for (const auto& txid : *tx) { - txFirstSeenTime.erase(txid); - } - } - } + if (m_signer) { + const auto cleanup_txes{m_signer->Cleanup()}; + LOCK(cs); + for (const auto& tx : cleanup_txes) { + if (!tx) continue; // be defensive against null entries + for (const auto& txid : *tx) { + txFirstSeenTime.erase(txid); + } + } + }Note: This aligns with the preference to avoid scope creep while still shoring up a crash vector locally.
src/net_processing.cpp (1)
2876-2886: Null-deref risk: guard cj_server when fetching DSQUEUE in ProcessGetData().Same issue as above: only checking m_active_ctx is insufficient. Ensure cj_server exists before dereferencing.
- 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;
🧹 Nitpick comments (32)
src/net.cpp (2)
1804-1821: Switched eviction protection gating to per-instance masternode state — good; consider accessor for consistencyUsing the instance field m_active_masternode removes reliance on the old global and scopes behavior to the CConnman instance. For consistency with other call sites that use IsActiveMasternode(), you might prefer the accessor here as well.
- if (m_active_masternode) { + if (IsActiveMasternode()) {
1974-1977: Inbound gating while unsynced is now per-instance — good; mirror accessor usageThe inbound gate now depends on the CConnman instance being an active masternode, matching the intent of dropping the global. For codebase consistency, consider the accessor here too.
- if (m_active_masternode && !mn_sync.IsBlockchainSynced()) { + if (IsActiveMasternode() && !mn_sync.IsBlockchainSynced()) {src/net.h (3)
1197-1198: Additive API looks good; clarify latched semantics of m_active_masternode.Adding Options::m_active_masternode with a default false is fine. Please document that this flag is latched during Init() and is not expected to change at runtime, to avoid future misuse (e.g., someone toggling it after threads start).
Apply:
struct Options { @@ - bool m_i2p_accept_incoming; + bool m_i2p_accept_incoming; + // Set once during Init(); indicates if the node runs in active masternode mode. + // Not intended to be mutated after threads are started. bool m_active_masternode = false; };
1260-1261: Accessor addition is fine; name matches semantics.IsActiveMasternode() is a clear replacement for the former global. Please add a brief comment to reinforce that this value is stable after Init().
- bool IsActiveMasternode() const { return m_active_masternode; } + // Latched during Init(); not expected to change at runtime. + bool IsActiveMasternode() const { return m_active_masternode; }
1235-1236: No unexpected writes; thread-safety future-proofing optionalI verified with ripgrep that the only write to
CConnman::m_active_masternodeoccurs in itsInitmethod (src/net.h:1235), and the only place the correspondingOptions::m_active_masternodeis set is in src/init.cpp:2441. There are no other assignments to this field, so the current startup-time initialization is thread-safe.Optional suggestion:
- If you ever need to toggle
m_active_masternodeat runtime after worker threads are running, convert the member inCConnmantostd::atomic_boolto ensure proper visibility across threads.test/lint/lint-circular-dependencies.py (1)
25-27: New expected cycles acknowledge ActiveContext wiring; consider breaking them post-merge.
- active/context -> coinjoin/server -> net_processing -> active/context
- coinjoin/coinjoin -> instantsend/instantsend -> net_processing -> coinjoin/context -> coinjoin/coinjoin
These likely stem from including concrete types across layers. After this refactor lands, we should aim to break these with forward declarations and dependency inversion (interfaces injected into net_processing). Not blocking.
Potential approaches:
- Replace net_processing includes in coinjoin/server and instantsend with forward declarations + factory methods.
- Have ActiveContext expose minimal interfaces (pure virtuals) rather than concrete types to net_processing.
src/llmq/ehf_signals.h (1)
44-45: UpdatedBlockTip signature removal of is_masternode parameter is fine; ensure call site cannot null-deref handler.The handler no longer needs the is_masternode toggle, which aligns with ActiveContext scoping. However, ActiveNotificationInterface appears to call m_active_ctx.ehf_sighandler->UpdatedBlockTip(pindexNew) unconditionally. If ehf_sighandler can be absent on non-MN nodes, that’s a crash.
Please confirm ehf_sighandler is always non-null in ActiveContext for all nodes. If not, guard the call:
diff --git a/src/active/notificationinterface.cpp b/src/active/notificationinterface.cpp @@ - m_active_ctx.ehf_sighandler->UpdatedBlockTip(pindexNew); + if (m_active_ctx.ehf_sighandler) { + m_active_ctx.ehf_sighandler->UpdatedBlockTip(pindexNew); + }Optionally mark the method noexcept since it should not throw:
- void UpdatedBlockTip(const CBlockIndex* const pindexNew) EXCLUSIVE_LOCKS_REQUIRED(!cs); + void UpdatedBlockTip(const CBlockIndex* const pindexNew) noexcept EXCLUSIVE_LOCKS_REQUIRED(!cs);src/chainlock/chainlock.h (1)
49-50: Raw pointer for m_signer: define clear lifetime/locking and assert on destruction.Using a raw pointer is fine for non-ownership, but add defense-in-depth:
- Annotate usage with a brief comment (non-owning, must be detached before destruction).
- Add a debug assertion in the destructor that m_signer == nullptr to catch shutdown leaks.
- chainlock::ChainLockSigner* m_signer{nullptr}; + // Non-owning; attached via ConnectSigner(), must be detached via DisconnectSigner() before destruction. + chainlock::ChainLockSigner* m_signer{nullptr};And in src/chainlock/chainlock.cpp destructor:
CChainLocksHandler::~CChainLocksHandler() { + assert(m_signer == nullptr && "DisconnectSigner() must be called before ~CChainLocksHandler()"); scheduler->stop(); scheduler_thread->join(); }src/coinjoin/server.cpp (1)
58-72: Reference-based m_mn_activeman usage looks correct; ensure invariant holds (server exists only in MN mode).Switching from pointer derefs to reference semantics at DSACCEPT time (GetOutPoint/GetProTxHash) removes the need for null checks and simplifies flow. Given ActiveContext constructs CCoinJoinServer only in masternode mode, this is sound. Consider adding a debug assertion at CCoinJoinServer construction (or here) to document the invariant for future maintainers.
Would you like a small follow-up PR adding an assertion in the CCoinJoinServer ctor to confirm that the provided CActiveMasternodeManager corresponds to a valid/registered MN at chain tip?
src/coinjoin/context.h (1)
33-36: CJContext decoupling is solid; constructor and ownership changes are coherent.
- Dropping server/connman/peerman from CJContext keeps it wallet/client-centric, as intended.
- Passing mn_activeman only to infer is_masternode for walletman/queueman is reasonable.
Optional: in a future sweep, consider passing a simple bool is_masternode instead of the pointer to reduce coupling further (constructor already treats it as a boolean).
Also applies to: 43-44
src/wallet/interfaces.cpp (1)
434-437: Forwarding of new fSkipMnCollateral flag looks correctThe WalletImpl override now cleanly propagates all three flags to CWallet::GetAnonymizableBalance.
Consider documenting the semantics/order of the three booleans at the declaration site (interfaces/wallet.h) or replacing them with a small flags enum for readability in future work. No action required for this PR.
src/coinjoin/util.cpp (1)
135-138: Avoid unused structured-binding variable (“_”) to prevent -Wunused-variable warningsUsing a named-but-unused binding often triggers warnings with our flags. Prefer accessing .second explicitly, or bind both names and cast-to-void.
Apply this minimal refactor to avoid the unused binding and keep intent clear:
- for (const auto& [_, outpoint] : tallyItem.coins) { - coinControl.Select(outpoint); - dummyTx.vin.emplace_back(outpoint, CScript()); - } + for (const auto& coin : tallyItem.coins) { + const COutPoint& outpoint = coin.second; + coinControl.Select(outpoint); + dummyTx.vin.emplace_back(outpoint, CScript()); + }src/masternode/node.h (1)
30-31: Decoupling from CValidationInterface — good moveDropping inheritance simplifies responsibilities. With ActiveNotificationInterface now mediating validation callbacks, this is the right direction.
Since the class no longer inherits from CValidationInterface, you can likely drop the heavy include and forward-declare CBlockIndex instead, reducing compile-time coupling:
-#include <validationinterface.h> +class CBlockIndex;Keep threadsafety annotations by retaining <threadsafety.h>. Ensure node.cpp or other .cpp files include validationinterface.h if needed.
src/interfaces/wallet.h (1)
239-239: Verification complete — no outdated calls remainI’ve confirmed that:
- The three-argument override exists in
src/wallet/interfaces.cpp(line 434).- There are no remaining two-argument calls to either
getAnonymizableBalance(...)orGetAnonymizableBalance(...)elsewhere in the codebase.Please add the following parameter documentation above the interface to improve clarity:
- //! Get anonymizable balance. - virtual CAmount getAnonymizableBalance(bool fSkipDenominated, bool fSkipMnCollateral, bool fSkipUnconfirmed) = 0; + //! Get anonymizable balance. + //! Params: + //! - fSkipDenominated Exclude denominated outputs that are reserved for mixing. + //! - fSkipMnCollateral Exclude outputs that match masternode collateral amounts. + //! - fSkipUnconfirmed Exclude unconfirmed UTXOs. + virtual CAmount getAnonymizableBalance(bool fSkipDenominated, bool fSkipMnCollateral, bool fSkipUnconfirmed) = 0;src/llmq/ehf_signals.cpp (1)
34-47: Confirm ActiveContext gating; optionally skip during IBD
- CEHFSignalsHandler is only constructed as part of ActiveContext (in src/active/context.cpp), which is only initialized in masternode mode—so non-MN nodes will never invoke UpdatedBlockTip.
- To reduce unnecessary work during initial block download, you may opt to early-exit in IBD before checking deployments.
Suggested minimal refactor (optional):
void CEHFSignalsHandler::UpdatedBlockTip(const CBlockIndex* const pindexNew) { + // Skip signing signals while catching up during IBD. + if (m_chainman.ActiveChainstate().IsInitialBlockDownload()) return; if (!DeploymentActiveAfter(pindexNew, Params().GetConsensus(), Consensus::DEPLOYMENT_V20)) return; … }src/wallet/test/wallet_tests.cpp (1)
1429-1436: Switch to tallyItem.coins: add a lightweight value assertion for completenessThe migration from outpoints.size() to coins.size() matches the new CompactTallyItem API. To slightly strengthen the test, you can assert the expected coin value for the single-coin case.
Optional addition:
BOOST_CHECK_EQUAL(vecTally.size(), 1); BOOST_CHECK_EQUAL(vecTally.at(0).nAmount, 500 * COIN); - BOOST_CHECK_EQUAL(vecTally.at(0).coins.size(), 1); + BOOST_CHECK_EQUAL(vecTally.at(0).coins.size(), 1); + // Each entry is (value, COutPoint); verify the value for clarity + BOOST_CHECK_EQUAL(vecTally.at(0).coins.at(0).first, 500 * COIN);src/wallet/test/coinjoin_tests.cpp (1)
207-217: tallyItem.coins now stores (value, outpoint); minor clarity nit on change_pos variable reuseThe emplace_back of (tx->vout[n].nValue, COutPoint{...}) and the updated size assertion are correct and align with the refactor.
Small readability nit: nChangePosRet is reused across iterations as both an input “request” and output “result.” Consider a per-tx local to avoid accidentally carrying a requested index into the next CreateTransaction call.
Suggested tweak:
- int nChangePosRet{RANDOM_CHANGE_POSITION}; + int change_pos_for_tx{RANDOM_CHANGE_POSITION}; ... - { - auto res = CreateTransaction(*wallet, {{GetScriptForDestination(tallyItem.txdest), nAmount, false}}, nChangePosRet, coinControl); + { + auto res = CreateTransaction(*wallet, {{GetScriptForDestination(tallyItem.txdest), nAmount, false}}, change_pos_for_tx, coinControl); BOOST_CHECK(res); tx = res->tx; - nChangePosRet = res->change_pos; + change_pos_for_tx = res->change_pos; } ... for (uint32_t n = 0; n < tx->vout.size(); ++n) { - if (nChangePosRet != RANDOM_CHANGE_POSITION && int(n) == nChangePosRet) { + if (change_pos_for_tx != RANDOM_CHANGE_POSITION && int(n) == change_pos_for_tx) { // Skip the change output to only return the requested coins continue; } tallyItem.coins.emplace_back(tx->vout[n].nValue, COutPoint{tx->GetHash(), n}); tallyItem.nAmount += tx->vout[n].nValue; } - assert(tallyItem.coins.size() == vecAmounts.size()); + assert(tallyItem.coins.size() == vecAmounts.size());src/active/context.cpp (3)
21-29: Assert LLMQContext dependencies are initialized (avoid latent nullptr derefs).cl_signer, is_signer, cj_server, and ehf_sighandler immediately dereference llmq_ctx subcomponents (clhandler, isman, sigman, shareman, qman). If ActiveContext is ever constructed before LLMQContext finishes wiring, this will crash. Add cheap debug asserts to encode the construction-order contract.
ActiveContext::ActiveContext(ChainstateManager& chainman, CConnman& connman, CDeterministicMNManager& dmnman, CDSTXManager& dstxman, CMasternodeMetaMan& mn_metaman, CMNHFManager& mnhfman, LLMQContext& llmq_ctx, CSporkManager& sporkman, CTxMemPool& mempool, PeerManager& peerman, const CActiveMasternodeManager& mn_activeman, const CMasternodeSync& mn_sync) : m_llmq_ctx{llmq_ctx}, + // Precondition (debug): LLMQContext must be fully initialized before ActiveContext + // (avoids nullptr derefs when constructing signers/handlers below). + /* NOLINTNEXTLINE(readability-braces-around-statements) */ assert(m_llmq_ctx.clhandler), + /* NOLINTNEXTLINE(readability-braces-around-statements) */ assert(m_llmq_ctx.isman), + /* NOLINTNEXTLINE(readability-braces-around-statements) */ assert(m_llmq_ctx.sigman), + /* NOLINTNEXTLINE(readability-braces-around-statements) */ assert(m_llmq_ctx.shareman), + /* NOLINTNEXTLINE(readability-braces-around-statements) */ assert(m_llmq_ctx.qman), cl_signer{std::make_unique<chainlock::ChainLockSigner>(chainman.ActiveChainstate(), *llmq_ctx.clhandler, *llmq_ctx.sigman, *llmq_ctx.shareman, sporkman, mn_sync)}, is_signer{std::make_unique<instantsend::InstantSendSigner>(chainman.ActiveChainstate(), *llmq_ctx.clhandler, *llmq_ctx.isman, *llmq_ctx.sigman, *llmq_ctx.shareman, *llmq_ctx.qman, sporkman, mempool, mn_sync)},Follow-up: please confirm init/shutdown order guarantees that LLMQContext outlives ActiveContext. If not, we should guard destructor usage as well (see below).
31-33: Make signer attachment robust to future lifecycle changes.ConnectSigner() calls are correct. To reduce the chance of disconnect omissions during refactors, consider RAII for the attachment (e.g., a small ScopedSignerAttachment held by ActiveContext that auto-disconnects in its destructor).
If you want, I can sketch a minimal RAII helper and wire it in a follow-up.
35-39: Destructor assumes LLMQContext still alive; add defensive checks or document invariant.If ActiveContext were ever destroyed after LLMQContext, these calls would UB. Either:
- Document the invariant that LLMQContext outlives ActiveContext, or
- Add defensive null checks (cheap) to prevent surprises in edge teardown paths.
ActiveContext::~ActiveContext() { - m_llmq_ctx.clhandler->DisconnectSigner(); - m_llmq_ctx.isman->DisconnectSigner(); + if (m_llmq_ctx.clhandler) m_llmq_ctx.clhandler->DisconnectSigner(); + if (m_llmq_ctx.isman) m_llmq_ctx.isman->DisconnectSigner(); }src/active/notificationinterface.cpp (1)
17-22: Mirror dsnotification semantics: skip pure disconnects and optionally gate during IBD.DSNotificationInterface::UpdatedBlockTip returns early when pindexNew == pindexFork and also exits during initial download. ActiveNotificationInterface should likely follow the same pattern to avoid redundant work and reduce churn during IBD.
void ActiveNotificationInterface::UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork, bool fInitialDownload) { - m_mn_activeman.UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); - m_active_ctx.ehf_sighandler->UpdatedBlockTip(pindexNew); + if (pindexNew == pindexFork) return; + m_mn_activeman.UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); + if (!fInitialDownload) { + // CEHFSignalsHandler::UpdatedBlockTip internally checks deployments, but skipping during IBD + // matches existing notification semantics and avoids unnecessary work. + m_active_ctx.ehf_sighandler->UpdatedBlockTip(pindexNew); + } }Please confirm there is no intentional behavioral difference here (e.g., EHF wanting earlier signals during IBD).
src/test/denialofservice_tests.cpp (2)
23-24: Header include likely incidental.Including <active/context.h> in this test compiles fine; however, ideally net_processing.h should forward-declare ActiveContext so tests don't need the concrete header when passing nullptr. If that is already in place, the include can be dropped.
151-155: ✅ All PeerManager::make call sites include the new active_ctx parameter — verification completeI’ve grep’d every occurrence of
PeerManager::make(across the codebase and confirmed that each call now supplies the/*active_ctx=*/argument in the correct position:
- src/init.cpp:2148–2150
- src/net_processing.cpp (factory signature at 1948–1950)
- src/test/net_peer_connection_tests.cpp:88–90
- src/test/util/setup_common.cpp:335–337
- src/test/denialofservice_tests.cpp:151–153, 255–257, 322–324, 427–429
LGTM — all call sites have been updated consistently.
Optional suggestion for future-proofing: you may want to refactor the factory to use a named-arguments builder or a single “dependency bag” struct to guard against parameter-order churn down the road.
src/active/notificationinterface.h (1)
8-8: Include what you use: add for std::unique_ptrThis header declares a std::unique_ptr but doesn’t include . Avoid relying on transitive includes.
#include <validationinterface.h> +#include <memory>src/test/util/setup_common.cpp (1)
48-48: Drop unused include to keep test build leanactive/context.h isn’t referenced in this TU. Consider removing it to reduce dependencies.
-#include <active/context.h>src/wallet/wallet.h (2)
135-137: Prefer a named struct over std::pair for per-coin entries; keep nAmount and coins in syncUsing std::pair<CAmount, COutPoint> is terse but harms readability and is error-prone (ordering confusion of <amount, outpoint>). Also, having both nAmount and coins introduces an invariant that must be maintained. Consider a small struct and a helper to append coins that also updates nAmount (or assert invariants in debug).
Apply this refactor for clarity and to enforce invariants:
struct CompactTallyItem { CTxDestination txdest; CAmount nAmount{0}; - std::vector<std::pair<CAmount, COutPoint>> coins{}; + struct TallyCoin { + CAmount amount; + COutPoint outpoint; + }; + std::vector<TallyCoin> coins{}; CompactTallyItem() = default; };And add a tiny helper (header-only, inline) to keep nAmount and coins consistent:
struct CompactTallyItem { + inline void AddCoin(CAmount a, const COutPoint& op) { + nAmount += a; + coins.push_back(TallyCoin{a, op}); + } };Then replace ad-hoc increments with
AddCoin(...)at call sites (e.g., in SelectCoinsGroupedByAddresses).
If you’d rather not add a helper, at least add a debug-only assertion where you construct vecTallyRet:#ifndef NDEBUG CAmount sum = 0; for (const auto& c : item.second.coins) sum += c.amount; assert(sum == item.second.nAmount); #endif
653-656: Add documentation for the new fSkipMnCollateral parameterThe API signature across
CWallet, its interface, and all callers has been updated consistently to includefSkipMnCollateral. To prevent misuse, please add a brief doc comment describing this flag in both the wallet header and the interface header.Files to update:
src/wallet/wallet.h(around line 653)src/interfaces/wallet.h(around line 239)Suggested diff (adjust insertion point as needed):
--- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -650,6 +650,13 @@ public: void ResendWalletTransactions(); + /** + * Get anonymizable balance. + * + * @param fSkipDenominated Exclude denominated CoinJoin outputs. + * @param fSkipMnCollateral Exclude deterministic masternode collateral-sized UTXOs. + * @param fSkipUnconfirmed Exclude unconfirmed outputs. + */ CAmount GetAnonymizableBalance(bool fSkipDenominated, bool fSkipMnCollateral, bool fSkipUnconfirmed) const; float GetAverageAnonymizedRounds() const; CAmount GetNormalizedAnonymizedBalance() const;--- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -236,6 +236,12 @@ public: virtual CAmount getBalance() = 0; + /** + * Get anonymizable balance. + * + * @param fSkipDenominated Exclude denominated CoinJoin outputs. + * @param fSkipMnCollateral Exclude deterministic masternode collateral-sized UTXOs. + * @param fSkipUnconfirmed Exclude unconfirmed outputs. + */ virtual CAmount getAnonymizableBalance(bool fSkipDenominated, bool fSkipMnCollateral, bool fSkipUnconfirmed) = 0; virtual float getAverageAnonymizedRounds() = 0;src/instantsend/instantsend.h (2)
56-57: Non-owning m_signer pointer: document lifetime and thread-safety expectationsUsing a raw non-owning pointer is fine given the ActiveContext wiring, but please explicitly document that:
- ConnectSigner must be called before Start(), and DisconnectSigner after Stop().
- The signer’s lifetime must outlive any access from this manager.
- Access is not synchronized; Connect/Disconnect must not race with reads on worker threads.
Alternatively, consider making m_signer an std::atomic<InstantSendSigner*> for benign races on pointer loads, though comments may suffice if initialization/shutdown order is guaranteed.
91-98: Add missing header for gsl::not_null and guard against double-disconnectConnect/Disconnect APIs are good. Two small tweaks:
- Ensure the header explicitly includes whatever defines gsl::not_null (to avoid transitive-include brittleness).
- Optionally assert in DisconnectSigner that we actually had a signer.
Apply:
#include <instantsend/signing.h> +#include <gsl/gsl> // or the project-specific gsl header that provides gsl::not_null void ConnectSigner(gsl::not_null<instantsend::InstantSendSigner*> signer) { // Prohibit double initialization assert(m_signer == nullptr); m_signer = signer; } - void DisconnectSigner() { m_signer = nullptr; } + void DisconnectSigner() { + assert(m_signer != nullptr); + m_signer = nullptr; + }If the project uses a different include path for GSL, swap
<gsl/gsl>accordingly.src/instantsend/instantsend.cpp (2)
76-81: Start ordering: start the signer before launching the IS worker to avoid a potential raceWorkThreadMain may consult m_signer and call ProcessPendingRetryLockTxs shortly after the thread is launched. Starting the signer first removes a window where the worker might observe a connected but not-yet-started signer (or worse, a concurrent TransactionAddedToMempool invokes signer methods before Start).
Apply:
- 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); });If there’s a strong reason to keep the current order, please document thread-safety guarantees for calling signer methods pre-Start.
916-941: Worker loop: benign read of m_signer without synchronization; document ordering guaranteesThe worker reads m_signer without locks. Given Connect/Disconnect are intended to run only during init/shutdown, this is acceptable. Please add a short comment near m_signer or in Connect/Disconnect guaranteeing no runtime re-wiring during operation.
src/net_processing.cpp (1)
599-603: Optional: decouple from unique_ptr ownership in member type for ActiveContext.Storing ActiveContext as
const std::unique_ptr<ActiveContext>&tightly couples PeerManagerImpl to the owner’s storage type. A raw pointer (ActiveContext* const) orstd::weak_ptr(if shared ownership ever becomes needed) would make the dependency clearer and ease future refactors without changing lifetime management. Not blocking—consistent with other members like m_dmnman.If you decide to refactor later:
- Change data member to
ActiveContext* const m_active_ctx;- Pass
active_ctx.get()at construction sites.Would you like a follow-up patch sketch to perform this refactor across the call sites?
Also applies to: 781-784, 1954-1959, 1967-1981
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (45)
src/Makefile.am(2 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/client.cpp(6 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/coinjoin/util.cpp(1 hunks)src/dsnotificationinterface.cpp(0 hunks)src/governance/governance.cpp(1 hunks)src/init.cpp(8 hunks)src/instantsend/instantsend.cpp(1 hunks)src/instantsend/instantsend.h(2 hunks)src/interfaces/wallet.h(1 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.h(2 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/qt/overviewpage.cpp(1 hunks)src/rpc/coinjoin.cpp(2 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/wallet/coinjoin.cpp(6 hunks)src/wallet/interfaces.cpp(1 hunks)src/wallet/test/coinjoin_tests.cpp(1 hunks)src/wallet/test/wallet_tests.cpp(2 hunks)src/wallet/wallet.h(3 hunks)test/lint/lint-circular-dependencies.py(1 hunks)test/util/data/non-backported.txt(1 hunks)
💤 Files with no reviewable changes (4)
- src/dsnotificationinterface.cpp
- src/llmq/context.h
- src/util/system.h
- src/util/system.cpp
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/node/context.cppsrc/rpc/coinjoin.cppsrc/governance/governance.cppsrc/net.cppsrc/wallet/interfaces.cppsrc/active/notificationinterface.hsrc/interfaces/wallet.hsrc/coinjoin/util.cppsrc/net_processing.hsrc/active/context.cppsrc/masternode/node.hsrc/chainlock/chainlock.hsrc/active/context.hsrc/llmq/ehf_signals.hsrc/active/notificationinterface.cppsrc/test/net_peer_connection_tests.cppsrc/chainlock/chainlock.cppsrc/wallet/test/coinjoin_tests.cppsrc/masternode/sync.cppsrc/wallet/test/wallet_tests.cppsrc/net.hsrc/instantsend/instantsend.cppsrc/qt/overviewpage.cppsrc/coinjoin/server.cppsrc/test/util/setup_common.cppsrc/node/context.hsrc/coinjoin/context.cppsrc/wallet/wallet.hsrc/instantsend/instantsend.hsrc/coinjoin/client.cppsrc/net_processing.cppsrc/llmq/context.cppsrc/coinjoin/server.hsrc/wallet/coinjoin.cppsrc/coinjoin/context.hsrc/init.cppsrc/llmq/ehf_signals.cppsrc/test/denialofservice_tests.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/node.hsrc/masternode/sync.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/net_peer_connection_tests.cppsrc/wallet/test/coinjoin_tests.cppsrc/wallet/test/wallet_tests.cppsrc/test/util/setup_common.cppsrc/test/denialofservice_tests.cpp
🧠 Learnings (7)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
📚 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/active/context.hsrc/instantsend/instantsend.hsrc/llmq/context.cpp
📚 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.hsrc/chainlock/chainlock.cpp
📚 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-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/wallet/test/wallet_tests.cpp
🧬 Code graph analysis (20)
src/active/notificationinterface.h (5)
src/masternode/node.cpp (1)
CActiveMasternodeManager(52-61)src/active/notificationinterface.cpp (3)
ActiveNotificationInterface(11-15)UpdatedBlockTip(17-22)UpdatedBlockTip(17-18)src/llmq/ehf_signals.cpp (2)
UpdatedBlockTip(34-47)UpdatedBlockTip(34-34)src/dsnotificationinterface.cpp (2)
UpdatedBlockTip(72-99)UpdatedBlockTip(72-72)src/active/context.cpp (2)
ActiveContext(16-33)ActiveContext(35-39)
src/interfaces/wallet.h (1)
src/wallet/interfaces.cpp (2)
fSkipDenominated(434-437)fSkipDenominated(434-434)
src/net_processing.h (1)
src/active/context.cpp (2)
ActiveContext(16-33)ActiveContext(35-39)
src/masternode/node.h (2)
src/masternode/node.cpp (3)
CActiveMasternodeManager(52-61)UpdatedBlockTip(171-211)UpdatedBlockTip(171-171)src/active/notificationinterface.cpp (2)
UpdatedBlockTip(17-22)UpdatedBlockTip(17-18)
src/chainlock/chainlock.h (4)
src/active/context.h (1)
chainlock(23-25)src/chainlock/chainlock.cpp (2)
CChainLocksHandler(46-57)CChainLocksHandler(59-63)src/instantsend/instantsend.h (2)
ConnectSigner(91-96)DisconnectSigner(97-97)src/chainlock/signing.cpp (2)
ChainLockSigner(18-28)ChainLockSigner(30-30)
src/active/context.h (7)
src/coinjoin/coinjoin.h (1)
CDSTXManager(378-406)src/net_processing.h (1)
PeerManager(67-67)src/llmq/ehf_signals.h (2)
llmq(16-52)CEHFSignalsHandler(21-51)src/llmq/context.h (1)
llmq(23-33)src/llmq/ehf_signals.cpp (2)
CEHFSignalsHandler(18-27)CEHFSignalsHandler(29-32)src/active/context.cpp (2)
ActiveContext(16-33)ActiveContext(35-39)src/llmq/context.cpp (2)
LLMQContext(19-44)LLMQContext(46-48)
src/llmq/ehf_signals.h (2)
src/llmq/ehf_signals.cpp (2)
UpdatedBlockTip(34-47)UpdatedBlockTip(34-34)src/active/notificationinterface.cpp (2)
UpdatedBlockTip(17-22)UpdatedBlockTip(17-18)
src/active/notificationinterface.cpp (2)
src/llmq/ehf_signals.cpp (2)
UpdatedBlockTip(34-47)UpdatedBlockTip(34-34)src/dsnotificationinterface.cpp (2)
UpdatedBlockTip(72-99)UpdatedBlockTip(72-72)
src/qt/overviewpage.cpp (1)
src/qt/test/wallettests.cpp (1)
walletModel(157-157)
src/coinjoin/server.cpp (1)
src/net_processing.cpp (6)
inv(632-632)inv(633-633)inv(634-634)inv(635-635)inv(930-930)inv(2309-2309)
src/node/context.h (1)
src/active/context.cpp (2)
ActiveContext(16-33)ActiveContext(35-39)
src/wallet/wallet.h (2)
src/interfaces/wallet.h (1)
std(146-346)src/wallet/coinjoin.cpp (4)
SelectCoinsGroupedByAddresses(124-230)SelectCoinsGroupedByAddresses(124-125)GetAnonymizableBalance(433-461)GetAnonymizableBalance(433-433)
src/instantsend/instantsend.h (4)
src/instantsend/instantsend.cpp (2)
CInstantSendManager(52-65)CInstantSendManager(67-67)src/chainlock/chainlock.h (2)
ConnectSigner(73-78)DisconnectSigner(79-79)src/active/context.h (1)
instantsend(26-28)src/instantsend/signing.cpp (2)
InstantSendSigner(30-44)InstantSendSigner(46-46)
src/coinjoin/client.cpp (1)
src/wallet/interfaces.cpp (16)
m_wallet(158-161)m_wallet(166-166)m_wallet(175-175)m_wallet(176-176)m_wallet(182-182)m_wallet(183-183)m_wallet(438-441)m_wallet(442-445)m_wallet(517-517)m_wallet(518-518)m_wallet(519-519)m_wallet(520-520)m_wallet(521-521)m_wallet(526-526)m_wallet(586-586)bal(405-422)
src/coinjoin/server.h (1)
src/net_processing.h (1)
PeerManager(67-67)
src/wallet/coinjoin.cpp (1)
src/wallet/interfaces.cpp (2)
fSkipDenominated(434-437)fSkipDenominated(434-434)
src/coinjoin/context.h (1)
src/coinjoin/context.cpp (2)
CJContext(12-23)CJContext(25-25)
src/init.cpp (2)
src/coinjoin/server.cpp (2)
DoMaintenance(871-879)DoMaintenance(871-871)src/coinjoin/client.cpp (6)
DoMaintenance(1845-1852)DoMaintenance(1845-1845)DoMaintenance(1854-1871)DoMaintenance(1854-1854)DoMaintenance(1916-1922)DoMaintenance(1916-1916)
src/llmq/ehf_signals.cpp (1)
src/active/notificationinterface.cpp (2)
UpdatedBlockTip(17-22)UpdatedBlockTip(17-18)
src/test/denialofservice_tests.cpp (1)
src/net_processing.cpp (2)
make(1948-1959)make(1948-1956)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: linux64_tsan-test / Test source
| "node/coinstats -> validation -> node/coinstats", | ||
| # Dash | ||
| "active/context -> coinjoin/server -> net_processing -> active/context", | ||
| "coinjoin/coinjoin -> instantsend/instantsend -> net_processing -> coinjoin/context -> coinjoin/coinjoin", |
There was a problem hiding this comment.
just a note for reviewer:
"coinjoin/coinjoin -> instantsend/instantsend -> net_processing -> coinjoin/context -> coinjoin/coinjoin",
It is not new circular dependency, it just has been discovered by removing a shorter circule, this one:
"coinjoin/context -> coinjoin/server -> net_processing -> coinjoin/context",
| "wallet/wallet -> wallet/walletdb -> wallet/wallet", | ||
| "node/coinstats -> validation -> node/coinstats", | ||
| # Dash | ||
| "active/context -> coinjoin/server -> net_processing -> active/context", |
There was a problem hiding this comment.
Do you have any idea how to refactor it out?
"active/context -> coinjoin/server -> net_processing -> active/context",
There was a problem hiding this comment.
This issue persists for LLMQContext and CJContext as well as we manage the context, compared to NodeContext, which simply stores it.
The wallet should not concern itself with masternode mode state. Client side mixing is not available for masternodes and masternodes are no longer allowed to have the collateral in their own local wallet, drop it.
We don't initialize mn_activeman in TestingSetup so we don't need to set `connOptions.m_active_masternode`
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/coinjoin/server.cpp (1)
5-25: Missing include causes sort() to be undefined.sort is used later; add the header to avoid compile errors.
Apply:
#include <coinjoin/server.h> +#include <algorithm>src/init.cpp (1)
2154-2158: Construct CJContext before CDSNotificationInterface to avoid null CJ pointer.g_ds_notification_interface is created and registered before node.cj_ctx exists; if the ctor or early callbacks touch CJ, this can crash.
Reorder:
@@ - g_ds_notification_interface = std::make_unique<CDSNotificationInterface>( - *node.connman, *node.mn_sync, *node.govman, *node.peerman, chainman, node.mn_activeman.get(), node.dmnman, node.llmq_ctx, node.cj_ctx - ); - RegisterValidationInterface(g_ds_notification_interface.get()); - - // ********************************************************* Step 7c: Setup CoinJoin - node.cj_ctx = std::make_unique<CJContext>(chainman, *node.dmnman, *node.mn_metaman, *node.mempool, - node.mn_activeman.get(), *node.mn_sync, *node.llmq_ctx->isman, - !ignores_incoming_txs); + // ********************************************************* Step 7c: Setup CoinJoin + node.cj_ctx = std::make_unique<CJContext>(chainman, *node.dmnman, *node.mn_metaman, *node.mempool, + node.mn_activeman.get(), *node.mn_sync, *node.llmq_ctx->isman, + !ignores_incoming_txs); + + g_ds_notification_interface = std::make_unique<CDSNotificationInterface>( + *node.connman, *node.mn_sync, *node.govman, *node.peerman, chainman, node.mn_activeman.get(), node.dmnman, node.llmq_ctx, node.cj_ctx + ); + RegisterValidationInterface(g_ds_notification_interface.get());Also applies to: 2161-2163
♻️ Duplicate comments (1)
src/init.cpp (1)
2165-2175: Global g_active_notification_interface: acceptable; consider NodeContext storage (optional).Pattern matches existing globals (ZMQ/DS), but storing in NodeContext improves testability and multi-node embedding.
🧹 Nitpick comments (13)
src/llmq/ehf_signals.h (2)
8-11: Includesync.hexplicitly for Mutex and thread-safety annotationsAvoid relying on transitive includes for
Mutex,GUARDED_BY, andEXCLUSIVE_LOCKS_REQUIRED.#include <llmq/signing.h> +#include <sync.h> #include <set>Also applies to: 33-35
39-41: Make handler non-copyable/non-movableObject holds references to managers; copying/moving is unsafe. Delete special members.
~CEHFSignalsHandler(); + CEHFSignalsHandler(const CEHFSignalsHandler&) = delete; + CEHFSignalsHandler& operator=(const CEHFSignalsHandler&) = delete; + CEHFSignalsHandler(CEHFSignalsHandler&&) = delete; + CEHFSignalsHandler& operator=(CEHFSignalsHandler&&) = delete;src/masternode/node.h (1)
12-12: Drop unused include and forward-declare CBlockIndex to reduce coupling.validationinterface.h is no longer needed; add a forward declaration for CBlockIndex instead.
Apply:
-#include <validationinterface.h> +class CBlockIndex;Also applies to: 14-14
src/test/net_peer_connection_tests.cpp (1)
21-22: Include addition is fine but not strictly required.active/context.h isn't needed at this call site since nullptr is passed; harmless to keep.
test/lint/lint-circular-dependencies.py (1)
25-26: New cycles acknowledged; prefer breaking them instead of whitelisting.Consider in a follow-up: invert dependencies via a thin relay interface injected into coinjoin/server and active/context to avoid net_processing <-> active/coinjoin cycles.
Possible direction:
- Define IPeerRelay { RelayDSQ, RelayInv, RelayTransaction } in a small header.
- Implement IPeerRelay in net_processing; have coinjoin/server depend only on IPeerRelay.
- For active/context, depend on NodeContext-like storage instead of management to avoid active/context -> net_processing references.
src/coinjoin/server.cpp (2)
255-255: Format specifier mismatch for entries count.entries is an int but printed with %lu; use %d or cast to unsigned long long with %llu.
Apply:
- if (int entries = GetEntriesCount(); entries != 0) LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- entries count %lu\n", entries); + if (int entries = GetEntriesCount(); entries != 0) LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- entries count %d\n", entries);
181-185: Relay/sign refactor aligns with new PeerManager interface — LGTM; keep DoS considerations. TheRelayInvmethod still offers theminProtoVersionoverload, butRelayDSQ(...)andRelayTransaction(...)only have single overloads that broadcast to all peers. If you need to restrict DSQ or transaction messages to non-legacy peers, add appropriateminProtoVersionchecks or overloads.src/coinjoin/context.cpp (1)
22-22: dstxman default construction is fine; watch init-order coupling with walletman/queueman.WalletManager takes a reference to queueman and queueman takes WalletManager. If either ctor dereferences the other during construction, that’s UB. Please verify ctors only stash refs and do not access each other until after CJContext is fully constructed. If unsure, break the cycle with a two-phase wire-up in the ctor body (setter) or ensure declaration order initializes the referenced member first.
src/test/util/setup_common.cpp (1)
335-339: Construct CJContext before PeerManager to avoid passing an uninitialized cj_ctx.Right now PeerManager::make receives m_node.cj_ctx before it’s constructed. If its parameter is a reference (e.g., std::unique_ptr&) this is technically okay only if it defers use; safer to init CJ first.
Apply this minimal reorder:
@@ - m_node.banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME); - m_node.peerman = PeerManager::make(chainparams, *m_node.connman, *m_node.addrman, m_node.banman.get(), - *m_node.chainman, *m_node.mempool, *m_node.mn_metaman, *m_node.mn_sync, - *m_node.govman, *m_node.sporkman, /*mn_activeman=*/nullptr, m_node.dmnman, - /*active_ctx=*/nullptr, m_node.cj_ctx, m_node.llmq_ctx, /*ignore_incoming_txs=*/false); + m_node.banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME); + m_node.cj_ctx = std::make_unique<CJContext>(*m_node.chainman, *m_node.dmnman, *m_node.mn_metaman, *m_node.mempool, + /*mn_activeman=*/nullptr, *m_node.mn_sync, *m_node.llmq_ctx->isman, + /*relay_txes=*/true); + m_node.peerman = PeerManager::make(chainparams, *m_node.connman, *m_node.addrman, m_node.banman.get(), + *m_node.chainman, *m_node.mempool, *m_node.mn_metaman, *m_node.mn_sync, + *m_node.govman, *m_node.sporkman, /*mn_activeman=*/nullptr, m_node.dmnman, + /*active_ctx=*/nullptr, m_node.cj_ctx, m_node.llmq_ctx, /*ignore_incoming_txs=*/false); @@ - m_node.cj_ctx = std::make_unique<CJContext>(*m_node.chainman, *m_node.dmnman, *m_node.mn_metaman, *m_node.mempool, - /*mn_activeman=*/nullptr, *m_node.mn_sync, *m_node.llmq_ctx->isman, - /*relay_txes=*/true);Also, confirm PeerManager::make’s new params accept nullptr for mn_activeman and active_ctx without dereferencing.
Also applies to: 346-348
src/coinjoin/server.h (2)
38-39: Switching to references clarifies ownership; document lifetime.m_peerman and m_mn_activeman as references enforce non-null. Please add a brief comment that CCoinJoinServer must not outlive PeerManager or CActiveMasternodeManager and is only constructed in masternode mode.
95-112: Ctor parameter/order update looks good; consider adding runtime asserts.Optionally add assertions to guard misuse:
- Assert(&peerman) // defensive, even with refs
- Assert(mn_activeman.IsActiveMasternode()) or equivalent
This can catch accidental construction outside masternode mode.
src/init.cpp (2)
1699-1707: Verify ctor safety; consider delaying mn_activeman construction.CActiveMasternodeManager is constructed before dmnman is initialized by LoadChainstate. Verify the ctor does not dereference node.dmnman until ThreadImport::Init. Optionally, construct after LoadChainstate (or pass only the key and inject dependencies later).
Also fix misleading comment:
- // Create and register mn_activeman, will init later in ThreadImport + // Create mn_activeman (registration handled via ActiveNotificationInterface), will init later in ThreadImport
2441-2441: Optional: derive m_active_masternode directly from args.Avoid coupling to construction timing of mn_activeman.
- connOptions.m_active_masternode = node.mn_activeman != nullptr; + connOptions.m_active_masternode = !args.GetArg("-masternodeblsprivkey", "").empty();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
src/Makefile.am(2 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/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(0 hunks)src/governance/governance.cpp(1 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.h(2 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/rpc/coinjoin.cpp(2 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/wallet/coinjoin.cpp(0 hunks)test/lint/lint-circular-dependencies.py(1 hunks)test/util/data/non-backported.txt(1 hunks)
💤 Files with no reviewable changes (5)
- src/util/system.cpp
- src/util/system.h
- src/dsnotificationinterface.cpp
- src/wallet/coinjoin.cpp
- src/llmq/context.h
🚧 Files skipped from review as they are similar to previous changes (22)
- src/net_processing.h
- test/util/data/non-backported.txt
- src/governance/governance.cpp
- src/Makefile.am
- src/active/notificationinterface.cpp
- src/active/context.cpp
- src/net.cpp
- src/masternode/sync.cpp
- src/chainlock/chainlock.h
- src/llmq/ehf_signals.cpp
- src/rpc/coinjoin.cpp
- src/node/context.cpp
- src/llmq/context.cpp
- src/net.h
- src/active/notificationinterface.h
- src/chainlock/chainlock.cpp
- src/instantsend/instantsend.h
- src/net_processing.cpp
- src/node/context.h
- src/instantsend/instantsend.cpp
- src/coinjoin/context.h
- src/active/context.h
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/llmq/ehf_signals.hsrc/test/util/setup_common.cppsrc/masternode/node.hsrc/coinjoin/context.cppsrc/init.cppsrc/test/denialofservice_tests.cppsrc/test/net_peer_connection_tests.cppsrc/coinjoin/server.hsrc/coinjoin/server.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/util/setup_common.cppsrc/test/denialofservice_tests.cppsrc/test/net_peer_connection_tests.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/node.h
🧠 Learnings (2)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
📚 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
🧬 Code graph analysis (5)
src/llmq/ehf_signals.h (4)
src/active/context.h (1)
llmq(29-31)src/llmq/context.h (1)
llmq(23-33)src/llmq/ehf_signals.cpp (2)
UpdatedBlockTip(34-47)UpdatedBlockTip(34-34)src/active/notificationinterface.cpp (2)
UpdatedBlockTip(17-22)UpdatedBlockTip(17-18)
src/masternode/node.h (2)
src/masternode/node.cpp (3)
CActiveMasternodeManager(52-61)UpdatedBlockTip(171-211)UpdatedBlockTip(171-171)src/active/notificationinterface.cpp (2)
UpdatedBlockTip(17-22)UpdatedBlockTip(17-18)
src/test/denialofservice_tests.cpp (1)
src/net_processing.cpp (2)
make(1948-1959)make(1948-1956)
src/coinjoin/server.h (1)
src/net_processing.h (1)
PeerManager(67-67)
src/coinjoin/server.cpp (1)
src/net_processing.cpp (6)
inv(632-632)inv(633-633)inv(634-634)inv(635-635)inv(930-930)inv(2309-2309)
⏰ 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). (7)
- GitHub Check: Lint / Run linters
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-w64-mingw32 / 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
🔇 Additional comments (15)
src/llmq/ehf_signals.h (2)
16-16: Namespace style change looks fineNo functional impact; consistent with surrounding headers.
44-44: Ensure CEHFSignalsHandler usage is masternode-only and document precondition
- Confirm
CEHFSignalsHandleris constructed and invoked exclusively viaActiveContextwhenmn_activeman != nullptr, so non-MN nodes never callUpdatedBlockTip.- Add this doc precondition above
UpdatedBlockTip:- /** - * Since Tip is updated it could be a time to generate EHF Signal - */ + /** + * Since Tip is updated it could be a time to generate EHF Signal. + * Precondition: Called only when running as an active masternode (ActiveContext). + */src/masternode/node.h (2)
55-57: Thread-safety contract retained — LGTM.UpdatedBlockTip keeps EXCLUSIVE_LOCKS_REQUIRED(!cs); consistent with node.cpp usage.
30-31: Decoupling complete: no remaining CValidationInterface usages for CActiveMasternodeManager.src/test/net_peer_connection_tests.cpp (1)
88-92: Signature update to PeerManager::make — LGTM.Argument order matches net_processing.cpp (… dmnman, active_ctx, cj_ctx, llmq_ctx, ignore_incoming_txs).
src/test/denialofservice_tests.cpp (2)
23-24: Extra include is fine.Brings ActiveContext into scope; benign even if not required.
151-155: Verify all PeerManager::make call sites include the new banman and active_ctx parameters
- src/init.cpp:2148
- src/test/net_peer_connection_tests.cpp:88
- src/test/denialofservice_tests.cpp:151, 255, 322, 427
- src/test/util/setup_common.cpp:335
src/coinjoin/server.cpp (2)
58-63: Switch to object-based mn_activeman access — LGTM.Using GetOutPoint()/GetProTxHash() avoids pointer checks; rejection path for missing dmn is handled.
Also applies to: 70-71
683-686: Session guards clarified — LGTM.Early returns in CreateNewSession/AddUserToExistingSession make states explicit.
Also applies to: 725-726
src/coinjoin/context.cpp (2)
10-10: Header swap to coinjoin/coinjoin.h looks correct.CDSTXManager and base CoinJoin types live here; this aligns with the server removal from CJContext.
12-15: Constructor signature simplification LGTM; confirm no hidden dependencies on connman/peerman.Given CJContext no longer owns CCoinJoinServer, ensure all call sites that previously relied on CJContext->server routing are updated to use ActiveContext paths (or equivalent). Also confirm wallet clients don’t depend on net-layer objects via CJContext anymore.
src/test/util/setup_common.cpp (1)
48-48: ActiveContext include added — OK.Matches new PeerManager::make signature usage below.
src/init.cpp (3)
79-80: LGTM: new Active headers are correctly included.
2285-2285: LGTM: schedule CoinJoin server maintenance only in MN mode.
2147-2153: Verify PeerManagerImpl safely handles null contexts
ThePeerManagerImplconstructor storesactive_ctxandcj_ctxwithout dereferencing; subsequent code only ever checks forif (m_active_ctx)orif (m_cj_ctx)before using them. No null dereferences occur.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/init.cpp (1)
324-327: Shutdown order is now safe.ActiveNotificationInterface is unregistered/reset before dropping ActiveContext; mn_activeman reset happens after. This addresses the prior dangling-ref risk.
Also applies to: 331-331, 385-385
🧹 Nitpick comments (2)
src/active/context.h (1)
59-61: Optional: don’t expose ownership in the public API.Consider keeping cj_server/ehf_sighandler private and exposing const refs/getters. This avoids leaking ownership semantics while preserving read-only access.
src/init.cpp (1)
2443-2443: Minor: base m_active_masternode on CLI arg, not pointer presence.Using !strMasterNodeBLSPrivKey.empty() avoids coupling the flag to mn_activeman construction timing.
- connOptions.m_active_masternode = node.mn_activeman != nullptr; + connOptions.m_active_masternode = !strMasterNodeBLSPrivKey.empty();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/Makefile.am(2 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/dsnotificationinterface.cpp(0 hunks)src/init.cpp(8 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.h(2 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
💤 Files with no reviewable changes (2)
- src/llmq/context.h
- src/dsnotificationinterface.cpp
🚧 Files skipped from review as they are similar to previous changes (9)
- src/Makefile.am
- src/active/notificationinterface.cpp
- src/active/context.cpp
- src/masternode/node.h
- src/active/notificationinterface.h
- test/lint/lint-circular-dependencies.py
- src/llmq/ehf_signals.h
- src/llmq/context.cpp
- src/llmq/ehf_signals.cpp
🧰 Additional context used
📓 Path-based instructions (1)
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/active/context.hsrc/init.cpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
📚 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/active/context.h
🧬 Code graph analysis (1)
src/active/context.h (6)
src/coinjoin/coinjoin.h (1)
CDSTXManager(378-406)src/llmq/ehf_signals.h (2)
llmq(16-52)CEHFSignalsHandler(21-51)src/llmq/context.h (1)
llmq(23-33)src/llmq/ehf_signals.cpp (2)
CEHFSignalsHandler(18-27)CEHFSignalsHandler(29-32)src/active/context.cpp (2)
ActiveContext(16-33)ActiveContext(35-39)src/llmq/context.cpp (2)
LLMQContext(19-44)LLMQContext(46-48)
⏰ 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). (7)
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (7)
src/active/context.h (1)
33-44: Good: clear ownership and lifecycle for LLMQ signers.Non-defaultable/non-copyable, ref to LLMQContext, and explicit Connect/Disconnect in ctor/dtor are correct and minimize dangling registrations.
src/init.cpp (6)
79-80: Includes look correct and localized.
2162-2165: CJContext wiring LGTM.New ctor signature and isman reference hookup look consistent.
2166-2176: ActiveContext + ActiveNotificationInterface wiring LGTM.Creation gated by mn_activeman, assertions guard duplicates, registration timing is sane.
2286-2286: Scheduler: CJ server maintenance guarded by MN mode.Looks correct; wallet-side maintenance is scheduled in the else branch.
1706-1708: Confirmnode.dmnmaninitialization before use
The CActiveMasternodeManager ctor is declared asCActiveMasternodeManager(const CBLSSecretKey& sk, CConnman& connman, const std::unique_ptr<CDeterministicMNManager>& dmnman);so passing
node.dmnman(not*node.dmnman) is correct. Verify thatnode.dmnmanhas been constructed (is non-null) at this spot to avoid null‐pointer dereferences.
2153-2153: No changes needed: PeerManager::make already tolerates null ActiveContext
The test suite repeatedly callsPeerManager::make(..., /*active_ctx=*/nullptr, …), demonstrating it safely accepts and stores a nullactive_ctxwithout dereferencing during construction.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/llmq/context.cpp (1)
72-74: Minor stop-order nit: unregister before stopping the workerTo avoid any late-delivered callbacks hitting a stopped worker, consider unregistering before stopping:
- shareman->StopWorkerThread(); - shareman->UnregisterAsRecoveredSigsListener(); + shareman->UnregisterAsRecoveredSigsListener(); + shareman->StopWorkerThread();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/Makefile.am(2 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/dsnotificationinterface.cpp(0 hunks)src/init.cpp(8 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.h(2 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
💤 Files with no reviewable changes (2)
- src/llmq/context.h
- src/dsnotificationinterface.cpp
🚧 Files skipped from review as they are similar to previous changes (8)
- src/masternode/node.h
- src/active/notificationinterface.h
- src/llmq/ehf_signals.h
- src/active/context.cpp
- src/Makefile.am
- test/lint/lint-circular-dependencies.py
- src/llmq/ehf_signals.cpp
- src/active/context.h
🧰 Additional context used
📓 Path-based instructions (1)
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/active/notificationinterface.cppsrc/llmq/context.cppsrc/init.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
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-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/llmq/context.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/init.cpp
🧬 Code graph analysis (1)
src/active/notificationinterface.cpp (2)
src/llmq/ehf_signals.cpp (2)
UpdatedBlockTip(34-47)UpdatedBlockTip(34-34)src/dsnotificationinterface.cpp (2)
UpdatedBlockTip(72-99)UpdatedBlockTip(72-72)
⏰ 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). (7)
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (15)
src/llmq/context.cpp (3)
37-40: Constructor updates look correct and align with the new APIsPassing
chainman.ActiveChainstate(), *qman, *sigman, sporkman, mempool, mn_synctoCChainLocksHandlerand threading that through toCInstantSendManagermatches the refactor goals and reduces legacy coupling. No functional concerns here.
65-67: Startup sequencing sanity checkStarting CL first (
clhandler->Start(*isman)) and then IS (isman->Start(peerman)) is sensible given IS depends on CL for notifications. Keep this ordering intact if further wiring changes occur.
37-40: No reordering needed:clhandleris already declared beforeismaninsrc/llmq/context.h(lines 70–71), so there’s no UB risk from member initialization order.Likely an incorrect or invalid review comment.
src/active/notificationinterface.cpp (3)
11-15: Constructor wiring looks correct.Storing references to ActiveContext and CActiveMasternodeManager is appropriate here.
27-27: Global instance definition is fine.Matches existing pattern (Validation/ZMQ/DS interfaces).
17-25: Remove unnecessary null-check for ehf_sighandler and confirm IBD divergence.
ehf_sighandleris astd::unique_ptrthat's always initialized inActiveContextviamake_uniqueand cannot be null.- Verify that skipping both
m_mn_activeman.UpdatedBlockTipandehf_sighandler->UpdatedBlockTipwhenfInitialDownloadis true is intentional.Likely an incorrect or invalid review comment.
src/init.cpp (9)
79-80: Include additions are appropriate.
1706-1708: Masternode manager construction LGTM.Instantiation gated on -masternodeblsprivkey is correct.
2152-2153: Verify passing active_ctx and cj_ctx before construction.PeerManager::make receives node.active_ctx and node.cj_ctx before they’re created (created in 2162–2164 and 2169–2175). If PeerManager only holds references to these std::unique_ptrs and expects them to be populated later, OK; if it copies raw pointers at construction, this could lock in nulls.
Please confirm construction/ownership semantics; otherwise, consider constructing CJContext (and ActiveContext if needed) before PeerManager::make.
2162-2164: CJContext creation order: consider moving earlier to avoid nulls in consumers.Given CDSNotificationInterface asserts m_cj_ctx in UpdatedBlockTip, ensure no callbacks can fire between its registration and this allocation. If any early tip notifications can occur, move CJContext creation before registering DS/Active interfaces.
2166-2175: ActiveContext + ActiveNotificationInterface wiring looks sound.Assertions, creation, registration, and later teardown align.
324-332: Teardown order fix: good.Unregistering and resetting ActiveNotificationInterface before dropping ActiveContext prevents dangling refs.
2285-2287: Is DKGS cleanup intended to run only in masternode mode now?CleanupOldContributions is scheduled inside the mn_activeman branch. If non-MNs never store contributions, fine; otherwise schedule it unconditionally.
2443-2443: Explicitly setting m_active_masternode is correct.
385-385: Resetting mn_activeman at the end is fine.
ActiveContext is going to host all entities are part of masternode mode (i.e. used by a node if it is an active masternode) and is required to separate interactive (i.e. networking-driven) portions of consensus from portions that can be done non-interactively/offline.
Now that CCoinJoinServer is conditionally initialized, we can remove checks that assumed its unconditional existence.
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 (2)
src/net_processing.cpp (1)
2876-2886: Guard cj_server in DSQ getdata retrievalSame null-deref risk when replying with DSQUEUE. Guard cj_server before use.
- 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;src/init.cpp (1)
2155-2165: Bug: DSNotificationInterface is constructed before CJContext exists. Reorder to avoid nullptr.
g_ds_notification_interface currently gets node.cj_ctx while it’s still null; constructor or later callbacks may dereference it.Apply this diff to construct CJContext first, then DSNotificationInterface:
@@ - g_ds_notification_interface = std::make_unique<CDSNotificationInterface>( - *node.connman, *node.mn_sync, *node.govman, *node.peerman, chainman, node.mn_activeman.get(), node.dmnman, node.llmq_ctx, node.cj_ctx - ); - RegisterValidationInterface(g_ds_notification_interface.get()); + // ********************************************************* Step 7c: Setup CoinJoin + node.cj_ctx = std::make_unique<CJContext>(chainman, *node.dmnman, *node.mn_metaman, *node.mempool, + node.mn_activeman.get(), *node.mn_sync, *node.llmq_ctx->isman, + !ignores_incoming_txs); + + // Now that CJContext exists, construct DS notification interface. + g_ds_notification_interface = std::make_unique<CDSNotificationInterface>( + *node.connman, *node.mn_sync, *node.govman, *node.peerman, chainman, node.mn_activeman.get(), node.dmnman, node.llmq_ctx, node.cj_ctx + ); + RegisterValidationInterface(g_ds_notification_interface.get()); @@ - // ********************************************************* Step 7c: Setup CoinJoin - - node.cj_ctx = std::make_unique<CJContext>(chainman, *node.dmnman, *node.mn_metaman, *node.mempool, - node.mn_activeman.get(), *node.mn_sync, *node.llmq_ctx->isman, - !ignores_incoming_txs); + // (moved CJContext construction above)
♻️ Duplicate comments (3)
src/instantsend/instantsend.h (1)
91-97: Guard Connect/Disconnect against concurrent usePublic ConnectSigner/DisconnectSigner can be called while WorkThreadMain/processors might dereference m_signer. Add a cheap runtime guard to ensure they’re only used when the worker is not running. This addresses the earlier race concern about calling DisconnectSigner during processing.
Apply this diff:
void ConnectSigner(gsl::not_null<instantsend::InstantSendSigner*> signer) { - // Prohibit double initialization + // Prohibit double initialization and ensure worker is not running + assert(!workThread.joinable()); assert(m_signer == nullptr); m_signer = signer; } - void DisconnectSigner() { m_signer = nullptr; } + void DisconnectSigner() + { + // Ensure worker is not running + assert(!workThread.joinable()); + m_signer = nullptr; + }To double-check there are no remaining call sites that could change the signer while the worker is active, run:
#!/bin/bash # Find all Connect/Disconnect invocations and surrounding context rg -nC3 --type=cpp $'\\bConnectSigner\\(|\\bDisconnectSigner\\(' src | sed -n '1,200p'src/net_processing.cpp (2)
2269-2272: Guard cj_server before dereferencing in DSQ AlreadyHave pathm_active_ctx being non-null doesn’t prove cj_server is initialized throughout startup/shutdown; add a cj_server check to avoid a null-deref.
#ifdef ENABLE_WALLET - return m_cj_ctx->queueman->HasQueue(inv.hash) || (m_active_ctx && m_active_ctx->cj_server->HasQueue(inv.hash)); + return m_cj_ctx->queueman->HasQueue(inv.hash) || + (m_active_ctx && m_active_ctx->cj_server && + m_active_ctx->cj_server->HasQueue(inv.hash)); #else - return m_active_ctx && m_active_ctx->cj_server->HasQueue(inv.hash); + return m_active_ctx && m_active_ctx->cj_server && + m_active_ctx->cj_server->HasQueue(inv.hash); #endif
5258-5260: Guard cj_server in ActiveContext message dispatchAdd an extra cj_server check to avoid dereferencing a null server during partial init/teardown.
- 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()); + }
🧹 Nitpick comments (12)
src/instantsend/instantsend.h (1)
56-56: Non-owning m_signer requires an explicit lifecycle contractNow that m_signer is a raw, non-owning pointer, please document and enforce that it is only mutated pre-Start/post-Stop to avoid races with the worker thread.
Apply this diff to add a brief contract note:
- instantsend::InstantSendSigner* m_signer{nullptr}; + // Non-owning. Mutated only before Start() or after Stop() (see asserts in Connect/Disconnect). + instantsend::InstantSendSigner* m_signer{nullptr};src/test/net_peer_connection_tests.cpp (1)
21-22: Test include may be redundantIf net_processing.h (or a forward decl) already exposes the ActiveContext pointer type, this include can likely be dropped to speed up test builds. Harmless either way.
src/masternode/active/context.cpp (1)
21-30: Add sanity asserts for LLMQ components before wiringGiven dereferences of llmq_ctx members, add defensive asserts to catch miswired contexts early.
Apply this diff:
#include <validation.h> +#include <cassert> ActiveContext::ActiveContext(ChainstateManager& chainman, CConnman& connman, CDeterministicMNManager& dmnman, CDSTXManager& dstxman, CMasternodeMetaMan& mn_metaman, CMNHFManager& mnhfman, LLMQContext& llmq_ctx, CSporkManager& sporkman, CTxMemPool& mempool, PeerManager& peerman, const CActiveMasternodeManager& mn_activeman, const CMasternodeSync& mn_sync) : m_llmq_ctx{llmq_ctx}, @@ *llmq_ctx.qman, sporkman, mempool, mn_sync)}, cj_server{std::make_unique<CCoinJoinServer>(chainman, connman, dmnman, dstxman, mn_metaman, mempool, peerman, mn_activeman, mn_sync, *llmq_ctx.isman)}, ehf_sighandler{std::make_unique<llmq::CEHFSignalsHandler>(chainman, mnhfman, *llmq_ctx.sigman, *llmq_ctx.shareman, *llmq_ctx.qman)} { + assert(m_llmq_ctx.clhandler && m_llmq_ctx.isman && m_llmq_ctx.sigman && m_llmq_ctx.shareman && m_llmq_ctx.qman); m_llmq_ctx.clhandler->ConnectSigner(cl_signer.get()); m_llmq_ctx.isman->ConnectSigner(is_signer.get()); }src/masternode/active/notificationinterface.h (2)
8-9: Missing include for std::unique_ptr in a public header.Relying on transitive includes is brittle. Include explicitly.
Apply:
#include <validationinterface.h> +#include <memory>Also applies to: 30-31
16-19: Delete move/copy assignments to prevent accidental misuse.Class holds references; disallow assignment/move to avoid dangling refs.
Apply:
ActiveNotificationInterface() = delete; ActiveNotificationInterface(const ActiveNotificationInterface&) = delete; + ActiveNotificationInterface(ActiveNotificationInterface&&) = delete; + ActiveNotificationInterface& operator=(const ActiveNotificationInterface&) = delete; + ActiveNotificationInterface& operator=(ActiveNotificationInterface&&) = delete; explicit ActiveNotificationInterface(ActiveContext& active_ctx, CActiveMasternodeManager& mn_activeman);src/chainlock/chainlock.h (2)
16-19: Header should include for assert usage.assert is used in this header; add the include to avoid relying on transitive headers.
@@ #include <chainlock/signing.h> #include <gsl/pointers.h> +#include <cassert>Also applies to: 73-79
69-71: Document lifecycle contract for (Dis)ConnectSigner.Please document (and enforce via debug assertions) that ConnectSigner is called before Start() and DisconnectSigner after Stop(), to avoid concurrent reads of m_signer.
Consider:
- Adding a comment above ConnectSigner/DisconnectSigner describing preconditions.
- Asserting the handler is not running when (dis)connecting (e.g., a boolean set in Start/Stop).
Also applies to: 73-80
src/masternode/active/context.h (3)
38-44: Signer ownership and lifetime are clear; add note on destruction order.ActiveContext holds signers and calls (Dis)connectSigner in (d/ctor). Please document that ActiveContext must outlive LLMQContext during runtime and be destroyed before LLMQContext to avoid disconnecting against a dead m_llmq_ctx.
46-53: Disable moves explicitly to prevent accidental lifetime bugs.Given the reference member and const unique_ptrs, moves are already implicitly deleted, but making it explicit helps prevent refactors from re-enabling moves later.
@@ ActiveContext() = delete; ActiveContext(const ActiveContext&) = delete; + ActiveContext(ActiveContext&&) = delete; + ActiveContext& operator=(ActiveContext&&) = delete; + ActiveContext& operator=(const ActiveContext&) = delete;
59-61: Consider dropping const on exported unique_ptrs to ease testing/mocking.const std::unique_ptr prevents reassignment in tests (e.g., injecting fakes). The pointee remains mutable either way.
- const std::unique_ptr<CCoinJoinServer> cj_server; - const std::unique_ptr<llmq::CEHFSignalsHandler> ehf_sighandler; + std::unique_ptr<CCoinJoinServer> cj_server; + std::unique_ptr<llmq::CEHFSignalsHandler> ehf_sighandler;src/test/util/setup_common.cpp (1)
62-62: Consider dropping this include unless strictly required.The file doesn’t use ActiveContext types directly; passing /active_ctx=/nullptr to PeerManager::make doesn’t need the header here. Removing it trims compile dependencies.
-#include <masternode/active/context.h>test/lint/lint-circular-dependencies.py (1)
29-38: Circular deps expanded via masternode/active/context — consider decouplingAccepting more cycles increases fragility. To reduce them, consider:
- Introducing a narrow P2P-facing interface (e.g., ICoinJoinServerP2P) consumed by net_processing, implemented by CCoinJoinServer in ActiveContext.
- Using callbacks/signals (as with ConnectSigner/DisconnectSigner) instead of direct net_processing ↔ active/context calls.
This would let you drop “... -> net_processing -> masternode/active/context -> ...” chains from EXPECTED_CIRCULAR_DEPENDENCIES over time.
Also applies to: 55-65
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
src/Makefile.am(2 hunks)src/chainlock/chainlock.cpp(1 hunks)src/chainlock/chainlock.h(2 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(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/active/context.cpp(1 hunks)src/masternode/active/context.h(1 hunks)src/masternode/active/notificationinterface.cpp(1 hunks)src/masternode/active/notificationinterface.h(1 hunks)src/masternode/node.h(2 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/rpc/coinjoin.cpp(2 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)test/lint/lint-circular-dependencies.py(1 hunks)test/util/data/non-backported.txt(1 hunks)
💤 Files with no reviewable changes (2)
- src/llmq/context.h
- src/dsnotificationinterface.cpp
🚧 Files skipped from review as they are similar to previous changes (10)
- src/rpc/coinjoin.cpp
- test/util/data/non-backported.txt
- src/node/context.h
- src/net_processing.h
- src/llmq/ehf_signals.h
- src/masternode/node.h
- src/llmq/ehf_signals.cpp
- src/chainlock/chainlock.cpp
- src/Makefile.am
- src/coinjoin/context.cpp
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/masternode/active/context.hsrc/chainlock/chainlock.hsrc/masternode/active/context.cppsrc/masternode/active/notificationinterface.cppsrc/masternode/active/notificationinterface.hsrc/coinjoin/context.hsrc/instantsend/instantsend.hsrc/node/context.cppsrc/test/util/setup_common.cppsrc/test/denialofservice_tests.cppsrc/llmq/context.cppsrc/coinjoin/server.cppsrc/init.cppsrc/instantsend/instantsend.cppsrc/test/net_peer_connection_tests.cppsrc/net_processing.cppsrc/coinjoin/server.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.hsrc/masternode/active/context.cppsrc/masternode/active/notificationinterface.cppsrc/masternode/active/notificationinterface.h
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/util/setup_common.cppsrc/test/denialofservice_tests.cppsrc/test/net_peer_connection_tests.cpp
🧠 Learnings (6)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
📚 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/masternode/active/context.hsrc/instantsend/instantsend.hsrc/llmq/context.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/chainlock/chainlock.hsrc/init.cpp
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
src/test/util/setup_common.cpp
📚 Learning: 2025-08-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/net_processing.cpp
📚 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/net_processing.cpp
🧬 Code graph analysis (10)
src/masternode/active/context.h (5)
src/coinjoin/coinjoin.h (1)
CDSTXManager(378-406)src/llmq/ehf_signals.h (2)
llmq(16-52)CEHFSignalsHandler(21-51)src/llmq/context.h (1)
llmq(23-33)src/llmq/ehf_signals.cpp (2)
CEHFSignalsHandler(18-27)CEHFSignalsHandler(29-32)src/masternode/active/context.cpp (2)
ActiveContext(16-33)ActiveContext(35-39)
src/chainlock/chainlock.h (4)
src/masternode/active/context.h (1)
chainlock(23-25)src/chainlock/chainlock.cpp (2)
CChainLocksHandler(46-57)CChainLocksHandler(59-63)src/instantsend/instantsend.h (2)
ConnectSigner(91-96)DisconnectSigner(97-97)src/chainlock/signing.cpp (2)
ChainLockSigner(18-28)ChainLockSigner(30-30)
src/masternode/active/notificationinterface.cpp (2)
src/llmq/ehf_signals.cpp (2)
UpdatedBlockTip(34-47)UpdatedBlockTip(34-34)src/dsnotificationinterface.cpp (2)
UpdatedBlockTip(72-99)UpdatedBlockTip(72-72)
src/masternode/active/notificationinterface.h (3)
src/masternode/active/notificationinterface.cpp (3)
ActiveNotificationInterface(11-15)UpdatedBlockTip(17-25)UpdatedBlockTip(17-18)src/llmq/ehf_signals.cpp (2)
UpdatedBlockTip(34-47)UpdatedBlockTip(34-34)src/masternode/active/context.cpp (2)
ActiveContext(16-33)ActiveContext(35-39)
src/coinjoin/context.h (1)
src/coinjoin/context.cpp (2)
CJContext(12-23)CJContext(25-25)
src/instantsend/instantsend.h (4)
src/instantsend/instantsend.cpp (2)
CInstantSendManager(52-65)CInstantSendManager(67-67)src/chainlock/chainlock.h (2)
ConnectSigner(73-78)DisconnectSigner(79-79)src/masternode/active/context.h (1)
instantsend(26-28)src/instantsend/signing.cpp (2)
InstantSendSigner(30-44)InstantSendSigner(46-46)
src/test/denialofservice_tests.cpp (1)
src/net_processing.cpp (2)
make(1948-1959)make(1948-1956)
src/coinjoin/server.cpp (1)
src/net_processing.cpp (6)
inv(632-632)inv(633-633)inv(634-634)inv(635-635)inv(930-930)inv(2309-2309)
src/init.cpp (2)
src/coinjoin/server.cpp (2)
DoMaintenance(871-879)DoMaintenance(871-871)src/masternode/sync.cpp (2)
DoMaintenance(367-372)DoMaintenance(367-367)
src/coinjoin/server.h (1)
src/net_processing.h (1)
PeerManager(67-67)
🔇 Additional comments (30)
src/instantsend/instantsend.h (1)
87-88: Validate all constructor callers and unit‐test flags
Ensure every instantiation of CInstantSendManager (global objects, make_unique/new calls, etc.) has been updated to the new parameter order and that the unitTests and fWipe flags continue to drive persistence behavior exactly as before.src/node/context.cpp (1)
19-19: LGTM: include adds the required dependency for ActiveContextNo concerns.
src/test/net_peer_connection_tests.cpp (1)
90-92: All PeerManager::make call sites updated with the new active_ctx parameter
Verification shows every invocation in src and tests now passes the active_ctx argument. No further changes required.src/masternode/active/context.cpp (1)
31-33: Lifecycle: connect signers before any LLMQ/InstantSend workers start
Verify that ActiveContext::ConnectSigner(...) runs prior to any LLMQContext::StartThreads or InstantSend worker‐thread creation. If your init wiring doesn’t already guarantee that order, enforce it with asserts (e.g. in instantsend.h) or document the required startup sequence.src/masternode/active/notificationinterface.cpp (2)
20-24: Confirm IBD behavior for Active MN manager updates.DSNotificationInterface forwards to m_mn_sync before returning on IBD; here we skip all forwarding in IBD. If CActiveMasternodeManager needs early tips for state, consider moving its UpdatedBlockTip call above the IBD guard (or explicitly confirm it’s not needed during IBD).
17-25: No UAF risk via ehf_sighandler: correct shutdown ordering. init.cpp unregisters and resets g_active_notification_interface (lines 324–327) before ActiveContext is destroyed, so m_active_ctx.ehf_sighandler cannot dangle.src/coinjoin/server.cpp (6)
58-74: Switch to reference-based CActiveMasternodeManager usage looks correct.Fetching dmn by collateral via m_mn_activeman.GetOutPoint() is appropriate with the new semantics.
180-186: Direct relay via PeerManager reference LGTM.Using m_peerman.RelayDSQ(dsq) aligns with the refactor away from pointer checks.
337-349: DSTX construction/sign/relay path updated correctly for references.Object-form GetOutPoint()/GetProTxHash() and dstxNew.Sign(m_mn_activeman) are consistent; RelayInv through m_peerman is fine.
452-458: Collateral relay via PeerManager is consistent with the new API.RelayTransaction(hash) usage looks good.
492-503: Finalize queue DSQ emission using references is correct.DSQ creation, signing, and relay use the new reference members consistently.
684-687: Session precondition guards are fine post-refactor.Retained semantics with clearer early returns.
Also applies to: 723-727
src/instantsend/instantsend.cpp (1)
52-55: Constructor no longer wires m_signer — verify runtime attachment path.With CSigSharesManager and is_masternode removed, signer functionality is off unless ActiveContext calls ConnectSigner. Please confirm all masternode-mode init paths construct ActiveContext and connect the signer before Start(), and tests cover both attached and detached cases.
Would you like me to generate a quick grep script to assert there are no remaining direct InstantSendSigner constructions outside ActiveContext?
Also applies to: 62-62
src/llmq/context.cpp (1)
37-40: Init order looks good; double-check signer connection occurs before Start().clhandler/isman are now constructed without signers. Ensure ActiveContext::ActiveContext() connects both signers before LLMQContext::Start() is invoked, otherwise m_signer-dependent behavior in Start() will be skipped.
I can provide a repo scan to trace Node/init wiring to confirm Create ActiveContext → ConnectSigner → LLMQContext::Start ordering.
src/coinjoin/context.h (2)
43-43: dstxman ownership exposure OK.Publishing dstxman via unique_ptr aligns with its independent lifecycle. No issues.
33-35: No stale CJContext callers; signature change is safe.
Search across .cpp and .h files shows only the constructor’s declaration and definition—no external invocations remain.src/test/util/setup_common.cpp (2)
346-348: CJContext ctor update looks correct.New signature matches the PR intent (no connman/peerman; includes isman and relay flag). No issues with the parameters themselves.
334-339: Ignore CJContext reorder; PeerManager tolerates null contexts
Greppingnet_processingshows no unconditionalactive_ctx->orcj_ctx->usages, and itsmakeimplementation stores contexts for conditional use only—passing nullptr here is intentional in tests.Likely an incorrect or invalid review comment.
src/net_processing.cpp (1)
599-603: ActiveContext wiring through PeerManager: LGTMConstructor/make signature updates and the new m_active_ctx reference are consistent with existing CJ/LLMQ context patterns.
Also applies to: 781-782, 1954-1956, 1958-1959, 1967-1971, 1978-1981
src/coinjoin/server.h (2)
38-40: References for PeerManager and CActiveMasternodeManager — document lifetime guaranteesStoring references tightens lifetime coupling. Please confirm (and document) that:
- PeerManager outlives CCoinJoinServer, and
- CCoinJoinServer’s destructor or shutdown path doesn’t call back into PeerManager.
Otherwise there’s a shutdown-order UAF risk. If that guarantee can’t be enforced, prefer non-owning pointers with explicit nullptr checks.
98-100: Constructor refactor: LGTMParameter reorder and member init match the new reference members.
Also applies to: 106-107
src/init.cpp (9)
94-95: Headers for ActiveContext wiring are correctly added.
324-332: Correct teardown order for ActiveNotificationInterface and ActiveContext.
Unregistering and resetting the interface before destroying ActiveContext prevents dangling callbacks. Looks good.
385-385: Resetting mn_activeman during shutdown is in the right place.
Happens after validation interfaces are unregistered; safe.
2166-2176: Masternode-only ActiveContext wiring and validation interface registration look correct.
Assertions prevent double-init; shutdown unregisters before destruction.
2177-2178: Comment-only change.
2286-2287: Scheduling CJ server maintenance via ActiveContext is properly gated on mn_activeman.
Lifecycle aligns with scheduler stop and shutdown.
2433-2433: Plumbing m_active_masternode into CConnman::Options is good.
Enables net stack to adapt behavior without globals.
1706-1708: No use of dmnman in CActiveMasternodeManager ctor; safe to construct early. The constructor merely stores thedmnmanpointer and asserts onskvalidity—there’s no dereference ofdmnmaninsrc/masternode/node.cpp:52–57.
2152-2153: Null-safety confirmed for active_ctx:PeerManagerImplguards everym_active_ctxdereference with anif (m_active_ctx)check, so no changes needed.
| std::unique_ptr<std::thread> scheduler_thread; | ||
|
|
||
| std::unique_ptr<chainlock::ChainLockSigner> m_signer{nullptr}; | ||
| chainlock::ChainLockSigner* m_signer{nullptr}; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make signer pointer updates thread-safe (guard with cs).
m_signer is mutated at runtime via Connect/Disconnect while CChainLocksHandler runs a scheduler thread. Without synchronization, this is a data race. Guard the pointer with cs and lock in ConnectSigner/DisconnectSigner.
Apply this minimal diff:
@@
- chainlock::ChainLockSigner* m_signer{nullptr};
+ chainlock::ChainLockSigner* m_signer GUARDED_BY(cs){nullptr};
@@
- void ConnectSigner(gsl::not_null<chainlock::ChainLockSigner*> signer)
+ void ConnectSigner(gsl::not_null<chainlock::ChainLockSigner*> signer)
{
// Prohibit double initialization
assert(m_signer == nullptr);
- m_signer = signer;
+ LOCK(cs);
+ m_signer = signer;
}
- void DisconnectSigner() { m_signer = nullptr; }
+ void DisconnectSigner()
+ {
+ LOCK(cs);
+ m_signer = nullptr;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| chainlock::ChainLockSigner* m_signer{nullptr}; | |
| // File: src/chainlock/chainlock.h | |
| // — inside CChainLocksHandler (around line 49), assuming `CCriticalSection cs;` is already declared above | |
| private: | |
| // Mark m_signer as guarded by cs for static analysis | |
| chainlock::ChainLockSigner* m_signer GUARDED_BY(cs){nullptr}; | |
| public: | |
| void ConnectSigner(gsl::not_null<chainlock::ChainLockSigner*> signer) | |
| { | |
| // Prohibit double initialization | |
| assert(m_signer == nullptr); | |
| // Synchronize assignment | |
| LOCK(cs); | |
| m_signer = signer; | |
| } | |
| void DisconnectSigner() | |
| { | |
| // Synchronize reset | |
| LOCK(cs); | |
| m_signer = nullptr; | |
| } |
There was a problem hiding this comment.
bot, please confirm we could just use an atomic here right? as we are just guarding for the writing to the pointer
There was a problem hiding this comment.
@PastaPastaPasta Yes, you're absolutely correct! Using std::atomic<chainlock::ChainLockSigner*> would be the better approach here since we're only guarding pointer read/write operations.
The access pattern is:
- Writes: ConnectSigner/DisconnectSigner (infrequent)
- Reads:
if (m_signer)checks followed by method calls (frequent)
std::atomic<T*> provides sequential consistency for these operations without the overhead of mutex locking. The usage would be:
std::atomic<chainlock::ChainLockSigner*> m_signer{nullptr};
void ConnectSigner(gsl::not_null<chainlock::ChainLockSigner*> signer) {
assert(m_signer.load() == nullptr);
m_signer.store(signer);
}
void DisconnectSigner() {
m_signer.store(nullptr);
}And existing if (m_signer) checks would become if (m_signer.load()) or just if (m_signer) since atomic pointers have implicit conversion.
This is much more efficient than mutex synchronization for this simple pointer swap pattern, especially given that reads are likely far more frequent than writes.
🧠 Learnings used
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.
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
| auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, /*banman=*/nullptr, | ||
| *m_node.chainman, *m_node.mempool, *m_node.mn_metaman, *m_node.mn_sync, | ||
| *m_node.govman, *m_node.sporkman, /* mn_activeman = */ nullptr, m_node.dmnman, | ||
| m_node.cj_ctx, m_node.llmq_ctx, /* ignore_incoming_txs = */ false); | ||
| *m_node.govman, *m_node.sporkman, /*mn_activeman=*/ nullptr, m_node.dmnman, | ||
| /*active_ctx=*/nullptr, m_node.cj_ctx, m_node.llmq_ctx, /*ignore_incoming_txs=*/false); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Passing nullptr to a const std::unique_ptr& creates a temporary that will dangle.
PeerManager::make takes const std::unique_ptr&. Passing nullptr binds a temporary unique_ptr that is destroyed at the end of the call; if PeerManagerImpl stores the reference (pattern used elsewhere), it will dangle → UB.
Apply (pass the persistent repository-held unique_ptr instead of a temporary):
- auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, /*banman=*/nullptr,
+ auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, /*banman=*/nullptr,
*m_node.chainman, *m_node.mempool, *m_node.mn_metaman, *m_node.mn_sync,
- *m_node.govman, *m_node.sporkman, /*mn_activeman=*/ nullptr, m_node.dmnman,
- /*active_ctx=*/nullptr, m_node.cj_ctx, m_node.llmq_ctx, /*ignore_incoming_txs=*/false);
+ *m_node.govman, *m_node.sporkman, /*mn_activeman=*/ nullptr, m_node.dmnman,
+ /*active_ctx=*/m_node.active_ctx, m_node.cj_ctx, m_node.llmq_ctx, /*ignore_incoming_txs=*/false);- auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, /*banman=*/nullptr,
+ auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, /*banman=*/nullptr,
*m_node.chainman, *m_node.mempool, *m_node.mn_metaman, *m_node.mn_sync,
- *m_node.govman, *m_node.sporkman, /*mn_activeman=*/ nullptr, m_node.dmnman,
- /*active_ctx=*/nullptr, m_node.cj_ctx, m_node.llmq_ctx, /*ignore_incoming_txs=*/false);
+ *m_node.govman, *m_node.sporkman, /*mn_activeman=*/ nullptr, m_node.dmnman,
+ /*active_ctx=*/m_node.active_ctx, m_node.cj_ctx, m_node.llmq_ctx, /*ignore_incoming_txs=*/false);- auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, banman.get(),
+ auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, banman.get(),
*m_node.chainman, *m_node.mempool, *m_node.mn_metaman, *m_node.mn_sync,
- *m_node.govman, *m_node.sporkman, /*mn_activeman=*/nullptr, m_node.dmnman,
- /*active_ctx=*/nullptr, m_node.cj_ctx, m_node.llmq_ctx, /*ignore_incoming_txs=*/false);
+ *m_node.govman, *m_node.sporkman, /*mn_activeman=*/nullptr, m_node.dmnman,
+ /*active_ctx=*/m_node.active_ctx, m_node.cj_ctx, m_node.llmq_ctx, /*ignore_incoming_txs=*/false);- auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, banman.get(),
+ auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, banman.get(),
*m_node.chainman, *m_node.mempool, *m_node.mn_metaman, *m_node.mn_sync,
- *m_node.govman, *m_node.sporkman, /*mn_activeman=*/nullptr, m_node.dmnman,
- /*active_ctx=*/nullptr, m_node.cj_ctx, m_node.llmq_ctx, /*ignore_incoming_txs=*/false);
+ *m_node.govman, *m_node.sporkman, /*mn_activeman=*/nullptr, m_node.dmnman,
+ /*active_ctx=*/m_node.active_ctx, m_node.cj_ctx, m_node.llmq_ctx, /*ignore_incoming_txs=*/false);Confirm how PeerManagerImpl stores the reference:
Also applies to: 255-259, 323-326, 428-431
🏁 Script executed:
#!/bin/bash
# Expect PeerManagerImpl to have members like `const std::unique_ptr<ActiveContext>& m_active_ctx;`
rg -n -C2 'PeerManagerImpl|class PeerManagerImpl|m_active_ctx|ActiveContext' src/net_processing.cpp src/node/*.cpp src/*.hLength of output: 29972
Replace /*active_ctx=*/nullptr with m_node.active_ctx to avoid a dangling reference
PeerManagerImpl holds const std::unique_ptr<ActiveContext>& m_active_ctx; passing nullptr creates a temporary unique_ptr that is destroyed after the call, leaving m_active_ctx dangling. In src/test/denialofservice_tests.cpp (at lines 151–155, 255–259, 323–326, 428–431), update each PeerManager::make invocation to use /*active_ctx=*/m_node.active_ctx instead of nullptr.
🤖 Prompt for AI Agents
In src/test/denialofservice_tests.cpp around lines 151–155 (and also update the
similar calls at 255–259, 323–326, 428–431), the PeerManager::make invocations
pass /*active_ctx=*/nullptr which creates a temporary unique_ptr and leaves
PeerManagerImpl's const std::unique_ptr<ActiveContext>& m_active_ctx dangling;
replace each /*active_ctx=*/nullptr argument with
/*active_ctx=*/m_node.active_ctx so the persistent unique_ptr from m_node is
forwarded and the reference remains valid for the lifetime of PeerManagerImpl.
|
@coderabbitai generate docstrings |
✅ Actions performedDocstrings generation started. |
|
Caution An unexpected error occurred while opening a pull request: Repository rule violations found Required status check "check_merge" is expected. |
…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
Additional Information
To allow removing
::fMasternodeMode, we needed to route all pending global usage and one usage, inwallet/coinjoin.cpp(source) required additional refactoring. When calculating a tally of all outpoints from an address, if the node is in masternode mode, we skip over outputs with collateral values.SelectCoinsGroupedByAddressesis used in three places, creating collaterals (source) and denoms (source) in the CoinJoin client, calculating the anonymizable balance (source) in wallet, and tests.As the wallet will have no way of querying if masternode mode is enabled, we need to pull this logic outside. It isn't required in the CoinJoin client as masternodes cannot participate in CoinJoin as a client (source) and so
fMasternodeModewill not evaluatetrue, leaving behindGetAnonymizableBalance(), which now acceptsfSkipMnCollateralas the caller can query if the node is in masternode mode (source).This requires storing the outpoint's value in order to filter it inThis has been reverted as masternodes can no longer hold collaterals in their local wallet, so we can remove the collateral check outright (comment).GetAnonymizableBalance(), which required a change inCompactTallyItem's structure (source).The signers spun-off in dash#6742 and dash#6761 have been moved to a dedicated masternode mode-only context,
ActiveContext, as part of an effort to pull out networking-heavy and interactive consensus elements out ofLLMQContext, forbitcoin-chainstate.As the signers plug into existing managers and aren't standalone, they are not public.
Alongside the signers, managers that are masternode mode-only have been moved to
ActiveContext, namelyCCoinJoinServer(source) andllmq::CEHFSignalsHandler(source).Breaking Changes
None expected.
Checklist