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.

Sign up for GitHub

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

Jump to bottom

Wake up wallet nodes before relaying messages or payments #2865

Merged
merged 13 commits into from
Aug 28, 2024
Merged

Wake up wallet nodes before relaying messages or payments #2865

merged 13 commits into from
Aug 28, 2024

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Jun 11, 2024 •
edited
Loading

This PR ports some of the architecture related to wallet wake-up from feature branches to master. It should be reviewed commit-by-commit, while comparing to feature branches to get a feel for the changes that will be needed on the feature branches to adapt to this architecture.

Note how the PeerReadyNotifier can easily be extended to send push notifications to wallet nodes before waiting for an incoming connection from them.

@t-bast t-bast mentioned this pull request Jun 13, 2024
Merged
@t-bast t-bast force-pushed the wake-up-before-relay branch 2 times, most recently from 89d3a1d to b5a8d98 Compare June 18, 2024 14:25
thomash-acinq
thomash-acinq previously approved these changes Jun 19, 2024
View reviewed changes
@t-bast
Copy link
Member Author

t-bast commented Jun 19, 2024

I amended the last commit which contained a bug in nextWalletNodeId: the case where we potentially relay to a non-blinded wallet node is when the recipient is a ClearRecipient with a non-empty trampoline packet, not a TrampolineRecipient. It's also important in that case that we only attempt wake-up when we're sure the next node is a wallet node, so this currently returns None and must be enriched in feature branches with a call to the push notifications DB to check whether the next node is a wallet.

thomash-acinq
thomash-acinq previously approved these changes Jun 20, 2024
View reviewed changes
@t-bast t-bast force-pushed the wake-up-before-relay branch 3 times, most recently from b02d00a to 3619ce9 Compare June 27, 2024 14:48
@t-bast
Copy link
Member Author

t-bast commented Jul 17, 2024

Rebased without any changes.

thomash-acinq
thomash-acinq previously approved these changes Aug 2, 2024
View reviewed changes
@t-bast
Reorder functions in NodeRelay.scala
cfb6c9c 
This commit doesn't contain any logical change, we just move code to
align with the FSM flow. It makes it easier to follow the progress of
the state machine to always scroll down when advancing states.
@t-bast
Rework node relay FSM flow
54185b0 
We refactor `NodeRelay.scala` to re-order some steps, without making
meaningful functional changes. The steps are:

1. Fully receive the incoming payment
2. Resolve the next node (unwrap blinded paths if needed)
3. Wake-up the next node if necessary (mobile wallet)
4. Relay outgoing payment

Note that we introduce a wake-up step, that will be enriched in future
commits and can be extended to include mobile notifications. The file
is now also easier to follow, as steps are done linearly by simply
scrolling down.
@t-bast
Add wake-up step to channel and message relay
f4c1354 
We allow relaying data to contain a wallet `node_id` instead of an scid.
When that's the case, we start by waking up that wallet node before we
try relaying onion messages or payments.
@t-bast
Create peer-wake-up config section
a395f9f 
Create a dedicated `peer-wake-up` configuration section.
This can be enriched with mobile notification sub-sections.
@t-bast
Make walletNodeId_opt a field in ChannelRelay
3a3d39e 
It turns out that we can keep `requestedShortChannelId_opt` and
`walletNodeId_opt` as fields of this actor instead of resolving
them once and forwarding the value in every function.
@t-bast
Include recipient in NodeRelay.sending
fbc4577 
This field is used in feature branches, so we include it to minimize
the diff.
@t-bast
Introduce PeerReadyNotifier private class
893aac2 
This commit doesn't contain any changes apart from using a private class
to factor common fields of the `PeerReadyNotifier`.
@t-bast
Refactor PeerReadyNotifier
aad3226 
- clean-up `SomePeerConnected` / `SomePeerDisconnected`
- include supervision directly inside the actor itself
@t-bast
Track pending PeerReadyNotifier instances
5613204 
It can be useful to track pending `PeerReadyNotifier` instances to avoid
performing duplicate actions when multiple `PeerReadyNotifier` spawn for
the same peer (e.g. sending a mobile notification).

When a `PeerReadyNotifier` actor is started, it registers itself into a
singleton `PeerReadyManager`, which tells it whether there are other
pending attempts for the same peer.
@t-bast
fixup! Track pending PeerReadyNotifier instances
ea613a9
@t-bast
fixup! Track pending PeerReadyNotifier instances
28e92c2
@t-bast
Move register outside of private class
bfb9090 
This way we can have `isFirstAttempt` as a class field, which makes
sense since this value won't change.
@t-bast
Allow disabling wake-up
3e23b85 
This makes testing easier. We also do small refactorings in the relay
actors without any behavior changes.
@t-bast t-bast merged commit fcd88b0 into master Aug 28, 2024
1 check passed
@t-bast t-bast deleted the wake-up-before-relay branch August 28, 2024 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pm47 pm47 pm47 approved these changes

@thomash-acinq thomash-acinq thomash-acinq left review comments

Assignees
No one assigned
Labels
Optech Make Me Famous!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@t-bast @pm47 @thomash-acinq