Add message ordering return value to handling channel_reestablish

Author: TheBlueMatt

src/ln/channel.rs

@@ -14,7 +14,7 @@ use crypto::digest::Digest;
 use crypto::hkdf::{hkdf_extract,hkdf_expand};
 
 use ln::msgs;
-use ln::msgs::{ErrorAction, HandleError};
+use ln::msgs::{ErrorAction, HandleError, RAACommitmentOrder};
 use ln::channelmonitor::ChannelMonitor;
 use ln::channelmanager::{PendingHTLCStatus, HTLCSource, PendingForwardHTLCInfo, HTLCFailReason, HTLCFailureMsg};
 use ln::chan_utils::{TxCreationKeys,HTLCOutputInCommitment,HTLC_SUCCESS_TX_WEIGHT,HTLC_TIMEOUT_TX_WEIGHT};
@@ -2072,7 +2072,7 @@ impl Channel {
 
 	/// May panic if some calls other than message-handling calls (which will all Err immediately)
 	/// have been called between remove_uncommitted_htlcs_and_mark_paused and this call.
-	pub fn channel_reestablish(&mut self, msg: &msgs::ChannelReestablish) -> Result<(Option<msgs::FundingLocked>, Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, Option<ChannelMonitor>), ChannelError> {
+	pub fn channel_reestablish(&mut self, msg: &msgs::ChannelReestablish) -> Result<(Option<msgs::FundingLocked>, Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, Option<ChannelMonitor>, RAACommitmentOrder), ChannelError> {
 		if self.channel_state & (ChannelState::PeerDisconnected as u32) == 0 {
 			// While BOLT 2 doesn't indicate explicitly we should error this channel here, it
 			// almost certainly indicates we are going to end up out-of-sync in some way, so we
@@ -2120,6 +2120,8 @@ impl Channel {
 			})
 		} else { None };
 
+		let order = RAACommitmentOrder::RevokeAndACKFirst;
+
 		if msg.next_local_commitment_number == our_next_remote_commitment_number {
 			if required_revoke.is_some() {
 				log_debug!(self, "Reconnected channel {} with only lost outbound RAA", log_bytes!(self.channel_id()));
@@ -2142,11 +2144,11 @@ impl Channel {
 							panic!("Got non-channel-failing result from free_holding_cell_htlcs");
 						}
 					},
-					Ok(Some((commitment_update, channel_monitor))) => return Ok((resend_funding_locked, required_revoke, Some(commitment_update), Some(channel_monitor))),
-					Ok(None) => return Ok((resend_funding_locked, required_revoke, None, None)),
+					Ok(Some((commitment_update, channel_monitor))) => return Ok((resend_funding_locked, required_revoke, Some(commitment_update), Some(channel_monitor), order)),
+					Ok(None) => return Ok((resend_funding_locked, required_revoke, None, None, order)),
 				}
 			} else {
-				return Ok((resend_funding_locked, required_revoke, None, None));
+				return Ok((resend_funding_locked, required_revoke, None, None, order));
 			}
 		} else if msg.next_local_commitment_number == our_next_remote_commitment_number - 1 {
 			if required_revoke.is_some() {
@@ -2206,7 +2208,7 @@ impl Channel {
 						update_add_htlcs, update_fulfill_htlcs, update_fail_htlcs, update_fail_malformed_htlcs,
 						update_fee: None, //TODO: We need to support re-generating any update_fees in the last commitment_signed!
 						commitment_signed: self.send_commitment_no_state_update().expect("It looks like we failed to re-generate a commitment_signed we had previously sent?").0,
-					}), None));
+					}), None, order));
 		} else {
 			return Err(ChannelError::Close("Peer attempted to reestablish channel with a very old remote commitment transaction"));
 		}

src/ln/channelmanager.rs

@@ -26,7 +26,7 @@ use ln::channel::{Channel, ChannelError, ChannelKeys};
 use ln::channelmonitor::ManyChannelMonitor;
 use ln::router::{Route,RouteHop};
 use ln::msgs;
-use ln::msgs::{HandleError,ChannelMessageHandler};
+use ln::msgs::{ChannelMessageHandler, HandleError, RAACommitmentOrder};
 use util::{byte_utils, events, internal_traits, rng};
 use util::sha2::Sha256;
 use util::ser::{Readable, Writeable};
@@ -1967,17 +1967,17 @@ impl ChannelManager {
 		Ok(())
 	}
 
-	fn internal_channel_reestablish(&self, their_node_id: &PublicKey, msg: &msgs::ChannelReestablish) -> Result<(Option<msgs::FundingLocked>, Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>), MsgHandleErrInternal> {
+	fn internal_channel_reestablish(&self, their_node_id: &PublicKey, msg: &msgs::ChannelReestablish) -> Result<(Option<msgs::FundingLocked>, Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, RAACommitmentOrder), MsgHandleErrInternal> {
 		let (res, chan_monitor) = {
 			let mut channel_state = self.channel_state.lock().unwrap();
 			match channel_state.by_id.get_mut(&msg.channel_id) {
 				Some(chan) => {
 					if chan.get_their_node_id() != *their_node_id {
 						return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!", msg.channel_id));
 					}
-					let (funding_locked, revoke_and_ack, commitment_update, channel_monitor) = chan.channel_reestablish(msg)
+					let (funding_locked, revoke_and_ack, commitment_update, channel_monitor, order) = chan.channel_reestablish(msg)
 						.map_err(|e| MsgHandleErrInternal::from_chan_maybe_close(e, msg.channel_id))?;
-					(Ok((funding_locked, revoke_and_ack, commitment_update)), channel_monitor)
+					(Ok((funding_locked, revoke_and_ack, commitment_update, order)), channel_monitor)
 				},
 				None => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel", msg.channel_id))
 			}
@@ -2247,7 +2247,7 @@ impl ChannelMessageHandler for ChannelManager {
 		handle_error!(self, self.internal_announcement_signatures(their_node_id, msg), their_node_id)
 	}
 
-	fn handle_channel_reestablish(&self, their_node_id: &PublicKey, msg: &msgs::ChannelReestablish) -> Result<(Option<msgs::FundingLocked>, Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>), HandleError> {
+	fn handle_channel_reestablish(&self, their_node_id: &PublicKey, msg: &msgs::ChannelReestablish) -> Result<(Option<msgs::FundingLocked>, Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, RAACommitmentOrder), HandleError> {
 		handle_error!(self, self.internal_channel_reestablish(their_node_id, msg), their_node_id)
 	}
 
@@ -4725,6 +4725,7 @@ mod tests {
 				assert!(chan_msgs.0.is_none());
 			}
 			if pending_raa.0 {
+				assert!(chan_msgs.3 == msgs::RAACommitmentOrder::RevokeAndACKFirst);
 				assert!(node_a.node.handle_revoke_and_ack(&node_b.node.get_our_node_id(), &chan_msgs.1.unwrap()).unwrap().is_none());
 				check_added_monitors!(node_a, 1);
 			} else {
@@ -4772,6 +4773,7 @@ mod tests {
 				assert!(chan_msgs.0.is_none());
 			}
 			if pending_raa.1 {
+				assert!(chan_msgs.3 == msgs::RAACommitmentOrder::RevokeAndACKFirst);
 				assert!(node_b.node.handle_revoke_and_ack(&node_a.node.get_our_node_id(), &chan_msgs.1.unwrap()).unwrap().is_none());
 				check_added_monitors!(node_b, 1);
 			} else {

src/ln/msgs.rs

@@ -488,6 +488,17 @@ pub enum HTLCFailChannelUpdate {
 	},
 }
 
+/// For events which result in both a RevokeAndACK and a CommitmentUpdate, by default they should
+/// be sent in the order they appear in the return value, however sometimes the order needs to be
+/// variable at rumtime. In those cases, this enum is also returned.
+#[derive(Clone, PartialEq)]
+pub enum RAACommitmentOrder {
+	/// Send the CommitmentUpdate messages first
+	CommitmentFirst,
+	/// Send the RevokeAndACK message first
+	RevokeAndACKFirst,
+}
+
 /// A trait to describe an object which can receive channel messages.
 ///
 /// Messages MAY be called in parallel when they originate from different their_node_ids, however
@@ -542,7 +553,7 @@ pub trait ChannelMessageHandler : events::EventsProvider + Send + Sync {
 	/// Handle a peer reconnecting, possibly generating channel_reestablish message(s).
 	fn peer_connected(&self, their_node_id: &PublicKey) -> Vec<ChannelReestablish>;
 	/// Handle an incoming channel_reestablish message from the given peer.
-	fn handle_channel_reestablish(&self, their_node_id: &PublicKey, msg: &ChannelReestablish) -> Result<(Option<FundingLocked>, Option<RevokeAndACK>, Option<CommitmentUpdate>), HandleError>;
+	fn handle_channel_reestablish(&self, their_node_id: &PublicKey, msg: &ChannelReestablish) -> Result<(Option<FundingLocked>, Option<RevokeAndACK>, Option<CommitmentUpdate>, RAACommitmentOrder), HandleError>;
 
 	// Error:
 	/// Handle an incoming error message from the given peer.

src/ln/peer_handler.rs

@@ -661,30 +661,44 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 											},
 											136 => {
 												let msg = try_potential_decodeerror!(msgs::ChannelReestablish::read(&mut reader));
-												let (funding_locked, revoke_and_ack, commitment_update) = try_potential_handleerror!(self.message_handler.chan_handler.handle_channel_reestablish(&peer.their_node_id.unwrap(), &msg));
+												let (funding_locked, revoke_and_ack, commitment_update, order) = try_potential_handleerror!(self.message_handler.chan_handler.handle_channel_reestablish(&peer.their_node_id.unwrap(), &msg));
 												if let Some(lock_msg) = funding_locked {
 													encode_and_send_msg!(lock_msg, 36);
 												}
-												if let Some(revoke_msg) = revoke_and_ack {
-													encode_and_send_msg!(revoke_msg, 133);
-												}
-												match commitment_update {
-													Some(resps) => {
-														for resp in resps.update_add_htlcs {
-															encode_and_send_msg!(resp, 128);
-														}
-														for resp in resps.update_fulfill_htlcs {
-															encode_and_send_msg!(resp, 130);
-														}
-														for resp in resps.update_fail_htlcs {
-															encode_and_send_msg!(resp, 131);
-														}
-														if let Some(resp) = resps.update_fee {
-															encode_and_send_msg!(resp, 134);
-														}
-														encode_and_send_msg!(resps.commitment_signed, 132);
+												macro_rules! handle_raa { () => {
+													if let Some(revoke_msg) = revoke_and_ack {
+														encode_and_send_msg!(revoke_msg, 133);
+													}
+												} }
+												macro_rules! handle_cu { () => {
+													match commitment_update {
+														Some(resps) => {
+															for resp in resps.update_add_htlcs {
+																encode_and_send_msg!(resp, 128);
+															}
+															for resp in resps.update_fulfill_htlcs {
+																encode_and_send_msg!(resp, 130);
+															}
+															for resp in resps.update_fail_htlcs {
+																encode_and_send_msg!(resp, 131);
+															}
+															if let Some(resp) = resps.update_fee {
+																encode_and_send_msg!(resp, 134);
+															}
+															encode_and_send_msg!(resps.commitment_signed, 132);
+														},
+														None => {},
+													}
+												} }
+												match order {
+													msgs::RAACommitmentOrder::RevokeAndACKFirst => {
+														handle_raa!();
+														handle_cu!();
+													},
+													msgs::RAACommitmentOrder::CommitmentFirst => {
+														handle_cu!();
+														handle_raa!();
 													},
-													None => {},
 												}
 											},
 

src/util/test_utils.rs

@@ -128,7 +128,7 @@ impl msgs::ChannelMessageHandler for TestChannelMessageHandler {
 	fn handle_announcement_signatures(&self, _their_node_id: &PublicKey, _msg: &msgs::AnnouncementSignatures) -> Result<(), HandleError> {
 		Err(HandleError { err: "", action: None })
 	}
-	fn handle_channel_reestablish(&self, _their_node_id: &PublicKey, _msg: &msgs::ChannelReestablish) -> Result<(Option<msgs::FundingLocked>, Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>), HandleError> {
+	fn handle_channel_reestablish(&self, _their_node_id: &PublicKey, _msg: &msgs::ChannelReestablish) -> Result<(Option<msgs::FundingLocked>, Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, msgs::RAACommitmentOrder), HandleError> {
 		Err(HandleError { err: "", action: None })
 	}
 	fn peer_disconnected(&self, _their_node_id: &PublicKey, _no_connection_possible: bool) {}