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
This commit is contained in:
Matt Corallo 2023-11-28 23:19:39 +00:00
parent 2659a2375e
commit 262072d816
6 changed files with 44 additions and 30 deletions

View 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 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 local_balance_msat: u64, // local balance before fees but considering dust limits
remote_balance_msat: u64, // remote 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. /// 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() { for ref htlc in self.pending_inbound_htlcs.iter() {
let (include, state_name) = match htlc.state { let (include, state_name) = match htlc.state {
InboundHTLCState::RemoteAnnounced(_) => (!generated_by_local, "RemoteAnnounced"), InboundHTLCState::RemoteAnnounced(_) => (!generated_by_local, "RemoteAnnounced"),
@ -1410,7 +1413,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
match &htlc.state { match &htlc.state {
&InboundHTLCState::LocalRemoved(ref reason) => { &InboundHTLCState::LocalRemoved(ref reason) => {
if generated_by_local { 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; 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() { for ref htlc in self.pending_outbound_htlcs.iter() {
let (include, state_name) = match htlc.state { 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 { if let Some(preimage) = preimage_opt {
preimages.push(preimage); outbound_htlc_preimages.push(preimage);
} }
if include { if include {
@ -1545,7 +1550,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
htlcs_included, htlcs_included,
local_balance_msat: value_to_self_msat as u64, local_balance_msat: value_to_self_msat as u64,
remote_balance_msat: value_to_remote_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 { let signature = match &self.holder_signer {
// TODO (taproot|arik): move match into calling method for Taproot // TODO (taproot|arik): move match into calling method for Taproot
ChannelSignerType::Ecdsa(ecdsa) => { 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()? .map(|(sig, _)| sig).ok()?
}, },
// TODO (taproot|arik) // TODO (taproot|arik)
@ -2178,7 +2184,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
match &self.holder_signer { match &self.holder_signer {
// TODO (arik): move match into calling method for Taproot // TODO (arik): move match into calling method for Taproot
ChannelSignerType::Ecdsa(ecdsa) => { 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 { .map(|(signature, _)| msgs::FundingSigned {
channel_id: self.channel_id(), channel_id: self.channel_id(),
signature, signature,
@ -3208,7 +3214,7 @@ impl<SP: Deref> Channel<SP> where
self.context.counterparty_funding_pubkey() 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()))?; .map_err(|_| ChannelError::Close("Failed to validate our commitment".to_owned()))?;
// Update state now that we've passed all the can-fail calls... // 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); htlcs.push(htlc);
} }
let res = ecdsa.sign_counterparty_commitment(&commitment_stats.tx, commitment_stats.preimages, &self.context.secp_ctx) let res = ecdsa.sign_counterparty_commitment(
.map_err(|_| ChannelError::Ignore("Failed to get signatures for new commitment_signed".to_owned()))?; &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; signature = res.0;
htlc_signatures = res.1; htlc_signatures = res.1;

View file

@ -746,7 +746,7 @@ fn test_update_fee_that_funder_cannot_afford() {
&mut htlcs, &mut htlcs,
&local_chan.context.channel_transaction_parameters.as_counterparty_broadcastable() &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 { let commit_signed_msg = msgs::CommitmentSigned {
@ -1493,7 +1493,7 @@ fn test_fee_spike_violation_fails_htlc() {
&mut vec![(accepted_htlc_info, ())], &mut vec![(accepted_htlc_info, ())],
&local_chan.context.channel_transaction_parameters.as_counterparty_broadcastable() &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 { let commit_signed_msg = msgs::CommitmentSigned {

View file

@ -29,16 +29,18 @@ pub trait EcdsaChannelSigner: ChannelSigner {
/// Policy checks should be implemented in this function, including checking the amount /// Policy checks should be implemented in this function, including checking the amount
/// sent to us and checking the HTLCs. /// 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 and inbound HTLCs that were fulfilled since the last commitment
/// A validating signer should ensure that an HTLC output is removed only when the matching /// are provided. A validating signer should ensure that an outbound HTLC output is removed
/// preimage is provided, or when the value to holder is restored. /// 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 /// Note that all the relevant preimages will be provided, but there may also be additional
/// irrelevant or duplicate preimages. /// irrelevant or duplicate preimages.
// //
// TODO: Document the things someone using this interface should enforce before signing. // TODO: Document the things someone using this interface should enforce before signing.
fn sign_counterparty_commitment(&self, commitment_tx: &CommitmentTransaction, 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>), ()>; ) -> Result<(Signature, Vec<Signature>), ()>;
/// Validate the counterparty's revocation. /// Validate the counterparty's revocation.
/// ///

View file

@ -594,14 +594,14 @@ pub trait ChannelSigner {
/// Policy checks should be implemented in this function, including checking the amount /// Policy checks should be implemented in this function, including checking the amount
/// sent to us and checking the HTLCs. /// 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 /// 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. /// 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 /// Note that all the relevant preimages will be provided, but there may also be additional
/// irrelevant or duplicate preimages. /// irrelevant or duplicate preimages.
fn validate_holder_commitment(&self, holder_tx: &HolderCommitmentTransaction, 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. /// Returns the holder's channel public keys and basepoints.
fn pubkeys(&self) -> &ChannelPublicKeys; fn pubkeys(&self) -> &ChannelPublicKeys;
@ -1080,7 +1080,7 @@ impl ChannelSigner for InMemorySigner {
chan_utils::build_commitment_secret(&self.commitment_seed, idx) 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(()) 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"; const MISSING_PARAMS_ERR: &'static str = "ChannelSigner::provide_channel_parameters must be called before signing operations";
impl EcdsaChannelSigner for InMemorySigner { 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 trusted_tx = commitment_tx.trust();
let keys = trusted_tx.keys(); let keys = trusted_tx.keys();
@ -1254,7 +1254,7 @@ impl TaprootChannelSigner for InMemorySigner {
todo!() 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!() todo!()
} }

View file

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

View file

@ -138,7 +138,7 @@ impl ChannelSigner for TestChannelSigner {
self.inner.release_commitment_secret(idx) 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 mut state = self.state.lock().unwrap();
let idx = holder_tx.commitment_number(); 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); 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 { 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); 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) 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<(), ()> { fn validate_counterparty_revocation(&self, idx: u64, _secret: &SecretKey) -> Result<(), ()> {
@ -288,7 +288,7 @@ impl TaprootChannelSigner for TestChannelSigner {
todo!() 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!() todo!()
} }