f - Replace RwLock<S> with S: LockableScore

Author: jkczyz

lightning-invoice/src/payment.rs

@@ -39,6 +39,7 @@
 //! # use lightning_invoice::payment::{InvoicePayer, Payer, RetryAttempts, Router};
 //! # use secp256k1::key::PublicKey;
 //! # use std::ops::Deref;
+//! # use std::sync::Mutex;
 //! #
 //! # struct FakeEventProvider {}
 //! # impl EventsProvider for FakeEventProvider {
@@ -88,9 +89,9 @@
 //! };
 //! # let payer = FakePayer {};
 //! # let router = FakeRouter {};
-//! # let scorer = FakeScorer {};
+//! # let scorer = Mutex::new(FakeScorer {});
 //! # let logger = FakeLogger {};
-//! let invoice_payer = InvoicePayer::new(&payer, router, scorer, &logger, event_handler, RetryAttempts(2));
+//! let invoice_payer = InvoicePayer::new(&payer, router, &scorer, &logger, event_handler, RetryAttempts(2));
 //!
 //! let invoice = "...";
 //! let invoice = invoice.parse::<Invoice>().unwrap();
@@ -117,6 +118,7 @@ use lightning::ln::{PaymentHash, PaymentSecret};
 use lightning::ln::channelmanager::{ChannelDetails, PaymentId, PaymentSendFailure};
 use lightning::ln::msgs::LightningError;
 use lightning::routing;
+use lightning::routing::Score;
 use lightning::routing::router::{Payee, Route, RouteParameters};
 use lightning::util::events::{Event, EventHandler};
 use lightning::util::logger::Logger;
@@ -125,21 +127,21 @@ use secp256k1::key::PublicKey;
 
 use std::collections::hash_map::{self, HashMap};
 use std::ops::Deref;
-use std::sync::{Mutex, RwLock};
+use std::sync::Mutex;
 use std::time::{Duration, SystemTime};
 
 /// A utility for paying [`Invoice]`s.
 pub struct InvoicePayer<P: Deref, R, S, L: Deref, E>
 where
 	P::Target: Payer,
 	R: Router,
-	S: routing::Score,
+	S: for <'a> routing::LockableScore<'a>,
 	L::Target: Logger,
 	E: EventHandler,
 {
 	payer: P,
 	router: R,
-	scorer: RwLock<S>,
+	scorer: S,
 	logger: L,
 	event_handler: E,
 	payment_cache: Mutex<HashMap<PaymentHash, usize>>,
@@ -187,22 +189,11 @@ pub enum PaymentError {
 	Sending(PaymentSendFailure),
 }
 
-/// A read-only version of the scorer.
-pub struct ReadOnlyScorer<'a, S: routing::Score>(std::sync::RwLockReadGuard<'a, S>);
-
-impl<'a, S: routing::Score> Deref for ReadOnlyScorer<'a, S> {
-	type Target = S;
-
-	fn deref(&self) -> &Self::Target {
-		&*self.0
-	}
-}
-
 impl<P: Deref, R, S, L: Deref, E> InvoicePayer<P, R, S, L, E>
 where
 	P::Target: Payer,
 	R: Router,
-	S: routing::Score,
+	S: for <'a> routing::LockableScore<'a>,
 	L::Target: Logger,
 	E: EventHandler,
 {
@@ -216,22 +207,14 @@ where
 		Self {
 			payer,
 			router,
-			scorer: RwLock::new(scorer),
+			scorer,
 			logger,
 			event_handler,
 			payment_cache: Mutex::new(HashMap::new()),
 			retry_attempts,
 		}
 	}
 
-	/// 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) -> ReadOnlyScorer<'_, S> {
-		ReadOnlyScorer(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() {
@@ -278,7 +261,7 @@ where
 					&payer,
 					&params,
 					Some(&first_hops.iter().collect::<Vec<_>>()),
-					&*self.scorer.read().unwrap(),
+					&self.scorer.lock(),
 				).map_err(|e| PaymentError::Routing(e))?;
 
 				let payment_hash = PaymentHash(invoice.payment_hash().clone().into_inner());
@@ -299,7 +282,7 @@ where
 		let first_hops = self.payer.first_hops();
 		let route = self.router.find_route(
 			&payer, &params, Some(&first_hops.iter().collect::<Vec<_>>()),
-			&*self.scorer.read().unwrap()
+			&self.scorer.lock()
 		).map_err(|e| PaymentError::Routing(e))?;
 		self.payer.retry_payment(&route, payment_id).map_err(|e| PaymentError::Sending(e))
 	}
@@ -326,7 +309,7 @@ impl<P: Deref, R, S, L: Deref, E> EventHandler for InvoicePayer<P, R, S, L, E>
 where
 	P::Target: Payer,
 	R: Router,
-	S: routing::Score,
+	S: for <'a> routing::LockableScore<'a>,
 	L::Target: Logger,
 	E: EventHandler,
 {
@@ -336,7 +319,7 @@ where
 				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);
+					self.scorer.lock().payment_path_failed(path, *short_channel_id);
 				}
 
 				let mut payment_cache = self.payment_cache.lock().unwrap();
@@ -468,10 +451,10 @@ mod tests {
 
 		let payer = TestPayer::new();
 		let router = TestRouter {};
-		let scorer = TestScorer::new();
+		let scorer = Mutex::new(TestScorer::new());
 		let logger = TestLogger::new();
 		let invoice_payer =
-			InvoicePayer::new(&payer, router, scorer, &logger, event_handler, RetryAttempts(0));
+			InvoicePayer::new(&payer, router, &scorer, &logger, event_handler, RetryAttempts(0));
 
 		let payment_id = Some(invoice_payer.pay_invoice(&invoice).unwrap());
 		assert_eq!(*payer.attempts.borrow(), 1);
@@ -497,10 +480,10 @@ mod tests {
 			.expect_value_msat(final_value_msat)
 			.expect_value_msat(final_value_msat / 2);
 		let router = TestRouter {};
-		let scorer = TestScorer::new();
+		let scorer = Mutex::new(TestScorer::new());
 		let logger = TestLogger::new();
 		let invoice_payer =
-			InvoicePayer::new(&payer, router, scorer, &logger, event_handler, RetryAttempts(2));
+			InvoicePayer::new(&payer, router, &scorer, &logger, event_handler, RetryAttempts(2));
 
 		let payment_id = Some(invoice_payer.pay_invoice(&invoice).unwrap());
 		assert_eq!(*payer.attempts.borrow(), 1);
@@ -538,10 +521,10 @@ mod tests {
 
 		let payer = TestPayer::new();
 		let router = TestRouter {};
-		let scorer = TestScorer::new();
+		let scorer = Mutex::new(TestScorer::new());
 		let logger = TestLogger::new();
 		let invoice_payer =
-			InvoicePayer::new(&payer, router, scorer, &logger, event_handler, RetryAttempts(2));
+			InvoicePayer::new(&payer, router, &scorer, &logger, event_handler, RetryAttempts(2));
 
 		let payment_id = Some(PaymentId([1; 32]));
 		let event = Event::PaymentPathFailed {
@@ -583,10 +566,10 @@ mod tests {
 			.expect_value_msat(final_value_msat / 2)
 			.expect_value_msat(final_value_msat / 2);
 		let router = TestRouter {};
-		let scorer = TestScorer::new();
+		let scorer = Mutex::new(TestScorer::new());
 		let logger = TestLogger::new();
 		let invoice_payer =
-			InvoicePayer::new(&payer, router, scorer, &logger, event_handler, RetryAttempts(2));
+			InvoicePayer::new(&payer, router, &scorer, &logger, event_handler, RetryAttempts(2));
 
 		let payment_id = Some(invoice_payer.pay_invoice(&invoice).unwrap());
 		assert_eq!(*payer.attempts.borrow(), 1);
@@ -633,10 +616,10 @@ mod tests {
 
 		let payer = TestPayer::new();
 		let router = TestRouter {};
-		let scorer = TestScorer::new();
+		let scorer = Mutex::new(TestScorer::new());
 		let logger = TestLogger::new();
 		let invoice_payer =
-			InvoicePayer::new(&payer, router, scorer, &logger, event_handler, RetryAttempts(2));
+			InvoicePayer::new(&payer, router, &scorer, &logger, event_handler, RetryAttempts(2));
 
 		let payment_preimage = PaymentPreimage([1; 32]);
 		let invoice = invoice(payment_preimage);
@@ -665,10 +648,10 @@ mod tests {
 
 		let payer = TestPayer::new();
 		let router = TestRouter {};
-		let scorer = TestScorer::new();
+		let scorer = Mutex::new(TestScorer::new());
 		let logger = TestLogger::new();
 		let invoice_payer =
-			InvoicePayer::new(&payer, router, scorer, &logger, event_handler, RetryAttempts(2));
+			InvoicePayer::new(&payer, router, &scorer, &logger, event_handler, RetryAttempts(2));
 
 		let payment_preimage = PaymentPreimage([1; 32]);
 		let invoice = expired_invoice(payment_preimage);
@@ -703,10 +686,10 @@ mod tests {
 			.fails_on_attempt(2)
 			.expect_value_msat(final_value_msat);
 		let router = TestRouter {};
-		let scorer = TestScorer::new();
+		let scorer = Mutex::new(TestScorer::new());
 		let logger = TestLogger::new();
 		let invoice_payer =
-			InvoicePayer::new(&payer, router, scorer, &logger, event_handler, RetryAttempts(2));
+			InvoicePayer::new(&payer, router, &scorer, &logger, event_handler, RetryAttempts(2));
 
 		let payment_id = Some(invoice_payer.pay_invoice(&invoice).unwrap());
 		assert_eq!(*payer.attempts.borrow(), 1);
@@ -733,10 +716,10 @@ mod tests {
 
 		let payer = TestPayer::new();
 		let router = TestRouter {};
-		let scorer = TestScorer::new();
+		let scorer = Mutex::new(TestScorer::new());
 		let logger = TestLogger::new();
 		let invoice_payer =
-			InvoicePayer::new(&payer, router, scorer, &logger, event_handler, RetryAttempts(2));
+			InvoicePayer::new(&payer, router, &scorer, &logger, event_handler, RetryAttempts(2));
 
 		let payment_preimage = PaymentPreimage([1; 32]);
 		let invoice = invoice(payment_preimage);
@@ -765,10 +748,10 @@ mod tests {
 
 		let payer = TestPayer::new();
 		let router = TestRouter {};
-		let scorer = TestScorer::new();
+		let scorer = Mutex::new(TestScorer::new());
 		let logger = TestLogger::new();
 		let invoice_payer =
-			InvoicePayer::new(&payer, router, scorer, &logger, event_handler, RetryAttempts(0));
+			InvoicePayer::new(&payer, router, &scorer, &logger, event_handler, RetryAttempts(0));
 
 		let payment_preimage = PaymentPreimage([1; 32]);
 		let invoice = invoice(payment_preimage);
@@ -806,10 +789,10 @@ mod tests {
 	fn fails_paying_invoice_with_routing_errors() {
 		let payer = TestPayer::new();
 		let router = FailingRouter {};
-		let scorer = TestScorer::new();
+		let scorer = Mutex::new(TestScorer::new());
 		let logger = TestLogger::new();
 		let invoice_payer =
-			InvoicePayer::new(&payer, router, scorer, &logger, |_: &_| {}, RetryAttempts(0));
+			InvoicePayer::new(&payer, router, &scorer, &logger, |_: &_| {}, RetryAttempts(0));
 
 		let payment_preimage = PaymentPreimage([1; 32]);
 		let invoice = invoice(payment_preimage);
@@ -824,10 +807,10 @@ mod tests {
 	fn fails_paying_invoice_with_sending_errors() {
 		let payer = TestPayer::new().fails_on_attempt(1);
 		let router = TestRouter {};
-		let scorer = TestScorer::new();
+		let scorer = Mutex::new(TestScorer::new());
 		let logger = TestLogger::new();
 		let invoice_payer =
-			InvoicePayer::new(&payer, router, scorer, &logger, |_: &_| {}, RetryAttempts(0));
+			InvoicePayer::new(&payer, router, &scorer, &logger, |_: &_| {}, RetryAttempts(0));
 
 		let payment_preimage = PaymentPreimage([1; 32]);
 		let invoice = invoice(payment_preimage);
@@ -850,10 +833,10 @@ mod tests {
 
 		let payer = TestPayer::new().expect_value_msat(final_value_msat);
 		let router = TestRouter {};
-		let scorer = TestScorer::new();
+		let scorer = Mutex::new(TestScorer::new());
 		let logger = TestLogger::new();
 		let invoice_payer =
-			InvoicePayer::new(&payer, router, scorer, &logger, event_handler, RetryAttempts(0));
+			InvoicePayer::new(&payer, router, &scorer, &logger, event_handler, RetryAttempts(0));
 
 		let payment_id =
 			Some(invoice_payer.pay_zero_value_invoice(&invoice, final_value_msat).unwrap());
@@ -873,10 +856,10 @@ mod tests {
 
 		let payer = TestPayer::new();
 		let router = TestRouter {};
-		let scorer = TestScorer::new();
+		let scorer = Mutex::new(TestScorer::new());
 		let logger = TestLogger::new();
 		let invoice_payer =
-			InvoicePayer::new(&payer, router, scorer, &logger, event_handler, RetryAttempts(0));
+			InvoicePayer::new(&payer, router, &scorer, &logger, event_handler, RetryAttempts(0));
 
 		let payment_preimage = PaymentPreimage([1; 32]);
 		let invoice = invoice(payment_preimage);
@@ -903,10 +886,10 @@ mod tests {
 
 		let payer = TestPayer::new();
 		let router = TestRouter {};
-		let scorer = TestScorer::new().expect_channel_failure(short_channel_id.unwrap());
+		let scorer = Mutex::new(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));
+			InvoicePayer::new(&payer, router, &scorer, &logger, event_handler, RetryAttempts(2));
 
 		let payment_id = Some(invoice_payer.pay_invoice(&invoice).unwrap());
 		let event = Event::PaymentPathFailed {

lightning/src/routing/mod.rs

@@ -17,6 +17,8 @@ use routing::network_graph::NodeId;
 use routing::router::RouteHop;
 
 use prelude::*;
+use core::ops::{Deref, DerefMut};
+use sync::{Mutex, MutexGuard};
 
 /// An interface used to score payment channels for path finding.
 ///
@@ -29,3 +31,37 @@ pub trait Score {
 	/// Handles updating channel penalties after failing to route through a channel.
 	fn payment_path_failed(&mut self, path: &Vec<RouteHop>, short_channel_id: u64);
 }
+
+/// A scorer that is accessed under a lock.
+///
+/// Needed so that calls to [`Score::channel_penalty_msat`] in [`find_route`] can be made while
+/// having shared ownership of a scorer but without requiring internal locking in [`Score`]
+/// implementations. Internal locking would be detrimental to route finding performance and could
+/// result in [`Score::channel_penalty_msat`] returning a different value for the same channel.
+///
+/// [`find_route`]: crate::routing::router::find_route
+pub trait LockableScore<'a> {
+	/// The locked [`Score`] type.
+	type Locked: 'a + Score;
+
+	/// Returns the locked scorer.
+	fn lock(&'a self) -> Self::Locked;
+}
+
+impl<'a, S: 'a + Score, T: Deref<Target=Mutex<S>>> LockableScore<'a> for T {
+	type Locked = MutexGuard<'a, S>;
+
+	fn lock(&'a self) -> MutexGuard<'a, S> {
+		self.deref().lock().unwrap()
+	}
+}
+
+impl<'a, S: Score> Score for MutexGuard<'a, S> {
+	fn channel_penalty_msat(&self, short_channel_id: u64, source: &NodeId, target: &NodeId) -> u64 {
+		self.deref().channel_penalty_msat(short_channel_id, source, target)
+	}
+
+	fn payment_path_failed(&mut self, path: &Vec<RouteHop>, short_channel_id: u64) {
+		self.deref_mut().payment_path_failed(path, short_channel_id)
+	}
+}