Remove check which makes us sometimes never send closing_signed

This is the case pointed out by nayuta-gondo at
https://github.com/lightningnetwork/lightning-rfc/issues/499#issuecomment-438623208
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.

diff --git a/src/ln/channel.rs b/src/ln/channel.rs
index 296d97c164ea967839fa2f6ec93ab66f215f56fe..f6177e9eb6b9cc2f1786c60247b1fcaecae8e5ac 100644 (file)
--- a/src/ln/channel.rs
+++ b/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;
                }
 

diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs
index a47236469719e181b9a33fce00e049502a858ae0..fc8af4ccaf25c89148dc6ff05f608cd3ba462373 100644 (file)
--- a/src/ln/channelmanager.rs
+++ b/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