sweeper: lower the default max fee rate and make it configurable #7823

yyforyongyu · 2023-07-12T13:49:02Z

This PR decreases the max fee rate by 10x and makes it configurable.

This change is

docs/release-notes/release-notes-0.17.0.md

lncfg/sweeper.go

Crypt-iQ · 2023-07-21T19:05:47Z

lncfg/sweeper.go

 )

 //nolint:lll
 type Sweeper struct {
-	BatchWindowDuration time.Duration `long:"batchwindowduration" description:"Duration of the sweep batch window. The sweep is held back during the batch window to allow more inputs to be added and thereby lower the fee per input."`
+	BatchWindowDuration time.Duration          `long:"batchwindowduration" description:"Duration of the sweep batch window. The sweep is held back during the batch window to allow more inputs to be added and thereby lower the fee per input."`
+	MaxFeeRate          chainfee.SatPerKWeight `long:"maxfeerate" description:"Maximum fee rate in sat/kw that the sweeper is allowed to pay."`


can this be in sat/vb? that way users don't need to do the conversion to sat/kw?

yeah sat/vb can be nice, tho one day we may want it to be decimal or use sat/kvb instead in case bitcoin goes to 1m.

sample-lnd.conf

feelancer21 · 2023-07-24T21:17:18Z

Thank you for the pull request. I agree with giving more control over the feerates for sweeps. However, I am cautious about the proposed minimum limit of 100 sat/vb. I would prefer to completely remove the limit and instead provide a clear warning message in the sample-lnd.conf and the help section, emphasizing that setting too low of a value could risk the funds. All Noderunners who still decide to modify the option despite the warning will presumably be aware of the potential risks.

Even with 100 sat/vb, there is no guarantee that all funds will be safely swept. In early May, some cases required as much as 500 sat/vb.

yyforyongyu

All Noderunners who still decide to modify the option despite the warning will presumably be aware of the potential risks.

I'm getting very different feedback here. Tho I'm leaning towards making it purely determined by node runners, some sanity check here is necessary - you will only hit this number if the mempool is very congested, in which case you probably wanna have a higher fee rate. If we decide to remove the limits, I'd say we should remove it gradually. Plus in my mind, a better config for the sweeper is to limit by fees paid proportional to the input value, not fee rate, which is something I'm working on atm.

Even with 100 sat/vb, there is no guarantee that all funds will be safely swept. In early May, some cases required as much as 500 sat/vb.

Yeah that's why we have a default of 1000.

yyforyongyu · 2023-07-26T08:14:11Z

lncfg/sweeper.go

 )

 //nolint:lll
 type Sweeper struct {
-	BatchWindowDuration time.Duration `long:"batchwindowduration" description:"Duration of the sweep batch window. The sweep is held back during the batch window to allow more inputs to be added and thereby lower the fee per input."`
+	BatchWindowDuration time.Duration          `long:"batchwindowduration" description:"Duration of the sweep batch window. The sweep is held back during the batch window to allow more inputs to be added and thereby lower the fee per input."`
+	MaxFeeRate          chainfee.SatPerKWeight `long:"maxfeerate" description:"Maximum fee rate in sat/kw that the sweeper is allowed to pay."`


yeah sat/vb can be nice, tho one day we may want it to be decimal or use sat/kvb instead in case bitcoin goes to 1m.

lncfg/sweeper.go

sample-lnd.conf

yyforyongyu · 2023-08-11T09:25:33Z

cc @ziggie1984

ziggie1984

TACK. I think it is definitely worth decreasing the max fee rate giving node runners a piece of mind so that their sweep fees will not skyrock.
This fee limit relates to the effective feerate currently meaning that when we CPFP a transaction fees can even go higher and here I would suggest to limit the fee rate to the maximum fee rate of a transaction. CPFPing big parents (big commitment transaction) could overshoot this limit highly and I think users are not aware of this wdyt ?

I am in favour of setting a minimum and allow for a gradual change. Different inputs have different priorities in terms of getting confirmed so I highly recommend to add a limit so that node runners do not shoot themselves into the foot.

Reviewed 8 of 11 files at r1.
Reviewable status: 8 of 11 files reviewed, 8 unresolved discussions (waiting on @Crypt-iQ, @feelancer21, and @yyforyongyu)

sample-lnd.conf line 1785 at r1 (raw file):

; sweeper.batchwindowduration=30s

; The max fee rate in sat/vb can be used when sweeping funds. Setting this

... in sat/vb which can be ....

sample-lnd.conf line 1787 at r1 (raw file):

; The max fee rate in sat/vb can be used when sweeping funds. Setting this
; value too low can result in transactions not being confirmed in time, causing
; HTLCs to expire.

... HTLCs to expire hence potentially losing funds.

docs/release-notes/release-notes-0.17.0.md line 133 at r1 (raw file):

  time, which could result in fund loss.
  Please note that the actual fee rate to be used is deteremined by the fee
  estimator used(for instance `bitcoind`), and this value is a cap on the max

I think it is worth pointing out, that the fee estimation is not really capped at the value 1000 but rather fee estimation fails completely when overestimating ?

sweep/sweeper.go line 500 at r1 (raw file):

			"rate %v, minimum is %v", feeRate, s.relayFeeRate)
	}
	if feeRate > s.cfg.MaxFeeRate.FeePerKWeight() {

What do you think about capping the value here instead of returning an error, rather publish a transaction with the capped limit than not publishing at all?

yyforyongyu

Reviewable status: 8 of 11 files reviewed, 8 unresolved discussions (waiting on @Crypt-iQ, @feelancer21, and @ziggie1984)

sample-lnd.conf line 1785 at r1 (raw file):

Previously, ziggie1984 (ziggieXXX) wrote…

... in sat/vb which can be ....

Done.

sample-lnd.conf line 1787 at r1 (raw file):

Previously, ziggie1984 (ziggieXXX) wrote…

... HTLCs to expire hence potentially losing funds.

Done.

docs/release-notes/release-notes-0.17.0.md line 133 at r1 (raw file):

Previously, ziggie1984 (ziggieXXX) wrote…

I think it is worth pointing out, that the fee estimation is not really capped at the value 1000 but rather fee estimation fails completely when overestimating ?

What do you mean fee estimation fails completely when overestimating ??

sweep/sweeper.go line 500 at r1 (raw file):

Previously, ziggie1984 (ziggieXXX) wrote…

What do you think about capping the value here instead of returning an error, rather publish a transaction with the capped limit than not publishing at all?

Yeah I like it, but let's do it in another PR to limit the scope.

ziggie1984

Reviewable status: 3 of 11 files reviewed, 8 unresolved discussions (waiting on @Crypt-iQ, @feelancer21, and @yyforyongyu)

docs/release-notes/release-notes-0.17.0.md line 133 at r1 (raw file):

Previously, yyforyongyu (Yong) wrote…

What do you mean fee estimation fails completely when overestimating ??

meaning that we fail the fee-estimation when we overestimate rather than cap on the max, for me cap on the max means we cap it and return, relates to the comment where I suggest capping the value to the max fee instead of failing.

yyforyongyu

Reviewable status: 3 of 11 files reviewed, 8 unresolved discussions (waiting on @Crypt-iQ, @feelancer21, and @ziggie1984)

docs/release-notes/release-notes-0.17.0.md line 133 at r1 (raw file):

Previously, ziggie1984 (ziggieXXX) wrote…

meaning that we fail the fee-estimation when we overestimate rather than cap on the max, for me cap on the max means we cap it and return, relates to the comment where I suggest capping the value to the max fee instead of failing.

gotya, yeah make sense. Added a commit to cap it instead as I think it'd be confusing if we change the behavior again and update the release notes. Will see the build results, hopefully it's a small change.

yyforyongyu

I don't CPFP should or would go over the limit? We have a check here, https://github.com/lightningnetwork/lnd/blob/master/sweep/sweeper.go#L995

So we'd do CPFP only when the newly estimated fee rate exceeds the parent fee rate. But I do remember some issues related to the parent fee calculation, but yeah we shouldn't allow it to happen.

Reviewable status: 3 of 11 files reviewed, 8 unresolved discussions (waiting on @Crypt-iQ, @feelancer21, and @ziggie1984)

ziggie1984

Really cool PR, I like that we also cap now the child transaction as well :)

Apart from the logging of feerates instead of absolute fees this PR ready to hit the road

Reviewed 2 of 11 files at r1, 1 of 11 files at r4.
Reviewable status: 6 of 20 files reviewed, 7 unresolved discussions (waiting on @Crypt-iQ, @feelancer21, and @yyforyongyu)

-- commits line 26 at r4:
Nit s/the/when/g

sweep/weight_estimator.go line 136 at r4 (raw file):

	maxFee := w.maxFeeRate.FeeForWeight(childWeight)
	if fee > maxFee {
		log.Warnf("Estimated fee %v exceeds max allowed fee %v, using "+

can we log the feer rates instead of the absolute fee values or maybe both ?

sweep/weight_estimator_test.go line 107 at r4 (raw file):

	require.NoError(t, w.add(&childInput))

	// The child weight should be 322 bytes.

weight units rather than bytes ?

ziggie1984

LGTM

sweep/sweeper.go

itest/lnd_onchain_test.go

sweep/sweeper.go

lncfg/sweeper.go

Crypt-iQ · 2023-09-01T17:22:49Z

sweep/weight_estimator.go

+	}
+
+	// Clamp the fee to the max fee rate.
+	maxFee := w.maxFeeRate.FeeForWeight(childWeight)


Not for this PR: I think we should be using a different function in the non-channel code because FeeForWeight rounds down and is necessary to agree on a fee with your peer, but in the sweeper and other places you can round up

Crypt-iQ · 2023-09-05T15:32:06Z

sweep/tx_input_set.go

@@ -118,11 +121,12 @@ type txInputSet struct {
 }

 // newTxInputSet constructs a new, empty input set.
-func newTxInputSet(wallet Wallet, feePerKW chainfee.SatPerKWeight,
+func newTxInputSet(wallet Wallet, feePerKW, maxFeeRrate chainfee.SatPerKWeight,


nit: maxFeeRate

lightninglabs-deploy · 2023-09-12T15:49:52Z

@yyforyongyu, remember to re-request review from reviewers when ready

This commit changes the DefaultMaxFeeRate from 10,120 sat/vb to 1,000 sat/vb.

Crypt-iQ

one nit

sweep/weight_estimator.go

ziggie1984

Reviewed 1 of 17 files at r6, all commit messages.
Reviewable status: 4 of 21 files reviewed, 11 unresolved discussions (waiting on @Crypt-iQ, @feelancer21, and @yyforyongyu)

docs/release-notes/release-notes-0.18.0.md line 32 at r8 (raw file):

  time, which could result in fund loss.
  Please note that the actual fee rate to be used is deteremined by the fee
  estimator used(for instance `bitcoind`), and this value is a cap on the max

Nit: Space

ziggie1984

LGTM 🥨

guggero

Just two inputs from my end, other than that looks good!

lncfg/sweeper.go

This commit caps the estimated fee rate at the configged max fee rate instead of returning an error.

This commit updates the `fee()` method in `weightEstimator` to make sure when doing CPFP we are not exceeding the max allowed fee rate. In order to use the max fee rate, we need to modify several methods to pass the configured value to the estimator.

alexbosworth · 2023-09-24T23:32:59Z

docs/release-notes/release-notes-0.18.0.md

+  funds. The default value is 1000 sat/vb. Setting this value below 100 sat/vb
+  is not allowed, as low fee rate can cause transactions not confirming in
+  time, which could result in fund loss.
+  Please note that the actual fee rate to be used is deteremined by the fee


deteremined -> determined

yyforyongyu self-assigned this Jul 12, 2023

yyforyongyu added fees Related to the fees paid for transactions (both LN and funding/commitment transactions) utxo sweeping labels Jul 12, 2023

alexbosworth reviewed Jul 17, 2023

View reviewed changes

docs/release-notes/release-notes-0.17.0.md Outdated Show resolved Hide resolved

yyforyongyu force-pushed the lower-max-fee-rate branch from 45326c7 to a28c696 Compare July 17, 2023 11:42

Crypt-iQ reviewed Jul 21, 2023

View reviewed changes

lncfg/sweeper.go Show resolved Hide resolved

Crypt-iQ reviewed Jul 21, 2023

View reviewed changes

feelancer21 reviewed Jul 24, 2023

View reviewed changes

sample-lnd.conf Outdated Show resolved Hide resolved

yyforyongyu commented Jul 26, 2023

View reviewed changes

yyforyongyu force-pushed the lower-max-fee-rate branch 2 times, most recently from e7a1418 to 804ed98 Compare July 26, 2023 09:12

yyforyongyu requested review from feelancer21 and Crypt-iQ July 26, 2023 09:14

yyforyongyu force-pushed the lower-max-fee-rate branch from 804ed98 to a87a9fd Compare July 26, 2023 09:54

yyforyongyu force-pushed the lower-max-fee-rate branch from a87a9fd to 32df3fb Compare August 8, 2023 09:35

yyforyongyu requested review from Roasbeef and removed request for feelancer21 and Roasbeef August 10, 2023 17:41

ziggie1984 reviewed Aug 11, 2023

View reviewed changes

yyforyongyu commented Aug 18, 2023

View reviewed changes

yyforyongyu force-pushed the lower-max-fee-rate branch from 32df3fb to 2465732 Compare August 18, 2023 08:39

ziggie1984 reviewed Aug 18, 2023

View reviewed changes

yyforyongyu force-pushed the lower-max-fee-rate branch from 2465732 to 5c9b968 Compare August 18, 2023 10:38

yyforyongyu commented Aug 18, 2023

View reviewed changes

yyforyongyu force-pushed the lower-max-fee-rate branch from 29a3c2d to 9fc40ed Compare August 22, 2023 04:42

ziggie1984 reviewed Aug 22, 2023

View reviewed changes

yyforyongyu force-pushed the lower-max-fee-rate branch from 9fc40ed to 4d75ac2 Compare August 22, 2023 10:31

ziggie1984 approved these changes Aug 28, 2023

View reviewed changes

saubyk added this to the v0.18.0 milestone Aug 29, 2023

yyforyongyu changed the base branch from master to 0-18-staging September 1, 2023 09:20

yyforyongyu force-pushed the lower-max-fee-rate branch from 5b4644c to 78da856 Compare September 1, 2023 09:22

Crypt-iQ reviewed Sep 5, 2023

View reviewed changes

yyforyongyu force-pushed the 0-18-staging branch from 7a34cac to e895b48 Compare September 9, 2023 23:17

yyforyongyu added 3 commits September 13, 2023 11:32

sweep: lower the max allowed fee rate for sweeping inputs

6312fa1

This commit changes the DefaultMaxFeeRate from 10,120 sat/vb to 1,000 sat/vb.

chainfee: add new unit SatPerVByte

62eb60a

sweep+itest: change MaxFeeRate to use SatPerVbyte

e825ed2

yyforyongyu force-pushed the lower-max-fee-rate branch from 78da856 to 89ec319 Compare September 13, 2023 03:56

yyforyongyu requested a review from Crypt-iQ September 13, 2023 03:58

Crypt-iQ reviewed Sep 15, 2023

View reviewed changes

sweep/weight_estimator.go Show resolved Hide resolved

yyforyongyu force-pushed the lower-max-fee-rate branch from 89ec319 to 8f6d531 Compare September 15, 2023 20:33

Crypt-iQ approved these changes Sep 18, 2023

View reviewed changes

yyforyongyu requested a review from ziggie1984 September 19, 2023 09:00

ziggie1984 reviewed Sep 19, 2023

View reviewed changes

ziggie1984 approved these changes Sep 19, 2023

View reviewed changes

guggero reviewed Sep 19, 2023

View reviewed changes

lncfg/sweeper.go Outdated Show resolved Hide resolved

lncfg/sweeper.go Outdated Show resolved Hide resolved

yyforyongyu added 5 commits September 20, 2023 15:36

multi: make max fee rate configurable

be0e446

docs: update release note for maxfeerate

19fc6fd

sweep: cap to max fee rate instead of failing

f12dce6

This commit caps the estimated fee rate at the configged max fee rate instead of returning an error.

rpcserver+sweep: fix linter re unused params

4622ea9

yyforyongyu force-pushed the lower-max-fee-rate branch from 8f6d531 to 4622ea9 Compare September 20, 2023 07:36

guggero approved these changes Sep 20, 2023

View reviewed changes

guggero merged commit 27aab00 into lightningnetwork:0-18-staging Sep 20, 2023
23 of 25 checks passed

yyforyongyu deleted the lower-max-fee-rate branch September 20, 2023 11:54

alexbosworth reviewed Sep 24, 2023

View reviewed changes

yyforyongyu mentioned this pull request Sep 28, 2023

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sweeper: lower the default max fee rate and make it configurable #7823

sweeper: lower the default max fee rate and make it configurable #7823

yyforyongyu commented Jul 12, 2023 •

edited

Loading

Crypt-iQ Jul 21, 2023

yyforyongyu Jul 26, 2023

feelancer21 commented Jul 24, 2023

yyforyongyu left a comment

yyforyongyu Jul 26, 2023

yyforyongyu commented Aug 11, 2023

ziggie1984 left a comment

yyforyongyu left a comment

ziggie1984 left a comment

yyforyongyu left a comment

yyforyongyu left a comment

ziggie1984 left a comment

ziggie1984 left a comment

Crypt-iQ Sep 1, 2023

Crypt-iQ Sep 5, 2023

yyforyongyu Sep 13, 2023

lightninglabs-deploy commented Sep 12, 2023

Crypt-iQ left a comment

ziggie1984 left a comment

ziggie1984 left a comment

guggero left a comment

alexbosworth Sep 24, 2023

sweeper: lower the default max fee rate and make it configurable #7823

sweeper: lower the default max fee rate and make it configurable #7823

Conversation

yyforyongyu commented Jul 12, 2023 • edited Loading

Crypt-iQ Jul 21, 2023

Choose a reason for hiding this comment

yyforyongyu Jul 26, 2023

Choose a reason for hiding this comment

feelancer21 commented Jul 24, 2023

yyforyongyu left a comment

Choose a reason for hiding this comment

yyforyongyu Jul 26, 2023

Choose a reason for hiding this comment

yyforyongyu commented Aug 11, 2023

ziggie1984 left a comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

ziggie1984 left a comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

ziggie1984 left a comment

Choose a reason for hiding this comment

ziggie1984 left a comment

Choose a reason for hiding this comment

Crypt-iQ Sep 1, 2023

Choose a reason for hiding this comment

Crypt-iQ Sep 5, 2023

Choose a reason for hiding this comment

yyforyongyu Sep 13, 2023

Choose a reason for hiding this comment

lightninglabs-deploy commented Sep 12, 2023

Crypt-iQ left a comment

Choose a reason for hiding this comment

ziggie1984 left a comment

Choose a reason for hiding this comment

ziggie1984 left a comment

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

alexbosworth Sep 24, 2023

Choose a reason for hiding this comment

yyforyongyu commented Jul 12, 2023 •

edited

Loading