refactor: move all network operations out of `CGovernanceManager` #7106

UdjinM6 · 2026-01-16T13:31:15Z

Issue being fixed or feature implemented

Move all network operations from CGovernanceManager to the appropriate network layer classes (SyncManager and NetGovernance).

What was done?

Refactor only, see individual commits

How Has This Been Tested?

Run tests, sync gov data on mainnet node

Breaking Changes

n/a

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

…thods Replace SyncObjects() and SyncSingleObjVotes() with pure data accessors: - GetSyncableObjectInvs(): returns inventory of syncable governance objects - GetSyncableVoteInvs(): returns inventory of syncable votes for an object Move network message construction (SYNCSTATUSCOUNT) to NetGovernance, following the established pattern in NetSigning and NetInstantSend. This improves separation of concerns: - CGovernanceManager handles data access only - NetGovernance handles network protocol operations Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Complete separation of network operations from CGovernanceManager by moving RequestGovernanceObject functionality to the appropriate network layer classes. Changes to CGovernanceManager (data access only): - Add GetVoteBloomFilter() to build bloom filter for sync requests - Add GetOrphanVoteObjectHashes() to get object hashes for orphan votes - Modify ProcessVote() to use output param hashToRequest instead of doing network operations internally - Remove RequestGovernanceObject() and RequestOrphanObjects() Changes to SyncManager: - Add SendGovernanceObjectSyncRequest() for vote sync requests - Update RequestGovernanceObjectVotes() to use new method Changes to NetGovernance: - Handle orphan vote requests in ProcessMessage() after ProcessVote - Move RequestOrphanObjects logic into Schedule() for periodic requests Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

coderabbitai · 2026-01-16T13:35:23Z

Walkthrough

The changes decouple governance data collection from network I/O by replacing several network-performing methods with pure getters: SyncObjects → GetSyncableObjectInvs, SyncSingleObjVotes → GetSyncableVoteInvs, RequestGovernanceObject → GetVoteBloomFilter, and RequestOrphanObjects → GetOrphanVoteObjectHashes. ProcessVote now returns an output hashToRequest instead of issuing network requests directly. Callers in net_governance and sync_manager now perform network messaging and object requests using the returned data; SyncManager gained SendGovernanceObjectSyncRequest to centralize sending sync messages.

Sequence Diagram(s)

sequenceDiagram
    participant SyncMgr as SyncManager
    participant GovMgr as CGovernanceManager
    participant Connman as CConnman/Peers

    SyncMgr->>GovMgr: GetSyncableObjectInvs()
    GovMgr-->>SyncMgr: vector<CInv> (objects)
    SyncMgr->>Connman: Send SYNCSTATUSCOUNT (GOVOBJ)
    SyncMgr->>Connman: Relay invs for each CInv

sequenceDiagram
    participant Peer as RemotePeer
    participant NetGov as NetGovernance
    participant GovMgr as CGovernanceManager
    participant SyncMgr as SyncManager
    participant Connman as CConnman/Peers

    Peer->>NetGov: MNGOVERNANCEOBJECTVOTE (vote)
    NetGov->>GovMgr: ProcessVote(peer, vote, exception, hashToRequest)
    GovMgr-->>NetGov: bool accepted, hashToRequest (maybe zero)
    alt vote rejected && hashToRequest != 0
        NetGov->>SyncMgr: SendGovernanceObjectSyncRequest(peer, hashToRequest, useFilter=true)
        SyncMgr->>Connman: Push MNGOVERNANCESYNC (hashToRequest, bloom filter)
    end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title directly and accurately describes the main refactoring objective: moving network operations out of CGovernanceManager to appropriate network layer classes.
Description check	✅ Passed	The PR description clearly explains the refactoring objective, what was done, testing approach, and indicates no breaking changes, all of which align with the changeset.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

knst

LGTM overall, consider further improvements (2 commits attached to my comments); nits are minor

knst · 2026-01-21T10:56:39Z

src/governance/governance.cpp

-    CBloomFilter filter;
+    auto pObj = FindConstGovernanceObjectInternal(nHash);
+    if (!pObj) {
+        return CBloomFilter();


nit: CBloomFilter{}

knst · 2026-01-21T11:04:24Z

src/node/sync_manager.cpp

+
+    LogPrint(BCLog::GOBJECT, "SyncManager::%s -- nHash %s peer=%d\n", __func__, nHash.ToString(), pnode->GetId());
+
+    CBloomFilter filter = fUseFilter ? m_gov_manager.GetVoteBloomFilter(nHash) : CBloomFilter();


nit: CBloomFilter{}

knst · 2026-01-21T11:05:29Z

src/governance/governance.cpp

+                                     uint256& hashToRequest)
 {
    AssertLockNotHeld(cs_store);
+    hashToRequest = uint256();


nit: uint256{}

knst · 2026-01-21T11:20:40Z

src/governance/governance.cpp

        if (cmmapOrphanVotes.Insert(nHashGovobj, vote_time_pair_t(vote, count_seconds(GetTime<std::chrono::seconds>() +
                                                                                      GOVERNANCE_ORPHAN_EXPIRATION_TIME)))) {
-            LEAVE_CRITICAL_SECTION(cs_store);
-            RequestGovernanceObject(pfrom, nHashGovobj, connman);


can drop manual LEAVE_CRITICAL_SECTION after this change, see: knst@3966b84

knst · 2026-01-21T11:22:17Z

src/governance/net_governance.cpp

+
+            MessageProcessingResult ret;
+            ret.m_inventory = std::move(invs);
+            m_peer_manager->PeerPostProcessMessage(std::move(ret));


MessageProocessingResult wrapper is not necessary in context of NetHandler. See bc88147

github-actions · 2026-01-21T18:25:12Z

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

knst

LGTM 32b761b

UdjinM6 and others added 2 commits January 16, 2026 12:11

UdjinM6 added this to the 23.1 milestone Jan 16, 2026

UdjinM6 requested review from PastaPastaPasta, knst and kwvg January 16, 2026 18:36

knst reviewed Jan 21, 2026

View reviewed changes

knst and others added 3 commits January 21, 2026 21:21

refactor: replace manual lock/unlock to RAII LOCK in governance.cpp

4a47b4e

refactor: relay inventory directly without using MessageProcessingResult

0a92e25

refactor: apply review suggestions

32b761b

knst approved these changes Jan 21, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move all network operations out of `CGovernanceManager` #7106

refactor: move all network operations out of `CGovernanceManager` #7106

Uh oh!

UdjinM6 commented Jan 16, 2026

Uh oh!

coderabbitai bot commented Jan 16, 2026 •

edited

Loading

Uh oh!

knst left a comment

Uh oh!

knst Jan 21, 2026

Uh oh!

knst Jan 21, 2026

Uh oh!

knst Jan 21, 2026

Uh oh!

knst Jan 21, 2026 •

edited

Loading

Uh oh!

knst Jan 21, 2026

Uh oh!

github-actions bot commented Jan 21, 2026

Uh oh!

knst left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		LogPrint(BCLog::GOBJECT, "SyncManager::%s -- nHash %s peer=%d\n", __func__, nHash.ToString(), pnode->GetId());

		CBloomFilter filter = fUseFilter ? m_gov_manager.GetVoteBloomFilter(nHash) : CBloomFilter();

refactor: move all network operations out of CGovernanceManager #7106

Are you sure you want to change the base?

refactor: move all network operations out of CGovernanceManager #7106

Uh oh!

Conversation

UdjinM6 commented Jan 16, 2026

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

coderabbitai bot commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Sequence Diagram(s)

Estimated code review effort

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

knst Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

knst Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

knst Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

knst Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knst Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 21, 2026

✅ No Merge Conflicts Detected

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

refactor: move all network operations out of `CGovernanceManager` #7106

refactor: move all network operations out of `CGovernanceManager` #7106

coderabbitai bot commented Jan 16, 2026 •

edited

Loading

knst Jan 21, 2026 •

edited

Loading