Merge pull request #374 from dongcarl/2019-08-channel-open-sanity
[rust-lightning] / src / ln / channel.rs
index 4d790cfc91e422433d1cc213f9dcc17292e62180..04a65b361e3cf8cfdbbf3fab118ecc8985ed6b53 100644 (file)
@@ -16,9 +16,9 @@ use secp256k1::{Secp256k1,Signature};
 use secp256k1;
 
 use ln::msgs;
-use ln::msgs::{DecodeError, OptionalField, LocalFeatures};
+use ln::msgs::{DecodeError, OptionalField, LocalFeatures, DataLossProtect};
 use ln::channelmonitor::ChannelMonitor;
-use ln::channelmanager::{PendingHTLCStatus, HTLCSource, HTLCFailReason, HTLCFailureMsg, PendingForwardHTLCInfo, RAACommitmentOrder, PaymentPreimage, PaymentHash};
+use ln::channelmanager::{PendingHTLCStatus, HTLCSource, HTLCFailReason, HTLCFailureMsg, PendingForwardHTLCInfo, RAACommitmentOrder, PaymentPreimage, PaymentHash, BREAKDOWN_TIMEOUT, MAX_LOCAL_BREAKDOWN_TIMEOUT};
 use ln::chan_utils::{TxCreationKeys,HTLCOutputInCommitment,HTLC_SUCCESS_TX_WEIGHT,HTLC_TIMEOUT_TX_WEIGHT};
 use ln::chan_utils;
 use chain::chaininterface::{FeeEstimator,ConfirmationTarget};
@@ -32,7 +32,7 @@ use util::config::{UserConfig,ChannelConfig};
 
 use std;
 use std::default::Default;
-use std::{cmp,mem};
+use std::{cmp,mem,fmt};
 use std::sync::{Arc};
 
 #[cfg(test)]
@@ -181,9 +181,9 @@ enum ChannelState {
        /// "disconnected" and no updates are allowed until after we've done a channel_reestablish
        /// dance.
        PeerDisconnected = (1 << 7),
-       /// Flag which is set on ChannelFunded and FundingSent indicating the user has told us they
-       /// failed to update our ChannelMonitor somewhere and we should pause sending any outbound
-       /// messages until they've managed to do so.
+       /// Flag which is set on ChannelFunded, FundingCreated, and FundingSent indicating the user has
+       /// told us they failed to update our ChannelMonitor somewhere and we should pause sending any
+       /// outbound messages until they've managed to do so.
        MonitorUpdateFailed = (1 << 8),
        /// Flag which implies that we have sent a commitment_signed but are awaiting the responding
        /// revoke_and_ack message. During this time period, we can't generate new commitment_signed
@@ -248,6 +248,7 @@ pub(super) struct Channel {
        /// send it first.
        resend_order: RAACommitmentOrder,
 
+       monitor_pending_funding_locked: bool,
        monitor_pending_revoke_and_ack: bool,
        monitor_pending_commitment_signed: bool,
        monitor_pending_forwards: Vec<(PendingForwardHTLCInfo, u64)>,
@@ -317,7 +318,7 @@ pub(super) struct Channel {
        their_htlc_minimum_msat: u64,
        our_htlc_minimum_msat: u64,
        their_to_self_delay: u16,
-       //implied by BREAKDOWN_TIMEOUT: our_to_self_delay: u16,
+       our_to_self_delay: u16,
        #[cfg(test)]
        pub their_max_accepted_htlcs: u16,
        #[cfg(not(test))]
@@ -347,14 +348,6 @@ pub const OUR_MAX_HTLCS: u16 = 50; //TODO
 /// 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;
-/// The amount of time we require our counterparty wait to claim their money (ie time between when
-/// we, or our watchtower, must check for them having broadcast a theft transaction).
-#[cfg(not(test))]
-const BREAKDOWN_TIMEOUT: u16 = 6 * 24 * 7; //TODO?
-#[cfg(test)]
-pub const BREAKDOWN_TIMEOUT: u16 = 6 * 24 * 7; //TODO?
-/// The amount of time we're willing to wait to claim money back to us
-const MAX_LOCAL_BREAKDOWN_TIMEOUT: u16 = 6 * 24 * 14;
 /// Exposing these two constants for use in test in ChannelMonitor
 pub const COMMITMENT_TX_BASE_WEIGHT: u64 = 724;
 pub const COMMITMENT_TX_WEIGHT_PER_HTLC: u64 = 172;
@@ -373,10 +366,23 @@ pub const OFFERED_HTLC_SCRIPT_WEIGHT: usize = 133;
 /// Used to return a simple Error back to ChannelManager. Will get converted to a
 /// msgs::ErrorAction::SendErrorMessage or msgs::ErrorAction::IgnoreError as appropriate with our
 /// channel_id in ChannelManager.
-#[derive(Debug)]
 pub(super) enum ChannelError {
        Ignore(&'static str),
        Close(&'static str),
+       CloseDelayBroadcast {
+               msg: &'static str,
+               update: Option<ChannelMonitor>
+       },
+}
+
+impl fmt::Debug for ChannelError {
+       fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+               match self {
+                       &ChannelError::Ignore(e) => write!(f, "Ignore : {}", e),
+                       &ChannelError::Close(e) => write!(f, "Close : {}", e),
+                       &ChannelError::CloseDelayBroadcast { msg, .. } => write!(f, "CloseDelayBroadcast : {}", msg)
+               }
+       }
 }
 
 macro_rules! secp_check {
@@ -397,7 +403,7 @@ impl Channel {
        /// Returns a minimum channel reserve value **they** need to maintain
        ///
        /// Guaranteed to return a value no larger than channel_value_satoshis
-       fn get_our_channel_reserve_satoshis(channel_value_satoshis: u64) -> u64 {
+       pub(crate) fn get_our_channel_reserve_satoshis(channel_value_satoshis: u64) -> u64 {
                let (q, _) = channel_value_satoshis.overflowing_div(100);
                cmp::min(channel_value_satoshis, cmp::max(q, 1000)) //TODO
        }
@@ -421,6 +427,9 @@ impl Channel {
                if push_msat > channel_value_satoshis * 1000 {
                        return Err(APIError::APIMisuseError{err: "push value > channel value"});
                }
+               if config.own_channel_config.our_to_self_delay < BREAKDOWN_TIMEOUT {
+                       return Err(APIError::APIMisuseError{err: "Configured with an unreasonable our_to_self_delay putting user funds at risks"});
+               }
 
 
                let background_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
@@ -432,7 +441,7 @@ impl Channel {
 
                let secp_ctx = Secp256k1::new();
                let channel_monitor = ChannelMonitor::new(&chan_keys.revocation_base_key, &chan_keys.delayed_payment_base_key,
-                                                         &chan_keys.htlc_base_key, &chan_keys.payment_base_key, &keys_provider.get_shutdown_pubkey(), BREAKDOWN_TIMEOUT,
+                                                         &chan_keys.htlc_base_key, &chan_keys.payment_base_key, &keys_provider.get_shutdown_pubkey(), config.own_channel_config.our_to_self_delay,
                                                          keys_provider.get_destination_script(), logger.clone());
 
                Ok(Channel {
@@ -462,6 +471,7 @@ impl Channel {
 
                        resend_order: RAACommitmentOrder::CommitmentFirst,
 
+                       monitor_pending_funding_locked: false,
                        monitor_pending_revoke_and_ack: false,
                        monitor_pending_commitment_signed: false,
                        monitor_pending_forwards: Vec::new(),
@@ -489,6 +499,7 @@ impl Channel {
                        their_htlc_minimum_msat: 0,
                        our_htlc_minimum_msat: Channel::derive_our_htlc_minimum_msat(feerate),
                        their_to_self_delay: 0,
+                       our_to_self_delay: config.own_channel_config.our_to_self_delay,
                        their_max_accepted_htlcs: 0,
                        minimum_depth: 0, // Filled in in accept_channel
 
@@ -526,6 +537,10 @@ impl Channel {
                let chan_keys = keys_provider.get_channel_keys(true);
                let mut local_config = (*config).channel_options.clone();
 
+               if config.own_channel_config.our_to_self_delay < BREAKDOWN_TIMEOUT {
+                       return Err(ChannelError::Close("Configured with an unreasonable our_to_self_delay putting user funds at risks"));
+               }
+
                // Check sanity of message fields:
                if msg.funding_satoshis >= MAX_FUNDING_SATOSHIS {
                        return Err(ChannelError::Close("funding value > 2^24"));
@@ -547,7 +562,7 @@ impl Channel {
                }
                Channel::check_remote_fee(fee_estimator, msg.feerate_per_kw)?;
 
-               if msg.to_self_delay > MAX_LOCAL_BREAKDOWN_TIMEOUT {
+               if msg.to_self_delay > config.peer_channel_config_limits.their_to_self_delay || msg.to_self_delay > MAX_LOCAL_BREAKDOWN_TIMEOUT {
                        return Err(ChannelError::Close("They wanted our payments to be delayed by a needlessly long period"));
                }
                if msg.max_accepted_htlcs < 1 {
@@ -620,7 +635,7 @@ impl Channel {
 
                let secp_ctx = Secp256k1::new();
                let mut channel_monitor = ChannelMonitor::new(&chan_keys.revocation_base_key, &chan_keys.delayed_payment_base_key,
-                                                             &chan_keys.htlc_base_key, &chan_keys.payment_base_key, &keys_provider.get_shutdown_pubkey(), BREAKDOWN_TIMEOUT,
+                                                             &chan_keys.htlc_base_key, &chan_keys.payment_base_key, &keys_provider.get_shutdown_pubkey(), config.own_channel_config.our_to_self_delay,
                                                              keys_provider.get_destination_script(), logger.clone());
                channel_monitor.set_their_base_keys(&msg.htlc_basepoint, &msg.delayed_payment_basepoint);
                channel_monitor.set_their_to_self_delay(msg.to_self_delay);
@@ -672,6 +687,7 @@ impl Channel {
 
                        resend_order: RAACommitmentOrder::CommitmentFirst,
 
+                       monitor_pending_funding_locked: false,
                        monitor_pending_revoke_and_ack: false,
                        monitor_pending_commitment_signed: false,
                        monitor_pending_forwards: Vec::new(),
@@ -700,6 +716,7 @@ impl Channel {
                        their_htlc_minimum_msat: msg.htlc_minimum_msat,
                        our_htlc_minimum_msat: Channel::derive_our_htlc_minimum_msat(msg.feerate_per_kw as u64),
                        their_to_self_delay: msg.to_self_delay,
+                       our_to_self_delay: config.own_channel_config.our_to_self_delay,
                        their_max_accepted_htlcs: msg.max_accepted_htlcs,
                        minimum_depth: config.own_channel_config.minimum_depth,
 
@@ -908,8 +925,6 @@ impl Channel {
                {
                        // Make sure that the to_self/to_remote is always either past the appropriate
                        // channel_reserve *or* it is making progress towards it.
-                       // TODO: This should happen after fee calculation, but we don't handle that correctly
-                       // yet!
                        let mut max_commitment_tx_output = if generated_by_local {
                                self.max_commitment_tx_output_local.lock().unwrap()
                        } else {
@@ -935,7 +950,7 @@ impl Channel {
                        log_trace!(self, "   ...including {} output with value {}", if local { "to_local" } else { "to_remote" }, value_to_a);
                        txouts.push((TxOut {
                                script_pubkey: chan_utils::get_revokeable_redeemscript(&keys.revocation_key,
-                                                                                      if local { self.their_to_self_delay } else { BREAKDOWN_TIMEOUT },
+                                                                                      if local { self.their_to_self_delay } else { self.our_to_self_delay },
                                                                                       &keys.a_delayed_payment_key).to_v0_p2wsh(),
                                value: value_to_a as u64
                        }, None));
@@ -1134,7 +1149,7 @@ impl Channel {
        /// @local is used only to convert relevant internal structures which refer to remote vs local
        /// to decide value of outputs and direction of HTLCs.
        fn build_htlc_transaction(&self, prev_hash: &Sha256dHash, htlc: &HTLCOutputInCommitment, local: bool, keys: &TxCreationKeys, feerate_per_kw: u64) -> Transaction {
-               chan_utils::build_htlc_transaction(prev_hash, feerate_per_kw, if local { self.their_to_self_delay } else { BREAKDOWN_TIMEOUT }, htlc, &keys.a_delayed_payment_key, &keys.revocation_key)
+               chan_utils::build_htlc_transaction(prev_hash, feerate_per_kw, if local { self.their_to_self_delay } else { self.our_to_self_delay }, htlc, &keys.a_delayed_payment_key, &keys.revocation_key)
        }
 
        fn create_htlc_tx_signature(&self, tx: &Transaction, htlc: &HTLCOutputInCommitment, keys: &TxCreationKeys) -> Result<(Script, Signature, bool), ChannelError> {
@@ -1388,7 +1403,7 @@ impl Channel {
                if msg.htlc_minimum_msat >= (self.channel_value_satoshis - msg.channel_reserve_satoshis) * 1000 {
                        return Err(ChannelError::Close("Minimum htlc value is full channel value"));
                }
-               if msg.to_self_delay > MAX_LOCAL_BREAKDOWN_TIMEOUT {
+               if msg.to_self_delay > config.peer_channel_config_limits.their_to_self_delay || msg.to_self_delay > MAX_LOCAL_BREAKDOWN_TIMEOUT {
                        return Err(ChannelError::Close("They wanted our payments to be delayed by a needlessly long period"));
                }
                if msg.max_accepted_htlcs < 1 {
@@ -1539,7 +1554,7 @@ impl Channel {
                if !self.channel_outbound {
                        return Err(ChannelError::Close("Received funding_signed for an inbound channel?"));
                }
-               if self.channel_state != ChannelState::FundingCreated as u32 {
+               if self.channel_state & !(ChannelState::MonitorUpdateFailed as u32) != ChannelState::FundingCreated as u32 {
                        return Err(ChannelError::Close("Received funding_signed in strange state!"));
                }
                if self.channel_monitor.get_min_seen_secret() != (1 << 48) ||
@@ -1560,10 +1575,14 @@ impl Channel {
                self.sign_commitment_transaction(&mut local_initial_commitment_tx, &msg.signature);
                self.channel_monitor.provide_latest_local_commitment_tx_info(local_initial_commitment_tx.clone(), local_keys, self.feerate_per_kw, Vec::new());
                self.last_local_commitment_txn = vec![local_initial_commitment_tx];
-               self.channel_state = ChannelState::FundingSent as u32;
+               self.channel_state = ChannelState::FundingSent as u32 | (self.channel_state & (ChannelState::MonitorUpdateFailed as u32));
                self.cur_local_commitment_transaction_number -= 1;
 
-               Ok(self.channel_monitor.clone())
+               if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 {
+                       Ok(self.channel_monitor.clone())
+               } else {
+                       Err(ChannelError::Ignore("Previous monitor update failure prevented funding_signed from allowing funding broadcast"))
+               }
        }
 
        pub fn funding_locked(&mut self, msg: &msgs::FundingLocked) -> Result<(), ChannelError> {
@@ -1578,10 +1597,13 @@ impl Channel {
                } else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::OurFundingLocked as u32) {
                        self.channel_state = ChannelState::ChannelFunded as u32 | (self.channel_state & MULTI_STATE_FLAGS);
                        self.channel_update_count += 1;
-               } else if self.channel_state & (ChannelState::ChannelFunded as u32) != 0 &&
-                               // Note that funding_signed/funding_created will have decremented both by 1!
-                               self.cur_local_commitment_transaction_number == INITIAL_COMMITMENT_NUMBER - 1 &&
-                               self.cur_remote_commitment_transaction_number == INITIAL_COMMITMENT_NUMBER - 1 {
+               } else if (self.channel_state & (ChannelState::ChannelFunded as u32) != 0 &&
+                                // Note that funding_signed/funding_created will have decremented both by 1!
+                                self.cur_local_commitment_transaction_number == INITIAL_COMMITMENT_NUMBER - 1 &&
+                                self.cur_remote_commitment_transaction_number == INITIAL_COMMITMENT_NUMBER - 1) ||
+                               // If we reconnected before sending our funding locked they may still resend theirs:
+                               (self.channel_state & (ChannelState::FundingSent as u32 | ChannelState::TheirFundingLocked as u32) ==
+                                                     (ChannelState::FundingSent as u32 | ChannelState::TheirFundingLocked as u32)) {
                        if self.their_cur_commitment_point != Some(msg.next_per_commitment_point) {
                                return Err(ChannelError::Close("Peer sent a reconnect funding_locked with a different point"));
                        }
@@ -1652,10 +1674,9 @@ impl Channel {
                if inbound_htlc_count + 1 > OUR_MAX_HTLCS as u32 {
                        return Err(ChannelError::Close("Remote tried to push more than our max accepted HTLCs"));
                }
-               //TODO: Spec is unclear if this is per-direction or in total (I assume per direction):
                // Check our_max_htlc_value_in_flight_msat
                if htlc_inbound_value_msat + msg.amount_msat > Channel::get_our_max_htlc_value_in_flight_msat(self.channel_value_satoshis) {
-                       return Err(ChannelError::Close("Remote HTLC add would put them over their max HTLC value in flight"));
+                       return Err(ChannelError::Close("Remote HTLC add would put them over our max HTLC value"));
                }
                // Check our_channel_reserve_satoshis (we're getting paid, so they have to at least meet
                // the reserve_satoshis we told them to always have as direct payment so that they lose
@@ -2344,10 +2365,29 @@ impl Channel {
        /// Indicates that the latest ChannelMonitor update has been committed by the client
        /// successfully and we should restore normal operation. Returns messages which should be sent
        /// to the remote side.
-       pub fn monitor_updating_restored(&mut self) -> (Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, RAACommitmentOrder, Vec<(PendingForwardHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>) {
+       pub fn monitor_updating_restored(&mut self) -> (Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, RAACommitmentOrder, Vec<(PendingForwardHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, bool, Option<msgs::FundingLocked>) {
                assert_eq!(self.channel_state & ChannelState::MonitorUpdateFailed as u32, ChannelState::MonitorUpdateFailed as u32);
                self.channel_state &= !(ChannelState::MonitorUpdateFailed as u32);
 
+               let needs_broadcast_safe = self.channel_state & (ChannelState::FundingSent as u32) != 0 && self.channel_outbound;
+
+               // Because we will never generate a FundingBroadcastSafe event when we're in
+               // MonitorUpdateFailed, if we assume the user only broadcast the funding transaction when
+               // they received the FundingBroadcastSafe event, we can only ever hit
+               // monitor_pending_funding_locked when we're an inbound channel which failed to persist the
+               // monitor on funding_created, and we even got the funding transaction confirmed before the
+               // monitor was persisted.
+               let funding_locked = if self.monitor_pending_funding_locked {
+                       assert!(!self.channel_outbound, "Funding transaction broadcast without FundingBroadcastSafe!");
+                       self.monitor_pending_funding_locked = false;
+                       let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
+                       let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
+                       Some(msgs::FundingLocked {
+                               channel_id: self.channel_id(),
+                               next_per_commitment_point: next_per_commitment_point,
+                       })
+               } else { None };
+
                let mut forwards = Vec::new();
                mem::swap(&mut forwards, &mut self.monitor_pending_forwards);
                let mut failures = Vec::new();
@@ -2356,7 +2396,7 @@ impl Channel {
                if self.channel_state & (ChannelState::PeerDisconnected as u32) != 0 {
                        self.monitor_pending_revoke_and_ack = false;
                        self.monitor_pending_commitment_signed = false;
-                       return (None, None, RAACommitmentOrder::RevokeAndACKFirst, forwards, failures);
+                       return (None, None, RAACommitmentOrder::RevokeAndACKFirst, forwards, failures, needs_broadcast_safe, funding_locked);
                }
 
                let raa = if self.monitor_pending_revoke_and_ack {
@@ -2369,11 +2409,12 @@ impl Channel {
                self.monitor_pending_revoke_and_ack = false;
                self.monitor_pending_commitment_signed = false;
                let order = self.resend_order.clone();
-               log_trace!(self, "Restored monitor updating resulting in {} commitment update and {} RAA, with {} first",
+               log_trace!(self, "Restored monitor updating resulting in {}{} commitment update and {} RAA, with {} first",
+                       if needs_broadcast_safe { "a funding broadcast safe, " } else { "" },
                        if commitment_update.is_some() { "a" } else { "no" },
                        if raa.is_some() { "an" } else { "no" },
                        match order { RAACommitmentOrder::CommitmentFirst => "commitment", RAACommitmentOrder::RevokeAndACKFirst => "RAA"});
-               (raa, commitment_update, order, forwards, failures)
+               (raa, commitment_update, order, forwards, failures, needs_broadcast_safe, funding_locked)
        }
 
        pub fn update_fee(&mut self, fee_estimator: &FeeEstimator, msg: &msgs::UpdateFee) -> Result<(), ChannelError> {
@@ -2451,7 +2492,7 @@ impl Channel {
                                update_add_htlcs.len(), update_fulfill_htlcs.len(), update_fail_htlcs.len(), update_fail_malformed_htlcs.len());
                msgs::CommitmentUpdate {
                        update_add_htlcs, update_fulfill_htlcs, update_fail_htlcs, update_fail_malformed_htlcs,
-                       update_fee: None, //TODO: We need to support re-generating any update_fees in the last commitment_signed!
+                       update_fee: None,
                        commitment_signed: self.send_commitment_no_state_update().expect("It looks like we failed to re-generate a commitment_signed we had previously sent?").0,
                }
        }
@@ -2471,6 +2512,22 @@ impl Channel {
                        return Err(ChannelError::Close("Peer sent a garbage channel_reestablish"));
                }
 
+               if msg.next_remote_commitment_number > 0 {
+                       match msg.data_loss_protect {
+                               OptionalField::Present(ref data_loss) => {
+                                       if chan_utils::build_commitment_secret(self.local_keys.commitment_seed, INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1) != data_loss.your_last_per_commitment_secret {
+                                               return Err(ChannelError::Close("Peer sent a garbage channel_reestablish with secret key not matching the commitment height provided"));
+                                       }
+                                       if msg.next_remote_commitment_number > INITIAL_COMMITMENT_NUMBER - self.cur_local_commitment_transaction_number {
+                                               self.channel_monitor.provide_rescue_remote_commitment_tx_info(data_loss.my_current_per_commitment_point);
+                                               return Err(ChannelError::CloseDelayBroadcast { msg: "We have fallen behind - we have received proof that if we broadcast remote is going to claim our funds - we can't do any automated broadcasting", update: Some(self.channel_monitor.clone())
+                                       });
+                                       }
+                               },
+                               OptionalField::Absent => {}
+                       }
+               }
+
                // Go ahead and unmark PeerDisconnected as various calls we may make check for it (and all
                // remaining cases either succeed or ErrorMessage-fail).
                self.channel_state &= !(ChannelState::PeerDisconnected as u32);
@@ -2483,7 +2540,9 @@ impl Channel {
                } else { None };
 
                if self.channel_state & (ChannelState::FundingSent as u32) == ChannelState::FundingSent as u32 {
-                       if self.channel_state & ChannelState::OurFundingLocked as u32 == 0 {
+                       // If we're waiting on a monitor update, we shouldn't re-send any funding_locked's.
+                       if self.channel_state & (ChannelState::OurFundingLocked as u32) == 0 ||
+                                       self.channel_state & (ChannelState::MonitorUpdateFailed as u32) != 0 {
                                if msg.next_remote_commitment_number != 0 {
                                        return Err(ChannelError::Close("Peer claimed they saw a revoke_and_ack but we haven't sent funding_locked yet"));
                                }
@@ -2545,7 +2604,7 @@ impl Channel {
                                // now!
                                match self.free_holding_cell_htlcs() {
                                        Err(ChannelError::Close(msg)) => return Err(ChannelError::Close(msg)),
-                                       Err(ChannelError::Ignore(_)) => panic!("Got non-channel-failing result from free_holding_cell_htlcs"),
+                                       Err(ChannelError::Ignore(_)) | Err(ChannelError::CloseDelayBroadcast { .. }) => 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), self.resend_order.clone(), shutdown_msg)),
                                        Ok(None) => return Ok((resend_funding_locked, required_revoke, None, None, self.resend_order.clone(), shutdown_msg)),
                                }
@@ -2832,7 +2891,6 @@ impl Channel {
                self.cur_remote_commitment_transaction_number + 2
        }
 
-       //TODO: Testing purpose only, should be changed in another way after #81
        #[cfg(test)]
        pub fn get_local_keys(&self) -> &ChannelKeys {
                &self.local_keys
@@ -2974,12 +3032,17 @@ impl Channel {
                                        //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 {
-                                               let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
-                                               let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
-                                               return Ok(Some(msgs::FundingLocked {
-                                                       channel_id: self.channel_id,
-                                                       next_per_commitment_point: next_per_commitment_point,
-                                               }));
+                                               if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 {
+                                                       let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
+                                                       let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
+                                                       return Ok(Some(msgs::FundingLocked {
+                                                               channel_id: self.channel_id,
+                                                               next_per_commitment_point: next_per_commitment_point,
+                                                       }));
+                                               } else {
+                                                       self.monitor_pending_funding_locked = true;
+                                                       return Ok(None);
+                                               }
                                        }
                                }
                        }
@@ -3072,7 +3135,7 @@ impl Channel {
                        channel_reserve_satoshis: Channel::get_our_channel_reserve_satoshis(self.channel_value_satoshis),
                        htlc_minimum_msat: self.our_htlc_minimum_msat,
                        feerate_per_kw: fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background) as u32,
-                       to_self_delay: BREAKDOWN_TIMEOUT,
+                       to_self_delay: self.our_to_self_delay,
                        max_accepted_htlcs: OUR_MAX_HTLCS,
                        funding_pubkey: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.funding_key),
                        revocation_basepoint: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.revocation_base_key),
@@ -3105,7 +3168,7 @@ impl Channel {
                        channel_reserve_satoshis: Channel::get_our_channel_reserve_satoshis(self.channel_value_satoshis),
                        htlc_minimum_msat: self.our_htlc_minimum_msat,
                        minimum_depth: self.minimum_depth,
-                       to_self_delay: BREAKDOWN_TIMEOUT,
+                       to_self_delay: self.our_to_self_delay,
                        max_accepted_htlcs: OUR_MAX_HTLCS,
                        funding_pubkey: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.funding_key),
                        revocation_basepoint: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.revocation_base_key),
@@ -3221,6 +3284,20 @@ impl Channel {
        pub fn get_channel_reestablish(&self) -> msgs::ChannelReestablish {
                assert_eq!(self.channel_state & ChannelState::PeerDisconnected as u32, ChannelState::PeerDisconnected as u32);
                assert_ne!(self.cur_remote_commitment_transaction_number, INITIAL_COMMITMENT_NUMBER);
+               let data_loss_protect = if self.cur_remote_commitment_transaction_number + 1 < INITIAL_COMMITMENT_NUMBER {
+                       let remote_last_secret = self.channel_monitor.get_secret(self.cur_remote_commitment_transaction_number + 2).unwrap();
+                       log_trace!(self, "Enough info to generate a Data Loss Protect with per_commitment_secret {}", log_bytes!(remote_last_secret));
+                       OptionalField::Present(DataLossProtect {
+                               your_last_per_commitment_secret: remote_last_secret,
+                               my_current_per_commitment_point: PublicKey::from_secret_key(&self.secp_ctx, &self.build_local_commitment_secret(self.cur_local_commitment_transaction_number + 1))
+                       })
+               } else {
+                       log_debug!(self, "We don't seen yet any revoked secret, if this channnel has already been updated it means we are fallen-behind, you should wait for other peer closing");
+                       OptionalField::Present(DataLossProtect {
+                               your_last_per_commitment_secret: [0;32],
+                               my_current_per_commitment_point: PublicKey::from_secret_key(&self.secp_ctx, &self.build_local_commitment_secret(self.cur_local_commitment_transaction_number))
+                       })
+               };
                msgs::ChannelReestablish {
                        channel_id: self.channel_id(),
                        // The protocol has two different commitment number concepts - the "commitment
@@ -3241,7 +3318,7 @@ impl Channel {
                        // dropped this channel on disconnect as it hasn't yet reached FundingSent so we can't
                        // overflow here.
                        next_remote_commitment_number: INITIAL_COMMITMENT_NUMBER - self.cur_remote_commitment_transaction_number - 1,
-                       data_loss_protect: OptionalField::Absent,
+                       data_loss_protect,
                }
        }
 
@@ -3281,16 +3358,15 @@ impl Channel {
                if outbound_htlc_count + 1 > self.their_max_accepted_htlcs as u32 {
                        return Err(ChannelError::Ignore("Cannot push more than their max accepted HTLCs"));
                }
-               //TODO: Spec is unclear if this is per-direction or in total (I assume per direction):
                // Check their_max_htlc_value_in_flight_msat
                if htlc_outbound_value_msat + amount_msat > self.their_max_htlc_value_in_flight_msat {
-                       return Err(ChannelError::Ignore("Cannot send value that would put us over the max HTLC value in flight"));
+                       return Err(ChannelError::Ignore("Cannot send value that would put us over the max HTLC value in flight our peer will accept"));
                }
 
                // Check self.their_channel_reserve_satoshis (the amount we must keep as
                // reserve for them to have something to claim if we misbehave)
                if self.value_to_self_msat < self.their_channel_reserve_satoshis * 1000 + amount_msat + htlc_outbound_value_msat {
-                       return Err(ChannelError::Ignore("Cannot send value that would put us over the reserve value"));
+                       return Err(ChannelError::Ignore("Cannot send value that would put us over their reserve value"));
                }
 
                //TODO: Check cltv_expiry? Do this in channel manager?
@@ -3698,6 +3774,7 @@ impl Writeable for Channel {
                        RAACommitmentOrder::RevokeAndACKFirst => 1u8.write(writer)?,
                }
 
+               self.monitor_pending_funding_locked.write(writer)?;
                self.monitor_pending_revoke_and_ack.write(writer)?;
                self.monitor_pending_commitment_signed.write(writer)?;
 
@@ -3754,6 +3831,7 @@ impl Writeable for Channel {
                self.their_htlc_minimum_msat.write(writer)?;
                self.our_htlc_minimum_msat.write(writer)?;
                self.their_to_self_delay.write(writer)?;
+               self.our_to_self_delay.write(writer)?;
                self.their_max_accepted_htlcs.write(writer)?;
                self.minimum_depth.write(writer)?;
 
@@ -3864,6 +3942,7 @@ impl<R : ::std::io::Read> ReadableArgs<R, Arc<Logger>> for Channel {
                        _ => return Err(DecodeError::InvalidValue),
                };
 
+               let monitor_pending_funding_locked = Readable::read(reader)?;
                let monitor_pending_revoke_and_ack = Readable::read(reader)?;
                let monitor_pending_commitment_signed = Readable::read(reader)?;
 
@@ -3915,6 +3994,7 @@ impl<R : ::std::io::Read> ReadableArgs<R, Arc<Logger>> for Channel {
                let their_htlc_minimum_msat = Readable::read(reader)?;
                let our_htlc_minimum_msat = Readable::read(reader)?;
                let their_to_self_delay = Readable::read(reader)?;
+               let our_to_self_delay = Readable::read(reader)?;
                let their_max_accepted_htlcs = Readable::read(reader)?;
                let minimum_depth = Readable::read(reader)?;
 
@@ -3959,6 +4039,7 @@ impl<R : ::std::io::Read> ReadableArgs<R, Arc<Logger>> for Channel {
 
                        resend_order,
 
+                       monitor_pending_funding_locked,
                        monitor_pending_revoke_and_ack,
                        monitor_pending_commitment_signed,
                        monitor_pending_forwards,
@@ -3992,6 +4073,7 @@ impl<R : ::std::io::Read> ReadableArgs<R, Arc<Logger>> for Channel {
                        their_htlc_minimum_msat,
                        our_htlc_minimum_msat,
                        their_to_self_delay,
+                       our_to_self_delay,
                        their_max_accepted_htlcs,
                        minimum_depth,