Check for misuse of funding_transaction_generated and panic
[rust-lightning] / src / ln / channel.rs
index 7d858555f8728d5a54d0deb232c05393fde7abdd..a34db5dc7165dd31c49e5e7af4fae1c815ba2433 100644 (file)
@@ -16,7 +16,7 @@ use crypto::hkdf::{hkdf_extract,hkdf_expand};
 use ln::msgs;
 use ln::msgs::{ErrorAction, HandleError, MsgEncodable};
 use ln::channelmonitor::ChannelMonitor;
-use ln::channelmanager::{PendingHTLCStatus, PendingForwardHTLCInfo, HTLCFailReason};
+use ln::channelmanager::{PendingHTLCStatus, PendingForwardHTLCInfo, HTLCFailReason, HTLCFailureMsg};
 use ln::chan_utils::{TxCreationKeys,HTLCOutputInCommitment,HTLC_SUCCESS_TX_WEIGHT,HTLC_TIMEOUT_TX_WEIGHT};
 use ln::chan_utils;
 use chain::chaininterface::{FeeEstimator,ConfirmationTarget};
@@ -373,6 +373,8 @@ impl Channel {
        }
 
        fn derive_minimum_depth(_channel_value_satoshis_msat: u64, _value_to_self_msat: u64) -> u32 {
+               // Note that in order to comply with BOLT 7 announcement_signatures requirements this must
+               // be at least 6.
                const CONF_TARGET: u32 = 12; //TODO: Should be much higher
                CONF_TARGET
        }
@@ -1018,10 +1020,13 @@ impl Channel {
                for (idx, htlc) in self.pending_htlcs.iter().enumerate() {
                        if !htlc.outbound && htlc.payment_hash == payment_hash_calc &&
                                        htlc.state != HTLCState::LocalRemoved && htlc.state != HTLCState::LocalRemovedAwaitingCommitment {
-                               if pending_idx != std::usize::MAX {
-                                       panic!("Duplicate HTLC payment_hash, ChannelManager should have prevented this!");
+                               if let Some(PendingHTLCStatus::Fail(_)) = htlc.pending_forward_state {
+                               } else {
+                                       if pending_idx != std::usize::MAX {
+                                               panic!("Duplicate HTLC payment_hash, ChannelManager should have prevented this!");
+                                       }
+                                       pending_idx = idx;
                                }
-                               pending_idx = idx;
                        }
                }
                if pending_idx == std::usize::MAX {
@@ -1128,20 +1133,27 @@ impl Channel {
                let mut htlc_amount_msat = 0;
                for htlc in self.pending_htlcs.iter_mut() {
                        if !htlc.outbound && htlc.payment_hash == *payment_hash_arg {
-                               if htlc_id != 0 {
-                                       panic!("Duplicate HTLC payment_hash, you probably re-used payment preimages, NEVER DO THIS!");
-                               }
-                               htlc_id = htlc.htlc_id;
-                               htlc_amount_msat += htlc.amount_msat;
                                if htlc.state == HTLCState::Committed {
                                        htlc.state = HTLCState::LocalRemoved;
                                } else if htlc.state == HTLCState::RemoteAnnounced {
-                                       panic!("Somehow forwarded HTLC prior to remote revocation!");
+                                       if let Some(PendingHTLCStatus::Forward(_)) = htlc.pending_forward_state {
+                                               panic!("Somehow forwarded HTLC prior to remote revocation!");
+                                       } else {
+                                               // We have to pretend this isn't here - we're probably a duplicate with the
+                                               // same payment_hash as some other HTLC, and the other is getting failed,
+                                               // we'll fail this one as soon as remote commits to it.
+                                               continue;
+                                       }
                                } else if htlc.state == HTLCState::LocalRemoved || htlc.state == HTLCState::LocalRemovedAwaitingCommitment {
                                        return Err(HandleError{err: "Unable to find a pending HTLC which matched the given payment preimage", action: None});
                                } else {
                                        panic!("Have an inbound HTLC when not awaiting remote revoke that had a garbage state");
                                }
+                               if htlc_id != 0 {
+                                       panic!("Duplicate HTLC payment_hash, you probably re-used payment preimages, NEVER DO THIS!");
+                               }
+                               htlc_id = htlc.htlc_id;
+                               htlc_amount_msat += htlc.amount_msat;
                        }
                }
                if htlc_amount_msat == 0 {
@@ -1633,6 +1645,7 @@ impl Channel {
                                                update_add_htlcs,
                                                update_fulfill_htlcs,
                                                update_fail_htlcs,
+                                               update_fail_malformed_htlcs: Vec::new(),
                                                commitment_signed,
                                        }, monitor_update)))
                                },
@@ -1670,7 +1683,8 @@ impl Channel {
 
                let mut to_forward_infos = Vec::new();
                let mut revoked_htlcs = Vec::new();
-               let mut failed_htlcs = Vec::new();
+               let mut update_fail_htlcs = Vec::new();
+               let mut update_fail_malformed_htlcs = Vec::new();
                let mut require_commitment = false;
                let mut value_to_self_msat_diff: i64 = 0;
                // We really shouldnt have two passes here, but retain gives a non-mutable ref (Rust bug)
@@ -1698,7 +1712,10 @@ impl Channel {
                                        PendingHTLCStatus::Fail(fail_msg) => {
                                                htlc.state = HTLCState::LocalRemoved;
                                                require_commitment = true;
-                                               failed_htlcs.push(fail_msg);
+                                               match fail_msg {
+                                                       HTLCFailureMsg::Relay(msg) => update_fail_htlcs.push(msg),
+                                                       HTLCFailureMsg::Malformed(msg) => update_fail_malformed_htlcs.push(msg),
+                                               }
                                        },
                                        PendingHTLCStatus::Forward(forward_info) => {
                                                to_forward_infos.push(forward_info);
@@ -1717,10 +1734,14 @@ impl Channel {
 
                match self.free_holding_cell_htlcs()? {
                        Some(mut commitment_update) => {
-                               commitment_update.0.update_fail_htlcs.reserve(failed_htlcs.len());
-                               for fail_msg in failed_htlcs.drain(..) {
+                               commitment_update.0.update_fail_htlcs.reserve(update_fail_htlcs.len());
+                               for fail_msg in update_fail_htlcs.drain(..) {
                                        commitment_update.0.update_fail_htlcs.push(fail_msg);
                                }
+                               commitment_update.0.update_fail_malformed_htlcs.reserve(update_fail_malformed_htlcs.len());
+                               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))
                        },
                        None => {
@@ -1729,7 +1750,8 @@ impl Channel {
                                        Ok((Some(msgs::CommitmentUpdate {
                                                update_add_htlcs: Vec::new(),
                                                update_fulfill_htlcs: Vec::new(),
-                                               update_fail_htlcs: failed_htlcs,
+                                               update_fail_htlcs,
+                                               update_fail_malformed_htlcs,
                                                commitment_signed
                                        }), to_forward_infos, revoked_htlcs, monitor_update))
                                } else {
@@ -2091,7 +2113,16 @@ impl Channel {
                                if tx.txid() == self.channel_monitor.get_funding_txo().unwrap().txid {
                                        let txo_idx = self.channel_monitor.get_funding_txo().unwrap().index as usize;
                                        if txo_idx >= tx.output.len() || tx.output[txo_idx].script_pubkey != self.get_funding_redeemscript().to_v0_p2wsh() ||
-                                               tx.output[txo_idx].value != self.channel_value_satoshis {
+                                                       tx.output[txo_idx].value != self.channel_value_satoshis {
+                                               if self.channel_outbound {
+                                                       // If we generated the funding transaction and it doesn't match what it
+                                                       // should, the client is really broken and we should just panic and
+                                                       // tell them off. That said, because hash collisions happen with high
+                                                       // probability in fuzztarget mode, if we're fuzzing we just close the
+                                                       // channel and move on.
+                                                       #[cfg(not(feature = "fuzztarget"))]
+                                                       panic!("Client called ChannelManager::funding_transaction_generated with bogus transaction!");
+                                               }
                                                self.channel_state = ChannelState::ShutdownComplete as u32;
                                                self.channel_update_count += 1;
                                                return Err(HandleError{err: "funding tx had wrong script/value", action: Some(ErrorAction::DisconnectPeer{msg: None})});
@@ -2253,18 +2284,22 @@ impl Channel {
 
        /// Gets an UnsignedChannelAnnouncement, as well as a signature covering it using our
        /// bitcoin_key, if available, for this channel. The channel must be publicly announceable and
-       /// available for use (have exchanged FundingLocked messages in both directions. Should be used
+       /// available for use (have exchanged FundingLocked messages in both directions). Should be used
        /// for both loose and in response to an AnnouncementSignatures message from the remote peer.
-       /// Note that you can get an announcement for a channel which is closing, though you should
-       /// likely not announce such a thing. In case its already been announced, a channel_update
-       /// message can mark the channel disabled.
+       /// Will only fail if we're not in a state where channel_announcement may be sent (including
+       /// closing).
+       /// Note that the "channel must be funded" requirement is stricter than BOLT 7 requires - see
+       /// https://github.com/lightningnetwork/lightning-rfc/issues/468
        pub fn get_channel_announcement(&self, our_node_id: PublicKey, chain_hash: Sha256dHash) -> Result<(msgs::UnsignedChannelAnnouncement, Signature), HandleError> {
                if !self.announce_publicly {
                        return Err(HandleError{err: "Channel is not available for public announcements", action: None});
                }
-               if self.channel_state & (ChannelState::ChannelFunded as u32) != (ChannelState::ChannelFunded as u32) {
+               if self.channel_state & (ChannelState::ChannelFunded as u32) == 0 {
                        return Err(HandleError{err: "Cannot get a ChannelAnnouncement until the channel funding has been locked", action: None});
                }
+               if (self.channel_state & (ChannelState::LocalShutdownSent as u32 | ChannelState::ShutdownComplete as u32)) != 0 {
+                       return Err(HandleError{err: "Cannot get a ChannelAnnouncement once the channel is closing", action: None});
+               }
 
                let were_node_one = our_node_id.serialize()[..] < self.their_node_id.serialize()[..];
                let our_bitcoin_key = PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.funding_key);
@@ -2277,6 +2312,7 @@ impl Channel {
                        node_id_2: if were_node_one { self.get_their_node_id() } else { our_node_id },
                        bitcoin_key_1: if were_node_one { our_bitcoin_key } else { self.their_funding_pubkey.unwrap() },
                        bitcoin_key_2: if were_node_one { self.their_funding_pubkey.unwrap() } else { our_bitcoin_key },
+                       excess_data: Vec::new(),
                };
 
                let msghash = Message::from_slice(&Sha256dHash::from_data(&msg.encode()[..])[..]).unwrap();