Skip to content

Commit 1e46d8a

Browse files
ryanofskyryanofsky
committed
Get rid of ambiguous OutputType::NONE value
Based on suggestion by Pieter Wuille <pieter.wuille@gmail.com> at bitcoin#12119 (comment) After bitcoin#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 5f0c6a7 commit 1e46d8a

File tree

7 files changed

+41
-31
lines changed

7 files changed

+41
-31
lines changed

‎doc/release-notes.md‎

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

108113
### Logging
109114

‎src/qt/paymentserver.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -648,7 +648,7 @@ void PaymentServer::fetchPaymentACK(WalletModel* walletModel, const SendCoinsRec
648648
// use for change. Despite an actual payment and not change, this is a close match:
649649
// it's the output type we use subject to privacy issues, but not restricted by what
650650
// other software supports.
651-
const OutputType change_type = walletModel->wallet().getDefaultChangeType() != OutputType::NONE ? walletModel->wallet().getDefaultChangeType() : walletModel->wallet().getDefaultAddressType();
651+
const OutputType change_type = walletModel->wallet().getDefaultChangeType() != OutputType::CHANGE_AUTO ? walletModel->wallet().getDefaultChangeType() : walletModel->wallet().getDefaultAddressType();
652652
walletModel->wallet().learnRelatedScripts(newKey, change_type);
653653
CTxDestination dest = GetDestinationForKey(newKey, change_type);
654654
std::string label = tr("Refund from %1").arg(recipient.authenticatedMerchant).toStdString();

‎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
}
@@ -1226,8 +1224,7 @@ UniValue addmultisigaddress(const JSONRPCRequest& request)
12261224

12271225
OutputType output_type = pwallet->m_default_address_type;
12281226
if (!request.params[3].isNull()) {
1229-
output_type = ParseOutputType(request.params[3].get_str(), output_type);
1230-
if (output_type == OutputType::NONE) {
1227+
if (!ParseOutputType(request.params[3].get_str(), output_type)) {
12311228
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[3].get_str()));
12321229
}
12331230
}
@@ -3180,8 +3177,8 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
31803177
if (options.exists("changeAddress")) {
31813178
throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both changeAddress and address_type options");
31823179
}
3183-
coinControl.m_change_type = ParseOutputType(options["change_type"].get_str(), pwallet->m_default_change_type);
3184-
if (coinControl.m_change_type == OutputType::NONE) {
3180+
coinControl.m_change_type = pwallet->m_default_change_type;
3181+
if (!ParseOutputType(options["change_type"].get_str(), *coinControl.m_change_type)) {
31853182
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown change type '%s'", options["change_type"].get_str()));
31863183
}
31873184
}

‎src/wallet/wallet.cpp‎

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

@@ -4022,16 +4022,12 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
40224022
}
40234023
}
40244024

4025-
walletInstance->m_default_address_type = ParseOutputType(gArgs.GetArg("-addresstype", ""), DEFAULT_ADDRESS_TYPE);
4026-
if (walletInstance->m_default_address_type == OutputType::NONE) {
4025+
if (!gArgs.GetArg("-addresstype", "").empty() && !ParseOutputType(gArgs.GetArg("-addresstype", ""), walletInstance->m_default_address_type)) {
40274026
InitError(strprintf("Unknown address type '%s'", gArgs.GetArg("-addresstype", "")));
40284027
return nullptr;
40294028
}
40304029

4031-
// If changetype is set in config file or parameter, check that it's valid.
4032-
// Default to OutputType::NONE if not set.
4033-
walletInstance->m_default_change_type = ParseOutputType(gArgs.GetArg("-changetype", ""), OutputType::NONE);
4034-
if (walletInstance->m_default_change_type == OutputType::NONE && !gArgs.GetArg("-changetype", "").empty()) {
4030+
if (!gArgs.GetArg("-changetype", "").empty() && !ParseOutputType(gArgs.GetArg("-changetype", ""), walletInstance->m_default_change_type)) {
40354031
InitError(strprintf("Unknown change type '%s'", gArgs.GetArg("-changetype", "")));
40364032
return nullptr;
40374033
}
@@ -4219,19 +4215,19 @@ static const std::string OUTPUT_TYPE_STRING_LEGACY = "legacy";
42194215
static const std::string OUTPUT_TYPE_STRING_P2SH_SEGWIT = "p2sh-segwit";
42204216
static const std::string OUTPUT_TYPE_STRING_BECH32 = "bech32";
42214217

4222-
OutputType ParseOutputType(const std::string& type, OutputType default_type)
4218+
bool ParseOutputType(const std::string& type, OutputType& output_type)
42234219
{
4224-
if (type.empty()) {
4225-
return default_type;
4226-
} else if (type == OUTPUT_TYPE_STRING_LEGACY) {
4227-
return OutputType::LEGACY;
4220+
if (type == OUTPUT_TYPE_STRING_LEGACY) {
4221+
output_type = OutputType::LEGACY;
4222+
return true;
42284223
} else if (type == OUTPUT_TYPE_STRING_P2SH_SEGWIT) {
4229-
return OutputType::P2SH_SEGWIT;
4224+
output_type = OutputType::P2SH_SEGWIT;
4225+
return true;
42304226
} else if (type == OUTPUT_TYPE_STRING_BECH32) {
4231-
return OutputType::BECH32;
4232-
} else {
4233-
return OutputType::NONE;
4227+
output_type = OutputType::BECH32;
4228+
return true;
42344229
}
4230+
return false;
42354231
}
42364232

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

‎src/wallet/wallet.h‎

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

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

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

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

112121
/** A key pool entry */
113122
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
/**

‎test/functional/rpc_fundrawtransaction.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ def run_test(self):
224224
outputs = { self.nodes[0].getnewaddress() : Decimal(4.0) }
225225
rawtx = self.nodes[2].createrawtransaction(inputs, outputs)
226226
assert_raises_rpc_error(-1, "JSON value is not a string as expected", self.nodes[2].fundrawtransaction, rawtx, {'change_type': None})
227-
assert_raises_rpc_error(-5, "Unknown change type", self.nodes[2].fundrawtransaction, rawtx, {'change_type': ''})
227+
assert_raises_rpc_error(-5, "Unknown change type ''", self.nodes[2].fundrawtransaction, rawtx, {'change_type': ''})
228228
rawtx = self.nodes[2].fundrawtransaction(rawtx, {'change_type': 'bech32'})
229229
dec_tx = self.nodes[2].decoderawtransaction(rawtx['hex'])
230230
assert_equal('witness_v0_keyhash', dec_tx['vout'][rawtx['changepos']]['scriptPubKey']['type'])

‎test/functional/wallet_address_types.py‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,10 @@ def run_test(self):
280280
self.log.info('getrawchangeaddress defaults to addresstype if -changetype is not set and argument is absent')
281281
self.test_address(3, self.nodes[3].getrawchangeaddress(), multisig=False, typ='bech32')
282282

283-
self.log.info('getrawchangeaddress fails with invalid changetype argument')
283+
self.log.info('test invalid address type arguments')
284+
assert_raises_rpc_error(-5, "Unknown address type ''", self.nodes[3].addmultisigaddress, 2, [compressed_1, compressed_2], None, '')
285+
assert_raises_rpc_error(-5, "Unknown address type ''", self.nodes[3].getnewaddress, None, '')
286+
assert_raises_rpc_error(-5, "Unknown address type ''", self.nodes[3].getrawchangeaddress, '')
284287
assert_raises_rpc_error(-5, "Unknown address type 'bech23'", self.nodes[3].getrawchangeaddress, 'bech23')
285288

286289
self.log.info("Nodes with changetype=p2sh-segwit never use a P2WPKH change output")

0 commit comments

Comments
 (0)