Use only the failed amount when retrying payments, not the full amt

TheBlueMatt · TheBlueMatt · commit 4055c7645d2d · 2023-02-01T17:51:37.000Z
When we landed the initial in-`ChannelManager` payment retries, we
stored the `RouteParameters` in the payment info, and then re-use
it as-is for new payments. `RouteParameters` is intended to store
the information specific to the *route*, `PaymentParameters` exists
to store information specific to a payment.

Worse, because we don't recalculate the amount stored in the
`RouteParameters` before fetching a new route with it, we end up
attempting to retry the full payment amount, rather than only the
failed part.

This issue brought to you by having redundant data in
datastructures, part 5,001.
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
@@ -7363,7 +7363,7 @@ where
 									entry.insert(PendingOutboundPayment::Retryable {
 										retry_strategy: None,
 										attempts: PaymentAttempts::new(),
-										route_params: None,
+										payment_params: None,
 										session_privs: [session_priv_bytes].iter().map(|a| *a).collect(),
 										payment_hash: htlc.payment_hash,
 										payment_secret,
diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
@@ -16,6 +16,7 @@ use bitcoin::secp256k1::{self, Secp256k1, SecretKey};
 use crate::chain::keysinterface::{EntropySource, NodeSigner, Recipient};
 use crate::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
 use crate::ln::channelmanager::{ChannelDetails, HTLCSource, IDEMPOTENCY_TIMEOUT_TICKS, MIN_HTLC_RELAY_HOLDING_CELL_MILLIS, PaymentId};
+use crate::ln::channelmanager::MIN_FINAL_CLTV_EXPIRY_DELTA as LDK_DEFAULT_MIN_FINAL_CLTV_EXPIRY_DELTA;
 use crate::ln::msgs::DecodeError;
 use crate::ln::onion_utils::HTLCFailReason;
 use crate::routing::router::{InFlightHtlcs, PaymentParameters, Route, RouteHop, RouteParameters, RoutePath, Router};
@@ -43,7 +44,7 @@ pub(crate) enum PendingOutboundPayment {
 	Retryable {
 		retry_strategy: Option<Retry>,
 		attempts: PaymentAttempts,
-		route_params: Option<RouteParameters>,
+		payment_params: Option<PaymentParameters>,
 		session_privs: HashSet<[u8; 32]>,
 		payment_hash: PaymentHash,
 		payment_secret: Option<PaymentSecret>,
@@ -102,9 +103,17 @@ impl PendingOutboundPayment {
 			_ => false,
 		}
 	}
+	fn payment_parameters(&mut self) -> Option<&mut PaymentParameters> {
+		match self {
+			PendingOutboundPayment::Retryable { payment_params: Some(ref mut params), .. } => {
+				Some(params)
+			},
+			_ => None,
+		}
+	}
 	pub fn insert_previously_failed_scid(&mut self, scid: u64) {
-		if let PendingOutboundPayment::Retryable { route_params: Some(params), .. } = self {
-			params.payment_params.previously_failed_channels.push(scid);
+		if let PendingOutboundPayment::Retryable { payment_params: Some(params), .. } = self {
+			params.previously_failed_channels.push(scid);
 		}
 	}
 	pub(super) fn is_fulfilled(&self) -> bool {
@@ -474,9 +483,18 @@ impl OutboundPayments {
 			let mut retry_id_route_params = None;
 			for (pmt_id, pmt) in outbounds.iter_mut() {
 				if pmt.is_auto_retryable_now() {
-					if let PendingOutboundPayment::Retryable { pending_amt_msat, total_msat, route_params: Some(params), .. } = pmt {
+					if let PendingOutboundPayment::Retryable { pending_amt_msat, total_msat, payment_params: Some(params), .. } = pmt {
 						if pending_amt_msat < total_msat {
-							retry_id_route_params = Some((*pmt_id, params.clone()));
+							retry_id_route_params = Some((*pmt_id, RouteParameters {
+								final_value_msat: *total_msat - *pending_amt_msat,
+								final_cltv_expiry_delta:
+									if let Some(delta) = params.final_cltv_expiry_delta { delta }
+									else {
+										debug_assert!(false, "We always set the final_cltv_expiry_delta when a path fails");
+										LDK_DEFAULT_MIN_FINAL_CLTV_EXPIRY_DELTA.into()
+									},
+								payment_params: params.clone(),
+							}));
 							break
 						}
 					}
@@ -522,7 +540,7 @@ impl OutboundPayments {
 		}))?;
 
 		let res = if let Some((payment_hash, payment_secret, retry_strategy)) = initial_send_info {
-			let onion_session_privs = self.add_new_pending_payment(payment_hash, *payment_secret, payment_id, &route, Some(retry_strategy), Some(route_params.clone()), entropy_source, best_block_height)?;
+			let onion_session_privs = self.add_new_pending_payment(payment_hash, *payment_secret, payment_id, &route, Some(retry_strategy), Some(route_params.payment_params.clone()), entropy_source, best_block_height)?;
 			self.pay_route_internal(&route, payment_hash, payment_secret, None, payment_id, None, onion_session_privs, node_signer, best_block_height, send_payment_along_path)
 		} else {
 			self.retry_payment_with_route(&route, payment_id, entropy_source, node_signer, best_block_height, send_payment_along_path)
@@ -672,7 +690,7 @@ impl OutboundPayments {
 
 	pub(super) fn add_new_pending_payment<ES: Deref>(
 		&self, payment_hash: PaymentHash, payment_secret: Option<PaymentSecret>, payment_id: PaymentId,
-		route: &Route, retry_strategy: Option<Retry>, route_params: Option<RouteParameters>,
+		route: &Route, retry_strategy: Option<Retry>, payment_params: Option<PaymentParameters>,
 		entropy_source: &ES, best_block_height: u32
 	) -> Result<Vec<[u8; 32]>, PaymentSendFailure> where ES::Target: EntropySource {
 		let mut onion_session_privs = Vec::with_capacity(route.paths.len());
@@ -687,7 +705,7 @@ impl OutboundPayments {
 				let payment = entry.insert(PendingOutboundPayment::Retryable {
 					retry_strategy,
 					attempts: PaymentAttempts::new(),
-					route_params,
+					payment_params,
 					session_privs: HashSet::new(),
 					pending_amt_msat: 0,
 					pending_fee_msat: Some(0),
@@ -965,6 +983,7 @@ impl OutboundPayments {
 		let mut all_paths_failed = false;
 		let mut full_failure_ev = None;
 		let mut pending_retry_ev = None;
+		let mut retry = None;
 		let attempts_remaining = if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(*payment_id) {
 			if !payment.get_mut().remove(&session_priv_bytes, Some(&path)) {
 				log_trace!(logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
@@ -978,6 +997,33 @@ impl OutboundPayments {
 			if let Some(scid) = short_channel_id {
 				payment.get_mut().insert_previously_failed_scid(scid);
 			}
+
+			// We want to move towards only using the `PaymentParameters` in the outbound payments
+			// map. However, for backwards-compatibility, we still need to support passing the
+			// `PaymentParameters` data that was shoved in the HTLC (and given to us via
+			// `payment_params`) back to the user.
+			let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
+			if let Some(params) = payment.get_mut().payment_parameters() {
+				if params.final_cltv_expiry_delta.is_none() {
+					// This should be rare, but a user could provide None for the payment data, and
+					// we need it when we go to retry the payment, so fill it in.
+					params.final_cltv_expiry_delta = Some(path_last_hop.cltv_expiry_delta);
+				}
+				retry = Some(RouteParameters {
+					payment_params: params.clone(),
+					final_value_msat: path_last_hop.fee_msat,
+					final_cltv_expiry_delta: params.final_cltv_expiry_delta.unwrap(),
+				});
+			} else if let Some(params) = payment_params {
+				retry = Some(RouteParameters {
+					payment_params: params.clone(),
+					final_value_msat: path_last_hop.fee_msat,
+					final_cltv_expiry_delta:
+						if let Some(delta) = params.final_cltv_expiry_delta { delta }
+						else { path_last_hop.cltv_expiry_delta },
+				});
+			}
+
 			if payment.get().remaining_parts() == 0 {
 				all_paths_failed = true;
 				if payment.get().abandoned() {
@@ -994,16 +1040,6 @@ impl OutboundPayments {
 			return
 		};
 		core::mem::drop(outbounds);
-		let mut retry = if let Some(payment_params_data) = payment_params {
-			let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
-			Some(RouteParameters {
-				payment_params: payment_params_data.clone(),
-				final_value_msat: path_last_hop.fee_msat,
-				final_cltv_expiry_delta:
-					if let Some(delta) = payment_params_data.final_cltv_expiry_delta { delta }
-					else { path_last_hop.cltv_expiry_delta },
-			})
-		} else { None };
 		log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
 
 		let path_failure = {
@@ -1115,13 +1151,13 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
 		(0, session_privs, required),
 		(1, pending_fee_msat, option),
 		(2, payment_hash, required),
-		(not_written, retry_strategy, (static_value, None)),
+		(3, payment_params, option),
 		(4, payment_secret, option),
-		(not_written, attempts, (static_value, PaymentAttempts::new())),
 		(6, total_msat, required),
-		(not_written, route_params, (static_value, None)),
 		(8, pending_amt_msat, required),
 		(10, starting_block_height, required),
+		(not_written, retry_strategy, (static_value, None)),
+		(not_written, attempts, (static_value, PaymentAttempts::new())),
 	},
 	(3, Abandoned) => {
 		(0, session_privs, required),
@@ -1212,7 +1248,7 @@ mod tests {
 
 		let err = if on_retry {
 			outbound_payments.add_new_pending_payment(PaymentHash([0; 32]), None, PaymentId([0; 32]),
-			&Route { paths: vec![], payment_params: None }, Some(Retry::Attempts(1)), Some(route_params.clone()),
+			&Route { paths: vec![], payment_params: None }, Some(Retry::Attempts(1)), Some(route_params.payment_params.clone()),
 			&&keys_manager, 0).unwrap();
 			outbound_payments.pay_internal(
 				PaymentId([0; 32]), None, route_params, &&router, vec![], InFlightHtlcs::new(),
diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs
@@ -2417,3 +2417,161 @@ fn no_extra_retries_on_back_to_back_fail() {
 		_ => panic!("Unexpected event"),
 	}
 }
+
+#[test]
+fn test_simple_partial_retry() {
+	// In the first version of the in-`ChannelManager` payment retries, retries were sent for the
+	// full amount of the payment, rather than only the missing amount. Here we simply test for
+	// this by sending a payment with two parts, failing one, and retrying the second. Note that
+	// `TestRouter` will check that the `RouteParameters` (which contain the amount) matches the
+	// request.
+	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);
+
+	let chan_1_scid = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000_000, 0).0.contents.short_channel_id;
+	let chan_2_scid = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 10_000_000, 0).0.contents.short_channel_id;
+
+	let amt_msat = 200_000_000;
+	let (_, payment_hash, _, payment_secret) = get_route_and_payment_hash!(&nodes[0], nodes[2], amt_msat);
+	#[cfg(feature = "std")]
+	let payment_expiry_secs = SystemTime::UNIX_EPOCH.elapsed().unwrap().as_secs() + 60 * 60;
+	#[cfg(not(feature = "std"))]
+	let payment_expiry_secs = 60 * 60;
+	let mut invoice_features = InvoiceFeatures::empty();
+	invoice_features.set_variable_length_onion_required();
+	invoice_features.set_payment_secret_required();
+	invoice_features.set_basic_mpp_optional();
+	let payment_params = PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV)
+		.with_expiry_time(payment_expiry_secs as u64)
+		.with_features(invoice_features);
+	let route_params = RouteParameters {
+		payment_params,
+		final_value_msat: amt_msat,
+		final_cltv_expiry_delta: TEST_FINAL_CLTV,
+	};
+
+	let mut route = Route {
+		paths: vec![
+			vec![RouteHop {
+				pubkey: nodes[1].node.get_our_node_id(),
+				node_features: nodes[1].node.node_features(),
+				short_channel_id: chan_1_scid,
+				channel_features: nodes[1].node.channel_features(),
+				fee_msat: 0, // nodes[1] will fail the payment as we don't pay its fee
+				cltv_expiry_delta: 100,
+			}, RouteHop {
+				pubkey: nodes[2].node.get_our_node_id(),
+				node_features: nodes[2].node.node_features(),
+				short_channel_id: chan_2_scid,
+				channel_features: nodes[2].node.channel_features(),
+				fee_msat: 100_000_000,
+				cltv_expiry_delta: 100,
+			}],
+			vec![RouteHop {
+				pubkey: nodes[1].node.get_our_node_id(),
+				node_features: nodes[1].node.node_features(),
+				short_channel_id: chan_1_scid,
+				channel_features: nodes[1].node.channel_features(),
+				fee_msat: 100_000,
+				cltv_expiry_delta: 100,
+			}, RouteHop {
+				pubkey: nodes[2].node.get_our_node_id(),
+				node_features: nodes[2].node.node_features(),
+				short_channel_id: chan_2_scid,
+				channel_features: nodes[2].node.channel_features(),
+				fee_msat: 100_000_000,
+				cltv_expiry_delta: 100,
+			}]
+		],
+		payment_params: Some(PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV)),
+	};
+	nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
+	let mut second_payment_params = route_params.payment_params.clone();
+	second_payment_params.previously_failed_channels = vec![chan_2_scid];
+	// On retry, we'll only be asked for one path (or 100k sats)
+	route.paths.remove(0);
+	nodes[0].router.expect_find_route(RouteParameters {
+			payment_params: second_payment_params,
+			final_value_msat: amt_msat / 2, final_cltv_expiry_delta: TEST_FINAL_CLTV,
+		}, Ok(route.clone()));
+
+	nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
+	let htlc_updates = SendEvent::from_node(&nodes[0]);
+	check_added_monitors!(nodes[0], 1);
+	assert_eq!(htlc_updates.msgs.len(), 1);
+
+	nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &htlc_updates.msgs[0]);
+	nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &htlc_updates.commitment_msg);
+	check_added_monitors!(nodes[1], 1);
+	let (bs_first_raa, bs_first_cs) = get_revoke_commit_msgs!(nodes[1], nodes[0].node.get_our_node_id());
+
+	nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_first_raa);
+	check_added_monitors!(nodes[0], 1);
+	let second_htlc_updates = SendEvent::from_node(&nodes[0]);
+
+	nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_first_cs);
+	check_added_monitors!(nodes[0], 1);
+	let as_first_raa = get_event_msg!(nodes[0], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());
+
+	nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &second_htlc_updates.msgs[0]);
+	nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &second_htlc_updates.commitment_msg);
+	check_added_monitors!(nodes[1], 1);
+	let bs_second_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[0].node.get_our_node_id());
+
+	nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_first_raa);
+	check_added_monitors!(nodes[1], 1);
+	let bs_fail_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
+
+	nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_second_raa);
+	check_added_monitors!(nodes[0], 1);
+
+	nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &bs_fail_update.update_fail_htlcs[0]);
+	nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_fail_update.commitment_signed);
+	check_added_monitors!(nodes[0], 1);
+	let (as_second_raa, as_third_cs) = get_revoke_commit_msgs!(nodes[0], nodes[1].node.get_our_node_id());
+
+	nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_second_raa);
+	check_added_monitors!(nodes[1], 1);
+
+	nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_third_cs);
+	check_added_monitors!(nodes[1], 1);
+
+	let bs_third_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[0].node.get_our_node_id());
+
+	nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_third_raa);
+	check_added_monitors!(nodes[0], 1);
+
+	let mut events = nodes[0].node.get_and_clear_pending_events();
+	assert_eq!(events.len(), 2);
+	match events[0] {
+		Event::PaymentPathFailed { payment_hash: ev_payment_hash, payment_failed_permanently, ..  } => {
+			assert_eq!(payment_hash, ev_payment_hash);
+			assert_eq!(payment_failed_permanently, false);
+		},
+		_ => panic!("Unexpected event"),
+	}
+	match events[1] {
+		Event::PendingHTLCsForwardable { .. } => {},
+		_ => panic!("Unexpected event"),
+	}
+
+	nodes[0].node.process_pending_htlc_forwards();
+	let retry_htlc_updates = SendEvent::from_node(&nodes[0]);
+	check_added_monitors!(nodes[0], 1);
+
+	nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &retry_htlc_updates.msgs[0]);
+	commitment_signed_dance!(nodes[1], nodes[0], &retry_htlc_updates.commitment_msg, false, true);
+
+	expect_pending_htlcs_forwardable!(nodes[1]);
+	check_added_monitors!(nodes[1], 1);
+
+	let bs_forward_update = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
+	nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[0]);
+	nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[1]);
+	commitment_signed_dance!(nodes[2], nodes[1], &bs_forward_update.commitment_signed, false);
+
+	expect_pending_htlcs_forwardable!(nodes[2]);
+	expect_payment_claimable!(nodes[2], payment_hash, payment_secret, amt_msat);
+}
