sweep+lnrpc: enforce provided fee rate is no less than relay fee

[shaurya947]

Change Description

Fixes #7512 by enforcing that if a manual fee rate is provided in RPC calls then that fee rate is no less than the relay fee of the underlying chain node. To achieve this, as suggested, the calculateFeeRate() function was refactored out of rpcserver.go and into rpc_utils.go, and reused in walletkit_server.go. This way FundPsbt rpc call can use the same fee estimation logic as OpenChannel, SendCoins, etc. FundPsbt will be handled in a follow-up PR.

I've tried my best to follow the required practices (code style, linting, comments etc.). Since this is my first PR, I'd love any feedback about what I may have overlooked 🙂

Steps to Test

Attempt any one of the rpc calls whilst providing a manual sat per vbyte which would fall below the relay fee of the chain node that lnd is connected to. Notice that the fee rate used is actually the relay fee.

• OpenChannel
• SendCoins
• SendMany
• CloseChannel

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative (error paths) are included. (existing test was updated)
• Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• Commits follow the Ideal Git Commit Structure.
• Any new logging statements use an appropriate subsystem and logging level.
• There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

[alexbosworth]

for fundpsbt it would be nice to provide a minimum relay fee to funding, maybe locally the minimum relay fee is higher than the chain daemon ultimately where it will be broadcast - a funded psbt can be broadcast outside of the local chain backend

[guggero]

for fundpsbt it would be nice to provide a minimum relay fee to funding, maybe locally the minimum relay fee is higher than the chain daemon ultimately where it will be broadcast - a funded psbt can be broadcast outside of the local chain backend

Why would you not just specify a custom --sat_per_vbyte value in that case?

[guggero]

Looks very good, thanks a lot for the PR!

[alexbosworth]

Why would you not just specify a custom --sat_per_vbyte value in that case?

right but wouldn't this block that if the specified rate was too low?

[guggero]

right but wouldn't this block that if the specified rate was too low?

The change in this PR wouldn't block you from using that lower fee rate but increase the manually specified rate to the minimum local rate. I can see why this might not always be the desired outcome. Maybe we need a --i_know_what_i_am_doing_let_me_publish_with_low_fees flag? :D Not sure how much of an edge case this actually is though? Have you ever encountered a situation like this where you would have wanted to specify a rate that was too low for the local node?

[alexbosworth]

right but wouldn't this block that if the specified rate was too low?

The change in this PR wouldn't block you from using that lower fee rate but increase the manually specified rate to the minimum local rate. I can see why this might not always be the desired outcome. Maybe we need a --i_know_what_i_am_doing_let_me_publish_with_low_fees flag? :D Not sure how much of an edge case this actually is though? Have you ever encountered a situation like this where you would have wanted to specify a rate that was too low for the local node?

yeah that's what i mean exactly

for fundpsbt definitely yes, especially in cases where psbts are being collaboratively formed but not all participants agree on a minimum relay fee. a lot of operators run a limited mempool to save on operational costs. for the other calls I can't think why you would care, only fundpsbt because that isn't necessarily going to be broadcast locally or maybe it will not be broadcast locally immediately

[guggero]

for fundpsbt definitely yes, especially in cases where psbts are being collaboratively formed but not all participants agree on a minimum relay fee. a lot of operators run a limited mempool to save on operational costs. for the other calls I can't think why you would care, only fundpsbt because that isn't necessarily going to be broadcast locally or maybe it will not be broadcast locally immediately

Okay, that case makes sense. Though I guess my question would be: Does it really make sense to publish a transaction with a fee rate that you know upfront won't make it into the default mempools? Assuming the resulting transaction most likely is a channel funding TX where you'd risk it not being confirmed in the necessary 2 weeks if the fee rate is too low? Or would the use case here be to low-ball it with 1 sat/vByte and then later CPFP it higher?

[alexbosworth]

Okay, that case makes sense. Though I guess my question would be: Does it really make sense to publish a transaction with a fee rate that you know upfront won't make it into the default mempools? Assuming the resulting transaction most likely is a channel funding TX where you'd risk it not being confirmed in the necessary 2 weeks if the fee rate is too low? Or would the use case here be to low-ball it with 1 sat/vByte and then later CPFP it higher?

Yeah the issue is the publish time and place which can be variable - potentially low-balling with cpfp or just waiting for publish at a later date and creating a series of potential txs that all can be used, but at different fee rates

[shaurya947]

Thanks for the review @guggero ! I'll get started on the feedback you provided.

I would also like a bit of help tackling this failing itest:

It tries to fund a PSBT with a fee of 1sat/vbyte, which translates to 250 sat/kw. However, with these new changes, we enforce a floor of FeePerKwFloor which is a constant with the value 253 sat/kw. How best to remedy this? There are a lot of hardcoded numbers in this itest file and I would like to make the most minimal change that makes sense.

Finally, I tried to follow the discussion between you and @alexbosworth -- is the ask to additionally provide a floor for FundPsbt that overrides the relay fee setting? If so, that seems to be a larger scoped change involving CLI + RPC additions. If this is desired, may I ask if it's okay to either:

1. Keep this PR as-is and add the new flag in a separate PR OR
2. Remove FundPsbt changes from this PR and put those in a separate PR (with the added flag for the floor override)

?

[alexbosworth]

For FundPsbt the idea is just to avoid the fee guardrails being mandatory

Another idea I had though is to allow for clamping the fee estimate, seems like a lot of times that this error condition happens it's because Bitcoin Core is returning a low end fee estimate that is below its minimum relay fee

This makes more sense in Bitcoin Core because the idea is that you rebroadcast your transactions and the mempool is more of a temporary cache rather than a guaranteed queue

[guggero]

Remove FundPsbt changes from this PR and put those in a separate PR (with the added flag for the floor override)

Yes, I think we should go with option 2, address FundPsbt in a separate PR (building on top of this one). The idea is to have an additional flag, for example --ignore_min_mempool_fee which would allow you to set --sat_per_vbyte explicitly without it being bumped to the minimum fee as it is done in the other RPCs. I'm not sure how Alex's fee estimate clamping would look like though, but I guess that can be discussed in the follow-up PR.

It tries to fund a PSBT with a fee of 1sat/vbyte, which translates to 250 sat/kw.

Ugh, yeah, that test is a bit of a pain with all the hard coded values. Since this sounds like a pure rounding thing (where the 3 additional sat/kw sometime result in a full satoshi more in fees), I think you just need to update the values until you get no failure. You can run that test in isolation by running make itest icase=lnd_wallet_import_test which should save you some time.

[shaurya947]

Thanks @guggero -- I removed the FundPsbt changes, but still kept the relocation of the calculateFeeRate function for use in the next PR since it's already been reviewed. I also addressed your comments.

The failing itest was due to FundPsbt so hopefully it's no longer a problem for this PR.

The --ignore_min_mempool_fee flag for FundPsbt sounds like a fine addition to address the edge case mentioned and I would be happy to work on a new PR which includes that. Although I would like to understand more @alexbosworth 's idea of "allow for clamping the fee estimate" as well before diving in.

[alexbosworth]

by clamping the fee estimate i mean just make sure that if you ask for a long confirm conf target like 1000 that it doesn't return or use a fee rate that is below the minimum relay fee rate

[shaurya947]

by clamping the fee estimate i mean just make sure that if you ask for a long confirm conf target like 1000 that it doesn't return or use a fee rate that is below the minimum relay fee rate

I believe that already happens for both bitcoind and btcd.

[alexbosworth]

by clamping the fee estimate i mean just make sure that if you ask for a long confirm conf target like 1000 that it doesn't return or use a fee rate that is below the minimum relay fee rate

I believe that already happens for both bitcoind and btcd.

ah ok great, nevermind then

[guggero]

@shaurya947 thanks for the updates. Can you apply the fixup commits please? I'll give this another round of review afterwards.

[shaurya947]

@shaurya947 thanks for the updates. Can you apply the fixup commits please? I'll give this another round of review afterwards.

Done @guggero 🙂

[guggero]

Only nits left, LGTM 🎉

[yyforyongyu]

Thanks for the PR⚡️ Major question is whether we should default to the min relay fee or abort the transaction so the user can decide to not open a channel.

I've tried my best to follow the required practices (code style, linting, comments etc.). Since this is my first PR, I'd love any feedback about what I may have overlooked 🙂

I'd probably squash the first two commits and the refactor in the third commit seems to be not applied anywhere, but this is good enough for a first-time contributor, good job!

[yyforyongyu]

Maybe we should disallow creating transactions instead of defaulting to the min relay fee? Maybe I'd choose to not open a channel if the min relay fee is too high, but I don't know until I open the channel. Not sure tho, cc @guggero

[guggero]

Hmm, good question... Maybe failing with an appropriate error message is actually the better option UX wise.

[alexbosworth]

+1 for error message

[shaurya947]

Thanks for the input guys. Happy to make the change but just wondering if there is an argument to be made about preserving existing behavior? Granted that the bump today is only to chainfee.FeePerKwFloor which, if I'm not mistaken, translates to 1sat/vbyte.

[guggero]

I think most RPCs disallow setting 0 or a fraction of sat/vByte. So the existing behavior would only come into place in RPCs where we have sat/kWU (which isn't the case here). But if we want to be 100% exact, we would split the check into two: if the fee rate is below the absolute floor value, bump it. If it's still lower than the min relay/mempool fee, return an error (probably mentioning the original, user-set value, not the bumped one).

[shaurya947]

I think most RPCs disallow setting 0 or a fraction of sat/vByte. So the existing behavior would only come into place in RPCs where we have sat/kWU (which isn't the case here).

This makes sense.

split the check into two: if the fee rate is below the absolute floor value, bump it. If it's still lower than the min relay/mempool fee, return an error (probably mentioning the original, user-set value, not the bumped one).

I would imagine in most instances the relay/mempool fee would be >= the absolute floor value. Is that accurate? If so, it doesn't seem very useful to introduce additional complexity to handle the case where the inequality is reversed (and a bumped fee would actually go through). What do you think?

[guggero]

I would imagine in most instances the relay/mempool fee would be >= the absolute floor value.

Yes, that's correct. I was just thinking about RPCs that use sat/kWU, if they also go through DetermineFeePerKw then users could actually enter a value lower than the absolute floor value. In that cases it would make sense to just bump that to the floor. But returning an error in both cases probably makes sense to keep the code simple.
We should probably be pretty specific in the error message, mentioning something along the lines of "fee of %d sat/vByte is too low to be accepted into the mempool or relayed to the network".

[shaurya947]

Also, any ideas why the itest check for backend=bitcoind dbbackend=etc fails (but not the others)? Is that expected? When I try running locally using dbbackend=etcd I'm not even able to get past the test set up (says found nil RPC clients)

[guggero]

Also, any ideas why the itest check for backend=bitcoind dbbackend=etc fails (but not the others)? Is that expected? When I try running locally using dbbackend=etcd I'm not even able to get past the test set up (says found nil RPC clients)

Itests are still flaky, we're working on that.

[shaurya947]

Hey @guggero and @yyforyongyu , I updated the PR to error out when the provided fee is too low. Updated the tests as well. Let me know how it looks now 🙂

[guggero]

Looks like we actually use a fee rate lower than 253 sat/kWU in one of the integration tests: manual fee rate input of 250 sat/kw is too low to be accepted into the mempool or relayed to the network. So maybe that extra automatic bump from 250 to 253 is actually needed.

[shaurya947]

Hey @guggero thanks for looking into the test failure.

I wanted to make sure I fully understand your suggestion. So we're considering two values, FeePerKwFloor and RelayFeePerKW. And we'd like to uphold the following two constraints at the same time:

1. If the specified fee is below FeePerKwFloor, bump it to FeePerKwFloor
2. If the specified fee is below RelayFeePerKW, reject with an error

Is that accurate?

If so, we consider the 3 possible inequalities:

• FeePerKwFloor < RelayFeePerKW: in this case, the bump (constraint 1) would always be a no-op because even after the bump we will always be below RelayFeePerKW
• FeePerKwFloor == RelayFeePerKW: in this case, the bump (constraint 1) would indeed work as intended, but in doing the bump, we would violate constraint 2
• FeePerKwFloor > RelayFeePerKW: I think this case has undefined behavior w.r.t the two constraints because now the interval [0, FeePerKwFloor) is split into two:
  • [0, RelayFeePerKW) conflict—should error out as per constraint 2, but do a bump as per constraint 1
  • [RelayFeePerKW, FeePerKwFloor) no conflict—bumps as per constraint 1 whilst also respecting constraint 2

Let me know if I'm misinterpreting or overthinking this. Otherwise I can just change the itest to use a higher fee value.

[guggero]

Hmm, you are right, there might be weird edge cases here. To me RelayFeePerKW is more or less a constant, since it requires an active config change in the bitcoind backend. But that might actually happen.

Otherwise I can just change the itest to use a higher fee value.

I wasn't sure how many tests would be affected. And it sounded a bit like a workaround to me. But maybe that is indeed the better option?

I guess what we always have to check is that the fee rate returned by feeEstimator.RelayFeePerKW() is larger or equal to FeePerKwFloor. Because if the rate is FeePerKwFloor it very likely won't propagate anyway (even if the local bitcoind would accept it due to custom/wrong user settings).

So perhaps do this in walletsweep.go instead?

		feePerKW := feePref.FeeRate
		minFeePerKW := chainfee.FeePerKwFloor
		minRelayFeePerKW := feeEstimator.RelayFeePerKW()
		if minRelayFeePerKW > chainfee.FeePerKwFloor {
			minFeePerKW := minRelayFeePerKW
		}

		if feePerKW < minFeePerKW {
			return 0, fmt.Errorf("manual fee rate input of %d "+
				"sat/kw is too low to be accepted into the "+
				"mempool or relayed to the network", feePerKW)
		}

[shaurya947]

Hey @guggero thanks for the feedback!

I guess what we always have to check is that the fee rate returned by feeEstimator.RelayFeePerKW() is larger or equal to FeePerKwFloor

This was actually going to be my next question 🙂 because I believe this already happens. RelayFeePerKW() calls minFeeManager.fetchMinFee(), which internally ensures that we're always >= FeePerKwFloor

lnd/lnwallet/chainfee/minfeemanager.go

Line 73 in bbbf7d3

if m.minFeePerKW < FeePerKwFloor {

So what do you think then, just update the tests?

[shaurya947]

Updated itests. Hopefully I caught everything :)

[guggero]

Ah, sorry, I should've taken a closer look at the failing tests first. But this latest change basically makes it impossible to ever use 1 sat/vByte again for anything on-chain. Which is definitely not what we want. I was expecting the itest changes to only be necessary for RPCs where we use sat/kw and had a value of 250 instead of 253.

So what we actually actually want (I believe) is:

	case feePref.FeeRate != 0:
		feePerKW := feePref.FeeRate

		// Because the user can specify 1 sat/vByte on the RPC
		// interface, which corresponds to 250 sat/kw, we need to bump
		// that to the minimum "safe" fee rate which is 253 sat/kw.
		if feePerKW == chainfee.AbsoluteFeePerKwFloor {
			feePerKW = chainfee.FeePerKwFloor
		}

		// If that bumped fee rate of at least 253 sat/kw is still too
		// low to be relayed or accepted into the mempool, we return an
		// error to let the user know.
		minFeePerKW := feeEstimator.RelayFeePerKW()
		if feePerKW < minFeePerKW {
			return 0, fmt.Errorf("manual fee rate input of %d "+
				"sat/kw is too low to be accepted into the "+
				"mempool or relayed to the network", feePerKW)
		}

		return feePerKW, nil

[shaurya947]

Makes sense @guggero , updated!

[guggero]

Nice work, LGTM 🎉

[yyforyongyu]

LGTM🏖