This PR adds the ability for a node operator to signal that a peer added via addnode supports the BIP324 transport protocol (aka v2).
Use case: Working on BIP324, I needed to be able to add a v2 peer and test the handshake. This required allowing the operator to inform the node that the manual connection supports the v2 protocol (on the network, the signaling is done via a service bit, NODE_P2P_V2 in addr relay).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
Shouldn’t handshaking be independent of advertised services?
I might be missing something but don’t we already use data from the advertised services in changing the nature of the connection? For example, (I’m not certain, but expect that), we do not ask pruned nodes for older blocks when in IBD.
I thought the idea for p2p v2 was to attempt an encrypted handshake, and if that failed, retry without encryption, and you’d just do that for all connection attempts.
The current plan is to initiate v2 connections to peers advertising NODE_P2P_V2 and if the handshake fails, try with a v1 handshake before moving on to another node from addrman. While the method you propose would work, attempting v2 connections with v1 nodes that do not advertise v2 support would waste bandwidth in the initial phases of deployment.
Otherwise attackers could simply advertise the wrong services for a peer when forwarding along addr messages, or mitm’ing dns seeds?
The MITM attacker could make a v2 node look like a v1 node which would result in a v1 connection, or a v1 node look like v2 which would result still in a v1 connection. Downgrade attacks are a risk mentioned in BIP324:
0If the responding peer closes the connection after receiving the handshake request, the initiating peer MAY try to connect again with the v1 p2p transport protocol. Such reconnects allow an attacker to "downgrade" the encryption to plaintext communication and thus, accepting v1 connections MUST not be done when the Bitcoin peer-to-peer network has almost entirely embraced v2 communication.
Shouldn’t handshaking be independent of advertised services? Otherwise attackers could simply advertise the wrong services for a peer when forwarding along addr messages
addrman will OR the nServices together when there is conflicting information (and overwrite when getting it from a connection to the node itself), so it should be far easier for an attacker to falsely “upgrade” the rumoured information than to downgrade it. So I think there is relatively little danger in only trying v2 with peers for which we believe they may support it.
300@@ -300,6 +301,9 @@ static RPCHelpMan addnode()
301 if (strCommand == "onetry")
302 {
303 CAddress addr;
304+ if (!request.params[2].isNull()) {
305+ addr.nServices = ServiceFlags{static_cast<uint64_t>(request.params[2].get_int64())};
I might be missing something but don’t we already use data from the advertised services in changing the nature of the connection? For example, (I’m not certain, but expect that), we do not ask pruned nodes for older blocks when in IBD.
We use that to decide not to connect to some peers, but when we do connect to them we treat them the same?
The current plan is to initiate v2 connections to peers advertising
NODE_P2P_V2and if the handshake fails, try with a v1 handshake before moving on to another node from addrman. While the method you propose would work, attempting v2 connections with v1 nodes that do not advertise v2 support would waste bandwidth in the initial phases of deployment.
I guess I’m thinking about it backwards: the whole point of p2p v2 is to have an incompatible handshake compared to current p2p, so if the goal is to pass around some advisory data on who supports the new handshake that helps people avoid wasting bandwidth, then using a service bit makes sense for connecting to random peers.
But if all that’s only for saving bandwidth, why apply it to addnode at all? Just have addnode always try p2pv2 first and fallback – a redundant connection attempt for addnode nodes only shouldn’t be much extra bandwidth?
Just feels like this is something that’s only relevant for p2pv2, not a reason for addnode to know about service bits in general.
(Another alternative would be to have “p2pv2” as an alternative to “onetry” to enable to p2pv2 handshake)
Changed the code to be (a slightly modified version of) the last approach suggested by @ajtowns. Instead of accepting nServices, the addnode rpc now has a boolean p2p_v2 argument. If set to true, the peer is treated as if it were advertising NODE_P2P_V2.
Ready for further review.
44@@ -45,7 +45,7 @@ FUZZ_TARGET_INIT(connman, initialize_connman)
45 random_string = fuzzed_data_provider.ConsumeRandomLengthString(64);
46 },
47 [&] {
48- connman.AddNode(random_string);
49+ connman.AddNode(random_string, fuzzed_data_provider.ConsumeBool());
2184 auto it = mapConnected.find(service);
2185 if (it != mapConnected.end()) {
2186 addedNode.resolvedAddress = service;
2187 addedNode.fConnected = true;
2188 addedNode.fInbound = it->second;
2189+ addedNode.use_p2p_v2 = use_p2p_v2;
use_p2p_v2 is already set in the initialization.
One complication is that addnode is both a bitcoind startup option and a RPC command - so one problem of adding options to the RPC command is that the bitcoind command line option can’t easily mimic them (without introducing some string parsing logic as for -bind or -whitebind), and it could lead to confusion for users if options were added to -addnode that are only configurable via the addnode RPC but not e.g. in bitcoin.conf setups.
I had thought about implementing the suggestion in #23763 with a very similar approach, which runs into the same issue.
1103@@ -1099,7 +1104,10 @@ class CConnman
1104 AddrMan& addrman;
1105 std::deque<std::string> m_addr_fetches GUARDED_BY(m_addr_fetches_mutex);
1106 Mutex m_addr_fetches_mutex;
1107- std::vector<std::string> m_added_nodes GUARDED_BY(m_added_nodes_mutex);
1108+
1109+ // connection string and whether to use v2 p2p
1110+ std::vector<std::pair<std::string, bool>> m_added_nodes GUARDED_BY(m_added_nodes_mutex);
92@@ -92,6 +93,7 @@ struct AddedNodeInfo
93 CService resolvedAddress;
94 bool fConnected;
95 bool fInbound;
96+ bool use_p2p_v2;
m_ so m_use_p2p_v2.
2987@@ -2977,7 +2988,8 @@ ServiceFlags CConnman::GetLocalServices() const
2988 unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; }
2989
2990 CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion)
2991- : m_connected{GetTime<std::chrono::seconds>()},
2992+ : nServices(addrIn.nServices),
2993+ m_connected(GetTimeSeconds()),
2145@@ -2146,7 +2146,7 @@ std::vector<AddedNodeInfo> CConnman::GetAddedNodeInfo() const
2146 {
2147 std::vector<AddedNodeInfo> ret;
2148
2149- std::list<std::string> lAddresses(0);
2150+ std::list<std::pair<std::string, bool>> lAddresses(0);
🐙 This pull request conflicts with the target branch and needs rebase.
Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.
Thanks, @ajtowns @mzumsande @kallewoof. As @ajtowns has pointed out, this is now just specific to BIP324. The code has been rolled into #24545 so let’s continue there. Closing this PR.
To address @mzumsande’s point about RPC vs CLI addnode, I think it is ok for the RPC functionality to be a superset of the CLI so long as the additional RPC functionality is only enabling things that are used for manual testing as is the case here. But of course, curious to hear what other contributors think about this as well.
I will address other comments from here in that code this week.