[rpc] Add getnodeaddresses RPC command #13152

chris-belcher · 2018-05-02T12:43:37Z

Implements issue #9463

New getnodeaddresses call gives access via RPC to the peers known by the node. It may be useful for bitcoin wallets to broadcast their transactions over tor for improved privacy without using the centralized DNS seeds. getnodeaddresses is very similar to the getaddr p2p method.

Please advise me on the best approach for writing an automated test. By my reading the getaddr p2p method also isn't really tested.

laanwj · 2018-05-02T13:04:43Z

Concept ACK, this would indeed be useful.

promag

Why not extend getpeerinfo?

promag · 2018-05-02T13:08:29Z

src/rpc/net.cpp

+    UniValue ret(UniValue::VARR);
+
+    int address_return_count = std::min(count, (int)vAddr.size());
+    for(int i = 0; i < address_return_count; ++i) {


Nit, space after for.

promag · 2018-05-02T13:10:50Z

src/rpc/net.cpp

+            "getaddress ( count )\n"
+            "\nReturn known addresses which can potentially be used to find new nodes in the network\n"
+            "\nArguments:\n"
+            "1. \"count\"    (numeric, optional) How many addresses to return, if available (default = 1)\n"


Why not return all?

Returning all would typically result in a massive json array with thousands of entries, about 2500 or so depending. Normally applications only need to connect to a few nodes, they don't need anywhere near that many.

promag · 2018-05-02T13:14:45Z

Please advise me on the best approach for writing an automated test

At least it can test arguments (valid and invalid) and result format and fields (their presence and types).

laanwj · 2018-05-02T13:23:14Z

I think this call should be called differently. getaddress is too easy to confuse with a bitcoin address (e.g. getnewaddress). Maybe getnodeaddresses?

Why not extend getpeerinfo?

You misunderstand what this does. It doesn't return currently conneced nodes, but potentially connectable addresses.

chris-belcher · 2018-05-02T14:16:54Z

Renamed to getnodeaddresses and fixed nits.

Looks like something went wrong with the rebase and now other people's commits are involved too. Does anyone know how I can fix this. (edit: fixed)

At least it can test arguments (valid and invalid) and result format and fields (their presence and types).

I think in the test suite the node won't know about any other addresses, so this RPC will return an empty json object, so it can't be tested. What would be the best way around this? Maybe to populate the vAddr somehow.

chris-belcher · 2018-05-02T15:32:49Z

Fixed git weirdness (thanks to viasil).

promag · 2018-05-02T16:02:54Z

Please rename PR.

MasterGrad17 · 2018-05-02T20:32:11Z

Does getnodeaddresses command return a list of reachable nodes in the network known by a node? like the result of getaddr message? is getaddress/getnodeaddresses RPC command currently available in v0.16.0rc4?

chris-belcher · 2018-05-02T20:37:13Z

@MasterGrad17 Yes the result is very similar as from the p2p getaddr method. The addresses come from gossiping between nodes, so the IP address may be reachable but that isn't guaranteed.

sipa · 2018-05-02T21:29:32Z

@MasterGrad17 You're commenting on the request to get it merged into Bitcoin Core, so obviously no.

jonasschnelli · 2018-05-03T06:58:03Z

src/rpc/net.cpp

+            "    \"time\": ttt,                        (numeric) Address timestamp in seconds since epoch (Jan 1 1970 GMT)\n"
+            "    \"services\": n,                      (numeric) The services offered\n"
+            "    \"servicesHex\": \"xxxxxxxxxxxxxxxx\",  (string) The hex string of services offered\n"
+            "    \"addr\": \"host\",                     (string) The IP address of the peer\n"


nit: could also be a tor address

jonasschnelli · 2018-05-03T07:01:30Z

src/rpc/net.cpp

+        CAddress addr = vAddr[i];
+        obj.pushKV("time", (int)addr.nTime);
+        obj.pushKV("services", addr.nServices);
+        obj.pushKV("servicesHex", strprintf("%016x", addr.nServices));


would be handy to have a string representation here. Like ("NODE_NETWORK | NODE_NETWORK_LIMITED | NODE_BLOOM", etc.). But can be done later.

jonasschnelli

Almost utACK c83ccf7.
Please rename command.
Maybe listpeeraddresses()

sipa · 2018-05-03T07:09:52Z

@jonasschnelli listpeeraddresses sounds like it's somehow exhaustive, or at least a complete specific subset of addresses. It's just giving a bunch of random ones; getpeeraddresses sounds clearer to me. Am i missing something?

jonasschnelli · 2018-05-03T07:16:34Z

listpeeraddresses sounds like it's somehow exhaustive, or at least a complete specific subset of addresses. It's just giving a bunch of random ones; getpeeraddresses sounds clearer to me. Am I missing something?

Good point.

chris-belcher · 2018-05-03T09:20:58Z

peer implies it returns addresses of peers you're actually connected to. Like how getpeerinfo gives information only about connected peers. How about getpossiblepeeraddresses? It's a bit of a mouthful admittedly. Other options could be getgossipedpeeraddresses or getpotentialpeeraddresses.

laanwj · 2018-05-03T09:50:54Z

Yes, I intentionally avoided 'peer' in my suggested name for that reason, as it's easy to confuse and a lot of people confused it already. Also let's stop asking the guy to rename his RPC call :)
I think getnodeaddresses is fine, just make sure that the documentation explains the functionality.

chris-belcher · 2018-05-03T13:03:26Z

Fixed nit

jnewbery

A few nits inline.

You should be able to test this in the functional test framework as follows:

start a node
add a P2PConnection to the node
send a msg_addr to the node with some addresses
call the new getnodeaddresses RPC
assert that the node addresses returned are the same as those sent in the p2p ADDR message.

Take a look at example_test.py for some pointers on how to write test cases. I think it makes sense to add this new test case to the rpc_net.py script.

Feel free to reach me on irc (jnewbery) if you need any pointers.

jnewbery · 2018-05-03T17:50:32Z

src/rpc/net.cpp

+            "getnodeaddresses ( count )\n"
+            "\nReturn known addresses which can potentially be used to find new nodes in the network\n"
+            "\nArguments:\n"
+            "1. \"count\"    (numeric, optional) How many addresses to return, if available (default = 1)\n"


Should document that the maximum number of nodes to return is:

2500 (due to ADDRMAN_GETADDR_MAX in CAddrMan::GetAddr_()); or

23% of all known nodes.
(whichever is smaller)

jnewbery · 2018-05-03T17:54:38Z

src/rpc/net.cpp

+            "\nResult:\n"
+            "[\n"
+            "  {\n"
+            "    \"time\": ttt,                        (numeric) Address timestamp in seconds since epoch (Jan 1 1970 GMT)\n"


What does 'address timestamp' mean here?

jnewbery · 2018-05-03T17:57:44Z

src/rpc/net.cpp

+            "  {\n"
+            "    \"time\": ttt,                        (numeric) Address timestamp in seconds since epoch (Jan 1 1970 GMT)\n"
+            "    \"services\": n,                      (numeric) The services offered\n"
+            "    \"servicesHex\": \"xxxxxxxxxxxxxxxx\",  (string) The hex string of services offered\n"


return fields should be named in camel_case

jnewbery · 2018-05-03T17:58:00Z

src/rpc/net.cpp

+            "    \"time\": ttt,                        (numeric) Address timestamp in seconds since epoch (Jan 1 1970 GMT)\n"
+            "    \"services\": n,                      (numeric) The services offered\n"
+            "    \"servicesHex\": \"xxxxxxxxxxxxxxxx\",  (string) The hex string of services offered\n"
+            "    \"addr\": \"host\",                     (string) The address of the peer\n"


prefer to name this address. No need to save characters!

jnewbery · 2018-05-03T18:46:29Z

src/rpc/net.cpp

+            "[\n"
+            "  {\n"
+            "    \"time\": ttt,                        (numeric) Address timestamp in seconds since epoch (Jan 1 1970 GMT)\n"
+            "    \"services\": n,                      (numeric) The services offered\n"


I don't think this is useful. Services is a bitfield. Displaying it as an int doesn't give any useful information to the user. Just display the hex version.

Depends on what 'the user' is. Programmatic RPC users will want the int, not have to do an additional step of parsing a hex string (though I don't care deeply in this case, just be mindful that manual command-line users are not the only clients of the RPC interface).

be mindful that manual command-line users are not the only clients of the RPC interface

Yes good point. In general though, I think we should avoid RPCs returning the same data in multiple formats.

chris-belcher · 2018-05-21T16:27:43Z

Fixed nits and created a test in rpc_net.py.

jnewbery

New test is great. Thanks for adding it!

A few nits inline. Feel free to take or leave them.

jnewbery · 2018-05-21T17:01:08Z

src/rpc/net.cpp

@@ -624,6 +624,55 @@ static UniValue setnetworkactive(const JSONRPCRequest& request)
    return g_connman->GetNetworkActive();
 }

+UniValue getnodeaddresses(const JSONRPCRequest& request)


nit: should be static

jnewbery · 2018-05-21T17:13:36Z

test/functional/rpc_net.py

@@ -16,6 +17,8 @@
    p2p_port,
    wait_until,
 )
+from test_framework.mininode import P2PInterface, network_thread_start


nit: sort imports (in PEP-8 ordering)

jnewbery · 2018-05-21T17:27:27Z

test/functional/rpc_net.py

+        for a in imported_addrs:
+            addr = CAddress()
+            addr.time = now
+            addr.services = NODE_NETWORK | NODE_WITNESS


Should be nServices

jnewbery · 2018-05-21T17:30:49Z

test/functional/rpc_net.py

+        self.nodes[0].p2p.wait_for_verack()
+
+        # send some addresses to the node via the p2p message addr
+        imported_addrs = set(["123.123." + str(i//255) + "." + str(i%255) for i in range(1000)])


nit: move this into the range loop below and use string formatters rather than concatenation:

# send some addresses to the node via the p2p message addr now = int(time.time()) msg = msg_addr() imported_addrs = [] for i in range(1000): a = "123.123.{}.{}".format(i // 255, i % 256) imported_addrs.append(a) addr = CAddress() ...

jnewbery · 2018-05-21T17:32:50Z

test/functional/rpc_net.py

+        NODE_ADDRESSES_REQUEST_COUNT = 10
+        node_addresses = self.nodes[0].getnodeaddresses(NODE_ADDRESSES_REQUEST_COUNT)
+        assert len(node_addresses) == NODE_ADDRESSES_REQUEST_COUNT
+        assert set([addr["address"] for addr in node_addresses]).issubset(imported_addrs)


nit: drop the set comparisons, loop over the return items and test all fields, eg:

for a in node_addresses: assert a["address"] in imported_addrs assert_equal(a["services"], NODE_NETWORK | NODE_WITNESS) assert_equal(a["time"], now) assert_equal(a["port"], 8333)

test/functional/rpc_net.py

src/rpc/net.cpp

promag · 2018-09-13T19:01:42Z

Looks good to me, utACK 8b96ebe.

jnewbery

Sorry to nit at this late stage. Thanks so much for sticking with this. I know it's taken a long time!

One tiny code-style change and I think this is ready for merge.

jnewbery · 2018-09-13T20:57:39Z

src/rpc/net.cpp

+    if (!request.params[0].isNull()) {
+        count = request.params[0].get_int();
+        if (count <= 0)
+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Address count out of range");


I'm really sorry to code-style nit when this PR has been through so much review, but all then clauses should be in braces or on the same line (https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c)

promag · 2018-09-13T21:14:34Z

test/functional/rpc_net.py

+        assert_equal(len(node_addresses), REQUEST_COUNT)
+        for a in node_addresses:
+            # the timestamps are usually offset by a few hours, so only check existence
+            assert "time" in a


nit, can be removed — since @jnewbery nit'ed

New getnodeaddresses call gives access via RPC to the peers known by the node. It may be useful for bitcoin wallets to broadcast their transactions over tor for improved privacy without using the centralized DNS seeds. getnodeaddresses is very similar to the getaddr p2p method. Tests the new rpc call by feeding IP address to a test node via the p2p protocol, then obtaining someone of those addresses with getnodeaddresses and checking that they are a subset.

chris-belcher · 2018-09-18T08:57:38Z

Fixed those nits

ajtowns · 2018-09-18T13:28:01Z

utACK a2eb6f5

maflcko · 2018-09-18T21:25:26Z

utACK a2eb6f5

promag · 2018-09-18T21:30:56Z

utACK a2eb6f5. Please fix PR description: "New getaddress call gives ..."

a2eb6f5 [rpc] Add getnodeaddresses RPC command (chris-belcher) Pull request description: Implements issue bitcoin#9463 New getnodeaddresses call gives access via RPC to the peers known by the node. It may be useful for bitcoin wallets to broadcast their transactions over tor for improved privacy without using the centralized DNS seeds. getnodeaddresses is very similar to the getaddr p2p method. Please advise me on the best approach for writing an automated test. By my reading the getaddr p2p method also isn't really tested. Tree-SHA512: ad03abf518847476495b76a2f5394b8030aa86654429167fa618e21460abb505c10ef9817ec1b80472320d41d0aff5dc94a8efce023aaaaf5e81386aa92b852b

a2eb6f5 [rpc] Add getnodeaddresses RPC command (chris-belcher) Pull request description: Implements issue bitcoin#9463 New getnodeaddresses call gives access via RPC to the peers known by the node. It may be useful for bitcoin wallets to broadcast their transactions over tor for improved privacy without using the centralized DNS seeds. getnodeaddresses is very similar to the getaddr p2p method. Please advise me on the best approach for writing an automated test. By my reading the getaddr p2p method also isn't really tested. Tree-SHA512: ad03abf518847476495b76a2f5394b8030aa86654429167fa618e21460abb505c10ef9817ec1b80472320d41d0aff5dc94a8efce023aaaaf5e81386aa92b852b # Conflicts: # src/rpc/client.cpp # src/rpc/net.cpp

a2eb6f5 [rpc] Add getnodeaddresses RPC command (chris-belcher) Pull request description: Implements issue bitcoin#9463 New getnodeaddresses call gives access via RPC to the peers known by the node. It may be useful for bitcoin wallets to broadcast their transactions over tor for improved privacy without using the centralized DNS seeds. getnodeaddresses is very similar to the getaddr p2p method. Please advise me on the best approach for writing an automated test. By my reading the getaddr p2p method also isn't really tested. Tree-SHA512: ad03abf518847476495b76a2f5394b8030aa86654429167fa618e21460abb505c10ef9817ec1b80472320d41d0aff5dc94a8efce023aaaaf5e81386aa92b852b

This reverts commit 54e38eca Signed-off-by: pasta <pasta@dashboost.org>

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

fanquake added the RPC/REST/ZMQ label May 2, 2018

promag reviewed May 2, 2018

View reviewed changes

chris-belcher force-pushed the 2018-04-rpc-getaddress branch from a5e0fc8 to 892daca Compare May 2, 2018 14:11

chris-belcher force-pushed the 2018-04-rpc-getaddress branch 2 times, most recently from a28d1cf to c83ccf7 Compare May 2, 2018 15:30

chris-belcher changed the title ~~[WIP] [rpc] Add getaddress RPC command~~ [WIP] [rpc] Add getnodeaddresses RPC command May 2, 2018

jonasschnelli reviewed May 3, 2018

View reviewed changes

jonasschnelli suggested changes May 3, 2018

View reviewed changes

chris-belcher force-pushed the 2018-04-rpc-getaddress branch 2 times, most recently from 73f046d to 9e6cac2 Compare May 3, 2018 12:51

jnewbery reviewed May 3, 2018

View reviewed changes

chris-belcher force-pushed the 2018-04-rpc-getaddress branch from 9e6cac2 to f0aca19 Compare May 10, 2018 18:39

chris-belcher force-pushed the 2018-04-rpc-getaddress branch from f0aca19 to dd7e1e6 Compare May 21, 2018 15:31

jnewbery reviewed May 21, 2018

View reviewed changes

chris-belcher mentioned this pull request Sep 9, 2018

Broadcasting transactions with Electrum and EPS over Tor chris-belcher/electrum-personal-server#52

Closed

DrahtBot added the Needs rebase label Sep 10, 2018

chris-belcher force-pushed the 2018-04-rpc-getaddress branch from c131694 to a3fd879 Compare September 10, 2018 18:42

DrahtBot removed the Needs rebase label Sep 10, 2018

practicalswift reviewed Sep 11, 2018

View reviewed changes

test/functional/rpc_net.py Outdated Show resolved Hide resolved

scravy approved these changes Sep 11, 2018

View reviewed changes

DrahtBot mentioned this pull request Sep 12, 2018

[WIP] RPC: Add Method To Retrieve HTTP Server Metrics #14202

Closed

chris-belcher force-pushed the 2018-04-rpc-getaddress branch from a3fd879 to b3512ba Compare September 13, 2018 13:06

ken2812221 reviewed Sep 13, 2018

View reviewed changes

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

chris-belcher force-pushed the 2018-04-rpc-getaddress branch from b3512ba to 8b96ebe Compare September 13, 2018 18:16

jnewbery reviewed Sep 13, 2018

View reviewed changes

promag reviewed Sep 13, 2018

View reviewed changes

chris-belcher force-pushed the 2018-04-rpc-getaddress branch from 8b96ebe to a2eb6f5 Compare September 17, 2018 21:57

maflcko merged commit a2eb6f5 into bitcoin:master Sep 18, 2018

chris-belcher deleted the 2018-04-rpc-getaddress branch September 27, 2018 09:18

Bushstar mentioned this pull request Oct 11, 2018

Updates from bitcoin/master FeatherCoin/Feathercoin#366

Merged

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jul 20, 2021

Revert "Merge bitcoin#13152: [rpc] Add getnodeaddresses RPC command"

21a8956

This reverts commit 54e38eca Signed-off-by: pasta <pasta@dashboost.org>

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 Sep 8, 2021

[rpc] Add getnodeaddresses RPC command #13152

[rpc] Add getnodeaddresses RPC command #13152

Uh oh!

Conversation

chris-belcher commented May 2, 2018 • edited by maflcko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

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!

promag commented May 2, 2018

Uh oh!

laanwj commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chris-belcher commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chris-belcher commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented May 2, 2018

Uh oh!

MasterGrad17 commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chris-belcher commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli left a comment

Choose a reason for hiding this comment

Uh oh!

sipa commented May 3, 2018

Uh oh!

jonasschnelli commented May 3, 2018

Uh oh!

chris-belcher commented May 3, 2018

Uh oh!

laanwj commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chris-belcher commented May 3, 2018

Uh oh!

jnewbery left a comment

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!

laanwj May 7, 2018 • 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!

chris-belcher commented May 21, 2018

Uh oh!

jnewbery left a comment

chris-belcher commented May 2, 2018 •

edited by maflcko

Loading

laanwj commented May 2, 2018 •

edited

Loading

laanwj commented May 2, 2018 •

edited

Loading

chris-belcher commented May 2, 2018 •

edited

Loading

chris-belcher commented May 2, 2018 •

edited

Loading

MasterGrad17 commented May 2, 2018 •

edited

Loading

chris-belcher commented May 2, 2018 •

edited

Loading

laanwj commented May 3, 2018 •

edited

Loading

laanwj May 7, 2018 •

edited

Loading

promag Sep 13, 2018 •

edited

Loading