lncli: validate negative sat_per_vbyte in closechannel

[Suvrat1629]

Change Description

This PR fixes a bug in lncli closechannel (Issue #9834) where a negative --sat_per_vbyte (e.g., -1) was accepted and wrapped to a large uint64, causing invalid fee rates. It adds validation to reject negative values, aligning with gRPC behavior.

Steps to Test

Steps for reviewers to follow to test the change.

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative (error paths) are included.
• Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• The change is not insubstantial. Typo fixes are not accepted to fight bot spam.
• The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• Commits follow the Ideal Git Commit Structure.
• Any new logging statements use an appropriate subsystem and logging level.
• Any new lncli commands have appropriate tags in the comments for the rpc in the proto file.
• There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

[gemini-code-assist]

Summary of Changes

Hello @Suvrat1629, I'm Gemini Code Assist¹! 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 enhances the robustness of the lncli closechannel command by implementing client-side validation for the --sat_per_vbyte parameter. This change prevents the acceptance of negative fee rate inputs, which previously caused incorrect fee calculations due to integer wrapping, thereby improving the reliability of channel closure operations.

Highlights

• lncli closechannel Bug Fix: Resolves a bug where negative --sat_per_vbyte values were incorrectly processed, leading to large uint64 values and invalid fee rates.
• Negative Fee Rate Validation: Introduces explicit validation to reject negative inputs for the sat_per_vbyte flag in lncli closechannel, aligning behavior with gRPC and preventing unexpected fee calculations.

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. 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

1. 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. ↩

[gemini-code-assist]

Code Review

This pull request correctly adds validation to prevent negative values for sat_per_vbyte in lncli closechannel, which previously caused an integer overflow. The implementation is sound. My review includes a suggestion to make the comments more concise and aligned with the project's style guide, which prefers explaining the 'why' over the 'what'.

[starius]

I managed to reproduce the original bug in regtest (LND 0.19.0).

In LNG log I see the following:

[DBG] RPCS: [/lnrpc.Lightning/CloseChannel] requested
[WRN] RPCS: Expected either 'sat_per_vbyte' or 'conf_target' to be set, using default conf of 6 instead

From this I conclude that the sent value was 0, not -1.

[starius]

Other commands may be also affected by this:

cmd/commands/cmd_open_channel.go:
lncli channelopen

cmd/commands/commands.go:
lncli sendcoins
lncli sendmany
lncli closeallchannels

All of them have flag sat_per_vbyte which is of int64 type.

I propose to apply the fix to all of them.

[starius]

Please sign your commits:
https://github.com/lightningnetwork/lnd/blob/master/docs/development_guidelines.md#sign-your-git-commits

[starius]

Please also change it for this command:

cmd/commands/cmd_open_channel.go:
lncli channelopen

[Suvrat1629]

@starius That should wrap it I guess.

[yyforyongyu]

Please make sure to follow the Pull Request Checklist.

[Suvrat1629]

@yyforyongyu Sorry could you be a bit more specific this is my first time contributing to lnd. You need me to add test cases for the changes I have made I presume?

[Suvrat1629]

@starius I was taking a look to add the test case for the sat_per_vbyte change I have made. But I had a question do I just add one test case for all the commands as all of them follow pretty much the same way in which they handle the negative sat_per_vbyte or am I expected to add different test cases for each command?

[saubyk]

@yyforyongyu Sorry could you be a bit more specific this is my first time contributing to lnd. You need me to add test cases for the changes I have made I presume?

@Suvrat1629 please make sure that you've gone through these docs:
https://github.com/lightningnetwork/lnd/blob/master/docs/code_contribution_guidelines.md
https://github.com/lightningnetwork/lnd/blob/master/docs/development_guidelines.md

[Suvrat1629]

@saubyk Thank you, I am working on the test cases! Will add them once this comment is addressed comment

[starius]

@Suvrat1629 I'm not aware about a method this fix can be tested automatically. It is a fix of lncli, not lnd, so it can't be tested in itests IIUC.

[Suvrat1629]

@starius Sorry I don't know exactly what is expected of me here but I have added a test please take a look. I you find it unnecessary I will remove it.

[starius]

@Suvrat1629 I reworked the test and pushed the patch to this PR. If you like the approach, please squash all the commits and extend the test with other affected commands.

[Suvrat1629]

@starius Well I have never squashed commits before but I have given it a shot and also extended the test cases please take a look. If there is any problem in the squashed commits I will close this pr and reopen it.

[starius]

@Suvrat1629 Check this how to squash multiple commits into one: https://stackoverflow.com/a/5189600

[Suvrat1629]

@starius Instead of working on squashing all commits I found an easier work around for this issue😅.Please take a look and tell me if it works for you. I apologize for the inconvenience I am causing over this pr.

[starius]

@Suvrat1629 The commit structure should be clear. There should not be merge commits in the PR.
You can do this:

$ git pull --rebase https://github.com/lightningnetwork/lnd master

This will rebase your changes on top of the master branch in the repo.

Then squash everything left into a single commit using this instruction: https://stackoverflow.com/a/5189600

Then you can do git push -f to push the changes to GitHub.

[Suvrat1629]

@starius Well I guess that should do it then. Once again thanks for your patience.

[starius]

Left few comments.
Please change commit description to "lncli: validate negative sat_per_vbyte in closechannel".

[starius]

LGTM! Thanks for the patch!
I checked the test - it captures the fix.
Please squash commits again into one commit.

[Suvrat1629]

@starius I have squashed the commits into one. Thanks for your patience and guidance learned quite a few things and also got started with contributing in lnd :)

[lightninglabs-deploy]

@yyforyongyu: review reminder
@Suvrat1629, remember to re-request review from reviewers when ready

[Suvrat1629]

@yyforyongyu @saubyk Gentle ping please take a look.
Thank you.