The nKey of the addrman is generated the first time the node is started with an empty peers.dat. Therefore, restarting a node or turning it off and on again won’t make a previously non-deterministic addrman deterministic.
This could lead to intermittent failures in feature_asmap.py and rpc_net.py
Fixes #29634
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | stratospher, brunoerg, kevkevinpal, 0xB10C |
| Concept ACK | BrandonOdiwuor |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
Concept ACK
Hope you saw #29007 (comment)
Concept ACK
Hope you saw #29007 (comment)
Thanks, yes I saw that. It’s the same underlying issue - in feature_asmap.py we didn’t use restart_node but stop_node and then start_node, so the current fix is not to use clear_addrman=True, but make addrman deterministic from the start.
The nKey of the addrman is generated the first time the node is
started. Therefore, restarting a node or turning it off and on
again won't make a previously non-deterministic addrman
deterministic.
Co-authored-by: 0xb10c <b10c@b10c.me>
318@@ -319,7 +319,7 @@ def test_getnodeaddresses(self):
319
320 def test_addpeeraddress(self):
321 self.log.info("Test addpeeraddress")
322- self.restart_node(1, ["-checkaddrman=1", "-test=addrman"])
323+ self.restart_node(1, ["-checkaddrman=1", "-test=addrman"], clear_addrman=True)
-test=addrman is not enough (and possibly required for other sub-tests needing a deterministic addrman too).
rpc_net.py yet, but just from looking at the code it seemed that this could fail too.
Tested ACK 432a542e271f5b6ecb1c6ea4fa9108ad4b3a5a43.
oh nice catch @mzumsande, @0xB10C! so nKey from Addrman’s constructor is always overwritten by previously stored nKey obtained from Unserialize().
I’ve checked the test logs in feature_asmap.py and rpc_net.py to verify that deterministic nKey is being used in the tests now.
ACK 432a542
ran this command grep -nri "restart_node" ./test/functional to check for where -test=addrman' is used and saw that we were just missing it in test/functional/rpc_net.py`
was not able to test this locally on my machine as I am on mac
38@@ -39,7 +39,8 @@ def expected_messages(filename):
39 class AsmapTest(BitcoinTestFramework):
40 def set_test_params(self):
41 self.num_nodes = 1
42- self.extra_args = [["-checkaddrman=1"]] # Do addrman checks on all operations.
43+ # Do addrman checks on all operations and use deterministic addrman
44+ self.extra_args = [["-checkaddrman=1", "-test=addrman"]]
clear_addrman = true required in the restart_node call inside test_asmap_interaction_with_addrman_containing_entries function below?
set_test_params() is called before the node is first started. clear_addrman = True would only be required if the node was running and a non-deterministic addrman had been created before.
318@@ -319,7 +319,9 @@ def test_getnodeaddresses(self):
319
320 def test_addpeeraddress(self):
321 self.log.info("Test addpeeraddress")
322- self.restart_node(1, ["-checkaddrman=1", "-test=addrman"])
323+ # The node has an existing, non-deterministic addrman from a previous test.
