From 262072d816a3927b42e80eaf6ce1bccccdb8afd7 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 28 Nov 2023 23:19:39 +0000 Subject: [PATCH] Provide inbound HTLC preimages to the `EcdsaChannelSigner` The VLS signer has a desire to see preimages for resolved forwarded HTLCs when they are first claimed by us, even if that claim was for the inbound edge (where claiming strictly increases our balance). Luckily, providing that information is rather trivial, which we do here. Fixes #2356 --- lightning/src/ln/channel.rs | 30 +++++++++++++++-------- lightning/src/ln/functional_tests.rs | 4 +-- lightning/src/sign/ecdsa.rs | 10 +++++--- lightning/src/sign/mod.rs | 10 ++++---- lightning/src/sign/taproot.rs | 12 +++++---- lightning/src/util/test_channel_signer.rs | 8 +++--- 6 files changed, 44 insertions(+), 30 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 8a50d47c..a9dbf5bb 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -479,7 +479,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, // preimages for successful offered HTLCs since last commitment + outbound_htlc_preimages: Vec, // preimages for successful offered HTLCs since last commitment + inbound_htlc_preimages: Vec, // preimages for successful received HTLCs since last commitment } /// Used when calculating whether we or the remote can afford an additional HTLC. @@ -1393,6 +1394,8 @@ impl ChannelContext where SP::Target: SignerProvider { } } + let mut inbound_htlc_preimages: Vec = Vec::new(); + for ref htlc in self.pending_inbound_htlcs.iter() { let (include, state_name) = match htlc.state { InboundHTLCState::RemoteAnnounced(_) => (!generated_by_local, "RemoteAnnounced"), @@ -1410,7 +1413,8 @@ impl ChannelContext 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; } } @@ -1420,7 +1424,8 @@ impl ChannelContext where SP::Target: SignerProvider { } } - let mut preimages: Vec = Vec::new(); + + let mut outbound_htlc_preimages: Vec = Vec::new(); for ref htlc in self.pending_outbound_htlcs.iter() { let (include, state_name) = match htlc.state { @@ -1439,7 +1444,7 @@ impl ChannelContext where SP::Target: SignerProvider { }; if let Some(preimage) = preimage_opt { - preimages.push(preimage); + outbound_htlc_preimages.push(preimage); } if include { @@ -1545,7 +1550,8 @@ impl ChannelContext 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, } } @@ -2141,7 +2147,7 @@ impl ChannelContext 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) @@ -2178,7 +2184,7 @@ impl ChannelContext 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, @@ -3208,7 +3214,7 @@ impl Channel 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... @@ -5709,8 +5715,12 @@ impl Channel 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; diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index cc00d93b..3475e892 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/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 { @@ -1493,7 +1493,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 { diff --git a/lightning/src/sign/ecdsa.rs b/lightning/src/sign/ecdsa.rs index c9677c8b..52855050 100644 --- a/lightning/src/sign/ecdsa.rs +++ b/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, secp_ctx: &Secp256k1 + inbound_htlc_preimages: Vec, + outbound_htlc_preimages: Vec, secp_ctx: &Secp256k1, ) -> Result<(Signature, Vec), ()>; /// Validate the counterparty's revocation. /// diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs index d8f71f59..2e7e39cc 100644 --- a/lightning/src/sign/mod.rs +++ b/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) -> Result<(), ()>; + outbound_htlc_preimages: Vec) -> Result<(), ()>; /// Returns the holder's channel public keys and basepoints. fn pubkeys(&self) -> &ChannelPublicKeys; @@ -1080,7 +1080,7 @@ impl ChannelSigner for InMemorySigner { chan_utils::build_commitment_secret(&self.commitment_seed, idx) } - fn validate_holder_commitment(&self, _holder_tx: &HolderCommitmentTransaction, _preimages: Vec) -> Result<(), ()> { + fn validate_holder_commitment(&self, _holder_tx: &HolderCommitmentTransaction, _outbound_htlc_preimages: Vec) -> Result<(), ()> { Ok(()) } @@ -1102,7 +1102,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, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { + fn sign_counterparty_commitment(&self, commitment_tx: &CommitmentTransaction, _inbound_htlc_preimages: Vec, _outbound_htlc_preimages: Vec, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { let trusted_tx = commitment_tx.trust(); let keys = trusted_tx.keys(); @@ -1254,7 +1254,7 @@ impl TaprootChannelSigner for InMemorySigner { todo!() } - fn partially_sign_counterparty_commitment(&self, counterparty_nonce: PublicNonce, commitment_tx: &CommitmentTransaction, preimages: Vec, secp_ctx: &Secp256k1) -> Result<(PartialSignatureWithNonce, Vec), ()> { + fn partially_sign_counterparty_commitment(&self, counterparty_nonce: PublicNonce, commitment_tx: &CommitmentTransaction, inbound_htlc_preimages: Vec, outbound_htlc_preimages: Vec, secp_ctx: &Secp256k1) -> Result<(PartialSignatureWithNonce, Vec), ()> { todo!() } diff --git a/lightning/src/sign/taproot.rs b/lightning/src/sign/taproot.rs index 0028a0cd..ebfce345 100644 --- a/lightning/src/sign/taproot.rs +++ b/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, - secp_ctx: &Secp256k1, + commitment_tx: &CommitmentTransaction, + inbound_htlc_preimages: Vec, + outbound_htlc_preimages: Vec, secp_ctx: &Secp256k1, ) -> Result<(PartialSignatureWithNonce, Vec), ()>; /// Creates a signature for a holder's commitment transaction. diff --git a/lightning/src/util/test_channel_signer.rs b/lightning/src/util/test_channel_signer.rs index ccdcfdd0..3fcbb50d 100644 --- a/lightning/src/util/test_channel_signer.rs +++ b/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) -> Result<(), ()> { + fn validate_holder_commitment(&self, holder_tx: &HolderCommitmentTransaction, _outbound_htlc_preimages: Vec) -> 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); @@ -156,7 +156,7 @@ impl ChannelSigner for TestChannelSigner { } impl EcdsaChannelSigner for TestChannelSigner { - fn sign_counterparty_commitment(&self, commitment_tx: &CommitmentTransaction, preimages: Vec, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { + fn sign_counterparty_commitment(&self, commitment_tx: &CommitmentTransaction, inbound_htlc_preimages: Vec, outbound_htlc_preimages: Vec, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { self.verify_counterparty_commitment_tx(commitment_tx, secp_ctx); { @@ -175,7 +175,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 validate_counterparty_revocation(&self, idx: u64, _secret: &SecretKey) -> Result<(), ()> { @@ -288,7 +288,7 @@ impl TaprootChannelSigner for TestChannelSigner { todo!() } - fn partially_sign_counterparty_commitment(&self, counterparty_nonce: PublicNonce, commitment_tx: &CommitmentTransaction, preimages: Vec, secp_ctx: &Secp256k1) -> Result<(PartialSignatureWithNonce, Vec), ()> { + fn partially_sign_counterparty_commitment(&self, counterparty_nonce: PublicNonce, commitment_tx: &CommitmentTransaction, inbound_htlc_preimages: Vec, outbound_htlc_preimages: Vec, secp_ctx: &Secp256k1) -> Result<(PartialSignatureWithNonce, Vec), ()> { todo!() } -- 2.30.2