lnworker: set invoice status after htlc resolved outside of pay session

 - if fulfilled after session, set to PR_PAID
 - if failed after session, set to UNPAID

Author: ecdsa

electrum/lnworker.py

@@ -2010,7 +2010,7 @@ async def pay_invoice(
                 util.trigger_callback('payment_succeeded', self.wallet, key)
             elif self.has_unresolved_sent_htlcs(payment_hash):
                 # The invoice stays PR_INFLIGHT until the htlcs resolve.
-                self.logger.info(f"pay_invoice: htlcs are still unresolved. ")
+                self.logger.info("pay_invoice: htlcs are still unresolved.")
             else:
                 self.set_invoice_status(key, PR_UNPAID)  # allows retries
                 util.trigger_callback('payment_failed', self.wallet, key, reason)
@@ -3218,6 +3218,27 @@ def notify_upstream_peer(self, htlc_key: str) -> None:
             upstream_peer.downstream_htlc_resolved_event.set()
             upstream_peer.downstream_htlc_resolved_event.clear()
 
+    def _set_sent_payment_succeeded(self, payment_hash: bytes) -> None:
+        key = payment_hash.hex()
+        info = self.get_payment_info(payment_hash, direction=SENT)
+        if info is not None and info.status != PR_PAID:
+            self.set_invoice_status(key, PR_PAID)
+            util.trigger_callback('payment_succeeded', self.wallet, key)
+
+    def _set_sent_payment_failed(self, payment_hash: bytes) -> None:
+        key = payment_hash.hex()
+        if self.has_unresolved_sent_htlcs(payment_hash):
+            return
+        if self.get_preimage(payment_hash) and self.wallet.get_request(key) is None:
+            # if we know the preimage don't consider the payment failed (unless we pay ourselves).
+            # maybe another htlc of the same mpp got fulfilled, or we saw a htlc-success tx in the mempool
+            # before claiming a revoked htlc with a justice tx which ultimately would make LNWatcher try to fail the htlc here
+            return
+        info = self.get_payment_info(payment_hash, direction=SENT)
+        if info is not None and (info.status != PR_UNPAID or key in self.inflight_payments):
+            self.set_invoice_status(key, PR_UNPAID)
+            util.trigger_callback('payment_failed', self.wallet, key, '')
+
     def htlc_fulfilled(self, chan: Channel, payment_hash: bytes, htlc_id: int):
         """Called when an HTLC *WE proposed* becomes irrevocably fulfilled."""
         # note: this may be called several times for the same htlc
@@ -3249,15 +3270,13 @@ def htlc_fulfilled(self, chan: Channel, payment_hash: bytes, htlc_id: int):
                 paysession_active = False
             else:
                 paysession_active = True
+            if not fw_key and not paysession.is_active:
+                self._set_sent_payment_succeeded(payment_hash)
         else:
             if fw_key:
                 paysession_active = False
             else:
-                key = payment_hash.hex()
-                info = self.get_payment_info(payment_hash, direction=SENT)
-                if info is not None and info.status != PR_PAID:
-                    self.set_invoice_status(key, PR_PAID)
-                    util.trigger_callback('payment_succeeded', self.wallet, key)
+                self._set_sent_payment_succeeded(payment_hash)
 
         if fw_key:
             fw_htlcs = self.active_forwardings[fw_key]
@@ -3328,20 +3347,14 @@ def htlc_failed(
                 paysession_active = False
             else:
                 paysession_active = True
+            if not fw_key and not paysession.is_active:
+                self._set_sent_payment_failed(payment_hash)
         else:
             if fw_key:
                 paysession_active = False
             else:
                 self.logger.info(f"received unknown htlc_failed, probably from previous session (phash={payment_hash.hex()})")
-                key = payment_hash.hex()
-                invoice = self.wallet.get_invoice(key)
-                if invoice and self.get_invoice_status(invoice) != PR_UNPAID \
-                        and (self.get_preimage(payment_hash) is None or self.wallet.get_request(key) is not None):
-                    # if we know the preimage don't consider the payment failed (unless we pay ourselves).
-                    # maybe another htlc of the same mpp got fulfilled, or we saw a htlc-success tx in the mempool
-                    # before claiming a revoked htlc with a justice tx which ultimately would make LNWatcher try to fail the htlc here
-                    self.set_invoice_status(key, PR_UNPAID)
-                    util.trigger_callback('payment_failed', self.wallet, key, '')
+                self._set_sent_payment_failed(payment_hash)
 
         if fw_key:
             fw_htlcs = self.active_forwardings[fw_key]

tests/test_lnpeer.py

@@ -46,7 +46,7 @@
 from electrum.logging import console_stderr_handler, Logger
 from electrum.lnonion import OnionFailureCode, OnionRoutingFailure, OnionHopsDataSingle, OnionPacket
 from electrum.lnutil import LOCAL, REMOTE, UpdateAddHtlc, RecvMPPResolution, RevocationStore
-from electrum.invoices import PR_PAID, PR_UNPAID, Invoice
+from electrum.invoices import PR_PAID, PR_UNPAID, PR_INFLIGHT, Invoice
 from electrum.interface import GracefulDisconnect
 from electrum.fee_policy import FeeTimeEstimates, FEE_ETA_TARGETS
 from electrum.mpp_split import split_amount_normal
@@ -915,6 +915,58 @@ async def f():
         for _test_trampoline in [False, True]:
             await run_test(_test_trampoline)
 
+    async def test_invoice_stays_inflight_while_htlcs_unresolved(self):
+        """Tests that we don't mark an invoice as failed while htlcs we sent for it are still
+        unresolved. The receiver can still fulfill those htlcs, so telling the user the payment
+        failed would trick them into paying twice, e.g. using a fresh invoice, which the
+        "did not clear" check in pay_invoice cannot detect as a retry.
+        """
+        graph = self.prepare_chans_and_peers_in_graph(self.GRAPH_DEFINITIONS['single_chan'])
+        p1, p2 = graph.peers.values()
+        w1, w2 = graph.workers.values()
+        w2.enable_htlc_settle = False  # bob holds the htlc: he neither settles nor fails it
+        lnaddr, pay_req = self.prepare_invoice(w2)
+        payment_hash = lnaddr.paymenthash
+
+        # simulate pay_to_node giving up (e.g. out of attempts) while an htlc is still unresolved
+        orig_pay_to_node = w1.pay_to_node
+
+        async def pay_to_node_that_gives_up(**kwargs):
+            task = asyncio.ensure_future(orig_pay_to_node(**kwargs))
+            await p2.received_commitsig_event.wait()
+            task.cancel()
+            await asyncio.gather(task, return_exceptions=True)
+            raise PaymentFailure('giving up while htlcs are unresolved')
+
+        w1.pay_to_node = pay_to_node_that_gives_up
+
+        async def pay():
+            result, log = await w1.pay_invoice(pay_req)
+            self.assertFalse(result)
+            self.assertTrue(w1.has_unresolved_sent_htlcs(payment_hash))
+            # the money is still at risk, so the invoice must not look failed/unpaid
+            self.assertEqual(PR_INFLIGHT, w1.get_invoice_status(pay_req))
+            with self.assertRaises(PaymentFailure):
+                await w1.pay_invoice(pay_req)
+            # now let bob fulfill the htlc. nobody is waiting for the payment anymore,
+            # but the invoice must still end up as paid.
+            w2.enable_htlc_settle = True
+            while w1.get_invoice_status(pay_req) != PR_PAID:
+                await asyncio.sleep(0.01)
+            raise SuccessfulTest()
+
+        async def f():
+            async with OldTaskGroup() as group:
+                await group.spawn(p1._message_loop())
+                await group.spawn(p1.htlc_switch())
+                await group.spawn(p2._message_loop())
+                await group.spawn(p2.htlc_switch())
+                await asyncio.sleep(0.01)
+                await group.spawn(pay())
+
+        with self.assertRaises(SuccessfulTest):
+            await f()
+
     async def test_payment_race(self):
         """Alice and Bob pay each other simultaneously.
         They both send 'update_add_htlc' and receive each other's update