Fix reconnect message order on remote updates while waiting on RAA

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};
@@ -296,6 +296,12 @@ pub(super) struct Channel {
 	cur_local_commitment_transaction_number: u64,
 	cur_remote_commitment_transaction_number: u64,
 	value_to_self_msat: u64, // Excluding all pending_htlcs, excluding fees
+	/// Upon receipt of a channel_reestablish we have to figure out whether to send a
+	/// revoke_and_ack first or a commitment update first. Generally, we prefer to send
+	/// revoke_and_ack first, but if we had a pending commitment update of our own waiting on a
+	/// remote revoke when we received the latest commitment update from the remote we have to make
+	/// sure that commitment update gets resent first.
+	received_commitment_while_awaiting_raa: bool,
 	pending_inbound_htlcs: Vec<InboundHTLCOutput>,
 	pending_outbound_htlcs: Vec<OutboundHTLCOutput>,
 	holding_cell_htlc_updates: Vec<HTLCUpdateAwaitingACK>,
@@ -492,6 +498,8 @@ impl Channel {
  cur_local_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  cur_remote_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  value_to_self_msat: channel_value_satoshis * 1000 - push_msat,
+ received_commitment_while_awaiting_raa: false,
+
  pending_inbound_htlcs: Vec::new(),
  pending_outbound_htlcs: Vec::new(),
  holding_cell_htlc_updates: Vec::new(),
@@ -647,6 +655,8 @@ impl Channel {
  cur_local_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  cur_remote_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  value_to_self_msat: msg.push_msat,
+ received_commitment_while_awaiting_raa: false,
+
  pending_inbound_htlcs: Vec::new(),
  pending_outbound_htlcs: Vec::new(),
  holding_cell_htlc_updates: Vec::new(),
@@ -1696,6 +1706,7 @@ impl Channel {
 
  self.cur_local_commitment_transaction_number -= 1;
  self.last_local_commitment_txn = new_local_commitment_txn;
+ self.received_commitment_while_awaiting_raa = (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) != 0;
 
  let (our_commitment_signed, monitor_update) = if need_our_commitment && (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == 0 {
  // If we're AwaitingRemoteRevoke we can't send a new commitment here, but that's ok -
@@ -1831,6 +1842,7 @@ impl Channel {
  self.their_prev_commitment_point = self.their_cur_commitment_point;
  self.their_cur_commitment_point = Some(msg.next_per_commitment_point);
  self.cur_remote_commitment_transaction_number -= 1;
+ self.received_commitment_while_awaiting_raa = false;
 
  let mut to_forward_infos = Vec::new();
  let mut revoked_htlcs = Vec::new();
@@ -2072,7 +2084,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 +2132,12 @@ impl Channel {
  })
  } else { None };
 
+ let order = if self.received_commitment_while_awaiting_raa {
+ RAACommitmentOrder::CommitmentFirst
+ } else {
+ 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 +2160,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 +2224,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, CLTV_CLAIM_BUFFER, HTLC_FAIL_TIMEOUT_BLOCKS};
 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};
@@ -2168,17 +2168,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))
  }
@@ -2448,7 +2448,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)
 	}
 
@@ -4938,6 +4938,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 {
@@ -4985,6 +4986,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 {
@@ -5316,6 +5318,141 @@ mod tests {
  claim_payment(&nodes[0], &[&nodes[1]], payment_preimage);
 	}
 
+	#[test]
+	fn test_drop_messages_peer_disconnect_dual_htlc() {
+ // Test that we can handle reconnecting when both sides of a channel have pending
+ // commitment_updates when we disconnect.
+ let mut nodes = create_network(2);
+ create_announced_chan_between_nodes(&nodes, 0, 1);
+
+ let (payment_preimage_1, _) = route_payment(&nodes[0], &[&nodes[1]], 1000000);
+
+ // Now try to send a second payment which will fail to send
+ let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
+ let (payment_preimage_2, payment_hash_2) = get_payment_preimage_hash!(nodes[0]);
+
+ nodes[0].node.send_payment(route.clone(), payment_hash_2).unwrap();
+ check_added_monitors!(nodes[0], 1);
+
+ let events_1 = nodes[0].node.get_and_clear_pending_events();
+ assert_eq!(events_1.len(), 1);
+ match events_1[0] {
+ Event::UpdateHTLCs { .. } => {},
+ _ => panic!("Unexpected event"),
+ }
+
+ assert!(nodes[1].node.claim_funds(payment_preimage_1));
+ check_added_monitors!(nodes[1], 1);
+
+ let events_2 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_2.len(), 1);
+ match events_2[0] {
+ Event::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 } } => {
+ assert_eq!(*node_id, nodes[0].node.get_our_node_id());
+ assert!(update_add_htlcs.is_empty());
+ assert_eq!(update_fulfill_htlcs.len(), 1);
+ assert!(update_fail_htlcs.is_empty());
+ assert!(update_fail_malformed_htlcs.is_empty());
+ assert!(update_fee.is_none());
+
+ nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &update_fulfill_htlcs[0]).unwrap();
+ let events_3 = nodes[0].node.get_and_clear_pending_events();
+ assert_eq!(events_3.len(), 1);
+ match events_3[0] {
+ Event::PaymentSent { ref payment_preimage } => {
+ assert_eq!(*payment_preimage, payment_preimage_1);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ let (_, commitment_update) = nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), commitment_signed).unwrap();
+ assert!(commitment_update.is_none());
+ check_added_monitors!(nodes[0], 1);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
+ nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
+
+ let reestablish_1 = nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
+ assert_eq!(reestablish_1.len(), 1);
+ let reestablish_2 = nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
+ assert_eq!(reestablish_2.len(), 1);
+
+ let as_resp = nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &reestablish_2[0]).unwrap();
+ let bs_resp = nodes[1].node.handle_channel_reestablish(&nodes[0].node.get_our_node_id(), &reestablish_1[0]).unwrap();
+
+ assert!(as_resp.0.is_none());
+ assert!(bs_resp.0.is_none());
+
+ assert!(bs_resp.1.is_none());
+ assert!(bs_resp.2.is_none());
+
+ assert!(as_resp.3 == msgs::RAACommitmentOrder::CommitmentFirst);
+
+ assert_eq!(as_resp.2.as_ref().unwrap().update_add_htlcs.len(), 1);
+ assert!(as_resp.2.as_ref().unwrap().update_fulfill_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fail_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fail_malformed_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fee.is_none());
+ nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &as_resp.2.as_ref().unwrap().update_add_htlcs[0]).unwrap();
+ let (bs_revoke_and_ack, bs_commitment_signed) = nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_resp.2.as_ref().unwrap().commitment_signed).unwrap();
+ assert!(bs_commitment_signed.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let bs_second_commitment_signed = nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), as_resp.1.as_ref().unwrap()).unwrap().unwrap();
+ assert!(bs_second_commitment_signed.update_add_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fulfill_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fail_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fail_malformed_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fee.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let as_commitment_signed = nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_revoke_and_ack).unwrap().unwrap();
+ assert!(as_commitment_signed.update_add_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fulfill_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fail_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fail_malformed_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fee.is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ let (as_revoke_and_ack, as_second_commitment_signed) = nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_second_commitment_signed.commitment_signed).unwrap();
+ assert!(as_second_commitment_signed.is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ let (bs_second_revoke_and_ack, bs_third_commitment_signed) = nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_commitment_signed.commitment_signed).unwrap();
+ assert!(bs_third_commitment_signed.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ assert!(nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_revoke_and_ack).unwrap().is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let events_4 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_4.len(), 1);
+ match events_4[0] {
+ Event::PendingHTLCsForwardable { .. } => { },
+ _ => panic!("Unexpected event"),
+ };
+
+ nodes[1].node.channel_state.lock().unwrap().next_forward = Instant::now();
+ nodes[1].node.process_pending_htlc_forwards();
+
+ let events_5 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_5.len(), 1);
+ match events_5[0] {
+ Event::PaymentReceived { ref payment_hash, amt: _ } => {
+ assert_eq!(payment_hash_2, *payment_hash);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ assert!(nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_second_revoke_and_ack).unwrap().is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
+	}
+
 	#[test]
 	fn test_invalid_channel_announcement() {
  //Test BOLT 7 channel_announcement msg requirement for final node, gather data to build customed channel_announcement msgs

src/ln/msgs.rs

@@ -500,6 +500,18 @@ 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 runtime (eg handle_channel_reestablish needs to re-send messages in the order they
+/// were originally sent). 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
@@ -554,7 +566,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/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};
@@ -296,6 +296,12 @@ pub(super) struct Channel {
 	cur_local_commitment_transaction_number: u64,
 	cur_remote_commitment_transaction_number: u64,
 	value_to_self_msat: u64, // Excluding all pending_htlcs, excluding fees
+	/// Upon receipt of a channel_reestablish we have to figure out whether to send a
+	/// revoke_and_ack first or a commitment update first. Generally, we prefer to send
+	/// revoke_and_ack first, but if we had a pending commitment update of our own waiting on a
+	/// remote revoke when we received the latest commitment update from the remote we have to make
+	/// sure that commitment update gets resent first.
+	received_commitment_while_awaiting_raa: bool,
 	pending_inbound_htlcs: Vec<InboundHTLCOutput>,
 	pending_outbound_htlcs: Vec<OutboundHTLCOutput>,
 	holding_cell_htlc_updates: Vec<HTLCUpdateAwaitingACK>,
@@ -492,6 +498,8 @@ impl Channel {
  cur_local_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  cur_remote_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  value_to_self_msat: channel_value_satoshis * 1000 - push_msat,
+ received_commitment_while_awaiting_raa: false,
+
  pending_inbound_htlcs: Vec::new(),
  pending_outbound_htlcs: Vec::new(),
  holding_cell_htlc_updates: Vec::new(),
@@ -647,6 +655,8 @@ impl Channel {
  cur_local_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  cur_remote_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  value_to_self_msat: msg.push_msat,
+ received_commitment_while_awaiting_raa: false,
+
  pending_inbound_htlcs: Vec::new(),
  pending_outbound_htlcs: Vec::new(),
  holding_cell_htlc_updates: Vec::new(),
@@ -1696,6 +1706,7 @@ impl Channel {
 
  self.cur_local_commitment_transaction_number -= 1;
  self.last_local_commitment_txn = new_local_commitment_txn;
+ self.received_commitment_while_awaiting_raa = (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) != 0;
 
  let (our_commitment_signed, monitor_update) = if need_our_commitment && (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == 0 {
  // If we're AwaitingRemoteRevoke we can't send a new commitment here, but that's ok -
@@ -1831,6 +1842,7 @@ impl Channel {
  self.their_prev_commitment_point = self.their_cur_commitment_point;
  self.their_cur_commitment_point = Some(msg.next_per_commitment_point);
  self.cur_remote_commitment_transaction_number -= 1;
+ self.received_commitment_while_awaiting_raa = false;
 
  let mut to_forward_infos = Vec::new();
  let mut revoked_htlcs = Vec::new();
@@ -2072,7 +2084,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 +2132,12 @@ impl Channel {
  })
  } else { None };
 
+ let order = if self.received_commitment_while_awaiting_raa {
+ RAACommitmentOrder::CommitmentFirst
+ } else {
+ 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 +2160,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 +2224,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, CLTV_CLAIM_BUFFER, HTLC_FAIL_TIMEOUT_BLOCKS};
 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};
@@ -2168,17 +2168,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))
  }
@@ -2448,7 +2448,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)
 	}
 
@@ -4938,6 +4938,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 {
@@ -4985,6 +4986,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 {
@@ -5316,6 +5318,141 @@ mod tests {
  claim_payment(&nodes[0], &[&nodes[1]], payment_preimage);
 	}
 
+	#[test]
+	fn test_drop_messages_peer_disconnect_dual_htlc() {
+ // Test that we can handle reconnecting when both sides of a channel have pending
+ // commitment_updates when we disconnect.
+ let mut nodes = create_network(2);
+ create_announced_chan_between_nodes(&nodes, 0, 1);
+
+ let (payment_preimage_1, _) = route_payment(&nodes[0], &[&nodes[1]], 1000000);
+
+ // Now try to send a second payment which will fail to send
+ let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
+ let (payment_preimage_2, payment_hash_2) = get_payment_preimage_hash!(nodes[0]);
+
+ nodes[0].node.send_payment(route.clone(), payment_hash_2).unwrap();
+ check_added_monitors!(nodes[0], 1);
+
+ let events_1 = nodes[0].node.get_and_clear_pending_events();
+ assert_eq!(events_1.len(), 1);
+ match events_1[0] {
+ Event::UpdateHTLCs { .. } => {},
+ _ => panic!("Unexpected event"),
+ }
+
+ assert!(nodes[1].node.claim_funds(payment_preimage_1));
+ check_added_monitors!(nodes[1], 1);
+
+ let events_2 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_2.len(), 1);
+ match events_2[0] {
+ Event::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 } } => {
+ assert_eq!(*node_id, nodes[0].node.get_our_node_id());
+ assert!(update_add_htlcs.is_empty());
+ assert_eq!(update_fulfill_htlcs.len(), 1);
+ assert!(update_fail_htlcs.is_empty());
+ assert!(update_fail_malformed_htlcs.is_empty());
+ assert!(update_fee.is_none());
+
+ nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &update_fulfill_htlcs[0]).unwrap();
+ let events_3 = nodes[0].node.get_and_clear_pending_events();
+ assert_eq!(events_3.len(), 1);
+ match events_3[0] {
+ Event::PaymentSent { ref payment_preimage } => {
+ assert_eq!(*payment_preimage, payment_preimage_1);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ let (_, commitment_update) = nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), commitment_signed).unwrap();
+ assert!(commitment_update.is_none());
+ check_added_monitors!(nodes[0], 1);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
+ nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
+
+ let reestablish_1 = nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
+ assert_eq!(reestablish_1.len(), 1);
+ let reestablish_2 = nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
+ assert_eq!(reestablish_2.len(), 1);
+
+ let as_resp = nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &reestablish_2[0]).unwrap();
+ let bs_resp = nodes[1].node.handle_channel_reestablish(&nodes[0].node.get_our_node_id(), &reestablish_1[0]).unwrap();
+
+ assert!(as_resp.0.is_none());
+ assert!(bs_resp.0.is_none());
+
+ assert!(bs_resp.1.is_none());
+ assert!(bs_resp.2.is_none());
+
+ assert!(as_resp.3 == msgs::RAACommitmentOrder::CommitmentFirst);
+
+ assert_eq!(as_resp.2.as_ref().unwrap().update_add_htlcs.len(), 1);
+ assert!(as_resp.2.as_ref().unwrap().update_fulfill_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fail_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fail_malformed_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fee.is_none());
+ nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &as_resp.2.as_ref().unwrap().update_add_htlcs[0]).unwrap();
+ let (bs_revoke_and_ack, bs_commitment_signed) = nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_resp.2.as_ref().unwrap().commitment_signed).unwrap();
+ assert!(bs_commitment_signed.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let bs_second_commitment_signed = nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), as_resp.1.as_ref().unwrap()).unwrap().unwrap();
+ assert!(bs_second_commitment_signed.update_add_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fulfill_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fail_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fail_malformed_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fee.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let as_commitment_signed = nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_revoke_and_ack).unwrap().unwrap();
+ assert!(as_commitment_signed.update_add_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fulfill_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fail_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fail_malformed_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fee.is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ let (as_revoke_and_ack, as_second_commitment_signed) = nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_second_commitment_signed.commitment_signed).unwrap();
+ assert!(as_second_commitment_signed.is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ let (bs_second_revoke_and_ack, bs_third_commitment_signed) = nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_commitment_signed.commitment_signed).unwrap();
+ assert!(bs_third_commitment_signed.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ assert!(nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_revoke_and_ack).unwrap().is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let events_4 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_4.len(), 1);
+ match events_4[0] {
+ Event::PendingHTLCsForwardable { .. } => { },
+ _ => panic!("Unexpected event"),
+ };
+
+ nodes[1].node.channel_state.lock().unwrap().next_forward = Instant::now();
+ nodes[1].node.process_pending_htlc_forwards();
+
+ let events_5 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_5.len(), 1);
+ match events_5[0] {
+ Event::PaymentReceived { ref payment_hash, amt: _ } => {
+ assert_eq!(payment_hash_2, *payment_hash);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ assert!(nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_second_revoke_and_ack).unwrap().is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
+	}
+
 	#[test]
 	fn test_invalid_channel_announcement() {
  //Test BOLT 7 channel_announcement msg requirement for final node, gather data to build customed channel_announcement msgs

src/ln/msgs.rs

@@ -500,6 +500,18 @@ 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 runtime (eg handle_channel_reestablish needs to re-send messages in the order they
+/// were originally sent). 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
@@ -554,7 +566,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/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};
@@ -296,6 +296,12 @@ pub(super) struct Channel {
 	cur_local_commitment_transaction_number: u64,
 	cur_remote_commitment_transaction_number: u64,
 	value_to_self_msat: u64, // Excluding all pending_htlcs, excluding fees
+	/// Upon receipt of a channel_reestablish we have to figure out whether to send a
+	/// revoke_and_ack first or a commitment update first. Generally, we prefer to send
+	/// revoke_and_ack first, but if we had a pending commitment update of our own waiting on a
+	/// remote revoke when we received the latest commitment update from the remote we have to make
+	/// sure that commitment update gets resent first.
+	received_commitment_while_awaiting_raa: bool,
 	pending_inbound_htlcs: Vec<InboundHTLCOutput>,
 	pending_outbound_htlcs: Vec<OutboundHTLCOutput>,
 	holding_cell_htlc_updates: Vec<HTLCUpdateAwaitingACK>,
@@ -492,6 +498,8 @@ impl Channel {
  cur_local_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  cur_remote_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  value_to_self_msat: channel_value_satoshis * 1000 - push_msat,
+ received_commitment_while_awaiting_raa: false,
+
  pending_inbound_htlcs: Vec::new(),
  pending_outbound_htlcs: Vec::new(),
  holding_cell_htlc_updates: Vec::new(),
@@ -647,6 +655,8 @@ impl Channel {
  cur_local_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  cur_remote_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  value_to_self_msat: msg.push_msat,
+ received_commitment_while_awaiting_raa: false,
+
  pending_inbound_htlcs: Vec::new(),
  pending_outbound_htlcs: Vec::new(),
  holding_cell_htlc_updates: Vec::new(),
@@ -1696,6 +1706,7 @@ impl Channel {
 
  self.cur_local_commitment_transaction_number -= 1;
  self.last_local_commitment_txn = new_local_commitment_txn;
+ self.received_commitment_while_awaiting_raa = (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) != 0;
 
  let (our_commitment_signed, monitor_update) = if need_our_commitment && (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == 0 {
  // If we're AwaitingRemoteRevoke we can't send a new commitment here, but that's ok -
@@ -1831,6 +1842,7 @@ impl Channel {
  self.their_prev_commitment_point = self.their_cur_commitment_point;
  self.their_cur_commitment_point = Some(msg.next_per_commitment_point);
  self.cur_remote_commitment_transaction_number -= 1;
+ self.received_commitment_while_awaiting_raa = false;
 
  let mut to_forward_infos = Vec::new();
  let mut revoked_htlcs = Vec::new();
@@ -2072,7 +2084,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 +2132,12 @@ impl Channel {
  })
  } else { None };
 
+ let order = if self.received_commitment_while_awaiting_raa {
+ RAACommitmentOrder::CommitmentFirst
+ } else {
+ 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 +2160,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 +2224,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, CLTV_CLAIM_BUFFER, HTLC_FAIL_TIMEOUT_BLOCKS};
 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};
@@ -2168,17 +2168,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))
  }
@@ -2448,7 +2448,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)
 	}
 
@@ -4938,6 +4938,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 {
@@ -4985,6 +4986,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 {
@@ -5316,6 +5318,141 @@ mod tests {
  claim_payment(&nodes[0], &[&nodes[1]], payment_preimage);
 	}
 
+	#[test]
+	fn test_drop_messages_peer_disconnect_dual_htlc() {
+ // Test that we can handle reconnecting when both sides of a channel have pending
+ // commitment_updates when we disconnect.
+ let mut nodes = create_network(2);
+ create_announced_chan_between_nodes(&nodes, 0, 1);
+
+ let (payment_preimage_1, _) = route_payment(&nodes[0], &[&nodes[1]], 1000000);
+
+ // Now try to send a second payment which will fail to send
+ let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
+ let (payment_preimage_2, payment_hash_2) = get_payment_preimage_hash!(nodes[0]);
+
+ nodes[0].node.send_payment(route.clone(), payment_hash_2).unwrap();
+ check_added_monitors!(nodes[0], 1);
+
+ let events_1 = nodes[0].node.get_and_clear_pending_events();
+ assert_eq!(events_1.len(), 1);
+ match events_1[0] {
+ Event::UpdateHTLCs { .. } => {},
+ _ => panic!("Unexpected event"),
+ }
+
+ assert!(nodes[1].node.claim_funds(payment_preimage_1));
+ check_added_monitors!(nodes[1], 1);
+
+ let events_2 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_2.len(), 1);
+ match events_2[0] {
+ Event::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 } } => {
+ assert_eq!(*node_id, nodes[0].node.get_our_node_id());
+ assert!(update_add_htlcs.is_empty());
+ assert_eq!(update_fulfill_htlcs.len(), 1);
+ assert!(update_fail_htlcs.is_empty());
+ assert!(update_fail_malformed_htlcs.is_empty());
+ assert!(update_fee.is_none());
+
+ nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &update_fulfill_htlcs[0]).unwrap();
+ let events_3 = nodes[0].node.get_and_clear_pending_events();
+ assert_eq!(events_3.len(), 1);
+ match events_3[0] {
+ Event::PaymentSent { ref payment_preimage } => {
+ assert_eq!(*payment_preimage, payment_preimage_1);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ let (_, commitment_update) = nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), commitment_signed).unwrap();
+ assert!(commitment_update.is_none());
+ check_added_monitors!(nodes[0], 1);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
+ nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
+
+ let reestablish_1 = nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
+ assert_eq!(reestablish_1.len(), 1);
+ let reestablish_2 = nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
+ assert_eq!(reestablish_2.len(), 1);
+
+ let as_resp = nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &reestablish_2[0]).unwrap();
+ let bs_resp = nodes[1].node.handle_channel_reestablish(&nodes[0].node.get_our_node_id(), &reestablish_1[0]).unwrap();
+
+ assert!(as_resp.0.is_none());
+ assert!(bs_resp.0.is_none());
+
+ assert!(bs_resp.1.is_none());
+ assert!(bs_resp.2.is_none());
+
+ assert!(as_resp.3 == msgs::RAACommitmentOrder::CommitmentFirst);
+
+ assert_eq!(as_resp.2.as_ref().unwrap().update_add_htlcs.len(), 1);
+ assert!(as_resp.2.as_ref().unwrap().update_fulfill_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fail_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fail_malformed_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fee.is_none());
+ nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &as_resp.2.as_ref().unwrap().update_add_htlcs[0]).unwrap();
+ let (bs_revoke_and_ack, bs_commitment_signed) = nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_resp.2.as_ref().unwrap().commitment_signed).unwrap();
+ assert!(bs_commitment_signed.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let bs_second_commitment_signed = nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), as_resp.1.as_ref().unwrap()).unwrap().unwrap();
+ assert!(bs_second_commitment_signed.update_add_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fulfill_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fail_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fail_malformed_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fee.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let as_commitment_signed = nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_revoke_and_ack).unwrap().unwrap();
+ assert!(as_commitment_signed.update_add_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fulfill_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fail_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fail_malformed_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fee.is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ let (as_revoke_and_ack, as_second_commitment_signed) = nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_second_commitment_signed.commitment_signed).unwrap();
+ assert!(as_second_commitment_signed.is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ let (bs_second_revoke_and_ack, bs_third_commitment_signed) = nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_commitment_signed.commitment_signed).unwrap();
+ assert!(bs_third_commitment_signed.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ assert!(nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_revoke_and_ack).unwrap().is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let events_4 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_4.len(), 1);
+ match events_4[0] {
+ Event::PendingHTLCsForwardable { .. } => { },
+ _ => panic!("Unexpected event"),
+ };
+
+ nodes[1].node.channel_state.lock().unwrap().next_forward = Instant::now();
+ nodes[1].node.process_pending_htlc_forwards();
+
+ let events_5 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_5.len(), 1);
+ match events_5[0] {
+ Event::PaymentReceived { ref payment_hash, amt: _ } => {
+ assert_eq!(payment_hash_2, *payment_hash);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ assert!(nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_second_revoke_and_ack).unwrap().is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
+	}
+
 	#[test]
 	fn test_invalid_channel_announcement() {
  //Test BOLT 7 channel_announcement msg requirement for final node, gather data to build customed channel_announcement msgs

src/ln/msgs.rs

@@ -500,6 +500,18 @@ 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 runtime (eg handle_channel_reestablish needs to re-send messages in the order they
+/// were originally sent). 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
@@ -554,7 +566,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/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};
@@ -296,6 +296,12 @@ pub(super) struct Channel {
 	cur_local_commitment_transaction_number: u64,
 	cur_remote_commitment_transaction_number: u64,
 	value_to_self_msat: u64, // Excluding all pending_htlcs, excluding fees
+	/// Upon receipt of a channel_reestablish we have to figure out whether to send a
+	/// revoke_and_ack first or a commitment update first. Generally, we prefer to send
+	/// revoke_and_ack first, but if we had a pending commitment update of our own waiting on a
+	/// remote revoke when we received the latest commitment update from the remote we have to make
+	/// sure that commitment update gets resent first.
+	received_commitment_while_awaiting_raa: bool,
 	pending_inbound_htlcs: Vec<InboundHTLCOutput>,
 	pending_outbound_htlcs: Vec<OutboundHTLCOutput>,
 	holding_cell_htlc_updates: Vec<HTLCUpdateAwaitingACK>,
@@ -492,6 +498,8 @@ impl Channel {
  cur_local_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  cur_remote_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  value_to_self_msat: channel_value_satoshis * 1000 - push_msat,
+ received_commitment_while_awaiting_raa: false,
+
  pending_inbound_htlcs: Vec::new(),
  pending_outbound_htlcs: Vec::new(),
  holding_cell_htlc_updates: Vec::new(),
@@ -647,6 +655,8 @@ impl Channel {
  cur_local_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  cur_remote_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  value_to_self_msat: msg.push_msat,
+ received_commitment_while_awaiting_raa: false,
+
  pending_inbound_htlcs: Vec::new(),
  pending_outbound_htlcs: Vec::new(),
  holding_cell_htlc_updates: Vec::new(),
@@ -1696,6 +1706,7 @@ impl Channel {
 
  self.cur_local_commitment_transaction_number -= 1;
  self.last_local_commitment_txn = new_local_commitment_txn;
+ self.received_commitment_while_awaiting_raa = (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) != 0;
 
  let (our_commitment_signed, monitor_update) = if need_our_commitment && (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == 0 {
  // If we're AwaitingRemoteRevoke we can't send a new commitment here, but that's ok -
@@ -1831,6 +1842,7 @@ impl Channel {
  self.their_prev_commitment_point = self.their_cur_commitment_point;
  self.their_cur_commitment_point = Some(msg.next_per_commitment_point);
  self.cur_remote_commitment_transaction_number -= 1;
+ self.received_commitment_while_awaiting_raa = false;
 
  let mut to_forward_infos = Vec::new();
  let mut revoked_htlcs = Vec::new();
@@ -2072,7 +2084,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 +2132,12 @@ impl Channel {
  })
  } else { None };
 
+ let order = if self.received_commitment_while_awaiting_raa {
+ RAACommitmentOrder::CommitmentFirst
+ } else {
+ 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 +2160,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 +2224,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, CLTV_CLAIM_BUFFER, HTLC_FAIL_TIMEOUT_BLOCKS};
 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};
@@ -2168,17 +2168,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))
  }
@@ -2448,7 +2448,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)
 	}
 
@@ -4938,6 +4938,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 {
@@ -4985,6 +4986,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 {
@@ -5316,6 +5318,141 @@ mod tests {
  claim_payment(&nodes[0], &[&nodes[1]], payment_preimage);
 	}
 
+	#[test]
+	fn test_drop_messages_peer_disconnect_dual_htlc() {
+ // Test that we can handle reconnecting when both sides of a channel have pending
+ // commitment_updates when we disconnect.
+ let mut nodes = create_network(2);
+ create_announced_chan_between_nodes(&nodes, 0, 1);
+
+ let (payment_preimage_1, _) = route_payment(&nodes[0], &[&nodes[1]], 1000000);
+
+ // Now try to send a second payment which will fail to send
+ let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
+ let (payment_preimage_2, payment_hash_2) = get_payment_preimage_hash!(nodes[0]);
+
+ nodes[0].node.send_payment(route.clone(), payment_hash_2).unwrap();
+ check_added_monitors!(nodes[0], 1);
+
+ let events_1 = nodes[0].node.get_and_clear_pending_events();
+ assert_eq!(events_1.len(), 1);
+ match events_1[0] {
+ Event::UpdateHTLCs { .. } => {},
+ _ => panic!("Unexpected event"),
+ }
+
+ assert!(nodes[1].node.claim_funds(payment_preimage_1));
+ check_added_monitors!(nodes[1], 1);
+
+ let events_2 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_2.len(), 1);
+ match events_2[0] {
+ Event::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 } } => {
+ assert_eq!(*node_id, nodes[0].node.get_our_node_id());
+ assert!(update_add_htlcs.is_empty());
+ assert_eq!(update_fulfill_htlcs.len(), 1);
+ assert!(update_fail_htlcs.is_empty());
+ assert!(update_fail_malformed_htlcs.is_empty());
+ assert!(update_fee.is_none());
+
+ nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &update_fulfill_htlcs[0]).unwrap();
+ let events_3 = nodes[0].node.get_and_clear_pending_events();
+ assert_eq!(events_3.len(), 1);
+ match events_3[0] {
+ Event::PaymentSent { ref payment_preimage } => {
+ assert_eq!(*payment_preimage, payment_preimage_1);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ let (_, commitment_update) = nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), commitment_signed).unwrap();
+ assert!(commitment_update.is_none());
+ check_added_monitors!(nodes[0], 1);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
+ nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
+
+ let reestablish_1 = nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
+ assert_eq!(reestablish_1.len(), 1);
+ let reestablish_2 = nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
+ assert_eq!(reestablish_2.len(), 1);
+
+ let as_resp = nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &reestablish_2[0]).unwrap();
+ let bs_resp = nodes[1].node.handle_channel_reestablish(&nodes[0].node.get_our_node_id(), &reestablish_1[0]).unwrap();
+
+ assert!(as_resp.0.is_none());
+ assert!(bs_resp.0.is_none());
+
+ assert!(bs_resp.1.is_none());
+ assert!(bs_resp.2.is_none());
+
+ assert!(as_resp.3 == msgs::RAACommitmentOrder::CommitmentFirst);
+
+ assert_eq!(as_resp.2.as_ref().unwrap().update_add_htlcs.len(), 1);
+ assert!(as_resp.2.as_ref().unwrap().update_fulfill_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fail_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fail_malformed_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fee.is_none());
+ nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &as_resp.2.as_ref().unwrap().update_add_htlcs[0]).unwrap();
+ let (bs_revoke_and_ack, bs_commitment_signed) = nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_resp.2.as_ref().unwrap().commitment_signed).unwrap();
+ assert!(bs_commitment_signed.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let bs_second_commitment_signed = nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), as_resp.1.as_ref().unwrap()).unwrap().unwrap();
+ assert!(bs_second_commitment_signed.update_add_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fulfill_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fail_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fail_malformed_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fee.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let as_commitment_signed = nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_revoke_and_ack).unwrap().unwrap();
+ assert!(as_commitment_signed.update_add_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fulfill_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fail_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fail_malformed_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fee.is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ let (as_revoke_and_ack, as_second_commitment_signed) = nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_second_commitment_signed.commitment_signed).unwrap();
+ assert!(as_second_commitment_signed.is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ let (bs_second_revoke_and_ack, bs_third_commitment_signed) = nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_commitment_signed.commitment_signed).unwrap();
+ assert!(bs_third_commitment_signed.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ assert!(nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_revoke_and_ack).unwrap().is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let events_4 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_4.len(), 1);
+ match events_4[0] {
+ Event::PendingHTLCsForwardable { .. } => { },
+ _ => panic!("Unexpected event"),
+ };
+
+ nodes[1].node.channel_state.lock().unwrap().next_forward = Instant::now();
+ nodes[1].node.process_pending_htlc_forwards();
+
+ let events_5 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_5.len(), 1);
+ match events_5[0] {
+ Event::PaymentReceived { ref payment_hash, amt: _ } => {
+ assert_eq!(payment_hash_2, *payment_hash);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ assert!(nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_second_revoke_and_ack).unwrap().is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
+	}
+
 	#[test]
 	fn test_invalid_channel_announcement() {
  //Test BOLT 7 channel_announcement msg requirement for final node, gather data to build customed channel_announcement msgs

src/ln/msgs.rs

@@ -500,6 +500,18 @@ 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 runtime (eg handle_channel_reestablish needs to re-send messages in the order they
+/// were originally sent). 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
@@ -554,7 +566,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/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};
@@ -296,6 +296,12 @@ pub(super) struct Channel {
 	cur_local_commitment_transaction_number: u64,
 	cur_remote_commitment_transaction_number: u64,
 	value_to_self_msat: u64, // Excluding all pending_htlcs, excluding fees
+	/// Upon receipt of a channel_reestablish we have to figure out whether to send a
+	/// revoke_and_ack first or a commitment update first. Generally, we prefer to send
+	/// revoke_and_ack first, but if we had a pending commitment update of our own waiting on a
+	/// remote revoke when we received the latest commitment update from the remote we have to make
+	/// sure that commitment update gets resent first.
+	received_commitment_while_awaiting_raa: bool,
 	pending_inbound_htlcs: Vec<InboundHTLCOutput>,
 	pending_outbound_htlcs: Vec<OutboundHTLCOutput>,
 	holding_cell_htlc_updates: Vec<HTLCUpdateAwaitingACK>,
@@ -492,6 +498,8 @@ impl Channel {
  cur_local_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  cur_remote_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  value_to_self_msat: channel_value_satoshis * 1000 - push_msat,
+ received_commitment_while_awaiting_raa: false,
+
  pending_inbound_htlcs: Vec::new(),
  pending_outbound_htlcs: Vec::new(),
  holding_cell_htlc_updates: Vec::new(),
@@ -647,6 +655,8 @@ impl Channel {
  cur_local_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  cur_remote_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  value_to_self_msat: msg.push_msat,
+ received_commitment_while_awaiting_raa: false,
+
  pending_inbound_htlcs: Vec::new(),
  pending_outbound_htlcs: Vec::new(),
  holding_cell_htlc_updates: Vec::new(),
@@ -1696,6 +1706,7 @@ impl Channel {
 
  self.cur_local_commitment_transaction_number -= 1;
  self.last_local_commitment_txn = new_local_commitment_txn;
+ self.received_commitment_while_awaiting_raa = (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) != 0;
 
  let (our_commitment_signed, monitor_update) = if need_our_commitment && (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == 0 {
  // If we're AwaitingRemoteRevoke we can't send a new commitment here, but that's ok -
@@ -1831,6 +1842,7 @@ impl Channel {
  self.their_prev_commitment_point = self.their_cur_commitment_point;
  self.their_cur_commitment_point = Some(msg.next_per_commitment_point);
  self.cur_remote_commitment_transaction_number -= 1;
+ self.received_commitment_while_awaiting_raa = false;
 
  let mut to_forward_infos = Vec::new();
  let mut revoked_htlcs = Vec::new();
@@ -2072,7 +2084,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 +2132,12 @@ impl Channel {
  })
  } else { None };
 
+ let order = if self.received_commitment_while_awaiting_raa {
+ RAACommitmentOrder::CommitmentFirst
+ } else {
+ 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 +2160,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 +2224,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, CLTV_CLAIM_BUFFER, HTLC_FAIL_TIMEOUT_BLOCKS};
 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};
@@ -2168,17 +2168,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))
  }
@@ -2448,7 +2448,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)
 	}
 
@@ -4938,6 +4938,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 {
@@ -4985,6 +4986,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 {
@@ -5316,6 +5318,141 @@ mod tests {
  claim_payment(&nodes[0], &[&nodes[1]], payment_preimage);
 	}
 
+	#[test]
+	fn test_drop_messages_peer_disconnect_dual_htlc() {
+ // Test that we can handle reconnecting when both sides of a channel have pending
+ // commitment_updates when we disconnect.
+ let mut nodes = create_network(2);
+ create_announced_chan_between_nodes(&nodes, 0, 1);
+
+ let (payment_preimage_1, _) = route_payment(&nodes[0], &[&nodes[1]], 1000000);
+
+ // Now try to send a second payment which will fail to send
+ let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
+ let (payment_preimage_2, payment_hash_2) = get_payment_preimage_hash!(nodes[0]);
+
+ nodes[0].node.send_payment(route.clone(), payment_hash_2).unwrap();
+ check_added_monitors!(nodes[0], 1);
+
+ let events_1 = nodes[0].node.get_and_clear_pending_events();
+ assert_eq!(events_1.len(), 1);
+ match events_1[0] {
+ Event::UpdateHTLCs { .. } => {},
+ _ => panic!("Unexpected event"),
+ }
+
+ assert!(nodes[1].node.claim_funds(payment_preimage_1));
+ check_added_monitors!(nodes[1], 1);
+
+ let events_2 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_2.len(), 1);
+ match events_2[0] {
+ Event::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 } } => {
+ assert_eq!(*node_id, nodes[0].node.get_our_node_id());
+ assert!(update_add_htlcs.is_empty());
+ assert_eq!(update_fulfill_htlcs.len(), 1);
+ assert!(update_fail_htlcs.is_empty());
+ assert!(update_fail_malformed_htlcs.is_empty());
+ assert!(update_fee.is_none());
+
+ nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &update_fulfill_htlcs[0]).unwrap();
+ let events_3 = nodes[0].node.get_and_clear_pending_events();
+ assert_eq!(events_3.len(), 1);
+ match events_3[0] {
+ Event::PaymentSent { ref payment_preimage } => {
+ assert_eq!(*payment_preimage, payment_preimage_1);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ let (_, commitment_update) = nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), commitment_signed).unwrap();
+ assert!(commitment_update.is_none());
+ check_added_monitors!(nodes[0], 1);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
+ nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
+
+ let reestablish_1 = nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
+ assert_eq!(reestablish_1.len(), 1);
+ let reestablish_2 = nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
+ assert_eq!(reestablish_2.len(), 1);
+
+ let as_resp = nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &reestablish_2[0]).unwrap();
+ let bs_resp = nodes[1].node.handle_channel_reestablish(&nodes[0].node.get_our_node_id(), &reestablish_1[0]).unwrap();
+
+ assert!(as_resp.0.is_none());
+ assert!(bs_resp.0.is_none());
+
+ assert!(bs_resp.1.is_none());
+ assert!(bs_resp.2.is_none());
+
+ assert!(as_resp.3 == msgs::RAACommitmentOrder::CommitmentFirst);
+
+ assert_eq!(as_resp.2.as_ref().unwrap().update_add_htlcs.len(), 1);
+ assert!(as_resp.2.as_ref().unwrap().update_fulfill_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fail_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fail_malformed_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fee.is_none());
+ nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &as_resp.2.as_ref().unwrap().update_add_htlcs[0]).unwrap();
+ let (bs_revoke_and_ack, bs_commitment_signed) = nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_resp.2.as_ref().unwrap().commitment_signed).unwrap();
+ assert!(bs_commitment_signed.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let bs_second_commitment_signed = nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), as_resp.1.as_ref().unwrap()).unwrap().unwrap();
+ assert!(bs_second_commitment_signed.update_add_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fulfill_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fail_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fail_malformed_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fee.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let as_commitment_signed = nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_revoke_and_ack).unwrap().unwrap();
+ assert!(as_commitment_signed.update_add_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fulfill_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fail_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fail_malformed_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fee.is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ let (as_revoke_and_ack, as_second_commitment_signed) = nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_second_commitment_signed.commitment_signed).unwrap();
+ assert!(as_second_commitment_signed.is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ let (bs_second_revoke_and_ack, bs_third_commitment_signed) = nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_commitment_signed.commitment_signed).unwrap();
+ assert!(bs_third_commitment_signed.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ assert!(nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_revoke_and_ack).unwrap().is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let events_4 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_4.len(), 1);
+ match events_4[0] {
+ Event::PendingHTLCsForwardable { .. } => { },
+ _ => panic!("Unexpected event"),
+ };
+
+ nodes[1].node.channel_state.lock().unwrap().next_forward = Instant::now();
+ nodes[1].node.process_pending_htlc_forwards();
+
+ let events_5 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_5.len(), 1);
+ match events_5[0] {
+ Event::PaymentReceived { ref payment_hash, amt: _ } => {
+ assert_eq!(payment_hash_2, *payment_hash);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ assert!(nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_second_revoke_and_ack).unwrap().is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
+	}
+
 	#[test]
 	fn test_invalid_channel_announcement() {
  //Test BOLT 7 channel_announcement msg requirement for final node, gather data to build customed channel_announcement msgs

src/ln/msgs.rs

@@ -500,6 +500,18 @@ 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 runtime (eg handle_channel_reestablish needs to re-send messages in the order they
+/// were originally sent). 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
@@ -554,7 +566,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/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};
@@ -296,6 +296,12 @@ pub(super) struct Channel {
 	cur_local_commitment_transaction_number: u64,
 	cur_remote_commitment_transaction_number: u64,
 	value_to_self_msat: u64, // Excluding all pending_htlcs, excluding fees
+	/// Upon receipt of a channel_reestablish we have to figure out whether to send a
+	/// revoke_and_ack first or a commitment update first. Generally, we prefer to send
+	/// revoke_and_ack first, but if we had a pending commitment update of our own waiting on a
+	/// remote revoke when we received the latest commitment update from the remote we have to make
+	/// sure that commitment update gets resent first.
+	received_commitment_while_awaiting_raa: bool,
 	pending_inbound_htlcs: Vec<InboundHTLCOutput>,
 	pending_outbound_htlcs: Vec<OutboundHTLCOutput>,
 	holding_cell_htlc_updates: Vec<HTLCUpdateAwaitingACK>,
@@ -492,6 +498,8 @@ impl Channel {
  cur_local_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  cur_remote_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  value_to_self_msat: channel_value_satoshis * 1000 - push_msat,
+ received_commitment_while_awaiting_raa: false,
+
  pending_inbound_htlcs: Vec::new(),
  pending_outbound_htlcs: Vec::new(),
  holding_cell_htlc_updates: Vec::new(),
@@ -647,6 +655,8 @@ impl Channel {
  cur_local_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  cur_remote_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  value_to_self_msat: msg.push_msat,
+ received_commitment_while_awaiting_raa: false,
+
  pending_inbound_htlcs: Vec::new(),
  pending_outbound_htlcs: Vec::new(),
  holding_cell_htlc_updates: Vec::new(),
@@ -1696,6 +1706,7 @@ impl Channel {
 
  self.cur_local_commitment_transaction_number -= 1;
  self.last_local_commitment_txn = new_local_commitment_txn;
+ self.received_commitment_while_awaiting_raa = (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) != 0;
 
  let (our_commitment_signed, monitor_update) = if need_our_commitment && (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == 0 {
  // If we're AwaitingRemoteRevoke we can't send a new commitment here, but that's ok -
@@ -1831,6 +1842,7 @@ impl Channel {
  self.their_prev_commitment_point = self.their_cur_commitment_point;
  self.their_cur_commitment_point = Some(msg.next_per_commitment_point);
  self.cur_remote_commitment_transaction_number -= 1;
+ self.received_commitment_while_awaiting_raa = false;
 
  let mut to_forward_infos = Vec::new();
  let mut revoked_htlcs = Vec::new();
@@ -2072,7 +2084,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 +2132,12 @@ impl Channel {
  })
  } else { None };
 
+ let order = if self.received_commitment_while_awaiting_raa {
+ RAACommitmentOrder::CommitmentFirst
+ } else {
+ 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 +2160,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 +2224,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, CLTV_CLAIM_BUFFER, HTLC_FAIL_TIMEOUT_BLOCKS};
 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};
@@ -2168,17 +2168,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))
  }
@@ -2448,7 +2448,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)
 	}
 
@@ -4938,6 +4938,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 {
@@ -4985,6 +4986,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 {
@@ -5316,6 +5318,141 @@ mod tests {
  claim_payment(&nodes[0], &[&nodes[1]], payment_preimage);
 	}
 
+	#[test]
+	fn test_drop_messages_peer_disconnect_dual_htlc() {
+ // Test that we can handle reconnecting when both sides of a channel have pending
+ // commitment_updates when we disconnect.
+ let mut nodes = create_network(2);
+ create_announced_chan_between_nodes(&nodes, 0, 1);
+
+ let (payment_preimage_1, _) = route_payment(&nodes[0], &[&nodes[1]], 1000000);
+
+ // Now try to send a second payment which will fail to send
+ let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
+ let (payment_preimage_2, payment_hash_2) = get_payment_preimage_hash!(nodes[0]);
+
+ nodes[0].node.send_payment(route.clone(), payment_hash_2).unwrap();
+ check_added_monitors!(nodes[0], 1);
+
+ let events_1 = nodes[0].node.get_and_clear_pending_events();
+ assert_eq!(events_1.len(), 1);
+ match events_1[0] {
+ Event::UpdateHTLCs { .. } => {},
+ _ => panic!("Unexpected event"),
+ }
+
+ assert!(nodes[1].node.claim_funds(payment_preimage_1));
+ check_added_monitors!(nodes[1], 1);
+
+ let events_2 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_2.len(), 1);
+ match events_2[0] {
+ Event::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 } } => {
+ assert_eq!(*node_id, nodes[0].node.get_our_node_id());
+ assert!(update_add_htlcs.is_empty());
+ assert_eq!(update_fulfill_htlcs.len(), 1);
+ assert!(update_fail_htlcs.is_empty());
+ assert!(update_fail_malformed_htlcs.is_empty());
+ assert!(update_fee.is_none());
+
+ nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &update_fulfill_htlcs[0]).unwrap();
+ let events_3 = nodes[0].node.get_and_clear_pending_events();
+ assert_eq!(events_3.len(), 1);
+ match events_3[0] {
+ Event::PaymentSent { ref payment_preimage } => {
+ assert_eq!(*payment_preimage, payment_preimage_1);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ let (_, commitment_update) = nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), commitment_signed).unwrap();
+ assert!(commitment_update.is_none());
+ check_added_monitors!(nodes[0], 1);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
+ nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
+
+ let reestablish_1 = nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
+ assert_eq!(reestablish_1.len(), 1);
+ let reestablish_2 = nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
+ assert_eq!(reestablish_2.len(), 1);
+
+ let as_resp = nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &reestablish_2[0]).unwrap();
+ let bs_resp = nodes[1].node.handle_channel_reestablish(&nodes[0].node.get_our_node_id(), &reestablish_1[0]).unwrap();
+
+ assert!(as_resp.0.is_none());
+ assert!(bs_resp.0.is_none());
+
+ assert!(bs_resp.1.is_none());
+ assert!(bs_resp.2.is_none());
+
+ assert!(as_resp.3 == msgs::RAACommitmentOrder::CommitmentFirst);
+
+ assert_eq!(as_resp.2.as_ref().unwrap().update_add_htlcs.len(), 1);
+ assert!(as_resp.2.as_ref().unwrap().update_fulfill_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fail_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fail_malformed_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fee.is_none());
+ nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &as_resp.2.as_ref().unwrap().update_add_htlcs[0]).unwrap();
+ let (bs_revoke_and_ack, bs_commitment_signed) = nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_resp.2.as_ref().unwrap().commitment_signed).unwrap();
+ assert!(bs_commitment_signed.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let bs_second_commitment_signed = nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), as_resp.1.as_ref().unwrap()).unwrap().unwrap();
+ assert!(bs_second_commitment_signed.update_add_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fulfill_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fail_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fail_malformed_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fee.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let as_commitment_signed = nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_revoke_and_ack).unwrap().unwrap();
+ assert!(as_commitment_signed.update_add_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fulfill_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fail_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fail_malformed_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fee.is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ let (as_revoke_and_ack, as_second_commitment_signed) = nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_second_commitment_signed.commitment_signed).unwrap();
+ assert!(as_second_commitment_signed.is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ let (bs_second_revoke_and_ack, bs_third_commitment_signed) = nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_commitment_signed.commitment_signed).unwrap();
+ assert!(bs_third_commitment_signed.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ assert!(nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_revoke_and_ack).unwrap().is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let events_4 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_4.len(), 1);
+ match events_4[0] {
+ Event::PendingHTLCsForwardable { .. } => { },
+ _ => panic!("Unexpected event"),
+ };
+
+ nodes[1].node.channel_state.lock().unwrap().next_forward = Instant::now();
+ nodes[1].node.process_pending_htlc_forwards();
+
+ let events_5 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_5.len(), 1);
+ match events_5[0] {
+ Event::PaymentReceived { ref payment_hash, amt: _ } => {
+ assert_eq!(payment_hash_2, *payment_hash);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ assert!(nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_second_revoke_and_ack).unwrap().is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
+	}
+
 	#[test]
 	fn test_invalid_channel_announcement() {
  //Test BOLT 7 channel_announcement msg requirement for final node, gather data to build customed channel_announcement msgs

src/ln/msgs.rs

@@ -500,6 +500,18 @@ 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 runtime (eg handle_channel_reestablish needs to re-send messages in the order they
+/// were originally sent). 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
@@ -554,7 +566,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/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};
@@ -296,6 +296,12 @@ pub(super) struct Channel {
 	cur_local_commitment_transaction_number: u64,
 	cur_remote_commitment_transaction_number: u64,
 	value_to_self_msat: u64, // Excluding all pending_htlcs, excluding fees
+	/// Upon receipt of a channel_reestablish we have to figure out whether to send a
+	/// revoke_and_ack first or a commitment update first. Generally, we prefer to send
+	/// revoke_and_ack first, but if we had a pending commitment update of our own waiting on a
+	/// remote revoke when we received the latest commitment update from the remote we have to make
+	/// sure that commitment update gets resent first.
+	received_commitment_while_awaiting_raa: bool,
 	pending_inbound_htlcs: Vec<InboundHTLCOutput>,
 	pending_outbound_htlcs: Vec<OutboundHTLCOutput>,
 	holding_cell_htlc_updates: Vec<HTLCUpdateAwaitingACK>,
@@ -492,6 +498,8 @@ impl Channel {
  cur_local_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  cur_remote_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  value_to_self_msat: channel_value_satoshis * 1000 - push_msat,
+ received_commitment_while_awaiting_raa: false,
+
  pending_inbound_htlcs: Vec::new(),
  pending_outbound_htlcs: Vec::new(),
  holding_cell_htlc_updates: Vec::new(),
@@ -647,6 +655,8 @@ impl Channel {
  cur_local_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  cur_remote_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  value_to_self_msat: msg.push_msat,
+ received_commitment_while_awaiting_raa: false,
+
  pending_inbound_htlcs: Vec::new(),
  pending_outbound_htlcs: Vec::new(),
  holding_cell_htlc_updates: Vec::new(),
@@ -1696,6 +1706,7 @@ impl Channel {
 
  self.cur_local_commitment_transaction_number -= 1;
  self.last_local_commitment_txn = new_local_commitment_txn;
+ self.received_commitment_while_awaiting_raa = (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) != 0;
 
  let (our_commitment_signed, monitor_update) = if need_our_commitment && (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == 0 {
  // If we're AwaitingRemoteRevoke we can't send a new commitment here, but that's ok -
@@ -1831,6 +1842,7 @@ impl Channel {
  self.their_prev_commitment_point = self.their_cur_commitment_point;
  self.their_cur_commitment_point = Some(msg.next_per_commitment_point);
  self.cur_remote_commitment_transaction_number -= 1;
+ self.received_commitment_while_awaiting_raa = false;
 
  let mut to_forward_infos = Vec::new();
  let mut revoked_htlcs = Vec::new();
@@ -2072,7 +2084,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 +2132,12 @@ impl Channel {
  })
  } else { None };
 
+ let order = if self.received_commitment_while_awaiting_raa {
+ RAACommitmentOrder::CommitmentFirst
+ } else {
+ 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 +2160,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 +2224,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, CLTV_CLAIM_BUFFER, HTLC_FAIL_TIMEOUT_BLOCKS};
 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};
@@ -2168,17 +2168,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))
  }
@@ -2448,7 +2448,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)
 	}
 
@@ -4938,6 +4938,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 {
@@ -4985,6 +4986,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 {
@@ -5316,6 +5318,141 @@ mod tests {
  claim_payment(&nodes[0], &[&nodes[1]], payment_preimage);
 	}
 
+	#[test]
+	fn test_drop_messages_peer_disconnect_dual_htlc() {
+ // Test that we can handle reconnecting when both sides of a channel have pending
+ // commitment_updates when we disconnect.
+ let mut nodes = create_network(2);
+ create_announced_chan_between_nodes(&nodes, 0, 1);
+
+ let (payment_preimage_1, _) = route_payment(&nodes[0], &[&nodes[1]], 1000000);
+
+ // Now try to send a second payment which will fail to send
+ let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
+ let (payment_preimage_2, payment_hash_2) = get_payment_preimage_hash!(nodes[0]);
+
+ nodes[0].node.send_payment(route.clone(), payment_hash_2).unwrap();
+ check_added_monitors!(nodes[0], 1);
+
+ let events_1 = nodes[0].node.get_and_clear_pending_events();
+ assert_eq!(events_1.len(), 1);
+ match events_1[0] {
+ Event::UpdateHTLCs { .. } => {},
+ _ => panic!("Unexpected event"),
+ }
+
+ assert!(nodes[1].node.claim_funds(payment_preimage_1));
+ check_added_monitors!(nodes[1], 1);
+
+ let events_2 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_2.len(), 1);
+ match events_2[0] {
+ Event::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 } } => {
+ assert_eq!(*node_id, nodes[0].node.get_our_node_id());
+ assert!(update_add_htlcs.is_empty());
+ assert_eq!(update_fulfill_htlcs.len(), 1);
+ assert!(update_fail_htlcs.is_empty());
+ assert!(update_fail_malformed_htlcs.is_empty());
+ assert!(update_fee.is_none());
+
+ nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &update_fulfill_htlcs[0]).unwrap();
+ let events_3 = nodes[0].node.get_and_clear_pending_events();
+ assert_eq!(events_3.len(), 1);
+ match events_3[0] {
+ Event::PaymentSent { ref payment_preimage } => {
+ assert_eq!(*payment_preimage, payment_preimage_1);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ let (_, commitment_update) = nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), commitment_signed).unwrap();
+ assert!(commitment_update.is_none());
+ check_added_monitors!(nodes[0], 1);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
+ nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
+
+ let reestablish_1 = nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
+ assert_eq!(reestablish_1.len(), 1);
+ let reestablish_2 = nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
+ assert_eq!(reestablish_2.len(), 1);
+
+ let as_resp = nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &reestablish_2[0]).unwrap();
+ let bs_resp = nodes[1].node.handle_channel_reestablish(&nodes[0].node.get_our_node_id(), &reestablish_1[0]).unwrap();
+
+ assert!(as_resp.0.is_none());
+ assert!(bs_resp.0.is_none());
+
+ assert!(bs_resp.1.is_none());
+ assert!(bs_resp.2.is_none());
+
+ assert!(as_resp.3 == msgs::RAACommitmentOrder::CommitmentFirst);
+
+ assert_eq!(as_resp.2.as_ref().unwrap().update_add_htlcs.len(), 1);
+ assert!(as_resp.2.as_ref().unwrap().update_fulfill_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fail_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fail_malformed_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fee.is_none());
+ nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &as_resp.2.as_ref().unwrap().update_add_htlcs[0]).unwrap();
+ let (bs_revoke_and_ack, bs_commitment_signed) = nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_resp.2.as_ref().unwrap().commitment_signed).unwrap();
+ assert!(bs_commitment_signed.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let bs_second_commitment_signed = nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), as_resp.1.as_ref().unwrap()).unwrap().unwrap();
+ assert!(bs_second_commitment_signed.update_add_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fulfill_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fail_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fail_malformed_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fee.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let as_commitment_signed = nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_revoke_and_ack).unwrap().unwrap();
+ assert!(as_commitment_signed.update_add_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fulfill_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fail_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fail_malformed_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fee.is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ let (as_revoke_and_ack, as_second_commitment_signed) = nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_second_commitment_signed.commitment_signed).unwrap();
+ assert!(as_second_commitment_signed.is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ let (bs_second_revoke_and_ack, bs_third_commitment_signed) = nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_commitment_signed.commitment_signed).unwrap();
+ assert!(bs_third_commitment_signed.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ assert!(nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_revoke_and_ack).unwrap().is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let events_4 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_4.len(), 1);
+ match events_4[0] {
+ Event::PendingHTLCsForwardable { .. } => { },
+ _ => panic!("Unexpected event"),
+ };
+
+ nodes[1].node.channel_state.lock().unwrap().next_forward = Instant::now();
+ nodes[1].node.process_pending_htlc_forwards();
+
+ let events_5 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_5.len(), 1);
+ match events_5[0] {
+ Event::PaymentReceived { ref payment_hash, amt: _ } => {
+ assert_eq!(payment_hash_2, *payment_hash);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ assert!(nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_second_revoke_and_ack).unwrap().is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
+	}
+
 	#[test]
 	fn test_invalid_channel_announcement() {
  //Test BOLT 7 channel_announcement msg requirement for final node, gather data to build customed channel_announcement msgs

src/ln/msgs.rs

@@ -500,6 +500,18 @@ 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 runtime (eg handle_channel_reestablish needs to re-send messages in the order they
+/// were originally sent). 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
@@ -554,7 +566,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/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};
@@ -296,6 +296,12 @@ pub(super) struct Channel {
 	cur_local_commitment_transaction_number: u64,
 	cur_remote_commitment_transaction_number: u64,
 	value_to_self_msat: u64, // Excluding all pending_htlcs, excluding fees
+	/// Upon receipt of a channel_reestablish we have to figure out whether to send a
+	/// revoke_and_ack first or a commitment update first. Generally, we prefer to send
+	/// revoke_and_ack first, but if we had a pending commitment update of our own waiting on a
+	/// remote revoke when we received the latest commitment update from the remote we have to make
+	/// sure that commitment update gets resent first.
+	received_commitment_while_awaiting_raa: bool,
 	pending_inbound_htlcs: Vec<InboundHTLCOutput>,
 	pending_outbound_htlcs: Vec<OutboundHTLCOutput>,
 	holding_cell_htlc_updates: Vec<HTLCUpdateAwaitingACK>,
@@ -492,6 +498,8 @@ impl Channel {
  cur_local_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  cur_remote_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  value_to_self_msat: channel_value_satoshis * 1000 - push_msat,
+ received_commitment_while_awaiting_raa: false,
+
  pending_inbound_htlcs: Vec::new(),
  pending_outbound_htlcs: Vec::new(),
  holding_cell_htlc_updates: Vec::new(),
@@ -647,6 +655,8 @@ impl Channel {
  cur_local_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  cur_remote_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
  value_to_self_msat: msg.push_msat,
+ received_commitment_while_awaiting_raa: false,
+
  pending_inbound_htlcs: Vec::new(),
  pending_outbound_htlcs: Vec::new(),
  holding_cell_htlc_updates: Vec::new(),
@@ -1696,6 +1706,7 @@ impl Channel {
 
  self.cur_local_commitment_transaction_number -= 1;
  self.last_local_commitment_txn = new_local_commitment_txn;
+ self.received_commitment_while_awaiting_raa = (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) != 0;
 
  let (our_commitment_signed, monitor_update) = if need_our_commitment && (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == 0 {
  // If we're AwaitingRemoteRevoke we can't send a new commitment here, but that's ok -
@@ -1831,6 +1842,7 @@ impl Channel {
  self.their_prev_commitment_point = self.their_cur_commitment_point;
  self.their_cur_commitment_point = Some(msg.next_per_commitment_point);
  self.cur_remote_commitment_transaction_number -= 1;
+ self.received_commitment_while_awaiting_raa = false;
 
  let mut to_forward_infos = Vec::new();
  let mut revoked_htlcs = Vec::new();
@@ -2072,7 +2084,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 +2132,12 @@ impl Channel {
  })
  } else { None };
 
+ let order = if self.received_commitment_while_awaiting_raa {
+ RAACommitmentOrder::CommitmentFirst
+ } else {
+ 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 +2160,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 +2224,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, CLTV_CLAIM_BUFFER, HTLC_FAIL_TIMEOUT_BLOCKS};
 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};
@@ -2168,17 +2168,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))
  }
@@ -2448,7 +2448,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)
 	}
 
@@ -4938,6 +4938,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 {
@@ -4985,6 +4986,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 {
@@ -5316,6 +5318,141 @@ mod tests {
  claim_payment(&nodes[0], &[&nodes[1]], payment_preimage);
 	}
 
+	#[test]
+	fn test_drop_messages_peer_disconnect_dual_htlc() {
+ // Test that we can handle reconnecting when both sides of a channel have pending
+ // commitment_updates when we disconnect.
+ let mut nodes = create_network(2);
+ create_announced_chan_between_nodes(&nodes, 0, 1);
+
+ let (payment_preimage_1, _) = route_payment(&nodes[0], &[&nodes[1]], 1000000);
+
+ // Now try to send a second payment which will fail to send
+ let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
+ let (payment_preimage_2, payment_hash_2) = get_payment_preimage_hash!(nodes[0]);
+
+ nodes[0].node.send_payment(route.clone(), payment_hash_2).unwrap();
+ check_added_monitors!(nodes[0], 1);
+
+ let events_1 = nodes[0].node.get_and_clear_pending_events();
+ assert_eq!(events_1.len(), 1);
+ match events_1[0] {
+ Event::UpdateHTLCs { .. } => {},
+ _ => panic!("Unexpected event"),
+ }
+
+ assert!(nodes[1].node.claim_funds(payment_preimage_1));
+ check_added_monitors!(nodes[1], 1);
+
+ let events_2 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_2.len(), 1);
+ match events_2[0] {
+ Event::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 } } => {
+ assert_eq!(*node_id, nodes[0].node.get_our_node_id());
+ assert!(update_add_htlcs.is_empty());
+ assert_eq!(update_fulfill_htlcs.len(), 1);
+ assert!(update_fail_htlcs.is_empty());
+ assert!(update_fail_malformed_htlcs.is_empty());
+ assert!(update_fee.is_none());
+
+ nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &update_fulfill_htlcs[0]).unwrap();
+ let events_3 = nodes[0].node.get_and_clear_pending_events();
+ assert_eq!(events_3.len(), 1);
+ match events_3[0] {
+ Event::PaymentSent { ref payment_preimage } => {
+ assert_eq!(*payment_preimage, payment_preimage_1);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ let (_, commitment_update) = nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), commitment_signed).unwrap();
+ assert!(commitment_update.is_none());
+ check_added_monitors!(nodes[0], 1);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
+ nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
+
+ let reestablish_1 = nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
+ assert_eq!(reestablish_1.len(), 1);
+ let reestablish_2 = nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id());
+ assert_eq!(reestablish_2.len(), 1);
+
+ let as_resp = nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &reestablish_2[0]).unwrap();
+ let bs_resp = nodes[1].node.handle_channel_reestablish(&nodes[0].node.get_our_node_id(), &reestablish_1[0]).unwrap();
+
+ assert!(as_resp.0.is_none());
+ assert!(bs_resp.0.is_none());
+
+ assert!(bs_resp.1.is_none());
+ assert!(bs_resp.2.is_none());
+
+ assert!(as_resp.3 == msgs::RAACommitmentOrder::CommitmentFirst);
+
+ assert_eq!(as_resp.2.as_ref().unwrap().update_add_htlcs.len(), 1);
+ assert!(as_resp.2.as_ref().unwrap().update_fulfill_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fail_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fail_malformed_htlcs.is_empty());
+ assert!(as_resp.2.as_ref().unwrap().update_fee.is_none());
+ nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &as_resp.2.as_ref().unwrap().update_add_htlcs[0]).unwrap();
+ let (bs_revoke_and_ack, bs_commitment_signed) = nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_resp.2.as_ref().unwrap().commitment_signed).unwrap();
+ assert!(bs_commitment_signed.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let bs_second_commitment_signed = nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), as_resp.1.as_ref().unwrap()).unwrap().unwrap();
+ assert!(bs_second_commitment_signed.update_add_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fulfill_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fail_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fail_malformed_htlcs.is_empty());
+ assert!(bs_second_commitment_signed.update_fee.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let as_commitment_signed = nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_revoke_and_ack).unwrap().unwrap();
+ assert!(as_commitment_signed.update_add_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fulfill_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fail_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fail_malformed_htlcs.is_empty());
+ assert!(as_commitment_signed.update_fee.is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ let (as_revoke_and_ack, as_second_commitment_signed) = nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_second_commitment_signed.commitment_signed).unwrap();
+ assert!(as_second_commitment_signed.is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ let (bs_second_revoke_and_ack, bs_third_commitment_signed) = nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_commitment_signed.commitment_signed).unwrap();
+ assert!(bs_third_commitment_signed.is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ assert!(nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_revoke_and_ack).unwrap().is_none());
+ check_added_monitors!(nodes[1], 1);
+
+ let events_4 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_4.len(), 1);
+ match events_4[0] {
+ Event::PendingHTLCsForwardable { .. } => { },
+ _ => panic!("Unexpected event"),
+ };
+
+ nodes[1].node.channel_state.lock().unwrap().next_forward = Instant::now();
+ nodes[1].node.process_pending_htlc_forwards();
+
+ let events_5 = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events_5.len(), 1);
+ match events_5[0] {
+ Event::PaymentReceived { ref payment_hash, amt: _ } => {
+ assert_eq!(payment_hash_2, *payment_hash);
+ },
+ _ => panic!("Unexpected event"),
+ }
+
+ assert!(nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_second_revoke_and_ack).unwrap().is_none());
+ check_added_monitors!(nodes[0], 1);
+
+ claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
+	}
+
 	#[test]
 	fn test_invalid_channel_announcement() {
  //Test BOLT 7 channel_announcement msg requirement for final node, gather data to build customed channel_announcement msgs

src/ln/msgs.rs

@@ -500,6 +500,18 @@ 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 runtime (eg handle_channel_reestablish needs to re-send messages in the order they
+/// were originally sent). 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
@@ -554,7 +566,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.