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.

Author: Antoine Riard

lightning/src/chain/chainmonitor.rs

@@ -95,8 +95,8 @@ impl<ChanSigner: ChannelKeys, C: Deref, T: Deref, F: Deref, L: Deref> 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<ChanSigner: ChannelKeys, C: Deref, T: Deref, F: Deref, L: Deref> 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);
 					}
 				}
 			}

lightning/src/chain/channelmonitor.rs

@@ -666,7 +666,7 @@ pub struct ChannelMonitor<ChanSigner: ChannelKeys> {
 	// 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<Txid, Vec<Script>>,
+	outputs_to_watch: HashMap<Txid, Vec<(u32, Script)>>,
 
 	#[cfg(test)]
 	pub onchain_tx_handler: OnchainTxHandler<ChanSigner>,
@@ -914,10 +914,11 @@ impl<ChanSigner: ChannelKeys + Writeable> ChannelMonitor<ChanSigner> {
 		}
 
 		(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<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 		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<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 	/// 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<Txid, Vec<Script>> {
+	pub fn get_outputs_to_watch(&self) -> &HashMap<Txid, Vec<(u32, Script)>> {
 		// 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<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 	/// 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<L: Deref>(&mut self, tx: &Transaction, height: u32, logger: &L) -> (Vec<ClaimRequest>, (Txid, Vec<TxOut>)) where L::Target: Logger {
+	fn check_spend_counterparty_transaction<L: Deref>(&mut self, tx: &Transaction, height: u32, logger: &L) -> (Vec<ClaimRequest>, (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<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 			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<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 			// 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<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 	}
 
 	/// Attempts to claim a counterparty HTLC-Success/HTLC-Timeout's outputs using the revocation key
-	fn check_spend_counterparty_htlc<L: Deref>(&mut self, tx: &Transaction, commitment_number: u64, height: u32, logger: &L) -> (Vec<ClaimRequest>, Option<(Txid, Vec<TxOut>)>) where L::Target: Logger {
+	fn check_spend_counterparty_htlc<L: Deref>(&mut self, tx: &Transaction, commitment_number: u64, height: u32, logger: &L) -> (Vec<ClaimRequest>, 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<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 		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<ClaimRequest>, Vec<TxOut>, Option<(Script, PublicKey, PublicKey)>) {
+	fn broadcast_by_holder_state(&self, commitment_tx: &Transaction, holder_tx: &HolderSignedTx) -> (Vec<ClaimRequest>, 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<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 							} 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<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 	/// 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<L: Deref>(&mut self, tx: &Transaction, height: u32, logger: &L) -> (Vec<ClaimRequest>, (Txid, Vec<TxOut>)) where L::Target: Logger {
+	fn check_spend_holder_transaction<L: Deref>(&mut self, tx: &Transaction, height: u32, logger: &L) -> (Vec<ClaimRequest>, (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<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 	/// [`get_outputs_to_watch`].
 	///
 	/// [`get_outputs_to_watch`]: #method.get_outputs_to_watch
-	pub fn block_connected<B: Deref, F: Deref, L: Deref>(&mut self, header: &BlockHeader, txdata: &TransactionData, height: u32, broadcaster: B, fee_estimator: F, logger: L)-> Vec<(Txid, Vec<TxOut>)>
+	pub fn block_connected<B: Deref, F: Deref, L: Deref>(&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<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 		// 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<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 	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<ChanSigner: ChannelKeys + Readable> 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::<Txid>() + mem::size_of::<Vec<Script>>())));
+		let mut outputs_to_watch = HashMap::with_capacity(cmp::min(outputs_to_watch_len as usize, MAX_ALLOC_SIZE / (mem::size_of::<Txid>() + mem::size_of::<u32>() + mem::size_of::<Vec<Script>>())));
 		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::<Script>()));
+			let mut outputs = Vec::with_capacity(cmp::min(outputs_len as usize, MAX_ALLOC_SIZE / (mem::size_of::<u32>() + mem::size_of::<Script>())));
 			for _ in 0..outputs_len {
-				outputs.push(Readable::read(reader)?);
+				outputs.push((Readable::read(reader)?, Readable::read(reader)?));
 			}
 			if let Some(_) = outputs_to_watch.insert(txid, outputs) {
 				return Err(DecodeError::InvalidValue);