From 5d4afa336410a560b2f8382da0e5785ae0c759e7 Mon Sep 17 00:00:00 2001 From: jolah1 Date: Tue, 7 Jul 2026 13:03:37 +0100 Subject: [PATCH] node: consolidate peer-store cleanup into ChannelClosed handler Peer-store cleanup on channel closure was split across two places: close_channel_internal removed the peer on a cooperative close, while the ChannelClosed handler removed it for an allowlist of counterparty/on-chain reasons. That allowlist missed terminal cases such as a channel closing before funding (CounterpartyCoopClosedUnfundedChannel), leaving those peers in the store and the reconnection loop retrying them indefinitely. Make the ChannelClosed handler the single owner of the decision: retain the peer only for HolderForceClosed -- where we deliberately keep reconnecting so channel_reestablish can drive recovery, important against LND peers that don't always handle force-closure error messages -- and drop it for every other terminal reason once no other channel with the peer remains. close_channel_internal no longer touches the peer store. Add an integration test for the counterparty force-close path and keep the retain/remove assertions in do_channel_full_cycle. Co-Authored-By: Claude Opus 4.8 --- src/event.rs | 37 +++++++++++++++++++++++- src/lib.rs | 11 +++++--- tests/common/mod.rs | 14 +++++++++ tests/integration_tests_rust.rs | 50 +++++++++++++++++++++++++++++++++ 4 files changed, 107 insertions(+), 5 deletions(-) diff --git a/src/event.rs b/src/event.rs index 80acd0690..7f3c898f8 100644 --- a/src/event.rs +++ b/src/event.rs @@ -1613,10 +1613,45 @@ where } => { log_info!(self.logger, "Channel {} closed due to: {}", channel_id, reason); + // `counterparty_node_id` has been set on every `ChannelClosed` since LDK 0.0.117. + let counterparty_node_id = counterparty_node_id + .expect("counterparty_node_id is always set since LDK 0.0.117"); + + // Drop the peer once its last channel with us has reached a terminal state + // that reconnection cannot recover. Every closure reason is terminal except + // `HolderForceClosed`: when *we* force-close, we keep reconnecting so that + // `channel_reestablish` can drive recovery (see `Node::close_channel_internal`). + // This also cleans up peers persisted for a channel that closed before funding + // (e.g. `CounterpartyCoopClosedUnfundedChannel`), which would otherwise be + // retried forever. + // We exclude `channel_id` from the count because LDK emits `ChannelClosed` + // before removing it from its internal list. + let dont_reconnect = !matches!(reason, ClosureReason::HolderForceClosed { .. }); + + if dont_reconnect { + 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).await { + 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), - counterparty_node_id, + counterparty_node_id: Some(counterparty_node_id), reason: Some(reason), }; diff --git a/src/lib.rs b/src/lib.rs index 34fa7f54d..583109356 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1919,10 +1919,13 @@ impl Node { })?; } - // Check if this was the last open channel, if so, forget the peer. - if open_channels.len() == 1 { - self.runtime.block_on(self.peer_store.remove_peer(&counterparty_node_id))?; - } + // Peer store cleanup is handled centrally in the `ChannelClosed` event handler, + // which drops the peer once its last channel reaches a terminal state that + // reconnection cannot recover. We intentionally do nothing here so that a + // force-closed peer is retained, letting the background reconnection task keep + // firing and drive the `channel_reestablish` recovery flow. This is especially + // important against LND peers, which don't always handle force-closure error + // messages correctly. } Ok(()) diff --git a/tests/common/mod.rs b/tests/common/mod.rs index adeb327bf..bde783456 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -1597,6 +1597,20 @@ pub(crate) async fn do_channel_full_cycle( assert!(node_b.list_balances().pending_balances_from_channel_closures.is_empty()); } + 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 diff --git a/tests/integration_tests_rust.rs b/tests/integration_tests_rust.rs index fab73ed0c..d53d8119d 100644 --- a/tests/integration_tests_rust.rs +++ b/tests/integration_tests_rust.rs @@ -97,6 +97,56 @@ async fn channel_full_cycle_force_close_trusted_no_reserve() { .await; } +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +async fn peer_removed_when_counterparty_force_closes_last_channel() { + // When we open a channel outbound, we persist the counterparty so the background + // reconnection task can reach them. If the counterparty then force-closes what turns out + // to be their last channel with us, the channel is terminal and there is nothing left for + // `channel_reestablish` to recover, so the peer should be dropped from the store rather + // than reconnected to forever. + let (bitcoind, electrsd) = setup_bitcoind_and_electrsd(); + let chain_source = random_chain_source(&bitcoind, &electrsd); + let (node_a, node_b) = setup_two_nodes(&chain_source, false, true, false); + + let address_a = node_a.onchain_payment().new_address().unwrap(); + let premine_amount_sat = 5_000_000; + premine_and_distribute_funds( + &bitcoind.client, + &electrsd.client, + vec![address_a], + Amount::from_sat(premine_amount_sat), + ) + .await; + node_a.sync_wallets().unwrap(); + + // node_a opens the channel, so node_a persists node_b in its peer store. + open_channel(&node_a, &node_b, 4_000_000, false, &electrsd).await; + generate_blocks_and_wait(&bitcoind.client, &electrsd.client, 6).await; + node_a.sync_wallets().unwrap(); + node_b.sync_wallets().unwrap(); + + let _user_channel_id_a = expect_channel_ready_event!(node_a, node_b.node_id()); + let user_channel_id_b = expect_channel_ready_event!(node_b, node_a.node_id()); + + assert!( + node_a.list_peers().iter().any(|p| p.node_id == node_b.node_id() && p.is_persisted), + "node_a should persist node_b after opening a channel to it" + ); + + // The counterparty force-closes their last channel with us. + node_b.force_close_channel(&user_channel_id_b, node_a.node_id(), None).unwrap(); + + expect_event!(node_a, ChannelClosed); + expect_event!(node_b, ChannelClosed); + + // node_a should have dropped node_b from its peer store. We assert on `is_persisted` rather + // than peer presence so a lingering transient TCP connection doesn't mask the removal. + assert!( + !node_a.list_peers().iter().any(|p| p.node_id == node_b.node_id() && p.is_persisted), + "node_a should drop node_b from its peer store after node_b force-closed the last channel" + ); +} + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn channel_full_cycle_0conf() { let (bitcoind, electrsd) = setup_bitcoind_and_electrsd();