From 1c25adf6d278eb1a1f018986a126d0eb8137e0ee Mon Sep 17 00:00:00 2001 From: John Newbery Date: Fri, 23 Oct 2020 09:34:27 +0100 Subject: [PATCH 1/6] [net] Construct addrman outside connman node.context owns the CAddrMan. CConnman holds a reference to the CAddrMan. --- src/init.cpp | 5 ++++- src/net.cpp | 4 ++-- src/net.h | 4 ++-- src/node/context.cpp | 1 + src/node/context.h | 2 ++ src/test/denialofservice_tests.cpp | 8 ++++---- src/test/fuzz/connman.cpp | 3 ++- src/test/util/setup_common.cpp | 5 ++++- 8 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 7d5420e3be67c..6ec46dbeb6a8d 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -228,6 +228,7 @@ void Shutdown(NodeContext& node) node.peerman.reset(); node.connman.reset(); node.banman.reset(); + node.addrman.reset(); if (node.mempool && node.mempool->IsLoaded() && node.args->GetArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) { DumpMempool(*node.mempool); @@ -1402,10 +1403,12 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA fDiscover = args.GetBoolArg("-discover", true); const bool ignores_incoming_txs{args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)}; + assert(!node.addrman); + node.addrman = std::make_unique(); assert(!node.banman); node.banman = std::make_unique(GetDataDir() / "banlist.dat", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME)); assert(!node.connman); - node.connman = std::make_unique(GetRand(std::numeric_limits::max()), GetRand(std::numeric_limits::max()), args.GetBoolArg("-networkactive", true)); + node.connman = std::make_unique(GetRand(std::numeric_limits::max()), GetRand(std::numeric_limits::max()), *node.addrman, args.GetBoolArg("-networkactive", true)); assert(!node.fee_estimator); // Don't initialize fee estimation with old data if we don't relay transactions, diff --git a/src/net.cpp b/src/net.cpp index 3b1ebede98d45..68b20421558a4 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2351,8 +2351,8 @@ void CConnman::SetNetworkActive(bool active) uiInterface.NotifyNetworkActiveChanged(fNetworkActive); } -CConnman::CConnman(uint64_t nSeed0In, uint64_t nSeed1In, bool network_active) - : nSeed0(nSeed0In), nSeed1(nSeed1In) +CConnman::CConnman(uint64_t nSeed0In, uint64_t nSeed1In, CAddrMan& addrman_in, bool network_active) + : addrman(addrman_in), nSeed0(nSeed0In), nSeed1(nSeed1In) { SetTryNewOutboundPeer(false); diff --git a/src/net.h b/src/net.h index 5228c4fbd3b3d..633e27ba80965 100644 --- a/src/net.h +++ b/src/net.h @@ -856,7 +856,7 @@ class CConnman m_onion_binds = connOptions.onion_binds; } - CConnman(uint64_t seed0, uint64_t seed1, bool network_active = true); + CConnman(uint64_t seed0, uint64_t seed1, CAddrMan& addrman, bool network_active = true); ~CConnman(); bool Start(CScheduler& scheduler, const Options& options); @@ -1130,7 +1130,7 @@ class CConnman std::vector vhListenSocket; std::atomic fNetworkActive{true}; bool fAddressesInitialized{false}; - CAddrMan addrman; + CAddrMan& addrman; std::deque m_addr_fetches GUARDED_BY(m_addr_fetches_mutex); RecursiveMutex m_addr_fetches_mutex; std::vector vAddedNodes GUARDED_BY(cs_vAddedNodes); diff --git a/src/node/context.cpp b/src/node/context.cpp index 958221a913529..6d22a6b110cf8 100644 --- a/src/node/context.cpp +++ b/src/node/context.cpp @@ -4,6 +4,7 @@ #include +#include #include #include #include diff --git a/src/node/context.h b/src/node/context.h index 9b611bf8f5278..2be9a584e6631 100644 --- a/src/node/context.h +++ b/src/node/context.h @@ -12,6 +12,7 @@ class ArgsManager; class BanMan; +class CAddrMan; class CBlockPolicyEstimator; class CConnman; class CScheduler; @@ -35,6 +36,7 @@ class WalletClient; //! any member functions. It should just be a collection of references that can //! be used without pulling in unwanted dependencies or functionality. struct NodeContext { + std::unique_ptr addrman; std::unique_ptr connman; std::unique_ptr mempool; std::unique_ptr fee_estimator; diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index e75982bc6fe2f..f592bd60c1332 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -67,7 +67,7 @@ BOOST_FIXTURE_TEST_SUITE(denialofservice_tests, TestingSetup) BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) { const CChainParams& chainparams = Params(); - auto connman = std::make_unique(0x1337, 0x1337); + auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); auto peerLogic = PeerManager::make(chainparams, *connman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool, false); @@ -137,7 +137,7 @@ static void AddRandomOutboundPeer(std::vector &vNodes, PeerManager &pee BOOST_AUTO_TEST_CASE(stale_tip_peer_management) { const CChainParams& chainparams = Params(); - auto connman = std::make_unique(0x1337, 0x1337); + auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); auto peerLogic = PeerManager::make(chainparams, *connman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool, false); @@ -211,7 +211,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) { const CChainParams& chainparams = Params(); auto banman = std::make_unique(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME); - auto connman = std::make_unique(0x1337, 0x1337); + auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); auto peerLogic = PeerManager::make(chainparams, *connman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool, false); @@ -258,7 +258,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) { const CChainParams& chainparams = Params(); auto banman = std::make_unique(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME); - auto connman = std::make_unique(0x1337, 0x1337); + auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); auto peerLogic = PeerManager::make(chainparams, *connman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool, false); diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index ae77a45e44598..7bb5ed96575ad 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -25,7 +25,8 @@ FUZZ_TARGET_INIT(connman, initialize_connman) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; SetMockTime(ConsumeTime(fuzzed_data_provider)); - CConnman connman{fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeBool()}; + CAddrMan addrman; + CConnman connman{fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral(), addrman, fuzzed_data_provider.ConsumeBool()}; CAddress random_address; CNetAddr random_netaddr; CNode random_node = ConsumeNode(fuzzed_data_provider); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index f866c2a1f9143..a4f5aa8b08db1 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -4,6 +4,7 @@ #include +#include #include #include #include @@ -155,6 +156,7 @@ ChainTestingSetup::~ChainTestingSetup() GetMainSignals().UnregisterBackgroundSignalScheduler(); m_node.connman.reset(); m_node.banman.reset(); + m_node.addrman.reset(); m_node.args = nullptr; UnloadBlockIndex(m_node.mempool.get(), *m_node.chainman); m_node.mempool.reset(); @@ -187,8 +189,9 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector(); m_node.banman = std::make_unique(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME); - m_node.connman = std::make_unique(0x1337, 0x1337); // Deterministic randomness for tests. + m_node.connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); // Deterministic randomness for tests. m_node.peerman = PeerManager::make(chainparams, *m_node.connman, m_node.banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool, false); From 392a95d393a9af01b53e5e68197e81968efb84fc Mon Sep 17 00:00:00 2001 From: John Newbery Date: Fri, 23 Oct 2020 10:17:45 +0100 Subject: [PATCH 2/6] [net_processing] Keep addrman reference in PeerManager --- src/init.cpp | 2 +- src/net_processing.cpp | 22 ++++++++++++---------- src/net_processing.h | 7 ++++--- src/test/denialofservice_tests.cpp | 16 ++++++++-------- src/test/util/setup_common.cpp | 6 +++--- 5 files changed, 28 insertions(+), 25 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 6ec46dbeb6a8d..54e30d31e55f7 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1424,7 +1424,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA ChainstateManager& chainman = *Assert(node.chainman); assert(!node.peerman); - node.peerman = PeerManager::make(chainparams, *node.connman, node.banman.get(), + node.peerman = PeerManager::make(chainparams, *node.connman, *node.addrman, node.banman.get(), *node.scheduler, chainman, *node.mempool, ignores_incoming_txs); RegisterValidationInterface(node.peerman.get()); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index e561f02c4acf3..fa24f061ef63c 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -225,9 +225,9 @@ using PeerRef = std::shared_ptr; class PeerManagerImpl final : public PeerManager { public: - PeerManagerImpl(const CChainParams& chainparams, CConnman& connman, BanMan* banman, - CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool, - bool ignore_incoming_txs); + PeerManagerImpl(const CChainParams& chainparams, CConnman& connman, CAddrMan& addrman, + BanMan* banman, CScheduler& scheduler, ChainstateManager& chainman, + CTxMemPool& pool, bool ignore_incoming_txs); /** Overridden from CValidationInterface. */ void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) override; @@ -322,6 +322,7 @@ class PeerManagerImpl final : public PeerManager const CChainParams& m_chainparams; CConnman& m_connman; + CAddrMan& m_addrman; /** Pointer to this node's banman. May be nullptr - check existence before dereferencing. */ BanMan* const m_banman; ChainstateManager& m_chainman; @@ -1201,18 +1202,19 @@ bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex) (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, m_chainparams.GetConsensus()) < STALE_RELAY_AGE_LIMIT); } -std::unique_ptr PeerManager::make(const CChainParams& chainparams, CConnman& connman, BanMan* banman, - CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool, - bool ignore_incoming_txs) +std::unique_ptr PeerManager::make(const CChainParams& chainparams, CConnman& connman, CAddrMan& addrman, + BanMan* banman, CScheduler& scheduler, ChainstateManager& chainman, + CTxMemPool& pool, bool ignore_incoming_txs) { - return std::make_unique(chainparams, connman, banman, scheduler, chainman, pool, ignore_incoming_txs); + return std::make_unique(chainparams, connman, addrman, banman, scheduler, chainman, pool, ignore_incoming_txs); } -PeerManagerImpl::PeerManagerImpl(const CChainParams& chainparams, CConnman& connman, BanMan* banman, - CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool, - bool ignore_incoming_txs) +PeerManagerImpl::PeerManagerImpl(const CChainParams& chainparams, CConnman& connman, CAddrMan& addrman, + BanMan* banman, CScheduler& scheduler, ChainstateManager& chainman, + CTxMemPool& pool, bool ignore_incoming_txs) : m_chainparams(chainparams), m_connman(connman), + m_addrman(addrman), m_banman(banman), m_chainman(chainman), m_mempool(pool), diff --git a/src/net_processing.h b/src/net_processing.h index f6f2d73721dda..4556d32377139 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -10,6 +10,7 @@ #include #include +class CAddrMan; class CChainParams; class CTxMemPool; class ChainstateManager; @@ -36,9 +37,9 @@ struct CNodeStateStats { class PeerManager : public CValidationInterface, public NetEventsInterface { public: - static std::unique_ptr make(const CChainParams& chainparams, CConnman& connman, BanMan* banman, - CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool, - bool ignore_incoming_txs); + static std::unique_ptr make(const CChainParams& chainparams, CConnman& connman, CAddrMan& addrman, + BanMan* banman, CScheduler& scheduler, ChainstateManager& chainman, + CTxMemPool& pool, bool ignore_incoming_txs); virtual ~PeerManager() { } /** Get statistics from node state */ diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index f592bd60c1332..a4ad7ed7cf58f 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -68,8 +68,8 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) { const CChainParams& chainparams = Params(); auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); - auto peerLogic = PeerManager::make(chainparams, *connman, nullptr, *m_node.scheduler, - *m_node.chainman, *m_node.mempool, false); + auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, nullptr, + *m_node.scheduler, *m_node.chainman, *m_node.mempool, false); // Mock an outbound peer CAddress addr1(ip(0xa0b0c001), NODE_NONE); @@ -138,8 +138,8 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management) { const CChainParams& chainparams = Params(); auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); - auto peerLogic = PeerManager::make(chainparams, *connman, nullptr, *m_node.scheduler, - *m_node.chainman, *m_node.mempool, false); + auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, nullptr, + *m_node.scheduler, *m_node.chainman, *m_node.mempool, false); constexpr int max_outbound_full_relay = MAX_OUTBOUND_FULL_RELAY_CONNECTIONS; CConnman::Options options; @@ -212,8 +212,8 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) const CChainParams& chainparams = Params(); auto banman = std::make_unique(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME); auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); - auto peerLogic = PeerManager::make(chainparams, *connman, banman.get(), *m_node.scheduler, - *m_node.chainman, *m_node.mempool, false); + auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, banman.get(), + *m_node.scheduler, *m_node.chainman, *m_node.mempool, false); banman->ClearBanned(); CAddress addr1(ip(0xa0b0c001), NODE_NONE); @@ -259,8 +259,8 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) const CChainParams& chainparams = Params(); auto banman = std::make_unique(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME); auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); - auto peerLogic = PeerManager::make(chainparams, *connman, banman.get(), *m_node.scheduler, - *m_node.chainman, *m_node.mempool, false); + auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, banman.get(), + *m_node.scheduler, *m_node.chainman, *m_node.mempool, false); banman->ClearBanned(); int64_t nStartTime = GetTime(); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index a4f5aa8b08db1..bfb3466dcf7d8 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -192,9 +192,9 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector(); m_node.banman = std::make_unique(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME); m_node.connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); // Deterministic randomness for tests. - m_node.peerman = PeerManager::make(chainparams, *m_node.connman, m_node.banman.get(), - *m_node.scheduler, *m_node.chainman, *m_node.mempool, - false); + m_node.peerman = PeerManager::make(chainparams, *m_node.connman, *m_node.addrman, + m_node.banman.get(), *m_node.scheduler, *m_node.chainman, + *m_node.mempool, false); { CConnman::Options options; options.m_msgproc = m_node.peerman.get(); From 8073673dbcb2744fcc9c011edf2d61388ca929cd Mon Sep 17 00:00:00 2001 From: John Newbery Date: Fri, 23 Oct 2020 10:19:25 +0100 Subject: [PATCH 3/6] [net] remove CConnman::SetServices It just forwards calls to CAddrMan::SetServices. --- src/net.cpp | 5 ----- src/net.h | 1 - src/net_processing.cpp | 2 +- src/test/fuzz/connman.cpp | 7 ------- 4 files changed, 1 insertion(+), 14 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 68b20421558a4..27c64a737e45a 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2635,11 +2635,6 @@ CConnman::~CConnman() Stop(); } -void CConnman::SetServices(const CService &addr, ServiceFlags nServices) -{ - addrman.SetServices(addr, nServices); -} - void CConnman::MarkAddressGood(const CAddress& addr) { addrman.Good(addr); diff --git a/src/net.h b/src/net.h index 633e27ba80965..0e4790a226afd 100644 --- a/src/net.h +++ b/src/net.h @@ -921,7 +921,6 @@ class CConnman }; // Addrman functions - void SetServices(const CService &addr, ServiceFlags nServices); void MarkAddressGood(const CAddress& addr); bool AddNewAddresses(const std::vector& vAddr, const CAddress& addrFrom, int64_t nTimePenalty = 0); std::vector GetAddresses(size_t max_addresses, size_t max_pct); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index fa24f061ef63c..6942f02a18ca2 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2332,7 +2332,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, nServices = ServiceFlags(nServiceInt); if (!pfrom.IsInboundConn()) { - m_connman.SetServices(pfrom.addr, nServices); + m_addrman.SetServices(pfrom.addr, nServices); } if (pfrom.ExpectServicesFromConn() && !HasAllDesirableServiceFlags(nServices)) { diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index 7bb5ed96575ad..4c4da242625e3 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -30,7 +30,6 @@ FUZZ_TARGET_INIT(connman, initialize_connman) CAddress random_address; CNetAddr random_netaddr; CNode random_node = ConsumeNode(fuzzed_data_provider); - CService random_service; CSubNet random_subnet; std::string random_string; while (fuzzed_data_provider.ConsumeBool()) { @@ -42,9 +41,6 @@ FUZZ_TARGET_INIT(connman, initialize_connman) [&] { random_netaddr = ConsumeNetAddr(fuzzed_data_provider); }, - [&] { - random_service = ConsumeService(fuzzed_data_provider); - }, [&] { random_subnet = ConsumeSubNet(fuzzed_data_provider); }, @@ -128,9 +124,6 @@ FUZZ_TARGET_INIT(connman, initialize_connman) [&] { connman.SetNetworkActive(fuzzed_data_provider.ConsumeBool()); }, - [&] { - connman.SetServices(random_service, ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS)); - }, [&] { connman.SetTryNewOutboundPeer(fuzzed_data_provider.ConsumeBool()); }); From bcd7f30b7944892db7ae37069175804567bb0cdf Mon Sep 17 00:00:00 2001 From: John Newbery Date: Fri, 23 Oct 2020 10:20:48 +0100 Subject: [PATCH 4/6] [net] remove CConnman::MarkAddressGood It just forwards calls to CAddrMan::Good. --- src/net.cpp | 5 ----- src/net.h | 1 - src/net_processing.cpp | 2 +- src/test/fuzz/connman.cpp | 7 ------- 4 files changed, 1 insertion(+), 14 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 27c64a737e45a..0e5909612eda5 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2635,11 +2635,6 @@ CConnman::~CConnman() Stop(); } -void CConnman::MarkAddressGood(const CAddress& addr) -{ - addrman.Good(addr); -} - bool CConnman::AddNewAddresses(const std::vector& vAddr, const CAddress& addrFrom, int64_t nTimePenalty) { return addrman.Add(vAddr, addrFrom, nTimePenalty); diff --git a/src/net.h b/src/net.h index 0e4790a226afd..f2add667d2b87 100644 --- a/src/net.h +++ b/src/net.h @@ -921,7 +921,6 @@ class CConnman }; // Addrman functions - void MarkAddressGood(const CAddress& addr); bool AddNewAddresses(const std::vector& vAddr, const CAddress& addrFrom, int64_t nTimePenalty = 0); std::vector GetAddresses(size_t max_addresses, size_t max_pct); /** diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6942f02a18ca2..ff1e46eb67800 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2476,7 +2476,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // // This moves an address from New to Tried table in Addrman, // resolves tried-table collisions, etc. - m_connman.MarkAddressGood(pfrom.addr); + m_addrman.Good(pfrom.addr); } std::string remoteAddr; diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index 4c4da242625e3..dec580ea22758 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -27,7 +27,6 @@ FUZZ_TARGET_INIT(connman, initialize_connman) SetMockTime(ConsumeTime(fuzzed_data_provider)); CAddrMan addrman; CConnman connman{fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral(), addrman, fuzzed_data_provider.ConsumeBool()}; - CAddress random_address; CNetAddr random_netaddr; CNode random_node = ConsumeNode(fuzzed_data_provider); CSubNet random_subnet; @@ -35,9 +34,6 @@ FUZZ_TARGET_INIT(connman, initialize_connman) while (fuzzed_data_provider.ConsumeBool()) { CallOneOf( fuzzed_data_provider, - [&] { - random_address = ConsumeAddress(fuzzed_data_provider); - }, [&] { random_netaddr = ConsumeNetAddr(fuzzed_data_provider); }, @@ -94,9 +90,6 @@ FUZZ_TARGET_INIT(connman, initialize_connman) [&] { (void)connman.GetNodeCount(fuzzed_data_provider.PickValueInArray({ConnectionDirection::None, ConnectionDirection::In, ConnectionDirection::Out, ConnectionDirection::Both})); }, - [&] { - connman.MarkAddressGood(random_address); - }, [&] { (void)connman.OutboundTargetReached(fuzzed_data_provider.ConsumeBool()); }, From 7c4cc67c0c3c50df004ee53cac5b2884b7fbab29 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Fri, 23 Oct 2020 10:24:16 +0100 Subject: [PATCH 5/6] [net] remove CConnman::AddNewAddresses It just forwards calls to CAddrMan::Add. --- src/net.cpp | 5 ----- src/net.h | 1 - src/net_processing.cpp | 2 +- src/rpc/net.cpp | 6 +++--- src/test/fuzz/connman.cpp | 8 -------- 5 files changed, 4 insertions(+), 18 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 0e5909612eda5..9553bbddfa581 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2635,11 +2635,6 @@ CConnman::~CConnman() Stop(); } -bool CConnman::AddNewAddresses(const std::vector& vAddr, const CAddress& addrFrom, int64_t nTimePenalty) -{ - return addrman.Add(vAddr, addrFrom, nTimePenalty); -} - std::vector CConnman::GetAddresses(size_t max_addresses, size_t max_pct) { std::vector addresses = addrman.GetAddr(max_addresses, max_pct); diff --git a/src/net.h b/src/net.h index f2add667d2b87..820b680c6c74d 100644 --- a/src/net.h +++ b/src/net.h @@ -921,7 +921,6 @@ class CConnman }; // Addrman functions - bool AddNewAddresses(const std::vector& vAddr, const CAddress& addrFrom, int64_t nTimePenalty = 0); std::vector GetAddresses(size_t max_addresses, size_t max_pct); /** * Cache is used to minimize topology leaks, so it should diff --git a/src/net_processing.cpp b/src/net_processing.cpp index ff1e46eb67800..4b91d86cfa711 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2681,7 +2681,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if (fReachable) vAddrOk.push_back(addr); } - m_connman.AddNewAddresses(vAddrOk, pfrom.addr, 2 * 60 * 60); + m_addrman.Add(vAddrOk, pfrom.addr, 2 * 60 * 60); if (vAddr.size() < 1000) pfrom.fGetAddr = false; if (pfrom.IsAddrFetchConn()) { diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index d4c1ab4b53202..96533a50c8429 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -907,8 +907,8 @@ static RPCHelpMan addpeeraddress() [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { NodeContext& node = EnsureNodeContext(request.context); - if (!node.connman) { - throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled"); + if (!node.addrman) { + throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Address manager functionality missing or disabled"); } UniValue obj(UniValue::VOBJ); @@ -925,7 +925,7 @@ static RPCHelpMan addpeeraddress() address.nTime = GetAdjustedTime(); // The source address is set equal to the address. This is equivalent to the peer // announcing itself. - if (!node.connman->AddNewAddresses({address}, address)) { + if (!node.addrman->Add(address, address)) { obj.pushKV("success", false); return obj; } diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index dec580ea22758..e07f25dedfa35 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -43,14 +43,6 @@ FUZZ_TARGET_INIT(connman, initialize_connman) [&] { random_string = fuzzed_data_provider.ConsumeRandomLengthString(64); }, - [&] { - std::vector addresses; - while (fuzzed_data_provider.ConsumeBool()) { - addresses.push_back(ConsumeAddress(fuzzed_data_provider)); - } - // Limit nTimePenalty to int32_t to avoid signed integer overflow - (void)connman.AddNewAddresses(addresses, ConsumeAddress(fuzzed_data_provider), fuzzed_data_provider.ConsumeIntegral()); - }, [&] { connman.AddNode(random_string); }, From 3fc06d3d7b43dc1143fe0850db23c4e7ffbfe682 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Fri, 23 Oct 2020 10:28:33 +0100 Subject: [PATCH 6/6] [net] remove fUpdateConnectionTime from FinalizeNode PeerManager can just call directly into CAddrMan::Connected() now. --- src/net.cpp | 6 +----- src/net.h | 2 +- src/net_processing.cpp | 22 ++++++++++++---------- src/test/denialofservice_tests.cpp | 14 +++++--------- 4 files changed, 19 insertions(+), 25 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 9553bbddfa581..f9ae67b75c5cb 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2621,11 +2621,7 @@ void CConnman::StopNodes() void CConnman::DeleteNode(CNode* pnode) { assert(pnode); - bool fUpdateConnectionTime = false; - m_msgproc->FinalizeNode(*pnode, fUpdateConnectionTime); - if (fUpdateConnectionTime) { - addrman.Connected(pnode->addr); - } + m_msgproc->FinalizeNode(*pnode); delete pnode; } diff --git a/src/net.h b/src/net.h index 820b680c6c74d..176fb3c74dfa1 100644 --- a/src/net.h +++ b/src/net.h @@ -770,7 +770,7 @@ class NetEventsInterface virtual void InitializeNode(CNode* pnode) = 0; /** Handle removal of a peer (clear state) */ - virtual void FinalizeNode(const CNode& node, bool& update_connection_time) = 0; + virtual void FinalizeNode(const CNode& node) = 0; /** * Process protocol messages received from a given node diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 4b91d86cfa711..68be99c3ff60c 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -238,7 +238,7 @@ class PeerManagerImpl final : public PeerManager /** Implement NetEventsInterface */ void InitializeNode(CNode* pnode) override; - void FinalizeNode(const CNode& node, bool& fUpdateConnectionTime) override; + void FinalizeNode(const CNode& node) override; bool ProcessMessages(CNode* pfrom, std::atomic& interrupt) override; bool SendMessages(CNode* pto) override EXCLUSIVE_LOCKS_REQUIRED(pto->cs_sendProcessing); @@ -969,12 +969,12 @@ void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler) scheduler.scheduleFromNow([&] { ReattemptInitialBroadcast(scheduler); }, delta); } -void PeerManagerImpl::FinalizeNode(const CNode& node, bool& fUpdateConnectionTime) +void PeerManagerImpl::FinalizeNode(const CNode& node) { NodeId nodeid = node.GetId(); - fUpdateConnectionTime = false; - LOCK(cs_main); int misbehavior{0}; + { + LOCK(cs_main); { // We remove the PeerRef from g_peer_map here, but we don't always // destruct the Peer. Sometimes another thread is still holding a @@ -991,12 +991,6 @@ void PeerManagerImpl::FinalizeNode(const CNode& node, bool& fUpdateConnectionTim if (state->fSyncStarted) nSyncStarted--; - if (node.fSuccessfullyConnected && misbehavior == 0 && - !node.IsBlockOnlyConn() && !node.IsInboundConn()) { - // Only change visible addrman state for outbound, full-relay peers - fUpdateConnectionTime = true; - } - for (const QueuedBlock& entry : state->vBlocksInFlight) { mapBlocksInFlight.erase(entry.hash); } @@ -1021,6 +1015,14 @@ void PeerManagerImpl::FinalizeNode(const CNode& node, bool& fUpdateConnectionTim assert(m_wtxid_relay_peers == 0); assert(m_txrequest.Size() == 0); } + } // cs_main + if (node.fSuccessfullyConnected && misbehavior == 0 && + !node.IsBlockOnlyConn() && !node.IsInboundConn()) { + // Only change visible addrman state for full outbound peers. We don't + // call Connected() for feeler connections since they don't have + // fSuccessfullyConnected set. + m_addrman.Connected(node.addr); + } LogPrint(BCLog::NET, "Cleared nodestate for peer=%d\n", nodeid); } diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index a4ad7ed7cf58f..7557d4618a5b1 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -117,8 +117,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) BOOST_CHECK(dummyNode1.fDisconnect == true); SetMockTime(0); - bool dummy; - peerLogic->FinalizeNode(dummyNode1, dummy); + peerLogic->FinalizeNode(dummyNode1); } static void AddRandomOutboundPeer(std::vector &vNodes, PeerManager &peerLogic, CConnmanTest* connman) @@ -199,9 +198,8 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management) BOOST_CHECK(vNodes[max_outbound_full_relay-1]->fDisconnect == true); BOOST_CHECK(vNodes.back()->fDisconnect == false); - bool dummy; for (const CNode *node : vNodes) { - peerLogic->FinalizeNode(*node, dummy); + peerLogic->FinalizeNode(*node); } connman->ClearNodes(); @@ -249,9 +247,8 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) BOOST_CHECK(banman->IsDiscouraged(addr1)); // Expect both 1 and 2 BOOST_CHECK(banman->IsDiscouraged(addr2)); // to be discouraged now - bool dummy; - peerLogic->FinalizeNode(dummyNode1, dummy); - peerLogic->FinalizeNode(dummyNode2, dummy); + peerLogic->FinalizeNode(dummyNode1); + peerLogic->FinalizeNode(dummyNode2); } BOOST_AUTO_TEST_CASE(DoS_bantime) @@ -279,8 +276,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) } BOOST_CHECK(banman->IsDiscouraged(addr)); - bool dummy; - peerLogic->FinalizeNode(dummyNode, dummy); + peerLogic->FinalizeNode(dummyNode); } class TxOrphanageTest : public TxOrphanage