Fix DefaultRouter type restrained to only MutexGuard #2383

henghonglee · 2023-06-28T17:01:45Z

Type of deref mut for default router was specialized to only MutexGuard. It should be generic around RefMut and MutexGuard. This PR fixes that

lightning/src/routing/router.rs

TheBlueMatt · 2023-06-29T04:25:37Z

Hmm, I really hate the dyn and Box indirection here, but I think you're onto something. I think maybe we can make a new concrete D which is a Deref like you have. Does it all work out if you work through an extra generic argument like this:

diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index 787777a9d..0c74318f5 100644
--- a/lightning/src/routing/router.rs
+++ b/lightning/src/routing/router.rs
@@ -33,9 +33,10 @@ use core::{cmp, fmt};
 use core::ops::{Deref,DerefMut};
 
 /// A [`Router`] implemented using [`find_route`].
-pub struct DefaultRouter<G: Deref<Target = NetworkGraph<L>>, L: Deref, S: Deref, SP: Sized, Sc: Score<Sco
reParams = SP>> where
+pub struct DefaultRouter<G: Deref<Target = NetworkGraph<L>>, L: Deref, S: Deref, SP: Sized, D, Sc: Score<
ScoreParams = SP>> where
        L::Target: Logger,
-       S::Target: for <'a> LockableScore<'a, Locked = 'a + DerefMut<Target = Sc>>,
+    for<'a> D: 'a + DerefMut<Target = Sc>,
+       S::Target: for <'a> LockableScore<'a, Locked = D>,
 {
        network_graph: G,
        logger: L,

henghonglee · 2023-06-29T15:56:57Z

for<'a> D: 'a + DerefMut<Target = Sc>,

Hmm. let me try abit more, i kind of get what you mean - to not try to use dyn DerefMut and instead properly wrap that locked functionality with a concrete object thats passed in as a generic

TheBlueMatt · 2023-06-29T20:54:26Z

Yea but sorry the patch missed the required additional Sized bound on D.

lightning-background-processor/src/lib.rs

TheBlueMatt · 2023-07-03T14:39:21Z

Oops, okay, sorry for the runaround, I played with this more and I think I have a working suggestion: instead of this change, make the LockableScore have two type parameters - one explicit Score and one Locked version that must DerefMut to the Score, like this:

pub trait LockableScore<'a> {
    /// The [`Score`] type.
    type Score: 'a + Score;

    /// The locked [`Score`] type.
    type Locked: DerefMut<Target = Self::Score> + Sized;

    /// Returns the locked scorer.
    fn lock(&'a self) -> Self::Locked;
}

then we can still bound the Sc in DefaultRouter without ever referring to the Locked thing -

pub struct DefaultRouter<G: Deref<Target = NetworkGraph<L>>, L: Deref, S: Deref, SP: Sized, Sc: Score<ScoreParams = SP>> where
    L::Target: Logger,
    S::Target: for <'a> LockableScore<'a, Score = Sc>,
{
    network_graph: G,
    logger: L,
    random_seed_bytes: Mutex<[u8; 32]>,
    scorer: S,
    score_params: SP
}

henghonglee · 2023-07-04T04:57:14Z

Oops, okay, sorry for the runaround, I played with this more and I think I have a working suggestion: instead of this change, make the LockableScore have two type parameters - one explicit Score and one Locked version that must DerefMut to the Score, like this:
pub trait LockableScore<'a> {
    /// The [`Score`] type.
    type Score: 'a + Score;

    /// The locked [`Score`] type.
    type Locked: DerefMut<Target = Self::Score> + Sized;

    /// Returns the locked scorer.
    fn lock(&'a self) -> Self::Locked;
}
then we can still bound the Sc in DefaultRouter without ever referring to the Locked thing -
pub struct DefaultRouter<G: Deref<Target = NetworkGraph<L>>, L: Deref, S: Deref, SP: Sized, Sc: Score<ScoreParams = SP>> where
    L::Target: Logger,
    S::Target: for <'a> LockableScore<'a, Score = Sc>,
{
    network_graph: G,
    logger: L,
    random_seed_bytes: Mutex<[u8; 32]>,
    scorer: S,
    score_params: SP
}

This is brilliant! thank you!

henghonglee · 2023-07-04T07:30:28Z

lightning/src/routing/scoring.rs

-#[cfg(c_bindings)]
-impl<'a, T: Score + 'a> Score for MultiThreadedScoreLock<'a, T> {
-	type ScoreParams = <T as Score>::ScoreParams;
-	fn channel_penalty_msat(&self, scid: u64, source: &NodeId, target: &NodeId, usage: ChannelUsage, score_params: &Self::ScoreParams) -> u64 {
-		self.0.channel_penalty_msat(scid, source, target, usage, score_params)
-	}
-	fn payment_path_failed(&mut self, path: &Path, short_channel_id: u64) {
-		self.0.payment_path_failed(path, short_channel_id)
-	}
-	fn payment_path_successful(&mut self, path: &Path) {
-		self.0.payment_path_successful(path)
-	}
-	fn probe_failed(&mut self, path: &Path, short_channel_id: u64) {
-		self.0.probe_failed(path, short_channel_id)
-	}
-	fn probe_successful(&mut self, path: &Path) {
-		self.0.probe_successful(path)
-	}
-}


now that MultiThreadedScoreLock<'a, T> implements DerefMut, this implementation is in conflict with scoring.rs:120-142 and can be removed.

henghonglee · 2023-07-04T07:32:11Z

lightning/src/routing/scoring.rs

+#[cfg(c_bindings)]
+/// A locked `MultiThreadedLockableScore`.
+pub struct MultiThreadedScoreLock<'a, T: Score>(MutexGuard<'a, T>);
+
+#[cfg(c_bindings)]
+impl<'a, T: 'a + Score> Writeable for MultiThreadedScoreLock<'a, T> {
+	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
+		self.0.write(writer)
+	}
+}
+
+#[cfg(c_bindings)]
+impl<'a, T: 'a + Score> DerefMut for MultiThreadedScoreLock<'a, T> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        self.0.deref_mut()
+    }
+}
+
+#[cfg(c_bindings)]
+impl<'a, T: 'a + Score> Deref for MultiThreadedScoreLock<'a, T> {
+	type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        self.0.deref()
+    }
+}
+


rearranged MultiThreadedScoreLock and MultiThreadedLockableScore methods to be colocated since their names are too similar and methods were interweaved

codecov-commenter · 2023-07-04T09:47:48Z

Codecov Report

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

Comparison is base (86fd9e7) 90.32% compared to head (54bcb6e) 90.31%.

❗ 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    #2383      +/-   ##
==========================================
- Coverage   90.32%   90.31%   -0.01%     
==========================================
  Files         106      106              
  Lines       54968    54965       -3     
  Branches    54968    54965       -3     
==========================================
- Hits        49651    49644       -7     
- Misses       5317     5321       +4

Impacted Files	Coverage Δ
lightning-background-processor/src/lib.rs	`82.58% <ø> (ø)`
lightning/src/ln/channelmanager.rs	`86.23% <ø> (-0.03%)`	⬇️
lightning/src/routing/router.rs	`93.55% <100.00%> (-0.01%)`	⬇️
lightning/src/routing/scoring.rs	`94.09% <100.00%> (ø)`
lightning/src/util/test_utils.rs	`72.25% <100.00%> (-0.05%)`	⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

henghonglee · 2023-07-04T10:27:52Z

@TheBlueMatt this one is now ready for review :)

TheBlueMatt

Awesome, thanks so much this is great. Can you add more details to the commit message, what is in the pr text would be fine.

TheBlueMatt · 2023-07-04T14:28:54Z

Since I think this is ready let's make sure we get it for 116.

Type of DerefMut for DefaultRouter was specialized to only MutexGuard. It should be generic around RefMut and MutexGuard. This commit fixes that

henghonglee · 2023-07-05T05:05:21Z

@TheBlueMatt this is good to go i think

henghonglee force-pushed the fix-dyn branch from 09db4f3 to 89ad41a Compare June 29, 2023 02:41

henghonglee commented Jun 29, 2023

View reviewed changes

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

henghonglee commented Jun 29, 2023

View reviewed changes

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

henghonglee force-pushed the fix-dyn branch 2 times, most recently from b512072 to bc2a02c Compare June 30, 2023 04:01

henghonglee commented Jun 30, 2023

View reviewed changes

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

henghonglee force-pushed the fix-dyn branch 3 times, most recently from d6fa5df to 87328f7 Compare July 2, 2023 12:16

henghonglee force-pushed the fix-dyn branch 2 times, most recently from 88edeb1 to 0ab8b3e Compare July 4, 2023 04:55

henghonglee force-pushed the fix-dyn branch from 0ab8b3e to 3b6a738 Compare July 4, 2023 05:07

henghonglee changed the title ~~Fix type restrained to only MutexGuard~~ Fix DefaultRouter type restrained to only MutexGuard Jul 4, 2023

henghonglee force-pushed the fix-dyn branch 4 times, most recently from 8e9383a to 00a8a90 Compare July 4, 2023 07:27

henghonglee commented Jul 4, 2023

View reviewed changes

henghonglee force-pushed the fix-dyn branch from 00a8a90 to 6aeee73 Compare July 4, 2023 08:22

henghonglee marked this pull request as ready for review July 4, 2023 09:25

henghonglee requested a review from TheBlueMatt July 4, 2023 11:27

TheBlueMatt approved these changes Jul 4, 2023

View reviewed changes

TheBlueMatt added this to the 0.0.116 milestone Jul 4, 2023

Fix DefaultRouter type restrained to only MutexGuard

54bcb6e

Type of DerefMut for DefaultRouter was specialized to only MutexGuard. It should be generic around RefMut and MutexGuard. This commit fixes that

henghonglee force-pushed the fix-dyn branch from 6aeee73 to 54bcb6e Compare July 4, 2023 14:31

wpaulino approved these changes Jul 5, 2023

View reviewed changes

wpaulino mentioned this pull request Jul 6, 2023

Make lightning::sync public #2376

Closed

TheBlueMatt assigned TheBlueMatt and wpaulino Jul 6, 2023

TheBlueMatt approved these changes Jul 6, 2023

View reviewed changes

TheBlueMatt merged commit 0d1072b into lightningdevkit:main Jul 6, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix DefaultRouter type restrained to only MutexGuard #2383

Fix DefaultRouter type restrained to only MutexGuard #2383

henghonglee commented Jun 28, 2023 •

edited

TheBlueMatt commented Jun 29, 2023

henghonglee commented Jun 29, 2023

TheBlueMatt commented Jun 29, 2023

TheBlueMatt commented Jul 3, 2023

henghonglee commented Jul 4, 2023

henghonglee Jul 4, 2023 •

edited

henghonglee Jul 4, 2023

codecov-commenter commented Jul 4, 2023 •

edited

henghonglee commented Jul 4, 2023

TheBlueMatt left a comment

TheBlueMatt commented Jul 4, 2023

henghonglee commented Jul 5, 2023

Fix DefaultRouter type restrained to only MutexGuard #2383

Fix DefaultRouter type restrained to only MutexGuard #2383

Conversation

henghonglee commented Jun 28, 2023 • edited

TheBlueMatt commented Jun 29, 2023

henghonglee commented Jun 29, 2023

TheBlueMatt commented Jun 29, 2023

TheBlueMatt commented Jul 3, 2023

henghonglee commented Jul 4, 2023

henghonglee Jul 4, 2023 • edited

Choose a reason for hiding this comment

henghonglee Jul 4, 2023

Choose a reason for hiding this comment

codecov-commenter commented Jul 4, 2023 • edited

Codecov Report

henghonglee commented Jul 4, 2023

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Jul 4, 2023

henghonglee commented Jul 5, 2023

henghonglee commented Jun 28, 2023 •

edited

henghonglee Jul 4, 2023 •

edited

codecov-commenter commented Jul 4, 2023 •

edited