From f0763aeccee699762836dca309255a6284091530 Mon Sep 17 00:00:00 2001 From: efiten Date: Thu, 25 Jun 2026 14:05:46 +0200 Subject: [PATCH] fix(#1726): clear stale "varies" hash size once a node settles (#1788) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #1726. ## Problem A MeshCore v1.16.0 repeater configured for 2-byte path hashes (`path.hash.mode=1`) — e.g. `36f6c7c7…` (`DK_3400_RAK_TEST`) — kept showing as **"varies"** / mixed 1-byte + 2-byte for the full 7-day advert window. Per the live data in the issue triage: of the node's ~20 recent adverts, exactly **one** (2026-06-09, across 15 distinct observer paths) was a genuine 1-byte flood advert; every other advert was 2-byte. The flip-flop heuristic in `computeNodeHashSizeInfo` weighs that stale advert equally with recent ones, so an operator who flips `path.hash.mode` mid-flight (or a single old 1-byte advert) stays flagged for the full window with no way to signal "the config is settled now." ## Fix Two coupled changes in `cmd/server/store.go` `computeNodeHashSizeInfo`: 1. **Chronological ordering.** `byPayloadType[4]` iterates in insertion order, not timestamp order, so `HashSize = Seq[last]` could pick the wrong advert under out-of-order MQTT ingest or chunked cold-load (the "carmack" concern from triage). We now collect `(FirstSeen, size)` pairs and **stable-sort by `FirstSeen`**; ties keep insertion order, preserving prior behavior when timestamps are equal. 2. **Recency decay.** After `transitions >= 2` raises the flip-flop flag, clear it when the most recent `hashSizeRecentAgreeCount` (= **3**) non-zero-hop adverts all agree on a single size. A node still flapping (recent adverts disagree) stays flagged. `3` mirrors the existing ≥3-observation threshold used to raise the flag. ## Policy note Triage marked this **needs-operator-input** because the decay is a behavior/policy change. This PR implements the rule the triage proposed ("if the last 3 adverts agree, clear inconsistent"), which matches the reporter's stated expectation. Happy to adjust the threshold or gate it differently per your call. ## Tests `cmd/server/issue1726_hash_decay_test.go`: - `TestIssue1726_SettledNodeNotInconsistent` — reporter's case (`[2,1,2,2,2]` within window) → `Inconsistent=false`, `HashSize=2`. - `TestIssue1726_HashSizeUsesChronologicallyLatest` — out-of-order insertion still reports the chronologically-latest size. - `TestIssue1726_ActiveFlapperStaysInconsistent` — a node whose recent adverts disagree stays flagged. Existing flip-flop / hash-collision tests unchanged and green; full `cmd/server` package suite passes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Erwin Fiten Co-authored-by: Claude Opus 4.8 (1M context) --- cmd/server/issue1726_hash_decay_test.go | 156 ++++++++++++++++++++++++ cmd/server/store.go | 93 +++++++++++--- 2 files changed, 233 insertions(+), 16 deletions(-) create mode 100644 cmd/server/issue1726_hash_decay_test.go diff --git a/cmd/server/issue1726_hash_decay_test.go b/cmd/server/issue1726_hash_decay_test.go new file mode 100644 index 00000000..f5003d1c --- /dev/null +++ b/cmd/server/issue1726_hash_decay_test.go @@ -0,0 +1,156 @@ +package main + +import ( + "testing" + "time" +) + +// Issue #1726: a MeshCore v1.16.0 repeater configured for 2-byte path hashes +// (path.hash.mode=1) was still shown as "varies" / mixed 1-byte+2-byte. The +// node had one genuine 1-byte flood advert mid-window, but every advert since +// has been 2-byte. The flip-flop heuristic weighed that stale 1-byte advert +// equally with recent ones, so the node stayed flagged for the full 7-day +// window even though its current config is settled. +// +// Expected: when the most recent non-zero-hop adverts all agree on a size, the +// node is "settled" and must NOT be flagged inconsistent. HashSize must reflect +// the chronologically-latest advert. + +// hashAdvertTx builds an ADVERT StoreTx (route FLOOD, non-zero hop) for a given +// pubkey, hash size and FirstSeen timestamp. +// +// hs=1 → path byte 0x01 (top 2 bits 00, hop bits non-zero) +// hs=2 → path byte 0x41 (top 2 bits 01) +func hashAdvertTx(pk string, hs int, firstSeen string) *StoreTx { + pt := 4 // ADVERT + pathByte := "01" + if hs == 2 { + pathByte = "41" + } + return &StoreTx{ + RawHex: "11" + pathByte + "aabb", // header 0x11 → routeType FLOOD + FirstSeen: firstSeen, + PayloadType: &pt, + DecodedJSON: `{"pubKey":"` + pk + `","name":"DK_3400_RAK_TEST","type":"ADVERT"}`, + } +} + +// ts formats a relative timestamp the way the ingestor writes first_seen +// (time.RFC3339, no fractional seconds). +func ts(d time.Duration) string { + return time.Now().UTC().Add(d).Format(time.RFC3339) +} + +func TestIssue1726_SettledNodeNotInconsistent(t *testing.T) { + ps := NewPacketStore(nil, nil) + pk := "36f6c7c73dfd265db996aeee25e1dc0cfe1ad4e5c1d6dd575325e3b241f4af78" + + // Chronological history within the 7-day window: a single 1-byte blip + // 5 days ago, then settled to 2-byte. Old heuristic: AllSizes={1,2}, + // 2 transitions → Inconsistent=true. The node is actually settled at 2-byte. + ps.byPayloadType[4] = []*StoreTx{ + hashAdvertTx(pk, 2, ts(-6*24*time.Hour)), + hashAdvertTx(pk, 1, ts(-5*24*time.Hour)), + hashAdvertTx(pk, 2, ts(-2*24*time.Hour)), + hashAdvertTx(pk, 2, ts(-1*24*time.Hour)), + hashAdvertTx(pk, 2, ts(-1*time.Hour)), + } + + info := ps.GetNodeHashSizeInfo() + ni := info[pk] + if ni == nil { + t.Fatalf("expected hash size info for %s", pk) + } + if ni.HashSize != 2 { + t.Errorf("HashSize = %d, want 2 (latest advert is 2-byte)", ni.HashSize) + } + if ni.Inconsistent { + t.Error("settled node (last 3 adverts all 2-byte) must NOT be flagged inconsistent") + } +} + +func TestIssue1726_HashSizeUsesChronologicallyLatest(t *testing.T) { + ps := NewPacketStore(nil, nil) + pk := "aaaa0000bbbb1111cccc2222dddd3333eeee4444ffff5555aaaa6666bbbb7777" + + // Inserted out of chronological order: the 2-byte advert is newest by + // FirstSeen but appended first. HashSize must follow time, not insertion. + ps.byPayloadType[4] = []*StoreTx{ + hashAdvertTx(pk, 2, ts(-1*time.Hour)), // newest + hashAdvertTx(pk, 1, ts(-5*24*time.Hour)), // oldest, appended last + } + + info := ps.GetNodeHashSizeInfo() + ni := info[pk] + if ni == nil { + t.Fatalf("expected hash size info for %s", pk) + } + if ni.HashSize != 2 { + t.Errorf("HashSize = %d, want 2 (chronologically-latest advert)", ni.HashSize) + } +} + +// Chronological ordering must be robust to FirstSeen format differences: +// the ingestor writes RFC3339 with no fractional seconds, but a fractional +// (".000Z") form must still order correctly relative to it. A naive string +// compare would sort the no-fraction "...05Z" after a same-second "...05.000Z" +// (because 'Z' > '.'), picking the wrong "latest" advert. +func TestIssue1726_OrderingRobustToTimestampFormat(t *testing.T) { + ps := NewPacketStore(nil, nil) + pk := "1234000056780000abcd0000ef120000345600007890000012340000567800ab" + + // Same wall-clock second, sub-second apart: the older advert in the + // no-fraction form, the newer 1ms later in the fractional form. A string + // compare sorts "...05Z" AFTER "...05.001Z" ('Z' > '.'), so it would treat + // the older 1-byte advert as latest; chronological parsing must not. + base := time.Now().UTC().Truncate(time.Second).Add(-3 * 24 * time.Hour) + older := base.Format(time.RFC3339) // "...05Z" + newer := base.Add(1 * time.Millisecond).Format("2006-01-02T15:04:05.000Z") // "...05.001Z" + + pt := 4 + mk := func(pathByte, firstSeen string) *StoreTx { + return &StoreTx{ + RawHex: "11" + pathByte + "aabb", + FirstSeen: firstSeen, + PayloadType: &pt, + DecodedJSON: `{"pubKey":"` + pk + `","type":"ADVERT"}`, + } + } + ps.byPayloadType[4] = []*StoreTx{ + mk("41", newer), // 2-byte, newest, appended first + mk("01", older), // 1-byte, oldest + } + + info := ps.GetNodeHashSizeInfo() + ni := info[pk] + if ni == nil { + t.Fatalf("expected hash size info for %s", pk) + } + if ni.HashSize != 2 { + t.Errorf("HashSize = %d, want 2 (newest advert, despite mixed timestamp formats)", ni.HashSize) + } +} + +// A node still genuinely flip-flopping (recent adverts disagree) must remain +// flagged — the decay only clears settled nodes, not active flappers. +func TestIssue1726_ActiveFlapperStaysInconsistent(t *testing.T) { + ps := NewPacketStore(nil, nil) + pk := "ffff111122223333444455556666777788889999aaaabbbbccccddddeeeeffff" + + ps.byPayloadType[4] = []*StoreTx{ + hashAdvertTx(pk, 2, ts(-4*24*time.Hour)), + hashAdvertTx(pk, 1, ts(-3*24*time.Hour)), + hashAdvertTx(pk, 2, ts(-2*24*time.Hour)), + hashAdvertTx(pk, 1, ts(-1*24*time.Hour)), + hashAdvertTx(pk, 2, ts(-1*time.Hour)), + } + + info := ps.GetNodeHashSizeInfo() + ni := info[pk] + if ni == nil { + t.Fatalf("expected hash size info for %s", pk) + } + if !ni.Inconsistent { + t.Error("node whose recent adverts disagree must stay flagged inconsistent") + } +} diff --git a/cmd/server/store.go b/cmd/server/store.go index a0f87e7f..c8b01fd4 100644 --- a/cmd/server/store.go +++ b/cmd/server/store.go @@ -8448,7 +8448,19 @@ func (s *PacketStore) computeNodeHashSizeInfo() map[string]*hashSizeNodeInfo { s.mu.RLock() defer s.mu.RUnlock() - info := make(map[string]*hashSizeNodeInfo) + // Collect (timestamp, hashSize) per pubkey so we can order adverts + // chronologically below. byPayloadType iteration is insertion order, which + // is not guaranteed to be chronological (out-of-order MQTT ingest, chunked + // cold-load), so we must sort by timestamp before reasoning about "latest" + // or "most recent" adverts. We parse FirstSeen rather than string-compare it + // so ordering is robust to timestamp-format differences (RFC3339 with or + // without fractional seconds); an unparseable/empty FirstSeen sorts oldest + // so it can never masquerade as the latest advert. + type hsEntry struct { + ts time.Time + size int + } + entries := make(map[string][]hsEntry) cutoff := time.Now().UTC().Add(-7 * 24 * time.Hour).Format("2006-01-02T15:04:05.000Z") @@ -8504,26 +8516,34 @@ func (s *PacketStore) computeNodeHashSizeInfo() map[string]*hashSizeNodeInfo { continue } - ni := info[pk] - if ni == nil { - ni = &hashSizeNodeInfo{AllSizes: make(map[int]bool)} - info[pk] = ni - } - ni.AllSizes[hs] = true - ni.Seq = append(ni.Seq, hs) + // time.Parse(time.RFC3339, ...) accepts both the ingestor's no-fraction + // form ("...05Z") and a fractional form ("...05.000Z"). Zero time on + // parse failure → sorts oldest. + ts, _ := time.Parse(time.RFC3339, tx.FirstSeen) + entries[pk] = append(entries[pk], hsEntry{ts: ts, size: hs}) } - // Post-process: use latest advert hash size and compute flip-flop flag. - // The most recent advert reflects the node's current hash size - // configuration. The upstream firmware bug causing stale path bytes in - // flood adverts was fixed (meshcore-dev/MeshCore#2154). - for _, ni := range info { + info := make(map[string]*hashSizeNodeInfo) + for pk, es := range entries { + // Order adverts chronologically. Stable sort so that adverts with equal + // (or unparseable) timestamps keep insertion order. + sort.SliceStable(es, func(i, j int) bool { return es[i].ts.Before(es[j].ts) }) + + ni := &hashSizeNodeInfo{AllSizes: make(map[int]bool), Seq: make([]int, len(es))} + for i, e := range es { + ni.Seq[i] = e.size + ni.AllSizes[e.size] = true + } + info[pk] = ni + // Use the most recent advert's hash size (last in chronological order). + // The upstream firmware bug causing stale path bytes in flood adverts + // was fixed (meshcore-dev/MeshCore#2154). ni.HashSize = ni.Seq[len(ni.Seq)-1] - // Flip-flop (inconsistent) flag: need >= 3 observations, + // Flip-flop (inconsistent) flag: need a minimum number of observations, // >= 2 unique sizes, and >= 2 transitions in the sequence. - if len(ni.Seq) < 3 || len(ni.AllSizes) < 2 { + if len(ni.Seq) < hashSizeMinObservations || len(ni.AllSizes) < 2 { continue } transitions := 0 @@ -8532,12 +8552,53 @@ func (s *PacketStore) computeNodeHashSizeInfo() map[string]*hashSizeNodeInfo { transitions++ } } - ni.Inconsistent = transitions >= 2 + if transitions < 2 { + continue + } + // Recency decay (issue #1726): if the most recent adverts all agree on + // a single size, the node has settled on its current hash mode (e.g. an + // operator flipped path.hash.mode mid-flight, or a lone stale 1-byte + // advert sits earlier in the 7-day window). Don't keep reporting "varies" + // over older history once the node is consistent again. A node whose + // recent adverts still disagree remains flagged. + // + // Known limitation: a node that flaps slowly (long stable stretches + // between toggles) is not flagged during a stable stretch. This is + // intentional — "varies" describes the node's *current* state — and the + // full history stays visible via hash_sizes_seen / AllSizes. + if recentAdvertsAgree(ni.Seq, hashSizeRecentAgreeCount) { + continue + } + ni.Inconsistent = true } return info } +// hashSizeMinObservations is the minimum number of non-zero-hop adverts in the +// window before a node is eligible to be flagged as flip-flopping at all. +const hashSizeMinObservations = 3 + +// hashSizeRecentAgreeCount is how many of the most recent non-zero-hop adverts +// must share a single hash size for a node to be considered "settled", clearing +// its flip-flop ("varies") flag. +const hashSizeRecentAgreeCount = 3 + +// recentAdvertsAgree reports whether the last n entries of a chronologically +// ordered hash-size sequence are all equal. +func recentAdvertsAgree(seq []int, n int) bool { + if len(seq) < n { + return false + } + last := seq[len(seq)-1] + for i := len(seq) - n; i < len(seq)-1; i++ { + if seq[i] != last { + return false + } + } + return true +} + // EnrichNodeWithHashSize populates hash_size, hash_size_inconsistent, and // hash_sizes_seen on a node map using precomputed hash size info. func EnrichNodeWithHashSize(node map[string]interface{}, info *hashSizeNodeInfo) {