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
Merged

Conversation

Empact
Copy link
Contributor

@Empact Empact 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
Copy link
Member

laanwj 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
Copy link
Contributor

DrahtBot 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
Copy link
Contributor Author

Empact 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
Copy link
Member

laanwj 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Empact
Copy link
Contributor Author

Empact 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 is-reachable branch 2 times, most recently from 7c9e200 to a7d6098 Compare January 10, 2019 20:23
@promag
Copy link
Contributor

promag 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
Copy link
Contributor Author

Empact 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
Copy link
Contributor

promag 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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@laanwj laanwj Jan 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

Empact commented Jan 11, 2019

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

@sipa
Copy link
Member

sipa commented Jan 11, 2019

utACK a7d6098edff439bc26b811b8f11852eb792e6082

@Empact
Copy link
Contributor Author

Empact 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)

src/net.h Outdated Show resolved Hide resolved
@ajtowns
Copy link
Contributor

ajtowns commented Jan 14, 2019

utACK 04a1b53661a71c240a9244e70e5f990ae767a0bf

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

These two methods have had the same meaning, but inverted, since
110b62f. Having one name for a single
concept simplifies the code.
bool IsLimited(const CNetAddr& addr);

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

@laanwj laanwj Jan 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@laanwj
Copy link
Member

laanwj commented Jan 14, 2019

utACK d6b076c

@laanwj laanwj merged commit d6b076c into bitcoin:master Jan 14, 2019
laanwj added a commit that referenced this pull request Jan 14, 2019
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 18, 2021
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 bitcoin#15051.
  /cc bitcoin#7553

Tree-SHA512: 347ceb9e2a55ea06f4c01226411c7bbcade09dd82130e4c59d0824ecefd960875938022edbe5d4bfdf12b0552c9b4cb78b09a688284d707119571daf4eb371b4
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 18, 2021
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 bitcoin#15051.
  /cc bitcoin#7553

Tree-SHA512: 347ceb9e2a55ea06f4c01226411c7bbcade09dd82130e4c59d0824ecefd960875938022edbe5d4bfdf12b0552c9b4cb78b09a688284d707119571daf4eb371b4
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Aug 11, 2021
ecde04a [Consensus] Bump Active Protocol version to 70923 for v5.3 (random-zebra)
b63e4f5 Consensus: Add v5.3 enforcement height for testnet. (furszy)
f44be94 Only relay IPv4, IPv6, Tor addresses (Pieter Wuille)
015298c fix: tor: Call event_base_loopbreak from the event's callback (furszy)
34ff7a8 Consensus: Add mnb ADDRv2 guard. (furszy)
b4515dc GUI: Present v3 onion addresses properly in MNs list. (furszy)
337d43d tests: don't export in6addr_loopback (Vasil Dimov)
2cde8e0 GUI: Do not show the tor v3 onion address in the topbar. (furszy)
0b5f406 Doc: update tor.md with latest upstream information. (furszy)
89df7f2 addrman: ensure old versions don't parse peers.dat (Vasil Dimov)
bb90c5c test: add getnetworkinfo network name regression tests (Jon Atack)
d8e01b5 rpc: update GetNetworksInfo() to not return unsupported networks (Jon Atack)
57fc7b0 net: update GetNetworkName() with all enum Network cases (Jon Atack)
647d60b tests: Modify rpc_bind to conform to bitcoin#14532 behaviour. (Carl Dong)
d4d6729 Allow running rpc_bind.py --nonloopback test without IPv6 (Kristaps Kaupe)
4a034d8 test: Add rpc_bind test to default-run tests (Wladimir J. van der Laan)
61a08af [tests] bind functional test nodes to 127.0.0.1  Prevents OSX firewall (Sjors Provoost)
6a4f1e0 test: Add basic addr relay test (furszy)
78aa61c net: Make addr relay mockable (furszy)
ba954ca Send and require SENDADDRV2 before VERACK (Pieter Wuille)
61c2ed4 Bump net protocol version + don't send 'sendaddrv2' to pre-70923 software (furszy)
ccd508a tor: make a TORv3 hidden service instead of TORv2 (Vasil Dimov)
6da9a14 net: advertise support for ADDRv2 via new message (furszy)
e58d5d0 Migrate to test_large_inv() to Misbehaving logging. (furszy)
d496b64 [QA] fix mininode CAddress ser/deser (Jonas Schnelli)
cec9567 net: CAddress & CAddrMan: (un)serialize as ADDRv2 Change the serialization of `CAddrMan` to serialize its addresses in ADDRv2/BIP155 format by default. Introduce a new `CAddrMan` format version (3). (furszy)
b8c1dda streams update: get rid of nType and nVersion. (furszy)
3eaa273 Support bypassing range check in ReadCompactSize (Pieter Wuille)
a237ba4 net: recognize TORv3/I2P/CJDNS networks (Vasil Dimov)
8e50853 util: make EncodeBase32 consume Spans (Sebastian Falbesoner)
1f67e30 net: CNetAddr: add support to (un)serialize as ADDRv2 (Vasil Dimov)
2455420 test: move HasReason so it can be reused (furszy)
d41adb4 util: move HasPrefix() so it can be reused (Vasil Dimov)
f6f86af Unroll Keccak-f implementation (Pieter Wuille)
45222e6 Implement keccak-f[1600] and SHA3-256 (Pieter Wuille)
08ad06d net: change CNetAddr::ip to have flexible size (furszy)
3337219 net: improve encapsulation of CNetAddr. (furszy)
910d5c4 test: Do not instantiate CAddrDB for static call (Hennadii Stepanov)
6b607ef Drop IsLimited in favor of IsReachable (Ben Woosley)
a40711b IsReachable is the inverse of IsLimited (DRY). Includes unit tests (marcaiaf)
8839828 net: don't accept non-left-contiguous netmasks (Vasil Dimov)
5d7f864 rpcbind: Warn about exposing RPC to untrusted networks (Luke Dashjr)
2a6abd8 CNetAddr: Add IsBindAny method to check for INADDR_ANY (Luke Dashjr)
4fdfa45 net: Always default rpcbind to localhost, never "all interfaces" (Luke Dashjr)
31064a8 net: Minor accumulated cleanups (furszy)
9f9c871 tests: Avoid using C-style NUL-terminated strings as arguments (practicalswift)
f6c52a3 tests: Add tests to make sure lookup methods fail on std::string parameters with embedded NUL characters (practicalswift)
a751b9b net: Avoid using C-style NUL-terminated strings as arguments in the netbase interface (furszy)
f30869d test: add IsRFC2544 tests (Mark Tyneway)
ed5abe1 Net: Proper CService deserialization + GetIn6Addr return false if addr isn't an IPv6 addr (furszy)
86d73fb net: save the network type explicitly in CNetAddr (Vasil Dimov)
ad57dfc net: document `enum Network` (Vasil Dimov)
cb160de netaddress: Update CNetAddr for ORCHIDv2 (Carl Dong)
c3c04e4 net: Better misbehaving logging (furszy)
3660487 net: Use C++11 member initialization in protocol (Marco)
082baa3 refactor: Drop unused CBufferedFile::Seek() (Hennadii Stepanov)
e2d776a util: CBufferedFile fixes (Larry Ruane)
6921f42 streams: backport OverrideStream class (furszy)

Pull request description:

  Conjunction of a large number of back ports, updates and refactorings that made with the final goal of implementing v3 Onion addresses support (BIP155 https://github.com/bitcoin/bips/blob/master/bip-0155.mediawiki) before the tor v2 addresses EOL, scheduled, by the Tor project, for (1) July 15th: v2 addr support removal from the code base, and (2) October 15th: v2 addr network disable, where **every peer in our network running under Tor will loose the connection and drop the network**.

  As BIP155 describes, this is introducing a new P2P message to gossip longer node addresses over the P2P network. This is required to support new-generation Onion addresses, I2P, and potentially other networks that have longer endpoint addresses than fit in the 128 bits of the current addr message.

  In order to achieve the end goal, had to:
  1.  Create Span class and push it up to latest Bitcoin implementation.
  2.  Update the whole serialization framework and every object using it up to latest Bitcoin implementation (3-4 years ahead of what we currently are in master).
  3.  Update the address manager implementing ASN-based bucketing of the network nodes.
  4.  Update and refactor the netAddress and address manager tests to latest Bitcoin implementation (4 years ahead of what we currently are in master).
  5.  Several util string, vector, encodings, parsing, hashing backports and more..

  Important note:
  This PR it is not meant to be merged as a standalone PR, will decouple smaller ones moving on. Adding on each sub-PR its own description isolated from this big monster.

  Second note:
  This is still a **work-in-progress**, not ready for testing yet. I'm probably missing to mention few PRs that have already adapted to our sources. Just making it public so can decouple the changes, we can start merging them and i can continue working a bit more confortable (rebase a +170 commits separate branch is not fun..).

  ### List of back ported and adapted PRs:

  Span and Serialization:
  ----------------
  *  bitcoin#12886.
  *  bitcoin#12916.
  *  bitcoin#13558.
  *  bitcoin#13697. (Only Span's commit 29943a9)
  *  bitcoin#17850.
  *  bitcoin#17896.
  *  bitcoin#12752.
  *  bitcoin#16577.
  *  bitcoin#16670. (without faebf62)
  *  bitcoin#17957.
  *  bitcoin#18021.
  *  bitcoin#18087.
  *  bitcoin#18112 (only from 353f376 that we don't support).
  *  bitcoin#18167.
  *  bitcoin#18317.
  *  bitcoin#18591 (only Span's commit 0fbde48)
  *  bitcoin#18468.
  *  bitcoin#19020.
  *  bitcoin#19032.
  *  bitcoin#19367.
  *  bitcoin#19387.

  Net, NetAddress and AddrMan:
  ----------------

  *  bitcoin#7932.
  *  bitcoin#10756.
  *  bitcoin#10765.
  *  bitcoin#12218.
  *  bitcoin#12855.
  *  bitcoin#13532.
  *  bitcoin#13575.
  *  bitcoin#13815.
  *  bitcoin#14532.
  *  bitcoin#15051.
  *  bitcoin#15138.
  *  bitcoin#15689.
  *  bitcoin#16702.
  *  bitcoin#17243.
  *  bitcoin#17345.
  *  bitcoin#17754.
  *  bitcoin#17758.
  *  bitcoin#17812.
  *  bitcoin#18023.
  *  bitcoin#18454.
  *  bitcoin#18512.
  *  bitcoin#19314.
  *  bitcoin#19687

  Keys and Addresses encoding:
  ----------------
  * bitcoin#11372.
  * bitcoin#17511.
  * bitcoin#17721.

  Util:
  ----------------
  * bitcoin#9140.
  * bitcoin#16577.
  * bitcoin#16889.
  * bitcoin#19593.

  Bench:
  ----------------
  * bitcoin#16299.

  BIP155:
  ----------------
  *  bitcoin#19351.
  *  bitcoin#19360.
  *  bitcoin#19534.
  *  bitcoin#19628.
  *  bitcoin#19841.
  *  bitcoin#19845.
  *  bitcoin#19954.
  *  bitcoin#19991 (pending).
  *  bitcoin#19845.
  *  bitcoin#20000 (pending).
  *  bitcoin#20120.
  *  bitcoin#20284.
  *  bitcoin#20564.
  *  bitcoin#21157 (pending).
  *  bitcoin#21564 (pending).
  *  Fully removed v2 onion addr support.
  *  Add hardcoded seeds.
  *  Add release-notes, changes to files.md and every needed documentation.

  I'm currently working on the PRs marked as "pending", this isn't over, but I'm pretty pretty close :). What a long road..

ACKs for top commit:
  random-zebra:
    utACK ecde04a
  Fuzzbawls:
    ACK ecde04a

Tree-SHA512: 82c95fbda76fce63f96d8a9af7fa9a89cb1e1b302b7891e27118a6103af0be23606bf202c7332fa61908205e6b6351764e2ec23d753f1e2484028f57c2e8b51a
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
@Empact Empact deleted the is-reachable branch April 18, 2022 19:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants