From 4c45dec79f65e48555575b38fc6d55c7444ec234 Mon Sep 17 00:00:00 2001 From: efiten Date: Wed, 2 Sep 2026 09:11:10 +0200 Subject: [PATCH] fix(#1902): don't attribute transported scopes from the 1-byte hop prefix (#1903) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #1902. ## The bug `byPathHop` is keyed on the raw hop string from `path_json`, and both relay-info paths look up the full pubkey **and** fold in `key[:2]` — the 1-byte wire prefix. `TransportedScopes` (#1751) accumulated over that folded set, so every node sharing a pubkey first byte reported the same scopes. On the live network all four active nodes with prefix `f7` returned an identical set: ``` f79616... BE repeater ['#be','#be-van','#de','#de-nw','#nl'] f7e718... BE repeater ['#be','#be-van','#de','#de-nw','#nl'] f788ad... BE repeater ['#be','#be-van','#de','#de-nw','#nl'] f752c2... DE/NRW repeat. ['#be','#be-van','#de','#de-nw','#nl'] ``` Their real sets, from unambiguous full-pubkey hops over the same 7 days, are disjoint: ``` f79616... (BE) #be 471, #eu 9, #nl 6, #de 3, #be-van 1 f752c2... (DE/NRW) #de 13, #de-nw 11 f7e718... (BE) (none) ``` A sysop reads a scope badge as a statement about how their repeater is configured, so a Belgian repeater badged `#de-nw` is a wrong answer, not an imprecise one. ## The change The prefix fold stays for the counters — that is the documented #662 trade-off, "a possible over-count for clearly false zeros", and `RelayCount1h/24h`, `LastRelayed` and `UnscopedRelayCount24h` are magnitudes where an over-count is tolerable. Scopes are not a magnitude. A 1-byte hop names one of N nodes and cannot substantiate a categorical claim. Entries reached only through the prefix bucket are now flagged (`relayEntry.viaPrefix` / a `viaPrefix` argument to the bulk `visit` closure) and excluded from scope accumulation only. Both computation paths are changed together so `/api/nodes` (bulk) and the node-detail endpoint (per-node) stay in parity: - `cmd/server/repeater_liveness.go` — `collectRelayEntriesLocked` / `computeRelayInfoFromEntries` - `cmd/server/repeater_enrich_bulk.go` — `computeRepeaterRelayInfoMap` The `public/nodes.js` tooltip is updated to describe what the field now actually means. Attribution does not collapse: `observations.resolved_path` carries full pubkeys for ~27% of observations on the live instance (408k of 1.54M over 7 days), and those rows produce the correct per-node sets above. A node with no resolved hop yet shows no badge rather than a borrowed one. ## Tests `TestTransportedScopes_CrossBucketFold` pinned the old behaviour ("a scope seen only in the prefix bucket must surface on the full key"), which is the bug. It is replaced by `TestTransportedScopes_PrefixBucketNotAttributed`, which asserts on **both** paths that: 1. a scope evidenced only by a 1-byte hop is not attributed; 2. a scope also present under the full key still is; 3. `RelayCount24h` still counts all three packets — narrowing scopes must not narrow the counters, i.e. the #662 fold is untouched. Red before the change, green after. ``` cd cmd/server && go test ./... ok github.com/corescope/server 98.2s node test-packet-filter.js 92 passed, 0 failed node test-aging.js 18 passed, 0 failed node test-frontend-helpers.js 625 passed, 2 failed ``` The two frontend failures (`favStar returns filled star for favorite`, `favStar returns empty star for non-favorite`) and `cmd/ingestor`'s `TestWriteStatsAtomic_SymlinkAtDestIsReplaced` are **pre-existing** — I ran them on a pristine `upstream/master` worktree and got byte-identical results (the ingestor one is a Windows symlink-privilege limitation, not a code failure). ## Perf No new work in any loop. The bulk path gains one bool argument to an existing closure and one `&& !viaPrefix` on a branch that already ran; the per-node path gains one bool field on `relayEntry`, which is stack/slice-local and not retained. Same complexity, same allocations. ## What I could not verify end-to-end, and why I built a fixture from live data (2512 nodes, 17k transmissions, 529k observations, including all eight `f7` nodes) and ran the before/after binaries against it. Neither reproduced the live field — both returned no `transported_scopes` and `relay_count_24h: 0` for every node. That turns out to be a **separate cold-start bug**: `LoadChunked` calls `indexResolvedPathHops` per observation while scanning chunks, which adds full-pubkey keys to `byPathHop`, and then the post-load block at `cmd/server/chunked_load.go:459` calls `buildPathHopIndex()`, which begins with `s.byPathHop = make(...)` and rebuilds from raw hops only. Every resolved full-pubkey key from the scan is discarded: ``` [store] Built path-hop index: 2924 unique keys <- raw hops only [store] LoadChunked: 17056 transmissions (527331 observations) ``` So on a freshly started server the full-pubkey buckets are empty and only refill from live ingestion. That is being filed separately; it is orthogonal to this change, but it does mean `transported_scopes` will be sparse for a while after any restart until it is fixed. This PR is therefore verified by unit tests on both computation paths plus the live-data derivation above, not by a local end-to-end run. Co-authored-by: Claude Opus 5 (1M context) --- cmd/server/repeater_enrich_bulk.go | 15 ++++-- cmd/server/repeater_liveness.go | 24 +++++++--- cmd/server/transported_scopes_1751_test.go | 53 ++++++++++++++++------ public/nodes.js | 2 +- 4 files changed, 69 insertions(+), 25 deletions(-) diff --git a/cmd/server/repeater_enrich_bulk.go b/cmd/server/repeater_enrich_bulk.go index 220d4720..1182f81d 100644 --- a/cmd/server/repeater_enrich_bulk.go +++ b/cmd/server/repeater_enrich_bulk.go @@ -129,7 +129,10 @@ func (s *PacketStore) computeRepeaterRelayInfoMap(windowHours float64) map[strin } } } - visit := func(txs []*StoreTx) { + // viaPrefix marks the 1-byte wire-prefix bucket, shared by every + // node with the same first pubkey byte. See collectRelayEntriesLocked + // for the counters-vs-scopes split (#662 / #1902). + visit := func(txs []*StoreTx, viaPrefix bool) { for _, tx := range txs { if tx == nil { continue @@ -152,7 +155,11 @@ func (s *PacketStore) computeRepeaterRelayInfoMap(windowHours float64) map[strin // unparseable first_seen still proves the repeater // transported that scope. RelayCount/LastRelayed below // remain timestamp-gated. - if tx.ScopeName != "" { + // + // #1902: it IS gated on full-pubkey attribution — a 1-byte + // hop cannot prove which of the nodes sharing that byte + // carried the packet. + if tx.ScopeName != "" && !viaPrefix { if scopeSet == nil { scopeSet = map[string]struct{}{} } @@ -180,11 +187,11 @@ func (s *PacketStore) computeRepeaterRelayInfoMap(windowHours float64) map[strin } } } - visit(list) + visit(list, false) if seen != nil { prefix := key[:2] if prefix != key { - visit(snap[prefix]) + visit(snap[prefix], true) } } info.TransportedScopes = sortedCappedScopes(scopeSet) diff --git a/cmd/server/repeater_liveness.go b/cmd/server/repeater_liveness.go index e2d44152..d5eec01d 100644 --- a/cmd/server/repeater_liveness.go +++ b/cmd/server/repeater_liveness.go @@ -114,6 +114,11 @@ type relayEntry struct { // scope is the tx's region scope name (transmissions.scope_name). // Empty when absent / on older schemas. Used for TransportedScopes (#1751). scope string + // viaPrefix marks an entry that was only reachable through the 1-byte + // wire-prefix bucket, i.e. some node whose pubkey starts with the same + // byte carried it — not necessarily this one. Counters accept that + // ambiguity (#662); scope attribution does not (#1902). + viaPrefix bool } // collectRelayEntriesLocked returns deduplicated relayEntry snapshots for @@ -128,7 +133,9 @@ type relayEntry struct { // // The 1-byte prefix lookup CAN over-count when multiple nodes share the // same first byte. This trades a possible over-count for clearly false -// zeros (issue #662). +// zeros (issue #662). Entries reached only that way are flagged viaPrefix +// so TransportedScopes can refuse them (#1902) while the counters keep the +// trade-off. func (s *PacketStore) collectRelayEntriesLocked(key string) []relayEntry { txList := s.byPathHop[key] var prefixList []*StoreTx @@ -147,7 +154,7 @@ func (s *PacketStore) collectRelayEntriesLocked(key string) []relayEntry { hint := len(txList) + len(prefixList) entries := make([]relayEntry, 0, hint) seen := make(map[int]bool, hint) - collect := func(list []*StoreTx) { + collect := func(list []*StoreTx, viaPrefix bool) { for _, tx := range list { if tx == nil || seen[tx.ID] { continue @@ -161,11 +168,11 @@ func (s *PacketStore) collectRelayEntriesLocked(key string) []relayEntry { if tx.RouteType != nil { rt = *tx.RouteType } - entries = append(entries, relayEntry{ts: tx.FirstSeen, pt: pt, rt: rt, scope: tx.ScopeName}) + entries = append(entries, relayEntry{ts: tx.FirstSeen, pt: pt, rt: rt, scope: tx.ScopeName, viaPrefix: viaPrefix}) } } - collect(txList) - collect(prefixList) + collect(txList, false) + collect(prefixList, true) return entries } @@ -189,7 +196,12 @@ func computeRelayInfoFromEntries(entries []relayEntry, windowHours float64) Repe // #1751: accumulate transported scopes BEFORE the timestamp gate — // a non-advert path-hop tx proves scope transport even if its // first_seen is unparseable. Mirrors the bulk path. - if e.scope != "" { + // + // #1902: but only when the tx named this node by its full pubkey. + // A 1-byte hop names one of every node sharing that byte, which is + // enough for an approximate count and not enough to claim the node + // serves a region. + if e.scope != "" && !e.viaPrefix { if scopeSet == nil { scopeSet = map[string]struct{}{} } diff --git a/cmd/server/transported_scopes_1751_test.go b/cmd/server/transported_scopes_1751_test.go index 49cf3706..4d65ff84 100644 --- a/cmd/server/transported_scopes_1751_test.go +++ b/cmd/server/transported_scopes_1751_test.go @@ -100,27 +100,52 @@ func TestTransportedScopes_EmptyWhenNoScope(t *testing.T) { } } -// TestTransportedScopes_CrossBucketFold covers the bulk path's prefix fold: -// for a full-pubkey key it also folds in the matching 1-byte raw-prefix bucket -// (deduping by tx.ID). A scope seen only in the prefix bucket must surface on -// the full key, and a tx present in BOTH buckets must not be double-processed. -func TestTransportedScopes_CrossBucketFold(t *testing.T) { - full := scopeTx(1, 2, "region-direct") // only in the full-key bucket - prefixOnly := scopeTx(2, 2, "region-via-prefix") // only in the 1-byte bucket - shared := scopeTx(3, 2, "region-shared") // in BOTH buckets (dedup by ID) +// TestTransportedScopes_PrefixBucketNotAttributed is the #1902 regression. +// +// A 1-byte hop prefix is shared by every node whose pubkey starts with that +// byte, so the prefix bucket holds other nodes' traffic. Scopes are a +// categorical claim about which regions a repeater serves — a 1-byte hop +// names one of N candidates and cannot substantiate it. On the live network +// this badged Belgian repeaters with #de/#de-nw purely because a German +// repeater shared their first pubkey byte. +// +// So TransportedScopes must come from the full-key bucket only, while the +// counters keep the #662 prefix fold (over-count beats false zeros). +// Asserted on BOTH computation paths so they stay in parity. +func TestTransportedScopes_PrefixBucketNotAttributed(t *testing.T) { + direct := scopeTx(1, 2, "region-direct") // only in the full-key bucket + foreign := scopeTx(2, 2, "region-foreign") // only in the 1-byte bucket + shared := scopeTx(3, 2, "region-shared") // in BOTH buckets (dedup by ID) store := &PacketStore{ byPathHop: map[string][]*StoreTx{ - scope1751Key: {full, shared}, - scope1751Key[:2]: {prefixOnly, shared}, + scope1751Key: {direct, shared}, + scope1751Key[:2]: {foreign, shared}, }, mu: sync.RWMutex{}, } - got := store.computeRepeaterRelayInfoMap(24)[scope1751Key].TransportedScopes - want := []string{"region-direct", "region-shared", "region-via-prefix"} - if !reflect.DeepEqual(got, want) { - t.Fatalf("cross-bucket fold TransportedScopes = %v, want %v", got, want) + // "region-foreign" is only evidenced by a 1-byte hop — it must not be + // attributed. "region-shared" appears under the full key too, so it stays. + want := []string{"region-direct", "region-shared"} + + bulk := store.computeRepeaterRelayInfoMap(24)[scope1751Key] + if !reflect.DeepEqual(bulk.TransportedScopes, want) { + t.Fatalf("bulk TransportedScopes = %v, want %v (prefix-bucket scopes excluded)", bulk.TransportedScopes, want) + } + + perNode := store.GetRepeaterRelayInfo(scope1751Key, 24) + if !reflect.DeepEqual(perNode.TransportedScopes, want) { + t.Fatalf("per-node TransportedScopes = %v, want %v (prefix-bucket scopes excluded)", perNode.TransportedScopes, want) + } + + // The #662 fold itself is untouched: all three in-window packets still + // count, deduped by tx ID. Narrowing scopes must not narrow the counters. + if bulk.RelayCount24h != 3 { + t.Fatalf("bulk RelayCount24h = %d, want 3 (prefix fold must still feed the counters)", bulk.RelayCount24h) + } + if perNode.RelayCount24h != 3 { + t.Fatalf("per-node RelayCount24h = %d, want 3 (prefix fold must still feed the counters)", perNode.RelayCount24h) } } diff --git a/public/nodes.js b/public/nodes.js index 7613706d..c13c1e38 100644 --- a/public/nodes.js +++ b/public/nodes.js @@ -673,7 +673,7 @@ const btooltip = "Normalized betweenness centrality (0..1). How often this node sits on the shortest path between other pairs of nodes in the affinity graph. 1.0 = the most structurally critical node on the mesh. High Bridge + low Traffic share = a quiet but irreplaceable chokepoint."; return `Bridge score ⓘ${bpct}% ${blabel}`; })() : ''} - ${(n.role === 'repeater' || n.role === 'room') && Array.isArray(n.transported_scopes) && n.transported_scopes.length ? `Transported scopes${n.transported_scopes.map(sc => '' + escapeHtml(String(sc)) + '').join('')}` : ''} + ${(n.role === 'repeater' || n.role === 'room') && Array.isArray(n.transported_scopes) && n.transported_scopes.length ? `Transported scopes${n.transported_scopes.map(sc => '' + escapeHtml(String(sc)) + '').join('')}` : ''} First Seen${renderNodeTimestampHtml(n.first_seen)} Total Packets${stats.totalTransmissions || stats.totalPackets || n.advert_count || 0}${stats.totalObservations && stats.totalObservations !== (stats.totalTransmissions || stats.totalPackets) ? ' (seen ' + stats.totalObservations + '×)' : ''} Packets Today${stats.packetsToday || 0}