spend: feerate using rounded-up vbytes can be lower than target value

wizardsardine / liana

The missing safety net for your coins

https://wizardsardine.com/liana

BSD 3-Clause "New" or "Revised" License

317 stars 56 forks source link

spend: feerate using rounded-up vbytes can be lower than target value #1132

Open jp1ac4 opened 4 months ago

jp1ac4 commented 4 months ago

This issue came up recently in https://github.com/wizardsardine/liana/pull/1125#discussion_r1647583242 and also in the past.

When creating a new spend, we target a feerate in terms of sat/vb, but the bdk_coin_select crate we use for coin selection and determining the change amount uses sat/wu, which can lead to the feerate in sat/vb being lower than the target value, given that we round up when converting from wu to vb: vb = ceil(wu/4).

One option to fix this is to modify the bdk_coin_select crate so that, if a feerate is specified in sat/vb, it calculates the excess and change amount of a given selection based on the size in vb of the transaction rather than its weight.

darosior commented 2 months ago

Michael opened an issue about this upstream: https://github.com/bitcoindevkit/coin-select/issues/26.

darosior commented 1 month ago

To be clear, and to make this appear in searches: this can in particular make a transaction feerate be below min fee (for 1sat/vb txs) thereby making the transaction not broadcastable.

darosior commented 1 month ago

Preventing the transaction from broadcasting is probably the worst case, and could probably be worked around short term for v7 (h/t @kloaec) by slightly increasing the feerate asked by the user when it's 1sat/vb. For instance making it 1.1 or 1.2 sat/vb. I guess this could work.

jp1ac4 commented 1 month ago

I don't think this specific rounding issue can lead to a feerate below 1 sat/vb:

If we enter 1 sat/vb, coin selection uses 0.25 sat/wu.
The fee paid will be at least ceil(0.25 * weight) (need ceiling to ensure integer fee).
Then vsize = ceil(0.25 * weight) and so feerate in sat/vb will be fee / vsize >= ceil(0.25 * weight) / ceil(0.25 * weight) = 1

jp1ac4 commented 2 weeks ago

It's worth noting that both the coin selection crate and mempool.space are using vsize = weight/4, without any rounding. If we were to follow the same approach, then we wouldn't face this particular issue.

As per my previous comment (https://github.com/wizardsardine/liana/issues/1132#issuecomment-2340396972), I don't think using this approach could lead to a feerate below 1 even if rounding up.

Unless I'm missing something, I would favour this approach of not rounding up vbytes.

jp1ac4 commented 1 week ago

Unless I'm missing something, I would favour this approach of not rounding up vbytes.

It seems that rounding up when calculating the feerate is consistent with Bitcoin Core (https://github.com/bitcoindevkit/coin-select/issues/26#issuecomment-2418926725) so let's stick to that for now.