From e1888c5a23ef2d655f010f27d5e62be8b774b10b Mon Sep 17 00:00:00 2001 From: optout <13562139+optout21@users.noreply.github.com> Date: Thu, 30 Jan 2025 18:57:49 +0100 Subject: [PATCH 1/2] Include base input fee in fee calculation The base input fee was missing in calculate_our_funding_satoshis(), it is added now; also add unit test. --- lightning/src/ln/channel.rs | 88 +++++++++++++++++++++++++++++++++++-- 1 file changed, 85 insertions(+), 3 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 1d64a396bfa..5935a29a865 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -57,6 +57,7 @@ use crate::chain::transaction::{OutPoint, TransactionData}; use crate::sign::ecdsa::EcdsaChannelSigner; use crate::sign::{EntropySource, ChannelSigner, SignerProvider, NodeSigner, Recipient}; use crate::events::{ClosureReason, Event}; +use crate::events::bump_transaction::BASE_INPUT_WEIGHT; use crate::routing::gossip::NodeId; use crate::util::ser::{Readable, ReadableArgs, TransactionU16LenLimited, Writeable, Writer}; use crate::util::logger::{Logger, Record, WithContext}; @@ -4475,7 +4476,6 @@ pub(super) fn calculate_our_funding_satoshis( holder_dust_limit_satoshis: u64, ) -> Result { let mut total_input_satoshis = 0u64; - let mut our_contributed_weight = 0u64; for (idx, input) in funding_inputs.iter().enumerate() { if let Some(output) = input.1.as_transaction().output.get(input.0.previous_output.vout as usize) { @@ -4486,6 +4486,9 @@ pub(super) fn calculate_our_funding_satoshis( input.1.as_transaction().compute_txid(), input.0.previous_output.vout, idx) }); } } + // inputs: + let mut our_contributed_weight = (funding_inputs.len() as u64) * BASE_INPUT_WEIGHT; + // witnesses: our_contributed_weight = our_contributed_weight.saturating_add(total_witness_weight.to_wu()); // If we are the initiator, we must pay for weight of all common fields in the funding transaction. @@ -10464,7 +10467,7 @@ mod tests { use bitcoin::amount::Amount; use bitcoin::constants::ChainHash; use bitcoin::script::{ScriptBuf, Builder}; - use bitcoin::transaction::{Transaction, TxOut, Version}; + use bitcoin::transaction::{Transaction, TxIn, TxOut, Version}; use bitcoin::opcodes; use bitcoin::network::Network; use crate::ln::onion_utils::INVALID_ONION_BLINDING; @@ -10486,7 +10489,7 @@ mod tests { use crate::routing::router::{Path, RouteHop}; use crate::util::config::UserConfig; use crate::util::errors::APIError; - use crate::util::ser::{ReadableArgs, Writeable}; + use crate::util::ser::{ReadableArgs, TransactionU16LenLimited, Writeable}; use crate::util::test_utils; use crate::util::test_utils::{OnGetShutdownScriptpubkey, TestKeysInterface}; use bitcoin::secp256k1::{Secp256k1, ecdsa::Signature}; @@ -12236,4 +12239,83 @@ mod tests { assert_eq!(node_a_chan.context.channel_state, ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::THEIR_CHANNEL_READY)); assert!(node_a_chan.check_get_channel_ready(0, &&logger).is_some()); } + + fn funding_input_sats(input_value_sats: u64) -> (TxIn, TransactionU16LenLimited) { + let input_1_prev_out = TxOut { value: Amount::from_sat(input_value_sats), script_pubkey: ScriptBuf::default() }; + let input_1_prev_tx = Transaction { + input: vec![], output: vec![input_1_prev_out], + version: Version::TWO, lock_time: bitcoin::absolute::LockTime::ZERO, + }; + let input_1_txin = TxIn { + previous_output: bitcoin::OutPoint { txid: input_1_prev_tx.compute_txid(), vout: 0 }, + ..Default::default() + }; + (input_1_txin, TransactionU16LenLimited::new(input_1_prev_tx).unwrap()) + } + + #[test] + fn test_calculate_our_funding_satoshis() { + use crate::ln::channel::calculate_our_funding_satoshis; + use bitcoin::Weight; + + // normal use case, output is less than the available inputs + assert_eq!( + calculate_our_funding_satoshis( + true, + &[ + funding_input_sats(200_000), + funding_input_sats(100_000), + ], + Weight::from_wu(300), + 2000, + 1000, + ).unwrap(), + 298332 + ); + + assert_eq!( + calculate_our_funding_satoshis( + true, + &[funding_input_sats(20_000)], + Weight::from_wu(300), + 2000, + 1000, + ).unwrap(), + 18652 + ); + + assert_eq!( + calculate_our_funding_satoshis( + true, + &[funding_input_sats(20_000)], + Weight::from_wu(0), + 2000, + 1000, + ).unwrap(), + 19252 + ); + + assert_eq!( + calculate_our_funding_satoshis( + false, + &[funding_input_sats(20_000)], + Weight::from_wu(0), + 2000, + 1000, + ).unwrap(), + 19680 + ); + + // below dust limit + assert_eq!( + calculate_our_funding_satoshis( + true, + &[funding_input_sats(20_000)], + Weight::from_wu(300), + 2000, + 20_000, + ).unwrap(), + 0 + ); + } } From c29ec0f926a4c98e7ecc0b839760ef7908a8b0ee Mon Sep 17 00:00:00 2001 From: optout <13562139+optout21@users.noreply.github.com> Date: Wed, 12 Feb 2025 07:36:11 +0100 Subject: [PATCH 2/2] Drop calculate_our_funding_satoshis(), keep only fee estimation This method does not take into the intended funding amount, and it's not currently used, therefore it's removed now. Its fee estimation part is kept (estimate_v2_funding_transaction_fee). --- lightning/src/ln/channel.rs | 152 ++++++++----------------- lightning/src/ln/channelmanager.rs | 17 ++- lightning/src/ln/dual_funding_tests.rs | 32 +++--- 3 files changed, 66 insertions(+), 135 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 5935a29a865..65e089cf256 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4469,34 +4469,25 @@ fn get_v2_channel_reserve_satoshis(channel_value_satoshis: u64, dust_limit_satos cmp::min(channel_value_satoshis, cmp::max(q, dust_limit_satoshis)) } -#[allow(dead_code)] // TODO(dual_funding): Remove once V2 channels is enabled. -pub(super) fn calculate_our_funding_satoshis( - is_initiator: bool, funding_inputs: &[(TxIn, TransactionU16LenLimited)], - total_witness_weight: Weight, funding_feerate_sat_per_1000_weight: u32, - holder_dust_limit_satoshis: u64, -) -> Result { - let mut total_input_satoshis = 0u64; - - for (idx, input) in funding_inputs.iter().enumerate() { - if let Some(output) = input.1.as_transaction().output.get(input.0.previous_output.vout as usize) { - total_input_satoshis = total_input_satoshis.saturating_add(output.value.to_sat()); - } else { - return Err(APIError::APIMisuseError { - err: format!("Transaction with txid {} does not have an output with vout of {} corresponding to TxIn at funding_inputs[{}]", - input.1.as_transaction().compute_txid(), input.0.previous_output.vout, idx) }); - } - } - // inputs: - let mut our_contributed_weight = (funding_inputs.len() as u64) * BASE_INPUT_WEIGHT; - // witnesses: - our_contributed_weight = our_contributed_weight.saturating_add(total_witness_weight.to_wu()); +/// Estimate our part of the fee of the new funding transaction. +/// input_count: Number of contributed inputs. +/// witness_weight: The witness weight for contributed inputs. +#[allow(dead_code)] // TODO(dual_funding): TODO(splicing): Remove allow once used. +fn estimate_v2_funding_transaction_fee( + is_initiator: bool, input_count: usize, witness_weight: Weight, + funding_feerate_sat_per_1000_weight: u32, +) -> u64 { + // Inputs + let mut weight = (input_count as u64) * BASE_INPUT_WEIGHT; + + // Witnesses + weight = weight.saturating_add(witness_weight.to_wu()); // If we are the initiator, we must pay for weight of all common fields in the funding transaction. if is_initiator { - our_contributed_weight = our_contributed_weight + weight = weight .saturating_add(TX_COMMON_FIELDS_WEIGHT) - // The weight of a P2WSH output to be added later. - // + // The weight of the funding output, a P2WSH output // NOTE: The witness script hash given here is irrelevant as it's a fixed size and we just want // to calculate the contributed weight, so we use an all-zero hash. .saturating_add(get_output_weight(&ScriptBuf::new_p2wsh( @@ -4504,13 +4495,7 @@ pub(super) fn calculate_our_funding_satoshis( )).to_wu()) } - let funding_satoshis = total_input_satoshis - .saturating_sub(fee_for_weight(funding_feerate_sat_per_1000_weight, our_contributed_weight)); - if funding_satoshis < holder_dust_limit_satoshis { - Ok(0) - } else { - Ok(funding_satoshis) - } + fee_for_weight(funding_feerate_sat_per_1000_weight, weight) } /// Context for dual-funded channels. @@ -9250,27 +9235,23 @@ impl PendingV2Channel where SP::Target: SignerProvider { /// Creates a new dual-funded channel from a remote side's request for one. /// Assumes chain_hash has already been checked and corresponds with what we expect! + /// TODO(dual_funding): Allow contributions, pass intended amount and inputs #[allow(dead_code)] // TODO(dual_funding): Remove once V2 channels is enabled. pub fn new_inbound( fee_estimator: &LowerBoundedFeeEstimator, entropy_source: &ES, signer_provider: &SP, holder_node_id: PublicKey, counterparty_node_id: PublicKey, our_supported_features: &ChannelTypeFeatures, their_features: &InitFeatures, msg: &msgs::OpenChannelV2, - funding_inputs: Vec<(TxIn, TransactionU16LenLimited)>, total_witness_weight: Weight, user_id: u128, config: &UserConfig, current_chain_height: u32, logger: &L, ) -> Result where ES::Target: EntropySource, F::Target: FeeEstimator, L::Target: Logger, { - let funding_satoshis = calculate_our_funding_satoshis( - false, &funding_inputs, total_witness_weight, msg.funding_feerate_sat_per_1000_weight, - msg.common_fields.dust_limit_satoshis - ).map_err(|_| ChannelError::Close( - ( - "Failed to accept channel".to_string(), - ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) }, - )))?; - let channel_value_satoshis = funding_satoshis.saturating_add(msg.common_fields.funding_satoshis); + // TODO(dual_funding): Take these as input once supported + let our_funding_satoshis = 0u64; + let our_funding_inputs = Vec::new(); + + let channel_value_satoshis = our_funding_satoshis.saturating_add(msg.common_fields.funding_satoshis); let counterparty_selected_channel_reserve_satoshis = get_v2_channel_reserve_satoshis( channel_value_satoshis, msg.common_fields.dust_limit_satoshis); let holder_selected_channel_reserve_satoshis = get_v2_channel_reserve_satoshis( @@ -9304,7 +9285,7 @@ impl PendingV2Channel where SP::Target: SignerProvider { logger, false, - funding_satoshis, + our_funding_satoshis, counterparty_pubkeys, channel_type, @@ -9319,10 +9300,10 @@ impl PendingV2Channel where SP::Target: SignerProvider { context.channel_id = channel_id; let dual_funding_context = DualFundingChannelContext { - our_funding_satoshis: funding_satoshis, + our_funding_satoshis: our_funding_satoshis, funding_tx_locktime: LockTime::from_consensus(msg.locktime), funding_feerate_sat_per_1000_weight: msg.funding_feerate_sat_per_1000_weight, - our_funding_inputs: funding_inputs.clone(), + our_funding_inputs: our_funding_inputs.clone(), }; let interactive_tx_constructor = Some(InteractiveTxConstructor::new( @@ -9334,7 +9315,7 @@ impl PendingV2Channel where SP::Target: SignerProvider { feerate_sat_per_kw: dual_funding_context.funding_feerate_sat_per_1000_weight, funding_tx_locktime: dual_funding_context.funding_tx_locktime, is_initiator: false, - inputs_to_contribute: funding_inputs, + inputs_to_contribute: our_funding_inputs, outputs_to_contribute: Vec::new(), expected_remote_shared_funding_output: Some((context.get_funding_redeemscript().to_p2wsh(), context.channel_value_satoshis)), } @@ -10467,7 +10448,7 @@ mod tests { use bitcoin::amount::Amount; use bitcoin::constants::ChainHash; use bitcoin::script::{ScriptBuf, Builder}; - use bitcoin::transaction::{Transaction, TxIn, TxOut, Version}; + use bitcoin::transaction::{Transaction, TxOut, Version}; use bitcoin::opcodes; use bitcoin::network::Network; use crate::ln::onion_utils::INVALID_ONION_BLINDING; @@ -10489,7 +10470,7 @@ mod tests { use crate::routing::router::{Path, RouteHop}; use crate::util::config::UserConfig; use crate::util::errors::APIError; - use crate::util::ser::{ReadableArgs, TransactionU16LenLimited, Writeable}; + use crate::util::ser::{ReadableArgs, Writeable}; use crate::util::test_utils; use crate::util::test_utils::{OnGetShutdownScriptpubkey, TestKeysInterface}; use bitcoin::secp256k1::{Secp256k1, ecdsa::Signature}; @@ -12240,82 +12221,39 @@ mod tests { assert!(node_a_chan.check_get_channel_ready(0, &&logger).is_some()); } - fn funding_input_sats(input_value_sats: u64) -> (TxIn, TransactionU16LenLimited) { - let input_1_prev_out = TxOut { value: Amount::from_sat(input_value_sats), script_pubkey: ScriptBuf::default() }; - let input_1_prev_tx = Transaction { - input: vec![], output: vec![input_1_prev_out], - version: Version::TWO, lock_time: bitcoin::absolute::LockTime::ZERO, - }; - let input_1_txin = TxIn { - previous_output: bitcoin::OutPoint { txid: input_1_prev_tx.compute_txid(), vout: 0 }, - ..Default::default() - }; - (input_1_txin, TransactionU16LenLimited::new(input_1_prev_tx).unwrap()) - } - #[test] - fn test_calculate_our_funding_satoshis() { - use crate::ln::channel::calculate_our_funding_satoshis; + fn test_estimate_v2_funding_transaction_fee() { + use crate::ln::channel::estimate_v2_funding_transaction_fee; use bitcoin::Weight; - // normal use case, output is less than the available inputs + // 2 inputs with weight 300, initiator, 2000 sat/kw feerate assert_eq!( - calculate_our_funding_satoshis( - true, - &[ - funding_input_sats(200_000), - funding_input_sats(100_000), - ], - Weight::from_wu(300), - 2000, - 1000, - ).unwrap(), - 298332 + estimate_v2_funding_transaction_fee(true, 2, Weight::from_wu(300), 2000), + 1668 ); + // higher feerate assert_eq!( - calculate_our_funding_satoshis( - true, - &[funding_input_sats(20_000)], - Weight::from_wu(300), - 2000, - 1000, - ).unwrap(), - 18652 + estimate_v2_funding_transaction_fee(true, 2, Weight::from_wu(300), 3000), + 2502 ); + // only 1 input assert_eq!( - calculate_our_funding_satoshis( - true, - &[funding_input_sats(20_000)], - Weight::from_wu(0), - 2000, - 1000, - ).unwrap(), - 19252 + estimate_v2_funding_transaction_fee(true, 1, Weight::from_wu(300), 2000), + 1348 ); + // 0 input weight assert_eq!( - calculate_our_funding_satoshis( - false, - &[funding_input_sats(20_000)], - Weight::from_wu(0), - 2000, - 1000, - ).unwrap(), - 19680 + estimate_v2_funding_transaction_fee(true, 1, Weight::from_wu(0), 2000), + 748 ); - // below dust limit + // not initiator assert_eq!( - calculate_our_funding_satoshis( - true, - &[funding_input_sats(20_000)], - Weight::from_wu(300), - 2000, - 20_000, - ).unwrap(), - 0 + estimate_v2_funding_transaction_fee(false, 1, Weight::from_wu(0), 2000), + 320 ); } } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 65c41bc531a..87c63b87b4c 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -18,7 +18,7 @@ //! imply it needs to fail HTLCs/payments/channels it manages). use bitcoin::block::Header; -use bitcoin::transaction::{Transaction, TxIn}; +use bitcoin::transaction::Transaction; use bitcoin::constants::ChainHash; use bitcoin::key::constants::SECRET_KEY_SIZE; use bitcoin::network::Network; @@ -30,7 +30,7 @@ use bitcoin::hash_types::{BlockHash, Txid}; use bitcoin::secp256k1::{SecretKey,PublicKey}; use bitcoin::secp256k1::Secp256k1; -use bitcoin::{secp256k1, Sequence, Weight}; +use bitcoin::{secp256k1, Sequence}; use crate::events::FundingInfo; use crate::blinded_path::message::{AsyncPaymentsContext, MessageContext, OffersContext}; @@ -83,7 +83,6 @@ use crate::util::wakers::{Future, Notifier}; use crate::util::scid_utils::fake_scid; use crate::util::string::UntrustedString; use crate::util::ser::{BigSize, FixedLengthReader, Readable, ReadableArgs, MaybeReadable, Writeable, Writer, VecWriter}; -use crate::util::ser::TransactionU16LenLimited; use crate::util::logger::{Level, Logger, WithContext}; use crate::util::errors::APIError; #[cfg(async_payments)] use { @@ -7648,7 +7647,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ /// [`Event::OpenChannelRequest`]: events::Event::OpenChannelRequest /// [`Event::ChannelClosed::user_channel_id`]: events::Event::ChannelClosed::user_channel_id pub fn accept_inbound_channel(&self, temporary_channel_id: &ChannelId, counterparty_node_id: &PublicKey, user_channel_id: u128) -> Result<(), APIError> { - self.do_accept_inbound_channel(temporary_channel_id, counterparty_node_id, false, user_channel_id, vec![], Weight::from_wu(0)) + self.do_accept_inbound_channel(temporary_channel_id, counterparty_node_id, false, user_channel_id) } /// Accepts a request to open a channel after a [`events::Event::OpenChannelRequest`], treating @@ -7670,13 +7669,13 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ /// [`Event::OpenChannelRequest`]: events::Event::OpenChannelRequest /// [`Event::ChannelClosed::user_channel_id`]: events::Event::ChannelClosed::user_channel_id pub fn accept_inbound_channel_from_trusted_peer_0conf(&self, temporary_channel_id: &ChannelId, counterparty_node_id: &PublicKey, user_channel_id: u128) -> Result<(), APIError> { - self.do_accept_inbound_channel(temporary_channel_id, counterparty_node_id, true, user_channel_id, vec![], Weight::from_wu(0)) + self.do_accept_inbound_channel(temporary_channel_id, counterparty_node_id, true, user_channel_id) } + /// TODO(dual_funding): Allow contributions, pass intended amount and inputs fn do_accept_inbound_channel( &self, temporary_channel_id: &ChannelId, counterparty_node_id: &PublicKey, accept_0conf: bool, - user_channel_id: u128, _funding_inputs: Vec<(TxIn, TransactionU16LenLimited)>, - _total_witness_weight: Weight, + user_channel_id: u128, ) -> Result<(), APIError> { let logger = WithContext::from(&self.logger, Some(*counterparty_node_id), Some(*temporary_channel_id), None); let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); @@ -7726,7 +7725,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ &self.fee_estimator, &self.entropy_source, &self.signer_provider, self.get_our_node_id(), *counterparty_node_id, &self.channel_type_features(), &peer_state.latest_features, - &open_channel_msg, _funding_inputs, _total_witness_weight, + &open_channel_msg, user_channel_id, &self.default_configuration, best_block_height, &self.logger, ).map_err(|_| MsgHandleErrInternal::from_chan_no_close( @@ -8012,7 +8011,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ let channel = PendingV2Channel::new_inbound( &self.fee_estimator, &self.entropy_source, &self.signer_provider, self.get_our_node_id(), *counterparty_node_id, &self.channel_type_features(), - &peer_state.latest_features, msg, vec![], Weight::from_wu(0), user_channel_id, + &peer_state.latest_features, msg, user_channel_id, &self.default_configuration, best_block_height, &self.logger, ).map_err(|e| MsgHandleErrInternal::from_chan_no_close(e, msg.common_fields.temporary_channel_id))?; let message_send_event = events::MessageSendEvent::SendAcceptChannelV2 { diff --git a/lightning/src/ln/dual_funding_tests.rs b/lightning/src/ln/dual_funding_tests.rs index 8cd47ad1765..9f5a811ec48 100644 --- a/lightning/src/ln/dual_funding_tests.rs +++ b/lightning/src/ln/dual_funding_tests.rs @@ -11,30 +11,28 @@ #[cfg(dual_funding)] use { - crate::chain::chaininterface::{ConfirmationTarget, FeeEstimator, LowerBoundedFeeEstimator}, + crate::chain::chaininterface::{ConfirmationTarget, LowerBoundedFeeEstimator}, crate::events::{Event, MessageSendEvent, MessageSendEventsProvider}, crate::ln::chan_utils::{ make_funding_redeemscript, ChannelPublicKeys, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, }, - crate::ln::channel::{ - calculate_our_funding_satoshis, PendingV2Channel, MIN_CHAN_DUST_LIMIT_SATOSHIS, - }, + crate::ln::channel::PendingV2Channel, crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint, RevocationBasepoint}, crate::ln::functional_test_utils::*, crate::ln::msgs::ChannelMessageHandler, crate::ln::msgs::{CommitmentSigned, TxAddInput, TxAddOutput, TxComplete}, crate::ln::types::ChannelId, crate::prelude::*, - crate::sign::{ChannelSigner as _, P2WPKH_WITNESS_WEIGHT}, + crate::sign::ChannelSigner as _, crate::util::ser::TransactionU16LenLimited, crate::util::test_utils, - bitcoin::Weight, }; #[cfg(dual_funding)] // Dual-funding: V2 Channel Establishment Tests struct V2ChannelEstablishmentTestSession { + funding_input_sats: u64, initiator_input_value_satoshis: u64, } @@ -60,17 +58,7 @@ fn do_test_v2_channel_establishment( .collect(); // Alice creates a dual-funded channel as initiator. - let funding_feerate = node_cfgs[0] - .fee_estimator - .get_est_sat_per_1000_weight(ConfirmationTarget::NonAnchorChannelFee); - let funding_satoshis = calculate_our_funding_satoshis( - true, - &initiator_funding_inputs[..], - Weight::from_wu(P2WPKH_WITNESS_WEIGHT), - funding_feerate, - MIN_CHAN_DUST_LIMIT_SATOSHIS, - ) - .unwrap(); + let funding_satoshis = session.funding_input_sats; let mut channel = PendingV2Channel::new_outbound( &LowerBoundedFeeEstimator(node_cfgs[0].fee_estimator), &nodes[0].node.entropy_source, @@ -260,12 +248,18 @@ fn do_test_v2_channel_establishment( fn test_v2_channel_establishment() { // Only initiator contributes, no persist pending do_test_v2_channel_establishment( - V2ChannelEstablishmentTestSession { initiator_input_value_satoshis: 100_000 }, + V2ChannelEstablishmentTestSession { + funding_input_sats: 100_000, + initiator_input_value_satoshis: 150_000, + }, false, ); // Only initiator contributes, persist pending do_test_v2_channel_establishment( - V2ChannelEstablishmentTestSession { initiator_input_value_satoshis: 100_000 }, + V2ChannelEstablishmentTestSession { + funding_input_sats: 100_000, + initiator_input_value_satoshis: 150_000, + }, true, ); }