From 824bbd1ffba3df7ffa6f5bfaa31298cd484473b1 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 24 Aug 2020 15:37:13 +0100 Subject: [PATCH 01/10] [move only] Collect all private members of PeerLogicValidation together We don't have a project style for ordering class members, but it always makes sense to have no more than one of each public/protected/private specifier. Also move documentation for MaybeDiscourageAndDisconnect to the header. --- src/net_processing.cpp | 5 ----- src/net_processing.h | 23 +++++++++++++---------- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 15508683ce..6608aa9904 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3795,11 +3795,6 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty return; } -/** Maybe disconnect a peer and discourage future connections from its address. - * - * @param[in] pnode The node to check. - * @return True if the peer was marked for disconnection in this function - */ bool PeerLogicValidation::MaybeDiscourageAndDisconnect(CNode& pnode) { const NodeId peer_id{pnode.GetId()}; diff --git a/src/net_processing.h b/src/net_processing.h index 4f1c52fc17..4ba8d34b68 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -28,15 +28,6 @@ static const bool DEFAULT_PEERBLOCKFILTERS = false; static const int DISCOURAGEMENT_THRESHOLD{100}; class PeerLogicValidation final : public CValidationInterface, public NetEventsInterface { -private: - CConnman& m_connman; - /** Pointer to this node's banman. May be nullptr - check existence before dereferencing. */ - BanMan* const m_banman; - ChainstateManager& m_chainman; - CTxMemPool& m_mempool; - - bool MaybeDiscourageAndDisconnect(CNode& pnode); - public: PeerLogicValidation(CConnman& connman, BanMan* banman, CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool); @@ -92,8 +83,20 @@ public: const std::atomic& interruptMsgProc); private: - int64_t m_stale_tip_check_time; //!< Next time to check for stale tip + /** Maybe disconnect a peer and discourage future connections from its address. + * + * @param[in] pnode The node to check. + * @return True if the peer was marked for disconnection in this function + */ + bool MaybeDiscourageAndDisconnect(CNode& pnode); + CConnman& m_connman; + /** Pointer to this node's banman. May be nullptr - check existence before dereferencing. */ + BanMan* const m_banman; + ChainstateManager& m_chainman; + CTxMemPool& m_mempool; + + int64_t m_stale_tip_check_time; //!< Next time to check for stale tip }; struct CNodeStateStats { From 2297b26b3ce95e935c0ebb8c38dabf19965054a5 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 12 Aug 2020 11:48:28 +0100 Subject: [PATCH 02/10] [net_processing] Pass chainparams to PeerLogicValidation constructor Keep a references to chainparams, rather than calling the global Params() function every time it's needed. This is fine, since globalChainParams does not get updated once it's been set, and it's available at the point of constructing the PeerLogicValidation object. --- src/init.cpp | 2 +- src/net_processing.cpp | 48 ++++++++++++++++-------------- src/net_processing.h | 7 +++-- src/test/denialofservice_tests.cpp | 12 +++++--- src/test/fuzz/process_message.cpp | 3 +- src/test/util/setup_common.cpp | 2 +- 6 files changed, 40 insertions(+), 34 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 633dd8cefc..e496276e10 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1376,7 +1376,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA node.chainman = &g_chainman; ChainstateManager& chainman = *Assert(node.chainman); - node.peer_logic.reset(new PeerLogicValidation(*node.connman, node.banman.get(), *node.scheduler, chainman, *node.mempool)); + node.peer_logic.reset(new PeerLogicValidation(chainparams, *node.connman, node.banman.get(), *node.scheduler, chainman, *node.mempool)); RegisterValidationInterface(node.peer_logic.get()); // sanitize comments per BIP-0014, format user agent and check total size diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6608aa9904..7892287877 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1241,8 +1241,10 @@ static bool BlockRequestAllowed(const CBlockIndex* pindex, const Consensus::Para (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, consensusParams) < STALE_RELAY_AGE_LIMIT); } -PeerLogicValidation::PeerLogicValidation(CConnman& connman, BanMan* banman, CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool) - : m_connman(connman), +PeerLogicValidation::PeerLogicValidation(const CChainParams& chainparams, CConnman& connman, BanMan* banman, + CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool) + : m_chainparams(chainparams), + m_connman(connman), m_banman(banman), m_chainman(chainman), m_mempool(pool), @@ -2340,7 +2342,7 @@ static void ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv, const CChainPar void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv, const std::chrono::microseconds time_received, - const CChainParams& chainparams, const std::atomic& interruptMsgProc) + const std::atomic& interruptMsgProc) { LogPrint(BCLog::NET, "received: %s (%u bytes) peer=%d\n", SanitizeString(msg_type), vRecv.size(), pfrom.GetId()); if (gArgs.IsArgSet("-dropmessagestest") && GetRand(gArgs.GetArg("-dropmessagestest", 0)) == 0) @@ -2772,7 +2774,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty } pfrom.vRecvGetData.insert(pfrom.vRecvGetData.end(), vInv.begin(), vInv.end()); - ProcessGetData(pfrom, chainparams, m_connman, m_mempool, interruptMsgProc); + ProcessGetData(pfrom, m_chainparams, m_connman, m_mempool, interruptMsgProc); return; } @@ -2825,7 +2827,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty } // If pruning, don't inv blocks unless we have on disk and are likely to still have // for some reasonable time window (1 hour) that block relay might require. - const int nPrunedBlocksLikelyToHave = MIN_BLOCKS_TO_KEEP - 3600 / chainparams.GetConsensus().nPowTargetSpacing; + const int nPrunedBlocksLikelyToHave = MIN_BLOCKS_TO_KEEP - 3600 / m_chainparams.GetConsensus().nPowTargetSpacing; if (fPruneMode && (!(pindex->nStatus & BLOCK_HAVE_DATA) || pindex->nHeight <= ::ChainActive().Tip()->nHeight - nPrunedBlocksLikelyToHave)) { LogPrint(BCLog::NET, " getblocks stopping, pruned or too old block at %d %s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); @@ -2886,7 +2888,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty } CBlock block; - bool ret = ReadBlockFromDisk(block, pindex, chainparams.GetConsensus()); + bool ret = ReadBlockFromDisk(block, pindex, m_chainparams.GetConsensus()); assert(ret); SendBlockTransactions(block, req, pfrom, m_connman); @@ -2920,7 +2922,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty return; } - if (!BlockRequestAllowed(pindex, chainparams.GetConsensus())) { + if (!BlockRequestAllowed(pindex, m_chainparams.GetConsensus())) { LogPrint(BCLog::NET, "%s: ignoring request from peer=%i for old block header that isn't in the main chain\n", __func__, pfrom.GetId()); return; } @@ -3198,7 +3200,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty const CBlockIndex *pindex = nullptr; BlockValidationState state; - if (!m_chainman.ProcessNewBlockHeaders({cmpctblock.header}, state, chainparams, &pindex)) { + if (!m_chainman.ProcessNewBlockHeaders({cmpctblock.header}, state, m_chainparams, &pindex)) { if (state.IsInvalid()) { MaybePunishNodeForBlock(pfrom.GetId(), state, /*via_compact_block*/ true, "invalid header via cmpctblock"); return; @@ -3254,10 +3256,10 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty } // If we're not close to tip yet, give up and let parallel block fetch work its magic - if (!fAlreadyInFlight && !CanDirectFetch(chainparams.GetConsensus())) + if (!fAlreadyInFlight && !CanDirectFetch(m_chainparams.GetConsensus())) return; - if (IsWitnessEnabled(pindex->pprev, chainparams.GetConsensus()) && !nodestate->fSupportsDesiredCmpctVersion) { + if (IsWitnessEnabled(pindex->pprev, m_chainparams.GetConsensus()) && !nodestate->fSupportsDesiredCmpctVersion) { // Don't bother trying to process compact blocks from v1 peers // after segwit activates. return; @@ -3341,8 +3343,9 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty } } // cs_main - if (fProcessBLOCKTXN) - return ProcessMessage(pfrom, NetMsgType::BLOCKTXN, blockTxnMsg, time_received, chainparams, interruptMsgProc); + if (fProcessBLOCKTXN) { + return ProcessMessage(pfrom, NetMsgType::BLOCKTXN, blockTxnMsg, time_received, interruptMsgProc); + } if (fRevertToHeaderProcessing) { // Headers received from HB compact block peers are permitted to be @@ -3350,7 +3353,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty // the peer if the header turns out to be for an invalid block. // Note that if a peer tries to build on an invalid chain, that // will be detected and the peer will be disconnected/discouraged. - return ProcessHeadersMessage(pfrom, m_connman, m_chainman, m_mempool, {cmpctblock.header}, chainparams, /*via_compact_block=*/true); + return ProcessHeadersMessage(pfrom, m_connman, m_chainman, m_mempool, {cmpctblock.header}, m_chainparams, /*via_compact_block=*/true); } if (fBlockReconstructed) { @@ -3370,7 +3373,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty // we have a chain with at least nMinimumChainWork), and we ignore // compact blocks with less work than our tip, it is safe to treat // reconstructed compact blocks as having been requested. - m_chainman.ProcessNewBlock(chainparams, pblock, /*fForceProcessing=*/true, &fNewBlock); + m_chainman.ProcessNewBlock(m_chainparams, pblock, /*fForceProcessing=*/true, &fNewBlock); if (fNewBlock) { pfrom.nLastBlockTime = GetTime(); } else { @@ -3460,7 +3463,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty // disk-space attacks), but this should be safe due to the // protections in the compact block handler -- see related comment // in compact block optimistic reconstruction handling. - m_chainman.ProcessNewBlock(chainparams, pblock, /*fForceProcessing=*/true, &fNewBlock); + m_chainman.ProcessNewBlock(m_chainparams, pblock, /*fForceProcessing=*/true, &fNewBlock); if (fNewBlock) { pfrom.nLastBlockTime = GetTime(); } else { @@ -3493,7 +3496,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty ReadCompactSize(vRecv); // ignore tx count; assume it is 0. } - return ProcessHeadersMessage(pfrom, m_connman, m_chainman, m_mempool, headers, chainparams, /*via_compact_block=*/false); + return ProcessHeadersMessage(pfrom, m_connman, m_chainman, m_mempool, headers, m_chainparams, /*via_compact_block=*/false); } if (msg_type == NetMsgType::BLOCK) @@ -3522,7 +3525,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty mapBlockSource.emplace(hash, std::make_pair(pfrom.GetId(), true)); } bool fNewBlock = false; - m_chainman.ProcessNewBlock(chainparams, pblock, forceProcessing, &fNewBlock); + m_chainman.ProcessNewBlock(m_chainparams, pblock, forceProcessing, &fNewBlock); if (fNewBlock) { pfrom.nLastBlockTime = GetTime(); } else { @@ -3751,17 +3754,17 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty } if (msg_type == NetMsgType::GETCFILTERS) { - ProcessGetCFilters(pfrom, vRecv, chainparams, m_connman); + ProcessGetCFilters(pfrom, vRecv, m_chainparams, m_connman); return; } if (msg_type == NetMsgType::GETCFHEADERS) { - ProcessGetCFHeaders(pfrom, vRecv, chainparams, m_connman); + ProcessGetCFHeaders(pfrom, vRecv, m_chainparams, m_connman); return; } if (msg_type == NetMsgType::GETCFCHECKPT) { - ProcessGetCFCheckPt(pfrom, vRecv, chainparams, m_connman); + ProcessGetCFCheckPt(pfrom, vRecv, m_chainparams, m_connman); return; } @@ -3839,7 +3842,6 @@ bool PeerLogicValidation::MaybeDiscourageAndDisconnect(CNode& pnode) bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic& interruptMsgProc) { - const CChainParams& chainparams = Params(); // // Message format // (4) message start @@ -3851,7 +3853,7 @@ bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic& inter bool fMoreWork = false; if (!pfrom->vRecvGetData.empty()) - ProcessGetData(*pfrom, chainparams, m_connman, m_mempool, interruptMsgProc); + ProcessGetData(*pfrom, m_chainparams, m_connman, m_mempool, interruptMsgProc); if (!pfrom->orphan_work_set.empty()) { std::list removed_txn; @@ -3916,7 +3918,7 @@ bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic& inter } try { - ProcessMessage(*pfrom, msg_type, vRecv, msg.m_time, chainparams, interruptMsgProc); + ProcessMessage(*pfrom, msg_type, vRecv, msg.m_time, interruptMsgProc); if (interruptMsgProc) return false; if (!pfrom->vRecvGetData.empty()) diff --git a/src/net_processing.h b/src/net_processing.h index 4ba8d34b68..65fabcca3d 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -29,7 +29,8 @@ static const int DISCOURAGEMENT_THRESHOLD{100}; class PeerLogicValidation final : public CValidationInterface, public NetEventsInterface { public: - PeerLogicValidation(CConnman& connman, BanMan* banman, CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool); + PeerLogicValidation(const CChainParams& chainparams, CConnman& connman, BanMan* banman, + CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool); /** * Overridden from CValidationInterface. @@ -79,8 +80,7 @@ public: /** Process a single message from a peer. Public for fuzz testing */ void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv, - const std::chrono::microseconds time_received, const CChainParams& chainparams, - const std::atomic& interruptMsgProc); + const std::chrono::microseconds time_received, const std::atomic& interruptMsgProc); private: /** Maybe disconnect a peer and discourage future connections from its address. @@ -90,6 +90,7 @@ private: */ bool MaybeDiscourageAndDisconnect(CNode& pnode); + const CChainParams& m_chainparams; CConnman& m_connman; /** Pointer to this node's banman. May be nullptr - check existence before dereferencing. */ BanMan* const m_banman; diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index bf0659587c..8918da6360 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -79,8 +79,9 @@ BOOST_FIXTURE_TEST_SUITE(denialofservice_tests, TestingSetup) // work. BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) { + const CChainParams& chainparams = Params(); auto connman = MakeUnique(0x1337, 0x1337); - auto peerLogic = MakeUnique(*connman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool); + auto peerLogic = MakeUnique(chainparams, *connman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool); // Mock an outbound peer CAddress addr1(ip(0xa0b0c001), NODE_NONE); @@ -149,8 +150,9 @@ static void AddRandomOutboundPeer(std::vector &vNodes, PeerLogicValidat BOOST_AUTO_TEST_CASE(stale_tip_peer_management) { + const CChainParams& chainparams = Params(); auto connman = MakeUnique(0x1337, 0x1337); - auto peerLogic = MakeUnique(*connman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool); + auto peerLogic = MakeUnique(chainparams, *connman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool); const Consensus::Params& consensusParams = Params().GetConsensus(); constexpr int max_outbound_full_relay = MAX_OUTBOUND_FULL_RELAY_CONNECTIONS; @@ -221,9 +223,10 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management) BOOST_AUTO_TEST_CASE(peer_discouragement) { + const CChainParams& chainparams = Params(); auto banman = MakeUnique(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME); auto connman = MakeUnique(0x1337, 0x1337); - auto peerLogic = MakeUnique(*connman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool); + auto peerLogic = MakeUnique(chainparams, *connman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool); banman->ClearBanned(); CAddress addr1(ip(0xa0b0c001), NODE_NONE); @@ -268,9 +271,10 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) BOOST_AUTO_TEST_CASE(DoS_bantime) { + const CChainParams& chainparams = Params(); auto banman = MakeUnique(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME); auto connman = MakeUnique(0x1337, 0x1337); - auto peerLogic = MakeUnique(*connman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool); + auto peerLogic = MakeUnique(chainparams, *connman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool); banman->ClearBanned(); int64_t nStartTime = GetTime(); diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp index 52efe5ddfa..f94a3310d2 100644 --- a/src/test/fuzz/process_message.cpp +++ b/src/test/fuzz/process_message.cpp @@ -76,8 +76,7 @@ void test_one_input(const std::vector& buffer) g_setup->m_node.peer_logic->InitializeNode(&p2p_node); try { g_setup->m_node.peer_logic->ProcessMessage(p2p_node, random_message_type, random_bytes_data_stream, - GetTime(), Params(), - std::atomic{false}); + GetTime(), std::atomic{false}); } catch (const std::ios_base::failure&) { } SyncWithValidationInterfaceQueue(); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 536a131313..a9752f8887 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -169,7 +169,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME); m_node.connman = MakeUnique(0x1337, 0x1337); // Deterministic randomness for tests. - m_node.peer_logic = MakeUnique(*m_node.connman, m_node.banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool); + m_node.peer_logic = MakeUnique(chainparams, *m_node.connman, m_node.banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool); { CConnman::Options options; options.m_msgproc = m_node.peer_logic.get(); From 58bd369b0ddd3383f7bdf7840912d18b96545f91 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Sat, 29 Aug 2020 10:31:11 +0100 Subject: [PATCH 03/10] scripted-diff: [net processing] Rename PeerLogicValidation to PeerManager -BEGIN VERIFY SCRIPT- sed -i 's/PeerLogicValidation/PeerManager/g' $(git grep -l PeerLogicValidation ./src ./test) sed -i 's/peer_logic/peerman/g' $(git grep -l peer_logic ./src ./test) -END VERIFY SCRIPT- PeerLogicValidation was originally net_processing's implementation to the validation interface. It has since grown to contain much of net_processing's logic. Therefore rename it to reflect its responsibilities. Suggested in https://github.com/bitcoin/bitcoin/pull/10756#pullrequestreview-53892618. --- src/init.cpp | 10 ++++----- src/net_processing.cpp | 34 +++++++++++++++--------------- src/net_processing.h | 4 ++-- src/node/context.h | 4 ++-- src/test/denialofservice_tests.cpp | 10 ++++----- src/test/fuzz/process_message.cpp | 4 ++-- src/test/fuzz/process_messages.cpp | 2 +- src/test/util/setup_common.cpp | 4 ++-- test/sanitizer_suppressions/tsan | 2 +- 9 files changed, 37 insertions(+), 37 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index e496276e10..7dceef7fff 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -200,7 +200,7 @@ void Shutdown(NodeContext& node) // Because these depend on each-other, we make sure that neither can be // using the other before destroying them. - if (node.peer_logic) UnregisterValidationInterface(node.peer_logic.get()); + if (node.peerman) UnregisterValidationInterface(node.peerman.get()); // Follow the lock order requirements: // * CheckForStaleTipAndEvictPeers locks cs_main before indirectly calling GetExtraOutboundCount // which locks cs_vNodes. @@ -227,7 +227,7 @@ void Shutdown(NodeContext& node) // After the threads that potentially access these pointers have been stopped, // destruct and reset all to nullptr. - node.peer_logic.reset(); + node.peerman.reset(); node.connman.reset(); node.banman.reset(); @@ -1376,8 +1376,8 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA node.chainman = &g_chainman; ChainstateManager& chainman = *Assert(node.chainman); - node.peer_logic.reset(new PeerLogicValidation(chainparams, *node.connman, node.banman.get(), *node.scheduler, chainman, *node.mempool)); - RegisterValidationInterface(node.peer_logic.get()); + node.peerman.reset(new PeerManager(chainparams, *node.connman, node.banman.get(), *node.scheduler, chainman, *node.mempool)); + RegisterValidationInterface(node.peerman.get()); // sanitize comments per BIP-0014, format user agent and check total size std::vector uacomments; @@ -1911,7 +1911,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA connOptions.nBestHeight = chain_active_height; connOptions.uiInterface = &uiInterface; connOptions.m_banman = node.banman.get(); - connOptions.m_msgproc = node.peer_logic.get(); + connOptions.m_msgproc = node.peerman.get(); connOptions.nSendBufferMaxSize = 1000 * args.GetArg("-maxsendbuffer", DEFAULT_MAXSENDBUFFER); connOptions.nReceiveFloodSize = 1000 * args.GetArg("-maxreceivebuffer", DEFAULT_MAXRECEIVEBUFFER); connOptions.m_added_nodes = args.GetArgs("-addnode"); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 7892287877..c92e841bc2 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -869,7 +869,7 @@ void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) if (state) state->m_last_block_announcement = time_in_seconds; } -void PeerLogicValidation::InitializeNode(CNode *pnode) { +void PeerManager::InitializeNode(CNode *pnode) { CAddress addr = pnode->addr; std::string addrName = pnode->GetAddrName(); NodeId nodeid = pnode->GetId(); @@ -887,7 +887,7 @@ void PeerLogicValidation::InitializeNode(CNode *pnode) { } } -void PeerLogicValidation::ReattemptInitialBroadcast(CScheduler& scheduler) const +void PeerManager::ReattemptInitialBroadcast(CScheduler& scheduler) const { std::map unbroadcast_txids = m_mempool.GetUnbroadcastTxs(); @@ -907,7 +907,7 @@ void PeerLogicValidation::ReattemptInitialBroadcast(CScheduler& scheduler) const scheduler.scheduleFromNow([&] { ReattemptInitialBroadcast(scheduler); }, delta); } -void PeerLogicValidation::FinalizeNode(NodeId nodeid, bool& fUpdateConnectionTime) { +void PeerManager::FinalizeNode(NodeId nodeid, bool& fUpdateConnectionTime) { fUpdateConnectionTime = false; LOCK(cs_main); int misbehavior{0}; @@ -1241,7 +1241,7 @@ static bool BlockRequestAllowed(const CBlockIndex* pindex, const Consensus::Para (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, consensusParams) < STALE_RELAY_AGE_LIMIT); } -PeerLogicValidation::PeerLogicValidation(const CChainParams& chainparams, CConnman& connman, BanMan* banman, +PeerManager::PeerManager(const CChainParams& chainparams, CConnman& connman, BanMan* banman, CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool) : m_chainparams(chainparams), m_connman(connman), @@ -1281,7 +1281,7 @@ PeerLogicValidation::PeerLogicValidation(const CChainParams& chainparams, CConnm * Evict orphan txn pool entries (EraseOrphanTx) based on a newly connected * block. Also save the time of the last tip update. */ -void PeerLogicValidation::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindex) +void PeerManager::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindex) { { LOCK(g_cs_orphans); @@ -1325,7 +1325,7 @@ void PeerLogicValidation::BlockConnected(const std::shared_ptr& pb } } -void PeerLogicValidation::BlockDisconnected(const std::shared_ptr &block, const CBlockIndex* pindex) +void PeerManager::BlockDisconnected(const std::shared_ptr &block, const CBlockIndex* pindex) { // To avoid relay problems with transactions that were previously // confirmed, clear our filter of recently confirmed transactions whenever @@ -1350,7 +1350,7 @@ static bool fWitnessesPresentInMostRecentCompactBlock GUARDED_BY(cs_most_recent_ * Maintain state about the best-seen block and fast-announce a compact block * to compatible peers. */ -void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock) { +void PeerManager::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock) { std::shared_ptr pcmpctblock = std::make_shared (*pblock, true); const CNetMsgMaker msgMaker(PROTOCOL_VERSION); @@ -1385,7 +1385,7 @@ void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std: if (state.fPreferHeaderAndIDs && (!fWitnessEnabled || state.fWantsCmpctWitness) && !PeerHasHeader(&state, pindex) && PeerHasHeader(&state, pindex->pprev)) { - LogPrint(BCLog::NET, "%s sending header-and-ids %s to peer=%d\n", "PeerLogicValidation::NewPoWValidBlock", + LogPrint(BCLog::NET, "%s sending header-and-ids %s to peer=%d\n", "PeerManager::NewPoWValidBlock", hashBlock.ToString(), pnode->GetId()); m_connman.PushMessage(pnode, msgMaker.Make(NetMsgType::CMPCTBLOCK, *pcmpctblock)); state.pindexBestHeaderSent = pindex; @@ -1397,7 +1397,7 @@ void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std: * Update our best height and announce any block hashes which weren't previously * in ::ChainActive() to our peers. */ -void PeerLogicValidation::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) { +void PeerManager::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) { const int nNewHeight = pindexNew->nHeight; m_connman.SetBestHeight(nNewHeight); @@ -1432,7 +1432,7 @@ void PeerLogicValidation::UpdatedBlockTip(const CBlockIndex *pindexNew, const CB * Handle invalid block rejection and consequent peer discouragement, maintain which * peers announce compact blocks. */ -void PeerLogicValidation::BlockChecked(const CBlock& block, const BlockValidationState& state) { +void PeerManager::BlockChecked(const CBlock& block, const BlockValidationState& state) { LOCK(cs_main); const uint256 hash(block.GetHash()); @@ -2340,7 +2340,7 @@ static void ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv, const CChainPar connman.PushMessage(&peer, std::move(msg)); } -void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv, +void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv, const std::chrono::microseconds time_received, const std::atomic& interruptMsgProc) { @@ -3798,7 +3798,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty return; } -bool PeerLogicValidation::MaybeDiscourageAndDisconnect(CNode& pnode) +bool PeerManager::MaybeDiscourageAndDisconnect(CNode& pnode) { const NodeId peer_id{pnode.GetId()}; PeerRef peer = GetPeerRef(peer_id); @@ -3840,7 +3840,7 @@ bool PeerLogicValidation::MaybeDiscourageAndDisconnect(CNode& pnode) return true; } -bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic& interruptMsgProc) +bool PeerManager::ProcessMessages(CNode* pfrom, std::atomic& interruptMsgProc) { // // Message format @@ -3932,7 +3932,7 @@ bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic& inter return fMoreWork; } -void PeerLogicValidation::ConsiderEviction(CNode& pto, int64_t time_in_seconds) +void PeerManager::ConsiderEviction(CNode& pto, int64_t time_in_seconds) { AssertLockHeld(cs_main); @@ -3985,7 +3985,7 @@ void PeerLogicValidation::ConsiderEviction(CNode& pto, int64_t time_in_seconds) } } -void PeerLogicValidation::EvictExtraOutboundPeers(int64_t time_in_seconds) +void PeerManager::EvictExtraOutboundPeers(int64_t time_in_seconds) { // Check whether we have too many outbound peers int extra_peers = m_connman.GetExtraOutboundCount(); @@ -4044,7 +4044,7 @@ void PeerLogicValidation::EvictExtraOutboundPeers(int64_t time_in_seconds) } } -void PeerLogicValidation::CheckForStaleTipAndEvictPeers(const Consensus::Params &consensusParams) +void PeerManager::CheckForStaleTipAndEvictPeers(const Consensus::Params &consensusParams) { LOCK(cs_main); @@ -4086,7 +4086,7 @@ public: }; } -bool PeerLogicValidation::SendMessages(CNode* pto) +bool PeerManager::SendMessages(CNode* pto) { const Consensus::Params& consensusParams = Params().GetConsensus(); diff --git a/src/net_processing.h b/src/net_processing.h index 65fabcca3d..873ee34db9 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -27,9 +27,9 @@ static const bool DEFAULT_PEERBLOCKFILTERS = false; /** Threshold for marking a node to be discouraged, e.g. disconnected and added to the discouragement filter. */ static const int DISCOURAGEMENT_THRESHOLD{100}; -class PeerLogicValidation final : public CValidationInterface, public NetEventsInterface { +class PeerManager final : public CValidationInterface, public NetEventsInterface { public: - PeerLogicValidation(const CChainParams& chainparams, CConnman& connman, BanMan* banman, + PeerManager(const CChainParams& chainparams, CConnman& connman, BanMan* banman, CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool); /** diff --git a/src/node/context.h b/src/node/context.h index d9d0750951..3228831ed1 100644 --- a/src/node/context.h +++ b/src/node/context.h @@ -16,7 +16,7 @@ class CConnman; class CScheduler; class CTxMemPool; class ChainstateManager; -class PeerLogicValidation; +class PeerManager; namespace interfaces { class Chain; class ChainClient; @@ -36,7 +36,7 @@ class WalletClient; struct NodeContext { std::unique_ptr connman; std::unique_ptr mempool; - std::unique_ptr peer_logic; + std::unique_ptr peerman; ChainstateManager* chainman{nullptr}; // Currently a raw pointer because the memory is not managed by this struct std::unique_ptr banman; ArgsManager* args{nullptr}; // Currently a raw pointer because the memory is not managed by this struct diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 8918da6360..2ee0d9b7de 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -81,7 +81,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) { const CChainParams& chainparams = Params(); auto connman = MakeUnique(0x1337, 0x1337); - auto peerLogic = MakeUnique(chainparams, *connman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool); + auto peerLogic = MakeUnique(chainparams, *connman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool); // Mock an outbound peer CAddress addr1(ip(0xa0b0c001), NODE_NONE); @@ -134,7 +134,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) peerLogic->FinalizeNode(dummyNode1.GetId(), dummy); } -static void AddRandomOutboundPeer(std::vector &vNodes, PeerLogicValidation &peerLogic, CConnmanTest* connman) +static void AddRandomOutboundPeer(std::vector &vNodes, PeerManager &peerLogic, CConnmanTest* connman) { CAddress addr(ip(g_insecure_rand_ctx.randbits(32)), NODE_NONE); vNodes.emplace_back(new CNode(id++, ServiceFlags(NODE_NETWORK | NODE_WITNESS), 0, INVALID_SOCKET, addr, 0, 0, CAddress(), "", ConnectionType::OUTBOUND_FULL_RELAY)); @@ -152,7 +152,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management) { const CChainParams& chainparams = Params(); auto connman = MakeUnique(0x1337, 0x1337); - auto peerLogic = MakeUnique(chainparams, *connman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool); + auto peerLogic = MakeUnique(chainparams, *connman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool); const Consensus::Params& consensusParams = Params().GetConsensus(); constexpr int max_outbound_full_relay = MAX_OUTBOUND_FULL_RELAY_CONNECTIONS; @@ -226,7 +226,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) const CChainParams& chainparams = Params(); auto banman = MakeUnique(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME); auto connman = MakeUnique(0x1337, 0x1337); - auto peerLogic = MakeUnique(chainparams, *connman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool); + auto peerLogic = MakeUnique(chainparams, *connman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool); banman->ClearBanned(); CAddress addr1(ip(0xa0b0c001), NODE_NONE); @@ -274,7 +274,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) const CChainParams& chainparams = Params(); auto banman = MakeUnique(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME); auto connman = MakeUnique(0x1337, 0x1337); - auto peerLogic = MakeUnique(chainparams, *connman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool); + auto peerLogic = MakeUnique(chainparams, *connman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool); banman->ClearBanned(); int64_t nStartTime = GetTime(); diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp index f94a3310d2..3d6947ca92 100644 --- a/src/test/fuzz/process_message.cpp +++ b/src/test/fuzz/process_message.cpp @@ -73,9 +73,9 @@ void test_one_input(const std::vector& buffer) p2p_node.nVersion = PROTOCOL_VERSION; p2p_node.SetSendVersion(PROTOCOL_VERSION); connman.AddTestNode(p2p_node); - g_setup->m_node.peer_logic->InitializeNode(&p2p_node); + g_setup->m_node.peerman->InitializeNode(&p2p_node); try { - g_setup->m_node.peer_logic->ProcessMessage(p2p_node, random_message_type, random_bytes_data_stream, + g_setup->m_node.peerman->ProcessMessage(p2p_node, random_message_type, random_bytes_data_stream, GetTime(), std::atomic{false}); } catch (const std::ios_base::failure&) { } diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp index 33385c06cf..c9433d325a 100644 --- a/src/test/fuzz/process_messages.cpp +++ b/src/test/fuzz/process_messages.cpp @@ -52,7 +52,7 @@ void test_one_input(const std::vector& buffer) p2p_node.fPauseSend = false; p2p_node.nVersion = PROTOCOL_VERSION; p2p_node.SetSendVersion(PROTOCOL_VERSION); - g_setup->m_node.peer_logic->InitializeNode(&p2p_node); + g_setup->m_node.peerman->InitializeNode(&p2p_node); connman.AddTestNode(p2p_node); } diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index a9752f8887..08aff07448 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -169,10 +169,10 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME); m_node.connman = MakeUnique(0x1337, 0x1337); // Deterministic randomness for tests. - m_node.peer_logic = MakeUnique(chainparams, *m_node.connman, m_node.banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool); + m_node.peerman = MakeUnique(chainparams, *m_node.connman, m_node.banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool); { CConnman::Options options; - options.m_msgproc = m_node.peer_logic.get(); + options.m_msgproc = m_node.peerman.get(); m_node.connman->Init(options); } } diff --git a/test/sanitizer_suppressions/tsan b/test/sanitizer_suppressions/tsan index 3ba2b2a103..625085c55b 100644 --- a/test/sanitizer_suppressions/tsan +++ b/test/sanitizer_suppressions/tsan @@ -11,7 +11,7 @@ mutex:CConnman::ThreadOpenConnections mutex:CConnman::ThreadOpenAddedConnections mutex:CConnman::SocketHandler mutex:UpdateTip -mutex:PeerLogicValidation::UpdatedBlockTip +mutex:PeerManager::UpdatedBlockTip mutex:g_best_block_mutex # race (TODO fix) race:CConnman::WakeMessageHandler From 64f6162651420be2f4aa1498f0378a86780bc089 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Sat, 29 Aug 2020 10:33:30 +0100 Subject: [PATCH 04/10] [whitespace] tidy up indentation after scripted diff --- src/net_processing.cpp | 2 +- src/net_processing.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index c92e841bc2..048cab3e7d 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1242,7 +1242,7 @@ static bool BlockRequestAllowed(const CBlockIndex* pindex, const Consensus::Para } PeerManager::PeerManager(const CChainParams& chainparams, CConnman& connman, BanMan* banman, - CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool) + CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool) : m_chainparams(chainparams), m_connman(connman), m_banman(banman), diff --git a/src/net_processing.h b/src/net_processing.h index 873ee34db9..6ffd6c4fd6 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -30,7 +30,7 @@ static const int DISCOURAGEMENT_THRESHOLD{100}; class PeerManager final : public CValidationInterface, public NetEventsInterface { public: PeerManager(const CChainParams& chainparams, CConnman& connman, BanMan* banman, - CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool); + CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool); /** * Overridden from CValidationInterface. From d7778351bf60a21925a97b7fc4e9df5541b6d995 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 24 Aug 2020 15:51:39 +0100 Subject: [PATCH 05/10] [net processing] Move ProcessHeadersMessage to PeerManager --- src/net_processing.cpp | 20 ++++++++++---------- src/net_processing.h | 4 ++++ 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 048cab3e7d..fac0f100d4 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1856,7 +1856,7 @@ inline void static SendBlockTransactions(const CBlock& block, const BlockTransac connman.PushMessage(&pfrom, msgMaker.Make(nSendFlags, NetMsgType::BLOCKTXN, resp)); } -static void ProcessHeadersMessage(CNode& pfrom, CConnman& connman, ChainstateManager& chainman, CTxMemPool& mempool, const std::vector& headers, const CChainParams& chainparams, bool via_compact_block) +void PeerManager::ProcessHeadersMessage(CNode& pfrom, const std::vector& headers, bool via_compact_block) { const CNetMsgMaker msgMaker(pfrom.GetSendVersion()); size_t nCount = headers.size(); @@ -1882,7 +1882,7 @@ static void ProcessHeadersMessage(CNode& pfrom, CConnman& connman, ChainstateMan // nUnconnectingHeaders gets reset back to 0. if (!LookupBlockIndex(headers[0].hashPrevBlock) && nCount < MAX_BLOCKS_TO_ANNOUNCE) { nodestate->nUnconnectingHeaders++; - connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETHEADERS, ::ChainActive().GetLocator(pindexBestHeader), uint256())); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETHEADERS, ::ChainActive().GetLocator(pindexBestHeader), uint256())); LogPrint(BCLog::NET, "received header %s: missing prev block %s, sending getheaders (%d) to end (peer=%d, nUnconnectingHeaders=%d)\n", headers[0].GetHash().ToString(), headers[0].hashPrevBlock.ToString(), @@ -1916,7 +1916,7 @@ static void ProcessHeadersMessage(CNode& pfrom, CConnman& connman, ChainstateMan } BlockValidationState state; - if (!chainman.ProcessNewBlockHeaders(headers, state, chainparams, &pindexLast)) { + if (!m_chainman.ProcessNewBlockHeaders(headers, state, m_chainparams, &pindexLast)) { if (state.IsInvalid()) { MaybePunishNodeForBlock(pfrom.GetId(), state, via_compact_block, "invalid header received"); return; @@ -1947,10 +1947,10 @@ static void ProcessHeadersMessage(CNode& pfrom, CConnman& connman, ChainstateMan // TODO: optimize: if pindexLast is an ancestor of ::ChainActive().Tip or pindexBestHeader, continue // from there instead. LogPrint(BCLog::NET, "more getheaders (%d) to end to peer=%d (startheight:%d)\n", pindexLast->nHeight, pfrom.GetId(), pfrom.nStartingHeight); - connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETHEADERS, ::ChainActive().GetLocator(pindexLast), uint256())); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETHEADERS, ::ChainActive().GetLocator(pindexLast), uint256())); } - bool fCanDirectFetch = CanDirectFetch(chainparams.GetConsensus()); + bool fCanDirectFetch = CanDirectFetch(m_chainparams.GetConsensus()); // If this set of headers is valid and ends in a block with at least as // much work as our tip, download as much as possible. if (fCanDirectFetch && pindexLast->IsValid(BLOCK_VALID_TREE) && ::ChainActive().Tip()->nChainWork <= pindexLast->nChainWork) { @@ -1960,7 +1960,7 @@ static void ProcessHeadersMessage(CNode& pfrom, CConnman& connman, ChainstateMan while (pindexWalk && !::ChainActive().Contains(pindexWalk) && vToFetch.size() <= MAX_BLOCKS_IN_TRANSIT_PER_PEER) { if (!(pindexWalk->nStatus & BLOCK_HAVE_DATA) && !mapBlocksInFlight.count(pindexWalk->GetBlockHash()) && - (!IsWitnessEnabled(pindexWalk->pprev, chainparams.GetConsensus()) || State(pfrom.GetId())->fHaveWitness)) { + (!IsWitnessEnabled(pindexWalk->pprev, m_chainparams.GetConsensus()) || State(pfrom.GetId())->fHaveWitness)) { // We don't have this block, and it's not yet in flight. vToFetch.push_back(pindexWalk); } @@ -1984,7 +1984,7 @@ static void ProcessHeadersMessage(CNode& pfrom, CConnman& connman, ChainstateMan } uint32_t nFetchFlags = GetFetchFlags(pfrom); vGetData.push_back(CInv(MSG_BLOCK | nFetchFlags, pindex->GetBlockHash())); - MarkBlockAsInFlight(mempool, pfrom.GetId(), pindex->GetBlockHash(), pindex); + MarkBlockAsInFlight(m_mempool, pfrom.GetId(), pindex->GetBlockHash(), pindex); LogPrint(BCLog::NET, "Requesting block %s from peer=%d\n", pindex->GetBlockHash().ToString(), pfrom.GetId()); } @@ -1997,7 +1997,7 @@ static void ProcessHeadersMessage(CNode& pfrom, CConnman& connman, ChainstateMan // In any case, we want to download using a compact block, not a regular one vGetData[0] = CInv(MSG_CMPCT_BLOCK, vGetData[0].hash); } - connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vGetData)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vGetData)); } } } @@ -3353,7 +3353,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat // the peer if the header turns out to be for an invalid block. // Note that if a peer tries to build on an invalid chain, that // will be detected and the peer will be disconnected/discouraged. - return ProcessHeadersMessage(pfrom, m_connman, m_chainman, m_mempool, {cmpctblock.header}, m_chainparams, /*via_compact_block=*/true); + return ProcessHeadersMessage(pfrom, {cmpctblock.header}, /*via_compact_block=*/true); } if (fBlockReconstructed) { @@ -3496,7 +3496,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat ReadCompactSize(vRecv); // ignore tx count; assume it is 0. } - return ProcessHeadersMessage(pfrom, m_connman, m_chainman, m_mempool, headers, m_chainparams, /*via_compact_block=*/false); + return ProcessHeadersMessage(pfrom, headers, /*via_compact_block=*/false); } if (msg_type == NetMsgType::BLOCK) diff --git a/src/net_processing.h b/src/net_processing.h index 6ffd6c4fd6..d4793ce303 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -11,6 +11,7 @@ #include #include +class CBlockHeader; class CChainParams; class CTxMemPool; class ChainstateManager; @@ -90,6 +91,9 @@ private: */ bool MaybeDiscourageAndDisconnect(CNode& pnode); + /** Process a single headers message from a peer. */ + void ProcessHeadersMessage(CNode& pfrom, const std::vector& headers, bool via_compact_block); + const CChainParams& m_chainparams; CConnman& m_connman; /** Pointer to this node's banman. May be nullptr - check existence before dereferencing. */ From b70cd890e375e904b7f36b3d959e5656f5a5cbcd Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 24 Aug 2020 15:56:14 +0100 Subject: [PATCH 06/10] [net processing] Move MaybePunishNodeForBlock into PeerManager --- src/net_processing.cpp | 14 +++----------- src/net_processing.h | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index fac0f100d4..79d1ebe3ab 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1132,17 +1132,9 @@ void Misbehaving(const NodeId pnode, const int howmuch, const std::string& messa } } -/** - * Potentially mark a node discouraged based on the contents of a BlockValidationState object - * - * @param[in] via_compact_block this bool is passed in because net_processing should - * punish peers differently depending on whether the data was provided in a compact - * block message or not. If the compact block had a valid header, but contained invalid - * txs, the peer should not be punished. See BIP 152. - * - * @return Returns true if the peer was punished (probably disconnected) - */ -static bool MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationState& state, bool via_compact_block, const std::string& message = "") { +bool PeerManager::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationState& state, + bool via_compact_block, const std::string& message) +{ switch (state.GetResult()) { case BlockValidationResult::BLOCK_RESULT_UNSET: break; diff --git a/src/net_processing.h b/src/net_processing.h index d4793ce303..c0a19f7bd2 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -11,6 +11,7 @@ #include #include +class BlockValidationState; class CBlockHeader; class CChainParams; class CTxMemPool; @@ -84,6 +85,19 @@ public: const std::chrono::microseconds time_received, const std::atomic& interruptMsgProc); private: + /** + * Potentially mark a node discouraged based on the contents of a BlockValidationState object + * + * @param[in] via_compact_block this bool is passed in because net_processing should + * punish peers differently depending on whether the data was provided in a compact + * block message or not. If the compact block had a valid header, but contained invalid + * txs, the peer should not be punished. See BIP 152. + * + * @return Returns true if the peer was punished (probably disconnected) + */ + bool MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationState& state, + bool via_compact_block, const std::string& message = ""); + /** Maybe disconnect a peer and discourage future connections from its address. * * @param[in] pnode The node to check. From e662e2d42afaf9c67c898634a0f3bc200255b6ea Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 24 Aug 2020 16:36:29 +0100 Subject: [PATCH 07/10] [net processing] Move ProcessOrphanTx to PeerManager --- src/net_processing.cpp | 12 ++++++------ src/net_processing.h | 2 ++ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 79d1ebe3ab..76913bc4b0 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2030,7 +2030,7 @@ void PeerManager::ProcessHeadersMessage(CNode& pfrom, const std::vector& orphan_work_set, std::list& removed_txn) EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_cs_orphans) +void PeerManager::ProcessOrphanTx(std::set& orphan_work_set, std::list& removed_txn) { AssertLockHeld(cs_main); AssertLockHeld(g_cs_orphans); @@ -2052,9 +2052,9 @@ void static ProcessOrphanTx(CConnman& connman, CTxMemPool& mempool, std::setGetWitnessHash(), connman); + RelayTransaction(orphanHash, porphanTx->GetWitnessHash(), m_connman); 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()) { @@ -2112,7 +2112,7 @@ void static ProcessOrphanTx(CConnman& connman, CTxMemPool& mempool, std::set& interruptMsgP if (!pfrom->orphan_work_set.empty()) { std::list removed_txn; LOCK2(cs_main, g_cs_orphans); - ProcessOrphanTx(m_connman, m_mempool, pfrom->orphan_work_set, removed_txn); + ProcessOrphanTx(pfrom->orphan_work_set, removed_txn); for (const CTransactionRef& removedTx : removed_txn) { AddToCompactExtraTransactions(removedTx); } diff --git a/src/net_processing.h b/src/net_processing.h index c0a19f7bd2..24866efd67 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -105,6 +105,8 @@ private: */ bool MaybeDiscourageAndDisconnect(CNode& pnode); + void ProcessOrphanTx(std::set& orphan_work_set, std::list& removed_txn) + EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_cs_orphans); /** Process a single headers message from a peer. */ void ProcessHeadersMessage(CNode& pfrom, const std::vector& headers, bool via_compact_block); From 3115e00f75b41d9765dcbb376e367b25f61a1d58 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 24 Aug 2020 16:40:10 +0100 Subject: [PATCH 08/10] [net processing] Move MaybePunishPeerForTx to PeerManager --- src/net_processing.cpp | 7 +------ src/net_processing.h | 8 ++++++++ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 76913bc4b0..c99b73bd31 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1182,12 +1182,7 @@ bool PeerManager::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationSt return false; } -/** - * Potentially disconnect and discourage a node based on the contents of a TxValidationState object - * - * @return Returns true if the peer was punished (probably disconnected) - */ -static bool MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state, const std::string& message = "") +bool PeerManager::MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state, const std::string& message) { switch (state.GetResult()) { case TxValidationResult::TX_RESULT_UNSET: diff --git a/src/net_processing.h b/src/net_processing.h index 24866efd67..4f350630f1 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -16,6 +16,7 @@ class CBlockHeader; class CChainParams; class CTxMemPool; class ChainstateManager; +class TxValidationState; extern RecursiveMutex cs_main; extern RecursiveMutex g_cs_orphans; @@ -98,6 +99,13 @@ private: bool MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationState& state, bool via_compact_block, const std::string& message = ""); + /** + * Potentially disconnect and discourage a node based on the contents of a TxValidationState object + * + * @return Returns true if the peer was punished (probably disconnected) + */ + bool MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state, const std::string& message = ""); + /** Maybe disconnect a peer and discourage future connections from its address. * * @param[in] pnode The node to check. From aa114b1c9b06c2bd3ed936bbb9fb32b31f75bdb2 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 24 Aug 2020 16:45:46 +0100 Subject: [PATCH 09/10] [net_processing] Move SendBlockTransactions into PeerManager --- src/net_processing.cpp | 8 ++++---- src/net_processing.h | 3 +++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index c99b73bd31..443e7c6eea 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1828,7 +1828,7 @@ static uint32_t GetFetchFlags(const CNode& pfrom) EXCLUSIVE_LOCKS_REQUIRED(cs_ma return nFetchFlags; } -inline void static SendBlockTransactions(const CBlock& block, const BlockTransactionsRequest& req, CNode& pfrom, CConnman& connman) { +void PeerManager::SendBlockTransactions(CNode& pfrom, const CBlock& block, const BlockTransactionsRequest& req) { BlockTransactions resp(req); for (size_t i = 0; i < req.indexes.size(); i++) { if (req.indexes[i] >= block.vtx.size()) { @@ -1840,7 +1840,7 @@ inline void static SendBlockTransactions(const CBlock& block, const BlockTransac LOCK(cs_main); const CNetMsgMaker msgMaker(pfrom.GetSendVersion()); int nSendFlags = State(pfrom.GetId())->fWantsCmpctWitness ? 0 : SERIALIZE_TRANSACTION_NO_WITNESS; - connman.PushMessage(&pfrom, msgMaker.Make(nSendFlags, NetMsgType::BLOCKTXN, resp)); + m_connman.PushMessage(&pfrom, msgMaker.Make(nSendFlags, NetMsgType::BLOCKTXN, resp)); } void PeerManager::ProcessHeadersMessage(CNode& pfrom, const std::vector& headers, bool via_compact_block) @@ -2845,7 +2845,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat // Unlock cs_most_recent_block to avoid cs_main lock inversion } if (recent_block) { - SendBlockTransactions(*recent_block, req, pfrom, m_connman); + SendBlockTransactions(pfrom, *recent_block, req); return; } @@ -2878,7 +2878,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat bool ret = ReadBlockFromDisk(block, pindex, m_chainparams.GetConsensus()); assert(ret); - SendBlockTransactions(block, req, pfrom, m_connman); + SendBlockTransactions(pfrom, block, req); return; } diff --git a/src/net_processing.h b/src/net_processing.h index 4f350630f1..c89fdb601b 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -11,6 +11,7 @@ #include #include +class BlockTransactionsRequest; class BlockValidationState; class CBlockHeader; class CChainParams; @@ -118,6 +119,8 @@ private: /** Process a single headers message from a peer. */ void ProcessHeadersMessage(CNode& pfrom, const std::vector& headers, bool via_compact_block); + void SendBlockTransactions(CNode& pfrom, const CBlock& block, const BlockTransactionsRequest& req); + const CChainParams& m_chainparams; CConnman& m_connman; /** Pointer to this node's banman. May be nullptr - check existence before dereferencing. */ From bb6a32ce9983c72afa90f41a43a47ffd703ca006 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 24 Aug 2020 16:56:15 +0100 Subject: [PATCH 10/10] [net processing] Move Misbehaving() to PeerManager --- src/net_processing.cpp | 6 +----- src/net_processing.h | 7 +++++++ src/test/denialofservice_tests.cpp | 9 ++++----- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 443e7c6eea..168a3cedea 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1110,11 +1110,7 @@ unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans) return nEvicted; } -/** - * Increment peer's misbehavior score. If the new value >= DISCOURAGEMENT_THRESHOLD, mark the node - * to be discouraged, meaning the peer might be disconnected and added to the discouragement filter. - */ -void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message) +void PeerManager::Misbehaving(const NodeId pnode, const int howmuch, const std::string& message) { assert(howmuch > 0); diff --git a/src/net_processing.h b/src/net_processing.h index c89fdb601b..520f7489e8 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -86,6 +86,13 @@ public: void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv, const std::chrono::microseconds time_received, const std::atomic& interruptMsgProc); + /** + * Increment peer's misbehavior score. If the new value >= DISCOURAGEMENT_THRESHOLD, mark the node + * to be discouraged, meaning the peer might be disconnected and added to the discouragement filter. + * Public for unit testing. + */ + void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message); + private: /** * Potentially mark a node discouraged based on the contents of a BlockValidationState object diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 2ee0d9b7de..f4d2204e1c 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -47,7 +47,6 @@ struct CConnmanTest : public CConnman { extern bool AddOrphanTx(const CTransactionRef& tx, NodeId peer); extern void EraseOrphansFor(NodeId peer); extern unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans); -extern void Misbehaving(NodeId nodeid, int howmuch, const std::string& message=""); struct COrphanTx { CTransactionRef tx; @@ -235,7 +234,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) peerLogic->InitializeNode(&dummyNode1); dummyNode1.nVersion = 1; dummyNode1.fSuccessfullyConnected = true; - Misbehaving(dummyNode1.GetId(), DISCOURAGEMENT_THRESHOLD); // Should be discouraged + peerLogic->Misbehaving(dummyNode1.GetId(), DISCOURAGEMENT_THRESHOLD, /* message */ ""); // Should be discouraged { LOCK(dummyNode1.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); @@ -249,14 +248,14 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) peerLogic->InitializeNode(&dummyNode2); dummyNode2.nVersion = 1; dummyNode2.fSuccessfullyConnected = true; - Misbehaving(dummyNode2.GetId(), DISCOURAGEMENT_THRESHOLD - 1); + peerLogic->Misbehaving(dummyNode2.GetId(), DISCOURAGEMENT_THRESHOLD - 1, /* message */ ""); { LOCK(dummyNode2.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode2)); } BOOST_CHECK(!banman->IsDiscouraged(addr2)); // 2 not discouraged yet... BOOST_CHECK(banman->IsDiscouraged(addr1)); // ... but 1 still should be - Misbehaving(dummyNode2.GetId(), 1); // 2 reaches discouragement threshold + peerLogic->Misbehaving(dummyNode2.GetId(), 1, /* message */ ""); // 2 reaches discouragement threshold { LOCK(dummyNode2.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode2)); @@ -287,7 +286,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) dummyNode.nVersion = 1; dummyNode.fSuccessfullyConnected = true; - Misbehaving(dummyNode.GetId(), DISCOURAGEMENT_THRESHOLD); + peerLogic->Misbehaving(dummyNode.GetId(), DISCOURAGEMENT_THRESHOLD, /* message */ ""); { LOCK(dummyNode.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode));