Do not retry sending if payment gets confirmed on chain (#1799)

The `PaymentLifecycle` state machine already had that mechanism, but the `MultiPartPaymentLifecycle` didn't.
ACINQ · May 17, 2021 · 0805d51 · 0805d51
1 parent 898c17b
commit 0805d51
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 15 deletions.
diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/payment/send/MultiPartPaymentLifecycle.scala b/eclair-core/src/main/scala/fr/acinq/eclair/payment/send/MultiPartPaymentLifecycle.scala
@@ -16,10 +16,14 @@
 
 package fr.acinq.eclair.payment.send
 
+import java.util.UUID
+import java.util.concurrent.TimeUnit
+
 import akka.actor.{ActorRef, FSM, Props, Status}
 import akka.event.Logging.MDC
 import fr.acinq.bitcoin.ByteVector32
 import fr.acinq.bitcoin.Crypto.PublicKey
+import fr.acinq.eclair.channel.{HtlcOverriddenByLocalCommit, HtlcsTimedoutDownstream, HtlcsWillTimeoutUpstream}
 import fr.acinq.eclair.db.{OutgoingPayment, OutgoingPaymentStatus, PaymentType}
 import fr.acinq.eclair.payment.Monitoring.{Metrics, Tags}
 import fr.acinq.eclair.payment.OutgoingPacket.Upstream
@@ -33,9 +37,6 @@ import fr.acinq.eclair.router.Router._
 import fr.acinq.eclair.wire.protocol._
 import fr.acinq.eclair.{CltvExpiry, FSMDiagnosticActorLogging, Logs, MilliSatoshi, MilliSatoshiLong, NodeParams}
 
-import java.util.UUID
-import java.util.concurrent.TimeUnit
-
 /**
  * Created by t-bast on 18/07/2019.
  */
@@ -112,7 +113,7 @@ class MultiPartPaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig,
       }
 
     case Event(pf: PaymentFailed, d: PaymentProgress) =>
-      if (isFinalRecipientFailure(pf, d)) {
+      if (abortPayment(pf, d)) {
         gotoAbortedOrStop(PaymentAborted(d.request, d.failures ++ pf.failures, d.pending.keySet - pf.id))
       } else {
         val ignore1 = PaymentFailure.updateIgnored(pf.failures, d.ignore)
@@ -130,7 +131,7 @@ class MultiPartPaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig,
 
   when(PAYMENT_IN_PROGRESS) {
     case Event(pf: PaymentFailed, d: PaymentProgress) =>
-      if (isFinalRecipientFailure(pf, d)) {
+      if (abortPayment(pf, d)) {
         gotoAbortedOrStop(PaymentAborted(d.request, d.failures ++ pf.failures, d.pending.keySet - pf.id))
       } else if (d.remainingAttempts == 0) {
         val failure = LocalFailure(Nil, PaymentError.RetryExhausted)
@@ -377,10 +378,14 @@ object MultiPartPaymentLifecycle {
     SendPaymentToRoute(replyTo, Right(route), finalPayload)
   }
 
-  /** When we receive an error from the final recipient, we should fail the whole payment, it's useless to retry. */
-  private def isFinalRecipientFailure(pf: PaymentFailed, d: PaymentProgress): Boolean = pf.failures.collectFirst {
-    case f: RemoteFailure if f.e.originNode == d.request.targetNodeId => true
-  }.getOrElse(false)
+  /** When we receive an error from the final recipient or payment gets settled on chain, we should fail the whole payment, it's useless to retry. */
+  private def abortPayment(pf: PaymentFailed, d: PaymentProgress): Boolean = pf.failures.exists {
+    case f: RemoteFailure => f.e.originNode == d.request.targetNodeId
+    case LocalFailure(_, _: HtlcOverriddenByLocalCommit) => true
+    case LocalFailure(_, _: HtlcsWillTimeoutUpstream) => true
+    case LocalFailure(_, _: HtlcsTimedoutDownstream) => true
+    case _ => false
+  }
 
   private def remainingToSend(nodeParams: NodeParams, request: SendMultiPartPayment, pending: Iterable[Route]): (MilliSatoshi, MilliSatoshi) = {
     val sentAmount = pending.map(_.amount).sum

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
@@ -141,12 +141,12 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
     }
     // we only retry if the error isn't fatal, and we haven't exhausted the max number of retried
     val doRetry = !isFatal && (d.failures.size + 1 < d.c.maxAttempts)
+    val localFailure = LocalFailure(cfg.fullRoute(d.route), t)
     if (doRetry) {
       log.info(s"received an error message from local, trying to use a different channel (failure=${t.getMessage})")
-      val failure = LocalFailure(cfg.fullRoute(d.route), t)
-      retry(failure, d)
+      retry(localFailure, d)
     } else {
-      onFailure(d.c.replyTo, PaymentFailed(id, paymentHash, d.failures :+ LocalFailure(cfg.fullRoute(d.route), t)))
+      onFailure(d.c.replyTo, PaymentFailed(id, paymentHash, d.failures :+ localFailure))
       stop(FSM.Normal)
     }
   }

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
@@ -18,9 +18,9 @@ package fr.acinq.eclair.payment
 
 import akka.actor.{ActorContext, ActorRef, Status}
 import akka.testkit.{TestFSMRef, TestProbe}
-import fr.acinq.bitcoin.{Block, Crypto}
+import fr.acinq.bitcoin.{Block, ByteVector32, Crypto}
 import fr.acinq.eclair._
-import fr.acinq.eclair.channel.{ChannelFlags, ChannelUnavailable}
+import fr.acinq.eclair.channel.{ChannelFlags, ChannelUnavailable, HtlcsTimedoutDownstream}
 import fr.acinq.eclair.crypto.Sphinx
 import fr.acinq.eclair.db.{FailureSummary, FailureType, OutgoingPaymentStatus}
 import fr.acinq.eclair.payment.OutgoingPacket.Upstream
@@ -36,8 +36,8 @@ import fr.acinq.eclair.wire.protocol._
 import org.scalatest.Outcome
 import org.scalatest.funsuite.FixtureAnyFunSuiteLike
 import scodec.bits.{ByteVector, HexStringSyntax}
-
 import java.util.UUID
+
 import scala.concurrent.duration._
 
 /**
@@ -384,6 +384,20 @@ class MultiPartPaymentLifecycleSpec extends TestKitBaseClass with FixtureAnyFunS
     assert(result.failures.length === 1)
   }
 
+  test("abort if payment gets settled on chain") { f =>
+    import f._
+
+    val payment = SendMultiPartPayment(sender.ref, randomBytes32, e, finalAmount, expiry, 5, routeParams = Some(routeParams))
+    sender.send(payFsm, payment)
+    router.expectMsgType[RouteRequest]
+    router.send(payFsm, RouteResponse(Seq(Route(finalAmount, hop_ab_1 :: hop_be :: Nil))))
+    childPayFsm.expectMsgType[SendPaymentToRoute]
+
+    val (failedId, failedRoute) = payFsm.stateData.asInstanceOf[PaymentProgress].pending.head
+    val result = abortAfterFailure(f, PaymentFailed(failedId, paymentHash, Seq(LocalFailure(failedRoute.hops, HtlcsTimedoutDownstream(channelId = ByteVector32.One, htlcs = Set.empty)))))
+    assert(result.failures.length === 1)
+  }
+
   test("abort if recipient sends error during retry") { f =>
     import f._