From 60c6d7996d0e93a61450b4179c4cca5e8a88b05b Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 11 Sep 2026 09:58:26 +1000 Subject: [PATCH 1/2] Recover from a 421 on the evidence rather than on the cache's age A 421 tells us, authoritatively, that the swarm we resolved for an account is wrong. `_handle_421_retry` answered it with `refresh_if_needed`, which decides on `cache_expiration` (2h) and so declines on any cache younger than that, and then re-read the same `_swarm_cache` entry - so the retry went to another node in the list that had just rejected us, and every later request for that account did the same until the cache aged out. Evicting the swarm cache entry alone cannot fix it: the entry is only ever a memo of `swarm::get_swarm(pubkey, _all_swarms)`, so it recomputes the identical answer. What the rejection disproves is the pool snapshot the swarms were generated from, and only a refresh replaces that. `invalidate_swarm` refreshes on that evidence, with a backoff that doubles each time another rejection arrives after a refresh we already ran for one, so a node rejecting everything costs a handful of refreshes over a couple of hours rather than one every minute for as long as it keeps it up. --- include/session/network/snode_pool.hpp | 10 ++ src/network/session_network.cpp | 18 ++-- src/network/snode_pool.cpp | 80 ++++++++++++++ tests/test_snode_pool.cpp | 143 +++++++++++++++++++++++++ 4 files changed, 243 insertions(+), 8 deletions(-) diff --git a/include/session/network/snode_pool.hpp b/include/session/network/snode_pool.hpp index 787e2c582..22c0194f2 100644 --- a/include/session/network/snode_pool.hpp +++ b/include/session/network/snode_pool.hpp @@ -76,6 +76,14 @@ class SnodePool : public std::enable_shared_from_this { const std::vector& in_use_nodes, std::function on_refresh_complete = nullptr); + // Re-resolves `swarm_pubkey` after a node we believed was in its swarm has told us the account + // isn't there. Where `refresh_if_needed` decides on the cache's age, this decides on that + // rejection, so a mapping we have direct proof is wrong can be replaced long before + // `cache_expiration`. `on_complete` runs once the swarm can be resolved again. + virtual void invalidate_swarm( + session::network::x25519_pubkey swarm_pubkey, + std::function on_complete = nullptr); + virtual void get_swarm( session::network::x25519_pubkey swarm_pubkey, bool ignore_strike_count, @@ -109,6 +117,8 @@ class SnodePool : public std::enable_shared_from_this { // Refresh logic std::chrono::system_clock::time_point _last_snode_cache_update; + std::chrono::system_clock::time_point _last_evidence_refresh; + std::chrono::seconds _evidence_refresh_backoff{0}; std::optional _current_snode_cache_refresh_id; int _snode_cache_refresh_failure_count = 0; std::vector _refresh_candidate_nodes; diff --git a/src/network/session_network.cpp b/src/network/session_network.cpp index 7d160fc41..474ed48cf 100644 --- a/src/network/session_network.cpp +++ b/src/network/session_network.cpp @@ -821,24 +821,26 @@ void Network::_handle_421_retry( "421 Misdirected Request for a request with no swarm"); } - // If we got a 421 it means our snode cache is outdated (because the swarm the destination node - // belongs to doesn't match our cache anymore) log::info( cat, - "Request {} received 421 from node {}, refreshing swarm if stale.", + "Request {} received 421 from node {}, re-resolving its swarm.", original_request.request_id, original_dest_node->to_string()); auto failed_node_copy = *original_dest_node; - std::vector nodes_to_exclude = _router->get_all_used_nodes(); - _snode_pool->refresh_if_needed( - std::move(nodes_to_exclude), + auto swarm_pubkey = *original_request.swarm_pubkey; + + // A node in the swarm we resolved has told us the account isn't in its swarm, which is proof + // the mapping is wrong now rather than a reason to suspect it might be; asking for a refresh + // by age (`refresh_if_needed`) would decline for as long as `cache_expiration`, leaving every + // request for this account failing until then. + _snode_pool->invalidate_swarm( + swarm_pubkey, [this, + swarm_pubkey, req_to_retry = std::move(original_request), cb = std::move(final_callback), failed_node = failed_node_copy] { - auto swarm_pubkey = *req_to_retry.swarm_pubkey; - _snode_pool->get_swarm( swarm_pubkey, false, diff --git a/src/network/snode_pool.cpp b/src/network/snode_pool.cpp index d4f61ad9a..273a0a0a2 100644 --- a/src/network/snode_pool.cpp +++ b/src/network/snode_pool.cpp @@ -42,6 +42,18 @@ namespace { const std::chrono::seconds STRIKE_EXPIRY = 48h; const std::chrono::seconds SAVE_THROTTLE = 5min; + + // A swarm rejection is evidence against the pool snapshot the swarm was derived from, so it is + // only worth acting on once that snapshot is old enough for a refresh to plausibly return + // something different. + const std::chrono::seconds EVIDENCE_REFRESH_MIN_POOL_AGE = 1min; + + // A rejection arriving *after* we already refreshed on one is the refresh telling us it didn't + // help - the network hasn't settled, or the node is lying - so each successive one waits twice + // as long, up to `cache_expiration`. A single 3-node refresh per minute sustained indefinitely + // is not an acceptable resting state for a client on a bad connection, and without this it is + // exactly what a persistent rejection source would produce. + const std::chrono::seconds EVIDENCE_REFRESH_BASE_BACKOFF = 1min; } // namespace SnodePool::SnodePool( @@ -1113,6 +1125,74 @@ void SnodePool::refresh_if_needed( }); } +void SnodePool::invalidate_swarm(x25519_pubkey swarm_pubkey, std::function on_complete) { + _loop->call([this, swarm_pubkey, cb = std::move(on_complete)]() mutable { + if (_suspended) { + log::info(cat, "Ignoring swarm invalidation as pool is suspended."); + + if (cb) + cb(); + return; + } + + // The swarm cache only ever memoises `swarm::get_swarm(pubkey, _all_swarms)`, and + // `_all_swarms` is generated from the pool, so dropping the single entry would recompute + // the same rejected answer. What the rejection actually disproves is the pool snapshot, + // and a refresh is the only thing that replaces it - clearing the swarm cache as it goes. + auto now = std::chrono::system_clock::now(); + + if (!_current_snode_cache_refresh_id) { + // The backoff is deliberately global rather than per-swarm: a refresh replaces the pool + // every swarm is derived from, so a rejection for one swarm is exactly as unhelpful to + // act on right after a refresh as a second rejection for the swarm that prompted it + auto pool_age = now - _last_snode_cache_update; + auto since_last = now - _last_evidence_refresh; + + // Quiet for twice the interval we were holding means whatever caused the last run is + // over, so the next incident starts from the base delay rather than inheriting it + if (since_last > 2 * _evidence_refresh_backoff) + _evidence_refresh_backoff = 0s; + + if (pool_age < EVIDENCE_REFRESH_MIN_POOL_AGE || since_last < _evidence_refresh_backoff) + log::info( + cat, + "Swarm {} was rejected, but the pool is {}s old and the last refresh on a " + "rejection was {}s ago (backoff {}s); leaving it alone.", + swarm_pubkey.hex(), + std::chrono::duration_cast(pool_age).count(), + std::chrono::duration_cast(since_last).count(), + _evidence_refresh_backoff.count()); + else { + _evidence_refresh_backoff = std::min( + std::chrono::seconds{_config.cache_expiration}, + (_evidence_refresh_backoff == 0s ? EVIDENCE_REFRESH_BASE_BACKOFF + : _evidence_refresh_backoff * 2)); + _last_evidence_refresh = now; + + log::info( + cat, + "Swarm {} was rejected by a node we had in it, refreshing the pool (next " + "rejection-driven refresh no sooner than {}s from now).", + swarm_pubkey.hex(), + _evidence_refresh_backoff.count()); + _refresh_snode_cache(); + } + } + + if (!cb) + return; + + // `_refresh_snode_cache` runs inline here (we are on the loop thread) and can decline to + // start, so it is the refresh id - not the call above - that says whether there is anything + // to wait for. A callback queued against a refresh that never started never runs, and on + // this path that would strand the request that triggered the rejection. + if (_current_snode_cache_refresh_id) + _after_snode_cache_refresh.push_back(std::move(cb)); + else + cb(); + }); +} + std::vector SnodePool::get_unused_nodes( size_t count, const std::vector& exclude_nodes) { // Kick of a cache refresh in the background if needed (call_soon to ensure it is scheduled diff --git a/tests/test_snode_pool.cpp b/tests/test_snode_pool.cpp index d470ed193..05073eff0 100644 --- a/tests/test_snode_pool.cpp +++ b/tests/test_snode_pool.cpp @@ -69,6 +69,30 @@ class TestSnodePool : public SnodePool { // loop thread void update_cache(std::vector nodes) { _update_cache("test", std::move(nodes)); } + bool debug_refresh_in_progress() { + return _loop->call_get([this] { return _current_snode_cache_refresh_id.has_value(); }); + } + + // Backdates the pool snapshot so the age-based policies can be exercised without waiting + void debug_age_pool(std::chrono::seconds by) { + _loop->call_get([this, by] { _last_snode_cache_update -= by; }); + } + + void debug_age_evidence_refresh(std::chrono::seconds by) { + _loop->call_get([this, by] { _last_evidence_refresh -= by; }); + } + + std::chrono::seconds debug_evidence_backoff() { + return _loop->call_get([this] { return _evidence_refresh_backoff; }); + } + + // Runs `fn` on the loop thread, which is what makes `get_swarm` / `invalidate_swarm` resolve + // inline rather than being queued: a test observing them from off the loop has no ordering + // guarantee at all + void debug_run_on_loop(std::function fn) { + _loop->call_get([fn = std::move(fn)] { fn(); }); + } + void debug_on_refresh_complete(std::vector> raw_results) { auto total_requests = static_cast(raw_results.size()); _loop->call_get([&] { @@ -77,6 +101,19 @@ class TestSnodePool : public SnodePool { } }; +// `TestSnodePool` stubs out `refresh_if_needed` so tests don't kick off real refreshes; the swarm +// invalidation test needs the real age-based policy to compare against, so it uses this instead +class TestSnodePoolAgePolicy : public TestSnodePool { + public: + using TestSnodePool::TestSnodePool; + + void refresh_if_needed( + const std::vector& in_use_nodes, + std::function on_refresh_complete = nullptr) override { + SnodePool::refresh_if_needed(in_use_nodes, std::move(on_refresh_complete)); + } +}; + // Encodes nodes the way the storage server returns them, so they can be fed to // `_on_refresh_complete`: 51 bytes per node, all multi-byte fields big-endian std::vector to_snode_cache_bin(const std::vector& nodes) { @@ -336,3 +373,109 @@ TEST_CASE("Network", "[network][refresh_min_cache_size]") { snode_pool->debug_on_refresh_complete({to_snode_cache_bin(enough)}); CHECK(snode_pool->size() == 12); } + +TEST_CASE("Network", "[network][invalidate_swarm]") { + session::network::config::SnodePool pool_config{ + .cache_expiration = 2h, + .cache_min_lifetime = 2s, + .enforce_subnet_diversity = false, + .retry_delay = network::opt::retry_delay{50ms, 200ms}, + .netid = opt::netid::Target::testnet, + .cache_min_size = 12, + .cache_min_swarm_size = 3, + .cache_num_nodes_to_use_for_refresh = 1, + .cache_min_num_refresh_presence_to_include_node = 1, + .cache_node_strike_threshold = 3}; + + auto ed_pk_before = "4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46ab7"_hexbytes; + auto ed_pk_after = "5ea34e72bb044654a6a23675690ef5ffaaf1656b02f93fb76655f9cbdbe89876"_hexbytes; + + // Two snapshots of the same two swarms, with an entirely different set of nodes serving them. + // Which swarm the pubkey lands in doesn't matter; what matters is that the correct answer + // changed and no node is common to both. + auto pool_snapshot = [](const std::vector& ed_pk, uint8_t subnet) { + std::vector nodes; + + for (uint16_t i = 0; i < 12; ++i) + nodes.emplace_back(service_node{ + ed25519_pubkey::from_bytes(ed_pk), + oxen::quic::ipv4{"192.168.{}.{}"_format(subnet, i)}, + static_cast(20000 + i), + static_cast(30000 + i), + {2, 11, 0}, + static_cast(i < 6 ? 0 : 1)}); + + return nodes; + }; + auto nodes_before = pool_snapshot(ed_pk_before, 0); + auto nodes_after = pool_snapshot(ed_pk_after, 1); + + auto loop = std::make_shared(); + auto disk_loop = std::make_shared(); + auto snode_pool = std::make_shared(pool_config, loop, disk_loop); + snode_pool->update_cache(nodes_before); + snode_pool->debug_age_pool(10min); + + auto swarm_pubkey = x25519_pubkey::from_hex( + "0000000000000000000000000000000000000000000000000000000000000000"); + + std::vector rejected_swarm, recovered_swarm; + bool age_policy_started_refresh = false, recovery_ran = false; + + snode_pool->debug_run_on_loop([&] { + snode_pool->get_swarm( + swarm_pubkey, true, [&](swarm::swarm_id_t, std::vector nodes) { + rejected_swarm = std::move(nodes); + }); + + // Asking by age is what the 421 path used to do, and on a cache this far short of + // `cache_expiration` it declines - which is what left the disproven mapping in place + snode_pool->refresh_if_needed({}); + age_policy_started_refresh = snode_pool->debug_refresh_in_progress(); + + snode_pool->invalidate_swarm(swarm_pubkey, [&] { + recovery_ran = true; + snode_pool->get_swarm( + swarm_pubkey, true, [&](swarm::swarm_id_t, std::vector nodes) { + recovered_swarm = std::move(nodes); + }); + }); + }); + + REQUIRE_FALSE(rejected_swarm.empty()); + CHECK_FALSE(age_policy_started_refresh); + + // The rejection has to produce a refresh, and the retry has to wait for it rather than being + // handed the answer that was just rejected + REQUIRE(snode_pool->debug_refresh_in_progress()); + CHECK_FALSE(recovery_ran); + + snode_pool->update_cache(nodes_after); + REQUIRE(recovery_ran); + REQUIRE_FALSE(recovered_swarm.empty()); + + for (const auto& node : recovered_swarm) + CHECK(std::ranges::find(rejected_swarm, node) == rejected_swarm.end()); + + // A rejection that arrives after we already refreshed on one has to be held off, or a node that + // rejects everything keeps a client refreshing from three nodes forever + CHECK(snode_pool->debug_evidence_backoff() == 1min); + snode_pool->debug_age_pool(10min); + snode_pool->debug_run_on_loop([&] { snode_pool->invalidate_swarm(swarm_pubkey); }); + CHECK_FALSE(snode_pool->debug_refresh_in_progress()); + + // Once it has elapsed the next one runs, and the one after it waits twice as long again + snode_pool->debug_age_evidence_refresh(90s); + snode_pool->debug_run_on_loop([&] { snode_pool->invalidate_swarm(swarm_pubkey); }); + CHECK(snode_pool->debug_refresh_in_progress()); + CHECK(snode_pool->debug_evidence_backoff() == 2min); + + // Staying quiet for twice the interval means the next incident starts from the base delay + // rather than inheriting an escalated one + snode_pool->update_cache(nodes_after); + snode_pool->debug_age_pool(10min); + snode_pool->debug_age_evidence_refresh(10min); + snode_pool->debug_run_on_loop([&] { snode_pool->invalidate_swarm(swarm_pubkey); }); + CHECK(snode_pool->debug_refresh_in_progress()); + CHECK(snode_pool->debug_evidence_backoff() == 1min); +} From 329f8b9d209e2a51830a9ad2c1e39af2c8fde93d Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 14 Sep 2026 09:15:48 +1000 Subject: [PATCH 2/2] Follow a 421's redirect instead of refreshing the whole pool Refreshing on a 421 recovers the account, but it means a swarm change has every client of that swarm fetch the full node list - 51 bytes per node from each of `cache_num_nodes_to_use_for_refresh` nodes - where nothing made them fetch it at all before. The backoff added with `invalidate_swarm` bounds how often one client repeats that; it does nothing about the aggregate. A node rejecting a request for an account usually names the swarm it actually belongs to, which corrects the one mapping we know is wrong without fetching anything. `get_swarm` prefers such a redirect over its own calculation until the pool is refreshed, and the refresh stays as the fallback for a 421 that carries no usable redirect. Only the node pubkeys are read from the response, and only ones that resolve against the pool we fetched ourselves, so a redirect reaches registered service nodes we already know about and nothing else - it cannot invent a node or name an address of its own choosing. It can still choose which of those nodes we talk to, so: a redirect naming the swarm we already calculated is refused, fewer than `cache_min_swarm_size` resolved names is refused, three in a row without an intervening refresh is refused, and a pool refresh drops every override it was correcting. The redirects are kept out of `_swarm_cache`, which stays a pure memo of `_all_swarms`. --- include/session/network/session_network.hpp | 5 +- include/session/network/snode_pool.hpp | 19 +++ src/network/session_network.cpp | 138 ++++++++++++-------- src/network/snode_pool.cpp | 92 +++++++++++++ tests/test_snode_pool.cpp | 124 ++++++++++++++++++ 5 files changed, 325 insertions(+), 53 deletions(-) diff --git a/include/session/network/session_network.hpp b/include/session/network/session_network.hpp index 9c8f07eb9..cc495e9c8 100644 --- a/include/session/network/session_network.hpp +++ b/include/session/network/session_network.hpp @@ -116,7 +116,10 @@ class Network : public std::enable_shared_from_this { void _recalculate_status(); void _update_status(ConnectionStatus new_status); void _update_network_state(const std::string& body); - void _handle_421_retry(Request original_request, network_response_callback_t final_callback); + void _handle_421_retry( + Request original_request, + std::optional response_body, + network_response_callback_t final_callback); void _resync_clock( std::optional original_request, network_response_callback_t request_callback); diff --git a/include/session/network/snode_pool.hpp b/include/session/network/snode_pool.hpp index 22c0194f2..25995fc69 100644 --- a/include/session/network/snode_pool.hpp +++ b/include/session/network/snode_pool.hpp @@ -84,6 +84,18 @@ class SnodePool : public std::enable_shared_from_this { session::network::x25519_pubkey swarm_pubkey, std::function on_complete = nullptr); + // Records the swarm a node named when it rejected a request for `swarm_pubkey`, so the mapping + // can be corrected without refreshing the whole pool - a swarm change would otherwise have + // every client of that swarm fetch the full node list, where nothing made them before. + // + // Only the node pubkeys are taken from the response, and only ones that resolve against the + // pool we fetched ourselves, so a redirect can reach registered service nodes we already know + // and nothing else. Returns false when the redirect is unusable, which is the caller's cue to + // fall back to `invalidate_swarm`. + virtual bool record_swarm_redirect( + session::network::x25519_pubkey swarm_pubkey, + const std::vector& swarm_node_keys); + virtual void get_swarm( session::network::x25519_pubkey swarm_pubkey, bool ignore_strike_count, @@ -108,6 +120,13 @@ class SnodePool : public std::enable_shared_from_this { std::vector>> _all_swarms; std::unordered_map>> _swarm_cache; + + // Swarms a node has redirected us to, which `get_swarm` prefers over its own calculation. Kept + // apart from `_swarm_cache` deliberately: that is a memo of `_all_swarms` and stays one, where + // these are claims from outside that outrank it until the pool is refreshed. The count bounds + // how far a disagreement between nodes can bounce us before we go and refresh instead. + std::unordered_map, uint8_t>> + _swarm_overrides; std::map> _snode_strikes; bool _strikes_flush_scheduled = false; diff --git a/src/network/session_network.cpp b/src/network/session_network.cpp index 474ed48cf..24d4babce 100644 --- a/src/network/session_network.cpp +++ b/src/network/session_network.cpp @@ -1,6 +1,7 @@ #include "session/network/session_network.hpp" #include +#include #include #include @@ -482,7 +483,7 @@ void Network::send_request(Request request, network_response_callback_t callback // cache, the original request might succeed after this refresh so we should // just automatically retry if (final_status_code == 421) { - _handle_421_retry(std::move(original_req), std::move(cb)); + _handle_421_retry(std::move(original_req), std::move(body), std::move(cb)); return; } @@ -779,8 +780,41 @@ void Network::_update_network_state(const std::string& body) { // MARK: Specific Error Handling +// Pulls the node pubkeys out of a 421 body. Only the pubkeys: the rest of each record is contact +// information we deliberately don't read, so a redirect can't put us in touch with anything the +// pool hasn't already told us about. +static std::vector redirect_node_keys(const std::optional& body) { + std::vector keys; + + if (!body) + return keys; + + try { + auto json = nlohmann::json::parse(*body); + + if (!json.contains("snodes") || !json["snodes"].is_array()) + return keys; + + for (const auto& entry : json["snodes"]) { + if (!entry.contains("pubkey_ed25519") || !entry["pubkey_ed25519"].is_string()) + continue; + + auto hex = entry["pubkey_ed25519"].get(); + + if (hex.size() == 64 && oxenc::is_hex(hex)) + keys.push_back(ed25519_pubkey::from_hex(hex)); + } + } catch (const std::exception& e) { + log::debug(cat, "Could not read a swarm redirect from the 421 body: {}", e.what()); + } + + return keys; +} + void Network::_handle_421_retry( - Request original_request, network_response_callback_t final_callback) { + Request original_request, + std::optional response_body, + network_response_callback_t final_callback) { if (original_request.retry_count >= config.redirect_retry_count) { log::error( cat, @@ -830,56 +864,56 @@ void Network::_handle_421_retry( auto failed_node_copy = *original_dest_node; auto swarm_pubkey = *original_request.swarm_pubkey; - // A node in the swarm we resolved has told us the account isn't in its swarm, which is proof - // the mapping is wrong now rather than a reason to suspect it might be; asking for a refresh - // by age (`refresh_if_needed`) would decline for as long as `cache_expiration`, leaving every - // request for this account failing until then. - _snode_pool->invalidate_swarm( - swarm_pubkey, - [this, - swarm_pubkey, - req_to_retry = std::move(original_request), - cb = std::move(final_callback), - failed_node = failed_node_copy] { - _snode_pool->get_swarm( - swarm_pubkey, - false, - [this, - req_to_retry = std::move(req_to_retry), - cb = std::move(cb), - failed_node](swarm::swarm_id_t, std::vector swarm_nodes) { - // Extract a single random index from the vector indices, but excluding - // the index of the failing node: - size_t new_target; - auto out = std::ranges::sample( - std::views::iota(0, static_cast(swarm_nodes.size())) | - std::views::filter([&](int i) { - return swarm_nodes[i] != failed_node; - }), - &new_target, - 1, - csrng); - - if (out == &new_target) - return cb( - false, - false, - ERROR_MISDIRECTED_REQUEST, - {content_type_plain_text}, - "421 Misdirected Request, but no other nodes in swarm to " - "retry"); - - log::info( - cat, - "Request {} retrying 421 error on new node {}.", - req_to_retry.request_id, - swarm_nodes[new_target].to_string()); - auto final_request = req_to_retry; - final_request.retry_count++; - final_request.destination = std::move(swarm_nodes[new_target]); - this->send_request(std::move(final_request), std::move(cb)); - }); - }); + auto retry_on_resolved_swarm = [this, + swarm_pubkey, + req_to_retry = std::move(original_request), + cb = std::move(final_callback), + failed_node = failed_node_copy]() mutable { + _snode_pool->get_swarm( + swarm_pubkey, + false, + [this, req_to_retry = std::move(req_to_retry), cb = std::move(cb), failed_node]( + swarm::swarm_id_t, std::vector swarm_nodes) { + // Extract a single random index from the vector indices, but excluding + // the index of the failing node: + size_t new_target; + auto out = std::ranges::sample( + std::views::iota(0, static_cast(swarm_nodes.size())) | + std::views::filter( + [&](int i) { return swarm_nodes[i] != failed_node; }), + &new_target, + 1, + csrng); + + if (out == &new_target) + return cb( + false, + false, + ERROR_MISDIRECTED_REQUEST, + {content_type_plain_text}, + "421 Misdirected Request, but no other nodes in swarm to " + "retry"); + + log::info( + cat, + "Request {} retrying 421 error on new node {}.", + req_to_retry.request_id, + swarm_nodes[new_target].to_string()); + auto final_request = req_to_retry; + final_request.retry_count++; + final_request.destination = std::move(swarm_nodes[new_target]); + this->send_request(std::move(final_request), std::move(cb)); + }); + }; + + // A node that rejects a request for an account usually names the swarm it actually belongs to. + // Taking its word (bounded - see `record_swarm_redirect`) fixes the one mapping we know is + // wrong, where refreshing has every client of a changed swarm fetch the full node list for it. + if (_snode_pool->record_swarm_redirect(swarm_pubkey, redirect_node_keys(response_body))) + return retry_on_resolved_swarm(); + + // No usable redirect, so the pool itself is the only thing that can put this right + _snode_pool->invalidate_swarm(swarm_pubkey, std::move(retry_on_resolved_swarm)); } void Network::_resync_clock( diff --git a/src/network/snode_pool.cpp b/src/network/snode_pool.cpp index 273a0a0a2..d41ce44d2 100644 --- a/src/network/snode_pool.cpp +++ b/src/network/snode_pool.cpp @@ -54,6 +54,10 @@ namespace { // is not an acceptable resting state for a client on a bad connection, and without this it is // exactly what a persistent rejection source would produce. const std::chrono::seconds EVIDENCE_REFRESH_BASE_BACKOFF = 1min; + + // Two nodes disagreeing about who owns a swarm can redirect us back and forth; after this many + // in a row without an intervening pool refresh, stop believing them and go and refresh. + constexpr uint8_t MAX_CONSECUTIVE_SWARM_REDIRECTS = 3; } // namespace SnodePool::SnodePool( @@ -889,6 +893,11 @@ void SnodePool::_update_cache(std::string refresh_id, std::vector _snode_cache = std::move(nodes); _all_swarms = swarm::generate_swarms(_snode_cache); _swarm_cache.clear(); + + // The pool these redirects were correcting has been replaced, so they have outlived what + // they were for. Keeping them would let one node's claim outlast the ground truth that + // would have overruled it. + _swarm_overrides.clear(); _last_snode_cache_update = std::chrono::system_clock::now(); // Reset all failure and refresh-in-progress state @@ -970,6 +979,7 @@ void SnodePool::clear_cache() { _snode_cache = {}; _all_swarms = {}; _swarm_cache = {}; + _swarm_overrides = {}; _disk_loop->call([path = _snode_cache_file_path] { SnodePool::_clear_disk_cache(path); }); }); @@ -1193,6 +1203,77 @@ void SnodePool::invalidate_swarm(x25519_pubkey swarm_pubkey, std::function& swarm_node_keys) { + return _loop->call_get([this, swarm_pubkey, &swarm_node_keys] { + if (_snode_cache.empty() || _all_swarms.empty()) + return false; + + // Resolving the names against our own pool is what makes a redirect safe to act on: the + // responding node chooses which registered nodes we end up talking to, but it cannot invent + // a node or point us at an address of its own choosing, because we never read the contact + // details it sent + std::unordered_set named{swarm_node_keys.begin(), swarm_node_keys.end()}; + std::vector resolved; + + for (const auto& node : _snode_cache) + if (named.count(node.remote_pubkey)) + resolved.push_back(node); + + if (resolved.size() < _config.cache_min_swarm_size) { + log::debug( + cat, + "Ignoring redirect for {}: only {}/{} named nodes are in our pool.", + swarm_pubkey.hex(), + resolved.size(), + swarm_node_keys.size()); + return false; + } + + // A redirect naming the swarm we already calculated is the node contradicting itself - it + // rejected the request and then pointed us back at the nodes we asked through. Refusing it + // sends the caller off to refresh the pool, which is the only thing that can help + auto computed = swarm::get_swarm(swarm_pubkey, _all_swarms); + if (computed.second.size() == resolved.size()) { + std::unordered_set computed_keys; + for (const auto& node : computed.second) + computed_keys.insert(node.remote_pubkey); + + if (std::ranges::all_of(resolved, [&computed_keys](const auto& node) { + return computed_keys.count(node.remote_pubkey) > 0; + })) { + log::debug( + cat, + "Ignoring redirect for {}: it names the swarm we already calculated.", + swarm_pubkey.hex()); + return false; + } + } + + auto& [nodes, redirects] = _swarm_overrides[swarm_pubkey]; + + if (++redirects > MAX_CONSECUTIVE_SWARM_REDIRECTS) { + log::warning( + cat, + "Dropping redirects for {} after {} in a row; refreshing the pool instead.", + swarm_pubkey.hex(), + MAX_CONSECUTIVE_SWARM_REDIRECTS); + _swarm_overrides.erase(swarm_pubkey); + return false; + } + + log::info( + cat, + "Redirected to a swarm of {} nodes for {} ({} of at most {}).", + resolved.size(), + swarm_pubkey.hex(), + redirects, + MAX_CONSECUTIVE_SWARM_REDIRECTS); + nodes = std::move(resolved); + return true; + }); +} + std::vector SnodePool::get_unused_nodes( size_t count, const std::vector& exclude_nodes) { // Kick of a cache refresh in the background if needed (call_soon to ensure it is scheduled @@ -1304,6 +1385,17 @@ void SnodePool::get_swarm( return nodes; }; + // A node that rejected a request for this account told us where it belongs, which beats + // anything we can work out from a pool we now know is behind. There is no swarm id to give + // back - a redirect names members, not an id - and nothing downstream reads it. + if (auto it = _swarm_overrides.find(swarm_pubkey); it != _swarm_overrides.end()) { + const auto& swarm_nodes = it->second.first; + + return cb( + swarm::INVALID_SWARM_ID, + (ignore_strike_count ? swarm_nodes : filter_by_strikes(swarm_nodes))); + } + // Check the in-memory swarm cache first if (auto it = _swarm_cache.find(swarm_pubkey); it != _swarm_cache.end()) { const auto& swarm_nodes = it->second.second; diff --git a/tests/test_snode_pool.cpp b/tests/test_snode_pool.cpp index 05073eff0..87c0e3994 100644 --- a/tests/test_snode_pool.cpp +++ b/tests/test_snode_pool.cpp @@ -479,3 +479,127 @@ TEST_CASE("Network", "[network][invalidate_swarm]") { CHECK(snode_pool->debug_refresh_in_progress()); CHECK(snode_pool->debug_evidence_backoff() == 1min); } + +TEST_CASE("Network", "[network][swarm_redirect]") { + session::network::config::SnodePool pool_config{ + .cache_expiration = 2h, + .cache_min_lifetime = 2s, + .enforce_subnet_diversity = false, + .retry_delay = network::opt::retry_delay{50ms, 200ms}, + .netid = opt::netid::Target::testnet, + .cache_min_size = 12, + .cache_min_swarm_size = 3, + .cache_num_nodes_to_use_for_refresh = 1, + .cache_min_num_refresh_presence_to_include_node = 1, + .cache_node_strike_threshold = 3}; + + // A redirect is resolved by node pubkey, so every node needs its own + auto key_for = [](uint16_t i) { + return ed25519_pubkey::from_hex( + "4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46a{:02x}"_format(i)); + }; + std::vector snode_cache; + + for (uint16_t i = 0; i < 12; ++i) + snode_cache.emplace_back(service_node{ + key_for(i), + oxen::quic::ipv4{"192.168.0.{}"_format(i)}, + static_cast(20000 + i), + static_cast(30000 + i), + {2, 11, 0}, + static_cast(i < 6 ? 0 : 1)}); + + auto loop = std::make_shared(); + auto disk_loop = std::make_shared(); + auto snode_pool = std::make_shared(pool_config, loop, disk_loop); + snode_pool->update_cache(snode_cache); + + // Old enough that the fallback would refresh, so "no refresh happened" below means the redirect + // was taken rather than the pool being too fresh to bother + snode_pool->debug_age_pool(10min); + + auto swarm_pubkey = x25519_pubkey::from_hex( + "0000000000000000000000000000000000000000000000000000000000000000"); + + std::vector calculated; + snode_pool->debug_run_on_loop([&] { + snode_pool->get_swarm( + swarm_pubkey, true, [&](swarm::swarm_id_t, std::vector nodes) { + calculated = std::move(nodes); + }); + }); + REQUIRE(calculated.size() == 6); + + // Everything the calculation didn't pick, which is what a real redirect would be naming + std::vector elsewhere; + for (const auto& node : snode_cache) + if (std::ranges::find(calculated, node) == calculated.end()) + elsewhere.push_back(node); + REQUIRE(elsewhere.size() == 6); + + // Membership is the claim in every comparison below; the pool is reshuffled on each refresh and + // a redirect is resolved in pool order, so neither side has an order worth asserting + auto sorted = [](std::vector nodes) { + std::ranges::sort(nodes); + return nodes; + }; + auto keys_of = [](const std::vector& nodes, size_t count) { + std::vector keys; + for (size_t i = 0; i < count && i < nodes.size(); ++i) + keys.push_back(nodes[i].remote_pubkey); + return keys; + }; + + // Nodes we've never heard of can't redirect us anywhere - this is what stops a response putting + // us in touch with something that isn't a registered service node + CHECK_FALSE(snode_pool->record_swarm_redirect(swarm_pubkey, {key_for(200), key_for(201)})); + + // Too few of the named nodes resolve to be a swarm + CHECK_FALSE(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 2))); + + // Naming the swarm we already calculated is the node contradicting itself + CHECK_FALSE(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(calculated, 6))); + + // A usable redirect is taken, and - the point of all this - no pool refresh is started for it + REQUIRE(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 4))); + CHECK_FALSE(snode_pool->debug_refresh_in_progress()); + + std::vector redirected; + snode_pool->debug_run_on_loop([&] { + snode_pool->get_swarm( + swarm_pubkey, true, [&](swarm::swarm_id_t, std::vector nodes) { + redirected = std::move(nodes); + }); + }); + CHECK(sorted(redirected) == + sorted(std::vector(elsewhere.begin(), elsewhere.begin() + 4))); + CHECK_FALSE(snode_pool->debug_refresh_in_progress()); + + // Nodes that keep disagreeing must not bounce us forever; after the third we stop believing + // them and the calculated swarm is back, for the caller to fall back on refreshing the pool + CHECK(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 5))); + CHECK(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 4))); + CHECK_FALSE(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 5))); + + std::vector after_giving_up; + snode_pool->debug_run_on_loop([&] { + snode_pool->get_swarm( + swarm_pubkey, true, [&](swarm::swarm_id_t, std::vector nodes) { + after_giving_up = std::move(nodes); + }); + }); + CHECK(sorted(after_giving_up) == sorted(calculated)); + + // A refreshed pool is ground truth again, so redirects correcting the old one are dropped + REQUIRE(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 4))); + snode_pool->update_cache(snode_cache); + + std::vector after_refresh; + snode_pool->debug_run_on_loop([&] { + snode_pool->get_swarm( + swarm_pubkey, true, [&](swarm::swarm_id_t, std::vector nodes) { + after_refresh = std::move(nodes); + }); + }); + CHECK(sorted(after_refresh) == sorted(calculated)); +}