Skip to content

Commit

Permalink
Compute max fee before route calculation (#1417)
Browse files Browse the repository at this point in the history 
Move the maximum fee computation outside of `findRoute`: this should be
done earlier in the payment pipeline if we want to allow accurate fee control
for MPP retries.

Right now MPP uses approximations when retrying which can lead to payments
that exceed the maximum configured fees. This is a first step towards
ensuring that this situation cannot happen anymore.
  • Loading branch information
@t-bast
t-bast committed May 20, 2020
1 parent ad44ab3 commit ce3629c
Show file tree
Hide file tree
Showing 8 changed files with 221 additions and 191 deletions.
3 changes: 2 additions & 1 deletion eclair-core/src/main/scala/fr/acinq/eclair/Eclair.scala
View file
Expand Up @@ -210,7 +210,8 @@ class EclairImpl(appKit: Kit) extends Eclair {
}

override def findRoute(targetNodeId: PublicKey, amount: MilliSatoshi, assistedRoutes: Seq[Seq[PaymentRequest.ExtraHop]] = Seq.empty)(implicit timeout: Timeout): Future[RouteResponse] = {
(appKit.router ? RouteRequest(appKit.nodeParams.nodeId, targetNodeId, amount, assistedRoutes)).mapTo[RouteResponse]
val maxFee = RouteCalculation.getDefaultRouteParams(appKit.nodeParams.routerConf).getMaxFee(amount)
(appKit.router ? RouteRequest(appKit.nodeParams.nodeId, targetNodeId, amount, maxFee, assistedRoutes)).mapTo[RouteResponse]
}

override def sendToRoute(amount: MilliSatoshi, recipientAmount_opt: Option[MilliSatoshi], externalId_opt: Option[String], parentId_opt: Option[UUID], invoice: PaymentRequest, finalCltvExpiryDelta: CltvExpiryDelta, route: Seq[PublicKey], trampolineSecret_opt: Option[ByteVector32], trampolineFees_opt: Option[MilliSatoshi], trampolineExpiryDelta_opt: Option[CltvExpiryDelta], trampolineNodes_opt: Seq[PublicKey])(implicit timeout: Timeout): Future[SendPaymentToRouteResponse] = {
Expand Down
11 changes: 7 additions & 4 deletions eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentLifecycle.scala
View file
Expand Up @@ -95,7 +95,7 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
// If the sender already provided a route to the target, no need to involve the router.
self ! RouteResponse(Seq(Route(c.finalPayload.amount, Nil, allowEmpty = true)))
} else {
router ! RouteRequest(c.getRouteRequestStart(nodeParams), c.targetNodeId, c.finalPayload.amount, c.assistedRoutes, routeParams = c.routeParams, ignoreNodes = ignoredNodes)
router ! RouteRequest(c.getRouteRequestStart(nodeParams), c.targetNodeId, c.finalPayload.amount, c.getMaxFee(nodeParams), c.assistedRoutes, routeParams = c.routeParams, ignoreNodes = ignoredNodes)
}
if (cfg.storeInDb) {
paymentsDb.addOutgoingPayment(OutgoingPayment(id, cfg.parentId, cfg.externalId, paymentHash, PaymentType.Standard, c.finalPayload.amount, cfg.recipientAmount, cfg.recipientNodeId, System.currentTimeMillis, cfg.paymentRequest, OutgoingPaymentStatus.Pending))
Expand Down Expand Up @@ -204,12 +204,12 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
case extraHop => extraHop
})
// let's try again, router will have updated its state
router ! RouteRequest(c.getRouteRequestStart(nodeParams), c.targetNodeId, c.finalPayload.amount, assistedRoutes1, ignoreNodes, ignoreChannels, c.routeParams)
router ! RouteRequest(c.getRouteRequestStart(nodeParams), c.targetNodeId, c.finalPayload.amount, c.getMaxFee(nodeParams), assistedRoutes1, ignoreNodes, ignoreChannels, c.routeParams)
ignoreNodes
} 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(c.getRouteRequestStart(nodeParams), c.targetNodeId, c.finalPayload.amount, c.assistedRoutes, ignoreNodes + nodeId, ignoreChannels, c.routeParams)
router ! RouteRequest(c.getRouteRequestStart(nodeParams), c.targetNodeId, c.finalPayload.amount, c.getMaxFee(nodeParams), c.assistedRoutes, ignoreNodes + nodeId, ignoreChannels, c.routeParams)
ignoreNodes + nodeId
}
goto(WAITING_FOR_ROUTE) using WaitingForRoute(s, c, failures :+ RemoteFailure(cfg.fullRoute(route), e), ignoreNodes1, ignoreChannels)
Expand Down Expand Up @@ -267,7 +267,7 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A

private def retry(failure: PaymentFailure, data: WaitingForComplete): FSM.State[PaymentLifecycle.State, PaymentLifecycle.Data] = {
val (ignoreNodes1, ignoreChannels1) = PaymentFailure.updateIgnored(failure, data.ignoreNodes, data.ignoreChannels)
router ! RouteRequest(data.c.getRouteRequestStart(nodeParams), data.c.targetNodeId, data.c.finalPayload.amount, data.c.assistedRoutes, ignoreNodes1, ignoreChannels1, data.c.routeParams)
router ! RouteRequest(data.c.getRouteRequestStart(nodeParams), data.c.targetNodeId, data.c.finalPayload.amount, data.c.getMaxFee(nodeParams), data.c.assistedRoutes, ignoreNodes1, ignoreChannels1, data.c.routeParams)
goto(WAITING_FOR_ROUTE) using WaitingForRoute(data.sender, data.c, data.failures :+ failure, ignoreNodes1, ignoreChannels1)
}

Expand Down Expand Up @@ -340,6 +340,9 @@ object PaymentLifecycle {
routePrefix: Seq[ChannelHop] = Nil) {
require(finalPayload.amount > 0.msat, s"amount must be > 0")

def getMaxFee(nodeParams: NodeParams): MilliSatoshi =
routeParams.getOrElse(RouteCalculation.getDefaultRouteParams(nodeParams.routerConf)).getMaxFee(finalPayload.amount)

/** Returns the node from which the path-finding algorithm should start. */
def getRouteRequestStart(nodeParams: NodeParams): PublicKey = routePrefix match {
case Nil => nodeParams.nodeId
Expand Down
2 changes: 2 additions & 0 deletions eclair-core/src/main/scala/fr/acinq/eclair/router/Monitoring.scala
View file
Expand Up @@ -26,6 +26,7 @@ object Monitoring {

object Metrics {
val FindRouteDuration = Kamon.timer("router.find-route.duration", "Path-finding duration")
val FindRouteErrors = Kamon.counter("router.find-route.errors", "Path-finding errors")
val RouteLength = Kamon.histogram("router.find-route.length", "Path-finding result length")

object QueryChannelRange {
Expand Down Expand Up @@ -69,6 +70,7 @@ object Monitoring {
val Amount = "amount"
val Announced = "announced"
val Direction = "direction"
val Error = "error"
val NumberOfRoutes = "numRoutes"

object Directions {
Expand Down
95 changes: 54 additions & 41 deletions eclair-core/src/main/scala/fr/acinq/eclair/router/RouteCalculation.scala
View file
Expand Up @@ -29,8 +29,9 @@ import fr.acinq.eclair.router.Router._
import fr.acinq.eclair.wire.ChannelUpdate
import fr.acinq.eclair.{ShortChannelId, _}

import scala.annotation.tailrec
import scala.concurrent.duration._
import scala.util.{Random, Try}
import scala.util.{Failure, Random, Success, Try}

object RouteCalculation {

Expand Down Expand Up @@ -71,9 +72,16 @@ object RouteCalculation {

log.info(s"finding a route ${r.source}->${r.target} with assistedChannels={} ignoreNodes={} ignoreChannels={} excludedChannels={}", assistedChannels.keys.mkString(","), r.ignoreNodes.map(_.value).mkString(","), r.ignoreChannels.mkString(","), d.excludedChannels.mkString(","))
log.info(s"finding a route with randomize={} params={}", routesToFind > 1, params)
findRoute(d.graph, r.source, r.target, r.amount, numRoutes = routesToFind, extraEdges = extraEdges, ignoredEdges = ignoredEdges, ignoredVertices = r.ignoreNodes, routeParams = params, currentBlockHeight)
.map(route => ctx.sender ! RouteResponse(route :: Nil))
.recover { case t => ctx.sender ! Status.Failure(t) }
KamonExt.time(Metrics.FindRouteDuration.withTag(Tags.NumberOfRoutes, routesToFind).withTag(Tags.Amount, Tags.amountBucket(r.amount))) {
findRoute(d.graph, r.source, r.target, r.amount, r.maxFee, routesToFind, extraEdges, ignoredEdges, r.ignoreNodes, params, currentBlockHeight) match {
case Success(routes) =>
Metrics.RouteLength.withTag(Tags.Amount, Tags.amountBucket(r.amount)).record(routes.head.length)
ctx.sender ! RouteResponse(routes)
case Failure(t) =>
Metrics.FindRouteErrors.withTag(Tags.Amount, Tags.amountBucket(r.amount)).withTag(Tags.Error, t.getClass.getSimpleName).increment()
ctx.sender ! Status.Failure(t)
}
}
d
}

Expand Down Expand Up @@ -146,63 +154,68 @@ object RouteCalculation {
* @param g graph of the whole network
* @param localNodeId sender node (payer)
* @param targetNodeId target node (final recipient)
* @param amount the amount that will be sent along this route
* @param numRoutes the number of shortest-paths to find
* @param amount the amount that the target node should receive
* @param maxFee the maximum fee of a resulting route
* @param numRoutes the number of routes to find
* @param extraEdges a set of extra edges we want to CONSIDER during the search
* @param ignoredEdges a set of extra edges we want to IGNORE during the search
* @param routeParams a set of parameters that can restrict the route search
* @return the computed route to the destination @targetNodeId
* @return the computed routes to the destination @param targetNodeId
*/
def findRoute(g: DirectedGraph,
localNodeId: PublicKey,
targetNodeId: PublicKey,
amount: MilliSatoshi,
maxFee: MilliSatoshi,
numRoutes: Int,
extraEdges: Set[GraphEdge] = Set.empty,
ignoredEdges: Set[ChannelDesc] = Set.empty,
ignoredVertices: Set[PublicKey] = Set.empty,
routeParams: RouteParams,
currentBlockHeight: Long): Try[Route] = Try {

if (localNodeId == targetNodeId) throw CannotRouteToSelf

def feeBaseOk(fee: MilliSatoshi): Boolean = fee <= routeParams.maxFeeBase

def feePctOk(fee: MilliSatoshi, amount: MilliSatoshi): Boolean = {
val maxFee = amount * routeParams.maxFeePct
fee <= maxFee
currentBlockHeight: Long): Try[Seq[Route]] = Try {
findRouteInternal(g, localNodeId, targetNodeId, amount, maxFee, numRoutes, extraEdges, ignoredEdges, ignoredVertices, routeParams, currentBlockHeight) match {
case Right(routes) => routes.map(route => Route(amount, route.path.map(graphEdgeToHop)))
case Left(ex) => return Failure(ex)
}
}

def feeOk(fee: MilliSatoshi, amount: MilliSatoshi): Boolean = feeBaseOk(fee) || feePctOk(fee, amount)
@tailrec
private def findRouteInternal(g: DirectedGraph,
localNodeId: PublicKey,
targetNodeId: PublicKey,
amount: MilliSatoshi,
maxFee: MilliSatoshi,
numRoutes: Int,
extraEdges: Set[GraphEdge] = Set.empty,
ignoredEdges: Set[ChannelDesc] = Set.empty,
ignoredVertices: Set[PublicKey] = Set.empty,
routeParams: RouteParams,
currentBlockHeight: Long): Either[RouterException, Seq[Graph.WeightedPath]] = {
if (localNodeId == targetNodeId) return Left(CannotRouteToSelf)

def feeOk(fee: MilliSatoshi): Boolean = fee <= maxFee

def lengthOk(length: Int): Boolean = length <= routeParams.routeMaxLength && length <= ROUTE_MAX_LENGTH

def cltvOk(cltv: CltvExpiryDelta): Boolean = cltv <= routeParams.routeMaxCltv

val boundaries: RichWeight => Boolean = { weight =>
feeOk(weight.cost - amount, amount) && lengthOk(weight.length) && cltvOk(weight.cltv)
}

val foundRoutes = KamonExt.time(Metrics.FindRouteDuration.withTag(Tags.NumberOfRoutes, numRoutes).withTag(Tags.Amount, Tags.amountBucket(amount))) {
Graph.yenKshortestPaths(g, localNodeId, targetNodeId, amount, ignoredEdges, ignoredVertices, extraEdges, numRoutes, routeParams.ratios, currentBlockHeight, boundaries).toList
}
foundRoutes match {
case Nil if routeParams.routeMaxLength < ROUTE_MAX_LENGTH => // if not found within the constraints we relax and repeat the search
Metrics.RouteLength.withTag(Tags.Amount, Tags.amountBucket(amount)).record(0)
return findRoute(g, localNodeId, targetNodeId, amount, numRoutes, extraEdges, ignoredEdges, ignoredVertices, routeParams.copy(routeMaxLength = ROUTE_MAX_LENGTH, routeMaxCltv = DEFAULT_ROUTE_MAX_CLTV), currentBlockHeight)
case Nil =>
Metrics.RouteLength.withTag(Tags.Amount, Tags.amountBucket(amount)).record(0)
throw RouteNotFound
case foundRoutes =>
val routes = foundRoutes.find(_.path.size == 1) match {
case Some(directRoute) => directRoute :: Nil
case _ => foundRoutes
}
// At this point 'routes' cannot be empty
val randomizedRoutes = if (routeParams.randomize) Random.shuffle(routes) else routes
val route = randomizedRoutes.head.path.map(graphEdgeToHop)
Metrics.RouteLength.withTag(Tags.Amount, Tags.amountBucket(amount)).record(route.length)
Route(amount, route)
val boundaries: RichWeight => Boolean = { weight => feeOk(weight.cost - amount) && lengthOk(weight.length) && cltvOk(weight.cltv) }

val foundRoutes: Seq[Graph.WeightedPath] = Graph.yenKshortestPaths(g, localNodeId, targetNodeId, amount, ignoredEdges, ignoredVertices, extraEdges, numRoutes, routeParams.ratios, currentBlockHeight, boundaries)
if (foundRoutes.nonEmpty) {
val (directRoutes, indirectRoutes) = foundRoutes.partition(_.path.length == 1)
val routes = if (routeParams.randomize) {
Random.shuffle(directRoutes) ++ Random.shuffle(indirectRoutes)
} else {
directRoutes ++ indirectRoutes
}
Right(routes)
} else if (routeParams.routeMaxLength < ROUTE_MAX_LENGTH) {
// if not found within the constraints we relax and repeat the search
val relaxedRouteParams = routeParams.copy(routeMaxLength = ROUTE_MAX_LENGTH, routeMaxCltv = DEFAULT_ROUTE_MAX_CLTV)
findRouteInternal(g, localNodeId, targetNodeId, amount, maxFee, numRoutes, extraEdges, ignoredEdges, ignoredVertices, relaxedRouteParams, currentBlockHeight)
} else {
Left(RouteNotFound)
}
}

Expand Down
8 changes: 7 additions & 1 deletion eclair-core/src/main/scala/fr/acinq/eclair/router/Router.scala
View file
Expand Up @@ -363,11 +363,17 @@ object Router {
override def fee(amount: MilliSatoshi): MilliSatoshi = fee
}

case class RouteParams(randomize: Boolean, maxFeeBase: MilliSatoshi, maxFeePct: Double, routeMaxLength: Int, routeMaxCltv: CltvExpiryDelta, ratios: Option[WeightRatios])
case class RouteParams(randomize: Boolean, maxFeeBase: MilliSatoshi, maxFeePct: Double, routeMaxLength: Int, routeMaxCltv: CltvExpiryDelta, ratios: Option[WeightRatios]) {
def getMaxFee(amount: MilliSatoshi): MilliSatoshi = {
// The payment fee must satisfy either the flat fee or the percentage fee, not necessarily both.
maxFeeBase.max(amount * maxFeePct)
}
}

case class RouteRequest(source: PublicKey,
target: PublicKey,
amount: MilliSatoshi,
maxFee: MilliSatoshi,
assistedRoutes: Seq[Seq[ExtraHop]] = Nil,
ignoreNodes: Set[PublicKey] = Set.empty,
ignoreChannels: Set[ChannelDesc] = Set.empty,
Expand Down

0 comments on commit ce3629c

Please sign in to comment.