bumpfee: add more flags as options #206

starius · 2025-02-11T14:51:23Z

Use new fields instead of deprecated

'force' -> 'immediate'
'sat_per_byte' -> 'sat_per_vbyte'

New options added: 'immediate', 'target_conf', 'budget'.

Pull Request Checklist

PR is opened against correct version branch.
Version compatibility matrix in the README and minimal required version
in lnd_services.go are updated.
Update macaroon_recipes.go if your PR adds a new method that is called
differently than the RPC method it invokes.

ziggie1984

Make LND release 18.5 min requirement.

ziggie1984 · 2025-02-11T18:27:30Z

walletkit_client.go

+// WithTargetConf is an option for setting the target_conf of BumpFee. If set,
+// the underlying fee estimator will use the target_conf to estimate the
+// starting fee rate for the fee function. The value of feeRate passed to
+// BumpFee will be ignored.


The conf-target only reflects this behaviour for the 18.5 release, before the conf target had a different meaning:

lightningnetwork/lnd#9470

I added a check that LND is at least 0.18.5 only if WithTargetConf is used. This commit can be undone after minimalCompatibleVersion is bumbed to 0.18.5.

ziggie1984 · 2025-02-11T18:28:13Z

walletkit_client.go

+
+		// We unset sat_per_vbyte, because if it is specified, it is
+		// used instead of target_conf.
+		r.SatPerVbyte = 0


I think that should not be done here, but its already done in LND that it's not allowed to set both.

I removed this code and added a check that if WithTargetConf is used, feeRate is 0. If both are specified, it is not clear, what the user wants and it is better to fail than to risk doing a wrong thing (e.g. overpay).

ziggie1984 · 2025-02-11T18:32:26Z

walletkit_client.go

+// WithBudget is an option for setting the budget of BumpFee. It is the max
+// amount in sats that can be used as the fees. Setting this value greater than
+// the input's value may result in CPFP - one or more wallet utxos will be used
+// to pay the fees specified by the budget. If not set, for new inputs, by
+// default 50% of the input's value will be treated as the budget for fee
+// bumping; for existing inputs, their current budgets will be retained.


maybe just let the descripiton of the RPC speak for itself and do not require these extensive comments ?

I copy-pasted this from https://lightning.engineering/api-docs/api/lnd/wallet-kit/bump-fee/
All parts of this description seem to be important since they define the behavior. IMHO better to keep this in the documentation as well so it is easier to access this information.

also specify that since 18.5 if the budget is set, the deadline has to be set too.

Currently there is no way to set the deadline. The field is missing in lnrpc of 0.18.4.

What happens if budget is provided without the deadline in a request against LND 0.18.5?

I was a bit confused, if you select the deadline you need to specify the budget, not the other way around.

https://github.com/lightningnetwork/lnd/blob/68105be1ebbd8cae18c74f112d85382702477d80/lnrpc/walletrpc/walletkit_server.go#L1022-L1023

yes, it would be cool if you'd open a PR.

In this version there is no deadline setting, since it is not supported by 0.18.4 anyway.
We can add the comment when we add the deadline setting (for lndclient 0.18.5+).

'force' -> 'immediate' 'sat_per_byte' -> 'sat_per_vbyte'

starius · 2025-02-15T15:02:20Z

I'll test the options manually to make sure the checks actually work.

starius · 2025-02-16T03:08:50Z

Tested. Works as expected!

ziggie1984

LGTM pending new comments.

ziggie1984 · 2025-02-16T08:50:33Z

walletkit_client.go

 		},
-	)
+		SatPerVbyte: uint64(feeRate.FeePerKVByte() / 1000),


Suggested change

SatPerVbyte: uint64(feeRate.FeePerKVByte() / 1000),

SatPerVbyte: uint64(feeRate.FeePerVByte()),

Applied. Thanks!

ziggie1984 · 2025-02-16T08:52:07Z

walletkit_client.go

+type BumpFeeOption func(*walletrpc.BumpFeeRequest)
+
+// WithImmediate is an option for enabling the immediate mode of BumpFee. The
+// sweeper will sweep this input without waiting for the next batch.


... without waiting for the next block.

Fixed. Thanks!

Does it make sense to fix the description of the field Immediate as well?
https://pkg.go.dev/github.com/lightningnetwork/[email protected]/lnrpc/walletrpc#BumpFeeRequest

Opened a PR here to fix the description: lightningnetwork/lnd#9518

ziggie1984 · 2025-02-16T08:53:27Z

walletkit_client.go

+// WithBudget is an option for setting the budget of BumpFee. It is the max
+// amount in sats that can be used as the fees. Setting this value greater than
+// the input's value may result in CPFP - one or more wallet utxos will be used
+// to pay the fees specified by the budget. If not set, for new inputs, by
+// default 50% of the input's value will be treated as the budget for fee
+// bumping; for existing inputs, their current budgets will be retained.


also specify that since 18.5 if the budget is set, the deadline has to be set too.

New options added: 'immediate', 'target_conf', 'budget'.

The conf-target only reflects this behaviour for the 0.18.5 release, before the conf target had a different meaning: lightningnetwork/lnd#9470 This commit can be undone after minimalCompatibleVersion is bumbed to 0.18.5.

ziggie1984

LGTM

guggero · 2025-02-17T16:46:42Z

Pushed a new tag with this: v0.18.4-11

starius marked this pull request as ready for review February 11, 2025 15:13

ziggie1984 suggested changes Feb 11, 2025

View reviewed changes

guggero force-pushed the lnd-18-4 branch from c39e850 to c5f2f9a Compare February 13, 2025 11:59

bumpfee: use new fields instead of deprecated ones

b07e7b7

'force' -> 'immediate' 'sat_per_byte' -> 'sat_per_vbyte'

starius force-pushed the bump-fee branch 2 times, most recently from cc6a0fa to db58130 Compare February 15, 2025 14:58

starius force-pushed the bump-fee branch from db58130 to 53c3fb7 Compare February 16, 2025 03:05

starius requested a review from ziggie1984 February 16, 2025 03:08

ziggie1984 approved these changes Feb 16, 2025

View reviewed changes

starius added 2 commits February 16, 2025 20:45

bumpfee: add more flags as options

aa0e286

New options added: 'immediate', 'target_conf', 'budget'.

bumpfee: check LND>=0.18.5 if target_conf is used

41eb830

The conf-target only reflects this behaviour for the 0.18.5 release, before the conf target had a different meaning: lightningnetwork/lnd#9470 This commit can be undone after minimalCompatibleVersion is bumbed to 0.18.5.

starius force-pushed the bump-fee branch from 53c3fb7 to 41eb830 Compare February 16, 2025 23:45

guggero requested a review from ziggie1984 February 17, 2025 16:36

ziggie1984 approved these changes Feb 17, 2025

View reviewed changes

guggero approved these changes Feb 17, 2025

View reviewed changes

guggero merged commit 3e177ef into lightninglabs:lnd-18-4 Feb 17, 2025
1 check passed

starius deleted the bump-fee branch February 17, 2025 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bumpfee: add more flags as options #206

bumpfee: add more flags as options #206

starius commented Feb 11, 2025

ziggie1984 left a comment

ziggie1984 Feb 11, 2025

starius Feb 15, 2025

ziggie1984 Feb 11, 2025

starius Feb 15, 2025

ziggie1984 Feb 11, 2025

starius Feb 15, 2025

ziggie1984 Feb 16, 2025

starius Feb 16, 2025

ziggie1984 Feb 17, 2025

ziggie1984 Feb 17, 2025

starius Feb 17, 2025

starius commented Feb 15, 2025

starius commented Feb 16, 2025

ziggie1984 left a comment

ziggie1984 Feb 16, 2025

starius Feb 16, 2025

ziggie1984 Feb 16, 2025

starius Feb 16, 2025

starius Feb 16, 2025

starius Feb 17, 2025

ziggie1984 Feb 16, 2025

ziggie1984 left a comment

guggero commented Feb 17, 2025

	SatPerVbyte: uint64(feeRate.FeePerKVByte() / 1000),
	SatPerVbyte: uint64(feeRate.FeePerVByte()),

bumpfee: add more flags as options #206

bumpfee: add more flags as options #206

Conversation

starius commented Feb 11, 2025

Pull Request Checklist

ziggie1984 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starius commented Feb 15, 2025

starius commented Feb 16, 2025

ziggie1984 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

ziggie1984 left a comment

Choose a reason for hiding this comment

guggero commented Feb 17, 2025