Fix to_remote SpendableOutputs generation in rare reorg cases
[rust-lightning] / lightning / src / chain / channelmonitor.rs
index 009281958bfd4a1d12fb19ea236497f66c8f7562..525718c71f5c928e7ca76158f4a017482e906795 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};
@@ -55,7 +55,7 @@ use prelude::*;
 use core::{cmp, mem};
 use std::io::Error;
 use core::ops::Deref;
-use std::sync::Mutex;
+use sync::Mutex;
 
 /// An update generated by the underlying Channel itself which contains some new information the
 /// ChannelMonitor should be made aware of.
@@ -114,7 +114,7 @@ impl Readable for ChannelMonitorUpdate {
 }
 
 /// An error enum representing a failure to persist a channel monitor update.
-#[derive(Clone, Debug)]
+#[derive(Clone, Copy, Debug, PartialEq)]
 pub enum ChannelMonitorUpdateErr {
        /// Used to indicate a temporary failure (eg connection to a watchtower or remote backup of
        /// our state failed, but is expected to succeed at some point in the future).
@@ -370,8 +370,8 @@ impl OnchainEventEntry {
                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()
        }
 }
 
@@ -1189,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> {
@@ -1331,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 {
@@ -1353,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);
                        }
                }
        }
@@ -1724,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);
@@ -1743,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);
                        }
                }
@@ -1856,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;
@@ -1926,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>(
@@ -2004,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));
@@ -2043,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);
@@ -2098,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.
@@ -2200,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
@@ -2211,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 {
@@ -2425,7 +2451,8 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                                        output: outp.clone(),
                                });
                                break;
-                       } else if let Some(ref broadcasted_holder_revokable_script) = self.broadcasted_holder_revokable_script {
+                       }
+                       if let Some(ref broadcasted_holder_revokable_script) = self.broadcasted_holder_revokable_script {
                                if broadcasted_holder_revokable_script.0 == outp.script_pubkey {
                                        spendable_output =  Some(SpendableOutputDescriptor::DelayedPaymentOutput(DelayedPaymentOutputDescriptor {
                                                outpoint: OutPoint { txid: tx.txid(), index: i as u16 },
@@ -2438,7 +2465,8 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                                        }));
                                        break;
                                }
-                       } else if self.counterparty_payment_script == outp.script_pubkey {
+                       }
+                       if self.counterparty_payment_script == outp.script_pubkey {
                                spendable_output = Some(SpendableOutputDescriptor::StaticPaymentOutput(StaticPaymentOutputDescriptor {
                                        outpoint: OutPoint { txid: tx.txid(), index: i as u16 },
                                        output: outp.clone(),
@@ -2446,11 +2474,13 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                                        channel_value_satoshis: self.channel_value_satoshis,
                                }));
                                break;
-                       } else if outp.script_pubkey == self.shutdown_script {
+                       }
+                       if outp.script_pubkey == self.shutdown_script {
                                spendable_output = Some(SpendableOutputDescriptor::StaticOutput {
                                        outpoint: OutPoint { txid: tx.txid(), index: i as u16 },
                                        output: outp.clone(),
                                });
+                               break;
                        }
                }
                if let Some(spendable_output) = spendable_output {
@@ -2807,17 +2837,17 @@ 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};
        use bitcoin::secp256k1::key::{SecretKey,PublicKey};
        use bitcoin::secp256k1::Secp256k1;
-       use std::sync::{Arc, Mutex};
+       use sync::{Arc, Mutex};
        use chain::keysinterface::InMemorySigner;
        use prelude::*;
 
@@ -2826,7 +2856,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() };