-
Notifications
You must be signed in to change notification settings - Fork 421
Allow outgoing splice request while disconnected #4122
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
base: main
Are you sure you want to change the base?
Allow outgoing splice request while disconnected #4122
Conversation
👋 Thanks for assigning @jkczyz as a reviewer! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4122 +/- ##
==========================================
+ Coverage 88.72% 88.75% +0.03%
==========================================
Files 180 180
Lines 135528 135891 +363
Branches 135528 135891 +363
==========================================
+ Hits 120241 120615 +374
+ Misses 12517 12505 -12
- Partials 2770 2771 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
🔔 3rd Reminder Hey @jkczyz! This PR has been waiting for your review. |
🔔 4th Reminder Hey @jkczyz! This PR has been waiting for your review. |
🔔 5th Reminder Hey @jkczyz! This PR has been waiting for your review. |
🔔 6th Reminder Hey @jkczyz! This PR has been waiting for your review. |
Splices negotiated with 0 confirmations require that we immediately lock it after exchanging `tx_signatures`.
This is crucial for peers that serve liquidity for low-availability (i.e., mobile) nodes. We should allow users to queue a splice request while the peer is offline, such that it is negotiated once reconnected. Note that there currently isn't a way to time out/cancel these requests, this is planned for the near future.
Since we don't yet support contributing to an incoming splice, we need to make sure we attempt our splice negotiation eventually if the counterparty was also attempting a splice at the same time but they won the quiescence tie-breaker. Since only one pending splice (without RBF) is allowed at a time, we do this after the existing splice becomes locked.
We'll use this in the next commit to test that we'll send a stfu message for a splice we intend to initiate upon reconnecting.
4bd54f0
to
0095e2a
Compare
Needs rebase now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some nits.
|| self.context.channel_state.is_awaiting_quiescence() | ||
|| self.context.channel_state.is_local_stfu_sent() | ||
{ | ||
log_info!(logger, "Channel is either pending or already quiescent"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is meant by "pending" here? Shouldn't we only be able to reach here if the channel is funded?
if self.context.is_live() { | ||
Ok(Some(self.send_stfu(logger)?)) | ||
} else { | ||
log_info!(logger, "Waiting for peer reconnection to send stfu"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this log and the one above be debug
given that is what is used at the start of the method? Without that context these messages might be unclear.
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
This is crucial for peers that serve liquidity for low-availability (i.e., mobile) nodes. We should allow users to queue a splice request while the peer is offline, such that it is negotiated once reconnected. Note that there currently isn't a way to time out/cancel these requests, this is planned for the near future.
The test added also includes coverage for 0-conf splices, which didn't work because they lacked test coverage to begin with.
Depends on #4079.
Fixes #1621 (comment).