MPP on the router side #646
Conversation
Codecov Report
@@ Coverage Diff @@
## master #646 +/- ##
==========================================
+ Coverage 91.23% 93.21% +1.98%
==========================================
Files 37 49 +12
Lines 22623 33943 +11320
==========================================
+ Hits 20639 31639 +11000
- Misses 1984 2304 +320
Continue to review full report at Codecov.
|
|
Hmm, a few notes then:
a) we need to not use MPP unless we have to - no point hurting our privacy/reliability unless the channel we’re routing over is small compared to our HTLC value,
b) in that context, skipping only one channel seems like a strange approach - we should avoid channels which are small compared to our HTLC value, not one at random.
As for randomness in general, users need to provide it. If we need a random value, we can add a parameter to get random bytes/number.
… On Jun 23, 2020, at 05:21, Gleb Naumenko ***@***.***> wrote:
@naumenkogs commented on this pull request.
In lightning/src/routing/router.rs:
> + res.push(new_entry);
+ }
+ res.last_mut().unwrap().fee_msat = final_value_msat;
+ res.last_mut().unwrap().cltv_expiry_delta = final_cltv;
+ payment_paths.insert(res);
+ break 'path_construction;
+ }
+ match network.get_nodes().get(&pubkey) {
+ None => {},
+ Some(node) => {
+ let mut channels_to_avoid = HashSet::new();
+ if removable_used_channels.len() > 0 {
+ // This sampling can be improved when we update the rand crate.
+ let mut rng = thread_rng();
+ let used_channels_indexes = Range::new(0, removable_used_channels.len());
+ // We avoid just one random channel currently, this can be improved,
It's actually very simple.
First A* pass, we compute a regular path (fully quit if not found). Store all used channels but first/last hops in removable_used_channels.
From removable_used_channels take 1 random channel and put it in channels_to_avoid
Do second A* pass, but skip those channels belonging to channels_to_avoid (currently just 1 channel always). If found, add used channels to removable_used_channels
Repeat 2-3 until found enough paths or wasted all tries.
(Non-first A* passes can currently hit a duplicate paths with each other, but that's probably fine)
As I mentioned in the original post, there are a bunch of improvements I'm planning to make (for instance, 2 can "avoid" multiple paths).
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
My initial intention was to expose how many paths shall be found to the caller (see Then, what do you think about this plan:
|
|
That sounds like a reasonable approach to me, there's also the htlc_maximum_msat field. |
naumenkogs
force-pushed
the
2020-06-router-mpp
branch
3 times, most recently
from
26561ab to
1b8bf5e
Compare
|
@TheBlueMatt I sketched an implementation, please take a look. It has a bunch of TODOs, and tests are not fixed against the changes. |
|
Hmm, right, if I'm reading this right it does seem to be the right approach, but there's a ton of complexity that isn't clear how to solve. eg you can't just say "using_low_capacity_channel" as a boolean, you need to track the value you're limited to, and then figure out how many paths you need to reach that value. Also, really annoyingly, I don't think its practical to do this in a perfectly-lowest-cost way, because once you calculate a path (but find out that its value-limited), you have to go back along that path and recalculate fees with the new value (and thus new proportional_msat fees at each hop). Thus, you may end up finding a path that is really low value that would have a low fee if it wasnt low value, but which actually has a high fee because its mostly static fees. One approach, I think, would be to just find a number of paths and then at the end select the best paths until you have enough value - only add channels who's values are "too low" to channels_to_avoid as you go, then at the end of the path go back and calculate the real fee. Reapeat until you have a number of candidate paths and mix + match the lowest fees until you find enough total value. Once you have a candidate paths list, you should be able to calcualte, for each, a absolute and relative feerate total, and then you can shift the balance towards the relative paths. |
|
@TheBlueMatt if we do MPP, how do we communicate the suggested path<->amount mapping? Unless I'm missing something I should modify Route struct? |
|
Correct - Route is now a list of "paths", and each RouteHop in each Path must have its value set to include the fees taken by that path, etc. So the algorithm needs to figure out how to split the value across different paths and then fill in the fees/value for each. |
naumenkogs
force-pushed
the
2020-06-router-mpp
branch
from
1b8bf5e to
b67d879
Compare
|
alright, another draft :) So the idea is:
|
naumenkogs
force-pushed
the
2020-06-router-mpp
branch
from
b67d879 to
23343cc
Compare
naumenkogs
force-pushed
the
2020-06-router-mpp
branch
from
23343cc to
d1759e4
Compare
|
Added a lot of comments |
There was a problem hiding this comment.
Just dumb questions for now. I'm still grasping the problem magnitude, I've to confess I wasn't a Routing Algorithms CS major :p
Maybe we should split up get_route in multiple logical chunks first ? We will succeed to merge this current PR but next time we touch about this code we or future reviewers may be lost again.
The two macros are logically heavy. We can make them separate functions, sure, but they will never be reused outside Other than that, I think the algorithm reads sequentially and it's already logically split. I agree it's a difficult place in the codebase, but I'm not sure how to make it easier. |
There was a problem hiding this comment.
Concept ACK.
I think that's an endless debate to try to get an efficient MPP algo, get_route refactoring, clear high-level documentation and good test coverage all at once :)
I'm good enough if we have simple tests demonstrating MPP is working.
Note, I've tried to refactor get_route by extracting macro : https://github.com/ariard/rust-lightning/pull/new/2020-09-refactor-routing. I think that's the right way to go, but let's get your stuff merged first and then I can try to split routing algorithm by logical components, even if they're only used once, code maintainability matters.
|
Here a rough model of pathfinding I've sketched for my own understanding. I don't propose it, it's more to help evaluating correctness of implemented algorithm. We have the following entities:
We assume we are the only node routing on the network and thus can't anticipate The problem we're solving is given a source S, a destination D and an amount A, A payment path weight is (sum of per-channel relay-fee) / (number of path channels). Conditions of payment path validity :
If we have multiple valid payment paths candidates, we choose the one with An additional condition (Multi-Payment-Path), if we don't have at least one payment We observe a weak interdependency in case of M > 1 payment paths. In case of AFAICT, how to allocate doesn't matter, the contended capacity is offered as the As a proposed routing algorithm :
|
naumenkogs
force-pushed
the
2020-06-router-mpp
branch
2 times, most recently
from
11cb677 to
b03b73a
Compare
|
I've addressed all the suggestions, now I'll finish some TODOs left and then start working on tests to see what it is capable of. @ariard I have hard times mapping your explanation to my algorithm... |
naumenkogs
force-pushed
the
2020-06-router-mpp
branch
from
ec237dd to
c09c383
Compare
|
Seems all passing except |
|
Chugging along, I'm still working on fixing the failing tests (though take a look at the https://github.com/TheBlueMatt/rust-lightning/commits/646-moar-test branch again, it should be right now, but still fails a few cases), but it managed to find its way into the |
naumenkogs
force-pushed
the
2020-06-router-mpp
branch
from
c09c383 to
7c7671a
Compare
|
@TheBlueMatt I fixed the specific |
naumenkogs
force-pushed
the
2020-06-router-mpp
branch
2 times, most recently
from
bb811de to
86196bd
Compare
|
Spent some time tracking down more errors - the same branch, above, is rebased on this PR and has one additional fix commit (which I dont believe is a regression in this PR, but breaking the fuzzer nonetheless). It has two commented out checks that both fail in the top commit. One with the input from https://pastebin.com/fRWGnSxZ the other with the input from https://pastebin.com/wxzbppjr. Given I don't think the top commit is a regression, I'd say leave it off and I (or you) can PR it with an additional test for that case. |
|
I agree. I updated one mistaken comment from earlier, added your commit and commented that we should do the same htlc_minimum_msat check in I think this can be merged now, although I didn't understand the point of this rename: |
naumenkogs
force-pushed
the
2020-06-router-mpp
branch
2 times, most recently
from
465734c to
8896492
Compare
|
Also, see one more commit addressing that TODO. Let me know if it causes some your test to fail, we can drop this code and leave for later. |
naumenkogs
force-pushed
the
2020-06-router-mpp
branch
from
a445567 to
725ee4b
Compare
naumenkogs
force-pushed
the
2020-06-router-mpp
branch
3 times, most recently
from
6b8d794 to
05d5955
Compare
| new_entry = match dist.remove(&ordered_hops.last().unwrap().route_hop.pubkey) { | ||
| Some(payment_hop) => payment_hop, | ||
| // We can't arrive at None because of how we traverse with add_entry. | ||
| None => unreachable!(), |
There was a problem hiding this comment.
@TheBlueMatt Is this "our (rather strange) modifications to dijkstras that might well be unique" you were mentioning on IRC ? If yes I would be glad to have more explications with a better comment :) Or maybe I should go on the slack thread between you and gleb
There was a problem hiding this comment.
I wrote up a long comment to be added in 815 at ef88cb0
At truncating the overpaid value, if we fall below htlc_minimum_msat, reach it by increasing fees.
We could have possibly constructed a slightly inconsistent path: since we reduce value being transferred all the way, we could have violated htlc_minimum_msat on some channels we already passed (assuming dest->source direction). Here, we recompute the fees again, so that if that's the case, we match the currently underpaid htlc_minimum_msat with fees.
naumenkogs
force-pushed
the
2020-06-router-mpp
branch
from
05d5955 to
e0600e5
Compare
I've implemented MPP for path finding part with minimum complexity overhead over given code. Closes #563.
Seeking approach ACK for now.