Merge bitcoin/bitcoin#22364: wallet: Make a tr() descriptor by default

4868c9f Extract Taproot internal keyid with GetKeyFromDestination (Andrew Chow)
d8abbe1 Mention bech32m in -addresstype and -changetype help (Andrew Chow)
8fb5784 Create a tr() descriptor bech32m DescriptorScriptPubKeyMan by default (Andrew Chow)
54b3699 Store pubkeys in TRDescriptor::MakeScripts (Andrew Chow)

Pull request description:

  Make a `tr()` descriptor by default in descriptor wallets so that users will be able to make and use segwit v1 bech32m addresses.

ACKs for top commit:
  MarcoFalke:
    Concept ACK 4868c9f
  Sjors:
    re-utACK 4868c9f
  gruve-p:
    ACK bitcoin/bitcoin@4868c9f
  meshcollider:
    Concept + code review ACK 4868c9f

Tree-SHA512: e5896e665b8d559f1d759b6582d1bb24f70d4698a57307684339d9fdcdac28ae9bc17bc946a7efec9cb35c130a95ffc36e3961a335124ec4535d77b8d00e9631

Author: MarcoFalke

src/script/descriptor.cpp

@@ -851,6 +851,7 @@ class TRDescriptor final : public DescriptorImpl
         builder.Finalize(xpk);
         WitnessV1Taproot output = builder.GetOutput();
         out.tr_spenddata[output].Merge(builder.GetSpendData());
+        out.pubkeys.emplace(keys[0].GetID(), keys[0]);
         return Vector(GetScriptForDestination(output));
     }
     bool ToStringSubScriptHelper(const SigningProvider* arg, std::string& ret, const StringType type, const DescriptorCache* cache = nullptr) const override

src/script/signingprovider.cpp

@@ -190,8 +190,8 @@ bool FillableSigningProvider::GetCScript(const CScriptID &hash, CScript& redeemS
 
 CKeyID GetKeyForDestination(const SigningProvider& store, const CTxDestination& dest)
 {
-    // Only supports destinations which map to single public keys, i.e. P2PKH,
-    // P2WPKH, and P2SH-P2WPKH.
+    // Only supports destinations which map to single public keys:
+    // P2PKH, P2WPKH, P2SH-P2WPKH, P2TR
     if (auto id = std::get_if<PKHash>(&dest)) {
         return ToKeyID(*id);
     }
@@ -208,5 +208,15 @@ CKeyID GetKeyForDestination(const SigningProvider& store, const CTxDestination&
             }
         }
     }
+    if (auto output_key = std::get_if<WitnessV1Taproot>(&dest)) {
+        TaprootSpendData spenddata;
+        CPubKey pub;
+        if (store.GetTaprootSpendData(*output_key, spenddata)
+            && !spenddata.internal_key.IsNull()
+            && spenddata.merkle_root.IsNull()
+            && store.GetPubKeyByXOnly(spenddata.internal_key, pub)) {
+            return pub.GetID();
+        }
+    }
     return CKeyID();
 }

src/wallet/init.cpp

@@ -43,9 +43,9 @@ const WalletInitInterface& g_wallet_init_interface = WalletInit();
 
 void WalletInit::AddWalletOptions(ArgsManager& argsman) const
 {
-    argsman.AddArg("-addresstype", strprintf("What type of addresses to use (\"legacy\", \"p2sh-segwit\", or \"bech32\", default: \"%s\")", FormatOutputType(DEFAULT_ADDRESS_TYPE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
+    argsman.AddArg("-addresstype", strprintf("What type of addresses to use (\"legacy\", \"p2sh-segwit\", \"bech32\", or \"bech32m\", default: \"%s\")", FormatOutputType(DEFAULT_ADDRESS_TYPE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
     argsman.AddArg("-avoidpartialspends", strprintf("Group outputs by address, selecting many (possibly all) or none, instead of selecting on a per-output basis. Privacy is improved as addresses are mostly swept with fewer transactions and outputs are aggregated in clean change addresses. It may result in higher fees due to less optimal coin selection caused by this added limitation and possibly a larger-than-necessary number of inputs being used. Always enabled for wallets with \"avoid_reuse\" enabled, otherwise default: %u.", DEFAULT_AVOIDPARTIALSPENDS), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
-    argsman.AddArg("-changetype", "What type of change to use (\"legacy\", \"p2sh-segwit\", or \"bech32\"). Default is same as -addresstype, except when -addresstype=p2sh-segwit a native segwit output is used when sending to a native segwit address)", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
+    argsman.AddArg("-changetype", "What type of change to use (\"legacy\", \"p2sh-segwit\", \"bech32\", or \"bech32m\"). Default is same as -addresstype, except when -addresstype=p2sh-segwit a native segwit output is used when sending to a native segwit address)", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
     argsman.AddArg("-consolidatefeerate=<amt>", strprintf("The maximum feerate (in %s/kvB) at which transaction building may use more inputs than strictly necessary so that the wallet's UTXO pool can be reduced (default: %s).", CURRENCY_UNIT, FormatMoney(DEFAULT_CONSOLIDATE_FEERATE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
     argsman.AddArg("-disablewallet", "Do not load the wallet and disable wallet RPC calls", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
     argsman.AddArg("-discardfee=<amt>", strprintf("The fee rate (in %s/kvB) that indicates your tolerance for discarding change by adding it to the fee (default: %s). "

src/wallet/scriptpubkeyman.cpp

@@ -1876,12 +1876,6 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const
 
 bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_key, OutputType addr_type, bool internal)
 {
-    if (addr_type == OutputType::BECH32M) {
-        // Don't allow setting up taproot descriptors yet
-        // TODO: Allow setting up taproot descriptors
-        return false;
-    }
-
     LOCK(cs_desc_man);
     assert(m_storage.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS));
 
@@ -1911,7 +1905,10 @@ bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_
         desc_prefix = "wpkh(" + xpub + "/84'";
         break;
     }
-    case OutputType::BECH32M: assert(false); // TODO: Setup taproot descriptor
+    case OutputType::BECH32M: {
+        desc_prefix = "tr(" + xpub  + "/86'";
+        break;
+    }
     } // no default case, so the compiler can warn about missing cases
     assert(!desc_prefix.empty());
 

src/wallet/test/fuzz/notifications.cpp

@@ -68,9 +68,6 @@ struct FuzzedWallet {
     CScript GetScriptPubKey(FuzzedDataProvider& fuzzed_data_provider)
     {
         auto type{fuzzed_data_provider.PickValueInArray(OUTPUT_TYPES)};
-        if (type == OutputType::BECH32M) {
-            type = OutputType::BECH32; // TODO: Setup taproot descriptor and remove this line
-        }
         CTxDestination dest;
         bilingual_str error;
         if (fuzzed_data_provider.ConsumeBool()) {

src/wallet/wallet.cpp

@@ -3168,11 +3168,6 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
 
         for (bool internal : {false, true}) {
             for (OutputType t : OUTPUT_TYPES) {
-                if (t == OutputType::BECH32M) {
-                    // Skip taproot (bech32m) for now
-                    // TODO: Setup taproot (bech32m) descriptors by default
-                    continue;
-                }
                 auto spk_manager = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this));
                 if (IsCrypted()) {
                     if (IsLocked()) {

test/functional/rpc_fundrawtransaction.py

@@ -571,12 +571,12 @@ def test_locked_wallet(self):
         if self.options.descriptors:
             self.nodes[1].walletpassphrase('test', 10)
             self.nodes[1].importdescriptors([{
-                'desc': descsum_create('wpkh(tprv8ZgxMBicQKsPdYeeZbPSKd2KYLmeVKtcFA7kqCxDvDR13MQ6us8HopUR2wLcS2ZKPhLyKsqpDL2FtL73LMHcgoCL7DXsciA8eX8nbjCR2eG/0h/*h)'),
+                'desc': descsum_create('tr(tprv8ZgxMBicQKsPdYeeZbPSKd2KYLmeVKtcFA7kqCxDvDR13MQ6us8HopUR2wLcS2ZKPhLyKsqpDL2FtL73LMHcgoCL7DXsciA8eX8nbjCR2eG/0h/*h)'),
                 'timestamp': 'now',
                 'active': True
             },
             {
-                'desc': descsum_create('wpkh(tprv8ZgxMBicQKsPdYeeZbPSKd2KYLmeVKtcFA7kqCxDvDR13MQ6us8HopUR2wLcS2ZKPhLyKsqpDL2FtL73LMHcgoCL7DXsciA8eX8nbjCR2eG/1h/*h)'),
+                'desc': descsum_create('tr(tprv8ZgxMBicQKsPdYeeZbPSKd2KYLmeVKtcFA7kqCxDvDR13MQ6us8HopUR2wLcS2ZKPhLyKsqpDL2FtL73LMHcgoCL7DXsciA8eX8nbjCR2eG/1h/*h)'),
                 'timestamp': 'now',
                 'active': True,
                 'internal': True
@@ -778,11 +778,18 @@ def test_option_feerate(self):
         for param, zero_value in product(["fee_rate", "feeRate"], [0, 0.000, 0.00000000, "0", "0.000", "0.00000000"]):
             assert_equal(self.nodes[3].fundrawtransaction(rawtx, {param: zero_value})["fee"], 0)
 
-        # With no arguments passed, expect fee of 141 satoshis.
-        assert_approx(node.fundrawtransaction(rawtx)["fee"], vexp=0.00000141, vspan=0.00000001)
-        # Expect fee to be 10,000x higher when an explicit fee rate 10,000x greater is specified.
-        result = node.fundrawtransaction(rawtx, {"fee_rate": 10000})
-        assert_approx(result["fee"], vexp=0.0141, vspan=0.0001)
+        if self.options.descriptors:
+            # With no arguments passed, expect fee of 153 satoshis as descriptor wallets now have a taproot output.
+            assert_approx(node.fundrawtransaction(rawtx)["fee"], vexp=0.00000153, vspan=0.00000001)
+            # Expect fee to be 10,000x higher when an explicit fee rate 10,000x greater is specified.
+            result = node.fundrawtransaction(rawtx, {"fee_rate": 10000})
+            assert_approx(result["fee"], vexp=0.0153, vspan=0.0001)
+        else:
+            # With no arguments passed, expect fee of 141 satoshis as legacy wallets only support up to segwit v0.
+            assert_approx(node.fundrawtransaction(rawtx)["fee"], vexp=0.00000141, vspan=0.00000001)
+            # Expect fee to be 10,000x higher when an explicit fee rate 10,000x greater is specified.
+            result = node.fundrawtransaction(rawtx, {"fee_rate": 10000})
+            assert_approx(result["fee"], vexp=0.0141, vspan=0.0001)
 
         self.log.info("Test fundrawtxn with invalid estimate_mode settings")
         for k, v in {"number": 42, "object": {"foo": "bar"}}.items():
@@ -1073,7 +1080,7 @@ def test_22670(self):
         # Make sure the default wallet will not be loaded when restarted with a high minrelaytxfee
         self.nodes[0].unloadwallet(self.default_wallet_name, False)
         feerate = Decimal("0.1")
-        self.restart_node(0, [f"-minrelaytxfee={feerate}", "-discardfee=0"]) # Set high minrelayfee, set discardfee to 0 for easier calculation
+        self.restart_node(0, [f"-minrelaytxfee={feerate}", "-discardfee=0", "-changetype=bech32", "-addresstype=bech32"]) # Set high minrelayfee, set discardfee to 0 for easier calculation
 
         self.nodes[0].loadwallet(self.default_wallet_name, True)
         funds = self.nodes[0].get_wallet_rpc(self.default_wallet_name)

test/functional/rpc_psbt.py

@@ -31,7 +31,7 @@ class PSBTTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 3
         self.extra_args = [
-            ["-walletrbf=1"],
+            ["-walletrbf=1", "-addresstype=bech32", "-changetype=bech32"], #TODO: Remove address type restrictions once taproot has psbt extensions
             ["-walletrbf=0", "-changetype=legacy"],
             []
         ]

test/functional/tool_wallet.py

@@ -70,8 +70,8 @@ def log_wallet_timestamp_comparison(self, old, new):
 
     def get_expected_info_output(self, name="", transactions=0, keypool=2, address=0):
         wallet_name = self.default_wallet_name if name == "" else name
-        output_types = 3  # p2pkh, p2sh, segwit
         if self.options.descriptors:
+            output_types = 4  # p2pkh, p2sh, segwit, bech32m
             return textwrap.dedent('''\
                 Wallet info
                 ===========
@@ -85,6 +85,7 @@ def get_expected_info_output(self, name="", transactions=0, keypool=2, address=0
                 Address Book: %d
             ''' % (wallet_name, keypool * output_types, transactions, address))
         else:
+            output_types = 3  # p2pkh, p2sh, segwit. Legacy wallets do not support bech32m.
             return textwrap.dedent('''\
                 Wallet info
                 ===========
@@ -298,8 +299,8 @@ def test_getwalletinfo_on_different_wallet(self):
             assert_equal(1000, out['keypoolsize_hd_internal'])
             assert_equal(True, 'hdseedid' in out)
         else:
-            assert_equal(3000, out['keypoolsize'])
-            assert_equal(3000, out['keypoolsize_hd_internal'])
+            assert_equal(4000, out['keypoolsize'])
+            assert_equal(4000, out['keypoolsize_hd_internal'])
 
         self.log_wallet_timestamp_comparison(timestamp_before, timestamp_after)
         assert_equal(timestamp_before, timestamp_after)

test/functional/wallet_address_types.py

@@ -121,6 +121,12 @@ def test_address(self, node, address, multisig, typ):
             assert_equal(info['witness_version'], 0)
             assert_equal(len(info['witness_program']), 40)
             assert 'pubkey' in info
+        elif not multisig and typ == "bech32m":
+            # P2TR single sig
+            assert info["isscript"]
+            assert info["iswitness"]
+            assert_equal(info["witness_version"], 1)
+            assert_equal(len(info["witness_program"]), 64)
         elif typ == 'legacy':
             # P2SH-multisig
             assert info['isscript']
@@ -339,19 +345,31 @@ def run_test(self):
         self.log.info("Nodes with addresstype=legacy never use a P2WPKH change output (unless changetype is set otherwise):")
         self.test_change_output_type(0, [to_address_bech32_1], 'legacy')
 
-        self.log.info("Nodes with addresstype=p2sh-segwit only use a P2WPKH change output if any destination address is bech32:")
-        self.test_change_output_type(1, [to_address_p2sh], 'p2sh-segwit')
-        self.test_change_output_type(1, [to_address_bech32_1], 'bech32')
-        self.test_change_output_type(1, [to_address_p2sh, to_address_bech32_1], 'bech32')
-        self.test_change_output_type(1, [to_address_bech32_1, to_address_bech32_2], 'bech32')
+        if self.options.descriptors:
+            self.log.info("Nodes with addresstype=p2sh-segwit only use a bech32m change output if any destination address is bech32:")
+            self.test_change_output_type(1, [to_address_p2sh], 'p2sh-segwit')
+            self.test_change_output_type(1, [to_address_bech32_1], 'bech32m')
+            self.test_change_output_type(1, [to_address_p2sh, to_address_bech32_1], 'bech32m')
+            self.test_change_output_type(1, [to_address_bech32_1, to_address_bech32_2], 'bech32m')
+        else:
+            self.log.info("Nodes with addresstype=p2sh-segwit only use a P2WPKH change output if any destination address is bech32:")
+            self.test_change_output_type(1, [to_address_p2sh], 'p2sh-segwit')
+            self.test_change_output_type(1, [to_address_bech32_1], 'bech32')
+            self.test_change_output_type(1, [to_address_p2sh, to_address_bech32_1], 'bech32')
+            self.test_change_output_type(1, [to_address_bech32_1, to_address_bech32_2], 'bech32')
 
         self.log.info("Nodes with change_type=bech32 always use a P2WPKH change output:")
         self.test_change_output_type(2, [to_address_bech32_1], 'bech32')
         self.test_change_output_type(2, [to_address_p2sh], 'bech32')
 
-        self.log.info("Nodes with addresstype=bech32 always use a P2WPKH change output (unless changetype is set otherwise):")
-        self.test_change_output_type(3, [to_address_bech32_1], 'bech32')
-        self.test_change_output_type(3, [to_address_p2sh], 'bech32')
+        if self.options.descriptors:
+            self.log.info("Nodes with addresstype=bech32 always use either a bech32 or bech32m change output (unless changetype is set otherwise):")
+            self.test_change_output_type(3, [to_address_bech32_1], 'bech32m')
+            self.test_change_output_type(3, [to_address_p2sh], 'bech32')
+        else:
+            self.log.info("Nodes with addresstype=bech32 always use a P2WPKH change output (unless changetype is set otherwise):")
+            self.test_change_output_type(3, [to_address_bech32_1], 'bech32')
+            self.test_change_output_type(3, [to_address_p2sh], 'bech32')
 
         self.log.info('getrawchangeaddress defaults to addresstype if -changetype is not set and argument is absent')
         self.test_address(3, self.nodes[3].getrawchangeaddress(), multisig=False, typ='bech32')
@@ -370,10 +388,9 @@ def run_test(self):
         self.test_address(4, self.nodes[4].getrawchangeaddress('bech32'), multisig=False, typ='bech32')
 
         if self.options.descriptors:
-            self.log.info("Descriptor wallets do not have bech32m addresses by default yet")
-            # TODO: Remove this when they do
-            assert_raises_rpc_error(-12, "Error: No bech32m addresses available", self.nodes[0].getnewaddress, "", "bech32m")
-            assert_raises_rpc_error(-12, "Error: No bech32m addresses available", self.nodes[0].getrawchangeaddress, "bech32m")
+            self.log.info("Descriptor wallets have bech32m addresses")
+            self.test_address(4, self.nodes[4].getnewaddress("", "bech32m"), multisig=False, typ="bech32m")
+            self.test_address(4, self.nodes[4].getrawchangeaddress("bech32m"), multisig=False, typ="bech32m")
         else:
             self.log.info("Legacy wallets cannot make bech32m addresses")
             assert_raises_rpc_error(-8, "Legacy wallets cannot provide bech32m addresses", self.nodes[0].getnewaddress, "", "bech32m")