From a38a4e8f039dfabfd9435f3a63f1a9b56de086d6 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 27 Jan 2021 22:05:13 +0000 Subject: [PATCH 1/2] [net processing] Move RelayTransaction into PeerManager We don't mark RelayTransaction as const. Even though it doesn't mutate PeerManagerImpl state, it _is_ mutating the internal state of a CNode object, by updating setInventoryTxToSend. In a subsequent commit, that field will be moved to the Peer object, which is owned by PeerMangerImpl. This requires PeerManagerImpl::ReattemptInitialBroadcast() to no longer be const. --- src/net_processing.cpp | 7 ++++--- src/net_processing.h | 7 ++++--- src/node/transaction.cpp | 3 ++- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index af61e7064e..f9aa63b199 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -242,6 +242,7 @@ public: bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) override; bool IgnoresIncomingTxs() override { return m_ignore_incoming_txs; } void SendPings() override; + void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& connman) 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. */ @@ -954,7 +955,7 @@ void PeerManagerImpl::InitializeNode(CNode *pnode) } } -void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler) const +void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler) { std::set unbroadcast_txids = m_mempool.GetUnbroadcastTxs(); @@ -1464,7 +1465,7 @@ void PeerManagerImpl::SendPings() for(auto& it : m_peer_map) it.second->m_ping_queued = true; } -void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& connman) +void PeerManagerImpl::RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& connman) { connman.ForEachNode([&txid, &wtxid](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { AssertLockHeld(::cs_main); diff --git a/src/net_processing.h b/src/net_processing.h index 3b09907443..f47594400c 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -47,6 +47,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, const uint256& wtxid, const CConnman& connman) + EXCLUSIVE_LOCKS_REQUIRED(cs_main) = 0; + /** Send ping message to all peers */ virtual void SendPings() = 0; @@ -71,7 +75,4 @@ public: const std::chrono::microseconds time_received, const std::atomic& interruptMsgProc) = 0; }; -/** Relay transaction to every node */ -void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& connman) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - #endif // BITCOIN_NET_PROCESSING_H diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index 3b3fab7b6b..b01381411a 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -32,6 +32,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t // node.connman is assigned both before chain clients and before RPC server is accepting calls, // and reset after chain clients and RPC sever are stopped. node.connman should never be null here. assert(node.connman); + assert(node.peerman); assert(node.mempool); std::promise promise; uint256 hashTx = tx->GetHash(); @@ -100,7 +101,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t node.mempool->AddUnbroadcastTx(hashTx); LOCK(cs_main); - RelayTransaction(hashTx, tx->GetWitnessHash(), *node.connman); + node.peerman->RelayTransaction(hashTx, tx->GetWitnessHash(), *node.connman); } return TransactionError::OK; From 680eb56d828ce358b4e000c140f5b247ff5e6179 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 27 Jan 2021 22:18:52 +0000 Subject: [PATCH 2/2] [net processing] Don't pass CConnman to RelayTransactions Use the local m_connman instead --- src/net_processing.cpp | 14 +++++++------- src/net_processing.h | 2 +- src/node/transaction.cpp | 7 +++---- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f9aa63b199..107c4da61b 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -242,7 +242,7 @@ public: bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) override; bool IgnoresIncomingTxs() override { return m_ignore_incoming_txs; } void SendPings() override; - void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& connman) override; + void RelayTransaction(const uint256& txid, const uint256& wtxid) 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, @@ -964,7 +964,7 @@ void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler) if (tx != nullptr) { LOCK(cs_main); - RelayTransaction(txid, tx->GetWitnessHash(), m_connman); + RelayTransaction(txid, tx->GetWitnessHash()); } else { m_mempool.RemoveUnbroadcastTx(txid, true); } @@ -1465,9 +1465,9 @@ void PeerManagerImpl::SendPings() for(auto& it : m_peer_map) it.second->m_ping_queued = true; } -void PeerManagerImpl::RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& connman) +void PeerManagerImpl::RelayTransaction(const uint256& txid, const uint256& wtxid) { - connman.ForEachNode([&txid, &wtxid](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + m_connman.ForEachNode([&txid, &wtxid](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { AssertLockHeld(::cs_main); CNodeState* state = State(pnode->GetId()); @@ -2047,7 +2047,7 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) { LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanHash.ToString()); - RelayTransaction(orphanHash, porphanTx->GetWitnessHash(), m_connman); + RelayTransaction(orphanHash, porphanTx->GetWitnessHash()); m_orphanage.AddChildrenToWorkSet(*porphanTx, orphan_work_set); m_orphanage.EraseTx(orphanHash); for (const CTransactionRef& removedTx : result.m_replaced_transactions.value()) { @@ -3051,7 +3051,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, 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(), tx.GetWitnessHash(), m_connman); + RelayTransaction(tx.GetHash(), tx.GetWitnessHash()); } } return; @@ -3066,7 +3066,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // requests for it. m_txrequest.ForgetTxHash(tx.GetHash()); m_txrequest.ForgetTxHash(tx.GetWitnessHash()); - RelayTransaction(tx.GetHash(), tx.GetWitnessHash(), m_connman); + RelayTransaction(tx.GetHash(), tx.GetWitnessHash()); m_orphanage.AddChildrenToWorkSet(tx, peer->m_orphan_work_set); pfrom.nLastTXTime = GetTime(); diff --git a/src/net_processing.h b/src/net_processing.h index f47594400c..c44bae60d7 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -48,7 +48,7 @@ public: virtual bool IgnoresIncomingTxs() = 0; /** Relay transaction to all peers. */ - virtual void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& connman) + virtual void RelayTransaction(const uint256& txid, const uint256& wtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_main) = 0; /** Send ping message to all peers */ diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index b01381411a..a46446f317 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -29,9 +29,8 @@ static TransactionError HandleATMPError(const TxValidationState& state, std::str TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback) { // BroadcastTransaction can be called by either sendrawtransaction RPC or wallet RPCs. - // node.connman is assigned both before chain clients and before RPC server is accepting calls, - // and reset after chain clients and RPC sever are stopped. node.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 promise; @@ -101,7 +100,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t node.mempool->AddUnbroadcastTx(hashTx); LOCK(cs_main); - node.peerman->RelayTransaction(hashTx, tx->GetWitnessHash(), *node.connman); + node.peerman->RelayTransaction(hashTx, tx->GetWitnessHash()); } return TransactionError::OK;