Refactor InvoicePayer for spontaneous payments
authorJeffrey Czyz <jkczyz@gmail.com>
Tue, 9 Nov 2021 15:37:34 +0000 (09:37 -0600)
committerJeffrey Czyz <jkczyz@gmail.com>
Tue, 16 Nov 2021 19:57:25 +0000 (13:57 -0600)
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 refactor the code's payment_cache locking such that it is
accessed consistently when needed, and tidy up the code a bit.

lightning-invoice/src/payment.rs

index 075559bfd8ed857878e391fcd2a396ae3742093c..1a7242b8ef6961ac0f6d60da1b929ffba0a1eb81 100644 (file)
@@ -144,6 +144,7 @@ where
        scorer: S,
        logger: L,
        event_handler: E,
+       /// Caches the overall attempts at making a payment, which is updated prior to retrying.
        payment_cache: Mutex<HashMap<PaymentHash, usize>>,
        retry_attempts: RetryAttempts,
 }
@@ -228,7 +229,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,140 +245,135 @@ 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));
+               match self.payment_cache.lock().unwrap().entry(payment_hash) {
+                       hash_map::Entry::Occupied(_) => return Err(PaymentError::Invoice("payment pending")),
+                       hash_map::Entry::Vacant(entry) => entry.insert(0),
+               };
+
+               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(&params, payment_hash, &payment_secret)
+                       .map_err(|e| { self.payment_cache.lock().unwrap().remove(&payment_hash); e })
+       }
+
+       fn pay_internal(
+               &self, 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 mut payment_cache = self.payment_cache.lock().unwrap();
+                                       let retry_count = payment_cache.get_mut(&payment_hash).unwrap();
+                                       if *retry_count >= self.retry_attempts.0 {
+                                               Err(e)
+                                       } else {
+                                               *retry_count += 1;
+                                               std::mem::drop(payment_cache);
+                                               Ok(self.pay_internal(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 {
+                                               // 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 {
+                                               // 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, payment_id: PaymentId, payment_hash: PaymentHash, params: &RouteParameters
+       ) -> Result<(), ()> {
+               let max_payment_attempts = self.retry_attempts.0 + 1;
+               let attempts = *self.payment_cache.lock().unwrap()
+                       .entry(payment_hash)
+                       .and_modify(|attempts| *attempts += 1)
+                       .or_insert(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 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(());
                }
 
-               let retry_res = self.payer.retry_payment(&route.unwrap(), payment_id);
-               match retry_res {
+               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)
                        },
-                       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)
-                               } else {
-                                       Ok(())
+                                       // Always return Ok for the same reason as noted in pay_internal.
+                                       let _ = self.retry_payment(payment_id, payment_hash, &retry);
                                }
+                               Ok(())
                        },
                }
        }
@@ -412,25 +408,25 @@ 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);
                                }
 
                                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() {
-                                               // We retried at least somewhat, don't provide the PaymentPathFailed event to the user.
-                                               return;
-                                       }
-                               } else {
+                               } 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(payment_id.unwrap(), *payment_hash, retry.as_ref().unwrap()).is_ok() {
+                                       // We retried at least somewhat, don't provide the PaymentPathFailed event to the user.
+                                       return;
                                }
+
                                if *all_paths_failed { self.payment_cache.lock().unwrap().remove(payment_hash); }
                        },
                        Event::PaymentSent { payment_hash, .. } => {