Merge pull request #890 from TheBlueMatt/2021-04-fix-chan-shutdown-crash
[rust-lightning] / lightning / src / ln / channel.rs
index 174aecd3ba036f46dc9acdc95d2b287ef536d812..8a1489d4ef775c7be7d515bcc3153895437eb726 100644 (file)
@@ -7,7 +7,6 @@
 // You may not use this file except in accordance with one or both of these
 // licenses.
 
-use bitcoin::blockdata::block::BlockHeader;
 use bitcoin::blockdata::script::{Script,Builder};
 use bitcoin::blockdata::transaction::{TxIn, TxOut, Transaction, SigHashType};
 use bitcoin::blockdata::opcodes;
@@ -25,7 +24,7 @@ use bitcoin::secp256k1;
 use ln::features::{ChannelFeatures, InitFeatures};
 use ln::msgs;
 use ln::msgs::{DecodeError, OptionalField, DataLossProtect};
-use ln::channelmanager::{PendingHTLCStatus, HTLCSource, HTLCFailReason, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, PaymentPreimage, PaymentHash, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, MAX_LOCAL_BREAKDOWN_TIMEOUT};
+use ln::channelmanager::{BestBlock, PendingHTLCStatus, HTLCSource, HTLCFailReason, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, PaymentPreimage, PaymentHash, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, MAX_LOCAL_BREAKDOWN_TIMEOUT};
 use ln::chan_utils::{CounterpartyCommitmentSecrets, TxCreationKeys, HTLCOutputInCommitment, HTLC_SUCCESS_TX_WEIGHT, HTLC_TIMEOUT_TX_WEIGHT, make_funding_redeemscript, ChannelPublicKeys, CommitmentTransaction, HolderCommitmentTransaction, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, MAX_HTLCS, get_commitment_transaction_number_obscure_factor};
 use ln::chan_utils;
 use chain::chaininterface::{FeeEstimator,ConfirmationTarget};
@@ -251,7 +250,7 @@ pub const INITIAL_COMMITMENT_NUMBER: u64 = (1 << 48) - 1;
 /// Liveness is called to fluctuate given peer disconnecton/monitor failures/closing.
 /// If channel is public, network should have a liveness view announced by us on a
 /// best-effort, which means we may filter out some status transitions to avoid spam.
-/// See further timer_chan_freshness_every_min.
+/// See further timer_tick_occurred.
 #[derive(PartialEq)]
 enum UpdateStatus {
        /// Status has been gossiped.
@@ -376,13 +375,10 @@ pub(super) struct Channel<Signer: Sign> {
 
        last_sent_closing_fee: Option<(u32, u64, Signature)>, // (feerate, fee, holder_sig)
 
-       /// The hash of the block in which the funding transaction reached our CONF_TARGET. We use this
-       /// to detect unconfirmation after a serialize-unserialize roundtrip where we may not see a full
-       /// series of block_connected/block_disconnected calls. Obviously this is not a guarantee as we
-       /// could miss the funding_tx_confirmed_in block as well, but it serves as a useful fallback.
+       /// The hash of the block in which the funding transaction was included.
        funding_tx_confirmed_in: Option<BlockHash>,
+       funding_tx_confirmation_height: u32,
        short_channel_id: Option<u64>,
-       funding_tx_confirmations: u64,
 
        counterparty_dust_limit_satoshis: u64,
        #[cfg(test)]
@@ -441,10 +437,6 @@ struct CommitmentTxInfoCached {
 }
 
 pub const OUR_MAX_HTLCS: u16 = 50; //TODO
-/// Confirmation count threshold at which we close a channel. Ideally we'd keep the channel around
-/// on ice until the funding transaction gets more confirmations, but the LN protocol doesn't
-/// really allow for this, so instead we're stuck closing it out at that point.
-const UNCONF_THRESHOLD: u32 = 6;
 const SPENDING_INPUT_FOR_A_OUTPUT_WEIGHT: u64 = 79; // prevout: 36, nSequence: 4, script len: 1, witness lengths: (3+1)/4, sig: 73/4, if-selector: 1, redeemScript: (6 ops + 2*33 pubkeys + 1*2 delay)/4
 const B_OUTPUT_PLUS_SPENDING_INPUT_WEIGHT: u64 = 104; // prevout: 40, nSequence: 4, script len: 1, witness lengths: 3/4, sig: 73/4, pubkey: 33/4, output: 31 (TODO: Wrong? Useless?)
 
@@ -581,8 +573,8 @@ impl<Signer: Sign> Channel<Signer> {
                        last_sent_closing_fee: None,
 
                        funding_tx_confirmed_in: None,
+                       funding_tx_confirmation_height: 0,
                        short_channel_id: None,
-                       funding_tx_confirmations: 0,
 
                        feerate_per_kw: feerate,
                        counterparty_dust_limit_satoshis: 0,
@@ -819,8 +811,8 @@ impl<Signer: Sign> Channel<Signer> {
                        last_sent_closing_fee: None,
 
                        funding_tx_confirmed_in: None,
+                       funding_tx_confirmation_height: 0,
                        short_channel_id: None,
-                       funding_tx_confirmations: 0,
 
                        feerate_per_kw: msg.feerate_per_kw,
                        channel_value_satoshis: msg.funding_satoshis,
@@ -1539,7 +1531,7 @@ impl<Signer: Sign> Channel<Signer> {
                &self.get_counterparty_pubkeys().funding_pubkey
        }
 
-       pub fn funding_created<L: Deref>(&mut self, msg: &msgs::FundingCreated, last_block_hash: BlockHash, logger: &L) -> Result<(msgs::FundingSigned, ChannelMonitor<Signer>), ChannelError> where L::Target: Logger {
+       pub fn funding_created<L: Deref>(&mut self, msg: &msgs::FundingCreated, best_block: BestBlock, logger: &L) -> Result<(msgs::FundingSigned, ChannelMonitor<Signer>), ChannelError> where L::Target: Logger {
                if self.is_outbound() {
                        return Err(ChannelError::Close("Received funding_created for an outbound channel?".to_owned()));
                }
@@ -1593,7 +1585,7 @@ impl<Signer: Sign> Channel<Signer> {
                                                          &self.channel_transaction_parameters,
                                                          funding_redeemscript.clone(), self.channel_value_satoshis,
                                                          obscure_factor,
-                                                         holder_commitment_tx, last_block_hash);
+                                                         holder_commitment_tx, best_block);
 
                channel_monitor.provide_latest_counterparty_commitment_tx(counterparty_initial_commitment_txid, Vec::new(), self.cur_counterparty_commitment_transaction_number, self.counterparty_cur_commitment_point.unwrap(), logger);
 
@@ -1610,7 +1602,7 @@ impl<Signer: Sign> Channel<Signer> {
 
        /// Handles a funding_signed message from the remote end.
        /// If this call is successful, broadcast the funding transaction (and not before!)
-       pub fn funding_signed<L: Deref>(&mut self, msg: &msgs::FundingSigned, last_block_hash: BlockHash, logger: &L) -> Result<(ChannelMonitor<Signer>, Transaction), ChannelError> where L::Target: Logger {
+       pub fn funding_signed<L: Deref>(&mut self, msg: &msgs::FundingSigned, best_block: BestBlock, logger: &L) -> Result<(ChannelMonitor<Signer>, Transaction), ChannelError> where L::Target: Logger {
                if !self.is_outbound() {
                        return Err(ChannelError::Close("Received funding_signed for an inbound channel?".to_owned()));
                }
@@ -1663,7 +1655,7 @@ impl<Signer: Sign> Channel<Signer> {
                                                          &self.channel_transaction_parameters,
                                                          funding_redeemscript.clone(), self.channel_value_satoshis,
                                                          obscure_factor,
-                                                         holder_commitment_tx, last_block_hash);
+                                                         holder_commitment_tx, best_block);
 
                channel_monitor.provide_latest_counterparty_commitment_tx(counterparty_initial_bitcoin_tx.txid, Vec::new(), self.cur_counterparty_commitment_transaction_number, self.counterparty_cur_commitment_point.unwrap(), logger);
 
@@ -3512,26 +3504,139 @@ impl<Signer: Sign> Channel<Signer> {
                self.network_sync == UpdateStatus::DisabledMarked
        }
 
-       /// When we receive a new block, we (a) check whether the block contains the funding
-       /// transaction (which would start us counting blocks until we send the funding_signed), and
-       /// (b) check the height of the block against outbound holding cell HTLCs in case we need to
-       /// give up on them prematurely and time them out. Everything else (e.g. commitment
-       /// transaction broadcasts, channel closure detection, HTLC transaction broadcasting, etc) is
+       fn check_get_funding_locked(&mut self, height: u32) -> Option<msgs::FundingLocked> {
+               if self.funding_tx_confirmation_height == 0 {
+                       return None;
+               }
+
+               let funding_tx_confirmations = height as i64 - self.funding_tx_confirmation_height as i64 + 1;
+               if funding_tx_confirmations <= 0 {
+                       self.funding_tx_confirmation_height = 0;
+               }
+
+               if funding_tx_confirmations < self.minimum_depth as i64 {
+                       return None;
+               }
+
+               let non_shutdown_state = self.channel_state & (!MULTI_STATE_FLAGS);
+               let need_commitment_update = if non_shutdown_state == ChannelState::FundingSent as u32 {
+                       self.channel_state |= ChannelState::OurFundingLocked as u32;
+                       true
+               } else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::TheirFundingLocked as u32) {
+                       self.channel_state = ChannelState::ChannelFunded as u32 | (self.channel_state & MULTI_STATE_FLAGS);
+                       self.update_time_counter += 1;
+                       true
+               } else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::OurFundingLocked as u32) {
+                       // We got a reorg but not enough to trigger a force close, just ignore.
+                       false
+               } else if self.channel_state < ChannelState::ChannelFunded as u32 {
+                       panic!("Started confirming a channel in a state pre-FundingSent?: {}", self.channel_state);
+               } else {
+                       // We got a reorg but not enough to trigger a force close, just ignore.
+                       false
+               };
+
+               if need_commitment_update {
+                       if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 {
+                               let next_per_commitment_point = self.holder_signer.get_per_commitment_point(self.cur_holder_commitment_transaction_number, &self.secp_ctx);
+                               return Some(msgs::FundingLocked {
+                                       channel_id: self.channel_id,
+                                       next_per_commitment_point,
+                               });
+                       } else {
+                               self.monitor_pending_funding_locked = true;
+                       }
+               }
+               None
+       }
+
+       /// When a transaction is confirmed, we check whether it is or spends the funding transaction
+       /// In the first case, we store the confirmation height and calculating the short channel id.
+       /// In the second, we simply return an Err indicating we need to be force-closed now.
+       pub fn transactions_confirmed<L: Deref>(&mut self, block_hash: &BlockHash, height: u32, txdata: &TransactionData, logger: &L)
+                       -> Result<Option<msgs::FundingLocked>, msgs::ErrorMessage> where L::Target: Logger {
+               let non_shutdown_state = self.channel_state & (!MULTI_STATE_FLAGS);
+               for &(index_in_block, tx) in txdata.iter() {
+                       if let Some(funding_txo) = self.get_funding_txo() {
+                               // If we haven't yet sent a funding_locked, but are in FundingSent (ignoring
+                               // whether they've sent a funding_locked or not), check if we should send one.
+                               if non_shutdown_state & !(ChannelState::TheirFundingLocked as u32) == ChannelState::FundingSent as u32 {
+                                       if tx.txid() == funding_txo.txid {
+                                               let txo_idx = funding_txo.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 {
+                                                       if self.is_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.update_time_counter += 1;
+                                                       return Err(msgs::ErrorMessage {
+                                                               channel_id: self.channel_id(),
+                                                               data: "funding tx had wrong script/value or output index".to_owned()
+                                                       });
+                                               } else {
+                                                       if self.is_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_confirmation_height = height;
+                                                       self.funding_tx_confirmed_in = Some(*block_hash);
+                                                       self.short_channel_id = match scid_from_parts(height as u64, index_in_block as u64, txo_idx as u64) {
+                                                               Ok(scid) => Some(scid),
+                                                               Err(_) => panic!("Block was bogus - either height was > 16 million, had > 16 million transactions, or had > 65k outputs"),
+                                                       }
+                                               }
+                                       }
+                                       // If we allow 1-conf funding, we may need to check for funding_locked here and
+                                       // 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));
+                                       }
+                               }
+                               for inp in tx.input.iter() {
+                                       if inp.previous_output == funding_txo.into_bitcoin_outpoint() {
+                                               log_trace!(logger, "Detected channel-closing tx {} spending {}:{}, closing channel {}", tx.txid(), inp.previous_output.txid, inp.previous_output.vout, log_bytes!(self.channel_id()));
+                                               return Err(msgs::ErrorMessage {
+                                                       channel_id: self.channel_id(),
+                                                       data: "Commitment or closing transaction was confirmed on chain.".to_owned()
+                                               });
+                                       }
+                               }
+                       }
+               }
+               Ok(None)
+       }
+
+       /// When a new block is connected, we check the height of the block against outbound holding
+       /// cell HTLCs in case we need to give up on them prematurely and time them out. Everything
+       /// else (e.g. commitment transaction broadcasts, HTLC transaction broadcasting, etc) is
        /// handled by the ChannelMonitor.
        ///
        /// If we return Err, the channel may have been closed, at which point the standard
        /// requirements apply - no calls may be made except those explicitly stated to be allowed
        /// post-shutdown.
-       /// Only returns an ErrorAction of DisconnectPeer, if Err.
        ///
        /// May return some HTLCs (and their payment_hash) which have timed out and should be failed
        /// back.
-       pub fn block_connected(&mut self, header: &BlockHeader, txdata: &TransactionData, height: 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| {
                        match htlc_update {
                                &HTLCUpdateAwaitingACK::AddHTLC { ref payment_hash, ref source, ref cltv_expiry, .. } => {
-                                       if *cltv_expiry <= height + HTLC_FAIL_BACK_BUFFER {
+                                       if *cltv_expiry <= unforwarded_htlc_cltv_limit {
                                                timed_out_htlcs.push((source.clone(), payment_hash.clone()));
                                                false
                                        } else { true }
@@ -3540,112 +3645,62 @@ impl<Signer: Sign> Channel<Signer> {
                        }
                });
 
-               if self.funding_tx_confirmations > 0 {
-                       self.funding_tx_confirmations += 1;
+               self.update_time_counter = cmp::max(self.update_time_counter, highest_header_time);
+
+               if let Some(funding_locked) = self.check_get_funding_locked(height) {
+                       return Ok((Some(funding_locked), timed_out_htlcs));
                }
 
                let non_shutdown_state = self.channel_state & (!MULTI_STATE_FLAGS);
-               if non_shutdown_state & !(ChannelState::TheirFundingLocked as u32) == ChannelState::FundingSent as u32 {
-                       for &(index_in_block, tx) in txdata.iter() {
-                               let funding_txo = self.get_funding_txo().unwrap();
-                               if tx.txid() == funding_txo.txid {
-                                       let txo_idx = funding_txo.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 {
-                                               if self.is_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.update_time_counter += 1;
-                                               return Err(msgs::ErrorMessage {
-                                                       channel_id: self.channel_id(),
-                                                       data: "funding tx had wrong script/value".to_owned()
-                                               });
-                                       } else {
-                                               if self.is_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 = match scid_from_parts(height as u64, index_in_block as u64, txo_idx as u64) {
-                                                       Ok(scid) => Some(scid),
-                                                       Err(_) => panic!("Block was bogus - either height was > 16 million, had > 16 million transactions, or had > 65k outputs"),
-                                               }
-                                       }
-                               }
+               if non_shutdown_state >= ChannelState::ChannelFunded as u32 ||
+                  (non_shutdown_state & ChannelState::OurFundingLocked as u32) == ChannelState::OurFundingLocked as u32 {
+                       let mut funding_tx_confirmations = height as i64 - self.funding_tx_confirmation_height as i64 + 1;
+                       if self.funding_tx_confirmation_height == 0 {
+                               // Note that check_get_funding_locked may reset funding_tx_confirmation_height to
+                               // zero if it has been reorged out, however in either case, our state flags
+                               // indicate we've already sent a funding_locked
+                               funding_tx_confirmations = 0;
                        }
-               }
 
-               self.update_time_counter = cmp::max(self.update_time_counter, header.time);
-               if self.funding_tx_confirmations > 0 {
-                       if self.funding_tx_confirmations == self.minimum_depth as u64 {
-                               let need_commitment_update = if non_shutdown_state == ChannelState::FundingSent as u32 {
-                                       self.channel_state |= ChannelState::OurFundingLocked as u32;
-                                       true
-                               } else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::TheirFundingLocked as u32) {
-                                       self.channel_state = ChannelState::ChannelFunded as u32 | (self.channel_state & MULTI_STATE_FLAGS);
-                                       self.update_time_counter += 1;
-                                       true
-                               } else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::OurFundingLocked as u32) {
-                                       // We got a reorg but not enough to trigger a force close, just update
-                                       // funding_tx_confirmed_in and return.
-                                       false
-                               } else if self.channel_state < ChannelState::ChannelFunded as u32 {
-                                       panic!("Started confirming a channel in a state pre-FundingSent?: {}", self.channel_state);
-                               } else {
-                                       // We got a reorg but not enough to trigger a force close, just update
-                                       // funding_tx_confirmed_in and return.
-                                       false
-                               };
-                               self.funding_tx_confirmed_in = Some(header.block_hash());
-
-                               //TODO: Note that this must be a duplicate of the previous commitment point they sent us,
-                               //as otherwise we will have a commitment transaction that they can't revoke (well, kinda,
-                               //they can by sending two revoke_and_acks back-to-back, but not really). This appears to be
-                               //a protocol oversight, but I assume I'm just missing something.
-                               if need_commitment_update {
-                                       if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 {
-                                               let next_per_commitment_point = self.holder_signer.get_per_commitment_point(self.cur_holder_commitment_transaction_number, &self.secp_ctx);
-                                               return Ok((Some(msgs::FundingLocked {
-                                                       channel_id: self.channel_id,
-                                                       next_per_commitment_point,
-                                               }), timed_out_htlcs));
-                                       } else {
-                                               self.monitor_pending_funding_locked = true;
-                                               return Ok((None, timed_out_htlcs));
-                                       }
-                               }
+                       // If we've sent funding_locked (or have both sent and received funding_locked), and
+                       // the funding transaction's confirmation count has dipped below minimum_depth / 2,
+                       // close the channel and hope we can get the latest state on chain (because presumably
+                       // the funding transaction is at least still in the mempool of most nodes).
+                       if funding_tx_confirmations < self.minimum_depth as i64 / 2 {
+                               return Err(msgs::ErrorMessage {
+                                       channel_id: self.channel_id(),
+                                       data: format!("Funding transaction was un-confirmed. Locked at {} confs, now have {} confs.", self.minimum_depth, funding_tx_confirmations),
+                               });
                        }
                }
+
                Ok((None, timed_out_htlcs))
        }
 
-       /// Called by channelmanager based on chain blocks being disconnected.
-       /// Returns true if we need to close the channel now due to funding transaction
-       /// unconfirmation/reorg.
-       pub fn block_disconnected(&mut self, header: &BlockHeader) -> bool {
-               if self.funding_tx_confirmations > 0 {
-                       self.funding_tx_confirmations -= 1;
-                       if self.funding_tx_confirmations == UNCONF_THRESHOLD as u64 {
-                               return true;
+       /// Indicates the funding transaction is no longer confirmed in the main chain. This may
+       /// force-close the channel, but may also indicate a harmless reorganization of a block or two
+       /// 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 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.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?");
+                                       Ok(())
+                               },
+                               Err(e) => Err(e)
                        }
+               } else {
+                       // We never learned about the funding confirmation anyway, just ignore
+                       Ok(())
                }
-               if Some(header.block_hash()) == self.funding_tx_confirmed_in {
-                       self.funding_tx_confirmations = self.minimum_depth as u64 - 1;
-               }
-               false
        }
 
        // Methods to get unprompted messages to send to the remote end (or where we already returned
@@ -4470,8 +4525,8 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
                }
 
                self.funding_tx_confirmed_in.write(writer)?;
+               self.funding_tx_confirmation_height.write(writer)?;
                self.short_channel_id.write(writer)?;
-               self.funding_tx_confirmations.write(writer)?;
 
                self.counterparty_dust_limit_satoshis.write(writer)?;
                self.holder_dust_limit_satoshis.write(writer)?;
@@ -4641,8 +4696,8 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
                };
 
                let funding_tx_confirmed_in = Readable::read(reader)?;
+               let funding_tx_confirmation_height = Readable::read(reader)?;
                let short_channel_id = Readable::read(reader)?;
-               let funding_tx_confirmations = Readable::read(reader)?;
 
                let counterparty_dust_limit_satoshis = Readable::read(reader)?;
                let holder_dust_limit_satoshis = Readable::read(reader)?;
@@ -4723,8 +4778,8 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
                        last_sent_closing_fee,
 
                        funding_tx_confirmed_in,
+                       funding_tx_confirmation_height,
                        short_channel_id,
-                       funding_tx_confirmations,
 
                        counterparty_dust_limit_satoshis,
                        holder_dust_limit_satoshis,
@@ -4769,15 +4824,15 @@ mod tests {
        use bitcoin::network::constants::Network;
        use bitcoin::hashes::hex::FromHex;
        use hex;
-       use ln::channelmanager::{HTLCSource, PaymentPreimage, PaymentHash};
-       use ln::channel::{Channel,Sign,InboundHTLCOutput,OutboundHTLCOutput,InboundHTLCState,OutboundHTLCState,HTLCOutputInCommitment,HTLCCandidate,HTLCInitiator,TxCreationKeys};
+       use ln::channelmanager::{BestBlock, HTLCSource, PaymentPreimage, PaymentHash};
+       use ln::channel::{Channel,InboundHTLCOutput,OutboundHTLCOutput,InboundHTLCState,OutboundHTLCState,HTLCOutputInCommitment,HTLCCandidate,HTLCInitiator,TxCreationKeys};
        use ln::channel::MAX_FUNDING_SATOSHIS;
        use ln::features::InitFeatures;
        use ln::msgs::{ChannelUpdate, DataLossProtect, DecodeError, OptionalField, UnsignedChannelUpdate};
        use ln::chan_utils;
        use ln::chan_utils::{ChannelPublicKeys, HolderCommitmentTransaction, CounterpartyChannelTransactionParameters, HTLC_SUCCESS_TX_WEIGHT, HTLC_TIMEOUT_TX_WEIGHT};
        use chain::chaininterface::{FeeEstimator,ConfirmationTarget};
-       use chain::keysinterface::{InMemorySigner, KeysInterface};
+       use chain::keysinterface::{InMemorySigner, KeysInterface, BaseSign};
        use chain::transaction::OutPoint;
        use util::config::UserConfig;
        use util::enforcing_trait_impls::EnforcingSigner;
@@ -4982,8 +5037,8 @@ mod tests {
                let secp_ctx = Secp256k1::new();
                let seed = [42; 32];
                let network = Network::Testnet;
-               let chain_hash = genesis_block(network).header.block_hash();
-               let last_block_hash = chain_hash;
+               let best_block = BestBlock::from_genesis(network);
+               let chain_hash = best_block.block_hash();
                let keys_provider = test_utils::TestKeysInterface::new(&seed, network);
 
                // Go through the flow of opening a channel between two nodes.
@@ -5009,10 +5064,10 @@ mod tests {
                }]};
                let funding_outpoint = OutPoint{ txid: tx.txid(), index: 0 };
                let funding_created_msg = node_a_chan.get_outbound_funding_created(tx.clone(), funding_outpoint, &&logger).unwrap();
-               let (funding_signed_msg, _) = node_b_chan.funding_created(&funding_created_msg, last_block_hash, &&logger).unwrap();
+               let (funding_signed_msg, _) = node_b_chan.funding_created(&funding_created_msg, best_block, &&logger).unwrap();
 
                // Node B --> Node A: funding signed
-               let _ = node_a_chan.funding_signed(&funding_signed_msg, last_block_hash, &&logger);
+               let _ = node_a_chan.funding_signed(&funding_signed_msg, best_block, &&logger);
 
                // Now disconnect the two nodes and check that the commitment point in
                // Node B's channel_reestablish message is sane.