Merge pull request #2529 from TheBlueMatt/2023-08-shutdown-remove-early-sign

Don't send init `closing_signed` too early after final HTLC removal

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -816,6 +816,19 @@ pub(super) struct ChannelContext<SP: Deref> where SP::Target: SignerProvider {
 	#[cfg(not(test))]
 	closing_fee_limits: Option<(u64, u64)>,
 
+	/// If we remove an HTLC (or fee update), commit, and receive our counterparty's
+	/// `revoke_and_ack`, we remove all knowledge of said HTLC (or fee update). However, the latest
+	/// local commitment transaction that we can broadcast still contains the HTLC (or old fee)
+	/// until we receive a further `commitment_signed`. Thus we are not eligible for initiating the
+	/// `closing_signed` negotiation if we're expecting a counterparty `commitment_signed`.
+	///
+	/// To ensure we don't send a `closing_signed` too early, we track this state here, waiting
+	/// until we see a `commitment_signed` before doing so.
+	///
+	/// We don't bother to persist this - we anticipate this state won't last longer than a few
+	/// milliseconds, so any accidental force-closes here should be exceedingly rare.
+	expecting_peer_commitment_signed: bool,
+
 	/// The hash of the block in which the funding transaction was included.
 	funding_tx_confirmed_in: Option<BlockHash>,
 	funding_tx_confirmation_height: u32,
@@ -3249,6 +3262,7 @@ impl<SP: Deref> Channel<SP> where
 		};
 
 		self.context.cur_holder_commitment_transaction_number -= 1;
+		self.context.expecting_peer_commitment_signed = false;
 		// Note that if we need_commitment & !AwaitingRemoteRevoke we'll call
 		// build_commitment_no_status_check() next which will reset this to RAAFirst.
 		self.context.resend_order = RAACommitmentOrder::CommitmentFirst;
@@ -3514,6 +3528,7 @@ impl<SP: Deref> Channel<SP> where
 			// Take references explicitly so that we can hold multiple references to self.context.
 			let pending_inbound_htlcs: &mut Vec<_> = &mut self.context.pending_inbound_htlcs;
 			let pending_outbound_htlcs: &mut Vec<_> = &mut self.context.pending_outbound_htlcs;
+			let expecting_peer_commitment_signed = &mut self.context.expecting_peer_commitment_signed;
 
 			// We really shouldnt have two passes here, but retain gives a non-mutable ref (Rust bug)
 			pending_inbound_htlcs.retain(|htlc| {
@@ -3522,6 +3537,7 @@ impl<SP: Deref> Channel<SP> where
 					if let &InboundHTLCRemovalReason::Fulfill(_) = reason {
 						value_to_self_msat_diff += htlc.amount_msat as i64;
 					}
+					*expecting_peer_commitment_signed = true;
 					false
 				} else { true }
 			});
@@ -3581,6 +3597,7 @@ impl<SP: Deref> Channel<SP> where
 				if let OutboundHTLCState::LocalAnnounced(_) = htlc.state {
 					log_trace!(logger, " ...promoting outbound LocalAnnounced {} to Committed", &htlc.payment_hash);
 					htlc.state = OutboundHTLCState::Committed;
+					*expecting_peer_commitment_signed = true;
 				}
 				if let &mut OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref mut outcome) = &mut htlc.state {
 					log_trace!(logger, " ...promoting outbound AwaitingRemoteRevokeToRemove {} to AwaitingRemovedRemoteRevoke", &htlc.payment_hash);
@@ -3601,6 +3618,7 @@ impl<SP: Deref> Channel<SP> where
 					log_trace!(logger, " ...promoting outbound fee update {} to Committed", feerate);
 					self.context.feerate_per_kw = feerate;
 					self.context.pending_update_fee = None;
+					self.context.expecting_peer_commitment_signed = true;
 				},
 				FeeUpdateState::RemoteAnnounced => { debug_assert!(!self.context.is_outbound()); },
 				FeeUpdateState::AwaitingRemoteRevokeToAnnounce => {
@@ -4381,6 +4399,10 @@ impl<SP: Deref> Channel<SP> where
 		-> Result<(Option<msgs::ClosingSigned>, Option<Transaction>, Option<ShutdownResult>), ChannelError>
 		where F::Target: FeeEstimator, L::Target: Logger
 	{
+		// If we're waiting on a monitor persistence, that implies we're also waiting to send some
+		// message to our counterparty (probably a `revoke_and_ack`). In such a case, we shouldn't
+		// initiate `closing_signed` negotiation until we're clear of all pending messages. Note
+		// that closing_negotiation_ready checks this case (as well as a few others).
 		if self.context.last_sent_closing_fee.is_some() || !self.closing_negotiation_ready() {
 			return Ok((None, None, None));
 		}
@@ -4392,6 +4414,12 @@ impl<SP: Deref> Channel<SP> where
 			return Ok((None, None, None));
 		}
 
+		// If we're waiting on a counterparty `commitment_signed` to clear some updates from our
+		// local commitment transaction, we can't yet initiate `closing_signed` negotiation.
+		if self.context.expecting_peer_commitment_signed {
+			return Ok((None, None, None));
+		}
+
 		let (our_min_fee, our_max_fee) = self.calculate_closing_fee_limits(fee_estimator);
 
 		assert!(self.context.shutdown_scriptpubkey.is_some());
@@ -6005,6 +6033,7 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
 
 				last_sent_closing_fee: None,
 				pending_counterparty_closing_signed: None,
+				expecting_peer_commitment_signed: false,
 				closing_fee_limits: None,
 				target_closing_feerate_sats_per_kw: None,
 
@@ -6637,6 +6666,7 @@ impl<SP: Deref> InboundV1Channel<SP> where SP::Target: SignerProvider {
 
 				last_sent_closing_fee: None,
 				pending_counterparty_closing_signed: None,
+				expecting_peer_commitment_signed: false,
 				closing_fee_limits: None,
 				target_closing_feerate_sats_per_kw: None,
 
@@ -7708,6 +7738,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
 
 				last_sent_closing_fee: None,
 				pending_counterparty_closing_signed: None,
+				expecting_peer_commitment_signed: false,
 				closing_fee_limits: None,
 				target_closing_feerate_sats_per_kw,
 

lightning/src/ln/shutdown_tests.rs

@@ -10,8 +10,9 @@
 //! Tests of our shutdown and closing_signed negotiation logic.
 
 use crate::sign::{EntropySource, SignerProvider};
+use crate::chain::ChannelMonitorUpdateStatus;
 use crate::chain::transaction::OutPoint;
-use crate::events::{MessageSendEvent, MessageSendEventsProvider, ClosureReason};
+use crate::events::{MessageSendEvent, HTLCDestination, MessageSendEventsProvider, ClosureReason};
 use crate::ln::channelmanager::{self, PaymentSendFailure, PaymentId, RecipientOnionFields, ChannelShutdownState, ChannelDetails};
 use crate::routing::router::{PaymentParameters, get_route, RouteParameters};
 use crate::ln::msgs;
@@ -1237,3 +1238,102 @@ fn simple_target_feerate_shutdown() {
 	check_closed_event!(nodes[0], 1, ClosureReason::CooperativeClosure, [nodes[1].node.get_our_node_id()], 100000);
 	check_closed_event!(nodes[1], 1, ClosureReason::CooperativeClosure, [nodes[0].node.get_our_node_id()], 100000);
 }
+
+fn do_outbound_update_no_early_closing_signed(use_htlc: bool) {
+	// Previously, if we have a pending inbound HTLC (or fee update) on a channel which has
+	// initiated shutdown, we'd send our initial closing_signed immediately after receiving the
+	// peer's last RAA to remove the HTLC/fee update, but before receiving their final
+	// commitment_signed for a commitment without the HTLC/with the new fee. This caused at least
+	// LDK peers to force-close as we initiated closing_signed prior to the channel actually being
+	// fully empty of pending updates/HTLCs.
+	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;
+
+	send_payment(&nodes[0], &[&nodes[1]], 1_000_000);
+	let payment_hash_opt = if use_htlc {
+		Some(route_payment(&nodes[1], &[&nodes[0]], 10_000).1)
+	} else {
+		None
+	};
+
+	if use_htlc {
+		nodes[0].node.fail_htlc_backwards(&payment_hash_opt.unwrap());
+		expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[0],
+			[HTLCDestination::FailedPayment { payment_hash: payment_hash_opt.unwrap() }]);
+	} else {
+		*chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap() *= 10;
+		nodes[0].node.timer_tick_occurred();
+	}
+	let updates = get_htlc_update_msgs(&nodes[0], &nodes[1].node.get_our_node_id());
+	check_added_monitors(&nodes[0], 1);
+
+	nodes[1].node.close_channel(&chan_id, &nodes[0].node.get_our_node_id()).unwrap();
+	let node_0_shutdown = get_event_msg!(nodes[1], MessageSendEvent::SendShutdown, nodes[0].node.get_our_node_id());
+	nodes[0].node.close_channel(&chan_id, &nodes[1].node.get_our_node_id()).unwrap();
+	let node_1_shutdown = get_event_msg!(nodes[0], MessageSendEvent::SendShutdown, nodes[1].node.get_our_node_id());
+
+	nodes[0].node.handle_shutdown(&nodes[1].node.get_our_node_id(), &node_0_shutdown);
+	nodes[1].node.handle_shutdown(&nodes[0].node.get_our_node_id(), &node_1_shutdown);
+
+	if use_htlc {
+		nodes[1].node.handle_update_fail_htlc(&nodes[0].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
+	} else {
+		nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), &updates.update_fee.unwrap());
+	}
+	nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &updates.commitment_signed);
+	check_added_monitors(&nodes[1], 1);
+	let (bs_raa, bs_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(), &bs_raa);
+	check_added_monitors(&nodes[0], 1);
+
+	// At this point the Channel on nodes[0] has no record of any HTLCs but the latest
+	// broadcastable commitment does contain the HTLC (but only the ChannelMonitor knows this).
+	// Thus, the channel should not yet initiate closing_signed negotiation (but previously did).
+	assert_eq!(nodes[0].node.get_and_clear_pending_msg_events(), Vec::new());
+
+	chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
+	nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_cs);
+	check_added_monitors(&nodes[0], 1);
+	assert_eq!(nodes[0].node.get_and_clear_pending_msg_events(), Vec::new());
+
+	expect_channel_shutdown_state!(nodes[0], chan_id, ChannelShutdownState::ResolvingHTLCs);
+	assert_eq!(nodes[0].node.get_and_clear_pending_msg_events(), Vec::new());
+	let (outpoint, latest_update, _) = nodes[0].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone();
+	nodes[0].chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint, latest_update);
+
+	let as_raa_closing_signed = nodes[0].node.get_and_clear_pending_msg_events();
+	assert_eq!(as_raa_closing_signed.len(), 2);
+
+	if let MessageSendEvent::SendRevokeAndACK { msg, .. } = &as_raa_closing_signed[0] {
+		nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &msg);
+		check_added_monitors(&nodes[1], 1);
+		if use_htlc {
+			expect_payment_failed!(nodes[1], payment_hash_opt.unwrap(), true);
+		}
+	} else { panic!("Unexpected message {:?}", as_raa_closing_signed[0]); }
+
+	if let MessageSendEvent::SendClosingSigned { msg, .. } = &as_raa_closing_signed[1] {
+		nodes[1].node.handle_closing_signed(&nodes[0].node.get_our_node_id(), &msg);
+	} else { panic!("Unexpected message {:?}", as_raa_closing_signed[1]); }
+
+	let bs_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(), &bs_closing_signed);
+	let (_, as_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(), &as_2nd_closing_signed.unwrap());
+	let (_, node_1_none) = get_closing_signed_broadcast!(nodes[1].node, nodes[0].node.get_our_node_id());
+	assert!(node_1_none.is_none());
+
+	check_closed_event!(nodes[0], 1, ClosureReason::CooperativeClosure, [nodes[1].node.get_our_node_id()], 100000);
+	check_closed_event!(nodes[1], 1, ClosureReason::CooperativeClosure, [nodes[0].node.get_our_node_id()], 100000);
+}
+
+#[test]
+fn outbound_update_no_early_closing_signed() {
+	do_outbound_update_no_early_closing_signed(true);
+	do_outbound_update_no_early_closing_signed(false);
+}