From d69304732a97c0aeaea1b82a3d9f178e77ad1479 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Wed, 4 Jun 2025 16:17:44 +0000 Subject: [PATCH] Don't generate a commitment if we cannot afford a holding cell feerate Upon releasing an `update_fee` from its holding cell, it can be dropped and not sent to the peer in case we no longer can afford the new feerate. When this happens, we previously still sent a commitment to the peer, which could break the spec if no other updates were sent to the peer. Now, when a fee update gets dropped from its holding cell, we also do not produce a commitment if no other updates are to be sent to the peer. --- lightning/src/ln/channel.rs | 9 +- lightning/src/ln/channelmanager.rs | 2 +- lightning/src/ln/update_fee_tests.rs | 184 +++++++++++++++++++++++++++ 3 files changed, 188 insertions(+), 7 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 7f33937f2dd..cfd8b39f814 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -6275,14 +6275,11 @@ impl FundedChannel where } } } - if update_add_count == 0 && update_fulfill_count == 0 && update_fail_count == 0 && self.context.holding_cell_update_fee.is_none() { + let update_fee = self.context.holding_cell_update_fee.take().and_then(|feerate| self.send_update_fee(feerate, false, fee_estimator, logger)); + + if update_add_count == 0 && update_fulfill_count == 0 && update_fail_count == 0 && update_fee.is_none() { return (None, htlcs_to_fail); } - let update_fee = if let Some(feerate) = self.context.holding_cell_update_fee.take() { - self.send_update_fee(feerate, false, fee_estimator, logger) - } else { - None - }; let mut additional_update = self.build_commitment_no_status_check(logger); // build_commitment_no_status_check and get_update_fulfill_htlc may bump latest_monitor_id diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 8a0061f8c6b..4b501a5d798 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -6586,7 +6586,7 @@ where NotifyOption::DoPersist } - #[cfg(fuzzing)] + #[cfg(any(test, fuzzing, feature = "_externalize_tests"))] /// In chanmon_consistency we want to sometimes do the channel fee updates done in /// timer_tick_occurred, but we can't generate the disabled channel updates as it considers /// these a fuzz failure (as they usually indicate a channel force-close, which is exactly what diff --git a/lightning/src/ln/update_fee_tests.rs b/lightning/src/ln/update_fee_tests.rs index 1512444c830..464b1b543f4 100644 --- a/lightning/src/ln/update_fee_tests.rs +++ b/lightning/src/ln/update_fee_tests.rs @@ -1009,3 +1009,187 @@ pub fn accept_busted_but_better_fee() { _ => panic!("Unexpected event"), }; } + +#[xtest(feature = "_externalize_tests")] +pub fn cannot_afford_on_holding_cell_release() { + do_cannot_afford_on_holding_cell_release(ChannelTypeFeatures::only_static_remote_key(), true); + do_cannot_afford_on_holding_cell_release(ChannelTypeFeatures::only_static_remote_key(), false); + do_cannot_afford_on_holding_cell_release( + ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies(), + true, + ); + do_cannot_afford_on_holding_cell_release( + ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies(), + false, + ); +} + +pub fn do_cannot_afford_on_holding_cell_release( + channel_type_features: ChannelTypeFeatures, can_afford: bool, +) { + // Test that if we can't afford a feerate update when releasing an + // update_fee from its holding cell, we do not generate any msg events + let chanmon_cfgs = create_chanmon_cfgs(2); + + let mut default_config = test_default_channel_config(); + default_config.channel_handshake_config.max_inbound_htlc_value_in_flight_percent_of_channel = + 100; + if channel_type_features.supports_anchors_zero_fee_htlc_tx() { + default_config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true; + default_config.manually_accept_inbound_channels = true; + } + + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let node_chanmgrs = + create_node_chanmgrs(2, &node_cfgs, &[Some(default_config.clone()), Some(default_config)]); + + let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + let node_a_id = nodes[0].node.get_our_node_id(); + let node_b_id = nodes[1].node.get_our_node_id(); + + let target_feerate = 1000; + let expected_tx_fee_sat = + chan_utils::commit_tx_fee_sat(target_feerate, 1, &channel_type_features); + // This is the number of htlcs that `send_update_fee` will account for when checking whether + // it can afford the new feerate upon releasing an update_fee from its holding cell, + // ie the buffer + the inbound HTLC we will add while the update_fee is in the holding cell + let buffer_htlcs = crate::ln::channel::CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize + 1; + let buffer_tx_fee_sat = + chan_utils::commit_tx_fee_sat(target_feerate, buffer_htlcs, &channel_type_features); + let anchor_value_satoshis = if channel_type_features.supports_anchors_zero_fee_htlc_tx() { + 2 * crate::ln::channel::ANCHOR_OUTPUT_VALUE_SATOSHI + } else { + 0 + }; + let channel_reserve_satoshis = 1000; + + let channel_value_sat = 100_000; + let node_0_balance_sat = buffer_tx_fee_sat + anchor_value_satoshis + channel_reserve_satoshis + - if can_afford { 0 } else { 1 }; + let node_1_balance_sat = channel_value_sat - node_0_balance_sat; + + let chan_id = + create_chan_between_nodes_with_value(&nodes[0], &nodes[1], channel_value_sat, 0).3; + + // Set node 0's balance to the can/can't afford threshold + send_payment(&nodes[0], &[&nodes[1]], node_1_balance_sat * 1000); + + { + // Sanity check the reserve + let per_peer_state_lock; + let mut peer_state_lock; + let chan = + get_channel_ref!(nodes[1], nodes[0], per_peer_state_lock, peer_state_lock, chan_id); + assert_eq!( + chan.funding().holder_selected_channel_reserve_satoshis, + channel_reserve_satoshis + ); + } + + { + // Bump the feerate + let mut feerate_lock = chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap(); + *feerate_lock = target_feerate; + } + + // Put the update fee into the holding cell of node 0 + + nodes[0].node.maybe_update_chan_fees(); + + // While the update_fee is in the holding cell, add an inbound HTLC + + let (route, payment_hash, _, payment_secret) = + get_route_and_payment_hash!(nodes[1], nodes[0], 5000 * 1000); + let onion = RecipientOnionFields::secret_only(payment_secret); + let id = PaymentId(payment_hash.0); + nodes[1].node.send_payment_with_route(route, payment_hash, onion, id).unwrap(); + check_added_monitors(&nodes[1], 1); + + let payment_event = { + let mut events_1 = nodes[1].node.get_and_clear_pending_msg_events(); + assert_eq!(events_1.len(), 1); + SendEvent::from_event(events_1.pop().unwrap()) + }; + assert_eq!(payment_event.node_id, node_a_id); + assert_eq!(payment_event.msgs.len(), 1); + + nodes[0].node.handle_update_add_htlc(node_b_id, &payment_event.msgs[0]); + nodes[0].node.handle_commitment_signed(node_b_id, &payment_event.commitment_msg[0]); + check_added_monitors(&nodes[0], 1); + + let (revoke_ack, commitment_signed) = get_revoke_commit_msgs!(nodes[0], node_b_id); + + nodes[1].node.handle_revoke_and_ack(node_a_id, &revoke_ack); + check_added_monitors(&nodes[1], 1); + nodes[1].node.handle_commitment_signed(node_a_id, &commitment_signed[0]); + check_added_monitors(&nodes[1], 1); + + let mut events = nodes[1].node.get_and_clear_pending_msg_events(); + assert_eq!(events.len(), 1); + + if let MessageSendEvent::SendRevokeAndACK { node_id, msg } = events.pop().unwrap() { + assert_eq!(node_id, node_a_id); + nodes[0].node.handle_revoke_and_ack(node_b_id, &msg); + check_added_monitors!(nodes[0], 1); + } else { + panic!(); + } + + let events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 1); + assert!(matches!(events[0], Event::PendingHTLCsForwardable { .. })); + + // Release the update_fee from its holding cell + + let mut events = nodes[0].node.get_and_clear_pending_msg_events(); + if can_afford { + // We could afford the update_fee, sanity check everything + assert_eq!(events.len(), 1); + if let MessageSendEvent::UpdateHTLCs { node_id, channel_id, updates } = + events.pop().unwrap() + { + assert_eq!(node_id, node_b_id); + assert_eq!(channel_id, chan_id); + assert_eq!(updates.commitment_signed.len(), 1); + assert_eq!(updates.commitment_signed[0].htlc_signatures.len(), 1); + assert_eq!(updates.update_add_htlcs.len(), 0); + assert_eq!(updates.update_fulfill_htlcs.len(), 0); + assert_eq!(updates.update_fail_htlcs.len(), 0); + assert_eq!(updates.update_fail_malformed_htlcs.len(), 0); + let update_fee = updates.update_fee.unwrap(); + assert_eq!(update_fee.channel_id, chan_id); + assert_eq!(update_fee.feerate_per_kw, target_feerate); + + nodes[1].node.handle_update_fee(node_a_id, &update_fee); + commitment_signed_dance!(nodes[1], nodes[0], updates.commitment_signed, false); + + // Confirm the feerate on node 0's commitment transaction + { + let commitment_tx = get_local_commitment_txn!(nodes[0], channel_id)[0].clone(); + + let mut actual_fee = + commitment_tx.output.iter().fold(0, |acc, output| acc + output.value.to_sat()); + actual_fee = channel_value_sat - actual_fee; + assert_eq!(expected_tx_fee_sat, actual_fee); + } + + // Confirm the feerate on node 1's commitment transaction + { + let commitment_tx = get_local_commitment_txn!(nodes[1], channel_id)[0].clone(); + + let mut actual_fee = + commitment_tx.output.iter().fold(0, |acc, output| acc + output.value.to_sat()); + actual_fee = channel_value_sat - actual_fee; + assert_eq!(expected_tx_fee_sat, actual_fee); + } + } else { + panic!(); + } + } else { + // We could not afford the update_fee, no events should be generated + assert_eq!(events.len(), 0); + let err = format!("Cannot afford to send new feerate at {}", target_feerate); + nodes[0].logger.assert_log("lightning::ln::channel", err, 1); + } +}