Repository navigation
port(upstream#1913): key the zero-hop advert hash-size skip on the path byte - #32
Merged
Merged
Conversation
…e route type (Kpa-clawbot#1913) ## Summary `computeNodeHashSizeInfo` skips zero-hop direct adverts by **route type**. It should skip them by the **content of the path byte**, because the two cases are no longer the same thing. A zero-hop direct advert carries no path, so its hop count is 0. Whether the two size bits next to it mean anything depends on the sender: - Firmware that predates [meshcore-dev/MeshCore#3293](meshcore-dev/MeshCore#3293) does `packet->path_len = 0` in `Mesh::sendZeroHop()`, wiping the whole byte including the size bits. `0x00` genuinely says nothing about the node's `path.hash.mode` — skipping it is right, and Kpa-clawbot#649 was right. - A sender that writes the size through `setPathHashSizeAndCount()` emits `0x40` (2 bytes) or `0x80` (3 bytes) with a zero hop count. On a zero-hop packet nothing else can set those bits, so they are a deliberate declaration. Kpa-clawbot#653 landed the skip as `pathByte & 0x3F == 0`, which swallows the second case too. The diagnosis in Kpa-clawbot#649 had actually proposed `pathByte == 0x00`; the review widened it on the reasoning that a zero hop count always implies zeroed size bits. That was true in April, when no firmware wrote them. It is not true now. On the Czech mesh (869.4 MHz), a 24h window of 10k packets holds **54 zero-hop direct adverts: 39 at `0x00` and 15 carrying a declared size** (14× `0x40`, 1× `0x80`). ## Why it matters for display, not just tidiness Measured on one node over a 7-day window. A companion was reconfigured from a 2-byte to a 3-byte path hash. Its first advert under the new setting was a zero-hop direct one on **24 Aug 15:36 UTC** declaring `0x80`. That packet was dropped, so the node kept reading as 2-byte until its next **flood** advert arrived on **25 Aug 10:18 UTC** — 18h42m serving a configuration the analyzer had already been told was stale, confirmed against both an unpatched and a patched instance. With local adverts typically every 2h and flood adverts every 25h, that gap is the normal case rather than a corner one. It bites hardest on an instance whose retention window is shorter than a flood advert interval: there the node has *no* countable advert at all and falls out of `hash_size` entirely (which is what Kpa-clawbot#1912 is about on the rendering side). ## Change `(pathByte & 0x3F) == 0` → `pathByte == 0x00`, in `computeNodeHashSizeInfo` and in `computeAnalyticsHashSizes` so the two views agree. `isZeroHop` renamed to `isUndeclaredZeroHop` in the latter, since that is now what it means. No complexity change — same single byte comparison inside the existing scan. ## Measured A/B Two builds of the **same commit**, one with the change, both run read-only against the same copy of a real 181k-transmission / 973-node database: | | baseline | patched | |---|---|---| | nodes changed | — | **1** | | nodes regressed | — | **0** | | `hash_size_inconsistent` | 6 | **6** | | `multi_byte_status` split | 726 / 161 / 86 | unchanged | The flip-flop flag not moving is the point worth checking: a node that legitimately changes its mode mid-window is still handled by the recency decay from Kpa-clawbot#1788, so reading these packets does not resurrect false "varies". ## Tests `cd cmd/server && go test ./...` → **ok**, 0 failures. Coverage 83.5%, unchanged from master. 5 new cases in `cmd/server/zerohop_hashsize_test.go`, two built from real off-air packets: - zero-hop DIRECT `0x40` → `HashSize 2` (was: dropped) - zero-hop DIRECT `0x80` → `HashSize 3` - zero-hop DIRECT `0x00` → still absent from the map, i.e. Kpa-clawbot#649's behaviour preserved - TRANSPORT_DIRECT at path-byte offset 5, declared vs wiped - the declared size reaching `computeMultiByteCapability` as `confirmed`, which is what the map's multi-byte overlay reads **One existing test changed, flagging it explicitly:** `TestHashSizeTransportDirectZeroHopSkipped` used `0x40` as its "should be skipped" fixture. It now uses `0x00` — the case it was written to cover, since Kpa-clawbot#747 was about the missing `RouteTransportDirect` skip rather than about the size bits. The `0x40` case is covered by the new tests with the opposite expectation. ## Deliberately not touched The decoders (`cmd/server/decoder.go:648`, `cmd/ingestor/decoder.go:1045`) still report `HashSize 0` for these packets, so per-packet views keep showing the size as unknown. Arguably they should follow the same rule, but that changes packet display rather than node attribution and felt like a separate call for you to make. ## Caveat worth stating This attributes a declared size to the pubkey inside the advert. That holds as long as the advert was transmitted by the node that owns it — the same assumption the existing zero-hop **flood** path already makes, so this change does not widen it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 97b6090) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Brings the branch up to master 8f2ae56 so CI runs on current master. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ments Independent review verified the bit layout against the repo's own decoder (decoder.go isValidPathLen: hashCount = byte & 0x3F, hashSize = (byte>>6)+1) and decoded both new fixtures with signature validation on — they are genuine Ed25519-signed adverts, so real firmware does emit zero-hop directs with the size bits set. It also found two things worth fixing. Half the diff was untested. Reverting ONLY the computeAnalyticsHashSizes change survived the whole cmd/server suite, because the one analytics zero-hop test uses path byte 0x00 — skipped under both the old and the new rule, so it cannot tell them apart. TestAnalyticsHashSizesZeroHopDeclared- SizeCounts now pins both directions: a node whose only advert is a zero-hop direct declaring 3-byte mode (0x80) must report 3, while one with an all-zero path byte must stay unknown rather than collapsing to 1-byte. Re-running that exact mutation now fails. Two doc comments became false with this change: hashSizeMinObservations and hashSizeRecentAgreeCount both said "non-zero-hop adverts". Zero-hop directs that declare a size now count, and they are frequent, so a node clears its "varies" flag sooner than before. The comments say so. Reserved hash size 4 is NOT guarded here, deliberately. Skipping on byte content widens a pre-existing gap from 0xC1 to also 0xC0: this function has always reported hs=4, and TestHashSizeTransportRoutePathByteOffset pins that for 0xC1, while computeAnalyticsHashSizes drops it. Adding the guard made that existing test fail, and changing an assertion to let my own fix through is not something I will do — the inconsistency is recorded in a comment and left as separate work. Also left as follow-up: the per-packet views (server and ingestor decoders, packets.js, nodes.js) still use the old hop-count rule, so a node page can now say "Multibyte: 2-byte" while the advert that established it shows no size badge and its packet detail shows no Hash Size at all. Co-Authored-By: Claude Opus 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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out of #25 (commit
67b7033bthere). This branch holds exactly one upstream change so it can be reviewed, tested and reverted on its own.Upstream
97b6090344a09ca22d806279d5f515ff2109c510git cherry-pick -xonto masterfda24ca5; upstream authorship kept, and the commit message carries the(cherry picked from commit …)line.cmd/server/store.go,cmd/server/zerohop_hashsize_test.go,cmd/server/coverage_test.go.Problem
computeNodeHashSizeInfoandcomputeAnalyticsHashSizesskipped direct zero-hop adverts whenever the hop-count bits were zero (pathByte & 0x3F == 0). That also discarded deliberate size declarations (0x40/0x80) from firmware that writes the hash size with a zero hop count (meshcore-dev/MeshCore#3293), so those nodes' multibyte hash size was never learned from such adverts.Change
Skip only when the whole path byte is
0x00, the pre-#3293 case where the size bits were wiped and say nothing. Skipped packets still count and still updatelastSeen.Adaptation to this fork
None. The cherry-pick applied without conflicts and the changed lines are identical to upstream.
Notes for review
Protocol reference per AGENTS.md: the rule is taken from upstream's reading of MeshCore#3293 and was not re-derived from
firmware/in this split.Dependencies and merge order
fda24ca5and needs no other PR from this split.TestPruneOldNeighborMetricsdeterministic). If test(ingestor): make neighbor metrics pruning deterministic #33 lands first, the expected CI failure named below disappears; nothing in this PR depends on it.Verification
Local run of the same commands as CI's “Go Build & Test” job (server tests with
-race), on this branch and on masterfda24ca5under the same conditions (same machine, run one after another):fda24ca5go-server-build-vetgo-server-test-racechannel-lib-testdecrypt-cli-build-testdockerfile-copy-invariantsdeclare -A), macOS has 3.2; identical on masterstaging-disk-monitorcss-vars-lintBaseline failures (fail identically on master; not introduced or changed here): see rows marked baseline failure, unchanged.
Browser validation (local, fixture DB, no staging/production): Not applicable (no frontend change).
Not run:
eslint(not installed locally; CI installs it on the fly).Expected GitHub CI: “Go Build & Test” is expected to fail on
TestPruneOldNeighborMetrics, which already fails on master (see #25's run). Downstream jobs (Playwright, image build) are therefore skipped. “Deploy Staging” and all GHCR publish steps only run onpushtomasterand cannot run for this PR.Two further ingestor tests have failed intermittently in this split's CI on branches whose
cmd/ingestortree is byte-identical to master (#27, #28), so they can also appear here without being caused by this change:TestBackfillTxLastSeen_ResolvesFromMaxObservationTimestamp: also reproduced locally on unmodified master.TestMQTTStallWatchdog_DisconnectedEscalationThrottled_1749: the suite flake that upstream test(ingestor): join the watchdog loop goroutine instead of only asking it to stop Kpa-clawbot/CoreScope#2003 (also split out of port(upstream): 26 clean upstream fixes — prune batching, /ws limits, observer liveness, watchdog race #25) addresses.GitHub CI result: run 34749789928 on
57df2016. Go Build & Test: failure; all downstream jobs incl. Deploy Staging skipped. Failed tests:TestPruneOldNeighborMetrics: fails on master, documented baseline🤖 Generated with Claude Code