Add channel scoring to get_route - #1124

Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom
jkczyz:2021-10-default-channel-scoring
Oct 16, 2021
Merged

Add channel scoring to get_route#1124
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom
jkczyz:2021-10-default-channel-scoring

Conversation

@jkczyz

@jkczyzjkczyz commented Oct 14, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

Failed payments may be retried, but calling get_route may return a Route with the same failing path. Add a routing::Score trait used to parameterize get_route, which it calls to determine how much a channel should be penalized in terms of msats willing to pay to avoid the channel.

Also, add a Scorer struct that implements routing::Score with a constant penalty. Subsequent changes will allow for more robust scoring by feeding back payment path success and failure to the scorer via event handling.

@codecov

codecovBot commented Oct 14, 2021•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1124 (84c0a48) into main (da498d7) will increase coverage by 0.76%.
The diff coverage is 91.52%.

❗ Current head 84c0a48 differs from pull request most recent head e15a18a. Consider uploading reports for the commit e15a18a to get more accurate results
Impacted file tree graph

@@ Coverage Diff @@## main #1124 +/- ##
==========================================
+ Coverage 90.60% 91.36% +0.76% 
==========================================
Files 66 67 +1 Lines 34474 37698 +3224 ==========================================
+ Hits 31235 34443 +3208 - Misses 3239 3255 +16
Impacted FilesCoverage Δ
lightning/src/routing/scorer.rs57.14% <57.14%> (ø)
lightning/src/routing/router.rs95.70% <91.78%> (-0.30%)⬇️
lightning-invoice/src/utils.rs84.26% <100.00%> (+0.17%)⬆️
lightning/src/ln/channelmanager.rs85.14% <100.00%> (+0.01%)⬆️
lightning/src/ln/functional_test_utils.rs97.32% <100.00%> (+2.24%)⬆️
lightning/src/ln/functional_tests.rs97.93% <100.00%> (+0.53%)⬆️
lightning/src/ln/shutdown_tests.rs95.87% <100.00%> (+<0.01%)⬆️
lightning/src/chain/mod.rs50.00% <0.00%> (-8.83%)⬇️
lightning-background-processor/src/lib.rs94.38% <0.00%> (+0.15%)⬆️
... 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 da498d7...e15a18a. Read the comment docs.

@TheBlueMattTheBlueMatt added this to the 0.0.102 milestone Oct 14, 2021
TheBlueMatt
TheBlueMatt reviewed Oct 14, 2021
View reviewed changes

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmarks are probably about what I'd expect given the expansion of of in-cmp logic. Something to look into eventually, probably:

Latest git

test ln::channelmanager::bench::bench_sends ... bench: 9,128,177 ns/iter (+/- 1,349,843)
test routing::network_graph::benches::read_network_graph ... bench: 1,777,566,078 ns/iter (+/- 23,897,528)
test routing::network_graph::benches::write_network_graph ... bench: 159,355,206 ns/iter (+/- 5,292,557)
test routing::router::benches::generate_mpp_routes ... bench: 74,852,606 ns/iter (+/- 55,662,508)
test routing::router::benches::generate_routes ... bench: 72,321,963 ns/iter (+/- 56,925,293)
test bench::bench_sends ... bench: 211,670,987 ns/iter (+/- 188,695,358)

This PR:

test ln::channelmanager::bench::bench_sends ... bench: 8,284,024 ns/iter (+/- 443,276)
test routing::network_graph::benches::read_network_graph ... bench: 1,614,888,492 ns/iter (+/- 2,280,811)
test routing::network_graph::benches::write_network_graph ... bench: 143,246,612 ns/iter (+/- 458,059)
test routing::router::benches::generate_mpp_routes ... bench: 96,742,965 ns/iter (+/- 52,848,627)
test routing::router::benches::generate_routes ... bench: 96,963,520 ns/iter (+/- 62,975,254)
test bench::bench_sends ... bench: 190,069,734 ns/iter (+/- 168,900,474)

Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/mod.rs Outdated
Comment threadlightning/src/routing/mod.rs Outdated
@TheBlueMatt

TheBlueMatt commented Oct 14, 2021

Copy link
Copy Markdown
Collaborator

Note for comparison bench for 0.0.101 was (there's obviously a ton of noise in these numbers from github CI).

test ln::channelmanager::bench::bench_sends ... bench: 7,587,098 ns/iter (+/- 1,026,747)
test routing::network_graph::benches::read_network_graph ... bench: 2,195,464,293 ns/iter (+/- 116,402,530)
test routing::network_graph::benches::write_network_graph ... bench: 132,630,621 ns/iter (+/- 11,344,574)
test routing::router::benches::generate_mpp_routes ... bench: 82,229,661 ns/iter (+/- 48,137,908)
test routing::router::benches::generate_routes ... bench: 83,419,028 ns/iter (+/- 62,531,148)
test bench::bench_sends ... bench: 175,605,757 ns/iter (+/- 148,418,861)

TheBlueMatt
TheBlueMatt reviewed Oct 14, 2021
View reviewed changes
Comment threadlightning/src/routing/router.rs Outdated
TheBlueMatt
TheBlueMatt reviewed Oct 14, 2021
View reviewed changes
Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs Outdated
@jkczyz

jkczyz commented Oct 14, 2021

Copy link
Copy Markdown
ContributorAuthor

All good feedback! Thanks for divining into benchmarks. All comments addressed.

@TheBlueMatt

TheBlueMatt commented Oct 14, 2021

Copy link
Copy Markdown
Collaborator

Github CI now says (which is, uhmmmm, faster than latest git? yea, no, that's just cause github's CI machines are clearly very heterogeneous....last time I look at that output.........)

test ln::channelmanager::bench::bench_sends ... bench: 8,363,865 ns/iter (+/- 1,362,103)
test routing::network_graph::benches::read_network_graph ... bench: 1,626,468,025 ns/iter (+/- 90,985,613)
test routing::network_graph::benches::write_network_graph ... bench: 142,291,900 ns/iter (+/- 6,987,924)
test routing::router::benches::generate_mpp_routes ... bench: 57,154,598 ns/iter (+/- 32,542,178)
test routing::router::benches::generate_routes ... bench: 57,344,572 ns/iter (+/- 39,866,175)
test bench::bench_sends ... bench: 197,672,739 ns/iter (+/- 170,050,060)

TheBlueMatt
TheBlueMatt approved these changes Oct 15, 2021
View reviewed changes

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs tests but otherwise good.

Comment threadlightning/src/routing/router.rs Outdated
path_htlc_minimum_msat: u64,
/// All penalties incurred *after* this hop on the way to the destination, as calculated using
/// channel scoring.
next_hops_penalty_msat: u64,

@TheBlueMattTheBlueMattOct 15, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of next_hops in these confused me somewhat - next_hops_fee_msat (both in a PathBuildingHop and in the add_node macro) refers to only fee after the hop but not including the fee charged at the hop, whereas here we're using it to include the penalty charged at a hop. Maybe path_penalty_msat or so?

@jkczyzjkczyzOct 15, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. I was a little confused by the naming, but now I see the pattern. Should be consistent now.

Comment threadlightning/src/ln/functional_tests.rs Outdated
use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT;
use routing::network_graph::{NetworkUpdate, RoutingFees};
use routing::router::{Route, RouteHop, RouteHint, RouteHintHop, get_route, get_keysend_route};
use routing::scorer::DefaultScorer;

@TheBlueMattTheBlueMattOct 15, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use either a TestScorer in tests or a DefaultScorer with a score of 0? I find it a bit...awkward? to change all of our routing parameters in all our tests, even if they still pass it kinda makes me nervous.

@jkczyzjkczyzOct 15, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. I removed TestScorer and renamed DefaultScorer to Scorer, using it with 0 in all tests excepted for the newly added one.

Comment threadlightning/src/routing/scorer.rs Outdated
impl Default for DefaultScorer {
/// Creates a new scorer using 100 msat as the channel penalty.
fn default() -> Self {
DefaultScorer::new(100)

@TheBlueMattTheBlueMattOct 15, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think default should be 500, just kinda gut-feel. Lots of "good" nodes charge 1 sat per payment, and we should probably be willing to prefer that over two-three "free" hops IMO. Maybe even 1k. We can tune this later, but 100 just seems very low.

@jkczyzjkczyzOct 15, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to 500 msats.

jkczyz
jkczyz commented Oct 15, 2021
View reviewed changes

@jkczyzjkczyz left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test but was getting a little confused with the fee_msat assertions. Let me know if it looks sane. I could add more tests if needed.

Comment threadlightning/src/routing/router.rs Outdated
path_htlc_minimum_msat: u64,
/// All penalties incurred *after* this hop on the way to the destination, as calculated using
/// channel scoring.
next_hops_penalty_msat: u64,

@jkczyzjkczyzOct 15, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. I was a little confused by the naming, but now I see the pattern. Should be consistent now.

Comment threadlightning/src/ln/functional_tests.rs Outdated
use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT;
use routing::network_graph::{NetworkUpdate, RoutingFees};
use routing::router::{Route, RouteHop, RouteHint, RouteHintHop, get_route, get_keysend_route};
use routing::scorer::DefaultScorer;

@jkczyzjkczyzOct 15, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. I removed TestScorer and renamed DefaultScorer to Scorer, using it with 0 in all tests excepted for the newly added one.

Comment threadlightning/src/routing/scorer.rs Outdated
impl Default for DefaultScorer {
/// Creates a new scorer using 100 msat as the channel penalty.
fn default() -> Self {
DefaultScorer::new(100)

@jkczyzjkczyzOct 15, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to 500 msats.

valentinewallace
valentinewallace reviewed Oct 15, 2021
View reviewed changes

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to get an initial version of this in!

Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/mod.rs
Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/scorer.rs Outdated
//!
//! # Example
//!
//! TODO: Fill in example once interface has been finalized.

@valentinewallacevalentinewallaceOct 15, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're good to fill this in?

@jkczyzjkczyzOct 15, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Not much to it yet, but could expand if we add more configurations. Also, may want to have an example with InvoicePayer for event handling once that is in. Or at least reference those docs.

valentinewallace
valentinewallace approved these changes Oct 15, 2021
View reviewed changes

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@TheBlueMatt

TheBlueMatt commented Oct 15, 2021

Copy link
Copy Markdown
Collaborator

Can you squash :).

@jkczyz
Add channel scoring to get_route
e15a18a
Failed payments may be retried, but calling get_route may return a Route
with the same failing path. Add a routing::Score trait used to
parameterize get_route, which it calls to determine how much a channel
should be penalized in terms of msats willing to pay to avoid the
channel.
Also, add a Scorer struct that implements routing::Score with a constant
constant penalty. Subsequent changes will allow for more robust scoring
by feeding back payment path success and failure to the scorer via event
handling.
@jkczyz
jkczyzforce-pushed the 2021-10-default-channel-scoring branch from 84c0a48 to e15a18aCompareOctober 15, 2021 20:31
TheBlueMatt
TheBlueMatt approved these changes Oct 15, 2021
View reviewed changes

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff since val's ack was just a squash:

$ git diff-tree -U1 84c0a486e e15a18a50 $

@TheBlueMatt
TheBlueMatt merged commit 2398f17 into lightningdevkit:mainOct 16, 2021
@jkczyzjkczyz mentioned this pull request Oct 30, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@valentinewallacevalentinewallacevalentinewallace approved these changes
@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.0.102

Development

Successfully merging this pull request may close these issues.

3 participants

@jkczyz@TheBlueMatt@valentinewallace
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Add channel scoring to get_route - #1124
Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
jkczyz:2021-10-default-channel-scoring
Oct 16, 2021
Merged
Add channel scoring to get_route#1124
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
jkczyz:2021-10-default-channel-scoring
Conversation
@jkczyz
@jkczyzjkczyz
commented
Oct 14, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
Failed payments may be retried, but calling get_route may return a Route with the same failing path. Add a routing::Score trait used to parameterize get_route, which it calls to determine how much a channel should be penalized in terms of msats willing to pay to avoid the channel.
Also, add a Scorer struct that implements routing::Score with a constant penalty. Subsequent changes will allow for more robust scoring by feeding back payment path success and failure to the scorer via event handling.
@codecov
codecovBot
commented
Oct 14, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1124 (84c0a48) into main (da498d7) will increase coverage by 0.76%.

The diff coverage is 91.52%.
❗ Current head 84c0a48 differs from pull request most recent head e15a18a. Consider uploading reports for the commit e15a18a to get more accurate results
Impacted file tree graph
@@ Coverage Diff @@## main #1124 +/- ##
==========================================
+ Coverage 90.60% 91.36% +0.76% 
==========================================
Files 66 67 +1 Lines 34474 37698 +3224 ==========================================
+ Hits 31235 34443 +3208 - Misses 3239 3255 +16 
Impacted FilesCoverage Δ
lightning/src/routing/scorer.rs57.14% <57.14%> (ø)
lightning/src/routing/router.rs95.70% <91.78%> (-0.30%)⬇️
lightning-invoice/src/utils.rs84.26% <100.00%> (+0.17%)⬆️
lightning/src/ln/channelmanager.rs85.14% <100.00%> (+0.01%)⬆️
lightning/src/ln/functional_test_utils.rs97.32% <100.00%> (+2.24%)⬆️
lightning/src/ln/functional_tests.rs97.93% <100.00%> (+0.53%)⬆️
lightning/src/ln/shutdown_tests.rs95.87% <100.00%> (+<0.01%)⬆️
lightning/src/chain/mod.rs50.00% <0.00%> (-8.83%)⬇️
lightning-background-processor/src/lib.rs94.38% <0.00%> (+0.15%)⬆️
... 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 da498d7...e15a18a. Read the comment docs.
@TheBlueMattTheBlueMatt
added this to the 0.0.102 milestone
Oct 14, 2021
TheBlueMatt
TheBlueMatt
reviewed
Oct 14, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmarks are probably about what I'd expect given the expansion of of in-cmp logic. Something to look into eventually, probably:
Latest git
test ln::channelmanager::bench::bench_sends ... bench: 9,128,177 ns/iter (+/- 1,349,843)
test routing::network_graph::benches::read_network_graph ... bench: 1,777,566,078 ns/iter (+/- 23,897,528)
test routing::network_graph::benches::write_network_graph ... bench: 159,355,206 ns/iter (+/- 5,292,557)
test routing::router::benches::generate_mpp_routes ... bench: 74,852,606 ns/iter (+/- 55,662,508)
test routing::router::benches::generate_routes ... bench: 72,321,963 ns/iter (+/- 56,925,293)
test bench::bench_sends ... bench: 211,670,987 ns/iter (+/- 188,695,358)

This PR:
test ln::channelmanager::bench::bench_sends ... bench: 8,284,024 ns/iter (+/- 443,276)
test routing::network_graph::benches::read_network_graph ... bench: 1,614,888,492 ns/iter (+/- 2,280,811)
test routing::network_graph::benches::write_network_graph ... bench: 143,246,612 ns/iter (+/- 458,059)
test routing::router::benches::generate_mpp_routes ... bench: 96,742,965 ns/iter (+/- 52,848,627)
test routing::router::benches::generate_routes ... bench: 96,963,520 ns/iter (+/- 62,975,254)
test bench::bench_sends ... bench: 190,069,734 ns/iter (+/- 168,900,474)

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/mod.rs
Outdated

Comment threadlightning/src/routing/mod.rs
Outdated

@TheBlueMatt
TheBlueMatt
commented
Oct 14, 2021

Copy link


Copy Markdown

Collaborator
Note for comparison bench for 0.0.101 was (there's obviously a ton of noise in these numbers from github CI).
test ln::channelmanager::bench::bench_sends ... bench: 7,587,098 ns/iter (+/- 1,026,747)
test routing::network_graph::benches::read_network_graph ... bench: 2,195,464,293 ns/iter (+/- 116,402,530)
test routing::network_graph::benches::write_network_graph ... bench: 132,630,621 ns/iter (+/- 11,344,574)
test routing::router::benches::generate_mpp_routes ... bench: 82,229,661 ns/iter (+/- 48,137,908)
test routing::router::benches::generate_routes ... bench: 83,419,028 ns/iter (+/- 62,531,148)
test bench::bench_sends ... bench: 175,605,757 ns/iter (+/- 148,418,861)

TheBlueMatt
TheBlueMatt
reviewed
Oct 14, 2021
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Oct 14, 2021
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

@jkczyz
jkczyz
commented
Oct 14, 2021

Copy link


Copy Markdown

ContributorAuthor
All good feedback! Thanks for divining into benchmarks. All comments addressed.
@TheBlueMatt
TheBlueMatt
commented
Oct 14, 2021

Copy link


Copy Markdown

Collaborator
Github CI now says (which is, uhmmmm, faster than latest git? yea, no, that's just cause github's CI machines are clearly very heterogeneous....last time I look at that output.........)
test ln::channelmanager::bench::bench_sends ... bench: 8,363,865 ns/iter (+/- 1,362,103)
test routing::network_graph::benches::read_network_graph ... bench: 1,626,468,025 ns/iter (+/- 90,985,613)
test routing::network_graph::benches::write_network_graph ... bench: 142,291,900 ns/iter (+/- 6,987,924)
test routing::router::benches::generate_mpp_routes ... bench: 57,154,598 ns/iter (+/- 32,542,178)
test routing::router::benches::generate_routes ... bench: 57,344,572 ns/iter (+/- 39,866,175)
test bench::bench_sends ... bench: 197,672,739 ns/iter (+/- 170,050,060)

TheBlueMatt
TheBlueMatt
approved these changes
Oct 15, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs tests but otherwise good.
Comment threadlightning/src/routing/router.rs
Outdated

	path_htlc_minimum_msat: u64,
	/// All penalties incurred *after* this hop on the way to the destination, as calculated using
	/// channel scoring.
	next_hops_penalty_msat: u64,
@TheBlueMattTheBlueMattOct 15, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of next_hops in these confused me somewhat - next_hops_fee_msat (both in a PathBuildingHop and in the add_node macro) refers to only fee after the hop but not including the fee charged at the hop, whereas here we're using it to include the penalty charged at a hop. Maybe path_penalty_msat or so?
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. I was a little confused by the naming, but now I see the pattern. Should be consistent now.
Comment threadlightning/src/ln/functional_tests.rs
Outdated

use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT;
use routing::network_graph::{NetworkUpdate, RoutingFees};
use routing::router::{Route, RouteHop, RouteHint, RouteHintHop, get_route, get_keysend_route};
use routing::scorer::DefaultScorer;
@TheBlueMattTheBlueMattOct 15, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use either a TestScorer in tests or a DefaultScorer with a score of 0? I find it a bit...awkward? to change all of our routing parameters in all our tests, even if they still pass it kinda makes me nervous.
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. I removed TestScorer and renamed DefaultScorer to Scorer, using it with 0 in all tests excepted for the newly added one.
Comment threadlightning/src/routing/scorer.rs
Outdated

impl Default for DefaultScorer {
	/// Creates a new scorer using 100 msat as the channel penalty.
	fn default() -> Self {
 DefaultScorer::new(100)
@TheBlueMattTheBlueMattOct 15, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think default should be 500, just kinda gut-feel. Lots of "good" nodes charge 1 sat per payment, and we should probably be willing to prefer that over two-three "free" hops IMO. Maybe even 1k. We can tune this later, but 100 just seems very low.
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to 500 msats.
jkczyz
jkczyz
commented
Oct 15, 2021
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test but was getting a little confused with the fee_msat assertions. Let me know if it looks sane. I could add more tests if needed.
Comment threadlightning/src/routing/router.rs
Outdated

	path_htlc_minimum_msat: u64,
	/// All penalties incurred *after* this hop on the way to the destination, as calculated using
	/// channel scoring.
	next_hops_penalty_msat: u64,
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. I was a little confused by the naming, but now I see the pattern. Should be consistent now.
Comment threadlightning/src/ln/functional_tests.rs
Outdated

use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT;
use routing::network_graph::{NetworkUpdate, RoutingFees};
use routing::router::{Route, RouteHop, RouteHint, RouteHintHop, get_route, get_keysend_route};
use routing::scorer::DefaultScorer;
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. I removed TestScorer and renamed DefaultScorer to Scorer, using it with 0 in all tests excepted for the newly added one.
Comment threadlightning/src/routing/scorer.rs
Outdated

impl Default for DefaultScorer {
	/// Creates a new scorer using 100 msat as the channel penalty.
	fn default() -> Self {
 DefaultScorer::new(100)
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to 500 msats.
valentinewallace
valentinewallace
reviewed
Oct 15, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to get an initial version of this in!
Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/mod.rs
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/scorer.rs
Outdated

//!
//! # Example
//!
//! TODO: Fill in example once interface has been finalized.
@valentinewallacevalentinewallaceOct 15, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're good to fill this in?
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Not much to it yet, but could expand if we add more configurations. Also, may want to have an example with InvoicePayer for event handling once that is in. Or at least reference those docs.
valentinewallace
valentinewallace
approved these changes
Oct 15, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀
@TheBlueMatt
TheBlueMatt
commented
Oct 15, 2021

Copy link


Copy Markdown

Collaborator
Can you squash :).
@jkczyz
Add channel scoring to get_route
e15a18a
Failed payments may be retried, but calling get_route may return a Route
with the same failing path. Add a routing::Score trait used to
parameterize get_route, which it calls to determine how much a channel
should be penalized in terms of msats willing to pay to avoid the
channel.
Also, add a Scorer struct that implements routing::Score with a constant
constant penalty. Subsequent changes will allow for more robust scoring
by feeding back payment path success and failure to the scorer via event
handling.
@jkczyz
jkczyzforce-pushed
the
2021-10-default-channel-scoring
branch
from
84c0a48 to
e15a18aCompareOctober 15, 2021 20:31
TheBlueMatt
TheBlueMatt
approved these changes
Oct 15, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff since val's ack was just a squash:
$ git diff-tree -U1 84c0a486e e15a18a50 $

@TheBlueMatt
TheBlueMatt
merged commit 2398f17
into
lightningdevkit:mainOct 16, 2021
@jkczyzjkczyz
mentioned this pull request
Oct 30, 2021
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@valentinewallacevalentinewallacevalentinewallace approved these changes
@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.102

Development

Successfully merging this pull request may close these issues.

3 participants

@jkczyz@TheBlueMatt@valentinewallace

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add channel scoring to get_route - #1124
Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
jkczyz:2021-10-default-channel-scoring
Oct 16, 2021
Merged
Add channel scoring to get_route#1124
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
jkczyz:2021-10-default-channel-scoring
Conversation
@jkczyz
@jkczyzjkczyz
commented
Oct 14, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
Failed payments may be retried, but calling get_route may return a Route with the same failing path. Add a routing::Score trait used to parameterize get_route, which it calls to determine how much a channel should be penalized in terms of msats willing to pay to avoid the channel.
Also, add a Scorer struct that implements routing::Score with a constant penalty. Subsequent changes will allow for more robust scoring by feeding back payment path success and failure to the scorer via event handling.
@codecov
codecovBot
commented
Oct 14, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1124 (84c0a48) into main (da498d7) will increase coverage by 0.76%.

The diff coverage is 91.52%.
❗ Current head 84c0a48 differs from pull request most recent head e15a18a. Consider uploading reports for the commit e15a18a to get more accurate results
Impacted file tree graph
@@ Coverage Diff @@## main #1124 +/- ##
==========================================
+ Coverage 90.60% 91.36% +0.76% 
==========================================
Files 66 67 +1 Lines 34474 37698 +3224 ==========================================
+ Hits 31235 34443 +3208 - Misses 3239 3255 +16 
Impacted FilesCoverage Δ
lightning/src/routing/scorer.rs57.14% <57.14%> (ø)
lightning/src/routing/router.rs95.70% <91.78%> (-0.30%)⬇️
lightning-invoice/src/utils.rs84.26% <100.00%> (+0.17%)⬆️
lightning/src/ln/channelmanager.rs85.14% <100.00%> (+0.01%)⬆️
lightning/src/ln/functional_test_utils.rs97.32% <100.00%> (+2.24%)⬆️
lightning/src/ln/functional_tests.rs97.93% <100.00%> (+0.53%)⬆️
lightning/src/ln/shutdown_tests.rs95.87% <100.00%> (+<0.01%)⬆️
lightning/src/chain/mod.rs50.00% <0.00%> (-8.83%)⬇️
lightning-background-processor/src/lib.rs94.38% <0.00%> (+0.15%)⬆️
... 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 da498d7...e15a18a. Read the comment docs.
@TheBlueMattTheBlueMatt
added this to the 0.0.102 milestone
Oct 14, 2021
TheBlueMatt
TheBlueMatt
reviewed
Oct 14, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmarks are probably about what I'd expect given the expansion of of in-cmp logic. Something to look into eventually, probably:
Latest git
test ln::channelmanager::bench::bench_sends ... bench: 9,128,177 ns/iter (+/- 1,349,843)
test routing::network_graph::benches::read_network_graph ... bench: 1,777,566,078 ns/iter (+/- 23,897,528)
test routing::network_graph::benches::write_network_graph ... bench: 159,355,206 ns/iter (+/- 5,292,557)
test routing::router::benches::generate_mpp_routes ... bench: 74,852,606 ns/iter (+/- 55,662,508)
test routing::router::benches::generate_routes ... bench: 72,321,963 ns/iter (+/- 56,925,293)
test bench::bench_sends ... bench: 211,670,987 ns/iter (+/- 188,695,358)

This PR:
test ln::channelmanager::bench::bench_sends ... bench: 8,284,024 ns/iter (+/- 443,276)
test routing::network_graph::benches::read_network_graph ... bench: 1,614,888,492 ns/iter (+/- 2,280,811)
test routing::network_graph::benches::write_network_graph ... bench: 143,246,612 ns/iter (+/- 458,059)
test routing::router::benches::generate_mpp_routes ... bench: 96,742,965 ns/iter (+/- 52,848,627)
test routing::router::benches::generate_routes ... bench: 96,963,520 ns/iter (+/- 62,975,254)
test bench::bench_sends ... bench: 190,069,734 ns/iter (+/- 168,900,474)

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/mod.rs
Outdated

Comment threadlightning/src/routing/mod.rs
Outdated

@TheBlueMatt
TheBlueMatt
commented
Oct 14, 2021

Copy link


Copy Markdown

Collaborator
Note for comparison bench for 0.0.101 was (there's obviously a ton of noise in these numbers from github CI).
test ln::channelmanager::bench::bench_sends ... bench: 7,587,098 ns/iter (+/- 1,026,747)
test routing::network_graph::benches::read_network_graph ... bench: 2,195,464,293 ns/iter (+/- 116,402,530)
test routing::network_graph::benches::write_network_graph ... bench: 132,630,621 ns/iter (+/- 11,344,574)
test routing::router::benches::generate_mpp_routes ... bench: 82,229,661 ns/iter (+/- 48,137,908)
test routing::router::benches::generate_routes ... bench: 83,419,028 ns/iter (+/- 62,531,148)
test bench::bench_sends ... bench: 175,605,757 ns/iter (+/- 148,418,861)

TheBlueMatt
TheBlueMatt
reviewed
Oct 14, 2021
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Oct 14, 2021
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

@jkczyz
jkczyz
commented
Oct 14, 2021

Copy link


Copy Markdown

ContributorAuthor
All good feedback! Thanks for divining into benchmarks. All comments addressed.
@TheBlueMatt
TheBlueMatt
commented
Oct 14, 2021

Copy link


Copy Markdown

Collaborator
Github CI now says (which is, uhmmmm, faster than latest git? yea, no, that's just cause github's CI machines are clearly very heterogeneous....last time I look at that output.........)
test ln::channelmanager::bench::bench_sends ... bench: 8,363,865 ns/iter (+/- 1,362,103)
test routing::network_graph::benches::read_network_graph ... bench: 1,626,468,025 ns/iter (+/- 90,985,613)
test routing::network_graph::benches::write_network_graph ... bench: 142,291,900 ns/iter (+/- 6,987,924)
test routing::router::benches::generate_mpp_routes ... bench: 57,154,598 ns/iter (+/- 32,542,178)
test routing::router::benches::generate_routes ... bench: 57,344,572 ns/iter (+/- 39,866,175)
test bench::bench_sends ... bench: 197,672,739 ns/iter (+/- 170,050,060)

TheBlueMatt
TheBlueMatt
approved these changes
Oct 15, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs tests but otherwise good.
Comment threadlightning/src/routing/router.rs
Outdated

	path_htlc_minimum_msat: u64,
	/// All penalties incurred *after* this hop on the way to the destination, as calculated using
	/// channel scoring.
	next_hops_penalty_msat: u64,
@TheBlueMattTheBlueMattOct 15, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of next_hops in these confused me somewhat - next_hops_fee_msat (both in a PathBuildingHop and in the add_node macro) refers to only fee after the hop but not including the fee charged at the hop, whereas here we're using it to include the penalty charged at a hop. Maybe path_penalty_msat or so?
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. I was a little confused by the naming, but now I see the pattern. Should be consistent now.
Comment threadlightning/src/ln/functional_tests.rs
Outdated

use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT;
use routing::network_graph::{NetworkUpdate, RoutingFees};
use routing::router::{Route, RouteHop, RouteHint, RouteHintHop, get_route, get_keysend_route};
use routing::scorer::DefaultScorer;
@TheBlueMattTheBlueMattOct 15, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use either a TestScorer in tests or a DefaultScorer with a score of 0? I find it a bit...awkward? to change all of our routing parameters in all our tests, even if they still pass it kinda makes me nervous.
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. I removed TestScorer and renamed DefaultScorer to Scorer, using it with 0 in all tests excepted for the newly added one.
Comment threadlightning/src/routing/scorer.rs
Outdated

impl Default for DefaultScorer {
	/// Creates a new scorer using 100 msat as the channel penalty.
	fn default() -> Self {
 DefaultScorer::new(100)
@TheBlueMattTheBlueMattOct 15, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think default should be 500, just kinda gut-feel. Lots of "good" nodes charge 1 sat per payment, and we should probably be willing to prefer that over two-three "free" hops IMO. Maybe even 1k. We can tune this later, but 100 just seems very low.
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to 500 msats.
jkczyz
jkczyz
commented
Oct 15, 2021
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test but was getting a little confused with the fee_msat assertions. Let me know if it looks sane. I could add more tests if needed.
Comment threadlightning/src/routing/router.rs
Outdated

	path_htlc_minimum_msat: u64,
	/// All penalties incurred *after* this hop on the way to the destination, as calculated using
	/// channel scoring.
	next_hops_penalty_msat: u64,
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. I was a little confused by the naming, but now I see the pattern. Should be consistent now.
Comment threadlightning/src/ln/functional_tests.rs
Outdated

use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT;
use routing::network_graph::{NetworkUpdate, RoutingFees};
use routing::router::{Route, RouteHop, RouteHint, RouteHintHop, get_route, get_keysend_route};
use routing::scorer::DefaultScorer;
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. I removed TestScorer and renamed DefaultScorer to Scorer, using it with 0 in all tests excepted for the newly added one.
Comment threadlightning/src/routing/scorer.rs
Outdated

impl Default for DefaultScorer {
	/// Creates a new scorer using 100 msat as the channel penalty.
	fn default() -> Self {
 DefaultScorer::new(100)
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to 500 msats.
valentinewallace
valentinewallace
reviewed
Oct 15, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to get an initial version of this in!
Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/mod.rs
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/scorer.rs
Outdated

//!
//! # Example
//!
//! TODO: Fill in example once interface has been finalized.
@valentinewallacevalentinewallaceOct 15, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're good to fill this in?
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Not much to it yet, but could expand if we add more configurations. Also, may want to have an example with InvoicePayer for event handling once that is in. Or at least reference those docs.
valentinewallace
valentinewallace
approved these changes
Oct 15, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀
@TheBlueMatt
TheBlueMatt
commented
Oct 15, 2021

Copy link


Copy Markdown

Collaborator
Can you squash :).
@jkczyz
Add channel scoring to get_route
e15a18a
Failed payments may be retried, but calling get_route may return a Route
with the same failing path. Add a routing::Score trait used to
parameterize get_route, which it calls to determine how much a channel
should be penalized in terms of msats willing to pay to avoid the
channel.
Also, add a Scorer struct that implements routing::Score with a constant
constant penalty. Subsequent changes will allow for more robust scoring
by feeding back payment path success and failure to the scorer via event
handling.
@jkczyz
jkczyzforce-pushed
the
2021-10-default-channel-scoring
branch
from
84c0a48 to
e15a18aCompareOctober 15, 2021 20:31
TheBlueMatt
TheBlueMatt
approved these changes
Oct 15, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff since val's ack was just a squash:
$ git diff-tree -U1 84c0a486e e15a18a50 $

@TheBlueMatt
TheBlueMatt
merged commit 2398f17
into
lightningdevkit:mainOct 16, 2021
@jkczyzjkczyz
mentioned this pull request
Oct 30, 2021
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@valentinewallacevalentinewallacevalentinewallace approved these changes
@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.102

Development

Successfully merging this pull request may close these issues.

3 participants

@jkczyz@TheBlueMatt@valentinewallace

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add channel scoring to get_route - #1124
Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
jkczyz:2021-10-default-channel-scoring
Oct 16, 2021
Merged
Add channel scoring to get_route#1124
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
jkczyz:2021-10-default-channel-scoring
Conversation
@jkczyz
@jkczyzjkczyz
commented
Oct 14, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
Failed payments may be retried, but calling get_route may return a Route with the same failing path. Add a routing::Score trait used to parameterize get_route, which it calls to determine how much a channel should be penalized in terms of msats willing to pay to avoid the channel.
Also, add a Scorer struct that implements routing::Score with a constant penalty. Subsequent changes will allow for more robust scoring by feeding back payment path success and failure to the scorer via event handling.
@codecov
codecovBot
commented
Oct 14, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1124 (84c0a48) into main (da498d7) will increase coverage by 0.76%.

The diff coverage is 91.52%.
❗ Current head 84c0a48 differs from pull request most recent head e15a18a. Consider uploading reports for the commit e15a18a to get more accurate results
Impacted file tree graph
@@ Coverage Diff @@## main #1124 +/- ##
==========================================
+ Coverage 90.60% 91.36% +0.76% 
==========================================
Files 66 67 +1 Lines 34474 37698 +3224 ==========================================
+ Hits 31235 34443 +3208 - Misses 3239 3255 +16 
Impacted FilesCoverage Δ
lightning/src/routing/scorer.rs57.14% <57.14%> (ø)
lightning/src/routing/router.rs95.70% <91.78%> (-0.30%)⬇️
lightning-invoice/src/utils.rs84.26% <100.00%> (+0.17%)⬆️
lightning/src/ln/channelmanager.rs85.14% <100.00%> (+0.01%)⬆️
lightning/src/ln/functional_test_utils.rs97.32% <100.00%> (+2.24%)⬆️
lightning/src/ln/functional_tests.rs97.93% <100.00%> (+0.53%)⬆️
lightning/src/ln/shutdown_tests.rs95.87% <100.00%> (+<0.01%)⬆️
lightning/src/chain/mod.rs50.00% <0.00%> (-8.83%)⬇️
lightning-background-processor/src/lib.rs94.38% <0.00%> (+0.15%)⬆️
... 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 da498d7...e15a18a. Read the comment docs.
@TheBlueMattTheBlueMatt
added this to the 0.0.102 milestone
Oct 14, 2021
TheBlueMatt
TheBlueMatt
reviewed
Oct 14, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmarks are probably about what I'd expect given the expansion of of in-cmp logic. Something to look into eventually, probably:
Latest git
test ln::channelmanager::bench::bench_sends ... bench: 9,128,177 ns/iter (+/- 1,349,843)
test routing::network_graph::benches::read_network_graph ... bench: 1,777,566,078 ns/iter (+/- 23,897,528)
test routing::network_graph::benches::write_network_graph ... bench: 159,355,206 ns/iter (+/- 5,292,557)
test routing::router::benches::generate_mpp_routes ... bench: 74,852,606 ns/iter (+/- 55,662,508)
test routing::router::benches::generate_routes ... bench: 72,321,963 ns/iter (+/- 56,925,293)
test bench::bench_sends ... bench: 211,670,987 ns/iter (+/- 188,695,358)

This PR:
test ln::channelmanager::bench::bench_sends ... bench: 8,284,024 ns/iter (+/- 443,276)
test routing::network_graph::benches::read_network_graph ... bench: 1,614,888,492 ns/iter (+/- 2,280,811)
test routing::network_graph::benches::write_network_graph ... bench: 143,246,612 ns/iter (+/- 458,059)
test routing::router::benches::generate_mpp_routes ... bench: 96,742,965 ns/iter (+/- 52,848,627)
test routing::router::benches::generate_routes ... bench: 96,963,520 ns/iter (+/- 62,975,254)
test bench::bench_sends ... bench: 190,069,734 ns/iter (+/- 168,900,474)

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/mod.rs
Outdated

Comment threadlightning/src/routing/mod.rs
Outdated

@TheBlueMatt
TheBlueMatt
commented
Oct 14, 2021

Copy link


Copy Markdown

Collaborator
Note for comparison bench for 0.0.101 was (there's obviously a ton of noise in these numbers from github CI).
test ln::channelmanager::bench::bench_sends ... bench: 7,587,098 ns/iter (+/- 1,026,747)
test routing::network_graph::benches::read_network_graph ... bench: 2,195,464,293 ns/iter (+/- 116,402,530)
test routing::network_graph::benches::write_network_graph ... bench: 132,630,621 ns/iter (+/- 11,344,574)
test routing::router::benches::generate_mpp_routes ... bench: 82,229,661 ns/iter (+/- 48,137,908)
test routing::router::benches::generate_routes ... bench: 83,419,028 ns/iter (+/- 62,531,148)
test bench::bench_sends ... bench: 175,605,757 ns/iter (+/- 148,418,861)

TheBlueMatt
TheBlueMatt
reviewed
Oct 14, 2021
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Oct 14, 2021
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

@jkczyz
jkczyz
commented
Oct 14, 2021

Copy link


Copy Markdown

ContributorAuthor
All good feedback! Thanks for divining into benchmarks. All comments addressed.
@TheBlueMatt
TheBlueMatt
commented
Oct 14, 2021

Copy link


Copy Markdown

Collaborator
Github CI now says (which is, uhmmmm, faster than latest git? yea, no, that's just cause github's CI machines are clearly very heterogeneous....last time I look at that output.........)
test ln::channelmanager::bench::bench_sends ... bench: 8,363,865 ns/iter (+/- 1,362,103)
test routing::network_graph::benches::read_network_graph ... bench: 1,626,468,025 ns/iter (+/- 90,985,613)
test routing::network_graph::benches::write_network_graph ... bench: 142,291,900 ns/iter (+/- 6,987,924)
test routing::router::benches::generate_mpp_routes ... bench: 57,154,598 ns/iter (+/- 32,542,178)
test routing::router::benches::generate_routes ... bench: 57,344,572 ns/iter (+/- 39,866,175)
test bench::bench_sends ... bench: 197,672,739 ns/iter (+/- 170,050,060)

TheBlueMatt
TheBlueMatt
approved these changes
Oct 15, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs tests but otherwise good.
Comment threadlightning/src/routing/router.rs
Outdated

	path_htlc_minimum_msat: u64,
	/// All penalties incurred *after* this hop on the way to the destination, as calculated using
	/// channel scoring.
	next_hops_penalty_msat: u64,
@TheBlueMattTheBlueMattOct 15, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of next_hops in these confused me somewhat - next_hops_fee_msat (both in a PathBuildingHop and in the add_node macro) refers to only fee after the hop but not including the fee charged at the hop, whereas here we're using it to include the penalty charged at a hop. Maybe path_penalty_msat or so?
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. I was a little confused by the naming, but now I see the pattern. Should be consistent now.
Comment threadlightning/src/ln/functional_tests.rs
Outdated

use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT;
use routing::network_graph::{NetworkUpdate, RoutingFees};
use routing::router::{Route, RouteHop, RouteHint, RouteHintHop, get_route, get_keysend_route};
use routing::scorer::DefaultScorer;
@TheBlueMattTheBlueMattOct 15, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use either a TestScorer in tests or a DefaultScorer with a score of 0? I find it a bit...awkward? to change all of our routing parameters in all our tests, even if they still pass it kinda makes me nervous.
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. I removed TestScorer and renamed DefaultScorer to Scorer, using it with 0 in all tests excepted for the newly added one.
Comment threadlightning/src/routing/scorer.rs
Outdated

impl Default for DefaultScorer {
	/// Creates a new scorer using 100 msat as the channel penalty.
	fn default() -> Self {
 DefaultScorer::new(100)
@TheBlueMattTheBlueMattOct 15, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think default should be 500, just kinda gut-feel. Lots of "good" nodes charge 1 sat per payment, and we should probably be willing to prefer that over two-three "free" hops IMO. Maybe even 1k. We can tune this later, but 100 just seems very low.
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to 500 msats.
jkczyz
jkczyz
commented
Oct 15, 2021
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test but was getting a little confused with the fee_msat assertions. Let me know if it looks sane. I could add more tests if needed.
Comment threadlightning/src/routing/router.rs
Outdated

	path_htlc_minimum_msat: u64,
	/// All penalties incurred *after* this hop on the way to the destination, as calculated using
	/// channel scoring.
	next_hops_penalty_msat: u64,
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. I was a little confused by the naming, but now I see the pattern. Should be consistent now.
Comment threadlightning/src/ln/functional_tests.rs
Outdated

use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT;
use routing::network_graph::{NetworkUpdate, RoutingFees};
use routing::router::{Route, RouteHop, RouteHint, RouteHintHop, get_route, get_keysend_route};
use routing::scorer::DefaultScorer;
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. I removed TestScorer and renamed DefaultScorer to Scorer, using it with 0 in all tests excepted for the newly added one.
Comment threadlightning/src/routing/scorer.rs
Outdated

impl Default for DefaultScorer {
	/// Creates a new scorer using 100 msat as the channel penalty.
	fn default() -> Self {
 DefaultScorer::new(100)
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to 500 msats.
valentinewallace
valentinewallace
reviewed
Oct 15, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to get an initial version of this in!
Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/mod.rs
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/scorer.rs
Outdated

//!
//! # Example
//!
//! TODO: Fill in example once interface has been finalized.
@valentinewallacevalentinewallaceOct 15, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're good to fill this in?
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Not much to it yet, but could expand if we add more configurations. Also, may want to have an example with InvoicePayer for event handling once that is in. Or at least reference those docs.
valentinewallace
valentinewallace
approved these changes
Oct 15, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀
@TheBlueMatt
TheBlueMatt
commented
Oct 15, 2021

Copy link


Copy Markdown

Collaborator
Can you squash :).
@jkczyz
Add channel scoring to get_route
e15a18a
Failed payments may be retried, but calling get_route may return a Route
with the same failing path. Add a routing::Score trait used to
parameterize get_route, which it calls to determine how much a channel
should be penalized in terms of msats willing to pay to avoid the
channel.
Also, add a Scorer struct that implements routing::Score with a constant
constant penalty. Subsequent changes will allow for more robust scoring
by feeding back payment path success and failure to the scorer via event
handling.
@jkczyz
jkczyzforce-pushed
the
2021-10-default-channel-scoring
branch
from
84c0a48 to
e15a18aCompareOctober 15, 2021 20:31
TheBlueMatt
TheBlueMatt
approved these changes
Oct 15, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff since val's ack was just a squash:
$ git diff-tree -U1 84c0a486e e15a18a50 $

@TheBlueMatt
TheBlueMatt
merged commit 2398f17
into
lightningdevkit:mainOct 16, 2021
@jkczyzjkczyz
mentioned this pull request
Oct 30, 2021
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@valentinewallacevalentinewallacevalentinewallace approved these changes
@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.102

Development

Successfully merging this pull request may close these issues.

3 participants

@jkczyz@TheBlueMatt@valentinewallace

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Add channel scoring to get_route - #1124
Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
jkczyz:2021-10-default-channel-scoring
Oct 16, 2021
Merged
Add channel scoring to get_route#1124
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
jkczyz:2021-10-default-channel-scoring
Conversation
@jkczyz
@jkczyzjkczyz
commented
Oct 14, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
Failed payments may be retried, but calling get_route may return a Route with the same failing path. Add a routing::Score trait used to parameterize get_route, which it calls to determine how much a channel should be penalized in terms of msats willing to pay to avoid the channel.
Also, add a Scorer struct that implements routing::Score with a constant penalty. Subsequent changes will allow for more robust scoring by feeding back payment path success and failure to the scorer via event handling.
@codecov
codecovBot
commented
Oct 14, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1124 (84c0a48) into main (da498d7) will increase coverage by 0.76%.

The diff coverage is 91.52%.
❗ Current head 84c0a48 differs from pull request most recent head e15a18a. Consider uploading reports for the commit e15a18a to get more accurate results
Impacted file tree graph
@@ Coverage Diff @@## main #1124 +/- ##
==========================================
+ Coverage 90.60% 91.36% +0.76% 
==========================================
Files 66 67 +1 Lines 34474 37698 +3224 ==========================================
+ Hits 31235 34443 +3208 - Misses 3239 3255 +16 
Impacted FilesCoverage Δ
lightning/src/routing/scorer.rs57.14% <57.14%> (ø)
lightning/src/routing/router.rs95.70% <91.78%> (-0.30%)⬇️
lightning-invoice/src/utils.rs84.26% <100.00%> (+0.17%)⬆️
lightning/src/ln/channelmanager.rs85.14% <100.00%> (+0.01%)⬆️
lightning/src/ln/functional_test_utils.rs97.32% <100.00%> (+2.24%)⬆️
lightning/src/ln/functional_tests.rs97.93% <100.00%> (+0.53%)⬆️
lightning/src/ln/shutdown_tests.rs95.87% <100.00%> (+<0.01%)⬆️
lightning/src/chain/mod.rs50.00% <0.00%> (-8.83%)⬇️
lightning-background-processor/src/lib.rs94.38% <0.00%> (+0.15%)⬆️
... 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 da498d7...e15a18a. Read the comment docs.
@TheBlueMattTheBlueMatt
added this to the 0.0.102 milestone
Oct 14, 2021
TheBlueMatt
TheBlueMatt
reviewed
Oct 14, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmarks are probably about what I'd expect given the expansion of of in-cmp logic. Something to look into eventually, probably:
Latest git
test ln::channelmanager::bench::bench_sends ... bench: 9,128,177 ns/iter (+/- 1,349,843)
test routing::network_graph::benches::read_network_graph ... bench: 1,777,566,078 ns/iter (+/- 23,897,528)
test routing::network_graph::benches::write_network_graph ... bench: 159,355,206 ns/iter (+/- 5,292,557)
test routing::router::benches::generate_mpp_routes ... bench: 74,852,606 ns/iter (+/- 55,662,508)
test routing::router::benches::generate_routes ... bench: 72,321,963 ns/iter (+/- 56,925,293)
test bench::bench_sends ... bench: 211,670,987 ns/iter (+/- 188,695,358)

This PR:
test ln::channelmanager::bench::bench_sends ... bench: 8,284,024 ns/iter (+/- 443,276)
test routing::network_graph::benches::read_network_graph ... bench: 1,614,888,492 ns/iter (+/- 2,280,811)
test routing::network_graph::benches::write_network_graph ... bench: 143,246,612 ns/iter (+/- 458,059)
test routing::router::benches::generate_mpp_routes ... bench: 96,742,965 ns/iter (+/- 52,848,627)
test routing::router::benches::generate_routes ... bench: 96,963,520 ns/iter (+/- 62,975,254)
test bench::bench_sends ... bench: 190,069,734 ns/iter (+/- 168,900,474)

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/mod.rs
Outdated

Comment threadlightning/src/routing/mod.rs
Outdated

@TheBlueMatt
TheBlueMatt
commented
Oct 14, 2021

Copy link


Copy Markdown

Collaborator
Note for comparison bench for 0.0.101 was (there's obviously a ton of noise in these numbers from github CI).
test ln::channelmanager::bench::bench_sends ... bench: 7,587,098 ns/iter (+/- 1,026,747)
test routing::network_graph::benches::read_network_graph ... bench: 2,195,464,293 ns/iter (+/- 116,402,530)
test routing::network_graph::benches::write_network_graph ... bench: 132,630,621 ns/iter (+/- 11,344,574)
test routing::router::benches::generate_mpp_routes ... bench: 82,229,661 ns/iter (+/- 48,137,908)
test routing::router::benches::generate_routes ... bench: 83,419,028 ns/iter (+/- 62,531,148)
test bench::bench_sends ... bench: 175,605,757 ns/iter (+/- 148,418,861)

TheBlueMatt
TheBlueMatt
reviewed
Oct 14, 2021
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Oct 14, 2021
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

@jkczyz
jkczyz
commented
Oct 14, 2021

Copy link


Copy Markdown

ContributorAuthor
All good feedback! Thanks for divining into benchmarks. All comments addressed.
@TheBlueMatt
TheBlueMatt
commented
Oct 14, 2021

Copy link


Copy Markdown

Collaborator
Github CI now says (which is, uhmmmm, faster than latest git? yea, no, that's just cause github's CI machines are clearly very heterogeneous....last time I look at that output.........)
test ln::channelmanager::bench::bench_sends ... bench: 8,363,865 ns/iter (+/- 1,362,103)
test routing::network_graph::benches::read_network_graph ... bench: 1,626,468,025 ns/iter (+/- 90,985,613)
test routing::network_graph::benches::write_network_graph ... bench: 142,291,900 ns/iter (+/- 6,987,924)
test routing::router::benches::generate_mpp_routes ... bench: 57,154,598 ns/iter (+/- 32,542,178)
test routing::router::benches::generate_routes ... bench: 57,344,572 ns/iter (+/- 39,866,175)
test bench::bench_sends ... bench: 197,672,739 ns/iter (+/- 170,050,060)

TheBlueMatt
TheBlueMatt
approved these changes
Oct 15, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs tests but otherwise good.
Comment threadlightning/src/routing/router.rs
Outdated

	path_htlc_minimum_msat: u64,
	/// All penalties incurred *after* this hop on the way to the destination, as calculated using
	/// channel scoring.
	next_hops_penalty_msat: u64,
@TheBlueMattTheBlueMattOct 15, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of next_hops in these confused me somewhat - next_hops_fee_msat (both in a PathBuildingHop and in the add_node macro) refers to only fee after the hop but not including the fee charged at the hop, whereas here we're using it to include the penalty charged at a hop. Maybe path_penalty_msat or so?
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. I was a little confused by the naming, but now I see the pattern. Should be consistent now.
Comment threadlightning/src/ln/functional_tests.rs
Outdated

use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT;
use routing::network_graph::{NetworkUpdate, RoutingFees};
use routing::router::{Route, RouteHop, RouteHint, RouteHintHop, get_route, get_keysend_route};
use routing::scorer::DefaultScorer;
@TheBlueMattTheBlueMattOct 15, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use either a TestScorer in tests or a DefaultScorer with a score of 0? I find it a bit...awkward? to change all of our routing parameters in all our tests, even if they still pass it kinda makes me nervous.
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. I removed TestScorer and renamed DefaultScorer to Scorer, using it with 0 in all tests excepted for the newly added one.
Comment threadlightning/src/routing/scorer.rs
Outdated

impl Default for DefaultScorer {
	/// Creates a new scorer using 100 msat as the channel penalty.
	fn default() -> Self {
 DefaultScorer::new(100)
@TheBlueMattTheBlueMattOct 15, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think default should be 500, just kinda gut-feel. Lots of "good" nodes charge 1 sat per payment, and we should probably be willing to prefer that over two-three "free" hops IMO. Maybe even 1k. We can tune this later, but 100 just seems very low.
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to 500 msats.
jkczyz
jkczyz
commented
Oct 15, 2021
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test but was getting a little confused with the fee_msat assertions. Let me know if it looks sane. I could add more tests if needed.
Comment threadlightning/src/routing/router.rs
Outdated

	path_htlc_minimum_msat: u64,
	/// All penalties incurred *after* this hop on the way to the destination, as calculated using
	/// channel scoring.
	next_hops_penalty_msat: u64,
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. I was a little confused by the naming, but now I see the pattern. Should be consistent now.
Comment threadlightning/src/ln/functional_tests.rs
Outdated

use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT;
use routing::network_graph::{NetworkUpdate, RoutingFees};
use routing::router::{Route, RouteHop, RouteHint, RouteHintHop, get_route, get_keysend_route};
use routing::scorer::DefaultScorer;
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. I removed TestScorer and renamed DefaultScorer to Scorer, using it with 0 in all tests excepted for the newly added one.
Comment threadlightning/src/routing/scorer.rs
Outdated

impl Default for DefaultScorer {
	/// Creates a new scorer using 100 msat as the channel penalty.
	fn default() -> Self {
 DefaultScorer::new(100)
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to 500 msats.
valentinewallace
valentinewallace
reviewed
Oct 15, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to get an initial version of this in!
Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/mod.rs
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/scorer.rs
Outdated

//!
//! # Example
//!
//! TODO: Fill in example once interface has been finalized.
@valentinewallacevalentinewallaceOct 15, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're good to fill this in?
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Not much to it yet, but could expand if we add more configurations. Also, may want to have an example with InvoicePayer for event handling once that is in. Or at least reference those docs.
valentinewallace
valentinewallace
approved these changes
Oct 15, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀
@TheBlueMatt
TheBlueMatt
commented
Oct 15, 2021

Copy link


Copy Markdown

Collaborator
Can you squash :).
@jkczyz
Add channel scoring to get_route
e15a18a
Failed payments may be retried, but calling get_route may return a Route
with the same failing path. Add a routing::Score trait used to
parameterize get_route, which it calls to determine how much a channel
should be penalized in terms of msats willing to pay to avoid the
channel.
Also, add a Scorer struct that implements routing::Score with a constant
constant penalty. Subsequent changes will allow for more robust scoring
by feeding back payment path success and failure to the scorer via event
handling.
@jkczyz
jkczyzforce-pushed
the
2021-10-default-channel-scoring
branch
from
84c0a48 to
e15a18aCompareOctober 15, 2021 20:31
TheBlueMatt
TheBlueMatt
approved these changes
Oct 15, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff since val's ack was just a squash:
$ git diff-tree -U1 84c0a486e e15a18a50 $

@TheBlueMatt
TheBlueMatt
merged commit 2398f17
into
lightningdevkit:mainOct 16, 2021
@jkczyzjkczyz
mentioned this pull request
Oct 30, 2021
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@valentinewallacevalentinewallacevalentinewallace approved these changes
@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.102

Development

Successfully merging this pull request may close these issues.

3 participants

@jkczyz@TheBlueMatt@valentinewallace

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add channel scoring to get_route - #1124
Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
jkczyz:2021-10-default-channel-scoring
Oct 16, 2021
Merged
Add channel scoring to get_route#1124
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
jkczyz:2021-10-default-channel-scoring
Conversation
@jkczyz
@jkczyzjkczyz
commented
Oct 14, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
Failed payments may be retried, but calling get_route may return a Route with the same failing path. Add a routing::Score trait used to parameterize get_route, which it calls to determine how much a channel should be penalized in terms of msats willing to pay to avoid the channel.
Also, add a Scorer struct that implements routing::Score with a constant penalty. Subsequent changes will allow for more robust scoring by feeding back payment path success and failure to the scorer via event handling.
@codecov
codecovBot
commented
Oct 14, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1124 (84c0a48) into main (da498d7) will increase coverage by 0.76%.

The diff coverage is 91.52%.
❗ Current head 84c0a48 differs from pull request most recent head e15a18a. Consider uploading reports for the commit e15a18a to get more accurate results
Impacted file tree graph
@@ Coverage Diff @@## main #1124 +/- ##
==========================================
+ Coverage 90.60% 91.36% +0.76% 
==========================================
Files 66 67 +1 Lines 34474 37698 +3224 ==========================================
+ Hits 31235 34443 +3208 - Misses 3239 3255 +16 
Impacted FilesCoverage Δ
lightning/src/routing/scorer.rs57.14% <57.14%> (ø)
lightning/src/routing/router.rs95.70% <91.78%> (-0.30%)⬇️
lightning-invoice/src/utils.rs84.26% <100.00%> (+0.17%)⬆️
lightning/src/ln/channelmanager.rs85.14% <100.00%> (+0.01%)⬆️
lightning/src/ln/functional_test_utils.rs97.32% <100.00%> (+2.24%)⬆️
lightning/src/ln/functional_tests.rs97.93% <100.00%> (+0.53%)⬆️
lightning/src/ln/shutdown_tests.rs95.87% <100.00%> (+<0.01%)⬆️
lightning/src/chain/mod.rs50.00% <0.00%> (-8.83%)⬇️
lightning-background-processor/src/lib.rs94.38% <0.00%> (+0.15%)⬆️
... 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 da498d7...e15a18a. Read the comment docs.
@TheBlueMattTheBlueMatt
added this to the 0.0.102 milestone
Oct 14, 2021
TheBlueMatt
TheBlueMatt
reviewed
Oct 14, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmarks are probably about what I'd expect given the expansion of of in-cmp logic. Something to look into eventually, probably:
Latest git
test ln::channelmanager::bench::bench_sends ... bench: 9,128,177 ns/iter (+/- 1,349,843)
test routing::network_graph::benches::read_network_graph ... bench: 1,777,566,078 ns/iter (+/- 23,897,528)
test routing::network_graph::benches::write_network_graph ... bench: 159,355,206 ns/iter (+/- 5,292,557)
test routing::router::benches::generate_mpp_routes ... bench: 74,852,606 ns/iter (+/- 55,662,508)
test routing::router::benches::generate_routes ... bench: 72,321,963 ns/iter (+/- 56,925,293)
test bench::bench_sends ... bench: 211,670,987 ns/iter (+/- 188,695,358)

This PR:
test ln::channelmanager::bench::bench_sends ... bench: 8,284,024 ns/iter (+/- 443,276)
test routing::network_graph::benches::read_network_graph ... bench: 1,614,888,492 ns/iter (+/- 2,280,811)
test routing::network_graph::benches::write_network_graph ... bench: 143,246,612 ns/iter (+/- 458,059)
test routing::router::benches::generate_mpp_routes ... bench: 96,742,965 ns/iter (+/- 52,848,627)
test routing::router::benches::generate_routes ... bench: 96,963,520 ns/iter (+/- 62,975,254)
test bench::bench_sends ... bench: 190,069,734 ns/iter (+/- 168,900,474)

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/mod.rs
Outdated

Comment threadlightning/src/routing/mod.rs
Outdated

@TheBlueMatt
TheBlueMatt
commented
Oct 14, 2021

Copy link


Copy Markdown

Collaborator
Note for comparison bench for 0.0.101 was (there's obviously a ton of noise in these numbers from github CI).
test ln::channelmanager::bench::bench_sends ... bench: 7,587,098 ns/iter (+/- 1,026,747)
test routing::network_graph::benches::read_network_graph ... bench: 2,195,464,293 ns/iter (+/- 116,402,530)
test routing::network_graph::benches::write_network_graph ... bench: 132,630,621 ns/iter (+/- 11,344,574)
test routing::router::benches::generate_mpp_routes ... bench: 82,229,661 ns/iter (+/- 48,137,908)
test routing::router::benches::generate_routes ... bench: 83,419,028 ns/iter (+/- 62,531,148)
test bench::bench_sends ... bench: 175,605,757 ns/iter (+/- 148,418,861)

TheBlueMatt
TheBlueMatt
reviewed
Oct 14, 2021
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Oct 14, 2021
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

@jkczyz
jkczyz
commented
Oct 14, 2021

Copy link


Copy Markdown

ContributorAuthor
All good feedback! Thanks for divining into benchmarks. All comments addressed.
@TheBlueMatt
TheBlueMatt
commented
Oct 14, 2021

Copy link


Copy Markdown

Collaborator
Github CI now says (which is, uhmmmm, faster than latest git? yea, no, that's just cause github's CI machines are clearly very heterogeneous....last time I look at that output.........)
test ln::channelmanager::bench::bench_sends ... bench: 8,363,865 ns/iter (+/- 1,362,103)
test routing::network_graph::benches::read_network_graph ... bench: 1,626,468,025 ns/iter (+/- 90,985,613)
test routing::network_graph::benches::write_network_graph ... bench: 142,291,900 ns/iter (+/- 6,987,924)
test routing::router::benches::generate_mpp_routes ... bench: 57,154,598 ns/iter (+/- 32,542,178)
test routing::router::benches::generate_routes ... bench: 57,344,572 ns/iter (+/- 39,866,175)
test bench::bench_sends ... bench: 197,672,739 ns/iter (+/- 170,050,060)

TheBlueMatt
TheBlueMatt
approved these changes
Oct 15, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs tests but otherwise good.
Comment threadlightning/src/routing/router.rs
Outdated

	path_htlc_minimum_msat: u64,
	/// All penalties incurred *after* this hop on the way to the destination, as calculated using
	/// channel scoring.
	next_hops_penalty_msat: u64,
@TheBlueMattTheBlueMattOct 15, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of next_hops in these confused me somewhat - next_hops_fee_msat (both in a PathBuildingHop and in the add_node macro) refers to only fee after the hop but not including the fee charged at the hop, whereas here we're using it to include the penalty charged at a hop. Maybe path_penalty_msat or so?
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. I was a little confused by the naming, but now I see the pattern. Should be consistent now.
Comment threadlightning/src/ln/functional_tests.rs
Outdated

use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT;
use routing::network_graph::{NetworkUpdate, RoutingFees};
use routing::router::{Route, RouteHop, RouteHint, RouteHintHop, get_route, get_keysend_route};
use routing::scorer::DefaultScorer;
@TheBlueMattTheBlueMattOct 15, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use either a TestScorer in tests or a DefaultScorer with a score of 0? I find it a bit...awkward? to change all of our routing parameters in all our tests, even if they still pass it kinda makes me nervous.
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. I removed TestScorer and renamed DefaultScorer to Scorer, using it with 0 in all tests excepted for the newly added one.
Comment threadlightning/src/routing/scorer.rs
Outdated

impl Default for DefaultScorer {
	/// Creates a new scorer using 100 msat as the channel penalty.
	fn default() -> Self {
 DefaultScorer::new(100)
@TheBlueMattTheBlueMattOct 15, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think default should be 500, just kinda gut-feel. Lots of "good" nodes charge 1 sat per payment, and we should probably be willing to prefer that over two-three "free" hops IMO. Maybe even 1k. We can tune this later, but 100 just seems very low.
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to 500 msats.
jkczyz
jkczyz
commented
Oct 15, 2021
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test but was getting a little confused with the fee_msat assertions. Let me know if it looks sane. I could add more tests if needed.
Comment threadlightning/src/routing/router.rs
Outdated

	path_htlc_minimum_msat: u64,
	/// All penalties incurred *after* this hop on the way to the destination, as calculated using
	/// channel scoring.
	next_hops_penalty_msat: u64,
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. I was a little confused by the naming, but now I see the pattern. Should be consistent now.
Comment threadlightning/src/ln/functional_tests.rs
Outdated

use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT;
use routing::network_graph::{NetworkUpdate, RoutingFees};
use routing::router::{Route, RouteHop, RouteHint, RouteHintHop, get_route, get_keysend_route};
use routing::scorer::DefaultScorer;
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. I removed TestScorer and renamed DefaultScorer to Scorer, using it with 0 in all tests excepted for the newly added one.
Comment threadlightning/src/routing/scorer.rs
Outdated

impl Default for DefaultScorer {
	/// Creates a new scorer using 100 msat as the channel penalty.
	fn default() -> Self {
 DefaultScorer::new(100)
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to 500 msats.
valentinewallace
valentinewallace
reviewed
Oct 15, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to get an initial version of this in!
Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/mod.rs
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/scorer.rs
Outdated

//!
//! # Example
//!
//! TODO: Fill in example once interface has been finalized.
@valentinewallacevalentinewallaceOct 15, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're good to fill this in?
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Not much to it yet, but could expand if we add more configurations. Also, may want to have an example with InvoicePayer for event handling once that is in. Or at least reference those docs.
valentinewallace
valentinewallace
approved these changes
Oct 15, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀
@TheBlueMatt
TheBlueMatt
commented
Oct 15, 2021

Copy link


Copy Markdown

Collaborator
Can you squash :).
@jkczyz
Add channel scoring to get_route
e15a18a
Failed payments may be retried, but calling get_route may return a Route
with the same failing path. Add a routing::Score trait used to
parameterize get_route, which it calls to determine how much a channel
should be penalized in terms of msats willing to pay to avoid the
channel.
Also, add a Scorer struct that implements routing::Score with a constant
constant penalty. Subsequent changes will allow for more robust scoring
by feeding back payment path success and failure to the scorer via event
handling.
@jkczyz
jkczyzforce-pushed
the
2021-10-default-channel-scoring
branch
from
84c0a48 to
e15a18aCompareOctober 15, 2021 20:31
TheBlueMatt
TheBlueMatt
approved these changes
Oct 15, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff since val's ack was just a squash:
$ git diff-tree -U1 84c0a486e e15a18a50 $

@TheBlueMatt
TheBlueMatt
merged commit 2398f17
into
lightningdevkit:mainOct 16, 2021
@jkczyzjkczyz
mentioned this pull request
Oct 30, 2021
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@valentinewallacevalentinewallacevalentinewallace approved these changes
@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.102

Development

Successfully merging this pull request may close these issues.

3 participants

@jkczyz@TheBlueMatt@valentinewallace

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add channel scoring to get_route - #1124
Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
jkczyz:2021-10-default-channel-scoring
Oct 16, 2021
Merged
Add channel scoring to get_route#1124
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
jkczyz:2021-10-default-channel-scoring
Conversation
@jkczyz
@jkczyzjkczyz
commented
Oct 14, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
Failed payments may be retried, but calling get_route may return a Route with the same failing path. Add a routing::Score trait used to parameterize get_route, which it calls to determine how much a channel should be penalized in terms of msats willing to pay to avoid the channel.
Also, add a Scorer struct that implements routing::Score with a constant penalty. Subsequent changes will allow for more robust scoring by feeding back payment path success and failure to the scorer via event handling.
@codecov
codecovBot
commented
Oct 14, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1124 (84c0a48) into main (da498d7) will increase coverage by 0.76%.

The diff coverage is 91.52%.
❗ Current head 84c0a48 differs from pull request most recent head e15a18a. Consider uploading reports for the commit e15a18a to get more accurate results
Impacted file tree graph
@@ Coverage Diff @@## main #1124 +/- ##
==========================================
+ Coverage 90.60% 91.36% +0.76% 
==========================================
Files 66 67 +1 Lines 34474 37698 +3224 ==========================================
+ Hits 31235 34443 +3208 - Misses 3239 3255 +16 
Impacted FilesCoverage Δ
lightning/src/routing/scorer.rs57.14% <57.14%> (ø)
lightning/src/routing/router.rs95.70% <91.78%> (-0.30%)⬇️
lightning-invoice/src/utils.rs84.26% <100.00%> (+0.17%)⬆️
lightning/src/ln/channelmanager.rs85.14% <100.00%> (+0.01%)⬆️
lightning/src/ln/functional_test_utils.rs97.32% <100.00%> (+2.24%)⬆️
lightning/src/ln/functional_tests.rs97.93% <100.00%> (+0.53%)⬆️
lightning/src/ln/shutdown_tests.rs95.87% <100.00%> (+<0.01%)⬆️
lightning/src/chain/mod.rs50.00% <0.00%> (-8.83%)⬇️
lightning-background-processor/src/lib.rs94.38% <0.00%> (+0.15%)⬆️
... 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 da498d7...e15a18a. Read the comment docs.
@TheBlueMattTheBlueMatt
added this to the 0.0.102 milestone
Oct 14, 2021
TheBlueMatt
TheBlueMatt
reviewed
Oct 14, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmarks are probably about what I'd expect given the expansion of of in-cmp logic. Something to look into eventually, probably:
Latest git
test ln::channelmanager::bench::bench_sends ... bench: 9,128,177 ns/iter (+/- 1,349,843)
test routing::network_graph::benches::read_network_graph ... bench: 1,777,566,078 ns/iter (+/- 23,897,528)
test routing::network_graph::benches::write_network_graph ... bench: 159,355,206 ns/iter (+/- 5,292,557)
test routing::router::benches::generate_mpp_routes ... bench: 74,852,606 ns/iter (+/- 55,662,508)
test routing::router::benches::generate_routes ... bench: 72,321,963 ns/iter (+/- 56,925,293)
test bench::bench_sends ... bench: 211,670,987 ns/iter (+/- 188,695,358)

This PR:
test ln::channelmanager::bench::bench_sends ... bench: 8,284,024 ns/iter (+/- 443,276)
test routing::network_graph::benches::read_network_graph ... bench: 1,614,888,492 ns/iter (+/- 2,280,811)
test routing::network_graph::benches::write_network_graph ... bench: 143,246,612 ns/iter (+/- 458,059)
test routing::router::benches::generate_mpp_routes ... bench: 96,742,965 ns/iter (+/- 52,848,627)
test routing::router::benches::generate_routes ... bench: 96,963,520 ns/iter (+/- 62,975,254)
test bench::bench_sends ... bench: 190,069,734 ns/iter (+/- 168,900,474)

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/mod.rs
Outdated

Comment threadlightning/src/routing/mod.rs
Outdated

@TheBlueMatt
TheBlueMatt
commented
Oct 14, 2021

Copy link


Copy Markdown

Collaborator
Note for comparison bench for 0.0.101 was (there's obviously a ton of noise in these numbers from github CI).
test ln::channelmanager::bench::bench_sends ... bench: 7,587,098 ns/iter (+/- 1,026,747)
test routing::network_graph::benches::read_network_graph ... bench: 2,195,464,293 ns/iter (+/- 116,402,530)
test routing::network_graph::benches::write_network_graph ... bench: 132,630,621 ns/iter (+/- 11,344,574)
test routing::router::benches::generate_mpp_routes ... bench: 82,229,661 ns/iter (+/- 48,137,908)
test routing::router::benches::generate_routes ... bench: 83,419,028 ns/iter (+/- 62,531,148)
test bench::bench_sends ... bench: 175,605,757 ns/iter (+/- 148,418,861)

TheBlueMatt
TheBlueMatt
reviewed
Oct 14, 2021
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Oct 14, 2021
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

@jkczyz
jkczyz
commented
Oct 14, 2021

Copy link


Copy Markdown

ContributorAuthor
All good feedback! Thanks for divining into benchmarks. All comments addressed.
@TheBlueMatt
TheBlueMatt
commented
Oct 14, 2021

Copy link


Copy Markdown

Collaborator
Github CI now says (which is, uhmmmm, faster than latest git? yea, no, that's just cause github's CI machines are clearly very heterogeneous....last time I look at that output.........)
test ln::channelmanager::bench::bench_sends ... bench: 8,363,865 ns/iter (+/- 1,362,103)
test routing::network_graph::benches::read_network_graph ... bench: 1,626,468,025 ns/iter (+/- 90,985,613)
test routing::network_graph::benches::write_network_graph ... bench: 142,291,900 ns/iter (+/- 6,987,924)
test routing::router::benches::generate_mpp_routes ... bench: 57,154,598 ns/iter (+/- 32,542,178)
test routing::router::benches::generate_routes ... bench: 57,344,572 ns/iter (+/- 39,866,175)
test bench::bench_sends ... bench: 197,672,739 ns/iter (+/- 170,050,060)

TheBlueMatt
TheBlueMatt
approved these changes
Oct 15, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs tests but otherwise good.
Comment threadlightning/src/routing/router.rs
Outdated

	path_htlc_minimum_msat: u64,
	/// All penalties incurred *after* this hop on the way to the destination, as calculated using
	/// channel scoring.
	next_hops_penalty_msat: u64,
@TheBlueMattTheBlueMattOct 15, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of next_hops in these confused me somewhat - next_hops_fee_msat (both in a PathBuildingHop and in the add_node macro) refers to only fee after the hop but not including the fee charged at the hop, whereas here we're using it to include the penalty charged at a hop. Maybe path_penalty_msat or so?
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. I was a little confused by the naming, but now I see the pattern. Should be consistent now.
Comment threadlightning/src/ln/functional_tests.rs
Outdated

use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT;
use routing::network_graph::{NetworkUpdate, RoutingFees};
use routing::router::{Route, RouteHop, RouteHint, RouteHintHop, get_route, get_keysend_route};
use routing::scorer::DefaultScorer;
@TheBlueMattTheBlueMattOct 15, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use either a TestScorer in tests or a DefaultScorer with a score of 0? I find it a bit...awkward? to change all of our routing parameters in all our tests, even if they still pass it kinda makes me nervous.
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. I removed TestScorer and renamed DefaultScorer to Scorer, using it with 0 in all tests excepted for the newly added one.
Comment threadlightning/src/routing/scorer.rs
Outdated

impl Default for DefaultScorer {
	/// Creates a new scorer using 100 msat as the channel penalty.
	fn default() -> Self {
 DefaultScorer::new(100)
@TheBlueMattTheBlueMattOct 15, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think default should be 500, just kinda gut-feel. Lots of "good" nodes charge 1 sat per payment, and we should probably be willing to prefer that over two-three "free" hops IMO. Maybe even 1k. We can tune this later, but 100 just seems very low.
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to 500 msats.
jkczyz
jkczyz
commented
Oct 15, 2021
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test but was getting a little confused with the fee_msat assertions. Let me know if it looks sane. I could add more tests if needed.
Comment threadlightning/src/routing/router.rs
Outdated

	path_htlc_minimum_msat: u64,
	/// All penalties incurred *after* this hop on the way to the destination, as calculated using
	/// channel scoring.
	next_hops_penalty_msat: u64,
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. I was a little confused by the naming, but now I see the pattern. Should be consistent now.
Comment threadlightning/src/ln/functional_tests.rs
Outdated

use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT;
use routing::network_graph::{NetworkUpdate, RoutingFees};
use routing::router::{Route, RouteHop, RouteHint, RouteHintHop, get_route, get_keysend_route};
use routing::scorer::DefaultScorer;
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. I removed TestScorer and renamed DefaultScorer to Scorer, using it with 0 in all tests excepted for the newly added one.
Comment threadlightning/src/routing/scorer.rs
Outdated

impl Default for DefaultScorer {
	/// Creates a new scorer using 100 msat as the channel penalty.
	fn default() -> Self {
 DefaultScorer::new(100)
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to 500 msats.
valentinewallace
valentinewallace
reviewed
Oct 15, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to get an initial version of this in!
Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/mod.rs
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/scorer.rs
Outdated

//!
//! # Example
//!
//! TODO: Fill in example once interface has been finalized.
@valentinewallacevalentinewallaceOct 15, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're good to fill this in?
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Not much to it yet, but could expand if we add more configurations. Also, may want to have an example with InvoicePayer for event handling once that is in. Or at least reference those docs.
valentinewallace
valentinewallace
approved these changes
Oct 15, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀
@TheBlueMatt
TheBlueMatt
commented
Oct 15, 2021

Copy link


Copy Markdown

Collaborator
Can you squash :).
@jkczyz
Add channel scoring to get_route
e15a18a
Failed payments may be retried, but calling get_route may return a Route
with the same failing path. Add a routing::Score trait used to
parameterize get_route, which it calls to determine how much a channel
should be penalized in terms of msats willing to pay to avoid the
channel.
Also, add a Scorer struct that implements routing::Score with a constant
constant penalty. Subsequent changes will allow for more robust scoring
by feeding back payment path success and failure to the scorer via event
handling.
@jkczyz
jkczyzforce-pushed
the
2021-10-default-channel-scoring
branch
from
84c0a48 to
e15a18aCompareOctober 15, 2021 20:31
TheBlueMatt
TheBlueMatt
approved these changes
Oct 15, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff since val's ack was just a squash:
$ git diff-tree -U1 84c0a486e e15a18a50 $

@TheBlueMatt
TheBlueMatt
merged commit 2398f17
into
lightningdevkit:mainOct 16, 2021
@jkczyzjkczyz
mentioned this pull request
Oct 30, 2021
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@valentinewallacevalentinewallacevalentinewallace approved these changes
@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.102

Development

Successfully merging this pull request may close these issues.

3 participants

@jkczyz@TheBlueMatt@valentinewallace

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Add channel scoring to get_route - #1124
Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
jkczyz:2021-10-default-channel-scoring
Oct 16, 2021
Merged
Add channel scoring to get_route#1124
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
jkczyz:2021-10-default-channel-scoring
Conversation
@jkczyz
@jkczyzjkczyz
commented
Oct 14, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
Failed payments may be retried, but calling get_route may return a Route with the same failing path. Add a routing::Score trait used to parameterize get_route, which it calls to determine how much a channel should be penalized in terms of msats willing to pay to avoid the channel.
Also, add a Scorer struct that implements routing::Score with a constant penalty. Subsequent changes will allow for more robust scoring by feeding back payment path success and failure to the scorer via event handling.
@codecov
codecovBot
commented
Oct 14, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1124 (84c0a48) into main (da498d7) will increase coverage by 0.76%.

The diff coverage is 91.52%.
❗ Current head 84c0a48 differs from pull request most recent head e15a18a. Consider uploading reports for the commit e15a18a to get more accurate results
Impacted file tree graph
@@ Coverage Diff @@## main #1124 +/- ##
==========================================
+ Coverage 90.60% 91.36% +0.76% 
==========================================
Files 66 67 +1 Lines 34474 37698 +3224 ==========================================
+ Hits 31235 34443 +3208 - Misses 3239 3255 +16 
Impacted FilesCoverage Δ
lightning/src/routing/scorer.rs57.14% <57.14%> (ø)
lightning/src/routing/router.rs95.70% <91.78%> (-0.30%)⬇️
lightning-invoice/src/utils.rs84.26% <100.00%> (+0.17%)⬆️
lightning/src/ln/channelmanager.rs85.14% <100.00%> (+0.01%)⬆️
lightning/src/ln/functional_test_utils.rs97.32% <100.00%> (+2.24%)⬆️
lightning/src/ln/functional_tests.rs97.93% <100.00%> (+0.53%)⬆️
lightning/src/ln/shutdown_tests.rs95.87% <100.00%> (+<0.01%)⬆️
lightning/src/chain/mod.rs50.00% <0.00%> (-8.83%)⬇️
lightning-background-processor/src/lib.rs94.38% <0.00%> (+0.15%)⬆️
... 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 da498d7...e15a18a. Read the comment docs.
@TheBlueMattTheBlueMatt
added this to the 0.0.102 milestone
Oct 14, 2021
TheBlueMatt
TheBlueMatt
reviewed
Oct 14, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmarks are probably about what I'd expect given the expansion of of in-cmp logic. Something to look into eventually, probably:
Latest git
test ln::channelmanager::bench::bench_sends ... bench: 9,128,177 ns/iter (+/- 1,349,843)
test routing::network_graph::benches::read_network_graph ... bench: 1,777,566,078 ns/iter (+/- 23,897,528)
test routing::network_graph::benches::write_network_graph ... bench: 159,355,206 ns/iter (+/- 5,292,557)
test routing::router::benches::generate_mpp_routes ... bench: 74,852,606 ns/iter (+/- 55,662,508)
test routing::router::benches::generate_routes ... bench: 72,321,963 ns/iter (+/- 56,925,293)
test bench::bench_sends ... bench: 211,670,987 ns/iter (+/- 188,695,358)

This PR:
test ln::channelmanager::bench::bench_sends ... bench: 8,284,024 ns/iter (+/- 443,276)
test routing::network_graph::benches::read_network_graph ... bench: 1,614,888,492 ns/iter (+/- 2,280,811)
test routing::network_graph::benches::write_network_graph ... bench: 143,246,612 ns/iter (+/- 458,059)
test routing::router::benches::generate_mpp_routes ... bench: 96,742,965 ns/iter (+/- 52,848,627)
test routing::router::benches::generate_routes ... bench: 96,963,520 ns/iter (+/- 62,975,254)
test bench::bench_sends ... bench: 190,069,734 ns/iter (+/- 168,900,474)

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/mod.rs
Outdated

Comment threadlightning/src/routing/mod.rs
Outdated

@TheBlueMatt
TheBlueMatt
commented
Oct 14, 2021

Copy link


Copy Markdown

Collaborator
Note for comparison bench for 0.0.101 was (there's obviously a ton of noise in these numbers from github CI).
test ln::channelmanager::bench::bench_sends ... bench: 7,587,098 ns/iter (+/- 1,026,747)
test routing::network_graph::benches::read_network_graph ... bench: 2,195,464,293 ns/iter (+/- 116,402,530)
test routing::network_graph::benches::write_network_graph ... bench: 132,630,621 ns/iter (+/- 11,344,574)
test routing::router::benches::generate_mpp_routes ... bench: 82,229,661 ns/iter (+/- 48,137,908)
test routing::router::benches::generate_routes ... bench: 83,419,028 ns/iter (+/- 62,531,148)
test bench::bench_sends ... bench: 175,605,757 ns/iter (+/- 148,418,861)

TheBlueMatt
TheBlueMatt
reviewed
Oct 14, 2021
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Oct 14, 2021
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

@jkczyz
jkczyz
commented
Oct 14, 2021

Copy link


Copy Markdown

ContributorAuthor
All good feedback! Thanks for divining into benchmarks. All comments addressed.
@TheBlueMatt
TheBlueMatt
commented
Oct 14, 2021

Copy link


Copy Markdown

Collaborator
Github CI now says (which is, uhmmmm, faster than latest git? yea, no, that's just cause github's CI machines are clearly very heterogeneous....last time I look at that output.........)
test ln::channelmanager::bench::bench_sends ... bench: 8,363,865 ns/iter (+/- 1,362,103)
test routing::network_graph::benches::read_network_graph ... bench: 1,626,468,025 ns/iter (+/- 90,985,613)
test routing::network_graph::benches::write_network_graph ... bench: 142,291,900 ns/iter (+/- 6,987,924)
test routing::router::benches::generate_mpp_routes ... bench: 57,154,598 ns/iter (+/- 32,542,178)
test routing::router::benches::generate_routes ... bench: 57,344,572 ns/iter (+/- 39,866,175)
test bench::bench_sends ... bench: 197,672,739 ns/iter (+/- 170,050,060)

TheBlueMatt
TheBlueMatt
approved these changes
Oct 15, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs tests but otherwise good.
Comment threadlightning/src/routing/router.rs
Outdated

	path_htlc_minimum_msat: u64,
	/// All penalties incurred *after* this hop on the way to the destination, as calculated using
	/// channel scoring.
	next_hops_penalty_msat: u64,
@TheBlueMattTheBlueMattOct 15, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of next_hops in these confused me somewhat - next_hops_fee_msat (both in a PathBuildingHop and in the add_node macro) refers to only fee after the hop but not including the fee charged at the hop, whereas here we're using it to include the penalty charged at a hop. Maybe path_penalty_msat or so?
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. I was a little confused by the naming, but now I see the pattern. Should be consistent now.
Comment threadlightning/src/ln/functional_tests.rs
Outdated

use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT;
use routing::network_graph::{NetworkUpdate, RoutingFees};
use routing::router::{Route, RouteHop, RouteHint, RouteHintHop, get_route, get_keysend_route};
use routing::scorer::DefaultScorer;
@TheBlueMattTheBlueMattOct 15, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use either a TestScorer in tests or a DefaultScorer with a score of 0? I find it a bit...awkward? to change all of our routing parameters in all our tests, even if they still pass it kinda makes me nervous.
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. I removed TestScorer and renamed DefaultScorer to Scorer, using it with 0 in all tests excepted for the newly added one.
Comment threadlightning/src/routing/scorer.rs
Outdated

impl Default for DefaultScorer {
	/// Creates a new scorer using 100 msat as the channel penalty.
	fn default() -> Self {
 DefaultScorer::new(100)
@TheBlueMattTheBlueMattOct 15, 2021

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think default should be 500, just kinda gut-feel. Lots of "good" nodes charge 1 sat per payment, and we should probably be willing to prefer that over two-three "free" hops IMO. Maybe even 1k. We can tune this later, but 100 just seems very low.
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to 500 msats.
jkczyz
jkczyz
commented
Oct 15, 2021
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test but was getting a little confused with the fee_msat assertions. Let me know if it looks sane. I could add more tests if needed.
Comment threadlightning/src/routing/router.rs
Outdated

	path_htlc_minimum_msat: u64,
	/// All penalties incurred *after* this hop on the way to the destination, as calculated using
	/// channel scoring.
	next_hops_penalty_msat: u64,
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. I was a little confused by the naming, but now I see the pattern. Should be consistent now.
Comment threadlightning/src/ln/functional_tests.rs
Outdated

use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT;
use routing::network_graph::{NetworkUpdate, RoutingFees};
use routing::router::{Route, RouteHop, RouteHint, RouteHintHop, get_route, get_keysend_route};
use routing::scorer::DefaultScorer;
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. I removed TestScorer and renamed DefaultScorer to Scorer, using it with 0 in all tests excepted for the newly added one.
Comment threadlightning/src/routing/scorer.rs
Outdated

impl Default for DefaultScorer {
	/// Creates a new scorer using 100 msat as the channel penalty.
	fn default() -> Self {
 DefaultScorer::new(100)
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to 500 msats.
valentinewallace
valentinewallace
reviewed
Oct 15, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to get an initial version of this in!
Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/mod.rs
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/scorer.rs
Outdated

//!
//! # Example
//!
//! TODO: Fill in example once interface has been finalized.
@valentinewallacevalentinewallaceOct 15, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're good to fill this in?
@jkczyzjkczyzOct 15, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Not much to it yet, but could expand if we add more configurations. Also, may want to have an example with InvoicePayer for event handling once that is in. Or at least reference those docs.
valentinewallace
valentinewallace
approved these changes
Oct 15, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀
@TheBlueMatt
TheBlueMatt
commented
Oct 15, 2021

Copy link


Copy Markdown

Collaborator
Can you squash :).
@jkczyz
Add channel scoring to get_route
e15a18a
Failed payments may be retried, but calling get_route may return a Route
with the same failing path. Add a routing::Score trait used to
parameterize get_route, which it calls to determine how much a channel
should be penalized in terms of msats willing to pay to avoid the
channel.
Also, add a Scorer struct that implements routing::Score with a constant
constant penalty. Subsequent changes will allow for more robust scoring
by feeding back payment path success and failure to the scorer via event
handling.
@jkczyz
jkczyzforce-pushed
the
2021-10-default-channel-scoring
branch
from
84c0a48 to
e15a18aCompareOctober 15, 2021 20:31
TheBlueMatt
TheBlueMatt
approved these changes
Oct 15, 2021
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff since val's ack was just a squash:
$ git diff-tree -U1 84c0a486e e15a18a50 $

@TheBlueMatt
TheBlueMatt
merged commit 2398f17
into
lightningdevkit:mainOct 16, 2021
@jkczyzjkczyz
mentioned this pull request
Oct 30, 2021
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@valentinewallacevalentinewallacevalentinewallace approved these changes
@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.102

Development

Successfully merging this pull request may close these issues.

3 participants

@jkczyz@TheBlueMatt@valentinewallace