Use new chan_restoration macros in channel_reestablish handling.

This merges the code for restoring channel functionality between
channel monitor updating restored and peer reconnection, reducing
redundant code.

Author: TheBlueMatt

lightning/src/ln/channelmanager.rs

@@ -786,20 +786,60 @@ macro_rules! maybe_break_monitor_err {
 }
 
 macro_rules! handle_chan_restoration_locked {
-	($self: expr, $channel_lock: expr, $channel_state: expr, $channel_entry: expr,
+	($self: ident, $channel_lock: expr, $channel_state: expr, $channel_entry: expr,
 	 $raa: expr, $commitment_update: expr, $order: expr,
+	 $pending_forwards: expr, $broadcast_safe: expr, $funding_locked: expr) => { {
+		let res = handle_chan_restoration_locked!($self, $channel_lock, $channel_state, $channel_entry,
+			$raa, $commitment_update, $order, None, $pending_forwards, $broadcast_safe, $funding_locked);
+		// If there was no ChannelMonitorUpdate, we should never generate an Err in the res loop
+		// below. Doing so would imply calling handle_err!() from channel_monitor_updated() which
+		// should *never* end up calling back to `chain_monitor.update_channel()`.
+		assert!(res.2.is_ok());
+		res
+	} };
+	($self: ident, $channel_lock: expr, $channel_state: expr, $channel_entry: expr,
+	 $raa: expr, $commitment_update: expr, $order: expr, $chanmon_update: expr,
 	 $pending_forwards: expr, $broadcast_safe: expr, $funding_locked: expr) => { {
 		let mut htlc_forwards = None;
 		let mut funding_broadcast_safe = None;
 		let counterparty_node_id = $channel_entry.get().get_counterparty_node_id();
 
-		{
-			if !$pending_forwards.is_empty() {
+		let res = loop {
+			let forwards: Vec<(PendingHTLCInfo, u64)> = $pending_forwards; // Force type-checking to resolve
+			if !forwards.is_empty() {
 				htlc_forwards = Some(($channel_entry.get().get_short_channel_id().expect("We can't have pending forwards before funding confirmation"),
-					$channel_entry.get().get_funding_txo().unwrap(), $pending_forwards));
+					$channel_entry.get().get_funding_txo().unwrap(), forwards));
+			}
+
+			let chanmon_update: Option<ChannelMonitorUpdate> = $chanmon_update; // Force type-checking to resolve
+			if chanmon_update.is_some() {
+				assert!($commitment_update.is_some());
+				assert!($funding_locked.is_none());
+			}
+
+			if let Some(msg) = $funding_locked {
+				$channel_state.pending_msg_events.push(events::MessageSendEvent::SendFundingLocked {
+					node_id: counterparty_node_id,
+					msg,
+				});
+				if let Some(announcement_sigs) = $self.get_announcement_sigs($channel_entry.get()) {
+					$channel_state.pending_msg_events.push(events::MessageSendEvent::SendAnnouncementSignatures {
+						node_id: counterparty_node_id,
+						msg: announcement_sigs,
+					});
+				}
+				$channel_state.short_to_id.insert($channel_entry.get().get_short_channel_id().unwrap(), $channel_entry.get().channel_id());
 			}
 
 			macro_rules! handle_cs { () => {
+				if let Some(monitor_update) = chanmon_update {
+					assert!($order == RAACommitmentOrder::RevokeAndACKFirst);
+					assert!(!$broadcast_safe);
+					assert!($commitment_update.is_some());
+					if let Err(e) = $self.chain_monitor.update_channel($channel_entry.get().get_funding_txo().unwrap(), monitor_update) {
+						break handle_monitor_err!($self, e, $channel_state, $channel_entry, RAACommitmentOrder::CommitmentFirst, false, true);
+					}
+				}
 				if let Some(update) = $commitment_update {
 					$channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
 						node_id: counterparty_node_id,
@@ -831,27 +871,18 @@ macro_rules! handle_chan_restoration_locked {
 					user_channel_id: $channel_entry.get().get_user_id(),
 				});
 			}
-			if let Some(msg) = $funding_locked {
-				$channel_state.pending_msg_events.push(events::MessageSendEvent::SendFundingLocked {
-					node_id: counterparty_node_id,
-					msg,
-				});
-				if let Some(announcement_sigs) = $self.get_announcement_sigs($channel_entry.get()) {
-					$channel_state.pending_msg_events.push(events::MessageSendEvent::SendAnnouncementSignatures {
-						node_id: counterparty_node_id,
-						msg: announcement_sigs,
-					});
-				}
-				$channel_state.short_to_id.insert($channel_entry.get().get_short_channel_id().unwrap(), $channel_entry.get().channel_id());
-			}
-		}
-		(htlc_forwards, funding_broadcast_safe)
+			break Ok(());
+		};
+
+		(htlc_forwards, funding_broadcast_safe, res, counterparty_node_id)
 	} }
 }
 
 macro_rules! post_handle_chan_restoration {
-	($self: expr, $locked_res: expr) => { {
-		let (htlc_forwards, funding_broadcast_safe) = $locked_res;
+	($self: ident, $locked_res: expr) => { {
+		let (htlc_forwards, funding_broadcast_safe, res, counterparty_node_id) = $locked_res;
+
+		let _ = handle_error!($self, res, counterparty_node_id);
 
 		if let Some(ev) = funding_broadcast_safe {
 			$self.pending_events.lock().unwrap().push(ev);
@@ -3045,77 +3076,34 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	}
 
 	fn internal_channel_reestablish(&self, counterparty_node_id: &PublicKey, msg: &msgs::ChannelReestablish) -> Result<(), MsgHandleErrInternal> {
-		let mut channel_state_lock = self.channel_state.lock().unwrap();
-		let channel_state = &mut *channel_state_lock;
+		let chan_restoration_res = {
+			let mut channel_state_lock = self.channel_state.lock().unwrap();
+			let channel_state = &mut *channel_state_lock;
 
-		match channel_state.by_id.entry(msg.channel_id) {
-			hash_map::Entry::Occupied(mut chan) => {
-				if chan.get().get_counterparty_node_id() != *counterparty_node_id {
-					return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!".to_owned(), msg.channel_id));
-				}
-				// Currently, we expect all holding cell update_adds to be dropped on peer
-				// disconnect, so Channel's reestablish will never hand us any holding cell
-				// freed HTLCs to fail backwards. If in the future we no longer drop pending
-				// add-HTLCs on disconnect, we may be handed HTLCs to fail backwards here.
-				let (funding_locked, revoke_and_ack, commitment_update, monitor_update_opt, mut order, shutdown) =
-					try_chan_entry!(self, chan.get_mut().channel_reestablish(msg, &self.logger), channel_state, chan);
-				if let Some(monitor_update) = monitor_update_opt {
-					if let Err(e) = self.chain_monitor.update_channel(chan.get().get_funding_txo().unwrap(), monitor_update) {
-						// channel_reestablish doesn't guarantee the order it returns is sensical
-						// for the messages it returns, but if we're setting what messages to
-						// re-transmit on monitor update success, we need to make sure it is sane.
-						if revoke_and_ack.is_none() {
-							order = RAACommitmentOrder::CommitmentFirst;
-						}
-						if commitment_update.is_none() {
-							order = RAACommitmentOrder::RevokeAndACKFirst;
-						}
-						return_monitor_err!(self, e, channel_state, chan, order, revoke_and_ack.is_some(), commitment_update.is_some());
-						//TODO: Resend the funding_locked if needed once we get the monitor running again
-					}
-				}
-				if let Some(msg) = funding_locked {
-					channel_state.pending_msg_events.push(events::MessageSendEvent::SendFundingLocked {
-						node_id: counterparty_node_id.clone(),
-						msg
-					});
-				}
-				macro_rules! send_raa { () => {
-					if let Some(msg) = revoke_and_ack {
-						channel_state.pending_msg_events.push(events::MessageSendEvent::SendRevokeAndACK {
-							node_id: counterparty_node_id.clone(),
-							msg
-						});
+			match channel_state.by_id.entry(msg.channel_id) {
+				hash_map::Entry::Occupied(mut chan) => {
+					if chan.get().get_counterparty_node_id() != *counterparty_node_id {
+						return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!".to_owned(), msg.channel_id));
 					}
-				} }
-				macro_rules! send_cu { () => {
-					if let Some(updates) = commitment_update {
-						channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
+					// Currently, we expect all holding cell update_adds to be dropped on peer
+					// disconnect, so Channel's reestablish will never hand us any holding cell
+					// freed HTLCs to fail backwards. If in the future we no longer drop pending
+					// add-HTLCs on disconnect, we may be handed HTLCs to fail backwards here.
+					let (funding_locked, revoke_and_ack, commitment_update, monitor_update_opt, order, shutdown) =
+						try_chan_entry!(self, chan.get_mut().channel_reestablish(msg, &self.logger), channel_state, chan);
+					if let Some(msg) = shutdown {
+						channel_state.pending_msg_events.push(events::MessageSendEvent::SendShutdown {
 							node_id: counterparty_node_id.clone(),
-							updates
+							msg,
 						});
 					}
-				} }
-				match order {
-					RAACommitmentOrder::RevokeAndACKFirst => {
-						send_raa!();
-						send_cu!();
-					},
-					RAACommitmentOrder::CommitmentFirst => {
-						send_cu!();
-						send_raa!();
-					},
-				}
-				if let Some(msg) = shutdown {
-					channel_state.pending_msg_events.push(events::MessageSendEvent::SendShutdown {
-						node_id: counterparty_node_id.clone(),
-						msg,
-					});
-				}
-				Ok(())
-			},
-			hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
-		}
+					handle_chan_restoration_locked!(self, channel_state_lock, channel_state, chan, revoke_and_ack, commitment_update, order, monitor_update_opt, Vec::new(), false, funding_locked)
+				},
+				hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
+			}
+		};
+		post_handle_chan_restoration!(self, chan_restoration_res);
+		Ok(())
 	}
 
 	/// Begin Update fee process. Allowed only on an outbound channel.

lightning/src/ln/functional_test_utils.rs

@@ -1362,6 +1362,11 @@ macro_rules! handle_chan_reestablish_msgs {
 				None
 			};
 
+			if let Some(&MessageSendEvent::SendAnnouncementSignatures { ref node_id, msg: _ }) = msg_events.get(idx) {
+				idx += 1;
+				assert_eq!(*node_id, $dst_node.node.get_our_node_id());
+			}
+
 			let mut revoke_and_ack = None;
 			let mut commitment_update = None;
 			let order = if let Some(ev) = msg_events.get(idx) {