Add utils to persist scorer in BackgroundProcessor #1416

jurvis · 2022-04-10T18:09:33Z

Addresses #1191

Implementation

Added a super-trait WriteableScore that makes LockableScore writeable.
Added a new parameter to BackgroundProcessor.start that takes in our new WriteableScore
Added a new persist_scorer function to Persister, and implemented it with BufWriter (placeholder; to update after Pipe filesystem writes in lightning-persister through BufWriter #1404) is merged

Notes/Questions

(see diff comments)

jurvis · 2022-04-10T18:11:03Z

lightning-persister/src/lib.rs

+			Ok(())
+		}
+	}
+


This is a placeholder implementation I borrowed from combining ideas from the sample and #1404. I will change it after that PR is merged.

I reckon it is probably best to use utils::write_to_file, but that currently requires D to be DiskWriteable, which may conflict with our new trait WriteableScore. Any advice?

You can implement DiskWriteable for any WriteableScore to fix it. DiskWriteable is also removed in #1417 so you can also wait for that to go in first (but I'll let you and @johncantrell97 figure out which order y'all want to land these).

yeah I think you'll want to use utils::write_to_file for this. in my PR I have removed DiskWriteable and updated write_to_file to operate on any object that implements Writeable.

you'll also just need to implement persist_scorer on the blanket implementation of Persister using the new KVStorePersister trait. this just means making sure the scorer is Writeable. at that point it will automatically be implemented using write_to_file for FilesystemPersister.

@johncantrell97 got it, thanks for clarifying!

codecov-commenter · 2022-04-10T18:29:17Z

Codecov Report

Merging #1416 (e06bfdf) into main (62edee5) will increase coverage by 0.99%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##             main    #1416      +/-   ##
==========================================
+ Coverage   90.88%   91.87%   +0.99%     
==========================================
  Files          75       75              
  Lines       41517    47909    +6392     
  Branches    41517    47909    +6392     
==========================================
+ Hits        37734    44018    +6284     
- Misses       3783     3891     +108

Impacted Files	Coverage Δ
lightning/src/routing/scoring.rs	`95.28% <ø> (+0.92%)`	⬆️
lightning-background-processor/src/lib.rs	`94.37% <92.85%> (-0.85%)`	⬇️
lightning/src/util/persist.rs	`94.73% <100.00%> (+0.98%)`	⬆️
lightning/src/util/config.rs	`45.05% <0.00%> (-0.78%)`	⬇️
lightning/src/util/ser.rs	`91.28% <0.00%> (-0.19%)`	⬇️
lightning/src/ln/features.rs	`99.59% <0.00%> (+0.15%)`	⬆️
lightning/src/ln/msgs.rs	`86.66% <0.00%> (+0.29%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`95.96% <0.00%> (+0.40%)`	⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62edee5...e06bfdf. Read the comment docs.

TheBlueMatt

Thanks! Sorry for the review delay - been travelling for Bitcoin Miami.

lightning-background-processor/src/lib.rs

vincenzopalazzo · 2022-04-11T23:23:36Z

lightning-background-processor/src/lib.rs

+			// Persist Scorer on exit
+			persister.persist_scorer(&scorer)?;
+
 			Ok(())


Suggested change

// Persist Scorer on exit

persister.persist_scorer(&scorer)?;

Ok(())

// Persist Scorer on exit

persister.persist_scorer(&scorer)?;

Ok(())

Why the failure is ignored and reported Ok() any way? not able to catch this, sorry

good question. I put this in in #1376 because in the past we always persist channel_manager last and will just simply return the Result from that block.

However, since we added NetworkGraph and Scorer persistence, they run depending on whether or not we can get them from net_graph_msg_handler or scorer. The compiler does not know ahead of time what will exactly run, so it complains if I do not have an Ok(()) there. In other words, Ok() does not run on failure, it just runs once it finishes attempting to persist the channel manager, scorer, and networkgraph, ignoring errors along the way.

Open to any suggestions to improve this, though!

TheBlueMatt · 2022-04-13T14:33:28Z

lightning/src/routing/scoring.rs

+/// use the Persister to persist it.
+pub trait WriteableScore<'a>: LockableScore<'a> {
+	/// Locks the LockableScore and writes it to disk
+	fn write<W: Writer>(&'a self, writer: &mut W) -> Result<(), io::Error>;


Hmm, why not just make WriteableScore extend Writeable instead of having a new write function?

hm, I just assumed from the sample app that we wanted to make sure that the scorer is lockable so we can do .lock().write(). If WriteableScore were to just only extend Writeable, we won't be able to lock it before writing it.

Sure, but the write implementation of the LockableScore will presumably do the locking internally, which is also totally fine.

TheBlueMatt · 2022-04-13T14:35:03Z

lightning-persister/src/lib.rs

+			Ok(())
+		}
+	}
+


You can implement DiskWriteable for any WriteableScore to fix it. DiskWriteable is also removed in #1417 so you can also wait for that to go in first (but I'll let you and @johncantrell97 figure out which order y'all want to land these).

jurvis · 2022-04-13T14:56:34Z

sounds good, thanks for your review. @johncantrell97 it looks like you have some good stuff in that PR so I'll let you merge that in first and build scorer persistence around your changes!

TheBlueMatt · 2022-04-21T19:01:12Z

Needs rebase now that #1417 has landed.

TheBlueMatt

Nice! Thanks. Some relatively minor nits and minor comments.

TheBlueMatt · 2022-04-22T14:25:06Z

lightning/src/routing/scoring.rs

+impl<'a, U: Writeable, T: LockableScore<'a>> WriteableScore<'a> for T
+	where T::Locked: DerefMut<Target=U>
+{
+    fn write<W: Writer>(&'a self, writer: &mut W) -> Result<(), io::Error> {


nit: tabs not spaces.

TheBlueMatt · 2022-04-22T14:25:50Z

lightning/src/routing/scoring.rs

+/// use the Persister to persist it.
+pub trait WriteableScore<'a>: LockableScore<'a> {
+	/// Locks the LockableScore and writes it to disk
+	fn write<W: Writer>(&'a self, writer: &mut W) -> Result<(), io::Error>;


Sure, but the write implementation of the LockableScore will presumably do the locking internally, which is also totally fine.

TheBlueMatt · 2022-04-22T14:30:00Z

lightning/src/routing/scoring.rs

+///
+/// We need this trait to be able to pass in a scorer to `lightning-background-processor` that will enable us to
+/// use the Persister to persist it.
+pub trait WriteableScore<'a>: LockableScore<'a> {


Nice, I think with this we can simplify some of the bindings changes we've been maintaining on previous versions. Don't need to bother with it here but I'll take a look as a followup after we land this.

TheBlueMatt · 2022-04-22T14:31:29Z

lightning-background-processor/src/lib.rs

 			}
+
+			// Persist Scorer on exit
+			if let Some(ref scorer) = scorer {


nit: lets do this before the network graph.

Would it be worth doing the same in the loop?

TheBlueMatt · 2022-04-22T14:31:57Z

lightning-background-processor/src/lib.rs

 		}
+
+		fn with_scorer_error(self, error: std::io::ErrorKind, message: &'static str) -> Self {
+			Self { scorer_error: Some((error, message)), ..self }


It looks like the scorer_error isn't used - need to update the persist method below.

TheBlueMatt · 2022-04-22T14:32:15Z

lightning/src/util/persist.rs

+
+	/// Persist the given [`WriteableScore`] to disk, returning an error if persistence failed.
+	fn persist_scorer(&self, scorer: &'a S) -> Result<(), io::Error> {
+		self.persist("scorer", scorer)


Lets mention the key used here in the docs, sadly that's currently part of the public API ;/.

TheBlueMatt · 2022-04-25T15:56:51Z

lightning/src/routing/scoring.rs

+	where T: LockableScore<'a> + Writeable
+{}
+
+impl<'a, T> LockableScore<'a> for Arc<T>


This can be blanket, no? Instead of implementing it only for Arc we can implement it for any Deref<T> where T: LockableScore and T::Locked: Writeable.

touché, not sure why I didn't think of that

@TheBlueMatt I tried implementing a blanket using what you suggested:

impl<'a, T, S> LockableScore<'a> for S where S: Deref<Target = T>, T: LockableScore<'a>, T::Locked: Writeable { type Locked = T::Locked; fn lock(&'a self) -> Self::Locked { self.deref().lock() } }

but it looks like it may be too generic as it conflicts with our implementation of LockableScore<'a> for Mutex and RefCell a couple of lines below. thoughts?

Ah, yea, lets not go that way - instead lets just make the WriteableScore we pass to the BackgroundProcessor actually a Deref<WriteableScore>, then we can drop both of the Arc implementations. You'll need to have two bounds - one for the WriteableScore and one for the Deref as you'll want to re-use the WriteableScore bound on the `Persister.

TheBlueMatt

Two comments left, then I'm happy :)

lightning/src/util/persist.rs

lightning-background-processor/src/lib.rs

TheBlueMatt

LGTM! Can you rebase and clean up the git history here a bit? You can make it one commit if you really want, but either way lets not clean up previous commits in later ones.

lightning/src/util/persist.rs

jurvis · 2022-05-02T16:24:51Z

@TheBlueMatt fixed and squashed.

either way lets not clean up previous commits in later ones.

I didn't know what you meant by this, but all the commits look like they are squashable so I did it anyways.

TheBlueMatt · 2022-05-02T18:18:25Z

I didn't know what you meant by this, but all the commits look like they are squashable so I did it anyways.

Right, I just meant that you had a commit that added some code (eg the macro), and then a later commit that removed that code.

jkczyz

Looks good! Mostly just some trivial clean-ups / nits.

jkczyz · 2022-05-02T19:00:23Z

lightning/src/util/persist.rs

 		K::Target: 'static + KeysInterface<Signer = Signer>,
 		F::Target: 'static + FeeEstimator,
 		L::Target: 'static + Logger,
+		S: WriteableScore<'a>


nit: add trailing comma

jkczyz · 2022-05-02T19:01:16Z

lightning/src/util/persist.rs

 		K::Target: 'static + KeysInterface<Signer = Signer>,
 		F::Target: 'static + FeeEstimator,
 		L::Target: 'static + Logger,
+		S: WriteableScore<'a>


nit: add trailing comma

jkczyz · 2022-05-02T19:04:48Z

lightning/src/routing/scoring.rs

+impl<'a, T> WriteableScore<'a> for T
+	where T: LockableScore<'a> + Writeable
+{}


This can fit on one line within a 100 characters, which is our aspirational line length limit. 🙂

jkczyz · 2022-05-02T19:05:53Z

lightning-background-processor/src/lib.rs

 		UMH: 'static + Deref + Send + Sync,
 		PM: 'static + Deref<Target = PeerManager<Descriptor, CMH, RMH, L, UMH>> + Send + Sync,
+		S: 'static + Deref<Target = SC> + Send + Sync,
+		SC: WriteableScore<'a>


nit: add trailing comma

jkczyz · 2022-05-02T19:11:40Z

lightning-background-processor/src/lib.rs

 			}
+
+			// Persist Scorer on exit
+			if let Some(ref scorer) = scorer {


Would it be worth doing the same in the loop?

jkczyz · 2022-05-02T19:12:55Z

lightning-background-processor/src/lib.rs

 		graph_error: Option<(std::io::ErrorKind, &'static str)>,
 		manager_error: Option<(std::io::ErrorKind, &'static str)>,
 		filesystem_persister: FilesystemPersister,
+		scorer_error: Option<(std::io::ErrorKind, &'static str)>


Let's move this before the prior field, which feels more logically consistent.

jkczyz · 2022-05-02T19:15:48Z

lightning-background-processor/src/lib.rs

 		let persister = Arc::new(Persister::new(data_dir));
 		let event_handler = |_: &_| {};
-		let bg_processor = BackgroundProcessor::start(persister, event_handler, nodes[0].chain_monitor.clone(), nodes[0].node.clone(), nodes[0].net_graph_msg_handler.clone(), nodes[0].peer_manager.clone(), nodes[0].logger.clone());
+		let scorer = Arc::new(Mutex::new(test_utils::TestScorer::with_penalty(0)));


Here and later tests, if the test doesn't explicitly check scoring, can we pass in None instead? Alternatively, add it the test Node struct so it only needs to be created in one place.

Done. I wasn't able to use just None because the compiler wasn't able to infer the type:

cannot infer type for type parameter S declared on the associated function

so I added None::<Arc<Mutex<FixedPenaltyScorer>>>.

I know FixedPenaltyScorer is deprecated, however, when I tried None::<Arc<Mutex<ProbabilisticScorer<NetworkGraph, TestLogger>>>, the compiler complains:

669 | ...e(), None::<Arc<Mutex<ProbabilisticScorer<NetworkGraph, Logger>>>>); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Deref` is not implemented for `NetworkGraph`

is that a problem?

also, should we create a follow-up issue for updating TestScorer to return a ProbabilisticScorer instead of FixedPenaltyScorer?

Done. I wasn't able to use just None because the compiler wasn't able to infer the type:

cannot infer type for type parameter S declared on the associated function

so I added None::<Arc<Mutex<FixedPenaltyScorer>>>.

Hmm... right, that's a bit verbose. I'd say add a scorer field to Node and do nodes[0].scorer.clone().

I know FixedPenaltyScorer is deprecated, however, when I tried None::<Arc<Mutex<ProbabilisticScorer<NetworkGraph, TestLogger>>>, the compiler complains:

669 | ...e(), None::<Arc<Mutex<ProbabilisticScorer<NetworkGraph, Logger>>>>); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Deref` is not implemented for `NetworkGraph`

is that a problem?

With this approach you would need to use &NetworkGraph or Arc<NetworkGraph> since the type parameter must implement Deref.

also, should we create a follow-up issue for updating TestScorer to return a ProbabilisticScorer instead of FixedPenaltyScorer?

No need. Scorer is deprecated not, FixedPenaltyScorer.

@jkczyz gotcha. I guess if FixedPenaltyScorer is not deprecated I won't change it to ProbabilisticScorer then. Just added the scorer field to Node. should be ready for review again :) thanks!

jkczyz

Looks good modulo one comment.

jkczyz · 2022-05-03T18:34:04Z

lightning-background-processor/src/lib.rs

+		let scorer = Arc::new(Mutex::new(test_utils::TestScorer::with_penalty(0)));
+		let bg_processor = BackgroundProcessor::start(persister, event_handler, nodes[0].chain_monitor.clone(), nodes[0].node.clone(), nodes[0].net_graph_msg_handler.clone(), nodes[0].peer_manager.clone(), nodes[0].logger.clone(), Some(scorer.clone()));


Can use the scorer in Node here now.

oops, thanks for catching that.

jkczyz · 2022-05-03T18:46:27Z

lightning-background-processor/src/lib.rs

 		let persister = Arc::new(Persister::new(data_dir));
 		let event_handler = |_: &_| {};
-		let bg_processor = BackgroundProcessor::start(persister, event_handler, nodes[0].chain_monitor.clone(), nodes[0].node.clone(), nodes[0].net_graph_msg_handler.clone(), nodes[0].peer_manager.clone(), nodes[0].logger.clone());
+		let scorer = Arc::new(Mutex::new(test_utils::TestScorer::with_penalty(0)));


Will want to remove this and replace the usage below now. 🙂

🤦 I'm so sorry -- fixed

TheBlueMatt · 2022-05-03T21:31:53Z

lightning-background-processor/src/lib.rs

 						last_prune_call = Instant::now();
 						have_pruned = true;
 					}
+					if let Some(ref scorer) = scorer {


Oops, this logic is busted if we don't have a net_graph_msg_handler provided (which, okay, should be rare). Just need to move the last_prune_call and have_pruned updates out of the above if. While you're at it, can you add a trace-level log before calling persist_scorer?

Looks like this predates this change. @jurvis Could you make a separate commit for this fix?

sure, I can do that 😄

@jkczyz done

jurvis · 2022-05-03T21:45:18Z

@TheBlueMatt good catch! resolved.

move last_prune_call back

jurvis commented Apr 10, 2022

View reviewed changes

TheBlueMatt reviewed Apr 10, 2022

View reviewed changes

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

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

vincenzopalazzo suggested changes Apr 11, 2022

View reviewed changes

jurvis requested a review from TheBlueMatt April 12, 2022 16:43

jkczyz self-requested a review April 12, 2022 19:43

TheBlueMatt reviewed Apr 13, 2022

View reviewed changes

TheBlueMatt added blocked on dependent pr and removed blocked on dependent pr labels Apr 15, 2022

jurvis force-pushed the jurvis/persist-scorer branch from 5001146 to 76619e6 Compare April 22, 2022 03:49

TheBlueMatt reviewed Apr 22, 2022

View reviewed changes

jurvis force-pushed the jurvis/persist-scorer branch 3 times, most recently from bafd8f8 to 6097f18 Compare April 24, 2022 15:32

TheBlueMatt reviewed Apr 25, 2022

View reviewed changes

jurvis force-pushed the jurvis/persist-scorer branch from 6097f18 to f43c10b Compare April 28, 2022 14:02

TheBlueMatt reviewed May 1, 2022

View reviewed changes

lightning/src/util/persist.rs Outdated Show resolved Hide resolved

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

vincenzopalazzo previously approved these changes May 2, 2022

View reviewed changes

TheBlueMatt reviewed May 2, 2022

View reviewed changes

lightning/src/util/persist.rs Outdated Show resolved Hide resolved

jurvis dismissed vincenzopalazzo’s stale review via 199f46d May 2, 2022 16:24

jurvis force-pushed the jurvis/persist-scorer branch from 52cb1b8 to 199f46d Compare May 2, 2022 16:24

TheBlueMatt previously approved these changes May 2, 2022

View reviewed changes

TheBlueMatt assigned jkczyz May 2, 2022

jkczyz reviewed May 2, 2022

View reviewed changes

jurvis dismissed TheBlueMatt’s stale review via de96726 May 3, 2022 00:25

jurvis force-pushed the jurvis/persist-scorer branch from 199f46d to de96726 Compare May 3, 2022 00:25

jurvis force-pushed the jurvis/persist-scorer branch from de96726 to 434979e Compare May 3, 2022 15:55

jkczyz previously approved these changes May 3, 2022

View reviewed changes

jurvis dismissed jkczyz’s stale review via 42383e4 May 3, 2022 18:39

jurvis force-pushed the jurvis/persist-scorer branch from 434979e to 42383e4 Compare May 3, 2022 18:39

jkczyz reviewed May 3, 2022

View reviewed changes

jurvis force-pushed the jurvis/persist-scorer branch from 42383e4 to 4506b9f Compare May 3, 2022 19:22

jkczyz previously approved these changes May 3, 2022

View reviewed changes

jurvis requested a review from TheBlueMatt May 3, 2022 21:17

TheBlueMatt reviewed May 3, 2022

View reviewed changes

jurvis dismissed jkczyz’s stale review via b7533b4 May 3, 2022 21:44

jurvis force-pushed the jurvis/persist-scorer branch from 4506b9f to b7533b4 Compare May 3, 2022 21:44

jurvis requested review from TheBlueMatt and jkczyz May 3, 2022 21:45

TheBlueMatt previously approved these changes May 3, 2022

View reviewed changes

jurvis added 2 commits May 3, 2022 15:28

Add utils to persist scorer in BackgroundProcessor

5eeb254

move last_prune_call back

Set last_prune_call outside of persistence block

e06bfdf

jurvis dismissed TheBlueMatt’s stale review via e06bfdf May 3, 2022 22:29

jurvis force-pushed the jurvis/persist-scorer branch from b7533b4 to e06bfdf Compare May 3, 2022 22:29

TheBlueMatt approved these changes May 3, 2022

View reviewed changes

jkczyz approved these changes May 4, 2022

View reviewed changes

jkczyz merged commit 2324012 into lightningdevkit:main May 4, 2022

tnull mentioned this pull request Jun 28, 2022

Provide Utilities to Persist NetworkGraph and Scorer #1191

Closed

		let scorer = Arc::new(Mutex::new(test_utils::TestScorer::with_penalty(0)));
		let bg_processor = BackgroundProcessor::start(persister, event_handler, nodes[0].chain_monitor.clone(), nodes[0].node.clone(), nodes[0].net_graph_msg_handler.clone(), nodes[0].peer_manager.clone(), nodes[0].logger.clone(), Some(scorer.clone()));

Add utils to persist scorer in BackgroundProcessor #1416

Add utils to persist scorer in BackgroundProcessor #1416

Uh oh!

Conversation

jurvis commented Apr 10, 2022

Implementation

Notes/Questions

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!

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

Uh oh!

Codecov Report

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!

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!

jurvis commented Apr 13, 2022

Uh oh!

TheBlueMatt commented Apr 21, 2022

Uh oh!

TheBlueMatt left a comment

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!

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!

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!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jurvis commented May 2, 2022

Uh oh!

TheBlueMatt commented May 2, 2022

Uh oh!

jkczyz left a comment

codecov-commenter commented Apr 10, 2022 •

edited

Loading