Currently, there is no way of making sure that even if a node is whitelisted (via whitebind or whitelist) a connection from it would be accepted if our node is already at peer capacity. This would only happen under certain non-default settings in where the node connection limit has been set bellow the eviction protection limits, meaning that on a new inbound connection request no current peer will be evicted, and the request will be simply dropped.
The connection limit for this to happen has to be bellow 38 IIRC, that’s 10 for outbound connections plus 28 for the different characteristics consider worth protecting under eviction::SelectNodeToEvict, and assuming no existing peer has been granted noban permissions.
The goal of this PR is to allow our node to evict one of the currently implicitly protected peers (one of the aforementioned 28) if a whitelisted node request a connection. Under this conditions, only outbounds and noban peers will be removed from the eviction candidates, lowering the bar for the minimum setting of the connection limit while still allowing whitelisting peers to connect to our node.
My overall opinion on the concept is positive. I think that this is something worth having even if it only applies to non-{default/standard} settings. Furthermore, some users have acknowledged this issue before. An easy solution without any code change for those users would be to increase their max connection limit to less restrictive settings, so there are still some eviction candidates after removing the implicitly protected peers. However, the change for this to work on those restrictive settings without making them consume more resources seems pretty small, so there is no good reason for not going forward with the PR.
I’m not that happy with the approach though, given it requires adding a new network permission for a really narrow usecase. I’d be happier if the issue could be fixed without the need for it, given it feels like a big change for something that is not super relevant. The reasoning for this is that just re-purposing noban would result in unexpected behavior for users who already use it, and may be abusable if whitebind is used on a publicly reachable interface. It feels to me that a global flag could work around that, given we may not want to distinguish between whitelisted peers we want guarantee a connection slots a the ones we would not, but I’m not too familiar with wether using a global flag is a good practice
First review (regarding the approach, no code comments):