Skip to content

Commit

Permalink
Remove the peer_disconnected no_connection_possible flag
Browse files Browse the repository at this point in the history 
Long ago, we used the `no_connection_possible` to signal that a
peer has some unknown feature set or some other condition prevents
us from ever connecting to the given peer. In that case we'd
automatically force-close all channels with the given peer. This
was somewhat surprising to users so we removed the automatic
force-close, leaving the flag serving no LDK-internal purpose.

Distilling the concept of "can we connect to this peer again in the
future" to a simple flag turns out to be ripe with edge cases, so
users actually using the flag to force-close channels would likely
cause surprising behavior.

Thus, there's really not a lot of reason to keep the flag,
especially given its untested and likely to be broken in subtle
ways anyway.
  • Loading branch information
@TheBlueMatt
TheBlueMatt committed Feb 21, 2023
1 parent 0f07d5c commit be6f263
Show file tree
Hide file tree
Showing 15 changed files with 145 additions and 171 deletions.
16 changes: 8 additions & 8 deletions fuzz/src/chanmon_consistency.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -978,16 +978,16 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {

0x0c => {
if !chan_a_disconnected {
nodes[0].peer_disconnected(&nodes[1].get_our_node_id(), false);
nodes[1].peer_disconnected(&nodes[0].get_our_node_id(), false);
nodes[0].peer_disconnected(&nodes[1].get_our_node_id());
nodes[1].peer_disconnected(&nodes[0].get_our_node_id());
chan_a_disconnected = true;
drain_msg_events_on_disconnect!(0);
}
},
0x0d => {
if !chan_b_disconnected {
nodes[1].peer_disconnected(&nodes[2].get_our_node_id(), false);
nodes[2].peer_disconnected(&nodes[1].get_our_node_id(), false);
nodes[1].peer_disconnected(&nodes[2].get_our_node_id());
nodes[2].peer_disconnected(&nodes[1].get_our_node_id());
chan_b_disconnected = true;
drain_msg_events_on_disconnect!(2);
}
Expand Down Expand Up @@ -1039,7 +1039,7 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {

0x2c => {
if !chan_a_disconnected {
nodes[1].peer_disconnected(&nodes[0].get_our_node_id(), false);
nodes[1].peer_disconnected(&nodes[0].get_our_node_id());
chan_a_disconnected = true;
drain_msg_events_on_disconnect!(0);
}
Expand All @@ -1053,14 +1053,14 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
},
0x2d => {
if !chan_a_disconnected {
nodes[0].peer_disconnected(&nodes[1].get_our_node_id(), false);
nodes[0].peer_disconnected(&nodes[1].get_our_node_id());
chan_a_disconnected = true;
nodes[0].get_and_clear_pending_msg_events();
ab_events.clear();
ba_events.clear();
}
if !chan_b_disconnected {
nodes[2].peer_disconnected(&nodes[1].get_our_node_id(), false);
nodes[2].peer_disconnected(&nodes[1].get_our_node_id());
chan_b_disconnected = true;
nodes[2].get_and_clear_pending_msg_events();
bc_events.clear();
Expand All @@ -1072,7 +1072,7 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
},
0x2e => {
if !chan_b_disconnected {
nodes[1].peer_disconnected(&nodes[2].get_our_node_id(), false);
nodes[1].peer_disconnected(&nodes[2].get_our_node_id());
chan_b_disconnected = true;
drain_msg_events_on_disconnect!(2);
}
Expand Down
4 changes: 2 additions & 2 deletions lightning-invoice/src/utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -842,13 +842,13 @@ mod test {

// With only one sufficient-value peer connected we should only get its hint
scid_aliases.remove(&chan_b.0.short_channel_id_alias.unwrap());
nodes[0].node.peer_disconnected(&nodes[2].node.get_our_node_id(), false);
nodes[0].node.peer_disconnected(&nodes[2].node.get_our_node_id());
match_invoice_routes(Some(1_000_000_000), &nodes[0], scid_aliases.clone());

// If we don't have any sufficient-value peers connected we should get all hints with
// sufficient value, even though there is a connected insufficient-value peer.
scid_aliases.insert(chan_b.0.short_channel_id_alias.unwrap());
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
match_invoice_routes(Some(1_000_000_000), &nodes[0], scid_aliases);
}

Expand Down
2 changes: 1 addition & 1 deletion lightning-net-tokio/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,7 @@ mod tests {
fn handle_update_fee(&self, _their_node_id: &PublicKey, _msg: &UpdateFee) {}
fn handle_announcement_signatures(&self, _their_node_id: &PublicKey, _msg: &AnnouncementSignatures) {}
fn handle_channel_update(&self, _their_node_id: &PublicKey, _msg: &ChannelUpdate) {}
fn peer_disconnected(&self, their_node_id: &PublicKey, _no_connection_possible: bool) {
fn peer_disconnected(&self, their_node_id: &PublicKey) {
if *their_node_id == self.expected_pubkey {
self.disconnected_flag.store(true, Ordering::SeqCst);
self.pubkey_disconnected.clone().try_send(()).unwrap();
Expand Down
52 changes: 26 additions & 26 deletions lightning/src/ln/chanmon_update_fail_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,8 @@ fn do_test_simple_monitor_temporary_update_fail(disconnect: bool) {
assert_eq!(nodes[0].node.list_channels().len(), 1);

if disconnect {
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());
reconnect_nodes(&nodes[0], &nodes[1], (true, true), (0, 0), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
}

Expand Down Expand Up @@ -234,8 +234,8 @@ fn do_test_simple_monitor_temporary_update_fail(disconnect: bool) {
assert_eq!(nodes[0].node.list_channels().len(), 1);

if disconnect {
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());
reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
}

Expand Down Expand Up @@ -337,8 +337,8 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
};

if disconnect_count & !disconnect_flags > 0 {
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());
}

// Now fix monitor updating...
Expand All @@ -348,8 +348,8 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
check_added_monitors!(nodes[0], 0);

macro_rules! disconnect_reconnect_peers { () => { {
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());

nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: nodes[1].node.init_features(), remote_network_address: None }).unwrap();
let reestablish_1 = get_chan_reestablish_msgs!(nodes[0], nodes[1]);
Expand Down Expand Up @@ -1110,8 +1110,8 @@ fn test_monitor_update_fail_reestablish() {

let (payment_preimage, payment_hash, _) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1_000_000);

nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());

nodes[2].node.claim_funds(payment_preimage);
check_added_monitors!(nodes[2], 1);
Expand Down Expand Up @@ -1146,8 +1146,8 @@ fn test_monitor_update_fail_reestablish() {
nodes[1].node.get_and_clear_pending_msg_events(); // Free the holding cell
check_added_monitors!(nodes[1], 1);

nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());

nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: nodes[1].node.init_features(), remote_network_address: None }).unwrap();
nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: nodes[0].node.init_features(), remote_network_address: None }).unwrap();
Expand Down Expand Up @@ -1315,8 +1315,8 @@ fn claim_while_disconnected_monitor_update_fail() {
// Forward a payment for B to claim
let (payment_preimage_1, payment_hash_1, _) = route_payment(&nodes[0], &[&nodes[1]], 1_000_000);

nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());

nodes[1].node.claim_funds(payment_preimage_1);
check_added_monitors!(nodes[1], 1);
Expand Down Expand Up @@ -1451,8 +1451,8 @@ fn monitor_failed_no_reestablish_response() {

// Now disconnect and immediately reconnect, delivering the channel_reestablish while nodes[1]
// is still failing to update monitors.
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());

nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: nodes[1].node.init_features(), remote_network_address: None }).unwrap();
nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: nodes[0].node.init_features(), remote_network_address: None }).unwrap();
Expand Down Expand Up @@ -1873,8 +1873,8 @@ fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf:
}

// Make sure nodes[1] isn't stupid enough to re-send the ChannelReady on reconnect
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());
reconnect_nodes(&nodes[0], &nodes[1], (false, confirm_a_first), (0, 0), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
Expand Down Expand Up @@ -2041,8 +2041,8 @@ fn test_pending_update_fee_ack_on_reconnect() {
let bs_first_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[0].node.get_our_node_id());
// bs_first_raa is not delivered until it is re-generated after reconnect

nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());

nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: nodes[1].node.init_features(), remote_network_address: None }).unwrap();
let as_connect_msg = get_chan_reestablish_msgs!(nodes[0], nodes[1]).pop().unwrap();
Expand Down Expand Up @@ -2169,8 +2169,8 @@ fn do_update_fee_resend_test(deliver_update: bool, parallel_updates: bool) {
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
}

nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());

nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: nodes[1].node.init_features(), remote_network_address: None }).unwrap();
let as_connect_msg = get_chan_reestablish_msgs!(nodes[0], nodes[1]).pop().unwrap();
Expand Down Expand Up @@ -2303,9 +2303,9 @@ fn do_channel_holding_cell_serialize(disconnect: bool, reload_a: bool) {
let chan_0_monitor_serialized = get_monitor!(nodes[0], chan_id).encode();
reload_node!(nodes[0], &nodes[0].node.encode(), &[&chan_0_monitor_serialized], persister, new_chain_monitor, nodes_0_deserialized);
} else {
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
}
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());

// Now reconnect the two
nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: nodes[1].node.init_features(), remote_network_address: None }).unwrap();
Expand Down Expand Up @@ -2493,8 +2493,8 @@ fn do_test_reconnect_dup_htlc_claims(htlc_status: HTLCStatusAtDupClaim, second_f
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
}

nodes[1].node.peer_disconnected(&nodes[2].node.get_our_node_id(), false);
nodes[2].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
nodes[1].node.peer_disconnected(&nodes[2].node.get_our_node_id());
nodes[2].node.peer_disconnected(&nodes[1].node.get_our_node_id());

if second_fails {
reconnect_nodes(&nodes[1], &nodes[2], (false, false), (0, 0), (0, 0), (1, 0), (0, 0), (0, 0), (false, false));
Expand Down
12 changes: 6 additions & 6 deletions lightning/src/ln/channelmanager.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6270,13 +6270,13 @@ where
let _ = handle_error!(self, self.internal_channel_reestablish(counterparty_node_id, msg), *counterparty_node_id);
}

fn peer_disconnected(&self, counterparty_node_id: &PublicKey, no_connection_possible: bool) {
fn peer_disconnected(&self, counterparty_node_id: &PublicKey) {
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
let mut failed_channels = Vec::new();
let mut per_peer_state = self.per_peer_state.write().unwrap();
let remove_peer = {
log_debug!(self.logger, "Marking channels with {} disconnected and generating channel_updates. We believe we {} make future connections to this peer.",
log_pubkey!(counterparty_node_id), if no_connection_possible { "cannot" } else { "can" });
log_debug!(self.logger, "Marking channels with {} disconnected and generating channel_updates.",
log_pubkey!(counterparty_node_id));
if let Some(peer_state_mutex) = per_peer_state.get(counterparty_node_id) {
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
let peer_state = &mut *peer_state_lock;
Expand Down Expand Up @@ -6332,7 +6332,7 @@ where

fn peer_connected(&self, counterparty_node_id: &PublicKey, init_msg: &msgs::Init) -> Result<(), ()> {
if !init_msg.features.supports_static_remote_key() {
log_debug!(self.logger, "Peer {} does not support static remote key, disconnecting with no_connection_possible", log_pubkey!(counterparty_node_id));
log_debug!(self.logger, "Peer {} does not support static remote key, disconnecting", log_pubkey!(counterparty_node_id));
return Err(());
}

Expand Down Expand Up @@ -8210,8 +8210,8 @@ mod tests {

let chan = create_announced_chan_between_nodes(&nodes, 0, 1);

nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());

nodes[0].node.force_close_broadcasting_latest_txn(&chan.2, &nodes[1].node.get_our_node_id()).unwrap();
check_closed_broadcast!(nodes[0], true);
Expand Down
Loading

0 comments on commit be6f263

Please sign in to comment.