fix : avoid our node to be a relaynode candidate#4037
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4037 +/- ##
========================================
- Coverage 0.56% 0.56% -0.01%
========================================
Files 405 405
Lines 24163 24167 +4
Branches 3100 3102 +2
========================================
Hits 136 136
- Misses 24006 24010 +4
Partials 21 21 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Pull request overview
This PR fixes relay node resolution by filtering out unseen nodes (lastHeard == 0) and explicitly excluding the local node from being selected as a relay candidate. This prevents false positives from suffix-based node ID matching that could result in inaccurate relay information.
Key Changes:
- Added
ourNodeNumparameter toPacket.getRelayNode()to enable local node exclusion - Filtered out nodes with
lastHeard == 0to exclude placeholder/unseen nodes - Updated all call sites to pass the local node number
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| core/database/src/main/kotlin/org/meshtastic/core/database/entity/Packet.kt | Modified getRelayNode() to accept ourNodeNum parameter, filter unseen nodes, and exclude local node from relay candidates |
| feature/settings/src/main/kotlin/org/meshtastic/feature/settings/debugging/DebugViewModel.kt | Updated to retrieve and pass myNodeNum to getRelayNode() for relay node annotation |
| feature/messaging/src/main/kotlin/org/meshtastic/feature/messaging/MessageListPaged.kt | Added ourNode parameter to MessageStatusDialog and passed ourNode?.num to getRelayNode() |
| val candidateRelayNodes = | ||
| nodes.filter { it.lastHeard != 0 && (it.num and RELAY_NODE_SUFFIX_MASK) == relayNodeIdSuffix } | ||
|
|
||
| val closestRelayNode = | ||
| if (candidateRelayNodes.size == 1) { | ||
| candidateRelayNodes.first() | ||
| } else { | ||
| candidateRelayNodes.minByOrNull { it.hopsAway } | ||
| } | ||
|
|
||
| if (closestRelayNode?.num == ourNodeNum) { | ||
| // it's us, useless to return relay node for ourselves | ||
| return null | ||
| } |
There was a problem hiding this comment.
The local node exclusion logic should be moved into the filter predicate (line 89) rather than being checked after finding the closest relay node. The current implementation has a bug: if there are multiple candidate relay nodes and the local node happens to be the closest (lowest hopsAway), it will return null instead of returning the second-closest relay node.
The filter should be:
nodes.filter { it.num != ourNodeNum && it.lastHeard != 0 && (it.num and RELAY_NODE_SUFFIX_MASK) == relayNodeIdSuffix }
This ensures the local node is excluded upfront and allows the algorithm to correctly select the next closest relay node when needed.
| fun getRelayNode(relayNodeId: Int, nodes: List<Node>, ourNodeNum: Int?): Node? { | ||
| val relayNodeIdSuffix = relayNodeId and RELAY_NODE_SUFFIX_MASK | ||
| val candidateRelayNodes = nodes.filter { (it.num and RELAY_NODE_SUFFIX_MASK) == relayNodeIdSuffix } | ||
|
|
||
| val candidateRelayNodes = | ||
| nodes.filter { it.lastHeard != 0 && (it.num and RELAY_NODE_SUFFIX_MASK) == relayNodeIdSuffix } | ||
|
|
||
| val closestRelayNode = | ||
| if (candidateRelayNodes.size == 1) { | ||
| candidateRelayNodes.first() | ||
| } else { | ||
| candidateRelayNodes.minByOrNull { it.hopsAway } | ||
| } | ||
|
|
||
| if (closestRelayNode?.num == ourNodeNum) { | ||
| // it's us, useless to return relay node for ourselves | ||
| return null | ||
| } | ||
|
|
||
| return closestRelayNode | ||
| } |
There was a problem hiding this comment.
The new relay node filtering logic lacks test coverage. Since the database module has existing unit tests (DatabaseManagerEvictionTest), this critical bug fix should include unit tests for the getRelayNode function to verify:
- Nodes with lastHeard == 0 are excluded
- The local node (matching ourNodeNum) is excluded
- Correct relay node selection when multiple candidates exist
- Proper handling when ourNodeNum is null
- Edge case: when the only candidate is the local node
| val relayNodeName by | ||
| remember(message.relayNode, nodes) { | ||
| derivedStateOf { | ||
| message.relayNode?.let { relayNodeId -> Packet.getRelayNode(relayNodeId, nodes)?.user?.longName } | ||
| message.relayNode?.let { relayNodeId -> | ||
| Packet.getRelayNode(relayNodeId, nodes, ourNode?.num)?.user?.longName | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The remember block is missing ourNode in its dependency list. The derivedStateOf uses ourNode?.num on line 453, but ourNode is not included in the remember keys on line 450. This means if ourNode changes (e.g., from null to a valid node), the relayNodeName won't be recomputed, potentially causing stale relay node information to be displayed.
|
I'm a bit hesitant to move forward with this feature at all - it was a bit half-baked to begin with and has had conversation about reworking. The partial id matching is pretty bad especially in larger networks where more collisions may occur. @RCGV1 has been involved in a lot of those conversations and may have more insight on the future of this feature. |
Hi, thanks for the feedback. In my independent development, I’m working on a UI feature that displays the I agree that relay resolution may still fail in very large or dense networks by using the method getRelayNode. 3 methods : Works decently. During testing, filtering out nodes obtained by getRelayNode with lastHeard != 0 and explicitly Also, copilot is right, im sorry i should have filtered before checking , i'll fix it |
|
Let's give it a try - the current implementation is bad, and this looks like it'll improve things. Thanks! |
What
Improves relay node resolution by preventing the local node and unseen nodes
(lastHeard == 0) from being selected as relay candidates.
Why
Suffix-based node ID matching may result in false positives, causing the local
node or placeholder nodes to be incorrectly resolved as relay nodes. This can
lead to inaccurate relay info and misleading debug information.
How
Fixes #4036