[#2189] Score Fee Params as a passed in parameter #2237

henghonglee · 2023-04-26T21:35:02Z

[#2189] Score Fee Params as a passed in parameter

Description:
Aims to fix https://github.com/lightningdevkit/rust-lightning/issues/2189

1. Introduce ScoreParams to Score Trait, allowing channel penalty calculations to have custom parameters
2. Split up ProbablisticScoreParamters into FeeParameters and DecayParameters
3. Couple tightly decay_params but decouple fee params so that modifying and customizing fee parameters can be more flexible.
4. Propagate changes to affect get_route and find_route in the router
5. Fix tests, put placeholder values on the various test scorers

TheBlueMatt

Thanks for working on this!

lightning/src/routing/router.rs

TheBlueMatt · 2023-04-28T21:22:55Z

Needs rebase, though it looks like you just have some commits that made their way upstream on this branch?

henghonglee

Hi @TheBlueMatt, Ive tried to list down the issues im facing here. tried a few different variations of this solution but ive rolled back to this one since its the simplest and imo closest to what i want to achieve

lightning/src/routing/router.rs

lightning-background-processor/src/lib.rs

henghonglee

@TheBlueMatt could you also take a quick look at the other comments that i made? in router.rs

lightning-background-processor/src/lib.rs

henghonglee

Hi @TheBlueMatt, it's finally ready for review :)

lightning-background-processor/src/lib.rs

lightning/src/routing/scoring.rs

TheBlueMatt · 2023-05-09T01:31:48Z

You can tell github thinks your line is too long because it cut off the last word and replace it with ... :). I think its cutoff is ~~around 75, but I'm not 100% sure. 70 is always safe.~~ It was 70.

arik-so · 2023-05-09T17:15:45Z

lightning/src/routing/scoring.rs

+	pub manual_node_penalties: HashMap<NodeId, u64>,
+
+
+	/// This penalty is applied when `htlc_maximum_msat` is equal to or larger than half of the


I think writing out htlc_maximum_msat ≥ 0.5 * channel_capacity might make this a bit easier to read

I just moved this. but happy to make this change if thats necessary

Do it in a separate commit to preserve the move-only-ness, but nothing wrong with cleaning things up when they're getting touched anyway.

sure i can tease that out it into a seperate commit.

arik-so · 2023-05-09T17:20:06Z

lightning/src/routing/scoring.rs

+/// these decay parameters affect the score of the channel penalty but are usually not changed on a per-route
+/// penalty cost call.


Nit: These
Also, "per-route" could probably go on the next line.

okay will do

lightning/src/routing/scoring.rs

TheBlueMatt · 2023-05-10T16:59:08Z

Its not critical, but if you do have to push again for some reason, your commit message still has lines (in the body, post-title) that are longer than 70 chars long. This LGTM, however, I'll let @arik-so take another look.

henghonglee · 2023-05-10T18:45:59Z

@arik-so

arik-so · 2023-05-10T18:54:07Z

Looks good, but there are failing tests, so I imagine you're probably gonna need to push again.

This PR aims to create a "stateless" scorer. Instead of passing in fee params at construction-time, we want to parametrize the scorer with an associated "parameter" type, which is then passed to the router function itself, and allows passing different parameters per route-finding call.

henghonglee · 2023-05-11T00:51:50Z

@arik-so checks seems to be passing now

arik-so

Thanks!

TheBlueMatt · 2023-05-11T05:16:11Z

lightning/src/routing/scoring.rs

+
+#[cfg(test)]
+impl ProbabilisticScoringDecayParameters {
+	fn zero_penalty() -> Self {


Heh, we don't really need a duplicate of default that does the same thing :)

henghonglee changed the title ~~Issue 2189: Changes to score params~~ https://github.com/lightningdevkit/rust-lightning/issues/2189 Issue 2189: Changes to score params Apr 27, 2023

henghonglee changed the title ~~https://github.com/lightningdevkit/rust-lightning/issues/2189 Issue 2189: Changes to score params~~ [Issue 2189] Changes to score params Apr 27, 2023

henghonglee force-pushed the issue-2189-score-params branch from 9ec4e0f to d3273d3 Compare April 27, 2023 07:34

TheBlueMatt reviewed Apr 28, 2023

View reviewed changes

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

henghonglee force-pushed the issue-2189-score-params branch 9 times, most recently from 65a65ea to b00c185 Compare April 28, 2023 21:21

henghonglee force-pushed the issue-2189-score-params branch 2 times, most recently from 7605325 to 25924b3 Compare May 3, 2023 15:21

henghonglee commented May 3, 2023

View reviewed changes

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

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

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

henghonglee force-pushed the issue-2189-score-params branch 3 times, most recently from db1db6e to 9362cbd Compare May 3, 2023 16:03

TheBlueMatt reviewed May 3, 2023

View reviewed changes

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

henghonglee commented May 3, 2023

View reviewed changes

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

henghonglee force-pushed the issue-2189-score-params branch 3 times, most recently from 530cb5d to 422d73d Compare May 5, 2023 01:15

henghonglee commented May 5, 2023

View reviewed changes

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

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

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

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

henghonglee force-pushed the issue-2189-score-params branch from 422d73d to 1c8c153 Compare May 5, 2023 01:23

henghonglee requested a review from TheBlueMatt May 5, 2023 01:23

henghonglee marked this pull request as ready for review May 5, 2023 04:15

henghonglee changed the title ~~[Issue 2189] Changes to score params~~ [Issue 2189] Score Fee Params as a passed in parameter May 5, 2023

henghonglee force-pushed the issue-2189-score-params branch 2 times, most recently from c3e3764 to 490f401 Compare May 9, 2023 00:53

henghonglee force-pushed the issue-2189-score-params branch 3 times, most recently from 6d882ab to 567a225 Compare May 9, 2023 04:50

arik-so self-requested a review May 9, 2023 16:51

arik-so reviewed May 9, 2023

View reviewed changes

henghonglee force-pushed the issue-2189-score-params branch from 567a225 to d1e9af7 Compare May 9, 2023 23:34

henghonglee requested a review from arik-so May 9, 2023 23:35

henghonglee force-pushed the issue-2189-score-params branch 5 times, most recently from 8aacbe7 to a2d0363 Compare May 10, 2023 05:11

henghonglee force-pushed the issue-2189-score-params branch 3 times, most recently from 04dc495 to 0614a6a Compare May 10, 2023 18:21

henghonglee force-pushed the issue-2189-score-params branch from 0614a6a to ecd7992 Compare May 10, 2023 19:53

Move functions around to colocate impl of FeeParams and DecayParams

86af670

henghonglee force-pushed the issue-2189-score-params branch from ecd7992 to 86af670 Compare May 10, 2023 22:32

arik-so approved these changes May 11, 2023

View reviewed changes

TheBlueMatt approved these changes May 11, 2023

View reviewed changes

TheBlueMatt merged commit e61b128 into lightningdevkit:main May 11, 2023

wpaulino mentioned this pull request Aug 15, 2023

Pass scoring params through the router #2189

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#2189] Score Fee Params as a passed in parameter #2237

[#2189] Score Fee Params as a passed in parameter #2237

henghonglee commented Apr 26, 2023 •

edited

Loading

TheBlueMatt left a comment

TheBlueMatt commented Apr 28, 2023

henghonglee left a comment

henghonglee left a comment

henghonglee left a comment •

edited

Loading

TheBlueMatt commented May 9, 2023 •

edited

Loading

arik-so May 9, 2023

henghonglee May 9, 2023

TheBlueMatt May 9, 2023

henghonglee May 10, 2023

henghonglee May 10, 2023

arik-so May 9, 2023

henghonglee May 9, 2023

TheBlueMatt commented May 10, 2023

henghonglee commented May 10, 2023

arik-so commented May 10, 2023

henghonglee commented May 11, 2023

arik-so left a comment

TheBlueMatt May 11, 2023

		pub manual_node_penalties: HashMap<NodeId, u64>,


		/// This penalty is applied when `htlc_maximum_msat` is equal to or larger than half of the

		/// these decay parameters affect the score of the channel penalty but are usually not changed on a per-route
		/// penalty cost call.

[#2189] Score Fee Params as a passed in parameter #2237

[#2189] Score Fee Params as a passed in parameter #2237

Conversation

henghonglee commented Apr 26, 2023 • edited Loading

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 28, 2023

henghonglee left a comment

Choose a reason for hiding this comment

henghonglee left a comment

Choose a reason for hiding this comment

henghonglee left a comment • edited Loading

Choose a reason for hiding this comment

TheBlueMatt commented May 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented May 10, 2023

henghonglee commented May 10, 2023

arik-so commented May 10, 2023

henghonglee commented May 11, 2023

arik-so left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henghonglee commented Apr 26, 2023 •

edited

Loading

henghonglee left a comment •

edited

Loading

TheBlueMatt commented May 9, 2023 •

edited

Loading