Repository navigation
fix(reach): hide blacklisted and hidden identities on the Reach page - #68
Merged
Merged
Conversation
/api/nodes/{pubkey}/reach exposed identities that other views hide:
- The target check only covered the node blacklist and a hidden node
name. Observer-blacklisted pubkeys, nodes hidden by their observer
name, and observer-only pubkeys with a hidden name got a full 200
report (name, links).
- A visible node's report listed hidden and blacklisted neighbours and
direct observers by pubkey and name.
One shared rule now decides (identityHidden): node blacklist, observer
blacklist, or any node/observer name with a hidden-name prefix.
- Target: blacklists are checked before the cache; names are read live
(one bulk json_each query) on every serve, so a hidden target 404s
with the unknown-node body even when its report is cached.
- Lists: hidden identities are removed from links and direct_observers
on every serve, cached or not, with bidirectional_links and
direct_observers recounted. A rename, a prefix change or a blacklist
change applies on the next request; the cached body is reused while
nothing changes.
- A failed name lookup fails closed (500), never serving unfiltered
data.
- neighbor_degree / degree_rank / nodes_with_edges are unchanged here.
Tests cover each case, after cache warm-up and in both directions; they
fail against the previous handler. A before/after benchmark measures
the added per-request lookup.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ility Independent review follow-ups: - A node hidden by name that aged out of `nodes` into inactive_nodes still appeared in links by pubkey (its adverts stay in a 14/30-day window). The live name lookup now includes inactive_nodes (probed, so minimal DBs without the table keep working). - Observer ids arrive raw from the MQTT topic; match them case-insensitively (one pass over the observers table) instead of only exact upper/lower case. - Gate the lookup on HasHiddenNamePrefixes, which uses exactly IsNameHidden's predicate (a whitespace-only prefix counts), instead of ActiveHiddenNamePrefixes. - Docs: hiding applies on the next request; un-hiding by rename can take up to the cache TTL (the recorded name still counts). - Tests for each case plus the recorded-name guard; the inactive, mixed-case and recorded-name tests fail with the respective check removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
inactive_nodes keeps a node's old row when it returns to `nodes` (INSERT OR REPLACE on move, never deleted), so a node that went quiet as "🚫 …" and came back under a visible name stayed hidden. Consult an inactive name only while the pubkey has no nodes row. Test covers the returned node; the inactive-only case stays covered by fixture node G. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A hidden node that came back with a nameless advert got a nodes row with name '', which switched its hidden inactive_nodes name off and listed its pubkey again. An inactive name now counts unless the pubkey has a nodes row with a non-empty name. Docs state the rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ation - Visible controls stay listed with their own Reach page: a name that merely contains the hidden prefix, and a node visible under both its node and observer name. - Cache: a hit reuses the computed report; a hidden-prefix change is applied to that same cached report at serve time (no recompute, no bypass); a blacklist change purges and recomputes. - neighbor_degree / degree_rank / nodes_with_edges are identical with and without hidden identities configured (rank contract untouched). - Move the singleflight comment back above the singleflight call. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Final independent review: with a hidden-name prefix configured, a Reach cache hit scanned one row per listed identity plus the whole observers table, 1.7 ms (hub: 150 links, 100 direct observers) and 5.0 ms (2000 observers) vs ~0.1 ms on master. - The lookup now filters by prefix in SQL (byte-exact substr/length on BLOB = strings.HasPrefix, inlined per enforced prefix), so it normally returns no rows: 0.33 ms (hub) and 0.66 ms (2000 observers). Go still confirms every returned name with IsNameHidden. - Without prefixes the lookup is skipped entirely and the hot path does no per-identity lower-casing: ~24 µs, at or below master. - The name lookup uses the request context (a gone client frees its pool connection). - Config: one lazy hide-prefix helper shared by IsNameHidden, ActiveHiddenNamePrefixes and the new EnforcedHiddenNamePrefixes (which replaces HasHiddenNamePrefixes and also feeds the SQL). - Tests: realistic-scale benchmark; SQL prefix match is byte-exact (case, multi-byte, exact-length, shorter); only hiding names are returned; reachCacheSetBody never attaches a body to a newer entry; the nothing-hidden report is compared in full, not by counts. - Docs: un-hiding the target by rename can also lag up to the cache TTL; honest worst-case cache size. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ve NOT EXISTS to its row Final-review follow-up: production schemas always have inactive_nodes, so the scale benchmark now creates it (with aged-out rows, some for listed pubkeys) and measures the lookup's third branch. Binding the NOT EXISTS to i.public_key instead of j.value is ~7 % faster in an interleaved A/B (1.09 -> 1.01 ms at 300 links / 2000 observers). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Join nodes and inactive_nodes by primary key in a single pass over the listed pubkeys instead of a separate inactive_nodes branch with a correlated NOT EXISTS; "no named nodes row" becomes COALESCE(n.name, '') = '' on the joined row. Same results (tests and mutation checks unchanged), ~11 % faster in an interleaved A/B: hub 518 -> 460 µs, 300 links / 2000 observers 985 -> 883 µs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dborup
marked this pull request as ready for review
September 19, 2026 05:34
dborup
pushed a commit
that referenced
this pull request
Sep 19, 2026
Integrates the merged Reach privacy/visibility work (#68, identity_visibility.go) with the leaderboard's rank-view cache (reach_rank.go). Real conflicts in cmd/server/node_reach.go and docs/api-spec.md — both files touched the response cache and the handler — resolved by combining both freshness dimensions on the cache entry: - hiddenKey (#68): which identities the served body's links / direct_observers omit, from the live visibility check. - viewID (#69): which reachRankView the served body's rank fields were applied from. reachBody now re-marshals only when either one moved on since the entry was computed; both are re-checked on every serve, cached or not, so neither a blacklist/prefix/rename change nor a rank-view change waits for the 5-minute cache TTL. computeNodeReach no longer sets the rank fields itself (unchanged from #69's design) — the handler applies them from the shared rank view (applyReachRank) after visibility filtering, so a node's own NeighborDegree still counts a hidden neighbour's edge (a number, not an identity) while DegreeRank/NodesWithEdges reflect only the visible population. Also fixes a parallel visibility rule found during review: reachRankVisible in reach_rank.go reimplemented the same IsBlacklisted/IsObserverBlacklisted/IsNameHidden combination as identityHidden instead of calling it, so the two endpoints could have drifted apart on a future change to the shared rule. It now delegates directly. Updated two of #69's own tests that asserted the pre-merge cache API/contract (reachCacheSetBody's new viewID param; NothingHiddenBodyUnchanged and FilteringLeavesRankFieldsUnchanged now apply the rank view before comparing, and assert the agreed 'visible population' contract — NodesWithEdges/DegreeRank reflect only visible nodes, NeighborDegree does not). Added TestNodeReach_HiddenNeighbourCountedNotListedOrRanked (cross-endpoint: a hidden neighbour is counted but never listed or ranked) and TestReachRankVisible_MatchesIdentityHidden (equivalence probe against identityHidden across blacklist/prefix/name-slot combinations), and strengthened TestReachRank_OnlyValidEdgesCount with a self-edge that exists only in non-canonical case form, closing a gap where the self-edge skip could be removed without any existing test catching it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dborup
pushed a commit
that referenced
this pull request
Sep 19, 2026
BenchmarkReachAndRankParallel measures /api/reach-rank and
/api/nodes/{pk}/reach served concurrently (3:1) on a warm
snapshot/view/cache, at GOMAXPROCS parallelism. Confirms the shared
degree-snapshot/rank-view mutex is not a bottleneck under concurrent
readers — part of the performance verification for the PR #68 sync.
Co-Authored-By: Claude Sonnet 5 <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
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.
Summary
Two privacy gaps on the existing per-node Reach endpoint,
GET /api/nodes/{pubkey}/reach. An independent review of the Reach leaderboard work found them; both predate it. This PR fixes them on its own, based on master without any leaderboard changes.#69 (Reach leaderboard) depends on this PR and is to be merged after it: dborup/CoreScope#69.
Contract
Hidden identity. A pubkey's identity is hidden when any of these holds (
identityHiddenincmd/server/identity_visibility.go):nodeBlacklist(IsBlacklisted);observerBlacklist(IsObserverBlacklisted);hiddenNamePrefixesentry (IsNameHidden). Its names are:nodesname;observersname;inactive_nodesname, but only while it has nonodesrow with a non-empty name. The ingestor never deletes a returning node'sinactive_nodesrow, so an existing current name supersedes the old one. A nameless return, stored as an empty name, does not.Rules for
/api/nodes/{pubkey}/reach{"error":"Not found"}, byte-identical to an unknown node. This is the existing per-pubkey contract used by node detail, neighbours and the rest.linksanddirect_observers.bidirectional_linksanddirect_observerscount only what is listed. No other field names another identity:node,windowandreliable_tokensdescribe the target itself.reach computation failed, never unfiltered data.neighbor_degree,degree_rank,nodes_with_edgesandrelay_observationsare counts over the whole graph and name no identity. They are not changed here, and a test asserts they are identical with and without hiding. feat(reach): Reach leaderboard with a shared visible-population rank #69 redefines them for Reach and the leaderboard from one snapshot.Behaviour before and after
nodeBlacklistobserverBlacklistinactive_nodeswith a hidden nameisPubkeyHiddenfailed openneighbor_degree/degree_rank/nodes_with_edgesConsistency with existing visibility rules
inactive_nodesname, which closes a real leak for aged-out hidden nodes;identityHiddenis the rule the leaderboard in feat(reach): Reach leaderboard with a shared visible-population rank #69 will call.Implementation
cmd/server/identity_visibility.goidentityHiddenis the rule itself.hiddenIdentityNames(ctx, pubkeys)is one bulk query per request that returns only names starting with an enforced prefix, so normally no rows:substr/lengthon BLOB), equal tostrings.HasPrefixinIsNameHidden. It is inlined once per prefix, and Go confirms each returned name withIsNameHidden.nodesandinactive_nodesare joined by primary key in a single pass over the listed pubkeys; "no namednodesrow" isCOALESCE(n.name,'') = ''.observersis read in one pass, matched case-insensitively.inactive_nodesis probed first, so minimal DBs without it keep working.isIdentityHiddenis the same check for a single pubkey.cmd/server/config.goIsNameHiddenandActiveHiddenNamePrefixes, with unchanged behaviour.EnforcedHiddenNamePrefixes()returns exactly the prefixesIsNameHiddenenforces. Without them, the name lookup and all per-identity name work are skipped.cmd/server/node_reach.govisibleReachfilters it on every serve.hiddenKey).reachCacheSetBodyonly attaches a body to the entry it was computed for.Tests
All results below are from feature head
eb4afecd, the last code commit. The later master merge6ab3103fchanges no file of this PR; see Base, head and CI.Suites and static checks
cd cmd/server && go test -race ./...: ok (307 s).cd cmd/ingestor && go test ./...: ok (88.7 s).go vet ./...clean; gofmt clean on every changed.gofile;git diff --checkclean.Targeted tests (
cmd/server/node_reach_visibility_test.go)HiddenTargets404HiddenNeighboursFilteredlinksanddirect_observerssets and recounted counts. No hidden pubkey or name anywhere in the body, including an inactive-only hidden node and blacklisted identities.VisibleControlsNotOverfilteredWarmCacheHonoursVisibilityChangesCacheHitMissAndInvalidationFilteringLeavesRankFieldsUnchangedneighbor_degree,degree_rankandnodes_with_edgesare identical with and without hiding.NameLookupFailureFailsClosedRecordedNameStillHidesAfterRowDeletedWhitespacePrefixStillLooksUpNamesNoInactiveNodesTableinactive_nodestable.StaleInactiveNameDoesNotHideReturnedNodeNothingHiddenBodyUnchangedHiddenIdentityNames_LargeBatchOnlyHidingNamesHiddenIdentityNames_PrefixMatchIsExactIsNameHidden: case, multi-byte, exact-length, shorter, not-at-start.ReachCacheSetBody_StaleEntryUntouchedNegative controls. Each of these mutations was applied to the final code in a scratch copy. Every one makes at least one of the tests above fail:
HiddenTargets404,WarmCache…,BlacklistMutationBustsCacheHiddenNeighboursFilteredandCacheHitMissAndInvalidationWarmCache…,CacheHitMiss…,WhitespacePrefix…,StaleInactive…HiddenTargets404,HiddenNeighboursFilteredinactive_nodeslookup removedHiddenNeighboursFiltered,LargeBatch…,StaleInactive…StaleInactive…NameLookupFailureFailsClosedRecordedNameStillHides…PrefixMatchIsExact,LargeBatch…atguard removedReachCacheSetBody_StaleEntryUntouchedLinux. GitHub CI (
ubuntu-latest) is the Linux verification; see CI below. A local Linux container was not practical: the Docker daemon isn't running here, and a Go image would have to be downloaded.Performance
BenchmarkNodeReachCacheHitVisibility(50 links, 5 direct observers) and…Scalemeasure a Reach cache hit.inactive_nodeswith aged-out rows.modernc.org/sqlitev1.34.5, load average about 3–4. Values are medians of-count=5 -benchmem, from the same benchmark file run on master and at headeb4afecd.Why it costs more with a prefix. It is the live per-request name lookup, which is what makes a rename apply on the next request instead of after the 5-minute cache TTL. It costs roughly 1 µs per listed identity plus about 0.13 µs per observers row.
config.example.jsonshipshiddenNamePrefixes: ["🚫"], so deployments using it take this path.What the review-driven optimisation bought. The first version returned every name of every listed identity. At the hub size it took 1.7 ms, and 5.0 ms at 2000 observers. Changes since then:
nodesandinactive_nodeslookups are merged into one pass;Query variants (interleaved A/B, same process):
Decision: the cost is accepted. The measured ~102–873 µs per cache hit with a prefix configured is accepted in exchange for immediate, fail-closed privacy. There is no extra name snapshot and no further optimisation in this PR.
Alternative not taken: a short-TTL shared name snapshot would make hits roughly master-speed. Renames would then take effect within that TTL instead of on the next request.
Review
A separate reviewer agent, which did not write the code, reviewed this PR in four rounds. It read the commits through git and ran its own probes and mutations in isolated copies.
be05d38e,783ca759): no blockers. Fixed:25986c9c): no BLOCKER. All seven requirements hold, and there is no path that serves unfiltered data. That covers cache hits, singleflight waiters, error paths and body reuse; a concurrent churn probe under-racefound 0 leaks.9c8352e8,b4de6b27andeb4afecd; see Performance.at-guard test, request context, deep-compare test, docs, prefix helper.9c8352e8, theneb4afecd): no BLOCKER.IsNameHiddenwith 0 mismatches over 2880 identities and 32 prefix sets. The inputs covered NULL, empty, whitespace, multi-byte, invalid UTF-8 and NUL, with the mergednodes/inactive_nodeslogic.Base, head and CI
Synced with master. master was merged into this branch with an ordinary merge commit; there was no rebase and no force-push.
6ab3103feb4afecd, the last code commit;8b9b9d60, after the merges of fork PRs 28 and 50.10530a0e. It is identical to the merge tree precomputed read-only withgit merge-treebefore merging.37d82e8a..eb4afecd..github/tree is identical to master's, including triggers, permissions, jobs,needsand fork guards.go vet(server, ingestor) are clean, and so isgit diff --check;-race -count=3;cd cmd/server && go test -race ./...is ok (337 s).CI run 35437448578: ✅ success. It ran on head
6ab3103fas apull_requestrun. The checked-out merge ref4a4efd72(master8b9b9d60+6ab3103f) has the same tree,10530a0e.E2E tests of interest:
test-channel-color-picker-e2e.js: 9/9, including "ArrowRight cycles focus across swatches".test-issue-1630-reach-mobile-e2e.js: 7/7.test-issue-1122-packets-filter-ux-e2e.jssuite passes 6/6, including "Path column row height stays bounded < 60px";test-issue-1122-details-row-clamp-e2e.jspasses 18/18.Side effects. Nothing was deployed, pushed to GHCR, released or committed as badges. Coverage badges were only uploaded as CI artifacts.
Previous run (35424296968, head
eb4afecd, against master31c2aa8e): Go passed. E2E failed on one assertion in the unchangedtest-channel-color-picker-e2e.js, "ArrowRight cycles focus across swatches". It was not re-run; this fresh run on the synced head replaces it, and that test passes there.Known limitations
inactive_nodesname./api/nodes/{pk}/neighborsdoes not filter its per-neighbour entries, so a visible node's neighbour list can still show hidden or blacklisted neighbours. This was found by reading the code in review./api/nodes/{pk}and/api/observers/{id}still show observer-blacklisted pubkeys and nodes hidden only by their observer name. A node detail page can therefore offer a Reach link that now returns 404./api/nodes/{pk}/rx-coverage(opt-in client RX coverage) checks only the node blacklist and node name for its target.Not tested
Staging and production were not tested. Validation used local Go tests and a local server on the committed CI e2e fixture DB, plus GitHub CI on Linux (
ubuntu-latest). Nothing was deployed.🤖 Generated with Claude Code