From a749fa539ae4330dd5d610286f418156e080e9dd Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 1 Nov 2021 15:32:19 +0000 Subject: [PATCH 1/5] [addrman] Remove AddrMan friends AddrMan's friends both inherit from AddrMan, so just make the private member protected and remove the friends. --- src/addrman.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/addrman.h b/src/addrman.h index a9f697f66f3..455d84ca71b 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -53,6 +53,7 @@ static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{0}; */ class AddrMan { +protected: const std::unique_ptr m_impl; public: @@ -135,9 +136,6 @@ public: void SetServices(const CService& addr, ServiceFlags nServices); const std::vector& GetAsmap() const; - - friend class AddrManTest; - friend class AddrManDeterministic; }; #endif // BITCOIN_ADDRMAN_H From 7784a9a374ac34acb4656f740fecf4ae1743f73f Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 1 Nov 2021 15:33:19 +0000 Subject: [PATCH 2/5] [addrman] [tests] Remove deterministic argument and member from AddrManTest It's always set to true. --- src/test/addrman_tests.cpp | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index eabc11c4677..f74906580f2 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -79,15 +79,10 @@ static CDataStream AddrmanToStream(const AddrManSerializationMock& _addrman) class AddrManTest : public AddrMan { -private: - bool deterministic; public: - explicit AddrManTest(bool makeDeterministic = true, - std::vector asmap = std::vector()) - : AddrMan(asmap, makeDeterministic, /* consistency_check_ratio */ 100) - { - deterministic = makeDeterministic; - } + explicit AddrManTest(std::vector asmap = std::vector()) + : AddrMan(asmap, /*deterministic=*/true, /* consistency_check_ratio */ 100) + {} AddrInfo* Find(const CService& addr, int* pnId = nullptr) { @@ -760,8 +755,8 @@ BOOST_AUTO_TEST_CASE(addrman_serialization) { std::vector asmap1 = FromBytes(asmap_raw, sizeof(asmap_raw) * 8); - auto addrman_asmap1 = std::make_unique(true, asmap1); - auto addrman_asmap1_dup = std::make_unique(true, asmap1); + auto addrman_asmap1 = std::make_unique(asmap1); + auto addrman_asmap1_dup = std::make_unique(asmap1); auto addrman_noasmap = std::make_unique(); CDataStream stream(SER_NETWORK, PROTOCOL_VERSION); @@ -792,7 +787,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(true, asmap1); + addrman_asmap1 = std::make_unique(asmap1); addrman_noasmap = std::make_unique(); addrman_noasmap->Add({addr}, default_source); stream << *addrman_noasmap; @@ -804,7 +799,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(true, asmap1); + addrman_asmap1 = std::make_unique(asmap1); addrman_noasmap = std::make_unique(); CAddress addr1 = CAddress(ResolveService("250.1.1.1"), NODE_NONE); CAddress addr2 = CAddress(ResolveService("250.2.1.1"), NODE_NONE); From d02098d1f042ff91c2206c27c48b385418ece0cc Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 1 Nov 2021 15:37:19 +0000 Subject: [PATCH 3/5] [addrman] [tests] Tidy up unused arguments in addrman test functions --- src/test/addrman_tests.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index f74906580f2..8c6c9415bd4 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -84,13 +84,13 @@ public: : 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); From dfbd3a6d71f17bf3fbcf88e46e3fedd18a7068f1 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 4 Nov 2021 11:30:29 +0000 Subject: [PATCH 4/5] [addrman] [tests] Remove AddrManCorrupted subclass It's only used to create a corrupted peers.dat file. We can do that directly in a pure function. --- src/test/addrman_tests.cpp | 62 +++++++++++++++++++------------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 8c6c9415bd4..dab68b497ff 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -41,32 +41,6 @@ public: } }; -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); @@ -1043,13 +1017,39 @@ BOOST_AUTO_TEST_CASE(load_addrman) 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 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(), /* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman1.size() == 0); @@ -1065,7 +1065,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(), /* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman2.size() == 0); From 36d3510303875c9f98eb00b28763c7c043d4dcee Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 4 Nov 2021 11:31:17 +0000 Subject: [PATCH 5/5] [addrman] [tests] Remove AddrManUncorrupted subclass It doesn't do anything different from the base AddrMan class. --- src/test/addrman_tests.cpp | 50 ++++++++++++-------------------------- 1 file changed, 16 insertions(+), 34 deletions(-) diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index dab68b497ff..572b9871dc6 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -22,35 +22,6 @@ using namespace std::literals; -class AddrManSerializationMock : public AddrMan -{ -public: - virtual void Serialize(CDataStream& s) const = 0; - - AddrManSerializationMock() - : AddrMan(/* asmap */ std::vector(), /* deterministic */ true, /* consistency_check_ratio */ 100) - {} -}; - -class AddrManUncorrupted : public AddrManSerializationMock -{ -public: - void Serialize(CDataStream& s) const override - { - AddrMan::Serialize(s); - } -}; - -static CDataStream AddrmanToStream(const AddrManSerializationMock& _addrman) -{ - CDataStream ssPeersIn(SER_DISK, CLIENT_VERSION); - ssPeersIn << Params().MessageStart(); - ssPeersIn << _addrman; - std::string str = ssPeersIn.str(); - std::vector vchData(str.begin(), str.end()); - return CDataStream(vchData, SER_DISK, CLIENT_VERSION); -} - class AddrManTest : public AddrMan { public: @@ -973,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 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(), /*deterministic=*/ true, + /*consistency_check_ratio=*/ 100}; CService addr1, addr2, addr3; BOOST_CHECK(Lookup("250.7.1.1", addr1, 8333, false)); @@ -988,11 +970,11 @@ BOOST_AUTO_TEST_CASE(load_addrman) CService source; BOOST_CHECK(Lookup("252.5.1.1", source, 8333, false)); std::vector 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(), /* deterministic */ false, /* consistency_check_ratio */ 100); @@ -1009,7 +991,7 @@ 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(), /* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman2.size() == 0);