Remove check which makes us sometimes never send closing_signed
[rust-lightning] / src / ln / channel.rs
index 1ae3333c1aa862c9262375008003d47f9cc70681..f6177e9eb6b9cc2f1786c60247b1fcaecae8e5ac 100644 (file)
@@ -1004,6 +1004,7 @@ impl Channel {
        #[inline]
        /// Creates a set of keys for build_commitment_transaction to generate a transaction which we
        /// will sign and send to our counterparty.
+       /// If an Err is returned, it is a ChannelError::Close (for get_outbound_funding_created)
        fn build_remote_transaction_keys(&self) -> Result<TxCreationKeys, ChannelError> {
                //TODO: Ensure that the payment_key derived here ends up in the library users' wallet as we
                //may see payments to it!
@@ -2288,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"));
                }
 
@@ -2303,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);
@@ -2321,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
@@ -2404,8 +2406,7 @@ impl Channel {
        fn maybe_propose_first_closing_signed(&mut self, fee_estimator: &FeeEstimator) -> Option<msgs::ClosingSigned> {
                if !self.channel_outbound || !self.pending_inbound_htlcs.is_empty() || !self.pending_outbound_htlcs.is_empty() ||
                                self.channel_state & (BOTH_SIDES_SHUTDOWN_MASK | ChannelState::AwaitingRemoteRevoke as u32) != BOTH_SIDES_SHUTDOWN_MASK ||
-                               self.last_sent_closing_fee.is_some() ||
-                               self.cur_remote_commitment_transaction_number != self.cur_local_commitment_transaction_number{
+                               self.last_sent_closing_fee.is_some() || self.pending_update_fee.is_some() {
                        return None;
                }
 
@@ -2949,6 +2950,7 @@ impl Channel {
                }
        }
 
+       /// If an Err is returned, it is a ChannelError::Close (for get_outbound_funding_created)
        fn get_outbound_funding_created_signature(&mut self) -> Result<(Signature, Transaction), ChannelError> {
                let funding_script = self.get_funding_redeemscript();
 
@@ -2966,6 +2968,7 @@ impl Channel {
        /// or if called on an inbound channel.
        /// Note that channel_id changes during this call!
        /// Do NOT broadcast the funding transaction until after a successful funding_signed call!
+       /// If an Err is returned, it is a ChannelError::Close.
        pub fn get_outbound_funding_created(&mut self, funding_txo: OutPoint) -> Result<(msgs::FundingCreated, ChannelMonitor), ChannelError> {
                if !self.channel_outbound {
                        panic!("Tried to create outbound funding_created message on an inbound channel!");
@@ -3050,11 +3053,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,
                }
        }
@@ -3898,6 +3917,7 @@ mod tests {
                }
 
                fn get_channel_keys(&self, _inbound: bool) -> ChannelKeys { self.chan_keys.clone() }
+               fn get_session_key(&self) -> SecretKey { panic!(); }
        }
 
        #[test]