Skip to content
Open
Changes from all commits
Commits
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
72 changes: 48 additions & 24 deletions cmd/commands/commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -1099,6 +1099,13 @@ var closeChannelCommand = cli.Command{
Action: actionDecorator(closeChannel),
}

// CloseInfo contains information about a channel close transaction.
type CloseInfo struct {
ClosingTxid string `json:"closing_txid"`
FeePerVbyte int64 `json:"fee_per_vbyte,omitempty"`
LocalCloseTx bool `json:"local_close_tx,omitempty"`
}

func closeChannel(ctx *cli.Context) error {
ctxc := getContext()
client, cleanUp := getClient(ctx)
Expand Down Expand Up @@ -1138,51 +1145,52 @@ func closeChannel(ctx *cli.Context) error {
}

// After parsing the request, we'll spin up a goroutine that will
// retrieve the closing transaction ID when attempting to close the
// retrieve the closing transaction details when attempting to close the
// channel. We do this to because `executeChannelClose` can block, so we
// would like to present the closing transaction ID to the user as soon
// would like to present the closing transaction details to the user as soon
// as it is broadcasted.
var wg sync.WaitGroup
txidChan := make(chan string, 1)
closeInfoChan := make(chan CloseInfo, 1)

wg.Add(1)
go func() {
defer wg.Done()

printJSON(struct {
ClosingTxid string `json:"closing_txid"`
}{
ClosingTxid: <-txidChan,
})
closeInfo := <-closeInfoChan
printJSON(closeInfo)
Comment on lines +1159 to +1160

Choose a reason for hiding this comment

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

high

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)
        }

}()

err = executeChannelClose(ctxc, client, req, txidChan, ctx.Bool("block"))
err = executeChannelClose(ctxc, client, req, closeInfoChan, ctx.Bool("block"))
if err != nil {
return err
}

// In the case that the user did not provide the `block` flag, then we
// need to wait for the goroutine to be done to prevent it from being
// destroyed when exiting before printing the closing transaction ID.
// destroyed when exiting before printing the closing transaction details.
wg.Wait()

return nil
}

// 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
// transaction information is sent through `closeInfoChan` 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 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 {
req *lnrpc.CloseChannelRequest, closeInfoChan chan<- CloseInfo, block bool) error {

Choose a reason for hiding this comment

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

critical

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)

stream, err := client.CloseChannel(ctxc, req)
if err != nil {
return err
}

// Track if we've already sent close info to prevent duplicate sends
// when ClosePending is received multiple times (RBF, rebroadcast, etc.)
var sentCloseInfo bool

for {
resp, err := stream.Recv()
if err == io.EOF {
Expand Down Expand Up @@ -1211,10 +1219,21 @@ func executeChannelClose(ctxc context.Context, client lnrpc.LightningClient,
return err
}

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

txidChan <- txid.String()
feeRate := update.ClosePending.FeePerVbyte
isLocalClose := update.ClosePending.LocalCloseTx

// Only send close info on the first ClosePending event.
// Subsequent events (from RBF, rebroadcast, etc.) are
// ignored to prevent duplicate output.
if !sentCloseInfo {
closeInfo := CloseInfo{
ClosingTxid: txid.String(),
FeePerVbyte: feeRate,
LocalCloseTx: isLocalClose,
}
closeInfoChan <- closeInfo
sentCloseInfo = true
}
Comment on lines +1228 to +1236
Copy link
Contributor

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?

Copy link
Author

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.


if !block {
return nil
Expand Down Expand Up @@ -1393,13 +1412,15 @@ func closeAllChannels(ctx *cli.Context) error {
}

// result defines the result of closing a channel. The closing
// transaction ID is populated if a channel is successfully closed.
// transaction information is populated if a channel is successfully closed.
// Otherwise, the error that prevented closing the channel is populated.
type result struct {
RemotePubKey string `json:"remote_pub_key"`
ChannelPoint string `json:"channel_point"`
ClosingTxid string `json:"closing_txid"`
FailErr string `json:"error"`
RemotePubKey string `json:"remote_pub_key"`
ChannelPoint string `json:"channel_point"`
ClosingTxid string `json:"closing_txid"`
FeePerVbyte int64 `json:"fee_per_vbyte,omitempty"`
LocalCloseTx bool `json:"local_close_tx,omitempty"`
FailErr string `json:"error"`
}

// Launch each channel closure in a goroutine in order to execute them
Expand Down Expand Up @@ -1442,15 +1463,18 @@ func closeAllChannels(ctx *cli.Context) error {
SatPerVbyte: ctx.Uint64(feeRateFlag),
}

txidChan := make(chan string, 1)
err = executeChannelClose(ctxc, client, req, txidChan, false)
closeInfoChan := make(chan CloseInfo, 1)
err = executeChannelClose(ctxc, client, req, closeInfoChan, false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

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 err != nil {
res.FailErr = fmt.Sprintf("unable to close "+
"channel: %v", err)
return
}

res.ClosingTxid = <-txidChan
closeInfo := <-closeInfoChan
res.ClosingTxid = closeInfo.ClosingTxid
res.FeePerVbyte = closeInfo.FeePerVbyte
res.LocalCloseTx = closeInfo.LocalCloseTx
Comment on lines +1474 to +1477

Choose a reason for hiding this comment

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

high

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.LocalCloseTx

}(channel)
}

Expand Down