backport/enhance lsp heuritic #10444
Conversation
The comment was incorrectly referring to HasLightningNode but the function is named HasNode. Update the comment to match the actual function name.
This commit adds the HasNode function to the RouterBackend struct, which checks if a node exists in the graph (i.e., has public channels). This function is needed by the LSP detection heuristic to determine if a node is publicly reachable. The function is wired up in rpcserver.go to query the graph database.
This commit implements a comprehensive LSP (Lightning Service Provider)
detection heuristic and updates the payment probing logic to handle
multiple LSPs with worst-case fee estimation.
Key changes:
1. LSP Detection Heuristic (isLSP function):
Implements three rules to detect LSP setups:
- Rule 1: If invoice target is public → NOT an LSP (route directly)
- Rule 2: If at least one destination hop is public → IS an LSP
- Rule 3: If all destination hops are private → NOT an LSP
2. LSP Route Preparation (prepareLspRouteHints function):
- Groups route hints by unique public LSP nodes
- Filters out non-LSP routes based on the heuristic
- Tracks worst-case fees and CLTV delays for each LSP
- Returns adjusted route hints with LSP hop stripped
3. Multi-LSP Probing (probePaymentRequest updates):
- Probes up to 3 unique LSPs maximum (griefing protection)
- Selects the WORST-CASE (most expensive) route for conservative
fee estimation
- Adds comprehensive debug logging for worst-case selection process
- Properly formats vertex logging using %v (calls Vertex.String())
The worst-case approach ensures users won't be surprised by higher fees
when the actual payment is sent, providing a more conservative and
reliable fee estimate.
This commit also adds extensive unit test coverage for the LSP detection
heuristic and route preparation logic.
TestIsLsp:
- Edge cases: empty route hints, nil scenarios
- Rule 1: Public invoice target (3 tests)
- Rule 2: All private destination hops (4 tests)
- Rule 3: At least one public destination hop (6 tests)
TestPrepareLspRouteHints:
- LSP grouping and filtering logic
- Worst-case fee selection across route hints
- Worst-case CLTV delta tracking
- Adjusted route hints validation (LSP hop stripped)
- Multi-LSP scenarios with different fees
This commit enhances the integration test to validate the LSP heuristic end-to-end with real network topology and payment probing. Network topology additions: - Added Frank node as a private destination - Created multi-LSP test scenario with Bob, Eve, and Dave as LSPs New test cases: 1. "probe based estimate, public target with public hop hints" - Validates Rule 1: public invoice target routes directly - Even with public hop hints, direct routing is used - Expected: standard single-hop fees 2. "probe based estimate, multiple different public LSPs" - Validates multi-LSP worst-case selection - Frank has routes through Bob (low fee), Eve (HIGH fee), Dave (medium) - Expected: Eve's worst-case fees (most expensive) - Tests griefing protection (max 3 LSP probes)
Summary of ChangesHello @ziggie1984, 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 refines the Highlights
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
|
![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 significantly enhances the LSP detection heuristic for fee estimation. The new logic, based on a clear set of rules, is more robust and correctly handles multi-LSP scenarios by probing for worst-case fees. The changes are well-tested and the documentation has been updated to reflect the new behavior. My feedback is focused on a couple of comments in the implementation that seem to contradict the new logic of finding the most expensive route, not the cheapest.
| // If the heuristic indicates an LSP, we filter and group route hints by | ||
| // public LSP nodes, then probe each unique LSP separately and return | ||
| // the cheapest route. |
There was a problem hiding this comment.
The comment here states that the logic will return the 'cheapest route'. However, the implementation, particularly the logic for worstCaseResp, is designed to find and return the worst-case (most expensive) route. The release notes and test comments also confirm this. To avoid confusion, this comment should be updated to reflect that the goal is to find the most expensive route for conservative fee estimation.
| // If the heuristic indicates an LSP, we filter and group route hints by | |
| // public LSP nodes, then probe each unique LSP separately and return | |
| // the cheapest route. | |
| // If the heuristic indicates an LSP, we filter and group route hints by | |
| // public LSP nodes, then probe each unique LSP separately and return | |
| // the worst-case (most expensive) route. |
There was a problem hiding this comment.
not changing it here because it needs to be changed on master as well
| // LspRouteGroup represents a group of route hints that share the same public | ||
| // LSP destination node. This is needed when probing LSPs separately to find | ||
| // the cheapest route. |
There was a problem hiding this comment.
The comment for LspRouteGroup mentions that it's used for finding the 'cheapest route'. This seems to contradict the implementation's goal of finding the worst-case (most expensive) fee estimate. To maintain consistency with the implementation and the overall goal of this feature, I suggest updating the comment to mention 'worst-case' or 'most expensive' route.
| // LspRouteGroup represents a group of route hints that share the same public | |
| // LSP destination node. This is needed when probing LSPs separately to find | |
| // the cheapest route. | |
| // LspRouteGroup represents a group of route hints that share the same public | |
| // LSP destination node. This is needed when probing LSPs separately to find | |
| // the worst-case (most expensive) route. |
Roasbeef
merged commit c6467a6
into
lightningnetwork:v0.20.x-branch
2 participants
backports: #10396