Make ChannelMonitor clonable again
[rust-lightning] / lightning / src / chain / channelmonitor.rs
index 5128600163a42854f0be6c2bb5f27f79608050e6..6daab991922f160be0d4062f5cb392545f69789e 100644 (file)
@@ -40,7 +40,7 @@ use ln::chan_utils::{CounterpartyCommitmentSecrets, HTLCOutputInCommitment, HTLC
 use ln::channelmanager::HTLCSource;
 use chain;
 use chain::{BestBlock, WatchedOutput};
-use chain::chaininterface::{BroadcasterInterface, FeeEstimator};
+use chain::chaininterface::{BroadcasterInterface, FeeEstimator, LowerBoundedFeeEstimator};
 use chain::transaction::{OutPoint, TransactionData};
 use chain::keysinterface::{SpendableOutputDescriptor, StaticPaymentOutputDescriptor, DelayedPaymentOutputDescriptor, Sign, KeysInterface};
 use chain::onchaintx::OnchainTxHandler;
@@ -256,7 +256,7 @@ impl_writeable_tlv_based!(HolderSignedTx, {
 
 /// We use this to track static counterparty commitment transaction data and to generate any
 /// justice or 2nd-stage preimage/timeout transactions.
-#[derive(PartialEq)]
+#[derive(Clone, PartialEq)]
 struct CounterpartyCommitmentParameters {
        counterparty_delayed_payment_base_key: PublicKey,
        counterparty_htlc_base_key: PublicKey,
@@ -310,7 +310,7 @@ impl Readable for CounterpartyCommitmentParameters {
 /// transaction causing it.
 ///
 /// Used to determine when the on-chain event can be considered safe from a chain reorganization.
-#[derive(PartialEq)]
+#[derive(Clone, PartialEq)]
 struct OnchainEventEntry {
        txid: Txid,
        height: u32,
@@ -346,7 +346,7 @@ impl OnchainEventEntry {
 
 /// Upon discovering of some classes of onchain tx by ChannelMonitor, we may have to take actions on it
 /// once they mature to enough confirmations (ANTI_REORG_DELAY)
-#[derive(PartialEq)]
+#[derive(Clone, PartialEq)]
 enum OnchainEvent {
        /// An outbound HTLC failing after a transaction is confirmed. Used
        ///  * when an outbound HTLC output is spent by us after the HTLC timed out
@@ -566,7 +566,7 @@ pub enum Balance {
 }
 
 /// An HTLC which has been irrevocably resolved on-chain, and has reached ANTI_REORG_DELAY.
-#[derive(PartialEq)]
+#[derive(Clone, PartialEq)]
 struct IrrevocablyResolvedHTLC {
        commitment_tx_output_idx: u32,
        /// Only set if the HTLC claim was ours using a payment preimage
@@ -601,6 +601,13 @@ pub struct ChannelMonitor<Signer: Sign> {
        inner: Mutex<ChannelMonitorImpl<Signer>>,
 }
 
+impl<Signer: Sign> Clone for ChannelMonitor<Signer> {
+       fn clone(&self) -> Self {
+               Self { inner: Mutex::new(self.inner.lock().unwrap().clone()) }
+       }
+}
+
+#[derive(Clone)]
 pub(crate) struct ChannelMonitorImpl<Signer: Sign> {
        latest_update_id: u64,
        commitment_transaction_number_obscure_factor: u64,
@@ -722,6 +729,9 @@ pub(crate) struct ChannelMonitorImpl<Signer: Sign> {
        // the full block_connected).
        best_block: BestBlock,
 
+       /// The node_id of our counterparty
+       counterparty_node_id: Option<PublicKey>,
+
        secp_ctx: Secp256k1<secp256k1::All>, //TODO: dedup this a bit...
 }
 
@@ -869,6 +879,9 @@ impl<Signer: Sign> Writeable for ChannelMonitorImpl<Signer> {
                        writer.write_all(&txid[..])?;
                        writer.write_all(&byte_utils::be64_to_array(htlc_infos.len() as u64))?;
                        for &(ref htlc_output, ref htlc_source) in htlc_infos.iter() {
+                               debug_assert!(htlc_source.is_none() || Some(**txid) == self.current_counterparty_commitment_txid
+                                               || Some(**txid) == self.prev_counterparty_commitment_txid,
+                                       "HTLC Sources for all revoked commitment transactions should be none!");
                                serialize_htlc_in_commitment!(htlc_output);
                                htlc_source.as_ref().map(|b| b.as_ref()).write(writer)?;
                        }
@@ -951,6 +964,7 @@ impl<Signer: Sign> Writeable for ChannelMonitorImpl<Signer> {
                        (3, self.htlcs_resolved_on_chain, vec_type),
                        (5, self.pending_monitor_events, vec_type),
                        (7, self.funding_spend_seen, required),
+                       (9, self.counterparty_node_id, option),
                });
 
                Ok(())
@@ -958,13 +972,20 @@ impl<Signer: Sign> Writeable for ChannelMonitorImpl<Signer> {
 }
 
 impl<Signer: Sign> ChannelMonitor<Signer> {
+       /// For lockorder enforcement purposes, we need to have a single site which constructs the
+       /// `inner` mutex, otherwise cases where we lock two monitors at the same time (eg in our
+       /// PartialEq implementation) we may decide a lockorder violation has occurred.
+       fn from_impl(imp: ChannelMonitorImpl<Signer>) -> Self {
+               ChannelMonitor { inner: Mutex::new(imp) }
+       }
+
        pub(crate) fn new(secp_ctx: Secp256k1<secp256k1::All>, keys: Signer, shutdown_script: Option<Script>,
                          on_counterparty_tx_csv: u16, destination_script: &Script, funding_info: (OutPoint, Script),
                          channel_parameters: &ChannelTransactionParameters,
                          funding_redeemscript: Script, channel_value_satoshis: u64,
                          commitment_transaction_number_obscure_factor: u64,
                          initial_holder_commitment_tx: HolderCommitmentTransaction,
-                         best_block: BestBlock) -> ChannelMonitor<Signer> {
+                         best_block: BestBlock, counterparty_node_id: PublicKey) -> ChannelMonitor<Signer> {
 
                assert!(commitment_transaction_number_obscure_factor <= (1 << 48));
                let payment_key_hash = WPubkeyHash::hash(&keys.pubkeys().payment_point.serialize());
@@ -1005,59 +1026,58 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
                let mut outputs_to_watch = HashMap::new();
                outputs_to_watch.insert(funding_info.0.txid, vec![(funding_info.0.index as u32, funding_info.1.clone())]);
 
-               ChannelMonitor {
-                       inner: Mutex::new(ChannelMonitorImpl {
-                               latest_update_id: 0,
-                               commitment_transaction_number_obscure_factor,
+               Self::from_impl(ChannelMonitorImpl {
+                       latest_update_id: 0,
+                       commitment_transaction_number_obscure_factor,
 
-                               destination_script: destination_script.clone(),
-                               broadcasted_holder_revokable_script: None,
-                               counterparty_payment_script,
-                               shutdown_script,
+                       destination_script: destination_script.clone(),
+                       broadcasted_holder_revokable_script: None,
+                       counterparty_payment_script,
+                       shutdown_script,
 
-                               channel_keys_id,
-                               holder_revocation_basepoint,
-                               funding_info,
-                               current_counterparty_commitment_txid: None,
-                               prev_counterparty_commitment_txid: None,
+                       channel_keys_id,
+                       holder_revocation_basepoint,
+                       funding_info,
+                       current_counterparty_commitment_txid: None,
+                       prev_counterparty_commitment_txid: None,
 
-                               counterparty_commitment_params,
-                               funding_redeemscript,
-                               channel_value_satoshis,
-                               their_cur_per_commitment_points: None,
+                       counterparty_commitment_params,
+                       funding_redeemscript,
+                       channel_value_satoshis,
+                       their_cur_per_commitment_points: None,
 
-                               on_holder_tx_csv: counterparty_channel_parameters.selected_contest_delay,
+                       on_holder_tx_csv: counterparty_channel_parameters.selected_contest_delay,
 
-                               commitment_secrets: CounterpartyCommitmentSecrets::new(),
-                               counterparty_claimable_outpoints: HashMap::new(),
-                               counterparty_commitment_txn_on_chain: HashMap::new(),
-                               counterparty_hash_commitment_number: HashMap::new(),
+                       commitment_secrets: CounterpartyCommitmentSecrets::new(),
+                       counterparty_claimable_outpoints: HashMap::new(),
+                       counterparty_commitment_txn_on_chain: HashMap::new(),
+                       counterparty_hash_commitment_number: HashMap::new(),
 
-                               prev_holder_signed_commitment_tx: None,
-                               current_holder_commitment_tx: holder_commitment_tx,
-                               current_counterparty_commitment_number: 1 << 48,
-                               current_holder_commitment_number,
+                       prev_holder_signed_commitment_tx: None,
+                       current_holder_commitment_tx: holder_commitment_tx,
+                       current_counterparty_commitment_number: 1 << 48,
+                       current_holder_commitment_number,
 
-                               payment_preimages: HashMap::new(),
-                               pending_monitor_events: Vec::new(),
-                               pending_events: Vec::new(),
+                       payment_preimages: HashMap::new(),
+                       pending_monitor_events: Vec::new(),
+                       pending_events: Vec::new(),
 
-                               onchain_events_awaiting_threshold_conf: Vec::new(),
-                               outputs_to_watch,
+                       onchain_events_awaiting_threshold_conf: Vec::new(),
+                       outputs_to_watch,
 
-                               onchain_tx_handler,
+                       onchain_tx_handler,
 
-                               lockdown_from_offchain: false,
-                               holder_tx_signed: false,
-                               funding_spend_seen: false,
-                               funding_spend_confirmed: None,
-                               htlcs_resolved_on_chain: Vec::new(),
+                       lockdown_from_offchain: false,
+                       holder_tx_signed: false,
+                       funding_spend_seen: false,
+                       funding_spend_confirmed: None,
+                       htlcs_resolved_on_chain: Vec::new(),
 
-                               best_block,
+                       best_block,
+                       counterparty_node_id: Some(counterparty_node_id),
 
-                               secp_ctx,
-                       }),
-               }
+                       secp_ctx,
+               })
        }
 
        #[cfg(test)]
@@ -1096,7 +1116,7 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
                payment_hash: &PaymentHash,
                payment_preimage: &PaymentPreimage,
                broadcaster: &B,
-               fee_estimator: &F,
+               fee_estimator: &LowerBoundedFeeEstimator<F>,
                logger: &L,
        ) where
                B::Target: BroadcasterInterface,
@@ -1126,7 +1146,7 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
                &self,
                updates: &ChannelMonitorUpdate,
                broadcaster: &B,
-               fee_estimator: &F,
+               fee_estimator: F,
                logger: &L,
        ) -> Result<(), ()>
        where
@@ -1201,6 +1221,14 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
                self.inner.lock().unwrap().get_cur_holder_commitment_number()
        }
 
+       /// Gets the `node_id` of the counterparty for this channel.
+       ///
+       /// Will be `None` for channels constructed on LDK versions prior to 0.0.110 and always `Some`
+       /// otherwise.
+       pub fn get_counterparty_node_id(&self) -> Option<PublicKey> {
+               self.inner.lock().unwrap().counterparty_node_id
+       }
+
        /// Used by ChannelManager deserialization to broadcast the latest holder state if its copy of
        /// the Channel was out-of-date. You may use it to get a broadcastable holder toxic tx in case of
        /// fallen-behind, i.e when receiving a channel_reestablish with a proof that our counterparty side knows
@@ -1292,8 +1320,9 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
                F::Target: FeeEstimator,
                L::Target: Logger,
        {
+               let bounded_fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator);
                self.inner.lock().unwrap().transactions_confirmed(
-                       header, txdata, height, broadcaster, fee_estimator, logger)
+                       header, txdata, height, broadcaster, &bounded_fee_estimator, logger)
        }
 
        /// Processes a transaction that was reorganized out of the chain.
@@ -1313,8 +1342,9 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
                F::Target: FeeEstimator,
                L::Target: Logger,
        {
+               let bounded_fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator);
                self.inner.lock().unwrap().transaction_unconfirmed(
-                       txid, broadcaster, fee_estimator, logger);
+                       txid, broadcaster, &bounded_fee_estimator, logger);
        }
 
        /// Updates the monitor with the current best chain tip, returning new outputs to watch. See
@@ -1337,8 +1367,9 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
                F::Target: FeeEstimator,
                L::Target: Logger,
        {
+               let bounded_fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator);
                self.inner.lock().unwrap().best_block_updated(
-                       header, height, broadcaster, fee_estimator, logger)
+                       header, height, broadcaster, &bounded_fee_estimator, logger)
        }
 
        /// Returns the set of txids that should be monitored for re-organization out of the chain.
@@ -1659,7 +1690,8 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
 /// as long as we examine both the current counterparty commitment transaction and, if it hasn't
 /// been revoked yet, the previous one, we we will never "forget" to resolve an HTLC.
 macro_rules! fail_unbroadcast_htlcs {
-       ($self: expr, $commitment_tx_type: expr, $commitment_tx_conf_height: expr, $confirmed_htlcs_list: expr, $logger: expr) => { {
+       ($self: expr, $commitment_tx_type: expr, $commitment_txid_confirmed: expr,
+        $commitment_tx_conf_height: expr, $confirmed_htlcs_list: expr, $logger: expr) => { {
                macro_rules! check_htlc_fails {
                        ($txid: expr, $commitment_tx: expr) => {
                                if let Some(ref latest_outpoints) = $self.counterparty_claimable_outpoints.get($txid) {
@@ -1675,9 +1707,14 @@ macro_rules! fail_unbroadcast_htlcs {
                                                        // cannot currently change after channel initialization, so we don't
                                                        // need to here.
                                                        let confirmed_htlcs_iter: &mut Iterator<Item = (&HTLCOutputInCommitment, Option<&HTLCSource>)> = &mut $confirmed_htlcs_list;
+
                                                        let mut matched_htlc = false;
                                                        for (ref broadcast_htlc, ref broadcast_source) in confirmed_htlcs_iter {
-                                                               if broadcast_htlc.transaction_output_index.is_some() && Some(&**source) == *broadcast_source {
+                                                               if broadcast_htlc.transaction_output_index.is_some() &&
+                                                                       (Some(&**source) == *broadcast_source ||
+                                                                        (broadcast_source.is_none() &&
+                                                                         broadcast_htlc.payment_hash == htlc.payment_hash &&
+                                                                         broadcast_htlc.amount_msat == htlc.amount_msat)) {
                                                                        matched_htlc = true;
                                                                        break;
                                                                }
@@ -1693,7 +1730,7 @@ macro_rules! fail_unbroadcast_htlcs {
                                                                }
                                                        });
                                                        let entry = OnchainEventEntry {
-                                                               txid: *$txid,
+                                                               txid: $commitment_txid_confirmed,
                                                                height: $commitment_tx_conf_height,
                                                                event: OnchainEvent::HTLCUpdate {
                                                                        source: (**source).clone(),
@@ -1702,8 +1739,9 @@ macro_rules! fail_unbroadcast_htlcs {
                                                                        commitment_tx_output_idx: None,
                                                                },
                                                        };
-                                                       log_trace!($logger, "Failing HTLC with payment_hash {} from {} counterparty commitment tx due to broadcast of {} commitment transaction, waiting for confirmation (at height {})",
-                                                               log_bytes!(htlc.payment_hash.0), $commitment_tx, $commitment_tx_type, entry.confirmation_threshold());
+                                                       log_trace!($logger, "Failing HTLC with payment_hash {} from {} counterparty commitment tx due to broadcast of {} commitment transaction {}, waiting for confirmation (at height {})",
+                                                               log_bytes!(htlc.payment_hash.0), $commitment_tx, $commitment_tx_type,
+                                                               $commitment_txid_confirmed, entry.confirmation_threshold());
                                                        $self.onchain_events_awaiting_threshold_conf.push(entry);
                                                }
                                        }
@@ -1719,6 +1757,26 @@ macro_rules! fail_unbroadcast_htlcs {
        } }
 }
 
+// In the `test_invalid_funding_tx` test, we need a bogus script which matches the HTLC-Accepted
+// witness length match (ie is 136 bytes long). We generate one here which we also use in some
+// in-line tests later.
+
+#[cfg(test)]
+pub fn deliberately_bogus_accepted_htlc_witness_program() -> Vec<u8> {
+       let mut ret = [opcodes::all::OP_NOP.into_u8(); 136];
+       ret[131] = opcodes::all::OP_DROP.into_u8();
+       ret[132] = opcodes::all::OP_DROP.into_u8();
+       ret[133] = opcodes::all::OP_DROP.into_u8();
+       ret[134] = opcodes::all::OP_DROP.into_u8();
+       ret[135] = opcodes::OP_TRUE.into_u8();
+       Vec::from(&ret[..])
+}
+
+#[cfg(test)]
+pub fn deliberately_bogus_accepted_htlc_witness() -> Vec<Vec<u8>> {
+       vec![Vec::new(), Vec::new(), Vec::new(), Vec::new(), deliberately_bogus_accepted_htlc_witness_program().into()].into()
+}
+
 impl<Signer: Sign> ChannelMonitorImpl<Signer> {
        /// Inserts a revocation secret into this channel monitor. Prunes old preimages if neither
        /// needed by holder commitment transactions HTCLs nor by counterparty ones. Unless we haven't already seen
@@ -1847,7 +1905,9 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 
        /// Provides a payment_hash->payment_preimage mapping. Will be automatically pruned when all
        /// commitment_tx_infos which contain the payment hash have been revoked.
-       fn provide_payment_preimage<B: Deref, F: Deref, L: Deref>(&mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage, broadcaster: &B, fee_estimator: &F, logger: &L)
+       fn provide_payment_preimage<B: Deref, F: Deref, L: Deref>(
+               &mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage, broadcaster: &B,
+               fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
        where B::Target: BroadcasterInterface,
                    F::Target: FeeEstimator,
                    L::Target: Logger,
@@ -1904,10 +1964,10 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                self.pending_monitor_events.push(MonitorEvent::CommitmentTxConfirmed(self.funding_info.0));
        }
 
-       pub fn update_monitor<B: Deref, F: Deref, L: Deref>(&mut self, updates: &ChannelMonitorUpdate, broadcaster: &B, fee_estimator: &F, logger: &L) -> Result<(), ()>
+       pub fn update_monitor<B: Deref, F: Deref, L: Deref>(&mut self, updates: &ChannelMonitorUpdate, broadcaster: &B, fee_estimator: F, logger: &L) -> Result<(), ()>
        where B::Target: BroadcasterInterface,
-                   F::Target: FeeEstimator,
-                   L::Target: Logger,
+               F::Target: FeeEstimator,
+               L::Target: Logger,
        {
                log_info!(logger, "Applying update to monitor {}, bringing update_id from {} to {} with {} changes.",
                        log_funding_info!(self), self.latest_update_id, updates.update_id, updates.updates.len());
@@ -1945,7 +2005,8 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                                },
                                ChannelMonitorUpdateStep::PaymentPreimage { payment_preimage } => {
                                        log_trace!(logger, "Updating ChannelMonitor with payment preimage");
-                                       self.provide_payment_preimage(&PaymentHash(Sha256::hash(&payment_preimage.0[..]).into_inner()), &payment_preimage, broadcaster, fee_estimator, logger)
+                                       let bounded_fee_estimator = LowerBoundedFeeEstimator::new(&*fee_estimator);
+                                       self.provide_payment_preimage(&PaymentHash(Sha256::hash(&payment_preimage.0[..]).into_inner()), &payment_preimage, broadcaster, &bounded_fee_estimator, logger)
                                },
                                ChannelMonitorUpdateStep::CommitmentSecret { idx, secret } => {
                                        log_trace!(logger, "Updating ChannelMonitor with commitment secret");
@@ -2100,7 +2161,16 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                                }
                                self.counterparty_commitment_txn_on_chain.insert(commitment_txid, commitment_number);
 
-                               fail_unbroadcast_htlcs!(self, "revoked counterparty", height, [].iter().map(|a| *a), logger);
+                               if let Some(per_commitment_data) = per_commitment_option {
+                                       fail_unbroadcast_htlcs!(self, "revoked_counterparty", commitment_txid, height,
+                                               per_commitment_data.iter().map(|(htlc, htlc_source)|
+                                                       (htlc, htlc_source.as_ref().map(|htlc_source| htlc_source.as_ref()))
+                                               ), logger);
+                               } else {
+                                       debug_assert!(false, "We should have per-commitment option for any recognized old commitment txn");
+                                       fail_unbroadcast_htlcs!(self, "revoked counterparty", commitment_txid, height,
+                                               [].iter().map(|reference| *reference), logger);
+                               }
                        }
                } else if let Some(per_commitment_data) = per_commitment_option {
                        // While this isn't useful yet, there is a potential race where if a counterparty
@@ -2116,7 +2186,10 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                        self.counterparty_commitment_txn_on_chain.insert(commitment_txid, commitment_number);
 
                        log_info!(logger, "Got broadcast of non-revoked counterparty commitment transaction {}", commitment_txid);
-                       fail_unbroadcast_htlcs!(self, "counterparty", height, per_commitment_data.iter().map(|(a, b)| (a, b.as_ref().map(|b| b.as_ref()))), logger);
+                       fail_unbroadcast_htlcs!(self, "counterparty", commitment_txid, height,
+                               per_commitment_data.iter().map(|(htlc, htlc_source)|
+                                       (htlc, htlc_source.as_ref().map(|htlc_source| htlc_source.as_ref()))
+                               ), logger);
 
                        let htlc_claim_reqs = self.get_counterparty_htlc_output_claim_reqs(commitment_number, commitment_txid, Some(tx));
                        for req in htlc_claim_reqs {
@@ -2272,7 +2345,9 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                        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);
-                       fail_unbroadcast_htlcs!(self, "latest holder", height, self.current_holder_commitment_tx.htlc_outputs.iter().map(|(a, _, c)| (a, c.as_ref())), logger);
+                       fail_unbroadcast_htlcs!(self, "latest holder", commitment_txid, height,
+                               self.current_holder_commitment_tx.htlc_outputs.iter()
+                               .map(|(htlc, _, htlc_source)| (htlc, htlc_source.as_ref())), logger);
                } else if let &Some(ref holder_tx) = &self.prev_holder_signed_commitment_tx {
                        if holder_tx.txid == commitment_txid {
                                is_holder_tx = true;
@@ -2280,7 +2355,9 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                                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);
-                               fail_unbroadcast_htlcs!(self, "previous holder", height, holder_tx.htlc_outputs.iter().map(|(a, _, c)| (a, c.as_ref())), logger);
+                               fail_unbroadcast_htlcs!(self, "previous holder", commitment_txid, height,
+                                       holder_tx.htlc_outputs.iter().map(|(htlc, _, htlc_source)| (htlc, htlc_source.as_ref())),
+                                       logger);
                        }
                }
 
@@ -2355,7 +2432,8 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                let block_hash = header.block_hash();
                self.best_block = BestBlock::new(block_hash, height);
 
-               self.transactions_confirmed(header, txdata, height, broadcaster, fee_estimator, logger)
+               let bounded_fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator);
+               self.transactions_confirmed(header, txdata, height, broadcaster, &bounded_fee_estimator, logger)
        }
 
        fn best_block_updated<B: Deref, F: Deref, L: Deref>(
@@ -2363,7 +2441,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                header: &BlockHeader,
                height: u32,
                broadcaster: B,
-               fee_estimator: F,
+               fee_estimator: &LowerBoundedFeeEstimator<F>,
                logger: L,
        ) -> Vec<TransactionOutputs>
        where
@@ -2390,7 +2468,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                txdata: &TransactionData,
                height: u32,
                broadcaster: B,
-               fee_estimator: F,
+               fee_estimator: &LowerBoundedFeeEstimator<F>,
                logger: L,
        ) -> Vec<TransactionOutputs>
        where
@@ -2487,7 +2565,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                mut watch_outputs: Vec<TransactionOutputs>,
                mut claimable_outpoints: Vec<PackageTemplate>,
                broadcaster: &B,
-               fee_estimator: &F,
+               fee_estimator: &LowerBoundedFeeEstimator<F>,
                logger: &L,
        ) -> Vec<TransactionOutputs>
        where
@@ -2561,7 +2639,8 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                                                matured_htlcs.push(source.clone());
                                        }
 
-                                       log_debug!(logger, "HTLC {} failure update has got enough confirmations to be passed upstream", log_bytes!(payment_hash.0));
+                                       log_debug!(logger, "HTLC {} failure update in {} has got enough confirmations to be passed upstream",
+                                               log_bytes!(payment_hash.0), entry.txid);
                                        self.pending_monitor_events.push(MonitorEvent::HTLCEvent(HTLCUpdate {
                                                payment_hash,
                                                payment_preimage: None,
@@ -2624,7 +2703,8 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                //- maturing spendable output has transaction paying us has been disconnected
                self.onchain_events_awaiting_threshold_conf.retain(|ref entry| entry.height < height);
 
-               self.onchain_tx_handler.block_disconnected(height, broadcaster, fee_estimator, logger);
+               let bounded_fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator);
+               self.onchain_tx_handler.block_disconnected(height, broadcaster, &bounded_fee_estimator, logger);
 
                self.best_block = BestBlock::new(header.prev_blockhash, height - 1);
        }
@@ -2633,14 +2713,17 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                &mut self,
                txid: &Txid,
                broadcaster: B,
-               fee_estimator: F,
+               fee_estimator: &LowerBoundedFeeEstimator<F>,
                logger: L,
        ) where
                B::Target: BroadcasterInterface,
                F::Target: FeeEstimator,
                L::Target: Logger,
        {
-               self.onchain_events_awaiting_threshold_conf.retain(|ref entry| entry.txid != *txid);
+               self.onchain_events_awaiting_threshold_conf.retain(|ref entry| if entry.txid == *txid {
+                       log_info!(logger, "Removing onchain event with txid {}", txid);
+                       false
+               } else { true });
                self.onchain_tx_handler.transaction_unconfirmed(txid, broadcaster, fee_estimator, logger);
        }
 
@@ -2671,14 +2754,21 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                                        if *idx == input.previous_output.vout {
                                                #[cfg(test)]
                                                {
-                                                       // If the expected script is a known type, check that the witness
-                                                       // appears to be spending the correct type (ie that the match would
-                                                       // actually succeed in BIP 158/159-style filters).
-                                                       if _script_pubkey.is_v0_p2wsh() {
-                                                               assert_eq!(&bitcoin::Address::p2wsh(&Script::from(input.witness.last().unwrap().to_vec()), bitcoin::Network::Bitcoin).script_pubkey(), _script_pubkey);
-                                                       } else if _script_pubkey.is_v0_p2wpkh() {
-                                                               assert_eq!(&bitcoin::Address::p2wpkh(&bitcoin::PublicKey::from_slice(&input.witness.last().unwrap()).unwrap(), bitcoin::Network::Bitcoin).unwrap().script_pubkey(), _script_pubkey);
-                                                       } else { panic!(); }
+                                                       // If the expected script is a known type, check that the witness
+                                                       // appears to be spending the correct type (ie that the match would
+                                                       // actually succeed in BIP 158/159-style filters).
+                                                       if _script_pubkey.is_v0_p2wsh() {
+                                                               if input.witness.last().unwrap().to_vec() == deliberately_bogus_accepted_htlc_witness_program() {
+                                                                       // In at least one test we use a deliberately bogus witness
+                                                                       // script which hit an old panic. Thus, we check for that here
+                                                                       // and avoid the assert if its the expected bogus script.
+                                                                       return true;
+                                                               }
+
+                                                               assert_eq!(&bitcoin::Address::p2wsh(&Script::from(input.witness.last().unwrap().to_vec()), bitcoin::Network::Bitcoin).script_pubkey(), _script_pubkey);
+                                                       } else if _script_pubkey.is_v0_p2wpkh() {
+                                                               assert_eq!(&bitcoin::Address::p2wpkh(&bitcoin::PublicKey::from_slice(&input.witness.last().unwrap()).unwrap(), bitcoin::Network::Bitcoin).unwrap().script_pubkey(), _script_pubkey);
+                                                       } else { panic!(); }
                                                }
                                                return true;
                                        }
@@ -2763,10 +2853,13 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                        let prev_last_witness_len = input.witness.second_to_last().map(|w| w.len()).unwrap_or(0);
                        let revocation_sig_claim = (witness_items == 3 && htlctype == Some(HTLCType::OfferedHTLC) && prev_last_witness_len == 33)
                                || (witness_items == 3 && htlctype == Some(HTLCType::AcceptedHTLC) && prev_last_witness_len == 33);
-                       let accepted_preimage_claim = witness_items == 5 && htlctype == Some(HTLCType::AcceptedHTLC);
+                       let accepted_preimage_claim = witness_items == 5 && htlctype == Some(HTLCType::AcceptedHTLC)
+                               && input.witness.second_to_last().unwrap().len() == 32;
                        #[cfg(not(fuzzing))]
                        let accepted_timeout_claim = witness_items == 3 && htlctype == Some(HTLCType::AcceptedHTLC) && !revocation_sig_claim;
-                       let offered_preimage_claim = witness_items == 3 && htlctype == Some(HTLCType::OfferedHTLC) && !revocation_sig_claim;
+                       let offered_preimage_claim = witness_items == 3 && htlctype == Some(HTLCType::OfferedHTLC) &&
+                               !revocation_sig_claim && input.witness.second_to_last().unwrap().len() == 32;
+
                        #[cfg(not(fuzzing))]
                        let offered_timeout_claim = witness_items == 5 && htlctype == Some(HTLCType::OfferedHTLC);
 
@@ -3276,69 +3369,70 @@ impl<'a, Signer: Sign, K: KeysInterface<Signer = Signer>> ReadableArgs<&'a K>
                let mut funding_spend_confirmed = None;
                let mut htlcs_resolved_on_chain = Some(Vec::new());
                let mut funding_spend_seen = Some(false);
+               let mut counterparty_node_id = None;
                read_tlv_fields!(reader, {
                        (1, funding_spend_confirmed, option),
                        (3, htlcs_resolved_on_chain, vec_type),
                        (5, pending_monitor_events, vec_type),
                        (7, funding_spend_seen, option),
+                       (9, counterparty_node_id, option),
                });
 
                let mut secp_ctx = Secp256k1::new();
                secp_ctx.seeded_randomize(&keys_manager.get_secure_random_bytes());
 
-               Ok((best_block.block_hash(), ChannelMonitor {
-                       inner: Mutex::new(ChannelMonitorImpl {
-                               latest_update_id,
-                               commitment_transaction_number_obscure_factor,
+               Ok((best_block.block_hash(), ChannelMonitor::from_impl(ChannelMonitorImpl {
+                       latest_update_id,
+                       commitment_transaction_number_obscure_factor,
 
-                               destination_script,
-                               broadcasted_holder_revokable_script,
-                               counterparty_payment_script,
-                               shutdown_script,
+                       destination_script,
+                       broadcasted_holder_revokable_script,
+                       counterparty_payment_script,
+                       shutdown_script,
 
-                               channel_keys_id,
-                               holder_revocation_basepoint,
-                               funding_info,
-                               current_counterparty_commitment_txid,
-                               prev_counterparty_commitment_txid,
+                       channel_keys_id,
+                       holder_revocation_basepoint,
+                       funding_info,
+                       current_counterparty_commitment_txid,
+                       prev_counterparty_commitment_txid,
 
-                               counterparty_commitment_params,
-                               funding_redeemscript,
-                               channel_value_satoshis,
-                               their_cur_per_commitment_points,
+                       counterparty_commitment_params,
+                       funding_redeemscript,
+                       channel_value_satoshis,
+                       their_cur_per_commitment_points,
 
-                               on_holder_tx_csv,
+                       on_holder_tx_csv,
 
-                               commitment_secrets,
-                               counterparty_claimable_outpoints,
-                               counterparty_commitment_txn_on_chain,
-                               counterparty_hash_commitment_number,
+                       commitment_secrets,
+                       counterparty_claimable_outpoints,
+                       counterparty_commitment_txn_on_chain,
+                       counterparty_hash_commitment_number,
 
-                               prev_holder_signed_commitment_tx,
-                               current_holder_commitment_tx,
-                               current_counterparty_commitment_number,
-                               current_holder_commitment_number,
+                       prev_holder_signed_commitment_tx,
+                       current_holder_commitment_tx,
+                       current_counterparty_commitment_number,
+                       current_holder_commitment_number,
 
-                               payment_preimages,
-                               pending_monitor_events: pending_monitor_events.unwrap(),
-                               pending_events,
+                       payment_preimages,
+                       pending_monitor_events: pending_monitor_events.unwrap(),
+                       pending_events,
 
-                               onchain_events_awaiting_threshold_conf,
-                               outputs_to_watch,
+                       onchain_events_awaiting_threshold_conf,
+                       outputs_to_watch,
 
-                               onchain_tx_handler,
+                       onchain_tx_handler,
 
-                               lockdown_from_offchain,
-                               holder_tx_signed,
-                               funding_spend_seen: funding_spend_seen.unwrap(),
-                               funding_spend_confirmed,
-                               htlcs_resolved_on_chain: htlcs_resolved_on_chain.unwrap(),
+                       lockdown_from_offchain,
+                       holder_tx_signed,
+                       funding_spend_seen: funding_spend_seen.unwrap(),
+                       funding_spend_confirmed,
+                       htlcs_resolved_on_chain: htlcs_resolved_on_chain.unwrap(),
 
-                               best_block,
+                       best_block,
+                       counterparty_node_id,
 
-                               secp_ctx,
-                       }),
-               }))
+                       secp_ctx,
+               })))
        }
 }
 
@@ -3360,6 +3454,8 @@ mod tests {
 
        use hex;
 
+       use crate::chain::chaininterface::LowerBoundedFeeEstimator;
+
        use super::ChannelMonitorUpdateStep;
        use ::{check_added_monitors, check_closed_broadcast, check_closed_event, check_spends, get_local_commitment_txn, get_monitor, get_route_and_payment_hash, unwrap_send_err};
        use chain::{BestBlock, Confirm};
@@ -3456,7 +3552,7 @@ mod tests {
 
                let broadcaster = TestBroadcaster::new(Arc::clone(&nodes[1].blocks));
                assert!(
-                       pre_update_monitor.update_monitor(&replay_update, &&broadcaster, &&chanmon_cfgs[1].fee_estimator, &nodes[1].logger)
+                       pre_update_monitor.update_monitor(&replay_update, &&broadcaster, &chanmon_cfgs[1].fee_estimator, &nodes[1].logger)
                        .is_err());
                // Even though we error'd on the first update, we should still have generated an HTLC claim
                // transaction
@@ -3481,7 +3577,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: Mutex::new(253) });
+               let fee_estimator = 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() };
@@ -3571,7 +3667,7 @@ mod tests {
                                                  (OutPoint { txid: Txid::from_slice(&[43; 32]).unwrap(), index: 0 }, Script::new()),
                                                  &channel_parameters,
                                                  Script::new(), 46, 0,
-                                                 HolderCommitmentTransaction::dummy(), best_block);
+                                                 HolderCommitmentTransaction::dummy(), best_block, dummy_key);
 
                monitor.provide_latest_holder_commitment_tx(HolderCommitmentTransaction::dummy(), preimages_to_holder_htlcs!(preimages[0..10])).unwrap();
                let dummy_txid = dummy_tx.txid();
@@ -3580,7 +3676,8 @@ mod tests {
                monitor.provide_latest_counterparty_commitment_tx(dummy_txid, preimages_slice_to_htlc_outputs!(preimages[17..20]), 281474976710653, dummy_key, &logger);
                monitor.provide_latest_counterparty_commitment_tx(dummy_txid, preimages_slice_to_htlc_outputs!(preimages[18..20]), 281474976710652, dummy_key, &logger);
                for &(ref preimage, ref hash) in preimages.iter() {
-                       monitor.provide_payment_preimage(hash, preimage, &broadcaster, &fee_estimator, &logger);
+                       let bounded_fee_estimator = LowerBoundedFeeEstimator::new(&fee_estimator);
+                       monitor.provide_payment_preimage(hash, preimage, &broadcaster, &bounded_fee_estimator, &logger);
                }
 
                // Now provide a secret, pruning preimages 10-15