Notify scorer of failing payment path and channel

Upon receiving a PaymentPathFailed event, the failing payment may be
retried on a different path. To avoid using the channel responsible for
the failure, a scorer should be notified of the failure before being
used to find a new route.

Add a payment_path_failed method to routing::Score and call it in
InvoicePayer's event handler.

diff --git a/lightning-invoice/src/payment.rs b/lightning-invoice/src/payment.rs
index f8fd8fcfabe8af03668726a1da04750af7d69875..365d9a2bf7f83445881548b1f976e825d732d835 100644 (file)
--- a/lightning-invoice/src/payment.rs
+++ b/lightning-invoice/src/payment.rs
@@ -32,7 +32,7 @@
 //! # use lightning::ln::msgs::LightningError;
 //! # use lightning::routing;
 //! # use lightning::routing::network_graph::NodeId;
-//! # use lightning::routing::router::{Route, RouteParameters};
+//! # use lightning::routing::router::{Route, RouteHop, RouteParameters};
 //! # use lightning::util::events::{Event, EventHandler, EventsProvider};
 //! # use lightning::util::logger::{Logger, Record};
 //! # use lightning_invoice::Invoice;
@@ -70,6 +70,7 @@
 //! #     fn channel_penalty_msat(
 //! #         &self, _short_channel_id: u64, _source: &NodeId, _target: &NodeId
 //! #     ) -> u64 { 0 }
+//! #     fn payment_path_failed(&mut self, _path: &Vec<RouteHop>, _short_channel_id: u64) {}
 //! # }
 //! #
 //! # struct FakeLogger {};
@@ -124,7 +125,7 @@ use secp256k1::key::PublicKey;
 
 use std::collections::hash_map::{self, HashMap};
 use std::ops::Deref;
-use std::sync::Mutex;
+use std::sync::{Mutex, RwLock};
 use std::time::{Duration, SystemTime};
 
 /// A utility for paying [`Invoice]`s.
@@ -138,7 +139,7 @@ where
 {
        payer: P,
        router: R,
-       scorer: S,
+       scorer: RwLock<S>,
        logger: L,
        event_handler: E,
        payment_cache: Mutex<HashMap<PaymentHash, usize>>,
@@ -204,7 +205,7 @@ where
                Self {
                        payer,
                        router,
-                       scorer,
+                       scorer: RwLock::new(scorer),
                        logger,
                        event_handler,
                        payment_cache: Mutex::new(HashMap::new()),
@@ -212,6 +213,14 @@ where
                }
        }
 
+       /// Returns a read-only reference to the parameterized [`routing::Score`].
+       ///
+       /// Useful if the scorer needs to be persisted. Be sure to drop the returned guard immediately
+       /// after use since retrying failed payment paths require write access.
+       pub fn scorer(&self) -> std::sync::RwLockReadGuard<'_, S> {
+               self.scorer.read().unwrap()
+       }
+
        /// Pays the given [`Invoice`], caching it for later use in case a retry is needed.
        pub fn pay_invoice(&self, invoice: &Invoice) -> Result<PaymentId, PaymentError> {
                if invoice.amount_milli_satoshis().is_none() {
@@ -258,7 +267,7 @@ where
                                        &payer,
                                        &params,
                                        Some(&first_hops.iter().collect::<Vec<_>>()),
-                                       &self.scorer,
+                                       &*self.scorer.read().unwrap(),
                                ).map_err(|e| PaymentError::Routing(e))?;
 
                                let payment_hash = PaymentHash(invoice.payment_hash().clone().into_inner());
@@ -278,7 +287,8 @@ where
                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
+                       &payer, &params, Some(&first_hops.iter().collect::<Vec<_>>()),
+                       &*self.scorer.read().unwrap()
                ).map_err(|e| PaymentError::Routing(e))?;
                self.payer.retry_payment(&route, payment_id).map_err(|e| PaymentError::Sending(e))
        }
@@ -311,7 +321,13 @@ where
 {
        fn handle_event(&self, event: &Event) {
                match event {
-                       Event::PaymentPathFailed { payment_id, payment_hash, rejected_by_dest, retry, .. } => {
+                       Event::PaymentPathFailed {
+                               payment_id, payment_hash, rejected_by_dest, path, short_channel_id, retry, ..
+                       } => {
+                               if let Some(short_channel_id) = short_channel_id {
+                                       self.scorer.write().unwrap().payment_path_failed(path, *short_channel_id);
+                               }
+
                                let mut payment_cache = self.payment_cache.lock().unwrap();
                                let entry = loop {
                                        let entry = payment_cache.entry(*payment_hash);
@@ -862,6 +878,39 @@ mod tests {
                }
        }
 
+       #[test]
+       fn scores_failed_channel() {
+               let event_handled = core::cell::RefCell::new(false);
+               let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+
+               let payment_preimage = PaymentPreimage([1; 32]);
+               let invoice = invoice(payment_preimage);
+               let payment_hash = PaymentHash(invoice.payment_hash().clone().into_inner());
+               let final_value_msat = invoice.amount_milli_satoshis().unwrap();
+               let path = TestRouter::path_for_value(final_value_msat);
+               let short_channel_id = Some(path[0].short_channel_id);
+
+               let payer = TestPayer::new();
+               let router = TestRouter {};
+               let scorer = TestScorer::new().expect_channel_failure(short_channel_id.unwrap());
+               let logger = TestLogger::new();
+               let invoice_payer =
+                       InvoicePayer::new(&payer, router, scorer, &logger, event_handler, RetryAttempts(2));
+
+               let payment_id = Some(invoice_payer.pay_invoice(&invoice).unwrap());
+               let event = Event::PaymentPathFailed {
+                       payment_id,
+                       payment_hash,
+                       network_update: None,
+                       rejected_by_dest: false,
+                       all_paths_failed: false,
+                       path,
+                       short_channel_id,
+                       retry: Some(TestRouter::retry_for_invoice(&invoice)),
+               };
+               invoice_payer.handle_event(&event);
+       }
+
        struct TestRouter;
 
        impl TestRouter {
@@ -933,16 +982,45 @@ mod tests {
                }
        }
 
-       struct TestScorer;
+       struct TestScorer {
+               expectations: std::collections::VecDeque<u64>,
+       }
 
        impl TestScorer {
-               fn new() -> Self { Self {} }
+               fn new() -> Self {
+                       Self {
+                               expectations: std::collections::VecDeque::new(),
+                       }
+               }
+
+               fn expect_channel_failure(mut self, short_channel_id: u64) -> Self {
+                       self.expectations.push_back(short_channel_id);
+                       self
+               }
        }
 
        impl routing::Score for TestScorer {
                fn channel_penalty_msat(
                        &self, _short_channel_id: u64, _source: &NodeId, _target: &NodeId
                ) -> u64 { 0 }
+
+               fn payment_path_failed(&mut self, _path: &Vec<RouteHop>, short_channel_id: u64) {
+                       if let Some(expected_short_channel_id) = self.expectations.pop_front() {
+                               assert_eq!(short_channel_id, expected_short_channel_id);
+                       }
+               }
+       }
+
+       impl Drop for TestScorer {
+               fn drop(&mut self) {
+                       if std::thread::panicking() {
+                               return;
+                       }
+
+                       if !self.expectations.is_empty() {
+                               panic!("Unsatisfied channel failure expectations: {:?}", self.expectations);
+                       }
+               }
        }
 
        struct TestPayer {

diff --git a/lightning/src/routing/mod.rs b/lightning/src/routing/mod.rs
index 51ffd91b50483d58bc2016440bdb98d9f82886e5..42669986015af593e1c851656026e00f2b346233 100644 (file)
--- a/lightning/src/routing/mod.rs
+++ b/lightning/src/routing/mod.rs
@@ -14,6 +14,9 @@ pub mod router;
 pub mod scorer;
 
 use routing::network_graph::NodeId;
+use routing::router::RouteHop;
+
+use prelude::*;
 
 /// An interface used to score payment channels for path finding.
 ///
@@ -22,4 +25,7 @@ pub trait Score {
        /// Returns the fee in msats willing to be paid to avoid routing through the given channel
        /// in the direction from `source` to `target`.
        fn channel_penalty_msat(&self, short_channel_id: u64, source: &NodeId, target: &NodeId) -> u64;
+
+       /// Handles updating channel penalties after failing to route through a channel.
+       fn payment_path_failed(&mut self, path: &Vec<RouteHop>, short_channel_id: u64);
 }

diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index 545ff9f24ce386520ec630dab31f13b75b9c08ae..226dbe04ff9e20e1b89aefc0957032bdd18978d5 100644 (file)
--- a/lightning/src/routing/router.rs
+++ b/lightning/src/routing/router.rs
@@ -4537,6 +4537,8 @@ mod tests {
                fn channel_penalty_msat(&self, short_channel_id: u64, _source: &NodeId, _target: &NodeId) -> u64 {
                        if short_channel_id == self.short_channel_id { u64::max_value() } else { 0 }
                }
+
+               fn payment_path_failed(&mut self, _path: &Vec<RouteHop>, _short_channel_id: u64) {}
        }
 
        struct BadNodeScorer {
@@ -4547,6 +4549,8 @@ mod tests {
                fn channel_penalty_msat(&self, _short_channel_id: u64, _source: &NodeId, target: &NodeId) -> u64 {
                        if *target == self.node_id { u64::max_value() } else { 0 }
                }
+
+               fn payment_path_failed(&mut self, _path: &Vec<RouteHop>, _short_channel_id: u64) {}
        }
 
        #[test]

diff --git a/lightning/src/routing/scorer.rs b/lightning/src/routing/scorer.rs
index e3f5c8679b68d6b57ca418b70befdc0ff34e35d7..01481f16c297f9218aa05082b3fbdc7f22e886ba 100644 (file)
--- a/lightning/src/routing/scorer.rs
+++ b/lightning/src/routing/scorer.rs
@@ -45,6 +45,9 @@
 use routing;
 
 use routing::network_graph::NodeId;
+use routing::router::RouteHop;
+
+use prelude::*;
 
 /// [`routing::Score`] implementation that provides reasonable default behavior.
 ///
@@ -78,4 +81,6 @@ impl routing::Score for Scorer {
        ) -> u64 {
                self.base_penalty_msat
        }
+
+       fn payment_path_failed(&mut self, _path: &Vec<RouteHop>, _short_channel_id: u64) {}
 }