Make the base fee configurable in ChannelConfig
[rust-lightning] / lightning / src / chain / channelmonitor.rs
index 26187ce34166d5a189a48a3be2522a46a181ceba..e78faf7764025b92c2ae773cbd8d5e32e4e62395 100644 (file)
@@ -37,9 +37,9 @@ use ln::{PaymentHash, PaymentPreimage};
 use ln::msgs::DecodeError;
 use ln::chan_utils;
 use ln::chan_utils::{CounterpartyCommitmentSecrets, HTLCOutputInCommitment, HTLCType, ChannelTransactionParameters, HolderCommitmentTransaction};
-use ln::channelmanager::{BestBlock, HTLCSource};
+use ln::channelmanager::HTLCSource;
 use chain;
-use chain::WatchedOutput;
+use chain::{BestBlock, WatchedOutput};
 use chain::chaininterface::{BroadcasterInterface, FeeEstimator};
 use chain::transaction::{OutPoint, TransactionData};
 use chain::keysinterface::{SpendableOutputDescriptor, StaticPaymentOutputDescriptor, DelayedPaymentOutputDescriptor, Sign, KeysInterface};
@@ -47,12 +47,11 @@ use chain::onchaintx::OnchainTxHandler;
 use chain::package::{CounterpartyOfferedHTLCOutput, CounterpartyReceivedHTLCOutput, HolderFundingOutput, HolderHTLCOutput, PackageSolvingData, PackageTemplate, RevokedOutput, RevokedHTLCOutput};
 use chain::Filter;
 use util::logger::Logger;
-use util::ser::{Readable, ReadableArgs, MaybeReadable, Writer, Writeable, U48};
+use util::ser::{Readable, ReadableArgs, MaybeReadable, Writer, Writeable, U48, OptionDeserWrapper};
 use util::byte_utils;
 use util::events::Event;
 
 use prelude::*;
-use std::collections::{HashMap, HashSet};
 use core::{cmp, mem};
 use std::io::Error;
 use core::ops::Deref;
@@ -90,22 +89,26 @@ pub const CLOSED_CHANNEL_UPDATE_ID: u64 = core::u64::MAX;
 
 impl Writeable for ChannelMonitorUpdate {
        fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
+               write_ver_prefix!(w, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);
                self.update_id.write(w)?;
                (self.updates.len() as u64).write(w)?;
                for update_step in self.updates.iter() {
                        update_step.write(w)?;
                }
+               write_tlv_fields!(w, {});
                Ok(())
        }
 }
 impl Readable for ChannelMonitorUpdate {
        fn read<R: ::std::io::Read>(r: &mut R) -> Result<Self, DecodeError> {
+               let _ver = read_ver_prefix!(r, SERIALIZATION_VERSION);
                let update_id: u64 = Readable::read(r)?;
                let len: u64 = Readable::read(r)?;
                let mut updates = Vec::with_capacity(cmp::min(len as usize, MAX_ALLOC_SIZE / ::core::mem::size_of::<ChannelMonitorUpdateStep>()));
                for _ in 0..len {
                        updates.push(Readable::read(r)?);
                }
+               read_tlv_fields!(r, {});
                Ok(Self { update_id, updates })
        }
 }
@@ -198,7 +201,11 @@ pub struct HTLCUpdate {
        pub(crate) payment_preimage: Option<PaymentPreimage>,
        pub(crate) source: HTLCSource
 }
-impl_writeable!(HTLCUpdate, 0, { payment_hash, payment_preimage, source });
+impl_writeable_tlv_based!(HTLCUpdate, {
+       (0, payment_hash, required),
+       (2, source, required),
+       (4, payment_preimage, option),
+});
 
 /// If an HTLC expires within this many blocks, don't try to claim it in a shared transaction,
 /// instead claiming it in its own individual transaction.
@@ -264,6 +271,16 @@ struct HolderSignedTx {
        feerate_per_kw: u32,
        htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>,
 }
+impl_writeable_tlv_based!(HolderSignedTx, {
+       (0, txid, required),
+       (2, revocation_key, required),
+       (4, a_htlc_key, required),
+       (6, b_htlc_key, required),
+       (8, delayed_payment_key, required),
+       (10, per_commitment_point, required),
+       (12, feerate_per_kw, required),
+       (14, htlc_outputs, vec_type)
+});
 
 /// We use this to track counterparty commitment transactions and htlcs outputs and
 /// use it to generate any justice or 2nd-stage preimage/timeout transactions.
@@ -277,9 +294,6 @@ struct CounterpartyCommitmentTransaction {
 
 impl Writeable for CounterpartyCommitmentTransaction {
        fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
-               self.counterparty_delayed_payment_base_key.write(w)?;
-               self.counterparty_htlc_base_key.write(w)?;
-               w.write_all(&byte_utils::be16_to_array(self.on_counterparty_tx_csv))?;
                w.write_all(&byte_utils::be64_to_array(self.per_htlc.len() as u64))?;
                for (ref txid, ref htlcs) in self.per_htlc.iter() {
                        w.write_all(&txid[..])?;
@@ -288,15 +302,17 @@ impl Writeable for CounterpartyCommitmentTransaction {
                                htlc.write(w)?;
                        }
                }
+               write_tlv_fields!(w, {
+                       (0, self.counterparty_delayed_payment_base_key, required),
+                       (2, self.counterparty_htlc_base_key, required),
+                       (4, self.on_counterparty_tx_csv, required),
+               });
                Ok(())
        }
 }
 impl Readable for CounterpartyCommitmentTransaction {
        fn read<R: ::std::io::Read>(r: &mut R) -> Result<Self, DecodeError> {
                let counterparty_commitment_transaction = {
-                       let counterparty_delayed_payment_base_key = Readable::read(r)?;
-                       let counterparty_htlc_base_key = Readable::read(r)?;
-                       let on_counterparty_tx_csv: u16 = Readable::read(r)?;
                        let per_htlc_len: u64 = Readable::read(r)?;
                        let mut per_htlc = HashMap::with_capacity(cmp::min(per_htlc_len as usize, MAX_ALLOC_SIZE / 64));
                        for _  in 0..per_htlc_len {
@@ -311,9 +327,17 @@ impl Readable for CounterpartyCommitmentTransaction {
                                        return Err(DecodeError::InvalidValue);
                                }
                        }
+                       let mut counterparty_delayed_payment_base_key = OptionDeserWrapper(None);
+                       let mut counterparty_htlc_base_key = OptionDeserWrapper(None);
+                       let mut on_counterparty_tx_csv: u16 = 0;
+                       read_tlv_fields!(r, {
+                               (0, counterparty_delayed_payment_base_key, required),
+                               (2, counterparty_htlc_base_key, required),
+                               (4, on_counterparty_tx_csv, required),
+                       });
                        CounterpartyCommitmentTransaction {
-                               counterparty_delayed_payment_base_key,
-                               counterparty_htlc_base_key,
+                               counterparty_delayed_payment_base_key: counterparty_delayed_payment_base_key.0.unwrap(),
+                               counterparty_htlc_base_key: counterparty_htlc_base_key.0.unwrap(),
                                on_counterparty_tx_csv,
                                per_htlc,
                        }
@@ -335,11 +359,19 @@ struct OnchainEventEntry {
 
 impl OnchainEventEntry {
        fn confirmation_threshold(&self) -> u32 {
-               self.height + ANTI_REORG_DELAY - 1
+               let mut conf_threshold = self.height + ANTI_REORG_DELAY - 1;
+               if let OnchainEvent::MaturingOutput {
+                       descriptor: SpendableOutputDescriptor::DelayedPaymentOutput(ref descriptor)
+               } = self.event {
+                       // A CSV'd transaction is confirmable in block (input height) + CSV delay, which means
+                       // it's broadcastable when we see the previous block.
+                       conf_threshold = cmp::max(conf_threshold, self.height + descriptor.to_self_delay as u32 - 1);
+               }
+               conf_threshold
        }
 
-       fn has_reached_confirmation_threshold(&self, height: u32) -> bool {
-               height >= self.confirmation_threshold()
+       fn has_reached_confirmation_threshold(&self, best_block: &BestBlock) -> bool {
+               best_block.height() >= self.confirmation_threshold()
        }
 }
 
@@ -351,13 +383,30 @@ enum OnchainEvent {
        /// inbound HTLC in backward channel. Note, in case of preimage, we pass info to upstream without delay as we can
        /// only win from it, so it's never an OnchainEvent
        HTLCUpdate {
-               htlc_update: (HTLCSource, PaymentHash),
+               source: HTLCSource,
+               payment_hash: PaymentHash,
        },
        MaturingOutput {
                descriptor: SpendableOutputDescriptor,
        },
 }
 
+impl_writeable_tlv_based!(OnchainEventEntry, {
+       (0, txid, required),
+       (2, height, required),
+       (4, event, required),
+});
+
+impl_writeable_tlv_based_enum!(OnchainEvent,
+       (0, HTLCUpdate) => {
+               (0, source, required),
+               (2, payment_hash, required),
+       },
+       (1, MaturingOutput) => {
+               (0, descriptor, required),
+       },
+;);
+
 #[cfg_attr(any(test, feature = "fuzztarget", feature = "_test_utils"), derive(PartialEq))]
 #[derive(Clone)]
 pub(crate) enum ChannelMonitorUpdateStep {
@@ -387,98 +436,28 @@ pub(crate) enum ChannelMonitorUpdateStep {
        },
 }
 
-impl Writeable for ChannelMonitorUpdateStep {
-       fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
-               match self {
-                       &ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo { ref commitment_tx, ref htlc_outputs } => {
-                               0u8.write(w)?;
-                               commitment_tx.write(w)?;
-                               (htlc_outputs.len() as u64).write(w)?;
-                               for &(ref output, ref signature, ref source) in htlc_outputs.iter() {
-                                       output.write(w)?;
-                                       signature.write(w)?;
-                                       source.write(w)?;
-                               }
-                       }
-                       &ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { commitment_txid, ref htlc_outputs, ref commitment_number, ref their_revocation_point } => {
-                               1u8.write(w)?;
-                               commitment_txid.write(w)?;
-                               commitment_number.write(w)?;
-                               their_revocation_point.write(w)?;
-                               (htlc_outputs.len() as u64).write(w)?;
-                               for &(ref output, ref source) in htlc_outputs.iter() {
-                                       output.write(w)?;
-                                       source.as_ref().map(|b| b.as_ref()).write(w)?;
-                               }
-                       },
-                       &ChannelMonitorUpdateStep::PaymentPreimage { ref payment_preimage } => {
-                               2u8.write(w)?;
-                               payment_preimage.write(w)?;
-                       },
-                       &ChannelMonitorUpdateStep::CommitmentSecret { ref idx, ref secret } => {
-                               3u8.write(w)?;
-                               idx.write(w)?;
-                               secret.write(w)?;
-                       },
-                       &ChannelMonitorUpdateStep::ChannelForceClosed { ref should_broadcast } => {
-                               4u8.write(w)?;
-                               should_broadcast.write(w)?;
-                       },
-               }
-               Ok(())
-       }
-}
-impl Readable for ChannelMonitorUpdateStep {
-       fn read<R: ::std::io::Read>(r: &mut R) -> Result<Self, DecodeError> {
-               match Readable::read(r)? {
-                       0u8 => {
-                               Ok(ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo {
-                                       commitment_tx: Readable::read(r)?,
-                                       htlc_outputs: {
-                                               let len: u64 = Readable::read(r)?;
-                                               let mut res = Vec::new();
-                                               for _ in 0..len {
-                                                       res.push((Readable::read(r)?, Readable::read(r)?, Readable::read(r)?));
-                                               }
-                                               res
-                                       },
-                               })
-                       },
-                       1u8 => {
-                               Ok(ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo {
-                                       commitment_txid: Readable::read(r)?,
-                                       commitment_number: Readable::read(r)?,
-                                       their_revocation_point: Readable::read(r)?,
-                                       htlc_outputs: {
-                                               let len: u64 = Readable::read(r)?;
-                                               let mut res = Vec::new();
-                                               for _ in 0..len {
-                                                       res.push((Readable::read(r)?, <Option<HTLCSource> as Readable>::read(r)?.map(|o| Box::new(o))));
-                                               }
-                                               res
-                                       },
-                               })
-                       },
-                       2u8 => {
-                               Ok(ChannelMonitorUpdateStep::PaymentPreimage {
-                                       payment_preimage: Readable::read(r)?,
-                               })
-                       },
-                       3u8 => {
-                               Ok(ChannelMonitorUpdateStep::CommitmentSecret {
-                                       idx: Readable::read(r)?,
-                                       secret: Readable::read(r)?,
-                               })
-                       },
-                       4u8 => {
-                               Ok(ChannelMonitorUpdateStep::ChannelForceClosed {
-                                       should_broadcast: Readable::read(r)?
-                               })
-                       },
-                       _ => Err(DecodeError::InvalidValue),
-               }
-       }
-}
+impl_writeable_tlv_based_enum!(ChannelMonitorUpdateStep,
+       (0, LatestHolderCommitmentTXInfo) => {
+               (0, commitment_tx, required),
+               (2, htlc_outputs, vec_type),
+       },
+       (1, LatestCounterpartyCommitmentTXInfo) => {
+               (0, commitment_txid, required),
+               (2, commitment_number, required),
+               (4, their_revocation_point, required),
+               (6, htlc_outputs, vec_type),
+       },
+       (2, PaymentPreimage) => {
+               (0, payment_preimage, required),
+       },
+       (3, CommitmentSecret) => {
+               (0, idx, required),
+               (2, secret, required),
+       },
+       (4, ChannelForceClosed) => {
+               (0, should_broadcast, required),
+       },
+;);
 
 /// A ChannelMonitor handles chain events (blocks connected and disconnected) and generates
 /// on-chain transactions to ensure no loss of funds occurs.
@@ -662,6 +641,7 @@ impl<Signer: Sign> Writeable for ChannelMonitor<Signer> {
        }
 }
 
+// These are also used for ChannelMonitorUpdate, above.
 const SERIALIZATION_VERSION: u8 = 1;
 const MIN_SERIALIZATION_VERSION: u8 = 1;
 
@@ -753,38 +733,14 @@ impl<Signer: Sign> Writeable for ChannelMonitorImpl<Signer> {
                        writer.write_all(&byte_utils::be48_to_array(*commitment_number))?;
                }
 
-               macro_rules! serialize_holder_tx {
-                       ($holder_tx: expr) => {
-                               $holder_tx.txid.write(writer)?;
-                               writer.write_all(&$holder_tx.revocation_key.serialize())?;
-                               writer.write_all(&$holder_tx.a_htlc_key.serialize())?;
-                               writer.write_all(&$holder_tx.b_htlc_key.serialize())?;
-                               writer.write_all(&$holder_tx.delayed_payment_key.serialize())?;
-                               writer.write_all(&$holder_tx.per_commitment_point.serialize())?;
-
-                               writer.write_all(&byte_utils::be32_to_array($holder_tx.feerate_per_kw))?;
-                               writer.write_all(&byte_utils::be64_to_array($holder_tx.htlc_outputs.len() as u64))?;
-                               for &(ref htlc_output, ref sig, ref htlc_source) in $holder_tx.htlc_outputs.iter() {
-                                       serialize_htlc_in_commitment!(htlc_output);
-                                       if let &Some(ref their_sig) = sig {
-                                               1u8.write(writer)?;
-                                               writer.write_all(&their_sig.serialize_compact())?;
-                                       } else {
-                                               0u8.write(writer)?;
-                                       }
-                                       htlc_source.write(writer)?;
-                               }
-                       }
-               }
-
                if let Some(ref prev_holder_tx) = self.prev_holder_signed_commitment_tx {
                        writer.write_all(&[1; 1])?;
-                       serialize_holder_tx!(prev_holder_tx);
+                       prev_holder_tx.write(writer)?;
                } else {
                        writer.write_all(&[0; 1])?;
                }
 
-               serialize_holder_tx!(self.current_holder_commitment_tx);
+               self.current_holder_commitment_tx.write(writer)?;
 
                writer.write_all(&byte_utils::be48_to_array(self.current_counterparty_commitment_number))?;
                writer.write_all(&byte_utils::be48_to_array(self.current_holder_commitment_number))?;
@@ -815,19 +771,7 @@ impl<Signer: Sign> Writeable for ChannelMonitorImpl<Signer> {
 
                writer.write_all(&byte_utils::be64_to_array(self.onchain_events_awaiting_threshold_conf.len() as u64))?;
                for ref entry in self.onchain_events_awaiting_threshold_conf.iter() {
-                       entry.txid.write(writer)?;
-                       writer.write_all(&byte_utils::be32_to_array(entry.height))?;
-                       match entry.event {
-                               OnchainEvent::HTLCUpdate { ref htlc_update } => {
-                                       0u8.write(writer)?;
-                                       htlc_update.0.write(writer)?;
-                                       htlc_update.1.write(writer)?;
-                               },
-                               OnchainEvent::MaturingOutput { ref descriptor } => {
-                                       1u8.write(writer)?;
-                                       descriptor.write(writer)?;
-                               },
-                       }
+                       entry.write(writer)?;
                }
 
                (self.outputs_to_watch.len() as u64).write(writer)?;
@@ -844,7 +788,7 @@ impl<Signer: Sign> Writeable for ChannelMonitorImpl<Signer> {
                self.lockdown_from_offchain.write(writer)?;
                self.holder_tx_signed.write(writer)?;
 
-               write_tlv_fields!(writer, {}, {});
+               write_tlv_fields!(writer, {});
 
                Ok(())
        }
@@ -1245,6 +1189,12 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
                txids.dedup();
                txids
        }
+
+       /// Gets the latest best block which was connected either via the [`chain::Listen`] or
+       /// [`chain::Confirm`] interfaces.
+       pub fn current_best_block(&self) -> BestBlock {
+               self.inner.lock().unwrap().best_block.clone()
+       }
 }
 
 impl<Signer: Sign> ChannelMonitorImpl<Signer> {
@@ -1387,7 +1337,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                macro_rules! claim_htlcs {
                        ($commitment_number: expr, $txid: expr) => {
                                let htlc_claim_reqs = self.get_counterparty_htlc_output_claim_reqs($commitment_number, $txid, None);
-                               self.onchain_tx_handler.update_claims_view(&Vec::new(), htlc_claim_reqs, self.best_block.height(), broadcaster, fee_estimator, logger);
+                               self.onchain_tx_handler.update_claims_view(&Vec::new(), htlc_claim_reqs, self.best_block.height(), self.best_block.height(), broadcaster, fee_estimator, logger);
                        }
                }
                if let Some(txid) = self.current_counterparty_commitment_txid {
@@ -1409,11 +1359,14 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                // *we* sign a holder commitment transaction, not when e.g. a watchtower broadcasts one of our
                // holder commitment transactions.
                if self.broadcasted_holder_revokable_script.is_some() {
-                       let (claim_reqs, _) = self.get_broadcasted_holder_claims(&self.current_holder_commitment_tx, 0);
-                       self.onchain_tx_handler.update_claims_view(&Vec::new(), claim_reqs, self.best_block.height(), broadcaster, fee_estimator, logger);
+                       // Assume that the broadcasted commitment transaction confirmed in the current best
+                       // block. Even if not, its a reasonable metric for the bump criteria on the HTLC
+                       // transactions.
+                       let (claim_reqs, _) = self.get_broadcasted_holder_claims(&self.current_holder_commitment_tx, self.best_block.height());
+                       self.onchain_tx_handler.update_claims_view(&Vec::new(), claim_reqs, self.best_block.height(), self.best_block.height(), broadcaster, fee_estimator, logger);
                        if let Some(ref tx) = self.prev_holder_signed_commitment_tx {
-                               let (claim_reqs, _) = self.get_broadcasted_holder_claims(&tx, 0);
-                               self.onchain_tx_handler.update_claims_view(&Vec::new(), claim_reqs, self.best_block.height(), broadcaster, fee_estimator, logger);
+                               let (claim_reqs, _) = self.get_broadcasted_holder_claims(&tx, self.best_block.height());
+                               self.onchain_tx_handler.update_claims_view(&Vec::new(), claim_reqs, self.best_block.height(), self.best_block.height(), broadcaster, fee_estimator, logger);
                        }
                }
        }
@@ -1595,7 +1548,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                        // Last, track onchain revoked commitment transaction and fail backward outgoing HTLCs as payment path is broken
                        if !claimable_outpoints.is_empty() || per_commitment_option.is_some() { // ie we're confident this is actually ours
                                // We're definitely a counterparty commitment transaction!
-                               log_trace!(logger, "Got broadcast of revoked counterparty commitment transaction, going to generate general spend tx with {} inputs", claimable_outpoints.len());
+                               log_error!(logger, "Got broadcast of revoked counterparty commitment transaction, going to generate general spend tx with {} inputs", claimable_outpoints.len());
                                for (idx, outp) in tx.output.iter().enumerate() {
                                        watch_outputs.push((idx as u32, outp.clone()));
                                }
@@ -1609,17 +1562,18 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                                                                        self.onchain_events_awaiting_threshold_conf.retain(|ref entry| {
                                                                                if entry.height != height { return true; }
                                                                                match entry.event {
-                                                                                        OnchainEvent::HTLCUpdate { ref htlc_update } => {
-                                                                                                htlc_update.0 != **source
-                                                                                        },
-                                                                                        _ => true,
+                                                                                       OnchainEvent::HTLCUpdate { source: ref update_source, .. } => {
+                                                                                               *update_source != **source
+                                                                                       },
+                                                                                       _ => true,
                                                                                }
                                                                        });
                                                                        let entry = OnchainEventEntry {
                                                                                txid: *$txid,
                                                                                height,
                                                                                event: OnchainEvent::HTLCUpdate {
-                                                                                       htlc_update: ((**source).clone(), htlc.payment_hash.clone())
+                                                                                       source: (**source).clone(),
+                                                                                       payment_hash: htlc.payment_hash.clone(),
                                                                                },
                                                                        };
                                                                        log_info!(logger, "Failing HTLC with payment_hash {} from {} counterparty commitment tx due to broadcast of revoked counterparty commitment transaction, waiting for confirmation (at height {})", log_bytes!(htlc.payment_hash.0), $commitment_tx, entry.confirmation_threshold());
@@ -1650,7 +1604,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                        }
                        self.counterparty_commitment_txn_on_chain.insert(commitment_txid, commitment_number);
 
-                       log_trace!(logger, "Got broadcast of non-revoked counterparty commitment transaction {}", commitment_txid);
+                       log_info!(logger, "Got broadcast of non-revoked counterparty commitment transaction {}", commitment_txid);
 
                        macro_rules! check_htlc_fails {
                                ($txid: expr, $commitment_tx: expr, $id: tt) => {
@@ -1675,17 +1629,18 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                                                                self.onchain_events_awaiting_threshold_conf.retain(|ref entry| {
                                                                        if entry.height != height { return true; }
                                                                        match entry.event {
-                                                                                OnchainEvent::HTLCUpdate { ref htlc_update } => {
-                                                                                        htlc_update.0 != **source
-                                                                                },
-                                                                                _ => true,
+                                                                               OnchainEvent::HTLCUpdate { source: ref update_source, .. } => {
+                                                                                       *update_source != **source
+                                                                               },
+                                                                               _ => true,
                                                                        }
                                                                });
                                                                self.onchain_events_awaiting_threshold_conf.push(OnchainEventEntry {
                                                                        txid: *$txid,
                                                                        height,
                                                                        event: OnchainEvent::HTLCUpdate {
-                                                                               htlc_update: ((**source).clone(), htlc.payment_hash.clone())
+                                                                               source: (**source).clone(),
+                                                                               payment_hash: htlc.payment_hash.clone(),
                                                                        },
                                                                });
                                                        }
@@ -1767,7 +1722,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                let per_commitment_key = ignore_error!(SecretKey::from_slice(&secret));
                let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key);
 
-               log_trace!(logger, "Counterparty HTLC broadcast {}:{}", htlc_txid, 0);
+               log_error!(logger, "Got broadcast of revoked counterparty HTLC transaction, spending {}:{}", htlc_txid, 0);
                let revk_outp = RevokedOutput::build(per_commitment_point, self.counterparty_tx_cache.counterparty_delayed_payment_base_key, self.counterparty_tx_cache.counterparty_htlc_base_key, per_commitment_key, tx.output[0].value, self.counterparty_tx_cache.on_counterparty_tx_csv);
                let justice_package = PackageTemplate::build_package(htlc_txid, 0, PackageSolvingData::RevokedOutput(revk_outp), height + self.counterparty_tx_cache.on_counterparty_tx_csv as u32, true, height);
                let claimable_outpoints = vec!(justice_package);
@@ -1778,7 +1733,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
        // Returns (1) `PackageTemplate`s that can be given to the OnChainTxHandler, so that the handler can
        // broadcast transactions claiming holder HTLC commitment outputs and (2) a holder revokable
        // script so we can detect whether a holder transaction has been seen on-chain.
-       fn get_broadcasted_holder_claims(&self, holder_tx: &HolderSignedTx, height: u32) -> (Vec<PackageTemplate>, Option<(Script, PublicKey, PublicKey)>) {
+       fn get_broadcasted_holder_claims(&self, holder_tx: &HolderSignedTx, conf_height: u32) -> (Vec<PackageTemplate>, Option<(Script, PublicKey, PublicKey)>) {
                let mut claim_requests = Vec::with_capacity(holder_tx.htlc_outputs.len());
 
                let redeemscript = chan_utils::get_revokeable_redeemscript(&holder_tx.revocation_key, self.on_holder_tx_csv, &holder_tx.delayed_payment_key);
@@ -1797,7 +1752,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                                                };
                                                HolderHTLCOutput::build_accepted(payment_preimage, htlc.amount_msat)
                                        };
-                               let htlc_package = PackageTemplate::build_package(holder_tx.txid, transaction_output_index, PackageSolvingData::HolderHTLCOutput(htlc_output), height, false, height);
+                               let htlc_package = PackageTemplate::build_package(holder_tx.txid, transaction_output_index, PackageSolvingData::HolderHTLCOutput(htlc_output), htlc.cltv_expiry, false, conf_height);
                                claim_requests.push(htlc_package);
                        }
                }
@@ -1829,16 +1784,16 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                                self.onchain_events_awaiting_threshold_conf.retain(|ref entry| {
                                        if entry.height != height { return true; }
                                        match entry.event {
-                                                OnchainEvent::HTLCUpdate { ref htlc_update } => {
-                                                        htlc_update.0 != $source
-                                                },
-                                                _ => true,
+                                               OnchainEvent::HTLCUpdate { source: ref update_source, .. } => {
+                                                       *update_source != $source
+                                               },
+                                               _ => true,
                                        }
                                });
                                let entry = OnchainEventEntry {
                                        txid: commitment_txid,
                                        height,
-                                       event: OnchainEvent::HTLCUpdate { htlc_update: ($source, $payment_hash) },
+                                       event: OnchainEvent::HTLCUpdate { source: $source, payment_hash: $payment_hash },
                                };
                                log_trace!(logger, "Failing HTLC with payment_hash {} from {} holder commitment tx due to broadcast of transaction, waiting confirmation (at height{})", log_bytes!($payment_hash.0), $commitment_tx, entry.confirmation_threshold());
                                self.onchain_events_awaiting_threshold_conf.push(entry);
@@ -1858,14 +1813,14 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 
                if self.current_holder_commitment_tx.txid == commitment_txid {
                        is_holder_tx = true;
-                       log_trace!(logger, "Got latest holder commitment tx broadcast, searching for available HTLCs to claim");
+                       log_info!(logger, "Got broadcast of latest holder commitment tx {}, searching for available HTLCs to claim", commitment_txid);
                        let res = self.get_broadcasted_holder_claims(&self.current_holder_commitment_tx, height);
                        let mut to_watch = self.get_broadcasted_holder_watch_outputs(&self.current_holder_commitment_tx, tx);
                        append_onchain_update!(res, to_watch);
                } else if let &Some(ref holder_tx) = &self.prev_holder_signed_commitment_tx {
                        if holder_tx.txid == commitment_txid {
                                is_holder_tx = true;
-                               log_trace!(logger, "Got previous holder commitment tx broadcast, searching for available HTLCs to claim");
+                               log_info!(logger, "Got broadcast of previous holder commitment tx {}, searching for available HTLCs to claim", commitment_txid);
                                let res = self.get_broadcasted_holder_claims(holder_tx, height);
                                let mut to_watch = self.get_broadcasted_holder_watch_outputs(holder_tx, tx);
                                append_onchain_update!(res, to_watch);
@@ -1895,7 +1850,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
        }
 
        pub fn get_latest_holder_commitment_txn<L: Deref>(&mut self, logger: &L) -> Vec<Transaction> where L::Target: Logger {
-               log_trace!(logger, "Getting signed latest holder commitment transaction!");
+               log_debug!(logger, "Getting signed latest holder commitment transaction!");
                self.holder_tx_signed = true;
                let commitment_tx = self.onchain_tx_handler.get_fully_signed_holder_tx(&self.funding_redeemscript);
                let txid = commitment_tx.txid();
@@ -1910,7 +1865,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                                } else if htlc.0.cltv_expiry > self.best_block.height() + 1 {
                                        // Don't broadcast HTLC-Timeout transactions immediately as they don't meet the
                                        // current locktime requirements on-chain. We will broadcast them in
-                                       // `block_confirmed` when `would_broadcast_at_height` returns true.
+                                       // `block_confirmed` when `should_broadcast_holder_commitment_txn` returns true.
                                        // Note that we add + 1 as transactions are broadcastable when they can be
                                        // confirmed in the next block.
                                        continue;
@@ -1929,7 +1884,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
        #[cfg(any(test,feature = "unsafe_revoked_tx_signing"))]
        /// Note that this includes possibly-locktimed-in-the-future transactions!
        fn unsafe_get_latest_holder_commitment_txn<L: Deref>(&mut self, logger: &L) -> Vec<Transaction> where L::Target: Logger {
-               log_trace!(logger, "Getting signed copy of latest holder commitment transaction!");
+               log_debug!(logger, "Getting signed copy of latest holder commitment transaction!");
                let commitment_tx = self.onchain_tx_handler.get_fully_signed_copy_holder_tx(&self.funding_redeemscript);
                let txid = commitment_tx.txid();
                let mut holder_transactions = vec![commitment_tx];
@@ -1980,13 +1935,13 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 
                if height > self.best_block.height() {
                        self.best_block = BestBlock::new(block_hash, height);
-                       self.block_confirmed(height, vec![], vec![], vec![], broadcaster, fee_estimator, logger)
-               } else {
+                       self.block_confirmed(height, vec![], vec![], vec![], &broadcaster, &fee_estimator, &logger)
+               } else if block_hash != self.best_block.block_hash() {
                        self.best_block = BestBlock::new(block_hash, height);
                        self.onchain_events_awaiting_threshold_conf.retain(|ref entry| entry.height <= height);
                        self.onchain_tx_handler.block_disconnected(height + 1, broadcaster, fee_estimator, logger);
                        Vec::new()
-               }
+               } else { Vec::new() }
        }
 
        fn transactions_confirmed<B: Deref, F: Deref, L: Deref>(
@@ -2058,33 +2013,49 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                        self.is_paying_spendable_output(&tx, height, &logger);
                }
 
-               self.block_confirmed(height, txn_matched, watch_outputs, claimable_outpoints, broadcaster, fee_estimator, logger)
+               if height > self.best_block.height() {
+                       self.best_block = BestBlock::new(block_hash, height);
+               }
+
+               self.block_confirmed(height, txn_matched, watch_outputs, claimable_outpoints, &broadcaster, &fee_estimator, &logger)
        }
 
+       /// Update state for new block(s)/transaction(s) confirmed. Note that the caller must update
+       /// `self.best_block` before calling if a new best blockchain tip is available. More
+       /// concretely, `self.best_block` must never be at a lower height than `conf_height`, avoiding
+       /// complexity especially in `OnchainTx::update_claims_view`.
+       ///
+       /// `conf_height` should be set to the height at which any new transaction(s)/block(s) were
+       /// confirmed at, even if it is not the current best height.
        fn block_confirmed<B: Deref, F: Deref, L: Deref>(
                &mut self,
-               height: u32,
+               conf_height: u32,
                txn_matched: Vec<&Transaction>,
                mut watch_outputs: Vec<TransactionOutputs>,
                mut claimable_outpoints: Vec<PackageTemplate>,
-               broadcaster: B,
-               fee_estimator: F,
-               logger: L,
+               broadcaster: &B,
+               fee_estimator: &F,
+               logger: &L,
        ) -> Vec<TransactionOutputs>
        where
                B::Target: BroadcasterInterface,
                F::Target: FeeEstimator,
                L::Target: Logger,
        {
-               let should_broadcast = self.would_broadcast_at_height(height, &logger);
+               debug_assert!(self.best_block.height() >= conf_height);
+
+               let should_broadcast = self.should_broadcast_holder_commitment_txn(logger);
                if should_broadcast {
                        let funding_outp = HolderFundingOutput::build(self.funding_redeemscript.clone());
-                       let commitment_package = PackageTemplate::build_package(self.funding_info.0.txid.clone(), self.funding_info.0.index as u32, PackageSolvingData::HolderFundingOutput(funding_outp), height, false, height);
+                       let commitment_package = PackageTemplate::build_package(self.funding_info.0.txid.clone(), self.funding_info.0.index as u32, PackageSolvingData::HolderFundingOutput(funding_outp), self.best_block.height(), false, self.best_block.height());
                        claimable_outpoints.push(commitment_package);
                        self.pending_monitor_events.push(MonitorEvent::CommitmentTxBroadcasted(self.funding_info.0));
                        let commitment_tx = self.onchain_tx_handler.get_fully_signed_holder_tx(&self.funding_redeemscript);
                        self.holder_tx_signed = true;
-                       let (mut new_outpoints, _) = self.get_broadcasted_holder_claims(&self.current_holder_commitment_tx, height);
+                       // Because we're broadcasting a commitment transaction, we should construct the package
+                       // assuming it gets confirmed in the next block. Sadly, we have code which considers
+                       // "not yet confirmed" things as discardable, so we cannot do that here.
+                       let (mut new_outpoints, _) = self.get_broadcasted_holder_claims(&self.current_holder_commitment_tx, self.best_block.height());
                        let new_outputs = self.get_broadcasted_holder_watch_outputs(&self.current_holder_commitment_tx, &commitment_tx);
                        if !new_outputs.is_empty() {
                                watch_outputs.push((self.current_holder_commitment_tx.txid.clone(), new_outputs));
@@ -2097,7 +2068,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                        self.onchain_events_awaiting_threshold_conf.drain(..).collect::<Vec<_>>();
                let mut onchain_events_reaching_threshold_conf = Vec::new();
                for entry in onchain_events_awaiting_threshold_conf {
-                       if entry.has_reached_confirmation_threshold(height) {
+                       if entry.has_reached_confirmation_threshold(&self.best_block) {
                                onchain_events_reaching_threshold_conf.push(entry);
                        } else {
                                self.onchain_events_awaiting_threshold_conf.push(entry);
@@ -2109,7 +2080,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                let unmatured_htlcs: Vec<_> = self.onchain_events_awaiting_threshold_conf
                        .iter()
                        .filter_map(|entry| match &entry.event {
-                               OnchainEvent::HTLCUpdate { htlc_update } => Some(htlc_update.0.clone()),
+                               OnchainEvent::HTLCUpdate { source, .. } => Some(source),
                                OnchainEvent::MaturingOutput { .. } => None,
                        })
                        .collect();
@@ -2119,32 +2090,32 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                // Produce actionable events from on-chain events having reached their threshold.
                for entry in onchain_events_reaching_threshold_conf.drain(..) {
                        match entry.event {
-                               OnchainEvent::HTLCUpdate { htlc_update } => {
+                               OnchainEvent::HTLCUpdate { ref source, payment_hash } => {
                                        // Check for duplicate HTLC resolutions.
                                        #[cfg(debug_assertions)]
                                        {
                                                debug_assert!(
-                                                       unmatured_htlcs.iter().find(|&htlc| htlc == &htlc_update.0).is_none(),
+                                                       unmatured_htlcs.iter().find(|&htlc| htlc == &source).is_none(),
                                                        "An unmature HTLC transaction conflicts with a maturing one; failed to \
                                                         call either transaction_unconfirmed for the conflicting transaction \
                                                         or block_disconnected for a block containing it.");
                                                debug_assert!(
-                                                       matured_htlcs.iter().find(|&htlc| htlc == &htlc_update.0).is_none(),
+                                                       matured_htlcs.iter().find(|&htlc| htlc == source).is_none(),
                                                        "A matured HTLC transaction conflicts with a maturing one; failed to \
                                                         call either transaction_unconfirmed for the conflicting transaction \
                                                         or block_disconnected for a block containing it.");
-                                               matured_htlcs.push(htlc_update.0.clone());
+                                               matured_htlcs.push(source.clone());
                                        }
 
-                                       log_trace!(logger, "HTLC {} failure update has got enough confirmations to be passed upstream", log_bytes!((htlc_update.1).0));
+                                       log_debug!(logger, "HTLC {} failure update has got enough confirmations to be passed upstream", log_bytes!(payment_hash.0));
                                        self.pending_monitor_events.push(MonitorEvent::HTLCEvent(HTLCUpdate {
-                                               payment_hash: htlc_update.1,
+                                               payment_hash: payment_hash,
                                                payment_preimage: None,
-                                               source: htlc_update.0,
+                                               source: source.clone(),
                                        }));
                                },
                                OnchainEvent::MaturingOutput { descriptor } => {
-                                       log_trace!(logger, "Descriptor {} has got enough confirmations to be passed upstream", log_spendable!(descriptor));
+                                       log_debug!(logger, "Descriptor {} has got enough confirmations to be passed upstream", log_spendable!(descriptor));
                                        self.pending_events.push(Event::SpendableOutputs {
                                                outputs: vec![descriptor]
                                        });
@@ -2152,7 +2123,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                        }
                }
 
-               self.onchain_tx_handler.update_claims_view(&txn_matched, claimable_outpoints, height, &&*broadcaster, &&*fee_estimator, &&*logger);
+               self.onchain_tx_handler.update_claims_view(&txn_matched, claimable_outpoints, conf_height, self.best_block.height(), broadcaster, fee_estimator, logger);
 
                // Determine new outputs to watch by comparing against previously known outputs to watch,
                // updating the latter in the process.
@@ -2254,7 +2225,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                false
        }
 
-       fn would_broadcast_at_height<L: Deref>(&self, height: u32, logger: &L) -> bool where L::Target: Logger {
+       fn should_broadcast_holder_commitment_txn<L: Deref>(&self, logger: &L) -> bool where L::Target: Logger {
                // We need to consider all HTLCs which are:
                //  * in any unrevoked counterparty commitment transaction, as they could broadcast said
                //    transactions and we'd end up in a race, or
@@ -2265,6 +2236,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                // to the source, and if we don't fail the channel we will have to ensure that the next
                // updates that peer sends us are update_fails, failing the channel if not. It's probably
                // easier to just fail the channel as this case should be rare enough anyway.
+               let height = self.best_block.height();
                macro_rules! scan_commitment {
                        ($htlcs: expr, $holder_tx: expr) => {
                                for ref htlc in $htlcs {
@@ -2437,18 +2409,18 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                                        self.onchain_events_awaiting_threshold_conf.retain(|ref entry| {
                                                if entry.height != height { return true; }
                                                match entry.event {
-                                                        OnchainEvent::HTLCUpdate { ref htlc_update } => {
-                                                                htlc_update.0 != source
-                                                        },
-                                                        _ => true,
+                                                       OnchainEvent::HTLCUpdate { source: ref htlc_source, .. } => {
+                                                               *htlc_source != source
+                                                       },
+                                                       _ => true,
                                                }
                                        });
                                        let entry = OnchainEventEntry {
                                                txid: tx.txid(),
                                                height,
-                                               event: OnchainEvent::HTLCUpdate { htlc_update: (source, payment_hash) },
+                                               event: OnchainEvent::HTLCUpdate { source: source, payment_hash: payment_hash },
                                        };
-                                       log_info!(logger, "Failing HTLC with payment_hash {} timeout by a spend tx, waiting for confirmation (at height{})", log_bytes!(payment_hash.0), entry.confirmation_threshold());
+                                       log_info!(logger, "Failing HTLC with payment_hash {} timeout by a spend tx, waiting for confirmation (at height {})", log_bytes!(payment_hash.0), entry.confirmation_threshold());
                                        self.onchain_events_awaiting_threshold_conf.push(entry);
                                }
                        }
@@ -2513,7 +2485,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                                height: height,
                                event: OnchainEvent::MaturingOutput { descriptor: spendable_output.clone() },
                        };
-                       log_trace!(logger, "Maturing {} until {}", log_spendable!(spendable_output), entry.confirmation_threshold());
+                       log_info!(logger, "Received spendable output {}, spendable at height {}", log_spendable!(spendable_output), entry.confirmation_threshold());
                        self.onchain_events_awaiting_threshold_conf.push(entry);
                }
        }
@@ -2723,46 +2695,14 @@ impl<'a, Signer: Sign, K: KeysInterface<Signer = Signer>> ReadableArgs<&'a K>
                        }
                }
 
-               macro_rules! read_holder_tx {
-                       () => {
-                               {
-                                       let txid = Readable::read(reader)?;
-                                       let revocation_key = Readable::read(reader)?;
-                                       let a_htlc_key = Readable::read(reader)?;
-                                       let b_htlc_key = Readable::read(reader)?;
-                                       let delayed_payment_key = Readable::read(reader)?;
-                                       let per_commitment_point = Readable::read(reader)?;
-                                       let feerate_per_kw: u32 = Readable::read(reader)?;
-
-                                       let htlcs_len: u64 = Readable::read(reader)?;
-                                       let mut htlcs = Vec::with_capacity(cmp::min(htlcs_len as usize, MAX_ALLOC_SIZE / 128));
-                                       for _ in 0..htlcs_len {
-                                               let htlc = read_htlc_in_commitment!();
-                                               let sigs = match <u8 as Readable>::read(reader)? {
-                                                       0 => None,
-                                                       1 => Some(Readable::read(reader)?),
-                                                       _ => return Err(DecodeError::InvalidValue),
-                                               };
-                                               htlcs.push((htlc, sigs, Readable::read(reader)?));
-                                       }
-
-                                       HolderSignedTx {
-                                               txid,
-                                               revocation_key, a_htlc_key, b_htlc_key, delayed_payment_key, per_commitment_point, feerate_per_kw,
-                                               htlc_outputs: htlcs
-                                       }
-                               }
-                       }
-               }
-
                let prev_holder_signed_commitment_tx = match <u8 as Readable>::read(reader)? {
                        0 => None,
                        1 => {
-                               Some(read_holder_tx!())
+                               Some(Readable::read(reader)?)
                        },
                        _ => return Err(DecodeError::InvalidValue),
                };
-               let current_holder_commitment_tx = read_holder_tx!();
+               let current_holder_commitment_tx = Readable::read(reader)?;
 
                let current_counterparty_commitment_number = <U48 as Readable>::read(reader)?.0;
                let current_holder_commitment_number = <U48 as Readable>::read(reader)?.0;
@@ -2801,25 +2741,7 @@ impl<'a, Signer: Sign, K: KeysInterface<Signer = Signer>> ReadableArgs<&'a K>
                let waiting_threshold_conf_len: u64 = Readable::read(reader)?;
                let mut onchain_events_awaiting_threshold_conf = Vec::with_capacity(cmp::min(waiting_threshold_conf_len as usize, MAX_ALLOC_SIZE / 128));
                for _ in 0..waiting_threshold_conf_len {
-                       let txid = Readable::read(reader)?;
-                       let height = Readable::read(reader)?;
-                       let event = match <u8 as Readable>::read(reader)? {
-                               0 => {
-                                       let htlc_source = Readable::read(reader)?;
-                                       let hash = Readable::read(reader)?;
-                                       OnchainEvent::HTLCUpdate {
-                                               htlc_update: (htlc_source, hash)
-                                       }
-                               },
-                               1 => {
-                                       let descriptor = Readable::read(reader)?;
-                                       OnchainEvent::MaturingOutput {
-                                               descriptor
-                                       }
-                               },
-                               _ => return Err(DecodeError::InvalidValue),
-                       };
-                       onchain_events_awaiting_threshold_conf.push(OnchainEventEntry { txid, height, event });
+                       onchain_events_awaiting_threshold_conf.push(Readable::read(reader)?);
                }
 
                let outputs_to_watch_len: u64 = Readable::read(reader)?;
@@ -2840,7 +2762,7 @@ impl<'a, Signer: Sign, K: KeysInterface<Signer = Signer>> ReadableArgs<&'a K>
                let lockdown_from_offchain = Readable::read(reader)?;
                let holder_tx_signed = Readable::read(reader)?;
 
-               read_tlv_fields!(reader, {}, {});
+               read_tlv_fields!(reader, {});
 
                let mut secp_ctx = Secp256k1::new();
                secp_ctx.seeded_randomize(&keys_manager.get_secure_random_bytes());
@@ -2911,11 +2833,11 @@ mod tests {
        use bitcoin::hash_types::Txid;
        use bitcoin::network::constants::Network;
        use hex;
+       use chain::BestBlock;
        use chain::channelmonitor::ChannelMonitor;
        use chain::package::{WEIGHT_OFFERED_HTLC, WEIGHT_RECEIVED_HTLC, WEIGHT_REVOKED_OFFERED_HTLC, WEIGHT_REVOKED_RECEIVED_HTLC, WEIGHT_REVOKED_OUTPUT};
        use chain::transaction::OutPoint;
        use ln::{PaymentPreimage, PaymentHash};
-       use ln::channelmanager::BestBlock;
        use ln::chan_utils;
        use ln::chan_utils::{HTLCOutputInCommitment, ChannelPublicKeys, ChannelTransactionParameters, HolderCommitmentTransaction, CounterpartyChannelTransactionParameters};
        use util::test_utils::{TestLogger, TestBroadcaster, TestFeeEstimator};
@@ -2930,7 +2852,7 @@ mod tests {
                let secp_ctx = Secp256k1::new();
                let logger = Arc::new(TestLogger::new());
                let broadcaster = Arc::new(TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new()), blocks: Arc::new(Mutex::new(Vec::new()))});
-               let fee_estimator = Arc::new(TestFeeEstimator { sat_per_kw: 253 });
+               let fee_estimator = Arc::new(TestFeeEstimator { sat_per_kw: Mutex::new(253) });
 
                let dummy_key = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
                let dummy_tx = Transaction { version: 0, lock_time: 0, input: Vec::new(), output: Vec::new() };