Merge bitcoin/bitcoin#23477: addrman: tidy up unit tests

36d3510 [addrman] [tests] Remove AddrManUncorrupted subclass (John Newbery)
dfbd3a6 [addrman] [tests] Remove AddrManCorrupted subclass (John Newbery)
d02098d [addrman] [tests] Tidy up unused arguments in addrman test functions (John Newbery)
7784a9a [addrman] [tests] Remove deterministic argument and member from AddrManTest (John Newbery)
a749fa5 [addrman] Remove AddrMan friends (John Newbery)

Pull request description:

  Various tidy-ups to the addrman tests.

ACKs for top commit:
  shaavan:
    crACK 36d3510
  promag:
    Code review ACK 36d3510.
  theStack:
    Code-review ACK 36d3510

Tree-SHA512: bbdb9d70863c15b023714ba3c73e816c635204f949c39678dd932a6e9a2e57b51b5d50332ec6843cf1b98a2fcbbdd5e6779f2e9c7e9cf90f4a6b3b4a7a1abe2f

Author: MarcoFalke

Diff for: src/addrman.h

@@ -53,6 +53,7 @@ static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{0};
  */
 class AddrMan
 {
+protected:
     const std::unique_ptr<AddrManImpl> m_impl;
 
 public:
@@ -135,9 +136,6 @@ class AddrMan
     void SetServices(const CService& addr, ServiceFlags nServices);
 
     const std::vector<bool>& GetAsmap() const;
-
-    friend class AddrManTest;
-    friend class AddrManDeterministic;
 };
 
 #endif // BITCOIN_ADDRMAN_H

Diff for: src/test/addrman_tests.cpp

@@ -22,80 +22,20 @@
 
 using namespace std::literals;
 
-class AddrManSerializationMock : public AddrMan
-{
-public:
-    virtual void Serialize(CDataStream& s) const = 0;
-
-    AddrManSerializationMock()
-        : AddrMan(/* asmap */ std::vector<bool>(), /* deterministic */ true, /* consistency_check_ratio */ 100)
-    {}
-};
-
-class AddrManUncorrupted : public AddrManSerializationMock
-{
-public:
-    void Serialize(CDataStream& s) const override
-    {
-        AddrMan::Serialize(s);
-    }
-};
-
-class AddrManCorrupted : public AddrManSerializationMock
-{
-public:
-    void Serialize(CDataStream& s) const override
-    {
-        // Produces corrupt output that claims addrman has 20 addrs when it only has one addr.
-        unsigned char nVersion = 1;
-        s << nVersion;
-        s << ((unsigned char)32);
-        s << uint256::ONE;
-        s << 10; // nNew
-        s << 10; // nTried
-
-        int nUBuckets = ADDRMAN_NEW_BUCKET_COUNT ^ (1 << 30);
-        s << nUBuckets;
-
-        CService serv;
-        BOOST_CHECK(Lookup("252.1.1.1", serv, 7777, false));
-        CAddress addr = CAddress(serv, NODE_NONE);
-        CNetAddr resolved;
-        BOOST_CHECK(LookupHost("252.2.2.2", resolved, false));
-        AddrInfo info = AddrInfo(addr, resolved);
-        s << info;
-    }
-};
-
-static CDataStream AddrmanToStream(const AddrManSerializationMock& _addrman)
-{
-    CDataStream ssPeersIn(SER_DISK, CLIENT_VERSION);
-    ssPeersIn << Params().MessageStart();
-    ssPeersIn << _addrman;
-    std::string str = ssPeersIn.str();
-    std::vector<unsigned char> vchData(str.begin(), str.end());
-    return CDataStream(vchData, SER_DISK, CLIENT_VERSION);
-}
-
 class AddrManTest : public AddrMan
 {
-private:
-    bool deterministic;
 public:
-    explicit AddrManTest(bool makeDeterministic = true,
-                         std::vector<bool> asmap = std::vector<bool>())
-        : AddrMan(asmap, makeDeterministic, /* consistency_check_ratio */ 100)
-    {
-        deterministic = makeDeterministic;
-    }
+    explicit AddrManTest(std::vector<bool> asmap = std::vector<bool>())
+        : AddrMan(asmap, /*deterministic=*/true, /* consistency_check_ratio */ 100)
+    {}
 
-    AddrInfo* Find(const CService& addr, int* pnId = nullptr)
+    AddrInfo* Find(const CService& addr)
     {
         LOCK(m_impl->cs);
-        return m_impl->Find(addr, pnId);
+        return m_impl->Find(addr);
     }
 
-    AddrInfo* Create(const CAddress& addr, const CNetAddr& addrSource, int* pnId = nullptr)
+    AddrInfo* Create(const CAddress& addr, const CNetAddr& addrSource, int* pnId)
     {
         LOCK(m_impl->cs);
         return m_impl->Create(addr, addrSource, pnId);
@@ -760,8 +700,8 @@ BOOST_AUTO_TEST_CASE(addrman_serialization)
 {
     std::vector<bool> asmap1 = FromBytes(asmap_raw, sizeof(asmap_raw) * 8);
 
-    auto addrman_asmap1 = std::make_unique<AddrManTest>(true, asmap1);
-    auto addrman_asmap1_dup = std::make_unique<AddrManTest>(true, asmap1);
+    auto addrman_asmap1 = std::make_unique<AddrManTest>(asmap1);
+    auto addrman_asmap1_dup = std::make_unique<AddrManTest>(asmap1);
     auto addrman_noasmap = std::make_unique<AddrManTest>();
     CDataStream stream(SER_NETWORK, PROTOCOL_VERSION);
 
@@ -792,7 +732,7 @@ BOOST_AUTO_TEST_CASE(addrman_serialization)
     BOOST_CHECK(bucketAndEntry_asmap1.second != bucketAndEntry_noasmap.second);
 
     // deserializing non-asmaped peers.dat to asmaped addrman
-    addrman_asmap1 = std::make_unique<AddrManTest>(true, asmap1);
+    addrman_asmap1 = std::make_unique<AddrManTest>(asmap1);
     addrman_noasmap = std::make_unique<AddrManTest>();
     addrman_noasmap->Add({addr}, default_source);
     stream << *addrman_noasmap;
@@ -804,7 +744,7 @@ BOOST_AUTO_TEST_CASE(addrman_serialization)
     BOOST_CHECK(bucketAndEntry_asmap1_deser.second == bucketAndEntry_asmap1_dup.second);
 
     // used to map to different buckets, now maps to the same bucket.
-    addrman_asmap1 = std::make_unique<AddrManTest>(true, asmap1);
+    addrman_asmap1 = std::make_unique<AddrManTest>(asmap1);
     addrman_noasmap = std::make_unique<AddrManTest>();
     CAddress addr1 = CAddress(ResolveService("250.1.1.1"), NODE_NONE);
     CAddress addr2 = CAddress(ResolveService("250.2.1.1"), NODE_NONE);
@@ -1004,9 +944,20 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks)
     BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0");
 }
 
+static CDataStream AddrmanToStream(const AddrMan& addrman)
+{
+    CDataStream ssPeersIn(SER_DISK, CLIENT_VERSION);
+    ssPeersIn << Params().MessageStart();
+    ssPeersIn << addrman;
+    std::string str = ssPeersIn.str();
+    std::vector<unsigned char> vchData(str.begin(), str.end());
+    return CDataStream(vchData, SER_DISK, CLIENT_VERSION);
+}
+
 BOOST_AUTO_TEST_CASE(load_addrman)
 {
-    AddrManUncorrupted addrmanUncorrupted;
+    AddrMan addrman{/*asmap=*/ std::vector<bool>(), /*deterministic=*/ true,
+                    /*consistency_check_ratio=*/ 100};
 
     CService addr1, addr2, addr3;
     BOOST_CHECK(Lookup("250.7.1.1", addr1, 8333, false));
@@ -1019,11 +970,11 @@ BOOST_AUTO_TEST_CASE(load_addrman)
     CService source;
     BOOST_CHECK(Lookup("252.5.1.1", source, 8333, false));
     std::vector<CAddress> addresses{CAddress(addr1, NODE_NONE), CAddress(addr2, NODE_NONE), CAddress(addr3, NODE_NONE)};
-    BOOST_CHECK(addrmanUncorrupted.Add(addresses, source));
-    BOOST_CHECK(addrmanUncorrupted.size() == 3);
+    BOOST_CHECK(addrman.Add(addresses, source));
+    BOOST_CHECK(addrman.size() == 3);
 
     // Test that the de-serialization does not throw an exception.
-    CDataStream ssPeers1 = AddrmanToStream(addrmanUncorrupted);
+    CDataStream ssPeers1 = AddrmanToStream(addrman);
     bool exceptionThrown = false;
     AddrMan addrman1(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 100);
 
@@ -1040,21 +991,47 @@ BOOST_AUTO_TEST_CASE(load_addrman)
     BOOST_CHECK(exceptionThrown == false);
 
     // Test that ReadFromStream creates an addrman with the correct number of addrs.
-    CDataStream ssPeers2 = AddrmanToStream(addrmanUncorrupted);
+    CDataStream ssPeers2 = AddrmanToStream(addrman);
 
     AddrMan addrman2(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 100);
     BOOST_CHECK(addrman2.size() == 0);
     ReadFromStream(addrman2, ssPeers2);
     BOOST_CHECK(addrman2.size() == 3);
 }
 
+// Produce a corrupt peers.dat that claims 20 addrs when it only has one addr.
+static CDataStream MakeCorruptPeersDat()
+{
+    CDataStream s(SER_DISK, CLIENT_VERSION);
+    s << ::Params().MessageStart();
+
+    unsigned char nVersion = 1;
+    s << nVersion;
+    s << ((unsigned char)32);
+    s << uint256::ONE;
+    s << 10; // nNew
+    s << 10; // nTried
+
+    int nUBuckets = ADDRMAN_NEW_BUCKET_COUNT ^ (1 << 30);
+    s << nUBuckets;
+
+    CService serv;
+    BOOST_CHECK(Lookup("252.1.1.1", serv, 7777, false));
+    CAddress addr = CAddress(serv, NODE_NONE);
+    CNetAddr resolved;
+    BOOST_CHECK(LookupHost("252.2.2.2", resolved, false));
+    AddrInfo info = AddrInfo(addr, resolved);
+    s << info;
+
+    std::string str = s.str();
+    std::vector<unsigned char> vchData(str.begin(), str.end());
+    return CDataStream(vchData, SER_DISK, CLIENT_VERSION);
+}
 
 BOOST_AUTO_TEST_CASE(load_addrman_corrupted)
 {
-    AddrManCorrupted addrmanCorrupted;
-
-    // Test that the de-serialization of corrupted addrman throws an exception.
-    CDataStream ssPeers1 = AddrmanToStream(addrmanCorrupted);
+    // Test that the de-serialization of corrupted peers.dat throws an exception.
+    CDataStream ssPeers1 = MakeCorruptPeersDat();
     bool exceptionThrown = false;
     AddrMan addrman1(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 100);
     BOOST_CHECK(addrman1.size() == 0);
@@ -1070,7 +1047,7 @@ BOOST_AUTO_TEST_CASE(load_addrman_corrupted)
     BOOST_CHECK(exceptionThrown);
 
     // Test that ReadFromStream fails if peers.dat is corrupt
-    CDataStream ssPeers2 = AddrmanToStream(addrmanCorrupted);
+    CDataStream ssPeers2 = MakeCorruptPeersDat();
 
     AddrMan addrman2(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 100);
     BOOST_CHECK(addrman2.size() == 0);