Refactor InvoicePayer for spontaneous payments

To support spontaneous payments, InvoicePayer's sending logic must be
invoice-agnostic. Refactor InvoicePayer::pay_invoice_internal such that
invoice-specific code is in pay_invoice_using_amount and the remaining
logic is in pay_internal.

Further refactors the code's payment_cache locking such that a single
lock is held during the course of sending and retry. This leads to less
indented, tidier code where payment_cache clean-up is less dispersed.

Author: jkczyz

lightning-invoice/src/payment.rs

@@ -193,6 +193,20 @@ pub enum PaymentError {
 	Sending(PaymentSendFailure),
 }
 
+type PaymentEntry<'a> = hash_map::OccupiedEntry<'a, PaymentHash, usize>;
+
+macro_rules! look_up_or_insert {
+	($payment_cache: ident, $payment_hash: expr) => {
+		loop {
+			let entry = $payment_cache.entry($payment_hash);
+			match entry {
+				hash_map::Entry::Occupied(entry) => break entry,
+				hash_map::Entry::Vacant(entry) => entry.insert(0),
+			};
+		};
+	}
+}
+
 impl<P: Deref, R, S: Deref, L: Deref, E> InvoicePayer<P, R, S, L, E>
 where
 	P::Target: Payer,
@@ -228,7 +242,7 @@ where
 		if invoice.amount_milli_satoshis().is_none() {
 			Err(PaymentError::Invoice("amount missing"))
 		} else {
-			self.pay_invoice_internal(invoice, None, 0)
+			self.pay_invoice_using_amount(invoice, None)
 		}
 	}
 
@@ -244,137 +258,126 @@ where
 		if invoice.amount_milli_satoshis().is_some() {
 			Err(PaymentError::Invoice("amount unexpected"))
 		} else {
-			self.pay_invoice_internal(invoice, Some(amount_msats), 0)
+			self.pay_invoice_using_amount(invoice, Some(amount_msats))
 		}
 	}
 
-	fn pay_invoice_internal(
-		&self, invoice: &Invoice, amount_msats: Option<u64>, retry_count: usize
+	fn pay_invoice_using_amount(
+		&self, invoice: &Invoice, amount_msats: Option<u64>
 	) -> Result<PaymentId, PaymentError> {
 		debug_assert!(invoice.amount_milli_satoshis().is_some() ^ amount_msats.is_some());
+
 		let payment_hash = PaymentHash(invoice.payment_hash().clone().into_inner());
-		if invoice.is_expired() {
-			log_trace!(self.logger, "Invoice expired prior to first send for payment {}", log_bytes!(payment_hash.0));
+		let mut payment_cache = self.payment_cache.lock().unwrap();
+		let mut entry = match payment_cache.entry(payment_hash) {
+			hash_map::Entry::Occupied(_) => return Err(PaymentError::Invoice("payment pending")),
+			hash_map::Entry::Vacant(_) => look_up_or_insert!(payment_cache, payment_hash),
+		};
+
+		let payment_secret = Some(invoice.payment_secret().clone());
+		let mut payee = Payee::from_node_id(invoice.recover_payee_pub_key())
+			.with_expiry_time(expiry_time_from_unix_epoch(&invoice).as_secs())
+			.with_route_hints(invoice.route_hints());
+		if let Some(features) = invoice.features() {
+			payee = payee.with_features(features.clone());
+		}
+		let params = RouteParameters {
+			payee,
+			final_value_msat: invoice.amount_milli_satoshis().or(amount_msats).unwrap(),
+			final_cltv_expiry_delta: invoice.min_final_cltv_expiry() as u32,
+		};
+
+		self.pay_internal(&mut entry, &params, payment_hash, &payment_secret)
+			.map_err(|e| { entry.remove(); e })
+	}
+
+	fn pay_internal(
+		&self, entry: &mut PaymentEntry, params: &RouteParameters, payment_hash: PaymentHash,
+		payment_secret: &Option<PaymentSecret>,
+	) -> Result<PaymentId, PaymentError> {
+		if has_expired(params) {
+			log_trace!(self.logger, "Invoice expired prior to send for payment {}", log_bytes!(payment_hash.0));
 			return Err(PaymentError::Invoice("Invoice expired prior to send"));
 		}
-		let retry_data_payment_id = loop {
-			let mut payment_cache = self.payment_cache.lock().unwrap();
-			match payment_cache.entry(payment_hash) {
-				hash_map::Entry::Vacant(entry) => {
-					let payer = self.payer.node_id();
-					let mut payee = Payee::from_node_id(invoice.recover_payee_pub_key())
-						.with_expiry_time(expiry_time_from_unix_epoch(&invoice).as_secs())
-						.with_route_hints(invoice.route_hints());
-					if let Some(features) = invoice.features() {
-						payee = payee.with_features(features.clone());
+
+		let payer = self.payer.node_id();
+		let first_hops = self.payer.first_hops();
+		let route = self.router.find_route(
+			&payer,
+			params,
+			Some(&first_hops.iter().collect::<Vec<_>>()),
+			&self.scorer.lock(),
+		).map_err(|e| PaymentError::Routing(e))?;
+
+		match self.payer.send_payment(&route, payment_hash, payment_secret) {
+			Ok(payment_id) => Ok(payment_id),
+			Err(e) => match e {
+				PaymentSendFailure::ParameterError(_) => Err(e),
+				PaymentSendFailure::PathParameterError(_) => Err(e),
+				PaymentSendFailure::AllFailedRetrySafe(_) => {
+					let retry_count = entry.get_mut();
+					if *retry_count >= self.retry_attempts.0 {
+						Err(e)
+					} else {
+						*retry_count += 1;
+						Ok(self.pay_internal(entry, params, payment_hash, payment_secret)?)
 					}
-					let params = RouteParameters {
-						payee,
-						final_value_msat: invoice.amount_milli_satoshis().or(amount_msats).unwrap(),
-						final_cltv_expiry_delta: invoice.min_final_cltv_expiry() as u32,
-					};
-					let first_hops = self.payer.first_hops();
-					let route = self.router.find_route(
-						&payer,
-						&params,
-						Some(&first_hops.iter().collect::<Vec<_>>()),
-						&self.scorer.lock(),
-					).map_err(|e| PaymentError::Routing(e))?;
-
-					let payment_secret = Some(invoice.payment_secret().clone());
-					let payment_id = match self.payer.send_payment(&route, payment_hash, &payment_secret) {
-						Ok(payment_id) => payment_id,
-						Err(PaymentSendFailure::ParameterError(e)) =>
-							return Err(PaymentError::Sending(PaymentSendFailure::ParameterError(e))),
-						Err(PaymentSendFailure::PathParameterError(e)) =>
-							return Err(PaymentError::Sending(PaymentSendFailure::PathParameterError(e))),
-						Err(PaymentSendFailure::AllFailedRetrySafe(e)) => {
-							if retry_count >= self.retry_attempts.0 {
-								return Err(PaymentError::Sending(PaymentSendFailure::AllFailedRetrySafe(e)))
-							}
-							break None;
-						},
-						Err(PaymentSendFailure::PartialFailure { results: _, failed_paths_retry, payment_id }) => {
-							if let Some(retry_data) = failed_paths_retry {
-								entry.insert(retry_count);
-								break Some((retry_data, payment_id));
-							} else {
-								// This may happen if we send a payment and some paths fail, but
-								// only due to a temporary monitor failure or the like, implying
-								// they're really in-flight, but we haven't sent the initial
-								// HTLC-Add messages yet.
-								payment_id
-							}
-						},
-					};
-					entry.insert(retry_count);
-					return Ok(payment_id);
 				},
-				hash_map::Entry::Occupied(_) => return Err(PaymentError::Invoice("payment pending")),
-			}
-		};
-		if let Some((retry_data, payment_id)) = retry_data_payment_id {
-			// Some paths were sent, even if we failed to send the full MPP value our recipient may
-			// misbehave and claim the funds, at which point we have to consider the payment sent,
-			// so return `Ok()` here, ignoring any retry errors.
-			let _ = self.retry_payment(payment_id, payment_hash, &retry_data);
-			Ok(payment_id)
-		} else {
-			self.pay_invoice_internal(invoice, amount_msats, retry_count + 1)
-		}
+				PaymentSendFailure::PartialFailure { failed_paths_retry, payment_id, .. } => {
+					if let Some(retry_data) = failed_paths_retry {
+						let _ = self.retry_payment(entry, payment_id, &retry_data);
+						Ok(payment_id)
+					} else {
+						// This may happen if we send a payment and some paths fail, but
+						// only due to a temporary monitor failure or the like, implying
+						// they're really in-flight, but we haven't sent the initial
+						// HTLC-Add messages yet.
+						Ok(payment_id)
+					}
+				},
+			},
+		}.map_err(|e| PaymentError::Sending(e))
 	}
 
-	fn retry_payment(&self, payment_id: PaymentId, payment_hash: PaymentHash, params: &RouteParameters)
-	-> Result<(), ()> {
-		let route;
-		{
-			let mut payment_cache = self.payment_cache.lock().unwrap();
-			let entry = loop {
-				let entry = payment_cache.entry(payment_hash);
-				match entry {
-					hash_map::Entry::Occupied(_) => break entry,
-					hash_map::Entry::Vacant(entry) => entry.insert(0),
-				};
-			};
-			if let hash_map::Entry::Occupied(mut entry) = entry {
-				let max_payment_attempts = self.retry_attempts.0 + 1;
-				let attempts = entry.get_mut();
-				*attempts += 1;
-
-				if *attempts >= max_payment_attempts {
-					log_trace!(self.logger, "Payment {} exceeded maximum attempts; not retrying (attempts: {})", log_bytes!(payment_hash.0), attempts);
-					return Err(());
-				} else if has_expired(params) {
-					log_trace!(self.logger, "Invoice expired for payment {}; not retrying (attempts: {})", log_bytes!(payment_hash.0), attempts);
-					return Err(());
-				}
+	fn retry_payment(
+		&self, entry: &mut PaymentEntry, payment_id: PaymentId, params: &RouteParameters
+	) -> Result<(), ()> {
+		let payment_hash = *entry.key();
+		let max_payment_attempts = self.retry_attempts.0 + 1;
+		let attempts = entry.get_mut();
+		*attempts += 1;
+
+		if *attempts >= max_payment_attempts {
+			log_trace!(self.logger, "Payment {} exceeded maximum attempts; not retrying (attempts: {})", log_bytes!(payment_hash.0), attempts);
+			return Err(());
+		}
 
-				let payer = self.payer.node_id();
-				let first_hops = self.payer.first_hops();
-				route = self.router.find_route(&payer, &params, Some(&first_hops.iter().collect::<Vec<_>>()), &self.scorer.lock());
-				if route.is_err() {
-					log_trace!(self.logger, "Failed to find a route for payment {}; not retrying (attempts: {})", log_bytes!(payment_hash.0), attempts);
-					return Err(());
-				}
-			} else {
-				unreachable!();
-			}
+		if has_expired(params) {
+			log_trace!(self.logger, "Invoice expired for payment {}; not retrying (attempts: {})", log_bytes!(payment_hash.0), attempts);
+			return Err(());
 		}
 
-		let retry_res = self.payer.retry_payment(&route.unwrap(), payment_id);
-		match retry_res {
+		let payer = self.payer.node_id();
+		let first_hops = self.payer.first_hops();
+		let route = self.router.find_route(&payer, &params, Some(&first_hops.iter().collect::<Vec<_>>()), &self.scorer.lock());
+		if route.is_err() {
+			log_trace!(self.logger, "Failed to find a route for payment {}; not retrying (attempts: {})", log_bytes!(payment_hash.0), attempts);
+			return Err(());
+		}
+
+		match self.payer.retry_payment(&route.unwrap(), payment_id) {
 			Ok(()) => Ok(()),
 			Err(PaymentSendFailure::ParameterError(_)) |
 			Err(PaymentSendFailure::PathParameterError(_)) => {
 				log_trace!(self.logger, "Failed to retry for payment {} due to bogus route/payment data, not retrying.", log_bytes!(payment_hash.0));
-				return Err(());
+				Err(())
 			},
 			Err(PaymentSendFailure::AllFailedRetrySafe(_)) => {
-				self.retry_payment(payment_id, payment_hash, params)
+				self.retry_payment(entry, payment_id, params)
 			},
-			Err(PaymentSendFailure::PartialFailure { results: _, failed_paths_retry, .. }) => {
+			Err(PaymentSendFailure::PartialFailure { failed_paths_retry, .. }) => {
 				if let Some(retry) = failed_paths_retry {
-					self.retry_payment(payment_id, payment_hash, &retry)
+					self.retry_payment(entry, payment_id, &retry)
 				} else {
 					Ok(())
 				}
@@ -412,26 +415,31 @@ where
 	fn handle_event(&self, event: &Event) {
 		match event {
 			Event::PaymentPathFailed {
-				all_paths_failed, payment_id, payment_hash, rejected_by_dest, path, short_channel_id, retry, ..
+				all_paths_failed, payment_id, payment_hash, rejected_by_dest, path,
+				short_channel_id, retry, ..
 			} => {
 				if let Some(short_channel_id) = short_channel_id {
-					let t = path.iter().collect::<Vec<_>>();
-					self.scorer.lock().payment_path_failed(&t, *short_channel_id);
+					let path = path.iter().collect::<Vec<_>>();
+					self.scorer.lock().payment_path_failed(&path, *short_channel_id);
 				}
 
+				let mut payment_cache = self.payment_cache.lock().unwrap();
+				let mut entry = look_up_or_insert!(payment_cache, *payment_hash);
+
 				if *rejected_by_dest {
 					log_trace!(self.logger, "Payment {} rejected by destination; not retrying", log_bytes!(payment_hash.0));
 				} else if payment_id.is_none() {
 					log_trace!(self.logger, "Payment {} has no id; not retrying", log_bytes!(payment_hash.0));
-				} else if let Some(params) = retry {
-					if self.retry_payment(payment_id.unwrap(), *payment_hash, params).is_ok() {
+				} else if retry.is_none() {
+					log_trace!(self.logger, "Payment {} missing retry params; not retrying", log_bytes!(payment_hash.0));
+				} else {
+					if self.retry_payment(&mut entry, payment_id.unwrap(), retry.as_ref().unwrap()).is_ok() {
 						// We retried at least somewhat, don't provide the PaymentPathFailed event to the user.
 						return;
 					}
-				} else {
-					log_trace!(self.logger, "Payment {} missing retry params; not retrying", log_bytes!(payment_hash.0));
 				}
-				if *all_paths_failed { self.payment_cache.lock().unwrap().remove(payment_hash); }
+
+				if *all_paths_failed { entry.remove(); }
 			},
 			Event::PaymentSent { payment_hash, .. } => {
 				let mut payment_cache = self.payment_cache.lock().unwrap();