Handle double-HTLC-claims without failing the backwards channel
[rust-lightning] / lightning / src / ln / channel.rs
index 26dd61d686f0b0a8eb65e414c848283387217791..6d95525773445363361f5c6b5ba0d9f5da499d9f 100644 (file)
@@ -26,9 +26,10 @@ use ln::{PaymentPreimage, PaymentHash};
 use ln::features::{ChannelFeatures, InitFeatures};
 use ln::msgs;
 use ln::msgs::{DecodeError, OptionalField, DataLossProtect};
-use ln::channelmanager::{BestBlock, PendingHTLCStatus, HTLCSource, HTLCFailReason, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, MAX_LOCAL_BREAKDOWN_TIMEOUT};
+use ln::channelmanager::{PendingHTLCStatus, HTLCSource, HTLCFailReason, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, 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::BestBlock;
 use chain::chaininterface::{FeeEstimator,ConfirmationTarget};
 use chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, HTLC_FAIL_BACK_BUFFER};
 use chain::transaction::{OutPoint, TransactionData};
@@ -114,8 +115,8 @@ enum InboundHTLCState {
        /// commitment transaction without it as otherwise we'll have to force-close the channel to
        /// claim it before the timeout (obviously doesn't apply to revoked HTLCs that we can't claim
        /// anyway). That said, ChannelMonitor does this for us (see
-       /// ChannelMonitor::would_broadcast_at_height) so we actually remove the HTLC from our own
-       /// local state before then, once we're sure that the next commitment_signed and
+       /// ChannelMonitor::should_broadcast_holder_commitment_txn) so we actually remove the HTLC from
+       /// our own local state before then, once we're sure that the next commitment_signed and
        /// ChannelMonitor::provide_latest_local_commitment_tx will not include this HTLC.
        LocalRemoved(InboundHTLCRemovalReason),
 }
@@ -288,7 +289,7 @@ impl HTLCCandidate {
 }
 
 /// Information needed for constructing an invoice route hint for this channel.
-#[derive(Clone)]
+#[derive(Clone, Debug, PartialEq)]
 pub struct CounterpartyForwardingInfo {
        /// Base routing fee in millisatoshis.
        pub fee_base_msat: u32,
@@ -396,7 +397,7 @@ pub(super) struct Channel<Signer: Sign> {
        counterparty_max_htlc_value_in_flight_msat: u64,
        //get_holder_max_htlc_value_in_flight_msat(): u64,
        /// minimum channel reserve for self to maintain - set by them.
-       counterparty_selected_channel_reserve_satoshis: u64,
+       counterparty_selected_channel_reserve_satoshis: Option<u64>,
        // get_holder_selected_channel_reserve_satoshis(channel_value_sats: u64): u64
        counterparty_htlc_minimum_msat: u64,
        holder_htlc_minimum_msat: u64,
@@ -405,7 +406,7 @@ pub(super) struct Channel<Signer: Sign> {
        #[cfg(not(test))]
        counterparty_max_accepted_htlcs: u16,
        //implied by OUR_MAX_HTLCS: max_accepted_htlcs: u16,
-       minimum_depth: u32,
+       minimum_depth: Option<u32>,
 
        counterparty_forwarding_info: Option<CounterpartyForwardingInfo>,
 
@@ -434,6 +435,24 @@ pub(super) struct Channel<Signer: Sign> {
        next_local_commitment_tx_fee_info_cached: Mutex<Option<CommitmentTxInfoCached>>,
        #[cfg(any(test, feature = "fuzztarget"))]
        next_remote_commitment_tx_fee_info_cached: Mutex<Option<CommitmentTxInfoCached>>,
+
+       /// lnd has a long-standing bug where, upon reconnection, if the channel is not yet confirmed
+       /// they will not send a channel_reestablish until the channel locks in. Then, they will send a
+       /// funding_locked *before* sending the channel_reestablish (which is clearly a violation of
+       /// the BOLT specs). We copy c-lightning's workaround here and simply store the funding_locked
+       /// message until we receive a channel_reestablish.
+       ///
+       /// See-also <https://github.com/lightningnetwork/lnd/issues/4006>
+       pub workaround_lnd_bug_4006: Option<msgs::FundingLocked>,
+
+       #[cfg(any(test, feature = "fuzztarget"))]
+       // When we receive an HTLC fulfill on an outbound path, we may immediately fulfill the
+       // corresponding HTLC on the inbound path. If, then, the outbound path channel is
+       // disconnected and reconnected (before we've exchange commitment_signed and revoke_and_ack
+       // messages), they may re-broadcast their update_fulfill_htlc, causing a duplicate claim. This
+       // is fine, but as a sanity check in our failure to generate the second claim, we check here
+       // that the original was a claim, and that we aren't now trying to fulfill a failed HTLC.
+       historical_inbound_htlc_fulfills: HashSet<u64>,
 }
 
 #[cfg(any(test, feature = "fuzztarget"))]
@@ -600,11 +619,11 @@ impl<Signer: Sign> Channel<Signer> {
                        counterparty_dust_limit_satoshis: 0,
                        holder_dust_limit_satoshis: MIN_DUST_LIMIT_SATOSHIS,
                        counterparty_max_htlc_value_in_flight_msat: 0,
-                       counterparty_selected_channel_reserve_satoshis: 0,
+                       counterparty_selected_channel_reserve_satoshis: None, // Filled in in accept_channel
                        counterparty_htlc_minimum_msat: 0,
                        holder_htlc_minimum_msat: if config.own_channel_config.our_htlc_minimum_msat == 0 { 1 } else { config.own_channel_config.our_htlc_minimum_msat },
                        counterparty_max_accepted_htlcs: 0,
-                       minimum_depth: 0, // Filled in in accept_channel
+                       minimum_depth: None, // Filled in in accept_channel
 
                        counterparty_forwarding_info: None,
 
@@ -633,6 +652,11 @@ impl<Signer: Sign> Channel<Signer> {
                        next_local_commitment_tx_fee_info_cached: Mutex::new(None),
                        #[cfg(any(test, feature = "fuzztarget"))]
                        next_remote_commitment_tx_fee_info_cached: Mutex::new(None),
+
+                       workaround_lnd_bug_4006: None,
+
+                       #[cfg(any(test, feature = "fuzztarget"))]
+                       historical_inbound_htlc_fulfills: HashSet::new(),
                })
        }
 
@@ -840,11 +864,11 @@ impl<Signer: Sign> Channel<Signer> {
                        counterparty_dust_limit_satoshis: msg.dust_limit_satoshis,
                        holder_dust_limit_satoshis: MIN_DUST_LIMIT_SATOSHIS,
                        counterparty_max_htlc_value_in_flight_msat: cmp::min(msg.max_htlc_value_in_flight_msat, msg.funding_satoshis * 1000),
-                       counterparty_selected_channel_reserve_satoshis: msg.channel_reserve_satoshis,
+                       counterparty_selected_channel_reserve_satoshis: Some(msg.channel_reserve_satoshis),
                        counterparty_htlc_minimum_msat: msg.htlc_minimum_msat,
                        holder_htlc_minimum_msat: if config.own_channel_config.our_htlc_minimum_msat == 0 { 1 } else { config.own_channel_config.our_htlc_minimum_msat },
                        counterparty_max_accepted_htlcs: msg.max_accepted_htlcs,
-                       minimum_depth: config.own_channel_config.minimum_depth,
+                       minimum_depth: Some(config.own_channel_config.minimum_depth),
 
                        counterparty_forwarding_info: None,
 
@@ -876,6 +900,11 @@ impl<Signer: Sign> Channel<Signer> {
                        next_local_commitment_tx_fee_info_cached: Mutex::new(None),
                        #[cfg(any(test, feature = "fuzztarget"))]
                        next_remote_commitment_tx_fee_info_cached: Mutex::new(None),
+
+                       workaround_lnd_bug_4006: None,
+
+                       #[cfg(any(test, feature = "fuzztarget"))]
+                       historical_inbound_htlc_fulfills: HashSet::new(),
                };
 
                Ok(chan)
@@ -1023,7 +1052,7 @@ impl<Signer: Sign> Channel<Signer> {
                        } else {
                                self.counterparty_max_commitment_tx_output.lock().unwrap()
                        };
-                       debug_assert!(broadcaster_max_commitment_tx_output.0 <= value_to_self_msat as u64 || value_to_self_msat / 1000 >= self.counterparty_selected_channel_reserve_satoshis as i64);
+                       debug_assert!(broadcaster_max_commitment_tx_output.0 <= value_to_self_msat as u64 || value_to_self_msat / 1000 >= self.counterparty_selected_channel_reserve_satoshis.unwrap() as i64);
                        broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, value_to_self_msat as u64);
                        debug_assert!(broadcaster_max_commitment_tx_output.1 <= value_to_remote_msat as u64 || value_to_remote_msat / 1000 >= Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis) as i64);
                        broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, value_to_remote_msat as u64);
@@ -1203,13 +1232,6 @@ impl<Signer: Sign> Channel<Signer> {
                make_funding_redeemscript(&self.get_holder_pubkeys().funding_pubkey, self.counterparty_funding_pubkey())
        }
 
-       /// Builds the htlc-success or htlc-timeout transaction which spends a given HTLC output
-       /// @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: &Txid, htlc: &HTLCOutputInCommitment, local: bool, keys: &TxCreationKeys, feerate_per_kw: u32) -> Transaction {
-               chan_utils::build_htlc_transaction(prev_hash, feerate_per_kw, if local { self.get_counterparty_selected_contest_delay() } else { self.get_holder_selected_contest_delay() }, htlc, &keys.broadcaster_delayed_payment_key, &keys.revocation_key)
-       }
-
        /// Per HTLC, only one get_update_fail_htlc or get_update_fulfill_htlc call may be made.
        /// In such cases we debug_assert!(false) and return a ChannelError::Ignore. Thus, will always
        /// return Ok(_) if debug assertions are turned on or preconditions are met.
@@ -1242,8 +1264,8 @@ impl<Signer: Sign> Channel<Signer> {
                                                if let &InboundHTLCRemovalReason::Fulfill(_) = reason {
                                                } else {
                                                        log_warn!(logger, "Have preimage and want to fulfill HTLC with payment hash {} we already failed against channel {}", log_bytes!(htlc.payment_hash.0), log_bytes!(self.channel_id()));
+                                                       debug_assert!(false, "Tried to fulfill an HTLC that was already failed");
                                                }
-                                               debug_assert!(false, "Tried to fulfill an HTLC that was already fail/fulfilled");
                                                return Ok((None, None));
                                        },
                                        _ => {
@@ -1256,7 +1278,11 @@ impl<Signer: Sign> Channel<Signer> {
                        }
                }
                if pending_idx == core::usize::MAX {
-                       return Err(ChannelError::Ignore("Unable to find a pending HTLC which matched the given HTLC ID".to_owned()));
+                       #[cfg(any(test, feature = "fuzztarget"))]
+                       // If we failed to find an HTLC to fulfill, make sure it was previously fulfilled and
+                       // this is simply a duplicate claim, not previously failed and we lost funds.
+                       debug_assert!(self.historical_inbound_htlc_fulfills.contains(&htlc_id_arg));
+                       return Ok((None, None));
                }
 
                // Now update local state:
@@ -1278,7 +1304,8 @@ impl<Signer: Sign> Channel<Signer> {
                                                if htlc_id_arg == htlc_id {
                                                        // Make sure we don't leave latest_monitor_update_id incremented here:
                                                        self.latest_monitor_update_id -= 1;
-                                                       debug_assert!(false, "Tried to fulfill an HTLC that was already fulfilled");
+                                                       #[cfg(any(test, feature = "fuzztarget"))]
+                                                       debug_assert!(self.historical_inbound_htlc_fulfills.contains(&htlc_id_arg));
                                                        return Ok((None, None));
                                                }
                                        },
@@ -1298,8 +1325,12 @@ impl<Signer: Sign> Channel<Signer> {
                        self.holding_cell_htlc_updates.push(HTLCUpdateAwaitingACK::ClaimHTLC {
                                payment_preimage: payment_preimage_arg, htlc_id: htlc_id_arg,
                        });
+                       #[cfg(any(test, feature = "fuzztarget"))]
+                       self.historical_inbound_htlc_fulfills.insert(htlc_id_arg);
                        return Ok((None, Some(monitor_update)));
                }
+               #[cfg(any(test, feature = "fuzztarget"))]
+               self.historical_inbound_htlc_fulfills.insert(htlc_id_arg);
 
                {
                        let htlc = &mut self.pending_inbound_htlcs[pending_idx];
@@ -1359,8 +1390,11 @@ impl<Signer: Sign> Channel<Signer> {
                        if htlc.htlc_id == htlc_id_arg {
                                match htlc.state {
                                        InboundHTLCState::Committed => {},
-                                       InboundHTLCState::LocalRemoved(_) => {
-                                               debug_assert!(false, "Tried to fail an HTLC that was already fail/fulfilled");
+                                       InboundHTLCState::LocalRemoved(ref reason) => {
+                                               if let &InboundHTLCRemovalReason::Fulfill(_) = reason {
+                                               } else {
+                                                       debug_assert!(false, "Tried to fail an HTLC that was already failed");
+                                               }
                                                return Ok(None);
                                        },
                                        _ => {
@@ -1372,7 +1406,11 @@ impl<Signer: Sign> Channel<Signer> {
                        }
                }
                if pending_idx == core::usize::MAX {
-                       return Err(ChannelError::Ignore("Unable to find a pending HTLC which matched the given HTLC ID".to_owned()));
+                       #[cfg(any(test, feature = "fuzztarget"))]
+                       // If we failed to find an HTLC to fail, make sure it was previously fulfilled and this
+                       // is simply a duplicate fail, not previously failed and we failed-back too early.
+                       debug_assert!(self.historical_inbound_htlc_fulfills.contains(&htlc_id_arg));
+                       return Ok(None);
                }
 
                // Now update local state:
@@ -1381,8 +1419,9 @@ impl<Signer: Sign> Channel<Signer> {
                                match pending_update {
                                        &HTLCUpdateAwaitingACK::ClaimHTLC { htlc_id, .. } => {
                                                if htlc_id_arg == htlc_id {
-                                                       debug_assert!(false, "Tried to fail an HTLC that was already fulfilled");
-                                                       return Err(ChannelError::Ignore("Unable to find a pending HTLC which matched the given HTLC ID".to_owned()));
+                                                       #[cfg(any(test, feature = "fuzztarget"))]
+                                                       debug_assert!(self.historical_inbound_htlc_fulfills.contains(&htlc_id_arg));
+                                                       return Ok(None);
                                                }
                                        },
                                        &HTLCUpdateAwaitingACK::FailHTLC { htlc_id, .. } => {
@@ -1475,6 +1514,12 @@ impl<Signer: Sign> Channel<Signer> {
                if msg.minimum_depth > config.peer_channel_config_limits.max_minimum_depth {
                        return Err(ChannelError::Close(format!("We consider the minimum depth to be unreasonably large. Expected minimum: ({}). Actual: ({})", config.peer_channel_config_limits.max_minimum_depth, msg.minimum_depth)));
                }
+               if msg.minimum_depth == 0 {
+                       // Note that if this changes we should update the serialization minimum version to
+                       // indicate to older clients that they don't understand some features of the current
+                       // channel.
+                       return Err(ChannelError::Close("Minimum confirmation depth must be at least 1".to_owned()));
+               }
 
                let counterparty_shutdown_scriptpubkey = if their_features.supports_upfront_shutdown_script() {
                        match &msg.shutdown_scriptpubkey {
@@ -1498,10 +1543,10 @@ impl<Signer: Sign> Channel<Signer> {
 
                self.counterparty_dust_limit_satoshis = msg.dust_limit_satoshis;
                self.counterparty_max_htlc_value_in_flight_msat = cmp::min(msg.max_htlc_value_in_flight_msat, self.channel_value_satoshis * 1000);
-               self.counterparty_selected_channel_reserve_satoshis = msg.channel_reserve_satoshis;
+               self.counterparty_selected_channel_reserve_satoshis = Some(msg.channel_reserve_satoshis);
                self.counterparty_htlc_minimum_msat = msg.htlc_minimum_msat;
                self.counterparty_max_accepted_htlcs = msg.max_accepted_htlcs;
-               self.minimum_depth = msg.minimum_depth;
+               self.minimum_depth = Some(msg.minimum_depth);
 
                let counterparty_pubkeys = ChannelPublicKeys {
                        funding_pubkey: msg.funding_pubkey,
@@ -1703,7 +1748,8 @@ impl<Signer: Sign> Channel<Signer> {
 
        pub fn funding_locked<L: Deref>(&mut self, msg: &msgs::FundingLocked, logger: &L) -> Result<(), ChannelError> where L::Target: Logger {
                if self.channel_state & (ChannelState::PeerDisconnected as u32) == ChannelState::PeerDisconnected as u32 {
-                       return Err(ChannelError::Close("Peer sent funding_locked when we needed a channel_reestablish".to_owned()));
+                       self.workaround_lnd_bug_4006 = Some(msg.clone());
+                       return Err(ChannelError::Ignore("Peer sent funding_locked when we needed a channel_reestablish. The peer is likely lnd, see https://github.com/lightningnetwork/lnd/issues/4006".to_owned()));
                }
 
                let non_shutdown_state = self.channel_state & (!MULTI_STATE_FLAGS);
@@ -1771,8 +1817,22 @@ impl<Signer: Sign> Channel<Signer> {
        /// corner case properly.
        pub fn get_inbound_outbound_available_balance_msat(&self) -> (u64, u64) {
                // Note that we have to handle overflow due to the above case.
-               (cmp::max(self.channel_value_satoshis as i64 * 1000 - self.value_to_self_msat as i64 - self.get_inbound_pending_htlc_stats().1 as i64, 0) as u64,
-               cmp::max(self.value_to_self_msat as i64 - self.get_outbound_pending_htlc_stats().1 as i64, 0) as u64)
+               (
+                       cmp::max(self.channel_value_satoshis as i64 * 1000
+                               - self.value_to_self_msat as i64
+                               - self.get_inbound_pending_htlc_stats().1 as i64
+                               - Self::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis) as i64 * 1000,
+                       0) as u64,
+                       cmp::max(self.value_to_self_msat as i64
+                               - self.get_outbound_pending_htlc_stats().1 as i64
+                               - self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0) as i64 * 1000,
+                       0) as u64
+               )
+       }
+
+       pub fn get_holder_counterparty_selected_channel_reserve_satoshis(&self) -> (u64, Option<u64>) {
+               (Self::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis),
+               self.counterparty_selected_channel_reserve_satoshis)
        }
 
        // Get the fee cost of a commitment tx with a given number of HTLC outputs.
@@ -2050,7 +2110,7 @@ impl<Signer: Sign> Channel<Signer> {
                        // Check that they won't violate our local required channel reserve by adding this HTLC.
                        let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
                        let local_commit_tx_fee_msat = self.next_local_commit_tx_fee_msat(htlc_candidate, None);
-                       if self.value_to_self_msat < self.counterparty_selected_channel_reserve_satoshis * 1000 + local_commit_tx_fee_msat {
+                       if self.value_to_self_msat < self.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 + local_commit_tx_fee_msat {
                                return Err(ChannelError::Close("Cannot accept HTLC that would put our balance under counterparty-announced channel reserve value".to_owned()));
                        }
                }
@@ -2222,7 +2282,10 @@ impl<Signer: Sign> Channel<Signer> {
                let mut htlcs_and_sigs = Vec::with_capacity(htlcs_cloned.len());
                for (idx, (htlc, source)) in htlcs_cloned.drain(..).enumerate() {
                        if let Some(_) = htlc.transaction_output_index {
-                               let htlc_tx = self.build_htlc_transaction(&commitment_txid, &htlc, true, &keys, feerate_per_kw);
+                               let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, feerate_per_kw,
+                                       self.get_counterparty_selected_contest_delay().unwrap(), &htlc,
+                                       &keys.broadcaster_delayed_payment_key, &keys.revocation_key);
+
                                let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &keys);
                                let htlc_sighash = hash_to_message!(&bip143::SigHashCache::new(&htlc_tx).signature_hash(0, &htlc_redeemscript, htlc.amount_msat / 1000, SigHashType::All)[..]);
                                log_trace!(logger, "Checking HTLC tx signature {} by key {} against tx {} (sighash {}) with redeemscript {} in channel {}.",
@@ -2422,7 +2485,14 @@ impl<Signer: Sign> Channel<Signer> {
                                        },
                                        &HTLCUpdateAwaitingACK::FailHTLC { htlc_id, ref err_packet } => {
                                                match self.get_update_fail_htlc(htlc_id, err_packet.clone(), logger) {
-                                                       Ok(update_fail_msg_option) => update_fail_htlcs.push(update_fail_msg_option.unwrap()),
+                                                       Ok(update_fail_msg_option) => {
+                                                               // If an HTLC failure was previously added to the holding cell (via
+                                                               // `get_update_fail_htlc`) then generating the fail message itself
+                                                               // must not fail - we should never end up in a state where we
+                                                               // double-fail an HTLC or fail-then-claim an HTLC as it indicates
+                                                               // we didn't wait for a full revocation before failing.
+                                                               update_fail_htlcs.push(update_fail_msg_option.unwrap())
+                                                       },
                                                        Err(e) => {
                                                                if let ChannelError::Ignore(_) = e {}
                                                                else {
@@ -2568,10 +2638,8 @@ impl<Signer: Sign> Channel<Signer> {
                        });
                        for htlc in pending_inbound_htlcs.iter_mut() {
                                let swap = if let &InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) = &htlc.state {
-                                       log_trace!(logger, " ...promoting inbound AwaitingRemoteRevokeToAnnounce {} to Committed", log_bytes!(htlc.payment_hash.0));
                                        true
                                } else if let &InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) = &htlc.state {
-                                       log_trace!(logger, " ...promoting inbound AwaitingAnnouncedRemoteRevoke {} to Committed", log_bytes!(htlc.payment_hash.0));
                                        true
                                } else { false };
                                if swap {
@@ -2579,11 +2647,13 @@ impl<Signer: Sign> Channel<Signer> {
                                        mem::swap(&mut state, &mut htlc.state);
 
                                        if let InboundHTLCState::AwaitingRemoteRevokeToAnnounce(forward_info) = state {
+                                               log_trace!(logger, " ...promoting inbound AwaitingRemoteRevokeToAnnounce {} to AwaitingAnnouncedRemoteRevoke", log_bytes!(htlc.payment_hash.0));
                                                htlc.state = InboundHTLCState::AwaitingAnnouncedRemoteRevoke(forward_info);
                                                require_commitment = true;
                                        } else if let InboundHTLCState::AwaitingAnnouncedRemoteRevoke(forward_info) = state {
                                                match forward_info {
                                                        PendingHTLCStatus::Fail(fail_msg) => {
+                                                               log_trace!(logger, " ...promoting inbound AwaitingAnnouncedRemoteRevoke {} to LocalRemoved due to PendingHTLCStatus indicating failure", log_bytes!(htlc.payment_hash.0));
                                                                require_commitment = true;
                                                                match fail_msg {
                                                                        HTLCFailureMsg::Relay(msg) => {
@@ -2597,6 +2667,7 @@ impl<Signer: Sign> Channel<Signer> {
                                                                }
                                                        },
                                                        PendingHTLCStatus::Forward(forward_info) => {
+                                                               log_trace!(logger, " ...promoting inbound AwaitingAnnouncedRemoteRevoke {} to Committed", log_bytes!(htlc.payment_hash.0));
                                                                to_forward_infos.push((forward_info, htlc.htlc_id));
                                                                htlc.state = InboundHTLCState::Committed;
                                                        }
@@ -3321,6 +3392,10 @@ impl<Signer: Sign> Channel<Signer> {
                self.channel_id
        }
 
+       pub fn minimum_depth(&self) -> Option<u32> {
+               self.minimum_depth
+       }
+
        /// Gets the "user_id" value passed into the construction of this channel. It has no special
        /// meaning and exists only to allow users to have a persistent identifier of a channel.
        pub fn get_user_id(&self) -> u64 {
@@ -3348,8 +3423,9 @@ impl<Signer: Sign> Channel<Signer> {
                &self.channel_transaction_parameters.holder_pubkeys
        }
 
-       fn get_counterparty_selected_contest_delay(&self) -> u16 {
-               self.channel_transaction_parameters.counterparty_parameters.as_ref().unwrap().selected_contest_delay
+       pub fn get_counterparty_selected_contest_delay(&self) -> Option<u16> {
+               self.channel_transaction_parameters.counterparty_parameters
+                       .as_ref().map(|params| params.selected_contest_delay)
        }
 
        fn get_counterparty_pubkeys(&self) -> &ChannelPublicKeys {
@@ -3423,7 +3499,7 @@ impl<Signer: Sign> Channel<Signer> {
                ChannelValueStat {
                        value_to_self_msat: self.value_to_self_msat,
                        channel_value_msat: self.channel_value_satoshis * 1000,
-                       channel_reserve_msat: self.counterparty_selected_channel_reserve_satoshis * 1000,
+                       channel_reserve_msat: self.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000,
                        pending_outbound_htlcs_amount_msat: self.pending_outbound_htlcs.iter().map(|ref h| h.amount_msat).sum::<u64>(),
                        pending_inbound_htlcs_amount_msat: self.pending_inbound_htlcs.iter().map(|ref h| h.amount_msat).sum::<u64>(),
                        holding_cell_outbound_amount_msat: {
@@ -3498,7 +3574,7 @@ impl<Signer: Sign> Channel<Signer> {
        /// is_usable() and considers things like the channel being temporarily disabled.
        /// Allowed in any state (including after shutdown)
        pub fn is_live(&self) -> bool {
-               self.is_usable() && (self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32) == 0)
+               self.is_usable() && (self.channel_state & (ChannelState::PeerDisconnected as u32) == 0)
        }
 
        /// Returns true if this channel has been marked as awaiting a monitor update to move forward.
@@ -3540,7 +3616,7 @@ impl<Signer: Sign> Channel<Signer> {
                        self.funding_tx_confirmation_height = 0;
                }
 
-               if funding_tx_confirmations < self.minimum_depth as i64 {
+               if funding_tx_confirmations < self.minimum_depth.unwrap_or(0) as i64 {
                        return None;
                }
 
@@ -3695,10 +3771,10 @@ impl<Signer: Sign> Channel<Signer> {
                        // 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 {
+                       if funding_tx_confirmations < self.minimum_depth.unwrap() 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),
+                                       data: format!("Funding transaction was un-confirmed. Locked at {} confs, now have {} confs.", self.minimum_depth.unwrap(), funding_tx_confirmations),
                                });
                        }
                }
@@ -3793,7 +3869,7 @@ impl<Signer: Sign> Channel<Signer> {
                        max_htlc_value_in_flight_msat: Channel::<Signer>::get_holder_max_htlc_value_in_flight_msat(self.channel_value_satoshis),
                        channel_reserve_satoshis: Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis),
                        htlc_minimum_msat: self.holder_htlc_minimum_msat,
-                       minimum_depth: self.minimum_depth,
+                       minimum_depth: self.minimum_depth.unwrap(),
                        to_self_delay: self.get_holder_selected_contest_delay(),
                        max_accepted_htlcs: OUR_MAX_HTLCS,
                        funding_pubkey: keys.funding_pubkey,
@@ -4015,10 +4091,18 @@ impl<Signer: Sign> Channel<Signer> {
 
        /// Adds a pending outbound HTLC to this channel, note that you probably want
        /// send_htlc_and_commit instead cause you'll want both messages at once.
-       /// This returns an option instead of a pure UpdateAddHTLC as we may be in a state where we are
-       /// waiting on the remote peer to send us a revoke_and_ack during which time we cannot add new
-       /// HTLCs on the wire or we wouldn't be able to determine what they actually ACK'ed.
-       /// You MUST call send_commitment prior to any other calls on this Channel
+       ///
+       /// This returns an optional UpdateAddHTLC as we may be in a state where we cannot add HTLCs on
+       /// the wire:
+       /// * In cases where we're waiting on the remote peer to send us a revoke_and_ack, we
+       ///   wouldn't be able to determine what they actually ACK'ed if we have two sets of updates
+       ///   awaiting ACK.
+       /// * In cases where we're marked MonitorUpdateFailed, we cannot commit to a new state as we
+       ///   may not yet have sent the previous commitment update messages and will need to regenerate
+       ///   them.
+       ///
+       /// You MUST call send_commitment prior to calling any other methods on this Channel!
+       ///
        /// If an Err is returned, it's a ChannelError::Ignore!
        pub fn send_htlc(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, onion_routing_packet: msgs::OnionPacket) -> Result<Option<msgs::UpdateAddHTLC>, ChannelError> {
                if (self.channel_state & (ChannelState::ChannelFunded as u32 | BOTH_SIDES_SHUTDOWN_MASK)) != (ChannelState::ChannelFunded as u32) {
@@ -4037,14 +4121,14 @@ impl<Signer: Sign> Channel<Signer> {
                        return Err(ChannelError::Ignore(format!("Cannot send less than their minimum HTLC value ({})", self.counterparty_htlc_minimum_msat)));
                }
 
-               if (self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 {
+               if (self.channel_state & (ChannelState::PeerDisconnected as u32)) != 0 {
                        // Note that this should never really happen, if we're !is_live() on receipt of an
                        // incoming HTLC for relay will result in us rejecting the HTLC and we won't allow
                        // the user to send directly into a !is_live() channel. However, if we
                        // disconnected during the time the previous hop was doing the commitment dance we may
                        // end up getting here after the forwarding delay. In any case, returning an
                        // IgnoreError will get ChannelManager to do the right thing and fail backwards now.
-                       return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected/frozen for channel monitor update".to_owned()));
+                       return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected from channel counterparty".to_owned()));
                }
 
                let (outbound_htlc_count, htlc_outbound_value_msat) = self.get_outbound_pending_htlc_stats();
@@ -4083,13 +4167,13 @@ impl<Signer: Sign> Channel<Signer> {
 
                // Check self.counterparty_selected_channel_reserve_satoshis (the amount we must keep as
                // reserve for the remote to have something to claim if we misbehave)
-               let chan_reserve_msat = self.counterparty_selected_channel_reserve_satoshis * 1000;
+               let chan_reserve_msat = self.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000;
                if pending_value_to_self_msat - amount_msat - commit_tx_fee_msat < chan_reserve_msat {
                        return Err(ChannelError::Ignore(format!("Cannot send value that would put our balance under counterparty-announced channel reserve value ({})", chan_reserve_msat)));
                }
 
                // Now update local state:
-               if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == (ChannelState::AwaitingRemoteRevoke as u32) {
+               if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 {
                        self.holding_cell_htlc_updates.push(HTLCUpdateAwaitingACK::AddHTLC {
                                amount_msat,
                                payment_hash,
@@ -4288,8 +4372,7 @@ impl<Signer: Sign> Channel<Signer> {
        }
 
        pub fn channel_update(&mut self, msg: &msgs::ChannelUpdate) -> Result<(), ChannelError> {
-               let usable_channel_value_msat = (self.channel_value_satoshis - self.counterparty_selected_channel_reserve_satoshis) * 1000;
-               if msg.contents.htlc_minimum_msat >= usable_channel_value_msat {
+               if msg.contents.htlc_minimum_msat >= self.channel_value_satoshis * 1000 {
                        return Err(ChannelError::Close("Minimum htlc value is greater than channel value".to_string()));
                }
                self.counterparty_forwarding_info = Some(CounterpartyForwardingInfo {
@@ -4617,11 +4700,11 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
                self.counterparty_dust_limit_satoshis.write(writer)?;
                self.holder_dust_limit_satoshis.write(writer)?;
                self.counterparty_max_htlc_value_in_flight_msat.write(writer)?;
-               self.counterparty_selected_channel_reserve_satoshis.write(writer)?;
+               self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0).write(writer)?;
                self.counterparty_htlc_minimum_msat.write(writer)?;
                self.holder_htlc_minimum_msat.write(writer)?;
                self.counterparty_max_accepted_htlcs.write(writer)?;
-               self.minimum_depth.write(writer)?;
+               self.minimum_depth.unwrap_or(0).write(writer)?;
 
                match &self.counterparty_forwarding_info {
                        Some(info) => {
@@ -4646,7 +4729,24 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 
                self.channel_update_status.write(writer)?;
 
-               write_tlv_fields!(writer, {}, {(0, self.announcement_sigs)});
+               #[cfg(any(test, feature = "fuzztarget"))]
+               (self.historical_inbound_htlc_fulfills.len() as u64).write(writer)?;
+               #[cfg(any(test, feature = "fuzztarget"))]
+               for htlc in self.historical_inbound_htlc_fulfills.iter() {
+                       htlc.write(writer)?;
+               }
+
+               write_tlv_fields!(writer, {
+                       (0, self.announcement_sigs, option),
+                       // minimum_depth and counterparty_selected_channel_reserve_satoshis used to have a
+                       // default value instead of being Option<>al. Thus, to maintain compatibility we write
+                       // them twice, once with their original default values above, and once as an option
+                       // here. On the read side, old versions will simply ignore the odd-type entries here,
+                       // and new versions map the default values to None and allow the TLV entries here to
+                       // override that.
+                       (1, self.minimum_depth, option),
+                       (3, self.counterparty_selected_channel_reserve_satoshis, option),
+               });
 
                Ok(())
        }
@@ -4789,11 +4889,21 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
                let counterparty_dust_limit_satoshis = Readable::read(reader)?;
                let holder_dust_limit_satoshis = Readable::read(reader)?;
                let counterparty_max_htlc_value_in_flight_msat = Readable::read(reader)?;
-               let counterparty_selected_channel_reserve_satoshis = Readable::read(reader)?;
+               let mut counterparty_selected_channel_reserve_satoshis = Some(Readable::read(reader)?);
+               if counterparty_selected_channel_reserve_satoshis == Some(0) {
+                       // Versions up to 0.0.98 had counterparty_selected_channel_reserve_satoshis as a
+                       // non-option, writing 0 for what we now consider None.
+                       counterparty_selected_channel_reserve_satoshis = None;
+               }
                let counterparty_htlc_minimum_msat = Readable::read(reader)?;
                let holder_htlc_minimum_msat = Readable::read(reader)?;
                let counterparty_max_accepted_htlcs = Readable::read(reader)?;
-               let minimum_depth = Readable::read(reader)?;
+               let mut minimum_depth = Some(Readable::read(reader)?);
+               if minimum_depth == Some(0) {
+                       // Versions up to 0.0.98 had minimum_depth as a non-option, writing 0 for what we now
+                       // consider None.
+                       minimum_depth = None;
+               }
 
                let counterparty_forwarding_info = match <u8 as Readable>::read(reader)? {
                        0 => None,
@@ -4818,8 +4928,22 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 
                let channel_update_status = Readable::read(reader)?;
 
+               #[cfg(any(test, feature = "fuzztarget"))]
+               let mut historical_inbound_htlc_fulfills = HashSet::new();
+               #[cfg(any(test, feature = "fuzztarget"))]
+               {
+                       let htlc_fulfills_len: u64 = Readable::read(reader)?;
+                       for _ in 0..htlc_fulfills_len {
+                               assert!(historical_inbound_htlc_fulfills.insert(Readable::read(reader)?));
+                       }
+               }
+
                let mut announcement_sigs = None;
-               read_tlv_fields!(reader, {}, {(0, announcement_sigs)});
+               read_tlv_fields!(reader, {
+                       (0, announcement_sigs, option),
+                       (1, minimum_depth, option),
+                       (3, counterparty_selected_channel_reserve_satoshis, option),
+               });
 
                let mut secp_ctx = Secp256k1::new();
                secp_ctx.seeded_randomize(&keys_source.get_secure_random_bytes());
@@ -4903,6 +5027,11 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
                        next_local_commitment_tx_fee_info_cached: Mutex::new(None),
                        #[cfg(any(test, feature = "fuzztarget"))]
                        next_remote_commitment_tx_fee_info_cached: Mutex::new(None),
+
+                       workaround_lnd_bug_4006: None,
+
+                       #[cfg(any(test, feature = "fuzztarget"))]
+                       historical_inbound_htlc_fulfills,
                })
        }
 }
@@ -4919,13 +5048,14 @@ mod tests {
        use bitcoin::hashes::hex::FromHex;
        use hex;
        use ln::{PaymentPreimage, PaymentHash};
-       use ln::channelmanager::{BestBlock, HTLCSource};
+       use ln::channelmanager::HTLCSource;
        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::BestBlock;
        use chain::chaininterface::{FeeEstimator,ConfirmationTarget};
        use chain::keysinterface::{InMemorySigner, KeysInterface, BaseSign};
        use chain::transaction::OutPoint;
@@ -5272,6 +5402,7 @@ mod tests {
                config.channel_options.announced_channel = false;
                let mut chan = Channel::<InMemorySigner>::new_outbound(&&feeest, &&keys_provider, counterparty_node_id, 10_000_000, 100000, 42, &config).unwrap(); // Nothing uses their network key in this test
                chan.holder_dust_limit_satoshis = 546;
+               chan.counterparty_selected_channel_reserve_satoshis = Some(0); // Filled in in accept_channel
 
                let funding_info = OutPoint{ txid: Txid::from_hex("8984484a580b825b9972d7adb15050b3ab624ccd731946b3eeddb92f4e7ef6be").unwrap(), index: 0 };
 
@@ -5359,7 +5490,9 @@ mod tests {
                                        let remote_signature = Signature::from_der(&hex::decode($counterparty_htlc_sig_hex).unwrap()[..]).unwrap();
 
                                        let ref htlc = htlcs[$htlc_idx];
-                                       let htlc_tx = chan.build_htlc_transaction(&unsigned_tx.txid, &htlc, true, &keys, chan.feerate_per_kw);
+                                       let htlc_tx = chan_utils::build_htlc_transaction(&unsigned_tx.txid, chan.feerate_per_kw,
+                                               chan.get_counterparty_selected_contest_delay().unwrap(),
+                                               &htlc, &keys.broadcaster_delayed_payment_key, &keys.revocation_key);
                                        let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &keys);
                                        let htlc_sighash = Message::from_slice(&bip143::SigHashCache::new(&htlc_tx).signature_hash(0, &htlc_redeemscript, htlc.amount_msat / 1000, SigHashType::All)[..]).unwrap();
                                        secp_ctx.verify(&htlc_sighash, &remote_signature, &keys.countersignatory_htlc_key).unwrap();