From 9a110511ee69a41a40fd06a2b53160e32b428ea5 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Wed, 12 Feb 2025 15:16:52 -0600 Subject: [PATCH] Move next_*_commitment_tx_fee_info_cached to FundingScope --- lightning/src/ln/channel.rs | 126 +++++++++++++++++++++++------------- 1 file changed, 80 insertions(+), 46 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 8fa20fbe7d0..a342aba4690 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1600,6 +1600,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>, + #[cfg(any(test, fuzzing))] + next_remote_commitment_tx_fee_info_cached: Mutex>, } impl FundingScope { @@ -1844,15 +1853,6 @@ pub(super) struct ChannelContext 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>, - #[cfg(any(test, fuzzing))] - next_remote_commitment_tx_fee_info_cached: Mutex>, - /// 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 @@ -2491,6 +2491,11 @@ impl ChannelContext 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, @@ -2599,11 +2604,6 @@ impl ChannelContext 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, @@ -2726,6 +2726,11 @@ impl ChannelContext 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, @@ -2831,11 +2836,6 @@ impl ChannelContext 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, @@ -3889,9 +3889,17 @@ impl ChannelContext 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; @@ -3920,7 +3928,11 @@ impl ChannelContext 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) @@ -4017,7 +4029,12 @@ impl ChannelContext 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()); @@ -4106,7 +4123,7 @@ impl ChannelContext 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 } @@ -4121,7 +4138,12 @@ impl ChannelContext 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, fee_spike_buffer_htlc: Option<()>) -> u64 { + fn next_remote_commit_tx_fee_msat( + &self, + #[cfg(any(test, fuzzing))] + funding: &FundingScope, + htlc: Option, 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; @@ -4200,7 +4222,7 @@ impl ChannelContext 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 } @@ -5233,7 +5255,11 @@ impl FundedChannel 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 @@ -5256,7 +5282,11 @@ impl FundedChannel 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())); } @@ -5434,8 +5464,8 @@ impl FundedChannel 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(); @@ -5804,8 +5834,8 @@ impl FundedChannel 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 { @@ -7445,7 +7475,11 @@ impl FundedChannel 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; } @@ -8387,8 +8421,8 @@ impl FundedChannel 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 @@ -10413,6 +10447,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, @@ -10508,11 +10547,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, @@ -10782,7 +10816,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); @@ -10791,7 +10825,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); } @@ -10819,13 +10853,13 @@ 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; @@ -10833,13 +10867,13 @@ mod tests { // 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); }