From fa9b5f4fe32c0cfe2e477bb11912756f84a52cfe Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 16 Nov 2023 15:43:15 +0100 Subject: [PATCH] refactor: NetMsg::Make() without nVersion The nVersion field is unused, so remove it. This is also required for future commits. Also, add PushMessage aliases in PeerManagerImpl to make calling code less verbose. Co-Authored-By: Anthony Towns --- src/net_processing.cpp | 149 ++++++++---------- src/netmessagemaker.h | 10 +- src/test/fuzz/p2p_transport_serialization.cpp | 2 +- src/test/net_tests.cpp | 5 +- src/test/util/net.cpp | 5 +- 5 files changed, 74 insertions(+), 97 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 1556dc7e67..0cd20680b0 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -51,6 +51,7 @@ #include #include #include +#include /** Headers download timeout. * Timeout = base + per_header * (expected number of headers) */ @@ -669,6 +670,14 @@ private: void AddTxAnnouncement(const CNode& node, const GenTxid& gtxid, std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + /** Send a message to a peer */ + void PushMessage(CNode& node, CSerializedNetMsg&& msg) const { m_connman.PushMessage(&node, std::move(msg)); } + template + void MakeAndPushMessage(CNode& node, std::string msg_type, Args&&... args) const + { + m_connman.PushMessage(&node, NetMsg::Make(std::move(msg_type), std::forward(args)...)); + } + /** Send a version message to a peer */ void PushNodeVersion(CNode& pnode, const Peer& peer); @@ -1277,14 +1286,14 @@ void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid) // As per BIP152, we only get 3 of our peers to announce // blocks using compact encodings. m_connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [this](CNode* pnodeStop){ - m_connman.PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION)); + MakeAndPushMessage(*pnodeStop, NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION); // save BIP152 bandwidth state: we select peer to be low-bandwidth pnodeStop->m_bip152_highbandwidth_to = false; return true; }); lNodesAnnouncingHeaderAndIDs.pop_front(); } - m_connman.PushMessage(pfrom, CNetMsgMaker(pfrom->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /*high_bandwidth=*/true, /*version=*/CMPCTBLOCKS_VERSION)); + MakeAndPushMessage(*pfrom, NetMsgType::SENDCMPCT, /*high_bandwidth=*/true, /*version=*/CMPCTBLOCKS_VERSION); // save BIP152 bandwidth state: we select peer to be high-bandwidth pfrom->m_bip152_highbandwidth_to = true; lNodesAnnouncingHeaderAndIDs.push_back(pfrom->GetId()); @@ -1487,10 +1496,10 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer) uint64_t your_services{addr.nServices}; const bool tx_relay{!RejectIncomingTxs(pnode)}; - m_connman.PushMessage(&pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, PROTOCOL_VERSION, my_services, nTime, + MakeAndPushMessage(pnode, NetMsgType::VERSION, PROTOCOL_VERSION, my_services, nTime, your_services, CNetAddr::V1(addr_you), // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime) my_services, CNetAddr::V1(CService{}), // Together the pre-version-31402 serialization of CAddress "addrMe" (without nTime) - nonce, strSubVersion, nNodeStartingHeight, tx_relay)); + nonce, strSubVersion, nNodeStartingHeight, tx_relay); if (fLogIPs) { LogPrint(BCLog::NET, "send version message: version %d, blocks=%d, them=%s, txrelay=%d, peer=%d\n", PROTOCOL_VERSION, nNodeStartingHeight, addr_you.ToStringAddrPort(), tx_relay, nodeid); @@ -1861,8 +1870,7 @@ std::optional PeerManagerImpl::FetchBlock(NodeId peer_id, const CBl // Send block request message to the peer bool success = m_connman.ForNode(peer_id, [this, &invs](CNode* node) { - const CNetMsgMaker msgMaker(node->GetCommonVersion()); - this->m_connman.PushMessage(node, msgMaker.Make(NetMsgType::GETDATA, invs)); + this->MakeAndPushMessage(*node, NetMsgType::GETDATA, invs); return true; }); @@ -1985,7 +1993,6 @@ void PeerManagerImpl::BlockDisconnected(const std::shared_ptr &blo void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock) { auto pcmpctblock = std::make_shared(*pblock); - const CNetMsgMaker msgMaker(PROTOCOL_VERSION); LOCK(cs_main); @@ -1997,7 +2004,7 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha uint256 hashBlock(pblock->GetHash()); const std::shared_future lazy_ser{ - std::async(std::launch::deferred, [&] { return msgMaker.Make(NetMsgType::CMPCTBLOCK, *pcmpctblock); })}; + std::async(std::launch::deferred, [&] { return NetMsg::Make(NetMsgType::CMPCTBLOCK, *pcmpctblock); })}; { auto most_recent_block_txs = std::make_unique>(); @@ -2028,7 +2035,7 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha hashBlock.ToString(), pnode->GetId()); const CSerializedNetMsg& ser_cmpctblock{lazy_ser.get()}; - m_connman.PushMessage(pnode, ser_cmpctblock.Copy()); + PushMessage(*pnode, ser_cmpctblock.Copy()); state.pindexBestHeaderSent = pindex; } }); @@ -2262,7 +2269,6 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& LogPrint(BCLog::NET, "%s: ignoring request from peer=%i for old block that isn't in the main chain\n", __func__, pfrom.GetId()); return; } - const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); // disconnect node in case we have reached the outbound limit for serving historical blocks if (m_connman.OutboundTargetReached(true) && (((m_chainman.m_best_header != nullptr) && (m_chainman.m_best_header->GetBlockTime() - pindex->GetBlockTime() > HISTORICAL_BLOCK_AGE)) || inv.IsMsgFilteredBlk()) && @@ -2296,7 +2302,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& if (!m_chainman.m_blockman.ReadRawBlockFromDisk(block_data, pindex->GetBlockPos())) { assert(!"cannot load block from disk"); } - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, Span{block_data})); + MakeAndPushMessage(pfrom, NetMsgType::BLOCK, Span{block_data}); // Don't set pblock as we've sent the block } else { // Send block from disk @@ -2308,9 +2314,9 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& } if (pblock) { if (inv.IsMsgBlk()) { - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, TX_NO_WITNESS(*pblock))); + MakeAndPushMessage(pfrom, NetMsgType::BLOCK, TX_NO_WITNESS(*pblock)); } else if (inv.IsMsgWitnessBlk()) { - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, TX_WITH_WITNESS(*pblock))); + MakeAndPushMessage(pfrom, NetMsgType::BLOCK, TX_WITH_WITNESS(*pblock)); } else if (inv.IsMsgFilteredBlk()) { bool sendMerkleBlock = false; CMerkleBlock merkleBlock; @@ -2322,7 +2328,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& } } if (sendMerkleBlock) { - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::MERKLEBLOCK, merkleBlock)); + MakeAndPushMessage(pfrom, NetMsgType::MERKLEBLOCK, merkleBlock); // CMerkleBlock just contains hashes, so also push any transactions in the block the client did not see // This avoids hurting performance by pointlessly requiring a round-trip // Note that there is currently no way for a node to request any single transactions we didn't send here - @@ -2331,7 +2337,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& // however we MUST always provide at least what the remote peer needs typedef std::pair PairType; for (PairType& pair : merkleBlock.vMatchedTxn) - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::TX, TX_NO_WITNESS(*pblock->vtx[pair.first]))); + MakeAndPushMessage(pfrom, NetMsgType::TX, TX_NO_WITNESS(*pblock->vtx[pair.first])); } // else // no response @@ -2342,13 +2348,13 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& // instead we respond with the full, non-compact block. if (CanDirectFetch() && pindex->nHeight >= m_chainman.ActiveChain().Height() - MAX_CMPCTBLOCK_DEPTH) { if (a_recent_compact_block && a_recent_compact_block->header.GetHash() == pindex->GetBlockHash()) { - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::CMPCTBLOCK, *a_recent_compact_block)); + MakeAndPushMessage(pfrom, NetMsgType::CMPCTBLOCK, *a_recent_compact_block); } else { CBlockHeaderAndShortTxIDs cmpctblock{*pblock}; - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::CMPCTBLOCK, cmpctblock)); + MakeAndPushMessage(pfrom, NetMsgType::CMPCTBLOCK, cmpctblock); } } else { - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, TX_WITH_WITNESS(*pblock))); + MakeAndPushMessage(pfrom, NetMsgType::BLOCK, TX_WITH_WITNESS(*pblock)); } } } @@ -2362,7 +2368,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& // wait for other stuff first. std::vector vInv; vInv.emplace_back(MSG_BLOCK, m_chainman.ActiveChain().Tip()->GetBlockHash()); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::INV, vInv)); + MakeAndPushMessage(pfrom, NetMsgType::INV, vInv); peer.m_continuation_block.SetNull(); } } @@ -2396,7 +2402,6 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic std::deque::iterator it = peer.m_getdata_requests.begin(); std::vector vNotFound; - const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); // Process as many TX items from the front of the getdata queue as // possible, since they're common and it's efficient to batch process @@ -2419,7 +2424,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic if (tx) { // WTX and WITNESS_TX imply we serialize with witness const auto maybe_with_witness = (inv.IsMsgTx() ? TX_NO_WITNESS : TX_WITH_WITNESS); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::TX, maybe_with_witness(*tx))); + MakeAndPushMessage(pfrom, NetMsgType::TX, maybe_with_witness(*tx)); m_mempool.RemoveUnbroadcastTx(tx->GetHash()); } else { vNotFound.push_back(inv); @@ -2454,7 +2459,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic // In normal operation, we often send NOTFOUND messages for parents of // transactions that we relay; if a peer is missing a parent, they may // assume we have them and request the parents from us. - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::NOTFOUND, vNotFound)); + MakeAndPushMessage(pfrom, NetMsgType::NOTFOUND, vNotFound); } } @@ -2478,8 +2483,7 @@ void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlo resp.txn[i] = block.vtx[req.indexes[i]]; } - const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCKTXN, resp)); + MakeAndPushMessage(pfrom, NetMsgType::BLOCKTXN, resp); } bool PeerManagerImpl::CheckHeadersPoW(const std::vector& headers, const Consensus::Params& consensusParams, Peer& peer) @@ -2707,14 +2711,12 @@ bool PeerManagerImpl::IsAncestorOfBestHeaderOrTip(const CBlockIndex* header) bool PeerManagerImpl::MaybeSendGetHeaders(CNode& pfrom, const CBlockLocator& locator, Peer& peer) { - const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); - const auto current_time = NodeClock::now(); // Only allow a new getheaders message to go out if we don't have a recent // one already in-flight if (current_time - peer.m_last_getheaders_timestamp > HEADERS_RESPONSE_TIME) { - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETHEADERS, locator, uint256())); + MakeAndPushMessage(pfrom, NetMsgType::GETHEADERS, locator, uint256()); peer.m_last_getheaders_timestamp = current_time; return true; } @@ -2728,8 +2730,6 @@ bool PeerManagerImpl::MaybeSendGetHeaders(CNode& pfrom, const CBlockLocator& loc */ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, const CBlockIndex& last_header) { - const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); - LOCK(cs_main); CNodeState *nodestate = State(pfrom.GetId()); @@ -2782,7 +2782,7 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, c // In any case, we want to download using a compact block, not a regular one vGetData[0] = CInv(MSG_CMPCT_BLOCK, vGetData[0].hash); } - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vGetData)); + MakeAndPushMessage(pfrom, NetMsgType::GETDATA, vGetData); } } } @@ -3155,9 +3155,7 @@ void PeerManagerImpl::ProcessGetCFilters(CNode& node,Peer& peer, CDataStream& vR } for (const auto& filter : filters) { - CSerializedNetMsg msg = CNetMsgMaker(node.GetCommonVersion()) - .Make(NetMsgType::CFILTER, filter); - m_connman.PushMessage(&node, std::move(msg)); + MakeAndPushMessage(node, NetMsgType::CFILTER, filter); } } @@ -3196,13 +3194,11 @@ void PeerManagerImpl::ProcessGetCFHeaders(CNode& node, Peer& peer, CDataStream& return; } - CSerializedNetMsg msg = CNetMsgMaker(node.GetCommonVersion()) - .Make(NetMsgType::CFHEADERS, + MakeAndPushMessage(node, NetMsgType::CFHEADERS, filter_type_ser, stop_index->GetBlockHash(), prev_header, filter_hashes); - m_connman.PushMessage(&node, std::move(msg)); } void PeerManagerImpl::ProcessGetCFCheckPt(CNode& node, Peer& peer, CDataStream& vRecv) @@ -3237,12 +3233,10 @@ void PeerManagerImpl::ProcessGetCFCheckPt(CNode& node, Peer& peer, CDataStream& } } - CSerializedNetMsg msg = CNetMsgMaker(node.GetCommonVersion()) - .Make(NetMsgType::CFCHECKPT, + MakeAndPushMessage(node, NetMsgType::CFCHECKPT, filter_type_ser, stop_index->GetBlockHash(), headers); - m_connman.PushMessage(&node, std::move(msg)); } void PeerManagerImpl::ProcessBlock(CNode& node, const std::shared_ptr& block, bool force_processing, bool min_pow_checked) @@ -3266,7 +3260,6 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl { std::shared_ptr pblock = std::make_shared(); bool fBlockRead{false}; - const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); { LOCK(cs_main); @@ -3302,7 +3295,7 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl // Might have collided, fall back to getdata now :( std::vector invs; invs.emplace_back(MSG_BLOCK | GetFetchFlags(peer), block_transactions.blockhash); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, invs)); + MakeAndPushMessage(pfrom, NetMsgType::GETDATA, invs); } else { RemoveBlockRequest(block_transactions.blockhash, pfrom.GetId()); LogPrint(BCLog::NET, "Peer %d sent us a compact block but it failed to reconstruct, waiting on first download to complete\n", pfrom.GetId()); @@ -3441,10 +3434,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, pfrom.SetCommonVersion(greatest_common_version); pfrom.nVersion = nVersion; - const CNetMsgMaker msg_maker(greatest_common_version); - if (greatest_common_version >= WTXID_RELAY_VERSION) { - m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::WTXIDRELAY)); + MakeAndPushMessage(pfrom, NetMsgType::WTXIDRELAY); } // Signal ADDRv2 support (BIP155). @@ -3453,7 +3444,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // implementations reject messages they don't know. As a courtesy, don't send // it to nodes with a version before 70016, as no software is known to support // BIP155 that doesn't announce at least that protocol version number. - m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::SENDADDRV2)); + MakeAndPushMessage(pfrom, NetMsgType::SENDADDRV2); } pfrom.m_has_all_wanted_services = HasAllDesirableServiceFlags(nServices); @@ -3493,12 +3484,12 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if (tx_relay && WITH_LOCK(tx_relay->m_bloom_filter_mutex, return tx_relay->m_relay_txs) && !pfrom.IsAddrFetchConn() && !m_opts.ignore_incoming_txs) { const uint64_t recon_salt = m_txreconciliation->PreRegisterPeer(pfrom.GetId()); - m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::SENDTXRCNCL, - TXRECONCILIATION_VERSION, recon_salt)); + MakeAndPushMessage(pfrom, NetMsgType::SENDTXRCNCL, + TXRECONCILIATION_VERSION, recon_salt); } } - m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::VERACK)); + MakeAndPushMessage(pfrom, NetMsgType::VERACK); // Potentially mark this peer as a preferred download peer. { @@ -3522,7 +3513,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // We skip this for block-relay-only peers. We want to avoid // potentially leaking addr information and we do not want to // indicate to the peer that we will participate in addr relay. - m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make(NetMsgType::GETADDR)); + MakeAndPushMessage(pfrom, NetMsgType::GETADDR); peer->m_getaddr_sent = true; // When requesting a getaddr, accept an additional MAX_ADDR_TO_SEND addresses in response // (bypassing the MAX_ADDR_PROCESSING_TOKEN_BUCKET limit). @@ -3568,7 +3559,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // If the peer is old enough to have the old alert system, send it the final alert. if (greatest_common_version <= 70012) { const auto finalAlert{ParseHex("60010000000000000000000000ffffff7f00000000ffffff7ffeffff7f01ffffff7f00000000ffffff7f00ffffff7f002f555247454e543a20416c657274206b657920636f6d70726f6d697365642c2075706772616465207265717569726564004630440220653febd6410f470f6bae11cad19c48413becb1ac2c17f908fd0fd53bdc3abd5202206d0e9c96fe88d4a0f01ed9dedae2b6f9e00da94cad0fecaae66ecf689bf71b50")}; - m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make("alert", Span{finalAlert})); + MakeAndPushMessage(pfrom, "alert", Span{finalAlert}); } // Feeler connections exist only to verify if address is online. @@ -3585,9 +3576,6 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, return; } - // At this point, the outgoing message serialization version can't change. - const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); - if (msg_type == NetMsgType::VERACK) { if (pfrom.fSuccessfullyConnected) { LogPrint(BCLog::NET, "ignoring redundant verack message from peer=%d\n", pfrom.GetId()); @@ -3612,7 +3600,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // cmpctblock messages. // We send this to non-NODE NETWORK peers as well, because // they may wish to request compact blocks from us - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION)); + MakeAndPushMessage(pfrom, NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION); } if (m_txreconciliation) { @@ -4119,7 +4107,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, LogPrint(BCLog::NET, "Ignoring getheaders from peer=%d because active chain has too little work; sending empty response\n", pfrom.GetId()); // Just respond with an empty headers message, to tell the peer to // go away but not treat us as unresponsive. - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::HEADERS, std::vector())); + MakeAndPushMessage(pfrom, NetMsgType::HEADERS, std::vector()); return; } @@ -4169,7 +4157,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // will re-announce the new block via headers (or compact blocks again) // in the SendMessages logic. nodestate->pindexBestHeaderSent = pindex ? pindex : m_chainman.ActiveChain().Tip(); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::HEADERS, TX_WITH_WITNESS(vHeaders))); + MakeAndPushMessage(pfrom, NetMsgType::HEADERS, TX_WITH_WITNESS(vHeaders)); return; } @@ -4471,7 +4459,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // so we just grab the block via normal getdata std::vector vInv(1); vInv[0] = CInv(MSG_BLOCK | GetFetchFlags(*peer), blockhash); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vInv)); + MakeAndPushMessage(pfrom, NetMsgType::GETDATA, vInv); } return; } @@ -4508,7 +4496,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // Duplicate txindexes, the block is now in-flight, so just request it std::vector vInv(1); vInv[0] = CInv(MSG_BLOCK | GetFetchFlags(*peer), blockhash); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vInv)); + MakeAndPushMessage(pfrom, NetMsgType::GETDATA, vInv); } else { // Give up for this peer and wait for other peer(s) RemoveBlockRequest(pindex->GetBlockHash(), pfrom.GetId()); @@ -4527,7 +4515,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // We will try to round-trip any compact blocks we get on failure, // as long as it's first... req.blockhash = pindex->GetBlockHash(); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETBLOCKTXN, req)); + MakeAndPushMessage(pfrom, NetMsgType::GETBLOCKTXN, req); } else if (pfrom.m_bip152_highbandwidth_to && (!pfrom.IsInboundConn() || IsBlockRequestedFromOutbound(blockhash) || @@ -4537,7 +4525,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // - we already have an outbound attempt in flight(so we'll take what we can get), or // - it's not the final parallel download slot (which we may reserve for first outbound) req.blockhash = pindex->GetBlockHash(); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETBLOCKTXN, req)); + MakeAndPushMessage(pfrom, NetMsgType::GETBLOCKTXN, req); } else { // Give up for this peer and wait for other peer(s) RemoveBlockRequest(pindex->GetBlockHash(), pfrom.GetId()); @@ -4566,7 +4554,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // mempool will probably be useless - request the block normally std::vector vInv(1); vInv[0] = CInv(MSG_BLOCK | GetFetchFlags(*peer), blockhash); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vInv)); + MakeAndPushMessage(pfrom, NetMsgType::GETDATA, vInv); return; } else { // If this was an announce-cmpctblock, we want the same treatment as a header message @@ -4796,7 +4784,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // it, if the remote node sends a ping once per second and this node takes 5 // seconds to respond to each, the 5th ping the remote sends would appear to // return very quickly. - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::PONG, nonce)); + MakeAndPushMessage(pfrom, NetMsgType::PONG, nonce); } return; } @@ -5297,7 +5285,6 @@ void PeerManagerImpl::MaybeSendPing(CNode& node_to, Peer& peer, std::chrono::mic return; } - const CNetMsgMaker msgMaker(node_to.GetCommonVersion()); bool pingSend = false; if (peer.m_ping_queued) { @@ -5319,11 +5306,11 @@ void PeerManagerImpl::MaybeSendPing(CNode& node_to, Peer& peer, std::chrono::mic peer.m_ping_start = now; if (node_to.GetCommonVersion() > BIP0031_VERSION) { peer.m_ping_nonce_sent = nonce; - m_connman.PushMessage(&node_to, msgMaker.Make(NetMsgType::PING, nonce)); + MakeAndPushMessage(node_to, NetMsgType::PING, nonce); } else { // Peer is too old to support ping command with nonce, pong will never arrive. peer.m_ping_nonce_sent = 0; - m_connman.PushMessage(&node_to, msgMaker.Make(NetMsgType::PING)); + MakeAndPushMessage(node_to, NetMsgType::PING); } } } @@ -5377,11 +5364,10 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::micros // No addr messages to send if (peer.m_addrs_to_send.empty()) return; - CNetMsgMaker mm(node.GetCommonVersion()); if (peer.m_wants_addrv2) { - m_connman.PushMessage(&node, mm.Make(NetMsgType::ADDRV2, CAddress::V2_NETWORK(peer.m_addrs_to_send))); + MakeAndPushMessage(node, NetMsgType::ADDRV2, CAddress::V2_NETWORK(peer.m_addrs_to_send)); } else { - m_connman.PushMessage(&node, mm.Make(NetMsgType::ADDR, CAddress::V1_NETWORK(peer.m_addrs_to_send))); + MakeAndPushMessage(node, NetMsgType::ADDR, CAddress::V1_NETWORK(peer.m_addrs_to_send)); } peer.m_addrs_to_send.clear(); @@ -5406,7 +5392,7 @@ void PeerManagerImpl::MaybeSendSendHeaders(CNode& node, Peer& peer) // We send this to non-NODE NETWORK peers as well, because even // non-NODE NETWORK peers can announce blocks (such as pruning // nodes) - m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(NetMsgType::SENDHEADERS)); + MakeAndPushMessage(node, NetMsgType::SENDHEADERS); peer.m_sent_sendheaders = true; } } @@ -5441,7 +5427,7 @@ void PeerManagerImpl::MaybeSendFeefilter(CNode& pto, Peer& peer, std::chrono::mi // We always have a fee filter of at least the min relay fee filterToSend = std::max(filterToSend, m_mempool.m_min_relay_feerate.GetFeePerK()); if (filterToSend != peer.m_fee_filter_sent) { - m_connman.PushMessage(&pto, CNetMsgMaker(pto.GetCommonVersion()).Make(NetMsgType::FEEFILTER, filterToSend)); + MakeAndPushMessage(pto, NetMsgType::FEEFILTER, filterToSend); peer.m_fee_filter_sent = filterToSend; } peer.m_next_send_feefilter = GetExponentialRand(current_time, AVG_FEEFILTER_BROADCAST_INTERVAL); @@ -5518,9 +5504,6 @@ bool PeerManagerImpl::SendMessages(CNode* pto) if (!pto->fSuccessfullyConnected || pto->fDisconnect) return true; - // If we get here, the outgoing message serialization version is set and can't change. - const CNetMsgMaker msgMaker(pto->GetCommonVersion()); - const auto current_time{GetTime()}; if (pto->IsAddrFetchConn() && current_time - pto->m_connected > 10 * AVG_ADDRESS_BROADCAST_INTERVAL) { @@ -5675,17 +5658,17 @@ bool PeerManagerImpl::SendMessages(CNode* pto) { LOCK(m_most_recent_block_mutex); if (m_most_recent_block_hash == pBestIndex->GetBlockHash()) { - cached_cmpctblock_msg = msgMaker.Make(NetMsgType::CMPCTBLOCK, *m_most_recent_compact_block); + cached_cmpctblock_msg = NetMsg::Make(NetMsgType::CMPCTBLOCK, *m_most_recent_compact_block); } } if (cached_cmpctblock_msg.has_value()) { - m_connman.PushMessage(pto, std::move(cached_cmpctblock_msg.value())); + PushMessage(*pto, std::move(cached_cmpctblock_msg.value())); } else { CBlock block; const bool ret{m_chainman.m_blockman.ReadBlockFromDisk(block, *pBestIndex)}; assert(ret); CBlockHeaderAndShortTxIDs cmpctblock{block}; - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::CMPCTBLOCK, cmpctblock)); + MakeAndPushMessage(*pto, NetMsgType::CMPCTBLOCK, cmpctblock); } state.pindexBestHeaderSent = pBestIndex; } else if (peer->m_prefers_headers) { @@ -5698,7 +5681,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) LogPrint(BCLog::NET, "%s: sending header %s to peer=%d\n", __func__, vHeaders.front().GetHash().ToString(), pto->GetId()); } - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::HEADERS, TX_WITH_WITNESS(vHeaders))); + MakeAndPushMessage(*pto, NetMsgType::HEADERS, TX_WITH_WITNESS(vHeaders)); state.pindexBestHeaderSent = pBestIndex; } else fRevertToInv = true; @@ -5743,7 +5726,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) for (const uint256& hash : peer->m_blocks_for_inv_relay) { vInv.emplace_back(MSG_BLOCK, hash); if (vInv.size() == MAX_INV_SZ) { - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::INV, vInv)); + MakeAndPushMessage(*pto, NetMsgType::INV, vInv); vInv.clear(); } } @@ -5796,7 +5779,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) tx_relay->m_tx_inventory_known_filter.insert(inv.hash); vInv.push_back(inv); if (vInv.size() == MAX_INV_SZ) { - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::INV, vInv)); + MakeAndPushMessage(*pto, NetMsgType::INV, vInv); vInv.clear(); } } @@ -5848,7 +5831,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) vInv.push_back(inv); nRelayedTransactions++; if (vInv.size() == MAX_INV_SZ) { - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::INV, vInv)); + MakeAndPushMessage(*pto, NetMsgType::INV, vInv); vInv.clear(); } tx_relay->m_tx_inventory_known_filter.insert(hash); @@ -5860,7 +5843,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) } } if (!vInv.empty()) - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::INV, vInv)); + MakeAndPushMessage(*pto, NetMsgType::INV, vInv); // Detect whether we're stalling auto stalling_timeout = m_block_stalling_timeout.load(); @@ -5980,7 +5963,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) gtxid.GetHash().ToString(), pto->GetId()); vGetData.emplace_back(gtxid.IsWtxid() ? MSG_WTX : (MSG_TX | GetFetchFlags(*peer)), gtxid.GetHash()); if (vGetData.size() >= MAX_GETDATA_SZ) { - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::GETDATA, vGetData)); + MakeAndPushMessage(*pto, NetMsgType::GETDATA, vGetData); vGetData.clear(); } m_txrequest.RequestedTx(pto->GetId(), gtxid.GetHash(), current_time + GETDATA_TX_INTERVAL); @@ -5993,7 +5976,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) if (!vGetData.empty()) - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::GETDATA, vGetData)); + MakeAndPushMessage(*pto, NetMsgType::GETDATA, vGetData); } // release cs_main MaybeSendFeefilter(*pto, *peer, current_time); return true; diff --git a/src/netmessagemaker.h b/src/netmessagemaker.h index ede03aa204..d04867e45e 100644 --- a/src/netmessagemaker.h +++ b/src/netmessagemaker.h @@ -9,19 +9,15 @@ #include #include -class CNetMsgMaker -{ -public: - explicit CNetMsgMaker(int /*unused*/) {} - +namespace NetMsg { template - CSerializedNetMsg Make(std::string msg_type, Args&&... args) const + CSerializedNetMsg Make(std::string msg_type, Args&&... args) { CSerializedNetMsg msg; msg.m_type = std::move(msg_type); VectorWriter{msg.data, 0, std::forward(args)...}; return msg; } -}; +} // namespace NetMsg #endif // BITCOIN_NETMESSAGEMAKER_H diff --git a/src/test/fuzz/p2p_transport_serialization.cpp b/src/test/fuzz/p2p_transport_serialization.cpp index 21d8dab536..6af6aa1d18 100644 --- a/src/test/fuzz/p2p_transport_serialization.cpp +++ b/src/test/fuzz/p2p_transport_serialization.cpp @@ -88,7 +88,7 @@ FUZZ_TARGET(p2p_transport_serialization, .init = initialize_p2p_transport_serial assert(msg.m_time == m_time); std::vector header; - auto msg2 = CNetMsgMaker{msg.m_recv.GetVersion()}.Make(msg.m_type, Span{msg.m_recv}); + auto msg2 = NetMsg::Make(msg.m_type, Span{msg.m_recv}); bool queued = send_transport.SetMessageToSend(msg2); assert(queued); std::optional known_more; diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 48e0706a53..3c9227cfc4 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -845,7 +845,6 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message) const uint64_t services{NODE_NETWORK | NODE_WITNESS}; const int64_t time{0}; - const CNetMsgMaker msg_maker{PROTOCOL_VERSION}; // Force ChainstateManager::IsInitialBlockDownload() to return false. // Otherwise PushAddress() isn't called by PeerManager::ProcessMessage(). @@ -858,13 +857,13 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message) std::chrono::microseconds time_received_dummy{0}; const auto msg_version = - msg_maker.Make(NetMsgType::VERSION, PROTOCOL_VERSION, services, time, services, CAddress::V1_NETWORK(peer_us)); + NetMsg::Make(NetMsgType::VERSION, PROTOCOL_VERSION, services, time, services, CAddress::V1_NETWORK(peer_us)); CDataStream msg_version_stream{msg_version.data, SER_NETWORK, PROTOCOL_VERSION}; m_node.peerman->ProcessMessage( peer, NetMsgType::VERSION, msg_version_stream, time_received_dummy, interrupt_dummy); - const auto msg_verack = msg_maker.Make(NetMsgType::VERACK); + const auto msg_verack = NetMsg::Make(NetMsgType::VERACK); CDataStream msg_verack_stream{msg_verack.data, SER_NETWORK, PROTOCOL_VERSION}; // Will set peer.fSuccessfullyConnected to true (necessary in SendMessages()). diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index e0404e33ed..9257a4964a 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -26,13 +26,12 @@ void ConnmanTestMsg::Handshake(CNode& node, { auto& peerman{static_cast(*m_msgproc)}; auto& connman{*this}; - const CNetMsgMaker mm{0}; peerman.InitializeNode(node, local_services); FlushSendBuffer(node); // Drop the version message added by InitializeNode. CSerializedNetMsg msg_version{ - mm.Make(NetMsgType::VERSION, + NetMsg::Make(NetMsgType::VERSION, version, // Using>(remote_services), // int64_t{}, // dummy time @@ -59,7 +58,7 @@ void ConnmanTestMsg::Handshake(CNode& node, assert(statestats.m_relay_txs == (relay_txs && !node.IsBlockOnlyConn())); assert(statestats.their_services == remote_services); if (successfully_connected) { - CSerializedNetMsg msg_verack{mm.Make(NetMsgType::VERACK)}; + CSerializedNetMsg msg_verack{NetMsg::Make(NetMsgType::VERACK)}; (void)connman.ReceiveMsgFrom(node, std::move(msg_verack)); node.fPauseSend = false; connman.ProcessMessagesOnce(node);