-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Allow zero amount paths with MinHTLC policies #10029
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
base: master
Are you sure you want to change the base?
Allow zero amount paths with MinHTLC policies #10029
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @a-mpch, 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 resolves a bug that prevented the successful construction of blinded paths for zero-amount payments when channels along the path had a minimum HTLC requirement. The core change involves adjusting the path building logic to conditionally bypass the MinHTLC
validation during construction for zero-amount payments, allowing these paths to be created. A new test case has been added to validate this behavior.
Highlights
- Bug Fix: Fixed an issue where building blinded paths for zero-amount payments would fail if intermediate channels had a
MinHTLC
policy greater than zero. This prevented the creation of blinded path invoices for such payments. - Logic Adjustment: Modified the
collectRelayInfo
function inrouting/blindedpath/blinded_path.go
to skip theMinHTLC
validation check when theValueMsat
(payment amount) is zero. This ensures that theMinHTLC
constraint is only validated at payment time from the sender's perspective, not during path construction for zero-amount payments. - New Test Case: Added a new unit test,
TestBuildBlindedPathZeroAmount
, inrouting/blindedpath/blinded_path_test.go
to specifically cover the scenario of building zero-amount blinded paths through channels with non-zeroMinHTLC
policies, ensuring the fix works as intended and preventing regressions.
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 is currently in preview and 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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly allows building blinded paths for zero-amount payments even when intermediate channels have a MinHTLC
policy. The change in routing/blindedpath/blinded_path.go
modifies the validation logic to skip the MinHTLC
check for zero-amount values. A corresponding unit test is added to verify this new behavior.
My review identified a potential side-effect where policy buffering is disabled for zero-amount payments, which could be an oversight. I've also included a minor suggestion to improve code readability. Overall, the change is in the right direction and addresses the stated issue.
routing/blindedpath/blinded_path.go
Outdated
// If the payment amount is 0, means the sender is deciding the | ||
// amount, so we don't need to check the min HTLC value while | ||
// building the path. Payment would fail if there is no | ||
// route with the min HTLC value from the sender's perspective. | ||
if policy.MinHTLCMsat > cfg.ValueMsat && cfg.ValueMsat > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this change correctly avoids an error for zero-amount payments, it introduces an inconsistency with the policy buffering logic that follows.
Specifically, the check at line 462:
if bufferPolicy.MinHTLCMsat <= cfg.ValueMsat {
policy = bufferPolicy
}
For a zero-amount payment (cfg.ValueMsat == 0
), this condition becomes bufferPolicy.MinHTLCMsat <= 0
. If the buffered MinHTLCMsat
is positive (which is likely if the original policy had a positive MinHTLC
and a buffer was applied), the buffered policy will not be used.
This means for zero-amount invoices, we are not applying any policy buffer, which might not be the intended behavior as it could make probing easier.
To fix this, the condition at line 462 should also account for zero-amount payments, similar to the change in this PR. For example:
if cfg.ValueMsat == 0 || bufferPolicy.MinHTLCMsat <= cfg.ValueMsat {
policy = bufferPolicy
}
Since this line is outside the current diff, I recommend addressing this in a follow-up commit within this PR.
Additionally, the new test case in blinded_path_test.go
uses a no-op AddPolicyBuffer
function, so it doesn't catch this issue. It would be good to update the test to use a real buffer to verify the correct behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch. I amended the changes
49ba4f1
to
5e70077
Compare
When building blinded paths for zero amount payments (where the sender decides the amount), skip MinHTLC validation during path construction. Zero amount payments indicate the final amount will be determined by the sender, so MinHTLC constraints should be validated at payment time rather than path construction time. This enables creation of blinded path invoices for zero amount payments even when intermediate channels have MinHTLC policies greater than zero.
5e70077
to
873af3d
Compare
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly addresses an issue with creating zero-amount blinded path invoices when intermediate channels have a MinHTLC
policy. The logic to skip the MinHTLC
check for zero-amount payments is sound, and the addition of a regression test is a great way to prevent this issue from recurring. My review focuses on ensuring adherence to the project's style guide, specifically regarding line length limits, for which I've found a few violations.
I guess gemini is counting differently the 80 character long 👀 // 2) The new minHTLC value does not violate the sender amount. has 79 columns |
@a-mpch, remember to re-request review from reviewers when ready |
Change Description
Skip returning error when building blinded paths amount is zero and channel has a greater
MinHTLC
policy. This fixes #9256Previously, building blinded paths would fail when
ValueMsat
was 0 (zero amount payments) and intermediate channels hadMinHTLC
policies greater than zero. This prevented creation of blinded path invoices for zero amount payments.The fix adds a condition to skip MinHTLC validation during path construction when
ValueMsat
is 0, since zero amount indicates the sender will decide the final payment amount. MinHTLC constraints will still be validated at payment time from the sender's perspective.Steps to Test
Added a test
make unit pkg=routing/blindedpath case=TestBuildBlindedPathZeroAmount
and can be tested on a regtest network runninglicli addinvoice --blind --amt 0
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]
in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.