From 7179afcfde0d8602213349ea5bd9ecd4c376f694 Mon Sep 17 00:00:00 2001 From: Kpa-clawbot Date: Sat, 16 May 2026 10:14:44 -0700 Subject: [PATCH] feat(#1228): reject geo-implausible neighbor-graph edges at build time (#1230) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #1228 — geo-implausible neighbor-graph edges are rejected at build time. Red commit: `5a6d9660` — failing tests for 4 cases (reject SF↔Berlin, accept local CA, accept no-GPS endpoint, counter increments). Live CI run (latest commit): https://github.com/Kpa-clawbot/CoreScope/actions?query=branch%3Afix%2Fissue-1228 ## Why The disambiguator's tier-1 affinity graph is built blindly from path co-occurrence. On wide-geo MQTT deployments, a single bad hop disambiguation seeds an edge across geographically impossible distances (e.g. Bay Area ↔ Berlin), which then reinforces the same wrong resolution next time. Self-poisoning spiral. ## What changed - `upsertEdge` now consults a per-graph GPS index. When **both** endpoints have known GPS and their haversine distance exceeds the threshold, the edge is dropped and `NeighborGraph.RejectedEdgesGeoFar` (atomic) is incremented. - Either endpoint missing GPS ⇒ accept (no signal to reject), per acceptance criteria. - Threshold is configurable via `neighborGraph.maxEdgeKm` (default **500 km** — well above any plausible terrestrial LoRa hop, including satellite-assisted). 0 ⇒ use default; negative ⇒ disable the filter. Exposed via `Config.NeighborMaxEdgeKm()`. - New `BuildFromStoreWithOptions` carrying the threshold; `BuildFromStore` and `BuildFromStoreWithLog` are kept as thin wrappers. - Stats are surfaced under `GET /api/analytics/neighbor-graph` as `stats.rejected_edges_geo_far`. - All rejection logs PII-truncate pubkeys to 8 hex chars (public repo discipline). - `config.example.json` updated with the new field + comment. ## Follow-up #1229 (per-region scoped affinity graphs) depends on this landing first. --------- Co-authored-by: corescope-bot --- cmd/server/config.go | 16 +++- cmd/server/neighbor_api.go | 27 +++--- cmd/server/neighbor_graph.go | 99 +++++++++++++++++++++- cmd/server/neighbor_graph_geo_test.go | 116 ++++++++++++++++++++++++++ config.example.json | 3 +- 5 files changed, 246 insertions(+), 15 deletions(-) create mode 100644 cmd/server/neighbor_graph_geo_test.go diff --git a/cmd/server/config.go b/cmd/server/config.go index b9454d1f..10a78007 100644 --- a/cmd/server/config.go +++ b/cmd/server/config.go @@ -129,7 +129,8 @@ type ResolvedPathConfig struct { // NeighborGraphConfig controls neighbor edge pruning. type NeighborGraphConfig struct { - MaxAgeDays int `json:"maxAgeDays"` // edges older than this are pruned (default 5) + MaxAgeDays int `json:"maxAgeDays"` // edges older than this are pruned (default 5) + MaxEdgeKm float64 `json:"maxEdgeKm"` // geo-implausibility threshold (km); 0 = default 500; negative disables (#1228) } // PacketStoreConfig controls in-memory packet store limits. @@ -185,6 +186,19 @@ func (c *Config) NeighborMaxAgeDays() int { return 5 } +// NeighborMaxEdgeKm returns the geo-implausibility threshold in km. +// 0 (unset) → DefaultMaxEdgeKm (500). Negative → 0 (filter disabled). +// See issue #1228. +func (c *Config) NeighborMaxEdgeKm() float64 { + if c == nil || c.NeighborGraph == nil || c.NeighborGraph.MaxEdgeKm == 0 { + return DefaultMaxEdgeKm + } + if c.NeighborGraph.MaxEdgeKm < 0 { + return 0 + } + return c.NeighborGraph.MaxEdgeKm +} + type TimestampConfig struct { DefaultMode string `json:"defaultMode"` // "ago" | "absolute" Timezone string `json:"timezone"` // "local" | "utc" diff --git a/cmd/server/neighbor_api.go b/cmd/server/neighbor_api.go index 27064493..3cca7fcf 100644 --- a/cmd/server/neighbor_api.go +++ b/cmd/server/neighbor_api.go @@ -6,6 +6,7 @@ import ( "sort" "strconv" "strings" + "sync/atomic" "time" "github.com/gorilla/mux" @@ -66,10 +67,11 @@ type GraphEdge struct { } type GraphStats struct { - TotalNodes int `json:"total_nodes"` - TotalEdges int `json:"total_edges"` - AmbiguousEdges int `json:"ambiguous_edges"` - AvgClusterSize float64 `json:"avg_cluster_size"` + TotalNodes int `json:"total_nodes"` + TotalEdges int `json:"total_edges"` + AmbiguousEdges int `json:"ambiguous_edges"` + AvgClusterSize float64 `json:"avg_cluster_size"` + RejectedEdgesGeoFar uint64 `json:"rejected_edges_geo_far"` // edges dropped at build time by the geo-implausibility filter (#1228) } // ─── Graph accessor on Server ────────────────────────────────────────────────── @@ -81,8 +83,12 @@ func (s *Server) getNeighborGraph() *NeighborGraph { if s.neighborGraph == nil || s.neighborGraph.IsStale() { if s.store != nil { - debugLog := s.cfg != nil && s.cfg.DebugAffinity - s.neighborGraph = BuildFromStoreWithLog(s.store, debugLog) + opts := BuildOptions{MaxEdgeKm: DefaultMaxEdgeKm} + if s.cfg != nil { + opts.EnableLog = s.cfg.DebugAffinity + opts.MaxEdgeKm = s.cfg.NeighborMaxEdgeKm() + } + s.neighborGraph = BuildFromStoreWithOptions(s.store, opts) } else { s.neighborGraph = NewNeighborGraph() } @@ -347,10 +353,11 @@ func (s *Server) handleNeighborGraph(w http.ResponseWriter, r *http.Request) { Nodes: nodes, Edges: filteredEdges, Stats: GraphStats{ - TotalNodes: len(nodes), - TotalEdges: len(filteredEdges), - AmbiguousEdges: ambiguousCount, - AvgClusterSize: avgCluster, + TotalNodes: len(nodes), + TotalEdges: len(filteredEdges), + AmbiguousEdges: ambiguousCount, + AvgClusterSize: avgCluster, + RejectedEdgesGeoFar: atomic.LoadUint64(&graph.RejectedEdgesGeoFar), }, } diff --git a/cmd/server/neighbor_graph.go b/cmd/server/neighbor_graph.go index 780dfc1f..587fae87 100644 --- a/cmd/server/neighbor_graph.go +++ b/cmd/server/neighbor_graph.go @@ -7,6 +7,7 @@ import ( "math" "strings" "sync" + "sync/atomic" "time" ) @@ -87,6 +88,27 @@ type NeighborGraph struct { byNode map[string][]*NeighborEdge // pubkey → edges involving this node builtAt time.Time logFn func(prefix, msg string) // optional structured logging callback + + // RejectedEdgesGeoFar counts edges dropped at build time because both + // endpoints had GPS and their haversine distance exceeded the + // configurable threshold (NeighborGraphConfig.MaxEdgeKm, default 500). + // Accessed via sync/atomic. See issue #1228. + RejectedEdgesGeoFar uint64 + + // maxEdgeKm is the geo-sanity threshold copied from config at build + // time. 0 means "no limit" / filter disabled. + maxEdgeKm float64 + + // nodeGeo maps lowercased pubkey → (lat, lon, hasGPS) for geo-sanity + // checks during upsertEdge. Populated by the builder; empty for graphs + // constructed via NewNeighborGraph directly (geo filter inert). + nodeGeo map[string]nodeGeoInfo +} + +// nodeGeoInfo is the minimal geo slice cached on the graph for upsertEdge. +type nodeGeoInfo struct { + Lat, Lon float64 + HasGPS bool } // NewNeighborGraph creates an empty graph. @@ -148,9 +170,20 @@ func (g *NeighborGraph) IsStale() bool { // BuildFromStore constructs the neighbor graph from all packets in the store. // The store's read-lock must NOT be held by the caller. func BuildFromStore(store *PacketStore) *NeighborGraph { - return BuildFromStoreWithLog(store, false) + return BuildFromStoreWithOptions(store, BuildOptions{MaxEdgeKm: DefaultMaxEdgeKm}) } +// BuildOptions controls optional behavior of BuildFromStoreWithOptions. +type BuildOptions struct { + EnableLog bool // structured disambiguation logging + MaxEdgeKm float64 // geo-sanity threshold; 0 disables the filter +} + +// DefaultMaxEdgeKm is the conservative built-in cap for the +// geo-implausibility filter (issue #1228). 500 km is comfortably above any +// plausible terrestrial LoRa hop (including satellite-relayed cases). +const DefaultMaxEdgeKm = 500.0 + // cachedToLower returns strings.ToLower(s), caching results to avoid // repeated allocations for the same pubkey string. func cachedToLower(cache map[string]string, s string) string { @@ -163,9 +196,16 @@ func cachedToLower(cache map[string]string, s string) string { } // BuildFromStoreWithLog constructs the neighbor graph, optionally logging disambiguation decisions. +// Kept for backward compatibility; new callers should use BuildFromStoreWithOptions. func BuildFromStoreWithLog(store *PacketStore, enableLog bool) *NeighborGraph { + return BuildFromStoreWithOptions(store, BuildOptions{EnableLog: enableLog, MaxEdgeKm: DefaultMaxEdgeKm}) +} + +// BuildFromStoreWithOptions constructs the neighbor graph with explicit options. +func BuildFromStoreWithOptions(store *PacketStore, opts BuildOptions) *NeighborGraph { g := NewNeighborGraph() - if enableLog { + g.maxEdgeKm = opts.MaxEdgeKm + if opts.EnableLog { g.logFn = func(prefix, msg string) { log.Printf("[affinity] resolve %s: %s", prefix, msg) } @@ -179,7 +219,16 @@ func BuildFromStoreWithLog(store *PacketStore, enableLog bool) *NeighborGraph { // Build prefix map for candidate resolution. // Use cached nodes+PM (avoids DB call if cache is fresh). - _, pm := store.getCachedNodesAndPM() + allNodes, pm := store.getCachedNodesAndPM() + + // Index node geo for upsertEdge geo-sanity checks (issue #1228). + geo := make(map[string]nodeGeoInfo, len(allNodes)) + for _, n := range allNodes { + geo[strings.ToLower(n.PublicKey)] = nodeGeoInfo{Lat: n.Lat, Lon: n.Lon, HasGPS: n.HasGPS} + } + g.mu.Lock() + g.nodeGeo = geo + g.mu.Unlock() // Local cache for strings.ToLower — pubkeys are immutable and repeat // across hundreds of thousands of observations. @@ -267,6 +316,13 @@ func jsonUnmarshalFast(data string, v interface{}) error { // upsertEdge adds/updates an edge between two fully-known pubkeys. func (g *NeighborGraph) upsertEdge(pubkeyA, pubkeyB, prefix, observer string, snr *float64, ts time.Time) { + // Geo-sanity guard (issue #1228): if both endpoints have known GPS and + // the haversine distance exceeds the configured threshold, drop the + // edge. When either lacks GPS we have no signal and accept. + if g.shouldRejectGeoFar(pubkeyA, pubkeyB) { + atomic.AddUint64(&g.RejectedEdgesGeoFar, 1) + return + } key := makeEdgeKey(pubkeyA, pubkeyB) g.mu.Lock() @@ -652,3 +708,40 @@ func (g *NeighborGraph) PruneOlderThan(cutoff time.Time) int { } return pruned } + +// shouldRejectGeoFar reports whether the edge (a, b) is geographically +// implausible under the configured threshold. Both endpoints must have known +// GPS to trigger a rejection; if either lacks GPS the edge is accepted +// (issue #1228 — "no signal to reject"). +// +// All log output is PII-truncated to the first 8 hex chars of each pubkey. +func (g *NeighborGraph) shouldRejectGeoFar(a, b string) bool { + if g == nil || g.maxEdgeKm <= 0 || g.nodeGeo == nil { + return false + } + if strings.HasPrefix(a, "prefix:") || strings.HasPrefix(b, "prefix:") { + return false + } + ga, oka := g.nodeGeo[a] + gb, okb := g.nodeGeo[b] + if !oka || !okb || !ga.HasGPS || !gb.HasGPS { + return false + } + d := haversineKm(ga.Lat, ga.Lon, gb.Lat, gb.Lon) + if d <= g.maxEdgeKm { + return false + } + // PII-truncated INFO log (8-char prefix max). + log.Printf("[neighbor-graph] reject geo-far edge %s↔%s distance=%.0fkm threshold=%.0fkm", + piiTruncPubkey(a), piiTruncPubkey(b), d, g.maxEdgeKm) + return true +} + +// piiTruncPubkey returns at most the first 8 hex chars of a pubkey for log +// output. The repo is public and observer/node pubkeys are PII-adjacent. +func piiTruncPubkey(pk string) string { + if len(pk) <= 8 { + return pk + } + return pk[:8] +} diff --git a/cmd/server/neighbor_graph_geo_test.go b/cmd/server/neighbor_graph_geo_test.go new file mode 100644 index 00000000..9dbef719 --- /dev/null +++ b/cmd/server/neighbor_graph_geo_test.go @@ -0,0 +1,116 @@ +package main + +import ( + "sync/atomic" + "testing" +) + +// TestBuildNeighborGraph_RejectsGeoFarEdge — RED test for #1228. +// +// Synthetic advert produces an edge between A (Bay Area) and B (Berlin). +// Distance ≈ 9 100 km, well above any plausible terrestrial LoRa hop. +// The geo-sanity filter must reject the edge at build time so the +// affinity graph cannot self-reinforce a wrong disambiguation. +func TestBuildNeighborGraph_RejectsGeoFarEdge(t *testing.T) { + nodes := []nodeInfo{ + // A: San Francisco + {Role: "repeater", PublicKey: "aaaa1111", Name: "A_SF", Lat: 37.77, Lon: -122.41, HasGPS: true}, + // B: Berlin + {Role: "repeater", PublicKey: "bbbb2222", Name: "B_BE", Lat: 52.52, Lon: 13.40, HasGPS: true}, + // Observer with GPS at SF (won't affect A↔B edge under test) + {Role: "repeater", PublicKey: "obs00001", Name: "Observer", Lat: 37.77, Lon: -122.41, HasGPS: true}, + } + // ADVERT originated by A, path=["bbbb"] → builder creates edge A↔B + // (originator ↔ path[0]). With geo sanity ON this edge must be dropped. + tx := ngMakeTx(1, 4, ngFromNodeJSON("aaaa1111"), []*StoreObs{ + ngMakeObs("obs00001", `["bbbb"]`, nowStr, ngFloatPtr(-10)), + }) + store := ngTestStore(nodes, []*StoreTx{tx}) + g := BuildFromStore(store) + + for _, e := range g.AllEdges() { + if (e.NodeA == "aaaa1111" && e.NodeB == "bbbb2222") || + (e.NodeA == "bbbb2222" && e.NodeB == "aaaa1111") { + t.Fatalf("geo-implausible edge A(SF)↔B(Berlin) was not rejected: %+v", e) + } + } +} + +// TestBuildNeighborGraph_AcceptsLocalEdge — A↔C within plausible LoRa range +// (both in CA, ~100 km apart) must remain in the graph. +func TestBuildNeighborGraph_AcceptsLocalEdge(t *testing.T) { + nodes := []nodeInfo{ + {Role: "repeater", PublicKey: "aaaa1111", Name: "A_SF", Lat: 37.77, Lon: -122.41, HasGPS: true}, + {Role: "repeater", PublicKey: "cccc3333", Name: "C_SJ", Lat: 37.34, Lon: -121.89, HasGPS: true}, + {Role: "repeater", PublicKey: "obs00001", Name: "Observer", Lat: 37.77, Lon: -122.41, HasGPS: true}, + } + tx := ngMakeTx(1, 4, ngFromNodeJSON("aaaa1111"), []*StoreObs{ + ngMakeObs("obs00001", `["cccc"]`, nowStr, ngFloatPtr(-10)), + }) + store := ngTestStore(nodes, []*StoreTx{tx}) + g := BuildFromStore(store) + + found := false + for _, e := range g.AllEdges() { + if (e.NodeA == "aaaa1111" && e.NodeB == "cccc3333") || + (e.NodeA == "cccc3333" && e.NodeB == "aaaa1111") { + found = true + } + } + if !found { + t.Fatalf("local A↔C edge (~50km) must be accepted") + } +} + +// TestBuildNeighborGraph_AcceptsEdgeWhenNoGPS — if either endpoint lacks GPS, +// we have no signal to reject; the edge is accepted. +func TestBuildNeighborGraph_AcceptsEdgeWhenNoGPS(t *testing.T) { + nodes := []nodeInfo{ + // A has GPS (Berlin) + {Role: "repeater", PublicKey: "aaaa1111", Name: "A", Lat: 52.52, Lon: 13.40, HasGPS: true}, + // D has no GPS + {Role: "repeater", PublicKey: "dddd4444", Name: "D"}, // HasGPS = false + {Role: "repeater", PublicKey: "obs00001", Name: "Observer"}, + } + tx := ngMakeTx(1, 4, ngFromNodeJSON("aaaa1111"), []*StoreObs{ + ngMakeObs("obs00001", `["dddd"]`, nowStr, nil), + }) + store := ngTestStore(nodes, []*StoreTx{tx}) + g := BuildFromStore(store) + + found := false + for _, e := range g.AllEdges() { + if (e.NodeA == "aaaa1111" && e.NodeB == "dddd4444") || + (e.NodeA == "dddd4444" && e.NodeB == "aaaa1111") { + found = true + } + } + if !found { + t.Fatalf("A(GPS)↔D(no-GPS) edge must be accepted (no signal to reject)") + } +} + +// TestBuildNeighborGraph_RejectedCounterIncrements — every dropped edge bumps +// the atomic counter surfaced by /api/analytics/neighbor-graph stats. +func TestBuildNeighborGraph_RejectedCounterIncrements(t *testing.T) { + nodes := []nodeInfo{ + {Role: "repeater", PublicKey: "aaaa1111", Name: "A_SF", Lat: 37.77, Lon: -122.41, HasGPS: true}, + {Role: "repeater", PublicKey: "bbbb2222", Name: "B_BE", Lat: 52.52, Lon: 13.40, HasGPS: true}, + {Role: "repeater", PublicKey: "obs00001", Name: "Observer", Lat: 37.77, Lon: -122.41, HasGPS: true}, + } + // Two adverts each producing the far A↔B edge attempt → counter ≥ 2. + txs := []*StoreTx{ + ngMakeTx(1, 4, ngFromNodeJSON("aaaa1111"), []*StoreObs{ + ngMakeObs("obs00001", `["bbbb"]`, nowStr, nil), + }), + ngMakeTx(2, 4, ngFromNodeJSON("aaaa1111"), []*StoreObs{ + ngMakeObs("obs00001", `["bbbb"]`, nowStr, nil), + }), + } + store := ngTestStore(nodes, txs) + g := BuildFromStore(store) + got := atomic.LoadUint64(&g.RejectedEdgesGeoFar) + if got < 2 { + t.Fatalf("RejectedEdgesGeoFar = %d, want >= 2", got) + } +} diff --git a/config.example.json b/config.example.json index f4a0cdc1..e2f64ad8 100644 --- a/config.example.json +++ b/config.example.json @@ -234,7 +234,8 @@ }, "neighborGraph": { "maxAgeDays": 5, - "_comment": "Neighbor edges older than this many days are pruned on startup and daily. Default: 5." + "maxEdgeKm": 500, + "_comment": "maxAgeDays: neighbor edges older than this many days are pruned on startup and daily. Default 5. maxEdgeKm: geo-implausibility filter — when both endpoints have GPS, edges with haversine distance > maxEdgeKm are rejected at build time to prevent disambiguator self-reinforcement on wide-geo MQTT deployments. Default 500 km (well above any plausible terrestrial LoRa hop). 0 ⇒ use default; set negative to disable the filter. Rejected count surfaces in /api/analytics/neighbor-graph stats. Issue #1228." }, "batteryThresholds": { "lowMv": 3300,