fix: improve acknowledged unicast retry reliability (#11320)

* Improve acknowledged unicast retry reliability

* Fix merged next-hop routing tests

---------

Co-authored-by: Ben Meadors <benmmeadors@gmail.com>

src/mesh/NextHopRouter.cpp

@@ -57,12 +57,18 @@ PendingPacket::PendingPacket(meshtastic_MeshPacket *p, uint8_t numRetransmission
 {
     packet = p;
     this->numRetransmissions = numRetransmissions - 1; // We subtract one, because we assume the user just did the first send
+    this->initialNumRetransmissions = this->numRetransmissions;
 }
 
 /**
  * Send a packet
  */
 ErrorCode NextHopRouter::send(meshtastic_MeshPacket *p)
+{
+    return sendWithNextHop(p, true);
+}
+
+ErrorCode NextHopRouter::sendWithNextHop(meshtastic_MeshPacket *p, bool trackRetransmission)
 {
     // Add any messages _we_ send to the seen message list (so we will ignore all retransmissions we see)
     p->relay_node = nodeDB->getLastByteOfNodeNum(getNodeNum()); // First set the relayer to us
@@ -73,7 +79,8 @@ ErrorCode NextHopRouter::send(meshtastic_MeshPacket *p)
 
     // If it's from us, ReliableRouter already handles retransmissions if want_ack is set. If a next hop is set and hop limit is
     // not 0 or want_ack is set, start retransmissions
-    if ((!isFromUs(p) || !p->want_ack) && p->next_hop != NO_NEXT_HOP_PREFERENCE && (p->hop_limit > 0 || p->want_ack)) {
+    if (trackRetransmission && (!isFromUs(p) || !p->want_ack) && p->next_hop != NO_NEXT_HOP_PREFERENCE &&
+        (p->hop_limit > 0 || p->want_ack)) {
         if (auto *copy = packetPool.allocCopy(*p))
             startRetransmission(copy); // start retransmission for relayed packet
     }
@@ -362,7 +369,7 @@ bool NextHopRouter::stopRetransmission(GlobalPacketId key)
         auto p = old->packet;
         /* Only when we already transmitted a packet via LoRa, we will cancel the packet in the Tx queue
           to avoid canceling a transmission if it was ACKed super fast via MQTT */
-        if (old->numRetransmissions < NUM_RELIABLE_RETX - 1) {
+        if (old->numRetransmissions < old->initialNumRetransmissions) {
             // We only cancel it if we are the original sender or if we're not a router(_late)
             if (isFromUs(p) || roleAllowsCancelingFromTxQueue(p)) {
                 // remove the 'original' (identified by originator and packet->id) from the txqueue and free it
@@ -475,13 +482,13 @@ int32_t NextHopRouter::doRetransmissions()
                             }
                         } else {
                             if (auto *copy = packetPool.allocCopy(*p.packet)) {
-                                if (NextHopRouter::send(copy) == ERRNO_SHOULD_RELEASE)
+                                if (sendWithNextHop(copy, false) == ERRNO_SHOULD_RELEASE)
                                     packetPool.release(copy);
                             }
                         }
 #else
                         if (auto *copy = packetPool.allocCopy(*p.packet)) {
-                            if (NextHopRouter::send(copy) == ERRNO_SHOULD_RELEASE)
+                            if (sendWithNextHop(copy, false) == ERRNO_SHOULD_RELEASE)
                                 packetPool.release(copy);
                         }
 #endif

src/mesh/NextHopRouter.h

@@ -39,6 +39,9 @@ struct PendingPacket {
     /** Starts at NUM_RETRANSMISSIONS -1 and counts down.  Once zero it will be removed from the list */
     uint8_t numRetransmissions = 0;
 
+    /** Initial remaining retry count, used to detect whether a retry has fired. */
+    uint8_t initialNumRetransmissions = 0;
+
     PendingPacket() {}
     explicit PendingPacket(meshtastic_MeshPacket *p, uint8_t numRetransmissions);
 };
@@ -77,8 +80,8 @@ class GlobalPacketIdHashFunction
   Namely, in the PacketHistory, we keep track of (up to 3) relayers of a packet. When the ACK is delivered back to us via a node
   that also relayed the original packet, we use that node as next hop for the destination from then on. This makes sure that only
   when there’s a two-way connection, we assign a next hop. Both the ReliableRouter and NextHopRouter will do retransmissions (the
-  NextHopRouter only 1 time). For the final retry, if no one actually relayed the packet, it will reset the next hop in order to
-  fall back to the FloodingRouter again. Note that thus also intermediate hops will do a single retransmission if the intended
+  NextHopRouter only a small number of times). For the final retry, if no one actually relayed the packet, it will reset the next
+  hop in order to fall back to the FloodingRouter again. Intermediate hops also do bounded retransmissions if the intended
   next-hop didn’t relay, in order to fix changes in the middle of the route.
 */
 class NextHopRouter : public FloodingRouter
@@ -109,10 +112,12 @@ class NextHopRouter : public FloodingRouter
         return min(d, r);
     }
 
-    // The number of retransmissions intermediate nodes will do (actually 1 less than this)
-    constexpr static uint8_t NUM_INTERMEDIATE_RETX = 2;
-    // The number of retransmissions the original sender will do
+    // Total attempts for directed hop-level delivery, including the initial send.
+    constexpr static uint8_t NUM_INTERMEDIATE_RETX = 3;
+    // Existing reliable broadcast budget, including the initial send.
     constexpr static uint8_t NUM_RELIABLE_RETX = 3;
+    // Total attempts for acknowledged unicast from the originating node.
+    constexpr static uint8_t NUM_RELIABLE_UNICAST_ATTEMPTS = 5;
 
     // M3: bounded RAM route-health table (reuse-oldest eviction, like PacketHistory)
     constexpr static uint8_t ROUTE_HEALTH_MAX = 32;              // ~12B/slot -> ~384B
@@ -155,6 +160,8 @@ class NextHopRouter : public FloodingRouter
      */
     PendingPacket *startRetransmission(meshtastic_MeshPacket *p, uint8_t numReTx = NUM_INTERMEDIATE_RETX);
 
+    ErrorCode sendWithNextHop(meshtastic_MeshPacket *p, bool trackRetransmission);
+
     // Return true if we're allowed to cancel a packet in the txQueue (so we may never transmit it even once)
     bool roleAllowsCancelingFromTxQueue(const meshtastic_MeshPacket *p);
 

src/mesh/ReliableRouter.cpp

@@ -30,8 +30,10 @@ ErrorCode ReliableRouter::send(meshtastic_MeshPacket *p)
         auto copy = packetPool.allocCopy(*p);
         DEBUG_HEAP_AFTER("ReliableRouter::send", copy);
 
-        if (copy)
-            startRetransmission(copy, NUM_RELIABLE_RETX);
+        if (copy) {
+            const uint8_t totalAttempts = isBroadcast(p->to) ? NUM_RELIABLE_RETX : NUM_RELIABLE_UNICAST_ATTEMPTS;
+            startRetransmission(copy, totalAttempts);
+        }
     }
 
     /* If we have pending retransmissions, add the airtime of this packet to it, because during that time we cannot receive an

test/test_nexthop_routing/test_main.cpp

@@ -106,6 +106,44 @@ class NextHopRouterTestShim : public NextHopRouter
     using NextHopRouter::relayOpaquePacket;
     using Router::shouldDecrementHopLimit; // protected in Router
 
+    PendingPacket *trackForTest(const meshtastic_MeshPacket &packet, uint8_t totalAttempts)
+    {
+        auto *copy = packetPool.allocCopy(packet);
+        TEST_ASSERT_NOT_NULL(copy);
+        return startRetransmission(copy, totalAttempts);
+    }
+
+    PendingPacket *trackWithDefaultBudgetForTest(const meshtastic_MeshPacket &packet)
+    {
+        auto *copy = packetPool.allocCopy(packet);
+        TEST_ASSERT_NOT_NULL(copy);
+        return startRetransmission(copy);
+    }
+
+    bool stopForTest(NodeNum from, PacketId id) { return stopRetransmission(from, id); }
+
+    meshtastic_MeshPacket *pendingPacketForTest(NodeNum from, PacketId id)
+    {
+        PendingPacket *entry = findPendingPacket(from, id);
+        return entry ? entry->packet : nullptr;
+    }
+
+    void fireNextRetryForTest(NodeNum from, PacketId id)
+    {
+        PendingPacket *entry = findPendingPacket(from, id);
+        TEST_ASSERT_NOT_NULL(entry);
+        entry->nextTxMsec = 0;
+        doRetransmissions();
+    }
+
+    void markOneRetryFiredForTest(NodeNum from, PacketId id)
+    {
+        PendingPacket *entry = findPendingPacket(from, id);
+        TEST_ASSERT_NOT_NULL(entry);
+        TEST_ASSERT_GREATER_THAN_UINT8(0, entry->numRetransmissions);
+        --entry->numRetransmissions;
+    }
+
     bool filterViaFlooding(const meshtastic_MeshPacket *p) { return FloodingRouter::shouldFilterReceived(p); }
     bool filterViaNextHop(const meshtastic_MeshPacket *p) { return NextHopRouter::shouldFilterReceived(p); }
 
@@ -132,6 +170,7 @@ class MockRadioInterface : public RadioInterface
         sendCount++;
         lastHopLimit = p->hop_limit;
         lastHopStart = p->hop_start;
+        sentNextHops.push_back(p->next_hop);
         if (declineAll || p->to == NODENUM_BROADCAST_NO_LORA)
             return ERRNO_SHOULD_RELEASE;
 
@@ -146,10 +185,18 @@ class MockRadioInterface : public RadioInterface
         return 0;
     }
 
+    bool cancelSending(NodeNum, PacketId) override
+    {
+        cancelCount++;
+        return true;
+    }
+
     int sendCount = 0;
+    uint32_t cancelCount = 0;
     bool declineAll = false;
     uint8_t lastHopLimit = 0;
     uint8_t lastHopStart = 0;
+    std::vector<uint8_t> sentNextHops;
 };
 
 class CaptureRadioInterface : public RadioInterface
@@ -773,6 +820,92 @@ void test_reliableAckStopsNormalPendingTransmission(void)
     TEST_ASSERT_EQUAL_UINT32(0, reliableShim->pendingCount());
 }
 
+void test_pending_does_not_cancel_radio_queue_before_first_retry(void)
+{
+    MockRadioInterface *mockIface = installMockIface();
+    meshtastic_MeshPacket p = makeRebroadcastCandidate(0x33333333);
+    p.from = kLocalNode;
+    p.id = 0x51000001;
+    shim->trackForTest(p, 5);
+
+    TEST_ASSERT_TRUE(shim->stopForTest(kLocalNode, p.id));
+    TEST_ASSERT_EQUAL_UINT32(0, mockIface->cancelCount);
+}
+
+void test_pending_cancels_radio_queue_after_first_retry_for_any_budget(void)
+{
+    MockRadioInterface *mockIface = installMockIface();
+    meshtastic_MeshPacket p = makeRebroadcastCandidate(0x33333333);
+    p.from = kLocalNode;
+    p.id = 0x51000002;
+    shim->trackForTest(p, 5);
+    shim->markOneRetryFiredForTest(kLocalNode, p.id);
+
+    TEST_ASSERT_TRUE(shim->stopForTest(kLocalNode, p.id));
+    TEST_ASSERT_EQUAL_UINT32(1, mockIface->cancelCount);
+}
+
+void test_directed_hop_tracks_three_total_attempts(void)
+{
+    installMockIface();
+    meshtastic_MeshPacket p = makeRebroadcastCandidate(0x33333333);
+    p.id = 0x51530003;
+
+    PendingPacket *entry = shim->trackWithDefaultBudgetForTest(p);
+    TEST_ASSERT_NOT_NULL(entry);
+    TEST_ASSERT_EQUAL_UINT8(3, entry->initialNumRetransmissions + 1);
+    TEST_ASSERT_TRUE(shim->stopForTest(p.from, p.id));
+}
+
+void test_intermediate_three_attempts_preserve_record_and_flood_last(void)
+{
+    MockRadioInterface *mockIface = installMockIface();
+    constexpr NodeNum dest = 0x33333333;
+    mockNodeDB->addNode(dest, 2, true, 60, meshtastic_Config_DeviceConfig_Role_CLIENT, false, false, 0xAB);
+    mockNodeDB->addNode(0x000007AB, 0, true, 60);
+
+    meshtastic_MeshPacket p = makeRebroadcastCandidate(dest);
+    p.id = 0x51530004;
+    p.next_hop = 0xAB;
+    PendingPacket *entry = shim->trackWithDefaultBudgetForTest(p);
+    TEST_ASSERT_NOT_NULL(entry);
+    meshtastic_MeshPacket *trackedPacket = entry->packet;
+
+    shim->fireNextRetryForTest(p.from, p.id);
+    TEST_ASSERT_EQUAL_UINT32(1, mockIface->sentNextHops.size());
+#if NEXTHOP_EARLY_FLOOD_ON_UNVERIFIED
+    TEST_ASSERT_EQUAL_HEX8(NO_NEXT_HOP_PREFERENCE, mockIface->sentNextHops[0]);
+#else
+    TEST_ASSERT_EQUAL_HEX8(0xAB, mockIface->sentNextHops[0]);
+#endif
+    TEST_ASSERT_EQUAL_PTR(trackedPacket, shim->pendingPacketForTest(p.from, p.id));
+
+    shim->fireNextRetryForTest(p.from, p.id);
+    TEST_ASSERT_EQUAL_UINT32(2, mockIface->sentNextHops.size());
+    TEST_ASSERT_EQUAL_HEX8(NO_NEXT_HOP_PREFERENCE, mockIface->sentNextHops[1]);
+    TEST_ASSERT_TRUE(shim->stopForTest(p.from, p.id));
+}
+
+void test_early_flood_preserves_fresh_verified_route(void)
+{
+    MockRadioInterface *mockIface = installMockIface();
+    constexpr NodeNum dest = 0x33333333;
+    mockNodeDB->addNode(dest, 2, true, 60, meshtastic_Config_DeviceConfig_Role_CLIENT, false, false, 0xAB);
+    mockNodeDB->addNode(0x000007AB, 0, true, 60);
+    shim->noteRouteLearned(dest, 0xAB, millis());
+
+    meshtastic_MeshPacket p = makeRebroadcastCandidate(dest);
+    p.id = 0x51530005;
+    p.next_hop = 0xAB;
+    TEST_ASSERT_NOT_NULL(shim->trackWithDefaultBudgetForTest(p));
+
+    shim->fireNextRetryForTest(p.from, p.id);
+    TEST_ASSERT_EQUAL_UINT32(1, mockIface->sentNextHops.size());
+    TEST_ASSERT_EQUAL_HEX8(0xAB, mockIface->sentNextHops[0]);
+    TEST_ASSERT_TRUE(shim->stopForTest(p.from, p.id));
+}
+
+// Control: proves the NO_LORA case below turns on the `to` field alone.
 void test_rebroadcast_normal_broadcast_is_relayed(void)
 {
     MockRadioInterface *mockIface = installMockIface();
@@ -846,6 +979,8 @@ void test_event_mode_hop_behavior(void)
 void setup()
 {
     initializeTestEnvironment();
+    AirTime testAirTime;
+    airTime = &testAirTime;
     UNITY_BEGIN();
 
     airTimeFixture = std::make_unique<ScopedAirTimeFixture>();
@@ -913,6 +1048,13 @@ void setup()
     RUN_TEST(test_eventPolicy_seededRetrySuppressesTxUntilGateOff);
     RUN_TEST(test_reliableAckStopsNormalPendingTransmission);
 
+    printf("\n=== pending retransmission bookkeeping ===\n");
+    RUN_TEST(test_pending_does_not_cancel_radio_queue_before_first_retry);
+    RUN_TEST(test_pending_cancels_radio_queue_after_first_retry_for_any_budget);
+    RUN_TEST(test_directed_hop_tracks_three_total_attempts);
+    RUN_TEST(test_intermediate_three_attempts_preserve_record_and_flood_last);
+    RUN_TEST(test_early_flood_preserves_fresh_verified_route);
+
     printf("\n=== rebroadcast of NODENUM_BROADCAST_NO_LORA ===\n");
     RUN_TEST(test_rebroadcast_normal_broadcast_is_relayed);
     RUN_TEST(test_rebroadcast_no_lora_broadcast_is_not_relayed);

test/test_packet_signing/test_main.cpp

@@ -174,6 +174,11 @@ class AuthPipelineRouter : public ReliableRouter
         PendingPacket *entry = findPendingPacket(from, id);
         return entry ? entry->nextTxMsec : 0;
     }
+    uint8_t pendingTotalAttempts(NodeNum from, PacketId id)
+    {
+        PendingPacket *entry = findPendingPacket(from, id);
+        return entry ? entry->initialNumRetransmissions + 1 : 0;
+    }
     size_t pendingCount() const { return pending.size(); }
     void clearPending()
     {
@@ -1548,6 +1553,26 @@ void test_C14_duty_cycle_limited_reliable_send_remains_pending(void)
     initRegion();
 }
 
+void test_C15_reliable_unicast_tracks_five_total_attempts(void)
+{
+    meshtastic_MeshPacket p = makeDecoded(LOCAL_NODE, REMOTE_NODE, meshtastic_PortNum_ROUTING_APP, SMALL_PAYLOAD);
+    p.id = 0x51530001;
+    p.want_ack = true;
+
+    TEST_ASSERT_EQUAL(ERRNO_OK, pipelineRouter->send(packetPool.allocCopy(p)));
+    TEST_ASSERT_EQUAL_UINT8(5, pipelineRouter->pendingTotalAttempts(LOCAL_NODE, p.id));
+}
+
+void test_C16_reliable_broadcast_keeps_three_total_attempts(void)
+{
+    meshtastic_MeshPacket p = makeDecoded(LOCAL_NODE, NODENUM_BROADCAST, meshtastic_PortNum_ROUTING_APP, SMALL_PAYLOAD);
+    p.id = 0x51530002;
+    p.want_ack = true;
+
+    TEST_ASSERT_EQUAL(ERRNO_OK, pipelineRouter->send(packetPool.allocCopy(p)));
+    TEST_ASSERT_EQUAL_UINT8(3, pipelineRouter->pendingTotalAttempts(LOCAL_NODE, p.id));
+}
+
 // C5: the packet survives (C4) but the identity claim inside it must not land - the pubkey guard
 // can't tell a signer from an impersonator replaying its (public) key. Only the write is refused.
 void test_N5_unsigned_unicast_nodeinfo_from_signer_does_not_change_name(void)
@@ -2123,6 +2148,8 @@ void setup()
     RUN_TEST(test_C12_exact_authenticated_replay_reuses_verdict_without_collision_bypass);
     RUN_TEST(test_C13_failed_initial_reliable_send_does_not_retry);
     RUN_TEST(test_C14_duty_cycle_limited_reliable_send_remains_pending);
+    RUN_TEST(test_C15_reliable_unicast_tracks_five_total_attempts);
+    RUN_TEST(test_C16_reliable_broadcast_keeps_three_total_attempts);
     printf("\n=== Group N: NodeInfoModule authentication ===\n");
     RUN_TEST(test_N1_unsigned_nodeinfo_from_signer_dropped);
     RUN_TEST(test_N2_signed_nodeinfo_from_signer_not_dropped);