TMM & Warmstore key source naming#11119
Conversation
|
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:
📝 WalkthroughWalkthroughTrafficManagementModule now tracks XEdDSA-signed and manually verified key provenance separately. WarmNodeStore and NodeDB use explicit XEdDSA metadata, while documentation and tests reflect the revised cache, replay-gate, and migration behavior. ChangesNode-key provenance
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant NodeInfoPacket
participant TrafficManagementModule
participant NodeDB
NodeInfoPacket->>TrafficManagementModule: deliver key and signature state
TrafficManagementModule->>NodeDB: request signer or manual verification state
NodeDB-->>TrafficManagementModule: return provenance state
TrafficManagementModule->>TrafficManagementModule: evaluate keyProven()
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
NomDeTom
added
tech debt
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Unifies key provenance terminology across implementation and docs by replacing the ambiguous “signer” concept with explicit XEdDSA-signed vs manually-verified provenance, and updates warm-store persistence/migration notes accordingly.
Changes:
- Renames warm-store “signer” bit to “xeddsa-signed” and updates related APIs, tests, comments, and logs.
- Splits TMM NodeInfo cache provenance into
keyXeddsaSignedandkeyManuallyVerified, with a combinedkeyProven()predicate. - Updates documentation to clarify storage/persistence details (LittleFS, PSRAM/heap) and provenance gates.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_warm_store/test_main.cpp | Updates tests to use renamed warm-store XEdDSA-signed bit accessors/params. |
| test/test_traffic_management/test_main.cpp | Updates traffic-management tests and mock helpers for renamed provenance param naming. |
| src/modules/TrafficManagementModule.h | Splits provenance flags and adds keyProven() helper predicate. |
| src/modules/TrafficManagementModule.cpp | Updates eviction/gates and reconcile logic to use combined provenance and re-seed manual verification. |
| src/mesh/WarmNodeStore.h | Renames on-disk “signer” metadata bit to explicit XEdDSA-signed naming in API and helpers. |
| src/mesh/WarmNodeStore.cpp | Updates persistence/migration logic and logging for renamed warm-store bit. |
| src/mesh/NodeDB.h / src/mesh/NodeDB.cpp | Updates warm-store signer queries/carry-over to new hasXeddsaSigned naming and accessor. |
| docs/traffic_management_module.md / docs/node_info_stores.md | Updates docs to reflect new provenance model and persistence/storage clarifications. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/test_traffic_management/test_main.cpp (1)
110-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the helper comment within the two-line limit.
The updated comment occupies three lines. Please condense it while retaining the XEdDSA terminology. As per coding guidelines, comments must be minimal and limited to one or two lines.
🤖 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 110 - 112, Condense the helper comment above the full-identity seed setup to no more than two lines, preserving the XEdDSA terminology and the key details about the hot-store index and reconcile/seeding tests.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`:
- Around line 110-112: Condense the helper comment above the full-identity seed
setup to no more than two lines, preserving the XEdDSA terminology and the key
details about the hot-store index and reconcile/seeding tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d510b3d5-ed8e-4c6a-b009-10818ba11c9f
📒 Files selected for processing (4)
docs/node_info_stores.mdsrc/modules/TrafficManagementModule.cppsrc/modules/TrafficManagementModule.htest/test_traffic_management/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/modules/TrafficManagementModule.h
- docs/node_info_stores.md
- src/modules/TrafficManagementModule.cpp
⚡ 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-22. Updated for |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (2)
docs/node_info_stores.md:36
- This section still refers to the warm rehydration carrying a "signer bit" and the hot store as the source of "signer provenance". With the terminology update in this PR, these should be updated to "XEdDSA-signed bit" and more generally "key provenance" for consistency and to avoid mixing old/new terms.
the node's essentials are **absorbed into the warm tier** (see §2); on re-admission the
warm record is rehydrated back (`take()`), including the signer bit.
- **Persistence:** the node database file in LittleFS, saved on the usual NodeDB cadence.
- **Authority:** key pinning (`updateUser`'s "Public Key mismatch" drop), signer
provenance, and identity content all originate here. The lookup helpers that other
docs/node_info_stores.md:62
- This warm-tier bullet still says it restores "role/protected/signer bits", but the field being restored is specifically the XEdDSA-signed bit. Updating the wording improves clarity, especially now that "key provenance" can also include manual verification in other tiers.
- **Persistence:** nRF52840 uses a 12 KB raw-flash record-ring below LittleFS
(append/replay/compact); everywhere else `/prefs/warm.dat` (LittleFS).
- **Membership invariant:** a node lives in the hot **XOR** warm tier. `take()` removes
the warm record when the node is re-admitted hot, restoring role/protected/signer bits.
|
@caveman99 the copilot stuff is just nitpicks, right? |
It's a bit more - 4 of them are outdated comments, one is a semantic thing. functionality is not reflected in the name. It's not a bug, just makes maintenance harder. |
- Log line now says "not key-proven" (gate is XEdDSA OR manual, not just signer) - Rename markKeySignerProvenForTest -> markKeyXeddsaSignedForTest (sets only the XEdDSA bit) - Docs + test comments: "signer bit" -> "XEdDSA-signed bit" clod helped too
Address PR meshtastic#11119 review: the copyPublicKey()/copyUser() out-parameter and the cache-path replay gate now report entry->keyProven() (XEdDSA-signed OR manually verified), so the "signerProven" name and "signer-proven" comments were misleading. Rename the public out-param to keyProven, the local cachedKeySignerProven to cachedKeyProven, and update coupled callers, log strings, docs headings, and comments to say "key-proven". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
93acc79 to
1bad60f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mesh/Router.cpp (1)
818-822: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten this comment to the essential trust boundary.
The PKI provenance rationale is useful, but this five-line comment exceeds the project’s one-or-two-line comment guideline and includes implementation detail. Preserve the non-obvious security constraint concisely:
Proposed revision
- // Resolve the sender's key only for actual PKI-decrypt candidates, not every encrypted channel - // packet: copyPublicKeyForDecrypt() can fall through to a linear scan of TrafficManagement's large - // NodeInfo cache. It returns authoritative keys (hot/warm), or a cold-tier cache key only when it is - // key-proven - an unverified TOFU cache key must not back authenticated (pki_encrypted, p->from) - // DM attribution. + // Resolve the sender key only for PKI candidates; this lookup may scan the NodeInfo cache. + // Only authoritative or key-proven cache keys may back PKI sender attribution.🤖 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/Router.cpp` around lines 818 - 822, Shorten the comment immediately above the sender-key resolution logic to one or two lines, preserving only the trust boundary: resolve keys for actual PKI-decrypt candidates, and never use unverified TOFU cache keys for authenticated DM attribution. Remove the scan and cache-tier implementation details.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 `@src/mesh/Router.cpp`:
- Around line 818-822: Shorten the comment immediately above the sender-key
resolution logic to one or two lines, preserving only the trust boundary:
resolve keys for actual PKI-decrypt candidates, and never use unverified TOFU
cache keys for authenticated DM attribution. Remove the scan and cache-tier
implementation details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a3fbb4f-debf-41de-8f31-48c373486a8b
📒 Files selected for processing (11)
docs/node_info_stores.mddocs/traffic_management_module.mdsrc/mesh/NodeDB.cppsrc/mesh/NodeDB.hsrc/mesh/Router.cppsrc/mesh/WarmNodeStore.cppsrc/mesh/WarmNodeStore.hsrc/modules/TrafficManagementModule.cppsrc/modules/TrafficManagementModule.htest/test_traffic_management/test_main.cpptest/test_warm_store/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (9)
- docs/traffic_management_module.md
- src/mesh/WarmNodeStore.cpp
- src/mesh/NodeDB.h
- src/modules/TrafficManagementModule.h
- src/mesh/WarmNodeStore.h
- test/test_warm_store/test_main.cpp
- src/mesh/NodeDB.cpp
- src/modules/TrafficManagementModule.cpp
- test/test_traffic_management/test_main.cpp
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)
src/modules/TrafficManagementModule.cpp (1)
1502-1509: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale macro reference in comment.
The comment says "See TMM_NODEINFO_REPLAY_REQUIRE_SIGNED" but the actual guard on line 1502 is
TMM_NODEINFO_REPLAY_SIGNED_GATE. This looks like a leftover reference from before the gate was renamed. Per the PR's own thread, this is exactly the kind of outdated-comment issue that "make[s] maintenance harder" even though it isn't a functional bug.🧹 Suggested fix
- // usedFallback entries were already gated above. See TMM_NODEINFO_REPLAY_REQUIRE_SIGNED. + // usedFallback entries were already gated above. See TMM_NODEINFO_REPLAY_SIGNED_GATE.Based on the PR objectives note that Caveman99 confirmed several comments in this PR are outdated and "not bugs but make maintenance harder."
🤖 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/modules/TrafficManagementModule.cpp` around lines 1502 - 1509, Update the comment above the guard in the TMM_NODEINFO_REPLAY_SIGNED_GATE block to reference TMM_NODEINFO_REPLAY_SIGNED_GATE instead of the stale TMM_NODEINFO_REPLAY_REQUIRE_SIGNED macro name; leave the guard logic unchanged.
🤖 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 `@src/modules/TrafficManagementModule.cpp`:
- Around line 1502-1509: Update the comment above the guard in the
TMM_NODEINFO_REPLAY_SIGNED_GATE block to reference
TMM_NODEINFO_REPLAY_SIGNED_GATE instead of the stale
TMM_NODEINFO_REPLAY_REQUIRE_SIGNED macro name; leave the guard logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 46d783e7-fcad-454e-8102-32bacd933614
📒 Files selected for processing (4)
docs/node_info_stores.mdsrc/modules/TrafficManagementModule.cpptest/test_traffic_management/test_main.cpptest/test_warm_store/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- test/test_warm_store/test_main.cpp
94f19ce to
af82974
Compare
This pull request updates the documentation and implementation for node information storage, focusing on clarifying and unifying the handling of key provenance, especially the distinction between XEdDSA-signed and manually verified keys. It also standardizes terminology and field names across the codebase and documentation, and updates the persistence and storage descriptions for the various node info tiers.
Key provenance and terminology updates:
docs/node_info_stores.mdnow clearly distinguishes between XEdDSA-signed (keyXeddsaSigned) and manually verified (keyManuallyVerified) key provenance for nodes, replacing the previous generic "signer" terminology throughout. ThekeyProven()predicate now covers both cases, and relevant table entries and descriptions have been updated for clarity. [1] [2] [3] [4]Persistence and storage clarifications:
Traffic management and replay gate logic:
docs/traffic_management_module.mdis now described as requiring the key to be "proven" (either XEdDSA-signed or manually verified), matching the updated code logic and terminology.Other improvements:
These changes improve clarity, correctness, and maintainability of both the documentation and implementation for node info storage and provenance handling.
🤝 Attestations
Summary by CodeRabbit