Skip to content

Randomize candidate paths during route selection. - #1359

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
tnull:2022-03-real-random-shuffle
Mar 24, 2022
Merged

Randomize candidate paths during route selection.#1359
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
tnull:2022-03-real-random-shuffle

Conversation

@tnull

@tnulltnull commented Mar 10, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

This PR is a follow-up to #1286: it implements a 'real random shuffle' to randomize candidate payment paths during route selection. I played with a few variants of this and now ended up generating a number of unique permutations.

During testing it became apparent that the behavior of Step (7) was indeterministic and dependent on the input order of paths when they were of equal value_msat, which broke htlc_minimum_overpay_test. I now addressed this by pre-sorting the paths according to their total_fee_paid_msat, which should drop more expensive paths first, i.e., always prefer lower-fee paths of equal value.

Closes#869.

@tnull
tnullforce-pushed the 2022-03-real-random-shuffle branch from a36e655 to ba73f3eCompareMarch 12, 2022 13:32
@codecov-commenter

codecov-commenter commented Mar 12, 2022•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1359 (d5a0738) into main (5ed2985) will increase coverage by 0.05%.
The diff coverage is 91.66%.

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

@@ Coverage Diff @@## main #1359 +/- ##
==========================================
+ Coverage 90.75% 90.81% +0.05% 
==========================================
Files 73 73 Lines 40884 41215 +331 Branches 40884 41215 +331 ==========================================
+ Hits 37106 37428 +322 - Misses 3778 3787 +9
Impacted FilesCoverage Δ
lightning/src/routing/router.rs92.62% <91.66%> (+0.25%)⬆️
lightning-net-tokio/src/lib.rs75.88% <0.00%> (-0.81%)⬇️
lightning/src/debug_sync.rs94.79% <0.00%> (-0.27%)⬇️
lightning/src/util/events.rs33.21% <0.00%> (-0.24%)⬇️
lightning-invoice/src/de.rs81.06% <0.00%> (-0.21%)⬇️
lightning/src/ln/functional_tests.rs97.06% <0.00%> (-0.10%)⬇️
lightning/src/routing/scoring.rs94.29% <0.00%> (ø)
lightning/src/routing/network_graph.rs89.54% <0.00%> (+0.01%)⬆️
lightning/src/ln/channelmanager.rs84.85% <0.00%> (+0.05%)⬆️
... and 4 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 5ed2985...ff7ec0c. Read the comment docs.

@tnull
tnullforce-pushed the 2022-03-real-random-shuffle branch from ba73f3e to 0b238c0CompareMarch 15, 2022 23:04
TheBlueMatt
TheBlueMatt reviewed Mar 16, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs Outdated
// In order to do so, we pre-sort by total fees paid, so that in case of equal
// values we prefer lower cost paths.
// (Descending order, so we drop higher-fee paths first)
cur_route.sort_by_key(|path| path.get_total_fee_paid_msat());

@TheBlueMattTheBlueMattMar 16, 2022

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.

This should include the scoring results, too, no?

@tnulltnullMar 17, 2022•
edited
Loading

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.

Yes, you're probably right. Maybe also htlc_minimum_msat? Do you have a suggestion how we would prioritize values, total fees, penalties, and HTLC minima?

@TheBlueMattTheBlueMattMar 17, 2022

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.

Hmm, right, yea, the issue with htlc min is it really just kicks in after a threshold. Maybe the way to address that is loop at the reduction stage and if we hit htlc min try the next path instead of just reducing and moving on. Then we could just sort by score+fee here (which is all designed to be added, so we always just add for that).

@tnulltnullMar 21, 2022

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'm now considering the score with 9175523. Not sure if the additional complexity needed to incorporate htlc_minimum_msat is worth the gain?

@tnull
tnullforce-pushed the 2022-03-real-random-shuffle branch from 0b238c0 to 84fa714CompareMarch 17, 2022 14:17
tnull
tnull commented Mar 17, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
@@ -1522,7 +1522,7 @@ where L::Target: Logger {
// Now, subtract the overpaid value from the most-expensive path.
// TODO: this could also be optimized by also sorting by feerate_per_sat_routed,

@tnulltnullMar 17, 2022•
edited
Loading

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'm not fully sure how to address this TODO while I'm here. What would we gain if we multisort here again? Don't we want to just optimize for fees at this point?

@TheBlueMattTheBlueMattMar 17, 2022

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.

Right, the TODO here notes that sorting by the path value isn't optimizing for fees, optimizing for a real feerate would be, instead of just feerate ignoring base fee.

@tnulltnullMar 21, 2022•
edited
Loading

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'm probably missing something here, but isn't this what sorting once more by get_total_fees_paid_msat() would do?

TheBlueMatt
TheBlueMatt reviewed Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs Outdated
TheBlueMatt
TheBlueMatt reviewed Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs Outdated
TheBlueMatt
TheBlueMatt reviewed Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs Outdated
@tnull

tnull commented Mar 21, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Added a simple refactor and will probably also try to address this TODO in router "while im here".

TheBlueMatt
TheBlueMatt reviewed Mar 21, 2022
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.

LGTM, can you clean up the git log somewhat?

@tnull
tnullforce-pushed the 2022-03-real-random-shuffle branch from 0a322d3 to 9a2b8f2CompareMarch 21, 2022 22:57
@tnull

tnull commented Mar 21, 2022

Copy link
Copy Markdown
ContributorAuthor

LGTM, can you clean up the git log somewhat?

Squashed without changes.

@tnull
tnullforce-pushed the 2022-03-real-random-shuffle branch from 9a2b8f2 to 013954eCompareMarch 21, 2022 23:02
tnull
tnull commented Mar 21, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs Outdated
@TheBlueMatt

TheBlueMatt commented Mar 21, 2022

Copy link
Copy Markdown
Collaborator

Hmm, it would be kinda nice if the unrelated changes are in a different commit, eg the wrapping adds and sort changes could be a separate commit.

@tnull
tnullforce-pushed the 2022-03-real-random-shuffle branch from 013954e to 90474d2CompareMarch 21, 2022 23:14
TheBlueMatt
TheBlueMatt previously approved these changes Mar 21, 2022
View reviewed changes
@jkczyz
jkczyz self-requested a review March 21, 2022 23:50
jkczyz
jkczyz reviewed Mar 22, 2022
View reviewed changes

@jkczyzjkczyz 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. Just some nits.

Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs Outdated
jkczyz
jkczyz reviewed Mar 24, 2022
View reviewed changes

@jkczyzjkczyz 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.

Will need to insert the last commit between the first two commits given it's a fixup for the first commit and will be squashed into it whereas the second commit is independent. I typically use git rebase -i when needing to interleave fixups.

Comment threadlightning/src/routing/router.rs
tnull added 2 commits March 24, 2022 09:12
@tnull
Randomize candidate paths during route selection.
b11dcf9
@tnull
Minor refactor for sort / add, and some nits.
ff7ec0c
- `sort_by_key` to `sort_unstable_by_key`
- `checked_add() .. max_value()` to `saturating_add()`
- Some typos and nits
@tnull
tnullforce-pushed the 2022-03-real-random-shuffle branch from d5a0738 to ff7ec0cCompareMarch 24, 2022 15:13
@TheBlueMattTheBlueMatt self-assigned this Mar 24, 2022
@TheBlueMatt
TheBlueMatt merged commit 216225c into lightningdevkit:mainMar 24, 2022
@tnulltnull mentioned this pull request May 16, 2022
Open
@TheBlueMattTheBlueMatt mentioned this pull request Jul 13, 2022
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@jkczyzjkczyzjkczyz approved these changes

Assignees

@TheBlueMattTheBlueMatt

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Change route selection algorithm to be actually (pseudo-)random

4 participants

@tnull@codecov-commenter@TheBlueMatt@jkczyz
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
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;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Randomize candidate paths during route selection. by tnull · Pull Request #1359 · lightningdevkit/rust-lightning · GitHub
Skip to content
Randomize candidate paths during route selection. - #1359
Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
tnull:2022-03-real-random-shuffle
Mar 24, 2022
Merged
Randomize candidate paths during route selection.#1359
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
tnull:2022-03-real-random-shuffle
Conversation
@tnull
@tnulltnull
commented
Mar 10, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR is a follow-up to #1286: it implements a 'real random shuffle' to randomize candidate payment paths during route selection. I played with a few variants of this and now ended up generating a number of unique permutations.
During testing it became apparent that the behavior of Step (7) was indeterministic and dependent on the input order of paths when they were of equal value_msat, which broke htlc_minimum_overpay_test. I now addressed this by pre-sorting the paths according to their total_fee_paid_msat, which should drop more expensive paths first, i.e., always prefer lower-fee paths of equal value.
Closes#869.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
a36e655 to
ba73f3eCompareMarch 12, 2022 13:32
@codecov-commenter
codecov-commenter
commented
Mar 12, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1359 (d5a0738) into main (5ed2985) will increase coverage by 0.05%.

The diff coverage is 91.66%.
❗ Current head d5a0738 differs from pull request most recent head ff7ec0c. Consider uploading reports for the commit ff7ec0c to get more accurate results
@@ Coverage Diff @@## main #1359 +/- ##
==========================================
+ Coverage 90.75% 90.81% +0.05% 
==========================================
Files 73 73 Lines 40884 41215 +331 Branches 40884 41215 +331 ==========================================
+ Hits 37106 37428 +322 - Misses 3778 3787 +9 
Impacted FilesCoverage Δ
lightning/src/routing/router.rs92.62% <91.66%> (+0.25%)⬆️
lightning-net-tokio/src/lib.rs75.88% <0.00%> (-0.81%)⬇️
lightning/src/debug_sync.rs94.79% <0.00%> (-0.27%)⬇️
lightning/src/util/events.rs33.21% <0.00%> (-0.24%)⬇️
lightning-invoice/src/de.rs81.06% <0.00%> (-0.21%)⬇️
lightning/src/ln/functional_tests.rs97.06% <0.00%> (-0.10%)⬇️
lightning/src/routing/scoring.rs94.29% <0.00%> (ø)
lightning/src/routing/network_graph.rs89.54% <0.00%> (+0.01%)⬆️
lightning/src/ln/channelmanager.rs84.85% <0.00%> (+0.05%)⬆️
... and 4 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 5ed2985...ff7ec0c. Read the comment docs.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
ba73f3e to
0b238c0CompareMarch 15, 2022 23:04
TheBlueMatt
TheBlueMatt
reviewed
Mar 16, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

 // In order to do so, we pre-sort by total fees paid, so that in case of equal
 // values we prefer lower cost paths.
 // (Descending order, so we drop higher-fee paths first)
 cur_route.sort_by_key(|path| path.get_total_fee_paid_msat());
@TheBlueMattTheBlueMattMar 16, 2022

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.

This should include the scoring results, too, no?
@tnulltnullMar 17, 2022•

edited

Loading

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.

Yes, you're probably right. Maybe also htlc_minimum_msat? Do you have a suggestion how we would prioritize values, total fees, penalties, and HTLC minima?
@TheBlueMattTheBlueMattMar 17, 2022

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.

Hmm, right, yea, the issue with htlc min is it really just kicks in after a threshold. Maybe the way to address that is loop at the reduction stage and if we hit htlc min try the next path instead of just reducing and moving on. Then we could just sort by score+fee here (which is all designed to be added, so we always just add for that).
@tnulltnullMar 21, 2022

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'm now considering the score with 9175523. Not sure if the additional complexity needed to incorporate htlc_minimum_msat is worth the gain?
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
0b238c0 to
84fa714CompareMarch 17, 2022 14:17
tnull
tnull
commented
Mar 17, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
@@ -1522,7 +1522,7 @@ where L::Target: Logger {
 // Now, subtract the overpaid value from the most-expensive path.
 // TODO: this could also be optimized by also sorting by feerate_per_sat_routed,
@tnulltnullMar 17, 2022•

edited

Loading

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'm not fully sure how to address this TODO while I'm here. What would we gain if we multisort here again? Don't we want to just optimize for fees at this point?
@TheBlueMattTheBlueMattMar 17, 2022

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.

Right, the TODO here notes that sorting by the path value isn't optimizing for fees, optimizing for a real feerate would be, instead of just feerate ignoring base fee.
@tnulltnullMar 21, 2022•

edited

Loading

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'm probably missing something here, but isn't this what sorting once more by get_total_fees_paid_msat() would do?
TheBlueMatt
TheBlueMatt
reviewed
Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

@tnull
tnull
commented
Mar 21, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Added a simple refactor and will probably also try to address this TODO in router "while im here".
TheBlueMatt
TheBlueMatt
reviewed
Mar 21, 2022
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.

LGTM, can you clean up the git log somewhat?
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
0a322d3 to
9a2b8f2CompareMarch 21, 2022 22:57
@tnull
tnull
commented
Mar 21, 2022

Copy link


Copy Markdown

ContributorAuthor
LGTM, can you clean up the git log somewhat?
Squashed without changes.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
9a2b8f2 to
013954eCompareMarch 21, 2022 23:02
tnull
tnull
commented
Mar 21, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

@TheBlueMatt
TheBlueMatt
commented
Mar 21, 2022

Copy link


Copy Markdown

Collaborator
Hmm, it would be kinda nice if the unrelated changes are in a different commit, eg the wrapping adds and sort changes could be a separate commit.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
013954e to
90474d2CompareMarch 21, 2022 23:14
TheBlueMatt
TheBlueMatt
previously approved these changes
Mar 21, 2022
View reviewed changes
@jkczyz
jkczyz
self-requested a review
March 21, 2022 23:50
jkczyz
jkczyz
reviewed
Mar 22, 2022
View reviewed changes
@jkczyzjkczyz
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. Just some nits.
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs
Outdated

jkczyz
jkczyz
reviewed
Mar 24, 2022
View reviewed changes
@jkczyzjkczyz
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.

Will need to insert the last commit between the first two commits given it's a fixup for the first commit and will be squashed into it whereas the second commit is independent. I typically use git rebase -i when needing to interleave fixups.
Comment threadlightning/src/routing/router.rs
tnull
added 2 commits
March 24, 2022 09:12
@tnull
Randomize candidate paths during route selection.
b11dcf9
@tnull
Minor refactor for sort / add, and some nits.
ff7ec0c
- `sort_by_key` to `sort_unstable_by_key`
- `checked_add() .. max_value()` to `saturating_add()`
- Some typos and nits
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
d5a0738 to
ff7ec0cCompareMarch 24, 2022 15:13
@TheBlueMattTheBlueMatt
self-assigned this
Mar 24, 2022
@TheBlueMatt
TheBlueMatt
merged commit 216225c
into
lightningdevkit:mainMar 24, 2022
@tnulltnull
mentioned this pull request
May 16, 2022

Open

@TheBlueMattTheBlueMatt
mentioned this pull request
Jul 13, 2022
 Merged

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

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@jkczyzjkczyzjkczyz approved these changes

Assignees

@TheBlueMattTheBlueMatt

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Change route selection algorithm to be actually (pseudo-)random


4 participants

@tnull@codecov-commenter@TheBlueMatt@jkczyz

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Randomize candidate paths during route selection. by tnull · Pull Request #1359 · lightningdevkit/rust-lightning · GitHub
Skip to content
Randomize candidate paths during route selection. - #1359
Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
tnull:2022-03-real-random-shuffle
Mar 24, 2022
Merged
Randomize candidate paths during route selection.#1359
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
tnull:2022-03-real-random-shuffle
Conversation
@tnull
@tnulltnull
commented
Mar 10, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR is a follow-up to #1286: it implements a 'real random shuffle' to randomize candidate payment paths during route selection. I played with a few variants of this and now ended up generating a number of unique permutations.
During testing it became apparent that the behavior of Step (7) was indeterministic and dependent on the input order of paths when they were of equal value_msat, which broke htlc_minimum_overpay_test. I now addressed this by pre-sorting the paths according to their total_fee_paid_msat, which should drop more expensive paths first, i.e., always prefer lower-fee paths of equal value.
Closes#869.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
a36e655 to
ba73f3eCompareMarch 12, 2022 13:32
@codecov-commenter
codecov-commenter
commented
Mar 12, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1359 (d5a0738) into main (5ed2985) will increase coverage by 0.05%.

The diff coverage is 91.66%.
❗ Current head d5a0738 differs from pull request most recent head ff7ec0c. Consider uploading reports for the commit ff7ec0c to get more accurate results
@@ Coverage Diff @@## main #1359 +/- ##
==========================================
+ Coverage 90.75% 90.81% +0.05% 
==========================================
Files 73 73 Lines 40884 41215 +331 Branches 40884 41215 +331 ==========================================
+ Hits 37106 37428 +322 - Misses 3778 3787 +9 
Impacted FilesCoverage Δ
lightning/src/routing/router.rs92.62% <91.66%> (+0.25%)⬆️
lightning-net-tokio/src/lib.rs75.88% <0.00%> (-0.81%)⬇️
lightning/src/debug_sync.rs94.79% <0.00%> (-0.27%)⬇️
lightning/src/util/events.rs33.21% <0.00%> (-0.24%)⬇️
lightning-invoice/src/de.rs81.06% <0.00%> (-0.21%)⬇️
lightning/src/ln/functional_tests.rs97.06% <0.00%> (-0.10%)⬇️
lightning/src/routing/scoring.rs94.29% <0.00%> (ø)
lightning/src/routing/network_graph.rs89.54% <0.00%> (+0.01%)⬆️
lightning/src/ln/channelmanager.rs84.85% <0.00%> (+0.05%)⬆️
... and 4 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 5ed2985...ff7ec0c. Read the comment docs.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
ba73f3e to
0b238c0CompareMarch 15, 2022 23:04
TheBlueMatt
TheBlueMatt
reviewed
Mar 16, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

 // In order to do so, we pre-sort by total fees paid, so that in case of equal
 // values we prefer lower cost paths.
 // (Descending order, so we drop higher-fee paths first)
 cur_route.sort_by_key(|path| path.get_total_fee_paid_msat());
@TheBlueMattTheBlueMattMar 16, 2022

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.

This should include the scoring results, too, no?
@tnulltnullMar 17, 2022•

edited

Loading

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.

Yes, you're probably right. Maybe also htlc_minimum_msat? Do you have a suggestion how we would prioritize values, total fees, penalties, and HTLC minima?
@TheBlueMattTheBlueMattMar 17, 2022

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.

Hmm, right, yea, the issue with htlc min is it really just kicks in after a threshold. Maybe the way to address that is loop at the reduction stage and if we hit htlc min try the next path instead of just reducing and moving on. Then we could just sort by score+fee here (which is all designed to be added, so we always just add for that).
@tnulltnullMar 21, 2022

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'm now considering the score with 9175523. Not sure if the additional complexity needed to incorporate htlc_minimum_msat is worth the gain?
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
0b238c0 to
84fa714CompareMarch 17, 2022 14:17
tnull
tnull
commented
Mar 17, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
@@ -1522,7 +1522,7 @@ where L::Target: Logger {
 // Now, subtract the overpaid value from the most-expensive path.
 // TODO: this could also be optimized by also sorting by feerate_per_sat_routed,
@tnulltnullMar 17, 2022•

edited

Loading

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'm not fully sure how to address this TODO while I'm here. What would we gain if we multisort here again? Don't we want to just optimize for fees at this point?
@TheBlueMattTheBlueMattMar 17, 2022

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.

Right, the TODO here notes that sorting by the path value isn't optimizing for fees, optimizing for a real feerate would be, instead of just feerate ignoring base fee.
@tnulltnullMar 21, 2022•

edited

Loading

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'm probably missing something here, but isn't this what sorting once more by get_total_fees_paid_msat() would do?
TheBlueMatt
TheBlueMatt
reviewed
Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

@tnull
tnull
commented
Mar 21, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Added a simple refactor and will probably also try to address this TODO in router "while im here".
TheBlueMatt
TheBlueMatt
reviewed
Mar 21, 2022
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.

LGTM, can you clean up the git log somewhat?
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
0a322d3 to
9a2b8f2CompareMarch 21, 2022 22:57
@tnull
tnull
commented
Mar 21, 2022

Copy link


Copy Markdown

ContributorAuthor
LGTM, can you clean up the git log somewhat?
Squashed without changes.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
9a2b8f2 to
013954eCompareMarch 21, 2022 23:02
tnull
tnull
commented
Mar 21, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

@TheBlueMatt
TheBlueMatt
commented
Mar 21, 2022

Copy link


Copy Markdown

Collaborator
Hmm, it would be kinda nice if the unrelated changes are in a different commit, eg the wrapping adds and sort changes could be a separate commit.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
013954e to
90474d2CompareMarch 21, 2022 23:14
TheBlueMatt
TheBlueMatt
previously approved these changes
Mar 21, 2022
View reviewed changes
@jkczyz
jkczyz
self-requested a review
March 21, 2022 23:50
jkczyz
jkczyz
reviewed
Mar 22, 2022
View reviewed changes
@jkczyzjkczyz
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. Just some nits.
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs
Outdated

jkczyz
jkczyz
reviewed
Mar 24, 2022
View reviewed changes
@jkczyzjkczyz
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.

Will need to insert the last commit between the first two commits given it's a fixup for the first commit and will be squashed into it whereas the second commit is independent. I typically use git rebase -i when needing to interleave fixups.
Comment threadlightning/src/routing/router.rs
tnull
added 2 commits
March 24, 2022 09:12
@tnull
Randomize candidate paths during route selection.
b11dcf9
@tnull
Minor refactor for sort / add, and some nits.
ff7ec0c
- `sort_by_key` to `sort_unstable_by_key`
- `checked_add() .. max_value()` to `saturating_add()`
- Some typos and nits
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
d5a0738 to
ff7ec0cCompareMarch 24, 2022 15:13
@TheBlueMattTheBlueMatt
self-assigned this
Mar 24, 2022
@TheBlueMatt
TheBlueMatt
merged commit 216225c
into
lightningdevkit:mainMar 24, 2022
@tnulltnull
mentioned this pull request
May 16, 2022

Open

@TheBlueMattTheBlueMatt
mentioned this pull request
Jul 13, 2022
 Merged

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

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@jkczyzjkczyzjkczyz approved these changes

Assignees

@TheBlueMattTheBlueMatt

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Change route selection algorithm to be actually (pseudo-)random


4 participants

@tnull@codecov-commenter@TheBlueMatt@jkczyz

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Randomize candidate paths during route selection. by tnull · Pull Request #1359 · lightningdevkit/rust-lightning · GitHub
Skip to content
Randomize candidate paths during route selection. - #1359
Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
tnull:2022-03-real-random-shuffle
Mar 24, 2022
Merged
Randomize candidate paths during route selection.#1359
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
tnull:2022-03-real-random-shuffle
Conversation
@tnull
@tnulltnull
commented
Mar 10, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR is a follow-up to #1286: it implements a 'real random shuffle' to randomize candidate payment paths during route selection. I played with a few variants of this and now ended up generating a number of unique permutations.
During testing it became apparent that the behavior of Step (7) was indeterministic and dependent on the input order of paths when they were of equal value_msat, which broke htlc_minimum_overpay_test. I now addressed this by pre-sorting the paths according to their total_fee_paid_msat, which should drop more expensive paths first, i.e., always prefer lower-fee paths of equal value.
Closes#869.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
a36e655 to
ba73f3eCompareMarch 12, 2022 13:32
@codecov-commenter
codecov-commenter
commented
Mar 12, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1359 (d5a0738) into main (5ed2985) will increase coverage by 0.05%.

The diff coverage is 91.66%.
❗ Current head d5a0738 differs from pull request most recent head ff7ec0c. Consider uploading reports for the commit ff7ec0c to get more accurate results
@@ Coverage Diff @@## main #1359 +/- ##
==========================================
+ Coverage 90.75% 90.81% +0.05% 
==========================================
Files 73 73 Lines 40884 41215 +331 Branches 40884 41215 +331 ==========================================
+ Hits 37106 37428 +322 - Misses 3778 3787 +9 
Impacted FilesCoverage Δ
lightning/src/routing/router.rs92.62% <91.66%> (+0.25%)⬆️
lightning-net-tokio/src/lib.rs75.88% <0.00%> (-0.81%)⬇️
lightning/src/debug_sync.rs94.79% <0.00%> (-0.27%)⬇️
lightning/src/util/events.rs33.21% <0.00%> (-0.24%)⬇️
lightning-invoice/src/de.rs81.06% <0.00%> (-0.21%)⬇️
lightning/src/ln/functional_tests.rs97.06% <0.00%> (-0.10%)⬇️
lightning/src/routing/scoring.rs94.29% <0.00%> (ø)
lightning/src/routing/network_graph.rs89.54% <0.00%> (+0.01%)⬆️
lightning/src/ln/channelmanager.rs84.85% <0.00%> (+0.05%)⬆️
... and 4 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 5ed2985...ff7ec0c. Read the comment docs.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
ba73f3e to
0b238c0CompareMarch 15, 2022 23:04
TheBlueMatt
TheBlueMatt
reviewed
Mar 16, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

 // In order to do so, we pre-sort by total fees paid, so that in case of equal
 // values we prefer lower cost paths.
 // (Descending order, so we drop higher-fee paths first)
 cur_route.sort_by_key(|path| path.get_total_fee_paid_msat());
@TheBlueMattTheBlueMattMar 16, 2022

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.

This should include the scoring results, too, no?
@tnulltnullMar 17, 2022•

edited

Loading

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.

Yes, you're probably right. Maybe also htlc_minimum_msat? Do you have a suggestion how we would prioritize values, total fees, penalties, and HTLC minima?
@TheBlueMattTheBlueMattMar 17, 2022

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.

Hmm, right, yea, the issue with htlc min is it really just kicks in after a threshold. Maybe the way to address that is loop at the reduction stage and if we hit htlc min try the next path instead of just reducing and moving on. Then we could just sort by score+fee here (which is all designed to be added, so we always just add for that).
@tnulltnullMar 21, 2022

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'm now considering the score with 9175523. Not sure if the additional complexity needed to incorporate htlc_minimum_msat is worth the gain?
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
0b238c0 to
84fa714CompareMarch 17, 2022 14:17
tnull
tnull
commented
Mar 17, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
@@ -1522,7 +1522,7 @@ where L::Target: Logger {
 // Now, subtract the overpaid value from the most-expensive path.
 // TODO: this could also be optimized by also sorting by feerate_per_sat_routed,
@tnulltnullMar 17, 2022•

edited

Loading

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'm not fully sure how to address this TODO while I'm here. What would we gain if we multisort here again? Don't we want to just optimize for fees at this point?
@TheBlueMattTheBlueMattMar 17, 2022

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.

Right, the TODO here notes that sorting by the path value isn't optimizing for fees, optimizing for a real feerate would be, instead of just feerate ignoring base fee.
@tnulltnullMar 21, 2022•

edited

Loading

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'm probably missing something here, but isn't this what sorting once more by get_total_fees_paid_msat() would do?
TheBlueMatt
TheBlueMatt
reviewed
Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

@tnull
tnull
commented
Mar 21, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Added a simple refactor and will probably also try to address this TODO in router "while im here".
TheBlueMatt
TheBlueMatt
reviewed
Mar 21, 2022
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.

LGTM, can you clean up the git log somewhat?
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
0a322d3 to
9a2b8f2CompareMarch 21, 2022 22:57
@tnull
tnull
commented
Mar 21, 2022

Copy link


Copy Markdown

ContributorAuthor
LGTM, can you clean up the git log somewhat?
Squashed without changes.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
9a2b8f2 to
013954eCompareMarch 21, 2022 23:02
tnull
tnull
commented
Mar 21, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

@TheBlueMatt
TheBlueMatt
commented
Mar 21, 2022

Copy link


Copy Markdown

Collaborator
Hmm, it would be kinda nice if the unrelated changes are in a different commit, eg the wrapping adds and sort changes could be a separate commit.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
013954e to
90474d2CompareMarch 21, 2022 23:14
TheBlueMatt
TheBlueMatt
previously approved these changes
Mar 21, 2022
View reviewed changes
@jkczyz
jkczyz
self-requested a review
March 21, 2022 23:50
jkczyz
jkczyz
reviewed
Mar 22, 2022
View reviewed changes
@jkczyzjkczyz
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. Just some nits.
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs
Outdated

jkczyz
jkczyz
reviewed
Mar 24, 2022
View reviewed changes
@jkczyzjkczyz
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.

Will need to insert the last commit between the first two commits given it's a fixup for the first commit and will be squashed into it whereas the second commit is independent. I typically use git rebase -i when needing to interleave fixups.
Comment threadlightning/src/routing/router.rs
tnull
added 2 commits
March 24, 2022 09:12
@tnull
Randomize candidate paths during route selection.
b11dcf9
@tnull
Minor refactor for sort / add, and some nits.
ff7ec0c
- `sort_by_key` to `sort_unstable_by_key`
- `checked_add() .. max_value()` to `saturating_add()`
- Some typos and nits
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
d5a0738 to
ff7ec0cCompareMarch 24, 2022 15:13
@TheBlueMattTheBlueMatt
self-assigned this
Mar 24, 2022
@TheBlueMatt
TheBlueMatt
merged commit 216225c
into
lightningdevkit:mainMar 24, 2022
@tnulltnull
mentioned this pull request
May 16, 2022

Open

@TheBlueMattTheBlueMatt
mentioned this pull request
Jul 13, 2022
 Merged

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

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@jkczyzjkczyzjkczyz approved these changes

Assignees

@TheBlueMattTheBlueMatt

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Change route selection algorithm to be actually (pseudo-)random


4 participants

@tnull@codecov-commenter@TheBlueMatt@jkczyz

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Randomize candidate paths during route selection. by tnull · Pull Request #1359 · lightningdevkit/rust-lightning · GitHub
Skip to content
Randomize candidate paths during route selection. - #1359
Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
tnull:2022-03-real-random-shuffle
Mar 24, 2022
Merged
Randomize candidate paths during route selection.#1359
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
tnull:2022-03-real-random-shuffle
Conversation
@tnull
@tnulltnull
commented
Mar 10, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR is a follow-up to #1286: it implements a 'real random shuffle' to randomize candidate payment paths during route selection. I played with a few variants of this and now ended up generating a number of unique permutations.
During testing it became apparent that the behavior of Step (7) was indeterministic and dependent on the input order of paths when they were of equal value_msat, which broke htlc_minimum_overpay_test. I now addressed this by pre-sorting the paths according to their total_fee_paid_msat, which should drop more expensive paths first, i.e., always prefer lower-fee paths of equal value.
Closes#869.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
a36e655 to
ba73f3eCompareMarch 12, 2022 13:32
@codecov-commenter
codecov-commenter
commented
Mar 12, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1359 (d5a0738) into main (5ed2985) will increase coverage by 0.05%.

The diff coverage is 91.66%.
❗ Current head d5a0738 differs from pull request most recent head ff7ec0c. Consider uploading reports for the commit ff7ec0c to get more accurate results
@@ Coverage Diff @@## main #1359 +/- ##
==========================================
+ Coverage 90.75% 90.81% +0.05% 
==========================================
Files 73 73 Lines 40884 41215 +331 Branches 40884 41215 +331 ==========================================
+ Hits 37106 37428 +322 - Misses 3778 3787 +9 
Impacted FilesCoverage Δ
lightning/src/routing/router.rs92.62% <91.66%> (+0.25%)⬆️
lightning-net-tokio/src/lib.rs75.88% <0.00%> (-0.81%)⬇️
lightning/src/debug_sync.rs94.79% <0.00%> (-0.27%)⬇️
lightning/src/util/events.rs33.21% <0.00%> (-0.24%)⬇️
lightning-invoice/src/de.rs81.06% <0.00%> (-0.21%)⬇️
lightning/src/ln/functional_tests.rs97.06% <0.00%> (-0.10%)⬇️
lightning/src/routing/scoring.rs94.29% <0.00%> (ø)
lightning/src/routing/network_graph.rs89.54% <0.00%> (+0.01%)⬆️
lightning/src/ln/channelmanager.rs84.85% <0.00%> (+0.05%)⬆️
... and 4 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 5ed2985...ff7ec0c. Read the comment docs.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
ba73f3e to
0b238c0CompareMarch 15, 2022 23:04
TheBlueMatt
TheBlueMatt
reviewed
Mar 16, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

 // In order to do so, we pre-sort by total fees paid, so that in case of equal
 // values we prefer lower cost paths.
 // (Descending order, so we drop higher-fee paths first)
 cur_route.sort_by_key(|path| path.get_total_fee_paid_msat());
@TheBlueMattTheBlueMattMar 16, 2022

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.

This should include the scoring results, too, no?
@tnulltnullMar 17, 2022•

edited

Loading

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.

Yes, you're probably right. Maybe also htlc_minimum_msat? Do you have a suggestion how we would prioritize values, total fees, penalties, and HTLC minima?
@TheBlueMattTheBlueMattMar 17, 2022

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.

Hmm, right, yea, the issue with htlc min is it really just kicks in after a threshold. Maybe the way to address that is loop at the reduction stage and if we hit htlc min try the next path instead of just reducing and moving on. Then we could just sort by score+fee here (which is all designed to be added, so we always just add for that).
@tnulltnullMar 21, 2022

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'm now considering the score with 9175523. Not sure if the additional complexity needed to incorporate htlc_minimum_msat is worth the gain?
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
0b238c0 to
84fa714CompareMarch 17, 2022 14:17
tnull
tnull
commented
Mar 17, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
@@ -1522,7 +1522,7 @@ where L::Target: Logger {
 // Now, subtract the overpaid value from the most-expensive path.
 // TODO: this could also be optimized by also sorting by feerate_per_sat_routed,
@tnulltnullMar 17, 2022•

edited

Loading

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'm not fully sure how to address this TODO while I'm here. What would we gain if we multisort here again? Don't we want to just optimize for fees at this point?
@TheBlueMattTheBlueMattMar 17, 2022

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.

Right, the TODO here notes that sorting by the path value isn't optimizing for fees, optimizing for a real feerate would be, instead of just feerate ignoring base fee.
@tnulltnullMar 21, 2022•

edited

Loading

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'm probably missing something here, but isn't this what sorting once more by get_total_fees_paid_msat() would do?
TheBlueMatt
TheBlueMatt
reviewed
Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

@tnull
tnull
commented
Mar 21, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Added a simple refactor and will probably also try to address this TODO in router "while im here".
TheBlueMatt
TheBlueMatt
reviewed
Mar 21, 2022
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.

LGTM, can you clean up the git log somewhat?
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
0a322d3 to
9a2b8f2CompareMarch 21, 2022 22:57
@tnull
tnull
commented
Mar 21, 2022

Copy link


Copy Markdown

ContributorAuthor
LGTM, can you clean up the git log somewhat?
Squashed without changes.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
9a2b8f2 to
013954eCompareMarch 21, 2022 23:02
tnull
tnull
commented
Mar 21, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

@TheBlueMatt
TheBlueMatt
commented
Mar 21, 2022

Copy link


Copy Markdown

Collaborator
Hmm, it would be kinda nice if the unrelated changes are in a different commit, eg the wrapping adds and sort changes could be a separate commit.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
013954e to
90474d2CompareMarch 21, 2022 23:14
TheBlueMatt
TheBlueMatt
previously approved these changes
Mar 21, 2022
View reviewed changes
@jkczyz
jkczyz
self-requested a review
March 21, 2022 23:50
jkczyz
jkczyz
reviewed
Mar 22, 2022
View reviewed changes
@jkczyzjkczyz
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. Just some nits.
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs
Outdated

jkczyz
jkczyz
reviewed
Mar 24, 2022
View reviewed changes
@jkczyzjkczyz
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.

Will need to insert the last commit between the first two commits given it's a fixup for the first commit and will be squashed into it whereas the second commit is independent. I typically use git rebase -i when needing to interleave fixups.
Comment threadlightning/src/routing/router.rs
tnull
added 2 commits
March 24, 2022 09:12
@tnull
Randomize candidate paths during route selection.
b11dcf9
@tnull
Minor refactor for sort / add, and some nits.
ff7ec0c
- `sort_by_key` to `sort_unstable_by_key`
- `checked_add() .. max_value()` to `saturating_add()`
- Some typos and nits
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
d5a0738 to
ff7ec0cCompareMarch 24, 2022 15:13
@TheBlueMattTheBlueMatt
self-assigned this
Mar 24, 2022
@TheBlueMatt
TheBlueMatt
merged commit 216225c
into
lightningdevkit:mainMar 24, 2022
@tnulltnull
mentioned this pull request
May 16, 2022

Open

@TheBlueMattTheBlueMatt
mentioned this pull request
Jul 13, 2022
 Merged

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

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@jkczyzjkczyzjkczyz approved these changes

Assignees

@TheBlueMattTheBlueMatt

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Change route selection algorithm to be actually (pseudo-)random


4 participants

@tnull@codecov-commenter@TheBlueMatt@jkczyz

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Randomize candidate paths during route selection. by tnull · Pull Request #1359 · lightningdevkit/rust-lightning · GitHub
Skip to content
Randomize candidate paths during route selection. - #1359
Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
tnull:2022-03-real-random-shuffle
Mar 24, 2022
Merged
Randomize candidate paths during route selection.#1359
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
tnull:2022-03-real-random-shuffle
Conversation
@tnull
@tnulltnull
commented
Mar 10, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR is a follow-up to #1286: it implements a 'real random shuffle' to randomize candidate payment paths during route selection. I played with a few variants of this and now ended up generating a number of unique permutations.
During testing it became apparent that the behavior of Step (7) was indeterministic and dependent on the input order of paths when they were of equal value_msat, which broke htlc_minimum_overpay_test. I now addressed this by pre-sorting the paths according to their total_fee_paid_msat, which should drop more expensive paths first, i.e., always prefer lower-fee paths of equal value.
Closes#869.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
a36e655 to
ba73f3eCompareMarch 12, 2022 13:32
@codecov-commenter
codecov-commenter
commented
Mar 12, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1359 (d5a0738) into main (5ed2985) will increase coverage by 0.05%.

The diff coverage is 91.66%.
❗ Current head d5a0738 differs from pull request most recent head ff7ec0c. Consider uploading reports for the commit ff7ec0c to get more accurate results
@@ Coverage Diff @@## main #1359 +/- ##
==========================================
+ Coverage 90.75% 90.81% +0.05% 
==========================================
Files 73 73 Lines 40884 41215 +331 Branches 40884 41215 +331 ==========================================
+ Hits 37106 37428 +322 - Misses 3778 3787 +9 
Impacted FilesCoverage Δ
lightning/src/routing/router.rs92.62% <91.66%> (+0.25%)⬆️
lightning-net-tokio/src/lib.rs75.88% <0.00%> (-0.81%)⬇️
lightning/src/debug_sync.rs94.79% <0.00%> (-0.27%)⬇️
lightning/src/util/events.rs33.21% <0.00%> (-0.24%)⬇️
lightning-invoice/src/de.rs81.06% <0.00%> (-0.21%)⬇️
lightning/src/ln/functional_tests.rs97.06% <0.00%> (-0.10%)⬇️
lightning/src/routing/scoring.rs94.29% <0.00%> (ø)
lightning/src/routing/network_graph.rs89.54% <0.00%> (+0.01%)⬆️
lightning/src/ln/channelmanager.rs84.85% <0.00%> (+0.05%)⬆️
... and 4 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 5ed2985...ff7ec0c. Read the comment docs.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
ba73f3e to
0b238c0CompareMarch 15, 2022 23:04
TheBlueMatt
TheBlueMatt
reviewed
Mar 16, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

 // In order to do so, we pre-sort by total fees paid, so that in case of equal
 // values we prefer lower cost paths.
 // (Descending order, so we drop higher-fee paths first)
 cur_route.sort_by_key(|path| path.get_total_fee_paid_msat());
@TheBlueMattTheBlueMattMar 16, 2022

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.

This should include the scoring results, too, no?
@tnulltnullMar 17, 2022•

edited

Loading

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.

Yes, you're probably right. Maybe also htlc_minimum_msat? Do you have a suggestion how we would prioritize values, total fees, penalties, and HTLC minima?
@TheBlueMattTheBlueMattMar 17, 2022

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.

Hmm, right, yea, the issue with htlc min is it really just kicks in after a threshold. Maybe the way to address that is loop at the reduction stage and if we hit htlc min try the next path instead of just reducing and moving on. Then we could just sort by score+fee here (which is all designed to be added, so we always just add for that).
@tnulltnullMar 21, 2022

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'm now considering the score with 9175523. Not sure if the additional complexity needed to incorporate htlc_minimum_msat is worth the gain?
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
0b238c0 to
84fa714CompareMarch 17, 2022 14:17
tnull
tnull
commented
Mar 17, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
@@ -1522,7 +1522,7 @@ where L::Target: Logger {
 // Now, subtract the overpaid value from the most-expensive path.
 // TODO: this could also be optimized by also sorting by feerate_per_sat_routed,
@tnulltnullMar 17, 2022•

edited

Loading

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'm not fully sure how to address this TODO while I'm here. What would we gain if we multisort here again? Don't we want to just optimize for fees at this point?
@TheBlueMattTheBlueMattMar 17, 2022

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.

Right, the TODO here notes that sorting by the path value isn't optimizing for fees, optimizing for a real feerate would be, instead of just feerate ignoring base fee.
@tnulltnullMar 21, 2022•

edited

Loading

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'm probably missing something here, but isn't this what sorting once more by get_total_fees_paid_msat() would do?
TheBlueMatt
TheBlueMatt
reviewed
Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

@tnull
tnull
commented
Mar 21, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Added a simple refactor and will probably also try to address this TODO in router "while im here".
TheBlueMatt
TheBlueMatt
reviewed
Mar 21, 2022
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.

LGTM, can you clean up the git log somewhat?
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
0a322d3 to
9a2b8f2CompareMarch 21, 2022 22:57
@tnull
tnull
commented
Mar 21, 2022

Copy link


Copy Markdown

ContributorAuthor
LGTM, can you clean up the git log somewhat?
Squashed without changes.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
9a2b8f2 to
013954eCompareMarch 21, 2022 23:02
tnull
tnull
commented
Mar 21, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

@TheBlueMatt
TheBlueMatt
commented
Mar 21, 2022

Copy link


Copy Markdown

Collaborator
Hmm, it would be kinda nice if the unrelated changes are in a different commit, eg the wrapping adds and sort changes could be a separate commit.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
013954e to
90474d2CompareMarch 21, 2022 23:14
TheBlueMatt
TheBlueMatt
previously approved these changes
Mar 21, 2022
View reviewed changes
@jkczyz
jkczyz
self-requested a review
March 21, 2022 23:50
jkczyz
jkczyz
reviewed
Mar 22, 2022
View reviewed changes
@jkczyzjkczyz
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. Just some nits.
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs
Outdated

jkczyz
jkczyz
reviewed
Mar 24, 2022
View reviewed changes
@jkczyzjkczyz
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.

Will need to insert the last commit between the first two commits given it's a fixup for the first commit and will be squashed into it whereas the second commit is independent. I typically use git rebase -i when needing to interleave fixups.
Comment threadlightning/src/routing/router.rs
tnull
added 2 commits
March 24, 2022 09:12
@tnull
Randomize candidate paths during route selection.
b11dcf9
@tnull
Minor refactor for sort / add, and some nits.
ff7ec0c
- `sort_by_key` to `sort_unstable_by_key`
- `checked_add() .. max_value()` to `saturating_add()`
- Some typos and nits
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
d5a0738 to
ff7ec0cCompareMarch 24, 2022 15:13
@TheBlueMattTheBlueMatt
self-assigned this
Mar 24, 2022
@TheBlueMatt
TheBlueMatt
merged commit 216225c
into
lightningdevkit:mainMar 24, 2022
@tnulltnull
mentioned this pull request
May 16, 2022

Open

@TheBlueMattTheBlueMatt
mentioned this pull request
Jul 13, 2022
 Merged

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

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@jkczyzjkczyzjkczyz approved these changes

Assignees

@TheBlueMattTheBlueMatt

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Change route selection algorithm to be actually (pseudo-)random


4 participants

@tnull@codecov-commenter@TheBlueMatt@jkczyz

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Randomize candidate paths during route selection. by tnull · Pull Request #1359 · lightningdevkit/rust-lightning · GitHub
Skip to content
Randomize candidate paths during route selection. - #1359
Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
tnull:2022-03-real-random-shuffle
Mar 24, 2022
Merged
Randomize candidate paths during route selection.#1359
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
tnull:2022-03-real-random-shuffle
Conversation
@tnull
@tnulltnull
commented
Mar 10, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR is a follow-up to #1286: it implements a 'real random shuffle' to randomize candidate payment paths during route selection. I played with a few variants of this and now ended up generating a number of unique permutations.
During testing it became apparent that the behavior of Step (7) was indeterministic and dependent on the input order of paths when they were of equal value_msat, which broke htlc_minimum_overpay_test. I now addressed this by pre-sorting the paths according to their total_fee_paid_msat, which should drop more expensive paths first, i.e., always prefer lower-fee paths of equal value.
Closes#869.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
a36e655 to
ba73f3eCompareMarch 12, 2022 13:32
@codecov-commenter
codecov-commenter
commented
Mar 12, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1359 (d5a0738) into main (5ed2985) will increase coverage by 0.05%.

The diff coverage is 91.66%.
❗ Current head d5a0738 differs from pull request most recent head ff7ec0c. Consider uploading reports for the commit ff7ec0c to get more accurate results
@@ Coverage Diff @@## main #1359 +/- ##
==========================================
+ Coverage 90.75% 90.81% +0.05% 
==========================================
Files 73 73 Lines 40884 41215 +331 Branches 40884 41215 +331 ==========================================
+ Hits 37106 37428 +322 - Misses 3778 3787 +9 
Impacted FilesCoverage Δ
lightning/src/routing/router.rs92.62% <91.66%> (+0.25%)⬆️
lightning-net-tokio/src/lib.rs75.88% <0.00%> (-0.81%)⬇️
lightning/src/debug_sync.rs94.79% <0.00%> (-0.27%)⬇️
lightning/src/util/events.rs33.21% <0.00%> (-0.24%)⬇️
lightning-invoice/src/de.rs81.06% <0.00%> (-0.21%)⬇️
lightning/src/ln/functional_tests.rs97.06% <0.00%> (-0.10%)⬇️
lightning/src/routing/scoring.rs94.29% <0.00%> (ø)
lightning/src/routing/network_graph.rs89.54% <0.00%> (+0.01%)⬆️
lightning/src/ln/channelmanager.rs84.85% <0.00%> (+0.05%)⬆️
... and 4 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 5ed2985...ff7ec0c. Read the comment docs.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
ba73f3e to
0b238c0CompareMarch 15, 2022 23:04
TheBlueMatt
TheBlueMatt
reviewed
Mar 16, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

 // In order to do so, we pre-sort by total fees paid, so that in case of equal
 // values we prefer lower cost paths.
 // (Descending order, so we drop higher-fee paths first)
 cur_route.sort_by_key(|path| path.get_total_fee_paid_msat());
@TheBlueMattTheBlueMattMar 16, 2022

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.

This should include the scoring results, too, no?
@tnulltnullMar 17, 2022•

edited

Loading

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.

Yes, you're probably right. Maybe also htlc_minimum_msat? Do you have a suggestion how we would prioritize values, total fees, penalties, and HTLC minima?
@TheBlueMattTheBlueMattMar 17, 2022

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.

Hmm, right, yea, the issue with htlc min is it really just kicks in after a threshold. Maybe the way to address that is loop at the reduction stage and if we hit htlc min try the next path instead of just reducing and moving on. Then we could just sort by score+fee here (which is all designed to be added, so we always just add for that).
@tnulltnullMar 21, 2022

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'm now considering the score with 9175523. Not sure if the additional complexity needed to incorporate htlc_minimum_msat is worth the gain?
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
0b238c0 to
84fa714CompareMarch 17, 2022 14:17
tnull
tnull
commented
Mar 17, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
@@ -1522,7 +1522,7 @@ where L::Target: Logger {
 // Now, subtract the overpaid value from the most-expensive path.
 // TODO: this could also be optimized by also sorting by feerate_per_sat_routed,
@tnulltnullMar 17, 2022•

edited

Loading

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'm not fully sure how to address this TODO while I'm here. What would we gain if we multisort here again? Don't we want to just optimize for fees at this point?
@TheBlueMattTheBlueMattMar 17, 2022

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.

Right, the TODO here notes that sorting by the path value isn't optimizing for fees, optimizing for a real feerate would be, instead of just feerate ignoring base fee.
@tnulltnullMar 21, 2022•

edited

Loading

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'm probably missing something here, but isn't this what sorting once more by get_total_fees_paid_msat() would do?
TheBlueMatt
TheBlueMatt
reviewed
Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

@tnull
tnull
commented
Mar 21, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Added a simple refactor and will probably also try to address this TODO in router "while im here".
TheBlueMatt
TheBlueMatt
reviewed
Mar 21, 2022
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.

LGTM, can you clean up the git log somewhat?
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
0a322d3 to
9a2b8f2CompareMarch 21, 2022 22:57
@tnull
tnull
commented
Mar 21, 2022

Copy link


Copy Markdown

ContributorAuthor
LGTM, can you clean up the git log somewhat?
Squashed without changes.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
9a2b8f2 to
013954eCompareMarch 21, 2022 23:02
tnull
tnull
commented
Mar 21, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

@TheBlueMatt
TheBlueMatt
commented
Mar 21, 2022

Copy link


Copy Markdown

Collaborator
Hmm, it would be kinda nice if the unrelated changes are in a different commit, eg the wrapping adds and sort changes could be a separate commit.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
013954e to
90474d2CompareMarch 21, 2022 23:14
TheBlueMatt
TheBlueMatt
previously approved these changes
Mar 21, 2022
View reviewed changes
@jkczyz
jkczyz
self-requested a review
March 21, 2022 23:50
jkczyz
jkczyz
reviewed
Mar 22, 2022
View reviewed changes
@jkczyzjkczyz
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. Just some nits.
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs
Outdated

jkczyz
jkczyz
reviewed
Mar 24, 2022
View reviewed changes
@jkczyzjkczyz
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.

Will need to insert the last commit between the first two commits given it's a fixup for the first commit and will be squashed into it whereas the second commit is independent. I typically use git rebase -i when needing to interleave fixups.
Comment threadlightning/src/routing/router.rs
tnull
added 2 commits
March 24, 2022 09:12
@tnull
Randomize candidate paths during route selection.
b11dcf9
@tnull
Minor refactor for sort / add, and some nits.
ff7ec0c
- `sort_by_key` to `sort_unstable_by_key`
- `checked_add() .. max_value()` to `saturating_add()`
- Some typos and nits
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
d5a0738 to
ff7ec0cCompareMarch 24, 2022 15:13
@TheBlueMattTheBlueMatt
self-assigned this
Mar 24, 2022
@TheBlueMatt
TheBlueMatt
merged commit 216225c
into
lightningdevkit:mainMar 24, 2022
@tnulltnull
mentioned this pull request
May 16, 2022

Open

@TheBlueMattTheBlueMatt
mentioned this pull request
Jul 13, 2022
 Merged

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

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@jkczyzjkczyzjkczyz approved these changes

Assignees

@TheBlueMattTheBlueMatt

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Change route selection algorithm to be actually (pseudo-)random


4 participants

@tnull@codecov-commenter@TheBlueMatt@jkczyz

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

edited

Loading

Copy link


Copy Markdown

Contributor
This PR is a follow-up to #1286: it implements a 'real random shuffle' to randomize candidate payment paths during route selection. I played with a few variants of this and now ended up generating a number of unique permutations.
During testing it became apparent that the behavior of Step (7) was indeterministic and dependent on the input order of paths when they were of equal value_msat, which broke htlc_minimum_overpay_test. I now addressed this by pre-sorting the paths according to their total_fee_paid_msat, which should drop more expensive paths first, i.e., always prefer lower-fee paths of equal value.
Closes#869.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
a36e655 to
ba73f3eCompareMarch 12, 2022 13:32
@codecov-commenter
codecov-commenter
commented
Mar 12, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1359 (d5a0738) into main (5ed2985) will increase coverage by 0.05%.

The diff coverage is 91.66%.
❗ Current head d5a0738 differs from pull request most recent head ff7ec0c. Consider uploading reports for the commit ff7ec0c to get more accurate results
@@ Coverage Diff @@## main #1359 +/- ##
==========================================
+ Coverage 90.75% 90.81% +0.05% 
==========================================
Files 73 73 Lines 40884 41215 +331 Branches 40884 41215 +331 ==========================================
+ Hits 37106 37428 +322 - Misses 3778 3787 +9 
Impacted FilesCoverage Δ
lightning/src/routing/router.rs92.62% <91.66%> (+0.25%)⬆️
lightning-net-tokio/src/lib.rs75.88% <0.00%> (-0.81%)⬇️
lightning/src/debug_sync.rs94.79% <0.00%> (-0.27%)⬇️
lightning/src/util/events.rs33.21% <0.00%> (-0.24%)⬇️
lightning-invoice/src/de.rs81.06% <0.00%> (-0.21%)⬇️
lightning/src/ln/functional_tests.rs97.06% <0.00%> (-0.10%)⬇️
lightning/src/routing/scoring.rs94.29% <0.00%> (ø)
lightning/src/routing/network_graph.rs89.54% <0.00%> (+0.01%)⬆️
lightning/src/ln/channelmanager.rs84.85% <0.00%> (+0.05%)⬆️
... and 4 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 5ed2985...ff7ec0c. Read the comment docs.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
ba73f3e to
0b238c0CompareMarch 15, 2022 23:04
TheBlueMatt
TheBlueMatt
reviewed
Mar 16, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Outdated

 // In order to do so, we pre-sort by total fees paid, so that in case of equal
 // values we prefer lower cost paths.
 // (Descending order, so we drop higher-fee paths first)
 cur_route.sort_by_key(|path| path.get_total_fee_paid_msat());
@TheBlueMattTheBlueMattMar 16, 2022

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.

This should include the scoring results, too, no?
@tnulltnullMar 17, 2022•

edited

Loading

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.

Yes, you're probably right. Maybe also htlc_minimum_msat? Do you have a suggestion how we would prioritize values, total fees, penalties, and HTLC minima?
@TheBlueMattTheBlueMattMar 17, 2022

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.

Hmm, right, yea, the issue with htlc min is it really just kicks in after a threshold. Maybe the way to address that is loop at the reduction stage and if we hit htlc min try the next path instead of just reducing and moving on. Then we could just sort by score+fee here (which is all designed to be added, so we always just add for that).
@tnulltnullMar 21, 2022

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'm now considering the score with 9175523. Not sure if the additional complexity needed to incorporate htlc_minimum_msat is worth the gain?
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
0b238c0 to
84fa714CompareMarch 17, 2022 14:17
tnull
tnull
commented
Mar 17, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
@@ -1522,7 +1522,7 @@ where L::Target: Logger {
 // Now, subtract the overpaid value from the most-expensive path.
 // TODO: this could also be optimized by also sorting by feerate_per_sat_routed,
@tnulltnullMar 17, 2022•

edited

Loading

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'm not fully sure how to address this TODO while I'm here. What would we gain if we multisort here again? Don't we want to just optimize for fees at this point?
@TheBlueMattTheBlueMattMar 17, 2022

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.

Right, the TODO here notes that sorting by the path value isn't optimizing for fees, optimizing for a real feerate would be, instead of just feerate ignoring base fee.
@tnulltnullMar 21, 2022•

edited

Loading

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'm probably missing something here, but isn't this what sorting once more by get_total_fees_paid_msat() would do?
TheBlueMatt
TheBlueMatt
reviewed
Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Mar 20, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

@tnull
tnull
commented
Mar 21, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Added a simple refactor and will probably also try to address this TODO in router "while im here".
TheBlueMatt
TheBlueMatt
reviewed
Mar 21, 2022
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.

LGTM, can you clean up the git log somewhat?
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
0a322d3 to
9a2b8f2CompareMarch 21, 2022 22:57
@tnull
tnull
commented
Mar 21, 2022

Copy link


Copy Markdown

ContributorAuthor
LGTM, can you clean up the git log somewhat?
Squashed without changes.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
9a2b8f2 to
013954eCompareMarch 21, 2022 23:02
tnull
tnull
commented
Mar 21, 2022
View reviewed changes
Comment threadlightning/src/routing/router.rs
Outdated

@TheBlueMatt
TheBlueMatt
commented
Mar 21, 2022

Copy link


Copy Markdown

Collaborator
Hmm, it would be kinda nice if the unrelated changes are in a different commit, eg the wrapping adds and sort changes could be a separate commit.
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
013954e to
90474d2CompareMarch 21, 2022 23:14
TheBlueMatt
TheBlueMatt
previously approved these changes
Mar 21, 2022
View reviewed changes
@jkczyz
jkczyz
self-requested a review
March 21, 2022 23:50
jkczyz
jkczyz
reviewed
Mar 22, 2022
View reviewed changes
@jkczyzjkczyz
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. Just some nits.
Comment threadlightning/src/routing/router.rs
Outdated

Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs
Outdated

jkczyz
jkczyz
reviewed
Mar 24, 2022
View reviewed changes
@jkczyzjkczyz
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.

Will need to insert the last commit between the first two commits given it's a fixup for the first commit and will be squashed into it whereas the second commit is independent. I typically use git rebase -i when needing to interleave fixups.
Comment threadlightning/src/routing/router.rs
tnull
added 2 commits
March 24, 2022 09:12
@tnull
Randomize candidate paths during route selection.
b11dcf9
@tnull
Minor refactor for sort / add, and some nits.
ff7ec0c
- `sort_by_key` to `sort_unstable_by_key`
- `checked_add() .. max_value()` to `saturating_add()`
- Some typos and nits
@tnull
tnullforce-pushed
the
2022-03-real-random-shuffle
branch
from
d5a0738 to
ff7ec0cCompareMarch 24, 2022 15:13
@TheBlueMattTheBlueMatt
self-assigned this
Mar 24, 2022
@TheBlueMatt
TheBlueMatt
merged commit 216225c
into
lightningdevkit:mainMar 24, 2022
@tnulltnull
mentioned this pull request
May 16, 2022

Open

@TheBlueMattTheBlueMatt
mentioned this pull request
Jul 13, 2022
 Merged

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

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@jkczyzjkczyzjkczyz approved these changes

Assignees

@TheBlueMattTheBlueMatt

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Change route selection algorithm to be actually (pseudo-)random


4 participants

@tnull@codecov-commenter@TheBlueMatt@jkczyz