Merge pull request #1846 from TheBlueMatt/2022-11-more-robust-unconfirmed
[rust-lightning] / lightning-invoice / src / payment.rs
index a64c8440399e67864d4a446371cf2738b3cce284..a1c187915b98a81cb6f78d582458bbc3e4524dd6 100644 (file)
 //! #     fn node_id(&self) -> PublicKey { unimplemented!() }
 //! #     fn first_hops(&self) -> Vec<ChannelDetails> { unimplemented!() }
 //! #     fn send_payment(
-//! #         &self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>
-//! #     ) -> Result<PaymentId, PaymentSendFailure> { unimplemented!() }
+//! #         &self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>,
+//! #         payment_id: PaymentId
+//! #     ) -> Result<(), PaymentSendFailure> { unimplemented!() }
 //! #     fn send_spontaneous_payment(
-//! #         &self, route: &Route, payment_preimage: PaymentPreimage
-//! #     ) -> Result<PaymentId, PaymentSendFailure> { unimplemented!() }
+//! #         &self, route: &Route, payment_preimage: PaymentPreimage, payment_id: PaymentId,
+//! #     ) -> Result<(), PaymentSendFailure> { unimplemented!() }
 //! #     fn retry_payment(
 //! #         &self, route: &Route, payment_id: PaymentId
 //! #     ) -> Result<(), PaymentSendFailure> { unimplemented!() }
 //! # }
 //! #
 //! # fn main() {
-//! let event_handler = |event: &Event| {
+//! let event_handler = |event: Event| {
 //!     match event {
 //!         Event::PaymentPathFailed { .. } => println!("payment failed after retries"),
 //!         Event::PaymentSent { .. } => println!("payment successful"),
@@ -156,6 +157,7 @@ use secp256k1::PublicKey;
 
 use core::fmt;
 use core::fmt::{Debug, Display, Formatter};
+use core::future::Future;
 use core::ops::Deref;
 use core::time::Duration;
 #[cfg(feature = "std")]
@@ -175,9 +177,21 @@ use crate::time_utils;
 #[cfg(feature = "no-std")]
 type ConfiguredTime = time_utils::Eternity;
 
+/// Sealed trait with a blanket implementation to allow both sync and async implementations of event
+/// handling to exist within the InvoicePayer.
+mod sealed {
+       pub trait BaseEventHandler {}
+       impl<T> BaseEventHandler for T {}
+}
+
 /// (C-not exported) generally all users should use the [`InvoicePayer`] type alias.
-pub struct InvoicePayerUsingTime<P: Deref, R: ScoringRouter, L: Deref, E: EventHandler, T: Time>
-where
+pub struct InvoicePayerUsingTime<
+       P: Deref,
+       R: ScoringRouter,
+       L: Deref,
+       E: sealed::BaseEventHandler,
+       T: Time
+> where
        P::Target: Payer,
        L::Target: Logger,
 {
@@ -241,6 +255,18 @@ impl<T: Time> Display for PaymentAttempts<T> {
 }
 
 /// A trait defining behavior of an [`Invoice`] payer.
+///
+/// While the behavior of [`InvoicePayer`] provides idempotency of duplicate `send_*payment` calls
+/// with the same [`PaymentHash`], it is up to the `Payer` to provide idempotency across restarts.
+///
+/// [`ChannelManager`] provides idempotency for duplicate payments with the same [`PaymentId`].
+///
+/// In order to trivially ensure idempotency for payments, the default `Payer` implementation
+/// reuses the [`PaymentHash`] bytes as the [`PaymentId`]. Custom implementations wishing to
+/// provide payment idempotency with a different idempotency key (i.e. [`PaymentId`]) should map
+/// the [`Invoice`] or spontaneous payment target pubkey to their own idempotency key.
+///
+/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
 pub trait Payer {
        /// Returns the payer's node id.
        fn node_id(&self) -> PublicKey;
@@ -250,13 +276,14 @@ pub trait Payer {
 
        /// Sends a payment over the Lightning Network using the given [`Route`].
        fn send_payment(
-               &self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>
-       ) -> Result<PaymentId, PaymentSendFailure>;
+               &self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>,
+               payment_id: PaymentId
+       ) -> Result<(), PaymentSendFailure>;
 
        /// Sends a spontaneous payment over the Lightning Network using the given [`Route`].
        fn send_spontaneous_payment(
-               &self, route: &Route, payment_preimage: PaymentPreimage
-       ) -> Result<PaymentId, PaymentSendFailure>;
+               &self, route: &Route, payment_preimage: PaymentPreimage, payment_id: PaymentId
+       ) -> Result<(), PaymentSendFailure>;
 
        /// Retries a failed payment path for the [`PaymentId`] using the given [`Route`].
        fn retry_payment(&self, route: &Route, payment_id: PaymentId) -> Result<(), PaymentSendFailure>;
@@ -328,7 +355,8 @@ pub enum PaymentError {
        Sending(PaymentSendFailure),
 }
 
-impl<P: Deref, R: ScoringRouter, L: Deref, E: EventHandler, T: Time> InvoicePayerUsingTime<P, R, L, E, T>
+impl<P: Deref, R: ScoringRouter, L: Deref, E: sealed::BaseEventHandler, T: Time>
+       InvoicePayerUsingTime<P, R, L, E, T>
 where
        P::Target: Payer,
        L::Target: Logger,
@@ -352,36 +380,76 @@ where
 
        /// Pays the given [`Invoice`], caching it for later use in case a retry is needed.
        ///
-       /// You should ensure that the `invoice.payment_hash()` is unique and the same payment_hash has
-       /// never been paid before. Because [`InvoicePayer`] is stateless no effort is made to do so
-       /// for you.
+       /// [`Invoice::payment_hash`] is used as the [`PaymentId`], which ensures idempotency as long
+       /// as the payment is still pending. Once the payment completes or fails, you must ensure that
+       /// a second payment with the same [`PaymentHash`] is never sent.
+       ///
+       /// If you wish to use a different payment idempotency token, see
+       /// [`Self::pay_invoice_with_id`].
        pub fn pay_invoice(&self, invoice: &Invoice) -> Result<PaymentId, PaymentError> {
+               let payment_id = PaymentId(invoice.payment_hash().into_inner());
+               self.pay_invoice_with_id(invoice, payment_id).map(|()| payment_id)
+       }
+
+       /// Pays the given [`Invoice`] with a custom idempotency key, caching the invoice for later use
+       /// in case a retry is needed.
+       ///
+       /// Note that idempotency is only guaranteed as long as the payment is still pending. Once the
+       /// payment completes or fails, no idempotency guarantees are made.
+       ///
+       /// You should ensure that the [`Invoice::payment_hash`] is unique and the same [`PaymentHash`]
+       /// has never been paid before.
+       ///
+       /// See [`Self::pay_invoice`] for a variant which uses the [`PaymentHash`] for the idempotency
+       /// token.
+       pub fn pay_invoice_with_id(&self, invoice: &Invoice, payment_id: PaymentId) -> Result<(), PaymentError> {
                if invoice.amount_milli_satoshis().is_none() {
                        Err(PaymentError::Invoice("amount missing"))
                } else {
-                       self.pay_invoice_using_amount(invoice, None)
+                       self.pay_invoice_using_amount(invoice, None, payment_id)
                }
        }
 
        /// Pays the given zero-value [`Invoice`] using the given amount, caching it for later use in
        /// case a retry is needed.
        ///
-       /// You should ensure that the `invoice.payment_hash()` is unique and the same payment_hash has
-       /// never been paid before. Because [`InvoicePayer`] is stateless no effort is made to do so
-       /// for you.
+       /// [`Invoice::payment_hash`] is used as the [`PaymentId`], which ensures idempotency as long
+       /// as the payment is still pending. Once the payment completes or fails, you must ensure that
+       /// a second payment with the same [`PaymentHash`] is never sent.
+       ///
+       /// If you wish to use a different payment idempotency token, see
+       /// [`Self::pay_zero_value_invoice_with_id`].
        pub fn pay_zero_value_invoice(
                &self, invoice: &Invoice, amount_msats: u64
        ) -> Result<PaymentId, PaymentError> {
+               let payment_id = PaymentId(invoice.payment_hash().into_inner());
+               self.pay_zero_value_invoice_with_id(invoice, amount_msats, payment_id).map(|()| payment_id)
+       }
+
+       /// Pays the given zero-value [`Invoice`] using the given amount and custom idempotency key,
+       /// caching the invoice for later use in case a retry is needed.
+       ///
+       /// Note that idempotency is only guaranteed as long as the payment is still pending. Once the
+       /// payment completes or fails, no idempotency guarantees are made.
+       ///
+       /// You should ensure that the [`Invoice::payment_hash`] is unique and the same [`PaymentHash`]
+       /// has never been paid before.
+       ///
+       /// See [`Self::pay_zero_value_invoice`] for a variant which uses the [`PaymentHash`] for the
+       /// idempotency token.
+       pub fn pay_zero_value_invoice_with_id(
+               &self, invoice: &Invoice, amount_msats: u64, payment_id: PaymentId
+       ) -> Result<(), PaymentError> {
                if invoice.amount_milli_satoshis().is_some() {
                        Err(PaymentError::Invoice("amount unexpected"))
                } else {
-                       self.pay_invoice_using_amount(invoice, Some(amount_msats))
+                       self.pay_invoice_using_amount(invoice, Some(amount_msats), payment_id)
                }
        }
 
        fn pay_invoice_using_amount(
-               &self, invoice: &Invoice, amount_msats: Option<u64>
-       ) -> Result<PaymentId, PaymentError> {
+               &self, invoice: &Invoice, amount_msats: Option<u64>, payment_id: PaymentId
+       ) -> Result<(), PaymentError> {
                debug_assert!(invoice.amount_milli_satoshis().is_some() ^ amount_msats.is_some());
 
                let payment_hash = PaymentHash(invoice.payment_hash().clone().into_inner());
@@ -404,7 +472,7 @@ where
                };
 
                let send_payment = |route: &Route| {
-                       self.payer.send_payment(route, payment_hash, &payment_secret)
+                       self.payer.send_payment(route, payment_hash, &payment_secret, payment_id)
                };
 
                self.pay_internal(&route_params, payment_hash, send_payment)
@@ -414,13 +482,41 @@ where
        /// Pays `pubkey` an amount using the hash of the given preimage, caching it for later use in
        /// case a retry is needed.
        ///
-       /// You should ensure that `payment_preimage` is unique and that its `payment_hash` has never
-       /// been paid before. Because [`InvoicePayer`] is stateless no effort is made to do so for you.
+       /// The hash of the [`PaymentPreimage`] is used as the [`PaymentId`], which ensures idempotency
+       /// as long as the payment is still pending. Once the payment completes or fails, you must
+       /// ensure that a second payment with the same [`PaymentPreimage`] is never sent.
        pub fn pay_pubkey(
                &self, pubkey: PublicKey, payment_preimage: PaymentPreimage, amount_msats: u64,
                final_cltv_expiry_delta: u32
        ) -> Result<PaymentId, PaymentError> {
                let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
+               let payment_id = PaymentId(payment_hash.0);
+               self.do_pay_pubkey(pubkey, payment_preimage, payment_hash, payment_id, amount_msats,
+                               final_cltv_expiry_delta)
+                       .map(|()| payment_id)
+       }
+
+       /// Pays `pubkey` an amount using the hash of the given preimage and a custom idempotency key,
+       /// caching the invoice for later use in case a retry is needed.
+       ///
+       /// Note that idempotency is only guaranteed as long as the payment is still pending. Once the
+       /// payment completes or fails, no idempotency guarantees are made.
+       ///
+       /// You should ensure that the [`PaymentPreimage`] is unique and the corresponding
+       /// [`PaymentHash`] has never been paid before.
+       pub fn pay_pubkey_with_id(
+               &self, pubkey: PublicKey, payment_preimage: PaymentPreimage, payment_id: PaymentId,
+               amount_msats: u64, final_cltv_expiry_delta: u32
+       ) -> Result<(), PaymentError> {
+               let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
+               self.do_pay_pubkey(pubkey, payment_preimage, payment_hash, payment_id, amount_msats,
+                               final_cltv_expiry_delta)
+       }
+
+       fn do_pay_pubkey(
+               &self, pubkey: PublicKey, payment_preimage: PaymentPreimage, payment_hash: PaymentHash,
+               payment_id: PaymentId, amount_msats: u64, final_cltv_expiry_delta: u32
+       ) -> Result<(), PaymentError> {
                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(PaymentInfo::new()),
@@ -433,15 +529,15 @@ where
                };
 
                let send_payment = |route: &Route| {
-                       self.payer.send_spontaneous_payment(route, payment_preimage)
+                       self.payer.send_spontaneous_payment(route, payment_preimage, payment_id)
                };
                self.pay_internal(&route_params, payment_hash, send_payment)
                        .map_err(|e| { self.payment_cache.lock().unwrap().remove(&payment_hash); e })
        }
 
-       fn pay_internal<F: FnOnce(&Route) -> Result<PaymentId, PaymentSendFailure> + Copy>(
+       fn pay_internal<F: FnOnce(&Route) -> Result<(), PaymentSendFailure> + Copy>(
                &self, params: &RouteParameters, payment_hash: PaymentHash, send_payment: F,
-       ) -> Result<PaymentId, PaymentError> {
+       ) -> Result<(), PaymentError> {
                #[cfg(feature = "std")] {
                        if has_expired(params) {
                                log_trace!(self.logger, "Invoice expired prior to send for payment {}", log_bytes!(payment_hash.0));
@@ -457,16 +553,17 @@ where
                ).map_err(|e| PaymentError::Routing(e))?;
 
                match send_payment(&route) {
-                       Ok(payment_id) => {
+                       Ok(()) => {
                                for path in route.paths {
                                        self.process_path_inflight_htlcs(payment_hash, path);
                                }
-                               Ok(payment_id)
+                               Ok(())
                        },
                        Err(e) => match e {
                                PaymentSendFailure::ParameterError(_) => Err(e),
                                PaymentSendFailure::PathParameterError(_) => Err(e),
-                               PaymentSendFailure::AllFailedRetrySafe(_) => {
+                               PaymentSendFailure::DuplicatePayment => Err(e),
+                               PaymentSendFailure::AllFailedResendSafe(_) => {
                                        let mut payment_cache = self.payment_cache.lock().unwrap();
                                        let payment_info = payment_cache.get_mut(&payment_hash).unwrap();
                                        payment_info.attempts.count += 1;
@@ -496,13 +593,13 @@ where
                                                // 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)
+                                               Ok(())
                                        } 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)
+                                               Ok(())
                                        }
                                },
                        },
@@ -577,9 +674,13 @@ where
                                log_trace!(self.logger, "Failed to retry for payment {} due to bogus route/payment data, not retrying.", log_bytes!(payment_hash.0));
                                Err(())
                        },
-                       Err(PaymentSendFailure::AllFailedRetrySafe(_)) => {
+                       Err(PaymentSendFailure::AllFailedResendSafe(_)) => {
                                self.retry_payment(payment_id, payment_hash, params)
                        },
+                       Err(PaymentSendFailure::DuplicatePayment) => {
+                               log_error!(self.logger, "Got a DuplicatePayment error when attempting to retry a payment, this shouldn't happen.");
+                               Err(())
+                       }
                        Err(PaymentSendFailure::PartialFailure { failed_paths_retry, results, .. }) => {
                                // If a `PartialFailure` error contains a result that is an `Ok()`, it means that
                                // part of our payment is retried. When we receive `MonitorUpdateInProgress`, it
@@ -662,12 +763,15 @@ fn has_expired(route_params: &RouteParameters) -> bool {
        } else { false }
 }
 
-impl<P: Deref, R: ScoringRouter, L: Deref, E: EventHandler, T: Time> EventHandler for InvoicePayerUsingTime<P, R, L, E, T>
+impl<P: Deref, R: ScoringRouter, L: Deref, E: sealed::BaseEventHandler, T: Time>
+       InvoicePayerUsingTime<P, R, L, E, T>
 where
        P::Target: Payer,
        L::Target: Logger,
 {
-       fn handle_event(&self, event: &Event) {
+       /// Returns a bool indicating whether the processed event should be forwarded to a user-provided
+       /// event handler.
+       fn handle_event_internal(&self, event: &Event) -> bool {
                match event {
                        Event::PaymentPathFailed { payment_hash, path, ..  }
                        | Event::PaymentPathSuccessful { path, payment_hash: Some(payment_hash), .. }
@@ -697,7 +801,7 @@ where
                                        self.payer.abandon_payment(payment_id.unwrap());
                                } 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;
+                                       return false;
                                } else {
                                        self.payer.abandon_payment(payment_id.unwrap());
                                }
@@ -732,7 +836,37 @@ where
                }
 
                // Delegate to the decorated event handler unless the payment is retried.
-               self.event_handler.handle_event(event)
+               true
+       }
+}
+
+impl<P: Deref, R: ScoringRouter, L: Deref, E: EventHandler, T: Time>
+       EventHandler for InvoicePayerUsingTime<P, R, L, E, T>
+where
+       P::Target: Payer,
+       L::Target: Logger,
+{
+       fn handle_event(&self, event: Event) {
+               let should_forward = self.handle_event_internal(&event);
+               if should_forward {
+                       self.event_handler.handle_event(event)
+               }
+       }
+}
+
+impl<P: Deref, R: ScoringRouter, L: Deref, T: Time, F: Future, H: Fn(Event) -> F>
+       InvoicePayerUsingTime<P, R, L, H, T>
+where
+       P::Target: Payer,
+       L::Target: Logger,
+{
+       /// Intercepts events required by the [`InvoicePayer`] and forwards them to the underlying event
+       /// handler, if necessary, to handle them asynchronously.
+       pub async fn handle_event_async(&self, event: Event) {
+               let should_forward = self.handle_event_internal(&event);
+               if should_forward {
+                       (self.event_handler)(event).await;
+               }
        }
 }
 
@@ -831,7 +965,7 @@ mod tests {
        #[test]
        fn pays_invoice_on_first_attempt() {
                let event_handled = core::cell::RefCell::new(false);
-               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+               let event_handler = |_: Event| { *event_handled.borrow_mut() = true; };
 
                let payment_preimage = PaymentPreimage([1; 32]);
                let invoice = invoice(payment_preimage);
@@ -847,7 +981,7 @@ mod tests {
                let payment_id = Some(invoice_payer.pay_invoice(&invoice).unwrap());
                assert_eq!(*payer.attempts.borrow(), 1);
 
-               invoice_payer.handle_event(&Event::PaymentSent {
+               invoice_payer.handle_event(Event::PaymentSent {
                        payment_id, payment_preimage, payment_hash, fee_paid_msat: None
                });
                assert_eq!(*event_handled.borrow(), true);
@@ -857,7 +991,7 @@ mod tests {
        #[test]
        fn pays_invoice_on_retry() {
                let event_handled = core::cell::RefCell::new(false);
-               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+               let event_handler = |_: Event| { *event_handled.borrow_mut() = true; };
 
                let payment_preimage = PaymentPreimage([1; 32]);
                let invoice = invoice(payment_preimage);
@@ -885,11 +1019,11 @@ mod tests {
                        short_channel_id: None,
                        retry: Some(TestRouter::retry_for_invoice(&invoice)),
                };
-               invoice_payer.handle_event(&event);
+               invoice_payer.handle_event(event);
                assert_eq!(*event_handled.borrow(), false);
                assert_eq!(*payer.attempts.borrow(), 2);
 
-               invoice_payer.handle_event(&Event::PaymentSent {
+               invoice_payer.handle_event(Event::PaymentSent {
                        payment_id, payment_preimage, payment_hash, fee_paid_msat: None
                });
                assert_eq!(*event_handled.borrow(), true);
@@ -898,7 +1032,7 @@ mod tests {
 
        #[test]
        fn pays_invoice_on_partial_failure() {
-               let event_handler = |_: &_| { panic!() };
+               let event_handler = |_: Event| { panic!() };
 
                let payment_preimage = PaymentPreimage([1; 32]);
                let invoice = invoice(payment_preimage);
@@ -922,7 +1056,7 @@ mod tests {
        #[test]
        fn retries_payment_path_for_unknown_payment() {
                let event_handled = core::cell::RefCell::new(false);
-               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+               let event_handler = |_: Event| { *event_handled.borrow_mut() = true; };
 
                let payment_preimage = PaymentPreimage([1; 32]);
                let invoice = invoice(payment_preimage);
@@ -948,15 +1082,15 @@ mod tests {
                        short_channel_id: None,
                        retry: Some(TestRouter::retry_for_invoice(&invoice)),
                };
-               invoice_payer.handle_event(&event);
+               invoice_payer.handle_event(event.clone());
                assert_eq!(*event_handled.borrow(), false);
                assert_eq!(*payer.attempts.borrow(), 1);
 
-               invoice_payer.handle_event(&event);
+               invoice_payer.handle_event(event.clone());
                assert_eq!(*event_handled.borrow(), false);
                assert_eq!(*payer.attempts.borrow(), 2);
 
-               invoice_payer.handle_event(&Event::PaymentSent {
+               invoice_payer.handle_event(Event::PaymentSent {
                        payment_id, payment_preimage, payment_hash, fee_paid_msat: None
                });
                assert_eq!(*event_handled.borrow(), true);
@@ -966,7 +1100,7 @@ mod tests {
        #[test]
        fn fails_paying_invoice_after_max_retry_counts() {
                let event_handled = core::cell::RefCell::new(false);
-               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+               let event_handler = |_: Event| { *event_handled.borrow_mut() = true; };
 
                let payment_preimage = PaymentPreimage([1; 32]);
                let invoice = invoice(payment_preimage);
@@ -994,7 +1128,7 @@ mod tests {
                        short_channel_id: None,
                        retry: Some(TestRouter::retry_for_invoice(&invoice)),
                };
-               invoice_payer.handle_event(&event);
+               invoice_payer.handle_event(event);
                assert_eq!(*event_handled.borrow(), false);
                assert_eq!(*payer.attempts.borrow(), 2);
 
@@ -1010,11 +1144,11 @@ mod tests {
                                final_value_msat: final_value_msat / 2, ..TestRouter::retry_for_invoice(&invoice)
                        }),
                };
-               invoice_payer.handle_event(&event);
+               invoice_payer.handle_event(event.clone());
                assert_eq!(*event_handled.borrow(), false);
                assert_eq!(*payer.attempts.borrow(), 3);
 
-               invoice_payer.handle_event(&event);
+               invoice_payer.handle_event(event.clone());
                assert_eq!(*event_handled.borrow(), true);
                assert_eq!(*payer.attempts.borrow(), 3);
        }
@@ -1023,7 +1157,7 @@ mod tests {
        #[test]
        fn fails_paying_invoice_after_max_retry_timeout() {
                let event_handled = core::cell::RefCell::new(false);
-               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+               let event_handler = |_: Event| { *event_handled.borrow_mut() = true; };
 
                let payment_preimage = PaymentPreimage([1; 32]);
                let invoice = invoice(payment_preimage);
@@ -1053,13 +1187,13 @@ mod tests {
                        short_channel_id: None,
                        retry: Some(TestRouter::retry_for_invoice(&invoice)),
                };
-               invoice_payer.handle_event(&event);
+               invoice_payer.handle_event(event.clone());
                assert_eq!(*event_handled.borrow(), false);
                assert_eq!(*payer.attempts.borrow(), 2);
 
                SinceEpoch::advance(Duration::from_secs(121));
 
-               invoice_payer.handle_event(&event);
+               invoice_payer.handle_event(event.clone());
                assert_eq!(*event_handled.borrow(), true);
                assert_eq!(*payer.attempts.borrow(), 2);
        }
@@ -1067,7 +1201,7 @@ mod tests {
        #[test]
        fn fails_paying_invoice_with_missing_retry_params() {
                let event_handled = core::cell::RefCell::new(false);
-               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+               let event_handler = |_: Event| { *event_handled.borrow_mut() = true; };
 
                let payment_preimage = PaymentPreimage([1; 32]);
                let invoice = invoice(payment_preimage);
@@ -1092,7 +1226,7 @@ mod tests {
                        short_channel_id: None,
                        retry: None,
                };
-               invoice_payer.handle_event(&event);
+               invoice_payer.handle_event(event);
                assert_eq!(*event_handled.borrow(), true);
                assert_eq!(*payer.attempts.borrow(), 1);
        }
@@ -1102,7 +1236,7 @@ mod tests {
        #[test]
        fn fails_paying_invoice_after_expiration() {
                let event_handled = core::cell::RefCell::new(false);
-               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+               let event_handler = |_: Event| { *event_handled.borrow_mut() = true; };
 
                let payer = TestPayer::new();
                let router = TestRouter::new(TestScorer::new());
@@ -1122,7 +1256,7 @@ mod tests {
        #[test]
        fn fails_retrying_invoice_after_expiration() {
                let event_handled = core::cell::RefCell::new(false);
-               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+               let event_handler = |_: Event| { *event_handled.borrow_mut() = true; };
 
                let payment_preimage = PaymentPreimage([1; 32]);
                let invoice = invoice(payment_preimage);
@@ -1151,7 +1285,7 @@ mod tests {
                        short_channel_id: None,
                        retry: Some(retry_data),
                };
-               invoice_payer.handle_event(&event);
+               invoice_payer.handle_event(event);
                assert_eq!(*event_handled.borrow(), true);
                assert_eq!(*payer.attempts.borrow(), 1);
        }
@@ -1159,7 +1293,7 @@ mod tests {
        #[test]
        fn fails_paying_invoice_after_retry_error() {
                let event_handled = core::cell::RefCell::new(false);
-               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+               let event_handler = |_: Event| { *event_handled.borrow_mut() = true; };
 
                let payment_preimage = PaymentPreimage([1; 32]);
                let invoice = invoice(payment_preimage);
@@ -1187,7 +1321,7 @@ mod tests {
                        short_channel_id: None,
                        retry: Some(TestRouter::retry_for_invoice(&invoice)),
                };
-               invoice_payer.handle_event(&event);
+               invoice_payer.handle_event(event);
                assert_eq!(*event_handled.borrow(), true);
                assert_eq!(*payer.attempts.borrow(), 2);
        }
@@ -1195,7 +1329,7 @@ mod tests {
        #[test]
        fn fails_paying_invoice_after_rejected_by_payee() {
                let event_handled = core::cell::RefCell::new(false);
-               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+               let event_handler = |_: Event| { *event_handled.borrow_mut() = true; };
 
                let payment_preimage = PaymentPreimage([1; 32]);
                let invoice = invoice(payment_preimage);
@@ -1220,7 +1354,7 @@ mod tests {
                        short_channel_id: None,
                        retry: Some(TestRouter::retry_for_invoice(&invoice)),
                };
-               invoice_payer.handle_event(&event);
+               invoice_payer.handle_event(event);
                assert_eq!(*event_handled.borrow(), true);
                assert_eq!(*payer.attempts.borrow(), 1);
        }
@@ -1228,7 +1362,7 @@ mod tests {
        #[test]
        fn fails_repaying_invoice_with_pending_payment() {
                let event_handled = core::cell::RefCell::new(false);
-               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+               let event_handler = |_: Event| { *event_handled.borrow_mut() = true; };
 
                let payment_preimage = PaymentPreimage([1; 32]);
                let invoice = invoice(payment_preimage);
@@ -1268,7 +1402,7 @@ mod tests {
                        short_channel_id: None,
                        retry: Some(TestRouter::retry_for_invoice(&invoice)),
                };
-               invoice_payer.handle_event(&event);
+               invoice_payer.handle_event(event);
                assert_eq!(*event_handled.borrow(), true);
        }
 
@@ -1278,7 +1412,7 @@ mod tests {
                let router = FailingRouter {};
                let logger = TestLogger::new();
                let invoice_payer =
-                       InvoicePayer::new(&payer, router, &logger, |_: &_| {}, Retry::Attempts(0));
+                       InvoicePayer::new(&payer, router, &logger, |_: Event| {}, Retry::Attempts(0));
 
                let payment_preimage = PaymentPreimage([1; 32]);
                let invoice = invoice(payment_preimage);
@@ -1301,7 +1435,7 @@ mod tests {
                let router = TestRouter::new(TestScorer::new());
                let logger = TestLogger::new();
                let invoice_payer =
-                       InvoicePayer::new(&payer, router, &logger, |_: &_| {}, Retry::Attempts(0));
+                       InvoicePayer::new(&payer, router, &logger, |_: Event| {}, Retry::Attempts(0));
 
                match invoice_payer.pay_invoice(&invoice) {
                        Err(PaymentError::Sending(_)) => {},
@@ -1313,7 +1447,7 @@ mod tests {
        #[test]
        fn pays_zero_value_invoice_using_amount() {
                let event_handled = core::cell::RefCell::new(false);
-               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+               let event_handler = |_: Event| { *event_handled.borrow_mut() = true; };
 
                let payment_preimage = PaymentPreimage([1; 32]);
                let invoice = zero_value_invoice(payment_preimage);
@@ -1330,7 +1464,7 @@ mod tests {
                        Some(invoice_payer.pay_zero_value_invoice(&invoice, final_value_msat).unwrap());
                assert_eq!(*payer.attempts.borrow(), 1);
 
-               invoice_payer.handle_event(&Event::PaymentSent {
+               invoice_payer.handle_event(Event::PaymentSent {
                        payment_id, payment_preimage, payment_hash, fee_paid_msat: None
                });
                assert_eq!(*event_handled.borrow(), true);
@@ -1340,7 +1474,7 @@ mod tests {
        #[test]
        fn fails_paying_zero_value_invoice_with_amount() {
                let event_handled = core::cell::RefCell::new(false);
-               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+               let event_handler = |_: Event| { *event_handled.borrow_mut() = true; };
 
                let payer = TestPayer::new();
                let router = TestRouter::new(TestScorer::new());
@@ -1362,7 +1496,7 @@ mod tests {
        #[test]
        fn pays_pubkey_with_amount() {
                let event_handled = core::cell::RefCell::new(false);
-               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+               let event_handler = |_: Event| { *event_handled.borrow_mut() = true; };
 
                let pubkey = pubkey();
                let payment_preimage = PaymentPreimage([1; 32]);
@@ -1398,11 +1532,11 @@ mod tests {
                        short_channel_id: None,
                        retry: Some(retry),
                };
-               invoice_payer.handle_event(&event);
+               invoice_payer.handle_event(event);
                assert_eq!(*event_handled.borrow(), false);
                assert_eq!(*payer.attempts.borrow(), 2);
 
-               invoice_payer.handle_event(&Event::PaymentSent {
+               invoice_payer.handle_event(Event::PaymentSent {
                        payment_id, payment_preimage, payment_hash, fee_paid_msat: None
                });
                assert_eq!(*event_handled.borrow(), true);
@@ -1412,7 +1546,7 @@ mod tests {
        #[test]
        fn scores_failed_channel() {
                let event_handled = core::cell::RefCell::new(false);
-               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+               let event_handler = |_: Event| { *event_handled.borrow_mut() = true; };
 
                let payment_preimage = PaymentPreimage([1; 32]);
                let invoice = invoice(payment_preimage);
@@ -1444,13 +1578,13 @@ mod tests {
                        short_channel_id,
                        retry: Some(TestRouter::retry_for_invoice(&invoice)),
                };
-               invoice_payer.handle_event(&event);
+               invoice_payer.handle_event(event);
        }
 
        #[test]
        fn scores_successful_channels() {
                let event_handled = core::cell::RefCell::new(false);
-               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+               let event_handler = |_: Event| { *event_handled.borrow_mut() = true; };
 
                let payment_preimage = PaymentPreimage([1; 32]);
                let invoice = invoice(payment_preimage);
@@ -1472,17 +1606,17 @@ mod tests {
                let event = Event::PaymentPathSuccessful {
                        payment_id, payment_hash, path: route.paths[0].clone()
                };
-               invoice_payer.handle_event(&event);
+               invoice_payer.handle_event(event);
                let event = Event::PaymentPathSuccessful {
                        payment_id, payment_hash, path: route.paths[1].clone()
                };
-               invoice_payer.handle_event(&event);
+               invoice_payer.handle_event(event);
        }
 
        #[test]
        fn generates_correct_inflight_map_data() {
                let event_handled = core::cell::RefCell::new(false);
-               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+               let event_handler = |_: Event| { *event_handled.borrow_mut() = true; };
 
                let payment_preimage = PaymentPreimage([1; 32]);
                let invoice = invoice(payment_preimage);
@@ -1509,7 +1643,7 @@ mod tests {
                assert_eq!(inflight_map.0.get(&(3, false)).unwrap().clone(), 74);
                assert_eq!(inflight_map.0.get(&(4, false)).unwrap().clone(), 64);
 
-               invoice_payer.handle_event(&Event::PaymentPathSuccessful {
+               invoice_payer.handle_event(Event::PaymentPathSuccessful {
                        payment_id, payment_hash, path: route.paths[0].clone()
                });
 
@@ -1528,7 +1662,7 @@ mod tests {
        fn considers_inflight_htlcs_between_invoice_payments_when_path_succeeds() {
                // First, let's just send a payment through, but only make sure one of the path completes
                let event_handled = core::cell::RefCell::new(false);
-               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+               let event_handler = |_: Event| { *event_handled.borrow_mut() = true; };
 
                let payment_preimage = PaymentPreimage([1; 32]);
                let payment_invoice = invoice(payment_preimage);
@@ -1562,7 +1696,7 @@ mod tests {
 
                // Succeed 1st path, leave 2nd path inflight
                let payment_id = invoice_payer.pay_invoice(&payment_invoice).unwrap();
-               invoice_payer.handle_event(&Event::PaymentPathSuccessful {
+               invoice_payer.handle_event(Event::PaymentPathSuccessful {
                        payment_id, payment_hash, path: route.paths[0].clone()
                });
 
@@ -1579,7 +1713,7 @@ mod tests {
        fn considers_inflight_htlcs_between_retries() {
                // First, let's just send a payment through, but only make sure one of the path completes
                let event_handled = core::cell::RefCell::new(false);
-               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+               let event_handler = |_: Event| { *event_handled.borrow_mut() = true; };
 
                let payment_preimage = PaymentPreimage([1; 32]);
                let payment_invoice = invoice(payment_preimage);
@@ -1620,7 +1754,7 @@ mod tests {
 
                // Fail 1st path, leave 2nd path inflight
                let payment_id = Some(invoice_payer.pay_invoice(&payment_invoice).unwrap());
-               invoice_payer.handle_event(&Event::PaymentPathFailed {
+               invoice_payer.handle_event(Event::PaymentPathFailed {
                        payment_id,
                        payment_hash,
                        network_update: None,
@@ -1632,7 +1766,7 @@ mod tests {
                });
 
                // Fails again the 1st path of our retry
-               invoice_payer.handle_event(&Event::PaymentPathFailed {
+               invoice_payer.handle_event(Event::PaymentPathFailed {
                        payment_id,
                        payment_hash,
                        network_update: None,
@@ -1650,7 +1784,7 @@ mod tests {
        #[test]
        fn accounts_for_some_inflight_htlcs_sent_during_partial_failure() {
                let event_handled = core::cell::RefCell::new(false);
-               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+               let event_handler = |_: Event| { *event_handled.borrow_mut() = true; };
 
                let payment_preimage = PaymentPreimage([1; 32]);
                let invoice_to_pay = invoice(payment_preimage);
@@ -1681,7 +1815,7 @@ mod tests {
        #[test]
        fn accounts_for_all_inflight_htlcs_sent_during_partial_failure() {
                let event_handled = core::cell::RefCell::new(false);
-               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+               let event_handler = |_: Event| { *event_handled.borrow_mut() = true; };
 
                let payment_preimage = PaymentPreimage([1; 32]);
                let invoice_to_pay = invoice(payment_preimage);
@@ -2038,13 +2172,13 @@ mod tests {
                        self
                }
 
-               fn check_attempts(&self) -> Result<PaymentId, PaymentSendFailure> {
+               fn check_attempts(&self) -> Result<(), PaymentSendFailure> {
                        let mut attempts = self.attempts.borrow_mut();
                        *attempts += 1;
 
                        match self.failing_on_attempt.borrow_mut().remove(&*attempts) {
                                Some(failure) => Err(failure),
-                               None => Ok(PaymentId([1; 32])),
+                               None => Ok(())
                        }
                }
 
@@ -2082,15 +2216,15 @@ mod tests {
 
                fn send_payment(
                        &self, route: &Route, _payment_hash: PaymentHash,
-                       _payment_secret: &Option<PaymentSecret>
-               ) -> Result<PaymentId, PaymentSendFailure> {
+                       _payment_secret: &Option<PaymentSecret>, _payment_id: PaymentId,
+               ) -> Result<(), PaymentSendFailure> {
                        self.check_value_msats(Amount::ForInvoice(route.get_total_amount()));
                        self.check_attempts()
                }
 
                fn send_spontaneous_payment(
-                       &self, route: &Route, _payment_preimage: PaymentPreimage,
-               ) -> Result<PaymentId, PaymentSendFailure> {
+                       &self, route: &Route, _payment_preimage: PaymentPreimage, _payment_id: PaymentId,
+               ) -> Result<(), PaymentSendFailure> {
                        self.check_value_msats(Amount::Spontaneous(route.get_total_amount()));
                        self.check_attempts()
                }
@@ -2099,7 +2233,7 @@ mod tests {
                        &self, route: &Route, _payment_id: PaymentId
                ) -> Result<(), PaymentSendFailure> {
                        self.check_value_msats(Amount::OnRetry(route.get_total_amount()));
-                       self.check_attempts().map(|_| ())
+                       self.check_attempts()
                }
 
                fn abandon_payment(&self, _payment_id: PaymentId) { }
@@ -2178,7 +2312,7 @@ mod tests {
                route.paths[1][0].fee_msat = 50_000_000;
                router.expect_find_route(Ok(route.clone()));
 
-               let event_handler = |_: &_| { panic!(); };
+               let event_handler = |_: Event| { panic!(); };
                let invoice_payer = InvoicePayer::new(nodes[0].node, router, nodes[0].logger, event_handler, Retry::Attempts(1));
 
                assert!(invoice_payer.pay_invoice(&create_invoice_from_channelmanager_and_duration_since_epoch(
@@ -2223,7 +2357,7 @@ mod tests {
                route.paths[1][0].fee_msat = 50_000_001;
                router.expect_find_route(Ok(route.clone()));
 
-               let event_handler = |_: &_| { panic!(); };
+               let event_handler = |_: Event| { panic!(); };
                let invoice_payer = InvoicePayer::new(nodes[0].node, router, nodes[0].logger, event_handler, Retry::Attempts(1));
 
                assert!(invoice_payer.pay_invoice(&create_invoice_from_channelmanager_and_duration_since_epoch(
@@ -2300,8 +2434,8 @@ mod tests {
                route.paths.remove(1);
                router.expect_find_route(Ok(route.clone()));
 
-               let expected_events: RefCell<VecDeque<&dyn Fn(&Event)>> = RefCell::new(VecDeque::new());
-               let event_handler = |event: &Event| {
+               let expected_events: RefCell<VecDeque<&dyn Fn(Event)>> = RefCell::new(VecDeque::new());
+               let event_handler = |event: Event| {
                        let event_checker = expected_events.borrow_mut().pop_front().unwrap();
                        event_checker(event);
                };
@@ -2376,7 +2510,7 @@ mod tests {
                // `PaymentPathFailed` being passed up to the user (us, in this case). Previously, we'd
                // treated this as "HTLC complete" and dropped the retry counter, causing us to retry again
                // if the final HTLC failed.
-               expected_events.borrow_mut().push_back(&|ev: &Event| {
+               expected_events.borrow_mut().push_back(&|ev: Event| {
                        if let Event::PaymentPathFailed { payment_failed_permanently, all_paths_failed, .. } = ev {
                                assert!(!payment_failed_permanently);
                                assert!(all_paths_failed);
@@ -2394,13 +2528,13 @@ mod tests {
                nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &bs_fail_update.update_fail_htlcs[0]);
                commitment_signed_dance!(nodes[0], nodes[1], &bs_fail_update.commitment_signed, false, true);
 
-               expected_events.borrow_mut().push_back(&|ev: &Event| {
+               expected_events.borrow_mut().push_back(&|ev: Event| {
                        if let Event::PaymentPathFailed { payment_failed_permanently, all_paths_failed, .. } = ev {
                                assert!(!payment_failed_permanently);
                                assert!(all_paths_failed);
                        } else { panic!("Unexpected event"); }
                });
-               expected_events.borrow_mut().push_back(&|ev: &Event| {
+               expected_events.borrow_mut().push_back(&|ev: Event| {
                        if let Event::PaymentFailed { .. } = ev {
                        } else { panic!("Unexpected event"); }
                });