Skip to content

refactor: move CDSNotificationInterface into NodeContext #7546

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PastaPastaPasta merged 2 commits into from
Aug 6, 2026
Merged

refactor: move CDSNotificationInterface into NodeContext #7546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2dcef69
refactor: pass CDeterministicMNManager by reference to CDSNotificatio…
PastaPastaPasta Aug 5, 2026
d1b4f75
refactor: move CDSNotificationInterface into NodeContext
PastaPastaPasta Aug 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions src/dsnotificationinterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,12 @@
#include <governance/governance.h>
#include <instantsend/instantsend.h>
#include <masternode/sync.h>
#include <util/check.h>

@knst knst Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: unrelated change?

@PastaPastaPasta PastaPastaPasta Aug 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think not; I think it's related to the remove of Assert down below now that it's not a pointer

#include <validation.h>


CDSNotificationInterface::CDSNotificationInterface(CConnman& connman, CDSTXManager& dstxman, CMasternodeSync& mn_sync,
CGovernanceManager& govman, const ChainstateManager& chainman,
const std::unique_ptr<CDeterministicMNManager>& dmnman) :
CDeterministicMNManager& dmnman) :

@PastaPastaPasta PastaPastaPasta Aug 5, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand since when / how this is safe; so I asked:

You're right that it was intentional and necessary when written. The paper trail:

  1. a35245653c15 (refactor: pass references to objects instead of using global definitions #4988, Sept 2022, kittywhiskers) — "refactor: pass references to objects instead of using global definitions" is where the const std::unique_ptr<CDeterministicMNManager>& parameter was born. At that commit, the interface was created at init.cpp:1943 — before "Step 7b: load block chain" — and the chainstate-load retry loop at init.cpp:2039-2040 did deterministicMNManager.reset(); deterministicMNManager.reset(new CDeterministicMNManager(...)) on every retry iteration (the "try again with reindex" fallback). A plain reference bound at construction would dangle after the first reset, and the object might not even exist yet in some orderings — so passing the always-alive global slot by reference, with null tolerance (Assert), was the only correct option. The same pattern was used for the LLMQ managers in that commit for the same reason.
  2. eca0a64ea10 (backport of refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager bitcoin/bitcoin#22564, Aug 2024, kwvg) — "Move mutable globals cleared in ::UnloadBlockIndex to BlockManager" is the commit that made the indirection vestigial. As part of that reordering, pdsNotificationInterface creation moved from before Step 7a to after the chainstate load loop (new location right after node.peerman creation, init.cpp:2115 vs. the loop ending before :2109). From that commit onward, node.dmnman has reached its final value before the interface constructor runs, and it isn't reset again until shutdown — where the interface is destroyed first. The unique_ptr& survived from 2024 to now purely by inertia, which is what the // todo: replace unique_ptr for dmnman to reference at the creation site was tracking. (dacf8592189 is a minor waypoint — it renamed the raw pointer to the g_ds_notification_interface unique_ptr global.)

So the one-line justification for the PR: #4988 introduced the slot-reference because init then constructed the interface before the chainstate-load retry loop that destroys and recreates deterministicMNManager; the bitcoin#22564 backport (eca0a64ea10) moved construction after that loop, so the referent is now stable for the interface's entire lifetime.

@kwvg kwvg Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PastaPastaPasta this checks out, dmnman is initialised at the beginning of Step 7(b) through LoadChainstate > DashChainstateSetup > make_unique and the notification interface is setup at the tail of that step (source).

Change should be safe.

@knst knst Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strange, I miss this unique_ptr during #7314 - had seen it but haven't noticed as it's ready. now it should be safe indeed ; long time before 7314.

m_connman{connman},
m_dstxman{dstxman},
m_mn_sync{mn_sync},
Expand Down Expand Up @@ -50,7 +49,7 @@ void CDSNotificationInterface::SynchronousUpdatedBlockTip(const CBlockIndex *pin
if (pindexNew == pindexFork) // blocks were disconnected without any new ones
return;

Assert(m_dmnman)->UpdatedBlockTip(pindexNew);
m_dmnman.UpdatedBlockTip(pindexNew);
}

void CDSNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload)
Expand Down Expand Up @@ -103,5 +102,3 @@ void CDSNotificationInterface::NotifyChainLock(const CBlockIndex* pindex,
m_dstxman.NotifyChainLock(pindex);
}
}

std::unique_ptr<CDSNotificationInterface> g_ds_notification_interface;
6 changes: 2 additions & 4 deletions src/dsnotificationinterface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class CDSNotificationInterface : public CValidationInterface
CDSNotificationInterface& operator=(const CDSNotificationInterface&) = delete;
explicit CDSNotificationInterface(CConnman& connman, CDSTXManager& dstxman, CMasternodeSync& mn_sync,
CGovernanceManager& govman, const ChainstateManager& chainman,
const std::unique_ptr<CDeterministicMNManager>& dmnman);
CDeterministicMNManager& dmnman);
virtual ~CDSNotificationInterface();

// CValidationInterface
Expand All @@ -45,9 +45,7 @@ class CDSNotificationInterface : public CValidationInterface
CMasternodeSync& m_mn_sync;
CGovernanceManager& m_govman;
const ChainstateManager& m_chainman;
const std::unique_ptr<CDeterministicMNManager>& m_dmnman;
CDeterministicMNManager& m_dmnman;
};

extern std::unique_ptr<CDSNotificationInterface> g_ds_notification_interface;

#endif // BITCOIN_DSNOTIFICATIONINTERFACE_H
14 changes: 7 additions & 7 deletions src/init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,9 +376,9 @@ void PrepareShutdown(NodeContext& node)
UnregisterValidationInterface(node.cj_walletman.get());
}

if (g_ds_notification_interface) {
UnregisterValidationInterface(g_ds_notification_interface.get());
g_ds_notification_interface.reset();
if (node.ds_notification_interface) {
UnregisterValidationInterface(node.ds_notification_interface.get());
node.ds_notification_interface.reset();
}

// After all scheduled tasks have been flushed, destroy pointers
Expand Down Expand Up @@ -2101,10 +2101,10 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
node.dmnman, node.cj_walletman, node.llmq_ctx, ignores_incoming_txs);
RegisterValidationInterface(node.peerman.get());

g_ds_notification_interface = std::make_unique<CDSNotificationInterface>(
*node.connman, *node.dstxman, *node.mn_sync, *node.govman, chainman, node.dmnman // todo: replace unique_ptr for dmnman to reference
node.ds_notification_interface = std::make_unique<CDSNotificationInterface>(
*node.connman, *node.dstxman, *node.mn_sync, *node.govman, chainman, *node.dmnman
);
RegisterValidationInterface(g_ds_notification_interface.get());
RegisterValidationInterface(node.ds_notification_interface.get());

// ********************************************************* Step 7d: Setup other Dash services

Expand Down Expand Up @@ -2375,7 +2375,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
// for quorum connection setup and skShare derivation.
// Only kick CDSNotificationInterface here (cached block
// height for DS/MN payments/budgets).
g_ds_notification_interface->InitializeCurrentBlockTip(tip, ibd);
node.ds_notification_interface->InitializeCurrentBlockTip(tip, ibd);
} else {
// Non-masternode nodes (including observer-only): broadcast
// to all subscribers now; no proTxHash dependency.
Expand Down
1 change: 1 addition & 0 deletions src/node/context.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <chainlock/handler.h>
#include <coinjoin/coinjoin.h>
#include <coinjoin/walletman.h>
#include <dsnotificationinterface.h>
#include <evo/chainhelper.h>
#include <evo/creditpool.h>
#include <evo/deterministicmns.h>
Expand Down
2 changes: 2 additions & 0 deletions src/node/context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class AddrMan;
class CBlockPolicyEstimator;
class CConnman;
class CDeterministicMNManager;
class CDSNotificationInterface;
class CDSTXManager;
class CChainstateHelper;
class ChainstateManager;
Expand Down Expand Up @@ -106,6 +107,7 @@ struct NodeContext {
std::unique_ptr<chainlock::Chainlocks> chainlocks;
std::unique_ptr<chainlock::ChainlockHandler> clhandler;
//! Dash contexts
std::unique_ptr<CDSNotificationInterface> ds_notification_interface;

@kwvg kwvg Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this change, upstream opted to keep their notification interface out of NodeContext (source)

@PastaPastaPasta PastaPastaPasta Aug 5, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kwvg Right that upstream keeps CZMQNotificationInterface global — and so do we; this PR doesn't touch g_zmq_notification_interface, deliberately, since that's a shared-with-upstream file where divergence has a real backport cost.

But I'd argue zmq is upstream's outlier rather than its policy. In v31.1, NodeContext owns notifications (KernelNotifications, "Issues blocking calls about sync status, errors and warnings"), validation_signals (the entire ValidationSignals dispatch mechanism, "Issues calls about blocks and transactions"), and peerman — which is itself a CValidationInterface subscriber and structurally the closest upstream analog to what CDSNotificationInterface is: an object that receives validation events and forwards them into node-owned subsystems.

The reason zmq works fine as a global is that it's self-contained: built from args alone (CZMQNotificationInterface::Create), references nothing in NodeContext, purely optional. CDSNotificationInterface is the opposite — it holds references to six NodeContext members (connman, dstxman, mn_sync, govman, chainman, dmnman), so its valid lifetime is exactly bracketed by NodeContext member lifetimes on both ends. As a global, that bracket is encoded implicitly in init.cpp/shutdown line ordering; as a member declared after everything it references, the destruction-order guarantee is in the type. That's also consistent with where the tree already is: active_ctx, observer_ctx, cj_walletman, and clhandler are all NodeContext-owned validation-interface subscribers — the DS interface was the last one out. And since dsnotificationinterface.{h,cpp} is Dash-only, there's no upstream file this diverges from.

🤖 Posted autonomously by Claude on behalf of pasta.

@knst knst Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: technically, it's not context, I think.

Should be

+std::unique_ptr<CDSNotificationInterface> ds_notification_interface;
 //! Dash contexts
-std::unique_ptr<CDSNotificationInterface> ds_notification_interface;

std::unique_ptr<ActiveContext> active_ctx;
std::unique_ptr<LLMQContext> llmq_ctx;
std::unique_ptr<llmq::ObserverContext> observer_ctx;
Expand Down