Merge 27600 via p2p_forceinbound-28+knots

src/net.cpp

@@ -632,6 +632,7 @@ void CNode::CopyStats(CNodeStats& stats)
         if (info.session_id) stats.m_session_id = HexStr(*info.session_id);
     }
     X(m_permission_flags);
+    X(m_forced_inbound);
 
     X(m_last_ping_time);
     X(m_min_ping_time);
@@ -1652,7 +1653,7 @@ std::pair<size_t, bool> CConnman::SocketSendData(CNode& node) const
  *   to forge.  In order to partition a node the attacker must be
  *   simultaneously better at all of them than honest peers.
  */
-bool CConnman::AttemptToEvictConnection()
+bool CConnman::AttemptToEvictConnection(bool force)
 {
     std::vector<NodeEvictionCandidate> vEvictionCandidates;
     {
@@ -1680,7 +1681,7 @@ bool CConnman::AttemptToEvictConnection()
             vEvictionCandidates.push_back(candidate);
         }
     }
-    const std::optional<NodeId> node_id_to_evict = SelectNodeToEvict(std::move(vEvictionCandidates));
+    const std::optional<NodeId> node_id_to_evict = SelectNodeToEvict(std::move(vEvictionCandidates), force);
     if (!node_id_to_evict) {
         return false;
     }
@@ -1773,13 +1774,19 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
         return;
     }
 
+    bool forced{false};
     if (nInbound >= m_max_inbound)
     {
-        if (!AttemptToEvictConnection()) {
+        // If the inbound connection attempt is granted ForceInbound permission, try a little harder
+        // to make room by evicting a peer we may not have otherwise evicted.
+        if (!AttemptToEvictConnection(NetPermissions::HasFlag(permission_flags, NetPermissionFlags::ForceInbound))) {
             // No connection to evict, disconnect the new connection
             LogPrint(BCLog::NET, "failed to find an eviction candidate - connection dropped (full)\n");
             return;
         }
+
+        // We kicked someone out
+        forced = true;
     }
 
     NodeId id = GetNewNodeId();
@@ -1803,6 +1810,7 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
                              CNodeOptions{
                                  .permission_flags = permission_flags,
                                  .prefer_evict = discouraged,
+                                 .forced_inbound = forced,
                                  .recv_flood_size = nReceiveFloodSize,
                                  .use_v2transport = use_v2transport,
                              });
@@ -3752,6 +3760,7 @@ CNode::CNode(NodeId idIn,
       m_dest(addrNameIn),
       m_inbound_onion{inbound_onion},
       m_prefer_evict{node_opts.prefer_evict},
+      m_forced_inbound{node_opts.forced_inbound},
       nKeyedNetGroup{nKeyedNetGroupIn},
       m_conn_type{conn_type_in},
       id{idIn},

src/net.h

@@ -220,6 +220,8 @@ public:
     TransportProtocolType m_transport_type;
     /** BIP324 session id string in hex, if any. */
     std::string m_session_id;
+    /** whether this peer forced its connection by evicting another */
+    bool m_forced_inbound;
 };
 
 
@@ -661,6 +663,8 @@ struct CNodeOptions
     NetPermissionFlags permission_flags = NetPermissionFlags::None;
     std::unique_ptr<i2p::sam::Session> i2p_sam_session = nullptr;
     bool prefer_evict = false;
+    // True if ForceInbound connection required evicting a peer
+    bool forced_inbound{false};
     size_t recv_flood_size{DEFAULT_MAXRECEIVEBUFFER * 1000};
     bool use_v2transport = false;
 };
@@ -718,6 +722,7 @@ public:
      */
     std::string cleanSubVer GUARDED_BY(m_subver_mutex){};
     const bool m_prefer_evict{false}; // This peer is preferred for eviction.
+    const bool m_forced_inbound{false}; // This peer forced an inbound connection
     bool HasPermission(NetPermissionFlags permission) const {
         return NetPermissions::HasFlag(m_permission_flags, permission);
     }
@@ -1343,7 +1348,14 @@ private:
      */
     bool AlreadyConnectedToAddress(const CAddress& addr);
 
-    bool AttemptToEvictConnection();
+    /**
+     * Attempt to disconnect a connected peer.
+     * Used to make room for new inbound connections, returns true if successful.
+     * @param[in] force     Try to evict a random inbound ban-able peer if
+     *                      all connections are otherwise protected.
+     */
+    bool AttemptToEvictConnection(bool force);
+
     CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type, bool use_v2transport) EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex);
     void AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr, const std::vector<NetWhitelistPermissions>& ranges) const;
 

src/net_permissions.cpp

@@ -18,7 +18,8 @@ const std::vector<std::string> NET_PERMISSIONS_DOC{
     "relay (relay even in -blocksonly mode, and unlimited transaction announcements)",
     "mempool (allow requesting BIP35 mempool contents)",
     "download (allow getheaders during IBD, no disconnect after maxuploadtarget limit)",
-    "addr (responses to GETADDR avoid hitting the cache and contain random records with the most up-to-date info)"
+    "addr (responses to GETADDR avoid hitting the cache and contain random records with the most up-to-date info)",
+    "forceinbound (when connections are full, attempt to evict a random unprotected inbound peer to open a slot; implies noban)"
 };
 
 namespace {
@@ -57,6 +58,7 @@ static bool TryParsePermissionFlags(const std::string& str, NetPermissionFlags&
             else if (permission == "all") NetPermissions::AddFlag(flags, NetPermissionFlags::All);
             else if (permission == "relay") NetPermissions::AddFlag(flags, NetPermissionFlags::Relay);
             else if (permission == "addr") NetPermissions::AddFlag(flags, NetPermissionFlags::Addr);
+            else if (permission == "forceinbound") NetPermissions::AddFlag(flags, NetPermissionFlags::ForceInbound);
             else if (permission == "in") connection_direction |= ConnectionDirection::In;
             else if (permission == "out") {
                 if (output_connection_direction == nullptr) {

src/net_permissions.h

@@ -40,6 +40,9 @@ enum class NetPermissionFlags : uint32_t {
     // Can request addrs without hitting a privacy-preserving cache, and send us
     // unlimited amounts of addrs.
     Addr = (1U << 7),
+    // Try harder to evict an existing connection if needed to open
+    // an inbound slot from this peer
+    ForceInbound = (1U << 10) | NoBan,
 
     // Can query compact filters even if -peerblockfilters is false
     BlockFilters = (1U << 8),
@@ -49,7 +52,7 @@ enum class NetPermissionFlags : uint32_t {
     // True if the user did not specifically set fine-grained permissions with
     // the -whitebind or -whitelist configuration options.
     Implicit = (1U << 31),
-    All = BloomFilter | ForceRelay | Relay | NoBan | Mempool | Download | Addr | BlockFilters,
+    All = BloomFilter | ForceRelay | Relay | NoBan | Mempool | Download | Addr | BlockFilters | ForceInbound,
 };
 static inline constexpr NetPermissionFlags operator|(NetPermissionFlags a, NetPermissionFlags b)
 {

src/node/eviction.cpp

@@ -3,6 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <node/eviction.h>
+#include <random.h>
 
 #include <algorithm>
 #include <array>
@@ -175,7 +176,7 @@ void ProtectEvictionCandidatesByRatio(std::vector<NodeEvictionCandidate>& evicti
     EraseLastKElements(eviction_candidates, ReverseCompareNodeTimeConnected, remaining_to_protect);
 }
 
-[[nodiscard]] std::optional<NodeId> SelectNodeToEvict(std::vector<NodeEvictionCandidate>&& vEvictionCandidates)
+[[nodiscard]] std::optional<NodeId> SelectNodeToEvict(std::vector<NodeEvictionCandidate>&& vEvictionCandidates, bool force)
 {
     // Protect connections with certain characteristics
 
@@ -183,6 +184,15 @@ void ProtectEvictionCandidatesByRatio(std::vector<NodeEvictionCandidate>& evicti
 
     ProtectOutboundConnections(vEvictionCandidates);
 
+    if (vEvictionCandidates.empty()) return std::nullopt;
+
+    // Hang on to one random node to evict if forced
+    std::optional<NodeId> force_evict;
+    if (force) {
+        uint64_t randpos{FastRandomContext().randrange(vEvictionCandidates.size())};
+        force_evict = vEvictionCandidates.at(randpos).id;
+    }
+
     // Deterministically select 4 peers to protect by netgroup.
     // An attacker cannot predict which netgroups will be protected
     EraseLastKElements(vEvictionCandidates, CompareNetGroupKeyed, 4);
@@ -204,7 +214,8 @@ void ProtectEvictionCandidatesByRatio(std::vector<NodeEvictionCandidate>& evicti
     // or disadvantaged characteristics.
     ProtectEvictionCandidatesByRatio(vEvictionCandidates);
 
-    if (vEvictionCandidates.empty()) return std::nullopt;
+    // May still return nullopt is `force` argument is false
+    if (vEvictionCandidates.empty()) return force_evict;
 
     // If any remaining peers are preferred for eviction consider only them.
     // This happens after the other preferences since if a peer is really the best by other criteria (esp relaying blocks)

src/node/eviction.h

@@ -38,8 +38,12 @@ struct NodeEvictionCandidate {
  * fixed numbers of desirable peers per various criteria, followed by (mostly)
  * ratios of desirable or disadvantaged peers. If any eviction candidates
  * remain, the selection logic chooses a peer to evict.
+ * @param[in]   force   Attempt to evict a random peer if no candidates
+ *                      are available among inbound no-ban connections.
+ *                      (see CConman::AttemptToEvictConnection())
+ *                      Default: false
  */
-[[nodiscard]] std::optional<NodeId> SelectNodeToEvict(std::vector<NodeEvictionCandidate>&& vEvictionCandidates);
+[[nodiscard]] std::optional<NodeId> SelectNodeToEvict(std::vector<NodeEvictionCandidate>&& vEvictionCandidates, bool force = false);
 
 /** Protect desirable or disadvantaged inbound peers from eviction by ratio.
  *

src/rpc/net.cpp

@@ -168,6 +168,7 @@ static RPCHelpMan getpeerinfo()
                     {
                         {RPCResult::Type::STR, "permission_type", Join(NET_PERMISSIONS_DOC, ",\n") + ".\n"},
                     }},
+                    {RPCResult::Type::BOOL, "forced_inbound", "Whether this peer forced a connection by evicting another."},
                     {RPCResult::Type::NUM, "minfeefilter", "The minimum fee rate for transactions this peer accepts"},
                     {RPCResult::Type::OBJ_DYN, "bytessent_per_msg", "",
                     {
@@ -275,6 +276,7 @@ static RPCHelpMan getpeerinfo()
             permissions.push_back(permission);
         }
         obj.pushKV("permissions", std::move(permissions));
+        obj.pushKV("forced_inbound", stats.m_forced_inbound);
         obj.pushKV("minfeefilter", ValueFromAmount(statestats.m_fee_filter_received));
 
         UniValue sendPerMsgType(UniValue::VOBJ);

src/test/fuzz/node_eviction.cpp

@@ -40,7 +40,7 @@ FUZZ_TARGET(node_eviction)
     // Make a copy since eviction_candidates may be in some valid but otherwise
     // indeterminate state after the SelectNodeToEvict(&&) call.
     const std::vector<NodeEvictionCandidate> eviction_candidates_copy = eviction_candidates;
-    const std::optional<NodeId> node_to_evict = SelectNodeToEvict(std::move(eviction_candidates));
+    const std::optional<NodeId> node_to_evict = SelectNodeToEvict(std::move(eviction_candidates), /*force=*/fuzzed_data_provider.ConsumeBool());
     if (node_to_evict) {
         assert(std::any_of(eviction_candidates_copy.begin(), eviction_candidates_copy.end(), [&node_to_evict](const NodeEvictionCandidate& eviction_candidate) { return *node_to_evict == eviction_candidate.id; }));
     }

src/test/net_peer_eviction_tests.cpp

@@ -573,7 +573,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
 bool IsEvicted(std::vector<NodeEvictionCandidate> candidates, const std::unordered_set<NodeId>& node_ids, FastRandomContext& random_context)
 {
     std::shuffle(candidates.begin(), candidates.end(), random_context);
-    const std::optional<NodeId> evicted_node_id = SelectNodeToEvict(std::move(candidates));
+    const std::optional<NodeId> evicted_node_id = SelectNodeToEvict(std::move(candidates), /*force=*/false);
     if (!evicted_node_id) {
         return false;
     }
@@ -666,14 +666,14 @@ BOOST_AUTO_TEST_CASE(peer_eviction_test)
         // four peers by net group, eight by lowest ping time, four by last time of novel tx, up to eight non-tx-relay
         // peers by last novel block time, and four more peers by last novel block time.
         if (number_of_nodes >= 29) {
-            BOOST_CHECK(SelectNodeToEvict(GetRandomNodeEvictionCandidates(number_of_nodes, random_context)));
+            BOOST_CHECK(SelectNodeToEvict(GetRandomNodeEvictionCandidates(number_of_nodes, random_context), /*force=*/false));
         }
 
         // No eviction is expected given <= 20 random eviction candidates. The eviction logic protects at least
         // four peers by net group, eight by lowest ping time, four by last time of novel tx and four peers by last
         // novel block time.
         if (number_of_nodes <= 20) {
-            BOOST_CHECK(!SelectNodeToEvict(GetRandomNodeEvictionCandidates(number_of_nodes, random_context)));
+            BOOST_CHECK(!SelectNodeToEvict(GetRandomNodeEvictionCandidates(number_of_nodes, random_context), /*force=*/false));
         }
 
         // Cases left to test:

test/functional/p2p_eviction.py

@@ -122,6 +122,43 @@ class P2PEvict(BitcoinTestFramework):
         self.log.debug("{} protected peers: {}".format(len(protected_peers), protected_peers))
         assert evicted_peers[0] not in protected_peers
 
+        self.log.info("Test that whitebind inbounds get extra eviction power")
+        # Allow 10 inbound connections, set whitebind and forceinbound
+        self.restart_node(0, extra_args=['-maxconnections=21', '-whitebind=127.0.0.1:30201', '-whitebind=forceinbound@127.0.0.1:30202'])
+        self.log.debug("Fill connections with unprivileged peers")
+        for i in range(10):
+            node.add_p2p_connection(P2PInterface())
+
+        # Create a peer that expects to be rejected
+        # FIXME: "multiprocess, i686, DEBUG" CI task has a reliable timeout issue for v2transport
+        class RejectedPeer(P2PInterface):
+            def connection_lost(self, exc):
+                return
+
+        allowed_peers = []
+
+        self.log.debug("Generic inbound gets rejected when full")
+        with node.assert_debug_log(["failed to find an eviction candidate - connection dropped (full)"]):
+            node.add_p2p_connection(RejectedPeer(), wait_for_verack=False, supports_v2_p2p=False)
+
+        self.log.debug("Default whitebind inbound gets rejected, even when full")
+        with node.assert_debug_log(["failed to find an eviction candidate - connection dropped (full)"]):
+            node.add_p2p_connection(RejectedPeer(), wait_for_verack=False, supports_v2_p2p=False, dstport=30201)
+
+        self.log.debug("ForceInbound whitebind inbound gets connected, even when full")
+        allowed_peers.append(node.add_p2p_connection(P2PInterface(), dstport=30202))
+
+        peerinfo = node.getpeerinfo()
+        assert_equal(len(peerinfo), 10)
+        for peer in peerinfo:
+            if "30202" in peer["addrbind"]:
+                assert peer["forced_inbound"]
+            else:
+                assert not peer["forced_inbound"]
+
+        self.log.debug("Generic inbound gets rejected when whitebind peer is filling inbound slot")
+        with node.assert_debug_log(["failed to find an eviction candidate - connection dropped (full)"]):
+            node.add_p2p_connection(RejectedPeer(), supports_v2_p2p=False, wait_for_verack=False)
 
 if __name__ == '__main__':
     P2PEvict(__file__).main()

test/functional/rpc_net.py

@@ -164,6 +164,7 @@ class NetTest(BitcoinTestFramework):
                 "minfeefilter": Decimal("0E-8"),
                 "network": "not_publicly_routable",
                 "permissions": [],
+                "forced_inbound": False,
                 "presynced_headers": -1,
                 "relaytxes": False,
                 "services": "0000000000000000",