Merge bitcoin/bitcoin#21129: fuzz: check that ser+unser produces the same AddrMan

8765179 fuzz: check that ser+unser produces the same AddrMan (Vasil Dimov)
6408b24 fuzz: move init code to the CAddrManDeterministic constructor (Vasil Dimov)

Pull request description:

  Add a fuzz test that fills addrman with a pile of randomly generated addresses, serializes it to a stream, unserializes the stream to another addrman object and compares the two.

  Some discussion of this already happened at jnewbery/bitcoin#18.

ACKs for top commit:
  practicalswift:
    cr ACK 8765179
  jonatack:
    ACK 8765179 rebased to current master, reviewed, fuzz build, ran `FUZZ=addrman_serdeser src/test/fuzz/fuzz`

Tree-SHA512: 7eda79279f14f2649840bf752e575d7b02cbaad541f74f7254855ebd4a32da988f042d78aa9228983350283bb74dd0c71f51f04c0846889c3ba2f19f01a0c303

Author: MarcoFalke

src/addrman.h

@@ -58,6 +58,7 @@ class CAddrInfo : public CAddress
     mutable int nRandomPos{-1};
 
     friend class CAddrMan;
+    friend class CAddrManDeterministic;
 
 public:
 
@@ -778,6 +779,7 @@ class CAddrMan
     void SetServices_(const CService &addr, ServiceFlags nServices) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     friend class CAddrManTest;
+    friend class CAddrManDeterministic;
 };
 
 #endif // BITCOIN_ADDRMAN_H

src/test/fuzz/addrman.cpp

@@ -12,6 +12,7 @@
 #include <time.h>
 #include <util/asmap.h>
 
+#include <cassert>
 #include <cstdint>
 #include <optional>
 #include <string>
@@ -25,25 +26,208 @@ void initialize_addrman()
 class CAddrManDeterministic : public CAddrMan
 {
 public:
-    void MakeDeterministic(const uint256& random_seed)
+    FuzzedDataProvider& m_fuzzed_data_provider;
+
+    explicit CAddrManDeterministic(FuzzedDataProvider& fuzzed_data_provider)
+        : m_fuzzed_data_provider(fuzzed_data_provider)
+    {
+        WITH_LOCK(cs, insecure_rand = FastRandomContext{ConsumeUInt256(fuzzed_data_provider)});
+        if (fuzzed_data_provider.ConsumeBool()) {
+            m_asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider);
+            if (!SanityCheckASMap(m_asmap)) {
+                m_asmap.clear();
+            }
+        }
+    }
+
+    /**
+     * Generate a random address. Always returns a valid address.
+     */
+    CNetAddr RandAddr() EXCLUSIVE_LOCKS_REQUIRED(cs)
+    {
+        CNetAddr addr;
+        if (m_fuzzed_data_provider.remaining_bytes() > 1 && m_fuzzed_data_provider.ConsumeBool()) {
+            addr = ConsumeNetAddr(m_fuzzed_data_provider);
+        } else {
+            // The networks [1..6] correspond to CNetAddr::BIP155Network (private).
+            static const std::map<uint8_t, uint8_t> net_len_map = {{1, ADDR_IPV4_SIZE},
+                                                                   {2, ADDR_IPV6_SIZE},
+                                                                   {4, ADDR_TORV3_SIZE},
+                                                                   {5, ADDR_I2P_SIZE},
+                                                                   {6, ADDR_CJDNS_SIZE}};
+            uint8_t net = insecure_rand.randrange(5) + 1; // [1..5]
+            if (net == 3) {
+                net = 6;
+            }
+
+            CDataStream s(SER_NETWORK, PROTOCOL_VERSION | ADDRV2_FORMAT);
+
+            s << net;
+            s << insecure_rand.randbytes(net_len_map.at(net));
+
+            s >> addr;
+        }
+
+        // Return a dummy IPv4 5.5.5.5 if we generated an invalid address.
+        if (!addr.IsValid()) {
+            in_addr v4_addr = {};
+            v4_addr.s_addr = 0x05050505;
+            addr = CNetAddr{v4_addr};
+        }
+
+        return addr;
+    }
+
+    /**
+     * Fill this addrman with lots of addresses from lots of sources.
+     */
+    void Fill()
     {
-        WITH_LOCK(cs, insecure_rand = FastRandomContext{random_seed});
-        Clear();
+        LOCK(cs);
+
+        // Add some of the addresses directly to the "tried" table.
+
+        // 0, 1, 2, 3 corresponding to 0%, 100%, 50%, 33%
+        const size_t n = m_fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 3);
+
+        const size_t num_sources = m_fuzzed_data_provider.ConsumeIntegralInRange<size_t>(10, 50);
+        CNetAddr prev_source;
+        // Use insecure_rand inside the loops instead of m_fuzzed_data_provider because when
+        // the latter is exhausted it just returns 0.
+        for (size_t i = 0; i < num_sources; ++i) {
+            const auto source = RandAddr();
+            const size_t num_addresses = insecure_rand.randrange(500) + 1; // [1..500]
+
+            for (size_t j = 0; j < num_addresses; ++j) {
+                const auto addr = CAddress{CService{RandAddr(), 8333}, NODE_NETWORK};
+                const auto time_penalty = insecure_rand.randrange(100000001);
+#if 1
+                // 2.83 sec to fill.
+                if (n > 0 && mapInfo.size() % n == 0 && mapAddr.find(addr) == mapAddr.end()) {
+                    // Add to the "tried" table (if the bucket slot is free).
+                    const CAddrInfo dummy{addr, source};
+                    const int bucket = dummy.GetTriedBucket(nKey, m_asmap);
+                    const int bucket_pos = dummy.GetBucketPosition(nKey, false, bucket);
+                    if (vvTried[bucket][bucket_pos] == -1) {
+                        int id;
+                        CAddrInfo* addr_info = Create(addr, source, &id);
+                        vvTried[bucket][bucket_pos] = id;
+                        addr_info->fInTried = true;
+                        ++nTried;
+                    }
+                } else {
+                    // Add to the "new" table.
+                    Add_(addr, source, time_penalty);
+                }
+#else
+                // 261.91 sec to fill.
+                Add_(addr, source, time_penalty);
+                if (n > 0 && mapInfo.size() % n == 0) {
+                    Good_(addr, false, GetTime());
+                }
+#endif
+                // Add 10% of the addresses from more than one source.
+                if (insecure_rand.randrange(10) == 0 && prev_source.IsValid()) {
+                    Add_(addr, prev_source, time_penalty);
+                }
+            }
+            prev_source = source;
+        }
+    }
+
+    /**
+     * 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 CAddrManDeterministic& other)
+    {
+        LOCK2(cs, other.cs);
+
+        if (mapInfo.size() != other.mapInfo.size() || nNew != other.nNew ||
+            nTried != other.nTried) {
+            return false;
+        }
+
+        // Check that all values in `mapInfo` are equal to all values in `other.mapInfo`.
+        // Keys may be different.
+
+        using CAddrInfoHasher = std::function<size_t(const CAddrInfo&)>;
+        using CAddrInfoEq = std::function<bool(const CAddrInfo&, const CAddrInfo&)>;
+
+        CNetAddrHash netaddr_hasher;
+
+        CAddrInfoHasher addrinfo_hasher = [&netaddr_hasher](const CAddrInfo& a) {
+            return netaddr_hasher(static_cast<CNetAddr>(a)) ^ netaddr_hasher(a.source) ^
+                   a.nLastSuccess ^ a.nAttempts ^ a.nRefCount ^ a.fInTried;
+        };
+
+        CAddrInfoEq addrinfo_eq = [](const CAddrInfo& lhs, const CAddrInfo& rhs) {
+            return static_cast<CNetAddr>(lhs) == static_cast<CNetAddr>(rhs) &&
+                   lhs.source == rhs.source && lhs.nLastSuccess == rhs.nLastSuccess &&
+                   lhs.nAttempts == rhs.nAttempts && lhs.nRefCount == rhs.nRefCount &&
+                   lhs.fInTried == rhs.fInTried;
+        };
+
+        using Addresses = std::unordered_set<CAddrInfo, CAddrInfoHasher, CAddrInfoEq>;
+
+        const size_t num_addresses{mapInfo.size()};
+
+        Addresses addresses{num_addresses, addrinfo_hasher, addrinfo_eq};
+        for (const auto& [id, addr] : mapInfo) {
+            addresses.insert(addr);
+        }
+
+        Addresses other_addresses{num_addresses, addrinfo_hasher, addrinfo_eq};
+        for (const auto& [id, addr] : other.mapInfo) {
+            other_addresses.insert(addr);
+        }
+
+        if (addresses != other_addresses) {
+            return false;
+        }
+
+        auto IdsReferToSameAddress = [&](int id, int other_id) EXCLUSIVE_LOCKS_REQUIRED(cs, other.cs) {
+            if (id == -1 && other_id == -1) {
+                return true;
+            }
+            if ((id == -1 && other_id != -1) || (id != -1 && other_id == -1)) {
+                return false;
+            }
+            return mapInfo.at(id) == other.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(vvNew[i][j], other.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(vvTried[i][j], other.vvTried[i][j])) {
+                    return false;
+                }
+            }
+        }
+
+        return true;
     }
 };
 
 FUZZ_TARGET_INIT(addrman, initialize_addrman)
 {
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
     SetMockTime(ConsumeTime(fuzzed_data_provider));
-    CAddrManDeterministic addr_man;
-    addr_man.MakeDeterministic(ConsumeUInt256(fuzzed_data_provider));
-    if (fuzzed_data_provider.ConsumeBool()) {
-        addr_man.m_asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider);
-        if (!SanityCheckASMap(addr_man.m_asmap)) {
-            addr_man.m_asmap.clear();
-        }
-    }
+    CAddrManDeterministic addr_man{fuzzed_data_provider};
     if (fuzzed_data_provider.ConsumeBool()) {
         const std::vector<uint8_t> serialized_data{ConsumeRandomLengthByteVector(fuzzed_data_provider)};
         CDataStream ds(serialized_data, SER_DISK, INIT_PROTO_VERSION);
@@ -123,3 +307,21 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman)
     CDataStream data_stream(SER_NETWORK, PROTOCOL_VERSION);
     data_stream << const_addr_man;
 }
+
+// Check that serialize followed by unserialize produces the same addrman.
+FUZZ_TARGET_INIT(addrman_serdeser, initialize_addrman)
+{
+    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
+    SetMockTime(ConsumeTime(fuzzed_data_provider));
+
+    CAddrManDeterministic addr_man1{fuzzed_data_provider};
+    CAddrManDeterministic addr_man2{fuzzed_data_provider};
+    addr_man2.m_asmap = addr_man1.m_asmap;
+
+    CDataStream data_stream(SER_NETWORK, PROTOCOL_VERSION);
+
+    addr_man1.Fill();
+    data_stream << addr_man1;
+    data_stream >> addr_man2;
+    assert(addr_man1 == addr_man2);
+}