Skip to content

fix(nodes): separate advert timestamps from confirmed relay activity - #64

Merged
dborup merged 6 commits into
masterfrom
codex/fix-node-advert-relay-liveness
Sep 18, 2026
Merged

dborup merged 6 commits into
masterfrom
codex/fix-node-advert-relay-liveness

Conversation

@dborup

@dborup dborup commented Sep 17, 2026 •

Copy link
Copy Markdown
Owner

Summary

Fix false activity on a repeater's node-detail page: a recent ACK attributed through a colliding short hop hash was being displayed as the node's own advert, and ambiguous traffic could advance relay timestamps/counts.

  • Add nullable stats.lastAdvert to single-node and bulk health. It uses only exact-origin ADVERT packets and rejects adverts explicitly marked signature-invalid.
  • Correct stats.lastHeard to use exact own adverts or unambiguously attributed observed flood hops, while preserving packet/SNR/observation analytics.
  • Reject colliding prefixes even when a heuristic previously placed the packet in a full-public-key index. Missing evidence fails closed. Known unique 1/2/3-byte prefixes remain usable.
  • Exclude DIRECT/TRANSPORT_DIRECT planned routes from observed relay activity. Firmware removes traversed direct hops, whereas flood forwarding appends traversed hashes.
  • Share relay collection/aggregation between single-node and bulk paths, retaining deduplication and existing transported-scope provenance guards.
  • Render Last Heard (advert) independently from general activity in full detail and the desktop sidepane. Do not promote legacy health traffic or inferred directory timestamps into safe detail status. Remove unsupported alive (idle) claims.
  • Add backend, frontend-helper and real-SPA Playwright regressions, and wire the new browser test into CI.

Protocol reference: MeshCore routing implementation.

Validation

Navigator independently reviewed commit 623ccc0cb3ccf3db473a07c9185be48d1b99d96a and ran:

  • cd cmd/server && go test ./... — PASS (34.563s).
  • cd cmd/ingestor && go test ./... — PASS (87.186s).
  • cd cmd/server && go test -race -run 'Test(NodeHealth_LastAdvert|NodeActivity_|RepeaterRelayActivity_|TransportedScopes|GetRepeaterRelayInfoMap)' ./... — PASS.
  • node test-node-liveness-e2e.js — PASS: five synthetic cases, 25 checks across full detail and desktop sidepane.
  • node test-packet-filter.js — 92 passed; node test-aging.js — 19 passed.
  • node test-frontend-helpers.js — 693 passed, two unchanged favorite-icon baseline failures (base: 687 passed, two failed). All six new regressions pass.
  • Every one of the 88 Node scripts listed in test-all.sh was run independently on base and head: the same 12 scripts fail, listed below; no newly failing script. The fail-fast runner otherwise stops at the first baseline failure. The npm test coverage wrapper was not run because local c8 is unavailable; no dependency was installed or changed.
  • JS syntax checks and git diff --check — PASS.
  • go test -run '^$' -bench '^BenchmarkConfirmedRelayBulk30KPackets2KNodes$' -benchtime=3x -benchmem — PASS: 47.25 ms/op, 48,646,032 B/op, 184,648 allocs/op at 30K packets / 2K nodes. This is a local scale measurement, not a before/after speedup claim.

Independent browser QA reproduced the original false Active / recent Last Heard / alive (idle) presentation, then verified:

  • stale own advert with recent unsafe directory timestamps stays Stale;
  • an older backend's unsafe health timestamp cannot become an advert;
  • a recent exact own advert remains Active;
  • relay-only safe activity can be Active while its advert timestamp is unknown;
  • both full detail and the desktop sidepane render the separated evidence.

Existing baseline failures / draft gate

The developer explicitly authorized a draft PR, with existing failures documented and unrelated repairs excluded. On unchanged base 693eb045dbb10ecdd4657444134a696f007297e1, both full Go suites passed, but 12 of 88 frontend scripts in test-all.sh failed:

  1. test-frontend-helpers.js — two obsolete Unicode favorite-icon expectations.
  2. test-channel-psk-ux.js
  3. test-analytics-channels-integration.js
  4. test-observers-headings.js
  5. test-issue-1648-m3-emoji-scan.js
  6. test-issue-1648-m6-final-sweep.js
  7. test-issue-1648-m6-lint-self.js
  8. test-issue-1438-customizer-mcrole.js
  9. test-issue-1446-cb-preset-cascade.js
  10. test-issue-1470-node-tile-helper.js
  11. test-issue-1485-live-anim-z.js
  12. test-naive-banner-tone.js

This PR does not claim an all-green baseline or deployment readiness.

Review follow-up (commits after 623ccc0)

Head is now 611b4eea1b307bf34301675e4eb8dcf15792cc2f. Five follow-up commits address independent review findings; each finding was reproduced by a failing regression before the fix.

  1. 1a36f4bf — Relay evidence from every observation path. Relay attribution previously inspected only the display (longest) observation, so a relay named only by a shorter observation of the same flood was missed. All raw observation paths are now scanned with a strict flat-array scanner that accepts exactly what parsePathJSON accepts and validates the whole input before yielding any hop. Health parity: single/bulk health now fold in the same relay evidence as relay status, so a unique raw-prefix relay can no longer be RelayActive while lastHeard is null/stale. lastAdvert stays strictly the node's own advert.
  2. 89b1617b — E2E: a relay-only node renders Active with an unknown advert time and no contradictory Stale label (and vice versa), in full detail and sidepane.
  3. fa50931d — Relay evidence after restart. buildPathHopIndex re-indexes display paths only, dropping non-display resolved hops from full-key path-hop buckets, so relay status lost them after a restart while health still saw them. byNode[key] is now an additional candidate bucket for single relay info, bulk relay info and health. Membership is never proof: every candidate is verified against raw observed flood paths (collisions, listener-only nodes and DIRECT routes stay rejected), counted once per transmission, and byNode candidates never contribute transported scopes. Regression loads the same transmission via Load()+WaitIndexesReady (with and without persisted resolved_path) and via IngestNewFromDB, requiring identical activity. Performance: health evaluates path evidence once per transmission per node, only for candidates newer than the current lastHeard (relay counts, scopes and lastAdvert are not gated); a per-key matcher avoids per-token ToLower/prefix-map lookups; bulk memoizes token identities per request. No code path stops early based on index order.
  4. e4f166a7 — Data race fix. computeRepeaterRelayInfoMap walked copied byPathHop/byNode slice headers after RUnlock, while eviction compacts those slices in place and ingest appends; an unindexed transmission could map to index 0 and credit or deduplicate an unrelated transmission (wrong relay counts). All reads of store data now happen under the read lock; only aggregation over call-owned values runs after unlock; missing index entries are skipped, never remapped.
  5. 611b4eea — Concurrency regression strengthened (relay evidence reachable only via byNode) so the oracle check detects miscounts without -race; comment and index-type nits.

"Confirmed relay" throughout means unique identity attribution among known relay-capable candidates in an observed flood path — not cryptographic proof of forwarding.

Follow-up verification

Fresh on the merge result of this head with current master (99d336e4d2a647a406560a4430a31b18aa14fcbb, merge tree 7f83d09c3614818d9788aa811bfd997da410d061, no rebase; all master changes preserved, store.go = PR + master's relayTimes removal):

  • cd cmd/server && go test -count=1 . — PASS.
  • Targeted activity/health/relay/Load/eviction/scope/listener/collision tests — 159 PASS, 0 FAIL, 0 SKIP; same set with -race — PASS, no races.
  • TestRepeaterRelayInfoMap_ConcurrentIngestAndEviction with -race, 5 runs — PASS.
  • node test-node-liveness-e2e.js — 35 checks PASS.
  • node test-frontend-helpers.js — 693 passed, 2 failed (the unchanged favStar baseline; master itself: 687 passed, same 2 failed).
  • go vet, gofmt on PR Go files, node --check, git diff --check — clean.

Regression evidence (branch commits, not merge tree):

  • Restart regression: FAIL on 89b1617b (relay empty after load vs active on live ingest), PASS after fa50931d.
  • Concurrency regression on fa50931d: -race 3/3 FAIL; without -race 10/20 FAIL (“matches no store state”). On 611b4eea: -race 10/10 PASS, without -race 30/30 PASS. An instrumented copy counted 26–199 missing index lookups per run before the fix, 0 after.
  • Independent reviewers (not the implementer) checked each follow-up; matcher equivalence was differentially tested (~76M key/token checks, 0 mismatches) and an order/timestamp oracle (non-chronological candidates, mixed/invalid timestamps) found 0 mismatches. Previously reported results for 623ccc0c (ingestor suite, other frontend scripts, baseline list below) were not re-run and remain historical.

Follow-up benchmarks (local, Apple M5, not CI)

Dataset: 30K flood transmissions, 2K repeaters, 3 observations per transmission (2/3/4 hops, mixed 1/2/3-byte hashes), resolved hops of all observations indexed (live index shape), 5% own adverts. Interleaved runs, -benchtime 10x, n=8 per revision; median [range], allocs/op.

623ccc0c 89b1617b fa50931d
GetBulkHealth(2000) 166.3 ms [157–190], 1.20M 457.1 ms [441–480], 2.86M 52.9 ms [49–57], 0.54M
GetBulkHealth(200) 24.1 ms [23.3–25.3], 160K 77.4 ms [75.8–81.7], 443K 6.4 ms [6.2–7.8], 54K
GetNodeHealth (hot node, 18.5K tx) 9.3 ms [9.1–12.9], 46K 34.7 ms [34.2–43.8], 174K 1.8 ms [1.7–1.9], 2.1K
Bulk relay map 60.9 ms [56.9–63.1], 176K 99.7 ms [95.3–112], 316K 55.6 ms [54.7–61.6], 95K

Race fix vs fa50931d (same dataset, interleaved, n=8; measured on the e4f166a7 tree): health unchanged (bulk 2K 48.7 vs 49.2 ms, 200 nodes 6.3 vs 6.3 ms, hot node 1.8 vs 1.8 ms, identical allocs); bulk relay map 54.1 vs 55.7 ms, 94.7K allocs, 27→33 MB/op. Because the whole candidate traversal now holds the read lock, the maximum wait of a concurrent writer during a bulk relay recompute rose from ~26 ms to ~52 ms (3 runs × 400 lock acquisitions each; p50/p99 0 ms). JSON output of bulk health, hot-node health and the relay map is identical between fa50931d and the race fix, and identical to 89b1617b on this dataset; versus 623ccc0c only lastHeard changes (1109 of 2000 nodes, never older). The later int32→int change in 611b4eea was not separately benchmarked.

Follow-up limitations

  • A raw hop that appears only in an observation path and has no membership in either byNode or byPathHop for that node (e.g. never resolved) is not covered.
  • The byNode fallback does not widen transported-scope provenance; after a restart, scopes carried only by byNode candidates are not credited.
  • The benchmark dataset represents the live index shape; the restart shape is functionally tested but not separately benchmarked.
  • Eviction keeps resolved full-key path-hop entries for evicted transmissions (pre-existing policy, unchanged here).
  • The concurrency regression is timing-dependent: it detects the old bug in many but not all single runs.

Performance and configuration

No per-node/per-hop SQL or frontend API fanout is added. Bulk enrichment processes indexed transmissions with request-local membership/timestamp caching and bounded packet-store/index data. Existing background relay-cache refresh remains in place. A 30K-packet / 2K-node benchmark exercises attribution and deduplication.

No schema, DB writer, threshold, customizer, dependency or manual cache-buster changes. Production has not been modified.

Scope and limitations

  • Attribution is conservative: ambiguous heuristic-only paths no longer count as confirmed relays. Relay figures can intentionally decrease.
  • “Confirmed” means exact/unique identity attribution among known relay-capable nodes in an observed flood path, not cryptographic proof of forwarding or a connectivity probe.
  • Existing directory last_seen can contain historical ingestor inference. Detail/sidepane status no longer lets that outrank safe health evidence, but node-list/map directory freshness and ingestor relay-touch policy are outside this PR. There is no retroactive timestamp rewrite.
  • Activity/advert statistics retain the in-memory packet window; unknown timestamps are explicitly null.
  • No merge or production deployment is requested.

dborup and others added 6 commits September 17, 2026 14:35
Review findings on 623ccc0:

A) txHasConfirmedRelay only inspected the transmission's display path,
   i.e. the longest observation. A relay named only by a shorter
   observation of the same flood (indexed under its full key via that
   observation's resolved path) was rejected by relay status and health.
   Relay evidence now scans the display path plus every raw observation
   path, with an allocation-free scanner that accepts exactly what
   parsePathJSON accepts. Resolved/full-key membership is still not proof:
   collisions, listener-only nodes, planned DIRECT routes and malformed
   paths stay rejected.

B) GetNodeHealth/GetBulkHealth derived lastHeard from byNode only, while
   relay status also accepts unique 1/2/3-byte raw-prefix buckets, so a
   node could be RelayActive with a null or stale health lastHeard. Both
   health paths now fold in the same forEachConfirmedRelayTx evidence used
   by GetRepeaterRelayInfo and the bulk relay map (deduplicated per
   transmission). lastAdvert remains strictly the node's own advert.

Regression tests fail before and pass after. Packet/SNR/observation
statistics, transported-scope provenance, indexing and schema unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Relay-only activity must render Active with an unknown advert time and no
Stale label (and stale fixtures no Active label), in both the full node
page and the side pane.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
F1: after a restart buildPathHopIndex re-indexes display paths only, so a
relay named solely by a non-display observation lost its full-key
path-hop entry. Health (via byNode) still saw it; relay status did not.
byNode[key] is now a relay candidate bucket for single relay info, bulk
relay info and health. Candidates are never proof: each is verified
against its raw observed flood paths (collisions, listener-only, DIRECT
still rejected), counted once per transmission, and byNode candidates
never contribute transported scopes. New regression loads the same
transmission via Load()+WaitIndexesReady (with and without persisted
resolved_path) and via IngestNewFromDB and requires identical activity.

F2: health evaluated relay path evidence before its timestamp gate and
twice per transmission (byNode loop, then indexed pass). Now:
- byNode loop only handles own adverts; one relay pass over deduplicated
  candidates, newest first, skips transmissions not newer than lastHeard
  before any path work (relay counts/scopes are not gated);
- relayKeyMatcher decides a key's unique wire prefixes once and matches
  tokens without ToLower or prefix-map lookups;
- bulk relay map memoizes token identities per request, keeps confirmed
  keys as small slices and dedupes with generation stamps.
Scanner comment corrected: the scanner does not allocate, visit may.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
computeRepeaterRelayInfoMap copied byPathHop/byNode slice headers and
walked them after RUnlock. Eviction compacts those slices in place and
ingest appends to them, so the unlocked pass raced with writers and could
see transmissions that were never indexed: index[tx.ID] then returned 0
and credited or deduplicated an unrelated transmission (wrong relay
counts), as reproduced by a -race probe with real EvictStaleWithRP.

All reads of store data (candidate buckets, StoreTx fields, observations)
now happen under the read lock. Per key it records only indexes into the
call-owned per-transmission entries; aggregation runs after unlock on
those owned values. Missing index entries are skipped, never mapped to
another transmission. Deduplication, full-key priority, byNode fallback,
scope provenance and identity checks are unchanged. Comments no longer
claim append-only slices or chronological candidate order.

New -race regression runs bulk relay info and bulk health concurrently
with ingest and in-place eviction; every bulk result must equal the
single-path result of one store state observed between two rounds.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review follow-up. The regression indexed each relay under its full key in
byPathHop as well, which eviction does not compact, so a stale byNode
candidate never changed the bulk result and only the race detector saw the
bug. Relay evidence is now reachable only via byNode (restart shape): on
fa50931 the oracle check fails without -race too. Also fixes the eviction
cutoff comment (keepTx+1 remain) and uses int for candidate indexes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@dborup
dborup marked this pull request as ready for review September 18, 2026 03:49
@dborup
dborup merged commit 25bffcd into master Sep 18, 2026
6 checks passed
adminopenclaw8-sketch pushed a commit that referenced this pull request Sep 18, 2026
Brings in:
- #64: fix(nodes): separate advert timestamps from confirmed relay activity
- #48: test(ingestor): reconcile watchdog Stop-test coverage (StopJoinsLoop + StopIsIdempotent), fixing the CI watchdog flake this PR previously hit

No conflicts. PR #28's own change (public/rx-coverage.js) is untouched by this merge.
adminopenclaw8-sketch pushed a commit that referenced this pull request Sep 18, 2026
Brings in:
- #64: fix(nodes): separate advert timestamps from confirmed relay activity
- #48: test(ingestor): reconcile watchdog Stop-test coverage (StopJoinsLoop + StopIsIdempotent)

No conflicts. PR #50's own change (channel-rainbow.json, internal/channel/channel_rainbow_test.go) is untouched by this merge. Does not address the row-height test Kpa-clawbot#1122/Kpa-clawbot#1124 failure — that is investigated separately, unresolved.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant