Delay closing_signed until update_fee exchanges complete

See lightning/bolts#499
for a bit more about the ambiguity we're addressing here.

Also drop holding cell update_fee the same way we drop holding
cell update_add_htlcs when sending shutdown, resolving a bug.

Author: TheBlueMatt

src/ln/channel.rs

@@ -1709,7 +1709,8 @@ impl Channel {
 			let (msg, monitor) = self.send_commitment_no_status_check()?;
 			(Some(msg), monitor, None)
 		} else if !need_our_commitment && self.channel_state & (BOTH_SIDES_SHUTDOWN_MASK | ChannelState::AwaitingRemoteRevoke as u32) == BOTH_SIDES_SHUTDOWN_MASK &&
-				self.channel_outbound && self.pending_inbound_htlcs.is_empty() && self.pending_outbound_htlcs.is_empty() && self.last_sent_closing_fee.is_none() {
+				self.channel_outbound && self.pending_inbound_htlcs.is_empty() && self.pending_outbound_htlcs.is_empty() && self.last_sent_closing_fee.is_none() &&
+				self.cur_remote_commitment_transaction_number == self.cur_local_commitment_transaction_number {
 			(None, self.channel_monitor.clone(), Some(self.propose_first_closing_signed(fee_estimator)))
 		} else { (None, self.channel_monitor.clone(), None) };
 
@@ -1976,7 +1977,9 @@ impl Channel {
 						commitment_signed
 					}), to_forward_infos, revoked_htlcs, None, monitor_update))
 				} else {
-					if self.channel_state & BOTH_SIDES_SHUTDOWN_MASK == BOTH_SIDES_SHUTDOWN_MASK && self.channel_outbound && self.pending_inbound_htlcs.is_empty() && self.pending_outbound_htlcs.is_empty() && self.last_sent_closing_fee.is_none() {
+					if self.channel_state & BOTH_SIDES_SHUTDOWN_MASK == BOTH_SIDES_SHUTDOWN_MASK && self.channel_outbound &&
+							self.pending_inbound_htlcs.is_empty() && self.pending_outbound_htlcs.is_empty() && self.last_sent_closing_fee.is_none() &&
+							self.cur_remote_commitment_transaction_number == self.cur_local_commitment_transaction_number {
 						Ok((None, to_forward_infos, revoked_htlcs, Some(self.propose_first_closing_signed(fee_estimator)), self.channel_monitor.clone()))
 					} else {
 						Ok((None, to_forward_infos, revoked_htlcs, None, self.channel_monitor.clone()))
@@ -2149,6 +2152,9 @@ impl Channel {
 		if self.channel_outbound {
 			return Err(ChannelError::Close("Non-funding remote tried to update channel fee"));
 		}
+		if (self.channel_state & (ChannelState::ChannelFunded as u32 | ChannelState::RemoteShutdownSent as u32)) != (ChannelState::ChannelFunded as u32) {
+			return Err(ChannelError::Close("Got update_fee message when channel was not in an operational state"));
+		}
 		if self.channel_state & (ChannelState::PeerDisconnected as u32) == ChannelState::PeerDisconnected as u32 {
 			return Err(ChannelError::Close("Peer sent update_fee when we needed a channel_reestablish"));
 		}
@@ -2414,6 +2420,7 @@ impl Channel {
 		// We can't send our shutdown until we've committed all of our pending HTLCs, but the
 		// remote side is unlikely to accept any new HTLCs, so we go ahead and "free" any holding
 		// cell HTLCs and return them to fail the payment.
+		self.holding_cell_update_fee = None;
 		let mut dropped_outbound_htlcs = Vec::with_capacity(self.holding_cell_htlc_updates.len());
 		self.holding_cell_htlc_updates.retain(|htlc_update| {
 			match htlc_update {
@@ -2439,7 +2446,9 @@ impl Channel {
 
 		self.channel_state |= ChannelState::LocalShutdownSent as u32;
 		self.channel_update_count += 1;
-		if self.channel_outbound && self.pending_inbound_htlcs.is_empty() && self.pending_outbound_htlcs.is_empty() {
+		if self.channel_outbound && self.pending_inbound_htlcs.is_empty() && self.pending_outbound_htlcs.is_empty() &&
+				self.cur_remote_commitment_transaction_number == self.cur_local_commitment_transaction_number &&
+				self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32) == 0 {
 			// There are no more HTLCs and we're the funder, this means we start the closing_signed
 			// dance with an initial fee proposal!
 			Ok((our_shutdown, Some(self.propose_first_closing_signed(fee_estimator)), dropped_outbound_htlcs))
@@ -3224,9 +3233,9 @@ impl Channel {
 		}
 		self.channel_update_count += 1;
 
-		// We can't send our shutdown until we've committed all of our pending HTLCs, but the
-		// remote side is unlikely to accept any new HTLCs, so we go ahead and "free" any holding
-		// cell HTLCs and return them to fail the payment.
+		// Go ahead and drop holding cell updates as we'd rather fail payments than wait to send
+		// our shutdown until we've committed all of the pending changes.
+		self.holding_cell_update_fee = None;
 		let mut dropped_outbound_htlcs = Vec::with_capacity(self.holding_cell_htlc_updates.len());
 		self.holding_cell_htlc_updates.retain(|htlc_update| {
 			match htlc_update {