From f74a668f2cad0da47522f99c854c328dc26296dd Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 25 Nov 2022 12:17:14 +0100 Subject: [PATCH] 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 --- src/script/descriptor.cpp | 32 ++++++++++++++++++++++---- test/functional/test_framework/util.py | 8 +++++++ test/functional/wallet_taproot.py | 12 ++++++---- 3 files changed, 43 insertions(+), 9 deletions(-) diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index 0987db194c4..76820d2626a 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -1144,13 +1144,37 @@ public: std::optional ScriptSize() const override { return 1 + 1 + 32; } std::optional MaxSatisfactionWeight(bool) const override { - // FIXME: We assume keypath spend, which can lead to very large underestimations. - return 1 + 65; + // Start from the size of a keypath spend. + int64_t max_weight = 1 + 65; + + // Then go through all the existing leaves to check if there is anything more expensive. + const bool dummy_max_sig = true; + for (size_t i = 0; i < m_subdescriptor_args.size(); ++i) { + // Anything inside a Tapscript leaf must have its satisfaction and script size set. + const auto sat_size = *Assume(m_subdescriptor_args[i]->MaxSatSize(dummy_max_sig)); + const auto script_size = *Assume(m_subdescriptor_args[i]->ScriptSize()); + const auto control_size = 33 + 32 * m_depths[i]; + const auto total_weight = GetSizeOfCompactSize(control_size) + control_size + GetSizeOfCompactSize(script_size) + script_size + GetSizeOfCompactSize(sat_size) + sat_size; + if (total_weight > max_weight) max_weight = total_weight; + } + + return max_weight; } std::optional MaxSatisfactionElems() const override { - // FIXME: See above, we assume keypath spend. - return 1; + // Start from the stack size of a keypath spend. + int64_t max_stack_size = 1; + + // Then go through all the existing leaves to check if there is anything more expensive. + for (size_t i = 0; i < m_subdescriptor_args.size(); ++i) { + // Anything inside a Tapscript leaf must have its satisfaction stack size set. + const auto sat_stack_size = *Assume(m_subdescriptor_args[i]->MaxSatisfactionElems()); + // Control block + script + script satisfaction + const auto stack_size = 1 + 1 + sat_stack_size; + if (stack_size > max_stack_size) max_stack_size = stack_size; + } + + return max_stack_size; } }; diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index c5b69a39549..c46dc9db2b8 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -52,6 +52,13 @@ def assert_fee_amount(fee, tx_size, feerate_BTC_kvB): raise AssertionError("Fee of %s BTC too high! (Should be %s BTC)" % (str(fee), str(target_fee))) +def assert_fee_enough(fee, tx_size, feerate_BTC_kvB): + """Assert the fee meets the feerate""" + target_fee = get_fee(tx_size, feerate_BTC_kvB) + if fee < target_fee: + raise AssertionError("Fee of %s BTC too low! (Should be at least %s BTC)" % (str(fee), str(target_fee))) + + def summarise_dict_differences(thing1, thing2): if not isinstance(thing1, dict) or not isinstance(thing2, dict): return thing1, thing2 @@ -66,6 +73,7 @@ def summarise_dict_differences(thing1, thing2): d2[k] = thing2[k] return d1, d2 + def assert_equal(thing1, thing2, *args): if thing1 != thing2 and not args and isinstance(thing1, dict) and isinstance(thing2, dict): d1,d2 = summarise_dict_differences(thing1, thing2) diff --git a/test/functional/wallet_taproot.py b/test/functional/wallet_taproot.py index a5d7445ce8d..ceedc9eca49 100755 --- a/test/functional/wallet_taproot.py +++ b/test/functional/wallet_taproot.py @@ -11,7 +11,7 @@ from decimal import Decimal from test_framework.address import output_key_to_p2tr from test_framework.key import H_POINT from test_framework.test_framework import BitcoinTestFramework -from test_framework.util import assert_equal +from test_framework.util import assert_equal, assert_fee_enough from test_framework.descriptors import descsum_create from test_framework.script import ( CScript, @@ -299,8 +299,9 @@ class WalletTaprootTest(BitcoinTestFramework): self.generatetoaddress(self.nodes[0], 1, self.boring.getnewaddress(), sync_fun=self.no_op) test_balance = int(rpc_online.getbalance() * 100000000) ret_amnt = random.randrange(100000, test_balance) - # Increase fee_rate to compensate for the wallet's inability to estimate fees for script path spends. - 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)) self.generatetoaddress(self.nodes[0], 1, self.boring.getnewaddress(), sync_fun=self.no_op) assert rpc_online.gettransaction(res)["confirmations"] > 0 @@ -351,8 +352,7 @@ class WalletTaprootTest(BitcoinTestFramework): self.generatetoaddress(self.nodes[0], 1, self.boring.getnewaddress(), sync_fun=self.no_op) test_balance = int(psbt_online.getbalance() * 100000000) ret_amnt = random.randrange(100000, test_balance) - # Increase fee_rate to compensate for the wallet's inability to estimate fees for script path spends. - psbt = psbt_online.walletcreatefundedpsbt([], [{self.boring.getnewaddress(): Decimal(ret_amnt) / 100000000}], None, {"subtractFeeFromOutputs":[0], "fee_rate": 200, "change_type": address_type})['psbt'] + psbt = psbt_online.walletcreatefundedpsbt([], [{self.boring.getnewaddress(): Decimal(ret_amnt) / 100000000}], None, {"subtractFeeFromOutputs":[0], "fee_rate": 10, "change_type": address_type})['psbt'] res = psbt_offline.walletprocesspsbt(psbt=psbt, finalize=False) for wallet in [psbt_offline, key_only_wallet]: res = wallet.walletprocesspsbt(psbt=psbt, finalize=False) @@ -374,6 +374,8 @@ class WalletTaprootTest(BitcoinTestFramework): assert res[0]["allowed"] txid = self.nodes[0].sendrawtransaction(rawtx) + txinfo = psbt_online.gettransaction(txid=txid, verbose=True) + assert_fee_enough(-txinfo["fee"], txinfo["decoded"]["vsize"], Decimal(0.00010000)) self.generatetoaddress(self.nodes[0], 1, self.boring.getnewaddress(), sync_fun=self.no_op) assert psbt_online.gettransaction(txid)['confirmations'] > 0