Skip to content

Commit

Permalink
More aggressive channel exclusion (#1441)
Browse files Browse the repository at this point in the history 
* Exclude channels in last payment attempt
* Exclude disabled routing hints
  • Loading branch information
@t-bast
t-bast authored Jun 23, 2020
1 parent 4199128 commit 365d091
Show file tree
Hide file tree
Showing 3 changed files with 107 additions and 31 deletions.
84 changes: 53 additions & 31 deletions eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentLifecycle.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,10 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
val failure = res match {
case Success(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, data)
case _ =>
}
RemoteFailure(cfg.fullRoute(route), e)
case Failure(t) =>
log.warning(s"cannot parse returned error: ${t.getMessage}")
Expand All @@ -168,37 +172,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 ignore1 = if (Announcements.checkSig(failureMessage.update, nodeId)) {
route.getChannelUpdateForNode(nodeId) match {
case Some(u) if u.shortChannelId != failureMessage.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=${failureMessage.update.shortChannelId} update=${failureMessage.update}")
case Some(u) if Announcements.areSame(u, failureMessage.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 = route.hops.find(_.nodeId == nodeId).get.nextNodeId
router ! ExcludeChannel(ChannelDesc(u.shortChannelId, nodeId, nextNodeId))
case Some(u) if PaymentFailure.hasAlreadyFailedOnce(nodeId, 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=${failureMessage.update}")
val nextNodeId = route.hops.find(_.nodeId == nodeId).get.nextNodeId
router ! ExcludeChannel(ChannelDesc(u.shortChannelId, nodeId, nextNodeId))
case Some(u) =>
log.info(s"got a new update for shortChannelId=${u.shortChannelId}: old=$u new=${failureMessage.update}")
case None =>
log.error(s"couldn't find a channel update for node=$nodeId, this should never happen")
}
// in any case, we forward the update to the router
router ! failureMessage.update
// we also update assisted routes, because they take precedence over the router's routing table
val assistedRoutes1 = c.assistedRoutes.map(_.map {
case extraHop: ExtraHop if extraHop.shortChannelId == failureMessage.update.shortChannelId => extraHop.copy(
cltvExpiryDelta = failureMessage.update.cltvExpiryDelta,
feeBase = failureMessage.update.feeBaseMsat,
feeProportionalMillionths = failureMessage.update.feeProportionalMillionths
)
case extraHop => extraHop
})
val assistedRoutes1 = handleUpdate(nodeId, failureMessage, data)
// 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)
ignore
Expand Down Expand Up @@ -266,6 +240,54 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
goto(WAITING_FOR_ROUTE) using WaitingForRoute(data.sender, data.c, data.failures :+ failure, ignore1)
}

/**
* Apply the channel update to our routing table.
*
* @return updated routing hints if applicable.
*/
private def handleUpdate(nodeId: PublicKey, failure: Update, data: WaitingForComplete): Seq[Seq[ExtraHop]] = {
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))
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))
case Some(u) =>
log.info(s"got a new update for shortChannelId=${u.shortChannelId}: old=$u new=${failure.update}")
case None =>
log.error(s"couldn't find a channel update for node=$nodeId, this should never happen")
}
// in any case, we forward the update to the router: if the channel is disabled, the router will remove it from its routing table
router ! failure.update
// we return updated assisted routes: they take precedence over the router's routing table
if (Announcements.isEnabled(failure.update.channelFlags)) {
data.c.assistedRoutes.map(_.map {
case extraHop: ExtraHop if extraHop.shortChannelId == failure.update.shortChannelId => extraHop.copy(
cltvExpiryDelta = failure.update.cltvExpiryDelta,
feeBase = failure.update.feeBaseMsat,
feeProportionalMillionths = failure.update.feeProportionalMillionths
)
case extraHop => extraHop
})
} 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.assistedRoutes.flatMap(r => RouteCalculation.toChannelDescs(r, data.c.targetNodeId))
.find(_.shortChannelId == failure.update.shortChannelId)
.foreach(desc => router ! ExcludeChannel(desc)) // 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.assistedRoutes
}
}

private def myStop(): State = {
stateSpan.foreach(_.finish())
span.finish()
Expand Down
5 changes: 5 additions & 0 deletions eclair-core/src/main/scala/fr/acinq/eclair/router/RouteCalculation.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,11 @@ object RouteCalculation {
}._2
}

def toChannelDescs(extraRoute: Seq[ExtraHop], targetNodeId: PublicKey): Seq[ChannelDesc] = {
val nextNodeIds = extraRoute.map(_.nodeId).drop(1) :+ targetNodeId
extraRoute.zip(nextNodeIds).map { case (hop, nextNodeId) => ChannelDesc(hop.shortChannelId, hop.nodeId, nextNodeId) }
}

/** Bolt 11 routing hints don't include the channel's capacity, so we round up the maximum htlc amount. */
private def htlcMaxToCapacity(htlcMaximum: MilliSatoshi): Satoshi = htlcMaximum.truncateToSatoshi + 1.sat

Expand Down
49 changes: 49 additions & 0 deletions eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentLifecycleSpec.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,29 @@ class PaymentLifecycleSpec extends BaseRouterSpec {
awaitCond(nodeParams.db.payments.getOutgoingPayment(id).exists(_.status.isInstanceOf[OutgoingPaymentStatus.Failed]))
}

test("payment failed (Update in last attempt)") { routerFixture =>
val payFixture = createPaymentLifecycle()
import payFixture._

val request = SendPayment(d, FinalLegacyPayload(defaultAmountMsat, defaultExpiry), 1)
sender.send(paymentFSM, request)
routerForwarder.expectMsg(defaultRouteRequest(nodeParams.nodeId, d))
routerForwarder.forward(routerFixture.router)
awaitCond(paymentFSM.stateName == WAITING_FOR_PAYMENT_COMPLETE)
val WaitingForComplete(_, _, cmd1, Nil, sharedSecrets1, _, _) = paymentFSM.stateData
register.expectMsg(ForwardShortId(channelId_ab, cmd1))

// the node replies with a temporary failure containing the same update as the one we already have (likely a balance issue)
val failure = TemporaryChannelFailure(update_bc)
sender.send(paymentFSM, UpdateFailHtlc(ByteVector32.Zeroes, 0, Sphinx.FailurePacket.create(sharedSecrets1.head._1, failure)))
// we should temporarily exclude that channel
routerForwarder.expectMsg(ExcludeChannel(ChannelDesc(update_bc.shortChannelId, b, c)))
routerForwarder.expectMsg(update_bc)

// this was a single attempt payment
sender.expectMsgType[PaymentFailed]
}

test("payment failed (Update in assisted route)") { routerFixture =>
val payFixture = createPaymentLifecycle()
import payFixture._
Expand Down Expand Up @@ -401,6 +424,32 @@ class PaymentLifecycleSpec extends BaseRouterSpec {
assert(cmd2.cltvExpiry > cmd1.cltvExpiry)
}

test("payment failed (Update disabled in assisted route)") { routerFixture =>
val payFixture = createPaymentLifecycle()
import payFixture._

// we build an assisted route for channel cd
val assistedRoutes = Seq(Seq(ExtraHop(c, channelId_cd, update_cd.feeBaseMsat, update_cd.feeProportionalMillionths, update_cd.cltvExpiryDelta)))
val request = SendPayment(d, FinalLegacyPayload(defaultAmountMsat, defaultExpiry), 1, assistedRoutes = assistedRoutes)
sender.send(paymentFSM, request)
awaitCond(paymentFSM.stateName == WAITING_FOR_ROUTE && nodeParams.db.payments.getOutgoingPayment(id).exists(_.status === OutgoingPaymentStatus.Pending))

routerForwarder.expectMsg(defaultRouteRequest(nodeParams.nodeId, d).copy(assistedRoutes = assistedRoutes))
routerForwarder.forward(routerFixture.router)
awaitCond(paymentFSM.stateName == WAITING_FOR_PAYMENT_COMPLETE)
val WaitingForComplete(_, _, cmd1, Nil, sharedSecrets1, _, _) = paymentFSM.stateData
register.expectMsg(ForwardShortId(channelId_ab, cmd1))

// we disable the channel
val channelUpdate_cd_disabled = makeChannelUpdate(Block.RegtestGenesisBlock.hash, priv_c, d, channelId_cd, CltvExpiryDelta(42), update_cd.htlcMinimumMsat, update_cd.feeBaseMsat, update_cd.feeProportionalMillionths, update_cd.htlcMaximumMsat.get, enable = false)
val failure = ChannelDisabled(channelUpdate_cd_disabled.messageFlags, channelUpdate_cd_disabled.channelFlags, channelUpdate_cd_disabled)
val failureOnion = Sphinx.FailurePacket.wrap(Sphinx.FailurePacket.create(sharedSecrets1(1)._1, failure), sharedSecrets1.head._1)
sender.send(paymentFSM, UpdateFailHtlc(ByteVector32.Zeroes, 0, failureOnion))

routerForwarder.expectMsg(channelUpdate_cd_disabled)
routerForwarder.expectMsg(ExcludeChannel(ChannelDesc(update_cd.shortChannelId, c, d)))
}

def testPermanentFailure(router: ActorRef, failure: FailureMessage): Unit = {
val payFixture = createPaymentLifecycle()
import payFixture._
Expand Down

0 comments on commit 365d091

Please sign in to comment.