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

jonatack commented at 2:52 AM on March 20, 2023: contributor

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 https://github.com/bitcoin/bitcoin/pull/27279/commits/f73782a9032a462a71569e9424db9bf9eeababf3 implements #27138 (comment) as an alternative to #27138. One of those two could potentially be backported to our currently supported releases.

DrahtBot commented at 2:52 AM on March 20, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	1440000bytes, vasild, pinheadmz, achow101
Approach ACK	TheCharlatan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/650 (Add Import to Wallet GUI by KolbyML)
#27322 (move-only: IsDeprecatedRPCEnabled to rpc/util to fix CI builds when called from wallet by jonatack)
#27138 (rpc, test: remove newline escape sequence from wallet warning fields by jonatack)
#26840 (refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc by KolbyML)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jonatack renamed this:
~~Add "warnings"/deprecate warning {create,load,unload,restore}wallet, deprecate "warning"~~
Add "warnings"/deprecate "warning" in {create,load,unload,restore}wallet, deprecate
on Mar 20, 2023

jonatack renamed this:
~~Add "warnings"/deprecate "warning" in {create,load,unload,restore}wallet, deprecate~~
Add "warnings"/deprecate "warning" in {create,load,unload,restore}wallet
on Mar 20, 2023

jonatack renamed this:
~~Add "warnings"/deprecate "warning" in {create,load,unload,restore}wallet~~
Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet
on Mar 20, 2023

jonatack force-pushed on Mar 20, 2023

in src/wallet/rpc/backup.cpp:1946 in 6a5ceb3fa3 outdated

1943 | +        // The "warning" field is deprecated in v25 for removal in v26.
1944 | +        obj.pushKV("warning", Join(warnings, Untranslated("\n")).original);
1945 | +    }
1946 | +    if (!warnings.empty()) {
1947 | +        obj.pushKV("warnings", BilingualStringsToUniValue(warnings));
1948 | +    }

vasild commented at 4:47 AM on March 22, 2023:

This is repeated in a few places and can be deduplicated (untested):

void PushWarnings(const std::vector<bilingual_str>& warnings, UniValue& to)
{
    if (IsDeprecatedRPCEnabled("warningfield")) {
        // The "warning" field is deprecated in v25 for removal in v26.
        to.pushKV("warning", Join(warnings, Untranslated("\n")).original);
    }
    if (warnings.empty()) {
        return;
    }
    to.pushKV("warnings", UniValue{UniValue::VARR});
    for (const auto& s : warnings) {
        to["warnings"].push_back(s.original);
    }
}

jonatack commented at 9:13 PM on March 23, 2023:

Thanks @vasild. It could maybe even be split further into two methods, one for the warning deprecations in 4 wallet RPCs, and one called by all the wallet RPCs having a warnings field. Will explore.

jonatack commented at 12:12 AM on March 30, 2023:

Done, thanks.

vasild commented at 4:47 AM on March 22, 2023: contributor

Concept ACK

jonatack force-pushed on Mar 23, 2023

in doc/release-notes-27279.md:9 in 748e755870 outdated

   0 | @@ -0,0 +1,10 @@
   1 | +Wallet
   2 | +------
   3 | +
   4 | +- In the createwallet, loadwallet, unloadwallet, and restorewallet RPCs, the
   5 | +  "warning" string field has been deprecated in favor of a "warnings" field
   6 | +  returning a JSON array of strings. This change is in order to better handle
   7 | +  returning multiple warning messages and for consistency with other wallet
   8 | +  RPCs. The deprecated "warning" field will be fully removed from these RPCs in
   9 | +  v25. It can be re-enabled during the deprecation period by launching bitcoind

jonatack commented at 9:22 PM on March 23, 2023:

s/v25/v26/

in test/functional/wallet_createwallet.py:182 in 748e755870 outdated

 178 | @@ -174,8 +179,18 @@ def run_test(self):
 179 |  
 180 |          if self.is_bdb_compiled():
 181 |              self.log.info("Test legacy wallet deprecation")
 182 | -            res = self.nodes[0].createwallet(wallet_name="legacy_w0", descriptors=False, passphrase=None)
 183 | -            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.")
 184 | +            # for name, passphrase in {("legacy_w0", None), ("legacy_w1", "")}:

jonatack commented at 9:23 PM on March 23, 2023:

Remove

jonatack force-pushed on Mar 23, 2023

jonatack force-pushed on Mar 29, 2023

jonatack force-pushed on Mar 30, 2023

jonatack marked this as ready for review on Mar 30, 2023

jonatack force-pushed on Mar 30, 2023

pinheadmz commented at 3:39 PM on March 31, 2023: member

concept ACK

Light code review. I prefer this to #27138 it seems like the right move. Built and ran tests, played with interface, nice!

--> bccli -regtest -named createwallet wallet_name=oops2 descriptors=false passphrase=""
{
  "name": "oops2",
  "warnings": [
    "Empty string given as passphrase, wallet will not be encrypted.",
    "Wallet created successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future."
  ]
}

I did a quick search for other RPCs that string together warnings, I think getblockchaininfo still does - worth including here?

Also, the GUI: I don't think encrypting wallet with a blank passphrase is possible from the UI so I don't know if there is a way to see multiple warnings in production, or if that is already handled.

jonatack commented at 5:34 PM on April 4, 2023: contributor

Thanks for testing!

I did a quick search for other RPCs that string together warnings, I think getblockchaininfo still does - worth including here?

Info getter RPCs getblockchaininfo, getmininginfo and getnetworkinfo return a different kind of warnings -- global ones unrelated to an operation performed by the RPC -- and have their own GetWarnings helper in src/warnings, which is also called by the GUI via getWarnings. They don't seem tightly related to the ones here.

Also, the GUI: I don't think encrypting wallet with a blank passphrase is possible from the UI so I don't know if there is a way to see multiple warnings in production, or if that is already handled.

I think the GUI handles these operations at a higher (non-RPC) layer, e.g. wallet, interfaces, qt, so these changes shouldn't be applicable to it.

<details><summary><code>git grep "loadWallet\|createWallet\|restoreWallet\|UnloadWallet\|handleUnload" src/interfaces/wallet.h src/wallet/interfaces.cpp src/qt</code></summary><p>

src/interfaces/wallet.h:282:    virtual std::unique_ptr<Handler> handleUnload(UnloadFn fn) = 0;
src/interfaces/wallet.h:323:    virtual util::Result<std::unique_ptr<Wallet>> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, std::vector<bilingual_str>& warnings) = 0;
src/interfaces/wallet.h:326:    virtual util::Result<std::unique_ptr<Wallet>> loadWallet(const std::string& name, std::vector<bilingual_str>& warnings) = 0;
src/interfaces/wallet.h:332:    virtual util::Result<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector<bilingual_str>& warnings) = 0;
src/interfaces/wallet.h:341:    //! createWallet and loadWallet above, and also triggered when wallets are
src/qt/bitcoingui.cpp:111:        connect(walletFrame, &WalletFrame::createWalletButtonClicked, [this] {
src/qt/walletcontroller.cpp:233:        createWallet();
src/qt/walletcontroller.cpp:240:void CreateWalletActivity::createWallet()
src/qt/walletcontroller.cpp:265:        auto wallet{node().walletLoader().createWallet(name, m_passphrase, flags, m_warning_message)};
src/qt/walletcontroller.cpp:319:            createWallet();
src/qt/walletcontroller.cpp:354:        auto wallet{node().walletLoader().loadWallet(path, m_warning_message)};
src/qt/walletcontroller.cpp:406:        auto wallet{node().walletLoader().restoreWallet(backup_file, wallet_name, m_warning_message)};
src/qt/walletcontroller.h:128:    void createWallet();
src/qt/walletframe.cpp:51:    connect(create_wallet_button, &QPushButton::clicked, this, &WalletFrame::createWalletButtonClicked);
src/qt/walletframe.h:50:    void createWalletButtonClicked();
src/qt/walletmodel.cpp:429:    m_handler_unload = m_wallet->handleUnload(std::bind(&NotifyUnload, this));
src/wallet/interfaces.cpp:488:    std::unique_ptr<Handler> handleUnload(UnloadFn fn) override
src/wallet/interfaces.cpp:533:    ~WalletLoaderImpl() override { UnloadWallets(m_context); }
src/wallet/interfaces.cpp:555:    util::Result<std::unique_ptr<Wallet>> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, std::vector<bilingual_str>& warnings) override
src/wallet/interfaces.cpp:571:    util::Result<std::unique_ptr<Wallet>> loadWallet(const std::string& name, std::vector<bilingual_str>& warnings) override
src/wallet/interfaces.cpp:585:    util::Result<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector<bilingual_str>& warnings) override

</p></details>

in test/functional/wallet_backwards_compatibility.py:268 in ba11479b45 outdated

 264 | @@ -265,7 +265,12 @@ def run_test(self):
 265 |              )
 266 |              load_res = node_master.loadwallet("u1_v16")
 267 |              # Make sure this wallet opens without warnings. See https://github.com/bitcoin/bitcoin/pull/19054
 268 | -            assert_equal(load_res['warning'], '')
 269 | +            if '"warnings"' in node_master.help("loadwallet"):

pinheadmz commented at 6:23 PM on April 5, 2023:

Is this the best way to test for this? Seems like it could be broken by someone using a word in a future doc update. You can't get bitcoind --version or something for the back-compat test?

jonatack commented at 5:01 PM on April 10, 2023:

It would only be broken if the word "warnings" wrapped in double quotes is backported into the loadwallet help in a previous release, but to address your request replaced this line with if int(node_master.getnetworkinfo()["version"]) >= 249900:

in test/functional/wallet_createwallet.py:28 in ba11479b45 outdated

  24 | @@ -25,6 +25,7 @@ def add_options(self, parser):
  25 |  
  26 |      def set_test_params(self):
  27 |          self.num_nodes = 1
  28 | +        self.extra_args = [["-deprecatedrpc=walletwarningfield"]]

pinheadmz commented at 6:26 PM on April 5, 2023:

Why do you set this flag for the entire test? Since the warnings is the new thing going forward, wouldn't it make more sense to write all the tests with only that field by default? And just add one test case with the flag to check warning was also included?

jonatack commented at 3:18 PM on April 10, 2023:

Either way someone will ask why it isn't done the other way, ie #17578#pullrequestreview-338232718 so went with the smallest diff.

in src/wallet/wallet.h:148 in 28339b0733 outdated

 144 | @@ -145,8 +145,6 @@ static const std::map<std::string,WalletFlags> WALLET_FLAG_MAP{
 145 |      {"external_signer", WALLET_FLAG_EXTERNAL_SIGNER}
 146 |  };
 147 |  
 148 | -extern const std::map<uint64_t,std::string> WALLET_FLAG_CAVEATS;

pinheadmz commented at 6:36 PM on April 5, 2023:

so no need to encumber wallet.h and wallet.cpp with it

I'm not sure I totally agree with this, but just because all the flags are defined here already. Like, you'd need to import this header file to use WALLET_FLAG_AVOID_REUSE anyway, why not keep the caveats right next to their associated flags?

jonatack commented at 3:04 PM on April 10, 2023:

WALLET_FLAG_AVOID_REUSE is defined in walletutil.h, not in wallet.h, and per git grep "#include <wallet/wallet.h>" | wc -l this commit would avoid needlessly adding WALLET_FLAG_CAVEATS to nearly fifty compilation units.

I'll add #include <wallet/walletutil.h> to wallet/rpc/wallet.cpp in this change. That include was indirectly happening via the one in wallet.h but should be added to this file.

pinheadmz commented at 6:41 PM on April 5, 2023: member

code review ACK @ e67da5fedd526c38e030d6d18f3678313a683dc9

A few questions.

TheCharlatan commented at 11:23 AM on April 6, 2023: contributor

Approach ACK

in src/wallet/rpc/backup.cpp:1908 in e67da5fedd outdated

1902 | @@ -1903,7 +1903,11 @@ RPCHelpMan restorewallet()
1903 |              RPCResult::Type::OBJ, "", "",
1904 |              {
1905 |                  {RPCResult::Type::STR, "name", "The wallet name if restored successfully."},
1906 | -                {RPCResult::Type::STR, "warning", "Warning message if wallet was not loaded cleanly."},
1907 | +                {RPCResult::Type::STR, "warning", /*optional=*/true, "Warning messages, if any, related to restoring the wallet. Multiple messages will be delimited by newline escape sequences (\"\\n\"). (DEPRECATED, returned only if config option -deprecatedrpc=walletwarningfield is passed.)"},
1908 | +                {RPCResult::Type::ARR, "warnings", /*optional=*/true, "Warning messages, if any, related to restoring the wallet.",
1909 | +                {

vasild commented at 11:50 AM on April 7, 2023:

nit, style: this indentation is misleading because the two opening {s have the same indentation, but actually the second one is nested in the first. This way it gives the wrong impression that there is a closing } at the end of the first line. I guess if you run clang-format it will totally change everything (ie the surrounding "100" lines)?

jonatack commented at 2:12 PM on April 10, 2023:

I followed the indentation style used for the same warnings field in RPCs createmultisig, addmultisigaddress, importmulti, importdescriptors, etc., as yes, clang-format would change the whole function.

in src/wallet/rpc/backup.cpp:1906 in e67da5fedd outdated

1902 | @@ -1903,7 +1903,11 @@ RPCHelpMan restorewallet()
1903 |              RPCResult::Type::OBJ, "", "",
1904 |              {
1905 |                  {RPCResult::Type::STR, "name", "The wallet name if restored successfully."},
1906 | -                {RPCResult::Type::STR, "warning", "Warning message if wallet was not loaded cleanly."},
1907 | +                {RPCResult::Type::STR, "warning", /*optional=*/true, "Warning messages, if any, related to restoring the wallet. Multiple messages will be delimited by newline escape sequences (\"\\n\"). (DEPRECATED, returned only if config option -deprecatedrpc=walletwarningfield is passed.)"},

vasild commented at 12:01 PM on April 7, 2023:

nit: actual newlines are used (0A), not newline escape sequences (5C 6E), thus:

                {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.)"},

jonatack commented at 5:19 PM on April 10, 2023:

Done.

in test/functional/wallet_createwallet.py:200 in e67da5fedd outdated

 197 | +            })
 198 | +
 199 | +        self.log.info('Test "warning" field deprecation, i.e. not returned without -deprecatedrpc=walletwarningfield')
 200 | +        self.restart_node(0, extra_args=[])
 201 | +        result = self.nodes[0].createwallet(wallet_name="w7_again", disable_private_keys=False, blank=False, passphrase="")
 202 | +        assert_equal(set([*result]), {"name", "warnings"})

vasild commented at 12:20 PM on April 7, 2023:

nit: this would needlessly fail if a new unrelated field is added. Here we actually care that warning is not in the list, something like assert("warning" not in result);

jonatack commented at 4:53 PM on April 10, 2023:

Done.

in src/wallet/rpc/backup.cpp:1382 in e67da5fedd outdated

1378 | @@ -1379,7 +1379,7 @@ RPCHelpMan importmulti()
1379 |  
1380 |          for (const UniValue& data : requests.getValues()) {
1381 |              const int64_t timestamp = std::max(GetImportTimestamp(data, now), minimumTimestamp);
1382 | -            const UniValue result = ProcessImport(*pwallet, data, timestamp);
1383 | +            const UniValue& result = ProcessImport(*pwallet, data, timestamp);

vasild commented at 12:34 PM on April 7, 2023:

Commit 363b422983e01a532df67b2432cb13e47bf092e0 rpc: rm unneeded UniValue copies in importmulti and importdescriptors is useless. There is not any copying happening and the old and the new code are the same wrt how many objects are being created and destroyed.

/* The rule of five. */
class R5
{
public:
    R5(int x, double y)
    {
        x_ = x;
        y_ = y;
        std::cout << "R5::R5() this=" << this << " x=" << x_ << ", y=" << y_ << std::endl;
    }

    ~R5()
    {
        std::cout << "R5::~R5() this=" << this << " x=" << x_ << ", y=" << y_ << std::endl;
    }

    R5(const R5& other)
#if 0
        = delete;
#else
    {
        std::cout << "R5::R5(const R5&) this=" << this << " other=" << &other << " x=" << other.x_
                  << ", y=" << other.y_ << std::endl;
        x_ = other.x_;
        y_ = other.y_;
    }
#endif

    R5(R5&& other)
    {
        std::cout << "R5::R5(R5&&) this=" << this << " other=" << &other << " x=" << other.x_
                  << ", y=" << other.y_ << std::endl;
        x_ = other.x_;
        y_ = other.y_;
        other.x_ = 0;
        other.y_ = 0;
    }

    R5& operator=(const R5& other)
#if 0
        = delete;
#else
    {
        std::cout << "R5::operator=(const R5&) this=" << this << " x=" << x_ << ", y=" << y_
                  << " other=" << &other << " x=" << other.x_ << ", y=" << other.y_ << std::endl;
        x_ = other.x_;
        y_ = other.y_;
        return *this;
    }
#endif

    R5& operator=(R5&& other)
    {
        std::cout << "R5::operator=(R5&&) this=" << this << " x=" << x_ << ", y=" << y_
                  << " other=" << &other << " x=" << other.x_ << ", y=" << other.y_ << std::endl;
        x_ = other.x_;
        y_ = other.y_;
        other.x_ = 0;
        other.y_ = 0;
        return *this;
    }

    std::ostream& operator<<(std::ostream& os) { return os << "(x=" << x_ << ", y=" << y_ << ")"; }

    int x_;
    double y_;
};

R5 f()
{
    R5 result{1, 2.3};
    result.x_ = 11;
    return result;
}

int main(int, char**)
{
    std::cout << "before f()\n";
    const R5 r5 = f();
    std::cout << "after f()\n";

    return 0;
}

prints the same with const R5 r5 and with const R5& r5.

vasild commented at 12:51 PM on April 7, 2023:

My code is very similar to the example in https://en.cppreference.com/w/cpp/language/copy_elision. From that doc it seems compilers are not required to do that optimization, which was my thinking. Anyway, if all supported compilers do that optimization, then this change is still unnecessary.

jonatack commented at 2:31 PM on April 10, 2023:

Thanks for looking! I'm not sure whether compilers optimize for it but happy to drop that unrelated change, done.

vasild approved

vasild commented at 12:47 PM on April 7, 2023: contributor

ACK e67da5fedd526c38e030d6d18f3678313a683dc9

Some non-blockers below.

doc: fix/improve warning helps in {create,load,unload,restore}wallet

- clarify that there can be multiple warning messages
- specify the correct wallet action
- describe the use of newlines as delimiters

f73782a903

rpc: extract wallet "warnings" fields to a util helper 079d8cdda8

rpc: add "warnings" field to RPCs {create,load,unload,restore}wallet

This new "warnings" field is a JSON array of strings intended to replace the
"warning" string field in these four RPCs, to better handle returning multiple
warning messages and for consistency with other wallet RPCs.

4a1e479ca6

test: add test coverage for "warnings" field in createwallet

and clarify the "warning" field behavior.

2f4a926e95

rpc: deprecate "warning" field in {create,load,unload,restore}wallet

This string field has been replaced in these four RPCs by a "warnings" field
returning a JSON array of strings.

645d7f75ac

test: createwallet "warning" field deprecation test 9ea8b3739a

doc: release note for wallet RPCs "warning" field deprecation 01df011ca2

rpc: move WALLET_FLAG_CAVEATS to the compilation unit of its caller

and add the walletutil.h include header for WALLET_FLAG_AVOID_REUSE that was
already missing before this change.

WALLET_FLAG_CAVEATS is only used in one RPC, so no need to encumber wallet.h and
wallet.cpp with it, along with all of the files that include wallet.h during
their compilation. Also apply clang-format per:

git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v

19d888ce40

test: fix importmulti/importdescriptors assertion

as these RPCs have a "warnings" field, not a "warning" one.

7ccdd741fe

jonatack force-pushed on Apr 10, 2023

jonatack commented at 7:09 PM on April 10, 2023: contributor

Thank you @pinheadmz, @vasild and @TheCharlatan. Repushed to address feedback, should be trivial to re-ACK.

diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index fc7d5a5c282..fb175fa2532 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -1379,7 +1379,7 @@ RPCHelpMan importmulti()
 
         for (const UniValue& data : requests.getValues()) {
             const int64_t timestamp = std::max(GetImportTimestamp(data, now), minimumTimestamp);
-            const UniValue& result = ProcessImport(*pwallet, data, timestamp);
+            const UniValue result = ProcessImport(*pwallet, data, timestamp);
             response.push_back(result);
 
             if (!fRescan) {
@@ -1675,7 +1675,7 @@ RPCHelpMan importdescriptors()
         for (const UniValue& request : requests.getValues()) {
             // This throws an error if "timestamp" doesn't exist
             const int64_t timestamp = std::max(GetImportTimestamp(request, now), minimum_timestamp);
-            const UniValue& result = ProcessDescriptorImport(*pwallet, request, timestamp);
+            const UniValue result = ProcessDescriptorImport(*pwallet, request, timestamp);
             response.push_back(result);
 
             if (lowest_timestamp > timestamp ) {
@@ -1903,7 +1903,7 @@ RPCHelpMan restorewallet()
             RPCResult::Type::OBJ, "", "",
             {
                 {RPCResult::Type::STR, "name", "The wallet name if restored successfully."},
-                {RPCResult::Type::STR, "warning", /*optional=*/true, "Warning messages, if any, related to restoring the wallet. Multiple messages will be delimited by newline escape sequences (\"\\n\"). (DEPRECATED, returned only if config option -deprecatedrpc=walletwarningfield is passed.)"},
+                {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, "", ""},
diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp
index 33466510b03..ea3507bc751 100644
--- a/src/wallet/rpc/wallet.cpp
+++ b/src/wallet/rpc/wallet.cpp
@@ -13,6 +13,7 @@
 #include <wallet/rpc/wallet.h>
 #include <wallet/rpc/util.h>
 #include <wallet/wallet.h>
+#include <wallet/walletutil.h>
 
 #include <optional>
 
@@ -215,7 +216,7 @@ static RPCHelpMan loadwallet()
                     RPCResult::Type::OBJ, "", "",
                     {
                         {RPCResult::Type::STR, "name", "The wallet name if loaded successfully."},
-                        {RPCResult::Type::STR, "warning", /*optional=*/true, "Warning messages, if any, related to loading the wallet. Multiple messages will be delimited by newline escape sequences (\"\\n\"). (DEPRECATED, returned only if config option -deprecatedrpc=walletwarningfield is passed.)"},
+                        {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, "", ""},
@@ -350,7 +351,7 @@ 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", /*optional=*/true, "Warning messages, if any, related to creating the wallet. Multiple messages will be delimited by newline escape sequences (\"\\n\"). (DEPRECATED, returned only if config option -deprecatedrpc=walletwarningfield is passed.)"},
+                {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, "", ""},
@@ -444,7 +445,7 @@ 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", /*optional=*/true, "Warning messages, if any, related to unloading the wallet. Multiple messages will be delimited by newline escape sequences (\"\\n\"). (DEPRECATED, returned only if config option -deprecatedrpc=walletwarningfield is passed.)"},
+                    {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, "", ""},
diff --git a/test/functional/wallet_backwards_compatibility.py b/test/functional/wallet_backwards_compatibility.py
index e02b0c318e9..a6401a76c13 100755
--- a/test/functional/wallet_backwards_compatibility.py
+++ b/test/functional/wallet_backwards_compatibility.py
@@ -265,7 +265,7 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
             )
             load_res = node_master.loadwallet("u1_v16")
             # Make sure this wallet opens without warnings. See [#19054](/bitcoin-bitcoin/19054/)
-            if '"warnings"' in node_master.help("loadwallet"):
+            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:
diff --git a/test/functional/wallet_createwallet.py b/test/functional/wallet_createwallet.py
index dc24151f77b..58cc6befbd3 100755
--- a/test/functional/wallet_createwallet.py
+++ b/test/functional/wallet_createwallet.py
@@ -197,7 +197,7 @@ class CreateWalletTest(BitcoinTestFramework):
         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_equal(set([*result]), {"name", "warnings"})
+        assert "warning" not in result

</p></details>

achow101 added this to the milestone 25.0 on Apr 11, 2023

unknown approved

unknown commented at 7:33 AM on April 11, 2023: none

utACK https://github.com/bitcoin/bitcoin/pull/27279/commits/7ccdd741fe1544c13b2a9b7baa5c5727e84d6e55

DrahtBot requested review from pinheadmz on Apr 11, 2023

DrahtBot requested review from vasild on Apr 11, 2023

vasild approved

vasild commented at 1:08 PM on April 11, 2023: contributor

ACK 7ccdd741fe1544c13b2a9b7baa5c5727e84d6e55

achow101 removed this from the milestone 25.0 on Apr 11, 2023

pinheadmz approved

pinheadmz commented at 3:50 PM on April 12, 2023: member

re-ACK 7ccdd741fe1544c13b2a9b7baa5c5727e84d6e55

confirmed trivial diff since last review, ran unit and functional tests locally, ran in regtest one more time to check UX with and without deprecated rpc flag

<details><summary>Show Signature</summary>

pinheadmz's public key is on keybase

</details>

achow101 commented at 5:02 PM on April 12, 2023: member

ACK 7ccdd741fe1544c13b2a9b7baa5c5727e84d6e55

achow101 merged this on Apr 12, 2023

achow101 closed this on Apr 12, 2023

jonatack deleted the branch on Apr 12, 2023

sidhujag referenced this in commit 3d89f7a1c7 on Apr 12, 2023

fanquake referenced this in commit fc8c1a8deb on Apr 18, 2023

fanquake referenced this in commit 15a24781d0 on Apr 18, 2023

achow101 referenced this in commit f0758d8a66 on Jun 16, 2023

sidhujag referenced this in commit 89506d89ba on Jun 19, 2023

bitcoin locked this on Apr 11, 2024