Do not Send FundingLocked messages while disconnected

While its generally harmless to do so (the messages will simply be
dropped in `PeerManager`) there is a potential race condition where
the FundingLocked message enters the outbound message queue, then
the peer reconnects, and then the FundingLocked message is
delivered prior to the normal ChannelReestablish flow.

We also take this opportunity to rewrite
`test_funding_peer_disconnect` to be explicit instead of using
`reconnect_peers`. This allows it to check each message being sent
carefully, whereas `reconnect_peers` is rather lazy and accepts
that sometimes signatures will be exchanged, and sometimes not.

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 9415a142b1c8b5231c78663231fcc61ec55bb6e3..0e0b8e5643abe76e03e6675f62296ba6708e60c3 100644 (file)
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -4291,11 +4291,13 @@ impl<Signer: Sign> Channel<Signer> {
 
                if need_commitment_update {
                        if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 {
-                               let next_per_commitment_point = self.holder_signer.get_per_commitment_point(self.cur_holder_commitment_transaction_number, &self.secp_ctx);
-                               return Some(msgs::FundingLocked {
-                                       channel_id: self.channel_id,
-                                       next_per_commitment_point,
-                               });
+                               if self.channel_state & (ChannelState::PeerDisconnected as u32) == 0 {
+                                       let next_per_commitment_point = self.holder_signer.get_per_commitment_point(self.cur_holder_commitment_transaction_number, &self.secp_ctx);
+                                       return Some(msgs::FundingLocked {
+                                               channel_id: self.channel_id,
+                                               next_per_commitment_point,
+                                       });
+                               }
                        } else {
                                self.monitor_pending_funding_locked = true;
                        }

diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index 4b38cb32c27658225f8b61e61da78002b269693f..2b80f35c82aac4b4a41ef2e5f755311a7c197990 100644 (file)
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -3794,15 +3794,7 @@ fn test_funding_peer_disconnect() {
 
        confirm_transaction(&nodes[0], &tx);
        let events_1 = nodes[0].node.get_and_clear_pending_msg_events();
-       let chan_id;
-       assert_eq!(events_1.len(), 1);
-       match events_1[0] {
-               MessageSendEvent::SendFundingLocked { ref node_id, ref msg } => {
-                       assert_eq!(*node_id, nodes[1].node.get_our_node_id());
-                       chan_id = msg.channel_id;
-               },
-               _ => panic!("Unexpected event"),
-       }
+       assert!(events_1.is_empty());
 
        reconnect_nodes(&nodes[0], &nodes[1], (false, true), (0, 0), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
 
@@ -3811,53 +3803,107 @@ fn test_funding_peer_disconnect() {
 
        confirm_transaction(&nodes[1], &tx);
        let events_2 = nodes[1].node.get_and_clear_pending_msg_events();
-       assert_eq!(events_2.len(), 2);
-       let funding_locked = match events_2[0] {
+       assert!(events_2.is_empty());
+
+       nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
+       let as_reestablish = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, nodes[1].node.get_our_node_id());
+       nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });
+       let bs_reestablish = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
+
+       // nodes[0] hasn't yet received a funding_locked, so it only sends that on reconnect.
+       nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &bs_reestablish);
+       let events_3 = nodes[0].node.get_and_clear_pending_msg_events();
+       assert_eq!(events_3.len(), 1);
+       let as_funding_locked = match events_3[0] {
+               MessageSendEvent::SendFundingLocked { ref node_id, ref msg } => {
+                       assert_eq!(*node_id, nodes[1].node.get_our_node_id());
+                       msg.clone()
+               },
+               _ => panic!("Unexpected event {:?}", events_3[0]),
+       };
+
+       // nodes[1] received nodes[0]'s funding_locked on the first reconnect above, so it should send
+       // announcement_signatures as well as channel_update.
+       nodes[1].node.handle_channel_reestablish(&nodes[0].node.get_our_node_id(), &as_reestablish);
+       let events_4 = nodes[1].node.get_and_clear_pending_msg_events();
+       assert_eq!(events_4.len(), 3);
+       let chan_id;
+       let bs_funding_locked = match events_4[0] {
                MessageSendEvent::SendFundingLocked { ref node_id, ref msg } => {
                        assert_eq!(*node_id, nodes[0].node.get_our_node_id());
+                       chan_id = msg.channel_id;
                        msg.clone()
                },
-               _ => panic!("Unexpected event"),
+               _ => panic!("Unexpected event {:?}", events_4[0]),
        };
-       let bs_announcement_sigs = match events_2[1] {
+       let bs_announcement_sigs = match events_4[1] {
                MessageSendEvent::SendAnnouncementSignatures { ref node_id, ref msg } => {
                        assert_eq!(*node_id, nodes[0].node.get_our_node_id());
                        msg.clone()
                },
-               _ => panic!("Unexpected event"),
+               _ => panic!("Unexpected event {:?}", events_4[1]),
        };
+       match events_4[2] {
+               MessageSendEvent::SendChannelUpdate { ref node_id, msg: _ } => {
+                       assert_eq!(*node_id, nodes[0].node.get_our_node_id());
+               },
+               _ => panic!("Unexpected event {:?}", events_4[2]),
+       }
 
-       reconnect_nodes(&nodes[0], &nodes[1], (true, true), (0, 0), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
+       // Re-deliver nodes[0]'s funding_locked, which nodes[1] can safely ignore. It currently
+       // generates a duplicative announcement_signatures
+       nodes[1].node.handle_funding_locked(&nodes[0].node.get_our_node_id(), &as_funding_locked);
+       let events_5 = nodes[1].node.get_and_clear_pending_msg_events();
+       assert_eq!(events_5.len(), 1);
+       match events_5[0] {
+               MessageSendEvent::SendAnnouncementSignatures { ref node_id, ref msg } => {
+                       assert_eq!(*node_id, nodes[0].node.get_our_node_id());
+                       assert_eq!(*msg, bs_announcement_sigs);
+               },
+               _ => panic!("Unexpected event {:?}", events_5[0]),
+       };
 
-       nodes[0].node.handle_funding_locked(&nodes[1].node.get_our_node_id(), &funding_locked);
-       nodes[0].node.handle_announcement_signatures(&nodes[1].node.get_our_node_id(), &bs_announcement_sigs);
-       let events_3 = nodes[0].node.get_and_clear_pending_msg_events();
-       assert_eq!(events_3.len(), 2);
-       let as_announcement_sigs = match events_3[0] {
+       // When we deliver nodes[1]'s funding_locked, however, nodes[0] will generate its
+       // announcement_signatures.
+       nodes[0].node.handle_funding_locked(&nodes[1].node.get_our_node_id(), &bs_funding_locked);
+       let events_6 = nodes[0].node.get_and_clear_pending_msg_events();
+       assert_eq!(events_6.len(), 1);
+       let as_announcement_sigs = match events_6[0] {
                MessageSendEvent::SendAnnouncementSignatures { ref node_id, ref msg } => {
                        assert_eq!(*node_id, nodes[1].node.get_our_node_id());
                        msg.clone()
                },
-               _ => panic!("Unexpected event"),
+               _ => panic!("Unexpected event {:?}", events_6[0]),
        };
-       let (as_announcement, as_update) = match events_3[1] {
+
+       // When we deliver nodes[1]'s announcement_signatures to nodes[0], nodes[0] should immediately
+       // broadcast the channel announcement globally, as well as re-send its (now-public)
+       // channel_update.
+       nodes[0].node.handle_announcement_signatures(&nodes[1].node.get_our_node_id(), &bs_announcement_sigs);
+       let events_7 = nodes[0].node.get_and_clear_pending_msg_events();
+       assert_eq!(events_7.len(), 1);
+       let (chan_announcement, as_update) = match events_7[0] {
                MessageSendEvent::BroadcastChannelAnnouncement { ref msg, ref update_msg } => {
                        (msg.clone(), update_msg.clone())
                },
-               _ => panic!("Unexpected event"),
+               _ => panic!("Unexpected event {:?}", events_7[0]),
        };
 
+       // Finally, deliver nodes[0]'s announcement_signatures to nodes[1] and make sure it creates the
+       // same channel_announcement.
        nodes[1].node.handle_announcement_signatures(&nodes[0].node.get_our_node_id(), &as_announcement_sigs);
-       let events_4 = nodes[1].node.get_and_clear_pending_msg_events();
-       assert_eq!(events_4.len(), 1);
-       let (_, bs_update) = match events_4[0] {
+       let events_8 = nodes[1].node.get_and_clear_pending_msg_events();
+       assert_eq!(events_8.len(), 1);
+       let bs_update = match events_8[0] {
                MessageSendEvent::BroadcastChannelAnnouncement { ref msg, ref update_msg } => {
-                       (msg.clone(), update_msg.clone())
+                       assert_eq!(*msg, chan_announcement);
+                       update_msg.clone()
                },
-               _ => panic!("Unexpected event"),
+               _ => panic!("Unexpected event {:?}", events_8[0]),
        };
 
-       nodes[0].net_graph_msg_handler.handle_channel_announcement(&as_announcement).unwrap();
+       // Provide the channel announcement and public updates to the network graph
+       nodes[0].net_graph_msg_handler.handle_channel_announcement(&chan_announcement).unwrap();
        nodes[0].net_graph_msg_handler.handle_channel_update(&bs_update).unwrap();
        nodes[0].net_graph_msg_handler.handle_channel_update(&as_update).unwrap();
 
@@ -3905,14 +3951,14 @@ fn test_funding_peer_disconnect() {
 
        reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
 
-       // as_announcement should be re-generated exactly by broadcast_node_announcement.
+       // The channel announcement should be re-generated exactly by broadcast_node_announcement.
        nodes[0].node.broadcast_node_announcement([0, 0, 0], [0; 32], Vec::new());
        let msgs = nodes[0].node.get_and_clear_pending_msg_events();
        let mut found_announcement = false;
        for event in msgs.iter() {
                match event {
                        MessageSendEvent::BroadcastChannelAnnouncement { ref msg, .. } => {
-                               if *msg == as_announcement { found_announcement = true; }
+                               if *msg == chan_announcement { found_announcement = true; }
                        },
                        MessageSendEvent::BroadcastNodeAnnouncement { .. } => {},
                        _ => panic!("Unexpected event"),