diff --git a/CHANGELOG.md b/CHANGELOG.md index cb89e2e0..c026b52f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 succeeded, and the failure grew likelier as the bloom fill ratio rose. Contributed by Arjen. +- A lookup request of this node's own, returning to it, is no longer counted as + a duplicate from the peer that delivered it. The fix above drops that copy, + and it recorded the drop under the existing `req_duplicate` rejection, whose + documented meaning is that a peer resent a request. A returning copy has a + nonzero floor in healthy operation and rises with the bloom fill ratio, so + folding the two together put a permanent number on a counter an operator + reads as neighbour misbehaviour, and made the two events indistinguishable. + It now has its own rejection reason and counter, `req_own_loopback`, shown in + `fipstop` as "Own Loopback". `req_duplicate` returns to meaning only what it + says. + ## [0.5.0] - 2026-08-30 ### Added diff --git a/src/bin/fipstop/ui/routing.rs b/src/bin/fipstop/ui/routing.rs index 1961c8c5..1cae2ca5 100644 --- a/src/bin/fipstop/ui/routing.rs +++ b/src/bin/fipstop/ui/routing.rs @@ -189,6 +189,7 @@ fn draw_routing_stats( ("Deduplicated", lookup("req_deduplicated")), ("Target Is Us", lookup("req_target_is_us")), ("Duplicate", lookup("req_duplicate")), + ("Own Loopback", lookup("req_own_loopback")), ("Bloom Miss", lookup("req_bloom_miss")), ("Backoff Suppressed", lookup("req_backoff_suppressed")), ("Fwd Rate Limited", lookup("req_forward_rate_limited")), diff --git a/src/control/snapshots/show_routing.json b/src/control/snapshots/show_routing.json index 99e8ddd2..0e0685da 100644 --- a/src/control/snapshots/show_routing.json +++ b/src/control/snapshots/show_routing.json @@ -20,6 +20,7 @@ "req_forwarded": 0, "req_initiated": 0, "req_no_tree_peer": 0, + "req_own_loopback": 0, "req_received": 0, "req_sign_rate_limited": 0, "req_target_is_us": 0, @@ -96,6 +97,7 @@ "req_forwarded": 0, "req_initiated": 0, "req_no_tree_peer": 0, + "req_own_loopback": 0, "req_received": 0, "req_sign_rate_limited": 0, "req_target_is_us": 0, diff --git a/src/node/handlers/lookup.rs b/src/node/handlers/lookup.rs index 76ec77d1..90a45208 100644 --- a/src/node/handlers/lookup.rs +++ b/src/node/handlers/lookup.rs @@ -139,6 +139,23 @@ impl Node { "Duplicate LookupRequest, dropping" ); } + RequestOutcome::OwnRequestLooped => { + // Our own flooded request, come back to us through a bloom + // false positive. Counted apart from ReqDuplicate: that one + // describes a peer resending, and this one describes our own + // fan-out returning, so folding them together would put a + // permanent healthy floor on a counter an operator reads as + // neighbour misbehaviour. + self.metrics() + .lookup + .record_reject(DiscoveryReject::ReqOwnLoopback); + debug!( + request_id = request.request_id, + from = %self.peer_display_name(from), + target = %self.peer_display_name(&request.target), + "LookupRequest we originated came back to us, dropping" + ); + } RequestOutcome::RespondAsTarget => { // Answering costs a fresh Schnorr signature every time: the // proof is bound to the requester's request_id, so it cannot diff --git a/src/node/metrics.rs b/src/node/metrics.rs index dc40d1ba..b5727d04 100644 --- a/src/node/metrics.rs +++ b/src/node/metrics.rs @@ -277,6 +277,7 @@ pub struct LookupMetrics { pub req_received: Padded, pub req_decode_error: Counter, pub req_duplicate: Counter, + pub req_own_loopback: Counter, pub req_dedup_cache_full: Counter, pub req_dedup_evicted: Counter, pub req_sign_rate_limited: Counter, @@ -309,6 +310,7 @@ impl LookupMetrics { match reason { DiscoveryReject::ReqDecodeError => self.req_decode_error.inc(), DiscoveryReject::ReqDuplicate => self.req_duplicate.inc(), + DiscoveryReject::ReqOwnLoopback => self.req_own_loopback.inc(), DiscoveryReject::ReqDedupCacheFull => self.req_dedup_cache_full.inc(), DiscoveryReject::ReqSignRateLimited => self.req_sign_rate_limited.inc(), DiscoveryReject::ReqTtlExhausted => self.req_ttl_exhausted.inc(), @@ -326,6 +328,7 @@ impl LookupMetrics { req_received: self.req_received.get(), req_decode_error: self.req_decode_error.get(), req_duplicate: self.req_duplicate.get(), + req_own_loopback: self.req_own_loopback.get(), req_dedup_cache_full: self.req_dedup_cache_full.get(), req_dedup_evicted: self.req_dedup_evicted.get(), req_sign_rate_limited: self.req_sign_rate_limited.get(), diff --git a/src/node/reject.rs b/src/node/reject.rs index fbaaeab2..40bfbd9d 100644 --- a/src/node/reject.rs +++ b/src/node/reject.rs @@ -117,6 +117,16 @@ pub enum DiscoveryReject { /// Tracked via /// [`DiscoveryStats::req_duplicate`](crate::node::stats::DiscoveryStats). ReqDuplicate, + /// A request this node originated, flooded to its bloom-matching tree + /// peers and circulated back to it by one of them. Dropped without being + /// recorded, so the answer is accepted here rather than reverse-path + /// forwarded to the peer that looped it. Tracked via + /// [`DiscoveryStats::req_own_loopback`](crate::node::stats::DiscoveryStats). + /// + /// **This has a nonzero floor in healthy operation** and rises with the + /// bloom fill ratio. It says nothing about the peer that delivered the + /// copy, which is why it is not counted as [`Self::ReqDuplicate`]. + ReqOwnLoopback, /// Request dedup cache (`recent_requests`) is at capacity, so the /// `LookupRequest` is dropped without being forwarded. Tracked via /// [`DiscoveryStats::req_dedup_cache_full`](crate::node::stats::DiscoveryStats). @@ -398,6 +408,7 @@ mod tests { let variants = [ DiscoveryReject::ReqDecodeError, DiscoveryReject::ReqDuplicate, + DiscoveryReject::ReqOwnLoopback, DiscoveryReject::ReqDedupCacheFull, DiscoveryReject::ReqTtlExhausted, DiscoveryReject::ReqSignRateLimited, diff --git a/src/node/stats.rs b/src/node/stats.rs index 8367d857..0d82f62f 100644 --- a/src/node/stats.rs +++ b/src/node/stats.rs @@ -334,6 +334,7 @@ pub struct LookupStatsSnapshot { pub req_received: u64, pub req_decode_error: u64, pub req_duplicate: u64, + pub req_own_loopback: u64, pub req_dedup_cache_full: u64, pub req_dedup_evicted: u64, pub req_sign_rate_limited: u64, diff --git a/src/proto/lookup/core.rs b/src/proto/lookup/core.rs index c3eb2315..2aa57ac2 100644 --- a/src/proto/lookup/core.rs +++ b/src/proto/lookup/core.rs @@ -148,6 +148,12 @@ pub(crate) fn plan_initiate(request: &LookupRequest, rv: &impl RoutingView) -> V pub(crate) enum RequestOutcome { /// request_id already in the dedup cache — drop. Duplicate, + /// A request this node originated, looped back to it — drop without + /// recording. Kept separate from `Duplicate`, which means another node + /// resent a request, so the two do not share a rejection counter: this + /// one has a nonzero floor in healthy operation and says nothing about + /// the peer that delivered it. + OwnRequestLooped, /// We are the lookup target — the shell generates + sends the response. RespondAsTarget, /// Forward the request onward (the shell calls the forward planner). @@ -267,13 +273,27 @@ pub(crate) fn classify_request( // request instead of being accepted here. Dropping it as the duplicate it // is also stops the copy being forwarded a second time: our flood already // reached the peers that could carry it. + // + // `request.origin` looks like the cheaper identity test and cannot be + // used. It is unsigned and set by whoever sends the frame, so a peer + // could put this node's address on any request and make it refuse to + // transit that request. An id has to have been issued here to match, + // which is what makes this test safe to drop on. + // + // The test reaches only as far as `PendingLookup::ids`, which keeps the + // last `MAX_RECORDED_IDS` that a target's ladder issued. A ladder + // configured with more rungs than that loses its earliest ids, and a + // returning copy of one of those attempts is recorded as transit again. + // The bound is inherited from the originator test in `classify_response` + // rather than introduced here, and a reply to such an attempt was already + // being dropped as unsolicited. if lookup .pending_lookups .get(&request.target) .is_some_and(|pending| pending.matches(request.request_id)) { return Classification { - outcome: RequestOutcome::Duplicate, + outcome: RequestOutcome::OwnRequestLooped, evicted: None, }; } diff --git a/src/proto/lookup/tests/core.rs b/src/proto/lookup/tests/core.rs index 43dd9c13..66f4e21c 100644 --- a/src/proto/lookup/tests/core.rs +++ b/src/proto/lookup/tests/core.rs @@ -489,7 +489,12 @@ fn classify_request_drops_our_own_request_looped_back_to_us() { pending.record(77); lookup.pending_lookups.insert(target, pending); - let request = make_request_id(77, target, 3); + // Built here rather than through `make_request_id`, whose origin is a + // third party: the defect is our *own* request returning, so the request + // under test has to carry our address as its origin. The guard keys on + // the id and ignores the origin, so this asserts the same behaviour the + // helper would; it asserts it on the shape the defect actually has. + let request = LookupRequest::new(77, target, my_addr, TreeCoordinate::root(my_addr), 3, 0); let classification = classify_request( &mut lookup, &request, @@ -500,7 +505,10 @@ fn classify_request_drops_our_own_request_looped_back_to_us() { 4096, 1, ); - assert!(matches!(classification.outcome, RequestOutcome::Duplicate)); + assert!(matches!( + classification.outcome, + RequestOutcome::OwnRequestLooped + )); assert!(classification.evicted.is_none()); // Crucially, our own id must not enter the transit dedup cache. assert!(!lookup.recent_requests.contains_key(&77));