net, cli: use v2transport for manual/addrfetch connections, add to -netinfo

mzumsande commented at 10:54 pm on December 11, 2023: contributor

Some preparations before enabling -v2transport as the default:

Use v2 for -connect, -addnode config arg and -seednode if -v2transport is enabled. Our peer may or may not support v2, but I don’t think an extra option is necessary for any of these (we have that for the addnode rpc), because we have the reconnection mechanism that will try again with v1 if our peer doesn’t support v2.
Add a column for the transport protocol to -netinfo. I added it next to the net column because I thought it looked nice there, but if people prefer it somewhere else I’m happy to move it.

Screenshot from 2023-12-11 17-51-22

DrahtBot commented at 10:54 pm on December 11, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stratospher, sipa, theStack, kristapsk, achow101
Concept ACK	naumenkogs

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

mzumsande marked this as a draft on Dec 11, 2023

in src/net.h:1091 in 749cc35b5f outdated

1088+            // Attempt v2 connection if we support v2 - if our peer doesn't support it, we'll reconnect with v1.
1089+            const bool use_v2transport(GetLocalServices() & NODE_P2P_V2);
1090             for (const std::string& added_node : connOptions.m_added_nodes) {
1091                 // -addnode cli arg does not currently have a way to signal BIP324 support
1092-                m_added_node_params.push_back({added_node, false});
1093+                m_added_node_params.push_back({added_node, use_v2transport});

theStack commented at 0:46 am on December 12, 2023:

nit: can the comment above ("… does not currently have a way to signal BIP324 support") now also be removed? It’s content is still true, but with the “always attempt v2 first if we support it”-approach, I guess it’s not relevant anymore.

mzumsande commented at 4:10 pm on December 12, 2023:

thanks, removed the comment.

in src/bitcoin-cli.cpp:554 in cd88bb3e8e outdated

546@@ -545,10 +547,11 @@ class NetinfoRequestHandler : public BaseRequestHandler
547             for (const Peer& peer : m_peers) {
548                 std::string version{ToString(peer.version) + peer.sub_version};
549                 result += strprintf(
550-                    "%3s %6s %5s%7s%7s%5s%5s%5s%5s  %2s %*s%*s%*s%*i %*s %-*s%s\n",
551+                    "%3s %6s %5s %2s%7s%7s%5s%5s%5s%5s  %2s %*s%*s%*s%*i %*s %-*s%s\n",
552                     peer.is_outbound ? "out" : "in",
553                     ConnectionTypeForNetinfo(peer.conn_type),
554                     peer.network,
555+                    peer.transport_protocol_type == "detecting" ? "*" : peer.transport_protocol_type,

theStack commented at 0:50 am on December 12, 2023:

nit: maybe ‘?’ would also work to express the “detecting” state. But no hard feelings, typically users wouldn’t see this a lot anyway.

mzumsande commented at 1:55 am on December 12, 2023:

Heh, I actually changed it from ? to * last thing before opening the PR, because * was already used in a few other places for netinfo and ? would be a new symbol. But maybe ? is more natural here, @jonatack do you have an opinion?

jonatack commented at 8:31 pm on January 6, 2024:

@jonatack do you have an opinion?

Am catching up with merged pulls and discussions; this one is high on my list when I’ve caught up.

jonatack commented at 9:04 pm on January 9, 2024:

@jonatack do you have an opinion?

Would suggest display nothing during peer setup, like for the -netinfo mping and ping columns (* already has a separate meaning in -netinfo, and ? might look like there is a bug)

Also on this line would suggest to avoid checking for the full string “detecting”, and instead do the cheaper check for the most frequent case of the string starting with “v”.

Finally, would propose to drop displaying the “v” prefix in all the rows, as it doesn’t add useful information, and instead use “v” for the column header.

Proposed in #29212.

theStack commented at 1:17 am on December 12, 2023: contributor

Concept ACK

Playing devil’s advocate: could there be any noticeable drawbacks of always trying with v2 first? I’m thinking of destination nodes behind firewalls with deep packet inspection which block ips from further connections if they don’t follow the protocol (i.e. for bitcoin v1 connections, initiating with a version message). If that’s the case, the user would only have the option to disable v2transport in general, or use the addnode RPC instead. Not sure how realistic that “i want to connect via v1 explicitly, as connecting via v2 first causes trouble”-scenario is at all though.

mzumsande marked this as ready for review on Dec 12, 2023

naumenkogs commented at 9:23 am on December 12, 2023: member

Concept ACK

mzumsande force-pushed on Dec 12, 2023

mzumsande commented at 4:15 pm on December 12, 2023: contributor

Playing devil’s advocate: could there be any noticeable drawbacks of always trying with v2 first? I’m thinking of destination nodes behind firewalls with deep packet inspection which block ips from further connections if they don’t follow the protocol (i.e. for bitcoin v1 connections, initiating with a version message). If that’s the case, the user would only have the option to disable v2transport in general, or use the addnode RPC instead. Not sure how realistic that “i want to connect via v1 explicitly, as connecting via v2 first causes trouble”-scenario is at all though.

I don’t know either if this scenario is realistic, and, as you point out, there is the always the workaround of using the addnode rpc. I guess in general it’s less nice to add options to -cli args compared to rpcs: The alternative is that we could allow some construction like -connect=1.2.3.4:8333|v1transport (and similar to -addnode / -seednode) but then we need additional string parsing / documentation for that, and I think we should only do it if there is an actual use case for it.

naumenkogs commented at 8:41 am on December 13, 2023: member

@theStack I think the philosophy behind v2 was always “prevent the firewall from discriminating authenticated connections by making them look exactly like non-authenticated (but also encrypted) connections”. From BIP:

Encryption, even when it is unauthenticated and only used when both endpoints support v2, impedes eavesdropping by forcing the attacker to become active: either by performing a persistent man-in-the-middle (MitM) attack, by downgrading connections to v1, or by spinning up their own nodes and getting honest nodes to make connections to them. Active attacks at scale are more resource intensive in general, but in the case of manual, deliberate connections (as opposed to automatic, random ones), they are also in principle detectable: even very basic checks, e.g., operators manually comparing protocol versions and session IDs (as supported by the proposed protocol), will expose the attacker.

So along that thinking, the situation you describe should trigger the user to be angry at their firewall (unless that becomes a widely used practice due to some wide regulation, which is only a theoretical, and i think unlikely, outcome in the future). Otherwise, if we encourage bending under such firewalls, the v2 battle is kinda lost anyway… @sipa could correct me

in src/net.cpp:2446 in c5d4137028 outdated

2438@@ -2437,6 +2439,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
2439             }
2440             if (!interruptNet.sleep_for(std::chrono::milliseconds(500)))
2441                 return;
2442+            PerformReconnections();

naumenkogs commented at 8:59 am on December 13, 2023:

c5d41370287f48c638ab87d3e02a117b1a17694a

What if the peer is v2, but disconnected us due to other reason? (say a limit of inbounds, or us coming from a blocked network)? Here and in the logic below. In that case we should not attempt reconnection with v1, it won’t help.

This is probably about ShouldReconnectV1 behavior. I can’t find where this case is handled.

mzumsande commented at 3:47 pm on December 13, 2023:

Yes, ShouldReconnectV1 (which is called in DisconnectNode()) will result in the peer only being put on the list of reconnections if it’s in RecvState::KEY. That is impossible if they ever sent us something, in which case PerformReconnections() will do nothing.

naumenkogs commented at 10:11 am on December 18, 2023:

Is it possible that they haven’t sent us anything (the state is KEY), but disconnected us for other reason than not supporting v2? I assume it’s possible if an alternative client implements whatever because this is not prohibited by any bips… But even in the current client, say that IsBanned is triggered? (in that case there is no real use in trying v2).

mzumsande commented at 10:52 pm on December 18, 2023:

Yes, you are right, and I could confirm this with two of my nodes: If we connect via v2 peer and are disconnected because they have banned us, we would try once again with v1 (and then get disconnected again). Both for automatic connections, and (with this PR) manual ones. Although for manual ones it arguably probably matters less because we’ll continuously retry with -connect and -addnode cli args anyway, even when everything is v1. I don’t really see a good way to avoid this, it’s not like they notify us why they have disconnected us, so both situations would look the same for us.

naumenkogs commented at 8:52 am on December 22, 2023:

Okay, so for this PR i’d just suggest clarifying the newly added comments like // Attempt v2 connection if we support v2 - if our peer doesn't support it, we'll reconnect with v1. Right now they are not accurate, as if the banning thing (or some other disconnection reason) don’t exist.

If you feel like it, might as well touch the existing comments as well (say around ShouldReconnectV1) :)

In future, we might communicate the disconnection reason (Ban, v1, eviction, protocol violation, or whatever else), probably through a new p2p message. Right now I’m not 100% sure the benefits are worthy of code complexity :) Better p2p logging (on the disconnected end) may be even better goal than saving up one useless v1/v2 reconnection. Curious what you think about this idea.

mzumsande commented at 10:38 pm on December 27, 2023:

Ok, I clarified the comments added in this PR. I think that a single connection / reconnection attempt doesn’t waste a meaningful amount of resources of either side, so I’m not sure if there is an actual problem here. Also, often one might not want the other side to know that you evicted them because they are banned (after all you banned them for a reason) - that might just help them evade the ban.

jonatack commented at 7:54 pm on January 12, 2024:

a single connection / reconnection attempt doesn’t waste a meaningful amount of resources of either side

Note that every I2P connection requires building 2 tunnels, one for each direction. These are meant to be long-lived connections, as they take network resources and waiting for tunnels to be built for every peer connection adds delay to connection setup time.

mzumsande commented at 3:57 pm on December 13, 2023: contributor

Also worth noting that while this PR deals with manual connections, we absolutely depend on the reconnection mechanism for automatic connections too: The logic that we try with v1 based on service flags is just a courtesy but cannot be relied on, since an attacker could add the v2 service flag to arbitrary v1 nodes in most addrmans by spamming the network with the addr with an added v2 service flag, and then everyone thinks it’s a v2 peer and depends on the reconnection mechanism when connecting to it.

theStack commented at 4:55 pm on December 13, 2023: contributor

I guess in general it’s less nice to add options to -cli args compared to rpcs: The alternative is that we could allow some construction like -connect=1.2.3.4:8333|v1transport (and similar to -addnode / -seednode) but then we need additional string parsing / documentation for that, and I think we should only do it if there is an actual use case for it.

I agree, this should only be done if there is a really good reason for it. @naumenkogs: Note that my concerns were not about the v2 approach in general, and I’m not worried at all about the reconnection mechanism. I was merely wondering whether connecting via v1 directly could still be demanded for some users during the next few releases until v2 is more widespread. In these rare cases (if they exist at all), they could of course always either turn off v2transport completely (probably unpleasant if you only need the direct-v1-connect for a single peer) or use the addnode RPC with the v2transport parameter set to false, so it shouldn’t be a problem at all.

I guess one could argue that it’s a bit inconsistent/confusing to allow explicit v1 connections for the addnode RPC, but not for the -addnode bitcoind parameter. I think they should at least behave identical if no explicit transport version is specified for the node to add, i.e. addnode should IMHO by default try to connect via v2 if -v2transport is set to bitcoind, but the v2transport boolean parameter wasn’t set explicitly. But this is unrelated to this PR, and probably fits better to a future “use v2transport by default” PR anyways.

sipa commented at 9:22 pm on December 20, 2023: member

utACK a30a1d9318d08ddd1007d7fe7c19e4175faa8b64

Conceptually, I believe this makes sense both now and eventually:

Right now, anyone using -v2transport is opting into the BIP324 experiment. If this causes problems for them (e.g. because somehow addnode with default v2 causes issues), they always have the ability to just not do that.
Eventually, having V2 default for addnode/connect makes sense. There is a significant UX advantage to not needing to specify, and BIP324 qualities are most apparent when it’s widely deployed. The downside is essentially doubling the number of connections made for v1 peers and peers that drop connections for unrelated reasons, but I expect manual connection attempts to be relatively rare, and only a fraction of all connection attempts in any case.

Before enabling -v2transport by default (which I’d image may happen after #24748) we could consider a way to force manual connections to be only-v1 or even only-v2 (disabling reconnect-with-v1). A possibility could be through a net permission flag, if #27114 makes it in.

DrahtBot requested review from theStack on Dec 20, 2023

DrahtBot requested review from naumenkogs on Dec 20, 2023

net: attempt v2 transport for manual connections if we support it

This affects manual connections made either with -connect, or with
-addnode provided as a bitcoind config arg (the addnode RPC has an
extra option for v2).

We don't necessarily know if our peer supports v2, but will reconnect
with v1 if they don't. In order to do that, improve the reconnection
behavior such that we will reconnect after a sleep of 500ms
(which usually should be enough for our peer to send us their
version message).

770c0311ef

net: attempt v2 transport for addrfetch connections if we support it 9eed22e870

cli: add transport protcol column to -netinfo fb5bfed26a

mzumsande force-pushed on Dec 27, 2023

mzumsande commented at 10:39 pm on December 27, 2023: contributor

a30a1d9 to fb5bfed: small doc-only update.

stratospher commented at 7:53 am on January 4, 2024: contributor

tested ACK fb5bfed. addrfetch + manual connections aren’t frequent and it would be useful to have this for transition to v2 one day.

used -connect, -addnode, -seednode and observed reconnections happening if other peer was v1 and no reconnection if other peer was v2. an interesting behaviour which i realised made sense was to keep retrying with v2 when latency issue failures happen on tor.

02024-01-04T06:14:58Z [net:debug] trying v2 connection "xyz" lastseen=0.0hrs
12024-01-04T06:14:58Z [net] SOCKS5 connecting "xyz"
22024-01-04T06:14:58Z Socks5() connect to "xyz":8333 failed: general failure
32024-01-04T06:14:59Z [net:debug] trying v2 connection "xyz" lastseen=0.0hrs
42024-01-04T06:14:59Z [net] SOCKS5 connecting "xyz"
52024-01-04T06:14:59Z Socks5() connect to "xyz":8333 failed: general failure
62024-01-04T06:15:00Z [net:debug] trying v2 connection "xyz" lastseen=0.0hrs
72024-01-04T06:15:00Z [net] SOCKS5 connecting "xyz"
82024-01-04T06:15:00Z Socks5() connect to "xyz":8333 failed: general failure

DrahtBot requested review from sipa on Jan 4, 2024

mzumsande commented at 4:13 pm on January 5, 2024: contributor

an interesting behaviour which i realised made sense was to keep retrying with v2 when latency issue failures happen on tor.

Yes! The connection must have been established on the TCP/IP level for a reconnection attempt to happen. It’s the same reason why we don’t try to reconnect with v1 if the peer is just offline.

sipa commented at 1:23 pm on January 7, 2024: member

utACK fb5bfed26a564014b83ccfc96ff00b630930fc61

DrahtBot removed review request from sipa on Jan 7, 2024

theStack approved

theStack commented at 0:16 am on January 8, 2024: contributor

ACK fb5bfed26a564014b83ccfc96ff00b630930fc61

Reviewed the code and verified the intended behavior for manual connections (-connect, -addnode) and addrfetch connections (-seednode) by specifying both a p2p-v2-supporting peer and a p2p-v1 peer in different bitcoind runs. For the v1 peer, the first (v2) connection attempt fails, and a successful v1 reconnect happens after, just as expected.

kristapsk approved

kristapsk commented at 2:00 am on January 8, 2024: contributor

ACK fb5bfed26a564014b83ccfc96ff00b630930fc61

ajtowns commented at 2:32 am on January 8, 2024: contributor

Not an objection, but this feels backwards to me: presumably the user has manually listed those peers as addnode/connect because it’s important that their node be connected to those peers, but if there are any bugs in the “v2 didn’t work, retry as v1” logic, it’s exactly those peers that will be inaccessible. But if we’re confident there aren’t bugs in that logic, why not just apply it to all peers? The “if you don’t like it, just set -v2transport=0” approach to avoiding bugs/risk works just as well in either case. (Doing it for addrfetch otoh seems fine)

mzumsande commented at 2:54 am on January 8, 2024: contributor

why not just apply it to all peers

Do you mean why not apply it to automatic peers as well per default (and ignore the information from the service flag)? I thought the idea was to save one disconnection / reconnection, and the time spent doing these when we probably know better. But as I wrote in #29058 (comment), I think that we depend on the reconnection mechanism for automatic connection as well, because anyone can add a wrong “v2” service flag to a given v1 node in most people’s addrman (but not vice versa!).

achow101 commented at 5:11 pm on January 9, 2024: member

ACK fb5bfed26a564014b83ccfc96ff00b630930fc61

achow101 merged this on Jan 9, 2024

achow101 closed this on Jan 9, 2024

in src/bitcoin-cli.cpp:521 in fb5bfed26a

517@@ -517,10 +518,11 @@ class NetinfoRequestHandler : public BaseRequestHandler
518                 const std::string addr{peer["addr"].get_str()};
519                 const std::string age{conn_time == 0 ? "" : ToString((time_now - conn_time) / 60)};
520                 const std::string sub_version{peer["subver"].get_str()};
521+                const std::string transport{peer["transport_protocol_type"].get_str()};

jonatack commented at 8:59 pm on January 9, 2024:

fb5bfed26a56401 This will cause -netinfo to break when calling it on a node that is running pre-v26 bitcoind, as getpeerinfo doesn’t yet return a “transport_protocol_type” field. It just needs an IsNull() check, as done in the next line and for a few other newer fields added after -netinfo.

Edit: addressed in #29212.

jonatack commented at 9:13 pm on January 9, 2024: contributor

Reviewed https://github.com/bitcoin/bitcoin/commit/fb5bfed26a564014b83ccfc96ff00b630930fc61; WIP review of the other two commits.

achow101 referenced this in commit 4baa162dbb on Jan 11, 2024

in src/net.cpp:2331 in fb5bfed26a

2327+    const bool use_v2transport(GetLocalServices() & NODE_P2P_V2);
2328     CAddress addr;
2329     CSemaphoreGrant grant(*semOutbound, /*fTry=*/true);
2330     if (grant) {
2331-        OpenNetworkConnection(addr, false, std::move(grant), strDest.c_str(), ConnectionType::ADDR_FETCH, /*use_v2transport=*/false);
2332+        OpenNetworkConnection(addr, false, std::move(grant), strDest.c_str(), ConnectionType::ADDR_FETCH, use_v2transport);

jonatack commented at 7:46 pm on January 12, 2024:

9eed22e nit, code unneeded until/unless there is a grant

 0@@ -2316,12 +2316,12 @@ void CConnman::ProcessAddrFetch()
 1         strDest = m_addr_fetches.front();
 2         m_addr_fetches.pop_front();
 3     }
 4-    // Attempt v2 connection if we support v2 - we'll reconnect with v1 if our
 5-    // peer doesn't support it or immediately disconnects us for another reason.
 6-    const bool use_v2transport(GetLocalServices() & NODE_P2P_V2);
 7-    CAddress addr;
 8     CSemaphoreGrant grant(*semOutbound, /*fTry=*/true);
 9     if (grant) {
10+        // Attempt v2 connection if we support v2 - we'll reconnect with v1 if our
11+        // peer doesn't support it or immediately disconnects us for another reason.
12+        const bool use_v2transport(GetLocalServices() & NODE_P2P_V2);
13+        CAddress addr;
14         OpenNetworkConnection(addr, false, std::move(grant), strDest.c_str(), ConnectionType::ADDR_FETCH, use_v2transport);
15     }
16 }

jonatack commented at 7:56 pm on January 12, 2024: contributor

Post-merge review ACK, pending digging a bit deeper into the IRL effects.

achow101 referenced this in commit 8106b268cd on Jan 16, 2024

mzumsande deleted the branch on Jan 17, 2024

achow101 referenced this in commit bef99176e6 on Mar 12, 2024

luke-jr referenced this in commit dbb57829de on Mar 14, 2024

luke-jr referenced this in commit 23e80cc7d9 on Mar 14, 2024

luke-jr referenced this in commit e245d0fde1 on Mar 14, 2024

net, cli: use v2transport for manual/addrfetch connections, add to -netinfo #29058

Code Coverage

Reviews