refactor: move CDSNotificationInterface into NodeContext - #7546
Conversation
|
✅ Final review complete — no blockers (commit d1b4f75) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Walkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Opus
Clean two-commit refactor that first changes CDSNotificationInterface to take CDeterministicMNManager by reference (removing the now-redundant Assert), then moves the g_ds_notification_interface global into NodeContext as ds_notification_interface. Verified all call sites updated, no stale references to the old global remain anywhere in the tree, and the new member is declared after connman/dstxman/dmnman/govman/mn_sync in NodeContext so default reverse-order destruction keeps it valid; PrepareShutdown also explicitly unregisters and resets it before those managers are torn down. Both automated review agents found zero issues and my independent verification agrees — this is a safe, well-scoped refactor with no consensus impact.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Opus reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (completed)
| CDSNotificationInterface::CDSNotificationInterface(CConnman& connman, CDSTXManager& dstxman, CMasternodeSync& mn_sync, | ||
| CGovernanceManager& govman, const ChainstateManager& chainman, | ||
| const std::unique_ptr<CDeterministicMNManager>& dmnman) : | ||
| CDeterministicMNManager& dmnman) : |
There was a problem hiding this comment.
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:
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 theconst std::unique_ptr<CDeterministicMNManager>¶meter was born. At that commit, the interface was created atinit.cpp:1943— before "Step 7b: load block chain" — and the chainstate-load retry loop atinit.cpp:2039-2040diddeterministicMNManager.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.eca0a64ea10(backport of refactor: Move mutable globals cleared in::UnloadBlockIndextoBlockManagerbitcoin/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,pdsNotificationInterfacecreation moved from before Step 7a to after the chainstate load loop (new location right afternode.peermancreation,init.cpp:2115vs. the loop ending before:2109). From that commit onward,node.dmnmanhas reached its final value before the interface constructor runs, and it isn't reset again until shutdown — where the interface is destroyed first. Theunique_ptr&survived from 2024 to now purely by inertia, which is what the// todo: replace unique_ptr for dmnman to referenceat the creation site was tracking. (dacf8592189is a minor waypoint — it renamed the raw pointer to theg_ds_notification_interfaceunique_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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
| std::unique_ptr<chainlock::Chainlocks> chainlocks; | ||
| std::unique_ptr<chainlock::ChainlockHandler> clhandler; | ||
| //! Dash contexts | ||
| std::unique_ptr<CDSNotificationInterface> ds_notification_interface; |
There was a problem hiding this comment.
Not sure about this change, upstream opted to keep their notification interface out of NodeContext (source)
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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;
| CDSNotificationInterface::CDSNotificationInterface(CConnman& connman, CDSTXManager& dstxman, CMasternodeSync& mn_sync, | ||
| CGovernanceManager& govman, const ChainstateManager& chainman, | ||
| const std::unique_ptr<CDeterministicMNManager>& dmnman) : | ||
| CDeterministicMNManager& dmnman) : |
There was a problem hiding this comment.
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.
| #include <governance/governance.h> | ||
| #include <instantsend/instantsend.h> | ||
| #include <masternode/sync.h> | ||
| #include <util/check.h> |
There was a problem hiding this comment.
I think not; I think it's related to the remove of Assert down below now that it's not a pointer
| std::unique_ptr<chainlock::Chainlocks> chainlocks; | ||
| std::unique_ptr<chainlock::ChainlockHandler> clhandler; | ||
| //! Dash contexts | ||
| std::unique_ptr<CDSNotificationInterface> ds_notification_interface; |
There was a problem hiding this comment.
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;
4 participants
Issue being fixed or feature implemented
Moves
CDSNotificationInterfacefrom a raw global (g_ds_notification_interface) intoNodeContext(node.ds_notification_interface).The global
g_ds_notification_interfacewas previously constructed ininit.cppand held raw pointer references to various node services. Moving it intoNodeContextaligns its lifecycle with other validation interface subscribers (such ascj_walletmanandactive_ctx).What was done?
Pass
CDeterministicMNManagerby reference toCDSNotificationInterface:const std::unique_ptr<CDeterministicMNManager>&toCDeterministicMNManager&.#include <util/check.h>header insrc/dsnotificationinterface.cppafter replacingAssert(m_dmnman)withm_dmnman.UpdatedBlockTip(...).// todo:note insrc/init.cpp.Move
CDSNotificationInterfaceintoNodeContext:ds_notification_interface(std::unique_ptr<CDSNotificationInterface>) toNodeContextinsrc/node/context.h. Placed it after all referenced managers to ensure correct C++ reverse-destruction order.<dsnotificationinterface.h>insrc/node/context.cpp.g_ds_notification_interfaceextern declaration and definition.src/init.cppto usenode.ds_notification_interface.How Has This Been Tested?
./src/test/test_dash --run_test=getarg_tests)feature_dip3_deterministicmns.pyandfeature_governance.pytest/lint/all-lint.pyBreaking Changes
None.
Checklist:
This pull request was created by Codex.