Do not force-close channels when we cannot communicate with peers #1429
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TheBlueMatt
force-pushed
the
2022-04-drop-no-conn-possible
branch
3 times, most recently
from
dba08e5 to
6741021
Compare
Codecov Report
@@ Coverage Diff @@
## main #1429 +/- ##
=======================================
Coverage 90.88% 90.88%
=======================================
Files 75 75
Lines 41517 41517
Branches 41517 41517
=======================================
Hits 37734 37734
Misses 3783 3783 Continue to review full report at Codecov.
|
| @@ -2170,15 +2170,15 @@ fn channel_monitor_network_test() { | |||
| assert_eq!(nodes[0].node.list_channels().len(), 0); | |||
| assert_eq!(nodes[1].node.list_channels().len(), 1); | |||
| check_closed_event!(nodes[0], 1, ClosureReason::CommitmentTxConfirmed); | |||
| check_closed_event!(nodes[1], 1, ClosureReason::DisconnectedPeer); | |||
| check_closed_event!(nodes[1], 1, ClosureReason::HolderForceClosed); | |||
There was a problem hiding this comment.
FWIW, I think after this we we'll be missing test coverage of ClosureReason::DisconnectedPeer
There was a problem hiding this comment.
Right, well this doesn't change coverage of it for the "peer disconnected before confirmation" reason, only the no_connection_possible reason, so it doesn't really change coverage.
vincenzopalazzo
previously approved these changes
Apr 18, 2022
valentinewallace
previously approved these changes
Apr 19, 2022
TheBlueMatt
dismissed stale reviews from valentinewallace and vincenzopalazzo
via
6902ccb
TheBlueMatt
force-pushed
the
2022-04-drop-no-conn-possible
branch
from
72c29ff to
6902ccb
Compare
|
Squashed |
valentinewallace
previously approved these changes
Apr 20, 2022
|
Squashed. |
TheBlueMatt
force-pushed
the
2022-04-drop-no-conn-possible
branch
from
e15c339 to
5033f80
Compare
arik-so
previously approved these changes
Apr 27, 2022
|
Oops missed that this now depends on #1435. |
TheBlueMatt
force-pushed
the
2022-04-drop-no-conn-possible
branch
from
5033f80 to
99f0855
Compare
TheBlueMatt
added
Seeking Code Review
and removed
blocked on dependent pr
Seeking Code Review
labels
In general, we should never be automatically force-closing our users' channels unless there is some immediate risk of funds loss (ie because of some HTLC(s) which are timing out soon). In any other case, we should trust the user to be able to figure out what is going on and close their channels manually instead of trying to be overly clever and automate closures if we think the channel is useless. In this case, even if a peer has some required feature that does not allow us to communicate with them, there is a strong possibility that some LDK upgrade may allow us to in the future. In the mean time, there is no reason to go on-chain unless the user needs funds immediately. In such a case, the user should already have logic to force-close channels with peers which are not available for any reason.
TheBlueMatt
force-pushed
the
2022-04-drop-no-conn-possible
branch
from
99f0855 to
e39d63c
Compare
|
Not really sure why this ended up based on #1435, but that landed now so rebased this. |
valentinewallace
approved these changes
Apr 28, 2022
arik-so
approved these changes
May 14, 2022
tnull
pushed a commit
to tnull/rust-lightning
that referenced
this pull request
May 17, 2022
…-no-conn-possible
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In general, we should never be automatically force-closing our
users' channels unless there is some immediate risk of funds loss
(ie because of some HTLC(s) which are timing out soon). In any
other case, we should trust the user to be able to figure out what
is going on and close their channels manually instead of trying to
be overly clever and automate closures if we think the channel is
useless.
In this case, even if a peer has some required feature that does
not allow us to communicate with them, there is a strong
possibility that some LDK upgrade may allow us to in the future. In
the mean time, there is no reason to go on-chain unless the user
needs funds immediately. In such a case, the user should already
have logic to force-close channels with peers which are not
available for any reason.