net: skip unusable addrman picks in ThreadPrivateBroadcast #36560

kravens commented at 5:54 PM on October 17, 2026: none

ThreadPrivateBroadcast keeps opening connections until all planned sends for a privately broadcast transaction are done (since #36277 also after the transaction has been received back). If every address addrman returns for the picked network is a peer we already have a connection to, OpenNetworkConnection() rejects it immediately and the loop retries every 100 ms, logging Failed to connect to ... will retry to a different address each time. On a node that only knows a few onion/I2P peers this spins for as long as the sends remain pending, which can be indefinitely.

I ran into this while testing v32.0rc4 (https://github.com/bitcoin/bitcoin/issues/36315#issuecomment-6235074058, observation 2): a regtest sender reachable only through Tor with a single .onion address in addrman logged ~600 Failed to connect lines per minute for as long as I watched, with no SOCKS traffic, after the transaction had already been delivered and received back.

This PR checks the conditions that make OpenNetworkConnection() refuse an address (already connected, banned or discouraged) before attempting the connection, and treats such picks like invalid ones: after 100 unusable picks in a row the thread waits 500 ms between picks and logs about it once per 100 picks instead of on every attempt. Behaviour for usable addresses is unchanged.

Same scenario with this change (master, two isolated regtest nodes over the real Tor network): the transaction was delivered in 16 s, no Failed to connect lines, and one Connections needed but addrman keeps returning unusable addresses, will keep retrying line every ~50 s while the remaining sends stay pending.

Tested with p2p_private_broadcast.py, p2p_private_broadcast_cap.py, p2p_private_broadcast_retry_v1.py, p2p_private_broadcast_completion.py and feature_proxy.py.

The rejection conditions are now checked both here and in OpenNetworkConnection(). Having OpenNetworkConnection() report whether it rejected the address up front or the attempt failed would avoid that duplication, but seemed out of scope for this fix.

Disclosure: AI (Claude Fable 5.1) was used to find this issue and to draft this PR.

net: skip unusable addrman picks in ThreadPrivateBroadcast

When a transaction is privately broadcast, ThreadPrivateBroadcast keeps
opening connections until all planned sends are done. If every address
addrman returns for the picked network is one we already have a
connection to (or is banned/discouraged), OpenNetworkConnection() fails
immediately and the loop retries every 100ms, logging a "Failed to
connect ... will retry" line each time. On a node that only knows a few
onion/I2P peers this spins for as long as the sends remain pending,
which can be indefinitely.

Check the conditions that make OpenNetworkConnection() refuse an address
before attempting the connection and treat such picks like invalid ones:
retry after 500ms once this has happened 100 times in a row, and log
about it once per 100 picks instead of on every attempt.

a2776cf463

DrahtBot added the label P2P on Oct 17, 2026

DrahtBot commented at 5:54 PM on October 17, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36560
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36560.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36560#author-ralph

Reviews

See the guideline and AI policy for information on the review process. A summary of reviews will appear here.

pinheadmz commented at 10:44 PM on October 17, 2026: member

Please review our AI policy, and then you can comment here if you'd still like to contribute to the project in a meaningful way.

https://github.com/bitcoin/bitcoin/blob/master/doc/AI_POLICY.md

pinheadmz closed this on Oct 17, 2026

kravens commented at 10:54 PM on October 17, 2026: none

Yes, I've added the disclosure after reading the policy. This is a small improvement I found during the release candidate testing for v32.0. It's a minor bugfix at best, you can ignore it if you don't want to review AI generated code at all.

pinheadmz commented at 10:58 PM on October 17, 2026: member

if you don't want to review AI generated code at all.

Did you review the AI generated code at all?

We want new people to contribute to the project, just in a more meaningful way. If you're interested in really helping the Bitcoin project, this is a good place to start: https://bitcoincore.academy/

mzumsande commented at 1:54 AM on October 18, 2026: contributor

What is missing is (human) insight why is this a problem: On startup, dozens of fixed seeds will be put into addrman, if you have just a few addresses you should 1) be asking yoursrelf why and fix this and 2) not be doing private broadcast in the first place. If you have multiple onion peers you are connected to, you should also be able to get more onion addresses quickly through address relay, so why would this situation stay indefinitely?

kravens commented at 8:30 AM on October 18, 2026: none

@pinheadmz I have reviewed and tested it on a local regtest node. I've been active on delvingbitcoin for a while, also a longtime bitdevs amsterdam participant. Hoped this could have been a nice small scope first PR :) will checkout the academy too! @mzumsande I've encountered this issue only during testing with regtest addrman, so you are right a regular mainnet node will not stay indefinitely. Two things remain. On a young node the tried table is roughly the set of currently connected peers (10 tried onion entries, 12 outbound connections in that run), and Select() picks tried 50% of the time, so about half of the private broadcast picks are already-connected peers that OpenNetworkConnection() rejects up front, each costing a 100 ms sleep and a "Failed to connect, will retry" debug line. And on signet/testnet the I2P pool is 7-14 fixed seeds, so a fully connected pool is reachable there. This PR would help those two cases, and is only a minor fix. Possibly another way is preventing private broadcast by sendrawtransaction with a tiny pool of peers.