Add utilities for getting a path's final value and cltv delta

valentinewallace · valentinewallace · commit 7b457e057610 · 2023-04-19T13:13:46.000-04:00
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
@@ -6895,7 +6895,7 @@ impl Readable for HTLCSource {
 				let path = path.unwrap();
 				if let Some(params) = payment_params.as_mut() {
 					if params.final_cltv_expiry_delta == 0 {
-						params.final_cltv_expiry_delta = path.last().unwrap().cltv_expiry_delta;
+						params.final_cltv_expiry_delta = path.final_cltv_expiry_delta();
 					}
 				}
 				Ok(HTLCSource::OutboundRoute {
@@ -7590,7 +7590,7 @@ where
 								return Err(DecodeError::InvalidValue);
 							}
 
-							let path_amt = path.last().unwrap().fee_msat;
+							let path_amt = path.final_value_msat();
 							let mut session_priv_bytes = [0; 32];
 							session_priv_bytes[..].copy_from_slice(&session_priv[..]);
 							match pending_outbounds.pending_outbound_payments.lock().unwrap().entry(payment_id) {
@@ -7600,7 +7600,7 @@ where
 										if newly_added { "Added" } else { "Had" }, path_amt, log_bytes!(session_priv_bytes), log_bytes!(htlc.payment_hash.0));
 								},
 								hash_map::Entry::Vacant(entry) => {
-									let path_fee = path.get_path_fees();
+									let path_fee = path.fee_msat();
 									entry.insert(PendingOutboundPayment::Retryable {
 										retry_strategy: None,
 										attempts: PaymentAttempts::new(),
diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
@@ -171,10 +171,9 @@ impl PendingOutboundPayment {
 		if remove_res {
 			if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, ref mut pending_fee_msat, .. } = self {
 				let path = path.expect("Fulfilling a payment should always come with a path");
-				let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
-				*pending_amt_msat -= path_last_hop.fee_msat;
+				*pending_amt_msat -= path.final_value_msat();
 				if let Some(fee_msat) = pending_fee_msat.as_mut() {
-					*fee_msat -= path.get_path_fees();
+					*fee_msat -= path.fee_msat();
 				}
 			}
 		}
@@ -192,10 +191,9 @@ impl PendingOutboundPayment {
 		};
 		if insert_res {
 			if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, ref mut pending_fee_msat, .. } = self {
-				let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
-				*pending_amt_msat += path_last_hop.fee_msat;
+				*pending_amt_msat += path.final_value_msat();
 				if let Some(fee_msat) = pending_fee_msat.as_mut() {
-					*fee_msat += path.get_path_fees();
+					*fee_msat += path.fee_msat();
 				}
 			}
 		}
@@ -723,7 +721,7 @@ impl OutboundPayments {
 						PendingOutboundPayment::Retryable {
 							total_msat, keysend_preimage, payment_secret, pending_amt_msat, ..
 						} => {
-							let retry_amt_msat: u64 = route.paths.iter().map(|path| path.last().unwrap().fee_msat).sum();
+							let retry_amt_msat = route.get_total_amount();
 							if retry_amt_msat + *pending_amt_msat > *total_msat * (100 + RETRY_OVERFLOW_PERCENTAGE) / 100 {
 								log_error!(logger, "retry_amt_msat of {} will put pending_amt_msat (currently: {}) more than 10% over total_payment_amt_msat of {}", retry_amt_msat, pending_amt_msat, total_msat);
 								abandon_with_entry!(payment, PaymentFailureReason::UnexpectedError);
@@ -961,7 +959,7 @@ impl OutboundPayments {
 					continue 'path_check;
 				}
 			}
-			total_value += path.last().unwrap().fee_msat;
+			total_value += path.final_value_msat();
 			path_errs.push(Ok(()));
 		}
 		if path_errs.iter().any(|e| e.is_err()) {
@@ -1009,7 +1007,7 @@ impl OutboundPayments {
 				has_err = true;
 				has_ok = true;
 			} else if res.is_err() {
-				pending_amt_unsent += path.last().unwrap().fee_msat;
+				pending_amt_unsent += path.final_value_msat();
 			}
 		}
 		if has_err && has_ok {
diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
@@ -261,17 +261,43 @@ pub struct Route {
 	pub payment_params: Option<PaymentParameters>,
 }
 
+// This trait is deleted in the next commit
 pub(crate) trait RoutePath {
 	/// Gets the fees for a given path, excluding any excess paid to the recipient.
-	fn get_path_fees(&self) -> u64;
+	fn fee_msat(&self) -> u64;
+
+	/// Gets the total amount paid on this path, excluding the fees.
+	fn final_value_msat(&self) -> u64;
+
+	/// Gets the final hop's CLTV expiry delta.
+	fn final_cltv_expiry_delta(&self) -> u32;
 }
 impl RoutePath for Vec<RouteHop> {
-	fn get_path_fees(&self) -> u64 {
+	fn fee_msat(&self) -> u64 {
 		// Do not count last hop of each path since that's the full value of the payment
 		self.split_last().map(|(_, path_prefix)| path_prefix).unwrap_or(&[])
 			.iter().map(|hop| &hop.fee_msat)
 			.sum()
 	}
+	fn final_value_msat(&self) -> u64 {
+		self.last().map_or(0, |hop| hop.fee_msat)
+	}
+	fn final_cltv_expiry_delta(&self) -> u32 {
+		self.last().map_or(0, |hop| hop.cltv_expiry_delta)
+	}
+}
+impl RoutePath for &[&RouteHop] {
+	fn fee_msat(&self) -> u64 {
+		self.split_last().map(|(_, path_prefix)| path_prefix).unwrap_or(&[])
+			.iter().map(|hop| &hop.fee_msat)
+			.sum()
+	}
+	fn final_value_msat(&self) -> u64 {
+		self.last().map_or(0, |hop| hop.fee_msat)
+	}
+	fn final_cltv_expiry_delta(&self) -> u32 {
+		self.last().map_or(0, |hop| hop.cltv_expiry_delta)
+	}
 }
 
 impl Route {
@@ -280,15 +306,13 @@ impl Route {
 	/// This doesn't include any extra payment made to the recipient, which can happen in excess of
 	/// the amount passed to [`find_route`]'s `params.final_value_msat`.
 	pub fn get_total_fees(&self) -> u64 {
-		self.paths.iter().map(|path| path.get_path_fees()).sum()
+		self.paths.iter().map(|path| path.fee_msat()).sum()
 	}
 
 	/// Returns the total amount paid on this [`Route`], excluding the fees. Might be more than
 	/// requested if we had to reach htlc_minimum_msat.
 	pub fn get_total_amount(&self) -> u64 {
-		return self.paths.iter()
-			.map(|path| path.split_last().map(|(hop, _)| hop.fee_msat).unwrap_or(0))
-			.sum();
+		return self.paths.iter().map(|path|path.final_value_msat()).sum()
 	}
 }
 
@@ -2183,7 +2207,7 @@ mod tests {
 	use crate::routing::gossip::{NetworkGraph, P2PGossipSync, NodeId, EffectiveCapacity};
 	use crate::routing::utxo::UtxoResult;
 	use crate::routing::router::{get_route, build_route_from_hops_internal, add_random_cltv_offset, default_node_features,
-		PaymentParameters, Route, RouteHint, RouteHintHop, RouteHop, RoutingFees,
+		PaymentParameters, Route, RouteHint, RouteHintHop, RouteHop, RoutingFees, RoutePath,
 		DEFAULT_MAX_TOTAL_CLTV_EXPIRY_DELTA, MAX_PATH_LENGTH_ESTIMATE};
 	use crate::routing::scoring::{ChannelUsage, FixedPenaltyScorer, Score, ProbabilisticScorer, ProbabilisticScoringParameters};
 	use crate::routing::test_utils::{add_channel, add_or_update_node, build_graph, build_line_graph, id_to_feature_flags, get_nodes, update_channel};
@@ -3487,7 +3511,7 @@ mod tests {
 			let path = route.paths.last().unwrap();
 			assert_eq!(path.len(), 2);
 			assert_eq!(path.last().unwrap().pubkey, nodes[2]);
-			assert_eq!(path.last().unwrap().fee_msat, 250_000_000);
+			assert_eq!(path.final_value_msat(), 250_000_000);
 		}
 
 		// Check that setting next_outbound_htlc_limit_msat in first_hops limits the channels.
@@ -3523,7 +3547,7 @@ mod tests {
 			let path = route.paths.last().unwrap();
 			assert_eq!(path.len(), 2);
 			assert_eq!(path.last().unwrap().pubkey, nodes[2]);
-			assert_eq!(path.last().unwrap().fee_msat, 200_000_000);
+			assert_eq!(path.final_value_msat(), 200_000_000);
 		}
 
 		// Enable channel #1 back.
@@ -3570,7 +3594,7 @@ mod tests {
 			let path = route.paths.last().unwrap();
 			assert_eq!(path.len(), 2);
 			assert_eq!(path.last().unwrap().pubkey, nodes[2]);
-			assert_eq!(path.last().unwrap().fee_msat, 15_000);
+			assert_eq!(path.final_value_msat(), 15_000);
 		}
 
 		// Now let's see if routing works if we know only capacity from the UTXO.
@@ -3641,7 +3665,7 @@ mod tests {
 			let path = route.paths.last().unwrap();
 			assert_eq!(path.len(), 2);
 			assert_eq!(path.last().unwrap().pubkey, nodes[2]);
-			assert_eq!(path.last().unwrap().fee_msat, 15_000);
+			assert_eq!(path.final_value_msat(), 15_000);
 		}
 
 		// Now let's see if routing chooses htlc_maximum_msat over UTXO capacity.
@@ -3673,7 +3697,7 @@ mod tests {
 			let path = route.paths.last().unwrap();
 			assert_eq!(path.len(), 2);
 			assert_eq!(path.last().unwrap().pubkey, nodes[2]);
-			assert_eq!(path.last().unwrap().fee_msat, 10_000);
+			assert_eq!(path.final_value_msat(), 10_000);
 		}
 	}
 
@@ -3786,7 +3810,7 @@ mod tests {
 			for path in &route.paths {
 				assert_eq!(path.len(), 4);
 				assert_eq!(path.last().unwrap().pubkey, nodes[3]);
-				total_amount_paid_msat += path.last().unwrap().fee_msat;
+				total_amount_paid_msat += path.final_value_msat();
 			}
 			assert_eq!(total_amount_paid_msat, 49_000);
 		}
@@ -3799,7 +3823,7 @@ mod tests {
 			for path in &route.paths {
 				assert_eq!(path.len(), 4);
 				assert_eq!(path.last().unwrap().pubkey, nodes[3]);
-				total_amount_paid_msat += path.last().unwrap().fee_msat;
+				total_amount_paid_msat += path.final_value_msat();
 			}
 			assert_eq!(total_amount_paid_msat, 50_000);
 		}
@@ -3847,7 +3871,7 @@ mod tests {
 			for path in &route.paths {
 				assert_eq!(path.len(), 2);
 				assert_eq!(path.last().unwrap().pubkey, nodes[2]);
-				total_amount_paid_msat += path.last().unwrap().fee_msat;
+				total_amount_paid_msat += path.final_value_msat();
 			}
 			assert_eq!(total_amount_paid_msat, 50_000);
 		}
@@ -3993,7 +4017,7 @@ mod tests {
 			for path in &route.paths {
 				assert_eq!(path.len(), 2);
 				assert_eq!(path.last().unwrap().pubkey, nodes[2]);
-				total_amount_paid_msat += path.last().unwrap().fee_msat;
+				total_amount_paid_msat += path.final_value_msat();
 			}
 			assert_eq!(total_amount_paid_msat, 250_000);
 		}
@@ -4007,7 +4031,7 @@ mod tests {
 			for path in &route.paths {
 				assert_eq!(path.len(), 2);
 				assert_eq!(path.last().unwrap().pubkey, nodes[2]);
-				total_amount_paid_msat += path.last().unwrap().fee_msat;
+				total_amount_paid_msat += path.final_value_msat();
 			}
 			assert_eq!(total_amount_paid_msat, 290_000);
 		}
@@ -4171,7 +4195,7 @@ mod tests {
 			let mut total_amount_paid_msat = 0;
 			for path in &route.paths {
 				assert_eq!(path.last().unwrap().pubkey, nodes[3]);
-				total_amount_paid_msat += path.last().unwrap().fee_msat;
+				total_amount_paid_msat += path.final_value_msat();
 			}
 			assert_eq!(total_amount_paid_msat, 300_000);
 		}
@@ -4333,7 +4357,7 @@ mod tests {
 			let mut total_paid_msat = 0;
 			for path in &route.paths {
 				assert_eq!(path.last().unwrap().pubkey, nodes[3]);
-				total_value_transferred_msat += path.last().unwrap().fee_msat;
+				total_value_transferred_msat += path.final_value_msat();
 				for hop in path {
 					total_paid_msat += hop.fee_msat;
 				}
@@ -4510,7 +4534,7 @@ mod tests {
 			let mut total_amount_paid_msat = 0;
 			for path in &route.paths {
 				assert_eq!(path.last().unwrap().pubkey, nodes[3]);
-				total_amount_paid_msat += path.last().unwrap().fee_msat;
+				total_amount_paid_msat += path.final_value_msat();
 			}
 			assert_eq!(total_amount_paid_msat, 200_000);
 			assert_eq!(route.get_total_fees(), 150_000);
@@ -4737,7 +4761,7 @@ mod tests {
 			for path in &route.paths {
 				assert_eq!(path.len(), 2);
 				assert_eq!(path.last().unwrap().pubkey, nodes[2]);
-				total_amount_paid_msat += path.last().unwrap().fee_msat;
+				total_amount_paid_msat += path.final_value_msat();
 			}
 			assert_eq!(total_amount_paid_msat, 125_000);
 		}
@@ -4750,7 +4774,7 @@ mod tests {
 			for path in &route.paths {
 				assert_eq!(path.len(), 2);
 				assert_eq!(path.last().unwrap().pubkey, nodes[2]);
-				total_amount_paid_msat += path.last().unwrap().fee_msat;
+				total_amount_paid_msat += path.final_value_msat();
 			}
 			assert_eq!(total_amount_paid_msat, 90_000);
 		}
diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs
@@ -56,7 +56,7 @@
 
 use crate::ln::msgs::DecodeError;
 use crate::routing::gossip::{EffectiveCapacity, NetworkGraph, NodeId};
-use crate::routing::router::RouteHop;
+use crate::routing::router::{RouteHop, RoutePath};
 use crate::util::ser::{Readable, ReadableArgs, Writeable, Writer};
 use crate::util::logger::Logger;
 use crate::util::time::Time;
@@ -1234,7 +1234,7 @@ impl<G: Deref<Target = NetworkGraph<L>>, L: Deref, T: Time> Score for Probabilis
 	}
 
 	fn payment_path_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
-		let amount_msat = path.split_last().map(|(hop, _)| hop.fee_msat).unwrap_or(0);
+		let amount_msat = path.final_value_msat();
 		log_trace!(self.logger, "Scoring path through to SCID {} as having failed at {} msat", short_channel_id, amount_msat);
 		let network_graph = self.network_graph.read_only();
 		for (hop_idx, hop) in path.iter().enumerate() {
@@ -1273,7 +1273,7 @@ impl<G: Deref<Target = NetworkGraph<L>>, L: Deref, T: Time> Score for Probabilis
 	}
 
 	fn payment_path_successful(&mut self, path: &[&RouteHop]) {
-		let amount_msat = path.split_last().map(|(hop, _)| hop.fee_msat).unwrap_or(0);
+		let amount_msat = path.final_value_msat();
 		log_trace!(self.logger, "Scoring path through SCID {} as having succeeded at {} msat.",
 			path.split_last().map(|(hop, _)| hop.short_channel_id).unwrap_or(0), amount_msat);
 		let network_graph = self.network_graph.read_only();

Original file line number	Diff line number	Diff line change
`@@ -171,10 +171,9 @@ impl PendingOutboundPayment {`
`171`	`171`	`if remove_res {`
`172`	`172`	`if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, ref mut pending_fee_msat, .. } = self {`
`173`	`173`	`let path = path.expect("Fulfilling a payment should always come with a path");`
`174`		`- let path_last_hop = path.last().expect("Outbound payments must have had a valid path");`
`175`		`- *pending_amt_msat -= path_last_hop.fee_msat;`
	`174`	`+ *pending_amt_msat -= path.final_value_msat();`
`176`	`175`	`if let Some(fee_msat) = pending_fee_msat.as_mut() {`
`177`		`- *fee_msat -= path.get_path_fees();`
	`176`	`+ *fee_msat -= path.fee_msat();`
`178`	`177`	`}`
`179`	`178`	`}`
`180`	`179`	`}`
`@@ -192,10 +191,9 @@ impl PendingOutboundPayment {`
`192`	`191`	`};`
`193`	`192`	`if insert_res {`
`194`	`193`	`if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, ref mut pending_fee_msat, .. } = self {`
`195`		`- let path_last_hop = path.last().expect("Outbound payments must have had a valid path");`
`196`		`- *pending_amt_msat += path_last_hop.fee_msat;`
	`194`	`+ *pending_amt_msat += path.final_value_msat();`
`197`	`195`	`if let Some(fee_msat) = pending_fee_msat.as_mut() {`
`198`		`- *fee_msat += path.get_path_fees();`
	`196`	`+ *fee_msat += path.fee_msat();`
`199`	`197`	`}`
`200`	`198`	`}`
`201`	`199`	`}`
`@@ -723,7 +721,7 @@ impl OutboundPayments {`
`723`	`721`	`PendingOutboundPayment::Retryable {`
`724`	`722`	`total_msat, keysend_preimage, payment_secret, pending_amt_msat, ..`
`725`	`723`	`} => {`
`726`		`- let retry_amt_msat: u64 = route.paths.iter().map(\|path\| path.last().unwrap().fee_msat).sum();`
	`724`	`+ let retry_amt_msat = route.get_total_amount();`
`727`	`725`	`if retry_amt_msat + pending_amt_msat > total_msat * (100 + RETRY_OVERFLOW_PERCENTAGE) / 100 {`
`728`	`726`	`log_error!(logger, "retry_amt_msat of {} will put pending_amt_msat (currently: {}) more than 10% over total_payment_amt_msat of {}", retry_amt_msat, pending_amt_msat, total_msat);`
`729`	`727`	`abandon_with_entry!(payment, PaymentFailureReason::UnexpectedError);`
`@@ -961,7 +959,7 @@ impl OutboundPayments {`
`961`	`959`	`continue 'path_check;`
`962`	`960`	`}`
`963`	`961`	`}`
`964`		`- total_value += path.last().unwrap().fee_msat;`
	`962`	`+ total_value += path.final_value_msat();`
`965`	`963`	`path_errs.push(Ok(()));`
`966`	`964`	`}`
`967`	`965`	`if path_errs.iter().any(\|e\| e.is_err()) {`
`@@ -1009,7 +1007,7 @@ impl OutboundPayments {`
`1009`	`1007`	`has_err = true;`
`1010`	`1008`	`has_ok = true;`
`1011`	`1009`	`} else if res.is_err() {`
`1012`		`- pending_amt_unsent += path.last().unwrap().fee_msat;`
	`1010`	`+ pending_amt_unsent += path.final_value_msat();`
`1013`	`1011`	`}`
`1014`	`1012`	`}`
`1015`	`1013`	`if has_err && has_ok {`