From 0cc67fbb2f5eaf92bab61c8e562a115d341ebed8 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Fri, 22 Oct 2021 13:02:26 -0500 Subject: [PATCH] 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. --- lightning-invoice/src/payment.rs | 96 +++++++++++++++++++++++++++++--- lightning/src/routing/mod.rs | 6 ++ lightning/src/routing/router.rs | 4 ++ lightning/src/routing/scorer.rs | 5 ++ 4 files changed, 102 insertions(+), 9 deletions(-) diff --git a/lightning-invoice/src/payment.rs b/lightning-invoice/src/payment.rs index f8fd8fcfa..365d9a2bf 100644 --- 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, _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, logger: L, event_handler: E, payment_cache: Mutex>, @@ -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 { if invoice.amount_milli_satoshis().is_none() { @@ -258,7 +267,7 @@ where &payer, ¶ms, Some(&first_hops.iter().collect::>()), - &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, ¶ms, Some(&first_hops.iter().collect::>()), &self.scorer + &payer, ¶ms, Some(&first_hops.iter().collect::>()), + &*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, + } 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, 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 51ffd91b5..426699860 100644 --- 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, short_channel_id: u64); } diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 545ff9f24..226dbe04f 100644 --- 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, _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, _short_channel_id: u64) {} } #[test] diff --git a/lightning/src/routing/scorer.rs b/lightning/src/routing/scorer.rs index e3f5c8679..01481f16c 100644 --- 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, _short_channel_id: u64) {} } -- 2.39.5