Fix #3452: only alter received packet if port number matches (#3474)

* Use `alterReceivedProtobuf()` for NeighborInfo and Traceroute
`alterReceived()` should never return NULL
Traceroute should be promiscuous

* Remove extensive logging from NeighborInfo module

src/mesh/FloodingRouter.cpp

@@ -49,15 +49,6 @@ void FloodingRouter::sniffReceived(const meshtastic_MeshPacket *p, const meshtas
 
                 tosend->hop_limit--; // bump down the hop count
 
-                if (p->which_payload_variant == meshtastic_MeshPacket_decoded_tag) {
-                    // If it is a traceRoute request, update the route that it went via me
-                    if (traceRouteModule && traceRouteModule->wantPacket(p))
-                        traceRouteModule->updateRoute(tosend);
-                    // If it is a neighborInfo packet, update last_sent_by_id
-                    if (neighborInfoModule && neighborInfoModule->wantPacket(p))
-                        neighborInfoModule->updateLastSentById(tosend);
-                }
-
                 LOG_INFO("Rebroadcasting received floodmsg to neighbors\n");
                 // Note: we are careful to resend using the original senders node id
                 // We are careful not to call our hooked version of send() - because we don't want to check this again

src/mesh/FloodingRouter.h

@@ -2,8 +2,6 @@
 
 #include "PacketHistory.h"
 #include "Router.h"
-#include "modules/NeighborInfoModule.h"
-#include "modules/TraceRouteModule.h"
 
 /**
  * This is a mixin that extends Router with the ability to do Naive Flooding (in the standard mesh protocol sense)

src/mesh/ProtobufModule.h

@@ -108,8 +108,8 @@ template <class T> class ProtobufModule : protected SinglePortModule
                 // if we can't decode it, nobody can process it!
                 return;
             }
-        }
 
-        return alterReceivedProtobuf(mp, decoded);
+            return alterReceivedProtobuf(mp, decoded);
+        }
     }
 };

src/modules/NeighborInfoModule.cpp

@@ -18,73 +18,24 @@ void NeighborInfoModule::printNeighborInfo(const char *header, const meshtastic_
 {
     LOG_DEBUG("%s NEIGHBORINFO PACKET from Node 0x%x to Node 0x%x (last sent by 0x%x)\n", header, np->node_id,
               nodeDB->getNodeNum(), np->last_sent_by_id);
-    LOG_DEBUG("----------------\n");
     LOG_DEBUG("Packet contains %d neighbors\n", np->neighbors_count);
     for (int i = 0; i < np->neighbors_count; i++) {
         LOG_DEBUG("Neighbor %d: node_id=0x%x, snr=%.2f\n", i, np->neighbors[i].node_id, np->neighbors[i].snr);
     }
-    LOG_DEBUG("----------------\n");
-}
-/*
-Prints the nodeDB nodes so we can see whose nodeInfo we have
-NOTE: for debugging only
-*/
-void NeighborInfoModule::printNodeDBNodes(const char *header)
-{
-    int num_nodes = nodeDB->getNumMeshNodes();
-    LOG_DEBUG("%s NODEDB SELECTION from Node 0x%x:\n", header, nodeDB->getNodeNum());
-    LOG_DEBUG("----------------\n");
-    LOG_DEBUG("DB contains %d nodes\n", num_nodes);
-    for (int i = 0; i < num_nodes; i++) {
-        const meshtastic_NodeInfoLite *dbEntry = nodeDB->getMeshNodeByIndex(i);
-        LOG_DEBUG("     Node %d: node_id=0x%x, snr=%.2f\n", i, dbEntry->num, dbEntry->snr);
-    }
-    LOG_DEBUG("----------------\n");
 }
 
 /*
 Prints the nodeDB neighbors
 NOTE: for debugging only
 */
-void NeighborInfoModule::printNodeDBNeighbors(const char *header)
+void NeighborInfoModule::printNodeDBNeighbors()
 {
     int num_neighbors = getNumNeighbors();
-    LOG_DEBUG("%s NODEDB SELECTION from Node 0x%x:\n", header, nodeDB->getNodeNum());
-    LOG_DEBUG("----------------\n");
-    LOG_DEBUG("DB contains %d neighbors\n", num_neighbors);
+    LOG_DEBUG("Our NodeDB contains %d neighbors\n", num_neighbors);
     for (int i = 0; i < num_neighbors; i++) {
         const meshtastic_Neighbor *dbEntry = getNeighborByIndex(i);
         LOG_DEBUG("     Node %d: node_id=0x%x, snr=%.2f\n", i, dbEntry->node_id, dbEntry->snr);
     }
-    LOG_DEBUG("----------------\n");
-}
-
-/*
-Prints the nodeDB with selectors for the neighbors we've chosen to send (inefficiently)
-Uses LOG_DEBUG, which equates to Console.log
-NOTE: For debugging only
-*/
-void NeighborInfoModule::printNodeDBSelection(const char *header, const meshtastic_NeighborInfo *np)
-{
-    int num_neighbors = getNumNeighbors();
-    LOG_DEBUG("%s NODEDB SELECTION from Node 0x%x:\n", header, nodeDB->getNodeNum());
-    LOG_DEBUG("----------------\n");
-    LOG_DEBUG("Selected %d neighbors of %d DB neighbors\n", np->neighbors_count, num_neighbors);
-    for (int i = 0; i < num_neighbors; i++) {
-        meshtastic_Neighbor *dbEntry = getNeighborByIndex(i);
-        bool chosen = false;
-        for (int j = 0; j < np->neighbors_count; j++) {
-            if (np->neighbors[j].node_id == dbEntry->node_id) {
-                chosen = true;
-            }
-        }
-        if (!chosen) {
-            LOG_DEBUG("     Node %d: neighbor=0x%x, snr=%.2f\n", i, dbEntry->node_id, dbEntry->snr);
-        } else {
-            LOG_DEBUG("---> Node %d: neighbor=0x%x, snr=%.2f\n", i, dbEntry->node_id, dbEntry->snr);
-        }
-    }
-    LOG_DEBUG("----------------\n");
 }
 
 /* Send our initial owner announcement 35 seconds after we start (to give network time to setup) */
@@ -129,9 +80,7 @@ uint32_t NeighborInfoModule::collectNeighborInfo(meshtastic_NeighborInfo *neighb
             neighborInfo->neighbors_count++;
         }
     }
-    printNodeDBNodes("DBSTATE");
-    printNodeDBNeighbors("NEIGHBORS");
-    printNodeDBSelection("COLLECTED", neighborInfo);
+    printNodeDBNeighbors();
     return neighborInfo->neighbors_count;
 }
 
@@ -218,20 +167,13 @@ bool NeighborInfoModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp,
 /*
 Copy the content of a current NeighborInfo packet into a new one and update the last_sent_by_id to our NodeNum
 */
-void NeighborInfoModule::updateLastSentById(meshtastic_MeshPacket *p)
+void NeighborInfoModule::alterReceivedProtobuf(meshtastic_MeshPacket &p, meshtastic_NeighborInfo *n)
 {
-    auto &incoming = p->decoded;
-    meshtastic_NeighborInfo scratch;
-    meshtastic_NeighborInfo *updated = NULL;
-    memset(&scratch, 0, sizeof(scratch));
-    pb_decode_from_bytes(incoming.payload.bytes, incoming.payload.size, &meshtastic_NeighborInfo_msg, &scratch);
-    updated = &scratch;
-
-    updated->last_sent_by_id = nodeDB->getNodeNum();
+    n->last_sent_by_id = nodeDB->getNodeNum();
 
     // Set updated last_sent_by_id to the payload of the to be flooded packet
-    p->decoded.payload.size =
-        pb_encode_to_bytes(p->decoded.payload.bytes, sizeof(p->decoded.payload.bytes), &meshtastic_NeighborInfo_msg, updated);
+    p.decoded.payload.size =
+        pb_encode_to_bytes(p.decoded.payload.bytes, sizeof(p.decoded.payload.bytes), &meshtastic_NeighborInfo_msg, n);
 }
 
 void NeighborInfoModule::resetNeighbors()

src/modules/NeighborInfoModule.h

@@ -20,9 +20,6 @@ class NeighborInfoModule : public ProtobufModule<meshtastic_NeighborInfo>, priva
 
     bool saveProtoForModule();
 
-    // Let FloodingRouter call updateLastSentById upon rebroadcasting a NeighborInfo packet
-    friend class FloodingRouter;
-
   protected:
     // Note: this holds our local info.
     meshtastic_NeighborInfo neighborState;
@@ -68,7 +65,7 @@ class NeighborInfoModule : public ProtobufModule<meshtastic_NeighborInfo>, priva
     void updateNeighbors(const meshtastic_MeshPacket &mp, const meshtastic_NeighborInfo *np);
 
     /* update a NeighborInfo packet with our NodeNum as last_sent_by_id */
-    void updateLastSentById(meshtastic_MeshPacket *p);
+    void alterReceivedProtobuf(meshtastic_MeshPacket &p, meshtastic_NeighborInfo *n) override;
 
     void loadProtoForModule();
 
@@ -81,8 +78,6 @@ class NeighborInfoModule : public ProtobufModule<meshtastic_NeighborInfo>, priva
 
     /* These are for debugging only */
     void printNeighborInfo(const char *header, const meshtastic_NeighborInfo *np);
-    void printNodeDBNodes(const char *header);
-    void printNodeDBNeighbors(const char *header);
-    void printNodeDBSelection(const char *header, const meshtastic_NeighborInfo *np);
+    void printNodeDBNeighbors();
 };
 extern NeighborInfoModule *neighborInfoModule;

src/modules/TraceRouteModule.cpp

@@ -1,5 +1,4 @@
 #include "TraceRouteModule.h"
-#include "FloodingRouter.h"
 #include "MeshService.h"
 
 TraceRouteModule *traceRouteModule;
@@ -14,23 +13,17 @@ bool TraceRouteModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, m
     return false; // let it be handled by RoutingModule
 }
 
-void TraceRouteModule::updateRoute(meshtastic_MeshPacket *p)
+void TraceRouteModule::alterReceivedProtobuf(meshtastic_MeshPacket &p, meshtastic_RouteDiscovery *r)
 {
-    auto &incoming = p->decoded;
-    // Only append an ID for the request (one way)
-    if (!incoming.request_id) {
-        meshtastic_RouteDiscovery scratch;
-        meshtastic_RouteDiscovery *updated = NULL;
-        memset(&scratch, 0, sizeof(scratch));
-        pb_decode_from_bytes(incoming.payload.bytes, incoming.payload.size, &meshtastic_RouteDiscovery_msg, &scratch);
-        updated = &scratch;
-
-        appendMyID(updated);
-        printRoute(updated, p->from, NODENUM_BROADCAST);
+    auto &incoming = p.decoded;
+    // Only append an ID for the request (one way) and if we are not the destination (the reply will have our NodeNum already)
+    if (!incoming.request_id && p.to != nodeDB->getNodeNum()) {
+        appendMyID(r);
+        printRoute(r, p.from, NODENUM_BROADCAST);
 
         // Set updated route to the payload of the to be flooded packet
-        p->decoded.payload.size = pb_encode_to_bytes(p->decoded.payload.bytes, sizeof(p->decoded.payload.bytes),
-                                                     &meshtastic_RouteDiscovery_msg, updated);
+        p.decoded.payload.size =
+            pb_encode_to_bytes(p.decoded.payload.bytes, sizeof(p.decoded.payload.bytes), &meshtastic_RouteDiscovery_msg, r);
     }
 }
 
@@ -85,4 +78,5 @@ TraceRouteModule::TraceRouteModule()
     : ProtobufModule("traceroute", meshtastic_PortNum_TRACEROUTE_APP, &meshtastic_RouteDiscovery_msg)
 {
     ourPortNum = meshtastic_PortNum_TRACEROUTE_APP;
+    isPromiscuous = true; // We need to update the route even if it is not destined to us
 }

src/modules/TraceRouteModule.h

@@ -9,17 +9,14 @@ class TraceRouteModule : public ProtobufModule<meshtastic_RouteDiscovery>
   public:
     TraceRouteModule();
 
-    // Let FloodingRouter call updateRoute upon rebroadcasting a TraceRoute request
-    friend class FloodingRouter;
-
   protected:
     bool handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshtastic_RouteDiscovery *r) override;
 
     virtual meshtastic_MeshPacket *allocReply() override;
 
-    /* Call before rebroadcasting a RouteDiscovery payload in order to update
+    /* Called before rebroadcasting a RouteDiscovery payload in order to update
        the route array containing the IDs of nodes this packet went through */
-    void updateRoute(meshtastic_MeshPacket *p);
+    void alterReceivedProtobuf(meshtastic_MeshPacket &p, meshtastic_RouteDiscovery *r) override;
 
   private:
     // Call to add your ID to the route array of a RouteDiscovery message