net: support overriding the proxy selection in ConnectNode()

vasild commented at 1:57 pm on September 22, 2025: contributor

Normally ConnectNode() would choose whether to use a proxy and which one. Make it possible to override this from the callers and same for OpenNetworkConnection() - pass down the proxy to ConnectNode().

Document both functions.

This is useful if we want to open connections to IPv4 or IPv6 peers through the Tor SOCKS5 proxy.

Also have OpenNetworkConnection() return whether the connection succeeded or not. This can be used when the caller needs to keep track of how many (successful) connections were opened.

This is part of #29415 Broadcast own transactions only via short-lived Tor or I2P connections. Putting it in its own PR to reduce the size of #29415 and because it does not depend on the other commits from there.

DrahtBot added the label P2P on Sep 22, 2025

DrahtBot commented at 1:57 pm on September 22, 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/33454.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stratospher, optout21, mzumsande, andrewtoth

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:

#32747 (Introduce SockMan (“lite”): low-level socket handling for HTTP by pinheadmz)
#32394 (net: make m_nodes_mutex non-recursive by vasild)
#32065 (i2p: make a time gap between creating transient sessions and using them by vasild)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
#28463 (p2p: Increase inbound capacity for block-relay only connections by mzumsande)

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.

vasild force-pushed on Sep 22, 2025

andrewtoth commented at 3:45 pm on September 22, 2025: contributor

Need to update https://github.com/bitcoin/bitcoin/pull/33454/files#diff-00021eed586a482abdb09d6cdada1d90115abe988a91421851960e26658bed02L2882 strDest -> pszDest to placate tidy. If you’re updating the name anyways, maybe just get rid of the hungarian notation and call it destination?

vasild force-pushed on Sep 23, 2025

vasild commented at 9:34 am on September 23, 2025: contributor

504803f909...48c5939cf4: fix tidy, note that also in master there is inconsistency: the prototype of OpenNetworkConnection() in net.h uses strDest whereas the implementation in net.cpp uses pszDest. Resolved by this PR.

If you’re updating the name anyways, maybe just get rid of the hungarian notation and call it destination?

Yes, I think this would be very nice to do. I avoided it because it would bloat the diff as a lot of lines inside the body of the functions will be touched. Then there are also the other parameters addrConnect and fCountFailure that would warrant a rename. Further, addrConnect and pszDest both mean the same thing and should both be called destination :-X maybe combine them into one variable std::variant<CAddress, const char*> destination? Either way I would like to keep this PR minimal, only containing what is needed for #29415.

in src/net.cpp:401 in 48c5939cf4

397+CNode* CConnman::ConnectNode(CAddress addrConnect,
398+                             const char* pszDest,
399+                             bool fCountFailure,
400+                             ConnectionType conn_type,
401+                             bool use_v2transport,
402+                             std::optional<Proxy> proxy_arg)

sipa commented at 12:17 pm on September 23, 2025:

Pass by reference? Proxy is pretty large (80 bytes).

vasild commented at 1:29 pm on September 23, 2025:

Right, done!

vasild force-pushed on Sep 23, 2025

vasild commented at 1:28 pm on September 23, 2025: contributor

48c5939cf4...ec7c215345: do #33454 (review)

andrewtoth approved

andrewtoth commented at 4:28 pm on September 27, 2025: contributor

utACK ec7c21534505bb727ad0344c7f6881836b4e9ec5

Fairly simple change that is clearly unused here so will have no effect. Hopefully can make #29415 easier to review and get in for v31.

optout21 commented at 10:21 am on September 29, 2025: none

Concept ACK ec7c21534505bb727ad0344c7f6881836b4e9ec5

One doubt: adding a return type to OpenNetworkConnection is not described in the commit msg nor in the PR; and it seems to be needed only later by #29415 . I suggest leaving it out (as the return value is not used in this PR), or alternatively, just documenting it in the commit msg & PR description.

maflcko commented at 10:25 am on September 29, 2025: member

The CI seems to fail reliably?

maflcko added the label CI failed on Sep 29, 2025

maflcko removed the label CI failed on Sep 29, 2025

vasild force-pushed on Sep 29, 2025

vasild commented at 12:04 pm on September 29, 2025: contributor

ec7c215345...82e46a7248: rebase to hopefully fix CI and elaborate the return value of OpenNetworkConnection() in the commit message.

in src/net.cpp:476 in 82e46a7248

472+            bool use_proxy;
473+            if (proxy_arg.has_value()) {
474+                use_proxy = true;
475+                proxy = proxy_arg.value();
476+            } else {
477+                use_proxy = GetProxy(addrConnect.GetNetwork(), proxy);

mzumsande commented at 7:48 pm on September 29, 2025:

Is this an oversight? Before, we’d use the network of target_addr instead of addrConnect, which makes more sense to me.

vasild commented at 7:43 am on September 30, 2025:

Right! Changed to target_addr.

in src/net.h:1416 in 82e46a7248

1412+    CNode* ConnectNode(CAddress addrConnect,
1413+                       const char* pszDest,
1414+                       bool fCountFailure,
1415+                       ConnectionType conn_type,
1416+                       bool use_v2transport,
1417+                       const std::optional<Proxy>& proxy)

mzumsande commented at 8:04 pm on September 29, 2025:

should be proxy_arg to be consistent with the definition (although maybe proxy_override would be better?)

vasild commented at 7:43 am on September 30, 2025:

I like proxy_override, changed.

mzumsande commented at 8:07 pm on September 29, 2025: contributor

Concept ACK

vasild force-pushed on Sep 30, 2025

vasild commented at 7:43 am on September 30, 2025: contributor

82e46a7248...2ae966b222: address suggestions, thanks!

in src/net.h:1160 in 2ae966b222

1156+                               bool fCountFailure,
1157+                               CountingSemaphoreGrant<>&& grant_outbound,
1158+                               const char* pszDest,
1159+                               ConnectionType conn_type,
1160+                               bool use_v2transport,
1161+                               const std::optional<Proxy>& proxy = std::nullopt)

optout21 commented at 8:27 am on September 30, 2025:

Would proxy_override be a more precise description of the semantics of the proxy parameter here as well, similar to how it’s named in ConnectNode?

vasild commented at 12:57 pm on September 30, 2025:

I agree. Changed.

vasild force-pushed on Sep 30, 2025

vasild commented at 12:57 pm on September 30, 2025: contributor

2ae966b222...0c718da693: #33454 (review) plus use the correct comment: /*proxy_override=*/.

optout21 commented at 3:40 pm on September 30, 2025: none

ACK 0c718da693ad4727009e656938a1208ec52866af

Fairly reduced scope; motivated by #29415 .

DrahtBot requested review from andrewtoth on Sep 30, 2025

DrahtBot requested review from mzumsande on Sep 30, 2025

DrahtBot added the label Needs rebase on Sep 30, 2025

vasild force-pushed on Oct 1, 2025

vasild commented at 2:19 pm on October 1, 2025: contributor

0c718da693...73071b0cdb: rebase due to conflicts

vasild force-pushed on Oct 1, 2025

vasild commented at 2:36 pm on October 1, 2025: contributor

73071b0cdb...5b22928a8e: fix silent merge conflict with a newly added call to OpenNetworkConnection() which upset tidy with /*strDest=*/ vs /*pszDest=*/.

DrahtBot removed the label Needs rebase on Oct 1, 2025

DrahtBot added the label Needs rebase on Oct 1, 2025

net: support overriding the proxy selection in ConnectNode()

Normally `ConnectNode()` would choose whether to use a proxy and which
one. Make it possible to override this from the callers and same for
`OpenNetworkConnection()` - pass down the proxy to `ConnectNode()`.

Document both functions.

This is useful if we want to open connections to IPv4 or IPv6 peers
through the Tor SOCKS5 proxy.

Also have `OpenNetworkConnection()` return whether the connection
succeeded or not. This can be used when the caller needs to keep track
of how many (successful) connections were opened.

c76de2eea1

vasild force-pushed on Oct 2, 2025

vasild commented at 7:55 am on October 2, 2025: contributor

5b22928a8e...c76de2eea1: rebase due to conflicts

DrahtBot removed the label Needs rebase on Oct 2, 2025

stratospher commented at 10:13 am on October 2, 2025: contributor

ACK c76de2e.

(non-blocking question) did you consider saving proxy in ThreadPrivateBroadcast() in m_private_broadcast object and then retrieving it later in ConnectNode instead of passing the proxy as a parameter through OpenNetworkConnection/ConnectNode? It would eliminate some parameter passing that’s only used for private broadcast connections. but might make proxy lifetime unclear (not sure about this), just curious if you considered this.

DrahtBot requested review from optout21 on Oct 2, 2025

optout21 commented at 10:19 am on October 2, 2025: none

ACK c76de2eea18076f91dd80b52f66ba790f071a2b1

Changes since last review: rebase only.

vasild commented at 11:37 am on October 2, 2025: contributor

did you consider saving proxy in ThreadPrivateBroadcast() in m_private_broadcast object and then retrieving it later in ConnectNode instead of passing the proxy as a parameter through OpenNetworkConnection/ConnectNode? It would eliminate some parameter passing that’s only used for private broadcast connections. but might make proxy lifetime unclear (not sure about this), just curious if you considered this.

In general, saving the proxy somewhere and later retrieving it from ConnectNode() would make it difficult, from within ConnectNode() to figure out (or to reason about) where did the value came from. During debugging and during code analysis. Similar to what happens with global variables.

Also that would make ConnectNode() private broadcast aware - in there will be code that is specific to private broadcast and cannot be reused. I think it is better to have a generally available functionality of being able to override the proxy.

mzumsande commented at 5:16 pm on October 2, 2025: contributor

Code Review ACK c76de2eea18076f91dd80b52f66ba790f071a2b1

andrewtoth approved

andrewtoth commented at 2:25 pm on October 4, 2025: contributor

ACK c76de2eea18076f91dd80b52f66ba790f071a2b1

glozow merged this on Oct 6, 2025

glozow closed this on Oct 6, 2025

vasild deleted the branch on Oct 6, 2025

net: support overriding the proxy selection in ConnectNode() #33454

Code Coverage & Benchmarks

Reviews

Conflicts