Merge dashpay#5977: refactor: move CConnman::RelayInv{Filtered} to PeerManager, remove CConnman::RelayTransaction

f2fe39f trivial: add `Asserts` for `m_peerman` pointer container uses (Kittywhiskers Van Gogh)
35be4e2 llmq: pass PeerManager to llmq::CInstantSendManager constructor (Kittywhiskers Van Gogh)
bfd33cd net: move CConnman::RelayInv{Filtered} into PeerManager (Kittywhiskers Van Gogh)
313a7e9 trivial: cleanup unnecessary headers in context files (Kittywhiskers Van Gogh)
c3f1ac2 net: retire CConnman::RelayTransaction, use PeerManager::RelayTransaction (Kittywhiskers Van Gogh)
0323c6c net: move Relay{Inv, InvFiltered, Transaction} out of CConnman (Kittywhiskers Van Gogh)
d54ba44 net: use ForEachNode instead of manually iterating through vNodes (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional information

  * Dependency for dashpay#5982
  * Unfortunately, we are stuck with using the `unique_ptr` const-ref of `PeerManager` for initializing {`CJ`, `LLMQ`}`Context` due to some unit tests depending on Dash-specific entities but based on a testing setup that doesn't initialize `PeerManager` (specifically `validation_chainstatemanager_tests`, which uses `ChainTestingSetup`, as opposed to `TestingSetup`, which initializes `PeerManager`).

    Attempting to dereference it earlier will result in crashing (see CI pipeline [here](https://gitlab.com/dashpay/dash/-/pipelines/1245924304))

  ## Breaking Changes

  None observed.

  ## Checklist:

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

ACKs for top commit:
  PastaPastaPasta:
    re-utACK [f2fe39f](dashpay@f2fe39f)
  knst:
    utACK [f2fe39f](dashpay@f2fe39f)

Tree-SHA512: 5e9ca47ba9f7f0c5fdc93bf6cfd99d91be6d134610d2bd3dccde17443a956c7d8848b3406082ee8b6ee3fd25f586ce7bac45bc89b568424479b8fc149172ae51

Author: PastaPastaPasta

src/coinjoin/context.cpp

@@ -4,24 +4,20 @@
 
 #include <coinjoin/context.h>
 
-#include <net.h>
-#include <txmempool.h>
-#include <validation.h>
-
 #ifdef ENABLE_WALLET
 #include <coinjoin/client.h>
 #endif // ENABLE_WALLET
 #include <coinjoin/server.h>
 
 CJContext::CJContext(CChainState& chainstate, CConnman& connman, CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_metaman,
                      CTxMemPool& mempool, const CActiveMasternodeManager* const mn_activeman, const CMasternodeSync& mn_sync,
-                     bool relay_txes) :
+                     const std::unique_ptr<PeerManager>& peerman, bool relay_txes) :
     dstxman{std::make_unique<CDSTXManager>()},
 #ifdef ENABLE_WALLET
     walletman{std::make_unique<CoinJoinWalletManager>(connman, dmnman, mn_metaman, mempool, mn_sync, queueman)},
     queueman {relay_txes ? std::make_unique<CCoinJoinClientQueueManager>(connman, *walletman, dmnman, mn_metaman, mn_sync) : nullptr},
 #endif // ENABLE_WALLET
-    server{std::make_unique<CCoinJoinServer>(chainstate, connman, dmnman, *dstxman, mn_metaman, mempool, mn_activeman, mn_sync)}
+    server{std::make_unique<CCoinJoinServer>(chainstate, connman, dmnman, *dstxman, mn_metaman, mempool, mn_activeman, mn_sync, peerman)}
 {}
 
 CJContext::~CJContext() {}

src/coinjoin/context.h

@@ -21,6 +21,7 @@ class CDSTXManager;
 class CMasternodeMetaMan;
 class CMasternodeSync;
 class CTxMemPool;
+class PeerManager;
 
 #ifdef ENABLE_WALLET
 class CCoinJoinClientQueueManager;
@@ -32,7 +33,7 @@ struct CJContext {
     CJContext(const CJContext&) = delete;
     CJContext(CChainState& chainstate, CConnman& connman, CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_metaman,
               CTxMemPool& mempool, const CActiveMasternodeManager* const mn_activeman, const CMasternodeSync& mn_sync,
-              bool relay_txes);
+              const std::unique_ptr<PeerManager>& peerman, bool relay_txes);
     ~CJContext();
 
     const std::unique_ptr<CDSTXManager> dstxman;

src/coinjoin/server.cpp

@@ -12,6 +12,7 @@
 #include <masternode/sync.h>
 #include <net.h>
 #include <netmessagemaker.h>
+#include <net_processing.h>
 #include <script/interpreter.h>
 #include <shutdown.h>
 #include <streams.h>
@@ -348,7 +349,7 @@ void CCoinJoinServer::CommitFinalTransaction()
     LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CommitFinalTransaction -- TRANSMITTING DSTX\n");
 
     CInv inv(MSG_DSTX, hashTx);
-    connman.RelayInv(inv);
+    Assert(m_peerman)->RelayInv(inv);
 
     // Tell the clients it was successful
     RelayCompletedTransaction(MSG_SUCCESS);
@@ -459,7 +460,7 @@ void CCoinJoinServer::ConsumeCollateral(const CTransactionRef& txref) const
     if (!ATMPIfSaneFee(m_chainstate, mempool, txref, false /* bypass_limits */)) {
         LogPrint(BCLog::COINJOIN, "%s -- AcceptToMemoryPool failed\n", __func__);
     } else {
-        connman.RelayTransaction(*txref, static_cast<bool>(m_dstxman.GetDSTX(txref->GetHash())));
+        Assert(m_peerman)->RelayTransaction(txref->GetHash());
         LogPrint(BCLog::COINJOIN, "%s -- Collateral was consumed\n", __func__);
     }
 }

src/coinjoin/server.h

@@ -18,6 +18,7 @@ class CDSTXManager;
 class CMasternodeMetaMan;
 class CNode;
 class CTxMemPool;
+class PeerManager;
 
 class UniValue;
 
@@ -34,6 +35,7 @@ class CCoinJoinServer : public CCoinJoinBaseSession, public CCoinJoinBaseManager
     CTxMemPool& mempool;
     const CActiveMasternodeManager* const m_mn_activeman;
     const CMasternodeSync& m_mn_sync;
+    const std::unique_ptr<PeerManager>& m_peerman;
 
     // Mixing uses collateral transactions to trust parties entering the pool
     // to behave honestly. If they don't it takes their money.
@@ -90,7 +92,7 @@ class CCoinJoinServer : public CCoinJoinBaseSession, public CCoinJoinBaseManager
 public:
     explicit CCoinJoinServer(CChainState& chainstate, CConnman& _connman, CDeterministicMNManager& dmnman, CDSTXManager& dstxman,
                              CMasternodeMetaMan& mn_metaman, CTxMemPool& mempool, const CActiveMasternodeManager* const mn_activeman,
-                             const CMasternodeSync& mn_sync) :
+                             const CMasternodeSync& mn_sync, const std::unique_ptr<PeerManager>& peerman) :
         m_chainstate(chainstate),
         connman(_connman),
         m_dmnman(dmnman),
@@ -99,6 +101,7 @@ class CCoinJoinServer : public CCoinJoinBaseSession, public CCoinJoinBaseManager
         mempool(mempool),
         m_mn_activeman(mn_activeman),
         m_mn_sync(mn_sync),
+        m_peerman(peerman),
         vecSessionCollaterals(),
         fUnitTest(false)
     {}

src/dsnotificationinterface.cpp

@@ -11,6 +11,7 @@
 #include <dsnotificationinterface.h>
 #include <governance/governance.h>
 #include <masternode/sync.h>
+#include <net_processing.h>
 #include <validation.h>
 
 #include <evo/deterministicmns.h>
@@ -26,13 +27,15 @@
 CDSNotificationInterface::CDSNotificationInterface(CConnman& connman,
                                                    CMasternodeSync& mn_sync,
                                                    CGovernanceManager& govman,
+                                                   PeerManager& peerman,
                                                    const CActiveMasternodeManager* const mn_activeman,
                                                    const std::unique_ptr<CDeterministicMNManager>& dmnman,
                                                    const std::unique_ptr<LLMQContext>& llmq_ctx,
                                                    const std::unique_ptr<CJContext>& cj_ctx)
   : m_connman(connman),
     m_mn_sync(mn_sync),
     m_govman(govman),
+    m_peerman(peerman),
     m_mn_activeman(mn_activeman),
     m_dmnman(dmnman),
     m_llmq_ctx(llmq_ctx),
@@ -96,7 +99,7 @@ void CDSNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, con
     m_llmq_ctx->qdkgsman->UpdatedBlockTip(pindexNew, fInitialDownload);
     m_llmq_ctx->ehfSignalsHandler->UpdatedBlockTip(pindexNew);
 
-    if (!fDisableGovernance) m_govman.UpdatedBlockTip(pindexNew, m_connman, m_mn_activeman);
+    if (!fDisableGovernance) m_govman.UpdatedBlockTip(pindexNew, m_connman, m_peerman, m_mn_activeman);
 }
 
 void CDSNotificationInterface::TransactionAddedToMempool(const CTransactionRef& ptx, int64_t nAcceptTime)

src/dsnotificationinterface.h

@@ -12,6 +12,7 @@ class CConnman;
 class CDeterministicMNManager;
 class CGovernanceManager;
 class CMasternodeSync;
+class PeerManager;
 struct CJContext;
 struct LLMQContext;
 
@@ -21,6 +22,7 @@ class CDSNotificationInterface : public CValidationInterface
     explicit CDSNotificationInterface(CConnman& connman,
                                       CMasternodeSync& mn_sync,
                                       CGovernanceManager& govman,
+                                      PeerManager& peerman,
                                       const CActiveMasternodeManager* const mn_activeman,
                                       const std::unique_ptr<CDeterministicMNManager>& dmnman,
                                       const std::unique_ptr<LLMQContext>& llmq_ctx,
@@ -45,10 +47,9 @@ class CDSNotificationInterface : public CValidationInterface
 
 private:
     CConnman& m_connman;
-
     CMasternodeSync& m_mn_sync;
     CGovernanceManager& m_govman;
-
+    PeerManager& m_peerman;
     const CActiveMasternodeManager* const m_mn_activeman;
     const std::unique_ptr<CDeterministicMNManager>& m_dmnman;
     const std::unique_ptr<LLMQContext>& m_llmq_ctx;

src/governance/governance.cpp

@@ -119,7 +119,7 @@ bool CGovernanceManager::SerializeVoteForHash(const uint256& nHash, CDataStream&
     return cmapVoteToObject.Get(nHash, pGovobj) && pGovobj->GetVoteFile().SerializeVoteToStream(nHash, ss);
 }
 
-PeerMsgRet CGovernanceManager::ProcessMessage(CNode& peer, CConnman& connman, std::string_view msg_type, CDataStream& vRecv)
+PeerMsgRet CGovernanceManager::ProcessMessage(CNode& peer, CConnman& connman, PeerManager& peerman, std::string_view msg_type, CDataStream& vRecv)
 {
     if (fDisableGovernance) return {};
     if (m_mn_sync == nullptr || !m_mn_sync->IsBlockchainSynced()) return {};
@@ -213,7 +213,7 @@ PeerMsgRet CGovernanceManager::ProcessMessage(CNode& peer, CConnman& connman, st
             return {};
         }
 
-        AddGovernanceObject(govobj, connman, &peer);
+        AddGovernanceObject(govobj, peerman, &peer);
     }
 
     // A NEW GOVERNANCE OBJECT VOTE HAS ARRIVED
@@ -248,7 +248,7 @@ PeerMsgRet CGovernanceManager::ProcessMessage(CNode& peer, CConnman& connman, st
         if (ProcessVote(&peer, vote, exception, connman)) {
             LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECTVOTE -- %s new\n", strHash);
             m_mn_sync->BumpAssetLastTime("MNGOVERNANCEOBJECTVOTE");
-            vote.Relay(connman, *m_mn_sync, tip_mn_list);
+            vote.Relay(peerman, *m_mn_sync, tip_mn_list);
         } else {
             LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECTVOTE -- Rejected vote, error = %s\n", exception.what());
             if ((exception.GetNodePenalty() != 0) && m_mn_sync->IsSynced()) {
@@ -260,7 +260,7 @@ PeerMsgRet CGovernanceManager::ProcessMessage(CNode& peer, CConnman& connman, st
     return {};
 }
 
-void CGovernanceManager::CheckOrphanVotes(CGovernanceObject& govobj, CConnman& connman)
+void CGovernanceManager::CheckOrphanVotes(CGovernanceObject& govobj, PeerManager& peerman)
 {
     uint256 nHash = govobj.GetHash();
     std::vector<vote_time_pair_t> vecVotePairs;
@@ -277,7 +277,7 @@ void CGovernanceManager::CheckOrphanVotes(CGovernanceObject& govobj, CConnman& c
         if (pairVote.second < nNow) {
             fRemove = true;
         } else if (govobj.ProcessVote(m_mn_metaman, *this, tip_mn_list, vote, e)) {
-            vote.Relay(connman, *Assert(m_mn_sync), tip_mn_list);
+            vote.Relay(peerman, *Assert(m_mn_sync), tip_mn_list);
             fRemove = true;
         }
         if (fRemove) {
@@ -286,7 +286,7 @@ void CGovernanceManager::CheckOrphanVotes(CGovernanceObject& govobj, CConnman& c
     }
 }
 
-void CGovernanceManager::AddGovernanceObject(CGovernanceObject& govobj, CConnman& connman, const CNode* pfrom)
+void CGovernanceManager::AddGovernanceObject(CGovernanceObject& govobj, PeerManager& peerman, const CNode* pfrom)
 {
     uint256 nHash = govobj.GetHash();
     std::string strHash = nHash.ToString();
@@ -331,7 +331,7 @@ void CGovernanceManager::AddGovernanceObject(CGovernanceObject& govobj, CConnman
     }
 
     LogPrint(BCLog::GOBJECT, "CGovernanceManager::AddGovernanceObject -- %s new, received from peer %s\n", strHash, pfrom ? pfrom->GetLogString() : "nullptr");
-    govobj.Relay(connman, *Assert(m_mn_sync));
+    govobj.Relay(peerman, *Assert(m_mn_sync));
 
     // Update the rate buffer
     MasternodeRateUpdate(govobj);
@@ -340,7 +340,7 @@ void CGovernanceManager::AddGovernanceObject(CGovernanceObject& govobj, CConnman
 
     // WE MIGHT HAVE PENDING/ORPHAN VOTES FOR THIS OBJECT
 
-    CheckOrphanVotes(govobj, connman);
+    CheckOrphanVotes(govobj, peerman);
 
     // SEND NOTIFICATION TO SCRIPT/ZMQ
     GetMainSignals().NotifyGovernanceObject(std::make_shared<const Governance::Object>(govobj.Object()));
@@ -672,7 +672,7 @@ std::optional<const CSuperblock> CGovernanceManager::CreateSuperblockCandidate(i
     return CSuperblock(nNextSuperblock, std::move(payments));
 }
 
-std::optional<const CGovernanceObject> CGovernanceManager::CreateGovernanceTrigger(const std::optional<const CSuperblock>& sb_opt, CConnman& connman,
+std::optional<const CGovernanceObject> CGovernanceManager::CreateGovernanceTrigger(const std::optional<const CSuperblock>& sb_opt, PeerManager& peerman,
                                                                                    const CActiveMasternodeManager* const mn_activeman)
 {
     if (!fMasternodeMode) return std::nullopt;
@@ -718,11 +718,11 @@ std::optional<const CGovernanceObject> CGovernanceManager::CreateGovernanceTrigg
     }
 
     // The trigger we just created looks good, submit it
-    AddGovernanceObject(gov_sb, connman);
+    AddGovernanceObject(gov_sb, peerman);
     return std::make_optional<CGovernanceObject>(gov_sb);
 }
 
-void CGovernanceManager::VoteGovernanceTriggers(const std::optional<const CGovernanceObject>& trigger_opt, CConnman& connman,
+void CGovernanceManager::VoteGovernanceTriggers(const std::optional<const CGovernanceObject>& trigger_opt, CConnman& connman, PeerManager& peerman,
                                                 const CActiveMasternodeManager* const mn_activeman)
 {
     // only active masternodes can vote on triggers
@@ -736,7 +736,7 @@ void CGovernanceManager::VoteGovernanceTriggers(const std::optional<const CGover
         assert(!votedFundingYesTriggerHash.has_value());
         // Vote YES-FUNDING for the trigger we like
         const uint256 gov_sb_hash = trigger_opt.value().GetHash();
-        if (!VoteFundingTrigger(gov_sb_hash, VOTE_OUTCOME_YES, connman, mn_activeman)) {
+        if (!VoteFundingTrigger(gov_sb_hash, VOTE_OUTCOME_YES, connman, peerman, mn_activeman)) {
             LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Voting YES-FUNDING for new trigger:%s failed\n", __func__, gov_sb_hash.ToString());
             // this should never happen, bail out
             return;
@@ -759,7 +759,7 @@ void CGovernanceManager::VoteGovernanceTriggers(const std::optional<const CGover
             LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Not voting NO-FUNDING for trigger:%s, we voted yes for it already\n", __func__, trigger_hash.ToString());
             continue;
         }
-        if (!VoteFundingTrigger(trigger_hash, VOTE_OUTCOME_NO, connman, mn_activeman)) {
+        if (!VoteFundingTrigger(trigger_hash, VOTE_OUTCOME_NO, connman, peerman, mn_activeman)) {
             LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Voting NO-FUNDING for trigger:%s failed\n", __func__, trigger_hash.ToString());
             // failing here is ok-ish
             continue;
@@ -768,7 +768,7 @@ void CGovernanceManager::VoteGovernanceTriggers(const std::optional<const CGover
     }
 }
 
-bool CGovernanceManager::VoteFundingTrigger(const uint256& nHash, const vote_outcome_enum_t outcome, CConnman& connman,
+bool CGovernanceManager::VoteFundingTrigger(const uint256& nHash, const vote_outcome_enum_t outcome, CConnman& connman, PeerManager& peerman,
                                             const CActiveMasternodeManager* const mn_activeman)
 {
     if (!fMasternodeMode) return false;
@@ -778,7 +778,7 @@ bool CGovernanceManager::VoteFundingTrigger(const uint256& nHash, const vote_out
     vote.Sign(*mn_activeman);
 
     CGovernanceException exception;
-    if (!ProcessVoteAndRelay(vote, exception, connman)) {
+    if (!ProcessVoteAndRelay(vote, exception, connman, peerman)) {
         LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Vote FUNDING %d for trigger:%s failed:%s\n", __func__, outcome, nHash.ToString(), exception.what());
         return false;
     }
@@ -1066,11 +1066,11 @@ bool CGovernanceManager::MasternodeRateCheck(const CGovernanceObject& govobj, bo
     return false;
 }
 
-bool CGovernanceManager::ProcessVoteAndRelay(const CGovernanceVote& vote, CGovernanceException& exception, CConnman& connman)
+bool CGovernanceManager::ProcessVoteAndRelay(const CGovernanceVote& vote, CGovernanceException& exception, CConnman& connman, PeerManager& peerman)
 {
     bool fOK = ProcessVote(/* pfrom = */ nullptr, vote, exception, connman);
     if (fOK) {
-        vote.Relay(connman, *Assert(m_mn_sync), Assert(m_dmnman)->GetListAtChainTip());
+        vote.Relay(peerman, *Assert(m_mn_sync), Assert(m_dmnman)->GetListAtChainTip());
     }
     return fOK;
 }
@@ -1129,7 +1129,7 @@ bool CGovernanceManager::ProcessVote(CNode* pfrom, const CGovernanceVote& vote,
     return fOk;
 }
 
-void CGovernanceManager::CheckPostponedObjects(CConnman& connman)
+void CGovernanceManager::CheckPostponedObjects(PeerManager& peerman)
 {
     if (!Assert(m_mn_sync)->IsSynced()) return;
 
@@ -1146,7 +1146,7 @@ void CGovernanceManager::CheckPostponedObjects(CConnman& connman)
         bool fMissingConfirmations;
         if (govobj.IsCollateralValid(strError, fMissingConfirmations)) {
             if (govobj.IsValidLocally(Assert(m_dmnman)->GetListAtChainTip(), strError, false)) {
-                AddGovernanceObject(govobj, connman);
+                AddGovernanceObject(govobj, peerman);
             } else {
                 LogPrint(BCLog::GOBJECT, "CGovernanceManager::CheckPostponedObjects -- %s invalid\n", nHash.ToString());
             }
@@ -1179,7 +1179,7 @@ void CGovernanceManager::CheckPostponedObjects(CConnman& connman)
             if (fValid) {
                 if (fReady) {
                     LogPrint(BCLog::GOBJECT, "CGovernanceManager::CheckPostponedObjects -- additional relay: hash = %s\n", govobj.GetHash().ToString());
-                    govobj.Relay(connman, *m_mn_sync);
+                    govobj.Relay(peerman, *m_mn_sync);
                 } else {
                     it++;
                     continue;
@@ -1474,7 +1474,7 @@ UniValue CGovernanceManager::ToJson() const
     return jsonObj;
 }
 
-void CGovernanceManager::UpdatedBlockTip(const CBlockIndex* pindex, CConnman& connman, const CActiveMasternodeManager* const mn_activeman)
+void CGovernanceManager::UpdatedBlockTip(const CBlockIndex* pindex, CConnman& connman, PeerManager& peerman, const CActiveMasternodeManager* const mn_activeman)
 {
     // Note this gets called from ActivateBestChain without cs_main being held
     // so it should be safe to lock our mutex here without risking a deadlock
@@ -1486,8 +1486,8 @@ void CGovernanceManager::UpdatedBlockTip(const CBlockIndex* pindex, CConnman& co
     }
 
     const auto sb_opt = CreateSuperblockCandidate(pindex->nHeight);
-    const auto trigger_opt = CreateGovernanceTrigger(sb_opt, connman, mn_activeman);
-    VoteGovernanceTriggers(trigger_opt, connman, mn_activeman);
+    const auto trigger_opt = CreateGovernanceTrigger(sb_opt, peerman, mn_activeman);
+    VoteGovernanceTriggers(trigger_opt, connman, peerman, mn_activeman);
 
     nCachedBlockHeight = pindex->nHeight;
     LogPrint(BCLog::GOBJECT, "CGovernanceManager::UpdatedBlockTip -- nCachedBlockHeight: %d\n", nCachedBlockHeight);
@@ -1496,7 +1496,7 @@ void CGovernanceManager::UpdatedBlockTip(const CBlockIndex* pindex, CConnman& co
         RemoveInvalidVotes();
     }
 
-    CheckPostponedObjects(connman);
+    CheckPostponedObjects(peerman);
 
     CSuperblockManager::ExecuteBestSuperblock(*this, Assert(m_dmnman)->GetListAtChainTip(), pindex->nHeight);
 }