perf: remove per-StoreTx ResolvedPath, replace with membership index + on-demand API decode

[Kpa-clawbot]

Revision history: v1 → v2 (folds in expert critique comment 1 and implementer review comment 2). Strikethroughs removed for readability; see comments for the original v1 critique threads.

Problem

Heap profiling on a representative server (388K observations, 37K transmissions) shows the in-memory packet store consumes ~630 MB. The dominant cost (per pprof inuse_space):

Source	Heap	%
PacketStore.Load()	445 MB	71%
↳ unmarshalResolvedPath (JSON decode of resolved_path column)	205 MB	33%
↳ sqlite.columnText (raw column reads)	100 MB	16%
↳ reflect.New (json reflection)	41 MB	7%
IngestNewObservations (steady state)	123 MB	20%
IngestNewFromDB	48 MB	8%
Subpath/path-hop indexes (combined)	~10 MB	1.5%

Extrapolating to a database with 1.66M observations (real user report in #791): roughly 1 GB just to deserialize and hold ResolvedPath slices on every StoreTx/StoreObs — directly responsible for OOM kills during startup on memory-constrained deployments.

Why ResolvedPath is so expensive

StoreTx.ResolvedPath and StoreObs.ResolvedPath are []*string — one full hex pubkey per hop, parallel-aligned with PathJSON. Loading 1M+ observations means:

• 1M+ JSON arrays decoded via encoding/json reflection
• 1M × ~5 hops × ~64 bytes per hex string = ~320 MB of small string allocations
• Each []*string slice header is ~24 bytes, plus pointer slots
• Held in heap for the lifetime of the process

Complete Audit of ResolvedPath / resolved_path Consumers

#	Location	Function	What it does	Refactor strategy
1	store.go:553–580	addToByNode	Indexes relay nodes from resolved hops into byNode	Decode-window: extract pubkeys at ingest before discarding
2	store.go:603–645	touchRelayLastSeen	Updates relay last_seen in DB (#660 / #755 area)	Decode-window: same as #1
3	store.go:513	pickBestObservation propagation obs→tx	Copies best obs's resolved path to tx	Goes away with field; consumers fed via decode window
4	store.go:832–833, 2177, 2213, 2237	txToMap/obsToMap	REST API serialization	On-demand SQL fetch (part B) for cold; broadcast capture for hot
5	store.go:1572, 1622, 1830–1831, 1921	IngestNewObservations / IngestNewFromDB	WebSocket broadcast + persist	Decode-window: pull resolved_path JSON straight into broadcast map and persist batch — never store on struct
6	store.go:2305–2336	nodeInResolvedPath	Disambiguation post-filter for "Paths through node X"	Replaced by membership index lookup + collision-safety SQL fallback
7	store.go:2441–2456	addTxToPathHopIndex	Indexes tx under raw bytes AND full resolved pubkeys in byPathHop	Decode-window: feed pubkeys into byPathHop at ingest before discarding strings; also feed the new resolvedPubkeyIndex
8	store.go:2469–2490	removeTxFromPathHopIndex	Removes tx from index entries on eviction/path-change	NEW: maintain reverse map txID → []uint64 indexedHashes so eviction can find what to remove
9	routes.go:2211, 2235	mapSliceToStoreTxs / mapSliceToObservations	Rehydrates from broadcast	Dead-code removal — broadcast still carries the JSON bytes, but they don't land on the struct
10	neighbor_persist.go:384, 451, 493	backfillResolvedPathsAsync	Backfill writes obs.ResolvedPath back to struct + persists to SQL	Refactored: backfill writes only to SQL + updates the membership index + updates byPathHop resolved-key entries; never touches struct
11	config.go:95	BackfillResolvedPathHours config	Config knob	Unchanged
12	main.go:154–161	ensureResolvedPathColumn	Schema migration	Unchanged — column stays

Frontend (no API contract change required — resolved_path stays in broadcast maps and API responses):

• public/packet-helpers.js:40–49 — getResolvedPath() parser
• public/hop-resolver.js:277 — resolveFromServer()
• public/packets.js:252, 387, 1183, 1768, 1961, 2328 — caching + detail panes
• public/live.js:547, 2026, 2112–2124 — broadcast consumption

Proposed refactor (revised)

Remove ResolvedPath []*string from StoreTx and StoreObs. Replace with three narrowly-scoped data structures and one code-flow rule:

A) Membership index (forward)

// resolvedPubkeyIndex maps an 8-byte xxhash64 of a resolved pubkey
// to the list of transmission IDs whose resolved path contains it.
resolvedPubkeyIndex map[uint64][]int   // pubkeyHash → []txID

• Built once during Load() from the same SQL data (decode → hash → insert → discard the strings).
• Replaces nodeInResolvedPath(tx, pk) with txID in resolvedPubkeyIndex[hash(pk)] followed by collision-safety check.
• Coverage: indexes pubkeys from every observation's resolved_path on the tx, not just the "best" observation. This matches current nodeInResolvedPath semantics (it scans all obs).

B) Reverse map for eviction (NEW — addresses critique #6)

// resolvedPubkeyReverse maps txID to the set of pubkey hashes it was
// indexed under. Required for clean removal on eviction / path change.
resolvedPubkeyReverse map[int][]uint64 // txID → []pubkeyHash

• Populated at the same time as the forward index.
• removeTxFromResolvedPubkeyIndex(txID) reads this, deletes from forward index, deletes the reverse entry. O(hops) per tx.
• Memory cost: ~5 hops × 8 bytes × 1M tx = ~40 MB + map overhead. Cheap.

C) Decode-window discipline (NEW — addresses gotchas #1–#5, #7, #10)

Rule: resolved_path JSON bytes flow through ingest exactly once, and during that one decode window:

1. The JSON is parsed into a temporary []*string (stack-allocated when small).
2. All consumers are fed in order: addToByNode, touchRelayLastSeen, addTxToPathHopIndex (resolved-pubkey branch), resolvedPubkeyIndex insert, resolvedPubkeyReverse insert, broadcast map (raw JSON bytes), persist batch (raw JSON bytes).
3. The []*string is dropped at the end of the function — never lands on StoreTx/StoreObs.

This single rule makes the refactor coherent: every consumer that needs resolved pubkeys gets them at the only time they're guaranteed cheap to produce.

D) On-demand SQL fetch for cold-path API responses

For txToMap/obsToMap calls that serve packets that came in via Load() (not the live ingest path), do a SQL lookup:

SELECT id, resolved_path FROM observations WHERE id IN (?, ?, ?, ...)

• Single batch query per API request, returning ≤500 rows.
• LRU cache (10K entries) is mandatory for the live polling path (addresses critique Potential XSS: decoded.text not escaped in node detail panel #5).
• For hot packets (those still in the broadcast pipeline), no SQL needed — the broadcast carries resolved_path directly.

E) Backfill refactor (addresses critique #3, gotcha #10)

Pseudocode for the new backfillResolvedPathsAsync inner loop:

for batch := range pendingObs {
    for _, obs := range batch {
        rp := resolvePathForObs(obs.PathJSON, obs.ObserverID, tx, pm, graph)
        rpJSON := marshalResolvedPath(rp)
        // 1. SQL update (keeps existing behavior)
        sqlUpdate(obs.ID, rpJSON)
        // 2. Update forward + reverse index
        s.mu.Lock()
        for _, pk := range rp {
            if pk == nil { continue }
            h := xxhash64(*pk)
            // Old entries (if any) are removed via the reverse map
            removeOldHashes(obs.TransmissionID)
            s.resolvedPubkeyIndex[h] = append(s.resolvedPubkeyIndex[h], obs.TransmissionID)
            s.resolvedPubkeyReverse[obs.TransmissionID] = append(s.resolvedPubkeyReverse[obs.TransmissionID], h)
        }
        // 3. Update byPathHop resolved-key entries (similar pattern)
        s.mu.Unlock()
    }
}

The remove-old/add-new sequence works because the reverse map tells us exactly what to remove — even though the field is gone.

F) xxhash64 collision strategy (addresses critique #8)

• Probability: ~1 in 4 billion per pair at 1M unique pubkeys. In practice, expect zero collisions.
• Detection + safety: when resolvedPubkeyIndex[h] returns candidates, the /api/nodes/{pubkey}/paths handler does a single batched SQL query against the resolved_path column to confirm the exact pubkey appears in each candidate's resolved path. Same query, same code path as the on-demand SQL fetch (D).
• No collision lists, no secondary hash, no open addressing. The SQL safety net handles it.
• Code: confirmResolvedPathContains(txIDs []int, pk string) []int — one helper, one query, called only on the "Paths through node" hot path.

G) Schema migration

No schema change required. The resolved_path SQLite column stays — it's still the source of truth for ingest-time resolution, the on-demand decode path, and the collision-safety check.

Memory budget (revised, addresses critique #2)

Structure	Estimate at 1M obs / 50K unique pubkeys	Estimate at 1M obs / 500K unique pubkeys
resolvedPubkeyIndex (forward map)	~50 MB	~120 MB
resolvedPubkeyReverse (reverse map)	~40 MB	~40 MB
LRU cache for API on-demand decode (10K × ~200 B)	~2 MB	~2 MB
Total new structures	~92 MB	~162 MB
Removed: per-StoreTx/Obs ResolvedPath slices	~1 GB	~1 GB
Net savings	~900 MB	~840 MB

Out of scope

• Replacing JSON with protobuf/CBOR for the resolved_path column. The point is to stop decoding the column en masse at startup, after which the on-disk format becomes irrelevant for OOM. JSON is fine for the hundreds of rows decoded per API request.
• Changing the neighbor affinity graph or backfill resolution logic.
• Removing the byPathHop raw-byte half (graceful degradation when resolved_path is NULL is preserved).

Required tests (revised)

Unit (cmd/server/store_test.go, routes_test.go, neighbor_persist_test.go)

• TestResolvedPubkeyIndex_BuildFromLoad — fixture with known pubkeys → after Load(), forward + reverse maps are consistent.
• TestResolvedPubkeyIndex_HashCollision — crafted-vector test with two different pubkeys producing the same xxhash64. Confirm /api/nodes/{pubkey}/paths returns only the true match (collision-safety SQL filters the false candidate).
• TestResolvedPubkeyIndex_IngestUpdate — after IngestNewObservations, both maps reflect new entries; no ResolvedPath field exists on the struct.
• TestResolvedPubkeyIndex_RemoveOnEvict — eviction removes all entries via reverse map; forward map has no orphan txIDs.
• TestResolvedPubkeyIndex_PerObsCoverage — fixture where best obs and a non-best obs have different resolved paths; both sets of pubkeys appear in the index.
• TestStoreTx_NoResolvedPathField — compile-time guard on struct shape.
• NEW: TestAddToByNode_WithoutResolvedPathField — relay nodes still indexed into byNode post-refactor.
• NEW: TestTouchRelayLastSeen_WithoutResolvedPathField — relay last_seen still updated; integration with bug: nodes only used for relaying/pathed traffic show as dead #660 / feat: repeater liveness indicator with relay stats (#662) #755 untouched.
• NEW: TestWebSocketBroadcast_IncludesResolvedPath — broadcast maps still carry resolved_path for the frontend (decode-window capture works).
• NEW: TestBackfill_UpdatesIndexAndByPathHop — backfill writes to SQL AND populates the new forward/reverse index AND updates byPathHop resolved-key entries; verify with a fixture starting from all-NULL.
• NEW: TestBackfill_RemoveOldOnReBackfill — if an obs is backfilled twice (e.g., graph improved), reverse map enables removing old hash entries before adding new ones.

Endpoint (routes_test.go)

• TestPathsThroughNode_PrecisionAfterRefactor — identical results before/after on a prefix-collision fixture.
• TestPathsThroughNode_NilResolvedPathFallback — packets with NULL resolved_path still returned via raw-byte index match.
• TestPathsThroughNode_CollisionSafety — when the hash index returns candidates due to a (crafted) collision, the SQL safety check filters the false candidate.
• TestPacketsAPI_OnDemandResolvedPath — /api/packets includes resolved_path for cold packets via SQL fetch.
• TestPacketsAPI_OnDemandResolvedPath_LRUHit — second request for the same packets uses the LRU cache (verify with cache hit counter).
• TestPacketsAPI_OnDemandResolvedPath_Empty — NULL resolved_path in DB returns null (or omitted) in API.
• TestLivePolling_ResolvedPathFromBroadcast — live polling responses include resolved_path from the in-flight broadcast cache, no SQL hit.
• TestLivePolling_LRUUnderConcurrentIngest — under 100 concurrent live polls + ingest writes, p95 latency < 50ms.

Integration / regression

• TestRepeaterLiveness_StillAccurate — relay-detection logic from PR feat: repeater liveness indicator with relay stats (#662) #755 area returns identical counts.
• TestSubpathDetail_StillWorks — /api/analytics/subpath-detail returns identical results (uses resolved hop names indirectly).
• TestNodeDetailPage_RelayCount — Node Detail "relayed N packets" count matches pre-refactor.

Memory / performance

• BenchmarkLoad_BeforeAfter — 100K observations fixture; runtime.MemStats.HeapAlloc after Load(). Target: ≥80% reduction (was ≥50% in v1; revised based on revised budget).
• BenchmarkResolvedPubkeyIndex_Memory — actual heap of the new structures at realistic 50K and 500K unique-pubkey distributions; verify within revised budget.
• BenchmarkPathsThroughNode_Latency — query node with ~5K candidates. Target: equal or faster.
• BenchmarkPacketsAPI_FirstPage — /api/packets?limit=100 end-to-end. Target: <20 ms regression.
• BenchmarkLivePolling_UnderIngest — sustained live polling at 1 Hz under continuous ingest at production rates. Target: p99 < 100 ms.

Manual validation (release checklist)

• Start server with the Existing SQLite database prevents CoreScope from becoming reachable, empty DB starts immediately #791 user's database (or synthetic 1M-observation DB). Verify startup completes in a 1 GB container.
• On Node Detail → Paths tab: open a node known to have prefix collisions. Verify only that node's actual paths appear.
• On Live page: verify hop names render correctly without flicker on new packets.
• On Packets browser: filter by node, verify resolved hop column shows correct names.
• After running for 1h: verify backfill log shows "[store] async resolved_path backfill complete" with no errors and the index reflects backfilled entries.

Risks (revised)

• Hash collisions: ~1 in 4 billion at 1M keys. Handled by the SQL safety check on the only hot lookup path. Risk: low.
• On-demand SQL latency under live polling: mitigated by mandatory LRU cache. Risk: medium until benchmarked.
• Concurrent index update: the forward + reverse maps share s.mu with everything else in the store. No new locking surface, but the backfill path now does more work under the write lock — verify backfill batch sizes don't cause noticeable API hiccups. Risk: low–medium.
• Backfill re-indexing: addressed by the reverse map. Risk: low.
• Subtle correctness bug in production: addressed by feature flag (see acceptance criteria).

Acceptance criteria (revised)

1. All required tests pass.
2. Startup heap with the Existing SQLite database prevents CoreScope from becoming reachable, empty DB starts immediately #791 database fits within a 1 GB container limit.
3. /api/nodes/{pubkey}/paths returns byte-identical results before and after on the regression fixture, including the prefix-collision fixture.
4. BenchmarkLoad shows ≥80% heap reduction.
5. Feature flag useResolvedPathIndex bool (default true in v3.6.0; default-off path falls back to the current per-StoreTx behavior for one release as a rollback safety net).
6. No new force-pushes, no schema migration required, no changes to firmware assumptions.
7. No new ResolvedPath field on StoreTx / StoreObs (compile-time guard test).

Estimated effort

8–12h for a senior Go developer familiar with the codebase (revised from v1's optimistic 4–6h):

• 4h: Core refactor (field removal, Load() index build, all decode-window consumers wired)
• 2h: On-demand SQL fetch + LRU cache for API endpoints
• 1h: Backfill refactor + reverse-map maintenance
• 1h: Memory accounting helper updates
• 2h: Unit + endpoint + collision tests
• 1h: Integration testing, benchmarks, edge cases (NULL resolved_path during backfill window, feature flag toggle)

Related: #791, #743, #748, #555, #660, #755. Replaces the approach in PR #796 (closed).

[Kpa-clawbot]

Critical review: #799 — ResolvedPath removal + membership index

Overall: A well-profiled, well-scoped proposal that correctly identifies the dominant heap cost and proposes a reasonable architecture — but several implementation details need resolution before this is safe to build.

Prioritized concerns

1. The byPathHop index already indexes by full resolved pubkey — what happens to it?
The proposal introduces resolvedPubkeyIndex but doesn't mention that addTxToPathHopIndex already indexes each tx under its resolved pubkeys in byPathHop (store.go ~L2456). With ResolvedPath removed from StoreTx, addTxToPathHopIndex loses its resolved-pubkey input. The proposal must explicitly address whether byPathHop keeps resolved-pubkey entries (fed from the new index?) or drops them (breaking the addCandidates(lowerPK) full-pubkey lookup in routes.go ~L1168). This is the most likely source of a silent correctness regression.
Resolve: Specify exactly how addTxToPathHopIndex gets resolved pubkeys post-refactor, or replace its resolved-key branch entirely with the new index.

2. Memory estimate for resolvedPubkeyIndex is optimistic.
The proposal estimates 80–120 MB for map[uint64][]int. But Go maps have significant per-bucket overhead (~50-70 bytes per entry for small slices), and each []int slice header is 24 bytes. With 1M observations × 5 hops = 5M index entries across maybe 50K unique pubkey hashes, each mapping to an average of 100 txIDs: the []int backing arrays alone are 5M × 8 bytes = 40 MB, but the map overhead for 50K entries is small. However, if unique pubkeys are closer to 500K (more realistic for large meshes), each mapping to ~10 txIDs, the map overhead becomes non-trivial. Run a quick simulation or benchmark with realistic cardinality before committing to the estimate.
Resolve: Add a BenchmarkResolvedPubkeyIndex_Memory test that measures actual heap with realistic key/value distributions.

3. Backfill interaction is under-specified and has a concurrency hazard.
backfillResolvedPathsAsync currently runs under s.mu.Lock() in batches and mutates obs.ResolvedPath in-place (store.go ~L1840–1880). The proposal says it "must instead update the resolved-pubkey index after writing to SQLite" — but this requires careful coordination: the backfill decodes JSON, extracts pubkeys, updates the index, AND updates the byPathHop index (since resolved pubkeys are currently added there too). The current backfill also does removeTxFromPathHopIndex with the old resolved path before adding with the new one — with the field gone, how does it know what to remove? This is not trivial.
Resolve: Write pseudocode for the refactored backfill path showing the remove-old/add-new sequence without ResolvedPath on the struct.

4. nodeInResolvedPath fallback to observations is critical and easy to break.
The current function (store.go ~L2309) checks tx.ResolvedPath first, then falls back to scanning each obs.ResolvedPath. The proposal replaces this with an index lookup, but the index is keyed by txID — observations with resolved paths that differ from the "best" observation's resolved path (e.g., different observer saw a different route) would need all their pubkeys indexed too. Does the index cover per-observation resolved paths or only the best observation's? The current code scans ALL observations. Missing this means false negatives on the paths-through-node query.
Resolve: Explicitly state whether the index covers all observations' resolved paths or only the best. If only best, justify why that's sufficient (it may be — but prove it).

5. On-demand SQL fetch for /api/live polling could be a latency trap.
Live polling hits every ~2 seconds. If each poll needs a SQL round-trip to fetch resolved_path for new packets, and the SQLite DB is under write pressure from ingest, WAL contention could push latency beyond acceptable bounds. The proposal mentions an optional LRU cache but doesn't commit to it.
Resolve: Make the LRU cache non-optional for the live endpoint, or benchmark under concurrent ingest load and prove it's unnecessary.

6. removeTxFromPathHopIndex needs resolved pubkeys it no longer has.
On eviction or path change, the current code reads tx.ResolvedPath to know which index keys to remove (store.go ~L2469). Without that field, removal requires either (a) a reverse index from txID → indexed keys, or (b) re-fetching from SQLite. Neither is mentioned. This is a correctness bug waiting to happen — stale index entries pointing to evicted txs.
Resolve: Design the removal strategy. A reverse map txID → []uint64 (hashes it was indexed under) is the cleanest approach — add it to the proposal.

7. No rollback plan.
The proposal says "no schema migration required" — good. But if the refactored code has a subtle correctness bug in production (e.g., false negatives on paths-through-node), the rollback is "revert the entire commit and redeploy." Given the scope of changes across store.go, routes.go, and types.go, this could be a complex revert. Consider a feature flag (useResolvedPathIndex bool) that falls back to the old behavior, at least for the first release.
Resolve: Add a feature flag or config toggle, or explicitly accept the revert-and-redeploy risk.

8. xxhash collision handling is hand-waved.
The proposal says collision rate is ~1 in 2^32 and "falls through to the raw-byte index." But the raw-byte index post-filter currently calls nodeInResolvedPath which reads tx.ResolvedPath — the field being removed. So what does the collision fallback actually look like post-refactor? If the membership index says "yes" due to collision, and there's no secondary check, you get false positives in the paths-through-node results.
Resolve: Spell out the exact collision-handling code path post-refactor. If the answer is "accept rare false positives," say so explicitly and quantify.

Recommendation: PROCEED-WITH-CHANGES

The direction is correct and the problem is real. But concerns #1, #3, #4, #6, and #8 each represent a path to silent correctness regressions. Resolve those in the spec before implementation begins. The memory benchmarking (#2) and live-endpoint latency (#5) can be validated during implementation.

---

Critical review — expert review pass

[Kpa-clawbot]

Implementer Review

Bottom line: ship it after addressing the gotchas below. The design is sound — the two-pronged approach (membership index + on-demand decode) correctly targets the dominant heap cost. But there are ~8 call sites and 3 code paths the proposal doesn't explicitly address, and the effort estimate is tight.

Implementation Gotchas

1. addToByNode reads ResolvedPath (store.go ~553–580)
The addToByNode helper iterates obs.ResolvedPath and tx.ResolvedPath to index relay nodes into the byNode map. With the field removed, this needs to either: (a) consume the resolved pubkeys at ingest time before discarding them, or (b) query the membership index in reverse (index stores pubkeyHash→[]txID, but here we need txID→[]pubkeys). Option (a) is simpler — just feed addToByNode during the ingest decode window before discarding strings.

2. touchRelayLastSeen reads ResolvedPath (store.go ~603–645)
Same pattern — iterates obs.ResolvedPath and tx.ResolvedPath to find relay pubkeys for last_seen DB updates. Must be fed during the decode window at ingest time. The current code runs under s.mu write lock, so the refactored version must extract pubkeys before releasing them.

3. WebSocket broadcast maps include resolved_path (store.go ~1572, ~1831)
IngestNewObservations and IngestNewFromDB both build broadcast maps with obs.ResolvedPath. These maps go to the frontend via WebSocket. With the field removed from the struct, the broadcast code must either: (a) include resolved_path from the decode window (before discarding), or (b) omit it from broadcasts and let the frontend fall back to client-side resolution. Option (a) preserves current behavior with no frontend change.

4. pickBestObservation propagates ResolvedPath from obs→tx (store.go ~513)
This copies best.ResolvedPath to tx.ResolvedPath. With both fields removed, this line disappears — but callers of tx.ResolvedPath in touchRelayLastSeen, addToByNode, nodeInResolvedPath, and addTxToPathHopIndex all need the replacement path.

5. asyncPersistResolvedPathsAndEdges reads obs.ResolvedPath for marshaling (store.go ~1622)
The persist path collects marshalResolvedPath(obs.ResolvedPath) for SQL UPDATE. Must capture rpJSON during the decode window and pass it to the persist batch.

6. obsToMap and txToMap include resolved_path (store.go ~2177, ~2213, ~2237)
These are used by the REST API handlers. With the field removed, these must do the on-demand SQL fetch. But note: txToMap and obsToMap are called under s.mu.RLock() in multiple routes. Spinning up SQLite reads while holding the store RLock should be fine — the SQLite connection pool is separate from the mutex, and reads are non-blocking with WAL mode. But verify there's no write path that holds s.mu and also waits on a SQLite write (which would deadlock with WAL checkpoint). Current code already does s.db.TouchNodeLastSeen under write lock, so this pattern exists and works.

7. mapSliceToStoreTxs / mapSliceToObservations in routes.go (~2211, ~2235)
These rehydrate ResolvedPath from broadcast maps back into structs. With the field removed from the struct, these become no-ops for resolved_path. But the broadcast maps (gotcha #3) still carry the data for the WebSocket JSON serialization, so no functional change needed — just dead code removal.

8. Memory accounting (store.go ~2660–2673)
estimateStoreObsBytes has a perResolvedPathElemBytes cost per element. With the field removed, this block disappears. The new resolvedPubkeyIndex map needs its own accounting: len(index) * 8 (key) + sum(len(txIDs)) * 8 (int slice elem) + map overhead.

Call Sites the Proposal Missed

Full grep of ResolvedPath / resolved_path in Go (excluding tests):

Location	What it does
store.go:553–580 (addToByNode)	Indexes relay nodes from resolved hops into byNode
store.go:603–645 (touchRelayLastSeen)	Updates relay last_seen in DB — #660 / #755 area
store.go:832–833 (txToMap)	REST API serialization
store.go:1572,1622 (IngestNewObservations)	WebSocket broadcast + persist
store.go:1830–1831,1921 (IngestNewFromDB)	WebSocket broadcast + persist
store.go:2177,2213,2237 (obsToMap/txToMap)	REST/detail API serialization
routes.go:2211,2235 (mapSliceToStoreTxs/mapSliceToObservations)	Broadcast rehydration
neighbor_persist.go:384,451,493 (backfill)	Backfill writes obs.ResolvedPath back to struct
config.go:95	Config for backfill hours
main.go:154–161	ensureResolvedPathColumn

Frontend Touch Points

These files consume resolved_path from API/WebSocket — no changes needed as long as the API response shape stays identical:

• public/packet-helpers.js — getResolvedPath() parser (lines 40–49)
• public/hop-resolver.js — resolveFromServer() (line 277)
• public/packets.js — cacheResolvedPaths() (line 252), detail pane (lines 387, 1183, 1768, 1961, 2328)
• public/live.js — broadcast consumption (line 547), resolveHopPositions (lines 2026, 2112–2124)

Key point: if WebSocket broadcasts stop including resolved_path (gotcha #3 option b), live.js falls back to client-side HopResolver — but this degrades accuracy for prefix-colliding nodes. Recommend keeping resolved_path in broadcasts.

xxhash Collision Strategy

Recommended: open-addressing with post-filter fallback.

At 1M unique pubkeys, xxhash64 has a ~1-in-4-billion collision probability per pair — effectively zero collisions expected. But for correctness:

1. resolvedPubkeyIndex[hash(pk)] returns []txID candidates.
2. For the nodeInResolvedPath replacement: the caller already has the target pubkey. On a candidate hit, do a single SQL query (SELECT resolved_path FROM observations WHERE transmission_id IN (?) AND resolved_path LIKE '%<pk>%') to confirm. This is the same on-demand decode path (part B of the proposal), so collision handling is essentially free.
3. No need for separate collision lists or open addressing in the map itself — Go maps handle bucket collisions internally. The collision we care about is semantic (two different pubkeys mapping to the same uint64 key), and the post-filter handles it.

Do NOT add a collision list or secondary hash. The probability doesn't justify the complexity. The post-filter SQL path is the correct safety net.

Effort Estimate

The proposal says "4–6h." Realistic estimate: 8–12h for a senior Go developer familiar with the codebase.

• 4h: Core refactor (remove field, build index in Load(), update addToByNode, touchRelayLastSeen, nodeInResolvedPath, addTxToPathHopIndex, broadcast paths, persist paths)
• 2h: On-demand decode for API endpoints (txToMap/obsToMap → SQL fetch, LRU cache for live polling)
• 1h: Memory accounting update + backfill interaction
• 2h: Unit tests (the proposed list is solid but missing addToByNode and touchRelayLastSeen coverage)
• 1h: Integration testing, benchmark validation, edge cases (NULL resolved_path during backfill window)

The 4–6h estimate is achievable only if you skip the on-demand SQL path and keep resolved_path in broadcasts from the decode window (never hitting SQL for API responses on hot-path packets). That's actually a viable V1: decode at ingest, feed all consumers, discard. Only Load() packets need the on-demand SQL path.

Additional Test Cases Needed

Beyond the proposed list:

• TestAddToByNode_WithoutResolvedPathField — relay nodes still indexed into byNode after refactor
• TestTouchRelayLastSeen_WithoutResolvedPathField — relay last_seen still updated
• TestWebSocketBroadcast_IncludesResolvedPath — broadcast maps still carry resolved_path for frontend

---

Implementer review:

[Kpa-clawbot]

Critical review (round 2): #799

Round-1 concern resolution

1. byPathHop resolved-key branch loses its input — ✅ Resolved. Consumer audit row WS handlers trigger full page reloads on every packet — no debouncing #7 explicitly addresses this: decode-window feeds pubkeys into byPathHop at ingest, and row Global window._xxx functions never cleaned up in destroy() #8 adds the reverse map for eviction. The addTxToPathHopIndex/removeTxFromPathHopIndex pair is covered.

2. Memory estimate optimistic — ✅ Resolved. Revised budget now has two columns (50K vs 500K unique pubkeys) and includes reverse map overhead. BenchmarkResolvedPubkeyIndex_Memory is in the test plan.

3. Backfill concurrency hazard — ✅ Resolved. Section E provides pseudocode showing remove-old/add-new via reverse map under s.mu. The pattern is clear.

4. nodeInResolvedPath per-observation coverage — ✅ Resolved. Consumer audit row packets.js renderLeft() rebuilds filter bar on every WS message #6 + section A explicitly state the index covers pubkeys from every observation's resolved path, not just the best. TestResolvedPubkeyIndex_PerObsCoverage test is listed.

5. On-demand SQL latency on live polling — ✅ Resolved. LRU cache (10K entries) is now marked "mandatory" in section D. TestLivePolling_LRUUnderConcurrentIngest benchmark added.

6. removeTxFromPathHopIndex needs resolved pubkeys it no longer has — ✅ Resolved. Section B (reverse map) directly addresses this. Clean O(hops) removal.

7. No rollback plan — ✅ Resolved. Feature flag useResolvedPathIndex added as acceptance criterion Potential XSS: decoded.text not escaped in node detail panel #5, default-off for one release.

8. xxhash collision handling hand-waved — ✅ Resolved. Section F spells out the exact path: index returns candidates → batched SQL confirmResolvedPathContains verifies exact pubkey match. One helper, one query.

Summary: all 8 round-1 concerns are substantively addressed, not just acknowledged.

New concerns from revision

N1 (medium). Feature flag doubles the testing surface.
useResolvedPathIndex means two fully functional code paths through Load(), nodeInResolvedPath, all decode-window consumers, and txToMap/obsToMap. That's not a toggle — it's a fork. Every test must run twice (flag on and off), or the off-path rots silently. The test plan doesn't mention flag-off variants.
Ask: Add at least 3 key tests (Load, nodeInResolvedPath, txToMap) with useResolvedPathIndex=false to the plan. And commit to removing the flag in v3.7.0 — not "eventually."

N2 (medium). Decode-window ordering under s.mu grows the critical section.
Section C lists 6 consumers that must fire in sequence during the decode window, all under the write lock. Today, IngestNewObservations already does significant work under s.mu, but adding resolvedPubkeyIndex insert + resolvedPubkeyReverse insert + byPathHop resolved-key update extends it. At production ingest rates (hundreds of packets/sec), measure the p99 lock hold time before and after.
Ask: Add BenchmarkIngestLockHoldTime to the test plan — even a simple time.Since assertion would catch regressions.

N3 (low–medium). Reverse map drift on partial failure in backfill.
Section E pseudocode does removeOldHashes(txID) then adds new entries. If the process crashes between remove and add (or between SQL write and index update), the forward index loses entries that SQL has. On next startup, Load() rebuilds from SQL — so forward index self-heals. But during the runtime gap, nodeInResolvedPath queries return false negatives for affected txs. This is acceptable if documented.
Ask: Add a one-line comment in the backfill pseudocode: "Transient false negatives possible on crash between remove-old and add-new; self-heals on next restart."

N4 (low). LRU cache invalidation on backfill.
The on-demand SQL cache (section D) caches resolved_path for cold packets. When backfill runs and populates a previously-NULL resolved_path, the cached entry is stale (null). The LRU has no invalidation hook from the backfill path.
Ask: Either (a) invalidate LRU entries for backfilled obs IDs, or (b) set a short TTL (e.g., 60s) so stale entries self-expire during the backfill window. Option (b) is simpler.

N5 (low). TestStoreTx_NoResolvedPathField as "compile-time guard" is misleading.
It's a runtime reflection check, not a compile-time guarantee. If someone adds the field back and doesn't run tests, it slips through. This is fine — just don't call it "compile-time."
Ask: Rename to TestStoreTx_ResolvedPathFieldAbsent and note it's a reflection check.

N6 (low). 8–12h estimate is honest but front-loaded.
The 4h core refactor block covers field removal + Load() index build + ALL decode-window consumers. That's touching addToByNode, touchRelayLastSeen, pickBestObservation, addTxToPathHopIndex, removeTxFromPathHopIndex, broadcast capture, persist capture, and the feature flag branch — across ~15 call sites. 4h is tight. If this block slips to 6h, the total hits 14h.
Ask: No change needed — just flagging that the estimate has ≤2h slack. Plan accordingly.

Verdict

GO — proceed with implementation.

All round-1 concerns are resolved. The new concerns (N1–N6) are implementable within the existing plan — N1 and N2 need test-plan additions, N3–N4 need one-line fixes, N5–N6 are cosmetic. The architecture is sound, the consumer audit is thorough, and the collision strategy is pragmatic. Ship it.

---

Critical review (round 2)

[Kpa-clawbot]

Implementer review (round 2): #799

Round-1 gotcha resolution status

R1#	Gotcha	Addressed?
1	addToByNode/indexByNode reads ResolvedPath	✅ Audit row #1 now covers it. Strategy: decode-window feed.
2	touchRelayLastSeen reads ResolvedPath	✅ Audit row #2. Same decode-window strategy.
3	WebSocket broadcast includes resolved_path	✅ Audit row #5. Decode-window capture into broadcast map.
4	pickBestObservation propagates RP from obs→tx	✅ Audit row #3. Goes away with the field.
5	asyncPersistResolvedPathsAndEdges marshals obs.ResolvedPath	✅ Folded into row #5 decode-window.
6	removeTxFromPathHopIndex needs RP for eviction	✅ Part B reverse map addresses this cleanly.
7	No rollback plan	✅ Feature flag added to acceptance criteria #5.
8	xxhash collision hand-waved	✅ Part F: batched SQL confirmation query on the one hot path.

All 8 round-1 items are addressed. The reverse map and decode-window discipline are the two key structural additions that make this spec implementable.

Audit table line-number verification

Row	Spec says	Actual code	Correct?
1	store.go:553–580 / addToByNode	Lines 553–580 are inside hopCount() + start of indexByNode. The actual RP iteration is lines ~598–630. The function consuming RP is indexByNode, not addToByNode (which is just the dedup helper at L634).	⚠️ Function name wrong. Should say indexByNode (calls addToByNode).
2	store.go:603–645 / touchRelayLastSeen	touchRelayLastSeen starts at L650, not L603. Lines 603–630 are still inside indexByNode.	⚠️ Line range off by ~50.
3	store.go:513 / pickBestObservation propagation	pickBestObservation is at L541; the tx.ResolvedPath = best.ResolvedPath line is at L560.	⚠️ Line off by ~47.
4	store.go:832–833, 2177, 2213, 2237 / txToMap/obsToMap	txToMap RP at L2261–2262. obsToMap RP at L2225–2226. The obs-within-tx serialization at L2284–2285. L832–833 are nowhere near these. L879–880 is where txToMap checks RP.	⚠️ Multiple lines wrong.
5	store.go:1572, 1622, 1830–1831, 1921	L1620–1621 (broadcast RP in IngestNewObservations), L1670–1671 (persist marshal), L1878–1879 (broadcast in IngestNewFromDB), L1896/1917–1922 (old RP save/restore for path-hop re-index). Close enough.	✅ Approximately correct.
6	store.go:2305–2336 / nodeInResolvedPath	Function starts at L2357, not L2305.	⚠️ Off by ~50.
7	store.go:2441–2456 / addTxToPathHopIndex	Function at L2490.	⚠️ Off by ~50.
8	store.go:2469–2490 / removeTxFromPathHopIndex	Function at L2514.	⚠️ Off by ~45.
9	routes.go:2211, 2235 / mapSliceToStoreTxs	L2290 and L2299/2314.	⚠️ Off by ~80.
10	neighbor_persist.go:384, 451, 493 / backfill	Backfill function starts at L352. The obs.ResolvedPath = r.rp assignment is at L493 (the in-memory update under write lock). L384 and L451 don't reference RP directly.	⚠️ Partially wrong.

Verdict on audit table: Every function reference is real and the refactor strategies are correct, but almost all line numbers are off by 30–80 lines (likely from a stale git show before recent PRs shifted things). Not a blocker — the function names are sufficient for implementation — but worth noting that the spec was written against an older revision.

MISSING from audit: eviction path in evictOldTransmissions

The audit table doesn't list the eviction cleanup code at ~L2870–2910 which iterates obs.ResolvedPath and tx.ResolvedPath to clean byNode/nodeHashes. This is a separate consumer from removeTxFromPathHopIndex (row #8). The reverse map (Part B) handles the byPathHop cleanup, but the byNode/nodeHashes cleanup also needs the resolved pubkeys.

Fix: Eviction must also use the reverse map to look up which pubkeys a tx was indexed under in byNode. Or maintain a parallel reverse map for nodeHashes. Since nodeHashes already stores txHash → set, the simplest approach: resolvedPubkeyReverse already has the pubkey hashes — convert back to full pubkeys via the forward map's key space... except you can't reverse an xxhash. You need to store the actual pubkey strings in the reverse map, not just hashes, or maintain a separate txID → []string for nodeHashes cleanup.

This is a real gap. The decode-window rule means the pubkey strings are gone after ingest. But eviction happens later. Options:

1. Store txID → []string{pubkeys} reverse map (adds ~50 bytes × 5 hops × 1M tx ≈ 250 MB — defeats the purpose)
2. On eviction, do a single SQL query to fetch resolved_path for the evicted tx's observations, decode, and clean up. Eviction is already O(n) and infrequent — one SQL batch per eviction cycle is fine.
3. Keep nodeHashes indexed only by decoded JSON fields (pubKey/destPubKey/srcPubKey) and drop the resolved-path entries from it entirely. The byNode map can be rebuilt from the membership index for query purposes.

Recommend option 2 — it's consistent with the on-demand SQL pattern and eviction is a cold path.

Concurrency under concurrent backfill + ingest + eviction

The revised backfill (Part E) does removeOldHashes + add under s.mu.Lock(). Ingest also updates the forward/reverse maps under s.mu.Lock(). Eviction also runs under s.mu.Lock(). Since all three share the same mutex and Go mutexes are exclusive, there's no concurrency hazard per se — but the concern is lock contention.

Current backfill already holds s.mu.Lock() for the in-memory update phase (one batch at a time, typically 1000 obs). The new work under lock adds: iterate resolved pubkeys → update forward + reverse maps + byPathHop. This is ~5 hash lookups + map inserts per observation — negligible.

No new concurrency hazard. The existing serialization via s.mu is sufficient.

LRU cache invalidation on graph mutation

When backfill re-resolves an observation (graph improved, new advert arrived), the resolved path may change. The LRU cache (Part D) caches obsID → resolved_path from SQL. If backfill writes a new resolved_path to SQL, the LRU entry becomes stale.

Fix: Backfill must invalidate LRU entries for affected observation IDs. Since backfill already knows which obs it updated, this is a simple cache.Delete(obsID) call in the backfill loop. Add this to the spec.

Batched collision-safety SQL

The spec says confirmResolvedPathContains(txIDs []int, pk string) does a batched SQL query. The implied query is something like:

SELECT transmission_id FROM observations
WHERE transmission_id IN (?, ?, ...) AND resolved_path LIKE '%' || ? || '%'

Injection risk: The pubkey is a hex string (0-9a-f, 64 chars). As long as it's passed as a parameterized ? and not interpolated, there's no LIKE-pattern injection. But: LIKE '%hex%' can't use an index — it's a full scan of the matched rows. Since the IN clause narrows to typically <100 rows (candidates from the membership index), this is fine.

One subtle bug: If pubkey abc123 is a substring of pubkey abc123def456, the LIKE query would false-match. Pubkeys are 64-char hex strings and the JSON format is ["abc...","def..."] with quotes. Use LIKE '%"' || ? || '"%' to require the surrounding quotes. Or use json_each for exact matching — but that's slower and unnecessary given the quote-wrapping.

Recommend: LIKE '%"' || ? || '"%' — simple, correct, and the IN clause keeps the scan small.

Index usage for the on-demand SQL fetch (Part D)

SELECT id, resolved_path FROM observations WHERE id IN (?, ?, ...)

observations.id is the integer primary key → this is an index lookup. Fast. No concerns.

Decode-window discipline enforceability

The spec proposes a "rule" that resolved_path strings never land on the struct. Can this be a compile-time invariant?

Yes, partially: Remove the ResolvedPath field from StoreTx and StoreObs structs entirely. The test TestStoreTx_NoResolvedPathField (already in the spec) enforces this via reflection. Any future contributor adding the field back would break the test.

But the decode-window ordering (all consumers fed in sequence) can't be enforced at compile time. A future contributor could add a new consumer that reads RP after the decode window closes. The mitigation: clear documentation in the decode-window function's godoc + the field literally doesn't exist on the struct, so they'd have to pass it explicitly. That's about as good as it gets without a type-system enforcement Go can't provide.

Verdict: Sufficiently enforceable. The struct field removal is the hard constraint; the ordering is a soft convention protected by godoc.

Feature flag feasibility

The spec says useResolvedPathIndex bool with fallback to old behavior. This requires keeping the old ResolvedPath field on the struct in the fallback path, which contradicts the "remove the field" directive.

Cleaner approach: The feature flag controls which query path is used for nodeInResolvedPath:

• Flag ON: membership index lookup + SQL confirmation
• Flag OFF: iterate resolved_path from SQL on-demand (no index, no struct field — just slower)

Both paths work without the struct field. The flag doesn't need ugly branching everywhere — it only affects nodeInResolvedPath and addCandidates in the paths-through-node handler. Two code paths in one function, not scattered branching. Feasible and clean.

Test harness compatibility

• TestBackfill_RemoveOldOnReBackfill: Requires a fixture where the same obs is backfilled twice with different graphs. The current test harness supports this — backfillResolvedPathsAsync can be called twice with different store.graph values. The test needs a mock prefixMap that returns different results based on graph state. Existing store_test.go already has similar fixtures. Testable.

• TestPathsThroughNode_CollisionSafety: Requires crafting two pubkeys with the same xxhash64. Finding an xxhash64 collision is computationally trivial (birthday attack on 64 bits... actually no, that's 2^32 work). Easier approach: Mock the hash function in the test, or inject the collision directly into the index. The test doesn't need a real collision — it needs to verify the SQL fallback works when the index returns false candidates. Inject two txIDs under the same hash key, one with the target pubkey in SQL and one without. Testable.

Ready to ship?

Yes, with two small spec amendments:

1. Add eviction's byNode/nodeHashes cleanup to the audit (row 11–12) with strategy: on-demand SQL fetch of resolved_path for evicted observations, same pattern as Part D.
2. Add LRU cache invalidation to the backfill path (Part E step 4: cache.Delete(obsID) for each updated obs).

Neither requires architectural changes — they're natural extensions of existing patterns in the spec.

Updated effort estimate

10–14h (up from 8–12h). The eviction path cleanup and LRU invalidation add ~1h each, and the collision-safety SQL needs the LIKE '%"pk"%' pattern care. The test surface is large but well-defined.

• 4h: Core refactor (field removal, decode-window, index build in Load())
• 2h: On-demand SQL fetch + LRU cache + invalidation
• 1.5h: Backfill refactor + reverse-map + eviction cleanup
• 1h: Memory accounting updates
• 3h: Full test suite (unit + endpoint + collision + benchmark)
• 1.5h: Integration testing, feature flag, edge cases

---

Implementer review (round 2):

[Kpa-clawbot]

Closing in favor of #800, which contains the final consolidated spec after two rounds of expert + implementer review. This issue is preserved for the discussion + critique history.