Merge #21162: Net Processing: Move RelayTransaction() into PeerManager

680eb56d828ce358b4e000c140f5b247ff5e6179 [net processing] Don't pass CConnman to RelayTransactions (John Newbery)
a38a4e8f039dfabfd9435f3a63f1a9b56de086d6 [net processing] Move RelayTransaction into PeerManager (John Newbery)

Pull request description:

  This is the first part of #21160. It moves the RelayTransaction() function to be a member function of the PeerManager class. This is required in order to move the transaction inventory data into the Peer object, since Peer objects are only accessible from within PeerManager.

ACKs for top commit:
  ajtowns:
    ACK 680eb56d828ce358b4e000c140f5b247ff5e6179

Tree-SHA512: 8c93491a4392b6369bb7f090de326a63cd62a088de59026e202f226f64ded50a0cf1a95ed703328860f02a9d2f64d3a87ca1bca9a6075b978bd111d384766235

src/net_processing.cpp

@@ -241,6 +241,7 @@ public:
     void CheckForStaleTipAndEvictPeers(const Consensus::Params &consensusParams) override;
     bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) override;
     bool IgnoresIncomingTxs() override { return m_ignore_incoming_txs; }
+    void RelayTransaction(const uint256& txid) override;
     void SetBestHeight(int height) override { m_best_height = height; };
     void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message = "") override;
     void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv,
@@ -255,7 +256,7 @@ private:
     void EvictExtraOutboundPeers(int64_t time_in_seconds) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
     /** Retrieve unbroadcast transactions from the mempool and reattempt sending to peers */
-    void ReattemptInitialBroadcast(CScheduler& scheduler) const;
+    void ReattemptInitialBroadcast(CScheduler& scheduler);
 
     /** Get a shared pointer to the Peer object.
      *  May return an empty shared_ptr if the Peer object can't be found. */
@@ -1187,14 +1188,14 @@ void PeerManagerImpl::InitializeNode(CNode *pnode) {
         PushNodeVersion(*pnode, GetTime());
 }
 
-void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler) const
+void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler)
 {
     std::set<uint256> unbroadcast_txids = m_mempool.GetUnbroadcastTxs();
 
     for (const uint256& txid : unbroadcast_txids) {
         // Sanity check: all unbroadcast txns should exist in the mempool
         if (m_mempool.exists(txid)) {
-            RelayTransaction(txid, m_connman);
+            RelayTransaction(txid);
         } else {
             m_mempool.RemoveUnbroadcastTx(txid, true);
         }
@@ -1885,10 +1886,10 @@ bool PeerManagerImpl::AlreadyHave(const CInv& inv)
     return true;
 }
 
-void RelayTransaction(const uint256& txid, const CConnman& connman)
+void PeerManagerImpl::RelayTransaction(const uint256& txid)
 {
     CInv inv(CCoinJoin::GetDSTX(txid) ? MSG_DSTX : MSG_TX, txid);
-    connman.ForEachNode([&inv](CNode* pnode)
+    m_connman.ForEachNode([&inv](CNode* pnode)
     {
         pnode->PushInventory(inv);
     });
@@ -2527,7 +2528,7 @@ void PeerManagerImpl::ProcessOrphanTx(std::set<uint256>& orphan_work_set)
         if (AcceptToMemoryPool(m_chainman.ActiveChainstate(), m_mempool, orphan_state, porphanTx,
                 false /* bypass_limits */, 0 /* nAbsurdFee */)) {
             LogPrint(BCLog::MEMPOOL, "   accepted orphan tx %s\n", orphanHash.ToString());
-            RelayTransaction(orphanTx.GetHash(), m_connman);
+            RelayTransaction(orphanTx.GetHash());
             for (unsigned int i = 0; i < orphanTx.vout.size(); i++) {
                 auto it_by_prev = mapOrphanTransactionsByPrev.find(COutPoint(orphanHash, i));
                 if (it_by_prev != mapOrphanTransactionsByPrev.end()) {
@@ -3578,7 +3579,7 @@ void PeerManagerImpl::ProcessMessage(
             }
 
             m_mempool.check(m_chainman.ActiveChainstate());
-            RelayTransaction(tx.GetHash(), m_connman);
+            RelayTransaction(tx.GetHash());
 
             for (unsigned int i = 0; i < tx.vout.size(); i++) {
                 auto it_by_prev = mapOrphanTransactionsByPrev.find(COutPoint(inv.hash, i));
@@ -3650,7 +3651,7 @@ void PeerManagerImpl::ProcessMessage(
                     LogPrintf("Not relaying non-mempool transaction %s from forcerelay peer=%d\n", tx.GetHash().ToString(), pfrom.GetId());
                 } else {
                     LogPrintf("Force relaying tx %s from peer=%d\n", tx.GetHash().ToString(), pfrom.GetId());
-                    RelayTransaction(tx.GetHash(), m_connman);
+                    RelayTransaction(tx.GetHash());
                 }
             }
         }

src/net_processing.h

@@ -52,6 +52,10 @@ public:
     /** Whether this node ignores txs received over p2p. */
     virtual bool IgnoresIncomingTxs() = 0;
 
+    /** Relay transaction to all peers. */
+    virtual void RelayTransaction(const uint256& txid)
+        EXCLUSIVE_LOCKS_REQUIRED(cs_main) = 0;
+
     /** Set the best height */
     virtual void SetBestHeight(int height) = 0;
 
@@ -77,7 +81,5 @@ void EraseObjectRequest(NodeId nodeId, const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED
 void RequestObject(NodeId nodeId, const CInv& inv, std::chrono::microseconds current_time, bool fForce=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 size_t GetRequestedObjectCount(NodeId nodeId) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
-/** Relay transaction to every node */
-void RelayTransaction(const uint256&, const CConnman& connman);
 
 #endif // BITCOIN_NET_PROCESSING_H

src/node/transaction.cpp

@@ -19,9 +19,9 @@
 TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback, bool bypass_limits)
 {
     // BroadcastTransaction can be called by either sendrawtransaction RPC or wallet RPCs.
-    // g_connman is assigned both before chain clients and before RPC server is accepting calls,
-    // and reset after chain clients and RPC sever are stopped. g_connman should never be null here.
-    assert(node.connman);
+    // node.peerman is assigned both before chain clients and before RPC server is accepting calls,
+    // and reset after chain clients and RPC sever are stopped. node.peerman should never be null here.
+    assert(node.peerman);
     assert(node.mempool);
     std::promise<void> promise;
     uint256 hashTx = tx->GetHash();
@@ -87,7 +87,8 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
         // best-effort of initial broadcast
         node.mempool->AddUnbroadcastTx(hashTx);
 
-        RelayTransaction(hashTx, *node.connman);
+        LOCK(cs_main);
+        node.peerman->RelayTransaction(hashTx);
     }
 
     return TransactionError::OK;