Identify failing node by its index (#3224)

When a HTLC is failed remotely, the failing node was previously identified by its node id.
However this is not enough if the same node appears multiple times in the payment route, for instance for circular rebalancing.
We now identify the failing node using its index in the payment route.

Author: thomash-acinq

eclair-core/src/main/scala/fr/acinq/eclair/crypto/Sphinx.scala

@@ -273,9 +273,10 @@ object Sphinx extends Logging {
    * A properly decrypted failure from a node in the route.
    *
    * @param originNode     public key of the node that generated the failure.
+   * @param index          position in the route of the node that generated the failure.
    * @param failureMessage friendly failure message.
    */
-  case class DecryptedFailurePacket(originNode: PublicKey, failureMessage: FailureMessage)
+  case class DecryptedFailurePacket(originNode: PublicKey, index: Int, failureMessage: FailureMessage)
 
   /**
    * The downstream failure could not be decrypted.
@@ -335,16 +336,16 @@ object Sphinx extends Logging {
      * @return failure message if the origin of the packet could be identified and the packet decrypted, the unwrapped
      *         failure packet otherwise.
      */
-    def decrypt(packet: ByteVector, attribution_opt: Option[ByteVector], sharedSecrets: Seq[SharedSecret]): HtlcFailure = {
+    def decrypt(packet: ByteVector, attribution_opt: Option[ByteVector], sharedSecrets: Seq[SharedSecret], index: Int = 1): HtlcFailure = {
       sharedSecrets match {
         case Nil => HtlcFailure(Nil, Left(CannotDecryptFailurePacket(packet, attribution_opt)))
         case ss :: tail =>
           val packet1 = wrap(packet, ss.secret)
           val attribution1_opt = attribution_opt.flatMap(Attribution.unwrap(_, packet1, ss.secret, sharedSecrets.length))
           val um = generateKey("um", ss.secret)
           val HtlcFailure(downstreamHoldTimes, failure) = FailureMessageCodecs.failureOnionCodec(Hmac256(um)).decode(packet1.toBitVector) match {
-            case Attempt.Successful(value) => HtlcFailure(Nil, Right(DecryptedFailurePacket(ss.remoteNodeId, value.value)))
-            case _ => decrypt(packet1, attribution1_opt.map(_._2), tail)
+            case Attempt.Successful(value) => HtlcFailure(Nil, Right(DecryptedFailurePacket(ss.remoteNodeId, index, value.value)))
+            case _ => decrypt(packet1, attribution1_opt.map(_._2), tail, index + 1)
           }
           HtlcFailure(attribution1_opt.map(n => HoldTime(n._1, ss.remoteNodeId) +: downstreamHoldTimes).getOrElse(Nil), failure)
       }

eclair-core/src/main/scala/fr/acinq/eclair/json/JsonSerializers.scala

@@ -375,7 +375,7 @@ object PaymentFailedSummarySerializer extends ConvertClassSerializer[PaymentFail
     val route = f.route.map(_.nodeId) ++ f.route.lastOption.map(_.nextNodeId)
     val message = f match {
       case LocalFailure(_, _, t) => t.getMessage
-      case RemoteFailure(_, _, Sphinx.DecryptedFailurePacket(origin, failureMessage)) => s"$origin returned: ${failureMessage.message}"
+      case RemoteFailure(_, _, Sphinx.DecryptedFailurePacket(origin, _, failureMessage)) => s"$origin returned: ${failureMessage.message}"
       case _: UnreadableRemoteFailure => "unreadable remote failure"
     }
     PaymentFailureSummaryJson(f.amount, route, message)

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

@@ -194,7 +194,7 @@ object PaymentFailure {
    */
   def hasAlreadyFailedOnce(nodeId: PublicKey, failures: Seq[PaymentFailure]): Boolean =
     failures
-      .collectFirst { case RemoteFailure(_, _, Sphinx.DecryptedFailurePacket(origin, u: Update)) if origin == nodeId => u.update_opt }
+      .collectFirst { case RemoteFailure(_, _, Sphinx.DecryptedFailurePacket(origin, _, u: Update)) if origin == nodeId => u.update_opt }
       .isDefined
 
   /** Ignore the channel outgoing from the given nodeId in the given route. */
@@ -213,12 +213,12 @@ object PaymentFailure {
 
   /** Update the set of nodes and channels to ignore in retries depending on the failure we received. */
   def updateIgnored(failure: PaymentFailure, ignore: Ignore): Ignore = failure match {
-    case RemoteFailure(_, hops, Sphinx.DecryptedFailurePacket(nodeId, _)) if nodeId == hops.last.nextNodeId =>
+    case RemoteFailure(_, hops, Sphinx.DecryptedFailurePacket(nodeId, _, _)) if nodeId == hops.last.nextNodeId =>
       // The failure came from the final recipient: the payment should be aborted without penalizing anyone in the route.
       ignore
-    case RemoteFailure(_, _, Sphinx.DecryptedFailurePacket(nodeId, _: Node)) =>
+    case RemoteFailure(_, _, Sphinx.DecryptedFailurePacket(nodeId, _, _: Node)) =>
       ignore + nodeId
-    case RemoteFailure(_, hops, Sphinx.DecryptedFailurePacket(nodeId, failureMessage: Update)) =>
+    case RemoteFailure(_, hops, Sphinx.DecryptedFailurePacket(nodeId, _, failureMessage: Update)) =>
       if (failureMessage.update_opt.forall(update => Announcements.checkSig(update, nodeId))) {
         val shouldIgnore = failureMessage match {
           case _: TemporaryChannelFailure => true
@@ -235,7 +235,7 @@ object PaymentFailure {
         // 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, _)) =>
+    case RemoteFailure(_, hops, Sphinx.DecryptedFailurePacket(nodeId, _, _)) =>
       ignoreNodeOutgoingEdge(nodeId, hops, ignore)
     case UnreadableRemoteFailure(_, hops, _, holdTimes) =>
       // TODO: Once everyone supports attributable errors, we should only exclude two nodes: the last for which we have attribution data and the next one.
@@ -267,7 +267,7 @@ 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)) => f.update_opt match {
+            case RemoteFailure(_, _, Sphinx.DecryptedFailurePacket(_, _, f: Update)) => f.update_opt match {
               case Some(update) => current.updated(update.shortChannelId, update)
               case None => current
             }

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

@@ -76,7 +76,7 @@ class Autoprobe(nodeParams: NodeParams, router: ActorRef, paymentInitiator: Acto
 
     case paymentResult: PaymentEvent =>
       paymentResult match {
-        case PaymentFailed(_, _, _ :+ RemoteFailure(_, _, DecryptedFailurePacket(targetNodeId, _: IncorrectOrUnknownPaymentDetails)), _) =>
+        case PaymentFailed(_, _, _ :+ RemoteFailure(_, _, DecryptedFailurePacket(targetNodeId, _, _: IncorrectOrUnknownPaymentDetails)), _) =>
           log.info(s"payment probe successful to node=$targetNodeId")
         case _ =>
           log.info(s"payment probe failed with paymentResult=$paymentResult")

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

@@ -208,43 +208,40 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
         Metrics.PaymentError.withTag(Tags.Failure, Tags.FailureType(UnreadableRemoteFailure(request.amount, Nil, e, htlcFailure.holdTimes))).increment()
         failure
     }) match {
-      case res@Right(Sphinx.DecryptedFailurePacket(nodeId, failureMessage)) =>
+      case res@Right(Sphinx.DecryptedFailurePacket(_, index, failureMessage)) =>
         // We have discovered some liquidity information with this payment: we update the router accordingly.
-        val stoppedRoute = route.stopAt(nodeId)
+        val stoppedRoute = route.stopAt(index)
         if (stoppedRoute.hops.length > 1) {
           router ! Router.RouteCouldRelay(stoppedRoute)
         }
         failureMessage match {
           case TemporaryChannelFailure(update_opt, _) =>
-            route.hops.find(_.nodeId == nodeId) match {
-              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 _ => ()
+            val failingHop = route.hops(index)
+            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 _ => // other errors should not be used for liquidity issues
         }
         res
       case res => res
     }) match {
-      case Right(e@Sphinx.DecryptedFailurePacket(nodeId, failureMessage)) if nodeId == recipient.nodeId =>
+      case Right(e@Sphinx.DecryptedFailurePacket(nodeId, _, failureMessage)) if nodeId == recipient.nodeId =>
         // if destination node returns an error, we fail the payment immediately
         log.warning(s"received an error message from target nodeId=$nodeId, failing the payment (failure=$failureMessage)")
         myStop(request, Left(PaymentFailed(id, paymentHash, failures :+ RemoteFailure(request.amount, route.fullRoute, e))))
-      case Right(e@Sphinx.DecryptedFailurePacket(nodeId, failureMessage)) if route.finalHop_opt.collect { case h: NodeHop if h.nodeId == nodeId => h }.nonEmpty =>
+      case Right(e@Sphinx.DecryptedFailurePacket(nodeId, _, failureMessage)) if route.finalHop_opt.collect { case h: NodeHop if h.nodeId == nodeId => h }.nonEmpty =>
         // if trampoline node returns an error, we fail the payment immediately
         log.warning(s"received an error message from trampoline nodeId=$nodeId, failing the payment (failure=$failureMessage)")
         myStop(request, Left(PaymentFailed(id, paymentHash, failures :+ RemoteFailure(request.amount, route.fullRoute, e))))
       case res if failures.size + 1 >= request.maxAttempts =>
         // otherwise we never try more than maxAttempts, no matter the kind of error returned
         val failure = res match {
-          case Right(e@Sphinx.DecryptedFailurePacket(nodeId, failureMessage)) =>
+          case Right(e@Sphinx.DecryptedFailurePacket(nodeId, _, failureMessage)) =>
             log.info(s"received an error message from nodeId=$nodeId (failure=$failureMessage)")
             failureMessage match {
               case failureMessage: Update => handleUpdate(nodeId, failureMessage, d)
@@ -261,11 +258,11 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
         log.warning(s"cannot parse returned error: unwrapped=$unwrapped, route=${route.printNodes()}")
         val failure = UnreadableRemoteFailure(request.amount, route.fullRoute, e, htlcFailure.holdTimes)
         retry(failure, d)
-      case Right(e@Sphinx.DecryptedFailurePacket(nodeId, failureMessage: Node)) =>
+      case Right(e@Sphinx.DecryptedFailurePacket(nodeId, _, failureMessage: Node)) =>
         log.info(s"received 'Node' type error message from nodeId=$nodeId, trying to route around it (failure=$failureMessage)")
         val failure = RemoteFailure(request.amount, route.fullRoute, e)
         retry(failure, d)
-      case Right(e@Sphinx.DecryptedFailurePacket(nodeId, failureMessage: Update)) =>
+      case Right(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 (failureMessage.update_opt.forall(update => Announcements.checkSig(update, nodeId))) {
@@ -292,7 +289,7 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
               goto(WAITING_FOR_ROUTE) using WaitingForRoute(request, failures :+ failure, ignore + nodeId)
           }
         }
-      case Right(e@Sphinx.DecryptedFailurePacket(nodeId, _: InvalidOnionBlinding)) =>
+      case Right(e@Sphinx.DecryptedFailurePacket(nodeId, _, _: InvalidOnionBlinding)) =>
         // there was a failure inside the blinded route we used: we cannot know why it failed, so let's ignore it.
         log.info(s"received an error coming from nodeId=$nodeId inside the blinded route, retrying with different blinded routes")
         val failure = RemoteFailure(request.amount, route.fullRoute, e)
@@ -305,7 +302,7 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
             router ! RouteRequest(self, nodeParams.nodeId, recipient, request.routeParams, ignore1, paymentContext = Some(cfg.paymentContext))
             goto(WAITING_FOR_ROUTE) using WaitingForRoute(request, failures :+ failure, ignore1)
         }
-      case Right(e@Sphinx.DecryptedFailurePacket(nodeId, failureMessage)) =>
+      case Right(e@Sphinx.DecryptedFailurePacket(nodeId, _, failureMessage)) =>
         log.info(s"received an error message from nodeId=$nodeId, trying to use a different channel (failure=$failureMessage)")
         val failure = RemoteFailure(request.amount, route.fullRoute, e)
         retry(failure, d)

eclair-core/src/main/scala/fr/acinq/eclair/router/Router.scala

@@ -692,9 +692,9 @@ object Router {
 
     def printChannels(): String = hops.map(_.shortChannelId).mkString("->")
 
-    def stopAt(nodeId: PublicKey): Route = {
-      val amountAtStop = hops.reverse.takeWhile(_.nextNodeId != nodeId).foldLeft(amount) { case (amount1, hop) => amount1 + hop.fee(amount1) }
-      Route(amountAtStop, hops.takeWhile(_.nodeId != nodeId), None)
+    def stopAt(index: Int): Route = {
+      val amountAtStop = hops.drop(index).foldRight(amount) { case (hop, amount1) => amount1 + hop.fee(amount1) }
+      Route(amountAtStop, hops.take(index), None)
     }
   }