From 25ada1023de4e605b946b1b8889177dae890ed0c Mon Sep 17 00:00:00 2001 From: George <509474+giannoug@users.noreply.github.com> Date: Tue, 8 Sep 2026 20:30:27 +0300 Subject: [PATCH] fix: correct traceroute path orientation when building graph edges (#162) * fix: correct traceroute path orientation when building graph edges A completed traceroute is observed as a response packet, whose from/to are reversed with respect to the path being traced: `to` is the node that started the traceroute and `from` is the node that answered. Meshview assembled paths as [from, *route, to], so both endpoint-adjacent edges were wrong for every completed traceroute with at least one intermediate hop. Because the false edges always attach to the endpoints, a busy initiator or gateway accumulated false direct links to every remote target it ever traced. Confirmed against TraceRouteModule::alterReceivedProtobuf, which renders a response as printRoute(r, p.to, p.from, false), against the meshtastic python CLI, which starts the forward path at p["to"] and the return path at p["from"], and against real recorded traceroutes where a request's route[] is a prefix of the matching response's route[]. Add meshview/traceroute.py and use it from /api/edges, /api/traceroute and graph_traceroute, which had three separate copies of this logic that had all drifted. /api/traceroute also gains initiator/target so the traceroute page stops re-deriving the endpoints itself, which is where a fourth copy lived. While in /api/edges, skip the NODENUM_BROADCAST placeholders that insertUnknownHops() writes into route[] (they became a pseudo-node hub in the graph) and guard against the (gateway, gateway) self edge that graph_traceroute already guarded against. * fix: do not invent links across unknown hops or unfinished returns Review of becf4a8 found two remaining path errors. Dropping NODENUM_BROADCAST from route[] during assembly joined the nodes either side of it, so A -> B -> ? -> C -> D became A -> B -> C -> D and a lone unknown hop collapsed into a false direct A -> D. Keep the placeholder in the path and skip the edges that touch it instead. On a 2660 row sample this removes 10 links that do not exist. done=True only means a response was observed, not that it arrived. A gateway usually reports one still travelling home with route_back holding just the hops so far, so always appending the initiator claimed a final link that was never seen. Append it only when the response reached the initiator, which the spare snr_back entry from SNRonly=isToUs() marks. Note this is rare in practice: 2 of 303 real responses, since the entry is added by the initiator and gateways see packets in flight. The deduplicated unique_forward_paths and unique_reverse_paths carry no per-observation provenance, so the traceroute page can no longer tell which of them reached the far endpoint. It now adds only the adjacent endpoint. * fix: treat a response as proof the forward path reached the target Winning path selection required return hops, so a response observed before its first return hop contributed nothing and the page fell back to the deduplicated reported paths, which cannot tell which of them reached the target. A response with route=[B,C] and no route_back rendered A > B > C, losing the target and the last link. Select winning paths from response observations instead. A response only exists because the request arrived, whatever its return trip went on to do. Dropping the non-empty forward_hops guard at the same time means a zero hop response contributes its real direct link, so a fully direct traceroute and a completed direct return are no longer excluded from the graph. Mark reported forward paths that some response reported, so the page can close those on the target and they deduplicate against the winning path instead of drawing a shorter copy underneath it. --- docs/API_Documentation.md | 43 ++++++++++++++++++----- meshview/templates/traceroute.html | 9 +++-- meshview/traceroute.py | 55 ++++++++++++++++++++++++++++++ meshview/web.py | 19 ++++++----- meshview/web_api/api.py | 53 ++++++++++++++++------------ 5 files changed, 138 insertions(+), 41 deletions(-) create mode 100644 meshview/traceroute.py diff --git a/docs/API_Documentation.md b/docs/API_Documentation.md index 564c135..5531c26 100644 --- a/docs/API_Documentation.md +++ b/docs/API_Documentation.md @@ -203,6 +203,12 @@ Returns network edges (connections between nodes) based on traceroutes and neigh Traceroute edges are collected over the last 12 hours. Neighbor edges are based on port 71 packets. +A completed traceroute is observed as a response packet, whose `from`/`to` are reversed +with respect to the traced path, so edges are built as `to -> route[] -> from`. `route[]` +may contain `0xFFFFFFFF` placeholders for hops the mesh could not identify; the edges on +either side of such a placeholder are omitted, because the nodes around an unknown hop +are not neighbours. + Query Parameters - `type` (optional, string): `traceroute` or `neighbor`. If omitted, returns both. - `node_id` (optional, int): Filter edges to only those touching a node. @@ -324,13 +330,30 @@ Returns traceroute details and derived paths for a packet. Path Parameters - `packet_id` (required, int): Packet ID. +A completed traceroute is observed as a response packet, whose `from`/`to` are reversed +with respect to the traced path. `initiator` and `target` resolve this: the forward path +runs `initiator -> forward_hops -> target` and the return path runs +`target -> reverse_hops -> initiator`, in both directions of travel. `forward_hops` and +`reverse_hops` contain only the intermediate hops, never the endpoints. + +A gateway usually reports a response that is still travelling home, so `reverse_hops` is +often incomplete. `reverse_complete` says whether that observation had reached the +initiator; only then does the corresponding `winning_paths.reverse` entry end at the +initiator. + +`winning_paths` is built from response observations, since a response proves the request +reached the target. `complete` on a `unique_forward_paths` entry says whether some +response reported that hop list, and so whether it is known to end at the target. + Response Example ```json { "packet": { "id": 123, - "from": 111, - "to": 222, + "from": 222, + "to": 111, + "initiator": 111, + "target": 222, "channel": "main" }, "traceroute_packets": [ @@ -338,19 +361,21 @@ Response Example "index": 0, "gateway_node_id": 333, "done": true, - "forward_hops": [111, 444, 222], - "reverse_hops": [222, 444, 111] + "forward_hops": [444, 555], + "reverse_hops": [555, 444], + "reverse_complete": true } ], "unique_forward_paths": [ - { "path": [111, 444, 222], "count": 2 } + { "path": [444, 555], "count": 2, "complete": true } ], "unique_reverse_paths": [ - [222, 444, 111] + [555, 444] ], - "winning_paths": [ - [111, 444, 222] - ] + "winning_paths": { + "forward": [[111, 444, 555, 222]], + "reverse": [[222, 555, 444, 111]] + } } ``` diff --git a/meshview/templates/traceroute.html b/meshview/templates/traceroute.html index 84aa833..f42993e 100644 --- a/meshview/templates/traceroute.html +++ b/meshview/templates/traceroute.html @@ -121,9 +121,11 @@ function addMissingEndpoints(path, startId, endId) { return fullPath; } +// These lists are deduplicated across observations. A forward path is only known to +// reach the target if some response reported it; a reverse path carries no such marker. function uniqueForwardPaths(data, originId, targetId) { return (data?.unique_forward_paths || []) - .map(item => addMissingEndpoints(item.path || [], originId, targetId)) + .map(item => addMissingEndpoints(item.path || [], originId, item.complete ? targetId : null)) .filter(path => path.length > 1); } @@ -157,8 +159,9 @@ async function loadTraceroute() { const nodes = new Map(); const edges = []; - const originId = data?.packet?.from != null ? String(data.packet.from) : null; - const targetId = data?.packet?.to != null ? String(data.packet.to) : null; + // A response packet carries from/to reversed, so use the endpoints the API resolved. + const originId = data?.packet?.initiator != null ? String(data.packet.initiator) : null; + const targetId = data?.packet?.target != null ? String(data.packet.target) : null; const forwardPaths = (data?.winning_paths?.forward || []).map(path => path.map(id => String(id))); const reversePaths = (data?.winning_paths?.reverse || []).map(path => path.map(id => String(id))); const winningPathKeys = new Set([ diff --git a/meshview/traceroute.py b/meshview/traceroute.py new file mode 100644 index 0000000..70a989d --- /dev/null +++ b/meshview/traceroute.py @@ -0,0 +1,55 @@ +"""Assemble node paths from Meshtastic RouteDiscovery payloads. + +A traceroute response carries the endpoints reversed with respect to the path +being traced: `to` is the node that started the traceroute and `from` is the node +that answered. Only the response direction is affected, so both orientations are +handled here rather than at each call site. +""" + +# Firmware inserts this into route[] as a placeholder for a hop it could not identify. +NODENUM_BROADCAST = 0xFFFFFFFF + + +def return_is_complete(route): + """True when the response was seen after it reached the node that started it. + + Relays append both their ID and their SNR, but the final recipient appends only its + SNR, so one spare SNR entry means the return trip finished. Gateways usually report + a response that is still in flight, where the remaining hops are not known yet. + """ + return len(route.snr_back) == len(route.route_back) + 1 + + +def forward_path(from_node_id, to_node_id, hops, done, gateway_node_id=None): + """Path towards the traced node, starting at the node that began the traceroute.""" + if done: + return [node for node in (to_node_id, *hops, from_node_id) if node is not None] + + path = [node for node in (from_node_id, *hops) if node is not None] + # Some nodes add themselves to the route before uplinking. + if gateway_node_id is not None and (not path or path[-1] != gateway_node_id): + path.append(gateway_node_id) + return path + + +def return_path(from_node_id, to_node_id, hops, complete): + """Path back from the traced node, starting at the node that answered.""" + path = [node for node in (from_node_id, *hops) if node is not None] + if complete and to_node_id is not None: + path.append(to_node_id) + return path + + +def edges(path): + """Consecutive pairs of a path, skipping links that run through an unknown hop.""" + for a, b in zip(path, path[1:], strict=False): + if NODENUM_BROADCAST in (a, b): + continue + yield a, b + + +def endpoints(from_node_id, to_node_id, done): + """The (initiator, target) of the traceroute, regardless of packet direction.""" + if done: + return to_node_id, from_node_id + return from_node_id, to_node_id diff --git a/meshview/web.py b/meshview/web.py index a393196..72beeb4 100644 --- a/meshview/web.py +++ b/meshview/web.py @@ -18,7 +18,7 @@ from markupsafe import Markup from meshtastic.protobuf.mesh_pb2 import Routing from meshtastic.protobuf.portnums_pb2 import PortNum -from meshview import config, database, decode_payload, migrations, models, store +from meshview import config, database, decode_payload, migrations, models, store, traceroute from meshview.__version__ import ( __version_string__, ) @@ -407,14 +407,17 @@ async def graph_traceroute(request): route = decode_payload.decode_payload(PortNum.TRACEROUTE_APP, tr.route) if route is None: continue - path = [packet.from_node_id] - path.extend(route.route) + path = traceroute.forward_path( + packet.from_node_id, + packet.to_node_id, + route.route, + tr.done, + tr.gateway_node_id, + ) + if not path: + continue if tr.done: - dest = packet.to_node_id - path.append(packet.to_node_id) - elif path[-1] != tr.gateway_node_id: - # It seems some nodes add them self to the list before uplinking - path.append(tr.gateway_node_id) + dest = path[-1] if not tr.done and tr.gateway_node_id not in node_seen_time and tr.import_time_us: node_seen_time[path[-1]] = tr.import_time_us diff --git a/meshview/web_api/api.py b/meshview/web_api/api.py index e4dbe66..3d94282 100644 --- a/meshview/web_api/api.py +++ b/meshview/web_api/api.py @@ -10,7 +10,7 @@ from aiohttp import web from sqlalchemy import func, select from meshtastic.protobuf.portnums_pb2 import PortNum -from meshview import database, decode_payload, store +from meshview import database, decode_payload, store, traceroute from meshview.__version__ import __version__, _git_revision_short, get_version_info from meshview.config import CONFIG from meshview.models import DailySnapshot, Node @@ -630,10 +630,15 @@ async def api_edges(request): if route is None or tr.packet is None: continue - path = [tr.packet.from_node_id] + list(route.route) - path.append(tr.packet.to_node_id if tr.done else tr.gateway_node_id) + path = traceroute.forward_path( + tr.packet.from_node_id, + tr.packet.to_node_id, + route.route, + tr.done, + tr.gateway_node_id, + ) - for a, b in zip(path, path[1:], strict=False): + for a, b in traceroute.edges(path): if (a, b) not in edges: edges[(a, b)] = "traceroute" edges_added_tr += 1 @@ -945,6 +950,7 @@ async def api_traceroute(request): "done": tr.done, "forward_hops": forward_list, "reverse_hops": reverse_list, + "reverse_complete": traceroute.return_is_complete(route), } ) @@ -955,6 +961,7 @@ async def api_traceroute(request): forward_paths = [] reverse_paths = [] + forward_complete = set() winning_forward_paths = [] winning_reverse_paths = [] @@ -968,10 +975,11 @@ async def api_traceroute(request): if tr["reverse_hops"]: reverse_paths.append(r) - if tr["reverse_hops"]: - if tr["forward_hops"]: - winning_forward_paths.append(f) - winning_reverse_paths.append(r) + # A response proves the request reached the target, whatever its return trip did. + if tr["done"]: + forward_complete.add(f) + winning_forward_paths.append(f) + winning_reverse_paths.append((r, tr["reverse_complete"])) # Deduplicate unique_forward_paths = sorted(set(forward_paths)) @@ -982,28 +990,26 @@ async def api_traceroute(request): # Convert for JSON output unique_forward_paths_json = [ - {"path": list(p), "count": forward_counts[p]} for p in unique_forward_paths + {"path": list(p), "count": forward_counts[p], "complete": p in forward_complete} + for p in unique_forward_paths ] unique_reverse_paths_json = [list(p) for p in unique_reverse_paths] from_node_id = packet.from_node_id to_node_id = packet.to_node_id + # Winning paths are only built from responses, which carry the endpoints reversed. winning_forward_with_endpoints = [] - for path in set(winning_forward_paths): - full_path = list(path) - if to_node_id is not None and (not full_path or full_path[-1] != to_node_id): - full_path = [to_node_id, *full_path] - if from_node_id is not None and (not full_path or full_path[-1] != from_node_id): - full_path = [*full_path, from_node_id] - winning_forward_with_endpoints.append(full_path) + for hops in dict.fromkeys(winning_forward_paths): + path = traceroute.forward_path(from_node_id, to_node_id, hops, done=True) + if len(path) > 1: + winning_forward_with_endpoints.append(path) winning_reverse_with_endpoints = [] - for path in set(winning_reverse_paths): - full_path = list(path) - if to_node_id is not None and (not full_path or full_path[0] != to_node_id): - full_path = [to_node_id, *full_path] - winning_reverse_with_endpoints.append(full_path) + for hops, complete in winning_reverse_paths: + path = traceroute.return_path(from_node_id, to_node_id, hops, complete) + if len(path) > 1 and path not in winning_reverse_with_endpoints: + winning_reverse_with_endpoints.append(path) winning_paths_json = { "forward": winning_forward_with_endpoints, @@ -1013,12 +1019,17 @@ async def api_traceroute(request): # -------------------------------------------- # Final API output # -------------------------------------------- + initiator, target = traceroute.endpoints( + from_node_id, to_node_id, any(tr["done"] for tr in tr_groups) + ) return web.json_response( { "packet": { "id": packet.id, "from": packet.from_node_id, "to": packet.to_node_id, + "initiator": initiator, + "target": target, "channel": packet.channel, }, "traceroute_packets": tr_groups,