Expose closing tx in waiting_close_channels #8426
Conversation
|
Important Auto Review SkippedAuto reviews are limited to the following labels: llm-review. Please add one of these labels to enable auto reviews. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe latest update introduces enhancements and documentation improvements to the system. Key features include the addition of Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
8c340e5 to
e19923c
Compare
There was a problem hiding this comment.
Very nice addition! My main comment is to add a param in PendingChannelsRequest and optionally populate this field when requested since it's a relatively large field. In addition, we should update our itest to validate ClosingTxHex is created as expected - maybe we could just add a few lines in one of our tests. Maybe here
lnd/itest/lnd_channel_force_close_test.go
Line 144 in b9a8e2f
or here
Line 560 in b9a8e2f
76198f4 to
5662b3e
Compare
Thanks for the pointer to the itest, that's very helpful. I made the IncludeTx boolean true for all testcases using the PendingChannels rpc. I didn't think it could hurt. The alternative is to let every test pass a boolean or a request to that PendingChannels call instead. One has code changes in quite a few places, and the other 'affects' all the tests in those places. Let me know which one you prefer. |
|
@coderabbitai review |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between b9a8e2f and 5662b3e7b6f3908c0b37c1d71ebaff7829eb5402.Files ignored due to path filters (3)
lnrpc/lightning.pb.gois excluded by:!**/*.pb.golnrpc/lightning.pb.gw.gois excluded by:!**/*.pb.gw.golnrpc/lightning.swagger.jsonis excluded by:!**/*.json
Files selected for processing (5)
- docs/release-notes/release-notes-0.18.0.md (2 hunks)
- itest/lnd_channel_force_close_test.go (2 hunks)
- lnrpc/lightning.proto (2 hunks)
- lntest/rpc/lnd.go (1 hunks)
- rpcserver.go (5 hunks)
Additional comments: 11
docs/release-notes/release-notes-0.18.0.md (2)
- 182-184: The addition of the raw hex of the closing transaction in
waiting_close_channelsis correctly documented under theRPC Updatessection. This change enhances thePendingChannelsfunctionality, aligning with the PR's objective to improve channel closure processes.- 282-282: Adding Jesse de Wit to the contributors list is appropriate and acknowledges their contributions to this release.
lntest/rpc/lnd.go (1)
- 134-136: The modification to include the
IncludeTx: trueparameter in thePendingChannelsRequeststruct correctly implements the new functionality introduced by the PR. This change ensures that the raw closing transaction hex is included in thePendingChannelsresponse when requested.itest/lnd_channel_force_close_test.go (2)
- 5-5: The addition of the
encoding/hexpackage is appropriate for decoding hex strings, which is necessary for the new functionality being tested.- 145-162: The validation of the closing transaction hex in the
testCommitmentTransactionDeadlinefunction is correctly implemented. It checks for the presence of theClosingTxHex, decodes it, deserializes the transaction, and compares the transaction ID to the expected value. This ensures that theClosingTxHexprovided in thePendingChannelsResponsematches the actual closing transaction.lnrpc/lightning.proto (2)
- 2639-2641: The addition of the
include_txboolean field toPendingChannelsRequestallows callers to request the inclusion of the raw closing transaction hex in the response. This is a useful feature for nodes to access unconfirmed close transactions directly.- 2739-2742: The addition of the
closing_tx_hexstring field toWaitingCloseChannelwithinPendingChannelsResponseenables the inclusion of the raw closing transaction hex wheninclude_txis true in the request. This enhancement facilitates the use of transaction accelerators and improves the reliability of channel closure processes.rpcserver.go (4)
- 3721-3722: The method signature for
fetchWaitingCloseChannelshas been updated to include anincludeTxboolean parameter. This change aligns with the PR's objective to conditionally include the closing transaction details in the response based on this parameter.- 3737-3772: The
getClosingTxhelper function has been introduced to locate the closing transaction of a waiting close channel. It first attempts to find the force closing transaction and, if not found, tries to locate the cooperative closing transaction. The function handles errors appropriately, returningnilfor the transaction if it's not found, which is a sensible approach given the context that the remote party might have broadcasted the closing transaction.- 3832-3837: Error handling within the loop that processes waiting close channels is correctly implemented. If an error occurs while trying to find the closing transaction for a channel, the error is logged, and the function returns immediately with the error, ensuring that the error state is not ignored.
- 3921-3923: The call to
fetchWaitingCloseChannelscorrectly passes theIncludeTxfield from the incoming request. This ensures that the caller's preference regarding the inclusion of the closing transaction details is respected.
5662b3e to
c4a186f
Compare
|
I've also added the flag to lncli, which was a piece I hadn't included before. |
c4a186f to
10b7b71
Compare
|
Addressed the rabbit's actionable comment. |
rpcserver.go
Outdated
| @@ -3857,11 +3856,28 @@ func (r *rpcServer) fetchWaitingCloseChannels() (waitingCloseChannels, | |||
| Memo: string(waitingClose.Memo), | |||
| } | |||
|
|
|||
| var closingTxid string | |||
There was a problem hiding this comment.
nit: either use a single var ( ... ) block or combine with var closingTxid, closingTxHex string.
There was a problem hiding this comment.
Done var closingTxid, closingTxHex string
lnrpc/lightning.proto
Outdated
| @@ -2733,6 +2736,10 @@ message PendingChannelsResponse { | |||
|
|
|||
| // The transaction id of the closing transaction | |||
| string closing_txid = 4; | |||
|
|
|||
| // The raw hex encoded bytes of the closing transaction. Included if | |||
| // include_raw_tx is true. | |||
There was a problem hiding this comment.
nit: "Included if include_raw_tx in the request is true", since there is no direct relationship between request/response in the API docs.
10b7b71 to
3d4dd9a
Compare
3d4dd9a to
172e836
Compare
|
Addressed @guggero's comments and rebased. |
Change Description
This PR exposes the raw closing tx of the waiting_close_channels in the PendingChannels response.
There are cases when a local force close transaction of a static remote key channel drops out of the mempool due to insufficient fees. Currently, the only way to get hold of this transaction to rebroadcast it is to restart LND. However, restarting LND is infeasible for large nodes that want to maintain their uptime. Even if restarting LND is an option, if the tx has insufficient fee, the only way to capture it currently is by scavenging the logs. By adding the raw tx to waiting_close_channels, unconfirmed closes can be pushed through a tx accelerator for example to ensure timely confirmation.
Steps to Test
A pending cooperate close and local force close in the waiting close state includes the raw tx hex of the closing transaction.
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.
Summary by CodeRabbit
MinConfparameter to theWalletBalanceRPC call for enhanced balance querying.PendingChannelsRPC call now optionally returns the raw hex of the closing transaction for channels in the waiting close state.lnclicommands for better user guidance.