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

a4d7854 [addrman] Make addrman consistency checks a runtime option (John Newbery)
10aac24 [tests] Make deterministic addrman use nKey = 1 (John Newbery)
fa9710f [addrman] Add deterministic argument to CAddrMan ctor (John Newbery)
ee458d8 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 a4d7854 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 a4d7854
  theStack:
    Code-review ACK a4d7854

Tree-SHA512: eaee003f7a99154822c5b5efbc62008d32c1efbecc6fec6e183427f6b2ae5d30b3be7924e3a7271b1a1de91517f5bd2a70011d45358c3105c6a0702f12b70f7c

Author: fanquake

src/addrman.cpp

@@ -431,11 +431,14 @@ CAddrInfo CAddrMan::Select_(bool newOnly) const
     }
 }
 
-#ifdef DEBUG_ADDRMAN
-int CAddrMan::Check_()
+int CAddrMan::Check_() const
 {
     AssertLockHeld(cs);
 
+    // Run consistency checks 1 in m_consistency_check_ratio times if enabled
+    if (m_consistency_check_ratio == 0) return 0;
+    if (insecure_rand.randrange(m_consistency_check_ratio) >= 1) return 0;
+
     std::unordered_set<int> setTried;
     std::unordered_map<int, int> mapNew;
 
@@ -458,8 +461,10 @@ int CAddrMan::Check_()
                 return -4;
             mapNew[n] = info.nRefCount;
         }
-        if (mapAddr[info] != n)
+        const auto it{mapAddr.find(info)};
+        if (it == mapAddr.end() || it->second != n) {
             return -5;
+        }
         if (info.nRandomPos < 0 || (size_t)info.nRandomPos >= vRandom.size() || vRandom[info.nRandomPos] != n)
             return -14;
         if (info.nLastTry < 0)
@@ -478,10 +483,13 @@ int CAddrMan::Check_()
             if (vvTried[n][i] != -1) {
                 if (!setTried.count(vvTried[n][i]))
                     return -11;
-                if (mapInfo[vvTried[n][i]].GetTriedBucket(nKey, m_asmap) != n)
+                const auto it{mapInfo.find(vvTried[n][i])};
+                if (it == mapInfo.end() || it->second.GetTriedBucket(nKey, m_asmap) != n) {
                     return -17;
-                if (mapInfo[vvTried[n][i]].GetBucketPosition(nKey, false, n) != i)
+                }
+                if (it->second.GetBucketPosition(nKey, false, n) != i) {
                     return -18;
+                }
                 setTried.erase(vvTried[n][i]);
             }
         }
@@ -492,8 +500,10 @@ int CAddrMan::Check_()
             if (vvNew[n][i] != -1) {
                 if (!mapNew.count(vvNew[n][i]))
                     return -12;
-                if (mapInfo[vvNew[n][i]].GetBucketPosition(nKey, true, n) != i)
+                const auto it{mapInfo.find(vvNew[n][i])};
+                if (it == mapInfo.end() || it->second.GetBucketPosition(nKey, true, n) != i) {
                     return -19;
+                }
                 if (--mapNew[vvNew[n][i]] == 0)
                     mapNew.erase(vvNew[n][i]);
             }
@@ -509,7 +519,6 @@ int CAddrMan::Check_()
 
     return 0;
 }
-#endif
 
 void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size_t max_pct, std::optional<Network> network) const
 {

src/addrman.h

@@ -26,6 +26,9 @@
 #include <unordered_map>
 #include <vector>
 
+/** Default for -checkaddrman */
+static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{0};
+
 /**
  * Extended statistics about a CAddress
  */
@@ -124,8 +127,8 @@ class CAddrInfo : public CAddress
  *        attempt was unsuccessful.
  *    * Bucket selection is based on cryptographic hashing, using a randomly-generated 256-bit key, which should not
  *      be observable by adversaries.
- *    * Several indexes are kept for high performance. Defining DEBUG_ADDRMAN will introduce frequent (and expensive)
- *      consistency checks for the entire data structure.
+ *    * Several indexes are kept for high performance. Setting m_consistency_check_ratio with the -checkaddrman
+ *      configuration option will introduce (expensive) consistency checks for the entire data structure.
  */
 
 //! total number of buckets for tried addresses
@@ -493,9 +496,12 @@ class CAddrMan
         mapAddr.clear();
     }
 
-    CAddrMan()
+    explicit CAddrMan(bool deterministic, int32_t consistency_check_ratio)
+        : insecure_rand{deterministic},
+          m_consistency_check_ratio{consistency_check_ratio}
     {
         Clear();
+        if (deterministic) nKey = uint256{1};
     }
 
     ~CAddrMan()
@@ -637,13 +643,13 @@ class CAddrMan
     //! secret key to randomize bucket select with
     uint256 nKey;
 
-    //! Source of random numbers for randomization in inner loops
-    mutable FastRandomContext insecure_rand GUARDED_BY(cs);
-
     //! A mutex to protect the inner data structures.
     mutable Mutex cs;
 
 private:
+    //! Source of random numbers for randomization in inner loops
+    mutable FastRandomContext insecure_rand GUARDED_BY(cs);
+
     //! Serialization versions.
     enum Format : uint8_t {
         V0_HISTORICAL = 0,    //!< historic format, before commit e6b343d88
@@ -698,6 +704,9 @@ class CAddrMan
     //! Holds addrs inserted into tried table that collide with existing entries. Test-before-evict discipline used to resolve these collisions.
     std::set<int> m_tried_collisions;
 
+    /** Perform consistency checks every m_consistency_check_ratio operations (if non-zero). */
+    const int32_t m_consistency_check_ratio;
+
     //! Find an entry.
     CAddrInfo* Find(const CNetAddr& addr, int *pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
@@ -735,22 +744,19 @@ class CAddrMan
     CAddrInfo SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Consistency check
-    void Check() const
-        EXCLUSIVE_LOCKS_REQUIRED(cs)
+    void Check() const EXCLUSIVE_LOCKS_REQUIRED(cs)
     {
-#ifdef DEBUG_ADDRMAN
         AssertLockHeld(cs);
+
         const int err = Check_();
         if (err) {
             LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err);
+            assert(false);
         }
-#endif
     }
 
-#ifdef DEBUG_ADDRMAN
     //! Perform consistency check. Returns an error code or zero.
     int Check_() const EXCLUSIVE_LOCKS_REQUIRED(cs);
-#endif
 
     /**
      * Return all or many randomly selected addresses, optionally by network.

src/bench/addrman.cpp

@@ -72,7 +72,7 @@ static void AddrManAdd(benchmark::Bench& bench)
 {
     CreateAddresses();
 
-    CAddrMan addrman;
+    CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0);
 
     bench.run([&] {
         AddAddressesToAddrMan(addrman);
@@ -82,7 +82,7 @@ static void AddrManAdd(benchmark::Bench& bench)
 
 static void AddrManSelect(benchmark::Bench& bench)
 {
-    CAddrMan addrman;
+    CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0);
 
     FillAddrMan(addrman);
 
@@ -94,7 +94,7 @@ static void AddrManSelect(benchmark::Bench& bench)
 
 static void AddrManGetAddr(benchmark::Bench& bench)
 {
-    CAddrMan addrman;
+    CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0);
 
     FillAddrMan(addrman);
 
@@ -112,10 +112,12 @@ static void AddrManGood(benchmark::Bench& bench)
      * we want to do the same amount of work in every loop iteration. */
 
     bench.epochs(5).epochIterations(1);
+    const size_t addrman_count{bench.epochs() * bench.epochIterations()};
 
-    std::vector<CAddrMan> addrmans(bench.epochs() * bench.epochIterations());
-    for (auto& addrman : addrmans) {
-        FillAddrMan(addrman);
+    std::vector<std::unique_ptr<CAddrMan>> addrmans(addrman_count);
+    for (size_t i{0}; i < addrman_count; ++i) {
+        addrmans[i] = std::make_unique<CAddrMan>(/* deterministic */ false, /* consistency_check_ratio */ 0);
+        FillAddrMan(*addrmans[i]);
     }
 
     auto markSomeAsGood = [](CAddrMan& addrman) {
@@ -130,7 +132,7 @@ static void AddrManGood(benchmark::Bench& bench)
 
     uint64_t i = 0;
     bench.run([&] {
-        markSomeAsGood(addrmans.at(i));
+        markSomeAsGood(*addrmans.at(i));
         ++i;
     });
 }

src/init.cpp

@@ -501,7 +501,8 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-checkblocks=<n>", strprintf("How many blocks to check at startup (default: %u, 0 = all)", DEFAULT_CHECKBLOCKS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-checklevel=<n>", strprintf("How thorough the block verification of -checkblocks is: %s (0-4, default: %u)", Join(CHECKLEVEL_DOC, ", "), DEFAULT_CHECKLEVEL), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-checkblockindex", strprintf("Do a consistency check for the block tree, chainstate, and other validation data structures occasionally. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
-    argsman.AddArg("-checkmempool=<n>", strprintf("Run checks every <n> transactions (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
+    argsman.AddArg("-checkaddrman=<n>", strprintf("Run addrman consistency checks every <n> operations. Use 0 to disable. (default: %u)", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
+    argsman.AddArg("-checkmempool=<n>", strprintf("Run mempool consistency checks every <n> transactions. Use 0 to disable. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-checkpoints", strprintf("Enable rejection of any forks from the known historical chain until block %s (default: %u)", defaultChainParams->Checkpoints().GetHeight(), DEFAULT_CHECKPOINTS_ENABLED), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-deprecatedrpc=<method>", "Allows deprecated RPC method(s) to be used", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-stopafterblockimport", strprintf("Stop running after importing blocks from disk (default: %u)", DEFAULT_STOPAFTERBLOCKIMPORT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
@@ -1164,7 +1165,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     const bool ignores_incoming_txs{args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)};
 
     assert(!node.addrman);
-    node.addrman = std::make_unique<CAddrMan>();
+    auto check_addrman = std::clamp<int32_t>(args.GetArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000);
+    node.addrman = std::make_unique<CAddrMan>(/* deterministic */ false, /* consistency_check_ratio */ check_addrman);
     assert(!node.banman);
     node.banman = std::make_unique<BanMan>(gArgs.GetDataDirNet() / "banlist", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
     assert(!node.connman);