From a162840ffd228f201ad281f715ef63c459eda14c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 27 Sep 2020 17:52:09 -0400 Subject: [PATCH 1/2] Drop the redundant/broken `ChannelMonitor::get_monitored_outpoints` In review of the final doc changes in #649, I noticed there appeared to be redundant monitored-outpoints function in `ChannelMonitor` - `get_monitored_outpoints()` and `get_outputs_to_watch()`. In 6f08779b0439e7e4367a75f4ee88de093dfb68cb, get_monitored_outpoints() was added, with its behavior largely the same as today's - only returning the set of remote commitment txn outputs that we've learned about on-chain. This is clearly not sufficient, and in 73dce207dd0ea6c3ac57af3ebb8b87ee03e82c9e, `get_outputs_to_watch` was added which was overly cautious to ensure nothing was missed. Still, the author of 73dce207dd0ea6c3ac5 (me) seemed entirely unaware of the work in 6f08779b0439e7e4367a75f (also me), despite the function being the literal next function in the same file. This is presumably because it was assumed that `get_monitored_outpoints` referred to oupoints for which we should monitor for spends of (which is true), while `get_outputs_to_watch` referred to outpouts which we should monitor for the transaction containing said output (which is not true), or something of that nature. Specifically, it is the expected behavior that the only time we care about `Filter::register_tx` is for the funding transaction (which we aren't aware of the inputs of), but for all other transactions we register interest on the basis of an outpoint in the previous transaction (ie via `Filter::register_output`). Here we drop the broken-on-day-one `get_monitored_outpoints()` version, but assert in testing that the values which it would return are all present in `get_outputs_to_watch()`. --- lightning/src/chain/channelmonitor.rs | 24 +++++++++--------------- lightning/src/ln/channelmanager.rs | 2 +- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index feff3978603..3adf3e0e705 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1214,23 +1214,17 @@ impl ChannelMonitor { /// /// (C-not exported) because we have no HashMap bindings pub fn get_outputs_to_watch(&self) -> &HashMap> { - &self.outputs_to_watch - } - - /// Gets the sets of all outpoints which this ChannelMonitor expects to hear about spends of. - /// Generally useful when deserializing as during normal operation the return values of - /// block_connected are sufficient to ensure all relevant outpoints are being monitored (note - /// that the get_funding_txo outpoint and transaction must also be monitored for!). - /// - /// (C-not exported) as there is no practical way to track lifetimes of returned values. - pub fn get_monitored_outpoints(&self) -> Vec<(Txid, u32, &Script)> { - let mut res = Vec::with_capacity(self.counterparty_commitment_txn_on_chain.len() * 2); - for (ref txid, &(_, ref outputs)) in self.counterparty_commitment_txn_on_chain.iter() { - for (idx, output) in outputs.iter().enumerate() { - res.push(((*txid).clone(), idx as u32, output)); + // 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. + for (txid, &(_, ref outputs)) in self.counterparty_commitment_txn_on_chain.iter() { + let watched_outputs = self.outputs_to_watch.get(txid).expect("Counterparty commitment txn which have been broadcast should have outputs registered"); + assert_eq!(watched_outputs.len(), outputs.len()); + for (watched, output) in watched_outputs.iter().zip(outputs.iter()) { + assert_eq!(watched, output); } } - res + &self.outputs_to_watch } /// Get the list of HTLCs who's status has been updated on chain. This should be called by diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index b5aa30896dd..6b2f50e9704 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3730,7 +3730,7 @@ impl Date: Sun, 27 Sep 2020 18:05:28 -0400 Subject: [PATCH 2/2] Drop now-unused Vec of outpoints in remote-commitment-tx-tracking This nearly fully reverts 6f08779b0439e7e4367a75f4ee88de093dfb68cb, removing the extra data storage that it added. --- lightning/src/chain/channelmonitor.rs | 31 ++++++++------------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 3adf3e0e705..0a9cf6997c9 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -631,7 +631,7 @@ pub struct ChannelMonitor { /// spending. Thus, in order to claim them via revocation key, we track all the counterparty /// commitment transactions which we find on-chain, mapping them to the commitment number which /// can be used to derive the revocation key and claim the transactions. - counterparty_commitment_txn_on_chain: HashMap)>, + counterparty_commitment_txn_on_chain: HashMap, /// Cache used to make pruning of payment_preimages faster. /// Maps payment_hash values to commitment numbers for counterparty transactions for non-revoked /// counterparty transactions (ie should remain pretty small). @@ -824,13 +824,9 @@ impl ChannelMonitor { } writer.write_all(&byte_utils::be64_to_array(self.counterparty_commitment_txn_on_chain.len() as u64))?; - for (ref txid, &(commitment_number, ref txouts)) in self.counterparty_commitment_txn_on_chain.iter() { + for (ref txid, commitment_number) in self.counterparty_commitment_txn_on_chain.iter() { writer.write_all(&txid[..])?; - writer.write_all(&byte_utils::be48_to_array(commitment_number))?; - (txouts.len() as u64).write(writer)?; - for script in txouts.iter() { - script.write(writer)?; - } + writer.write_all(&byte_utils::be48_to_array(*commitment_number))?; } writer.write_all(&byte_utils::be64_to_array(self.counterparty_hash_commitment_number.len() as u64))?; @@ -1217,12 +1213,8 @@ impl ChannelMonitor { // 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. - for (txid, &(_, ref outputs)) in self.counterparty_commitment_txn_on_chain.iter() { - let watched_outputs = self.outputs_to_watch.get(txid).expect("Counterparty commitment txn which have been broadcast should have outputs registered"); - assert_eq!(watched_outputs.len(), outputs.len()); - for (watched, output) in watched_outputs.iter().zip(outputs.iter()) { - assert_eq!(watched, output); - } + for (txid, _) in self.counterparty_commitment_txn_on_chain.iter() { + self.outputs_to_watch.get(txid).expect("Counterparty commitment txn which have been broadcast should have outputs registered"); } &self.outputs_to_watch } @@ -1328,7 +1320,7 @@ impl ChannelMonitor { // 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()); - self.counterparty_commitment_txn_on_chain.insert(commitment_txid, (commitment_number, tx.output.iter().map(|output| { output.script_pubkey.clone() }).collect())); + self.counterparty_commitment_txn_on_chain.insert(commitment_txid, commitment_number); macro_rules! check_htlc_fails { ($txid: expr, $commitment_tx: expr) => { @@ -1375,7 +1367,7 @@ impl ChannelMonitor { // 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()); - self.counterparty_commitment_txn_on_chain.insert(commitment_txid, (commitment_number, tx.output.iter().map(|output| { output.script_pubkey.clone() }).collect())); + self.counterparty_commitment_txn_on_chain.insert(commitment_txid, commitment_number); log_trace!(logger, "Got broadcast of non-revoked counterparty commitment transaction {}", commitment_txid); @@ -1713,7 +1705,7 @@ impl ChannelMonitor { claimable_outpoints.append(&mut new_outpoints); } } else { - if let Some(&(commitment_number, _)) = self.counterparty_commitment_txn_on_chain.get(&prevout.txid) { + if let Some(&commitment_number) = self.counterparty_commitment_txn_on_chain.get(&prevout.txid) { let (mut new_outpoints, new_outputs_option) = self.check_spend_counterparty_htlc(&tx, commitment_number, height, &logger); claimable_outpoints.append(&mut new_outpoints); if let Some(new_outputs) = new_outputs_option { @@ -2205,12 +2197,7 @@ impl Readable for (BlockHash, ChannelMonitor for _ in 0..counterparty_commitment_txn_on_chain_len { let txid: Txid = Readable::read(reader)?; let commitment_number = ::read(reader)?.0; - let outputs_count = ::read(reader)?; - let mut outputs = Vec::with_capacity(cmp::min(outputs_count as usize, MAX_ALLOC_SIZE / 8)); - for _ in 0..outputs_count { - outputs.push(Readable::read(reader)?); - } - if let Some(_) = counterparty_commitment_txn_on_chain.insert(txid, (commitment_number, outputs)) { + if let Some(_) = counterparty_commitment_txn_on_chain.insert(txid, commitment_number) { return Err(DecodeError::InvalidValue); } }