diff --git a/lightning/src/ln/async_signer_tests.rs b/lightning/src/ln/async_signer_tests.rs index a8d15d8af06..a6801c8dc00 100644 --- a/lightning/src/ln/async_signer_tests.rs +++ b/lightning/src/ln/async_signer_tests.rs @@ -854,11 +854,13 @@ fn test_async_holder_signatures_remote_commitment_anchors() { #[test] fn test_closing_signed() { - do_test_closing_signed(false); - do_test_closing_signed(true); + do_test_closing_signed(false, false); + do_test_closing_signed(true, false); + do_test_closing_signed(false, true); + do_test_closing_signed(true, true); } -fn do_test_closing_signed(extra_closing_signed: bool) { +fn do_test_closing_signed(extra_closing_signed: bool, reconnect: bool) { // Based off of `expect_channel_shutdown_state`. // Test that we can asynchronously sign closing transactions. let chanmon_cfgs = create_chanmon_cfgs(2); @@ -867,6 +869,9 @@ fn do_test_closing_signed(extra_closing_signed: bool) { let nodes = create_network(2, &node_cfgs, &node_chanmgrs); let chan_1 = create_announced_chan_between_nodes(&nodes, 0, 1); + // Avoid extra channel ready message upon reestablish later + send_payment(&nodes[0], &vec![&nodes[1]][..], 8_000_000); + expect_channel_shutdown_state!(nodes[0], chan_1.2, ChannelShutdownState::NotShuttingDown); nodes[0].node.close_channel(&chan_1.2, &nodes[1].node.get_our_node_id()).unwrap(); @@ -914,7 +919,7 @@ fn do_test_closing_signed(extra_closing_signed: bool) { let mut node_1_closing_signed_2 = node_1_closing_signed.clone(); let holder_script = nodes[0].keys_manager.get_shutdown_scriptpubkey().unwrap(); let counterparty_script = nodes[1].keys_manager.get_shutdown_scriptpubkey().unwrap(); - let funding_outpoint = bitcoin::OutPoint { txid: chan_1.3.txid(), vout: 0 }; + let funding_outpoint = bitcoin::OutPoint { txid: chan_1.3.compute_txid(), vout: 0 }; let closing_tx_2 = ClosingTransaction::new(50000, 0, holder_script.into(), counterparty_script.into(), funding_outpoint); @@ -941,9 +946,45 @@ fn do_test_closing_signed(extra_closing_signed: bool) { }; } + if reconnect { + 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].fee_estimator.sat_per_kw.lock().unwrap() *= 8; + *nodes[1].fee_estimator.sat_per_kw.lock().unwrap() *= 8; + + connect_nodes(&nodes[0], &nodes[1]); + let node_0_reestablish = get_chan_reestablish_msgs!(nodes[0], nodes[1]).pop().unwrap(); + let node_1_reestablish = get_chan_reestablish_msgs!(nodes[1], nodes[0]).pop().unwrap(); + nodes[1].node.handle_channel_reestablish(&nodes[0].node.get_our_node_id(), &node_0_reestablish); + nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &node_1_reestablish); + + let node_0_msgs = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(node_0_msgs.len(), 2); + let node_0_2nd_shutdown = match node_0_msgs[0] { + MessageSendEvent::SendShutdown { ref msg, .. } => { + msg.clone() + }, + _ => panic!(), + }; + let node_0_2nd_closing_signed = match node_0_msgs[1] { + MessageSendEvent::SendClosingSigned { ref msg, .. } => { + msg.clone() + }, + _ => panic!(), + }; + let node_1_2nd_shutdown = get_event_msg!(nodes[1], MessageSendEvent::SendShutdown, nodes[0].node.get_our_node_id()); + + nodes[1].node.handle_shutdown(&nodes[0].node.get_our_node_id(), &node_0_2nd_shutdown); + assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); + nodes[0].node.handle_shutdown(&nodes[1].node.get_our_node_id(), &node_1_2nd_shutdown); + nodes[1].node.handle_closing_signed(&nodes[0].node.get_our_node_id(), &node_0_2nd_closing_signed); + let node_1_closing_signed = get_event_msg!(nodes[1], MessageSendEvent::SendClosingSigned, nodes[0].node.get_our_node_id()); + nodes[0].node.handle_closing_signed(&nodes[1].node.get_our_node_id(), &node_1_closing_signed); + } + nodes[0].node.signer_unblocked(None); let (_, node_0_2nd_closing_signed) = get_closing_signed_broadcast!(nodes[0].node, nodes[1].node.get_our_node_id()); - nodes[1].node.handle_closing_signed(&nodes[0].node.get_our_node_id(), &node_0_2nd_closing_signed.unwrap()); let (_, node_1_closing_signed) = get_closing_signed_broadcast!(nodes[1].node, nodes[0].node.get_our_node_id()); assert!(node_1_closing_signed.is_none()); @@ -952,4 +993,5 @@ fn do_test_closing_signed(extra_closing_signed: bool) { assert!(nodes[1].node.list_channels().is_empty()); check_closed_event!(nodes[0], 1, ClosureReason::LocallyInitiatedCooperativeClosure, [nodes[1].node.get_our_node_id()], 100000); check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyInitiatedCooperativeClosure, [nodes[0].node.get_our_node_id()], 100000); + } diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 641a61c3140..8fe8a33400b 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -909,6 +909,7 @@ pub(super) struct SignerResumeUpdates { pub order: RAACommitmentOrder, pub closing_signed: Option, pub signed_closing_tx: Option, + pub shutdown_result: Option, } /// The return value of `channel_reestablish` @@ -5508,7 +5509,7 @@ impl Channel where commitment_update = None; } - let (closing_signed, signed_closing_tx) = if self.context.signer_pending_closing { + let (closing_signed, signed_closing_tx, shutdown_result) = if self.context.signer_pending_closing { debug_assert!(self.context.last_sent_closing_fee.is_some()); if let Some((fee, skip_remote_output, fee_range, holder_sig)) = self.context.last_sent_closing_fee.clone() { debug_assert!(holder_sig.is_none()); @@ -5524,19 +5525,21 @@ impl Channel where &self.context.get_counterparty_pubkeys().funding_pubkey).is_ok()); Some(self.build_signed_closing_transaction(&closing_tx, &counterparty_sig, signature)) } else { None }; - (closing_signed, signed_tx) - } else { (None, None) } - } else { (None, None) }; + let shutdown_result = signed_tx.as_ref().map(|_| self.shutdown_result_coop_close()); + (closing_signed, signed_tx, shutdown_result) + } else { (None, None, None) } + } else { (None, None, None) }; log_trace!(logger, "Signer unblocked with {} commitment_update, {} revoke_and_ack, with resend order {:?}, {} funding_signed, {} channel_ready, - {} closing_signed, and {} signed_closing_tx", + {} closing_signed, {} signed_closing_tx, and {} shutdown result", if commitment_update.is_some() { "a" } else { "no" }, if revoke_and_ack.is_some() { "a" } else { "no" }, self.context.resend_order, if funding_signed.is_some() { "a" } else { "no" }, if channel_ready.is_some() { "a" } else { "no" }, if closing_signed.is_some() { "a" } else { "no" }, - if signed_closing_tx.is_some() { "a" } else { "no" }); + if signed_closing_tx.is_some() { "a" } else { "no" }, + if shutdown_result.is_some() { "a" } else { "no" }); SignerResumeUpdates { commitment_update, @@ -5546,6 +5549,7 @@ impl Channel where order: self.context.resend_order.clone(), closing_signed, signed_closing_tx, + shutdown_result, } } @@ -6170,6 +6174,27 @@ impl Channel where }) } + fn shutdown_result_coop_close(&self) -> ShutdownResult { + let closure_reason = if self.initiated_shutdown() { + ClosureReason::LocallyInitiatedCooperativeClosure + } else { + ClosureReason::CounterpartyInitiatedCooperativeClosure + }; + ShutdownResult { + closure_reason, + monitor_update: None, + dropped_outbound_htlcs: Vec::new(), + unbroadcasted_batch_funding_txid: self.context.unbroadcasted_batch_funding_txid(), + channel_id: self.context.channel_id, + user_channel_id: self.context.user_id, + channel_capacity_satoshis: self.context.channel_value_satoshis, + counterparty_node_id: self.context.counterparty_node_id, + unbroadcasted_funding_tx: self.context.unbroadcasted_funding(), + is_manual_broadcast: self.context.is_manual_broadcast, + channel_funding_txo: self.context.get_funding_txo(), + } + } + pub fn closing_signed( &mut self, fee_estimator: &LowerBoundedFeeEstimator, msg: &msgs::ClosingSigned, logger: &L) -> Result<(Option, Option, Option), ChannelError> @@ -6226,28 +6251,10 @@ impl Channel where } } - let closure_reason = if self.initiated_shutdown() { - ClosureReason::LocallyInitiatedCooperativeClosure - } else { - ClosureReason::CounterpartyInitiatedCooperativeClosure - }; - assert!(self.context.shutdown_scriptpubkey.is_some()); if let Some((last_fee, _, _, Some(sig))) = self.context.last_sent_closing_fee { if last_fee == msg.fee_satoshis { - let shutdown_result = ShutdownResult { - closure_reason, - monitor_update: None, - dropped_outbound_htlcs: Vec::new(), - unbroadcasted_batch_funding_txid: self.context.unbroadcasted_batch_funding_txid(), - channel_id: self.context.channel_id, - user_channel_id: self.context.user_id, - channel_capacity_satoshis: self.context.channel_value_satoshis, - counterparty_node_id: self.context.counterparty_node_id, - unbroadcasted_funding_tx: self.context.unbroadcasted_funding(), - is_manual_broadcast: self.context.is_manual_broadcast, - channel_funding_txo: self.context.get_funding_txo(), - }; + let shutdown_result = self.shutdown_result_coop_close(); let tx = self.build_signed_closing_transaction(&mut closing_tx, &msg.signature, &sig); self.context.channel_state = ChannelState::ShutdownComplete; self.context.update_time_counter += 1; @@ -6268,19 +6275,8 @@ impl Channel where let closing_signed = self.get_closing_signed_msg(&closing_tx, skip_remote_output, used_fee, our_min_fee, our_max_fee, logger); let (signed_tx, shutdown_result) = if $new_fee == msg.fee_satoshis { - let shutdown_result = ShutdownResult { - closure_reason, - monitor_update: None, - dropped_outbound_htlcs: Vec::new(), - unbroadcasted_batch_funding_txid: self.context.unbroadcasted_batch_funding_txid(), - channel_id: self.context.channel_id, - user_channel_id: self.context.user_id, - channel_capacity_satoshis: self.context.channel_value_satoshis, - counterparty_node_id: self.context.counterparty_node_id, - unbroadcasted_funding_tx: self.context.unbroadcasted_funding(), - is_manual_broadcast: self.context.is_manual_broadcast, - channel_funding_txo: self.context.get_funding_txo(), - }; + let shutdown_result = closing_signed.as_ref() + .map(|_| self.shutdown_result_coop_close()); if closing_signed.is_some() { self.context.channel_state = ChannelState::ShutdownComplete; } @@ -6288,7 +6284,7 @@ impl Channel where self.context.last_received_closing_sig = Some(msg.signature.clone()); let tx = closing_signed.as_ref().map(|ClosingSigned { signature, .. }| self.build_signed_closing_transaction(&closing_tx, &msg.signature, signature)); - (tx, Some(shutdown_result)) + (tx, shutdown_result) } else { (None, None) }; diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index b3047930f61..85dc48946ea 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -7827,7 +7827,7 @@ where if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() { let logger = WithChannelContext::from(&self.logger, &chan.context, None); let (closing_signed, tx, shutdown_result) = try_chan_phase_entry!(self, chan.closing_signed(&self.fee_estimator, &msg, &&logger), chan_phase_entry); - debug_assert_eq!(shutdown_result.is_some(), chan.is_shutdown() || chan.is_shutdown_pending_signature()); + debug_assert_eq!(shutdown_result.is_some(), chan.is_shutdown()); if let Some(msg) = closing_signed { peer_state.pending_msg_events.push(events::MessageSendEvent::SendClosingSigned { node_id: counterparty_node_id.clone(), @@ -8650,7 +8650,7 @@ where let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); // Returns whether we should remove this channel as it's just been closed. - let unblock_chan = |phase: &mut ChannelPhase, pending_msg_events: &mut Vec| -> bool { + let unblock_chan = |phase: &mut ChannelPhase, pending_msg_events: &mut Vec| -> Option { let node_id = phase.context().get_counterparty_node_id(); match phase { ChannelPhase::Funded(chan) => { @@ -8703,11 +8703,8 @@ where msg: update }); } - - // We should return true to remove the channel if we just - // broadcasted the closing transaction. - true - } else { false } + } + msgs.shutdown_result } ChannelPhase::UnfundedOutboundV1(chan) => { if let Some(msg) = chan.signer_maybe_unblocked(&self.logger) { @@ -8716,12 +8713,13 @@ where msg, }); } - false + None } - ChannelPhase::UnfundedInboundV1(_) => false, + ChannelPhase::UnfundedInboundV1(_) => None, } }; + let mut shutdown_results = Vec::new(); let per_peer_state = self.per_peer_state.read().unwrap(); let per_peer_state_iter = per_peer_state.iter().filter(|(cp_id, _)| { if let Some((counterparty_node_id, _)) = channel_opt { @@ -8732,16 +8730,23 @@ where let mut peer_state_lock = peer_state_mutex.lock().unwrap(); let peer_state = &mut *peer_state_lock; peer_state.channel_by_id.retain(|_, chan| { - let should_remove = match channel_opt { - Some((_, channel_id)) if chan.context().channel_id() != channel_id => false, + let shutdown_result = match channel_opt { + Some((_, channel_id)) if chan.context().channel_id() != channel_id => None, _ => unblock_chan(chan, &mut peer_state.pending_msg_events), }; - if should_remove { + if let Some(shutdown_result) = shutdown_result { log_trace!(self.logger, "Removing channel after unblocking signer"); + shutdown_results.push(shutdown_result); + false + } else { + true } - !should_remove }); } + drop(per_peer_state); + for shutdown_result in shutdown_results.drain(..) { + self.finish_close_channel(shutdown_result); + } } /// Check whether any channels have finished removing all pending updates after a shutdown @@ -8770,8 +8775,8 @@ where node_id: chan.context.get_counterparty_node_id(), msg, }); } + debug_assert_eq!(shutdown_result_opt.is_some(), chan.is_shutdown()); if let Some(shutdown_result) = shutdown_result_opt { - debug_assert!(chan.is_shutdown() || chan.is_shutdown_pending_signature()); shutdown_results.push(shutdown_result); } if let Some(tx) = tx_opt { diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index f3290bf1583..6b0a8a23182 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -3295,7 +3295,7 @@ pub fn create_network<'a, 'b: 'a, 'c: 'b>(node_count: usize, cfgs: &'b Vec(node_a: &Node<'a, 'b, 'c>, node_b: &Node<'a, 'b, 'c>) { +pub fn connect_nodes<'a, 'b: 'a, 'c: 'b>(node_a: &Node<'a, 'b, 'c>, node_b: &Node<'a, 'b, 'c>) { let node_id_a = node_a.node.get_our_node_id(); let node_id_b = node_b.node.get_our_node_id();