Allow coop closing a channel with HTLCs on it via lncli #9491
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 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
ziggie1984
force-pushed
the
closechannel-rpc
branch
from
a978a6c to
6ce096d
Compare
ziggie1984
force-pushed
the
closechannel-rpc
branch
2 times, most recently
from
6f66e70 to
1e9cbc8
Compare
ziggie1984
force-pushed
the
closechannel-rpc
branch
2 times, most recently
from
67122b4 to
9e289f7
Compare
ziggie1984
force-pushed
the
closechannel-rpc
branch
2 times, most recently
from
7f47e31 to
83bc0b9
Compare
There was a problem hiding this comment.
Reviewed 3 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 4 of 5 files reviewed, 4 unresolved discussions (waiting on @starius)
itest/lnd_coop_close_with_htlcs_test.go line 263 at r2 (raw file):
// Wait for Bob to understand that the channel is ready to use. ht.AssertChannelInGraph(bob, chanPoint)
use ht.CreateSimpleNetwork instead?
itest/lnd_coop_close_with_htlcs_test.go line 270 at r2 (raw file):
// Have alice attempt to close the channel but we the expected fee rate // exceeds the max fee rate so we fail the closing process. closeClient := alice.RPC.CloseChannel(&lnrpc.CloseChannelRequest{
nit: can use ht.CloseChannelAssertErr instead
cmd/commands/commands.go line 1036 at r2 (raw file):
Name: "block", Usage: `block will wait for the channel to be closed, "meaning that it will wait for the channel tx to get 1
should be close tx or channel close tx, not sure what channel tx is
rpcserver.go line 2876 at r2 (raw file):
).FeePerKWeight() if maxFee != 0 && maxFee < feeRate {
I'm not sure about this approach, either should it abort the flow, or cap the value. Since right now, the chan closer will always use the maxFee instead when it's set.
| @@ -1101,6 +1108,9 @@ func closeChannel(ctx *cli.Context) error { | |||
| SatPerVbyte: ctx.Uint64(feeRateFlag), | |||
| DeliveryAddress: ctx.String("delivery_addr"), | |||
| MaxFeePerVbyte: ctx.Uint64("max_fee_rate"), | |||
| // This makes sure that a coop close will also be executed if | |||
| // active HTLCs are present on the channel. | |||
| NoWait: true, | |||
There was a problem hiding this comment.
The effect of waiting for HTLCs is documented in rpcserver.go, but not in the schema.
// If true, then the rpc call will not block while it awaits a closing txid.
// Consequently this RPC call will not return a closing txid if this value
// is set.
bool no_wait = 8;// If the user hasn't specified NoWait, then before we attempt
// to close the channel we ensure there are no active HTLCs on
// the link.
if !in.NoWait && len(channel.ActiveHtlcs()) != 0 {I propose to add this important detail to the schema as well and maybe rename the flag to coop_with_htlcs or wait_for_htlcs or something to make it clear that it affects the closing transaction (making it coop instead of force close), not only for how long it executes.
Looking at the current name and the description of NoWait I thought originally, that it is a flag similar to --block before I read the code of rpcserver.go.
There was a problem hiding this comment.
we cannot rename it because of backwards comp. issues. I think with a proper description we can keep it like this.
There was a problem hiding this comment.
The description is great! Thanks!
Is it possible to decouple it into two flags? So NoWait only affects RPC behavior (whether or now to wait in RPC for transaction being broadcasted and return its txid) and a new flag would define what to do if there is a pending HTLC (force-close or wait-and-coop-close).
In that case NoWait can be removed, since it can be implemented on the client side similar to --block. The former will wait on gRPC stream for a transaction being broadcast, while the later waits on gRPC stream for confirmation of the transaction.
What do you think?
There was a problem hiding this comment.
yeah I think resolving this with 2 distinct flags is the way to go, but I keep it low priority for now.
ziggie1984
force-pushed
the
closechannel-rpc
branch
from
83bc0b9 to
ad10e21
Compare
There was a problem hiding this comment.
Reviewable status: 1 of 12 files reviewed, 12 unresolved discussions (waiting on @starius and @yyforyongyu)
rpcserver.go line 2876 at r2 (raw file):
Previously, yyforyongyu (Yong) wrote…
I'm not sure about this approach, either should it abort the flow, or cap the value. Since right now, the chan closer will always use the
maxFeeinstead when it's set.
Could you elaborate, we only want to abort if the user specified a feerate which is lower than the estimate for the first intial fee rate ?
cmd/commands/commands.go line 1036 at r2 (raw file):
Previously, yyforyongyu (Yong) wrote…
should be close tx or channel close tx, not sure what channel tx is
👌
itest/lnd_coop_close_with_htlcs_test.go line 270 at r2 (raw file):
Previously, yyforyongyu (Yong) wrote…
nit: can use
ht.CloseChannelAssertErrinstead
ok but had to change the func signature for that, now I pass in the req instead of the single arguments.
There was a problem hiding this comment.
Looks like the CI failed
Reviewed 9 of 11 files at r3.
Reviewable status: 10 of 12 files reviewed, 11 unresolved discussions (waiting on @starius and @ziggie1984)
rpcserver.go line 2876 at r2 (raw file):
Previously, ziggie1984 (ziggieXXX) wrote…
Could you elaborate, we only want to abort if the user specified a feerate which is lower than the estimate for the first intial fee rate ?
yeah nvm, was thinking we should cap it since it could cause the coop negotiation to fail, but maybe it's easier to just abort it here.
rpcserver.go line 2899 at r3 (raw file):
if in.NoWait { rpcsLog.Trace("[closechannel] sending instant update") activeHtlcs := len(channel.ActiveHtlcs())
I think this value is already obtained above.
| @@ -1101,6 +1108,9 @@ func closeChannel(ctx *cli.Context) error { | |||
| SatPerVbyte: ctx.Uint64(feeRateFlag), | |||
| DeliveryAddress: ctx.String("delivery_addr"), | |||
| MaxFeePerVbyte: ctx.Uint64("max_fee_rate"), | |||
| // This makes sure that a coop close will also be executed if | |||
| // active HTLCs are present on the channel. | |||
| NoWait: true, | |||
There was a problem hiding this comment.
The description is great! Thanks!
Is it possible to decouple it into two flags? So NoWait only affects RPC behavior (whether or now to wait in RPC for transaction being broadcasted and return its txid) and a new flag would define what to do if there is a pending HTLC (force-close or wait-and-coop-close).
In that case NoWait can be removed, since it can be implemented on the client side similar to --block. The former will wait on gRPC stream for a transaction being broadcast, while the later waits on gRPC stream for confirmation of the transaction.
What do you think?
ziggie1984
force-pushed
the
closechannel-rpc
branch
from
ad10e21 to
1400bbe
Compare
There was a problem hiding this comment.
Reviewable status: 6 of 12 files reviewed, 11 unresolved discussions (waiting on @starius and @yyforyongyu)
rpcserver.go line 2899 at r3 (raw file):
Previously, yyforyongyu (Yong) wrote…
I think this value is already obtained above.
Done.
ziggie1984
force-pushed
the
closechannel-rpc
branch
from
1400bbe to
1d5f7bc
Compare
ziggie1984
force-pushed
the
closechannel-rpc
branch
from
1d5f7bc to
559b4c6
Compare
For the lncli cmd we now always initiate the coop close even if there are active HTLCs on the channel. In case HTLCs are on the channel and the coop close is initiated LND handles the closing flow in the background and the lncli cmd will block until the transaction is broadcasted to the mempool. In the background LND disallows any new HTLCs and waits until all HTLCs are resolved before kicking of the negotiation process. Moreover if active HTLCs are present and the no_wait param is not set the error msg is now highlightning it so the user can react accordingly.
ziggie1984
force-pushed
the
closechannel-rpc
branch
from
559b4c6 to
e77c362
Compare
ziggie1984
force-pushed
the
closechannel-rpc
branch
from
e77c362 to
6b7cbac
Compare
This PR adds the possibility to coop close a channel which has still HTLCS on it. Moreover it makes sure we do not exceed the max-fee-rate we specify.
This change is