fix: defer direct messages while learning peer keys - #11222
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR updates protobuf resources, extends queue-status reporting, rejects zero-filled public keys, and adds deferred direct-message recovery for PKI key exchange. Router, ReliableRouter, NodeInfoModule, and RoutingModule coordinate deferral, suppression, retries, timeouts, and terminal errors, with expanded test coverage. ChangesDeferred direct-message PKI flow
Estimated code review effort: 4 (Complex) | ~60 minutes 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 |
⚡ 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 (31)
Build artifacts expire on 2026-08-25. Updated for |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mesh/NodeDB.cpp (1)
3389-3399: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject zero-filled incoming public keys before persisting them.
This guard only treats an existing all-zero key as absent. An incoming
p.public_keywithsize == 32and all-zero bytes still reachesCopyUserToNodeInfoLite(info, p)and is stored. Later lookup helpers reject it, but downstream code can still interpretsize == 32as key presence, causing PKI failures and allowing malformed NodeInfo to poison the peer record.Validate the incoming key before the existing-key comparison and reject or normalize it as absent.
Proposed fix
`#if` !(MESHTASTIC_EXCLUDE_PKI) if (p.public_key.size == 32 && nodeId != nodeDB->getNodeNum()) { + if (memfll(p.public_key.bytes, 0, sizeof(p.public_key.bytes))) { + LOG_WARN("Rejecting zero-filled public key for node 0x%08x", nodeId); + return false; + } printBytes("Incoming Pubkey: ", p.public_key.bytes, 32);🤖 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.cpp` around lines 3389 - 3399, Validate p.public_key before the existing-key comparison in the surrounding NodeInfo update flow, treating a 32-byte all-zero incoming key as absent and preventing it from reaching CopyUserToNodeInfoLite(info, p). Preserve the current mismatch rejection for valid incoming keys and existing usable keys, while ensuring malformed zero-filled keys are rejected or normalized before persistence.
🧹 Nitpick comments (3)
test/test_packet_signing/test_main.cpp (1)
243-261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGroup M leaves process globals mutated for later groups.
nodeInfoModule,airTime, andportduino_config.force_simradioare set byenableNodeInfoForDmKeyWait()/enablePkiForLocalNode()but only restored ad-hoc insidetest_M2_...(Lines 1525-1527). Everything after Group M (Groups N/D/E) runs with the DM shims installed andforce_simradioforced false. Restoring these insetUp(or atearDown) would make the ordering irrelevant.🤖 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_packet_signing/test_main.cpp` around lines 243 - 261, Restore the process-wide DM test state in the common test lifecycle rather than only inside test_M2_..., including nodeInfoModule, airTime, the installed DM shims, and ARCH_PORTDUINO’s portduino_config.force_simradio. Update setUp or tearDown to reset these values after each test, reusing the existing saved-state variables and preserving the original configuration for subsequent groups.src/mesh/Router.h (1)
246-264: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the attempt-table bound and align constant casing.
peerKeyExchangeAttempts[8]is the only bound not expressed as a named constant. Also, project convention is UPPER_SNAKE_CASE for constants, while these are camelCase.♻️ Suggested tidy-up
- static constexpr uint8_t deferredDmCapacity = 2; + static constexpr uint8_t DEFERRED_DM_CAPACITY = 2; + static constexpr uint8_t PEER_KEY_ATTEMPT_CAPACITY = 8; @@ - } peerKeyExchangeAttempts[8]; + } peerKeyExchangeAttempts[PEER_KEY_ATTEMPT_CAPACITY];As per coding guidelines: "Use PascalCase for classes, camelCase for functions and member variables, and UPPER_SNAKE_CASE for constants and defines."
🤖 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.h` around lines 246 - 264, Update the constants near DeferredDm to use UPPER_SNAKE_CASE, and add a named constant for the peer-key exchange attempt capacity currently hardcoded as 8. Replace the literal array bound in peerKeyExchangeAttempts with that new capacity constant, and update all references to the renamed constants consistently.Source: Coding guidelines
src/mesh/ReliableRouter.cpp (1)
161-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the NodeDB copy-out accessor for the sender key, and format the reason code as hex.
Router.cppreads peer keys vianodeDB->copyPublicKey(...); readingsender->public_keyoff the returnedNodeInfoLitepointer here diverges from that pattern (and from the NodeDB accessor guidance). Also the error reason is logged with%dinstead of0x%x.♻️ Proposed change
- const meshtastic_NodeInfoLite *sender = nodeDB->getMeshNode(p->from); - const bool hasSenderKey = sender && sender->public_key.size == 32 && - !memfll(sender->public_key.bytes, 0, sizeof(sender->public_key.bytes)); + meshtastic_NodeInfoLite_public_key_t senderKey = {0, {0}}; + const bool hasSenderKey = nodeDB->copyPublicKey(p->from, senderKey) && senderKey.size == 32 && + !memfll(senderKey.bytes, 0, sizeof(senderKey.bytes)); const auto error = hasSenderKey ? meshtastic_Routing_Error_PKI_FAILED : meshtastic_Routing_Error_PKI_UNKNOWN_PUBKEY; - LOG_INFO("Undecryptable PKI packet from 0x%08x, send error %d", p->from, error); + LOG_INFO("Undecryptable PKI packet from 0x%08x, send error 0x%x", p->from, error);As per coding guidelines: "Format 32-bit node and packet IDs as
0x%08x; format one-byte values, flags, addresses, and reason codes as0x%x" and "use satellite copy-out accessors and flatNodeInfoLitefields".🤖 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/ReliableRouter.cpp` around lines 161 - 168, Update the sender-key check in the PKI handling branch to obtain the key through nodeDB->copyPublicKey(...) instead of reading sender->public_key from NodeInfoLite, while preserving the existing validity checks. Change the send-error LOG_INFO reason-code format from %d to 0x%x.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 `@src/mesh/Router.cpp`:
- Around line 1503-1524: The PEER_KEY retry in Router::processDeferredDms can
re-enter deferPeerKeyDm and indefinitely refresh deferred waits when another DM
for the same peer remains queued. Add a per-packet “already preflighted” marker
for the DM released by processDeferredDms, have deferPeerKeyDm recognize and
consume that marker to bypass the key-exchange deferral once, and preserve
normal deferral behavior for newly submitted packets.
In `@src/modules/NodeInfoModule.cpp`:
- Around line 177-181: The forceSend path in NodeInfo request handling bypasses
all per-destination rate limiting, allowing repeated recovery requests. Update
requestNodeInfo and its destination-key recovery caller to apply the existing
per-peer key-exchange attempt throttle, or enforce a shorter minimum interval
when forceSend is true, while preserving normal forced-send behavior outside
this recovery path.
In `@test/test_packet_signing/test_main.cpp`:
- Around line 2275-2279: Remove the post-send `dm->id` assertion from the test
after `pipelineRouter->sendLocal(dm, ...)`, since ownership has transferred and
`originalDmId` came from the same object. Keep the existing deferred-state and
radio-send assertions, or validate the packet ID through an owned record such as
`pipelineRadio->sentPackets` if that behavior must be checked.
---
Outside diff comments:
In `@src/mesh/NodeDB.cpp`:
- Around line 3389-3399: Validate p.public_key before the existing-key
comparison in the surrounding NodeInfo update flow, treating a 32-byte all-zero
incoming key as absent and preventing it from reaching
CopyUserToNodeInfoLite(info, p). Preserve the current mismatch rejection for
valid incoming keys and existing usable keys, while ensuring malformed
zero-filled keys are rejected or normalized before persistence.
---
Nitpick comments:
In `@src/mesh/ReliableRouter.cpp`:
- Around line 161-168: Update the sender-key check in the PKI handling branch to
obtain the key through nodeDB->copyPublicKey(...) instead of reading
sender->public_key from NodeInfoLite, while preserving the existing validity
checks. Change the send-error LOG_INFO reason-code format from %d to 0x%x.
In `@src/mesh/Router.h`:
- Around line 246-264: Update the constants near DeferredDm to use
UPPER_SNAKE_CASE, and add a named constant for the peer-key exchange attempt
capacity currently hardcoded as 8. Replace the literal array bound in
peerKeyExchangeAttempts with that new capacity constant, and update all
references to the renamed constants consistently.
In `@test/test_packet_signing/test_main.cpp`:
- Around line 243-261: Restore the process-wide DM test state in the common test
lifecycle rather than only inside test_M2_..., including nodeInfoModule,
airTime, the installed DM shims, and ARCH_PORTDUINO’s
portduino_config.force_simradio. Update setUp or tearDown to reset these values
after each test, reusing the existing saved-state variables and preserving the
original configuration for subsequent groups.
🪄 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: 45351631-fa5b-48b7-8942-6dda2165da52
⛔ Files ignored due to path filters (2)
src/mesh/generated/meshtastic/mesh.pb.cppis excluded by!**/generated/**,!src/mesh/generated/**src/mesh/generated/meshtastic/mesh.pb.his excluded by!**/generated/**,!src/mesh/generated/**
📒 Files selected for processing (16)
protobufssrc/mesh/MeshService.cppsrc/mesh/MeshService.hsrc/mesh/NodeDB.cppsrc/mesh/PhoneAPI.cppsrc/mesh/RadioInterface.hsrc/mesh/RadioLibInterface.cppsrc/mesh/ReliableRouter.cppsrc/mesh/Router.cppsrc/mesh/Router.hsrc/modules/NodeInfoModule.cppsrc/modules/NodeInfoModule.hsrc/modules/RoutingModule.cppsrc/platform/portduino/SimRadio.cpptest/test_mesh_module/test_main.cpptest/test_packet_signing/test_main.cpp
| void Router::processDeferredDms() | ||
| { | ||
| for (auto &deferred : deferredDms) { | ||
| meshtastic_MeshPacket *p = deferred.p; | ||
| if (!p) | ||
| continue; | ||
|
|
||
| if (deferred.reason == DeferredDm::Reason::PEER_KEY) { | ||
| if (!Throttle::isWithinTimespanMs(deferred.queuedAtMs, deferredDmPeerKeyWaitMs)) { | ||
| deferred.p = nullptr; | ||
| deferred.queuedAtMs = 0; | ||
| deferred.keyExchangeId = 0; | ||
| LOG_INFO("Retrying deferred DM id=0x%08x after NodeInfo response wait for 0x%08x", p->id, p->to); | ||
| rememberPeerKeyExchangeAttempt(p->to); | ||
| const PacketId dmId = p->id; | ||
| const ErrorCode result = send(p); | ||
| const auto state = isDeferredDm(dmId) ? meshtastic_QueueStatus_State_KEY_EXCHANGE | ||
| : meshtastic_QueueStatus_State_STATE_UNSPECIFIED; | ||
| service->sendQueueStatusToPhone(getQueueStatus(), result, dmId, state); | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Two DMs to the same peer can be deferred forever.
When the PEER_KEY wait expires, this clears the slot and re-enters send() → deferPeerKeyDm(). If a second DM to the same peer is still sitting in deferredDms, the lookup at Lines 1390-1395 inherits its keyExchangeId, which bypasses the hasPeerKeyExchangeAttempt() throttle at Line 1396, so the packet is re-deferred with a fresh queuedAtMs. Each DM keeps re-deferring the other and neither is ever transmitted, while runOnce() polls every second indefinitely.
Consider marking a DM that was just released from the deferred queue (e.g. a "already preflighted" flag consulted by deferPeerKeyDm) so the retry path always falls through to the actual send.
🤖 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 1503 - 1524, The PEER_KEY retry in
Router::processDeferredDms can re-enter deferPeerKeyDm and indefinitely refresh
deferred waits when another DM for the same peer remains queued. Add a
per-packet “already preflighted” marker for the DM released by
processDeferredDms, have deferPeerKeyDm recognize and consume that marker to
bypass the key-exchange deferral once, and preserve normal deferral behavior for
newly submitted packets.
| if (!forceSend && !shorterTimeout && lastNodeInfo && Throttle::isWithinTimespanMs(lastNodeInfo, timeoutMs)) { | ||
| LOG_DEBUG("Skip send NodeInfo since we sent it <%us ago", timeoutMs / 1000); | ||
| ignoreRequest = true; // Mark it as ignored for MeshModule | ||
| return NULL; | ||
| } else if (shorterTimeout && lastNodeInfo && Throttle::isWithinTimespanMs(lastNodeInfo, 60 * 1000)) { | ||
| } else if (!forceSend && shorterTimeout && lastNodeInfo && Throttle::isWithinTimespanMs(lastNodeInfo, 60 * 1000)) { |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
forceSend removes every NodeInfo rate limit on the DESTINATION_KEY recovery path.
Router::deferMissingKeyDm() calls requestNodeInfo() (force=true) with no equivalent of the hasPeerKeyExchangeAttempt() throttle used by the peer-key path, so each DM attempt to a keyless node emits an unthrottled directed NodeInfo. Only isTxAllowedChannelUtil() remains between a chatty client and repeated airtime use.
Suggest gating the destination-key request with the same per-peer attempt window, or keeping a shorter floor (rather than none) when forceSend is set.
🤖 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/NodeInfoModule.cpp` around lines 177 - 181, The forceSend path in
NodeInfo request handling bypasses all per-destination rate limiting, allowing
repeated recovery requests. Update requestNodeInfo and its destination-key
recovery caller to apply the existing per-peer key-exchange attempt throttle, or
enforce a shorter minimum interval when forceSend is true, while preserving
normal forced-send behavior outside this recovery path.
Source: Coding guidelines
| TEST_ASSERT_EQUAL(1, pipelineRouter->deferredDmPending()); | ||
| TEST_ASSERT_EQUAL(1, pipelineRadio->sendCalls); | ||
| TEST_ASSERT_FALSE(nodeDB->copyPublicKey(REMOTE_NODE, storedKey)); | ||
| TEST_ASSERT_EQUAL_HEX32(originalDmId, dm->id); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Don't dereference dm after sendLocal() took ownership.
pipelineRouter->sendLocal(dm, ...) transfers ownership of the packet (deferred store or release to pool), so dm->id on Line 2278 reads memory the test no longer owns; it only happens to be alive because the DM is still deferred. The assertion is also tautological — originalDmId was copied from that same object. Drop it, or assert against pipelineRadio->sentPackets/deferred state instead.
🧹 Proposed cleanup
TEST_ASSERT_EQUAL(1, pipelineRouter->deferredDmPending());
TEST_ASSERT_EQUAL(1, pipelineRadio->sendCalls);
TEST_ASSERT_FALSE(nodeDB->copyPublicKey(REMOTE_NODE, storedKey));
- TEST_ASSERT_EQUAL_HEX32(originalDmId, dm->id);
+ TEST_ASSERT_TRUE(pipelineRouter->isDeferredDm(originalDmId));
}🤖 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_packet_signing/test_main.cpp` around lines 2275 - 2279, Remove the
post-send `dm->id` assertion from the test after `pipelineRouter->sendLocal(dm,
...)`, since ownership has transferred and `originalDmId` came from the same
object. Keep the existing deferred-state and radio-send assertions, or validate
the packet ID through an owned record such as `pipelineRadio->sentPackets` if
that behavior must be checked.
1 participant
Summary
KEY_EXCHANGEfor a duplicate client submission while the original message is still pending, without a second RF transmission.QueueStatus.state = KEY_EXCHANGEassociated with the original message ID for every deferred state, including a re-deferred retry.Client behavior
QueueStatus.state = KEY_EXCHANGEmeans the radio is waiting for a short NodeInfo/key-refresh exchange, not that the DM was delivered. Clients should retain the existing pending row formesh_packet_idand update its state rather than create a second message.Dependency
Depends on meshtastic/protobufs#1020, which adds the additive QueueStatus lifecycle field.
Testing
native-macostest_packet_signing: 86/86 passednative-macostest_mesh_module: 25/25 passednative-macostest_stream_api: 13/13 passedtrunk fmtandgit diff --checkpassedHardware scope
The attached nRF test nodes are USB-visible but currently do not complete app-protocol discovery or serial DFU handshakes. This PR does not claim a two-node PKI DM delivery test on the physical bench.
Summary by CodeRabbit
New Features
Bug Fixes
Tests