rpc: p2p_v2 rpc argument for addnode #23900

dhruv · 2021-12-29T21:26:32Z

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).

DrahtBot · 2021-12-30T11:26:27Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24170 (p2p, rpc: Manual block-relay-only connections with addnode by mzumsande)
#23604 (Use Sock in CNode by vasild)
#23531 (Add Yggdrasil support by prusnak)

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.

ajtowns · 2022-01-10T08:29:55Z

Shouldn't handshaking be independent of advertised services? Otherwise attackers could simply advertise the wrong services for a peer when forwarding along addr messages, or mitm'ing dns seeds? 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.

dhruv · 2022-01-10T19:43:24Z

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:

If 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.

sipa · 2022-01-10T21:24:51Z

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.

sipa · 2022-01-10T21:25:10Z

src/rpc/net.cpp

@@ -300,6 +301,9 @@ static RPCHelpMan addnode()
    if (strCommand == "onetry")
    {
        CAddress addr;
+        if (!request.params[2].isNull()) {
+            addr.nServices = ServiceFlags{static_cast<uint64_t>(request.params[2].get_int64())};


Why only for onetry connections?

I just happened to be using it like that, but, you're right - it should work for the "add" sub-command as well. Fixed.

ajtowns · 2022-01-12T09:30:46Z

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_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.

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)

dhruv · 2022-01-14T05:03:22Z

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.

dhruv · 2022-01-14T05:10:19Z

src/test/fuzz/connman.cpp

@@ -45,7 +45,7 @@ FUZZ_TARGET_INIT(connman, initialize_connman)
                random_string = fuzzed_data_provider.ConsumeRandomLengthString(64);
            },
            [&] {
-                connman.AddNode(random_string);
+                connman.AddNode(random_string, fuzzed_data_provider.ConsumeBool());


This will invalidate existing fuzz seeds (I think).

mzumsande

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.

mzumsande · 2022-01-17T00:32:19Z

src/net.cpp

        if (service.IsValid()) {
            // strAddNode is an IP:port
            auto it = mapConnected.find(service);
            if (it != mapConnected.end()) {
                addedNode.resolvedAddress = service;
                addedNode.fConnected = true;
                addedNode.fInbound = it->second;
+                addedNode.use_p2p_v2 = use_p2p_v2;


This line (+ the one in the else branch) is unnecessary, use_p2p_v2 is already set in the initialization.

Thanks. Done in #24545.

kallewoof · 2022-01-17T03:45:26Z

src/net.h

-    std::vector<std::string> m_added_nodes GUARDED_BY(m_added_nodes_mutex);
+
+    // connection string and whether to use v2 p2p
+    std::vector<std::pair<std::string, bool>> m_added_nodes GUARDED_BY(m_added_nodes_mutex);


Personally think adding a small struct with two vars is superior to this std::pair/tie stuff where you have to look up/remember what the components are.

Done in #24545.

kallewoof · 2022-01-17T03:46:13Z

src/net.h

@@ -92,6 +93,7 @@ struct AddedNodeInfo
    CService resolvedAddress;
    bool fConnected;
    bool fInbound;
+    bool use_p2p_v2;


Nit: Current convention is to prefix with m_ so m_use_p2p_v2.

Done in #24545.

kallewoof · 2022-01-17T03:46:37Z

src/net.cpp

@@ -2977,7 +2988,8 @@ ServiceFlags CConnman::GetLocalServices() const
 unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; }

 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)
-    : m_connected{GetTime<std::chrono::seconds>()},
+    : nServices(addrIn.nServices),
+      m_connected(GetTimeSeconds()),


I'm fine with the change but wondering why it is happening here/now?

IIRC, there was a compiler warning(maybe error actually) about the order in the initializer list needing to be the same as that in the class member declaration.

kallewoof · 2022-01-17T03:48:56Z

src/net.cpp

@@ -2146,7 +2146,7 @@ std::vector<AddedNodeInfo> CConnman::GetAddedNodeInfo() const
 {
    std::vector<AddedNodeInfo> ret;

-    std::list<std::string> lAddresses(0);
+    std::list<std::pair<std::string, bool>> lAddresses(0);


See personal opinion on using structs.

Done in #24545

ajtowns · 2022-01-18T11:38:34Z

Approach ACK, but if it's specific to P2P v2 now, doesn't seem like it needs to be broken out into its own PR prior to P2P v2?

DrahtBot · 2022-02-04T08:59:33Z

🐙 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".}

dhruv · 2022-03-14T15:21:47Z

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.

DrahtBot added P2P RPC/REST/ZMQ labels Dec 29, 2021

This was referenced Dec 30, 2021

Use Sock in CNode #23604

Merged

Add Yggdrasil support #23531

Closed

Make all networking code mockable #21878

Closed

sipa reviewed Jan 10, 2022

View reviewed changes

[rpc] addnode rpc arg to use v2 p2p

609ca6d

dhruv force-pushed the addnode-advertise-services branch from 43dc78e to 609ca6d Compare January 14, 2022 04:58

dhruv changed the title ~~rpc: nServices rpc argument for addnode~~ rpc: p2p_v2 rpc argument for addnode Jan 14, 2022

dhruv commented Jan 14, 2022

View reviewed changes

mzumsande reviewed Jan 17, 2022

View reviewed changes

kallewoof reviewed Jan 17, 2022

View reviewed changes

DrahtBot mentioned this pull request Jan 26, 2022

p2p, rpc: Manual block-relay-only connections with addnode #24170

Closed

DrahtBot added the Needs rebase label Feb 4, 2022

dhruv closed this Mar 14, 2022

dhruv mentioned this pull request Mar 30, 2022

BIP324: Enable v2 P2P encrypted transport #24545

Closed

bitcoin locked and limited conversation to collaborators Mar 30, 2023

rpc: p2p_v2 rpc argument for addnode #23900

rpc: p2p_v2 rpc argument for addnode #23900

Uh oh!

Conversation

dhruv commented Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ajtowns commented Jan 10, 2022

Uh oh!

dhruv commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Jan 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajtowns commented Jan 12, 2022

Uh oh!

dhruv commented Jan 14, 2022

Uh oh!

dhruv Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzumsande left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajtowns commented Jan 18, 2022

Uh oh!

DrahtBot commented Feb 4, 2022

Uh oh!

dhruv commented Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dhruv commented Dec 29, 2021 •

edited

Loading

DrahtBot commented Dec 30, 2021 •

edited

Loading

dhruv commented Jan 10, 2022 •

edited

Loading

dhruv Jan 14, 2022 •

edited

Loading

mzumsande left a comment •

edited

Loading

dhruv commented Mar 14, 2022 •

edited

Loading