From 2f590a80e22a6ad0c5dc0b6bd5540dc144042654 Mon Sep 17 00:00:00 2001 From: Bastien Teinturier <31281497+t-bast@users.noreply.github.com> Date: Thu, 11 Aug 2022 11:32:43 +0200 Subject: [PATCH] Refactor routing hint failure updates (#2370) This is a follow up of #2264 where we refactor handling of channel updates in failures coming from routing hints. For failures in one of the routing hints, we use the node_id pair (source, destination) instead of the short_channel_id to identify the edge. --- .../fr/acinq/eclair/payment/Invoice.scala | 5 +- .../payment/send/PaymentLifecycle.scala | 75 +++++++++++-------- .../scala/fr/acinq/eclair/router/Router.scala | 3 - .../eclair/payment/PaymentLifecycleSpec.scala | 2 +- 4 files changed, 47 insertions(+), 38 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/payment/Invoice.scala b/eclair-core/src/main/scala/fr/acinq/eclair/payment/Invoice.scala index 241240dec..b6f5bb78b 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/payment/Invoice.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/payment/Invoice.scala @@ -55,17 +55,20 @@ trait Invoice { } object Invoice { + /** An extra edge that can be used to pay a given invoice and may not be part of the public graph. */ sealed trait ExtraEdge { + // @formatter:off def sourceNodeId: PublicKey def feeBase: MilliSatoshi def feeProportionalMillionths: Long def cltvExpiryDelta: CltvExpiryDelta def htlcMinimum: MilliSatoshi def htlcMaximum_opt: Option[MilliSatoshi] - def relayFees: Relayer.RelayFees = Relayer.RelayFees(feeBase = feeBase, feeProportionalMillionths = feeProportionalMillionths) + // @formatter:on } + /** A normal graph edge, that should be handled exactly like public graph edges. */ case class BasicEdge(sourceNodeId: PublicKey, targetNodeId: PublicKey, shortChannelId: ShortChannelId, 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 index 6d4e5268c..d13c533e3 100644 --- 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 @@ -262,43 +262,52 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A * @return updated routing hints if applicable. */ private def handleUpdate(nodeId: PublicKey, failure: Update, data: WaitingForComplete): Seq[ExtraEdge] = { - // TODO: properly handle updates to channels provided as routing hints in the invoice - data.route.getChannelUpdateForNode(nodeId) match { - case Some(u) if u.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(s"received an update for a different channel than the one we asked: requested=${u.shortChannelId} actual=${failure.update.shortChannelId} update=${failure.update}") - case Some(u) if Announcements.areSame(u, 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(s"received exact same update from nodeId=$nodeId, excluding the channel from futures routes") - val nextNodeId = data.route.hops.find(_.nodeId == nodeId).get.nextNodeId - router ! ExcludeChannel(ChannelDesc(u.shortChannelId, nodeId, nextNodeId), Some(nodeParams.routerConf.channelExcludeDuration)) - case Some(u) 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(s"it is the second time nodeId=$nodeId answers with a new update, excluding it: old=$u new=${failure.update}") - val nextNodeId = data.route.hops.find(_.nodeId == nodeId).get.nextNodeId - router ! ExcludeChannel(ChannelDesc(u.shortChannelId, nodeId, nextNodeId), Some(nodeParams.routerConf.channelExcludeDuration)) - case Some(u) => - log.info(s"got a new update for shortChannelId=${u.shortChannelId}: old=$u new=${failure.update}") + val extraEdges1 = data.route.hops.find(_.nodeId == nodeId) match { + case Some(hop) => hop.params match { + case ann: ChannelRelayParams.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) + } + data.c.extraEdges + case _: ChannelRelayParams.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.c.extraEdges.map { + case edge: BasicEdge if edge.sourceNodeId == nodeId && edge.targetNodeId == hop.nextNodeId => edge.update(failure.update) + case edge: BasicEdge => 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.c.extraEdges + .find { case edge: BasicEdge => edge.sourceNodeId == nodeId && edge.targetNodeId == hop.nextNodeId } + .foreach { case edge: BasicEdge => router ! ExcludeChannel(ChannelDesc(edge.shortChannelId, edge.sourceNodeId, edge.targetNodeId), Some(nodeParams.routerConf.channelExcludeDuration)) } + // we remove this edge for our next payment attempt + data.c.extraEdges.filterNot { case edge: BasicEdge => edge.sourceNodeId == nodeId && edge.targetNodeId == hop.nextNodeId } + } + } case None => - log.error(s"couldn't find a channel update for node=$nodeId, this should never happen") + log.error(s"couldn't find node=$nodeId in the route, this should never happen") + data.c.extraEdges } - // in any case, we forward the update to the router: if the channel is disabled, the router will remove it from its routing table + // in all cases, we forward the update to the router: if the channel is disabled, the router will remove it from its routing table + // if the channel is not announced (e.g. was from a hint), the router will simply ignore the update router ! failure.update // we return updated assisted routes: they take precedence over the router's routing table - if (failure.update.channelFlags.isEnabled) { - data.c.extraEdges.map { - case edge: BasicEdge if edge.shortChannelId == failure.update.shortChannelId => edge.update(failure.update) - case edge => 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 - data.c.extraEdges - .find { case edge: BasicEdge => edge.shortChannelId == failure.update.shortChannelId } - .foreach { case edge: BasicEdge => router ! ExcludeChannel(ChannelDesc(edge.shortChannelId, edge.sourceNodeId, edge.targetNodeId), Some(nodeParams.routerConf.channelExcludeDuration)) } // we want the exclusion to be router-wide so that sister payments in the case of MPP are aware the channel is faulty - data.c.extraEdges - } + extraEdges1 } def myStop(request: SendPayment, result: Either[PaymentFailed, PaymentSent]): State = { diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/router/Router.scala b/eclair-core/src/main/scala/fr/acinq/eclair/router/Router.scala index 4fd7d6756..8ff883bcc 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/router/Router.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/router/Router.scala @@ -545,9 +545,6 @@ object Router { amountToSend - amount } - /** This method retrieves the channel update that we used when we built the route. */ - def getChannelUpdateForNode(nodeId: PublicKey): Option[ChannelUpdate] = hops.find(_.nodeId == nodeId).map(_.params).collect { case s: ChannelRelayParams.FromAnnouncement => s.channelUpdate } - def printNodes(): String = hops.map(_.nextNodeId).mkString("->") def printChannels(): String = hops.map(_.shortChannelId).mkString("->") 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 index 8d2366752..aac7d9a7c 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentLifecycleSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentLifecycleSpec.scala @@ -629,8 +629,8 @@ class PaymentLifecycleSpec extends BaseRouterSpec { sender.send(paymentFSM, addCompleted(HtlcResult.RemoteFail(UpdateFailHtlc(ByteVector32.Zeroes, 0, failureOnion)))) assert(routerForwarder.expectMsgType[RouteCouldRelay].route.hops.map(_.shortChannelId) == Seq(update_ab, update_bc).map(_.shortChannelId)) - routerForwarder.expectMsg(channelUpdate_cd_disabled) routerForwarder.expectMsg(ExcludeChannel(ChannelDesc(update_cd.shortChannelId, c, d), Some(nodeParams.routerConf.channelExcludeDuration))) + routerForwarder.expectMsg(channelUpdate_cd_disabled) } def testPermanentFailure(router: ActorRef, failure: FailureMessage): Unit = {