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 singleton actor AsyncPaymentTriggerer to monitor when the receiver of an async payment reconnects #2491

Merged
merged 9 commits into from
Jan 2, 2023

Conversation

remyers
Copy link
Contributor

@remyers remyers commented Nov 16, 2022

The new AsyncPaymentTriggerer singleton actor spawns only a single PeerReadyNotifier actor for each peer that is watched. This prevents multiple actors from redundantly polling for when the same peer reconnects.

Each Watcher for a given peer triggers a specific async payment (managed by a NodeRelay actor) and has a unique timeout (in block height). The NodeRelay actor will receive an AsyncPaymentTriggered when the target peer reconnects, or AsyncPaymentTimeout if the timeout block height is reached.

This PR requires a change to allow PeerReadyNotifier to have an infinite timeout so that AsyncPaymentTriggerer can instead manage multiple timeouts for a given peer.

The AsyncPaymentTriggerer singleton must be created before the Relayer actor is created, but requires a reference to the singleton Switchboard actor to spawn PeerReadyNotifier actors. Because the swtichboard is indirectly instantiated with the Relayer reference (via PeerFactory and ChannelFactory), the AsyncPaymentTriggerer actor must be further initialized by sending a Start message with the Switchboard actor reference before being used.

In the future the AsyncPaymentTriggerer actor can be extended to watch for onion messages from async payment receivers, instead of only watching for local peer re-connections. In the onion message case, the paymentHash will be used to identify which Watcher to trigger.

@remyers remyers force-pushed the peer-watcher branch 2 times, most recently from 4b286a2 to a1bd1b4 Compare November 16, 2022 14:56
@codecov-commenter
Copy link

codecov-commenter commented Nov 16, 2022

Codecov Report

Merging #2491 (4e5511d) into master (c2eb357) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2491      +/-   ##
==========================================
+ Coverage   85.11%   85.24%   +0.12%     
==========================================
  Files         203      204       +1     
  Lines       16081    16107      +26     
  Branches      706      689      -17     
==========================================
+ Hits        13688    13731      +43     
+ Misses       2393     2376      -17     
Impacted Files Coverage Δ
...ir-core/src/main/scala/fr/acinq/eclair/Setup.scala 79.03% <100.00%> (+0.22%) ⬆️
...q/eclair/payment/relay/AsyncPaymentTriggerer.scala 100.00% <100.00%> (ø)
...cala/fr/acinq/eclair/payment/relay/NodeRelay.scala 97.87% <100.00%> (+0.61%) ⬆️
...la/fr/acinq/eclair/payment/relay/NodeRelayer.scala 100.00% <100.00%> (ø)
.../scala/fr/acinq/eclair/payment/relay/Relayer.scala 89.36% <100.00%> (-2.13%) ⬇️
...fr/acinq/eclair/wire/protocol/FailureMessage.scala 78.75% <0.00%> (-3.75%) ⬇️
...src/main/scala/fr/acinq/eclair/db/pg/PgUtils.scala 89.39% <0.00%> (-1.52%) ⬇️
...inq/eclair/channel/fsm/CommonFundingHandlers.scala 90.90% <0.00%> (-0.76%) ⬇️
...main/scala/fr/acinq/eclair/io/PeerConnection.scala 86.08% <0.00%> (-0.42%) ⬇️
... and 9 more

@remyers remyers changed the title Add singleton actor AsyncPaymentTrigger to monitor when the receiver of an async payment reconnects Add singleton actor AsyncPaymentTriggerer to monitor when the receiver of an async payment reconnects Nov 16, 2022
@remyers remyers marked this pull request as ready for review November 16, 2022 19:26
@remyers remyers requested a review from t-bast November 16, 2022 19:26
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.

Looks good to me, the design is clean and test coverage looks good.
I did some refactoring and renaming in ac5118b, let me know what you think.

I'll update #2464 to allow disabling the timeout, I think it's useful to have. I'll do it slightly differently than what you did, I think using an Option is more explicit than an infinite duration.

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, we should be able to integrate this once we integrate #2464, good job 👍

remyers and others added 5 commits December 22, 2022 16:46
- extract Start at the top-level
- register only once to EventStream
- handle peer missing in map
- rename classes for clarity
- remove unnecessary comments
- use receiveMessagePartial
- simplify tests
@remyers
Copy link
Contributor Author

remyers commented Dec 29, 2022

Do we need to add cancelasyncpayment to the API? There's currently no way for a trampoline node to query for the list of pending payments. Trampoline nodes should not have to manage this list. Perhaps I should roll back 0c7b049.

Otoh, we will need a way for a node that creates an async payment to cancel a pending payment queued by the trampoline node they designated to hold it. This will likely be accomplished with a custom onion message, but could also be a custom Lightning Message.

@t-bast
Copy link
Member

t-bast commented Dec 29, 2022

Do we need to add cancelasyncpayment to the API? There's currently no way for a trampoline node to query for the list of pending payments. Trampoline nodes should not have to manage this list. Perhaps I should roll back 0c7b049.

I think you should indeed rollback 0c7b049

I think there is a confusion between the sender node and the relaying node here. Cancelling an async payment is done by the sender node telling the relaying node to abort the payment via a custom lightning message or an onion message. The API is for the relaying node operator, but it's not their decision to cancel async payments.

I don't think you should add anything else to this PR, it's just making it harder to review. Follow-up work (e.g. adding the custom cancel_async_payment lightning message) should be done in follow-up PRs.

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! This seems pretty robust and well tested now 👍

@remyers remyers merged commit 33ca262 into ACINQ:master Jan 2, 2023
@remyers remyers deleted the peer-watcher branch January 2, 2023 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants