contractcourt+sweep: fix fee function and deadline issue #8751

yyforyongyu · 2024-05-14T13:03:59Z

TODO:

fix itest

coderabbitai · 2024-05-14T13:04:05Z

Important

Auto Review Skipped

Auto reviews are disabled on this repository.

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.

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 a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

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

CodeRabbit Configration 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.

morehouse

Needs an itest for #8738.

contractcourt/channel_arbitrator.go

sweep/fee_function.go

morehouse · 2024-05-14T16:32:18Z

sweep/fee_function.go

+	// budget is too small.
 	if l.deltaFeeRate == 0 && l.width != 1 {
 		log.Errorf("Failed to init fee function: startingFeeRate=%v, "+
 			"endingFeeRate=%v, width=%v, delta=%v", start, end,
-			confTarget, l.deltaFeeRate)
+			l.width, l.deltaFeeRate)


What is the reason for this check? Can we remove it?

It seems like the check prevents broadcasting the transaction in cases where the budget is actually enough. For example:

confTarget is 2

starting feerate calculated from confTarget is 30 sat/vB

ending feerate for budget is 30 sat/vB

If we broadcast immediately at 30 sat/vB, the transaction is likely to confirm in time. But currently an error is returned instead, and broadcasting is delayed until the next block when confTarget is 1 and the branch above is taken instead. But we are likely to miss the deadline in that case, since we broadcasted late.

As shown in the unit test, when conf target is 2, the max fee rate will be used. The check is added so we don't have a fee rate delta of 0, since in that case the fee func cannot do anything, also indicates the budget is too small.

I think it makes sense to keep this case in place and not broadcast the sweep when we still have not reached the deadline (conftarget=2) so that more inputs can eventually increase the endFeeRate. Eventually the sweep is triggered in case the deadline is reached.

As shown in the unit test, when conf target is 2, the max fee rate will be used.

Right, bad example, sorry.

The check is added so we don't have a fee rate delta of 0, since in that case the fee func cannot do anything, also indicates the budget is too small.

In this case, shouldn't we just broadcast with max budget immediately and hope for the best?

Let's say conf target is 80 and fee rate delta is 0. Isn't it better to broadcast immediately at the max feerate? Returning an error means we will still broadcast at max feerate but we wait until 1 block away from the deadline, which basically guarantees we will miss the deadline.

I think it makes sense to keep this case in place and not broadcast the sweep when we still have not reached the deadline (conftarget=2) so that more inputs can eventually increase the endFeeRate.

It is very unlikely that future inputs will have the same deadline as the current inputs, so they are unlikely to be batched together.

Even so, there's no guarantee that future inputs will improve the situation -- they might not pay for their own weight either.

In this case, shouldn't we just broadcast with max budget immediately and hope for the best?

I guess the question is where is the hope coming from:)

I don't know if the age of a tx is still a factor for the miners these days. If not and, if the tx gets confirmed at some blocks later, it means the network fee rate goes down -> the fee func will be created then?

The initial intention is to return an error here, so the user can increase the budget - for instance, when the htlc is large and the budget ratio is small, the user can intervene by specifying a larger budget via BumpFee.

It is very unlikely that future inputs will have the same deadline as the current inputs, so they are unlikely to be batched together.

Yeah this is an issue IMO - if the deadlines are already large, I think they can be grouped - like, how different in terms of urgency they have between deadlines of 1008 and 1007 - think there should be a ladder structure, sth like the fee rate bucket we used to have.

I don't know if the age of a tx is still a factor for the miners these days. If not and, if the tx gets confirmed at some blocks later, it means the network fee rate goes down -> the fee func will be created then?

Maybe the fee func will be created then, or maybe not. How sensitive is bitcoind's fee estimator to temporary drops in fee rate? If not too sensitive, then the fee func will continue to fail.

And bitcoind's fee estimator is backwards-looking, so it will always be delayed in adjusting downward. So by the time we actually broadcast the transaction, it might be too late (because deadline is missed or because we missed the lower-fee window and mempool fees went back up).

Overall, I think we're better off to have the transaction in the mempool so it can potentially be mined. Until we broadcast it, it can never be mined.

The initial intention is to return an error here, so the user can increase the budget - for instance, when the htlc is large and the budget ratio is small, the user can intervene by specifying a larger budget via BumpFee.

Perhaps an early sanity check specifically for BumpFee is more appropriate? If budget is less than bitcoind's estimate, we can quickly return an error from BumpFee instead of processing further.

For contractcourt sweeps, I think it's better to broadcast at max budget than to give up completely.

Yeah this is an issue IMO - if the deadlines are already large, I think they can be grouped - like, how different in terms of urgency they have between deadlines of 1008 and 1007 - think there should be a ladder structure, sth like the fee rate bucket we used to have.

+1

contractcourt/channel_arbitrator.go

cmd/lncli/walletrpc_active.go

contractcourt/chain_arbitrator.go

ziggie1984

Had some minor comments apart from that it's LGTM

contractcourt/channel_arbitrator.go

sweep/fee_function.go

itest/lnd_sweep_test.go

itest/lnd_channel_force_close_test.go

itest/lnd_multi-hop_test.go

morehouse

Still have an itest failing for neutrino:

    harness_miner.go:217: 
        	Error Trace:	/home/runner/work/lnd/lnd/lntest/harness_miner.go:217
        	            				/home/runner/work/lnd/lnd/lntest/harness.go:1770
        	            				/home/runner/work/lnd/lnd/itest/lnd_multi-hop_test.go:2499
        	            				/home/runner/work/lnd/lnd/itest/lnd_multi-hop_test.go:152
        	Error:      	Received unexpected error:
        	            	want 1, got 2 in mempool: [08f01a438347fca122b815936cfa9417a66fbbaf2d3b02497eec7c4c382ca4aa 6b76702b74421fdc0a7b0fbdb12bd076bb7ff215dac9fdaae79ab5f54655355b]
        	Test:       	TestLightningNetworkDaemon/tranche03/134-of-155/neutrino/htlc_timeout_resolver_extract_preimage_remote/zeroconf=true/committype=ANCHORS
        	Messages:   	assert tx in mempool timeout

And possibly related failures on windows.

contractcourt/channel_arbitrator.go

morehouse · 2024-05-17T16:07:30Z

sweep/fee_function.go

+	// budget is too small.
 	if l.deltaFeeRate == 0 && l.width != 1 {
 		log.Errorf("Failed to init fee function: startingFeeRate=%v, "+
 			"endingFeeRate=%v, width=%v, delta=%v", start, end,
-			confTarget, l.deltaFeeRate)
+			l.width, l.deltaFeeRate)


As shown in the unit test, when conf target is 2, the max fee rate will be used.

Right, bad example, sorry.

The check is added so we don't have a fee rate delta of 0, since in that case the fee func cannot do anything, also indicates the budget is too small.

In this case, shouldn't we just broadcast with max budget immediately and hope for the best?

Let's say conf target is 80 and fee rate delta is 0. Isn't it better to broadcast immediately at the max feerate? Returning an error means we will still broadcast at max feerate but we wait until 1 block away from the deadline, which basically guarantees we will miss the deadline.

I think it makes sense to keep this case in place and not broadcast the sweep when we still have not reached the deadline (conftarget=2) so that more inputs can eventually increase the endFeeRate.

It is very unlikely that future inputs will have the same deadline as the current inputs, so they are unlikely to be batched together.

Even so, there's no guarantee that future inputs will improve the situation -- they might not pay for their own weight either.

itest/lnd_sweep_test.go

itest/lnd_channel_force_close_test.go

yyforyongyu · 2024-05-17T21:30:34Z

@morehouse Yeah the itest failure is annoying, don't think it can be properly fixed until blockbeat is in. Here's some logs which show that the subsystems have very different views of block heights,

2024-05-18 04:24:15.394 [DBG] SWPR: Received new block: height=476, attempt sweeping 0 inputs
...
2024-05-18 04:24:15.418 [INF] CNCT: *contractcourt.htlcSuccessResolver(ee04c77ad329073b6b7e1899e0173aa6c1835727f647b0853099fc7dcc6e0d2b): offering second-level success tx output to sweeper with no deadline and budget=0.00050000 BTC at height=471
...
2024-05-18 04:24:15.418 [INF] SWPR: Sweep request received: out_point=e0749b9458c5f6693eb79b6a6293feb44435467b2a68680185921f0d86425805:0, witness_type=HtlcAcceptedSuccessSecondLevel, relative_time_lock=4, absolute_time_lock=0, amount=0.00100000 BTC, parent=(<nil>), params=(startingFeeRate={false 0}, immediate=false, exclusive_group=none, budget=0.00050000 BTC, deadline=none)
2024-05-18 04:24:15.418 [INF] CNCT: *contractcourt.htlcSuccessResolver(ee04c77ad329073b6b7e1899e0173aa6c1835727f647b0853099fc7dcc6e0d2b): waiting for second-level HTLC output to be spent after csv_delay=4
...
2024-05-18 04:24:15.418 [DBG] SWPR: TxPublisher received new block: 471

As for now, I did an extra check in the test to make it pass. More work to do in 18.1, as I wanna fix these multi-hop tests so it's easier to maintain in the future.

This commit changes how the deadline is calculated for CPFP anchor sweeping. In order to sweep the second-level HTLCs, we need to first get the FC tx confirmed. If we use a larger conf target for CPFP, we'd end up having few blocks to sweep the HTLCs, as these two sweeping txns share the deadline of the HTLC, as shown below, ``` More aggressive on the CPFP part. |-CPFP-|-----HTLC-----| Share the deadlines evenly. |---CPFP---|---HTLC---| More aggressive on the HTLC part. |-----CPFP-----|-HTLC-| ``` In this commit, we decide to share the deadlines evenly as a starting point so neither side will have a short of deadlines.

Replaced `testSweepAnchorCPFPLocalForceClose` with dedicated tests.

This commit changes the logging levels and add a few more loggings based on the testing results from the testnet/mainnet.

Roasbeef

LGTM 🐠

yyforyongyu added utxo sweeping no-changelog labels May 14, 2024

yyforyongyu added this to the v0.18.0 milestone May 14, 2024

yyforyongyu self-assigned this May 14, 2024

morehouse reviewed May 14, 2024

View reviewed changes

saubyk requested a review from ziggie1984 May 16, 2024 15:18

saubyk linked an issue May 16, 2024 that may be closed by this pull request

[bug]: contractcourt: max budget used immediately for anchor sweeps #8738

Closed

yyforyongyu force-pushed the fix-sweeper-18 branch 4 times, most recently from 1245201 to 49b6994 Compare May 16, 2024 21:07

Roasbeef reviewed May 16, 2024

View reviewed changes

contractcourt/channel_arbitrator.go Show resolved Hide resolved

cmd/lncli/walletrpc_active.go Show resolved Hide resolved

contractcourt/chain_arbitrator.go Show resolved Hide resolved

yyforyongyu force-pushed the fix-sweeper-18 branch from 49b6994 to ed56a94 Compare May 17, 2024 10:07

ziggie1984 approved these changes May 17, 2024

View reviewed changes

morehouse reviewed May 17, 2024

View reviewed changes

yyforyongyu force-pushed the fix-sweeper-18 branch from ed56a94 to 36cb2f6 Compare May 17, 2024 21:27

yyforyongyu added 5 commits May 18, 2024 07:45

sweep: update and fix README

2906b8b

sweep: make sure the budget is used up at deadlineHeight-1

ac5af48

sweep: check all states in TestMarkInputsPublishFailed

38184e8

itest: add new itests to check CPFP anchor sweeping behavior

e68c023

Replaced `testSweepAnchorCPFPLocalForceClose` with dedicated tests.

yyforyongyu force-pushed the fix-sweeper-18 branch 2 times, most recently from 52ef42b to 1b33068 Compare May 20, 2024 15:11

yyforyongyu added 4 commits May 21, 2024 00:53

itest+sweep: fix current itest re anchor deadline

4079f61

lncli: add missing SatPerVbyte field in bumpfee cli

71ee50f

contractcourt+itest: make sure ChannelArbitrator has the latest htlcs

e45db07

sweep: improve loggings based on feedback

3475377

This commit changes the logging levels and add a few more loggings based on the testing results from the testnet/mainnet.

yyforyongyu force-pushed the fix-sweeper-18 branch from 1b33068 to 3475377 Compare May 20, 2024 16:53

Roasbeef approved these changes May 21, 2024

View reviewed changes

Roasbeef merged commit e1c5fe2 into lightningnetwork:master May 21, 2024
32 of 34 checks passed

yyforyongyu deleted the fix-sweeper-18 branch May 21, 2024 04:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contractcourt+sweep: fix fee function and deadline issue #8751

contractcourt+sweep: fix fee function and deadline issue #8751

yyforyongyu commented May 14, 2024 •

edited by Roasbeef

coderabbitai bot commented May 14, 2024

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

morehouse left a comment

morehouse May 14, 2024

yyforyongyu May 16, 2024

ziggie1984 May 17, 2024

morehouse May 17, 2024

yyforyongyu May 17, 2024

morehouse May 21, 2024

ziggie1984 left a comment

morehouse left a comment

morehouse May 17, 2024

yyforyongyu commented May 17, 2024

Roasbeef left a comment

contractcourt+sweep: fix fee function and deadline issue #8751

contractcourt+sweep: fix fee function and deadline issue #8751

Conversation

yyforyongyu commented May 14, 2024 • edited by Roasbeef

coderabbitai bot commented May 14, 2024

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

morehouse left a comment

Choose a reason for hiding this comment

morehouse May 14, 2024

Choose a reason for hiding this comment

yyforyongyu May 16, 2024

Choose a reason for hiding this comment

ziggie1984 May 17, 2024

Choose a reason for hiding this comment

morehouse May 17, 2024

Choose a reason for hiding this comment

yyforyongyu May 17, 2024

Choose a reason for hiding this comment

morehouse May 21, 2024

Choose a reason for hiding this comment

ziggie1984 left a comment

Choose a reason for hiding this comment

morehouse left a comment

Choose a reason for hiding this comment

morehouse May 17, 2024

Choose a reason for hiding this comment

yyforyongyu commented May 17, 2024

Roasbeef left a comment

Choose a reason for hiding this comment

yyforyongyu commented May 14, 2024 •

edited by Roasbeef

CodeRabbit Configration File (`.coderabbit.yaml`)