From f57ee0bd71c132b836abc7c9f7575fff74ed06f6 Mon Sep 17 00:00:00 2001 From: Ben Meadors Date: Fri, 14 Aug 2026 12:15:31 -0500 Subject: [PATCH] fix(mesh): restore the implicit ACK for our own overheard PKI DMs (#11502) * fix(mesh): restore the implicit ACK for our own overheard PKI DMs A DM we originate is PKI-encrypted to the recipient, so when we overhear it being rebroadcast we cannot decrypt it. perhapsHandleReceived() classifies it DECODE_OPAQUE and returns before shouldFilterReceived() runs, which is where the implicit ACK for our own transmission is generated. The client therefore never receives the ROUTING_APP ack it renders as "Delivered to mesh" for a DM, and the message sits in "sending" until it either succeeds outright or times out as max retransmissions. The ACK only needs the packet header (from/id), not the decoded payload, so split it out of shouldFilterReceived() into perhapsGenerateImplicitAckForOwnOverheard() and also call it from the opaque short-circuit for packets that are from us. Behavior on the decodable path is unchanged. Broadcasts on a PSK channel decode normally and always reached the generator, which is why channel messages were unaffected and only DMs showed the symptom. * test: rename implicit-ack tests to avoid a trufflehog false positive The camelCase identifiers tripped trunk's trufflehog/Lob secret detector. * test: shorten one test name past trunk's Lob secret-detector pattern trufflehog's Lob rule matches test_ followed by exactly 35 word characters, which both new test names happened to hit. Unrelated to the fix. --- src/mesh/ReliableRouter.cpp | 49 ++++++++++++---------- src/mesh/ReliableRouter.h | 5 +++ src/mesh/Router.cpp | 6 +++ src/mesh/Router.h | 8 ++++ test/test_nexthop_routing/test_main.cpp | 55 +++++++++++++++++++++++++ 5 files changed, 102 insertions(+), 21 deletions(-) diff --git a/src/mesh/ReliableRouter.cpp b/src/mesh/ReliableRouter.cpp index f823232c2..4e8d2c2e9 100644 --- a/src/mesh/ReliableRouter.cpp +++ b/src/mesh/ReliableRouter.cpp @@ -53,34 +53,41 @@ ErrorCode ReliableRouter::send(meshtastic_MeshPacket *p) return result; } -bool ReliableRouter::shouldFilterReceived(const meshtastic_MeshPacket *p) +void ReliableRouter::perhapsGenerateImplicitAckForOwnOverheard(const meshtastic_MeshPacket *p) { // Note: do not use getFrom() here, because we want to ignore messages sent from phone - if (p->from == getNodeNum()) { - printPacket("Rx someone rebroadcasting for us", p); + if (p->from != getNodeNum()) + return; - // We are seeing someone rebroadcast one of our broadcast attempts. - // If this is the first time we saw this, cancel any retransmissions we have queued up and generate an internal ack for - // the original sending process. + printPacket("Rx someone rebroadcasting for us", p); - // This "optimization", does save lots of airtime. For DMs, you also get a real ACK back - // from the intended recipient. - auto key = GlobalPacketId(getFrom(p), p->id); - auto old = findPendingPacket(key); - if (old) { - LOG_DEBUG("Generate implicit ack"); - // NOTE: we do NOT check p->wantAck here because p is the INCOMING rebroadcast and that packet is not expected to be - // marked as wantAck - sendAckNak(meshtastic_Routing_Error_NONE, getFrom(p), p->id, old->packet->channel); + // We are seeing someone rebroadcast one of our transmissions. If this is the first time we saw + // this, cancel any retransmissions we have queued up and generate an internal ack for the + // original sending process. Header-only (from/id), so it works even for a packet we cannot + // decrypt - notably a PKI DM we originated, which is opaque to us when overheard. - // Only stop retransmissions if the rebroadcast came via LoRa - if (p->transport_mechanism == meshtastic_MeshPacket_TransportMechanism_TRANSPORT_LORA) { - stopRetransmission(key); - } - } else { - LOG_DEBUG("Didn't find pending packet"); + // This "optimization", does save lots of airtime. For DMs, you also get a real ACK back + // from the intended recipient. + auto key = GlobalPacketId(getFrom(p), p->id); + auto old = findPendingPacket(key); + if (old) { + LOG_DEBUG("Generate implicit ack"); + // NOTE: we do NOT check p->wantAck here because p is the INCOMING rebroadcast and that packet is not expected to be + // marked as wantAck + sendAckNak(meshtastic_Routing_Error_NONE, getFrom(p), p->id, old->packet->channel); + + // Only stop retransmissions if the rebroadcast came via LoRa + if (p->transport_mechanism == meshtastic_MeshPacket_TransportMechanism_TRANSPORT_LORA) { + stopRetransmission(key); } + } else { + LOG_DEBUG("Didn't find pending packet"); } +} + +bool ReliableRouter::shouldFilterReceived(const meshtastic_MeshPacket *p) +{ + perhapsGenerateImplicitAckForOwnOverheard(p); /* At this point we have already deleted the pending retransmission if this packet was an (implicit) ACK to it. Now for all other pending retransmissions, we have to add the airtime of this received packet to the retransmission timer, diff --git a/src/mesh/ReliableRouter.h b/src/mesh/ReliableRouter.h index 33121de6b..1dafaca80 100644 --- a/src/mesh/ReliableRouter.h +++ b/src/mesh/ReliableRouter.h @@ -32,6 +32,11 @@ class ReliableRouter : public NextHopRouter */ virtual bool shouldFilterReceived(const meshtastic_MeshPacket *p) override; + /** + * Header-only implicit ACK for our own overheard rebroadcast (also usable before decode). + */ + virtual void perhapsGenerateImplicitAckForOwnOverheard(const meshtastic_MeshPacket *p) override; + private: /** * Should this packet be ACKed with a want_ack for reliable delivery? diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index 2aa6a6c63..e4f52c779 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -1611,6 +1611,12 @@ void Router::perhapsHandleReceived(meshtastic_MeshPacket *p) return; } if (authVerdict == RoutingAuthVerdict::OPAQUE_RELAY_ONLY) { + // A packet we originated but cannot decrypt (a PKI DM we sent, overheard being rebroadcast) + // is opaque to us and would otherwise skip shouldFilterReceived entirely, so the implicit + // ACK that marks a DM "Delivered to mesh" never fires. The ACK is header-only (from/id), so + // generate it here from the still-encrypted packet before opaque relay. + if (isFromUs(p)) + perhapsGenerateImplicitAckForOwnOverheard(p); relayOpaquePacket(p); packetPool.release(p); return; diff --git a/src/mesh/Router.h b/src/mesh/Router.h index d5ea73cfe..eb1213de3 100644 --- a/src/mesh/Router.h +++ b/src/mesh/Router.h @@ -137,6 +137,14 @@ class Router : protected concurrency::OSThread, protected PacketHistory /** Relay an opaque packet without admitting it to local routing/history state. */ virtual bool relayOpaquePacket(const meshtastic_MeshPacket *) { return false; } + /** + * Generate the implicit ACK for our own transmission overheard being rebroadcast, using header + * fields only (from/id). Split out of shouldFilterReceived() so it can also run when the auth + * gate short-circuits a packet we cannot decrypt (a PKI DM we originated is opaque to us, so + * without this the client never sees "Delivered to mesh" for DMs). + */ + virtual void perhapsGenerateImplicitAckForOwnOverheard(const meshtastic_MeshPacket *) {} + /** * Determine if hop_limit should be decremented for a relay operation. * Returns false (preserve hop_limit) only if all conditions are met: diff --git a/test/test_nexthop_routing/test_main.cpp b/test/test_nexthop_routing/test_main.cpp index 4dca5b5e4..c4891056c 100644 --- a/test/test_nexthop_routing/test_main.cpp +++ b/test/test_nexthop_routing/test_main.cpp @@ -268,6 +268,8 @@ class ReliableRouterTestShim : public ReliableRouter ReliableRouter::sniffReceived(p, routing); } + void implicitAckForTest(const meshtastic_MeshPacket *p) { perhapsGenerateImplicitAckForOwnOverheard(p); } + void clearPendingForTest() { while (!pending.empty()) @@ -820,6 +822,57 @@ void test_reliableAckStopsNormalPendingTransmission(void) TEST_ASSERT_EQUAL_UINT32(0, reliableShim->pendingCount()); } +// A PKI DM we originated is encrypted to the recipient, so when we overhear it being rebroadcast we +// cannot decode it. The routing auth gate classifies it opaque and returns before +// shouldFilterReceived() runs, so the implicit ACK has to be reachable from the header alone - +// otherwise the client never sees "Delivered to mesh" for a DM. +void test_implicit_ack_for_opaque_own_packet(void) +{ + auto original = makeBehaviorPacket(meshtastic_PortNum_TEXT_MESSAGE_APP, kLocalNode, kRemoteNode, 0, /*wantAck=*/true); + reliableShim->seedRetry(original, NextHopRouter::NUM_RELIABLE_UNICAST_ATTEMPTS); + TEST_ASSERT_EQUAL_UINT32(1, reliableShim->pendingCount()); + mockRoutingModule->ackNaks.clear(); + + // The overheard copy as it actually arrives: still encrypted, nothing decoded. + meshtastic_MeshPacket overheard = meshtastic_MeshPacket_init_zero; + overheard.from = kLocalNode; + overheard.to = kRemoteNode; + overheard.id = original.id; + overheard.channel = 0; + overheard.which_payload_variant = meshtastic_MeshPacket_encrypted_tag; + overheard.encrypted.size = 32; + + reliableShim->implicitAckForTest(&overheard); + + TEST_ASSERT_EQUAL_UINT32(1, mockRoutingModule->ackNaks.size()); + const auto &ack = mockRoutingModule->ackNaks.front(); + TEST_ASSERT_EQUAL(meshtastic_Routing_Error_NONE, std::get<0>(ack)); + TEST_ASSERT_EQUAL_UINT32(kLocalNode, std::get<1>(ack)); // addressed to us -> reaches the phone + TEST_ASSERT_EQUAL_UINT32(original.id, std::get<2>(ack)); + + reliableShim->clearPendingForTest(); +} + +// Someone else's traffic must never mint an ACK, even with a colliding id. +void test_implicit_ack_ignores_foreign_pkt(void) +{ + auto original = makeBehaviorPacket(meshtastic_PortNum_TEXT_MESSAGE_APP, kLocalNode, kRemoteNode, 0, /*wantAck=*/true); + reliableShim->seedRetry(original, NextHopRouter::NUM_RELIABLE_UNICAST_ATTEMPTS); + mockRoutingModule->ackNaks.clear(); + + meshtastic_MeshPacket foreign = meshtastic_MeshPacket_init_zero; + foreign.from = kRemoteNode; + foreign.to = kLocalNode; + foreign.id = original.id; + foreign.which_payload_variant = meshtastic_MeshPacket_encrypted_tag; + foreign.encrypted.size = 32; + + reliableShim->implicitAckForTest(&foreign); + + TEST_ASSERT_EQUAL_UINT32(0, mockRoutingModule->ackNaks.size()); + reliableShim->clearPendingForTest(); +} + void test_pending_does_not_cancel_radio_queue_before_first_retry(void) { MockRadioInterface *mockIface = installMockIface(); @@ -1049,6 +1102,8 @@ void setup() RUN_TEST(test_reliableAckStopsNormalPendingTransmission); printf("\n=== pending retransmission bookkeeping ===\n"); + RUN_TEST(test_implicit_ack_for_opaque_own_packet); + RUN_TEST(test_implicit_ack_ignores_foreign_pkt); 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);