Merge #20228: addrman: Make addrman a top-level component

3fc06d3d7b43dc1143fe0850db23c4e7ffbfe682 [net] remove fUpdateConnectionTime from FinalizeNode (John Newbery)
7c4cc67c0c3c50df004ee53cac5b2884b7fbab29 [net] remove CConnman::AddNewAddresses (John Newbery)
bcd7f30b7944892db7ae37069175804567bb0cdf [net] remove CConnman::MarkAddressGood (John Newbery)
8073673dbcb2744fcc9c011edf2d61388ca929cd [net] remove CConnman::SetServices (John Newbery)
392a95d393a9af01b53e5e68197e81968efb84fc [net_processing] Keep addrman reference in PeerManager (John Newbery)
1c25adf6d278eb1a1f018986a126d0eb8137e0ee [net] Construct addrman outside connman (John Newbery)

Pull request description:

  Addrman is currently a member variable of connman. Make it a top-level component with lifetime owned by node.context, and add a reference to addrman in peerman. This allows us to eliminate some functions in connman that are simply forwarding requests to addrman, and simplifies the connman-peerman interface.

  By constructing the addrman in init, we can also add parameters to the ctor, which allows us to test it better. See #20233, where we enable consistency checking for addrman in our functional tests.

ACKs for top commit:
  MarcoFalke:
    re-ACK 3fc06d3d7b43dc1143fe0850db23c4e7ffbfe682 only change is squash 🏀
  vasild:
    ACK 3fc06d3d7b43dc1143fe0850db23c4e7ffbfe682

Tree-SHA512: 17662c65cbedcd9bd1c194914bc4bb4216f4e3581a06222de78f026d6796f1da6fe3e0bf28c2d26a102a12ad4fbf13f815944a297f000e3acf46faea42855e07

3 files changed, 26 insertions, 48 deletions

diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
index e75982bc6f..7557d4618a 100644
--- a/src/test/denialofservice_tests.cpp
+++ b/src/test/denialofservice_tests.cpp
@@ -67,9 +67,9 @@ BOOST_FIXTURE_TEST_SUITE(denialofservice_tests, TestingSetup)
 BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
 {
     const CChainParams& chainparams = Params();
-    auto connman = std::make_unique<CConnman>(0x1337, 0x1337);
-    auto peerLogic = PeerManager::make(chainparams, *connman, nullptr, *m_node.scheduler,
-                                       *m_node.chainman, *m_node.mempool, false);
+    auto connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman);
+    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);
@@ -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<CNode *> &vNodes, PeerManager &peerLogic, CConnmanTest* connman)
@@ -137,9 +136,9 @@ static void AddRandomOutboundPeer(std::vector<CNode *> &vNodes, PeerManager &pee
 BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
 {
     const CChainParams& chainparams = Params();
-    auto connman = std::make_unique<CConnmanTest>(0x1337, 0x1337);
-    auto peerLogic = PeerManager::make(chainparams, *connman, nullptr, *m_node.scheduler,
-                                       *m_node.chainman, *m_node.mempool, false);
+    auto connman = std::make_unique<CConnmanTest>(0x1337, 0x1337, *m_node.addrman);
+    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;
@@ -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();
@@ -211,9 +209,9 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
 {
     const CChainParams& chainparams = Params();
     auto banman = std::make_unique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
-    auto connman = std::make_unique<CConnman>(0x1337, 0x1337);
-    auto peerLogic = PeerManager::make(chainparams, *connman, banman.get(), *m_node.scheduler,
-                                       *m_node.chainman, *m_node.mempool, false);
+    auto connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman);
+    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);
@@ -249,18 +247,17 @@ 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)
 {
     const CChainParams& chainparams = Params();
     auto banman = std::make_unique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
-    auto connman = std::make_unique<CConnman>(0x1337, 0x1337);
-    auto peerLogic = PeerManager::make(chainparams, *connman, banman.get(), *m_node.scheduler,
-                                       *m_node.chainman, *m_node.mempool, false);
+    auto connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman);
+    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();
@@ -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
diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp
index ae77a45e44..e07f25dedf 100644
--- a/src/test/fuzz/connman.cpp
+++ b/src/test/fuzz/connman.cpp
@@ -25,40 +25,25 @@ 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<uint64_t>(), fuzzed_data_provider.ConsumeIntegral<uint64_t>(), fuzzed_data_provider.ConsumeBool()};
-    CAddress random_address;
+    CAddrMan addrman;
+    CConnman connman{fuzzed_data_provider.ConsumeIntegral<uint64_t>(), fuzzed_data_provider.ConsumeIntegral<uint64_t>(), addrman, fuzzed_data_provider.ConsumeBool()};
     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()) {
         CallOneOf(
             fuzzed_data_provider,
             [&] {
-                random_address = ConsumeAddress(fuzzed_data_provider);
-            },
-            [&] {
                 random_netaddr = ConsumeNetAddr(fuzzed_data_provider);
             },
             [&] {
-                random_service = ConsumeService(fuzzed_data_provider);
-            },
-            [&] {
                 random_subnet = ConsumeSubNet(fuzzed_data_provider);
             },
             [&] {
                 random_string = fuzzed_data_provider.ConsumeRandomLengthString(64);
             },
             [&] {
-                std::vector<CAddress> 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<int32_t>());
-            },
-            [&] {
                 connman.AddNode(random_string);
             },
             [&] {
@@ -98,9 +83,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());
             },
             [&] {
@@ -128,9 +110,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());
             });
     }
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index f866c2a1f9..bfb3466dcf 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -4,6 +4,7 @@
 
 #include <test/util/setup_common.h>
 
+#include <addrman.h>
 #include <banman.h>
 #include <chainparams.h>
 #include <consensus/consensus.h>
@@ -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,11 +189,12 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
         throw std::runtime_error(strprintf("ActivateBestChain failed. (%s)", state.ToString()));
     }
 
+    m_node.addrman = std::make_unique<CAddrMan>();
     m_node.banman = std::make_unique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
-    m_node.connman = std::make_unique<CConnman>(0x1337, 0x1337); // 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.connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman); // Deterministic randomness for tests.
+    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();