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
4 times, most recently
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.
Abdulkbk
force-pushed
the
onion-msg-path
branch
2 times, most recently
from
c33e0dd to
d39f72b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces pathfinding support for routing onion messages, enabling the router to find the shortest path through the channel graph for nodes advertising onion message support. This includes adding a new FindPath function, defining new error types (ErrNoPathFound, ErrDestinationNoOnionSupport, ErrNodeNotFound), and providing comprehensive unit tests for various pathfinding scenarios. The release notes have been updated, and a new contributor has been added. Review feedback indicates that the FindPath function signature needs to be wrapped to comply with line length limits, and several new functions (Error method of bfsDoneError, addNode, addEdge, vertexFromByte, onionFeatures, noOnionFeatures) require comments explaining their purpose and assumptions, as per the style guide.
Abdulkbk
force-pushed
the
onion-msg-path
branch
3 times, most recently
from
87dc101 to
628822f
Compare
There was a problem hiding this comment.
A great start for onion message pathfinding and nice tests 👍. I have two recommendations (see comments).
Some thoughts about future improvements (not for this PR):
- There could be multiple destination nodes if an offer contains several blinded paths.
- We may need several routes to the introduction node, to be able to send redundant onion messages (invoice requests) to the payment receiver. This way we can cut down the invoice response times and make the process more reliable. Perhaps a path-deletion approach for disjoint alternatives could give some redundancy. Otherwise some k-shortest path algo could help as well.
- We may want to track response times somehow to only use the faster nodes (can be a cutoff instead of a weight-based search), but this requires some correlation mechanism of request and response. It's probably better to check first if slow onion message router are a problem in practice.
Abdulkbk
force-pushed
the
onion-msg-path
branch
from
4d9aa89 to
12833ee
Compare
|
Thanks for the review @bitromortac, addressed your feedback.
These are all worth considering in follow ups as the feature progress, for example, the first point could be addressed when we integrate offer decoding. |
Abdulkbk
force-pushed
the
onion-msg-path
branch
from
12833ee to
88e78bc
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
3098b93 to
bf0d924
Compare
bitromortac
left a comment
bitromortac
left a comment
There was a problem hiding this comment.
Looks good, thank you for the updates, I just have some small remarks.
These are all worth considering in follow ups as the feature progress, for example, the first point could be addressed when we integrate offer decoding.
Right, it's probably best to wait for when the use case is uncovered.
| // | ||
| //nolint:ll | ||
| if errors.Is(err, context.Canceled) || | ||
| errors.Is(err, context.DeadlineExceeded) { | ||
|
|
||
| return err | ||
| } |
There was a problem hiding this comment.
This can be simplified to
// If the context is canceled or
// deadline exceeded, propagate
// the error.
if ctx.Err() != nil {
return err
}| log.Errorf("Unable to fetch features for destination %v: %v", | ||
| destination.String(), err) | ||
| return nil, err | ||
| } | ||
|
|
||
| // An empty feature vector means the node is absent from our graph. | ||
| // In that case, we send back a NotFound error. | ||
| if len(destFeatures.Features()) == 0 { | ||
| log.Debugf("Node %v not in graph, attempting direct "+ | ||
| "send", destination.String()) | ||
| return nil, ErrNodeNotFound | ||
| } | ||
|
|
||
| if !destFeatures.HasFeature(lnwire.OnionMessagesOptional) { | ||
| log.Debugf("Node %v does not support onion messages", | ||
| destination.String()) | ||
| return nil, ErrDestinationNoOnionSupport |
There was a problem hiding this comment.
I'd say we should remove the logging calls as the caller will have enough context of what to do with the error, which are fine-grained enough. The first error for the destination features may get wrapped with the details though.
5 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.