From f24830719af671f2998a42c3ce248147e94fe0d2 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 9 Nov 2023 03:28:45 +0000 Subject: [PATCH] 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. --- lightning/src/ln/channel.rs | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 1b5d6cfa7..8332cf322 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4180,10 +4180,12 @@ impl Channel 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 Channel 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 Channel 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 Channel 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, + ))) } } -- 2.39.5