test: create deterministic addrman in the functional tests

stratospher commented at 2:55 am on December 6, 2023: contributor

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 #26988 (review), #23084, #22831(comment).

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.

DrahtBot commented at 2:55 am on December 6, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	0xB10C, amitiuttarwar, mzumsande, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28998 (rpc: addpeeraddress tried return error on failure by 0xB10C)
#28802 (ArgsManager: support subcommand-specific options by ajtowns)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Tests on Dec 6, 2023

in src/init.cpp:582 in 964e1a8fae outdated

578@@ -579,7 +579,7 @@ void SetupServerArgs(ArgsManager& argsman)
579     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);
580     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);
581     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);
582-    argsman.AddArg("-addrmantest", "Allows to test address relay on localhost", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
583+    argsman.AddArg("-addrtest=<option>", strprintf("Test general address related functionalities. Only used in tests. When -addrtest=relay is specified, test address relay on localhost. When -addrtest=addrman is specified, use a deterministic addrman. (default: %s)", "relay"), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);

maflcko commented at 10:41 am on December 6, 2023:

I wonder if this can be generalized to pass test-only options (along with a helper HasTestOption, similar to IsDeprecatedRPCEnabled?

The user-facing debug-help output of this seems the wrong place to document test-only options, and the source code seems sufficient to be self-documenting.

0    argsman.AddArg("-test=<option>", "Pass a test-only option", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);

maflcko commented at 10:41 am on December 6, 2023:

In any case, (default: %s)", "relay" is wrong, no?

stratospher commented at 4:39 am on December 7, 2023:

I wonder if this can be generalized to pass test-only options (along with a helper HasTestOption, similar to IsDeprecatedRPCEnabled?

what would a function like HasTestOption do though? we could introduce it here to check if we’re using a known test option(“relay”, “addrman”). even if we use unknown test options, we don’t need to throw an error right.

In any case, (default: %s)", “relay” is wrong, no?

done.

maflcko commented at 7:49 am on December 7, 2023:

what would a function like HasTestOption do though? we could introduce it here to check if we’re using a known test option(“relay”, “addrman”). even if we use unknown test options, we don’t need to throw an error right.

It would just be a one-line function to hold the code you already wrote in two places:

0return any_of(args.GetArgs("test"), test_option);

mzumsande commented at 9:37 pm on December 14, 2023:

Are you suggesting to move other existing test-only options (-acceptstalefeeestimates or -fastprune come to mind) under this command?

maflcko commented at 10:00 pm on December 14, 2023:

Yes. (Not here, obviously, but in a follow-up possibly)

stratospher commented at 6:34 am on December 16, 2023:

It would just be a one-line function to hold the code you already wrote in two places:

oh got it now.

I wonder if this can be generalized to pass test-only options (along with a helper HasTestOption, similar to IsDeprecatedRPCEnabled?

good idea - liked how we can make sure users don’t accidentally use these test-only options in their config file. done.

in src/addrdb.cpp:189 in 964e1a8fae outdated

184@@ -185,7 +185,13 @@ void ReadFromStream(AddrMan& addr, DataStream& ssPeers)
185 util::Result<std::unique_ptr<AddrMan>> LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args)
186 {
187     auto check_addrman = std::clamp<int32_t>(args.GetIntArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000);
188-    auto addrman{std::make_unique<AddrMan>(netgroupman, /*deterministic=*/false, /*consistency_check_ratio=*/check_addrman)};
189+
190+    const auto options = gArgs.GetArgs("-addrtest");

maflcko commented at 10:42 am on December 6, 2023:

0    const auto options = args.GetArgs("-addrtest");

stratospher commented at 4:39 am on December 7, 2023:

done.

in test/functional/rpc_net.py:307 in 2474dd1cb0 outdated

304@@ -305,7 +305,7 @@ def test_addpeeraddress(self):
305         by first testing adding a tried table entry before testing adding a new table one.
306         """

0xB10C commented at 11:54 am on December 6, 2023:

You might also want to edit/update/remove(?) this comment. I think mentioning that we avoid collisions by adding known-working addresses to the deterministic addrman would be good.

stratospher commented at 4:39 am on December 7, 2023:

done.

amitiuttarwar commented at 1:17 am on December 19, 2023:

I think it makes sense to update the comment further. here’s why-

on current master, this test sets up addrman by putting 1 addr into tried, followed by 1 addr into new. the comment explains to future devs why this setup must be maintained.

this patch doesn’t update the setup in the addpeeraddress test. the comment on master is outdated because we no longer need to preserve the careful ordering, but I find the currently proposed comment misleading since the deterministic addrman isn’t yet used for collision prevention in this subtest at all

I think it makes sense to just delete this comment. I’m leaving suggestions for improving the structure between subtests in the overall review comment

stratospher commented at 7:08 am on December 21, 2023:

true, i’ve removed the comment in this commit.

in test/functional/feature_asmap.py:46 in 964e1a8fae outdated

41@@ -42,8 +42,8 @@ def set_test_params(self):
42         self.extra_args = [["-checkaddrman=1"]]  # Do addrman checks on all operations.
43 
44     def fill_addrman(self, node_id):
45-        """Add 1 tried address to the addrman, followed by 1 new address."""
46-        for addr, tried in [[0, True], [1, False]]:
47+        """Add 2 tried address to the addrman, followed by 2 new address."""
48+        for addr, tried in [[0, True], [1, True], [2, False], [3, False]]:

brunoerg commented at 2:20 pm on December 6, 2023:

In 964e1a8faec984df2799171bc2a9268a3765fbca: Any specific reason to increase the number of added addresses here?

0xB10C commented at 2:34 pm on December 6, 2023:

This was lowered to 1 tried and 1 new in #23084 as more than one address meant the test could sporadically fail if there’s a collision. @stratospher, I think, if you want to make this clearer, you could have this commit be a revert of https://github.com/bitcoin/bitcoin/commit/5825b34783545f9470d5ab94b87c918980715675 or note it in the commit message.

brunoerg commented at 2:37 pm on December 6, 2023:

Thanks, @0xB10C. An explanation in the commit message would be valuable.

stratospher commented at 4:39 am on December 7, 2023:

done.

stratospher force-pushed on Dec 7, 2023

stratospher commented at 4:43 am on December 7, 2023: contributor

thanks @maflcko, @0xb10c, @brunoerg. I’ve updated the PR to address your comments. Open to more thoughts/suggestions in #29007 (review).

DrahtBot added the label CI failed on Dec 7, 2023

in test/functional/rpc_net.py:477 in 054598132b outdated

467+                        "address": "c4gfnttsuwqomiygupdqqqyy5y5emnk5c73hrfvatri67prd7vyq.b32.i2p",
468+                        "port": 8333,
469+                        "services": 9,
470+                        "network": "i2p",
471+                        "source": "c4gfnttsuwqomiygupdqqqyy5y5emnk5c73hrfvatri67prd7vyq.b32.i2p",
472+                        "source_network": "i2p",

0xB10C commented at 8:29 am on December 7, 2023:

I’ve been thinking about how to clear the addrman tables in a functional test. I think it would be ideal if we could avoid the addpeeraddress test leaking into getaddrmaninfo and getrawaddrman tests.

This also becomes relevant in the context of #28998 where we might want to produce a tried table collision on purpose to have coverage of the addpeeraddress RPC failure path. The easiest way of producing a collision is by adding more and more addresses to the tried table (as we do in the unit test). This would leave us with quite a few additional addresses already in the tried table.

I currently see two possibilities for clearing the addrman tables between tests:

shutdown the node, delete peers.dat, and restart
have a third node for this test that’s only used for getrawaddrman.

0xB10C commented at 8:30 am on December 7, 2023:

We’re doing option 1. in the feature_addrman.py functional test.

stratospher commented at 6:35 am on December 16, 2023:

I think it would be ideal if we could avoid the addpeeraddress test leaking into getaddrmaninfo and getrawaddrman tests.

it would be cleaner to not need to know the existing context of addrman when dealing with getaddrmaninfo/getrawaddrman tests. both the options sound good to me!

The easiest way of producing a collision is by adding more and more addresses to the tried table (as we do in the unit test). This would leave us with quite a few additional addresses already in the tried table.

these collisions would only affect future tests added after getaddrmaninfo/getrawaddrman tests though.

0xB10C commented at 11:23 am on December 16, 2023:

The easiest way of producing a collision is by adding more and more addresses to the tried table (as we do in the unit test). This would leave us with quite a few additional addresses already in the tried table.

these collisions would only affect future tests added after getaddrmaninfo/getrawaddrman tests though.

We’d need to clean up after we found a collision.

in src/net.cpp:242 in b607c39e21 outdated

236@@ -237,7 +237,13 @@ static int GetnScore(const CService& addr)
237 std::optional<CService> GetLocalAddrForPeer(CNode& node)
238 {
239     CService addrLocal{GetLocalAddress(node)};
240-    if (gArgs.GetBoolArg("-addrmantest", false)) {
241+
242+    const auto options = gArgs.GetArgs("-addrtest");
243+    bool relayForTests = std::any_of(options.begin(), options.end(), [](const auto& option) {

mzumsande commented at 8:20 pm on December 14, 2023:

nit: code style, relay_for_tests

stratospher commented at 6:35 am on December 16, 2023:

done.

mzumsande commented at 9:31 pm on December 14, 2023: contributor

Concept ACK

Needs rebase due to a silent conflict with #27581 (feature_asmap.py).

stratospher force-pushed on Dec 16, 2023

DrahtBot removed the label CI failed on Dec 16, 2023

in test/functional/rpc_net.py:309 in ae05dd35d5 outdated

309+        colliding (depending on the value of nKey in the addrman). We avoid these collisions
310+        by using known-working addresses in a deterministic addrman with a fixed nKey value.
311         """
312         self.log.info("Test addpeeraddress")
313-        self.restart_node(1, ["-checkaddrman=1"])
314+        self.restart_node(1, ["-checkaddrman=1", "-cjdnsreachable", "-test=addrman"])

amitiuttarwar commented at 0:54 am on December 19, 2023:

these init params are added here, but are only relevant until a couple subtests later in test_getaddrmaninfo. having unrelated init params can lead to unexpected behaviors, like in this recent example. implicit dependencies between subtests can also make future development confusing, such as updating setup in an earlier test unexpectedly causing failures in seemingly unrelated tests.

leaving suggestions for improving the structure between subtests in the overall review comment

stratospher commented at 7:08 am on December 21, 2023:

makes sense! added an option in restart_node() to restart the node with an empty addrman which could potentially be useful in other tests. addresses added in addpeeraddress test don’t leak into addrman tests now.

in src/init.cpp:614 in 4629b7c85d outdated

610@@ -611,6 +611,7 @@ void SetupServerArgs(ArgsManager& argsman)
611     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);
612     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);
613     argsman.AddArg("-addrmantest", "Allows to test address relay on localhost", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
614+    argsman.AddArg("-test=<option>", "Pass a test-only option", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);

amitiuttarwar commented at 2:02 am on December 19, 2023:

I like the approach of using -test to group test-only options

two suggestions for improvement:

include the options in the help text (eg. how -onlynet or -debug handles)
throw some sort of error (or at least warning?) if an invalid option is passed through

stratospher commented at 7:09 am on December 21, 2023:

right, i’ve included both improvements!

amitiuttarwar commented at 2:08 am on December 19, 2023: contributor

approach ACK. yay for finally having addrman determinism in the functional tests 🙌🏽 couple thoughts to consider-

1. clearer subtest addrman setup in `rpc_net.py`

@0xB10C

I think it would be ideal if we could avoid the addpeeraddress test leaking into getaddrmaninfo and getrawaddrman tests.

agreed. right now, the 3 subtests (test_addpeeraddress, test_getaddrmaninfo & test_getrawaddrman) have a lot of implicit dependencies, making the overall test harder to understand & develop on.

here’s a suggestion with the intention of making the addrman setup more clear in each subtest:

introduce a helper, eg. seed_addrman which uses addpeeraddress to add addresses from all the different networks to addrman.
in the beginning of test_getaddrmaninfo & test_getrawaddrman, get a clean addrman (by using either method mentioned by @0xB10C) and call seed_addrman to populate it in an expected way. then each subtest would check the results with the relevant RPC

2. remove `addrmantest` / `test=localaddr`

I am not convinced that this test-only behavior is worth maintaining. the only thing that p2p_node_network_limited.py is testing is that 1. an addr message is received & 2. the expected services are advertised. # 1 is forced to true via the test-only codepath in GetLocalAddrForPeer, and the value of # 2 is nominal due to how easily the test-only code could diverge from the mainnet logic.

My vote is to remove this functionality & related code. Curious to hear what you think @stratospher, as well as the opinions of other reviewers.

stratospher force-pushed on Dec 21, 2023

DrahtBot added the label CI failed on Dec 21, 2023

stratospher commented at 7:10 am on December 21, 2023: contributor

thanks for the thoughtful feedback @amitiuttarwar, @0xB10C! I’ve updated the PR to address your comments.

Main changes were: (git diff)

adding an option to restart the node with an empty addrman (method 1 suggested here #29007 (review)) to prevent addpeeraddress test leaking into getaddrmaninfo and getrawaddrman tests.
listing options in -help-debug for -test=<option>

I am not convinced that this test-only behavior is worth maintaining. the only thing that p2p_node_network_limited.py is testing is that 1. an addr message is received & 2. the expected services are advertised. # 1 is forced to true via the test-only codepath in GetLocalAddrForPeer, and the value of # 2 is nominal due to how easily the test-only code could diverge from the mainnet logic.

My vote is to remove this functionality & related code. Curious to hear what you think @stratospher, as well as the opinions of other reviewers.

yes, i agree. don’t think it’s useful to have a command line arg which is used in only 1 line in 1 test. we would want to know if the address with expected services are really advertised in real world logic and not in hard coded test path logic. looks like the option was introduced in this commit.

stratospher force-pushed on Dec 22, 2023

stratospher commented at 6:28 am on December 22, 2023: contributor

Updated the PR to remove -addrmantest (git diff) in 5d27993.

in test/functional/test_framework/test_framework.py:584 in 291a1a325d outdated

581+    def restart_node(self, i, extra_args=None, clear_addrman=False):
582         """Stop and start a test node"""
583         self.stop_node(i)
584-        self.start_node(i, extra_args)
585+        if clear_addrman:
586+            peers_dat = os.path.join(self.nodes[i].chain_path, "peers.dat")

maflcko commented at 8:46 am on December 22, 2023:

0            peers_dat = self.nodes[i].chain_path / "peers.dat"

nit: You can use / to join paths

stratospher commented at 8:22 am on January 2, 2024:

nice! done.

stratospher force-pushed on Jan 2, 2024

DrahtBot removed the label CI failed on Jan 2, 2024

DrahtBot added the label Needs rebase on Jan 5, 2024

[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.

802e6e128b

[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.

be25ac3092

[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.

69e091f3e1

Revert "test: avoid non-determinism in asmap-addrman test"

This reverts commit 5825b34783545f9470d5ab94b87c918980715675.
The non-determinism is avoided by using a deterministic addrman.

7b868e6b67

[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.

71c19915c0

[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

a897866109

[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.

2cc8ca19f4

stratospher force-pushed on Jan 8, 2024

DrahtBot removed the label Needs rebase on Jan 8, 2024

DrahtBot added the label CI failed on Jan 16, 2024

in src/init.cpp:1029 in 2cc8ca19f4

1023@@ -1024,6 +1024,22 @@ bool AppInitParameterInteraction(const ArgsManager& args)
1024     if (args.GetBoolArg("-peerbloomfilters", DEFAULT_PEERBLOOMFILTERS))
1025         nLocalServices = ServiceFlags(nLocalServices | NODE_BLOOM);
1026 
1027+    if (args.IsArgSet("-test")) {
1028+        if (chainparams.GetChainType() != ChainType::REGTEST) {
1029+            return InitError(Untranslated("-test=<option> should only be used in functional tests"));

0xB10C commented at 10:30 am on January 29, 2024:

nit: I think this makes the error slightly clearer

0            return InitError(Untranslated("-test=<option> can only be used when on regtest"));

mzumsande commented at 4:37 pm on February 13, 2024:

i had the same suggestion independently (we have functional tests that don’t use regtest but testnet)

0xB10C commented at 4:06 pm on March 12, 2024:

included in https://github.com/bitcoin/bitcoin/pull/29636

0xB10C approved

0xB10C commented at 10:33 am on January 29, 2024: contributor

ACK 2cc8ca19f4185490f30a49516c890b2289fbab71

Code review and played around with the new -test option.

DrahtBot requested review from mzumsande on Jan 29, 2024

DrahtBot requested review from amitiuttarwar on Jan 29, 2024

amitiuttarwar commented at 6:57 am on February 8, 2024: contributor

ACK 2cc8ca19f4185490f30a49516c890b2289fbab71

reviewed the code & tested the init option

DrahtBot removed review request from amitiuttarwar on Feb 8, 2024

in src/addrdb.cpp:190 in 69e091f3e1 outdated

184@@ -185,7 +185,9 @@ void ReadFromStream(AddrMan& addr, DataStream& ssPeers)
185 util::Result<std::unique_ptr<AddrMan>> LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args)
186 {
187     auto check_addrman = std::clamp<int32_t>(args.GetIntArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000);
188-    auto addrman{std::make_unique<AddrMan>(netgroupman, /*deterministic=*/false, /*consistency_check_ratio=*/check_addrman)};
189+    bool deterministic = HasTestOption(args, "addrman"); // use a deterministic addrman only for tests
190+
191+    auto addrman{std::make_unique<AddrMan>(netgroupman, /*deterministic=*/deterministic, /*consistency_check_ratio=*/check_addrman)};

mzumsande commented at 4:51 pm on February 13, 2024:

nit: I don’t know if we have recomendations about this, but I find /*deterministic=*/deterministic a bit silly, might as well drop the named argument.

0xB10C commented at 4:06 pm on March 12, 2024:

included in https://github.com/bitcoin/bitcoin/pull/29636

mzumsande commented at 6:37 pm on February 13, 2024: contributor

Code Review ACK 2cc8ca19f4185490f30a49516c890b2289fbab71

DrahtBot requested review from mzumsande on Feb 13, 2024

DrahtBot removed review request from mzumsande on Feb 13, 2024

maflcko added this to the milestone 28.0 on Feb 22, 2024

achow101 commented at 2:12 pm on March 11, 2024: member

ACK 2cc8ca19f4185490f30a49516c890b2289fbab71

achow101 merged this on Mar 11, 2024

achow101 closed this on Mar 11, 2024

in test/functional/rpc_net.py:55 in 2cc8ca19f4

50+    node.addpeeraddress(address="1233:3432:2434:2343:3234:2345:6546:4534", tried=True, port=8333)
51+    node.addpeeraddress(address="2803:0:1234:abcd::1", port=45324)
52+    node.addpeeraddress(address="fc00:1:2:3:4:5:6:7", port=8333)
53+    node.addpeeraddress(address="pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion", tried=True, port=8333)
54+    node.addpeeraddress(address="nrfj6inpyf73gpkyool35hcmne5zwfmse3jl3aw23vk7chdemalyaqad.onion", port=45324, tried=True)
55+    node.addpeeraddress(address="c4gfnttsuwqomiygupdqqqyy5y5emnk5c73hrfvatri67prd7vyq.b32.i2p", port=8333)

0xB10C commented at 1:19 pm on March 12, 2024:

Ideally, we should check that these addresses are successfully added to the addrman. They work with the current deterministic addrman, but this might break silently when the addrman bucket/position calculation changes and these cause a collision.

0xB10C commented at 4:07 pm on March 12, 2024:

included in https://github.com/bitcoin/bitcoin/pull/29636

0xB10C commented at 2:58 pm on March 12, 2024: contributor

Maybe this reduces someone’s time spent debugging: I noticed in #28998 that restarting a node with -test=addrman doesn’t make an existing addrman deterministic. You also need to set clear_addrman=True to start off with fresh and deterministic addrman.

0self.restart_node(x, ["-test=addrman"], clear_addrman=True)

0xB10C referenced this in commit c54d597060 on Mar 12, 2024

0xB10C referenced this in commit 2951239915 on Mar 12, 2024

0xB10C referenced this in commit 4e46519716 on Mar 13, 2024

0xB10C referenced this in commit 777fb3d920 on Mar 13, 2024

test: create deterministic addrman in the functional tests #29007

Code Coverage

Reviews

Conflicts

1. clearer subtest addrman setup in rpc_net.py

2. remove addrmantest / test=localaddr

1. clearer subtest addrman setup in `rpc_net.py`

2. remove `addrmantest` / `test=localaddr`