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

Find route for messages #2656

Merged
merged 13 commits into from
Jun 19, 2023
Merged

Find route for messages #2656

merged 13 commits into from
Jun 19, 2023

Conversation

thomash-acinq
Copy link
Member

When sending a message, the postman can now ask the router to find a route using channels only. The same route is also used as a reply path when applicable.

@thomash-acinq thomash-acinq requested a review from t-bast May 11, 2023 14:08
@codecov-commenter
Copy link

codecov-commenter commented May 11, 2023

Codecov Report

Merging #2656 (0d63fdc) into master (b084d73) will increase coverage by 0.08%.
The diff coverage is 95.39%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #2656      +/-   ##
==========================================
+ Coverage   85.85%   85.93%   +0.08%     
==========================================
  Files         214      215       +1     
  Lines       17591    17728     +137     
  Branches      723      752      +29     
==========================================
+ Hits        15102    15235     +133     
- Misses       2489     2493       +4     
Impacted Files Coverage Δ
...ir-core/src/main/scala/fr/acinq/eclair/Setup.scala 75.42% <ø> (-0.14%) ⬇️
.../scala/fr/acinq/eclair/message/OnionMessages.scala 77.41% <0.00%> (-1.27%) ⬇️
...cala/fr/acinq/eclair/router/RouteCalculation.scala 94.56% <85.71%> (-0.87%) ⬇️
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala 57.87% <90.00%> (+2.10%) ⬆️
...c/main/scala/fr/acinq/eclair/message/Postman.scala 95.58% <94.00%> (-4.42%) ⬇️
.../src/main/scala/fr/acinq/eclair/router/Graph.scala 97.33% <96.49%> (-0.18%) ⬇️
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala 93.81% <100.00%> (+0.16%) ⬆️
...a/fr/acinq/eclair/payment/offer/OfferManager.scala 92.59% <100.00%> (ø)
...la/fr/acinq/eclair/payment/send/OfferPayment.scala 75.00% <100.00%> (ø)
...cinq/eclair/remote/EclairInternalsSerializer.scala 97.82% <100.00%> (+0.04%) ⬆️
... and 4 more

... and 16 files with indirect coverage changes

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This turned out more complex than I expected! I proposed some simplifications and refactorings in #2663. We're getting closer to an MVP release!

@thomash-acinq
Copy link
Member Author

I've merged your improvements but kept a separate routing algorithm for messages and payments. Routing messages is too different to be solved by the current payment routing algorithm and I find it much simpler to separate the two than to try to make something generic.

@thomash-acinq
Copy link
Member Author

Now using Dijkstra for message routing too. We can prioritize big channels, old channels and penalize disabled edges (but still be able to consider them).
Rebased on master to fix a conflict.

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks mostly good to me, but it's hard to gauge the potential performance regressions, and this is quite a critical component...I didn't realize we'd need that much changes on the graph structure (but I agree that they make sense and are necessary). I think we should leave this PR out of the release, we should spend time testing it on our node before releasing it. We'll make the release first, and will merge this to master right after the release and after your performance benchmarks.

ActiveEdge and DisabledEdge extend GraphEdge
Copy link
Member Author

@thomash-acinq thomash-acinq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some benchmarks:

DirectedGraph.makeGraph now takes 653ms instead of 225ms (x2.9)
.addEdges now takes 356ms instead of 2.676s to update the whole graph (x0.13)
yenKshortestPaths now takes 2.578s to find paths to the top 1000 nodes compared to 2.345s before (x1.1)

I don't think writing ad hoc code for DirectedGraph.makeGraph is worth it as it only called once at start up.
It looks like storing edges in a map instead of a list costs us 10% on the path-finding side and brings a 10x improvement on the update side.

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is looking good to me, that's really good work!
I'm going to spend more time on the benchmarks and I'll report early next week.

@t-bast
Copy link
Member

t-bast commented Jun 19, 2023

I'm finding the same results by tweaking your performance benchmarks. I think the additional 10% cost on path-finding is acceptable (even though it would be nice if we're able to improve that in the future), I'm a bit more frustrated at the makeGraph performance hit, but I don't see an easy way of improving it. We really are adding more data in the Graph structure, so it's expected that it's more complex to instantiate it...but that's also something we can try to improve in the future, let's start with that!

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@thomash-acinq thomash-acinq merged commit 194f5dd into master Jun 19, 2023
1 check passed
@thomash-acinq thomash-acinq deleted the route-message branch June 19, 2023 15:01
thomash-acinq added a commit that referenced this pull request Sep 25, 2023
Other implementations and Eclair (#2703) disable channels only when the other peer is offline.
So disabled channels should no be used for routing messages.

This PR removes `ActiveEdge` that was introduced in #2656
thomash-acinq added a commit that referenced this pull request Oct 2, 2023
Other implementations and Eclair (#2703) disable channels only when the other peer is offline.
So disabled channels should no be used for routing messages.

This PR removes `ActiveEdge` that was introduced in #2656
thomash-acinq added a commit that referenced this pull request Oct 2, 2023
Other implementations and Eclair (#2703) disable channels only when the other peer is offline.
So disabled channels should no be used for routing messages.

This PR removes `ActiveEdge` that was introduced in #2656
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