Send announcement_signatures msgs out-of-band for ordered delivery

Author: TheBlueMatt

src/ln/channelmanager.rs

@@ -1762,8 +1762,9 @@ impl ChannelManager {
 		Ok(())
 	}
 
-	fn internal_funding_locked(&self, their_node_id: &PublicKey, msg: &msgs::FundingLocked) -> Result<Option<msgs::AnnouncementSignatures>, MsgHandleErrInternal> {
-		let mut channel_state = self.channel_state.lock().unwrap();
+	fn internal_funding_locked(&self, their_node_id: &PublicKey, msg: &msgs::FundingLocked) -> Result<(), MsgHandleErrInternal> {
+		let mut channel_state_lock = self.channel_state.lock().unwrap();
+		let channel_state = channel_state_lock.borrow_parts();
 		match channel_state.by_id.get_mut(&msg.channel_id) {
 			Some(chan) => {
 				if chan.get_their_node_id() != *their_node_id {
@@ -1772,10 +1773,16 @@ impl ChannelManager {
 				}
 				chan.funding_locked(&msg)
 					.map_err(|e| MsgHandleErrInternal::from_chan_maybe_close(e, msg.channel_id))?;
-				return Ok(self.get_announcement_sigs(chan));
+				if let Some(announcement_sigs) = self.get_announcement_sigs(chan) {
+					channel_state.pending_msg_events.push(events::MessageSendEvent::SendAnnouncementSignatures {
+						node_id: their_node_id.clone(),
+						msg: announcement_sigs,
+					});
+				}
+				Ok(())
 			},
-			None => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel", msg.channel_id))
-		};
+			None => Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel", msg.channel_id))
+		}
 	}
 
 	fn internal_shutdown(&self, their_node_id: &PublicKey, msg: &msgs::Shutdown) -> Result<(Option<msgs::Shutdown>, Option<msgs::ClosingSigned>), MsgHandleErrInternal> {
@@ -2363,12 +2370,16 @@ impl ChainListener for ChannelManager {
 			channel_state.by_id.retain(|_, channel| {
 				let chan_res = channel.block_connected(header, height, txn_matched, indexes_of_txn_matched);
 				if let Ok(Some(funding_locked)) = chan_res {
-					let announcement_sigs = self.get_announcement_sigs(channel);
 					pending_msg_events.push(events::MessageSendEvent::SendFundingLocked {
 						node_id: channel.get_their_node_id(),
 						msg: funding_locked,
-						announcement_sigs: announcement_sigs
 					});
+					if let Some(announcement_sigs) = self.get_announcement_sigs(channel) {
+						pending_msg_events.push(events::MessageSendEvent::SendAnnouncementSignatures {
+							node_id: channel.get_their_node_id(),
+							msg: announcement_sigs,
+						});
+					}
 					short_to_id.insert(channel.get_short_channel_id().unwrap(), channel.channel_id());
 				} else if let Err(e) = chan_res {
 					pending_msg_events.push(events::MessageSendEvent::HandleError {
@@ -2507,7 +2518,7 @@ impl ChannelMessageHandler for ChannelManager {
 		handle_error!(self, self.internal_funding_signed(their_node_id, msg), their_node_id)
 	}
 
-	fn handle_funding_locked(&self, their_node_id: &PublicKey, msg: &msgs::FundingLocked) -> Result<Option<msgs::AnnouncementSignatures>, HandleError> {
+	fn handle_funding_locked(&self, their_node_id: &PublicKey, msg: &msgs::FundingLocked) -> Result<(), HandleError> {
 		handle_error!(self, self.internal_funding_locked(their_node_id, msg), their_node_id)
 	}
 
@@ -2954,30 +2965,27 @@ mod tests {
 
 	fn create_chan_between_nodes_with_value_confirm(node_a: &Node, node_b: &Node, tx: &Transaction) -> ((msgs::FundingLocked, msgs::AnnouncementSignatures), [u8; 32]) {
 		confirm_transaction(&node_b.chain_monitor, &tx, tx.version);
-		let events_5 = node_b.node.get_and_clear_pending_msg_events();
-		assert_eq!(events_5.len(), 1);
-		match events_5[0] {
-			MessageSendEvent::SendFundingLocked { ref node_id, ref msg, ref announcement_sigs } => {
-				assert_eq!(*node_id, node_a.node.get_our_node_id());
-				assert!(announcement_sigs.is_none());
-				node_a.node.handle_funding_locked(&node_b.node.get_our_node_id(), msg).unwrap()
-			},
-			_ => panic!("Unexpected event"),
-		};
+		node_a.node.handle_funding_locked(&node_b.node.get_our_node_id(), &get_event_msg!(node_b, MessageSendEvent::SendFundingLocked, node_a.node.get_our_node_id())).unwrap();
 
 		let channel_id;
 
 		confirm_transaction(&node_a.chain_monitor, &tx, tx.version);
 		let events_6 = node_a.node.get_and_clear_pending_msg_events();
-		assert_eq!(events_6.len(), 1);
-		(match events_6[0] {
-			MessageSendEvent::SendFundingLocked { ref node_id, ref msg, ref announcement_sigs } => {
+		assert_eq!(events_6.len(), 2);
+		((match events_6[0] {
+			MessageSendEvent::SendFundingLocked { ref node_id, ref msg } => {
 				channel_id = msg.channel_id.clone();
 				assert_eq!(*node_id, node_b.node.get_our_node_id());
-				(msg.clone(), announcement_sigs.clone().unwrap())
+				msg.clone()
+			},
+			_ => panic!("Unexpected event"),
+		}, match events_6[1] {
+			MessageSendEvent::SendAnnouncementSignatures { ref node_id, ref msg } => {
+				assert_eq!(*node_id, node_b.node.get_our_node_id());
+				msg.clone()
 			},
 			_ => panic!("Unexpected event"),
-		}, channel_id)
+		}), channel_id)
 	}
 
 	fn create_chan_between_nodes_with_value_a(node_a: &Node, node_b: &Node, channel_value: u64, push_msat: u64) -> ((msgs::FundingLocked, msgs::AnnouncementSignatures), [u8; 32], Transaction) {
@@ -2987,11 +2995,9 @@ mod tests {
 	}
 
 	fn create_chan_between_nodes_with_value_b(node_a: &Node, node_b: &Node, as_funding_msgs: &(msgs::FundingLocked, msgs::AnnouncementSignatures)) -> (msgs::ChannelAnnouncement, msgs::ChannelUpdate, msgs::ChannelUpdate) {
-		let bs_announcement_sigs = {
-			let bs_announcement_sigs = node_b.node.handle_funding_locked(&node_a.node.get_our_node_id(), &as_funding_msgs.0).unwrap().unwrap();
-			node_b.node.handle_announcement_signatures(&node_a.node.get_our_node_id(), &as_funding_msgs.1).unwrap();
-			bs_announcement_sigs
-		};
+		node_b.node.handle_funding_locked(&node_a.node.get_our_node_id(), &as_funding_msgs.0).unwrap();
+		let bs_announcement_sigs = get_event_msg!(node_b, MessageSendEvent::SendAnnouncementSignatures, node_a.node.get_our_node_id());
+		node_b.node.handle_announcement_signatures(&node_a.node.get_our_node_id(), &as_funding_msgs.1).unwrap();
 
 		let events_7 = node_b.node.get_and_clear_pending_msg_events();
 		assert_eq!(events_7.len(), 1);
@@ -5034,9 +5040,14 @@ mod tests {
 
 		for chan_msgs in resp_1.drain(..) {
 			if pre_all_htlcs {
-				let a = node_a.node.handle_funding_locked(&node_b.node.get_our_node_id(), &chan_msgs.0.unwrap());
-				let _announcement_sigs_opt = a.unwrap();
-				//TODO: Test announcement_sigs re-sending when we've implemented it
+				node_a.node.handle_funding_locked(&node_b.node.get_our_node_id(), &chan_msgs.0.unwrap()).unwrap();
+				let announcement_event = node_a.node.get_and_clear_pending_msg_events();
+				if !announcement_event.is_empty() {
+					assert_eq!(announcement_event.len(), 1);
+					if let MessageSendEvent::SendAnnouncementSignatures { .. } = announcement_event[0] {
+						//TODO: Test announcement_sigs re-sending
+					} else { panic!("Unexpected event!"); }
+				}
 			} else {
 				assert!(chan_msgs.0.is_none());
 			}
@@ -5083,8 +5094,14 @@ mod tests {
 
 		for chan_msgs in resp_2.drain(..) {
 			if pre_all_htlcs {
-				let _announcement_sigs_opt = node_b.node.handle_funding_locked(&node_a.node.get_our_node_id(), &chan_msgs.0.unwrap()).unwrap();
-				//TODO: Test announcement_sigs re-sending when we've implemented it
+				node_b.node.handle_funding_locked(&node_a.node.get_our_node_id(), &chan_msgs.0.unwrap()).unwrap();
+				let announcement_event = node_b.node.get_and_clear_pending_msg_events();
+				if !announcement_event.is_empty() {
+					assert_eq!(announcement_event.len(), 1);
+					if let MessageSendEvent::SendAnnouncementSignatures { .. } = announcement_event[0] {
+						//TODO: Test announcement_sigs re-sending
+					} else { panic!("Unexpected event!"); }
+				}
 			} else {
 				assert!(chan_msgs.0.is_none());
 			}
@@ -5390,9 +5407,8 @@ mod tests {
 		let events_1 = nodes[0].node.get_and_clear_pending_msg_events();
 		assert_eq!(events_1.len(), 1);
 		match events_1[0] {
-			MessageSendEvent::SendFundingLocked { ref node_id, msg: _, ref announcement_sigs } => {
+			MessageSendEvent::SendFundingLocked { ref node_id, msg: _ } => {
 				assert_eq!(*node_id, nodes[1].node.get_our_node_id());
-				assert!(announcement_sigs.is_none());
 			},
 			_ => panic!("Unexpected event"),
 		}
@@ -5401,9 +5417,8 @@ mod tests {
 		let events_2 = nodes[1].node.get_and_clear_pending_msg_events();
 		assert_eq!(events_2.len(), 1);
 		match events_2[0] {
-			MessageSendEvent::SendFundingLocked { ref node_id, msg: _, ref announcement_sigs } => {
+			MessageSendEvent::SendFundingLocked { ref node_id, msg: _ } => {
 				assert_eq!(*node_id, nodes[0].node.get_our_node_id());
-				assert!(announcement_sigs.is_none());
 			},
 			_ => panic!("Unexpected event"),
 		}

src/ln/msgs.rs

@@ -536,7 +536,7 @@ pub trait ChannelMessageHandler : events::MessageSendEventsProvider + Send + Syn
 	/// Handle an incoming funding_signed message from the given peer.
 	fn handle_funding_signed(&self, their_node_id: &PublicKey, msg: &FundingSigned) -> Result<(), HandleError>;
 	/// Handle an incoming funding_locked message from the given peer.
-	fn handle_funding_locked(&self, their_node_id: &PublicKey, msg: &FundingLocked) -> Result<Option<AnnouncementSignatures>, HandleError>;
+	fn handle_funding_locked(&self, their_node_id: &PublicKey, msg: &FundingLocked) -> Result<(), HandleError>;
 
 	// Channl close:
 	/// Handle an incoming shutdown message from the given peer.

src/ln/peer_handler.rs

@@ -576,11 +576,7 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 											},
 											36 => {
 												let msg = try_potential_decodeerror!(msgs::FundingLocked::read(&mut reader));
-												let resp_option = try_potential_handleerror!(self.message_handler.chan_handler.handle_funding_locked(&peer.their_node_id.unwrap(), &msg));
-												match resp_option {
-													Some(resp) => encode_and_send_msg!(resp, 259),
-													None => {},
-												}
+												try_potential_handleerror!(self.message_handler.chan_handler.handle_funding_locked(&peer.their_node_id.unwrap(), &msg));
 											},
 
 											38 => {
@@ -828,19 +824,25 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 						peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(msg, 35)));
 						Self::do_attempt_write_data(&mut descriptor, peer);
 					},
-					MessageSendEvent::SendFundingLocked { ref node_id, ref msg, ref announcement_sigs } => {
-						log_trace!(self, "Handling SendFundingLocked event in peer_handler for node {}{} for channel {}",
+					MessageSendEvent::SendFundingLocked { ref node_id, ref msg } => {
+						log_trace!(self, "Handling SendFundingLocked event in peer_handler for node {} for channel {}",
 								log_pubkey!(node_id),
-								if announcement_sigs.is_some() { " with announcement sigs" } else { "" },
 								log_bytes!(msg.channel_id));
 						let (mut descriptor, peer) = get_peer_for_forwarding!(node_id, {
 								//TODO: Do whatever we're gonna do for handling dropped messages
 							});
 						peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(msg, 36)));
-						match announcement_sigs {
-							&Some(ref announce_msg) => peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(announce_msg, 259))),
-							&None => {},
-						}
+						Self::do_attempt_write_data(&mut descriptor, peer);
+					},
+					MessageSendEvent::SendAnnouncementSignatures { ref node_id, ref msg } => {
+						log_trace!(self, "Handling SendAnnouncementSignatures event in peer_handler for node {} for channel {})",
+								log_pubkey!(node_id),
+								log_bytes!(msg.channel_id));
+						let (mut descriptor, peer) = get_peer_for_forwarding!(node_id, {
+								//TODO: generate a DiscardFunding event indicating to the wallet that
+								//they should just throw away this funding transaction
+							});
+						peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(msg, 259)));
 						Self::do_attempt_write_data(&mut descriptor, peer);
 					},
 					MessageSendEvent::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref update_fail_malformed_htlcs, ref update_fee, ref commitment_signed } } => {

src/util/events.rs

@@ -127,8 +127,13 @@ pub enum MessageSendEvent {
 		node_id: PublicKey,
 		/// The funding_locked message which should be sent.
 		msg: msgs::FundingLocked,
-		/// An optional additional announcement_signatures message which should be sent.
-		announcement_sigs: Option<msgs::AnnouncementSignatures>,
+	},
+	/// Used to indicate that an announcement_signatures message should be sent to the peer with the given node_id.
+	SendAnnouncementSignatures {
+		/// The node_id of the node which should receive these message(s)
+		node_id: PublicKey,
+		/// The announcement_signatures message which should be sent.
+		msg: msgs::AnnouncementSignatures,
 	},
 	/// Used to indicate that a series of HTLC update messages, as well as a commitment_signed
 	/// message should be sent to the peer with the given node_id.

src/util/test_utils.rs

@@ -98,7 +98,7 @@ impl msgs::ChannelMessageHandler for TestChannelMessageHandler {
 	fn handle_funding_signed(&self, _their_node_id: &PublicKey, _msg: &msgs::FundingSigned) -> Result<(), HandleError> {
 		Err(HandleError { err: "", action: None })
 	}
-	fn handle_funding_locked(&self, _their_node_id: &PublicKey, _msg: &msgs::FundingLocked) -> Result<Option<msgs::AnnouncementSignatures>, HandleError> {
+	fn handle_funding_locked(&self, _their_node_id: &PublicKey, _msg: &msgs::FundingLocked) -> Result<(), HandleError> {
 		Err(HandleError { err: "", action: None })
 	}
 	fn handle_shutdown(&self, _their_node_id: &PublicKey, _msg: &msgs::Shutdown) -> Result<(Option<msgs::Shutdown>, Option<msgs::ClosingSigned>), HandleError> {