From e16f420547fc72a5a2902927aa7138e43c0fb7c8 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Fri, 29 Sep 2023 23:23:36 +0200 Subject: [PATCH 1/4] net: Optionally include terrible addresses in GetAddr results --- src/addrman.cpp | 12 ++++++------ src/addrman.h | 3 ++- src/addrman_impl.h | 4 ++-- src/net.cpp | 4 ++-- src/net.h | 3 ++- 5 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 6ce9c81c63a0a..93f596cf009fb 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -800,7 +800,7 @@ int AddrManImpl::GetEntry(bool use_tried, size_t bucket, size_t position) const return -1; } -std::vector AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct, std::optional network) const +std::vector AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct, std::optional network, const bool filtered) const { AssertLockHeld(cs); @@ -830,7 +830,7 @@ std::vector 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); } @@ -1214,11 +1214,11 @@ std::pair AddrManImpl::Select(bool new_only, std::optiona return addrRet; } -std::vector AddrManImpl::GetAddr(size_t max_addresses, size_t max_pct, std::optional network) const +std::vector AddrManImpl::GetAddr(size_t max_addresses, size_t max_pct, std::optional 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; } @@ -1317,9 +1317,9 @@ std::pair AddrMan::Select(bool new_only, std::optionalSelect(new_only, network); } -std::vector AddrMan::GetAddr(size_t max_addresses, size_t max_pct, std::optional network) const +std::vector AddrMan::GetAddr(size_t max_addresses, size_t max_pct, std::optional 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> AddrMan::GetEntries(bool use_tried) const diff --git a/src/addrman.h b/src/addrman.h index 4d44c943ac808..6318fbef972fa 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -164,10 +164,11 @@ class AddrMan * @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 GetAddr(size_t max_addresses, size_t max_pct, std::optional network) const; + std::vector GetAddr(size_t max_addresses, size_t max_pct, std::optional 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 512f085a21f4a..867c894d01ded 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -129,7 +129,7 @@ class AddrManImpl std::pair Select(bool new_only, std::optional network) const EXCLUSIVE_LOCKS_REQUIRED(!cs); - std::vector GetAddr(size_t max_addresses, size_t max_pct, std::optional network) const + std::vector GetAddr(size_t max_addresses, size_t max_pct, std::optional network, const bool filtered = true) const EXCLUSIVE_LOCKS_REQUIRED(!cs); std::vector> GetEntries(bool from_tried) const @@ -261,7 +261,7 @@ class AddrManImpl * */ int GetEntry(bool use_tried, size_t bucket, size_t position) const EXCLUSIVE_LOCKS_REQUIRED(cs); - std::vector GetAddr_(size_t max_addresses, size_t max_pct, std::optional network) const EXCLUSIVE_LOCKS_REQUIRED(cs); + std::vector GetAddr_(size_t max_addresses, size_t max_pct, std::optional network, const bool filtered = true) const EXCLUSIVE_LOCKS_REQUIRED(cs); std::vector> GetEntries_(bool from_tried) const EXCLUSIVE_LOCKS_REQUIRED(cs); diff --git a/src/net.cpp b/src/net.cpp index 13f443042402d..8c39eaab2b0a8 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3410,9 +3410,9 @@ CConnman::~CConnman() Stop(); } -std::vector CConnman::GetAddresses(size_t max_addresses, size_t max_pct, std::optional network) const +std::vector CConnman::GetAddresses(size_t max_addresses, size_t max_pct, std::optional network, const bool filtered) const { - std::vector addresses = addrman.GetAddr(max_addresses, max_pct, network); + std::vector 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);}), diff --git a/src/net.h b/src/net.h index 2f7b832fbaa66..5a80896db4b05 100644 --- a/src/net.h +++ b/src/net.h @@ -1172,8 +1172,9 @@ class CConnman * @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 GetAddresses(size_t max_addresses, size_t max_pct, std::optional network) const; + std::vector GetAddresses(size_t max_addresses, size_t max_pct, std::optional 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. From b8843d37aed1276ff8527328c956c70c6e02ee13 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Fri, 29 Sep 2023 23:27:48 +0200 Subject: [PATCH 2/4] fuzz: Let fuzzers use filter options in GetAddr/GetAddresses --- src/test/fuzz/addrman.cpp | 3 ++- src/test/fuzz/connman.cpp | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index 1b11ff6fdf3f0..ece396aadfac5 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(0, 4096), /*max_pct=*/fuzzed_data_provider.ConsumeIntegralInRange(0, 4096), - network); + network, + /*filtered=*/fuzzed_data_provider.ConsumeBool()); (void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool(), network); std::optional in_new; if (fuzzed_data_provider.ConsumeBool()) { diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index 0dab2a2e9747f..a9a4d5756052a 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(), /*max_pct=*/fuzzed_data_provider.ConsumeIntegral(), - /*network=*/std::nullopt); + /*network=*/std::nullopt, + /*filtered=*/fuzzed_data_provider.ConsumeBool()); }, [&] { (void)connman.GetAddresses( From 28d7e55dff826a69f3f8e58139dbffb611cc5947 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Fri, 29 Sep 2023 23:42:28 +0200 Subject: [PATCH 3/4] test: Add tests for unfiltered GetAddr usage --- src/test/addrman_tests.cpp | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index b01ba81c5f119..1a8475cd25a5a 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(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(); + // 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) { From 3ea54e5db7d53da5afa321e1800c29aa269dd3b3 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Fri, 5 May 2023 11:14:51 +0200 Subject: [PATCH 4/4] net: Add continuous ASMap health check logging --- src/net.cpp | 19 +++++++++++++++++++ src/net.h | 3 +++ src/netgroup.cpp | 21 +++++++++++++++++++++ src/netgroup.h | 10 ++++++++++ test/functional/feature_asmap.py | 9 +++++++++ 5 files changed, 62 insertions(+) diff --git a/src/net.cpp b/src/net.cpp index 8c39eaab2b0a8..7ca33efb8d592 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3305,6 +3305,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; } @@ -3853,6 +3859,19 @@ void CConnman::PerformReconnections() } } +void CConnman::ASMapHealthCheck() +{ + const std::vector v4_addrs{GetAddresses(/*max_addresses=*/ 0, /*max_pct=*/ 0, Network::NET_IPV4, /*filtered=*/ false)}; + const std::vector v6_addrs{GetAddresses(/*max_addresses=*/ 0, /*max_pct=*/ 0, Network::NET_IPV6, /*filtered=*/ false)}; + std::vector 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(addr); }); + std::transform(v6_addrs.begin(), v6_addrs.end(), std::back_inserter(clearnet_addrs), + [](const CAddress& addr) { return static_cast(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 5a80896db4b05..8fcb9a00727e5 100644 --- a/src/net.h +++ b/src/net.h @@ -87,6 +87,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}; @@ -1138,6 +1140,7 @@ class CConnman 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); diff --git a/src/netgroup.cpp b/src/netgroup.cpp index 2d4d231f2b262..96e2b0974679e 100644 --- a/src/netgroup.cpp +++ b/src/netgroup.cpp @@ -5,6 +5,7 @@ #include #include +#include #include 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& clearnet_addrs) const { + std::set 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 2dd63ec66b056..5aa6ef774253f 100644 --- a/src/netgroup.h +++ b/src/netgroup.h @@ -41,6 +41,16 @@ class NetGroupManager { */ uint32_t GetMappedAS(const CNetAddr& address) const; + /** + * Analyze and log current health of ASMap based buckets. + */ + void ASMapHealthCheck(const std::vector& 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/test/functional/feature_asmap.py b/test/functional/feature_asmap.py index 9cff8042a8394..ae483fe4498d9 100755 --- a/test/functional/feature_asmap.py +++ b/test/functional/feature_asmap.py @@ -111,6 +111,14 @@ def test_empty_asmap(self): 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 @@ def run_test(self): 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__':