Merge bitcoin/bitcoin#29536: fuzz: fuzz connman with non-empty addrman + ASMap

552cae2 fuzz: cover `ASMapHealthCheck` in connman target (brunoerg)
33b0f3a fuzz: use `ConsumeNetGroupManager` in connman target (brunoerg)
18c8a09 fuzz: move `ConsumeNetGroupManager` to util (brunoerg)
fe62463 fuzz: fuzz `connman` with a non-empty addrman (brunoerg)
0a12cff fuzz: move `AddrManDeterministic` to util (brunoerg)

Pull request description:

  ### Motivation

  Currently, we fuzz connman with an addrman from `NodeContext`. However,
  fuzzing connman with only empty addrman might not be effective, especially
  for functions like `GetAddresses` and other ones that plays with addrman. Also,
  we do not fuzz connman with ASMap, what would be good for functions that need
  `GetGroup`, or even for addrman. Without it, I do not see how effective would be
   fuzzing `ASMapHealthCheck`, for example.

  ### Changes

  - Move `AddrManDeterministic` and `ConsumeNetGroupManager` to util.
  - Use `ConsumeNetGroupManager` in connman target to construct a netgroupmanager
  and use it for `ConnmanTestMsg`.
  - Use `AddrManDeterministic` in connman target to create an addrman. It does
   not slow down as "filling" the addrman (e.g. with `FillAddrman`).
  - Add coverage for `ASMapHealthCheck`.

ACKs for top commit:
  maflcko:
    review ACK 552cae2 🏀
  dergoegge:
    Code review ACK 552cae2
  marcofleon:
    Code review ACK 552cae2. Changes match the PR description.

Tree-SHA512: ba861c839602054077e4bf3649763eeb48357cda83ca3ddd32b02a1b61f4e44a0c5070182f001f9bf531d0d64717876279a7de3ddb9de028b343533b89233851

Author: fanquake

src/test/fuzz/addrman.cpp

@@ -39,13 +39,6 @@ void initialize_addrman()
     g_setup = testing_setup.get();
 }
 
-[[nodiscard]] inline NetGroupManager ConsumeNetGroupManager(FuzzedDataProvider& fuzzed_data_provider) noexcept
-{
-    std::vector<bool> asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider);
-    if (!SanityCheckASMap(asmap, 128)) asmap.clear();
-    return NetGroupManager(asmap);
-}
-
 FUZZ_TARGET(data_stream_addr_man, .init = initialize_addrman)
 {
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
@@ -118,121 +111,19 @@ void FillAddrman(AddrMan& addrman, FuzzedDataProvider& fuzzed_data_provider)
     }
 }
 
-class AddrManDeterministic : public AddrMan
-{
-public:
-    explicit AddrManDeterministic(const NetGroupManager& netgroupman, FuzzedDataProvider& fuzzed_data_provider)
-        : AddrMan(netgroupman, /*deterministic=*/true, GetCheckRatio())
-    {
-        WITH_LOCK(m_impl->cs, m_impl->insecure_rand.Reseed(ConsumeUInt256(fuzzed_data_provider)));
-    }
-
-    /**
-     * Compare with another AddrMan.
-     * This compares:
-     * - the values in `mapInfo` (the keys aka ids are ignored)
-     * - vvNew entries refer to the same addresses
-     * - vvTried entries refer to the same addresses
-     */
-    bool operator==(const AddrManDeterministic& other) const
-    {
-        LOCK2(m_impl->cs, other.m_impl->cs);
-
-        if (m_impl->mapInfo.size() != other.m_impl->mapInfo.size() || m_impl->nNew != other.m_impl->nNew ||
-            m_impl->nTried != other.m_impl->nTried) {
-            return false;
-        }
-
-        // Check that all values in `mapInfo` are equal to all values in `other.mapInfo`.
-        // Keys may be different.
-
-        auto addrinfo_hasher = [](const AddrInfo& a) {
-            CSipHasher hasher(0, 0);
-            auto addr_key = a.GetKey();
-            auto source_key = a.source.GetAddrBytes();
-            hasher.Write(TicksSinceEpoch<std::chrono::seconds>(a.m_last_success));
-            hasher.Write(a.nAttempts);
-            hasher.Write(a.nRefCount);
-            hasher.Write(a.fInTried);
-            hasher.Write(a.GetNetwork());
-            hasher.Write(a.source.GetNetwork());
-            hasher.Write(addr_key.size());
-            hasher.Write(source_key.size());
-            hasher.Write(addr_key);
-            hasher.Write(source_key);
-            return (size_t)hasher.Finalize();
-        };
-
-        auto addrinfo_eq = [](const AddrInfo& lhs, const AddrInfo& rhs) {
-            return std::tie(static_cast<const CService&>(lhs), lhs.source, lhs.m_last_success, lhs.nAttempts, lhs.nRefCount, lhs.fInTried) ==
-                   std::tie(static_cast<const CService&>(rhs), rhs.source, rhs.m_last_success, rhs.nAttempts, rhs.nRefCount, rhs.fInTried);
-        };
-
-        using Addresses = std::unordered_set<AddrInfo, decltype(addrinfo_hasher), decltype(addrinfo_eq)>;
-
-        const size_t num_addresses{m_impl->mapInfo.size()};
-
-        Addresses addresses{num_addresses, addrinfo_hasher, addrinfo_eq};
-        for (const auto& [id, addr] : m_impl->mapInfo) {
-            addresses.insert(addr);
-        }
-
-        Addresses other_addresses{num_addresses, addrinfo_hasher, addrinfo_eq};
-        for (const auto& [id, addr] : other.m_impl->mapInfo) {
-            other_addresses.insert(addr);
-        }
-
-        if (addresses != other_addresses) {
-            return false;
-        }
-
-        auto IdsReferToSameAddress = [&](nid_type id, nid_type other_id) EXCLUSIVE_LOCKS_REQUIRED(m_impl->cs, other.m_impl->cs) {
-            if (id == -1 && other_id == -1) {
-                return true;
-            }
-            if ((id == -1 && other_id != -1) || (id != -1 && other_id == -1)) {
-                return false;
-            }
-            return m_impl->mapInfo.at(id) == other.m_impl->mapInfo.at(other_id);
-        };
-
-        // Check that `vvNew` contains the same addresses as `other.vvNew`. Notice - `vvNew[i][j]`
-        // contains just an id and the address is to be found in `mapInfo.at(id)`. The ids
-        // themselves may differ between `vvNew` and `other.vvNew`.
-        for (size_t i = 0; i < ADDRMAN_NEW_BUCKET_COUNT; ++i) {
-            for (size_t j = 0; j < ADDRMAN_BUCKET_SIZE; ++j) {
-                if (!IdsReferToSameAddress(m_impl->vvNew[i][j], other.m_impl->vvNew[i][j])) {
-                    return false;
-                }
-            }
-        }
-
-        // Same for `vvTried`.
-        for (size_t i = 0; i < ADDRMAN_TRIED_BUCKET_COUNT; ++i) {
-            for (size_t j = 0; j < ADDRMAN_BUCKET_SIZE; ++j) {
-                if (!IdsReferToSameAddress(m_impl->vvTried[i][j], other.m_impl->vvTried[i][j])) {
-                    return false;
-                }
-            }
-        }
-
-        return true;
-    }
-};
-
 FUZZ_TARGET(addrman, .init = initialize_addrman)
 {
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
     SetMockTime(ConsumeTime(fuzzed_data_provider));
     NetGroupManager netgroupman{ConsumeNetGroupManager(fuzzed_data_provider)};
-    auto addr_man_ptr = std::make_unique<AddrManDeterministic>(netgroupman, fuzzed_data_provider);
+    auto addr_man_ptr = std::make_unique<AddrManDeterministic>(netgroupman, fuzzed_data_provider, GetCheckRatio());
     if (fuzzed_data_provider.ConsumeBool()) {
         const std::vector<uint8_t> serialized_data{ConsumeRandomLengthByteVector(fuzzed_data_provider)};
         DataStream ds{serialized_data};
         try {
             ds >> *addr_man_ptr;
         } catch (const std::ios_base::failure&) {
-            addr_man_ptr = std::make_unique<AddrManDeterministic>(netgroupman, fuzzed_data_provider);
+            addr_man_ptr = std::make_unique<AddrManDeterministic>(netgroupman, fuzzed_data_provider, GetCheckRatio());
         }
     }
     AddrManDeterministic& addr_man = *addr_man_ptr;
@@ -310,8 +201,8 @@ FUZZ_TARGET(addrman_serdeser, .init = initialize_addrman)
     SetMockTime(ConsumeTime(fuzzed_data_provider));
 
     NetGroupManager netgroupman{ConsumeNetGroupManager(fuzzed_data_provider)};
-    AddrManDeterministic addr_man1{netgroupman, fuzzed_data_provider};
-    AddrManDeterministic addr_man2{netgroupman, fuzzed_data_provider};
+    AddrManDeterministic addr_man1{netgroupman, fuzzed_data_provider, GetCheckRatio()};
+    AddrManDeterministic addr_man2{netgroupman, fuzzed_data_provider, GetCheckRatio()};
 
     DataStream data_stream{};
 

src/test/fuzz/connman.cpp

@@ -20,6 +20,12 @@
 
 namespace {
 const TestingSetup* g_setup;
+
+int32_t GetCheckRatio()
+{
+    return std::clamp<int32_t>(g_setup->m_node.args->GetIntArg("-checkaddrman", 0), 0, 1000000);
+}
+
 } // namespace
 
 void initialize_connman()
@@ -32,10 +38,22 @@ FUZZ_TARGET(connman, .init = initialize_connman)
 {
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
     SetMockTime(ConsumeTime(fuzzed_data_provider));
+    auto netgroupman{ConsumeNetGroupManager(fuzzed_data_provider)};
+    auto addr_man_ptr{std::make_unique<AddrManDeterministic>(netgroupman, fuzzed_data_provider, GetCheckRatio())};
+    if (fuzzed_data_provider.ConsumeBool()) {
+        const std::vector<uint8_t> serialized_data{ConsumeRandomLengthByteVector(fuzzed_data_provider)};
+        DataStream ds{serialized_data};
+        try {
+            ds >> *addr_man_ptr;
+        } catch (const std::ios_base::failure&) {
+            addr_man_ptr = std::make_unique<AddrManDeterministic>(netgroupman, fuzzed_data_provider, GetCheckRatio());
+        }
+    }
+    AddrManDeterministic& addr_man{*addr_man_ptr};
     ConnmanTestMsg connman{fuzzed_data_provider.ConsumeIntegral<uint64_t>(),
                      fuzzed_data_provider.ConsumeIntegral<uint64_t>(),
-                     *g_setup->m_node.addrman,
-                     *g_setup->m_node.netgroupman,
+                     addr_man,
+                     netgroupman,
                      Params(),
                      fuzzed_data_provider.ConsumeBool()};
 
@@ -140,6 +158,7 @@ FUZZ_TARGET(connman, .init = initialize_connman)
     (void)connman.GetTotalBytesSent();
     (void)connman.GetTryNewOutboundPeer();
     (void)connman.GetUseAddrmanOutgoing();
+    (void)connman.ASMapHealthCheck();
 
     connman.ClearTestNodes();
 }

src/test/fuzz/util/net.h

@@ -5,6 +5,8 @@
 #ifndef BITCOIN_TEST_FUZZ_UTIL_NET_H
 #define BITCOIN_TEST_FUZZ_UTIL_NET_H
 
+#include <addrman.h>
+#include <addrman_impl.h>
 #include <net.h>
 #include <net_permissions.h>
 #include <netaddress.h>
@@ -15,6 +17,7 @@
 #include <test/fuzz/util.h>
 #include <test/util/net.h>
 #include <threadsafety.h>
+#include <util/asmap.h>
 #include <util/sock.h>
 
 #include <chrono>
@@ -34,6 +37,108 @@
  */
 CNetAddr ConsumeNetAddr(FuzzedDataProvider& fuzzed_data_provider, FastRandomContext* rand = nullptr) noexcept;
 
+class AddrManDeterministic : public AddrMan
+{
+public:
+    explicit AddrManDeterministic(const NetGroupManager& netgroupman, FuzzedDataProvider& fuzzed_data_provider, int32_t check_ratio)
+        : AddrMan(netgroupman, /*deterministic=*/true, check_ratio)
+    {
+        WITH_LOCK(m_impl->cs, m_impl->insecure_rand.Reseed(ConsumeUInt256(fuzzed_data_provider)));
+    }
+
+    /**
+     * Compare with another AddrMan.
+     * This compares:
+     * - the values in `mapInfo` (the keys aka ids are ignored)
+     * - vvNew entries refer to the same addresses
+     * - vvTried entries refer to the same addresses
+     */
+    bool operator==(const AddrManDeterministic& other) const
+    {
+        LOCK2(m_impl->cs, other.m_impl->cs);
+
+        if (m_impl->mapInfo.size() != other.m_impl->mapInfo.size() || m_impl->nNew != other.m_impl->nNew ||
+            m_impl->nTried != other.m_impl->nTried) {
+            return false;
+        }
+
+        // Check that all values in `mapInfo` are equal to all values in `other.mapInfo`.
+        // Keys may be different.
+
+        auto addrinfo_hasher = [](const AddrInfo& a) {
+            CSipHasher hasher(0, 0);
+            auto addr_key = a.GetKey();
+            auto source_key = a.source.GetAddrBytes();
+            hasher.Write(TicksSinceEpoch<std::chrono::seconds>(a.m_last_success));
+            hasher.Write(a.nAttempts);
+            hasher.Write(a.nRefCount);
+            hasher.Write(a.fInTried);
+            hasher.Write(a.GetNetwork());
+            hasher.Write(a.source.GetNetwork());
+            hasher.Write(addr_key.size());
+            hasher.Write(source_key.size());
+            hasher.Write(addr_key);
+            hasher.Write(source_key);
+            return (size_t)hasher.Finalize();
+        };
+
+        auto addrinfo_eq = [](const AddrInfo& lhs, const AddrInfo& rhs) {
+            return std::tie(static_cast<const CService&>(lhs), lhs.source, lhs.m_last_success, lhs.nAttempts, lhs.nRefCount, lhs.fInTried) ==
+                   std::tie(static_cast<const CService&>(rhs), rhs.source, rhs.m_last_success, rhs.nAttempts, rhs.nRefCount, rhs.fInTried);
+        };
+
+        using Addresses = std::unordered_set<AddrInfo, decltype(addrinfo_hasher), decltype(addrinfo_eq)>;
+
+        const size_t num_addresses{m_impl->mapInfo.size()};
+
+        Addresses addresses{num_addresses, addrinfo_hasher, addrinfo_eq};
+        for (const auto& [id, addr] : m_impl->mapInfo) {
+            addresses.insert(addr);
+        }
+
+        Addresses other_addresses{num_addresses, addrinfo_hasher, addrinfo_eq};
+        for (const auto& [id, addr] : other.m_impl->mapInfo) {
+            other_addresses.insert(addr);
+        }
+
+        if (addresses != other_addresses) {
+            return false;
+        }
+
+        auto IdsReferToSameAddress = [&](nid_type id, nid_type other_id) EXCLUSIVE_LOCKS_REQUIRED(m_impl->cs, other.m_impl->cs) {
+            if (id == -1 && other_id == -1) {
+                return true;
+            }
+            if ((id == -1 && other_id != -1) || (id != -1 && other_id == -1)) {
+                return false;
+            }
+            return m_impl->mapInfo.at(id) == other.m_impl->mapInfo.at(other_id);
+        };
+
+        // Check that `vvNew` contains the same addresses as `other.vvNew`. Notice - `vvNew[i][j]`
+        // contains just an id and the address is to be found in `mapInfo.at(id)`. The ids
+        // themselves may differ between `vvNew` and `other.vvNew`.
+        for (size_t i = 0; i < ADDRMAN_NEW_BUCKET_COUNT; ++i) {
+            for (size_t j = 0; j < ADDRMAN_BUCKET_SIZE; ++j) {
+                if (!IdsReferToSameAddress(m_impl->vvNew[i][j], other.m_impl->vvNew[i][j])) {
+                    return false;
+                }
+            }
+        }
+
+        // Same for `vvTried`.
+        for (size_t i = 0; i < ADDRMAN_TRIED_BUCKET_COUNT; ++i) {
+            for (size_t j = 0; j < ADDRMAN_BUCKET_SIZE; ++j) {
+                if (!IdsReferToSameAddress(m_impl->vvTried[i][j], other.m_impl->vvTried[i][j])) {
+                    return false;
+                }
+            }
+        }
+
+        return true;
+    }
+};
+
 class FuzzedSock : public Sock
 {
     FuzzedDataProvider& m_fuzzed_data_provider;
@@ -93,6 +198,13 @@ class FuzzedSock : public Sock
     return FuzzedSock{fuzzed_data_provider};
 }
 
+[[nodiscard]] inline NetGroupManager ConsumeNetGroupManager(FuzzedDataProvider& fuzzed_data_provider) noexcept
+{
+    std::vector<bool> asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider);
+    if (!SanityCheckASMap(asmap, 128)) asmap.clear();
+    return NetGroupManager(asmap);
+}
+
 inline CSubNet ConsumeSubNet(FuzzedDataProvider& fuzzed_data_provider) noexcept
 {
     return {ConsumeNetAddr(fuzzed_data_provider), fuzzed_data_provider.ConsumeIntegral<uint8_t>()};