Fix detection of payments.

1. In lnhtlc, sent_in_ctn and failed_in_ctn need to look at the
remote ctx, and they need to be called when we receive a revocation,
not when we send one.

2. In lnchannel, we use 3 lnworker callbacks:
   - payment sent/payment failed (called when we receive a revocation)
   - payment received (called when we send a revocation)

3. Make revoke_current_commitment return a single value.
The second value was only used in tests, there is no need
to bloat the code with that

electrum/gui/qt/channel_details.py

@@ -75,7 +75,7 @@ class ChannelDetailsDialog(QtWidgets.QDialog):
         dest_mapping = self.keyname_rows[to]
         dest_mapping[payment_hash] = len(dest_mapping)
 
-    ln_payment_completed = QtCore.pyqtSignal(str, float, Direction, UpdateAddHtlc, bytes, bytes)
+    ln_payment_completed = QtCore.pyqtSignal(str, bytes, bytes)
     htlc_added = QtCore.pyqtSignal(str, UpdateAddHtlc, LnAddr, Direction)
 
     @QtCore.pyqtSlot(str, UpdateAddHtlc, LnAddr, Direction)
@@ -84,11 +84,11 @@ class ChannelDetailsDialog(QtWidgets.QDialog):
         mapping[htlc.payment_hash] = len(mapping)
         self.folders['inflight'].appendRow(self.make_htlc_item(htlc, direction))
 
-    @QtCore.pyqtSlot(str, float, Direction, UpdateAddHtlc, bytes, bytes)
+    @QtCore.pyqtSlot(str, bytes, bytes)
-    def do_ln_payment_completed(self, evtname, date, direction, htlc, preimage, chan_id):
+    def do_ln_payment_completed(self, evtname, payment_hash, chan_id):
         if chan_id != self.chan.channel_id:
             return
-        self.move('inflight', 'settled', htlc.payment_hash)
+        self.move('inflight', 'settled', payment_hash)
         self.update_sent_received()
 
     def update_sent_received(self):

electrum/lnchannel.py

@@ -561,25 +561,17 @@ class Channel(Logger):
             raise Exception("refusing to revoke as remote sig does not fit")
         with self.db_lock:
             self.hm.send_rev()
-        received = self.hm.received_in_ctn(new_ctn)
-        sent = self.hm.sent_in_ctn(new_ctn)
-        failed = self.hm.failed_in_ctn(new_ctn)
         if self.lnworker:
+            received = self.hm.received_in_ctn(new_ctn)
             for htlc in received:
-                self.lnworker.payment_completed(self, RECEIVED, htlc)
+                self.lnworker.payment_received(self, htlc.payment_hash)
-            for htlc in sent:
-                self.lnworker.payment_completed(self, SENT, htlc)
-            for htlc in failed:
-                reason = self._receive_fail_reasons.get(htlc.htlc_id)
-                self.lnworker.payment_failed(htlc.payment_hash, reason)
-        received_this_batch = htlcsum(received)
-        sent_this_batch = htlcsum(sent)
         last_secret, last_point = self.get_secret_and_point(LOCAL, new_ctn - 1)
         next_secret, next_point = self.get_secret_and_point(LOCAL, new_ctn + 1)
-        return RevokeAndAck(last_secret, next_point), (received_this_batch, sent_this_batch)
+        return RevokeAndAck(last_secret, next_point)
 
     def receive_revocation(self, revocation: RevokeAndAck):
         self.logger.info("receive_revocation")
+        new_ctn = self.get_latest_ctn(REMOTE)
         cur_point = self.config[REMOTE].current_per_commitment_point
         derived_point = ecc.ECPrivkey(revocation.per_commitment_secret).get_public_key_bytes(compressed=True)
         if cur_point != derived_point:
@@ -590,6 +582,15 @@ class Channel(Logger):
             self.hm.recv_rev()
             self.config[REMOTE].current_per_commitment_point=self.config[REMOTE].next_per_commitment_point
             self.config[REMOTE].next_per_commitment_point=revocation.next_per_commitment_point
+        # lnworker callbacks
+        if self.lnworker:
+            sent = self.hm.sent_in_ctn(new_ctn)
+            for htlc in sent:
+                self.lnworker.payment_sent(self, htlc.payment_hash)
+            failed = self.hm.failed_in_ctn(new_ctn)
+            for htlc in failed:
+                reason = self._receive_fail_reasons.get(htlc.htlc_id)
+                self.lnworker.payment_failed(self, htlc.payment_hash, reason)
 
     def balance(self, whose, *, ctx_owner=HTLCOwner.LOCAL, ctn=None):
         """

electrum/lnhtlc.py

@@ -289,19 +289,31 @@ class HTLCManager:
         return sent + received
 
     def received_in_ctn(self, ctn: int) -> Sequence[UpdateAddHtlc]:
+        """
+        received htlcs that became fulfilled when we send a revocation.
+        we check only local, because they are commited in the remote ctx first.
+        """
         return [self.log[REMOTE]['adds'][htlc_id]
                 for htlc_id, ctns in self.log[REMOTE]['settles'].items()
                 if ctns[LOCAL] == ctn]
 
     def sent_in_ctn(self, ctn: int) -> Sequence[UpdateAddHtlc]:
+        """
+        sent htlcs that became fulfilled when we received a revocation
+        we check only remote, because they are commited in the local ctx first.
+        """
         return [self.log[LOCAL]['adds'][htlc_id]
                 for htlc_id, ctns in self.log[LOCAL]['settles'].items()
-                if ctns[LOCAL] == ctn]
+                if ctns[REMOTE] == ctn]
 
     def failed_in_ctn(self, ctn: int) -> Sequence[UpdateAddHtlc]:
+        """
+        sent htlcs that became failed when we received a revocation
+        we check only remote, because they are commited in the local ctx first.
+        """
         return [self.log[LOCAL]['adds'][htlc_id]
                 for htlc_id, ctns in self.log[LOCAL]['fails'].items()
-                if ctns[LOCAL] == ctn]
+                if ctns[REMOTE] == ctn]
 
     ##### Queries re Fees:
 

electrum/lnpeer.py

@@ -1047,7 +1047,7 @@ class Peer(Logger):
 
     def send_revoke_and_ack(self, chan: Channel):
         self.logger.info(f'send_revoke_and_ack. chan {chan.short_channel_id}. ctn: {chan.get_oldest_unrevoked_ctn(LOCAL)}')
-        rev, _ = chan.revoke_current_commitment()
+        rev = chan.revoke_current_commitment()
         self.lnworker.save_channel(chan)
         self.send_message("revoke_and_ack",
             channel_id=chan.channel_id,
@@ -1209,8 +1209,6 @@ class Peer(Logger):
         self.logger.info(f"_fulfill_htlc. chan {chan.short_channel_id}. htlc_id {htlc_id}")
         assert chan.can_send_ctx_updates(), f"cannot send updates: {chan.short_channel_id}"
         chan.settle_htlc(preimage, htlc_id)
-        payment_hash = sha256(preimage)
-        self.lnworker.payment_received(payment_hash)
         self.send_message("update_fulfill_htlc",
                           channel_id=chan.channel_id,
                           id=htlc_id,

electrum/lnworker.py

@@ -517,15 +517,6 @@ class LNWallet(LNWorker):
             return ps.name
         return cs.name
 
-    def payment_completed(self, chan: Channel, direction: Direction,
-                          htlc: UpdateAddHtlc):
-        chan_id = chan.channel_id
-        preimage = self.get_preimage(htlc.payment_hash)
-        timestamp = int(time.time())
-        self.network.trigger_callback('ln_payment_completed', timestamp, direction, htlc, preimage, chan_id)
-        if direction == SENT:
-            self.payment_sent(htlc.payment_hash)
-
     def get_settled_payments(self):
         # return one item per payment_hash
         # note: with AMP we will have several channels per payment
@@ -1208,22 +1199,24 @@ class LNWallet(LNWorker):
         info = info._replace(status=status)
         self.save_payment_info(info)
 
-    def payment_failed(self, payment_hash: bytes, reason):
+    def payment_failed(self, chan, payment_hash: bytes, reason):
         self.set_payment_status(payment_hash, PR_UNPAID)
         f = self.pending_payments[payment_hash]
         if not f.cancelled():
             f.set_result((False, None, reason))
 
-    def payment_sent(self, payment_hash: bytes):
+    def payment_sent(self, chan, payment_hash: bytes):
         self.set_payment_status(payment_hash, PR_PAID)
         preimage = self.get_preimage(payment_hash)
         f = self.pending_payments[payment_hash]
         if not f.cancelled():
             f.set_result((True, preimage, None))
+        self.network.trigger_callback('ln_payment_completed', payment_hash, chan.channel_id)
 
-    def payment_received(self, payment_hash: bytes):
+    def payment_received(self, chan, payment_hash: bytes):
         self.set_payment_status(payment_hash, PR_PAID)
         self.network.trigger_callback('request_status', payment_hash.hex(), PR_PAID)
+        self.network.trigger_callback('ln_payment_completed', payment_hash, chan.channel_id)
 
     async def _calc_routing_hints_for_invoice(self, amount_sat):
         """calculate routing hints (BOLT-11 'r' field)"""

electrum/tests/test_lnchannel.py

@@ -316,7 +316,7 @@ class TestChannel(ElectrumTestCase):
 
         # Bob revokes his prior commitment given to him by Alice, since he now
         # has a valid signature for a newer commitment.
-        bobRevocation, _ = bob_channel.revoke_current_commitment()
+        bobRevocation = bob_channel.revoke_current_commitment()
         self.assertTrue(bob_channel.signature_fits(bob_channel.get_latest_commitment(LOCAL)))
 
         # Bob finally sends a signature for Alice's commitment transaction.
@@ -365,7 +365,7 @@ class TestChannel(ElectrumTestCase):
         self.assertNotEqual(tx0, tx1)
 
         # Alice then generates a revocation for bob.
-        aliceRevocation, _ = alice_channel.revoke_current_commitment()
+        aliceRevocation = alice_channel.revoke_current_commitment()
 
         tx2 = str(alice_channel.force_close_tx())
         # since alice already has the signature for the next one, it doesn't change her force close tx (it was already the newer one)
@@ -441,7 +441,7 @@ class TestChannel(ElectrumTestCase):
         self.assertEqual(alice_channel.balance(LOCAL), 500000000000)
         self.assertEqual(1, alice_channel.get_oldest_unrevoked_ctn(LOCAL))
         self.assertEqual(len(alice_channel.included_htlcs(LOCAL, RECEIVED, ctn=2)), 0)
-        aliceRevocation2, _ = alice_channel.revoke_current_commitment()
+        aliceRevocation2 = alice_channel.revoke_current_commitment()
         aliceSig2, aliceHtlcSigs2 = alice_channel.sign_next_commitment()
         self.assertEqual(aliceHtlcSigs2, [], "alice should generate no htlc signatures")
         self.assertEqual(len(bob_channel.get_latest_commitment(LOCAL).outputs()), 3)
@@ -449,7 +449,8 @@ class TestChannel(ElectrumTestCase):
 
         bob_channel.receive_new_commitment(aliceSig2, aliceHtlcSigs2)
 
-        bobRevocation2, (received, sent) = bob_channel.revoke_current_commitment()
+        bobRevocation2 = bob_channel.revoke_current_commitment()
+        received = lnchannel.htlcsum(bob_channel.hm.received_in_ctn(bob_channel.get_latest_ctn(LOCAL)))
         self.assertEqual(one_bitcoin_in_msat, received)
         alice_channel.receive_revocation(bobRevocation2)
 
@@ -525,7 +526,7 @@ class TestChannel(ElectrumTestCase):
 
         self.assertNotEqual(fee, bob_channel.get_oldest_unrevoked_feerate(LOCAL))
         self.assertEqual(fee, bob_channel.get_latest_feerate(LOCAL))
-        rev, _ = bob_channel.revoke_current_commitment()
+        rev = bob_channel.revoke_current_commitment()
         self.assertEqual(fee, bob_channel.get_oldest_unrevoked_feerate(LOCAL))
 
         alice_channel.receive_revocation(rev)
@@ -536,7 +537,7 @@ class TestChannel(ElectrumTestCase):
 
         self.assertNotEqual(fee, alice_channel.get_oldest_unrevoked_feerate(LOCAL))
         self.assertEqual(fee, alice_channel.get_latest_feerate(LOCAL))
-        rev, _ = alice_channel.revoke_current_commitment()
+        rev = alice_channel.revoke_current_commitment()
         self.assertEqual(fee, alice_channel.get_oldest_unrevoked_feerate(LOCAL))
 
         bob_channel.receive_revocation(rev)
@@ -552,14 +553,14 @@ class TestChannel(ElectrumTestCase):
         bob_sig, bob_htlc_sigs = bob_channel.sign_next_commitment()
         alice_channel.receive_new_commitment(bob_sig, bob_htlc_sigs)
 
-        alice_revocation, _ = alice_channel.revoke_current_commitment()
+        alice_revocation = alice_channel.revoke_current_commitment()
         bob_channel.receive_revocation(alice_revocation)
         alice_sig, alice_htlc_sigs = alice_channel.sign_next_commitment()
         bob_channel.receive_new_commitment(alice_sig, alice_htlc_sigs)
 
         self.assertNotEqual(fee, bob_channel.get_oldest_unrevoked_feerate(LOCAL))
         self.assertEqual(fee, bob_channel.get_latest_feerate(LOCAL))
-        bob_revocation, _ = bob_channel.revoke_current_commitment()
+        bob_revocation = bob_channel.revoke_current_commitment()
         self.assertEqual(fee, bob_channel.get_oldest_unrevoked_feerate(LOCAL))
 
         bob_sig, bob_htlc_sigs = bob_channel.sign_next_commitment()
@@ -568,7 +569,7 @@ class TestChannel(ElectrumTestCase):
 
         self.assertNotEqual(fee, alice_channel.get_oldest_unrevoked_feerate(LOCAL))
         self.assertEqual(fee, alice_channel.get_latest_feerate(LOCAL))
-        alice_revocation, _ = alice_channel.revoke_current_commitment()
+        alice_revocation = alice_channel.revoke_current_commitment()
         self.assertEqual(fee, alice_channel.get_oldest_unrevoked_feerate(LOCAL))
 
         bob_channel.receive_revocation(alice_revocation)
@@ -805,11 +806,11 @@ class TestDust(ElectrumTestCase):
 
 def force_state_transition(chanA, chanB):
     chanB.receive_new_commitment(*chanA.sign_next_commitment())
-    rev, _ = chanB.revoke_current_commitment()
+    rev = chanB.revoke_current_commitment()
     bob_sig, bob_htlc_sigs = chanB.sign_next_commitment()
     chanA.receive_revocation(rev)
     chanA.receive_new_commitment(bob_sig, bob_htlc_sigs)
-    chanB.receive_revocation(chanA.revoke_current_commitment()[0])
+    chanB.receive_revocation(chanA.revoke_current_commitment())
 
 # calcStaticFee calculates appropriate fees for commitment transactions.  This
 # function provides a simple way to allow test balance assertions to take fee

electrum/tests/test_lnpeer.py

@@ -126,6 +126,7 @@ class MockLNWallet:
     await_payment = LNWallet.await_payment
     payment_received = LNWallet.payment_received
     payment_sent = LNWallet.payment_sent
+    payment_failed = LNWallet.payment_failed
     save_preimage = LNWallet.save_preimage
     get_preimage = LNWallet.get_preimage
     _create_route_from_invoice = LNWallet._create_route_from_invoice
@@ -134,7 +135,7 @@ class MockLNWallet:
     _pay = LNWallet._pay
     force_close_channel = LNWallet.force_close_channel
     get_first_timestamp = lambda self: 0
-    payment_completed = LNWallet.payment_completed
+
 
 class MockTransport:
     def __init__(self, name):