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.

   Type	Reviewers
   ACK	i-am-yuvi, marcofleon, maflcko

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #31758 (test: deduplicates p2p_tx_download constants by sr-gi)

   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.

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
8. 
   i-am-yuvi commented at 5:19 pm on January 6, 2025: contributor

   Concept ACK
9. 
   i-am-yuvi commented at 6:06 pm on January 24, 2025: contributor

   tACK 846a1387280fa584f70ccb1f5d0198339b065528 good catch

   • successfully ran the test
10. 
    marcofleon commented at 3:59 pm on January 29, 2025: contributor

    lgtm ACK 846a1387280fa584f70ccb1f5d0198339b065528

    I commented out the delay addition for a non-preferred peer in AddTxAnnouncement and the inbound connection check in test_preferred_inv failed as expected.

    Also did a simple change making all peers preferred when calling ReceivedInv. Removing that distinction causes test_preferred_tiebreaker_inv to fail (most of the time) because there’s only a 1/11 chance pref_peer gets selected first.

11. 
    maflcko commented at 12:27 pm on February 5, 2025: member

    ACK 846a1387280fa584f70ccb1f5d0198339b065528 🍕

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: ACK 846a1387280fa584f70ccb1f5d0198339b065528 🍕
    3AFgNKa+56fCH4tbZo+hqgq9JYWYY9ALK0y7EO/XjxR90SvE3Tnl5CPe5Pl1NiCCVdc+eJbc4hez/022gMmMDDA==
    

12. fanquake merged this on Feb 5, 2025
13. fanquake closed this on Feb 5, 2025

    ---

14. 
    sr-gi commented at 3:55 pm on February 6, 2025: member

    This seems to have heavily increased the run time of the test (from ~30s to ~90s on my local setup) :/
15. 
    instagibbs commented at 4:02 pm on February 6, 2025: member

    @sr-gi I’ll take a look

Contributors
instagibbs DrahtBot mckinly i-am-yuvi marcofleon maflcko sr-gi