net: use the proxy if overriden when doing v2->v1 reconnections #35319

OpenNetworkConnection() supports overriding the proxy to use for connecting. However when v2 connection is attempted and it fails a v1 connection is tried without that proxy.

Store the override proxy in CNode and pass it to CConnman::m_reconnections to be used for v1 retries.

This is a serious flaw I have overlooked when the override proxy is used for private broadcast connections. Reported in https://www.erisian.com.au/bitcoin-core-dev/log-2026-05-18.html#l-234

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35319.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

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

<!--5faf32d7da4f0f540f40219e4f7537a3-->

However when v2 connection is attempted and it fails a v1 connection is tried without that proxy

Just to clarify the impact, does this mean that "ipv4-through-tor" private broadcast connections that fall back to v1 (e.g. because the peer does not support it) are re-attempted as clearnet connections, and therefore directly reveal the IP address of the tx sender?

Whoops!

Seems like a great time to get some V2 tests in p2p_private_broadcast.py :) Looks like we mainly disable it currently.

How does this play with FinalizeNode? Do we still make a new private broadcast connection to a new peer? ISTM that we might make two broadcasts; one to original peer using V1, and one to a new peer, which might be acceptable or might not be what we want...

(answers without testing, just by reading the code) @dergoegge, yes, exactly! @willcl-ark, I think it will do v1 to the original address and schedule a new connection.

I am looking to add a test for this. Trying to avoid extending p2p_private_broadcast.py because it is already big and to avoid adding a new 200+ lines test and to avoid changing non-test code in order to accommodate the new test...

... a test for this ...

Here are two preliminary tests. Both fail without this PR and pass with this PR:

Unit test The test is 106 lines, not too bad. Plus a reusable extension is added to TestingSetup which makes a total of +150/-0 diff. Non-testing code is not modified. https://github.com/vasild/bitcoin/commit/49f8d35345fdcb922dde4257e3658d6e9789b247

Functional test The test is 105 lines. Plus an extension of the addnode RPC - a total of +139/-9 diff. Non-testing code is modified to accommodate the test, which I do not like. https://github.com/vasild/bitcoin/commit/c96c769e5ba3dc2842b953943a53e4848a39f2a6

What do you think?

PS should be possible to make a functional test that does not need non-testing code modifications. That would exercise the OpenNetworkConnection(proxy_override=...) path via private broadcast which currently is the only user of the "override proxy" feature of OpenNetworkConnection(). I guess that would go in the 200+ lines. Maybe sharing some code with p2p_private_broadcast.py would reduce it.

How does this play with FinalizeNode? Do we still make a new private broadcast connection to a new peer? ISTM that we might make two broadcasts; one to original peer using V1, and one to a new peer, which might be acceptable or might not be what we want...

This seems like a bug or at least is confusing: in addition to connecting to a new peer (since FinalizeNode on the downgraded peer using v2 would increment the counter again), if the downgraded peer using v1 also fails in DidNodeConfirmReception in FinalizeNode, the m_num_to_open counter leaks (temporarily) by 1.

tried a slightly different tactic to reduce chance of future regressions, seeing what people think about it: https://github.com/instagibbs/bitcoin/commits/2026-05-private_tx_proxy/

Unit test The test is 106 lines, not too bad. Plus a reusable extension is added to TestingSetup which makes a total of +150/-0 diff. Non-testing code is not modified. https://github.com/vasild/bitcoin/commit/49f8d35345fdcb922dde4257e3658d6e9789b247

I think this is helpful as a unit test, though imo it'd be great if the existing p2p_private_broadcast.py functional test could be changed to also test the downgrade which might let us get coverage for the extra connection mentioned here.

The test is 105 lines. Plus an extension of the addnode RPC - a total of +139/-9 diff. Non-testing code is modified to accommodate the test, which I do not like. https://github.com/vasild/bitcoin/commit/c96c769e5ba3dc2842b953943a53e4848a39f2a6

Is there a specific reason it can't be added to the existing functional test? Is it too complicated? I think modifying non-test code here is a downside. I think @instagibbs branch would let you do the functional test without modifying addnode since there's no threading proxy_override.

tried a slightly different tactic to reduce chance of future regressions, seeing what people think about it: https://github.com/instagibbs/bitcoin/commits/2026-05-private_tx_proxy/

My thoughts in: https://github.com/instagibbs/bitcoin/commit/fc8ed47d39817b17239a4705021ed7e2d229e150#r186068246 or https://github.com/instagibbs/bitcoin/commit/fc8ed47d39817b17239a4705021ed7e2d229e150#commitcomment-186068694 (not sure if github deleted one of those)

Is there a specific reason it can't be added to the existing functional test? @Crypt-iQ, from #35319 (comment):

Trying to avoid extending p2p_private_broadcast.py because it is already big

Probably it can be added and doing so it tempting and will probably result in less number of lines added. However I have walked this road before and it does not lead to a nice place - eventually we end up with a huge test that is difficult to manage and debug once some piece of it fails. I would rather try to split p2p_private_broadcast.py into smaller independent pieces instead of bloating it further. We already had an issue with it being too big and difficult to manage where some addition to that test caused it to fail in sporadic and non-obvious way.

I think @instagibbs #35319 (comment) would let you do the functional test without modifying addnode since there's no threading proxy_override

Hmm... threading (as in passing around the proxy_override and remembering it for the v1 retry) or not shouldn't make a difference to the outside user who uses the RPC interface?

I would rather try to split p2p_private_broadcast.py into smaller independent pieces instead of bloating it further.

I think that would be good, if there is time to do that before the minor release.

Hmm... threading (as in passing around the proxy_override and remembering it for the v1 retry) or not shouldn't make a difference to the outside user who uses the RPC interface?

By this I meant that in your functional test above you needed to modify addnode to pass in a proxy_override and in Greg's test there wouldn't be a need to do that. If there's a way to test without the non-test change, then I'm all for it.

My thoughts in: https://github.com/instagibbs/bitcoin/commit/fc8ed47d39817b17239a4705021ed7e2d229e150#r186068246 or https://github.com/instagibbs/bitcoin/commit/fc8ed47d39817b17239a4705021ed7e2d229e150#commitcomment-186068694 (not sure if github deleted one of those)

Github deleted one (time to move...). I think the layering violation point makes sense, though I'll point out there is some awareness of private broadcast in ConnectNode since it checks if the connection type is private broadcast in one branch.

https://github.com/instagibbs/bitcoin/commit/6f786de776df31b40a8193ac50d4d5333e132a24

alternative test attempt for consideration in part/full/whatever. For convenience adds a service flag arg for debug only rpc, though not strictly required. It's kinda long, but should be fairly robust to regressions?

What about adding something like this so we don't connect directly for any reason:

diff --git a/src/net.cpp b/src/net.cpp
index 7edb8eacf4..430db0eb08 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -488,6 +488,9 @@ CNode* CConnman::ConnectNode(CAddress addrConnect,
                 LogDebug(BCLog::PROXY, "Using proxy: %s to connect to %s\n", use_proxy->ToString(), target_addr.ToStringAddrPort());
                 sock = ConnectThroughProxy(*use_proxy, target_addr.ToStringAddr(), target_addr.GetPort(), proxyConnectionFailed);
             } else {
+                if (conn_type == ConnectionType::PRIVATE_BROADCAST && target_addr.GetSAFamily() != AF_UNSPEC) {
+                    continue;
+                }
                 // no proxy needed (none set for target network)
                 sock = ConnectDirectly(target_addr, conn_type == ConnectionType::MANUAL);
             }