feat(#1784): gate ingestor neighbor-edge creation on the path-trust threshold - #1863
Open
SaarMesh-Bot wants to merge 6 commits into
Open
feat(#1784): gate ingestor neighbor-edge creation on the path-trust threshold#1863SaarMesh-Bot wants to merge 6 commits into
SaarMesh-Bot wants to merge 6 commits into
Conversation
… shared helper Step 2 of the Kpa-clawbot#1784 multi-PR project (config + helper only; no consumer changes yet, per the bot triage's recommended ordering). Adds packetpath.TrustConfig / packetpath.MeetsPathTrust as the shared decision point future PRs will call into for neighbor_graph.go, path_resolver.go, neighbor_builder.go, and path_inspect.go, instead of each site reimplementing "prefix bytes = len(hex)/2" trust logic independently (DRY, per the call-site audit). Config: pathTrust.minHashBytesForMapping in config.json, mirrored in both cmd/server/config.go and cmd/ingestor/config.go (type-aliased to the shared packetpath.TrustConfig, same pattern as GeoFilterConfig). config.example.json documents the new section per AGENTS.md's config documentation rule (field + _comment_pathTrust: valid range, default rationale, trust-all opt-in). Operator-confirmed defaults for the 3 blocking design questions from the triage: - Default = 2 (conservative). This IS a behavior change once consumers wire it in: 1-byte prefix matches (256-value collision space) will no longer count as mapping/topology evidence by default. Operators who want the prior trust-all behavior can set minHashBytesForMapping: 1. - Legacy bucket-0 (pre-Kpa-clawbot#1638 persisted edges with no per-mode breakdown, NeighborEdge.CountsByMode[0]) is excluded once the threshold is >= 2, since we cannot prove those observations were >= 2 bytes. - Scope confirmed: config + helper only in this PR. node_reach.go (already empirical) and computeMultiByteCapability (separate confirmed/suspected axis) are explicitly out of scope, matching the triage's exclusions. Fixup 1: GetPathTrust() panicked on a nil *Config receiver (found while building the neighbor_graph.go follow-up consumer PR, which calls it from a Server/PacketStore that a test constructs without cfg). Guarded with a nil check, matching how a real nil-Config caller should degrade to the default threshold rather than crash. Added regression tests (TestGetPathTrustNilConfig) in both cmd/server and cmd/ingestor. Fixup 2 (review): MinHashBytesOrDefault now clamps MinHashBytesForMapping to MaxHashBytes (3) instead of accepting values above it silently — an unclamped typo like minHashBytesForMapping: 99 would otherwise exclude every observation (no real prefix exceeds 3 bytes) without any error. Added MaxHashBytes constant + clamping tests. Also added the missing config.example.json documentation flagged as a blocking MAJOR in review. Co-Authored-By: Claude <noreply@anthropic.com>
…urce (Kpa-clawbot#1777) ANON_REQ (payload type 0x07) carries the sender's full Ed25519 ephemeral pubkey (decoder.go's EphemeralPubKey / "ephemeralPubKey" field) — the same trust level as an ADVERT's pubKey. Both the ingestor's persisted-edge builder (neighbor_builder.go, writes neighbor_edges) and the server's in-memory mirror (neighbor_graph.go, BuildFromStore) only ever used ADVERT for the originator↔path[0] edge, missing ANON_REQ as a legitimate second source of full-pubkey originator identity. This implements suggestion Kpa-clawbot#1 from the issue discussion only. Suggestion Kpa-clawbot#2 (treating the 1-byte truncated src/dst hash on REQ/RESP/PATH/TXT as a hop) was explicitly rejected in triage and by the reporter themselves (damn-simple-scripts, 2026-06-22) — a 1-byte hash has ~1/256 collision odds and would manufacture false edges. REQ/RESP/PATH/TXT remain excluded from originator-edge creation; only their existing observer↔path[last] edge (unaffected by this change, already applies to all packet types) still applies. Changes: - cmd/ingestor/maintenance.go: extractPubkeyFromAnonReqJSON, mirroring the existing extractPubkeyFromAdvertJSON helper. - cmd/ingestor/neighbor_builder.go: payloadAnonReq const; hasFullOriginator bool (isAdvert || isAnonReq) gates the originator-edge branches; falls back to the new JSON helper when from_pubkey is empty (from_pubkey is only populated for ADVERT at write time, per db.go's Kpa-clawbot#1143 comment — left untouched here to keep this change scoped to the two call sites the triage identified). - cmd/server/neighbor_graph.go: extractFromNode also checks "ephemeralPubKey" (safe unconditionally — the field only appears in ANON_REQ's decoded_json via the shared Payload struct's omitempty); hasFullOriginator bool gates the zero-hop and Edge-1 branches the same way as the ingestor. Tests: 2 new cases in neighbor_graph_test.go (ANON_REQ produces the edge; REQ still doesn't) and 2 new cases in neighbor_builder_test.go (same, against the persisted DB path via the from_pubkey JSON fallback). All existing neighbor-graph/-builder tests still pass unchanged, including TestBuildNeighborGraph_ADVERTOnlyConstraint (still covers a non-ADVERT, non-ANON_REQ type; no rename needed). cmd/server full suite: pass, except 2 pre-existing flaky tests (TestHandleNodePaths_*_1352, an index-loading race unrelated to this change — reproduces intermittently on unmodified master too). cmd/ingestor full suite: pass. Co-Authored-By: Claude <noreply@anthropic.com>
…lPubKey on payload type (Kpa-clawbot#1822 review) Bot review round-1 nits (both minor, no blockers): 1. (munger) extractEdgesFromObs / edgeCandidate in neighbor_persist.go had zero callers anywhere in the codebase, including tests — verified via grep. Its doc comment claimed 'the in-memory graph builder (neighbor_graph.go) also calls it', which was already stale: BuildFromStore has its own inline ADVERT-only loop, unrelated to this function. Keeping an unreferenced parallel implementation around is exactly the drift risk the review flagged: if it were ever wired back up, it would silently reintroduce the ADVERT-only originator-edge bug this PR fixes elsewhere, since it was never updated for ANON_REQ. Deleted rather than duplicated the fix into dead code. 2. (carmack) extractFromNode's safety for including "ephemeralPubKey" unconditionally in the field-check list rested entirely on the invisible premise that no other payload type ever populates that JSON key. Gated the ephemeralPubKey lookup on tx.PayloadType == PayloadANON_REQ instead of trusting key-presence alone, so this stays correct even if a future decoder change reuses the field name for a different, non-originator purpose on some other payload type. No behavior change for ADVERT/ANON_REQ/REQ/RESP/PATH/TXT — all existing and Kpa-clawbot#1777 tests pass unchanged (TestBuildNeighborGraph_AnonReqSingleHopPath, TestBuildNeighborGraph_ReqRespStillExcluded, TestBuildNeighborGraph_ ADVERTOnlyConstraint, full neighbor-graph/-persist suite). cmd/server full suite: pass (2 runs clean; the previously-seen TestHandleNodePaths_*_1352 index-loading race did not reproduce in either run here, consistent with it being an unrelated pre-existing flake, not a regression from this change). Co-Authored-By: Claude <noreply@anthropic.com>
…ath-trust threshold Wires packetpath.MeetsPathTrust (added in Kpa-clawbot#1824, no consumers yet) into its first real call site: the ingestor's neighbor-edge builder, which owns the persisted neighbor_edges table (Kpa-clawbot#1287 Option 4). The gate lives inside resolvePrefix rather than at each call site, so every consumer of the resolver -- the originator edge, the observer edge, and any hop pair added later -- inherits the threshold without having to remember it. Why this matters at scale: a 1-byte hop hash has 256 possible values. On SaarMesh (1071 nodes) all 256 are occupied and only 13 nodes remain uniquely resolvable by a single byte, while 1-byte hops make up 69% of observed path elements. Such a resolution is unique only relative to the nodes currently known -- a later-joining repeater sharing that byte silently turns it into a wrong persisted edge. At 2 bytes, 1065 of 1071 nodes resolve uniquely, so the default threshold keeps the bulk of the evidence while dropping the unreliable tail. Scope note: the server-side in-memory graph (cmd/server/neighbor_graph.go) is deliberately NOT gated here. It already models short-prefix uncertainty explicitly -- Ambiguous edges with candidate lists, CountsByMode buckets (Kpa-clawbot#1638), and context-based disambiguation -- so a hard drop there would destroy information the graph is designed to carry, and would make CountsByMode[1] structurally unreachable. Gating derived views wants the threshold expressed as a confidence signal plus UI indication (per Kpa-clawbot#1784's acceptance criteria), which is a separate change. Existing builder tests use 1-byte fixtures to exercise edge shape; they now pass the legacy trust-all threshold explicitly so their intent is unchanged. Two new tests pin the gate itself. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wires
packetpath.MeetsPathTrustinto its first real consumer. #1824 added the config and the helper but deliberately changed no call sites, so the threshold currently has no effect anywhere — this PR makes it do something.What changed
The gate goes inside
resolvePrefixincmd/ingestor/neighbor_builder.gorather than at each call site. Every consumer of the resolver — the originator↔path[0] edge, the observer↔path[last] edge, and any hop pair added later — inherits the threshold without having to remember it.buildAndPersistNeighborEdgesandStartNeighborEdgesBuildertake the config through;cmd/ingestor/main.goresolves it once viacfg.GetPathTrust().Why 2 bytes is the right default at scale
A 1-byte hop hash has 256 possible values. Measured on SaarMesh (1071 nodes, 18.9M observed hop occurrences):
All 256 one-byte prefixes are occupied, so
resolvePrefixalready returns ambiguous for 98.8 % of nodes and those hops produce nothing today. The problem is the remaining 1.2 %: those prefixes are unique only relative to the nodes we currently know about. A later-joining repeater sharing that byte silently converts a "unique" resolution into a wrong edge — and because these are persisted edges, the error outlives the observation that caused it.Operators on smaller meshes, where a single byte is genuinely near-unique, keep the old behaviour with
"pathTrust": {"minHashBytesForMapping": 1}.What is deliberately not in scope
The server-side in-memory graph (
cmd/server/neighbor_graph.go) is not gated. I implemented it, and the existing tests correctly rejected it: that graph already models short-prefix uncertainty rather than discarding it —Ambiguousedges with candidate lists,CountsByModebuckets from #1638, andresolveAmbiguousEdgescontext disambiguation. A hard drop there deletes information the structure is built to carry and makesCountsByMode[1]structurally unreachable. Gating derived views wants the threshold expressed as a confidence signal plus the UI indication #1784's acceptance criteria call for. Separate change, separate PR.Relationship to other PRs
internal/packetpath/trust.goonly exists there, so its commits are included in this diff. If feat(#1784): add pathTrust.minHashBytesForMapping config + shared helper #1824 merges first, this shrinks to the ingestor changes.buildAndPersistNeighborEdgesland as one coherent unit. I'll close feat(neighbor-graph): treat ANON_REQ as an originator↔path[0] edge source (#1777) #1822 pointing here.minInteriorEdgeHashBytes = 2for the interior-hop edges after the numbers above came up. Because the gate lives inresolvePrefix, that loop inherits the shared threshold on rebase and the parallel constant can go.Testing
Existing builder tests use 1-byte fixtures to exercise edge shape; they now pass the legacy trust-all threshold explicitly so their intent is unchanged. Two new tests pin the gate:
TestNeighborEdgesBuilderPathTrustExcludesOneByteand...AllowsTwoByte.Full
cmd/ingestorsuite green (104s),cmd/serverneighbor/graph tests green.