From 23c2cef5f42c201ed5f87edeffaab77490d7b427 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 26 Nov 2018 18:31:51 -0500 Subject: [PATCH 1/1] Fix channel_reestablish generation/handling around next_remote. --- src/ln/channel.rs | 43 ++++++++++++++++++++++++++++------------ src/ln/channelmanager.rs | 25 +++++++++++++++++++++++ 2 files changed, 55 insertions(+), 13 deletions(-) diff --git a/src/ln/channel.rs b/src/ln/channel.rs index 0ad0c0f52..296d97c16 100644 --- a/src/ln/channel.rs +++ b/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, } } diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index 2a2a40bce..a47236469 100644 --- a/src/ln/channelmanager.rs +++ b/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(); -- 2.39.5