test: fix intermittent failures with test=addrman #29639
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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) |
mzumsande
force-pushed
the
202403_fix_feature_asmap
branch
2 times, most recently
from
82f4ecf to
bdf80d1
Compare
Thanks, yes I saw that. It's the same underlying issue - in |
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 <[email protected]>
mzumsande
force-pushed
the
202403_fix_feature_asmap
branch
from
bdf80d1 to
432a542
Compare
|
Concept ACK |
stratospher
left a comment
stratospher
left a comment
There was a problem hiding this comment.
Tested ACK 432a542.
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 was not able to test this locally on my machine as I am on mac |
| self.num_nodes = 1 | ||
| self.extra_args = [["-checkaddrman=1"]] # Do addrman checks on all operations. | ||
| # Do addrman checks on all operations and use deterministic addrman | ||
| self.extra_args = [["-checkaddrman=1", "-test=addrman"]] |
There was a problem hiding this comment.
Isn't clear_addrman = true required in the restart_node call inside test_asmap_interaction_with_addrman_containing_entries function below?
There was a problem hiding this comment.
In this case it's not needed. 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.
There was a problem hiding this comment.
Thanks for the explanation. 🙌
| def test_addpeeraddress(self): | ||
| self.log.info("Test addpeeraddress") | ||
| self.restart_node(1, ["-checkaddrman=1", "-test=addrman"]) | ||
| # The node has an existing, non-deterministic addrman from a previous test. |
There was a problem hiding this comment.
Which is that previous test? Should we add the name in the comment?
There was a problem hiding this comment.
Any previous test that started the node without a deterministic addrman. I don't think it's needed in the comment.
10 participants
The
nKeyof the addrman is generated the first time the node is started with an emptypeers.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.pyandrpc_net.pyFixes #29634