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.
• [x] Tests covering the positive and negative (error paths) are included.
• [x] Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• [x] The change is not insubstantial. Typo fixes are not accepted to fight bot spam.
• [x] The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• [x] 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.

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

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

[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 :)

[Suvrat1629]

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

[Suvrat1629]

@yyforyongyu @starius Ok I have never really done the adding to 'release notes' thing could you help me with it. Which file do I need to change and what changes do I need to add.

[yyforyongyu]

You can take a look at this doc, and add a new entry in this one - basically breifly describe what's changed, and add your name to the contributor list below.

[Suvrat1629]

@starius @yyforyongyu I have made changes to release-notes-0.21.0.md. Please take a look. Thank you.

[Suvrat1629]

@yyforyongyu @starius @NishantBansal2003 Gentle ping

[lightninglabs-deploy]

@starius: review reminder @yyforyongyu: review reminder @nishantbansal2003: review reminder @suvrat1629, remember to re-request review from reviewers when ready