net, rpc, cli: expose peer network in getpeerinfo; simplify/improve -netinfo

jonatack commented at 11:47 am on September 23, 2020: member

This PR:

builds on #19991 and #19998
exposes peer networks via a new getpeerinfo network field (“ipv4”, “ipv6”, or “onion”), and adds functional tests
updates -netinfo to use getpeerinfo network rather than detecting the peer networks client-side
refactors -netinfo to easily add future networks

practicalswift commented at 11:50 am on September 23, 2020: contributor

Concept ACK

Thanks for improving -netinfo! :)

jonatack force-pushed on Sep 23, 2020

DrahtBot added the label P2P on Sep 23, 2020

DrahtBot added the label RPC/REST/ZMQ on Sep 23, 2020

jonatack force-pushed on Sep 23, 2020

DrahtBot commented at 4:22 pm on September 23, 2020: 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:

#20120 (net, rpc, test, bugfix: update GetNetworkName, GetNetworksInfo, regression tests by jonatack)
#20115 (cli: -netinfo quick updates/fixups and release note by jonatack)

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.

jonatack force-pushed on Sep 23, 2020

jonatack force-pushed on Sep 24, 2020

jonatack renamed this:
~~net, rpc, cli: expose CNetAddr::GetNetClass in getpeerinfo, use in -netinfo~~
net, rpc, cli: expose GetNetClass()/ConnectedViaTor() in getpeerinfo, use in -netinfo
on Sep 24, 2020

hebasto commented at 2:53 pm on September 24, 2020: member

Concept ACK.

DrahtBot added the label Needs rebase on Sep 26, 2020

jonatack force-pushed on Oct 2, 2020

DrahtBot removed the label Needs rebase on Oct 2, 2020

jonatack renamed this:
~~net, rpc, cli: expose GetNetClass()/ConnectedViaTor() in getpeerinfo, use in -netinfo~~
net, rpc, cli: expose peer network in getpeerinfo; simplify/improve -netinfo
on Oct 2, 2020

hebasto commented at 7:48 am on October 3, 2020: member

Tested 5604a61435c5dfdeed41db5eca489b353dee0b99 on Linux Mint 20 (x86_64):

 0$ src/bitcoin-cli -netinfo 4
 1Bitcoin Core v0.20.99.0-5604a6143 - 70016/Satoshi:0.20.99/
 2
 3Peer connections sorted by direction and min ping
 4<-> relay   net mping   ping send recv  txn  blk uptime  id address                     version
 5 in  full onion   178   1081   11   76              229 171 127.0.0.1:37328             70015/bitnodes.io:0.1/
 6 in block onion   241    558   29   29              265 157 127.0.0.1:34910             70015/Satoshi:0.20.1/
 7 in block onion   245    399  110  110              289 144 127.0.0.1:33322             70015/Satoshi:0.20.1/
 8 in  full onion   359    430   10    1    1   37     44 268 127.0.0.1:49482             70015/Satoshi:0.19.1/
 9out  full onion   251    450    1    3    0         468  78 jqsx6ag4hujpapxq.onion:8333 70015/Satoshi:0.20.0/
10out  full onion   261    352    1    1    0   12    625   6 tuwj7ju4s25345pg.onion:8333 70015/Satoshi:0.19.0.1/
11out block onion   280    528   58   58       624    625   9 kzrnyo7fadxihvgb.onion:8333 70015/Satoshi:0.20.0/
12out  full onion   329    474    2    5    0  623    625   7 2mmxouhv6nebowkq.onion:8333 70015/Satoshi:0.18.1/
13out  full onion   372    622    3    3    1  623    625   4 q467hmvzy5kfvoed.onion:8333 70015/Satoshi:0.20.1/
14out  full onion   380    546    2   11    0  624    625   8 ee5skvb3ktbposl3.onion:8333 70015/Satoshi:0.20.0/
15out  full onion   392    623    0    6    0  624    626   2 6xiqqr4fxnstd5fo.onion:8333 70015/Satoshi:0.20.1/
16out  full onion   421    556    2    7    0  267    625   5 ohrieqovuxe6y4gl.onion:8333 70015/Satoshi:0.20.0/
17out block onion   465    578  112  111              246 162 a53vtdm7uiet5vdl.onion:8333 70015/Satoshi:0.19.1/
18out  full onion  1717   1717    5    0    0           1 290 mbciulcc3wuksb3f.onion:8333 70015/Satoshi:0.20.0/
19                   ms     ms  sec  sec  min  min    min
20
21        ipv4    ipv6   onion   total  block-relay
22in         0       0       4       4       2
23out        0       0      10      10       2
24total      0       0      14      14       4
25...

in src/rpc/net.cpp:97 in bebd062eef outdated

93@@ -94,6 +94,7 @@ static RPCHelpMan getpeerinfo()
94                             {RPCResult::Type::STR, "addr", "(host:port) The IP address and port of the peer"},
95                             {RPCResult::Type::STR, "addrbind", "(ip:port) Bind address of the connection to the peer"},
96                             {RPCResult::Type::STR, "addrlocal", "(ip:port) Local address as reported by the peer"},
97+                            {RPCResult::Type::STR, "network", "Network (ipv4, ipv6, or onion) the peer connected through"},

hebasto commented at 8:08 am on October 3, 2020:

bebd062eef3fa2d33c9216c368977dbedb70a443

While here, mind making “addrbind” and “addrlocal” ordering consistent for help and output?

jonatack commented at 7:16 pm on October 11, 2020:

Good idea. Done.

in test/functional/feature_proxy.py:165 in bebd062eef outdated

161@@ -143,6 +162,7 @@ def node_test(self, node, proxies, auth, test_onion=True):
162             assert_equal(cmd.username, None)
163             assert_equal(cmd.password, None)
164         rv.append(cmd)
165+        self.network_test(node, addr, network=NET_UNROUTABLE)

hebasto commented at 8:31 am on October 3, 2020:

bebd062eef3fa2d33c9216c368977dbedb70a443

Testing of network=NET_UNROUTABLE is actually skipped. Could we avoid such skipping?

jonatack commented at 7:23 pm on October 11, 2020:

When I run feature_proxy.py with -l debug, or with the following patch, it doesn’t seem to be skipped in my testing. LMK if I’m misunderstanding.

0+++ b/test/functional/feature_proxy.py
1@@ -101,6 +101,7 @@ class ProxyTest(BitcoinTestFramework):
2         for peer in node.getpeerinfo():
3             if peer["addr"] == addr:
4                 assert_equal(peer["network"], network)
5+                import pprint; pprint.pprint(network)

in src/bitcoin-cli.cpp:299 in 5604a61435 outdated

310-        const size_t onion_pos{addr.rfind(ONION)};
311-        return mapped_as == 0 && onion_pos != std::string::npos && addr_len > ONION_LEN &&
312-               (onion_pos == addr_len - ONION_LEN || onion_pos == addr.find_last_of(":") - ONION_LEN);
313-    }
314-    uint8_t m_details_level{0}; //!< Optional user-supplied arg to set dashboard details level
315+    static const int m_networks = 3; // Number of possible networks

hebasto commented at 8:39 am on October 3, 2020:

5604a61435c5dfdeed41db5eca489b353dee0b99

0    static constexpr int m_networks = 3; //!< Number of possible networks.

jonatack commented at 7:17 pm on October 11, 2020:

Done

in src/bitcoin-cli.cpp:300 in 5604a61435 outdated

311-        return mapped_as == 0 && onion_pos != std::string::npos && addr_len > ONION_LEN &&
312-               (onion_pos == addr_len - ONION_LEN || onion_pos == addr.find_last_of(":") - ONION_LEN);
313-    }
314-    uint8_t m_details_level{0}; //!< Optional user-supplied arg to set dashboard details level
315+    static const int m_networks = 3; // Number of possible networks
316+    uint8_t m_details_level = 0;     //!< Optional user-supplied arg to set dashboard details level

hebasto commented at 8:40 am on October 3, 2020:

Why switch from modern {} initialization to the old style?

jonatack commented at 7:18 pm on October 11, 2020:

Done, retained modern style. Thanks.

hebasto approved

hebasto commented at 8:42 am on October 3, 2020: member

ACK 5604a61435c5dfdeed41db5eca489b353dee0b99

jonatack force-pushed on Oct 11, 2020

jonatack commented at 7:25 pm on October 11, 2020: member

Thanks for reviewing @hebasto. I’ve addressed your feedback.

laanwj referenced this in commit f79a4a8952 on Oct 12, 2020

laanwj commented at 4:42 pm on October 12, 2020: member

Needs rebase after #19998.

DrahtBot added the label Needs rebase on Oct 12, 2020

jonatack force-pushed on Oct 12, 2020

DrahtBot removed the label Needs rebase on Oct 12, 2020

jonatack commented at 10:12 am on October 13, 2020: member

Rebased. If merged for 0.21 will add a release note manually in the wiki.

sidhujag referenced this in commit 2180fb4d7a on Oct 13, 2020

jonatack force-pushed on Oct 14, 2020

in src/bitcoin-cli.cpp:306 in 6c08a4b6e7 outdated

314-        return mapped_as == 0 && onion_pos != std::string::npos && addr_len > ONION_LEN &&
315-               (onion_pos == addr_len - ONION_LEN || onion_pos == addr.find_last_of(":") - ONION_LEN);
316+        for (const auto& network : m_networks) {
317+            if (str == network.first) return network.second;
318+        }
319+        assert(false);

laanwj commented at 11:27 am on October 14, 2020:

Let’s not do assert(false) here. A crash on “invalid” input from the RPC is never good, it is good to be slightly forgiving with regard to forward-compatibility. Maybe return an ‘unknown’ category instead that is ignored with regard to the statistics/table.

jonatack commented at 1:29 pm on October 14, 2020:

Done!

in src/interfaces/node.h:26 in 548f8c8e92 outdated

22@@ -23,7 +23,7 @@
23 class BanMan;
24 class CCoinControl;
25 class CFeeRate;
26-class CNodeStats;
27+struct CNodeStats;

laanwj commented at 11:31 am on October 14, 2020:

~0 on the change from a class to a struct, I think calling it a struct is more consistent with how it’s used but it doesn’t seem to be a logical part of this change

jonatack commented at 1:29 pm on October 14, 2020:

Removed the struct change.

laanwj commented at 11:37 am on October 14, 2020: member

Rebased. If merged for 0.21 will add a release note manually in the wiki.

+1 in getting this into 0.21, it’s a low risk change and is kind of the culmination of work that’s been going on for the entire release cycle

laanwj added this to the milestone 0.21.0 on Oct 14, 2020

hebasto commented at 11:39 am on October 14, 2020: member

Tested 6c08a4b6e755efa40f021ff4e7ec0d69379cc773 on Linux Mint 20 (x86_64). LGTM.

net: add peer network to CNodeStats 6df7882029

rpc, test: expose CNodeStats network in RPC getpeerinfo 4938a109ad

cli: simplify -netinfo using getpeerinfo network field 5133fab37e

refactor: promote some -netinfo localvars to class members 82fd40216c

in src/bitcoin-cli.cpp:299 in 6c08a4b6e7 outdated

295@@ -298,30 +296,22 @@ class GetinfoRequestHandler: public BaseRequestHandler
296 class NetinfoRequestHandler : public BaseRequestHandler
297 {
298 private:
299-    bool IsAddrIPv6(const std::string& addr) const
300+    const std::vector<std::pair<std::string, uint8_t>> m_networks{{"ipv4", 0}, {"ipv6", 1}, {"onion", 2}};

laanwj commented at 11:47 am on October 14, 2020:

As these IDs are always consecutively, and you iterate over it linearly, and you make the assumption that maxid is size()-1. why not simply

0const std::vector<std::string> m_networks{"ipv4","ipv6","onion"};

alternatively, lookup would be easier with a std::map.

jonatack commented at 1:32 pm on October 14, 2020:

Good idea to flatten it–done, and changed the new data structures in the last commit to fixed-sized std::arrays.

jonatack force-pushed on Oct 14, 2020

jonatack commented at 1:38 pm on October 14, 2020: member

Thanks for the feedback! Updated. (Edit: repushed to wrap array initialization in extra braces for one compiler on travis that needed appeasing.)

jonatack force-pushed on Oct 14, 2020

refactor: enable -netinfo to add future networks (i2p, cjdns)

After this commit, a new network may be added by changing 4 lines:
- increment the value of `m_networks_size`
- add the network name to `m_networks`
- add the network name to this line: `result += "        ipv4    ipv6   onion   total  block-relay\n";`
- add "counts.at(i).at(<m_networks pos>)" to this line: `result += strprintf("%-5s  %5i   %5i   %5i   %5i   %5i\n...`

6272604bef

jonatack force-pushed on Oct 14, 2020

laanwj commented at 3:43 pm on October 15, 2020: member

ACK 6272604bef3b409455b010d134b4b62c8f6ff49f

laanwj merged this on Oct 15, 2020

laanwj closed this on Oct 15, 2020

jonatack deleted the branch on Oct 15, 2020

sidhujag referenced this in commit 37b55282d6 on Oct 16, 2020

jonatack commented at 5:14 pm on November 2, 2020: member

Rebased. If merged for 0.21 will add a release note manually in the wiki.

Done, manually added a release note in the wiki for getpeerinfo#network.

Fabcien referenced this in commit 58dc89eaaa on Nov 24, 2021

Fabcien referenced this in commit 0e151b815a on Nov 24, 2021

Fabcien referenced this in commit 937e21bca4 on Nov 24, 2021

Fabcien referenced this in commit 47738f027e on Nov 24, 2021

Fabcien referenced this in commit 46866bc04b on Nov 24, 2021

DrahtBot locked this on Feb 15, 2022

net, rpc, cli: expose peer network in getpeerinfo; simplify/improve -netinfo #20002

Conflicts