Skip to content

Handle missing case in reestablish local commitment number checks #2721

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

Merged
merged 3 commits into from
Nov 29, 2023
Merged

Handle missing case in reestablish local commitment number checks #2721

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
589a88e
Fix `data_loss_protect` test to actually test DLP
TheBlueMatt Nov 9, 2023
f248307
Clean up error messages and conditionals in reestablish handling
TheBlueMatt Nov 9, 2023
ac3fd98
Handle missing case in reestablish local commitment number checks
TheBlueMatt Nov 9, 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
5 changes: 3 additions & 2 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4152,14 +4152,15 @@ impl<SP: Deref> Channel<SP> where
return Err(ChannelError::Close("Peer sent an invalid channel_reestablish to force close in a non-standard way".to_owned()));
}

let our_commitment_transaction = INITIAL_COMMITMENT_NUMBER - self.context.cur_holder_commitment_transaction_number - 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

somewhat poor choice for a variable name that's a number/index, and not an actual tx

if msg.next_remote_commitment_number > 0 {
let expected_point = self.context.holder_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1, &self.context.secp_ctx);
let given_secret = SecretKey::from_slice(&msg.your_last_per_commitment_secret)
.map_err(|_| ChannelError::Close("Peer sent a garbage channel_reestablish with unparseable secret key".to_owned()))?;
if expected_point != PublicKey::from_secret_key(&self.context.secp_ctx, &given_secret) {
return Err(ChannelError::Close("Peer sent a garbage channel_reestablish with secret key not matching the commitment height provided".to_owned()));
}
if msg.next_remote_commitment_number > INITIAL_COMMITMENT_NUMBER - self.context.cur_holder_commitment_transaction_number {
if msg.next_remote_commitment_number > our_commitment_transaction {
macro_rules! log_and_panic {
($err_msg: expr) => {
log_error!(logger, $err_msg, &self.context.channel_id, log_pubkey!(self.context.counterparty_node_id));
Expand All @@ -4179,7 +4180,6 @@ impl<SP: Deref> Channel<SP> where

// Before we change the state of the channel, we check if the peer is sending a very old
// commitment transaction number, if yes we send a warning message.
let our_commitment_transaction = INITIAL_COMMITMENT_NUMBER - self.context.cur_holder_commitment_transaction_number - 1;
if msg.next_remote_commitment_number + 1 < our_commitment_transaction {
return Err(ChannelError::Warn(format!(
"Peer attempted to reestablish channel with a very old local commitment transaction: {} (received) vs {} (expected)",
Expand Down Expand Up @@ -4239,6 +4239,7 @@ impl<SP: Deref> Channel<SP> where
Some(self.get_last_revoke_and_ack())
}
} else {
debug_assert!(false, "All values should have been handled in the four cases above");
return Err(ChannelError::Close(format!(
"Peer attempted to reestablish channel expecting a future local commitment transaction: {} (received) vs {} (expected)",
msg.next_remote_commitment_number,
Expand Down
211 changes: 137 additions & 74 deletions lightning/src/ln/reload_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,10 @@ use crate::chain::channelmonitor::{CLOSED_CHANNEL_UPDATE_ID, ChannelMonitor};
use crate::sign::EntropySource;
use crate::chain::transaction::OutPoint;
use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
use crate::ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, PaymentId, RecipientOnionFields};
use crate::ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, PaymentId, Retry, RecipientOnionFields};
use crate::ln::msgs;
use crate::ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, ErrorAction};
use crate::routing::router::{RouteParameters, PaymentParameters};
use crate::util::test_channel_signer::TestChannelSigner;
use crate::util::test_utils;
use crate::util::errors::APIError;
Expand Down Expand Up @@ -494,7 +495,7 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() {
}

#[cfg(feature = "std")]
fn do_test_data_loss_protect(reconnect_panicing: bool) {
fn do_test_data_loss_protect(reconnect_panicing: bool, substantially_old: bool, not_stale: bool) {
// When we get a data_loss_protect proving we're behind, we immediately panic as the
// chain::Watch API requirements have been violated (e.g. the user restored from a backup). The
// panic message informs the user they should force-close without broadcasting, which is tested
Expand All @@ -518,8 +519,38 @@ fn do_test_data_loss_protect(reconnect_panicing: bool) {
let previous_node_state = nodes[0].node.encode();
let previous_chain_monitor_state = get_monitor!(nodes[0], chan.2).encode();

send_payment(&nodes[0], &vec!(&nodes[1])[..], 8000000);
send_payment(&nodes[0], &vec!(&nodes[1])[..], 8000000);
assert!(!substantially_old || !not_stale, "substantially_old and not_stale doesn't make sense");
if not_stale || !substantially_old {
// Previously, we'd only hit the data_loss_protect assertion if we had a state which
// revoked at least two revocations ago, not the latest revocation. Here, we use
// `not_stale` to test the boundary condition.
let pay_params = PaymentParameters::for_keysend(nodes[1].node.get_our_node_id(), 100, false);
let route_params = RouteParameters::from_payment_params_and_value(pay_params, 40000);
nodes[0].node.send_spontaneous_payment_with_retry(None, RecipientOnionFields::spontaneous_empty(), PaymentId([0; 32]), route_params, Retry::Attempts(0));
check_added_monitors(&nodes[0], 1);
let update_add_commit = SendEvent::from_node(&nodes[0]);

nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add_commit.msgs[0]);
nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &update_add_commit.commitment_msg);
check_added_monitors(&nodes[1], 1);
let (raa, cs) = get_revoke_commit_msgs(&nodes[1], &nodes[0].node.get_our_node_id());

nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &raa);
check_added_monitors(&nodes[0], 1);
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
if !not_stale {
nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &cs);
check_added_monitors(&nodes[0], 1);
// A now revokes their original state, at which point reconnect should panic
let raa = get_event_msg!(nodes[0], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());
nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &raa);
check_added_monitors(&nodes[1], 1);
expect_pending_htlcs_forwardable_ignore!(nodes[1]);
}
} else {
send_payment(&nodes[0], &[&nodes[1]], 8000000);
send_payment(&nodes[0], &[&nodes[1]], 8000000);
}

nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());
Expand All @@ -536,99 +567,131 @@ fn do_test_data_loss_protect(reconnect_panicing: bool) {

let reestablish_1 = get_chan_reestablish_msgs!(nodes[0], nodes[1]);

// Check we close channel detecting A is fallen-behind
// Check that we sent the warning message when we detected that A has fallen behind,
// and give the possibility for A to recover from the warning.
// If A has fallen behind substantially, B should send it a message letting it know
// that.
nodes[1].node.handle_channel_reestablish(&nodes[0].node.get_our_node_id(), &reestablish_1[0]);
let warn_msg = "Peer attempted to reestablish channel with a very old local commitment transaction: 0 (received) vs 4 (expected)".to_owned();

let warn_reestablish = nodes[1].node.get_and_clear_pending_msg_events();
assert_eq!(warn_reestablish.len(), 2);
match warn_reestablish[1] {
MessageSendEvent::HandleError { action: ErrorAction::SendWarningMessage { ref msg, .. }, .. } => {
assert_eq!(msg.data, warn_msg);
},
_ => panic!("Unexpected event"),
let reestablish_msg;
if substantially_old {
let warn_msg = "Peer attempted to reestablish channel with a very old local commitment transaction: 0 (received) vs 4 (expected)".to_owned();

let warn_reestablish = nodes[1].node.get_and_clear_pending_msg_events();
assert_eq!(warn_reestablish.len(), 2);
match warn_reestablish[1] {
MessageSendEvent::HandleError { action: ErrorAction::SendWarningMessage { ref msg, .. }, .. } => {
assert_eq!(msg.data, warn_msg);
},
_ => panic!("Unexpected events: {:?}", warn_reestablish),
}
reestablish_msg = match &warn_reestablish[0] {
MessageSendEvent::SendChannelReestablish { msg, .. } => msg.clone(),
_ => panic!("Unexpected events: {:?}", warn_reestablish),
};
} else {
let msgs = nodes[1].node.get_and_clear_pending_msg_events();
assert!(msgs.len() >= 4);
match msgs.last() {
Some(MessageSendEvent::SendChannelUpdate { .. }) => {},
_ => panic!("Unexpected events: {:?}", msgs),
}
assert!(msgs.iter().any(|msg| matches!(msg, MessageSendEvent::SendRevokeAndACK { .. })));
assert!(msgs.iter().any(|msg| matches!(msg, MessageSendEvent::UpdateHTLCs { .. })));
reestablish_msg = match &msgs[0] {
MessageSendEvent::SendChannelReestablish { msg, .. } => msg.clone(),
_ => panic!("Unexpected events: {:?}", msgs),
};
}
let reestablish_msg = match &warn_reestablish[0] {
MessageSendEvent::SendChannelReestablish { msg, .. } => msg.clone(),
_ => panic!("Unexpected event"),
};

{
let mut node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().clone();
// The node B should not broadcast the transaction to force close the channel!
let mut node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
// The node B should never force-close the channel.
assert!(node_txn.is_empty());
}

// Check A panics upon seeing proof it has fallen behind.
assert!(std::panic::catch_unwind(|| {
let reconnect_res = std::panic::catch_unwind(|| {
nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &reestablish_msg);
}).is_err());
std::mem::forget(nodes); // Skip the `Drop` handler for `Node`
return; // By this point we should have panic'ed!
}
});
if not_stale {
assert!(reconnect_res.is_ok());
// At this point A gets confused because B expects a commitment state newer than A
// has sent, but not a newer revocation secret, so A just (correctly) closes.
check_closed_broadcast(&nodes[0], 1, true);
check_added_monitors(&nodes[0], 1);
check_closed_event!(nodes[0], 1, ClosureReason::ProcessingError {
err: "Peer attempted to reestablish channel with a future remote commitment transaction: 2 (received) vs 1 (expected)".to_owned()
}, [nodes[1].node.get_our_node_id()], 1000000);
} else {
assert!(reconnect_res.is_err());
// Skip the `Drop` handler for `Node`s as some may be in an invalid (panicked) state.
std::mem::forget(nodes);
}
} else {
assert!(!not_stale, "We only care about the stale case when not testing panicking");

nodes[0].node.force_close_without_broadcasting_txn(&chan.2, &nodes[1].node.get_our_node_id()).unwrap();
check_added_monitors!(nodes[0], 1);
check_closed_event!(nodes[0], 1, ClosureReason::HolderForceClosed, [nodes[1].node.get_our_node_id()], 1000000);
{
let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
assert_eq!(node_txn.len(), 0);
}
nodes[0].node.force_close_without_broadcasting_txn(&chan.2, &nodes[1].node.get_our_node_id()).unwrap();
check_added_monitors!(nodes[0], 1);
check_closed_event!(nodes[0], 1, ClosureReason::HolderForceClosed, [nodes[1].node.get_our_node_id()], 1000000);
{
let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
assert_eq!(node_txn.len(), 0);
}

for msg in nodes[0].node.get_and_clear_pending_msg_events() {
if let MessageSendEvent::BroadcastChannelUpdate { .. } = msg {
} else if let MessageSendEvent::HandleError { ref action, .. } = msg {
for msg in nodes[0].node.get_and_clear_pending_msg_events() {
if let MessageSendEvent::BroadcastChannelUpdate { .. } = msg {
} else if let MessageSendEvent::HandleError { ref action, .. } = msg {
match action {
&ErrorAction::DisconnectPeer { ref msg } => {
assert_eq!(msg.as_ref().unwrap().data, "Channel force-closed");
},
_ => panic!("Unexpected event!"),
}
} else {
panic!("Unexpected event {:?}", msg)
}
}

// after the warning message sent by B, we should not able to
// use the channel, or reconnect with success to the channel.
assert!(nodes[0].node.list_usable_channels().is_empty());
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();
let retry_reestablish = get_chan_reestablish_msgs!(nodes[1], nodes[0]);

nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &retry_reestablish[0]);
let mut err_msgs_0 = Vec::with_capacity(1);
if let MessageSendEvent::HandleError { ref action, .. } = nodes[0].node.get_and_clear_pending_msg_events()[1] {
match action {
&ErrorAction::DisconnectPeer { ref msg } => {
assert_eq!(msg.as_ref().unwrap().data, "Channel force-closed");
&ErrorAction::SendErrorMessage { ref msg } => {
assert_eq!(msg.data, format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", &nodes[1].node.get_our_node_id()));
err_msgs_0.push(msg.clone());
},
_ => panic!("Unexpected event!"),
}
} else {
panic!("Unexpected event {:?}", msg)
}
}

// after the warning message sent by B, we should not able to
// use the channel, or reconnect with success to the channel.
assert!(nodes[0].node.list_usable_channels().is_empty());
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();
let retry_reestablish = get_chan_reestablish_msgs!(nodes[1], nodes[0]);

nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &retry_reestablish[0]);
let mut err_msgs_0 = Vec::with_capacity(1);
if let MessageSendEvent::HandleError { ref action, .. } = nodes[0].node.get_and_clear_pending_msg_events()[1] {
match action {
&ErrorAction::SendErrorMessage { ref msg } => {
assert_eq!(msg.data, format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", &nodes[1].node.get_our_node_id()));
err_msgs_0.push(msg.clone());
},
_ => panic!("Unexpected event!"),
panic!("Unexpected event!");
}
} else {
panic!("Unexpected event!");
assert_eq!(err_msgs_0.len(), 1);
nodes[1].node.handle_error(&nodes[0].node.get_our_node_id(), &err_msgs_0[0]);
assert!(nodes[1].node.list_usable_channels().is_empty());
check_added_monitors!(nodes[1], 1);
check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: UntrustedString(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", &nodes[1].node.get_our_node_id())) }
, [nodes[0].node.get_our_node_id()], 1000000);
check_closed_broadcast!(nodes[1], false);
}
assert_eq!(err_msgs_0.len(), 1);
nodes[1].node.handle_error(&nodes[0].node.get_our_node_id(), &err_msgs_0[0]);
assert!(nodes[1].node.list_usable_channels().is_empty());
check_added_monitors!(nodes[1], 1);
check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: UntrustedString(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", &nodes[1].node.get_our_node_id())) }
, [nodes[0].node.get_our_node_id()], 1000000);
check_closed_broadcast!(nodes[1], false);
}

#[test]
#[cfg(feature = "std")]
fn test_data_loss_protect() {
do_test_data_loss_protect(true);
do_test_data_loss_protect(false);
do_test_data_loss_protect(true, false, true);
do_test_data_loss_protect(true, true, false);
do_test_data_loss_protect(true, false, false);
do_test_data_loss_protect(false, true, false);
do_test_data_loss_protect(false, false, false);
}

#[test]
Expand Down