tests: add coverage to feature_addrman.py #28176

kevkevinpal · 2023-07-28T04:35:33Z

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

DrahtBot · 2023-07-28T04:35:37Z

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

Reviews

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.

ismaelsadeeq

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.

you can calculate them in a first commit

diff --git a/test/functional/feature_addrman.py b/test/functional/feature_addrman.py
index 7877f9d302..191a68d612 100755
--- a/test/functional/feature_addrman.py
+++ b/test/functional/feature_addrman.py
@@ -14,7 +14,9 @@ from test_framework.test_framework import BitcoinTestFramework
 from test_framework.test_node import ErrorMatch
 from test_framework.util import assert_equal
 
 -
+ADDRMAN_NEW_BUCKET_COUNT = 1 << 10
+ADDRMAN_TRIED_BUCKET_COUNT = 1 << 8
+ADDRMAN_BUCKET_SIZE = 1 << 6
 def serialize_addrman(
     *,
     format=1,
@@ -117,17 +119,19 @@ class AddrmanTest(BitcoinTestFramework):
 
         self.log.info("Check that corrupt addrman cannot be read (len_tried)")
         self.stop_node(0)
+        max_len_tried = ADDRMAN_TRIED_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE
         write_addrman(peers_dat, len_tried=-1)
         self.nodes[0].assert_start_raises_init_error(
-            expected_msg=init_error("Corrupt AddrMan serialization: nTried=-1, should be in \\[0, 16384\\]:.*"),
+            expected_msg=init_error(f"Corrupt AddrMan serialization: nTried=-1, should be in \\[0, {max_len_tried}\\]:.*"),
             match=ErrorMatch.FULL_REGEX,
         )
 
         self.log.info("Check that corrupt addrman cannot be read (len_new)")
         self.stop_node(0)
+        max_len_n_new = ADDRMAN_NEW_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE
         write_addrman(peers_dat, len_new=-1)
         self.nodes[0].assert_start_raises_init_error(
-            expected_msg=init_error("Corrupt AddrMan serialization: nNew=-1, should be in \\[0, 65536\\]:.*"),
+            expected_msg=init_error(f"Corrupt AddrMan serialization: nNew=-1, should be in \\[0, {max_len_n_new}\\]:.*"),
             match=ErrorMatch.FULL_REGEX,
         )

then use them for the test coverage in second commit

diff --git a/test/functional/feature_addrman.py b/test/functional/feature_addrman.py
index 191a68d612..eb7eaaf9df 100755
--- a/test/functional/feature_addrman.py
+++ b/test/functional/feature_addrman.py
@@ -126,6 +126,13 @@ class AddrmanTest(BitcoinTestFramework):
             match=ErrorMatch.FULL_REGEX,
         )
 
+        self.log.info("Check that corrupt addrman cannot be read (large len_tried)")
+        write_addrman(peers_dat, len_tried=max_len_tried + 1)
+        self.nodes[0].assert_start_raises_init_error(
+            expected_msg=init_error(f"Corrupt AddrMan serialization: nTried={max_len_tried + 1}, should be in \\[0, {max_len_tried}\\]:.*"),
+            match=ErrorMatch.FULL_REGEX,
+        )
+
         self.log.info("Check that corrupt addrman cannot be read (len_new)")
         self.stop_node(0)
         max_len_n_new = ADDRMAN_NEW_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE
@@ -135,6 +142,14 @@ class AddrmanTest(BitcoinTestFramework):
             match=ErrorMatch.FULL_REGEX,
         )
 
+        self.log.info("Check that corrupt addrman cannot be read (large len_new)")
+        self.stop_node(0)
+        write_addrman(peers_dat, len_new=max_len_n_new + 1)
+        self.nodes[0].assert_start_raises_init_error(
+            expected_msg=init_error(f"Corrupt AddrMan serialization: nNew={max_len_n_new + 1}, should be in \\[0, {max_len_n_new}\\]:.*"),
+            match=ErrorMatch.FULL_REGEX,
+        )
+

see e309230 and 19921ce

kevkevinpal · 2023-09-25T05:15:13Z

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.
you can calculate them in a first commit
then use them for the test coverage in second commit

see e309230 and 19921ce

Makes sense took your two commits e309230 and 19921ce squashed them into one and added you as coauthor thanks for the review!

test/functional/feature_addrman.py

ismaelsadeeq · 2023-09-25T16:01:49Z

CI failure is unrelated?

kevkevinpal · 2023-09-25T22:12:02Z

CI failure is unrelated?

yea I think so looks like the test is failing in test/functional/wallet_fundrawtransaction.py

stratospher

ACK eb195d6.

test/functional/feature_addrman.py

ismaelsadeeq

Tested ACK 1b044a309d48821ae014256eb56d0f83a1de969b

kevkevinpal · 2023-09-30T19:27:09Z

force pushed to d2f87a3

This addresses #28176 (comment) and moves the constant variables to the netutil.py file (cc @0xB10C)

test/functional/test_framework/netutil.py

0xB10C

Code Review ACK d2f87a3f5bc6ac833fee6e5d05452ebe2e137f22

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 <[email protected]>

ismaelsadeeq · 2023-10-02T10:24:13Z

ACK 380130d, code looks good to me 🍃 .
Tests passed this branch rebased on master.

0xB10C · 2023-10-02T10:25:50Z

Re-ACK 380130d

380130d test: add coverage to feature_addrman.py (kevkevin) Pull request description: 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 ACKs for top commit: ismaelsadeeq: ACK 380130d, code looks good to me 🍃 . 0xB10C: Re-ACK 380130d Tree-SHA512: a063bd9ca4d2d536a27c8c22a28fb13759a96f19cd8ba6cb8879cf7f65046d4ff6e8f70df17feaffd0d0d08ef914cb18a11258d313a4841c811a7e7ae4df6d5b

DrahtBot added the Tests label Jul 28, 2023

achow101 requested a review from theStack September 20, 2023 17:16

ismaelsadeeq reviewed Sep 24, 2023

View reviewed changes

kevkevinpal force-pushed the test-addrman-large-vals branch 2 times, most recently from c9ecdac to 4b84a69 Compare September 25, 2023 05:13

DrahtBot added the CI failed label Sep 25, 2023

kevkevinpal requested a review from ismaelsadeeq September 25, 2023 15:08

ismaelsadeeq reviewed Sep 25, 2023

View reviewed changes

test/functional/feature_addrman.py Outdated Show resolved Hide resolved

DrahtBot removed the CI failed label Sep 27, 2023

kevkevinpal force-pushed the test-addrman-large-vals branch from 4b84a69 to eb195d6 Compare September 28, 2023 03:59

stratospher reviewed Sep 29, 2023

View reviewed changes

test/functional/feature_addrman.py Outdated Show resolved Hide resolved

test/functional/feature_addrman.py Outdated Show resolved Hide resolved

kevkevinpal force-pushed the test-addrman-large-vals branch from eb195d6 to 1b044a3 Compare September 29, 2023 16:53

ismaelsadeeq approved these changes Sep 30, 2023

View reviewed changes

DrahtBot requested a review from stratospher September 30, 2023 12:23

kevkevinpal force-pushed the test-addrman-large-vals branch 2 times, most recently from bcab758 to d2f87a3 Compare September 30, 2023 19:26

DrahtBot added the CI failed label Sep 30, 2023

DrahtBot removed the CI failed label Sep 30, 2023

0xB10C reviewed Oct 1, 2023

View reviewed changes

test/functional/test_framework/netutil.py Outdated Show resolved Hide resolved

0xB10C mentioned this pull request Oct 1, 2023

rpc: add hidden getrawaddrman RPC to list addrman table entries #28523

Merged

0xB10C approved these changes Oct 1, 2023

View reviewed changes

DrahtBot requested review from ismaelsadeeq and stratospher and removed request for stratospher October 1, 2023 10:06

test: add coverage to feature_addrman.py

380130d

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 <[email protected]>

kevkevinpal force-pushed the test-addrman-large-vals branch from d2f87a3 to 380130d Compare October 2, 2023 03:44

DrahtBot requested review from 0xB10C and stratospher and removed request for ismaelsadeeq and stratospher October 2, 2023 10:24

DrahtBot requested review from stratospher and removed request for 0xB10C and stratospher October 2, 2023 10:25

fanquake merged commit 8909667 into bitcoin:master Oct 2, 2023

bitcoin locked and limited conversation to collaborators Oct 1, 2024

tests: add coverage to feature_addrman.py #28176

tests: add coverage to feature_addrman.py #28176

Uh oh!

Conversation

kevkevinpal commented Jul 28, 2023 • edited by fanquake Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Uh oh!

kevkevinpal commented Sep 25, 2023

Uh oh!

Uh oh!

ismaelsadeeq commented Sep 25, 2023

Uh oh!

kevkevinpal commented Sep 25, 2023

Uh oh!

stratospher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Uh oh!

kevkevinpal commented Sep 30, 2023

Uh oh!

Uh oh!

0xB10C left a comment

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq commented Oct 2, 2023

Uh oh!

0xB10C commented Oct 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kevkevinpal commented Jul 28, 2023 •

edited by fanquake

Loading

DrahtBot commented Jul 28, 2023 •

edited

Loading