Merge bitcoin/bitcoin#29007: test: create deterministic addrman in the functional tests

2cc8ca19f4185490f30a49516c890b2289fbab71 [test] Use deterministic addrman in addrman info tests (stratospher)
a8978661093500df8d8dcf2a9c90837afacd0aab [test] Restart a node with empty addrman (stratospher)
71c19915c0c716d6f8a539dd92b8ad41e8c447ee [test] Use deterministic addrman in addpeeraddress test (stratospher)
7b868e6b678502e86571976d696c0e3cb72c0884 Revert "test: avoid non-determinism in asmap-addrman test" (stratospher)
69e091f3e1f65b12cf1804e7d39651f55a01827a [init] Create deterministic addrman in tests using -test=addrman (stratospher)
be25ac3092b7755e26e1ec6c33a27cd0e3dd9eac [init] Remove -addrmantest command line arg (stratospher)
802e6e128bba5ffa6d4ec53ff45acccb7cb28f21 [init] Add new command line arg for use only in functional tests (stratospher)

Pull request description:

  An address is placed in a `[bucket,position]` in the addrman table (new table or tried table) using the `addpeeraddress` RPC. This `[bucket,position]` is calculated using `nKey`(and other metrics) for the addrman which is chosen randomly during every run.

  Supposing there are 2 addresses to be placed in an addrman table. During every test run, a different `[bucket,position]` would be calculated for each address.These calculated `[bucket,position]` could even be the same for the 2 addresses in some test runs and result in collisions in the addrman. We wouldn't be able to predict when the collisions are going to happen because we can't predict the `nKey` value which is chosen at random. This can cause flaky tests.

  Because of these non deterministic collisions, we are limited in what we can do to test addrman functionality. Currently in our tests don't add a second address to prevent these collisions from happening - we only keep 1 address in the new table and 1 address in the tried table. See https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1091145647, https://github.com/bitcoin/bitcoin/pull/23084, [#22831(comment)](https://github.com/bitcoin/bitcoin/pull/22831/files#r708302639).

  This PR lets us create a deterministic addrman with fixed `nKey` so that we can know the `[bucket,position]` collisions beforehand, safely add more addresses in an addrman table and write more extensive tests.

ACKs for top commit:
  amitiuttarwar:
    ACK 2cc8ca19f4185490f30a49516c890b2289fbab71
  achow101:
    ACK 2cc8ca19f4185490f30a49516c890b2289fbab71
  0xB10C:
    ACK 2cc8ca19f4185490f30a49516c890b2289fbab71
  mzumsande:
    Code Review ACK 2cc8ca19f4185490f30a49516c890b2289fbab71

Tree-SHA512: 8acd9bdfe7de1eb44d22373bf13533d8ecf602df966fdd5b8b78afcd8cc35a286c95d2712f67a89473a0d68dded7d38f5599f6e4bf95a6589475444545bfb189

5 files changed, 40 insertions, 10 deletions

diff --git a/src/addrdb.cpp b/src/addrdb.cpp
index fd2a363b8a..3f5916560e 100644
--- a/src/addrdb.cpp
+++ b/src/addrdb.cpp
@@ -189,7 +189,9 @@ void ReadFromStream(AddrMan& addr, DataStream& ssPeers)
 util::Result<std::unique_ptr<AddrMan>> LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args)
 {
     auto check_addrman = std::clamp<int32_t>(args.GetIntArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000);
-    auto addrman{std::make_unique<AddrMan>(netgroupman, /*deterministic=*/false, /*consistency_check_ratio=*/check_addrman)};
+    bool deterministic = HasTestOption(args, "addrman"); // use a deterministic addrman only for tests
+
+    auto addrman{std::make_unique<AddrMan>(netgroupman, /*deterministic=*/deterministic, /*consistency_check_ratio=*/check_addrman)};
 
     const auto start{SteadyClock::now()};
     const auto path_addr{args.GetDataDirNet() / "peers.dat"};
@@ -198,7 +200,7 @@ util::Result<std::unique_ptr<AddrMan>> LoadAddrman(const NetGroupManager& netgro
         LogPrintf("Loaded %i addresses from peers.dat  %dms\n", addrman->Size(), Ticks<std::chrono::milliseconds>(SteadyClock::now() - start));
     } catch (const DbNotFoundError&) {
         // Addrman can be in an inconsistent state after failure, reset it
-        addrman = std::make_unique<AddrMan>(netgroupman, /*deterministic=*/false, /*consistency_check_ratio=*/check_addrman);
+        addrman = std::make_unique<AddrMan>(netgroupman, /*deterministic=*/deterministic, /*consistency_check_ratio=*/check_addrman);
         LogPrintf("Creating peers.dat because the file was not found (%s)\n", fs::quoted(fs::PathToString(path_addr)));
         DumpPeerAddresses(args, *addrman);
     } catch (const InvalidAddrManVersionError&) {
@@ -206,7 +208,7 @@ util::Result<std::unique_ptr<AddrMan>> LoadAddrman(const NetGroupManager& netgro
             return util::Error{strprintf(_("Failed to rename invalid peers.dat file. Please move or delete it and try again."))};
         }
         // Addrman can be in an inconsistent state after failure, reset it
-        addrman = std::make_unique<AddrMan>(netgroupman, /*deterministic=*/false, /*consistency_check_ratio=*/check_addrman);
+        addrman = std::make_unique<AddrMan>(netgroupman, /*deterministic=*/deterministic, /*consistency_check_ratio=*/check_addrman);
         LogPrintf("Creating new peers.dat because the file version was not compatible (%s). Original backed up to peers.dat.bak\n", fs::quoted(fs::PathToString(path_addr)));
         DumpPeerAddresses(args, *addrman);
     } catch (const std::exception& e) {
diff --git a/src/common/args.cpp b/src/common/args.cpp
index a9108e5916..c90eb0c685 100644
--- a/src/common/args.cpp
+++ b/src/common/args.cpp
@@ -682,6 +682,18 @@ std::string HelpMessageOpt(const std::string &option, const std::string &message
            std::string("\n\n");
 }
 
+const std::vector<std::string> TEST_OPTIONS_DOC{
+    "addrman (use deterministic addrman)",
+};
+
+bool HasTestOption(const ArgsManager& args, const std::string& test_option)
+{
+    const auto options = args.GetArgs("-test");
+    return std::any_of(options.begin(), options.end(), [test_option](const auto& option) {
+        return option == test_option;
+    });
+}
+
 fs::path GetDefaultDataDir()
 {
     // Windows: C:\Users\Username\AppData\Roaming\Bitcoin
diff --git a/src/common/args.h b/src/common/args.h
index 6451b194d1..78a61313b9 100644
--- a/src/common/args.h
+++ b/src/common/args.h
@@ -447,6 +447,11 @@ bool HelpRequested(const ArgsManager& args);
 /** Add help options to the args manager */
 void SetupHelpOptions(ArgsManager& args);
 
+extern const std::vector<std::string> TEST_OPTIONS_DOC;
+
+/** Checks if a particular test option is present in -test command-line arg options */
+bool HasTestOption(const ArgsManager& args, const std::string& test_option);
+
 /**
  * Format a string to be used as group of options in help messages
  *
diff --git a/src/init.cpp b/src/init.cpp
index 9ea7b881cb..b9a0bb732a 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -614,7 +614,7 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-limitancestorsize=<n>", strprintf("Do not accept transactions whose size with all in-mempool ancestors exceeds <n> kilobytes (default: %u)", DEFAULT_ANCESTOR_SIZE_LIMIT_KVB), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-limitdescendantcount=<n>", strprintf("Do not accept transactions if any ancestor would have <n> or more in-mempool descendants (default: %u)", DEFAULT_DESCENDANT_LIMIT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-limitdescendantsize=<n>", strprintf("Do not accept transactions if any ancestor would have more than <n> kilobytes of in-mempool descendants (default: %u).", DEFAULT_DESCENDANT_SIZE_LIMIT_KVB), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
-    argsman.AddArg("-addrmantest", "Allows to test address relay on localhost", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
+    argsman.AddArg("-test=<option>", "Pass a test-only option. Options include : " + Join(TEST_OPTIONS_DOC, ", ") + ".", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-capturemessages", "Capture all P2P messages to disk", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-mocktime=<n>", "Replace actual time with " + UNIX_EPOCH_TIME + " (default: 0)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-maxsigcachesize=<n>", strprintf("Limit sum of signature cache and script execution cache sizes to <n> MiB (default: %u)", DEFAULT_MAX_SIG_CACHE_BYTES >> 20), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
@@ -1028,6 +1028,22 @@ bool AppInitParameterInteraction(const ArgsManager& args)
     if (args.GetBoolArg("-peerbloomfilters", DEFAULT_PEERBLOOMFILTERS))
         nLocalServices = ServiceFlags(nLocalServices | NODE_BLOOM);
 
+    if (args.IsArgSet("-test")) {
+        if (chainparams.GetChainType() != ChainType::REGTEST) {
+            return InitError(Untranslated("-test=<option> should only be used in functional tests"));
+        }
+        const std::vector<std::string> options = args.GetArgs("-test");
+        for (const std::string& option : options) {
+            auto it = std::find_if(TEST_OPTIONS_DOC.begin(), TEST_OPTIONS_DOC.end(), [&option](const std::string& doc_option) {
+                size_t pos = doc_option.find(" (");
+                return (pos != std::string::npos) && (doc_option.substr(0, pos) == option);
+            });
+            if (it == TEST_OPTIONS_DOC.end()) {
+                InitWarning(strprintf(_("Unrecognised option \"%s\" provided in -test=<option>."), option));
+            }
+        }
+    }
+
     // Also report errors from parsing before daemonization
     {
         kernel::Notifications notifications{};
diff --git a/src/net.cpp b/src/net.cpp
index 7c82f01d75..2fbfd75261 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -238,10 +238,6 @@ static int GetnScore(const CService& addr)
 std::optional<CService> GetLocalAddrForPeer(CNode& node)
 {
     CService addrLocal{GetLocalAddress(node)};
-    if (gArgs.GetBoolArg("-addrmantest", false)) {
-        // use IPv4 loopback during addrmantest
-        addrLocal = CService(LookupNumeric("127.0.0.1", GetListenPort()));
-    }
     // If discovery is enabled, sometimes give our peer the address it
     // tells us that it sees us as in case it has a better idea of our
     // address than we do.
@@ -261,8 +257,7 @@ std::optional<CService> GetLocalAddrForPeer(CNode& node)
             addrLocal.SetIP(node.GetAddrLocal());
         }
     }
-    if (addrLocal.IsRoutable() || gArgs.GetBoolArg("-addrmantest", false))
-    {
+    if (addrLocal.IsRoutable()) {
         LogPrint(BCLog::NET, "Advertising address %s to peer=%d\n", addrLocal.ToStringAddrPort(), node.GetId());
         return addrLocal;
     }