rpc: add network field to getnodeaddresses #21594

jonatack · 2021-04-04T15:14:44Z

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.

jarolrod

ACK 34d84f3dd70d83dd19db1ebd99aa1ed6b4cad03b

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

Tested in the GUI Console window:

src/rpc/net.cpp

sipa · 2021-04-05T22:20:11Z

utACK 34d84f3dd70d83dd19db1ebd99aa1ed6b4cad03b

maflcko · 2021-04-06T06:41:40Z

src/rpc/net.cpp

Suggested change

obj.pushKV("services", static_cast<uint64_t>(addr.nServices));

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)

That does work.

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

jonatack · 2021-04-06T07:46:01Z

Thanks everyone. Dropped the named casts change and added a release note into the first commit.

DrahtBot · 2021-04-06T12:49:55Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

RPC: Tolerate invalid rpcauth options #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

Concept and Code-review ACK 79d3e8f8ae9525a6404e56f83ce9351678da1ca3

promag

Concept ACK.

nit, could squash 79d3e8f8ae9525a6404e56f83ce9351678da1ca3 in 1st.

src/rpc/net.cpp

jonatack · 2021-04-07T11:03:06Z

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

re-ACK 5c44678

[
  {
    "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 · 2021-04-07T13:55:01Z

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

promag

Code review ACK 5c44678.

laanwj · 2021-04-07T16:54:19Z

Tested ACK 5c44678
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"
  },

Github-Pull: bitcoin#21594 Rebased-From: 3bb6e7b

e9d28da [Doc] Document getnodeaddresses in release notes (Fuzzbawls) ebe2a46 test: improve getnodeaddresses coverage, test by network (Fuzzbawls) 91ac5c7 rpc: enable filtering getnodeaddresses by network (Fuzzbawls) badfc49 p2p: allow CConnman::GetAddresses() by network, add doxygen (Fuzzbawls) fa78233 p2p: allow CAddrMan::GetAddr() by network, add doxygen (Fuzzbawls) a4d2733 p2p: pull time call out of loop in CAddrMan::GetAddr_() (Fuzzbawls) d5c1250 p2p: enable CAddrMan::GetAddr_() by network, add doxygen (Fuzzbawls) 67e2c2f [RPC] add network field to getnodeaddresses (Fuzzbawls) b4832b2 [Net] Add addpeeraddress RPC method (Fuzzbawls) 1a31b67 [test] Test that getnodeaddresses() can return all known addresses (Fuzzbawls) e83fd0e [Addrman] Specify max addresses and pct when calling GetAddresses() (Fuzzbawls) 792655d [RPC] Add getnodeaddresses RPC command (chris-belcher) Pull request description: Implement a new RPC command (`getnodeaddresses`) that provides RPC access to the node's addrman. It may be useful for PIVX wallets to broadcast their transactions over tor for improved privacy without using the centralized DNS seeds. I've included upstream improvements to this feature here as well, and this RPC command was used to scrape the Tor v3 hardcoded seed nodes added in #2502 --------- Based on the following upstream PRs: * bitcoin#13152 * bitcoin#19658 * bitcoin#21594 * bitcoin#21843 ACKs for top commit: furszy: all good, tested ACK e9d28da. random-zebra: utACK e9d28da and merging... Tree-SHA512: 2e50108504ac8e172c2e31eb64813d6aae6b6819cf197eace2e4e15686906708b2f82262909faad00ce64af0f61945089a36b857064d3ce2398b3acb14e4ad7d

Summary: Add network field to rpc `getnodeaddresses`. Simplify/constify code. Improve help. This is a backport of [[bitcoin/bitcoin#21594 | core#21594]] Depends on D11125 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11126

jonatack mentioned this pull request Apr 4, 2021

cli: create -addrinfo #21595

Merged

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

jarolrod reviewed Apr 5, 2021

View reviewed changes

kiminuo reviewed Apr 5, 2021

View reviewed changes

src/rpc/net.cpp Outdated Show resolved Hide resolved

sipa reviewed Apr 5, 2021

View reviewed changes

src/rpc/net.cpp Outdated Show resolved Hide resolved

maflcko reviewed Apr 6, 2021

View reviewed changes

jonatack force-pushed the getnodeaddresses-network branch from 34d84f3 to 79d3e8f Compare April 6, 2021 07:45

DrahtBot mentioned this pull request Apr 6, 2021

RPC: Tolerate invalid rpcauth options #20550

Closed

theStack approved these changes Apr 6, 2021

View reviewed changes

promag reviewed Apr 7, 2021

View reviewed changes

src/rpc/net.cpp Outdated Show resolved Hide resolved

jonatack added 3 commits April 7, 2021 12:57

rpc: add network field to rpc getnodeaddresses

3bb6e7b

rpc: simplify/constify getnodeaddresses code

1b91898

rpc: improve getnodeaddresses help

5c44678

jonatack force-pushed the getnodeaddresses-network branch from 79d3e8f to 5c44678 Compare April 7, 2021 11:02

jarolrod reviewed Apr 7, 2021

View reviewed changes

promag reviewed Apr 7, 2021

View reviewed changes

laanwj merged commit cb79cab into bitcoin:master Apr 7, 2021

jonatack deleted the getnodeaddresses-network branch April 7, 2021 17:02

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 7, 2021

Merge bitcoin#21594: rpc: add network field to getnodeaddresses

9694528

jonatack mentioned this pull request May 3, 2021

net: add I2P hardcoded seeds #21825

Merged

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021

rpc: add network field to rpc getnodeaddresses

b945073

Github-Pull: bitcoin#21594 Rebased-From: 3bb6e7b

Fuzzbawls mentioned this pull request Aug 12, 2021

[RPC][Net] Add getnodeaddresses RPC command PIVX-Project/PIVX#2503

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

	obj.pushKV("services", static_cast<uint64_t>(addr.nServices));
	obj.pushKV("services", uint64_t{addr.nServices});

rpc: add network field to getnodeaddresses #21594

rpc: add network field to getnodeaddresses #21594

Uh oh!

Conversation

jonatack commented Apr 4, 2021

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sipa commented Apr 5, 2021

Uh oh!

maflcko Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

jonatack Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

jonatack commented Apr 6, 2021

Uh oh!

DrahtBot commented Apr 6, 2021

Conflicts

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented Apr 7, 2021

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Apr 7, 2021

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Apr 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants