Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use channel balance in path-finding #1395

Merged
merged 7 commits into from May 11, 2020
Merged

Use channel balance in path-finding #1395

merged 7 commits into from May 11, 2020

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Apr 27, 2020

This is a step towards more flexibility for path-finding in the router.
I started wanting to simply add support for channel balance, but as I dived into the code I couldn't help myself and refactored, renamed, improved documentation and fixed some weirdness that I stumbled upon.
I hope the resulting code is easier to read and reason about, but I'm obviously biased so let me know how I can improve.

I split that work into three commits; it may help to review commit-by-commit, or to look at the resulting code instead of looking at the diff.

If you're not familiar with Yen's algorithm, the example on the wikipedia page should make it easy to understand.

Taking channel capacity into account

The first commit is quite small.

It updates the path finding algorithm to use channel capacity instead of htlcMaximumMsat.
It also takes into account channel balance when available and excludes channels that don't have enough funds to relay the payment.

This change also fixes an off-by-one error in weight computation: we were incorrectly applying a channel's fee to the amount that needs to be relayed through that channel (whereas this is instead what the node needs to receive to collect enough fee before relaying).

Refactoring the graph file

In the second commit, I heavily refactor the graph file without introducing functional changes (except for very minor tweaks). This is the one that's somewhat hard to review, but I hope the result makes it easier to understand for people who don't know the code.

Simplify path-finding implementations

In the third commit, I fix a couple confusing steps in the implementation of Yen's algorithm.

The first one was the computation of the edgesToIgnore and the specific handling of the case i = 0. This specific case wasn't needed and made the code a bit hard to read.

The second one was the weight provided to dijkstra for spur paths.
The weight of the root path was applied to the target node.
It was probably an attempt to take into account the fact that dijkstra wasn't computing a complete path and that fees may not match, but it couldn't really work.
I removed that and added a fee check at the end of the path-finding.

Tests

I did an apple-to-apple comparison between the new and old implementation, by selecting two nodes at random from the mainnet graph and trying to find 3 routes (Yen's algorithm) between them for different amounts. When the results of the old and new algorithm didn't match I manually checked the results.

The new algorithm consistently finds more routes and cheaper ones. The route prefix are more diverse, which is good as well (especially for MPP). The only cases where the old implementation was finding routes that the new implementation didn't find are cases where that route was in fact invalid, because one of the nodes was missing an htlcMaximumMsat and we didn't correctly take into account its capacity.

I also run performance benchmarks between the two algorithms, and the new one is consistently 25% faster on my machine (when looking for 3 routes).

t-bast added 3 commits Apr 27, 2020
The path finding algorithm uses channel capacity instead of htlcMaximumMsat.
It also takes into account channel balance when available and excludes
channels that don't have enough funds to relay the payment.

This change also fixes an off-by-one error in weight computation: we were
incorrectly applying a channel's fee to the amount that needs to be relayed
through that channel (whereas this is instead what the node needs to receive
to collect enough fee *before* relaying).
Add documentation, update comments, rename fields and reformat to (helpfully)
make the code clearer.

There is no functional change in this commit, this is just cosmetic.
Functional changes and bug fixes will come in next commits.
There were a couple confusing steps in the implementation of Yen's algorithm.

The first one was the computation of the `edgesToIgnore` and the specific
handling of the case i = 0. This specific case wasn't needed and made the
code a bit hard to read.

The second one was the weight provided to dijkstra for spur paths.
The weight of the root path was applied to the target node. It was probably
an attempt to take into account the fact that dijkstra wasn't computing
a complete path and that fees may not match, but it couldn't really work.
I removed that and added a fee check at the end of the path-finding.
@t-bast t-bast requested review from pm47, araspitzu and sstone and removed request for pm47 Apr 28, 2020
Copy link
Contributor

@araspitzu araspitzu left a comment

That's a nice overall improvement considering that balance will help us finding better routes 🎉

This change also fixes an off-by-one error in weight computation: we were incorrectly applying a channel's fee to the amount that needs to be relayed through that channel (whereas this is instead what the node needs to receive to collect enough fee before relaying).

Good catch! This is a little more precise estimation on whether the selected edge can relay the payment amount (plus fees).

I'm not sure about the refactoring done in 9d655c5 when this code was written i chose a slightly more imperative style because i think it's more readable and easier to understand when comparing it to pseudocode, i think the proposed refactoring loses a bit of readability but i'm not strongly opposed to it.

The second one was the weight provided to dijkstra for spur paths.
The weight of the root path was applied to the target node.
It was probably an attempt to take into account the fact that dijkstra wasn't computing a complete path and that fees may not match, but it couldn't really work.
I removed that and added a fee check at the end of the path-finding.

It's a way to make sure we don't find spur paths that exceeds the parameters for path-finding, i think with the proposed changes there is a regression: the proposed change might find an (invalid) spur path that is outside the boundaries parameters (i.e too hi CLTV) because we don't consider the previous part of the route (the root path); however the current code could find an alternative spur path that is coherent with the boundaries because it has knowledge of the total CLTV of the route (the total CLTV of the root path + the CLTV of the spur path).

Interesting results! By the way we have a JMH microbenchmark setup in this branch that i used in the past to benchmark the results of our path finding.

@t-bast
Copy link
Member Author

@t-bast t-bast commented May 4, 2020

I'm not sure about the refactoring done in 9d655c5 when this code was written i chose a slightly more imperative style because i think it's more readable and easier to understand when comparing it to pseudocode, i think the proposed refactoring loses a bit of readability but i'm not strongly opposed to it.

That's a reasonable argument indeed, the thing that lead me to that refactoring was mostly the return, because they have a quite surprising and not-so-expected behavior. I wanted to get rid of them and that slowly led me to that. But I don't mind going back to something more imperative, let's see what @sstone prefers to settle it!

EDIT: done in 702493f, let me know what you think (I can revert that commit if needed).

It's a way to make sure we don't find spur paths that exceeds the parameters for path-finding, i think with the proposed changes there is a regression: the proposed change might find an (invalid) spur path that is outside the boundaries parameters (i.e too hi CLTV) because we don't consider the previous part of the route (the root path); however the current code could find an alternative spur path that is coherent with the boundaries because it has knowledge of the total CLTV of the route (the total CLTV of the root path + the CLTV of the spur path).

This is handled by the added call to validatePath. Applying the weight like was done before was incorrect because the weight of the spur path will be different from the weight of the current shortest path.

If you have a current shortest path A -> B -> C -> E and use C as spur node to find A -> B -> C -> D -> E, the weight of C -> D -> E will be different from the weight of C -> E that you used for the root path, so the whole calculation of the root path cost is incorrect.

I think it's better to do a "normal" dijkstra to find C -> D -> E with the right final amount applied to E, then validate that concatenating both results in a valid path (and I find that the code is clearer).

However you're right that we may want validatePath to check the boundaries as well, I should add that. EDIT: actually boundaries are checked on the candidatePath, so we should be good.

t-bast added 2 commits May 4, 2020
This case regularly happens after a restart: the router already has the
latest channel_update for that channel, but we want to update the graph's
balances because they are all at `None` after a restart.
Copy link
Member

@sstone sstone left a comment

LGTM! And readability is better, just a few nits/comments

Copy link
Contributor

@araspitzu araspitzu left a comment

Just a small nit, for the rest it LGTM!

sstone
sstone approved these changes May 11, 2020
@t-bast t-bast merged commit ba4cca2 into master May 11, 2020
4 checks passed
@t-bast t-bast deleted the router-graph-balance branch May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants