diff --git a/doc/release-notes-27279.md b/doc/release-notes-27279.md new file mode 100644 index 0000000000..b664aee755 --- /dev/null +++ b/doc/release-notes-27279.md @@ -0,0 +1,10 @@ +Wallet +------ + +- In the createwallet, loadwallet, unloadwallet, and restorewallet RPCs, the + "warning" string field is deprecated in favor of a "warnings" field that + returns a JSON array of strings to better handle multiple warning messages and + for consistency with other wallet RPCs. The "warning" field will be fully + removed from these RPCs in v26. It can be temporarily re-enabled during the + deprecation period by launching bitcoind with the configuration option + `-deprecatedrpc=walletwarningfield`. (#27279) diff --git a/src/rpc/output_script.cpp b/src/rpc/output_script.cpp index 3d44bd9d66..b94dab24cd 100644 --- a/src/rpc/output_script.cpp +++ b/src/rpc/output_script.cpp @@ -189,12 +189,12 @@ static RPCHelpMan createmultisig() result.pushKV("redeemScript", HexStr(inner)); result.pushKV("descriptor", descriptor->ToString()); - UniValue warnings(UniValue::VARR); - if (descriptor->GetOutputType() != output_type) { - // Only warns if the user has explicitly chosen an address type we cannot generate - warnings.push_back("Unable to make chosen address type, please ensure no uncompressed public keys are present."); - } - if (!warnings.empty()) result.pushKV("warnings", warnings); + UniValue warnings(UniValue::VARR); + if (descriptor->GetOutputType() != output_type) { + // Only warns if the user has explicitly chosen an address type we cannot generate + warnings.push_back("Unable to make chosen address type, please ensure no uncompressed public keys are present."); + } + PushWarnings(warnings, result); return result; }, diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index 810dbd7bbd..42af6cd603 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -1387,3 +1387,26 @@ UniValue AmountMapToUniv(const CAmountMap& balanceOrig, std::string strasset) // END ELEMENTS // + +/** Convert a vector of bilingual strings to a UniValue::VARR containing their original untranslated values. */ +[[nodiscard]] static UniValue BilingualStringsToUniValue(const std::vector& bilingual_strings) +{ + CHECK_NONFATAL(!bilingual_strings.empty()); + UniValue result{UniValue::VARR}; + for (const auto& s : bilingual_strings) { + result.push_back(s.original); + } + return result; +} + +void PushWarnings(const UniValue& warnings, UniValue& obj) +{ + if (warnings.empty()) return; + obj.pushKV("warnings", warnings); +} + +void PushWarnings(const std::vector& warnings, UniValue& obj) +{ + if (warnings.empty()) return; + obj.pushKV("warnings", BilingualStringsToUniValue(warnings)); +} diff --git a/src/rpc/util.h b/src/rpc/util.h index 61273be1fa..c1778ccb7b 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -386,4 +386,13 @@ private: const RPCExamples m_examples; }; +/** + * Push warning messages to an RPC "warnings" field as a JSON array of strings. + * + * @param[in] warnings Warning messages to push. + * @param[out] obj UniValue object to push the warnings array object to. + */ +void PushWarnings(const UniValue& warnings, UniValue& obj); +void PushWarnings(const std::vector& warnings, UniValue& obj); + #endif // BITCOIN_RPC_UTIL_H diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index 2655e3e2b3..53ebd8e368 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -318,7 +318,7 @@ RPCHelpMan addmultisigaddress() // Only warns if the user has explicitly chosen an address type we cannot generate warnings.push_back("Unable to make chosen address type, please ensure no uncompressed public keys are present."); } - if (!warnings.empty()) result.pushKV("warnings", warnings); + PushWarnings(warnings, result); return result; }, diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index cc9f34514a..b4cb85b201 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -1261,7 +1261,7 @@ static UniValue ProcessImport(CWallet& wallet, const UniValue& data, const int64 result.pushKV("error", JSONRPCError(RPC_MISC_ERROR, "Missing required fields")); } - if (warnings.size()) result.pushKV("warnings", warnings); + PushWarnings(warnings, result); return result; } @@ -1616,7 +1616,7 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c result.pushKV("success", UniValue(false)); result.pushKV("error", e); } - if (warnings.size()) result.pushKV("warnings", warnings); + PushWarnings(warnings, result); return result; } @@ -1940,7 +1940,11 @@ RPCHelpMan restorewallet() RPCResult::Type::OBJ, "", "", { {RPCResult::Type::STR, "name", "The wallet name if restored successfully."}, - {RPCResult::Type::STR, "warning", "Warning message if wallet was not loaded cleanly."}, + {RPCResult::Type::STR, "warning", /*optional=*/true, "Warning messages, if any, related to restoring the wallet. Multiple messages will be delimited by newlines. (DEPRECATED, returned only if config option -deprecatedrpc=walletwarningfield is passed.)"}, + {RPCResult::Type::ARR, "warnings", /*optional=*/true, "Warning messages, if any, related to restoring the wallet.", + { + {RPCResult::Type::STR, "", ""}, + }}, } }, RPCExamples{ @@ -1970,7 +1974,10 @@ RPCHelpMan restorewallet() UniValue obj(UniValue::VOBJ); obj.pushKV("name", wallet->GetName()); - obj.pushKV("warning", Join(warnings, Untranslated("\n")).original); + if (wallet->chain().rpcEnableDeprecated("walletwarningfield")) { + obj.pushKV("warning", Join(warnings, Untranslated("\n")).original); + } + PushWarnings(warnings, obj); return obj; diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index c70e1a1b1e..0ab5eed14f 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -12,6 +12,8 @@ #include #include #include +#include +#include #include @@ -19,6 +21,14 @@ namespace wallet { + +static const std::map WALLET_FLAG_CAVEATS{ + {WALLET_FLAG_AVOID_REUSE, + "You need to rescan the blockchain in order to correctly mark used " + "destinations in the past. Until this is done, some destinations may " + "be considered unused, even if the opposite is the case."}, +}; + /** Checks if a CKey is in the given CWallet compressed or otherwise*/ bool HaveKey(const SigningProvider& wallet, const CKey& key) { @@ -215,7 +225,11 @@ static RPCHelpMan loadwallet() RPCResult::Type::OBJ, "", "", { {RPCResult::Type::STR, "name", "The wallet name if loaded successfully."}, - {RPCResult::Type::STR, "warning", "Warning message if wallet was not loaded cleanly."}, + {RPCResult::Type::STR, "warning", /*optional=*/true, "Warning messages, if any, related to loading the wallet. Multiple messages will be delimited by newlines. (DEPRECATED, returned only if config option -deprecatedrpc=walletwarningfield is passed.)"}, + {RPCResult::Type::ARR, "warnings", /*optional=*/true, "Warning messages, if any, related to loading the wallet.", + { + {RPCResult::Type::STR, "", ""}, + }}, } }, RPCExamples{ @@ -248,7 +262,10 @@ static RPCHelpMan loadwallet() UniValue obj(UniValue::VOBJ); obj.pushKV("name", wallet->GetName()); - obj.pushKV("warning", Join(warnings, Untranslated("\n")).original); + if (wallet->chain().rpcEnableDeprecated("walletwarningfield")) { + obj.pushKV("warning", Join(warnings, Untranslated("\n")).original); + } + PushWarnings(warnings, obj); return obj; }, @@ -343,7 +360,11 @@ static RPCHelpMan createwallet() RPCResult::Type::OBJ, "", "", { {RPCResult::Type::STR, "name", "The wallet name if created successfully. If the wallet was created using a full path, the wallet_name will be the full path."}, - {RPCResult::Type::STR, "warning", "Warning message if wallet was not loaded cleanly."}, + {RPCResult::Type::STR, "warning", /*optional=*/true, "Warning messages, if any, related to creating the wallet. Multiple messages will be delimited by newlines. (DEPRECATED, returned only if config option -deprecatedrpc=walletwarningfield is passed.)"}, + {RPCResult::Type::ARR, "warnings", /*optional=*/true, "Warning messages, if any, related to creating the wallet.", + { + {RPCResult::Type::STR, "", ""}, + }}, } }, RPCExamples{ @@ -413,7 +434,10 @@ static RPCHelpMan createwallet() UniValue obj(UniValue::VOBJ); obj.pushKV("name", wallet->GetName()); - obj.pushKV("warning", Join(warnings, Untranslated("\n")).original); + if (wallet->chain().rpcEnableDeprecated("walletwarningfield")) { + obj.pushKV("warning", Join(warnings, Untranslated("\n")).original); + } + PushWarnings(warnings, obj); return obj; }, @@ -430,7 +454,11 @@ static RPCHelpMan unloadwallet() {"load_on_startup", RPCArg::Type::BOOL, RPCArg::Optional::OMITTED, "Save wallet name to persistent settings and load on startup. True to add wallet to startup list, false to remove, null to leave unchanged."}, }, RPCResult{RPCResult::Type::OBJ, "", "", { - {RPCResult::Type::STR, "warning", "Warning message if wallet was not unloaded cleanly."}, + {RPCResult::Type::STR, "warning", /*optional=*/true, "Warning messages, if any, related to unloading the wallet. Multiple messages will be delimited by newlines. (DEPRECATED, returned only if config option -deprecatedrpc=walletwarningfield is passed.)"}, + {RPCResult::Type::ARR, "warnings", /*optional=*/true, "Warning messages, if any, related to unloading the wallet.", + { + {RPCResult::Type::STR, "", ""}, + }}, }}, RPCExamples{ HelpExampleCli("unloadwallet", "wallet_name") @@ -468,11 +496,13 @@ static RPCHelpMan unloadwallet() throw JSONRPCError(RPC_MISC_ERROR, "Requested wallet already unloaded"); } } + UniValue result(UniValue::VOBJ); + if (wallet->chain().rpcEnableDeprecated("walletwarningfield")) { + result.pushKV("warning", Join(warnings, Untranslated("\n")).original); + } + PushWarnings(warnings, result); UnloadWallet(std::move(wallet)); - - UniValue result(UniValue::VOBJ); - result.pushKV("warning", Join(warnings, Untranslated("\n")).original); return result; }, }; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3f677950e1..59b4697292 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -60,15 +60,8 @@ using interfaces::FoundBlock; namespace wallet { -isminetype InputIsMine(const CWallet& wallet, const COutPoint& txin); // ELEMENTS: forward declaration to remove circular dep on receive.h -const std::map WALLET_FLAG_CAVEATS{ - {WALLET_FLAG_AVOID_REUSE, - "You need to rescan the blockchain in order to correctly mark used " - "destinations in the past. Until this is done, some destinations may " - "be considered unused, even if the opposite is the case." - }, -}; +isminetype InputIsMine(const CWallet& wallet, const COutPoint& txin); // ELEMENTS: forward declaration to remove circular dep on receive.h bool AddWalletSetting(interfaces::Chain& chain, const std::string& wallet_name) { util::SettingsValue setting_value = chain.getRwSetting("wallet"); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 75a557138f..37039a4244 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -186,8 +186,6 @@ static const std::map WALLET_FLAG_MAP{ {"external_signer", WALLET_FLAG_EXTERNAL_SIGNER} }; -extern const std::map WALLET_FLAG_CAVEATS; - /** A wrapper to reserve an address from a wallet * * ReserveDestination is used to reserve an address. diff --git a/test/functional/wallet_backwards_compatibility.py b/test/functional/wallet_backwards_compatibility.py index 76aac3e486..a6401a76c1 100755 --- a/test/functional/wallet_backwards_compatibility.py +++ b/test/functional/wallet_backwards_compatibility.py @@ -265,7 +265,12 @@ class BackwardsCompatibilityTest(BitcoinTestFramework): ) load_res = node_master.loadwallet("u1_v16") # Make sure this wallet opens without warnings. See https://github.com/bitcoin/bitcoin/pull/19054 - assert_equal(load_res['warning'], '') + if int(node_master.getnetworkinfo()["version"]) >= 249900: + # loadwallet#warnings (added in v25) -- only present if there is a warning + assert "warnings" not in load_res + else: + # loadwallet#warning (deprecated in v25) -- always present, but empty string if no warning + assert_equal(load_res["warning"], '') wallet = node_master.get_wallet_rpc("u1_v16") info = wallet.getaddressinfo(v16_addr) descriptor = f"wpkh([{info['hdmasterfingerprint']}{hdkeypath[1:]}]{v16_pubkey})" diff --git a/test/functional/wallet_createwallet.py b/test/functional/wallet_createwallet.py index 22c491441b..58cc6befbd 100755 --- a/test/functional/wallet_createwallet.py +++ b/test/functional/wallet_createwallet.py @@ -15,12 +15,17 @@ from test_framework.util import ( ) from test_framework.wallet_util import bytes_to_wif, generate_wif_key +EMPTY_PASSPHRASE_MSG = "Empty string given as passphrase, wallet will not be encrypted." +LEGACY_WALLET_MSG = "Wallet created successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future." + + class CreateWalletTest(BitcoinTestFramework): def add_options(self, parser): self.add_wallet_options(parser) def set_test_params(self): self.num_nodes = 1 + self.extra_args = [["-deprecatedrpc=walletwarningfield"]] def skip_test_if_missing_module(self): self.skip_if_no_wallet() @@ -55,7 +60,7 @@ class CreateWalletTest(BitcoinTestFramework): else: result = w1.importmulti([{'scriptPubKey': {'address': key_to_p2wpkh(eckey.get_pubkey().get_bytes())}, 'timestamp': 'now', 'keys': [privkey]}]) assert not result[0]['success'] - assert 'warning' not in result[0] + assert 'warnings' not in result[0] assert_equal(result[0]['error']['code'], -4) assert_equal(result[0]['error']['message'], 'Cannot import private keys to a wallet with private keys disabled') @@ -159,7 +164,9 @@ class CreateWalletTest(BitcoinTestFramework): assert_equal(walletinfo['keypoolsize_hd_internal'], keys) # Allow empty passphrase, but there should be a warning resp = self.nodes[0].createwallet(wallet_name='w7', disable_private_keys=False, blank=False, passphrase='') - assert 'Empty string given as passphrase, wallet will not be encrypted.' in resp['warning'] + assert_equal(resp["warning"], EMPTY_PASSPHRASE_MSG if self.options.descriptors else f"{EMPTY_PASSPHRASE_MSG}\n{LEGACY_WALLET_MSG}") + assert_equal(resp["warnings"], [EMPTY_PASSPHRASE_MSG] if self.options.descriptors else [EMPTY_PASSPHRASE_MSG, LEGACY_WALLET_MSG]) + w7 = node.get_wallet_rpc('w7') assert_raises_rpc_error(-15, 'Error: running with an unencrypted wallet, but walletpassphrase was called.', w7.walletpassphrase, '', 60) @@ -174,8 +181,24 @@ class CreateWalletTest(BitcoinTestFramework): if self.is_bdb_compiled(): self.log.info("Test legacy wallet deprecation") - res = self.nodes[0].createwallet(wallet_name="legacy_w0", descriptors=False, passphrase=None) - assert_equal(res["warning"], "Wallet created successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future.") + result = self.nodes[0].createwallet(wallet_name="legacy_w0", descriptors=False, passphrase=None) + assert_equal(result, { + "name": "legacy_w0", + "warning": LEGACY_WALLET_MSG, + "warnings": [LEGACY_WALLET_MSG], + }) + result = self.nodes[0].createwallet(wallet_name="legacy_w1", descriptors=False, passphrase="") + assert_equal(result, { + "name": "legacy_w1", + "warning": f"{EMPTY_PASSPHRASE_MSG}\n{LEGACY_WALLET_MSG}", + "warnings": [EMPTY_PASSPHRASE_MSG, LEGACY_WALLET_MSG], + }) + + self.log.info('Test "warning" field deprecation, i.e. not returned without -deprecatedrpc=walletwarningfield') + self.restart_node(0, extra_args=[]) + result = self.nodes[0].createwallet(wallet_name="w7_again", disable_private_keys=False, blank=False, passphrase="") + assert "warning" not in result + if __name__ == '__main__': CreateWalletTest().main()