From: Matt Corallo <649246+TheBlueMatt@users.noreply.github.com> Date: Thu, 15 Oct 2020 00:45:33 +0000 (-0700) Subject: Merge pull request #653 from ariard/2020-06-fix-outputs-tracking X-Git-Tag: v0.0.12~11 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=8a798776b4a48a31d404c55394f3aa04b55280d1;hp=df778b605a28580905cb5ca63b3ec8bbe99afc2f;p=rust-lightning Merge pull request #653 from ariard/2020-06-fix-outputs-tracking Add outpoint index in watch_outputs to fix tracking --- diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index 179d0edb..6c67f54f 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 31e4565e..d8433ef5 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,9 +1769,23 @@ 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() }); + #[cfg(test)] + { + // If we see a transaction for which we registered outputs previously, + // make sure the registered scriptpubkey at the expected index match + // the actual transaction output one. We failed this case before #653. + for tx in &txn_matched { + if let Some(outputs) = self.get_outputs_to_watch().get(&tx.txid()) { + for idx_and_script in outputs.iter() { + assert!((idx_and_script.0 as usize) < tx.output.len()); + assert_eq!(tx.output[idx_and_script.0 as usize].script_pubkey, idx_and_script.1); + } + } + } + } watch_outputs } @@ -1813,8 +1833,19 @@ 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 { + #[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().clone()), 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; } } @@ -2316,13 +2347,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::