rpc: add network field to getnodeaddresses #21594

pull jonatack wants to merge 3 commits into bitcoin:master from jonatack:getnodeaddresses-network changing 3 files +15 −13
  1. jonatack commented at 3:14 pm on April 4, 2021: member

    This patch adds a network field to RPC getnodeaddresses, which is useful on its own, particularly with the addition of new networks like I2P and others in the future, and which I also found helpful for adding a new CLI command as a follow-up to this pull that calls getnodeaddresses and needs to know the network of each address.

    While here, also improve the getnodeaddresses code and help.

     0$ bitcoin-cli -signet getnodeaddresses 3
     1[
     2  {
     3    "time": 1611564659,
     4    "services": 1033,
     5    "address": "2600:1702:3c30:734f:8f2e:744b:2a51:dfa5",
     6    "port": 38333,
     7    "network": "ipv6"
     8  },
     9  {
    10    "time": 1617531931,
    11    "services": 1033,
    12    "address": "153.126.143.201",
    13    "port": 38333,
    14    "network": "ipv4"
    15  },
    16  {
    17    "time": 1617473058,
    18    "services": 1033,
    19    "address": "nsgyo7begau4yecc46ljfecaykyzszcseapxmtu6adrfagfrrzrlngyd.onion",
    20    "port": 38333,
    21    "network": "onion"
    22  }
    23]
    24
    25$ bitcoin-cli help getnodeaddresses 
    26getnodeaddresses ( count )
    27
    28Return known addresses, which can potentially be used to find new nodes in the network.
    29
    30Arguments:
    311. count    (numeric, optional, default=1) The maximum number of addresses to return. Specify 0 to return all known addresses.
    32
    33Result:
    34[                         (json array)
    35  {                       (json object)
    36    "time" : xxx,         (numeric) The UNIX epoch time when the node was last seen
    37    "services" : n,       (numeric) The services offered by the node
    38    "address" : "str",    (string) The address of the node
    39    "port" : n,           (numeric) The port number of the node
    40    "network" : "str"     (string) The network (ipv4, ipv6, onion, i2p) the node connected through
    41  },
    42  ...
    43]
    

    Future idea: allow passing getnodeaddresses a network (or networks) as an argument to return only addresses in that network.

  2. DrahtBot added the label RPC/REST/ZMQ on Apr 4, 2021
  3. jarolrod commented at 0:01 am on April 5, 2021: member

    ACK 34d84f3dd70d83dd19db1ebd99aa1ed6b4cad03b

    Straightforward change that adds a useful network field and makes welcome improvements to the getnodeaddresses code.

    Tested in the GUI Console window: Screen Shot 2021-04-04 at 7 59 30 PM

  4. in src/rpc/net.cpp:879 in 34d84f3dd7 outdated
    884-        obj.pushKV("services", (uint64_t)addr.nServices);
    885+        obj.pushKV("time", static_cast<int>(addr.nTime));
    886+        obj.pushKV("services", static_cast<uint64_t>(addr.nServices));
    887         obj.pushKV("address", addr.ToStringIP());
    888         obj.pushKV("port", addr.GetPort());
    889+        obj.pushKV("network", GetNetworkName(addr.GetNetClass()));
    


    kiminuo commented at 11:20 am on April 5, 2021:
    Just out of curiosity: Is ipv6 returned for both NET_CJDNS and NET_IPV6?

    jonatack commented at 11:47 am on April 5, 2021:
    It shouldn’t. This essentially returns the stringified CNetAddr::m_net. See CNetAddr::SetNetFromBIP155Network() in netaddress.cpp. (We also don’t yet support CJDNS.)

    kiminuo commented at 11:52 am on April 5, 2021:

    Ah, so that’s the reason it is not mentioned in the help in your OP. Thanks.

    “network” : “str” (string) The network (ipv4, ipv6, onion, i2p) the node connected through


    jonatack commented at 3:29 pm on April 5, 2021:

    Yes, GetNetworkNames() doesn’t return cjdns because of this line:

    0if (network == NET_UNROUTABLE || network == NET_CJDNS || network == NET_INTERNAL) continue;
    

    When cjdns support is added, that line will need to be updated.

  5. in src/rpc/net.cpp:875 in 65b2578f4f outdated
    871@@ -872,8 +872,8 @@ static RPCHelpMan getnodeaddresses()
    872 
    873     for (const CAddress& addr : vAddr) {
    874         UniValue obj(UniValue::VOBJ);
    875-        obj.pushKV("time", (int)addr.nTime);
    876-        obj.pushKV("services", (uint64_t)addr.nServices);
    877+        obj.pushKV("time", static_cast<int>(addr.nTime));
    


    sipa commented at 10:18 pm on April 5, 2021:
    Hmm, I think we generally use C-style casts for primitive types (as they are syntactically simpler, and for those types they’re all identical in semantics anyway). No problem using these in new code you write yourself of course, but mostly a matter of personal taste.

    MarcoFalke commented at 6:40 am on April 6, 2021:
    Ideally there’d be no casts here, but I suspect that will require some changes in univalue

    jonatack commented at 7:40 am on April 6, 2021:

    Dropped the casts commit. Definitely an optional change.

    (https://github.com/bitcoin/bitcoin/pull/20965#issuecomment-770375623 summarizes why I tended to use named casts even for primitives.)

  6. sipa commented at 10:20 pm on April 5, 2021: member
    utACK 34d84f3dd70d83dd19db1ebd99aa1ed6b4cad03b
  7. in src/rpc/net.cpp:876 in 34d84f3dd7 outdated
    881     for (const CAddress& addr : vAddr) {
    882         UniValue obj(UniValue::VOBJ);
    883-        obj.pushKV("time", (int)addr.nTime);
    884-        obj.pushKV("services", (uint64_t)addr.nServices);
    885+        obj.pushKV("time", static_cast<int>(addr.nTime));
    886+        obj.pushKV("services", static_cast<uint64_t>(addr.nServices));
    


    MarcoFalke commented at 6:41 am on April 6, 2021:
    0        obj.pushKV("services", uint64_t{addr.nServices});
    

    nit: I keep forgetting if this works for service flags, but C++11 casts are preferred over static_casts ;)

    (Obviously don’t force push unless you have to for other reasons)


    jonatack commented at 7:31 am on April 6, 2021:

    That does work.

    I wonder why we don’t print the same services output(s) as for getpeerinfo and getnetworkinfo?

  8. jonatack force-pushed on Apr 6, 2021
  9. jonatack commented at 7:46 am on April 6, 2021: member
    Thanks everyone. Dropped the named casts change and added a release note into the first commit.
  10. DrahtBot commented at 12:49 pm on April 6, 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:

    • #20550 (RPC: Tolerate invalid rpcauth options by luke-jr)

    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.

  11. theStack approved
  12. theStack commented at 10:50 pm on April 6, 2021: member
    Concept and Code-review ACK 79d3e8f8ae9525a6404e56f83ce9351678da1ca3
  13. in src/rpc/net.cpp:866 in 1f24a27a4c outdated
    868-        count = request.params[0].get_int();
    869-        if (count < 0) {
    870-            throw JSONRPCError(RPC_INVALID_PARAMETER, "Address count out of range");
    871-        }
    872-    }
    873+    const int count{request.params[0].isNull() ? 1 : request.params[0].get_int()};
    


    promag commented at 8:03 am on April 7, 2021:

    1f24a27a4c80f182b275a75afdb4777ec792fd91

    FWIW #20017 makes this more simple.


    jonatack commented at 10:51 am on April 7, 2021:
    TIL about that pull, thanks! (One line as done here is pretty simple though?) Resolving as not actionable ATM in this patch.
  14. promag commented at 8:08 am on April 7, 2021: member

    Concept ACK.

    nit, could squash 79d3e8f8ae9525a6404e56f83ce9351678da1ca3 in 1st.

  15. rpc: add network field to rpc getnodeaddresses 3bb6e7b655
  16. rpc: simplify/constify getnodeaddresses code 1b9189866a
  17. rpc: improve getnodeaddresses help 5c446784b1
  18. jonatack force-pushed on Apr 7, 2021
  19. jonatack commented at 11:03 am on April 7, 2021: member

    Concept ACK.

    nit, could squash 79d3e8f in 1st.

    Done, squashed.

    With ACKs by @jarolrod, @sipa and @theStack, this is hopefully ready for final ACKs or merge.

  20. jarolrod commented at 1:43 pm on April 7, 2021: member

    re-ACK 5c446784b10b168a6f649469a6627ac231eb1de2

     0[
     1  {
     2    "time": 1615790934,
     3    "services": 1033,
     4    "address": "49.207.195.108",
     5    "port": 18333,
     6    "network": "ipv4"
     7  },
     8  {
     9    "time": 1617277601,
    10    "services": 1037,
    11    "address": "2409:4070:4e8e:f61a:69a1:5d52:dc9:6b4e",
    12    "port": 18333,
    13    "network": "ipv6"
    14  }
    15]
    
  21. laanwj commented at 1:55 pm on April 7, 2021: member
    Hah great idea, I had been thinking about doing this myself. Concept ACK.
  22. promag commented at 2:03 pm on April 7, 2021: member
    Code review ACK 5c446784b10b168a6f649469a6627ac231eb1de2.
  23. laanwj commented at 4:54 pm on April 7, 2021: member

    Tested ACK 5c446784b10b168a6f649469a6627ac231eb1de2 All supported address types are reflected correctly. I’ve checked that TorV2 and V3 appear as “onion”.

     0  {
     1    "time": 1617800381,
     2    "services": 1032,
     3    "address": "X.X.X.X",
     4    "port": 8333,
     5    "network": "ipv4"
     6  },
     7  {
     8    "time": 1617793195,
     9    "services": 1037,
    10    "address": "X:X:X:X:X:X:X:X",
    11    "port": 8333,
    12    "network": "ipv6"
    13  },
    14  {
    15    "time": 1617724603,
    16    "services": 1033,
    17    "address": "X.b32.i2p",
    18    "port": 8333,
    19    "network": "i2p"
    20  },
    21  {
    22    "time": 1615423976,
    23    "services": 1033,
    24    "address": "X.onion",
    25    "port": 8333,
    26    "network": "onion"
    27  },
    
  24. laanwj merged this on Apr 7, 2021
  25. laanwj closed this on Apr 7, 2021

  26. jonatack deleted the branch on Apr 7, 2021
  27. sidhujag referenced this in commit 9694528f97 on Apr 7, 2021
  28. luke-jr referenced this in commit b945073f8c on Jun 27, 2021
  29. random-zebra referenced this in commit 69979ce40b on Aug 12, 2021
  30. deadalnix referenced this in commit 6c0176b53e on Mar 3, 2022
  31. DrahtBot locked this on Aug 16, 2022

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-10-04 22:12 UTC

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