Skip to content
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.

Sign up for GitHub

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

Allow coop closing a channel with HTLCs on it via lncli #9491

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Allow coop closing a channel with HTLCs on it via lncli #9491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
506f5fe
multi: coop close with active HTLCs on the channel
ziggie1984 Feb 13, 2025
d0cff41
lnd: add max fee rate check to closechannel rpc
ziggie1984 Feb 9, 2025
505a599
htlcswitch: fix log output
ziggie1984 Feb 14, 2025
6b7cbac
docs: add release-notes
ziggie1984 Feb 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 35 additions & 5 deletions cmd/commands/commands.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1011,6 +1011,11 @@ var closeChannelCommand = cli.Command{
comparison is the end boundary of the fee negotiation, if not specified
it's always x3 of the starting value. Increasing this value increases
the chance of a successful negotiation.
Moreover if the channel has active HTLCs on it, the coop close will
wait until all HTLCs are resolved and will not allow any new HTLCs on
the channel. The channel will appear as disabled in the listchannels
output. The command will block in that case until the channel close tx
is broadcasted.

In the case of a cooperative closure, one can manually set the address
to deliver funds to upon closure. This is optional, and may only be used
Expand Down Expand Up @@ -1042,8 +1047,10 @@ var closeChannelCommand = cli.Command{
Usage: "attempt an uncooperative closure",
},
cli.BoolFlag{
Name: "block",
Usage: "block until the channel is closed",
Name: "block",
Usage: `block will wait for the channel to be closed,
"meaning that it will wait for the channel close tx to
get 1 confirmation.`,
ziggie1984 marked this conversation as resolved.
Show resolved Hide resolved
},
cli.Int64Flag{
Name: "conf_target",
Expand Down Expand Up @@ -1117,6 +1124,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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we cannot rename it because of backwards comp. issues. I think with a proper description we can keep it like this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I think resolving this with 2 distinct flags is the way to go, but I keep it low priority for now.

}

// After parsing the request, we'll spin up a goroutine that will
Expand Down Expand Up @@ -1154,7 +1164,9 @@ func closeChannel(ctx *cli.Context) error {
// executeChannelClose attempts to close the channel from a request. The closing
// transaction ID is sent through `txidChan` as soon as it is broadcasted to the
// network. The block boolean is used to determine if we should block until the
// closing transaction receives all of its required confirmations.
// closing transaction receives a confirmation of 1 block. The logging outputs
// are sent to stderr to avoid conflicts with the JSON output of the command
// and potential work flows which depend on a proper JSON output.
func executeChannelClose(ctxc context.Context, client lnrpc.LightningClient,
req *lnrpc.CloseChannelRequest, txidChan chan<- string, block bool) error {

Expand All @@ -1173,22 +1185,40 @@ func executeChannelClose(ctxc context.Context, client lnrpc.LightningClient,

switch update := resp.Update.(type) {
case *lnrpc.CloseStatusUpdate_CloseInstant:
if req.NoWait {
ziggie1984 marked this conversation as resolved.
Show resolved Hide resolved
return nil
fmt.Fprintln(os.Stderr, "Channel close successfully "+
"initiated")

pendingHtlcs := update.CloseInstant.NumPendingHtlcs
if pendingHtlcs > 0 {
fmt.Fprintf(os.Stderr, "Cooperative channel "+
"close waiting for %d HTLCs to be "+
"resolved before the close process "+
"can kick off\n", pendingHtlcs)
}

case *lnrpc.CloseStatusUpdate_ClosePending:
closingHash := update.ClosePending.Txid
txid, err := chainhash.NewHash(closingHash)
if err != nil {
return err
}

fmt.Fprintf(os.Stderr, "Channel close transaction "+
"broadcasted: %v\n", txid)

ziggie1984 marked this conversation as resolved.
Show resolved Hide resolved
txidChan <- txid.String()

if !block {
return nil
}

fmt.Fprintln(os.Stderr, "Waiting for channel close "+
"confirmation ...")

case *lnrpc.CloseStatusUpdate_ChanClose:
fmt.Fprintln(os.Stderr, "Channel close successfully "+
"confirmed")

return nil
}
}
Expand Down
9 changes: 9 additions & 0 deletions docs/release-notes/release-notes-0.19.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,10 @@
config is added `disable-backup-archive`, with default set to false, to
determine if previous channel backups should be archived or not.

* [The max fee rate](https://github.com/lightningnetwork/lnd/pull/9491) is now
respected when a coop close is initiated. Before the max fee rate would only
be effective for the remote party in the negotiation.

## Functional Enhancements
* [Add ability](https://github.com/lightningnetwork/lnd/pull/8998) to paginate
wallet transactions.
Expand Down Expand Up @@ -141,6 +145,11 @@
the misnomer of `chan_id` which was describing the short channel
id to `scid` to represent what it really is.

* [In the coop close](https://github.com/lightningnetwork/lnd/pull/9491) case
we always initiate the cooperative close flow even if there are HTLCs active
on the channel. LND will disable the channel for new HTLCs and kick of the
cooperative close flow automatically when the channel has no HTLCs left.

# Improvements
## Functional Updates

Expand Down
2 changes: 1 addition & 1 deletion htlcswitch/switch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1556,7 +1556,7 @@ out:
s.indexMtx.RUnlock()

peerPub := link.PeerPubKey()
log.Debugf("Requesting local channel close: peer=%v, "+
log.Debugf("Requesting local channel close: peer=%x, "+
"chan_id=%x", link.PeerPubKey(), chanID[:])

go s.cfg.LocalChannelClose(peerPub[:], req)
Expand Down
4 changes: 4 additions & 0 deletions itest/list_on_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,10 @@ var allTestCases = []*lntest.TestCase{
Name: "coop close with htlcs",
TestFunc: testCoopCloseWithHtlcs,
},
{
Name: "coop close exceeds max fee",
TestFunc: testCoopCloseExceedsMaxFee,
},
{
Name: "open channel locked balance",
TestFunc: testOpenChannelLockedBalance,
Expand Down
10 changes: 6 additions & 4 deletions itest/lnd_channel_backup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,14 +283,16 @@ func (c *chanRestoreScenario) testScenario(ht *lntest.HarnessTest,

// We don't get an error directly but only when reading the first
// message of the stream.
err := ht.CloseChannelAssertErr(
dave, &lnrpc.ChannelPoint{
req := &lnrpc.CloseChannelRequest{
ChannelPoint: &lnrpc.ChannelPoint{
FundingTxid: &lnrpc.ChannelPoint_FundingTxidStr{
FundingTxidStr: chanPointParts[0],
},
OutputIndex: uint32(chanPointIndex),
}, true,
)
},
Force: true,
}
err := ht.CloseChannelAssertErr(dave, req)
require.Contains(ht, err.Error(), "cannot close channel with state: ")
require.Contains(ht, err.Error(), "ChanStatusRestored")

Expand Down
84 changes: 80 additions & 4 deletions itest/lnd_coop_close_with_htlcs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/lightningnetwork/lnd/lntest"
"github.com/lightningnetwork/lnd/lntest/wait"
"github.com/lightningnetwork/lnd/lntypes"
"github.com/lightningnetwork/lnd/lnwallet/chainfee"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -93,14 +94,17 @@ func coopCloseWithHTLCs(ht *lntest.HarnessTest) {
// closure is set up. Let's settle the invoice.
alice.RPC.SettleInvoice(preimage[:])

// Pull the instant update off the wire to clear the path for the
// close pending update.
_, err := closeClient.Recv()
// Pull the instant update off the wire and make sure the number of
// pending HTLCs is as expected.
update, err := closeClient.Recv()
require.NoError(ht, err)
closeInstant := update.GetCloseInstant()
require.NotNil(ht, closeInstant)
require.Equal(ht, closeInstant.NumPendingHtlcs, int32(1))

// Wait for the next channel closure update. Now that we have settled
// the only HTLC this should be imminent.
update, err := closeClient.Recv()
update, err = closeClient.Recv()
require.NoError(ht, err)

// This next update should be a GetClosePending as it should be the
Expand Down Expand Up @@ -243,3 +247,75 @@ func coopCloseWithHTLCsWithRestart(ht *lntest.HarnessTest) {
// Show that the address used is the one she requested.
require.Equal(ht, outputDetail.Address, newAddr.Address)
}

// testCoopCloseExceedsMaxFee tests that we fail the coop close process if
// the max fee rate exceeds the expected fee rate for the initial closing fee
// proposal.
func testCoopCloseExceedsMaxFee(ht *lntest.HarnessTest) {
const chanAmt = 1000000

// Create a channel Alice->Bob.
chanPoints, nodes := ht.CreateSimpleNetwork(
[][]string{nil, nil}, lntest.OpenChannelParams{
Amt: chanAmt,
},
)

alice, _ := nodes[0], nodes[1]
chanPoint := chanPoints[0]

// Set the fee estimate for one block to 10 sat/vbyte.
ht.SetFeeEstimateWithConf(chainfee.SatPerVByte(10).FeePerKWeight(), 1)

// Have alice attempt to close the channel but we the expected fee rate
// exceeds the max fee rate so we fail the closing process.
req := &lnrpc.CloseChannelRequest{
ChannelPoint: chanPoint,
MaxFeePerVbyte: 5,
NoWait: true,
TargetConf: 1,
}
err := ht.CloseChannelAssertErr(alice, req)
require.Contains(ht, err.Error(), "max_fee_per_vbyte (1250 sat/kw) is "+
"less than the required fee rate (2500 sat/kw)")

// Now close the channel with a appropriate max fee rate.
closeClient := alice.RPC.CloseChannel(&lnrpc.CloseChannelRequest{
ChannelPoint: chanPoint,
NoWait: true,
TargetConf: 1,
MaxFeePerVbyte: 10,
})

// Pull the instant update off the wire to clear the path for the
// close pending update. Moreover confirm that there are no pending
// HTLCs on the channel.
update, err := closeClient.Recv()
require.NoError(ht, err)
closeInstant := update.GetCloseInstant()
require.NotNil(ht, closeInstant)
require.Equal(ht, closeInstant.NumPendingHtlcs, int32(0))

// Wait for the channel to be closed.
update, err = closeClient.Recv()
require.NoError(ht, err)

// This next update should be a GetClosePending as it should be the
// negotiation of the coop close tx.
closePending := update.GetClosePending()
require.NotNil(ht, closePending)

// Convert the txid we get from the PendingUpdate to a Hash so we can
// wait for it to be mined.
var closeTxid chainhash.Hash
require.NoError(
ht, closeTxid.SetBytes(closePending.Txid),
"invalid closing txid",
)

// Wait for the close tx to be in the Mempool.
ht.AssertTxInMempool(closeTxid)

// Wait for it to get mined and finish tearing down.
ht.AssertStreamChannelCoopClosed(alice, chanPoint, false, closeClient)
}
10 changes: 8 additions & 2 deletions itest/lnd_funding_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -674,7 +674,10 @@ func runExternalFundingScriptEnforced(ht *lntest.HarnessTest) {
// First, we'll try to close the channel as Carol, the initiator. This
// should fail as a frozen channel only allows the responder to
// initiate a channel close.
err := ht.CloseChannelAssertErr(carol, chanPoint2, false)
req := &lnrpc.CloseChannelRequest{
ChannelPoint: chanPoint2,
}
err := ht.CloseChannelAssertErr(carol, req)
require.Contains(ht, err.Error(), "cannot co-op close frozen channel")

// Before Dave closes the channel, he needs to check the invoice is
Expand Down Expand Up @@ -831,7 +834,10 @@ func runExternalFundingTaproot(ht *lntest.HarnessTest) {
// First, we'll try to close the channel as Carol, the initiator. This
// should fail as a frozen channel only allows the responder to
// initiate a channel close.
err := ht.CloseChannelAssertErr(carol, chanPoint2, false)
req := &lnrpc.CloseChannelRequest{
ChannelPoint: chanPoint2,
}
err := ht.CloseChannelAssertErr(carol, req)
require.Contains(ht, err.Error(), "cannot co-op close frozen channel")

// Before Dave closes the channel, he needs to check the invoice is
Expand Down
Loading
Loading