Fix batchopen fee calculation #8896

ziggie1984 · 2024-07-04T13:39:38Z

Simple fix, though the underlying design probably needs some refactor because even for the batchopen flow we don't really need to do the fee-calculation for every channel but rather for the whole request.

Fixes #8895

Will add release-notes as soon as we decided in which release this will be included.

coderabbitai · 2024-07-04T13:39:43Z

Important

Review skipped

Auto reviews are limited to specific labels.

Labels to auto review (1)

llm-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

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?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

bitromortac

Nice! Looks good, I think it would be great to add a check in the itest that the batched opening transaction has the right fee rate

rpcserver.go

yyforyongyu

Cool I think this is now missing a release note and test, otherwise good to go.

In terms of testing, I think we can update one of the batch opening tests, with some modifications,

set a fee rate for a targeted conf via ht.SetFeeEstimateWithConf
open it with the targeted conf
validate the fee rate is used.

ziggie1984 · 2024-07-11T20:29:56Z

Let me know if I should remove the lncli refactor, but I thought we should use the sync call there as well when the incentive is to open a channel in a non-blocking way. Otherwise the for loop on the LND server is still running although the client already exited after receiving the PendingOpenRequest. So it is a slight improvement because the server side will only cancel the goroutine when the Channel is open (has the required confirmations).

yyforyongyu · 2024-07-17T00:03:34Z

cmd/lncli/cmd_open_channel.go

@@ -953,6 +965,37 @@ func printChanOpen(update *lnrpc.OpenStatusUpdate_ChanOpen) error {
 	return nil
 }

+// printPendingChan prints the funding txID of the pending channel opening.
+func printPendingChan(pendingChan *lnrpc.ChannelPoint) error {


think we already have printChanPending?

renamed it to printPendingChanPoint because on of them takes in a lnrpc.OpenStatusUpdate_ChanPending the other a lnrpc.ChannelPoint

yyforyongyu · 2024-07-17T00:05:09Z

cmd/lncli/cmd_open_channel.go

@@ -460,6 +460,22 @@ func openChannel(ctx *cli.Context) error {
 			"combination with the --psbt flag")
 	}

+	// In case we dont want to wait until the channel is fully confirmed
+	// onchain we use the `OpenChannelSync` rpc call.


I think the current block already does that?

tried to explain it in the other comment:

Otherwise the for loop on the LND server is still running although the client already exited after receiving the PendingOpenRequest. So it is a slight improvement because the server side will only cancel the goroutine when the Channel is open (has the required confirmations).

verified it on regtest, we indeed keep the goroutine of the RPC server open even if we opened a channel in an "unblock" way, thats why we should use the sync call in this case:

goroutine 896 [select, 6 minutes]: github.com/lightningnetwork/lnd.(*rpcServer).OpenChannel(0x1400018ca00, 0x14000d17e60, {0x1024e69d0, 0x1400074a140}) github.com/lightningnetwork/lnd/rpcserver.go:2408 +0x2a4 github.com/lightningnetwork/lnd/lnrpc._Lightning_OpenChannel_Handler({0x1024a6120, 0x1400018ca00}, {0x1024e1f70, 0x140023c78c0}) github.com/lightningnetwork/lnd/lnrpc/lightning_grpc.pb.go:2392 +0x120 github.com/grpc-ecosystem/go-grpc-prometheus.init.(*ServerMetrics).StreamServerInterceptor.func4({0x1024a6120, 0x1400018ca00}, {0x1024e1748, 0x14002862000}, 0x140025933f8, 0x1024b2fc0) github.com/grpc-ecosystem/[email protected]/server_metrics.go:121 +0xc8

moved to #8934

yyforyongyu · 2024-07-17T00:19:21Z

itest/lnd_funding_test.go

+	require.NoError(ht, err)
+
+	// Mine 1 block to confirm the funding transaction.
+	block := ht.MineBlocksAndAssertNumTxes(1, 1)[0]


don't think we need to check whether the tx is found in the block or not since that's the only tx that could happen in this context. Meanwhile we can use h.GetNumTxsFromMempool instead to get the tx,

yyforyongyu · 2024-07-17T00:20:11Z

cmd/lncli/cmd_open_channel.go

+	// In case we dont want to wait until the channel is fully confirmed
+	// onchain we use the `OpenChannelSync` rpc call.
+	if !ctx.Bool("block") {
+		pendingChan, err := client.OpenChannelSync(ctxc, req)


Feels like this is unrelated to this PR.

ok will move this change into its own new PR.

bitromortac · 2024-07-19T14:26:16Z

itest/lnd_funding_test.go

+	// channel is still not confirmed but pending in the mempool.
+	chanPoint := ht.OpenChannelSync(alice, bob, lntest.OpenChannelParams{
+		Amt:        chanAmt,
+		ConfTarget: fn.Some(int32(3)),


thanks for adding that test! to have different coverage than the batch open test, we could test for the sat per vbyte here

bitromortac · 2024-07-22T08:16:03Z

server.go

@@ -4562,6 +4562,10 @@ func (s *server) OpenChannel(
 			req.Err <- err
 			return req.Updates, req.Err
 		}
+
+		srvrLog.Infof("Funding fee rate was not set, using defaut "+


does it make sense to remove the code above and error in case no fee rate was set? it's a bit strange that we deal with default target confirmations in two different places

totally agree, let's error out if no feerate is specified.

rpcserver.go

ziggie1984 · 2024-07-23T16:37:02Z

So this PR needs to remove the lncli change in its own PR apart from this, everything should be ready to go.

ziggie1984 · 2024-07-24T08:26:42Z

Moved the lncli improvement to #8934

yyforyongyu

Close! CI failed tho, need to update a few callsites as they were changed in master.

yyforyongyu · 2024-07-24T10:20:31Z

server.go

-		}
-		req.FundingFeePerKw = feeRate
+		req.Err <- fmt.Errorf("no FundingFeePerKw specified for " +
+			"the channel opening transaction")


why do we change this? I think initially I did make it mandatory to include a conf target or feerate, then #8693 was added to revert it back - think we should move this change to a new commit in this PR if this is a bug fix.

yes I think we can return an error here because we already do this in the parsing function.

moved it into a separate commit, tho I want to mention this is not a bug fix, but just unifying the place where we inject a default setting.

yyforyongyu · 2024-07-24T10:24:44Z

lntest/harness.go

@@ -1094,6 +1094,16 @@ func (h *HarnessTest) openChannelAssertPending(srcNode,
 	return update.ChanPending, respStream
 }

+// OpenChannelSync attempts to open a channel between srcNode and destNode with
+// the passed channel funding parameters.
+func (h *HarnessTest) OpenChannelSync(srcNode,


move to #8934?

yyforyongyu · 2024-07-24T10:25:03Z

itest/list_on_test.go

@@ -101,6 +101,10 @@ var allTestCases = []*lntest.TestCase{
 		Name:     "wallet rescan address detection",
 		TestFunc: testRescanAddressDetection,
 	},
+	{
+		Name:     "open channel sync funding",
+		TestFunc: testChannelOpeningSync,


let's move this to #8934 since it's not related to this PR?

bitromortac

LGTM 🎉

rpcserver.go

yyforyongyu

LGTM🦾

Include the fee calculaltion for the psbt flow. Moreover include fee rate testing in the itest environment.

Setting default values for the channel opening fee rate is already done elsewhere therefore we remove on of those checks and return an error if no fee rate is specified.

ziggie1984 self-assigned this Jul 4, 2024

ziggie1984 marked this pull request as ready for review July 4, 2024 13:53

saubyk requested a review from bitromortac July 4, 2024 15:26

bitromortac reviewed Jul 8, 2024

View reviewed changes

rpcserver.go Show resolved Hide resolved

rpcserver.go Show resolved Hide resolved

rpcserver.go Show resolved Hide resolved

saubyk added this to the v0.18.3 milestone Jul 9, 2024

yyforyongyu reviewed Jul 10, 2024

View reviewed changes

ziggie1984 force-pushed the batchopen-feerate-fix branch 2 times, most recently from c3befc3 to cef9954 Compare July 11, 2024 20:21

ziggie1984 requested review from yyforyongyu and bitromortac July 11, 2024 20:23

ziggie1984 force-pushed the batchopen-feerate-fix branch from cef9954 to 5cc2a18 Compare July 11, 2024 20:26

ziggie1984 force-pushed the batchopen-feerate-fix branch from 5cc2a18 to a364eab Compare July 11, 2024 20:50

yyforyongyu reviewed Jul 17, 2024

View reviewed changes

bitromortac reviewed Jul 22, 2024

View reviewed changes

ziggie1984 force-pushed the batchopen-feerate-fix branch 2 times, most recently from 671454a to 14ebaed Compare July 23, 2024 16:35

ziggie1984 mentioned this pull request Jul 24, 2024

lncli openchannel fix #8934

Open

ziggie1984 force-pushed the batchopen-feerate-fix branch from 14ebaed to c48ea4e Compare July 24, 2024 08:25

ziggie1984 requested review from yyforyongyu and bitromortac July 24, 2024 08:27

yyforyongyu reviewed Jul 24, 2024

View reviewed changes

ziggie1984 force-pushed the batchopen-feerate-fix branch from c48ea4e to a474341 Compare July 24, 2024 13:04

bitromortac approved these changes Jul 25, 2024

View reviewed changes

rpcserver.go Outdated Show resolved Hide resolved

yyforyongyu approved these changes Jul 25, 2024

View reviewed changes

ziggie1984 added 2 commits July 26, 2024 10:48

rpcserver: include fee calc. for psbt flow.

d0a7765

Include the fee calculaltion for the psbt flow. Moreover include fee rate testing in the itest environment.

lnd: unify the default setting behaviour.

bf38aed

Setting default values for the channel opening fee rate is already done elsewhere therefore we remove on of those checks and return an error if no fee rate is specified.

docs: add release-notes.

eb7818a

ziggie1984 force-pushed the batchopen-feerate-fix branch from a474341 to eb7818a Compare July 26, 2024 08:48

guggero merged commit b7c59b3 into lightningnetwork:master Jul 26, 2024
25 of 34 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix batchopen fee calculation #8896

Fix batchopen fee calculation #8896

ziggie1984 commented Jul 4, 2024 •

edited by saubyk

Loading

coderabbitai bot commented Jul 4, 2024 •

edited

Loading

Review skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

bitromortac left a comment

yyforyongyu left a comment

ziggie1984 commented Jul 11, 2024 •

edited

Loading

yyforyongyu Jul 17, 2024

ziggie1984 Jul 23, 2024

yyforyongyu Jul 17, 2024

ziggie1984 Jul 23, 2024 •

edited

Loading

ziggie1984 Jul 24, 2024

yyforyongyu Jul 17, 2024

yyforyongyu Jul 17, 2024

ziggie1984 Jul 23, 2024

bitromortac Jul 19, 2024

bitromortac Jul 22, 2024

ziggie1984 Jul 23, 2024

ziggie1984 commented Jul 23, 2024

ziggie1984 commented Jul 24, 2024

yyforyongyu left a comment

yyforyongyu Jul 24, 2024

ziggie1984 Jul 24, 2024

ziggie1984 Jul 24, 2024

yyforyongyu Jul 24, 2024

yyforyongyu Jul 24, 2024

bitromortac left a comment

yyforyongyu left a comment

Fix batchopen fee calculation #8896

Fix batchopen fee calculation #8896

Conversation

ziggie1984 commented Jul 4, 2024 • edited by saubyk Loading

coderabbitai bot commented Jul 4, 2024 • edited Loading

Review skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

bitromortac left a comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

ziggie1984 commented Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziggie1984 Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziggie1984 commented Jul 23, 2024

ziggie1984 commented Jul 24, 2024

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitromortac left a comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

ziggie1984 commented Jul 4, 2024 •

edited by saubyk

Loading

coderabbitai bot commented Jul 4, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

ziggie1984 commented Jul 11, 2024 •

edited

Loading

ziggie1984 Jul 23, 2024 •

edited

Loading