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>

src/script/descriptor.cpp

@@ -1144,13 +1144,37 @@ public:
     std::optional<int64_t> ScriptSize() const override { return 1 + 1 + 32; }
 
     std::optional<int64_t> 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<int64_t> 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;
     }
 };
 

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)

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