Merge #12119: [wallet] use P2WPKH change output if any destination is P2WPKH or P2WSH

596c446 [wallet] use P2WPKH change output if any destination is P2WPKH or P2WSH (Sjors Provoost)

Pull request description:

  If `-changetype` is not explicitly set, then regardless of `-addresstype`, the wallet will use a ~`bech32` change address~ `P2WPKH` change output if any destination is `P2WPKH` or `P2WSH`.

  This seems more intuitive to me and more in line with the spirit of [BIP-69](https://github.com/bitcoin/bips/blob/master/bip-0069.mediawiki).

  When combined with #11991 a QT user could opt to use `bech32` exclusively without having to figure out how to launch with `-changetype=bech32`, although so would #11937.

Tree-SHA512: 9238d3ccd1f3be8dfdd43444ccf45d6bdc6584ced3172a3045f3ecfec4a7cc8999db0cdb76ae49236492a84e6dbf3a1fdf18544d3eaf6d518e1f8bd241db33e7

5 files changed, 44 insertions, 9 deletions

diff --git a/src/qt/paymentserver.cpp b/src/qt/paymentserver.cpp
index dc729649b8..bc69d4f945 100644
--- a/src/qt/paymentserver.cpp
+++ b/src/qt/paymentserver.cpp
@@ -643,8 +643,9 @@ void PaymentServer::fetchPaymentACK(CWallet* wallet, const SendCoinsRecipient& r
         // use for change. Despite an actual payment and not change, this is a close match:
         // it's the output type we use subject to privacy issues, but not restricted by what
         // other software supports.
-        wallet->LearnRelatedScripts(newKey, g_change_type);
-        CTxDestination dest = GetDestinationForKey(newKey, g_change_type);
+        const OutputType change_type = g_change_type != OUTPUT_TYPE_NONE ? g_change_type : g_address_type;
+        wallet->LearnRelatedScripts(newKey, change_type);
+        CTxDestination dest = GetDestinationForKey(newKey, change_type);
         wallet->SetAddressBook(dest, strAccount, "refund");
 
         CScript s = GetScriptForDestination(dest);
diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp
index 2d26f7ae0f..c7f19bc90a 100644
--- a/src/wallet/init.cpp
+++ b/src/wallet/init.cpp
@@ -17,7 +17,7 @@ std::string GetWalletHelpString(bool showDebug)
 {
     std::string strUsage = HelpMessageGroup(_("Wallet options:"));
     strUsage += HelpMessageOpt("-addresstype", strprintf(_("What type of addresses to use (\"legacy\", \"p2sh-segwit\", or \"bech32\", default: \"%s\")"), FormatOutputType(OUTPUT_TYPE_DEFAULT)));
-    strUsage += HelpMessageOpt("-changetype", _("What type of change to use (\"legacy\", \"p2sh-segwit\", or \"bech32\", default is same as -addresstype)"));
+    strUsage += HelpMessageOpt("-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)"));
     strUsage += HelpMessageOpt("-disablewallet", _("Do not load the wallet and disable wallet RPC calls"));
     strUsage += HelpMessageOpt("-keypool=<n>", strprintf(_("Set key pool size to <n> (default: %u)"), DEFAULT_KEYPOOL_SIZE));
     strUsage += HelpMessageOpt("-fallbackfee=<amt>", strprintf(_("A fee rate (in %s/kB) that will be used when fee estimation has insufficient data (default: %s)"),
@@ -182,8 +182,10 @@ bool WalletParameterInteraction()
         return InitError(strprintf(_("Unknown address type '%s'"), gArgs.GetArg("-addresstype", "")));
     }
 
-    g_change_type = ParseOutputType(gArgs.GetArg("-changetype", ""), g_address_type);
-    if (g_change_type == OUTPUT_TYPE_NONE) {
+    // If changetype is set in config file or parameter, check that it's valid.
+    // Default to OUTPUT_TYPE_NONE if not set.
+    g_change_type = ParseOutputType(gArgs.GetArg("-changetype", ""), OUTPUT_TYPE_NONE);
+    if (g_change_type == OUTPUT_TYPE_NONE && !gArgs.GetArg("-changetype", "").empty()) {
         return InitError(strprintf(_("Unknown change type '%s'"), gArgs.GetArg("-changetype", "")));
     }
 
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index a77a886428..6849671dcf 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -257,9 +257,9 @@ UniValue getrawchangeaddress(const JSONRPCRequest& request)
         pwallet->TopUpKeyPool();
     }
 
-    OutputType output_type = g_change_type;
+    OutputType output_type = g_change_type != OUTPUT_TYPE_NONE ? g_change_type : g_address_type;
     if (!request.params[0].isNull()) {
-        output_type = ParseOutputType(request.params[0].get_str(), g_change_type);
+        output_type = ParseOutputType(request.params[0].get_str(), output_type);
         if (output_type == OUTPUT_TYPE_NONE) {
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[0].get_str()));
         }
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index dd9bc64728..7fb26900e1 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2674,6 +2674,34 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
     return true;
 }
 
+OutputType CWallet::TransactionChangeType(const std::vector<CRecipient>& vecSend)
+{
+    // If -changetype is specified, always use that change type.
+    if (g_change_type != OUTPUT_TYPE_NONE) {
+        return g_change_type;
+    }
+
+    // if g_address_type is legacy, use legacy address as change (even
+    // if some of the outputs are P2WPKH or P2WSH).
+    if (g_address_type == OUTPUT_TYPE_LEGACY) {
+        return OUTPUT_TYPE_LEGACY;
+    }
+
+    // if any destination is P2WPKH or P2WSH, use P2WPKH for the change
+    // output.
+    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)) {
+            return OUTPUT_TYPE_BECH32;
+        }
+    }
+
+    // else use g_address_type for change
+    return g_address_type;
+}
+
 bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletTx& wtxNew, CReserveKey& reservekey, CAmount& nFeeRet,
                                 int& nChangePosInOut, std::string& strFailReason, const CCoinControl& coin_control, bool sign)
 {
@@ -2769,8 +2797,10 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
                     return false;
                 }
 
-                LearnRelatedScripts(vchPubKey, g_change_type);
-                scriptChange = GetScriptForDestination(GetDestinationForKey(vchPubKey, g_change_type));
+                const OutputType change_type = TransactionChangeType(vecSend);
+
+                LearnRelatedScripts(vchPubKey, change_type);
+                scriptChange = GetScriptForDestination(GetDestinationForKey(vchPubKey, change_type));
             }
             CTxOut change_prototype_txout(0, scriptChange);
             size_t change_prototype_size = GetSerializeSize(change_prototype_txout, SER_DISK, 0);
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 354f524821..e8f536634e 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -965,6 +965,8 @@ public:
     CAmount GetLegacyBalance(const isminefilter& filter, int minDepth, const std::string* account) const;
     CAmount GetAvailableBalance(const CCoinControl* coinControl = nullptr) const;
 
+    OutputType TransactionChangeType(const std::vector<CRecipient>& vecSend);
+
     /**
      * Insert additional inputs into the transaction by
      * calling CreateTransaction();