Skip to content

Wait to free the holding cell during channel_reestablish handling #1859

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 4 commits into from
Nov 22, 2022
Merged

Wait to free the holding cell during channel_reestablish handling #1859

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c8c0997
Remove log assertions in `chanmon_update_fail_tests`
TheBlueMatt Nov 17, 2022
7e9b88a
Wait to free the holding cell during channel_reestablish handling
TheBlueMatt Nov 17, 2022
f1c6cd8
Convert the `handle_chan_restoration_locked` macro to a function
TheBlueMatt Nov 17, 2022
e82cfa7
Remove the `post_handle_chan_restoration` macro
TheBlueMatt Nov 21, 2022
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
30 changes: 6 additions & 24 deletions lightning/src/ln/chanmon_update_fail_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,6 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), commitment_signed);
check_added_monitors!(nodes[0], 1);
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented generation of RAA".to_string(), 1);
}

(update_fulfill_htlcs[0].clone(), commitment_signed.clone())
Expand Down Expand Up @@ -633,7 +632,6 @@ fn test_monitor_update_fail_cs() {
chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &send_event.commitment_msg);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
check_added_monitors!(nodes[1], 1);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());

Expand Down Expand Up @@ -664,7 +662,6 @@ fn test_monitor_update_fail_cs() {
chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &updates.commitment_signed);
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
check_added_monitors!(nodes[0], 1);
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
},
Expand Down Expand Up @@ -726,7 +723,6 @@ fn test_monitor_update_fail_no_rebroadcast() {
chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &bs_raa);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
check_added_monitors!(nodes[1], 1);
Expand Down Expand Up @@ -784,12 +780,11 @@ fn test_monitor_update_raa_while_paused() {
nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &send_event_2.msgs[0]);
nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &send_event_2.commitment_msg);
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
check_added_monitors!(nodes[0], 1);
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());

nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_raa);
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Existing pending monitor update prevented responses to RAA".to_string(), 1);
check_added_monitors!(nodes[0], 1);

chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed);
Expand Down Expand Up @@ -875,7 +870,6 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_revoke_and_ack);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
check_added_monitors!(nodes[1], 1);
Expand Down Expand Up @@ -911,8 +905,6 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &send_event.commitment_msg);
check_added_monitors!(nodes[1], 1);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented generation of RAA".to_string(), 1);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
(Some(payment_preimage_4), Some(payment_hash_4))
} else { (None, None) };

Expand Down Expand Up @@ -1136,7 +1128,7 @@ fn test_monitor_update_fail_reestablish() {
get_event_msg!(nodes[0], MessageSendEvent::SendChannelUpdate, nodes[1].node.get_our_node_id())
.contents.flags & 2, 0); // The "disabled" bit should be unset as we just reconnected

nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
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);
Expand Down Expand Up @@ -1228,12 +1220,11 @@ fn raa_no_response_awaiting_raa_state() {
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &payment_event.commitment_msg);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
check_added_monitors!(nodes[1], 1);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());

nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_raa);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Existing pending monitor update prevented responses to RAA".to_string(), 1);
check_added_monitors!(nodes[1], 1);

chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed);
Expand Down Expand Up @@ -1331,7 +1322,6 @@ fn claim_while_disconnected_monitor_update_fail() {

nodes[1].node.handle_channel_reestablish(&nodes[0].node.get_our_node_id(), &as_reconnect);
let _bs_channel_update = get_event_msg!(nodes[1], MessageSendEvent::SendChannelUpdate, nodes[0].node.get_our_node_id());
nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
check_added_monitors!(nodes[1], 1);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());

Expand All @@ -1348,7 +1338,6 @@ fn claim_while_disconnected_monitor_update_fail() {
nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_updates.commitment_signed);
check_added_monitors!(nodes[1], 1);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented generation of RAA".to_string(), 1);
// Note that nodes[1] not updating monitor here is OK - it wont take action on the new HTLC
// until we've channel_monitor_update'd and updated for the new commitment transaction.

Expand Down Expand Up @@ -1440,7 +1429,6 @@ fn monitor_failed_no_reestablish_response() {
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &payment_event.commitment_msg);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
check_added_monitors!(nodes[1], 1);

// Now disconnect and immediately reconnect, delivering the channel_reestablish while nodes[1]
Expand Down Expand Up @@ -1540,7 +1528,6 @@ fn first_message_on_recv_ordering() {
// to the next message also tests resetting the delivery order.
nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_raa);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
check_added_monitors!(nodes[1], 1);

// Now deliver the update_add_htlc/commitment_signed for the second payment, which does need an
Expand All @@ -1550,7 +1537,6 @@ fn first_message_on_recv_ordering() {
nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &payment_event.commitment_msg);
check_added_monitors!(nodes[1], 1);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented generation of RAA".to_string(), 1);

chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed);
let (outpoint, latest_update, _) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
Expand Down Expand Up @@ -1599,7 +1585,7 @@ fn test_monitor_update_fail_claim() {
chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
nodes[1].node.claim_funds(payment_preimage_1);
expect_payment_claimed!(nodes[1], payment_hash_1, 1_000_000);
nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Temporary failure claiming HTLC, treating as success: Failed to update ChannelMonitor".to_string(), 1);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
check_added_monitors!(nodes[1], 1);

// Note that at this point there is a pending commitment transaction update for A being held by
Expand Down Expand Up @@ -1730,7 +1716,6 @@ fn test_monitor_update_on_pending_forwards() {
expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], vec![HTLCDestination::NextHopChannel { node_id: Some(nodes[2].node.get_our_node_id()), channel_id: chan_2.2 }]);
check_added_monitors!(nodes[1], 1);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);

chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed);
let (outpoint, latest_update, _) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_1.2).unwrap().clone();
Expand Down Expand Up @@ -1791,9 +1776,7 @@ fn monitor_update_claim_fail_no_response() {
expect_payment_claimed!(nodes[1], payment_hash_1, 1_000_000);
check_added_monitors!(nodes[1], 1);

let events = nodes[1].node.get_and_clear_pending_msg_events();
assert_eq!(events.len(), 0);
nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Temporary failure claiming HTLC, treating as success: Failed to update ChannelMonitor".to_string(), 1);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());

chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed);
let (outpoint, latest_update, _) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
Expand Down Expand Up @@ -1841,9 +1824,8 @@ fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf:

chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id()));
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
check_added_monitors!(nodes[0], 1);
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed);
let (outpoint, latest_update, _) = nodes[0].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
Expand Down
59 changes: 9 additions & 50 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,8 +439,6 @@ pub(super) struct ReestablishResponses {
pub raa: Option<msgs::RevokeAndACK>,
pub commitment_update: Option<msgs::CommitmentUpdate>,
pub order: RAACommitmentOrder,
pub mon_update: Option<ChannelMonitorUpdate>,
pub holding_cell_failed_htlcs: Vec<(HTLCSource, PaymentHash)>,
pub announcement_sigs: Option<msgs::AnnouncementSignatures>,
pub shutdown_msg: Option<msgs::Shutdown>,
}
Expand Down Expand Up @@ -3955,9 +3953,8 @@ impl<Signer: Sign> Channel<Signer> {
// Short circuit the whole handler as there is nothing we can resend them
return Ok(ReestablishResponses {
channel_ready: None,
raa: None, commitment_update: None, mon_update: None,
raa: None, commitment_update: None,
order: RAACommitmentOrder::CommitmentFirst,
holding_cell_failed_htlcs: Vec::new(),
shutdown_msg, announcement_sigs,
});
}
Expand All @@ -3970,9 +3967,8 @@ impl<Signer: Sign> Channel<Signer> {
next_per_commitment_point,
short_channel_id_alias: Some(self.outbound_scid_alias),
}),
raa: None, commitment_update: None, mon_update: None,
raa: None, commitment_update: None,
order: RAACommitmentOrder::CommitmentFirst,
holding_cell_failed_htlcs: Vec::new(),
shutdown_msg, announcement_sigs,
});
}
Expand Down Expand Up @@ -4015,46 +4011,12 @@ impl<Signer: Sign> Channel<Signer> {
log_debug!(logger, "Reconnected channel {} with no loss", log_bytes!(self.channel_id()));
}

if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateInProgress as u32)) == 0 {
// We're up-to-date and not waiting on a remote revoke (if we are our
// channel_reestablish should result in them sending a revoke_and_ack), but we may
// have received some updates while we were disconnected. Free the holding cell
// now!
match self.free_holding_cell_htlcs(logger) {
Err(ChannelError::Close(msg)) => Err(ChannelError::Close(msg)),
Err(ChannelError::Warn(_)) | Err(ChannelError::Ignore(_)) =>
panic!("Got non-channel-failing result from free_holding_cell_htlcs"),
Ok((Some((commitment_update, monitor_update)), holding_cell_failed_htlcs)) => {
Ok(ReestablishResponses {
channel_ready, shutdown_msg, announcement_sigs,
raa: required_revoke,
commitment_update: Some(commitment_update),
order: self.resend_order.clone(),
mon_update: Some(monitor_update),
holding_cell_failed_htlcs,
})
},
Ok((None, holding_cell_failed_htlcs)) => {
Ok(ReestablishResponses {
channel_ready, shutdown_msg, announcement_sigs,
raa: required_revoke,
commitment_update: None,
order: self.resend_order.clone(),
mon_update: None,
holding_cell_failed_htlcs,
})
},
}
} else {
Ok(ReestablishResponses {
channel_ready, shutdown_msg, announcement_sigs,
raa: required_revoke,
commitment_update: None,
order: self.resend_order.clone(),
mon_update: None,
holding_cell_failed_htlcs: Vec::new(),
})
}
Ok(ReestablishResponses {
channel_ready, shutdown_msg, announcement_sigs,
raa: required_revoke,
commitment_update: None,
order: self.resend_order.clone(),
})
} else if msg.next_local_commitment_number == next_counterparty_commitment_number - 1 {
if required_revoke.is_some() {
log_debug!(logger, "Reconnected channel {} with lost outbound RAA and lost remote commitment tx", log_bytes!(self.channel_id()));
Expand All @@ -4066,18 +4028,15 @@ impl<Signer: Sign> Channel<Signer> {
self.monitor_pending_commitment_signed = true;
Ok(ReestablishResponses {
channel_ready, shutdown_msg, announcement_sigs,
commitment_update: None, raa: None, mon_update: None,
commitment_update: None, raa: None,
order: self.resend_order.clone(),
holding_cell_failed_htlcs: Vec::new(),
})
} else {
Ok(ReestablishResponses {
channel_ready, shutdown_msg, announcement_sigs,
raa: required_revoke,
commitment_update: Some(self.get_last_commitment_update(logger)),
order: self.resend_order.clone(),
mon_update: None,
holding_cell_failed_htlcs: Vec::new(),
})
}
} else {
Expand Down
Loading