onionmessage: graph-based pathfinding for onion messages #10612
onionmessage: graph-based pathfinding for onion messages #10612Abdulkbk wants to merge 2 commits intolightningnetwork:masterfrom
Conversation
Summary of ChangesHello @Abdulkbk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly advances onion message capabilities by introducing a graph-based pathfinding mechanism. It refactors the internal actor system for more robust message handling and establishes a dedicated endpoint for processing and forwarding onion messages. These changes lay the groundwork for more sophisticated onion message interactions within the network, allowing nodes to efficiently discover and utilize paths while adhering to new feature signaling standards. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
The newly added commit is 744769f. |
lightninglabs-deploy
added
the
severity-critical
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces graph-based pathfinding for onion messages and refactors the actor model's mailbox implementation. My review focuses on improving code clarity, documentation, and robustness in the new onionmessage and related packages. I've suggested simplifying a boolean flag in the message endpoint, correcting a function comment to align with the style guide, adding a missing function comment, and adjusting a length check for more precise error handling in payload decoding. Overall, the changes are well-structured and the new functionality is thoroughly tested.
lightninglabs-deploy
added
severity-critical
lightninglabs-deploy
added
severity-critical
Abdulkbk
force-pushed
the
onion-msg-path
branch
from
49e6964 to
ea5cfb0
Compare
|
Rebased on master since #10089 is merged |
Abdulkbk
force-pushed
the
onion-msg-path
branch
4 times, most recently
from
5addfcd to
6727f67
Compare
|
@Abdulkbk, remember to re-request review from reviewers when ready |
Abdulkbk
force-pushed
the
onion-msg-path
branch
3 times, most recently
from
3943218 to
1e27ec9
Compare
In this commit we add FindPath, a BFS-based shsortest-path algorithm that finds routes through the channel graph for onion messages. The search filters nodes by the OnionMessage feature bits (38/39). We also add a unit tests covering: direct neighbor routing, multi-hop paths, feature-bit filtering, missing destination nodes, destination without onion support, max hop limits, cycle handling, and shortest-path selection. choice of BFS is because there isn't any weight involve.
Abdulkbk
force-pushed
the
onion-msg-path
branch
from
a137bab to
2e142e0
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces graph-based pathfinding for onion messages using a Breadth-First Search (BFS) algorithm, which is a solid approach for finding the shortest path by hop count. The implementation is clean and well-tested, with good coverage of various scenarios including feature filtering, cycles, and disconnected graphs. I have a couple of minor suggestions to improve readability and efficiency.
| ErrDestinationNoOnionSupport = errors.New("destination does not " + | ||
| "support onion messages") |
There was a problem hiding this comment.
The string concatenation here is unnecessary as the line is not long. It can be simplified to a single string literal for better readability, which is a key principle of the repository's style guide.
ErrDestinationNoOnionSupport = errors.New("destination does not support onion messages")References
- Readable code is the most important requirement for any commit created. (link)
| var path []route.Vertex | ||
|
|
||
| current := destination | ||
| for current != source { | ||
| path = append(path, current) | ||
| current = parent[current] | ||
| } | ||
|
|
||
| // Reverse the path to get source-to-destination order. | ||
| for i, j := 0, len(path)-1; i < j; i, j = i+1, j-1 { | ||
| path[i], path[j] = path[j], path[i] | ||
| } | ||
|
|
||
| return &OnionMessagePath{Hops: path} |
There was a problem hiding this comment.
The current implementation of reconstructPath first appends hops to a slice in reverse order and then reverses the slice. A more efficient and idiomatic approach in Go is to first determine the path length, pre-allocate the slice to the correct size, and then populate it in the correct order. This avoids repeated slice reallocations from append and the need for a separate reversal loop.
// Calculate path length to pre-allocate the slice.
pathLen := 0
for curr := destination; curr != source; curr = parent[curr] {
pathLen++
}
// Populate the path in correct order, avoiding a separate reversal step.
path := make([]route.Vertex, pathLen)
curr := destination
for i := pathLen - 1; i >= 0; i-- {
path[i] = curr
curr = parent[curr]
}
return &OnionMessagePath{Hops: path}3 participants
Change Description
Part of #10220
Depends on #10089
This PR builds on top of #10089 (onion message forwarding) and is part of the onion message tracking issue #10220. The forwarding layer can relay messages hop-by-hop, but a sender still needs a way to find a path through the graph to a destination. This PR adds that piece.
A FindPath function in
onionmessage/pathfind.gothat discovers a route from a local node to a destination through nodes that advertise onion message support (feature bit 38/39, OnionMessagesOptional/OnionMessagesRequired).We use
Breadth-First Search(BFS) over hop count. Since onion messages are not priced by fees or liquidity, the meaningful routing metric is path length. Shorter paths reduce the relay burden on intermediate nodes and lower latency. BFS naturally finds the minimum-hop path, which is appropriate for the channel graph. Intermediate nodes are included only if they advertise support for onion messages (bits 39/38).A set of unit tests is included to test for various scenarios.