From 94e0ecec68d85ca1e10d08d6b35a9c9284c25ebb Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Fri, 13 Oct 2023 09:28:35 -0700 Subject: [PATCH] Disconnect peer when force closing a funded channel with an error We do this to ensure that the counterparty will always broadcast their latest state when we broadcast ours. Usually, they'll do this with the `error` message alone, but if they don't receive it or ignore it, then we'll force them to broadcast by sending them a bogus `channel_reestablish` upon reconnecting. Note that this doesn't apply to unfunded channels as there is no commitment transaction to broadcast. --- lightning/src/ln/channelmanager.rs | 37 ++++++++++++----------- lightning/src/ln/functional_test_utils.rs | 26 ++++++++++++++-- lightning/src/ln/functional_tests.rs | 24 +++++++-------- lightning/src/ln/reload_tests.rs | 4 +-- 4 files changed, 58 insertions(+), 33 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 35107f10b..d1f561e26 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -447,16 +447,17 @@ impl MsgHandleErrInternal { } #[inline] fn from_finish_shutdown(err: String, channel_id: ChannelId, user_channel_id: u128, shutdown_res: ShutdownResult, channel_update: Option, channel_capacity: u64) -> Self { + let err_msg = msgs::ErrorMessage { channel_id, data: err.clone() }; + let action = if let (Some(_), ..) = &shutdown_res { + // We have a closing `ChannelMonitorUpdate`, which means the channel was funded and we + // should disconnect our peer such that we force them to broadcast their latest + // commitment upon reconnecting. + msgs::ErrorAction::DisconnectPeer { msg: Some(err_msg) } + } else { + msgs::ErrorAction::SendErrorMessage { msg: err_msg } + }; Self { - err: LightningError { - err: err.clone(), - action: msgs::ErrorAction::SendErrorMessage { - msg: msgs::ErrorMessage { - channel_id, - data: err - }, - }, - }, + err: LightningError { err, action }, chan_id: Some((channel_id, user_channel_id)), shutdown_finish: Some((shutdown_res, channel_update)), channel_capacity: Some(channel_capacity) @@ -2812,8 +2813,8 @@ where peer_state.pending_msg_events.push( events::MessageSendEvent::HandleError { node_id: counterparty_node_id, - action: msgs::ErrorAction::SendErrorMessage { - msg: msgs::ErrorMessage { channel_id: *channel_id, data: "Channel force-closed".to_owned() } + action: msgs::ErrorAction::DisconnectPeer { + msg: Some(msgs::ErrorMessage { channel_id: *channel_id, data: "Channel force-closed".to_owned() }) }, } ); @@ -6928,8 +6929,8 @@ where self.issue_channel_close_events(&chan.context, ClosureReason::HolderForceClosed); pending_msg_events.push(events::MessageSendEvent::HandleError { node_id: chan.context.get_counterparty_node_id(), - action: msgs::ErrorAction::SendErrorMessage { - msg: msgs::ErrorMessage { channel_id: chan.context.channel_id(), data: "Channel force-closed".to_owned() } + action: msgs::ErrorAction::DisconnectPeer { + msg: Some(msgs::ErrorMessage { channel_id: chan.context.channel_id(), data: "Channel force-closed".to_owned() }) }, }); } @@ -7713,10 +7714,12 @@ where self.issue_channel_close_events(&channel.context, reason); pending_msg_events.push(events::MessageSendEvent::HandleError { node_id: channel.context.get_counterparty_node_id(), - action: msgs::ErrorAction::SendErrorMessage { msg: msgs::ErrorMessage { - channel_id: channel.context.channel_id(), - data: reason_message, - } }, + action: msgs::ErrorAction::DisconnectPeer { + msg: Some(msgs::ErrorMessage { + channel_id: channel.context.channel_id(), + data: reason_message, + }) + }, }); return false; } diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index acf1c7dfb..42403f89c 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -665,6 +665,12 @@ pub fn get_err_msg(node: &Node, recipient: &PublicKey) -> msgs::ErrorMessage { assert_eq!(node_id, recipient); (*msg).clone() }, + MessageSendEvent::HandleError { + action: msgs::ErrorAction::DisconnectPeer { ref msg }, ref node_id + } => { + assert_eq!(node_id, recipient); + msg.as_ref().unwrap().clone() + }, _ => panic!("Unexpected event"), } } @@ -1446,10 +1452,15 @@ pub fn check_closed_broadcast(node: &Node, num_channels: usize, with_error_msg: assert_eq!(msg.contents.flags & 2, 2); None }, - MessageSendEvent::HandleError { action: msgs::ErrorAction::SendErrorMessage { ref msg }, node_id: _ } => { + MessageSendEvent::HandleError { action: msgs::ErrorAction::SendErrorMessage { msg }, node_id: _ } => { assert!(with_error_msg); // TODO: Check node_id - Some(msg.clone()) + Some(msg) + }, + MessageSendEvent::HandleError { action: msgs::ErrorAction::DisconnectPeer { msg }, node_id: _ } => { + assert!(with_error_msg); + // TODO: Check node_id + Some(msg.unwrap()) }, _ => panic!("Unexpected event"), } @@ -2921,6 +2932,13 @@ pub fn handle_announce_close_broadcast_events<'a, 'b, 'c>(nodes: &Vec { + assert_eq!(node_id, nodes[b].node.get_our_node_id()); + assert_eq!(msg.as_ref().unwrap().data, expected_error); + if needs_err_handle { + nodes[b].node.handle_error(&nodes[a].node.get_our_node_id(), msg.as_ref().unwrap()); + } + }, _ => panic!("Unexpected event"), } @@ -2938,6 +2956,10 @@ pub fn handle_announce_close_broadcast_events<'a, 'b, 'c>(nodes: &Vec { + assert_eq!(node_id, nodes[a].node.get_our_node_id()); + assert_eq!(msg.as_ref().unwrap().data, expected_error); + }, _ => panic!("Unexpected event"), } } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 5e90ee7e9..cabbba8f3 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -1338,9 +1338,9 @@ fn test_duplicate_htlc_different_direction_onchain() { for e in events { match e { MessageSendEvent::BroadcastChannelUpdate { .. } => {}, - MessageSendEvent::HandleError { node_id, action: msgs::ErrorAction::SendErrorMessage { ref msg } } => { + MessageSendEvent::HandleError { node_id, action: msgs::ErrorAction::DisconnectPeer { ref msg } } => { assert_eq!(node_id, nodes[1].node.get_our_node_id()); - assert_eq!(msg.data, "Channel closed because commitment or closing transaction was confirmed on chain."); + assert_eq!(msg.as_ref().unwrap().data, "Channel closed because commitment or closing transaction was confirmed on chain."); }, MessageSendEvent::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref update_fail_malformed_htlcs, .. } } => { assert!(update_add_htlcs.is_empty()); @@ -2369,7 +2369,7 @@ fn channel_monitor_network_test() { _ => panic!("Unexpected event"), }; match events[1] { - MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { .. }, node_id } => { + MessageSendEvent::HandleError { action: ErrorAction::DisconnectPeer { .. }, node_id } => { assert_eq!(node_id, nodes[4].node.get_our_node_id()); }, _ => panic!("Unexpected event"), @@ -2401,7 +2401,7 @@ fn channel_monitor_network_test() { _ => panic!("Unexpected event"), }; match events[1] { - MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { .. }, node_id } => { + MessageSendEvent::HandleError { action: ErrorAction::DisconnectPeer { .. }, node_id } => { assert_eq!(node_id, nodes[3].node.get_our_node_id()); }, _ => panic!("Unexpected event"), @@ -2913,7 +2913,7 @@ fn test_htlc_on_chain_success() { let nodes_0_event = remove_first_msg_event_to_node(&nodes[0].node.get_our_node_id(), &mut events); match nodes_2_event { - MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { .. }, node_id: _ } => {}, + MessageSendEvent::HandleError { action: ErrorAction::DisconnectPeer { .. }, node_id: _ } => {}, _ => panic!("Unexpected event"), } @@ -3358,7 +3358,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use let nodes_2_event = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut events); match nodes_2_event { - MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { msg: msgs::ErrorMessage { channel_id, ref data } }, node_id: _ } => { + MessageSendEvent::HandleError { action: ErrorAction::DisconnectPeer { msg: Some(msgs::ErrorMessage { channel_id, ref data }) }, node_id: _ } => { assert_eq!(channel_id, chan_2.2); assert_eq!(data.as_str(), "Channel closed because commitment or closing transaction was confirmed on chain."); }, @@ -4920,7 +4920,7 @@ fn test_onchain_to_onchain_claim() { let nodes_0_event = remove_first_msg_event_to_node(&nodes[0].node.get_our_node_id(), &mut msg_events); match nodes_2_event { - MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { .. }, node_id: _ } => {}, + MessageSendEvent::HandleError { action: ErrorAction::DisconnectPeer { .. }, node_id: _ } => {}, _ => panic!("Unexpected event"), } @@ -7860,9 +7860,9 @@ fn test_channel_conf_timeout() { let close_ev = nodes[1].node.get_and_clear_pending_msg_events(); assert_eq!(close_ev.len(), 1); match close_ev[0] { - MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { ref msg }, ref node_id } => { + MessageSendEvent::HandleError { action: ErrorAction::DisconnectPeer { ref msg }, ref node_id } => { assert_eq!(*node_id, nodes[0].node.get_our_node_id()); - assert_eq!(msg.data, "Channel closed because funding transaction failed to confirm within 2016 blocks"); + assert_eq!(msg.as_ref().unwrap().data, "Channel closed because funding transaction failed to confirm within 2016 blocks"); }, _ => panic!("Unexpected event"), } @@ -9212,8 +9212,8 @@ fn test_invalid_funding_tx() { assert_eq!(events_2.len(), 1); if let MessageSendEvent::HandleError { node_id, action } = &events_2[0] { assert_eq!(*node_id, nodes[0].node.get_our_node_id()); - if let msgs::ErrorAction::SendErrorMessage { msg } = action { - assert_eq!(msg.data, "Channel closed because of an exception: ".to_owned() + expected_err); + if let msgs::ErrorAction::DisconnectPeer { msg } = action { + assert_eq!(msg.as_ref().unwrap().data, "Channel closed because of an exception: ".to_owned() + expected_err); } else { panic!(); } } else { panic!(); } assert_eq!(nodes[1].node.list_channels().len(), 0); @@ -10652,7 +10652,7 @@ fn do_test_funding_and_commitment_tx_confirm_same_block(confirm_remote_commitmen let mut msg_events = closing_node.node.get_and_clear_pending_msg_events(); assert_eq!(msg_events.len(), 1); match msg_events.pop().unwrap() { - MessageSendEvent::HandleError { action: msgs::ErrorAction::SendErrorMessage { .. }, .. } => {}, + MessageSendEvent::HandleError { action: msgs::ErrorAction::DisconnectPeer { .. }, .. } => {}, _ => panic!("Unexpected event"), } check_added_monitors(closing_node, 1); diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs index 9ea13f131..58da52eb7 100644 --- a/lightning/src/ln/reload_tests.rs +++ b/lightning/src/ln/reload_tests.rs @@ -566,8 +566,8 @@ fn do_test_data_loss_protect(reconnect_panicing: bool) { if let MessageSendEvent::BroadcastChannelUpdate { .. } = msg { } else if let MessageSendEvent::HandleError { ref action, .. } = msg { match action { - &ErrorAction::SendErrorMessage { ref msg } => { - assert_eq!(msg.data, "Channel force-closed"); + &ErrorAction::DisconnectPeer { ref msg } => { + assert_eq!(msg.as_ref().unwrap().data, "Channel force-closed"); }, _ => panic!("Unexpected event!"), } -- 2.39.5