Clean up error messages and conditionals in reestablish handling

When we reestablish there are generally always 4 conditions for
both local and remote commitment transactions:
 * we're stale and have possibly lost data
 * we're ahead and the peer has lost data
 * we're caught up
 * we're nearly caught up and need to retransmit one update.

In especially the local commitment case we had a mess of different
comparisons, which is improved here. Further, the error messages
are clarified and include more information.

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -4179,11 +4179,12 @@ impl<SP: Deref> Channel<SP> where
 
 		// Before we change the state of the channel, we check if the peer is sending a very old
 		// commitment transaction number, if yes we send a warning message.
-		let our_commitment_transaction = INITIAL_COMMITMENT_NUMBER - self.context.cur_holder_commitment_transaction_number - 1;
-		if  msg.next_remote_commitment_number + 1 < our_commitment_transaction {
-			return Err(
-				ChannelError::Warn(format!("Peer attempted to reestablish channel with a very old local commitment transaction: {} (received) vs {} (expected)", msg.next_remote_commitment_number, our_commitment_transaction))
-			);
+		if msg.next_remote_commitment_number + 1 < our_commitment_transaction {
+			return Err(ChannelError::Warn(format!(
+				"Peer attempted to reestablish channel with a very old local commitment transaction: {} (received) vs {} (expected)",
+				msg.next_remote_commitment_number,
+				our_commitment_transaction
+			)));
 		}
 
 		// Go ahead and unmark PeerDisconnected as various calls we may make check for it (and all
@@ -4225,19 +4226,23 @@ impl<SP: Deref> Channel<SP> where
 			});
 		}
 
-		let required_revoke = if msg.next_remote_commitment_number + 1 == INITIAL_COMMITMENT_NUMBER - self.context.cur_holder_commitment_transaction_number {
+		let required_revoke = if msg.next_remote_commitment_number == our_commitment_transaction {
 			// Remote isn't waiting on any RevokeAndACK from us!
 			// Note that if we need to repeat our ChannelReady we'll do that in the next if block.
 			None
-		} else if msg.next_remote_commitment_number + 1 == (INITIAL_COMMITMENT_NUMBER - 1) - self.context.cur_holder_commitment_transaction_number {
+		} else if msg.next_remote_commitment_number + 1 == our_commitment_transaction {
 			if self.context.channel_state & (ChannelState::MonitorUpdateInProgress as u32) != 0 {
 				self.context.monitor_pending_revoke_and_ack = true;
 				None
 			} else {
 				Some(self.get_last_revoke_and_ack())
 			}
 		} else {
-			return Err(ChannelError::Close("Peer attempted to reestablish channel with a very old local commitment transaction".to_owned()));
+			return Err(ChannelError::Close(format!(
+				"Peer attempted to reestablish channel expecting a future local commitment transaction: {} (received) vs {} (expected)",
+				msg.next_remote_commitment_number,
+				our_commitment_transaction
+			)));
 		};
 
 		// We increment cur_counterparty_commitment_transaction_number only upon receipt of
@@ -4295,8 +4300,18 @@ impl<SP: Deref> Channel<SP> where
 					order: self.context.resend_order.clone(),
 				})
 			}
+		} else if msg.next_local_commitment_number < next_counterparty_commitment_number {
+			Err(ChannelError::Close(format!(
+				"Peer attempted to reestablish channel with a very old local commitment transaction: {} (received) vs {} (expected)",
+				msg.next_local_commitment_number,
+				next_counterparty_commitment_number,
+			)))
 		} else {
-			Err(ChannelError::Close("Peer attempted to reestablish channel with a very old remote commitment transaction".to_owned()))
+			Err(ChannelError::Close(format!(
+				"Peer attempted to reestablish channel with a future local commitment transaction: {} (received) vs {} (expected)",
+				msg.next_local_commitment_number,
+				next_counterparty_commitment_number,
+			)))
 		}
 	}