Merge #19396: refactor: Remove confusing OutputType::CHANGE_AUTO

fa927ff884ae373c676deed63180a8d238872cdc Enable Wswitch for OutputType (MarcoFalke)
faddad71f648ed99734f4f8811bd4bc7232ca670 Remove confusing OutputType::CHANGE_AUTO (MarcoFalke)
fa2eb383522249a5f4d48726c520cec5de496614 interfaces: Remove unused getDefaultChangeType (MarcoFalke)

Pull request description:

  `OutputType::CHANGE_AUTO` is problematic for several reasons:

  * An output that is not change must never be described by `CHANGE_AUTO`. Simply allowing that option makes the code confusing and review harder than it needs to be.
  * To make review even harder, `CHANGE_AUTO` requires `-Wswitch` to be disabled for `OutputType`

  Fix both issues by removing `CHANGE_AUTO` and then enabling `-Wswitch` for `OutputType`

ACKs for top commit:
  promag:
    Code review ACK fa927ff884ae373c676deed63180a8d238872cdc.
  laanwj:
    Code review ACK fa927ff884ae373c676deed63180a8d238872cdc

Tree-SHA512: 24fd809757aa343866c94dafe9a7130b50cda4f77c97666d407f99b813f75b115a7d8e688a6bc2a737e87cba64ddd4e43f2b3c5538fd35fabb5845807bb39134

src/interfaces/wallet.cpp

@@ -438,7 +438,6 @@ public:
     bool canGetAddresses() override { return m_wallet->CanGetAddresses(); }
     bool privateKeysDisabled() override { return m_wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS); }
     OutputType getDefaultAddressType() override { return m_wallet->m_default_address_type; }
-    OutputType getDefaultChangeType() override { return m_wallet->m_default_change_type; }
     CAmount getDefaultMaxTxFee() override { return m_wallet->m_default_max_tx_fee; }
     void remove() override
     {

src/interfaces/wallet.h

@@ -256,9 +256,6 @@ public:
     // Get default address type.
     virtual OutputType getDefaultAddressType() = 0;
 
-    // Get default change type.
-    virtual OutputType getDefaultChangeType() = 0;
-
     //! Get max tx fee.
     virtual CAmount getDefaultMaxTxFee() = 0;
 

src/outputtype.cpp

@@ -42,8 +42,8 @@ const std::string& FormatOutputType(OutputType type)
     case OutputType::LEGACY: return OUTPUT_TYPE_STRING_LEGACY;
     case OutputType::P2SH_SEGWIT: return OUTPUT_TYPE_STRING_P2SH_SEGWIT;
     case OutputType::BECH32: return OUTPUT_TYPE_STRING_BECH32;
-    default: assert(false);
+    } // no default case, so the compiler can warn about missing cases
-    }
+    assert(false);
 }
 
 CTxDestination GetDestinationForKey(const CPubKey& key, OutputType type)
@@ -61,8 +61,8 @@ CTxDestination GetDestinationForKey(const CPubKey& key, OutputType type)
             return witdest;
         }
     }
-    default: assert(false);
+    } // no default case, so the compiler can warn about missing cases
-    }
+    assert(false);
 }
 
 std::vector<CTxDestination> GetAllDestinationsForKey(const CPubKey& key)
@@ -100,6 +100,6 @@ CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore,
             return ScriptHash(witprog);
         }
     }
-    default: assert(false);
+    } // no default case, so the compiler can warn about missing cases
-    }
+    assert(false);
 }

src/outputtype.h

@@ -18,14 +18,6 @@ enum class OutputType {
     LEGACY,
     P2SH_SEGWIT,
     BECH32,
-
-    /**
-     * Special output type for change outputs only. Automatically choose type
-     * based on address type setting and the types other of non-change outputs
-     * (see -changetype option documentation and implementation in
-     * CWallet::TransactionChangeType for details).
-     */
-    CHANGE_AUTO,
 };
 
 extern const std::array<OutputType, 3> OUTPUT_TYPES;

src/wallet/rpcwallet.cpp

@@ -306,7 +306,7 @@ static UniValue getrawchangeaddress(const JSONRPCRequest& request)
         throw JSONRPCError(RPC_WALLET_ERROR, "Error: This wallet has no available keys");
     }
 
-    OutputType output_type = pwallet->m_default_change_type != OutputType::CHANGE_AUTO ? pwallet->m_default_change_type : pwallet->m_default_address_type;
+    OutputType output_type = pwallet->m_default_change_type.get_value_or(pwallet->m_default_address_type);
     if (!request.params[0].isNull()) {
         if (!ParseOutputType(request.params[0].get_str(), output_type)) {
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[0].get_str()));
@@ -2993,10 +2993,11 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f
             if (options.exists("changeAddress")) {
                 throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both changeAddress and address_type options");
             }
-            coinControl.m_change_type = pwallet->m_default_change_type;
+            OutputType out_type;
-            if (!ParseOutputType(options["change_type"].get_str(), *coinControl.m_change_type)) {
+            if (!ParseOutputType(options["change_type"].get_str(), out_type)) {
                 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown change type '%s'", options["change_type"].get_str()));
             }
+            coinControl.m_change_type.emplace(out_type);
         }
 
         coinControl.fAllowWatchOnly = ParseIncludeWatchonly(options["includeWatching"], *pwallet);

src/wallet/scriptpubkeyman.cpp

@@ -1900,8 +1900,8 @@ bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_
         desc_prefix = "wpkh(" + xpub + "/84'";
         break;
     }
-    default: assert(false);
+    } // no default case, so the compiler can warn about missing cases
-    }
+    assert(!desc_prefix.empty());
 
     // Mainnet derives at 0', testnet and regtest derive at 1'
     if (Params().IsTestChain()) {

src/wallet/wallet.cpp

@@ -2669,11 +2669,11 @@ static uint32_t GetLocktimeForNewTransaction(interfaces::Chain& chain, const uin
     return locktime;
 }
 
-OutputType CWallet::TransactionChangeType(OutputType change_type, const std::vector<CRecipient>& vecSend)
+OutputType CWallet::TransactionChangeType(const Optional<OutputType>& change_type, const std::vector<CRecipient>& vecSend)
 {
     // If -changetype is specified, always use that change type.
-    if (change_type != OutputType::CHANGE_AUTO) {
+    if (change_type) {
-        return change_type;
+        return *change_type;
     }
 
     // if m_default_address_type is legacy, use legacy address as change (even
@@ -3842,15 +3842,21 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
         }
     }
 
-    if (!gArgs.GetArg("-addresstype", "").empty() && !ParseOutputType(gArgs.GetArg("-addresstype", ""), walletInstance->m_default_address_type)) {
+    if (!gArgs.GetArg("-addresstype", "").empty()) {
+        if (!ParseOutputType(gArgs.GetArg("-addresstype", ""), walletInstance->m_default_address_type)) {
             error = strprintf(_("Unknown address type '%s'"), gArgs.GetArg("-addresstype", ""));
             return nullptr;
         }
+    }
 
-    if (!gArgs.GetArg("-changetype", "").empty() && !ParseOutputType(gArgs.GetArg("-changetype", ""), walletInstance->m_default_change_type)) {
+    if (!gArgs.GetArg("-changetype", "").empty()) {
+        OutputType out_type;
+        if (!ParseOutputType(gArgs.GetArg("-changetype", ""), out_type)) {
             error = strprintf(_("Unknown change type '%s'"), gArgs.GetArg("-changetype", ""));
             return nullptr;
         }
+        walletInstance->m_default_change_type = out_type;
+    }
 
     if (gArgs.IsArgSet("-mintxfee")) {
         CAmount n = 0;

src/wallet/wallet.h

@@ -105,9 +105,6 @@ class ReserveDestination;
 //! Default for -addresstype
 constexpr OutputType DEFAULT_ADDRESS_TYPE{OutputType::BECH32};
 
-//! Default for -changetype
-constexpr OutputType DEFAULT_CHANGE_TYPE{OutputType::CHANGE_AUTO};
-
 static constexpr uint64_t KNOWN_WALLET_FLAGS =
         WALLET_FLAG_AVOID_REUSE
     |   WALLET_FLAG_BLANK_WALLET
@@ -934,7 +931,7 @@ public:
     Balance GetBalance(int min_depth = 0, bool avoid_reuse = true) const;
     CAmount GetAvailableBalance(const CCoinControl* coinControl = nullptr) const;
 
-    OutputType TransactionChangeType(OutputType change_type, const std::vector<CRecipient>& vecSend);
+    OutputType TransactionChangeType(const Optional<OutputType>& change_type, const std::vector<CRecipient>& vecSend);
 
     /**
      * Insert additional inputs into the transaction by
@@ -1012,7 +1009,13 @@ public:
     CFeeRate m_fallback_fee{DEFAULT_FALLBACK_FEE};
     CFeeRate m_discard_rate{DEFAULT_DISCARD_FEE};
     OutputType m_default_address_type{DEFAULT_ADDRESS_TYPE};
-    OutputType m_default_change_type{DEFAULT_CHANGE_TYPE};
+    /**
+     * Default output type for change outputs. When unset, automatically choose type
+     * based on address type setting and the types other of non-change outputs
+     * (see -changetype option documentation and implementation in
+     * CWallet::TransactionChangeType for details).
+     */
+    Optional<OutputType> m_default_change_type{};
     /** Absolute maximum transaction fee (in satoshis) used by default for the wallet */
     CAmount m_default_max_tx_fee{DEFAULT_TRANSACTION_MAXFEE};