Skip to content

Commit

Permalink
Move next_*_commitment_tx_fee_info_cached to FundingScope
Browse files Browse the repository at this point in the history
  • Loading branch information
@jkczyz
jkczyz committed Feb 18, 2025
1 parent 3600d49 commit 04abb58
Showing 1 changed file with 80 additions and 46 deletions.
126 changes: 80 additions & 46 deletions lightning/src/ln/channel.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1579,6 +1579,15 @@ pub(super) struct FundingScope {
#[cfg(debug_assertions)]
/// Max to_local and to_remote outputs in a remote-generated commitment transaction
counterparty_max_commitment_tx_output: Mutex<(u64, u64)>,

// We save these values so we can make sure `next_local_commit_tx_fee_msat` and
// `next_remote_commit_tx_fee_msat` properly predict what the next commitment transaction fee will
// be, by comparing the cached values to the fee of the transaction generated by
// `build_commitment_transaction`.
#[cfg(any(test, fuzzing))]
next_local_commitment_tx_fee_info_cached: Mutex<Option<CommitmentTxInfoCached>>,
#[cfg(any(test, fuzzing))]
next_remote_commitment_tx_fee_info_cached: Mutex<Option<CommitmentTxInfoCached>>,
}

impl FundingScope {
Expand Down Expand Up @@ -1823,15 +1832,6 @@ pub(super) struct ChannelContext<SP: Deref> where SP::Target: SignerProvider {
/// This can be used to rebroadcast the channel_announcement message later.
announcement_sigs: Option<(Signature, Signature)>,

// We save these values so we can make sure `next_local_commit_tx_fee_msat` and
// `next_remote_commit_tx_fee_msat` properly predict what the next commitment transaction fee will
// be, by comparing the cached values to the fee of the tranaction generated by
// `build_commitment_transaction`.
#[cfg(any(test, fuzzing))]
next_local_commitment_tx_fee_info_cached: Mutex<Option<CommitmentTxInfoCached>>,
#[cfg(any(test, fuzzing))]
next_remote_commitment_tx_fee_info_cached: Mutex<Option<CommitmentTxInfoCached>>,

/// lnd has a long-standing bug where, upon reconnection, if the channel is not yet confirmed
/// they will not send a channel_reestablish until the channel locks in. Then, they will send a
/// channel_ready *before* sending the channel_reestablish (which is clearly a violation of
Expand Down Expand Up @@ -2470,6 +2470,11 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
holder_max_commitment_tx_output: Mutex::new((value_to_self_msat, (channel_value_satoshis * 1000 - msg_push_msat).saturating_sub(value_to_self_msat))),
#[cfg(debug_assertions)]
counterparty_max_commitment_tx_output: Mutex::new((value_to_self_msat, (channel_value_satoshis * 1000 - msg_push_msat).saturating_sub(value_to_self_msat))),

#[cfg(any(test, fuzzing))]
next_local_commitment_tx_fee_info_cached: Mutex::new(None),
#[cfg(any(test, fuzzing))]
next_remote_commitment_tx_fee_info_cached: Mutex::new(None),
};
let channel_context = ChannelContext {
user_id,
Expand Down Expand Up @@ -2578,11 +2583,6 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {

announcement_sigs: None,

#[cfg(any(test, fuzzing))]
next_local_commitment_tx_fee_info_cached: Mutex::new(None),
#[cfg(any(test, fuzzing))]
next_remote_commitment_tx_fee_info_cached: Mutex::new(None),

workaround_lnd_bug_4006: None,
sent_message_awaiting_response: None,

Expand Down Expand Up @@ -2705,6 +2705,11 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
holder_max_commitment_tx_output: Mutex::new((channel_value_satoshis * 1000 - push_msat, push_msat)),
#[cfg(debug_assertions)]
counterparty_max_commitment_tx_output: Mutex::new((channel_value_satoshis * 1000 - push_msat, push_msat)),

#[cfg(any(test, fuzzing))]
next_local_commitment_tx_fee_info_cached: Mutex::new(None),
#[cfg(any(test, fuzzing))]
next_remote_commitment_tx_fee_info_cached: Mutex::new(None),
};
let channel_context = Self {
user_id,
Expand Down Expand Up @@ -2810,11 +2815,6 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {

announcement_sigs: None,

#[cfg(any(test, fuzzing))]
next_local_commitment_tx_fee_info_cached: Mutex::new(None),
#[cfg(any(test, fuzzing))]
next_remote_commitment_tx_fee_info_cached: Mutex::new(None),

workaround_lnd_bug_4006: None,
sent_message_awaiting_response: None,

Expand Down Expand Up @@ -3868,9 +3868,17 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
}

let htlc_above_dust = HTLCCandidate::new(real_dust_limit_timeout_sat * 1000, HTLCInitiator::LocalOffered);
let mut max_reserved_commit_tx_fee_msat = context.next_local_commit_tx_fee_msat(htlc_above_dust, Some(()));
let mut max_reserved_commit_tx_fee_msat = context.next_local_commit_tx_fee_msat(
#[cfg(any(test, fuzzing))]
&funding,
htlc_above_dust, Some(()),
);
let htlc_dust = HTLCCandidate::new(real_dust_limit_timeout_sat * 1000 - 1, HTLCInitiator::LocalOffered);
let mut min_reserved_commit_tx_fee_msat = context.next_local_commit_tx_fee_msat(htlc_dust, Some(()));
let mut min_reserved_commit_tx_fee_msat = context.next_local_commit_tx_fee_msat(
#[cfg(any(test, fuzzing))]
&funding,
htlc_dust, Some(()),
);
if !context.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
max_reserved_commit_tx_fee_msat *= FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE;
min_reserved_commit_tx_fee_msat *= FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE;
Expand Down Expand Up @@ -3899,7 +3907,11 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
}

let htlc_above_dust = HTLCCandidate::new(real_dust_limit_success_sat * 1000, HTLCInitiator::LocalOffered);
let max_reserved_commit_tx_fee_msat = context.next_remote_commit_tx_fee_msat(Some(htlc_above_dust), None);
let max_reserved_commit_tx_fee_msat = context.next_remote_commit_tx_fee_msat(
#[cfg(any(test, fuzzing))]
&funding,
Some(htlc_above_dust), None,
);

let holder_selected_chan_reserve_msat = funding.holder_selected_channel_reserve_satoshis * 1000;
let remote_balance_msat = (funding.channel_value_satoshis * 1000 - funding.value_to_self_msat)
Expand Down Expand Up @@ -3996,7 +4008,12 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
/// second allows for creating a buffer to ensure a further HTLC can always be accepted/added.
///
/// Dust HTLCs are excluded.
fn next_local_commit_tx_fee_msat(&self, htlc: HTLCCandidate, fee_spike_buffer_htlc: Option<()>) -> u64 {
fn next_local_commit_tx_fee_msat(
&self,
#[cfg(any(test, fuzzing))]
funding: &FundingScope,
htlc: HTLCCandidate, fee_spike_buffer_htlc: Option<()>,
) -> u64 {
let context = &self;
assert!(context.is_outbound());

Expand Down Expand Up @@ -4085,7 +4102,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
},
feerate: context.feerate_per_kw,
};
*context.next_local_commitment_tx_fee_info_cached.lock().unwrap() = Some(commitment_tx_info);
*funding.next_local_commitment_tx_fee_info_cached.lock().unwrap() = Some(commitment_tx_info);
}
res
}
Expand All @@ -4100,7 +4117,12 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
/// second allows for creating a buffer to ensure a further HTLC can always be accepted/added.
///
/// Dust HTLCs are excluded.
fn next_remote_commit_tx_fee_msat(&self, htlc: Option<HTLCCandidate>, fee_spike_buffer_htlc: Option<()>) -> u64 {
fn next_remote_commit_tx_fee_msat(
&self,
#[cfg(any(test, fuzzing))]
funding: &FundingScope,
htlc: Option<HTLCCandidate>, fee_spike_buffer_htlc: Option<()>,
) -> u64 {
debug_assert!(htlc.is_some() || fee_spike_buffer_htlc.is_some(), "At least one of the options must be set");

let context = &self;
Expand Down Expand Up @@ -4179,7 +4201,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
},
feerate: context.feerate_per_kw,
};
*context.next_remote_commitment_tx_fee_info_cached.lock().unwrap() = Some(commitment_tx_info);
*funding.next_remote_commitment_tx_fee_info_cached.lock().unwrap() = Some(commitment_tx_info);
}
res
}
Expand Down Expand Up @@ -5199,7 +5221,11 @@ impl<SP: Deref> FundedChannel<SP> where
{
let remote_commit_tx_fee_msat = if self.context.is_outbound() { 0 } else {
let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
self.context.next_remote_commit_tx_fee_msat(Some(htlc_candidate), None) // Don't include the extra fee spike buffer HTLC in calculations
self.context.next_remote_commit_tx_fee_msat(
#[cfg(any(test, fuzzing))]
&self.funding,
Some(htlc_candidate), None, // Don't include the extra fee spike buffer HTLC in calculations
)
};
let anchor_outputs_value_msat = if !self.context.is_outbound() && self.context.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
ANCHOR_OUTPUT_VALUE_SATOSHI * 2 * 1000
Expand All @@ -5222,7 +5248,11 @@ impl<SP: Deref> FundedChannel<SP> where
if self.context.is_outbound() {
// Check that they won't violate our local required channel reserve by adding this HTLC.
let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
let local_commit_tx_fee_msat = self.context.next_local_commit_tx_fee_msat(htlc_candidate, None);
let local_commit_tx_fee_msat = self.context.next_local_commit_tx_fee_msat(
#[cfg(any(test, fuzzing))]
&self.funding,
htlc_candidate, None,
);
if self.funding.value_to_self_msat < self.funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 + local_commit_tx_fee_msat + anchor_outputs_value_msat {
return Err(ChannelError::close("Cannot accept HTLC that would put our balance under counterparty-announced channel reserve value".to_owned()));
}
Expand Down Expand Up @@ -5400,8 +5430,8 @@ impl<SP: Deref> FundedChannel<SP> where
#[cfg(any(test, fuzzing))]
{
if self.context.is_outbound() {
let projected_commit_tx_info = self.context.next_local_commitment_tx_fee_info_cached.lock().unwrap().take();
*self.context.next_remote_commitment_tx_fee_info_cached.lock().unwrap() = None;
let projected_commit_tx_info = self.funding.next_local_commitment_tx_fee_info_cached.lock().unwrap().take();
*self.funding.next_remote_commitment_tx_fee_info_cached.lock().unwrap() = None;
if let Some(info) = projected_commit_tx_info {
let total_pending_htlcs = self.context.pending_inbound_htlcs.len() + self.context.pending_outbound_htlcs.len()
+ self.context.holding_cell_htlc_updates.len();
Expand Down Expand Up @@ -5770,8 +5800,8 @@ impl<SP: Deref> FundedChannel<SP> where

#[cfg(any(test, fuzzing))]
{
*self.context.next_local_commitment_tx_fee_info_cached.lock().unwrap() = None;
*self.context.next_remote_commitment_tx_fee_info_cached.lock().unwrap() = None;
*self.funding.next_local_commitment_tx_fee_info_cached.lock().unwrap() = None;
*self.funding.next_remote_commitment_tx_fee_info_cached.lock().unwrap() = None;
}

match &self.context.holder_signer {
Expand Down Expand Up @@ -7411,7 +7441,11 @@ impl<SP: Deref> FundedChannel<SP> where
//
// A `None` `HTLCCandidate` is used as in this case because we're already accounting for
// the incoming HTLC as it has been fully committed by both sides.
let mut remote_fee_cost_incl_stuck_buffer_msat = self.context.next_remote_commit_tx_fee_msat(None, Some(()));
let mut remote_fee_cost_incl_stuck_buffer_msat = self.context.next_remote_commit_tx_fee_msat(
#[cfg(any(test, fuzzing))]
&self.funding,
None, Some(()),
);
if !self.context.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
remote_fee_cost_incl_stuck_buffer_msat *= FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE;
}
Expand Down Expand Up @@ -8353,8 +8387,8 @@ impl<SP: Deref> FundedChannel<SP> where
#[cfg(any(test, fuzzing))]
{
if !self.context.is_outbound() {
let projected_commit_tx_info = self.context.next_remote_commitment_tx_fee_info_cached.lock().unwrap().take();
*self.context.next_local_commitment_tx_fee_info_cached.lock().unwrap() = None;
let projected_commit_tx_info = self.funding.next_remote_commitment_tx_fee_info_cached.lock().unwrap().take();
*self.funding.next_local_commitment_tx_fee_info_cached.lock().unwrap() = None;
if let Some(info) = projected_commit_tx_info {
let total_pending_htlcs = self.context.pending_inbound_htlcs.len() + self.context.pending_outbound_htlcs.len();
if info.total_pending_htlcs == total_pending_htlcs
Expand Down Expand Up @@ -10375,6 +10409,11 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
holder_max_commitment_tx_output: Mutex::new((0, 0)),
#[cfg(debug_assertions)]
counterparty_max_commitment_tx_output: Mutex::new((0, 0)),

#[cfg(any(test, fuzzing))]
next_local_commitment_tx_fee_info_cached: Mutex::new(None),
#[cfg(any(test, fuzzing))]
next_remote_commitment_tx_fee_info_cached: Mutex::new(None),
},
context: ChannelContext {
user_id,
Expand Down Expand Up @@ -10470,11 +10509,6 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch

announcement_sigs,

#[cfg(any(test, fuzzing))]
next_local_commitment_tx_fee_info_cached: Mutex::new(None),
#[cfg(any(test, fuzzing))]
next_remote_commitment_tx_fee_info_cached: Mutex::new(None),

workaround_lnd_bug_4006: None,
sent_message_awaiting_response: None,

Expand Down Expand Up @@ -10744,7 +10778,7 @@ mod tests {
// Make sure when Node A calculates their local commitment transaction, none of the HTLCs pass
// the dust limit check.
let htlc_candidate = HTLCCandidate::new(htlc_amount_msat, HTLCInitiator::LocalOffered);
let local_commit_tx_fee = node_a_chan.context.next_local_commit_tx_fee_msat(htlc_candidate, None);
let local_commit_tx_fee = node_a_chan.context.next_local_commit_tx_fee_msat(&node_a_chan.funding, htlc_candidate, None);
let local_commit_fee_0_htlcs = commit_tx_fee_sat(node_a_chan.context.feerate_per_kw, 0, node_a_chan.context.get_channel_type()) * 1000;
assert_eq!(local_commit_tx_fee, local_commit_fee_0_htlcs);

Expand All @@ -10753,7 +10787,7 @@ mod tests {
node_a_chan.context.channel_transaction_parameters.is_outbound_from_holder = false;
let remote_commit_fee_3_htlcs = commit_tx_fee_sat(node_a_chan.context.feerate_per_kw, 3, node_a_chan.context.get_channel_type()) * 1000;
let htlc_candidate = HTLCCandidate::new(htlc_amount_msat, HTLCInitiator::LocalOffered);
let remote_commit_tx_fee = node_a_chan.context.next_remote_commit_tx_fee_msat(Some(htlc_candidate), None);
let remote_commit_tx_fee = node_a_chan.context.next_remote_commit_tx_fee_msat(&node_a_chan.funding, Some(htlc_candidate), None);
assert_eq!(remote_commit_tx_fee, remote_commit_fee_3_htlcs);
}

Expand Down Expand Up @@ -10781,27 +10815,27 @@ mod tests {
// counted as dust when it shouldn't be.
let htlc_amt_above_timeout = ((253 * htlc_timeout_tx_weight(chan.context.get_channel_type()) / 1000) + chan.context.holder_dust_limit_satoshis + 1) * 1000;
let htlc_candidate = HTLCCandidate::new(htlc_amt_above_timeout, HTLCInitiator::LocalOffered);
let commitment_tx_fee = chan.context.next_local_commit_tx_fee_msat(htlc_candidate, None);
let commitment_tx_fee = chan.context.next_local_commit_tx_fee_msat(&chan.funding, htlc_candidate, None);
assert_eq!(commitment_tx_fee, commitment_tx_fee_1_htlc);

// If swapped: this HTLC would be counted as non-dust when it shouldn't be.
let dust_htlc_amt_below_success = ((253 * htlc_success_tx_weight(chan.context.get_channel_type()) / 1000) + chan.context.holder_dust_limit_satoshis - 1) * 1000;
let htlc_candidate = HTLCCandidate::new(dust_htlc_amt_below_success, HTLCInitiator::RemoteOffered);
let commitment_tx_fee = chan.context.next_local_commit_tx_fee_msat(htlc_candidate, None);
let commitment_tx_fee = chan.context.next_local_commit_tx_fee_msat(&chan.funding, htlc_candidate, None);
assert_eq!(commitment_tx_fee, commitment_tx_fee_0_htlcs);

chan.context.channel_transaction_parameters.is_outbound_from_holder = false;

// If swapped: this HTLC would be counted as non-dust when it shouldn't be.
let dust_htlc_amt_above_timeout = ((253 * htlc_timeout_tx_weight(chan.context.get_channel_type()) / 1000) + chan.context.counterparty_dust_limit_satoshis + 1) * 1000;
let htlc_candidate = HTLCCandidate::new(dust_htlc_amt_above_timeout, HTLCInitiator::LocalOffered);
let commitment_tx_fee = chan.context.next_remote_commit_tx_fee_msat(Some(htlc_candidate), None);
let commitment_tx_fee = chan.context.next_remote_commit_tx_fee_msat(&chan.funding, Some(htlc_candidate), None);
assert_eq!(commitment_tx_fee, commitment_tx_fee_0_htlcs);

// If swapped: this HTLC would be counted as dust when it shouldn't be.
let htlc_amt_below_success = ((253 * htlc_success_tx_weight(chan.context.get_channel_type()) / 1000) + chan.context.counterparty_dust_limit_satoshis - 1) * 1000;
let htlc_candidate = HTLCCandidate::new(htlc_amt_below_success, HTLCInitiator::RemoteOffered);
let commitment_tx_fee = chan.context.next_remote_commit_tx_fee_msat(Some(htlc_candidate), None);
let commitment_tx_fee = chan.context.next_remote_commit_tx_fee_msat(&chan.funding, Some(htlc_candidate), None);
assert_eq!(commitment_tx_fee, commitment_tx_fee_1_htlc);
}

Expand Down

0 comments on commit 04abb58

Please sign in to comment.