Apply snode strikes where they were being ignored

include/session/network/routing/onion_request_router.hpp

@@ -31,7 +31,6 @@ namespace config {
         uint8_t path_strike_threshold;
         uint8_t path_build_retry_limit;
         std::chrono::minutes path_rotation_frequency;
-        uint8_t node_strike_threshold;
         bool disable_pre_build_paths;
         bool single_path_mode;
         std::unordered_map<PathCategory, uint8_t> min_path_counts;
@@ -168,6 +167,7 @@ class OnionRequestRouter : public IRouter, public std::enable_shared_from_this<O
     // All of the below functions should only be called from within `_loop`
     void _finish_setup();
     void _pre_build_paths_if_needed();
+    void _drop_struck_cached_edge_nodes();
     void _close_connections();
     void _update_status();
     void _send_request_internal(Request request, network_response_callback_t callback);

include/session/network/snode_pool.hpp

@@ -69,6 +69,13 @@ class SnodePool : public std::enable_shared_from_this<SnodePool> {
     virtual void record_node_failure(const ed25519_pubkey& key, bool permanent = false);
     uint16_t node_strike_count(const service_node& node);
     uint16_t node_strike_count(const ed25519_pubkey& key);
+
+    // Whether the node has collected enough unexpired strikes to be kept out of node selection.
+    // Callers that pick a node by some other route - a cached one, say - need this to apply the
+    // same bar `get_unused_nodes` does, rather than comparing a raw count to a threshold they'd
+    // have to know about.
+    bool node_struck_out(const service_node& node);
+    bool node_struck_out(const ed25519_pubkey& key);
     void clear_node_strikes();
 
     // Checks if the pool is empty or stale and triggers a refresh if needed
@@ -115,6 +122,11 @@ class SnodePool : public std::enable_shared_from_this<SnodePool> {
     std::vector<std::vector<std::byte>> _snode_refresh_results;
     std::vector<std::function<void()>> _after_snode_cache_refresh;
 
+    // Strikes are only pruned when the node collects a new one, so the stored vector can still
+    // hold expired stamps - count them out before deciding anything.
+    size_t _active_strike_count(const ed25519_pubkey& key) const;
+    bool _node_struck_out(const ed25519_pubkey& key) const;
+
     // Disk I/O functions
     void _load_from_disk();
     static void _clear_disk_cache(const std::filesystem::path& path);

src/network/routing/onion_request_router.cpp

@@ -595,9 +595,31 @@ void OnionRequestRouter::_finish_setup() {
     }
 }
 
+// A cached edge node is forced as a path's first hop, so it never passes the strike filter
+// `get_unused_nodes` applies to the rest; erasing rather than skipping is what stops it reclaiming
+// the role when its strikes expire.
+void OnionRequestRouter::_drop_struck_cached_edge_nodes() {
+    auto snode_pool = _snode_pool.lock();
+
+    if (!snode_pool)
+        return;
+
+    std::erase_if(_cached_edge_nodes, [&snode_pool](const auto& cached) {
+        if (!snode_pool->node_struck_out(cached.node))
+            return false;
+
+        log::debug(
+                cat,
+                "Dropping cached edge node {}, it has been struck out.",
+                cached.node.to_string());
+        return true;
+    });
+}
+
 void OnionRequestRouter::_pre_build_paths_if_needed() {
     if (!_config.disable_pre_build_paths) {
         log::info(cat, "Pre-building initial paths.");
+        _drop_struck_cached_edge_nodes();
         std::vector<cached_edge_node> edge_nodes = _cached_edge_nodes;
 
         if (_config.single_path_mode) {
@@ -1720,8 +1742,7 @@ void OnionRequestRouter::_handle_transport_response(
                         for (const auto& node : path.nodes) {
                             auto node_key = ed25519_pubkey::from_bytes(node.view_remote_key());
 
-                            if (snode_pool->node_strike_count(node_key) >=
-                                _config.node_strike_threshold)
+                            if (snode_pool->node_struck_out(node_key))
                                 nodes_to_repair.push_back(node_key);
                         }
 
@@ -2128,13 +2149,17 @@ void OnionRequestRouter::_rotate_path(const std::string& path_id, PathCategory c
     }
 
     // Get enough nodes for the path (if the edge node has been used for longer than the cache
-    // duration then we should create an entirely new path, otherwise we should try to reuse the
-    // edge node)
+    // duration, or has been struck out since we connected to it, then we should create an entirely
+    // new path, otherwise we should try to reuse the edge node)
     auto now = std::chrono::system_clock::now();
     auto rotate_at = (std::chrono::steady_clock::now() + _config.path_rotation_frequency);
     std::vector<service_node> rotated_path_nodes;
 
-    if (now > path.edge_first_connected_at + _config.edge_node_cache_duration)
+    bool new_edge =
+            (now > path.edge_first_connected_at + _config.edge_node_cache_duration ||
+             snode_pool->node_struck_out(edge_node));
+
+    if (new_edge)
         rotated_path_nodes = snode_pool->get_unused_nodes(_config.path_length, nodes_to_exclude);
     else {
         rotated_path_nodes =
@@ -2158,7 +2183,10 @@ void OnionRequestRouter::_rotate_path(const std::string& path_id, PathCategory c
     }
 
     OnionPath new_path{
-            new_path_id, std::move(rotated_path_nodes), now, path.edge_first_connected_at};
+            new_path_id,
+            std::move(rotated_path_nodes),
+            now,
+            (new_edge ? now : path.edge_first_connected_at)};
 
     // Send /info request to verify path before rotating
     Request info_request{

src/network/session_network.cpp

@@ -113,7 +113,6 @@ namespace {
                 main_config.onionreq_path_strike_threshold,
                 main_config.onionreq_path_build_retry_limit,
                 main_config.onionreq_path_rotation_frequency,
-                main_config.cache_node_strike_threshold,
                 main_config.onionreq_disable_pre_build_paths,
                 main_config.onionreq_single_path_mode,
                 main_config.onionreq_min_path_counts};

src/network/snode_pool.cpp

@@ -963,25 +963,43 @@ void SnodePool::clear_cache() {
     });
 }
 
+size_t SnodePool::_active_strike_count(const ed25519_pubkey& key) const {
+    auto it = _snode_strikes.find(key);
+
+    if (it == _snode_strikes.end())
+        return 0;
+
+    auto threshold = sysclock_now_s() - STRIKE_EXPIRY;
+
+    return std::ranges::count_if(it->second, [threshold](auto t) { return t > threshold; });
+}
+
+bool SnodePool::_node_struck_out(const ed25519_pubkey& key) const {
+    auto strikes = _active_strike_count(key);
+
+    // The `strikes > 0` is what keeps a threshold of 0 meaning "drop a node on its first strike";
+    // comparing straight against 0 also drops every node that has never failed at all
+    return (strikes > 0 && strikes >= _config.cache_node_strike_threshold);
+}
+
 void SnodePool::record_node_failure(const service_node& node, bool permanent) {
     record_node_failure(node.remote_pubkey, permanent);
 }
 
 void SnodePool::record_node_failure(const ed25519_pubkey& key, bool permanent) {
     _loop->call([this, key, permanent] {
         auto now = sysclock_now_s();
+        auto& stamps = _snode_strikes[key];
+        std::erase_if(stamps, [threshold = now - STRIKE_EXPIRY](auto t) { return t <= threshold; });
 
-        if (permanent)
-            for (int i = 0; i < _config.cache_node_strike_threshold; ++i)
-                _snode_strikes[key].push_back(now);
-        else
-            _snode_strikes[key].push_back(now);
+        // A permanent failure has to strike the node out whatever the threshold is - looping up to
+        // a threshold of 0 records nothing and leaves the node in rotation
+        auto strikes = (permanent ? std::max<uint16_t>(1, _config.cache_node_strike_threshold) : 1);
 
-        log::trace(
-                cat,
-                "Recorded strike for node {}, total: {}",
-                key.hex(),
-                _snode_strikes[key].size());
+        for (uint16_t i = 0; i < strikes; ++i)
+            stamps.push_back(now);
+
+        log::trace(cat, "Recorded strike for node {}, total: {}", key.hex(), stamps.size());
 
         // Throttle persisting the strikes to disk to at most every X minutes
         if (!_strikes_flush_scheduled && !_suspended) {
@@ -1009,22 +1027,16 @@ uint16_t SnodePool::node_strike_count(const service_node& node) {
 }
 
 uint16_t SnodePool::node_strike_count(const ed25519_pubkey& key) {
-    return _loop->call_get([this, &key] {
-        auto it = _snode_strikes.find(key);
-        if (it == _snode_strikes.end())
-            return uint16_t{0};
-
-        const auto& stamps = it->second;
-
-        const auto threshold = sysclock_now_s() - STRIKE_EXPIRY;
+    return _loop->call_get(
+            [this, &key] { return static_cast<uint16_t>(_active_strike_count(key)); });
+}
 
-        uint16_t count = 0;
-        for (auto t : stamps)
-            if (t > threshold)
-                count++;
+bool SnodePool::node_struck_out(const service_node& node) {
+    return node_struck_out(node.remote_pubkey);
+}
 
-        return count;
-    });
+bool SnodePool::node_struck_out(const ed25519_pubkey& key) {
+    return _loop->call_get([this, &key] { return _node_struck_out(key); });
 }
 
 void SnodePool::clear_node_strikes() {
@@ -1068,9 +1080,7 @@ void SnodePool::refresh_if_needed(
                     in_use_keys.insert(node.remote_pubkey);
 
                 for (const auto& node : _snode_cache) {
-                    auto it = _snode_strikes.find(node.remote_pubkey);
-                    if (it != _snode_strikes.end() &&
-                        it->second.size() >= _config.cache_node_strike_threshold)
+                    if (_node_struck_out(node.remote_pubkey))
                         continue;
 
                     // If the caller considers the node as already in use then it wouldn't be
@@ -1158,9 +1168,7 @@ std::vector<service_node> SnodePool::get_unused_nodes(
                 continue;
 
             // Skip nodes with too many failures
-            auto it = _snode_strikes.find(node.remote_pubkey);
-            if (it != _snode_strikes.end() &&
-                it->second.size() >= _config.cache_node_strike_threshold)
+            if (_node_struck_out(node.remote_pubkey))
                 continue;
 
             // Skip nodes whos IP addresses are in the exclusion list
@@ -1196,15 +1204,14 @@ void SnodePool::get_swarm(
             std::ranges::shuffle(nodes, csrng);
 
             auto get_strike_count = [this](const service_node& node) -> size_t {
-                auto it = _snode_strikes.find(node.remote_pubkey);
-                return (it != _snode_strikes.end() ? it->second.size() : 0);
+                return _active_strike_count(node.remote_pubkey);
             };
 
             // Partition into below-threshold and above-thresold.  This keeps the shuffled order of
             // each set:
             auto over_nodes = std::ranges::stable_partition(
-                    nodes.begin(), nodes.end(), [&](const auto& node) {
-                        return get_strike_count(node) < _config.cache_node_strike_threshold;
+                    nodes.begin(), nodes.end(), [this](const auto& node) {
+                        return !_node_struck_out(node.remote_pubkey);
                     });
 
             auto under_count = nodes.size() - over_nodes.size();