func test: Expand tx download preference tests #31437

instagibbs commented at 5:41 PM on December 6, 2024: member

Check that outbound nodes are treated the same as whitelisted connections for the purposes of getdata delays
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.

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.

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

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

instagibbs force-pushed on Dec 9, 2024

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 :)

mckinly commented at 5:09 AM on December 12, 2024: none

Screenshot 2024-12-11 at 9 07 45 PM

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

yuvicc commented at 5:19 PM on January 6, 2025: contributor

Concept ACK

yuvicc commented at 6:06 PM on January 24, 2025: contributor

tACK 846a1387280fa584f70ccb1f5d0198339b065528 good catch

successfully ran the test

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.

maflcko commented at 12:27 PM on February 5, 2025: member

ACK 846a1387280fa584f70ccb1f5d0198339b065528 🍕

<details><summary>Show signature</summary>

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 846a1387280fa584f70ccb1f5d0198339b065528 🍕
AFgNKa+56fCH4tbZo+hqgq9JYWYY9ALK0y7EO/XjxR90SvE3Tnl5CPe5Pl1NiCCVdc+eJbc4hez/022gMmMDDA==

</details>

fanquake merged this on Feb 5, 2025

fanquake closed this on Feb 5, 2025

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) :/

instagibbs commented at 4:02 PM on February 6, 2025: member

@sr-gi I'll take a look

glozow referenced this in commit fb0ada982a on Feb 7, 2025

sedited referenced this in commit f78a01a560 on Feb 22, 2025

stickies-v referenced this in commit d760fd3dda on Mar 17, 2025

stickies-v referenced this in commit cc83553352 on Mar 17, 2025

stickies-v referenced this in commit 2614933f06 on Mar 17, 2025

stickies-v referenced this in commit b70418c5fc on Mar 17, 2025

stickies-v referenced this in commit 69f8a1fe50 on Mar 17, 2025

bug-castercv502 referenced this in commit d15545ffaa on Sep 28, 2025

knst referenced this in commit b33ef3b8be on Oct 22, 2025

bitcoin locked this on Feb 6, 2026