bumpforceclosefee rpc #8843
bumpforceclosefee rpc #8843
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 WalkthroughThe recent update enhances the fee-bumping mechanism for force close operations in Lightning Network channels, particularly when no HTLCs are involved. It introduces the Changes
Assessment against linked issues
Possibly related issues
Poem
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 Configuration File (
|
ziggie1984
force-pushed
the
bumpforceclosefee-rpc
branch
3 times, most recently
from
3946e44 to
219764f
Compare
|
@coderabbitai review |
Actions performedReview triggered.
|
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... CommitsFiles that changed from the base of the PR and between 68494fd and 219764f418dac9f25fd0f64a50559d5c4fa652cd. Files ignored due to path filters (5)
Files selected for processing (9)
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 (
|
ziggie1984
added
rpc
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (3)
cmd/lncli/walletrpc_active.go (1)
Line range hint
528-528: Remove unused functiongetWaitingCloseCommitments.The function
getWaitingCloseCommitmentsis marked as unused by static analysis tools. If this function is indeed not required, consider removing it to clean up the codebase.- func getWaitingCloseCommitments(ctxc context.Context, - client lnrpc.LightningClient, channelPoint string) ( - *lnrpc.PendingChannelsResponse_Commitments, error) { - req := &lnrpc.PendingChannelsRequest{} - resp, err := client.PendingChannels(ctxc, req) - if err != nil { - return nil, err - } - for _, channel := range resp.WaitingCloseChannels { - if channel.Channel.ChannelPoint == channelPoint { - return channel.Commitments, nil - } - } - return nil, errors.New("channel not found") - }lnrpc/walletrpc/walletkit_server.go (1)
Line range hint
2406-2407: Potential security risk detected by Gitleaks.The lines 2406-2407, 2412-2413, and 2416-2417 have been flagged by Gitleaks for containing a Generic API Key. This could potentially expose access to various services and sensitive operations. It's crucial to investigate these findings, remove any hardcoded secrets, and possibly use a secure vault or environment variables.
Also applies to: 2412-2413, 2416-2417
contractcourt/channel_arbitrator.go (1)
Line range hint
410-435: ### Review: Interaction with Blockchain State insweepAnchors
ThesweepAnchorsfunction (lines 410-435) interacts directly with the blockchain to manage anchor outputs. It's crucial that this interaction is robust against blockchain reorgs and other anomalies. The current implementation assumes a stable chain state, which might not always be the case. Consider adding reorg-safe mechanisms or checks to ensure that the anchor handling remains consistent even when the blockchain state changes unexpectedly.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Commits
Files that changed from the base of the PR and between 68494fd and 219764f418dac9f25fd0f64a50559d5c4fa652cd.
Files ignored due to path filters (5)
lnrpc/walletrpc/walletkit.pb.gois excluded by!**/*.pb.go,!**/*.pb.golnrpc/walletrpc/walletkit.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.golnrpc/walletrpc/walletkit.swagger.jsonis excluded by!**/*.jsonlnrpc/walletrpc/walletkit.yamlis excluded by!**/*.yamllnrpc/walletrpc/walletkit_grpc.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (9)
- cmd/lncli/walletrpc_active.go (1 hunks)
- contractcourt/channel_arbitrator.go (1 hunks)
- docs/release-notes/release-notes-0.18.1.md (1 hunks)
- lnrpc/walletrpc/config_active.go (2 hunks)
- lnrpc/walletrpc/walletkit.pb.json.go (1 hunks)
- lnrpc/walletrpc/walletkit.proto (1 hunks)
- lnrpc/walletrpc/walletkit_server.go (3 hunks)
- lnwallet/wallet.go (1 hunks)
- subrpcserver_config.go (1 hunks)
Additional context used
LanguageTool
docs/release-notes/release-notes-0.18.1.md
[uncategorized] ~28-~28: A comma may be missing after the conjunctive/linking adverb ‘Moreover’. (SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
Context: ...ment when the channel was force closed. Moreover a new walletrpc endpoint `BumpForceClos...
Markdownlint
docs/release-notes/release-notes-0.18.1.md
22-22: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
26-26: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
36-36: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
40-40: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
48-48: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
51-51: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
56-56: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
59-59: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
62-62: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
78-78: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
79-79: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
4-4: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
5-5: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
6-6: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
8-8: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
9-9: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
10-10: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
11-11: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
12-12: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
14-14: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
15-15: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
16-16: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
17-17: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
18-18: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
26-26: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
48-48: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
58-58: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
1-1: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
31-31: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
32-32: Expected: 1; Actual: 0; Above (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
32-32: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
33-33: Expected: 1; Actual: 0; Above (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
33-33: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
34-34: Expected: 1; Actual: 0; Above (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
44-44: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
45-45: Expected: 1; Actual: 0; Above (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
45-45: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
46-46: Expected: 1; Actual: 0; Above (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
65-65: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
66-66: Expected: 1; Actual: 0; Above (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
66-66: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
67-67: Expected: 1; Actual: 0; Above (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
69-69: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
70-70: Expected: 1; Actual: 0; Above (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
70-70: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
71-71: Expected: 1; Actual: 0; Above (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
71-71: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
72-72: Expected: 1; Actual: 0; Above (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
72-72: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
73-73: Expected: 1; Actual: 0; Above (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
73-73: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
74-74: Expected: 1; Actual: 0; Above (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
20-20: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
31-31: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
44-44: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
69-69: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
76-76: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
2-2: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
GitHub Check: lint code
cmd/lncli/walletrpc_active.go
[failure] 528-528:
funcgetWaitingCloseCommitmentsis unused (unused)lnrpc/walletrpc/walletkit_server.go
[failure] 1136-1136:
unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
Gitleaks
lnrpc/walletrpc/walletkit_server.go
2406-2407: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
2412-2413: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
2416-2417: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
Additional comments not posted (9)
lnrpc/walletrpc/config_active.go (1)
82-85: The addition ofGetWaitingCloseChannelto theConfigstruct aligns with the PR objectives to enable channel closure functionalities at the RPC level. Ensure that the function provided to this field is thoroughly tested for various scenarios to guarantee robustness.subrpcserver_config.go (1)
205-207: Properly setting theGetWaitingCloseChannelin theWalletKitRPCconfiguration is essential for the new fee bump functionality. This change appears correctly implemented.lnrpc/walletrpc/walletkit.pb.json.go (1)
575-598: The addition of theBumpForceCloseFeecallback is crucial for the new RPC functionality to be accessible via JSON. The implementation appears correct, ensuring proper error handling and response marshaling. It's important to test this thoroughly to ensure it handles various edge cases correctly.lnrpc/walletrpc/walletkit.proto (1)
276-282: The addition ofBumpForceCloseFeeRPC method is well-implemented and correctly documented. It specifically addresses the need to bump the fee for force-closed anchor channels, aligning with the PR objectives.cmd/lncli/walletrpc_active.go (1)
511-518: Ensure proper error handling and context propagation.Given the critical nature of the
BumpForceCloseFeeoperation, it's essential to ensure that all errors are handled appropriately and that the context is correctly propagated. Please verify that all potential errors from downstream calls are managed effectively and that the operation's context is consistent with the expected behavior.Verification successful
The new context provided by the shell script results shows that error handling around the
BumpForceCloseFeeoperation is indeed present in multiple instances across the codebase. Specifically, there are checks forif err != nilafter theBumpForceCloseFeecalls, ensuring that errors are managed appropriately.Additionally, context propagation appears to be handled correctly, as seen in the various usages of
ctxand the creation of new contexts withcontext.WithCancelandruntime.AnnotateContext.Given this information, it is clear that the concerns regarding error handling and context propagation are addressed.
- Locations with proper error handling and context propagation:
lnrpc/walletrpc/walletkit.pb.json.golnrpc/walletrpc/walletkit.pb.gw.golnrpc/walletrpc/walletkit_grpc.pb.gocmd/lncli/walletrpc_active.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling in BumpForceCloseFee operation. # Test: Search for error handling patterns around BumpForceCloseFee calls. rg --type go 'BumpForceCloseFee' | grep 'if err != nil'Length of output: 55
Script:
#!/bin/bash # Description: Verify error handling and context propagation in BumpForceCloseFee operation. # Search for the BumpForceCloseFee function calls in the codebase. rg --type go 'BumpForceCloseFee' -A 5 -B 5 # Check for context propagation by searching for context usage around BumpForceCloseFee. rg --type go 'BumpForceCloseFee' -A 5 -B 5 | rg 'ctx'Length of output: 29091
lnwallet/wallet.go (1)
1160-1179: The implementation ofGetWaitingCloseChannelappears functionally correct.This method extends the
LightningWalletstruct to support fetching channels in a 'waiting close' state, which is relevant for the PR's objectives related to managing channel states more effectively. Good use of error handling and clear structure in the method.lnrpc/walletrpc/walletkit_server.go (1)
110-113: Ensure correct permission settings for the new RPC method.The permissions for the new RPC method
BumpForceCloseFeeare correctly set to "write" which aligns with its functionality of modifying the fee rate of unconfirmed anchor channels. This is crucial for maintaining security and appropriate access control.contractcourt/channel_arbitrator.go (2)
1326-1333: ### Review: Logic to Handle Anchor Sweeping with No HTLCs at Stake
The logic for sweeping anchors when no HTLCs are at stake (lines 1326-1333) appears well-implemented. It correctly checks for the absence of a deadline and logs the sweeping operation, which is crucial for maintaining the state of the channel in scenarios where no immediate action is required on the HTLCs. This is aligned with the PR's objective of handling anchor channels more effectively, especially in edge cases like the absence of HTLCs.
Line range hint
288-308: ### Review: Error Handling inForceCloseChan
The functionForceCloseChanin theArbChannelinterface (lines 288-308) is critical as it forces the closure of a channel and updates the state accordingly. The error handling here is crucial because any failures in this process can lead to inconsistent states or loss of funds. It's important to ensure that all possible errors are handled gracefully and that the channel's state is accurately updated in the database to reflect these changes.Verification successful
The implementations of
ForceCloseChanincontractcourt/chain_arbitrator.goandcontractcourt/channel_arbitrator_test.gohandle errors appropriately. The main implementation incontractcourt/chain_arbitrator.goincludes error checks after each critical operation, ensuring that errors are returned immediately and handled gracefully. The mock implementation incontractcourt/channel_arbitrator_test.goalso simulates error handling by returning predefined errors.
contractcourt/chain_arbitrator.go:319-356: Proper error handling is implemented at each step.contractcourt/channel_arbitrator_test.go:2994-3004: Mock implementation simulates error handling.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all implementations of ForceCloseChan handle errors correctly. rg "ForceCloseChan" --type goLength of output: 2141
Script:
#!/bin/bash # Extract the implementation of ForceCloseChan in contractcourt/channel_arbitrator.go and contractcourt/chain_arbitrator.go ast-grep --lang go --pattern 'func (a *arbChannel) ForceCloseChan() ($_, error) { $$$ }' contractcourt/chain_arbitrator.go ast-grep --lang go --pattern 'func (m *mockChannel) ForceCloseChan() ($_, error) { $$$ }' contractcourt/channel_arbitrator_test.go ast-grep --lang go --pattern 'ForceCloseChan() ($_, error)' contractcourt/channel_arbitrator.goLength of output: 3771
ziggie1984
force-pushed
the
bumpforceclosefee-rpc
branch
3 times, most recently
from
ae64e45 to
b313721
Compare
ziggie1984
force-pushed
the
bumpforceclosefee-rpc
branch
from
b313721 to
0a11b8d
Compare
|
We definitely need to prioritize the flattening of the mulit_hop tests, fixing those takes super long because they include a lot of different special case for different channel types ... |
|
waiting for #8717, so that itests are not updated twice. |
|
I think this should be moved to 0.19 since we are adding an RPC? |
Need to understand this better. I thought rpc was already added in 0.18 but not doing its function of bumping up the fee |
|
This PR needs a final decision before I will proceed fixing the itests, basically the following: Currently when creating the Force-Close transaction we will not account for the parent fee rate immediately but rely on the When force-closing a channel the conf target will never select the right fee-rate which was essentially required but will only reach this feerate several blocks down the road which can be a long time until the desired feerate is reached in case the commitment transaction (parent) is very big. Moreover when trying to bump the feerate of a closing transaction with this newly introduced RPC we will not be able to specify the exact feerate and therefore this might be a bad user experience. Based on the decision above when we account for the parent feerate we need to change some itests which are now failing, but if we decide against it those itests remain unchanged. That's why I am waiting for your feedback before proceeding. Maybe @morehouse and @yyforyongyu and comment on this matter ? Thank you in advance for your time. |
With the new sweeper I don't think it makes sense to specify a feerate anymore. Instead, users should seek to specify a budget, which is more accurate IMO, as in the end you wanna control the fees paid. Previously we achieved so by specifying a feerate, but it was not precise as it's mostly estimation. |
Although its a new RPC, the logic was mostly copied from the |
ziggie1984
force-pushed
the
bumpforceclosefee-rpc
branch
2 times, most recently
from
ba02bd0 to
96f6a97
Compare
ziggie1984
force-pushed
the
bumpforceclosefee-rpc
branch
from
96f6a97 to
773be0a
Compare
|
@yyforyongyu: review reminder |
lntest/wait/wait.go
Outdated
| @@ -71,6 +71,40 @@ func NoError(f func() error, timeout time.Duration) error { | |||
| return nil | |||
| } | |||
|
|
|||
| // NoErrorWithValue runs the function f and returns its value if it succeeds | |||
There was a problem hiding this comment.
Looks like this can be removed now.
| chanPoint := chanPoints[0] | ||
| alice := nodes[0] | ||
|
|
||
| // We need to fund alice with 2 wallet inputs so that we can test to |
There was a problem hiding this comment.
This does remind me something - that we need to check how many UTXOs are there and return an error if it's below 2, otherwise we can only bump one of the anchors below, and the error will only be shown in the logs, which can be a bad UX. Not in this PR tho as we will soon merge it.
There was a problem hiding this comment.
Ahh ok so you mean we would need 3 wallet inputs in the above case, so that we can be sure that we have enough wallet inputs to at least try to publish the anchor transaction.
Hmm however I think one input for all the anchor sweeps is enough, because as soon as one succeeds we can be sure the others won't make it into the mempool anyways ?
I added a TODO to investigate it.
There was a problem hiding this comment.
For full nodes yes, for neutrino nope - for instance when a local force close happens, and we only have one wallet utxo, that utxo could be used to bump the remote anchor, while an error won't be returned there.
There was a problem hiding this comment.
Makes sense, you mean we should generalize this behavior because this test for example skips the neutrino case, however you are right if we are not generalizing it we might run into this problem when all of a sudden we want to support the neutrino case here.
Add a new bumpforceclosefee rpc endpoint to the wallet server. Move the logic from the lncli level to the wallet server rpc level. This is more in line with a proper client-server design. wallet lncli: use new bumpforceclosefee endpoint. Besides using the new bumpforceclosefee rpc endpoint we also enable the bumping of taproot anchor channels.
Legacy Channel types are not tested anymore via the multi-hop test harness, therefore we can remove unused code.
ziggie1984
force-pushed
the
bumpforceclosefee-rpc
branch
2 times, most recently
from
780ced1 to
68781ca
Compare
return the transaction when found in the mempool.
Add an itest which will bump the close fee rate of an anchor channel which is force closed without having any HTLCs at stake.
Make sure we only trigger the sweep of a specific input when updating its fee parameters not the all the current pending registered sweeps.
ziggie1984
force-pushed
the
bumpforceclosefee-rpc
branch
from
68781ca to
08aba8c
Compare
| @@ -0,0 +1,56 @@ | |||
| # Release Notes | |||
|
0.18.3 is on a side branch, there is no merge freeze in effect. |
Fixes #8837For reviewers please also focus on the following questions I included into the code starting with
Should we add an Integration Test for this new RPC call? => Yes I will
Thank you in advance 🙏
EDIT: Depends on #8946