[feature]: disallow creating on-chain transactions below mempoolminfee

[guggero]

Related to: #7505

In situations where the mempool of a node becomes full (e.g. exceeds the default 300 MB size), bitcoind starts to not accept new transactions into the mempool that are below the mempoolminfee threshold reported by the getmempoolinfo RPC call (introduced in bitcoind v0.21.0.

Because transactions with a fee that is too low cannot get into the mempool in the first place, they also cannot be fee bumped, as the CPFP transaction is seen as invalid if its parent isn't known to the node.

The solution is to increase the mempool size (-maxmempool=XXXX) of the bitcoind node, but that is not practicable for most users, especially when running on low-end hardware.
Instead we should make sure that lnd doesn't allow creating transactions below the current mempoolminfee.

This should be a bitcoind specific fix, as btcd currently doesn't have a mempool size limit and Neutrino is dependent on the node it connects to and cannot ask it for its setting.

[shaurya947]

Hi @guggero , I decided to look into this since I'd like to find areas where I can start contributing.

I see that inside of rpcserver.go, the function parseOpenChannelReq calls calculateFeeRate with the chainfee.Estimator (which would be of type BitcoindEstimator). Furthermore, BitcoindEstimator already has the fetchMinMempoolFee function implemented, which is passed to its minFeeManager.

However, I see that inside of walletsweep.go, the DetermineFeePerKw function does not consult the estimator's RelayFeePerKW (which would enforce the mempool min fee for bitcoind) in the case where a manual sat/vbyte rate is set. Is all that's needed here checking RelayFeePerKW and ensuring that the manual fee rate is at least that much?

I would be happy to attempt this fix if you can confirm :)

Edit: threw together this quick commit in case it's easier to just see the code

[guggero]

@shaurya947 nice investigation, I wasn't aware that we already pulled that value from bitcoind (though I didn't look too closely)!

I think it makes sense to fix the sweeper that it always creates transactions that can be added to the local mempool.

But apart from the sweeper, which is part of the automated functionality of lnd, we also need to add similar checks to user-created transactions, such as from the SendCoins, SendMany, OpenChannel, FundPsbt (and probably a few more) RPCs.

Also, while looking through the sweeper code, I also found this sentence: https://github.com/lightningnetwork/lnd/blob/master/sweep/sweeper.go#L367 ("Assume that this will not change from here on"). Which might hold true for pure dust calculations, but not for fee rate calculations (as the min mempool fee can rise and fall during the sweeper's lifetime). So we should probably check that assumption as well.

[shaurya947]

Hey @guggero thanks for the feedback.

My research is indicating that even though the code I edited is in the sweep module, it is indeed used for fee calculation for the below 4 calls (all of which go through rpcserver.go#calculateFeeRate():

• For SendCoins: cmd/lncli/commands.go#sendCoins() -> rpcserver.go#SendCoins() -> rpcserver.go#calculateFeeRate() -> sweep/walletsweep.go#DetermineFeePerKw()
• For SendMany: cmd/lncli/commands.go#sendMany() -> rpcserver.go#SendMany() -> rpcserver.go#calculateFeeRate() -> sweep/walletsweep.go#DetermineFeePerKw()
• For OpenChannel: cmd/lncli/cmd_open_channel.go#openChannel() -> rpcserver.go#OpenChannel() -> rpcserver.go#parseOpenChannelReq() -> rpcserver.go#calculateFeeRate() -> sweep/walletsweep.go#DetermineFeePerKw()
• Similar for CloseChannel

For FundPsbt, it looks like we are in lnrpc/walletrpc/walletkit_server.go#FundPsbt() where a similar problem exists: if a confirmation target is specified, we use the the wallet's fee estimator (which will enforce min relay fee), but if a manual fee rate is specified we don't do any checking.

I would have suggested to replace the switch above with a call to sweep/walletsweep.go#DetermineFeePerKw() (since the sweep module is already imported inside lnrpc/walletrpc/walletkit_server.go), but the logic is slightly different since FundPsbt wants a conf. target that's at least 2.

All that being said, what would you suggest? I can think of three options:

1. Make a PR to handle the first 4 RPC commands that all share the same code path, worry about FundPsbt and any other RPC calls that aren't covered separately/later.
2. Handle the first 4 RPC commands + FundPsbt by using sweep/walletsweep.go#DetermineFeePerKw() only in the manual fee case (so as to preserve the different logic in the conf. target case)
3. Handle the first 4 RPC commands + FundPsbt by somehow modifying sweep/walletsweep.go#DetermineFeePerKw() to take in an additional param for minConfTarget, which would allow us to replace the entire switch in the case of FundPsbt.

[guggero]

You are correct, it seems like using a conf target is already handled correctly everywhere. Though most users are more comfortable using --sat_per_vbyte directly, so we do need to add a check for those manual values. I think that can be done here directly. It does seem like this is used everywhere and not just by the sweeper, so that's good.

I think a PR that does the following would be good:

• Move the rpcserver.go#calculateFeeRate() function to the rpc_util.go file and export it.
• Use that rpc_util.go#CalculateFeeRate as well in lnrpc/walletrpc/walletkit_server.go for the fee calculation.
• Ad a check that a manual fee rate is greater or equal to the minimum relay/mempool fee rate in walletsweep.go#DetermineFeePerKw.

We should then look through all *.proto files to identify manual fee rate flags/fields that are not yet being handled through CalculateFeeRate() and make sure they are.

[shaurya947]

Sounds good! I'll aim to have a PR up today 🙂

[shaurya947]

Gave it a shot here!