Merge bitcoin/bitcoin#22621: make ParseOutputType return a std::optional<OutputType>

MarcoFalke · MarcoFalke · commit 4f1a75b1aa94 · 2021-08-04T19:02:04.000+02:00
32fa49a make ParseOutputType return a std::optional<OutputType> (fanquake) Pull request description: Similar to #22220. Skipped using `auto` here for the same reasons outlined in that PR. ACKs for top commit: jnewbery: utACK 32fa49a jonatack: Code review ACK 32fa49a and debian clang 13 debug build is clean / unit tests locally are green MarcoFalke: review ACK 32fa49a 🍢 Tree-SHA512: 7752193117669b800889226185d49d164395697853828f8acb568f07651789bc5b2cddc45555957450353886e46b9a1e13c77a5e730a14c6ee621fabc8dc3d10
diff --git a/src/outputtype.cpp b/src/outputtype.cpp
@@ -13,29 +13,26 @@
 #include <util/vector.h>
 
 #include <assert.h>
+#include <optional>
 #include <string>
 
 static const std::string OUTPUT_TYPE_STRING_LEGACY = "legacy";
 static const std::string OUTPUT_TYPE_STRING_P2SH_SEGWIT = "p2sh-segwit";
 static const std::string OUTPUT_TYPE_STRING_BECH32 = "bech32";
 static const std::string OUTPUT_TYPE_STRING_BECH32M = "bech32m";
 
-bool ParseOutputType(const std::string& type, OutputType& output_type)
+std::optional<OutputType> ParseOutputType(const std::string& type)
 {
     if (type == OUTPUT_TYPE_STRING_LEGACY) {
-        output_type = OutputType::LEGACY;
-        return true;
+        return OutputType::LEGACY;
     } else if (type == OUTPUT_TYPE_STRING_P2SH_SEGWIT) {
-        output_type = OutputType::P2SH_SEGWIT;
-        return true;
+        return OutputType::P2SH_SEGWIT;
     } else if (type == OUTPUT_TYPE_STRING_BECH32) {
-        output_type = OutputType::BECH32;
-        return true;
+        return OutputType::BECH32;
     } else if (type == OUTPUT_TYPE_STRING_BECH32M) {
-        output_type = OutputType::BECH32M;
-        return true;
+        return OutputType::BECH32M;
     }
-    return false;
+    return std::nullopt;
 }
 
 const std::string& FormatOutputType(OutputType type)
diff --git a/src/outputtype.h b/src/outputtype.h
@@ -11,6 +11,7 @@
 #include <script/standard.h>
 
 #include <array>
+#include <optional>
 #include <string>
 #include <vector>
 
@@ -28,7 +29,7 @@ static constexpr auto OUTPUT_TYPES = std::array{
     OutputType::BECH32M,
 };
 
-[[nodiscard]] bool ParseOutputType(const std::string& str, OutputType& output_type);
+std::optional<OutputType> ParseOutputType(const std::string& str);
 const std::string& FormatOutputType(OutputType type);
 
 /**
diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp
@@ -24,6 +24,7 @@
 #include <util/strencodings.h>
 #include <util/system.h>
 
+#include <optional>
 #include <stdint.h>
 #include <tuple>
 #ifdef HAVE_MALLOC_INFO
@@ -128,12 +129,13 @@ static RPCHelpMan createmultisig()
     // Get the output type
     OutputType output_type = OutputType::LEGACY;
     if (!request.params[2].isNull()) {
-        if (!ParseOutputType(request.params[2].get_str(), output_type)) {
+        std::optional<OutputType> parsed = ParseOutputType(request.params[2].get_str());
+        if (!parsed) {
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[2].get_str()));
-        }
-        if (output_type == OutputType::BECH32M) {
+        } else if (parsed.value() == OutputType::BECH32M) {
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "createmultisig cannot create bech32m multisig addresses");
         }
+        output_type = parsed.value();
     }
 
     // Construct using pay-to-script-hash:
diff --git a/src/test/fuzz/kitchen_sink.cpp b/src/test/fuzz/kitchen_sink.cpp
@@ -13,6 +13,7 @@
 
 #include <array>
 #include <cstdint>
+#include <optional>
 #include <vector>
 
 namespace {
@@ -46,11 +47,10 @@ FUZZ_TARGET(kitchen_sink)
 
     const OutputType output_type = fuzzed_data_provider.PickValueInArray(OUTPUT_TYPES);
     const std::string& output_type_string = FormatOutputType(output_type);
-    OutputType output_type_parsed;
-    const bool parsed = ParseOutputType(output_type_string, output_type_parsed);
+    const std::optional<OutputType> parsed = ParseOutputType(output_type_string);
     assert(parsed);
-    assert(output_type == output_type_parsed);
-    (void)ParseOutputType(fuzzed_data_provider.ConsumeRandomLengthString(64), output_type_parsed);
+    assert(output_type == parsed.value());
+    (void)ParseOutputType(fuzzed_data_provider.ConsumeRandomLengthString(64));
 
     const std::vector<uint8_t> bytes = ConsumeRandomLengthByteVector(fuzzed_data_provider);
     const std::vector<bool> bits = BytesToBits(bytes);
diff --git a/src/test/fuzz/string.cpp b/src/test/fuzz/string.cpp
@@ -66,8 +66,7 @@ FUZZ_TARGET(string)
         (void)ParseNonRFCJSONValue(random_string_1);
     } catch (const std::runtime_error&) {
     }
-    OutputType output_type;
-    (void)ParseOutputType(random_string_1, output_type);
+    (void)ParseOutputType(random_string_1);
     (void)RemovePrefix(random_string_1, random_string_2);
     (void)ResolveErrMsg(random_string_1, random_string_2);
     try {
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
@@ -266,12 +266,13 @@ static RPCHelpMan getnewaddress()
 
     OutputType output_type = pwallet->m_default_address_type;
     if (!request.params[1].isNull()) {
-        if (!ParseOutputType(request.params[1].get_str(), output_type)) {
+        std::optional<OutputType> parsed = ParseOutputType(request.params[1].get_str());
+        if (!parsed) {
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[1].get_str()));
-        }
-        if (output_type == OutputType::BECH32M && pwallet->GetLegacyScriptPubKeyMan()) {
+        } else if (parsed.value() == OutputType::BECH32M && pwallet->GetLegacyScriptPubKeyMan()) {
             throw JSONRPCError(RPC_INVALID_PARAMETER, "Legacy wallets cannot provide bech32m addresses");
         }
+        output_type = parsed.value();
     }
 
     CTxDestination dest;
@@ -313,12 +314,13 @@ static RPCHelpMan getrawchangeaddress()
 
     OutputType output_type = pwallet->m_default_change_type.value_or(pwallet->m_default_address_type);
     if (!request.params[0].isNull()) {
-        if (!ParseOutputType(request.params[0].get_str(), output_type)) {
+        std::optional<OutputType> parsed = ParseOutputType(request.params[0].get_str());
+        if (!parsed) {
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[0].get_str()));
-        }
-        if (output_type == OutputType::BECH32M && pwallet->GetLegacyScriptPubKeyMan()) {
+        } else if (parsed.value() == OutputType::BECH32M && pwallet->GetLegacyScriptPubKeyMan()) {
             throw JSONRPCError(RPC_INVALID_PARAMETER, "Legacy wallets cannot provide bech32m addresses");
         }
+        output_type = parsed.value();
     }
 
     CTxDestination dest;
@@ -1007,12 +1009,13 @@ static RPCHelpMan addmultisigaddress()
 
     OutputType output_type = pwallet->m_default_address_type;
     if (!request.params[3].isNull()) {
-        if (!ParseOutputType(request.params[3].get_str(), output_type)) {
+        std::optional<OutputType> parsed = ParseOutputType(request.params[3].get_str());
+        if (!parsed) {
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[3].get_str()));
-        }
-        if (output_type == OutputType::BECH32M) {
+        } else if (parsed.value() == OutputType::BECH32M) {
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Bech32m multisig addresses cannot be created with legacy wallets");
         }
+        output_type = parsed.value();
     }
 
     // Construct using pay-to-script-hash:
@@ -3133,11 +3136,11 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out,
             if (options.exists("changeAddress") || options.exists("change_address")) {
                 throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both change address and address type options");
             }
-            OutputType out_type;
-            if (!ParseOutputType(options["change_type"].get_str(), out_type)) {
+            if (std::optional<OutputType> parsed = ParseOutputType(options["change_type"].get_str())) {
+                coinControl.m_change_type.emplace(parsed.value());
+            } else {
                 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown change type '%s'", options["change_type"].get_str()));
             }
-            coinControl.m_change_type.emplace(out_type);
         }
 
         const UniValue include_watching_option = options.exists("include_watching") ? options["include_watching"] : options["includeWatching"];
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -2586,19 +2586,21 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain* chain, const std::st
     }
 
     if (!gArgs.GetArg("-addresstype", "").empty()) {
-        if (!ParseOutputType(gArgs.GetArg("-addresstype", ""), walletInstance->m_default_address_type)) {
+        std::optional<OutputType> parsed = ParseOutputType(gArgs.GetArg("-addresstype", ""));
+        if (!parsed) {
             error = strprintf(_("Unknown address type '%s'"), gArgs.GetArg("-addresstype", ""));
             return nullptr;
         }
+        walletInstance->m_default_address_type = parsed.value();
     }
 
     if (!gArgs.GetArg("-changetype", "").empty()) {
-        OutputType out_type;
-        if (!ParseOutputType(gArgs.GetArg("-changetype", ""), out_type)) {
+        std::optional<OutputType> parsed = ParseOutputType(gArgs.GetArg("-changetype", ""));
+        if (!parsed) {
             error = strprintf(_("Unknown change type '%s'"), gArgs.GetArg("-changetype", ""));
             return nullptr;
         }
-        walletInstance->m_default_change_type = out_type;
+        walletInstance->m_default_change_type = parsed.value();
     }
 
     if (gArgs.IsArgSet("-mintxfee")) {

Original file line number	Diff line number	Diff line change
`@@ -2586,19 +2586,21 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain* chain, const std::st`
`2586`	`2586`	`}`
`2587`	`2587`
`2588`	`2588`	`if (!gArgs.GetArg("-addresstype", "").empty()) {`
`2589`		`- if (!ParseOutputType(gArgs.GetArg("-addresstype", ""), walletInstance->m_default_address_type)) {`
	`2589`	`+ std::optional<OutputType> parsed = ParseOutputType(gArgs.GetArg("-addresstype", ""));`
	`2590`	`+ if (!parsed) {`
`2590`	`2591`	`error = strprintf(_("Unknown address type '%s'"), gArgs.GetArg("-addresstype", ""));`
`2591`	`2592`	`return nullptr;`
`2592`	`2593`	`}`
	`2594`	`+ walletInstance->m_default_address_type = parsed.value();`
`2593`	`2595`	`}`
`2594`	`2596`
`2595`	`2597`	`if (!gArgs.GetArg("-changetype", "").empty()) {`
`2596`		`- OutputType out_type;`
`2597`		`- if (!ParseOutputType(gArgs.GetArg("-changetype", ""), out_type)) {`
	`2598`	`+ std::optional<OutputType> parsed = ParseOutputType(gArgs.GetArg("-changetype", ""));`
	`2599`	`+ if (!parsed) {`
`2598`	`2600`	`error = strprintf(_("Unknown change type '%s'"), gArgs.GetArg("-changetype", ""));`
`2599`	`2601`	`return nullptr;`
`2600`	`2602`	`}`
`2601`		`- walletInstance->m_default_change_type = out_type;`
	`2603`	`+ walletInstance->m_default_change_type = parsed.value();`
`2602`	`2604`	`}`
`2603`	`2605`
`2604`	`2606`	`if (gArgs.IsArgSet("-mintxfee")) {`