Retrieve all possible spendable outputs from transactions

Assuming our keys haven't been compromised, and that random transactions
aren't learning of these scripts somehow and sending funds to them, it
was only possible for one spendable output to exist within a
transaction.

- `shutdown_script` can only exist in co-op close transactions.
- `counterparty_payment_script` can only exist in counterparty
  commitment transactions.
- `broadcasted_holder_revokable_script` can only exist in holder
  commitment/HTLC transactions.
- `destination_script` can exist in any other type of claim we support.

Now that we're exposing this API to users such that they can rescan any
relevant transactions, there's no harm in allowing them to claim more
funds from spendable outputs than we expected.

Author: wpaulino

lightning/src/chain/channelmonitor.rs

@@ -1670,8 +1670,8 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
 		);
 	}
 
-	/// Returns the descriptor for a relevant output (i.e., one that we can spend) within the
-	/// transaction if one exists and the transaction has at least [`ANTI_REORG_DELAY`]
+	/// Returns the descriptors for relevant outputs (i.e., those that we can spend) within the
+	/// transaction if they exist and the transaction has at least [`ANTI_REORG_DELAY`]
 	/// confirmations.
 	///
 	/// Descriptors returned by this method are primarily exposed via [`Event::SpendableOutputs`]
@@ -1683,17 +1683,17 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
 	/// transactions starting from the channel's funding or closing transaction that have at least
 	/// [`ANTI_REORG_DELAY`] confirmations.
 	///
-	/// `tx` is a transaction we'll scan the outputs of. Any transaction can be provided. If an
-	/// output which can be spent by us is found, a descriptor is returned.
+	/// `tx` is a transaction we'll scan the outputs of. Any transaction can be provided. If any
+	/// outputs which can be spent by us are found, at least one descriptor is returned.
 	///
 	/// `confirmation_height` must be the height of the block in which `tx` was included in.
-	pub fn get_spendable_output(&self, tx: &Transaction, confirmation_height: u32) -> Option<SpendableOutputDescriptor> {
+	pub fn get_spendable_outputs(&self, tx: &Transaction, confirmation_height: u32) -> Vec<SpendableOutputDescriptor> {
 		let inner = self.inner.lock().unwrap();
 		let current_height = inner.best_block.height;
 		if current_height.saturating_sub(ANTI_REORG_DELAY) + 1 >= confirmation_height {
-			inner.get_spendable_output(tx)
+			inner.get_spendable_outputs(tx)
 		} else {
-			None
+			Vec::new()
 		}
 	}
 }
@@ -3468,7 +3468,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 				}
 				self.is_resolving_htlc_output(&tx, height, &block_hash, &logger);
 
-				self.check_tx_and_push_spendable_output(&tx, height, &block_hash, &logger);
+				self.check_tx_and_push_spendable_outputs(&tx, height, &block_hash, &logger);
 			}
 		}
 
@@ -4014,31 +4014,18 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		}
 	}
 
-	fn get_spendable_output(&self, tx: &Transaction) -> Option<SpendableOutputDescriptor> {
-		for (i, outp) in tx.output.iter().enumerate() { // There is max one spendable output for any channel tx, including ones generated by us
-			if i > ::core::u16::MAX as usize {
-				// While it is possible that an output exists on chain which is greater than the
-				// 2^16th output in a given transaction, this is only possible if the output is not
-				// in a lightning transaction and was instead placed there by some third party who
-				// wishes to give us money for no reason.
-				// Namely, any lightning transactions which we pre-sign will never have anywhere
-				// near 2^16 outputs both because such transactions must have ~2^16 outputs who's
-				// scripts are not longer than one byte in length and because they are inherently
-				// non-standard due to their size.
-				// Thus, it is completely safe to ignore such outputs, and while it may result in
-				// us ignoring non-lightning fund to us, that is only possible if someone fills
-				// nearly a full block with garbage just to hit this case.
-				continue;
-			}
+	fn get_spendable_outputs(&self, tx: &Transaction) -> Vec<SpendableOutputDescriptor> {
+		let mut spendable_outputs = Vec::new();
+		for (i, outp) in tx.output.iter().enumerate() {
 			if outp.script_pubkey == self.destination_script {
-				return Some(SpendableOutputDescriptor::StaticOutput {
+				spendable_outputs.push(SpendableOutputDescriptor::StaticOutput {
 					outpoint: OutPoint { txid: tx.txid(), index: i as u16 },
 					output: outp.clone(),
 				});
 			}
 			if let Some(ref broadcasted_holder_revokable_script) = self.broadcasted_holder_revokable_script {
 				if broadcasted_holder_revokable_script.0 == outp.script_pubkey {
-					return Some(SpendableOutputDescriptor::DelayedPaymentOutput(DelayedPaymentOutputDescriptor {
+					spendable_outputs.push(SpendableOutputDescriptor::DelayedPaymentOutput(DelayedPaymentOutputDescriptor {
 						outpoint: OutPoint { txid: tx.txid(), index: i as u16 },
 						per_commitment_point: broadcasted_holder_revokable_script.1,
 						to_self_delay: self.on_holder_tx_csv,
@@ -4050,29 +4037,29 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 				}
 			}
 			if self.counterparty_payment_script == outp.script_pubkey {
-				return Some(SpendableOutputDescriptor::StaticPaymentOutput(StaticPaymentOutputDescriptor {
+				spendable_outputs.push(SpendableOutputDescriptor::StaticPaymentOutput(StaticPaymentOutputDescriptor {
 					outpoint: OutPoint { txid: tx.txid(), index: i as u16 },
 					output: outp.clone(),
 					channel_keys_id: self.channel_keys_id,
 					channel_value_satoshis: self.channel_value_satoshis,
 				}));
 			}
 			if self.shutdown_script.as_ref() == Some(&outp.script_pubkey) {
-				return Some(SpendableOutputDescriptor::StaticOutput {
+				spendable_outputs.push(SpendableOutputDescriptor::StaticOutput {
 					outpoint: OutPoint { txid: tx.txid(), index: i as u16 },
 					output: outp.clone(),
 				});
 			}
 		}
-		None
+		spendable_outputs
 	}
 
 	/// Checks if the confirmed transaction is paying funds back to some address we can assume to
 	/// own.
-	fn check_tx_and_push_spendable_output<L: Deref>(
+	fn check_tx_and_push_spendable_outputs<L: Deref>(
 		&mut self, tx: &Transaction, height: u32, block_hash: &BlockHash, logger: &L,
 	) where L::Target: Logger {
-		if let Some(spendable_output) = self.get_spendable_output(tx) {
+		for spendable_output in self.get_spendable_outputs(tx) {
 			let entry = OnchainEventEntry {
 				txid: tx.txid(),
 				transaction: Some(tx.clone()),

lightning/src/ln/monitor_tests.rs

@@ -93,15 +93,15 @@ fn chanmon_fail_from_stale_commitment() {
 	expect_payment_failed_with_update!(nodes[0], payment_hash, false, update_a.contents.short_channel_id, true);
 }
 
-fn test_spendable_output<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, spendable_tx: &Transaction) -> SpendableOutputDescriptor {
+fn test_spendable_output<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, spendable_tx: &Transaction) -> Vec<SpendableOutputDescriptor> {
 	let mut spendable = node.chain_monitor.chain_monitor.get_and_clear_pending_events();
 	assert_eq!(spendable.len(), 1);
-	if let Event::SpendableOutputs { mut outputs, .. } = spendable.pop().unwrap() {
+	if let Event::SpendableOutputs { outputs, .. } = spendable.pop().unwrap() {
 		assert_eq!(outputs.len(), 1);
 		let spend_tx = node.keys_manager.backing.spend_spendable_outputs(&[&outputs[0]], Vec::new(),
 			Builder::new().push_opcode(opcodes::all::OP_RETURN).into_script(), 253, None, &Secp256k1::new()).unwrap();
 		check_spends!(spend_tx, spendable_tx);
-		outputs.pop().unwrap()
+		outputs
 	} else { panic!(); }
 }
 
@@ -222,9 +222,9 @@ fn chanmon_claim_value_coop_close() {
 	connect_blocks(&nodes[1], ANTI_REORG_DELAY - 2);
 
 	assert!(get_monitor!(nodes[0], chan_id)
-		.get_spendable_output(&shutdown_tx[0], shutdown_tx_conf_height_a).is_none());
+		.get_spendable_outputs(&shutdown_tx[0], shutdown_tx_conf_height_a).is_empty());
 	assert!(get_monitor!(nodes[1], chan_id)
-		.get_spendable_output(&shutdown_tx[0], shutdown_tx_conf_height_b).is_none());
+		.get_spendable_outputs(&shutdown_tx[0], shutdown_tx_conf_height_b).is_empty());
 
 	connect_blocks(&nodes[0], 1);
 	connect_blocks(&nodes[1], 1);
@@ -234,18 +234,16 @@ fn chanmon_claim_value_coop_close() {
 	assert_eq!(Vec::<Balance>::new(),
 		nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances());
 
-	let spendable_output_a = test_spendable_output(&nodes[0], &shutdown_tx[0]);
+	let spendable_outputs_a = test_spendable_output(&nodes[0], &shutdown_tx[0]);
 	assert_eq!(
-		get_monitor!(nodes[0], chan_id)
-			.get_spendable_output(&shutdown_tx[0], shutdown_tx_conf_height_a).unwrap(),
-		spendable_output_a
+		get_monitor!(nodes[0], chan_id).get_spendable_outputs(&shutdown_tx[0], shutdown_tx_conf_height_a),
+		spendable_outputs_a
 	);
 
-	let spendable_output_b = test_spendable_output(&nodes[1], &shutdown_tx[0]);
+	let spendable_outputs_b = test_spendable_output(&nodes[1], &shutdown_tx[0]);
 	assert_eq!(
-		get_monitor!(nodes[1], chan_id)
-			.get_spendable_output(&shutdown_tx[0], shutdown_tx_conf_height_b).unwrap(),
-		spendable_output_b
+		get_monitor!(nodes[1], chan_id).get_spendable_outputs(&shutdown_tx[0], shutdown_tx_conf_height_b),
+		spendable_outputs_b
 	);
 
 	check_closed_event!(nodes[0], 1, ClosureReason::CooperativeClosure, [nodes[1].node.get_our_node_id()], 1000000);