Use FundingTxInput instead of Utxo in CoinSelection

In order to reuse CoinSelectionSource for splicing, the previous
transaction of each UTXO is needed. Update CoinSelection to use
FundingTxInput (renamed to ConfirmedUtxo) so that it is available.

This requires adding a method to WalletSource to look up a previous
transaction for a UTXO. Otherwise, Wallet's implementation of
CoinSelectionSource would need WalletSource to include the previous
transactions when listing confirmed UTXOs to select from. But this would
be inefficient since only some UTXOs are selected.

Author: jkczyz

lightning/src/events/bump_transaction/mod.rs

@@ -30,6 +30,7 @@ use crate::ln::chan_utils::{
 	HTLC_TIMEOUT_INPUT_KEYED_ANCHOR_WITNESS_WEIGHT, HTLC_TIMEOUT_INPUT_P2A_ANCHOR_WITNESS_WEIGHT,
 	P2WSH_TXOUT_WEIGHT, SEGWIT_MARKER_FLAG_WEIGHT, TRUC_CHILD_MAX_WEIGHT, TRUC_MAX_WEIGHT,
 };
+use crate::ln::funding::FundingTxInput;
 use crate::ln::types::ChannelId;
 use crate::prelude::*;
 use crate::sign::ecdsa::EcdsaChannelSigner;
@@ -340,20 +341,33 @@ impl Utxo {
 	}
 }
 
+/// An unspent transaction output with at least one confirmation.
+pub type ConfirmedUtxo = FundingTxInput;
+
 /// The result of a successful coin selection attempt for a transaction requiring additional UTXOs
 /// to cover its fees.
 #[derive(Clone, Debug)]
 pub struct CoinSelection {
 	/// The set of UTXOs (with at least 1 confirmation) to spend and use within a transaction
 	/// requiring additional fees.
-	pub confirmed_utxos: Vec<Utxo>,
+	pub confirmed_utxos: Vec<ConfirmedUtxo>,
 	/// An additional output tracking whether any change remained after coin selection. This output
 	/// should always have a value above dust for its given `script_pubkey`. It should not be
 	/// spent until the transaction it belongs to confirms to ensure mempool descendant limits are
 	/// not met. This implies no other party should be able to spend it except us.
 	pub change_output: Option<TxOut>,
 }
 
+impl CoinSelection {
+	fn satisfaction_weight(&self) -> u64 {
+		self.confirmed_utxos.iter().map(|ConfirmedUtxo { utxo, .. }| utxo.satisfaction_weight).sum()
+	}
+
+	fn amount(&self) -> Amount {
+		self.confirmed_utxos.iter().map(|ConfirmedUtxo { utxo, .. }| utxo.output.value).sum()
+	}
+}
+
 /// An abstraction over a bitcoin wallet that can perform coin selection over a set of UTXOs and can
 /// sign for them. The coin selection method aims to mimic Bitcoin Core's `fundrawtransaction` RPC,
 /// which most wallets should be able to satisfy. Otherwise, consider implementing [`WalletSource`],
@@ -424,11 +438,18 @@ pub trait WalletSource {
 	fn list_confirmed_utxos<'a>(
 		&'a self,
 	) -> impl Future<Output = Result<Vec<Utxo>, ()>> + MaybeSend + 'a;
+
+	/// Returns the previous transaction containing the UTXO.
+	fn get_prevtx<'a>(
+		&'a self, utxo: &Utxo,
+	) -> impl Future<Output = Result<Transaction, ()>> + MaybeSend + 'a;
+
 	/// Returns a script to use for change above dust resulting from a successful coin selection
 	/// attempt.
 	fn get_change_script<'a>(
 		&'a self,
 	) -> impl Future<Output = Result<ScriptBuf, ()>> + MaybeSend + 'a;
+
 	/// Signs and provides the full [`TxIn::script_sig`] and [`TxIn::witness`] for all inputs within
 	/// the transaction known to the wallet (i.e., any provided via
 	/// [`WalletSource::list_confirmed_utxos`]).
@@ -616,10 +637,13 @@ where
 			Some(TxOut { script_pubkey: change_script, value: change_output_amount })
 		};
 
-		Ok(CoinSelection {
-			confirmed_utxos: selected_utxos.into_iter().map(|(utxo, _)| utxo).collect(),
-			change_output,
-		})
+		let mut confirmed_utxos = Vec::with_capacity(selected_utxos.len());
+		for (utxo, _) in selected_utxos {
+			let prevtx = self.source.get_prevtx(&utxo).await?;
+			confirmed_utxos.push(ConfirmedUtxo { utxo, prevtx });
+		}
+
+		Ok(CoinSelection { confirmed_utxos, change_output })
 	}
 }
 
@@ -730,7 +754,7 @@ where
 
 	/// Updates a transaction with the result of a successful coin selection attempt.
 	fn process_coin_selection(&self, tx: &mut Transaction, coin_selection: &CoinSelection) {
-		for utxo in coin_selection.confirmed_utxos.iter() {
+		for ConfirmedUtxo { utxo, .. } in coin_selection.confirmed_utxos.iter() {
 			tx.input.push(TxIn {
 				previous_output: utxo.outpoint,
 				script_sig: ScriptBuf::new(),
@@ -852,12 +876,10 @@ where
 				output: vec![],
 			};
 
-			let input_satisfaction_weight: u64 =
-				coin_selection.confirmed_utxos.iter().map(|utxo| utxo.satisfaction_weight).sum();
+			let input_satisfaction_weight = coin_selection.satisfaction_weight();
 			let total_satisfaction_weight =
 				anchor_input_witness_weight + EMPTY_SCRIPT_SIG_WEIGHT + input_satisfaction_weight;
-			let total_input_amount = must_spend_amount
-				+ coin_selection.confirmed_utxos.iter().map(|utxo| utxo.output.value).sum();
+			let total_input_amount = must_spend_amount + coin_selection.amount();
 
 			self.process_coin_selection(&mut anchor_tx, &coin_selection);
 			let anchor_txid = anchor_tx.compute_txid();
@@ -872,10 +894,10 @@ where
 				let index = idx + 1;
 				debug_assert_eq!(
 					anchor_psbt.unsigned_tx.input[index].previous_output,
-					utxo.outpoint
+					utxo.outpoint()
 				);
-				if utxo.output.script_pubkey.is_witness_program() {
-					anchor_psbt.inputs[index].witness_utxo = Some(utxo.output);
+				if utxo.output().script_pubkey.is_witness_program() {
+					anchor_psbt.inputs[index].witness_utxo = Some(utxo.into_output());
 				}
 			}
 
@@ -1107,13 +1129,11 @@ where
 			utxo_id = claim_id.step_with_bytes(&broadcasted_htlcs.to_be_bytes());
 
 			#[cfg(debug_assertions)]
-			let input_satisfaction_weight: u64 =
-				coin_selection.confirmed_utxos.iter().map(|utxo| utxo.satisfaction_weight).sum();
+			let input_satisfaction_weight = coin_selection.satisfaction_weight();
 			#[cfg(debug_assertions)]
 			let total_satisfaction_weight = must_spend_satisfaction_weight + input_satisfaction_weight;
 			#[cfg(debug_assertions)]
-			let input_value: u64 =
-				coin_selection.confirmed_utxos.iter().map(|utxo| utxo.output.value.to_sat()).sum();
+			let input_value = coin_selection.amount().to_sat();
 			#[cfg(debug_assertions)]
 			let total_input_amount = must_spend_amount + input_value;
 
@@ -1134,9 +1154,12 @@ where
 			for (idx, utxo) in coin_selection.confirmed_utxos.into_iter().enumerate() {
 				// offset to skip the htlc inputs
 				let index = idx + selected_htlcs.len();
-				debug_assert_eq!(htlc_psbt.unsigned_tx.input[index].previous_output, utxo.outpoint);
-				if utxo.output.script_pubkey.is_witness_program() {
-					htlc_psbt.inputs[index].witness_utxo = Some(utxo.output);
+				debug_assert_eq!(
+					htlc_psbt.unsigned_tx.input[index].previous_output,
+					utxo.outpoint()
+				);
+				if utxo.output().script_pubkey.is_witness_program() {
+					htlc_psbt.inputs[index].witness_utxo = Some(utxo.into_output());
 				}
 			}
 
@@ -1280,9 +1303,8 @@ mod tests {
 	use crate::util::ser::Readable;
 	use crate::util::test_utils::{TestBroadcaster, TestLogger};
 
-	use bitcoin::hashes::Hash;
 	use bitcoin::hex::FromHex;
-	use bitcoin::{Network, ScriptBuf, Transaction, Txid};
+	use bitcoin::{Network, ScriptBuf, Transaction};
 
 	struct TestCoinSelectionSource {
 		// (commitment + anchor value, commitment + input weight, target feerate, result)
@@ -1302,8 +1324,12 @@ mod tests {
 			Ok(res)
 		}
 		fn sign_psbt(&self, psbt: Psbt) -> Result<Transaction, ()> {
-			// FIXME: Why does handle_channel_close override the witness?
-			Ok(psbt.unsigned_tx)
+			let mut tx = psbt.unsigned_tx;
+			// Channel output, add a realistic size witness to make the assertions happy
+			//
+			// FIXME: This doesn't seem to be needed since handle_channel_close overrides it
+			tx.input.first_mut().unwrap().witness = Witness::from_slice(&[vec![42; 162]]);
+			Ok(tx)
 		}
 	}
 
@@ -1339,6 +1365,13 @@ mod tests {
 				.weight()
 				.to_wu();
 
+		let prevtx = Transaction {
+			version: Version::TWO,
+			lock_time: LockTime::ZERO,
+			input: vec![],
+			output: vec![TxOut { value: Amount::from_sat(200), script_pubkey: ScriptBuf::new() }],
+		};
+
 		let broadcaster = TestBroadcaster::new(Network::Testnet);
 		let source = TestCoinSelectionSource {
 			expected_selects: Mutex::new(vec![
@@ -1353,14 +1386,14 @@ mod tests {
 					commitment_and_anchor_fee,
 					868,
 					CoinSelection {
-						confirmed_utxos: vec![Utxo {
-							outpoint: OutPoint { txid: Txid::from_byte_array([44; 32]), vout: 0 },
-							output: TxOut {
-								value: Amount::from_sat(200),
-								script_pubkey: ScriptBuf::new(),
+						confirmed_utxos: vec![ConfirmedUtxo {
+							utxo: Utxo {
+								outpoint: OutPoint { txid: prevtx.compute_txid(), vout: 0 },
+								output: prevtx.output[0].clone(),
+								satisfaction_weight: 5, // Just the script_sig and witness lengths
+								sequence: Sequence::ENABLE_RBF_NO_LOCKTIME,
 							},
-							satisfaction_weight: 5, // Just the script_sig and witness lengths
-							sequence: Sequence::ENABLE_RBF_NO_LOCKTIME,
+							prevtx,
 						}],
 						change_output: None,
 					},

lightning/src/events/bump_transaction/sync.rs

@@ -37,9 +37,14 @@ use super::{
 pub trait WalletSourceSync {
 	/// Returns all UTXOs, with at least 1 confirmation each, that are available to spend.
 	fn list_confirmed_utxos(&self) -> Result<Vec<Utxo>, ()>;
+
+	/// Returns the previous transaction containing the UTXO.
+	fn get_prevtx(&self, utxo: &Utxo) -> Result<Transaction, ()>;
+
 	/// Returns a script to use for change above dust resulting from a successful coin selection
 	/// attempt.
 	fn get_change_script(&self) -> Result<ScriptBuf, ()>;
+
 	/// Signs and provides the full [`TxIn::script_sig`] and [`TxIn::witness`] for all inputs within
 	/// the transaction known to the wallet (i.e., any provided via
 	/// [`WalletSource::list_confirmed_utxos`]).
@@ -79,6 +84,14 @@ where
 		async move { utxos }
 	}
 
+	/// Returns the previous transaction containing the UTXO.
+	fn get_prevtx<'a>(
+		&'a self, utxo: &Utxo,
+	) -> impl Future<Output = Result<Transaction, ()>> + MaybeSend + 'a {
+		let prevtx = self.0.get_prevtx(utxo);
+		Box::pin(async move { prevtx })
+	}
+
 	fn get_change_script<'a>(
 		&'a self,
 	) -> impl Future<Output = Result<ScriptBuf, ()>> + MaybeSend + 'a {

lightning/src/ln/functional_test_utils.rs

@@ -395,8 +395,7 @@ fn do_connect_block_without_consistency_checks<'a, 'b, 'c, 'd>(
 		let wallet_script = node.wallet_source.get_change_script().unwrap();
 		for (idx, output) in tx.output.iter().enumerate() {
 			if output.script_pubkey == wallet_script {
-				let outpoint = bitcoin::OutPoint { txid: tx.compute_txid(), vout: idx as u32 };
-				node.wallet_source.add_utxo(outpoint, output.value);
+				node.wallet_source.add_utxo(tx.clone(), idx as u32);
 			}
 		}
 	}

lightning/src/ln/funding.rs

@@ -103,16 +103,17 @@ impl SpliceContribution {
 /// establishment protocol or when splicing.
 #[derive(Debug, Clone)]
 pub struct FundingTxInput {
-	/// The unspent [`TxOut`] that the input spends.
+	/// The unspent [`TxOut`] found in [`prevtx`].
 	///
 	/// [`TxOut`]: bitcoin::TxOut
-	pub(super) utxo: Utxo,
+	/// [`prevtx`]: Self::prevtx
+	pub(crate) utxo: Utxo,
 
 	/// The transaction containing the unspent [`TxOut`] referenced by [`utxo`].
 	///
 	/// [`TxOut`]: bitcoin::TxOut
 	/// [`utxo`]: Self::utxo
-	pub(super) prevtx: Transaction,
+	pub(crate) prevtx: Transaction,
 }
 
 impl_writeable_tlv_based!(FundingTxInput, {
@@ -225,6 +226,11 @@ impl FundingTxInput {
 		self.utxo.outpoint
 	}
 
+	/// The unspent output.
+	pub fn output(&self) -> &TxOut {
+		&self.utxo.output
+	}
+
 	/// The sequence number to use in the [`TxIn`].
 	///
 	/// [`TxIn`]: bitcoin::TxIn
@@ -239,8 +245,13 @@ impl FundingTxInput {
 		self.utxo.sequence = sequence;
 	}
 
-	/// Converts the [`FundingTxInput`] into a [`Utxo`] for coin selection.
+	/// Converts the [`FundingTxInput`] into a [`Utxo`].
 	pub fn into_utxo(self) -> Utxo {
 		self.utxo
 	}
+
+	/// Converts the [`FundingTxInput`] into a [`TxOut`].
+	pub fn into_output(self) -> TxOut {
+		self.utxo.output
+	}
 }

lightning/src/util/test_utils.rs

@@ -22,7 +22,7 @@ use crate::chain::channelmonitor::{
 use crate::chain::transaction::OutPoint;
 use crate::chain::WatchedOutput;
 use crate::events::bump_transaction::sync::WalletSourceSync;
-use crate::events::bump_transaction::Utxo;
+use crate::events::bump_transaction::{ConfirmedUtxo, Utxo};
 #[cfg(any(test, feature = "_externalize_tests"))]
 use crate::ln::chan_utils::CommitmentTransaction;
 use crate::ln::channel_state::ChannelDetails;
@@ -2211,7 +2211,7 @@ impl Drop for TestScorer {
 
 pub struct TestWalletSource {
 	secret_key: SecretKey,
-	utxos: Mutex<Vec<Utxo>>,
+	utxos: Mutex<Vec<ConfirmedUtxo>>,
 	secp: Secp256k1<bitcoin::secp256k1::All>,
 }
 
@@ -2220,21 +2220,13 @@ impl TestWalletSource {
 		Self { secret_key, utxos: Mutex::new(Vec::new()), secp: Secp256k1::new() }
 	}
 
-	pub fn add_utxo(&self, outpoint: bitcoin::OutPoint, value: Amount) -> TxOut {
-		let public_key = bitcoin::PublicKey::new(self.secret_key.public_key(&self.secp));
-		let utxo = Utxo::new_v0_p2wpkh(outpoint, value, &public_key.wpubkey_hash().unwrap());
-		self.utxos.lock().unwrap().push(utxo.clone());
-		utxo.output
-	}
-
-	pub fn add_custom_utxo(&self, utxo: Utxo) -> TxOut {
-		let output = utxo.output.clone();
+	pub fn add_utxo(&self, prevtx: Transaction, vout: u32) {
+		let utxo = ConfirmedUtxo::new_p2wpkh(prevtx, vout).unwrap();
 		self.utxos.lock().unwrap().push(utxo);
-		output
 	}
 
 	pub fn remove_utxo(&self, outpoint: bitcoin::OutPoint) {
-		self.utxos.lock().unwrap().retain(|utxo| utxo.outpoint != outpoint);
+		self.utxos.lock().unwrap().retain(|utxo| utxo.outpoint() != outpoint);
 	}
 
 	pub fn clear_utxos(&self) {
@@ -2247,12 +2239,12 @@ impl TestWalletSource {
 		let utxos = self.utxos.lock().unwrap();
 		for i in 0..tx.input.len() {
 			if let Some(utxo) =
-				utxos.iter().find(|utxo| utxo.outpoint == tx.input[i].previous_output)
+				utxos.iter().find(|utxo| utxo.outpoint() == tx.input[i].previous_output)
 			{
 				let sighash = SighashCache::new(&tx).p2wpkh_signature_hash(
 					i,
-					&utxo.output.script_pubkey,
-					utxo.output.value,
+					&utxo.output().script_pubkey,
+					utxo.output().value,
 					EcdsaSighashType::All,
 				)?;
 				#[cfg(not(feature = "grind_signatures"))]
@@ -2277,7 +2269,23 @@ impl TestWalletSource {
 
 impl WalletSourceSync for TestWalletSource {
 	fn list_confirmed_utxos(&self) -> Result<Vec<Utxo>, ()> {
-		Ok(self.utxos.lock().unwrap().clone())
+		Ok(self
+			.utxos
+			.lock()
+			.unwrap()
+			.iter()
+			.map(|ConfirmedUtxo { utxo, .. }| utxo.clone())
+			.collect())
+	}
+
+	fn get_prevtx(&self, utxo: &Utxo) -> Result<Transaction, ()> {
+		self.utxos
+			.lock()
+			.unwrap()
+			.iter()
+			.find(|confirmed_utxo| confirmed_utxo.utxo == *utxo)
+			.map(|ConfirmedUtxo { prevtx, .. }| prevtx.clone())
+			.ok_or(())
 	}
 
 	fn get_change_script(&self) -> Result<ScriptBuf, ()> {