Drop IsLimited in favor of IsReachable #15138

These two methods have had the same meaning, but inverted, since 110b62f06992d0fb989153afff2dc3aea62a674f. Having one name for a single concept simplifies the code.

This is a follow-up to #15051. /cc #7553

I like IsReachable better than IsLimited tbh, I think it's clearer, if we have to do this (I'm not convinced) I'd pefer to go for that.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #14856 ([WIP] net: remove more CConnman globals (theuni) by dongcarl)
• #14425 (Net: Do not re-enable Onion network when it was disabled via onlynet by wodry)

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.

Fair enough, I agree that's clearer.

• I removed the SetReachable default arg because I don't think it clarifies the call sites.
• I switched from bool[] to std::vector<bool> because it enables easy initialization.

Thanks, concept ACK.

I switched from bool[] to std::vector<bool> because it enables easy initialization.

What is the reasoning behind this? What what difficult about the old initialization?

Concept ACK, +1 reachable.

@laanwj I chose to invert the underlying variable to avoid a need to convert on the way in and out. The challenge of that is that it's surprisingly difficult to directly initialize an array of unknown size to all 1s.

As of now, using std::bitset as @promag suggested.

As of now, using std::bitset with an integer initializer, plus a static_assert to assure the int covers all bits.

What is the problem with initializing it with std::bitset<NET_MAX>{}.set()?

What is the problem with initializing it with std::bitset<NET_MAX>{}.set()?

Switched to your approach after I wrote that - int + static_assert is nice because it's a compile-time operation, but decided that's a form of micro-optimization, not worth the cost in readability.

Just noting that SetReachable now throws std::out_of_range (for instance SetReachable((Network)100, true)). It would be similar with std::array::at instead of std::array::operator[].

ACK a7d6098.

FYI I double-checked that NET_TEREDO, which would be out of bounds, does not extend into these apis.

utACK a7d6098edff439bc26b811b8f11852eb792e6082

Dropped std::bitset in favor of the original array, as per #15138 (review)

diff --git a/src/net.cpp b/src/net.cpp
index 95177fbae..40ec17d8e 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -37,7 +37,6 @@
 #include <miniupnpc/upnperrors.h>
 #endif
 
-#include <bitset>
 #include <unordered_map>
 
 #include <math.h>
@@ -93,7 +92,7 @@ bool fListen = true;
 bool fRelayTxes = true;
 CCriticalSection cs_mapLocalHost;
 std::map<CNetAddr, LocalServiceInfo> mapLocalHost GUARDED_BY(cs_mapLocalHost);
-static std::bitset<NET_MAX> g_nets_reachable GUARDED_BY(cs_mapLocalHost)(std::bitset<NET_MAX>().set());
+static bool vfLimited[NET_MAX] GUARDED_BY(cs_mapLocalHost) = {};
 std::string strSubVersion;
 
 limitedmap<uint256, int64_t> mapAlreadyAskedFor(MAX_INV_SZ);
@@ -258,13 +257,13 @@ void SetReachable(enum Network net, bool reachable)
     if (net == NET_UNROUTABLE || net == NET_INTERNAL)
         return;
     LOCK(cs_mapLocalHost);
-    g_nets_reachable.set(net, reachable);
+    vfLimited[net] = !reachable;
 }
 
 bool IsReachable(enum Network net)
 {
     LOCK(cs_mapLocalHost);
-    return g_nets_reachable.test(net);
+    return !vfLimited[net];
 }
 
 bool IsReachable(const CNetAddr &addr)

utACK 04a1b53661a71c240a9244e70e5f990ae767a0bf

nit: comment for SetReachable seems like it's a bit inverted (describes SetLimited` better)

utACK d6b076c17bc7d513243711563b262524ef0ba74c