loopout: re-target sweep's feerate every block #838

starius · 2024-10-19T04:05:36Z

Add type loopOutSweepFeerateProvider which determines confTarget based on distance to swap expiration, then determines feerate and fee using. Fee rate is plugged into sweepbatcher using WithCustomFeeRate. Option WithPublishDelay is used to make sure fee-rate is updated by loopout.go before the value is used by sweepbatcher.

When determining confTarget, there are few adjustments over raw distance to cltv_expiry:

make sure confTarget is positive (if the swap has expired, raw distance is negative)
If confTarget is less than or equal to DefaultSweepConfTargetDelta (10), cap it with urgentSweepConfTarget and apply fee factor (1.1x).

Also, if feerate is less than floor (1 sat/vbyte), then the floor is used.

DefaultSweepConfTargetDelta was decreased from 18 to 10.

Every block 100 sats/kw fee bump is disabled. Sweepbatcher re-targets feerate every block according to current mempool conditions and the number of blocks until expiry.

Added tests for loopOutSweepFeerateProvider simulating various conditions.

Pull Request Checklist

Update release_notes.md if your PR contains major features, breaking changes or bugfixes

hieblmi

First pass done. I left a question regarding the condition in SweepFeeRate.

sweeper/sweep_fee_provider.go

sweeper/sweep_fee_provider_test.go

hieblmi · 2024-10-21T09:14:43Z

sweeper/sweep_fee_provider.go

+		// UrgentSweepConfTarget instead of confTarget or uses the
+		// proportional approach to fees, not relying on confTarget.
+	} else {
+		// If the swap size is below a significant economical size,


don't we assume here that amt >= p.HighPrioSwapAmount but we could land here with the other condition?

+1, the comments needs a little bit of update.

Updated the comment:

} else { // If the sweep size is below a significant economical size or // the sweep expires in <=UrgentSweepConfTarget blocks, we'll // obtain the fee we should use for the sweep based on our // current best weight estimates as well as the confirmation // target. fee, feeRate, weight, err = p.Sweeper.GetSweepFeeDetails( ctx, addInputToEstimator, destAddr, confTarget, label, )

I was wrong in the sweep expires in <=UrgentSweepConfTarget blocks part. It checks confTarget, not the raw distance. ConfTarget is UrgentSweepConfTarget, when distance is DefaultSweepConfTargetDelta (which is larger).

I reworded the comment:

} else { // If the sweep size is below a significant economical size or // the sweep is not expiring soon, we'll obtain the fee we // should use for the sweep based on our current best weight // estimates as well as the confirmation target.

This code was removed from the PR.

loopout_feerate.go

bhandras

Looks good, just a few comments.

bhandras · 2024-10-21T12:57:22Z

sweeper/sweep_fee_provider.go

+	// HighPrioSwapAmount is a threshold at which a swap is prioritized to
+	// be swept with a fee that is related to its size. The fee amount is
+	// determined by HighPrioFeePPM.
+	HighPrioSwapAmount btcutil.Amount


nit: given that this is now more general than just per swap fee we could rename this variable to HighPrioSweepAmount and generally "swap" to "sweep" in this file.

Replaced "swap" with "sweep" everywhere in the package.

sweeper/sweep_fee_provider.go

bhandras · 2024-10-21T13:01:13Z

sweeper/sweep_fee_provider.go

+		// UrgentSweepConfTarget instead of confTarget or uses the
+		// proportional approach to fees, not relying on confTarget.
+	} else {
+		// If the swap size is below a significant economical size,


+1, the comments needs a little bit of update.

sweeper/sweep_fee_provider.go

sweeper/sweep_fee_provider_test.go

bhandras · 2024-10-21T14:38:11Z

loopout.go

+	// greater or equal the size of highPrioSwapAmount to expedite the
+	// availability of on-chain funds to generate revenue on loop-out
+	// operations.
+	highPrioFeePPM = 50


While this default is sane, I'd still recommend it (as well as high prio swap amount) to be configurable.

+1

I'll do it. Also need to make sure high prio fee is not lower than a regular fee (which is more likely to happen given the "high prio" parameters will be configurable).

I added a flag in SweepFeeProvider which disabled high prio logic altogether. It is enabled in Loop client. So we don't need options to configure these parameters.

sweep/log.go

bhandras · 2024-10-22T18:30:32Z

sweeper/log.go

+var log btclog.Logger
+
+// The default amount of logging is none.
+func init() {


nit: similarly, add the sub logger in loopd/log.go.

I removed sweeper package from the PR, so this logger doesn't exist anymore.

bhandras · 2024-10-22T20:11:39Z

sweeper/sweep_fee_provider.go

+
+	// DisableHighPrioLogic disables special treatment of sweeps larger than
+	// or equal to HighPrioSweepAmount.
+	DisableHighPrioLogic bool


I think he high prio part could be completely removed.

Done. Now all the confTarget and feeRate logic is in method GetConfTargetAndFeeRate.

starius · 2024-10-22T21:57:01Z

I removed SweepFeeProvider and moved parts of the logic to loopOutSweepFeerateProvider (method GetConfTargetAndFeeRate).

Decided to keep urgent logic and floor logic. High prio logic is removed.

New description of the change looks like this:

Add type loopOutSweepFeerateProvider which determines confTarget based on distance to swap expiration, then determines feerate and fee using. Fee rate is plugged into sweepbatcher using WithCustomFeeRate.

When determining confTarget, there are few adjustments over raw distance to cltv_expiry:

make sure confTarget is positive (if the swap has expired, raw distance is negative)
If confTarget is less than or equal to DefaultSweepConfTargetDelta (10), cap it with urgentSweepConfTarget and apply fee factor (1.1x).

Also, if feerate is less than floor (1 sat/vbyte), then the floor is used.

DefaultSweepConfTargetDelta was decreased from 18 to 10.

Every block 100 sats/kw fee bump is disabled. Sweepbatcher re-targets feerate every block according to current mempool conditions and the number of blocks until expiry.

Added tests for loopOutSweepFeerateProvider simulating various conditions.

bhandras

LGTM 🥇

hieblmi

I like the concise calculation of the confTarget. LGTM 💾

hieblmi · 2024-10-24T14:35:44Z

loopout.go

-	//
-	// TODO(wilmer): tune?
-	DefaultSweepConfTargetDelta = DefaultSweepConfTarget * 2
+	// swap's expiration height at which we begin to cap sweep confirmation


nit: ... at which be begin to cap the sweep...

Fixed. Thanks!

hieblmi · 2024-10-24T14:36:19Z

loopout.go

+	// blocks to expire).
+	urgentSweepConfTarget = 3
+
+	// urgentSweepConfTargetFactor is the factor we apply to fee-rate of


nit: feerate

hieblmi · 2024-10-24T14:39:11Z

loopout_feerate.go

+		// Assume the server is cooperative and we produce keyspend.
+		addInputToEstimator = func(e *input.TxWeightEstimator) error {
+			e.AddTaprootKeySpendInput(txscript.SigHashDefault)
+			return nil


nit: nl before return for readability

hieblmi · 2024-10-24T14:43:14Z

loopout_feerate.go

+	var addInputToEstimator func(e *input.TxWeightEstimator) error
+	if canKeyspend {
+		// Assume the server is cooperative and we produce keyspend.
+		addInputToEstimator = func(e *input.TxWeightEstimator) error {


should we later add htlc.AddTaprootKeySpendInput() in a separate PR?

Looks good to me! I would call it htlc.AddCooperativeSuccessToEstimator to be more generic.

Opened an issue: https://github.com/lightninglabs/nautilus/issues/1210

hieblmi · 2024-10-24T14:48:15Z

loopout_feerate.go

+		// increase it.
+		newConfTarget := int32(urgentSweepConfTarget)
+		if confTarget < newConfTarget {
+			newConfTarget = confTarget


hieblmi · 2024-10-24T14:49:11Z

loopout_feerate.go

+		log.Infof("Got too low fee rate for swap %x: %v. Increasing "+
+			"it to %v.", swapHash[:6], feeRate, minFeeRate)
+
+		feeRate = minFeeRate


lightninglabs-deploy · 2024-11-07T21:56:15Z

@sputn1ck: review reminder

starius · 2024-11-11T04:34:43Z

I tested the PR in regtest simulating changing of feerate environment.

It works!

But I found a bug in the approved version of the PR: loopout.go calls AddSweep after sweepbatcher tries to sweep. The fee rate is updated by sweepbatcher reacting on AddSweep call, so this results in the fee rate being outdated by one block:

2024-11-11 00:32:29.156 [DBG] SWEEP: [Batch 1] received block 163                                                                                                                                                  
2024-11-11 00:32:29.157 [DBG] RSRV: Received block 163                                                                                                                                                             
2024-11-11 00:32:29.687 [INF] SWEEP: [Batch 1] attempting to publish coop tx=4ea6cde3857e0bedee5098995903d26c578398d1777d99332bdda0d449053343 with feerate=500 sat/kw, weight=444 wu, feeForWeight=0.00000222 BTC, 
fee=0.00000222 BTC, sweeps=1, destAddr=bcrt1phptyurxht2p8masl8vqflgv4xfzg0n72cdax62yl8r7c95q7ngtsgkxgpj
2024-11-11 00:32:29.687 [DBG] SWEEP: [Batch 1] serialized coop sweep: 0200000000010113ddb9206aadd5be8c08da7edf6e3767dd2977f2ffda39c9551ecce3198ceaa000000000000000000001b2cf030000000000225120b8564e0cd75a827df61f3
b009fa195324487cfcac37a6d289f38fd82d01e9a170140803e6a6c415dcc89964580c2188288954ba9daaa9d49d713c8b32b84e7c35604baa621812ef504b83b82c7dc3ea3cc46b3e072be5d1becb04853186393e29443a3000000
2024-11-11 00:32:30.064 [INF] SWEEP: [Batch 1] co-op publish error: rpc error: code = Unknown desc = insufficient fee
2024-11-11 00:32:30.212 [DBG] SWP: Estimations for a tx (label=loopout-sweep-eb60dd506db6): weight=444 wu, fee=0.00002220 BTC, feerate=5000 sat/kw, sweepConfTarget=22.
2024-11-11 00:32:30.212 [DBG] LOOP: Estimated for swap eb60dd506db6: feeRate=5000 sat/kw, confTarget=22.
2024-11-11 00:32:30.212 [INF] SWEEP: Batcher handling sweep eb60dd506db6, completed=false

In this example, the fee rate spiked from 500 sat/kw to 5000 sat/kw, but the old value (500 sat/kw) was used when making the sweep transaction.

I fixed this by adding sweepbatcher.WithPublishDelay option to sweepbatcher. It sets the delay to 2 seconds. The delay in loopout.go before calling AddSweep is 1 second. Now the order is correct:

2024-11-11 01:10:40.613 [DBG] RSRV: Received block 176
2024-11-11 01:10:41.618 [DBG] SWP: Estimations for a tx (label=loopout-sweep-f0b718b1bb25): weight=444 wu, fee=0.00002220 BTC, feerate=5000 sat/kw, sweepConfTarget=48.
2024-11-11 01:10:41.618 [DBG] LOOP: Estimated for swap f0b718b1bb25: feeRate=5000 sat/kw, confTarget=48.
2024-11-11 01:10:41.619 [INF] SWEEP: Batcher handling sweep f0b718b1bb25, completed=false
2024-11-11 01:10:42.644 [INF] SWEEP: [Batch 2] attempting to publish coop tx=8870799108c578670426cb2dcd21e4ae2eda6d01642f1f37c85bcb95bc7a5c97 with feerate=5000 sat/kw, weight=444 wu, feeForWeight=0.00002220 BTC, fee=0.00002220 BTC, sweeps=1, destAddr=bcrt1p22v8sjqkvkn9wl94p32cfxemsu4cnj2kntxzcjpa5jwxnw5ay5lsq4twtf
2024-11-11 01:10:42.644 [DBG] SWEEP: [Batch 2] serialized coop sweep: 020000000001016e42dde2cfe3dbb077d887a58682df43776d65af1db9316164f06e3fabc3160a00000000000000000001e4c7030000000000225120529878481665a6577cb50c55849b3b872b89c9569acc2c483da49c69ba9d253f01401fd68c3d194d3770d4da71cf4ff266a12adeaa228cf2373e731a7b55f07d1c2294a5fcb2de81e2c67b58408bf8bb6d1c8453734c471d918bfeb5c70cfa4c17fcb0000000
2024-11-11 01:10:42.940 [INF] SWEEP: [Batch 2] co-op publish error: rpc error: code = Unknown desc = insufficient fee

Fee estimation (and possible update) now happens before the sweep transaction creation.

I pushed the fix in a separate commit. I'll squash it with the previous commit before merging.

hieblmi

Thanks for the investigation! LGTM.

The first result of function sweepConfTarget was not used. The function was renamed to canSweep to better reflect what it does.

There are some numeric constants that used to be defined as vars complicating their usage. They were turned into constants: MinLoopOutPreimageRevealDelta, DefaultSweepConfTarget, DefaultHtlcConfTarget, DefaultSweepConfTargetDelta. Also make liquidity.defaultHtlcConfTarget a constant, not a var.

Added argument 'label' to GetSweepFee and GetSweepFeeDetails. It is logged together with expected weight, fee and feerate.

Add type loopOutSweepFeerateProvider which determines confTarget based on distance to swap expiration, then determines feerate and fee using. Fee rate is plugged into sweepbatcher using WithCustomFeeRate. Option WithPublishDelay is used to make sure fee-rate is updated by loopout.go before the value is used by sweepbatcher. When determining confTarget, there are few adjustments over raw distance to cltv_expiry: - make sure confTarget is positive (if the swap has expired, raw distance is negative) - If confTarget is less than or equal to DefaultSweepConfTargetDelta (10), cap it with urgentSweepConfTarget and apply fee factor (1.1x). Also, if feerate is less than floor (1 sat/vbyte), then the floor is used. DefaultSweepConfTargetDelta was decreased from 18 to 10. Every block 100 sats/kw fee bump is disabled. Sweepbatcher re-targets feerate every block according to current mempool conditions and the number of blocks until expiry. Added tests for loopOutSweepFeerateProvider simulating various conditions.

starius requested review from bhandras, hieblmi and sputn1ck October 19, 2024 04:05

starius force-pushed the loopout-retarget-feerate-ever-block branch from 292f1d6 to 6a58e56 Compare October 19, 2024 04:13

starius marked this pull request as ready for review October 19, 2024 04:14

hieblmi reviewed Oct 21, 2024

View reviewed changes

bhandras reviewed Oct 21, 2024

View reviewed changes

starius force-pushed the loopout-retarget-feerate-ever-block branch 3 times, most recently from 0f153a7 to 27efdad Compare October 22, 2024 17:56

starius requested review from bhandras and hieblmi October 22, 2024 18:51

bhandras reviewed Oct 22, 2024

View reviewed changes

starius force-pushed the loopout-retarget-feerate-ever-block branch from 27efdad to 83afe80 Compare October 22, 2024 21:52

starius force-pushed the loopout-retarget-feerate-ever-block branch 2 times, most recently from 739fef6 to 2e571bf Compare October 22, 2024 22:05

starius requested a review from bhandras October 22, 2024 22:07

bhandras approved these changes Oct 24, 2024

View reviewed changes

levmi assigned starius Oct 24, 2024

hieblmi approved these changes Oct 24, 2024

View reviewed changes

starius force-pushed the loopout-retarget-feerate-ever-block branch from 2e571bf to a366205 Compare October 24, 2024 19:46

starius requested review from bhandras and hieblmi November 11, 2024 04:34

hieblmi approved these changes Nov 11, 2024

View reviewed changes

starius added 3 commits November 11, 2024 20:35

loopout: remove dead code

6c62ded

The first result of function sweepConfTarget was not used. The function was renamed to canSweep to better reflect what it does.

sweep: log tx label in GetSweepFee methods

697e8d6

Added argument 'label' to GetSweepFee and GetSweepFeeDetails. It is logged together with expected weight, fee and feerate.

starius force-pushed the loopout-retarget-feerate-ever-block branch from 235eb5e to 2f22f96 Compare November 11, 2024 23:39

starius merged commit 0fe952a into lightninglabs:master Nov 11, 2024
4 checks passed

starius deleted the loopout-retarget-feerate-ever-block branch November 11, 2024 23:56

loopout: re-target sweep's feerate every block #838

loopout: re-target sweep's feerate every block #838

Uh oh!

Conversation

starius commented Oct 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Checklist

Uh oh!

hieblmi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bhandras 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!

Uh oh!

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!

starius commented Oct 22, 2024

Uh oh!

bhandras left a comment

Choose a reason for hiding this comment

Uh oh!

hieblmi 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!

starius commented Oct 19, 2024 •

edited

Loading