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.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #35027 (net: add -outboundbind option for outgoing source address by 8144225309)

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.

<!--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);
             }

65c790e841bad84b2b3c2db67b27c8bc576e9b31...280708cef37434305d04bdc0b4563246ef9fe770: address suggestions

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

Makes sense, a kind of safety net for the future. Added with some mods: no need to check the address type - all private broadcast connections require a proxy. And made it an Assume() - if that happens it designates a bug, severe enough to stop debug builds and with a reasonable action "try another address" in release builds.

This is a serious flaw

Can you also add a release note (which we will also use for the minor release notes), which explains the issue, so anyone using -privatebroadcast can know if they've been impacted (I can't recall if all users may be affected, or if there are any "safe" configurations).

280708cef37434305d04bdc0b4563246ef9fe770...32d072a49f3048dffe2397ceb2155770a3e09af5: add release notes and take the fuzz test change from above.

Can we get the unit test added here at least?

Can we remove proxy_override default in OpenNetworkConnection while we're at it? Then it is the caller's responsibility to provide it, which I think is safer.

<details>

<summary>diff on 32d072a49f3048dffe2397ceb2155770a3e09af5</summary>

diff --git a/src/net.cpp b/src/net.cpp
index 3e2b81b09a..a97341cec2 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1919,7 +1919,7 @@ bool CConnman::AddConnection(const std::string& address, ConnectionType conn_typ
     CountingSemaphoreGrant<> grant(*semOutbound, true);
     if (!grant) return false;
 
-    OpenNetworkConnection(CAddress(), false, std::move(grant), address.c_str(), conn_type, /*use_v2transport=*/use_v2transport);
+    OpenNetworkConnection(CAddress(), false, std::move(grant), address.c_str(), conn_type, /*use_v2transport=*/use_v2transport, std::nullopt);
     return true;
 }
 
@@ -2443,7 +2443,7 @@ void CConnman::ProcessAddrFetch()
     CAddress addr;
     CountingSemaphoreGrant<> grant(*semOutbound, /*fTry=*/true);
     if (grant) {
-        OpenNetworkConnection(addr, false, std::move(grant), strDest.c_str(), ConnectionType::ADDR_FETCH, use_v2transport);
+        OpenNetworkConnection(addr, false, std::move(grant), strDest.c_str(), ConnectionType::ADDR_FETCH, use_v2transport, std::nullopt);
     }
 }
 
@@ -2572,7 +2572,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, std
             for (const std::string& strAddr : connect)
             {
                 CAddress addr(CService(), NODE_NONE);
-                OpenNetworkConnection(addr, false, {}, strAddr.c_str(), ConnectionType::MANUAL, /*use_v2transport=*/use_v2transport);
+                OpenNetworkConnection(addr, false, {}, strAddr.c_str(), ConnectionType::MANUAL, /*use_v2transport=*/use_v2transport, std::nullopt);
                 for (int i = 0; i < 10 && i < nLoop; i++)
                 {
                     if (!m_interrupt_net->sleep_for(500ms)) {
@@ -2922,7 +2922,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, std
             const bool count_failures{((int)outbound_ipv46_peer_netgroups.size() + outbound_privacy_network_peers) >= std::min(m_max_automatic_connections - 1, 2)};
             // Use BIP324 transport when both us and them have NODE_V2_P2P set.
             const bool use_v2transport(addrConnect.nServices & GetLocalServices() & NODE_P2P_V2);
-            OpenNetworkConnection(addrConnect, count_failures, std::move(grant), /*pszDest=*/nullptr, conn_type, use_v2transport);
+            OpenNetworkConnection(addrConnect, count_failures, std::move(grant), /*pszDest=*/nullptr, conn_type, use_v2transport, std::nullopt);
         }
     }
 }
@@ -3022,7 +3022,7 @@ void CConnman::ThreadOpenAddedConnections()
             }
             tried = true;
             CAddress addr(CService(), NODE_NONE);
-            OpenNetworkConnection(addr, false, std::move(grant), info.m_params.m_added_node.c_str(), ConnectionType::MANUAL, info.m_params.m_use_v2transport);
+            OpenNetworkConnection(addr, false, std::move(grant), info.m_params.m_added_node.c_str(), ConnectionType::MANUAL, info.m_params.m_use_v2transport, std::nullopt);
             if (!m_interrupt_net->sleep_for(500ms)) return;
             grant = CountingSemaphoreGrant<>(*semAddnode, /*fTry=*/true);
         }
diff --git a/src/net.h b/src/net.h
index fe8322d1f3..a56f9a20d1 100644
--- a/src/net.h
+++ b/src/net.h
@@ -1191,7 +1191,7 @@ public:
                                const char* pszDest,
                                ConnectionType conn_type,
                                bool use_v2transport,
-                               const std::optional<Proxy>& proxy_override = std::nullopt)
+                               const std::optional<Proxy>& proxy_override)
         EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_unused_i2p_sessions_mutex);
 
     /// Group of private broadcast related members.
diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index 20bb1cb480..06c756fd69 100644
--- a/src/rpc/net.cpp
+++ b/src/rpc/net.cpp
@@ -358,7 +358,7 @@ static RPCMethod addnode()
     if (command == "onetry")
     {
         CAddress addr;
-        connman.OpenNetworkConnection(addr, /*fCountFailure=*/false, /*grant_outbound=*/{}, std::string{node_arg}.c_str(), ConnectionType::MANUAL, use_v2transport);
+        connman.OpenNetworkConnection(addr, /*fCountFailure=*/false, /*grant_outbound=*/{}, std::string{node_arg}.c_str(), ConnectionType::MANUAL, use_v2transport, std::nullopt);
         return UniValue::VNULL;
     }

</details>

Edit: changed diff to use std::nullopt instead of {}

@andrewtoth I'd expect a solid regression test, unless there's compelling reasoning given otherwise. I can work on modifying the last test I posted to make no rpc changes if that simplifies things

Can we get the unit test added here at least?

I am working on a functional test that exercises this via the natural path of a private broadcast to IPv4 address with p2p_v2=1 flag which actually does not support v2 and a subsequent v1 retry. If that works, I think it will trump the other tests. Since it needs the test set up for private broadcast (e.g. addrman filled, automatic connections enabled, SOCKS5 proxy with a destinations factory, etc) that duplicates with parts of p2p_private_broadcast.py. I have extracted those parts into a separate file so both tests can reuse it. Will make it easier to write new private broadcast tests (as opposed to appending new tests to the existent p2p_private_broadcast.py).

Can we remove proxy_override default in OpenNetworkConnection

Ok, will do.

I am working on a functional test that exercises this via the natural path of a private broadcast to IPv4 address with p2p_v2=1 flag which actually does not support v2 and a subsequent v1 retry.

Done in #35410, which contains this PR + a functional test.

I did not append the test here to this PR because I feel that the mood among reviewers might be to merge the fix and debate on the test longer. With two PRs reviewers can choose - review only the fix (here) or review the fix and the test (https://github.com/bitcoin/bitcoin/pull/35410).

There are a bunch of tests for this which fail without the fix and pass with the fix. That should give some confidence to reviewers that the fix indeed works even if they do not thoroughly review the tests themselves:

#35319 (comment) easy unit and functional tests that engage directly with OpenNetworkConnection()

#35319 (comment) I did not try this one from @instagibbs, thanks!

#35410 a functional test that uses private broadcast instead of engaging with OpenNetworkConnection() directly

I did not append the test here to this PR because I feel that the mood among reviewers might be to merge the fix and debate on the test longer.

This is not what i get from reading the thread. All 3 active reviewers explicitly asked you to add a test here. I’m also baffled that you’d expect a fix for a critical bug in a feature we just shipped to be merged without a test confirming that it actually fixes the issue.

I agree. We wouldn't merge a bugfix here without a test, and wouldn't backport that fix into a release branch, with no test. Review seems to have shifted to #35410, indicating that the reviewers here did want the tests, so I guess we can close this, and continue review and discussion in #35410?