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.

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 1b5d6cfa7bfe575a7c7ea75a8bd59193ca97ce08..8332cf322d5929738d338feb1d87bed96716e157 100644 (file)
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -4180,10 +4180,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,11 +4227,11 @@ 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
@@ -4237,7 +4239,11 @@ impl<SP: Deref> Channel<SP> where
                                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 +4301,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 remote 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 remote commitment transaction: {} (received) vs {} (expected)",
+                               msg.next_local_commitment_number,
+                               next_counterparty_commitment_number,
+                       )))
                }
        }