Merge bitcoin/bitcoin#27581: net: Continuous ASMap health check

3ea54e5db7d53da5afa321e1800c29aa269dd3b3 net: Add continuous ASMap health check logging (Fabian Jahr)
28d7e55dff826a69f3f8e58139dbffb611cc5947 test: Add tests for unfiltered GetAddr usage (Fabian Jahr)
b8843d37aed1276ff8527328c956c70c6e02ee13 fuzz: Let fuzzers use filter options in GetAddr/GetAddresses (Fabian Jahr)
e16f420547fc72a5a2902927aa7138e43c0fb7c8 net: Optionally include terrible addresses in GetAddr results (Fabian Jahr)

Pull request description:

  There are certain statistics we can collect by running all our known clearnet addresses against the ASMap file. This could show issues with a maliciously manipulated file or with an old file that has decayed with time.

  This is just a proof of concept for now. My idea currently is to run the analysis once per day and print the results to logs if an ASMap file is used.

ACKs for top commit:
  achow101:
    ACK 3ea54e5db7d53da5afa321e1800c29aa269dd3b3
  mzumsande:
    ACK 3ea54e5db7d53da5afa321e1800c29aa269dd3b3
  brunoerg:
    crACK 3ea54e5db7d53da5afa321e1800c29aa269dd3b3

Tree-SHA512: 777acbfac43cc43ce4a0a3612434e4ddbc65f59ae8ffc9e24f21de09011bccb297f0599cbaa82bcf40ef68e5af582c4e98556379db7ceff7d9f97574a1cf8e09

11 files changed, 98 insertions, 14 deletions

diff --git a/src/addrman.cpp b/src/addrman.cpp
index 5a11526471..a8206de6ee 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -802,7 +802,7 @@ int AddrManImpl::GetEntry(bool use_tried, size_t bucket, size_t position) const
     return -1;
 }
 
-std::vector<CAddress> AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network) const
+std::vector<CAddress> AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const
 {
     AssertLockHeld(cs);
 
@@ -832,7 +832,7 @@ std::vector<CAddress> AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct
         if (network != std::nullopt && ai.GetNetClass() != network) continue;
 
         // Filter for quality
-        if (ai.IsTerrible(now)) continue;
+        if (ai.IsTerrible(now) && filtered) continue;
 
         addresses.push_back(ai);
     }
@@ -1216,11 +1216,11 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select(bool new_only, std::optiona
     return addrRet;
 }
 
-std::vector<CAddress> AddrManImpl::GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network) const
+std::vector<CAddress> AddrManImpl::GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const
 {
     LOCK(cs);
     Check();
-    auto addresses = GetAddr_(max_addresses, max_pct, network);
+    auto addresses = GetAddr_(max_addresses, max_pct, network, filtered);
     Check();
     return addresses;
 }
@@ -1319,9 +1319,9 @@ std::pair<CAddress, NodeSeconds> AddrMan::Select(bool new_only, std::optional<Ne
     return m_impl->Select(new_only, network);
 }
 
-std::vector<CAddress> AddrMan::GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network) const
+std::vector<CAddress> AddrMan::GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const
 {
-    return m_impl->GetAddr(max_addresses, max_pct, network);
+    return m_impl->GetAddr(max_addresses, max_pct, network, filtered);
 }
 
 std::vector<std::pair<AddrInfo, AddressPosition>> AddrMan::GetEntries(bool use_tried) const
diff --git a/src/addrman.h b/src/addrman.h
index 67dc7604a4..ef9c766eff 100644
--- a/src/addrman.h
+++ b/src/addrman.h
@@ -164,10 +164,11 @@ public:
      * @param[in] max_addresses  Maximum number of addresses to return (0 = all).
      * @param[in] max_pct        Maximum percentage of addresses to return (0 = all).
      * @param[in] network        Select only addresses of this network (nullopt = all).
+     * @param[in] filtered       Select only addresses that are considered good quality (false = all).
      *
      * @return                   A vector of randomly selected addresses from vRandom.
      */
-    std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network) const;
+    std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const;
 
     /**
      * Returns an information-location pair for all addresses in the selected addrman table.
diff --git a/src/addrman_impl.h b/src/addrman_impl.h
index 512f085a21..867c894d01 100644
--- a/src/addrman_impl.h
+++ b/src/addrman_impl.h
@@ -129,7 +129,7 @@ public:
     std::pair<CAddress, NodeSeconds> Select(bool new_only, std::optional<Network> network) const
         EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
-    std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network) const
+    std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const
         EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
     std::vector<std::pair<AddrInfo, AddressPosition>> GetEntries(bool from_tried) const
@@ -261,7 +261,7 @@ private:
      * */
     int GetEntry(bool use_tried, size_t bucket, size_t position) const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
-    std::vector<CAddress> GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network) const EXCLUSIVE_LOCKS_REQUIRED(cs);
+    std::vector<CAddress> GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     std::vector<std::pair<AddrInfo, AddressPosition>> GetEntries_(bool from_tried) const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
diff --git a/src/net.cpp b/src/net.cpp
index dc76fdfb44..102d81579f 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -3278,6 +3278,12 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
     // Dump network addresses
     scheduler.scheduleEvery([this] { DumpAddresses(); }, DUMP_PEERS_INTERVAL);
 
+    // Run the ASMap Health check once and then schedule it to run every 24h.
+    if (m_netgroupman.UsingASMap()) {
+        ASMapHealthCheck();
+        scheduler.scheduleEvery([this] { ASMapHealthCheck(); }, ASMAP_HEALTH_CHECK_INTERVAL);
+    }
+
     return true;
 }
 
@@ -3383,9 +3389,9 @@ CConnman::~CConnman()
     Stop();
 }
 
-std::vector<CAddress> CConnman::GetAddresses(size_t max_addresses, size_t max_pct, std::optional<Network> network) const
+std::vector<CAddress> CConnman::GetAddresses(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const
 {
-    std::vector<CAddress> addresses = addrman.GetAddr(max_addresses, max_pct, network);
+    std::vector<CAddress> addresses = addrman.GetAddr(max_addresses, max_pct, network, filtered);
     if (m_banman) {
         addresses.erase(std::remove_if(addresses.begin(), addresses.end(),
                         [this](const CAddress& addr){return m_banman->IsDiscouraged(addr) || m_banman->IsBanned(addr);}),
@@ -3840,6 +3846,19 @@ void CConnman::PerformReconnections()
     }
 }
 
+void CConnman::ASMapHealthCheck()
+{
+    const std::vector<CAddress> v4_addrs{GetAddresses(/*max_addresses=*/ 0, /*max_pct=*/ 0, Network::NET_IPV4, /*filtered=*/ false)};
+    const std::vector<CAddress> v6_addrs{GetAddresses(/*max_addresses=*/ 0, /*max_pct=*/ 0, Network::NET_IPV6, /*filtered=*/ false)};
+    std::vector<CNetAddr> clearnet_addrs;
+    clearnet_addrs.reserve(v4_addrs.size() + v6_addrs.size());
+    std::transform(v4_addrs.begin(), v4_addrs.end(), std::back_inserter(clearnet_addrs),
+        [](const CAddress& addr) { return static_cast<CNetAddr>(addr); });
+    std::transform(v6_addrs.begin(), v6_addrs.end(), std::back_inserter(clearnet_addrs),
+        [](const CAddress& addr) { return static_cast<CNetAddr>(addr); });
+    m_netgroupman.ASMapHealthCheck(clearnet_addrs);
+}
+
 // Dump binary message to file, with timestamp.
 static void CaptureMessageToFile(const CAddress& addr,
                                  const std::string& msg_type,
diff --git a/src/net.h b/src/net.h
index 28f304b062..547e032ba6 100644
--- a/src/net.h
+++ b/src/net.h
@@ -88,6 +88,8 @@ static const bool DEFAULT_BLOCKSONLY = false;
 static const int64_t DEFAULT_PEER_CONNECT_TIMEOUT = 60;
 /** Number of file descriptors required for message capture **/
 static const int NUM_FDS_MESSAGE_CAPTURE = 1;
+/** Interval for ASMap Health Check **/
+static constexpr std::chrono::hours ASMAP_HEALTH_CHECK_INTERVAL{24};
 
 static constexpr bool DEFAULT_FORCEDNSSEED{false};
 static constexpr bool DEFAULT_DNSSEED{true};
@@ -1112,6 +1114,7 @@ public:
     void SetNetworkActive(bool active);
     void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant&& grant_outbound, const char* strDest, ConnectionType conn_type, bool use_v2transport) EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex);
     bool CheckIncomingNonce(uint64_t nonce);
+    void ASMapHealthCheck();
 
     // alias for thread safety annotations only, not defined
     RecursiveMutex& GetNodesMutex() const LOCK_RETURNED(m_nodes_mutex);
@@ -1146,8 +1149,9 @@ public:
      * @param[in] max_addresses  Maximum number of addresses to return (0 = all).
      * @param[in] max_pct        Maximum percentage of addresses to return (0 = all).
      * @param[in] network        Select only addresses of this network (nullopt = all).
+     * @param[in] filtered       Select only addresses that are considered high quality (false = all).
      */
-    std::vector<CAddress> GetAddresses(size_t max_addresses, size_t max_pct, std::optional<Network> network) const;
+    std::vector<CAddress> GetAddresses(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const;
     /**
      * Cache is used to minimize topology leaks, so it should
      * be used for all non-trusted calls, for example, p2p.
diff --git a/src/netgroup.cpp b/src/netgroup.cpp
index a03927b152..0ae229b3f3 100644
--- a/src/netgroup.cpp
+++ b/src/netgroup.cpp
@@ -5,6 +5,7 @@
 #include <netgroup.h>
 
 #include <hash.h>
+#include <logging.h>
 #include <util/asmap.h>
 
 uint256 NetGroupManager::GetAsmapChecksum() const
@@ -109,3 +110,23 @@ uint32_t NetGroupManager::GetMappedAS(const CNetAddr& address) const
     uint32_t mapped_as = Interpret(m_asmap, ip_bits);
     return mapped_as;
 }
+
+void NetGroupManager::ASMapHealthCheck(const std::vector<CNetAddr>& clearnet_addrs) const {
+    std::set<uint32_t> clearnet_asns{};
+    int unmapped_count{0};
+
+    for (const auto& addr : clearnet_addrs) {
+        uint32_t asn = GetMappedAS(addr);
+        if (asn == 0) {
+            ++unmapped_count;
+            continue;
+        }
+        clearnet_asns.insert(asn);
+    }
+
+    LogPrintf("ASMap Health Check: %i clearnet peers are mapped to %i ASNs with %i peers being unmapped\n", clearnet_addrs.size(), clearnet_asns.size(), unmapped_count);
+}
+
+bool NetGroupManager::UsingASMap() const {
+    return m_asmap.size() > 0;
+}
diff --git a/src/netgroup.h b/src/netgroup.h
index 2dd63ec66b..5aa6ef7742 100644
--- a/src/netgroup.h
+++ b/src/netgroup.h
@@ -41,6 +41,16 @@ public:
      */
     uint32_t GetMappedAS(const CNetAddr& address) const;
 
+    /**
+     *  Analyze and log current health of ASMap based buckets.
+     */
+    void ASMapHealthCheck(const std::vector<CNetAddr>& clearnet_addrs) const;
+
+    /**
+     *  Indicates whether ASMap is being used for clearnet bucketing.
+     */
+    bool UsingASMap() const;
+
 private:
     /** Compressed IP->ASN mapping, loaded from a file when a node starts.
      *
diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp
index bfefc3ff97..5bd4f271cd 100644
--- a/src/test/addrman_tests.cpp
+++ b/src/test/addrman_tests.cpp
@@ -429,6 +429,24 @@ BOOST_AUTO_TEST_CASE(addrman_getaddr)
     BOOST_CHECK_EQUAL(addrman->Size(), 2006U);
 }
 
+BOOST_AUTO_TEST_CASE(getaddr_unfiltered)
+{
+    auto addrman = std::make_unique<AddrMan>(EMPTY_NETGROUPMAN, DETERMINISTIC, GetCheckRatio(m_node));
+
+    // Set time on this addr so isTerrible = false
+    CAddress addr1 = CAddress(ResolveService("250.250.2.1", 8333), NODE_NONE);
+    addr1.nTime = Now<NodeSeconds>();
+    // Not setting time so this addr should be isTerrible = true
+    CAddress addr2 = CAddress(ResolveService("250.251.2.2", 9999), NODE_NONE);
+
+    CNetAddr source = ResolveIP("250.1.2.1");
+    BOOST_CHECK(addrman->Add({addr1, addr2}, source));
+
+    // Filtered GetAddr should only return addr1
+    BOOST_CHECK_EQUAL(addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt).size(), 1U);
+    // Unfiltered GetAddr should return addr1 and addr2
+    BOOST_CHECK_EQUAL(addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt, /*filtered=*/false).size(), 2U);
+}
 
 BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket_legacy)
 {
diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp
index 1b11ff6fdf..ece396aadf 100644
--- a/src/test/fuzz/addrman.cpp
+++ b/src/test/fuzz/addrman.cpp
@@ -286,7 +286,8 @@ FUZZ_TARGET(addrman, .init = initialize_addrman)
     (void)const_addr_man.GetAddr(
         /*max_addresses=*/fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096),
         /*max_pct=*/fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096),
-        network);
+        network,
+        /*filtered=*/fuzzed_data_provider.ConsumeBool());
     (void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool(), network);
     std::optional<bool> in_new;
     if (fuzzed_data_provider.ConsumeBool()) {
diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp
index 551e1c526d..24f91abd25 100644
--- a/src/test/fuzz/connman.cpp
+++ b/src/test/fuzz/connman.cpp
@@ -88,7 +88,8 @@ FUZZ_TARGET(connman, .init = initialize_connman)
                 (void)connman.GetAddresses(
                     /*max_addresses=*/fuzzed_data_provider.ConsumeIntegral<size_t>(),
                     /*max_pct=*/fuzzed_data_provider.ConsumeIntegral<size_t>(),
-                    /*network=*/std::nullopt);
+                    /*network=*/std::nullopt,
+                    /*filtered=*/fuzzed_data_provider.ConsumeBool());
             },
             [&] {
                 (void)connman.GetAddresses(
diff --git a/test/functional/feature_asmap.py b/test/functional/feature_asmap.py
index 9cff8042a8..ae483fe449 100755
--- a/test/functional/feature_asmap.py
+++ b/test/functional/feature_asmap.py
@@ -111,6 +111,14 @@ class AsmapTest(BitcoinTestFramework):
         self.node.assert_start_raises_init_error(extra_args=['-asmap'], expected_msg=msg)
         os.remove(self.default_asmap)
 
+    def test_asmap_health_check(self):
+        self.log.info('Test bitcoind -asmap logs ASMap Health Check with basic stats')
+        shutil.copyfile(self.asmap_raw, self.default_asmap)
+        msg = "ASMap Health Check: 2 clearnet peers are mapped to 1 ASNs with 0 peers being unmapped"
+        with self.node.assert_debug_log(expected_msgs=[msg]):
+            self.start_node(0, extra_args=['-asmap'])
+        os.remove(self.default_asmap)
+
     def run_test(self):
         self.node = self.nodes[0]
         self.datadir = self.node.chain_path
@@ -124,6 +132,7 @@ class AsmapTest(BitcoinTestFramework):
         self.test_asmap_interaction_with_addrman_containing_entries()
         self.test_default_asmap_with_missing_file()
         self.test_empty_asmap()
+        self.test_asmap_health_check()
 
 
 if __name__ == '__main__':