Skip to content

Commit 7173a3c

Browse files
laanwjlaanwj
committed
Merge #19396: refactor: Remove confusing OutputType::CHANGE_AUTO
fa927ff Enable Wswitch for OutputType (MarcoFalke) faddad7 Remove confusing OutputType::CHANGE_AUTO (MarcoFalke) fa2eb38 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 fa927ff. laanwj: Code review ACK fa927ff Tree-SHA512: 24fd809757aa343866c94dafe9a7130b50cda4f77c97666d407f99b813f75b115a7d8e688a6bc2a737e87cba64ddd4e43f2b3c5538fd35fabb5845807bb39134
2 parents d6fe5b2 + fa927ff commit 7173a3c

File tree

8 files changed

+35
-37
lines changed

8 files changed

+35
-37
lines changed

‎src/interfaces/wallet.cpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -438,7 +438,6 @@ class WalletImpl : public Wallet
438438
bool canGetAddresses() override { return m_wallet->CanGetAddresses(); }
439439
bool privateKeysDisabled() override { return m_wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS); }
440440
OutputType getDefaultAddressType() override { return m_wallet->m_default_address_type; }
441-
OutputType getDefaultChangeType() override { return m_wallet->m_default_change_type; }
442441
CAmount getDefaultMaxTxFee() override { return m_wallet->m_default_max_tx_fee; }
443442
void remove() override
444443
{

‎src/interfaces/wallet.h

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -256,9 +256,6 @@ class Wallet
256256
// Get default address type.
257257
virtual OutputType getDefaultAddressType() = 0;
258258

259-
// Get default change type.
260-
virtual OutputType getDefaultChangeType() = 0;
261-
262259
//! Get max tx fee.
263260
virtual CAmount getDefaultMaxTxFee() = 0;
264261

‎src/outputtype.cpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,8 @@ const std::string& FormatOutputType(OutputType type)
4242
case OutputType::LEGACY: return OUTPUT_TYPE_STRING_LEGACY;
4343
case OutputType::P2SH_SEGWIT: return OUTPUT_TYPE_STRING_P2SH_SEGWIT;
4444
case OutputType::BECH32: return OUTPUT_TYPE_STRING_BECH32;
45-
default: assert(false);
46-
}
45+
} // no default case, so the compiler can warn about missing cases
46+
assert(false);
4747
}
4848

4949
CTxDestination GetDestinationForKey(const CPubKey& key, OutputType type)
@@ -61,8 +61,8 @@ CTxDestination GetDestinationForKey(const CPubKey& key, OutputType type)
6161
return witdest;
6262
}
6363
}
64-
default: assert(false);
65-
}
64+
} // no default case, so the compiler can warn about missing cases
65+
assert(false);
6666
}
6767

6868
std::vector<CTxDestination> GetAllDestinationsForKey(const CPubKey& key)
@@ -100,6 +100,6 @@ CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore,
100100
return ScriptHash(witprog);
101101
}
102102
}
103-
default: assert(false);
104-
}
103+
} // no default case, so the compiler can warn about missing cases
104+
assert(false);
105105
}

‎src/outputtype.h

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,6 @@ enum class OutputType {
1818
LEGACY,
1919
P2SH_SEGWIT,
2020
BECH32,
21-
22-
/**
23-
* Special output type for change outputs only. Automatically choose type
24-
* based on address type setting and the types other of non-change outputs
25-
* (see -changetype option documentation and implementation in
26-
* CWallet::TransactionChangeType for details).
27-
*/
28-
CHANGE_AUTO,
2921
};
3022

3123
extern const std::array<OutputType, 3> OUTPUT_TYPES;

‎src/wallet/rpcwallet.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -306,7 +306,7 @@ static UniValue getrawchangeaddress(const JSONRPCRequest& request)
306306
throw JSONRPCError(RPC_WALLET_ERROR, "Error: This wallet has no available keys");
307307
}
308308

309-
OutputType output_type = pwallet->m_default_change_type != OutputType::CHANGE_AUTO ? pwallet->m_default_change_type : pwallet->m_default_address_type;
309+
OutputType output_type = pwallet->m_default_change_type.get_value_or(pwallet->m_default_address_type);
310310
if (!request.params[0].isNull()) {
311311
if (!ParseOutputType(request.params[0].get_str(), output_type)) {
312312
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
29932993
if (options.exists("changeAddress")) {
29942994
throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both changeAddress and address_type options");
29952995
}
2996-
coinControl.m_change_type = pwallet->m_default_change_type;
2997-
if (!ParseOutputType(options["change_type"].get_str(), *coinControl.m_change_type)) {
2996+
OutputType out_type;
2997+
if (!ParseOutputType(options["change_type"].get_str(), out_type)) {
29982998
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown change type '%s'", options["change_type"].get_str()));
29992999
}
3000+
coinControl.m_change_type.emplace(out_type);
30003001
}
30013002

30023003
coinControl.fAllowWatchOnly = ParseIncludeWatchonly(options["includeWatching"], *pwallet);

‎src/wallet/scriptpubkeyman.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1900,8 +1900,8 @@ bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_
19001900
desc_prefix = "wpkh(" + xpub + "/84'";
19011901
break;
19021902
}
1903-
default: assert(false);
1904-
}
1903+
} // no default case, so the compiler can warn about missing cases
1904+
assert(!desc_prefix.empty());
19051905

19061906
// Mainnet derives at 0', testnet and regtest derive at 1'
19071907
if (Params().IsTestChain()) {

‎src/wallet/wallet.cpp

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2669,11 +2669,11 @@ static uint32_t GetLocktimeForNewTransaction(interfaces::Chain& chain, const uin
26692669
return locktime;
26702670
}
26712671

2672-
OutputType CWallet::TransactionChangeType(OutputType change_type, const std::vector<CRecipient>& vecSend)
2672+
OutputType CWallet::TransactionChangeType(const Optional<OutputType>& change_type, const std::vector<CRecipient>& vecSend)
26732673
{
26742674
// If -changetype is specified, always use that change type.
2675-
if (change_type != OutputType::CHANGE_AUTO) {
2676-
return change_type;
2675+
if (change_type) {
2676+
return *change_type;
26772677
}
26782678

26792679
// if m_default_address_type is legacy, use legacy address as change (even
@@ -3842,14 +3842,20 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
38423842
}
38433843
}
38443844

3845-
if (!gArgs.GetArg("-addresstype", "").empty() && !ParseOutputType(gArgs.GetArg("-addresstype", ""), walletInstance->m_default_address_type)) {
3846-
error = strprintf(_("Unknown address type '%s'"), gArgs.GetArg("-addresstype", ""));
3847-
return nullptr;
3845+
if (!gArgs.GetArg("-addresstype", "").empty()) {
3846+
if (!ParseOutputType(gArgs.GetArg("-addresstype", ""), walletInstance->m_default_address_type)) {
3847+
error = strprintf(_("Unknown address type '%s'"), gArgs.GetArg("-addresstype", ""));
3848+
return nullptr;
3849+
}
38483850
}
38493851

3850-
if (!gArgs.GetArg("-changetype", "").empty() && !ParseOutputType(gArgs.GetArg("-changetype", ""), walletInstance->m_default_change_type)) {
3851-
error = strprintf(_("Unknown change type '%s'"), gArgs.GetArg("-changetype", ""));
3852-
return nullptr;
3852+
if (!gArgs.GetArg("-changetype", "").empty()) {
3853+
OutputType out_type;
3854+
if (!ParseOutputType(gArgs.GetArg("-changetype", ""), out_type)) {
3855+
error = strprintf(_("Unknown change type '%s'"), gArgs.GetArg("-changetype", ""));
3856+
return nullptr;
3857+
}
3858+
walletInstance->m_default_change_type = out_type;
38533859
}
38543860

38553861
if (gArgs.IsArgSet("-mintxfee")) {

‎src/wallet/wallet.h

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -105,9 +105,6 @@ class ReserveDestination;
105105
//! Default for -addresstype
106106
constexpr OutputType DEFAULT_ADDRESS_TYPE{OutputType::BECH32};
107107

108-
//! Default for -changetype
109-
constexpr OutputType DEFAULT_CHANGE_TYPE{OutputType::CHANGE_AUTO};
110-
111108
static constexpr uint64_t KNOWN_WALLET_FLAGS =
112109
WALLET_FLAG_AVOID_REUSE
113110
| WALLET_FLAG_BLANK_WALLET
@@ -934,7 +931,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
934931
Balance GetBalance(int min_depth = 0, bool avoid_reuse = true) const;
935932
CAmount GetAvailableBalance(const CCoinControl* coinControl = nullptr) const;
936933

937-
OutputType TransactionChangeType(OutputType change_type, const std::vector<CRecipient>& vecSend);
934+
OutputType TransactionChangeType(const Optional<OutputType>& change_type, const std::vector<CRecipient>& vecSend);
938935

939936
/**
940937
* Insert additional inputs into the transaction by
@@ -1012,7 +1009,13 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
10121009
CFeeRate m_fallback_fee{DEFAULT_FALLBACK_FEE};
10131010
CFeeRate m_discard_rate{DEFAULT_DISCARD_FEE};
10141011
OutputType m_default_address_type{DEFAULT_ADDRESS_TYPE};
1015-
OutputType m_default_change_type{DEFAULT_CHANGE_TYPE};
1012+
/**
1013+
* Default output type for change outputs. When unset, automatically choose type
1014+
* based on address type setting and the types other of non-change outputs
1015+
* (see -changetype option documentation and implementation in
1016+
* CWallet::TransactionChangeType for details).
1017+
*/
1018+
Optional<OutputType> m_default_change_type{};
10161019
/** Absolute maximum transaction fee (in satoshis) used by default for the wallet */
10171020
CAmount m_default_max_tx_fee{DEFAULT_TRANSACTION_MAXFEE};
10181021

0 commit comments

Comments
 (0)