Merge pull request #890 from TheBlueMatt/2021-04-fix-chan-shutdown-crash
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Sat, 24 Apr 2021 00:03:42 +0000 (00:03 +0000)
committerGitHub <noreply@github.com>
Sat, 24 Apr 2021 00:03:42 +0000 (00:03 +0000)
Fix (and test) panic when our counterparty uses a bogus funding tx

1  2 
lightning/src/ln/channel.rs
lightning/src/ln/channelmanager.rs
lightning/src/ln/functional_tests.rs

index 512b6d96ab32782942b6a992331642d2e0f00344,1c34e3391493fe33a9bddff6c2c680a66f9eac34..8a1489d4ef775c7be7d515bcc3153895437eb726
@@@ -3574,7 -3574,6 +3574,6 @@@ impl<Signer: Sign> Channel<Signer> 
                                                                #[cfg(not(feature = "fuzztarget"))]
                                                                panic!("Client called ChannelManager::funding_transaction_generated with bogus transaction!");
                                                        }
-                                                       self.channel_state = ChannelState::ShutdownComplete as u32;
                                                        self.update_time_counter += 1;
                                                        return Err(msgs::ErrorMessage {
                                                                channel_id: self.channel_id(),
                                                }
                                        }
                                        // If we allow 1-conf funding, we may need to check for funding_locked here and
 -                                      // send it immediately instead of waiting for an update_best_block call (which
 +                                      // send it immediately instead of waiting for a best_block_updated call (which
                                        // may have already happened for this block).
                                        if let Some(funding_locked) = self.check_get_funding_locked(height) {
                                                return Ok(Some(funding_locked));
        ///
        /// May return some HTLCs (and their payment_hash) which have timed out and should be failed
        /// back.
 -      pub fn update_best_block(&mut self, height: u32, highest_header_time: u32) -> Result<(Option<msgs::FundingLocked>, Vec<(HTLCSource, PaymentHash)>), msgs::ErrorMessage> {
 +      pub fn best_block_updated(&mut self, height: u32, highest_header_time: u32) -> Result<(Option<msgs::FundingLocked>, Vec<(HTLCSource, PaymentHash)>), msgs::ErrorMessage> {
                let mut timed_out_htlcs = Vec::new();
                let unforwarded_htlc_cltv_limit = height + HTLC_FAIL_BACK_BUFFER;
                self.holding_cell_htlc_updates.retain(|htlc_update| {
        /// before the channel has reached funding_locked and we can just wait for more blocks.
        pub fn funding_transaction_unconfirmed(&mut self) -> Result<(), msgs::ErrorMessage> {
                if self.funding_tx_confirmation_height != 0 {
 -                      // We handle the funding disconnection by calling update_best_block with a height one
 +                      // We handle the funding disconnection by calling best_block_updated with a height one
                        // below where our funding was connected, implying a reorg back to conf_height - 1.
                        let reorg_height = self.funding_tx_confirmation_height - 1;
                        // We use the time field to bump the current time we set on channel updates if its
                        // larger. If we don't know that time has moved forward, we can just set it to the last
                        // time we saw and it will be ignored.
                        let best_time = self.update_time_counter;
 -                      match self.update_best_block(reorg_height, best_time) {
 +                      match self.best_block_updated(reorg_height, best_time) {
                                Ok((funding_locked, timed_out_htlcs)) => {
                                        assert!(funding_locked.is_none(), "We can't generate a funding with 0 confirmations?");
                                        assert!(timed_out_htlcs.is_empty(), "We can't have accepted HTLCs with a timeout before our funding confirmation?");
index 978171b1ba4f5bb68f600c543f6e5f6fc75ada73,0360f8a69a8649dcb72e03fcf295ed2f2e9daa59..80edbacf573a7a545107a7908698fb0bf371cb13
@@@ -36,7 -36,6 +36,7 @@@ use bitcoin::secp256k1::ecdh::SharedSec
  use bitcoin::secp256k1;
  
  use chain;
 +use chain::Confirm;
  use chain::Watch;
  use chain::chaininterface::{BroadcasterInterface, FeeEstimator};
  use chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, ChannelMonitorUpdateErr, HTLC_FAIL_BACK_BUFFER, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY, MonitorEvent, CLOSED_CHANNEL_UPDATE_ID};
@@@ -1564,61 -1563,14 +1564,14 @@@ impl<Signer: Sign, M: Deref, T: Deref, 
                }
        }
  
-       /// Call this upon creation of a funding transaction for the given channel.
-       ///
-       /// Returns an [`APIError::APIMisuseError`] if the funding_transaction spent non-SegWit outputs
-       /// or if no output was found which matches the parameters in [`Event::FundingGenerationReady`].
-       ///
-       /// Panics if a funding transaction has already been provided for this channel.
-       ///
-       /// May panic if the output found in the funding transaction is duplicative with some other
-       /// channel (note that this should be trivially prevented by using unique funding transaction
-       /// keys per-channel).
-       ///
-       /// Do NOT broadcast the funding transaction yourself. When we have safely received our
-       /// counterparty's signature the funding transaction will automatically be broadcast via the
-       /// [`BroadcasterInterface`] provided when this `ChannelManager` was constructed.
-       ///
-       /// Note that this includes RBF or similar transaction replacement strategies - lightning does
-       /// not currently support replacing a funding transaction on an existing channel. Instead,
-       /// create a new channel with a conflicting funding transaction.
-       pub fn funding_transaction_generated(&self, temporary_channel_id: &[u8; 32], funding_transaction: Transaction) -> Result<(), APIError> {
-               let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
-               for inp in funding_transaction.input.iter() {
-                       if inp.witness.is_empty() {
-                               return Err(APIError::APIMisuseError {
-                                       err: "Funding transaction must be fully signed and spend Segwit outputs".to_owned()
-                               });
-                       }
-               }
+       /// Handles the generation of a funding transaction, optionally (for tests) with a function
+       /// which checks the correctness of the funding transaction given the associated channel.
+       fn funding_transaction_generated_intern<FundingOutput: Fn(&Channel<Signer>, &Transaction) -> Result<OutPoint, APIError>>
+                       (&self, temporary_channel_id: &[u8; 32], funding_transaction: Transaction, find_funding_output: FundingOutput) -> Result<(), APIError> {
                let (chan, msg) = {
                        let (res, chan) = match self.channel_state.lock().unwrap().by_id.remove(temporary_channel_id) {
                                Some(mut chan) => {
-                                       let mut output_index = None;
-                                       let expected_spk = chan.get_funding_redeemscript().to_v0_p2wsh();
-                                       for (idx, outp) in funding_transaction.output.iter().enumerate() {
-                                               if outp.script_pubkey == expected_spk && outp.value == chan.get_value_satoshis() {
-                                                       if output_index.is_some() {
-                                                               return Err(APIError::APIMisuseError {
-                                                                       err: "Multiple outputs matched the expected script and value".to_owned()
-                                                               });
-                                                       }
-                                                       if idx > u16::max_value() as usize {
-                                                               return Err(APIError::APIMisuseError {
-                                                                       err: "Transaction had more than 2^16 outputs, which is not supported".to_owned()
-                                                               });
-                                                       }
-                                                       output_index = Some(idx as u16);
-                                               }
-                                       }
-                                       if output_index.is_none() {
-                                               return Err(APIError::APIMisuseError {
-                                                       err: "No output matched the script_pubkey and value in the FundingGenerationReady event".to_owned()
-                                               });
-                                       }
-                                       let funding_txo = OutPoint { txid: funding_transaction.txid(), index: output_index.unwrap() };
+                                       let funding_txo = find_funding_output(&chan, &funding_transaction)?;
  
                                        (chan.get_outbound_funding_created(funding_transaction, funding_txo, &self.logger)
                                                .map_err(|e| if let ChannelError::Close(msg) = e {
                Ok(())
        }
  
+       #[cfg(test)]
+       pub(crate) fn funding_transaction_generated_unchecked(&self, temporary_channel_id: &[u8; 32], funding_transaction: Transaction, output_index: u16) -> Result<(), APIError> {
+               self.funding_transaction_generated_intern(temporary_channel_id, funding_transaction, |_, tx| {
+                       Ok(OutPoint { txid: tx.txid(), index: output_index })
+               })
+       }
+       /// Call this upon creation of a funding transaction for the given channel.
+       ///
+       /// Returns an [`APIError::APIMisuseError`] if the funding_transaction spent non-SegWit outputs
+       /// or if no output was found which matches the parameters in [`Event::FundingGenerationReady`].
+       ///
+       /// Panics if a funding transaction has already been provided for this channel.
+       ///
+       /// May panic if the output found in the funding transaction is duplicative with some other
+       /// channel (note that this should be trivially prevented by using unique funding transaction
+       /// keys per-channel).
+       ///
+       /// Do NOT broadcast the funding transaction yourself. When we have safely received our
+       /// counterparty's signature the funding transaction will automatically be broadcast via the
+       /// [`BroadcasterInterface`] provided when this `ChannelManager` was constructed.
+       ///
+       /// Note that this includes RBF or similar transaction replacement strategies - lightning does
+       /// not currently support replacing a funding transaction on an existing channel. Instead,
+       /// create a new channel with a conflicting funding transaction.
+       pub fn funding_transaction_generated(&self, temporary_channel_id: &[u8; 32], funding_transaction: Transaction) -> Result<(), APIError> {
+               let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
+               for inp in funding_transaction.input.iter() {
+                       if inp.witness.is_empty() {
+                               return Err(APIError::APIMisuseError {
+                                       err: "Funding transaction must be fully signed and spend Segwit outputs".to_owned()
+                               });
+                       }
+               }
+               self.funding_transaction_generated_intern(temporary_channel_id, funding_transaction, |chan, tx| {
+                       let mut output_index = None;
+                       let expected_spk = chan.get_funding_redeemscript().to_v0_p2wsh();
+                       for (idx, outp) in tx.output.iter().enumerate() {
+                               if outp.script_pubkey == expected_spk && outp.value == chan.get_value_satoshis() {
+                                       if output_index.is_some() {
+                                               return Err(APIError::APIMisuseError {
+                                                       err: "Multiple outputs matched the expected script and value".to_owned()
+                                               });
+                                       }
+                                       if idx > u16::max_value() as usize {
+                                               return Err(APIError::APIMisuseError {
+                                                       err: "Transaction had more than 2^16 outputs, which is not supported".to_owned()
+                                               });
+                                       }
+                                       output_index = Some(idx as u16);
+                               }
+                       }
+                       if output_index.is_none() {
+                               return Err(APIError::APIMisuseError {
+                                       err: "No output matched the script_pubkey and value in the FundingGenerationReady event".to_owned()
+                               });
+                       }
+                       Ok(OutPoint { txid: tx.txid(), index: output_index.unwrap() })
+               })
+       }
        fn get_announcement_sigs(&self, chan: &Channel<Signer>) -> Option<msgs::AnnouncementSignatures> {
                if !chan.should_announce() {
                        log_trace!(self.logger, "Can't send announcement_signatures for private channel {}", log_bytes!(chan.channel_id()));
@@@ -3364,8 -3378,8 +3379,8 @@@ wher
                }
  
                let txdata: Vec<_> = block.txdata.iter().enumerate().collect();
 -              self.transactions_confirmed(&block.header, height, &txdata);
 -              self.update_best_block(&block.header, height);
 +              self.transactions_confirmed(&block.header, &txdata, height);
 +              self.best_block_updated(&block.header, height);
        }
  
        fn block_disconnected(&self, header: &BlockHeader, height: u32) {
                        *best_block = BestBlock::new(header.prev_blockhash, new_height)
                }
  
 -              self.do_chain_event(Some(new_height), |channel| channel.update_best_block(new_height, header.time));
 +              self.do_chain_event(Some(new_height), |channel| channel.best_block_updated(new_height, header.time));
 +      }
 +}
 +
 +impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> chain::Confirm for ChannelManager<Signer, M, T, K, F, L>
 +where
 +      M::Target: chain::Watch<Signer>,
 +      T::Target: BroadcasterInterface,
 +      K::Target: KeysInterface<Signer = Signer>,
 +      F::Target: FeeEstimator,
 +      L::Target: Logger,
 +{
 +      fn transactions_confirmed(&self, header: &BlockHeader, txdata: &TransactionData, height: u32) {
 +              // Note that we MUST NOT end up calling methods on self.chain_monitor here - we're called
 +              // during initialization prior to the chain_monitor being fully configured in some cases.
 +              // See the docs for `ChannelManagerReadArgs` for more.
 +
 +              let block_hash = header.block_hash();
 +              log_trace!(self.logger, "{} transactions included in block {} at height {} provided", txdata.len(), block_hash, height);
 +
 +              let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
 +              self.do_chain_event(Some(height), |channel| channel.transactions_confirmed(&block_hash, height, txdata, &self.logger).map(|a| (a, Vec::new())));
 +      }
 +
 +      fn best_block_updated(&self, header: &BlockHeader, height: u32) {
 +              // Note that we MUST NOT end up calling methods on self.chain_monitor here - we're called
 +              // during initialization prior to the chain_monitor being fully configured in some cases.
 +              // See the docs for `ChannelManagerReadArgs` for more.
 +
 +              let block_hash = header.block_hash();
 +              log_trace!(self.logger, "New best block: {} at height {}", block_hash, height);
 +
 +              let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
 +
 +              *self.best_block.write().unwrap() = BestBlock::new(block_hash, height);
 +
 +              self.do_chain_event(Some(height), |channel| channel.best_block_updated(height, header.time));
 +
 +              loop {
 +                      // Update last_node_announcement_serial to be the max of its current value and the
 +                      // block timestamp. This should keep us close to the current time without relying on
 +                      // having an explicit local time source.
 +                      // Just in case we end up in a race, we loop until we either successfully update
 +                      // last_node_announcement_serial or decide we don't need to.
 +                      let old_serial = self.last_node_announcement_serial.load(Ordering::Acquire);
 +                      if old_serial >= header.time as usize { break; }
 +                      if self.last_node_announcement_serial.compare_exchange(old_serial, header.time as usize, Ordering::AcqRel, Ordering::Relaxed).is_ok() {
 +                              break;
 +                      }
 +              }
 +      }
 +
 +      fn get_relevant_txids(&self) -> Vec<Txid> {
 +              let channel_state = self.channel_state.lock().unwrap();
 +              let mut res = Vec::with_capacity(channel_state.short_to_id.len());
 +              for chan in channel_state.by_id.values() {
 +                      if let Some(funding_txo) = chan.get_funding_txo() {
 +                              res.push(funding_txo.txid);
 +                      }
 +              }
 +              res
 +      }
 +
 +      fn transaction_unconfirmed(&self, txid: &Txid) {
 +              let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
 +              self.do_chain_event(None, |channel| {
 +                      if let Some(funding_txo) = channel.get_funding_txo() {
 +                              if funding_txo.txid == *txid {
 +                                      channel.funding_transaction_unconfirmed().map(|_| (None, Vec::new()))
 +                              } else { Ok((None, Vec::new())) }
 +                      } else { Ok((None, Vec::new())) }
 +              });
        }
  }
  
  impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<Signer, M, T, K, F, L>
 -      where M::Target: chain::Watch<Signer>,
 -        T::Target: BroadcasterInterface,
 -        K::Target: KeysInterface<Signer = Signer>,
 -        F::Target: FeeEstimator,
 -        L::Target: Logger,
 +where
 +      M::Target: chain::Watch<Signer>,
 +      T::Target: BroadcasterInterface,
 +      K::Target: KeysInterface<Signer = Signer>,
 +      F::Target: FeeEstimator,
 +      L::Target: Logger,
  {
        /// Calls a function which handles an on-chain event (blocks dis/connected, transactions
        /// un/confirmed, etc) on each channel, handling any resulting errors or messages generated by
                }
        }
  
 -      /// Updates channel state to take note of transactions which were confirmed in the given block
 -      /// at the given height.
 -      ///
 -      /// Note that you must still call (or have called) [`update_best_block`] with the block
 -      /// information which is included here.
 -      ///
 -      /// This method may be called before or after [`update_best_block`] for a given block's
 -      /// transaction data and may be called multiple times with additional transaction data for a
 -      /// given block.
 -      ///
 -      /// This method may be called for a previous block after an [`update_best_block`] call has
 -      /// been made for a later block, however it must *not* be called with transaction data from a
 -      /// block which is no longer in the best chain (ie where [`update_best_block`] has already
 -      /// been informed about a blockchain reorganization which no longer includes the block which
 -      /// corresponds to `header`).
 -      ///
 -      /// [`update_best_block`]: `Self::update_best_block`
 -      pub fn transactions_confirmed(&self, header: &BlockHeader, height: u32, txdata: &TransactionData) {
 -              // Note that we MUST NOT end up calling methods on self.chain_monitor here - we're called
 -              // during initialization prior to the chain_monitor being fully configured in some cases.
 -              // See the docs for `ChannelManagerReadArgs` for more.
 -
 -              let block_hash = header.block_hash();
 -              log_trace!(self.logger, "{} transactions included in block {} at height {} provided", txdata.len(), block_hash, height);
 -
 -              let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
 -              self.do_chain_event(Some(height), |channel| channel.transactions_confirmed(&block_hash, height, txdata, &self.logger).map(|a| (a, Vec::new())));
 -      }
 -
 -      /// Updates channel state with the current best blockchain tip. You should attempt to call this
 -      /// quickly after a new block becomes available, however if multiple new blocks become
 -      /// available at the same time, only a single `update_best_block()` call needs to be made.
 -      ///
 -      /// This method should also be called immediately after any block disconnections, once at the
 -      /// reorganization fork point, and once with the new chain tip. Calling this method at the
 -      /// blockchain reorganization fork point ensures we learn when a funding transaction which was
 -      /// previously confirmed is reorganized out of the blockchain, ensuring we do not continue to
 -      /// accept payments which cannot be enforced on-chain.
 -      ///
 -      /// In both the block-connection and block-disconnection case, this method may be called either
 -      /// once per block connected or disconnected, or simply at the fork point and new tip(s),
 -      /// skipping any intermediary blocks.
 -      pub fn update_best_block(&self, header: &BlockHeader, height: u32) {
 -              // Note that we MUST NOT end up calling methods on self.chain_monitor here - we're called
 -              // during initialization prior to the chain_monitor being fully configured in some cases.
 -              // See the docs for `ChannelManagerReadArgs` for more.
 -
 -              let block_hash = header.block_hash();
 -              log_trace!(self.logger, "New best block: {} at height {}", block_hash, height);
 -
 -              let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
 -
 -              *self.best_block.write().unwrap() = BestBlock::new(block_hash, height);
 -
 -              self.do_chain_event(Some(height), |channel| channel.update_best_block(height, header.time));
 -
 -              loop {
 -                      // Update last_node_announcement_serial to be the max of its current value and the
 -                      // block timestamp. This should keep us close to the current time without relying on
 -                      // having an explicit local time source.
 -                      // Just in case we end up in a race, we loop until we either successfully update
 -                      // last_node_announcement_serial or decide we don't need to.
 -                      let old_serial = self.last_node_announcement_serial.load(Ordering::Acquire);
 -                      if old_serial >= header.time as usize { break; }
 -                      if self.last_node_announcement_serial.compare_exchange(old_serial, header.time as usize, Ordering::AcqRel, Ordering::Relaxed).is_ok() {
 -                              break;
 -                      }
 -              }
 -      }
 -
 -      /// Gets the set of txids which should be monitored for their confirmation state.
 -      ///
 -      /// If you're providing information about reorganizations via [`transaction_unconfirmed`], this
 -      /// is the set of transactions which you may need to call [`transaction_unconfirmed`] for.
 -      ///
 -      /// This may be useful to poll to determine the set of transactions which must be registered
 -      /// with an Electrum server or for which an Electrum server needs to be polled to determine
 -      /// transaction confirmation state.
 -      ///
 -      /// This may update after any [`transactions_confirmed`] or [`block_connected`] call.
 -      ///
 -      /// Note that this is NOT the set of transactions which must be included in calls to
 -      /// [`transactions_confirmed`] if they are confirmed, but a small subset of it.
 -      ///
 -      /// [`transactions_confirmed`]: Self::transactions_confirmed
 -      /// [`transaction_unconfirmed`]: Self::transaction_unconfirmed
 -      /// [`block_connected`]: chain::Listen::block_connected
 -      pub fn get_relevant_txids(&self) -> Vec<Txid> {
 -              let channel_state = self.channel_state.lock().unwrap();
 -              let mut res = Vec::with_capacity(channel_state.short_to_id.len());
 -              for chan in channel_state.by_id.values() {
 -                      if let Some(funding_txo) = chan.get_funding_txo() {
 -                              res.push(funding_txo.txid);
 -                      }
 -              }
 -              res
 -      }
 -
 -      /// Marks a transaction as having been reorganized out of the blockchain.
 -      ///
 -      /// If a transaction is included in [`get_relevant_txids`], and is no longer in the main branch
 -      /// of the blockchain, this function should be called to indicate that the transaction should
 -      /// be considered reorganized out.
 -      ///
 -      /// Once this is called, the given transaction will no longer appear on [`get_relevant_txids`],
 -      /// though this may be called repeatedly for a given transaction without issue.
 -      ///
 -      /// Note that if the transaction is confirmed on the main chain in a different block (indicated
 -      /// via a call to [`transactions_confirmed`]), it may re-appear in [`get_relevant_txids`], thus
 -      /// be very wary of race-conditions wherein the final state of a transaction indicated via
 -      /// these APIs is not the same as its state on the blockchain.
 -      ///
 -      /// [`transactions_confirmed`]: Self::transactions_confirmed
 -      /// [`get_relevant_txids`]: Self::get_relevant_txids
 -      pub fn transaction_unconfirmed(&self, txid: &Txid) {
 -              let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
 -              self.do_chain_event(None, |channel| {
 -                      if let Some(funding_txo) = channel.get_funding_txo() {
 -                              if funding_txo.txid == *txid {
 -                                      channel.funding_transaction_unconfirmed().map(|_| (None, Vec::new()))
 -                              } else { Ok((None, Vec::new())) }
 -                      } else { Ok((None, Vec::new())) }
 -              });
 -      }
 -
        /// Blocks until ChannelManager needs to be persisted or a timeout is reached. It returns a bool
        /// indicating whether persistence is necessary. Only one listener on
        /// `await_persistable_update` or `await_persistable_update_timeout` is guaranteed to be woken
index 731bf7c1b0e213133072b10b5afc8bd11daf23aa,f3b8ca15594dcccfa6ec0965a59e94e8c1957f14..7a39db2f312b1bf97b7a340906d55018b6d13e53
@@@ -12,7 -12,6 +12,7 @@@
  //! claim outputs on-chain.
  
  use chain;
 +use chain::Listen;
  use chain::Watch;
  use chain::channelmonitor;
  use chain::channelmonitor::{ChannelMonitor, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY};
@@@ -8226,7 -8225,7 +8226,7 @@@ fn test_update_err_monitor_lockdown() 
                watchtower
        };
        let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
 -      watchtower.chain_monitor.block_connected(&header, &[], 200);
 +      watchtower.chain_monitor.block_connected(&Block { header, txdata: vec![] }, 200);
  
        // Try to update ChannelMonitor
        assert!(nodes[1].node.claim_funds(preimage, &None, 9_000_000));
@@@ -8285,7 -8284,7 +8285,7 @@@ fn test_concurrent_monitor_claim() 
                watchtower
        };
        let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
 -      watchtower_alice.chain_monitor.block_connected(&header, &vec![], CHAN_CONFIRM_DEPTH + 1 + TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS);
 +      watchtower_alice.chain_monitor.block_connected(&Block { header, txdata: vec![] }, CHAN_CONFIRM_DEPTH + 1 + TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS);
  
        // Watchtower Alice should have broadcast a commitment/HTLC-timeout
        {
                watchtower
        };
        let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
 -      watchtower_bob.chain_monitor.block_connected(&header, &vec![], CHAN_CONFIRM_DEPTH + TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS);
 +      watchtower_bob.chain_monitor.block_connected(&Block { header, txdata: vec![] }, CHAN_CONFIRM_DEPTH + TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS);
  
        // Route another payment to generate another update with still previous HTLC pending
        let (_, payment_hash) = get_payment_preimage_hash!(nodes[0]);
        check_added_monitors!(nodes[0], 1);
  
        //// Provide one more block to watchtower Bob, expect broadcast of commitment and HTLC-Timeout
 -      watchtower_bob.chain_monitor.block_connected(&header, &vec![], CHAN_CONFIRM_DEPTH + 1 + TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS);
 +      let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
 +      watchtower_bob.chain_monitor.block_connected(&Block { header, txdata: vec![] }, CHAN_CONFIRM_DEPTH + 1 + TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS);
  
        // Watchtower Bob should have broadcast a commitment/HTLC-timeout
        let bob_state_y;
        };
  
        // We confirm Bob's state Y on Alice, she should broadcast a HTLC-timeout
 -      watchtower_alice.chain_monitor.block_connected(&header, &vec![(0, &bob_state_y)], CHAN_CONFIRM_DEPTH + 2 + TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS);
 +      let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
 +      watchtower_alice.chain_monitor.block_connected(&Block { header, txdata: vec![bob_state_y.clone()] }, CHAN_CONFIRM_DEPTH + 2 + TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS);
        {
                let htlc_txn = chanmon_cfgs[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
                // We broadcast twice the transaction, once due to the HTLC-timeout, once due
@@@ -8828,3 -8825,53 +8828,53 @@@ fn test_error_chans_closed() 
        assert_eq!(nodes[0].node.list_usable_channels().len(), 1);
        assert!(nodes[0].node.list_usable_channels()[0].channel_id == chan_3.2);
  }
+ #[test]
+ fn test_invalid_funding_tx() {
+       // Test that we properly handle invalid funding transactions sent to us from a peer.
+       //
+       // Previously, all other major lightning implementations had failed to properly sanitize
+       // funding transactions from their counterparties, leading to a multi-implementation critical
+       // security vulnerability (though we always sanitized properly, we've previously had
+       // un-released crashes in the sanitization process).
+       let chanmon_cfgs = create_chanmon_cfgs(2);
+       let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+       let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+       let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+       nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100_000, 10_000, 42, None).unwrap();
+       nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(), &get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id()));
+       nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), InitFeatures::known(), &get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id()));
+       let (temporary_channel_id, mut tx, _) = create_funding_transaction(&nodes[0], 100_000, 42);
+       for output in tx.output.iter_mut() {
+               // Make the confirmed funding transaction have a bogus script_pubkey
+               output.script_pubkey = bitcoin::Script::new();
+       }
+       nodes[0].node.funding_transaction_generated_unchecked(&temporary_channel_id, tx.clone(), 0).unwrap();
+       nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id()));
+       check_added_monitors!(nodes[1], 1);
+       nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id()));
+       check_added_monitors!(nodes[0], 1);
+       let events_1 = nodes[0].node.get_and_clear_pending_events();
+       assert_eq!(events_1.len(), 0);
+       assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().len(), 1);
+       assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap()[0], tx);
+       nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clear();
+       confirm_transaction_at(&nodes[1], &tx, 1);
+       check_added_monitors!(nodes[1], 1);
+       let events_2 = nodes[1].node.get_and_clear_pending_msg_events();
+       assert_eq!(events_2.len(), 1);
+       if let MessageSendEvent::HandleError { node_id, action } = &events_2[0] {
+               assert_eq!(*node_id, nodes[0].node.get_our_node_id());
+               if let msgs::ErrorAction::SendErrorMessage { msg } = action {
+                       assert_eq!(msg.data, "funding tx had wrong script/value or output index");
+               } else { panic!(); }
+       } else { panic!(); }
+       assert_eq!(nodes[1].node.list_channels().len(), 0);
+ }