Sweep inputs even the budget cannot be covered #9627

yyforyongyu · 2025-03-21T04:13:59Z

This PR fixes the issue of the sweep being delayed due to the current wallet UTXOs not being able to cover the full budget. An example case is this sweeping tx, in which an HTLC output was swept with overshooting fees. This happened because the sweeper requires the budget to be fully covered before attempting the sweep, otherwise the sweeper will wait until more wallet UTXOs are available - by that time, the deadline may already be very close, or even passed, causing the full budget to be used up at once.

This PR changes the all or nothing behavior by starting the sweeping process asap if the budget can be covered partially. Later on, when there are more wallet UTXOs, the sweeper will add them to make up the rest of the budget.

coderabbitai · 2025-03-21T04:14:06Z

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.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ 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>, please review it.
- 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 gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @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 using 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 generate docstrings to generate docstrings for this PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai plan to trigger planning for file edits and PR creation.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

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.

ziggie1984

First pass done

Will need another look to evaluate the sweeper changes tho.

So when an input set cannot cover its fees, the inputs will be marked as failed, but the input set will still remain in memory and we are going to add another wallet input when another block arrives ?

sweep/tx_input_set.go

ziggie1984 · 2025-03-21T18:59:06Z

rpcserver.go

 	for _, htlc := range dbChannel.RemoteCommitment.Htlcs {
-		remoteHTLCs.Add(htlc.RHash)
+		remoteHTLCs.Add(htlc.HtlcIndex)


I think that won't work either because incoming and outgoing htlcs start a new counter so the counter are not unique, you either need a comibnation of the hash+index, or incoming_bool + index to fix it properly.

yeah right - fixed!

ziggie1984 · 2025-03-21T20:43:51Z

sweep/fee_bumper.go

+// fee rate for the next sweep attempt if the inputs are to be retried. An error
+// is returned when the fee func is nil and created without error, otherwise an


An error is returned when the fee func is nil and created without error :=> can you rephrase this I am unsure I understand what you are trying to say.

ziggie1984 · 2025-03-21T21:26:15Z

sweep/fee_bumper.go

+		feeFunc = f
+	}
+
+	// Since the sweeping tx has been replaced by another party's tx, we


Q: Is this always the case that a sweep is replaced by another party, I mean we can also just run out of budget or ?

Roasbeef

First pass, the change makes intuitive sense: we should at least attempt a sweep even if we don't have all the wallet inputs we need to publish at our ideal fee rate.

sweep/tx_input_set.go

Roasbeef · 2025-03-21T23:22:59Z

rpcserver.go

@@ -9085,5 +9091,10 @@ func (r *rpcServer) getChainSyncInfo() (*chainSyncInfo, error) {
 	// Overwrite isSynced and return.
 	info.isSynced = height == bestHeight

+	if !info.isSynced {
+		rpcsLog.Debugf("Blockbeat is not synced to height %v yet",


So simply logging this slows things down enough for a flake to not occur?

nope - it's a rare flake that caused the syncing to be blocked, but there was little logging in that build. I added more debug logs such that if it happens again, we can have more info here.

yyforyongyu · 2025-03-24T08:51:37Z

So when an input set cannot cover its fees, the inputs will be marked as failed, but the input set will still remain in memory and we are going to add another wallet input when another block arrives ?

Yeah except it's not the input set, but the inputs will remain and we will attempt again in another block. These inputs will be put in a new input set.

ziggie1984

Looks good to me, still two open question from my side then it's gtg:

Can we also fire the sweep if we have no walletInputs at all but wallet inputs are needed to cover the full budget
Limit the budget depending on the MaxFeeRate so that we do not use WalletInputs which are not needed in the first place because we will never use up the full wallet balance.

sweep/tx_input_set.go

sweep/fee_bumper.go

ziggie1984 · 2025-03-24T14:47:37Z

sweep/fee_bumper.go

@@ -1893,3 +1854,59 @@ func (t *TxPublisher) handleReplacementTxError(r *monitorRecord,

 	return result
 }
+
+// calculateRetryFeeRate calculates a new fee rate to be used as the starting
+// fee rate for the next sweep attempt if the inputs are to be retried. When the


So for this function to be used, do the inputs all need to be "retried inputs" or can even new inputs be swept using this function when regrouped with "retried inputs" ? This might sweep new inputs if regrouped with new inputs at a higher fee than necessary ?

yeah that's a tradeoff here - either we retry the sweeping with a starting fee rate of 0, which leaves the new inputs on the fee func line, or we keep the retried inputs on the line by using a starting fee rate derived from its last attempt, and keep the new inputs using the larger starting fee rate. This will be fixed if we have the ability to sweep by groups, and before that's fixed, I choose the latter approach since it's unlikely new inputs will be grouped given their deadlines usually vary.

understand thank you for the explanation.

sweep/fee_bumper.go

ziggie1984 · 2025-03-24T15:06:45Z

itest/lnd_sweep_test.go

 	// - budget: 1000 sats.
+	//
+	// NOTE: The starting fee rate is 0 instead of 1 because the budget has


I am a bit confused here, how can the starting feerate be 0, you mean not set at all and it will be set later in the fee_function ?

I think it is explained below - the sweeper will update the starting fee rate to the last attempted fee rate, which in this case is 0, because we don't set it when error out in ErrMaxPosistion.

Seems like a design decision, I wonder why you not just capped the value at the max feerate rather than not setting it at all, for me the former feels more intuative.

I also don't understand this change.

ok decided to change the behavior. So previously two things were happening,

we update the starting fee rate when there's TxFailed event to prepare for the next fee bump

we set the next fee rate in the fee bump it when the budget is not enough or the inputs are not enough
This was why the itest was updated -this particular fee bump will fail with max position error, which means a) it's a failed event, so the fee rate will be updated and, b) it's a max position error, so the next fee rate was not set in the bump result, hence when we update the starting fee rate, it will be set to 0 (empty value).

This is now changed that, if there's a max position error, we always set the next fee rate before returning the fee bump result to the sweeper to handle.

ziggie1984 · 2025-03-24T15:16:26Z

itest/lnd_sweep_test.go

+		resp.UnconfirmedBalance + resp.ConfirmedBalance,
+	)
+
+	fee := walletBalance*2 - balance


Q: Why the *2 here ?

ok updated the desc to make them more clear

ziggie1984 · 2025-03-24T15:22:16Z

itest/lnd_sweep_test.go

+
+	// Assert the above sweeping tx is still in the mempool.
+	ht.AssertTxInMempool(sweepTx2.TxHash())
+


Q: What would happen in case of a restart and a new utxo sweep being added to the group ?

in that case I think they will be batched together to create a new sweeping tx.

ziggie1984 · 2025-03-24T15:23:01Z

itest/lnd_sweep_test.go

+	// Fund Alice 200k sats, which will be used to cover the budget.
+	//
+	// TODO(yy): We are funding Alice more than enough - at this stage Alice
+	// has a confirmed UTXO of `walletBalance`` amount in her wallet, so


Nit: `walletBalance`` => `walletBalance`

ziggie1984 · 2025-03-24T15:24:34Z

itest/lnd_sweep_test.go

+	// since the confirmed wallet UTXO has already been used in sweepTx2,
+	// there's no easy way to tell her wallet to reuse that UTXO in the
+	// upcoming sweeping tx.


Q: I wonder what would happen in a restart, would the walletUTXO still be considered 0-conf and not be used ?

itest/lnd_sweep_test.go

yyforyongyu · 2025-03-25T02:35:23Z

Limit the budget depending on the MaxFeeRate so that we do not use WalletInputs which are not needed in the first place because we will never use up the full wallet balance.

There's already a TODO for the coin selection strategy, which is out of scope here.

ziggie1984

LGTM 🫡 - had only some final non-blocking comments.

ziggie1984 · 2025-03-25T15:38:04Z

sweep/tx_input_set.go

@@ -296,7 +296,9 @@ func (b *BudgetInputSet) NeedWalletInput() bool {
 		}

 		// Get the amount left after covering the input's own budget.
-		// This amount can then be lent to the above input.
+		// This amount can then be lent to the above input. For a wallet
+		// input, its `Budget`` is set to zero, which means the whole


Nit: Budget`` => Budget`

sweep/fee_bumper.go

ziggie1984 · 2025-03-25T15:40:12Z

sweep/fee_bumper.go

+	_, err := feeFunc.Increment()
+	if err != nil {
+		// The fee function has reached its max position - nothing we
+		// can do here other than letting the user increase the budget.


I realised lately that according to my taste we work way to much with comments rather than using specific errors which we can filter for. Would be great to have like a max_position error rather than having a detailed comment that it can only be the max_position error.

yeah totally agree - I actually thought about whether it's possible to refactor the method so it doesn't even return an error, then add a new method like MaxPositionReached and use it in other places when we want to check whether we've reached the end of the fee function. I think it works, tho it now means we now need to care about the state of the fee function, but I guess we already do implicitly.

As for here, I think we want to skip any error returned here, plus the error log, should give us enough info about what happened.

Moving forward, I think I will try to avoid error equality checks as much as possible, it's like try catch, think there's always a way to refactor the error out of existence in the first place.

ziggie1984 · 2025-03-25T17:14:06Z

sweep/tx_input_set.go

-	// The wallet doesn't have enough utxos to cover the budget. Revert the
-	// input set to its original state.
-	b.inputs = originalInputs
+	log.Warn("Not enough wallet UTXOs to cover the budget, sweeping " +


Nit: Add the amount we are short for the budget in the logs

it's actually a bit involved, but think it can be very helpful so added the log,

2025-03-26 16:27:58.316 [WRN] SWPR: Not enough wallet UTXOs: need budget=0.00020000 BTC, has spendable=0.00010000 BTC, total=0.00020000 BTC, missing at least 0.00010000 BTC, sweeping anyway...

ziggie1984 · 2025-03-25T17:14:41Z

sweep/fee_bumper.go

@@ -1893,3 +1854,59 @@ func (t *TxPublisher) handleReplacementTxError(r *monitorRecord,

 	return result
 }
+
+// calculateRetryFeeRate calculates a new fee rate to be used as the starting
+// fee rate for the next sweep attempt if the inputs are to be retried. When the


understand thank you for the explanation.

sweep/sweeper.go

ziggie1984 · 2025-03-25T17:58:33Z

itest/lnd_sweep_test.go

 	// - budget: 1000 sats.
+	//
+	// NOTE: The starting fee rate is 0 instead of 1 because the budget has


Seems like a design decision, I wonder why you not just capped the value at the max feerate rather than not setting it at all, for me the former feels more intuative.

lntest/harness.go

ziggie1984 · 2025-03-25T18:14:06Z

itest/lnd_sweep_test.go

+
+	// Fund Alice 200k sats, which will be used to cover the budget.
+	//
+	// TODO(yy): We are funding Alice more than enough - at this stage Alice


Nit: I would propose creating an issue for this TODO, otherwise it might become forgotten in the itest files. Wallet inputs are scarce and we should use them efficiently.

Not a nit, we need an issue for this.

At this stage I don't know who else is gonna fix that...but yeah it's the 3rd item listed here #8680.

Think I will create an issue too in case this gets to be the bitcoin summer internship project.

sweep/tx_input_set.go

sweep/tx_input_set_test.go

morehouse · 2025-03-25T18:47:09Z

sweep/fee_bumper.go

+	// send a TxFailed event so these inputs can be retried when the wallet
+	// has more UTXOs.
+	case errors.Is(err, ErrNotEnoughInputs),
+		errors.Is(err, ErrNotEnoughBudget):


Does ErrNotEnoughBudget really belong here? Previously this error was considered TxFatal so the user can increase the budget if needed.

Good q - I think it actually needs to be TxFailed since the inputs will be removed when it's TxFatal. I guess this in theory should never happen because we derive the fees based on the budget first, so the fees should never be greater than the budget, unless there's a bug in the tx weight calculation.

sweep/fee_bumper.go

itest/lnd_sweep_test.go

morehouse · 2025-03-25T19:37:27Z

itest/lnd_sweep_test.go

 	// - budget: 1000 sats.
+	//
+	// NOTE: The starting fee rate is 0 instead of 1 because the budget has


I also don't understand this change.

morehouse · 2025-03-25T19:43:31Z

lntest/harness_miner.go

@@ -247,7 +247,15 @@ func (h *HarnessTest) GetBestBlock() (*chainhash.Hash, int32) {

 // MineBlockWithTx mines a single block to include the specifies tx only.
 func (h *HarnessTest) MineBlockWithTx(tx *wire.MsgTx) *wire.MsgBlock {
-	return h.miner.MineBlockWithTx(tx)
+	// Update the harness's current height.
+	defer h.updateCurrentHeight()


Why does this need a defer?

Just a pattern we use in all MineBlockXXX methods

morehouse · 2025-03-25T20:01:49Z

itest/lnd_sweep_test.go

+
+	// Fund Alice 200k sats, which will be used to cover the budget.
+	//
+	// TODO(yy): We are funding Alice more than enough - at this stage Alice


Not a nit, we need an issue for this.

A minor refactor to prepare for upcoming changes.

We now always create the sweeping tx even though the budget cannot be covered so we don't miss the deadline. Note that the fee bump will fail once the provided wallet input cannot cover the increase fees, which is fine as these inputs will be marked as failed and be retried again in the next block. When that happens, if there are new wallet UTXOs, a new batch will be created to perform the fee bump.

A minor refactor to prepare the upcoming changes.

We now return the next retry fee rate in `TxFailed` event in `TxPublisher`. When handling the event, `UtxoSweeper` will update the inputs to make sure the starting fee rate is set before attempting the next sweep.

Make sure we assertPendingSweepResp in a wait call to wait for the updated resp.

This is used is a following test.

Make sure we update the harness's current height and assert nodes have been synced. Also fixes some typo found.

This is added to fix a flake found in starting the node.

We now start the sweeping process if there are normal inputs to partially cover the budget.

yyforyongyu added utxo sweeping size/kilo medium, proper context needed, less than 1000 lines labels Mar 21, 2025

yyforyongyu added this to the v0.19.0 milestone Mar 21, 2025

yyforyongyu self-assigned this Mar 21, 2025

yyforyongyu force-pushed the sweep-under-budget branch from 80f23bf to 071ed00 Compare March 21, 2025 07:12

ziggie1984 self-requested a review March 21, 2025 18:48

ziggie1984 reviewed Mar 21, 2025

View reviewed changes

Roasbeef reviewed Mar 21, 2025

View reviewed changes

yyforyongyu force-pushed the sweep-under-budget branch from 071ed00 to 7d0b4b0 Compare March 24, 2025 08:47

ziggie1984 reviewed Mar 24, 2025

View reviewed changes

yyforyongyu force-pushed the sweep-under-budget branch from 7d0b4b0 to 360566f Compare March 25, 2025 02:30

yyforyongyu requested a review from Roasbeef March 25, 2025 02:40

yyforyongyu force-pushed the sweep-under-budget branch 2 times, most recently from e8630f7 to b0a9c14 Compare March 25, 2025 05:36

yyforyongyu requested a review from ziggie1984 March 25, 2025 07:03

ziggie1984 approved these changes Mar 25, 2025

View reviewed changes

morehouse reviewed Mar 25, 2025

View reviewed changes

yyforyongyu added 3 commits March 26, 2025 14:25

sweep: refactor AddWalletInputs by adding addWalletInput

3c4fd1b

A minor refactor to prepare for upcoming changes.

sweep: add method calculateRetryFeeRate

6dbf4ce

A minor refactor to prepare the upcoming changes.

yyforyongyu force-pushed the sweep-under-budget branch from b0a9c14 to 1e658cd Compare March 26, 2025 09:20

yyforyongyu added 6 commits March 26, 2025 18:24

sweep+itest: return next retry fee rate in TxFailed event

eea3561

We now return the next retry fee rate in `TxFailed` event in `TxPublisher`. When handling the event, `UtxoSweeper` will update the inputs to make sure the starting fee rate is set before attempting the next sweep.

itest: refactor runBumpFee to fix a flake

4abc146

Make sure we assertPendingSweepResp in a wait call to wait for the updated resp.

lntest+itest: return the tx from FundCoins

883381d

This is used is a following test.

lntest+itest: update block height in MineBlockWithTx

64f7a7f

Make sure we update the harness's current height and assert nodes have been synced. Also fixes some typo found.

itest: add testBumpFeeLowBudget

8ad122b

lnd: log sync status in GetInfo

70dec8e

This is added to fix a flake found in starting the node.

yyforyongyu added 4 commits March 26, 2025 18:24

rpcserver: use HtlcIndex as the unique key

43409c7

docs: update release notes

b6daa3b

sweep: start the sweeping if there are normal inputs

c7bea07

We now start the sweeping process if there are normal inputs to partially cover the budget.

sweep: remove dead code

ec2f3ad

yyforyongyu force-pushed the sweep-under-budget branch from 1e658cd to ec2f3ad Compare March 26, 2025 10:24

morehouse approved these changes Mar 26, 2025

View reviewed changes

yyforyongyu merged commit 15dbc43 into lightningnetwork:master Mar 27, 2025
34 of 36 checks passed

yyforyongyu deleted the sweep-under-budget branch March 27, 2025 05:32

		// fee rate for the next sweep attempt if the inputs are to be retried. An error
		// is returned when the fee func is nil and created without error, otherwise an


		// Assert the above sweeping tx is still in the mempool.
		ht.AssertTxInMempool(sweepTx2.TxHash())

Sweep inputs even the budget cannot be covered #9627

Sweep inputs even the budget cannot be covered #9627

Uh oh!

Conversation

yyforyongyu commented Mar 21, 2025

Uh oh!

coderabbitai bot commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

ziggie1984 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yyforyongyu commented Mar 24, 2025

Uh oh!

ziggie1984 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yyforyongyu Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot commented Mar 21, 2025 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

yyforyongyu Mar 26, 2025 •

edited

Loading