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.
660+ // returns a shuffled list of CAddress
661+ std::vector<CAddress> vAddr = g_connman->GetAddresses();
662+ UniValue ret(UniValue::VARR);
663+
664+ int address_return_count = std::min(count, (int)vAddr.size());
665+ for(int i = 0; i < address_return_count; ++i) {
for.
629+ if (request.fHelp || request.params.size() > 1) {
630+ throw std::runtime_error(
631+ "getaddress ( count )\n"
632+ "\nReturn known addresses which can potentially be used to find new nodes in the network\n"
633+ "\nArguments:\n"
634+ "1. \"count\" (numeric, optional) How many addresses to return, if available (default = 1)\n"
getpeerinfo?
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).
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.
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.
636+ "[\n"
637+ " {\n"
638+ " \"time\": ttt, (numeric) Address timestamp in seconds since epoch (Jan 1 1970 GMT)\n"
639+ " \"services\": n, (numeric) The services offered\n"
640+ " \"servicesHex\": \"xxxxxxxxxxxxxxxx\", (string) The hex string of services offered\n"
641+ " \"addr\": \"host\", (string) The IP address of the peer\n"
665+ for (int i = 0; i < address_return_count; ++i) {
666+ UniValue obj(UniValue::VOBJ);
667+ CAddress addr = vAddr[i];
668+ obj.pushKV("time", (int)addr.nTime);
669+ obj.pushKV("services", addr.nServices);
670+ obj.pushKV("servicesHex", strprintf("%016x", addr.nServices));
"NODE_NETWORK | NODE_NETWORK_LIMITED | NODE_BLOOM", etc.). But can be done later.
listpeeraddresses()
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?
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.
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.
getnodeaddresses is fine, just make sure that the documentation explains the functionality.
629+ if (request.fHelp || request.params.size() > 1) {
630+ throw std::runtime_error(
631+ "getnodeaddresses ( count )\n"
632+ "\nReturn known addresses which can potentially be used to find new nodes in the network\n"
633+ "\nArguments:\n"
634+ "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:
ADDRMAN_GETADDR_MAX in CAddrMan::GetAddr_()); or633+ "\nArguments:\n"
634+ "1. \"count\" (numeric, optional) How many addresses to return, if available (default = 1)\n"
635+ "\nResult:\n"
636+ "[\n"
637+ " {\n"
638+ " \"time\": ttt, (numeric) Address timestamp in seconds since epoch (Jan 1 1970 GMT)\n"
635+ "\nResult:\n"
636+ "[\n"
637+ " {\n"
638+ " \"time\": ttt, (numeric) Address timestamp in seconds since epoch (Jan 1 1970 GMT)\n"
639+ " \"services\": n, (numeric) The services offered\n"
640+ " \"servicesHex\": \"xxxxxxxxxxxxxxxx\", (string) The hex string of services offered\n"
636+ "[\n"
637+ " {\n"
638+ " \"time\": ttt, (numeric) Address timestamp in seconds since epoch (Jan 1 1970 GMT)\n"
639+ " \"services\": n, (numeric) The services offered\n"
640+ " \"servicesHex\": \"xxxxxxxxxxxxxxxx\", (string) The hex string of services offered\n"
641+ " \"addr\": \"host\", (string) The address of the peer\n"
address. No need to save characters!
634+ "1. \"count\" (numeric, optional) How many addresses to return, if available (default = 1)\n"
635+ "\nResult:\n"
636+ "[\n"
637+ " {\n"
638+ " \"time\": ttt, (numeric) Address timestamp in seconds since epoch (Jan 1 1970 GMT)\n"
639+ " \"services\": n, (numeric) The services offered\n"
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.
A few nits inline.
You should be able to test this in the functional test framework as follows:
getnodeaddresses RPCTake 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.
rpc_net.py.
623@@ -624,6 +624,55 @@ static UniValue setnetworkactive(const JSONRPCRequest& request)
624 return g_connman->GetNetworkActive();
625 }
626
627+UniValue getnodeaddresses(const JSONRPCRequest& request)
static
16@@ -16,6 +17,8 @@
17 p2p_port,
18 wait_until,
19 )
20+from test_framework.mininode import P2PInterface, network_thread_start
110+ now = int(time.time())
111+ msg = msg_addr()
112+ for a in imported_addrs:
113+ addr = CAddress()
114+ addr.time = now
115+ addr.services = NODE_NETWORK | NODE_WITNESS
nServices
104+ self.nodes[0].add_p2p_connection(P2PInterface())
105+ network_thread_start()
106+ self.nodes[0].p2p.wait_for_verack()
107+
108+ # send some addresses to the node via the p2p message addr
109+ 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:
0 # send some addresses to the node via the p2p message addr
1 now = int(time.time())
2 msg = msg_addr()
3 imported_addrs = []
4 for i in range(1000):
5 a = "123.123.{}.{}".format(i // 255, i % 256)
6 imported_addrs.append(a)
7 addr = CAddress()
8 ...
120+
121+ # obtain addresses via rpc call and check they are a subset
122+ NODE_ADDRESSES_REQUEST_COUNT = 10
123+ node_addresses = self.nodes[0].getnodeaddresses(NODE_ADDRESSES_REQUEST_COUNT)
124+ assert len(node_addresses) == NODE_ADDRESSES_REQUEST_COUNT
125+ 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:
0 for a in node_addresses:
1 assert a["address"] in imported_addrs
2 assert_equal(a["services"], NODE_NETWORK | NODE_WITNESS)
3 assert_equal(a["time"], now)
4 assert_equal(a["port"], 8333)
New test is great. Thanks for adding it!
A few nits inline. Feel free to take or leave them.
5@@ -6,6 +6,7 @@
6
7 Tests correspond to code in rpc/net.cpp.
8 """
9+import time
time to set the time field in the ADDR message. As long as the CAddress’s time is greater than 100000000, it’ll be accepted. You can avoid this import by just setting time to 100000000 + i or similar
Fixed nits, thanks for reviewing @jnewbery.
bitcoind modifies the timestamp so it’s complicated to check whether the time was equal. That’s not the point of the RPC or the test anyway so the test only checks whether the “time” field exists.
105+
106+ # send some addresses to the node via the p2p message addr
107+ msg = msg_addr()
108+ imported_addrs = []
109+ for i in range(1000):
110+ a = "123.123.{}.{}".format(i // 255, i % 256)
Note that you’ll only ever be able to get a maximum of 64 entries into the new addresses buckets by doing this. All of these addresses are in the same /16 address range, which means that addrman will place them into the same new bucket, and start evicting addresses once there are 64 entries (see the comment in addrman.h).
You may wish to simplify this to just send 256 addresses:
0 for i in range(256):
1 a = "123.123.123.{}".format(i)
One more comment inline, which answers the question you had on #bitcoin-core-dev IRC earlier.
bitcoind modifies the timestamp so it’s complicated to check whether the time was equal.
bitcoind modifies the timestamp if it falls outside these bounds:
0 if (addr.nTime <= 100000000 || addr.nTime > nNow + 10 * 60)
1 addr.nTime = nNow - 5 * 24 * 60 * 60;
but getting a valid time in that range is a bit difficult. if the time is greater than 30 days old, then the address will be marked as IsTerrible and won’t be returned in your RPC call. You could set the node’s mock time and then test that the returned time is that + (5 * 24 * 60 * 60).
utACK f10e38063048294afc6bd45c3a51687cdbdd96a3
(sorry for the delay)
rpc_net.py functional test is failing.
97@@ -96,5 +98,34 @@ def _test_getpeerinfo(self):
98 assert_equal(peer_info[0][0]['addrbind'], peer_info[1][0]['addr'])
99 assert_equal(peer_info[1][0]['addrbind'], peer_info[0][0]['addr'])
100
101+ def _test_getnodeaddresses(self):
102+ self.nodes[0].add_p2p_connection(P2PInterface())
103+ self.nodes[0].p2p.wait_for_verack()
add_p2p_connection() calls wait_for_verack() now)
Looks great. tACK 316201485933b4fcdf8d4005d823effdfc4b913b.
One nit inline. I also think the two commits can be squashed when you rebase this.
Rebased.
The test has failed but works when I run it locally.
Unrelated build error:
0...
1Get:17 http://archive.ubuntu.com/ubuntu bionic-updates/main amd64 Packages [389 kB]
2Get:18 http://archive.ubuntu.com/ubuntu bionic-backports/universe amd64 Packages [2807 B]
3Fetched 25.9 MB in 8min 28s (50.9 kB/s)
4Reading package lists...
5No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
6Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
Restarted job.
654+ throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");
655+ }
656+
657+ int count = 1;
658+ if (!request.params[0].isNull()) {
659+ count = request.params[0].get_int();
count > 0? Also, should have a maximum value?
121+ msg.addrs.append(addr)
122+ self.nodes[0].p2p.send_and_ping(msg)
123+
124+ # obtain addresses via rpc call and check they were ones sent in before
125+ NODE_ADDRESSES_REQUEST_COUNT = 10
126+ node_addresses = self.nodes[0].getnodeaddresses(NODE_ADDRESSES_REQUEST_COUNT)
NODE_ADDRESSES_REQUEST_COUNT.
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.
663+ UniValue ret(UniValue::VARR);
664+
665+ int address_return_count = std::min(count, (int)vAddr.size());
666+ for (int i = 0; i < address_return_count; ++i) {
667+ UniValue obj(UniValue::VOBJ);
668+ CAddress addr = vAddr[i];
const CAddress&?
124+ # obtain addresses via rpc call and check they were ones sent in before
125+ NODE_ADDRESSES_REQUEST_COUNT = 10
126+ node_addresses = self.nodes[0].getnodeaddresses(NODE_ADDRESSES_REQUEST_COUNT)
127+ assert_equal(len(node_addresses), NODE_ADDRESSES_REQUEST_COUNT)
128+ for a in node_addresses:
129+ # the timestamps are usually offset by a few hours, so only check existance
660+ }
661+ // returns a shuffled list of CAddress
662+ std::vector<CAddress> vAddr = g_connman->GetAddresses();
663+ UniValue ret(UniValue::VARR);
664+
665+ int address_return_count = std::min(count, (int)vAddr.size());
659+ count = request.params[0].get_int();
660+ if (count <= 0)
661+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Address count out of range");
662+ }
663+ // returns a shuffled list of CAddress
664+ std::vector<CAddress> vAddr = g_connman->GetAddresses();
125+ # obtain addresses via rpc call and check they were ones sent in before
126+ REQUEST_COUNT = 10
127+ node_addresses = self.nodes[0].getnodeaddresses(REQUEST_COUNT)
128+ assert_equal(len(node_addresses), REQUEST_COUNT)
129+ for a in node_addresses:
130+ # the timestamps are usually offset by a few hours, so only check existance
codespell: existance
127+ node_addresses = self.nodes[0].getnodeaddresses(REQUEST_COUNT)
128+ assert_equal(len(node_addresses), REQUEST_COUNT)
129+ for a in node_addresses:
130+ # the timestamps are usually offset by a few hours, so only check existence
131+ assert "time" in a
132+ assert_greater_than(a["time"], 1527811200) #1st June 2018
A very small nit, but to please PEP-8 consider fixing this:
0./test/functional/rpc_net.py:132:56: E262 inline comment should start with '# '
667+ int address_return_count = std::min<int>(count, vAddr.size());
668+ for (int i = 0; i < address_return_count; ++i) {
669+ UniValue obj(UniValue::VOBJ);
670+ const CAddress& addr = vAddr[i];
671+ obj.pushKV("time", (int)addr.nTime);
672+ obj.pushKV("services", addr.nServices);
Appveyor error:
0c:\projects\bitcoin\src\rpc\net.cpp(672): error C2668: 'UniValue::pushKV': ambiguous call to overloaded function
Maybe make it an explicit type?
0obj.pushKV("services", (uint64_t)addr.nServices);
656+
657+ int count = 1;
658+ if (!request.params[0].isNull()) {
659+ count = request.params[0].get_int();
660+ if (count <= 0)
661+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Address count out of range");
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.
126+ REQUEST_COUNT = 10
127+ node_addresses = self.nodes[0].getnodeaddresses(REQUEST_COUNT)
128+ assert_equal(len(node_addresses), REQUEST_COUNT)
129+ for a in node_addresses:
130+ # the timestamps are usually offset by a few hours, so only check existence
131+ assert "time" in a
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.
