From a5302633f9c8a0c07ddd2bf5527e56959cd8bdf6 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Mon, 28 Apr 2025 18:39:48 +0000 Subject: [PATCH 1/8] Fix spurious MPP pathfinding failure This bug was recently surfaced to us by a user who wrote a test where the sender is attempting to route an MPP payment split across the two channels that it has with its LSP, where the LSP has a single large channel to the recipient. Previously this led to a pathfinding failure because our router was not sending the maximum possible value over the first MPP path it found due to overestimating the fees needed to cover the following hops. Because the path that had just been found was not maxxed out, our router assumed that we had already found enough paths to cover the full payment amount and that we were finding additional paths for the purpose of redundant path selection. This caused the router to mark the recipient's only channel as exhausted, with the intention of choosing more unique paths in future iterations. In reality, this ended up with the recipient's only channel being disabled and subsequently failing to find a route entirely. Update the router to fully utilize the capacity of any paths it finds in this situation, preventing the "redundant path selection" behavior from kicking in. Use and `rustfmt` conflicts resolved in: * lightning/src/blinded_path/payment.rs The new test was also updated to avoid APIs not present on 0.1. --- lightning/src/blinded_path/payment.rs | 49 ++++++++++++++------ lightning/src/ln/payment_tests.rs | 52 +++++++++++++++++++++ lightning/src/routing/router.rs | 67 +++++++++++++++++++++------ 3 files changed, 139 insertions(+), 29 deletions(-) diff --git a/lightning/src/blinded_path/payment.rs b/lightning/src/blinded_path/payment.rs index e3a81927146..901f717ea93 100644 --- a/lightning/src/blinded_path/payment.rs +++ b/lightning/src/blinded_path/payment.rs @@ -30,6 +30,7 @@ use crate::offers::nonce::Nonce; use crate::offers::offer::OfferId; use crate::routing::gossip::{NodeId, ReadOnlyNetworkGraph}; use crate::sign::{EntropySource, NodeSigner, Recipient}; +use crate::types::routing::RoutingFees; use crate::util::ser::{FixedLengthReader, LengthReadableArgs, HighZeroBytesDroppedBigSize, Readable, WithoutLength, Writeable, Writer}; use core::mem; @@ -530,20 +531,17 @@ pub(crate) fn amt_to_forward_msat(inbound_amt_msat: u64, payment_relay: &Payment u64::try_from(amt_to_forward).ok() } -pub(super) fn compute_payinfo( - intermediate_nodes: &[PaymentForwardNode], payee_tlvs: &UnauthenticatedReceiveTlvs, - payee_htlc_maximum_msat: u64, min_final_cltv_expiry_delta: u16, -) -> Result { +// Returns (aggregated_base_fee, aggregated_proportional_fee) +pub(crate) fn compute_aggregated_base_prop_fee(hops_fees: I) -> Result<(u64, u64), ()> +where + I: DoubleEndedIterator, +{ let mut curr_base_fee: u64 = 0; let mut curr_prop_mil: u64 = 0; - let mut cltv_expiry_delta: u16 = min_final_cltv_expiry_delta; - for tlvs in intermediate_nodes.iter().rev().map(|n| &n.tlvs) { - // In the future, we'll want to take the intersection of all supported features for the - // `BlindedPayInfo`, but there are no features in that context right now. - if tlvs.features.requires_unknown_bits_from(&BlindedHopFeatures::empty()) { return Err(()) } + for fees in hops_fees.rev() { + let next_base_fee = fees.base_msat as u64; + let next_prop_mil = fees.proportional_millionths as u64; - let next_base_fee = tlvs.payment_relay.fee_base_msat as u64; - let next_prop_mil = tlvs.payment_relay.fee_proportional_millionths as u64; // Use integer arithmetic to compute `ceil(a/b)` as `(a+b-1)/b` // ((curr_base_fee * (1_000_000 + next_prop_mil)) / 1_000_000) + next_base_fee curr_base_fee = curr_base_fee.checked_mul(1_000_000 + next_prop_mil) @@ -558,13 +556,34 @@ pub(super) fn compute_payinfo( .map(|f| f / 1_000_000) .and_then(|f| f.checked_sub(1_000_000)) .ok_or(())?; - - cltv_expiry_delta = cltv_expiry_delta.checked_add(tlvs.payment_relay.cltv_expiry_delta).ok_or(())?; } + Ok((curr_base_fee, curr_prop_mil)) +} + +pub(super) fn compute_payinfo( + intermediate_nodes: &[PaymentForwardNode], payee_tlvs: &UnauthenticatedReceiveTlvs, + payee_htlc_maximum_msat: u64, min_final_cltv_expiry_delta: u16, +) -> Result { + let (aggregated_base_fee, aggregated_prop_fee) = + compute_aggregated_base_prop_fee(intermediate_nodes.iter().map(|node| RoutingFees { + base_msat: node.tlvs.payment_relay.fee_base_msat, + proportional_millionths: node.tlvs.payment_relay.fee_proportional_millionths, + }))?; + let mut htlc_minimum_msat: u64 = 1; let mut htlc_maximum_msat: u64 = 21_000_000 * 100_000_000 * 1_000; // Total bitcoin supply + let mut cltv_expiry_delta: u16 = min_final_cltv_expiry_delta; for node in intermediate_nodes.iter() { + // In the future, we'll want to take the intersection of all supported features for the + // `BlindedPayInfo`, but there are no features in that context right now. + if node.tlvs.features.requires_unknown_bits_from(&BlindedHopFeatures::empty()) { + return Err(()); + } + + cltv_expiry_delta = + cltv_expiry_delta.checked_add(node.tlvs.payment_relay.cltv_expiry_delta).ok_or(())?; + // The min htlc for an intermediate node is that node's min minus the fees charged by all of the // following hops for forwarding that min, since that fee amount will automatically be included // in the amount that this node receives and contribute towards reaching its min. @@ -583,8 +602,8 @@ pub(super) fn compute_payinfo( if htlc_maximum_msat < htlc_minimum_msat { return Err(()) } Ok(BlindedPayInfo { - fee_base_msat: u32::try_from(curr_base_fee).map_err(|_| ())?, - fee_proportional_millionths: u32::try_from(curr_prop_mil).map_err(|_| ())?, + fee_base_msat: u32::try_from(aggregated_base_fee).map_err(|_| ())?, + fee_proportional_millionths: u32::try_from(aggregated_prop_fee).map_err(|_| ())?, cltv_expiry_delta, htlc_minimum_msat, htlc_maximum_msat, diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 0963ed0aa4f..348cace949d 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -4479,3 +4479,55 @@ fn pay_route_without_params() { ClaimAlongRouteArgs::new(&nodes[0], &[&[&nodes[1]]], payment_preimage) ); } + +#[test] +fn max_out_mpp_path() { + // In this setup, the sender is attempting to route an MPP payment split across the two channels + // that it has with its LSP, where the LSP has a single large channel to the recipient. + // + // Previously a user ran into a pathfinding failure here because our router was not sending the + // maximum possible value over the first MPP path it found due to overestimating the fees needed + // to cover the following hops. Because the path that had just been found was not maxxed out, our + // router assumed that we had already found enough paths to cover the full payment amount and that + // we were finding additional paths for the purpose of redundant path selection. This caused the + // router to mark the recipient's only channel as exhausted, with the intention of choosing more + // unique paths in future iterations. In reality, this ended up with the recipient's only channel + // being disabled and subsequently failing to find a route entirely. + // + // The router has since been updated to fully utilize the capacity of any paths it finds in this + // situation, preventing the "redundant path selection" behavior from kicking in. + + let mut user_cfg = test_default_channel_config(); + user_cfg.channel_config.forwarding_fee_base_msat = 0; + user_cfg.channel_handshake_config.max_inbound_htlc_value_in_flight_percent_of_channel = 100; + let mut lsp_cfg = test_default_channel_config(); + lsp_cfg.channel_config.forwarding_fee_base_msat = 0; + lsp_cfg.channel_config.forwarding_fee_proportional_millionths = 3000; + lsp_cfg.channel_handshake_config.max_inbound_htlc_value_in_flight_percent_of_channel = 100; + + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs( + 3, &node_cfgs, &[Some(user_cfg.clone()), Some(lsp_cfg.clone()), Some(user_cfg.clone())] + ); + let nodes = create_network(3, &node_cfgs, &node_chanmgrs); + + create_unannounced_chan_between_nodes_with_value(&nodes, 0, 1, 200_000, 0); + create_unannounced_chan_between_nodes_with_value(&nodes, 0, 1, 300_000, 0); + create_unannounced_chan_between_nodes_with_value(&nodes, 1, 2, 600_000, 0); + + let amt_msat = 350_000_000; + let invoice_params = crate::ln::channelmanager::Bolt11InvoiceParameters { + amount_msats: Some(amt_msat), + ..Default::default() + }; + let invoice = nodes[2].node.create_bolt11_invoice(invoice_params).unwrap(); + + let (hash, onion, params) = + crate::ln::bolt11_payment::payment_parameters_from_invoice(&invoice).unwrap(); + nodes[0].node.send_payment(hash, onion, PaymentId([42; 32]), params, Retry::Attempts(0)).unwrap(); + + assert!(nodes[0].node.list_recent_payments().len() == 1); + check_added_monitors(&nodes[0], 2); // one monitor update per MPP part + nodes[0].node.get_and_clear_pending_msg_events(); +} diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 079b83563c3..ce9108eddbe 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1890,9 +1890,10 @@ impl<'a> PaymentPath<'a> { // that it the value being transferred has decreased while we were doing path finding, leading // to the fees being paid not lining up with the actual limits. // - // Note that this function is not aware of the available_liquidity limit, and thus does not - // support increasing the value being transferred beyond what was selected during the initial - // routing passes. + // This function may also be used to increase the value being transferred in the case that + // overestimating later hops' fees caused us to underutilize earlier hops' capacity. + // + // Note that this function is not aware of the available_liquidity limit of any hops. // // Returns the amount that this path contributes to the total payment value, which may be greater // than `value_msat` if we had to overpay to meet the final node's `htlc_minimum_msat`. @@ -1957,15 +1958,56 @@ impl<'a> PaymentPath<'a> { cur_hop.hop_use_fee_msat = new_fee; total_fee_paid_msat += new_fee; } else { - // It should not be possible because this function is called only to reduce the - // value. In that case, compute_fee was already called with the same fees for - // larger amount and there was no overflow. + // It should not be possible because this function is only called either to reduce the + // value or with a larger amount that was already checked for overflow in + // `compute_max_final_value_contribution`. In the former case, compute_fee was already + // called with the same fees for larger amount and there was no overflow. unreachable!(); } } } value_msat + extra_contribution_msat } + + // Returns the maximum contribution that this path can make to the final value of the payment. May + // be slightly lower than the actual max due to rounding errors when aggregating fees along the + // path. + fn compute_max_final_value_contribution( + &self, used_liquidities: &HashMap, channel_saturation_pow_half: u8 + ) -> u64 { + let mut max_path_contribution = u64::MAX; + for (idx, (hop, _)) in self.hops.iter().enumerate() { + let hop_effective_capacity_msat = hop.candidate.effective_capacity(); + let hop_max_msat = max_htlc_from_capacity( + hop_effective_capacity_msat, channel_saturation_pow_half + ).saturating_sub(*used_liquidities.get(&hop.candidate.id()).unwrap_or(&0_u64)); + + let next_hops_feerates_iter = self.hops + .iter() + .skip(idx + 1) + .map(|(hop, _)| hop.candidate.fees()); + + // Aggregate the fees of the hops that come after this one, and use those fees to compute the + // maximum amount that this hop can contribute to the final value received by the payee. + let (next_hops_aggregated_base, next_hops_aggregated_prop) = + crate::blinded_path::payment::compute_aggregated_base_prop_fee(next_hops_feerates_iter).unwrap(); + + // ceil(((hop_max_msat - agg_base) * 1_000_000) / (1_000_000 + agg_prop)) + let hop_max_final_value_contribution = (hop_max_msat as u128) + .checked_sub(next_hops_aggregated_base as u128) + .and_then(|f| f.checked_mul(1_000_000)) + .and_then(|f| f.checked_add(1_000_000 - 1)) + .and_then(|f| f.checked_add(next_hops_aggregated_prop as u128)) + .map(|f| f / ((next_hops_aggregated_prop as u128).saturating_add(1_000_000))); + + if let Some(hop_contribution) = hop_max_final_value_contribution { + let hop_contribution: u64 = hop_contribution.try_into().unwrap_or(u64::MAX); + max_path_contribution = core::cmp::min(hop_contribution, max_path_contribution); + } else { debug_assert!(false); } + } + + max_path_contribution + } } #[inline(always)] @@ -3116,7 +3158,10 @@ where L::Target: Logger { // recompute the fees again, so that if that's the case, we match the currently // underpaid htlc_minimum_msat with fees. debug_assert_eq!(payment_path.get_value_msat(), value_contribution_msat); - let desired_value_contribution = cmp::min(value_contribution_msat, final_value_msat); + let max_path_contribution_msat = payment_path.compute_max_final_value_contribution( + &used_liquidities, channel_saturation_pow_half + ); + let desired_value_contribution = cmp::min(max_path_contribution_msat, final_value_msat); value_contribution_msat = payment_path.update_value_and_recompute_fees(desired_value_contribution); // Since a path allows to transfer as much value as @@ -3128,7 +3173,6 @@ where L::Target: Logger { // might have been computed considering a larger value. // Remember that we used these channels so that we don't rely // on the same liquidity in future paths. - let mut prevented_redundant_path_selection = false; for (hop, _) in payment_path.hops.iter() { let spent_on_hop_msat = value_contribution_msat + hop.next_hops_fee_msat; let used_liquidity_msat = used_liquidities @@ -3137,14 +3181,9 @@ where L::Target: Logger { .or_insert(spent_on_hop_msat); let hop_capacity = hop.candidate.effective_capacity(); let hop_max_msat = max_htlc_from_capacity(hop_capacity, channel_saturation_pow_half); - if *used_liquidity_msat == hop_max_msat { - // If this path used all of this channel's available liquidity, we know - // this path will not be selected again in the next loop iteration. - prevented_redundant_path_selection = true; - } debug_assert!(*used_liquidity_msat <= hop_max_msat); } - if !prevented_redundant_path_selection { + if max_path_contribution_msat > value_contribution_msat { // If we weren't capped by hitting a liquidity limit on a channel in the path, // we'll probably end up picking the same path again on the next iteration. // Decrease the available liquidity of a hop in the middle of the path. From 8d8f15bcb74c56259687c444f3cec3674f1b2d53 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 28 Apr 2025 23:43:37 +0000 Subject: [PATCH 2/8] Use `floor` when computing max value of a path, not `ceil` When calculating the maximum contribution of a path to a larger route, we want to ensure we don't overshoot as that might cause us to violate a maximum value limit. In 209cb2aa2e0d67bf89a130b070f7116178e9ddb4, we started by calculating with `ceil`, which can trigger exactly that, so here we drop the extra addition, switching us to `floor`. Found both by the `router` fuzzer as well as the `generate_large_mpp_routes` test. --- lightning/src/routing/router.rs | 96 ++++++++++++++++++++++++++++++++- 1 file changed, 94 insertions(+), 2 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index ce9108eddbe..c09a014dc62 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1992,11 +1992,10 @@ impl<'a> PaymentPath<'a> { let (next_hops_aggregated_base, next_hops_aggregated_prop) = crate::blinded_path::payment::compute_aggregated_base_prop_fee(next_hops_feerates_iter).unwrap(); - // ceil(((hop_max_msat - agg_base) * 1_000_000) / (1_000_000 + agg_prop)) + // floor(((hop_max_msat - agg_base) * 1_000_000) / (1_000_000 + agg_prop)) let hop_max_final_value_contribution = (hop_max_msat as u128) .checked_sub(next_hops_aggregated_base as u128) .and_then(|f| f.checked_mul(1_000_000)) - .and_then(|f| f.checked_add(1_000_000 - 1)) .and_then(|f| f.checked_add(next_hops_aggregated_prop as u128)) .map(|f| f / ((next_hops_aggregated_prop as u128).saturating_add(1_000_000))); @@ -8504,6 +8503,99 @@ mod tests { assert_eq!(route.get_total_fees(), 123); } + #[test] + fn test_max_final_contribution() { + // When `compute_max_final_value_contribution` was added, it had a bug where it would + // over-estimate the maximum value contribution of a hop by using `ceil` rather than + // `floor`. This tests that case by attempting to send 1 million sats over a channel where + // the remaining hops have a base fee of zero and a proportional fee of 1 millionth. + + let (secp_ctx, network_graph, gossip_sync, _, logger) = build_graph(); + let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx); + let scorer = ln_test_utils::TestScorer::new(); + let random_seed_bytes = [42; 32]; + + // Enable channel 1, setting max HTLC to 1M sats + update_channel(&gossip_sync, &secp_ctx, &our_privkey, UnsignedChannelUpdate { + chain_hash: ChainHash::using_genesis_block(Network::Testnet), + short_channel_id: 1, + timestamp: 2, + message_flags: 1, // Only must_be_one + channel_flags: 0, + cltv_expiry_delta: (1 << 4) | 1, + htlc_minimum_msat: 0, + htlc_maximum_msat: 1_000_000, + fee_base_msat: 0, + fee_proportional_millionths: 0, + excess_data: Vec::new() + }); + + // Set the fee on channel 3 to zero + update_channel(&gossip_sync, &secp_ctx, &privkeys[0], UnsignedChannelUpdate { + chain_hash: ChainHash::using_genesis_block(Network::Testnet), + short_channel_id: 3, + timestamp: 2, + message_flags: 1, // Only must_be_one + channel_flags: 0, + cltv_expiry_delta: (3 << 4) | 1, + htlc_minimum_msat: 0, + htlc_maximum_msat: 1_000_000_000, + fee_base_msat: 0, + fee_proportional_millionths: 0, + excess_data: Vec::new() + }); + + // Set the fee on channel 6 to 1 millionth + update_channel(&gossip_sync, &secp_ctx, &privkeys[2], UnsignedChannelUpdate { + chain_hash: ChainHash::using_genesis_block(Network::Testnet), + short_channel_id: 6, + timestamp: 2, + message_flags: 1, // Only must_be_one + channel_flags: 0, + cltv_expiry_delta: (6 << 4) | 1, + htlc_minimum_msat: 0, + htlc_maximum_msat: 1_000_000_000, + fee_base_msat: 0, + fee_proportional_millionths: 1, + excess_data: Vec::new() + }); + + // Now attempt to pay over the channel 1 -> channel 3 -> channel 6 path + // This should fail as we need to send 1M + 1 sats to cover the fee but channel 1 only + // allows for 1M sats to flow over it. + let config = UserConfig::default(); + let payment_params = PaymentParameters::from_node_id(nodes[4], 42) + .with_bolt11_features(channelmanager::provided_bolt11_invoice_features(&config)) + .unwrap(); + let route_params = RouteParameters::from_payment_params_and_value(payment_params, 1_000_000); + get_route(&our_id, &route_params, &network_graph.read_only(), None, + Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes).unwrap_err(); + + // Now set channel 1 max HTLC to 1M + 1 sats + update_channel(&gossip_sync, &secp_ctx, &our_privkey, UnsignedChannelUpdate { + chain_hash: ChainHash::using_genesis_block(Network::Testnet), + short_channel_id: 1, + timestamp: 3, + message_flags: 1, // Only must_be_one + channel_flags: 0, + cltv_expiry_delta: (1 << 4) | 1, + htlc_minimum_msat: 0, + htlc_maximum_msat: 1_000_001, + fee_base_msat: 0, + fee_proportional_millionths: 0, + excess_data: Vec::new() + }); + + // And attempt the same payment again, but this time it should work. + let route = get_route(&our_id, &route_params, &network_graph.read_only(), None, + Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes).unwrap(); + assert_eq!(route.paths.len(), 1); + assert_eq!(route.paths[0].hops.len(), 3); + assert_eq!(route.paths[0].hops[0].short_channel_id, 1); + assert_eq!(route.paths[0].hops[1].short_channel_id, 3); + assert_eq!(route.paths[0].hops[2].short_channel_id, 6); + } + #[test] fn allow_us_being_first_hint() { // Check that we consider a route hint even if we are the src of the first hop. From 42c8dd4cba75308bc38546512d2d6c30a1196565 Mon Sep 17 00:00:00 2001 From: Philip Kannegaard Hayes Date: Tue, 22 Apr 2025 17:57:44 -0700 Subject: [PATCH 3/8] offers: avoid panic when truncating payer_note in UTF-8 code point `String::truncate` takes a byte index but panics if we split in the middle of a UTF-8 codepoint. Sadly, in `InvoiceRequest::fields` we want to tuncate the payer note to a maximum of 512 bytes, which may be in the middle of a UTF-8 codepoint and can cause panic. Here we iterate over the bytes in the string until we find one not in the middle of a UTF-8 codepoint and then split the string there. Trivial `rustfmt` conflicts resolved in: * lightning/src/offers/invoice_request.rs --- lightning/src/offers/invoice_request.rs | 80 ++++++++++++++++++++++--- 1 file changed, 71 insertions(+), 9 deletions(-) diff --git a/lightning/src/offers/invoice_request.rs b/lightning/src/offers/invoice_request.rs index 8d7d25cf2b5..fc4d7d274bd 100644 --- a/lightning/src/offers/invoice_request.rs +++ b/lightning/src/offers/invoice_request.rs @@ -944,13 +944,37 @@ impl VerifiedInvoiceRequest { InvoiceRequestFields { payer_signing_pubkey: *payer_signing_pubkey, quantity: *quantity, - payer_note_truncated: payer_note.clone() - .map(|mut s| { s.truncate(PAYER_NOTE_LIMIT); UntrustedString(s) }), + payer_note_truncated: payer_note + .clone() + // Truncate the payer note to `PAYER_NOTE_LIMIT` bytes, rounding + // down to the nearest valid UTF-8 code point boundary. + .map(|s| UntrustedString(string_truncate_safe(s, PAYER_NOTE_LIMIT))), human_readable_name: self.offer_from_hrn().clone(), } } } +/// `String::truncate(new_len)` panics if you split inside a UTF-8 code point, +/// which would leave the `String` containing invalid UTF-8. This function will +/// instead truncate the string to the next smaller code point boundary so the +/// truncated string always remains valid UTF-8. +/// +/// This can still split a grapheme cluster, but that's probably fine. +/// We'd otherwise have to pull in the `unicode-segmentation` crate and its big +/// unicode tables to find the next smaller grapheme cluster boundary. +fn string_truncate_safe(mut s: String, new_len: usize) -> String { + // Finds the largest byte index `x` not exceeding byte index `index` where + // `s.is_char_boundary(x)` is true. + // TODO(phlip9): remove when `std::str::floor_char_boundary` stabilizes. + let truncated_len = if new_len >= s.len() { + s.len() + } else { + (0..=new_len).rev().find(|idx| s.is_char_boundary(*idx)).unwrap_or(0) + }; + s.truncate(truncated_len); + s +} + impl InvoiceRequestContents { pub(super) fn metadata(&self) -> &[u8] { self.inner.metadata() @@ -1339,7 +1363,8 @@ mod tests { use crate::ln::inbound_payment::ExpandedKey; use crate::ln::msgs::{DecodeError, MAX_VALUE_MSAT}; use crate::offers::invoice::{Bolt12Invoice, SIGNATURE_TAG as INVOICE_SIGNATURE_TAG}; - use crate::offers::merkle::{SignatureTlvStreamRef, TaggedHash, TlvStream, self}; + use crate::offers::invoice_request::string_truncate_safe; + use crate::offers::merkle::{self, SignatureTlvStreamRef, TaggedHash, TlvStream}; use crate::offers::nonce::Nonce; use crate::offers::offer::{Amount, ExperimentalOfferTlvStreamRef, OfferTlvStreamRef, Quantity}; #[cfg(not(c_bindings))] @@ -2611,12 +2636,22 @@ mod tests { .build().unwrap(); assert_eq!(offer.issuer_signing_pubkey(), Some(node_id)); + // UTF-8 payer note that we can't naively `.truncate(PAYER_NOTE_LIMIT)` + // because it would split a multi-byte UTF-8 code point. + let payer_note = "❤️".repeat(86); + assert_eq!(payer_note.len(), PAYER_NOTE_LIMIT + 4); + let expected_payer_note = "❤️".repeat(85); + let invoice_request = offer - .request_invoice(&expanded_key, nonce, &secp_ctx, payment_id).unwrap() - .chain(Network::Testnet).unwrap() - .quantity(1).unwrap() - .payer_note("0".repeat(PAYER_NOTE_LIMIT * 2)) - .build_and_sign().unwrap(); + .request_invoice(&expanded_key, nonce, &secp_ctx, payment_id) + .unwrap() + .chain(Network::Testnet) + .unwrap() + .quantity(1) + .unwrap() + .payer_note(payer_note) + .build_and_sign() + .unwrap(); match invoice_request.verify_using_metadata(&expanded_key, &secp_ctx) { Ok(invoice_request) => { let fields = invoice_request.fields(); @@ -2626,7 +2661,7 @@ mod tests { InvoiceRequestFields { payer_signing_pubkey: invoice_request.payer_signing_pubkey(), quantity: Some(1), - payer_note_truncated: Some(UntrustedString("0".repeat(PAYER_NOTE_LIMIT))), + payer_note_truncated: Some(UntrustedString(expected_payer_note)), human_readable_name: None, } ); @@ -2641,4 +2676,31 @@ mod tests { Err(_) => panic!("unexpected error"), } } + + #[test] + fn test_string_truncate_safe() { + // We'll correctly truncate to the nearest UTF-8 code point boundary: + // ❤ variation-selector + // e29da4 efb88f + let s = String::from("❤️"); + assert_eq!(s.len(), 6); + assert_eq!(s, string_truncate_safe(s.clone(), 7)); + assert_eq!(s, string_truncate_safe(s.clone(), 6)); + assert_eq!("❤", string_truncate_safe(s.clone(), 5)); + assert_eq!("❤", string_truncate_safe(s.clone(), 4)); + assert_eq!("❤", string_truncate_safe(s.clone(), 3)); + assert_eq!("", string_truncate_safe(s.clone(), 2)); + assert_eq!("", string_truncate_safe(s.clone(), 1)); + assert_eq!("", string_truncate_safe(s.clone(), 0)); + + // Every byte in an ASCII string is also a full UTF-8 code point. + let s = String::from("my ASCII string!"); + for new_len in 0..(s.len() + 5) { + if new_len >= s.len() { + assert_eq!(s, string_truncate_safe(s.clone(), new_len)); + } else { + assert_eq!(s[..new_len], string_truncate_safe(s.clone(), new_len)); + } + } + } } From cf684faaea0c8cac5ae84741bbb851370da76682 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 25 Apr 2025 18:07:42 +0000 Subject: [PATCH 4/8] Make it easier for the fuzzer to get a `VerifiedInvoiceRequest` In the next commit we attempt to verify `InvoiceRequest`s when fuzzing so that we can test fetching the `InvoiceRequestFields`, but its useful to allow the verification to succeed more often first, which we do here. --- lightning/src/offers/signer.rs | 41 +++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/lightning/src/offers/signer.rs b/lightning/src/offers/signer.rs index fa9fdfa3467..acfa169bffb 100644 --- a/lightning/src/offers/signer.rs +++ b/lightning/src/offers/signer.rs @@ -361,19 +361,30 @@ fn verify_metadata( let derived_keys = Keypair::from_secret_key( secp_ctx, &SecretKey::from_slice(hmac.as_byte_array()).unwrap() ); - if fixed_time_eq(&signing_pubkey.serialize(), &derived_keys.public_key().serialize()) { + #[allow(unused_mut)] + let mut ok = fixed_time_eq(&signing_pubkey.serialize(), &derived_keys.public_key().serialize()); + #[cfg(fuzzing)] + if metadata[0] & 1 == 0 { + ok = true; + } + if ok { Ok(Some(derived_keys)) } else { Err(()) } - } else if metadata[Nonce::LENGTH..].len() == Sha256::LEN { - if fixed_time_eq(&metadata[Nonce::LENGTH..], &hmac.to_byte_array()) { + } else { + #[allow(unused_mut)] + let mut ok = metadata.len() == Nonce::LENGTH + Sha256::LEN + && fixed_time_eq(&metadata[Nonce::LENGTH..], &hmac.to_byte_array()); + #[cfg(fuzzing)] + if metadata.is_empty() || metadata[0] & 1 == 0 { + ok = true; + } + if ok { Ok(None) } else { Err(()) } - } else { - Err(()) } } @@ -381,16 +392,20 @@ fn hmac_for_message<'a>( metadata: &[u8], expanded_key: &ExpandedKey, iv_bytes: &[u8; IV_LEN], tlv_stream: impl core::iter::Iterator> ) -> Result, ()> { - if metadata.len() < Nonce::LENGTH { - return Err(()); - } - - let nonce = match Nonce::try_from(&metadata[..Nonce::LENGTH]) { - Ok(nonce) => nonce, - Err(_) => return Err(()), - }; let mut hmac = expanded_key.hmac_for_offer(); hmac.input(iv_bytes); + + let nonce = if metadata.len() < Nonce::LENGTH { + // In fuzzing its relatively challenging for the fuzzer to find cases where we have issues + // in a BOLT 12 object but also have a right-sized nonce. So instead we allow any size + // nonce. + if !cfg!(fuzzing) { + return Err(()); + } + Nonce::try_from(&[42; Nonce::LENGTH][..]).unwrap() + } else { + Nonce::try_from(&metadata[..Nonce::LENGTH])? + }; hmac.input(&nonce.0); for record in tlv_stream { From 406e0314bf716e4f59e60798b2061d914d615f42 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 25 Apr 2025 18:22:03 +0000 Subject: [PATCH 5/8] Fuzz fetching `InvoiceRequestFields` from `VerifiedInvoiceRequest`s This should allow us to reach the panic from two commits ago from the fuzzer. --- fuzz/src/invoice_request_deser.rs | 26 +++++++++++++++++-------- lightning/src/offers/invoice_request.rs | 14 ++++++++++++- 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/fuzz/src/invoice_request_deser.rs b/fuzz/src/invoice_request_deser.rs index 37668c1d801..beff5c21a0b 100644 --- a/fuzz/src/invoice_request_deser.rs +++ b/fuzz/src/invoice_request_deser.rs @@ -85,16 +85,26 @@ fn build_response( let expanded_key = ExpandedKey::new([42; 32]); let entropy_source = Randomness {}; let nonce = Nonce::from_entropy_source(&entropy_source); + + let invoice_request_fields = + if let Ok(ver) = invoice_request.clone().verify_using_metadata(&expanded_key, secp_ctx) { + // Previously we had a panic where we'd truncate the payer note possibly cutting a + // Unicode character in two here, so try to fetch fields if we can validate. + ver.fields() + } else { + InvoiceRequestFields { + payer_signing_pubkey: invoice_request.payer_signing_pubkey(), + quantity: invoice_request.quantity(), + payer_note_truncated: invoice_request + .payer_note() + .map(|s| UntrustedString(s.to_string())), + human_readable_name: None, + } + }; + let payment_context = PaymentContext::Bolt12Offer(Bolt12OfferContext { offer_id: OfferId([42; 32]), - invoice_request: InvoiceRequestFields { - payer_signing_pubkey: invoice_request.payer_signing_pubkey(), - quantity: invoice_request.quantity(), - payer_note_truncated: invoice_request - .payer_note() - .map(|s| UntrustedString(s.to_string())), - human_readable_name: None, - }, + invoice_request: invoice_request_fields, }); let payee_tlvs = UnauthenticatedReceiveTlvs { payment_secret: PaymentSecret([42; 32]), diff --git a/lightning/src/offers/invoice_request.rs b/lightning/src/offers/invoice_request.rs index fc4d7d274bd..36491fdb645 100644 --- a/lightning/src/offers/invoice_request.rs +++ b/lightning/src/offers/invoice_request.rs @@ -933,7 +933,14 @@ impl VerifiedInvoiceRequest { #[cfg(c_bindings)] invoice_request_respond_with_derived_signing_pubkey_methods!(self, self.inner, InvoiceWithDerivedSigningPubkeyBuilder); - pub(crate) fn fields(&self) -> InvoiceRequestFields { + /// Fetch the [`InvoiceRequestFields`] for this verified invoice. + /// + /// These are fields which we expect to be useful when receiving a payment for this invoice + /// request, and include the returned [`InvoiceRequestFields`] in the + /// [`PaymentContext::Bolt12Offer`]. + /// + /// [`PaymentContext::Bolt12Offer`]: crate::blinded_path::payment::PaymentContext::Bolt12Offer + pub fn fields(&self) -> InvoiceRequestFields { let InvoiceRequestContents { payer_signing_pubkey, inner: InvoiceRequestContentsWithoutPayerSigningPubkey { @@ -1316,8 +1323,13 @@ pub struct InvoiceRequestFields { } /// The maximum number of characters included in [`InvoiceRequestFields::payer_note_truncated`]. +#[cfg(not(fuzzing))] pub const PAYER_NOTE_LIMIT: usize = 512; +/// The maximum number of characters included in [`InvoiceRequestFields::payer_note_truncated`]. +#[cfg(fuzzing)] +pub const PAYER_NOTE_LIMIT: usize = 8; + impl Writeable for InvoiceRequestFields { fn write(&self, writer: &mut W) -> Result<(), io::Error> { write_tlv_fields!(writer, { From d5f149dde1611f26c23a4e424797c5665ca1f767 Mon Sep 17 00:00:00 2001 From: shaavan Date: Wed, 12 Mar 2025 17:45:00 +0530 Subject: [PATCH 6/8] Make InvoiceReceived event generation idempotent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ensures `InvoiceReceived` events are not generated multiple times when `manually_handle_bolt12_invoice` is enabled. Duplicate events for the same invoice could cause confusion—this change introduces an idempotency check to prevent that. Conflicts resolved in: * lightning/src/ln/outbound_payment.rs due to the migration upstream from `max_total_routing_fee_msat` to a more general config struct. --- lightning/src/ln/channelmanager.rs | 5 ++ lightning/src/ln/outbound_payment.rs | 73 +++++++++++++++++++--------- 2 files changed, 55 insertions(+), 23 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 14d3c0ea5cb..33ad59a4139 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -12148,6 +12148,11 @@ where ); if self.default_configuration.manually_handle_bolt12_invoices { + // Update the corresponding entry in `PendingOutboundPayment` for this invoice. + // This ensures that event generation remains idempotent in case we receive + // the same invoice multiple times. + self.pending_outbound_payments.mark_invoice_received(&invoice, payment_id).ok()?; + let event = Event::InvoiceReceived { payment_id, invoice, context, responder, }; diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 7b579b7a261..1719b47dab3 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -73,9 +73,9 @@ pub(crate) enum PendingOutboundPayment { max_total_routing_fee_msat: Option, retryable_invoice_request: Option }, - // This state will never be persisted to disk because we transition from `AwaitingInvoice` to - // `Retryable` atomically within the `ChannelManager::total_consistency_lock`. Useful to avoid - // holding the `OutboundPayments::pending_outbound_payments` lock during pathfinding. + // Represents the state after the invoice has been received, transitioning from the corresponding + // `AwaitingInvoice` state. + // Helps avoid holding the `OutboundPayments::pending_outbound_payments` lock during pathfinding. InvoiceReceived { payment_hash: PaymentHash, retry_strategy: Retry, @@ -833,26 +833,8 @@ impl OutboundPayments { IH: Fn() -> InFlightHtlcs, SP: Fn(SendAlongPathArgs) -> Result<(), APIError>, { - let payment_hash = invoice.payment_hash(); - let max_total_routing_fee_msat; - let retry_strategy; - match self.pending_outbound_payments.lock().unwrap().entry(payment_id) { - hash_map::Entry::Occupied(entry) => match entry.get() { - PendingOutboundPayment::AwaitingInvoice { - retry_strategy: retry, max_total_routing_fee_msat: max_total_fee, .. - } => { - retry_strategy = *retry; - max_total_routing_fee_msat = *max_total_fee; - *entry.into_mut() = PendingOutboundPayment::InvoiceReceived { - payment_hash, - retry_strategy: *retry, - max_total_routing_fee_msat, - }; - }, - _ => return Err(Bolt12PaymentError::DuplicateInvoice), - }, - hash_map::Entry::Vacant(_) => return Err(Bolt12PaymentError::UnexpectedInvoice), - } + let (payment_hash, retry_strategy, max_total_routing_fee_msat, _) = self + .mark_invoice_received_and_get_details(invoice, payment_id)?; if invoice.invoice_features().requires_unknown_bits_from(&features) { self.abandon_payment( @@ -1754,6 +1736,51 @@ impl OutboundPayments { } } + pub(super) fn mark_invoice_received( + &self, invoice: &Bolt12Invoice, payment_id: PaymentId + ) -> Result<(), Bolt12PaymentError> { + self.mark_invoice_received_and_get_details(invoice, payment_id) + .and_then(|(_, _, _, is_newly_marked)| { + is_newly_marked + .then_some(()) + .ok_or(Bolt12PaymentError::DuplicateInvoice) + }) + } + + fn mark_invoice_received_and_get_details( + &self, invoice: &Bolt12Invoice, payment_id: PaymentId + ) -> Result<(PaymentHash, Retry, Option, bool), Bolt12PaymentError> { + match self.pending_outbound_payments.lock().unwrap().entry(payment_id) { + hash_map::Entry::Occupied(entry) => match entry.get() { + PendingOutboundPayment::AwaitingInvoice { + retry_strategy: retry, max_total_routing_fee_msat: max_total_fee, .. + } => { + let payment_hash = invoice.payment_hash(); + let retry = *retry; + let max_total_fee = *max_total_fee; + *entry.into_mut() = PendingOutboundPayment::InvoiceReceived { + payment_hash, + retry_strategy: retry, + max_total_routing_fee_msat: max_total_fee, + }; + + Ok((payment_hash, retry, max_total_fee, true)) + }, + // When manual invoice handling is enabled, the corresponding `PendingOutboundPayment` entry + // is already updated at the time the invoice is received. This ensures that `InvoiceReceived` + // event generation remains idempotent, even if the same invoice is received again before the + // event is handled by the user. + PendingOutboundPayment::InvoiceReceived { + retry_strategy, max_total_routing_fee_msat, .. + } => { + Ok((invoice.payment_hash(), *retry_strategy, *max_total_routing_fee_msat, false)) + }, + _ => Err(Bolt12PaymentError::DuplicateInvoice), + }, + hash_map::Entry::Vacant(_) => Err(Bolt12PaymentError::UnexpectedInvoice), + } + } + fn pay_route_internal( &self, route: &Route, payment_hash: PaymentHash, recipient_onion: &RecipientOnionFields, keysend_preimage: Option, invoice_request: Option<&InvoiceRequest>, From a248367e56385a2761608bf842144088eeef3724 Mon Sep 17 00:00:00 2001 From: shaavan Date: Mon, 10 Mar 2025 23:18:45 +0530 Subject: [PATCH 7/8] Introduce idempotency check in tests --- lightning/src/ln/offers_tests.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/offers_tests.rs b/lightning/src/ln/offers_tests.rs index 35a4c61713c..48171d4faeb 100644 --- a/lightning/src/ln/offers_tests.rs +++ b/lightning/src/ln/offers_tests.rs @@ -1185,7 +1185,14 @@ fn pays_bolt12_invoice_asynchronously() { let onion_message = alice.onion_messenger.next_onion_message_for_peer(bob_id).unwrap(); bob.onion_messenger.handle_onion_message(alice_id, &onion_message); - let (invoice, context) = match get_event!(bob, Event::InvoiceReceived) { + // Re-process the same onion message to ensure idempotency — + // we should not generate a duplicate `InvoiceReceived` event. + bob.onion_messenger.handle_onion_message(alice_id, &onion_message); + + let mut events = bob.node.get_and_clear_pending_events(); + assert_eq!(events.len(), 1); + + let (invoice, context) = match events.pop().unwrap() { Event::InvoiceReceived { payment_id: actual_payment_id, invoice, context, .. } => { assert_eq!(actual_payment_id, payment_id); (invoice, context) From 4deb527265eccd5e4a08d39b4989ba303a91429b Mon Sep 17 00:00:00 2001 From: Arik Sosman Date: Tue, 11 Mar 2025 00:00:21 -0700 Subject: [PATCH 8/8] Pin once_cell@1.20.3 for older Rust versions --- ci/ci-tests.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ci/ci-tests.sh b/ci/ci-tests.sh index f4987569fda..1d11a5fd624 100755 --- a/ci/ci-tests.sh +++ b/ci/ci-tests.sh @@ -21,6 +21,9 @@ PIN_RELEASE_DEPS # pin the release dependencies in our main workspace # The addr2line v0.21 crate (a dependency of `backtrace` starting with 0.3.69) relies on rustc 1.65 [ "$RUSTC_MINOR_VERSION" -lt 65 ] && cargo update -p backtrace --precise "0.3.68" --verbose +# The once_cell v1.21.0 crate (a dependency of `proptest`) relies on rustc 1.70 +[ "$RUSTC_MINOR_VERSION" -lt 70 ] && cargo update -p once_cell --precise "1.20.3" --verbose + # proptest 1.3.0 requires rustc 1.64.0 [ "$RUSTC_MINOR_VERSION" -lt 64 ] && cargo update -p proptest --precise "1.2.0" --verbose