Wallet: don't underestimate the fees when spending a Taproot output#26573
Closed
darosior wants to merge 1 commit intobitcoin:masterfrom
Closed
Wallet: don't underestimate the fees when spending a Taproot output#26573darosior wants to merge 1 commit intobitcoin:masterfrom
darosior wants to merge 1 commit intobitcoin:masterfrom
Conversation
Contributor
|
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. |
darosior
changed the title
This was referenced Nov 25, 2022
Closed
Member
|
Concept ACK |
darosior
force-pushed
the
taproot_over_dont_under_estimate
branch
from
03fb943 to
08f288f
Compare
Member
Author
|
Rebased. |
This was referenced Feb 5, 2023
Member
|
Moving this to draft for now, given it's based on #26567. |
darosior
force-pushed
the
taproot_over_dont_under_estimate
branch
from
08f288f to
9120567
Compare
Member
Author
|
Rebased. Still based on #26567. |
Member
|
ACK 52cc58ab3ad0ee9df9ccc5303c7f0ce85cf8afee modulo rebase |
darosior
force-pushed
the
taproot_over_dont_under_estimate
branch
from
52cc58a to
efb5d24
Compare
Member
Author
|
Rebased. |
Member
Author
|
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 <[email protected]>
darosior
force-pushed
the
taproot_over_dont_under_estimate
branch
from
efb5d24 to
f74a668
Compare
Member
Author
|
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)) |
Member
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?
Member
|
Concept ACK. Will review soon. |
Contributor
|
Needs rebase, if still relevant |
Member
Author
|
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. |
Member
|
Picked up in #32964. |
tboy1337
added a commit
to tboy1337/bitcoin
that referenced
this pull request
Jan 23, 2026
This fixes the MaxSatisfactionWeight calculation for Taproot descriptors to properly account for script path spending and correct Schnorr signature sizes. Key changes: 1. PKDescriptor::MaxSatSize: Corrected from 64 to 65 bytes for Schnorr signatures (64-byte signature + 1-byte sighash flag) 2. MultiADescriptor::MaxSatSize: Updated to use 65 bytes per signature 3. TRDescriptor::MaxSatisfactionWeight: Implemented proper script path weight calculation using Assume() and GetSizeOfCompactSize() 4. Updated test expectations to reflect corrected weight calculations The previous implementation incorrectly assumed SIGHASH_DEFAULT could be omitted from maximum size calculations. As achow101 noted: "The maximum size of a signature must include the size of the sighash flag." This ensures accurate fee estimation, proper PSBT construction, and optimal coin selection for Taproot transactions. Tests: - All 695 C++ unit tests pass - wallet_taproot.py functional test passes Based on approach from PR bitcoin#26573 by darosior.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
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.