TMM extend to ephemeral 3rd tier store#11050
Open
NomDeTom wants to merge 50 commits into
Open
Conversation
NomDeTom
added
enhancement
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNodeDB, WarmNodeStore, Router, and TrafficManagementModule now share authoritative key lookup, signer provenance, cache write-through, purge, reconciliation, and NodeInfo response freshness controls. Tests cover cache and fallback behavior with deterministic timing. ChangesNodeInfo identity and traffic management
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Packet as Incoming packet
participant Router
participant NodeDB
participant TMM as TrafficManagementModule
Packet->>Router: request PKI decode or NodeInfo handling
Router->>NodeDB: resolve key or signer status
NodeDB->>TMM: write through committed identity or key
TMM->>TMM: validate cache provenance and freshness
TMM-->>Router: permit response, relay, or suppress cache use
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 (30)
Build artifacts expire on 2026-08-19. 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
NomDeTom
force-pushed
the
tmm-super-superset
branch
from
July 18, 2026 01:04
d4c557b to
7e2e7e0
Compare
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 18, 2026
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>
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
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/test_traffic_management/test_main.cpp (1)
1220-1248: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMove the shared cleanup into
tearDown()These tests reset
trafficManagementModuleand RTC state after the assertions. If aTEST_ASSERT_*aborts early, the global can stay pointed at a stackmoduleor the fake clock can leak into later cases. Reset that state intearDown()or with an RAII guard.🤖 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 `@test/test_traffic_management/test_main.cpp` around lines 1220 - 1248, Move the shared cleanup for trafficManagementModule and RTC/fake-clock state into the test fixture’s tearDown(), ensuring it runs even when TEST_ASSERT_* aborts early. Remove the duplicated per-test cleanup where covered, and preserve the existing reset behavior for subsequent tests.
🤖 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.
Outside diff comments:
In `@test/test_traffic_management/test_main.cpp`:
- Around line 1220-1248: Move the shared cleanup for trafficManagementModule and
RTC/fake-clock state into the test fixture’s tearDown(), ensuring it runs even
when TEST_ASSERT_* aborts early. Remove the duplicated per-test cleanup where
covered, and preserve the existing reset behavior for subsequent tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d6dd3de-4f69-4d3a-9b29-f1b5d5151916
📒 Files selected for processing (7)
docs/node_info_stores.mdsrc/mesh/NodeDB.cppsrc/mesh/NodeDB.hsrc/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 (5)
- src/mesh/WarmNodeStore.h
- docs/node_info_stores.md
- src/mesh/NodeDB.cpp
- src/modules/TrafficManagementModule.h
- src/modules/TrafficManagementModule.cpp
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 18, 2026
Several tests reset per-test global state (trafficManagementModule pointing at a stack `module`, owner.public_key, the RTC fake clock) only after their assertions. A TEST_ASSERT_* failure longjmps out and skips that trailing cleanup, leaving the state to dangle into later cases - concretely, the next setUp()'s resetNodes() would call trafficManagementModule->purgeAll() on a destroyed object. Move the resets into tearDown(), which runs unconditionally between tests, and drop the now-redundant per-test cleanups. Addresses a CodeRabbit review comment on PR meshtastic#11050. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> clod helped too
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/test_traffic_management/test_main.cpp (2)
1246-1246: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant comments restating test framework behavior.
As per coding guidelines, do not restate straightforward code. Explicitly stating that global state is reset in
tearDown()is redundant because that is the standard, well-known behavior of the Unity test framework's teardown phase.
test/test_traffic_management/test_main.cpp#L1246-L1246: Remove the comment.test/test_traffic_management/test_main.cpp#L1285-L1285: Remove the comment.test/test_traffic_management/test_main.cpp#L1980-L1980: Remove the comment.test/test_traffic_management/test_main.cpp#L2000-L2000: Remove the comment.🤖 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 `@test/test_traffic_management/test_main.cpp` at line 1246, Remove the redundant teardown/reset comments at test/test_traffic_management/test_main.cpp lines 1246, 1285, 1980, and 2000; no code changes are needed.Source: Coding guidelines
3138-3142: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep comments minimal.
As per coding guidelines, keep code comments minimal (one or two lines maximum) and do not add multi-paragraph explanatory blocks. Please shorten this explanation.
♻️ Proposed refactor
- // Runs even when a TEST_ASSERT_* aborts a test mid-way (Unity longjmps out, skipping any - // cleanup the test itself does after the assertion), so per-test global state can never - // dangle into later cases. The dangling-pointer path is concrete: a test points the global - // at its stack `module`, and the next setUp()'s resetNodes() would then call - // trafficManagementModule->purgeAll() on a destroyed object. + // Clean up per-test global state to prevent dangling pointers + // when Unity longjmps out on TEST_ASSERT failure.🤖 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 `@test/test_traffic_management/test_main.cpp` around lines 3138 - 3142, Shorten the explanatory comment at the test cleanup logic to one or two lines, retaining only that cleanup runs after aborted tests and prevents dangling per-test global state. Remove the detailed longjmp behavior and stack-object failure-path explanation.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.
Nitpick comments:
In `@test/test_traffic_management/test_main.cpp`:
- Line 1246: Remove the redundant teardown/reset comments at
test/test_traffic_management/test_main.cpp lines 1246, 1285, 1980, and 2000; no
code changes are needed.
- Around line 3138-3142: Shorten the explanatory comment at the test cleanup
logic to one or two lines, retaining only that cleanup runs after aborted tests
and prevents dangling per-test global state. Remove the detailed longjmp
behavior and stack-object failure-path explanation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9078be84-1ffc-4715-b2ea-53961b912a4c
📒 Files selected for processing (1)
test/test_traffic_management/test_main.cpp
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 18, 2026
Several tests reset per-test global state (trafficManagementModule pointing at a stack `module`, owner.public_key, the RTC fake clock) only after their assertions. A TEST_ASSERT_* failure longjmps out and skips that trailing cleanup, leaving the state to dangle into later cases - concretely, the next setUp()'s resetNodes() would call trafficManagementModule->purgeAll() on a destroyed object. Move the resets into tearDown(), which runs unconditionally between tests, and drop the now-redundant per-test cleanups. Addresses a CodeRabbit review comment on PR meshtastic#11050. clod helped too
NomDeTom
force-pushed
the
tmm-super-superset
branch
from
July 18, 2026 14:24
595fdbd to
6b8fb0f
Compare
NomDeTom
force-pushed
the
tmm-super-superset
branch
from
July 20, 2026 12:00
6b8fb0f to
5a0ca93
Compare
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 20, 2026
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>
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 20, 2026
Several tests reset per-test global state (trafficManagementModule pointing at a stack `module`, owner.public_key, the RTC fake clock) only after their assertions. A TEST_ASSERT_* failure longjmps out and skips that trailing cleanup, leaving the state to dangle into later cases - concretely, the next setUp()'s resetNodes() would call trafficManagementModule->purgeAll() on a destroyed object. Move the resets into tearDown(), which runs unconditionally between tests, and drop the now-redundant per-test cleanups. Addresses a CodeRabbit review comment on PR meshtastic#11050. clod helped too
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
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>
Several tests reset per-test global state (trafficManagementModule pointing at a stack `module`, owner.public_key, the RTC fake clock) only after their assertions. A TEST_ASSERT_* failure longjmps out and skips that trailing cleanup, leaving the state to dangle into later cases - concretely, the next setUp()'s resetNodes() would call trafficManagementModule->purgeAll() on a destroyed object. Move the resets into tearDown(), which runs unconditionally between tests, and drop the now-redundant per-test cleanups. Addresses a CodeRabbit review comment on PR meshtastic#11050. clod helped too
A direct response is addressed to the requesting packet's from field, which is unauthenticated, and is sent by every neighbour that holds the target in cache. One request therefore makes several nodes transmit at an address the requester chose, and nothing limited how often that could be repeated. Replies are now spaced per requester, which bounds how much any single node can be made to receive, plus a global floor on how much airtime the feature can consume. The check sits where a reply is about to be sent, so requests declined for other reasons do not consume the budget.
Makes the suite green for now. meshtastic#11104's per-requester + global-floor throttle (60 s) is layered over the branch's per-target throttle, so the three existing "served again" checks (psram, fallback, sweep) now use a fresh requester to avoid the per-requester window masking the mechanism each one actually exercises. Adds a dedicated test for the new per-requester + 1 s global-floor behaviour (the reflector-flood gap the per-target throttle leaves open, and the bound that survives on non-PSRAM builds). Deferred: the branch's now-redundant fallback global stamp (nodeInfoFallbackLastResponseMs) is left in place as a follow-up cleanup. Clod wants credit.
… tables
Replace the two path-specific NodeInfo-spoof throttles (per-entry respTick on
the PSRAM cache path, single global stamp on the fallback path) with three
symmetric bounds that behave identically with and without PSRAM:
- per requester (60 s): how much any one node can be made to receive;
- per target (60 s): how often we vouch for the same identity;
- global floor (1 s): total airtime, the backstop once an attacker cycles
requester/target past the 8-slot tables.
Both axes are fixed 8-slot LRU tables in internal RAM (not the PSRAM NodeInfo
cache), compared by wrap-safe uint32 ms subtraction, so there is no tick clock
and no sweep to maintain. directResponseAllowed() resolves both slots before
stamping either, so a reply one axis throttles never consumes the other's
budget, and records the send itself.
Retires: nodeInfoFallbackLastResponseMs, kNodeInfoResponseThrottleMs,
nowStampMs, the respTick byte + hasResponded bit on every cache entry,
currentRespTick/kNodeInfoRespTickMs/kNodeInfoThrottleTicks, and the sweep's
respTick clear. Renumbers peekNodeInfoFlagsForTest (drops the responded bit).
Tests: the psram/fallback throttle tests now assert the per-target axis at 60 s,
isolated via a different requester, proving it holds with and without PSRAM;
perRequesterAndGlobalFloor isolates the other two axes; the obsolete respTick
wrap-safety test is removed. 83/83 native cases pass.
clod helped too
Upstream develop carries 38 test_* suite directories but its native-suite-count file still reads 37 (a suite was added without bumping the count). Rebasing onto develop inherits that stale file, so the runner flags AMBER. Correct the registered total to 38. clod helped too
NomDeTom
force-pushed
the
tmm-super-superset
branch
from
July 21, 2026 08:33
22141a4 to
a95cc26
Compare
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
Security
Performance & Reliability
Documentation
Tests