Send funding_signed messages out-of-band to ensure ordered delivery

Author: TheBlueMatt

fuzz/fuzz_targets/full_stack_target.rs

@@ -757,7 +757,7 @@ mod tests {
 
 		let log_entries = logger.lines.lock().unwrap();
 		assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Handling SendAcceptChannel event in peer_handler for node 030000000000000000000000000000000000000000000000000000000000000000 for channel ff4f00f805273c1b203bb5ebf8436bfde57b3be8c2f5e95d9491dbb181909679".to_string())), Some(&1)); // 1
-		assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Encoding and sending message of type 35 to 030000000000000000000000000000000000000000000000000000000000000000".to_string())), Some(&1)); // 2
+		assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Handling SendFundingSigned event in peer_handler for node 030000000000000000000000000000000000000000000000000000000000000000 for channel 3d00000000000000000000000000000000000000000000000000000000000000".to_string())), Some(&1)); // 2
 		assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Handling SendFundingLocked event in peer_handler for node 030000000000000000000000000000000000000000000000000000000000000000 for channel 3d00000000000000000000000000000000000000000000000000000000000000".to_string())), Some(&1)); // 3
 		assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Handling SendFundingLocked event in peer_handler for node 030200000000000000000000000000000000000000000000000000000000000000 for channel 3f00000000000000000000000000000000000000000000000000000000000000".to_string())), Some(&1)); // 4
 		assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Encoding and sending message of type 133 to 030000000000000000000000000000000000000000000000000000000000000000".to_string())), Some(&4)); // 5

src/ln/channelmanager.rs

@@ -1693,7 +1693,7 @@ impl ChannelManager {
 		Ok(())
 	}
 
-	fn internal_funding_created(&self, their_node_id: &PublicKey, msg: &msgs::FundingCreated) -> Result<msgs::FundingSigned, MsgHandleErrInternal> {
+	fn internal_funding_created(&self, their_node_id: &PublicKey, msg: &msgs::FundingCreated) -> Result<(), MsgHandleErrInternal> {
 		let (chan, funding_msg, monitor_update) = {
 			let mut channel_state = self.channel_state.lock().unwrap();
 			match channel_state.by_id.entry(msg.temporary_channel_id.clone()) {
@@ -1719,16 +1719,21 @@ impl ChannelManager {
 		if let Err(_e) = self.monitor.add_update_monitor(monitor_update.get_funding_txo().unwrap(), monitor_update) {
 			unimplemented!();
 		}
-		let mut channel_state = self.channel_state.lock().unwrap();
+		let mut channel_state_lock = self.channel_state.lock().unwrap();
+		let channel_state = channel_state_lock.borrow_parts();
 		match channel_state.by_id.entry(funding_msg.channel_id) {
 			hash_map::Entry::Occupied(_) => {
 				return Err(MsgHandleErrInternal::send_err_msg_no_close("Already had channel with the new channel_id", funding_msg.channel_id))
 			},
 			hash_map::Entry::Vacant(e) => {
+				channel_state.pending_msg_events.push(events::MessageSendEvent::SendFundingSigned {
+					node_id: their_node_id.clone(),
+					msg: funding_msg,
+				});
 				e.insert(chan);
 			}
 		}
-		Ok(funding_msg)
+		Ok(())
 	}
 
 	fn internal_funding_signed(&self, their_node_id: &PublicKey, msg: &msgs::FundingSigned) -> Result<(), MsgHandleErrInternal> {
@@ -2494,7 +2499,7 @@ impl ChannelMessageHandler for ChannelManager {
 		handle_error!(self, self.internal_accept_channel(their_node_id, msg), their_node_id)
 	}
 
-	fn handle_funding_created(&self, their_node_id: &PublicKey, msg: &msgs::FundingCreated) -> Result<msgs::FundingSigned, HandleError> {
+	fn handle_funding_created(&self, their_node_id: &PublicKey, msg: &msgs::FundingCreated) -> Result<(), HandleError> {
 		handle_error!(self, self.internal_funding_created(their_node_id, msg), their_node_id)
 	}
 
@@ -2918,22 +2923,15 @@ mod tests {
 			_ => panic!("Unexpected event"),
 		}
 
-		let events_3 = node_a.node.get_and_clear_pending_msg_events();
-		assert_eq!(events_3.len(), 1);
-		let funding_signed = match events_3[0] {
-			MessageSendEvent::SendFundingCreated { ref node_id, ref msg } => {
-				assert_eq!(*node_id, node_b.node.get_our_node_id());
-				let res = node_b.node.handle_funding_created(&node_a.node.get_our_node_id(), msg).unwrap();
-				let mut added_monitors = node_b.chan_monitor.added_monitors.lock().unwrap();
-				assert_eq!(added_monitors.len(), 1);
-				assert_eq!(added_monitors[0].0, funding_output);
-				added_monitors.clear();
-				res
-			},
-			_ => panic!("Unexpected event"),
-		};
+		node_b.node.handle_funding_created(&node_a.node.get_our_node_id(), &get_event_msg!(node_a, MessageSendEvent::SendFundingCreated, node_b.node.get_our_node_id())).unwrap();
+		{
+			let mut added_monitors = node_b.chan_monitor.added_monitors.lock().unwrap();
+			assert_eq!(added_monitors.len(), 1);
+			assert_eq!(added_monitors[0].0, funding_output);
+			added_monitors.clear();
+		}
 
-		node_a.node.handle_funding_signed(&node_b.node.get_our_node_id(), &funding_signed).unwrap();
+		node_a.node.handle_funding_signed(&node_b.node.get_our_node_id(), &get_event_msg!(node_b, MessageSendEvent::SendFundingSigned, node_a.node.get_our_node_id())).unwrap();
 		{
 			let mut added_monitors = node_a.chan_monitor.added_monitors.lock().unwrap();
 			assert_eq!(added_monitors.len(), 1);

src/ln/msgs.rs

@@ -212,6 +212,7 @@ pub struct AcceptChannel {
 }
 
 /// A funding_created message to be sent or received from a peer
+#[derive(Clone)]
 pub struct FundingCreated {
 	pub(crate) temporary_channel_id: [u8; 32],
 	pub(crate) funding_txid: Sha256dHash,
@@ -220,6 +221,7 @@ pub struct FundingCreated {
 }
 
 /// A funding_signed message to be sent or received from a peer
+#[derive(Clone)]
 pub struct FundingSigned {
 	pub(crate) channel_id: [u8; 32],
 	pub(crate) signature: Signature,
@@ -530,7 +532,7 @@ pub trait ChannelMessageHandler : events::MessageSendEventsProvider + Send + Syn
 	/// Handle an incoming accept_channel message from the given peer.
 	fn handle_accept_channel(&self, their_node_id: &PublicKey, msg: &AcceptChannel) -> Result<(), HandleError>;
 	/// Handle an incoming funding_created message from the given peer.
-	fn handle_funding_created(&self, their_node_id: &PublicKey, msg: &FundingCreated) -> Result<FundingSigned, HandleError>;
+	fn handle_funding_created(&self, their_node_id: &PublicKey, msg: &FundingCreated) -> Result<(), HandleError>;
 	/// 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.

src/ln/peer_handler.rs

@@ -568,8 +568,7 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 
 											34 => {
 												let msg = try_potential_decodeerror!(msgs::FundingCreated::read(&mut reader));
-												let resp = try_potential_handleerror!(self.message_handler.chan_handler.handle_funding_created(&peer.their_node_id.unwrap(), &msg));
-												encode_and_send_msg!(resp, 35);
+												try_potential_handleerror!(self.message_handler.chan_handler.handle_funding_created(&peer.their_node_id.unwrap(), &msg));
 											},
 											35 => {
 												let msg = try_potential_decodeerror!(msgs::FundingSigned::read(&mut reader));
@@ -818,6 +817,17 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 						peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(msg, 34)));
 						Self::do_attempt_write_data(&mut descriptor, peer);
 					},
+					MessageSendEvent::SendFundingSigned { ref node_id, ref msg } => {
+						log_trace!(self, "Handling SendFundingSigned 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, 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 {}",
 								log_pubkey!(node_id),

src/util/events.rs

@@ -114,6 +114,13 @@ pub enum MessageSendEvent {
 		/// The message which should be sent.
 		msg: msgs::FundingCreated,
 	},
+	/// Used to indicate that a funding_signed message should be sent to the peer with the given node_id.
+	SendFundingSigned {
+		/// The node_id of the node which should receive this message
+		node_id: PublicKey,
+		/// The message which should be sent.
+		msg: msgs::FundingSigned,
+	},
 	/// Used to indicate that a funding_locked message should be sent to the peer with the given node_id.
 	SendFundingLocked {
 		/// The node_id of the node which should receive these message(s)

src/util/test_utils.rs

@@ -92,7 +92,7 @@ impl msgs::ChannelMessageHandler for TestChannelMessageHandler {
 	fn handle_accept_channel(&self, _their_node_id: &PublicKey, _msg: &msgs::AcceptChannel) -> Result<(), HandleError> {
 		Err(HandleError { err: "", action: None })
 	}
-	fn handle_funding_created(&self, _their_node_id: &PublicKey, _msg: &msgs::FundingCreated) -> Result<msgs::FundingSigned, HandleError> {
+	fn handle_funding_created(&self, _their_node_id: &PublicKey, _msg: &msgs::FundingCreated) -> Result<(), HandleError> {
 		Err(HandleError { err: "", action: None })
 	}
 	fn handle_funding_signed(&self, _their_node_id: &PublicKey, _msg: &msgs::FundingSigned) -> Result<(), HandleError> {