From 97b6090344a09ca22d806279d5f515ff2109c510 Mon Sep 17 00:00:00 2001 From: TeTeHacko Date: Wed, 2 Sep 2026 14:21:42 +0200 Subject: [PATCH] fix(hash-size): key the zero-hop advert skip on the path byte, not the route type (#1913) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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](https://github.com/meshcore-dev/MeshCore/pull/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 #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. #653 landed the skip as `pathByte & 0x3F == 0`, which swallows the second case too. The diagnosis in #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 #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 #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. #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 #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) --- cmd/server/coverage_test.go | 15 ++- cmd/server/store.go | 27 ++-- cmd/server/zerohop_hashsize_test.go | 187 ++++++++++++++++++++++++++++ 3 files changed, 214 insertions(+), 15 deletions(-) create mode 100644 cmd/server/zerohop_hashsize_test.go diff --git a/cmd/server/coverage_test.go b/cmd/server/coverage_test.go index b6c3da77..cfb28c5d 100644 --- a/cmd/server/coverage_test.go +++ b/cmd/server/coverage_test.go @@ -3378,17 +3378,20 @@ func TestHashSizeTransportDirectZeroHopSkipped(t *testing.T) { db.conn.Exec(`INSERT INTO observers (id, name, iata, last_seen, first_seen, packet_count) VALUES ('obs1', 'Obs', 'SJC', ?, '2026-01-01T00:00:00Z', 10)`, recent) - // RouteDirect (2) zero-hop: path byte 0x40 → hop_count=0, hash_size bits=01 - // Should be skipped (existing behavior) + // RouteDirect (2) zero-hop with an all-zero path byte: the sender wiped the + // size bits, so there is nothing to read. Should be skipped. + // (This fixture used to be 0x40. A zero hop count with the size bits SET is + // a deliberate declaration — see TestZeroHopDirectAdvertDeclaredSizeIsEvidence + // — so the skip is keyed on the byte's content, not on the route type.) db.conn.Exec(`INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type, decoded_json) - VALUES ('1240aabbccdd', 'direct_zh', ?, 2, 4, '{"pubKey":"bbbb000000000001","name":"Direct-ZH","type":"ADVERT"}')`, recent) + VALUES ('1200aabbccdd', 'direct_zh', ?, 2, 4, '{"pubKey":"bbbb000000000001","name":"Direct-ZH","type":"ADVERT"}')`, recent) db.conn.Exec(`INSERT INTO observations (transmission_id, observer_idx, snr, rssi, path_json, timestamp) VALUES (1, 1, 10.0, -90, '[]', ?)`, recentEpoch) - // RouteTransportDirect (3) zero-hop: 4 transport bytes + path byte 0x40 → hop_count=0 - // Should ALSO be skipped (this was the missing case) + // RouteTransportDirect (3) zero-hop: 4 transport bytes + all-zero path byte + // Should ALSO be skipped (this was the missing case in #747) db.conn.Exec(`INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type, decoded_json) - VALUES ('130102030440aabb', 'tdirect_zh', ?, 3, 4, '{"pubKey":"bbbb000000000002","name":"TDirect-ZH","type":"ADVERT"}')`, recent) + VALUES ('130102030400aabb', 'tdirect_zh', ?, 3, 4, '{"pubKey":"bbbb000000000002","name":"TDirect-ZH","type":"ADVERT"}')`, recent) db.conn.Exec(`INSERT INTO observations (transmission_id, observer_idx, snr, rssi, path_json, timestamp) VALUES (2, 1, 10.0, -90, '[]', ?)`, recentEpoch) diff --git a/cmd/server/store.go b/cmd/server/store.go index 245fadbc..8ca5bc60 100644 --- a/cmd/server/store.go +++ b/cmd/server/store.go @@ -8063,14 +8063,17 @@ func (s *PacketStore) computeAnalyticsHashSizes(region, area string) map[string] name = pk } } - // Skip zero-hop direct adverts for hash_size — the - // path byte is locally generated and unreliable. - // Still count the packet and update lastSeen. - isZeroHop := (routeType == uint64(RouteDirect) || routeType == uint64(RouteTransportDirect)) && (actualPathByte&0x3F) == 0 + // Skip zero-hop direct adverts whose path byte is entirely zero — + // there the size bits were wiped by the sender and say nothing. + // A non-zero byte with a zero hop count (0x40 / 0x80) is a + // deliberate size declaration; keep it. Same rule as + // computeNodeHashSizeInfo. Skipped packets still count and still + // update lastSeen. + isUndeclaredZeroHop := (routeType == uint64(RouteDirect) || routeType == uint64(RouteTransportDirect)) && actualPathByte == 0x00 if byNode[pk] == nil { role := nodeRoleByPK[pk] // empty if unknown initHS := hashSize - if isZeroHop { + if isUndeclaredZeroHop { initHS = 0 } byNode[pk] = map[string]interface{}{ @@ -8080,7 +8083,7 @@ func (s *PacketStore) computeAnalyticsHashSizes(region, area string) map[string] } } byNode[pk]["packets"] = byNode[pk]["packets"].(int) + 1 - if !isZeroHop { + if !isUndeclaredZeroHop { byNode[pk]["hashSize"] = hashSize } byNode[pk]["lastSeen"] = tx.FirstSeen @@ -8677,9 +8680,15 @@ func (s *PacketStore) computeNodeHashSizeInfo() map[string]*hashSizeNodeInfo { if err != nil { continue } - // Direct zero-hop adverts (route types 2 and 3) use path byte 0x00 - // locally and can misreport multibyte hash mode as 1-byte. - if (routeType == RouteDirect || routeType == RouteTransportDirect) && (pathByte&0x3F) == 0 { + // Direct zero-hop adverts carry no path, so the hop count is 0. Whether + // the SIZE bits are meaningful depends on the sender: firmware that + // predates meshcore-dev/MeshCore#3293 does `path_len = 0`, wiping the + // whole byte including the two size bits, so 0x00 says nothing about + // the node's path.hash.mode. A sender that writes the size through + // setPathHashSizeAndCount() emits 0x40 / 0x80 with a zero hop count — + // that is a deliberate declaration and the only way those bits can be + // non-zero here. Skip on the byte's content, not on the route type. + if (routeType == RouteDirect || routeType == RouteTransportDirect) && pathByte == 0x00 { continue } hs := int((pathByte>>6)&0x3) + 1 diff --git a/cmd/server/zerohop_hashsize_test.go b/cmd/server/zerohop_hashsize_test.go new file mode 100644 index 00000000..6509989e --- /dev/null +++ b/cmd/server/zerohop_hashsize_test.go @@ -0,0 +1,187 @@ +package main + +import ( + "fmt" + "testing" + "time" +) + +// Zero-hop direct adverts carry no path, so the hop count is always 0. Whether +// the two SIZE bits next to it mean anything depends on the sender: +// +// - Firmware predating meshcore-dev/MeshCore#3293 does `path_len = 0`, which +// wipes the whole byte. 0x00 therefore says nothing about path.hash.mode +// and must not be read as "1 byte". +// - A sender that writes the size through setPathHashSizeAndCount() emits +// 0x40 (2 bytes) or 0x80 (3 bytes) with a zero hop count. Nothing else can +// produce those bits here, so they are a deliberate declaration. +// +// Before this fix the skip was keyed on the route type and swallowed both +// cases. These tests pin the distinction. The two multi-byte fixtures are real +// packets off the Czech mesh (869.4 MHz), captured 2026-08-24/25. + +// insertAdvert adds one ADVERT transmission plus a matching observation. +func insertAdvert(t *testing.T, db *DB, id int, rawHex, hash, pubKey, name string, routeType int, seen string, seenEpoch int64) { + t.Helper() + if _, err := db.conn.Exec( + `INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type, decoded_json) + VALUES (?, ?, ?, ?, 4, ?)`, + rawHex, hash, seen, routeType, + fmt.Sprintf(`{"pubKey":"%s","name":"%s","type":"ADVERT"}`, pubKey, name), + ); err != nil { + t.Fatalf("insert transmission %s: %v", hash, err) + } + if _, err := db.conn.Exec( + `INSERT INTO observations (transmission_id, observer_idx, snr, rssi, path_json, timestamp) + VALUES (?, 1, 10.0, -90, '[]', ?)`, id, seenEpoch, + ); err != nil { + t.Fatalf("insert observation %s: %v", hash, err) + } +} + +func zeroHopStore(t *testing.T) (*DB, string, int64) { + t.Helper() + db := setupTestDB(t) + now := time.Now().UTC().Add(-1 * time.Hour) + recent := now.Format(time.RFC3339) + db.conn.Exec(`INSERT INTO observers (id, name, iata, last_seen, first_seen, packet_count) + VALUES ('obs1', 'Obs', 'PRG', ?, '2026-01-01T00:00:00Z', 10)`, recent) + return db, recent, now.Unix() +} + +// TestZeroHopDirectAdvertDeclaredSizeIsEvidence: a zero-hop DIRECT advert whose +// path byte is 0x40 declares a 2-byte path hash and must be counted. +// Fixture: tth-hrebecna, 2026-08-25T09:29:40Z, header 0x12 (route 2, ADVERT). +func TestZeroHopDirectAdvertDeclaredSizeIsEvidence(t *testing.T) { + db, recent, epoch := zeroHopStore(t) + defer db.Close() + + const hrebecnaPK = "5361aa08929b6248bb61cb1fab0fdb97d6dba8ae44f53a44427f63e7798a6b6b" + insertAdvert(t, db, 1, + "1240"+hrebecnaPK+"83608d6a3d38eeb187a94a745daf2ffb3191d1ae49fb82f30a083ba081ffbb20"+ + "266331879c931267408acd04bcb8c8aec558d106ab40e942c70ce8e214af68529faa580b9243bd000322a5c300"+ + "7474682d6872656265636e61", + "zh_direct_2b", hrebecnaPK, "tth-hrebecna", 2, recent, epoch) + + store := NewPacketStore(db, nil) + store.Load() + info := store.GetNodeHashSizeInfo() + + ni, ok := info[hrebecnaPK] + if !ok { + t.Fatal("node with a 0x40 zero-hop advert missing from hash size info — the declared size was dropped") + } + if ni.HashSize != 2 { + t.Errorf("want HashSize=2 from path byte 0x40, got %d", ni.HashSize) + } + if ni.Inconsistent { + t.Error("single advert must not be flagged inconsistent") + } +} + +// TestZeroHopDirectAdvertThreeByteDeclaration: same rule at 3 bytes (0x80). +// Fixture: TTH-L1, 2026-08-24T15:36:45Z. +func TestZeroHopDirectAdvertThreeByteDeclaration(t *testing.T) { + db, recent, epoch := zeroHopStore(t) + defer db.Close() + + const pk = "e11e55684170982339efe2266a423c9040f909339646ee2adeea19c21ed2b02a" + insertAdvert(t, db, 1, + "1280"+pk+"0c658c6afcf586f0935c87b563dc0770f1ccb558126d16e4805eb4c556d511bf"+ + "c1f5d832a760bbe2f08d73b62c068c1a269425faadb2f575a69bcfc53ebae0cb65b71d0981"+ + "5454482d4c31", + "zh_direct_3b", pk, "TTH-L1", 2, recent, epoch) + + store := NewPacketStore(db, nil) + store.Load() + info := store.GetNodeHashSizeInfo() + + ni, ok := info[pk] + if !ok { + t.Fatal("node with a 0x80 zero-hop advert missing from hash size info") + } + if ni.HashSize != 3 { + t.Errorf("want HashSize=3 from path byte 0x80, got %d", ni.HashSize) + } +} + +// TestZeroHopDirectAdvertAllZeroByteStillSkipped: 0x00 carries no information — +// the sender wiped the size bits — so the node must stay absent rather than be +// recorded as 1 byte. This is the behaviour #649 asked for and it must survive. +func TestZeroHopDirectAdvertAllZeroByteStillSkipped(t *testing.T) { + db, recent, epoch := zeroHopStore(t) + defer db.Close() + + const pk = "aaaa000000000000000000000000000000000000000000000000000000000001" + insertAdvert(t, db, 1, "1200"+pk+"cafe", "zh_direct_zero", pk, "Legacy-Node", 2, recent, epoch) + + store := NewPacketStore(db, nil) + store.Load() + + if ni, ok := store.GetNodeHashSizeInfo()[pk]; ok { + t.Errorf("node seen only via a 0x00 zero-hop advert must have no hash size evidence, got %d", ni.HashSize) + } +} + +// TestZeroHopTransportDirectDeclaredSize: route type 3 puts the path byte at +// offset 5, behind four transport code bytes. The same content rule applies. +func TestZeroHopTransportDirectDeclaredSize(t *testing.T) { + db, recent, epoch := zeroHopStore(t) + defer db.Close() + + const declared = "aaaa000000000000000000000000000000000000000000000000000000000002" + const wiped = "aaaa000000000000000000000000000000000000000000000000000000000003" + + // header 0x13 = payload_type 4 (ADVERT), route type 3; 4 transport bytes; path byte 0x40. + insertAdvert(t, db, 1, "1301020304"+"40"+declared+"cafe", "zh_td_2b", declared, "TD-Declared", 3, recent, epoch) + // Same shape, path byte 0x00 → no evidence. + insertAdvert(t, db, 2, "1305060708"+"00"+wiped+"cafe", "zh_td_zero", wiped, "TD-Wiped", 3, recent, epoch) + + store := NewPacketStore(db, nil) + store.Load() + info := store.GetNodeHashSizeInfo() + + if ni, ok := info[declared]; !ok { + t.Error("transport-direct node with 0x40 at offset 5 missing from hash size info") + } else if ni.HashSize != 2 { + t.Errorf("transport-direct: want HashSize=2, got %d", ni.HashSize) + } + if _, ok := info[wiped]; ok { + t.Error("transport-direct node with an all-zero path byte must have no evidence") + } +} + +// TestZeroHopDeclaredSizeReachesMultiByteCapability: the capability view derives +// "confirmed" from advert evidence, so a node heard only via zero-hop adverts +// should now be confirmed rather than left unknown. This is the user-visible +// half — it is what the map's multi-byte overlay reads. +func TestZeroHopDeclaredSizeReachesMultiByteCapability(t *testing.T) { + db, recent, epoch := zeroHopStore(t) + defer db.Close() + + const pk = "5361aa08929b6248bb61cb1fab0fdb97d6dba8ae44f53a44427f63e7798a6b6b" + db.conn.Exec(`INSERT INTO nodes (public_key, name, role, last_seen, first_seen, advert_count) + VALUES (?, 'tth-hrebecna', 'repeater', ?, ?, 1)`, pk, recent, recent) + insertAdvert(t, db, 1, "1240"+pk+"cafe", "zh_cap", pk, "tth-hrebecna", 2, recent, epoch) + + store := NewPacketStore(db, nil) + store.Load() + + var found *MultiByteCapEntry + for _, e := range store.computeMultiByteCapability(nil) { + if e.PublicKey == pk { + entry := e + found = &entry + break + } + } + if found == nil { + t.Fatal("node missing from multi-byte capability output") + } + if found.Status != "confirmed" { + t.Errorf("want status=confirmed from a declared zero-hop advert, got %q (evidence %q)", found.Status, found.Evidence) + } + if found.MaxHashSize != 2 { + t.Errorf("want MaxHashSize=2, got %d", found.MaxHashSize) + } +}