Merge pull request #2753 from TheBlueMatt/2023-11-inbound-preimages
authorWilmer Paulino <9447167+wpaulino@users.noreply.github.com>
Mon, 4 Dec 2023 21:15:10 +0000 (13:15 -0800)
committerGitHub <noreply@github.com>
Mon, 4 Dec 2023 21:15:10 +0000 (13:15 -0800)
Provide inbound HTLC preimages to the `EcdsaChannelSigner`

lightning/src/ln/channel.rs
lightning/src/ln/functional_tests.rs
lightning/src/sign/ecdsa.rs
lightning/src/sign/mod.rs
lightning/src/sign/taproot.rs
lightning/src/util/test_channel_signer.rs

index 7943a6ce4604b9275741ed80c1388c90ed0ca7b0..9455a76140b5b627a21bb0a948caefa85dc3c1c7 100644 (file)
@@ -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;
 
index a1d136427666a01cf51a2f2b68a6d233edea3d45..a41cf306880d73220742168f64a4258327b5043b 100644 (file)
@@ -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 {
index 194d714c977855e6058165ef4b1d4839f54ee2a2..2e98213c1825f2c0af8802bcc0838c2dc4d7af55 100644 (file)
@@ -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.
        ///
index 5a80cb2318adb22c3e297a9c27238c8041eaf72a..4e418f049bbc9867a7a2a7920706e17c135ed23e 100644 (file)
@@ -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!()
        }
 
index f2d3f62831374a5091d55a9825f46e494f66e459..230383f4f7d6496bcb1b7d40829b086ea6d39d05 100644 (file)
@@ -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.
index 51edce1768c7283aac89b865e09dc12e0a7395f6..a2cbf78b70053d2467cbe41fafe041eb30fa68e4 100644 (file)
@@ -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!()
        }