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

nit: The counterparty_node_id field is always set post LDK v0.0.117 and LDK Node v0.2 was already based on LDK v0.0.118. We should be good to just expecting it, reducing indetation.

let counterparty_initiated = matches!(
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.

Codex:

  • [P2] Include on-chain force-closes in peer cleanup — /home/tnull/workspace/ldk-node/src/event.rs:1595-1599
    When a peer unilaterally broadcasts its commitment while we are disconnected (or their error message is never processed), LDK closes our channel with ClosureReason::CommitmentTxConfirmed, not CounterpartyForceClosed. Because this match only treats error-message force closes as counterparty
    initiated, the last-channel peer remains in peer_store and the reconnection loop continues indefinitely after the channel is gone; handle the confirmed commitment/closing-tx case or otherwise clean it up as well.

• Yes, the retention is intentional, but only for locally initiated force-closes.

The review is pointing at a different case: the peer force-closes while we are disconnected, and LDK later reports the terminal close as ClosureReason::CommitmentTxConfirmed rather than CounterpartyForceClosed. At that point the channel is already closed on-chain, so keeping the peer
persisted no longer helps channel_reestablish; it just keeps the reconnect loop alive for a peer with no remaining channel.

So I’d treat the review as valid. The fix should preserve:

  • HolderForceClosed { .. }: keep persisted peer, per the comment in src/lib.rs:1847
  • CounterpartyForceClosed { .. }: remove if it was their last channel
  • CommitmentTxConfirmed: also remove if it was their last channel, because this is the disconnected/on-chain remote force-close path

Caveat: CommitmentTxConfirmed is not a perfect “counterparty initiated” signal; LDK documents that it can also come from our own monitor copy. But for peer-store cleanup after a terminal ChannelClosed, it still seems closer to “no reconnect needed” than to the local HolderForceClosed
recovery case.

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