Trampoline/MPP API changes #1297

t-bast · 2020-01-30T10:26:12Z

Update the API to allow users to easily use MPP and/or Trampoline.

Note that the second commit is completely unrelated: it fixes a small race condition in an E2E test and adds debug output (see commit message).

Let a sender manually split a payment and specify a trampoline route.

I'm seeing a few rare failures in those tests in the CI. Adding more output to the assertions will help troubleshoot those. Fix two flaky tests where the order of payment parts could be different, resulting in a failed equality test.

codecov-io · 2020-01-30T10:42:56Z

Codecov Report

Merging #1297 into master will increase coverage by 0.82%.
The diff coverage is 86%.

@@            Coverage Diff             @@
##           master    #1297      +/-   ##
==========================================
+ Coverage   77.33%   78.15%   +0.82%     
==========================================
  Files         144      144              
  Lines       10095    10412     +317     
  Branches      399      416      +17     
==========================================
+ Hits         7807     8138     +331     
+ Misses       2288     2274      -14

Impacted Files	Coverage Δ
...in/scala/fr/acinq/eclair/api/JsonSerializers.scala	`95.45% <ø> (ø)`	⬆️
...cala/fr/acinq/eclair/db/sqlite/SqliteAuditDb.scala	`99.06% <100%> (ø)`	⬆️
...e/src/main/scala/fr/acinq/eclair/api/Service.scala	`70.98% <100%> (+0.5%)`	⬆️
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala	`55.78% <64.28%> (+1.76%)`	⬆️
...r/acinq/eclair/payment/send/PaymentInitiator.scala	`95.89% <92%> (-0.67%)`	⬇️
...clair/blockchain/electrum/ElectrumClientPool.scala	`74.19% <0%> (-4.31%)`	⬇️
...in/scala/fr/acinq/eclair/channel/Commitments.scala	`90.32% <0%> (-0.36%)`	⬇️
...nq/eclair/blockchain/electrum/ElectrumWallet.scala	`80.75% <0%> (-0.25%)`	⬇️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala	`83.56% <0%> (+0.45%)`	⬆️
...clair/payment/send/MultiPartPaymentLifecycle.scala	`98.2% <0%> (+1.19%)`	⬆️
... and 4 more

araspitzu

Nice update to the API but there is something unclear to me: does the caller have the ability to select how to split the payment into parts? Because i see the amount interacting with recipientAmount (which is the final amount of the invoice) but the caller can only specify one amount which seem to force all subpart payments have the same amount.

eclair-core/src/main/scala/fr/acinq/eclair/Eclair.scala

araspitzu · 2020-01-30T10:57:33Z

eclair-core/src/main/scala/fr/acinq/eclair/Eclair.scala

-    externalId_opt match {
-      case Some(externalId) if externalId.length > externalIdMaxLength => Future.failed(new IllegalArgumentException("externalId is too long: cannot exceed 66 characters"))
-      case _ => (appKit.paymentInitiator ? SendPaymentRequest(amount, paymentHash, route.last, 1, finalCltvExpiryDelta, invoice_opt, externalId_opt, route)).mapTo[UUID]
+  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] = {


I'm not a big fan of functions with so many parameters (this one has 11) but i'm not sure if we can do better here, maybe a small encapsulating case class that names the trampoline parameters i.e TrampolineParams

Yeah I don't really know how to deal with that. The API does require a lot of parameters when you want to do trampoline + MPP.
My view at the moment is that the Eclair trait reflects the plain API parameters and its job is to convert to something nice for the payment initiator.
I don't know if we want to move that grouping to the Service.scala or not...

I don't really see a big advantage to moving all those args to a case class constructor.

t-bast · 2020-01-30T12:25:48Z

does the caller have the ability to select how to split the payment into parts?

Yes the goal here is that the caller makes one call per payment part.
Here is an example Trampoline+MPP:

alice-eclair-cli sendtoroute --amountMsat=170060000 --route=$ALICE_ID,$BOB_ID --trampolineNodes=$BOB_ID,$DAVE_ID --trampolineFeesMsat=100000 --trampolineCltvExpiry=144 --finalCltvExpiry=16 --invoice=$INVOICE
alice-eclair-cli sendtoroute --amountMsat=140040000 --parentId=$PARENT_ID --trampolineSecret=$SECRET --route=$ALICE_ID,$BOB_ID --trampolineNodes=$BOB_ID,$DAVE_ID --trampolineFeesMsat=100000 --trampolineCltvExpiry=144 --finalCltvExpiry=16 --invoice=$INVOICE

t-bast · 2020-01-30T12:26:29Z

I have example scripts at https://github.com/t-bast/lightning-cfg and I'm about to push new ones that use the new API with MPP/Trampoline.

araspitzu · 2020-01-30T12:55:35Z

does the caller have the ability to select how to split the payment into parts?

Yes the goal here is that the caller makes one call per payment part.
Here is an example Trampoline+MPP:

alice-eclair-cli sendtoroute --amountMsat=170060000 --route=$ALICE_ID,$BOB_ID --trampolineNodes=$BOB_ID,$DAVE_ID --trampolineFeesMsat=100000 --trampolineCltvExpiry=144 --finalCltvExpiry=16 --invoice=$INVOICE
alice-eclair-cli sendtoroute --amountMsat=140040000 --parentId=$PARENT_ID --trampolineSecret=$SECRET --route=$ALICE_ID,$BOB_ID --trampolineNodes=$BOB_ID,$DAVE_ID --trampolineFeesMsat=100000 --trampolineCltvExpiry=144 --finalCltvExpiry=16 --invoice=$INVOICE

Nice, so i suppose if the caller doesn't want to use MPP it would just omit the amount field. I think we need to document this very well providing examples on how to use the API in scenarios like: legacy, trampoline, MPP, trampoline + MPP. Also we got to make very clear that if the caller wants to perform a multipart payment it needs to call the API multiple times (each per part) specifying the parentId of the payment.

t-bast · 2020-01-30T13:25:36Z

As discussed offline, a probably good future way of doing this would be to create stateful APIs for MPP and Trampoline.

The user would do something like:

> start-mpp-session
{ "paymentId":"id1", "paymentSecret":"s1" }
> send-shard --paymentId=id1 --route=...
> send-shard --paymentId=id1 --route=...
> end-mpp-session

But this is quite a lot of re-work of the API, so it's probably something we would do in a second version, once we get user feedback (if we have enough users that want to use the CLI to do MPP or Trampoline).

t-bast · 2020-01-30T13:26:33Z

I think we need to document this very well providing examples on how to use the API in scenarios like: legacy, trampoline, MPP, trampoline + MPP

I have a note to update the documentation and API once we release the new eclair-core version :)

If we're relaying multiple HTLCs for the same payment_hash, we need to list all of those. The previous code only handled that when Trampoline was used.

t-bast · 2020-01-30T15:36:14Z

I added a small fix for an issue that I just spotted related to relaying MPP without Trampoline.
It's not directly related to the API work, but it was small enough that I didn't want to do another PR.

eclair-core/src/main/scala/fr/acinq/eclair/db/sqlite/SqliteAuditDb.scala

t-bast added 2 commits January 29, 2020 17:54

Update pay APIs for MPP and Trampoline

c58d22d

Let a sender manually split a payment and specify a trampoline route.

Improve MPP/Trampoline IntegrationSpec

a9b95ba

I'm seeing a few rare failures in those tests in the CI. Adding more output to the assertions will help troubleshoot those. Fix two flaky tests where the order of payment parts could be different, resulting in a failed equality test.

t-bast requested review from araspitzu and pm47 January 30, 2020 10:26

araspitzu reviewed Jan 30, 2020

View reviewed changes

pm47 previously approved these changes Jan 30, 2020

View reviewed changes

t-bast added 2 commits January 30, 2020 16:30

fixup! Update pay APIs for MPP and Trampoline

196cc5c

Fix a small bug for AuditDB listRelayed

44324c4

If we're relaying multiple HTLCs for the same payment_hash, we need to list all of those. The previous code only handled that when Trampoline was used.

t-bast dismissed pm47’s stale review via 44324c4 January 30, 2020 15:32

pm47 reviewed Jan 31, 2020

View reviewed changes

eclair-core/src/main/scala/fr/acinq/eclair/db/sqlite/SqliteAuditDb.scala Outdated Show resolved Hide resolved

Improve listRelayed further

16dd4ff

pm47 approved these changes Jan 31, 2020

View reviewed changes

t-bast merged commit 48ad9b3 into master Jan 31, 2020

t-bast deleted the trampoline-api-changes branch January 31, 2020 10:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trampoline/MPP API changes #1297

Trampoline/MPP API changes #1297

t-bast commented Jan 30, 2020

codecov-io commented Jan 30, 2020 •

edited

Loading

araspitzu left a comment

araspitzu Jan 30, 2020

t-bast Jan 30, 2020

pm47 Jan 30, 2020

t-bast commented Jan 30, 2020

t-bast commented Jan 30, 2020

araspitzu commented Jan 30, 2020

t-bast commented Jan 30, 2020

t-bast commented Jan 30, 2020

t-bast commented Jan 30, 2020

Trampoline/MPP API changes #1297

Trampoline/MPP API changes #1297

Conversation

t-bast commented Jan 30, 2020

codecov-io commented Jan 30, 2020 • edited Loading

Codecov Report

araspitzu left a comment

Choose a reason for hiding this comment

araspitzu Jan 30, 2020

Choose a reason for hiding this comment

t-bast Jan 30, 2020

Choose a reason for hiding this comment

pm47 Jan 30, 2020

Choose a reason for hiding this comment

t-bast commented Jan 30, 2020

t-bast commented Jan 30, 2020

araspitzu commented Jan 30, 2020

t-bast commented Jan 30, 2020

t-bast commented Jan 30, 2020

t-bast commented Jan 30, 2020

codecov-io commented Jan 30, 2020 •

edited

Loading