test: deduplicates p2p_tx_download constants #31758

sr-gi commented at 7:13 PM on January 29, 2025: member

Some of the networking constants defined in p2p_tx_download.py are more generally defined in p2p.py

DrahtBot commented at 7:13 PM on January 29, 2025: 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/31758.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	i-am-yuvi, danielabrozzoni, tdb3, maflcko

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Tests on Jan 29, 2025

in test/functional/p2p_tx_download.py:52 in efeaf2e0c1 outdated

  45 | @@ -43,13 +46,10 @@ def on_getdata(self, message):
  46 |  
  47 |  
  48 |  # Constants from net_processing
  49 | -GETDATA_TX_INTERVAL = 60  # seconds
  50 |  INBOUND_PEER_TX_DELAY = 2  # seconds
  51 | -TXID_RELAY_DELAY = 2 # seconds
  52 |  OVERLOADED_PEER_DELAY = 2 # seconds
  53 |  MAX_GETDATA_IN_FLIGHT = 100
  54 |  MAX_PEER_TX_ANNOUNCEMENTS = 5000

tdb3 commented at 1:38 AM on January 30, 2025:

Maybe it's worth migrating some of these to p2p.py as well?

sr-gi commented at 5:03 PM on January 30, 2025:

I thought about this, but some of these are only used in this test.

I just realized that some of them were even duplicated amongst themselves. For instance, INBOUND_PEER_TX_DELAY and NONPREF_PEER_TX_DELAY refer to the same concept.

I'll do a second pass to further trim this

tdb3 approved

tdb3 commented at 1:39 AM on January 30, 2025: contributor

ACK efeaf2e0c1f0de0d7c3819eb9beaefee2ddf0710

yuvicc approved

yuvicc commented at 5:47 AM on January 30, 2025: contributor

ACK efeaf2e0c1f0de0d7c3819eb9beaefee2ddf0710

TEST               | STATUS    | DURATION

p2p_tx_download.py | ✓ Passed  | 33 s

ALL                | ✓ Passed  | 33 s (accumulated)

sr-gi force-pushed on Jan 30, 2025

sr-gi commented at 5:17 PM on January 30, 2025: member

@tdb3 I de-duplicated even more, plus renamed some of the remaining ones to match the actual names in txdownloadman.h (and updated the comment referring to net_processing, given the constants do not live there anymore).

As mentioned in-line, notice how INBOUND_PEER_TX_DELAY and NONPREF_PEER_TX_DELAY refer to the same concept. The latter was introduced in #20171 instead of remaining (or re-using) the former.

in test/functional/p2p_tx_download.py:52 in 2b94e1abbf outdated

  52 | -TXID_RELAY_DELAY = 2 # seconds
  53 | -OVERLOADED_PEER_DELAY = 2 # seconds
  54 | -MAX_GETDATA_IN_FLIGHT = 100
  55 | +# Constants from txdownloadman
  56 | +MAX_PEER_TX_REQUEST_IN_FLIGHT = 100
  57 |  MAX_PEER_TX_ANNOUNCEMENTS = 5000

tdb3 commented at 1:07 AM on January 31, 2025:

nit: Not a blocker, but would it make sense for all of these types of constants to live in p2p.py?

sr-gi commented at 3:45 PM on January 31, 2025:

I don't have a strong opinion on this.

I've checked and looks like no other test uses this, or re-defines it. I'd lean towards leaving it locally to avoid bloating p2p.py, but I'm open to move it if there is support for it

tdb3 commented at 3:48 PM on January 31, 2025:

Works for me. If/when the file is touched again, it could be moved then.

tdb3 approved

tdb3 commented at 1:07 AM on January 31, 2025: contributor

ACK 2b94e1abbfdef058546221d63f53b0b58eea22ea

Nice cleanup

DrahtBot requested review from yuvicc on Jan 31, 2025

yuvicc approved

yuvicc commented at 5:33 PM on February 1, 2025: contributor

re-ACK 2b94e1abbfdef058546221d63f53b0b58eea22ea

danielabrozzoni approved

danielabrozzoni commented at 3:30 PM on February 3, 2025: member

tACK 2b94e1abbfdef058546221d63f53b0b58eea22ea

The code looks good to me, I think renaming MAX_GETDATA_IN_FLIGHT to MAX_PEER_TX_REQUEST_IN_FLIGHT makes it easier to refer back to txdownloadman

DrahtBot added the label Needs rebase on Feb 5, 2025

test: deduplicates p2p_tx_download constants

Some of the networking constants defined in p2p_tx_download.py are more generally
defined in p2p.py

Also, rename the remaining ones to match ones defined in txdownloadman

0a02e7fdea

sr-gi force-pushed on Feb 6, 2025

DrahtBot removed the label Needs rebase on Feb 6, 2025

sr-gi commented at 3:56 PM on February 6, 2025: member

Rebased to fix merge conflicts.

Thanks for the review @tdb3 @danielabrozzoni @i-am-yuvi

yuvicc commented at 7:42 PM on February 6, 2025: contributor

re-ACK 0a02e7fdeaca26455b3710ef76b11101ac816e53

DrahtBot requested review from tdb3 on Feb 6, 2025

DrahtBot requested review from danielabrozzoni on Feb 6, 2025

sr-gi commented at 4:28 PM on February 13, 2025: member

@tdb3 @danielabrozzoni can you give this another look? The changes should be trivial

danielabrozzoni commented at 5:14 PM on February 13, 2025: member

Whoops, I had forgotten about this one :)

re-ACK 0a02e7fdeaca26455b3710ef76b11101ac816e53

tdb3 approved

tdb3 commented at 12:55 AM on February 14, 2025: contributor

re ACK 0a02e7fdeaca26455b3710ef76b11101ac816e53

maflcko commented at 6:34 AM on February 14, 2025: member

review ACK 0a02e7fdeaca26455b3710ef76b11101ac816e53 🔖

<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: review ACK 0a02e7fdeaca26455b3710ef76b11101ac816e53 🔖
jwepvbYV1gQLs1UZ9KxP6py22frJ6KEnwpkEv92Yl2aCC/5z7nmNDGrLnib/Rr1nVykvGBpPblkyvCq8YjX2AQ==

</details>

fanquake merged this on Feb 14, 2025

fanquake closed this on Feb 14, 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

tomt1664 referenced this in commit 9a162e29ff on Apr 15, 2026

Kino1994 referenced this in commit 9f27e6b2f7 on Jun 28, 2026

BigcoinBGC referenced this in commit 4d6265f6a9 on Jun 30, 2026

bitcoin locked this on Jul 30, 2026