Offer parsing tests
[rust-lightning] / lightning-invoice / src / payment.rs
index 4b2a682b3fa2142bb5b491a0010e835a5e3c5728..96b20958fb7453517a085dec9bae44a29a2f6fb0 100644 (file)
 //! # use lightning::ln::channelmanager::{ChannelDetails, PaymentId, PaymentSendFailure};
 //! # use lightning::ln::msgs::LightningError;
 //! # use lightning::routing::gossip::NodeId;
-//! # use lightning::routing::router::{Route, RouteHop, RouteParameters};
+//! # use lightning::routing::router::{InFlightHtlcs, Route, RouteHop, RouteParameters, Router};
 //! # use lightning::routing::scoring::{ChannelUsage, Score};
 //! # use lightning::util::events::{Event, EventHandler, EventsProvider};
 //! # use lightning::util::logger::{Logger, Record};
 //! # use lightning::util::ser::{Writeable, Writer};
 //! # use lightning_invoice::Invoice;
-//! # use lightning_invoice::payment::{InFlightHtlcs, InvoicePayer, Payer, Retry, Router};
+//! # use lightning_invoice::payment::{InvoicePayer, Payer, Retry, ScoringRouter};
 //! # use secp256k1::PublicKey;
 //! # use std::cell::RefCell;
 //! # use std::ops::Deref;
 //! #     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!() }
 //! # struct FakeRouter {}
 //! # impl Router for FakeRouter {
 //! #     fn find_route(
-//! #         &self, payer: &PublicKey, params: &RouteParameters, payment_hash: &PaymentHash,
+//! #         &self, payer: &PublicKey, params: &RouteParameters,
 //! #         first_hops: Option<&[&ChannelDetails]>, _inflight_htlcs: InFlightHtlcs
 //! #     ) -> Result<Route, LightningError> { unimplemented!() }
-//! #
+//! # }
+//! # impl ScoringRouter for FakeRouter {
 //! #     fn notify_payment_path_failed(&self, path: &[&RouteHop], short_channel_id: u64) {  unimplemented!() }
 //! #     fn notify_payment_path_successful(&self, path: &[&RouteHop]) {  unimplemented!() }
 //! #     fn notify_payment_probe_successful(&self, path: &[&RouteHop]) {  unimplemented!() }
@@ -140,16 +142,14 @@ use bitcoin_hashes::Hash;
 use bitcoin_hashes::sha256::Hash as Sha256;
 
 use crate::prelude::*;
-use lightning::io;
 use lightning::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
 use lightning::ln::channelmanager::{ChannelDetails, PaymentId, PaymentSendFailure};
 use lightning::ln::msgs::LightningError;
 use lightning::routing::gossip::NodeId;
-use lightning::routing::router::{PaymentParameters, Route, RouteHop, RouteParameters};
+use lightning::routing::router::{InFlightHtlcs, PaymentParameters, Route, RouteHop, RouteParameters, Router};
 use lightning::util::errors::APIError;
 use lightning::util::events::{Event, EventHandler};
 use lightning::util::logger::Logger;
-use lightning::util::ser::Writeable;
 use crate::time_utils::Time;
 use crate::sync::Mutex;
 
@@ -177,7 +177,7 @@ use crate::time_utils;
 type ConfiguredTime = time_utils::Eternity;
 
 /// (C-not exported) generally all users should use the [`InvoicePayer`] type alias.
-pub struct InvoicePayerUsingTime<P: Deref, R: Router, L: Deref, E: EventHandler, T: Time>
+pub struct InvoicePayerUsingTime<P: Deref, R: ScoringRouter, L: Deref, E: EventHandler, T: Time>
 where
        P::Target: Payer,
        L::Target: Logger,
@@ -242,6 +242,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;
@@ -251,13 +263,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>;
@@ -266,13 +279,20 @@ pub trait Payer {
        fn abandon_payment(&self, payment_id: PaymentId);
 }
 
-/// A trait defining behavior for routing an [`Invoice`] payment.
-pub trait Router {
-       /// Finds a [`Route`] between `payer` and `payee` for a payment with the given values.
-       fn find_route(
-               &self, payer: &PublicKey, route_params: &RouteParameters, payment_hash: &PaymentHash,
-               first_hops: Option<&[&ChannelDetails]>, inflight_htlcs: InFlightHtlcs
-       ) -> Result<Route, LightningError>;
+/// A trait defining behavior for a [`Router`] implementation that also supports scoring channels
+/// based on payment and probe success/failure.
+///
+/// [`Router`]: lightning::routing::router::Router
+pub trait ScoringRouter: Router {
+       /// Finds a [`Route`] between `payer` and `payee` for a payment with the given values. Includes
+       /// `PaymentHash` and `PaymentId` to be able to correlate the request with a specific payment.
+       fn find_route_with_id(
+               &self, payer: &PublicKey, route_params: &RouteParameters,
+               first_hops: Option<&[&ChannelDetails]>, inflight_htlcs: InFlightHtlcs,
+               _payment_hash: PaymentHash, _payment_id: PaymentId
+       ) -> Result<Route, LightningError> {
+               self.find_route(payer, route_params, first_hops, inflight_htlcs)
+       }
        /// Lets the router know that payment through a specific path has failed.
        fn notify_payment_path_failed(&self, path: &[&RouteHop], short_channel_id: u64);
        /// Lets the router know that payment through a specific path was successful.
@@ -322,7 +342,7 @@ pub enum PaymentError {
        Sending(PaymentSendFailure),
 }
 
-impl<P: Deref, R: Router, L: Deref, E: EventHandler, T: Time> InvoicePayerUsingTime<P, R, L, E, T>
+impl<P: Deref, R: ScoringRouter, L: Deref, E: EventHandler, T: Time> InvoicePayerUsingTime<P, R, L, E, T>
 where
        P::Target: Payer,
        L::Target: Logger,
@@ -346,36 +366,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());
@@ -398,7 +458,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)
@@ -408,13 +468,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()),
@@ -427,15 +515,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));
@@ -447,16 +535,15 @@ where
                let first_hops = self.payer.first_hops();
                let inflight_htlcs = self.create_inflight_map();
                let route = self.router.find_route(
-                       &payer, &params, &payment_hash, Some(&first_hops.iter().collect::<Vec<_>>()),
-                       inflight_htlcs
+                       &payer, &params, Some(&first_hops.iter().collect::<Vec<_>>()), inflight_htlcs
                ).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),
@@ -491,13 +578,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(())
                                        }
                                },
                        },
@@ -552,8 +639,7 @@ where
                let inflight_htlcs = self.create_inflight_map();
 
                let route = self.router.find_route(
-                       &payer, &params, &payment_hash, Some(&first_hops.iter().collect::<Vec<_>>()),
-                       inflight_htlcs
+                       &payer, &params, Some(&first_hops.iter().collect::<Vec<_>>()), inflight_htlcs
                );
 
                if route.is_err() {
@@ -606,9 +692,8 @@ where
                self.payment_cache.lock().unwrap().remove(payment_hash);
        }
 
-       /// Given a [`PaymentHash`], this function looks up inflight path attempts in the payment_cache.
-       /// Then, it uses the path information inside the cache to construct a HashMap mapping a channel's
-       /// short channel id and direction to the amount being sent through it.
+       /// Use path information in the payment_cache to construct a HashMap mapping a channel's short
+       /// channel id and direction to the amount being sent through it.
        ///
        /// This function should be called whenever we need information about currently used up liquidity
        /// across payments.
@@ -644,7 +729,7 @@ where
                        }
                }
 
-               InFlightHtlcs(total_inflight_map)
+               InFlightHtlcs::new(total_inflight_map)
        }
 }
 
@@ -659,7 +744,7 @@ fn has_expired(route_params: &RouteParameters) -> bool {
        } else { false }
 }
 
-impl<P: Deref, R: Router, L: Deref, E: EventHandler, T: Time> EventHandler for InvoicePayerUsingTime<P, R, L, E, T>
+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,
@@ -733,31 +818,6 @@ where
        }
 }
 
-/// A map with liquidity value (in msat) keyed by a short channel id and the direction the HTLC
-/// is traveling in. The direction boolean is determined by checking if the HTLC source's public
-/// key is less than its destination. See [`InFlightHtlcs::used_liquidity_msat`] for more
-/// details.
-pub struct InFlightHtlcs(HashMap<(u64, bool), u64>);
-
-impl InFlightHtlcs {
-       /// Returns liquidity in msat given the public key of the HTLC source, target, and short channel
-       /// id.
-       pub fn used_liquidity_msat(&self, source: &NodeId, target: &NodeId, channel_scid: u64) -> Option<u64> {
-               self.0.get(&(channel_scid, source < target)).map(|v| *v)
-       }
-}
-
-impl Writeable for InFlightHtlcs {
-       fn write<W: lightning::util::ser::Writer>(&self, writer: &mut W) -> Result<(), io::Error> { self.0.write(writer) }
-}
-
-impl lightning::util::ser::Readable for InFlightHtlcs {
-       fn read<R: io::Read>(reader: &mut R) -> Result<Self, lightning::ln::msgs::DecodeError> {
-               let infight_map: HashMap<(u64, bool), u64> = lightning::util::ser::Readable::read(reader)?;
-               Ok(Self(infight_map))
-       }
-}
-
 #[cfg(test)]
 mod tests {
        use super::*;
@@ -770,7 +830,7 @@ mod tests {
        use lightning::ln::functional_test_utils::*;
        use lightning::ln::msgs::{ChannelMessageHandler, ErrorAction, LightningError};
        use lightning::routing::gossip::{EffectiveCapacity, NodeId};
-       use lightning::routing::router::{PaymentParameters, Route, RouteHop};
+       use lightning::routing::router::{InFlightHtlcs, PaymentParameters, Route, RouteHop, Router};
        use lightning::routing::scoring::{ChannelUsage, LockableScore, Score};
        use lightning::util::test_utils::TestLogger;
        use lightning::util::errors::APIError;
@@ -1813,7 +1873,7 @@ mod tests {
 
        impl Router for TestRouter {
                fn find_route(
-                       &self, payer: &PublicKey, route_params: &RouteParameters, _payment_hash: &PaymentHash,
+                       &self, payer: &PublicKey, route_params: &RouteParameters,
                        _first_hops: Option<&[&ChannelDetails]>, inflight_htlcs: InFlightHtlcs
                ) -> Result<Route, LightningError> {
                        // Simulate calling the Scorer just as you would in find_route
@@ -1844,7 +1904,9 @@ mod tests {
                                payment_params: Some(route_params.payment_params.clone()), ..Self::route_for_value(route_params.final_value_msat)
                        })
                }
+       }
 
+       impl ScoringRouter for TestRouter {
                fn notify_payment_path_failed(&self, path: &[&RouteHop], short_channel_id: u64) {
                        self.scorer.lock().payment_path_failed(path, short_channel_id);
                }
@@ -1866,12 +1928,14 @@ mod tests {
 
        impl Router for FailingRouter {
                fn find_route(
-                       &self, _payer: &PublicKey, _params: &RouteParameters, _payment_hash: &PaymentHash,
-                       _first_hops: Option<&[&ChannelDetails]>, _inflight_htlcs: InFlightHtlcs
+                       &self, _payer: &PublicKey, _params: &RouteParameters, _first_hops: Option<&[&ChannelDetails]>,
+                       _inflight_htlcs: InFlightHtlcs,
                ) -> Result<Route, LightningError> {
                        Err(LightningError { err: String::new(), action: ErrorAction::IgnoreError })
                }
+       }
 
+       impl ScoringRouter for FailingRouter {
                fn notify_payment_path_failed(&self, _path: &[&RouteHop], _short_channel_id: u64) {}
 
                fn notify_payment_path_successful(&self, _path: &[&RouteHop]) {}
@@ -2056,13 +2120,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(())
                        }
                }
 
@@ -2100,15 +2164,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()
                }
@@ -2117,7 +2181,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) { }
@@ -2128,12 +2192,13 @@ mod tests {
 
        impl Router for ManualRouter {
                fn find_route(
-                       &self, _payer: &PublicKey, _params: &RouteParameters, _payment_hash: &PaymentHash,
-                       _first_hops: Option<&[&ChannelDetails]>, _inflight_htlcs: InFlightHtlcs
+                       &self, _payer: &PublicKey, _params: &RouteParameters, _first_hops: Option<&[&ChannelDetails]>,
+                       _inflight_htlcs: InFlightHtlcs
                ) -> Result<Route, LightningError> {
                        self.0.borrow_mut().pop_front().unwrap()
                }
-
+       }
+       impl ScoringRouter for ManualRouter {
                fn notify_payment_path_failed(&self, _path: &[&RouteHop], _short_channel_id: u64) {}
 
                fn notify_payment_path_successful(&self, _path: &[&RouteHop]) {}