rpc: add network field to getnodeaddresses #21594

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.

$ bitcoin-cli -signet getnodeaddresses 3
[
  {
    "time": 1611564659,
    "services": 1033,
    "address": "2600:1702:3c30:734f:8f2e:744b:2a51:dfa5",
    "port": 38333,
    "network": "ipv6"
  },
  {
    "time": 1617531931,
    "services": 1033,
    "address": "153.126.143.201",
    "port": 38333,
    "network": "ipv4"
  },
  {
    "time": 1617473058,
    "services": 1033,
    "address": "nsgyo7begau4yecc46ljfecaykyzszcseapxmtu6adrfagfrrzrlngyd.onion",
    "port": 38333,
    "network": "onion"
  }
]

$ bitcoin-cli help getnodeaddresses 
getnodeaddresses ( count )

Return known addresses, which can potentially be used to find new nodes in the network.

Arguments:
1. count    (numeric, optional, default=1) The maximum number of addresses to return. Specify 0 to return all known addresses.

Result:
[                         (json array)
  {                       (json object)
    "time" : xxx,         (numeric) The UNIX epoch time when the node was last seen
    "services" : n,       (numeric) The services offered by the node
    "address" : "str",    (string) The address of the node
    "port" : n,           (numeric) The port number of the node
    "network" : "str"     (string) The network (ipv4, ipv6, onion, i2p) the node connected through
  },
  ...
]

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

DrahtBot added the label RPC/REST/ZMQ on Apr 4, 2021

jarolrod commented at 12: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

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:

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

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

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

sipa commented at 10:20 PM on April 5, 2021: member

utACK 34d84f3dd70d83dd19db1ebd99aa1ed6b4cad03b

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:

        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?

jonatack force-pushed on Apr 6, 2021

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.

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.

theStack approved

theStack commented at 10:50 PM on April 6, 2021: member

Concept and Code-review ACK 79d3e8f8ae9525a6404e56f83ce9351678da1ca3

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.

promag commented at 8:08 AM on April 7, 2021: member

Concept ACK.

nit, could squash 79d3e8f8ae9525a6404e56f83ce9351678da1ca3 in 1st.

rpc: add network field to rpc getnodeaddresses 3bb6e7b655

rpc: simplify/constify getnodeaddresses code 1b9189866a

rpc: improve getnodeaddresses help 5c446784b1

jonatack force-pushed on Apr 7, 2021

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.

jarolrod commented at 1:43 PM on April 7, 2021: member

re-ACK 5c446784b10b168a6f649469a6627ac231eb1de2

[
  {
    "time": 1615790934,
    "services": 1033,
    "address": "49.207.195.108",
    "port": 18333,
    "network": "ipv4"
  },
  {
    "time": 1617277601,
    "services": 1037,
    "address": "2409:4070:4e8e:f61a:69a1:5d52:dc9:6b4e",
    "port": 18333,
    "network": "ipv6"
  }
]

laanwj commented at 1:55 PM on April 7, 2021: member

Hah great idea, I had been thinking about doing this myself. Concept ACK.

promag commented at 2:03 PM on April 7, 2021: member

Code review ACK 5c446784b10b168a6f649469a6627ac231eb1de2.

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

  {
    "time": 1617800381,
    "services": 1032,
    "address": "X.X.X.X",
    "port": 8333,
    "network": "ipv4"
  },
  {
    "time": 1617793195,
    "services": 1037,
    "address": "X:X:X:X:X:X:X:X",
    "port": 8333,
    "network": "ipv6"
  },
  {
    "time": 1617724603,
    "services": 1033,
    "address": "X.b32.i2p",
    "port": 8333,
    "network": "i2p"
  },
  {
    "time": 1615423976,
    "services": 1033,
    "address": "X.onion",
    "port": 8333,
    "network": "onion"
  },

laanwj merged this on Apr 7, 2021

laanwj closed this on Apr 7, 2021

jonatack deleted the branch on Apr 7, 2021

sidhujag referenced this in commit 9694528f97 on Apr 7, 2021

luke-jr referenced this in commit b945073f8c on Jun 27, 2021

random-zebra referenced this in commit 69979ce40b on Aug 12, 2021

deadalnix referenced this in commit 6c0176b53e on Mar 3, 2022

DrahtBot locked this on Aug 16, 2022