Test and document where the own-request loop guard stops recognising an id

The guard that drops a returning copy of this node's own lookup request
recognises an id only while that lookup is outstanding and the id is
among the last eight its ladder issued. Nothing pinned either limit: the
existing test covers only an id inside the window of a live lookup. Two
new tests do. A copy arriving after the lookup timed out is recorded and
forwarded as transit, and its next copy is a duplicate. A copy of an
attempt older than the recorded window is likewise recorded and
forwarded, while a recent id on the same lookup is still dropped as our
own.

The mesh operation design said a node tests its own outstanding lookups
before consulting recent_requests, and that a returning copy of the
originator's request never enters the transit cache. The first is the
response path's order; the request path runs the dedup test first and
the own-request check second, where the order is immaterial because an
id the originator check recognises is never in the dedup cache. The
second holds only while the check recognises the id. The design now
says both, and how far the check reaches, and its heading no longer
says the originator check runs first.

When a node's own looped-back lookup request got its own counter, the
comments justified the split by saying req_duplicate means a peer resent
a request, and that the own-loop counter says nothing about the peer.
Neither holds. The duplicate test is on the request id alone, so one
flood arriving through two neighbours is counted there too, and no
discovery counter is per peer. Justify the split by cause instead: the
node's own fan-out returning, versus a request id the node has already
recorded arriving again. Say that neither counter identifies the
delivering peer, that own-request loopbacks come back through bloom
false positives and so rise with the filter's fill ratio, and that an
own copy outside the loop check's reach is recorded and forwarded as
transit, with a later copy counted as a duplicate.

docs/design/fips-mesh-operation.md

@@ -272,18 +272,28 @@ follows the same path as the request. Greedy tree routing toward the
 `origin_coords` is used only as a fallback if the reverse-path entry has
 expired.
 
-**Originator check first**: a node tests its own outstanding lookups before
-consulting `recent_requests`. A request is flooded to every bloom-matching
-tree peer, and a bloom false positive can send a copy out into the wider
-network and back to the originator, which would otherwise file its own
-`request_id` as an ordinary transit entry and relay its own answer away. The
-originator arm therefore wins: a response naming a target with a lookup
+**Originator check**: on the response path, a node tests its own outstanding
+lookups before consulting `recent_requests`. A request is flooded to every
+bloom-matching tree peer, and a bloom false positive can send a copy out into
+the wider network and back to the originator, which would otherwise file its
+own `request_id` as an ordinary transit entry and relay its own answer away.
+The originator arm therefore wins: a response naming a target with a lookup
 outstanding, carrying a `request_id` that lookup issued, is accepted here
-whatever the dedup cache holds. A returning copy of the request is likewise
-dropped rather than recorded, so the originator's id never enters the transit
-cache. The drop is counted as `req_own_loopback` rather than as a duplicate: a
-returning copy has a nonzero floor in healthy operation and says nothing about
-the peer that delivered it.
+whatever the dedup cache holds. A returning copy of the request is dropped
+rather than recorded, so while the check recognises the id, it never enters the
+transit cache. The drop is counted as `req_own_loopback` rather than as a
+duplicate, because the cause differs: the node's own fan-out returning, not a
+request id it has already recorded arriving again. Neither counter identifies
+the peer that delivered the copy. The request path runs the two tests the other
+way round, the dedup test first, and the order is immaterial there: an id the
+originator check recognises is never in `recent_requests`, because the
+originator records the ids it issues only in its pending lookups and a request
+is recorded only after it has passed the check. Separately, the check
+recognises an id only while its lookup is outstanding and the id is among the
+last `MAX_RECORDED_IDS` (eight) that the lookup's retry ladder issued for that
+target. A copy that returns after the lookup completed or timed out, or a copy
+of an older attempt on a longer ladder, is recorded and forwarded as transit,
+and a later copy of it is dropped as a duplicate.
 
 **Response-forwarded flag**: Each `recent_requests` entry tracks whether a
 response has already been forwarded for that `request_id`. If a second

src/node/handlers/lookup.rs

@@ -141,11 +141,11 @@ impl Node {
             }
             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.
+                // false positive. Counted apart from ReqDuplicate by cause:
+                // this is our own fan-out returning, that is a request id we
+                // already recorded, seen again. Neither counter says which
+                // peer delivered the copy; this debug line is the only
+                // per-peer record.
                 self.metrics()
                     .lookup
                     .record_reject(DiscoveryReject::ReqOwnLoopback);

src/node/reject.rs

@@ -123,9 +123,12 @@ pub enum DiscoveryReject {
     /// 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`].
+    /// **This can occur in healthy operation**: a bloom false positive is
+    /// enough to send a copy back, so the rate rises with the bloom fill
+    /// ratio. It is counted apart from [`Self::ReqDuplicate`] by cause: this
+    /// is the node's own fan-out returning, while `ReqDuplicate` is a request
+    /// id the node has already recorded, seen again. Neither identifies the
+    /// peer that delivered the copy.
     ReqOwnLoopback,
     /// Request dedup cache (`recent_requests`) is at capacity, so the
     /// `LookupRequest` is dropped without being forwarded. Tracked via

src/proto/lookup/core.rs

@@ -146,13 +146,19 @@ pub(crate) fn plan_initiate(request: &LookupRequest, rv: &impl RoutingView) -> V
 
 /// Classification of an inbound LookupRequest, decided from Lookup state.
 pub(crate) enum RequestOutcome {
-    /// request_id already in the dedup cache — drop.
+    /// request_id already in the dedup cache — drop. The test is on the id
+    /// alone, so one flood reaching this node through two neighbours lands
+    /// here just as a request sent twice does; it does not identify the peer
+    /// that delivered the copy.
     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.
+    /// recording. Kept apart from `Duplicate` by cause: this is the node's
+    /// own fan-out returning, while `Duplicate` is a request id the node has
+    /// already recorded, seen again. Neither identifies the delivering peer.
+    /// Recognised only while the lookup is outstanding and the id is among
+    /// the last `MAX_RECORDED_IDS` it issued; an own copy outside that reach
+    /// is recorded and forwarded as transit, and a later copy of it is
+    /// dropped as a duplicate.
     OwnRequestLooped,
     /// We are the lookup target — the shell generates + sends the response.
     RespondAsTarget,

src/proto/lookup/tests/core.rs

@@ -520,6 +520,95 @@ fn classify_request_drops_our_own_request_looped_back_to_us() {
     ));
 }
 
+#[test]
+fn classify_request_records_our_own_id_as_transit_once_its_lookup_has_timed_out() {
+    // The own-request guard reaches only as far as the pending lookup. Once
+    // the ladder times out and the entry is dropped, a late copy of our own
+    // request is indistinguishable from transit: it is recorded and forwarded,
+    // and a later copy of it is a duplicate.
+    let mut lookup = empty_lookup();
+    let looping_peer = make_node_addr(0x01);
+    let my_addr = make_node_addr(0x99);
+    let target = make_node_addr(0xAA);
+    let t0 = 1000u64;
+    let mut pending = PendingLookup::new(t0);
+    pending.record(77);
+    lookup.pending_lookups.insert(target, pending);
+
+    // A one-rung ladder of 1s: the entry times out at t0 + 1000.
+    let now = t0 + 1000;
+    let outcome = poll_pending(&mut lookup, now, &[1]);
+    assert_eq!(outcome.timeouts.len(), 1, "the lookup must time out");
+
+    let request = make_request_id(77, target, 3);
+    let first = classify_request(
+        &mut lookup,
+        &request,
+        &looping_peer,
+        &my_addr,
+        now,
+        5000,
+        4096,
+        1,
+    );
+    assert!(matches!(first.outcome, RequestOutcome::Forward));
+    assert!(lookup.recent_requests.contains_key(&77));
+
+    let second = classify_request(
+        &mut lookup,
+        &request,
+        &looping_peer,
+        &my_addr,
+        now,
+        5000,
+        4096,
+        1,
+    );
+    assert!(matches!(second.outcome, RequestOutcome::Duplicate));
+}
+
+#[test]
+fn classify_request_records_an_own_id_older_than_the_recorded_window_as_transit() {
+    // A pending lookup keeps only the last MAX_RECORDED_IDS (eight) ids its
+    // ladder issued. A returning copy of an earlier attempt is outside the
+    // guard's reach and goes through as transit; a recent one is still caught.
+    let mut lookup = empty_lookup();
+    let looping_peer = make_node_addr(0x01);
+    let my_addr = make_node_addr(0x99);
+    let target = make_node_addr(0xAA);
+    let mut pending = PendingLookup::new(1000);
+    for id in 1..=9 {
+        pending.record(id);
+    }
+    lookup.pending_lookups.insert(target, pending);
+
+    let oldest = classify_request(
+        &mut lookup,
+        &make_request_id(1, target, 3),
+        &looping_peer,
+        &my_addr,
+        1000,
+        5000,
+        4096,
+        1,
+    );
+    assert!(matches!(oldest.outcome, RequestOutcome::Forward));
+    assert!(lookup.recent_requests.contains_key(&1));
+
+    let newest = classify_request(
+        &mut lookup,
+        &make_request_id(9, target, 3),
+        &looping_peer,
+        &my_addr,
+        1000,
+        5000,
+        4096,
+        1,
+    );
+    assert!(matches!(newest.outcome, RequestOutcome::OwnRequestLooped));
+    assert!(!lookup.recent_requests.contains_key(&9));
+}
+
 #[test]
 fn classify_request_still_transits_a_foreign_id_for_a_target_we_are_looking_up() {
     // The guard keys on the id, not the target: another node's lookup for the