-
Notifications
You must be signed in to change notification settings - Fork 37.7k
test: add addpeeraddress "tried", test addrman checks on restart with asmap #22831
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: add addpeeraddress "tried", test addrman checks on restart with asmap #22831
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK on adding the test
2fb6933
to
792c9c2
Compare
Re-pushed with a 20x smaller |
Maybe still include docs on how to generate the file? (See also #16796 (review)) |
792c9c2
to
24a73b4
Compare
Good idea. Done in the commit message. |
24a73b4
to
d507068
Compare
They began as the same commit together, then I realized that it makes more sense for them to be separate:
The easiest way to test the peers.dat file is to copy it to a datadir, start a node, and run commands on it like |
d507068
to
48820e6
Compare
Rebased, improved the commit messages, added additional test coverage, and renamed the test file to |
48820e6
to
e35494f
Compare
e35494f
to
4cda5cf
Compare
Rebased and merged the addrman and asmap tests into one test file. |
55d1411
to
4de3271
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review ACK 4de3271 🍛
Show signature and timestamp
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
review ACK 4de3271b1cb65111ea4b37d8fa9791aa0cd63b81 🍛
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjVpwwAnW+G/Az3yxcnBnDlYpPF+EnaE+9mRU+rBuVH4ta4EXbKmnkGBXcUMFES
vGyOSh214YYpBVMO0ibGV/5V0PwSeijSnSgFS90JgpL6kbGJ9a+Bcp2uc+nH8yAI
6dCGwn40XN8oA0sNHbbqffsaFe4PdDIPsR+zV8Up/2SynpJhwxcN58Nf1KvC+dZD
vliNJO3LnMfVB9Gdah6ObJ2d99iqqWIyg8+aaWxYpUwMp/4oRtkKjZ7CHS9C4TdE
aeVybThyVnD8csbWpx/yKAxjiP9dVZ2yPzXmhTmVZPoWrUH+T3nN8IgYLv0+EkU6
NFXIdz1qZZbJ93TKl9RFIPxgB8LX/NH0nO9sRZji61jsSFKJMSv6HmQfYdAO5/tC
8VUD2pqVnSvbnKYCfpfQEk8lkepDfM0FSH4uE7h3U8VysCbPsDJZpomC845eynai
THR4aeDwVylKCVnHZ7MUWSxAxxvjuh/i6SUYsPEKyxM8r9PcErhmYML2mafVKRdq
86qyGL//
=dWm5
-----END PGP SIGNATURE-----
Timestamp of file with hash 4c359cb1138b60d4da195cd16d1d3fb668bf9409ad9ed02c51ecd1964b7f50f2 -
5593ea1
to
d586817
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review ACK d586817
two comment suggestions to consider if you retouch
Co-authored-by: Martin Zumsande <mzumsande@gmail.com> Co-authored-by: John Newbery <john@johnnewbery.com> Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org> Co-authored-by: John Newbery <john@johnnewbery.com> Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
d586817
to
71bea5d
Compare
PR bitcoin#22697 introduced a reproducible issue in commit 181a120 that causes the addrman tried table to fail consistency checks and significantly lose peer entries when the `-asmap` configuration option is used. The issue occurs on bitcoind restart due to an initialization order change in `src/init.cpp` in that commit whereby CAddrman asmap is set after deserializing `peers.dat`, rather than before. Issue reported on the `#bitcoin-core-dev` IRC channel starting at https://www.erisian.com.au/bitcoin-core-dev/log-2021-08-23.html#l-263. ``` addrman lost 22813 new and 2 tried addresses due to collisions or invalid addresses ADDRMAN CONSISTENCY CHECK FAILED!!! err=-17 bitcoind: ./addrman.h:707: void CAddrMan::Check() const: Assertion `false' failed. Aborted ``` How to reproduce: - `git checkout 181a120` and recompile - launch bitcoind with `-asmap` and `-checkaddrman=1` config options - restart bitcoind - bitcoind aborts on second call to `CAddrMan::Check()` This commit adds a regression test to reproduce the case; it passes or fails with the same error. Co-authored-by: John Newbery <john@johnnewbery.com> Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
71bea5d
to
cdaab90
Compare
Very good suggestions (thanks!) Updated per diff
diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index f7b6a2a333..227eec722f 100644
--- a/src/rpc/net.cpp
+++ b/src/rpc/net.cpp
@@ -956,7 +956,7 @@ static RPCHelpMan addpeeraddress()
if (node.addrman->Add({address}, address)) {
success = true;
if (tried) {
- // Mark entry as good and possibly move it to the "tried" addresses table.
+ // Attempt to move the address to the tried addresses table.
node.addrman->Good(address);
}
}
diff --git a/test/functional/feature_asmap.py b/test/functional/feature_asmap.py
index 46d55077d7..2dc1e3a7cb 100755
--- a/test/functional/feature_asmap.py
+++ b/test/functional/feature_asmap.py
@@ -14,7 +14,7 @@ Verify node behaviour and debug log when launching bitcoind in these cases:
4. `bitcoind -asmap/-asmap=` with no file specified, using the default asmap
-5. `bitcoind -asmap` with an addrman containing new and tried entries
+5. `bitcoind -asmap` restart with an addrman containing new and tried entries
6. `bitcoind -asmap` with no file specified and a missing default asmap file
@@ -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 2 new addresses and 2 tried addresses to the addrman."""
- for addr, tried in [[0, False], [1, False], [2, True], [3, True]]:
+ """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):
@@ -81,13 +81,18 @@ class AsmapTest(BitcoinTestFramework):
os.remove(self.default_asmap)
def test_asmap_interaction_with_addrman_containing_entries(self):
- self.log.info("Test bitcoind -asmap with an addrman containing new and tried entries")
+ 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.fill_addrman(node_id=0)
self.restart_node(0, ["-asmap", "-checkaddrman=1"])
- with self.node.assert_debug_log(expected_msgs=["Addrman checks started: new 2, tried 2, total 4"]):
+ with self.node.assert_debug_log(
+ expected_msgs=[
+ "Addrman checks started: new 2, tried 2, total 4",
+ "Addrman checks completed successfully",
+ ]
+ ):
self.node.getnodeaddresses() # getnodeaddresses re-runs the addrman checks
os.remove(self.default_asmap)
diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py
index e2dfa3ea23..4f5d8ce241 100755
--- a/test/functional/rpc_net.py
+++ b/test/functional/rpc_net.py
@@ -239,6 +239,14 @@ 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"])
node = self.nodes[1]
@@ -252,19 +260,21 @@ class NetTest(BitcoinTestFramework):
assert_equal(node.addpeeraddress(address="", port=8333), {"success": False})
assert_equal(node.getnodeaddresses(count=0), [])
- self.log.debug("Test that adding a valid address succeeds")
- assert_equal(node.addpeeraddress(address="1.2.3.4", port=8333), {"success": True})
- addrs = node.getnodeaddresses(count=0)
- assert_equal(len(addrs), 1)
- assert_equal(addrs[0]["address"], "1.2.3.4")
- assert_equal(addrs[0]["port"], 8333)
+ self.log.debug("Test that adding a valid address to the tried table succeeds")
+ assert_equal(node.addpeeraddress(address="1.2.3.4", tried=True, port=8333), {"success": True})
+ with node.assert_debug_log(expected_msgs=["Addrman checks started: new 0, tried 1, total 1"]):
+ addrs = node.getnodeaddresses(count=0) # getnodeaddresses re-runs the addrman checks
+ assert_equal(len(addrs), 1)
+ assert_equal(addrs[0]["address"], "1.2.3.4")
+ assert_equal(addrs[0]["port"], 8333)
- self.log.debug("Test that adding the same address again when already present fails")
- assert_equal(node.addpeeraddress(address="1.2.3.4", port=8333), {"success": False})
+ self.log.debug("Test that adding an already-present tried address to the new and tried tables fails")
+ for value in [True, False]:
+ assert_equal(node.addpeeraddress(address="1.2.3.4", tried=value, port=8333), {"success": False})
assert_equal(len(node.getnodeaddresses(count=0)), 1)
- self.log.debug("Test adding a second address (with a different /16) to the tried table")
- assert_equal(node.addpeeraddress(address="2.0.0.0", port=8333, tried=True), {"success": True})
+ self.log.debug("Test that adding a second address, this time to the new table, succeeds")
+ assert_equal(node.addpeeraddress(address="2.0.0.0", port=8333), {"success": True})
with node.assert_debug_log(expected_msgs=["Addrman checks started: new 1, tried 1, total 2"]):
addrs = node.getnodeaddresses(count=0) # getnodeaddresses re-runs the addrman checks |
utACK cdaab90 This isn't a blocker, but I've never liked relying too heavily on the In this case, we could extend the Like I said, not a blocker for this PR, but perhaps something to consider as a potential follow-up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK cdaab90
The new test Test bitcoind -asmap restart with addrman containing new and tried entries
fails before the fix (50fd77045e~
) and passes with the fix (50fd77045e
).
"""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. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this nondeterminism turns out to be a problem also in other cases, then I guess a new option bitcoind -addrmandeterministic
could be introduced which creates a deterministic addrman.
with node.assert_debug_log(expected_msgs=["Addrman checks started: new 1, tried 1, total 2"]): | ||
addrs = node.getnodeaddresses(count=0) # getnodeaddresses re-runs the addrman checks | ||
assert_equal(len(addrs), 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't check the contents of addrs[]
here, but checked it above. If the reason for this is that the order is undeterministic, then I guess it can be sorted and then checked that it contains 1.2.3.4
and 2.0.0.0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in #23035.
This seems ready for merge. |
I don't want to belabor drama here, but there is a categorical difference between (i) extant bugs and (ii) footgun conditions that might lead to a bug. One case requires a clear and immediate fix (and if feasible an accompanying test) and the other can be responsibly treated in a number of ways with varying rapidity.
While I appreciate the sentiment here, this is a controversial, security-critical project that is likely to someday experience subversion, overt or otherwise. Consequently we should be eager to recognize and address process failures, and we should certainly not be gunshy about adding test coverage, whether or not its implementation is ideal. |
Thanks for adjusting this to use Concept ACK (didn't re-review) |
@jamesob - I'm not going to continue the discussion of process in this PR, since I think we're basically going in circles at this point. However, I do want to respond to your final point:
I totally disagree with this. Our standards for test code should be as high as for code in the product itself, since all code (test or production) carries an ongoing maintenance cost. If a proposed test introduces difficult-to-maintain binary files or intermittently fails, then those issues absolutely should be brought up in review and resolved before merge. I agree with @fanquake's assessment here:
There's no rush here. A merge-first-fix-later attitude increases the overall cost of this code and transfers that cost onto developers who'll have to maintain/modify the tests later. |
… asmap Summary: ``` This pull adds a tried argument to RPC addpeeraddress and a regression test for the recent addrman/asmap changes and issue. PR #22697 introduced a reproducible bug in commit 181a120 that fails addrman consistency checks and causes it to significantly lose peer entries when the -asmap configuration option is used. The issue occurs upon bitcoind restart due to an initialization order change in src/init.cpp in that commit, whereby CAddrman asmap is set after deserializing peers.dat, rather than before. ``` Backport of [[bitcoin/bitcoin#22831 | core#22831]]. Note that we never suffered that bug, but still it's worth checking we don't introduce it at a later point in time. Depends on D12312. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D12313
This pull adds a
tried
argument to RPC addpeeraddress and a regression test for the recent addrman/asmap changes and issue.PR #22697 introduced a reproducible bug in commit 181a120 that fails addrman consistency checks and causes it to significantly lose peer entries when the
-asmap
configuration option is used.The issue occurs upon bitcoind restart due to an initialization order change in
src/init.cpp
in that commit, whereby CAddrman asmap is set after deserializingpeers.dat
, rather than before.Issue reported on the
#bitcoin-core-dev
IRC channel starting at https://www.erisian.com.au/bitcoin-core-dev/log-2021-08-23.html#l-263.How to reproduce:
git checkout 181a1207
, build, and launch bitcoind with the-asmap
and-checkaddrman=1
configuration options enabledCAddrMan::Check()
How to test this pull:
git checkout 181a1207
, cherry pick the first commit of this branch, build, git checkout this branch, runtest/functional/rpc_net.py
, which should pass, and then runtest/functional/feature_asmap.py
, which should fail with the following output: