From 2290141ed9323957ef45041b1ab1cf2776efc577 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 14 May 2023 23:34:35 +0000 Subject: [PATCH 01/10] Disallow sending an HTLC when the balance needed is pending removal While its nice to be able to push an HTLC which spends balance that is removed in our local commitment transaction but awaiting an RAA from our peer for final removal its by no means a critical feature. Because peers should really be sending RAAs quickly after we send a commitment, this should be an exceedingly rare case, and we already don't expose this as available balance when routing, so this isn't even made available when sending, only forwarding. Note that `test_pending_claimed_htlc_no_balance_underflow` is removed as it tested a case which was only possible because of this and now is no longer possible. --- lightning/src/ln/channel.rs | 16 ++++++++---- lightning/src/ln/functional_tests.rs | 39 ---------------------------- 2 files changed, 11 insertions(+), 44 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 11f0261d677..829ce3a9976 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -5857,14 +5857,13 @@ impl Channel { return Err(ChannelError::Ignore(format!("Cannot send value that would put us over the max HTLC value in flight our peer will accept ({})", self.counterparty_max_htlc_value_in_flight_msat))); } - let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number); - let commitment_stats = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, true, logger); if !self.is_outbound() { // Check that we won't violate the remote channel reserve by adding this HTLC. let htlc_candidate = HTLCCandidate::new(amount_msat, HTLCInitiator::LocalOffered); let counterparty_commit_tx_fee_msat = self.next_remote_commit_tx_fee_msat(htlc_candidate, None); let holder_selected_chan_reserve_msat = self.holder_selected_channel_reserve_satoshis * 1000; - if commitment_stats.remote_balance_msat < counterparty_commit_tx_fee_msat + holder_selected_chan_reserve_msat { + let remote_balance_msat = (self.channel_value_satoshis * 1000 - self.value_to_self_msat).saturating_sub(inbound_stats.pending_htlcs_value_msat); + if remote_balance_msat < counterparty_commit_tx_fee_msat + holder_selected_chan_reserve_msat { return Err(ChannelError::Ignore("Cannot send value that would put counterparty balance under holder-announced channel reserve value".to_owned())); } } @@ -5894,7 +5893,8 @@ impl Channel { } } - let holder_balance_msat = commitment_stats.local_balance_msat - outbound_stats.holding_cell_msat; + let holder_balance_msat = self.value_to_self_msat + .saturating_sub(outbound_stats.pending_htlcs_value_msat); if holder_balance_msat < amount_msat { return Err(ChannelError::Ignore(format!("Cannot send value that would overdraw remaining funds. Amount: {}, pending value to self {}", amount_msat, holder_balance_msat))); } @@ -5915,7 +5915,13 @@ impl Channel { return Err(ChannelError::Ignore(format!("Cannot send value that would put our balance under counterparty-announced channel reserve value ({})", chan_reserve_msat))); } - if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateInProgress as u32)) != 0 { + let need_holding_cell = (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateInProgress as u32)) != 0; + log_debug!(logger, "Pushing new outbound HTLC for {} msat {}", amount_msat, + if force_holding_cell { "into holding cell" } + else if need_holding_cell { "into holding cell as we're awaiting an RAA or monitor" } + else { "to peer" }); + + if need_holding_cell { force_holding_cell = true; } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 98fc4bd95b6..c811cf2f7fc 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -7618,45 +7618,6 @@ fn test_bump_txn_sanitize_tracking_maps() { } } -#[test] -fn test_pending_claimed_htlc_no_balance_underflow() { - // Tests that if we have a pending outbound HTLC as well as a claimed-but-not-fully-removed - // HTLC we will not underflow when we call `Channel::get_balance_msat()`. - 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); - create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, 0); - - let (payment_preimage, payment_hash, _) = route_payment(&nodes[0], &[&nodes[1]], 1_010_000); - nodes[1].node.claim_funds(payment_preimage); - expect_payment_claimed!(nodes[1], payment_hash, 1_010_000); - check_added_monitors!(nodes[1], 1); - let fulfill_ev = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); - - nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &fulfill_ev.update_fulfill_htlcs[0]); - expect_payment_sent_without_paths!(nodes[0], payment_preimage); - nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &fulfill_ev.commitment_signed); - check_added_monitors!(nodes[0], 1); - let (_raa, _cs) = get_revoke_commit_msgs!(nodes[0], nodes[1].node.get_our_node_id()); - - // At this point nodes[1] has received 1,010k msat (10k msat more than their reserve) and can - // send an HTLC back (though it will go in the holding cell). Send an HTLC back and check we - // can get our balance. - - // Get a route from nodes[1] to nodes[0] by getting a route going the other way and then flip - // the public key of the only hop. This works around ChannelDetails not showing the - // almost-claimed HTLC as available balance. - let (mut route, _, _, _) = get_route_and_payment_hash!(nodes[0], nodes[1], 10_000); - route.payment_params = None; // This is all wrong, but unnecessary - route.paths[0].hops[0].pubkey = nodes[0].node.get_our_node_id(); - let (_, payment_hash_2, payment_secret_2) = get_payment_preimage_hash!(nodes[0]); - nodes[1].node.send_payment_with_route(&route, payment_hash_2, - RecipientOnionFields::secret_only(payment_secret_2), PaymentId(payment_hash_2.0)).unwrap(); - - assert_eq!(nodes[1].node.list_channels()[0].balance_msat, 1_000_000); -} - #[test] fn test_channel_conf_timeout() { // Tests that, for inbound channels, we give up on them if the funding transaction does not From 4b900b7e08b388611501049562d95f6b1dcc0c29 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 15 May 2023 02:24:17 +0000 Subject: [PATCH 02/10] Simplify test_fail_holding_cell_htlc_upon_free_multihop somewhat In the coming commits we redo our next-HTLC-available logic which requires some minor test changes for tests which relied on calculating routes which were not usable. Here we do a minor prefactor to simplify a test which now no longer requires later changes. --- lightning/src/ln/functional_tests.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index c811cf2f7fc..92a6e2695a0 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -5878,10 +5878,10 @@ fn test_free_and_fail_holding_cell_htlcs() { fn test_fail_holding_cell_htlc_upon_free_multihop() { let chanmon_cfgs = create_chanmon_cfgs(3); let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); - // When this test was written, the default base fee floated based on the HTLC count. - // It is now fixed, so we simply set the fee to the expected value here. + // Avoid having to include routing fees in calculations let mut config = test_default_channel_config(); - config.channel_config.forwarding_fee_base_msat = 196; + config.channel_config.forwarding_fee_base_msat = 0; + config.channel_config.forwarding_fee_proportional_millionths = 0; let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[Some(config.clone()), Some(config.clone()), Some(config.clone())]); let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); let chan_0_1 = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 95000000); @@ -5913,9 +5913,7 @@ fn test_fail_holding_cell_htlc_upon_free_multihop() { let opt_anchors = get_opt_anchors!(nodes[0], nodes[1], chan_0_1.2); // Send a payment which passes reserve checks but gets stuck in the holding cell. - let feemsat = 239; - let total_routing_fee_msat = (nodes.len() - 2) as u64 * feemsat; - let max_can_send = 5000000 - channel_reserve - 2*commit_tx_fee_msat(feerate, 1 + 1, opt_anchors) - total_routing_fee_msat; + let max_can_send = 5000000 - channel_reserve - 2*commit_tx_fee_msat(feerate, 1 + 1, opt_anchors); let (route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], max_can_send); let payment_event = { nodes[0].node.send_payment_with_route(&route, our_payment_hash, From e43cfbd5f16c0c1d0a62a499dd356335e6eef4b9 Mon Sep 17 00:00:00 2001 From: Gleb Naumenko Date: Tue, 7 Mar 2023 10:01:05 +0200 Subject: [PATCH 03/10] Consider commitment tx fee while assembling a route When calculating the amount available to send for the next HTLC, if we over-count we may create routes which are not actually usable. Historically this has been an issue, which we resolve over a few commits. Here we include the cost of the commitment transaction fee in our calculation, subtracting the commitment tx fee cost from the available as we do in `send_payment`. We also add some testing when sending to ensure that send failures are accounted for in our balance calculations. This commit is based on original work by Gleb Naumenko and modified by Matt Corallo . --- lightning/src/ln/channel.rs | 48 +++++++++++++++++++++++++--- lightning/src/ln/functional_tests.rs | 26 +++++++++++---- 2 files changed, 62 insertions(+), 12 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 829ce3a9976..bda0bfa509c 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2686,10 +2686,45 @@ impl Channel { } balance_msat -= outbound_stats.pending_htlcs_value_msat; - let outbound_capacity_msat = cmp::max(self.value_to_self_msat as i64 - - outbound_stats.pending_htlcs_value_msat as i64 - - self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0) as i64 * 1000, - 0) as u64; + let outbound_capacity_msat = self.value_to_self_msat + .saturating_sub(outbound_stats.pending_htlcs_value_msat) + .saturating_sub( + self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0) * 1000); + + let mut available_capacity_msat = outbound_capacity_msat; + + if self.is_outbound() { + // We should mind channel commit tx fee when computing how much of the available capacity + // can be used in the next htlc. Mirrors the logic in send_htlc. + // + // The fee depends on whether the amount we will be sending is above dust or not, + // and the answer will in turn change the amount itself — making it a circular + // dependency. + // This complicates the computation around dust-values, up to the one-htlc-value. + let mut real_dust_limit_timeout_sat = self.holder_dust_limit_satoshis; + if !self.opt_anchors() { + real_dust_limit_timeout_sat += self.feerate_per_kw as u64 * htlc_timeout_tx_weight(false) / 1000; + } + + let htlc_above_dust = HTLCCandidate::new(real_dust_limit_timeout_sat * 1000, HTLCInitiator::LocalOffered); + let max_reserved_commit_tx_fee_msat = FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE * self.next_local_commit_tx_fee_msat(htlc_above_dust, Some(())); + let htlc_dust = HTLCCandidate::new(real_dust_limit_timeout_sat * 1000 - 1, HTLCInitiator::LocalOffered); + let min_reserved_commit_tx_fee_msat = FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE * self.next_local_commit_tx_fee_msat(htlc_dust, Some(())); + + // We will first subtract the fee as if we were above-dust. Then, if the resulting + // value ends up being below dust, we have this fee available again. In that case, + // match the value to right-below-dust. + let mut capacity_minus_commitment_fee_msat: i64 = (available_capacity_msat as i64) - (max_reserved_commit_tx_fee_msat as i64); + if capacity_minus_commitment_fee_msat < (real_dust_limit_timeout_sat as i64) * 1000 { + let one_htlc_difference_msat = max_reserved_commit_tx_fee_msat - min_reserved_commit_tx_fee_msat; + debug_assert!(one_htlc_difference_msat != 0); + capacity_minus_commitment_fee_msat += one_htlc_difference_msat as i64; + capacity_minus_commitment_fee_msat = cmp::min(real_dust_limit_timeout_sat as i64 * 1000 - 1, capacity_minus_commitment_fee_msat); + available_capacity_msat = cmp::max(0, cmp::min(capacity_minus_commitment_fee_msat, available_capacity_msat as i64)) as u64; + } else { + available_capacity_msat = capacity_minus_commitment_fee_msat as u64; + } + } AvailableBalances { inbound_capacity_msat: cmp::max(self.channel_value_satoshis as i64 * 1000 - self.value_to_self_msat as i64 @@ -2697,7 +2732,7 @@ impl Channel { - self.holder_selected_channel_reserve_satoshis as i64 * 1000, 0) as u64, outbound_capacity_msat, - next_outbound_htlc_limit_msat: cmp::max(cmp::min(outbound_capacity_msat as i64, + next_outbound_htlc_limit_msat: cmp::max(cmp::min(available_capacity_msat as i64, self.counterparty_max_htlc_value_in_flight_msat as i64 - outbound_stats.pending_htlcs_value_msat as i64), 0) as u64, @@ -5896,6 +5931,7 @@ impl Channel { let holder_balance_msat = self.value_to_self_msat .saturating_sub(outbound_stats.pending_htlcs_value_msat); if holder_balance_msat < amount_msat { + debug_assert!(amount_msat > self.get_available_balances().next_outbound_htlc_limit_msat); return Err(ChannelError::Ignore(format!("Cannot send value that would overdraw remaining funds. Amount: {}, pending value to self {}", amount_msat, holder_balance_msat))); } @@ -5905,6 +5941,7 @@ impl Channel { FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE * self.next_local_commit_tx_fee_msat(htlc_candidate, Some(())) } else { 0 }; if holder_balance_msat - amount_msat < commit_tx_fee_msat { + debug_assert!(amount_msat > self.get_available_balances().next_outbound_htlc_limit_msat); return Err(ChannelError::Ignore(format!("Cannot send value that would not leave enough to pay for fees. Pending value to self: {}. local_commit_tx_fee {}", holder_balance_msat, commit_tx_fee_msat))); } @@ -5912,6 +5949,7 @@ impl Channel { // reserve for the remote to have something to claim if we misbehave) let chan_reserve_msat = self.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000; if holder_balance_msat - amount_msat - commit_tx_fee_msat < chan_reserve_msat { + debug_assert!(amount_msat > self.get_available_balances().next_outbound_htlc_limit_msat); return Err(ChannelError::Ignore(format!("Cannot send value that would put our balance under counterparty-announced channel reserve value ({})", chan_reserve_msat))); } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 92a6e2695a0..81c42861a8d 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -1342,7 +1342,9 @@ fn test_basic_channel_reserve() { // The 2* and +1 are for the fee spike reserve. let commit_tx_fee = 2 * commit_tx_fee_msat(get_feerate!(nodes[0], nodes[1], chan.2), 1 + 1, get_opt_anchors!(nodes[0], nodes[1], chan.2)); let max_can_send = 5000000 - channel_reserve - commit_tx_fee; - let (route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], max_can_send + 1); + let (mut route, our_payment_hash, _, our_payment_secret) = + get_route_and_payment_hash!(nodes[0], nodes[1], max_can_send); + route.paths[0].hops.last_mut().unwrap().fee_msat += 1; let err = nodes[0].node.send_payment_with_route(&route, our_payment_hash, RecipientOnionFields::secret_only(our_payment_secret), PaymentId(our_payment_hash.0)).err().unwrap(); match err { @@ -1369,7 +1371,9 @@ fn test_fee_spike_violation_fails_htlc() { let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 95000000); - let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], 3460001); + let (mut route, payment_hash, _, payment_secret) = + get_route_and_payment_hash!(nodes[0], nodes[1], 3460000); + route.paths[0].hops[0].fee_msat += 1; // Need to manually create the update_add_htlc message to go around the channel reserve check in send_htlc() let secp_ctx = Secp256k1::new(); let session_priv = SecretKey::from_slice(&[42; 32]).expect("RNG is bad!"); @@ -1732,7 +1736,8 @@ fn test_chan_reserve_violation_inbound_htlc_inbound_chan() { let commit_tx_fee_2_htlcs = commit_tx_fee_msat(feerate, 2, opt_anchors); let recv_value_2 = chan_stat.value_to_self_msat - amt_msat_1 - chan_stat.channel_reserve_msat - total_routing_fee_msat - commit_tx_fee_2_htlcs + 1; let amt_msat_2 = recv_value_2 + total_routing_fee_msat; - let (route_2, _, _, _) = get_route_and_payment_hash!(nodes[0], nodes[2], amt_msat_2); + let mut route_2 = route_1.clone(); + route_2.paths[0].hops.last_mut().unwrap().fee_msat = amt_msat_2; // Need to manually create the update_add_htlc message to go around the channel reserve check in send_htlc() let secp_ctx = Secp256k1::new(); @@ -1901,7 +1906,9 @@ fn test_channel_reserve_holding_cell_htlcs() { // channel reserve test with htlc pending output > 0 let recv_value_2 = stat01.value_to_self_msat - amt_msat_1 - stat01.channel_reserve_msat - total_fee_msat - commit_tx_fee_2_htlcs; { - let (route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], recv_value_2 + 1); + let mut route = route_1.clone(); + route.paths[0].hops.last_mut().unwrap().fee_msat = recv_value_2 + 1; + let (_, our_payment_hash, our_payment_secret) = get_payment_preimage_hash!(nodes[2]); unwrap_send_err!(nodes[0].node.send_payment_with_route(&route, our_payment_hash, RecipientOnionFields::secret_only(our_payment_secret), PaymentId(our_payment_hash.0) ), true, APIError::ChannelUnavailable { ref err }, @@ -1930,7 +1937,9 @@ fn test_channel_reserve_holding_cell_htlcs() { // test with outbound holding cell amount > 0 { - let (route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], recv_value_22+1); + let (mut route, our_payment_hash, _, our_payment_secret) = + get_route_and_payment_hash!(nodes[0], nodes[2], recv_value_22); + route.paths[0].hops.last_mut().unwrap().fee_msat += 1; unwrap_send_err!(nodes[0].node.send_payment_with_route(&route, our_payment_hash, RecipientOnionFields::secret_only(our_payment_secret), PaymentId(our_payment_hash.0) ), true, APIError::ChannelUnavailable { ref err }, @@ -6241,12 +6250,15 @@ fn test_update_add_htlc_bolt2_receiver_check_max_htlc_limit() { let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 95000000); - let (route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], 3999999); + let send_amt = 3999999; + let (mut route, our_payment_hash, _, our_payment_secret) = + get_route_and_payment_hash!(nodes[0], nodes[1], 1000); + route.paths[0].hops[0].fee_msat = send_amt; let session_priv = SecretKey::from_slice(&[42; 32]).unwrap(); let cur_height = nodes[0].node.best_block.read().unwrap().height() + 1; let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::signing_only(), &route.paths[0], &session_priv).unwrap(); let (onion_payloads, _htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads( - &route.paths[0], 3999999, RecipientOnionFields::secret_only(our_payment_secret), cur_height, &None).unwrap(); + &route.paths[0], send_amt, RecipientOnionFields::secret_only(our_payment_secret), cur_height, &None).unwrap(); let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &our_payment_hash).unwrap(); let mut msg = msgs::UpdateAddHTLC { From b09ccd10be82a0093ae7b1b196c739fb8f32c42b Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 15 May 2023 03:34:18 +0000 Subject: [PATCH 04/10] Consider HTLC in-flight count limits when assembling a route When calculating the amount available to send for the next HTLC, if we over-count we may create routes which are not actually usable. Historically this has been an issue, which we resolve over a few commits. Here we consider the number of in-flight HTLCs which we are allowed to push towards a counterparty at once, setting the available balance to zero if we cannot push any further HTLCs. We also add some testing when sending to ensure that send failures are accounted for in our balance calculations. --- lightning/src/ln/channel.rs | 15 +++++++++++---- lightning/src/ln/functional_tests.rs | 7 +++++-- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index bda0bfa509c..e0be0af5d72 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2725,6 +2725,14 @@ impl Channel { available_capacity_msat = capacity_minus_commitment_fee_msat as u64; } } + + available_capacity_msat = cmp::min(available_capacity_msat, + self.counterparty_max_htlc_value_in_flight_msat - outbound_stats.pending_htlcs_value_msat); + + if outbound_stats.pending_htlcs + 1 > self.counterparty_max_accepted_htlcs as u32 { + available_capacity_msat = 0; + } + AvailableBalances { inbound_capacity_msat: cmp::max(self.channel_value_satoshis as i64 * 1000 - self.value_to_self_msat as i64 @@ -2732,10 +2740,7 @@ impl Channel { - self.holder_selected_channel_reserve_satoshis as i64 * 1000, 0) as u64, outbound_capacity_msat, - next_outbound_htlc_limit_msat: cmp::max(cmp::min(available_capacity_msat as i64, - self.counterparty_max_htlc_value_in_flight_msat as i64 - - outbound_stats.pending_htlcs_value_msat as i64), - 0) as u64, + next_outbound_htlc_limit_msat: available_capacity_msat, balance_msat, } } @@ -5885,10 +5890,12 @@ impl Channel { let inbound_stats = self.get_inbound_pending_htlc_stats(None); let outbound_stats = self.get_outbound_pending_htlc_stats(None); if outbound_stats.pending_htlcs + 1 > self.counterparty_max_accepted_htlcs as u32 { + debug_assert!(amount_msat > self.get_available_balances().next_outbound_htlc_limit_msat); return Err(ChannelError::Ignore(format!("Cannot push more than their max accepted HTLCs ({})", self.counterparty_max_accepted_htlcs))); } // Check their_max_htlc_value_in_flight_msat if outbound_stats.pending_htlcs_value_msat + amount_msat > self.counterparty_max_htlc_value_in_flight_msat { + debug_assert!(amount_msat > self.get_available_balances().next_outbound_htlc_limit_msat); return Err(ChannelError::Ignore(format!("Cannot send value that would put us over the max HTLC value in flight our peer will accept ({})", self.counterparty_max_htlc_value_in_flight_msat))); } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 81c42861a8d..dfb101b1f90 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -1102,6 +1102,9 @@ fn holding_cell_htlc_counting() { create_announced_chan_between_nodes(&nodes, 0, 1); let chan_2 = create_announced_chan_between_nodes(&nodes, 1, 2); + // Fetch a route in advance as we will be unable to once we're unable to send. + let (route, payment_hash_1, _, payment_secret_1) = get_route_and_payment_hash!(nodes[1], nodes[2], 100000); + let mut payments = Vec::new(); for _ in 0..50 { let (route, payment_hash, payment_preimage, payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[2], 100000); @@ -1119,7 +1122,6 @@ fn holding_cell_htlc_counting() { // There is now one HTLC in an outbound commitment transaction and (OUR_MAX_HTLCS - 1) HTLCs in // the holding cell waiting on B's RAA to send. At this point we should not be able to add // another HTLC. - let (route, payment_hash_1, _, payment_secret_1) = get_route_and_payment_hash!(nodes[1], nodes[2], 100000); { unwrap_send_err!(nodes[1].node.send_payment_with_route(&route, payment_hash_1, RecipientOnionFields::secret_only(payment_secret_1), PaymentId(payment_hash_1.0) @@ -6112,6 +6114,8 @@ fn test_update_add_htlc_bolt2_sender_exceed_max_htlc_num_and_htlc_id_increment() let max_accepted_htlcs = nodes[1].node.per_peer_state.read().unwrap().get(&nodes[0].node.get_our_node_id()) .unwrap().lock().unwrap().channel_by_id.get(&chan.2).unwrap().counterparty_max_accepted_htlcs as u64; + // Fetch a route in advance as we will be unable to once we're unable to send. + let (route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], 100000); for i in 0..max_accepted_htlcs { let (route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], 100000); let payment_event = { @@ -6135,7 +6139,6 @@ fn test_update_add_htlc_bolt2_sender_exceed_max_htlc_num_and_htlc_id_increment() expect_pending_htlcs_forwardable!(nodes[1]); expect_payment_claimable!(nodes[1], our_payment_hash, our_payment_secret, 100000); } - let (route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], 100000); unwrap_send_err!(nodes[0].node.send_payment_with_route(&route, our_payment_hash, RecipientOnionFields::secret_only(our_payment_secret), PaymentId(our_payment_hash.0) ), true, APIError::ChannelUnavailable { ref err }, From 52a90577f2b8eb67d6abe26242c63041212e8115 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 16 May 2023 03:26:21 +0000 Subject: [PATCH 05/10] Consider counterparty commitment tx fees when assembling a route When calculating the amount available to send for the next HTLC, if we over-count we may create routes which are not actually usable. Historically this has been an issue, which we resolve over a few commits. Here we consider whether one additional HTLC's commitment tx fees would result in the counterparty's commitment tx fees being greater than the reserve we've picked for them and, if so, limit our next HTLC value to only include dust HTLCs. We also add some testing when sending to ensure that send failures are accounted for in our balance calculations. This, and the previous few commits, fixes #1126. --- lightning/src/ln/channel.rs | 22 ++++++++++++++++++++++ lightning/src/ln/functional_tests.rs | 11 ++++++++--- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index e0be0af5d72..23e56f94bd7 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2676,6 +2676,7 @@ impl Channel { /// corner case properly. pub fn get_available_balances(&self) -> AvailableBalances { // Note that we have to handle overflow due to the above case. + let inbound_stats = self.get_inbound_pending_htlc_stats(None); let outbound_stats = self.get_outbound_pending_htlc_stats(None); let mut balance_msat = self.value_to_self_msat; @@ -2724,6 +2725,26 @@ impl Channel { } else { available_capacity_msat = capacity_minus_commitment_fee_msat as u64; } + } else { + // If the channel is inbound (i.e. counterparty pays the fee), we need to make sure + // sending a new HTLC won't reduce their balance below our reserve threshold. + let mut real_dust_limit_success_sat = self.counterparty_dust_limit_satoshis; + if !self.opt_anchors() { + real_dust_limit_success_sat += self.feerate_per_kw as u64 * htlc_success_tx_weight(false) / 1000; + } + + let htlc_above_dust = HTLCCandidate::new(real_dust_limit_success_sat * 1000, HTLCInitiator::LocalOffered); + let max_reserved_commit_tx_fee_msat = self.next_remote_commit_tx_fee_msat(htlc_above_dust, None); + + let holder_selected_chan_reserve_msat = self.holder_selected_channel_reserve_satoshis * 1000; + let remote_balance_msat = (self.channel_value_satoshis * 1000 - self.value_to_self_msat) + .saturating_sub(inbound_stats.pending_htlcs_value_msat); + + if remote_balance_msat < max_reserved_commit_tx_fee_msat + holder_selected_chan_reserve_msat { + // If another HTLC's fee would reduce the remote's balance below the reserve limit + // we've selected for them, we can only send dust HTLCs. + available_capacity_msat = cmp::min(available_capacity_msat, real_dust_limit_success_sat * 1000 - 1); + } } available_capacity_msat = cmp::min(available_capacity_msat, @@ -5906,6 +5927,7 @@ impl Channel { let holder_selected_chan_reserve_msat = self.holder_selected_channel_reserve_satoshis * 1000; let remote_balance_msat = (self.channel_value_satoshis * 1000 - self.value_to_self_msat).saturating_sub(inbound_stats.pending_htlcs_value_msat); if remote_balance_msat < counterparty_commit_tx_fee_msat + holder_selected_chan_reserve_msat { + debug_assert!(amount_msat > self.get_available_balances().next_outbound_htlc_limit_msat); return Err(ChannelError::Ignore("Cannot send value that would put counterparty balance under holder-announced channel reserve value".to_owned())); } } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index dfb101b1f90..3f400d9b822 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -1527,13 +1527,14 @@ fn test_chan_reserve_violation_outbound_htlc_inbound_chan() { let _ = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, push_amt); + // Fetch a route in advance as we will be unable to once we're unable to send. + let (route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], 1_000_000); // Sending exactly enough to hit the reserve amount should be accepted for _ in 0..MIN_AFFORDABLE_HTLC_COUNT { let (_, _, _) = route_payment(&nodes[1], &[&nodes[0]], 1_000_000); } // However one more HTLC should be significantly over the reserve amount and fail. - let (route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], 1_000_000); unwrap_send_err!(nodes[1].node.send_payment_with_route(&route, our_payment_hash, RecipientOnionFields::secret_only(our_payment_secret), PaymentId(our_payment_hash.0) ), true, APIError::ChannelUnavailable { ref err }, @@ -1565,7 +1566,9 @@ fn test_chan_reserve_violation_inbound_htlc_outbound_channel() { let (_, _, _) = route_payment(&nodes[1], &[&nodes[0]], 1_000_000); } - let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], 700_000); + let (mut route, payment_hash, _, payment_secret) = + get_route_and_payment_hash!(nodes[1], nodes[0], 1000); + route.paths[0].hops[0].fee_msat = 700_000; // Need to manually create the update_add_htlc message to go around the channel reserve check in send_htlc() let secp_ctx = Secp256k1::new(); let session_priv = SecretKey::from_slice(&[42; 32]).unwrap(); @@ -1627,7 +1630,9 @@ fn test_chan_reserve_dust_inbound_htlcs_outbound_chan() { } // One more than the dust amt should fail, however. - let (route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], dust_amt + 1); + let (mut route, our_payment_hash, _, our_payment_secret) = + get_route_and_payment_hash!(nodes[1], nodes[0], dust_amt); + route.paths[0].hops[0].fee_msat += 1; unwrap_send_err!(nodes[1].node.send_payment_with_route(&route, our_payment_hash, RecipientOnionFields::secret_only(our_payment_secret), PaymentId(our_payment_hash.0) ), true, APIError::ChannelUnavailable { ref err }, From 3aa8a1721c6968c6a5b40ffc11d3642646b34ceb Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 16 May 2023 20:01:08 +0000 Subject: [PATCH 06/10] Add a next-outbound-HTLC minimum field to chan details and use it In the coming commits, in order to ensure all routes we generate are usable, we'll start calculating the next-HTLC minimum for our channels and using it in the router. Here we set this up by adding an always-0 field for it in `ChannelDetails` and use it when routing. --- fuzz/src/router.rs | 1 + lightning/src/ln/channel.rs | 3 +++ lightning/src/ln/channelmanager.rs | 16 ++++++++++++---- lightning/src/routing/router.rs | 4 +++- 4 files changed, 19 insertions(+), 5 deletions(-) diff --git a/fuzz/src/router.rs b/fuzz/src/router.rs index 00c53dfe58e..72935f153ea 100644 --- a/fuzz/src/router.rs +++ b/fuzz/src/router.rs @@ -265,6 +265,7 @@ pub fn do_test(data: &[u8], out: Out) { balance_msat: 0, outbound_capacity_msat: capacity.saturating_mul(1000), next_outbound_htlc_limit_msat: capacity.saturating_mul(1000), + next_outbound_htlc_minimum_msat: 0, inbound_htlc_minimum_msat: None, inbound_htlc_maximum_msat: None, config: None, diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 23e56f94bd7..03acb73309d 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -73,6 +73,8 @@ pub struct AvailableBalances { pub outbound_capacity_msat: u64, /// The maximum value we can assign to the next outbound HTLC pub next_outbound_htlc_limit_msat: u64, + /// The minimum value we can assign to the next outbound HTLC + pub next_outbound_htlc_minimum_msat: u64, } #[derive(Debug, Clone, Copy, PartialEq)] @@ -2762,6 +2764,7 @@ impl Channel { 0) as u64, outbound_capacity_msat, next_outbound_htlc_limit_msat: available_capacity_msat, + next_outbound_htlc_minimum_msat: 0, balance_msat, } } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 30eab9066fd..b5e7d603411 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1274,8 +1274,14 @@ pub struct ChannelDetails { /// the current state and per-HTLC limit(s). This is intended for use when routing, allowing us /// to use a limit as close as possible to the HTLC limit we can currently send. /// - /// See also [`ChannelDetails::balance_msat`] and [`ChannelDetails::outbound_capacity_msat`]. + /// See also [`ChannelDetails::next_outbound_htlc_minimum_msat`], + /// [`ChannelDetails::balance_msat`], and [`ChannelDetails::outbound_capacity_msat`]. pub next_outbound_htlc_limit_msat: u64, + /// The minimum value for sending a single HTLC to the remote peer. This is the equivalent of + /// [`ChannelDetails::next_outbound_htlc_limit_msat`] but represents a lower-bound, rather than + /// an upper-bound. This is intended for use when routing, allowing us to ensure we pick a + /// route which is valid. + pub next_outbound_htlc_minimum_msat: u64, /// The available inbound capacity for the remote peer to send HTLCs to us. This does not /// include any pending HTLCs which are not yet fully resolved (and, thus, whose balance is not /// available for inclusion in new inbound HTLCs). @@ -1395,6 +1401,7 @@ impl ChannelDetails { inbound_capacity_msat: balance.inbound_capacity_msat, outbound_capacity_msat: balance.outbound_capacity_msat, next_outbound_htlc_limit_msat: balance.next_outbound_htlc_limit_msat, + next_outbound_htlc_minimum_msat: balance.next_outbound_htlc_minimum_msat, user_channel_id: channel.get_user_id(), confirmations_required: channel.minimum_depth(), confirmations: Some(channel.get_funding_tx_confirmations(best_block_height)), @@ -6951,10 +6958,9 @@ impl Writeable for ChannelDetails { (14, user_channel_id_low, required), (16, self.balance_msat, required), (18, self.outbound_capacity_msat, required), - // Note that by the time we get past the required read above, outbound_capacity_msat will be - // filled in, so we can safely unwrap it here. - (19, self.next_outbound_htlc_limit_msat, (default_value, outbound_capacity_msat.0.unwrap() as u64)), + (19, self.next_outbound_htlc_limit_msat, required), (20, self.inbound_capacity_msat, required), + (21, self.next_outbound_htlc_minimum_msat, required), (22, self.confirmations_required, option), (24, self.force_close_spend_delay, option), (26, self.is_outbound, required), @@ -6991,6 +6997,7 @@ impl Readable for ChannelDetails { // filled in, so we can safely unwrap it here. (19, next_outbound_htlc_limit_msat, (default_value, outbound_capacity_msat.0.unwrap() as u64)), (20, inbound_capacity_msat, required), + (21, next_outbound_htlc_minimum_msat, (default_value, 0)), (22, confirmations_required, option), (24, force_close_spend_delay, option), (26, is_outbound, required), @@ -7024,6 +7031,7 @@ impl Readable for ChannelDetails { balance_msat: balance_msat.0.unwrap(), outbound_capacity_msat: outbound_capacity_msat.0.unwrap(), next_outbound_htlc_limit_msat: next_outbound_htlc_limit_msat.0.unwrap(), + next_outbound_htlc_minimum_msat: next_outbound_htlc_minimum_msat.0.unwrap(), inbound_capacity_msat: inbound_capacity_msat.0.unwrap(), confirmations_required, confirmations, diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index ef6fef0a7eb..bf854aed637 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -929,7 +929,7 @@ impl<'a> CandidateRouteHop<'a> { fn htlc_minimum_msat(&self) -> u64 { match self { - CandidateRouteHop::FirstHop { .. } => 0, + CandidateRouteHop::FirstHop { details } => details.next_outbound_htlc_minimum_msat, CandidateRouteHop::PublicHop { info, .. } => info.direction().htlc_minimum_msat, CandidateRouteHop::PrivateHop { hint } => hint.htlc_minimum_msat.unwrap_or(0), } @@ -2460,6 +2460,7 @@ mod tests { balance_msat: 0, outbound_capacity_msat, next_outbound_htlc_limit_msat: outbound_capacity_msat, + next_outbound_htlc_minimum_msat: 0, inbound_capacity_msat: 42, unspendable_punishment_reserve: None, confirmations_required: None, @@ -6121,6 +6122,7 @@ pub(crate) mod bench_utils { user_channel_id: 0, balance_msat: 10_000_000_000, outbound_capacity_msat: 10_000_000_000, + next_outbound_htlc_minimum_msat: 0, next_outbound_htlc_limit_msat: 10_000_000_000, inbound_capacity_msat: 0, unspendable_punishment_reserve: None, From 7edbf547fb71f374d1ca11112177d66e8a05852c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 16 May 2023 05:00:01 +0000 Subject: [PATCH 07/10] Consider dust exposure when assembling a route When calculating the amount available to send for the next HTLC, if we over-count we may create routes which are not actually usable. Historically this has been an issue, which we resolve over a few commits. Here we consider how much adding one additional (dust) HTLC would impact our total dust exposure, setting the new next-HTLC-minimum field to require HTLCs be non-dust if required or set our next-HTLC maximum if we cannot send a dust HTLC but do have some additional exposure remaining. We also add some testing when sending to ensure that send failures are accounted for in our balance calculations. Fixes #2252. --- lightning/src/ln/channel.rs | 46 +++++++++++++++++++++++++++- lightning/src/ln/functional_tests.rs | 28 +++++++++++------ 2 files changed, 64 insertions(+), 10 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 03acb73309d..c404b46f11d 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2749,6 +2749,45 @@ impl Channel { } } + let mut next_outbound_htlc_minimum_msat = self.counterparty_htlc_minimum_msat; + + // If we get close to our maximum dust exposure, we end up in a situation where we can send + // between zero and the remaining dust exposure limit remaining OR above the dust limit. + // Because we cannot express this as a simple min/max, we prefer to tell the user they can + // send above the dust limit (as the router can always overpay to meet the dust limit). + let mut remaining_msat_below_dust_exposure_limit = None; + let mut dust_exposure_dust_limit_msat = 0; + + let (htlc_success_dust_limit, htlc_timeout_dust_limit) = if self.opt_anchors() { + (self.counterparty_dust_limit_satoshis, self.holder_dust_limit_satoshis) + } else { + let dust_buffer_feerate = self.get_dust_buffer_feerate(None) as u64; + (self.counterparty_dust_limit_satoshis + dust_buffer_feerate * htlc_success_tx_weight(false) / 1000, + self.holder_dust_limit_satoshis + dust_buffer_feerate * htlc_timeout_tx_weight(false) / 1000) + }; + let on_counterparty_dust_htlc_exposure_msat = inbound_stats.on_counterparty_tx_dust_exposure_msat + outbound_stats.on_counterparty_tx_dust_exposure_msat; + if on_counterparty_dust_htlc_exposure_msat as i64 + htlc_success_dust_limit as i64 * 1000 - 1 > self.get_max_dust_htlc_exposure_msat() as i64 { + remaining_msat_below_dust_exposure_limit = + Some(self.get_max_dust_htlc_exposure_msat().saturating_sub(on_counterparty_dust_htlc_exposure_msat)); + dust_exposure_dust_limit_msat = cmp::max(dust_exposure_dust_limit_msat, htlc_success_dust_limit * 1000); + } + + let on_holder_dust_htlc_exposure_msat = inbound_stats.on_holder_tx_dust_exposure_msat + outbound_stats.on_holder_tx_dust_exposure_msat; + if on_holder_dust_htlc_exposure_msat as i64 + htlc_timeout_dust_limit as i64 * 1000 - 1 > self.get_max_dust_htlc_exposure_msat() as i64 { + remaining_msat_below_dust_exposure_limit = Some(cmp::min( + remaining_msat_below_dust_exposure_limit.unwrap_or(u64::max_value()), + self.get_max_dust_htlc_exposure_msat().saturating_sub(on_holder_dust_htlc_exposure_msat))); + dust_exposure_dust_limit_msat = cmp::max(dust_exposure_dust_limit_msat, htlc_timeout_dust_limit * 1000); + } + + if let Some(remaining_limit_msat) = remaining_msat_below_dust_exposure_limit { + if available_capacity_msat < dust_exposure_dust_limit_msat { + available_capacity_msat = cmp::min(available_capacity_msat, remaining_limit_msat); + } else { + next_outbound_htlc_minimum_msat = cmp::max(next_outbound_htlc_minimum_msat, dust_exposure_dust_limit_msat); + } + } + available_capacity_msat = cmp::min(available_capacity_msat, self.counterparty_max_htlc_value_in_flight_msat - outbound_stats.pending_htlcs_value_msat); @@ -2764,7 +2803,7 @@ impl Channel { 0) as u64, outbound_capacity_msat, next_outbound_htlc_limit_msat: available_capacity_msat, - next_outbound_htlc_minimum_msat: 0, + next_outbound_htlc_minimum_msat, balance_msat, } } @@ -5898,6 +5937,7 @@ impl Channel { } if amount_msat < self.counterparty_htlc_minimum_msat { + debug_assert!(amount_msat < self.get_available_balances().next_outbound_htlc_minimum_msat); return Err(ChannelError::Ignore(format!("Cannot send less than their minimum HTLC value ({})", self.counterparty_htlc_minimum_msat))); } @@ -5946,6 +5986,8 @@ impl Channel { if amount_msat / 1000 < exposure_dust_limit_success_sats { let on_counterparty_dust_htlc_exposure_msat = inbound_stats.on_counterparty_tx_dust_exposure_msat + outbound_stats.on_counterparty_tx_dust_exposure_msat + amount_msat; if on_counterparty_dust_htlc_exposure_msat > self.get_max_dust_htlc_exposure_msat() { + debug_assert!(amount_msat > self.get_available_balances().next_outbound_htlc_limit_msat || + amount_msat < self.get_available_balances().next_outbound_htlc_minimum_msat); return Err(ChannelError::Ignore(format!("Cannot send value that would put our exposure to dust HTLCs at {} over the limit {} on counterparty commitment tx", on_counterparty_dust_htlc_exposure_msat, self.get_max_dust_htlc_exposure_msat()))); } @@ -5955,6 +5997,8 @@ impl Channel { if amount_msat / 1000 < exposure_dust_limit_timeout_sats { let on_holder_dust_htlc_exposure_msat = inbound_stats.on_holder_tx_dust_exposure_msat + outbound_stats.on_holder_tx_dust_exposure_msat + amount_msat; if on_holder_dust_htlc_exposure_msat > self.get_max_dust_htlc_exposure_msat() { + debug_assert!(amount_msat > self.get_available_balances().next_outbound_htlc_limit_msat || + amount_msat < self.get_available_balances().next_outbound_htlc_minimum_msat); return Err(ChannelError::Ignore(format!("Cannot send value that would put our exposure to dust HTLCs at {} over the limit {} on holder commitment tx", on_holder_dust_htlc_exposure_msat, self.get_max_dust_htlc_exposure_msat()))); } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 3f400d9b822..3985ac03bbd 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -9635,6 +9635,10 @@ fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, exposure_breach_e let (announcement, as_update, bs_update) = create_chan_between_nodes_with_value_b(&nodes[0], &nodes[1], &channel_ready); update_nodes_with_chan_announce(&nodes, 0, 1, &announcement, &as_update, &bs_update); + // Fetch a route in advance as we will be unable to once we're unable to send. + let (mut route, payment_hash, _, payment_secret) = + get_route_and_payment_hash!(nodes[0], nodes[1], 1000); + let dust_buffer_feerate = { let per_peer_state = nodes[0].node.per_peer_state.read().unwrap(); let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap(); @@ -9647,7 +9651,7 @@ fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, exposure_breach_e let dust_inbound_htlc_on_holder_tx_msat: u64 = (dust_buffer_feerate * htlc_success_tx_weight(opt_anchors) / 1000 + open_channel.dust_limit_satoshis - 1) * 1000; let dust_inbound_htlc_on_holder_tx: u64 = config.channel_config.max_dust_htlc_exposure_msat / dust_inbound_htlc_on_holder_tx_msat; - let dust_htlc_on_counterparty_tx: u64 = 25; + let dust_htlc_on_counterparty_tx: u64 = 4; let dust_htlc_on_counterparty_tx_msat: u64 = config.channel_config.max_dust_htlc_exposure_msat / dust_htlc_on_counterparty_tx; if on_holder_tx { @@ -9672,7 +9676,7 @@ fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, exposure_breach_e if dust_outbound_balance { // Outbound dust threshold: 2132 sats (`dust_buffer_feerate` * HTLC_TIMEOUT_TX_WEIGHT / 1000 + counteparty's `dust_limit_satoshis`) // Outbound dust balance: 5000 sats - for _ in 0..dust_htlc_on_counterparty_tx { + for _ in 0..dust_htlc_on_counterparty_tx - 1 { let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], dust_htlc_on_counterparty_tx_msat); nodes[0].node.send_payment_with_route(&route, payment_hash, RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0)).unwrap(); @@ -9680,15 +9684,15 @@ fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, exposure_breach_e } else { // Inbound dust threshold: 2031 sats (`dust_buffer_feerate` * HTLC_TIMEOUT_TX_WEIGHT / 1000 + counteparty's `dust_limit_satoshis`) // Inbound dust balance: 5000 sats - for _ in 0..dust_htlc_on_counterparty_tx { + for _ in 0..dust_htlc_on_counterparty_tx - 1 { route_payment(&nodes[1], &[&nodes[0]], dust_htlc_on_counterparty_tx_msat); } } } - let dust_overflow = dust_htlc_on_counterparty_tx_msat * (dust_htlc_on_counterparty_tx + 1); if exposure_breach_event == ExposureEvent::AtHTLCForward { - let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], if on_holder_tx { dust_outbound_htlc_on_holder_tx_msat } else { dust_htlc_on_counterparty_tx_msat }); + route.paths[0].hops.last_mut().unwrap().fee_msat = + if on_holder_tx { dust_outbound_htlc_on_holder_tx_msat } else { dust_htlc_on_counterparty_tx_msat + 1 }; let mut config = UserConfig::default(); // With default dust exposure: 5000 sats if on_holder_tx { @@ -9702,10 +9706,13 @@ fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, exposure_breach_e unwrap_send_err!(nodes[0].node.send_payment_with_route(&route, payment_hash, RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0) ), true, APIError::ChannelUnavailable { ref err }, - assert_eq!(err, &format!("Cannot send value that would put our exposure to dust HTLCs at {} over the limit {} on counterparty commitment tx", dust_overflow, config.channel_config.max_dust_htlc_exposure_msat))); + assert_eq!(err, + &format!("Cannot send value that would put our exposure to dust HTLCs at {} over the limit {} on counterparty commitment tx", + dust_htlc_on_counterparty_tx_msat * (dust_htlc_on_counterparty_tx - 1) + dust_htlc_on_counterparty_tx_msat + 1, + config.channel_config.max_dust_htlc_exposure_msat))); } } else if exposure_breach_event == ExposureEvent::AtHTLCReception { - let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], if on_holder_tx { dust_inbound_htlc_on_holder_tx_msat } else { dust_htlc_on_counterparty_tx_msat }); + let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], if on_holder_tx { dust_inbound_htlc_on_holder_tx_msat } else { dust_htlc_on_counterparty_tx_msat + 1 }); nodes[1].node.send_payment_with_route(&route, payment_hash, RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0)).unwrap(); check_added_monitors!(nodes[1], 1); @@ -9721,10 +9728,13 @@ fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, exposure_breach_e nodes[0].logger.assert_log("lightning::ln::channel".to_string(), format!("Cannot accept value that would put our exposure to dust HTLCs at {} over the limit {} on holder commitment tx", if dust_outbound_balance { dust_outbound_overflow } else { dust_inbound_overflow }, config.channel_config.max_dust_htlc_exposure_msat), 1); } else { // Outbound dust balance: 5200 sats - nodes[0].logger.assert_log("lightning::ln::channel".to_string(), format!("Cannot accept value that would put our exposure to dust HTLCs at {} over the limit {} on counterparty commitment tx", dust_overflow, config.channel_config.max_dust_htlc_exposure_msat), 1); + nodes[0].logger.assert_log("lightning::ln::channel".to_string(), + format!("Cannot accept value that would put our exposure to dust HTLCs at {} over the limit {} on counterparty commitment tx", + dust_htlc_on_counterparty_tx_msat * (dust_htlc_on_counterparty_tx - 1) + dust_htlc_on_counterparty_tx_msat + 1, + config.channel_config.max_dust_htlc_exposure_msat), 1); } } else if exposure_breach_event == ExposureEvent::AtUpdateFeeOutbound { - let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], 2_500_000); + route.paths[0].hops.last_mut().unwrap().fee_msat = 2_500_000; nodes[0].node.send_payment_with_route(&route, payment_hash, RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0)).unwrap(); { From 66c4f454f0316bb8c79d53a97308c706d2d3725e Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 17 May 2023 00:56:22 +0000 Subject: [PATCH 08/10] Ensure a 1:1 mapping of value sendable to send success in fuzzing Now that the value available to send is expected to match the success or failure of sending exactly, we should assert this in the `chanmon_consistency` fuzzer. In the next commit we'll actually rip the checks out of `send_htlc` which will make this a somewhat less useful test, however fuzzing on this specific commit can help to reveal bugs. --- fuzz/src/chanmon_consistency.rs | 47 +++++++++++++++++++++++++-------- lightning/src/ln/channel.rs | 2 ++ 2 files changed, 38 insertions(+), 11 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index f8c6c08a2ba..bb406670063 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -285,7 +285,7 @@ impl KeyProvider { } #[inline] -fn check_api_err(api_err: APIError) { +fn check_api_err(api_err: APIError, sendable_bounds_violated: bool) { match api_err { APIError::APIMisuseError { .. } => panic!("We can't misuse the API"), APIError::FeeRateTooHigh { .. } => panic!("We can't send too much fee?"), @@ -305,6 +305,7 @@ fn check_api_err(api_err: APIError) { _ if err.starts_with("Cannot send value that would put our exposure to dust HTLCs at") => {}, _ => panic!("{}", err), } + assert!(sendable_bounds_violated); }, APIError::MonitorUpdateInProgress => { // We can (obviously) temp-fail a monitor update @@ -313,17 +314,17 @@ fn check_api_err(api_err: APIError) { } } #[inline] -fn check_payment_err(send_err: PaymentSendFailure) { +fn check_payment_err(send_err: PaymentSendFailure, sendable_bounds_violated: bool) { match send_err { - PaymentSendFailure::ParameterError(api_err) => check_api_err(api_err), + PaymentSendFailure::ParameterError(api_err) => check_api_err(api_err, sendable_bounds_violated), PaymentSendFailure::PathParameterError(per_path_results) => { - for res in per_path_results { if let Err(api_err) = res { check_api_err(api_err); } } + for res in per_path_results { if let Err(api_err) = res { check_api_err(api_err, sendable_bounds_violated); } } }, PaymentSendFailure::AllFailedResendSafe(per_path_results) => { - for api_err in per_path_results { check_api_err(api_err); } + for api_err in per_path_results { check_api_err(api_err, sendable_bounds_violated); } }, PaymentSendFailure::PartialFailure { results, .. } => { - for res in results { if let Err(api_err) = res { check_api_err(api_err); } } + for res in results { if let Err(api_err) = res { check_api_err(api_err, sendable_bounds_violated); } } }, PaymentSendFailure::DuplicatePayment => panic!(), } @@ -351,6 +352,11 @@ fn send_payment(source: &ChanMan, dest: &ChanMan, dest_chan_id: u64, amt: u64, p let mut payment_id = [0; 32]; payment_id[0..8].copy_from_slice(&payment_idx.to_ne_bytes()); *payment_idx += 1; + let (min_value_sendable, max_value_sendable) = source.list_usable_channels() + .iter().find(|chan| chan.short_channel_id == Some(dest_chan_id)) + .map(|chan| + (chan.next_outbound_htlc_minimum_msat, chan.next_outbound_htlc_limit_msat)) + .unwrap_or((0, 0)); if let Err(err) = source.send_payment_with_route(&Route { paths: vec![Path { hops: vec![RouteHop { pubkey: dest.get_our_node_id(), @@ -362,9 +368,15 @@ fn send_payment(source: &ChanMan, dest: &ChanMan, dest_chan_id: u64, amt: u64, p }], blinded_tail: None }], payment_params: None, }, payment_hash, RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_id)) { - check_payment_err(err); + check_payment_err(err, amt > max_value_sendable || amt < min_value_sendable); false - } else { true } + } else { + // Note that while the max is a strict upper-bound, we can occasionally send substantially + // below the minimum, with some gap which is unusable immediately below the minimum. Thus, + // we don't check against min_value_sendable here. + assert!(amt <= max_value_sendable); + true + } } #[inline] fn send_hop_payment(source: &ChanMan, middle: &ChanMan, middle_chan_id: u64, dest: &ChanMan, dest_chan_id: u64, amt: u64, payment_id: &mut u8, payment_idx: &mut u64) -> bool { @@ -373,13 +385,19 @@ fn send_hop_payment(source: &ChanMan, middle: &ChanMan, middle_chan_id: u64, des let mut payment_id = [0; 32]; payment_id[0..8].copy_from_slice(&payment_idx.to_ne_bytes()); *payment_idx += 1; + let (min_value_sendable, max_value_sendable) = source.list_usable_channels() + .iter().find(|chan| chan.short_channel_id == Some(middle_chan_id)) + .map(|chan| + (chan.next_outbound_htlc_minimum_msat, chan.next_outbound_htlc_limit_msat)) + .unwrap_or((0, 0)); + let first_hop_fee = 50_000; if let Err(err) = source.send_payment_with_route(&Route { paths: vec![Path { hops: vec![RouteHop { pubkey: middle.get_our_node_id(), node_features: middle.node_features(), short_channel_id: middle_chan_id, channel_features: middle.channel_features(), - fee_msat: 50000, + fee_msat: first_hop_fee, cltv_expiry_delta: 100, },RouteHop { pubkey: dest.get_our_node_id(), @@ -391,9 +409,16 @@ fn send_hop_payment(source: &ChanMan, middle: &ChanMan, middle_chan_id: u64, des }], blinded_tail: None }], payment_params: None, }, payment_hash, RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_id)) { - check_payment_err(err); + let sent_amt = amt + first_hop_fee; + check_payment_err(err, sent_amt < min_value_sendable || sent_amt > max_value_sendable); false - } else { true } + } else { + // Note that while the max is a strict upper-bound, we can occasionally send substantially + // below the minimum, with some gap which is unusable immediately below the minimum. Thus, + // we don't check against min_value_sendable here. + assert!(amt + first_hop_fee <= max_value_sendable); + true + } } #[inline] diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index c404b46f11d..2ea2ebd58c4 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -6029,6 +6029,8 @@ impl Channel { return Err(ChannelError::Ignore(format!("Cannot send value that would put our balance under counterparty-announced channel reserve value ({})", chan_reserve_msat))); } + debug_assert!(amount_msat <= self.get_available_balances().next_outbound_htlc_limit_msat); + let need_holding_cell = (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateInProgress as u32)) != 0; log_debug!(logger, "Pushing new outbound HTLC for {} msat {}", amount_msat, if force_holding_cell { "into holding cell" } From 10e213cf40c5b6797e7d1a8615616f79524dbbe9 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 17 May 2023 01:33:42 +0000 Subject: [PATCH 09/10] Replace `send_htlc` amount checking with available balances Now that the `get_available_balances` min/max bounds are exact, we can stop doing all the explicit checks in `send_htlc` entirely, instead comparing against the `get_available_balances` bounds and failing if the amount is out of those bounds. This breaks support for sending amounts below the dust limit if there is some amount of dust exposure remaining before we hit our cap, however we will no longer generate such routes anyway. --- fuzz/src/chanmon_consistency.rs | 9 +-- lightning/src/ln/channel.rs | 92 +++------------------------- lightning/src/ln/functional_tests.rs | 61 ++++-------------- lightning/src/ln/payment_tests.rs | 6 +- 4 files changed, 26 insertions(+), 142 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index bb406670063..02c8450fa62 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -296,13 +296,8 @@ fn check_api_err(api_err: APIError, sendable_bounds_violated: bool) { // is probably just stale and you should add new messages here. match err.as_str() { "Peer for first hop currently disconnected" => {}, - _ if err.starts_with("Cannot push more than their max accepted HTLCs ") => {}, - _ if err.starts_with("Cannot send value that would put us over the max HTLC value in flight our peer will accept ") => {}, - _ if err.starts_with("Cannot send value that would put our balance under counterparty-announced channel reserve value") => {}, - _ if err.starts_with("Cannot send value that would put counterparty balance under holder-announced channel reserve value") => {}, - _ if err.starts_with("Cannot send value that would overdraw remaining funds.") => {}, - _ if err.starts_with("Cannot send value that would not leave enough to pay for fees.") => {}, - _ if err.starts_with("Cannot send value that would put our exposure to dust HTLCs at") => {}, + _ if err.starts_with("Cannot send less than our next-HTLC minimum - ") => {}, + _ if err.starts_with("Cannot send more than our next-HTLC maximum - ") => {}, _ => panic!("{}", err), } assert!(sendable_bounds_violated); diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 2ea2ebd58c4..577223c6382 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -5936,9 +5936,15 @@ impl Channel { return Err(ChannelError::Ignore("Cannot send 0-msat HTLC".to_owned())); } - if amount_msat < self.counterparty_htlc_minimum_msat { - debug_assert!(amount_msat < self.get_available_balances().next_outbound_htlc_minimum_msat); - return Err(ChannelError::Ignore(format!("Cannot send less than their minimum HTLC value ({})", self.counterparty_htlc_minimum_msat))); + let available_balances = self.get_available_balances(); + if amount_msat < available_balances.next_outbound_htlc_minimum_msat { + return Err(ChannelError::Ignore(format!("Cannot send less than our next-HTLC minimum - {} msat", + available_balances.next_outbound_htlc_minimum_msat))); + } + + if amount_msat > available_balances.next_outbound_htlc_limit_msat { + return Err(ChannelError::Ignore(format!("Cannot send more than our next-HTLC maximum - {} msat", + available_balances.next_outbound_htlc_limit_msat))); } if (self.channel_state & (ChannelState::PeerDisconnected as u32)) != 0 { @@ -5951,86 +5957,6 @@ impl Channel { return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected from channel counterparty".to_owned())); } - let inbound_stats = self.get_inbound_pending_htlc_stats(None); - let outbound_stats = self.get_outbound_pending_htlc_stats(None); - if outbound_stats.pending_htlcs + 1 > self.counterparty_max_accepted_htlcs as u32 { - debug_assert!(amount_msat > self.get_available_balances().next_outbound_htlc_limit_msat); - return Err(ChannelError::Ignore(format!("Cannot push more than their max accepted HTLCs ({})", self.counterparty_max_accepted_htlcs))); - } - // Check their_max_htlc_value_in_flight_msat - if outbound_stats.pending_htlcs_value_msat + amount_msat > self.counterparty_max_htlc_value_in_flight_msat { - debug_assert!(amount_msat > self.get_available_balances().next_outbound_htlc_limit_msat); - return Err(ChannelError::Ignore(format!("Cannot send value that would put us over the max HTLC value in flight our peer will accept ({})", self.counterparty_max_htlc_value_in_flight_msat))); - } - - if !self.is_outbound() { - // Check that we won't violate the remote channel reserve by adding this HTLC. - let htlc_candidate = HTLCCandidate::new(amount_msat, HTLCInitiator::LocalOffered); - let counterparty_commit_tx_fee_msat = self.next_remote_commit_tx_fee_msat(htlc_candidate, None); - let holder_selected_chan_reserve_msat = self.holder_selected_channel_reserve_satoshis * 1000; - let remote_balance_msat = (self.channel_value_satoshis * 1000 - self.value_to_self_msat).saturating_sub(inbound_stats.pending_htlcs_value_msat); - if remote_balance_msat < counterparty_commit_tx_fee_msat + holder_selected_chan_reserve_msat { - debug_assert!(amount_msat > self.get_available_balances().next_outbound_htlc_limit_msat); - return Err(ChannelError::Ignore("Cannot send value that would put counterparty balance under holder-announced channel reserve value".to_owned())); - } - } - - let (htlc_success_dust_limit, htlc_timeout_dust_limit) = if self.opt_anchors() { - (0, 0) - } else { - let dust_buffer_feerate = self.get_dust_buffer_feerate(None) as u64; - (dust_buffer_feerate * htlc_success_tx_weight(false) / 1000, - dust_buffer_feerate * htlc_timeout_tx_weight(false) / 1000) - }; - let exposure_dust_limit_success_sats = htlc_success_dust_limit + self.counterparty_dust_limit_satoshis; - if amount_msat / 1000 < exposure_dust_limit_success_sats { - let on_counterparty_dust_htlc_exposure_msat = inbound_stats.on_counterparty_tx_dust_exposure_msat + outbound_stats.on_counterparty_tx_dust_exposure_msat + amount_msat; - if on_counterparty_dust_htlc_exposure_msat > self.get_max_dust_htlc_exposure_msat() { - debug_assert!(amount_msat > self.get_available_balances().next_outbound_htlc_limit_msat || - amount_msat < self.get_available_balances().next_outbound_htlc_minimum_msat); - return Err(ChannelError::Ignore(format!("Cannot send value that would put our exposure to dust HTLCs at {} over the limit {} on counterparty commitment tx", - on_counterparty_dust_htlc_exposure_msat, self.get_max_dust_htlc_exposure_msat()))); - } - } - - let exposure_dust_limit_timeout_sats = htlc_timeout_dust_limit + self.holder_dust_limit_satoshis; - if amount_msat / 1000 < exposure_dust_limit_timeout_sats { - let on_holder_dust_htlc_exposure_msat = inbound_stats.on_holder_tx_dust_exposure_msat + outbound_stats.on_holder_tx_dust_exposure_msat + amount_msat; - if on_holder_dust_htlc_exposure_msat > self.get_max_dust_htlc_exposure_msat() { - debug_assert!(amount_msat > self.get_available_balances().next_outbound_htlc_limit_msat || - amount_msat < self.get_available_balances().next_outbound_htlc_minimum_msat); - return Err(ChannelError::Ignore(format!("Cannot send value that would put our exposure to dust HTLCs at {} over the limit {} on holder commitment tx", - on_holder_dust_htlc_exposure_msat, self.get_max_dust_htlc_exposure_msat()))); - } - } - - let holder_balance_msat = self.value_to_self_msat - .saturating_sub(outbound_stats.pending_htlcs_value_msat); - if holder_balance_msat < amount_msat { - debug_assert!(amount_msat > self.get_available_balances().next_outbound_htlc_limit_msat); - return Err(ChannelError::Ignore(format!("Cannot send value that would overdraw remaining funds. Amount: {}, pending value to self {}", amount_msat, holder_balance_msat))); - } - - // `2 *` and extra HTLC are for the fee spike buffer. - let commit_tx_fee_msat = if self.is_outbound() { - let htlc_candidate = HTLCCandidate::new(amount_msat, HTLCInitiator::LocalOffered); - FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE * self.next_local_commit_tx_fee_msat(htlc_candidate, Some(())) - } else { 0 }; - if holder_balance_msat - amount_msat < commit_tx_fee_msat { - debug_assert!(amount_msat > self.get_available_balances().next_outbound_htlc_limit_msat); - return Err(ChannelError::Ignore(format!("Cannot send value that would not leave enough to pay for fees. Pending value to self: {}. local_commit_tx_fee {}", holder_balance_msat, commit_tx_fee_msat))); - } - - // Check self.counterparty_selected_channel_reserve_satoshis (the amount we must keep as - // reserve for the remote to have something to claim if we misbehave) - let chan_reserve_msat = self.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000; - if holder_balance_msat - amount_msat - commit_tx_fee_msat < chan_reserve_msat { - debug_assert!(amount_msat > self.get_available_balances().next_outbound_htlc_limit_msat); - return Err(ChannelError::Ignore(format!("Cannot send value that would put our balance under counterparty-announced channel reserve value ({})", chan_reserve_msat))); - } - - debug_assert!(amount_msat <= self.get_available_balances().next_outbound_htlc_limit_msat); - let need_holding_cell = (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateInProgress as u32)) != 0; log_debug!(logger, "Pushing new outbound HTLC for {} msat {}", amount_msat, if force_holding_cell { "into holding cell" } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 3985ac03bbd..ec0068f3194 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -1125,10 +1125,8 @@ fn holding_cell_htlc_counting() { { unwrap_send_err!(nodes[1].node.send_payment_with_route(&route, payment_hash_1, RecipientOnionFields::secret_only(payment_secret_1), PaymentId(payment_hash_1.0) - ), true, APIError::ChannelUnavailable { ref err }, - assert!(regex::Regex::new(r"Cannot push more than their max accepted HTLCs \(\d+\)").unwrap().is_match(err))); + ), true, APIError::ChannelUnavailable { .. }, {}); assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); - nodes[1].logger.assert_log_contains("lightning::ln::channelmanager", "Cannot push more than their max accepted HTLCs", 1); } // This should also be true if we try to forward a payment. @@ -1351,16 +1349,12 @@ fn test_basic_channel_reserve() { RecipientOnionFields::secret_only(our_payment_secret), PaymentId(our_payment_hash.0)).err().unwrap(); match err { PaymentSendFailure::AllFailedResendSafe(ref fails) => { - match &fails[0] { - &APIError::ChannelUnavailable{ref err} => - assert!(regex::Regex::new(r"Cannot send value that would put our balance under counterparty-announced channel reserve value \(\d+\)").unwrap().is_match(err)), - _ => panic!("Unexpected error variant"), - } + if let &APIError::ChannelUnavailable { .. } = &fails[0] {} + else { panic!("Unexpected error variant"); } }, _ => panic!("Unexpected error variant"), } assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); - nodes[0].logger.assert_log_contains("lightning::ln::channelmanager", "Cannot send value that would put our balance under counterparty-announced channel reserve value", 1); send_payment(&nodes[0], &vec![&nodes[1]], max_can_send); } @@ -1537,10 +1531,8 @@ fn test_chan_reserve_violation_outbound_htlc_inbound_chan() { // However one more HTLC should be significantly over the reserve amount and fail. unwrap_send_err!(nodes[1].node.send_payment_with_route(&route, our_payment_hash, RecipientOnionFields::secret_only(our_payment_secret), PaymentId(our_payment_hash.0) - ), true, APIError::ChannelUnavailable { ref err }, - assert_eq!(err, "Cannot send value that would put counterparty balance under holder-announced channel reserve value")); + ), true, APIError::ChannelUnavailable { .. }, {}); assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); - nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Cannot send value that would put counterparty balance under holder-announced channel reserve value".to_string(), 1); } #[test] @@ -1635,8 +1627,7 @@ fn test_chan_reserve_dust_inbound_htlcs_outbound_chan() { route.paths[0].hops[0].fee_msat += 1; unwrap_send_err!(nodes[1].node.send_payment_with_route(&route, our_payment_hash, RecipientOnionFields::secret_only(our_payment_secret), PaymentId(our_payment_hash.0) - ), true, APIError::ChannelUnavailable { ref err }, - assert_eq!(err, "Cannot send value that would put counterparty balance under holder-announced channel reserve value")); + ), true, APIError::ChannelUnavailable { .. }, {}); } #[test] @@ -1844,10 +1835,8 @@ fn test_channel_reserve_holding_cell_htlcs() { unwrap_send_err!(nodes[0].node.send_payment_with_route(&route, our_payment_hash, RecipientOnionFields::secret_only(our_payment_secret), PaymentId(our_payment_hash.0) - ), true, APIError::ChannelUnavailable { ref err }, - assert!(regex::Regex::new(r"Cannot send value that would put us over the max HTLC value in flight our peer will accept \(\d+\)").unwrap().is_match(err))); + ), true, APIError::ChannelUnavailable { .. }, {}); assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); - nodes[0].logger.assert_log_contains("lightning::ln::channelmanager", "Cannot send value that would put us over the max HTLC value in flight our peer will accept", 1); } // channel reserve is bigger than their_max_htlc_value_in_flight_msat so loop to deplete @@ -1918,8 +1907,7 @@ fn test_channel_reserve_holding_cell_htlcs() { let (_, our_payment_hash, our_payment_secret) = get_payment_preimage_hash!(nodes[2]); unwrap_send_err!(nodes[0].node.send_payment_with_route(&route, our_payment_hash, RecipientOnionFields::secret_only(our_payment_secret), PaymentId(our_payment_hash.0) - ), true, APIError::ChannelUnavailable { ref err }, - assert!(regex::Regex::new(r"Cannot send value that would put our balance under counterparty-announced channel reserve value \(\d+\)").unwrap().is_match(err))); + ), true, APIError::ChannelUnavailable { .. }, {}); assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); } @@ -1949,10 +1937,8 @@ fn test_channel_reserve_holding_cell_htlcs() { route.paths[0].hops.last_mut().unwrap().fee_msat += 1; unwrap_send_err!(nodes[0].node.send_payment_with_route(&route, our_payment_hash, RecipientOnionFields::secret_only(our_payment_secret), PaymentId(our_payment_hash.0) - ), true, APIError::ChannelUnavailable { ref err }, - assert!(regex::Regex::new(r"Cannot send value that would put our balance under counterparty-announced channel reserve value \(\d+\)").unwrap().is_match(err))); + ), true, APIError::ChannelUnavailable { .. }, {}); assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); - nodes[0].logger.assert_log_contains("lightning::ln::channelmanager", "Cannot send value that would put our balance under counterparty-announced channel reserve value", 2); } let (route_22, our_payment_hash_22, our_payment_preimage_22, our_payment_secret_22) = get_route_and_payment_hash!(nodes[0], nodes[2], recv_value_22); @@ -5735,9 +5721,6 @@ fn test_fail_holding_cell_htlc_upon_free() { chan_stat = get_channel_value_stat!(nodes[0], nodes[1], chan.2); assert_eq!(chan_stat.holding_cell_outbound_amount_msat, 0); nodes[0].logger.assert_log("lightning::ln::channel".to_string(), format!("Freeing holding cell with 1 HTLC updates in channel {}", hex::encode(chan.2)), 1); - let failure_log = format!("Failed to send HTLC with payment_hash {} due to Cannot send value that would put our balance under counterparty-announced channel reserve value ({}) in channel {}", - hex::encode(our_payment_hash.0), chan_stat.channel_reserve_msat, hex::encode(chan.2)); - nodes[0].logger.assert_log("lightning::ln::channel".to_string(), failure_log.to_string(), 1); // Check that the payment failed to be sent out. let events = nodes[0].node.get_and_clear_pending_events(); @@ -5826,9 +5809,6 @@ fn test_free_and_fail_holding_cell_htlcs() { chan_stat = get_channel_value_stat!(nodes[0], nodes[1], chan.2); assert_eq!(chan_stat.holding_cell_outbound_amount_msat, 0); nodes[0].logger.assert_log("lightning::ln::channel".to_string(), format!("Freeing holding cell with 2 HTLC updates in channel {}", hex::encode(chan.2)), 1); - let failure_log = format!("Failed to send HTLC with payment_hash {} due to Cannot send value that would put our balance under counterparty-announced channel reserve value ({}) in channel {}", - hex::encode(payment_hash_2.0), chan_stat.channel_reserve_msat, hex::encode(chan.2)); - nodes[0].logger.assert_log("lightning::ln::channel".to_string(), failure_log.to_string(), 1); // Check that the second payment failed to be sent out. let events = nodes[0].node.get_and_clear_pending_events(); @@ -6037,10 +6017,8 @@ fn test_update_add_htlc_bolt2_sender_value_below_minimum_msat() { unwrap_send_err!(nodes[0].node.send_payment_with_route(&route, our_payment_hash, RecipientOnionFields::secret_only(our_payment_secret), PaymentId(our_payment_hash.0) - ), true, APIError::ChannelUnavailable { ref err }, - assert!(regex::Regex::new(r"Cannot send less than their minimum HTLC value \(\d+\)").unwrap().is_match(err))); + ), true, APIError::ChannelUnavailable { .. }, {}); assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); - nodes[0].logger.assert_log_contains("lightning::ln::channelmanager", "Cannot send less than their minimum HTLC value", 1); } #[test] @@ -6146,11 +6124,9 @@ fn test_update_add_htlc_bolt2_sender_exceed_max_htlc_num_and_htlc_id_increment() } unwrap_send_err!(nodes[0].node.send_payment_with_route(&route, our_payment_hash, RecipientOnionFields::secret_only(our_payment_secret), PaymentId(our_payment_hash.0) - ), true, APIError::ChannelUnavailable { ref err }, - assert!(regex::Regex::new(r"Cannot push more than their max accepted HTLCs \(\d+\)").unwrap().is_match(err))); + ), true, APIError::ChannelUnavailable { .. }, {}); assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); - nodes[0].logger.assert_log_contains("lightning::ln::channelmanager", "Cannot push more than their max accepted HTLCs", 1); } #[test] @@ -6172,11 +6148,8 @@ fn test_update_add_htlc_bolt2_sender_exceed_max_htlc_value_in_flight() { route.paths[0].hops[0].fee_msat = max_in_flight + 1; unwrap_send_err!(nodes[0].node.send_payment_with_route(&route, our_payment_hash, RecipientOnionFields::secret_only(our_payment_secret), PaymentId(our_payment_hash.0) - ), true, APIError::ChannelUnavailable { ref err }, - assert!(regex::Regex::new(r"Cannot send value that would put us over the max HTLC value in flight our peer will accept \(\d+\)").unwrap().is_match(err))); - + ), true, APIError::ChannelUnavailable { .. }, {}); assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); - nodes[0].logger.assert_log_contains("lightning::ln::channelmanager", "Cannot send value that would put us over the max HTLC value in flight our peer will accept", 1); send_payment(&nodes[0], &[&nodes[1]], max_in_flight); } @@ -9693,23 +9666,15 @@ fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, exposure_breach_e if exposure_breach_event == ExposureEvent::AtHTLCForward { route.paths[0].hops.last_mut().unwrap().fee_msat = if on_holder_tx { dust_outbound_htlc_on_holder_tx_msat } else { dust_htlc_on_counterparty_tx_msat + 1 }; - let mut config = UserConfig::default(); // With default dust exposure: 5000 sats if on_holder_tx { - let dust_outbound_overflow = dust_outbound_htlc_on_holder_tx_msat * (dust_outbound_htlc_on_holder_tx + 1); - let dust_inbound_overflow = dust_inbound_htlc_on_holder_tx_msat * dust_inbound_htlc_on_holder_tx + dust_outbound_htlc_on_holder_tx_msat; unwrap_send_err!(nodes[0].node.send_payment_with_route(&route, payment_hash, RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0) - ), true, APIError::ChannelUnavailable { ref err }, - assert_eq!(err, &format!("Cannot send value that would put our exposure to dust HTLCs at {} over the limit {} on holder commitment tx", if dust_outbound_balance { dust_outbound_overflow } else { dust_inbound_overflow }, config.channel_config.max_dust_htlc_exposure_msat))); + ), true, APIError::ChannelUnavailable { .. }, {}); } else { unwrap_send_err!(nodes[0].node.send_payment_with_route(&route, payment_hash, RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0) - ), true, APIError::ChannelUnavailable { ref err }, - assert_eq!(err, - &format!("Cannot send value that would put our exposure to dust HTLCs at {} over the limit {} on counterparty commitment tx", - dust_htlc_on_counterparty_tx_msat * (dust_htlc_on_counterparty_tx - 1) + dust_htlc_on_counterparty_tx_msat + 1, - config.channel_config.max_dust_htlc_exposure_msat))); + ), true, APIError::ChannelUnavailable { .. }, {}); } } else if exposure_breach_event == ExposureEvent::AtHTLCReception { let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], if on_holder_tx { dust_inbound_htlc_on_holder_tx_msat } else { dust_htlc_on_counterparty_tx_msat + 1 }); diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 9e044d1c92d..5efd1cb9148 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -2167,12 +2167,11 @@ fn retry_multi_path_single_failed_payment() { assert_eq!(events.len(), 1); match events[0] { Event::PaymentPathFailed { payment_hash: ev_payment_hash, payment_failed_permanently: false, - failure: PathFailure::InitialSend { err: APIError::ChannelUnavailable { err: ref err_msg }}, + failure: PathFailure::InitialSend { err: APIError::ChannelUnavailable { .. }}, short_channel_id: Some(expected_scid), .. } => { assert_eq!(payment_hash, ev_payment_hash); assert_eq!(expected_scid, route.paths[1].hops[0].short_channel_id); - assert!(err_msg.contains("max HTLC")); }, _ => panic!("Unexpected event"), } @@ -2242,12 +2241,11 @@ fn immediate_retry_on_failure() { assert_eq!(events.len(), 1); match events[0] { Event::PaymentPathFailed { payment_hash: ev_payment_hash, payment_failed_permanently: false, - failure: PathFailure::InitialSend { err: APIError::ChannelUnavailable { err: ref err_msg }}, + failure: PathFailure::InitialSend { err: APIError::ChannelUnavailable { .. }}, short_channel_id: Some(expected_scid), .. } => { assert_eq!(payment_hash, ev_payment_hash); assert_eq!(expected_scid, route.paths[1].hops[0].short_channel_id); - assert!(err_msg.contains("max HTLC")); }, _ => panic!("Unexpected event"), } From 4bc3a7936522010e55fa3de9a60bc6f213f0781c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 29 May 2023 18:50:35 +0000 Subject: [PATCH 10/10] Slightly improve docs on `next_*_commit_tx_fee_msat` --- lightning/src/ln/channel.rs | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 577223c6382..32750d048ca 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2827,10 +2827,16 @@ impl Channel { feerate_per_kw as u64 * (commitment_tx_base_weight(opt_anchors) + num_htlcs as u64 * COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000 } - // Get the commitment tx fee for the local's (i.e. our) next commitment transaction based on the - // number of pending HTLCs that are on track to be in our next commitment tx, plus an additional - // HTLC if `fee_spike_buffer_htlc` is Some, plus a new HTLC given by `new_htlc_amount`. Dust HTLCs - // are excluded. + /// Get the commitment tx fee for the local's (i.e. our) next commitment transaction based on the + /// number of pending HTLCs that are on track to be in our next commitment tx. + /// + /// Optionally includes the `HTLCCandidate` given by `htlc` and an additional non-dust HTLC if + /// `fee_spike_buffer_htlc` is `Some`. + /// + /// The first extra HTLC is useful for determining whether we can accept a further HTLC, the + /// second allows for creating a buffer to ensure a further HTLC can always be accepted/added. + /// + /// Dust HTLCs are excluded. fn next_local_commit_tx_fee_msat(&self, htlc: HTLCCandidate, fee_spike_buffer_htlc: Option<()>) -> u64 { assert!(self.is_outbound()); @@ -2924,10 +2930,16 @@ impl Channel { res } - // Get the commitment tx fee for the remote's next commitment transaction based on the number of - // pending HTLCs that are on track to be in their next commitment tx, plus an additional HTLC if - // `fee_spike_buffer_htlc` is Some, plus a new HTLC given by `new_htlc_amount`. Dust HTLCs are - // excluded. + /// Get the commitment tx fee for the remote's next commitment transaction based on the number of + /// pending HTLCs that are on track to be in their next commitment tx + /// + /// Optionally includes the `HTLCCandidate` given by `htlc` and an additional non-dust HTLC if + /// `fee_spike_buffer_htlc` is `Some`. + /// + /// The first extra HTLC is useful for determining whether we can accept a further HTLC, the + /// second allows for creating a buffer to ensure a further HTLC can always be accepted/added. + /// + /// Dust HTLCs are excluded. fn next_remote_commit_tx_fee_msat(&self, htlc: HTLCCandidate, fee_spike_buffer_htlc: Option<()>) -> u64 { assert!(!self.is_outbound());