]> git.bitcoin.ninja Git - rust-lightning/commitdiff
Provide inbound HTLC preimages to the `EcdsaChannelSigner` 2023-11-inbound-preimages
authorMatt Corallo <git@bluematt.me>
Tue, 28 Nov 2023 23:19:39 +0000 (23:19 +0000)
committerMatt Corallo <git@bluematt.me>
Mon, 4 Dec 2023 19:36:49 +0000 (19:36 +0000)
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
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 8a50d47cd20607cc72131d52d5544eb1884f60e0..a9dbf5bbe51936f592055e2ba2070688a1f71827 100644 (file)
@@ -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<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.
@@ -1393,6 +1394,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"),
@@ -1410,7 +1413,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;
                                                        }
                                                }
@@ -1420,7 +1424,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 {
@@ -1439,7 +1444,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 {
@@ -1545,7 +1550,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,
                }
        }
 
@@ -2141,7 +2147,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)
@@ -2178,7 +2184,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,
@@ -3208,7 +3214,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...
@@ -5709,8 +5715,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 cc00d93b17abf7026b62288d1ffd6a49d1588d10..3475e89264d2eb6e0fc1b41fb7f0f97b3c815190 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 {
@@ -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 {
index c9677c8ba250245084e7ddf1dbed8b5be59b194b..52855050b3d9408b0c694467ef48bd0c95c1def8 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>), ()>;
        /// Validate the counterparty's revocation.
        ///
index d8f71f590534555988d5f06d4a9757b70baa4f8c..2e7e39cc9cb9ccc82db63fd1f6e1af991681b758 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<(), ()>;
 
        /// 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<PaymentPreimage>) -> Result<(), ()> {
+       fn validate_holder_commitment(&self, _holder_tx: &HolderCommitmentTransaction, _outbound_htlc_preimages: Vec<PaymentPreimage>) -> 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<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();
 
@@ -1254,7 +1254,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 0028a0cd12bb4e111aa04da29aa6d36788b3a404..ebfce345fcb7de22f8797e14428ac56d8eb9e1f7 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 ccdcfdd09346123cab89ecc56857c7f82a7337fc..3fcbb50d450dd134fad8a7d0aaaab738fd22b962 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);
@@ -156,7 +156,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);
 
                {
@@ -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<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!()
        }