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.
This commit is contained in:
Johnathan Corgan
2026-08-31 20:26:46 +00:00
parent 2787062436
commit e5586cb333
9 changed files with 77 additions and 3 deletions
+11
View File
@@ -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
+1
View File
@@ -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")),
+2
View File
@@ -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,
+17
View File
@@ -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
+3
View File
@@ -277,6 +277,7 @@ pub struct LookupMetrics {
pub req_received: Padded<Counter>,
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(),
+11
View File
@@ -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,
+1
View File
@@ -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,
+21 -1
View File
@@ -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,
};
}
+10 -2
View File
@@ -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));