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.

Author: valentinewallace

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<u64>,

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")]

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<u64>,
+		/// 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<u64>,
 	},
 }
 
@@ -252,6 +256,7 @@ impl PendingOutboundPayment {
 	fn get_pending_fee_msat(&self) -> Option<u64> {
 		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),

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,