Merge pull request #253 from TheBlueMatt/2018-11-misc-tweaks
[rust-lightning] / src / ln / channel.rs
index 33e09448f4170f2a6e4fa70ef0f9f5a568cb938c..63d8754d903e9c79412b8b993fd3645b3fb68cb7 100644 (file)
@@ -2,11 +2,9 @@ use bitcoin::blockdata::block::BlockHeader;
 use bitcoin::blockdata::script::{Script,Builder};
 use bitcoin::blockdata::transaction::{TxIn, TxOut, Transaction, SigHashType};
 use bitcoin::blockdata::opcodes;
-use bitcoin::util::hash::{Sha256dHash, Hash160};
+use bitcoin::util::hash::{BitcoinHash, Sha256dHash, Hash160};
 use bitcoin::util::bip143;
-use bitcoin::network;
-use bitcoin::network::serialize::{BitcoinHash, RawDecoder, RawEncoder};
-use bitcoin::network::encodable::{ConsensusEncodable, ConsensusDecodable};
+use bitcoin::consensus::encode::{self, Encodable, Decodable};
 
 use secp256k1::key::{PublicKey,SecretKey};
 use secp256k1::{Secp256k1,Message,Signature};
@@ -1122,8 +1120,9 @@ impl Channel {
                Ok(our_sig)
        }
 
-       /// May return an IgnoreError, but should not, and will always return Ok(_) when
-       /// debug_assertions are turned on
+       /// Per HTLC, only one get_update_fail_htlc or get_update_fulfill_htlc call may be made.
+       /// In such cases we debug_assert!(false) and return an IgnoreError. Thus, will always return
+       /// Ok(_) if debug assertions are turned on and preconditions are met.
        fn get_update_fulfill_htlc(&mut self, htlc_id_arg: u64, payment_preimage_arg: [u8; 32]) -> Result<(Option<msgs::UpdateFulfillHTLC>, Option<ChannelMonitor>), HandleError> {
                // Either ChannelFunded got set (which means it wont bet unset) or there is no way any
                // caller thought we could have something claimed (cause we wouldn't have accepted in an
@@ -1175,7 +1174,9 @@ impl Channel {
                                        &HTLCUpdateAwaitingACK::FailHTLC { htlc_id, .. } => {
                                                if htlc_id_arg == htlc_id {
                                                        debug_assert!(false, "Tried to fulfill an HTLC we already had a holding-cell failure on");
-                                                       return Err(HandleError{err: "Unable to find a pending HTLC which matched the given HTLC ID", action: Some(msgs::ErrorAction::IgnoreError)});
+                                                       // Return the new channel monitor in a last-ditch effort to hit the
+                                                       // chain and claim the funds
+                                                       return Ok((None, Some(self.channel_monitor.clone())));
                                                }
                                        },
                                        _ => {}
@@ -1215,8 +1216,9 @@ impl Channel {
                }
        }
 
-       /// May return an IgnoreError, but should not, and will always return Ok(_) when
-       /// debug_assertions are turned on
+       /// Per HTLC, only one get_update_fail_htlc or get_update_fulfill_htlc call may be made.
+       /// In such cases we debug_assert!(false) and return an IgnoreError. Thus, will always return
+       /// Ok(_) if debug assertions are turned on and preconditions are met.
        pub fn get_update_fail_htlc(&mut self, htlc_id_arg: u64, err_packet: msgs::OnionErrorPacket) -> Result<Option<msgs::UpdateFailHTLC>, HandleError> {
                if (self.channel_state & (ChannelState::ChannelFunded as u32)) != (ChannelState::ChannelFunded as u32) {
                        panic!("Was asked to fail an HTLC when channel was not in an operational state");
@@ -1553,6 +1555,12 @@ impl Channel {
 
                //TODO: Check msg.cltv_expiry further? Do this in channel manager?
 
+               if self.channel_state & ChannelState::LocalShutdownSent as u32 != 0 {
+                       if let PendingHTLCStatus::Forward(_) = pending_forward_state {
+                               panic!("ChannelManager shouldn't be trying to add a forwardable HTLC after we've started closing");
+                       }
+               }
+
                // Now update local state:
                self.next_remote_htlc_id += 1;
                self.pending_inbound_htlcs.push(InboundHTLCOutput {
@@ -1632,13 +1640,16 @@ impl Channel {
                self.mark_outbound_htlc_removed(msg.htlc_id, None, Some(fail_reason))
        }
 
-       pub fn commitment_signed(&mut self, msg: &msgs::CommitmentSigned) -> Result<(msgs::RevokeAndACK, Option<msgs::CommitmentSigned>, ChannelMonitor), HandleError> {
+       pub fn commitment_signed(&mut self, msg: &msgs::CommitmentSigned, fee_estimator: &FeeEstimator) -> Result<(msgs::RevokeAndACK, Option<msgs::CommitmentSigned>, Option<msgs::ClosingSigned>, ChannelMonitor), HandleError> {
                if (self.channel_state & (ChannelState::ChannelFunded as u32)) != (ChannelState::ChannelFunded as u32) {
                        return Err(HandleError{err: "Got commitment signed message when channel was not in an operational state", action: None});
                }
                if self.channel_state & (ChannelState::PeerDisconnected as u32) == ChannelState::PeerDisconnected as u32 {
                        return Err(HandleError{err: "Peer sent commitment_signed when we needed a channel_reestablish", action: Some(msgs::ErrorAction::SendErrorMessage{msg: msgs::ErrorMessage{data: "Peer sent commitment_signed when we needed a channel_reestablish".to_string(), channel_id: msg.channel_id}})});
                }
+               if self.channel_state & BOTH_SIDES_SHUTDOWN_MASK == BOTH_SIDES_SHUTDOWN_MASK && self.last_sent_closing_fee.is_some() {
+                       return Err(HandleError{err: "Peer sent commitment_signed after we'd started exchanging closing_signeds", action: Some(msgs::ErrorAction::SendErrorMessage{msg: msgs::ErrorMessage{data: "Peer sent commitment_signed after we'd started exchanging closing_signeds".to_string(), channel_id: msg.channel_id}})});
+               }
 
                let funding_script = self.get_funding_redeemscript();
 
@@ -1730,19 +1741,21 @@ impl Channel {
                        return Err(HandleError{err: "Previous monitor update failure prevented generation of RAA", action: Some(ErrorAction::IgnoreError)});
                }
 
-               let (our_commitment_signed, monitor_update) = if need_our_commitment && (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == 0 {
+               let (our_commitment_signed, monitor_update, closing_signed) = if need_our_commitment && (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == 0 {
                        // If we're AwaitingRemoteRevoke we can't send a new commitment here, but that's ok -
                        // we'll send one right away when we get the revoke_and_ack when we
                        // free_holding_cell_htlcs().
                        let (msg, monitor) = self.send_commitment_no_status_check()?;
-                       (Some(msg), monitor)
-               } else { (None, self.channel_monitor.clone()) };
+                       (Some(msg), monitor, None)
+               } else if !need_our_commitment {
+                       (None, self.channel_monitor.clone(), self.maybe_propose_first_closing_signed(fee_estimator))
+               } else { (None, self.channel_monitor.clone(), None) };
 
                Ok((msgs::RevokeAndACK {
                        channel_id: self.channel_id,
                        per_commitment_secret: per_commitment_secret,
                        next_per_commitment_point: next_per_commitment_point,
-               }, our_commitment_signed, monitor_update))
+               }, our_commitment_signed, closing_signed, monitor_update))
        }
 
        /// Used to fulfill holding_cell_htlcs when we get a remote ack (or implicitly get it by them
@@ -1843,13 +1856,16 @@ impl Channel {
        /// waiting on this revoke_and_ack. The generation of this new commitment_signed may also fail,
        /// generating an appropriate error *after* the channel state has been updated based on the
        /// revoke_and_ack message.
-       pub fn revoke_and_ack(&mut self, msg: &msgs::RevokeAndACK) -> Result<(Option<msgs::CommitmentUpdate>, Vec<(PendingForwardHTLCInfo, u64)>, Vec<(HTLCSource, [u8; 32], HTLCFailReason)>, ChannelMonitor), HandleError> {
+       pub fn revoke_and_ack(&mut self, msg: &msgs::RevokeAndACK, fee_estimator: &FeeEstimator) -> Result<(Option<msgs::CommitmentUpdate>, Vec<(PendingForwardHTLCInfo, u64)>, Vec<(HTLCSource, [u8; 32], HTLCFailReason)>, Option<msgs::ClosingSigned>, ChannelMonitor), HandleError> {
                if (self.channel_state & (ChannelState::ChannelFunded as u32)) != (ChannelState::ChannelFunded as u32) {
                        return Err(HandleError{err: "Got revoke/ACK message when channel was not in an operational state", action: None});
                }
                if self.channel_state & (ChannelState::PeerDisconnected as u32) == ChannelState::PeerDisconnected as u32 {
                        return Err(HandleError{err: "Peer sent revoke_and_ack when we needed a channel_reestablish", action: Some(msgs::ErrorAction::SendErrorMessage{msg: msgs::ErrorMessage{data: "Peer sent revoke_and_ack when we needed a channel_reestablish".to_string(), channel_id: msg.channel_id}})});
                }
+               if self.channel_state & BOTH_SIDES_SHUTDOWN_MASK == BOTH_SIDES_SHUTDOWN_MASK && self.last_sent_closing_fee.is_some() {
+                       return Err(HandleError{err: "Peer sent revoke_and_ack after we'd started exchanging closing_signeds", action: Some(msgs::ErrorAction::SendErrorMessage{msg: msgs::ErrorMessage{data: "Peer sent revoke_and_ack after we'd started exchanging closing_signeds".to_string(), channel_id: msg.channel_id}})});
+               }
 
                if let Some(their_prev_commitment_point) = self.their_prev_commitment_point {
                        if PublicKey::from_secret_key(&self.secp_ctx, &secp_call!(SecretKey::from_slice(&self.secp_ctx, &msg.per_commitment_secret), "Peer provided an invalid per_commitment_secret", self.channel_id())) != their_prev_commitment_point {
@@ -1971,7 +1987,7 @@ impl Channel {
                        }
                        self.monitor_pending_forwards.append(&mut to_forward_infos);
                        self.monitor_pending_failures.append(&mut revoked_htlcs);
-                       return Ok((None, Vec::new(), Vec::new(), self.channel_monitor.clone()));
+                       return Ok((None, Vec::new(), Vec::new(), None, self.channel_monitor.clone()));
                }
 
                match self.free_holding_cell_htlcs()? {
@@ -1984,7 +2000,7 @@ impl Channel {
                                for fail_msg in update_fail_malformed_htlcs.drain(..) {
                                        commitment_update.0.update_fail_malformed_htlcs.push(fail_msg);
                                }
-                               Ok((Some(commitment_update.0), to_forward_infos, revoked_htlcs, commitment_update.1))
+                               Ok((Some(commitment_update.0), to_forward_infos, revoked_htlcs, None, commitment_update.1))
                        },
                        None => {
                                if require_commitment {
@@ -1996,9 +2012,9 @@ impl Channel {
                                                update_fail_malformed_htlcs,
                                                update_fee: None,
                                                commitment_signed
-                                       }), to_forward_infos, revoked_htlcs, monitor_update))
+                                       }), to_forward_infos, revoked_htlcs, None, monitor_update))
                                } else {
-                                       Ok((None, to_forward_infos, revoked_htlcs, self.channel_monitor.clone()))
+                                       Ok((None, to_forward_infos, revoked_htlcs, self.maybe_propose_first_closing_signed(fee_estimator), self.channel_monitor.clone()))
                                }
                        }
                }
@@ -2057,6 +2073,9 @@ impl Channel {
                        self.channel_state = ChannelState::ShutdownComplete as u32;
                        return outbound_drops;
                }
+               // Upon reconnect we have to start the closing_signed dance over, but shutdown messages
+               // will be retransmitted.
+               self.last_sent_closing_fee = None;
 
                let mut inbound_drop_count = 0;
                self.pending_inbound_htlcs.retain(|htlc| {
@@ -2244,7 +2263,7 @@ impl Channel {
 
        /// May panic if some calls other than message-handling calls (which will all Err immediately)
        /// have been called between remove_uncommitted_htlcs_and_mark_paused and this call.
-       pub fn channel_reestablish(&mut self, msg: &msgs::ChannelReestablish) -> Result<(Option<msgs::FundingLocked>, Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, Option<ChannelMonitor>, RAACommitmentOrder), ChannelError> {
+       pub fn channel_reestablish(&mut self, msg: &msgs::ChannelReestablish) -> Result<(Option<msgs::FundingLocked>, Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, Option<ChannelMonitor>, RAACommitmentOrder, Option<msgs::Shutdown>), ChannelError> {
                if self.channel_state & (ChannelState::PeerDisconnected as u32) == 0 {
                        // While BOLT 2 doesn't indicate explicitly we should error this channel here, it
                        // almost certainly indicates we are going to end up out-of-sync in some way, so we
@@ -2260,9 +2279,16 @@ impl Channel {
                // remaining cases either succeed or ErrorMessage-fail).
                self.channel_state &= !(ChannelState::PeerDisconnected as u32);
 
+               let shutdown_msg = if self.channel_state & (ChannelState::LocalShutdownSent as u32) != 0 {
+                       Some(msgs::Shutdown {
+                               channel_id: self.channel_id,
+                               scriptpubkey: self.get_closing_scriptpubkey(),
+                       })
+               } 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));
+                       return Ok((None, None, None, None, RAACommitmentOrder::CommitmentFirst, shutdown_msg));
                }
 
                if msg.next_local_commitment_number == 0 || msg.next_remote_commitment_number == 0 {
@@ -2275,7 +2301,7 @@ impl Channel {
                        return Ok((Some(msgs::FundingLocked {
                                channel_id: self.channel_id(),
                                next_per_commitment_point: next_per_commitment_point,
-                       }), None, None, None, RAACommitmentOrder::CommitmentFirst));
+                       }), 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 {
@@ -2338,11 +2364,11 @@ impl Channel {
                                                        panic!("Got non-channel-failing result from free_holding_cell_htlcs");
                                                }
                                        },
-                                       Ok(Some((commitment_update, channel_monitor))) => return Ok((resend_funding_locked, required_revoke, Some(commitment_update), Some(channel_monitor), order)),
-                                       Ok(None) => return Ok((resend_funding_locked, required_revoke, None, None, order)),
+                                       Ok(Some((commitment_update, channel_monitor))) => return Ok((resend_funding_locked, required_revoke, Some(commitment_update), Some(channel_monitor), order, shutdown_msg)),
+                                       Ok(None) => return Ok((resend_funding_locked, required_revoke, None, None, order, shutdown_msg)),
                                }
                        } else {
-                               return Ok((resend_funding_locked, required_revoke, None, None, order));
+                               return Ok((resend_funding_locked, required_revoke, None, None, order, shutdown_msg));
                        }
                } else if msg.next_local_commitment_number == our_next_remote_commitment_number - 1 {
                        if required_revoke.is_some() {
@@ -2356,17 +2382,20 @@ impl Channel {
 
                        if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) != 0 {
                                self.monitor_pending_commitment_signed = true;
-                               return Ok((resend_funding_locked, None, None, None, order));
+                               return Ok((resend_funding_locked, None, None, None, order, shutdown_msg));
                        }
 
-                       return Ok((resend_funding_locked, required_revoke, Some(self.get_last_commitment_update()), None, order));
+                       return Ok((resend_funding_locked, required_revoke, Some(self.get_last_commitment_update()), None, order, shutdown_msg));
                } else {
                        return Err(ChannelError::Close("Peer attempted to reestablish channel with a very old remote commitment transaction"));
                }
        }
 
        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() {
+               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{
                        return None;
                }
 
@@ -2404,9 +2433,6 @@ impl Channel {
                                return Err(ChannelError::Close("Got shutdown with remote pending HTLCs"));
                        }
                }
-               if (self.channel_state & ChannelState::RemoteShutdownSent as u32) == ChannelState::RemoteShutdownSent as u32 {
-                       return Err(ChannelError::Ignore("Remote peer sent duplicate shutdown message"));
-               }
                assert_eq!(self.channel_state & ChannelState::ShutdownComplete as u32, 0);
 
                // BOLT 2 says we must only send a scriptpubkey of certain standard forms, which are up to
@@ -2436,6 +2462,7 @@ impl Channel {
                // We can't send our shutdown until we've committed all of our pending HTLCs, but the
                // remote side is unlikely to accept any new HTLCs, so we go ahead and "free" any holding
                // cell HTLCs and return them to fail the payment.
+               self.holding_cell_update_fee = None;
                let mut dropped_outbound_htlcs = Vec::with_capacity(self.holding_cell_htlc_updates.len());
                self.holding_cell_htlc_updates.retain(|htlc_update| {
                        match htlc_update {
@@ -2798,6 +2825,16 @@ impl Channel {
                                                self.channel_update_count += 1;
                                                return Err(HandleError{err: "funding tx had wrong script/value", action: Some(ErrorAction::DisconnectPeer{msg: None})});
                                        } else {
+                                               if self.channel_outbound {
+                                                       for input in tx.input.iter() {
+                                                               if input.witness.is_empty() {
+                                                                       // We generated a malleable funding transaction, implying we've
+                                                                       // just exposed ourselves to funds loss to our counterparty.
+                                                                       #[cfg(not(feature = "fuzztarget"))]
+                                                                       panic!("Client called ChannelManager::funding_transaction_generated with bogus transaction!");
+                                                               }
+                                                       }
+                                               }
                                                self.funding_tx_confirmations = 1;
                                                self.short_channel_id = Some(((height as u64)          << (5*8)) |
                                                                             ((*index_in_block as u64) << (2*8)) |
@@ -3244,9 +3281,9 @@ impl Channel {
                }
                self.channel_update_count += 1;
 
-               // We can't send our shutdown until we've committed all of our pending HTLCs, but the
-               // remote side is unlikely to accept any new HTLCs, so we go ahead and "free" any holding
-               // cell HTLCs and return them to fail the payment.
+               // Go ahead and drop holding cell updates as we'd rather fail payments than wait to send
+               // our shutdown until we've committed all of the pending changes.
+               self.holding_cell_update_fee = None;
                let mut dropped_outbound_htlcs = Vec::with_capacity(self.holding_cell_htlc_updates.len());
                self.holding_cell_htlc_updates.retain(|htlc_update| {
                        match htlc_update {
@@ -3486,9 +3523,9 @@ impl Writeable for Channel {
 
                (self.last_local_commitment_txn.len() as u64).write(writer)?;
                for tx in self.last_local_commitment_txn.iter() {
-                       if let Err(e) = tx.consensus_encode(&mut RawEncoder::new(WriterWriteAdaptor(writer))) {
+                       if let Err(e) = tx.consensus_encode(&mut WriterWriteAdaptor(writer)) {
                                match e {
-                                       network::serialize::Error::Io(e) => return Err(e),
+                                       encode::Error::Io(e) => return Err(e),
                                        _ => panic!("last_local_commitment_txn must have been well-formed!"),
                                }
                        }
@@ -3665,7 +3702,7 @@ impl<R : ::std::io::Read> ReadableArgs<R, Arc<Logger>> for Channel {
                let last_local_commitment_txn_count: u64 = Readable::read(reader)?;
                let mut last_local_commitment_txn = Vec::with_capacity(cmp::min(last_local_commitment_txn_count as usize, OUR_MAX_HTLCS as usize*2 + 1));
                for _ in 0..last_local_commitment_txn_count {
-                       last_local_commitment_txn.push(match Transaction::consensus_decode(&mut RawDecoder::new(reader.by_ref())) {
+                       last_local_commitment_txn.push(match Transaction::consensus_decode(reader.by_ref()) {
                                Ok(tx) => tx,
                                Err(_) => return Err(DecodeError::InvalidValue),
                        });
@@ -3793,7 +3830,7 @@ impl<R : ::std::io::Read> ReadableArgs<R, Arc<Logger>> for Channel {
 mod tests {
        use bitcoin::util::hash::{Sha256dHash, Hash160};
        use bitcoin::util::bip143;
-       use bitcoin::network::serialize::serialize;
+       use bitcoin::consensus::encode::serialize;
        use bitcoin::blockdata::script::{Script, Builder};
        use bitcoin::blockdata::transaction::Transaction;
        use bitcoin::blockdata::opcodes;
@@ -3915,7 +3952,7 @@ mod tests {
 
                                chan.sign_commitment_transaction(&mut unsigned_tx.0, &their_signature);
 
-                               assert_eq!(serialize(&unsigned_tx.0).unwrap()[..],
+                               assert_eq!(serialize(&unsigned_tx.0)[..],
                                                hex::decode($tx_hex).unwrap()[..]);
                        };
                }
@@ -3948,7 +3985,7 @@ mod tests {
                                }
 
                                chan.sign_htlc_transaction(&mut htlc_tx, &remote_signature, &preimage, &htlc, &keys).unwrap();
-                               assert_eq!(serialize(&htlc_tx).unwrap()[..],
+                               assert_eq!(serialize(&htlc_tx)[..],
                                                hex::decode($tx_hex).unwrap()[..]);
                        };
                }