Wallet: don’t underestimate the fees when spending a Taproot output #26573

pull darosior wants to merge 1 commits into bitcoin:master from darosior:taproot_over_dont_under_estimate changing 3 files +43 −9
  1. darosior commented at 12:21 pm on November 25, 2022: member

    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.

  2. DrahtBot commented at 12:22 pm on November 25, 2022: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/26573.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK sipa, murchandamus, furszy
    Stale ACK achow101

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31874 (qa wallet: Activate dormant checks in wallet_multisig_descriptor_psbt.py by hodlinator)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. darosior renamed this:
    Taproot over dont under estimate
    Wallet: don't underestimate the fees when spending a Taproot output
    on Nov 25, 2022
  4. DrahtBot added the label Wallet on Nov 25, 2022
  5. luke-jr commented at 0:13 am on November 27, 2022: member

    Alternative to #23502.

    Not really an alternative, since #26567 shouldn’t be eligible for backporting, but this is arguably a fix.

    IMO #23502 should still get reviewed & merged first, then let #26567 include this in its refactor.

  6. DrahtBot added the label Needs rebase on Dec 5, 2022
  7. achow101 commented at 0:02 am on February 4, 2023: member
    Concept ACK
  8. darosior force-pushed on Feb 5, 2023
  9. darosior commented at 6:18 pm on February 5, 2023: member
    Rebased.
  10. DrahtBot removed the label Needs rebase on Feb 5, 2023
  11. DrahtBot added the label Needs rebase on Feb 16, 2023
  12. fanquake commented at 3:27 pm on February 24, 2023: member
    Moving this to draft for now, given it’s based on #26567.
  13. fanquake marked this as a draft on Feb 24, 2023
  14. darosior force-pushed on Jul 19, 2023
  15. darosior commented at 1:18 pm on July 19, 2023: member
    Rebased. Still based on #26567.
  16. DrahtBot removed the label Needs rebase on Jul 19, 2023
  17. DrahtBot added the label Needs rebase on Jul 27, 2023
  18. darosior force-pushed on Jul 31, 2023
  19. DrahtBot removed the label Needs rebase on Aug 1, 2023
  20. DrahtBot added the label Needs rebase on Aug 17, 2023
  21. darosior force-pushed on Aug 18, 2023
  22. DrahtBot removed the label Needs rebase on Aug 18, 2023
  23. DrahtBot added the label CI failed on Aug 18, 2023
  24. darosior force-pushed on Aug 25, 2023
  25. DrahtBot removed the label CI failed on Aug 25, 2023
  26. darosior force-pushed on Sep 7, 2023
  27. darosior commented at 8:04 am on September 7, 2023: member
    Rebased and undrafting now that #26567 is merged.
  28. darosior marked this as ready for review on Sep 7, 2023
  29. in src/script/descriptor.cpp:1106 in 1ec49d2c15 outdated 
    1103+        for (size_t i = 0; i < m_subdescriptor_args.size(); ++i) {
1104+            // Anything inside a Tapscript leave must have its satisfaction and script size set.
1105+            const auto sat_size = m_subdescriptor_args[i]->MaxSatSize(dummy_max_sig);
1106+            const auto script_size = m_subdescriptor_args[i]->ScriptSize();
1107+            const auto control_size = 33 + 32 * m_depths[i];
1108+            const auto total_weight = 1 + control_size + 1 + *script_size + 1 + *sat_size;
    murchandamus commented at 7:49 pm on October 4, 2023:
    I assume the 1s here are the CompactSize length indicators for the witness items. It seems to me that any of the three could be more than 252 bytes, so perhaps you need to check for that and use 3 bytes for the CompactSize in case the corresponding element is longer?
    achow101 commented at 8:25 pm on October 4, 2023:
    One of those 1s should also be for the entire stack size, which could also be larger than 252.
    sipa commented at 6:47 pm on January 2, 2024:
    Seems better to use GetSizeOfCompactSize here.
    darosior commented at 7:40 am on January 3, 2024:
    Done.
  30. murchandamus commented at 7:54 pm on October 4, 2023: contributor
    Approach ACK
  31. in src/script/descriptor.cpp:1119 in 1ec49d2c15 outdated 
    1118+        // Start from the stack size of a keypath spend.
1119+        int64_t max_stack_size = 1;
1120+
1121+        // Then go through all the existing leaves to check if there is anything more expensive.
1122+        for (size_t i = 0; i < m_subdescriptor_args.size(); ++i) {
1123+            // Anything inside a Tapscript leave must have its satisfaction stack size set.
    sipa commented at 6:40 pm on January 2, 2024:
    Same here.
  32. sipa commented at 6:47 pm on January 2, 2024: member
    Concept ACK
  33. in src/script/descriptor.cpp:1102 in 1ec49d2c15 outdated 
    1099+        int64_t max_weight = 1 + 65;
1100+
1101+        // Then go through all the existing leaves to check if there is anything more expensive.
1102+        const bool dummy_max_sig = true;
1103+        for (size_t i = 0; i < m_subdescriptor_args.size(); ++i) {
1104+            // Anything inside a Tapscript leave must have its satisfaction and script size set.
    sipa commented at 6:48 pm on January 2, 2024:
    Nit: leave -> leaf
    darosior commented at 7:40 am on January 3, 2024:
    Done
  34. darosior force-pushed on Jan 3, 2024
  35. DrahtBot added the label CI failed on Jan 13, 2024
  36. DrahtBot added the label Needs rebase on Mar 11, 2024
  37. achow101 requested review from achow101 on Apr 9, 2024
  38. achow101 removed review request from achow101 on Apr 15, 2024
  39. achow101 commented at 9:56 pm on April 15, 2024: member
    ACK 52cc58ab3ad0ee9df9ccc5303c7f0ce85cf8afee modulo rebase
  40. DrahtBot requested review from murchandamus on Apr 15, 2024
  41. DrahtBot requested review from sipa on Apr 15, 2024
  42. darosior force-pushed on Jun 28, 2024
  43. darosior commented at 11:17 am on June 28, 2024: member
    Rebased.
  44. DrahtBot removed the label Needs rebase on Jun 28, 2024
  45. DrahtBot removed the label CI failed on Jun 28, 2024
  46. darosior commented at 1:56 pm on June 28, 2024: member

    CI is erroring on

     02024-06-28T11:41:00.1099453Z 5/309 - wallet_fundrawtransaction.py --descriptors failed, Duration: 30 s
 12024-06-28T11:41:00.1100206Z 
 22024-06-28T11:41:00.1100438Z stdout:
 32024-06-28T11:41:00.1101244Z 2024-06-28T11:40:29.220000Z TestFramework (INFO): PRNG seed is: 3960890322720202555
 42024-06-28T11:41:00.1108001Z 2024-06-28T11:40:29.221000Z TestFramework (INFO): Initializing test directory /home/runner/work/_temp/ci/scratch/test_runner/test_runner_₿_🏃_20240628_113945/wallet_fundrawtransaction_297
 52024-06-28T11:41:00.1110607Z 2024-06-28T11:40:32.793000Z TestFramework (INFO): Connect nodes, set fees, generate blocks, and sync
 62024-06-28T11:41:00.1112058Z 2024-06-28T11:40:37.036000Z TestFramework (INFO): Test 'add_inputs' default value
 72024-06-28T11:41:00.1116113Z 2024-06-28T11:40:38.689000Z TestFramework (INFO): Test wallet preset inputs are not double-counted or reused in coin selection
 82024-06-28T11:41:00.1118223Z 2024-06-28T11:40:39.538000Z TestFramework (INFO): Test weight calculation with external inputs
 92024-06-28T11:41:00.1119520Z 2024-06-28T11:40:40.376000Z TestFramework (INFO): Test weight limits
102024-06-28T11:41:00.1120605Z 2024-06-28T11:40:57.670000Z TestFramework (ERROR): Assertion failed
112024-06-28T11:41:00.1121410Z Traceback (most recent call last):
122024-06-28T11:41:00.1123136Z   File "/home/runner/work/_temp/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 165, in try_rpc
132024-06-28T11:41:00.1124528Z     fun(*args, **kwds)
142024-06-28T11:41:00.1126042Z   File "/home/runner/work/_temp/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/coverage.py", line 50, in __call__
152024-06-28T11:41:00.1127695Z     return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
162024-06-28T11:41:00.1128575Z                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
172024-06-28T11:41:00.1130261Z   File "/home/runner/work/_temp/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 146, in __call__
182024-06-28T11:41:00.1131879Z     raise JSONRPCException(response['error'], status)
192024-06-28T11:41:00.1139938Z test_framework.authproxy.JSONRPCException: The combination of the pre-selected inputs and the wallet automatic inputs selection exceeds the transaction maximum weight. Please try sending a smaller amount or manually consolidating your wallet's UTXOs (-4)
202024-06-28T11:41:00.1141911Z 
212024-06-28T11:41:00.1142328Z During handling of the above exception, another exception occurred:
222024-06-28T11:41:00.1143194Z 
232024-06-28T11:41:00.1143417Z Traceback (most recent call last):
242024-06-28T11:41:00.1145069Z   File "/home/runner/work/_temp/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
252024-06-28T11:41:00.1146447Z     self.run_test()
262024-06-28T11:41:00.1148039Z   File "/home/runner/work/_temp/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_fundrawtransaction.py", line 117, in run_test
272024-06-28T11:41:00.1149453Z     self.test_weight_limits()
282024-06-28T11:41:00.1151071Z   File "/home/runner/work/_temp/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_fundrawtransaction.py", line 1335, in test_weight_limits
292024-06-28T11:41:00.1157890Z     assert_raises_rpc_error(-4, "Transaction too large", wallet.fundrawtransaction, hexstring=rawtx, input_weights=input_weights)
302024-06-28T11:41:00.1160275Z   File "/home/runner/work/_temp/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 156, in assert_raises_rpc_error
312024-06-28T11:41:00.1161994Z     assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
322024-06-28T11:41:00.1162821Z            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
332024-06-28T11:41:00.1174793Z   File "/home/runner/work/_temp/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 171, in try_rpc
342024-06-28T11:41:00.1176133Z     raise AssertionError(
352024-06-28T11:41:00.1176759Z AssertionError: Expected substring not found in error message:
362024-06-28T11:41:00.1177569Z substring: 'Transaction too large'
372024-06-28T11:41:00.1179589Z error message: 'The combination of the pre-selected inputs and the wallet automatic inputs selection exceeds the transaction maximum weight. Please try sending a smaller amount or manually consolidating your wallet's UTXOs'.
382024-06-28T11:41:00.1181671Z 2024-06-28T11:40:57.728000Z TestFramework (INFO): Stopping nodes
392024-06-28T11:41:00.1184029Z 2024-06-28T11:40:58.407000Z TestFramework (WARNING): Not cleaning up dir /home/runner/work/_temp/ci/scratch/test_runner/test_runner_₿_🏃_20240628_113945/wallet_fundrawtransaction_297
402024-06-28T11:41:00.1187216Z 2024-06-28T11:40:58.407000Z TestFramework (ERROR): Test failed. Test logging available at /home/runner/work/_temp/ci/scratch/test_runner/test_runner_₿_🏃_20240628_113945/wallet_fundrawtransaction_297/test_framework.log
412024-06-28T11:41:00.1189460Z 2024-06-28T11:40:58.407000Z TestFramework (ERROR): 
422024-06-28T11:41:00.1192381Z 2024-06-28T11:40:58.408000Z TestFramework (ERROR): Hint: Call /home/runner/work/_temp/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/combine_logs.py '/home/runner/work/_temp/ci/scratch/test_runner/test_runner_₿_🏃_20240628_113945/wallet_fundrawtransaction_297' to consolidate all logs
432024-06-28T11:41:00.1195268Z 2024-06-28T11:40:58.408000Z TestFramework (ERROR): 
442024-06-28T11:41:00.1197110Z 2024-06-28T11:40:58.408000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
452024-06-28T11:41:00.1199145Z 2024-06-28T11:40:58.408000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
462024-06-28T11:41:00.1200240Z 2024-06-28T11:40:58.408000Z TestFramework (ERROR):

    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.

  47. descriptor: don't underestimate the size of a Taproot spend 
    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>
    f74a668f2c
  48. darosior force-pushed on Jun 28, 2024
  49. darosior commented at 3:40 pm on June 29, 2024: member
    Force-pushed to add some Assumes. Looks like the CI issue was unrelated.
  50. DrahtBot added the label CI failed on Sep 6, 2024
  51. DrahtBot removed the label CI failed on Sep 15, 2024
  52. achow101 requested review from achow101 on Oct 15, 2024
  53. in test/functional/wallet_taproot.py:304 in f74a668f2c 
    298@@ -299,8 +299,9 @@ def do_test_sendtoaddress(self, comment, pattern, privmap, treefn, keys_pay, key
299             self.generatetoaddress(self.nodes[0], 1, self.boring.getnewaddress(), sync_fun=self.no_op)
300             test_balance = int(rpc_online.getbalance() * 100000000)
301             ret_amnt = random.randrange(100000, test_balance)
302-            # Increase fee_rate to compensate for the wallet's inability to estimate fees for script path spends.
303-            res = rpc_online.sendtoaddress(address=self.boring.getnewaddress(), amount=Decimal(ret_amnt) / 100000000, subtractfeefromamount=True, fee_rate=200)
304+            res = rpc_online.sendtoaddress(address=self.boring.getnewaddress(), amount=Decimal(ret_amnt) / 100000000, subtractfeefromamount=True, fee_rate=10)
305+            txinfo = rpc_online.gettransaction(txid=res, verbose=True)
306+            assert_fee_enough(-txinfo["fee"], txinfo["decoded"]["vsize"], Decimal(0.00010000))
    murchandamus commented at 4:59 pm on February 6, 2025:
    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?
  54. murchandamus commented at 4:59 pm on February 6, 2025: contributor
    Concept ACK
  55. furszy commented at 8:05 pm on February 6, 2025: member
    Concept ACK. Will review soon.


darosior DrahtBot luke-jr achow101 fanquake murchandamus sipa furszy


sipa achow101

Labels
Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-02-23 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me