lnworker: fix type error re pending_payments, and impl malformed htlcs

In old code, in lnpeer.htlc_switch(), "error" in lnworker.pending_payments
had incorrect type.

TODO: we need tests for payment failures...

electrum/lnchannel.py

@@ -50,7 +50,7 @@ from .lnutil import (Outpoint, LocalConfig, RemoteConfig, Keypair, OnlyPubkeyKey
                     HTLC_TIMEOUT_WEIGHT, HTLC_SUCCESS_WEIGHT, extract_ctn_from_tx_and_chan, UpdateAddHtlc,
                     funding_output_script, SENT, RECEIVED, LOCAL, REMOTE, HTLCOwner, make_commitment_outputs,
                     ScriptHtlc, PaymentFailure, calc_onchain_fees, RemoteMisbehaving, make_htlc_output_witness_script,
-                    ShortChannelID, map_htlcs_to_ctx_output_idxs, LNPeerAddr)
+                    ShortChannelID, map_htlcs_to_ctx_output_idxs, LNPeerAddr, BarePaymentAttemptLog)
 from .lnsweep import create_sweeptxs_for_our_ctx, create_sweeptxs_for_their_ctx
 from .lnsweep import create_sweeptx_for_their_revoked_htlc, SweepInfo
 from .lnhtlc import HTLCManager
@@ -158,7 +158,7 @@ class Channel(Logger):
         self._chan_ann_without_sigs = None  # type: Optional[bytes]
         self.revocation_store = RevocationStore(state["revocation_store"])
         self._can_send_ctx_updates = True  # type: bool
-        self._receive_fail_reasons = {}
+        self._receive_fail_reasons = {}  # type: Dict[int, BarePaymentAttemptLog]
 
     def get_id_for_log(self) -> str:
         scid = self.short_channel_id
@@ -622,8 +622,8 @@ class Channel(Logger):
                 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)
+                payment_attempt = self._receive_fail_reasons.get(htlc.htlc_id)
-                self.lnworker.payment_failed(self, htlc.payment_hash, reason)
+                self.lnworker.payment_failed(self, htlc.payment_hash, payment_attempt)
 
     def balance(self, whose: HTLCOwner, *, ctx_owner=HTLCOwner.LOCAL, ctn: int = None) -> int:
         """
@@ -793,11 +793,16 @@ class Channel(Logger):
         with self.db_lock:
             self.hm.send_fail(htlc_id)
 
-    def receive_fail_htlc(self, htlc_id: int, reason: bytes):
+    def receive_fail_htlc(self, htlc_id: int, *,
+                          error_bytes: Optional[bytes],
+                          reason: Optional[OnionRoutingFailureMessage] = None):
         self.logger.info("receive_fail_htlc")
         with self.db_lock:
             self.hm.recv_fail(htlc_id)
-        self._receive_fail_reasons[htlc_id] = reason
+        self._receive_fail_reasons[htlc_id] = BarePaymentAttemptLog(success=False,
+                                                                    preimage=None,
+                                                                    error_bytes=error_bytes,
+                                                                    error_reason=reason)
 
     def pending_local_fee(self):
         return self.constraints.capacity - sum(x.value for x in self.get_next_commitment(LOCAL).outputs())

electrum/lnpeer.py

@@ -1007,7 +1007,7 @@ class Peer(Logger):
         htlc_id = int.from_bytes(payload["id"], "big")
         reason = payload["reason"]
         self.logger.info(f"on_update_fail_htlc. chan {chan.short_channel_id}. htlc_id {htlc_id}")
-        chan.receive_fail_htlc(htlc_id, reason)  # TODO handle exc and maybe fail channel (e.g. bad htlc_id)
+        chan.receive_fail_htlc(htlc_id, error_bytes=reason)  # TODO handle exc and maybe fail channel (e.g. bad htlc_id)
         self.maybe_send_commitment(chan)
 
     def maybe_send_commitment(self, chan: Channel):
@@ -1093,10 +1093,9 @@ class Peer(Logger):
         if failure_code & OnionFailureCodeMetaFlag.BADONION == 0:
             asyncio.ensure_future(self.lnworker.try_force_closing(chan.channel_id))
             raise RemoteMisbehaving(f"received update_fail_malformed_htlc with unexpected failure code: {failure_code}")
-        reason = b''  # TODO somehow propagate "failure_code" ?
+        reason = OnionRoutingFailureMessage(code=failure_code, data=payload["sha256_of_onion"])
-        chan.receive_fail_htlc(htlc_id, reason)  # TODO handle exc and maybe fail channel (e.g. bad htlc_id)
+        chan.receive_fail_htlc(htlc_id, error_bytes=None, reason=reason)
         self.maybe_send_commitment(chan)
-        # TODO when forwarding, we need to propagate this "update_fail_malformed_htlc" downstream
 
     def on_update_add_htlc(self, chan: Channel, payload):
         payment_hash = payload["payment_hash"]
@@ -1222,20 +1221,22 @@ class Peer(Logger):
                           id=htlc_id,
                           payment_preimage=preimage)
 
-    def fail_htlc(self, chan: Channel, htlc_id: int, onion_packet: Optional[OnionPacket],
+    def fail_htlc(self, *, chan: Channel, htlc_id: int, onion_packet: Optional[OnionPacket],
-                  reason: OnionRoutingFailureMessage):
+                  reason: Optional[OnionRoutingFailureMessage], error_bytes: Optional[bytes]):
         self.logger.info(f"fail_htlc. chan {chan.short_channel_id}. htlc_id {htlc_id}. reason: {reason}")
         assert chan.can_send_ctx_updates(), f"cannot send updates: {chan.short_channel_id}"
         chan.fail_htlc(htlc_id)
         if onion_packet:
-            error_packet = construct_onion_error(reason, onion_packet, our_onion_private_key=self.privkey)
+            error_bytes = construct_onion_error(reason, onion_packet, our_onion_private_key=self.privkey)
+        if error_bytes:
             self.send_message("update_fail_htlc",
                               channel_id=chan.channel_id,
                               id=htlc_id,
-                              len=len(error_packet),
+                              len=len(error_bytes),
-                              reason=error_packet)
+                              reason=error_bytes)
         else:
-            assert len(reason.data) == 32, f"unexpected reason when sending 'update_fail_malformed_htlc': {reason!r}"
+            if not (reason.code & OnionFailureCodeMetaFlag.BADONION and len(reason.data) == 32):
+                raise Exception(f"unexpected reason when sending 'update_fail_malformed_htlc': {reason!r}")
             self.send_message("update_fail_malformed_htlc",
                               channel_id=chan.channel_id,
                               id=htlc_id,
@@ -1425,7 +1426,8 @@ class Peer(Logger):
                     chan.logger.info(f'found unfulfilled htlc: {htlc_id}')
                     htlc = chan.hm.log[REMOTE]['adds'][htlc_id]
                     payment_hash = htlc.payment_hash
-                    error = None  # type: Optional[OnionRoutingFailureMessage]
+                    error_reason = None  # type: Optional[OnionRoutingFailureMessage]
+                    error_bytes = None  # type: Optional[bytes]
                     preimage = None
                     onion_packet_bytes = bytes.fromhex(onion_packet_hex)
                     onion_packet = None
@@ -1433,36 +1435,45 @@ class Peer(Logger):
                         onion_packet = OnionPacket.from_bytes(onion_packet_bytes)
                         processed_onion = process_onion_packet(onion_packet, associated_data=payment_hash, our_onion_private_key=self.privkey)
                     except UnsupportedOnionPacketVersion:
-                        error = OnionRoutingFailureMessage(code=OnionFailureCode.INVALID_ONION_VERSION, data=sha256(onion_packet_bytes))
+                        error_reason = OnionRoutingFailureMessage(code=OnionFailureCode.INVALID_ONION_VERSION, data=sha256(onion_packet_bytes))
                     except InvalidOnionPubkey:
-                        error = OnionRoutingFailureMessage(code=OnionFailureCode.INVALID_ONION_KEY, data=sha256(onion_packet_bytes))
+                        error_reason = OnionRoutingFailureMessage(code=OnionFailureCode.INVALID_ONION_KEY, data=sha256(onion_packet_bytes))
                     except InvalidOnionMac:
-                        error = OnionRoutingFailureMessage(code=OnionFailureCode.INVALID_ONION_HMAC, data=sha256(onion_packet_bytes))
+                        error_reason = OnionRoutingFailureMessage(code=OnionFailureCode.INVALID_ONION_HMAC, data=sha256(onion_packet_bytes))
                     else:
                         if processed_onion.are_we_final:
-                            preimage, error = self.maybe_fulfill_htlc(
+                            preimage, error_reason = self.maybe_fulfill_htlc(
                                 chan=chan,
                                 htlc=htlc,
                                 onion_packet=onion_packet,
                                 processed_onion=processed_onion)
                         elif not forwarded:
-                            error = self.maybe_forward_htlc(
+                            error_reason = self.maybe_forward_htlc(
                                 chan=chan,
                                 htlc=htlc,
                                 onion_packet=onion_packet,
                                 processed_onion=processed_onion)
-                            if not error:
+                            if not error_reason:
                                 unfulfilled[htlc_id] = local_ctn, remote_ctn, onion_packet_hex, True
                         else:
+                            # TODO self.lnworker.pending_payments is not persisted,
+                            #      so what happens if we restart the process?...
                             f = self.lnworker.pending_payments[payment_hash]
                             if f.done():
-                                success, preimage, error = f.result()
+                                payment_attempt = f.result()
+                                preimage = payment_attempt.preimage
+                                error_bytes = payment_attempt.error_bytes
+                                error_reason = payment_attempt.error_reason
                         if preimage:
                             await self.lnworker.enable_htlc_settle.wait()
                             self.fulfill_htlc(chan, htlc.htlc_id, preimage)
                             done.add(htlc_id)
-                    if error:
+                    if error_reason or error_bytes:
-                        self.fail_htlc(chan, htlc.htlc_id, onion_packet, error)
+                        self.fail_htlc(chan=chan,
+                                       htlc_id=htlc.htlc_id,
+                                       onion_packet=onion_packet,
+                                       reason=error_reason,
+                                       error_bytes=error_bytes)
                         done.add(htlc_id)
                 # cleanup
                 for htlc_id in done:

electrum/lnutil.py

@@ -112,7 +112,7 @@ class Outpoint(StoredObject):
 
 
 class PaymentAttemptFailureDetails(NamedTuple):
-    sender_idx: int
+    sender_idx: Optional[int]
     failure_msg: 'OnionRoutingFailureMessage'
     is_blacklisted: bool
 
@@ -128,16 +128,17 @@ class PaymentAttemptLog(NamedTuple):
         if not self.exception:
             route = self.route
             route_str = '%d'%len(route)
+            short_channel_id = None
             if not self.success:
                 sender_idx = self.failure_details.sender_idx
                 failure_msg = self.failure_details.failure_msg
-                short_channel_id = route[sender_idx+1].short_channel_id
+                if sender_idx is not None:
-                data = failure_msg.data
+                    short_channel_id = route[sender_idx+1].short_channel_id
                 message = str(failure_msg.code.name)
             else:
                 short_channel_id = route[-1].short_channel_id
                 message = _('Success')
-            chan_str = str(short_channel_id)
+            chan_str = str(short_channel_id) if short_channel_id else _("Unknown")
         else:
             route_str = 'None'
             chan_str = 'N/A'
@@ -145,6 +146,13 @@ class PaymentAttemptLog(NamedTuple):
         return route_str, chan_str, message
 
 
+class BarePaymentAttemptLog(NamedTuple):
+    success: bool
+    preimage: Optional[bytes]
+    error_bytes: Optional[bytes]
+    error_reason: Optional['OnionRoutingFailureMessage'] = None
+
+
 class LightningError(Exception): pass
 class LightningPeerConnectionClosed(LightningError): pass
 class UnableToDeriveSecret(LightningError): pass

electrum/lnworker.py

@@ -53,7 +53,8 @@ from .lnutil import (Outpoint, LNPeerAddr,
                      UnknownPaymentHash, MIN_FINAL_CLTV_EXPIRY_FOR_INVOICE,
                      NUM_MAX_EDGES_IN_PAYMENT_PATH, SENT, RECEIVED, HTLCOwner,
                      UpdateAddHtlc, Direction, LnLocalFeatures,
-                     ShortChannelID, PaymentAttemptLog, PaymentAttemptFailureDetails)
+                     ShortChannelID, PaymentAttemptLog, PaymentAttemptFailureDetails,
+                     BarePaymentAttemptLog)
 from .lnutil import ln_dummy_address, ln_compare_features
 from .transaction import PartialTxOutput, PartialTransaction, PartialTxInput
 from .lnonion import OnionFailureCode, process_onion_packet, OnionPacket
@@ -436,8 +437,7 @@ class LNWallet(LNWorker):
         for channel_id, c in channels.items():
             self.channels[bfh(channel_id)] = Channel(c, sweep_address=self.sweep_address, lnworker=self)
 
-        # timestamps of opening and closing transactions
+        self.pending_payments = defaultdict(asyncio.Future)  # type: Dict[bytes, asyncio.Future[BarePaymentAttemptLog]]
-        self.pending_payments = defaultdict(asyncio.Future)
 
     @ignore_exceptions
     @log_exceptions
@@ -954,31 +954,36 @@ class LNWallet(LNWorker):
         await peer.initialized
         htlc = peer.pay(route, chan, int(lnaddr.amount * COIN * 1000), lnaddr.paymenthash, lnaddr.get_min_final_cltv_expiry())
         self.network.trigger_callback('htlc_added', htlc, lnaddr, SENT)
-        success, preimage, reason = await self.await_payment(lnaddr.paymenthash)
+        payment_attempt = await self.await_payment(lnaddr.paymenthash)
-        if success:
+        if payment_attempt.success:
             failure_log = None
         else:
-            # TODO this blacklisting is fragile, consider (who to ban/penalize?):
+            if payment_attempt.error_bytes:
-            #      - we might not be able to decode "reason" (coming from update_fail_htlc).
+                # TODO "decode_onion_error" might raise, catch and maybe blacklist/penalise someone?
-            #      - handle update_fail_malformed_htlc case, where there is (kinda) no "reason"
+                failure_msg, sender_idx = chan.decode_onion_error(payment_attempt.error_bytes, route, htlc.htlc_id)
-            failure_msg, sender_idx = chan.decode_onion_error(reason, route, htlc.htlc_id)
+                is_blacklisted = self.handle_error_code_from_failed_htlc(failure_msg, sender_idx, route, peer)
-            blacklist = self.handle_error_code_from_failed_htlc(failure_msg, sender_idx, route, peer)
+                if is_blacklisted:
-            if blacklist:
+                    # blacklist channel after reporter node
-                # blacklist channel after reporter node
+                    # TODO this should depend on the error (even more granularity)
-                # TODO this should depend on the error (even more granularity)
+                    # also, we need finer blacklisting (directed edges; nodes)
-                # also, we need finer blacklisting (directed edges; nodes)
+                    try:
-                try:
+                        short_chan_id = route[sender_idx + 1].short_channel_id
-                    short_chan_id = route[sender_idx + 1].short_channel_id
+                    except IndexError:
-                except IndexError:
+                        self.logger.info("payment destination reported error")
-                    self.logger.info("payment destination reported error")
+                    else:
-                else:
+                        self.network.path_finder.add_to_blacklist(short_chan_id)
-                    self.network.path_finder.add_to_blacklist(short_chan_id)
+            else:
+                # probably got "update_fail_malformed_htlc". well... who to penalise now?
+                assert payment_attempt.error_reason is not None
+                sender_idx = None
+                failure_msg = payment_attempt.error_reason
+                is_blacklisted = False
             failure_log = PaymentAttemptFailureDetails(sender_idx=sender_idx,
                                                        failure_msg=failure_msg,
-                                                       is_blacklisted=blacklist)
+                                                       is_blacklisted=is_blacklisted)
         return PaymentAttemptLog(route=route,
-                                 success=success,
+                                 success=payment_attempt.success,
-                                 preimage=preimage,
+                                 preimage=payment_attempt.preimage,
                                  failure_details=failure_log)
 
     def handle_error_code_from_failed_htlc(self, failure_msg, sender_idx, route, peer):
@@ -1205,10 +1210,10 @@ class LNWallet(LNWorker):
         if status in SAVED_PR_STATUS:
             self.set_payment_status(bfh(key), status)
 
-    async def await_payment(self, payment_hash):
+    async def await_payment(self, payment_hash: bytes) -> BarePaymentAttemptLog:
-        success, preimage, reason = await self.pending_payments[payment_hash]
+        payment_attempt = await self.pending_payments[payment_hash]
         self.pending_payments.pop(payment_hash)
-        return success, preimage, reason
+        return payment_attempt
 
     def set_payment_status(self, payment_hash: bytes, status):
         try:
@@ -1219,12 +1224,12 @@ class LNWallet(LNWorker):
         info = info._replace(status=status)
         self.save_payment_info(info)
 
-    def payment_failed(self, chan, payment_hash: bytes, reason: bytes):
+    def payment_failed(self, chan, payment_hash: bytes, payment_attempt: BarePaymentAttemptLog):
         self.set_payment_status(payment_hash, PR_UNPAID)
         key = payment_hash.hex()
         f = self.pending_payments.get(payment_hash)
         if f and not f.cancelled():
-            f.set_result((False, None, reason))
+            f.set_result(payment_attempt)
         else:
             chan.logger.info('received unexpected payment_failed, probably from previous session')
             self.network.trigger_callback('invoice_status', key)
@@ -1237,7 +1242,10 @@ class LNWallet(LNWorker):
         key = payment_hash.hex()
         f = self.pending_payments.get(payment_hash)
         if f and not f.cancelled():
-            f.set_result((True, preimage, None))
+            payment_attempt = BarePaymentAttemptLog(success=True,
+                                                    preimage=preimage,
+                                                    error_bytes=None)
+            f.set_result(payment_attempt)
         else:
             chan.logger.info('received unexpected payment_sent, probably from previous session')
             self.network.trigger_callback('invoice_status', key)

electrum/tests/test_lnchannel.py

@@ -619,7 +619,7 @@ class TestAvailableToSpend(ElectrumTestCase):
         bob_idx = bob_channel.receive_htlc(htlc_dict).htlc_id
         force_state_transition(alice_channel, bob_channel)
         bob_channel.fail_htlc(bob_idx)
-        alice_channel.receive_fail_htlc(alice_idx, None)
+        alice_channel.receive_fail_htlc(alice_idx, error_bytes=None)
         # Alice now has gotten all her original balance (5 BTC) back, however,
         # adding a new HTLC at this point SHOULD fail, since if she adds the
         # HTLC and signs the next state, Bob cannot assume she received the