From 767fe3971246cfed975d4fdbdf95e9b72ca3c3ed Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Wed, 8 Apr 2026 17:24:48 +0000 Subject: [PATCH 1/2] Error if the calculated v2 reserve is greater than the channel value In 0FC channels, capping the reserve to the total value of the channel allowed a splice initiator to withdraw past their reserve in case the acceptor had no balance in the channel. This is because the post-splice value of the channel was equal to the initiator's post splice balance. Hence, this post splice balance always matched the reserve, even though the reserve was below the dust limit. The only thing that prevented the initiator from withdrawing all their balance was the script dust limit check in `interactivetxs::NegotiationContext::receive_tx_add_output`. In case the splice acceptor had any balance in the channel, or there were HTLCs in the channel, or the channel was not 0FC, the splice initiator's post-splice balance was always below the full channel value. Hence when the reserve was capped at the channel value, the post-splice balance was always below the reserve, and the splice was rejected. Also, in `validate_splice_contributions`, to determine the `counterparty_selected_channel_reserve`, we now read the holder's dust limit from the context, instead of the current global constant. --- lightning/src/ln/channel.rs | 56 +++++-- lightning/src/ln/splicing_tests.rs | 247 ++++++++++++++++++++++++++++- lightning/src/sign/tx_builder.rs | 80 ++++++---- 3 files changed, 333 insertions(+), 50 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index e07ee7fceab..b8a74af3a0c 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2752,16 +2752,14 @@ impl FundingScope { ) -> Result { if our_funding_contribution.unsigned_abs() > Amount::MAX_MONEY { return Err(format!( - "Channel {} cannot be spliced; our {} contribution exceeds the total bitcoin supply", - context.channel_id(), + "Our {} contribution exceeds the total bitcoin supply", our_funding_contribution, )); } if their_funding_contribution.unsigned_abs() > Amount::MAX_MONEY { return Err(format!( - "Channel {} cannot be spliced; their {} contribution exceeds the total bitcoin supply", - context.channel_id(), + "Their {} contribution exceeds the total bitcoin supply", their_funding_contribution, )); } @@ -2821,17 +2819,20 @@ impl FundingScope { // New reserve values are based on the new channel value and are v2-specific let counterparty_selected_channel_reserve_satoshis = get_v2_channel_reserve_satoshis( post_channel_value_sat, - MIN_CHAN_DUST_LIMIT_SATOSHIS, + context.holder_dust_limit_satoshis, prev_funding .counterparty_selected_channel_reserve_satoshis .expect("counterparty reserve is set") == 0, - ); + ) + .map_err(|()| format!("The post-splice channel value {post_channel_value_sat} is smaller than our dust limit {MIN_CHAN_DUST_LIMIT_SATOSHIS}"))?; + let their_dust_limit_satoshis = context.counterparty_dust_limit_satoshis; let holder_selected_channel_reserve_satoshis = get_v2_channel_reserve_satoshis( post_channel_value_sat, - context.counterparty_dust_limit_satoshis, + their_dust_limit_satoshis, prev_funding.holder_selected_channel_reserve_satoshis == 0, - ); + ) + .map_err(|()| format!("The post-splice channel value {post_channel_value_sat} is smaller than their dust limit {their_dust_limit_satoshis}"))?; Ok(Self { channel_transaction_parameters: post_channel_transaction_parameters, @@ -3384,6 +3385,9 @@ pub(super) struct ChannelContext { /// We use this to close if funding is never broadcasted. pub(super) channel_creation_height: u32, + #[cfg(any(test, feature = "_test_utils"))] + pub(crate) counterparty_dust_limit_satoshis: u64, + #[cfg(not(any(test, feature = "_test_utils")))] counterparty_dust_limit_satoshis: u64, #[cfg(any(test, feature = "_test_utils"))] @@ -6776,19 +6780,24 @@ pub(crate) fn get_legacy_default_holder_selected_channel_reserve_satoshis( /// Returns a minimum channel reserve value each party needs to maintain, fixed in the spec to a /// default of 1% of the total channel value. /// -/// Guaranteed to return a value no larger than channel_value_satoshis +/// Guaranteed to return a value no larger than `channel_value_satoshis` /// /// This is used both for outbound and inbound channels and has lower bound /// of `dust_limit_satoshis`. +/// +/// Returns `Err` if `channel_value_satoshis` is smaller than `dust_limit_satoshis`. pub(crate) fn get_v2_channel_reserve_satoshis( channel_value_satoshis: u64, dust_limit_satoshis: u64, is_0reserve: bool, -) -> u64 { +) -> Result { + if channel_value_satoshis < dust_limit_satoshis { + return Err(()); + } if is_0reserve { - return 0; + return Ok(0); } // Fixed at 1% of channel value by spec. let (q, _) = channel_value_satoshis.overflowing_div(100); - cmp::min(channel_value_satoshis, cmp::max(q, dust_limit_satoshis)) + Ok(cmp::max(q, dust_limit_satoshis)) } /// Returns the minimum feerate for RBF attempts given a previous feerate. @@ -12824,7 +12833,8 @@ where their_funding_contribution, counterparty_funding_pubkey, our_new_holder_keys, - )?; + ) + .map_err(|e| format!("Channel {} cannot be spliced; {}", self.context.channel_id(), e))?; let (post_splice_holder_balance, post_splice_counterparty_balance) = self.get_holder_counterparty_balances_floor_incl_fee(&candidate_scope).map_err( @@ -15117,8 +15127,13 @@ impl PendingV2Channel { }); let holder_selected_channel_reserve_satoshis = get_v2_channel_reserve_satoshis( - funding_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS, trusted_channel_features.is_some_and(|f| f.is_0reserve())); - + funding_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS, trusted_channel_features.is_some_and(|f| f.is_0reserve()) + ).map_err(|()| APIError::APIMisuseError { + err: format!( + "The channel value {funding_satoshis} is smaller than their dust \ + limit {MIN_CHAN_DUST_LIMIT_SATOSHIS}" + ) + })?; let funding_feerate_sat_per_1000_weight = fee_estimator.bounded_sat_per_1000_weight(funding_confirmation_target); let funding_tx_locktime = LockTime::from_height(current_chain_height) .map_err(|_| APIError::APIMisuseError { @@ -15257,9 +15272,16 @@ impl PendingV2Channel { let channel_value_satoshis = our_funding_contribution_sats.saturating_add(msg.common_fields.funding_satoshis); let counterparty_selected_channel_reserve_satoshis = get_v2_channel_reserve_satoshis( - channel_value_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS, msg.disable_channel_reserve.is_some()); + channel_value_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS, msg.disable_channel_reserve.is_some() + ).map_err(|()| ChannelError::close(format!( + "The channel value {channel_value_satoshis} is smaller than our dust limit {MIN_CHAN_DUST_LIMIT_SATOSHIS}" + )))?; + let their_dust_limit_satoshis = msg.common_fields.dust_limit_satoshis; let holder_selected_channel_reserve_satoshis = get_v2_channel_reserve_satoshis( - channel_value_satoshis, msg.common_fields.dust_limit_satoshis, trusted_channel_features.is_some_and(|f| f.is_0reserve())); + channel_value_satoshis, their_dust_limit_satoshis, trusted_channel_features.is_some_and(|f| f.is_0reserve()) + ).map_err(|()| ChannelError::close(format!( + "The channel value {channel_value_satoshis} is smaller than their dust limit {their_dust_limit_satoshis}" + )))?; let channel_type = channel_type_from_open_channel(&msg.common_fields, our_supported_features)?; diff --git a/lightning/src/ln/splicing_tests.rs b/lightning/src/ln/splicing_tests.rs index a5361358653..0de7574d416 100644 --- a/lightning/src/ln/splicing_tests.rs +++ b/lightning/src/ln/splicing_tests.rs @@ -7865,13 +7865,15 @@ fn do_test_0reserve_splice_counterparty_validation( // They obviously can't afford their contribution, so we fail before even // querying `TxBuilder` format!( - "Got non-closing error: Their contribution candidate {funding_contribution_sat}sat \ + "Got non-closing error: Channel {channel_id} cannot be spliced; \ + Their contribution candidate {funding_contribution_sat}sat \ is greater than their total balance in the channel {initiator_value_to_self_sat}sat" ) } else if post_channel_value_sat < MIN_CHANNEL_VALUE_SATOSHIS { // We require all spliced channels to have a value of at least 1000 satoshis after the splice format!( - "Got non-closing error: Spliced channel value must be at least {MIN_CHANNEL_VALUE_SATOSHIS} satoshis. \ + "Got non-closing error: Channel {channel_id} cannot be spliced; \ + Spliced channel value must be at least {MIN_CHANNEL_VALUE_SATOSHIS} satoshis. \ It would be {post_channel_value_sat}" ) } else { @@ -7888,3 +7890,244 @@ fn do_test_0reserve_splice_counterparty_validation( channel_type } + +/// We previously allowed a splice initiator to splice out funds past their channel reserve if the +/// the acceptor had no balance in the channel, and there were no HTLCs in the channel +#[cfg(test)] +enum AcceptorBalance { + NoBalance, + BalanceInHTLC, + SettledBalance, +} + +#[cfg(test)] +enum ValidationCase { + Passes, + FailsAtHolder, + FailsAtCounterparty, +} + +#[test] +fn test_splice_out_initiator_reserve_breach_zero_fee_commitments() { + do_test_splice_out_initiator_reserve_breach_zero_fee_commitments( + AcceptorBalance::NoBalance, + ValidationCase::Passes, + ); + do_test_splice_out_initiator_reserve_breach_zero_fee_commitments( + AcceptorBalance::BalanceInHTLC, + ValidationCase::Passes, + ); + do_test_splice_out_initiator_reserve_breach_zero_fee_commitments( + AcceptorBalance::SettledBalance, + ValidationCase::Passes, + ); + + // We used to fail this case here + do_test_splice_out_initiator_reserve_breach_zero_fee_commitments( + AcceptorBalance::NoBalance, + ValidationCase::FailsAtHolder, + ); + + do_test_splice_out_initiator_reserve_breach_zero_fee_commitments( + AcceptorBalance::BalanceInHTLC, + ValidationCase::FailsAtHolder, + ); + do_test_splice_out_initiator_reserve_breach_zero_fee_commitments( + AcceptorBalance::SettledBalance, + ValidationCase::FailsAtHolder, + ); + + // We used to fail this case here + do_test_splice_out_initiator_reserve_breach_zero_fee_commitments( + AcceptorBalance::NoBalance, + ValidationCase::FailsAtCounterparty, + ); + + do_test_splice_out_initiator_reserve_breach_zero_fee_commitments( + AcceptorBalance::BalanceInHTLC, + ValidationCase::FailsAtCounterparty, + ); + do_test_splice_out_initiator_reserve_breach_zero_fee_commitments( + AcceptorBalance::SettledBalance, + ValidationCase::FailsAtCounterparty, + ); +} + +#[cfg(test)] +fn do_test_splice_out_initiator_reserve_breach_zero_fee_commitments( + acceptor_balance: AcceptorBalance, validation_case: ValidationCase, +) { + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let mut config = test_default_channel_config(); + // This reserve breach was only possible in 0FC channels + config.channel_handshake_config.negotiate_anchor_zero_fee_commitments = true; + config.channel_handshake_config.our_htlc_minimum_msat = 1; + let node_chanmgrs = + create_node_chanmgrs(2, &node_cfgs, &[Some(config.clone()), Some(config.clone())]); + let nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + // Node 0 is initiator, node 1 is acceptor + let _node_id_0 = nodes[0].node.get_our_node_id(); + let _node_id_1 = nodes[1].node.get_our_node_id(); + + let channel_value_sat = 100_000; + let node_1_settled_balance_msat = + if matches!(acceptor_balance, AcceptorBalance::SettledBalance) { 1 } else { 0 }; + let node_1_htlc_balance_msat = + if matches!(acceptor_balance, AcceptorBalance::BalanceInHTLC) { 1 } else { 0 }; + let node_0_balance_msat = + channel_value_sat * 1000 - node_1_settled_balance_msat - node_1_htlc_balance_msat; + + // Bump initiator's dust limit to the highest value we allow in anchor channels + let high_dust_limit_satoshis = 10_000; + + let (_, _, channel_id, _tx) = create_announced_chan_between_nodes_with_value( + &nodes, + 0, + 1, + channel_value_sat, + node_1_settled_balance_msat, + ); + + if matches!(acceptor_balance, AcceptorBalance::BalanceInHTLC) { + let _ = route_payment(&nodes[0], &[&nodes[1]], node_1_htlc_balance_msat); + } + + { + let per_peer_lock; + let mut peer_state_lock; + let channel = + get_channel_ref!(nodes[0], nodes[1], per_peer_lock, peer_state_lock, channel_id); + if let Some(chan) = channel.as_funded_mut() { + chan.context.holder_dust_limit_satoshis = high_dust_limit_satoshis; + } else { + panic!("Unexpected Channel phase"); + } + } + + { + let per_peer_lock; + let mut peer_state_lock; + let channel = + get_channel_ref!(nodes[1], nodes[0], per_peer_lock, peer_state_lock, channel_id); + if let Some(chan) = channel.as_funded_mut() { + chan.context.counterparty_dust_limit_satoshis = high_dust_limit_satoshis; + } else { + panic!("Unexpected Channel phase"); + } + } + + if matches!(validation_case, ValidationCase::Passes) { + let node_0_balance_leftover_amount = Amount::from_sat(high_dust_limit_satoshis); + // Estimated fees of a splice_out at 253sat/kw + let estimated_fees = 183; + // Note in 0FC we've got no fee spike buffer, no commit tx fee, no anchors + let splice_out_output_sat = + node_0_balance_msat / 1000 - node_0_balance_leftover_amount.to_sat() - estimated_fees; + let splice_out_output_amount = Amount::from_sat(splice_out_output_sat); + let outputs = vec![TxOut { + value: splice_out_output_amount, + script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(), + }]; + let contribution = initiate_splice_out(&nodes[0], &nodes[1], channel_id, outputs).unwrap(); + + let (splice_tx, _) = splice_channel(&nodes[0], &nodes[1], channel_id, contribution); + mine_transaction(&nodes[0], &splice_tx); + mine_transaction(&nodes[1], &splice_tx); + lock_splice_after_blocks(&nodes[0], &nodes[1], ANTI_REORG_DELAY - 1); + } else { + let node_0_balance_leftover_amount = Amount::from_sat(high_dust_limit_satoshis - 1); + // Note in 0FC we've got no fee spike buffer, no commit tx fee, no anchors + let funding_contribution_sat = + -((node_0_balance_msat / 1000 - node_0_balance_leftover_amount.to_sat()) as i64); + let value = if matches!(validation_case, ValidationCase::FailsAtHolder) { + Amount::from_sat(funding_contribution_sat.unsigned_abs() - 183) + } else if matches!(validation_case, ValidationCase::FailsAtCounterparty) { + // Splice out some dummy amount to get past the initiator's validation, + // we'll modify the message in-flight. + Amount::from_sat(1000) + } else { + panic!("Unexpected test case"); + }; + let outputs = vec![TxOut { + value, + script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(), + }]; + let contribution = initiate_splice_out(&nodes[0], &nodes[1], channel_id, outputs); + + if matches!(validation_case, ValidationCase::FailsAtHolder) { + assert_eq!( + contribution.unwrap_err(), + APIError::APIMisuseError { + err: format!("Channel {channel_id} cannot accept funding contribution"), + } + ); + let splice_out_value = value + Amount::from_sat(183); + let splice_out_max = splice_out_value - Amount::ONE_SAT; + let cannot_splice_out = format!( + "Channel {channel_id} cannot be funded: \ + Our splice-out value of {splice_out_value} is greater than the \ + maximum {splice_out_max}" + ); + nodes[0].logger.assert_log("lightning::ln::channel", cannot_splice_out, 1); + return; + } + + // The dummy contribution should have passed the holder's validation + assert!(contribution.is_ok()); + + // When acceptor has no balance, the reserve the initiator should keep should remain + // clamped at its dust limit. We previously allowed the initiator to withdraw past + // this point. + let v2_channel_reserve = Amount::from_sat(high_dust_limit_satoshis); + + let initiator = &nodes[0]; + let acceptor = &nodes[1]; + let node_id_initiator = initiator.node.get_our_node_id(); + let node_id_acceptor = acceptor.node.get_our_node_id(); + + let stfu_init = get_event_msg!(initiator, MessageSendEvent::SendStfu, node_id_acceptor); + acceptor.node.handle_stfu(node_id_initiator, &stfu_init); + let stfu_ack = get_event_msg!(acceptor, MessageSendEvent::SendStfu, node_id_initiator); + initiator.node.handle_stfu(node_id_acceptor, &stfu_ack); + + let mut splice_init = + get_event_msg!(initiator, MessageSendEvent::SendSpliceInit, node_id_acceptor); + // Make the modification here, acceptor should now complain. If the acceptor has no + // balance, we previously would not complain. + splice_init.funding_contribution_satoshis = funding_contribution_sat; + acceptor.node.handle_splice_init(node_id_initiator, &splice_init); + let msg_events = acceptor.node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 1); + if let MessageSendEvent::HandleError { action, .. } = &msg_events[0] { + assert!(matches!(action, msgs::ErrorAction::DisconnectPeerWithWarning { .. })); + } else { + panic!("Expected MessageSendEvent::HandleError"); + } + let post_splice_channel_value_sat = node_0_balance_leftover_amount.to_sat(); + let cannot_splice_out = if matches!(acceptor_balance, AcceptorBalance::NoBalance) { + format!( + "Got non-closing error: Channel {channel_id} cannot \ + be spliced; The post-splice channel value {post_splice_channel_value_sat} \ + is smaller than their dust limit {high_dust_limit_satoshis}" + ) + } else { + // As soon as we've pushed any sats out of our balance, the channel value + // is now at the dust limit, so we don't complain when determining the new + // dust limits, but later when we check the balances against those new + // dust limits + assert_eq!( + channel_value_sat.checked_add_signed(funding_contribution_sat).unwrap(), + high_dust_limit_satoshis + ); + format!( + "Got non-closing error: Channel {channel_id} cannot \ + be spliced out; their post-splice channel balance \ + {node_0_balance_leftover_amount} is smaller than our selected v2 reserve \ + {v2_channel_reserve}" + ) + }; + acceptor.logger.assert_log("lightning::ln::channelmanager", cannot_splice_out, 1); + } +} diff --git a/lightning/src/sign/tx_builder.rs b/lightning/src/sign/tx_builder.rs index ffb01c571b7..f9a990e8c94 100644 --- a/lightning/src/sign/tx_builder.rs +++ b/lightning/src/sign/tx_builder.rs @@ -365,7 +365,8 @@ fn get_next_splice_out_maximum_sat( channel_value_satoshis, channel_constraints.holder_dust_limit_satoshis, false, - ); + ) + .unwrap(); // If the holder cannot splice out anything, they must be at or // below the v2 reserve debug_assert!(current_balance_sat <= v2_reserve_sat); @@ -374,7 +375,8 @@ fn get_next_splice_out_maximum_sat( channel_value_satoshis.saturating_sub(max_splice_out_sat), channel_constraints.holder_dust_limit_satoshis, false, - ); + ) + .unwrap(); // If the holder can splice out some maximum, splicing out that // maximum lands them at exactly the new v2 reserve + the // `post_splice_delta_above_reserve_sat` @@ -382,40 +384,56 @@ fn get_next_splice_out_maximum_sat( local_balance_before_fee_sat.saturating_sub(max_splice_out_sat), post_splice_reserve_sat.saturating_add(post_splice_delta_above_reserve_sat) ); + // Splice out an additional satoshi, and check that we are offside + let offside_splice_out_sat = max_splice_out_sat + 1; + let post_splice_reserve_sat_result = get_v2_channel_reserve_satoshis( + channel_value_satoshis.saturating_sub(offside_splice_out_sat), + channel_constraints.holder_dust_limit_satoshis, + false, + ); + match post_splice_reserve_sat_result { + Ok(reserve) => debug_assert!( + local_balance_before_fee_sat.saturating_sub(offside_splice_out_sat) + < reserve.saturating_add(post_splice_delta_above_reserve_sat) + ), + Err(()) => (), + } } max_splice_out_sat } else { // In a zero-reserve channel, the holder is free to withdraw up to its `post_splice_delta_above_reserve_sat` - local_balance_before_fee_sat.saturating_sub(post_splice_delta_above_reserve_sat) - }; + let mut next_splice_out_maximum_sat = + local_balance_before_fee_sat.saturating_sub(post_splice_delta_above_reserve_sat); - // We only bother to check the local commitment here, the counterparty will check its own commitment. - // - // If the current `next_splice_out_maximum_sat` would produce a local commitment with no - // outputs, bump this maximum such that, after the splice, the holder's balance covers at - // least `dust_limit_satoshis` and, if they are the funder, `current_spiked_tx_fee_sat`. - // We don't include an additional non-dust inbound HTLC in the `current_spiked_tx_fee_sat`, - // because we don't mind if the holder dips below their dust limit to cover the fee for that - // inbound non-dust HTLC. - if !has_output( - is_outbound_from_holder, - local_balance_before_fee_msat.saturating_sub(next_splice_out_maximum_sat * 1000), - remote_balance_before_fee_msat, - spiked_feerate, - spiked_feerate_nondust_htlc_count, - channel_constraints.holder_dust_limit_satoshis, - channel_type, - ) { - let dust_limit_satoshis = channel_constraints.holder_dust_limit_satoshis; - let current_spiked_tx_fee_sat = commit_tx_fee_sat(spiked_feerate, 0, channel_type); - let min_balance_sat = if is_outbound_from_holder { - dust_limit_satoshis.saturating_add(current_spiked_tx_fee_sat) - } else { - dust_limit_satoshis - }; - next_splice_out_maximum_sat = - (local_balance_before_fee_msat / 1000).saturating_sub(min_balance_sat); - } + // We only bother to check the local commitment here, the counterparty will check its own commitment. + // + // If the current `next_splice_out_maximum_sat` would produce a local commitment with no + // outputs, bump this maximum such that, after the splice, the holder's balance covers at + // least `dust_limit_satoshis` and, if they are the funder, `current_spiked_tx_fee_sat`. + // We don't include an additional non-dust inbound HTLC in the `current_spiked_tx_fee_sat`, + // because we don't mind if the holder dips below their dust limit to cover the fee for that + // inbound non-dust HTLC. + if !has_output( + is_outbound_from_holder, + local_balance_before_fee_msat.saturating_sub(next_splice_out_maximum_sat * 1000), + remote_balance_before_fee_msat, + spiked_feerate, + spiked_feerate_nondust_htlc_count, + channel_constraints.holder_dust_limit_satoshis, + channel_type, + ) { + let dust_limit_satoshis = channel_constraints.holder_dust_limit_satoshis; + let current_spiked_tx_fee_sat = commit_tx_fee_sat(spiked_feerate, 0, channel_type); + let min_balance_sat = if is_outbound_from_holder { + dust_limit_satoshis.saturating_add(current_spiked_tx_fee_sat) + } else { + dust_limit_satoshis + }; + next_splice_out_maximum_sat = + (local_balance_before_fee_msat / 1000).saturating_sub(min_balance_sat); + } + next_splice_out_maximum_sat + }; if channel_value_satoshis < next_splice_out_maximum_sat + MIN_CHANNEL_VALUE_SATOSHIS { next_splice_out_maximum_sat = From 966946554604d5ff77a782b90db924505dd2e25c Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Thu, 30 Apr 2026 00:08:49 +0000 Subject: [PATCH 2/2] Error if the calculated v1 reserve is greater than the channel value We made the same change to the calculation of the v2 reserve in the previous commit. --- lightning/src/ln/channel.rs | 81 ++++++++++++++++----- lightning/src/ln/channel_open_tests.rs | 14 +++- lightning/src/ln/functional_tests.rs | 3 +- lightning/src/ln/htlc_reserve_unit_tests.rs | 15 ++-- lightning/src/ln/payment_tests.rs | 2 +- lightning/src/ln/update_fee_tests.rs | 7 +- 6 files changed, 89 insertions(+), 33 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index b8a74af3a0c..e59855d2892 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -6750,20 +6750,32 @@ fn get_legacy_default_holder_max_htlc_value_in_flight_msat(channel_value_satoshi /// This is used both for outbound and inbound channels and has lower bound /// of `MIN_THEIR_CHAN_RESERVE_SATOSHIS`, and the `dust_limit_satoshis` of /// the counterparty. +/// +/// Returns `Err` if `channel_value_satoshis` is smaller than +/// `MIN_THEIR_CHAN_RESERVE_SATOSHIS` or the `dust_limit_satoshis` of the +/// counterparty. pub(crate) fn get_holder_selected_channel_reserve_satoshis( channel_value_satoshis: u64, their_dust_limit_satoshis: u64, config: &UserConfig, is_0reserve: bool, -) -> u64 { +) -> Result { + if channel_value_satoshis < MIN_THEIR_CHAN_RESERVE_SATOSHIS + || channel_value_satoshis < their_dust_limit_satoshis + { + return Err(()); + } if is_0reserve { - return 0; + return Ok(0); } - let counterparty_chan_reserve_prop_mil = - config.channel_handshake_config.their_channel_reserve_proportional_millionths as u64; + // As described in the `ChannelHandshakeConfig` docs, we cap this value at 1_000_000. + let counterparty_chan_reserve_prop_mil = cmp::min( + config.channel_handshake_config.their_channel_reserve_proportional_millionths as u64, + 1_000_000, + ); let calculated_reserve = channel_value_satoshis.saturating_mul(counterparty_chan_reserve_prop_mil) / 1_000_000; let channel_reserve_satoshis = cmp::max(calculated_reserve, MIN_THEIR_CHAN_RESERVE_SATOSHIS); let channel_reserve_satoshis = cmp::max(channel_reserve_satoshis, their_dust_limit_satoshis); - cmp::min(channel_value_satoshis, channel_reserve_satoshis) + Ok(channel_reserve_satoshis) } /// This is for legacy reasons, present for forward-compatibility. @@ -14468,12 +14480,19 @@ impl OutboundV1Channel { // a dust limit higher than our selected reserve. let their_dust_limit_satoshis = 0; let is_0reserve = trusted_channel_features.is_some_and(|f| f.is_0reserve()); - let holder_selected_channel_reserve_satoshis = get_holder_selected_channel_reserve_satoshis( - channel_value_satoshis, - their_dust_limit_satoshis, - config, - is_0reserve, - ); + let holder_selected_channel_reserve_satoshis = + get_holder_selected_channel_reserve_satoshis( + channel_value_satoshis, + their_dust_limit_satoshis, + config, + is_0reserve, + ) + .map_err(|()| APIError::APIMisuseError { + err: format!( + "The channel value {channel_value_satoshis} is smaller than \ + {MIN_THEIR_CHAN_RESERVE_SATOSHIS}" + ), + })?; if holder_selected_channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS && !is_0reserve { // Protocol level safety check in place, although it should never happen because // of `MIN_THEIR_CHAN_RESERVE_SATOSHIS` and `MIN_CHANNEL_VALUE_SATOSHIS` @@ -14865,12 +14884,20 @@ impl InboundV1Channel { let channel_type = channel_type_from_open_channel(&msg.common_fields, our_supported_features)?; - let holder_selected_channel_reserve_satoshis = get_holder_selected_channel_reserve_satoshis( - msg.common_fields.funding_satoshis, - msg.common_fields.dust_limit_satoshis, - config, - trusted_channel_features.is_some_and(|f| f.is_0reserve()), - ); + let holder_selected_channel_reserve_satoshis = + get_holder_selected_channel_reserve_satoshis( + msg.common_fields.funding_satoshis, + msg.common_fields.dust_limit_satoshis, + config, + trusted_channel_features.is_some_and(|f| f.is_0reserve()), + ) + .map_err(|()| { + ChannelError::close(format!( + "The channel value {} is smaller than either their dust \ + limit {}, or {MIN_THEIR_CHAN_RESERVE_SATOSHIS}", + msg.common_fields.funding_satoshis, msg.common_fields.dust_limit_satoshis, + )) + })?; let counterparty_pubkeys = ChannelPublicKeys { funding_pubkey: msg.common_fields.funding_pubkey, revocation_basepoint: RevocationBasepoint::from(msg.common_fields.revocation_basepoint), @@ -17472,6 +17499,10 @@ mod tests { // to channel value test_self_and_counterparty_channel_reserve(10_000_000, 0.50, 0.50); test_self_and_counterparty_channel_reserve(10_000_000, 0.60, 0.50); + + // Make sure we correctly handle reserves greater than the channel value + test_self_and_counterparty_channel_reserve(100_000, 1.1, 0.30); + test_self_and_counterparty_channel_reserve(100_000, 0.30, 1.1); } #[rustfmt::skip] @@ -17491,7 +17522,19 @@ mod tests { outbound_node_config.channel_handshake_config.their_channel_reserve_proportional_millionths = (outbound_selected_channel_reserve_perc * 1_000_000.0) as u32; let mut chan = OutboundV1Channel::<&TestKeysInterface>::new(&&fee_est, &&keys_provider, &&keys_provider, outbound_node_id, &channelmanager::provided_init_features(&outbound_node_config), channel_value_satoshis, 100_000, 42, &outbound_node_config, 0, 42, None, &logger, None).unwrap(); - let expected_outbound_selected_chan_reserve = cmp::max(MIN_THEIR_CHAN_RESERVE_SATOSHIS, (chan.funding.get_value_satoshis() as f64 * outbound_selected_channel_reserve_perc) as u64); + let outbound_capped_reserve_perc = if outbound_selected_channel_reserve_perc.lt(&1.0) { + outbound_selected_channel_reserve_perc + } else { + 1.0 + }; + + let inbound_capped_reserve_perc = if inbound_selected_channel_reserve_perc.lt(&1.0) { + inbound_selected_channel_reserve_perc + } else { + 1.0 + }; + + let expected_outbound_selected_chan_reserve = cmp::max(MIN_THEIR_CHAN_RESERVE_SATOSHIS, (chan.funding.get_value_satoshis() as f64 * outbound_capped_reserve_perc) as u64); assert_eq!(chan.funding.holder_selected_channel_reserve_satoshis, expected_outbound_selected_chan_reserve); let chan_open_channel_msg = chan.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap(); @@ -17501,7 +17544,7 @@ mod tests { if outbound_selected_channel_reserve_perc + inbound_selected_channel_reserve_perc < 1.0 { let chan_inbound_node = InboundV1Channel::<&TestKeysInterface>::new(&&fee_est, &&keys_provider, &&keys_provider, inbound_node_id, &channelmanager::provided_channel_type_features(&inbound_node_config), &channelmanager::provided_init_features(&outbound_node_config), &chan_open_channel_msg, 7, &inbound_node_config, 0, &&logger, None).unwrap(); - let expected_inbound_selected_chan_reserve = cmp::max(MIN_THEIR_CHAN_RESERVE_SATOSHIS, (chan.funding.get_value_satoshis() as f64 * inbound_selected_channel_reserve_perc) as u64); + let expected_inbound_selected_chan_reserve = cmp::max(MIN_THEIR_CHAN_RESERVE_SATOSHIS, (chan.funding.get_value_satoshis() as f64 * inbound_capped_reserve_perc) as u64); assert_eq!(chan_inbound_node.funding.holder_selected_channel_reserve_satoshis, expected_inbound_selected_chan_reserve); assert_eq!(chan_inbound_node.funding.counterparty_selected_channel_reserve_satoshis.unwrap(), expected_outbound_selected_chan_reserve); diff --git a/lightning/src/ln/channel_open_tests.rs b/lightning/src/ln/channel_open_tests.rs index ac4a1b67994..50ef0721e07 100644 --- a/lightning/src/ln/channel_open_tests.rs +++ b/lightning/src/ln/channel_open_tests.rs @@ -16,7 +16,8 @@ use crate::chain::{self, ChannelMonitorUpdateStatus}; use crate::events::{ClosureReason, Event, FundingInfo}; use crate::ln::channel::{ get_holder_selected_channel_reserve_satoshis, ChannelError, InboundV1Channel, - OutboundV1Channel, COINBASE_MATURITY, UNFUNDED_CHANNEL_AGE_LIMIT_TICKS, + OutboundV1Channel, COINBASE_MATURITY, MIN_THEIR_CHAN_RESERVE_SATOSHIS, + UNFUNDED_CHANNEL_AGE_LIMIT_TICKS, }; use crate::ln::channelmanager::{ self, TrustedChannelFeatures, BREAKDOWN_TIMEOUT, MAX_UNFUNDED_CHANNEL_PEERS, @@ -473,7 +474,8 @@ pub fn test_insane_channel_opens() { // funding satoshis let channel_value_sat = 31337; // same as funding satoshis let channel_reserve_satoshis = - get_holder_selected_channel_reserve_satoshis(channel_value_sat, 0, &legacy_cfg, false); + get_holder_selected_channel_reserve_satoshis(channel_value_sat, 0, &legacy_cfg, false) + .unwrap(); let push_msat = (channel_value_sat - channel_reserve_satoshis) * 1000; // Have node0 initiate a channel to node1 with aforementioned parameters @@ -552,7 +554,13 @@ pub fn test_insane_channel_opens() { }, ); - insane_open_helper("Peer never wants payout outputs?", |mut msg| { + let crazy_dust_limit = channel_value_sat + 1; + let expected_error_str = format!( + "Got non-closing error: The channel value \ + {channel_value_sat} is smaller than either their dust limit {crazy_dust_limit}, or \ + {MIN_THEIR_CHAN_RESERVE_SATOSHIS}" + ); + insane_open_helper(&expected_error_str, |mut msg| { msg.common_fields.dust_limit_satoshis = msg.common_fields.funding_satoshis + 1; msg }); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index c8ecb40fa6d..8bbb9b99479 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -415,7 +415,8 @@ pub fn test_inbound_outbound_capacity_is_not_zero() { assert_eq!(channels0.len(), 1); assert_eq!(channels1.len(), 1); - let reserve = get_holder_selected_channel_reserve_satoshis(100_000, 0, &default_config, false); + let reserve = + get_holder_selected_channel_reserve_satoshis(100_000, 0, &default_config, false).unwrap(); assert_eq!(channels0[0].inbound_capacity_msat, 95000000 - reserve * 1000); assert_eq!(channels1[0].outbound_capacity_msat, 95000000 - reserve * 1000); diff --git a/lightning/src/ln/htlc_reserve_unit_tests.rs b/lightning/src/ln/htlc_reserve_unit_tests.rs index 45d3cf5950f..a4d92b7a045 100644 --- a/lightning/src/ln/htlc_reserve_unit_tests.rs +++ b/lightning/src/ln/htlc_reserve_unit_tests.rs @@ -55,8 +55,9 @@ fn do_test_counterparty_no_reserve(send_from_initiator: bool) { push_amt -= feerate_per_kw as u64 * (commitment_tx_base_weight(&channel_type_features) + 4 * COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000 * 1000; - push_amt -= - get_holder_selected_channel_reserve_satoshis(100_000, 0, &default_config, false) * 1000; + push_amt -= get_holder_selected_channel_reserve_satoshis(100_000, 0, &default_config, false) + .unwrap() + * 1000; let push = if send_from_initiator { 0 } else { push_amt }; let temp_channel_id = @@ -1002,8 +1003,9 @@ pub fn test_chan_reserve_violation_outbound_htlc_inbound_chan() { &channel_type_features, ); - push_amt -= - get_holder_selected_channel_reserve_satoshis(100_000, 0, &default_config, false) * 1000; + push_amt -= get_holder_selected_channel_reserve_satoshis(100_000, 0, &default_config, false) + .unwrap() + * 1000; let _ = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, push_amt); @@ -1048,8 +1050,9 @@ pub fn test_chan_reserve_dust_inbound_htlcs_outbound_chan() { MIN_AFFORDABLE_HTLC_COUNT as u64, &channel_type_features, ); - push_amt -= - get_holder_selected_channel_reserve_satoshis(100_000, 0, &default_config, false) * 1000; + push_amt -= get_holder_selected_channel_reserve_satoshis(100_000, 0, &default_config, false) + .unwrap() + * 1000; create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, push_amt); let (htlc_success_tx_fee_sat, _) = diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 5b4f5f93d71..ccb933a95d7 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -5043,7 +5043,7 @@ fn test_htlc_forward_considers_anchor_outputs_value() { create_announced_chan_between_nodes_with_value(&nodes, 1, 2, CHAN_AMT, PUSH_MSAT); let channel_reserve_msat = - get_holder_selected_channel_reserve_satoshis(CHAN_AMT, 0, &config, false) * 1000; + get_holder_selected_channel_reserve_satoshis(CHAN_AMT, 0, &config, false).unwrap() * 1000; let commitment_fee_msat = chan_utils::commit_tx_fee_sat( *nodes[1].fee_estimator.sat_per_kw.lock().unwrap(), 2, diff --git a/lightning/src/ln/update_fee_tests.rs b/lightning/src/ln/update_fee_tests.rs index b1f8257088e..1cb04f13a33 100644 --- a/lightning/src/ln/update_fee_tests.rs +++ b/lightning/src/ln/update_fee_tests.rs @@ -410,7 +410,7 @@ pub fn do_test_update_fee_that_funder_cannot_afford(channel_type_features: Chann let channel_id = chan.2; let secp_ctx = Secp256k1::new(); let bs_channel_reserve_sats = - get_holder_selected_channel_reserve_satoshis(channel_value, 0, &cfg, false); + get_holder_selected_channel_reserve_satoshis(channel_value, 0, &cfg, false).unwrap(); let (anchor_outputs_value_sats, outputs_num_no_htlcs) = if channel_type_features.supports_anchors_zero_fee_htlc_tx() { (ANCHOR_OUTPUT_VALUE_SATOSHI * 2, 4) @@ -886,8 +886,9 @@ pub fn test_chan_init_feerate_unaffordability() { // During open, we don't have a "counterparty channel reserve" to check against, so that // requirement only comes into play on the open_channel handling side. - push_amt -= - get_holder_selected_channel_reserve_satoshis(100_000, 0, &default_config, false) * 1000; + push_amt -= get_holder_selected_channel_reserve_satoshis(100_000, 0, &default_config, false) + .unwrap() + * 1000; nodes[0].node.create_channel(node_b_id, 100_000, push_amt, 42, None, None).unwrap(); let mut open_channel_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, node_b_id);