Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jp1ac4 opened this issue Jun 21, 2024 · 7 comments
Open

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

jp1ac4 opened this issue Jun 21, 2024 · 7 comments
Assignees
Labels
Bug Something isn't working as expected Daemon / Liana library This is about lianad or the liana library (not the GUI)

Comments

@jp1ac4
Copy link
Collaborator

jp1ac4 commented Jun 21, 2024

This issue came up recently in #1125 (comment) 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.

@nondiremanuel nondiremanuel moved this to Todo in Liana General Jun 21, 2024
@darosior darosior added Bug Something isn't working as expected Daemon / Liana library This is about lianad or the liana library (not the GUI) labels Jun 22, 2024
@nondiremanuel nondiremanuel added this to the Liana v7 milestone Jun 25, 2024
@darosior
Copy link
Member

Michael opened an issue about this upstream: bitcoindevkit/coin-select#26.

@nondiremanuel nondiremanuel modified the milestones: v7 - Liana, v8 - Liana Sep 4, 2024
@nondiremanuel nondiremanuel moved this from Todo to To be discussed / defined in Liana General Sep 9, 2024
@darosior
Copy link
Member

darosior commented Sep 9, 2024

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
Copy link
Member

darosior commented Sep 9, 2024

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
Copy link
Collaborator Author

jp1ac4 commented Sep 10, 2024

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

@nondiremanuel nondiremanuel modified the milestones: v7 - Liana, v8 - Liana Sep 11, 2024
@jp1ac4 jp1ac4 changed the title spend: feerate in sat/vb can be lower than target value spend: feerate using rounded-up vbytes can be lower than target value Oct 8, 2024
@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Oct 14, 2024

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 (#1132 (comment)), 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
Copy link
Collaborator Author

jp1ac4 commented Oct 17, 2024

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 (bitcoindevkit/coin-select#26 (comment)) so let's stick to that for now.

@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Oct 30, 2024

The fix has now been merged upstream (bitcoindevkit/coin-select#29) so we just need to wait for the next release of the crate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working as expected Daemon / Liana library This is about lianad or the liana library (not the GUI)
Projects
Status: Todo
Development

No branches or pull requests

3 participants