WarmNodeStore: carry the XEdDSA signer flag through the warm tier#11020
Conversation
The signer flag is learned from verified traffic rather than from NodeInfo, so a node evicted to the warm tier and re-admitted came back with it clear until the next signed packet arrived. Cache it alongside the role and protected category. last_heard now packs 7 metadata bits (role 4, protected 2, signer 1). Bit 6 was part of the timestamp in the v2 format, so the record magic moves to WRN3/WRM3 and load() clears that bit on v2 records rather than reading it as a signer. v1 handling is unchanged. Entry size stays 40 B.
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughWarm-node persistence now stores the XEDDSA signer bit, supports v3 warm storage metadata, migrates v1/v2 records, restores the bit during NodeDB re-admission, and adds round-trip and migration tests. ChangesWarm signer-bit persistence
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant NodeDB
participant WarmNodeStore
participant WarmStorage
NodeDB->>WarmNodeStore: absorb node metadata and signer bit
WarmNodeStore->>WarmStorage: persist v3 warm record
WarmStorage-->>WarmNodeStore: load and migrate v1/v2 records
WarmNodeStore-->>NodeDB: return warm entry and signer bit
NodeDB->>NodeDB: restore XEDDSA signer status
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/mesh/WarmNodeStore.h (1)
46-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCondense the new explanatory comments to at most two lines.
src/mesh/WarmNodeStore.h#L46-L53: summarize the metadata layout and timestamp quantization.src/mesh/WarmNodeStore.cpp#L19-L23: reduce the legacy-format explanation.src/mesh/WarmNodeStore.cpp#L406-L408: shorten corrupt-header handling rationale.src/mesh/WarmNodeStore.cpp#L457-L460: condense V1/V2 normalization rationale.src/mesh/WarmNodeStore.cpp#L478-L480: shorten the forced-rotation explanation.src/mesh/WarmNodeStore.cpp#L582-L584: condense the V1 filesystem migration note.test/test_warm_store/test_main.cpp#L285-L287: shorten the V2 test description.test/test_nodedb_blocked/test_main.cpp#L135-L138: shorten the signer round-trip description.As per coding guidelines, keep comments to one or two lines maximum and only explain non-obvious reasons.
🤖 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/WarmNodeStore.h` around lines 46 - 53, Condense the explanatory comments to one or two lines at each listed site: summarize metadata layout and timestamp quantization in WarmNodeStore.h; reduce the legacy-format, corrupt-header, V1/V2 normalization, forced-rotation, and V1 filesystem migration explanations in WarmNodeStore.cpp; and shorten the V2 test and signer round-trip descriptions in the two test_main.cpp files. Preserve only the non-obvious rationale and do not change code behavior.Source: Coding guidelines
test/test_warm_store/test_main.cpp (1)
329-334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that V2 migration preserves the timestamp.
The test promises time preservation but only verifies signer, role, and protection. Assert
warmTimeOf(e)so corruption of the upper timestamp bits cannot pass unnoticed.Proposed test assertion
TEST_ASSERT_FALSE_MESSAGE(warmSignerOf(e), "a v2 timestamp bit must not read as a signer"); + TEST_ASSERT_EQUAL(123456u & WARM_TIME_MASK, warmTimeOf(e)); TEST_ASSERT_EQUAL(5, warmRoleOf(e));🤖 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_warm_store/test_main.cpp` around lines 329 - 334, Extend the V2 migration assertions in the test around warmSignerOf, warmRoleOf, and warmProtOf to also verify warmTimeOf(e) preserves the expected timestamp value. Use the timestamp established by the test fixture or migration input, ensuring upper timestamp-bit corruption is detected.
🤖 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/WarmNodeStore.h`:
- Around line 46-53: Condense the explanatory comments to one or two lines at
each listed site: summarize metadata layout and timestamp quantization in
WarmNodeStore.h; reduce the legacy-format, corrupt-header, V1/V2 normalization,
forced-rotation, and V1 filesystem migration explanations in WarmNodeStore.cpp;
and shorten the V2 test and signer round-trip descriptions in the two
test_main.cpp files. Preserve only the non-obvious rationale and do not change
code behavior.
In `@test/test_warm_store/test_main.cpp`:
- Around line 329-334: Extend the V2 migration assertions in the test around
warmSignerOf, warmRoleOf, and warmProtOf to also verify warmTimeOf(e) preserves
the expected timestamp value. Use the timestamp established by the test fixture
or migration input, ensuring upper timestamp-bit corruption is detected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 865aead0-f9b1-4861-a5bd-b2f4133ffe93
📒 Files selected for processing (5)
src/mesh/NodeDB.cppsrc/mesh/WarmNodeStore.cppsrc/mesh/WarmNodeStore.htest/test_nodedb_blocked/test_main.cpptest/test_warm_store/test_main.cpp
|
Both addressed in 990297a. Comment length: condensed every comment this PR adds or touches to two lines, per the one-or-two-line guideline. Skipped one site from the list: the corrupt-header rationale in Timestamp assertion: added, good catch. The test claimed time carries over but only checked the flag bits, so a mask error in the upper bits would have gone unnoticed. Note the fixture happens to make this sharper than it looks: 123456 has bit 6 set in its own timestamp, so the assertion also pins that |
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR updates WarmNodeStore persistence to retain whether a node has ever produced a verified XEdDSA signature across eviction/re-admission, including safe migration for older warm-store formats.
Changes:
- Packs an additional “XEdDSA signer seen” bit into
last_heardmetadata and restores it on node re-admission. - Updates warm-store page/file magic (WRN3/WRM3) and migrates v1/v2 records safely (including clearing v2’s former timestamp bit 6).
- Adds tests covering signer-bit round trips, v2 snapshot migration, and NodeDB eviction/re-admission behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_warm_store/test_main.cpp | Adds signer-bit and v2-migration tests; updates quantization commentary. |
| test/test_nodedb_blocked/test_main.cpp | Adds NodeDB test ensuring signer bit survives warm-tier round trip. |
| src/mesh/WarmNodeStore.h | Expands packed metadata to include signer bit; adds format enum and accessor. |
| src/mesh/WarmNodeStore.cpp | Implements v3 magic + v1/v2 replay normalization for both ring and file backends. |
| src/mesh/NodeDB.cpp | Persists and restores signer bit when demoting/evicting/re-admitting nodes. |
…, test robustness)
|
Copilot findings addressed in 87a94c6:
Left the identical read/write pattern in the pre-existing v1-migration and oversized-snapshot tests as-is to keep this diff to the signer change; happy to tighten those in a follow-up if you'd like them consistent. |
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
The warm tier already caches an evicted node's role and protected category, but not
whether we ever verified an XEdDSA signature from it. That flag is learned from
verified traffic rather than from NodeInfo, so a node that was evicted and later
re-admitted came back with it clear until the next signed packet arrived.
Cache it in the same packed word:
last_heardnow carries 7 metadata bits (role 4,protected 2, signer 1) and
getOrCreateMeshNode()restores the flag on re-admissionalongside the role and key.
WarmNodeEntrystays 40 B.Bit 6 was part of the quantised timestamp under the v2 record format, so a v2 record
read with the new layout would report a signer about half the time. The record magic
moves to WRN3/WRM3 and
load()clears that bit on v2 records on both backends(raw-flash ring and warm.dat), rewriting them in the current format. v2 keeps its
role/protected bits; v1 handling is unchanged (identity + key kept, last_heard
discarded). The ring forces a rotation when the head page predates v3 so new records
never land in an older-format page.
Tests: signer round trip through absorb/take with a cleared-flag control; a v2
snapshot migration asserting a stale timestamp bit does not read as a signer; and an
eviction/re-admission round trip through NodeDB with a non-signer control.
Note for #10967, which touches the same metadata: it adds
WarmProtected::XeddsaSigneras a fourth category value for this purpose. Since
WarmProtectedholds one value,that makes the flag exclusive with the existing categories, and
warmProtectedCategory()returns it ahead of the favourite/ignored/manually-verified check, so a favourite or a
TRACKER that has signed comes back without its protected category. A dedicated bit keeps
the two independent.
hasSeenXeddsaSigner()from that PR works unchanged on top of thisvia
lookupMeta; only the category value needs dropping on rebase.Summary by CodeRabbit