From b4b3bfb5bf4732dc4b8709ddb98c9c424a2d0775 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 19 Mar 2026 14:52:08 -0700 Subject: [PATCH 1/5] Remove wallet argument from FundingTemplate::splice_out It does not require coin selection, so the wallet argument is not necessary. --- fuzz/src/chanmon_consistency.rs | 20 +--- fuzz/src/full_stack.rs | 10 +- lightning/src/ln/funding.rs | 180 ++++++++++++++++------------- lightning/src/ln/splicing_tests.rs | 11 +- 4 files changed, 112 insertions(+), 109 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index f20f93c789c..07aae78f22f 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -1504,7 +1504,6 @@ pub fn do_test(data: &[u8], out: Out) { counterparty_node_id: &PublicKey, channel_id: &ChannelId, wallet: &TestWalletSource, - logger: Arc, funding_feerate_sat_per_kw: FeeRate| { // We conditionally splice out `MAX_STD_OUTPUT_DUST_LIMIT_SATOSHIS` only when the node // has double the balance required to send a payment upon a `0xff` byte. We do this to @@ -1524,12 +1523,7 @@ pub fn do_test(data: &[u8], out: Out) { value: Amount::from_sat(MAX_STD_OUTPUT_DUST_LIMIT_SATOSHIS), script_pubkey: wallet.get_change_script().unwrap(), }]; - funding_template.splice_out_sync( - outputs, - feerate, - FeeRate::MAX, - &WalletSync::new(wallet, logger.clone()), - ) + funding_template.splice_out(outputs, feerate, FeeRate::MAX) }); }; @@ -2450,30 +2444,26 @@ pub fn do_test(data: &[u8], out: Out) { 0xa4 => { let cp_node_id = nodes[1].get_our_node_id(); let wallet = &wallets[0]; - let logger = Arc::clone(&loggers[0]); let feerate_sat_per_kw = fee_estimators[0].feerate_sat_per_kw(); - splice_out(&nodes[0], &cp_node_id, &chan_a_id, wallet, logger, feerate_sat_per_kw); + splice_out(&nodes[0], &cp_node_id, &chan_a_id, wallet, feerate_sat_per_kw); }, 0xa5 => { let cp_node_id = nodes[0].get_our_node_id(); let wallet = &wallets[1]; - let logger = Arc::clone(&loggers[1]); let feerate_sat_per_kw = fee_estimators[1].feerate_sat_per_kw(); - splice_out(&nodes[1], &cp_node_id, &chan_a_id, wallet, logger, feerate_sat_per_kw); + splice_out(&nodes[1], &cp_node_id, &chan_a_id, wallet, feerate_sat_per_kw); }, 0xa6 => { let cp_node_id = nodes[2].get_our_node_id(); let wallet = &wallets[1]; - let logger = Arc::clone(&loggers[1]); let feerate_sat_per_kw = fee_estimators[1].feerate_sat_per_kw(); - splice_out(&nodes[1], &cp_node_id, &chan_b_id, wallet, logger, feerate_sat_per_kw); + splice_out(&nodes[1], &cp_node_id, &chan_b_id, wallet, feerate_sat_per_kw); }, 0xa7 => { let cp_node_id = nodes[1].get_our_node_id(); let wallet = &wallets[2]; - let logger = Arc::clone(&loggers[2]); let feerate_sat_per_kw = fee_estimators[2].feerate_sat_per_kw(); - splice_out(&nodes[2], &cp_node_id, &chan_b_id, wallet, logger, feerate_sat_per_kw); + splice_out(&nodes[2], &cp_node_id, &chan_b_id, wallet, feerate_sat_per_kw); }, // Sync node by 1 block to cover confirmation of a transaction. diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs index c1d7982e5e4..f300ded4fb7 100644 --- a/fuzz/src/full_stack.rs +++ b/fuzz/src/full_stack.rs @@ -1083,13 +1083,9 @@ pub fn do_test(mut data: &[u8], logger: &Arc value: Amount::from_sat(splice_out_sats), script_pubkey: wallet.get_change_script().unwrap(), }]; - let wallet_sync = WalletSync::new(&wallet, Arc::clone(&logger)); - if let Ok(contribution) = funding_template.splice_out_sync( - outputs, - feerate, - FeeRate::MAX, - &wallet_sync, - ) { + if let Ok(contribution) = + funding_template.splice_out(outputs, feerate, FeeRate::MAX) + { let _ = channelmanager.funding_contributed( &chan_id, &counterparty, diff --git a/lightning/src/ln/funding.rs b/lightning/src/ln/funding.rs index 0a5fb647de1..470e8bc71f1 100644 --- a/lightning/src/ln/funding.rs +++ b/lightning/src/ln/funding.rs @@ -206,10 +206,12 @@ impl PriorContribution { /// For a fresh splice (no pending splice to replace), build a new contribution using one of /// the splice methods: /// - [`FundingTemplate::splice_in_sync`] to add funds to the channel -/// - [`FundingTemplate::splice_out_sync`] to remove funds from the channel +/// - [`FundingTemplate::splice_out`] to remove funds from the channel /// - [`FundingTemplate::splice_in_and_out_sync`] to do both /// -/// These perform coin selection and require `min_feerate` and `max_feerate` parameters. +/// These require `min_feerate` and `max_feerate` parameters. The splice-in variants perform +/// coin selection when wallet inputs are needed, while splice-out spends only from the channel +/// balance. /// /// # Replace By Fee (RBF) /// @@ -287,31 +289,13 @@ macro_rules! build_funding_contribution { let max_feerate: FeeRate = $max_feerate; let force_coin_selection: bool = $force_coin_selection; - if feerate > max_feerate { - return Err(FundingContributionError::FeeRateExceedsMaximum { feerate, max_feerate }); - } - - if let Some(min_rbf_feerate) = min_rbf_feerate { - if feerate < min_rbf_feerate { - return Err(FundingContributionError::FeeRateBelowRbfMinimum { feerate, min_rbf_feerate }); - } - } - - // Validate user-provided amounts are within MAX_MONEY before coin selection to - // ensure FundingContribution::net_value() arithmetic cannot overflow. With all - // amounts bounded by MAX_MONEY (~2.1e15 sat), the worst-case net_value() - // computation is -2 * MAX_MONEY (~-4.2e15), well within i64::MIN (~-9.2e18). - if value_added > Amount::MAX_MONEY { - return Err(FundingContributionError::InvalidSpliceValue); - } - - let mut value_removed = Amount::ZERO; - for txout in outputs.iter() { - value_removed = match value_removed.checked_add(txout.value) { - Some(sum) if sum <= Amount::MAX_MONEY => sum, - _ => return Err(FundingContributionError::InvalidSpliceValue), - }; - } + let value_removed = validate_funding_contribution_params( + value_added, + &outputs, + min_rbf_feerate, + feerate, + max_feerate, + )?; let is_splice = shared_input.is_some(); @@ -350,25 +334,52 @@ macro_rules! build_funding_contribution { let CoinSelection { confirmed_utxos: inputs, change_output } = coin_selection; - // The caller creating a FundingContribution is always the initiator for fee estimation - // purposes — this is conservative, overestimating rather than underestimating fees if - // the node ends up as the acceptor. - let estimated_fee = estimate_transaction_fee(&inputs, &outputs, change_output.as_ref(), true, is_splice, feerate); - debug_assert!(estimated_fee <= Amount::MAX_MONEY); - - let contribution = FundingContribution { + Ok(FundingContribution::new( value_added, - estimated_fee, - inputs, outputs, + inputs, change_output, feerate, max_feerate, is_splice, + )) + }}; +} + +fn validate_funding_contribution_params( + value_added: Amount, outputs: &[TxOut], min_rbf_feerate: Option, feerate: FeeRate, + max_feerate: FeeRate, +) -> Result { + if feerate > max_feerate { + return Err(FundingContributionError::FeeRateExceedsMaximum { feerate, max_feerate }); + } + + if let Some(min_rbf_feerate) = min_rbf_feerate { + if feerate < min_rbf_feerate { + return Err(FundingContributionError::FeeRateBelowRbfMinimum { + feerate, + min_rbf_feerate, + }); + } + } + + // Validate user-provided amounts are within MAX_MONEY before coin selection to + // ensure FundingContribution::net_value() arithmetic cannot overflow. With all + // amounts bounded by MAX_MONEY (~2.1e15 sat), the worst-case net_value() + // computation is -2 * MAX_MONEY (~-4.2e15), well within i64::MIN (~-9.2e18). + if value_added > Amount::MAX_MONEY { + return Err(FundingContributionError::InvalidSpliceValue); + } + + let mut value_removed = Amount::ZERO; + for txout in outputs.iter() { + value_removed = match value_removed.checked_add(txout.value) { + Some(sum) if sum <= Amount::MAX_MONEY => sum, + _ => return Err(FundingContributionError::InvalidSpliceValue), }; + } - Ok(contribution) - }}; + Ok(value_removed) } impl FundingTemplate { @@ -422,54 +433,37 @@ impl FundingTemplate { ) } - /// Creates a [`FundingContribution`] for removing funds from a channel using `wallet` to - /// perform coin selection. + /// Creates a [`FundingContribution`] for removing funds from a channel. + /// + /// Fees are paid from the channel balance, so this does not perform coin selection or spend + /// wallet inputs. /// /// `outputs` are the complete set of withdrawal outputs for this contribution. When /// replacing a prior contribution via RBF, use [`FundingTemplate::prior_contribution`] to /// inspect the prior parameters. To keep existing withdrawals and add new ones, include the /// prior's outputs: combine [`FundingContribution::outputs`] with the new outputs. - pub async fn splice_out( - self, outputs: Vec, min_feerate: FeeRate, max_feerate: FeeRate, wallet: W, + pub fn splice_out( + self, outputs: Vec, min_feerate: FeeRate, max_feerate: FeeRate, ) -> Result { if outputs.is_empty() { return Err(FundingContributionError::InvalidSpliceValue); } - let FundingTemplate { shared_input, min_rbf_feerate, .. } = self; - build_funding_contribution!( + validate_funding_contribution_params( Amount::ZERO, - outputs, - shared_input, - min_rbf_feerate, + &outputs, + self.min_rbf_feerate, min_feerate, max_feerate, - false, - wallet, - await - ) - } - - /// Creates a [`FundingContribution`] for removing funds from a channel using `wallet` to - /// perform coin selection. - /// - /// See [`FundingTemplate::splice_out`] for details. - pub fn splice_out_sync( - self, outputs: Vec, min_feerate: FeeRate, max_feerate: FeeRate, wallet: W, - ) -> Result { - if outputs.is_empty() { - return Err(FundingContributionError::InvalidSpliceValue); - } - let FundingTemplate { shared_input, min_rbf_feerate, .. } = self; - build_funding_contribution!( + )?; + Ok(FundingContribution::new( Amount::ZERO, outputs, - shared_input, - min_rbf_feerate, + vec![], + None, min_feerate, max_feerate, - false, - wallet, - ) + self.shared_input.is_some(), + )) } /// Creates a [`FundingContribution`] for both adding and removing funds from a channel using @@ -760,6 +754,35 @@ impl_writeable_tlv_based!(FundingContribution, { }); impl FundingContribution { + fn new( + value_added: Amount, outputs: Vec, inputs: Vec, + change_output: Option, feerate: FeeRate, max_feerate: FeeRate, is_splice: bool, + ) -> Self { + // The caller creating a FundingContribution is always the initiator for fee estimation + // purposes — this is conservative, overestimating rather than underestimating fees if the + // node ends up as the acceptor. + let estimated_fee = estimate_transaction_fee( + &inputs, + &outputs, + change_output.as_ref(), + true, + is_splice, + feerate, + ); + debug_assert!(estimated_fee <= Amount::MAX_MONEY); + + Self { + value_added, + estimated_fee, + inputs, + outputs, + change_output, + feerate, + max_feerate, + is_splice, + } + } + pub(super) fn feerate(&self) -> FeeRate { self.feerate } @@ -1492,17 +1515,17 @@ mod tests { )); } - // splice_out_sync with single output value > MAX_MONEY + // splice_out with single output value > MAX_MONEY { let template = FundingTemplate::new(None, None, None); let outputs = vec![funding_output_sats(over_max.to_sat())]; assert!(matches!( - template.splice_out_sync(outputs, feerate, feerate, UnreachableWallet), + template.splice_out(outputs, feerate, feerate), Err(FundingContributionError::InvalidSpliceValue), )); } - // splice_out_sync with multiple outputs summing > MAX_MONEY + // splice_out with multiple outputs summing > MAX_MONEY { let template = FundingTemplate::new(None, None, None); let half_over = Amount::MAX_MONEY / 2 + Amount::from_sat(1); @@ -1511,7 +1534,7 @@ mod tests { funding_output_sats(half_over.to_sat()), ]; assert!(matches!( - template.splice_out_sync(outputs, feerate, feerate, UnreachableWallet), + template.splice_out(outputs, feerate, feerate), Err(FundingContributionError::InvalidSpliceValue), )); } @@ -2506,7 +2529,7 @@ mod tests { } #[test] - fn test_splice_out_sync_skips_coin_selection_during_rbf() { + fn test_splice_out_skips_coin_selection_during_rbf() { // When splice_out_sync is called on a template with min_rbf_feerate set (user // choosing a fresh splice-out instead of rbf_sync), coin selection should NOT run. // Fees come from the channel balance. @@ -2517,12 +2540,11 @@ mod tests { let template = FundingTemplate::new(Some(shared_input(100_000)), Some(min_rbf_feerate), None); - // UnreachableWallet panics if coin selection runs — verifying it is skipped. - let contribution = template - .splice_out_sync(vec![withdrawal.clone()], feerate, FeeRate::MAX, UnreachableWallet) - .unwrap(); + let contribution = + template.splice_out(vec![withdrawal.clone()], feerate, FeeRate::MAX).unwrap(); assert_eq!(contribution.value_added, Amount::ZERO); assert!(contribution.inputs.is_empty()); + assert!(contribution.change_output.is_none()); assert_eq!(contribution.outputs, vec![withdrawal]); } } diff --git a/lightning/src/ln/splicing_tests.rs b/lightning/src/ln/splicing_tests.rs index 9adccd17627..54929214ab6 100644 --- a/lightning/src/ln/splicing_tests.rs +++ b/lightning/src/ln/splicing_tests.rs @@ -271,9 +271,7 @@ pub fn initiate_splice_out<'a, 'b, 'c, 'd>( let floor_feerate = FeeRate::from_sat_per_kwu(FEERATE_FLOOR_SATS_PER_KW as u64); let funding_template = initiator.node.splice_channel(&channel_id, &node_id_acceptor).unwrap(); let feerate = funding_template.min_rbf_feerate().unwrap_or(floor_feerate); - let wallet = WalletSync::new(Arc::clone(&initiator.wallet_source), initiator.logger); - let funding_contribution = - funding_template.splice_out_sync(outputs, feerate, FeeRate::MAX, &wallet).unwrap(); + let funding_contribution = funding_template.splice_out(outputs, feerate, FeeRate::MAX).unwrap(); match initiator.node.funding_contributed( &channel_id, &node_id_acceptor, @@ -1370,9 +1368,8 @@ fn fails_initiating_concurrent_splices(reconnect: bool) { let feerate = FeeRate::from_sat_per_kwu(FEERATE_FLOOR_SATS_PER_KW as u64); let funding_template = nodes[0].node.splice_channel(&channel_id, &node_1_id).unwrap(); - let wallet = WalletSync::new(Arc::clone(&nodes[0].wallet_source), nodes[0].logger); let funding_contribution = - funding_template.splice_out_sync(outputs.clone(), feerate, FeeRate::MAX, &wallet).unwrap(); + funding_template.splice_out(outputs.clone(), feerate, FeeRate::MAX).unwrap(); nodes[0] .node .funding_contributed(&channel_id, &node_1_id, funding_contribution.clone(), None) @@ -6420,9 +6417,7 @@ fn test_splice_revalidation_at_quiescence() { let feerate = FeeRate::from_sat_per_kwu(FEERATE_FLOOR_SATS_PER_KW as u64); let funding_template = nodes[0].node.splice_channel(&channel_id, &node_id_1).unwrap(); - let wallet = WalletSync::new(Arc::clone(&nodes[0].wallet_source), nodes[0].logger); - let contribution = - funding_template.splice_out_sync(outputs, feerate, FeeRate::MAX, &wallet).unwrap(); + let contribution = funding_template.splice_out(outputs, feerate, FeeRate::MAX).unwrap(); nodes[0].node.funding_contributed(&channel_id, &node_id_1, contribution.clone(), None).unwrap(); assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty(), "stfu should be delayed"); From 92c529c5802bae38649cb7838a247bc0bd2d2b8a Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 9 Apr 2026 11:42:06 -0700 Subject: [PATCH 2/5] Make PriorContribution::holder_balance non-optional The `holder_balance` is computed by `FundedChannel::get_holder_counterparty_balances_floor_incl_fee`, which may unexpectedly fail due to the balance either being too high or too low. These cases are highly unlikely to happen given we have validation to ensure we never enter such a state to begin with. If they were to happen, something has gone wrong with the channel and it doesn't make sense to allow splicing anyway. Therefore, we opt to make `PriorContribution::holder_balance` non-optional and return an error that the channel cannot be spliced at the moment. --- lightning/src/ln/channel.rs | 35 ++++++++++++----------- lightning/src/ln/funding.rs | 55 +++++++++++++++++-------------------- 2 files changed, 44 insertions(+), 46 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 32c0e94bdc8..b99b2a19667 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -12324,7 +12324,25 @@ where ); let min_rbf_feerate = prev_feerate.map(min_rbf_feerate); let prior = if pending_splice.last_funding_feerate_sat_per_1000_weight.is_some() { - self.build_prior_contribution() + if let Some(prior) = self + .pending_splice + .as_ref() + .and_then(|pending_splice| pending_splice.contributions.last()) + { + let holder_balance = self + .get_holder_counterparty_balances_floor_incl_fee(&self.funding) + .map(|(h, _)| h) + .map_err(|e| APIError::ChannelUnavailable { + err: format!( + "Channel {} cannot be spliced at this time: {}", + self.context.channel_id(), + e + ), + })?; + Some(PriorContribution::new(prior.clone(), holder_balance)) + } else { + None + } } else { None }; @@ -12346,21 +12364,6 @@ where Ok(FundingTemplate::new(Some(shared_input), min_rbf_feerate, prior_contribution)) } - /// Clones the prior contribution and fetches the holder balance for deferred feerate - /// adjustment. - fn build_prior_contribution(&self) -> Option { - debug_assert!( - self.pending_splice.is_some(), - "build_prior_contribution requires pending_splice" - ); - let prior = self.pending_splice.as_ref()?.contributions.last()?; - let holder_balance = self - .get_holder_counterparty_balances_floor_incl_fee(&self.funding) - .map(|(h, _)| h) - .ok(); - Some(PriorContribution::new(prior.clone(), holder_balance)) - } - /// Returns whether this channel can ever RBF, independent of splice state. fn is_rbf_compatible(&self) -> Result<(), String> { if self.context.minimum_depth(&self.funding) == Some(0) { diff --git a/lightning/src/ln/funding.rs b/lightning/src/ln/funding.rs index 470e8bc71f1..6341b104dbb 100644 --- a/lightning/src/ln/funding.rs +++ b/lightning/src/ln/funding.rs @@ -174,8 +174,7 @@ impl core::fmt::Display for FundingContributionError { #[derive(Debug, Clone, PartialEq, Eq)] pub(super) struct PriorContribution { contribution: FundingContribution, - /// The holder's balance, used for feerate adjustment. `None` when the balance computation - /// fails, in which case adjustment is skipped and coin selection is re-run. + /// The holder's balance, used for feerate adjustment. /// /// This value is captured at [`ChannelManager::splice_channel`] time and may become stale /// if balances change before the contribution is used. Staleness is acceptable here because @@ -186,11 +185,11 @@ pub(super) struct PriorContribution { /// /// [`ChannelManager::splice_channel`]: crate::ln::channelmanager::ChannelManager::splice_channel /// [`ChannelManager::funding_contributed`]: crate::ln::channelmanager::ChannelManager::funding_contributed - holder_balance: Option, + holder_balance: Amount, } impl PriorContribution { - pub(super) fn new(contribution: FundingContribution, holder_balance: Option) -> Self { + pub(super) fn new(contribution: FundingContribution, holder_balance: Amount) -> Self { Self { contribution, holder_balance } } } @@ -562,17 +561,15 @@ impl FundingTemplate { // buffer is insufficient (splice-in), or if the prior's feerate is already // above rbf_feerate (e.g., from a counterparty-initiated RBF that locked // at a higher feerate). In all cases, fall through to re-run coin selection. - if let Some(holder_balance) = holder_balance { - if contribution - .net_value_for_initiator_at_feerate(rbf_feerate, holder_balance) - .is_ok() - { - let mut adjusted = contribution - .for_initiator_at_feerate(rbf_feerate, holder_balance) - .expect("feerate compatibility already checked"); - adjusted.max_feerate = max_feerate; - return Ok(adjusted); - } + if contribution + .net_value_for_initiator_at_feerate(rbf_feerate, holder_balance) + .is_ok() + { + let mut adjusted = contribution + .for_initiator_at_feerate(rbf_feerate, holder_balance) + .expect("feerate compatibility already checked"); + adjusted.max_feerate = max_feerate; + return Ok(adjusted); } build_funding_contribution!( contribution.value_added, @@ -620,17 +617,15 @@ impl FundingTemplate { match prior_contribution { Some(PriorContribution { contribution, holder_balance }) => { // See comment in `rbf` for details on when this adjustment fails. - if let Some(holder_balance) = holder_balance { - if contribution - .net_value_for_initiator_at_feerate(rbf_feerate, holder_balance) - .is_ok() - { - let mut adjusted = contribution - .for_initiator_at_feerate(rbf_feerate, holder_balance) - .expect("feerate compatibility already checked"); - adjusted.max_feerate = max_feerate; - return Ok(adjusted); - } + if contribution + .net_value_for_initiator_at_feerate(rbf_feerate, holder_balance) + .is_ok() + { + let mut adjusted = contribution + .for_initiator_at_feerate(rbf_feerate, holder_balance) + .expect("feerate compatibility already checked"); + adjusted.max_feerate = max_feerate; + return Ok(adjusted); } build_funding_contribution!( contribution.value_added, @@ -2355,7 +2350,7 @@ mod tests { let template = FundingTemplate::new( None, Some(min_rbf_feerate), - Some(PriorContribution::new(prior, None)), + Some(PriorContribution::new(prior, Amount::MAX)), ); assert!(matches!( template.rbf_sync(max_feerate, UnreachableWallet), @@ -2390,7 +2385,7 @@ mod tests { let template = FundingTemplate::new( None, Some(min_rbf_feerate), - Some(PriorContribution::new(prior, Some(Amount::MAX))), + Some(PriorContribution::new(prior, Amount::MAX)), ); let contribution = template.rbf_sync(max_feerate, UnreachableWallet).unwrap(); assert_eq!(contribution.feerate, min_rbf_feerate); @@ -2452,7 +2447,7 @@ mod tests { let template = FundingTemplate::new( Some(shared_input(100_000)), Some(min_rbf_feerate), - Some(PriorContribution::new(prior, None)), + Some(PriorContribution::new(prior, Amount::ZERO)), ); let wallet = SingleUtxoWallet { @@ -2513,7 +2508,7 @@ mod tests { let template = FundingTemplate::new( Some(shared_input(100_000)), Some(min_rbf_feerate), - Some(PriorContribution::new(prior, None)), + Some(PriorContribution::new(prior, Amount::MAX)), ); let wallet = SingleUtxoWallet { From 2c21fdb7bab5399869fb82782547463961cc9348 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Fri, 3 Apr 2026 10:53:28 -0700 Subject: [PATCH 3/5] Derive FundingContribution::net_value implicitly This commit removes `FundingContribution::value_added` as tracking it is unnecessary -- it can just be derived from the total amount in minus total amount out minus fees. Doing so also highlighted that there was an incorrect assumption in how feerates are computed/adjusted. Ultimately, we don't care whether a contribution has inputs or not. Instead, we look at whether it's contributing a positive or negative amount to the channel, and enforce different constraints for each. --- fuzz/src/full_stack.rs | 18 +- lightning/src/ln/funding.rs | 658 +++++++++++------------------ lightning/src/ln/splicing_tests.rs | 39 +- 3 files changed, 259 insertions(+), 456 deletions(-) diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs index f300ded4fb7..1f1cf425c92 100644 --- a/fuzz/src/full_stack.rs +++ b/fuzz/src/full_stack.rs @@ -1886,8 +1886,8 @@ fn splice_seed() -> Vec { // CommitmentSigned message with proper signature (r=f7, s=01...) and funding_txid TLV // signature r encodes sighash first byte f7, s follows the pattern from funding_created // TLV type 1 (odd/optional) for funding_txid as per impl_writeable_msg!(CommitmentSigned, ...) - // Note: txid is encoded in reverse byte order (Bitcoin standard), so to get display 0000...0033, encode 3300...0000 - ext_from_hex("0084 c000000000000000000000000000000000000000000000000000000000000000 00000000000000000000000000000000000000000000000000000000000000f7 0100000000000000000000000000000000000000000000000000000000000000 0000 01 20 3300000000000000000000000000000000000000000000000000000000000000 03000000000000000000000000000000", &mut test); + // Note: txid is encoded in reverse byte order (Bitcoin standard), so to get display 0000...0031, encode 3100...0000 + ext_from_hex("0084 c000000000000000000000000000000000000000000000000000000000000000 00000000000000000000000000000000000000000000000000000000000000f7 0100000000000000000000000000000000000000000000000000000000000000 0000 01 20 3100000000000000000000000000000000000000000000000000000000000000 03000000000000000000000000000000", &mut test); // After commitment_signed exchange, we need to exchange tx_signatures. // Message type IDs: TxSignatures = 71 (0x0047) @@ -1900,19 +1900,19 @@ fn splice_seed() -> Vec { // inbound read from peer id 0 of len 150 (134 message + 16 MAC) ext_from_hex("030096", &mut test); // TxSignatures message with shared_input_signature TLV (type 0) - // txid must match the splice funding txid (0x33 in reverse byte order) + // txid must match the splice funding txid (0x31 in reverse byte order) // shared_input_signature: 64-byte fuzz signature for the shared input - ext_from_hex("0047 c000000000000000000000000000000000000000000000000000000000000000 3300000000000000000000000000000000000000000000000000000000000000 0000 00 40 00000000000000000000000000000000000000000000000000000000000000dc 0100000000000000000000000000000000000000000000000000000000000000 03000000000000000000000000000000", &mut test); + ext_from_hex("0047 c000000000000000000000000000000000000000000000000000000000000000 3100000000000000000000000000000000000000000000000000000000000000 0000 00 40 00000000000000000000000000000000000000000000000000000000000000dc 0100000000000000000000000000000000000000000000000000000000000000 03000000000000000000000000000000", &mut test); // Connect a block with the splice funding transaction to confirm it // The splice funding tx: version(4) + input_count(1) + txid(32) + vout(4) + script_len(1) + sequence(4) // + output_count(1) + value(8) + script_len(1) + script(34) + locktime(4) = 94 bytes = 0x5e // Transaction structure from FundingTransactionReadyForSigning: // - Input: spending c000...00:0 with sequence 0xfffffffd - // - Output: 115536 sats to OP_0 PUSH32 6e00...00 + // - Output: 115538 sats to OP_0 PUSH32 6e00...00 // - Locktime: 13 ext_from_hex("0c005e", &mut test); - ext_from_hex("02000000 01 c000000000000000000000000000000000000000000000000000000000000000 00000000 00 fdffffff 01 50c3010000000000 22 00206e00000000000000000000000000000000000000000000000000000000000000 0d000000", &mut test); + ext_from_hex("02000000 01 c000000000000000000000000000000000000000000000000000000000000000 00000000 00 fdffffff 01 52c3010000000000 22 00206e00000000000000000000000000000000000000000000000000000000000000 0d000000", &mut test); // Connect additional blocks to reach minimum_depth confirmations for _ in 0..5 { @@ -1929,8 +1929,8 @@ fn splice_seed() -> Vec { // inbound read from peer id 0 of len 82 (66 message + 16 MAC) ext_from_hex("030052", &mut test); // SpliceLocked message (type 77 = 0x004d): channel_id + splice_txid + mac - // splice_txid must match the splice funding txid (0x33 in reverse byte order) - ext_from_hex("004d c000000000000000000000000000000000000000000000000000000000000000 3300000000000000000000000000000000000000000000000000000000000000 03000000000000000000000000000000", &mut test); + // splice_txid must match the splice funding txid (0x31 in reverse byte order) + ext_from_hex("004d c000000000000000000000000000000000000000000000000000000000000000 3100000000000000000000000000000000000000000000000000000000000000 03000000000000000000000000000000", &mut test); test } @@ -2060,6 +2060,6 @@ mod tests { // Splice locked assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Handling SendSpliceLocked event in peer_handler for node 030000000000000000000000000000000000000000000000000000000000000002 for channel c000000000000000000000000000000000000000000000000000000000000000".to_string())), Some(&1)); - assert_eq!(log_entries.get(&("lightning::ln::channel".to_string(), "Promoting splice funding txid 0000000000000000000000000000000000000000000000000000000000000033".to_string())), Some(&1)); + assert_eq!(log_entries.get(&("lightning::ln::channel".to_string(), "Promoting splice funding txid 0000000000000000000000000000000000000000000000000000000000000031".to_string())), Some(&1)); } } diff --git a/lightning/src/ln/funding.rs b/lightning/src/ln/funding.rs index 6341b104dbb..62499871e34 100644 --- a/lightning/src/ln/funding.rs +++ b/lightning/src/ln/funding.rs @@ -334,7 +334,6 @@ macro_rules! build_funding_contribution { let CoinSelection { confirmed_utxos: inputs, change_output } = coin_selection; Ok(FundingContribution::new( - value_added, outputs, inputs, change_output, @@ -455,7 +454,6 @@ impl FundingTemplate { max_feerate, )?; Ok(FundingContribution::new( - Amount::ZERO, outputs, vec![], None, @@ -572,7 +570,7 @@ impl FundingTemplate { return Ok(adjusted); } build_funding_contribution!( - contribution.value_added, + contribution.value_added(), contribution.outputs, shared_input, min_rbf_feerate, @@ -628,7 +626,7 @@ impl FundingTemplate { return Ok(adjusted); } build_funding_contribution!( - contribution.value_added, + contribution.value_added(), contribution.outputs, shared_input, min_rbf_feerate, @@ -707,12 +705,6 @@ fn estimate_transaction_fee( /// The components of a funding transaction contributed by one party. #[derive(Debug, Clone, PartialEq, Eq)] pub struct FundingContribution { - /// The amount to contribute to the channel. - /// - /// If `value_added` is [`Amount::ZERO`], then any fees will be deducted from the channel - /// balance instead of paid by `inputs`. - value_added: Amount, - /// The estimate fees responsible to be paid for the contribution. estimated_fee: Amount, @@ -738,20 +730,19 @@ pub struct FundingContribution { } impl_writeable_tlv_based!(FundingContribution, { - (1, value_added, required), - (3, estimated_fee, required), - (5, inputs, optional_vec), - (7, outputs, optional_vec), - (9, change_output, option), - (11, feerate, required), - (13, max_feerate, required), - (15, is_splice, required), + (1, estimated_fee, required), + (3, inputs, optional_vec), + (5, outputs, optional_vec), + (7, change_output, option), + (9, feerate, required), + (11, max_feerate, required), + (13, is_splice, required), }); impl FundingContribution { fn new( - value_added: Amount, outputs: Vec, inputs: Vec, - change_output: Option, feerate: FeeRate, max_feerate: FeeRate, is_splice: bool, + outputs: Vec, inputs: Vec, change_output: Option, + feerate: FeeRate, max_feerate: FeeRate, is_splice: bool, ) -> Self { // The caller creating a FundingContribution is always the initiator for fee estimation // purposes — this is conservative, overestimating rather than underestimating fees if the @@ -766,16 +757,7 @@ impl FundingContribution { ); debug_assert!(estimated_fee <= Amount::MAX_MONEY); - Self { - value_added, - estimated_fee, - inputs, - outputs, - change_output, - feerate, - max_feerate, - is_splice, - } + Self { estimated_fee, inputs, outputs, change_output, feerate, max_feerate, is_splice } } pub(super) fn feerate(&self) -> FeeRate { @@ -794,9 +776,18 @@ impl FundingContribution { self.outputs.iter().chain(self.change_output.iter()) } - /// Returns the amount added to the channel by this contribution. + /// The value that will be added to the channel after fees. See [`Self::net_value`] for the net + /// value contribution to the channel. pub fn value_added(&self) -> Amount { - self.value_added + let total_input_value = self.inputs.iter().map(|i| i.utxo.output.value).sum::(); + total_input_value + .checked_sub(self.estimated_fee) + .and_then(|v| { + v.checked_sub( + self.change_output.as_ref().map_or(Amount::ZERO, |output| output.value), + ) + }) + .unwrap_or(Amount::ZERO) } /// Returns the outputs (e.g., withdrawal destinations) included in this contribution. @@ -871,38 +862,6 @@ impl FundingContribution { } } - // Fees for splice-out are paid from the channel balance whereas fees for splice-in - // are paid by the funding inputs. Therefore, in the case of splice-out, we add the - // fees on top of the user-specified contribution. We leave the user-specified - // contribution as-is for splice-ins. - if !self.inputs.is_empty() { - let mut total_input_value = Amount::ZERO; - for FundingTxInput { utxo, .. } in self.inputs.iter() { - total_input_value = total_input_value - .checked_add(utxo.output.value) - .ok_or("Sum of input values is greater than the total bitcoin supply")?; - } - - // If the inputs are enough to cover intended contribution amount plus fees (which - // include the change output weight when present), we are fine. - // If the inputs are less, but enough to cover intended contribution amount with - // (lower) fees without change, we are also fine (change will not be generated). - // Since estimated_fee includes change weight, this check is conservative. - // - // Note: dust limit is not relevant in this check. - - let contributed_input_value = self.value_added; - let estimated_fee = self.estimated_fee; - let minimal_input_amount_needed = contributed_input_value - .checked_add(estimated_fee) - .ok_or(format!("{contributed_input_value} contribution plus {estimated_fee} fee estimate exceeds the total bitcoin supply"))?; - if total_input_value < minimal_input_amount_needed { - return Err(format!( - "Total input amount {total_input_value} is lower than needed for splice-in contribution {contributed_input_value}, considering fees of {estimated_fee}. Need more inputs.", - )); - } - } - Ok(()) } @@ -951,54 +910,36 @@ impl FundingContribution { } } - if !self.inputs.is_empty() { - if let Some(ref change_output) = self.change_output { - let old_change_value = change_output.value; - let dust_limit = change_output.script_pubkey.minimal_non_dust(); + let target_fee = estimate_transaction_fee( + &self.inputs, + &self.outputs, + self.change_output.as_ref(), + is_initiator, + self.is_splice, + target_feerate, + ); - // Target fee including the change output's weight. - let target_fee = estimate_transaction_fee( - &self.inputs, - &self.outputs, - self.change_output.as_ref(), - is_initiator, - self.is_splice, - target_feerate, - ); + let net_value_without_fee = self.net_value_without_fee(); + if net_value_without_fee.is_positive() { + let fee_buffer = self + .estimated_fee + .checked_add( + self.change_output.as_ref().map_or(Amount::ZERO, |output| output.value), + ) + .ok_or(FeeRateAdjustmentError::FeeBufferOverflow)?; - let fee_buffer = self - .estimated_fee - .checked_add(old_change_value) - .ok_or(FeeRateAdjustmentError::FeeBufferOverflow)?; - - match fee_buffer.checked_sub(target_fee) { - Some(new_change_value) if new_change_value >= dust_limit => { - Ok((target_fee, Some(new_change_value))) - }, - _ => { - // Change would be below dust or negative. Try without change. - let target_fee_no_change = estimate_transaction_fee( - &self.inputs, - &self.outputs, - None, - is_initiator, - self.is_splice, - target_feerate, - ); - if target_fee_no_change > fee_buffer { - Err(FeeRateAdjustmentError::FeeBufferInsufficient { - source: "estimated fee + change value", - available: fee_buffer, - required: target_fee_no_change, - }) - } else { - Ok((target_fee_no_change, None)) - } - }, + if let Some(change_output) = self.change_output.as_ref() { + let dust_limit = change_output.script_pubkey.minimal_non_dust(); + if let Some(new_change_value) = fee_buffer.checked_sub(target_fee) { + if new_change_value >= dust_limit { + return Ok((target_fee, Some(new_change_value))); + } + + // Our remaining change was not enough to be a valid output, fallthrough to the + // no remaining change case. } - } else { - // No change output. - let target_fee = estimate_transaction_fee( + + let target_fee_no_change = estimate_transaction_fee( &self.inputs, &self.outputs, None, @@ -1006,50 +947,39 @@ impl FundingContribution { self.is_splice, target_feerate, ); - // The fee buffer is total input value minus value_added and output values. - // This is estimated_fee plus the coin selection surplus (dust burned to - // fees), ensuring we never silently reduce value_added beyond the small - // surplus from coin selection. - let total_input_value: Amount = - self.inputs.iter().map(|i| i.utxo.output.value).sum(); - let output_values: Amount = self.outputs.iter().map(|o| o.value).sum(); - let fee_buffer = total_input_value - .checked_sub(self.value_added) - .and_then(|v| v.checked_sub(output_values)) - .ok_or(FeeRateAdjustmentError::FeeBufferOverflow)?; - if target_fee > fee_buffer { - return Err(FeeRateAdjustmentError::FeeBufferInsufficient { - source: "estimated fee + coin selection surplus", + if target_fee_no_change > fee_buffer { + Err(FeeRateAdjustmentError::FeeBufferInsufficient { + source: "estimated fee + change value", available: fee_buffer, - required: target_fee, - }); + required: target_fee_no_change, + }) + } else { + Ok((target_fee_no_change, None)) } + } else if let Some(_surplus) = fee_buffer.checked_sub(target_fee) { Ok((target_fee, None)) + } else { + Err(FeeRateAdjustmentError::FeeBufferInsufficient { + source: "estimated fee", + available: fee_buffer, + required: target_fee, + }) } } else { - // No inputs (splice-out): fees paid from channel balance. - let target_fee = estimate_transaction_fee( - &[], - &self.outputs, - None, - is_initiator, - self.is_splice, - target_feerate, - ); - - // Check that the channel balance can cover the withdrawal outputs plus fees. - let value_removed: Amount = self.outputs.iter().map(|o| o.value).sum(); + // Check that the channel balance can cover the negative contribution. If there is a + // surplus, it implicitly goes back to the channel balance. let total_cost = target_fee - .checked_add(value_removed) + .checked_add(net_value_without_fee.unsigned_abs()) .ok_or(FeeRateAdjustmentError::FeeBufferOverflow)?; if total_cost > holder_balance { return Err(FeeRateAdjustmentError::FeeBufferInsufficient { - source: "channel balance - withdrawal outputs", - available: holder_balance.checked_sub(value_removed).unwrap_or(Amount::ZERO), + source: "channel balance", + available: holder_balance + .checked_sub(net_value_without_fee.unsigned_abs()) + .unwrap_or(Amount::ZERO), required: target_fee, }); } - // Surplus goes back to the channel balance. Ok((target_fee, None)) } } @@ -1062,12 +992,10 @@ impl FundingContribution { ) -> Result { let (new_estimated_fee, new_change) = self.compute_feerate_adjustment(feerate, holder_balance, is_initiator)?; - let surplus = self.fee_buffer_surplus(new_estimated_fee, &new_change); match new_change { Some(value) => self.change_output.as_mut().unwrap().value = value, None => self.change_output = None, } - self.value_added += surplus; self.estimated_fee = new_estimated_fee; self.feerate = feerate; Ok(self) @@ -1106,15 +1034,28 @@ impl FundingContribution { ) -> Result { let (new_estimated_fee, new_change) = self.compute_feerate_adjustment(target_feerate, holder_balance, is_initiator)?; - let surplus = self - .fee_buffer_surplus(new_estimated_fee, &new_change) + + let prev_fee = self + .estimated_fee + .to_signed() + .expect("total input amount cannot exceed Amount::MAX_MONEY"); + let prev_change = self + .change_output + .as_ref() + .map_or(Amount::ZERO, |output| output.value) .to_signed() - .expect("surplus does not exceed Amount::MAX_MONEY"); - let net_value = self - .net_value_with_fee(new_estimated_fee) - .checked_add(surplus) - .expect("net_value + surplus does not overflow"); - Ok(net_value) + .expect("total input amount cannot exceed Amount::MAX_MONEY"); + + let new_fee = new_estimated_fee + .to_signed() + .expect("total input amount cannot exceed Amount::MAX_MONEY"); + let new_change = new_change + .unwrap_or(Amount::ZERO) + .to_signed() + .expect("total input amount cannot exceed Amount::MAX_MONEY"); + + let prev_net_value = self.net_value(); + Ok(prev_net_value + prev_fee + prev_change - new_fee - new_change) } /// Returns the net value at the given target feerate without mutating `self`, @@ -1133,55 +1074,35 @@ impl FundingContribution { self.net_value_at_feerate(target_feerate, holder_balance, true) } - /// Returns the fee buffer surplus when a change output is removed. - /// - /// The fee buffer is the actual amount available for fees from inputs: total input value - /// minus value_added and output values. This includes both the weight-based estimated_fee - /// and any coin selection surplus (dust burned to fees). When the change output is removed, - /// the fee buffer may exceed the new fee; the surplus is returned so it can be redirected - /// to value_added rather than being burned as excess fees. - /// - /// Returns [`Amount::ZERO`] when there are no inputs or the change output is kept. - fn fee_buffer_surplus(&self, new_estimated_fee: Amount, new_change: &Option) -> Amount { - if !self.inputs.is_empty() && new_change.is_none() { - let total_input_value: Amount = self.inputs.iter().map(|i| i.utxo.output.value).sum(); - let output_values: Amount = self.outputs.iter().map(|o| o.value).sum(); - let fee_buffer = total_input_value - self.value_added - output_values; - debug_assert!(fee_buffer >= new_estimated_fee); - fee_buffer - new_estimated_fee - } else { - Amount::ZERO - } - } - - /// The net value contributed to a channel by the splice. If negative, more value will be - /// spliced out than spliced in. Fees will be deducted from the expected splice-out amount - /// if no inputs were included. + /// The net value contributed to a channel by the splice. pub fn net_value(&self) -> SignedAmount { - self.net_value_with_fee(self.estimated_fee) + let estimated_fee = self + .estimated_fee + .to_signed() + .expect("total_input_value is validated to not exceed Amount::MAX_MONEY"); + self.net_value_without_fee() + .checked_sub(estimated_fee) + .expect("all amounts are validated to not exceed Amount::MAX_MONEY") } - /// Computes the net value using the given `estimated_fee` for the splice-out (no inputs) - /// case. For splice-in, fees are paid by inputs so `estimated_fee` is not deducted. - fn net_value_with_fee(&self, estimated_fee: Amount) -> SignedAmount { - let unpaid_fees = if self.inputs.is_empty() { estimated_fee } else { Amount::ZERO } - .to_signed() - .expect("estimated_fee is validated to not exceed Amount::MAX_MONEY"); - let value_added = self - .value_added + fn net_value_without_fee(&self) -> SignedAmount { + let total_input_value = self + .inputs + .iter() + .map(|input| input.utxo.output.value) + .sum::() .to_signed() - .expect("value_added is validated to not exceed Amount::MAX_MONEY"); - let value_removed = self + .expect("total_input_value is validated to not exceed Amount::MAX_MONEY"); + let total_output_value = self .outputs .iter() + .chain(self.change_output.iter()) .map(|txout| txout.value) .sum::() .to_signed() - .expect("value_removed is validated to not exceed Amount::MAX_MONEY"); - - let contribution_amount = value_added - value_removed; - contribution_amount - .checked_sub(unpaid_fees) + .expect("total_output_value is validated to not exceed Amount::MAX_MONEY"); + total_input_value + .checked_sub(total_output_value) .expect("all amounts are validated to not exceed Amount::MAX_MONEY") } } @@ -1298,190 +1219,6 @@ mod tests { } } - #[test] - #[rustfmt::skip] - fn test_check_v2_funding_inputs_sufficient() { - // positive case, inputs well over intended contribution - { - let expected_fee = if cfg!(feature = "grind_signatures") { 2278 } else { 2284 }; - let contribution = FundingContribution { - value_added: Amount::from_sat(220_000), - estimated_fee: Amount::from_sat(expected_fee), - inputs: vec![ - funding_input_sats(200_000), - funding_input_sats(100_000), - ], - outputs: vec![], - change_output: None, - is_splice: true, - feerate: FeeRate::from_sat_per_kwu(2000), - max_feerate: FeeRate::MAX, - }; - assert!(contribution.validate().is_ok()); - assert_eq!(contribution.net_value(), contribution.value_added.to_signed().unwrap()); - } - - // Net splice-in - { - let expected_fee = if cfg!(feature = "grind_signatures") { 2526 } else { 2532 }; - let contribution = FundingContribution { - value_added: Amount::from_sat(220_000), - estimated_fee: Amount::from_sat(expected_fee), - inputs: vec![ - funding_input_sats(200_000), - funding_input_sats(100_000), - ], - outputs: vec![ - funding_output_sats(200_000), - ], - change_output: None, - is_splice: true, - feerate: FeeRate::from_sat_per_kwu(2000), - max_feerate: FeeRate::MAX, - }; - assert!(contribution.validate().is_ok()); - assert_eq!(contribution.net_value(), SignedAmount::from_sat(220_000 - 200_000)); - } - - // Net splice-out - { - let expected_fee = if cfg!(feature = "grind_signatures") { 2526 } else { 2532 }; - let contribution = FundingContribution { - value_added: Amount::from_sat(220_000), - estimated_fee: Amount::from_sat(expected_fee), - inputs: vec![ - funding_input_sats(200_000), - funding_input_sats(100_000), - ], - outputs: vec![ - funding_output_sats(400_000), - ], - change_output: None, - is_splice: true, - feerate: FeeRate::from_sat_per_kwu(2000), - max_feerate: FeeRate::MAX, - }; - assert!(contribution.validate().is_ok()); - assert_eq!(contribution.net_value(), SignedAmount::from_sat(220_000 - 400_000)); - } - - // Net splice-out, inputs insufficient to cover fees - { - let expected_fee = if cfg!(feature = "grind_signatures") { 113670 } else { 113940 }; - let contribution = FundingContribution { - value_added: Amount::from_sat(220_000), - estimated_fee: Amount::from_sat(expected_fee), - inputs: vec![ - funding_input_sats(200_000), - funding_input_sats(100_000), - ], - outputs: vec![ - funding_output_sats(400_000), - ], - change_output: None, - is_splice: true, - feerate: FeeRate::from_sat_per_kwu(90000), - max_feerate: FeeRate::MAX, - }; - assert_eq!( - contribution.validate(), - Err(format!( - "Total input amount 0.00300000 BTC is lower than needed for splice-in contribution 0.00220000 BTC, considering fees of {}. Need more inputs.", - Amount::from_sat(expected_fee), - )), - ); - } - - // negative case, inputs clearly insufficient - { - let expected_fee = if cfg!(feature = "grind_signatures") { 1736 } else { 1740 }; - let contribution = FundingContribution { - value_added: Amount::from_sat(220_000), - estimated_fee: Amount::from_sat(expected_fee), - inputs: vec![ - funding_input_sats(100_000), - ], - outputs: vec![], - change_output: None, - is_splice: true, - feerate: FeeRate::from_sat_per_kwu(2000), - max_feerate: FeeRate::MAX, - }; - assert_eq!( - contribution.validate(), - Err(format!( - "Total input amount 0.00100000 BTC is lower than needed for splice-in contribution 0.00220000 BTC, considering fees of {}. Need more inputs.", - Amount::from_sat(expected_fee), - )), - ); - } - - // barely covers - { - let expected_fee = if cfg!(feature = "grind_signatures") { 2278 } else { 2284 }; - let contribution = FundingContribution { - value_added: Amount::from_sat(300_000 - expected_fee - 20), - estimated_fee: Amount::from_sat(expected_fee), - inputs: vec![ - funding_input_sats(200_000), - funding_input_sats(100_000), - ], - outputs: vec![], - change_output: None, - is_splice: true, - feerate: FeeRate::from_sat_per_kwu(2000), - max_feerate: FeeRate::MAX, - }; - assert!(contribution.validate().is_ok()); - assert_eq!(contribution.net_value(), contribution.value_added.to_signed().unwrap()); - } - - // higher fee rate, does not cover - { - let expected_fee = if cfg!(feature = "grind_signatures") { 2506 } else { 2513 }; - let contribution = FundingContribution { - value_added: Amount::from_sat(298032), - estimated_fee: Amount::from_sat(expected_fee), - inputs: vec![ - funding_input_sats(200_000), - funding_input_sats(100_000), - ], - outputs: vec![], - change_output: None, - is_splice: true, - feerate: FeeRate::from_sat_per_kwu(2200), - max_feerate: FeeRate::MAX, - }; - assert_eq!( - contribution.validate(), - Err(format!( - "Total input amount 0.00300000 BTC is lower than needed for splice-in contribution 0.00298032 BTC, considering fees of {}. Need more inputs.", - Amount::from_sat(expected_fee), - )), - ); - } - - // barely covers, less fees (not a splice) - { - let expected_fee = if cfg!(feature = "grind_signatures") { 1512 } else { 1516 }; - let contribution = FundingContribution { - value_added: Amount::from_sat(300_000 - expected_fee - 20), - estimated_fee: Amount::from_sat(expected_fee), - inputs: vec![ - funding_input_sats(200_000), - funding_input_sats(100_000), - ], - outputs: vec![], - change_output: None, - is_splice: false, - feerate: FeeRate::from_sat_per_kwu(2000), - max_feerate: FeeRate::MAX, - }; - assert!(contribution.validate().is_ok()); - assert_eq!(contribution.net_value(), contribution.value_added.to_signed().unwrap()); - } - } - struct UnreachableWallet; impl CoinSelectionSourceSync for UnreachableWallet { @@ -1612,7 +1349,6 @@ mod tests { estimate_transaction_fee(&inputs, &[], Some(&change), true, true, original_feerate); let contribution = FundingContribution { - value_added: Amount::from_sat(50_000), estimated_fee, inputs: inputs.clone(), outputs: vec![], @@ -1650,7 +1386,6 @@ mod tests { estimate_transaction_fee(&inputs, &[], Some(&change), true, true, original_feerate); let contribution = FundingContribution { - value_added: Amount::from_sat(50_000), estimated_fee, inputs, outputs: vec![], @@ -1691,7 +1426,6 @@ mod tests { let change = funding_output_sats(change_value.to_sat()); let contribution = FundingContribution { - value_added, estimated_fee, inputs: inputs.clone(), outputs: vec![], @@ -1727,7 +1461,6 @@ mod tests { estimate_transaction_fee(&inputs, &[], Some(&change), true, true, original_feerate); let contribution = FundingContribution { - value_added: Amount::from_sat(50_000), estimated_fee, inputs, outputs: vec![], @@ -1753,7 +1486,6 @@ mod tests { estimate_transaction_fee(&[], &outputs, None, true, true, original_feerate); let contribution = FundingContribution { - value_added: Amount::ZERO, estimated_fee, inputs: vec![], outputs: outputs.clone(), @@ -1783,7 +1515,6 @@ mod tests { estimate_transaction_fee(&[], &outputs, None, true, true, original_feerate); let contribution = FundingContribution { - value_added: Amount::ZERO, estimated_fee, inputs: vec![], outputs, @@ -1807,12 +1538,12 @@ mod tests { let target_feerate = FeeRate::from_sat_per_kwu(3000); let inputs = vec![funding_input_sats(100_000)]; let change = funding_output_sats(10_000); + let change_value = change.value; let estimated_fee = estimate_transaction_fee(&inputs, &[], Some(&change), true, true, original_feerate); let contribution = FundingContribution { - value_added: Amount::from_sat(50_000), estimated_fee, inputs, outputs: vec![], @@ -1827,7 +1558,10 @@ mod tests { let net_at_feerate = contribution.net_value_for_acceptor_at_feerate(target_feerate, Amount::MAX).unwrap(); assert_eq!(net_at_feerate, contribution.net_value()); - assert_eq!(net_at_feerate, Amount::from_sat(50_000).to_signed().unwrap()); + assert_eq!( + net_at_feerate, + (Amount::from_sat(100_000) - estimated_fee - change_value).to_signed().unwrap(), + ); } #[test] @@ -1842,7 +1576,6 @@ mod tests { estimate_transaction_fee(&[], &outputs, None, true, true, original_feerate); let contribution = FundingContribution { - value_added: Amount::ZERO, estimated_fee, inputs: vec![], outputs: outputs.clone(), @@ -1866,6 +1599,81 @@ mod tests { assert!(net_at_feerate > contribution.net_value()); } + #[test] + fn test_for_acceptor_at_feerate_mixed_splice_out_balance_insufficient() { + // Inputs are present, but the contribution is net-negative overall, so the remaining value + // must still come from the channel balance. + let original_feerate = FeeRate::from_sat_per_kwu(2000); + let target_feerate = original_feerate; + let inputs = vec![funding_input_sats(100_000)]; + let outputs = vec![funding_output_sats(120_000)]; + + let estimated_fee = + estimate_transaction_fee(&inputs, &outputs, None, true, true, original_feerate); + let target_fee = + estimate_transaction_fee(&inputs, &outputs, None, false, true, target_feerate); + + let contribution = FundingContribution { + estimated_fee, + inputs, + outputs, + change_output: None, + feerate: original_feerate, + max_feerate: FeeRate::MAX, + is_splice: true, + }; + + let holder_balance = target_fee + Amount::from_sat(19_999); + match contribution.for_acceptor_at_feerate(target_feerate, holder_balance) { + Err(FeeRateAdjustmentError::FeeBufferInsufficient { source, available, required }) => { + assert_eq!(source, "channel balance"); + assert_eq!(available, target_fee - Amount::from_sat(1)); + assert_eq!(required, target_fee); + }, + other => panic!("Expected channel-balance shortfall, got {other:?}"), + } + } + + #[test] + fn test_for_acceptor_at_feerate_mixed_splice_out_balance_sufficient() { + // Inputs are present, but the contribution is net-negative overall, so success still depends + // on the channel balance covering the remaining amount plus fees. + let original_feerate = FeeRate::from_sat_per_kwu(2000); + let target_feerate = original_feerate; + let inputs = vec![funding_input_sats(100_000)]; + let outputs = vec![funding_output_sats(120_000)]; + + let estimated_fee = + estimate_transaction_fee(&inputs, &outputs, None, true, true, original_feerate); + let target_fee = + estimate_transaction_fee(&inputs, &outputs, None, false, true, target_feerate); + + let contribution = FundingContribution { + estimated_fee, + inputs: inputs.clone(), + outputs: outputs.clone(), + change_output: None, + feerate: original_feerate, + max_feerate: FeeRate::MAX, + is_splice: true, + }; + + let holder_balance = target_fee + Amount::from_sat(20_000); + let adjusted = + contribution.for_acceptor_at_feerate(target_feerate, holder_balance).unwrap(); + + assert_eq!(adjusted.inputs, inputs); + assert_eq!(adjusted.outputs, outputs); + assert!(adjusted.change_output.is_none()); + assert_eq!(adjusted.estimated_fee, target_fee); + assert_eq!( + adjusted.net_value(), + SignedAmount::ZERO + - Amount::from_sat(20_000).to_signed().unwrap() + - target_fee.to_signed().unwrap(), + ); + } + #[test] fn test_net_value_for_acceptor_at_feerate_does_not_mutate() { // Verify net_value_for_acceptor_at_feerate does not modify the contribution. @@ -1878,7 +1686,6 @@ mod tests { estimate_transaction_fee(&inputs, &[], Some(&change), true, true, original_feerate); let contribution = FundingContribution { - value_added: Amount::from_sat(50_000), estimated_fee, inputs, outputs: vec![], @@ -1900,6 +1707,54 @@ mod tests { assert_eq!(contribution.change_output.as_ref().unwrap().value, change_before); } + #[test] + fn test_net_value_for_initiator_at_feerate_change_removed() { + // The non-mutating initiator path should match the adjusted contribution when increasing the + // feerate drops the change output. + let original_feerate = FeeRate::from_sat_per_kwu(2000); + let target_feerate = FeeRate::from_sat_per_kwu(3000); + let value_added = Amount::from_sat(50_000); + let change_value = Amount::from_sat(1_000); + + let dummy_inputs = vec![funding_input_sats(1)]; + let change = funding_output_sats(change_value.to_sat()); + let estimated_fee = estimate_transaction_fee( + &dummy_inputs, + &[], + Some(&change), + true, + true, + original_feerate, + ); + let inputs = + vec![funding_input_sats((value_added + estimated_fee + change_value).to_sat())]; + + let contribution = FundingContribution { + estimated_fee, + inputs: inputs.clone(), + outputs: vec![], + change_output: Some(change), + feerate: original_feerate, + max_feerate: FeeRate::MAX, + is_splice: true, + }; + + let adjusted = + contribution.clone().for_initiator_at_feerate(target_feerate, Amount::MAX).unwrap(); + let net_at_feerate = + contribution.net_value_for_initiator_at_feerate(target_feerate, Amount::MAX).unwrap(); + + let expected_fee_no_change = + estimate_transaction_fee(&inputs, &[], None, true, true, target_feerate); + assert!(adjusted.change_output.is_none()); + assert_eq!(adjusted.estimated_fee, expected_fee_no_change); + assert_eq!(net_at_feerate, adjusted.net_value()); + + // The non-mutating helper must leave the original contribution untouched. + assert_eq!(contribution.estimated_fee, estimated_fee); + assert_eq!(contribution.change_output.as_ref().unwrap().value, change_value); + } + #[test] fn test_net_value_for_acceptor_at_feerate_too_high() { // net_value_for_acceptor_at_feerate returns Err when feerate can't be accommodated. @@ -1912,7 +1767,6 @@ mod tests { estimate_transaction_fee(&inputs, &[], Some(&change), true, true, original_feerate); let contribution = FundingContribution { - value_added: Amount::from_sat(50_000), estimated_fee, inputs, outputs: vec![], @@ -1940,7 +1794,6 @@ mod tests { estimate_transaction_fee(&inputs, &[], Some(&change), true, true, original_feerate); let contribution = FundingContribution { - value_added: Amount::from_sat(50_000), estimated_fee, inputs, outputs: vec![], @@ -1972,7 +1825,6 @@ mod tests { estimate_transaction_fee(&inputs, &[], Some(&change), true, true, original_feerate); let contribution = FundingContribution { - value_added: Amount::from_sat(50_000), estimated_fee, inputs, outputs: vec![], @@ -2007,7 +1859,6 @@ mod tests { estimate_transaction_fee(&inputs, &[], Some(&change), true, true, original_feerate); let contribution = FundingContribution { - value_added: Amount::from_sat(50_000), estimated_fee, inputs, outputs: vec![], @@ -2050,7 +1901,6 @@ mod tests { assert!(target_fee > estimated_fee); let contribution = FundingContribution { - value_added, estimated_fee, inputs, outputs: vec![], @@ -2083,7 +1933,6 @@ mod tests { assert!(target_fee > estimated_fee); let contribution = FundingContribution { - value_added, estimated_fee, inputs, outputs: vec![], @@ -2122,7 +1971,6 @@ mod tests { assert!(estimated_fee - target_fee < dust_limit); let contribution = FundingContribution { - value_added: Amount::from_sat(50_000), estimated_fee, inputs, outputs: vec![], @@ -2159,7 +2007,6 @@ mod tests { let target_fee = estimate_transaction_fee(&inputs, &[], None, false, true, feerate); let contribution = FundingContribution { - value_added, estimated_fee, inputs, outputs: vec![], @@ -2178,20 +2025,17 @@ mod tests { assert!(adjusted.change_output.is_none()); assert_eq!(adjusted.estimated_fee, target_fee); let surplus = estimated_fee - target_fee; - assert_eq!(adjusted.value_added, value_added + surplus); + assert_eq!(adjusted.value_added(), value_added + surplus); assert_eq!(adjusted.net_value(), net_value_before + surplus.to_signed().unwrap()); } #[test] - fn test_for_acceptor_at_feerate_fee_buffer_overflow() { - // Construct a contribution with estimated_fee and change values that overflow Amount. + fn test_for_acceptor_at_feerate_fee_buffer_overflow_with_change() { + // Overflow in estimated_fee + change value should surface as FeeBufferOverflow. let feerate = FeeRate::from_sat_per_kwu(2000); - let inputs = vec![funding_input_sats(100_000)]; - let contribution = FundingContribution { - value_added: Amount::from_sat(50_000), estimated_fee: Amount::MAX, - inputs, + inputs: vec![funding_input_sats(100_000)], outputs: vec![], change_output: Some(funding_output_sats(1)), feerate, @@ -2214,7 +2058,6 @@ mod tests { estimate_transaction_fee(&[], &outputs, None, true, true, original_feerate); let contribution = FundingContribution { - value_added: Amount::ZERO, estimated_fee, inputs: vec![], outputs: outputs.clone(), @@ -2241,7 +2084,6 @@ mod tests { estimate_transaction_fee(&[], &outputs, None, true, true, original_feerate); let contribution = FundingContribution { - value_added: Amount::ZERO, estimated_fee, inputs: vec![], outputs: outputs.clone(), @@ -2272,7 +2114,6 @@ mod tests { estimate_transaction_fee(&[], &outputs, None, true, true, original_feerate); let contribution = FundingContribution { - value_added: Amount::ZERO, estimated_fee, inputs: vec![], outputs, @@ -2301,7 +2142,6 @@ mod tests { estimate_transaction_fee(&inputs, &[], Some(&change), true, true, original_feerate); let contribution = FundingContribution { - value_added: Amount::from_sat(50_000), estimated_fee, inputs, outputs: vec![], @@ -2336,7 +2176,6 @@ mod tests { let max_feerate = FeeRate::from_sat_per_kwu(2020); let prior = FundingContribution { - value_added: Amount::from_sat(50_000), estimated_fee: Amount::from_sat(1_000), inputs: vec![funding_input_sats(100_000)], outputs: vec![], @@ -2372,7 +2211,6 @@ mod tests { estimate_transaction_fee(&inputs, &[], Some(&change), true, true, prior_feerate); let prior = FundingContribution { - value_added: Amount::from_sat(50_000), estimated_fee, inputs, outputs: vec![], @@ -2434,7 +2272,6 @@ mod tests { let withdrawal = funding_output_sats(20_000); let prior = FundingContribution { - value_added: Amount::ZERO, estimated_fee: Amount::from_sat(500), inputs: vec![], outputs: vec![withdrawal.clone()], @@ -2457,8 +2294,8 @@ mod tests { // rbf_sync should succeed and the contribution should have inputs from coin selection. let contribution = template.rbf_sync(FeeRate::MAX, &wallet).unwrap(); - assert_eq!(contribution.value_added, Amount::ZERO); assert!(!contribution.inputs.is_empty(), "coin selection should have added inputs"); + assert!(contribution.value_added() > Amount::ZERO); assert_eq!(contribution.outputs, vec![withdrawal]); assert_eq!(contribution.feerate, min_rbf_feerate); } @@ -2478,8 +2315,8 @@ mod tests { }; let contribution = template.rbf_sync(FeeRate::MAX, &wallet).unwrap(); - assert_eq!(contribution.value_added, Amount::ZERO); assert!(!contribution.inputs.is_empty(), "coin selection should have added inputs"); + assert!(contribution.value_added() > Amount::ZERO); assert!(contribution.outputs.is_empty()); assert_eq!(contribution.feerate, min_rbf_feerate); } @@ -2495,7 +2332,6 @@ mod tests { let withdrawal = funding_output_sats(20_000); let prior = FundingContribution { - value_added: Amount::ZERO, estimated_fee: Amount::from_sat(500), inputs: vec![], outputs: vec![withdrawal.clone()], @@ -2537,7 +2373,7 @@ mod tests { let contribution = template.splice_out(vec![withdrawal.clone()], feerate, FeeRate::MAX).unwrap(); - assert_eq!(contribution.value_added, Amount::ZERO); + assert_eq!(contribution.value_added(), Amount::ZERO); assert!(contribution.inputs.is_empty()); assert!(contribution.change_output.is_none()); assert_eq!(contribution.outputs, vec![withdrawal]); diff --git a/lightning/src/ln/splicing_tests.rs b/lightning/src/ln/splicing_tests.rs index 54929214ab6..3004c76fb93 100644 --- a/lightning/src/ln/splicing_tests.rs +++ b/lightning/src/ln/splicing_tests.rs @@ -158,40 +158,6 @@ impl CoinSelectionSourceSync for TightBudgetWallet { } } -#[test] -fn test_validate_accounts_for_change_output_weight() { - // Demonstrates that estimated_fee includes the change output's weight when building a - // FundingContribution. A mock wallet returns a single input whose value is between - // estimated_fee_without_change (1736/1740 sats) and estimated_fee_with_change (1984/1988 - // sats) above value_added. The validate() check correctly catches that the inputs are - // insufficient when the change output weight is included. Without accounting for the change - // output weight, the check would incorrectly pass. - 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, &[None, None]); - let nodes = create_network(2, &node_cfgs, &node_chanmgrs); - - let (_, _, channel_id, _) = - create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, 0); - - let feerate = FeeRate::from_sat_per_kwu(2000); - let funding_template = - nodes[0].node.splice_channel(&channel_id, &nodes[1].node.get_our_node_id()).unwrap(); - - // Input value = value_added + 1800: above 1736/1740 (fee without change), below 1984/1988 - // (fee with change). - let value_added = Amount::from_sat(20_000); - let wallet = TightBudgetWallet { - utxo_value: value_added + Amount::from_sat(1800), - change_value: Amount::from_sat(1000), - }; - let contribution = - funding_template.splice_in_sync(value_added, feerate, FeeRate::MAX, &wallet).unwrap(); - - assert!(contribution.change_output().is_some()); - assert!(contribution.validate().is_err()); -} - pub fn negotiate_splice_tx<'a, 'b, 'c, 'd>( initiator: &'a Node<'b, 'c, 'd>, acceptor: &'a Node<'b, 'c, 'd>, channel_id: ChannelId, funding_contribution: FundingContribution, @@ -1862,7 +1828,8 @@ fn do_test_splice_commitment_broadcast(splice_status: SpliceStatus, claim_htlcs: let splice_in_amount = initial_channel_capacity / 2; let initiator_contribution = do_initiate_splice_in(&nodes[0], &nodes[1], channel_id, Amount::from_sat(splice_in_amount)); - let (splice_tx, _) = splice_channel(&nodes[0], &nodes[1], channel_id, initiator_contribution); + let (splice_tx, _) = + splice_channel(&nodes[0], &nodes[1], channel_id, initiator_contribution.clone()); let (preimage2, payment_hash2, ..) = route_payment(&nodes[0], &[&nodes[1]], payment_amount); let htlc_expiry = nodes[0].best_block_info().1 + TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS; @@ -1913,7 +1880,7 @@ fn do_test_splice_commitment_broadcast(splice_status: SpliceStatus, claim_htlcs: message: "test".to_owned(), }; let closed_channel_capacity = if splice_status == SpliceStatus::Locked { - initial_channel_capacity + splice_in_amount + initial_channel_capacity + initiator_contribution.net_value().to_sat() as u64 } else { initial_channel_capacity }; From dc290f803d310706f4f8fd67af9dde346553daf8 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Fri, 3 Apr 2026 11:44:34 -0700 Subject: [PATCH 4/5] Introduce FundingBuilder for splice requests This lets callers easily amend a prior contribution in place and only re-run coin selection when the new request cannot be satisfied with the existing inputs. --- lightning/src/ln/channel.rs | 22 +- lightning/src/ln/funding.rs | 850 ++++++++++++++++++++++++++++++++++-- 2 files changed, 831 insertions(+), 41 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index b99b2a19667..c9301ac67f5 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -12535,14 +12535,12 @@ where }; } - if let Err(e) = contribution.validate().and_then(|()| { - // For splice-out, our_funding_contribution is adjusted to cover fees if there - // aren't any inputs. - let our_funding_contribution = contribution.net_value(); + let our_funding_contribution = contribution.net_value(); + + if let Err(e) = self.validate_splice_contributions(our_funding_contribution, SignedAmount::ZERO) - }) { + { log_error!(logger, "Channel {} cannot be funded: {}", self.context.channel_id(), e); - return Err(QuiescentError::FailSplice(self.splice_funding_failed_for(contribution))); } @@ -14104,13 +14102,11 @@ where // funding_contributed and quiescence, reducing the holder's // balance. If invalid, disconnect and return the contribution so // the user can reclaim their inputs. - if let Err(e) = contribution.validate().and_then(|()| { - let our_funding_contribution = contribution.net_value(); - self.validate_splice_contributions( - our_funding_contribution, - SignedAmount::ZERO, - ) - }) { + let our_funding_contribution = contribution.net_value(); + if let Err(e) = self.validate_splice_contributions( + our_funding_contribution, + SignedAmount::ZERO, + ) { let failed = self.splice_funding_failed_for(contribution); return Err(( ChannelError::WarnAndDisconnect(format!( diff --git a/lightning/src/ln/funding.rs b/lightning/src/ln/funding.rs index 62499871e34..e7464cc8969 100644 --- a/lightning/src/ln/funding.rs +++ b/lightning/src/ln/funding.rs @@ -133,8 +133,12 @@ pub enum FundingContributionError { }, /// The splice value is invalid (zero, empty outputs, or exceeds the maximum money supply). InvalidSpliceValue, + /// An input's `prevtx` is too large to fit in a `tx_add_input` message. + PrevTxTooLarge, /// Coin selection failed to find suitable inputs. CoinSelectionFailed, + /// Coin selection is required but no coin selection source was provided. + MissingCoinSelectionSource, /// This is not an RBF scenario (no minimum RBF feerate available). NotRbfScenario, } @@ -151,9 +155,15 @@ impl core::fmt::Display for FundingContributionError { FundingContributionError::InvalidSpliceValue => { write!(f, "Invalid splice value (zero, empty, or exceeds limit)") }, + FundingContributionError::PrevTxTooLarge => { + write!(f, "Input prevtx is too large to fit in a tx_add_input message") + }, FundingContributionError::CoinSelectionFailed => { write!(f, "Coin selection failed to find suitable inputs") }, + FundingContributionError::MissingCoinSelectionSource => { + write!(f, "Coin selection source required to build this contribution") + }, FundingContributionError::NotRbfScenario => { write!(f, "Not an RBF scenario (no minimum RBF feerate)") }, @@ -702,6 +712,44 @@ fn estimate_transaction_fee( Weight::from_wu(weight) * feerate } +fn validate_inputs(inputs: &[FundingTxInput]) -> Result<(), FundingContributionError> { + let mut total_value = Amount::ZERO; + for input in inputs { + use crate::util::ser::Writeable; + const MESSAGE_TEMPLATE: msgs::TxAddInput = msgs::TxAddInput { + channel_id: ChannelId([0; 32]), + serial_id: 0, + prevtx: None, + prevtx_out: 0, + sequence: 0, + // Mutually exclusive with prevtx, which is accounted for below. + shared_input_txid: None, + }; + let message_len = MESSAGE_TEMPLATE.serialized_length() + input.prevtx.serialized_length(); + (message_len <= LN_MAX_MSG_LEN) + .then(|| ()) + .ok_or(FundingContributionError::PrevTxTooLarge)?; + + total_value = match total_value.checked_add(input.utxo.output.value) { + Some(sum) if sum <= Amount::MAX_MONEY => sum, + _ => return Err(FundingContributionError::InvalidSpliceValue), + }; + } + + Ok(()) +} + +/// Describes how an amended contribution should source its wallet-backed inputs. +enum FundingInputs { + None, + /// Reuses the contribution's existing inputs while targeting at least `value_added` added to + /// the channel after fees. If dropping the change output leaves surplus value, it remains in + /// the channel contribution. + CoinSelected { + value_added: Amount, + }, +} + /// The components of a funding transaction contributed by one party. #[derive(Debug, Clone, PartialEq, Eq)] pub struct FundingContribution { @@ -805,6 +853,104 @@ impl FundingContribution { self.change_output.as_ref() } + fn with_inputs_and_outputs( + self, funding_inputs: FundingInputs, outputs: &[TxOut], + ) -> Option { + let (target_value_added, inputs) = match funding_inputs { + FundingInputs::None => (None, Vec::new()), + FundingInputs::CoinSelected { value_added } => (Some(value_added), self.inputs), + }; + + if inputs.is_empty() && target_value_added.unwrap_or(Amount::ZERO) != Amount::ZERO { + // Prior contribution didn't have any inputs, but now we need some. + return None; + } + + let estimated_fee_no_change = + estimate_transaction_fee(&inputs, &outputs, None, true, self.is_splice, self.feerate); + + // When inputs are coin-selected, adjust the existing change output, if any, to account for + // the new value added. + if let Some(value_added) = target_value_added { + let estimated_fee = estimate_transaction_fee( + &inputs, + &outputs, + self.change_output.as_ref(), + true, + self.is_splice, + self.feerate, + ); + let required_value = value_added.checked_add(estimated_fee)?; + + let total_input_value: Amount = + inputs.iter().map(|input| input.utxo.output.value).sum(); + + if self.change_output.is_none() && total_input_value < required_value { + // The prior input selection is not enough, we may need to re-attempt coin + // selection. + return None; + } + + if let Some(change_output) = self.change_output.as_ref() { + let dust_limit = change_output.script_pubkey.minimal_non_dust(); + match total_input_value.checked_sub(required_value) { + Some(new_change_value) if new_change_value >= dust_limit => { + let new_change_output = TxOut { + value: new_change_value, + script_pubkey: change_output.script_pubkey.clone(), + }; + return Some(FundingContribution { + estimated_fee, + inputs, + outputs: outputs.to_vec(), + change_output: Some(new_change_output), + ..self + }); + }, + _ => { + let required_value = value_added.checked_add(estimated_fee_no_change)?; + if total_input_value < required_value { + return None; + } + }, + } + } + } + + Some(FundingContribution { + estimated_fee: estimated_fee_no_change, + outputs: outputs.to_vec(), + inputs, + change_output: None, + ..self + }) + } + + /// Tries to satisfy a new request using only this contribution's existing inputs. + /// + /// For input-backed contributions, this reuses the current inputs, adjusts the explicit + /// outputs, and shrinks or drops the change output as needed before applying + /// `target_feerate`. If dropping change leaves surplus value, that surplus remains in the + /// channel contribution. + /// + /// For input-less contributions, `holder_balance` must be provided to cover the outputs and + /// fees from the channel balance. + /// + /// Returns `None` if the request would require new wallet inputs or cannot accommodate the + /// requested feerate. + fn amend_without_coin_selection( + self, funding_inputs: FundingInputs, outputs: &[TxOut], target_feerate: FeeRate, + max_feerate: FeeRate, holder_balance: Amount, + ) -> Option { + let new_contribution_at_current_feerate = + self.with_inputs_and_outputs(funding_inputs, outputs)?; + let mut new_contribution_at_target_feerate = new_contribution_at_current_feerate + .at_feerate(target_feerate, holder_balance, true) + .ok()?; + new_contribution_at_target_feerate.max_feerate = max_feerate; + Some(new_contribution_at_target_feerate) + } + pub(super) fn into_tx_parts(self) -> (Vec, Vec) { let FundingContribution { inputs, mut outputs, change_output, .. } = self; @@ -839,32 +985,6 @@ impl FundingContribution { } } - /// Validates that the funding inputs are suitable for use in the interactive transaction - /// protocol, checking prevtx sizes and input sufficiency. - pub fn validate(&self) -> Result<(), String> { - for FundingTxInput { utxo, prevtx, .. } in self.inputs.iter() { - use crate::util::ser::Writeable; - const MESSAGE_TEMPLATE: msgs::TxAddInput = msgs::TxAddInput { - channel_id: ChannelId([0; 32]), - serial_id: 0, - prevtx: None, - prevtx_out: 0, - sequence: 0, - // Mutually exclusive with prevtx, which is accounted for below. - shared_input_txid: None, - }; - let message_len = MESSAGE_TEMPLATE.serialized_length() + prevtx.serialized_length(); - if message_len > LN_MAX_MSG_LEN { - return Err(format!( - "Funding input references a prevtx that is too large for tx_add_input: {}", - utxo.outpoint - )); - } - } - - Ok(()) - } - /// Computes the adjusted fee and change output value at the given target feerate, which may /// differ from the feerate used during coin selection. /// @@ -1111,17 +1231,515 @@ impl FundingContribution { /// establishment protocol or when splicing. pub type FundingTxInput = crate::util::wallet_utils::ConfirmedUtxo; +#[derive(Debug, Clone, PartialEq, Eq)] +struct NoCoinSelectionSource; +#[derive(Debug, Clone, PartialEq, Eq)] +struct AsyncCoinSelectionSource(W); +#[derive(Debug, Clone, PartialEq, Eq)] +struct SyncCoinSelectionSource(W); + +#[derive(Debug, Clone, PartialEq, Eq)] +struct FundingBuilderInner { + shared_input: Option, + min_rbf_feerate: Option, + prior_contribution: Option, + value_added: Amount, + outputs: Vec, + feerate: FeeRate, + max_feerate: FeeRate, + state: State, +} + +/// A builder for composing or amending a [`FundingContribution`]. +/// +/// The builder tracks a requested amount to add to the channel together with any explicit +/// withdrawal outputs. Building without an attached wallet only succeeds when the request can be +/// satisfied by reusing or amending a prior contribution, or by constructing a pure splice-out +/// that pays fees from the channel balance. +/// +/// Attach a wallet via [`FundingBuilder::with_coin_selection_source`] or +/// [`FundingBuilder::with_coin_selection_source_sync`] when the request may need new wallet +/// inputs. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct FundingBuilder(FundingBuilderInner); + +/// A [`FundingBuilder`] with an attached asynchronous [`CoinSelectionSource`]. +/// +/// Created by [`FundingBuilder::with_coin_selection_source`]. The attached wallet is only used +/// if the request cannot be satisfied by reusing a prior contribution or by building a pure +/// splice-out directly. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct AsyncFundingBuilder(FundingBuilderInner>); + +/// A [`FundingBuilder`] with an attached synchronous [`CoinSelectionSourceSync`]. +/// +/// Created by [`FundingBuilder::with_coin_selection_source_sync`]. The attached wallet is only +/// used if the request cannot be satisfied by reusing a prior contribution or by building a pure +/// splice-out directly. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SyncFundingBuilder(FundingBuilderInner>); + +impl FundingBuilderInner { + fn request_matches_prior(&self, prior_contribution: &FundingContribution) -> bool { + self.value_added == prior_contribution.value_added() + && self.outputs == prior_contribution.outputs + } + + fn build_from_prior_contribution( + &mut self, contribution: PriorContribution, + ) -> Result { + let PriorContribution { contribution, holder_balance } = contribution; + + if self.request_matches_prior(&contribution) { + // Same request, but the feerate may have changed. Adjust the prior contribution + // to the new feerate if possible. + return contribution + .for_initiator_at_feerate(self.feerate, holder_balance) + .map(|mut adjusted| { + adjusted.max_feerate = self.max_feerate; + adjusted + }) + .map_err(|_| FundingContributionError::MissingCoinSelectionSource); + } + + let funding_inputs = if self.value_added != Amount::ZERO { + FundingInputs::CoinSelected { value_added: self.value_added } + } else { + FundingInputs::None + }; + return contribution + .amend_without_coin_selection( + funding_inputs, + &self.outputs, + self.feerate, + self.max_feerate, + holder_balance, + ) + .ok_or_else(|| FundingContributionError::MissingCoinSelectionSource); + } + + /// Tries to build the current request without selecting any new wallet inputs. + /// + /// This first attempts to reuse or amend any prior contribution. If there is no prior + /// contribution, it also supports pure splice-out requests by building a contribution that pays + /// fees from the channel balance. + /// + /// Returns [`FundingContributionError::MissingCoinSelectionSource`] if the request is + /// otherwise valid but needs wallet inputs. + fn try_build_without_coin_selection( + &mut self, + ) -> Result { + if let Some(contribution) = self.prior_contribution.take() { + return self.build_from_prior_contribution(contribution); + } + + if self.value_added == Amount::ZERO { + let estimated_fee = estimate_transaction_fee( + &[], + &self.outputs, + None, + true, + self.shared_input.is_some(), + self.feerate, + ); + return Ok(FundingContribution { + estimated_fee, + inputs: vec![], + outputs: core::mem::take(&mut self.outputs), + change_output: None, + feerate: self.feerate, + max_feerate: self.max_feerate, + is_splice: self.shared_input.is_some(), + }); + } + + Err(FundingContributionError::MissingCoinSelectionSource) + } + + fn prepare_coin_selection_request( + &self, + ) -> Result<(Vec, Vec), FundingContributionError> { + let value_removed = self.outputs.iter().map(|output| output.value).sum(); + + let dummy_pubkey = PublicKey::from_slice(&[2; 33]).unwrap(); + let shared_output = bitcoin::TxOut { + value: self + .shared_input + .as_ref() + .map(|shared_input| shared_input.previous_utxo.value) + .unwrap_or(Amount::ZERO) + .checked_add(self.value_added) + .ok_or(FundingContributionError::InvalidSpliceValue)? + .checked_sub(value_removed) + .ok_or(FundingContributionError::InvalidSpliceValue)?, + script_pubkey: make_funding_redeemscript(&dummy_pubkey, &dummy_pubkey).to_p2wsh(), + }; + + let must_spend = self.shared_input.clone().map(|input| vec![input]).unwrap_or_default(); + let must_pay_to = if self.outputs.is_empty() { + vec![shared_output] + } else { + self.outputs.iter().cloned().chain(core::iter::once(shared_output)).collect() + }; + + Ok((must_spend, must_pay_to)) + } + + fn validate_contribution_parameters(&self) -> Result<(), FundingContributionError> { + if self.feerate > self.max_feerate { + return Err(FundingContributionError::FeeRateExceedsMaximum { + feerate: self.feerate, + max_feerate: self.max_feerate, + }); + } + + if let Some(min_rbf_feerate) = self.min_rbf_feerate.as_ref() { + if self.feerate < *min_rbf_feerate { + return Err(FundingContributionError::FeeRateBelowRbfMinimum { + feerate: self.feerate, + min_rbf_feerate: *min_rbf_feerate, + }); + } + } + + if self.value_added == Amount::ZERO && self.outputs.is_empty() { + return Err(FundingContributionError::InvalidSpliceValue); + } + + // Validate user-provided amounts are within MAX_MONEY before coin selection to + // ensure FundingContribution::net_value() arithmetic cannot overflow. With all + // amounts bounded by MAX_MONEY (~2.1e15 sat), the worst-case net_value() + // computation is -2 * MAX_MONEY (~-4.2e15), well within i64::MIN (~-9.2e18). + if self.value_added > Amount::MAX_MONEY { + return Err(FundingContributionError::InvalidSpliceValue); + } + + let mut value_removed = Amount::ZERO; + for output in self.outputs.iter() { + value_removed = match value_removed.checked_add(output.value) { + Some(sum) if sum <= Amount::MAX_MONEY => sum, + _ => return Err(FundingContributionError::InvalidSpliceValue), + }; + } + + Ok(()) + } +} + +impl FundingBuilder { + fn new(template: FundingTemplate, feerate: FeeRate, max_feerate: FeeRate) -> FundingBuilder { + let FundingTemplate { shared_input, min_rbf_feerate, prior_contribution } = template; + let (value_added, outputs) = match prior_contribution.as_ref() { + Some(prior) => { + let outputs = prior.contribution.outputs.clone(); + (prior.contribution.value_added(), outputs) + }, + None => (Amount::ZERO, Vec::new()), + }; + + FundingBuilder(FundingBuilderInner { + shared_input, + min_rbf_feerate, + prior_contribution, + value_added, + outputs, + feerate, + max_feerate, + state: NoCoinSelectionSource, + }) + } + + /// Attaches an asynchronous [`CoinSelectionSource`] for later use. + /// + /// The wallet is only consulted if [`AsyncFundingBuilder::build`] cannot satisfy the request by + /// reusing a prior contribution or by constructing a pure splice-out directly. + pub fn with_coin_selection_source( + self, wallet: W, + ) -> AsyncFundingBuilder { + AsyncFundingBuilder(self.0.with_state(AsyncCoinSelectionSource(wallet))) + } + + /// Attaches a synchronous [`CoinSelectionSourceSync`] for later use. + /// + /// The wallet is only consulted if [`SyncFundingBuilder::build`] cannot satisfy the request by + /// reusing a prior contribution or by constructing a pure splice-out directly. + pub fn with_coin_selection_source_sync( + self, wallet: W, + ) -> SyncFundingBuilder { + SyncFundingBuilder(self.0.with_state(SyncCoinSelectionSource(wallet))) + } + + /// Adds a withdrawal output to the request. + /// + /// `output` is appended to the current set of explicit outputs. If the builder was seeded from + /// a prior contribution, this adds an additional withdrawal on top of the prior outputs. This + /// does not affect any change output derived when the contribution is built. + pub fn add_output(self, output: TxOut) -> Self { + FundingBuilder(self.0.add_output_inner(output)) + } + + /// Removes all explicit withdrawal outputs whose script pubkey matches `script_pubkey`. + /// + /// This only affects outputs returned by [`FundingContribution::outputs`]; it never removes the + /// change output returned by [`FundingContribution::change_output`]. + pub fn remove_outputs(self, script_pubkey: &ScriptBuf) -> Self { + FundingBuilder(self.0.remove_outputs_inner(script_pubkey)) + } + + /// Builds a [`FundingContribution`] without coin selection. + /// + /// This succeeds when the request can be satisfied by reusing or amending a prior + /// contribution, or by building a splice-out contribution that pays fees from the channel + /// balance. + /// + /// Returns [`FundingContributionError::MissingCoinSelectionSource`] if additional wallet + /// inputs are needed. + pub fn build(mut self) -> Result { + self.0.build_without_coin_selection() + } +} + +impl FundingBuilderInner { + fn with_state(self, state: NewState) -> FundingBuilderInner { + FundingBuilderInner { + shared_input: self.shared_input, + min_rbf_feerate: self.min_rbf_feerate, + prior_contribution: self.prior_contribution, + value_added: self.value_added, + outputs: self.outputs, + feerate: self.feerate, + max_feerate: self.max_feerate, + state, + } + } + + fn add_value_inner(mut self, value: Amount) -> Self { + self.value_added = + Amount::from_sat(self.value_added.to_sat().saturating_add(value.to_sat())); + self + } + + fn remove_value_inner(mut self, value: Amount) -> Self { + self.value_added = + Amount::from_sat(self.value_added.to_sat().saturating_sub(value.to_sat())); + self + } + + fn add_output_inner(mut self, output: TxOut) -> Self { + self.outputs.push(output); + self + } + + fn remove_outputs_inner(mut self, script_pubkey: &ScriptBuf) -> Self { + self.outputs.retain(|output| output.script_pubkey != *script_pubkey); + self + } + + /// Validates the current request and then tries to build it without selecting new wallet + /// inputs. + /// + /// Returns [`FundingContributionError::MissingCoinSelectionSource`] if the request is valid but + /// cannot be satisfied without wallet inputs. + fn build_without_coin_selection( + &mut self, + ) -> Result { + self.validate_contribution_parameters()?; + self.try_build_without_coin_selection() + } +} + +impl AsyncFundingBuilder { + /// Adds a withdrawal output to the request. + /// + /// `output` is appended to the current set of explicit outputs. If the builder was seeded from + /// a prior contribution, this adds an additional withdrawal on top of the prior outputs. This + /// does not affect any change output derived when the contribution is built. + pub fn add_output(self, output: TxOut) -> Self { + AsyncFundingBuilder(self.0.add_output_inner(output)) + } + + /// Removes all explicit withdrawal outputs whose script pubkey matches `script_pubkey`. + /// + /// This only affects outputs returned by [`FundingContribution::outputs`]; it never removes the + /// change output returned by [`FundingContribution::change_output`]. + pub fn remove_outputs(self, script_pubkey: &ScriptBuf) -> Self { + AsyncFundingBuilder(self.0.remove_outputs_inner(script_pubkey)) + } + + /// Increases the requested amount to add to the channel. + /// + /// `value` is added on top of the builder's current request. If the builder was seeded from a + /// prior contribution, this increases that prior contribution's current amount added to the + /// channel. If the updated request cannot be satisfied in-place, [`AsyncFundingBuilder::build`] + /// may re-run coin selection and return a contribution with a different input set. + pub fn add_value(self, value: Amount) -> Self { + AsyncFundingBuilder(self.0.add_value_inner(value)) + } + + /// Decreases the requested amount to add to the channel. + /// + /// `value` is subtracted from the builder's current request, saturating at zero. If the builder + /// was seeded from a prior contribution, this decreases that prior contribution's current + /// amount added to the channel. If the updated request cannot be satisfied in-place, + /// [`AsyncFundingBuilder::build`] may re-run coin selection and return a contribution with a + /// different input set. + pub fn remove_value(self, value: Amount) -> Self { + AsyncFundingBuilder(self.0.remove_value_inner(value)) + } +} + +impl AsyncFundingBuilder { + /// Builds a [`FundingContribution`], using the attached asynchronous wallet only when needed. + /// + /// If the request can be satisfied by reusing or amending a prior contribution, or by building + /// a pure splice-out directly, the attached wallet is ignored. + pub async fn build(self) -> Result { + let mut inner = self.0; + match inner.build_without_coin_selection() { + Err(FundingContributionError::MissingCoinSelectionSource) => {}, + other => return other, + } + + let (must_spend, must_pay_to) = inner.prepare_coin_selection_request()?; + let AsyncCoinSelectionSource(wallet) = inner.state; + let coin_selection = wallet + .select_confirmed_utxos( + None, + must_spend, + &must_pay_to, + inner.feerate.to_sat_per_kwu() as u32, + u64::MAX, + ) + .await + .map_err(|_| FundingContributionError::CoinSelectionFailed)?; + + let CoinSelection { confirmed_utxos: inputs, change_output } = coin_selection; + validate_inputs(&inputs)?; + + let outputs = inner.outputs; + let is_splice = inner.shared_input.is_some(); + let estimated_fee = estimate_transaction_fee( + &inputs, + &outputs, + change_output.as_ref(), + true, + is_splice, + inner.feerate, + ); + + return Ok(FundingContribution { + estimated_fee, + inputs, + outputs, + change_output, + feerate: inner.feerate, + max_feerate: inner.max_feerate, + is_splice, + }); + } +} + +impl SyncFundingBuilder { + /// Adds a withdrawal output to the request. + /// + /// `output` is appended to the current set of explicit outputs. If the builder was seeded from + /// a prior contribution, this adds an additional withdrawal on top of the prior outputs. This + /// does not affect any change output derived when the contribution is built. + pub fn add_output(self, output: TxOut) -> Self { + SyncFundingBuilder(self.0.add_output_inner(output)) + } + + /// Removes all explicit withdrawal outputs whose script pubkey matches `script_pubkey`. + /// + /// This only affects outputs returned by [`FundingContribution::outputs`]; it never removes the + /// change output returned by [`FundingContribution::change_output`]. + pub fn remove_outputs(self, script_pubkey: &ScriptBuf) -> Self { + SyncFundingBuilder(self.0.remove_outputs_inner(script_pubkey)) + } + + /// Increases the requested amount to add to the channel. + /// + /// `value` is added on top of the builder's current request. If the builder was seeded from a + /// prior contribution, this increases that prior contribution's current amount added to the + /// channel. If the updated request cannot be satisfied in-place, [`SyncFundingBuilder::build`] + /// may re-run coin selection and return a contribution with a different input set. + pub fn add_value(self, value: Amount) -> Self { + SyncFundingBuilder(self.0.add_value_inner(value)) + } + + /// Decreases the requested amount to add to the channel. + /// + /// `value` is subtracted from the builder's current request, saturating at zero. If the builder + /// was seeded from a prior contribution, this decreases that prior contribution's current + /// amount added to the channel. If the updated request cannot be satisfied in-place, + /// [`SyncFundingBuilder::build`] may re-run coin selection and return a contribution with a + /// different input set. + pub fn remove_value(self, value: Amount) -> Self { + SyncFundingBuilder(self.0.remove_value_inner(value)) + } +} + +impl SyncFundingBuilder { + /// Builds a [`FundingContribution`], using the attached synchronous wallet only when needed. + /// + /// If the request can be satisfied by reusing or amending a prior contribution, or by building + /// a pure splice-out directly, the attached wallet is ignored. + pub fn build(self) -> Result { + let mut inner = self.0; + match inner.build_without_coin_selection() { + Err(FundingContributionError::MissingCoinSelectionSource) => {}, + other => return other, + } + + let (must_spend, must_pay_to) = inner.prepare_coin_selection_request()?; + let SyncCoinSelectionSource(wallet) = inner.state; + let coin_selection = wallet + .select_confirmed_utxos( + None, + must_spend, + &must_pay_to, + inner.feerate.to_sat_per_kwu() as u32, + u64::MAX, + ) + .map_err(|_| FundingContributionError::CoinSelectionFailed)?; + + let CoinSelection { confirmed_utxos: inputs, change_output } = coin_selection; + validate_inputs(&inputs)?; + + let outputs = inner.outputs; + let is_splice = inner.shared_input.is_some(); + let estimated_fee = estimate_transaction_fee( + &inputs, + &outputs, + change_output.as_ref(), + true, + is_splice, + inner.feerate, + ); + + return Ok(FundingContribution { + estimated_fee, + inputs, + outputs, + change_output, + feerate: inner.feerate, + max_feerate: inner.max_feerate, + is_splice, + }); + } +} + #[cfg(test)] mod tests { use super::{ - estimate_transaction_fee, FeeRateAdjustmentError, FundingContribution, + estimate_transaction_fee, FeeRateAdjustmentError, FundingBuilder, FundingContribution, FundingContributionError, FundingTemplate, FundingTxInput, PriorContribution, }; use crate::chain::ClaimId; use crate::util::wallet_utils::{CoinSelection, CoinSelectionSourceSync, Input}; use bitcoin::hashes::Hash; use bitcoin::transaction::{Transaction, TxOut, Version}; - use bitcoin::{Amount, FeeRate, Psbt, ScriptBuf, SignedAmount, WPubkeyHash}; + use bitcoin::{Amount, FeeRate, Psbt, ScriptBuf, SignedAmount, WPubkeyHash, WScriptHash}; #[test] #[rustfmt::skip] @@ -1233,6 +1851,154 @@ mod tests { } } + #[test] + fn test_funding_builder_builds_splice_out_without_wallet() { + let feerate = FeeRate::from_sat_per_kwu(2000); + let output = funding_output_sats(25_000); + + let contribution = + FundingBuilder::new(FundingTemplate::new(None, None, None), feerate, FeeRate::MAX) + .add_output(output.clone()) + .build() + .unwrap(); + + let expected_fee = estimate_transaction_fee( + &[], + std::slice::from_ref(&output), + None, + true, + false, + feerate, + ); + assert!(contribution.inputs.is_empty()); + assert_eq!(contribution.outputs, vec![output.clone()]); + assert!(contribution.change_output.is_none()); + assert_eq!(contribution.estimated_fee, expected_fee); + assert_eq!( + contribution.net_value(), + -output.value.to_signed().unwrap() - expected_fee.to_signed().unwrap(), + ); + } + + #[test] + fn test_funding_builder_requires_wallet_for_splice_in() { + let feerate = FeeRate::from_sat_per_kwu(2000); + let builder = + FundingBuilder::new(FundingTemplate::new(None, None, None), feerate, FeeRate::MAX); + let builder = FundingBuilder(builder.0.add_value_inner(Amount::from_sat(25_000))); + + assert!(matches!( + builder.build(), + Err(FundingContributionError::MissingCoinSelectionSource), + )); + } + + #[test] + fn test_funding_builder_amends_prior_by_dropping_subdust_change() { + let feerate = FeeRate::from_sat_per_kwu(2000); + let inputs = vec![funding_input_sats(100_000)]; + let change = funding_output_sats(500); + let dust_limit = change.script_pubkey.minimal_non_dust(); + assert!(change.value >= dust_limit); + + let estimated_fee_with_change = + estimate_transaction_fee(&inputs, &[], Some(&change), true, true, feerate); + let estimated_fee_no_change = + estimate_transaction_fee(&inputs, &[], None, true, true, feerate); + let prior = FundingContribution { + estimated_fee: estimated_fee_with_change, + inputs: inputs.clone(), + outputs: vec![], + change_output: Some(change.clone()), + feerate, + max_feerate: FeeRate::MAX, + is_splice: true, + }; + + let delta = Amount::from_sat(change.value.to_sat() - dust_limit.to_sat() + 1); + let target_value_added = prior.value_added().checked_add(delta).unwrap(); + let total_input_value: Amount = inputs.iter().map(|input| input.utxo.output.value).sum(); + let remaining_change = total_input_value + .checked_sub(target_value_added.checked_add(estimated_fee_with_change).unwrap()) + .unwrap(); + assert_eq!(remaining_change.to_sat(), dust_limit.to_sat() - 1); + assert!( + total_input_value >= target_value_added.checked_add(estimated_fee_no_change).unwrap() + ); + + let builder = + FundingTemplate::new(None, None, Some(PriorContribution::new(prior, Amount::MAX))) + .with_prior_contribution(feerate, FeeRate::MAX); + let contribution = FundingBuilder(builder.0.add_value_inner(delta)).build().unwrap(); + + assert!(contribution.change_output.is_none()); + assert_eq!(contribution.inputs, inputs); + assert!(contribution.outputs.is_empty()); + assert_eq!(contribution.estimated_fee, estimated_fee_no_change); + assert_eq!( + contribution.value_added(), + total_input_value.checked_sub(estimated_fee_no_change).unwrap() + ); + assert!(contribution.value_added() > target_value_added); + } + + #[test] + fn test_funding_builder_remove_outputs_removes_all_matching_scripts() { + let feerate = FeeRate::from_sat_per_kwu(2000); + let removed_script = ScriptBuf::new_p2wpkh(&WPubkeyHash::all_zeros()); + let kept_script = ScriptBuf::new_p2wsh(&WScriptHash::all_zeros()); + let removed_output_1 = + TxOut { value: Amount::from_sat(10_000), script_pubkey: removed_script.clone() }; + let removed_output_2 = + TxOut { value: Amount::from_sat(12_000), script_pubkey: removed_script.clone() }; + let kept_output = TxOut { value: Amount::from_sat(15_000), script_pubkey: kept_script }; + + let contribution = + FundingBuilder::new(FundingTemplate::new(None, None, None), feerate, FeeRate::MAX) + .add_output(removed_output_1) + .add_output(kept_output.clone()) + .add_output(removed_output_2) + .remove_outputs(&removed_script) + .build() + .unwrap(); + + assert_eq!(contribution.outputs, vec![kept_output]); + } + + #[test] + fn test_sync_funding_builder_add_and_remove_value_update_request() { + let feerate = FeeRate::from_sat_per_kwu(2000); + let builder = + FundingBuilder::new(FundingTemplate::new(None, None, None), feerate, FeeRate::MAX) + .with_coin_selection_source_sync(UnreachableWallet) + .add_value(Amount::from_sat(20_000)) + .add_value(Amount::from_sat(5_000)) + .remove_value(Amount::from_sat(10_000)); + + let (_, must_pay_to) = builder.0.prepare_coin_selection_request().unwrap(); + assert_eq!(must_pay_to.len(), 1); + assert_eq!(must_pay_to[0].value, Amount::from_sat(15_000)); + } + + #[test] + fn test_sync_funding_builder_remove_value_saturates_at_zero() { + let feerate = FeeRate::from_sat_per_kwu(2000); + let output = funding_output_sats(8_000); + let contribution = + FundingBuilder::new(FundingTemplate::new(None, None, None), feerate, FeeRate::MAX) + .with_coin_selection_source_sync(UnreachableWallet) + .add_value(Amount::from_sat(10_000)) + .remove_value(Amount::from_sat(15_000)) + .add_output(output.clone()) + .build() + .unwrap(); + + assert!(contribution.inputs.is_empty()); + assert_eq!(contribution.outputs, vec![output]); + assert!(contribution.change_output.is_none()); + assert_eq!(contribution.value_added(), Amount::ZERO); + } + #[test] fn test_build_funding_contribution_validates_max_money() { let over_max = Amount::MAX_MONEY + Amount::from_sat(1); @@ -1333,6 +2099,34 @@ mod tests { } } + #[test] + fn test_build_funding_contribution_rejects_oversized_prevtx() { + use crate::util::ser::Writeable; + + let feerate = FeeRate::from_sat_per_kwu(2000); + let prevtx = Transaction { + input: vec![], + output: vec![funding_output_sats(50_000); 2_200], + version: Version::TWO, + lock_time: bitcoin::absolute::LockTime::ZERO, + }; + assert!(prevtx.serialized_length() > crate::ln::LN_MAX_MSG_LEN); + + let wallet = SingleUtxoWallet { + utxo: FundingTxInput::new_p2wpkh(prevtx, 0).unwrap(), + change_output: None, + }; + assert!(matches!( + FundingTemplate::new(None, None, None).splice_in_sync( + Amount::from_sat(10_000), + feerate, + feerate, + wallet, + ), + Err(FundingContributionError::PrevTxTooLarge), + )); + } + #[test] fn test_for_acceptor_at_feerate_higher_change_adjusted() { // Splice-in: higher target feerate reduces the change output. From fefea5ce806ae9044792532b52cef3d83bac0afb Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Fri, 3 Apr 2026 14:31:24 -0700 Subject: [PATCH 5/5] Replace FundingTemplate contribution methods with FundingBuilder This results in a slight change of behavior: now these methods reuse and amend the prior contribution, as opposed to always starting from a fresh contribution, which would be the desired expected behavior by users. --- lightning/src/ln/funding.rs | 755 ++++++++++++----------------- lightning/src/ln/splicing_tests.rs | 213 ++++++-- 2 files changed, 484 insertions(+), 484 deletions(-) diff --git a/lightning/src/ln/funding.rs b/lightning/src/ln/funding.rs index e7464cc8969..904e1f30406 100644 --- a/lightning/src/ln/funding.rs +++ b/lightning/src/ln/funding.rs @@ -139,7 +139,7 @@ pub enum FundingContributionError { CoinSelectionFailed, /// Coin selection is required but no coin selection source was provided. MissingCoinSelectionSource, - /// This is not an RBF scenario (no minimum RBF feerate available). + /// This template cannot build an RBF contribution. NotRbfScenario, } @@ -165,7 +165,7 @@ impl core::fmt::Display for FundingContributionError { write!(f, "Coin selection source required to build this contribution") }, FundingContributionError::NotRbfScenario => { - write!(f, "Not an RBF scenario (no minimum RBF feerate)") + write!(f, "This template cannot build an RBF contribution") }, } } @@ -174,13 +174,13 @@ impl core::fmt::Display for FundingContributionError { /// The user's prior contribution from a previous splice negotiation on this channel. /// /// When a pending splice exists with negotiated candidates, the prior contribution is -/// available for reuse (e.g., to bump the feerate via RBF). Contains the raw contribution and -/// the holder's balance for deferred feerate adjustment in [`FundingTemplate::rbf_sync`] or -/// [`FundingTemplate::rbf`]. +/// available for reuse. It stores the raw contribution together with the holder's balance for +/// deferred feerate adjustment when the contribution is later reused via +/// [`FundingTemplate::with_prior_contribution`] or [`FundingTemplate::rbf_prior_contribution`]. /// /// Use [`FundingTemplate::prior_contribution`] to inspect the prior contribution before -/// deciding whether to call [`FundingTemplate::rbf_sync`] or one of the splice methods -/// with different parameters. +/// deciding whether to reuse it or replace it with +/// [`FundingTemplate::without_prior_contribution`]. #[derive(Debug, Clone, PartialEq, Eq)] pub(super) struct PriorContribution { contribution: FundingContribution, @@ -212,29 +212,27 @@ impl PriorContribution { /// /// # Building a Contribution /// -/// For a fresh splice (no pending splice to replace), build a new contribution using one of -/// the splice methods: -/// - [`FundingTemplate::splice_in_sync`] to add funds to the channel -/// - [`FundingTemplate::splice_out`] to remove funds from the channel -/// - [`FundingTemplate::splice_in_and_out_sync`] to do both +/// For a fresh splice (no pending splice to replace), either use the convenience methods +/// [`FundingTemplate::splice_in_sync`] and [`FundingTemplate::splice_out`] or start with +/// [`FundingTemplate::without_prior_contribution`] to compose a request manually. /// -/// These require `min_feerate` and `max_feerate` parameters. The splice-in variants perform -/// coin selection when wallet inputs are needed, while splice-out spends only from the channel -/// balance. +/// The builder API supports adding value, adding withdrawal outputs, or both. Attach a wallet +/// when the request may need new wallet inputs; pure splice-out requests can be built without one +/// and pay fees from the channel balance. /// /// # Replace By Fee (RBF) /// -/// When a pending splice exists that hasn't been locked yet, use [`FundingTemplate::rbf_sync`] -/// (or [`FundingTemplate::rbf`] for async) to build an RBF contribution. This handles the -/// prior contribution logic internally — reusing an adjusted prior when possible, re-running -/// coin selection when needed, or creating a fee-bump-only contribution. +/// When a pending splice exists that hasn't been locked yet, use +/// [`FundingTemplate::rbf_prior_contribution_sync`] (or +/// [`FundingTemplate::rbf_prior_contribution`] for async) to retry the stored prior contribution +/// at an RBF-compatible feerate. To amend that prior request before building, start from +/// [`FundingTemplate::with_prior_contribution`] instead. /// /// Check [`FundingTemplate::min_rbf_feerate`] for the minimum feerate required (the greater of /// the previous feerate + 25 sat/kwu and the spec's 25/24 rule). Use -/// [`FundingTemplate::prior_contribution`] to inspect the prior -/// contribution's parameters (e.g., [`FundingContribution::value_added`], -/// [`FundingContribution::outputs`]) before deciding whether to reuse it via the RBF methods -/// or build a fresh contribution with different parameters using the splice methods above. +/// [`FundingTemplate::prior_contribution`] to inspect the stored contribution before deciding +/// whether to reuse it or replace it with a fresh request via +/// [`FundingTemplate::without_prior_contribution`]. /// /// [`ChannelManager::splice_channel`]: crate::ln::channelmanager::ChannelManager::splice_channel /// [`ChannelManager::funding_contributed`]: crate::ln::channelmanager::ChannelManager::funding_contributed @@ -264,8 +262,8 @@ impl FundingTemplate { /// Returns the minimum RBF feerate, if this template is for an RBF attempt. /// - /// When set, the `min_feerate` passed to the splice methods (e.g., - /// [`FundingTemplate::splice_in_sync`]) must be at least this value. + /// When set, the `min_feerate` passed to the splice/builder methods must be at least this + /// value. pub fn min_rbf_feerate(&self) -> Option { self.min_rbf_feerate } @@ -273,392 +271,167 @@ impl FundingTemplate { /// Returns a reference to the prior contribution from a previous splice negotiation, if /// available. /// - /// Use this to inspect the prior contribution's parameters (e.g., - /// [`FundingContribution::value_added`], [`FundingContribution::outputs`]) before deciding - /// whether to reuse it via [`FundingTemplate::rbf_sync`] or build a fresh contribution - /// with different parameters using the splice methods. + /// Use this to inspect the prior contribution's current parameters (for example, + /// [`FundingContribution::outputs`], [`FundingContribution::change_output`], and + /// [`FundingContribution::net_value`]) before deciding + /// whether to reuse it via [`FundingTemplate::rbf_prior_contribution`] or build a fresh + /// contribution with different parameters using + /// [`FundingTemplate::without_prior_contribution`]. /// /// Note: the returned contribution may reflect a different feerate than originally provided, /// as it may have been adjusted for RBF or for the counterparty's feerate when acting as - /// the acceptor. This can change other parameters too (e.g., - /// [`FundingContribution::value_added`] may be higher if the change output was removed to - /// cover a higher fee). + /// the acceptor. This can change other parameters too; for example, the amount added to the + /// channel may increase if the change output was removed to cover a higher fee. pub fn prior_contribution(&self) -> Option<&FundingContribution> { self.prior_contribution.as_ref().map(|p| &p.contribution) } -} -macro_rules! build_funding_contribution { - ($value_added:expr, $outputs:expr, $shared_input:expr, $min_rbf_feerate:expr, $feerate:expr, $max_feerate:expr, $force_coin_selection:expr, $wallet:ident, $($await:tt)*) => {{ - let value_added: Amount = $value_added; - let outputs: Vec = $outputs; - let shared_input: Option = $shared_input; - let min_rbf_feerate: Option = $min_rbf_feerate; - let feerate: FeeRate = $feerate; - let max_feerate: FeeRate = $max_feerate; - let force_coin_selection: bool = $force_coin_selection; - - let value_removed = validate_funding_contribution_params( - value_added, - &outputs, - min_rbf_feerate, - feerate, - max_feerate, - )?; - - let is_splice = shared_input.is_some(); - - let coin_selection = if value_added == Amount::ZERO && !force_coin_selection { - CoinSelection { confirmed_utxos: vec![], change_output: None } - } else { - // Used for creating a redeem script for the new funding txo, since the funding pubkeys - // are unknown at this point. Only needed when selecting which UTXOs to include in the - // funding tx that would be sufficient to pay for fees. Hence, the value doesn't matter. - let dummy_pubkey = PublicKey::from_slice(&[2; 33]).unwrap(); - - let shared_output = bitcoin::TxOut { - value: shared_input - .as_ref() - .map(|shared_input| shared_input.previous_utxo.value) - .unwrap_or(Amount::ZERO) - .checked_add(value_added) - .ok_or(FundingContributionError::InvalidSpliceValue)? - .checked_sub(value_removed) - .ok_or(FundingContributionError::InvalidSpliceValue)?, - script_pubkey: make_funding_redeemscript(&dummy_pubkey, &dummy_pubkey).to_p2wsh(), - }; - - let claim_id = None; - let must_spend = shared_input.map(|input| vec![input]).unwrap_or_default(); - if outputs.is_empty() { - let must_pay_to = &[shared_output]; - $wallet.select_confirmed_utxos(claim_id, must_spend, must_pay_to, feerate.to_sat_per_kwu() as u32, u64::MAX)$(.$await)*.map_err(|_| FundingContributionError::CoinSelectionFailed)? - } else { - let must_pay_to: Vec<_> = outputs.iter().cloned().chain(core::iter::once(shared_output)).collect(); - $wallet.select_confirmed_utxos(claim_id, must_spend, &must_pay_to, feerate.to_sat_per_kwu() as u32, u64::MAX)$(.$await)*.map_err(|_| FundingContributionError::CoinSelectionFailed)? - } - }; - - // NOTE: Must NOT fail after UTXO selection - - let CoinSelection { confirmed_utxos: inputs, change_output } = coin_selection; - - Ok(FundingContribution::new( - outputs, - inputs, - change_output, - feerate, - max_feerate, - is_splice, - )) - }}; -} - -fn validate_funding_contribution_params( - value_added: Amount, outputs: &[TxOut], min_rbf_feerate: Option, feerate: FeeRate, - max_feerate: FeeRate, -) -> Result { - if feerate > max_feerate { - return Err(FundingContributionError::FeeRateExceedsMaximum { feerate, max_feerate }); - } - - if let Some(min_rbf_feerate) = min_rbf_feerate { - if feerate < min_rbf_feerate { - return Err(FundingContributionError::FeeRateBelowRbfMinimum { - feerate, - min_rbf_feerate, - }); - } - } - - // Validate user-provided amounts are within MAX_MONEY before coin selection to - // ensure FundingContribution::net_value() arithmetic cannot overflow. With all - // amounts bounded by MAX_MONEY (~2.1e15 sat), the worst-case net_value() - // computation is -2 * MAX_MONEY (~-4.2e15), well within i64::MIN (~-9.2e18). - if value_added > Amount::MAX_MONEY { - return Err(FundingContributionError::InvalidSpliceValue); + /// Creates a [`FundingBuilder`] for constructing a contribution. + /// + /// If a prior contribution is available, the builder starts from it automatically and builder + /// mutations amend that prior request. Use [`FundingTemplate::without_prior_contribution`] to + /// start empty instead. + /// + /// `feerate` is the feerate used for fee estimation and, if wallet inputs are needed, coin + /// selection. When [`FundingTemplate::min_rbf_feerate`] is set, it must be at least that value. + /// `max_feerate` is the highest feerate we are willing to tolerate if we end up as the + /// acceptor, and must be at least `feerate`. + pub fn with_prior_contribution(self, feerate: FeeRate, max_feerate: FeeRate) -> FundingBuilder { + FundingBuilder::new(self, feerate, max_feerate) } - let mut value_removed = Amount::ZERO; - for txout in outputs.iter() { - value_removed = match value_removed.checked_add(txout.value) { - Some(sum) if sum <= Amount::MAX_MONEY => sum, - _ => return Err(FundingContributionError::InvalidSpliceValue), - }; + /// Creates a [`FundingBuilder`] for constructing a contribution without using any prior + /// contribution. + /// + /// `feerate` and `max_feerate` have the same meaning as in + /// [`FundingTemplate::with_prior_contribution`]. This is useful when an RBF template carries a + /// prior contribution but the caller wants to replace, rather than amend, that request. + pub fn without_prior_contribution( + mut self, feerate: FeeRate, max_feerate: FeeRate, + ) -> FundingBuilder { + self.prior_contribution.take(); + FundingBuilder::new(self, feerate, max_feerate) } - Ok(value_removed) -} - -impl FundingTemplate { - /// Creates a [`FundingContribution`] for adding funds to a channel using `wallet` to perform - /// coin selection. + /// Creates a [`FundingContribution`] for adding funds to a channel. /// - /// `value_added` is the total amount to add to the channel for this contribution. When - /// replacing a prior contribution via RBF, use [`FundingTemplate::prior_contribution`] to - /// inspect the prior parameters. To add funds on top of the prior contribution's amount, - /// combine them: `prior.value_added() + additional_amount`. + /// This is a convenience wrapper around [`FundingTemplate::with_prior_contribution`]. As a + /// result, if this template carries a prior contribution, `value_added` is added on top of the + /// amount that prior request was already adding to the channel instead of replacing it. Use + /// [`FundingTemplate::without_prior_contribution`] if you want to replace the prior request + /// instead. + /// + /// `value_added` is the amount of additional value to add to the channel. `min_feerate` is the + /// feerate used for fee estimation and, if needed, coin selection; when + /// [`FundingTemplate::min_rbf_feerate`] is set, it must be at least that value. `max_feerate` is + /// the highest feerate we are willing to tolerate if we end up as the acceptor, and must be at + /// least `min_feerate`. `wallet` is only consulted if the request cannot be satisfied by + /// reusing/amending the prior contribution. When this template carries a prior contribution, + /// increasing its value may therefore re-run coin selection and yield a different input set than + /// the prior contribution used. pub async fn splice_in( self, value_added: Amount, min_feerate: FeeRate, max_feerate: FeeRate, wallet: W, ) -> Result { - if value_added == Amount::ZERO { - return Err(FundingContributionError::InvalidSpliceValue); - } - let FundingTemplate { shared_input, min_rbf_feerate, .. } = self; - build_funding_contribution!( - value_added, - vec![], - shared_input, - min_rbf_feerate, - min_feerate, - max_feerate, - false, - wallet, - await - ) + self.with_prior_contribution(min_feerate, max_feerate) + .with_coin_selection_source(wallet) + .add_value(value_added) + .build() + .await } - /// Creates a [`FundingContribution`] for adding funds to a channel using `wallet` to perform - /// coin selection. + /// Creates a [`FundingContribution`] for adding funds to a channel. /// - /// See [`FundingTemplate::splice_in`] for details. + /// This is the synchronous variant of [`FundingTemplate::splice_in`]; `value_added`, + /// `min_feerate`, `max_feerate`, and `wallet` have the same meaning. pub fn splice_in_sync( self, value_added: Amount, min_feerate: FeeRate, max_feerate: FeeRate, wallet: W, ) -> Result { - if value_added == Amount::ZERO { - return Err(FundingContributionError::InvalidSpliceValue); - } - let FundingTemplate { shared_input, min_rbf_feerate, .. } = self; - build_funding_contribution!( - value_added, - vec![], - shared_input, - min_rbf_feerate, - min_feerate, - max_feerate, - false, - wallet, - ) + self.with_prior_contribution(min_feerate, max_feerate) + .with_coin_selection_source_sync(wallet) + .add_value(value_added) + .build() } /// Creates a [`FundingContribution`] for removing funds from a channel. /// - /// Fees are paid from the channel balance, so this does not perform coin selection or spend - /// wallet inputs. + /// This is a convenience wrapper around [`FundingTemplate::with_prior_contribution`] with no + /// wallet attached. For a fresh splice, fees are paid from the channel balance, so this does + /// not perform coin selection or spend wallet inputs. When a prior contribution is present, + /// `outputs` are appended to the prior [`FundingContribution::outputs`] instead of replacing + /// them. Use [`FundingTemplate::without_prior_contribution`] if you want to replace the prior + /// outputs instead. /// - /// `outputs` are the complete set of withdrawal outputs for this contribution. When - /// replacing a prior contribution via RBF, use [`FundingTemplate::prior_contribution`] to - /// inspect the prior parameters. To keep existing withdrawals and add new ones, include the - /// prior's outputs: combine [`FundingContribution::outputs`] with the new outputs. + /// `outputs` are the additional withdrawal outputs to include. `min_feerate` is the feerate + /// used for fee estimation and must be at least [`FundingTemplate::min_rbf_feerate`] when that + /// is set. `max_feerate` is the highest feerate we are willing to tolerate if we end up as the + /// acceptor, and must be at least `min_feerate`. + /// + /// If amending a prior contribution would require selecting new wallet inputs, this method + /// returns [`FundingContributionError::MissingCoinSelectionSource`]. In that case, use the + /// builder APIs with a coin selection source instead. pub fn splice_out( self, outputs: Vec, min_feerate: FeeRate, max_feerate: FeeRate, ) -> Result { - if outputs.is_empty() { - return Err(FundingContributionError::InvalidSpliceValue); - } - validate_funding_contribution_params( - Amount::ZERO, - &outputs, - self.min_rbf_feerate, - min_feerate, - max_feerate, - )?; - Ok(FundingContribution::new( - outputs, - vec![], - None, - min_feerate, - max_feerate, - self.shared_input.is_some(), - )) - } - - /// Creates a [`FundingContribution`] for both adding and removing funds from a channel using - /// `wallet` to perform coin selection. - /// - /// `value_added` and `outputs` are the complete parameters for this contribution, not - /// increments on top of a prior contribution. When replacing a prior contribution via RBF, - /// use [`FundingTemplate::prior_contribution`] to inspect the prior parameters and combine - /// them as needed. - pub async fn splice_in_and_out( - self, value_added: Amount, outputs: Vec, min_feerate: FeeRate, max_feerate: FeeRate, - wallet: W, - ) -> Result { - if value_added == Amount::ZERO && outputs.is_empty() { - return Err(FundingContributionError::InvalidSpliceValue); - } - let FundingTemplate { shared_input, min_rbf_feerate, .. } = self; - build_funding_contribution!( - value_added, - outputs, - shared_input, - min_rbf_feerate, - min_feerate, - max_feerate, - false, - wallet, - await - ) - } - - /// Creates a [`FundingContribution`] for both adding and removing funds from a channel using - /// `wallet` to perform coin selection. - /// - /// See [`FundingTemplate::splice_in_and_out`] for details. - pub fn splice_in_and_out_sync( - self, value_added: Amount, outputs: Vec, min_feerate: FeeRate, max_feerate: FeeRate, - wallet: W, - ) -> Result { - if value_added == Amount::ZERO && outputs.is_empty() { - return Err(FundingContributionError::InvalidSpliceValue); - } - let FundingTemplate { shared_input, min_rbf_feerate, .. } = self; - build_funding_contribution!( - value_added, - outputs, - shared_input, - min_rbf_feerate, - min_feerate, - max_feerate, - false, - wallet, - ) + self.with_prior_contribution(min_feerate, max_feerate).add_outputs(outputs).build() } /// Creates a [`FundingContribution`] for an RBF (Replace-By-Fee) attempt on a pending splice. /// - /// `max_feerate` is the maximum feerate the caller is willing to accept as acceptor. It is - /// used as the returned contribution's `max_feerate` and also constrains coin selection when - /// re-running it for prior contributions that cannot be adjusted or fee-bump-only - /// contributions. + /// This requires [`FundingTemplate::prior_contribution`] to be available. `feerate` overrides + /// the template's minimum RBF feerate; passing `None` uses + /// [`FundingTemplate::min_rbf_feerate`]. `max_feerate` is the highest feerate we are willing to + /// tolerate if we end up as the acceptor, and must be at least the effective feerate. `wallet` + /// is only consulted if the prior contribution cannot be reused or adjusted directly. The + /// chosen `max_feerate` is stored on the returned contribution and also constrains coin + /// selection when prior contributions cannot be adjusted in-place. /// /// This handles the prior contribution logic internally: - /// - If the prior contribution's feerate can be adjusted to the minimum RBF feerate, the + /// - If the prior contribution's feerate can be adjusted to the effective target feerate, the /// adjusted contribution is returned directly. For splice-in, the change output absorbs /// the fee difference. For splice-out (no wallet inputs), the holder's channel balance /// covers the higher fees. - /// - If adjustment fails, coin selection is re-run using the prior contribution's - /// parameters and the caller's `max_feerate`. For splice-out contributions, this changes + /// - If adjustment fails, coin selection is re-run using the prior contribution's current + /// request and the caller's `max_feerate`. For splice-out contributions, this changes /// the fee source: wallet inputs are selected to cover fees instead of deducting them /// from the channel balance. - /// - If no prior contribution exists, coin selection is run for a fee-bump-only contribution - /// (`value_added = 0`), covering fees for the common fields and shared input/output via - /// a newly selected input. Check [`FundingTemplate::prior_contribution`] to see if this - /// is intended. /// /// # Errors /// - /// Returns a [`FundingContributionError`] if this is not an RBF scenario, if `max_feerate` - /// is below the minimum RBF feerate, or if coin selection fails. - pub async fn rbf( - self, max_feerate: FeeRate, wallet: W, + /// Returns a [`FundingContributionError`] if there is no reusable prior contribution, if no + /// effective RBF feerate is available, if the effective feerate violates the template's fee + /// constraints, or if coin selection fails. + pub async fn rbf_prior_contribution( + self, feerate: Option, max_feerate: FeeRate, wallet: W, ) -> Result { - let FundingTemplate { shared_input, min_rbf_feerate, prior_contribution } = self; - let rbf_feerate = min_rbf_feerate.ok_or(FundingContributionError::NotRbfScenario)?; - if rbf_feerate > max_feerate { - return Err(FundingContributionError::FeeRateExceedsMaximum { - feerate: rbf_feerate, - max_feerate, - }); - } - - match prior_contribution { - Some(PriorContribution { contribution, holder_balance }) => { - // Try to adjust the prior contribution to the RBF feerate. This fails if - // the holder balance can't cover the adjustment (splice-out) or the fee - // buffer is insufficient (splice-in), or if the prior's feerate is already - // above rbf_feerate (e.g., from a counterparty-initiated RBF that locked - // at a higher feerate). In all cases, fall through to re-run coin selection. - if contribution - .net_value_for_initiator_at_feerate(rbf_feerate, holder_balance) - .is_ok() - { - let mut adjusted = contribution - .for_initiator_at_feerate(rbf_feerate, holder_balance) - .expect("feerate compatibility already checked"); - adjusted.max_feerate = max_feerate; - return Ok(adjusted); - } - build_funding_contribution!( - contribution.value_added(), - contribution.outputs, - shared_input, - min_rbf_feerate, - rbf_feerate, - max_feerate, - true, - wallet, - await - ) - }, - None => { - build_funding_contribution!( - Amount::ZERO, - vec![], - shared_input, - min_rbf_feerate, - rbf_feerate, - max_feerate, - true, - wallet, - await - ) - }, + if self.prior_contribution().is_none() { + return Err(FundingContributionError::NotRbfScenario); } + let feerate = feerate + .or_else(|| self.min_rbf_feerate()) + .ok_or(FundingContributionError::NotRbfScenario)?; + self.with_prior_contribution(feerate, max_feerate) + .with_coin_selection_source(wallet) + .build() + .await } /// Creates a [`FundingContribution`] for an RBF (Replace-By-Fee) attempt on a pending splice. /// - /// See [`FundingTemplate::rbf`] for details. - pub fn rbf_sync( - self, max_feerate: FeeRate, wallet: W, + /// This is the synchronous variant of [`FundingTemplate::rbf_prior_contribution`]; `feerate`, + /// `max_feerate`, and `wallet` have the same meaning. + pub fn rbf_prior_contribution_sync( + self, feerate: Option, max_feerate: FeeRate, wallet: W, ) -> Result { - let FundingTemplate { shared_input, min_rbf_feerate, prior_contribution } = self; - let rbf_feerate = min_rbf_feerate.ok_or(FundingContributionError::NotRbfScenario)?; - if rbf_feerate > max_feerate { - return Err(FundingContributionError::FeeRateExceedsMaximum { - feerate: rbf_feerate, - max_feerate, - }); + if self.prior_contribution().is_none() { + return Err(FundingContributionError::NotRbfScenario); } + let feerate = feerate + .or_else(|| self.min_rbf_feerate()) + .ok_or(FundingContributionError::NotRbfScenario)?; - match prior_contribution { - Some(PriorContribution { contribution, holder_balance }) => { - // See comment in `rbf` for details on when this adjustment fails. - if contribution - .net_value_for_initiator_at_feerate(rbf_feerate, holder_balance) - .is_ok() - { - let mut adjusted = contribution - .for_initiator_at_feerate(rbf_feerate, holder_balance) - .expect("feerate compatibility already checked"); - adjusted.max_feerate = max_feerate; - return Ok(adjusted); - } - build_funding_contribution!( - contribution.value_added(), - contribution.outputs, - shared_input, - min_rbf_feerate, - rbf_feerate, - max_feerate, - true, - wallet, - ) - }, - None => { - build_funding_contribution!( - Amount::ZERO, - vec![], - shared_input, - min_rbf_feerate, - rbf_feerate, - max_feerate, - true, - wallet, - ) - }, - } + self.with_prior_contribution(feerate, max_feerate) + .with_coin_selection_source_sync(wallet) + .build() } } @@ -788,26 +561,6 @@ impl_writeable_tlv_based!(FundingContribution, { }); impl FundingContribution { - fn new( - outputs: Vec, inputs: Vec, change_output: Option, - feerate: FeeRate, max_feerate: FeeRate, is_splice: bool, - ) -> Self { - // The caller creating a FundingContribution is always the initiator for fee estimation - // purposes — this is conservative, overestimating rather than underestimating fees if the - // node ends up as the acceptor. - let estimated_fee = estimate_transaction_fee( - &inputs, - &outputs, - change_output.as_ref(), - true, - is_splice, - feerate, - ); - debug_assert!(estimated_fee <= Amount::MAX_MONEY); - - Self { estimated_fee, inputs, outputs, change_output, feerate, max_feerate, is_splice } - } - pub(super) fn feerate(&self) -> FeeRate { self.feerate } @@ -1478,6 +1231,15 @@ impl FundingBuilder { FundingBuilder(self.0.add_output_inner(output)) } + /// Adds withdrawal outputs to the request. + /// + /// `outputs` are appended to the current set of explicit outputs. If the builder was seeded + /// from a prior contribution, this adds additional withdrawals on top of the prior outputs. + /// This does not affect any change output derived when the contribution is built. + pub fn add_outputs(self, outputs: Vec) -> Self { + FundingBuilder(self.0.add_outputs_inner(outputs)) + } + /// Removes all explicit withdrawal outputs whose script pubkey matches `script_pubkey`. /// /// This only affects outputs returned by [`FundingContribution::outputs`]; it never removes the @@ -1530,6 +1292,11 @@ impl FundingBuilderInner { self } + fn add_outputs_inner(mut self, outputs: Vec) -> Self { + self.outputs.extend(outputs); + self + } + fn remove_outputs_inner(mut self, script_pubkey: &ScriptBuf) -> Self { self.outputs.retain(|output| output.script_pubkey != *script_pubkey); self @@ -1558,6 +1325,15 @@ impl AsyncFundingBuilder { AsyncFundingBuilder(self.0.add_output_inner(output)) } + /// Adds withdrawal outputs to the request. + /// + /// `outputs` are appended to the current set of explicit outputs. If the builder was seeded + /// from a prior contribution, this adds additional withdrawals on top of the prior outputs. + /// This does not affect any change output derived when the contribution is built. + pub fn add_outputs(self, outputs: Vec) -> Self { + AsyncFundingBuilder(self.0.add_outputs_inner(outputs)) + } + /// Removes all explicit withdrawal outputs whose script pubkey matches `script_pubkey`. /// /// This only affects outputs returned by [`FundingContribution::outputs`]; it never removes the @@ -1649,6 +1425,15 @@ impl SyncFundingBuilder { SyncFundingBuilder(self.0.add_output_inner(output)) } + /// Adds withdrawal outputs to the request. + /// + /// `outputs` are appended to the current set of explicit outputs. If the builder was seeded + /// from a prior contribution, this adds additional withdrawals on top of the prior outputs. + /// This does not affect any change output derived when the contribution is built. + pub fn add_outputs(self, outputs: Vec) -> Self { + SyncFundingBuilder(self.0.add_outputs_inner(outputs)) + } + /// Removes all explicit withdrawal outputs whose script pubkey matches `script_pubkey`. /// /// This only affects outputs returned by [`FundingContribution::outputs`]; it never removes the @@ -2036,38 +1821,38 @@ mod tests { Err(FundingContributionError::InvalidSpliceValue), )); } + } - // splice_in_and_out_sync with value_added > MAX_MONEY - { - let template = FundingTemplate::new(None, None, None); - let outputs = vec![funding_output_sats(1_000)]; - assert!(matches!( - template.splice_in_and_out_sync( - over_max, - outputs, - feerate, - feerate, - UnreachableWallet - ), - Err(FundingContributionError::InvalidSpliceValue), - )); - } + #[test] + fn test_funding_builder_validates_mixed_request_max_money() { + let over_max = Amount::MAX_MONEY + Amount::from_sat(1); + let feerate = FeeRate::from_sat_per_kwu(2000); - // splice_in_and_out_sync with output sum > MAX_MONEY - { - let template = FundingTemplate::new(None, None, None); - let outputs = vec![funding_output_sats(over_max.to_sat())]; - assert!(matches!( - template.splice_in_and_out_sync( - Amount::from_sat(1_000), - outputs, - feerate, - feerate, - UnreachableWallet, - ), - Err(FundingContributionError::InvalidSpliceValue), - )); - } + // Mixed add/remove request with value_added > MAX_MONEY. + assert!(matches!( + FundingTemplate::new(None, None, None) + .without_prior_contribution(feerate, feerate) + .with_coin_selection_source_sync(UnreachableWallet) + .add_value(over_max) + .add_outputs(vec![funding_output_sats(1_000)]) + .build(), + Err(FundingContributionError::InvalidSpliceValue), + )); + + // Mixed add/remove request with outputs summing > MAX_MONEY. + let half_over = Amount::MAX_MONEY / 2 + Amount::from_sat(1); + assert!(matches!( + FundingTemplate::new(None, None, None) + .without_prior_contribution(feerate, feerate) + .with_coin_selection_source_sync(UnreachableWallet) + .add_value(Amount::from_sat(1_000)) + .add_outputs(vec![ + funding_output_sats(half_over.to_sat()), + funding_output_sats(half_over.to_sat()), + ]) + .build(), + Err(FundingContributionError::InvalidSpliceValue), + )); } #[test] @@ -2963,8 +2748,8 @@ mod tests { #[test] fn test_rbf_sync_rejects_max_feerate_below_min_rbf_feerate() { - // When the caller's max_feerate is below the minimum RBF feerate, rbf_sync should - // return Err(()). + // When the caller's max_feerate is below the minimum RBF feerate, + // rbf_prior_contribution_sync should return an error. let prior_feerate = FeeRate::from_sat_per_kwu(2000); let min_rbf_feerate = FeeRate::from_sat_per_kwu(2025); let max_feerate = FeeRate::from_sat_per_kwu(2020); @@ -2986,7 +2771,7 @@ mod tests { Some(PriorContribution::new(prior, Amount::MAX)), ); assert!(matches!( - template.rbf_sync(max_feerate, UnreachableWallet), + template.rbf_prior_contribution_sync(None, max_feerate, UnreachableWallet), Err(FundingContributionError::FeeRateExceedsMaximum { .. }), )); } @@ -2994,7 +2779,8 @@ mod tests { #[test] fn test_rbf_sync_adjusts_prior_to_rbf_feerate() { // When the prior contribution's feerate is below the minimum RBF feerate and holder - // balance is available, rbf_sync should adjust the prior to the RBF feerate. + // balance is available, rbf_prior_contribution_sync should adjust the prior to the + // RBF feerate. let prior_feerate = FeeRate::from_sat_per_kwu(2000); let min_rbf_feerate = FeeRate::from_sat_per_kwu(2025); let max_feerate = FeeRate::from_sat_per_kwu(5000); @@ -3019,11 +2805,109 @@ mod tests { Some(min_rbf_feerate), Some(PriorContribution::new(prior, Amount::MAX)), ); - let contribution = template.rbf_sync(max_feerate, UnreachableWallet).unwrap(); + let contribution = + template.rbf_prior_contribution_sync(None, max_feerate, UnreachableWallet).unwrap(); assert_eq!(contribution.feerate, min_rbf_feerate); assert_eq!(contribution.max_feerate, max_feerate); } + #[test] + fn test_rbf_sync_uses_explicit_override_feerate() { + let prior_feerate = FeeRate::from_sat_per_kwu(2000); + let min_rbf_feerate = FeeRate::from_sat_per_kwu(2025); + let override_feerate = FeeRate::from_sat_per_kwu(2100); + let max_feerate = FeeRate::from_sat_per_kwu(5000); + + let inputs = vec![funding_input_sats(100_000)]; + let change = funding_output_sats(10_000); + let estimated_fee = + estimate_transaction_fee(&inputs, &[], Some(&change), true, true, prior_feerate); + + let prior = FundingContribution { + estimated_fee, + inputs, + outputs: vec![], + change_output: Some(change), + feerate: prior_feerate, + max_feerate: FeeRate::MAX, + is_splice: true, + }; + + let template = FundingTemplate::new( + None, + Some(min_rbf_feerate), + Some(PriorContribution::new(prior, Amount::MAX)), + ); + let contribution = template + .rbf_prior_contribution_sync(Some(override_feerate), max_feerate, UnreachableWallet) + .unwrap(); + assert_eq!(contribution.feerate, override_feerate); + assert_eq!(contribution.max_feerate, max_feerate); + } + + #[test] + fn test_rbf_sync_rejects_explicit_override_below_min_rbf_feerate() { + let prior_feerate = FeeRate::from_sat_per_kwu(2000); + let min_rbf_feerate = FeeRate::from_sat_per_kwu(2025); + let override_feerate = FeeRate::from_sat_per_kwu(2024); + + let prior = FundingContribution { + estimated_fee: Amount::from_sat(1_000), + inputs: vec![funding_input_sats(100_000)], + outputs: vec![], + change_output: None, + feerate: prior_feerate, + max_feerate: FeeRate::MAX, + is_splice: true, + }; + + let template = FundingTemplate::new( + None, + Some(min_rbf_feerate), + Some(PriorContribution::new(prior, Amount::MAX)), + ); + assert!(matches!( + template.rbf_prior_contribution_sync( + Some(override_feerate), + FeeRate::MAX, + UnreachableWallet, + ), + Err(FundingContributionError::FeeRateBelowRbfMinimum { .. }), + )); + } + + #[test] + fn test_rbf_sync_rejects_explicit_override_above_max_feerate() { + let prior_feerate = FeeRate::from_sat_per_kwu(2000); + let min_rbf_feerate = FeeRate::from_sat_per_kwu(2025); + let override_feerate = FeeRate::from_sat_per_kwu(2100); + let max_feerate = FeeRate::from_sat_per_kwu(2099); + + let prior = FundingContribution { + estimated_fee: Amount::from_sat(1_000), + inputs: vec![funding_input_sats(100_000)], + outputs: vec![], + change_output: None, + feerate: prior_feerate, + max_feerate: FeeRate::MAX, + is_splice: true, + }; + + let template = FundingTemplate::new( + None, + Some(min_rbf_feerate), + Some(PriorContribution::new(prior, Amount::MAX)), + ); + assert!(matches!( + template.rbf_prior_contribution_sync( + Some(override_feerate), + max_feerate, + UnreachableWallet, + ), + Err(FundingContributionError::FeeRateExceedsMaximum { .. }), + )); + } + /// A mock wallet that returns a single UTXO for coin selection. struct SingleUtxoWallet { utxo: FundingTxInput, @@ -3059,8 +2943,8 @@ mod tests { #[test] fn test_rbf_sync_unadjusted_splice_out_runs_coin_selection() { // When the prior contribution's feerate is below the minimum RBF feerate and no - // holder balance is available, rbf_sync should run coin selection to add inputs that - // cover the higher RBF fee. + // holder balance is available, rbf_prior_contribution_sync should run coin selection to + // add inputs that cover the higher RBF fee. let prior_feerate = FeeRate::from_sat_per_kwu(2000); let min_rbf_feerate = FeeRate::from_sat_per_kwu(2025); let withdrawal = funding_output_sats(20_000); @@ -3086,40 +2970,21 @@ mod tests { change_output: Some(funding_output_sats(40_000)), }; - // rbf_sync should succeed and the contribution should have inputs from coin selection. - let contribution = template.rbf_sync(FeeRate::MAX, &wallet).unwrap(); + // rbf_prior_contribution_sync should succeed and the contribution should have inputs from + // coin selection. + let contribution = + template.rbf_prior_contribution_sync(None, FeeRate::MAX, &wallet).unwrap(); assert!(!contribution.inputs.is_empty(), "coin selection should have added inputs"); assert!(contribution.value_added() > Amount::ZERO); assert_eq!(contribution.outputs, vec![withdrawal]); assert_eq!(contribution.feerate, min_rbf_feerate); } - #[test] - fn test_rbf_sync_no_prior_fee_bump_only_runs_coin_selection() { - // When there is no prior contribution (e.g., acceptor), rbf_sync should run coin - // selection to add inputs for a fee-bump-only contribution. - let min_rbf_feerate = FeeRate::from_sat_per_kwu(2025); - - let template = - FundingTemplate::new(Some(shared_input(100_000)), Some(min_rbf_feerate), None); - - let wallet = SingleUtxoWallet { - utxo: funding_input_sats(50_000), - change_output: Some(funding_output_sats(45_000)), - }; - - let contribution = template.rbf_sync(FeeRate::MAX, &wallet).unwrap(); - assert!(!contribution.inputs.is_empty(), "coin selection should have added inputs"); - assert!(contribution.value_added() > Amount::ZERO); - assert!(contribution.outputs.is_empty()); - assert_eq!(contribution.feerate, min_rbf_feerate); - } - #[test] fn test_rbf_sync_unadjusted_uses_callers_max_feerate() { // When the prior contribution's feerate is below the minimum RBF feerate and no - // holder balance is available, rbf_sync should use the caller's max_feerate (not the - // prior's) for the resulting contribution. + // holder balance is available, rbf_prior_contribution_sync should use the caller's + // max_feerate (not the prior's) for the resulting contribution. let min_rbf_feerate = FeeRate::from_sat_per_kwu(2025); let prior_max_feerate = FeeRate::from_sat_per_kwu(50_000); let callers_max_feerate = FeeRate::from_sat_per_kwu(10_000); @@ -3146,7 +3011,8 @@ mod tests { change_output: Some(funding_output_sats(40_000)), }; - let contribution = template.rbf_sync(callers_max_feerate, &wallet).unwrap(); + let contribution = + template.rbf_prior_contribution_sync(None, callers_max_feerate, &wallet).unwrap(); assert_eq!( contribution.max_feerate, callers_max_feerate, "should use caller's max_feerate, not prior's" @@ -3155,8 +3021,9 @@ mod tests { #[test] fn test_splice_out_skips_coin_selection_during_rbf() { - // When splice_out_sync is called on a template with min_rbf_feerate set (user - // choosing a fresh splice-out instead of rbf_sync), coin selection should NOT run. + // When splice_out is called on a template with min_rbf_feerate set (user choosing a + // fresh splice-out instead of rbf_prior_contribution_sync), coin selection should NOT + // run. // Fees come from the channel balance. let min_rbf_feerate = FeeRate::from_sat_per_kwu(2025); let feerate = FeeRate::from_sat_per_kwu(2025); diff --git a/lightning/src/ln/splicing_tests.rs b/lightning/src/ln/splicing_tests.rs index 3004c76fb93..3885db54e8b 100644 --- a/lightning/src/ln/splicing_tests.rs +++ b/lightning/src/ln/splicing_tests.rs @@ -20,7 +20,7 @@ use crate::ln::channel::{ }; use crate::ln::channelmanager::{provided_init_features, PaymentId, BREAKDOWN_TIMEOUT}; use crate::ln::functional_test_utils::*; -use crate::ln::funding::FundingContribution; +use crate::ln::funding::{FundingContribution, FundingContributionError}; use crate::ln::msgs::{self, BaseMessageHandler, ChannelMessageHandler, MessageSendEvent}; use crate::ln::outbound_payment::RecipientOnionFields; use crate::ln::types::ChannelId; @@ -41,7 +41,7 @@ use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey}; use bitcoin::transaction::Version; use bitcoin::{ Amount, FeeRate, OutPoint as BitcoinOutPoint, Psbt, ScriptBuf, Transaction, TxOut, Txid, - WPubkeyHash, + WPubkeyHash, WScriptHash, }; #[test] @@ -221,7 +221,11 @@ pub fn do_initiate_rbf_splice_in_and_out<'a, 'b, 'c, 'd>( let funding_template = node.node.splice_channel(&channel_id, &node_id_counterparty).unwrap(); let wallet = WalletSync::new(Arc::clone(&node.wallet_source), node.logger); let funding_contribution = funding_template - .splice_in_and_out_sync(value_added, outputs, feerate, FeeRate::MAX, &wallet) + .without_prior_contribution(feerate, FeeRate::MAX) + .with_coin_selection_source_sync(&wallet) + .add_value(value_added) + .add_outputs(outputs) + .build() .unwrap(); node.node .funding_contributed(&channel_id, &node_id_counterparty, funding_contribution.clone(), None) @@ -269,7 +273,11 @@ pub fn do_initiate_splice_in_and_out<'a, 'b, 'c, 'd>( let feerate = funding_template.min_rbf_feerate().unwrap_or(floor_feerate); let wallet = WalletSync::new(Arc::clone(&initiator.wallet_source), initiator.logger); let funding_contribution = funding_template - .splice_in_and_out_sync(value_added, outputs, feerate, FeeRate::MAX, &wallet) + .without_prior_contribution(feerate, FeeRate::MAX) + .with_coin_selection_source_sync(&wallet) + .add_value(value_added) + .add_outputs(outputs) + .build() .unwrap(); initiator .node @@ -3654,7 +3662,7 @@ fn test_funding_contributed_splice_already_pending() { let splice_in_amount = Amount::from_sat(20_000); provide_utxo_reserves(&nodes, 2, splice_in_amount * 2); - // Use splice_in_and_out with an output so we can test output filtering + // Use the contribution builder with an output so we can test output filtering let first_splice_out = TxOut { value: Amount::from_sat(5_000), script_pubkey: ScriptBuf::new_p2wpkh(&WPubkeyHash::from_raw_hash(Hash::all_zeros())), @@ -3663,13 +3671,11 @@ fn test_funding_contributed_splice_already_pending() { let funding_template = nodes[0].node.splice_channel(&channel_id, &node_id_1).unwrap(); let wallet = WalletSync::new(Arc::clone(&nodes[0].wallet_source), nodes[0].logger); let first_contribution = funding_template - .splice_in_and_out_sync( - splice_in_amount, - vec![first_splice_out.clone()], - feerate, - FeeRate::MAX, - &wallet, - ) + .with_prior_contribution(feerate, FeeRate::MAX) + .with_coin_selection_source_sync(&wallet) + .add_value(splice_in_amount) + .add_output(first_splice_out.clone()) + .build() .unwrap(); // Initiate a second splice with a DIFFERENT output to test that different outputs @@ -3691,13 +3697,11 @@ fn test_funding_contributed_splice_already_pending() { let funding_template = nodes[0].node.splice_channel(&channel_id, &node_id_1).unwrap(); let wallet = WalletSync::new(Arc::clone(&nodes[0].wallet_source), nodes[0].logger); let second_contribution = funding_template - .splice_in_and_out_sync( - splice_in_amount, - vec![second_splice_out.clone()], - feerate, - FeeRate::MAX, - &wallet, - ) + .without_prior_contribution(feerate, FeeRate::MAX) + .with_coin_selection_source_sync(&wallet) + .add_value(splice_in_amount) + .add_output(second_splice_out.clone()) + .build() .unwrap(); // First funding_contributed - this sets up the quiescent action @@ -5507,7 +5511,8 @@ fn test_splice_rbf_after_counterparty_rbf_aborted() { nodes[0].node.get_and_clear_pending_events(); nodes[1].node.get_and_clear_pending_events(); - // Step 5: Node 1 initiates its own RBF via splice_channel → rbf_sync. + // Step 5: Node 1 initiates its own RBF via splice_channel → + // rbf_prior_contribution_sync. // The prior contribution's feerate is restored to the original floor feerate, not the // RBF-adjusted feerate. provide_utxo_reserves(&nodes, 2, added_value * 2); @@ -5521,7 +5526,8 @@ fn test_splice_rbf_after_counterparty_rbf_aborted() { ); let wallet = WalletSync::new(Arc::clone(&nodes[1].wallet_source), nodes[1].logger); - let rbf_contribution = funding_template.rbf_sync(FeeRate::MAX, &wallet); + let rbf_contribution = + funding_template.rbf_prior_contribution_sync(None, FeeRate::MAX, &wallet); assert!(rbf_contribution.is_ok()); } @@ -5722,6 +5728,128 @@ fn test_splice_rbf_sequential() { ); } +#[test] +fn test_splice_rbf_amends_prior_contribution_request() { + 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, &[None, None]); + let nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + let node_id_0 = nodes[0].node.get_our_node_id(); + let node_id_1 = nodes[1].node.get_our_node_id(); + + let (_, _, channel_id, _) = + create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, 0); + + let initial_added_value = Amount::from_sat(100_000); + let half_added_value = Amount::from_sat(initial_added_value.to_sat() / 2); + provide_utxo_reserves(&nodes, 1, Amount::from_sat(250_000)); + + let initial_contribution = + do_initiate_splice_in(&nodes[0], &nodes[1], channel_id, initial_added_value); + let (initial_inputs, _) = initial_contribution.clone().into_contributed_inputs_and_outputs(); + let (splice_tx_0, new_funding_script) = + splice_channel(&nodes[0], &nodes[1], channel_id, initial_contribution.clone()); + + let wallet = WalletSync::new(Arc::clone(&nodes[0].wallet_source), nodes[0].logger); + let first_output = TxOut { + value: Amount::from_sat(10_000), + script_pubkey: ScriptBuf::new_p2wpkh(&WPubkeyHash::from_raw_hash(Hash::all_zeros())), + }; + let second_output = TxOut { + value: Amount::from_sat(15_000), + script_pubkey: ScriptBuf::new_p2wsh(&WScriptHash::all_zeros()), + }; + + let run_rbf_round = |contribution: FundingContribution| { + nodes[0] + .node + .funding_contributed(&channel_id, &node_id_1, contribution.clone(), None) + .unwrap(); + complete_rbf_handshake(&nodes[0], &nodes[1]); + complete_interactive_funding_negotiation( + &nodes[0], + &nodes[1], + channel_id, + contribution, + new_funding_script.clone(), + ); + let (tx, splice_locked) = sign_interactive_funding_tx(&nodes[0], &nodes[1], false); + assert!(splice_locked.is_none()); + expect_splice_pending_event(&nodes[0], &node_id_1); + expect_splice_pending_event(&nodes[1], &node_id_0); + tx + }; + + let funding_template = nodes[0].node.splice_channel(&channel_id, &node_id_1).unwrap(); + assert!(funding_template.prior_contribution().unwrap().outputs().is_empty()); + let rbf_feerate = funding_template.min_rbf_feerate().unwrap(); + let contribution_1 = funding_template + .splice_out(vec![first_output.clone(), second_output.clone()], rbf_feerate, FeeRate::MAX) + .unwrap(); + let (inputs_1, _) = contribution_1.clone().into_contributed_inputs_and_outputs(); + assert_eq!(inputs_1, initial_inputs); + assert_eq!(contribution_1.outputs(), &[first_output.clone(), second_output.clone()]); + assert!(contribution_1.net_value() < initial_contribution.net_value()); + let splice_tx_1 = run_rbf_round(contribution_1.clone()); + + let funding_template = nodes[0].node.splice_channel(&channel_id, &node_id_1).unwrap(); + assert_eq!(funding_template.prior_contribution().unwrap().outputs(), contribution_1.outputs()); + let rbf_feerate = funding_template.min_rbf_feerate().unwrap(); + let contribution_2 = funding_template + .with_prior_contribution(rbf_feerate, FeeRate::MAX) + .with_coin_selection_source_sync(&wallet) + .remove_value(half_added_value) + .build() + .unwrap(); + let (inputs_2, _) = contribution_2.clone().into_contributed_inputs_and_outputs(); + assert_eq!(inputs_2, initial_inputs); + assert_eq!(contribution_2.outputs(), contribution_1.outputs()); + assert!(contribution_2.net_value() < contribution_1.net_value()); + let splice_tx_2 = run_rbf_round(contribution_2.clone()); + + let funding_template = nodes[0].node.splice_channel(&channel_id, &node_id_1).unwrap(); + assert_eq!(funding_template.prior_contribution().unwrap().outputs(), contribution_2.outputs()); + let rbf_feerate = funding_template.min_rbf_feerate().unwrap(); + let contribution_3 = funding_template + .with_prior_contribution(rbf_feerate, FeeRate::MAX) + .remove_outputs(&first_output.script_pubkey) + .build() + .unwrap(); + let (inputs_3, _) = contribution_3.clone().into_contributed_inputs_and_outputs(); + assert_eq!(inputs_3, initial_inputs); + assert_eq!(contribution_3.outputs(), std::slice::from_ref(&second_output)); + assert!(contribution_3.net_value() > contribution_2.net_value()); + let splice_tx_3 = run_rbf_round(contribution_3.clone()); + + let funding_template = nodes[0].node.splice_channel(&channel_id, &node_id_1).unwrap(); + assert_eq!(funding_template.prior_contribution().unwrap().outputs(), contribution_3.outputs()); + let contribution_4 = + funding_template.rbf_prior_contribution_sync(None, FeeRate::MAX, &wallet).unwrap(); + let (inputs_4, _) = contribution_4.clone().into_contributed_inputs_and_outputs(); + assert_eq!(inputs_4, initial_inputs); + assert_eq!(contribution_4.outputs(), contribution_3.outputs()); + assert_eq!(contribution_4.net_value(), contribution_3.net_value()); + assert!( + contribution_4.change_output().unwrap().value + < contribution_3.change_output().unwrap().value + ); + let rbf_tx_final = run_rbf_round(contribution_4); + + lock_rbf_splice_after_blocks( + &nodes[0], + &nodes[1], + &rbf_tx_final, + ANTI_REORG_DELAY - 1, + &[ + splice_tx_0.compute_txid(), + splice_tx_1.compute_txid(), + splice_tx_2.compute_txid(), + splice_tx_3.compute_txid(), + ], + ); +} + #[test] fn test_splice_rbf_acceptor_contributes_then_disconnects() { // When both nodes contribute to a splice and the initiator RBFs (with the acceptor @@ -5974,9 +6102,9 @@ fn test_splice_channel_with_pending_splice_includes_rbf_floor() { assert_eq!(funding_template.min_rbf_feerate(), Some(expected_floor)); assert!(funding_template.prior_contribution().is_some()); - // rbf_sync returns the Adjusted prior contribution directly. + // rbf_prior_contribution_sync returns the adjusted prior contribution directly. let wallet = WalletSync::new(Arc::clone(&nodes[0].wallet_source), nodes[0].logger); - assert!(funding_template.rbf_sync(FeeRate::MAX, &wallet).is_ok()); + assert!(funding_template.rbf_prior_contribution_sync(None, FeeRate::MAX, &wallet).is_ok()); } #[test] @@ -6177,8 +6305,8 @@ fn test_funding_contributed_rbf_adjustment_insufficient_budget() { #[test] fn test_prior_contribution_unadjusted_when_max_feerate_too_low() { - // Test that rbf_sync re-runs coin selection when the prior contribution's max_feerate is - // too low to accommodate the minimum RBF feerate. + // Test that rbf_prior_contribution_sync re-runs coin selection when the prior + // contribution's max_feerate is too low to accommodate the minimum RBF feerate. 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, &[None, None]); @@ -6208,13 +6336,13 @@ fn test_prior_contribution_unadjusted_when_max_feerate_too_low() { let (_splice_tx, _) = splice_channel(&nodes[0], &nodes[1], channel_id, funding_contribution); // Call splice_channel again — the minimum RBF feerate (floor + 25 sat/kwu) exceeds the prior - // contribution's max_feerate (floor), so adjustment fails. rbf_sync re-runs coin selection - // with the caller's max_feerate. + // contribution's max_feerate (floor), so adjustment fails. + // rbf_prior_contribution_sync re-runs coin selection with the caller's max_feerate. let funding_template = nodes[0].node.splice_channel(&channel_id, &node_id_1).unwrap(); assert!(funding_template.min_rbf_feerate().is_some()); assert!(funding_template.prior_contribution().is_some()); let wallet = WalletSync::new(Arc::clone(&nodes[0].wallet_source), nodes[0].logger); - assert!(funding_template.rbf_sync(FeeRate::MAX, &wallet).is_ok()); + assert!(funding_template.rbf_prior_contribution_sync(None, FeeRate::MAX, &wallet).is_ok()); } #[test] @@ -6255,17 +6383,19 @@ fn test_splice_channel_during_negotiation_includes_rbf_feerate() { let expected_floor = FeeRate::from_sat_per_kwu(FEERATE_FLOOR_SATS_PER_KW as u64 + 25); assert_eq!(template.min_rbf_feerate(), Some(expected_floor)); - // No prior contribution since there are no negotiated candidates yet. rbf_sync runs - // fee-bump-only coin selection. + // No prior contribution since there are no negotiated candidates yet, so RBF is rejected. assert!(template.prior_contribution().is_none()); let wallet = WalletSync::new(Arc::clone(&nodes[0].wallet_source), nodes[0].logger); - assert!(template.rbf_sync(FeeRate::MAX, &wallet).is_ok()); + assert!(matches!( + template.rbf_prior_contribution_sync(None, FeeRate::MAX, &wallet), + Err(FundingContributionError::NotRbfScenario) + )); } #[test] fn test_rbf_sync_returns_err_when_no_min_rbf_feerate() { - // Test that rbf_sync returns Err(()) when there is no pending splice (min_rbf_feerate is - // None), indicating this is not an RBF scenario. + // Test that rbf_prior_contribution_sync returns `NotRbfScenario` when there is no pending + // splice (min_rbf_feerate is None). 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, &[None, None]); @@ -6287,15 +6417,15 @@ fn test_rbf_sync_returns_err_when_no_min_rbf_feerate() { let wallet = WalletSync::new(Arc::clone(&nodes[0].wallet_source), nodes[0].logger); assert!(matches!( - template.rbf_sync(FeeRate::MAX, &wallet), + template.rbf_prior_contribution_sync(None, FeeRate::MAX, &wallet), Err(crate::ln::funding::FundingContributionError::NotRbfScenario), )); } #[test] fn test_rbf_sync_returns_err_when_max_feerate_below_min_rbf() { - // Test that rbf_sync returns Err when the caller's max_feerate is below the minimum - // RBF feerate. + // Test that rbf_prior_contribution_sync returns an error when the caller's max_feerate is + // below the minimum RBF feerate. 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, &[None, None]); @@ -6323,7 +6453,7 @@ fn test_rbf_sync_returns_err_when_max_feerate_below_min_rbf() { FeeRate::from_sat_per_kwu(min_rbf_feerate.to_sat_per_kwu().saturating_sub(1)); let wallet = WalletSync::new(Arc::clone(&nodes[0].wallet_source), nodes[0].logger); assert!(matches!( - funding_template.rbf_sync(too_low_feerate, &wallet), + funding_template.rbf_prior_contribution_sync(None, too_low_feerate, &wallet), Err(crate::ln::funding::FundingContributionError::FeeRateExceedsMaximum { .. }), )); } @@ -6594,9 +6724,12 @@ fn test_splice_rbf_rejects_own_low_feerate_after_several_attempts() { let rbf_feerate = FeeRate::from_sat_per_kwu(next_feerate); let funding_template = nodes[0].node.splice_channel(&channel_id, &node_id_1).unwrap(); let wallet = WalletSync::new(Arc::clone(&nodes[0].wallet_source), nodes[0].logger); - let contribution = - funding_template.splice_in_sync(added_value, rbf_feerate, FeeRate::MAX, &wallet).unwrap(); - + let contribution = funding_template + .without_prior_contribution(rbf_feerate, FeeRate::MAX) + .with_coin_selection_source_sync(&wallet) + .add_value(added_value) + .build() + .unwrap(); let result = nodes[0].node.funding_contributed(&channel_id, &node_id_1, contribution, None); assert!(result.is_err(), "Expected rejection for low feerate: {:?}", result);