I added two new tests that will cover the nNew and nTried tests which add coverage to the if block by checking values larger than our range since we only check for negative values now
adding coverage to these lines https://github.com/bitcoin/bitcoin/blob/master/src/addrman.cpp#L273 https://github.com/bitcoin/bitcoin/blob/master/src/addrman.cpp#L280
our test seem to only cover the nTried < 0 and nNew < 0 scenarios
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | ismaelsadeeq, 0xB10C |
| Stale ACK | stratospher |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Concept ACK on adding test coverage to feature_addrman
You could make it more standard by calculating the maximum len_new and len_tried with the real values of ADDRMAN_NEW_BUCKET_COUNT, ADDRMAN_TRIED_BUCKET_COUNT, ADDRMAN_BUCKET_SIZE.
This reduced magic numbers and make it easier to update if any of the constants were to be updated in a future PR.
0diff --git a/test/functional/feature_addrman.py b/test/functional/feature_addrman.py
1index 7877f9d302..191a68d612 100755
2--- a/test/functional/feature_addrman.py
3+++ b/test/functional/feature_addrman.py
4@@ -14,7 +14,9 @@ from test_framework.test_framework import BitcoinTestFramework
5 from test_framework.test_node import ErrorMatch
6 from test_framework.util import assert_equal
7
8 -
9+ADDRMAN_NEW_BUCKET_COUNT = 1 << 10
10+ADDRMAN_TRIED_BUCKET_COUNT = 1 << 8
11+ADDRMAN_BUCKET_SIZE = 1 << 6
12 def serialize_addrman(
13 *,
14 format=1,
15@@ -117,17 +119,19 @@ class AddrmanTest(BitcoinTestFramework):
16
17 self.log.info("Check that corrupt addrman cannot be read (len_tried)")
18 self.stop_node(0)
19+ max_len_tried = ADDRMAN_TRIED_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE
20 write_addrman(peers_dat, len_tried=-1)
21 self.nodes[0].assert_start_raises_init_error(
22- expected_msg=init_error("Corrupt AddrMan serialization: nTried=-1, should be in \\[0, 16384\\]:.*"),
23+ expected_msg=init_error(f"Corrupt AddrMan serialization: nTried=-1, should be in \\[0, {max_len_tried}\\]:.*"),
24 match=ErrorMatch.FULL_REGEX,
25 )
26
27 self.log.info("Check that corrupt addrman cannot be read (len_new)")
28 self.stop_node(0)
29+ max_len_n_new = ADDRMAN_NEW_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE
30 write_addrman(peers_dat, len_new=-1)
31 self.nodes[0].assert_start_raises_init_error(
32- expected_msg=init_error("Corrupt AddrMan serialization: nNew=-1, should be in \\[0, 65536\\]:.*"),
33+ expected_msg=init_error(f"Corrupt AddrMan serialization: nNew=-1, should be in \\[0, {max_len_n_new}\\]:.*"),
34 match=ErrorMatch.FULL_REGEX,
35 )
0diff --git a/test/functional/feature_addrman.py b/test/functional/feature_addrman.py
1index 191a68d612..eb7eaaf9df 100755
2--- a/test/functional/feature_addrman.py
3+++ b/test/functional/feature_addrman.py
4@@ -126,6 +126,13 @@ class AddrmanTest(BitcoinTestFramework):
5 match=ErrorMatch.FULL_REGEX,
6 )
7
8+ self.log.info("Check that corrupt addrman cannot be read (large len_tried)")
9+ write_addrman(peers_dat, len_tried=max_len_tried + 1)
10+ self.nodes[0].assert_start_raises_init_error(
11+ expected_msg=init_error(f"Corrupt AddrMan serialization: nTried={max_len_tried + 1}, should be in \\[0, {max_len_tried}\\]:.*"),
12+ match=ErrorMatch.FULL_REGEX,
13+ )
14+
15 self.log.info("Check that corrupt addrman cannot be read (len_new)")
16 self.stop_node(0)
17 max_len_n_new = ADDRMAN_NEW_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE
18@@ -135,6 +142,14 @@ class AddrmanTest(BitcoinTestFramework):
19 match=ErrorMatch.FULL_REGEX,
20 )
21
22+ self.log.info("Check that corrupt addrman cannot be read (large len_new)")
23+ self.stop_node(0)
24+ write_addrman(peers_dat, len_new=max_len_n_new + 1)
25+ self.nodes[0].assert_start_raises_init_error(
26+ expected_msg=init_error(f"Corrupt AddrMan serialization: nNew={max_len_n_new + 1}, should be in \\[0, {max_len_n_new}\\]:.*"),
27+ match=ErrorMatch.FULL_REGEX,
28+ )
29+
see https://github.com/bitcoin/bitcoin/commit/e309230694512b03c055409f5d70136ab51719f6 and https://github.com/bitcoin/bitcoin/commit/19921ce5e95624469e64d00bf9194b4a67c1ce95
Concept ACK on adding test coverage to
feature_addrmanYou could make it more standard by calculating the maximum
len_newandlen_triedwith the real values ofADDRMAN_NEW_BUCKET_COUNT,ADDRMAN_TRIED_BUCKET_COUNT,ADDRMAN_BUCKET_SIZE. This reduced magic numbers and make it easier to update if any of the constants were to be updated in a future PR. you can calculate them in a first commit then use them for the test coverage in second commit
Makes sense took your two commits https://github.com/bitcoin/bitcoin/commit/e309230694512b03c055409f5d70136ab51719f6 and https://github.com/bitcoin/bitcoin/commit/19921ce5e95624469e64d00bf9194b4a67c1ce95 squashed them into one and added you as coauthor thanks for the review!
13@@ -14,7 +14,9 @@
14 from test_framework.test_node import ErrorMatch
15 from test_framework.util import assert_equal
16
17-
18+ADDRMAN_NEW_BUCKET_COUNT = 1 << 10
19+ADDRMAN_TRIED_BUCKET_COUNT = 1 << 8
20+ADDRMAN_BUCKET_SIZE = 1 << 6
CI failure is unrelated?
yea I think so looks like the test is failing in test/functional/wallet_fundrawtransaction.py
135 match=ErrorMatch.FULL_REGEX,
136 )
137
138 self.log.info("Check that corrupt addrman cannot be read (len_new)")
139 self.stop_node(0)
140+ max_len_n_new = ADDRMAN_NEW_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE
s/max_len_n_new/max_len_new for consistency in naming with max_len_tried?
force pushed to d2f87a3
This addresses #28176 (review) and moves the constant variables to the netutil.py file (cc @0xB10C)
24@@ -25,6 +25,10 @@
25 STATE_LISTEN = '0A'
26 # STATE_CLOSING = '0B'
27
nit
0
1# Address manager size constants as defined in addrman_impl.h
I added two new tests that will cover the nNew and nTried tests which
add coverage to the if block by checking values larger than our range
since we only check for negative values now
Co-authored-by: ismaelsadeeq <ask4ismailsadiq@gmail.com>
