Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send bogus ChannelReestablish for unknown channels #2658

Merged
merged 3 commits into from
Oct 18, 2023
Merged

Send bogus ChannelReestablish for unknown channels #2658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9f7de47
Send bogus ChannelReestablish for unknown channels
wpaulino Oct 11, 2023
94e0ece
Disconnect peer when force closing a funded channel with an error
wpaulino Oct 13, 2023
ed38eac
Release short_to_chan_info lock throughout forwarding_channel_not_found
wpaulino Oct 16, 2023
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
2 changes: 1 addition & 1 deletion lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3992,7 +3992,7 @@ impl<SP: Deref> Channel<SP> where

if msg.next_local_commitment_number >= INITIAL_COMMITMENT_NUMBER || msg.next_remote_commitment_number >= INITIAL_COMMITMENT_NUMBER ||
msg.next_local_commitment_number == 0 {
return Err(ChannelError::Close("Peer sent a garbage channel_reestablish (usually an lnd node with lost state asking us to force-close for them)".to_owned()));
return Err(ChannelError::Close("Peer sent an invalid channel_reestablish to force close in a non-standard way".to_owned()));
}

if msg.next_remote_commitment_number > 0 {
Expand Down
142 changes: 121 additions & 21 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<msgs::ChannelUpdate>, 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)
Expand Down Expand Up @@ -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() })
},
}
);
Expand Down Expand Up @@ -4296,8 +4297,9 @@ where
}
}
}
let (counterparty_node_id, forward_chan_id) = match self.short_to_chan_info.read().unwrap().get(&short_chan_id) {
Some((cp_id, chan_id)) => (cp_id.clone(), chan_id.clone()),
let chan_info_opt = self.short_to_chan_info.read().unwrap().get(&short_chan_id).cloned();
let (counterparty_node_id, forward_chan_id) = match chan_info_opt {
Some((cp_id, chan_id)) => (cp_id, chan_id),
None => {
forwarding_channel_not_found!();
continue;
Expand Down Expand Up @@ -6785,7 +6787,10 @@ where
let peer_state_mutex = per_peer_state.get(counterparty_node_id)
.ok_or_else(|| {
debug_assert!(false);
MsgHandleErrInternal::send_err_msg_no_close(format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id), msg.channel_id)
MsgHandleErrInternal::send_err_msg_no_close(
format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id),
msg.channel_id
)
})?;
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
let peer_state = &mut *peer_state_lock;
Expand Down Expand Up @@ -6829,7 +6834,39 @@ where
"Got a channel_reestablish message for an unfunded channel!".into())), chan_phase_entry);
}
},
hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", counterparty_node_id), msg.channel_id))
hash_map::Entry::Vacant(_) => {
log_debug!(self.logger, "Sending bogus ChannelReestablish for unknown channel {} to force channel closure",
log_bytes!(msg.channel_id.0));
// Unfortunately, lnd doesn't force close on errors
// (https://github.com/lightningnetwork/lnd/blob/abb1e3463f3a83bbb843d5c399869dbe930ad94f/htlcswitch/link.go#L2119).
// One of the few ways to get an lnd counterparty to force close is by
// replicating what they do when restoring static channel backups (SCBs). They
// send an invalid `ChannelReestablish` with `0` commitment numbers and an
// invalid `your_last_per_commitment_secret`.
//
// Since we received a `ChannelReestablish` for a channel that doesn't exist, we
// can assume it's likely the channel closed from our point of view, but it
// remains open on the counterparty's side. By sending this bogus
// `ChannelReestablish` message now as a response to theirs, we trigger them to
// force close broadcasting their latest state. If the closing transaction from
// our point of view remains unconfirmed, it'll enter a race with the
// counterparty's to-be-broadcast latest commitment transaction.
peer_state.pending_msg_events.push(MessageSendEvent::SendChannelReestablish {
node_id: *counterparty_node_id,
msg: msgs::ChannelReestablish {
channel_id: msg.channel_id,
next_local_commitment_number: 0,
next_remote_commitment_number: 0,
your_last_per_commitment_secret: [1u8; 32],
my_current_per_commitment_point: PublicKey::from_slice(&[2u8; 33]).unwrap(),
next_funding_txid: None,
},
});
return Err(MsgHandleErrInternal::send_err_msg_no_close(
format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}",
counterparty_node_id), msg.channel_id)
)
}
}
};

Expand Down Expand Up @@ -6893,8 +6930,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() })
},
});
}
Expand Down Expand Up @@ -7678,10 +7715,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;
}
Expand Down Expand Up @@ -11219,6 +11258,67 @@ mod tests {
let payment_preimage = PaymentPreimage([42; 32]);
assert_eq!(format!("{}", &payment_preimage), "2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a");
}

#[test]
fn test_trigger_lnd_force_close() {
let chanmon_cfg = create_chanmon_cfgs(2);
let node_cfg = create_node_cfgs(2, &chanmon_cfg);
let user_config = test_default_channel_config();
let node_chanmgr = create_node_chanmgrs(2, &node_cfg, &[Some(user_config), Some(user_config)]);
let nodes = create_network(2, &node_cfg, &node_chanmgr);

// Open a channel, immediately disconnect each other, and broadcast Alice's latest state.
let (_, _, chan_id, funding_tx) = create_announced_chan_between_nodes(&nodes, 0, 1);
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_id, &nodes[1].node.get_our_node_id()).unwrap();
check_closed_broadcast(&nodes[0], 1, true);
check_added_monitors(&nodes[0], 1);
check_closed_event!(nodes[0], 1, ClosureReason::HolderForceClosed, [nodes[1].node.get_our_node_id()], 100000);
{
let txn = nodes[0].tx_broadcaster.txn_broadcast();
assert_eq!(txn.len(), 1);
check_spends!(txn[0], funding_tx);
}

// Since they're disconnected, Bob won't receive Alice's `Error` message. Reconnect them
// such that Bob sends a `ChannelReestablish` to Alice since the channel is still open from
// their side.
nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init {
features: nodes[1].node.init_features(), networks: None, remote_network_address: None
}, true).unwrap();
nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init {
features: nodes[0].node.init_features(), networks: None, remote_network_address: None
}, false).unwrap();
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
let channel_reestablish = get_event_msg!(
nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id()
);
nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &channel_reestablish);

// Alice should respond with an error since the channel isn't known, but a bogus
// `ChannelReestablish` should be sent first, such that we actually trigger Bob to force
// close even if it was an lnd node.
let msg_events = nodes[0].node.get_and_clear_pending_msg_events();
assert_eq!(msg_events.len(), 2);
if let MessageSendEvent::SendChannelReestablish { node_id, msg } = &msg_events[0] {
assert_eq!(*node_id, nodes[1].node.get_our_node_id());
assert_eq!(msg.next_local_commitment_number, 0);
assert_eq!(msg.next_remote_commitment_number, 0);
nodes[1].node.handle_channel_reestablish(&nodes[0].node.get_our_node_id(), &msg);
} else { panic!() };
check_closed_broadcast(&nodes[1], 1, true);
check_added_monitors(&nodes[1], 1);
let expected_close_reason = ClosureReason::ProcessingError {
err: "Peer sent an invalid channel_reestablish to force close in a non-standard way".to_string()
};
check_closed_event!(nodes[1], 1, expected_close_reason, [nodes[0].node.get_our_node_id()], 100000);
{
let txn = nodes[1].tx_broadcaster.txn_broadcast();
assert_eq!(txn.len(), 1);
check_spends!(txn[0], funding_tx);
}
}
}

#[cfg(ldk_bench)]
Expand Down
26 changes: 24 additions & 2 deletions lightning/src/ln/functional_test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
}
}
Expand Down Expand Up @@ -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"),
}
Expand Down Expand Up @@ -2921,6 +2932,13 @@ pub fn handle_announce_close_broadcast_events<'a, 'b, 'c>(nodes: &Vec<Node<'a, '
nodes[b].node.handle_error(&nodes[a].node.get_our_node_id(), msg);
}
},
MessageSendEvent::HandleError { node_id, action: msgs::ErrorAction::DisconnectPeer { ref msg } } => {
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"),
}

Expand All @@ -2938,6 +2956,10 @@ pub fn handle_announce_close_broadcast_events<'a, 'b, 'c>(nodes: &Vec<Node<'a, '
assert_eq!(node_id, nodes[a].node.get_our_node_id());
assert_eq!(msg.data, expected_error);
},
MessageSendEvent::HandleError { node_id, action: msgs::ErrorAction::DisconnectPeer { ref msg } } => {
assert_eq!(node_id, nodes[a].node.get_our_node_id());
assert_eq!(msg.as_ref().unwrap().data, expected_error);
},
_ => panic!("Unexpected event"),
}
}
Expand Down
24 changes: 12 additions & 12 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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"),
Expand Down Expand Up @@ -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"),
Expand Down Expand Up @@ -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"),
}

Expand Down Expand Up @@ -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.");
},
Expand Down Expand Up @@ -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"),
}

Expand Down Expand Up @@ -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"),
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
8 changes: 4 additions & 4 deletions lightning/src/ln/payment_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,8 +706,8 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) {
let bs_reestablish = get_chan_reestablish_msgs!(nodes[1], nodes[0]).pop().unwrap();
nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &bs_reestablish);
let as_err = nodes[0].node.get_and_clear_pending_msg_events();
assert_eq!(as_err.len(), 1);
match as_err[0] {
assert_eq!(as_err.len(), 2);
match as_err[1] {
MessageSendEvent::HandleError { node_id, action: msgs::ErrorAction::SendErrorMessage { ref msg } } => {
assert_eq!(node_id, nodes[1].node.get_our_node_id());
nodes[1].node.handle_error(&nodes[0].node.get_our_node_id(), msg);
Expand Down Expand Up @@ -881,9 +881,9 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) {
let bs_reestablish = get_chan_reestablish_msgs!(nodes[1], nodes[0]).pop().unwrap();
nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &bs_reestablish);
let as_err = nodes[0].node.get_and_clear_pending_msg_events();
assert_eq!(as_err.len(), 1);
assert_eq!(as_err.len(), 2);
let bs_commitment_tx;
match as_err[0] {
match as_err[1] {
MessageSendEvent::HandleError { node_id, action: msgs::ErrorAction::SendErrorMessage { ref msg } } => {
assert_eq!(node_id, nodes[1].node.get_our_node_id());
nodes[1].node.handle_error(&nodes[0].node.get_our_node_id(), msg);
Expand Down
Loading