From 802e6e128bba5ffa6d4ec53ff45acccb7cb28f21 Mon Sep 17 00:00:00 2001
From: stratospher <44024636+stratospher@users.noreply.github.com>
Date: Fri, 15 Dec 2023 13:41:30 +0530
Subject: [PATCH 1/7] [init] Add new command line arg for use only in
 functional tests

some of the existing command line args are to be only used in
functional tests. ex: addrmantest, fastprune etc.. make a separate
category -test=<option> for these so that code is cleaner and
user's debug-help output is straightforward.
---
 src/common/args.cpp | 11 +++++++++++
 src/common/args.h   |  5 +++++
 src/init.cpp        | 17 +++++++++++++++++
 3 files changed, 33 insertions(+)

diff --git a/src/common/args.cpp b/src/common/args.cpp
index a9108e59162f9..efd132f541aa0 100644
--- a/src/common/args.cpp
+++ b/src/common/args.cpp
@@ -682,6 +682,17 @@ std::string HelpMessageOpt(const std::string &option, const std::string &message
            std::string("\n\n");
 }
 
+const std::vector<std::string> TEST_OPTIONS_DOC{
+};
+
+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 6451b194d1cc2..78a61313b91df 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 39ee9fe60dc05..276908349aec8 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -611,6 +611,7 @@ void SetupServerArgs(ArgsManager& argsman)
     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);
@@ -1024,6 +1025,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{};

From be25ac3092b7755e26e1ec6c33a27cd0e3dd9eac Mon Sep 17 00:00:00 2001
From: stratospher <44024636+stratospher@users.noreply.github.com>
Date: Wed, 11 Oct 2023 10:50:31 +0530
Subject: [PATCH 2/7] [init] Remove -addrmantest command line arg

-addrmantest is only used in `p2p_node_network_limited.py` test to
test if the node self-advertises a hard-coded local address
(which wouldn't be advertised in the tests because it's unroutable
without the test-only code path) to check pruning-related services
are correct in that addr.

Remove -addrmantest because the self advertisement happens because
of hard coded test path logic, and expected services are nominal
due to how easily the test-only code could diverge from mainnet
logic. It's also being used only in 1 test.
---
 src/init.cpp                                |  1 -
 src/net.cpp                                 |  7 +------
 test/functional/p2p_node_network_limited.py | 13 +------------
 3 files changed, 2 insertions(+), 19 deletions(-)

diff --git a/src/init.cpp b/src/init.cpp
index 276908349aec8..488cb0fe9fe27 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -610,7 +610,6 @@ 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);
diff --git a/src/net.cpp b/src/net.cpp
index 41f5323d910eb..4e5ac387db6f6 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;
     }
diff --git a/test/functional/p2p_node_network_limited.py b/test/functional/p2p_node_network_limited.py
index 89c35e943b5d4..26d725aca665f 100755
--- a/test/functional/p2p_node_network_limited.py
+++ b/test/functional/p2p_node_network_limited.py
@@ -15,7 +15,6 @@
     NODE_P2P_V2,
     NODE_WITNESS,
     msg_getdata,
-    msg_verack,
 )
 from test_framework.p2p import P2PInterface
 from test_framework.test_framework import BitcoinTestFramework
@@ -43,7 +42,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)
@@ -78,16 +77,6 @@ def run_test(self):
         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

From 69e091f3e1f65b12cf1804e7d39651f55a01827a Mon Sep 17 00:00:00 2001
From: stratospher <44024636+stratospher@users.noreply.github.com>
Date: Wed, 11 Oct 2023 10:51:21 +0530
Subject: [PATCH 3/7] [init] Create deterministic addrman in tests using
 -test=addrman

Supposing there are 2 different 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
end up being the same for the 2 different 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.

Improve this by allowing deterministic addrman creation in the
functional tests. This creates an addrman with fixed `nKey` = 1 and
we can know the [bucket,position] collisions beforehand, safely add
more addresses in an addrman table and write more extensive tests.
---
 src/addrdb.cpp      | 8 +++++---
 src/common/args.cpp | 1 +
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/addrdb.cpp b/src/addrdb.cpp
index 8cf932bcb6396..6bd5a651326a3 100644
--- a/src/addrdb.cpp
+++ b/src/addrdb.cpp
@@ -185,7 +185,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"};
@@ -194,7 +196,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&) {
@@ -202,7 +204,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 efd132f541aa0..c90eb0c6856d8 100644
--- a/src/common/args.cpp
+++ b/src/common/args.cpp
@@ -683,6 +683,7 @@ std::string HelpMessageOpt(const std::string &option, const std::string &message
 }
 
 const std::vector<std::string> TEST_OPTIONS_DOC{
+    "addrman (use deterministic addrman)",
 };
 
 bool HasTestOption(const ArgsManager& args, const std::string& test_option)

From 7b868e6b678502e86571976d696c0e3cb72c0884 Mon Sep 17 00:00:00 2001
From: stratospher <44024636+stratospher@users.noreply.github.com>
Date: Wed, 6 Dec 2023 22:10:29 +0530
Subject: [PATCH 4/7] Revert "test: avoid non-determinism in asmap-addrman
 test"

This reverts commit 5825b34783545f9470d5ab94b87c918980715675.
The non-determinism is avoided by using a deterministic addrman.
---
 test/functional/feature_asmap.py | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/test/functional/feature_asmap.py b/test/functional/feature_asmap.py
index ae483fe4498d9..7e32f8d515b2d 100755
--- a/test/functional/feature_asmap.py
+++ b/test/functional/feature_asmap.py
@@ -42,8 +42,8 @@ def set_test_params(self):
         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."""
-        for addr, tried in [[0, True], [1, False]]:
+        """Add 2 tried addresses to the addrman, followed by 2 new addresses."""
+        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 @@ def test_asmap_interaction_with_addrman_containing_entries(self):
         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 @@ def test_empty_asmap(self):
     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)

From 71c19915c0c716d6f8a539dd92b8ad41e8c447ee Mon Sep 17 00:00:00 2001
From: stratospher <44024636+stratospher@users.noreply.github.com>
Date: Thu, 21 Dec 2023 10:56:00 +0530
Subject: [PATCH 5/7] [test] Use deterministic addrman in addpeeraddress test

this test inserts 1 address into the new table and 1 address into
the tried table so that no collisions can happen in either table
if a second address is added. this setup does not need to be
maintained anymore since we can use a deterministic addrman and
safely add many addresses in both tables without collisions. Remove
comment explaining why previous setup needed to be maintained.
---
 test/functional/rpc_net.py | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py
index e1820b0f5578f..a824228c3548b 100755
--- a/test/functional/rpc_net.py
+++ b/test/functional/rpc_net.py
@@ -299,16 +299,8 @@ def test_getnodeaddresses(self):
         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")

From a8978661093500df8d8dcf2a9c90837afacd0aab Mon Sep 17 00:00:00 2001
From: stratospher <44024636+stratospher@users.noreply.github.com>
Date: Wed, 20 Dec 2023 21:57:58 +0530
Subject: [PATCH 6/7] [test] Restart a node with empty addrman

Currently in tests where we are interested in contents of addrman,
addresses which were added to the node's addrman in previous tests
leak into the current test. example: addresses added in addpeeraddress
test leak into getaddrmaninfo and getrawaddrman tests.

It is cleaner to design the tests to be modular and without such
leaks so that we don't need to deal with context from previous tests
---
 test/functional/feature_addrman.py               |  7 +------
 test/functional/test_framework/test_framework.py | 10 ++++++++--
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/test/functional/feature_addrman.py b/test/functional/feature_addrman.py
index 9839993115aea..20adad1e52080 100755
--- a/test/functional/feature_addrman.py
+++ b/test/functional/feature_addrman.py
@@ -157,12 +157,7 @@ def run_test(self):
         )
 
         self.log.info("Check that missing addrman is recreated")
-        self.stop_node(0)
-        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)
+        self.restart_node(0, clear_addrman=True)
         assert_equal(self.nodes[0].getnodeaddresses(), [])
 
 
diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 0ee332b75bf5f..7f56842b04cc4 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -577,10 +577,16 @@ def stop_nodes(self, wait=0):
             # 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)
-        self.start_node(i, extra_args)
+        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):
         self.nodes[i].process.wait(timeout)

From 2cc8ca19f4185490f30a49516c890b2289fbab71 Mon Sep 17 00:00:00 2001
From: stratospher <44024636+stratospher@users.noreply.github.com>
Date: Fri, 1 Dec 2023 12:54:43 +0530
Subject: [PATCH 7/7] [test] Use deterministic addrman in addrman info tests

The nodes are restarted with an empty addrman and populated
with addresses from different networks using a helper function.
We can safely add multiple addresses to addrman tables without
worrying about unpredictable collisions since bucket:position
is fixed in a deterministic addrman.
---
 test/functional/rpc_net.py | 169 ++++++++++++++++++++++---------------
 1 file changed, 99 insertions(+), 70 deletions(-)

diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py
index a824228c3548b..228e2a3ff4a65 100755
--- a/test/functional/rpc_net.py
+++ b/test/functional/rpc_net.py
@@ -13,7 +13,6 @@
 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
@@ -376,25 +389,33 @@ def test_sendmsgtopeer(self):
 
     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)
+
+        expected_network_count = {
+            '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},
+        }
 
-        # current count of ipv4 addresses in addrman is {'new':1, 'tried':1}
-        self.log.info("Test that count of addresses in addrman match expected values")
+        self.log.debug("Test that count of addresses in addrman match expected values")
         res = node.getaddrmaninfo()
-        assert_equal(res["ipv4"]["new"], 1)
-        assert_equal(res["ipv4"]["tried"], 1)
-        assert_equal(res["ipv4"]["total"], 2)
-        assert_equal(res["all_networks"]["new"], 1)
-        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)
+        for network, count in expected_network_count.items():
+            assert_equal(res[network]['new'], count['new'])
+            assert_equal(res[network]['tried'], count['tried'])
+            assert_equal(res[network]['total'], count['total'])
 
     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.
@@ -416,88 +437,96 @@ def check_getrawaddrman_entries(expected):
             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": {
-                "bucket_count": ADDRMAN_NEW_BUCKET_COUNT,
-                "entries": [
+            "new": [
                     {
+                        "bucket_position": "82/8",
                         "address": "2.0.0.0",
                         "port": 8333,
                         "services": 9,
                         "network": "ipv4",
                         "source": "2.0.0.0",
                         "source_network": "ipv4",
+                    },
+                    {
+                        "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_count": ADDRMAN_TRIED_BUCKET_COUNT,
-                "entries": [
+            ],
+            "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,
+                        "services": 9,
+                        "network": "ipv6",
+                        "source": "1233:3432:2434:2343:3234:2345:6546:4534",
+                        "source_network": "ipv6",
+                    },
+                    {
+                        "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": 45324,
                     }
-                ]
-            }
+            ]
         }
 
-        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_network": "ipv6",
-            "port": -1,  # set once addpeeraddress is successful
-        })
-        expected["tried"]["entries"].append({
-            "address": "nrfj6inpyf73gpkyool35hcmne5zwfmse3jl3aw23vk7chdemalyaqad.onion",
-            "services": 9,
-            "network": "onion",
-            "source": "nrfj6inpyf73gpkyool35hcmne5zwfmse3jl3aw23vk7chdemalyaqad.onion",
-            "source_network": "onion",
-            "port": -1,  # set once addpeeraddress is successful
-        })
-
-        port = 0
-        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")
+        self.log.debug("Test that getrawaddrman contains information about newly added addresses in each addrman table")
         check_getrawaddrman_entries(expected)