bugfix payment lifecycle payment attempts#10125
bugfix payment lifecycle payment attempts#10125guggero merged 4 commits intolightningnetwork:masterfrom
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @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 addresses a bug in the payment lifecycle related to payment attempts. The core change involves modifying the HTLC (Hashed Timelock Contract) forwarding logic to bypass a specific policy check for locally forwarded payments, which was presumably causing incorrect behavior or failures.
Highlights
- HTLC Policy Check Bypass: I've identified that the
CheckHtlcTransitpolicy validation for outgoing HTLCs on local links withinhtlcswitch/switch.gohas been commented out. This change, affecting lines 925-935, effectively disables the outgoing channel policy check for locally forwarded HTLCs. This is likely a targeted fix to address a bug where this specific check was causing issues or was incorrectly applied to local payment attempts.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request comments out a block of code responsible for validating locally-initiated payments against the outgoing channel's policy. While the title suggests this is a bugfix, the lack of a description and the use of commented-out code makes the intent unclear. My primary concern is that this change weakens validation and could lead to inefficient payment attempts that are rejected by peers. I've left a comment asking for clarification and suggesting that commented-out code should be avoided in favor of removal with explanation.
9c8ae52 to
7b03809
Compare
|
We need to find a proper way how we are going to fix this, because I am not sure when Tapd needs to skip this check, so open for suggestions how to carve-out the check here. cc @guggero |
403478d to
e981bb5
Compare
e981bb5 to
a2506e4
Compare
a2506e4 to
51147e6
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request fixes a bug where LND could enter an endless loop trying to send a payment that violates a local channel policy, such as max_htlc. The fix involves refactoring the HTLC amount validation to correctly identify and skip policy checks for custom HTLCs, preventing them from being caught in this loop. A new integration test is added to cover this scenario. My review focuses on style guide adherence and potential improvements in error handling. I've found a minor style guide violation in a function signature, a typo in a comment, and a case of silent error handling that could be improved with logging.
b691344 to
4ed1231
Compare
Shouldn't this be prevented as we check up front if we can transit a link before sending? EDIT: nvm that's only called after we send a payment down into the link |
|
Re the above, perhaps this is caused a divergence between the local max HTLC value and the value that's set with the routing policy? We'll skip adding an edge if it exceeds the max amt policy for a given channel: Lines 190 to 197 in c4db070 So I think my hypothesis above may be correct. The channel update max HTLC can be changed, but the link level version can't until something like dynamic commitments exists. |
Ah ok, I missed this bit (🥁). |
guggero
left a comment
There was a problem hiding this comment.
Looks good, thanks a lot for the fix! Just a few small suggestions.
4ed1231 to
ddb21a9
Compare
ddb21a9 to
a19e7f2
Compare
Problem description:
Looking through some logs of bigger node-runners I saw a lot of these messages:
Making some local testing I realized that LND will easily end up in a 1 min endless loop always trying the same channel because local failures are currently not registered in the Mission-Control setup.
See here:
Problem was introduced here:
#9049
but only since #8390 (LND 19) this bug would be triggered.
Code part:
lnd/routing/unified_edges.go
Line 273 in 37523b6
Codewise the following happens:
lnd/routing/unified_edges.go
Line 273 in 37523b6
because the EndorsementBit is always set in the payment firstHop data here:
lnd/lnrpc/routerrpc/router_backend.go
Lines 936 to 949 in 37523b6
lnd/routing/result_interpretation.go
Lines 209 to 235 in 37523b6
lnd/lnrpc/routerrpc/router_server.go
Line 358 in 37523b6
This is just a preliminary commit to discuss potential fixes:
TrafficShapersignals it