Skip to content

Commit 3be62ae

Browse files
ryanofskyryanofsky
committed
Get rid of ambiguous OutputType::NONE value
Based on suggestion by Pieter Wuille <[email protected]> at #12119 (comment) After #12119, the NONE output type was overloaded to refer to either an output type that couldn't be parsed, or to an automatic change output mode. This change drops the NONE enum and uses a simple bool indicate parse failure, and a new CHANGE_AUTO enum to refer the change output type. This change is almost a pure refactoring except it makes RPCs reject empty string ("") address types instead of treating them like they were unset. This simplifies the parsing code a little bit and could prevent RPC usage mistakes. It's noted in the release notes.
1 parent ee7b67e commit 3be62ae

File tree

5 files changed

+36
-29
lines changed

5 files changed

+36
-29
lines changed

‎doc/release-notes.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,11 @@ Low-level RPC changes
9393
now the empty string `""` instead of `"wallet.dat"`. If bitcoin is started
9494
with any `-wallet=<path>` options, there is no change in behavior, and the
9595
name of any wallet is just its `<path>` string.
96+
- Passing an empty string (`""`) as the `address_type` parameter to
97+
`getnewaddress`, `getrawchangeaddress`, `addmultisigaddress`,
98+
`addmultisigaddress`, `fundrawtransaction` RPCs is now an error. Previously,
99+
this would fall back to using the default address type. It is still possible
100+
to pass null or leave the parameter unset to use the default address type.
96101

97102
### Logging
98103

‎src/qt/paymentserver.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -643,7 +643,7 @@ void PaymentServer::fetchPaymentACK(CWallet* wallet, const SendCoinsRecipient& r
643643
// use for change. Despite an actual payment and not change, this is a close match:
644644
// it's the output type we use subject to privacy issues, but not restricted by what
645645
// other software supports.
646-
const OutputType change_type = wallet->m_default_change_type != OutputType::NONE ? wallet->m_default_change_type : wallet->m_default_address_type;
646+
const OutputType change_type = wallet->m_default_change_type != OutputType::CHANGE_AUTO ? wallet->m_default_change_type : wallet->m_default_address_type;
647647
wallet->LearnRelatedScripts(newKey, change_type);
648648
CTxDestination dest = GetDestinationForKey(newKey, change_type);
649649
wallet->SetAddressBook(dest, strAccount, "refund");

‎src/wallet/rpcwallet.cpp‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -164,8 +164,7 @@ UniValue getnewaddress(const JSONRPCRequest& request)
164164

165165
OutputType output_type = pwallet->m_default_address_type;
166166
if (!request.params[1].isNull()) {
167-
output_type = ParseOutputType(request.params[1].get_str(), pwallet->m_default_address_type);
168-
if (output_type == OutputType::NONE) {
167+
if (!ParseOutputType(request.params[1].get_str(), output_type)) {
169168
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[1].get_str()));
170169
}
171170
}
@@ -259,10 +258,9 @@ UniValue getrawchangeaddress(const JSONRPCRequest& request)
259258
pwallet->TopUpKeyPool();
260259
}
261260

262-
OutputType output_type = pwallet->m_default_change_type != OutputType::NONE ? pwallet->m_default_change_type : pwallet->m_default_address_type;
261+
OutputType output_type = pwallet->m_default_change_type != OutputType::CHANGE_AUTO ? pwallet->m_default_change_type : pwallet->m_default_address_type;
263262
if (!request.params[0].isNull()) {
264-
output_type = ParseOutputType(request.params[0].get_str(), output_type);
265-
if (output_type == OutputType::NONE) {
263+
if (!ParseOutputType(request.params[0].get_str(), output_type)) {
266264
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[0].get_str()));
267265
}
268266
}
@@ -1223,8 +1221,7 @@ UniValue addmultisigaddress(const JSONRPCRequest& request)
12231221

12241222
OutputType output_type = pwallet->m_default_address_type;
12251223
if (!request.params[3].isNull()) {
1226-
output_type = ParseOutputType(request.params[3].get_str(), output_type);
1227-
if (output_type == OutputType::NONE) {
1224+
if (!ParseOutputType(request.params[3].get_str(), output_type)) {
12281225
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[3].get_str()));
12291226
}
12301227
}
@@ -3183,8 +3180,8 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
31833180
if (options.exists("changeAddress")) {
31843181
throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both changeAddress and address_type options");
31853182
}
3186-
coinControl.m_change_type = ParseOutputType(options["change_type"].get_str(), pwallet->m_default_change_type);
3187-
if (coinControl.m_change_type == OutputType::NONE) {
3183+
coinControl.m_change_type = pwallet->m_default_change_type;
3184+
if (!ParseOutputType(options["change_type"].get_str(), *coinControl.m_change_type)) {
31883185
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown change type '%s'", options["change_type"].get_str()));
31893186
}
31903187
}

‎src/wallet/wallet.cpp‎

Lines changed: 12 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2647,7 +2647,7 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
26472647
OutputType CWallet::TransactionChangeType(OutputType change_type, const std::vector<CRecipient>& vecSend)
26482648
{
26492649
// If -changetype is specified, always use that change type.
2650-
if (change_type != OutputType::NONE) {
2650+
if (change_type != OutputType::CHANGE_AUTO) {
26512651
return change_type;
26522652
}
26532653

@@ -4012,16 +4012,12 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
40124012
}
40134013
}
40144014

4015-
walletInstance->m_default_address_type = ParseOutputType(gArgs.GetArg("-addresstype", ""), DEFAULT_ADDRESS_TYPE);
4016-
if (walletInstance->m_default_address_type == OutputType::NONE) {
4015+
if (!gArgs.GetArg("-addresstype", "").empty() && !ParseOutputType(gArgs.GetArg("-addresstype", ""), walletInstance->m_default_address_type)) {
40174016
InitError(strprintf("Unknown address type '%s'", gArgs.GetArg("-addresstype", "")));
40184017
return nullptr;
40194018
}
40204019

4021-
// If changetype is set in config file or parameter, check that it's valid.
4022-
// Default to OutputType::NONE if not set.
4023-
walletInstance->m_default_change_type = ParseOutputType(gArgs.GetArg("-changetype", ""), OutputType::NONE);
4024-
if (walletInstance->m_default_change_type == OutputType::NONE && !gArgs.GetArg("-changetype", "").empty()) {
4020+
if (!gArgs.GetArg("-changetype", "").empty() && !ParseOutputType(gArgs.GetArg("-changetype", ""), walletInstance->m_default_change_type)) {
40254021
InitError(strprintf("Unknown change type '%s'", gArgs.GetArg("-changetype", "")));
40264022
return nullptr;
40274023
}
@@ -4209,19 +4205,19 @@ static const std::string OUTPUT_TYPE_STRING_LEGACY = "legacy";
42094205
static const std::string OUTPUT_TYPE_STRING_P2SH_SEGWIT = "p2sh-segwit";
42104206
static const std::string OUTPUT_TYPE_STRING_BECH32 = "bech32";
42114207

4212-
OutputType ParseOutputType(const std::string& type, OutputType default_type)
4208+
bool ParseOutputType(const std::string& type, OutputType& output_type)
42134209
{
4214-
if (type.empty()) {
4215-
return default_type;
4216-
} else if (type == OUTPUT_TYPE_STRING_LEGACY) {
4217-
return OutputType::LEGACY;
4210+
if (type == OUTPUT_TYPE_STRING_LEGACY) {
4211+
output_type = OutputType::LEGACY;
4212+
return true;
42184213
} else if (type == OUTPUT_TYPE_STRING_P2SH_SEGWIT) {
4219-
return OutputType::P2SH_SEGWIT;
4214+
output_type = OutputType::P2SH_SEGWIT;
4215+
return true;
42204216
} else if (type == OUTPUT_TYPE_STRING_BECH32) {
4221-
return OutputType::BECH32;
4222-
} else {
4223-
return OutputType::NONE;
4217+
output_type = OutputType::BECH32;
4218+
return true;
42244219
}
4220+
return false;
42254221
}
42264222

42274223
const std::string& FormatOutputType(OutputType type)

‎src/wallet/wallet.h‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -98,15 +98,24 @@ enum WalletFeature
9898
};
9999

100100
enum class OutputType {
101-
NONE,
102101
LEGACY,
103102
P2SH_SEGWIT,
104103
BECH32,
104+
105+
/**
106+
* Special output type for change outputs only. Automatically choose type
107+
* based on address type setting and the types other of non-change outputs
108+
* (see -changetype option documentation and implementation in
109+
* CWallet::TransactionChangeType for details).
110+
*/
111+
CHANGE_AUTO,
105112
};
106113

107114
//! Default for -addresstype
108115
constexpr OutputType DEFAULT_ADDRESS_TYPE{OutputType::P2SH_SEGWIT};
109116

117+
//! Default for -changetype
118+
constexpr OutputType DEFAULT_CHANGE_TYPE{OutputType::CHANGE_AUTO};
110119

111120
/** A key pool entry */
112121
class CKeyPool
@@ -988,7 +997,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
988997
static CFeeRate fallbackFee;
989998
static CFeeRate m_discard_rate;
990999
OutputType m_default_address_type{DEFAULT_ADDRESS_TYPE};
991-
OutputType m_default_change_type{OutputType::NONE}; // Default to OutputType::NONE if not set by -changetype
1000+
OutputType m_default_change_type{DEFAULT_CHANGE_TYPE};
9921001

9931002
bool NewKeyPool();
9941003
size_t KeypoolCountExternalKeys();
@@ -1232,7 +1241,7 @@ class CAccount
12321241
}
12331242
};
12341243

1235-
OutputType ParseOutputType(const std::string& str, OutputType default_type);
1244+
bool ParseOutputType(const std::string& str, OutputType& output_type);
12361245
const std::string& FormatOutputType(OutputType type);
12371246

12381247
/**

0 commit comments

Comments
 (0)