Limit NodeInfo discovery replies to direct neighbors#11131
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMeshModule centralizes response hop-limit selection through an overridable hook. NodeInfoModule restricts qualifying direct broadcast discovery responses to zero hops, ignores other broadcast requests, and adds tests for these behaviors. ChangesResponse hop-limit policy
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Requester
participant NodeInfoModule
participant MeshModule
participant RoutingModule
Requester->>NodeInfoModule: Send broadcast NodeInfo request
NodeInfoModule->>NodeInfoModule: Check direct discovery conditions
NodeInfoModule->>MeshModule: Allocate response
MeshModule->>NodeInfoModule: Get response hop limit
NodeInfoModule->>MeshModule: Return 0 for direct discovery
MeshModule->>Requester: Send zero-hop NodeInfo response
Possibly related PRs
Suggested labels: 🚥 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 |
⚡ 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-20. 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: 1
🧹 Nitpick comments (1)
test/test_mesh_module/test_main.cpp (1)
455-492: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an end-to-end NodeInfo regression test.
These tests exercise the predicate and hook directly, but not
NodeInfoModule::allocReply()orMeshModule::sendResponse(). Add coverage for a relayed/unknown broadcast followed by a valid direct broadcast from the same sender, asserting that the direct request still receives a zero-hop reply and invalid requests produce no NAK.🤖 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_mesh_module/test_main.cpp` around lines 455 - 492, Add an end-to-end NodeInfo regression test that drives relayed/unknown and valid direct broadcast requests from the same sender through NodeInfoModule::allocReply() and MeshModule::sendResponse(). Assert invalid requests produce no NAK, then verify the subsequent valid direct broadcast receives a zero-hop reply, covering the full response path rather than only NodeInfoPolicyShim.
🤖 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/NodeInfoModule.cpp`:
- Around line 163-169: Update handleReceivedProtobuf so rejected non-direct
broadcast requests do not update lastNodeInfoSeen before the suppression check.
Perform the isBroadcastRequest/isDirectBroadcastDiscoveryRequest qualification
before recording the request, or explicitly bypass the cache update for rejected
broadcasts, while preserving the existing ignoreRequest behavior and valid
direct-discovery reply flow.
---
Nitpick comments:
In `@test/test_mesh_module/test_main.cpp`:
- Around line 455-492: Add an end-to-end NodeInfo regression test that drives
relayed/unknown and valid direct broadcast requests from the same sender through
NodeInfoModule::allocReply() and MeshModule::sendResponse(). Assert invalid
requests produce no NAK, then verify the subsequent valid direct broadcast
receives a zero-hop reply, covering the full response path rather than only
NodeInfoPolicyShim.
🪄 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: 2818a64d-05a4-4f29-8d3a-43ee3d2ccae0
📒 Files selected for processing (5)
src/mesh/MeshModule.cppsrc/mesh/MeshModule.hsrc/modules/NodeInfoModule.cppsrc/modules/NodeInfoModule.htest/test_mesh_module/test_main.cpp
|
@coderabbitai review |
✅ Action performedReview finished.
|
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/NodeInfoModule.cpp (1)
34-44: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate the request before updating suppression state.
This cache update still happens before the later
is_licensedand unsigned-signed broadcast checks. A qualifying but rejected direct broadcast can therefore poisonlastNodeInfoSeenand suppress the next valid discovery from that sender for 12 hours. Move suppression tracking after those rejection checks, or explicitly avoid caching rejected requests.🤖 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 34 - 44, Move the `lastNodeInfoSeen` lookup, suppression decision, and cache update out of the initial request block so they run only after the later `is_licensed` and unsigned-signed broadcast rejection checks. Ensure rejected direct broadcasts do not set `suppressReplyForCurrentRequest` or update `lastNodeInfoSeen`, while valid discovery requests retain the existing suppression behavior.
🤖 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/NodeInfoModule.cpp`:
- Around line 34-44: Move the `lastNodeInfoSeen` lookup, suppression decision,
and cache update out of the initial request block so they run only after the
later `is_licensed` and unsigned-signed broadcast rejection checks. Ensure
rejected direct broadcasts do not set `suppressReplyForCurrentRequest` or update
`lastNodeInfoSeen`, while valid discovery requests retain the existing
suppression behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 15cdbbb5-1a0a-42ba-a527-fead54ae88bb
📒 Files selected for processing (2)
src/modules/NodeInfoModule.cpptest/test_mesh_module/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- test/test_mesh_module/test_main.cpp
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
For a while we talked about dynamically detecting we were on a busy mesh. This seems like another optimization that should be gated to busy meshes, EG, when the nodedb is full and nodes are regularly falling off the end of the DB. |
2 participants
Summary
Validation
trunk checkpio test -e native -f test_mesh_module(blocked before test compilation on macOS: Portduino requiresargp.h)./bin/run-tests.sh(blocked on macOS Bash 3.2 / BSDfind:mapfileand-printfunavailable)pio run -e heltec-v3(blocked by incomplete local Xtensa toolchain install; compiler not found)Summary by CodeRabbit
Bug Fixes
Tests