Scorer serialization #1146

jkczyz · 2021-10-29T16:04:28Z

Scorer should be serialized to retain penalty data between restarts. Implement (de)serialization for Scorer by serializing last failure times as duration since the UNIX epoch. For no-std, the zero-Duration is used.

Parameterize Scorer with a new Clock trait to simplify no-std support and allow for tesing without using a real clock.

Based on #1144.

TODO: Test Scorer now that it can be parameterized by a fake Clock.

codecov · 2021-10-29T16:19:26Z

Codecov Report

Merging #1146 (73ffbcf) into main (6e86776) will decrease coverage by 0.06%.
The diff coverage is 51.19%.

❗ Current head 73ffbcf differs from pull request most recent head ae210e7. Consider uploading reports for the commit ae210e7 to get more accurate results

@@            Coverage Diff             @@
##             main    #1146      +/-   ##
==========================================
- Coverage   90.22%   90.15%   -0.07%     
==========================================
  Files          70       70              
  Lines       36361    36224     -137     
==========================================
- Hits        32805    32658     -147     
- Misses       3556     3566      +10

Impacted Files	Coverage Δ
lightning/src/util/ser.rs	`88.14% <0.00%> (-0.88%)`	⬇️
lightning/src/util/test_utils.rs	`81.91% <ø> (ø)`
lightning/src/routing/scorer.rs	`34.42% <7.89%> (-17.96%)`	⬇️
lightning/src/routing/router.rs	`91.98% <93.33%> (ø)`
lightning-background-processor/src/lib.rs	`94.97% <100.00%> (+0.02%)`	⬆️
lightning-invoice/src/utils.rs	`74.52% <100.00%> (-8.96%)`	⬇️
lightning/src/ln/channelmanager.rs	`83.82% <100.00%> (+0.09%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`95.14% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.29% <100.00%> (-0.09%)`	⬇️
lightning/src/ln/shutdown_tests.rs	`95.89% <100.00%> (+0.01%)`	⬆️
... and 10 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 6e86776...ae210e7. Read the comment docs.

TheBlueMatt · 2021-10-29T18:42:14Z

I'm not really sure if I buy that we care about Scorer for no-std at all, let alone ensuring custom times work in no-std for it. If you're in a no-std platform you probably aren't storing the full network graph and routing anyway. Does it make sense to just implement the serialization against times with no-std?

jkczyz · 2021-10-29T19:12:40Z

I'm not really sure if I buy that we care about Scorer for no-std at all, let alone ensuring custom times work in no-std for it. If you're in a no-std platform you probably aren't storing the full network graph and routing anyway. Does it make sense to just implement the serialization against times with no-std?

I made Scorer not look parameterized to users in order to address the rust doc failures.

I'm gonna need the parameterization to test this, though, so there really isn't much extra needed to support no-std. I think it's mostly just one line of code induration_since_epoch plus AlwaysPresent, the latter of which is used in TestScorer.

TheBlueMatt · 2021-10-29T19:21:14Z

lightning/src/routing/scorer.rs

+	fn elapsed(&self) -> Duration;
+}
+
+impl<C: Clock + Sub<Duration, Output = C>> ScorerUsingClock<C> {


Why not just make the Sub requirement a direct parent of Clock?

Good point. Done!

lightning/src/routing/scorer.rs

valentinewallace · 2021-11-01T20:02:11Z

lightning/src/routing/scorer.rs

+	#[inline]
+	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
+		self.undecayed_penalty_msat.write(w)?;
+		(duration_since_epoch() - self.last_failed.elapsed()).write(w)


Hmm, I wonder if we should document somewhere that an LDK node serialized by std should never be deserialized by no-std and vice versa. Seems like some weird negative durations could occur here in those situations

Updated docs about mixing types being undefined. Also, moved duration_since_epoch into Time trait to make the association clearer.

valentinewallace

Shaping up! Just these last few comments I think.

valentinewallace · 2021-11-01T20:22:47Z

lightning/src/routing/scorer.rs

-	channel_failures: HashMap<u64, (u64, Instant)>,
-	#[cfg(feature = "no-std")]
-	channel_failures: HashMap<u64, u64>,
+	channel_failures: HashMap<u64, ChannelFailure<C>>,


Wondering if we'll need a mutex/multi-threaded access to channel_failures if we're gonna be persisting it in the background

No need to change anything since the scorer will be accessed using a LockableScore (e.g., a Mutex).

valentinewallace · 2021-11-01T20:26:18Z

lightning/src/routing/scorer.rs

-	channel_failures: HashMap<u64, (u64, Instant)>,
-	#[cfg(feature = "no-std")]
-	channel_failures: HashMap<u64, u64>,
+	channel_failures: HashMap<u64, ChannelFailure<C>>,


Seems self.channel_failures may grow over time, may be nice to have a TODO for pruning it

Good observation. I suppose a scorer should be notified of changes to the NetworkGraph to allow for this. Added a TODO.

valentinewallace · 2021-11-01T21:17:42Z

lightning/src/routing/scorer.rs

+/// A clock that is always present, now or after time has passed.
+pub struct AlwaysPresent;
+
+impl Clock for AlwaysPresent {


IMO Instant would be clearer than Clock for this API, though not sure what ScorerUsingClock would be renamed to 🤔

Renamed Clock to Time which is more consistent with how SystemTime and Instant are described. Also, renamed AlwaysPresent to Eternity (i.e., where time has no meaning) since "present" in the former could have been read with a different meaning (e.g., a field that is always present).

This also will work better with testing where time will be read from a Clock, which the tests will advance as needed.

valentinewallace

LGTM!

I think a new warning may have been introduced ("unused variable: network_graph")

valentinewallace · 2021-11-02T00:59:48Z

lightning/src/routing/scorer.rs

 	fn now() -> Self;

-	/// Returns the amount of time elapsed since the clock was created.
+	/// Returns the amount of time elapsed since created.


nit: prefer slightly more explicit "since this Time instance was created"

Used the less verbose self.

valentinewallace · 2021-11-02T02:01:07Z

lightning/src/routing/scorer.rs

+	/// # Note
+	///
+	/// When time is an [`Eternity`], as is default when enabling feature `no-std`, it will never
+	/// elapse. Therefore, this penalty will never decay.


nit: s/this penalty/failure_penalty_msat

That wouldn't be accurate since failure_penalty_msat is added each time the channel fails to relay a payment, i.e., "this penalty" is referring to "any accumulated channel failure penalties". I've re-worded a bit and included a link to failure_penalty_msat.

TheBlueMatt

Can you squash?

Move channel failure penalty logic into a ChannelFailure abstraction. This encapsulates the logic for accumulating penalties and decaying them over time. It also is responsible for the no-std behavior. This cleans up Scorer and will make it easier to serialize it.

Scorer uses time to determine how much to penalize a channel after a failure occurs. Parameterizing it by time cleans up the code such that no-std support is in a single AlwaysPresent struct, which implements the Time trait. Time is implemented for std::time::Instant when std is available. This parameterization also allows for deterministic testing since a clock could be devised to advance forward as needed.

Scorer should be serialized to retain penalty data between restarts. Implement (de)serialization for Scorer by serializing last failure times as duration since the UNIX epoch. For no-std, the zero-Duration is used.

TheBlueMatt · 2021-11-02T20:19:42Z

Squashed without diff, will land after CI:

$ git diff-tree -U1 6f9c9e5ec400 ae210e7
$

TheBlueMatt added this to the 0.0.103 milestone Oct 29, 2021

jkczyz mentioned this pull request Oct 29, 2021

Penalize failed channels #1144

Merged

jkczyz force-pushed the 2021-10-score-serialization branch from bc516ff to fc1323b Compare October 29, 2021 19:05

TheBlueMatt reviewed Oct 29, 2021

View reviewed changes

jkczyz force-pushed the 2021-10-score-serialization branch from fc1323b to d465e4a Compare October 29, 2021 20:26

jkczyz mentioned this pull request Oct 30, 2021

Add default implementation for tracking metadata #636

Closed

valentinewallace self-requested a review November 1, 2021 16:51

valentinewallace reviewed Nov 1, 2021

View reviewed changes

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

lightning/src/routing/scorer.rs Show resolved Hide resolved

valentinewallace reviewed Nov 1, 2021

View reviewed changes

jkczyz force-pushed the 2021-10-score-serialization branch from 5713d28 to 73ffbcf Compare November 1, 2021 23:42

Clarify Scorer docs around penalizing channels

1c2b3c2

jkczyz force-pushed the 2021-10-score-serialization branch from 73ffbcf to 960854c Compare November 2, 2021 02:01

valentinewallace approved these changes Nov 2, 2021

View reviewed changes

jkczyz force-pushed the 2021-10-score-serialization branch from 960854c to 6f9c9e5 Compare November 2, 2021 16:24

TheBlueMatt approved these changes Nov 2, 2021

View reviewed changes

jkczyz added 3 commits November 2, 2021 14:48

Implement (de)serialization for Scorer

ae210e7

Scorer should be serialized to retain penalty data between restarts. Implement (de)serialization for Scorer by serializing last failure times as duration since the UNIX epoch. For no-std, the zero-Duration is used.

jkczyz force-pushed the 2021-10-score-serialization branch from 6f9c9e5 to ae210e7 Compare November 2, 2021 19:49

TheBlueMatt merged commit 094ddb2 into lightningdevkit:main Nov 2, 2021

Uh oh!

Scorer serialization #1146

Scorer serialization #1146

Uh oh!

Conversation

jkczyz commented Oct 29, 2021

Uh oh!

codecov bot commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

TheBlueMatt commented Oct 29, 2021

Uh oh!

jkczyz commented Oct 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace 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!

valentinewallace 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!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Nov 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Oct 29, 2021 •

edited

Loading