Remove check which makes us sometimes never send closing_signed

This is the case pointed out by nayuta-gondo at
lightning/bolts#499 (comment)
though this doesn't actually solve the issue of ensuring we have a
consistent fee view when we start shutdown processing. There isn't
a clear solution to that however without adding additional state
tracking in Channel.

This also removes an associated test that tests for the correct
behavior (but didn't consider the bug) as we no longer behave
correctly. This should be fine as we'll be removing all the
update_fee garbage with option_simplified_commitment anyway.

Author: TheBlueMatt

src/ln/channel.rs

@@ -2406,8 +2406,7 @@ impl Channel {
 	fn maybe_propose_first_closing_signed(&mut self, fee_estimator: &FeeEstimator) -> Option<msgs::ClosingSigned> {
 		if !self.channel_outbound || !self.pending_inbound_htlcs.is_empty() || !self.pending_outbound_htlcs.is_empty() ||
 				self.channel_state & (BOTH_SIDES_SHUTDOWN_MASK | ChannelState::AwaitingRemoteRevoke as u32) != BOTH_SIDES_SHUTDOWN_MASK ||
-				self.last_sent_closing_fee.is_some() ||
-				self.cur_remote_commitment_transaction_number != self.cur_local_commitment_transaction_number{
+				self.last_sent_closing_fee.is_some() || self.pending_update_fee.is_some() {
 			return None;
 		}
 

src/ln/channelmanager.rs

@@ -4921,52 +4921,6 @@ mod tests {
 		assert!(nodes[2].node.list_channels().is_empty());
 	}
 
-	#[test]
-	fn update_fee_async_shutdown() {
-		// Test update_fee works after shutdown start if messages are delivered out-of-order
-		let nodes = create_network(2);
-		let chan_1 = create_announced_chan_between_nodes(&nodes, 0, 1);
-
-		let starting_feerate = nodes[0].node.channel_state.lock().unwrap().by_id.get(&chan_1.2).unwrap().get_feerate();
-		nodes[0].node.update_fee(chan_1.2.clone(), starting_feerate + 20).unwrap();
-		check_added_monitors!(nodes[0], 1);
-		let updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
-		assert!(updates.update_add_htlcs.is_empty());
-		assert!(updates.update_fulfill_htlcs.is_empty());
-		assert!(updates.update_fail_htlcs.is_empty());
-		assert!(updates.update_fail_malformed_htlcs.is_empty());
-		assert!(updates.update_fee.is_some());
-
-		nodes[1].node.close_channel(&chan_1.2).unwrap();
-		let node_1_shutdown = get_event_msg!(nodes[1], MessageSendEvent::SendShutdown, nodes[0].node.get_our_node_id());
-		nodes[0].node.handle_shutdown(&nodes[1].node.get_our_node_id(), &node_1_shutdown).unwrap();
-		// Note that we don't actually test normative behavior here. The spec indicates we could
-		// actually send a closing_signed here, but is kinda unclear and could possibly be amended
-		// to require waiting on the full commitment dance before doing so (see
-		// https://github.com/lightningnetwork/lightning-rfc/issues/499). In any case, to avoid
-		// ambiguity, we should wait until after the full commitment dance to send closing_signed.
-		let node_0_shutdown = get_event_msg!(nodes[0], MessageSendEvent::SendShutdown, nodes[1].node.get_our_node_id());
-
-		nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), &updates.update_fee.unwrap()).unwrap();
-		nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &updates.commitment_signed).unwrap();
-		check_added_monitors!(nodes[1], 1);
-		nodes[1].node.handle_shutdown(&nodes[0].node.get_our_node_id(), &node_0_shutdown).unwrap();
-		let node_0_closing_signed = commitment_signed_dance!(nodes[1], nodes[0], (), false, true, true);
-
-		assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
-		nodes[1].node.handle_closing_signed(&nodes[0].node.get_our_node_id(), match node_0_closing_signed.unwrap() {
-			MessageSendEvent::SendClosingSigned { ref node_id, ref msg } => {
-				assert_eq!(*node_id, nodes[1].node.get_our_node_id());
-				msg
-			},
-			_ => panic!("Unexpected event"),
-		}).unwrap();
-		let (_, node_1_closing_signed) = get_closing_signed_broadcast!(nodes[1].node, nodes[0].node.get_our_node_id());
-		nodes[0].node.handle_closing_signed(&nodes[1].node.get_our_node_id(), &node_1_closing_signed.unwrap()).unwrap();
-		let (_, node_0_none) = get_closing_signed_broadcast!(nodes[0].node, nodes[1].node.get_our_node_id());
-		assert!(node_0_none.is_none());
-	}
-
 	fn do_test_shutdown_rebroadcast(recv_count: u8) {
 		// Test that shutdown/closing_signed is re-sent on reconnect with a variable number of
 		// messages delivered prior to disconnect