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

i-am-yuvi approved

i-am-yuvi 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 i-am-yuvi on Jan 31, 2025

i-am-yuvi approved

i-am-yuvi 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: contributor

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

i-am-yuvi 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: contributor

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