Merge #7105: refactor: drop dependency of CGovernanceManager on CNetFulfilledRequestManager

295567f refactor: drop dependency of CGovernanceManager on CNetFulfilledRequestManager (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  `CGovernanceManager` should not be directly aware of network stuff.

  ## What was done?
  Moved usages of `CNetFulfilledRequestManager` from CGovernanceManager to NetGovernance where it belongs to, to follow-up #7008

  ## How Has This Been Tested?
  Run unit & functional tests.

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] 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
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  kwvg:
    utACK 295567f
  UdjinM6:
    utACK 295567f

Tree-SHA512: a5b6016064676d59dd94d08eac9880047ede0e87624edb6ebc08686436818b97d696ee4a724063325171dec73cd743bd1e9f40eb3f10b957acdac89a6c720269

Author: PastaPastaPasta

src/governance/governance.cpp

@@ -15,7 +15,6 @@
 #include <governance/validators.h>
 #include <masternode/meta.h>
 #include <masternode/sync.h>
-#include <netfulfilledman.h>
 #include <netmessagemaker.h>
 #include <protocol.h>
 #include <shutdown.h>
@@ -63,12 +62,11 @@ GovernanceStore::GovernanceStore() :
 {
 }
 
-CGovernanceManager::CGovernanceManager(CMasternodeMetaMan& mn_metaman, CNetFulfilledRequestManager& netfulfilledman,
+CGovernanceManager::CGovernanceManager(CMasternodeMetaMan& mn_metaman,
                                        const ChainstateManager& chainman,
                                        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_netfulfilledman{netfulfilledman},
     m_chainman{chainman},
     m_dmnman{dmnman},
     m_mn_sync{mn_sync},
@@ -649,18 +647,9 @@ MessageProcessingResult CGovernanceManager::SyncSingleObjVotes(CNode& peer, cons
 MessageProcessingResult CGovernanceManager::SyncObjects(CNode& peer, CConnman& connman) const
 {
     LOCK(cs_store);
-    assert(m_netfulfilledman.IsValid());
-
     // do not provide any data until our node is synced
     if (!m_mn_sync.IsSynced()) return {};
 
-    if (m_netfulfilledman.HasFulfilledRequest(peer.addr, NetMsgType::MNGOVERNANCESYNC)) {
-        // Asking for the whole list multiple times in a short period of time is no good
-        LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s -- peer already asked me for the list\n", __func__);
-        return MisbehavingError{20};
-    }
-    m_netfulfilledman.AddFulfilledRequest(peer.addr, NetMsgType::MNGOVERNANCESYNC);
-
     // SYNC GOVERNANCE OBJECTS WITH OTHER CLIENT
 
     LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s -- syncing all objects to peer=%d\n", __func__, peer.GetId());

src/governance/governance.h

@@ -41,7 +41,6 @@ class CGovernanceObject;
 class CGovernanceVote;
 class CMasternodeMetaMan;
 class CMasternodeSync;
-class CNetFulfilledRequestManager;
 class CSporkManager;
 class CSuperblock;
 
@@ -252,7 +251,6 @@ class CGovernanceManager : public GovernanceStore, public GovernanceSignerParent
     bool is_valid{false};
 
     CMasternodeMetaMan& m_mn_metaman;
-    CNetFulfilledRequestManager& m_netfulfilledman;
     const ChainstateManager& m_chainman;
     const std::unique_ptr<CDeterministicMNManager>& m_dmnman;
     CMasternodeSync& m_mn_sync;
@@ -274,7 +272,7 @@ class CGovernanceManager : public GovernanceStore, public GovernanceSignerParent
     CGovernanceManager() = delete;
     CGovernanceManager(const CGovernanceManager&) = delete;
     CGovernanceManager& operator=(const CGovernanceManager&) = delete;
-    explicit CGovernanceManager(CMasternodeMetaMan& mn_metaman, CNetFulfilledRequestManager& netfulfilledman,
+    explicit CGovernanceManager(CMasternodeMetaMan& mn_metaman,
                                 const ChainstateManager& chainman,
                                 const std::unique_ptr<CDeterministicMNManager>& dmnman, CMasternodeSync& mn_sync);
     ~CGovernanceManager();

src/governance/net_governance.cpp

@@ -10,6 +10,7 @@
 #include <logging.h>
 #include <masternode/sync.h>
 #include <net.h>
+#include <netfulfilledman.h>
 #include <scheduler.h>
 
 class CConnman;
@@ -61,7 +62,15 @@ void NetGovernance::ProcessMessage(CNode& peer, const std::string& msg_type, CDa
 
         LogPrint(BCLog::GOBJECT, "MNGOVERNANCESYNC -- syncing governance objects to our peer %s\n", peer.GetLogString());
         if (nProp == uint256()) {
-            m_peer_manager->PeerPostProcessMessage(m_gov_manager.SyncObjects(peer, m_connman));
+            assert(m_netfulfilledman.IsValid());
+            if (!m_netfulfilledman.HasFulfilledRequest(peer.addr, NetMsgType::MNGOVERNANCESYNC)) {
+                m_netfulfilledman.AddFulfilledRequest(peer.addr, NetMsgType::MNGOVERNANCESYNC);
+                m_peer_manager->PeerPostProcessMessage(m_gov_manager.SyncObjects(peer, m_connman));
+            } else {
+                // Asking for the whole list multiple times in a short period of time is no good
+                LogPrint(BCLog::GOBJECT, "MNGOVERNANCESYNC -- peer already asked me for the list\n");
+                m_peer_manager->PeerMisbehaving(peer.GetId(), 20);
+            }
         } else {
             m_peer_manager->PeerPostProcessMessage(m_gov_manager.SyncSingleObjVotes(peer, nProp, filter, m_connman));
         }

src/governance/net_governance.h

@@ -9,15 +9,17 @@
 
 class CGovernanceManager;
 class CMasternodeSync;
+class CNetFulfilledRequestManager;
 
 class NetGovernance final : public NetHandler
 {
 public:
-    NetGovernance(PeerManagerInternal* peer_manager, CGovernanceManager& gov_manager, CMasternodeSync& node_sync,
+    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)
     {
     }
@@ -28,6 +30,7 @@ class NetGovernance final : public NetHandler
 private:
     CGovernanceManager& m_gov_manager;
     CMasternodeSync& m_node_sync;
+    CNetFulfilledRequestManager& m_netfulfilledman;
     CConnman& m_connman;
 };
 

src/init.cpp

@@ -1983,7 +1983,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
          */
         node.mn_sync = std::make_unique<CMasternodeSync>(std::make_unique<NodeSyncNotifierImpl>(*node.connman, *node.netfulfilledman));
 
-        node.govman = std::make_unique<CGovernanceManager>(*node.mn_metaman, *node.netfulfilledman, *node.chainman, node.dmnman, *node.mn_sync);
+        node.govman = std::make_unique<CGovernanceManager>(*node.mn_metaman, *node.chainman, node.dmnman, *node.mn_sync);
 
         const bool fReset = fReindex;
         bilingual_str strLoadError;
@@ -2259,7 +2259,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
             }
             return InitError(strprintf(_("Failed to clear governance cache at %s"), file_path));
         }
-        node.peerman->AddExtraHandler(std::make_unique<NetGovernance>(node.peerman.get(), *node.govman, *node.mn_sync, *node.connman));
+        node.peerman->AddExtraHandler(std::make_unique<NetGovernance>(node.peerman.get(), *node.govman, *node.mn_sync, *node.netfulfilledman, *node.connman));
     }
     node.peerman->AddExtraHandler(std::make_unique<SyncManager>(node.peerman.get(), *node.govman, *node.mn_sync, *node.connman, *node.netfulfilledman));
 

src/test/util/setup_common.cpp

@@ -286,7 +286,7 @@ ChainTestingSetup::ChainTestingSetup(const std::string& chainName, const std::ve
 
     m_node.mnhf_manager = std::make_unique<CMNHFManager>(*m_node.evodb, *m_node.chainman);
     m_node.mn_sync = std::make_unique<CMasternodeSync>(std::make_unique<NodeSyncNotifierImpl>(*m_node.connman, *m_node.netfulfilledman));
-    m_node.govman = std::make_unique<CGovernanceManager>(*m_node.mn_metaman, *m_node.netfulfilledman, *m_node.chainman, m_node.dmnman, *m_node.mn_sync);
+    m_node.govman = std::make_unique<CGovernanceManager>(*m_node.mn_metaman, *m_node.chainman, m_node.dmnman, *m_node.mn_sync);
 
     // Start script-checking threads. Set g_parallel_script_checks to true so they are used.
     constexpr int script_check_threads = 2;