diff --git a/docs/design/fips-mesh-operation.md b/docs/design/fips-mesh-operation.md index c38d1581..5d92cd78 100644 --- a/docs/design/fips-mesh-operation.md +++ b/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 diff --git a/src/node/handlers/lookup.rs b/src/node/handlers/lookup.rs index 3d3791e6..88cae3c0 100644 --- a/src/node/handlers/lookup.rs +++ b/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); diff --git a/src/node/reject.rs b/src/node/reject.rs index 34ed5e32..ab2a270a 100644 --- a/src/node/reject.rs +++ b/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 diff --git a/src/proto/lookup/core.rs b/src/proto/lookup/core.rs index 2aa57ac2..5c5b8b71 100644 --- a/src/proto/lookup/core.rs +++ b/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, diff --git a/src/proto/lookup/tests/core.rs b/src/proto/lookup/tests/core.rs index 66f4e21c..917616b4 100644 --- a/src/proto/lookup/tests/core.rs +++ b/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