WIP: make sweeper deadline aware by yyforyongyu · Pull Request #7549 · lightningnetwork/lnd

yyforyongyu · 2023-03-29T18:44:09Z

This is a very raw PoC PR to make the sweeper deadline-aware and fixes #4215.

Depends on:

In this PR, we extend the current sweeper to perform a fee bump if the input specifies it's deadline-aware. The sweeper will then continuously fee bump the sweeping tx in every new block if it's not confirmed.

An alternative approach would be fee bumping at the callsite, similar to how anchor sweeping is handled here. This approach IMO is not as good as the current one as a) the current approach has a deeper interface on sweeper and b) we can pre-define a set of fee bumping strategies so they can be easily applied based on specific conditions.

TODO:

clean up commits and add docs
implement the equations in sweeper+contractcourt: deadline aware HTLC claims & commitment confirmation #4215
add unit test
add itest

Crypt-iQ · 2023-03-30T13:51:15Z

nit: could use switch statement?

Crypt-iQ · 2023-03-30T13:54:57Z

I think we need to watch for underflow here which would make target very large

Crypt-iQ · 2023-03-30T14:05:16Z

If target = 1, then we're not technically behind the deadline as there is still 1 more block to go before the timelock expires.

Also, why is 2 the minimal value?

Crypt-iQ · 2023-03-30T14:26:50Z

This returns a conf target, but in low-fee environments I believe the conf target can give the same feerate. We want to change the feerate up to some maximum % of the HTLC value here. I think instead this should only use a feerate rather than a conf-target. This would mean passing in a fee-rate from the timeout resolver.

Crypt-iQ · 2023-03-30T14:37:13Z

Does this also need to be called up above when the input already exists in the sweeper?

Crypt-iQ · 2023-03-30T14:48:20Z

I think the deadline should be its own parameter because I think the current usage is that call-sites usually have a deadline but pass a conf target that is unrelated to the deadline. So adding the conf target to the current height might be too aggressive in some cases

Crypt-iQ · 2023-03-30T14:53:46Z

I think this will block?

Crypt-iQ · 2023-03-30T15:01:35Z

missing the strategy field?

Crypt-iQ · 2023-03-30T15:02:10Z

think we can get rid of Force, if it's uneconomical it's ok to give up

Crypt-iQ · 2023-03-30T15:04:30Z

IMO this should only be for the deadline-aware inputs. The logic is a bit weird, but I think we can improve that later. We need to make sure that deadline-aware inputs don't stop after MaxSweepAttempts since they are going to be fee-bumping more since they are being re-submitted more in this function

This commit adds a new service, `feeBumper` along with its interface `Bumper`, to perform fee bumping over time sensitive inputs. This commit focuses on building the framework for the bumper, and the following commits will implement the actual strategies.

This commit adds a new strategy, `StrategyLinear`, to linearly increase the fee rate used for a given input when the deadline approaches.

Roasbeef

Nice work porting this over from that other codebase! Left some inline comments, more specific to the lnd context. Main thing is that I think we can break up the new abstract strategies into an interface to further encapsulate the logic and allow for composition/re-use.

Roasbeef · 2023-04-03T19:59:07Z

+	log btclog.Logger
+
+	// feeLog is used by the fee bumper.
+	feeLog btclog.Logger


Could make this a sub-package to have it get its own distinct logger, and also be useable as a dep-minimized sub-module.

Roasbeef · 2023-04-03T19:59:45Z

+
+	// Generate a pkScript to be used by the fee bumper for weight
+	// calculation.
+	pkScript, err := s.cfg.GenSweepScript()


Can we alternatively just make something that's P2TR sized? This way we don't need to extend the look ahead gap each time we restart.

Roasbeef · 2023-04-03T20:11:19Z

+	}
+
+	// Add the input.
+	if err := weightEstimate.add(m.Input); err != nil {


This doesn't seem to factor in the additional overhead of the type of inputs added to bump the transaction.

Roasbeef · 2023-04-03T20:15:13Z

+
+	// Check if the request has already been processed.
+	//
+	// TODO(yy): allow overwrite?


Or return a nil error unless we expect the caller to properly handle the returned ErrAlreadyMonitored.

Roasbeef · 2023-04-03T20:16:35Z

+		feeRateUsed = feeRate
+	}
+
+	// If the requst doesn't specify a broadcast height, use the manager's


requst -> request

Roasbeef · 2023-04-03T20:17:13Z

+	var deadlineHeight uint32
+
+	// Calculate the deadline height if the conf target is set.
+	if req.ConfTarget != NoHeightInfo {


Shouldn't this be ==? Since in this case, no conf target was specified, so we need to enforce a default conf target.

Roasbeef · 2023-04-03T20:21:10Z

+			f.monitoredInputs.Range(func(_ *wire.OutPoint,
+				r *monitoredInput) bool {
+
+				f.wg.Add(1)


I wonder if this should actually be done serially 🤔

Though with the way the sweeper works, all the goroutines will be serialized anyway, so it's nice to not have to block here so we can proceed other blocks/requests.

Roasbeef · 2023-04-03T20:22:13Z

+}
+
+// Strategy specifies how the fee bump is performed.
+type Strategy uint8


A slightly different way to represent this would be to have Strategy itself be an interface with methods for: seeing it things need to be bumped, and suggesting the proper fee rate. This would allow us to further encapsulate the logic of each strategy, and each strategy can also maintain its own state to make better decisions.

Roasbeef · 2023-04-03T20:25:15Z

 }

+// TODO(yy): cache the results?
+func (f *feeBumper) feeRateForStrategyLinear(


If we make this part of the strategy interface, then we can easily add caching there (though seems to be a premature optimization at this point imo).

Roasbeef · 2024-04-22T18:20:05Z

Superseded by #8667

saubyk linked an issue Mar 29, 2023 that may be closed by this pull request

sweeper+contractcourt: deadline aware HTLC claims & commitment confirmation #4215

Closed

3 tasks

Crypt-iQ reviewed Mar 30, 2023

View reviewed changes

yyforyongyu force-pushed the deadline-aware branch 3 times, most recently from 0be3133 to 30b61da Compare April 2, 2023 16:47

yyforyongyu added 3 commits April 3, 2023 00:58

sweep: add strategy to linearly increase fee rate

7e8043e

This commit adds a new strategy, `StrategyLinear`, to linearly increase the fee rate used for a given input when the deadline approaches.

contractcourt: use StrategyLinear to sweep timeout htlc

1308f3a

yyforyongyu force-pushed the deadline-aware branch from 30b61da to 1308f3a Compare April 2, 2023 16:58

Roasbeef requested changes Apr 3, 2023

View reviewed changes

saubyk added this to the v0.16.1 milestone Apr 10, 2023

saubyk assigned yyforyongyu Apr 10, 2023

saubyk modified the milestones: v0.16.1, v0.17.0 Apr 12, 2023

saubyk requested a review from ellemouton May 2, 2023 17:07

yyforyongyu mentioned this pull request May 4, 2023

[feature]: Conservative fee bumping for time sensitive sweeps of FCs with outgoing htlcs #7554

Closed

saubyk modified the milestones: v0.17.0, v0.17.1 Jun 15, 2023

saubyk modified the milestones: High Priority, v0.18.0 Aug 4, 2023

yyforyongyu mentioned this pull request Sep 7, 2023

sweeper: relax anchor sweeping when there's no deadline pressure #7965

Merged

yyforyongyu mentioned this pull request Sep 28, 2023

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

Closed

saubyk removed the request for review from ellemouton November 14, 2023 03:33

yyforyongyu mentioned this pull request Jan 25, 2024

[5/5]sweep: introduce budget-based deadline aware fee bumper #8424

Merged

7 tasks

saubyk removed a link to an issue Jan 25, 2024

sweeper+contractcourt: deadline aware HTLC claims & commitment confirmation #4215

Closed

3 tasks

saubyk removed this from the v0.18.0 milestone Feb 14, 2024

Roasbeef closed this Apr 22, 2024

yyforyongyu deleted the deadline-aware branch April 23, 2024 17:00

Conversation

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

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Roasbeef commented Apr 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yyforyongyu commented Mar 29, 2023 •

edited

Loading