Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[epic] sweeper: improve sweeper with defined responsibility and functionalities #8042

Closed
yyforyongyu opened this issue Sep 28, 2023 · 2 comments · Fixed by #8667
Closed

[epic] sweeper: improve sweeper with defined responsibility and functionalities #8042

yyforyongyu opened this issue Sep 28, 2023 · 2 comments · Fixed by #8667
Assignees
Labels
brainstorming Long term ideas/discussion/requests for feedback design review enhancement Improvements to existing features / behaviour epic Issues created to track large feature development P1 MUST be fixed or reviewed utxo sweeping

Comments

@yyforyongyu
Copy link
Collaborator

Ever since sweeper being introduced a few years ago, we've had a better understanding of this subsystem's responsibilities and functionalities. On a high level, our sweeper is responsible for,

  1. sweeping UTXOs back to lnd's wallet to maximize its economic benefits
  2. aggregating UTXOs to keep the UTXO set from bloating

In details, the sweeper will,

  • take inputs from other subsystems
  • group the inputs based on either economical interest or UTXO aggregation
  • create and broadcast the sweeping transactions
  • monitor the blockchain to make sure the transactions confirms
  • react to different mempool conditions, which may involve RBF

And it is NOT responsible for waiting for locktime to expire. The spending condition of the input should be met when it's received, in other words, this input should be immediately spendable. It's the other subsystem's responsibility to watch for the locktime expiration.

Bug Fixes

Before attempting to improve the sweeper, there are few bugs we need to fix first,

Simplification of Clustering Rules

Our current clustering rules make it very difficult to control the fee rate accurately by the caller. For any given input, it's categorized based on whether it has a locktime or not. Within each category, we then create clusters and average the fee rates in each cluster. Then the two categories are merged, which always lift the fee rate for timelock clusters if the non-timelock clusters have a higher one. This clustering logic is unnecessary as,

  • our sweeper is NOT and will NOT manage the timelocks, inputs received here can be immediately spent, so there's no need to create the clusters.
  • the fee rate deviates from what's been specified by the caller. In short, for inputs with timelock, the actual fee rate used is higher.
  • this makes it difficult for our future fee bumper to control the fee rate

Thus, we should remove the cluster by locktime logic, and only keep the fee rate bucket logic - we still need the fee rate bucket because we want to support users to specify an inputs fee rate for compatibility.

A New Way to Specify Fee Preference

Instead of specifying fee rate or conf targets to express the urgency of the sweep, we will support specifying the fee by proportion. In details,

  • Instead of using a conf target or a fee rate, users will now specify MaxFeeRatio instead to tell the sweeper the max allowed fee it can use when sweeping this transaction.
  • MaxFeeRatio is the max value proportional to the value of the input.
  • Conf target and fee rate are estimates made by bitcoind, and our future fee bumper will only use it as a reference instead of solely relying on it.
  • By using MaxFeeRatio, inputs with different fee rates which cannot be swept together previously can now be put in the same transaction.

Note that fee rate already implicitly expresses the max fee ratio, and this implicitly makes the fee bumper implementation more difficult.

Fee Bumper

A subservice that's responsible for bumping the fee rate when the deadline is approaching. More details in #4215 and #7549.

With this new service, we'd solve the following issues,

New RPC and Configs

We'd also want to provide more config options and RPC endpoints for users to interact with the sweeper.

We may further add RemoveInput and AddInput, which requires much more work so won't happen in the near future.

Other Improvements

  • Each input should have a state that's used to track its lifecycle. We may want to persist it on disk to solve the restart issue found in [bug]: Channel is stuck in pending state. #8028.

  • remove the logic that we retry the failed broadcasts, instead, implement testmempoolaccept in btcd, and use this check to increase the success rate of the broadcast. Our current logic retries the inputs in a failed transaction, which may end up in an RBF situation that the sweeper is not able to handle.

  • utxonursery: delete #3688

@yyforyongyu yyforyongyu added enhancement Improvements to existing features / behaviour utxo sweeping labels Sep 28, 2023
@Roasbeef Roasbeef added brainstorming Long term ideas/discussion/requests for feedback design review labels Sep 28, 2023
@saubyk saubyk added P1 MUST be fixed or reviewed epic Issues created to track large feature development labels Oct 3, 2023
@saubyk saubyk changed the title sweeper: improve sweeper with defined responsibility and functionalities [epic] sweeper: improve sweeper with defined responsibility and functionalities Oct 3, 2023
@morehouse
Copy link
Collaborator

Do we have a target release for this functionality? Maybe it is worth separating it into smaller components and setting target releases individually?

The changes to fee preference and fee bumping logic seem especially valuable to protect funds when surprising things are happening in the mempool or with miners -- e.g., rapidly changing fee rates, mempool partitioning, replacement cycling attacks, miner transaction censorship. Our best defense in these situations is to increase fees aggressively as the deadline approaches, and we can't always count on our bitcoin node's recommended fee rate to get our transaction confirmed.

@yyforyongyu yyforyongyu self-assigned this Nov 14, 2023
@ziggie1984
Copy link
Collaborator

I think it would also be cool to have like an RPC call which shows the active sweeper setting, like for example the batchwindowduration or the max fee rates ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
brainstorming Long term ideas/discussion/requests for feedback design review enhancement Improvements to existing features / behaviour epic Issues created to track large feature development P1 MUST be fixed or reviewed utxo sweeping
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants