func test: Expand tx download preference tests #31437

1. 
   instagibbs commented at 5:41 pm on December 6, 2024: member

   1. Check that outbound nodes are treated the same as whitelisted connections for the purposes of getdata delays

   2. Add test case that demonstrates download retries are preferentially given to outbound (preferred) connections even when multiple announcements are considered ready.

   NUM_INBOUND is a magic number large enough that it should fail over 90% of the time if the underlying outbound->preferred->PriorityComputer logic was broken. Bumping this to 100 peers cost another 14 seconds locally for the sub-test, so I made it pretty small.

2. 
   DrahtBot commented at 5:41 pm on December 6, 2024: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31437.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

3. func test: Expand tx download preference tests

   1. Check that outbound nodes are treated
   the same as whitelisted connections for
   the purposes of getdata delays
   
   2. Add test case that demonstrates
   download retries are preferentially
   given to outbound (preferred) connections
   even when multiple announcements are
   considered ready.

   846a138728
4. in test/functional/p2p_tx_download.py:196 in 8902a5971d outdated

   192@@ -193,25 +193,90 @@ def test_notfound_fallback(self):
   193         peer_notfound.send_and_ping(msg_notfound(vec=[CInv(MSG_WTX, WTXID)]))  # Send notfound, so that fallback peer is selected
   194         peer_fallback.wait_until(lambda: peer_fallback.tx_getdata_count >= 1, timeout=1)
   195 
   196-    def test_preferred_inv(self, preferred=False):
   197-        if preferred:
   198-            self.log.info('Check invs from preferred peers are downloaded immediately')
   199+    def test_preferred_inv(self, preferred_type):
   

   ---

   ---

   

   mckinly commented at 1:48 am on December 9, 2024:

   nit: consider updating the preferred_type with a type hint and an enum to avoid using “magic strings”

   ---

   

   instagibbs commented at 3:25 pm on December 9, 2024:

   good idea, done
5. instagibbs force-pushed on Dec 9, 2024
6. in test/functional/p2p_tx_download.py:207 in 846a138728

   202@@ -193,25 +203,90 @@ def test_notfound_fallback(self):
   203         peer_notfound.send_and_ping(msg_notfound(vec=[CInv(MSG_WTX, WTXID)]))  # Send notfound, so that fallback peer is selected
   204         peer_fallback.wait_until(lambda: peer_fallback.tx_getdata_count >= 1, timeout=1)
   205 
   206-    def test_preferred_inv(self, preferred=False):
   207-        if preferred:
   208-            self.log.info('Check invs from preferred peers are downloaded immediately')
   209+    def test_preferred_inv(self, connection_type: ConnectionType):
   210+        if connection_type == ConnectionType.WHITELIST:
   

   ---

   ---

   

   mckinly commented at 5:02 pm on December 9, 2024:

   One more nit 😅 would a switch (match/case) work better here than if/else? Edit: just reviewed the PR guidelines again. Testing and N/ACK come first before nits (if any). My bad!

   ---

   

   instagibbs commented at 6:41 pm on December 9, 2024:

   a bit too nitty, keeping as is :)
7. 
   mckinly commented at 5:09 am on December 12, 2024: none

   

   1. Checked out the branch
   2. ✅ Ran build/test/functional/test_runner.py p2p_tx_download.py

Contributors
instagibbs DrahtBot mckinly