Fix replies dropped when flood.max.unscoped is low

examples/simple_repeater/MyMesh.cpp

@@ -411,24 +411,31 @@ bool MyMesh::isLooped(const mesh::Packet* packet, const uint8_t max_counters[])
 }
 
 void MyMesh::sendFloodReply(mesh::Packet* packet, unsigned long delay_millis, uint8_t path_hash_size) {
-  if (recv_pkt_region && !recv_pkt_region->isWildcard()) {  // if _request_ packet scope is known, send reply with same scope
-    TransportKey scope;
-    if (region_map.getTransportKeysFor(*recv_pkt_region, &scope, 1) > 0) {
-      sendFloodScoped(scope, packet, delay_millis, path_hash_size);
-    } else {
+  TransportKey req_scope;
+  bool is_wildcard = recv_pkt_region != NULL && recv_pkt_region->isWildcard();
+  bool req_scope_known = recv_pkt_region != NULL && !is_wildcard
+                      && region_map.getTransportKeysFor(*recv_pkt_region, &req_scope, 1) > 0;
+
+  switch (mesh::chooseReplyScope(req_scope_known, is_wildcard, !default_scope.isNull())) {
+    case mesh::REPLY_SCOPE_REQUEST:
+      sendFloodScoped(req_scope, packet, delay_millis, path_hash_size);   // reply with same scope as request
+      break;
+    case mesh::REPLY_SCOPE_DEFAULT:
+      // requester's scope is unknown: DIRECT request (no transport codes), or code matched no Region.
+      // un-scoped would be dropped at hop 0 by repeaters running flood.max.unscoped=0
+      sendFloodScoped(default_scope, packet, delay_millis, path_hash_size);
+      break;
+    case mesh::REPLY_SCOPE_NONE:
       sendFlood(packet, delay_millis, path_hash_size);  // send un-scoped
-    }
-  } else {
-    sendFlood(packet, delay_millis, path_hash_size);  // send un-scoped
+      break;
   }
 }
 
 bool MyMesh::allowPacketForward(const mesh::Packet *packet) {
   if (_prefs.disable_fwd) return false;
-  if (packet->isRouteFlood()) {
-    if (packet->getPathHashCount() >= _prefs.flood_max) return false;
-    if (packet->getRouteType() == ROUTE_TYPE_FLOOD && packet->getPathHashCount() >= _prefs.flood_max_unscoped) return false;
-    if (packet->getPayloadType() == PAYLOAD_TYPE_ADVERT && packet->getPathHashCount() >= _prefs.flood_max_advert) return false;
+  if (packet->isRouteFlood()
+      && mesh::isFloodHopLimitExceeded(packet, _prefs.flood_max, _prefs.flood_max_unscoped, _prefs.flood_max_advert)) {
+    return false;
   }
   if (packet->isRouteFlood() && recv_pkt_region == NULL) {
     MESH_DEBUG_PRINTLN("allowPacketForward: unknown transport code, or wildcard not allowed for FLOOD packet");
@@ -586,17 +593,29 @@ void MyMesh::onAnonDataRecv(mesh::Packet *packet, const uint8_t *secret, const m
 
     if (reply_len == 0) return;   // invalid request
 
-    if (packet->isRouteFlood()) {
+    // a DIRECT login can reply via the stored out_path, as onPeerDataRecv() does for REQ
+    ClientInfo* client = acl.getClient(sender.pub_key, PUB_KEY_SIZE);
+    bool have_out_path = client != NULL && client->out_path_len != OUT_PATH_UNKNOWN;
+
+    auto route = mesh::chooseReplyRoute(packet->isRouteFlood(), reply_path_len != 0xFF, have_out_path);
+
+    if (route == mesh::REPLY_ROUTE_PATH_RETURN) {
       // let this sender know path TO here, so they can use sendDirect(), and ALSO encode the response
       mesh::Packet* path = createPathReturn(sender, secret, packet->path, packet->path_len,
                                             PAYLOAD_TYPE_RESPONSE, reply_data, reply_len);
       if (path) sendFloodReply(path, SERVER_RESPONSE_DELAY, packet->getPathHashSize());
-    } else if (reply_path_len == 0xFF) {
-      mesh::Packet* reply = createDatagram(PAYLOAD_TYPE_RESPONSE, sender, secret, reply_data, reply_len);
-      if (reply) sendFloodReply(reply, SERVER_RESPONSE_DELAY, packet->getPathHashSize());
+      return;
+    }
+
+    mesh::Packet* reply = createDatagram(PAYLOAD_TYPE_RESPONSE, sender, secret, reply_data, reply_len);
+    if (reply == NULL) return;
+
+    if (route == mesh::REPLY_ROUTE_DIRECT_SUPPLIED) {
+      sendDirect(reply, reply_path, reply_path_len, SERVER_RESPONSE_DELAY);
+    } else if (route == mesh::REPLY_ROUTE_DIRECT_OUT_PATH) {
+      sendDirect(reply, client->out_path, client->out_path_len, SERVER_RESPONSE_DELAY);
     } else {
-      mesh::Packet* reply = createDatagram(PAYLOAD_TYPE_RESPONSE, sender, secret, reply_data, reply_len);
-      if (reply) sendDirect(reply, reply_path, reply_path_len, SERVER_RESPONSE_DELAY);
+      sendFloodReply(reply, SERVER_RESPONSE_DELAY, packet->getPathHashSize());
     }
   }
 }

examples/simple_repeater/MyMesh.h

@@ -34,6 +34,7 @@
 #include <helpers/StatsFormatHelper.h>
 #include <helpers/TxtDataHelpers.h>
 #include <helpers/RegionMap.h>
+#include <helpers/RoutingPolicy.h>
 #include "RateLimiter.h"
 
 #ifdef WITH_BRIDGE

examples/simple_room_server/MyMesh.cpp

@@ -299,10 +299,9 @@ uint32_t MyMesh::getDirectRetransmitDelay(const mesh::Packet *packet) {
 
 bool MyMesh::allowPacketForward(const mesh::Packet *packet) {
   if (_prefs.disable_fwd) return false;
-  if (packet->isRouteFlood()) {
-    if (packet->getPathHashCount() >= _prefs.flood_max) return false;
-    if (packet->getRouteType() == ROUTE_TYPE_FLOOD && packet->getPathHashCount() >= _prefs.flood_max_unscoped) return false;
-    if (packet->getPayloadType() == PAYLOAD_TYPE_ADVERT && packet->getPathHashCount() >= _prefs.flood_max_advert) return false;
+  if (packet->isRouteFlood()
+      && mesh::isFloodHopLimitExceeded(packet, _prefs.flood_max, _prefs.flood_max_unscoped, _prefs.flood_max_advert)) {
+    return false;
   }
   return true;
 }
@@ -749,15 +748,23 @@ void MyMesh::sendFloodScoped(const TransportKey& scope, mesh::Packet* pkt, uint3
 }
 
 void MyMesh::sendFloodReply(mesh::Packet* packet, unsigned long delay_millis, uint8_t path_hash_size) {
-  if (recv_pkt_region && !recv_pkt_region->isWildcard()) {  // if _request_ packet scope is known, send reply with same scope
-    TransportKey scope;
-    if (region_map.getTransportKeysFor(*recv_pkt_region, &scope, 1) > 0) {
-      sendFloodScoped(scope, packet, delay_millis, path_hash_size);
-    } else {
+  TransportKey req_scope;
+  bool is_wildcard = recv_pkt_region != NULL && recv_pkt_region->isWildcard();
+  bool req_scope_known = recv_pkt_region != NULL && !is_wildcard
+                      && region_map.getTransportKeysFor(*recv_pkt_region, &req_scope, 1) > 0;
+
+  switch (mesh::chooseReplyScope(req_scope_known, is_wildcard, !default_scope.isNull())) {
+    case mesh::REPLY_SCOPE_REQUEST:
+      sendFloodScoped(req_scope, packet, delay_millis, path_hash_size);   // reply with same scope as request
+      break;
+    case mesh::REPLY_SCOPE_DEFAULT:
+      // requester's scope is unknown: DIRECT request (no transport codes), or code matched no Region.
+      // un-scoped would be dropped at hop 0 by repeaters running flood.max.unscoped=0
+      sendFloodScoped(default_scope, packet, delay_millis, path_hash_size);
+      break;
+    case mesh::REPLY_SCOPE_NONE:
       sendFlood(packet, delay_millis, path_hash_size);   // send un-scoped
-    }
-  } else {
-    sendFlood(packet, delay_millis, path_hash_size);   // send un-scoped
+      break;
   }
 }
 

examples/simple_room_server/MyMesh.h

@@ -21,6 +21,7 @@
 #include <helpers/StatsFormatHelper.h>
 #include <helpers/ClientACL.h>
 #include <helpers/RegionMap.h>
+#include <helpers/RoutingPolicy.h>
 #include <RTClib.h>
 #include <target.h>
 

src/helpers/RoutingPolicy.h

@@ -0,0 +1,68 @@
+#pragma once
+
+#include <Packet.h>
+
+namespace mesh {
+
+/**
+ * \brief  Test a flood packet against the configured hop limits.
+ * \param  packet  inbound flood packet (caller has already checked isRouteFlood())
+ * \param  flood_max            max hops for any flood packet
+ * \param  flood_max_unscoped   max hops for ROUTE_TYPE_FLOOD (ie. un-scoped) packets
+ * \param  flood_max_advert     max hops for ADVERT packets
+ * \returns  true if the packet has exceeded a limit, and must not be forwarded
+ */
+inline bool isFloodHopLimitExceeded(const Packet* packet, uint8_t flood_max,
+                                    uint8_t flood_max_unscoped, uint8_t flood_max_advert) {
+  uint8_t hops = packet->getPathHashCount();
+  if (hops >= flood_max) return true;
+  if (packet->getRouteType() == ROUTE_TYPE_FLOOD && hops >= flood_max_unscoped) return true;
+  if (packet->getPayloadType() == PAYLOAD_TYPE_ADVERT && hops >= flood_max_advert) return true;
+  return false;
+}
+
+/**
+ * \brief  How a server routes a reply back to the requesting client.
+ */
+enum ReplyRoute : uint8_t {
+  REPLY_ROUTE_PATH_RETURN,       // request arrived by flood: reply with a PATH return, flooded back
+  REPLY_ROUTE_DIRECT_SUPPLIED,   // reply DIRECT, along the return path supplied in the request
+  REPLY_ROUTE_DIRECT_OUT_PATH,   // reply DIRECT, along the out_path already stored for this client
+  REPLY_ROUTE_FLOOD,             // no return path known: flood the reply
+};
+
+/**
+ * \param  inbound_is_flood    the request arrived as a flood packet
+ * \param  have_supplied_path  the request payload carried an explicit reply path
+ * \param  have_out_path       this server already has a stored out_path for the client
+ */
+inline ReplyRoute chooseReplyRoute(bool inbound_is_flood, bool have_supplied_path, bool have_out_path) {
+  if (inbound_is_flood) return REPLY_ROUTE_PATH_RETURN;
+  if (have_supplied_path) return REPLY_ROUTE_DIRECT_SUPPLIED;
+  if (have_out_path) return REPLY_ROUTE_DIRECT_OUT_PATH;
+  return REPLY_ROUTE_FLOOD;
+}
+
+/**
+ * \brief  Which transport scope a flooded reply should be sent with.
+ */
+enum ReplyScope : uint8_t {
+  REPLY_SCOPE_REQUEST,   // re-use the scope the request arrived on
+  REPLY_SCOPE_DEFAULT,   // fall back to this node's default region scope
+  REPLY_SCOPE_NONE,      // send un-scoped (ROUTE_TYPE_FLOOD)
+};
+
+/**
+ * \param  request_scope_known         request arrived scoped, and we resolved its Region's key
+ * \param  request_was_unscoped_flood  request arrived as an un-scoped flood
+ * \param  default_scope_known         this node has a default Region with a usable transport key
+ */
+inline ReplyScope chooseReplyScope(bool request_scope_known, bool request_was_unscoped_flood,
+                                   bool default_scope_known) {
+  if (request_scope_known) return REPLY_SCOPE_REQUEST;
+  if (request_was_unscoped_flood) return REPLY_SCOPE_NONE;  // requester chose un-scoped, so mirror it
+  if (default_scope_known) return REPLY_SCOPE_DEFAULT;      // scope unknowable: DIRECT, or unresolved Region
+  return REPLY_SCOPE_NONE;
+}
+
+}

test/test_routing_policy/test_routing_policy.cpp

@@ -0,0 +1,110 @@
+#include <gtest/gtest.h>
+#include "helpers/RoutingPolicy.h"
+
+using namespace mesh;
+
+static Packet makeFlood(uint8_t route_type, uint8_t payload_type, uint8_t hops) {
+    Packet p;
+    p.header = route_type | (payload_type << PH_TYPE_SHIFT);
+    p.setPathHashSizeAndCount(1, hops);
+    p.payload_len = 1;
+    return p;
+}
+
+TEST(FloodHopLimit, UnscopedFloodIsDroppedAtFirstHopWhenMaxUnscopedIsZero) {
+    auto pkt = makeFlood(ROUTE_TYPE_FLOOD, PAYLOAD_TYPE_RESPONSE, 0);
+    EXPECT_TRUE(isFloodHopLimitExceeded(&pkt, 64, 0, 8));
+}
+
+TEST(FloodHopLimit, ScopedFloodIsForwardedWhenMaxUnscopedIsZero) {
+    for (uint8_t hops = 0; hops < 4; hops++) {
+        auto pkt = makeFlood(ROUTE_TYPE_TRANSPORT_FLOOD, PAYLOAD_TYPE_RESPONSE, hops);
+        EXPECT_FALSE(isFloodHopLimitExceeded(&pkt, 64, 0, 8)) << "hops=" << (int)hops;
+    }
+}
+
+TEST(FloodHopLimit, UnscopedFloodSurvivesUpToMaxUnscopedHops) {
+    // matches the reported workaround: raising flood.max.unscoped to the expected hop count
+    auto ok = makeFlood(ROUTE_TYPE_FLOOD, PAYLOAD_TYPE_RESPONSE, 2);
+    EXPECT_FALSE(isFloodHopLimitExceeded(&ok, 64, 3, 8));
+
+    auto too_far = makeFlood(ROUTE_TYPE_FLOOD, PAYLOAD_TYPE_RESPONSE, 3);
+    EXPECT_TRUE(isFloodHopLimitExceeded(&too_far, 64, 3, 8));
+}
+
+TEST(FloodHopLimit, ScopedFloodStillHonoursFloodMaxAndAdvertMax) {
+    auto beyond_max = makeFlood(ROUTE_TYPE_TRANSPORT_FLOOD, PAYLOAD_TYPE_RESPONSE, 5);
+    EXPECT_TRUE(isFloodHopLimitExceeded(&beyond_max, 5, 64, 8));
+
+    auto advert = makeFlood(ROUTE_TYPE_TRANSPORT_FLOOD, PAYLOAD_TYPE_ADVERT, 8);
+    EXPECT_TRUE(isFloodHopLimitExceeded(&advert, 64, 64, 8));
+}
+
+// flood.max.unscoped=0 hits adverts too, well before flood_max_advert applies: a node
+// still advertising un-scoped is invisible past its immediate neighbours
+TEST(FloodHopLimit, UnscopedAdvertIsAlsoDroppedAtHopZero) {
+    auto advert = makeFlood(ROUTE_TYPE_FLOOD, PAYLOAD_TYPE_ADVERT, 0);
+    EXPECT_TRUE(isFloodHopLimitExceeded(&advert, 64, 0, 8));
+
+    auto scoped = makeFlood(ROUTE_TYPE_TRANSPORT_FLOOD, PAYLOAD_TYPE_ADVERT, 0);
+    EXPECT_FALSE(isFloodHopLimitExceeded(&scoped, 64, 0, 8));
+}
+
+TEST(ReplyRoute, FloodRequestGetsAPathReturn) {
+    EXPECT_EQ(REPLY_ROUTE_PATH_RETURN,
+              chooseReplyRoute(true, false, false));
+    EXPECT_EQ(REPLY_ROUTE_PATH_RETURN,
+              chooseReplyRoute(true, false, true));
+}
+
+TEST(ReplyRoute, DirectRequestWithSuppliedPathRepliesDirect) {
+    EXPECT_EQ(REPLY_ROUTE_DIRECT_SUPPLIED, chooseReplyRoute(false, true, false));
+}
+
+// the reported bug: a DIRECT login (app already has a path) was answered by flooding, even
+// with an out_path stored. Under flood.max.unscoped=0 that reply never arrives.
+TEST(ReplyRoute, DirectRequestWithKnownOutPathRepliesDirect) {
+    EXPECT_EQ(REPLY_ROUTE_DIRECT_OUT_PATH,
+              chooseReplyRoute(false, false, true));
+}
+
+TEST(ReplyRoute, SuppliedPathWinsOverStoredOutPath) {
+    EXPECT_EQ(REPLY_ROUTE_DIRECT_SUPPLIED, chooseReplyRoute(false, true, true));
+}
+
+TEST(ReplyRoute, DirectRequestWithNoReturnPathFallsBackToFlood) {
+    EXPECT_EQ(REPLY_ROUTE_FLOOD, chooseReplyRoute(false, false, false));
+}
+
+TEST(ReplyScope, MirrorsTheRequestScopeWhenKnown) {
+    EXPECT_EQ(REPLY_SCOPE_REQUEST, chooseReplyScope(true,
+                                                    false,
+                                                    false));
+    EXPECT_EQ(REPLY_SCOPE_REQUEST, chooseReplyScope(true, false, true));
+}
+
+// un-scoped is itself a known scope, so mirror it. Replying scoped would change a path that
+// works today, and repeaters not holding our default Region would drop it anyway.
+TEST(ReplyScope, RepliesUnscopedToAnUnscopedFloodEvenWhenADefaultScopeExists) {
+    EXPECT_EQ(REPLY_SCOPE_NONE, chooseReplyScope(false,
+                                                 true,
+                                                 true));
+}
+
+// second half of the bug: a DIRECT request carries no transport codes, so recv_pkt_region is
+// always NULL. Un-scoped is dropped under flood.max.unscoped=0, and floods the mesh otherwise.
+TEST(ReplyScope, FallsBackToDefaultScopeWhenRequestScopeUnknown) {
+    EXPECT_EQ(REPLY_SCOPE_DEFAULT, chooseReplyScope(false,
+                                                    false,
+                                                    true));
+}
+
+TEST(ReplyScope, SendsUnscopedOnlyWhenNoScopeIsAvailableAtAll) {
+    EXPECT_EQ(REPLY_SCOPE_NONE, chooseReplyScope(false, false, false));
+    EXPECT_EQ(REPLY_SCOPE_NONE, chooseReplyScope(false, true, false));
+}
+
+int main(int argc, char** argv) {
+    ::testing::InitGoogleTest(&argc, argv);
+    return RUN_ALL_TESTS();
+}