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

Compute routes backward #797

Merged
merged 20 commits into from Jan 11, 2019

Conversation

Projects
None yet
3 participants
@araspitzu
Copy link
Member

araspitzu commented Jan 2, 2019

Compute the routes backward from the target and with the correct channels cost, nodes that are 'neighbor' to the local node (direct channel) will have a routing cost of 0.

araspitzu added some commits Dec 28, 2018

@araspitzu

This comment has been minimized.

Copy link
Member Author

araspitzu commented Jan 2, 2019

I chose to have an explicit parameter on the graph construction and manipulation functions, alternatively we can have a separate function with reverse in the name i.e "addEdgeReverse", please let me know what do you think of it.

araspitzu
@sstone

This comment has been minimized.

Copy link
Member

sstone commented Jan 2, 2019

Isn't it possible to use the same graph (i.e. not need for the additional reverse parameter) and just compute the route backwards internally ? It should be just an implementation choice that is not even exposed in the API ?

@araspitzu

This comment has been minimized.

Copy link
Member Author

araspitzu commented Jan 2, 2019

Sure it's possible, especially if we want to support only the graph in reverse order.

araspitzu added some commits Jan 2, 2019

araspitzu
@araspitzu

This comment has been minimized.

Copy link
Member Author

araspitzu commented Jan 3, 2019

Latest commit hid the "reverse" parameter and now the graph and shortestPath method work only in reverse mode (i left some comment in the docs to clarify the graph operations as it might be a little confusing)

@@ -791,7 +791,7 @@ object Router {
def findRoute(g: DirectedGraph, localNodeId: PublicKey, targetNodeId: PublicKey, amountMsat: Long, extraEdges: Set[GraphEdge] = Set.empty, ignoredEdges: Set[ChannelDesc] = Set.empty): Try[Seq[Hop]] = Try {
if (localNodeId == targetNodeId) throw CannotRouteToSelf

Graph.shortestPath(g, localNodeId, targetNodeId, amountMsat, ignoredEdges, extraEdges) match {
Graph.shortestPath(g, targetNodeId, localNodeId, amountMsat, ignoredEdges.map(reverseDesc), extraEdges.map(edge => edge.copy(desc = reverseDesc(edge.desc)))) match {

This comment has been minimized.

@sstone

sstone Jan 4, 2019

Member

I think we should keep the method and parameters as they were, we are still computing a shortest path from the source to the destination, it's just that internally we compute it backwards

araspitzu and others added some commits Jan 6, 2019

@pm47
Copy link
Member

pm47 left a comment

What is the overall performance impact in terms of compute time?

// note: the default value here will never be used, as there is always an entry for the current in the 'cost' map
val newMinimumKnownCost = cost.get(current.key) + edgeWeightByAmount(edge, amountMsat)
// test for ignored edges
if (!(edge.update.htlcMaximumMsat.exists(_ < newMinimumKnownCost) ||

This comment has been minimized.

@pm47

pm47 Jan 8, 2019

Member

is that the most optimal order for the testing conditions? Given that the first to return true will return immediately

This comment has been minimized.

@araspitzu

araspitzu Jan 10, 2019

Author Member

done - optimized in 1151af9

* @param isNeighborTarget true if the receiving vertex of this edge is the target node (source in a reversed graph), which has cost 0
* @return the new amount updated with the necessary fees for this edge
*/
private def edgeWeight(edge: GraphEdge, amountWithFees: Long, isNeighborTarget: Boolean): Long = isNeighborTarget match {

This comment has been minimized.

@pm47

pm47 Jan 8, 2019

Member

would an if be more optimal here?

This comment has been minimized.

@araspitzu

araspitzu Jan 9, 2019

Author Member

Depends on which one you consider more readable, i've used 'match' for testing conditions and this is consistent to it.

Show resolved Hide resolved eclair-core/src/main/scala/fr/acinq/eclair/router/Graph.scala Outdated
Show resolved Hide resolved eclair-core/src/main/scala/fr/acinq/eclair/router/Graph.scala Outdated
@araspitzu

This comment has been minimized.

Copy link
Member Author

araspitzu commented Jan 9, 2019

Performance impact is not significant, i just benchmarked it and the graph initialization is still around 2.4s with 13k channels (measurements not taken on the mobile phone). On the other hand we're now computing the correct routes thanks to the backward search and incremental edge cost.

Benchmark                                   Mode  Cnt  Score   Error  Units
GraphBenchmark.findPath                     avgt       0.002           s/op
GraphBenchmark.findPathWithExtraChannels    avgt       0.003           s/op
GraphBenchmark.findPathWithIgnoredChannels  avgt       0.019           s/op
GraphBenchmark.routerLoadingTime            avgt       2.401           s/op

araspitzu added some commits Jan 9, 2019

@araspitzu

This comment has been minimized.

Copy link
Member Author

araspitzu commented Jan 9, 2019

Todo: update the wiki as soon as this is merged

araspitzu added some commits Jan 10, 2019

@sstone

sstone approved these changes Jan 11, 2019

@sstone sstone merged commit fd3ff91 into master Jan 11, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@sstone sstone deleted the routing_backward branch Jan 11, 2019

@araspitzu

This comment has been minimized.

Copy link
Member Author

araspitzu commented Jan 11, 2019

wiki updated

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