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.

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!

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.

👍

NETWORK_SHORTNAMES?

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.

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

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

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

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

thanks, fixed in bf69a13677680dbf2065e18e3e5c7882ec3aab33

thanks, fixed in bf69a13677680dbf2065e18e3e5c7882ec3aab33

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.

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.

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

thanks fixed in bf69a13677680dbf2065e18e3e5c7882ec3aab33

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

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

std::map sorts alphabetically, IIRC?

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

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

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

Or maybe add a nice utility function like:

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

Then we can do:

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

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

-        for (const 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);
-            }
+        for (size_t network_id : UNREACHABLE_NETWORK_IDS) {
+            if (m_counts.at(2).at(network_id) == 0) continue;
+            result += strprintf("%8s", NETWORK_SHORT_NAMES.at(network_id)); // column header
+            reachable_networks.push_back(network_id);
         }

Thanks! both suggestions applied.