Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions src/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
);
Comment on lines +1594 to +1599
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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.


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) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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),
Expand Down
12 changes: 10 additions & 2 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1841,8 +1841,16 @@ impl Node {
})?;
}

// Check if this was the last open channel, if so, forget the peer.
if open_channels.len() == 1 {
// If this was the last open channel and we're closing cooperatively, forget the peer
// since we have no further reason to reconnect.

// For force-closes we intentionally keep the peer in the store so the background reconnection
// task keeps firing and can drive the channel_reestablish recovery flow.
// This is especially important against LND peers, which don't always handle force-closure error messages correctly.

// Note that this means a force-closed peer is retained until the user explicitly calls Node::disconnect.

if open_channels.len() == 1 && !force {
self.peer_store.remove_peer(&counterparty_node_id)?;
}
}
Expand Down
14 changes: 14 additions & 0 deletions tests/common/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1545,6 +1545,20 @@ pub(crate) async fn do_channel_full_cycle<E: ElectrumApi>(
node_b.sync_wallets().unwrap();
}

if force_close {
// Peer retained after local force-close to allow channel_reestablish recovery.
assert!(
node_a.list_peers().iter().any(|p| p.node_id == node_b.node_id() && p.is_persisted),
"node_b should remain persisted in node_a peer store after locally-initiated force-close"
);
} else {
// Peer removed after cooperative close — no further reason to reconnect.
assert!(
!node_a.list_peers().iter().any(|p| p.node_id == node_b.node_id() && p.is_persisted),
"node_b should be removed from node_a peer store after cooperative close"
);
}

let sum_of_all_payments_sat = (push_msat
+ invoice_amount_1_msat
+ overpaid_amount_msat
Expand Down
Loading