Randomize candidate paths during route selection. #1359
Conversation
tnull
force-pushed
the
2022-03-real-random-shuffle
branch
from
a36e655 to
ba73f3e
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
tnull
force-pushed
the
2022-03-real-random-shuffle
branch
from
ba73f3e to
0b238c0
Compare
lightning/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()); |
There was a problem hiding this comment.
This should include the scoring results, too, no?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
force-pushed
the
2022-03-real-random-shuffle
branch
from
0b238c0 to
84fa714
Compare
| @@ -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, | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm probably missing something here, but isn't this what sorting once more by get_total_fees_paid_msat() would do?
|
Added a simple refactor and will probably also try to address this TODO in |
tnull
force-pushed
the
2022-03-real-random-shuffle
branch
from
0a322d3 to
9a2b8f2
Compare
Squashed without changes. |
tnull
force-pushed
the
2022-03-real-random-shuffle
branch
from
9a2b8f2 to
013954e
Compare
|
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
force-pushed
the
2022-03-real-random-shuffle
branch
from
013954e to
90474d2
Compare
- `sort_by_key` to `sort_unstable_by_key` - `checked_add() .. max_value()` to `saturating_add()` - Some typos and nits
tnull
force-pushed
the
2022-03-real-random-shuffle
branch
from
d5a0738 to
ff7ec0c
Compare
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 equalvalue_msat, which brokehtlc_minimum_overpay_test. I now addressed this by pre-sorting the paths according to theirtotal_fee_paid_msat, which should drop more expensive paths first, i.e., always prefer lower-fee paths of equal value.Closes #869.