Merge pull request lightningdevkit#2753 from TheBlueMatt/2023-11-inbound-preimages

Provide inbound HTLC preimages to the `EcdsaChannelSigner`

Author: wpaulino

lightning/src/ln/channel.rs

@@ -512,7 +512,8 @@ struct CommitmentStats<'a> {
 	htlcs_included: Vec<(HTLCOutputInCommitment, Option<&'a HTLCSource>)>, // the list of HTLCs (dust HTLCs *included*) which were not ignored when building the transaction
 	local_balance_msat: u64, // local balance before fees but considering dust limits
 	remote_balance_msat: u64, // remote balance before fees but considering dust limits
-	preimages: Vec<PaymentPreimage>, // preimages for successful offered HTLCs since last commitment
+	outbound_htlc_preimages: Vec<PaymentPreimage>, // preimages for successful offered HTLCs since last commitment
+	inbound_htlc_preimages: Vec<PaymentPreimage>, // preimages for successful received HTLCs since last commitment
 }
 
 /// Used when calculating whether we or the remote can afford an additional HTLC.
@@ -1428,6 +1429,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider  {
 			}
 		}
 
+		let mut inbound_htlc_preimages: Vec<PaymentPreimage> = Vec::new();
+
 		for ref htlc in self.pending_inbound_htlcs.iter() {
 			let (include, state_name) = match htlc.state {
 				InboundHTLCState::RemoteAnnounced(_) => (!generated_by_local, "RemoteAnnounced"),
@@ -1445,7 +1448,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider  {
 				match &htlc.state {
 					&InboundHTLCState::LocalRemoved(ref reason) => {
 						if generated_by_local {
-							if let &InboundHTLCRemovalReason::Fulfill(_) = reason {
+							if let &InboundHTLCRemovalReason::Fulfill(preimage) = reason {
+								inbound_htlc_preimages.push(preimage);
 								value_to_self_msat_offset += htlc.amount_msat as i64;
 							}
 						}
@@ -1455,7 +1459,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider  {
 			}
 		}
 
-		let mut preimages: Vec<PaymentPreimage> = Vec::new();
+
+		let mut outbound_htlc_preimages: Vec<PaymentPreimage> = Vec::new();
 
 		for ref htlc in self.pending_outbound_htlcs.iter() {
 			let (include, state_name) = match htlc.state {
@@ -1474,7 +1479,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider  {
 			};
 
 			if let Some(preimage) = preimage_opt {
-				preimages.push(preimage);
+				outbound_htlc_preimages.push(preimage);
 			}
 
 			if include {
@@ -1580,7 +1585,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider  {
 			htlcs_included,
 			local_balance_msat: value_to_self_msat as u64,
 			remote_balance_msat: value_to_remote_msat as u64,
-			preimages
+			inbound_htlc_preimages,
+			outbound_htlc_preimages,
 		}
 	}
 
@@ -2178,7 +2184,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider  {
 		let signature = match &self.holder_signer {
 			// TODO (taproot|arik): move match into calling method for Taproot
 			ChannelSignerType::Ecdsa(ecdsa) => {
-				ecdsa.sign_counterparty_commitment(&counterparty_initial_commitment_tx, Vec::new(), &self.secp_ctx)
+				ecdsa.sign_counterparty_commitment(&counterparty_initial_commitment_tx, Vec::new(), Vec::new(), &self.secp_ctx)
 					.map(|(sig, _)| sig).ok()?
 			},
 			// TODO (taproot|arik)
@@ -2216,7 +2222,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider  {
 		match &self.holder_signer {
 			// TODO (arik): move match into calling method for Taproot
 			ChannelSignerType::Ecdsa(ecdsa) => {
-				let funding_signed = ecdsa.sign_counterparty_commitment(&counterparty_initial_commitment_tx, Vec::new(), &self.secp_ctx)
+				let funding_signed = ecdsa.sign_counterparty_commitment(&counterparty_initial_commitment_tx, Vec::new(), Vec::new(), &self.secp_ctx)
 					.map(|(signature, _)| msgs::FundingSigned {
 						channel_id: self.channel_id(),
 						signature,
@@ -3247,7 +3253,7 @@ impl<SP: Deref> Channel<SP> where
 			self.context.counterparty_funding_pubkey()
 		);
 
-		self.context.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, commitment_stats.preimages)
+		self.context.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, commitment_stats.outbound_htlc_preimages)
 			.map_err(|_| ChannelError::Close("Failed to validate our commitment".to_owned()))?;
 
 		// Update state now that we've passed all the can-fail calls...
@@ -5780,8 +5786,12 @@ impl<SP: Deref> Channel<SP> where
 						htlcs.push(htlc);
 					}
 
-					let res = ecdsa.sign_counterparty_commitment(&commitment_stats.tx, commitment_stats.preimages, &self.context.secp_ctx)
-						.map_err(|_| ChannelError::Ignore("Failed to get signatures for new commitment_signed".to_owned()))?;
+					let res = ecdsa.sign_counterparty_commitment(
+							&commitment_stats.tx,
+							commitment_stats.inbound_htlc_preimages,
+							commitment_stats.outbound_htlc_preimages,
+							&self.context.secp_ctx,
+						).map_err(|_| ChannelError::Ignore("Failed to get signatures for new commitment_signed".to_owned()))?;
 					signature = res.0;
 					htlc_signatures = res.1;
 

lightning/src/ln/functional_tests.rs

@@ -746,7 +746,7 @@ fn test_update_fee_that_funder_cannot_afford() {
 			&mut htlcs,
 			&local_chan.context.channel_transaction_parameters.as_counterparty_broadcastable()
 		);
-		local_chan_signer.as_ecdsa().unwrap().sign_counterparty_commitment(&commitment_tx, Vec::new(), &secp_ctx).unwrap()
+		local_chan_signer.as_ecdsa().unwrap().sign_counterparty_commitment(&commitment_tx, Vec::new(), Vec::new(), &secp_ctx).unwrap()
 	};
 
 	let commit_signed_msg = msgs::CommitmentSigned {
@@ -1494,7 +1494,7 @@ fn test_fee_spike_violation_fails_htlc() {
 			&mut vec![(accepted_htlc_info, ())],
 			&local_chan.context.channel_transaction_parameters.as_counterparty_broadcastable()
 		);
-		local_chan_signer.as_ecdsa().unwrap().sign_counterparty_commitment(&commitment_tx, Vec::new(), &secp_ctx).unwrap()
+		local_chan_signer.as_ecdsa().unwrap().sign_counterparty_commitment(&commitment_tx, Vec::new(), Vec::new(), &secp_ctx).unwrap()
 	};
 
 	let commit_signed_msg = msgs::CommitmentSigned {

lightning/src/sign/ecdsa.rs

@@ -29,16 +29,18 @@ pub trait EcdsaChannelSigner: ChannelSigner {
 	/// Policy checks should be implemented in this function, including checking the amount
 	/// sent to us and checking the HTLCs.
 	///
-	/// The preimages of outgoing HTLCs that were fulfilled since the last commitment are provided.
-	/// A validating signer should ensure that an HTLC output is removed only when the matching
-	/// preimage is provided, or when the value to holder is restored.
+	/// The preimages of outbound and inbound HTLCs that were fulfilled since the last commitment
+	/// are provided. A validating signer should ensure that an outbound HTLC output is removed
+	/// only when the matching preimage is provided and after the corresponding inbound HTLC has
+	/// been removed for forwarded payments.
 	///
 	/// Note that all the relevant preimages will be provided, but there may also be additional
 	/// irrelevant or duplicate preimages.
 	//
 	// TODO: Document the things someone using this interface should enforce before signing.
 	fn sign_counterparty_commitment(&self, commitment_tx: &CommitmentTransaction,
-		preimages: Vec<PaymentPreimage>, secp_ctx: &Secp256k1<secp256k1::All>
+		inbound_htlc_preimages: Vec<PaymentPreimage>,
+		outbound_htlc_preimages: Vec<PaymentPreimage>, secp_ctx: &Secp256k1<secp256k1::All>,
 	) -> Result<(Signature, Vec<Signature>), ()>;
 	/// Creates a signature for a holder's commitment transaction.
 	///

lightning/src/sign/mod.rs

@@ -594,14 +594,14 @@ pub trait ChannelSigner {
 	/// Policy checks should be implemented in this function, including checking the amount
 	/// sent to us and checking the HTLCs.
 	///
-	/// The preimages of outgoing HTLCs that were fulfilled since the last commitment are provided.
+	/// The preimages of outbound HTLCs that were fulfilled since the last commitment are provided.
 	/// A validating signer should ensure that an HTLC output is removed only when the matching
 	/// preimage is provided, or when the value to holder is restored.
 	///
 	/// Note that all the relevant preimages will be provided, but there may also be additional
 	/// irrelevant or duplicate preimages.
 	fn validate_holder_commitment(&self, holder_tx: &HolderCommitmentTransaction,
-		preimages: Vec<PaymentPreimage>) -> Result<(), ()>;
+		outbound_htlc_preimages: Vec<PaymentPreimage>) -> Result<(), ()>;
 
 	/// Validate the counterparty's revocation.
 	///
@@ -1086,7 +1086,7 @@ impl ChannelSigner for InMemorySigner {
 		chan_utils::build_commitment_secret(&self.commitment_seed, idx)
 	}
 
-	fn validate_holder_commitment(&self, _holder_tx: &HolderCommitmentTransaction, _preimages: Vec<PaymentPreimage>) -> Result<(), ()> {
+	fn validate_holder_commitment(&self, _holder_tx: &HolderCommitmentTransaction, _outbound_htlc_preimages: Vec<PaymentPreimage>) -> Result<(), ()> {
 		Ok(())
 	}
 
@@ -1112,7 +1112,7 @@ impl ChannelSigner for InMemorySigner {
 const MISSING_PARAMS_ERR: &'static str = "ChannelSigner::provide_channel_parameters must be called before signing operations";
 
 impl EcdsaChannelSigner for InMemorySigner {
-	fn sign_counterparty_commitment(&self, commitment_tx: &CommitmentTransaction, _preimages: Vec<PaymentPreimage>, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()> {
+	fn sign_counterparty_commitment(&self, commitment_tx: &CommitmentTransaction, _inbound_htlc_preimages: Vec<PaymentPreimage>, _outbound_htlc_preimages: Vec<PaymentPreimage>, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()> {
 		let trusted_tx = commitment_tx.trust();
 		let keys = trusted_tx.keys();
 
@@ -1260,7 +1260,7 @@ impl TaprootChannelSigner for InMemorySigner {
 		todo!()
 	}
 
-	fn partially_sign_counterparty_commitment(&self, counterparty_nonce: PublicNonce, commitment_tx: &CommitmentTransaction, preimages: Vec<PaymentPreimage>, secp_ctx: &Secp256k1<All>) -> Result<(PartialSignatureWithNonce, Vec<schnorr::Signature>), ()> {
+	fn partially_sign_counterparty_commitment(&self, counterparty_nonce: PublicNonce, commitment_tx: &CommitmentTransaction, inbound_htlc_preimages: Vec<PaymentPreimage>, outbound_htlc_preimages: Vec<PaymentPreimage>, secp_ctx: &Secp256k1<All>) -> Result<(PartialSignatureWithNonce, Vec<schnorr::Signature>), ()> {
 		todo!()
 	}
 

lightning/src/sign/taproot.rs

@@ -27,17 +27,19 @@ pub trait TaprootChannelSigner: ChannelSigner {
 	/// Policy checks should be implemented in this function, including checking the amount
 	/// sent to us and checking the HTLCs.
 	///
-	/// The preimages of outgoing HTLCs that were fulfilled since the last commitment are provided.
-	/// A validating signer should ensure that an HTLC output is removed only when the matching
-	/// preimage is provided, or when the value to holder is restored.
+	/// The preimages of outbound and inbound HTLCs that were fulfilled since the last commitment
+	/// are provided. A validating signer should ensure that an outbound HTLC output is removed
+	/// only when the matching preimage is provided and after the corresponding inbound HTLC has
+	/// been removed for forwarded payments.
 	///
 	/// Note that all the relevant preimages will be provided, but there may also be additional
 	/// irrelevant or duplicate preimages.
 	//
 	// TODO: Document the things someone using this interface should enforce before signing.
 	fn partially_sign_counterparty_commitment(&self, counterparty_nonce: PublicNonce,
-		commitment_tx: &CommitmentTransaction, preimages: Vec<PaymentPreimage>,
-		secp_ctx: &Secp256k1<secp256k1::All>,
+		commitment_tx: &CommitmentTransaction,
+		inbound_htlc_preimages: Vec<PaymentPreimage>,
+		outbound_htlc_preimages: Vec<PaymentPreimage>, secp_ctx: &Secp256k1<secp256k1::All>,
 	) -> Result<(PartialSignatureWithNonce, Vec<Signature>), ()>;
 
 	/// Creates a signature for a holder's commitment transaction.

lightning/src/util/test_channel_signer.rs

@@ -138,7 +138,7 @@ impl ChannelSigner for TestChannelSigner {
 		self.inner.release_commitment_secret(idx)
 	}
 
-	fn validate_holder_commitment(&self, holder_tx: &HolderCommitmentTransaction, _preimages: Vec<PaymentPreimage>) -> Result<(), ()> {
+	fn validate_holder_commitment(&self, holder_tx: &HolderCommitmentTransaction, _outbound_htlc_preimages: Vec<PaymentPreimage>) -> Result<(), ()> {
 		let mut state = self.state.lock().unwrap();
 		let idx = holder_tx.commitment_number();
 		assert!(idx == state.last_holder_commitment || idx == state.last_holder_commitment - 1, "expecting to validate the current or next holder commitment - trying {}, current {}", idx, state.last_holder_commitment);
@@ -166,7 +166,7 @@ impl ChannelSigner for TestChannelSigner {
 }
 
 impl EcdsaChannelSigner for TestChannelSigner {
-	fn sign_counterparty_commitment(&self, commitment_tx: &CommitmentTransaction, preimages: Vec<PaymentPreimage>, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()> {
+	fn sign_counterparty_commitment(&self, commitment_tx: &CommitmentTransaction, inbound_htlc_preimages: Vec<PaymentPreimage>, outbound_htlc_preimages: Vec<PaymentPreimage>, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()> {
 		self.verify_counterparty_commitment_tx(commitment_tx, secp_ctx);
 
 		{
@@ -185,7 +185,7 @@ impl EcdsaChannelSigner for TestChannelSigner {
 			state.last_counterparty_commitment = cmp::min(last_commitment_number, actual_commitment_number)
 		}
 
-		Ok(self.inner.sign_counterparty_commitment(commitment_tx, preimages, secp_ctx).unwrap())
+		Ok(self.inner.sign_counterparty_commitment(commitment_tx, inbound_htlc_preimages, outbound_htlc_preimages, secp_ctx).unwrap())
 	}
 
 	fn sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()> {
@@ -288,7 +288,7 @@ impl TaprootChannelSigner for TestChannelSigner {
 		todo!()
 	}
 
-	fn partially_sign_counterparty_commitment(&self, counterparty_nonce: PublicNonce, commitment_tx: &CommitmentTransaction, preimages: Vec<PaymentPreimage>, secp_ctx: &Secp256k1<All>) -> Result<(PartialSignatureWithNonce, Vec<secp256k1::schnorr::Signature>), ()> {
+	fn partially_sign_counterparty_commitment(&self, counterparty_nonce: PublicNonce, commitment_tx: &CommitmentTransaction, inbound_htlc_preimages: Vec<PaymentPreimage>, outbound_htlc_preimages: Vec<PaymentPreimage>, secp_ctx: &Secp256k1<All>) -> Result<(PartialSignatureWithNonce, Vec<secp256k1::schnorr::Signature>), ()> {
 		todo!()
 	}