From e58184cf2ac214e41f5341d9f87aed673ea0a544 Mon Sep 17 00:00:00 2001
From: Leo Nash <hello@leonash.net>
Date: Wed, 26 Feb 2025 05:17:18 +0000
Subject: [PATCH 01/10] Remove redundant parameters from the
`CommitmentTransaction` constructor
The `DirectedChannelTransactionParameters` argument of the
`CommitmentTransaction` constructor already contains the broadcaster,
and the countersignatory public keys, which include the respective
funding keys.
It is therefore not necessary to ask for these funding keys in
additional arguments.
---
lightning/src/chain/channelmonitor.rs | 14 ++------
lightning/src/ln/chan_utils.rs | 41 +++++++++++------------
lightning/src/ln/channel.rs | 7 ----
lightning/src/ln/functional_tests.rs | 21 +++++-------
lightning/src/util/test_channel_signer.rs | 14 ++------
5 files changed, 32 insertions(+), 65 deletions(-)
diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index d6fb676d294..91fddee3eeb 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -3506,20 +3506,12 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
to_broadcaster_value: u64, to_countersignatory_value: u64, feerate_per_kw: u32,
mut nondust_htlcs: Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>
) -> CommitmentTransaction {
- let broadcaster_keys =
- &self.funding.channel_parameters.counterparty_parameters.as_ref().unwrap().pubkeys;
- let countersignatory_keys = &self.funding.channel_parameters.holder_pubkeys;
-
- let broadcaster_funding_key = broadcaster_keys.funding_pubkey;
- let countersignatory_funding_key = countersignatory_keys.funding_pubkey;
- let keys = TxCreationKeys::from_channel_static_keys(&their_per_commitment_point,
- &broadcaster_keys, &countersignatory_keys, &self.onchain_tx_handler.secp_ctx);
let channel_parameters = &self.funding.channel_parameters.as_counterparty_broadcastable();
+ let keys = TxCreationKeys::from_channel_static_keys(their_per_commitment_point,
+ channel_parameters.broadcaster_pubkeys(), channel_parameters.countersignatory_pubkeys(), &self.onchain_tx_handler.secp_ctx);
CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number,
- to_broadcaster_value, to_countersignatory_value, broadcaster_funding_key,
- countersignatory_funding_key, keys, feerate_per_kw, &mut nondust_htlcs,
- channel_parameters)
+ to_broadcaster_value, to_countersignatory_value, keys, feerate_per_kw, &mut nondust_htlcs, channel_parameters)
}
fn counterparty_commitment_txs_from_update(&self, update: &ChannelMonitorUpdate) -> Vec<CommitmentTransaction> {
diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs
index af5e7d7101b..c02c65bd81f 100644
--- a/lightning/src/ln/chan_utils.rs
+++ b/lightning/src/ln/chan_utils.rs
@@ -1208,7 +1208,7 @@ impl HolderCommitmentTransaction {
for _ in 0..htlcs.len() {
counterparty_htlc_sigs.push(dummy_sig);
}
- let inner = CommitmentTransaction::new_with_auxiliary_htlc_data(0, 0, 0, dummy_key.clone(), dummy_key.clone(), keys, 0, htlcs, &channel_parameters.as_counterparty_broadcastable());
+ let inner = CommitmentTransaction::new_with_auxiliary_htlc_data(0, 0, 0, keys, 0, htlcs, &channel_parameters.as_counterparty_broadcastable());
htlcs.sort_by_key(|htlc| htlc.0.transaction_output_index);
HolderCommitmentTransaction {
inner,
@@ -1518,12 +1518,12 @@ impl CommitmentTransaction {
/// Only include HTLCs that are above the dust limit for the channel.
///
/// This is not exported to bindings users due to the generic though we likely should expose a version without
- pub fn new_with_auxiliary_htlc_data<T>(commitment_number: u64, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, broadcaster_funding_key: PublicKey, countersignatory_funding_key: PublicKey, keys: TxCreationKeys, feerate_per_kw: u32, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters) -> CommitmentTransaction {
+ pub fn new_with_auxiliary_htlc_data<T>(commitment_number: u64, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, keys: TxCreationKeys, feerate_per_kw: u32, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters) -> CommitmentTransaction {
let to_broadcaster_value_sat = Amount::from_sat(to_broadcaster_value_sat);
let to_countersignatory_value_sat = Amount::from_sat(to_countersignatory_value_sat);
// Sort outputs and populate output indices while keeping track of the auxiliary data
- let (outputs, htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, htlcs_with_aux, channel_parameters, &broadcaster_funding_key, &countersignatory_funding_key).unwrap();
+ let (outputs, htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, htlcs_with_aux, channel_parameters).unwrap();
let (obscured_commitment_transaction_number, txins) = Self::internal_build_inputs(commitment_number, channel_parameters);
let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs);
@@ -1552,11 +1552,11 @@ impl CommitmentTransaction {
self
}
- fn internal_rebuild_transaction(&self, keys: &TxCreationKeys, channel_parameters: &DirectedChannelTransactionParameters, broadcaster_funding_key: &PublicKey, countersignatory_funding_key: &PublicKey) -> Result<BuiltCommitmentTransaction, ()> {
+ fn internal_rebuild_transaction(&self, keys: &TxCreationKeys, channel_parameters: &DirectedChannelTransactionParameters) -> Result<BuiltCommitmentTransaction, ()> {
let (obscured_commitment_transaction_number, txins) = Self::internal_build_inputs(self.commitment_number, channel_parameters);
let mut htlcs_with_aux = self.htlcs.iter().map(|h| (h.clone(), ())).collect();
- let (outputs, _) = Self::internal_build_outputs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, &mut htlcs_with_aux, channel_parameters, broadcaster_funding_key, countersignatory_funding_key)?;
+ let (outputs, _) = Self::internal_build_outputs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, &mut htlcs_with_aux, channel_parameters)?;
let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs);
let txid = transaction.compute_txid();
@@ -1580,17 +1580,20 @@ impl CommitmentTransaction {
// - initial sorting of outputs / HTLCs in the constructor, in which case T is auxiliary data the
// caller needs to have sorted together with the HTLCs so it can keep track of the output index
// - building of a bitcoin transaction during a verify() call, in which case T is just ()
- fn internal_build_outputs<T>(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters, broadcaster_funding_key: &PublicKey, countersignatory_funding_key: &PublicKey) -> Result<(Vec<TxOut>, Vec<HTLCOutputInCommitment>), ()> {
- let countersignatory_pubkeys = channel_parameters.countersignatory_pubkeys();
+ fn internal_build_outputs<T>(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters) -> Result<(Vec<TxOut>, Vec<HTLCOutputInCommitment>), ()> {
+ let countersignatory_payment_point = &channel_parameters.countersignatory_pubkeys().payment_point;
+ let countersignatory_funding_key = &channel_parameters.countersignatory_pubkeys().funding_pubkey;
+ let broadcaster_funding_key = &channel_parameters.broadcaster_pubkeys().funding_pubkey;
+ let channel_type = channel_parameters.channel_type_features();
let contest_delay = channel_parameters.contest_delay();
let mut txouts: Vec<(TxOut, Option<&mut HTLCOutputInCommitment>)> = Vec::new();
if to_countersignatory_value_sat > Amount::ZERO {
- let script = if channel_parameters.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
- get_to_countersigner_keyed_anchor_redeemscript(&countersignatory_pubkeys.payment_point).to_p2wsh()
+ let script = if channel_type.supports_anchors_zero_fee_htlc_tx() {
+ get_to_countersigner_keyed_anchor_redeemscript(countersignatory_payment_point).to_p2wsh()
} else {
- ScriptBuf::new_p2wpkh(&Hash160::hash(&countersignatory_pubkeys.payment_point.serialize()).into())
+ ScriptBuf::new_p2wpkh(&Hash160::hash(&countersignatory_payment_point.serialize()).into())
};
txouts.push((
TxOut {
@@ -1616,7 +1619,7 @@ impl CommitmentTransaction {
));
}
- if channel_parameters.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
+ if channel_type.supports_anchors_zero_fee_htlc_tx() {
if to_broadcaster_value_sat > Amount::ZERO || !htlcs_with_aux.is_empty() {
let anchor_script = get_keyed_anchor_redeemscript(broadcaster_funding_key);
txouts.push((
@@ -1642,7 +1645,7 @@ impl CommitmentTransaction {
let mut htlcs = Vec::with_capacity(htlcs_with_aux.len());
for (htlc, _) in htlcs_with_aux {
- let script = get_htlc_redeemscript(&htlc, &channel_parameters.channel_type_features(), &keys);
+ let script = get_htlc_redeemscript(htlc, channel_type, keys);
let txout = TxOut {
script_pubkey: script.to_p2wsh(),
value: htlc.to_bitcoin_amount(),
@@ -1753,14 +1756,14 @@ impl CommitmentTransaction {
///
/// An external validating signer must call this method before signing
/// or using the built transaction.
- pub fn verify<T: secp256k1::Signing + secp256k1::Verification>(&self, channel_parameters: &DirectedChannelTransactionParameters, broadcaster_keys: &ChannelPublicKeys, countersignatory_keys: &ChannelPublicKeys, secp_ctx: &Secp256k1<T>) -> Result<TrustedCommitmentTransaction, ()> {
+ pub fn verify<T: secp256k1::Signing + secp256k1::Verification>(&self, channel_parameters: &DirectedChannelTransactionParameters, secp_ctx: &Secp256k1<T>) -> Result<TrustedCommitmentTransaction, ()> {
// This is the only field of the key cache that we trust
- let per_commitment_point = self.keys.per_commitment_point;
- let keys = TxCreationKeys::from_channel_static_keys(&per_commitment_point, broadcaster_keys, countersignatory_keys, secp_ctx);
+ let per_commitment_point = &self.keys.per_commitment_point;
+ let keys = TxCreationKeys::from_channel_static_keys(per_commitment_point, channel_parameters.broadcaster_pubkeys(), channel_parameters.countersignatory_pubkeys(), secp_ctx);
if keys != self.keys {
return Err(());
}
- let tx = self.internal_rebuild_transaction(&keys, channel_parameters, &broadcaster_keys.funding_pubkey, &countersignatory_keys.funding_pubkey)?;
+ let tx = self.internal_rebuild_transaction(&keys, channel_parameters)?;
if self.built.transaction != tx.transaction || self.built.txid != tx.txid {
return Err(());
}
@@ -1983,8 +1986,6 @@ mod tests {
struct TestCommitmentTxBuilder {
commitment_number: u64,
- holder_funding_pubkey: PublicKey,
- counterparty_funding_pubkey: PublicKey,
keys: TxCreationKeys,
feerate_per_kw: u32,
htlcs_with_aux: Vec<(HTLCOutputInCommitment, ())>,
@@ -2023,8 +2024,6 @@ mod tests {
Self {
commitment_number: 0,
- holder_funding_pubkey: holder_pubkeys.funding_pubkey,
- counterparty_funding_pubkey: counterparty_pubkeys.funding_pubkey,
keys,
feerate_per_kw: 1,
htlcs_with_aux,
@@ -2036,8 +2035,6 @@ mod tests {
fn build(&mut self, to_broadcaster_sats: u64, to_countersignatory_sats: u64) -> CommitmentTransaction {
CommitmentTransaction::new_with_auxiliary_htlc_data(
self.commitment_number, to_broadcaster_sats, to_countersignatory_sats,
- self.holder_funding_pubkey.clone(),
- self.counterparty_funding_pubkey.clone(),
self.keys.clone(), self.feerate_per_kw,
&mut self.htlcs_with_aux, &self.channel_parameters.as_holder_broadcastable()
)
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 9268d07449d..eb70c2f8c98 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -3795,11 +3795,6 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
let mut value_to_a = if local { value_to_self } else { value_to_remote };
let mut value_to_b = if local { value_to_remote } else { value_to_self };
- let (funding_pubkey_a, funding_pubkey_b) = if local {
- (funding.get_holder_pubkeys().funding_pubkey, funding.get_counterparty_pubkeys().funding_pubkey)
- } else {
- (funding.get_counterparty_pubkeys().funding_pubkey, funding.get_holder_pubkeys().funding_pubkey)
- };
if value_to_a >= (broadcaster_dust_limit_satoshis as i64) {
log_trace!(logger, " ...including {} output with value {}", if local { "to_local" } else { "to_remote" }, value_to_a);
@@ -3821,8 +3816,6 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
let tx = CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number,
value_to_a as u64,
value_to_b as u64,
- funding_pubkey_a,
- funding_pubkey_b,
keys.clone(),
feerate_per_kw,
&mut included_non_dust_htlcs,
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index 3a365919a46..3bc791fdbff 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -734,16 +734,15 @@ pub fn test_update_fee_that_funder_cannot_afford() {
// Get the TestChannelSigner for each channel, which will be used to (1) get the keys
// needed to sign the new commitment tx and (2) sign the new commitment tx.
- let (local_revocation_basepoint, local_htlc_basepoint, local_funding) = {
+ let (local_revocation_basepoint, local_htlc_basepoint) = {
let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
let local_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap();
let chan_signer = local_chan.get_signer();
let pubkeys = chan_signer.as_ref().pubkeys(None, &secp_ctx);
- (pubkeys.revocation_basepoint, pubkeys.htlc_basepoint,
- pubkeys.funding_pubkey)
+ (pubkeys.revocation_basepoint, pubkeys.htlc_basepoint)
};
- let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point, remote_funding) = {
+ let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point) = {
let per_peer_state = nodes[1].node.per_peer_state.read().unwrap();
let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().lock().unwrap();
let remote_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap();
@@ -751,7 +750,7 @@ pub fn test_update_fee_that_funder_cannot_afford() {
let pubkeys = chan_signer.as_ref().pubkeys(None, &secp_ctx);
(pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint,
chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap(),
- pubkeys.funding_pubkey)
+ )
};
// Assemble the set of keys we can use for signatures for our commitment_signed message.
@@ -768,7 +767,6 @@ pub fn test_update_fee_that_funder_cannot_afford() {
INITIAL_COMMITMENT_NUMBER - 1,
push_sats,
channel_value - push_sats - commit_tx_fee_msat(non_buffer_feerate + 4, 0, &channel_type_features) / 1000,
- local_funding, remote_funding,
commit_tx_keys.clone(),
non_buffer_feerate + 4,
&mut htlcs,
@@ -1462,7 +1460,7 @@ pub fn test_fee_spike_violation_fails_htlc() {
// Get the TestChannelSigner for each channel, which will be used to (1) get the keys
// needed to sign the new commitment tx and (2) sign the new commitment tx.
- let (local_revocation_basepoint, local_htlc_basepoint, local_secret, next_local_point, local_funding) = {
+ let (local_revocation_basepoint, local_htlc_basepoint, local_secret, next_local_point) = {
let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
let local_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap();
@@ -1473,18 +1471,16 @@ pub fn test_fee_spike_violation_fails_htlc() {
let pubkeys = chan_signer.as_ref().pubkeys(None, &secp_ctx);
(pubkeys.revocation_basepoint, pubkeys.htlc_basepoint,
chan_signer.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER).unwrap(),
- chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 2, &secp_ctx).unwrap(),
- chan_signer.as_ref().pubkeys(None, &secp_ctx).funding_pubkey)
+ chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 2, &secp_ctx).unwrap())
};
- let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point, remote_funding) = {
+ let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point) = {
let per_peer_state = nodes[1].node.per_peer_state.read().unwrap();
let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().lock().unwrap();
let remote_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap();
let chan_signer = remote_chan.get_signer();
let pubkeys = chan_signer.as_ref().pubkeys(None, &secp_ctx);
(pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint,
- chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap(),
- chan_signer.as_ref().pubkeys(None, &secp_ctx).funding_pubkey)
+ chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap())
};
// Assemble the set of keys we can use for signatures for our commitment_signed message.
@@ -1514,7 +1510,6 @@ pub fn test_fee_spike_violation_fails_htlc() {
commitment_number,
95000,
local_chan_balance,
- local_funding, remote_funding,
commit_tx_keys.clone(),
feerate_per_kw,
&mut vec![(accepted_htlc_info, ())],
diff --git a/lightning/src/util/test_channel_signer.rs b/lightning/src/util/test_channel_signer.rs
index 018c027a831..ea6451157cc 100644
--- a/lightning/src/util/test_channel_signer.rs
+++ b/lightning/src/util/test_channel_signer.rs
@@ -549,12 +549,7 @@ impl TestChannelSigner {
commitment_tx: &'a CommitmentTransaction, secp_ctx: &Secp256k1<T>,
) -> TrustedCommitmentTransaction<'a> {
commitment_tx
- .verify(
- &channel_parameters.as_counterparty_broadcastable(),
- channel_parameters.counterparty_pubkeys().unwrap(),
- &channel_parameters.holder_pubkeys,
- secp_ctx,
- )
+ .verify(&channel_parameters.as_counterparty_broadcastable(), secp_ctx)
.expect("derived different per-tx keys or built transaction")
}
@@ -563,12 +558,7 @@ impl TestChannelSigner {
commitment_tx: &'a CommitmentTransaction, secp_ctx: &Secp256k1<T>,
) -> TrustedCommitmentTransaction<'a> {
commitment_tx
- .verify(
- &channel_parameters.as_holder_broadcastable(),
- &channel_parameters.holder_pubkeys,
- channel_parameters.counterparty_pubkeys().unwrap(),
- secp_ctx,
- )
+ .verify(&channel_parameters.as_holder_broadcastable(), secp_ctx)
.expect("derived different per-tx keys or built transaction")
}
}
From 0ac188fe0a013fdd9cef354acadc2cfa44bde11c Mon Sep 17 00:00:00 2001
From: Leo Nash <hello@leonash.net>
Date: Wed, 26 Feb 2025 23:07:18 +0000
Subject: [PATCH 02/10] Let `CommitmentTransaction` build the `TxCreationKeys`
it will cache
Instead of asking callers to generate the `TxCreationKeys` on every new
commitment before constructing a `CommitmentTransaction`, this commit
lets `CommitmentTransaction` derive the `TxCreationKeys` it will use to
build the raw commitment transaction.
This allows a tighter coupling between the per-commitment keys, and the
corresponding commitment transaction.
As new states are generated, callers now only have to derive new
commitment points; `CommitmentTransaction` takes care of deriving the
per-commitment keys.
This commit also serves to limit the objects in LDK that derive
per-commitment keys according to the LN Specification in preparation
for enabling custom derivations in the future.
---
lightning/src/chain/channelmonitor.rs | 9 ++---
lightning/src/ln/chan_utils.rs | 34 +++++++------------
lightning/src/ln/channel.rs | 5 +--
lightning/src/ln/functional_tests.rs | 49 +++++++--------------------
4 files changed, 30 insertions(+), 67 deletions(-)
diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index 91fddee3eeb..887df61f3af 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -38,7 +38,7 @@ use crate::types::features::ChannelTypeFeatures;
use crate::types::payment::{PaymentHash, PaymentPreimage};
use crate::ln::msgs::DecodeError;
use crate::ln::channel_keys::{DelayedPaymentKey, DelayedPaymentBasepoint, HtlcBasepoint, HtlcKey, RevocationKey, RevocationBasepoint};
-use crate::ln::chan_utils::{self,CommitmentTransaction, CounterpartyCommitmentSecrets, HTLCOutputInCommitment, HTLCClaim, ChannelTransactionParameters, HolderCommitmentTransaction, TxCreationKeys};
+use crate::ln::chan_utils::{self,CommitmentTransaction, CounterpartyCommitmentSecrets, HTLCOutputInCommitment, HTLCClaim, ChannelTransactionParameters, HolderCommitmentTransaction};
use crate::ln::channelmanager::{HTLCSource, SentHTLCId, PaymentClaimDetails};
use crate::chain;
use crate::chain::{BestBlock, WatchedOutput};
@@ -3507,11 +3507,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
mut nondust_htlcs: Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>
) -> CommitmentTransaction {
let channel_parameters = &self.funding.channel_parameters.as_counterparty_broadcastable();
- let keys = TxCreationKeys::from_channel_static_keys(their_per_commitment_point,
- channel_parameters.broadcaster_pubkeys(), channel_parameters.countersignatory_pubkeys(), &self.onchain_tx_handler.secp_ctx);
-
- CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number,
- to_broadcaster_value, to_countersignatory_value, keys, feerate_per_kw, &mut nondust_htlcs, channel_parameters)
+ CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number, their_per_commitment_point,
+ to_broadcaster_value, to_countersignatory_value, feerate_per_kw, &mut nondust_htlcs, channel_parameters, &self.onchain_tx_handler.secp_ctx)
}
fn counterparty_commitment_txs_from_update(&self, update: &ChannelMonitorUpdate) -> Vec<CommitmentTransaction> {
diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs
index c02c65bd81f..df6924a7057 100644
--- a/lightning/src/ln/chan_utils.rs
+++ b/lightning/src/ln/chan_utils.rs
@@ -1180,13 +1180,6 @@ impl HolderCommitmentTransaction {
let dummy_key = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
let dummy_sig = sign(&secp_ctx, &secp256k1::Message::from_digest([42; 32]), &SecretKey::from_slice(&[42; 32]).unwrap());
- let keys = TxCreationKeys {
- per_commitment_point: dummy_key.clone(),
- revocation_key: RevocationKey::from_basepoint(&secp_ctx, &RevocationBasepoint::from(dummy_key), &dummy_key),
- broadcaster_htlc_key: HtlcKey::from_basepoint(&secp_ctx, &HtlcBasepoint::from(dummy_key), &dummy_key),
- countersignatory_htlc_key: HtlcKey::from_basepoint(&secp_ctx, &HtlcBasepoint::from(dummy_key), &dummy_key),
- broadcaster_delayed_payment_key: DelayedPaymentKey::from_basepoint(&secp_ctx, &DelayedPaymentBasepoint::from(dummy_key), &dummy_key),
- };
let channel_pubkeys = ChannelPublicKeys {
funding_pubkey: dummy_key.clone(),
revocation_basepoint: RevocationBasepoint::from(dummy_key),
@@ -1208,7 +1201,7 @@ impl HolderCommitmentTransaction {
for _ in 0..htlcs.len() {
counterparty_htlc_sigs.push(dummy_sig);
}
- let inner = CommitmentTransaction::new_with_auxiliary_htlc_data(0, 0, 0, keys, 0, htlcs, &channel_parameters.as_counterparty_broadcastable());
+ let inner = CommitmentTransaction::new_with_auxiliary_htlc_data(0, &dummy_key, 0, 0, 0, htlcs, &channel_parameters.as_counterparty_broadcastable(), &secp_ctx);
htlcs.sort_by_key(|htlc| htlc.0.transaction_output_index);
HolderCommitmentTransaction {
inner,
@@ -1518,9 +1511,10 @@ impl CommitmentTransaction {
/// Only include HTLCs that are above the dust limit for the channel.
///
/// This is not exported to bindings users due to the generic though we likely should expose a version without
- pub fn new_with_auxiliary_htlc_data<T>(commitment_number: u64, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, keys: TxCreationKeys, feerate_per_kw: u32, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters) -> CommitmentTransaction {
+ pub fn new_with_auxiliary_htlc_data<T>(commitment_number: u64, per_commitment_point: &PublicKey, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, feerate_per_kw: u32, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters, secp_ctx: &Secp256k1<secp256k1::All>) -> CommitmentTransaction {
let to_broadcaster_value_sat = Amount::from_sat(to_broadcaster_value_sat);
let to_countersignatory_value_sat = Amount::from_sat(to_countersignatory_value_sat);
+ let keys = TxCreationKeys::from_channel_static_keys(per_commitment_point, channel_parameters.broadcaster_pubkeys(), channel_parameters.countersignatory_pubkeys(), secp_ctx);
// Sort outputs and populate output indices while keeping track of the auxiliary data
let (outputs, htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, htlcs_with_aux, channel_parameters).unwrap();
@@ -1970,8 +1964,8 @@ pub fn get_commitment_transaction_number_obscure_factor(
mod tests {
use super::{CounterpartyCommitmentSecrets, ChannelPublicKeys};
use crate::chain;
- use crate::ln::chan_utils::{get_htlc_redeemscript, get_to_countersigner_keyed_anchor_redeemscript, CommitmentTransaction, TxCreationKeys, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, HTLCOutputInCommitment};
- use bitcoin::secp256k1::{PublicKey, SecretKey, Secp256k1};
+ use crate::ln::chan_utils::{get_htlc_redeemscript, get_to_countersigner_keyed_anchor_redeemscript, CommitmentTransaction, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, HTLCOutputInCommitment};
+ use bitcoin::secp256k1::{self, PublicKey, SecretKey, Secp256k1};
use crate::util::test_utils;
use crate::sign::{ChannelSigner, SignerProvider};
use bitcoin::{Network, Txid, ScriptBuf, CompressedPublicKey};
@@ -1986,11 +1980,12 @@ mod tests {
struct TestCommitmentTxBuilder {
commitment_number: u64,
- keys: TxCreationKeys,
+ per_commitment_point: PublicKey,
feerate_per_kw: u32,
htlcs_with_aux: Vec<(HTLCOutputInCommitment, ())>,
channel_parameters: ChannelTransactionParameters,
counterparty_pubkeys: ChannelPublicKeys,
+ secp_ctx: Secp256k1::<secp256k1::All>,
}
impl TestCommitmentTxBuilder {
@@ -2015,28 +2010,23 @@ mod tests {
channel_type_features: ChannelTypeFeatures::only_static_remote_key(),
channel_value_satoshis: 3000,
};
- let directed_parameters = channel_parameters.as_holder_broadcastable();
- let keys = TxCreationKeys::from_channel_static_keys(
- &per_commitment_point, directed_parameters.broadcaster_pubkeys(),
- directed_parameters.countersignatory_pubkeys(), &secp_ctx,
- );
let htlcs_with_aux = Vec::new();
Self {
commitment_number: 0,
- keys,
+ per_commitment_point,
feerate_per_kw: 1,
htlcs_with_aux,
channel_parameters,
counterparty_pubkeys,
+ secp_ctx,
}
}
fn build(&mut self, to_broadcaster_sats: u64, to_countersignatory_sats: u64) -> CommitmentTransaction {
CommitmentTransaction::new_with_auxiliary_htlc_data(
- self.commitment_number, to_broadcaster_sats, to_countersignatory_sats,
- self.keys.clone(), self.feerate_per_kw,
- &mut self.htlcs_with_aux, &self.channel_parameters.as_holder_broadcastable()
+ self.commitment_number, &self.per_commitment_point, to_broadcaster_sats, to_countersignatory_sats, self.feerate_per_kw,
+ &mut self.htlcs_with_aux, &self.channel_parameters.as_holder_broadcastable(), &self.secp_ctx
)
}
}
@@ -2084,7 +2074,7 @@ mod tests {
builder.channel_parameters.channel_type_features = ChannelTypeFeatures::only_static_remote_key();
builder.htlcs_with_aux = vec![(received_htlc.clone(), ()), (offered_htlc.clone(), ())];
let tx = builder.build(3000, 0);
- let keys = &builder.keys.clone();
+ let keys = tx.trust().keys();
assert_eq!(tx.built.transaction.output.len(), 3);
assert_eq!(tx.built.transaction.output[0].script_pubkey, get_htlc_redeemscript(&received_htlc, &ChannelTypeFeatures::only_static_remote_key(), &keys).to_p2wsh());
assert_eq!(tx.built.transaction.output[1].script_pubkey, get_htlc_redeemscript(&offered_htlc, &ChannelTypeFeatures::only_static_remote_key(), &keys).to_p2wsh());
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index eb70c2f8c98..d15adea880d 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -3814,12 +3814,13 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
if local { funding.channel_transaction_parameters.as_holder_broadcastable() }
else { funding.channel_transaction_parameters.as_counterparty_broadcastable() };
let tx = CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number,
+ &keys.per_commitment_point,
value_to_a as u64,
value_to_b as u64,
- keys.clone(),
feerate_per_kw,
&mut included_non_dust_htlcs,
- &channel_parameters
+ &channel_parameters,
+ &self.secp_ctx,
);
let mut htlcs_included = included_non_dust_htlcs;
// The unwrap is safe, because all non-dust HTLCs have been assigned an output index
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index 3bc791fdbff..068199fc1c9 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -732,31 +732,14 @@ pub fn test_update_fee_that_funder_cannot_afford() {
const INITIAL_COMMITMENT_NUMBER: u64 = 281474976710654;
- // Get the TestChannelSigner for each channel, which will be used to (1) get the keys
- // needed to sign the new commitment tx and (2) sign the new commitment tx.
- let (local_revocation_basepoint, local_htlc_basepoint) = {
- let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
- let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
- let local_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap();
- let chan_signer = local_chan.get_signer();
- let pubkeys = chan_signer.as_ref().pubkeys(None, &secp_ctx);
- (pubkeys.revocation_basepoint, pubkeys.htlc_basepoint)
- };
- let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point) = {
+ let remote_point = {
let per_peer_state = nodes[1].node.per_peer_state.read().unwrap();
let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().lock().unwrap();
let remote_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap();
let chan_signer = remote_chan.get_signer();
- let pubkeys = chan_signer.as_ref().pubkeys(None, &secp_ctx);
- (pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint,
- chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap(),
- )
+ chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap()
};
- // Assemble the set of keys we can use for signatures for our commitment_signed message.
- let commit_tx_keys = chan_utils::TxCreationKeys::derive_new(&secp_ctx, &remote_point, &remote_delayed_payment_basepoint,
- &remote_htlc_basepoint, &local_revocation_basepoint, &local_htlc_basepoint);
-
let res = {
let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
let local_chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
@@ -765,12 +748,13 @@ pub fn test_update_fee_that_funder_cannot_afford() {
let mut htlcs: Vec<(HTLCOutputInCommitment, ())> = vec![];
let commitment_tx = CommitmentTransaction::new_with_auxiliary_htlc_data(
INITIAL_COMMITMENT_NUMBER - 1,
+ &remote_point,
push_sats,
channel_value - push_sats - commit_tx_fee_msat(non_buffer_feerate + 4, 0, &channel_type_features) / 1000,
- commit_tx_keys.clone(),
non_buffer_feerate + 4,
&mut htlcs,
- &local_chan.funding.channel_transaction_parameters.as_counterparty_broadcastable()
+ &local_chan.funding.channel_transaction_parameters.as_counterparty_broadcastable(),
+ &secp_ctx,
);
local_chan_signer.as_ecdsa().unwrap().sign_counterparty_commitment(
&local_chan.funding.channel_transaction_parameters, &commitment_tx, Vec::new(),
@@ -1458,9 +1442,7 @@ pub fn test_fee_spike_violation_fails_htlc() {
const INITIAL_COMMITMENT_NUMBER: u64 = (1 << 48) - 1;
- // Get the TestChannelSigner for each channel, which will be used to (1) get the keys
- // needed to sign the new commitment tx and (2) sign the new commitment tx.
- let (local_revocation_basepoint, local_htlc_basepoint, local_secret, next_local_point) = {
+ let (local_secret, next_local_point) = {
let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
let local_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap();
@@ -1468,25 +1450,17 @@ pub fn test_fee_spike_violation_fails_htlc() {
// Make the signer believe we validated another commitment, so we can release the secret
chan_signer.as_ecdsa().unwrap().get_enforcement_state().last_holder_commitment -= 1;
- let pubkeys = chan_signer.as_ref().pubkeys(None, &secp_ctx);
- (pubkeys.revocation_basepoint, pubkeys.htlc_basepoint,
- chan_signer.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER).unwrap(),
+ (chan_signer.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER).unwrap(),
chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 2, &secp_ctx).unwrap())
};
- let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point) = {
+ let remote_point = {
let per_peer_state = nodes[1].node.per_peer_state.read().unwrap();
let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().lock().unwrap();
let remote_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap();
let chan_signer = remote_chan.get_signer();
- let pubkeys = chan_signer.as_ref().pubkeys(None, &secp_ctx);
- (pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint,
- chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap())
+ chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap()
};
- // Assemble the set of keys we can use for signatures for our commitment_signed message.
- let commit_tx_keys = chan_utils::TxCreationKeys::derive_new(&secp_ctx, &remote_point, &remote_delayed_payment_basepoint,
- &remote_htlc_basepoint, &local_revocation_basepoint, &local_htlc_basepoint);
-
// Build the remote commitment transaction so we can sign it, and then later use the
// signature for the commitment_signed message.
let local_chan_balance = 1313;
@@ -1508,12 +1482,13 @@ pub fn test_fee_spike_violation_fails_htlc() {
let local_chan_signer = local_chan.get_signer();
let commitment_tx = CommitmentTransaction::new_with_auxiliary_htlc_data(
commitment_number,
+ &remote_point,
95000,
local_chan_balance,
- commit_tx_keys.clone(),
feerate_per_kw,
&mut vec![(accepted_htlc_info, ())],
- &local_chan.funding.channel_transaction_parameters.as_counterparty_broadcastable()
+ &local_chan.funding.channel_transaction_parameters.as_counterparty_broadcastable(),
+ &secp_ctx,
);
local_chan_signer.as_ecdsa().unwrap().sign_counterparty_commitment(
&local_chan.funding.channel_transaction_parameters, &commitment_tx, Vec::new(),
From 9b71c644d5566074392eb2d904e0ff293808f400 Mon Sep 17 00:00:00 2001
From: Leo Nash <hello@leonash.net>
Date: Wed, 5 Mar 2025 07:24:39 +0000
Subject: [PATCH 03/10] Remove some unneeded `Result`'s in
`CommitmentTransaction`
---
lightning/src/ln/chan_utils.rs | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs
index df6924a7057..88efc9e3e3c 100644
--- a/lightning/src/ln/chan_utils.rs
+++ b/lightning/src/ln/chan_utils.rs
@@ -1517,7 +1517,7 @@ impl CommitmentTransaction {
let keys = TxCreationKeys::from_channel_static_keys(per_commitment_point, channel_parameters.broadcaster_pubkeys(), channel_parameters.countersignatory_pubkeys(), secp_ctx);
// Sort outputs and populate output indices while keeping track of the auxiliary data
- let (outputs, htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, htlcs_with_aux, channel_parameters).unwrap();
+ let (outputs, htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, htlcs_with_aux, channel_parameters);
let (obscured_commitment_transaction_number, txins) = Self::internal_build_inputs(commitment_number, channel_parameters);
let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs);
@@ -1546,11 +1546,11 @@ impl CommitmentTransaction {
self
}
- fn internal_rebuild_transaction(&self, keys: &TxCreationKeys, channel_parameters: &DirectedChannelTransactionParameters) -> Result<BuiltCommitmentTransaction, ()> {
+ fn internal_rebuild_transaction(&self, keys: &TxCreationKeys, channel_parameters: &DirectedChannelTransactionParameters) -> BuiltCommitmentTransaction {
let (obscured_commitment_transaction_number, txins) = Self::internal_build_inputs(self.commitment_number, channel_parameters);
let mut htlcs_with_aux = self.htlcs.iter().map(|h| (h.clone(), ())).collect();
- let (outputs, _) = Self::internal_build_outputs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, &mut htlcs_with_aux, channel_parameters)?;
+ let (outputs, _) = Self::internal_build_outputs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, &mut htlcs_with_aux, channel_parameters);
let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs);
let txid = transaction.compute_txid();
@@ -1558,7 +1558,7 @@ impl CommitmentTransaction {
transaction,
txid
};
- Ok(built_transaction)
+ built_transaction
}
fn make_transaction(obscured_commitment_transaction_number: u64, txins: Vec<TxIn>, outputs: Vec<TxOut>) -> Transaction {
@@ -1574,7 +1574,7 @@ impl CommitmentTransaction {
// - initial sorting of outputs / HTLCs in the constructor, in which case T is auxiliary data the
// caller needs to have sorted together with the HTLCs so it can keep track of the output index
// - building of a bitcoin transaction during a verify() call, in which case T is just ()
- fn internal_build_outputs<T>(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters) -> Result<(Vec<TxOut>, Vec<HTLCOutputInCommitment>), ()> {
+ fn internal_build_outputs<T>(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters) -> (Vec<TxOut>, Vec<HTLCOutputInCommitment>) {
let countersignatory_payment_point = &channel_parameters.countersignatory_pubkeys().payment_point;
let countersignatory_funding_key = &channel_parameters.countersignatory_pubkeys().funding_pubkey;
let broadcaster_funding_key = &channel_parameters.broadcaster_pubkeys().funding_pubkey;
@@ -1671,7 +1671,7 @@ impl CommitmentTransaction {
}
outputs.push(out.0);
}
- Ok((outputs, htlcs))
+ (outputs, htlcs)
}
fn internal_build_inputs(commitment_number: u64, channel_parameters: &DirectedChannelTransactionParameters) -> (u64, Vec<TxIn>) {
@@ -1757,7 +1757,7 @@ impl CommitmentTransaction {
if keys != self.keys {
return Err(());
}
- let tx = self.internal_rebuild_transaction(&keys, channel_parameters)?;
+ let tx = self.internal_rebuild_transaction(&keys, channel_parameters);
if self.built.transaction != tx.transaction || self.built.txid != tx.txid {
return Err(());
}
From 1023d0bcf5aa724ea37e0f4d7f1fa0ba357a77b3 Mon Sep 17 00:00:00 2001
From: Leo Nash <hello@leonash.net>
Date: Wed, 26 Feb 2025 06:53:13 +0000
Subject: [PATCH 04/10] Remove explicit per-commitment key derivations from
channel objects
Channel objects should not impose a particular per-commitment derivation
scheme on the builders of commitment transactions. This will make it
easier to enable custom derivations in the future.
Instead of building `TxCreationKeys` explicitly, the function
`TrustedCommitmentTransaction::keys` should be used when the keys of the
commitment transaction are needed. We do that in this commit.
---
lightning/src/ln/channel.rs | 80 ++++++++++---------------------------
1 file changed, 21 insertions(+), 59 deletions(-)
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index d15adea880d..531784fb956 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -40,7 +40,7 @@ use crate::ln::script::{self, ShutdownScript};
use crate::ln::channel_state::{ChannelShutdownState, CounterpartyForwardingInfo, InboundHTLCDetails, InboundHTLCStateDetails, OutboundHTLCDetails, OutboundHTLCStateDetails};
use crate::ln::channelmanager::{self, OpenChannelMessage, PendingHTLCStatus, HTLCSource, SentHTLCId, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, PaymentClaimDetails, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, MAX_LOCAL_BREAKDOWN_TIMEOUT};
use crate::ln::chan_utils::{
- CounterpartyCommitmentSecrets, TxCreationKeys, HTLCOutputInCommitment, htlc_success_tx_weight,
+ CounterpartyCommitmentSecrets, HTLCOutputInCommitment, htlc_success_tx_weight,
htlc_timeout_tx_weight, ChannelPublicKeys, CommitmentTransaction,
HolderCommitmentTransaction, ChannelTransactionParameters,
CounterpartyChannelTransactionParameters, max_htlcs,
@@ -2045,8 +2045,7 @@ trait InitialRemoteCommitmentReceiver<SP: Deref> where SP::Target: SignerProvide
) -> Result<CommitmentTransaction, ChannelError> where L::Target: Logger {
let funding_script = self.funding().get_funding_redeemscript();
- let keys = self.context().build_holder_transaction_keys(&self.funding(), holder_commitment_point.current_point());
- let initial_commitment_tx = self.context().build_commitment_transaction(self.funding(), holder_commitment_point.transaction_number(), &keys, true, false, logger).tx;
+ let initial_commitment_tx = self.context().build_commitment_transaction(self.funding(), holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(), true, false, logger).tx;
let trusted_tx = initial_commitment_tx.trust();
let initial_commitment_bitcoin_tx = trusted_tx.built_transaction();
let sighash = initial_commitment_bitcoin_tx.get_sighash_all(&funding_script, self.funding().get_value_satoshis());
@@ -2083,8 +2082,7 @@ trait InitialRemoteCommitmentReceiver<SP: Deref> where SP::Target: SignerProvide
}
};
let context = self.context();
- let counterparty_keys = context.build_remote_transaction_keys(self.funding());
- let counterparty_initial_commitment_tx = context.build_commitment_transaction(self.funding(), context.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).tx;
+ let counterparty_initial_commitment_tx = context.build_commitment_transaction(self.funding(), context.cur_counterparty_commitment_transaction_number, &context.counterparty_cur_commitment_point.unwrap(), false, false, logger).tx;
let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust();
let counterparty_initial_bitcoin_tx = counterparty_trusted_tx.built_transaction();
@@ -3481,9 +3479,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
{
let funding_script = funding.get_funding_redeemscript();
- let keys = self.build_holder_transaction_keys(funding, holder_commitment_point.current_point());
-
- let commitment_stats = self.build_commitment_transaction(funding, holder_commitment_point.transaction_number(), &keys, true, false, logger);
+ let commitment_stats = self.build_commitment_transaction(funding, holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(), true, false, logger);
let commitment_txid = {
let trusted_tx = commitment_stats.tx.trust();
let bitcoin_tx = trusted_tx.built_transaction();
@@ -3551,19 +3547,20 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
let mut nondust_htlc_sources = Vec::with_capacity(htlcs_cloned.len());
let mut htlcs_and_sigs = Vec::with_capacity(htlcs_cloned.len());
+ let holder_keys = commitment_stats.tx.trust().keys();
for (idx, (htlc, mut source_opt)) in htlcs_cloned.drain(..).enumerate() {
if let Some(_) = htlc.transaction_output_index {
let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_stats.feerate_per_kw,
funding.get_counterparty_selected_contest_delay().unwrap(), &htlc, &self.channel_type,
- &keys.broadcaster_delayed_payment_key, &keys.revocation_key);
+ &holder_keys.broadcaster_delayed_payment_key, &holder_keys.revocation_key);
- let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &self.channel_type, &keys);
+ let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &self.channel_type, &holder_keys);
let htlc_sighashtype = if self.channel_type.supports_anchors_zero_fee_htlc_tx() { EcdsaSighashType::SinglePlusAnyoneCanPay } else { EcdsaSighashType::All };
let htlc_sighash = hash_to_message!(&sighash::SighashCache::new(&htlc_tx).p2wsh_signature_hash(0, &htlc_redeemscript, htlc.to_bitcoin_amount(), htlc_sighashtype).unwrap()[..]);
log_trace!(logger, "Checking HTLC tx signature {} by key {} against tx {} (sighash {}) with redeemscript {} in channel {}.",
- log_bytes!(msg.htlc_signatures[idx].serialize_compact()[..]), log_bytes!(keys.countersignatory_htlc_key.to_public_key().serialize()),
+ log_bytes!(msg.htlc_signatures[idx].serialize_compact()[..]), log_bytes!(holder_keys.countersignatory_htlc_key.to_public_key().serialize()),
encode::serialize_hex(&htlc_tx), log_bytes!(htlc_sighash[..]), encode::serialize_hex(&htlc_redeemscript), &self.channel_id());
- if let Err(_) = self.secp_ctx.verify_ecdsa(&htlc_sighash, &msg.htlc_signatures[idx], &keys.countersignatory_htlc_key.to_public_key()) {
+ if let Err(_) = self.secp_ctx.verify_ecdsa(&htlc_sighash, &msg.htlc_signatures[idx], &holder_keys.countersignatory_htlc_key.to_public_key()) {
return Err(ChannelError::close("Invalid HTLC tx signature from peer".to_owned()));
}
if !separate_nondust_htlc_sources {
@@ -3612,7 +3609,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
/// generated by the peer which proposed adding the HTLCs, and thus we need to understand both
/// which peer generated this transaction and "to whom" this transaction flows.
#[inline]
- fn build_commitment_transaction<L: Deref>(&self, funding: &FundingScope, commitment_number: u64, keys: &TxCreationKeys, local: bool, generated_by_local: bool, logger: &L) -> CommitmentStats
+ fn build_commitment_transaction<L: Deref>(&self, funding: &FundingScope, commitment_number: u64, per_commitment_point: &PublicKey, local: bool, generated_by_local: bool, logger: &L) -> CommitmentStats
where L::Target: Logger
{
let mut included_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::new();
@@ -3814,7 +3811,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
if local { funding.channel_transaction_parameters.as_holder_broadcastable() }
else { funding.channel_transaction_parameters.as_counterparty_broadcastable() };
let tx = CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number,
- &keys.per_commitment_point,
+ per_commitment_point,
value_to_a as u64,
value_to_b as u64,
feerate_per_kw,
@@ -3840,32 +3837,6 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
}
}
- #[inline]
- /// Creates a set of keys for build_commitment_transaction to generate a transaction which our
- /// counterparty will sign (ie DO NOT send signatures over a transaction created by this to
- /// our counterparty!)
- /// The result is a transaction which we can revoke broadcastership of (ie a "local" transaction)
- /// TODO Some magic rust shit to compile-time check this?
- fn build_holder_transaction_keys(&self, funding: &FundingScope, per_commitment_point: PublicKey) -> TxCreationKeys {
- let delayed_payment_base = &funding.get_holder_pubkeys().delayed_payment_basepoint;
- let htlc_basepoint = &funding.get_holder_pubkeys().htlc_basepoint;
- let counterparty_pubkeys = funding.get_counterparty_pubkeys();
-
- TxCreationKeys::derive_new(&self.secp_ctx, &per_commitment_point, delayed_payment_base, htlc_basepoint, &counterparty_pubkeys.revocation_basepoint, &counterparty_pubkeys.htlc_basepoint)
- }
-
- #[inline]
- /// Creates a set of keys for build_commitment_transaction to generate a transaction which we
- /// will sign and send to our counterparty.
- /// If an Err is returned, it is a ChannelError::Close (for get_funding_created)
- fn build_remote_transaction_keys(&self, funding: &FundingScope) -> TxCreationKeys {
- let revocation_basepoint = &funding.get_holder_pubkeys().revocation_basepoint;
- let htlc_basepoint = &funding.get_holder_pubkeys().htlc_basepoint;
- let counterparty_pubkeys = funding.get_counterparty_pubkeys();
-
- TxCreationKeys::derive_new(&self.secp_ctx, &self.counterparty_cur_commitment_point.unwrap(), &counterparty_pubkeys.delayed_payment_basepoint, &counterparty_pubkeys.htlc_basepoint, revocation_basepoint, htlc_basepoint)
- }
-
pub fn get_feerate_sat_per_1000_weight(&self) -> u32 {
self.feerate_per_kw
}
@@ -4648,9 +4619,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
SP::Target: SignerProvider,
L::Target: Logger
{
- let counterparty_keys = self.build_remote_transaction_keys(funding);
let counterparty_initial_commitment_tx = self.build_commitment_transaction(
- funding, self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).tx;
+ funding, self.cur_counterparty_commitment_transaction_number, &self.counterparty_cur_commitment_point.unwrap(), false, false, logger).tx;
match self.holder_signer {
// TODO (taproot|arik): move match into calling method for Taproot
ChannelSignerType::Ecdsa(ref ecdsa) => {
@@ -6363,8 +6333,7 @@ impl<SP: Deref> FundedChannel<SP> where
// Before proposing a feerate update, check that we can actually afford the new fee.
let dust_exposure_limiting_feerate = self.context.get_dust_exposure_limiting_feerate(&fee_estimator);
let htlc_stats = self.context.get_pending_htlc_stats(Some(feerate_per_kw), dust_exposure_limiting_feerate);
- let keys = self.context.build_holder_transaction_keys(&self.funding, self.holder_commitment_point.current_point());
- let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.holder_commitment_point.transaction_number(), &keys, true, true, logger);
+ let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.holder_commitment_point.transaction_number(), &self.holder_commitment_point.current_point(), true, true, logger);
let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_stats.num_nondust_htlcs + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.context.get_channel_type()) * 1000;
let holder_balance_msat = commitment_stats.local_balance_msat - htlc_stats.outbound_holding_cell_msat;
if holder_balance_msat < buffer_fee_msat + self.funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 {
@@ -6677,8 +6646,7 @@ impl<SP: Deref> FundedChannel<SP> where
self.holder_commitment_point.try_resolve_pending(&self.context.holder_signer, &self.context.secp_ctx, logger);
}
let funding_signed = if self.context.signer_pending_funding && !self.funding.is_outbound() {
- let counterparty_keys = self.context.build_remote_transaction_keys(&self.funding);
- let counterparty_initial_commitment_tx = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number + 1, &counterparty_keys, false, false, logger).tx;
+ let counterparty_initial_commitment_tx = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number + 1, &self.context.counterparty_cur_commitment_point.unwrap(), false, false, logger).tx;
self.context.get_funding_signed_msg(&self.funding.channel_transaction_parameters, logger, counterparty_initial_commitment_tx)
} else { None };
// Provide a `channel_ready` message if we need to, but only if we're _not_ still pending
@@ -8751,8 +8719,7 @@ impl<SP: Deref> FundedChannel<SP> where
-> (Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>, CommitmentTransaction)
where L::Target: Logger
{
- let counterparty_keys = self.context.build_remote_transaction_keys(&self.funding);
- let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, true, logger);
+ let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger);
let counterparty_commitment_tx = commitment_stats.tx;
#[cfg(any(test, fuzzing))]
@@ -8783,8 +8750,7 @@ impl<SP: Deref> FundedChannel<SP> where
#[cfg(any(test, fuzzing))]
self.build_commitment_no_state_update(logger);
- let counterparty_keys = self.context.build_remote_transaction_keys(&self.funding);
- let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, true, logger);
+ let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger);
let counterparty_commitment_txid = commitment_stats.tx.trust().txid();
match &self.context.holder_signer {
@@ -8812,6 +8778,7 @@ impl<SP: Deref> FundedChannel<SP> where
&counterparty_commitment_txid, encode::serialize_hex(&self.funding.get_funding_redeemscript()),
log_bytes!(signature.serialize_compact()[..]), &self.context.channel_id());
+ let counterparty_keys = commitment_stats.tx.trust().keys();
for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(htlcs) {
log_trace!(logger, "Signed remote HTLC tx {} with redeemscript {} with pubkey {} -> {} in channel {}",
encode::serialize_hex(&chan_utils::build_htlc_transaction(&counterparty_commitment_txid, commitment_stats.feerate_per_kw, self.funding.get_holder_selected_contest_delay(), htlc, &self.context.channel_type, &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)),
@@ -9268,8 +9235,7 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
/// Only allowed after [`FundingScope::channel_transaction_parameters`] is set.
fn get_funding_created_msg<L: Deref>(&mut self, logger: &L) -> Option<msgs::FundingCreated> where L::Target: Logger {
- let counterparty_keys = self.context.build_remote_transaction_keys(&self.funding);
- let counterparty_initial_commitment_tx = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).tx;
+ let counterparty_initial_commitment_tx = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &self.context.counterparty_cur_commitment_point.unwrap(), false, false, logger).tx;
let signature = match &self.context.holder_signer {
// TODO (taproot|arik): move match into calling method for Taproot
ChannelSignerType::Ecdsa(ecdsa) => {
@@ -11833,7 +11799,7 @@ mod tests {
use bitcoin::secp256k1::Message;
use crate::sign::{ChannelDerivationParameters, HTLCDescriptor, ecdsa::EcdsaChannelSigner};
use crate::types::payment::PaymentPreimage;
- use crate::ln::channel::{HTLCOutputInCommitment ,TxCreationKeys};
+ use crate::ln::channel::HTLCOutputInCommitment;
use crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint};
use crate::ln::chan_utils::{ChannelPublicKeys, HolderCommitmentTransaction, CounterpartyChannelTransactionParameters};
use crate::util::logger::Logger;
@@ -11901,11 +11867,6 @@ mod tests {
// build_commitment_transaction.
let per_commitment_secret = SecretKey::from_slice(&<Vec<u8>>::from_hex("1f1e1d1c1b1a191817161514131211100f0e0d0c0b0a09080706050403020100").unwrap()[..]).unwrap();
let per_commitment_point = PublicKey::from_secret_key(&secp_ctx, &per_commitment_secret);
- let directed_params = chan.funding.channel_transaction_parameters.as_holder_broadcastable();
- let keys = TxCreationKeys::from_channel_static_keys(
- &per_commitment_point, directed_params.broadcaster_pubkeys(),
- directed_params.countersignatory_pubkeys(), &secp_ctx,
- );
macro_rules! test_commitment {
( $counterparty_sig_hex: expr, $sig_hex: expr, $tx_hex: expr, $($remain:tt)* ) => {
@@ -11926,7 +11887,7 @@ mod tests {
$( { $htlc_idx: expr, $counterparty_htlc_sig_hex: expr, $htlc_sig_hex: expr, $htlc_tx_hex: expr } ), *
} ) => { {
let (commitment_tx, htlcs): (_, Vec<HTLCOutputInCommitment>) = {
- let mut commitment_stats = chan.context.build_commitment_transaction(&chan.funding, 0xffffffffffff - 42, &keys, true, false, &logger);
+ let mut commitment_stats = chan.context.build_commitment_transaction(&chan.funding, 0xffffffffffff - 42, &per_commitment_point, true, false, &logger);
let htlcs = commitment_stats.htlcs_included.drain(..)
.filter_map(|(htlc, _)| if htlc.transaction_output_index.is_some() { Some(htlc) } else { None })
@@ -11974,6 +11935,7 @@ mod tests {
let remote_signature = Signature::from_der(&<Vec<u8>>::from_hex($counterparty_htlc_sig_hex).unwrap()[..]).unwrap();
let ref htlc = htlcs[$htlc_idx];
+ let keys = commitment_tx.trust().keys();
let mut htlc_tx = chan_utils::build_htlc_transaction(&unsigned_tx.txid, chan.context.feerate_per_kw,
chan.funding.get_counterparty_selected_contest_delay().unwrap(),
&htlc, $opt_anchors, &keys.broadcaster_delayed_payment_key, &keys.revocation_key);
From 592f17d3974ec9600a0ee1ad657aa14304046c60 Mon Sep 17 00:00:00 2001
From: Leo Nash <hello@leonash.net>
Date: Sun, 2 Mar 2025 01:40:18 +0000
Subject: [PATCH 05/10] Remove unnecessary allocation in
`send_commitment_no_state_update`
The logging for loop in `send_commitment_no_state_update` only iterates
over the non-dust HTLCs. Thus we do not need to create a
`Vec<HTLCOutputInCommitment>` that includes both dust and non-dust
HTLCs, we can grab the `Vec<HTLCOutputInCommitment>` that holds only
non-dust HTLCs directly from `CommitmentTransaction`.
---
lightning/src/ln/channel.rs | 32 +++++++++++---------------------
1 file changed, 11 insertions(+), 21 deletions(-)
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 531784fb956..262674d82f5 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -6831,7 +6831,7 @@ impl<SP: Deref> FundedChannel<SP> where
log_trace!(logger, "Regenerating latest commitment update in channel {} with{} {} update_adds, {} update_fulfills, {} update_fails, and {} update_fail_malformeds",
&self.context.channel_id(), if update_fee.is_some() { " update_fee," } else { "" },
update_add_htlcs.len(), update_fulfill_htlcs.len(), update_fail_htlcs.len(), update_fail_malformed_htlcs.len());
- let commitment_signed = if let Ok(update) = self.send_commitment_no_state_update(logger).map(|(cu, _)| cu) {
+ let commitment_signed = if let Ok(update) = self.send_commitment_no_state_update(logger) {
if self.context.signer_pending_commitment_update {
log_trace!(logger, "Commitment update generated: clearing signer_pending_commitment_update");
self.context.signer_pending_commitment_update = false;
@@ -8745,7 +8745,7 @@ impl<SP: Deref> FundedChannel<SP> where
/// Only fails in case of signer rejection. Used for channel_reestablish commitment_signed
/// generation when we shouldn't change HTLC/channel state.
- fn send_commitment_no_state_update<L: Deref>(&self, logger: &L) -> Result<(msgs::CommitmentSigned, (Txid, Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>)), ChannelError> where L::Target: Logger {
+ fn send_commitment_no_state_update<L: Deref>(&self, logger: &L) -> Result<msgs::CommitmentSigned, ChannelError> where L::Target: Logger {
// Get the fee tests from `build_commitment_no_state_update`
#[cfg(any(test, fuzzing))]
self.build_commitment_no_state_update(logger);
@@ -8758,11 +8758,6 @@ impl<SP: Deref> FundedChannel<SP> where
let (signature, htlc_signatures);
{
- let mut htlcs = Vec::with_capacity(commitment_stats.htlcs_included.len());
- for &(ref htlc, _) in commitment_stats.htlcs_included.iter() {
- htlcs.push(htlc);
- }
-
let res = ecdsa.sign_counterparty_commitment(
&self.funding.channel_transaction_parameters,
&commitment_stats.tx,
@@ -8779,7 +8774,8 @@ impl<SP: Deref> FundedChannel<SP> where
log_bytes!(signature.serialize_compact()[..]), &self.context.channel_id());
let counterparty_keys = commitment_stats.tx.trust().keys();
- for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(htlcs) {
+ debug_assert_eq!(htlc_signatures.len(), commitment_stats.tx.htlcs().len());
+ for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(commitment_stats.tx.htlcs()) {
log_trace!(logger, "Signed remote HTLC tx {} with redeemscript {} with pubkey {} -> {} in channel {}",
encode::serialize_hex(&chan_utils::build_htlc_transaction(&counterparty_commitment_txid, commitment_stats.feerate_per_kw, self.funding.get_holder_selected_contest_delay(), htlc, &self.context.channel_type, &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)),
encode::serialize_hex(&chan_utils::get_htlc_redeemscript(&htlc, &self.context.channel_type, &counterparty_keys)),
@@ -8788,14 +8784,14 @@ impl<SP: Deref> FundedChannel<SP> where
}
}
- Ok((msgs::CommitmentSigned {
+ Ok(msgs::CommitmentSigned {
channel_id: self.context.channel_id,
signature,
htlc_signatures,
batch: None,
#[cfg(taproot)]
partial_signature_with_nonce: None,
- }, (counterparty_commitment_txid, commitment_stats.htlcs_included)))
+ })
},
// TODO (taproot|arik)
#[cfg(taproot)]
@@ -11886,14 +11882,8 @@ mod tests {
( $counterparty_sig_hex: expr, $sig_hex: expr, $tx_hex: expr, $opt_anchors: expr, {
$( { $htlc_idx: expr, $counterparty_htlc_sig_hex: expr, $htlc_sig_hex: expr, $htlc_tx_hex: expr } ), *
} ) => { {
- let (commitment_tx, htlcs): (_, Vec<HTLCOutputInCommitment>) = {
- let mut commitment_stats = chan.context.build_commitment_transaction(&chan.funding, 0xffffffffffff - 42, &per_commitment_point, true, false, &logger);
-
- let htlcs = commitment_stats.htlcs_included.drain(..)
- .filter_map(|(htlc, _)| if htlc.transaction_output_index.is_some() { Some(htlc) } else { None })
- .collect();
- (commitment_stats.tx, htlcs)
- };
+ let commitment_stats = chan.context.build_commitment_transaction(&chan.funding, 0xffffffffffff - 42, &per_commitment_point, true, false, &logger);
+ let commitment_tx = commitment_stats.tx;
let trusted_tx = commitment_tx.trust();
let unsigned_tx = trusted_tx.built_transaction();
let redeemscript = chan.funding.get_funding_redeemscript();
@@ -11908,10 +11898,10 @@ mod tests {
counterparty_htlc_sigs.clear(); // Don't warn about excess mut for no-HTLC calls
$({
let remote_signature = Signature::from_der(&<Vec<u8>>::from_hex($counterparty_htlc_sig_hex).unwrap()[..]).unwrap();
- per_htlc.push((htlcs[$htlc_idx].clone(), Some(remote_signature)));
+ per_htlc.push((commitment_tx.htlcs()[$htlc_idx].clone(), Some(remote_signature)));
counterparty_htlc_sigs.push(remote_signature);
})*
- assert_eq!(htlcs.len(), per_htlc.len());
+ assert_eq!(commitment_tx.htlcs().len(), per_htlc.len());
let holder_commitment_tx = HolderCommitmentTransaction::new(
commitment_tx.clone(),
@@ -11934,7 +11924,7 @@ mod tests {
log_trace!(logger, "verifying htlc {}", $htlc_idx);
let remote_signature = Signature::from_der(&<Vec<u8>>::from_hex($counterparty_htlc_sig_hex).unwrap()[..]).unwrap();
- let ref htlc = htlcs[$htlc_idx];
+ let ref htlc = commitment_tx.htlcs()[$htlc_idx];
let keys = commitment_tx.trust().keys();
let mut htlc_tx = chan_utils::build_htlc_transaction(&unsigned_tx.txid, chan.context.feerate_per_kw,
chan.funding.get_counterparty_selected_contest_delay().unwrap(),
From 728e7c28bd4c16b775742c067fa63df265877786 Mon Sep 17 00:00:00 2001
From: Leo Nash <hello@leonash.net>
Date: Wed, 26 Feb 2025 04:40:20 +0000
Subject: [PATCH 06/10] Remove `i64` casts in
`ChannelContext::build_commitment_transaction`
Instead of converting operands to `i64` and checking if the subtractions
overflowed by checking if the `i64` is smaller than zero, we instead
choose to do checked and saturating subtractions on the original
unsigned integers.
---
lightning/src/ln/channel.rs | 64 +++++++++++++++++++++----------------
1 file changed, 37 insertions(+), 27 deletions(-)
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 262674d82f5..f158e682a41 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -3758,14 +3758,17 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
}
}
- let value_to_self_msat: i64 = (funding.value_to_self_msat - local_htlc_total_msat) as i64 + value_to_self_msat_offset;
- assert!(value_to_self_msat >= 0);
- // Note that in case they have several just-awaiting-last-RAA fulfills in-progress (ie
- // AwaitingRemoteRevokeToRemove or AwaitingRemovedRemoteRevoke) we may have allowed them to
- // "violate" their reserve value by couting those against it. Thus, we have to convert
- // everything to i64 before subtracting as otherwise we can overflow.
- let value_to_remote_msat: i64 = (funding.get_value_satoshis() * 1000) as i64 - (funding.value_to_self_msat as i64) - (remote_htlc_total_msat as i64) - value_to_self_msat_offset;
- assert!(value_to_remote_msat >= 0);
+ // # Panics
+ //
+ // While we expect `value_to_self_msat_offset` to be negative in some cases, the value going
+ // to each party MUST be 0 or positive, even if all HTLCs pending in the commitment clear by
+ // failure.
+
+ // TODO: When MSRV >= 1.66.0, use u64::checked_add_signed
+ let mut value_to_self_msat = (funding.value_to_self_msat as i64 + value_to_self_msat_offset).try_into().unwrap();
+ let mut value_to_remote_msat = (funding.get_value_satoshis() * 1000).checked_sub(value_to_self_msat).unwrap();
+ value_to_self_msat = value_to_self_msat.checked_sub(local_htlc_total_msat).unwrap();
+ value_to_remote_msat = value_to_remote_msat.checked_sub(remote_htlc_total_msat).unwrap();
#[cfg(debug_assertions)]
{
@@ -3776,33 +3779,40 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
} else {
funding.counterparty_max_commitment_tx_output.lock().unwrap()
};
- debug_assert!(broadcaster_max_commitment_tx_output.0 <= value_to_self_msat as u64 || value_to_self_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap() as i64);
- broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, value_to_self_msat as u64);
- debug_assert!(broadcaster_max_commitment_tx_output.1 <= value_to_remote_msat as u64 || value_to_remote_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis as i64);
- broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, value_to_remote_msat as u64);
+ debug_assert!(broadcaster_max_commitment_tx_output.0 <= value_to_self_msat || value_to_self_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap());
+ broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, value_to_self_msat);
+ debug_assert!(broadcaster_max_commitment_tx_output.1 <= value_to_remote_msat || value_to_remote_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis);
+ broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, value_to_remote_msat);
}
+ // We MUST use saturating subs here, as the funder's balance is not guaranteed to be greater
+ // than or equal to the sum of `total_fee_sat` and `anchors_val`.
+ //
+ // This is because when the remote party sends an `update_fee` message, we build the new
+ // commitment transaction *before* checking whether the remote party's balance is enough to
+ // cover the total fee and the anchors.
+
let total_fee_sat = commit_tx_fee_sat(feerate_per_kw, included_non_dust_htlcs.len(), &funding.channel_transaction_parameters.channel_type_features);
- let anchors_val = if funding.channel_transaction_parameters.channel_type_features.supports_anchors_zero_fee_htlc_tx() { ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 } as i64;
+ let anchors_val = if funding.channel_transaction_parameters.channel_type_features.supports_anchors_zero_fee_htlc_tx() { ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 };
let (value_to_self, value_to_remote) = if funding.is_outbound() {
- (value_to_self_msat / 1000 - anchors_val - total_fee_sat as i64, value_to_remote_msat / 1000)
+ ((value_to_self_msat / 1000).saturating_sub(anchors_val).saturating_sub(total_fee_sat), value_to_remote_msat / 1000)
} else {
- (value_to_self_msat / 1000, value_to_remote_msat / 1000 - anchors_val - total_fee_sat as i64)
+ (value_to_self_msat / 1000, (value_to_remote_msat / 1000).saturating_sub(anchors_val).saturating_sub(total_fee_sat))
};
- let mut value_to_a = if local { value_to_self } else { value_to_remote };
- let mut value_to_b = if local { value_to_remote } else { value_to_self };
+ let mut to_broadcaster_value_sat = if local { value_to_self } else { value_to_remote };
+ let mut to_countersignatory_value_sat = if local { value_to_remote } else { value_to_self };
- if value_to_a >= (broadcaster_dust_limit_satoshis as i64) {
- log_trace!(logger, " ...including {} output with value {}", if local { "to_local" } else { "to_remote" }, value_to_a);
+ if to_broadcaster_value_sat >= broadcaster_dust_limit_satoshis {
+ log_trace!(logger, " ...including {} output with value {}", if local { "to_local" } else { "to_remote" }, to_broadcaster_value_sat);
} else {
- value_to_a = 0;
+ to_broadcaster_value_sat = 0;
}
- if value_to_b >= (broadcaster_dust_limit_satoshis as i64) {
- log_trace!(logger, " ...including {} output with value {}", if local { "to_remote" } else { "to_local" }, value_to_b);
+ if to_countersignatory_value_sat >= broadcaster_dust_limit_satoshis {
+ log_trace!(logger, " ...including {} output with value {}", if local { "to_remote" } else { "to_local" }, to_countersignatory_value_sat);
} else {
- value_to_b = 0;
+ to_countersignatory_value_sat = 0;
}
let num_nondust_htlcs = included_non_dust_htlcs.len();
@@ -3812,8 +3822,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
else { funding.channel_transaction_parameters.as_counterparty_broadcastable() };
let tx = CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number,
per_commitment_point,
- value_to_a as u64,
- value_to_b as u64,
+ to_broadcaster_value_sat,
+ to_countersignatory_value_sat,
feerate_per_kw,
&mut included_non_dust_htlcs,
&channel_parameters,
@@ -3830,8 +3840,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
total_fee_sat,
num_nondust_htlcs,
htlcs_included,
- local_balance_msat: value_to_self_msat as u64,
- remote_balance_msat: value_to_remote_msat as u64,
+ local_balance_msat: value_to_self_msat,
+ remote_balance_msat: value_to_remote_msat,
inbound_htlc_preimages,
outbound_htlc_preimages,
}
From 22be385c1b097fb9ca1944d52119bbf529acdfa8 Mon Sep 17 00:00:00 2001
From: Leo Nash <hello@leonash.net>
Date: Wed, 26 Feb 2025 04:56:22 +0000
Subject: [PATCH 07/10] Clean up tautologies in
`ChannelContext::build_commitment_transaction`
There is no need for an if statement if it will always be true.
---
lightning/src/ln/channel.rs | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index f158e682a41..2ff84ccb254 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -3703,13 +3703,9 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
} else {
log_trace!(logger, " ...not including inbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name);
match &htlc.state {
- &InboundHTLCState::LocalRemoved(ref reason) => {
- if generated_by_local {
- if let &InboundHTLCRemovalReason::Fulfill(preimage) = reason {
- inbound_htlc_preimages.push(preimage);
- value_to_self_msat_offset += htlc.amount_msat as i64;
- }
- }
+ &InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(preimage)) => {
+ inbound_htlc_preimages.push(preimage);
+ value_to_self_msat_offset += htlc.amount_msat as i64;
},
_ => {},
}
@@ -3745,13 +3741,10 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
} else {
log_trace!(logger, " ...not including outbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name);
match htlc.state {
- OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_))|OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_)) => {
- value_to_self_msat_offset -= htlc.amount_msat as i64;
- },
+ OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_)) |
+ OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_)) |
OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(_)) => {
- if !generated_by_local {
- value_to_self_msat_offset -= htlc.amount_msat as i64;
- }
+ value_to_self_msat_offset -= htlc.amount_msat as i64;
},
_ => {},
}
From 09422ddc62d2d5dd95ea9a69b22f52bfb3abce38 Mon Sep 17 00:00:00 2001
From: Leo Nash <hello@leonash.net>
Date: Wed, 26 Feb 2025 07:51:49 +0000
Subject: [PATCH 08/10] Remove redundant fields in `CommitmentStats`
The fields `feerate_per_kw` and `num_nondust_htlcs` of `CommitmentStats`
can both be accessed directly from the `tx: CommitmentTransaction`
field.
We also take this opportunity to rename the balance fields to make it
extra clear what they refer to.
---
lightning/src/ln/channel.rs | 30 ++++++++++++------------------
1 file changed, 12 insertions(+), 18 deletions(-)
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 2ff84ccb254..170af140803 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -880,12 +880,10 @@ struct HTLCStats {
/// An enum gathering stats on commitment transaction, either local or remote.
struct CommitmentStats<'a> {
tx: CommitmentTransaction, // the transaction info
- feerate_per_kw: u32, // the feerate included to build the transaction
total_fee_sat: u64, // the total fee included in the transaction
- num_nondust_htlcs: usize, // the number of HTLC outputs (dust HTLCs *non*-included)
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 *not* considering dust limits
- remote_balance_msat: u64, // remote balance before fees *not* considering dust limits
+ local_balance_before_fee_msat: u64, // local balance before fees *not* considering dust limits
+ remote_balance_before_fee_msat: u64, // remote balance before fees *not* considering dust limits
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
}
@@ -3504,7 +3502,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
if update_fee {
debug_assert!(!funding.is_outbound());
let counterparty_reserve_we_require_msat = funding.holder_selected_channel_reserve_satoshis * 1000;
- if commitment_stats.remote_balance_msat < commitment_stats.total_fee_sat * 1000 + counterparty_reserve_we_require_msat {
+ if commitment_stats.remote_balance_before_fee_msat < commitment_stats.total_fee_sat * 1000 + counterparty_reserve_we_require_msat {
return Err(ChannelError::close("Funding remote cannot afford proposed new fee".to_owned()));
}
}
@@ -3526,8 +3524,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
}
}
- if msg.htlc_signatures.len() != commitment_stats.num_nondust_htlcs {
- return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_stats.num_nondust_htlcs)));
+ if msg.htlc_signatures.len() != commitment_stats.tx.htlcs().len() {
+ return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_stats.tx.htlcs().len())));
}
// Up to LDK 0.0.115, HTLC information was required to be duplicated in the
@@ -3550,7 +3548,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
let holder_keys = commitment_stats.tx.trust().keys();
for (idx, (htlc, mut source_opt)) in htlcs_cloned.drain(..).enumerate() {
if let Some(_) = htlc.transaction_output_index {
- let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_stats.feerate_per_kw,
+ let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_stats.tx.feerate_per_kw(),
funding.get_counterparty_selected_contest_delay().unwrap(), &htlc, &self.channel_type,
&holder_keys.broadcaster_delayed_payment_key, &holder_keys.revocation_key);
@@ -3808,8 +3806,6 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
to_countersignatory_value_sat = 0;
}
- let num_nondust_htlcs = included_non_dust_htlcs.len();
-
let channel_parameters =
if local { funding.channel_transaction_parameters.as_holder_broadcastable() }
else { funding.channel_transaction_parameters.as_counterparty_broadcastable() };
@@ -3829,12 +3825,10 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
CommitmentStats {
tx,
- feerate_per_kw,
total_fee_sat,
- num_nondust_htlcs,
htlcs_included,
- local_balance_msat: value_to_self_msat,
- remote_balance_msat: value_to_remote_msat,
+ local_balance_before_fee_msat: value_to_self_msat,
+ remote_balance_before_fee_msat: value_to_remote_msat,
inbound_htlc_preimages,
outbound_htlc_preimages,
}
@@ -6337,8 +6331,8 @@ impl<SP: Deref> FundedChannel<SP> where
let dust_exposure_limiting_feerate = self.context.get_dust_exposure_limiting_feerate(&fee_estimator);
let htlc_stats = self.context.get_pending_htlc_stats(Some(feerate_per_kw), dust_exposure_limiting_feerate);
let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.holder_commitment_point.transaction_number(), &self.holder_commitment_point.current_point(), true, true, logger);
- let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_stats.num_nondust_htlcs + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.context.get_channel_type()) * 1000;
- let holder_balance_msat = commitment_stats.local_balance_msat - htlc_stats.outbound_holding_cell_msat;
+ let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_stats.tx.htlcs().len() + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.context.get_channel_type()) * 1000;
+ let holder_balance_msat = commitment_stats.local_balance_before_fee_msat - htlc_stats.outbound_holding_cell_msat;
if holder_balance_msat < buffer_fee_msat + self.funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 {
//TODO: auto-close after a number of failures?
log_debug!(logger, "Cannot afford to send new feerate at {}", feerate_per_kw);
@@ -8736,7 +8730,7 @@ impl<SP: Deref> FundedChannel<SP> where
&& info.next_holder_htlc_id == self.context.next_holder_htlc_id
&& info.next_counterparty_htlc_id == self.context.next_counterparty_htlc_id
&& info.feerate == self.context.feerate_per_kw {
- let actual_fee = commit_tx_fee_sat(self.context.feerate_per_kw, commitment_stats.num_nondust_htlcs, self.context.get_channel_type()) * 1000;
+ let actual_fee = commit_tx_fee_sat(self.context.feerate_per_kw, counterparty_commitment_tx.htlcs().len(), self.context.get_channel_type()) * 1000;
assert_eq!(actual_fee, info.fee);
}
}
@@ -8780,7 +8774,7 @@ impl<SP: Deref> FundedChannel<SP> where
debug_assert_eq!(htlc_signatures.len(), commitment_stats.tx.htlcs().len());
for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(commitment_stats.tx.htlcs()) {
log_trace!(logger, "Signed remote HTLC tx {} with redeemscript {} with pubkey {} -> {} in channel {}",
- encode::serialize_hex(&chan_utils::build_htlc_transaction(&counterparty_commitment_txid, commitment_stats.feerate_per_kw, self.funding.get_holder_selected_contest_delay(), htlc, &self.context.channel_type, &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)),
+ encode::serialize_hex(&chan_utils::build_htlc_transaction(&counterparty_commitment_txid, commitment_stats.tx.feerate_per_kw(), self.funding.get_holder_selected_contest_delay(), htlc, &self.context.channel_type, &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)),
encode::serialize_hex(&chan_utils::get_htlc_redeemscript(&htlc, &self.context.channel_type, &counterparty_keys)),
log_bytes!(counterparty_keys.broadcaster_htlc_key.to_public_key().serialize()),
log_bytes!(htlc_sig.serialize_compact()[..]), &self.context.channel_id());
From 935a8fd2835ffee35a6dd5c5bcfb2f1c11b520ef Mon Sep 17 00:00:00 2001
From: Leo Nash <hello@leonash.net>
Date: Thu, 27 Feb 2025 17:48:29 +0000
Subject: [PATCH 09/10] Reorg return values of
`ChannelContext::build_commitment_transaction`
We choose to include in `CommitmentStats` only fields that will be
calculated or constructed by transaction builders.
The other fields are created by channel, and placed in a new struct
we call `CommitmentData`.
---
lightning/src/ln/channel.rs | 121 +++++++++++++++++++++++-------------
1 file changed, 77 insertions(+), 44 deletions(-)
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 170af140803..d6fbd162388 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -877,15 +877,20 @@ struct HTLCStats {
on_holder_tx_outbound_holding_cell_htlcs_count: u32, // dust HTLCs *non*-included
}
-/// An enum gathering stats on commitment transaction, either local or remote.
-struct CommitmentStats<'a> {
+/// A struct gathering data on a commitment, either local or remote.
+struct CommitmentData<'a> {
+ stats: CommitmentStats,
+ htlcs_included: Vec<(HTLCOutputInCommitment, Option<&'a HTLCSource>)>, // the list of HTLCs (dust HTLCs *included*) which were not ignored when building the transaction
+ 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
+}
+
+/// A struct gathering stats on a commitment transaction, either local or remote.
+struct CommitmentStats {
tx: CommitmentTransaction, // the transaction info
total_fee_sat: u64, // the total fee included in 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_before_fee_msat: u64, // local balance before fees *not* considering dust limits
remote_balance_before_fee_msat: u64, // remote balance before fees *not* considering dust limits
- 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.
@@ -2043,7 +2048,10 @@ trait InitialRemoteCommitmentReceiver<SP: Deref> where SP::Target: SignerProvide
) -> Result<CommitmentTransaction, ChannelError> where L::Target: Logger {
let funding_script = self.funding().get_funding_redeemscript();
- let initial_commitment_tx = self.context().build_commitment_transaction(self.funding(), holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(), true, false, logger).tx;
+ let commitment_data = self.context().build_commitment_transaction(self.funding(),
+ holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(),
+ true, false, logger);
+ let initial_commitment_tx = commitment_data.stats.tx;
let trusted_tx = initial_commitment_tx.trust();
let initial_commitment_bitcoin_tx = trusted_tx.built_transaction();
let sighash = initial_commitment_bitcoin_tx.get_sighash_all(&funding_script, self.funding().get_value_satoshis());
@@ -2080,7 +2088,10 @@ trait InitialRemoteCommitmentReceiver<SP: Deref> where SP::Target: SignerProvide
}
};
let context = self.context();
- let counterparty_initial_commitment_tx = context.build_commitment_transaction(self.funding(), context.cur_counterparty_commitment_transaction_number, &context.counterparty_cur_commitment_point.unwrap(), false, false, logger).tx;
+ let commitment_data = context.build_commitment_transaction(self.funding(),
+ context.cur_counterparty_commitment_transaction_number,
+ &context.counterparty_cur_commitment_point.unwrap(), false, false, logger);
+ let counterparty_initial_commitment_tx = commitment_data.stats.tx;
let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust();
let counterparty_initial_bitcoin_tx = counterparty_trusted_tx.built_transaction();
@@ -3477,9 +3488,11 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
{
let funding_script = funding.get_funding_redeemscript();
- let commitment_stats = self.build_commitment_transaction(funding, holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(), true, false, logger);
+ let commitment_data = self.build_commitment_transaction(funding,
+ holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(),
+ true, false, logger);
let commitment_txid = {
- let trusted_tx = commitment_stats.tx.trust();
+ let trusted_tx = commitment_data.stats.tx.trust();
let bitcoin_tx = trusted_tx.built_transaction();
let sighash = bitcoin_tx.get_sighash_all(&funding_script, funding.get_value_satoshis());
@@ -3492,7 +3505,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
}
bitcoin_tx.txid
};
- let mut htlcs_cloned: Vec<_> = commitment_stats.htlcs_included.iter().map(|htlc| (htlc.0.clone(), htlc.1.map(|h| h.clone()))).collect();
+ let mut htlcs_cloned: Vec<_> = commitment_data.htlcs_included.iter().map(|htlc| (htlc.0.clone(), htlc.1.map(|h| h.clone()))).collect();
// If our counterparty updated the channel fee in this commitment transaction, check that
// they can actually afford the new fee now.
@@ -3502,7 +3515,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
if update_fee {
debug_assert!(!funding.is_outbound());
let counterparty_reserve_we_require_msat = funding.holder_selected_channel_reserve_satoshis * 1000;
- if commitment_stats.remote_balance_before_fee_msat < commitment_stats.total_fee_sat * 1000 + counterparty_reserve_we_require_msat {
+ if commitment_data.stats.remote_balance_before_fee_msat < commitment_data.stats.total_fee_sat * 1000 + counterparty_reserve_we_require_msat {
return Err(ChannelError::close("Funding remote cannot afford proposed new fee".to_owned()));
}
}
@@ -3518,14 +3531,14 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
&& info.next_holder_htlc_id == self.next_holder_htlc_id
&& info.next_counterparty_htlc_id == self.next_counterparty_htlc_id
&& info.feerate == self.feerate_per_kw {
- assert_eq!(commitment_stats.total_fee_sat, info.fee / 1000);
+ assert_eq!(commitment_data.stats.total_fee_sat, info.fee / 1000);
}
}
}
}
- if msg.htlc_signatures.len() != commitment_stats.tx.htlcs().len() {
- return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_stats.tx.htlcs().len())));
+ if msg.htlc_signatures.len() != commitment_data.stats.tx.htlcs().len() {
+ return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_data.stats.tx.htlcs().len())));
}
// Up to LDK 0.0.115, HTLC information was required to be duplicated in the
@@ -3545,10 +3558,10 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
let mut nondust_htlc_sources = Vec::with_capacity(htlcs_cloned.len());
let mut htlcs_and_sigs = Vec::with_capacity(htlcs_cloned.len());
- let holder_keys = commitment_stats.tx.trust().keys();
+ let holder_keys = commitment_data.stats.tx.trust().keys();
for (idx, (htlc, mut source_opt)) in htlcs_cloned.drain(..).enumerate() {
if let Some(_) = htlc.transaction_output_index {
- let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_stats.tx.feerate_per_kw(),
+ let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_data.stats.tx.feerate_per_kw(),
funding.get_counterparty_selected_contest_delay().unwrap(), &htlc, &self.channel_type,
&holder_keys.broadcaster_delayed_payment_key, &holder_keys.revocation_key);
@@ -3576,14 +3589,14 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
}
let holder_commitment_tx = HolderCommitmentTransaction::new(
- commitment_stats.tx,
+ commitment_data.stats.tx,
msg.signature,
msg.htlc_signatures.clone(),
&funding.get_holder_pubkeys().funding_pubkey,
funding.counterparty_funding_pubkey()
);
- self.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, commitment_stats.outbound_htlc_preimages)
+ self.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, commitment_data.outbound_htlc_preimages)
.map_err(|_| ChannelError::close("Failed to validate our commitment".to_owned()))?;
Ok(LatestHolderCommitmentTXInfo {
@@ -3607,7 +3620,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
/// generated by the peer which proposed adding the HTLCs, and thus we need to understand both
/// which peer generated this transaction and "to whom" this transaction flows.
#[inline]
- fn build_commitment_transaction<L: Deref>(&self, funding: &FundingScope, commitment_number: u64, per_commitment_point: &PublicKey, local: bool, generated_by_local: bool, logger: &L) -> CommitmentStats
+ fn build_commitment_transaction<L: Deref>(&self, funding: &FundingScope, commitment_number: u64, per_commitment_point: &PublicKey, local: bool, generated_by_local: bool, logger: &L) -> CommitmentData
where L::Target: Logger
{
let mut included_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::new();
@@ -3823,12 +3836,16 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
htlcs_included.sort_unstable_by_key(|h| h.0.transaction_output_index.unwrap());
htlcs_included.append(&mut included_dust_htlcs);
- CommitmentStats {
+ let stats = CommitmentStats {
tx,
total_fee_sat,
- htlcs_included,
local_balance_before_fee_msat: value_to_self_msat,
remote_balance_before_fee_msat: value_to_remote_msat,
+ };
+
+ CommitmentData {
+ stats,
+ htlcs_included,
inbound_htlc_preimages,
outbound_htlc_preimages,
}
@@ -4616,8 +4633,10 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
SP::Target: SignerProvider,
L::Target: Logger
{
- let counterparty_initial_commitment_tx = self.build_commitment_transaction(
- funding, self.cur_counterparty_commitment_transaction_number, &self.counterparty_cur_commitment_point.unwrap(), false, false, logger).tx;
+ let commitment_data = self.build_commitment_transaction(funding,
+ self.cur_counterparty_commitment_transaction_number,
+ &self.counterparty_cur_commitment_point.unwrap(), false, false, logger);
+ let counterparty_initial_commitment_tx = commitment_data.stats.tx;
match self.holder_signer {
// TODO (taproot|arik): move match into calling method for Taproot
ChannelSignerType::Ecdsa(ref ecdsa) => {
@@ -6330,9 +6349,11 @@ impl<SP: Deref> FundedChannel<SP> where
// Before proposing a feerate update, check that we can actually afford the new fee.
let dust_exposure_limiting_feerate = self.context.get_dust_exposure_limiting_feerate(&fee_estimator);
let htlc_stats = self.context.get_pending_htlc_stats(Some(feerate_per_kw), dust_exposure_limiting_feerate);
- let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.holder_commitment_point.transaction_number(), &self.holder_commitment_point.current_point(), true, true, logger);
- let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_stats.tx.htlcs().len() + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.context.get_channel_type()) * 1000;
- let holder_balance_msat = commitment_stats.local_balance_before_fee_msat - htlc_stats.outbound_holding_cell_msat;
+ let commitment_data = self.context.build_commitment_transaction(&self.funding,
+ self.holder_commitment_point.transaction_number(),
+ &self.holder_commitment_point.current_point(), true, true, logger);
+ let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_data.stats.tx.htlcs().len() + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.context.get_channel_type()) * 1000;
+ let holder_balance_msat = commitment_data.stats.local_balance_before_fee_msat - htlc_stats.outbound_holding_cell_msat;
if holder_balance_msat < buffer_fee_msat + self.funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 {
//TODO: auto-close after a number of failures?
log_debug!(logger, "Cannot afford to send new feerate at {}", feerate_per_kw);
@@ -6643,7 +6664,10 @@ impl<SP: Deref> FundedChannel<SP> where
self.holder_commitment_point.try_resolve_pending(&self.context.holder_signer, &self.context.secp_ctx, logger);
}
let funding_signed = if self.context.signer_pending_funding && !self.funding.is_outbound() {
- let counterparty_initial_commitment_tx = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number + 1, &self.context.counterparty_cur_commitment_point.unwrap(), false, false, logger).tx;
+ let commitment_data = self.context.build_commitment_transaction(&self.funding,
+ self.context.cur_counterparty_commitment_transaction_number + 1,
+ &self.context.counterparty_cur_commitment_point.unwrap(), false, false, logger);
+ let counterparty_initial_commitment_tx = commitment_data.stats.tx;
self.context.get_funding_signed_msg(&self.funding.channel_transaction_parameters, logger, counterparty_initial_commitment_tx)
} else { None };
// Provide a `channel_ready` message if we need to, but only if we're _not_ still pending
@@ -8716,8 +8740,10 @@ impl<SP: Deref> FundedChannel<SP> where
-> (Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>, CommitmentTransaction)
where L::Target: Logger
{
- let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger);
- let counterparty_commitment_tx = commitment_stats.tx;
+ let commitment_data = self.context.build_commitment_transaction(&self.funding,
+ self.context.cur_counterparty_commitment_transaction_number,
+ &self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger);
+ let counterparty_commitment_tx = commitment_data.stats.tx;
#[cfg(any(test, fuzzing))]
{
@@ -8737,7 +8763,7 @@ impl<SP: Deref> FundedChannel<SP> where
}
}
- (commitment_stats.htlcs_included, counterparty_commitment_tx)
+ (commitment_data.htlcs_included, counterparty_commitment_tx)
}
/// Only fails in case of signer rejection. Used for channel_reestablish commitment_signed
@@ -8747,8 +8773,10 @@ impl<SP: Deref> FundedChannel<SP> where
#[cfg(any(test, fuzzing))]
self.build_commitment_no_state_update(logger);
- let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger);
- let counterparty_commitment_txid = commitment_stats.tx.trust().txid();
+ let commitment_data = self.context.build_commitment_transaction(&self.funding,
+ self.context.cur_counterparty_commitment_transaction_number,
+ &self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger);
+ let counterparty_commitment_tx = commitment_data.stats.tx;
match &self.context.holder_signer {
ChannelSignerType::Ecdsa(ecdsa) => {
@@ -8757,24 +8785,25 @@ impl<SP: Deref> FundedChannel<SP> where
{
let res = ecdsa.sign_counterparty_commitment(
&self.funding.channel_transaction_parameters,
- &commitment_stats.tx,
- commitment_stats.inbound_htlc_preimages,
- commitment_stats.outbound_htlc_preimages,
+ &counterparty_commitment_tx,
+ commitment_data.inbound_htlc_preimages,
+ commitment_data.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;
+ let trusted_tx = counterparty_commitment_tx.trust();
log_trace!(logger, "Signed remote commitment tx {} (txid {}) with redeemscript {} -> {} in channel {}",
- encode::serialize_hex(&commitment_stats.tx.trust().built_transaction().transaction),
- &counterparty_commitment_txid, encode::serialize_hex(&self.funding.get_funding_redeemscript()),
+ encode::serialize_hex(&trusted_tx.built_transaction().transaction),
+ &trusted_tx.txid(), encode::serialize_hex(&self.funding.get_funding_redeemscript()),
log_bytes!(signature.serialize_compact()[..]), &self.context.channel_id());
- let counterparty_keys = commitment_stats.tx.trust().keys();
- debug_assert_eq!(htlc_signatures.len(), commitment_stats.tx.htlcs().len());
- for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(commitment_stats.tx.htlcs()) {
+ let counterparty_keys = trusted_tx.keys();
+ debug_assert_eq!(htlc_signatures.len(), trusted_tx.htlcs().len());
+ for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(trusted_tx.htlcs()) {
log_trace!(logger, "Signed remote HTLC tx {} with redeemscript {} with pubkey {} -> {} in channel {}",
- encode::serialize_hex(&chan_utils::build_htlc_transaction(&counterparty_commitment_txid, commitment_stats.tx.feerate_per_kw(), self.funding.get_holder_selected_contest_delay(), htlc, &self.context.channel_type, &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)),
+ encode::serialize_hex(&chan_utils::build_htlc_transaction(&trusted_tx.txid(), trusted_tx.feerate_per_kw(), self.funding.get_holder_selected_contest_delay(), htlc, &self.context.channel_type, &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)),
encode::serialize_hex(&chan_utils::get_htlc_redeemscript(&htlc, &self.context.channel_type, &counterparty_keys)),
log_bytes!(counterparty_keys.broadcaster_htlc_key.to_public_key().serialize()),
log_bytes!(htlc_sig.serialize_compact()[..]), &self.context.channel_id());
@@ -9228,7 +9257,10 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
/// Only allowed after [`FundingScope::channel_transaction_parameters`] is set.
fn get_funding_created_msg<L: Deref>(&mut self, logger: &L) -> Option<msgs::FundingCreated> where L::Target: Logger {
- let counterparty_initial_commitment_tx = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &self.context.counterparty_cur_commitment_point.unwrap(), false, false, logger).tx;
+ let commitment_data = self.context.build_commitment_transaction(&self.funding,
+ self.context.cur_counterparty_commitment_transaction_number,
+ &self.context.counterparty_cur_commitment_point.unwrap(), false, false, logger);
+ let counterparty_initial_commitment_tx = commitment_data.stats.tx;
let signature = match &self.context.holder_signer {
// TODO (taproot|arik): move match into calling method for Taproot
ChannelSignerType::Ecdsa(ecdsa) => {
@@ -11879,8 +11911,9 @@ mod tests {
( $counterparty_sig_hex: expr, $sig_hex: expr, $tx_hex: expr, $opt_anchors: expr, {
$( { $htlc_idx: expr, $counterparty_htlc_sig_hex: expr, $htlc_sig_hex: expr, $htlc_tx_hex: expr } ), *
} ) => { {
- let commitment_stats = chan.context.build_commitment_transaction(&chan.funding, 0xffffffffffff - 42, &per_commitment_point, true, false, &logger);
- let commitment_tx = commitment_stats.tx;
+ let commitment_data = chan.context.build_commitment_transaction(&chan.funding,
+ 0xffffffffffff - 42, &per_commitment_point, true, false, &logger);
+ let commitment_tx = commitment_data.stats.tx;
let trusted_tx = commitment_tx.trust();
let unsigned_tx = trusted_tx.built_transaction();
let redeemscript = chan.funding.get_funding_redeemscript();
From ff259bafdaeea3ffa47733acc442c14c562c97f4 Mon Sep 17 00:00:00 2001
From: Leo Nash <hello@leonash.net>
Date: Mon, 17 Mar 2025 20:55:54 +0000
Subject: [PATCH 10/10] Cover the saturating sub of the fee and anchors from
funder's balance
The saturating subtractions used in
`ChannelContext::build_commitment_transaction` are there for a reason;
when the remote party updates the fees, we build the new commitment
transaction before checking whether the remote party has enough balance
to cover the new fee, so we cannot guarantee that the remote party's
balance will be greater than or equal to the sum of the fee and the
anchors.
---
lightning/src/ln/functional_tests.rs | 84 ++++++++++++++++++++++++++++
1 file changed, 84 insertions(+)
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index 068199fc1c9..4cfd63ab2d6 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -789,6 +789,90 @@ pub fn test_update_fee_that_funder_cannot_afford() {
[nodes[0].node.get_our_node_id()], channel_value);
}
+#[xtest(feature = "_externalize_tests")]
+pub fn test_update_fee_that_saturates_subs() {
+ // Check that when a remote party sends us an `update_fee` message that results in a total fee
+ // on the commitment transaction that is greater than her balance, we saturate the subtractions,
+ // and force close the channel.
+
+ let mut default_config = test_default_channel_config();
+ let secp_ctx = Secp256k1::new();
+
+ let chanmon_cfgs = create_chanmon_cfgs(2);
+ let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+ let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(default_config), Some(default_config)]);
+ let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+ let chan_id = create_chan_between_nodes_with_value(&nodes[0], &nodes[1], 10_000, 8_500_000).3;
+
+ const FEERATE: u32 = 250 * 10; // 10sat/vb
+
+ // Assert that the new feerate will completely exhaust the balance of node 0, and saturate the
+ // subtraction of the total fee from node 0's balance.
+ let total_fee_sat = chan_utils::commit_tx_fee_sat(FEERATE, 0, &ChannelTypeFeatures::empty());
+ assert!(total_fee_sat > 1500);
+
+ const INITIAL_COMMITMENT_NUMBER: u64 = 281474976710654;
+
+ // We build a commitment transcation here only to pass node 1's check of node 0's signature
+ // in `commitment_signed`.
+
+ let remote_point = {
+ let per_peer_state = nodes[1].node.per_peer_state.read().unwrap();
+ let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().lock().unwrap();
+ let remote_chan = chan_lock.channel_by_id.get(&chan_id).and_then(Channel::as_funded).unwrap();
+ let chan_signer = remote_chan.get_signer();
+ chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER, &secp_ctx).unwrap()
+ };
+
+ let res = {
+ let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
+ let local_chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
+ let local_chan = local_chan_lock.channel_by_id.get(&chan_id).and_then(Channel::as_funded).unwrap();
+ let local_chan_signer = local_chan.get_signer();
+ let mut htlcs: Vec<(HTLCOutputInCommitment, ())> = vec![];
+ let commitment_tx = CommitmentTransaction::new_with_auxiliary_htlc_data(
+ INITIAL_COMMITMENT_NUMBER,
+ &remote_point,
+ 8500,
+ // Set a zero balance here: this is the transaction that node 1 will expect a signature for, as
+ // he will do a saturating subtraction of the total fees from node 0's balance.
+ 0,
+ FEERATE,
+ &mut htlcs,
+ &local_chan.funding.channel_transaction_parameters.as_counterparty_broadcastable(),
+ &secp_ctx,
+ );
+ local_chan_signer.as_ecdsa().unwrap().sign_counterparty_commitment(
+ &local_chan.funding.channel_transaction_parameters, &commitment_tx, Vec::new(),
+ Vec::new(), &secp_ctx,
+ ).unwrap()
+ };
+
+ let commit_signed_msg = msgs::CommitmentSigned {
+ channel_id: chan_id,
+ signature: res.0,
+ htlc_signatures: res.1,
+ batch: None,
+ #[cfg(taproot)]
+ partial_signature_with_nonce: None,
+ };
+
+ let update_fee = msgs::UpdateFee {
+ channel_id: chan_id,
+ feerate_per_kw: FEERATE,
+ };
+
+ nodes[1].node.handle_update_fee(nodes[0].node.get_our_node_id(), &update_fee);
+
+ nodes[1].node.handle_commitment_signed(nodes[0].node.get_our_node_id(), &commit_signed_msg);
+ nodes[1].logger.assert_log_contains("lightning::ln::channelmanager", "Funding remote cannot afford proposed new fee", 3);
+ check_added_monitors!(nodes[1], 1);
+ check_closed_broadcast!(nodes[1], true);
+ check_closed_event!(nodes[1], 1, ClosureReason::ProcessingError { err: String::from("Funding remote cannot afford proposed new fee") },
+ [nodes[0].node.get_our_node_id()], 10_000);
+}
+
#[xtest(feature = "_externalize_tests")]
pub fn test_update_fee_with_fundee_update_add_htlc() {
let chanmon_cfgs = create_chanmon_cfgs(2);