Handle CJDNS from LookupSubNet()

vasild commented at 11:20 am on February 10, 2023: contributor

LookupSubNet() would treat addresses that start with fc as IPv6 even if -cjdnsreachable is set. This creates the following problems where it is called:

NetWhitelistPermissions::TryParse(): otherwise -whitelist= fails to white list CJDNS addresses: when a CJDNS peer connects to us, it will be matched against IPv6 fc... subnet and the match will never succeed.
BanMapFromJson(): CJDNS bans are stored as just IPv6 addresses in banlist.json. Upon reading from disk they have to be converted back to CJDNS, otherwise, after restart, a ban entry like (fc00::1, IPv6) would not match a peer (fc00::1, CJDNS).
RPCConsole::unbanSelectedNode(): in the GUI the ban entries go through CSubNet::ToString() and back via LookupSubNet(). Then it must match whatever is stored in BanMan, otherwise it is impossible to unban via the GUI.

These were uncovered by #26859.

Thus, flip the result of LookupSubNet() to CJDNS if the network base address starts with fc and -cjdnsreachable is set. Since subnetting/masking does not make sense for CJDNS (the address is “random” bytes, like Tor and I2P, there is no hierarchy) treat fc.../mask as an invalid CSubNet.

To achieve that, MaybeFlipIPv6toCJDNS() has to be moved from net to netbase and thus also IsReachable(). In the process of moving IsReachable(), SetReachable() and vfLimited[] encapsulate those in a class.

DrahtBot commented at 11:20 am on February 10, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	mzumsande, jonatack, achow101
Concept ACK	brunoerg

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28248 (p2p: peer connection bug fixes, logic and logging improvements by jonatack)
#28170 (p2p: adaptive connections services flags by furszy)
#27837 (net: introduce block tracker to retry to download blocks after failure by furszy)
#27557 (net: call getaddrinfo() in detachable thread to prevent stalling by pinheadmz)
#27375 (net: support unix domain sockets for -proxy and -onion by pinheadmz)
#25390 (sync: introduce a thread-safe generic container and use it to remove a bunch of “GlobalMutex"es by vasild)

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.

in src/netbase.cpp:35 in 573117f79a outdated

39@@ -40,6 +40,8 @@ bool fNameLookup = DEFAULT_NAME_LOOKUP;
40 int g_socks5_recv_timeout = 20 * 1000;
41 static std::atomic<bool> interruptSocks5Recv(false);
42 
43+ReachableNets g_reachable_nets;

mzumsande commented at 9:18 pm on February 12, 2023:

Does this need a Mutex, considering it’s being accessed from both Node and GUI?

vasild commented at 5:12 pm on February 13, 2023:

It has inside, ReachableNets::m_mutex.

in src/netbase.cpp:697 in 45a79d48ca outdated

693@@ -694,6 +694,7 @@ bool LookupSubNet(const std::string& subnet_str, CSubNet& subnet_out)
694     CNetAddr addr;
695 
696     if (LookupHost(str_addr, addr, /*fAllowLookup=*/false)) {
697+        addr = MaybeFlipIPv6toCJDNS(CService{addr, /*port=*/0});

mzumsande commented at 3:38 pm on February 13, 2023:

Now that MaybeFlipIPv6toCJDNS is available in netbase, would it make sense (maybe in follow-ups) to also use it in the Lookup() functions, not just LookupSubNet) and reduce the number of occurrences we have to call MaybeFlipIPv6toCJDNS from outside it?

vasild commented at 5:53 pm on February 13, 2023:

Oh, well. I feel like MaybeFlipIPv6toCJDNS() is warranted in some of the lower level Lookup*() functions. But they are called from so many places, the change would be very invasive. I have to gather some bravery in order to look into that. If that is to be done, maybe it should be preceded by some simplification of the Lookup*() family of functions. I get a headache every time I have to deal with:

0bool LookupHost(const std::string& name, std::vector<CNetAddr>& vIP, unsigned int nMaxSolutions, bool fAllowLookup, DNSLookupFn dns_lookup_function = g_dns_lookup);
1bool LookupHost(const std::string& name, CNetAddr& addr, bool fAllowLookup, DNSLookupFn dns_lookup_function = g_dns_lookup);
2bool Lookup(const std::string& name, std::vector<CService>& vAddr, uint16_t portDefault, bool fAllowLookup, unsigned int nMaxSolutions, DNSLookupFn dns_lookup_function = g_dns_lookup);
3bool Lookup(const std::string& name, CService& addr, uint16_t portDefault, bool fAllowLookup, DNSLookupFn dns_lookup_function = g_dns_lookup);
4CService LookupNumeric(const std::string& name, uint16_t portDefault = 0, DNSLookupFn dns_lookup_function = g_dns_lookup);
5bool LookupIntern(const std::string& name, std::vector<CNetAddr>& vIP, unsigned int nMaxSolutions, bool fAllowLookup, DNSLookupFn dns_lookup_function);

mzumsande commented at 3:51 pm on February 13, 2023: contributor

Concept ACK to fixing this - however, I’m unsure about how the approach fits into the larger architecture:

I think of netbase as a module for mostly context-free basic low-level methods - this PR moves IsReachable to it and saves its state in a global variable, accessing it both from different threads within the node and the GUI. Shouldn’t the information flow between the Node and the GUI rather go through node/interfaces instead of having a shared global variable?

Did you consider the alternative approach of letting banman be agnostic of CJDNS (everything stays IPv6, CJDNS addresses get a /128 netmask just like single IPv6 addresses), and add special “flipping” logic to the points where we need to compare actual addresses from peers with the results from banman? I didn’t try it yet so not sure if viable, but that was my first idea of how to fix the issue of not being able to ban CJDNS peers.

in src/test/fuzz/util/net.cpp:41 in d9b95cb1ee outdated

35@@ -36,7 +36,11 @@ CNetAddr ConsumeNetAddr(FuzzedDataProvider& fuzzed_data_provider) noexcept
36     } else if (network == Network::NET_IPV6) {
37         if (fuzzed_data_provider.remaining_bytes() >= 16) {
38             in6_addr v6_addr = {};
39-            memcpy(v6_addr.s6_addr, fuzzed_data_provider.ConsumeBytes<uint8_t>(16).data(), 16);
40+            auto addr_bytes = fuzzed_data_provider.ConsumeBytes<uint8_t>(16);
41+            if (addr_bytes[0] == 0xFC) { // Avoid generating IPv6 addresses that look like CJDNS.
42+                addr_bytes[0] = 0x55;

brunoerg commented at 5:25 pm on February 13, 2023:

In d9b95cb1eedb9f165df93dcae0aba96ede40767a, I could check that all CJDNS address starts with OxFC, so, it switches the first byte to 0x55 to avoid generating it that look like CJDNS. But why specifically 0x55?

vasild commented at 2:20 pm on February 14, 2023:

There is no reason for 0x55 specifically. I added a comment to clarify:

// Just an arbitrary number, anything != 0xFC would do.

brunoerg commented at 4:41 pm on February 14, 2023:

Got it, thank you!

in src/net_processing.cpp:3638 in 9b656a5eb1 outdated

3634@@ -3635,14 +3635,15 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3635                 continue;
3636             }
3637             ++num_proc;
3638-            bool fReachable = IsReachable(addr);
3639+            const bool reachable = g_reachable_nets.Contains(addr);

brunoerg commented at 5:26 pm on February 13, 2023:

nit:

0            const bool reachable{g_reachable_nets.Contains(addr)};

vasild commented at 2:19 pm on February 14, 2023:

Done.

vasild commented at 5:41 pm on February 13, 2023: contributor

I think of netbase as a module for mostly context-free basic low-level methods - this PR moves IsReachable to it and saves its state in a global variable,

I was thinking about the same, but netbase is already stateful - proxyInfo, nameProxy, nConnectTimeout, fNameLookup, g_socks5_recv_timeout, interruptSocks5Recv are global variables that keep state, which influence the output of the functions defined in netbase, making them context-dependent.

SetProxy(), GetProxy() (already defined in netbase) are very similar to the SetReachable(), IsReachable() functions (which this PR moves into netbase). Thus I think the move is appropriate.

accessing it both from different threads within the node and the GUI. Shouldn’t the information flow between the Node and the GUI rather go through node/interfaces instead of having a shared global variable?

I am not sure IsReachable() fits into that. It is not like that in master where there is the global variable vfLimited[].

Did you consider the alternative approach of letting banman be agnostic of CJDNS (everything stays IPv6, CJDNS addresses get a /128 netmask just like single IPv6 addresses), and add special “flipping” logic to the points where we need to compare actual addresses from peers with the results from banman? I didn’t try it yet so not sure if viable, but that was my first idea of how to fix the issue of not being able to ban CJDNS peers.

banman is using CSubNet::Match(). This would mean in that method we should match IPv6 vs CJDNS. I did not like the idea because CSubNet is used also elsewhere and it could influence other parts of the code. But in general it feels wrong to “pretend” that an address from network X belongs to a subnet from network Y. I even coded that, but eventually decided to avoid it:

 0--- a/src/netaddress.cpp
 1+++ b/src/netaddress.cpp
 2@@ -1039,24 +1039,40 @@ CSubNet::CSubNet(const CNetAddr& addr) : CSubNet()
 3 /**
 4  * [@returns](/bitcoin-bitcoin/contributor/returns/) True if this subnet is valid, the specified address is valid, and
 5  *          the specified address belongs in this subnet.
 6  */
 7 bool CSubNet::Match(const CNetAddr &addr) const
 8 {
 9-    if (!valid || !addr.IsValid() || network.m_net != addr.m_net)
10+    if (!valid || !addr.IsValid()) {
11         return false;
12+    }
13 
14-    switch (network.m_net) {
15+    Network subnet_net = network.m_net;
16+    Network addr_net = addr.m_net;
17+
18+    if ((subnet_net == NET_IPV6 && addr_net == NET_CJDNS) ||
19+        (subnet_net == NET_CJDNS && addr_net == NET_IPV6)) {
20+        // Treat both as CJDNS.
21+        subnet_net = NET_CJDNS;
22+        addr_net = NET_CJDNS;
23+    }
24+
25+    if (subnet_net != addr_net) {
26+        return false;
27+    }
28+
29+    switch (subnet_net) {
30     case NET_IPV4:
31     case NET_IPV6:
32         break;
33     case NET_ONION:
34     case NET_I2P:
35-    case NET_CJDNS:
36     case NET_INTERNAL:
37         return addr == network;
38+    case NET_CJDNS:
39+        return addr.m_addr == network.m_addr;
40     case NET_UNROUTABLE:
41     case NET_MAX:
42         return false;
43     }
44 
45     assert(network.m_addr.size() == addr.m_addr.size());

Note also that the current PR also resolves two other issues, in addition to the banman one - listed as items in the OP.

mzumsande commented at 6:47 pm on February 13, 2023: contributor

I am not sure IsReachable() fits into that. It is not like that in master where there is the global variable vfLimited[].

Yes, within Node it’s still one of several global variables, so this is not a change wrt master. However, vfLimited wasn’t accessed by the GUI because it wasn’t part of LookupSubnet() before, while g_reachable_nets is. So I’m curious if this would work well together with multiprocess (see #10102, fyi @ryanofsky ). I think most of the data in vfLimited is determined during init and does not change later (so it might be initialised at the fork point?) - but torcontrol calling SetReachable() later could be an exception?

vasild force-pushed on Feb 14, 2023

vasild commented at 5:09 pm on February 14, 2023: contributor

9b656a5...7a93d7b: rebase and address suggestions

in src/init.cpp:1313 in 7a93d7b0f9 outdated

1327+        g_reachable_nets.RemoveAll();
1328         for (const std::string& snet : args.GetArgs("-onlynet")) {
1329             enum Network net = ParseNetwork(snet);
1330             if (net == NET_UNROUTABLE)
1331                 return InitError(strprintf(_("Unknown network specified in -onlynet: '%s'"), snet));
1332-            nets.insert(net);

jonatack commented at 8:54 pm on February 14, 2023:

nit, while touching this it may be good to add the missing brackets to if (net == NET_UNROUTABLE), feel free to ignore

vasild commented at 5:06 pm on February 15, 2023:

Ignoring to minimize the size of the diff.

in src/netbase.cpp:820 in 7a93d7b0f9 outdated

812@@ -810,3 +813,13 @@ bool IsBadPort(uint16_t port)
813     }
814     return false;
815 }
816+
817+CService MaybeFlipIPv6toCJDNS(const CService& service)
818+{
819+    CService ret{service};
820+    if (ret.m_net == NET_IPV6 && ret.m_addr[0] == 0xfc && g_reachable_nets.Contains(NET_CJDNS)) {

jonatack commented at 9:04 pm on February 14, 2023:

nit, it may be nice to hoist 0xfc used here and in src/test/fuzz/util/net.cpp#L40-41 to a constexpr/constant, as grepping the codebase for it returns many unrelated false positives.

vasild commented at 5:07 pm on February 15, 2023:

Done.

in src/netbase.h:102 in 7a93d7b0f9 outdated

 99+        LOCK(m_mutex);
100+        return m_reachable.count(net) > 0;
101+    }
102+
103+    bool Contains(const CNetAddr& addr) const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
104+    {

jonatack commented at 9:05 pm on February 14, 2023:

9e4781805b5b59dc

Add AssertLockNotHeld(m_mutex); to the RemoveAll() and to both of the Contains() class methods?

Perhaps also add [[nodiscard]] to the latter two declarations.

vasild commented at 5:13 pm on February 15, 2023:

Added [[nodiscard]] and one AssertLockNotHeld() where it is not immediately followed by LOCK(). I do not see the point in:

0AssertLockNotHeld(m); // the point of this is to crash if m is held
1LOCK(m); // but this will crash if m is held, so what is the point?

If it is really desirable to have AssertLockNotHeld() before every LOCK(), then AssertLockNotHeld() should be put inside LOCK().

This is another story:

0AssertLockNotHeld(m);
1if (x) {
2    LOCK(m);
3    ...
4}

because if it gets executed with x == false and AssertLockNotHeld() is missing then it will not catch the problem (by crashing).

jonatack commented at 5:53 pm on February 15, 2023:

LOCK(m); // but this will crash if m is held, so what is the point?

Ah, I see that double lock detection at runtime was added by you in 95975dd08d8f.

0+++ b/src/netbase.h
1@@ -73,6 +73,7 @@ public:
2     void Add(Network net) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
3     {
4         AssertLockNotHeld(m_mutex);
5+        LOCK(m_mutex);
6         if (net != NET_UNROUTABLE && net != NET_INTERNAL) {
7             LOCK(m_mutex);

02023-02-15T17:48:41Z SetNetworkActive: true
12023-02-15T17:48:41Z DOUBLE LOCK DETECTED
22023-02-15T17:48:41Z Lock order:
32023-02-15T17:48:41Z  (*) 'm_mutex' in netbase.h:76 (in thread 'init')
42023-02-15T17:48:41Z  (*) 'm_mutex' in netbase.h:78 (in thread 'init')
5Assertion failed: detected double lock for 'm_mutex' in netbase.h:78 (in thread 'init'), details in debug log.

jonatack commented at 6:02 pm on February 15, 2023:

If it is really desirable to have AssertLockNotHeld() before every LOCK()

I think the general idea is to have a runtime lock assertion corresponding to every thread safety annotation, where it makes sense (cf the developer notes).

vasild commented at 8:38 am on February 16, 2023:

The double lock detection inside LOCK() is only if DEBUG_LOCKORDER is defined. Edit: and AssertLockNotHeld() is also only being done if DEBUG_LOCKORDER is defined.

vasild commented at 10:55 am on February 17, 2023:

Opened #27116 to amend the docs.

vasild commented at 12:00 pm on August 15, 2023:

Since #27116 is stalled I have added AssertLockNotHeld(), e.g.:

0        AssertLockNotHeld(m_mutex);
1        LOCK(m_mutex);
2        m_reachable.insert(net);

in src/netbase.h:117 in 7a93d7b0f9 outdated

113+        NET_IPV4,
114+        NET_IPV6,
115+        NET_ONION,
116+        NET_I2P,
117+        NET_CJDNS,
118+        NET_INTERNAL

jonatack commented at 9:10 pm on February 14, 2023:

9e4781805b5b59dc As BIP155 networks are added or removed over time, m_reachable could go out of sync with enum Network in netaddress.h … perhaps construct it programmatically from enum Network (now or in a follow-up)?

vasild commented at 11:58 am on August 15, 2023:

There is no built in construct in the language to do that and I do not like NET_MAX which I perceive as a hack.

jonatack commented at 5:44 pm on October 13, 2023:

Yes, I agree with you about NET_MAX but using it to populate m_reachable may still be preferable to having a silent dependency where we need to keep ReachableNets::m_reachable in netbase.h manually in sync with the Network enumerators in netaddress.h. If you prefer the latter approach, perhaps comment next to ReachableNets::m_reachable that it needs to be kept in sync.

vasild commented at 10:16 am on October 16, 2023:

What about something like the following as a followup, aiming to workaround the limitation in C++ in general:

 0namespace Nets
 1{
 2    enum Net : uint8_t {
 3        UNROUTABLE,
 4        IPV4,
 5        IPV6,
 6        ONION,
 7        I2P,
 8        CJDNS,
 9        INTERNAL
10    };
11
12    static constexpr std::array<Net, 7> all{UNROUTABLE, IPV4, IPV6, ONION, I2P, CJDNS, INTERNAL};
13
14    std::string ToString(Net net)
15    {
16        switch (net) {
17        case UNROUTABLE: return "UNROUTABLE";
18        case IPV4: return "IPV4";
19        case IPV6: return "IPV6";
20        case ONION: return "ONION";
21        case I2P: return "I2P";
22        case CJDNS: return "CJDNS";
23        case INTERNAL: return "INTERNAL";
24        }
25    }
26};
27
28...
29
30printf("IPV6 = %d\n", Nets::IPV6);
31
32for (const auto& net: Nets::all) {
33    std::cout << Nets::ToString(net) << std::endl;
34}

jonatack commented at 6:43 pm on October 16, 2023:

I’d review it.

jonatack commented at 9:14 pm on February 14, 2023: member

Approach ACK. (Edit: it looks like the fixed issues could use test coverage, if feasible.)

vasild force-pushed on Feb 15, 2023

vasild commented at 5:05 pm on February 15, 2023: contributor

7a93d7b0f9...8991ed2c6e: address suggestions

in src/netbase.cpp:825 in 8991ed2c6e outdated

820+    if (ret.m_net == NET_IPV6 && ret.m_addr[0] == CJDNS_PREFIX && g_reachable_nets.Contains(NET_CJDNS)) {
821+        ret.m_net = NET_CJDNS;
822+    }
823+    return ret;
824+}
825+

jonatack commented at 6:46 pm on February 15, 2023:

e1910f6743d73a extra newline at EOF

Unrelated, would it be worth avoiding a CService copy in the most frequent case? (maybe returning std::optional<CService> if flipped to CJDNS could be a good idea too)

 0 CService MaybeFlipIPv6toCJDNS(const CService& service)
 1 {
 2+    if (service.m_net != NET_IPV6 || service.m_addr[0] != CJDNS_PREFIX || !g_reachable_nets.Contains(NET_CJDNS)) return service;
 3     CService ret{service};
 4-    if (ret.m_net == NET_IPV6 && ret.m_addr[0] == CJDNS_PREFIX && g_reachable_nets.Contains(NET_CJDNS)) {
 5-        ret.m_net = NET_CJDNS;
 6-    }
 7+    ret.m_net = NET_CJDNS;
 8     return ret;
 9 }
10-

vasild commented at 3:28 pm on February 24, 2023:

I tested this and there is one copy in either case. I guess the compiler is smart enough to avoid unnecessary copies.

vasild commented at 12:49 pm on February 27, 2023:

Also ran both variants in nanobench and there is no difference.

jonatack commented at 7:06 pm on February 15, 2023: member

ACK 8991ed2c6e38a35199725b6296a615733f948f87

vasild commented at 8:29 am on February 16, 2023: contributor

Just thinking aloud:

Subnetting does not make sense for tor/i2p/cjdns
BanMan entries are always subnets (CSubNet objects), even for single host bans

It follows that banman is doing something that does not make sense (e.g. subnetting tor). This is the root of the problem. I think if that is eradicated, then the rest will untangle by itself.

What this means - store two ban maps in BanMan - one for single hosts and one for subnets (only for IPv[46] subnets). This way also lookup will be faster for single hosts - if a hash table is used for the single host bans, then lookup will be O(1). Right now we iterate over all ban entries and check if CSubNet::Match() is true (which is O(number of bans)).

Hmm…

brunoerg commented at 6:34 pm on February 17, 2023: contributor

Subnetting does not make sense for tor/i2p/cjdns It follows that banman is doing something that does not make sense (e.g. subnetting tor). This is the root of the problem. I think if that is eradicated, then the rest will untangle by itself.

CConnman::DisconnectNode also handles addresses as subnets. Is this a problem?

DrahtBot added the label Needs rebase on Feb 17, 2023

vasild commented at 2:30 pm on February 20, 2023: contributor

CConnman::DisconnectNode also handles addresses as subnets. Is this a problem?

Well, it works, but since it does something nonsensical (e.g. subnetting tor) it could be a source of confusion or ultimately, bugs. IMO it shouldn’t be doing that. I think the existence and implementation of CConnman::DisconnectNode(const CSubNet& subnet) is ok - it will/should disconnect any IPv[46] address that matches the given IPv[46] subnet. But this one:

0bool CConnman::DisconnectNode(const CNetAddr& addr)
1{
2    return DisconnectNode(CSubNet(addr));
3}

would better iterate m_nodes and check for equality between the given addr and CNode::addr.

I will try to allow only IPv[46] CSubNets and see if everything blows up in a spectacular way… :bomb: :confetti_ball:

mzumsande commented at 6:46 pm on February 20, 2023: contributor

It follows that banman is doing something that does not make sense (e.g. subnetting tor). This is the root of the problem.

Not sure if I agree that this is the root of the problem - I think conceptually it’s fine to internally treat single addresses as a special case of a subnet (like <ipv4>/32 or <ipv6>/128), and just prevent nonsensical things during user input.

In my opinion, the root of the problem is that fc... addresses are sometimes interpreted as CJDNS, and sometimes as IPv6, depending on startup options. Since large parts of the networking code were designed before CJDNS support was introduced, this makes workarounds like MaybeFlipIPv6toCJDNS() and global availability of reachability necessary.

However, I still feel strongly that having a global, non-constant variable shared by the GUI and node is something we should absolutely avoid. Another possibility to achieve this would be to pass a bool cjdnsreachable as a variable to LookupSubNet() and add IsReachable() to node/interfaces.cpp. With this, there could still be a global g_reachable_nets used for the call sites within node, but the information to the GUI would flow through the interface. A similar approach was chosen for the proxy, which is also state from netbase: The GUI code calls getProxy from node/interfaces, which then calls GetProxy() to retrieve the proxy info from netbase. The GUI doesn’t use GetProxy() directly.

vasild force-pushed on Feb 24, 2023

DrahtBot removed the label Needs rebase on Feb 24, 2023

vasild force-pushed on Feb 24, 2023

vasild commented at 3:20 pm on February 24, 2023: contributor

8991ed2c6e...c2a86ba667: rebase due to conflicts and address suggestion

Invalidates ACK from @jonatack @mzumsande, actually there are two problems - I am talking about Tor subnets and you are talking about a global variable used by the GUI. I will try to mimic the proxy in order to address the latter.

jonatack commented at 4:30 pm on February 24, 2023: member

I will try to mimic the proxy in order to address the latter.

In case you plan to do that, or the Tor subnets issue, in a follow-up:

re-ACK c2a86ba6674008996552830877d66a1bf888fad1 per git range-diff be2e748 8991ed2 c2a86ba6

vasild commented at 1:38 pm on March 6, 2023: contributor

@mzumsande, it is possible to avoid calling LookupSubNet() from the GUI, see https://github.com/bitcoin-core/gui/pull/717. If that gets merged then this PR will be simplified - no need to touch the GUI code from here.

hebasto commented at 1:54 pm on March 9, 2023: member

@mzumsande, it is possible to avoid calling LookupSubNet() from the GUI, see bitcoin-core/gui#717. If that gets merged then this PR will be simplified - no need to touch the GUI code from here.

https://github.com/bitcoin-core/gui/pull/717 has just been merged.

vasild force-pushed on Mar 15, 2023

vasild commented at 5:41 pm on March 15, 2023: contributor

c2a86ba667...fcd4d34b96: rebase even though it is not strictly necessary. Now (after https://github.com/bitcoin-core/gui/pull/717 is merged) the GUI does not call LookupSubNet().

Invalidates ACK from @jonatack

I still think not using CSubNet for Tor, I2P and CJDNS (which are not sub-nettable) makes sense and should be done. That looks like a bigger change and can be done after this PR or independently of it. It would Introduce a class that holds a list of addresses:

 0class NetAddrList
 1{
 2public:
 3    bool Contains(const CNetAddr& addr) const;
 4
 5    void InsertAddr(const CNetAddr& addr);
 6
 7    void InsertSubnet(const CSubNet& subnet); // must be only IPv4 or IPv6
 8
 9private:
10    std::unordered_set<CSubNet> m_subnets;
11    std::unordered_set<CNetAddr> m_addresses;
12}

and use that in some (all?) places where CSubNet is used now. LookupSubNet() should be changed to check if the input contains /, then require it to be IP subnet - that is, return either CSubNet or CNetAddr or maybe have that as a method of the above NetAddrList: void InsertString(const std::string& addr) which would add an entry to m_subnets or m_addresses.

brunoerg commented at 3:00 pm on April 4, 2023: contributor

Concept ACK

ewiseg approved

DrahtBot added the label Needs rebase on May 30, 2023

vasild force-pushed on Jun 8, 2023

vasild commented at 8:11 am on June 8, 2023: contributor

fcd4d34b96...8b495486e2: rebase due to conflicts

DrahtBot removed the label Needs rebase on Jun 8, 2023

in src/netbase.h:91 in 8b495486e2 outdated

89+    }
90+
91+    void RemoveAll() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
92+    {
93+        LOCK(m_mutex);
94+        m_reachable.clear();

nberlin1980 commented at 1:21 pm on July 2, 2023:

This will remove NET_UNROUTABLE and NET_INTERNAL, after which they cannot be re-added. Is that intentional (the function name implies it is intentional)? I ask because the other accessors guard against their removal and insertion.

vasild commented at 11:04 am on August 15, 2023:

Good question. Yes, it should be like this to preserve the behavior from master wrt which networks are reachable if -onlynet is used. The relevant snippet from this PR:

 0     if (args.IsArgSet("-onlynet")) {
 1-        std::set<enum Network> nets;
 2+        g_reachable_nets.RemoveAll();
 3         for (const std::string& snet : args.GetArgs("-onlynet")) {
 4             enum Network net = ParseNetwork(snet);
 5             if (net == NET_UNROUTABLE)
 6                 return InitError(strprintf(_("Unknown network specified in -onlynet: '%s'"), snet));
 7-            nets.insert(net);
 8-        }
 9-        for (int n = 0; n < NET_MAX; n++) {
10-            enum Network net = (enum Network)n;
11-            assert(IsReachable(net));
12-            if (!nets.count(net))
13-                SetReachable(net, false);
14+            g_reachable_nets.Add(net);
15         }
16     }

By default all networks are reachable (in master and in this PR), including the dummy NET_UNROUTABLE and NET_INTERNAL. If -onlynet=X is used (where X can be ipv4, ipv6, tor, i2p, cjdns) then just X is made reachable and others unreachable (NET_UNROUTABLE and NET_INTERNAL also become unreachable).

As for the Add() and Remove() methods - I wanted to preserve the behavior of the old SetReachable():

0-void SetReachable(enum Network net, bool reachable)
1-{
2-    if (net == NET_UNROUTABLE || net == NET_INTERNAL)
3-        return;

but no caller is ever calling SetReachable() with NET_UNROUTABLE or NET_INTERNAL, so I have removed those checks from the new Add() and Remove() methods.

Thanks!

in src/test/net_tests.cpp:760 in 8b495486e2 outdated

788+    BOOST_CHECK(g_reachable_nets.Contains(NET_INTERNAL));
789 
790-    SetReachable(NET_UNROUTABLE, false);
791-    SetReachable(NET_INTERNAL, false);
792+    g_reachable_nets.Remove(NET_UNROUTABLE);
793+    g_reachable_nets.Remove(NET_INTERNAL);

nberlin1980 commented at 1:33 pm on July 2, 2023:

Perhaps add test cases for RemoveAll as it relates to these two.

vasild commented at 11:05 am on August 15, 2023:

Done, thanks for the suggestion!

DrahtBot added the label Needs rebase on Jul 13, 2023

vasild force-pushed on Aug 15, 2023

vasild commented at 11:56 am on August 15, 2023: contributor

8b495486e2...be7ae7b1ec: rebase due to conflicts and address suggestions

vasild commented at 12:02 pm on August 15, 2023: contributor

@mzumsande, what’s your take on this now that it does not touch the GUI?

DrahtBot removed the label Needs rebase on Aug 15, 2023

DrahtBot added the label CI failed on Aug 15, 2023

maflcko commented at 9:26 am on August 17, 2023: member

Could rebase for green CI, if still relevant?

vasild force-pushed on Aug 17, 2023

vasild commented at 9:44 am on August 17, 2023: contributor

be7ae7b1ec...e92b7dee6d: rebase for CI

DrahtBot removed the label CI failed on Aug 17, 2023

in src/net.cpp:332 in f935a5fd97 outdated

323@@ -325,25 +324,6 @@ void RemoveLocal(const CService& addr)
324     mapLocalHost.erase(addr);
325 }
326 
327-void SetReachable(enum Network net, bool reachable)
328-{
329-    if (net == NET_UNROUTABLE || net == NET_INTERNAL)

mzumsande commented at 3:20 pm on August 25, 2023:

Not having this in ReachableNets means that SetReachable will no longer be ignored for these two networks, which is a small change in behavior (at least in theory, it seeems impossible that this function would be called with NET_UNROUTABLE or NET_INTERNAL outside of tests). That seems fine to me, but it could be mentioned in the commit message. [Edit]: Now I see this has been discussed above. Could still be mentioned in the commit message.

vasild commented at 2:58 pm on August 28, 2023:

Done

mzumsande commented at 5:50 pm on August 25, 2023: contributor

@mzumsande, what’s your take on this now that it does not touch the GUI?

Sorry, I had forgotten about this PR. Looks fine to me now approach-wise. I will take a deeper look next week and try to test banning / whitelisting cjdns addresses.

vasild force-pushed on Aug 28, 2023

vasild commented at 2:57 pm on August 28, 2023: contributor

e92b7dee6d...6ff2241e47: extend a commit message, see #27071 (review)

in src/test/net_tests.cpp:767 in 692aa7ed24 outdated

799+
800+    g_reachable_nets.Add(NET_IPV4);
801+    g_reachable_nets.Add(NET_IPV6);
802+    g_reachable_nets.Add(NET_ONION);
803+    g_reachable_nets.Add(NET_I2P);
804+    g_reachable_nets.Add(NET_CJDNS);

mzumsande commented at 4:00 pm on September 13, 2023:

Now we could add NET_UNROUTABLE and NET_INTERNAL back at the end (so that this unit test doesn’t change global state).

vasild commented at 1:16 pm on October 5, 2023:

Done, thanks!

mzumsande commented at 8:47 pm on September 13, 2023: contributor

Tested ACK 6ff2241e4757ed2f26244f266a44982a04029815

By having two nodes connect to each other I verified that the following operations now work for CJDNS peers

whitelisting
banning, also over restarts
unbanning via GUI

One detail: If we ban a cjdns address or subnet while not running with -cjdnsreachable and then restart with -cjdnsreachable, the entries will be deleted. That is probably the correct behavior, because banning them in the first place didn’t make any sense.

DrahtBot requested review from jonatack on Sep 13, 2023

DrahtBot added the label Needs rebase on Sep 21, 2023

net: put CJDNS prefix byte in a constant 64d6f77907

fuzz: ConsumeNetAddr(): avoid IPv6 addresses that look like CJDNS

The fuzz testing framework runs as if `-cjdnsreachable` is set and in
this case addresses like `{net=IPv6, addr=fc...}` are not possible.

c42ded3d9b

net: move IsReachable() code to netbase and encapsulate it

`vfLimited`, `IsReachable()`, `SetReachable()` need not be in the `net`
module. Move them to `netbase` because they will be needed in
`LookupSubNet()` to possibly flip the result to CJDNS (if that network
is reachable).

In the process, encapsulate them in a class.

`NET_UNROUTABLE` and `NET_INTERNAL` are no longer ignored when adding
or removing reachable networks. This was unnecessary.

6e308651c4

net: move MaybeFlipIPv6toCJDNS() from net to netbase

It need not be in the `net` module and we need to call it from
`LookupSubNet()`, thus move it to `netbase`.

53afa68026

vasild force-pushed on Oct 5, 2023

vasild commented at 1:15 pm on October 5, 2023: contributor

6ff2241e47...2f3f793605: rebase due to conflicts and address suggestion

DrahtBot removed the label Needs rebase on Oct 5, 2023

achow101 added this to the milestone 26.0 on Oct 12, 2023

in src/rpc/net.cpp:733 in 2f3f793605 outdated

729@@ -730,7 +730,7 @@ static RPCHelpMan setban()
730     if (!isSubnet) {
731         const std::optional<CNetAddr> addr{LookupHost(request.params[0].get_str(), false)};
732         if (addr.has_value()) {
733-            netAddr = addr.value();
734+            netAddr = MaybeFlipIPv6toCJDNS(CService{addr.value(), /*port=*/0});

jonatack commented at 6:03 pm on October 13, 2023:

0994d45 unsure, would it be clearer to be explicit about this conversion?

0            netAddr = static_cast<CNetAddr>(MaybeFlipIPv6toCJDNS(CService{addr.value(), /*port=*/0}));

vasild commented at 11:01 am on October 16, 2023:

Done.

in src/netbase.cpp:659 in 2f3f793605 outdated

652@@ -651,9 +653,10 @@ bool LookupSubNet(const std::string& subnet_str, CSubNet& subnet_out)
653 
654     const size_t slash_pos{subnet_str.find_last_of('/')};
655     const std::string str_addr{subnet_str.substr(0, slash_pos)};
656-    const std::optional<CNetAddr> addr{LookupHost(str_addr, /*fAllowLookup=*/false)};
657+    std::optional<CNetAddr> addr{LookupHost(str_addr, /*fAllowLookup=*/false)};
658 
659     if (addr.has_value()) {
660+        addr = MaybeFlipIPv6toCJDNS(CService{addr.value(), /*port=*/0});

jonatack commented at 6:03 pm on October 13, 2023:

0994d45 unsure, would it be clearer to be explicit about this conversion?

0        addr = static_cast<CNetAddr>(MaybeFlipIPv6toCJDNS(CService{addr.value(), /*port=*/0}));

vasild commented at 11:01 am on October 16, 2023:

Done.

jonatack commented at 6:06 pm on October 13, 2023: member

ACK 2f3f7936

In 0994d45 the commit message might be out of date regarding unbanSelectedNode after the merge of https://github.com/bitcoin-core/gui/pull/717.

A couple questions and a comment, feel free to ignore

DrahtBot requested review from brunoerg on Oct 13, 2023

DrahtBot requested review from mzumsande on Oct 13, 2023

netbase: possibly change the result of LookupSubNet() to CJDNS

All callers of `LookupSubNet()` need the result to be of CJDNS type if
`-cjdnsreachable` is set and the address begins with `fc`:

* `NetWhitelistPermissions::TryParse()`: otherwise `-whitelist=` fails
  to white list CJDNS addresses: when a CJDNS peer connects to us, it
  will be matched against IPv6 `fc...` subnet and the match will never
  succeed.

* `BanMapFromJson()`: CJDNS bans are stored as just IPv6 addresses in
  `banlist.json`. Upon reading from disk they have to be converted back
  to CJDNS, otherwise, after restart, a ban entry like (`fc00::1`, IPv6)
  would not match a peer (`fc00::1`, CJDNS).

* `setban()` (in `rpc/net.cpp`): otherwise `setban fc.../mask add` would
  add an IPv6 entry to BanMan. Subnetting does not make sense for CJDNS
  addresses, thus treat `fc.../mask` as invalid `CSubNet`. The result of
  `LookupHost()` has to be converted for the case of banning a single
  host.

* `InitHTTPAllowList()`: not necessary since before this change
  `-rpcallowip=fc...` would match IPv6 subnets against IPv6 peers even
  if they started with `fc`. But because it is necessary for the above,
  `HTTPRequest::GetPeer()` also has to be adjusted to return CJDNS peer,
  so that now CJDNS peers are matched against CJDNS subnets.

9482cb780f

net: remove unused CConnman::FindNode(const CSubNet&) 0e6f6ebc06

vasild force-pushed on Oct 16, 2023

vasild commented at 11:01 am on October 16, 2023: contributor

2f3f793605...0e6f6ebc06: address suggestions, thanks!

In https://github.com/bitcoin/bitcoin/commit/0994d4573b53b2763cba83d057b38f221486a983 the commit message might be out of date…

Updated!

DrahtBot added the label CI failed on Oct 16, 2023

DrahtBot removed the label CI failed on Oct 16, 2023

mzumsande commented at 6:38 pm on October 16, 2023: contributor

re-ACK 0e6f6ebc064c5fb425fc3699efe760ec6cd4b6af

only rebase / minor changes since last ACK

DrahtBot requested review from jonatack on Oct 16, 2023

jonatack commented at 6:50 pm on October 16, 2023: member

Code review ACK 0e6f6ebc064c5fb425fc3699efe760ec6cd4b6af

DrahtBot removed review request from jonatack on Oct 16, 2023

achow101 commented at 4:46 pm on October 19, 2023: member

ACK 0e6f6ebc064c5fb425fc3699efe760ec6cd4b6af

achow101 merged this on Oct 19, 2023

achow101 closed this on Oct 19, 2023

Frank-GER referenced this in commit 59e39fd22d on Oct 21, 2023

maflcko commented at 4:39 pm on October 22, 2023: member

This crashes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=63434

0echo '////ICAgICAgICD/ICAgIAYgICAgeXn/ICAgIAAAAIAgIP8gICA=' | base64 --decode > /tmp/minimized-banman-crash
1FUZZ=banman ./src/test/fuzz/fuzz /tmp/minimized-banman-crash
2
3
4fuzz: test/fuzz/banman.cpp:112: void banman_fuzz_target(FuzzBufferType): Assertion `banmap == banmap_read' failed.

09482cb780fe04c1f1d9050edd1b8e549e52c86ce is the first bad commit
1commit 9482cb780fe04c1f1d9050edd1b8e549e52c86ce
2Author: Vasil Dimov <vd@FreeBSD.org>
3Date:   Tue Feb 7 15:16:57 2023 +0100
4
5    netbase: possibly change the result of LookupSubNet() to CJDNS

vasild deleted the branch on Oct 23, 2023

bitcoin locked this on Oct 22, 2024

Handle CJDNS from LookupSubNet() #27071

Code Coverage

Reviews

Conflicts