Merge the development line up

# Conflicts:
#	CHANGELOG.md

CHANGELOG.md

@@ -273,6 +273,35 @@ with v0.5.x or earlier peers.
   smaller-NodeAddr resolution rule. Mirrored to the FSP rekey msg1
   path for symmetry.
 
+- A node no longer relays away the answer to its own lookup. A request is
+  flooded to every tree peer whose bloom filter claims the target, so a false
+  positive can send a copy out into the wider network and circulate it back to
+  the node that originated it. The only identity test on arrival was whether
+  the request named this node as the target, which a lookup this node
+  originated never satisfies, so the copy was filed in the request dedup cache
+  as ordinary transit under this node's own `request_id`. When the target
+  answered, the reply was reverse-path forwarded to the peer that looped the
+  request, the pending lookup was never satisfied, and discovery reported that
+  its requests went unanswered while the answers were in fact arriving. An
+  inbound response is now matched against this node's outstanding lookups
+  before the transit dedup record, and a returning copy of this node's own
+  request is dropped as the duplicate it is rather than recorded, so that id
+  never enters the transit cache at all. This was a race rather than a hard
+  failure: a reply that beat the looped copy found a clean cache and
+  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.
+
 ### Security
 
 - The peer static key is verified on both FMP handshake paths, not only at

docs/design/fips-mesh-operation.md

@@ -272,6 +272,17 @@ follows the same path as the request. Greedy tree routing toward the
 greedy tree routing toward the origin's coordinates 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
+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 as a duplicate rather than recorded, so the originator's id never
+enters the transit cache.
+
 **Response-forwarded flag**: Each `recent_requests` entry tracks whether a
 response has already been forwarded for that `request_id`. If a second
 response arrives (e.g., from convergent request paths that reached the

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")),

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,

src/node/handlers/lookup.rs

@@ -154,6 +154,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

src/node/metrics.rs

@@ -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(),

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).
@@ -417,6 +427,7 @@ mod tests {
         let variants = [
             DiscoveryReject::ReqDecodeError,
             DiscoveryReject::ReqDuplicate,
+            DiscoveryReject::ReqOwnLoopback,
             DiscoveryReject::ReqDedupCacheFull,
             DiscoveryReject::ReqTtlExhausted,
             DiscoveryReject::ReqSignRateLimited,

src/node/stats.rs

@@ -340,6 +340,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,

src/proto/lookup/core.rs

@@ -172,6 +172,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).
@@ -282,6 +288,40 @@ pub(crate) fn classify_request(
         };
     }
 
+    // A request this node originated, flooded to every bloom-matching tree
+    // peer and circulated back to us by one of them. The only identity test
+    // below is `request.target == *my_addr`, and for a lookup we originated
+    // the target is someone else, so without this the copy is filed as an
+    // ordinary transit entry keyed on our own `request_id` — and the answer,
+    // when it comes, is reverse-path forwarded to the peer that looped the
+    // 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::OwnRequestLooped,
+            evicted: None,
+        };
+    }
+
     let evicted = make_room(lookup, from, max_recent, peer_count);
     lookup.record_recent(request.request_id, *from, now_ms);
 
@@ -302,8 +342,9 @@ pub(crate) fn classify_request(
     Classification { outcome, evicted }
 }
 
-/// How an inbound LookupResponse should be routed, decided from the
-/// recent-request dedup state.
+/// How an inbound LookupResponse should be routed, decided from the pending
+/// lookups this node has outstanding and, failing that, the recent-request
+/// dedup state.
 pub(crate) enum ResponseRoute {
     /// A response for a request we forwarded, but we already reverse-forwarded
     /// one for this request_id — drop to prevent response routing loops.
@@ -318,7 +359,8 @@ pub(crate) enum ResponseRoute {
     Unsolicited,
 }
 
-/// Classify an inbound LookupResponse against the recent-request dedup cache.
+/// Classify an inbound LookupResponse against the pending lookups and the
+/// recent-request dedup cache, in that order.
 ///
 /// Pure decision over `Lookup` state: sets `response_forwarded` when this is
 /// the first response we transit for the request. No I/O, no view, no metrics.
@@ -327,6 +369,30 @@ pub(crate) fn classify_response(
     request_id: u64,
     target: &NodeAddr,
 ) -> ResponseRoute {
+    // Originator-ness is decided first, ahead of the transit dedup record.
+    // The response names a target this node has a lookup outstanding for and
+    // carries an id that lookup issued, so it answers us whatever else the
+    // dedup cache happens to hold for that id. Testing `recent_requests`
+    // first instead made this arm unreachable whenever a copy of our own
+    // flooded request found its way back to us and was filed as transit: the
+    // reply was relayed away rather than accepted, the pending lookup was
+    // never satisfied, and discovery failed while the answers were arriving.
+    //
+    // The id is fresh 64-bit randomness drawn per attempt and the target
+    // signs over it, so a harvested response is bound to the request it
+    // answered and cannot be redirected or replayed: an id we never issued
+    // still cannot match, and preferring this arm takes nothing away from
+    // what the `Unsolicited` arm protects. Replies to earlier attempts of a
+    // still-outstanding lookup match too, which is the common case on a link
+    // whose round trip exceeds the first rung of the retry ladder.
+    if lookup
+        .pending_lookups
+        .get(target)
+        .is_some_and(|pending| pending.matches(request_id))
+    {
+        return ResponseRoute::Originator;
+    }
+
     match lookup.recent_requests.get_mut(&request_id) {
         Some(recent) => {
             if recent.response_forwarded {
@@ -338,25 +404,9 @@ pub(crate) fn classify_response(
                 }
             }
         }
-        // Not a request we transited, so it claims to answer one of ours.
-        // Require that it names a target with a lookup outstanding and carries
-        // an id issued for it. The id is fresh 64-bit randomness drawn per
-        // attempt and the target signs over it, so a harvested response is
-        // bound to the request it answered and cannot be redirected or
-        // replayed. Replies to earlier attempts of a still-outstanding lookup
-        // still match, which is the common case on a link whose round trip
-        // exceeds the first rung of the retry ladder.
-        None => {
-            let solicited = lookup
-                .pending_lookups
-                .get(target)
-                .is_some_and(|pending| pending.matches(request_id));
-            if solicited {
-                ResponseRoute::Originator
-            } else {
-                ResponseRoute::Unsolicited
-            }
-        }
+        // Neither a request we transited nor an answer to a lookup we have
+        // outstanding for its target. Nobody asked for it.
+        None => ResponseRoute::Unsolicited,
     }
 }
 

src/proto/lookup/tests/core.rs

@@ -234,8 +234,8 @@ fn classify_response_transit_on_fresh_forwarded_request() {
         .recent_requests
         .insert(42, RecentRequest::new(from_peer, 1000));
 
-    // Transit is decided by the dedup record alone, so the target the response
-    // names plays no part here — pass one no pending lookup mentions.
+    // With no lookup of ours outstanding for the target, transit is decided by
+    // the dedup record — pass a target no pending lookup mentions.
     match classify_response(&mut lookup, 42, &make_node_addr(0xF1)) {
         ResponseRoute::Transit { from_peer: peer } => assert_eq!(peer, from_peer),
         _ => panic!("expected Transit"),
@@ -263,6 +263,38 @@ fn classify_response_already_forwarded_on_second_call() {
     ));
 }
 
+#[test]
+fn classify_response_originator_wins_over_a_transit_dedup_record() {
+    // Regression. A copy of a request we originated can be flooded back to us
+    // and filed in `recent_requests` as ordinary transit, keyed on our own
+    // request_id. Consulting the dedup record first then classified the answer
+    // as Transit and relayed it to the peer that looped the request, so the
+    // pending lookup was never satisfied and discovery failed with "no reply"
+    // while the responses were in fact arriving. Originator-ness wins.
+    let target = make_node_addr(0x61);
+    let looping_peer = make_node_addr(0x62);
+    let mut lookup = empty_lookup();
+    lookup
+        .recent_requests
+        .insert(4242, RecentRequest::new(looping_peer, 1000));
+    let mut pending = PendingLookup::new(1000);
+    pending.record(4242);
+    lookup.pending_lookups.insert(target, pending);
+
+    assert!(matches!(
+        classify_response(&mut lookup, 4242, &target),
+        ResponseRoute::Originator
+    ));
+    // And nothing was reverse-path forwarded on our behalf.
+    assert!(
+        !lookup
+            .recent_requests
+            .get(&4242)
+            .unwrap()
+            .response_forwarded
+    );
+}
+
 #[test]
 fn classify_response_originator_when_a_pending_lookup_issued_the_id() {
     // No dedup record, so this is not a response we transit. It counts as ours
@@ -524,6 +556,71 @@ fn classify_request_duplicate_on_second_call() {
     ));
 }
 
+#[test]
+fn classify_request_drops_our_own_request_looped_back_to_us() {
+    // Regression, request side of the same defect. Our flood reaches a peer
+    // that circulates it back; the only identity test is `target == my_addr`,
+    // which a lookup we originated never satisfies. Recording it would file
+    // our own request_id as a transit entry and hand the eventual answer to
+    // the looping peer, so the copy is dropped as the duplicate it is.
+    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);
+    pending.record(77);
+    lookup.pending_lookups.insert(target, pending);
+
+    // 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, 3, 0);
+    let classification = classify_request(
+        &mut lookup,
+        &request,
+        &looping_peer,
+        &my_addr,
+        1000,
+        5000,
+        4096,
+        1,
+    );
+    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));
+    assert!(lookup.recent_by_peer.is_empty());
+    // A response for it is then ours to accept.
+    assert!(matches!(
+        classify_response(&mut lookup, 77, &target),
+        ResponseRoute::Originator
+    ));
+}
+
+#[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
+    // same target must still be transited normally while ours is outstanding.
+    let mut lookup = empty_lookup();
+    let from = make_node_addr(0x01);
+    let my_addr = make_node_addr(0x99);
+    let target = make_node_addr(0xAA);
+    let mut pending = PendingLookup::new(1000);
+    pending.record(77);
+    lookup.pending_lookups.insert(target, pending);
+
+    let request = make_request_id(88, target, 3);
+    let outcome =
+        classify_request(&mut lookup, &request, &from, &my_addr, 1000, 5000, 4096, 1).outcome;
+    assert!(matches!(outcome, RequestOutcome::Forward));
+    assert_eq!(lookup.recent_requests.get(&88).unwrap().from_peer, from);
+}
+
 #[test]
 fn classify_request_evicts_rather_than_refusing_a_full_dedup_cache() {
     // Regression. The cache-full path used to drop the arriving request, so