Reset the channel state before restarting the channel establishment process. #2983
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2983 +/- ##
==========================================
+ Coverage 89.15% 89.18% +0.03%
==========================================
Files 118 118
Lines 97521 98325 +804
Branches 97521 98325 +804
==========================================
+ Hits 86941 87690 +749
- Misses 8343 8398 +55
Partials 2237 2237 ☔ View full report in Codecov by Sentry. |
|
I don't think this is the right approach - from the user's perspective they created a channel, funded it, and then suddenly get told to generate a new funding transaction? |
|
Updated from pr2983.01 to pr2983.02 (diff): Updates:
As we discussed on Discord, I have introduced logic to get funding from context if present. And I would love to get some views on the approach and to confirm if it's the right way. I will soon update the PR with a doc update, and introduce tests as soon as I get a rough Approach ACK. Thanks! |
lightning/src/ln/channel.rs
Outdated
| @@ -2123,6 +2123,10 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider { | |||
| self.channel_state > ChannelState::NegotiatingFunding(NegotiatingFundingFlags::OUR_INIT_SENT) | |||
| } | |||
|
|
|||
| pub(super) fn reset_state(&mut self) { | |||
There was a problem hiding this comment.
I think the point of fetching the funding info from the channel is so we dont have to reset the state and can just repeat the messages when the peer comes online.
There was a problem hiding this comment.
I agree with your point.
But my idea was to use channel_state as a syncing parameter between us and the counterparty’s knowledge of the outbound channel.
Also, resetting channel_state allows for preserving the current flow of checks including the have_received_message check that causes the panic in #2982.
There was a problem hiding this comment.
Also, resetting channel_state allows for preserving the current flow of checks including the have_received_message check that causes the panic in #2982.
I'm not sure why we want to preserve the current checks, though? Re-opening a channel after the peer disconnected is a fundamentally different flow from the normal one. eg we'll need to check that the AcceptChannel we received is the same as the one we previously received, not set fields based on it.
There was a problem hiding this comment.
I think that makes sense. Having a parallel but distinct flow to handle "retries" sounds like a more correct way to go forward with it.
Let me try to come up with an approach. Thanks for the pointer!
There was a problem hiding this comment.
Created a new flow to handle the outbound channel handshake post disconnection/reconnection in 3dfc379
Thanks a lot for all the pointers & suggestions!
1. This field saves info about the previously received AcceptChannel and previously created FundingTransaction.
- If our channel_state has moved forward, we check if it is at the negotiation stage, and that we have access to previously received accept_channel_msg. We panic only when both conditions fail.
1. If we had an acceptchannel earlier, we check if it matches the one received just now. If not we error, because the channel parameters have changed and that warrants renegotiating parameters again. 2. If they do match we simply skip the rest of the function because we had updated those values already.
- Introduce a new enum that tracks if the Funding Transaction is a previously received transaction or a newly created one. - Update the flow and sanity checks accordingly to allow using previously used transaction if present.
|
Updated from pr2983.02 to pr2983.03 (diff): Updates:
Instead of resetting The approach and subsequent changes are explained in depth in the commit messages. @TheBlueMatt |
|
Can you add a test which demonstrates the panic and also exercises the new flow to retry messages? |
|
Looks like this still will at a minimum generate a |
|
Dropping from 0.0.123 as this didn't finish nearly in time, we can do #3034 for 0.0.123. |
|
Closing this PR in favor of #3034, which addresses issue #2982 by removing the OutboundChannel for which the peer was disconnected after messages were exchanged and a funding transaction was created. Rationale: During each channel handshake (involving messages like OpenChannel, AcceptChannel, FundingCreated, etc.), the Pubkeys in these messages change. Consequently, previous messages and their associated funding transactions become obsolete. This means that even if a funding transaction was previously created for an outbound channel, if the peer gets disconnected, creating a new funding transaction becomes necessary. However, this process offers no advantage over simply removing the disconnected channel and initiating a new Channel handshake flow later when needed. |
Fixes #2982
When our counterparty disconnects during channel creation, we could have progressed along the channel creation process.
In such cases, retrying to send the OpenChannel message would fail because the channel state had progressed, leading to a panic in the
get_open_channel.To prevent this issue, introduce a new function,
reset_channel_state, which resets the channel state to its initial state and uses it before regenerating the OpenChannel message for an Unfunded Outbound channel.