Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Accept onion failure without a channel_update #2854

Merged
merged 2 commits into from
Jun 4, 2024
Merged

Accept onion failure without a channel_update #2854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fd47ba5
Accept onion failure without a `channel_update`
t-bast May 7, 2024
2816203
Rename `update_opt` field
t-bast Jun 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,7 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with
case PostRevocationAction.RejectHtlc(add) =>
log.debug("rejecting incoming htlc {}", add)
// NB: we don't set commit = true, we will sign all updates at once afterwards.
self ! CMD_FAIL_HTLC(add.id, Right(TemporaryChannelFailure(d.channelUpdate)), commit = true)
self ! CMD_FAIL_HTLC(add.id, Right(TemporaryChannelFailure(Some(d.channelUpdate))), commit = true)
case PostRevocationAction.RelayFailure(result) =>
log.debug("forwarding {} to relayer", result)
relayer ! result
Expand Down
11 changes: 7 additions & 4 deletions eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentEvents.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,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 }
.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. */
Expand All @@ -211,7 +211,7 @@ object PaymentFailure {
case RemoteFailure(_, _, Sphinx.DecryptedFailurePacket(nodeId, _: Node)) =>
ignore + nodeId
case RemoteFailure(_, hops, Sphinx.DecryptedFailurePacket(nodeId, failureMessage: Update)) =>
if (Announcements.checkSig(failureMessage.update, nodeId)) {
if (failureMessage.update_opt.forall(update => Announcements.checkSig(update, nodeId))) {
val shouldIgnore = failureMessage match {
case _: TemporaryChannelFailure => true
case _: ChannelDisabled => true
Expand All @@ -224,7 +224,7 @@ object PaymentFailure {
ignore
}
} else {
// This node is fishy, it gave us a bad signature, so let's filter it out.
// 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, _)) =>
Expand Down Expand Up @@ -257,7 +257,10 @@ 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)) => current.updated(f.update.shortChannelId, f.update)
case RemoteFailure(_, _, Sphinx.DecryptedFailurePacket(_, f: Update)) => f.update_opt match {
case Some(update) => current.updated(update.shortChannelId, update)
case None => current
}
case _ => current
}
}
Expand Down
26 changes: 13 additions & 13 deletions eclair-core/src/main/scala/fr/acinq/eclair/payment/relay/ChannelRelay.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,15 +71,15 @@ object ChannelRelay {
*/
def translateLocalError(error: Throwable, channelUpdate_opt: Option[ChannelUpdate]): FailureMessage = {
(error, channelUpdate_opt) match {
case (_: ExpiryTooSmall, Some(channelUpdate)) => ExpiryTooSoon(channelUpdate)
case (_: ExpiryTooSmall, Some(channelUpdate)) => ExpiryTooSoon(Some(channelUpdate))
case (_: ExpiryTooBig, _) => ExpiryTooFar()
case (_: InsufficientFunds, Some(channelUpdate)) => TemporaryChannelFailure(channelUpdate)
case (_: TooManyAcceptedHtlcs, Some(channelUpdate)) => TemporaryChannelFailure(channelUpdate)
case (_: HtlcValueTooHighInFlight, Some(channelUpdate)) => TemporaryChannelFailure(channelUpdate)
case (_: LocalDustHtlcExposureTooHigh, Some(channelUpdate)) => TemporaryChannelFailure(channelUpdate)
case (_: RemoteDustHtlcExposureTooHigh, Some(channelUpdate)) => TemporaryChannelFailure(channelUpdate)
case (_: FeerateTooDifferent, Some(channelUpdate)) => TemporaryChannelFailure(channelUpdate)
case (_: ChannelUnavailable, Some(channelUpdate)) if !channelUpdate.channelFlags.isEnabled => ChannelDisabled(channelUpdate.messageFlags, channelUpdate.channelFlags, channelUpdate)
case (_: InsufficientFunds, Some(channelUpdate)) => TemporaryChannelFailure(Some(channelUpdate))
case (_: TooManyAcceptedHtlcs, Some(channelUpdate)) => TemporaryChannelFailure(Some(channelUpdate))
case (_: HtlcValueTooHighInFlight, Some(channelUpdate)) => TemporaryChannelFailure(Some(channelUpdate))
case (_: LocalDustHtlcExposureTooHigh, Some(channelUpdate)) => TemporaryChannelFailure(Some(channelUpdate))
case (_: RemoteDustHtlcExposureTooHigh, Some(channelUpdate)) => TemporaryChannelFailure(Some(channelUpdate))
case (_: FeerateTooDifferent, Some(channelUpdate)) => TemporaryChannelFailure(Some(channelUpdate))
case (_: ChannelUnavailable, Some(channelUpdate)) if !channelUpdate.channelFlags.isEnabled => ChannelDisabled(channelUpdate.messageFlags, channelUpdate.channelFlags, Some(channelUpdate))
case (_: ChannelUnavailable, None) => PermanentChannelFailure()
case _ => TemporaryNodeFailure()
}
Expand All @@ -91,7 +91,7 @@ object ChannelRelay {
case f: HtlcResult.RemoteFailMalformed => CMD_FAIL_HTLC(originHtlcId, Right(createBadOnionFailure(f.fail.onionHash, f.fail.failureCode)), commit = true)
case _: HtlcResult.OnChainFail => CMD_FAIL_HTLC(originHtlcId, Right(PermanentChannelFailure()), commit = true)
case HtlcResult.ChannelFailureBeforeSigned => CMD_FAIL_HTLC(originHtlcId, Right(PermanentChannelFailure()), commit = true)
case f: HtlcResult.DisconnectedBeforeSigned => CMD_FAIL_HTLC(originHtlcId, Right(TemporaryChannelFailure(f.channelUpdate)), commit = true)
case f: HtlcResult.DisconnectedBeforeSigned => CMD_FAIL_HTLC(originHtlcId, Right(TemporaryChannelFailure(Some(f.channelUpdate))), commit = true)
}
}

Expand Down Expand Up @@ -294,16 +294,16 @@ class ChannelRelay private(nodeParams: NodeParams,
case None =>
RelayFailure(CMD_FAIL_HTLC(r.add.id, Right(UnknownNextPeer()), commit = true))
case Some(c) if !c.channelUpdate.channelFlags.isEnabled =>
RelayFailure(CMD_FAIL_HTLC(r.add.id, Right(ChannelDisabled(c.channelUpdate.messageFlags, c.channelUpdate.channelFlags, c.channelUpdate)), commit = true))
RelayFailure(CMD_FAIL_HTLC(r.add.id, Right(ChannelDisabled(c.channelUpdate.messageFlags, c.channelUpdate.channelFlags, Some(c.channelUpdate))), commit = true))
case Some(c) if r.amountToForward < c.channelUpdate.htlcMinimumMsat =>
RelayFailure(CMD_FAIL_HTLC(r.add.id, Right(AmountBelowMinimum(r.amountToForward, c.channelUpdate)), commit = true))
RelayFailure(CMD_FAIL_HTLC(r.add.id, Right(AmountBelowMinimum(r.amountToForward, Some(c.channelUpdate))), commit = true))
case Some(c) if r.expiryDelta < c.channelUpdate.cltvExpiryDelta =>
RelayFailure(CMD_FAIL_HTLC(r.add.id, Right(IncorrectCltvExpiry(r.outgoingCltv, c.channelUpdate)), commit = true))
RelayFailure(CMD_FAIL_HTLC(r.add.id, Right(IncorrectCltvExpiry(r.outgoingCltv, Some(c.channelUpdate))), commit = true))
case Some(c) if r.relayFeeMsat < nodeFee(c.channelUpdate.relayFees, r.amountToForward) &&
// fees also do not satisfy the previous channel update for `enforcementDelay` seconds after current update
(TimestampSecond.now() - c.channelUpdate.timestamp > nodeParams.relayParams.enforcementDelay ||
outgoingChannel_opt.flatMap(_.prevChannelUpdate).forall(c => r.relayFeeMsat < nodeFee(c.relayFees, r.amountToForward))) =>
RelayFailure(CMD_FAIL_HTLC(r.add.id, Right(FeeInsufficient(r.add.amountMsat, c.channelUpdate)), commit = true))
RelayFailure(CMD_FAIL_HTLC(r.add.id, Right(FeeInsufficient(r.add.amountMsat, Some(c.channelUpdate))), commit = true))
case Some(c: OutgoingChannel) =>
val origin = Origin.ChannelRelayedHot(addResponseAdapter.toClassic, r.add, r.amountToForward)
val nextBlindingKey_opt = r.payload match {
Expand Down
92 changes: 55 additions & 37 deletions eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentLifecycle.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,11 +181,18 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
router ! Router.RouteCouldRelay(stoppedRoute)
}
failureMessage match {
case TemporaryChannelFailure(update, _) =>
case TemporaryChannelFailure(update_opt, _) =>
route.hops.find(_.nodeId == nodeId) match {
case Some(failingHop) if HopRelayParams.areSame(failingHop.params, HopRelayParams.FromAnnouncement(update), ignoreHtlcSize = true) =>
router ! Router.ChannelCouldNotRelay(stoppedRoute.amount, failingHop)
case _ => // otherwise the relay parameters may have changed, so it's not necessarily a liquidity issue
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 _ => ()
}
case _ => // other errors should not be used for liquidity issues
}
Expand Down Expand Up @@ -227,7 +234,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 failure = RemoteFailure(request.amount, route.fullRoute, e)
if (Announcements.checkSig(failureMessage.update, nodeId)) {
if (failureMessage.update_opt.forall(update => Announcements.checkSig(update, nodeId))) {
val recipient1 = handleUpdate(nodeId, failureMessage, d)
val ignore1 = PaymentFailure.updateIgnored(failure, ignore)
// let's try again, router will have updated its state
Expand All @@ -240,8 +247,8 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
goto(WAITING_FOR_ROUTE) using WaitingForRoute(request.copy(recipient = recipient1), failures :+ failure, 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}")
// this node is fishy, it gave us a bad channel update signature: let's filter it out.
log.warning(s"got bad signature from node=$nodeId update=${failureMessage.update_opt}")
request match {
case _: SendPaymentToRoute =>
log.error("unexpected retry during SendPaymentToRoute")
Expand Down Expand Up @@ -289,38 +296,49 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
val extraEdges1 = data.route.hops.find(_.nodeId == nodeId) match {
case Some(hop) => hop.params match {
case ann: HopRelayParams.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)
failure.update_opt match {
case Some(update) if ann.channelUpdate.shortChannelId != 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, update.shortChannelId, update)
case Some(update) if Announcements.areSame(ann.channelUpdate, 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))
case Some(_) 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_opt)
router ! ExcludeChannel(ChannelDesc(ann.channelUpdate.shortChannelId, nodeId, hop.nextNodeId), Some(nodeParams.routerConf.channelExcludeDuration))
case Some(update) =>
log.info("got a new update for shortChannelId={}: old={} new={}", ann.channelUpdate.shortChannelId, ann.channelUpdate, update)
case None =>
// this isn't a relay parameter issue, so it's probably a liquidity issue
log.info("update not provided for shortChannelId={}", ann.channelUpdate.shortChannelId)
router ! ExcludeChannel(ChannelDesc(ann.channelUpdate.shortChannelId, nodeId, hop.nextNodeId), Some(nodeParams.routerConf.channelExcludeDuration))
}
data.recipient.extraEdges
case _: HopRelayParams.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.recipient.extraEdges.map {
case edge: ExtraEdge if edge.sourceNodeId == nodeId && edge.targetNodeId == hop.nextNodeId => edge.update(failure.update)
case edge: ExtraEdge => 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.recipient.extraEdges
.find(edge => edge.sourceNodeId == nodeId && edge.targetNodeId == hop.nextNodeId)
.foreach(edge => router ! ExcludeChannel(ChannelDesc(edge.shortChannelId, edge.sourceNodeId, edge.targetNodeId), Some(nodeParams.routerConf.channelExcludeDuration)))
// we remove this edge for our next payment attempt
data.recipient.extraEdges.filterNot(edge => edge.sourceNodeId == nodeId && edge.targetNodeId == hop.nextNodeId)
case hint: HopRelayParams.FromHint =>
failure.update_opt match {
case Some(update) =>
log.info("received an update for a routing hint (shortChannelId={} nodeId={} enabled={} update={})", update.shortChannelId, nodeId, update.channelFlags.isEnabled, failure.update_opt)
if (update.channelFlags.isEnabled) {
data.recipient.extraEdges.map {
case edge: ExtraEdge if edge.sourceNodeId == nodeId && edge.targetNodeId == hop.nextNodeId => edge.update(update)
case edge: ExtraEdge => 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.recipient.extraEdges
.find(edge => edge.sourceNodeId == nodeId && edge.targetNodeId == hop.nextNodeId)
.foreach(edge => router ! ExcludeChannel(ChannelDesc(edge.shortChannelId, edge.sourceNodeId, edge.targetNodeId), Some(nodeParams.routerConf.channelExcludeDuration)))
// we remove this edge for our next payment attempt
data.recipient.extraEdges.filterNot(edge => edge.sourceNodeId == nodeId && edge.targetNodeId == hop.nextNodeId)
}
case None =>
// this is most likely a liquidity issue, we remove this edge for our next payment attempt
data.recipient.extraEdges.filterNot(edge => edge.sourceNodeId == nodeId && edge.targetNodeId == hop.nextNodeId)
}
}
case None =>
Expand Down
Loading
Loading