Use largest stack size when estimating size

achow101 · achow101 · commit 5345b8000539 · 2022-11-04T16:23:04.000-04:00
When estimating the size of a taproot witness (i.e.
DummySignatureCreator is being used), use the largest stack size rather
than the smallest. This avoids underpaying fees.

Also updates the tests to make sure that Taproot spends do not
underestimate the fee
diff --git a/src/script/sign.cpp b/src/script/sign.cpp
@@ -226,6 +226,11 @@ static bool SignTaproot(const SigningProvider& provider, const BaseSignatureCrea
         sigdata.tr_builder = builder;
     }
 
+    // Detect if this is the dummy signer. If so, we want to choose the largest stack size for worst case size estimation
+    bool use_largest = creator.UseLargest();
+    std::vector<std::vector<unsigned char>> largest_result_stack;
+    uint64_t largest_result_stack_size = 0;
+
     // Try key path spending.
     {
         KeyOriginInfo info;
@@ -248,26 +253,43 @@ static bool SignTaproot(const SigningProvider& provider, const BaseSignatureCrea
             }
         }
         if (sigdata.taproot_key_path_sig.size()) {
-            result = Vector(sigdata.taproot_key_path_sig);
-            return true;
+            if (use_largest) {
+                largest_result_stack = Vector(sigdata.taproot_key_path_sig);
+                largest_result_stack_size = GetSerializeSize(largest_result_stack, PROTOCOL_VERSION);
+            } else {
+                result = Vector(sigdata.taproot_key_path_sig);
+                return true;
+            }
         }
     }
 
     // Try script path spending.
     std::vector<std::vector<unsigned char>> smallest_result_stack;
+    uint64_t smallest_result_stack_size = 0;
     for (const auto& [key, control_blocks] : sigdata.tr_spenddata.scripts) {
         const auto& [script, leaf_ver] = key;
         std::vector<std::vector<unsigned char>> result_stack;
         if (SignTaprootScript(provider, creator, sigdata, leaf_ver, script, result_stack)) {
             result_stack.emplace_back(std::begin(script), std::end(script)); // Push the script
             result_stack.push_back(*control_blocks.begin()); // Push the smallest control block
-            if (smallest_result_stack.size() == 0 ||
-                GetSerializeSize(result_stack, PROTOCOL_VERSION) < GetSerializeSize(smallest_result_stack, PROTOCOL_VERSION)) {
-                smallest_result_stack = std::move(result_stack);
+            uint64_t stack_size = GetSerializeSize(result_stack, PROTOCOL_VERSION);
+            if (smallest_result_stack.empty() ||
+                stack_size < smallest_result_stack_size) {
+                smallest_result_stack = result_stack;
+                smallest_result_stack_size = stack_size;
+            }
+            if (use_largest && (largest_result_stack.empty() ||
+                stack_size > largest_result_stack_size)) {
+                largest_result_stack = result_stack;
+                largest_result_stack_size = stack_size;
             }
         }
     }
-    if (smallest_result_stack.size() != 0) {
+    if (use_largest && !largest_result_stack.empty()) {
+        result = std::move(largest_result_stack);
+        return true;
+    }
+    if (!smallest_result_stack.empty()) {
         result = std::move(smallest_result_stack);
         return true;
     }
diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
@@ -51,6 +51,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 assert_equal(thing1, thing2, *args):
     if thing1 != thing2 or any(thing1 != arg for arg in args):
         raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
diff --git a/test/functional/wallet_taproot.py b/test/functional/wallet_taproot.py
@@ -11,7 +11,7 @@
 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,
@@ -296,8 +296,9 @@ def do_test_sendtoaddress(self, comment, pattern, privmap, treefn, keys_pay, key
             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)
 
@@ -348,8 +349,7 @@ def do_test_psbt(self, comment, pattern, privmap, treefn, keys_pay, keys_change)
             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)
@@ -371,6 +371,8 @@ def do_test_psbt(self, comment, pattern, privmap, treefn, keys_pay, keys_change)
                 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)
 
