From fb344791fe73dd1bc6e828c0464754a98f4998b5 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Mon, 1 Jun 2026 14:38:58 -0400 Subject: [PATCH] Fix missing PaymentSent::fee_paid_msat If an outbound payment was abandoned with htlcs in-flight and later claimed, we would previously have the PaymentSent::fee_paid_msat be set to None. This contradicted some docs on the event that stated the field would always be Some after 0.0.103. --- lightning/src/events/mod.rs | 2 +- lightning/src/ln/functional_tests.rs | 21 ++++++++++++++++++- lightning/src/ln/outbound_payment.rs | 16 +++++++++++++++ lightning/src/ln/payment_tests.rs | 30 ++++++++++++++++++++++++++++ 4 files changed, 67 insertions(+), 2 deletions(-) diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs index 271e135d51d..deb9391284d 100644 --- a/lightning/src/events/mod.rs +++ b/lightning/src/events/mod.rs @@ -1201,7 +1201,7 @@ pub enum Event { /// If the recipient or an intermediate node misbehaves and gives us free money, this may /// overstate the amount paid, though this is unlikely. /// - /// This is only `None` for payments initiated on LDK versions prior to 0.0.103. + /// May be `None` for payments initiated on LDK versions prior to 0.4. /// /// [`Route::get_total_fees`]: crate::routing::router::Route::get_total_fees fee_paid_msat: Option, diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 37dd5187700..3eacfeb0e18 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -8437,6 +8437,17 @@ pub fn test_dup_htlc_second_rejected() { #[xtest(feature = "_externalize_tests")] pub fn test_inconsistent_mpp_params() { + do_test_inconsistent_mpp_params(false); +} + +#[xtest(feature = "_externalize_tests")] +pub fn test_inconsistent_mpp_params_abandoned_then_retried() { + // Same as test_inconsistent_mpp_params, but abandon the payment before it is claimed to check + // that we'll still provide the correct fee in the `PaymentSent` in that case. + do_test_inconsistent_mpp_params(true); +} + +fn do_test_inconsistent_mpp_params(abandon_early: bool) { // Test that if we recieve two HTLCs with different payment parameters we fail back the first // such HTLC and allow the second to stay. let chanmon_cfgs = create_chanmon_cfgs(4); @@ -8512,6 +8523,14 @@ pub fn test_inconsistent_mpp_params() { .unwrap(); check_added_monitors(&nodes[0], 1); + if abandon_early { + // Abandon before the mismatched MPP part's rejection makes it back to us. HTLCs are still + // in-flight at this point, so when the rejection arrives, we'll need to do fee accounting with + // the payment in the `Abandoned` rather than `Retryable` state. + nodes[0].node.abandon_payment(id); + assert!(nodes[0].node.get_and_clear_pending_events().is_empty()); + } + { let mut events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 1); @@ -8579,7 +8598,7 @@ pub fn test_inconsistent_mpp_params() { pass_along_path(&nodes[0], path_b, real_amt, hash, Some(payment_secret), event, true, None); do_claim_payment_along_route(ClaimAlongRouteArgs::new(&nodes[0], &[path_a, path_b], preimage)); - expect_payment_sent(&nodes[0], preimage, Some(None), true, true); + expect_payment_sent(&nodes[0], preimage, Some(Some(2_000)), true, true); } #[xtest(feature = "_externalize_tests")] diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 273ed4ec1d2..df736e5d704 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -164,6 +164,10 @@ pub(crate) enum PendingOutboundPayment { /// The total payment amount across all paths, used to be able to issue `PaymentSent` if /// an HTLC still happens to succeed after we marked the payment as abandoned. total_msat: Option, + /// Preserved from `Retryable` so we can still report `fee_paid_msat` if an HTLC succeeds + /// after the payment was abandoned. `None` for payments abandoned prior to this field being + /// added, or for payments that were never in the `Retryable` state. + pending_fee_msat: Option, }, } @@ -252,6 +256,7 @@ impl PendingOutboundPayment { fn get_pending_fee_msat(&self) -> Option { match self { PendingOutboundPayment::Retryable { pending_fee_msat, .. } => pending_fee_msat.clone(), + PendingOutboundPayment::Abandoned { pending_fee_msat, .. } => pending_fee_msat.clone(), _ => None, } } @@ -308,6 +313,7 @@ impl PendingOutboundPayment { _ => new_hash_set(), }; let total_msat = self.total_msat(); + let pending_fee_msat = self.get_pending_fee_msat(); match self { Self::Retryable { payment_hash, .. } | Self::InvoiceReceived { payment_hash, .. } | @@ -318,6 +324,7 @@ impl PendingOutboundPayment { payment_hash: *payment_hash, reason: Some(reason), total_msat, + pending_fee_msat, }; }, _ => {} @@ -354,6 +361,14 @@ impl PendingOutboundPayment { if let Some(max_total_routing_fee_msat) = remaining_max_total_routing_fee_msat.as_mut() { *max_total_routing_fee_msat = max_total_routing_fee_msat.saturating_add(path_fee_msat); } + } else if let PendingOutboundPayment::Abandoned { pending_fee_msat, .. } = self { + // Keep `pending_fee_msat` in sync with the remaining in-flight HTLCs so that if + // any path still succeeds after abandonment, the eventual `PaymentSent` reports + // only the fee actually paid. + let path = path.expect("Removing a failed payment should always come with a path"); + if let Some(fee_msat) = pending_fee_msat.as_mut() { + *fee_msat -= path.fee_msat(); + } } } remove_res @@ -2778,6 +2793,7 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment, (1, reason, upgradable_option), (2, payment_hash, required), (3, total_msat, option), + (5, pending_fee_msat, option), }, (5, AwaitingInvoice) => { (0, expiration, required), diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 33c7df93ddb..90656b34429 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -2241,6 +2241,36 @@ fn abandoned_send_payment_idempotent() { claim_payment(&nodes[0], &[&nodes[1]], second_payment_preimage); } +#[test] +fn abandoned_payment_fulfilled_preserves_fee_paid_msat() { + // Previously, if we abandoned a payment with HTLCs in-flight and the payment eventually + // succeeded, we would set the `Event::PaymentSent::fee_paid_msat` to None, even though we had + // docs guaranteeing that it would always be Some after 0.0.103. + 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, &[None, None, None]); + let nodes = create_network(3, &node_cfgs, &node_chanmgrs); + + create_announced_chan_between_nodes(&nodes, 0, 1); + create_announced_chan_between_nodes(&nodes, 1, 2); + + let amt_msat = 10_000_000; + let (route, payment_hash, payment_preimage, payment_secret) = + get_route_and_payment_hash!(&nodes[0], nodes[2], amt_msat); + let payment_id = PaymentId(payment_hash.0); + let onion = RecipientOnionFields::secret_only(payment_secret, amt_msat); + nodes[0].node.send_payment_with_route(route, payment_hash, onion, payment_id).unwrap(); + check_added_monitors(&nodes[0], 1); + + let path: &[&Node] = &[&nodes[1], &nodes[2]]; + pass_along_route(&nodes[0], &[path], amt_msat, payment_hash, payment_secret); + + nodes[0].node.abandon_payment(payment_id); + assert!(nodes[0].node.get_and_clear_pending_events().is_empty()); + + claim_payment_along_route(ClaimAlongRouteArgs::new(&nodes[0], &[path], payment_preimage)); +} + #[derive(PartialEq)] enum InterceptTest { Forward,