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

2cc8ca19f4 [test] Use deterministic addrman in addrman info tests (stratospher)
a897866109 [test] Restart a node with empty addrman (stratospher)
71c19915c0 [test] Use deterministic addrman in addpeeraddress test (stratospher)
7b868e6b67 Revert "test: avoid non-determinism in asmap-addrman test" (stratospher)
69e091f3e1 [init] Create deterministic addrman in tests using -test=addrman (stratospher)
be25ac3092 [init] Remove -addrmantest command line arg (stratospher)
802e6e128b [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 2cc8ca19f4
  achow101:
    ACK 2cc8ca19f4
  0xB10C:
    ACK 2cc8ca19f4
  mzumsande:
    Code Review ACK 2cc8ca19f4

Tree-SHA512: 8acd9bdfe7de1eb44d22373bf13533d8ecf602df966fdd5b8b78afcd8cc35a286c95d2712f67a89473a0d68dded7d38f5599f6e4bf95a6589475444545bfb189

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) {

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

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
  *

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{};

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;
     }

test/functional/feature_addrman.py

@@ -156,12 +156,7 @@ class AddrmanTest(BitcoinTestFramework):
         )
 
         self.log.info("Check that missing addrman is recreated")
-        self.stop_node(0)
+        self.restart_node(0, clear_addrman=True)
-        os.remove(peers_dat)
-        with self.nodes[0].assert_debug_log([
-                f'Creating peers.dat because the file was not found ("{peers_dat}")',
-        ]):
-            self.start_node(0)
         assert_equal(self.nodes[0].getnodeaddresses(), [])
 
 

test/functional/feature_asmap.py

@@ -42,8 +42,8 @@ class AsmapTest(BitcoinTestFramework):
         self.extra_args = [["-checkaddrman=1"]]  # Do addrman checks on all operations.
 
     def fill_addrman(self, node_id):
-        """Add 1 tried address to the addrman, followed by 1 new address."""
+        """Add 2 tried addresses to the addrman, followed by 2 new addresses."""
-        for addr, tried in [[0, True], [1, False]]:
+        for addr, tried in [[0, True], [1, True], [2, False], [3, False]]:
             self.nodes[node_id].addpeeraddress(address=f"101.{addr}.0.0", tried=tried, port=8333)
 
     def test_without_asmap_arg(self):
@@ -84,12 +84,12 @@ class AsmapTest(BitcoinTestFramework):
         self.log.info("Test bitcoind -asmap restart with addrman containing new and tried entries")
         self.stop_node(0)
         shutil.copyfile(self.asmap_raw, self.default_asmap)
-        self.start_node(0, ["-asmap", "-checkaddrman=1"])
+        self.start_node(0, ["-asmap", "-checkaddrman=1", "-test=addrman"])
         self.fill_addrman(node_id=0)
-        self.restart_node(0, ["-asmap", "-checkaddrman=1"])
+        self.restart_node(0, ["-asmap", "-checkaddrman=1", "-test=addrman"])
         with self.node.assert_debug_log(
             expected_msgs=[
-                "CheckAddrman: new 1, tried 1, total 2 started",
+                "CheckAddrman: new 2, tried 2, total 4 started",
                 "CheckAddrman: completed",
             ]
         ):
@@ -114,7 +114,7 @@ class AsmapTest(BitcoinTestFramework):
     def test_asmap_health_check(self):
         self.log.info('Test bitcoind -asmap logs ASMap Health Check with basic stats')
         shutil.copyfile(self.asmap_raw, self.default_asmap)
-        msg = "ASMap Health Check: 2 clearnet peers are mapped to 1 ASNs with 0 peers being unmapped"
+        msg = "ASMap Health Check: 4 clearnet peers are mapped to 3 ASNs with 0 peers being unmapped"
         with self.node.assert_debug_log(expected_msgs=[msg]):
             self.start_node(0, extra_args=['-asmap'])
         os.remove(self.default_asmap)

test/functional/p2p_node_network_limited.py

@@ -15,7 +15,6 @@ from test_framework.messages import (
     NODE_P2P_V2,
     NODE_WITNESS,
     msg_getdata,
-    msg_verack,
 )
 from test_framework.p2p import P2PInterface
 from test_framework.test_framework import BitcoinTestFramework
@@ -47,7 +46,7 @@ class NodeNetworkLimitedTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 3
-        self.extra_args = [['-prune=550', '-addrmantest'], [], []]
+        self.extra_args = [['-prune=550'], [], []]
 
     def disconnect_all(self):
         self.disconnect_nodes(0, 1)
@@ -139,16 +138,6 @@ class NodeNetworkLimitedTest(BitcoinTestFramework):
         self.log.info("Requesting block at height 2 (tip-289) must fail (ignored).")
         node.send_getdata_for_block(blocks[0])  # first block outside of the 288+2 limit
         node.wait_for_disconnect(5)
-
-        self.log.info("Check local address relay, do a fresh connection.")
-        self.nodes[0].disconnect_p2ps()
-        node1 = self.nodes[0].add_p2p_connection(P2PIgnoreInv())
-        node1.send_message(msg_verack())
-
-        node1.wait_for_addr()
-        #must relay address with NODE_NETWORK_LIMITED
-        assert_equal(node1.firstAddrnServices, expected_services)
-
         self.nodes[0].disconnect_p2ps()
 
         # connect unsynced node 2 with pruned NODE_NETWORK_LIMITED peer

test/functional/rpc_net.py

@@ -13,7 +13,6 @@ import platform
 import time
 
 import test_framework.messages
-from test_framework.netutil import ADDRMAN_NEW_BUCKET_COUNT, ADDRMAN_TRIED_BUCKET_COUNT, ADDRMAN_BUCKET_SIZE
 from test_framework.p2p import (
     P2PInterface,
     P2P_SERVICES,
@@ -42,6 +41,20 @@ def assert_net_servicesnames(servicesflag, servicenames):
     assert servicesflag_generated == servicesflag
 
 
+def seed_addrman(node):
+    """ Populate the addrman with addresses from different networks.
+    Here 2 ipv4, 2 ipv6, 1 cjdns, 2 onion and 1 i2p addresses are added.
+    """
+    node.addpeeraddress(address="1.2.3.4", tried=True, port=8333)
+    node.addpeeraddress(address="2.0.0.0", port=8333)
+    node.addpeeraddress(address="1233:3432:2434:2343:3234:2345:6546:4534", tried=True, port=8333)
+    node.addpeeraddress(address="2803:0:1234:abcd::1", port=45324)
+    node.addpeeraddress(address="fc00:1:2:3:4:5:6:7", port=8333)
+    node.addpeeraddress(address="pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion", tried=True, port=8333)
+    node.addpeeraddress(address="nrfj6inpyf73gpkyool35hcmne5zwfmse3jl3aw23vk7chdemalyaqad.onion", port=45324, tried=True)
+    node.addpeeraddress(address="c4gfnttsuwqomiygupdqqqyy5y5emnk5c73hrfvatri67prd7vyq.b32.i2p", port=8333)
+
+
 class NetTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 2
@@ -305,16 +318,8 @@ class NetTest(BitcoinTestFramework):
         assert_raises_rpc_error(-8, "Network not recognized: Foo", self.nodes[0].getnodeaddresses, 1, "Foo")
 
     def test_addpeeraddress(self):
-        """RPC addpeeraddress sets the source address equal to the destination address.
-        If an address with the same /16 as an existing new entry is passed, it will be
-        placed in the same new bucket and have a 1/64 chance of the bucket positions
-        colliding (depending on the value of nKey in the addrman), in which case the
-        new address won't be added.  The probability of collision can be reduced to
-        1/2^16 = 1/65536 by using an address from a different /16.  We avoid this here
-        by first testing adding a tried table entry before testing adding a new table one.
-        """
         self.log.info("Test addpeeraddress")
-        self.restart_node(1, ["-checkaddrman=1"])
+        self.restart_node(1, ["-checkaddrman=1", "-test=addrman"])
         node = self.nodes[1]
 
         self.log.debug("Test that addpeerinfo is a hidden RPC")
@@ -390,25 +395,33 @@ class NetTest(BitcoinTestFramework):
 
     def test_getaddrmaninfo(self):
         self.log.info("Test getaddrmaninfo")
+        self.restart_node(1, extra_args=["-cjdnsreachable", "-test=addrman"], clear_addrman=True)
         node = self.nodes[1]
+        seed_addrman(node)
 
-        # current count of ipv4 addresses in addrman is {'new':1, 'tried':1}
+        expected_network_count = {
-        self.log.info("Test that count of addresses in addrman match expected values")
+            'all_networks': {'new': 4, 'tried': 4, 'total': 8},
+            'ipv4': {'new': 1, 'tried': 1, 'total': 2},
+            'ipv6': {'new': 1, 'tried': 1, 'total': 2},
+            'onion': {'new': 0, 'tried': 2, 'total': 2},
+            'i2p': {'new': 1, 'tried': 0, 'total': 1},
+            'cjdns': {'new': 1, 'tried': 0, 'total': 1},
+        }
+
+        self.log.debug("Test that count of addresses in addrman match expected values")
         res = node.getaddrmaninfo()
-        assert_equal(res["ipv4"]["new"], 1)
+        for network, count in expected_network_count.items():
-        assert_equal(res["ipv4"]["tried"], 1)
+            assert_equal(res[network]['new'], count['new'])
-        assert_equal(res["ipv4"]["total"], 2)
+            assert_equal(res[network]['tried'], count['tried'])
-        assert_equal(res["all_networks"]["new"], 1)
+            assert_equal(res[network]['total'], count['total'])
-        assert_equal(res["all_networks"]["tried"], 1)
-        assert_equal(res["all_networks"]["total"], 2)
-        for net in ["ipv6", "onion", "i2p", "cjdns"]:
-            assert_equal(res[net]["new"], 0)
-            assert_equal(res[net]["tried"], 0)
-            assert_equal(res[net]["total"], 0)
 
     def test_getrawaddrman(self):
         self.log.info("Test getrawaddrman")
+        self.restart_node(1, extra_args=["-cjdnsreachable", "-test=addrman"], clear_addrman=True)
         node = self.nodes[1]
+        self.addr_time = int(time.time())
+        node.setmocktime(self.addr_time)
+        seed_addrman(node)
 
         self.log.debug("Test that getrawaddrman is a hidden RPC")
         # It is hidden from general help, but its detailed help may be called directly.
@@ -430,88 +443,96 @@ class NetTest(BitcoinTestFramework):
             getrawaddrman = node.getrawaddrman()
             getaddrmaninfo = node.getaddrmaninfo()
             for (table_name, table_info) in expected.items():
-                assert_equal(len(getrawaddrman[table_name]), len(table_info["entries"]))
+                assert_equal(len(getrawaddrman[table_name]), len(table_info))
                 assert_equal(len(getrawaddrman[table_name]), getaddrmaninfo["all_networks"][table_name])
 
                 for bucket_position in getrawaddrman[table_name].keys():
-                    bucket = int(bucket_position.split("/")[0])
-                    position = int(bucket_position.split("/")[1])
-
-                    # bucket and position only be sanity checked here as the
-                    # test-addrman isn't deterministic
-                    assert 0 <= int(bucket) < table_info["bucket_count"]
-                    assert 0 <= int(position) < ADDRMAN_BUCKET_SIZE
-
                     entry = getrawaddrman[table_name][bucket_position]
-                    expected_entry = list(filter(lambda e: e["address"] == entry["address"], table_info["entries"]))[0]
+                    expected_entry = list(filter(lambda e: e["address"] == entry["address"], table_info))[0]
+                    assert bucket_position == expected_entry["bucket_position"]
                     check_addr_information(entry, expected_entry)
 
-        # we expect one addrman new and tried table entry, which were added in a previous test
+        # we expect 4 new and 4 tried table entries in the addrman which were added using seed_addrman()
         expected = {
-            "new": {
+            "new": [
-                "bucket_count": ADDRMAN_NEW_BUCKET_COUNT,
-                "entries": [
                     {
+                        "bucket_position": "82/8",
                         "address": "2.0.0.0",
                         "port": 8333,
                         "services": 9,
                         "network": "ipv4",
                         "source": "2.0.0.0",
                         "source_network": "ipv4",
-                    }
-                ]
                     },
-            "tried": {
-                "bucket_count": ADDRMAN_TRIED_BUCKET_COUNT,
-                "entries": [
                     {
+                        "bucket_position": "336/24",
+                        "address": "fc00:1:2:3:4:5:6:7",
+                        "port": 8333,
+                        "services": 9,
+                        "network": "cjdns",
+                        "source": "fc00:1:2:3:4:5:6:7",
+                        "source_network": "cjdns",
+                    },
+                    {
+                        "bucket_position": "963/46",
+                        "address": "c4gfnttsuwqomiygupdqqqyy5y5emnk5c73hrfvatri67prd7vyq.b32.i2p",
+                        "port": 8333,
+                        "services": 9,
+                        "network": "i2p",
+                        "source": "c4gfnttsuwqomiygupdqqqyy5y5emnk5c73hrfvatri67prd7vyq.b32.i2p",
+                        "source_network": "i2p",
+                    },
+                    {
+                        "bucket_position": "613/6",
+                        "address": "2803:0:1234:abcd::1",
+                        "services": 9,
+                        "network": "ipv6",
+                        "source": "2803:0:1234:abcd::1",
+                        "source_network": "ipv6",
+                        "port": 45324,
+                    }
+            ],
+            "tried": [
+                    {
+                        "bucket_position": "6/33",
                         "address": "1.2.3.4",
                         "port": 8333,
                         "services": 9,
                         "network": "ipv4",
                         "source": "1.2.3.4",
                         "source_network": "ipv4",
-                    }
+                    },
-                ]
+                    {
-            }
+                        "bucket_position": "197/34",
-        }
+                        "address": "1233:3432:2434:2343:3234:2345:6546:4534",
-
+                        "port": 8333,
-        self.log.debug("Test that the getrawaddrman contains information about the addresses added in a previous test")
-        check_getrawaddrman_entries(expected)
-
-        self.log.debug("Add one new address to each addrman table")
-        expected["new"]["entries"].append({
-            "address": "2803:0:1234:abcd::1",
                         "services": 9,
                         "network": "ipv6",
-            "source": "2803:0:1234:abcd::1",
+                        "source": "1233:3432:2434:2343:3234:2345:6546:4534",
                         "source_network": "ipv6",
-            "port": -1,  # set once addpeeraddress is successful
+                    },
-        })
+                    {
-        expected["tried"]["entries"].append({
+                        "bucket_position": "72/61",
+                        "address": "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion",
+                        "port": 8333,
+                        "services": 9,
+                        "network": "onion",
+                        "source": "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion",
+                        "source_network": "onion"
+                    },
+                    {
+                        "bucket_position": "139/46",
                         "address": "nrfj6inpyf73gpkyool35hcmne5zwfmse3jl3aw23vk7chdemalyaqad.onion",
                         "services": 9,
                         "network": "onion",
                         "source": "nrfj6inpyf73gpkyool35hcmne5zwfmse3jl3aw23vk7chdemalyaqad.onion",
                         "source_network": "onion",
-            "port": -1,  # set once addpeeraddress is successful
+                        "port": 45324,
-        })
+                    }
+            ]
+        }
 
-        port = 0
+        self.log.debug("Test that getrawaddrman contains information about newly added addresses in each addrman table")
-        for (table_name, table_info) in expected.items():
-            # There's a slight chance that the to-be-added address collides with an already
-            # present table entry. To avoid this, we increment the port until an address has been
-            # added. Incrementing the port changes the position in the new table bucket (bucket
-            # stays the same) and changes both the bucket and the position in the tried table.
-            while True:
-                if node.addpeeraddress(address=table_info["entries"][1]["address"], port=port, tried=table_name == "tried")["success"]:
-                    table_info["entries"][1]["port"] = port
-                    self.log.debug(f"Added {table_info['entries'][1]['address']} to {table_name} table")
-                    break
-                else:
-                    port += 1
-
-        self.log.debug("Test that the newly added addresses appear in getrawaddrman")
         check_getrawaddrman_entries(expected)
 
 

test/functional/test_framework/test_framework.py

@@ -581,9 +581,15 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
             # Wait for nodes to stop
             node.wait_until_stopped()
 
-    def restart_node(self, i, extra_args=None):
+    def restart_node(self, i, extra_args=None, clear_addrman=False):
         """Stop and start a test node"""
         self.stop_node(i)
+        if clear_addrman:
+            peers_dat = self.nodes[i].chain_path / "peers.dat"
+            os.remove(peers_dat)
+            with self.nodes[i].assert_debug_log(expected_msgs=[f'Creating peers.dat because the file was not found ("{peers_dat}")']):
+                self.start_node(i, extra_args)
+        else:
             self.start_node(i, extra_args)
 
     def wait_for_node_exit(self, i, timeout):