Skip to content

fix(nodes): read the transport-direct path byte at offset 5 (#2225) - #2234

Merged
efiten merged 2 commits into
Kpa-clawbot:masterfrom
anieto:fix/issue-2225
Oct 16, 2026
Merged

efiten merged 2 commits into
Kpa-clawbot:masterfrom
anieto:fix/issue-2225

Conversation

@anieto

@anieto anieto commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

Red commit: 3bb8125 (fails on assertions both ways: zero-hop counts = map[pk_relayed:map[SAT:1]], want map[pk_transport:map[AUS:1]])

Fixes #2225

What this does

nodeZeroHopRegionCounts read the path byte at offset 1 for ROUTE_TYPE_TRANSPORT_DIRECT adverts. Transport routes carry 4 transport-code bytes between the header and path_length (firmware docs/packet_format.md), so offset 1 is the first byte of transport_code_1. Two effects, both fixed:

  • A transport-direct zero-hop advert counted as zero-hop only when that code byte's low 6 bits happened to be 0, so the node usually got fewest_hops instead of zero_hop, and Bug/feature: Use actual repeaters region data and not observer based #804 analytics missed it.
  • A relayed transport-direct advert whose first code byte was 0x00 (or 0x40, 0x80, …) counted as zero-hop.

How

  • New packetpath.PathByte(rawHex) (routeType int, pathByte byte, ok bool) applies the transport-code offset in one place, next to the existing IsTransportRoute, HashSize and DecodePathFromRawHex.

  • Every reader of the path byte from raw_hex in store.go now uses it:

    • nodeZeroHopRegionCounts (the bug);
    • computeAnalyticsHashSizes;
    • computeNodeHashSizeInfo;
    • computeMultiByteCapability.

    The last three already had the right offset, each with its own copy of the rule, so their behavior doesn't change. packetpath.HashSize uses PathByte too.

Performance

  • PathByte runs once per advert on every recompute, so it must not allocate. hexByteAt now decodes the two hex nibbles itself instead of calling hex.DecodeString; TestPathByteDoesNotAllocate pins it at 0 allocs.
  • Without that, BenchmarkRecomputeNodeHomeRegionSets went from 17,232 to 457,232 allocs/op. With it, the count is unchanged from master.
  • Time on the same machine (Apple M5, Docker linux/arm64, -count 3): ~61 ms/op on master, ~54 ms/op here. Your hardware measured ~3x higher on fix(nodes): place nodes in regions by zero-hop home regions (#1879) #2190, so expect proportionally more there; the change itself only removes work.

Tests

  • TestZeroHopCountsReadTransportDirectPathByte: a transport-direct zero-hop advert whose first code byte is 0xaa counts, and a relayed one (path byte 0x41) whose first code byte is 0x00 doesn't. Checked through both nodeZeroHopRegionCounts and computeNodeHomeRegions (Bug/feature: Use actual repeaters region data and not observer based #804).
  • TestPathByte covers all four route types, upper-case hex, and too-short or malformed input. TestPathByteDoesNotAllocate pins the allocation count.
  • go test passes for cmd/server, cmd/ingestor and internal/packetpath (CGO_ENABLED=1). gofmt and go vet are clean.

No API or UI change, no new map[string]interface{}, and no config.

…-clawbot#2225)

nodeZeroHopRegionCounts reads the path byte at offset 1 for
TRANSPORT_DIRECT too, but transport routes carry 4 transport-code bytes
first, so it reads transport_code_1. Fails on assertions both ways: a
zero-hop transport-direct advert is missed, and a relayed one whose first
code byte is 0x00 is counted.
…bot#2225)

nodeZeroHopRegionCounts read the path byte at offset 1 for
TRANSPORT_DIRECT, which is the first transport-code byte, so a
transport-direct zero-hop advert counted as zero-hop only by chance and a
relayed one could count by chance. This fed the Kpa-clawbot#804 home-region tally
and the zero_hop region placement.

New packetpath.PathByte returns the route type and path byte with the
transport-code offset applied, and every reader of the path byte from
raw_hex in store.go now uses it: the zero-hop tally, the hash-size
analytics, computeNodeHashSizeInfo, and the multi-byte capability scan.
The other three already had the right offset; this keeps one definition.
packetpath.HashSize uses it too.

hexByteAt now decodes the two nibbles itself instead of
hex.DecodeString, so PathByte doesn't allocate. It runs per advert on
every recompute; without this, BenchmarkRecomputeNodeHomeRegionSets went
from 17,232 to 457,232 allocs/op.

@efiten efiten left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this matches the firmware and closes #2225. Approving and merging.

What I checked at 0aa0ea17

  • Offset rule. firmware/src/Packet.h:14-17,67 (TRANSPORT_FLOOD 0x00, TRANSPORT_DIRECT 0x03, hasTransportCodes()) and Packet.cpp:56-59,69-74 put path_len after 4 code bytes for those two route types, so offset 5 for routes 0/3 and offset 1 otherwise.
  • Red/green. At 3bb81254, TestZeroHopCountsReadTransportDirectPathByte fails in both directions (zero-hop counts = map[pk_relayed:map[SAT:1]], want map[pk_transport:map[AUS:1]], and the same through computeNodeHomeRegions). At head it passes.
  • The three call sites you describe as unchanged. A throwaway differential test compared the old inline strconv.ParseUint logic with PathByte on 2,000,000 random strings (length 4-17, mixed case, odd lengths, non-hex characters): 0 mismatches. Shorter input is still caught by each call site's own guard.
  • Checks. go test -count=1 ./... passes in cmd/server, cmd/ingestor and internal/packetpath; go vet and gofmt are clean. TestPathByteDoesNotAllocate passes. I did not rerun BenchmarkRecomputeNodeHomeRegionSets.

Minor, no action needed

  1. cmd/server/store.go:4058 still checks len(tx.RawHex) < 4, which PathByte now covers. Harmless.
  2. DecodePathFromRawHex (internal/packetpath/path.go:25-31) and AdvertRouteEvidence (internal/packetpath/advert.go:27-30) still derive the transport offset inline. They work on decoded bytes, not raw_hex, so they are outside #2225; a shared pathByteOffset(routeType) could cover all three later.

@efiten
efiten merged commit 4fdd494 into Kpa-clawbot:master Oct 16, 2026
12 checks passed
anieto added a commit to anieto/CoreScope that referenced this pull request Oct 18, 2026
Picks up Kpa-clawbot#2190 (region placement), Kpa-clawbot#2234 (transport-direct path byte)
and 56 other upstream commits.

Conflicts:
- region_nodes.go: the 14-day direct-hearing snapshot still decides
  every node it knows; upstream placements (in-memory) decide nodes the
  snapshot does not know yet, then the observer scan.
- resolved_index.go / store.go: take upstream rpJob/applyResolvedPaths
  (Kpa-clawbot#2147, no SQL under s.mu); the batched resolved_path prewarm moves
  into applyResolvedPaths so every caller gets it, outside the lock.
- route-view.js: upstream true-hop count (Kpa-clawbot#2240) with our no-GPS vs
  unresolved split; the on-map badge uses the same hop count.
efiten added a commit that referenced this pull request Oct 22, 2026
Release notes for v3.15.0: `docs/release-notes/v3.15.0.md` and the
`CHANGELOG.md` entry. The release workflow uses the notes file as the
GitHub release body.

## What is in it

- 77 commits since v3.14.0 (76 merged pull requests plus this one): 27
fix, 23 perf, 19 feat, 3 test, 2 ci, 2 docs, 1 chore. Nineteen feats, so
minor.
- "Read this before upgrading" first. It covers:
  - the liveness change (#2261, #2262);
- the `users.db` v6 migration and what it means for a rollback (#2174);
- the backfill at every ingestor start (#2133) and the 24 h
planner-statistics refresh (#2158);
- the caches that trade freshness for latency (#2165, #2222, #2147,
#2156);
- changes in hop names and region placement (#2188, #2190, #2234, #2245,
#2220);
  - the role thresholds on favorites (#2215);
  - the geo-filter save on a single-file mount (#2137);
  - the `/api/stats` polling in hidden tabs (#2162).
- Features, fixes, performance, then tests/CI/docs.
- 29 issues closed, taken from each pull request's
`closingIssuesReferences`.
- Contributors: pull request authors and issue reporters with counts.
The commit co-author trailers only name the pull request's own author or
tooling.

## How it was checked

- I read every pull request body. Every number in the notes comes from a
pull request body.
- I checked the config keys and defaults against `config.example.json`
on `2bb000f2`.
- I checked the UNASKED and opt-in claims for #2125, #2133, #2235 and
#2239 against the source.
- For the other pull requests I relied on their bodies and did not open
the diffs.

## Not covered

- The uncertain count is a single count on one staging instance on the
day of the change (1,682 repeaters and rooms). It is not a before/after
comparison.
- #2133 gives no measured cost for its backfill on a large database, and
the notes say so.

## After merge

Tag `v3.15.0` on the merge commit once its `:edge` image exists. Then
check in the registry that `v3.15.0`, `v3.15`, `v3` and `latest` share
one digest, and that the release has the notes as its body and both
`corescope-decrypt` binaries.

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@anieto
anieto deleted the fix/issue-2225 branch October 26, 2026 15:37
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.

bug: zero-hop detection reads the path byte at the wrong offset for transport-direct adverts

2 participants