Skip to content

fix: filter inconsistent hash sizes by role and add 7-day time window#567

Merged
KpaBap merged 3 commits into
masterfrom
fix/issue-566
Apr 4, 2026
Merged

fix: filter inconsistent hash sizes by role and add 7-day time window#567
KpaBap merged 3 commits into
masterfrom
fix/issue-566

Conversation

@Kpa-clawbot
Copy link
Copy Markdown
Owner

Summary

Fixes #566 — The "Inconsistent Hash Sizes" list on the Analytics page included all node types and had no time window, causing false positives.

Changes

1. Role filter on inconsistent nodes (cmd/server/store.go)

Added role filter to the inconsistentNodes loop in computeHashCollisions() so only repeaters and room servers are included. Companions are excluded since they were never affected by the firmware bug. This matches the existing role filter on collision bucketing from #441.

// Before:
if cn.HashSizeInconsistent {

// After:
if cn.HashSizeInconsistent && (cn.Role == "repeater" || cn.Role == "room_server") {

2. 7-day time window on hash size computation (cmd/server/store.go)

Added a 7-day recency cutoff to computeNodeHashSizeInfo(). Adverts older than 7 days are now skipped, preventing legitimate historical config changes (e.g., testing different byte sizes) from creating permanent false positives.

3. Frontend description text (public/analytics.js)

Updated the description to reflect the filtered scope: now says "Repeaters and room servers" instead of "Nodes", mentions the 7-day window, and notes that companions are excluded.

Tests

  • TestInconsistentNodesExcludesCompanions — verifies companions are excluded while repeaters and room servers are included
  • TestHashSizeInfoTimeWindow — verifies adverts older than 7 days are excluded from hash size computation
  • Updated existing hash size tests to use recent timestamps (compatible with the new time window)
  • All existing tests pass: cmd/server ✅, cmd/ingestor

Perf justification

The time window filter adds a single string comparison per advert in the scan loop — O(n) with a tiny constant. No impact on hot paths.

you added 2 commits April 4, 2026 16:04
…#566)

- Filter inconsistentNodes to only include repeaters and room servers,
  excluding companions which were never affected by the firmware bug
- Add 7-day recency window to computeNodeHashSizeInfo() so historical
  config changes during testing don't create permanent false positives
- Update frontend description text to reflect the filtered node types
- Add tests for role filtering and time window behavior

Fixes #566
…ogical assertions

The test packets used header 0x04 (routeType=DIRECT) with pathByte 0x40/0x80
(lower 6 bits = 0), which caused them to be silently skipped by the
direct zero-hop filter in computeNodeHashSizeInfo(). Tests passed for
wrong reasons (empty results matched vacuous assertions).

Fix: use header 0x11 (routeType=FLOOD) with pathByte 0x41/0x81 (non-zero
hop count) so packets are actually processed by the hash size computation.
Also fix packet ID collisions in TestInconsistentNodesExcludesCompanions
(all pubkeys were 64 chars, making len(n.pk) identical for all nodes).
@Kpa-clawbot
Copy link
Copy Markdown
Owner Author

🔍 Charlie Munger Review

"All I want to know is where I'm going to die, so I'll never go there." Let me tell you where this PR might die.

The Good (briefly)

The role filter and time window are sensible, narrowly scoped fixes. The string comparison on ISO 8601 timestamps works because the format is lexicographically sortable — that's fine. The PR description is clear and the perf justification is adequate.

Must-Fix

TestGetNodeHashSizeInfoLatestWins lost its reason for existing

The original test used incrementing timestamps ("2024-01-01T0" + strconv.Itoa(i)) to verify that when multiple adverts exist, the latest one determines the hash size. You changed all three timestamps to time.Now().UTC() — they're now identical. The test still passes, but it no longer tests ordering. It's testing "all-same-timestamp wins," which proves nothing.

This is the kind of thing that looks harmless today and causes a regression six months from now when someone changes the ordering logic and this test silently agrees.

Fix: Use recent but distinct timestamps. Something like:

baseTime := time.Now().UTC().Add(-1 * time.Hour)
// ...
FirstSeen: baseTime.Add(time.Duration(i) * time.Minute).Format("2006-01-02T15:04:05.000Z"),

Same issue applies to TestGetNodeHashSizeInfoIgnoreDirectZeroHop which also used incrementing timestamps for ordering and lost that property.

Approved with the above caveat

Everything else is solid. The filtering logic is correct, tests cover the new behavior, and the frontend copy is accurate. Fix the timestamp ordering in those two tests and this is ready to merge.

TestGetNodeHashSizeInfoLatestWins and TestGetNodeHashSizeInfoIgnoreDirectZeroHop
were using identical time.Now() calls for all packets, making them unable to
verify ordering semantics as their names claim. Use a base time with per-packet
minute offsets so the 'latest wins' logic is actually exercised.
@Kpa-clawbot
Copy link
Copy Markdown
Owner Author

✅ Review feedback addressed (commit 49c646a):

  1. Restored incrementing timestamps in TestGetNodeHashSizeInfoLatestWins — uses baseTime + i*minute offsets so the latest-wins ordering is actually exercised
  2. Restored incrementing timestamps in TestGetNodeHashSizeInfoIgnoreDirectZeroHop — same pattern, ensures temporal ordering matters
  3. All tests pass (cmd/server and cmd/ingestor)

@Kpa-clawbot
Copy link
Copy Markdown
Owner Author

🐧 Torvalds Final Review

Clean. Small. Does what it says. No over-engineering.

What I checked

  1. Prior review fix verified. Munger caught that TestGetNodeHashSizeInfoLatestWins and TestGetNodeHashSizeInfoIgnoreDirectZeroHop lost their temporal ordering when timestamps were updated. The fix landed — both now use baseTime + i*minute offsets. The ordering property is preserved. ✅

  2. Role filter (store.go:4659). Two-condition filter on computeHashCollisions. Simple, correct, matches the existing role filter on collision bucketing. No abstraction needed for two string comparisons. ✅

  3. Time window (store.go:4876-4882). String comparison on ISO 8601 timestamps — lexicographically sortable, works correctly. Cutoff computed once before the loop. O(n) with negligible constant. ✅

  4. New tests. TestInconsistentNodesExcludesCompanions — creates all three roles with flip-flop hash sizes, asserts companion excluded, repeater and room_server included. TestHashSizeInfoTimeWindow — old adverts ignored, recent ones counted. Both test the actual behavior, not copies. Cache invalidation between calls is correct. ✅

  5. Existing test updates. All hardcoded 2024 timestamps replaced with time.Now() variants to stay within the 7-day window. No test logic was lost. ✅

  6. Frontend copy. Accurate — says "repeaters and room servers", mentions 7-day window, notes companion exclusion. No code changes, just text. ✅

Verdict

Zero must-fix items. This is a textbook narrow fix — identifies the problem, solves it with minimal code, tests the fix, doesn't touch anything else. Ship it.

APPROVED

@KpaBap KpaBap merged commit aac038a into master Apr 4, 2026
5 checks passed
@KpaBap KpaBap deleted the fix/issue-566 branch April 4, 2026 16:22
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: companion nodes included in Inconsistent Hash Size list

2 participants