Merge bitcoin/bitcoin#27279: Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet

achow101 · achow101 · commit 6a167325f0d7 · 2023-04-12T13:09:23.000-04:00
7ccdd74 test: fix importmulti/importdescriptors assertion (Jon Atack) 19d888c rpc: move WALLET_FLAG_CAVEATS to the compilation unit of its caller (Jon Atack) 01df011 doc: release note for wallet RPCs "warning" field deprecation (Jon Atack) 9ea8b37 test: createwallet "warning" field deprecation test (Jon Atack) 645d7f7 rpc: deprecate "warning" field in {create,load,unload,restore}wallet (Jon Atack) 2f4a926 test: add test coverage for "warnings" field in createwallet (Jon Atack) 4a1e479 rpc: add "warnings" field to RPCs {create,load,unload,restore}wallet (Jon Atack) 079d8cd rpc: extract wallet "warnings" fields to a util helper (Jon Atack) f73782a doc: fix/improve warning helps in {create,load,unload,restore}wallet (Jon Atack) Pull request description: Based on discussion and concept ACKed in #27138, add a `warnings` field to RPCs createwallet, loadwallet, unloadwallet, and restorewallet as a JSON array of strings to replace the `warning` string field in these 4 RPCs. The idea is to more gracefully handle multiple warning messages and for consistency with other wallet RPCs. Then, deprecate the latter fields, which represent all the remaining RPC `warning` fields. The first commit bitcoin/bitcoin@f73782a implements bitcoin/bitcoin#27138 (comment) as an alternative to #27138. One of those two could potentially be backported to our currently supported releases. ACKs for top commit: achow101: ACK 7ccdd74 1440000bytes: utACK bitcoin/bitcoin@7ccdd74 vasild: ACK 7ccdd74 pinheadmz: re-ACK 7ccdd74 Tree-SHA512: 314e0a4c41fa383d95e2817bfacf359d449e460529d235c3eb902851e2f4eacbabe646d9a5a4beabc4964cdfabf6397ed8301366a58d344a2f787f83b75e9d64
diff --git a/doc/release-notes-27279.md 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
@@ -162,7 +162,7 @@ static RPCHelpMan createmultisig()
                 // 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/rpc/util.cpp b/src/rpc/util.cpp
@@ -1174,3 +1174,26 @@ UniValue GetServicesNames(ServiceFlags services)
 
     return servicesNames;
 }
+
+/** Convert a vector of bilingual strings to a UniValue::VARR containing their original untranslated values. */
+[[nodiscard]] static UniValue BilingualStringsToUniValue(const std::vector<bilingual_str>& 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<bilingual_str>& warnings, UniValue& obj)
+{
+    if (warnings.empty()) return;
+    obj.pushKV("warnings", BilingualStringsToUniValue(warnings));
+}
diff --git a/src/rpc/util.h b/src/rpc/util.h
@@ -381,4 +381,13 @@ class RPCHelpMan
     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<bilingual_str>& warnings, UniValue& obj);
+
 #endif // BITCOIN_RPC_UTIL_H
diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp
@@ -300,7 +300,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
@@ -1225,7 +1225,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;
 }
 
@@ -1579,7 +1579,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;
 }
 
@@ -1903,7 +1903,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{
@@ -1933,7 +1937,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
@@ -13,13 +13,22 @@
 #include <wallet/rpc/wallet.h>
 #include <wallet/rpc/util.h>
 #include <wallet/wallet.h>
+#include <wallet/walletutil.h>
 
 #include <optional>
 
 #include <univalue.h>
 
 
 namespace wallet {
+
+static const std::map<uint64_t, std::string> 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)
 {
@@ -207,7 +216,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{
@@ -240,7 +253,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;
 },
@@ -335,7 +351,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{
@@ -405,7 +425,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;
 },
@@ -422,7 +445,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")
@@ -460,11 +487,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
@@ -52,13 +52,6 @@
 using interfaces::FoundBlock;
 
 namespace wallet {
-const std::map<uint64_t,std::string> 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."
-    },
-};
 
 bool AddWalletSetting(interfaces::Chain& chain, const std::string& wallet_name)
 {
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
@@ -146,8 +146,6 @@ static const std::map<std::string,WalletFlags> WALLET_FLAG_MAP{
     {"external_signer", WALLET_FLAG_EXTERNAL_SIGNER}
 };
 
-extern const std::map<uint64_t,std::string> 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
@@ -265,7 +265,12 @@ def run_test(self):
             )
             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
@@ -15,12 +15,17 @@
 )
 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 @@ def run_test(self):
         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 @@ def run_test(self):
         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 @@ def run_test(self):
 
         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()

Original file line number	Diff line number	Diff line change
`@@ -162,7 +162,7 @@ static RPCHelpMan createmultisig()`
`162`	`162`	`// Only warns if the user has explicitly chosen an address type we cannot generate`
`163`	`163`	`warnings.push_back("Unable to make chosen address type, please ensure no uncompressed public keys are present.");`
`164`	`164`	`}`
`165`		`- if (!warnings.empty()) result.pushKV("warnings", warnings);`
	`165`	`+ PushWarnings(warnings, result);`
`166`	`166`
`167`	`167`	`return result;`
`168`	`168`	`},`
Original file line number	Diff line number	Diff line change
`@@ -300,7 +300,7 @@ RPCHelpMan addmultisigaddress()`
`300`	`300`	`// Only warns if the user has explicitly chosen an address type we cannot generate`
`301`	`301`	`warnings.push_back("Unable to make chosen address type, please ensure no uncompressed public keys are present.");`
`302`	`302`	`}`
`303`		`- if (!warnings.empty()) result.pushKV("warnings", warnings);`
	`303`	`+ PushWarnings(warnings, result);`
`304`	`304`
`305`	`305`	`return result;`
`306`	`306`	`},`
Original file line number	Diff line number	Diff line change
`@@ -1225,7 +1225,7 @@ static UniValue ProcessImport(CWallet& wallet, const UniValue& data, const int64`
`1225`	`1225`
`1226`	`1226`	`result.pushKV("error", JSONRPCError(RPC_MISC_ERROR, "Missing required fields"));`
`1227`	`1227`	`}`
`1228`		`- if (warnings.size()) result.pushKV("warnings", warnings);`
	`1228`	`+ PushWarnings(warnings, result);`
`1229`	`1229`	`return result;`
`1230`	`1230`	`}`
`1231`	`1231`
`@@ -1579,7 +1579,7 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c`
`1579`	`1579`	`result.pushKV("success", UniValue(false));`
`1580`	`1580`	`result.pushKV("error", e);`
`1581`	`1581`	`}`
`1582`		`- if (warnings.size()) result.pushKV("warnings", warnings);`
	`1582`	`+ PushWarnings(warnings, result);`
`1583`	`1583`	`return result;`
`1584`	`1584`	`}`
`1585`	`1585`
`@@ -1903,7 +1903,11 @@ RPCHelpMan restorewallet()`
`1903`	`1903`	`RPCResult::Type::OBJ, "", "",`
`1904`	`1904`	`{`
`1905`	`1905`	`{RPCResult::Type::STR, "name", "The wallet name if restored successfully."},`
`1906`		`- {RPCResult::Type::STR, "warning", "Warning message if wallet was not loaded cleanly."},`
	`1906`	`+ {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.)"},`
	`1907`	`+ {RPCResult::Type::ARR, "warnings", /optional=/true, "Warning messages, if any, related to restoring the wallet.",`
	`1908`	`+ {`
	`1909`	`+ {RPCResult::Type::STR, "", ""},`
	`1910`	`+ }},`
`1907`	`1911`	`}`
`1908`	`1912`	`},`
`1909`	`1913`	`RPCExamples{`
`@@ -1933,7 +1937,10 @@ RPCHelpMan restorewallet()`
`1933`	`1937`
`1934`	`1938`	`UniValue obj(UniValue::VOBJ);`
`1935`	`1939`	`obj.pushKV("name", wallet->GetName());`
`1936`		`- obj.pushKV("warning", Join(warnings, Untranslated("\n")).original);`
	`1940`	`+ if (wallet->chain().rpcEnableDeprecated("walletwarningfield")) {`
	`1941`	`+ obj.pushKV("warning", Join(warnings, Untranslated("\n")).original);`
	`1942`	`+ }`
	`1943`	`+ PushWarnings(warnings, obj);`
`1937`	`1944`
`1938`	`1945`	`return obj;`
`1939`	`1946`
Original file line number	Diff line number	Diff line change
`@@ -146,8 +146,6 @@ static const std::map<std::string,WalletFlags> WALLET_FLAG_MAP{`
`146`	`146`	`{"external_signer", WALLET_FLAG_EXTERNAL_SIGNER}`
`147`	`147`	`};`
`148`	`148`
`149`		`-extern const std::map<uint64_t,std::string> WALLET_FLAG_CAVEATS;`
`150`		`-`
`151`	`149`	`/** A wrapper to reserve an address from a wallet`
`152`	`150`	`*`
`153`	`151`	`* ReserveDestination is used to reserve an address.`