refactor: drop dependency of CGovernanceManager on CNetFulfilledRequestManager #7105
Conversation
|
WalkthroughThis pull request refactors the dependency injection pattern for governance-related components. The Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/governance/net_governance.h (1)
12-35: Clang-format CI is failing, and the new reference dependency makes lifetime correctness critical.
The injection ofCNetFulfilledRequestManager&intoNetGovernanceis the right direction for separation, but ensure (1) clang-format is applied and (2) every construction site guaranteesnetfulfilledmanoutlivesNetGovernance(esp. in tests/tools).Proposed formatting-only tweak (likely what clang-format wants)
- NetGovernance(PeerManagerInternal* peer_manager, CGovernanceManager& gov_manager, CMasternodeSync& node_sync, CNetFulfilledRequestManager& netfulfilledman, - CConnman& connman) : + NetGovernance(PeerManagerInternal* peer_manager, + CGovernanceManager& gov_manager, + CMasternodeSync& node_sync, + CNetFulfilledRequestManager& netfulfilledman, + CConnman& connman) : NetHandler(peer_manager), m_gov_manager(gov_manager), m_node_sync(node_sync), m_netfulfilledman(netfulfilledman), m_connman(connman) { }src/governance/governance.cpp (1)
65-77: Clang-format CI is failing here, and the ctor signature formatting is the likely culprit.
The signature change is correct for dropping the dependency, but please clang-format the file to fix CI. Also, since the anti-spam guard moved out of governance, double-check no other call paths invokeCGovernanceManager::SyncObjectswithout going through the new NetGovernance gating.Proposed formatting-only tweak (likely what clang-format wants)
CGovernanceManager::CGovernanceManager(CMasternodeMetaMan& mn_metaman, const ChainstateManager& chainman, - const std::unique_ptr<CDeterministicMNManager>& dmnman, CMasternodeSync& mn_sync) : + const std::unique_ptr<CDeterministicMNManager>& dmnman, + CMasternodeSync& mn_sync) : m_db{std::make_unique<db_type>("governance.dat", "magicGovernanceCache")}, m_mn_metaman{mn_metaman}, m_chainman{chainman}, m_dmnman{dmnman}, m_mn_sync{mn_sync}, cmapVoteToObject{MAX_CACHE_SIZE}, mapPostponedObjects{}, mapTrigger{} { }
🤖 Fix all issues with AI agents
In `@src/governance/governance.h`:
- Around line 275-278: The constructor declaration for CGovernanceManager (and
its destructor) is misformatted relative to clang-format and must match the
definition in src/governance/governance.cpp; run clang-format on
src/governance/governance.h (and/or apply the same line breaks/spacing used in
the cpp definition) so the signature for explicit
CGovernanceManager(CMasternodeMetaMan& mn_metaman, const ChainstateManager&
chainman, const std::unique_ptr<CDeterministicMNManager>& dmnman,
CMasternodeSync& mn_sync); and ~CGovernanceManager(); exactly mirror the
formatting in the implementation, fixing spacing/line breaks to satisfy CI.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/governance/governance.cppsrc/governance/governance.hsrc/governance/net_governance.cppsrc/governance/net_governance.hsrc/init.cppsrc/test/util/setup_common.cpp
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/governance/governance.hsrc/governance/net_governance.hsrc/governance/governance.cppsrc/governance/net_governance.cppsrc/test/util/setup_common.cppsrc/init.cpp
src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Files:
src/governance/governance.hsrc/governance/net_governance.hsrc/governance/governance.cppsrc/governance/net_governance.cpp
src/governance/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
Files:
src/governance/governance.hsrc/governance/net_governance.hsrc/governance/governance.cppsrc/governance/net_governance.cpp
src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
Files:
src/governance/governance.hsrc/governance/net_governance.hsrc/governance/governance.cppsrc/governance/net_governance.cpp
src/{test,wallet/test}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Files:
src/test/util/setup_common.cpp
🧠 Learnings (10)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 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
Repo: dashpay/dash PR: 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-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/governance/**/*.{cpp,h} : Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
Applied to files:
src/governance/governance.hsrc/governance/governance.cppsrc/governance/net_governance.cppsrc/init.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/evodb/**/*.{cpp,h} : Evolution Database (CEvoDb) must handle masternode snapshots, quorum state, governance objects with efficient differential updates for masternode lists
Applied to files:
src/governance/governance.hsrc/governance/governance.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Applied to files:
src/governance/governance.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features
Applied to files:
src/governance/governance.hsrc/governance/governance.cpp
📚 Learning: 2025-12-22T15:42:48.595Z
Learnt from: kwvg
Repo: dashpay/dash PR: 7068
File: src/qt/guiutil_font.h:68-77
Timestamp: 2025-12-22T15:42:48.595Z
Learning: In C++/Qt codebases, use fail-fast asserts in setters to enforce invariants (e.g., ensuring internal maps contain necessary keys). Prefer assert() for programmer errors that should be caught in development and debugging, rather than defensive runtime checks with fallbacks. This helps catch invariant violations early during development. Apply to header and source files where invariant-driven state mutations occur, especially in setters like SetWeightBold/SetWeightNormal that assume established relationships or preconditions.
Applied to files:
src/governance/governance.hsrc/governance/net_governance.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,consensus,net_processing}/**/*.{cpp,h} : ValidationInterface callbacks must be used for event-driven architecture to coordinate subsystems during block/transaction processing
Applied to files:
src/governance/governance.cpp
📚 Learning: 2025-11-25T10:53:37.523Z
Learnt from: knst
Repo: dashpay/dash PR: 7008
File: src/masternode/sync.h:17-18
Timestamp: 2025-11-25T10:53:37.523Z
Learning: The file src/masternode/sync.h containing `CMasternodeSync` is misnamed and misplaced—it has nothing to do with "masternode" functionality. It should eventually be renamed to `NodeSyncing` or `NodeSyncStatus` and moved to src/node/sync.h as a future refactoring.
Applied to files:
src/governance/governance.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
Applied to files:
src/governance/governance.cpp
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash PR: 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
🧬 Code graph analysis (2)
src/governance/governance.h (1)
src/governance/governance.cpp (2)
CGovernanceManager(65-77)CGovernanceManager(79-83)
src/governance/net_governance.h (1)
src/net_processing.h (3)
NetHandler(76-99)NetHandler(80-83)PeerManagerInternal(61-74)
🪛 GitHub Actions: Clang Diff Format Check
src/governance/governance.h
[error] 1-1: Clang-format differences detected in governance.h. Run 'clang-format -i' to fix formatting.
src/governance/net_governance.h
[error] 1-1: Clang-format differences detected in net_governance.h. Run 'clang-format -i' to fix formatting.
src/governance/governance.cpp
[error] 1-1: Clang-format differences detected in governance.cpp. Run 'clang-format -i' to fix formatting.
🔇 Additional comments (4)
src/init.cpp (1)
1984-1987: Wiring refactoring is complete and correct.CGovernanceManagerno longer depends onnetfulfilledman(lines 1986-1987), whileNetGovernancereceives it explicitly (line 2262). There is only oneNetGovernanceconstructor call site in the codebase and it matches the refactored signature.SyncObjectscalls are properly guarded withHasFulfilledRequestchecks.src/test/util/setup_common.cpp (1)
289-290: Updated test setup wiring looks consistent with the newCGovernanceManagerconstructor.Verification confirms both constructor call sites (src/init.cpp and the test setup) use the same updated 4-parameter signature, with no remaining calls using the removed parameter.
src/governance/net_governance.cpp (2)
13-13: LGTM!Include added for the new
CNetFulfilledRequestManagerdependency.
64-73: LGTM - fulfillment tracking correctly relocated.The logic properly prevents peers from repeatedly requesting full governance object sync. The misbehavior penalty of 20 is consistent with other penalties in this file.
The
assert(m_netfulfilledman.IsValid())is appropriate here:m_netfulfilledmanis a reference initialized at construction and itsis_validflag is set during startup viaLoadCache()in init.cpp before the node processes any messages. The assert validates this invariant—if ProcessMessage somehow runs before initialization completes, that's a programming error that should fail loudly in debug builds.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| explicit CGovernanceManager(CMasternodeMetaMan& mn_metaman, | ||
| const ChainstateManager& chainman, | ||
| const std::unique_ptr<CDeterministicMNManager>& dmnman, CMasternodeSync& mn_sync); | ||
| ~CGovernanceManager(); |
There was a problem hiding this comment.
Clang-format CI is failing for this constructor declaration (and it’s the only changed hunk here).
The signature change aligns with the refactor goal; please run clang-format (CI currently fails) and ensure the declaration matches the definition in src/governance/governance.cpp.
Proposed formatting-only tweak (likely what clang-format wants)
- explicit CGovernanceManager(CMasternodeMetaMan& mn_metaman,
- const ChainstateManager& chainman,
- const std::unique_ptr<CDeterministicMNManager>& dmnman, CMasternodeSync& mn_sync);
+ explicit CGovernanceManager(CMasternodeMetaMan& mn_metaman,
+ const ChainstateManager& chainman,
+ const std::unique_ptr<CDeterministicMNManager>& dmnman,
+ CMasternodeSync& mn_sync);📝 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.
| explicit CGovernanceManager(CMasternodeMetaMan& mn_metaman, | |
| const ChainstateManager& chainman, | |
| const std::unique_ptr<CDeterministicMNManager>& dmnman, CMasternodeSync& mn_sync); | |
| ~CGovernanceManager(); | |
| explicit CGovernanceManager(CMasternodeMetaMan& mn_metaman, | |
| const ChainstateManager& chainman, | |
| const std::unique_ptr<CDeterministicMNManager>& dmnman, | |
| CMasternodeSync& mn_sync); | |
| ~CGovernanceManager(); |
🤖 Prompt for AI Agents
In `@src/governance/governance.h` around lines 275 - 278, The constructor
declaration for CGovernanceManager (and its destructor) is misformatted relative
to clang-format and must match the definition in src/governance/governance.cpp;
run clang-format on src/governance/governance.h (and/or apply the same line
breaks/spacing used in the cpp definition) so the signature for explicit
CGovernanceManager(CMasternodeMetaMan& mn_metaman, const ChainstateManager&
chainman, const std::unique_ptr<CDeterministicMNManager>& dmnman,
CMasternodeSync& mn_sync); and ~CGovernanceManager(); exactly mirror the
formatting in the implementation, fixing spacing/line breaks to satisfy CI.
4 participants
Issue being fixed or feature implemented
CGovernanceManagershould not be directly aware of network stuff.What was done?
Moved usages of
CNetFulfilledRequestManagerfrom CGovernanceManager to NetGovernance where it belongs to, to follow-up #7008How Has This Been Tested?
Run unit & functional tests.
Breaking Changes
N/A
Checklist: