From 86512620c98f68c6204b4ff6ed8d78b4a1c62a72 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 15508683c..6608aa990 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 4f1c52fc1..4ba8d34b6 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 @@ class PeerLogicValidation final : public CValidationInterface, public NetEventsI 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 3be3c35edf36deeddeeec6acc9a29cbfb9cbe7c3 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 633dd8cef..e496276e1 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 6608aa990..789228787 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 4ba8d34b6..65fabcca3 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 @@ class PeerLogicValidation final : public CValidationInterface, public NetEventsI /** 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 @@ class PeerLogicValidation final : public CValidationInterface, public NetEventsI */ 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 bf0659587..8918da636 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 52efe5ddf..f94a3310d 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 536a13131..a9752f888 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 68bf31dc1d27f432b931c91352599feb7346a097 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 e496276e1..7dceef7ff 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 789228787..c92e841bc 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 @@ class CompareInvMempoolOrder }; } -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 65fabcca3..873ee34db 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 d9d075095..3228831ed 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 8918da636..2ee0d9b7d 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 f94a3310d..3d6947ca9 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 33385c06c..c9433d325 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 a9752f888..08aff0744 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 3ba2b2a10..625085c55 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 2459556cb4dd255c155afa7bce1a44bab2b21ae2 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 c92e841bc..048cab3e7 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 873ee34db..6ffd6c4fd 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 d2d3682e291347fe7729508ee36921a02191e68a 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 048cab3e7..fac0f100d 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 6ffd6c4fd..d4793ce30 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 @@ class PeerManager final : public CValidationInterface, public NetEventsInterface */ 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 cdb1541619d862422fac8b959282b95b7499fce2 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 fac0f100d..79d1ebe3a 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 d4793ce30..c0a19f7bd 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 @@ class PeerManager final : public CValidationInterface, public NetEventsInterface 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 794321a2c2526459eed534f5a37b2e72add02bf0 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 79d1ebe3a..76913bc4b 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 c0a19f7bd..24866efd6 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -105,6 +105,8 @@ class PeerManager final : public CValidationInterface, public NetEventsInterface */ 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 5d713c46fdf4ca91e5bd5bfb38426be161095974 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 76913bc4b..c99b73bd3 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 24866efd6..4f350630f 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 @@ class PeerManager final : public CValidationInterface, public NetEventsInterface 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 2583e4353f9ef3659b451e9f0972a5d5b538e4ee 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 c99b73bd3..443e7c6ee 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 4f350630f..c89fdb601 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 @@ class PeerManager final : public CValidationInterface, public NetEventsInterface /** 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 9156849870b1f0f4b3ab96a1aff49ff6626a9dd3 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 443e7c6ee..168a3cede 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 c89fdb601..520f7489e 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -86,6 +86,13 @@ class PeerManager final : public CValidationInterface, public NetEventsInterface 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 2ee0d9b7d..f4d2204e1 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));