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.

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

Add initial support for async payment trampoline relay #2435

Merged
merged 15 commits into from
Sep 29, 2022

Conversation

remyers
Copy link
Contributor

@remyers remyers commented Sep 23, 2022

This is the first step in adding "async payments" as described in issue #2424 .

@remyers
Copy link
Contributor Author

remyers commented Sep 23, 2022

Some Questions for my next set of changes to this PR:

  • What information should go in the AsyncPaymentFeatures onion tlv?

I currently use invoice feature bits because an empty tlv seemed wrong. The sender will look for the AsyncPaymentPrototype feature bit in the invoice, but the trampoline relay does not have access to the invoice (for privacy). That's why the sender must add a tlv specifically to the onion of the trampoline relay.

  • When I change the AsyncPaymentTrigger message to be a non-spec LightningMessage that is routed to the NodeRelayer, should it be keyed off of the nodeRelayPacket.outerPayload.paymentSecret for the node relayer ?

  • Would it be better to publish an event when an AsyncPaymentTrigger non-spec LightningMessage is received, or route it to the set of running NodeRelayer actors ?

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, you found your way around the right parts of the codebase, most of this looks correct!

When I change the AsyncPaymentTrigger message to be a non-spec LightningMessage that is routed to the NodeRelayer, should it be keyed off of the nodeRelayPacket.outerPayload.paymentSecret for the node relayer ?

I don't think we need to define a LightningMessage for the AsyncPaymentTrigger.
This won't be something we receive from the recipient peer when it comes back online.
It will just be an internal component that will send this internal message to the event stream.

Let me know what you think, I think the flow should be the following:

  • we should add a singleton actor named something like AsyncPaymentWatcher
  • when the NodeRelay component goes to the waitForTrigger behavior, it should send a NotifyWhenPeerOnline(replyTo, remoteNodeId, paymentHash) to the AsyncPaymentWatcher
  • the job of the AsyncPaymentWatcher will be to check when the peer comes back online, and when the peer is back online, to send AsyncPaymentTrigger/RelayAsyncPayment to the corresponding replyTo actor

What I don't know yet is if this should be a new actor or if we should enrich an existing actor with that new responsibility.

@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2022

Codecov Report

Merging #2435 (f670676) into master (ba2b928) will increase coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2435      +/-   ##
==========================================
+ Coverage   84.71%   84.86%   +0.15%     
==========================================
  Files         199      198       -1     
  Lines       15748    15796      +48     
  Branches      656      680      +24     
==========================================
+ Hits        13341    13406      +65     
+ Misses       2407     2390      -17     
Impacted Files Coverage Δ
.../scala/fr/acinq/eclair/payment/PaymentEvents.scala 97.61% <ø> (ø)
.../scala/fr/acinq/eclair/payment/relay/Relayer.scala 89.13% <ø> (ø)
...core/src/main/scala/fr/acinq/eclair/Features.scala 99.27% <100.00%> (+0.02%) ⬆️
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala 93.26% <100.00%> (+0.12%) ⬆️
...cala/fr/acinq/eclair/payment/relay/NodeRelay.scala 96.55% <100.00%> (+0.61%) ⬆️
...a/fr/acinq/eclair/wire/protocol/PaymentOnion.scala 99.25% <100.00%> (+0.02%) ⬆️
...acinq/eclair/channel/fsm/DualFundingHandlers.scala 80.43% <0.00%> (-2.18%) ⬇️
...scala/fr/acinq/eclair/router/BalanceEstimate.scala 98.91% <0.00%> (-1.09%) ⬇️
...main/scala/fr/acinq/eclair/io/PeerConnection.scala 86.02% <0.00%> (-0.37%) ⬇️
...in/scala/fr/acinq/eclair/channel/fsm/Channel.scala 86.16% <0.00%> (-0.12%) ⬇️
... and 11 more

@remyers
Copy link
Contributor Author

remyers commented Sep 28, 2022

RE: @t-bast 's point earlier about the possibility of a channel closure if the receiver does not trigger the payment and the sender is not back online within cancel-safety-before-timeout-blocks of when it times out.

An async payment sender should probably set the payment expiry well beyond their LSP's expected hold-timeout-blocks (default: 1008 blocks, ~7 days). If the default async payment expiry was something like twice the default hold-timeout-blocks, then the sender would need to be online once within a 6 day period before the payment expires to prevent a channel closure. They would also have a 7 day period before the LSP's timeout to manually cancel the payment.

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, a last couple of nits and we should be good to go 🚀

@t-bast t-bast marked this pull request as ready for review September 28, 2022 15:30
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Don't forget to use github's "squash and merge" option, this is good to go 👍

@remyers remyers merged commit afdaf46 into ACINQ:master Sep 29, 2022
@remyers remyers deleted the async-payments branch September 29, 2022 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants