cli: include local ("unroutable") peers in -netinfo table

pinheadmz commented at 1:55 pm on November 27, 2022: member

The -netinfo dashboard did not list peers that were connected via “unroutable” networks. This included local peers including local-network peers. Personally, I run one bitcoind instance on my network that is used by other services like Wasabi Wallet and LND running on other machines.

This PR adds an “npr” (not publicly routable) column to the table of networks (ipv4, ipv6, onion, etc) so that every connection to the node is listed, and the totals are accurate as they relate to max inbound and max outbound limits.

Example connecting in regtest mode to one local and one remote peer:

 0Bitcoin Core client v24.99.0-151ce099ea8f-dirty regtest - server 70016/Satoshi:24.99.0/
 1
 2<->   type   net  mping   ping send recv  txn  blk  hb addrp addrl  age id address         version
 3 in          npr      0      0   90   90                              1  1 127.0.0.1:59180 70016/Satoshi:24.99.0/
 4out manual  ipv4     63     63   84   84         3                    3  0 143.244.175.41  70016/Satoshi:24.0.1/
 5                     ms     ms  sec  sec  min  min                  min
 6
 7         ipv4    ipv6     npr   total   block  manual
 8in          0       0       1       1
 9out         1       0       0       1       0       1
10total       1       0       1       2
11
12Local addresses: n/a

DrahtBot commented at 1:55 pm on November 27, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	jonatack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26988 (rpc: Add test-only RPC addrmaninfo for new/tried table address count by stratospher)

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 commented at 3:52 pm on November 27, 2022: contributor

Thanks! I’ll have a look today.

in src/bitcoin-cli.cpp:58 in fb0f5d0bd4 outdated

54@@ -55,7 +55,7 @@ static constexpr int DEFAULT_WAIT_CLIENT_TIMEOUT = 0;
55 static const bool DEFAULT_NAMED=false;
56 static const int CONTINUE_EXECUTION=-1;
57 static constexpr int8_t UNKNOWN_NETWORK{-1};
58-static constexpr std::array NETWORKS{"ipv4", "ipv6", "onion", "i2p", "cjdns"};
59+static constexpr std::array NETWORKS{"ipv4", "ipv6", "onion", "i2p", "cjdns", "other"};

jonatack commented at 4:41 pm on November 27, 2022:

Changing NETWORKS here also affects -addrinfo, so it would need to be updated as well. Otherwise, with this change -addrinfo will print an additional line that always returns "other": 0,.

Would it make any sense to handle Network::NET_UNROUTABLE and Network::INTERNAL separately? Say, print npr for “not publicly routable” and int for “internal”. In theory both could potentially be returned by getpeerinfo, which calls GetNetworkName() for the string value, and the GUI handles both of them in NetworkToQString(). It doesn’t seem to make sense that we would ever see an “internal” returned by getpeerinfo (called by -netinfo) or by getnodeaddresses (called by -addrinfo, and getnodeaddresses also raises if called with “not publicly routable” passed as a network), but as a sanity check perhaps handle them like the GUI does.

Also, the -netinfo help would need to be updated for the net column description.

pinheadmz commented at 6:57 pm on November 27, 2022:

updated and force-pushed, i think the logic is cleaner now anyway, by just including the extra categories from GetNetworkName() but only displaying them if peers are actually connected there. Thanks!

in src/bitcoin-cli.cpp:575 in fb0f5d0bd4 outdated

569@@ -571,6 +570,10 @@ class NetinfoRequestHandler : public BaseRequestHandler
570                 reachable_networks.push_back(network_id);
571             }
572         };
573+
574+        reachable_networks.push_back(NETWORKS.size() - 1);
575+        result += strprintf("%8s", "other");

jonatack commented at 4:45 pm on November 27, 2022:

Just as we only print columns for networks we are using, I think it would be better to only print this column if we have any such peers.

pinheadmz commented at 6:56 pm on November 27, 2022:

:+1:

jonatack commented at 5:01 pm on November 27, 2022: contributor

Concept ACK

pinheadmz force-pushed on Nov 27, 2022

maflcko renamed this:
~~cli: include local ("unroutable") peers in -netinfo table~~
cli: include local ("unroutable") peers in -netinfo table
on Nov 28, 2022

DrahtBot added the label Scripts and tools on Nov 28, 2022

maflcko renamed this:
~~cli: include local ("unroutable") peers in -netinfo table~~
cli: include local ("unroutable") peers in -netinfo table
on Nov 28, 2022

in src/bitcoin-cli.cpp:60 in c0c45165e3 outdated

54@@ -55,7 +55,10 @@ static constexpr int DEFAULT_WAIT_CLIENT_TIMEOUT = 0;
55 static const bool DEFAULT_NAMED=false;
56 static const int CONTINUE_EXECUTION=-1;
57 static constexpr int8_t UNKNOWN_NETWORK{-1};
58-static constexpr std::array NETWORKS{"ipv4", "ipv6", "onion", "i2p", "cjdns"};
59+// See GetNetworkName() in netbase.cpp
60+static constexpr std::array NETWORKS{"not_publicly_routable", "ipv4", "ipv6", "onion", "i2p", "cjdns", "internal"};
61+static constexpr std::array NETWORK_NAMES{"npr", "ipv4", "ipv6", "onion", "i2p", "cjdns", "int"};

luke-jr commented at 9:29 pm on November 29, 2022:

NETWORK_SHORTNAMES?

in src/bitcoin-cli.cpp:61 in c0c45165e3 outdated

54@@ -55,7 +55,10 @@ static constexpr int DEFAULT_WAIT_CLIENT_TIMEOUT = 0;
55 static const bool DEFAULT_NAMED=false;
56 static const int CONTINUE_EXECUTION=-1;
57 static constexpr int8_t UNKNOWN_NETWORK{-1};
58-static constexpr std::array NETWORKS{"ipv4", "ipv6", "onion", "i2p", "cjdns"};
59+// See GetNetworkName() in netbase.cpp
60+static constexpr std::array NETWORKS{"not_publicly_routable", "ipv4", "ipv6", "onion", "i2p", "cjdns", "internal"};
61+static constexpr std::array NETWORK_NAMES{"npr", "ipv4", "ipv6", "onion", "i2p", "cjdns", "int"};
62+static constexpr std::array NETWORK_UNREACHABLE{0 /* "not_publicly_routable" */, 6 /* "internal" */};

luke-jr commented at 9:33 pm on November 29, 2022:

This code looks like it should probably get some refactoring. No reason it can’t work with network strings (not ids) and figure out what “unlisted” networks have peers on its own, and doing so would be more future-proof as an added bonus.

pinheadmz commented at 2:53 pm on November 30, 2022:

Good idea – would it make sense to #include <netbase.h> in bitcoin-cli.cpp? Then we can share one single list enum Network as well as GetNetworkName() and ParseNetwork() without re-writing those in bitcoin-cli. I could also add a new function GetNetworkShortName() in netbase.cpp so everything is in one place. If new networks are added in the future, bitcoin-cli.cpp shouldn’t need any changes…

pinheadmz commented at 7:16 pm on November 30, 2022:

@luke-jr gave this refactor a try in 7dd8b095ada2711487057788206deb7e310b8092

luke-jr commented at 9:51 pm on December 3, 2022:

No, I’m saying there shouldn’t be an enum Network or network_id at all - just use the strings.

pinheadmz commented at 1:50 pm on December 4, 2022:

ok something like std::map<std::string, int> to keep track of count of peers by network string?

pinheadmz force-pushed on Dec 2, 2022

luke-jr commented at 9:51 pm on December 3, 2022: member

0## Commit message ##
1-    cli: include local ("unroutable") peers in -netinfo table
2+    cli: include local ("unreachable") peers in -netinfo table

Both of these are kinda wrong :|

pinheadmz force-pushed on Dec 8, 2022

pinheadmz commented at 5:09 pm on December 8, 2022: member

@luke-jr I think I implemented the cleaner refactor you were thinking of in 0d0994a2e2e7b10ee428fde0f6c9235faf2a8e6a

I also applied that to -addrinfo which does have one drawback, we will now only list networks that come up in the getnodeaddresses RPC call, and not “all” networks, which were previously defined in a hard-coded list. Dunno if @jonatack has any thoughts on that, or if this is even considered a breaking API change ?

fanquake commented at 9:57 am on December 9, 2022: member

or if this is even considered a breaking API change ?

These interfaces have no API or guarantees. This is called out in -netinfo, and the same should hold for -addrinfo:

This human-readable interface will change regularly and is not intended to be a stable API.

in src/bitcoin-cli.cpp:281 in 0d0994a2e2 outdated

282-            ++counts.at(network_id);
283+            std::string network{node["network"].get_str()};
284+            if (m_counts.find(network) == m_counts.end()) {
285+                m_counts.insert({network, 0});
286+            }
287+            ++m_counts.find(network)->second;

luke-jr commented at 0:21 am on December 13, 2022:

0            ++m_counts[network];

pinheadmz commented at 2:24 pm on December 13, 2022:

thanks, fixed in bf69a13677680dbf2065e18e3e5c7882ec3aab33

in src/bitcoin-cli.cpp:464 in 0d0994a2e2 outdated

457@@ -470,20 +458,36 @@ class NetinfoRequestHandler : public BaseRequestHandler
458         if (networkinfo["version"].getInt<int>() < 209900) {
459             throw std::runtime_error("-netinfo requires bitcoind server to be running v0.21.0 and up");
460         }
461+        // Initialize the peer-count-by-network-type map with all the reachable networks (always display)
462+        for (const UniValue& network : networkinfo["networks"].getValues()) {
463+            if (!network["reachable"].get_bool())
464+                continue;

luke-jr commented at 0:24 am on December 13, 2022:

0            if (!network["reachable"].get_bool()) {
1                continue;
2            }

pinheadmz commented at 2:24 pm on December 13, 2022:

thanks, fixed in bf69a13677680dbf2065e18e3e5c7882ec3aab33

in src/bitcoin-cli.cpp:371 in 0d0994a2e2 outdated

367@@ -373,16 +368,9 @@ class NetinfoRequestHandler : public BaseRequestHandler
368 {
369 private:
370     static constexpr uint8_t MAX_DETAIL_LEVEL{4};
371-    std::array<std::array<uint16_t, NETWORKS.size() + 1>, 3> m_counts{{{}}}; //!< Peer counts by (in/out/total, networks/total)
372+    std::map<std::string, std::array<int, 3>> m_counts; //!< Peer counts by network type (in / out / total)

luke-jr commented at 0:32 am on December 13, 2022:

Total is just in+out, and turning the std::array into a std::pair gets default zero initialisation, which would be useful later. We only need total one place, so IMO we should just sum it there rather than counting one by one.

pinheadmz commented at 2:25 pm on December 13, 2022:

fixed in bf69a13677680dbf2065e18e3e5c7882ec3aab33 – the only messy part was that std::pair values can’t be retrieved by variable like i in the for loop, so I put a switch in there so that 0 = first and 1 = second, I dunno if that’s best.

in src/bitcoin-cli.cpp:515 in 0d0994a2e2 outdated

509@@ -506,7 +510,10 @@ class NetinfoRequestHandler : public BaseRequestHandler
510                 const bool is_addr_relay_enabled{peer["addr_relay_enabled"].isNull() ? false : peer["addr_relay_enabled"].get_bool()};
511                 const bool is_bip152_hb_from{peer["bip152_hb_from"].get_bool()};
512                 const bool is_bip152_hb_to{peer["bip152_hb_to"].get_bool()};
513-                m_peers.push_back({addr, sub_version, conn_type, network, age, min_ping, ping, addr_processed, addr_rate_limited, last_blck, last_recv, last_send, last_trxn, peer_id, mapped_as, version, is_addr_relay_enabled, is_bip152_hb_from, is_bip152_hb_to, is_outbound, is_tx_relay});
514+                std::string name = network;
515+                if (NETWORK_SHORTNAMES.find(name) != NETWORK_SHORTNAMES.end())
516+                    name = NETWORK_SHORTNAMES.find(name)->second;

luke-jr commented at 0:37 am on December 13, 2022:

Needs braces at least, but it would be nice to avoid finding twice too.

pinheadmz commented at 2:25 pm on December 13, 2022:

thanks fixed in bf69a13677680dbf2065e18e3e5c7882ec3aab33

in src/bitcoin-cli.cpp:571 in 0d0994a2e2 outdated

577-                result += strprintf("%8s", network_name); // column header
578-                reachable_networks.push_back(network_id);
579-            }
580-        };
581-        result += "   total   block";
582+        for (const auto& type : m_counts) {

luke-jr commented at 0:40 am on December 13, 2022:

Do we want to sort the networks in some particular order?

pinheadmz commented at 2:28 pm on December 13, 2022:

the networks will be sorted by reachable networks first (ipv4, ipv6) followed by any non-reachable networks we still have peers for (npr, i.e. local). The order of reachable networks will be provided by rpc getnetworkinfo which I think ultimately comes form the order of network names in netbase.cpp

luke-jr commented at 3:21 am on December 14, 2022:

std::map sorts alphabetically, IIRC?

luke-jr changes_requested

jonatack commented at 3:32 pm on December 13, 2022: contributor

Not sure I’m keen on the refactorings; are they necessary? It looks like they are trading an efficient data structure for a less-efficient one and moving additional logic/processing work into the loop.

luke-jr commented at 3:20 am on December 14, 2022: member

I’m not sure a std::map is less efficient than mapping to arbitrary [hash-like] values, the latter of which breaks/has to be kept updated any time the software changes. The refactor just makes the interface generic so it works without a hard-coded network list.

in src/bitcoin-cli.cpp:481 in bf69a13677 outdated

482+            m_counts[network];
483+            if (is_outbound) {
484+                ++m_counts.find(network)->second.second;
485+            } else {
486+                ++m_counts.find(network)->second.first;
487+            }

luke-jr commented at 3:25 am on December 14, 2022:

0            ++std::get<is_outbound ? 1 : 0>(m_counts[network]);

pinheadmz commented at 6:33 pm on December 15, 2022:

I didn’t think you can use a variable in std::get< ? > ?

0bitcoin-cli.cpp:476:15: error: no matching function for call to 'get'
1 ++std::get<is_outbound ? 1 : 0>(m_counts[network]);

https://stackoverflow.com/questions/43621098/c-stdgetvariable-fails

in src/bitcoin-cli.cpp:508 in bf69a13677 outdated

500@@ -506,7 +501,12 @@ class NetinfoRequestHandler : public BaseRequestHandler
501                 const bool is_addr_relay_enabled{peer["addr_relay_enabled"].isNull() ? false : peer["addr_relay_enabled"].get_bool()};
502                 const bool is_bip152_hb_from{peer["bip152_hb_from"].get_bool()};
503                 const bool is_bip152_hb_to{peer["bip152_hb_to"].get_bool()};
504-                m_peers.push_back({addr, sub_version, conn_type, network, age, min_ping, ping, addr_processed, addr_rate_limited, last_blck, last_recv, last_send, last_trxn, peer_id, mapped_as, version, is_addr_relay_enabled, is_bip152_hb_from, is_bip152_hb_to, is_outbound, is_tx_relay});
505+                std::string name = network;
506+                const std::map<std::string, std::string>::const_iterator it = NETWORK_SHORTNAMES.find(name);
507+                if (it != NETWORK_SHORTNAMES.end()) {
508+                    name = it->second;
509+                }

luke-jr commented at 3:31 am on December 14, 2022:

0                const std::map<std::string, std::string>::const_iterator it = NETWORK_SHORTNAMES.find(network);
1                const std::string& network_display_name = (it == NETWORK_SHORTNAMES.end()) ? network : it->second;

luke-jr commented at 3:46 am on December 14, 2022:

Or maybe add a nice utility function like:

0template <typename M>
1const M::mapped_type map_at_or(const M& map, const M::key_type& key, const M::mapped_type& default) {
2    const auto it = map.find(key);
3    if (it == map.end()) return default;
4    return it->second;
5}

Then we can do:

0                const std::string& network_display_name = map_at_or(NETWORK_SHORTNAMES, network, network);

in src/bitcoin-cli.cpp:591 in bf69a13677 outdated

597+                        count = std::get<1>(type.second); // outbound peers by network
598+                        break;
599+                    case 2:
600+                        count = std::get<0>(type.second) + std::get<1>(type.second); // total peers by network
601+                        break;
602+                }

luke-jr commented at 3:52 am on December 14, 2022:

0            for (const auto& [network, node_counts] : m_counts) {
1                const int count = (i == 2) ? (node_counts.first + node_counts.second) : std::get<i>(node_counts);

luke-jr changes_requested

pinheadmz force-pushed on Dec 16, 2022

pinheadmz commented at 7:35 pm on December 16, 2022: member

I reset this PR back to the original commit (with Luke’s nit addressed). I’d like to try and just get the minimal code changes merged to improve the feature. Then if you guys like we can talk about ways to refactor if necessary

pinheadmz force-pushed on Feb 1, 2023

jonatack commented at 9:05 pm on February 1, 2023: contributor

Tested ACK with suggestions for clarity or coherence with the existing code.

0@@ -562,6 +562,10 @@ public:
1             result += strprintf("                     ms     ms  sec  sec  min  min                %*s\n\n", m_max_age_length, "min");
2         }
3 
4+        m_counts.at(0).at(0) += 1;
5+        m_counts.at(1).at(0) += 2;
6+        m_counts.at(2).at(0) += 3;
7+
8         // Report peer connection totals by type.

 0@@ -58,7 +58,7 @@ static constexpr int8_t UNKNOWN_NETWORK{-1};
 1 // See GetNetworkName() in netbase.cpp
 2 static constexpr std::array NETWORKS{"not_publicly_routable", "ipv4", "ipv6", "onion", "i2p", "cjdns", "internal"};
 3 static constexpr std::array NETWORK_SHORT_NAMES{"npr", "ipv4", "ipv6", "onion", "i2p", "cjdns", "int"};
 4-static constexpr std::array NETWORK_UNREACHABLE{0 /* "not_publicly_routable" */, 6 /* "internal" */};
 5+static constexpr std::array UNREACHABLE_NETWORK_IDS{/*not_publicly_routable*/0, /*internal*/6};
 6 
 7 /** Default number of blocks to generate for RPC generatetoaddress. */
 8 static const std::string DEFAULT_NBLOCKS = "1";
 9@@ -575,10 +575,10 @@ public:
10         for (const UniValue& network : networkinfo["networks"].getValues()) {
11             if (network["reachable"].get_bool()) {
12                 const std::string& network_name{network["name"].get_str()};
13                 const int8_t network_id{NetworkStringToId(network_name)};
14                 if (network_id == UNKNOWN_NETWORK) continue;
15                 result += strprintf("%8s", network_name); // column header
16                 reachable_networks.push_back(network_id);
17             }
18         };
19 
20-        for (const size_t index : NETWORK_UNREACHABLE) {
21-            if (m_counts.at(2).at(index)) {
22-                reachable_networks.push_back(index);
23-                result += strprintf("%8s", NETWORK_SHORT_NAMES[index]);
24+        for (size_t network_id : UNREACHABLE_NETWORK_IDS) {
25+            if (m_counts.at(2).at(network_id)) {
26+                result += strprintf("%8s", NETWORK_SHORT_NAMES.at(network_id)); // column header
27+                reachable_networks.push_back(network_id);
28             }
29         }

pinheadmz force-pushed on Feb 1, 2023

pinheadmz commented at 9:21 pm on February 1, 2023: member

@jonatack thank you! applied suggestions and f-pushed

in src/bitcoin-cli.cpp:582 in ea4fbc5277 outdated

578+        for (const size_t network_id : UNREACHABLE_NETWORK_IDS) {
579+            if (m_counts.at(2).at(network_id)) {
580+                result += strprintf("%8s", NETWORK_SHORT_NAMES.at(network_id)); // column header
581+                reachable_networks.push_back(network_id);
582+            }
583+        }

jonatack commented at 9:43 pm on February 1, 2023:

Can ignore, the following version might be more idiomatic C++.

0-        for (const size_t network_id : UNREACHABLE_NETWORK_IDS) {
1-            if (m_counts.at(2).at(network_id)) {
2-                result += strprintf("%8s", NETWORK_SHORT_NAMES.at(network_id)); // column header
3-                reachable_networks.push_back(network_id);
4-            }
5+        for (size_t network_id : UNREACHABLE_NETWORK_IDS) {
6+            if (m_counts.at(2).at(network_id) == 0) continue;
7+            result += strprintf("%8s", NETWORK_SHORT_NAMES.at(network_id)); // column header
8+            reachable_networks.push_back(network_id);
9         }

pinheadmz commented at 12:44 pm on February 2, 2023:

Thanks! both suggestions applied.

jonatack commented at 9:44 pm on February 1, 2023: contributor

ACK ea4fbc5277c6fdf43cf675c8cc612f4821f55868, modulo the following change so that -addrinfo prints only the relevant networks:

0@@ -292,7 +292,7 @@ public:
1         // Prepare result to return to user.
2         UniValue result{UniValue::VOBJ}, addresses{UniValue::VOBJ};
3         uint64_t total{0}; // Total address count
4-        for (size_t i = 0; i < NETWORKS.size(); ++i) {
5+        for (size_t i = 1; i < NETWORKS.size() - 1; ++i) {

Tested this further with the following change on a running node:

0--- a/src/bitcoin-cli.cpp
1+++ b/src/bitcoin-cli.cpp
2@@ -478,7 +478,7 @@ public:
3         // Count peer connection totals, and if DetailsRequested(), store peer data in a vector of structs.
4         for (const UniValue& peer : batch[ID_PEERINFO]["result"].getValues()) {
5             const std::string network{peer["network"].get_str()};
6-            const int8_t network_id{NetworkStringToId(network)};
7+            const int8_t network_id = NetworkStringToId(network) % 2 ? 0 : 6;

pinheadmz force-pushed on Feb 2, 2023

cli: include local ("unreachable") peers in -netinfo table 77192c9598

pinheadmz force-pushed on Feb 2, 2023

jonatack commented at 8:10 pm on February 2, 2023: contributor

Re-tested ACK 77192c959816dc8daee138d88bd6f3250ce3bdb6

jonatack commented at 3:20 pm on February 13, 2023: contributor

This is a small, contained, non-risky change and seems fine to merge.

maflcko merged this on Feb 15, 2023

maflcko closed this on Feb 15, 2023

sidhujag referenced this in commit 9789149df4 on Feb 15, 2023

bitcoin locked this on Feb 15, 2024

cli: include local (“unroutable”) peers in -netinfo table #26584

Reviews

Conflicts