-
Notifications
You must be signed in to change notification settings - Fork 138
Fix peer store management on channel closure #895
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1585,6 +1585,40 @@ where | |
| } => { | ||
| log_info!(self.logger, "Channel {} closed due to: {}", channel_id, reason); | ||
|
|
||
| // If the counterparty initiated closure of their last remaining channel, | ||
| // remove them from the peer store so we stop reconnect attempts. | ||
|
|
||
| // We exclude `channel_id` from the check because LDK emits | ||
| // ChannelClosed before removing the channel from its internal list. | ||
|
|
||
| if let Some(counterparty_node_id) = counterparty_node_id { | ||
| let counterparty_initiated = matches!( | ||
| reason, | ||
| ClosureReason::CounterpartyForceClosed { .. } | ||
| | ClosureReason::CounterpartyInitiatedCooperativeClosure | ||
| ); | ||
|
|
||
| if counterparty_initiated { | ||
| let has_other_channels = self | ||
| .channel_manager | ||
| .list_channels_with_counterparty(&counterparty_node_id) | ||
| .iter() | ||
| .any(|c| c.channel_id != channel_id); | ||
|
|
||
| if !has_other_channels { | ||
| if let Err(e) = self.peer_store.remove_peer(&counterparty_node_id) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We usually replay the event in case of persistence failures. While this path is not critical, we should probably do the same here to keep it consistent.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, I've updated this to return ReplayEvent() on failure to keep it consistent with other persistence paths. |
||
| log_error!( | ||
| self.logger, | ||
| "Failed to remove peer {} from peer store: {}", | ||
| counterparty_node_id, | ||
| e | ||
| ); | ||
| return Err(ReplayEvent()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let event = Event::ChannelClosed { | ||
| channel_id, | ||
| user_channel_id: UserChannelId(user_channel_id), | ||
|
|
||
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.
We should flip the order here and check the closure reason first, so the read on the channel manager only happens when it's actually relevant
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.
That's true, no point querying the channel manager if the closure reason rules out action.
It has been reordered.