Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop IsLimited in favor of IsReachable #15138

Merged
merged 1 commit into from Jan 14, 2019

Conversation

Projects
None yet
7 participants
@Empact
Copy link
Member

commented Jan 10, 2019

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

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

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.

@laanwj laanwj added the P2P label Jan 10, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2019

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

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.

@Empact Empact force-pushed the Empact:is-reachable branch Jan 10, 2019

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2019

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.

@laanwj laanwj changed the title Drop IsReachable in favor of IsLimited Drop IsLimited in favor of IsReachable Jan 10, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

Thanks, concept ACK.

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

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

src/net.cpp Outdated
@@ -92,7 +92,7 @@ bool fListen = true;
bool fRelayTxes = true;
CCriticalSection cs_mapLocalHost;
std::map<CNetAddr, LocalServiceInfo> mapLocalHost GUARDED_BY(cs_mapLocalHost);
static bool vfLimited[NET_MAX] GUARDED_BY(cs_mapLocalHost) = {};
static std::vector<bool> nets_reachable GUARDED_BY(cs_mapLocalHost)(NET_MAX, true);

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jan 10, 2019

Member

nit: std::array<...> g_nets_reachable?

This comment has been minimized.

Copy link
@promag

promag Jan 10, 2019

Member
static auto g_nets_reachable GUARDED_BY(cs_mapLocalHost)(std::bitset<NET_MAX>{}.set());
@promag
Copy link
Member

left a comment

Concept ACK, +1 reachable.

src/net.cpp Outdated
@@ -92,7 +92,7 @@ bool fListen = true;
bool fRelayTxes = true;
CCriticalSection cs_mapLocalHost;
std::map<CNetAddr, LocalServiceInfo> mapLocalHost GUARDED_BY(cs_mapLocalHost);
static bool vfLimited[NET_MAX] GUARDED_BY(cs_mapLocalHost) = {};
static std::vector<bool> nets_reachable GUARDED_BY(cs_mapLocalHost)(NET_MAX, true);

This comment has been minimized.

Copy link
@promag

promag Jan 10, 2019

Member
static auto g_nets_reachable GUARDED_BY(cs_mapLocalHost)(std::bitset<NET_MAX>{}.set());

@Empact Empact force-pushed the Empact:is-reachable branch Jan 10, 2019

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2019

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

@Empact Empact force-pushed the Empact:is-reachable branch 2 times, most recently Jan 10, 2019

@promag

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

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()?

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2019

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.

@promag

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

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.

src/net.cpp Outdated
@@ -92,7 +93,7 @@ bool fListen = true;
bool fRelayTxes = true;
CCriticalSection cs_mapLocalHost;
std::map<CNetAddr, LocalServiceInfo> mapLocalHost GUARDED_BY(cs_mapLocalHost);
static bool vfLimited[NET_MAX] GUARDED_BY(cs_mapLocalHost) = {};
static std::bitset<NET_MAX> g_nets_reachable GUARDED_BY(cs_mapLocalHost)(std::bitset<NET_MAX>().set());

This comment has been minimized.

Copy link
@promag

promag Jan 11, 2019

Member

nit, I still have a preference for

static auto g_nets_reachable GUARDED_BY(cs_mapLocalHost){std::bitset<NET_MAX>().set()};

since it doesn't repeat the type — here the type is obvious.

This comment has been minimized.

Copy link
@Empact

Empact Jan 11, 2019

Author Member

It's a small difference, but I included it because I think it makes the declaration more readable - after GUARDED_BY seems a bit buried if you're scanning down the page.

This comment has been minimized.

Copy link
@laanwj

laanwj Jan 12, 2019

Member

This is the first use of std::bitset in this project. It's a small set, the memory savings here are minimal compared to the extra code that is being introduced to handle it.
I think the original code here was fine.

This comment has been minimized.

Copy link
@Empact

Empact Jan 12, 2019

Author Member

Yeah - I think of bitset as just vector<bool> without the interface requirements of vector, thus able to be more constrained (e.g. fixed size). But I'll revert to the array to narrow the focus of this PR.

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2019

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

@sipa

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

utACK a7d6098edff439bc26b811b8f11852eb792e6082

@Empact Empact force-pushed the Empact:is-reachable branch Jan 12, 2019

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jan 12, 2019

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

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)
Show resolved Hide resolved src/net.h Outdated

@Empact Empact force-pushed the Empact:is-reachable branch Jan 13, 2019

@ajtowns

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

utACK 04a1b53661a71c240a9244e70e5f990ae767a0bf

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

Drop IsLimited in favor of IsReachable
These two methods have had the same meaning, but inverted, since
110b62f. Having one name for a single
concept simplifies the code.

@Empact Empact force-pushed the Empact:is-reachable branch to d6b076c Jan 14, 2019

bool IsLimited(const CNetAddr& addr);

/**
* Mark a network as reachable or unreachable (no automatic connects to it)

This comment has been minimized.

Copy link
@laanwj

laanwj Jan 14, 2019

Member

I think the SetReachable comment is ok…? (oh it was updated, good)

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

utACK d6b076c

@laanwj laanwj merged commit d6b076c into bitcoin:master Jan 14, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 14, 2019

Merge #15138: Drop IsLimited in favor of IsReachable
d6b076c Drop IsLimited in favor of IsReachable (Ben Woosley)

Pull request description:

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

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

Tree-SHA512: 347ceb9e2a55ea06f4c01226411c7bbcade09dd82130e4c59d0824ecefd960875938022edbe5d4bfdf12b0552c9b4cb78b09a688284d707119571daf4eb371b4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.