Merge bitcoin/bitcoin#20233: addrman: Make consistency checks a runtime option

a4d78546b0858602c60c03fdf8b35ca666ab2e56 [addrman] Make addrman consistency checks a runtime option (John Newbery)
10aac241455a3270462d49b53732477ed97623e7 [tests] Make deterministic addrman use nKey = 1 (John Newbery)
fa9710f62c29c7f8d71c9f281001c9b5e70946bf [addrman] Add deterministic argument to CAddrMan ctor (John Newbery)
ee458d84fc187d69f002ebead6fccc4f4f9c0744 Add missing const to CAddrMan::Check_() (MarcoFalke)

Pull request description:

  CAddrMan has internal consistency checks. Currently, these are only run when the program is compiled with the  `DEBUG_ADDRMAN` option. This option is not enabled on any of our CI builds, and it's likely that no-one is running them at all.

  This PR makes consistency checks a (hidden) runtime option that can be enabled with `-checkaddrman`, where `-checkaddrman=n` will result in the consistency checks running every n operations (similar to `-checkmempool=n`). We set the ratio to 1/100 for our unit tests, and leave it disabled by default for all networks. Additionally, a consistency check failure now asserts, rather than logging and continuing. This matches the behavior of CTxMemPool and TxRequestTracker, where a failed consistency check asserts.

ACKs for top commit:
  jonatack:
    ACK a4d78546b0858602c60c03fdf8b35ca666ab2e56 per `git diff 00fd089 a4d7854`, tested by adding logging similar to #22479 and running with `-checkaddrman=<n>` for various values 0/1/10/100 etc, tested the updated docs with `bitcoind -help-debug | grep -A2 "checkaddrman\|checkmempool"` and verified rebased on master that compiling with `CPPFLAGS="-DDEBUG_ADDRMAN"` no longer causes the build to error.
  mzumsande:
    Code-review ACK a4d78546b0858602c60c03fdf8b35ca666ab2e56
  theStack:
    Code-review ACK a4d78546b0858602c60c03fdf8b35ca666ab2e56

Tree-SHA512: eaee003f7a99154822c5b5efbc62008d32c1efbecc6fec6e183427f6b2ae5d30b3be7924e3a7271b1a1de91517f5bd2a70011d45358c3105c6a0702f12b70f7c

7 files changed, 66 insertions, 76 deletions

diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp
index 79c7102c4f..3c64461605 100644
--- a/src/test/addrman_tests.cpp
+++ b/src/test/addrman_tests.cpp
@@ -21,24 +21,13 @@ private:
     bool deterministic;
 public:
     explicit CAddrManTest(bool makeDeterministic = true,
-        std::vector<bool> asmap = std::vector<bool>())
+                          std::vector<bool> asmap = std::vector<bool>())
+        : CAddrMan(makeDeterministic, /* consistency_check_ratio */ 100)
     {
-        if (makeDeterministic) {
-            //  Set addrman addr placement to be deterministic.
-            MakeDeterministic();
-        }
         deterministic = makeDeterministic;
         m_asmap = asmap;
     }
 
-    //! Ensure that bucket placement is always the same for testing purposes.
-    void MakeDeterministic()
-    {
-        LOCK(cs);
-        nKey.SetNull();
-        insecure_rand = FastRandomContext(true);
-    }
-
     CAddrInfo* Find(const CNetAddr& addr, int* pnId = nullptr)
     {
         LOCK(cs);
@@ -89,7 +78,7 @@ public:
         CAddrMan::Clear();
         if (deterministic) {
             LOCK(cs);
-            nKey.SetNull();
+            nKey = uint256{1};
             insecure_rand = FastRandomContext(true);
         }
     }
@@ -267,24 +256,27 @@ BOOST_AUTO_TEST_CASE(addrman_new_collisions)
 
     CNetAddr source = ResolveIP("252.2.2.2");
 
-    BOOST_CHECK_EQUAL(addrman.size(), 0U);
+    uint32_t num_addrs{0};
+
+    BOOST_CHECK_EQUAL(addrman.size(), num_addrs);
 
-    for (unsigned int i = 1; i < 18; i++) {
-        CService addr = ResolveService("250.1.1." + ToString(i));
+    while (num_addrs < 22) {  // Magic number! 250.1.1.1 - 250.1.1.22 do not collide with deterministic key = 1
+        CService addr = ResolveService("250.1.1." + ToString(++num_addrs));
         BOOST_CHECK(addrman.Add(CAddress(addr, NODE_NONE), source));
 
         //Test: No collision in new table yet.
-        BOOST_CHECK_EQUAL(addrman.size(), i);
+        BOOST_CHECK_EQUAL(addrman.size(), num_addrs);
     }
 
     //Test: new table collision!
-    CService addr1 = ResolveService("250.1.1.18");
+    CService addr1 = ResolveService("250.1.1." + ToString(++num_addrs));
+    uint32_t collisions{1};
     BOOST_CHECK(addrman.Add(CAddress(addr1, NODE_NONE), source));
-    BOOST_CHECK_EQUAL(addrman.size(), 17U);
+    BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions);
 
-    CService addr2 = ResolveService("250.1.1.19");
+    CService addr2 = ResolveService("250.1.1." + ToString(++num_addrs));
     BOOST_CHECK(addrman.Add(CAddress(addr2, NODE_NONE), source));
-    BOOST_CHECK_EQUAL(addrman.size(), 18U);
+    BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions);
 }
 
 BOOST_AUTO_TEST_CASE(addrman_tried_collisions)
@@ -293,25 +285,28 @@ BOOST_AUTO_TEST_CASE(addrman_tried_collisions)
 
     CNetAddr source = ResolveIP("252.2.2.2");
 
-    BOOST_CHECK_EQUAL(addrman.size(), 0U);
+    uint32_t num_addrs{0};
+
+    BOOST_CHECK_EQUAL(addrman.size(), num_addrs);
 
-    for (unsigned int i = 1; i < 80; i++) {
-        CService addr = ResolveService("250.1.1." + ToString(i));
+    while (num_addrs < 64) {  // Magic number! 250.1.1.1 - 250.1.1.64 do not collide with deterministic key = 1
+        CService addr = ResolveService("250.1.1." + ToString(++num_addrs));
         BOOST_CHECK(addrman.Add(CAddress(addr, NODE_NONE), source));
         addrman.Good(CAddress(addr, NODE_NONE));
 
         //Test: No collision in tried table yet.
-        BOOST_CHECK_EQUAL(addrman.size(), i);
+        BOOST_CHECK_EQUAL(addrman.size(), num_addrs);
     }
 
     //Test: tried table collision!
-    CService addr1 = ResolveService("250.1.1.80");
+    CService addr1 = ResolveService("250.1.1." + ToString(++num_addrs));
+    uint32_t collisions{1};
     BOOST_CHECK(addrman.Add(CAddress(addr1, NODE_NONE), source));
-    BOOST_CHECK_EQUAL(addrman.size(), 79U);
+    BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions);
 
-    CService addr2 = ResolveService("250.1.1.81");
+    CService addr2 = ResolveService("250.1.1." + ToString(++num_addrs));
     BOOST_CHECK(addrman.Add(CAddress(addr2, NODE_NONE), source));
-    BOOST_CHECK_EQUAL(addrman.size(), 80U);
+    BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions);
 }
 
 BOOST_AUTO_TEST_CASE(addrman_find)
@@ -861,9 +856,9 @@ BOOST_AUTO_TEST_CASE(addrman_noevict)
 {
     CAddrManTest addrman;
 
-    // Add twenty two addresses.
+    // Add 35 addresses.
     CNetAddr source = ResolveIP("252.2.2.2");
-    for (unsigned int i = 1; i < 23; i++) {
+    for (unsigned int i = 1; i < 36; i++) {
         CService addr = ResolveService("250.1.1."+ToString(i));
         BOOST_CHECK(addrman.Add(CAddress(addr, NODE_NONE), source));
         addrman.Good(addr);
@@ -873,20 +868,20 @@ BOOST_AUTO_TEST_CASE(addrman_noevict)
         BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
     }
 
-    // Collision between 23 and 19.
-    CService addr23 = ResolveService("250.1.1.23");
-    BOOST_CHECK(addrman.Add(CAddress(addr23, NODE_NONE), source));
-    addrman.Good(addr23);
+    // Collision between 36 and 19.
+    CService addr36 = ResolveService("250.1.1.36");
+    BOOST_CHECK(addrman.Add(CAddress(addr36, NODE_NONE), source));
+    addrman.Good(addr36);
 
-    BOOST_CHECK(addrman.size() == 23);
-    BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "250.1.1.19:0");
+    BOOST_CHECK(addrman.size() == 36);
+    BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().ToString(), "250.1.1.19:0");
 
-    // 23 should be discarded and 19 not evicted.
+    // 36 should be discarded and 19 not evicted.
     addrman.ResolveCollisions();
     BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
 
     // Lets create two collisions.
-    for (unsigned int i = 24; i < 33; i++) {
+    for (unsigned int i = 37; i < 59; i++) {
         CService addr = ResolveService("250.1.1."+ToString(i));
         BOOST_CHECK(addrman.Add(CAddress(addr, NODE_NONE), source));
         addrman.Good(addr);
@@ -896,17 +891,17 @@ BOOST_AUTO_TEST_CASE(addrman_noevict)
     }
 
     // Cause a collision.
-    CService addr33 = ResolveService("250.1.1.33");
-    BOOST_CHECK(addrman.Add(CAddress(addr33, NODE_NONE), source));
-    addrman.Good(addr33);
-    BOOST_CHECK(addrman.size() == 33);
+    CService addr59 = ResolveService("250.1.1.59");
+    BOOST_CHECK(addrman.Add(CAddress(addr59, NODE_NONE), source));
+    addrman.Good(addr59);
+    BOOST_CHECK(addrman.size() == 59);
 
-    BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "250.1.1.27:0");
+    BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().ToString(), "250.1.1.10:0");
 
     // Cause a second collision.
-    BOOST_CHECK(!addrman.Add(CAddress(addr23, NODE_NONE), source));
-    addrman.Good(addr23);
-    BOOST_CHECK(addrman.size() == 33);
+    BOOST_CHECK(!addrman.Add(CAddress(addr36, NODE_NONE), source));
+    addrman.Good(addr36);
+    BOOST_CHECK(addrman.size() == 59);
 
     BOOST_CHECK(addrman.SelectTriedCollision().ToString() != "[::]:0");
     addrman.ResolveCollisions();
@@ -922,9 +917,9 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks)
     // Empty addrman should return blank addrman info.
     BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
 
-    // Add twenty two addresses.
+    // Add 35 addresses
     CNetAddr source = ResolveIP("252.2.2.2");
-    for (unsigned int i = 1; i < 23; i++) {
+    for (unsigned int i = 1; i < 36; i++) {
         CService addr = ResolveService("250.1.1."+ToString(i));
         BOOST_CHECK(addrman.Add(CAddress(addr, NODE_NONE), source));
         addrman.Good(addr);
@@ -934,34 +929,34 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks)
         BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
     }
 
-    // Collision between 23 and 19.
-    CService addr = ResolveService("250.1.1.23");
+    // Collision between 36 and 19.
+    CService addr = ResolveService("250.1.1.36");
     BOOST_CHECK(addrman.Add(CAddress(addr, NODE_NONE), source));
     addrman.Good(addr);
 
-    BOOST_CHECK(addrman.size() == 23);
+    BOOST_CHECK_EQUAL(addrman.size(), 36);
     CAddrInfo info = addrman.SelectTriedCollision();
-    BOOST_CHECK(info.ToString() == "250.1.1.19:0");
+    BOOST_CHECK_EQUAL(info.ToString(), "250.1.1.19:0");
 
     // Ensure test of address fails, so that it is evicted.
     addrman.SimConnFail(info);
 
-    // Should swap 23 for 19.
+    // Should swap 36 for 19.
     addrman.ResolveCollisions();
     BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
 
-    // If 23 was swapped for 19, then this should cause no collisions.
+    // If 36 was swapped for 19, then this should cause no collisions.
     BOOST_CHECK(!addrman.Add(CAddress(addr, NODE_NONE), source));
     addrman.Good(addr);
 
     BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
 
-    // If we insert 19 is should collide with 23.
+    // If we insert 19 it should collide with 36
     CService addr19 = ResolveService("250.1.1.19");
     BOOST_CHECK(!addrman.Add(CAddress(addr19, NODE_NONE), source));
     addrman.Good(addr19);
 
-    BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "250.1.1.23:0");
+    BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().ToString(), "250.1.1.36:0");
 
     addrman.ResolveCollisions();
     BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp
index 4c29a8ee53..60fba5730a 100644
--- a/src/test/fuzz/addrman.cpp
+++ b/src/test/fuzz/addrman.cpp
@@ -29,7 +29,8 @@ public:
     FuzzedDataProvider& m_fuzzed_data_provider;
 
     explicit CAddrManDeterministic(FuzzedDataProvider& fuzzed_data_provider)
-        : m_fuzzed_data_provider(fuzzed_data_provider)
+        : CAddrMan(/* deterministic */ true, /* consistency_check_ratio */ 0)
+        , m_fuzzed_data_provider(fuzzed_data_provider)
     {
         WITH_LOCK(cs, insecure_rand = FastRandomContext{ConsumeUInt256(fuzzed_data_provider)});
         if (fuzzed_data_provider.ConsumeBool()) {
diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp
index bbec5943af..0e323ddc20 100644
--- a/src/test/fuzz/connman.cpp
+++ b/src/test/fuzz/connman.cpp
@@ -25,7 +25,7 @@ FUZZ_TARGET_INIT(connman, initialize_connman)
 {
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
     SetMockTime(ConsumeTime(fuzzed_data_provider));
-    CAddrMan addrman;
+    CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0);
     CConnman connman{fuzzed_data_provider.ConsumeIntegral<uint64_t>(), fuzzed_data_provider.ConsumeIntegral<uint64_t>(), addrman, fuzzed_data_provider.ConsumeBool()};
     CNetAddr random_netaddr;
     CNode random_node = ConsumeNode(fuzzed_data_provider);
diff --git a/src/test/fuzz/data_stream.cpp b/src/test/fuzz/data_stream.cpp
index 473caec6ff..53400082ab 100644
--- a/src/test/fuzz/data_stream.cpp
+++ b/src/test/fuzz/data_stream.cpp
@@ -21,6 +21,6 @@ FUZZ_TARGET_INIT(data_stream_addr_man, initialize_data_stream_addr_man)
 {
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
     CDataStream data_stream = ConsumeDataStream(fuzzed_data_provider);
-    CAddrMan addr_man;
+    CAddrMan addr_man(/* deterministic */ false, /* consistency_check_ratio */ 0);
     CAddrDB::Read(addr_man, data_stream);
 }
diff --git a/src/test/fuzz/deserialize.cpp b/src/test/fuzz/deserialize.cpp
index d5b56cb7cd..49503e8dc6 100644
--- a/src/test/fuzz/deserialize.cpp
+++ b/src/test/fuzz/deserialize.cpp
@@ -188,7 +188,7 @@ FUZZ_TARGET_DESERIALIZE(blockmerkleroot, {
     BlockMerkleRoot(block, &mutated);
 })
 FUZZ_TARGET_DESERIALIZE(addrman_deserialize, {
-    CAddrMan am;
+    CAddrMan am(/* deterministic */ false, /* consistency_check_ratio */ 0);
     DeserializeFromFuzzingInput(buffer, am);
 })
 FUZZ_TARGET_DESERIALIZE(blockheader_deserialize, {
diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
index acbbf357d2..1915f9c7d5 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -34,13 +34,9 @@ class CAddrManSerializationMock : public CAddrMan
 public:
     virtual void Serialize(CDataStream& s) const = 0;
 
-    //! Ensure that bucket placement is always the same for testing purposes.
-    void MakeDeterministic()
-    {
-        LOCK(cs);
-        nKey.SetNull();
-        insecure_rand = FastRandomContext(true);
-    }
+    CAddrManSerializationMock()
+        : CAddrMan(/* deterministic */ true, /* consistency_check_ratio */ 100)
+    {}
 };
 
 class CAddrManUncorrupted : public CAddrManSerializationMock
@@ -105,7 +101,6 @@ BOOST_AUTO_TEST_CASE(cnode_listen_port)
 BOOST_AUTO_TEST_CASE(caddrdb_read)
 {
     CAddrManUncorrupted addrmanUncorrupted;
-    addrmanUncorrupted.MakeDeterministic();
 
     CService addr1, addr2, addr3;
     BOOST_CHECK(Lookup("250.7.1.1", addr1, 8333, false));
@@ -124,7 +119,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read)
     // Test that the de-serialization does not throw an exception.
     CDataStream ssPeers1 = AddrmanToStream(addrmanUncorrupted);
     bool exceptionThrown = false;
-    CAddrMan addrman1;
+    CAddrMan addrman1(/* deterministic */ false, /* consistency_check_ratio */ 100);
 
     BOOST_CHECK(addrman1.size() == 0);
     try {
@@ -141,7 +136,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read)
     // Test that CAddrDB::Read creates an addrman with the correct number of addrs.
     CDataStream ssPeers2 = AddrmanToStream(addrmanUncorrupted);
 
-    CAddrMan addrman2;
+    CAddrMan addrman2(/* deterministic */ false, /* consistency_check_ratio */ 100);
     BOOST_CHECK(addrman2.size() == 0);
     BOOST_CHECK(CAddrDB::Read(addrman2, ssPeers2));
     BOOST_CHECK(addrman2.size() == 3);
@@ -151,12 +146,11 @@ BOOST_AUTO_TEST_CASE(caddrdb_read)
 BOOST_AUTO_TEST_CASE(caddrdb_read_corrupted)
 {
     CAddrManCorrupted addrmanCorrupted;
-    addrmanCorrupted.MakeDeterministic();
 
     // Test that the de-serialization of corrupted addrman throws an exception.
     CDataStream ssPeers1 = AddrmanToStream(addrmanCorrupted);
     bool exceptionThrown = false;
-    CAddrMan addrman1;
+    CAddrMan addrman1(/* deterministic */ false, /* consistency_check_ratio */ 100);
     BOOST_CHECK(addrman1.size() == 0);
     try {
         unsigned char pchMsgTmp[4];
@@ -172,7 +166,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read_corrupted)
     // Test that CAddrDB::Read leaves addrman in a clean state if de-serialization fails.
     CDataStream ssPeers2 = AddrmanToStream(addrmanCorrupted);
 
-    CAddrMan addrman2;
+    CAddrMan addrman2(/* deterministic */ false, /* consistency_check_ratio */ 100);
     BOOST_CHECK(addrman2.size() == 0);
     BOOST_CHECK(!CAddrDB::Read(addrman2, ssPeers2));
     BOOST_CHECK(addrman2.size() == 0);
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index ce81fc378f..c9bb863a7b 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -193,7 +193,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
         throw std::runtime_error(strprintf("ActivateBestChain failed. (%s)", state.ToString()));
     }
 
-    m_node.addrman = std::make_unique<CAddrMan>();
+    m_node.addrman = std::make_unique<CAddrMan>(/* deterministic */ false, /* consistency_check_ratio */ 0);
     m_node.banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME);
     m_node.connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman); // Deterministic randomness for tests.
     m_node.peerman = PeerManager::make(chainparams, *m_node.connman, *m_node.addrman,