add another lock to lockable_score #2197

jbesraa · 2023-04-17T18:32:55Z

resolves #2192

lightning/src/routing/scoring.rs

TheBlueMatt · 2023-04-19T03:08:47Z

Looks like CI is sad because the code under cfg(c_bindings) wasn't updated to match the new API.

codecov-commenter · 2023-04-23T09:33:59Z

Codecov Report

Patch coverage: 84.31% and project coverage change: -0.01% ⚠️

Comparison is base (bbe20c3) 90.54% compared to head (c08f2aa) 90.53%.
Report is 2 commits behind head on main.

❗ Current head c08f2aa differs from pull request most recent head 3695b2a. Consider uploading reports for the commit 3695b2a to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2197      +/-   ##
==========================================
- Coverage   90.54%   90.53%   -0.01%     
==========================================
  Files         109      109              
  Lines       57188    57189       +1     
  Branches    57188    57189       +1     
==========================================
- Hits        51780    51777       -3     
- Misses       5408     5412       +4

Files Changed	Coverage Δ
lightning/src/ln/channelmanager.rs	`87.19% <ø> (ø)`
lightning/src/ln/payment_tests.rs	`97.51% <ø> (ø)`
lightning/src/routing/router.rs	`93.98% <42.85%> (+0.37%)`	⬆️
lightning/src/util/test_utils.rs	`73.61% <60.00%> (ø)`
lightning-background-processor/src/lib.rs	`82.15% <100.00%> (-0.53%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`88.86% <100.00%> (ø)`
lightning/src/ln/functional_tests.rs	`98.17% <100.00%> (ø)`
lightning/src/ln/outbound_payment.rs	`89.73% <100.00%> (ø)`
lightning/src/routing/scoring.rs	`93.76% <100.00%> (ø)`

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lightning-background-processor/src/lib.rs

lightning/src/routing/scoring.rs

TheBlueMatt · 2023-05-11T05:30:46Z

Looks like this needs a small rebase as some other changes have landed (which make this much more useful for devs, though), sorry about that!

lightning/src/routing/scoring.rs

lightning/src/routing/router.rs

lightning/src/routing/scoring.rs

TheBlueMatt · 2023-07-08T20:13:35Z

Any desire to pick this back up @jbesraa? It needs a rebase, sadly.

jbesraa · 2023-07-10T08:00:14Z

hey @TheBlueMatt
rebased and fixed the c binding issues

jbesraa · 2023-07-21T18:53:49Z

up^^

TheBlueMatt

Nice thanks! Sorry for the delay while 0.0.116 was getting out the door.

lightning/src/routing/router.rs

lightning-background-processor/src/lib.rs

jbesraa · 2023-08-06T20:47:31Z

@TheBlueMatt @wpaulino this is ready for another review

lightning-background-processor/src/lib.rs

lightning/src/routing/scoring.rs

lightning/src/routing/router.rs

TheBlueMatt

Thanks for working on this, really just one thing we need to figure out.

lightning/src/routing/router.rs

wpaulino

CI needs a small fix for the c_bindings build:

diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs
index 63c466b7..4707bb23 100644
--- a/lightning/src/util/test_utils.rs
+++ b/lightning/src/util/test_utils.rs
@@ -118,8 +118,8 @@ impl<'a> Router for TestRouter<'a> {
 		if let Some((find_route_query, find_route_res)) = self.next_routes.lock().unwrap().pop_front() {
 			assert_eq!(find_route_query, *params);
 			if let Ok(ref route) = find_route_res {
-                                let scorer = &self.scorer.read().unwrap();
-				let scorer = ScorerAccountingForInFlightHtlcs::new(&scorer, &inflight_htlcs);
+				let scorer = &self.scorer.read().unwrap();
+				let scorer = ScorerAccountingForInFlightHtlcs::new(&*scorer, &inflight_htlcs);
 				for path in &route.paths {
 					let mut aggregate_msat = 0u64;
 					for (idx, hop) in path.hops.iter().rev().enumerate() {

lightning-background-processor/src/lib.rs

lightning/src/ln/outbound_payment.rs

lightning/src/routing/scoring.rs

lightning/src/util/test_utils.rs

jbesraa · 2023-08-23T11:47:11Z

@wpaulino thank you so much! made the requested changes and the ci seems to be happier

TheBlueMatt

Basically there! A few pretty small comments, otherwise ACK.

lightning/src/routing/scoring.rs

lightning/src/routing/router.rs

TheBlueMatt · 2023-08-23T18:26:32Z

lightning/src/routing/scoring.rs

+	}
+}
+
+impl<'a, SP:Sized,  T: 'a + ScoreUpdate + ScoreLookUp<ScoreParams = SP>> LockableScore<'a> for RwLock<T> {


This also needs a #[cfg(not(c_bindings))]

because this depends on std?

The bindings don't know anything about std types. While we can manually write wrappers for std types if we wish, its much simpler to write the wrapper here and the bindings can pick up the wrapper type and auto-generate bindings for it.

lightning/src/routing/scoring.rs

TheBlueMatt · 2023-08-24T00:22:13Z

New push LGTM, feel free to squash, @wpaulino usually doesn't mind.

jkczyz · 2023-08-24T13:41:45Z

lightning/src/routing/scoring.rs

@@ -86,8 +86,24 @@ use crate::sync::{Mutex, MutexGuard};
 macro_rules! define_score { ($($supertrait: path)*) => {
 /// An interface used to score payment channels for path finding.
 ///
+///	ScoreUpdate is used to update the scorer's internal state after a payment attempt.


Space not tab after ///. Likewise for ScoreLookUp docs.

jkczyz · 2023-08-24T13:44:31Z

lightning/src/routing/scoring.rs

@@ -86,8 +86,24 @@ use crate::sync::{Mutex, MutexGuard};
 macro_rules! define_score { ($($supertrait: path)*) => {
 /// An interface used to score payment channels for path finding.
 ///
+///	ScoreUpdate is used to update the scorer's internal state after a payment attempt.
+pub trait ScoreUpdate $(: $supertrait)* {


Would move this after ScoreLookUp to make the patchset smaller. Likewise for the impl blocks that follow and for ProbabilisticScorerUsingTime.

jkczyz · 2023-08-24T13:47:21Z

lightning/src/routing/scoring.rs

@@ -86,8 +86,24 @@ use crate::sync::{Mutex, MutexGuard};
 macro_rules! define_score { ($($supertrait: path)*) => {
 /// An interface used to score payment channels for path finding.
 ///
+///	ScoreUpdate is used to update the scorer's internal state after a payment attempt.


Add ticks around identifiers. Likewise for ScoreLookUp.

jkczyz · 2023-08-24T13:51:58Z

lightning/src/util/test_utils.rs

-impl Score for TestScorer {
+impl ScoreUpdate for TestScorer {
+	fn payment_path_failed(&mut self, _actual_path: &Path, _actual_short_channel_id: u64) {}
+
+	fn payment_path_successful(&mut self, _actual_path: &Path) {}
+
+	fn probe_failed(&mut self, _actual_path: &Path, _: u64) {}
+
+	fn probe_successful(&mut self, _actual_path: &Path) {}
+}
+
+impl ScoreLookUp for TestScorer {


Likewise regarding order.

jbesraa · 2023-08-24T17:53:09Z

no code changes, did a reorder and some docs fixes as suggested

jkczyz · 2023-08-24T18:41:05Z

lightning/src/routing/scoring.rs

-impl Score for FixedPenaltyScorer {
-	type ScoreParams = ();
-	fn channel_penalty_msat(&self, _: u64, _: &NodeId, _: &NodeId, _: ChannelUsage, _score_params: &Self::ScoreParams) -> u64 {
-		self.penalty_msat
-	}
-


Ordering here.

jkczyz · 2023-08-24T18:41:23Z

lightning/src/routing/scoring.rs

@@ -1235,50 +1302,7 @@ impl<L: DerefMut<Target = u64>, BRT: DerefMut<Target = HistoricalBucketRangeTrac
 	}
 }

-impl<G: Deref<Target = NetworkGraph<L>>, L: Deref, T: Time> Score for ProbabilisticScorerUsingTime<G, L, T> where L::Target: Logger {


jkczyz · 2023-08-24T18:41:46Z

lightning/src/util/test_utils.rs

@@ -1176,7 +1177,10 @@ impl Score for TestScorer {
 		}
 		0
 	}
+}
+


nit: only one blank line between blocks

jkczyz · 2023-08-24T18:43:00Z

lightning/src/routing/scoring.rs

@@ -117,14 +122,16 @@ pub trait Score $(: $supertrait)* {
 	fn probe_successful(&mut self, path: &Path);
 }

-impl<S: Score, T: DerefMut<Target=S> $(+ $supertrait)*> Score for T {
-	type ScoreParams = S::ScoreParams;
+impl<SP:Sized, S: ScoreLookUp<ScoreParams = SP>, T: Deref<Target=S> $(+ $supertrait)*> ScoreLookUp for T {


nit: space after : for trait bounds

- Split Score from LockableScore to ScoreLookUp to handle read operations and ScoreUpdate to handle write operations - Change all struct that implemented Score to implement ScoreLookUp and/or ScoreUpdate - Change Mutex's to RwLocks to allow multiple data readers - Change LockableScore to Deref in ScorerAccountingForInFlightHtlcs as we only need to read - Add ScoreLookUp and ScoreUpdate docs - Remove reference(&'a) and Sized from Score in ScorerAccountingForInFlightHtlcs as Score implements Deref - Split MultiThreadedScoreLock into MultiThreadedScoreLockWrite and MultiThreadedScoreLockRead. After splitting LockableScore, we split MultiThreadedScoreLock following the same way, splitting a single score into two srtucts, one for read and other for write. MultiThreadedScoreLock is used in c_bindings.

jbesraa · 2023-08-25T02:09:56Z

thanks @jkczyz. pushed the changes

TheBlueMatt · 2023-08-25T18:34:21Z

Please do not unnecessarily rebase onto a newer upstream HEAD (ie rebase onto the same merge commit as you were based on before unless there are conflicts).

TheBlueMatt

Only changes since @wpaulino's ACK at dc95f704ec62f4ec995ae09429977c609d195033 and the current commit are move + whitespace only, plus:

the ScoreUpdate docs changing from:

 /// An interface used to score payment channels for path finding.
-///    Scoring is in terms of fees willing to be paid in order to avoid routing through a channel.
 ///
-///    ScoreUpdate is used to update the scorer's internal state after a payment attempt.
-pub trait ScoreUpdate $(: $supertrait)* {

to

+/// `ScoreUpdate` is used to update the scorer's internal state after a payment attempt.
+pub trait ScoreUpdate $(: $supertrait)* {

and the ScoreLookUp docs changing from:

-///    ScoreLookUp is used to determine the penalty for a given channel.

to

 /// An interface used to score payment channels for path finding.
 ///
+/// `ScoreLookUp` is used to determine the penalty for a given channel.
+///
+/// Scoring is in terms of fees willing to be paid in order to avoid routing through a channel.
 pub trait ScoreLookUp $(: $supertrait)* {

jbesraa mentioned this pull request Apr 17, 2023

Make lockablescore RW #2192

Closed

TheBlueMatt reviewed Apr 17, 2023

View reviewed changes

lightning/src/routing/scoring.rs Outdated Show resolved Hide resolved

jbesraa force-pushed the feat/lockable_score_rw branch from b6f42c4 to 10a4763 Compare April 18, 2023 16:21

jbesraa force-pushed the feat/lockable_score_rw branch from 10a4763 to 6a6ed2b Compare April 23, 2023 07:33

wpaulino reviewed Apr 24, 2023

View reviewed changes

lightning-background-processor/src/lib.rs Outdated Show resolved Hide resolved

lightning/src/routing/scoring.rs Outdated Show resolved Hide resolved

jbesraa force-pushed the feat/lockable_score_rw branch from 6a6ed2b to ee8e824 Compare April 25, 2023 07:01

TheBlueMatt reviewed Apr 27, 2023

View reviewed changes

lightning/src/routing/scoring.rs Outdated Show resolved Hide resolved

lightning/src/routing/scoring.rs Outdated Show resolved Hide resolved

jbesraa force-pushed the feat/lockable_score_rw branch from ee8e824 to d6cdcb6 Compare May 16, 2023 23:02

TheBlueMatt reviewed May 21, 2023

View reviewed changes

lightning/src/routing/scoring.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/scoring.rs Outdated Show resolved Hide resolved

jbesraa force-pushed the feat/lockable_score_rw branch from d6cdcb6 to f12efd0 Compare July 9, 2023 19:54

jbesraa force-pushed the feat/lockable_score_rw branch 3 times, most recently from 3dcdd43 to 334cb06 Compare July 21, 2023 06:06

TheBlueMatt reviewed Jul 24, 2023

View reviewed changes

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Jul 24, 2023

View reviewed changes

lightning-background-processor/src/lib.rs Outdated Show resolved Hide resolved

jbesraa force-pushed the feat/lockable_score_rw branch 3 times, most recently from 4dd4861 to 46fb68d Compare August 6, 2023 18:03

wpaulino reviewed Aug 7, 2023

View reviewed changes

lightning-background-processor/src/lib.rs Outdated Show resolved Hide resolved

lightning/src/routing/scoring.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

jbesraa force-pushed the feat/lockable_score_rw branch 2 times, most recently from 54a3b68 to 3caa653 Compare August 8, 2023 15:43

TheBlueMatt reviewed Aug 8, 2023

View reviewed changes

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

jbesraa force-pushed the feat/lockable_score_rw branch from 3caa653 to 427e7e3 Compare August 15, 2023 09:19

jbesraa force-pushed the feat/lockable_score_rw branch 4 times, most recently from 813c5ff to 5457d8b Compare August 20, 2023 03:55

wpaulino reviewed Aug 21, 2023

View reviewed changes

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

jbesraa force-pushed the feat/lockable_score_rw branch from 5457d8b to 2b98309 Compare August 22, 2023 16:07

wpaulino reviewed Aug 22, 2023

View reviewed changes

jbesraa force-pushed the feat/lockable_score_rw branch from 2b98309 to b099f75 Compare August 23, 2023 10:33

wpaulino previously approved these changes Aug 23, 2023

View reviewed changes

TheBlueMatt reviewed Aug 23, 2023

View reviewed changes

jbesraa dismissed wpaulino’s stale review via e98ad1d August 24, 2023 00:09

jbesraa force-pushed the feat/lockable_score_rw branch 2 times, most recently from e98ad1d to 4347b2a Compare August 24, 2023 00:14

jbesraa force-pushed the feat/lockable_score_rw branch from 4347b2a to c08f2aa Compare August 24, 2023 07:31

jkczyz reviewed Aug 24, 2023

View reviewed changes

jbesraa force-pushed the feat/lockable_score_rw branch from c08f2aa to ad705fb Compare August 24, 2023 17:51

jkczyz reviewed Aug 24, 2023

View reviewed changes

jbesraa force-pushed the feat/lockable_score_rw branch from ad705fb to 3695b2a Compare August 25, 2023 01:35

jkczyz approved these changes Aug 25, 2023

View reviewed changes

TheBlueMatt approved these changes Aug 25, 2023

View reviewed changes

TheBlueMatt merged commit afdcd1c into lightningdevkit:main Aug 25, 2023

@@ @@ -1176,7 +1177,10 @@ impl Score for TestScorer { @@
               		}
 
               	}
+              }

add another lock to lockable_score #2197

add another lock to lockable_score #2197

Uh oh!

Conversation

jbesraa commented Apr 17, 2023

Uh oh!

Uh oh!

TheBlueMatt commented Apr 19, 2023

Uh oh!

codecov-commenter commented Apr 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented May 11, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Jul 8, 2023

Uh oh!

jbesraa commented Jul 10, 2023

Uh oh!

jbesraa commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jbesraa commented Aug 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wpaulino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jbesraa commented Aug 23, 2023

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented Aug 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbesraa commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

codecov-commenter commented Apr 23, 2023 •

edited

Loading

jbesraa commented Jul 21, 2023 •

edited

Loading

jbesraa commented Aug 6, 2023 •

edited

Loading

jbesraa commented Aug 24, 2023 •

edited

Loading