Negotiate close even if we already consider the channel closed. #4559
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
rustyrussell
force-pushed
the
guilt/htlc-leftover-while-closingd
branch
from
e51988c
to
c247f9f
Compare
cdecker
force-pushed
the
guilt/htlc-leftover-while-closingd
branch
from
c247f9f
to
875d2c8
Compare
|
Rebased on top of |
cdecker
force-pushed
the
guilt/htlc-leftover-while-closingd
branch
from
0caf809
to
f7f6b60
Compare
|
This opened a longstanding issue with "I consider your channel closed so I will no longer talk to you about it!". I will fix that on top of this too, since it's been a flaw for a while. Edit: done |
rustyrussell
force-pushed
the
guilt/htlc-leftover-while-closingd
branch
from
f7f6b60
to
38fb12c
Compare
rustyrussell
changed the title
|
Fixed, all ready for review. |
rustyrussell
force-pushed
the
guilt/htlc-leftover-while-closingd
branch
3 times, most recently
from
b13e72b
to
2e83d0e
Compare
This allows us to ensure a packet is read by the other end, but we don't read anything else from them or write anything to them. Using '+' is similar, but because it closes the connection, the peer might notice before receiving the packet (such as if it does a write). Signed-off-by: Rusty Russell <[email protected]>
This was turned by at random by CI: 1. Alice has sent shutdown, but it still waiting for revoke_and_ack. 2. Bob has sent and received shutdown, and sent revoke_and_ack, so it considers it time for signature exchange. 3. Disconnect before Alice received revoke_and_ack. 4. Reconnect, Bob is in closingd, which doesn't rexmit revoke_and_ack. 5. Timeout. Signed-off-by: Rusty Russell <[email protected]>
It handles all the cases of retransmission, and in the normal case retransmits shutdown and immediately returns for us to run closingd. This is actually far simpler and reduces code duplication. [ Includes fixup to stop warn_unused_result from Christian ] Signed-off-by: Rusty Russell <[email protected]> Changelog-Fixed: Protocol: We could get stuck on signature exchange if we needed to retransmit the final revoke_and_ack.
In particular, if one side sees the final CLOSING_SIGNED and the other doesn't, we won't talk when it reconnects. Signed-off-by: Rusty Russell <[email protected]>
Let the callers do that (only channeld needs to do this). We temporarily send an error on unknown reestablish in openingd, as this mimic previous behavior and avoids breaking tests (it does leave a BROKEN message in the logs though, so test_funding_external_wallet_corners needs to ignore that for now. Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
This supports reestablish on a closed channel: we tell channeld to respond to the reestablish message appropriately, then close the channel. Signed-off-by: Rusty Russell <[email protected]>
It simply uses connectd to send an error if it doesn't know anything about the channel. Signed-off-by: Rusty Russell <[email protected]>
This lets us transition (with a few supporting changes) to closingd, which will happily let them mutual close with us. We already handle the case where this mutual close is redundant (for packet loss), so this is easy. Signed-off-by: Rusty Russell <[email protected]> Changelog-Added: Protocol: We will now reestablish and negotiate mutual close on channels we've already closed (great if peer has lost their database).
…ain. Signed-off-by: Rusty Russell <[email protected]>
rustyrussell
force-pushed
the
guilt/htlc-leftover-while-closingd
branch
from
2e83d0e
to
015ab60
Compare
|
ACK 015ab60 |
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.
This was initially found by CI: one peer got the final signature and the other didn't, so one was closing and the other wasn't. This is generally harmless (the one who thinks it's closed broadcasts, so the peer will see it eventually), but it's painful if they lost information and were relying on the reconnection to provide it.
This adds a test for that, but also allows us to renegotiate closing even if we consider the channel completely closed (in which case, openingd will receive the REESTABLISH msg). I didn't support this for dualopend yet, since there the potential channel and the connection are quite closely tied, and it's hard to dispose of the (uncreated) channel without closing the connection.
It's a nice-to-have, but not vital.