mirrors/eclair
1
0
Fork 0
mirror of https://github.com/ACINQ/eclair.git synced 2024-11-19 01:43:22 +01:00
Code Issues Projects Releases Wiki Activity

Improve payinvoice API response (#2525)

Browse Source 
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
This commit is contained in:
Bastien Teinturier 2022-12-21 09:16:34 +01:00 committed by GitHub
parent 656812e8ca
commit 5b19e58601
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 24 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
docs/release-notes/eclair-vnext.md
View File

@ -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

10
eclair-core/src/main/scala/fr/acinq/eclair/Eclair.scala
View File

@ -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]
}
}

8
eclair-core/src/main/scala/fr/acinq/eclair/payment/relay/NodeRelay.scala
View File

@ -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 {

11
eclair-core/src/main/scala/fr/acinq/eclair/payment/send/MultiPartPaymentLifecycle.scala
View File

@ -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

12
eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentInitiator.scala
View File

@ -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))
}
}

2
eclair-core/src/test/scala/fr/acinq/eclair/payment/MultiPartPaymentLifecycleSpec.scala
View File

@ -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)))

2
eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentInitiatorSpec.scala
View File

@ -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
}

17
eclair-node/src/test/scala/fr/acinq/eclair/api/ApiServiceSpec.scala
View File

@ -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) ~>