From e5586cb33315f9519123c5a6a5436a3aa3b1bf6b Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Mon, 31 Aug 2026 20:26:46 +0000 Subject: [PATCH] fix(lookup): count a request of our own that came back apart from a duplicate Dropping our own flooded request when a bloom false positive circulates it back to us recorded the drop under `req_duplicate`, whose documented meaning is that a peer resent a request. The two events are not the same, and only one of them says anything about the peer that delivered the frame. A returning copy has a nonzero floor in healthy operation and rises with the bloom fill ratio, so folding it into `req_duplicate` puts a permanent number on a counter an operator reads as neighbour misbehaviour, and leaves no way to tell a resending peer from this node's own fan-out coming home. It now carries its own rejection reason and counter, `req_own_loopback`, shown in fipstop as "Own Loopback", and its own log line naming the target. The control socket's show routing fixture gains the field. Two comments record what the guard rests on. `request.origin` is the obvious cheaper identity test and is unusable: 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. And the test reaches only as far as the last MAX_RECORDED_IDS a target's ladder issued, so a ladder configured with more rungs than that loses its earliest ids. The request-side regression test now builds its request with this node's own address as the origin, which is the shape the defect actually has. It was using a helper whose origin is a third party, so it exercised the guard without reproducing the case. --- CHANGELOG.md | 11 +++++++++++ src/bin/fipstop/ui/routing.rs | 1 + src/control/snapshots/show_routing.json | 2 ++ src/node/handlers/lookup.rs | 17 +++++++++++++++++ src/node/metrics.rs | 3 +++ src/node/reject.rs | 11 +++++++++++ src/node/stats.rs | 1 + src/proto/lookup/core.rs | 22 +++++++++++++++++++++- src/proto/lookup/tests/core.rs | 12 ++++++++++-- 9 files changed, 77 insertions(+), 3 deletions(-) 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));