Accept onion failure without a channel_update

lightning/bolts#1163 makes the channel update in
onion failures optional. One reason for this change is that it can be a
privacy issue: by applying a `channel_update` received from an payment
attempt, you may reveal that you are the sender. Another reason is that
some nodes have been omitting that field for years (which was arguably
a bug), and it's better to be able to correctly handle such failures.

Author: t-bast

eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala

@@ -640,7 +640,7 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with
             case PostRevocationAction.RejectHtlc(add) =>
               log.debug("rejecting incoming htlc {}", add)
               // NB: we don't set commit = true, we will sign all updates at once afterwards.
-              self ! CMD_FAIL_HTLC(add.id, Right(TemporaryChannelFailure(d.channelUpdate)), commit = true)
+              self ! CMD_FAIL_HTLC(add.id, Right(TemporaryChannelFailure(Some(d.channelUpdate))), commit = true)
             case PostRevocationAction.RelayFailure(result) =>
               log.debug("forwarding {} to relayer", result)
               relayer ! result

eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentEvents.scala

@@ -211,7 +211,7 @@ object PaymentFailure {
     case RemoteFailure(_, _, Sphinx.DecryptedFailurePacket(nodeId, _: Node)) =>
       ignore + nodeId
     case RemoteFailure(_, hops, Sphinx.DecryptedFailurePacket(nodeId, failureMessage: Update)) =>
-      if (Announcements.checkSig(failureMessage.update, nodeId)) {
+      if (failureMessage.update.forall(update => Announcements.checkSig(update, nodeId))) {
         val shouldIgnore = failureMessage match {
           case _: TemporaryChannelFailure => true
           case _: ChannelDisabled => true
@@ -224,7 +224,7 @@ object PaymentFailure {
           ignore
         }
       } else {
-        // This node is fishy, it gave us a bad signature, so let's filter it out.
+        // This node is fishy, it gave us a bad channel update signature, so let's filter it out.
         ignore + nodeId
       }
     case RemoteFailure(_, hops, Sphinx.DecryptedFailurePacket(nodeId, _)) =>
@@ -257,7 +257,10 @@ object PaymentFailure {
         // We're only interested in the last channel update received per channel.
         val updates = failures.foldLeft(Map.empty[ShortChannelId, ChannelUpdate]) {
           case (current, failure) => failure match {
-            case RemoteFailure(_, _, Sphinx.DecryptedFailurePacket(_, f: Update)) => current.updated(f.update.shortChannelId, f.update)
+            case RemoteFailure(_, _, Sphinx.DecryptedFailurePacket(_, f: Update)) => f.update match {
+              case Some(update) => current.updated(update.shortChannelId, update)
+              case None => current
+            }
             case _ => current
           }
         }

eclair-core/src/main/scala/fr/acinq/eclair/payment/relay/ChannelRelay.scala

@@ -71,15 +71,15 @@ object ChannelRelay {
    */
   def translateLocalError(error: Throwable, channelUpdate_opt: Option[ChannelUpdate]): FailureMessage = {
     (error, channelUpdate_opt) match {
-      case (_: ExpiryTooSmall, Some(channelUpdate)) => ExpiryTooSoon(channelUpdate)
+      case (_: ExpiryTooSmall, Some(channelUpdate)) => ExpiryTooSoon(Some(channelUpdate))
       case (_: ExpiryTooBig, _) => ExpiryTooFar()
-      case (_: InsufficientFunds, Some(channelUpdate)) => TemporaryChannelFailure(channelUpdate)
-      case (_: TooManyAcceptedHtlcs, Some(channelUpdate)) => TemporaryChannelFailure(channelUpdate)
-      case (_: HtlcValueTooHighInFlight, Some(channelUpdate)) => TemporaryChannelFailure(channelUpdate)
-      case (_: LocalDustHtlcExposureTooHigh, Some(channelUpdate)) => TemporaryChannelFailure(channelUpdate)
-      case (_: RemoteDustHtlcExposureTooHigh, Some(channelUpdate)) => TemporaryChannelFailure(channelUpdate)
-      case (_: FeerateTooDifferent, Some(channelUpdate)) => TemporaryChannelFailure(channelUpdate)
-      case (_: ChannelUnavailable, Some(channelUpdate)) if !channelUpdate.channelFlags.isEnabled => ChannelDisabled(channelUpdate.messageFlags, channelUpdate.channelFlags, channelUpdate)
+      case (_: InsufficientFunds, Some(channelUpdate)) => TemporaryChannelFailure(Some(channelUpdate))
+      case (_: TooManyAcceptedHtlcs, Some(channelUpdate)) => TemporaryChannelFailure(Some(channelUpdate))
+      case (_: HtlcValueTooHighInFlight, Some(channelUpdate)) => TemporaryChannelFailure(Some(channelUpdate))
+      case (_: LocalDustHtlcExposureTooHigh, Some(channelUpdate)) => TemporaryChannelFailure(Some(channelUpdate))
+      case (_: RemoteDustHtlcExposureTooHigh, Some(channelUpdate)) => TemporaryChannelFailure(Some(channelUpdate))
+      case (_: FeerateTooDifferent, Some(channelUpdate)) => TemporaryChannelFailure(Some(channelUpdate))
+      case (_: ChannelUnavailable, Some(channelUpdate)) if !channelUpdate.channelFlags.isEnabled => ChannelDisabled(channelUpdate.messageFlags, channelUpdate.channelFlags, Some(channelUpdate))
       case (_: ChannelUnavailable, None) => PermanentChannelFailure()
       case _ => TemporaryNodeFailure()
     }
@@ -91,7 +91,7 @@ object ChannelRelay {
       case f: HtlcResult.RemoteFailMalformed => CMD_FAIL_HTLC(originHtlcId, Right(createBadOnionFailure(f.fail.onionHash, f.fail.failureCode)), commit = true)
       case _: HtlcResult.OnChainFail => CMD_FAIL_HTLC(originHtlcId, Right(PermanentChannelFailure()), commit = true)
       case HtlcResult.ChannelFailureBeforeSigned => CMD_FAIL_HTLC(originHtlcId, Right(PermanentChannelFailure()), commit = true)
-      case f: HtlcResult.DisconnectedBeforeSigned => CMD_FAIL_HTLC(originHtlcId, Right(TemporaryChannelFailure(f.channelUpdate)), commit = true)
+      case f: HtlcResult.DisconnectedBeforeSigned => CMD_FAIL_HTLC(originHtlcId, Right(TemporaryChannelFailure(Some(f.channelUpdate))), commit = true)
     }
   }
 
@@ -294,16 +294,16 @@ class ChannelRelay private(nodeParams: NodeParams,
       case None =>
         RelayFailure(CMD_FAIL_HTLC(r.add.id, Right(UnknownNextPeer()), commit = true))
       case Some(c) if !c.channelUpdate.channelFlags.isEnabled =>
-        RelayFailure(CMD_FAIL_HTLC(r.add.id, Right(ChannelDisabled(c.channelUpdate.messageFlags, c.channelUpdate.channelFlags, c.channelUpdate)), commit = true))
+        RelayFailure(CMD_FAIL_HTLC(r.add.id, Right(ChannelDisabled(c.channelUpdate.messageFlags, c.channelUpdate.channelFlags, Some(c.channelUpdate))), commit = true))
       case Some(c) if r.amountToForward < c.channelUpdate.htlcMinimumMsat =>
-        RelayFailure(CMD_FAIL_HTLC(r.add.id, Right(AmountBelowMinimum(r.amountToForward, c.channelUpdate)), commit = true))
+        RelayFailure(CMD_FAIL_HTLC(r.add.id, Right(AmountBelowMinimum(r.amountToForward, Some(c.channelUpdate))), commit = true))
       case Some(c) if r.expiryDelta < c.channelUpdate.cltvExpiryDelta =>
-        RelayFailure(CMD_FAIL_HTLC(r.add.id, Right(IncorrectCltvExpiry(r.outgoingCltv, c.channelUpdate)), commit = true))
+        RelayFailure(CMD_FAIL_HTLC(r.add.id, Right(IncorrectCltvExpiry(r.outgoingCltv, Some(c.channelUpdate))), commit = true))
       case Some(c) if r.relayFeeMsat < nodeFee(c.channelUpdate.relayFees, r.amountToForward) &&
         // fees also do not satisfy the previous channel update for `enforcementDelay` seconds after current update
         (TimestampSecond.now() - c.channelUpdate.timestamp > nodeParams.relayParams.enforcementDelay ||
           outgoingChannel_opt.flatMap(_.prevChannelUpdate).forall(c => r.relayFeeMsat < nodeFee(c.relayFees, r.amountToForward))) =>
-        RelayFailure(CMD_FAIL_HTLC(r.add.id, Right(FeeInsufficient(r.add.amountMsat, c.channelUpdate)), commit = true))
+        RelayFailure(CMD_FAIL_HTLC(r.add.id, Right(FeeInsufficient(r.add.amountMsat, Some(c.channelUpdate))), commit = true))
       case Some(c: OutgoingChannel) =>
         val origin = Origin.ChannelRelayedHot(addResponseAdapter.toClassic, r.add, r.amountToForward)
         val nextBlindingKey_opt = r.payload match {

eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentLifecycle.scala

@@ -181,11 +181,18 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
           router ! Router.RouteCouldRelay(stoppedRoute)
         }
         failureMessage match {
-          case TemporaryChannelFailure(update, _) =>
+          case TemporaryChannelFailure(update_opt, _) =>
             route.hops.find(_.nodeId == nodeId) match {
-              case Some(failingHop) if HopRelayParams.areSame(failingHop.params, HopRelayParams.FromAnnouncement(update), ignoreHtlcSize = true) =>
-                router ! Router.ChannelCouldNotRelay(stoppedRoute.amount, failingHop)
-              case _ => // otherwise the relay parameters may have changed, so it's not necessarily a liquidity issue
+              case Some(failingHop) =>
+                val isLiquidityIssue = update_opt match {
+                  // If the relay parameters have changed, it's not necessarily a liquidity issue.
+                  case Some(update) => HopRelayParams.areSame(failingHop.params, HopRelayParams.FromAnnouncement(update), ignoreHtlcSize = true)
+                  case None => true
+                }
+                if (isLiquidityIssue) {
+                  router ! Router.ChannelCouldNotRelay(stoppedRoute.amount, failingHop)
+                }
+              case _ => ()
             }
           case _ => // other errors should not be used for liquidity issues
         }
@@ -227,7 +234,7 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
       case Success(e@Sphinx.DecryptedFailurePacket(nodeId, failureMessage: Update)) =>
         log.info(s"received 'Update' type error message from nodeId=$nodeId, retrying payment (failure=$failureMessage)")
         val failure = RemoteFailure(request.amount, route.fullRoute, e)
-        if (Announcements.checkSig(failureMessage.update, nodeId)) {
+        if (failureMessage.update.forall(update => Announcements.checkSig(update, nodeId))) {
           val recipient1 = handleUpdate(nodeId, failureMessage, d)
           val ignore1 = PaymentFailure.updateIgnored(failure, ignore)
           // let's try again, router will have updated its state
@@ -240,7 +247,7 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
               goto(WAITING_FOR_ROUTE) using WaitingForRoute(request.copy(recipient = recipient1), failures :+ failure, ignore1)
           }
         } else {
-          // this node is fishy, it gave us a bad sig!! let's filter it out
+          // this node is fishy, it gave us a bad channel update signature: let's filter it out.
           log.warning(s"got bad signature from node=$nodeId update=${failureMessage.update}")
           request match {
             case _: SendPaymentToRoute =>
@@ -289,38 +296,49 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
     val extraEdges1 = data.route.hops.find(_.nodeId == nodeId) match {
       case Some(hop) => hop.params match {
         case ann: HopRelayParams.FromAnnouncement =>
-          if (ann.channelUpdate.shortChannelId != failure.update.shortChannelId) {
-            // it is possible that nodes in the route prefer using a different channel (to the same N+1 node) than the one we requested, that's fine
-            log.info("received an update for a different channel than the one we asked: requested={} actual={} update={}", ann.channelUpdate.shortChannelId, failure.update.shortChannelId, failure.update)
-          } else if (Announcements.areSame(ann.channelUpdate, failure.update)) {
-            // node returned the exact same update we used, this can happen e.g. if the channel is imbalanced
-            // in that case, let's temporarily exclude the channel from future routes, giving it time to recover
-            log.info("received exact same update from nodeId={}, excluding the channel from futures routes", nodeId)
-            router ! ExcludeChannel(ChannelDesc(ann.channelUpdate.shortChannelId, nodeId, hop.nextNodeId), Some(nodeParams.routerConf.channelExcludeDuration))
-          } else if (PaymentFailure.hasAlreadyFailedOnce(nodeId, data.failures)) {
-            // this node had already given us a new channel update and is still unhappy, it is probably messing with us, let's exclude it
-            log.warning("it is the second time nodeId={} answers with a new update, excluding it: old={} new={}", nodeId, ann.channelUpdate, failure.update)
-            router ! ExcludeChannel(ChannelDesc(ann.channelUpdate.shortChannelId, nodeId, hop.nextNodeId), Some(nodeParams.routerConf.channelExcludeDuration))
-          } else {
-            log.info("got a new update for shortChannelId={}: old={} new={}", ann.channelUpdate.shortChannelId, ann.channelUpdate, failure.update)
+          failure.update match {
+            case Some(update) if ann.channelUpdate.shortChannelId != update.shortChannelId =>
+              // it is possible that nodes in the route prefer using a different channel (to the same N+1 node) than the one we requested, that's fine
+              log.info("received an update for a different channel than the one we asked: requested={} actual={} update={}", ann.channelUpdate.shortChannelId, update.shortChannelId, update)
+            case Some(update) if Announcements.areSame(ann.channelUpdate, update) =>
+              // node returned the exact same update we used, this can happen e.g. if the channel is imbalanced
+              // in that case, let's temporarily exclude the channel from future routes, giving it time to recover
+              log.info("received exact same update from nodeId={}, excluding the channel from futures routes", nodeId)
+              router ! ExcludeChannel(ChannelDesc(ann.channelUpdate.shortChannelId, nodeId, hop.nextNodeId), Some(nodeParams.routerConf.channelExcludeDuration))
+            case Some(_) if PaymentFailure.hasAlreadyFailedOnce(nodeId, data.failures) =>
+              // this node had already given us a new channel update and is still unhappy, it is probably messing with us, let's exclude it
+              log.warning("it is the second time nodeId={} answers with a new update, excluding it: old={} new={}", nodeId, ann.channelUpdate, failure.update)
+              router ! ExcludeChannel(ChannelDesc(ann.channelUpdate.shortChannelId, nodeId, hop.nextNodeId), Some(nodeParams.routerConf.channelExcludeDuration))
+            case Some(update) =>
+              log.info("got a new update for shortChannelId={}: old={} new={}", ann.channelUpdate.shortChannelId, ann.channelUpdate, update)
+            case None =>
+              // this isn't a relay parameter issue, so it's probably a liquidity issue
+              log.info("update not provided for shortChannelId={}", ann.channelUpdate.shortChannelId)
+              router ! ExcludeChannel(ChannelDesc(ann.channelUpdate.shortChannelId, nodeId, hop.nextNodeId), Some(nodeParams.routerConf.channelExcludeDuration))
           }
           data.recipient.extraEdges
-        case _: HopRelayParams.FromHint =>
-          log.info("received an update for a routing hint (shortChannelId={} nodeId={} enabled={} update={})", failure.update.shortChannelId, nodeId, failure.update.channelFlags.isEnabled, failure.update)
-          if (failure.update.channelFlags.isEnabled) {
-            data.recipient.extraEdges.map {
-              case edge: ExtraEdge if edge.sourceNodeId == nodeId && edge.targetNodeId == hop.nextNodeId => edge.update(failure.update)
-              case edge: ExtraEdge => edge
-            }
-          } else {
-            // if the channel is disabled, we temporarily exclude it: this is necessary because the routing hint doesn't
-            // contain channel flags to indicate that it's disabled
-            // we want the exclusion to be router-wide so that sister payments in the case of MPP are aware the channel is faulty
-            data.recipient.extraEdges
-              .find(edge => edge.sourceNodeId == nodeId && edge.targetNodeId == hop.nextNodeId)
-              .foreach(edge => router ! ExcludeChannel(ChannelDesc(edge.shortChannelId, edge.sourceNodeId, edge.targetNodeId), Some(nodeParams.routerConf.channelExcludeDuration)))
-            // we remove this edge for our next payment attempt
-            data.recipient.extraEdges.filterNot(edge => edge.sourceNodeId == nodeId && edge.targetNodeId == hop.nextNodeId)
+        case hint: HopRelayParams.FromHint =>
+          failure.update match {
+            case Some(update) =>
+              log.info("received an update for a routing hint (shortChannelId={} nodeId={} enabled={} update={})", update.shortChannelId, nodeId, update.channelFlags.isEnabled, failure.update)
+              if (update.channelFlags.isEnabled) {
+                data.recipient.extraEdges.map {
+                  case edge: ExtraEdge if edge.sourceNodeId == nodeId && edge.targetNodeId == hop.nextNodeId => edge.update(update)
+                  case edge: ExtraEdge => edge
+                }
+              } else {
+                // if the channel is disabled, we temporarily exclude it: this is necessary because the routing hint doesn't
+                // contain channel flags to indicate that it's disabled
+                // we want the exclusion to be router-wide so that sister payments in the case of MPP are aware the channel is faulty
+                data.recipient.extraEdges
+                  .find(edge => edge.sourceNodeId == nodeId && edge.targetNodeId == hop.nextNodeId)
+                  .foreach(edge => router ! ExcludeChannel(ChannelDesc(edge.shortChannelId, edge.sourceNodeId, edge.targetNodeId), Some(nodeParams.routerConf.channelExcludeDuration)))
+                // we remove this edge for our next payment attempt
+                data.recipient.extraEdges.filterNot(edge => edge.sourceNodeId == nodeId && edge.targetNodeId == hop.nextNodeId)
+              }
+            case None =>
+              // this is most likely a liquidity issue, we remove this edge for our next payment attempt
+              data.recipient.extraEdges.filterNot(edge => edge.sourceNodeId == nodeId && edge.targetNodeId == hop.nextNodeId)
           }
       }
       case None =>