Fix channel_reestablish generation/handling around next_remote.

Author: TheBlueMatt

src/ln/channel.rs

@@ -2289,7 +2289,8 @@ impl Channel {
 			return Err(ChannelError::Close("Peer sent a loose channel_reestablish not after reconnect"));
 		}
 
-		if msg.next_local_commitment_number >= INITIAL_COMMITMENT_NUMBER || msg.next_remote_commitment_number >= INITIAL_COMMITMENT_NUMBER {
+		if msg.next_local_commitment_number >= INITIAL_COMMITMENT_NUMBER || msg.next_remote_commitment_number >= INITIAL_COMMITMENT_NUMBER ||
+			msg.next_local_commitment_number == 0 {
 			return Err(ChannelError::Close("Peer sent a garbage channel_reestablish"));
 		}
 
@@ -2304,15 +2305,15 @@ impl Channel {
 			})
 		} else { None };
 
-		if self.channel_state & (ChannelState::FundingSent as u32 | ChannelState::OurFundingLocked as u32) == ChannelState::FundingSent as u32 {
-			// Short circuit the whole handler as there is nothing we can resend them
-			return Ok((None, None, None, None, RAACommitmentOrder::CommitmentFirst, shutdown_msg));
-		}
-
-		if msg.next_local_commitment_number == 0 || msg.next_remote_commitment_number == 0 {
-			if self.channel_state & (ChannelState::FundingSent as u32) != ChannelState::FundingSent as u32 {
-				return Err(ChannelError::Close("Peer sent a pre-funding channel_reestablish after we exchanged funding_locked"));
+		if self.channel_state & (ChannelState::FundingSent as u32) == ChannelState::FundingSent as u32 {
+			if self.channel_state & ChannelState::OurFundingLocked as u32 == 0 {
+				if msg.next_remote_commitment_number != 0 {
+					return Err(ChannelError::Close("Peer claimed they saw a revoke_and_ack but we haven't sent funding_locked yet"));
+				}
+				// Short circuit the whole handler as there is nothing we can resend them
+				return Ok((None, None, None, None, RAACommitmentOrder::CommitmentFirst, shutdown_msg));
 			}
+
 			// We have OurFundingLocked set!
 			let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
 			let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
@@ -2322,11 +2323,11 @@ impl Channel {
 			}), None, None, None, RAACommitmentOrder::CommitmentFirst, shutdown_msg));
 		}
 
-		let required_revoke = if msg.next_remote_commitment_number == INITIAL_COMMITMENT_NUMBER - self.cur_local_commitment_transaction_number {
+		let required_revoke = if msg.next_remote_commitment_number + 1 == INITIAL_COMMITMENT_NUMBER - self.cur_local_commitment_transaction_number {
 			// Remote isn't waiting on any RevokeAndACK from us!
 			// Note that if we need to repeat our FundingLocked we'll do that in the next if block.
 			None
-		} else if msg.next_remote_commitment_number == (INITIAL_COMMITMENT_NUMBER - 1) - self.cur_local_commitment_transaction_number {
+		} else if msg.next_remote_commitment_number + 1 == (INITIAL_COMMITMENT_NUMBER - 1) - self.cur_local_commitment_transaction_number {
 			if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) != 0 {
 				self.monitor_pending_revoke_and_ack = true;
 				None
@@ -3053,11 +3054,27 @@ impl Channel {
 	/// self.remove_uncommitted_htlcs_and_mark_paused()'d
 	pub fn get_channel_reestablish(&self) -> msgs::ChannelReestablish {
 		assert_eq!(self.channel_state & ChannelState::PeerDisconnected as u32, ChannelState::PeerDisconnected as u32);
+		assert_ne!(self.cur_remote_commitment_transaction_number, INITIAL_COMMITMENT_NUMBER);
 		msgs::ChannelReestablish {
 			channel_id: self.channel_id(),
+			// The protocol has two different commitment number concepts - the "commitment
+			// transaction number", which starts from 0 and counts up, and the "revocation key
+			// index" which starts at INITIAL_COMMITMENT_NUMBER and counts down. We track
+			// commitment transaction numbers by the index which will be used to reveal the
+			// revocation key for that commitment transaction, which means we have to convert them
+			// to protocol-level commitment numbers here...
+
+			// next_local_commitment_number is the next commitment_signed number we expect to
+			// receive (indicating if they need to resend one that we missed).
 			next_local_commitment_number: INITIAL_COMMITMENT_NUMBER - self.cur_local_commitment_transaction_number,
-			next_remote_commitment_number: INITIAL_COMMITMENT_NUMBER - self.cur_remote_commitment_transaction_number -
-				if self.channel_state & (ChannelState::FundingSent as u32 | ChannelState::OurFundingLocked as u32) == (ChannelState::FundingSent as u32) { 1 } else { 0 },
+			// We have to set next_remote_commitment_number to the next revoke_and_ack we expect to
+			// receive, however we track it by the next commitment number for a remote transaction
+			// (which is one further, as they always revoke previous commitment transaction, not
+			// the one we send) so we have to decrement by 1. Note that if
+			// cur_remote_commitment_transaction_number is INITIAL_COMMITMENT_NUMBER we will have
+			// dropped this channel on disconnect as it hasn't yet reached FundingSent so we can't
+			// overflow here.
+			next_remote_commitment_number: INITIAL_COMMITMENT_NUMBER - self.cur_remote_commitment_transaction_number - 1,
 			data_loss_protect: None,
 		}
 	}

src/ln/channelmanager.rs

@@ -6315,6 +6315,31 @@ mod tests {
 		node_b.node.peer_connected(&node_a.node.get_our_node_id());
 		let reestablish_2 = get_chan_reestablish_msgs!(node_b, node_a);
 
+		if send_funding_locked.0 {
+			// If a expects a funding_locked, it better not think it has received a revoke_and_ack
+			// from b
+			for reestablish in reestablish_1.iter() {
+				assert_eq!(reestablish.next_remote_commitment_number, 0);
+			}
+		}
+		if send_funding_locked.1 {
+			// If b expects a funding_locked, it better not think it has received a revoke_and_ack
+			// from a
+			for reestablish in reestablish_2.iter() {
+				assert_eq!(reestablish.next_remote_commitment_number, 0);
+			}
+		}
+		if send_funding_locked.0 || send_funding_locked.1 {
+			// If we expect any funding_locked's, both sides better have set
+			// next_local_commitment_number to 1
+			for reestablish in reestablish_1.iter() {
+				assert_eq!(reestablish.next_local_commitment_number, 1);
+			}
+			for reestablish in reestablish_2.iter() {
+				assert_eq!(reestablish.next_local_commitment_number, 1);
+			}
+		}
+
 		let mut resp_1 = Vec::new();
 		for msg in reestablish_1 {
 			node_b.node.handle_channel_reestablish(&node_a.node.get_our_node_id(), &msg).unwrap();