Better handling of TemporaryChannelFailure (#1726)

When a node returns a TemporaryChannelFailure, we should ignore this channel when retrying. In some cases (such as channels from routing hints) this was not correctly handled. Fixes #1725
ACINQ · Mar 10, 2021 · 2772138 · 2772138
1 parent 6364ae3
commit 2772138
Show file tree

Hide file tree

Showing 4 changed files with 57 additions and 17 deletions.
diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentEvents.scala b/eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentEvents.scala
@@ -22,7 +22,7 @@ import fr.acinq.eclair.crypto.Sphinx
 import fr.acinq.eclair.payment.PaymentRequest.ExtraHop
 import fr.acinq.eclair.router.Announcements
 import fr.acinq.eclair.router.Router.{ChannelDesc, ChannelHop, Hop, Ignore}
-import fr.acinq.eclair.wire.{ChannelUpdate, Node}
+import fr.acinq.eclair.wire.{ChannelDisabled, ChannelUpdate, Node, TemporaryChannelFailure}
 import fr.acinq.eclair.{MilliSatoshi, ShortChannelId}
 
 import java.util.UUID
@@ -158,29 +158,42 @@ object PaymentFailure {
       .collectFirst { case RemoteFailure(_, Sphinx.DecryptedFailurePacket(origin, u: Update)) if origin == nodeId => u.update }
       .isDefined
 
+  /** Ignore the channel outgoing from the given nodeId in the given route. */
+  private def ignoreNodeOutgoingChannel(nodeId: PublicKey, hops: Seq[Hop], ignore: Ignore): Ignore = {
+    hops.collectFirst {
+      case hop: ChannelHop if hop.nodeId == nodeId => ChannelDesc(hop.lastUpdate.shortChannelId, hop.nodeId, hop.nextNodeId)
+    } match {
+      case Some(faultyChannel) => ignore + faultyChannel
+      case None => ignore
+    }
+  }
+
   /** 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 =>
       // 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)) =>
       ignore + nodeId
-    case RemoteFailure(_, Sphinx.DecryptedFailurePacket(nodeId, failureMessage: Update)) =>
+    case RemoteFailure(hops, Sphinx.DecryptedFailurePacket(nodeId, failureMessage: Update)) =>
       if (Announcements.checkSig(failureMessage.update, nodeId)) {
-        // We were using an outdated channel update, we should retry with the new one and nobody should be penalized.
-        ignore
+        val shouldIgnore = failureMessage match {
+          case _: TemporaryChannelFailure => true
+          case _: ChannelDisabled => true
+          case _ => false
+        }
+        if (shouldIgnore) {
+          ignoreNodeOutgoingChannel(nodeId, hops, ignore)
+        } else {
+          // We were using an outdated channel update, we should retry with the new one and nobody should be penalized.
+          ignore
+        }
       } else {
         // This node is fishy, it gave us a bad signature, so let's filter it out.
         ignore + nodeId
       }
     case RemoteFailure(hops, Sphinx.DecryptedFailurePacket(nodeId, _)) =>
-      // Let's ignore the channel outgoing from nodeId.
-      hops.collectFirst {
-        case hop: ChannelHop if hop.nodeId == nodeId => ChannelDesc(hop.lastUpdate.shortChannelId, hop.nodeId, hop.nextNodeId)
-      } match {
-        case Some(faultyChannel) => ignore + faultyChannel
-        case None => ignore
-      }
+      ignoreNodeOutgoingChannel(nodeId, hops, ignore)
     case UnreadableRemoteFailure(hops) =>
       // We don't know which node is sending garbage, let's blacklist all nodes except the one we are directly connected to and the final recipient.
       val blacklist = hops.map(_.nextNodeId).drop(1).dropRight(1).toSet

diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentLifecycle.scala b/eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentLifecycle.scala
@@ -193,18 +193,20 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
         retry(failure, d)
       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(cfg.fullRoute(route), e)
         val ignore1 = if (Announcements.checkSig(failureMessage.update, nodeId)) {
           val assistedRoutes1 = handleUpdate(nodeId, failureMessage, d)
+          val ignore1 = PaymentFailure.updateIgnored(failure, ignore)
           // let's try again, router will have updated its state
-          router ! RouteRequest(nodeParams.nodeId, c.targetNodeId, c.finalPayload.amount, c.getMaxFee(nodeParams), assistedRoutes1, ignore, c.routeParams, paymentContext = Some(cfg.paymentContext))
-          ignore
+          router ! RouteRequest(nodeParams.nodeId, c.targetNodeId, c.finalPayload.amount, c.getMaxFee(nodeParams), assistedRoutes1, ignore1, c.routeParams, paymentContext = Some(cfg.paymentContext))
+          ignore1
         } else {
           // this node is fishy, it gave us a bad sig!! let's filter it out
           log.warning(s"got bad signature from node=$nodeId update=${failureMessage.update}")
           router ! RouteRequest(nodeParams.nodeId, c.targetNodeId, c.finalPayload.amount, c.getMaxFee(nodeParams), c.assistedRoutes, ignore + nodeId, c.routeParams, paymentContext = Some(cfg.paymentContext))
           ignore + nodeId
         }
-        goto(WAITING_FOR_ROUTE) using WaitingForRoute(c, failures :+ RemoteFailure(cfg.fullRoute(route), e), ignore1)
+        goto(WAITING_FOR_ROUTE) using WaitingForRoute(c, failures :+ failure, ignore1)
       case Success(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(cfg.fullRoute(route), e)

diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/payment/MultiPartPaymentLifecycleSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/payment/MultiPartPaymentLifecycleSpec.scala
@@ -30,8 +30,8 @@ import fr.acinq.eclair.payment.send.MultiPartPaymentLifecycle._
 import fr.acinq.eclair.payment.send.PaymentError.RetryExhausted
 import fr.acinq.eclair.payment.send.PaymentInitiator.SendPaymentConfig
 import fr.acinq.eclair.payment.send.PaymentLifecycle.SendPaymentToRoute
-import fr.acinq.eclair.router.RouteNotFound
 import fr.acinq.eclair.router.Router._
+import fr.acinq.eclair.router.{Announcements, RouteNotFound}
 import fr.acinq.eclair.wire._
 import org.scalatest.Outcome
 import org.scalatest.funsuite.FixtureAnyFunSuiteLike
@@ -261,6 +261,30 @@ class MultiPartPaymentLifecycleSpec extends TestKitBaseClass with FixtureAnyFunS
     assert(router.expectMsgType[RouteRequest].assistedRoutes.head.head === ExtraHop(b, channelUpdate.shortChannelId, 250 msat, 150, CltvExpiryDelta(24)))
   }
 
+  test("retry with ignored routing hints (temporary channel failure)") { f =>
+    import f._
+
+    // The B -> E channel is private and provided in the invoice routing hints.
+    val routingHint = ExtraHop(b, hop_be.lastUpdate.shortChannelId, hop_be.lastUpdate.feeBaseMsat, hop_be.lastUpdate.feeProportionalMillionths, hop_be.lastUpdate.cltvExpiryDelta)
+    val payment = SendMultiPartPayment(sender.ref, randomBytes32, e, finalAmount, expiry, 3, routeParams = Some(routeParams), assistedRoutes = List(List(routingHint)))
+    sender.send(payFsm, payment)
+    assert(router.expectMsgType[RouteRequest].assistedRoutes.head.head === routingHint)
+    val route = Route(finalAmount, hop_ab_1 :: hop_be :: Nil)
+    router.send(payFsm, RouteResponse(Seq(route)))
+    childPayFsm.expectMsgType[SendPaymentToRoute]
+    childPayFsm.expectNoMsg(100 millis)
+
+    // B doesn't have enough liquidity on this channel.
+    // NB: we need a channel update with a valid signature, otherwise we'll ignore the node instead of this specific channel.
+    val channelUpdate = Announcements.makeChannelUpdate(hop_be.lastUpdate.chainHash, priv_b, e, hop_be.lastUpdate.shortChannelId, hop_be.lastUpdate.cltvExpiryDelta, hop_be.lastUpdate.htlcMinimumMsat, hop_be.lastUpdate.feeBaseMsat, hop_be.lastUpdate.feeProportionalMillionths, hop_be.lastUpdate.htlcMaximumMsat.get)
+    val childId = payFsm.stateData.asInstanceOf[PaymentProgress].pending.keys.head
+    childPayFsm.send(payFsm, PaymentFailed(childId, paymentHash, Seq(RemoteFailure(route.hops, Sphinx.DecryptedFailurePacket(b, TemporaryChannelFailure(channelUpdate))))))
+    // We update the routing hints accordingly before requesting a new route and ignore the channel.
+    val routeRequest = router.expectMsgType[RouteRequest]
+    assert(routeRequest.assistedRoutes.head.head === routingHint)
+    assert(routeRequest.ignore.channels.map(_.shortChannelId) === Set(channelUpdate.shortChannelId))
+  }
+
   test("update routing hints") { _ =>
     val routingHints = Seq(
       Seq(ExtraHop(a, ShortChannelId(1), 10 msat, 0, CltvExpiryDelta(12)), ExtraHop(b, ShortChannelId(2), 0 msat, 100, CltvExpiryDelta(24))),
@@ -536,7 +560,8 @@ object MultiPartPaymentLifecycleSpec {
    * where a has multiple channels with each of his peers.
    */
 
-  val a :: b :: c :: d :: e :: Nil = Seq.fill(5)(randomKey.publicKey)
+  val priv_a :: priv_b :: priv_c :: priv_d :: priv_e :: Nil = Seq.fill(5)(randomKey)
+  val a :: b :: c :: d :: e :: Nil = Seq(priv_a, priv_b, priv_c, priv_d, priv_e).map(_.publicKey)
   val channelId_ab_1 = ShortChannelId(1)
   val channelId_ab_2 = ShortChannelId(2)
   val channelId_be = ShortChannelId(3)

diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentLifecycleSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentLifecycleSpec.scala
@@ -392,7 +392,7 @@ class PaymentLifecycleSpec extends BaseRouterSpec {
     // payment lifecycle forwards the embedded channelUpdate to the router
     routerForwarder.expectMsg(update_bc)
     awaitCond(paymentFSM.stateName == WAITING_FOR_ROUTE)
-    routerForwarder.expectMsg(defaultRouteRequest(a, d, cfg))
+    routerForwarder.expectMsg(defaultRouteRequest(a, d, cfg).copy(ignore = Ignore(Set.empty, Set(ChannelDesc(update_bc.shortChannelId, b, c)))))
     routerForwarder.forward(routerFixture.router)
     // we allow 2 tries, so we send a 2nd request to the router
     assert(sender.expectMsgType[PaymentFailed].failures === RemoteFailure(route.hops, Sphinx.DecryptedFailurePacket(b, failure)) :: LocalFailure(Nil, RouteNotFound) :: Nil)