From 30aad79df8548d06fdd2270349fadf4afa07ee94 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Wed, 1 Jul 2020 13:49:11 -0400 Subject: [PATCH] Add transaction index in watched_outputs Previously, outputs were monitored based on txid and an index yelled from an enumeration over the returned selected outputs by monitoring code. This is always have been broken but was only discovered while introducing anchor outputs as those ones rank always first per BIP69. We didn't have test cases where a HTLC was bigger than a party balance on a holder commitment and thus not ranking first. Next commit introduce test coverage. --- lightning/src/chain/chainmonitor.rs | 8 ++--- lightning/src/chain/channelmonitor.rs | 50 +++++++++++++++------------ 2 files changed, 32 insertions(+), 26 deletions(-) diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index 179d0edb5..6c67f54f1 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -95,8 +95,8 @@ impl ChainMonit if let Some(ref chain_source) = self.chain_source { for (txid, outputs) in txn_outputs.drain(..) { - for (idx, output) in outputs.iter().enumerate() { - chain_source.register_output(&OutPoint { txid, index: idx as u16 }, &output.script_pubkey); + for (idx, output) in outputs.iter() { + chain_source.register_output(&OutPoint { txid, index: *idx as u16 }, &output.script_pubkey); } } } @@ -152,8 +152,8 @@ impl ChainMonit if let Some(ref chain_source) = self.chain_source { chain_source.register_tx(&funding_txo.0.txid, &funding_txo.1); for (txid, outputs) in monitor.get_outputs_to_watch().iter() { - for (idx, script_pubkey) in outputs.iter().enumerate() { - chain_source.register_output(&OutPoint { txid: *txid, index: idx as u16 }, &script_pubkey); + for (idx, script_pubkey) in outputs.iter() { + chain_source.register_output(&OutPoint { txid: *txid, index: *idx as u16 }, script_pubkey); } } } diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 31e4565ec..e0f9d6078 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -666,7 +666,7 @@ pub struct ChannelMonitor { // interface knows about the TXOs that we want to be notified of spends of. We could probably // be smart and derive them from the above storage fields, but its much simpler and more // Obviously Correct (tm) if we just keep track of them explicitly. - outputs_to_watch: HashMap>, + outputs_to_watch: HashMap>, #[cfg(test)] pub onchain_tx_handler: OnchainTxHandler, @@ -914,10 +914,11 @@ impl ChannelMonitor { } (self.outputs_to_watch.len() as u64).write(writer)?; - for (txid, output_scripts) in self.outputs_to_watch.iter() { + for (txid, idx_scripts) in self.outputs_to_watch.iter() { txid.write(writer)?; - (output_scripts.len() as u64).write(writer)?; - for script in output_scripts.iter() { + (idx_scripts.len() as u64).write(writer)?; + for (idx, script) in idx_scripts.iter() { + idx.write(writer)?; script.write(writer)?; } } @@ -963,7 +964,7 @@ impl ChannelMonitor { onchain_tx_handler.provide_latest_holder_tx(initial_holder_commitment_tx); let mut outputs_to_watch = HashMap::new(); - outputs_to_watch.insert(funding_info.0.txid, vec![funding_info.1.clone()]); + outputs_to_watch.insert(funding_info.0.txid, vec![(funding_info.0.index as u32, funding_info.1.clone())]); ChannelMonitor { latest_update_id: 0, @@ -1209,7 +1210,7 @@ impl ChannelMonitor { /// transaction), which we must learn about spends of via block_connected(). /// /// (C-not exported) because we have no HashMap bindings - pub fn get_outputs_to_watch(&self) -> &HashMap> { + pub fn get_outputs_to_watch(&self) -> &HashMap> { // If we've detected a counterparty commitment tx on chain, we must include it in the set // of outputs to watch for spends of, otherwise we're likely to lose user funds. Because // its trivial to do, double-check that here. @@ -1264,7 +1265,7 @@ impl ChannelMonitor { /// HTLC-Success/HTLC-Timeout transactions. /// Return updates for HTLC pending in the channel and failed automatically by the broadcast of /// revoked counterparty commitment tx - fn check_spend_counterparty_transaction(&mut self, tx: &Transaction, height: u32, logger: &L) -> (Vec, (Txid, Vec)) where L::Target: Logger { + fn check_spend_counterparty_transaction(&mut self, tx: &Transaction, height: u32, logger: &L) -> (Vec, (Txid, Vec<(u32, TxOut)>)) where L::Target: Logger { // Most secp and related errors trying to create keys means we have no hope of constructing // a spend transaction...so we return no transactions to broadcast let mut claimable_outpoints = Vec::new(); @@ -1319,7 +1320,9 @@ impl ChannelMonitor { 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()); - watch_outputs.append(&mut tx.output.clone()); + for (idx, outp) in tx.output.iter().enumerate() { + watch_outputs.push((idx as u32, outp.clone())); + } self.counterparty_commitment_txn_on_chain.insert(commitment_txid, commitment_number); macro_rules! check_htlc_fails { @@ -1366,7 +1369,9 @@ impl ChannelMonitor { // already processed the block, resulting in the counterparty_commitment_txn_on_chain entry // not being generated by the above conditional. Thus, to be safe, we go ahead and // insert it here. - watch_outputs.append(&mut tx.output.clone()); + for (idx, outp) in tx.output.iter().enumerate() { + watch_outputs.push((idx as u32, outp.clone())); + } self.counterparty_commitment_txn_on_chain.insert(commitment_txid, commitment_number); log_trace!(logger, "Got broadcast of non-revoked counterparty commitment transaction {}", commitment_txid); @@ -1456,7 +1461,7 @@ impl ChannelMonitor { } /// Attempts to claim a counterparty HTLC-Success/HTLC-Timeout's outputs using the revocation key - fn check_spend_counterparty_htlc(&mut self, tx: &Transaction, commitment_number: u64, height: u32, logger: &L) -> (Vec, Option<(Txid, Vec)>) where L::Target: Logger { + fn check_spend_counterparty_htlc(&mut self, tx: &Transaction, commitment_number: u64, height: u32, logger: &L) -> (Vec, Option<(Txid, Vec<(u32, TxOut)>)>) where L::Target: Logger { let htlc_txid = tx.txid(); if tx.input.len() != 1 || tx.output.len() != 1 || tx.input[0].witness.len() != 5 { return (Vec::new(), None) @@ -1478,10 +1483,11 @@ impl ChannelMonitor { log_trace!(logger, "Counterparty HTLC broadcast {}:{}", htlc_txid, 0); let witness_data = InputMaterial::Revoked { per_commitment_point, counterparty_delayed_payment_base_key: self.counterparty_tx_cache.counterparty_delayed_payment_base_key, counterparty_htlc_base_key: self.counterparty_tx_cache.counterparty_htlc_base_key, per_commitment_key, input_descriptor: InputDescriptors::RevokedOutput, amount: tx.output[0].value, htlc: None, on_counterparty_tx_csv: self.counterparty_tx_cache.on_counterparty_tx_csv }; let claimable_outpoints = vec!(ClaimRequest { absolute_timelock: height + self.counterparty_tx_cache.on_counterparty_tx_csv as u32, aggregable: true, outpoint: BitcoinOutPoint { txid: htlc_txid, vout: 0}, witness_data }); - (claimable_outpoints, Some((htlc_txid, tx.output.clone()))) + let outputs = vec![(0, tx.output[0].clone())]; + (claimable_outpoints, Some((htlc_txid, outputs))) } - fn broadcast_by_holder_state(&self, commitment_tx: &Transaction, holder_tx: &HolderSignedTx) -> (Vec, Vec, Option<(Script, PublicKey, PublicKey)>) { + fn broadcast_by_holder_state(&self, commitment_tx: &Transaction, holder_tx: &HolderSignedTx) -> (Vec, Vec<(u32, TxOut)>, Option<(Script, PublicKey, PublicKey)>) { let mut claim_requests = Vec::with_capacity(holder_tx.htlc_outputs.len()); let mut watch_outputs = Vec::with_capacity(holder_tx.htlc_outputs.len()); @@ -1502,7 +1508,7 @@ impl ChannelMonitor { } else { None }, amount: htlc.amount_msat, }}); - watch_outputs.push(commitment_tx.output[transaction_output_index as usize].clone()); + watch_outputs.push((transaction_output_index, commitment_tx.output[transaction_output_index as usize].clone())); } } @@ -1512,7 +1518,7 @@ impl ChannelMonitor { /// Attempts to claim any claimable HTLCs in a commitment transaction which was not (yet) /// revoked using data in holder_claimable_outpoints. /// Should not be used if check_spend_revoked_transaction succeeds. - fn check_spend_holder_transaction(&mut self, tx: &Transaction, height: u32, logger: &L) -> (Vec, (Txid, Vec)) where L::Target: Logger { + fn check_spend_holder_transaction(&mut self, tx: &Transaction, height: u32, logger: &L) -> (Vec, (Txid, Vec<(u32, TxOut)>)) where L::Target: Logger { let commitment_txid = tx.txid(); let mut claim_requests = Vec::new(); let mut watch_outputs = Vec::new(); @@ -1662,7 +1668,7 @@ impl ChannelMonitor { /// [`get_outputs_to_watch`]. /// /// [`get_outputs_to_watch`]: #method.get_outputs_to_watch - pub fn block_connected(&mut self, header: &BlockHeader, txdata: &TransactionData, height: u32, broadcaster: B, fee_estimator: F, logger: L)-> Vec<(Txid, Vec)> + pub fn block_connected(&mut self, header: &BlockHeader, txdata: &TransactionData, height: u32, broadcaster: B, fee_estimator: F, logger: L)-> Vec<(Txid, Vec<(u32, TxOut)>)> where B::Target: BroadcasterInterface, F::Target: FeeEstimator, L::Target: Logger, @@ -1763,8 +1769,8 @@ impl ChannelMonitor { // Determine new outputs to watch by comparing against previously known outputs to watch, // updating the latter in the process. watch_outputs.retain(|&(ref txid, ref txouts)| { - let output_scripts = txouts.iter().map(|o| o.script_pubkey.clone()).collect(); - self.outputs_to_watch.insert(txid.clone(), output_scripts).is_none() + let idx_and_scripts = txouts.iter().map(|o| (o.0, o.1.script_pubkey.clone())).collect(); + self.outputs_to_watch.insert(txid.clone(), idx_and_scripts).is_none() }); watch_outputs } @@ -1813,8 +1819,8 @@ impl ChannelMonitor { fn spends_watched_output(&self, tx: &Transaction) -> bool { for input in tx.input.iter() { if let Some(outputs) = self.get_outputs_to_watch().get(&input.previous_output.txid) { - for (idx, _script_pubkey) in outputs.iter().enumerate() { - if idx == input.previous_output.vout as usize { + for (idx, _script_pubkey) in outputs.iter() { + if *idx == input.previous_output.vout { return true; } } @@ -2316,13 +2322,13 @@ impl Readable for (BlockHash, ChannelMonitor } let outputs_to_watch_len: u64 = Readable::read(reader)?; - let mut outputs_to_watch = HashMap::with_capacity(cmp::min(outputs_to_watch_len as usize, MAX_ALLOC_SIZE / (mem::size_of::() + mem::size_of::>()))); + let mut outputs_to_watch = HashMap::with_capacity(cmp::min(outputs_to_watch_len as usize, MAX_ALLOC_SIZE / (mem::size_of::() + mem::size_of::() + mem::size_of::>()))); for _ in 0..outputs_to_watch_len { let txid = Readable::read(reader)?; let outputs_len: u64 = Readable::read(reader)?; - let mut outputs = Vec::with_capacity(cmp::min(outputs_len as usize, MAX_ALLOC_SIZE / mem::size_of::