Address PR comments

Author: jurvis

lightning-invoice/src/payment.rs

@@ -220,7 +220,7 @@ impl<T: Time> PaymentInfo<T> {
 	fn new() -> Self {
 		PaymentInfo {
 			attempts: PaymentAttempts::new(),
-			paths: vec![vec![]],
+			paths: vec![],
 		}
 	}
 }
@@ -234,11 +234,10 @@ struct AccountForInFlightHtlcs<'a, S: Score> {
 
 impl<'a, S: Score> Score for AccountForInFlightHtlcs<'a, S> {
 	fn channel_penalty_msat(&self, short_channel_id: u64, source: &NodeId, target: &NodeId, usage: ChannelUsage) -> u64 {
-		if let Some(used_liqudity) = self.inflight_htlcs.get(&(short_channel_id, target < source)) {
+		if let Some(used_liqudity) = self.inflight_htlcs.get(&(short_channel_id, source > target)) {
 			let usage = ChannelUsage {
-				amount_msat: usage.amount_msat,
 				inflight_htlc_msat: usage.inflight_htlc_msat + used_liqudity,
-				effective_capacity: usage.effective_capacity,
+				..usage
 			};
 
 			self.scorer.channel_penalty_msat(short_channel_id, source, target, usage)
@@ -503,22 +502,22 @@ where
 			if let Some(payment_info) = self.payment_cache.lock().unwrap().get(&payment_hash) {
 				let mut total_inflight_map: HashMap<(u64, bool), u64> = HashMap::new();
 
-				for hops in &payment_info.paths {
-					if hops.len() < 1 { break };
+				for path in &payment_info.paths {
+					if path.is_empty() { break };
 					// total_inflight_map needs to be direction-sensitive when keeping track of the HTLC value
 					// that is held up. However, the `hops` array, which is a path returned by `find_route` in
 					// the router excludes the payer node. In the following lines, the payer's information is
 					// hardcoded with an inflight value of 0 so that we can correctly represent the first hop
 					// in our sliding window of two.
 					let our_node_id: PublicKey = self.payer.node_id();
 					let reversed_hops_with_payer = core::iter::once((0u64, our_node_id)).chain(
-						hops.split_last().unwrap().1.iter().map(|hop| (hop.fee_msat, hop.pubkey))).rev();
-					let mut cumulative_msat = hops.last().unwrap().fee_msat;
+						path.split_last().unwrap().1.iter().map(|hop| (hop.fee_msat, hop.pubkey))).rev();
+					let mut cumulative_msat = path.last().unwrap().fee_msat;
 
 					// Taking the reversed vector from above, we zip it with just the reversed hops list to
 					// work "backwards" of the given path, since the last hop's `fee_msat` actually represents
 					// the total amount sent.
-					for (next_hop, prev_hop) in hops.iter().rev().zip(reversed_hops_with_payer) {
+					for (next_hop, prev_hop) in path.iter().rev().zip(reversed_hops_with_payer) {
 						cumulative_msat += prev_hop.0;
 						total_inflight_map
 							.entry((next_hop.short_channel_id, next_hop.pubkey < prev_hop.1))
@@ -533,19 +532,16 @@ where
 			}
 		};
 
-		let route = {
-			let scorer = AccountForInFlightHtlcs { scorer: &mut self.scorer.lock(), inflight_htlcs };
-
-			self.router.find_route(
+		let route = self.router.find_route(
 				&payer, &params, &payment_hash, Some(&first_hops.iter().collect::<Vec<_>>()),
-				&scorer
-			).map_err(|e| PaymentError::Routing(e))?
-		};
+				&AccountForInFlightHtlcs { scorer: &mut self.scorer.lock(), inflight_htlcs }
+			).map_err(|e| PaymentError::Routing(e))?;
+
 
 		match send_payment(&route) {
 			Ok(payment_id) => {
 				for hops in route.paths {
-					self.process_path_inflight_htlcs(payment_hash, hops);
+					self.process_path_inflight_htlcs(payment_hash, &hops);
 				}
 				Ok(payment_id)
 			},
@@ -564,20 +560,21 @@ where
 					}
 				},
 				PaymentSendFailure::PartialFailure { failed_paths_retry, payment_id, results } => {
-					// The ones that are `Ok()`s + `MonitorUpdateFailed` in `results` need to be used to update the map
-					// get the index of them and run to `route` to get the values we need to care about.
+					// If a `PartialFailure` event returns a result that is either `Ok()` or an error
+					// with `MonitorUpdateFailed`, it means that the HTLCs for our payment is now
+					// either inflight, or being retried.
+					//
+					// Since the `results` field returned within `PartialFailure` are in the same
+					// order as our route hops, we can use the index for each `result` to look up its
+					// path in the `route` we found.
 					for (idx, result) in results.iter().enumerate() {
 						match result {
 							Ok(_) => {
-								if let Some(failed_path) = route.paths.get(idx) {
-									self.process_path_inflight_htlcs(payment_hash, failed_path.to_vec());
-								}
+								self.process_path_inflight_htlcs(payment_hash, &route.paths[idx]);
 							}
 							Err(err) => match err {
 								APIError::MonitorUpdateFailed => {
-									if let Some(failed_path) = route.paths.get(idx) {
-										self.process_path_inflight_htlcs(payment_hash, failed_path.to_vec());
-									}
+									self.process_path_inflight_htlcs(payment_hash, &route.paths[idx])
 								}
 								_ => {}
 							}
@@ -605,23 +602,21 @@ where
 
 	// Takes in a path to have its information stored in `payment_cache`. This is done for paths
 	// that are pending retry.
-	fn process_path_inflight_htlcs(&self, payment_hash: PaymentHash, hops: Vec<RouteHop>) {
-		let payment_info_route_hops: Vec<PaymentInfoRouteHop> = hops.iter().map(|h| PaymentInfoRouteHop { pubkey: h.pubkey, short_channel_id: h.short_channel_id, fee_msat: h.fee_msat }).collect();
+	fn process_path_inflight_htlcs(&self, payment_hash: PaymentHash, hops: &Vec<RouteHop>) {
+		let payment_info_route_hops: Vec<PaymentInfoRouteHop> = hops.iter()
+			.map(|h| PaymentInfoRouteHop { pubkey: h.pubkey, short_channel_id: h.short_channel_id, fee_msat: h.fee_msat })
+			.collect();
 
 		self.payment_cache.lock().unwrap().entry(payment_hash)
-			.or_insert(PaymentInfo {
-				attempts: PaymentAttempts {
-					count: 1,
-					first_attempted_at: T::now()
-				},
-				paths: vec![]
-			})
+			.or_insert(PaymentInfo::new())
 			.paths.push(payment_info_route_hops);
 	}
 
 	// Find the path we want to remove in `payment_cache`. If it doesn't exist, do nothing.
 	fn remove_path_inflight_htlcs(&self, payment_hash: PaymentHash, hops: &Vec<RouteHop>) {
-		let payment_info_route_hops: Vec<PaymentInfoRouteHop> = hops.iter().map(|h| PaymentInfoRouteHop { pubkey: h.pubkey, short_channel_id: h.short_channel_id, fee_msat: h.fee_msat }).collect();
+		let payment_info_route_hops: Vec<PaymentInfoRouteHop> = hops.iter()
+			.map(|h| PaymentInfoRouteHop { pubkey: h.pubkey, short_channel_id: h.short_channel_id, fee_msat: h.fee_msat })
+			.collect();
 
 		self.payment_cache.lock().unwrap().entry(payment_hash)
 			.and_modify(|payment_info| {
@@ -636,13 +631,7 @@ where
 	) -> Result<(), ()> {
 		let attempts = self.payment_cache.lock().unwrap().entry(payment_hash)
 			.and_modify(|info| info.attempts.count += 1 )
-			.or_insert(PaymentInfo {
-				attempts: PaymentAttempts {
-					count: 1,
-					first_attempted_at: T::now()
-				},
-				paths: vec![vec![]]
-			}).attempts;
+			.or_insert({ PaymentInfo::new() }).attempts;
 
 		if !self.retry.is_retryable_now(&attempts) {
 			log_trace!(self.logger, "Payment {} exceeded maximum attempts; not retrying ({})", log_bytes!(payment_hash.0), attempts);