WIP: make sweeper deadline aware #7549

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

sweep/fee_bump.go

+
+// validateStrategy checks that a given strategy is defined.
+func validateStrategy(s Strategy) error {
+	if s < strategySentinel {


nit: could use switch statement?

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

sweep/fee_bump.go

+	case StrategyDeadlineAware:
+		deadlineHeight := f.deadline
+
+		target := deadlineHeight - currentHeight


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

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

sweep/fee_bump.go

+
+		// If we are already behind deadline, fallback to conf target 2
+		// because that's minimal allowed value.
+		if target < 2 {


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

sweep/fee_bump.go

+}
+
+// decideConfTarget calculates a new conf target based on the strategy used.
+func (f *feeBumpParam) decideConfTarget(currentHeight uint32) uint32 {


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

sweep/sweeper.go

@@ -677,6 +695,16 @@ func (s *UtxoSweeper) collector(blockEpochs <-chan *chainntnfs.BlockEpoch) {
 			}
 			log.Tracef("input %v scheduled", outpoint)

+			if input.params.Strategy == StrategyDeadlineAware {
+				err := s.handleDeadlineAwareInput(


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

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

sweep/sweeper.go

+	params := &feeBumpParam{
+		strategy:          StrategyDeadlineAware,
+		initialConfTarget: initialConfTarget,
+		deadline:          uint32(currentHeight) + initialConfTarget,


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

sweep/sweeper.go

+			},
+		}
+
+		// TODO(yy): Inside a goroutine?


I think this will block?

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

sweep/sweeper.go

+			input: *op,
+			params: ParamsUpdate{
+				Fee: FeePreference{
+					ConfTarget: confTarget,


missing the strategy field?

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

sweep/sweeper.go

+				Fee: FeePreference{
+					ConfTarget: confTarget,
+				},
+				Force: true,


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

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

sweep/sweeper.go

+	return nil
+}
+
+func (s *UtxoSweeper) handleFeeBump(currentHeight uint32) {


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

sweep/log.go

+	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

sweep/sweeper.go

+
+	// 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

sweep/fee_bumper.go

+	}
+
+	// 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

sweep/fee_bumper.go

+
+	// 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

sweep/fee_bumper.go

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


requst -> request

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

sweep/fee_bumper.go

+	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

sweep/fee_bumper.go

+			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

sweep/fee_bumper.go

+}
+
+// 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

sweep/fee_bumper.go

 	default:
 		feeLog.Errorf("Unknown strategy %v", r.strategy)
 		return r.currentFeeRate
 	}
 }

+// 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: make sweeper deadline aware #7549

WIP: make sweeper deadline aware #7549

yyforyongyu commented Mar 29, 2023 •

edited

Loading

Crypt-iQ Mar 30, 2023

Crypt-iQ Mar 30, 2023

Crypt-iQ Mar 30, 2023

Crypt-iQ Mar 30, 2023

Crypt-iQ Mar 30, 2023

Crypt-iQ Mar 30, 2023

Crypt-iQ Mar 30, 2023

Crypt-iQ Mar 30, 2023

Crypt-iQ Mar 30, 2023

Crypt-iQ Mar 30, 2023

Roasbeef left a comment

Roasbeef Apr 3, 2023

Roasbeef Apr 3, 2023

Roasbeef Apr 3, 2023

Roasbeef Apr 3, 2023

Roasbeef Apr 3, 2023

Roasbeef Apr 3, 2023

Roasbeef Apr 3, 2023

Roasbeef Apr 3, 2023

Roasbeef Apr 3, 2023

Roasbeef commented Apr 22, 2024

WIP: make sweeper deadline aware #7549

WIP: make sweeper deadline aware #7549

Conversation

yyforyongyu commented Mar 29, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef 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

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

Roasbeef commented Apr 22, 2024

yyforyongyu commented Mar 29, 2023 •

edited

Loading