refactor: move all network operations out of CGovernanceManager#7106
Conversation
…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>
WalkthroughThe 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 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)
✅ 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 |
knst
left a comment
There was a problem hiding this comment.
LGTM overall, consider further improvements (2 commits attached to my comments); nits are minor
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
ping @PastaPastaPasta |
…ainstate` 72b66af fix: add missing GMP dependency to dash-chainstate binary (Konstantin Akimov) ff7a110 fix: missing changes from bitcoin#26056 for dash-chainstate (Konstantin Akimov) fa9161e Merge bitcoin#24304: [kernel 0/n] Introduce `bitcoin-chainstate` (MarcoFalke) 02e5c3f refactor: move network code from llmq/utils to llmq/net_quorum (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented This PR is a first backport of the `libdashkernel` project, see for details: bitcoin#24303 ## What was done? This PR introduces an example/demo dash-chainstate executable using said library which can print out information about a datadir and take in new blocks on stdin. There are multiple PR that has been extracted from this PR and merged independently: - [merged] #7178 - [excluded, replaced by NullNodeSyncNotified] ~#7212 - [merged] #7247 - [merged] #7299 - [merged] #7303 Beside these PR multiple other PR has been done in the past, such as: #7178, #7070, #7008, #6992, #6959, #7106 (by Udjin), #7065 (by kwvg) and many other PRs. This PR: - new stub implementation NullNodeSyncNotifier of CActiveMaternodeSync for governance - moved network code from llmq/utils to llmq/net_quorum to avoid network dependencies for dash-chainstate - backport bitcoin#24304 There are several important notes about implementation: - UpdateUncommittedBlockStructures is segwit related; omitted from bitcoin-chainstate.cpp - call chainman.UnloadBlockIndex is removed by bitcoin#22564, not backported - binary name is renamed from bitcoin-chainstate to dash-chainstate. Names of source files and some variables are untouched, but final artefact has the correct dashified name - added LIBDASHBLS to list of libraries to link - couple functions are reimplemented to avoid balooning dash-chainstate by including extra heavy depenencies - ValueFromAmount (reimplemented, core_write.cpp) - GetPrettyExceptionStr (reimplemented, stacktrackes.cpp) - g_stats_client (redefined) There are several modules that should be removed in the near future from dash-chainstate binary, but out-of-scope of this PR: - protocol.cpp -- move NetMsgType inline helpers out of chainlock/clsig.h, governance/object.h, evo/simplifiedmns.h into their .cpp files - llmq/{dkgsession,dkgsessionhandler,dkgsessionmgr,debug}.cpp and batchedlogger.cpp -- CQuorumManager already null-guards m_qdkgsman; drop once ConnectManagers() is never called on the kernel side - llmq/signing.cpp -- split CSigningManager out of LLMQContext into a post-construction Connect() so the context becomes quorum-only - governance/*.cpp -- promote the 4 methods used by CMNPaymentsProcessor (IsValid, IsSuperblockTriggered, IsValidSuperblock, GetSuperblockPayments) to a base interface; instantiate a NullGovernanceManager in bitcoin-chainstate.cpp ## How Has This Been Tested? New binary `dash-chainstate` is produced, functional & unit tests succeed. Run dash-chainstate: ``` $ src/dash-chainstate /home/knst/.dashcore/ Hello! I'm going to print out some information about your datadir. Path: "/home/knst/.dashcore" Reindexing: false Snapshot Active: false Active Height: 2294070 Active IBD: true CBlockIndex(pprev=0x5654a3bdb218, nHeight=2294070, merkle=5c87e557b9e697e9308a4a6bd684e362910fe418ca60e59ae1376e91c487df77, hashBlock=0000000000000010938b2d1c2f1ff08163dae17f4196a41d73b9e6e3c4114cc2) ``` ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [x] 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 - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 72b66af Tree-SHA512: 5b58f9359493f6d7887d80950facefd5f0043a13c6aa1ad09228c1f861e6524b4ed5bdc4b5e7b7d7bc4ae32b2517943ad7f2cf99acb6c0b84b35af346cf6140d
Issue being fixed or feature implemented
Move all network operations from
CGovernanceManagerto the appropriate network layer classes (SyncManagerandNetGovernance).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: