Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Provide inbound HTLC preimages to the EcdsaChannelSigner #2753

Merged
merged 1 commit into from
Dec 4, 2023
Merged

Provide inbound HTLC preimages to the EcdsaChannelSigner #2753

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 20 additions & 10 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outbound vs outgoing, pick one 😛

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm, good point, not just that, but I realized outbound/inbound is confusing on its own, cause they're preimages which are for outbound HTLCs, ie the preimages themselves are incoming from the peer....stuck with *bound and included the _htlc everywhere.

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.
Expand Down Expand Up @@ -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"),
Expand All @@ -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;
}
}
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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,
}
}

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
TheBlueMatt marked this conversation as resolved.
Show resolved Hide resolved
.map_err(|_| ChannelError::Close("Failed to validate our commitment".to_owned()))?;

// Update state now that we've passed all the can-fail calls...
Expand Down Expand Up @@ -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;

Expand Down
4 changes: 2 additions & 2 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
10 changes: 6 additions & 4 deletions lightning/src/sign/ecdsa.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand Down
10 changes: 5 additions & 5 deletions lightning/src/sign/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(())
}

Expand All @@ -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();

Expand Down Expand Up @@ -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!()
}

Expand Down
12 changes: 7 additions & 5 deletions lightning/src/sign/taproot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
8 changes: 4 additions & 4 deletions lightning/src/util/test_channel_signer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);

{
Expand All @@ -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<(), ()> {
Expand Down Expand Up @@ -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!()
}

Expand Down