diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 115d68acc14..57b95f3e61a 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1863,6 +1863,7 @@ where holder_commitment_point, #[cfg(splicing)] pending_splice: None, + quiescent_action: None, }; let res = funded_channel.initial_commitment_signed_v2(msg, best_block, signer_provider, logger) .map(|monitor| (Some(monitor), None)) @@ -2429,6 +2430,15 @@ impl PendingSplice { } } +pub(crate) enum QuiescentAction { + // TODO: Make this test-only once we have another variant (as some code requires *a* variant). + DoNothing, +} + +impl_writeable_tlv_based_enum_upgradable!(QuiescentAction, + (99, DoNothing) => {}, +); + /// Wrapper around a [`Transaction`] useful for caching the result of [`Transaction::compute_txid`]. struct ConfirmedTransaction<'a> { tx: &'a Transaction, @@ -2728,10 +2738,6 @@ where /// If we can't release a [`ChannelMonitorUpdate`] until some external action completes, we /// store it here and only release it to the `ChannelManager` once it asks for it. blocked_monitor_updates: Vec, - - /// Only set when a counterparty `stfu` has been processed to track which node is allowed to - /// propose "something fundamental" upon becoming quiescent. - is_holder_quiescence_initiator: Option, } /// A channel struct implementing this trait can receive an initial counterparty commitment @@ -3306,8 +3312,6 @@ where blocked_monitor_updates: Vec::new(), is_manual_broadcast: false, - - is_holder_quiescence_initiator: None, }; Ok((funding, channel_context)) @@ -3544,8 +3548,6 @@ where blocked_monitor_updates: Vec::new(), local_initiated_shutdown: None, is_manual_broadcast: false, - - is_holder_quiescence_initiator: None, }; Ok((funding, channel_context)) @@ -6058,6 +6060,12 @@ where /// Info about an in-progress, pending splice (if any), on the pre-splice channel #[cfg(splicing)] pending_splice: Option, + + /// Once we become quiescent, if we're the initiator, there's some action we'll want to take. + /// This keeps track of that action. Note that if we become quiescent and we're not the + /// initiator we may be able to merge this action into what the counterparty wanted to do (e.g. + /// in the case of splicing). + quiescent_action: Option, } #[cfg(splicing)] @@ -8198,11 +8206,13 @@ where // Reset any quiescence-related state as it is implicitly terminated once disconnected. if matches!(self.context.channel_state, ChannelState::ChannelReady(_)) { - self.context.channel_state.clear_awaiting_quiescence(); + if self.quiescent_action.is_some() { + // If we were trying to get quiescent, try again after reconnection. + self.context.channel_state.set_awaiting_quiescence(); + } self.context.channel_state.clear_local_stfu_sent(); self.context.channel_state.clear_remote_stfu_sent(); self.context.channel_state.clear_quiescent(); - self.context.is_holder_quiescence_initiator.take(); } self.context.channel_state.set_peer_disconnected(); @@ -11538,30 +11548,36 @@ where #[cfg(any(test, fuzzing))] #[rustfmt::skip] pub fn propose_quiescence( - &mut self, logger: &L, + &mut self, logger: &L, action: QuiescentAction, ) -> Result, ChannelError> where L::Target: Logger, { log_debug!(logger, "Attempting to initiate quiescence"); - if !self.context.is_live() { + if !self.context.is_usable() { return Err(ChannelError::Ignore( - "Channel is not in a live state to propose quiescence".to_owned() + "Channel is not in a usable state to propose quiescence".to_owned() )); } - if self.context.channel_state.is_quiescent() { - return Err(ChannelError::Ignore("Channel is already quiescent".to_owned())); + if self.quiescent_action.is_some() { + return Err(ChannelError::Ignore("Channel is already quiescing".to_owned())); } - if self.context.channel_state.is_awaiting_quiescence() + self.quiescent_action = Some(action); + if self.context.channel_state.is_quiescent() + || self.context.channel_state.is_awaiting_quiescence() || self.context.channel_state.is_local_stfu_sent() { return Ok(None); } self.context.channel_state.set_awaiting_quiescence(); - Ok(Some(self.send_stfu(logger)?)) + if self.context.is_live() { + Ok(Some(self.send_stfu(logger)?)) + } else { + Ok(None) + } } // Assumes we are either awaiting quiescence or our counterparty has requested quiescence. @@ -11571,7 +11587,6 @@ where L::Target: Logger, { debug_assert!(!self.context.channel_state.is_local_stfu_sent()); - // Either state being set implies the channel is live. debug_assert!( self.context.channel_state.is_awaiting_quiescence() || self.context.channel_state.is_remote_stfu_sent() @@ -11591,18 +11606,10 @@ where self.context.channel_state.clear_awaiting_quiescence(); self.context.channel_state.clear_remote_stfu_sent(); self.context.channel_state.set_quiescent(); - if let Some(initiator) = self.context.is_holder_quiescence_initiator.as_ref() { - log_debug!( - logger, - "Responding to counterparty stfu with our own, channel is now quiescent and we are{} the initiator", - if !initiator { " not" } else { "" } - ); - - *initiator - } else { - debug_assert!(false, "Quiescence initiator must have been set when we received stfu"); - false - } + // We are sending an stfu in response to our couterparty's stfu, but had not yet sent + // our own stfu (even if `awaiting_quiescence` was set). Thus, the counterparty is the + // initiator and they can do "something fundamental". + false } else { log_debug!(logger, "Sending stfu as quiescence initiator"); debug_assert!(self.context.channel_state.is_awaiting_quiescence()); @@ -11633,9 +11640,7 @@ where )); } - if self.context.channel_state.is_awaiting_quiescence() - || !self.context.channel_state.is_local_stfu_sent() - { + if !self.context.channel_state.is_local_stfu_sent() { if !msg.initiator { return Err(ChannelError::WarnAndDisconnect( "Peer sent unexpected `stfu` without signaling as initiator".to_owned() @@ -11649,15 +11654,6 @@ where // then. self.context.channel_state.set_remote_stfu_sent(); - let is_holder_initiator = if self.context.channel_state.is_awaiting_quiescence() { - // We were also planning to propose quiescence, let the tie-breaker decide the - // initiator. - self.funding.is_outbound() - } else { - false - }; - self.context.is_holder_quiescence_initiator = Some(is_holder_initiator); - log_debug!(logger, "Received counterparty stfu proposing quiescence"); return self.send_stfu(logger).map(|stfu| Some(stfu)); } @@ -11665,7 +11661,6 @@ where // We already sent `stfu` and are now processing theirs. It may be in response to ours, or // we happened to both send `stfu` at the same time and a tie-break is needed. let is_holder_quiescence_initiator = !msg.initiator || self.funding.is_outbound(); - self.context.is_holder_quiescence_initiator = Some(is_holder_quiescence_initiator); // We were expecting to receive `stfu` because we already sent ours. self.mark_response_received(); @@ -11693,6 +11688,21 @@ where if !is_holder_quiescence_initiator { " not" } else { "" } ); + if is_holder_quiescence_initiator { + match self.quiescent_action.take() { + None => { + debug_assert!(false); + return Err(ChannelError::WarnAndDisconnect( + "Internal Error: Didn't have anything to do after reaching quiescence".to_owned() + )); + }, + Some(QuiescentAction::DoNothing) => { + // In quiescence test we want to just hang out here, letting the test manually + // leave quiescence. + }, + } + } + Ok(None) } @@ -11708,6 +11718,10 @@ where && self.context.channel_state.is_remote_stfu_sent()) ); + if !self.context.is_live() { + return Ok(None); + } + // We need to send our `stfu`, either because we're trying to initiate quiescence, or the // counterparty is and we've yet to send ours. if self.context.channel_state.is_awaiting_quiescence() @@ -11733,13 +11747,10 @@ where debug_assert!(!self.context.channel_state.is_local_stfu_sent()); debug_assert!(!self.context.channel_state.is_remote_stfu_sent()); - if self.context.channel_state.is_quiescent() { - self.mark_response_received(); - self.context.channel_state.clear_quiescent(); - self.context.is_holder_quiescence_initiator.take().expect("Must always be set while quiescent") - } else { - false - } + self.mark_response_received(); + let was_quiescent = self.context.channel_state.is_quiescent(); + self.context.channel_state.clear_quiescent(); + was_quiescent } pub fn remove_legacy_scids_before_block(&mut self, height: u32) -> alloc::vec::Drain<'_, u64> { @@ -12061,6 +12072,7 @@ where holder_commitment_point, #[cfg(splicing)] pending_splice: None, + quiescent_action: None, }; let need_channel_ready = channel.check_get_channel_ready(0, logger).is_some() @@ -12347,6 +12359,7 @@ where holder_commitment_point, #[cfg(splicing)] pending_splice: None, + quiescent_action: None, }; let need_channel_ready = channel.check_get_channel_ready(0, logger).is_some() || channel.context.signer_pending_channel_ready; @@ -12848,7 +12861,11 @@ where match channel_state { ChannelState::AwaitingChannelReady(_) => {}, ChannelState::ChannelReady(_) => { - channel_state.clear_awaiting_quiescence(); + if self.quiescent_action.is_some() { + // If we're trying to get quiescent to do something, try again when we + // reconnect to the peer. + channel_state.set_awaiting_quiescence(); + } channel_state.clear_local_stfu_sent(); channel_state.clear_remote_stfu_sent(); channel_state.clear_quiescent(); @@ -13256,6 +13273,7 @@ where (60, self.context.historical_scids, optional_vec), // Added in 0.2 (61, fulfill_attribution_data, optional_vec), // Added in 0.2 (63, holder_commitment_point_current, option), // Added in 0.2 + (65, self.quiescent_action, option), // Added in 0.2 }); Ok(()) @@ -13617,6 +13635,8 @@ where let mut minimum_depth_override: Option = None; + let mut quiescent_action = None; + read_tlv_fields!(reader, { (0, announcement_sigs, option), (1, minimum_depth, option), @@ -13660,6 +13680,7 @@ where (60, historical_scids, optional_vec), // Added in 0.2 (61, fulfill_attribution_data, optional_vec), // Added in 0.2 (63, holder_commitment_point_current_opt, option), // Added in 0.2 + (65, quiescent_action, upgradable_option), // Added in 0.2 }); let holder_signer = signer_provider.derive_channel_signer(channel_keys_id); @@ -14001,13 +14022,12 @@ where blocked_monitor_updates: blocked_monitor_updates.unwrap(), is_manual_broadcast: is_manual_broadcast.unwrap_or(false), - - is_holder_quiescence_initiator: None, }, interactive_tx_signing_session, holder_commitment_point, #[cfg(splicing)] pending_splice: None, + quiescent_action, }) } } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index af82f862878..da51b43c69c 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -58,8 +58,8 @@ use crate::events::{ }; use crate::events::{FundingInfo, PaidBolt12Invoice}; use crate::ln::chan_utils::selected_commitment_sat_per_1000_weight; -// Since this struct is returned in `list_channels` methods, expose it here in case users want to -// construct one themselves. +#[cfg(any(test, fuzzing))] +use crate::ln::channel::QuiescentAction; use crate::ln::channel::{ self, hold_time_since, Channel, ChannelError, ChannelUpdateStatus, FundedChannel, InboundV1Channel, OutboundV1Channel, PendingV2Channel, ReconnectionMsg, ShutdownResult, @@ -11712,7 +11712,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ &self.logger, Some(*counterparty_node_id), Some(*channel_id), None ); - match chan.propose_quiescence(&&logger) { + match chan.propose_quiescence(&&logger, QuiescentAction::DoNothing) { Ok(None) => {}, Ok(Some(stfu)) => { peer_state.pending_msg_events.push(MessageSendEvent::SendStfu { diff --git a/lightning/src/ln/quiescence_tests.rs b/lightning/src/ln/quiescence_tests.rs index 211e79adb6d..c13f9e72645 100644 --- a/lightning/src/ln/quiescence_tests.rs +++ b/lightning/src/ln/quiescence_tests.rs @@ -33,7 +33,7 @@ fn test_quiescence_tie() { assert!(stfu_node_0.initiator && stfu_node_1.initiator); assert!(nodes[0].node.exit_quiescence(&nodes[1].node.get_our_node_id(), &chan_id).unwrap()); - assert!(!nodes[1].node.exit_quiescence(&nodes[0].node.get_our_node_id(), &chan_id).unwrap()); + assert!(nodes[1].node.exit_quiescence(&nodes[0].node.get_our_node_id(), &chan_id).unwrap()); } #[test] @@ -173,7 +173,8 @@ fn allow_shutdown_while_awaiting_quiescence(local_shutdown: bool) { // Now that the state machine is no longer pending, and `closing_signed` is ready to be sent, // make sure we're still not waiting for the quiescence handshake to complete. - local_node.node.exit_quiescence(&remote_node_id, &chan_id).unwrap(); + // Note that we never actually reached full quiescence here. + assert!(!local_node.node.exit_quiescence(&remote_node_id, &chan_id).unwrap()); let _ = get_event_msg!(local_node, MessageSendEvent::SendClosingSigned, remote_node_id); check_added_monitors(local_node, 2); // One for the last revoke_and_ack, another for closing_signed @@ -279,8 +280,8 @@ fn test_quiescence_waits_for_async_signer_and_monitor_update() { let stfu = get_event_msg!(&nodes[0], MessageSendEvent::SendStfu, node_id_1); nodes[1].node.handle_stfu(node_id_0, &stfu); - nodes[0].node.exit_quiescence(&node_id_1, &chan_id).unwrap(); - nodes[1].node.exit_quiescence(&node_id_0, &chan_id).unwrap(); + assert!(nodes[0].node.exit_quiescence(&node_id_1, &chan_id).unwrap()); + assert!(nodes[1].node.exit_quiescence(&node_id_0, &chan_id).unwrap()); // After exiting quiescence, we should be able to resume payments from nodes[0]. send_payment(&nodes[0], &[&nodes[1]], payment_amount); @@ -336,8 +337,8 @@ fn test_quiescence_on_final_revoke_and_ack_pending_monitor_update() { panic!(); } - nodes[0].node.exit_quiescence(&node_id_1, &chan_id).unwrap(); - nodes[1].node.exit_quiescence(&node_id_0, &chan_id).unwrap(); + assert!(nodes[0].node.exit_quiescence(&node_id_1, &chan_id).unwrap()); + assert!(nodes[1].node.exit_quiescence(&node_id_0, &chan_id).unwrap()); } #[test] @@ -406,8 +407,8 @@ fn quiescence_updates_go_to_holding_cell(fail_htlc: bool) { let stfu = get_event_msg!(&nodes[0], MessageSendEvent::SendStfu, node_id_1); nodes[1].node.handle_stfu(node_id_0, &stfu); - nodes[0].node.exit_quiescence(&node_id_1, &chan_id).unwrap(); - nodes[1].node.exit_quiescence(&node_id_0, &chan_id).unwrap(); + assert!(nodes[0].node.exit_quiescence(&node_id_1, &chan_id).unwrap()); + assert!(nodes[1].node.exit_quiescence(&node_id_0, &chan_id).unwrap()); // Now that quiescence is over, nodes are allowed to make updates again. nodes[1] will have its // outbound HTLC finally go out, along with the fail/claim of nodes[0]'s payment. @@ -547,3 +548,112 @@ fn test_quiescence_timeout_while_waiting_for_counterparty_stfu() { }; assert!(nodes[1].node.get_and_clear_pending_msg_events().iter().find_map(f).is_some()); } + +fn do_test_quiescence_during_disconnection(with_pending_claim: bool, propose_disconnected: bool) { + // Test that we'll start trying for quiescence immediately after reconnection if we're waiting + // to do some quiescence-required action. + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let nodes = create_network(2, &node_cfgs, &node_chanmgrs); + let chan_id = create_announced_chan_between_nodes(&nodes, 0, 1).2; + + let node_a_id = nodes[0].node.get_our_node_id(); + let node_b_id = nodes[1].node.get_our_node_id(); + + // First get both nodes off the starting state so we don't have to deal with channel_ready + // retransmissions on reconect. + send_payment(&nodes[0], &[&nodes[1]], 100_000); + + let (preimage, payment_hash, ..) = route_payment(&nodes[0], &[&nodes[1]], 100_000); + if with_pending_claim { + // Optionally reconnect with pending quiescence while there's some pending messages to + // deliver. + nodes[1].node.claim_funds(preimage); + check_added_monitors(&nodes[1], 1); + expect_payment_claimed!(nodes[1], payment_hash, 100_000); + let _ = get_htlc_update_msgs(&nodes[1], &node_a_id); + } + + if !propose_disconnected { + nodes[1].node.maybe_propose_quiescence(&node_a_id, &chan_id).unwrap(); + } + + nodes[0].node.peer_disconnected(node_b_id); + nodes[1].node.peer_disconnected(node_a_id); + + if propose_disconnected { + nodes[1].node.maybe_propose_quiescence(&node_a_id, &chan_id).unwrap(); + } + + let init_msg = msgs::Init { + features: nodes[1].node.init_features(), + networks: None, + remote_network_address: None, + }; + nodes[0].node.peer_connected(node_b_id, &init_msg, true).unwrap(); + nodes[1].node.peer_connected(node_a_id, &init_msg, true).unwrap(); + + let reestab_a = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, node_b_id); + let reestab_b = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, node_a_id); + + nodes[0].node.handle_channel_reestablish(node_b_id, &reestab_b); + get_event_msg!(nodes[0], MessageSendEvent::SendChannelUpdate, node_b_id); + + nodes[1].node.handle_channel_reestablish(node_a_id, &reestab_a); + let mut bs_msgs = nodes[1].node.get_and_clear_pending_msg_events(); + bs_msgs.retain(|msg| !matches!(msg, MessageSendEvent::SendChannelUpdate { .. })); + assert_eq!(bs_msgs.len(), 1, "{bs_msgs:?}"); + let stfu = if with_pending_claim { + // Node B should first re-send its channel update, then try to enter quiescence once that + // completes... + let msg = bs_msgs.pop().unwrap(); + if let MessageSendEvent::UpdateHTLCs { mut updates, .. } = msg { + let fulfill = updates.update_fulfill_htlcs.pop().unwrap(); + nodes[0].node.handle_update_fulfill_htlc(node_b_id, fulfill); + let cs = updates.commitment_signed; + nodes[0].node.handle_commitment_signed_batch_test(node_b_id, &cs); + check_added_monitors(&nodes[0], 1); + + let (raa, cs) = get_revoke_commit_msgs(&nodes[0], &node_b_id); + nodes[1].node.handle_revoke_and_ack(node_a_id, &raa); + check_added_monitors(&nodes[1], 1); + nodes[1].node.handle_commitment_signed_batch_test(node_a_id, &cs); + check_added_monitors(&nodes[1], 1); + + let mut bs_raa_stfu = nodes[1].node.get_and_clear_pending_msg_events(); + assert_eq!(bs_raa_stfu.len(), 2); + if let MessageSendEvent::SendRevokeAndACK { msg, .. } = &bs_raa_stfu[0] { + nodes[0].node.handle_revoke_and_ack(node_b_id, &msg); + expect_payment_sent!(&nodes[0], preimage); + } else { + panic!("Unexpected first message {bs_raa_stfu:?}"); + } + + bs_raa_stfu.pop().unwrap() + } else { + panic!("Unexpected message {msg:?}"); + } + } else { + bs_msgs.pop().unwrap() + }; + if let MessageSendEvent::SendStfu { msg, .. } = stfu { + nodes[0].node.handle_stfu(node_b_id, &msg); + } else { + panic!("Unexpected message {stfu:?}"); + } + + let stfu_resp = get_event_msg!(nodes[0], MessageSendEvent::SendStfu, node_b_id); + nodes[1].node.handle_stfu(node_a_id, &stfu_resp); + + assert!(nodes[0].node.exit_quiescence(&node_b_id, &chan_id).unwrap()); + assert!(nodes[1].node.exit_quiescence(&node_a_id, &chan_id).unwrap()); +} + +#[test] +fn test_quiescence_during_disconnection() { + do_test_quiescence_during_disconnection(false, false); + do_test_quiescence_during_disconnection(true, false); + do_test_quiescence_during_disconnection(false, true); + do_test_quiescence_during_disconnection(true, true); +}