Merge 3ac3805 into merged_master (Bitcoin PR bitcoin/bitcoin#23789)

src/wallet/wallet.cpp

@@ -2212,29 +2212,58 @@ OutputType CWallet::TransactionChangeType(const std::optional<OutputType>& chang
         return *change_type;
     }
 
-    // if m_default_address_type is legacy, use legacy address as change (even
-    // if some of the outputs are P2WPKH or P2WSH).
+    // if m_default_address_type is legacy, use legacy address as change.
     if (m_default_address_type == OutputType::LEGACY) {
         return OutputType::LEGACY;
     }
 
-    // if any destination is P2WPKH or P2WSH, use P2WPKH for the change
-    // output.
+    bool any_tr{false};
+    bool any_wpkh{false};
+    bool any_sh{false};
+    bool any_pkh{false};
+
     for (const auto& recipient : vecSend) {
-        // Check if any destination contains a witness program:
-        int witnessversion = 0;
-        std::vector<unsigned char> witnessprogram;
-        if (recipient.scriptPubKey.IsWitnessProgram(witnessversion, witnessprogram)) {
-            if (GetScriptPubKeyMan(OutputType::BECH32M, true)) {
-                return OutputType::BECH32M;
-            } else if (GetScriptPubKeyMan(OutputType::BECH32, true)) {
-                return OutputType::BECH32;
-            } else {
-                return m_default_address_type;
-            }
-        }
+        std::vector<std::vector<uint8_t>> dummy;
+        const TxoutType type{Solver(recipient.scriptPubKey, dummy)};
+        if (type == TxoutType::WITNESS_V1_TAPROOT) {
+            any_tr = true;
+        } else if (type == TxoutType::WITNESS_V0_KEYHASH) {
+            any_wpkh = true;
+        } else if (type == TxoutType::SCRIPTHASH) {
+            any_sh = true;
+        } else if (type == TxoutType::PUBKEYHASH) {
+            any_pkh = true;
+        }
+    }
+
+    const bool has_bech32m_spkman(GetScriptPubKeyMan(OutputType::BECH32M, /*internal=*/true));
+    if (has_bech32m_spkman && any_tr) {
+        // Currently tr is the only type supported by the BECH32M spkman
+        return OutputType::BECH32M;
+    }
+    const bool has_bech32_spkman(GetScriptPubKeyMan(OutputType::BECH32, /*internal=*/true));
+    if (has_bech32_spkman && any_wpkh) {
+        // Currently wpkh is the only type supported by the BECH32 spkman
+        return OutputType::BECH32;
+    }
+    const bool has_p2sh_segwit_spkman(GetScriptPubKeyMan(OutputType::P2SH_SEGWIT, /*internal=*/true));
+    if (has_p2sh_segwit_spkman && any_sh) {
+        // Currently sh_wpkh is the only type supported by the P2SH_SEGWIT spkman
+        // As of 2021 about 80% of all SH are wrapping WPKH, so use that
+        return OutputType::P2SH_SEGWIT;
+    }
+    const bool has_legacy_spkman(GetScriptPubKeyMan(OutputType::LEGACY, /*internal=*/true));
+    if (has_legacy_spkman && any_pkh) {
+        // Currently pkh is the only type supported by the LEGACY spkman
+        return OutputType::LEGACY;
     }
 
+    if (has_bech32m_spkman) {
+        return OutputType::BECH32M;
+    }
+    if (has_bech32_spkman) {
+        return OutputType::BECH32;
+    }
     // else use m_default_address_type for change
     return m_default_address_type;
 }

test/functional/rpc_fundrawtransaction.py

@@ -605,12 +605,12 @@ def test_locked_wallet(self):
         if self.options.descriptors:
             self.nodes[1].walletpassphrase('test', 10)
             self.nodes[1].importdescriptors([{
-                'desc': descsum_create('tr(tprv8ZgxMBicQKsPdYeeZbPSKd2KYLmeVKtcFA7kqCxDvDR13MQ6us8HopUR2wLcS2ZKPhLyKsqpDL2FtL73LMHcgoCL7DXsciA8eX8nbjCR2eG/0h/*h)'),
+                'desc': descsum_create('wpkh(tprv8ZgxMBicQKsPdYeeZbPSKd2KYLmeVKtcFA7kqCxDvDR13MQ6us8HopUR2wLcS2ZKPhLyKsqpDL2FtL73LMHcgoCL7DXsciA8eX8nbjCR2eG/0h/*h)'),
                 'timestamp': 'now',
                 'active': True
             },
             {
-                'desc': descsum_create('tr(tprv8ZgxMBicQKsPdYeeZbPSKd2KYLmeVKtcFA7kqCxDvDR13MQ6us8HopUR2wLcS2ZKPhLyKsqpDL2FtL73LMHcgoCL7DXsciA8eX8nbjCR2eG/1h/*h)'),
+                'desc': descsum_create('wpkh(tprv8ZgxMBicQKsPdYeeZbPSKd2KYLmeVKtcFA7kqCxDvDR13MQ6us8HopUR2wLcS2ZKPhLyKsqpDL2FtL73LMHcgoCL7DXsciA8eX8nbjCR2eG/1h/*h)'),
                 'timestamp': 'now',
                 'active': True,
                 'internal': True
@@ -827,18 +827,10 @@ 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)
 
-        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.00001386, 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": 1000}) # ELEMENTS: reduce by 10x
-            assert_approx(result["fee"], vexp=0.01386, 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.00001374, 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": 1000}) # ELEMENTS: reduce by 10x
-            assert_approx(result["fee"], vexp=0.01374, vspan=0.0001)
+        assert_approx(node.fundrawtransaction(rawtx)["fee"], vexp=0.00001374, 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": 1000}) # ELEMENTS: reduce by 10x
+        assert_approx(result["fee"], vexp=0.01374, vspan=0.0001)
 
         self.log.info("Test fundrawtxn with invalid estimate_mode settings")
         for k, v in {"number": 42, "object": {"foo": "bar"}}.items():
@@ -1164,7 +1156,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, ["-maxtxfee=10000000", f"-minrelaytxfee={feerate}", "-discardfee=0", "-changetype=bech32", "-addresstype=bech32"]) # Set high minrelayfee, set discardfee to 0 for easier calculation
+        self.restart_node(0, ["-maxtxfee=10000000", f"-minrelaytxfee={feerate}", "-discardfee=0"]) # 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/wallet_address_types.py

@@ -351,13 +351,13 @@ def run_test(self):
         self.test_change_output_type(0, [to_address_bech32_1], 'legacy')
 
         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.log.info("Nodes with addresstype=p2sh-segwit match the change output")
             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')
+            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')
         else:
-            self.log.info("Nodes with addresstype=p2sh-segwit only use a P2WPKH change output if any destination address is bech32:")
+            self.log.info("Nodes with addresstype=p2sh-segwit match the change output")
             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')
@@ -368,13 +368,13 @@ def run_test(self):
         self.test_change_output_type(2, [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')
+            self.log.info("Nodes with addresstype=bech32 match the 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], 'p2sh-segwit')
         else:
-            self.log.info("Nodes with addresstype=bech32 always use a P2WPKH change output (unless changetype is set otherwise):")
+            self.log.info("Nodes with addresstype=bech32 match the 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.test_change_output_type(3, [to_address_p2sh], 'p2sh-segwit')
 
         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')

test/functional/wallet_balance.py

@@ -249,7 +249,7 @@ def test_balances(*, fee_node_1=0):
             struct.pack(">q", 99 * 10**8).hex(),
             struct.pack(">q", 98 * 10**8).hex(),
         )
-        fee = 6560 if self.options.descriptors else 6520
+        fee = 6520
         tx_replace = tx_replace.replace( ## is there something less fragile we can do here?
             struct.pack(">q", fee).hex(),
             struct.pack(">q", fee + 10**8).hex(),

test/functional/wallet_groups.py

@@ -114,17 +114,10 @@ def run_test(self):
         assert_equal(input_addrs[0], input_addrs[1])
         # Node 2 enforces avoidpartialspends so needs no checking here
 
-        if self.options.descriptors:
-            # Descriptor wallets will use Taproot change by default which has different fees
-            tx4_ungrouped_fee = 3060
-            tx4_grouped_fee = 4400
-            tx5_6_ungrouped_fee = 5760
-            tx5_6_grouped_fee = 8480
-        else:
-            tx4_ungrouped_fee = 2820
-            tx4_grouped_fee = 4160
-            tx5_6_ungrouped_fee = 5520
-            tx5_6_grouped_fee = 8240
+        tx4_ungrouped_fee = 2820
+        tx4_grouped_fee = 4160
+        tx5_6_ungrouped_fee = 5520
+        tx5_6_grouped_fee = 8240
 
         self.log.info("Test wallet option maxapsfee")
         addr_aps = self.nodes[3].getnewaddress()

test/functional/wallet_hd.py

@@ -138,7 +138,7 @@ def run_test(self):
                 keypath = self.nodes[1].getaddressinfo(out['scriptPubKey']['address'])['hdkeypath']
 
         if self.options.descriptors:
-            assert_equal(keypath[0:14], "m/86'/1'/0'/1/")
+            assert_equal(keypath[0:14], "m/84'/1'/0'/1/")
         else:
             assert_equal(keypath[0:7], "m/0'/1'")