Include source and destination nodes in routing::Score by jkczyz · Pull Request #1133 · lightningdevkit/rust-lightning

jkczyz · 2021-10-18T23:43:45Z

Expand routing::Score::channel_penalty_msat to include the source and target node ids of the channel. This allows scorers to avoid certain nodes altogether if desired.

In order to make the scoring tests easier to read, only check the relevant RouteHop fields. The remaining fields are tested elsewhere. Expand the test to show the path used without scoring.

codecov · 2021-10-19T00:04:07Z

Codecov Report

Merging #1133 (a950b8f) into main (001bc71) will increase coverage by 0.00%.
The diff coverage is 96.61%.

❗ Current head a950b8f differs from pull request most recent head 54f490c. Consider uploading reports for the commit 54f490c to get more accurate results

@@ Coverage Diff @@## main #1133 +/- ##
=======================================
Coverage 90.56% 90.56% =======================================
Files 67 67 Lines 34567 34597 +30 =======================================
+ Hits 31305 31334 +29 - Misses 3262 3263 +1

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`95.71% <96.42%> (+<0.01%)`	⬆️
lightning/src/routing/scorer.rs	`66.66% <100.00%> (+9.52%)`	⬆️
lightning-invoice/src/de.rs	`80.73% <0.00%> (-0.21%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.33% <0.00%> (+0.01%)`	⬆️

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 001bc71...54f490c. Read the comment docs.

TheBlueMatt · 2021-10-19T00:12:58Z

 ///	Scoring is in terms of fees willing to be paid in order to avoid routing through a channel.
 pub trait Score {
 	/// Returns the fee in msats willing to be paid to avoid routing through the given channel.
-	fn channel_penalty_msat(&self, short_channel_id: u64) -> u64;


nit +"in the given direction" on the docs.

valentinewallace · 2021-10-19T16:22:18Z

 pub trait Score {
-	/// Returns the fee in msats willing to be paid to avoid routing through the given channel.
+	/// Returns the fee in msats willing to be paid to avoid routing through the given channel from
+	/// `source` to `destination`.


This isn't the source and dest of the payment itself, right? Just the forwarder/forwardee of the given short_channel_id? Those names are a bit overloaded for me lol

Correct. I could use the more general term target instead of destination and reword the docs a bit.

valentinewallace

Thanks, that makes it clearer for me

TheBlueMatt

Can you squash.

Expand routing::Score::channel_penalty_msat to include the source and target node ids of the channel. This allows scorers to avoid certain nodes altogether if desired.

TheBlueMatt · 2021-10-19T21:55:46Z

Squashed without changed:

$ git diff-tree -U1 e3a1b8acd 54f490c8c
$

jkczyz · 2021-10-18T23:43:45Z

Expand routing::Score::channel_penalty_msat to include the source and target node ids of the channel. This allows scorers to avoid certain nodes altogether if desired.

In order to make the scoring tests easier to read, only check the relevant RouteHop fields. The remaining fields are tested elsewhere. Expand the test to show the path used without scoring.

codecov · 2021-10-19T00:04:07Z

Codecov Report

Merging #1133 (a950b8f) into main (001bc71) will increase coverage by 0.00%.
The diff coverage is 96.61%.

❗ Current head a950b8f differs from pull request most recent head 54f490c. Consider uploading reports for the commit 54f490c to get more accurate results

@@ Coverage Diff @@## main #1133 +/- ##
=======================================
Coverage 90.56% 90.56% =======================================
Files 67 67 Lines 34567 34597 +30 =======================================
+ Hits 31305 31334 +29 - Misses 3262 3263 +1

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`95.71% <96.42%> (+<0.01%)`	⬆️
lightning/src/routing/scorer.rs	`66.66% <100.00%> (+9.52%)`	⬆️
lightning-invoice/src/de.rs	`80.73% <0.00%> (-0.21%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.33% <0.00%> (+0.01%)`	⬆️

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 001bc71...54f490c. Read the comment docs.

TheBlueMatt · 2021-10-19T00:12:58Z

 ///	Scoring is in terms of fees willing to be paid in order to avoid routing through a channel.
 pub trait Score {
 	/// Returns the fee in msats willing to be paid to avoid routing through the given channel.
-	fn channel_penalty_msat(&self, short_channel_id: u64) -> u64;


nit +"in the given direction" on the docs.

valentinewallace · 2021-10-19T16:22:18Z

 pub trait Score {
-	/// Returns the fee in msats willing to be paid to avoid routing through the given channel.
+	/// Returns the fee in msats willing to be paid to avoid routing through the given channel from
+	/// `source` to `destination`.


This isn't the source and dest of the payment itself, right? Just the forwarder/forwardee of the given short_channel_id? Those names are a bit overloaded for me lol

Correct. I could use the more general term target instead of destination and reword the docs a bit.

valentinewallace

Thanks, that makes it clearer for me

TheBlueMatt

Can you squash.

Expand routing::Score::channel_penalty_msat to include the source and target node ids of the channel. This allows scorers to avoid certain nodes altogether if desired.

TheBlueMatt · 2021-10-19T21:55:46Z

Squashed without changed:

$ git diff-tree -U1 e3a1b8acd 54f490c8c
$

jkczyz · 2021-10-18T23:43:45Z

Expand routing::Score::channel_penalty_msat to include the source and target node ids of the channel. This allows scorers to avoid certain nodes altogether if desired.

In order to make the scoring tests easier to read, only check the relevant RouteHop fields. The remaining fields are tested elsewhere. Expand the test to show the path used without scoring.

codecov · 2021-10-19T00:04:07Z

Codecov Report

Merging #1133 (a950b8f) into main (001bc71) will increase coverage by 0.00%.
The diff coverage is 96.61%.

❗ Current head a950b8f differs from pull request most recent head 54f490c. Consider uploading reports for the commit 54f490c to get more accurate results

@@ Coverage Diff @@## main #1133 +/- ##
=======================================
Coverage 90.56% 90.56% =======================================
Files 67 67 Lines 34567 34597 +30 =======================================
+ Hits 31305 31334 +29 - Misses 3262 3263 +1

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`95.71% <96.42%> (+<0.01%)`	⬆️
lightning/src/routing/scorer.rs	`66.66% <100.00%> (+9.52%)`	⬆️
lightning-invoice/src/de.rs	`80.73% <0.00%> (-0.21%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.33% <0.00%> (+0.01%)`	⬆️

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 001bc71...54f490c. Read the comment docs.

TheBlueMatt · 2021-10-19T00:12:58Z

 ///	Scoring is in terms of fees willing to be paid in order to avoid routing through a channel.
 pub trait Score {
 	/// Returns the fee in msats willing to be paid to avoid routing through the given channel.
-	fn channel_penalty_msat(&self, short_channel_id: u64) -> u64;


nit +"in the given direction" on the docs.

valentinewallace · 2021-10-19T16:22:18Z

 pub trait Score {
-	/// Returns the fee in msats willing to be paid to avoid routing through the given channel.
+	/// Returns the fee in msats willing to be paid to avoid routing through the given channel from
+	/// `source` to `destination`.


This isn't the source and dest of the payment itself, right? Just the forwarder/forwardee of the given short_channel_id? Those names are a bit overloaded for me lol

Correct. I could use the more general term target instead of destination and reword the docs a bit.

valentinewallace

Thanks, that makes it clearer for me

TheBlueMatt

Can you squash.

Expand routing::Score::channel_penalty_msat to include the source and target node ids of the channel. This allows scorers to avoid certain nodes altogether if desired.

TheBlueMatt · 2021-10-19T21:55:46Z

Squashed without changed:

$ git diff-tree -U1 e3a1b8acd 54f490c8c
$

jkczyz · 2021-10-18T23:43:45Z

Expand routing::Score::channel_penalty_msat to include the source and target node ids of the channel. This allows scorers to avoid certain nodes altogether if desired.

In order to make the scoring tests easier to read, only check the relevant RouteHop fields. The remaining fields are tested elsewhere. Expand the test to show the path used without scoring.

codecov · 2021-10-19T00:04:07Z

Codecov Report

Merging #1133 (a950b8f) into main (001bc71) will increase coverage by 0.00%.
The diff coverage is 96.61%.

❗ Current head a950b8f differs from pull request most recent head 54f490c. Consider uploading reports for the commit 54f490c to get more accurate results

@@ Coverage Diff @@## main #1133 +/- ##
=======================================
Coverage 90.56% 90.56% =======================================
Files 67 67 Lines 34567 34597 +30 =======================================
+ Hits 31305 31334 +29 - Misses 3262 3263 +1

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`95.71% <96.42%> (+<0.01%)`	⬆️
lightning/src/routing/scorer.rs	`66.66% <100.00%> (+9.52%)`	⬆️
lightning-invoice/src/de.rs	`80.73% <0.00%> (-0.21%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.33% <0.00%> (+0.01%)`	⬆️

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 001bc71...54f490c. Read the comment docs.

TheBlueMatt · 2021-10-19T00:12:58Z

 ///	Scoring is in terms of fees willing to be paid in order to avoid routing through a channel.
 pub trait Score {
 	/// Returns the fee in msats willing to be paid to avoid routing through the given channel.
-	fn channel_penalty_msat(&self, short_channel_id: u64) -> u64;


nit +"in the given direction" on the docs.

valentinewallace · 2021-10-19T16:22:18Z

 pub trait Score {
-	/// Returns the fee in msats willing to be paid to avoid routing through the given channel.
+	/// Returns the fee in msats willing to be paid to avoid routing through the given channel from
+	/// `source` to `destination`.


This isn't the source and dest of the payment itself, right? Just the forwarder/forwardee of the given short_channel_id? Those names are a bit overloaded for me lol

Correct. I could use the more general term target instead of destination and reword the docs a bit.

valentinewallace

Thanks, that makes it clearer for me

TheBlueMatt

Can you squash.

Expand routing::Score::channel_penalty_msat to include the source and target node ids of the channel. This allows scorers to avoid certain nodes altogether if desired.

TheBlueMatt · 2021-10-19T21:55:46Z

Squashed without changed:

$ git diff-tree -U1 e3a1b8acd 54f490c8c
$

jkczyz · 2021-10-18T23:43:45Z

Expand routing::Score::channel_penalty_msat to include the source and target node ids of the channel. This allows scorers to avoid certain nodes altogether if desired.

In order to make the scoring tests easier to read, only check the relevant RouteHop fields. The remaining fields are tested elsewhere. Expand the test to show the path used without scoring.

codecov · 2021-10-19T00:04:07Z

Codecov Report

Merging #1133 (a950b8f) into main (001bc71) will increase coverage by 0.00%.
The diff coverage is 96.61%.

❗ Current head a950b8f differs from pull request most recent head 54f490c. Consider uploading reports for the commit 54f490c to get more accurate results

@@ Coverage Diff @@## main #1133 +/- ##
=======================================
Coverage 90.56% 90.56% =======================================
Files 67 67 Lines 34567 34597 +30 =======================================
+ Hits 31305 31334 +29 - Misses 3262 3263 +1

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`95.71% <96.42%> (+<0.01%)`	⬆️
lightning/src/routing/scorer.rs	`66.66% <100.00%> (+9.52%)`	⬆️
lightning-invoice/src/de.rs	`80.73% <0.00%> (-0.21%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.33% <0.00%> (+0.01%)`	⬆️

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 001bc71...54f490c. Read the comment docs.

TheBlueMatt · 2021-10-19T00:12:58Z

 ///	Scoring is in terms of fees willing to be paid in order to avoid routing through a channel.
 pub trait Score {
 	/// Returns the fee in msats willing to be paid to avoid routing through the given channel.
-	fn channel_penalty_msat(&self, short_channel_id: u64) -> u64;


nit +"in the given direction" on the docs.

valentinewallace · 2021-10-19T16:22:18Z

 pub trait Score {
-	/// Returns the fee in msats willing to be paid to avoid routing through the given channel.
+	/// Returns the fee in msats willing to be paid to avoid routing through the given channel from
+	/// `source` to `destination`.


This isn't the source and dest of the payment itself, right? Just the forwarder/forwardee of the given short_channel_id? Those names are a bit overloaded for me lol

Correct. I could use the more general term target instead of destination and reword the docs a bit.

valentinewallace

Thanks, that makes it clearer for me

TheBlueMatt

Can you squash.

Expand routing::Score::channel_penalty_msat to include the source and target node ids of the channel. This allows scorers to avoid certain nodes altogether if desired.

TheBlueMatt · 2021-10-19T21:55:46Z

Squashed without changed:

$ git diff-tree -U1 e3a1b8acd 54f490c8c
$

jkczyz · 2021-10-18T23:43:45Z

Expand routing::Score::channel_penalty_msat to include the source and target node ids of the channel. This allows scorers to avoid certain nodes altogether if desired.

In order to make the scoring tests easier to read, only check the relevant RouteHop fields. The remaining fields are tested elsewhere. Expand the test to show the path used without scoring.

codecov · 2021-10-19T00:04:07Z

Codecov Report

Merging #1133 (a950b8f) into main (001bc71) will increase coverage by 0.00%.
The diff coverage is 96.61%.

❗ Current head a950b8f differs from pull request most recent head 54f490c. Consider uploading reports for the commit 54f490c to get more accurate results

@@ Coverage Diff @@## main #1133 +/- ##
=======================================
Coverage 90.56% 90.56% =======================================
Files 67 67 Lines 34567 34597 +30 =======================================
+ Hits 31305 31334 +29 - Misses 3262 3263 +1

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`95.71% <96.42%> (+<0.01%)`	⬆️
lightning/src/routing/scorer.rs	`66.66% <100.00%> (+9.52%)`	⬆️
lightning-invoice/src/de.rs	`80.73% <0.00%> (-0.21%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.33% <0.00%> (+0.01%)`	⬆️

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 001bc71...54f490c. Read the comment docs.

TheBlueMatt · 2021-10-19T00:12:58Z

 ///	Scoring is in terms of fees willing to be paid in order to avoid routing through a channel.
 pub trait Score {
 	/// Returns the fee in msats willing to be paid to avoid routing through the given channel.
-	fn channel_penalty_msat(&self, short_channel_id: u64) -> u64;


nit +"in the given direction" on the docs.

valentinewallace · 2021-10-19T16:22:18Z

 pub trait Score {
-	/// Returns the fee in msats willing to be paid to avoid routing through the given channel.
+	/// Returns the fee in msats willing to be paid to avoid routing through the given channel from
+	/// `source` to `destination`.


This isn't the source and dest of the payment itself, right? Just the forwarder/forwardee of the given short_channel_id? Those names are a bit overloaded for me lol

Correct. I could use the more general term target instead of destination and reword the docs a bit.

valentinewallace

Thanks, that makes it clearer for me

TheBlueMatt

Can you squash.

Expand routing::Score::channel_penalty_msat to include the source and target node ids of the channel. This allows scorers to avoid certain nodes altogether if desired.

TheBlueMatt · 2021-10-19T21:55:46Z

Squashed without changed:

$ git diff-tree -U1 e3a1b8acd 54f490c8c
$

jkczyz · 2021-10-18T23:43:45Z

Expand routing::Score::channel_penalty_msat to include the source and target node ids of the channel. This allows scorers to avoid certain nodes altogether if desired.

In order to make the scoring tests easier to read, only check the relevant RouteHop fields. The remaining fields are tested elsewhere. Expand the test to show the path used without scoring.

codecov · 2021-10-19T00:04:07Z

Codecov Report

Merging #1133 (a950b8f) into main (001bc71) will increase coverage by 0.00%.
The diff coverage is 96.61%.

❗ Current head a950b8f differs from pull request most recent head 54f490c. Consider uploading reports for the commit 54f490c to get more accurate results

@@ Coverage Diff @@## main #1133 +/- ##
=======================================
Coverage 90.56% 90.56% =======================================
Files 67 67 Lines 34567 34597 +30 =======================================
+ Hits 31305 31334 +29 - Misses 3262 3263 +1

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`95.71% <96.42%> (+<0.01%)`	⬆️
lightning/src/routing/scorer.rs	`66.66% <100.00%> (+9.52%)`	⬆️
lightning-invoice/src/de.rs	`80.73% <0.00%> (-0.21%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.33% <0.00%> (+0.01%)`	⬆️

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 001bc71...54f490c. Read the comment docs.

TheBlueMatt · 2021-10-19T00:12:58Z

 ///	Scoring is in terms of fees willing to be paid in order to avoid routing through a channel.
 pub trait Score {
 	/// Returns the fee in msats willing to be paid to avoid routing through the given channel.
-	fn channel_penalty_msat(&self, short_channel_id: u64) -> u64;


nit +"in the given direction" on the docs.

valentinewallace · 2021-10-19T16:22:18Z

 pub trait Score {
-	/// Returns the fee in msats willing to be paid to avoid routing through the given channel.
+	/// Returns the fee in msats willing to be paid to avoid routing through the given channel from
+	/// `source` to `destination`.


This isn't the source and dest of the payment itself, right? Just the forwarder/forwardee of the given short_channel_id? Those names are a bit overloaded for me lol

Correct. I could use the more general term target instead of destination and reword the docs a bit.

valentinewallace

Thanks, that makes it clearer for me

TheBlueMatt

Can you squash.

Expand routing::Score::channel_penalty_msat to include the source and target node ids of the channel. This allows scorers to avoid certain nodes altogether if desired.

TheBlueMatt · 2021-10-19T21:55:46Z

Squashed without changed:

$ git diff-tree -U1 e3a1b8acd 54f490c8c
$

jkczyz · 2021-10-18T23:43:45Z

Expand routing::Score::channel_penalty_msat to include the source and target node ids of the channel. This allows scorers to avoid certain nodes altogether if desired.

In order to make the scoring tests easier to read, only check the relevant RouteHop fields. The remaining fields are tested elsewhere. Expand the test to show the path used without scoring.

codecov · 2021-10-19T00:04:07Z

Codecov Report

Merging #1133 (a950b8f) into main (001bc71) will increase coverage by 0.00%.
The diff coverage is 96.61%.

❗ Current head a950b8f differs from pull request most recent head 54f490c. Consider uploading reports for the commit 54f490c to get more accurate results

@@ Coverage Diff @@## main #1133 +/- ##
=======================================
Coverage 90.56% 90.56% =======================================
Files 67 67 Lines 34567 34597 +30 =======================================
+ Hits 31305 31334 +29 - Misses 3262 3263 +1

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`95.71% <96.42%> (+<0.01%)`	⬆️
lightning/src/routing/scorer.rs	`66.66% <100.00%> (+9.52%)`	⬆️
lightning-invoice/src/de.rs	`80.73% <0.00%> (-0.21%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.33% <0.00%> (+0.01%)`	⬆️

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 001bc71...54f490c. Read the comment docs.

TheBlueMatt · 2021-10-19T00:12:58Z

 ///	Scoring is in terms of fees willing to be paid in order to avoid routing through a channel.
 pub trait Score {
 	/// Returns the fee in msats willing to be paid to avoid routing through the given channel.
-	fn channel_penalty_msat(&self, short_channel_id: u64) -> u64;


nit +"in the given direction" on the docs.

valentinewallace · 2021-10-19T16:22:18Z

 pub trait Score {
-	/// Returns the fee in msats willing to be paid to avoid routing through the given channel.
+	/// Returns the fee in msats willing to be paid to avoid routing through the given channel from
+	/// `source` to `destination`.


This isn't the source and dest of the payment itself, right? Just the forwarder/forwardee of the given short_channel_id? Those names are a bit overloaded for me lol

Correct. I could use the more general term target instead of destination and reword the docs a bit.

valentinewallace

Thanks, that makes it clearer for me

TheBlueMatt

Can you squash.

Expand routing::Score::channel_penalty_msat to include the source and target node ids of the channel. This allows scorers to avoid certain nodes altogether if desired.

TheBlueMatt · 2021-10-19T21:55:46Z

Squashed without changed:

$ git diff-tree -U1 e3a1b8acd 54f490c8c
$

Simplify prefers_shorter_route_with_higher_fees
57f0822
In order to make the scoring tests easier to read, only check the relevant RouteHop fields. The remaining fields are tested elsewhere. Expand the test to show the path used without scoring.