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 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/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/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/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) 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>, 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/offers/invoice_request.rs b/lightning/src/offers/invoice_request.rs index 8d7d25cf2b5..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 { @@ -944,13 +951,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() @@ -1292,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, { @@ -1339,7 +1375,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 +2648,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 +2673,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 +2688,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)); + } + } + } } 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 { diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 079b83563c3..c09a014dc62 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,55 @@ 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(); + + // 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(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 +3157,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 +3172,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 +3180,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. @@ -8465,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.