test: let connections happen in any order in p2p_private

vasild commented at 12:05 pm on January 26, 2026: contributor

If the following two events happen:

(likely) the automatic 10 initial connections are not made to all networks
(unlikely) the network-specific logic kicks in almost immediately. It is using exponential distribution with a mean of 5 minutes (rng.rand_exp_duration(EXTRA_NETWORK_PEER_INTERVAL)).

So if both happen, then the 11th connection may not be the expected private broadcast, but a network-specific connection.

Fix this by retrieving the connection type from destinations_factory(). This is more flexible because it allows connections to happen in any order and does not break if e.g. the 11th connection is not the expected first private broadcast.

This also makes the test run faster: before: 19-44 sec now: 10-25 sec because for example there is no need to wait for the initial 10 automatic outbound connections to be made in order to proceed.

Fixes: #34387

DrahtBot added the label Tests on Jan 26, 2026

DrahtBot commented at 12:05 pm on January 26, 2026: 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/34410.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	w0xlt, Bicaru20

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34457 (wallet: add private broadcast support for wallet transactions by w0xlt)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

“to to_addr:to_port” -> “to to_addr:to_port” [remove duplicated preposition; the phrase currently has “to” twice before the address, which is confusing — better: “to to_addr:to_port” or, clearer, “to {to_addr}:{to_port}”]
“’n’th.” -> “n-th” or “nth” [the form “’n’th” is not standard English; use “n-th” or “nth” to indicate the nth element]

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

LogDebug(BCLog::NET, “trying %s connection (%s) to %s, lastseen=%.1fhrs\n”, use_v2transport ? “v2” : “v1”, ConnectionTypeAsString(conn_type), pszDest ? pszDest : addrConnect.ToStringAddrPort(), Ticks(pszDest ? 0h : Now() - addrConnect.nTime)) in src/net.cpp
check_broadcasts(“Basic”, txs[0], NUM_PRIVATE_BROADCAST_PER_TX - 1, 0) in test/functional/p2p_private_broadcast.py

2026-02-04 05:42:03

maflcko added this to the milestone 31.0 on Jan 26, 2026

DrahtBot added the label CI failed on Jan 26, 2026

fanquake commented at 2:15 pm on January 26, 2026: member

Looks like this causes the test to timeout in most CIs. I see the same failure locally.

vasild force-pushed on Jan 27, 2026

vasild commented at 1:44 pm on January 27, 2026: contributor

01757699f3e7a03c32289de4aaee6d04cd2de921...38c0c8900af3a88c9a0c0e79df9fb20511f4bfa2: fixup, my counting was flawed - manual connections count toward network specific connections, but not for automatic connections. I wonder if this can be made better, by deciding on the fly where to redirect each connection…

DrahtBot removed the label CI failed on Jan 27, 2026

vasild force-pushed on Jan 30, 2026

vasild renamed this:
~~test: ensure p2p_private_broadcast.py has connections to all nets~~
test: let connections happen in any order in p2p_private_broadcast.py
on Jan 30, 2026

vasild commented at 1:24 pm on January 30, 2026: contributor

… I wonder if this can be made better, by deciding on the fly where to redirect each connection…

Yes:

38c0c8900af3a88c9a0c0e79df9fb20511f4bfa2...25ee8bd7f3973ddadbd484a132ffe1f1b5925e57: change the approach to let connections happen in any order, is more robust and faster.

vasild force-pushed on Jan 30, 2026

vasild commented at 1:27 pm on January 30, 2026: contributor

25ee8bd7f3973ddadbd484a132ffe1f1b5925e57...4e12c91a581d646b15c3c825cbc9cc0a9632399c: pet python linter

DrahtBot added the label CI failed on Jan 30, 2026

DrahtBot commented at 1:27 pm on January 30, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/21517256715/job/61998660210 LLM reason (✨ experimental): CI failed due to lint errors (ruff E711: comparisons to None) in Python code.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

net: extend log message to include attempted connection type 67696b207f

vasild force-pushed on Feb 2, 2026

vasild commented at 8:17 am on February 2, 2026: contributor

4e12c91a581d646b15c3c825cbc9cc0a9632399c...e28c3bd45f514974bdf411e48224fe7f1e079080: magic spell to tame the QA gods

DrahtBot removed the label CI failed on Feb 2, 2026

in test/functional/p2p_private_broadcast.py:306 in e28c3bd45f

301+            def get_destinations_len():
302+                with self.destinations_lock:
303+                    return len(self.destinations)
304+            self.wait_until(lambda: get_destinations_len() > i)
305+            with self.destinations_lock:
306+                return self.destinations[i]

Bicaru20 commented at 12:01 pm on February 2, 2026:

Since you are passing the parameter n, isn’t it better to use the n instead of the i?

0            self.wait_until(lambda: get_destinations_len() > n)
1            with self.destinations_lock:
2                return self.destinations[n]

vasild commented at 5:42 am on February 4, 2026:

Right, thanks!

in test/functional/p2p_private_broadcast.py:409 in e28c3bd45f outdated

404+                        if tx_returner is None:
405+                            tx_returner = dest["node"]
406+                            assert(type(tx_returner) is P2PDataStore)
407+                        elif other_peer is None:
408+                            other_peer = dest["node"]
409+                            assert(type(other_peer) is P2PInterface)

Bicaru20 commented at 7:03 pm on February 2, 2026:

I believe isinstance is better and a little bit faster in for checking the type. The only reason I can think of for using type is to avoid subclasses passing the condition. If that is the case, then typeis fine.

0                            assert(isinstance(tx_returner, P2PDataStore))
1                        elif other_peer is None:
2                            other_peer = dest["node"]
3                            assert(isinstance(other_peer, P2PInterface))

vasild commented at 5:45 am on February 4, 2026:

P2PDataStore is a subclass of P2PInterface, so if isinstance is used the second assert could pass when it should fail.

Bicaru20 commented at 7:05 pm on February 2, 2026: none

Concept ACK. I left some comments to try to improve the code.

test: let connections happen in any order in p2p_private_broadcast.py

If the following two events happen:

* (likely) the automatic 10 initial connections are not made to all
  networks
* (unlikely) the network-specific logic kicks in almost immediately.
  It is using exponential distribution with a mean of 5 minutes
  (`rng.rand_exp_duration(EXTRA_NETWORK_PEER_INTERVAL)`).

So if both happen, then the 11th connection may not be the expected
private broadcast, but a network-specific connection.

Fix this by retrieving the connection type from
`destinations_factory()`. This is more flexible because it allows
connections to happen in any order and does not break if e.g. the 11th
connection is not the expected first private broadcast.

This also makes the test run faster:
before: 19-44 sec
now: 10-25 sec
because for example there is no need to wait for the initial 10
automatic outbound connections to be made in order to proceed.

Fixes: https://github.com/bitcoin/bitcoin/issues/34387

8aa763d464

test: use port 0 for I2P addresses in p2p_private_broadcast.py

I2P addresses must use port=0, otherwise `bitcoind` refuses to connect.

The test `p2p_private_broadcast.py` cannot simulate connections to I2P
peers, so the I2P proxy is set to a dummy `127.0.0.1:1`. Still it is
good to pick I2P addresses and attempt connections to increase coverage.

However the test uses port=8333 for I2P addresses and thus the
connection attempts fail for the "wrong" reason:

```
Error connecting to ...i2p:8333, connection refused due to arbitrary port 8333
```

Using the proper port=0 makes the failures:

```
Error connecting to ...i2p:0: Cannot connect to 127.0.0.1:1
```

which will make possible simulated I2P connections once we have a test
I2P proxy.

fd7afbe2ab

in test/functional/p2p_private_broadcast.py:193 in e28c3bd45f

188+            Return the connection type (outbound-full-relay, private-broadcast, etc) or
189+            None if there is no connection attempt to to_addr:to_port.
190+            """
191+            with open(self.tx_originator_debug_log_path, mode="r", encoding="utf-8") as debug_log:
192+                for line in debug_log.readlines():
193+                    match = re.match(f".*trying v. connection .(.+). to .?{to_addr}.?:{to_port},.*", line)

w0xlt commented at 4:36 am on February 3, 2026:

nit: regex pattern could be more precise

0                    match = re.match(f".*trying v. connection \\(([^)]+)\\) to .?{to_addr}.?:{to_port},.*", line)

vasild commented at 5:48 am on February 4, 2026:

Yes, I avoided the \\ because they give me a headache ;-) changed now. Also changed the .? to the more specific [? and ]? which surround IPv6 addresses.

vasild force-pushed on Feb 4, 2026

vasild commented at 5:42 am on February 4, 2026: contributor

e28c3bd45f514974bdf411e48224fe7f1e079080...fd7afbe2abaf7fb8bf32450008fd4dfd18967ed8: address suggestions

DrahtBot added the label CI failed on Feb 4, 2026

DrahtBot removed the label CI failed on Feb 4, 2026

w0xlt commented at 8:51 pm on February 4, 2026: contributor

ACK fd7afbe2abaf7fb8bf32450008fd4dfd18967ed8

DrahtBot requested review from Bicaru20 on Feb 4, 2026

Bicaru20 commented at 10:54 am on February 10, 2026: none

Code review ACK fd7afbe2abaf7fb8bf32450008fd4dfd18967ed8. I tested and reviewed the code and everything looks good.

test: let connections happen in any order in p2p_private_broadcast.py #34410

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)