-
Notifications
You must be signed in to change notification settings - Fork 2.2k
lncli: show fee_per_vbyte and local_close_tx in closechannel and closeallchannels output #10300
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?
Conversation
…loseallchannels output - Update closechannel and closeallchannels commands to display fee_per_vbyte and local_close_tx in the JSON output when available. - Refactor executeChannelClose to send all close details (txid, fee, local flag) via a struct.
Summary of ChangesHello @Suvrat1629, 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 significantly improves the user experience for Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
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 refactors the closechannel and closeallchannels commands to include fee_per_vbyte and local_close_tx in their JSON output. The change introduces a new CloseInfo struct and modifies executeChannelClose to communicate more details about the channel closure. The implementation is mostly correct, but there is a critical issue that can lead to deadlocks in both commands if the channel close stream ends before sending closing transaction details. I've provided comments and suggestions to fix this by ensuring the communication channel is always closed and handling this case in the receiving goroutines.
| func executeChannelClose(ctxc context.Context, client lnrpc.LightningClient, | ||
| req *lnrpc.CloseChannelRequest, txidChan chan<- string, block bool) error { | ||
| req *lnrpc.CloseChannelRequest, closeInfoChan chan<- CloseInfo, block bool) error { | ||
|
|
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.
To prevent a potential deadlock in the calling functions (closeChannel and closeAllChannels), closeInfoChan should be closed when this function exits. If executeChannelClose returns without sending a value (for example, if the stream ends with io.EOF before a ClosePending update), the receiving goroutines will block indefinitely. Using defer to close the channel is the idiomatic way to ensure it's closed on all execution paths.
defer close(closeInfoChan)| closeInfo := <-closeInfoChan | ||
| printJSON(closeInfo) |
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.
To complement the proposed change of always closing closeInfoChan in executeChannelClose, this read operation should be updated to handle the case where the channel is closed without a value being sent. Using the two-variable assignment form of a channel receive (value, ok := <-ch) allows checking whether a value was received before attempting to use it, preventing the printing of an empty JSON object if no close information is sent.
closeInfo, ok := <-closeInfoChan
if ok {
printJSON(closeInfo)
}| closeInfo := <-closeInfoChan | ||
| res.ClosingTxid = closeInfo.ClosingTxid | ||
| res.FeePerVbyte = closeInfo.FeePerVbyte | ||
| res.LocalCloseTx = closeInfo.LocalCloseTx |
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.
This blocking read from closeInfoChan can lead to a deadlock if executeChannelClose returns without sending data. To fix this, you should handle the case where the channel is closed without a value being sent. In the context of closeAllChannels, not receiving close information when no error occurred should be treated as a failure for that specific channel closure, and an appropriate error message should be set in the result.
closeInfo, ok := <-closeInfoChan
if !ok {
res.FailErr = "channel close stream ended " +
"before sending closing transaction"
return
}
res.ClosingTxid = closeInfo.ClosingTxid
res.FeePerVbyte = closeInfo.FeePerVbyte
res.LocalCloseTx = closeInfo.LocalCloseTxThere 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.
Nice work 👍, the direction looks good
My main comment is FeePerVbyte and LocalCloseTx are only populated when the node opts for rbf-coop-close, otherwise, these values are not set. When returning the response, they appear with default (wrong) values, so we should decide whether this is the intended behavior or if they should be updated in all cases.
| txidChan := make(chan string, 1) | ||
| err = executeChannelClose(ctxc, client, req, txidChan, false) | ||
| closeInfoChan := make(chan CloseInfo, 1) | ||
| err = executeChannelClose(ctxc, client, req, closeInfoChan, false) |
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.
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.
Could you please point out what I am missing?
Thank you.
| if !sentCloseInfo { | ||
| closeInfo := CloseInfo{ | ||
| ClosingTxid: txid.String(), | ||
| FeePerVbyte: feeRate, | ||
| LocalCloseTx: isLocalClose, | ||
| } | ||
| closeInfoChan <- closeInfo | ||
| sentCloseInfo = true | ||
| } |
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.
Q: In which case could the CloseStatusUpdate_ClosePending stream be received more than once?
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.
Well based on my understanding one would be 'Manual Fee Bumping via lncli wallet bumpfee' but I am not really sure about this.
Change Description
This PR updates the lncli
closechannelandcloseallchannelscommands to includefee_per_vbyteandlocal_close_txin their JSON output when available.executeChannelCloseto send all close details (txid, fee, local flag) via a struct.Fixes #9831
Steps to Test
lncli closechannelwith a custom fee.closing_txid,fee_per_vbyte, andlocal_close_tx.lncli closeallchannelsand confirm the output for each channel includes the new fields.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.