-
Notifications
You must be signed in to change notification settings - Fork 2.2k
improve CloseChannel docs #9958
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?
improve CloseChannel docs #9958
Conversation
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Do I also need to update |
Yes, you need to generate the stubs with |
6380421
to
89cd7f2
Compare
@ziggie1984 please review. |
89cd7f2
to
e4caccb
Compare
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.
Nice addition!
lnrpc/lightning.proto
Outdated
// If `no_wait=true` (`wait=false`) when a coop close is attempted, then the | ||
// rpc call will not initially block while it awaits a closing txid to be | ||
// broadcasted to the mempool. Instead, a coop close will be initiated even | ||
// if there are HTLCs active in flight on the channel and a `close_instant` |
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.
and a close_instant
msg will sent over the stream ...
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.
please check my update
lnrpc/lightning.proto
Outdated
// resolved before that can happen. If a coop close is attempted with no in | ||
// flight HTLCs on the channel OR a force close is attempted, the `no_wait` | ||
// option is ignored. In summary, the `no_wait` option controls if the user |
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.
If a coop close is attempted with no inflight HTLCs on the channel the no_wait=true
parameter has still the effect that the stream sends the close_instant
msg immediately however that should not make much of a difference because LND should broadcast the closing tx without much time delay and therefore should send the close_pending
update right after.
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.
please check my update on this. I'm not sure if I got it right about force closing.
lnrpc/lightning.proto
Outdated
// wait for the in flight HTLCs to be resolved and then start the coop | ||
// closing process. Note: `lncli closechannel` always sets `no_wait=true` | ||
// and its `--block` option controls if `lncli` should wait for a | ||
// `close_pending` update and a `chan_close` update before returning or if |
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.
should wait for the chan_close update (we still on the lncli side wait for the close_pending, which signals when the tx is broadcasted to the mempool)
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.
please check my update
e4caccb
to
ae1a1d1
Compare
lnrpc/lightning.proto
Outdated
// will fail and abort the closing process because it can't wait for a | ||
// closing TXID to be broadcast to the mempool because it needs to | ||
// immediately tell the user that there are in flight HTLCs that need to be | ||
// resolved before that can happen. If a coop close is attempted with no in |
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.
then the rpc call will fail and abort the closing process because the caller wants to wait for the closing tx being broadcasted to the mempool but because of the inflight HTLCs this can be very long therefore LND will not allow initiating the coop-close with this setting.
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.
it is not that we can't we just decided to not do so because of the above reasons, does it make sense ?
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.
no, that is very convoluted.
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.
Ok I am trying to say, that we decided to not close the channel in that case because we do not want the user to wait until the HTLCs resolves because he signaled via "wait=true" he wants to wait for the closing information. But we definitely could, like technically this is not a problem, it is just we decided not to, to make sure the user does not have to wait forever
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.
so how exactly would you re-write the sentence?
ae1a1d1
to
4eab4b3
Compare
taking out of draft as I'd like to see some feedback from others. @saubyk , do you have any feedback? |
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.
LGTM
// initiated even if HTLCs are active on the channel. The channel will wait | ||
// until all HTLCs are resolved and then start the coop closing process. The | ||
// channel will be disabled in the meantime and will disallow any new HTLCs. | ||
// When force closing, `no_wait` has no effect. |
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 is an extremely verbose description and it doesn't seem to me that this is right place to get into the detailed scenarios here. How and where would this content would be made available to the user?
My first reaction is that it's not easy to follow and digest. And we may need to supplement the documentation somewhere else, rather than here.
This is a docs change only to improve the clarity of how to use
CloseChannel
.Fixes #9771
Fixes #9837