rpc: p2p_v2 rpc argument for addnode #23900

pull dhruv wants to merge 1 commits into bitcoin:master from dhruv:addnode-advertise-services changing 7 files +48 −18
  1. dhruv commented at 9:26 pm on December 29, 2021: member

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

  2. DrahtBot added the label P2P on Dec 29, 2021
  3. DrahtBot added the label RPC/REST/ZMQ on Dec 29, 2021
  4. DrahtBot commented at 11:26 am on December 30, 2021: member

    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.

  5. ajtowns commented at 8:29 am on January 10, 2022: member
    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.
  6. dhruv commented at 7:43 pm on January 10, 2022: member

    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. 
    
  7. sipa commented at 9:24 pm on January 10, 2022: member

    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.

  8. in src/rpc/net.cpp:305 in 43dc78e2ff outdated
    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())};
    


    sipa commented at 9:25 pm on January 10, 2022:
    Why only for onetry connections?

    dhruv commented at 4:59 am on January 14, 2022:
    I just happened to be using it like that, but, you’re right - it should work for the “add” sub-command as well. Fixed.
  9. ajtowns commented at 9:30 am on January 12, 2022: member

    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)

  10. [rpc] addnode rpc arg to use v2 p2p 609ca6dd2d
  11. dhruv force-pushed on Jan 14, 2022
  12. dhruv commented at 5:03 am on January 14, 2022: member

    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.

  13. dhruv renamed this:
    rpc: nServices rpc argument for addnode
    rpc: p2p_v2 rpc argument for addnode
    on Jan 14, 2022
  14. in src/test/fuzz/connman.cpp:48 in 609ca6dd2d
    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());
    


    dhruv commented at 5:10 am on January 14, 2022:
    This will invalidate existing fuzz seeds (I think).
  15. in src/net.cpp:2187 in 609ca6dd2d
    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;
    


    mzumsande commented at 0:32 am on January 17, 2022:
    This line (+ the one in the else branch) is unnecessary, use_p2p_v2 is already set in the initialization.

    dhruv commented at 4:10 am on March 30, 2022:
    Thanks. Done in #24545.
  16. mzumsande commented at 1:00 am on January 17, 2022: member

    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.

  17. in src/net.h:1109 in 609ca6dd2d
    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);
    


    kallewoof commented at 3:45 am on January 17, 2022:
    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.

    dhruv commented at 4:11 am on March 30, 2022:
    Done in #24545.
  18. in src/net.h:96 in 609ca6dd2d
    92@@ -92,6 +93,7 @@ struct AddedNodeInfo
    93     CService resolvedAddress;
    94     bool fConnected;
    95     bool fInbound;
    96+    bool use_p2p_v2;
    


    kallewoof commented at 3:46 am on January 17, 2022:
    Nit: Current convention is to prefix with m_ so m_use_p2p_v2.

    dhruv commented at 4:11 am on March 30, 2022:
    Done in #24545.
  19. in src/net.cpp:2992 in 609ca6dd2d
    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()),
    


    kallewoof commented at 3:46 am on January 17, 2022:
    I’m fine with the change but wondering why it is happening here/now?

    dhruv commented at 4:12 am on March 30, 2022:
    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.
  20. in src/net.cpp:2149 in 609ca6dd2d
    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);
    


    kallewoof commented at 3:48 am on January 17, 2022:
    See personal opinion on using structs.

    dhruv commented at 4:12 am on March 30, 2022:
    Done in #24545
  21. ajtowns commented at 11:38 am on January 18, 2022: member
    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?
  22. DrahtBot commented at 8:59 am on February 4, 2022: member

    🐙 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”.

  23. DrahtBot added the label Needs rebase on Feb 4, 2022
  24. dhruv commented at 3:21 pm on March 14, 2022: member

    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.

  25. dhruv closed this on Mar 14, 2022

  26. dhruv added this to the "Closed unmerged" column in a project

  27. DrahtBot locked this on Mar 30, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-19 03:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me