Skip to content

Commit

Permalink
Merge pull request #2035 from TheBlueMatt/2023-02-fix-no-con-discon
Browse files Browse the repository at this point in the history 
Fix (and DRY) the conditionals before calling peer_disconnected
  • Loading branch information
@TheBlueMatt
TheBlueMatt authored Feb 21, 2023
2 parents 068c856 + be6f263 commit e954ee8
Show file tree
Hide file tree
Showing 16 changed files with 250 additions and 222 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 @@ -979,16 +979,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 @@ -1040,7 +1040,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 @@ -1054,14 +1054,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 @@ -1073,7 +1073,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
6 changes: 1 addition & 5 deletions fuzz/src/full_stack.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -634,11 +634,7 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
if let Err(e) = channelmanager.funding_transaction_generated(&funding_generation.0, &funding_generation.1, tx.clone()) {
// It's possible the channel has been closed in the mean time, but any other
// failure may be a bug.
if let APIError::ChannelUnavailable { err } = e {
if !err.starts_with("Can't find a peer matching the passed counterparty node_id ") {
assert_eq!(err, "No such channel");
}
} else { panic!(); }
if let APIError::ChannelUnavailable { .. } = e { } else { panic!(); }
}
pending_funding_signatures.insert(funding_output, tx);
}
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 @@ -1879,8 +1879,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 @@ -2047,8 +2047,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 @@ -2175,8 +2175,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 @@ -2309,9 +2309,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 @@ -2499,8 +2499,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
16 changes: 8 additions & 8 deletions lightning/src/ln/channelmanager.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2666,7 +2666,7 @@ where
(chan, funding_msg)
},
Err(_) => { return Err(APIError::ChannelUnavailable {
err: "Error deriving keys or signing initial commitment transactions - either our RNG or our counterparty's RNG is broken or the Signer refused to sign".to_owned()
err: "Signer refused to sign the initial commitment transaction".to_owned()
}) },
}
};
Expand Down Expand Up @@ -6245,13 +6245,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 @@ -6293,7 +6293,7 @@ where
debug_assert!(peer_state.is_connected, "A disconnected peer cannot disconnect");
peer_state.is_connected = false;
peer_state.ok_to_remove(true)
} else { true }
} else { debug_assert!(false, "Unconnected peer disconnected"); true }
};
if remove_peer {
per_peer_state.remove(counterparty_node_id);
Expand All @@ -6307,7 +6307,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 @@ -8186,8 +8186,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 e954ee8

Please sign in to comment.