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 |
ZZiigguurraatt
force-pushed
the
closechannel_docs
branch
2 times, most recently
from
6380421 to
89cd7f2
Compare
|
@ziggie1984 please review. |
ZZiigguurraatt
force-pushed
the
closechannel_docs
branch
from
89cd7f2 to
e4caccb
Compare
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.
and a close_instant msg will sent over the stream ...
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.
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.
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.
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)
ZZiigguurraatt
force-pushed
the
closechannel_docs
branch
from
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.
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.
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.
no, that is very convoluted.
There was a problem hiding this comment.
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.
so how exactly would you re-write the sentence?
ZZiigguurraatt
force-pushed
the
closechannel_docs
branch
from
ae1a1d1 to
4eab4b3
Compare
|
taking out of draft as I'd like to see some feedback from others. @saubyk , do you have any feedback? |
| // 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.
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.
|
I think we should deprecate this field in the first place and default to no_wait=true if everybody agrees. I don't see a usecase for |
That is not a bad idea. |
|
What about breaking b0c0cde out into a separate PR so that can at least be added and then we can get more feedback from the team on #9958 (comment) ? |
| @@ -878,7 +879,7 @@ | |||
| }, | |||
| { | |||
| "name": "no_wait", | |||
| "description": "If true, then the rpc call will not block while it awaits a closing txid\nto be broadcasted to the mempool. To obtain the closing tx one has to\nlisten to the stream for the particular updates. Moreover if a coop close\nis specified and this flag is set to true the coop closing flow will be\ninitiated even if HTLCs are active on the channel. The channel will wait\nuntil all HTLCs are resolved and then start the coop closing process. The\nchannel will be disabled in the meantime and will disallow any new HTLCs.", | |||
| "description": "When force closing, `no_wait` has no effect.\nIf `no_wait=true` (`wait=false`) when a coop close is attempted, then the\nrpc call will not initially block while it awaits a closing txid to be\nbroadcasted to the mempool. Instead, a coop close will be initiated even\nif there are HTLCs active inflight on the channel and a `close_instant`\nmessage will be immediately returned over the stream which contains\n`num_pending_htlcs` if there are inflight HTLCs that still need to be\nresolved before a coop close can be initiated. After the `close_instant`\nis received, then the channel will be disabled to disallow any new HTLCs\nand the rpc will block while it waits for the exsting inflight HTLCs to\nbe resolved and then start the coop closing process. To obtain the\nclosing TXID one has to listen to the stream for a `close_pending`\nupdate. If the client disconnects the stream before this is received, LND\nwill continue to wait for the inflight HTLCs to be resolved and then\nstart the coop closing process. Note: `lncli closechannel` always sets\n`no_wait=true` and its `--block` option controls if `lncli` should wait\nfor a `chan_close` update (indicating the channel close has been mined)\nbefore returning or if it should disconnect the stream and return\nimmediately after `close_pending` is received when the TX is broadcast to\nthe mempool. If `no_wait=false` (`wait=true`) when a coop close is\nattempted and there are inflight HTLCs on the channel, then the rpc call\nwill fail and abort the closing process because it can't wait for a\nclosing TXID to be broadcast to the mempool because it needs to\nimmediately tell the user that there are inflight HTLCs that need to be\nresolved before that can happen. If a coop close is attempted with no\ninflight HTLCs on the channel with `no_wait=true`, it will still result\nin an immediate `close_instant` message being returned on the stream but\n`num_pending_htlcs` will be 0 and *nearly* immediately after that the\n`close_pending` message will be returned on the stream. If a coop close\nis attempted with no inflight HTLCs on the channel with `no_wait=false`,\nthe `close_instant` message will be skipped and *nearly* immediately the\n`close_pending` message will be returned on the stream. In summary, the\n`no_wait` option controls if the user is willing to wait for the rpc call\nto send an initial immediate response message back indicating if there\nare inflight HTLCs on the channel and a coop close is attempted. It then\nindirectly controls if the rpc call later waits for inflight HTLCs to be\nresolved before attempting a coop close or if it will instantly fail.\nThe default value is false.", | |||
There was a problem hiding this comment.
Please consider removing the non-existent wait parameter from the document, as I believe it makes the documentation confusing. I actually went looking for the wait param in the API after reading this. 😄
Here's a suggested text to reduce the verbosity and improved readability:
The no_wait parameter governs the RPC call's behavior during a cooperative channel close but has no effect on a force close.
Setting no_wait=true prevents the RPC call from blocking, even if the channel has active HTLCs. Instead, it immediately returns a close_instant message indicating the number of pending HTLCs, disables the channel to block new payments, and then waits for the existing HTLCs to be resolved before initiating the cooperative close. To obtain the closing transaction ID, the client must listen to the stream for a close_pending update. The lncli closechannel command always uses no_wait=true, and its --block option separately controls whether it waits for the closing transaction to be broadcast (close_pending) or fully mined (chan_close).
Conversely, if no_wait=false and active HTLCs exist, the RPC call will fail immediately, as it cannot simultaneously wait for HTLC resolution and inform the user of their presence. When no HTLCs are active, setting no_wait=true results in a close_instant message (with zero pending HTLCs) followed quickly by the close_pending update, whereas no_wait=false skips the close_instant message and proceeds directly to the close_pending update.
There was a problem hiding this comment.
agree it's confusing - I went to look it up too😅
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR improves the documentation for CloseChannel, especially around the new RBF cooperative close feature and the no_wait parameter. The changes are spread across several files, including the proto definitions and generated code.
I have one suggestion to improve the readability of the documentation for the no_wait parameter in lnrpc/lightning.proto. The new text is very detailed but also very dense, which can make it hard to understand the complex behavior. I've proposed a restructured version that should be easier to parse.
| // When force closing, `no_wait` has no effect. | ||
| // 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 inflight on the channel and a `close_instant` | ||
| // message will be immediately returned over the stream which contains | ||
| // `num_pending_htlcs` if there are inflight HTLCs that still need to be | ||
| // resolved before a coop close can be initiated. After the `close_instant` | ||
| // is received, then the channel will be disabled to disallow any new HTLCs | ||
| // and the rpc will block while it waits for the exsting inflight HTLCs to | ||
| // be resolved and then start the coop closing process. To obtain the | ||
| // closing TXID one has to listen to the stream for a `close_pending` | ||
| // update. If the client disconnects the stream before this is received, LND | ||
| // will continue to wait for the inflight 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 `chan_close` update (indicating the channel close has been mined) | ||
| // before returning or if it should disconnect the stream and return | ||
| // immediately after `close_pending` is received when the TX is broadcast to | ||
| // the mempool. If `no_wait=false` (`wait=true`) when a coop close is | ||
| // attempted and there are inflight HTLCs on the channel, then the rpc call | ||
| // 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 inflight HTLCs that need to be | ||
| // resolved before that can happen. If a coop close is attempted with no | ||
| // inflight HTLCs on the channel with `no_wait=true`, it will still result | ||
| // in an immediate `close_instant` message being returned on the stream but | ||
| // `num_pending_htlcs` will be 0 and *nearly* immediately after that the | ||
| // `close_pending` message will be returned on the stream. If a coop close | ||
| // is attempted with no inflight HTLCs on the channel with `no_wait=false`, | ||
| // the `close_instant` message will be skipped and *nearly* immediately the | ||
| // `close_pending` message will be returned on the stream. In summary, the | ||
| // `no_wait` option controls if the user is willing to wait for the rpc call | ||
| // to send an initial immediate response message back indicating if there | ||
| // are inflight HTLCs on the channel and a coop close is attempted. It then | ||
| // indirectly controls if the rpc call later waits for inflight HTLCs to be | ||
| // resolved before attempting a coop close or if it will instantly fail. | ||
| // The default value is false. |
There was a problem hiding this comment.
This documentation block is very dense and hard to read. It describes complex behavior, so improving its structure would greatly enhance clarity for developers.
I've restructured it with paragraphs and bullet points to explain the different scenarios for no_wait. I also fixed a typo (exsting -> existing).
// When force closing, `no_wait` has no effect.
//
// For cooperative closures, this option controls whether the client is willing
// to wait for an initial response from the server about inflight HTLCs. This
// indirectly determines if the RPC call will wait for HTLCs to be resolved or
// fail immediately.
//
// The behavior is as follows:
//
// If `no_wait=true` (`wait=false`):
// A `close_instant` message is immediately returned, indicating the number of
// pending HTLCs. The channel is then disabled, and the RPC call blocks until
// all existing inflight HTLCs are resolved before proceeding with the
// cooperative close. The final closing transaction ID is sent in a
// `close_pending` update. If the client disconnects, LND will continue the
// process in the background.
//
// If `no_wait=false` (`wait=true`):
// If there are any inflight HTLCs, the RPC call will fail immediately, as it
// cannot wait for a closing transaction while HTLCs need resolution.
//
// Behavior with no inflight HTLCs:
// - `no_wait=true`: An immediate `close_instant` message is returned with
// `num_pending_htlcs=0`, followed shortly by a `close_pending` message.
// - `no_wait=false`: The `close_instant` message is skipped, and a
// `close_pending` message is returned shortly after.
//
// Note on `lncli`: `lncli closechannel` always sets `no_wait=true`. Its
// `--block` option controls whether `lncli` waits for the channel close to be
// mined (`chan_close` update) or returns after the transaction is broadcast
// (`close_pending` update).
//
// The default value is false.
There was a problem hiding this comment.
@gemini-code-assist I think the NoWait is a bit confusing, what would be a good alternative for it? sth like FailOnInflightHtlcs or AutoResolveHtlcs?
There was a problem hiding this comment.
That's an excellent question, yyforyongyu! The name no_wait for a boolean flag can indeed be confusing, as it's a negative assertion. Generally, it's good practice to name boolean flags positively, so true means the action described by the name will occur.
Let's consider your suggestions:
-
FailOnInflightHtlcs: If this flag weretrueby default, it would align with the currentno_wait=falsebehavior (failing if HTLCs are present). However, if we want to keep the current default value (false) and simply rename the flag, this would implyFailOnInflightHtlcs: falsemeans it won't fail, which is still a double negative. -
AutoResolveHtlcs: This name is much clearer. IfAutoResolveHtlcsistrue, it means the system will automatically resolve inflight HTLCs before closing (which corresponds to the currentno_wait=truebehavior). IfAutoResolveHtlcsisfalse(which is the current default forno_wait), it means it won't auto-resolve, and thus will fail if HTLCs are present. This aligns perfectly with the existing default value and provides a positive, descriptive name.
Therefore, AutoResolveHtlcs seems like a strong candidate for a clearer name, as it directly describes the behavior when the flag is set to true and maintains consistency with the current default false value of no_wait.
It's important to note that renaming a field in a .proto file (like no_wait to auto_resolve_htlcs) would be a breaking change for existing API clients, as the field name changes in the generated code and the wire format. This would typically require careful versioning or migration strategies if this RPC is already in use by external consumers.
There was a problem hiding this comment.
@gemini-code-assist can you come up with a better name pls?
There was a problem hiding this comment.
That's a great follow-up question, yyforyongyu! It's important to find the most intuitive name for such a critical parameter.
Building on our previous discussion about positive boolean naming, let's consider another strong candidate:
AllowPendingHtlcs:- If
AllowPendingHtlcsistrue(corresponding tono_wait=true), it means the cooperative close process is allowed to begin even if there are HTLCs currently pending on the channel. The RPC call would return immediately with aclose_instantmessage, and the HTLCs would be resolved asynchronously via the stream. - If
AllowPendingHtlcsisfalse(corresponding tono_wait=false, the current default), it means the RPC call will not allow pending HTLCs and will fail immediately if any are present.
- If
Comparing AllowPendingHtlcs with AutoResolveHtlcs:
AutoResolveHtlcsemphasizes the action taken by the system (automatically resolving HTLCs in the background).AllowPendingHtlcsemphasizes the condition under which the RPC proceeds (permitting pending HTLCs at the start of the close attempt).
Both are good, positive names that clearly describe the behavior when set to true. The choice between them might come down to whether the primary emphasis should be on the system's action (AutoResolveHtlcs) or the state it tolerates (AllowPendingHtlcs). Given the original description's focus on initiating a close "even if there are HTLCs active inflight," AllowPendingHtlcs might slightly better capture that specific nuance.
As mentioned before, any renaming of this field in the .proto file would constitute a breaking change for existing API clients, requiring careful consideration for versioning and migration.
|
I think we should take gemini's suggestion into consideration and update the docs a bit. We may also create a followup PR to deprecate the flag |
|
@ZZiigguurraatt, remember to re-request review from reviewers when ready |
This is a docs change only to improve the clarity of how to use
CloseChannel.Fixes #9771
Fixes #9837