lnrpc: include a status indicator for channel verification operations

[ardevd]

Change Description

Description of change / link to associated issue.

Steps to Test

1. Export a backup.
2. Verify the backup with lncli verifychanbackup.
   Response now includes a success indicator.

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

[ardevd]

I also wonder if we want to keep it so that the verification returns an error directly or whether the rpc response can set success to false and include an optional message in the response? Thoughts?

[guggero]

I also wonder if we want to keep it so that the verification returns an error directly or whether the rpc response can set success to false and include an optional message in the response? Thoughts?

I don't think we want to change that behavior. When using RPCs programmatically you want to be sure that you get an RPC error back when something goes wrong. If there is no error, you can assume that everything was fine. That's the reason why the original response was empty, because there can only ever be failure or success (which makes sense on an API level but looks weird on the command line). So this PR is purely cosmetic for the CLI user but shouldn't change the RPC/API behavior.

Code looks like it should be working, but can you add a check here that the return value is true now? And add release notes for 0.16.1 as well as run the formatter. Thanks!

[ardevd]

@guggero Done!

[guggero]

Thanks, LGTM 🎉

Just two small nits remaining.

[yyforyongyu]

Can this ever be false?

[guggero]

No, this is purely to improve the output on the command line (which currently is just empty braces).

[guggero]

@ardevd can you rebase this on top of the 0-16-1-staging branch please? Then we can get it merged.

[ardevd]

Of course. However, having spent more time with lnd I realize that the empty json responses are used quite extensively to indicate a successful operation. Introducing a "success" indicator for backup verification alone might add more confusion than clarity. Any thoughts @guggero ?

[guggero]

Yes, it takes some getting used to, but eventually it's easy to interpret the "no error message means success" pattern of lnd. Though I think it would be quite nice if we at least got an idea of what's in a backup? So we could instead return the number of channels contained (doesn't make a lot of sense for a single backup file) or the channel points of the backup?

[ardevd]

Agreed. Makes more sense than to effectively have a hardcoded Boolean value. It would actually be pretty useful to get an indication of the number of channel points that were indeed processed/verified.

[yyforyongyu]

Should we rebase this, update the 0.17.0 release notes, and merge it?

[guggero]

I can take this over for 0.17. Would be nice to get a list of channel points contained in a backup instead of just a boolean value. @ardevd do you plan on continuing with this or should I take it over?

[ardevd]

I'd love to work on this but am currently pretty time constrained. Happy for you to take it over @guggero :)

[lightninglabs-deploy]

@ardevd, remember to re-request review from reviewers when ready

[yyforyongyu]

Closed due to inactivity.