net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full

pinheadmz commented at 7:59 pm on May 8, 2023: member

Use case: I run a full node that accepts inbound connections and have a whitebind setting so my personal light client can always connect, even when maxconnections (and particularly all inbound slots) is already full.

Currently when connections are full, if we receive an inbound peer request, we look for a current connection to evict so the new peer can have a slot. To find an evict-able peer we go through all our peers and “protect” multiple categories of peers, then we evict the “worst” peer that is left unprotected. If there are no peers left to evict, the inbound connection is denied.

With this PR, if the inbound connection has forceinbound permission we start the eviction process by first protecting all noban and outbound connections, then selecting one of the remaining peers (if any) at random. Then we loop through all our current connections, removing protected peers from the evict-able list. If we end up protecting all our remaining connections, the randomly chosen peer is evicted.

forceinbound implies noban permission.

All outbound and noban connections remain protected from eviction.

DrahtBot commented at 7:59 pm on May 8, 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
Concept ACK	mzumsande
Stale ACK	LarryRuane, stickies-v, willcl-ark

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:

#28463 (p2p: Increase inbound capacity for block-relay only connections by mzumsande)
#27581 (net: Continuous ASMap health check by fjahr)
#27114 (p2p: Allow whitelisting outgoing connections by brunoerg)

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.

pinheadmz renamed this:
~~Make peer eviction slightly more aggresive to make room for whitelisted inbound connections~~
Allow inbound whitebind connections to more aggresivey evict peers when slots are full
on May 8, 2023

DrahtBot renamed this:
~~Allow inbound whitebind connections to more aggresivey evict peers when slots are full~~
net: Allow inbound whitebind connections to more aggresivey evict peers when slots are full
on May 9, 2023

DrahtBot added the label P2P on May 9, 2023

in src/node/eviction.cpp:234 in e71d495ffb outdated

240+    if (vEvictionCandidates.empty()) {
241+        if (force) {
242+            return last_out.id;
243+        } else {
244+            return std::nullopt;
245+        }

stickies-v commented at 9:34 pm on May 11, 2023:

nit

0        return force ? std::make_optional(last_out.id) : std::nullopt;

jonatack commented at 10:01 pm on May 11, 2023:

Not sure, but IIRC last time I looked RVO wasn’t working with a ternary.

stickies-v commented at 2:42 pm on May 16, 2023:

Not really familiar with this stuff, but that seems to be an MSVC bug? https://stackoverflow.com/questions/55152552/visual-studio-not-performing-rvo-when-ternary-operator-is-used-and-move-copy-cto

We do ternary operators in plenty of places, don’t think we need to workaround this?

LarryRuane commented at 10:48 pm on May 16, 2023:

Does RVO even apply here? I thought it applies to complex structures, a large vector being the canonical example, where we don’t want to copy every element. But here we’re just returning a NodeId which is a int64_t, wrapped by std::optional. I don’t think there’s any savings by moving instead of copying one of these.

stickies-v commented at 6:13 pm on May 17, 2023:

Oh, good point @LarryRuane. I didn’t realize NodeId was just a typedef for int64_t. I think that entirely resolves it indeed.

stickies-v commented at 9:58 pm on May 11, 2023: contributor

Concept ACK, makes sense to prioritize whitelisted peers. Will review more in-depth soon.

pinheadmz renamed this:
~~net: Allow inbound whitebind connections to more aggresivey evict peers when slots are full~~
net: Allow inbound whitebind connections to more aggressively evict peers when slots are full
on May 12, 2023

in src/node/eviction.cpp:87 in e71d495ffb outdated

86     std::sort(elements.begin(), elements.end(), comparator);
87     size_t eraseSize = std::min(k, elements.size());
88+
89+    // Return the last erased element
90+    std::optional<NodeEvictionCandidate> last_out{std::nullopt};
91+    if (eraseSize > 0) last_out = elements.at(elements.size() - eraseSize);

stickies-v commented at 2:46 pm on May 15, 2023:

I’m not sure this is correct, it ignores the checking of predicate?

in src/node/eviction.cpp:205 in e71d495ffb outdated

203+
204     // Deterministically select 4 peers to protect by netgroup.
205     // An attacker cannot predict which netgroups will be protected
206-    EraseLastKElements(vEvictionCandidates, CompareNetGroupKeyed, 4);
207+    auto last = EraseLastKElements(vEvictionCandidates, CompareNetGroupKeyed, 4);
208+    if (last.has_value()) last_out = last.value();

stickies-v commented at 9:01 am on May 16, 2023:

An alternative approach to the duplication of these lines is to just store all the return values in a vector, like:

0std::vector<std::optional<NodeEvictionCandidate>> removed;
1...
2removed.emplace_back(EraseLastKElements(vEvictionCandidates, CompareNetGroupKeyed, 4));

and then just return the last one at the end?

in src/node/eviction.cpp:78 in e71d495ffb outdated

77-template <typename T, typename Comparator>
78-static void EraseLastKElements(
79-    std::vector<T>& elements, Comparator comparator, size_t k,
80+//! Sort an array of NodeEvictionCandidates by the specified comparator, then erase the last K elements where predicate is true.
81+//! Return the last element removed if any.
82+static std::optional<NodeEvictionCandidate> EraseLastKElements(

stickies-v commented at 9:07 am on May 16, 2023:

Returning just the last removed element feels like a very bespoke and perhaps unintuitive interface to me. I know we don’t currently need it, but perhaps it’s worth generalizing this a bit more and returning all the removed elements instead of just the last one? For the callsite, it’s trivial to keep just the last one? Just thinking out loud, perhaps this is difficult to do without an unacceptable overhead in terms of allocations etc.

in src/net.cpp:1689 in e71d495ffb outdated

924@@ -925,7 +925,7 @@ bool CConnman::AttemptToEvictConnection()
925             vEvictionCandidates.push_back(candidate);
926         }
927     }
928-    const std::optional<NodeId> node_id_to_evict = SelectNodeToEvict(std::move(vEvictionCandidates));
929+    const std::optional<NodeId> node_id_to_evict = SelectNodeToEvict(std::move(vEvictionCandidates), force);

stickies-v commented at 9:09 am on May 16, 2023:

nit (and in quite a few other places):

0    const std::optional<NodeId> node_id_to_evict{SelectNodeToEvict(std::move(vEvictionCandidates), force)};

in src/node/eviction.cpp:231 in e71d495ffb outdated

237+    last = ProtectEvictionCandidatesByRatio(vEvictionCandidates);
238+    if (last.has_value()) last_out = last.value();
239+
240+    if (vEvictionCandidates.empty()) {
241+        if (force) {
242+            return last_out.id;

stickies-v commented at 9:14 am on May 16, 2023:

All the last_out assignments are behind an if condition, I think this theoretically can be a zero-initialized NodeEvictionCandidate?

in src/net.h:1349 in e71d495ffb outdated

981@@ -982,7 +982,7 @@ class CConnman
982      */
983     bool AlreadyConnectedToAddress(const CAddress& addr);
984 
985-    bool AttemptToEvictConnection();
986+    bool AttemptToEvictConnection(bool force);

stickies-v commented at 9:40 am on May 16, 2023:

I think it would be nice to document the effect of force both here and for SelectNodeToEvict

in src/node/eviction.cpp:85 in e71d495ffb outdated

84     std::function<bool(const NodeEvictionCandidate&)> predicate = [](const NodeEvictionCandidate& n) { return true; })
85 {
86     std::sort(elements.begin(), elements.end(), comparator);
87     size_t eraseSize = std::min(k, elements.size());
88+
89+    // Return the last erased element

LarryRuane commented at 2:52 pm on May 17, 2023:

0    // Return the last erased (not-to-be-evicted) element

mzumsande commented at 6:44 pm on May 17, 2023: contributor

Concept ACK

As I suggested in the review club, an alternative, more simple approach would be to just pick a random peer after removing NoBan and outbound connections when in force-mode. Then, if at the end of the usual eviction algorithm, we don’t have a evict-able peer, we would evict the random one instead. This would make the code simpler (no need to change EraseLastKElements or keep track of last), and I don’t really see a major downside:

The EraseLastKElements eviction criteria don’t really seem to be sorted by priority anyway right now (because order doesn’t matter so far), so evicting a peer that would’ve been protected by later calls doesn’t seem better than evicting one that would’ve been protected by earlier ones.
Whitebind No-Ban peers are more trusted to begin with, but if they maliciously wanted to take over all inbound slots by repeatedly connecting to us on the whitebind address, they can do that anyway, whether the eviction is random or whether the current approach of the PR is used. So it’s not clear to me what the extra benefit of the current approach over random eviction is.

pinheadmz force-pushed on May 17, 2023

pinheadmz commented at 8:18 pm on May 17, 2023: member

@stickies-v @mzumsande thanks for your review and feedback, I refactored the PR so we evict a random peer when forced if all other peers are protected. It is MUCH simpler ;-)

in src/node/eviction.cpp:189 in c826187b50 outdated

185 
186     ProtectOutboundConnections(vEvictionCandidates);
187 
188+    // Hang on to one random node to evict if forced
189+    std::optional<NodeId> force_evict;
190+    if (vEvictionCandidates.size() > 0) {

stickies-v commented at 8:34 pm on May 17, 2023:

This block can be skipped if !force

0    if (vEvictionCandidates.size() > 0 && force) {

in src/node/eviction.cpp:191 in c826187b50 outdated

187 
188+    // Hang on to one random node to evict if forced
189+    std::optional<NodeId> force_evict;
190+    if (vEvictionCandidates.size() > 0) {
191+        FastRandomContext rng;
192+        size_t randpos = rng.randrange(vEvictionCandidates.size());

stickies-v commented at 8:35 pm on May 17, 2023:

nit:

0        size_t randpos{FastRandomContext().randrange(vEvictionCandidates.size())};

in src/node/eviction.cpp:217 in c826187b50 outdated

212@@ -204,7 +213,8 @@ void ProtectEvictionCandidatesByRatio(std::vector<NodeEvictionCandidate>& evicti
213     // or disadvantaged characteristics.
214     ProtectEvictionCandidatesByRatio(vEvictionCandidates);
215 
216-    if (vEvictionCandidates.empty()) return std::nullopt;
217+    if (vEvictionCandidates.empty())
218+        return force ? force_evict : std::nullopt;

stickies-v commented at 8:41 pm on May 17, 2023:

Since force_evict is already an optional defaulting to nullopt, this can be simplified in conjunction with my other suggestion.

0    if (vEvictionCandidates.empty()) return force_evict;

in src/node/eviction.h:46 in c826187b50 outdated

38@@ -39,7 +39,7 @@ struct NodeEvictionCandidate {
39  * ratios of desirable or disadvantaged peers. If any eviction candidates
40  * remain, the selection logic chooses a peer to evict.
41  */
42-[[nodiscard]] std::optional<NodeId> SelectNodeToEvict(std::vector<NodeEvictionCandidate>&& vEvictionCandidates);
43+[[nodiscard]] std::optional<NodeId> SelectNodeToEvict(std::vector<NodeEvictionCandidate>&& vEvictionCandidates, bool force = false);

stickies-v commented at 8:42 pm on May 17, 2023:

Documenting the force parameter (or linking to AttemptToEvictConnection) would be helpful I think.

in src/net.h:986 in c826187b50 outdated

981@@ -982,7 +982,13 @@ class CConnman
982      */
983     bool AlreadyConnectedToAddress(const CAddress& addr);
984 
985-    bool AttemptToEvictConnection();
986+    /**
987+     * Attempt to find a connected node to disconnect.

stickies-v commented at 8:48 pm on May 17, 2023:

Imo, “find” makes it sound like this has no side effects, whereas I think this actually does disconnect the node.

mzumsande commented at 9:15 pm on May 17, 2023:

Maybe just move the doc from net.cpp? That’s basically what I did in a WIP branch.

stickies-v commented at 10:31 am on May 18, 2023:

I’m not sure that’s a strict improvement? Documenting the interface (i.e. pretty much just the first line of what’s in net.cpp) seems more appropriate to be in the header indeed, but all the rest sounds more like implementation details that may best be kept close to the source code?

Also, my main issue is with (having just) the word “find”, which to me sounds like there are no side effects.

in src/net.h:988 in c826187b50 outdated

981@@ -982,7 +982,13 @@ class CConnman
982      */
983     bool AlreadyConnectedToAddress(const CAddress& addr);
984 
985-    bool AttemptToEvictConnection();
986+    /**
987+     * Attempt to find a connected node to disconnect.
988+     * Used to make room for new inbound connections, returns true if successful.
989+     * @param[in] force     Try to evict a random node if all connections are otherwise protected.

stickies-v commented at 8:50 pm on May 17, 2023:

suggested rephrasing (although I’m not sure about the “NoBan” qualifier, maybe we have a better established term?)

0     * [@param](/bitcoin-bitcoin/contributor/param/)[in] force     If all connections are otherwise protected, still evict a random inbound NoBan node if it exists

stickies-v commented at 8:57 pm on May 17, 2023: contributor

Light approach ACK c826187b5070ce89edcde0536183714a1c2e3207

I think this random selection approach suggested by mzumsande is a more elegant approach. “Light” in approach ACK because I need to build more confidence that this doesn’t introduce new attack vectors.

pinheadmz force-pushed on May 18, 2023

pinheadmz commented at 2:19 pm on May 18, 2023: member

Thanks @stickies-v nits addressed! 🙏

pinheadmz force-pushed on May 18, 2023

DrahtBot added the label CI failed on May 18, 2023

in src/node/eviction.cpp:189 in 96b513f605 outdated

185 
186     ProtectOutboundConnections(vEvictionCandidates);
187 
188+    // Hang on to one random node to evict if forced
189+    std::optional<NodeId> force_evict;
190+    if (vEvictionCandidates.size() > 0 && force) {

stickies-v commented at 2:54 pm on May 22, 2023:

nit: if vEvictionCandidates is empty, there’s no point (I think, couldn’t see any side effects in EraseLastKElements or ProtectEvictionCandidatesByRatio) executing the next lines so might as well return early? Unless you think this makes the code less clear?

0    if (vEvictionCandidates.empty()) return std::nullopt;
1    std::optional<NodeId> force_evict;
2    if (force) {

pinheadmz commented at 2:32 pm on May 24, 2023:

Good point, although “if empty, return now” logic would make sense after each step in this function (why bother calling EraseLastKElements() with an empty array?) even without this PR. One thing we could do is add that logic to the beginning of EraseLastKElements() itself, but I dunno how much time is really wasted in there (sort, min, erase … ?)

stickies-v commented at 2:52 pm on May 24, 2023:

I had similar considerations to e.g. wrap the EraseLastKElements call in a lambda fn, but it’s probably not worth the LoC change.

I think this case is slightly different in that it better highlights that if there are no inbound NoBan peers, the result is always std::nullopt.

pinheadmz commented at 2:55 pm on May 24, 2023:

good point, i’ll add it. one line i think in this case will improve readability

in test/functional/p2p_eviction.py:127 in 96b513f605 outdated

121@@ -122,6 +122,36 @@ def run_test(self):
122         self.log.debug("{} protected peers: {}".format(len(protected_peers), protected_peers))
123         assert evicted_peers[0] not in protected_peers
124 
125+        self.log.info("Test that whitebind inbounds get extra eviction power")
126+        # Only allow 1 inbound connection, but set whitebind
127+        self.restart_node(0, extra_args=['-maxconnections=12', '-whitebind=0.0.0.0:30201'])

stickies-v commented at 6:09 pm on May 22, 2023:

As you mentioned in the review club, whitelisting just based on ports enables an attacker that discovered which port(s) are whitelisted to take over all (non-whitelisted) inbound connections slots.

Therefore, I think we have to be careful not to accidentally lead people to this footgun. Binding to 127.0.0.1 seems much more prudent to not set a bad example.

0        self.restart_node(0, extra_args=['-maxconnections=12', '-whitebind=127.0.0.1:30201'])

pinheadmz commented at 5:18 pm on May 24, 2023:

updated test and also included warning in release note

stickies-v commented at 6:19 pm on May 22, 2023: contributor

Code review ACK 96b513f605fb2df441b66da583056b1c8acd4dbc, but I think the first commit should be removed from this PR, I think it’s an accident?

I think the code is good and suits the intent of the PR well. My only concern is about the potential footgun introduced, even if it is relatively mild. Anyone running bitcoind with whitebind=0.0.0.0:<port> will now be vulnerable to having all their inbound non-whitelisted slots taken over by an attacker that has figured out what the whitelisted port is. It doesn’t seem like a huge concern, given that:

it only affects inbound connections, which we already assume to be more susceptible to these kinds of attacks, and
it does require the user to manually whitebind to 0.0.0.0

but I just wanted to flag it here anyway in case it is actually more serious than I see it to be.

Also, this seems like behaviour change that would benefit from a mention in the release notes?

pinheadmz force-pushed on May 24, 2023

pinheadmz referenced this in commit fa78fc543c on May 24, 2023

pinheadmz requested review from stickies-v on May 24, 2023

in doc/release-notes-27600.md:8 in fa78fc543c outdated

0@@ -0,0 +1,10 @@
1+P2P
2+---
3+
4+- Inbound connections from hosts protected by `whitebind` or `whitelist`
5+  permissions will now be more likely to connect even if `maxconnections` is
6+  reached and the node's inbound slots are full. Because this is achieved by
7+  more aggressively finding a current connection to evict, users should take
8+  care only to set these permissions on local ports that attackers can not

stickies-v commented at 9:52 pm on May 24, 2023:

I think “on local ports” is unnecessarily restrictive/scary. I think it’s still completely fine to whitelist remote addresses, you mostly just want to avoid ranges, right?

pinheadmz commented at 1:51 pm on May 25, 2023:

@mzumsande what’s your opinion on this? Is this PR a big enough change to warrant this kind of warning?

mzumsande commented at 2:47 pm on May 30, 2023:

Not really sure, I’ve never looked into these options much and don’t know about best practices - I think that if you use -whitebind with non-local addresses, you’d at least have to make sure that that address is not self-advertised. I guess that this applies more to -whitebind than -whitelist, so that the preferred approach in case of a non-local connection would be to use -whitelist? fyi @vasild, do you have an opinion on this?

pinheadmz commented at 3:06 pm on May 30, 2023:

That’s a good point, whitelist is harder to attack than whitebind because the attacker would have to spoof their origin IP repeatedly to fill up your inbounds. If a user whitebind’s a port and an attacker figures out that port numnber, they can trivially evict all your other inbounds

vasild commented at 3:18 pm on June 1, 2023:

I agree with what you say above - whitebind on publicly accessible address:port with this new permission sounds bad. Similar with whitelist - if a range is used.

This PR currently expands the semantic of the noban permission. This will affect existent setups that already use it. Would it make sense to introduce a new permisson, separate from noban? I mean - now if somebody is running -whitebind=noban@publicaddr:port then a bad actor could cause harm on master, but even more harm with this PR.

pinheadmz commented at 4:35 pm on June 2, 2023:

interesting idea, so we could add NetPermissionFlags::NoBanForce or something. The danger will still be present for users of the permission, but existing users of NoBan wouldn’t have to worry. And since NoBan is a default of whitebind it’ll require more user attention anyway to use the more dangerous option.

In that case, the release note should still warn the user about using NoBanForce (or whatever we call it) but that warning can just be general, like, keep an eye on your netinfo if you do this. As opposed to just recommending only setting local addresses with whitebind

LarryRuane commented at 5:47 pm on June 10, 2023:

@vasild

This PR currently expands the semantic of the noban permission

Could you please elaborate on this point? I thought that with this PR, noban nodes are still completely protected (they are removed from the eviction candidates list before the random node is chosen). But it’s certain that I’m missing something. Thanks.

pinheadmz commented at 3:41 pm on June 12, 2023:

@LarryRuane you are correct but what we are changing (in the current state of this branch) is that noban nodes can now force disconnection of other peers. Since many users may already have noban set, maybe even for a large range of IPs, this branch would introduce a new vulnerability they may not be aware of. Because of that I think it does make more sense to specify a new permission so users can narrow the attack surface

DrahtBot removed the label CI failed on May 25, 2023

LarryRuane commented at 4:52 pm on June 10, 2023: contributor

Please consider editing the description slightly, if my understanding is correct.

With this PR, if the inbound connection is on our whitelist we start the eviction process by selecting a random unprotected peer.

At this point in the process, where we choose a random peer, we’re considering all (inbound, not noban) peers, is that correct? They are all (other than outbound or noban) “unprotected” because we haven’t protected any of them yet. So maybe this should say, “… selecting a random peer” or “… selecting a random inbound peer” – because we may randomly choose a peer that ends up being protected, or not protected. That would make this PR easier for me to understand.

I do think the updated algorithm is very elegant (great suggestion, @mzumsande). It chooses a random peer, which may end up being erased from the eviction candidate list, or may not, but we don’t care either way; we evict it if we need to, all the better if it actually did not get protected (but, again, we don’t care).

By the way, I’m going to highlight this PR in the Optech newsletter next week, that’s why I’m particularly interested in making sure I understand it.

in src/node/eviction.cpp:218 in fa78fc543c outdated

214@@ -204,7 +215,7 @@ void ProtectEvictionCandidatesByRatio(std::vector<NodeEvictionCandidate>& evicti
215     // or disadvantaged characteristics.
216     ProtectEvictionCandidatesByRatio(vEvictionCandidates);
217 
218-    if (vEvictionCandidates.empty()) return std::nullopt;
219+    if (vEvictionCandidates.empty()) return force_evict;

LarryRuane commented at 5:53 pm on June 10, 2023:

Perhaps insert a comment before this line // This may still return std::nullopt

pinheadmz commented at 3:52 pm on June 12, 2023:

sure!

naumenkogs commented at 7:42 am on November 9, 2023:

I’d suggest expanding this comment to explain what exactly happened.

in src/node/eviction.h:46 in fa78fc543c outdated

37@@ -38,8 +38,10 @@ struct NodeEvictionCandidate {
38  * fixed numbers of desirable peers per various criteria, followed by (mostly)
39  * ratios of desirable or disadvantaged peers. If any eviction candidates
40  * remain, the selection logic chooses a peer to evict.
41+ * @param[in]   force   Attempt to evict a random peer if no candidates
42+ *                      are available. (see CConman::AttemptToEvictConnection())
43  */
44-[[nodiscard]] std::optional<NodeId> SelectNodeToEvict(std::vector<NodeEvictionCandidate>&& vEvictionCandidates);
45+[[nodiscard]] std::optional<NodeId> SelectNodeToEvict(std::vector<NodeEvictionCandidate>&& vEvictionCandidates, bool force = false);

LarryRuane commented at 6:17 pm on June 10, 2023:

I’m not a fan of default arguments; consider making this not have a default. It makes sense if there are many existing calls to a function and you don’t want to touch them all, but there’s only one call to this function in production code. You would have to touch about 6 call sites in test code, but that’s not too bad. The reason I’m not a fan of default arguments is that they hide something that may be helpful to see when just looking at the call site. (You might think, “Oh, it’s possible that eviction can be ‘forced’, what does that mean?) But this is an optional suggestion, fine if you leave it as-is, just something to consider.

pinheadmz commented at 3:52 pm on June 12, 2023:

ok I like this, will do

naumenkogs commented at 7:43 am on November 9, 2023:

This still remains default, no?

LarryRuane commented at 6:20 pm on June 10, 2023: contributor

utACK fa78fc543cd46ee1c7181ecf6b696c2892b9f8d3 except for possible attack vectors (for example, NoBanForce) that I’m not really qualified to comment on. I may try to contribute to that discussion after I’ve learned more. Nice PR!

DrahtBot requested review from stickies-v on Jun 10, 2023

LarryRuane commented at 8:00 pm on June 10, 2023: contributor

Even though not needed for this PR, you may want to consider adding a commit to remove the unnecessary templating. It’s perfectly reasonable not to make this change in this PR, but I thought I’d document it just in case; I do think it makes the code easier to understand.

 0--- a/src/node/eviction.cpp
 1+++ b/src/node/eviction.cpp
 2@@ -74,9 +74,10 @@ struct CompareNodeNetworkTime {
 3 };
 4 
 5 //! Sort an array by the specified comparator, then erase the last K elements where predicate is true.
 6-template <typename T, typename Comparator>
 7 static void EraseLastKElements(
 8-    std::vector<T>& elements, Comparator comparator, size_t k,
 9+    std::vector<NodeEvictionCandidate>& elements,
10+    std::function<bool(const NodeEvictionCandidate&, const NodeEvictionCandidate&)> comparator,
11+    size_t k,
12     std::function<bool(const NodeEvictionCandidate&)> predicate = [](const NodeEvictionCandidate& n) { return true; })
13 {
14     std::sort(elements.begin(), elements.end(), comparator);

pinheadmz commented at 8:57 pm on June 10, 2023: member

@LarryRuane thanks for reviewing. I actually had removed the templating in the very first version of this branch but after changing the approach I just left it alone. The function is written like a generic utility function, even though we don’t currently use it for any other vectors. So I think the function could go as written into a util.cpp or something, in a future PR ?

pinheadmz commented at 4:05 pm on June 12, 2023: member

@LarryRuane updated PR description to clarify where we get the random peer, added comment about the possible nullopt return value, and removed the default force value in SelectNodeToEvict()

pinheadmz referenced this in commit 468df13aa5 on Jun 12, 2023

pinheadmz force-pushed on Jun 12, 2023

pinheadmz referenced this in commit 09dffe5594 on Jun 12, 2023

pinheadmz referenced this in commit f21452378c on Jun 12, 2023

pinheadmz force-pushed on Jun 12, 2023

pinheadmz commented at 5:11 pm on June 12, 2023: member

@vasild @mzumsande Added a commit that restricts the aggressive eviction to new net permission ForceInbound which leaves NoBan unchanged.

in src/test/fuzz/node_eviction.cpp:43 in 2a6a981e4d outdated

39@@ -40,7 +40,7 @@ FUZZ_TARGET(node_eviction)
40     // Make a copy since eviction_candidates may be in some valid but otherwise
41     // indeterminate state after the SelectNodeToEvict(&&) call.
42     const std::vector<NodeEvictionCandidate> eviction_candidates_copy = eviction_candidates;
43-    const std::optional<NodeId> node_to_evict = SelectNodeToEvict(std::move(eviction_candidates));
44+    const std::optional<NodeId> node_to_evict = SelectNodeToEvict(std::move(eviction_candidates), /*force=*/false);

LarryRuane commented at 5:22 pm on June 12, 2023:

(I didn’t test that this is okay)

0    const std::optional<NodeId> node_to_evict = SelectNodeToEvict(std::move(eviction_candidates), /*force=*/fuzzed_data_provider.ConsumeBool());

pinheadmz commented at 3:18 pm on June 16, 2023:

yeah thanks good idea

LarryRuane commented at 5:22 pm on June 12, 2023: contributor

utACK 2a6a981e4d892c3d7196ddce54471350950affc7 These latest changes look good! I’ll test later in the week.

in doc/release-notes-27600.md:6 in 2a6a981e4d outdated

0@@ -0,0 +1,10 @@
1+P2P
2+---
3+
4+- A new net permission `ForceInbound` (set with `-whitelist=forceinbound@...`
5+  or `-whitebind=forceinbound@...`) is introduced that extends `NoBan` permissions.
6+  Inbound connections from hosts protected by `ForceInbound` will now be more

stickies-v commented at 4:06 pm on June 14, 2023:

nit: the capitalized spelling is an internal implementation detail, would stick to forceinbound and noban here

0- A new net permission `forceinbound` (set with `-whitelist=forceinbound@...`
1  or `-whitebind=forceinbound@...`) is introduced that extends `noban` permissions.
2  Inbound connections from hosts protected by `forceinbound` will now be more

pinheadmz commented at 2:02 pm on June 16, 2023:

done, thanks

stickies-v referenced this in commit 456b9ad924 on Jun 14, 2023

stickies-v commented at 4:36 pm on June 14, 2023: contributor

Code review ACK 2a6a981e4d892c3d7196ddce54471350950affc7 (modulo my commit reordering comment at the end)

By adding the forceinbound (extending noban) permission we prevent existing setups from being exposed to a wider attack surface when they have whitelisted ranges of IP addresses. I find it difficult to judge whether the increased complexity (not so much extra code, but more permissions means potentially more interactions to test for etc; as well as the user having to make more choices) is really necessary, but I can see this being a pretty common use case - and this is an elegant solution.

I think the commits should be reordered though, as in https://github.com/stickies-v/bitcoin/commits/pr/27600: I would first introduce “net: add new permission ForceInbound” so we don’t need to immediately update the logic, test and docs to go from NoBan -> ForceInbound. Doesn’t make things more complex from a re-review pov, imo (range-diff works well for me), and I think it’s bad practice to introduce “vulnerabilities” into master, e.g. in case of revert. The rebase is straightforward too.

pinheadmz referenced this in commit c8ce23745a on Jun 16, 2023

pinheadmz force-pushed on Jun 16, 2023

pinheadmz renamed this:
~~net: Allow inbound whitebind connections to more aggressively evict peers when slots are full~~
net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full
on Jun 16, 2023

pinheadmz commented at 3:37 pm on June 16, 2023: member

@stickies-v and @LarryRuane thanks for your review. I re-ordered the commits so the history makes more sense now. Also edited the title and description of the PR since we are adding an optional feature now, not changing any existing behavior.

git diff 2a6a981e4d c8ce23745a reflects the addressed comments, range-diff is screwed up because of commit reordering

stickies-v commented at 3:53 pm on June 16, 2023: contributor

Code Review re-ACK c8ce23745a

No changes but the review comments from LarryRuane and myself.

DrahtBot requested review from LarryRuane on Jun 16, 2023

pinheadmz requested review from vasild on Jun 22, 2023

pinheadmz requested review from mzumsande on Jun 22, 2023

luke-jr commented at 4:59 pm on June 23, 2023: member

Would be better to score the peers rather than choose one at random.

DrahtBot removed review request from LarryRuane on Jun 23, 2023

DrahtBot requested review from LarryRuane on Jun 23, 2023

pinheadmz commented at 5:40 pm on June 23, 2023: member

Would be better to score the peers rather than choose one at random.

I had logic for this initially but preferred this suggestion by @mzumsande

DrahtBot removed review request from LarryRuane on Jun 23, 2023

DrahtBot requested review from LarryRuane on Jun 23, 2023

mzumsande commented at 7:37 pm on June 30, 2023: contributor

Do I understand it correctly that using the new forceinbound permission instead of noban would be useful in the following two cases: 1.) If the user lowered -maxconnections to a value so low that the protection logic (that protects a fixed number between 20 and 28 inbound peers from eviction) could protect all inbound peers, but we still want to make sure to accept an inbound connection from a protected peer. 2) If we have a slightly higher -maxconnections but want to make sure a larger number of protected peers can connect to us at the same time.

So that a user running with the default -maxconnections of 125 would never have a reason to use forceinbound over noban unless they anticipate to have a large number of protected peers (>80 I guess?) simultaneously?

DrahtBot removed review request from LarryRuane on Jun 30, 2023

DrahtBot requested review from LarryRuane on Jun 30, 2023

pinheadmz commented at 4:06 pm on July 5, 2023: member

@mzumsande This summary looks correct to me. Point (1) is covered by the functional test. You are also right that by default most nodes will not need the flag since there will be enough open slots or evictable peers.

I think the use case highlighted in the issue (as well as my own use case) is more about “personal” nodes or limited-resource nodes, where max connections are more limited but we still always want our light clients to be able to connect.

DrahtBot removed review request from LarryRuane on Jul 5, 2023

DrahtBot requested review from LarryRuane on Jul 5, 2023

naumenkogs commented at 10:02 am on July 25, 2023: member

Consider when a malicious Internet provider or something can request the forceinbound IP address.

Does this PR open any fundamentally new risk in this case? I’m thinking of disrupting the network by causing many disconnections if the forceinbound practice is broad.

This probably depends on whether it’s possible to take multiple slots from the same forceinbound IP (or, rather, initiate multiple evictions), which I’m not 100% sure about.

If my suspicions are correct, this might better wait for authenticated connections…

DrahtBot removed review request from LarryRuane on Jul 25, 2023

DrahtBot requested review from LarryRuane on Jul 25, 2023

pinheadmz commented at 11:26 am on July 25, 2023: member

Thanks so much for taking a look Gleb and for your insight, yeah I have the same concerns. That attack vector is not super trivial, the attacker needs to both learn and then spoof the protected IP range. The release notes could recommend only using ranges for local subnets, or we could enforce that in the code. We could also change it soforceinbound does not evict existing peers but may add connections beyond the maximum, or set another internal limit just for forceinbound

DrahtBot removed review request from LarryRuane on Jul 25, 2023

DrahtBot requested review from LarryRuane on Jul 25, 2023

naumenkogs commented at 9:02 am on July 26, 2023: member

the attacker needs to both learn and then spoof the protected IP range

I think a malicious ISP gets that trivially (at least in the pre-BIP324 world) by simply monitoring the traffic.

In theory, specifying “forceinbound=iprange;n_extra_slots” instead of eviction is probably a good idea, but I’m not sure how ugly the code will look like. I remember max_connections stuff is already sophisticated :(

DrahtBot removed review request from LarryRuane on Jul 26, 2023

DrahtBot requested review from LarryRuane on Jul 26, 2023

pinheadmz commented at 3:51 pm on July 26, 2023: member

I guess allowing more connections beyond the max is a DoS vector, which could potentially crash a node and break all existing connections anyway… hm.

DrahtBot removed review request from LarryRuane on Jul 26, 2023

DrahtBot requested review from LarryRuane on Jul 26, 2023

DrahtBot removed review request from LarryRuane on Jul 26, 2023

DrahtBot requested review from LarryRuane on Jul 26, 2023

naumenkogs commented at 9:36 am on July 27, 2023: member

@pinheadmz In the example I had above n_extra_slots controls how many extra connections you allow so it should be safe.

DrahtBot removed review request from LarryRuane on Jul 27, 2023

DrahtBot requested review from LarryRuane on Jul 27, 2023

pinheadmz commented at 8:24 pm on July 28, 2023: member

@naumenkogs I added one more commit that limits the number of simultaneous forced-inbound connections to 8. I didn’t add it as an init option yet but we could if you like that approach. Although I think it should be set by a separate argument like -maxforceinbound instead of adding another syntactic element to the whitebind setting.

A connection only counts as forced-inbound if it succeeded in evicting another peer, and that is marked by a new bool in CNodeOptions

naumenkogs commented at 9:44 am on July 31, 2023: member

@pinheadmz yeah this is probably the best attempt at minimizing the problem I mentioned. Whether it’s satisfactory is still at question :) I’m looking forward to seeing what others think.

vasild commented at 11:04 am on July 31, 2023: contributor

this might better wait for authenticated connections

I2P and CJDNS connections are already authenticated. Addresses in those networks represent the node’s public key and connections have a source address. Thus to spoof, one would need to steal the private key from its owner.

luke-jr referenced this in commit f95b5f8840 on Aug 16, 2023

luke-jr referenced this in commit 7f86217b5b on Aug 16, 2023

luke-jr referenced this in commit a47f27c183 on Aug 16, 2023

luke-jr referenced this in commit 4a2bbbffe2 on Aug 16, 2023

luke-jr referenced this in commit 812df253be on Aug 29, 2023

luke-jr referenced this in commit a88314bccc on Aug 29, 2023

luke-jr referenced this in commit df56306e8a on Aug 29, 2023

luke-jr referenced this in commit 92f93e5559 on Aug 29, 2023

achow101 removed review request from LarryRuane on Sep 20, 2023

achow101 requested review from willcl-ark on Sep 20, 2023

achow101 requested review from amitiuttarwar on Sep 20, 2023

in test/functional/p2p_eviction.py:147 in bd64f1fabb outdated

139+
140+        self.log.debug("Default whitebind inbound gets rejected, even when full")
141+        with node.assert_debug_log(["failed to find an eviction candidate - connection dropped (full)"]):
142+            node.add_p2p_connection(RejectedPeer(), wait_for_verack=False, dstport=30201)
143+
144+        self.log.debug("ForceInbound whitebind inbound gets connected, even when full")

willcl-ark commented at 6:02 pm on September 25, 2023:

I was wondering if it might be nice to add a test which showed a previously-protected connection, e.g. a single Tor connection, being disconnected when a forceinbound connection from the whitebind port arrives. But as our test P2P connections already originate from localhost (127.0.0.1) my understanding is that these already count as a “previously-protected” group (as they might be an inbound Tor peer), so perhaps not necessary…

pinheadmz commented at 5:13 pm on September 26, 2023:

I’m trying to figure out if

Local peers are protected in any special way (I don’t think they are?)
Inbound onion connections are considered local (They aren’t - inbound onions are identifiable because they bind to a different network port onion_service_target_port)

And then, I don’t think all onion peers are protected per se but their latency is compensated for in the eviction process:

https://github.com/bitcoin/bitcoin/blob/c9f288244b8d183e09a917025922b99e3368ef78/src/node/eviction.cpp#L105-L112

in src/net_permissions.cpp:19 in 9190383b5e outdated

14@@ -15,7 +15,8 @@ const std::vector<std::string> NET_PERMISSIONS_DOC{
15     "relay (relay even in -blocksonly mode, and unlimited transaction announcements)",
16     "mempool (allow requesting BIP35 mempool contents)",
17     "download (allow getheaders during IBD, no disconnect after maxuploadtarget limit)",
18-    "addr (responses to GETADDR avoid hitting the cache and contain random records with the most up-to-date info)"
19+    "addr (responses to GETADDR avoid hitting the cache and contain random records with the most up-to-date info)",
20+    "forceinbound (when connections are full, attempt to evict a random unprotected peer to open a slot; implies noban)"

willcl-ark commented at 6:09 pm on September 25, 2023:

nit: if you touch again you could add here that it’s specifically a “random unprotected inbound peer”, as this is what many users will see in the --help (vs the release notes or code comments)

pinheadmz commented at 2:19 pm on September 26, 2023:

adding “inbound” good idea thanks

in src/net.h:358 in 8585fe3f80 outdated

353@@ -352,6 +354,8 @@ struct CNodeOptions
354     NetPermissionFlags permission_flags = NetPermissionFlags::None;
355     std::unique_ptr<i2p::sam::Session> i2p_sam_session = nullptr;
356     bool prefer_evict = false;
357+    // True if ForceInbound connection required evicting a peer
358+    bool forced_inbound = false;

willcl-ark commented at 7:57 am on September 26, 2023:

style nit: it’s not in our style docs but as I understand it we prefer brace initialization for new code

0bool forced_inbound{false};

pinheadmz commented at 2:16 pm on September 26, 2023:

ah thanks, will touch this up and in one other place in the top commit

willcl-ark approved

willcl-ark commented at 8:52 am on September 26, 2023: contributor

ACK 8585fe3f80

Only optional style nits if you touch again.

I’m pleased to see the final commit limiting the max number of forceinbound connections, as otherwise I too would have been a bit apprehensive of a user sharing a whitebind address:port with the world and seeing all their (non-noban) inbounds disconnected.

I’ve experienced the issue this pull should fix myself, usually solving it by having to remotely restart the node and racing other nodes in re-connecting, which is not particularly user-friendly.

Side note: whilst playing with various option combinations I noticed that we cannot set -listen=0 in combinatation with -whitebind=.... On one level this does make sense, but I now wonder whether -whitebind (but not -bind) could be permitted when listen=0, as it could make sense to want to accept only connections of this type?