TMM extend to ephemeral 3rd tier store#11050
Open
NomDeTom wants to merge 38 commits into
Open
Conversation
NomDeTom
added
enhancement
Contributor
📝 WalkthroughWalkthroughNodeInfo caching now coordinates with NodeDB and WarmNodeStore identity state. TMM adds signer-aware reconciliation, freshness and replay throttling, cache purges, authoritative key fallbacks, and expanded tests and documentation. ChangesTMM NodeInfo cache and signer-aware replay
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant NodeInfoPacket
participant TrafficManagementModule
participant NodeDB
participant Requester
NodeInfoPacket->>TrafficManagementModule: provide identity and signature state
TrafficManagementModule->>NodeDB: check freshness and signer provenance
TrafficManagementModule->>Requester: send eligible NodeInfo response
TrafficManagementModule->>TrafficManagementModule: stamp response throttle state
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (29)
Build artifacts expire on 2026-08-17. Updated for |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the TrafficManagementModule (TMM) NodeInfo cache into a more robust multi-tier identity/key store that stays reconciled with NodeDB (hot store + warm tier), adds purge/write-through hooks to prevent stale identity resurrection, and strengthens AdminModule session passkey rollover safety.
Changes:
- Added reconciliation + membership marking so the TMM NodeInfo cache remains a superset of NodeDB identities, with staleness/throttle gates and signer-proven provenance handling.
- Added write-through hooks and purge hooks so NodeDB updates/removals/factory reset propagate into TMM caches (and key learns that bypass
updateUser()are also reflected). - Improved AdminModule session passkey generation and expiry checks using a stronger RNG source and rollover-safe timing helpers; expanded unit tests for the new cache semantics and gates.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/test_traffic_management/test_main.cpp | Adds/extends unit tests for NodeInfo cache seeding, staleness/throttling, provenance, and purge behavior across cache/fallback paths. |
| src/modules/TrafficManagementModule.h | Introduces compile-time gates (TMM_HAS_NODEINFO_CACHE, signed-only replay gate), NodeInfo cache API (copyUser/copyPublicKey, hooks, purges), and tick-based timing constants. |
| src/modules/TrafficManagementModule.cpp | Implements NodeInfo cache allocation, reconcile/seed logic, key pinning/provenance, staleness + throttle gates, purge operations, and test hooks. |
| src/modules/KeyVerificationModule.cpp | Writes manually-verified public keys through to the TMM cache via onNodeKeyCommitted(). |
| src/modules/AdminModule.h | Makes session passkey timing millis-based and adds an explicit validity flag to avoid the “millis()==0” sentinel collision. |
| src/modules/AdminModule.cpp | Uses HardwareRNG (fallback CryptRNG) for session key generation and Throttle for rollover-safe lifetime checks. |
| src/mesh/WarmNodeStore.h | Adds isVerifiedSigner() and slot-indexed entryAt() to support reconciliation and signer provenance across tiers. |
| src/mesh/WarmNodeStore.cpp | Implements isVerifiedSigner(). |
| src/mesh/Router.cpp | Writes admin-channel key learns through to the TMM cache via onNodeKeyCommitted(). |
| src/mesh/NodeDB.h | Adds authoritative-only key copy and a key-matched signer-provenance helper (isVerifiedSignerForKey). |
| src/mesh/NodeDB.cpp | Hooks NodeDB identity/key lifecycle into TMM (write-through, purge-on-reset/removal) and uses TMM as a last-resort key source + warm-name rehydration support. |
| .notes/tmm-super-superset-reconciliation.md | Design/decision notes documenting the reconciliation of two prior approaches and key semantics choices. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/TrafficManagementModule.cpp`:
- Around line 318-350: Update purgeNode and purgeAll so NodeInfo cache clearing
is guarded independently by TMM_HAS_NODEINFO_CACHE, rather than being nested
under TRAFFIC_MANAGEMENT_CACHE_SIZE > 0. Preserve the unified-cache clearing
under its existing guard, while ensuring nodeInfoPayload entries are cleared
whenever the NodeInfo cache is enabled, including builds where the unified cache
is compiled out.
- Around line 1129-1138: Update the unauthenticated identity gate in the
NodeInfo handling around isNodeInfo and unauthenticatedSigner to derive the
sender’s authoritative key and classify verified signer provenance with
isVerifiedSignerForKey(), covering both hot and warm-tier entries. Ensure
unsigned NodeInfo from any verified signer is excluded before
cacheNodeInfoPacket(), role-cache refresh, or updateUser() writes. Add a
regression test using a warm signer=true entry and an unsigned forged NodeInfo,
verifying no forged identity or role data is cached or applied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 47c902d8-ec19-48cc-ac9b-6794e2f6a1ce
📒 Files selected for processing (12)
.notes/tmm-super-superset-reconciliation.mdsrc/mesh/NodeDB.cppsrc/mesh/NodeDB.hsrc/mesh/Router.cppsrc/mesh/WarmNodeStore.cppsrc/mesh/WarmNodeStore.hsrc/modules/AdminModule.cppsrc/modules/AdminModule.hsrc/modules/KeyVerificationModule.cppsrc/modules/TrafficManagementModule.cppsrc/modules/TrafficManagementModule.htest/test_traffic_management/test_main.cpp
NomDeTom
marked this pull request as ready for review
July 17, 2026 21:37
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/mesh/NodeDB.h (1)
363-370: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the added public API comments to one or two lines.
The declarations are functionally consistent, but the new documentation blocks exceed the repository’s minimal-comment convention.
src/mesh/NodeDB.h#L363-L370: retain a concise authoritative-key contract.src/mesh/NodeDB.h#L372-L377: retain a concise key-specific signer contract.src/mesh/NodeDB.h#L379-L385: retain a concise key-agnostic signer contract.src/mesh/WarmNodeStore.h#L122-L125: shorten the signer-bit contract.src/mesh/WarmNodeStore.h#L139-L142: shorten theentryAt()behavior contract.As per coding guidelines: “Keep comments minimal—one or two lines maximum.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mesh/NodeDB.h` around lines 363 - 370, Shorten the documentation comments for copyPublicKeyAuthoritative and the two signer-related NodeDB declarations in src/mesh/NodeDB.h (anchor 363-370; siblings 372-377 and 379-385) to one or two lines while preserving their key-specific contracts. Also shorten the signer-bit and entryAt() comments in src/mesh/WarmNodeStore.h (122-125 and 139-142) to one or two lines without removing their essential behavior contracts.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/node_info_stores.md`:
- Around line 20-22: Update the NodeInfoLite description in the “What” section
to describe its current flattened fields and satellite copy-out accessors,
removing references to nested identity members and telemetry pointers such as
node->position and node->device_metrics. Preserve the explanation that meshNodes
is the primary full-identity store and other data is cached or fallback.
- Around line 3-11: Update the overview and corresponding later section to
separate traffic state from identity-tier ordering: describe the TMM NodeInfo
payload cache as the third identity tier, and state that the TMM unified cache
is traffic-shaping state that sits beside the identity lookup chain rather than
within it. Keep the four-store overview intact while clarifying this distinction
around the relevant store descriptions.
---
Nitpick comments:
In `@src/mesh/NodeDB.h`:
- Around line 363-370: Shorten the documentation comments for
copyPublicKeyAuthoritative and the two signer-related NodeDB declarations in
src/mesh/NodeDB.h (anchor 363-370; siblings 372-377 and 379-385) to one or two
lines while preserving their key-specific contracts. Also shorten the signer-bit
and entryAt() comments in src/mesh/WarmNodeStore.h (122-125 and 139-142) to one
or two lines without removing their essential behavior contracts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 382e76f0-f752-433c-b805-9d06e393188d
📒 Files selected for processing (8)
docs/node_info_stores.mdsrc/mesh/NodeDB.cppsrc/mesh/NodeDB.hsrc/mesh/Router.cppsrc/mesh/WarmNodeStore.hsrc/modules/TrafficManagementModule.cppsrc/modules/TrafficManagementModule.htest/test_traffic_management/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- src/mesh/NodeDB.cpp
- src/modules/TrafficManagementModule.h
- test/test_traffic_management/test_main.cpp
- src/modules/TrafficManagementModule.cpp
The NodeInfo direct-response path answered queries on behalf of other nodes from an unauthenticated, never-expiring cache while suppressing the genuine request (STOP). That let a long-gone or forged identity be served as a fresh-looking, authoritative reply indefinitely. Three fixes: - Staleness gate: refuse to spoof a reply for a node not actually heard within ~6h (PSRAM cache via lastObservedMs; NodeDB fallback via sinceLastSeen, which tolerates last_heard==0 when the clock is unset). A stale entry now falls through so the real request propagates instead of being answered for a dead node. - Key hygiene: mirror NodeDB::updateUser() PKI checks when caching overheard NodeInfo - reject a packet advertising our own public key, and pin keys (drop a NodeInfo whose key mismatches an already-known key from NodeDB or from our own cache). Prevents cache poisoning / key substitution via the spoofed-reply path. - Response throttle: at most one spoofed reply per target node per 30s. Direct responses bypass the per-sender rate limiter (they STOP the request first) and the reply target is attacker-controlled, so this bounds airtime exhaustion and reflected floods. Recorded in a new NodeInfoPayloadEntry.lastResponseMs (PSRAM; self-expiring timestamp compare, no sweep needed). Tests: native NodeDB-fallback staleness (stale drops, fresh serves) plus PSRAM-guarded staleness, throttle, and key-mismatch cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G7QggdwMM1CJkguuCmMfZs
Comment-only. Tags each site flagged in the review of the direct-response throttle/staleness/key-hygiene change so the follow-ups are visible in-context: T1 throttle black-holes distinct requestors; T2 per-target key doesn't bound aggregate TX; T3 non-PSRAM fallback unthrottled; T4 millis-wrap defeats the staleness gate; T5 redundant second O(n) scan for the throttle stamp; T6 duplicated 32-byte key compares; T7 sinceLastSeen() called twice; T8 two 6h constants can desync; T9 clockMs()==0 collides with the 0 sentinel. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G7QggdwMM1CJkguuCmMfZs
T8: derive kNodeInfoMaxServeAgeSecs from kNodeInfoMaxServeAgeMs so the PSRAM
and NodeDB-fallback staleness windows cannot desync.
T6: collapse the three duplicated 32-byte key compares in cacheNodeInfoPacket
(owner-impersonation + NodeDB pin + cache TOFU pin) into one pubKeysEqual()
helper, so the compare lives in one place.
T7: compute sinceLastSeen(node) once on the fallback staleness path so the
tested age and the logged age cannot diverge (it calls getTime() each time).
Remaining review items still marked in-code: T1/T2/T3 (throttle design),
T4/T9 (millis-wrap + 0-sentinel), T5 (redundant throttle-stamp scan).
Native test_traffic_management suite: 48/48 passing.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G7QggdwMM1CJkguuCmMfZs
The per-target NodeInfo response throttle lived only in the PSRAM NodeInfoPayloadEntry, so non-PSRAM boards - which answer from the NodeDB fallback path - emitted spoofed direct replies with no rate limit at all, leaving the airtime/reflection surface the throttle exists to close. Add a single module-global fallback stamp (nodeInfoFallbackLastResponseMs, 4 bytes, guarded by cacheLock) that throttles the fallback path to one spoofed reply per window across all targets. Coarser than the PSRAM per-target throttle, but the fallback path has no per-node slot to stamp, and a global cap still bounds spoofed transmissions - which is the point. Add a native test exercising the fallback throttle window. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G7QggdwMM1CJkguuCmMfZs
T9: the millis-based fields that use 0 as a "never" sentinel (lastObservedMs, lastResponseMs, nodeInfoFallbackLastResponseMs) could be stamped with a literal 0 during the one-millisecond clockMs()==0 instant at each ~49.7-day wrap, momentarily colliding with the sentinel and disabling the staleness/throttle gate for that entry. Route all such stamps through a new nowStampMs() that maps 0 -> 1; the 1 ms skew is irrelevant to every window these fields feed. T4: the staleness gate's modular age compare is only unambiguous while true age < 2^32 ms (~49.7 days); an entry that lingered unrefreshed that long could wrap to a small age and read as fresh, defeating the gate. Add a NodeInfo eviction pass to the maintenance sweep that drops entries past the serve window, so an entry is removed long before its age can approach the wrap boundary. This also frees slots holding stale identities. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G7QggdwMM1CJkguuCmMfZs
The post-send throttle stamp did a second full O(n) scan of the PSRAM NodeInfo array under a fresh lock, even though findNodeInfoEntry() had already located the slot at the top of shouldRespondToNodeInfo(). Capture the slot index during that initial lookup and address the entry directly on the stamp path. Because the cache lock is released between the two accesses, the slot could have been evicted or reused, so re-validate node == p->to under the lock before stamping. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G7QggdwMM1CJkguuCmMfZs
Convert the T1/T2 TODO markers into a NOTE: the per-target PSRAM throttle keying and its lack of an aggregate bound are deliberate design choices, not pending work. A distinct requestor being throttled for one window is harmless on a redundant mesh, and keeping the PSRAM path per-target preserves throughput for legitimate multi-target responders (the fallback path already bounds aggregate via its global stamp). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G7QggdwMM1CJkguuCmMfZs
Add keySignerProven to NodeInfoPayloadEntry, set when an observed NODEINFO frame's XEdDSA signature was verified (mp.xeddsa_signed). This distinguishes a trust-on-first-use key from one proven to belong to a signer. The flag is monotonic - once proven it stays proven, and the existing key-pin checks forbid the underlying key from changing - so a later unsigned frame cannot downgrade it. A signature can only be verified against a key we already held, so a first-contact key is always TOFU until a later signed frame upgrades it. Groundwork for using the TMM cache as a last-resort public-key source, where this flag serves as a trust tier. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G7QggdwMM1CJkguuCmMfZs
Add TrafficManagementModule::copyPublicKey() and consult it from NodeDB::copyPublicKey() after the hot (NodeInfoLite) and warm (WarmNodeStore) tiers miss. This extends the pool of peers the node can encrypt to: a key the NodeInfo direct-response cache overheard for a node that has since aged out of both NodeDB tiers can still be used. The getter reports whether the key is signer-proven or trust-on-first-use. NodeDB serves TOFU keys here too - the same first-contact trust NodeDB already applies in updateUser() - so the pool actually expands to new long-tail nodes rather than only re-confirming known keys. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G7QggdwMM1CJkguuCmMfZs
Now that NodeDB::copyPublicKey() draws keys from the NodeInfo cache, the flat 6 h serve-window eviction would discard useful keys the moment a node stops being servable. Split retention: an entry carrying a 32-byte public key is kept for kNodeInfoKeyRetentionMs (7 d), while a keyless entry still expires at the serve window. Both windows stay well under the ~49.7-day millis wrap, preserving the T4 wrap-safety guarantee. Make LRU victim selection trust-tiered to match: a keyless slot is sacrificed before any keyed one, and a trust-on-first-use key before a signer-proven key; within a tier the oldest loses. Mirrors WarmNodeStore's keyed-first admission so the most valuable keys are the stickiest under memory pressure. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G7QggdwMM1CJkguuCmMfZs
PSRAM-gated tests (run on the ESP32 build, alongside the existing NodeInfo PSRAM tests): - copyPublicKey serves a TOFU key learned from an unsigned NodeInfo and reports signerProven=false - a later signature-verified NodeInfo upgrades provenance to signer-proven while the pinned key bytes stay unchanged - copyPublicKey reports a miss for an uncached node Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G7QggdwMM1CJkguuCmMfZs
When the TrafficManagement NodeInfo cache re-caches a node, mark its key signer-proven if NodeDB already knows the node as a verified signer for that same key - even if this particular (unicast/unsigned) frame carried no signature. Previously the flag only upgraded on a frame we verified ourselves, so a node we had already proven elsewhere looked TOFU here. Add NodeDB::isVerifiedSignerForKey(), which checks both tiers - the hot store's signed bitfield and the warm tier's cached signer bit - and requires the key to match so a rotated/mismatched key never inherits a stale verdict. Add WarmNodeStore::isVerifiedSigner() to expose the warm signer bit (the rebase onto develop added the bit itself; this surfaces it for lookups). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G7QggdwMM1CJkguuCmMfZs
keySignerProven cost zero bytes: sourceChannel, the two bools, and decodedBitfield are four 1-byte fields that fill a single 4-byte tail word (struct alignment is 4), so the flag consumed former padding rather than growing the 2000-entry PSRAM array. Add a static_assert pinning the entry to sizeof(meshtastic_User) + 20 so a future 5th trailing byte - which would open a fresh word (~8 KB PSRAM across the array) - fails the build instead of silently costing memory, prompting new flags to be packed into existing bytes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G7QggdwMM1CJkguuCmMfZs
Fold hasDecodedBitfield and keySignerProven into adjacent uint8_t:1 bitfields so they share a single byte, reserving 6 spare bits for future flags without growing the 2000-entry PSRAM array. sizeof is unchanged (still one packed tail word, sizeof(meshtastic_User) + 20); access is by name exactly as before, so no call sites change. Reorders decodedBitfield ahead of the flags so the two 1-bit members stay adjacent and the compiler packs them together. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G7QggdwMM1CJkguuCmMfZs
…lt on) Add TMM_NODEINFO_REPLAY_REQUIRE_SIGNED (default 1): the direct-response path now only spoofs a reply for a node whose key is signer-proven, in addition to the staleness gate - so replay is based on flagging AND staleness. Replay is a courtesy feature, and vouching for an unverified (trust-on-first-use) identity to other nodes is the risk this closes, so signed-only is the safer default. Define the macro to 0 at build time to also serve fresh TOFU-only nodes. Both paths are gated: the PSRAM path checks the cached keySignerProven flag, the NodeDB fallback checks nodeInfoLiteHasXeddsaSigned(node). The effective gate (TMM_NODEINFO_REPLAY_SIGNED_GATE) is bypassed when PKI is excluded from the build, since nothing can be signed there and the courtesy feature would otherwise be disabled outright. Tests: existing reply-expecting tests establish signer-proven state (a new markKeySignerProvenForTest hook for the PSRAM cache, the NodeDB signed bit for the fallback path); add fallback and PSRAM tests asserting an unsigned/TOFU node is withheld under the default gate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G7QggdwMM1CJkguuCmMfZs
The warm tier keeps an evicted node's key but not its name (the 40-byte record has no room), so a re-admitted long-tail node is nameless until its next NodeInfo broadcast. The TrafficManagement NodeInfo cache is much larger (2000 PSRAM entries) and commonly still holds the full User, so use it as a name reservoir the warm tier structurally cannot be. On re-admission (getOrCreateMeshNode), after the warm-tier restore, copy the cached identity from the TMM cache via a new copyUser() getter - but only when its cached key matches the key just restored from warm, so a name never attaches to a different identity than the one we encrypt to (key-matched trust). Guarded by HAS_TRAFFIC_MANAGEMENT and a null check; a no-op without the PSRAM cache or when no key is present. CopyUserToNodeInfoLite sets only user-related bits, so the warm-restored signer bit is preserved. Limitations (by design): PSRAM-only, and the TMM cache is RAM-only, so this helps within an uptime session, not across reboot. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G7QggdwMM1CJkguuCmMfZs
The static_assert pinned sizeof(NodeInfoPayloadEntry) to sizeof(meshtastic_User) + 20, but nanopb packs the generated User struct differently per platform: on pico its size is not a multiple of 4, so alignment padding before the uint32 timestamps makes the overhead 22, not 20, and the assert failed the build (native happened to be +20 and passed, hiding it). The bitfield packing it was guarding still stands; replace the fixed-count assert with a comment, since no portable byte count exists. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G7QggdwMM1CJkguuCmMfZs
The NodeInfo direct-response cache and everything layered on it - key pinning, signer provenance, staleness, throttle - was compiled only for ARCH_ESP32 + BOARD_HAS_PSRAM, so its entire test set was skipped by the native CI suite and ran nowhere but real hardware. Fold the scattered platform guards into one TMM_HAS_NODEINFO_CACHE macro that also enables the cache (plain heap, delete[] path already existed) for ARCH_PORTDUINO unit-test builds. Production portduino and embedded test builds are unchanged. Tests that exercise the NodeDB fallback path drop the cache explicitly via a new dropNodeInfoCacheForTest() hook, so both response paths are covered in one binary. Native suite: 60/60 (was 50 with the 10 cache tests compiled out). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KWHDDxY4U2XLYW2WJJ2oLT (cherry picked from commit b2dafbb)
cacheNodeInfoPacket() mirrored NodeDB::updateUser()'s key pin but checked only getMeshNode() - the hot store. updateUser's own pin effectively covers the warm tier too (getOrCreateMeshNode rehydrates the warm key before the check), so the mirror had a gap: for a node evicted to the warm tier whose cache slot was also gone, an attacker's NodeInfo with a bogus key passed the pin, and the cache's TOFU pin then locked the genuine node's frames out until the poisoned entry aged away. Split the authoritative lookup out of NodeDB::copyPublicKey() as copyPublicKeyAuthoritative() (hot store, then warm tier - never the opportunistic TMM tier, which would compare the cache against itself) and pin against that. Test: warm-tier-only key rejects a mismatching NodeInfo and accepts the matching one. Native suite: 61/61. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KWHDDxY4U2XLYW2WJJ2oLT (cherry picked from commit 28e8808)
Replace the entry's uint32 millis stamps (lastObservedMs, lastResponseMs) with three uint8 free-running tick clocks - the same modular scheme the UnifiedCache counters already use: obsTick 3 min/tick (12.8 h period) - replay staleness gate respTick 5 s/tick (21.3 min) - per-target response throttle retTick 1 h/tick (10.67 d) - retention TTL + LRU age Validity is an explicit flag bit (hasObserved / hasResponded), not a 0 sentinel, and the maintenance sweep saturates a stamp (clears its flag) once its window passes, so no stamp can age toward its ~256-tick aliasing horizon. That retires the wrap/sentinel special cases (T4, T9) - nowStampMs() survives only for the fallback path's module-global millis stamp. obsTick is separated from retention by design: only a genuinely heard NODEINFO frame stamps it, so later membership-based retention refresh can never make a silent node look servable. Also drops lastObservedRxTime, which was only echoed in a debug log. Entry shrinks 136 -> 128 B; the 2000-entry array 272 kB -> 256 kB, 8 kB below the original develop footprint while keeping the throttle and retention features. Tick granularity costs at most one tick per window (+-5 s on 30 s, +-3 min on 6 h, +-1 h on 7 d). Native suite: 61/61. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KWHDDxY4U2XLYW2WJJ2oLT (cherry picked from commit e48ed9c)
The cache learned identities only from overheard NODEINFO frames, so its membership was opportunistic: a NodeDB-tier node whose frame was never heard this boot had no entry (no key pin to protect it, no name to rehydrate), and the retention TTL evicted entries for nodes that still lived in the hot store or warm tier. Add an anti-entropy pass to the maintenance sweep (reconcileNodeInfoMembershipLocked): every hot-store / warm-tier node gets an entry - full identity from the hot store (hasFullUser), key-only from the warm tier - with signer provenance inherited key-matched, and NodeDB's key adopted wholesale on conflict. Member entries (isMember) are re-stamped each sweep, so they never age toward the retention TTL; when a node leaves both tiers the keep-alive stops and its entry ages out from its final member re-stamp. Membership also outranks key trust in LRU victim selection, and the reconcile pass skips seeding rather than churn one member out for another when hot+warm exceeds the cache. Two properties are load-bearing: - seeding/keep-alive never touch obsTick/hasObserved, so a seeded or retained entry is never served as a spoofed reply - only genuinely heard frames make a node servable; - copyUser() now requires hasFullUser, so a key-only warm seed can never stamp HAS_USER onto a nameless node via name-rehydration. Native suite: 64/64. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KWHDDxY4U2XLYW2WJJ2oLT (cherry picked from commit 4f1b30d)
updateUser() is the single chokepoint through which every remote identity/key write enters NodeDB (key-verification keys stay in CryptoEngine's pending buffer until a NodeInfo carries them here), so one hook at its tail lets the NodeInfo cache reflect a commit immediately instead of waiting up to a minute for the reconcile sweep - which stays in place as the anti-entropy backstop. onNodeIdentityCommitted() upserts the full User: NodeDB's key is authoritative (a conflicting cached key is stale residue - replaced, provenance dropped), a keyless commit keeps an already-cached TOFU key, and signer provenance transfers only for the committed key. The observation stamp is never touched: knowledge is not observation, so a hook write can never make a never-heard node servable - covered by the new test alongside immediate copyUser/copyPublicKey visibility. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KWHDDxY4U2XLYW2WJJ2oLT (cherry picked from commit e160b89)
NodeDB::removeNodeByNum() already forgets the node everywhere NodeDB owns - hot store, satellite stores, warm tier - but the TrafficManagement caches kept the deleted identity: the NodeInfo entry went on feeding the key pool (NodeDB::copyPublicKey) and name rehydration, and the unified slot kept its role/next-hop/dedup state, resurrecting the node on next contact. Add purgeNode(): clears both the unified cache slot and the NodeInfo cache entry, called from removeNodeByNum() alongside the warm-tier removal. Removal means full removal; passive eviction never calls this, and the reconcile sweep will not re-seed a node absent from both NodeDB tiers. Test: an observed identity plus a next-hop hint both vanish after removeNodeByNum(). Left for CI to execute per request. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KWHDDxY4U2XLYW2WJJ2oLT (cherry picked from commit b5564c1)
Two parallel implementations of the same superset design are being combined on this branch. This decision matrix records every divergence and which side each reconciled feature takes, ahead of the code commits that apply them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KWHDDxY4U2XLYW2WJJ2oLT
Adopt tmm-fix-superset's retention model (reconciliation decisions #3, #4, #9 in .notes/tmm-super-superset-reconciliation.md): - Entries are never evicted on a timer. The 7-day retention TTL and its third tick clock (retTick) are gone; wrap-safety rests entirely on the sweep's presence-bit saturation, and a quiet entry keeps its value (pubkey pool, name rehydration). Slots are reclaimed only by trust/membership-tiered LRU on insert - age scored by obsTick, with never-observed entries counting oldest - or by an explicit purge. - Membership refresh (entry -> NodeDB contains checks) runs every sweep; the heavy seeding pass runs at boot and then hourly, with the write-through hooks carrying immediacy in between. - Tick constants and helpers move into the header beside the existing UnifiedCache tick idiom, with static_asserts tying the tick windows to the fallback path's seconds/ms forms. Kept from tmm-fix-2 (decision #4): the warm tier is still seeded (key-only records via WarmNodeStore::entryAt) so the invariant remains cache superset-of hot AND warm, and the spareMembers guard still stops seeding from churning one member out for another when hot+warm exceeds the cache. Entry shrinks to 128 B (2000 entries = 256 kB). The TTL-based retention test is superseded by a no-timed-eviction test: a quiet keyed entry survives 9 days of sweeps while the serve gate saturates as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KWHDDxY4U2XLYW2WJJ2oLT
Adopt tmm-fix-superset's hook coverage (reconciliation decisions #5-#8): - onNodeKeyCommitted(): write-through for the two key-write sites that bypass NodeDB::updateUser - the Router's admin-key learn (TOFU-grade) and KeyVerificationModule's manual-verification commit (proven=true, the strongest provenance the cache can carry). Both were coverage gaps in the tmm-fix-2 write-through design. - updateUser's hook call now transfers signer status key-matched (isVerifiedSignerForKey) instead of the node's bare signed flag, and runs on acceptance rather than only on change. - purgeAll(): factoryReset() and resetNodes() clear both TMM tables - removal-is-full-removal applies to bulk resets too, without relying on the usual post-reset reboot. - purgeNode() reuses the existing finders instead of open-coded scans and logs the (user-initiated) purge. - peekNodeInfoFlagsForTest(): flag introspection so saturation and membership tests can assert sweep effects directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KWHDDxY4U2XLYW2WJJ2oLT
Port tmm-fix-superset's unique tests, adapted to run natively under TMM_HAS_NODEINFO_CACHE (reconciliation decision #11): - keyHook_upsertsAndGovernsProvenance: TOFU learn -> manual-verification upgrade -> NodeDB-senior rotation resets provenance. - tickSaturation_sweepClearsObserved: the sweep saturates hasObserved past the serve window, the entry persists (no TTL), and a full 256-tick clock wrap cannot alias a saturated stamp back to fresh. - sweepMembershipMarking: reconciliation seeds a hot identity as member+fullUser+unobserved; the next sweep clears membership once the node leaves NodeDB, while the entry itself persists. tmm-fix-2's tests (warm pin, hot/warm seeding, updateUser write-through, removal purge - now also asserting the entry is gone via the new peek hook - and the no-timed-eviction rewrite) were already on this branch. Suite now counts 70 test functions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KWHDDxY4U2XLYW2WJJ2oLT
Two tests needed updating for interactions the validation run surfaced: - The meshtastic#11035 test (ignoresUnsignedSignerIdentity) was written for a world without the native NodeInfo cache: it needs the NodeDB fallback path (dropNodeInfoCacheForTest) and the signed replay gate satisfied for its target, like the other fallback tests on this branch. - The hot-seed test's "observed frame makes it servable" step now sends a signature-verified frame: its node is a known signer, and per meshtastic#11035 an unsigned frame from a signer must not (and does not) drive cache writes - the gate working as designed. Full native suite: 70/70 under ASan. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KWHDDxY4U2XLYW2WJJ2oLT
runOnce() nested the entire NodeInfo maintenance block (tick-stamp saturation, membership refresh, boot/hourly reconcile) inside #if TRAFFIC_MANAGEMENT_CACHE_SIZE > 0, so a variant overriding the unified cache to 0 on a PSRAM board would compile the NodeInfo cache without its maintenance: hasObserved would never saturate, obsTick would alias past its 12.8 h wrap, and the 6 h staleness gate - whose wrap-safety explicitly depends on the sweep - would serve spoofed replies for long-gone nodes. Extract maintainNodeInfoCacheLocked(), guarded by TMM_HAS_NODEINFO_CACHE alone, and give runOnce() the same independent-guard structure purgeAll() already has. Also close the runtime cousin of the same mismatch: the write-through hooks (onNodeIdentityCommitted / onNodeKeyCommitted) now no-op while moduleConfig.has_traffic_management is off. Previously they kept filling the cache from NodeDB commits while runOnce() returned INT32_MAX, accumulating entries that were never swept or reconciled. Purges and reads stay ungated: removal must always work, and reads just miss an empty cache. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…suming them The direct-response throttle returned true, which made handleReceived() STOP the request: within the 30 s window a repeat request was neither answered nor forwarded toward the genuine target, and the call site still logged "respond" and bumped nodeinfo_cache_hits for a reply that was never sent. A requester whose first reply was lost on a noisy link got silence for the whole window. Return false instead: the request flows through normal relay handling, so the genuine node (or another cache-holder) can answer, while our own spoofed TX stays bounded. Repeats of the same packet id are already absorbed by the router's duplicate detection, and the stats counter now only counts replies actually sent. Also log purgeNode() only when a slot was actually cleared - it runs for every NodeDB removal, including nodes the caches never tracked. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fourteen scattered #if TMM_HAS_NODEINFO_CACHE guards (one per function, each with its own #else stub of (void) casts) collapse into a single guarded region holding every NodeInfo-cache-only function, terminated by one #else block of no-op stub definitions. Because the stubs now exist on every build, the call sites in runOnce(), purgeNode() and purgeAll() drop their guards too; inner guards remain only for orthogonal features (PKI, warm tier) and for real conditional work (constructor allocation, PSRAM paths in shouldRespondToNodeInfo). No behavior change. Compile-checked on both sides of the macro: the native app build (stubs) and the native unit-test build (region). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
perhapsDecode() resolved the sender's public key unconditionally for every encrypted packet, before even checking whether the packet could be PKI-decrypted (channel 0, unicast to us). Since copyPublicKey() grew its TrafficManagement fallback tier, a full hot+warm miss - the common case for channel traffic from senders outside both NodeDB tiers - additionally walked the 2000-entry NodeInfo cache under its lock, per packet, for a key that was then discarded. Move the resolution inside the PKI-candidate branch; remotePublic and haveRemoteKey had no consumers outside it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 60 s sweep re-derived isMember with a per-entry NodeDB lookup: O(entries x members) - about 700k node-number comparisons per minute on a full PSRAM cache (2000 entries x 250 hot + 100 warm), with strided PSRAM reads, all while holding cacheLock against the packet path. Move the refresh into the hourly reconcile pass, which is already O(members x entries): after the seeding loops (so the upsert pass still sees last pass's bits for spareMembers protection), clear every isMember bit and re-mark from both NodeDB tiers - including keyless warm records, which seed nothing but are still members. Accepted tradeoff, now documented on the field: membership lags a passive NodeDB eviction by up to an hour (the entry just stays LRU-sticky slightly longer). Additions stay immediate via the write-through hooks, explicit removals via purgeNode(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The two key-write sites that bypass updateUser() (admin-channel learn in Router::perhapsDecode, manual verification in KeyVerificationModule) each carried their own #if HAS_TRAFFIC_MANAGEMENT write-through boilerplate - the pattern this PR itself had to retrofit twice, and which any future direct key write would silently miss, leaving the TrafficManagement cache divergent until the next hourly reconcile. Add NodeDB::commitRemoteKey(n, key32, KeyCommitTrust): writes the key to the hot store and routes the TrafficManagement write-through in one place, with provenance explicit at the call site (AdminChannelProven maps to TOFU-grade, ManuallyVerified to proven). The bypass sites keep their reason for existing - bare-key commits with provenance that updateUser's User-payload/TOFU-pin path cannot express - but no longer know about TrafficManagement at all; both stale includes are dropped (Router's was already unused on develop). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
node_info_stores.md gains a "Tick clocks and wrap safety" section recording which mechanism keeps each modular tick clock honest: the unified-cache clocks pair the 60 s sweep with read-time window resets, the NodeInfo obs/resp clocks are sweep-only (hence the compile-time invariant that maintainNodeInfoCacheLocked() is guarded by TMM_HAS_NODEINFO_CACHE alone), and the warm tier is immune by design - absolute unix-seconds, no wrap until 2106. Also brought current with this series: membership refresh moved to the hourly reconcile, throttled direct-response requests forward instead of being consumed, the commitRemoteKey() bare-key funnel, and the module-disabled gate on the write-through hooks. CodeRabbit doc review (PR meshtastic#11050): present the NodeInfo payload cache as the third *identity* tier with the unified cache beside the chain, and describe NodeInfoLite's flattened fields / satellite copy-out accessors instead of the removed nested members. Two stale docs/tmm_node_stores.md references in the header now point at the real file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ership Throttle tests (PSRAM and NodeDB-fallback paths): a request inside the 30 s window now CONTINUEs into normal relay handling instead of being consumed - assert no spoofed TX, no NAK suppression, and (PSRAM path) that nodeinfo_cache_hits counts only replies actually sent. Membership test (renamed reconcileMembershipMarking): the per-minute sweep no longer refreshes isMember, so the test now pins down both halves of the new contract - the very next sweep after a passive NodeDB eviction still shows the stale member bit (the documented up-to-an-hour lag), and a reconcile interval's worth of sweeps clears it. Disabled-module test additionally proves the write-through hooks share the has_traffic_management gate: a key commit while disabled must not land in the NodeInfo cache. Not covered here: a build permutation with TRAFFIC_MANAGEMENT_CACHE_SIZE overridden to 0 (the configuration the maintenance-guard fix protects) would need its own PlatformIO env plus guards on every unified-cache test - left as a follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
NomDeTom
force-pushed
the
tmm-super-superset
branch
from
July 18, 2026 01:04
d4c557b to
7e2e7e0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/node_info_stores.md`:
- Around line 10-11: Update the TMM NodeInfo payload cache description in the
overview and the corresponding repeated text to qualify its storage location as
PSRAM-backed on supported targets and plain heap in native tests, replacing the
unconditional “in PSRAM” wording.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e0288475-ea02-452a-8bae-d9921abaafbe
📒 Files selected for processing (10)
docs/node_info_stores.mdsrc/mesh/NodeDB.cppsrc/mesh/NodeDB.hsrc/mesh/Router.cppsrc/mesh/WarmNodeStore.cppsrc/mesh/WarmNodeStore.hsrc/modules/KeyVerificationModule.cppsrc/modules/TrafficManagementModule.cppsrc/modules/TrafficManagementModule.htest/test_traffic_management/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- src/mesh/WarmNodeStore.h
- src/mesh/WarmNodeStore.cpp
- src/mesh/NodeDB.cpp
- src/modules/TrafficManagementModule.h
- test/test_traffic_management/test_main.cpp
- src/modules/TrafficManagementModule.cpp
Comment on lines
+10
to
+11
| 3. **TMM NodeInfo payload cache** (extended) - the ephemeral **third identity tier**: full | ||
| `User` payloads plus direct-response metadata, in PSRAM. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Qualify the extended cache’s storage location.
The overview says the cache is “in PSRAM,” but the availability section correctly notes that native unit-test builds use the plain heap. Use wording such as “PSRAM-backed on supported targets; plain heap in native tests” to keep the overview accurate.
Also applies to: 91-93
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/node_info_stores.md` around lines 10 - 11, Update the TMM NodeInfo
payload cache description in the overview and the corresponding repeated text to
qualify its storage location as PSRAM-backed on supported targets and plain heap
in native tests, replacing the unconditional “in PSRAM” wording.
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
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.
This pull request is a unified, robust cache and reconciliation logic, improved test coverage, and several security and maintainability enhancements. It also includes important fixes to the AdminModule session passkey logic, improving rollover safety.
The most important changes are:
TrafficManagement NodeInfo Cache & NodeDB Reconciliation
NodeDB.cpp) [1] [2] [3]updateUser) is immediately reflected in the TMM cache, ensuring consistency and up-to-date provenance. (NodeDB.cpp,Router.cpp) [1] [2]Key and Signer Provenance Handling
copyPublicKeyAuthoritativefor authoritative keys (hot/warm only), and enhancedcopyPublicKeyto opportunistically use the TMM cache as a last resort. AddedisVerifiedSignerForKeyto robustly track signer provenance across tiers. (NodeDB.cpp,NodeDB.h,WarmNodeStore.cpp,WarmNodeStore.h) [1] [2] [3] [4] [5] [6]Test Coverage and Hooks
🤝 Attestations
Summary by CodeRabbit