From 5b19e586014920d5ed9fa1071b6af3eacd81c0ee Mon Sep 17 00:00:00 2001 From: Bastien Teinturier <31281497+t-bast@users.noreply.github.com> Date: Wed, 21 Dec 2022 09:16:34 +0100 Subject: [PATCH] Improve `payinvoice` API response (#2525) When sending an outgoing multi-part payment, we forward the preimage back to the sender as soon as we receive the first `update_fulfill_htlc`. This is particularly useful when relaying trampoline payments, to be able to propagate the fulfill upstream as early as possible. However this meant that callers of the HTTP API would receive this preimage event instead of the final payment result, which was quite bad. We now disable this first event when used with the `--blocking` argument, which ensures that the API always returns the payment result. Fixes #2389 --- docs/release-notes/eclair-vnext.md | 1 + .../src/main/scala/fr/acinq/eclair/Eclair.scala | 10 +++------- .../acinq/eclair/payment/relay/NodeRelay.scala | 8 ++++---- .../send/MultiPartPaymentLifecycle.scala | 11 +++++++---- .../eclair/payment/send/PaymentInitiator.scala | 12 +++++------- .../payment/MultiPartPaymentLifecycleSpec.scala | 2 +- .../eclair/payment/PaymentInitiatorSpec.scala | 2 +- .../fr/acinq/eclair/api/ApiServiceSpec.scala | 17 ++--------------- 8 files changed, 24 insertions(+), 39 deletions(-) diff --git a/docs/release-notes/eclair-vnext.md b/docs/release-notes/eclair-vnext.md index 4ed7fff8c..1ca68d5aa 100644 --- a/docs/release-notes/eclair-vnext.md +++ b/docs/release-notes/eclair-vnext.md @@ -10,6 +10,7 @@ - `audit` now accepts `--count` and `--skip` parameters to limit the number of retrieved items (#2474, #2487) - `sendtoroute` removes the `--trampolineNodes` argument and implicitly uses a single trampoline hop (#2480) +- `payinvoice` always returns the payment result when used with `--blocking`, even when using MPP (#2525) ### Miscellaneous improvements and bug fixes diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/Eclair.scala b/eclair-core/src/main/scala/fr/acinq/eclair/Eclair.scala index ed4f63a02..374f90083 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/Eclair.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/Eclair.scala @@ -42,7 +42,6 @@ import fr.acinq.eclair.payment._ import fr.acinq.eclair.payment.receive.MultiPartHandler.ReceiveStandardPayment import fr.acinq.eclair.payment.relay.Relayer.{ChannelBalance, GetOutgoingChannels, OutgoingChannels, RelayFees} import fr.acinq.eclair.payment.send.ClearRecipient -import fr.acinq.eclair.payment.send.MultiPartPaymentLifecycle.PreimageReceived import fr.acinq.eclair.payment.send.PaymentInitiator._ import fr.acinq.eclair.router.Router import fr.acinq.eclair.router.Router._ @@ -114,7 +113,7 @@ trait Eclair { def send(externalId_opt: Option[String], amount: MilliSatoshi, invoice: Bolt11Invoice, maxAttempts_opt: Option[Int] = None, maxFeeFlat_opt: Option[Satoshi] = None, maxFeePct_opt: Option[Double] = None, pathFindingExperimentName_opt: Option[String] = None)(implicit timeout: Timeout): Future[UUID] - def sendBlocking(externalId_opt: Option[String], amount: MilliSatoshi, invoice: Bolt11Invoice, maxAttempts_opt: Option[Int] = None, maxFeeFlat_opt: Option[Satoshi] = None, maxFeePct_opt: Option[Double] = None, pathFindingExperimentName_opt: Option[String] = None)(implicit timeout: Timeout): Future[Either[PreimageReceived, PaymentEvent]] + def sendBlocking(externalId_opt: Option[String], amount: MilliSatoshi, invoice: Bolt11Invoice, maxAttempts_opt: Option[Int] = None, maxFeeFlat_opt: Option[Satoshi] = None, maxFeePct_opt: Option[Double] = None, pathFindingExperimentName_opt: Option[String] = None)(implicit timeout: Timeout): Future[PaymentEvent] def sendWithPreimage(externalId_opt: Option[String], recipientNodeId: PublicKey, amount: MilliSatoshi, paymentPreimage: ByteVector32 = randomBytes32(), maxAttempts_opt: Option[Int] = None, maxFeeFlat_opt: Option[Satoshi] = None, maxFeePct_opt: Option[Double] = None, pathFindingExperimentName_opt: Option[String] = None)(implicit timeout: Timeout): Future[UUID] @@ -370,13 +369,10 @@ class EclairImpl(appKit: Kit) extends Eclair with Logging { } } - override def sendBlocking(externalId_opt: Option[String], amount: MilliSatoshi, invoice: Bolt11Invoice, maxAttempts_opt: Option[Int], maxFeeFlat_opt: Option[Satoshi], maxFeePct_opt: Option[Double], pathFindingExperimentName_opt: Option[String])(implicit timeout: Timeout): Future[Either[PreimageReceived, PaymentEvent]] = { + override def sendBlocking(externalId_opt: Option[String], amount: MilliSatoshi, invoice: Bolt11Invoice, maxAttempts_opt: Option[Int], maxFeeFlat_opt: Option[Satoshi], maxFeePct_opt: Option[Double], pathFindingExperimentName_opt: Option[String])(implicit timeout: Timeout): Future[PaymentEvent] = { createSendPaymentRequest(externalId_opt, amount, invoice, maxAttempts_opt, maxFeeFlat_opt, maxFeePct_opt, pathFindingExperimentName_opt) match { case Left(ex) => Future.failed(ex) - case Right(req) => (appKit.paymentInitiator ? req.copy(blockUntilComplete = true)).map { - case e: PreimageReceived => Left(e) - case e: PaymentEvent => Right(e) - } + case Right(req) => (appKit.paymentInitiator ? req.copy(blockUntilComplete = true)).mapTo[PaymentEvent] } } diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/payment/relay/NodeRelay.scala b/eclair-core/src/main/scala/fr/acinq/eclair/payment/relay/NodeRelay.scala index 89dcded82..d0859921e 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/payment/relay/NodeRelay.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/payment/relay/NodeRelay.scala @@ -75,7 +75,7 @@ object NodeRelay { override def spawnOutgoingPayFSM(context: ActorContext[Command], cfg: SendPaymentConfig, multiPart: Boolean): ActorRef = { if (multiPart) { - context.toClassic.actorOf(MultiPartPaymentLifecycle.props(nodeParams, cfg, router, paymentFactory)) + context.toClassic.actorOf(MultiPartPaymentLifecycle.props(nodeParams, cfg, publishPreimage = true, router, paymentFactory)) } else { context.toClassic.actorOf(PaymentLifecycle.props(nodeParams, cfg, router, register)) } @@ -107,7 +107,7 @@ object NodeRelay { } } - def validateRelay(nodeParams: NodeParams, upstream: Upstream.Trampoline, payloadOut: IntermediatePayload.NodeRelay.Standard): Option[FailureMessage] = { + private def validateRelay(nodeParams: NodeParams, upstream: Upstream.Trampoline, payloadOut: IntermediatePayload.NodeRelay.Standard): Option[FailureMessage] = { val fee = nodeFee(nodeParams.relayParams.minTrampolineFees, payloadOut.amountToForward) if (upstream.amountIn - payloadOut.amountToForward < fee) { Some(TrampolineFeeInsufficient) @@ -125,7 +125,7 @@ object NodeRelay { } /** Compute route params that honor our fee and cltv requirements. */ - def computeRouteParams(nodeParams: NodeParams, amountIn: MilliSatoshi, expiryIn: CltvExpiry, amountOut: MilliSatoshi, expiryOut: CltvExpiry): RouteParams = { + private def computeRouteParams(nodeParams: NodeParams, amountIn: MilliSatoshi, expiryIn: CltvExpiry, amountOut: MilliSatoshi, expiryOut: CltvExpiry): RouteParams = { nodeParams.routerConf.pathFindingExperimentConf.getRandomConf().getDefaultRouteParams .modify(_.boundaries.maxFeeProportional).setTo(0) // we disable percent-based max fee calculation, we're only interested in collecting our node fee .modify(_.boundaries.maxCltv).setTo(expiryIn - expiryOut) @@ -137,7 +137,7 @@ object NodeRelay { * This helper method translates relaying errors (returned by the downstream nodes) to a BOLT 4 standard error that we * should return upstream. */ - def translateError(nodeParams: NodeParams, failures: Seq[PaymentFailure], upstream: Upstream.Trampoline, nextPayload: IntermediatePayload.NodeRelay.Standard): Option[FailureMessage] = { + private def translateError(nodeParams: NodeParams, failures: Seq[PaymentFailure], upstream: Upstream.Trampoline, nextPayload: IntermediatePayload.NodeRelay.Standard): Option[FailureMessage] = { val routeNotFound = failures.collectFirst { case f@LocalFailure(_, _, RouteNotFound) => f }.nonEmpty val routingFeeHigh = upstream.amountIn - nextPayload.amountToForward >= nodeFee(nodeParams.relayParams.minTrampolineFees, nextPayload.amountToForward) * 5 failures match { 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 index 97fb1d872..8f0b09090 100644 --- 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 @@ -20,7 +20,7 @@ import akka.actor.{ActorRef, FSM, Props, Status} import akka.event.Logging.MDC import fr.acinq.bitcoin.scalacompat.ByteVector32 import fr.acinq.eclair.channel.{HtlcOverriddenByLocalCommit, HtlcsTimedoutDownstream, HtlcsWillTimeoutUpstream} -import fr.acinq.eclair.db.{OutgoingPayment, OutgoingPaymentStatus, PaymentType} +import fr.acinq.eclair.db.{OutgoingPayment, OutgoingPaymentStatus} import fr.acinq.eclair.payment.Monitoring.{Metrics, Tags} import fr.acinq.eclair.payment.OutgoingPaymentPacket.Upstream import fr.acinq.eclair.payment.PaymentSent.PartialPayment @@ -41,7 +41,7 @@ import java.util.concurrent.TimeUnit * Sender for a multi-part payment (see https://github.com/lightningnetwork/lightning-rfc/blob/master/04-onion-routing.md#basic-multi-part-payments). * The payment will be split into multiple sub-payments that will be sent in parallel. */ -class MultiPartPaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: ActorRef, paymentFactory: PaymentInitiator.PaymentFactory) extends FSMDiagnosticActorLogging[MultiPartPaymentLifecycle.State, MultiPartPaymentLifecycle.Data] { +class MultiPartPaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, publishPreimage: Boolean, router: ActorRef, paymentFactory: PaymentInitiator.PaymentFactory) extends FSMDiagnosticActorLogging[MultiPartPaymentLifecycle.State, MultiPartPaymentLifecycle.Data] { import MultiPartPaymentLifecycle._ @@ -211,7 +211,9 @@ class MultiPartPaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, } private def gotoSucceededOrStop(d: PaymentSucceeded): State = { - d.request.replyTo ! PreimageReceived(paymentHash, d.preimage) + if (publishPreimage) { + d.request.replyTo ! PreimageReceived(paymentHash, d.preimage) + } if (d.pending.isEmpty) { myStop(d.request, Right(cfg.createPaymentSent(d.request.recipient, d.preimage, d.parts))) } else @@ -292,7 +294,8 @@ class MultiPartPaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, object MultiPartPaymentLifecycle { - def props(nodeParams: NodeParams, cfg: SendPaymentConfig, router: ActorRef, paymentFactory: PaymentInitiator.PaymentFactory) = Props(new MultiPartPaymentLifecycle(nodeParams, cfg, router, paymentFactory)) + def props(nodeParams: NodeParams, cfg: SendPaymentConfig, publishPreimage: Boolean, router: ActorRef, paymentFactory: PaymentInitiator.PaymentFactory) = + Props(new MultiPartPaymentLifecycle(nodeParams, cfg, publishPreimage, router, paymentFactory)) /** * Send a payment to a given node. The payment may be split into multiple child payments, for which a path-finding diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentInitiator.scala b/eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentInitiator.scala index f0fc579f2..59a5decc3 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentInitiator.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentInitiator.scala @@ -58,7 +58,7 @@ class PaymentInitiator(nodeParams: NodeParams, outgoingPaymentFactory: PaymentIn if (!nodeParams.features.invoiceFeatures().areSupported(recipient.features)) { sender() ! PaymentFailed(paymentId, r.paymentHash, LocalFailure(r.recipientAmount, Nil, UnsupportedFeatures(recipient.features)) :: Nil) } else if (Features.canUseFeature(nodeParams.features.invoiceFeatures(), recipient.features, Features.BasicMultiPartPayment)) { - val fsm = outgoingPaymentFactory.spawnOutgoingMultiPartPayment(context, paymentCfg) + val fsm = outgoingPaymentFactory.spawnOutgoingMultiPartPayment(context, paymentCfg, publishPreimage = !r.blockUntilComplete) fsm ! MultiPartPaymentLifecycle.SendMultiPartPayment(self, recipient, r.maxAttempts, r.routeParams) context become main(pending + (paymentId -> PendingPaymentToNode(sender(), r))) } else { @@ -168,8 +168,6 @@ class PaymentInitiator(nodeParams: NodeParams, outgoingPaymentFactory: PaymentIn context become main(pending - pf.id) } - case _: MultiPartPaymentLifecycle.PreimageReceived => // we received the preimage, but we wait for the PaymentSent event that will contain more data - case ps: PaymentSent => pending.get(ps.id).foreach(pp => { pp.sender ! ps pp match { @@ -208,7 +206,7 @@ class PaymentInitiator(nodeParams: NodeParams, outgoingPaymentFactory: PaymentIn val trampolineHop = NodeHop(r.trampolineNodeId, r.recipientNodeId, trampolineExpiryDelta, trampolineFees) val paymentCfg = SendPaymentConfig(paymentId, paymentId, None, r.paymentHash, r.recipientNodeId, Upstream.Local(paymentId), Some(r.invoice), storeInDb = true, publishEvent = false, recordPathFindingMetrics = true) buildTrampolineRecipient(r, trampolineHop).map { recipient => - val fsm = outgoingPaymentFactory.spawnOutgoingMultiPartPayment(context, paymentCfg) + val fsm = outgoingPaymentFactory.spawnOutgoingMultiPartPayment(context, paymentCfg, publishPreimage = false) fsm ! MultiPartPaymentLifecycle.SendMultiPartPayment(self, recipient, nodeParams.maxPaymentAttempts, r.routeParams) } } @@ -222,7 +220,7 @@ object PaymentInitiator { } trait MultiPartPaymentFactory extends PaymentFactory { - def spawnOutgoingMultiPartPayment(context: ActorContext, cfg: SendPaymentConfig): ActorRef + def spawnOutgoingMultiPartPayment(context: ActorContext, cfg: SendPaymentConfig, publishPreimage: Boolean): ActorRef } case class SimplePaymentFactory(nodeParams: NodeParams, router: ActorRef, register: ActorRef) extends MultiPartPaymentFactory { @@ -230,8 +228,8 @@ object PaymentInitiator { context.actorOf(PaymentLifecycle.props(nodeParams, cfg, router, register)) } - override def spawnOutgoingMultiPartPayment(context: ActorContext, cfg: SendPaymentConfig): ActorRef = { - context.actorOf(MultiPartPaymentLifecycle.props(nodeParams, cfg, router, this)) + override def spawnOutgoingMultiPartPayment(context: ActorContext, cfg: SendPaymentConfig, publishPreimage: Boolean): ActorRef = { + context.actorOf(MultiPartPaymentLifecycle.props(nodeParams, cfg, publishPreimage, router, this)) } } 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 index ae9707958..7657279f8 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/payment/MultiPartPaymentLifecycleSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/payment/MultiPartPaymentLifecycleSpec.scala @@ -70,7 +70,7 @@ class MultiPartPaymentLifecycleSpec extends TestKitBaseClass with FixtureAnyFunS val cfg = SendPaymentConfig(id, id, Some("42"), paymentHash, randomKey().publicKey, Upstream.Local(id), None, storeInDb = true, publishEvent = true, recordPathFindingMetrics = true) val nodeParams = TestConstants.Alice.nodeParams val (childPayFsm, router, sender, eventListener, metricsListener) = (TestProbe(), TestProbe(), TestProbe(), TestProbe(), TestProbe()) - val paymentHandler = TestFSMRef(new MultiPartPaymentLifecycle(nodeParams, cfg, router.ref, FakePaymentFactory(childPayFsm))) + val paymentHandler = TestFSMRef(new MultiPartPaymentLifecycle(nodeParams, cfg, publishPreimage = true, router.ref, FakePaymentFactory(childPayFsm))) system.eventStream.subscribe(eventListener.ref, classOf[PaymentEvent]) system.eventStream.subscribe(metricsListener.ref, classOf[PathFindingExperimentMetrics]) withFixture(test.toNoArgTest(FixtureParam(cfg, nodeParams, paymentHandler, router, sender, childPayFsm, eventListener, metricsListener))) diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentInitiatorSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentInitiatorSpec.scala index 5e4215a35..272643467 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentInitiatorSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentInitiatorSpec.scala @@ -90,7 +90,7 @@ class PaymentInitiatorSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike payFsm.ref ! cfg payFsm.ref } - override def spawnOutgoingMultiPartPayment(context: ActorContext, cfg: SendPaymentConfig): ActorRef = { + override def spawnOutgoingMultiPartPayment(context: ActorContext, cfg: SendPaymentConfig, publishPreimage: Boolean): ActorRef = { multiPartPayFsm.ref ! cfg multiPartPayFsm.ref } diff --git a/eclair-node/src/test/scala/fr/acinq/eclair/api/ApiServiceSpec.scala b/eclair-node/src/test/scala/fr/acinq/eclair/api/ApiServiceSpec.scala index 280954583..79d649567 100644 --- a/eclair-node/src/test/scala/fr/acinq/eclair/api/ApiServiceSpec.scala +++ b/eclair-node/src/test/scala/fr/acinq/eclair/api/ApiServiceSpec.scala @@ -43,7 +43,6 @@ import fr.acinq.eclair.io.{NodeURI, Peer} import fr.acinq.eclair.message.OnionMessages import fr.acinq.eclair.payment._ import fr.acinq.eclair.payment.relay.Relayer.ChannelBalance -import fr.acinq.eclair.payment.send.MultiPartPaymentLifecycle.PreimageReceived import fr.acinq.eclair.payment.send.PaymentInitiator.SendPaymentToRouteResponse import fr.acinq.eclair.router.Router import fr.acinq.eclair.router.Router.{HopRelayParams, PredefinedNodeRoute} @@ -630,21 +629,9 @@ class ApiServiceSpec extends AnyFunSuite with ScalatestRouteTest with IdiomaticM val eclair = mock[Eclair] val mockService = new MockService(eclair) - eclair.sendBlocking(any, any, any, any, any, any, any)(any[Timeout]).returns(Future.successful(Left(PreimageReceived(ByteVector32.Zeroes, ByteVector32.One)))) - Post("/payinvoice", FormData("invoice" -> invoice, "blocking" -> "true").toEntity) ~> - addCredentials(BasicHttpCredentials("", mockApi().password)) ~> - Route.seal(mockService.payInvoice) ~> - check { - assert(handled) - assert(status == OK) - val response = entityAs[String] - val expected = """{"paymentHash":"0000000000000000000000000000000000000000000000000000000000000000","paymentPreimage":"0100000000000000000000000000000000000000000000000000000000000000"}""" - assert(response == expected) - } - val uuid = UUID.fromString("487da196-a4dc-4b1e-92b4-3e5e905e9f3f") val paymentSent = PaymentSent(uuid, ByteVector32.Zeroes, ByteVector32.One, 25 msat, aliceNodeId, Seq(PaymentSent.PartialPayment(uuid, 21 msat, 1 msat, ByteVector32.Zeroes, None, TimestampMilli(1553784337711L)))) - eclair.sendBlocking(any, any, any, any, any, any, any)(any[Timeout]).returns(Future.successful(Right(paymentSent))) + eclair.sendBlocking(any, any, any, any, any, any, any)(any[Timeout]).returns(Future.successful(paymentSent)) Post("/payinvoice", FormData("invoice" -> invoice, "blocking" -> "true").toEntity) ~> addCredentials(BasicHttpCredentials("", mockApi().password)) ~> Route.seal(mockService.payInvoice) ~> @@ -657,7 +644,7 @@ class ApiServiceSpec extends AnyFunSuite with ScalatestRouteTest with IdiomaticM } val paymentFailed = PaymentFailed(uuid, ByteVector32.Zeroes, failures = Seq.empty, timestamp = TimestampMilli(1553784963659L)) - eclair.sendBlocking(any, any, any, any, any, any, any)(any[Timeout]).returns(Future.successful(Right(paymentFailed))) + eclair.sendBlocking(any, any, any, any, any, any, any)(any[Timeout]).returns(Future.successful(paymentFailed)) Post("/payinvoice", FormData("invoice" -> invoice, "blocking" -> "true").toEntity) ~> addCredentials(BasicHttpCredentials("", mockApi().password)) ~> Route.seal(mockService.payInvoice) ~>