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

Closes #26579

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:

Bitcoin Core client v24.99.0-151ce099ea8f-dirty regtest - server 70016/Satoshi:24.99.0/

<->   type   net  mping   ping send recv  txn  blk  hb addrp addrl  age id address         version
 in          npr      0      0   90   90                              1  1 127.0.0.1:59180 70016/Satoshi:24.99.0/
out manual  ipv4     63     63   84   84         3                    3  0 143.244.175.41  70016/Satoshi:24.0.1/
                     ms     ms  sec  sec  min  min                  min

         ipv4    ipv6     npr   total   block  manual
in          0       0       1       1
out         1       0       0       1       0       1
total       1       0       1       2

Local addresses: n/a

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

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

<!--174a7506f384e20aa4161008e828411d-->

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.

Thanks! I'll have a look today.

Concept ACK

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

Both of these are kinda wrong :|

@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 ?

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.

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.

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.

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

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

<details><summary>tested by adding the following 3 lines</summary><p>

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

</p></details>

<details><summary>suggestions diff</summary><p>

@@ -58,7 +58,7 @@ static constexpr int8_t UNKNOWN_NETWORK{-1};
 // See GetNetworkName() in netbase.cpp
 static constexpr std::array NETWORKS{"not_publicly_routable", "ipv4", "ipv6", "onion", "i2p", "cjdns", "internal"};
 static constexpr std::array NETWORK_SHORT_NAMES{"npr", "ipv4", "ipv6", "onion", "i2p", "cjdns", "int"};
-static constexpr std::array NETWORK_UNREACHABLE{0 /* "not_publicly_routable" */, 6 /* "internal" */};
+static constexpr std::array UNREACHABLE_NETWORK_IDS{/*not_publicly_routable*/0, /*internal*/6};
 
 /** Default number of blocks to generate for RPC generatetoaddress. */
 static const std::string DEFAULT_NBLOCKS = "1";
@@ -575,10 +575,10 @@ public:
         for (const UniValue& network : networkinfo["networks"].getValues()) {
             if (network["reachable"].get_bool()) {
                 const std::string& network_name{network["name"].get_str()};
                 const int8_t network_id{NetworkStringToId(network_name)};
                 if (network_id == UNKNOWN_NETWORK) continue;
                 result += strprintf("%8s", network_name); // column header
                 reachable_networks.push_back(network_id);
             }
         };
 
-        for (const size_t index : NETWORK_UNREACHABLE) {
-            if (m_counts.at(2).at(index)) {
-                reachable_networks.push_back(index);
-                result += strprintf("%8s", NETWORK_SHORT_NAMES[index]);
+        for (size_t network_id : UNREACHABLE_NETWORK_IDS) {
+            if (m_counts.at(2).at(network_id)) {
+                result += strprintf("%8s", NETWORK_SHORT_NAMES.at(network_id)); // column header
+                reachable_networks.push_back(network_id);
             }
         }

</p></details>

@jonatack thank you! applied suggestions and f-pushed

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

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

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

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

<img width="482" alt="Screenshot 2023-02-01 at 13 36 12" src="https://user-images.githubusercontent.com/2415484/216168855-70c6437d-9efa-40fa-ae3a-bb0dfd03cf6e.png">

Re-tested ACK 77192c959816dc8daee138d88bd6f3250ce3bdb6

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