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

Absolute fee constraint in TxBuilder not being respected #1066

Closed
sjeohp opened this issue Aug 7, 2023 · 15 comments · Fixed by #1158
Closed

Absolute fee constraint in TxBuilder not being respected #1066

sjeohp opened this issue Aug 7, 2023 · 15 comments · Fixed by #1158
Milestone

Comments

@sjeohp
Copy link
Contributor

sjeohp commented Aug 7, 2023

When creating a tx the absolute fee amount when set does not always match the actual fee amount. I see this is because the amount is added to several times, so it must be deliberate, but for someone using the library it is unintuitive. What is the rationale?

@notmandatory
Copy link
Member

Can you also give a little code example and which version of bdk are you using? This would help us know which documentation needs to be improved.

@sjeohp
Copy link
Contributor Author

sjeohp commented Aug 8, 2023

crates/bdk/wallet/mod.rs
fn create_tx

@sjeohp
Copy link
Contributor Author

sjeohp commented Aug 8, 2023

For example

let mut tx_builder = wallet.build_tx();
tx_builder.fee_absolute(fee);
let (psbt, details) = tx_builder.finish()?;

In my experience the fee will not necessarily match the fee that was set.

@LLFourn
Copy link
Contributor

LLFourn commented Aug 9, 2023

It's worth pointing out that the "absolute fee" is a really a minimum absolute fee -- it's possible to overshoot it slightly since adding a change output to drian the remaining excess might not be viable.

How far off is the fee vs expected?

@LLFourn LLFourn changed the title Question about absolute fee Absolute fee constraint in TxBuilder not be respected Aug 9, 2023
@LLFourn LLFourn changed the title Absolute fee constraint in TxBuilder not be respected Absolute fee constraint in TxBuilder not being respected Aug 9, 2023
@sjeohp
Copy link
Contributor Author

sjeohp commented Aug 10, 2023

Making tiny txs on testnet the fee varies by 100-150%.

Is there a way to upper bound it? Or reduce variability at the cost of something else?

@LLFourn
Copy link
Contributor

LLFourn commented Aug 14, 2023

Appreciated the work looking into this. No but I am not convinced that the implementation isn't simply wrong with respect to absolute fees. Is the fee wrong even when a change output is added? That would be the next question since the builder adds a change output and the resulting tx does not have the correct fee that would mean there's a bug I think.

@sjeohp
Copy link
Contributor Author

sjeohp commented Aug 19, 2023

Yes same thing with the change descriptor.

With <desc> having 4500 satoshis and <change_desc> having 1000 satoshis or 0 (no difference):
bdk-cli wallet -d <desc> -c <change_desc> create_tx --to <receiver_addr>:500 --fee_abs 250 returns a fee of 500 most times, sometimes 250.

Also I don't exactly understand this:

bdk-cli wallet -d <DESC_A> -c <DESC_B> get_balance returns the balance of both wallets combined but needs to be synced with both combined. If both are synced separately or inverted like bdk-cli wallet -d <DESC_B> -c <DESC_A> sync the balance will be wrong and create_tx might fail.

@LLFourn
Copy link
Contributor

LLFourn commented Aug 21, 2023

Yes same thing with the change descriptor.

With <desc> having 4500 satoshis and <change_desc> having 1000 satoshis or 0 (no difference): bdk-cli wallet -d <desc> -c <change_desc> create_tx --to <receiver_addr>:500 --fee_abs 250 returns a fee of 500 most times, sometimes 250.

This does sound like a bug to me. Can you just check that we can distinguish the cases via whether change output is added vs when it is not. i.e. is it always 500 when a change output is added but when you have an output that is close enough to the target amount (here 500) it goes to 250 if not change output is added. In other words i think the bug occurs in the path that change output is added. The culprit is likely a change in or around #641. Strange that tests don't pick it up.

@sjeohp
Copy link
Contributor Author

sjeohp commented Aug 21, 2023

Same behaviour without the change flag.

@LLFourn
Copy link
Contributor

LLFourn commented Aug 21, 2023

Same behaviour without the change flag.

Sorry I don't mean the flag. BDK will add a change output regardless of whether you add a change descriptor (it will just use the main descriptor for the change script pubkey). The question is about whether the change output correlates being added correlates with the bug. You'd need to check the number of outputs the tx has to confirm/deny this. If there's two then a change was added but if there's only one then it was not.

@sjeohp
Copy link
Contributor Author

sjeohp commented Aug 21, 2023

Yes you are right the 250 fee corresponds with two outputs, the 500 with only one.

@notmandatory notmandatory moved this to Todo in BDK Maintenance Aug 21, 2023
@sjeohp
Copy link
Contributor Author

sjeohp commented Aug 24, 2023

What might cause the random difference in number of outputs? Fee estimate? or coin selection algorithm?

@sjeohp
Copy link
Contributor Author

sjeohp commented Aug 24, 2023

The issue is when there is no exact match utxos the single_random_draw can result in utxo selection with a remainder that is below the dust threshold, in which case it is added to the fee. IMO this results in unintuitive behaviour in the situations above but is not exactly a bug since whether it is best to create a change output for tiny remainders or add it to the fee is not clear. (It can be cheaper in the long term to not create the change output). Probably if fee_abs is set and exceeded the operation should fail. At any rate I believe the fee should only fluctuate by amounts comparable to the dust threshold.

@LLFourn
Copy link
Contributor

LLFourn commented Aug 25, 2023

Right this is what I was suggesting here: #1066 (comment)

I doubted it though because it seems unlikely that it would result in exactly 250 error (such a round number). Why would not adding a change output lead to an excess fee of exactly double the requirement -- I guess it's just because you were using rather round numbers for inputs and outputs in the experiment...

@danielabrozzoni
Copy link
Member

I've opened #1158 and #1159 to clarify the meaning of absolute_fee

danielabrozzoni added a commit to danielabrozzoni/bdk that referenced this issue Oct 9, 2023
danielabrozzoni added a commit to danielabrozzoni/bdk that referenced this issue Oct 10, 2023
danielabrozzoni added a commit to danielabrozzoni/bdk that referenced this issue Oct 11, 2023
danielabrozzoni added a commit to danielabrozzoni/bdk that referenced this issue Oct 11, 2023
danielabrozzoni added a commit to danielabrozzoni/bdk that referenced this issue Oct 11, 2023
danielabrozzoni added a commit to danielabrozzoni/bdk that referenced this issue Nov 8, 2023
danielabrozzoni added a commit to danielabrozzoni/bdk that referenced this issue Nov 8, 2023
@notmandatory notmandatory added this to BDK Dec 5, 2023
@notmandatory notmandatory moved this from Todo to In Progress in BDK Maintenance Dec 5, 2023
@notmandatory notmandatory moved this to In Progress in BDK Dec 5, 2023
@notmandatory notmandatory added this to the 1.0.0-alpha.3 milestone Dec 5, 2023
@nondiremanuel nondiremanuel moved this from In Progress to Needs Review in BDK Dec 5, 2023
notmandatory added a commit that referenced this issue Dec 5, 2023
0ecc028 doc(bdk): Clarify the absolute_fee, fee_rate docs (Daniela Brozzoni)

Pull request description:

  Fixes #1066

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

ACKs for top commit:
  notmandatory:
    ACK 0ecc028

Tree-SHA512: 152e48b86c21d4fad711abf76dd8fdc0e8955030331005c1ba6ff0c866c52870161f91ec740838f8238c5ad1c620e06212099308a55d130699cb18e4666e3f3f
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Dec 5, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in BDK Maintenance Dec 5, 2023
notmandatory pushed a commit to notmandatory/bdk that referenced this issue Dec 7, 2023
danielabrozzoni added a commit to danielabrozzoni/bdk that referenced this issue Apr 18, 2024
notmandatory pushed a commit to danielabrozzoni/bdk that referenced this issue Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants