Avoid needless MPP on multiple channels to the same first-hop #1370
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #1370 +/- ##
==========================================
- Coverage 90.75% 90.75% -0.01%
==========================================
Files 73 73
Lines 40884 40906 +22
Branches 40884 40906 +22
==========================================
+ Hits 37106 37123 +17
- Misses 3778 3783 +5
Continue to review full report at Codecov.
|
ViktorT-11
reviewed
Mar 19, 2022
valentinewallace
previously approved these changes
Mar 22, 2022
Contributor
valentinewallace
left a comment
valentinewallace
left a comment
There was a problem hiding this comment.
Makes sense. Tried some random capacities above and below payment amount, seems to work as advertised
jkczyz
reviewed
Mar 23, 2022
Comment on lines
+818
to
+825
| // First, if channels are below `recommended_value_msat`, sort them in descending order, | ||
| // preferring larger channels to avoid splitting the payment into more MPP parts than is | ||
| // required. | ||
| // | ||
| // Second, because simply always sorting in descending order would always use our largest | ||
| // available outbound capacity, needlessly fragmenting our available channel capacities, | ||
| // sort channels above `recommended_value_msat` in ascending order, preferring channels | ||
| // which have enough, but not too much, capacity for the payment. |
Contributor
There was a problem hiding this comment.
Seems like the order of these two paragraphs should be swapped given the conditions are in the opposite order below.
Collaborator
Author
There was a problem hiding this comment.
I found it more readable this way, I'll just swap the conditional.
valentinewallace
previously approved these changes
Mar 23, 2022
jkczyz
previously approved these changes
Mar 23, 2022
When we have many channels to the same first-hop, many of which do not have sufficient balance to make the requested payment, but when some do, instead of simply using the available channel balance we may switch to MPP, potentially with many, many paths. Instead, we should seek to use the smallest channel which can easily handle the requested payment, which we do here by sorting the first_hops in our router before beginning the graph search. Note that the "real" fix for this should be to instead decide which channel to use at HTLC-send time, as most other nodes do during relay, but this provides a minimal fix without needing to do the rather-large work of refactoring our HTLC send+relay pipelines. Issues with overly-aggressive MPP on many channels were reported by Cash App.
TheBlueMatt
dismissed stale reviews from jkczyz and valentinewallace
via
b1bc078
TheBlueMatt
force-pushed
the
2022-03-pref-first-hop-chans
branch
from
81ecd4d to
b1bc078
Compare
Collaborator
Author
|
Squashed without changes. |
valentinewallace
approved these changes
Mar 23, 2022
jkczyz
approved these changes
Mar 23, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we have many channels to the same first-hop, many of which do
not have sufficient balance to make the requested payment, but when
some do, instead of simply using the available channel balance we
may switch to MPP, potentially with many, many paths.
Instead, we should seek to use the smallest channel which can
easily handle the requested payment, which we do here by sorting
the first_hops in our router before beginning the graph search.
Note that the "real" fix for this should be to instead decide which
channel to use at HTLC-send time, as most other nodes do during
relay, but this provides a minimal fix without needing to do the
rather-large work of refactoring our HTLC send+relay pipelines.
Issues with overly-aggressive MPP on many channels were reported by
Cash App.