partial bitcoin#20228: Make addrman a top-level component

kwvg · kwvg · commit 574682d718e8 · 2023-02-18T07:50:11.000Z
diff --git a/src/init.cpp b/src/init.cpp
@@ -278,6 +278,7 @@ void PrepareShutdown(NodeContext& node)
     node.peer_logic.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);
@@ -1755,10 +1756,12 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc
     fDiscover = args.GetBoolArg("-discover", true);
     g_relay_txes = !args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY);
 
+    assert(!node.addrman);
+    node.addrman = std::make_unique<CAddrMan>();
     assert(!node.banman);
     node.banman = std::make_unique<BanMan>(GetDataDir() / "banlist.dat", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
     assert(!node.connman);
-    node.connman = std::make_unique<CConnman>(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max()));
+    node.connman = std::make_unique<CConnman>(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max()), *node.addrman);
 
     assert(!node.fee_estimator);
     // Don't initialize fee estimation with old data if we don't relay transactions,
@@ -1774,7 +1777,7 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc
     ChainstateManager& chainman = *Assert(node.chainman);
 
     node.peer_logic.reset(new PeerLogicValidation(
-        *node.connman, node.banman.get(), *node.scheduler, chainman, *node.mempool, node.llmq_ctx
+        *node.connman, *node.addrman, node.banman.get(), *node.scheduler, chainman, *node.mempool, node.llmq_ctx
     ));
     RegisterValidationInterface(node.peer_logic.get());
 
diff --git a/src/net.cpp b/src/net.cpp
@@ -2949,9 +2949,8 @@ void CConnman::SetNetworkActive(bool active)
     uiInterface.NotifyNetworkActiveChanged(fNetworkActive);
 }
 
-CConnman::CConnman(uint64_t nSeed0In, uint64_t nSeed1In) :
-        addrman(Params().AllowMultiplePorts()),
-        nSeed0(nSeed0In), nSeed1(nSeed1In)
+CConnman::CConnman(uint64_t nSeed0In, uint64_t nSeed1In, CAddrMan& addrman_in) :
+        addrman(addrman_in), nSeed0(nSeed0In), nSeed1(nSeed1In)
 {
     SetTryNewOutboundPeer(false);
 
@@ -3309,11 +3308,7 @@ void CConnman::Stop()
 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;
 }
 
@@ -3323,21 +3318,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);
-}
-
-void CConnman::AddNewAddresses(const std::vector<CAddress>& vAddr, const CAddress& addrFrom, int64_t nTimePenalty)
-{
-    addrman.Add(vAddr, addrFrom, nTimePenalty);
-}
-
 std::vector<CAddress> CConnman::GetAddresses()
 {
     return addrman.GetAddr();
diff --git a/src/net.h b/src/net.h
@@ -213,7 +213,7 @@ friend class CNode;
         socketEventsMode = connOptions.socketEventsMode;
     }
 
-    CConnman(uint64_t seed0, uint64_t seed1);
+    CConnman(uint64_t seed0, uint64_t seed1, CAddrMan& addrman);
     ~CConnman();
     bool Start(CScheduler& scheduler, const Options& options);
 
@@ -399,9 +399,6 @@ friend class CNode;
     void RelayInvFiltered(CInv &inv, const uint256 &relatedTxHash, const int minProtoVersion = MIN_PEER_PROTO_VERSION);
 
     // Addrman functions
-    void SetServices(const CService &addr, ServiceFlags nServices);
-    void MarkAddressGood(const CAddress& addr);
-    void AddNewAddresses(const std::vector<CAddress>& vAddr, const CAddress& addrFrom, int64_t nTimePenalty = 0);
     std::vector<CAddress> GetAddresses();
 
     // This allows temporarily exceeding m_max_outbound_full_relay, with the goal of finding
@@ -580,7 +577,7 @@ friend class CNode;
     std::vector<ListenSocket> vhListenSocket;
     std::atomic<bool> fNetworkActive{true};
     bool fAddressesInitialized{false};
-    CAddrMan addrman;
+    CAddrMan& addrman;
     std::deque<std::string> vOneShots GUARDED_BY(cs_vOneShots);
     CCriticalSection cs_vOneShots;
     std::vector<std::string> vAddedNodes GUARDED_BY(cs_vAddedNodes);
@@ -708,7 +705,7 @@ class NetEventsInterface
     virtual bool ProcessMessages(CNode* pnode, std::atomic<bool>& interrupt) = 0;
     virtual bool SendMessages(CNode* pnode) = 0;
     virtual void InitializeNode(CNode* pnode) = 0;
-    virtual void FinalizeNode(const CNode& node, bool& update_connection_time) = 0;
+    virtual void FinalizeNode(const CNode& node) = 0;
 
 protected:
     /**
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -965,11 +965,11 @@ void PeerLogicValidation::ReattemptInitialBroadcast(CScheduler& scheduler) const
     scheduler.scheduleFromNow([&] { ReattemptInitialBroadcast(scheduler); }, delta);
 }
 
-void PeerLogicValidation::FinalizeNode(const CNode& node, bool& fUpdateConnectionTime) {
+void PeerLogicValidation::FinalizeNode(const CNode& node) {
     NodeId nodeid = node.GetId();
-    fUpdateConnectionTime = false;
-    LOCK(cs_main);
     int misbehavior{0};
+    LOCK(cs_main);
+    {
     {
         PeerRef peer = GetPeerRef(nodeid);
         assert(peer != nullptr);
@@ -983,11 +983,6 @@ void PeerLogicValidation::FinalizeNode(const CNode& node, bool& fUpdateConnectio
     if (state->fSyncStarted)
         nSyncStarted--;
 
-    if (node.fSuccessfullyConnected && misbehavior == 0 /* && !node.IsBlockOnlyConn() */) {
-        // Note: we avoid changing visible addrman state for block-relay-only peers
-        fUpdateConnectionTime = true;
-    }
-
     for (const QueuedBlock& entry : state->vBlocksInFlight) {
         mapBlocksInFlight.erase(entry.hash);
     }
@@ -1007,6 +1002,15 @@ void PeerLogicValidation::FinalizeNode(const CNode& node, bool& fUpdateConnectio
         assert(nPeersWithValidatedDownloads == 0);
         assert(g_outbound_peers_with_protect_from_disconnect == 0);
     }
+    } // cs_main
+
+    if (node.fSuccessfullyConnected && misbehavior == 0 /* && !node.IsBlockOnlyConn() */) {
+        // 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);
 }
 
@@ -1296,9 +1300,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,
+PeerLogicValidation::PeerLogicValidation(CConnman& connman, CAddrMan& addrman, BanMan* banman, CScheduler &scheduler, ChainstateManager& chainman, CTxMemPool& pool,
                                         std::unique_ptr<LLMQContext>& llmq_ctx)
     : m_connman(connman),
+      m_addrman(addrman),
       m_banman(banman),
       m_chainman(chainman),
       m_mempool(pool),
@@ -2644,7 +2649,7 @@ void PeerLogicValidation::ProcessMessage(
         nServices = ServiceFlags(nServiceInt);
         if (!pfrom.fInbound)
         {
-            m_connman.SetServices(pfrom.addr, nServices);
+            m_addrman.SetServices(pfrom.addr, nServices);
         }
         if (!pfrom.fInbound && !pfrom.fFeeler && !pfrom.m_manual_connection && !HasAllDesirableServiceFlags(nServices))
         {
@@ -2782,7 +2787,7 @@ void PeerLogicValidation::ProcessMessage(
             // Get recent addresses
             m_connman.PushMessage(&pfrom, CNetMsgMaker(nSendVersion).Make(NetMsgType::GETADDR));
             pfrom.fGetAddr = true;
-            m_connman.MarkAddressGood(pfrom.addr);
+            m_addrman.Good(pfrom.addr);
         }
 
         std::string remoteAddr;
@@ -2954,7 +2959,7 @@ void PeerLogicValidation::ProcessMessage(
             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.fOneShot)
diff --git a/src/net_processing.h b/src/net_processing.h
@@ -11,6 +11,7 @@
 #include <sync.h>
 #include <validationinterface.h>
 
+class CAddrMan;
 class CTxMemPool;
 class ChainstateManager;
 struct LLMQContext;
@@ -28,14 +29,15 @@ class PeerLogicValidation final : public CValidationInterface, public NetEventsI
 private:
     CConnman& m_connman;
     BanMan* const m_banman;
+    CAddrMan& m_addrman;
     ChainstateManager& m_chainman;
     CTxMemPool& m_mempool;
     std::unique_ptr<LLMQContext>& m_llmq_ctx;
 
     bool MaybeDiscourageAndDisconnect(CNode& pnode);
 
 public:
-    PeerLogicValidation(CConnman& connman, BanMan* banman, CScheduler &scheduler, ChainstateManager& chainman, CTxMemPool& pool,
+    PeerLogicValidation(CConnman& connman, CAddrMan& addrman, BanMan* banman, CScheduler &scheduler, ChainstateManager& chainman, CTxMemPool& pool,
                         std::unique_ptr<LLMQContext>& llmq_ctx);
 
     /**
@@ -59,7 +61,7 @@ class PeerLogicValidation final : public CValidationInterface, public NetEventsI
     /** Initialize a peer by adding it to mapNodeState and pushing a message requesting its version */
     void InitializeNode(CNode* pnode) override;
     /** Handle removal of a peer by updating various state and removing it from mapNodeState */
-    void FinalizeNode(const CNode& node, bool& fUpdateConnectionTime) override;
+    void FinalizeNode(const CNode& node) override;
     /**
     * Process protocol messages received from a given node
     *
diff --git a/src/node/context.cpp b/src/node/context.cpp
@@ -4,6 +4,7 @@
 
 #include <node/context.h>
 
+#include <addrman.h>
 #include <banman.h>
 #include <interfaces/chain.h>
 #include <llmq/context.h>
diff --git 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;
@@ -37,6 +38,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<CAddrMan> addrman;
     std::unique_ptr<CConnman> connman;
     std::unique_ptr<CTxMemPool> mempool;
     std::unique_ptr<CBlockPolicyEstimator> fee_estimator;
diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
@@ -80,9 +80,9 @@ BOOST_FIXTURE_TEST_SUITE(denialofservice_tests, TestingSetup)
 // work.
 BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
 {
-    auto connman = std::make_unique<CConnman>(0x1337, 0x1337);
+    auto connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman);
     auto peerLogic = std::make_unique<PeerLogicValidation>(
-        *connman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx
+        *connman, *m_node.addrman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx
     );
 
     // Mock an outbound peer
@@ -133,8 +133,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<CNode *> &vNodes, PeerLogicValidation &peerLogic, CConnmanTest* connman)
@@ -153,9 +152,9 @@ static void AddRandomOutboundPeer(std::vector<CNode *> &vNodes, PeerLogicValidat
 
 BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
 {
-    auto connman = std::make_unique<CConnmanTest>(0x1337, 0x1337);
+    auto connman = std::make_unique<CConnmanTest>(0x1337, 0x1337, *m_node.addrman);
     auto peerLogic = std::make_unique<PeerLogicValidation>(
-        *connman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx
+        *connman, *m_node.addrman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx
     );
 
     const Consensus::Params& consensusParams = Params().GetConsensus();
@@ -217,9 +216,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();
@@ -228,9 +226,9 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
 BOOST_AUTO_TEST_CASE(DoS_banning)
 {
     auto banman = std::make_unique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
-    auto connman = std::make_unique<CConnman>(0x1337, 0x1337);
+    auto connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman);
     auto peerLogic = std::make_unique<PeerLogicValidation>(
-        *connman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx
+        *connman, *m_node.addrman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx
     );
 
     banman->ClearBanned();
@@ -268,17 +266,16 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
     }
     BOOST_CHECK(banman->IsDiscouraged(addr2));
 
-    bool dummy;
-    peerLogic->FinalizeNode(dummyNode1, dummy);
-    peerLogic->FinalizeNode(dummyNode2, dummy);
+    peerLogic->FinalizeNode(dummyNode1);
+    peerLogic->FinalizeNode(dummyNode2);
 }
 
 BOOST_AUTO_TEST_CASE(DoS_banscore)
 {
     auto banman = std::make_unique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
-    auto connman = std::make_unique<CConnman>(0x1337, 0x1337);
+    auto connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman);
     auto peerLogic = std::make_unique<PeerLogicValidation>(
-        *connman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx
+        *connman, *m_node.addrman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx
     );
 
     banman->ClearBanned();
@@ -315,16 +312,15 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)
     BOOST_CHECK(banman->IsDiscouraged(addr1));
     gArgs.ForceSetArg("-banscore", ToString(DEFAULT_BANSCORE_THRESHOLD));
 
-    bool dummy;
-    peerLogic->FinalizeNode(dummyNode1, dummy);
+    peerLogic->FinalizeNode(dummyNode1);
 }
 
 BOOST_AUTO_TEST_CASE(DoS_bantime)
 {
     auto banman = std::make_unique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
-    auto connman = std::make_unique<CConnman>(0x1337, 0x1337);
+    auto connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman);
     auto peerLogic = std::make_unique<PeerLogicValidation>(
-        *connman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx
+        *connman, *m_node.addrman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx
     );
 
     banman->ClearBanned();
@@ -345,8 +341,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
     }
     BOOST_CHECK(banman->IsDiscouraged(addr));
 
-    bool dummy;
-    peerLogic->FinalizeNode(dummyNode, dummy);
+    peerLogic->FinalizeNode(dummyNode);
 }
 
 static CTransactionRef RandomOrphan()
diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp
diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
