New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[rpc] Add getnodeaddresses RPC command #13152

Merged
merged 1 commit into from Sep 18, 2018

Conversation

@chris-belcher
Copy link
Contributor

chris-belcher commented May 2, 2018

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

This comment has been minimized.

Copy link
Member

laanwj commented May 2, 2018

Concept ACK, this would indeed be useful.

@promag
Copy link
Member

promag left a comment

Why not extend getpeerinfo?

UniValue ret(UniValue::VARR);

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

This comment has been minimized.

@promag

promag May 2, 2018

Member

Nit, space after for.

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

This comment has been minimized.

@promag

promag May 2, 2018

Member

Why not return all?

This comment has been minimized.

@chris-belcher

chris-belcher May 2, 2018

Contributor

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

This comment has been minimized.

Copy link
Member

promag commented May 2, 2018

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

This comment has been minimized.

Copy link
Member

laanwj commented May 2, 2018

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 chris-belcher force-pushed the chris-belcher:2018-04-rpc-getaddress branch from a5e0fc8 to 892daca May 2, 2018

@chris-belcher

This comment has been minimized.

Copy link
Contributor

chris-belcher commented May 2, 2018

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 chris-belcher force-pushed the chris-belcher:2018-04-rpc-getaddress branch 2 times, most recently from a28d1cf to c83ccf7 May 2, 2018

@chris-belcher

This comment has been minimized.

Copy link
Contributor

chris-belcher commented May 2, 2018

Fixed git weirdness (thanks to viasil).

@promag

This comment has been minimized.

Copy link
Member

promag commented May 2, 2018

Please rename PR.

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

@MasterGrad17

This comment has been minimized.

Copy link

MasterGrad17 commented May 2, 2018

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

This comment has been minimized.

Copy link
Contributor

chris-belcher commented May 2, 2018

@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

This comment has been minimized.

Copy link
Member

sipa commented May 2, 2018

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

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

This comment has been minimized.

@jonasschnelli

jonasschnelli May 3, 2018

Member

nit: could also be a tor address

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

This comment has been minimized.

@jonasschnelli

jonasschnelli May 3, 2018

Member

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

@jonasschnelli
Copy link
Member

jonasschnelli left a comment

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

@sipa

This comment has been minimized.

Copy link
Member

sipa commented May 3, 2018

@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

This comment has been minimized.

Copy link
Member

jonasschnelli commented May 3, 2018

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

This comment has been minimized.

Copy link
Contributor

chris-belcher commented May 3, 2018

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

This comment has been minimized.

Copy link
Member

laanwj commented May 3, 2018

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 chris-belcher force-pushed the chris-belcher:2018-04-rpc-getaddress branch 2 times, most recently from 73f046d to 9e6cac2 May 3, 2018

@chris-belcher

This comment has been minimized.

Copy link
Contributor

chris-belcher commented May 3, 2018

Fixed nit

@jnewbery
Copy link
Member

jnewbery left a comment

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.

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

This comment has been minimized.

@jnewbery

jnewbery May 3, 2018

Member

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)
"\nResult:\n"
"[\n"
" {\n"
" \"time\": ttt, (numeric) Address timestamp in seconds since epoch (Jan 1 1970 GMT)\n"

This comment has been minimized.

@jnewbery

jnewbery May 3, 2018

Member

What does 'address timestamp' mean here?

" {\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"

This comment has been minimized.

@jnewbery

jnewbery May 3, 2018

Member

return fields should be named in camel_case

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

This comment has been minimized.

@jnewbery

jnewbery May 3, 2018

Member

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

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

This comment has been minimized.

@jnewbery

jnewbery May 3, 2018

Member

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.

This comment has been minimized.

@laanwj

laanwj May 7, 2018

Member

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

This comment has been minimized.

@jnewbery

jnewbery May 7, 2018

Member

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 chris-belcher force-pushed the chris-belcher:2018-04-rpc-getaddress branch from 9e6cac2 to f0aca19 May 10, 2018

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

@chris-belcher

This comment has been minimized.

Copy link
Contributor

chris-belcher commented May 21, 2018

Fixed nits and created a test in rpc_net.py.

@jnewbery
Copy link
Member

jnewbery left a comment

New test is great. Thanks for adding it!

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

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

UniValue getnodeaddresses(const JSONRPCRequest& request)

This comment has been minimized.

@jnewbery

jnewbery May 21, 2018

Member

nit: should be static

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

This comment has been minimized.

@jnewbery

jnewbery May 21, 2018

Member

nit: sort imports (in PEP-8 ordering)

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

This comment has been minimized.

@jnewbery

jnewbery May 21, 2018

Member

Should be nServices

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

This comment has been minimized.

@jnewbery

jnewbery May 21, 2018

Member

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

This comment has been minimized.

@jnewbery

jnewbery May 21, 2018

Member

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)
@promag
Copy link
Member

promag left a comment

Care to add a release note by creating the file doc/release-notes-13152.md?

Show resolved Hide resolved src/rpc/net.cpp

# obtain addresses via rpc call and check they were ones sent in before
NODE_ADDRESSES_REQUEST_COUNT = 10
node_addresses = self.nodes[0].getnodeaddresses(NODE_ADDRESSES_REQUEST_COUNT)

This comment has been minimized.

@promag

promag Aug 27, 2018

Member

Use named arguments instead of long and verbose variables? Then ditch NODE_ADDRESSES_REQUEST_COUNT.

This comment has been minimized.

@promag

promag Aug 27, 2018

Member

Could also test that the result is limited by the existing addressed, not by the requested count.

This comment has been minimized.

@chris-belcher

chris-belcher Aug 31, 2018

Contributor

Unless I'm missing something, that verbose variable is still needed for the assert_equal check in the line after. It seems a better way than having magic numbers. I've reduced the length of the variable name though.

I haven't put a max value check because it depends on the size of g_connman->GetAddresses at that time. How that max value works is explained in the docs.

int address_return_count = std::min(count, (int)vAddr.size());
for (int i = 0; i < address_return_count; ++i) {
UniValue obj(UniValue::VOBJ);
CAddress addr = vAddr[i];

This comment has been minimized.

@promag

promag Aug 27, 2018

Member

const CAddress&?

node_addresses = self.nodes[0].getnodeaddresses(NODE_ADDRESSES_REQUEST_COUNT)
assert_equal(len(node_addresses), NODE_ADDRESSES_REQUEST_COUNT)
for a in node_addresses:
# the timestamps are usually offset by a few hours, so only check existance

This comment has been minimized.

@promag

promag Aug 27, 2018

Member

But could check that value is greater than some value?

std::vector<CAddress> vAddr = g_connman->GetAddresses();
UniValue ret(UniValue::VARR);

int address_return_count = std::min(count, (int)vAddr.size());

This comment has been minimized.

@promag

promag Aug 27, 2018

Member

Why the cast? Could inline std::min below.

@chris-belcher chris-belcher force-pushed the chris-belcher:2018-04-rpc-getaddress branch 2 times, most recently from c5c4e59 to e6a6cea Aug 31, 2018

@chris-belcher

This comment has been minimized.

Copy link
Contributor

chris-belcher commented Aug 31, 2018

Thanks for the review @promag. I have created the release notes entry in the style of the release-notes-0.15.0 file.

Show resolved Hide resolved src/rpc/net.cpp Outdated
node_addresses = self.nodes[0].getnodeaddresses(REQUEST_COUNT)
assert_equal(len(node_addresses), REQUEST_COUNT)
for a in node_addresses:
# the timestamps are usually offset by a few hours, so only check existance

This comment has been minimized.

@practicalswift

practicalswift Sep 2, 2018

Member

Typo found by codespell: existance

@chris-belcher chris-belcher force-pushed the chris-belcher:2018-04-rpc-getaddress branch from e6a6cea to c131694 Sep 3, 2018

@chris-belcher chris-belcher force-pushed the chris-belcher:2018-04-rpc-getaddress branch from c131694 to a3fd879 Sep 10, 2018

@DrahtBot DrahtBot removed the Needs rebase label Sep 10, 2018

@scravy

scravy approved these changes Sep 11, 2018

@chris-belcher chris-belcher force-pushed the chris-belcher:2018-04-rpc-getaddress branch from a3fd879 to b3512ba Sep 13, 2018

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

@chris-belcher chris-belcher force-pushed the chris-belcher:2018-04-rpc-getaddress branch from b3512ba to 8b96ebe Sep 13, 2018

@promag

This comment has been minimized.

Copy link
Member

promag commented Sep 13, 2018

Looks good to me, utACK 8b96ebe.

@jnewbery
Copy link
Member

jnewbery left a comment

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.

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

This comment has been minimized.

@jnewbery

jnewbery Sep 13, 2018

Member

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)

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

This comment has been minimized.

@promag

promag Sep 13, 2018

Member

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

[rpc] Add getnodeaddresses RPC command
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 chris-belcher force-pushed the chris-belcher:2018-04-rpc-getaddress branch from 8b96ebe to a2eb6f5 Sep 17, 2018

@chris-belcher

This comment has been minimized.

Copy link
Contributor

chris-belcher commented Sep 18, 2018

Fixed those nits

@ajtowns

This comment has been minimized.

Copy link
Contributor

ajtowns commented Sep 18, 2018

utACK a2eb6f5

1 similar comment
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Sep 18, 2018

utACK a2eb6f5

@promag

This comment has been minimized.

Copy link
Member

promag commented Sep 18, 2018

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

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Sep 18, 2018

Merge bitcoin#13152: [rpc] Add getnodeaddresses RPC command
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

@MarcoFalke MarcoFalke merged commit a2eb6f5 into bitcoin:master Sep 18, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@chris-belcher chris-belcher deleted the chris-belcher:2018-04-rpc-getaddress branch Sep 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment