Wallet: don't underestimate the fees when spending a Taproot output #26573
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/26573. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
This was referenced Nov 25, 2022
Closed
|
Concept ACK |
03fb943 to
08f288f
Compare
|
Rebased. |
This was referenced Feb 5, 2023
|
Moving this to draft for now, given it's based on #26567. |
08f288f to
9120567
Compare
|
Rebased. Still based on #26567. |
|
ACK 52cc58a modulo rebase |
52cc58a to
efb5d24
Compare
|
Rebased. |
|
CI is erroring on I'll look into it. So i can't reproduce locally, even when using the same RNG seed. I'm trying building with ASAN since that's where it seems to show up in CI. It also seems very unrelated to this change, the line where it errors is when spending legacy outputs... Couldn't reproduce with sanitizers either. |
We were previously assuming the key path was always used for size estimation, which could lead to underestimate the fees if one of the script paths was used in the end. Instead, overestimate: use the most expensive between the key path and all existing script paths. The functional test changes were authored by Ava Chow for PR 23502. Co-Authored-by: Ava Chow <github@achow101.com>
efb5d24 to
f74a668
Compare
|
Force-pushed to add some |
1 task
murchandamus
reviewed
Feb 6, 2025
| res = rpc_online.sendtoaddress(address=self.boring.getnewaddress(), amount=Decimal(ret_amnt) / 100000000, subtractfeefromamount=True, fee_rate=200) | ||
| res = rpc_online.sendtoaddress(address=self.boring.getnewaddress(), amount=Decimal(ret_amnt) / 100000000, subtractfeefromamount=True, fee_rate=10) | ||
| txinfo = rpc_online.gettransaction(txid=res, verbose=True) | ||
| assert_fee_enough(-txinfo["fee"], txinfo["decoded"]["vsize"], Decimal(0.00010000)) |
There was a problem hiding this comment.
Do I understand right that this test will try the whole battery of different script trees seen below and uses the maximal estimate across script trees, so if there are multiple different leaves that we can satisfy, our transaction might overpay, but no longer underpay?
|
Concept ACK. Will review soon. |
|
Needs rebase, if still relevant |
|
Closing my old open PRs. I have intended to come back to it for a while but as a matter of fact i did not. Anyone interested feel free to pick it up. |
|
Picked up in #32964. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Alternative to #23502.
Currently, when estimating the size an input spending a Taproot output will have once signed, we always assume the key path will be used. Even if there are script paths. This can lead to pretty severe fee underestimation if the script path turns out to be used in the end. So instead assume the most expensive between all script paths and the key path will be used.
This is still not ideal, as there may be a huge gap between the size of a script path spend and a key path spend. Still, this is less bad than undershooting the fees.