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

pull jonatack wants to merge 9 commits into bitcoin:master from jonatack:2023-03-migrate-wallet-warning-fields-to-warnings changing 11 files +125 −28
  1. 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.

  2. 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.

  3. 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
  4. 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
  5. 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
  6. jonatack force-pushed on Mar 20, 2023
  7. 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):

     0void PushWarnings(const std::vector<bilingual_str>& warnings, UniValue& to)
     1{
     2    if (IsDeprecatedRPCEnabled("warningfield")) {
     3        // The "warning" field is deprecated in v25 for removal in v26.
     4        to.pushKV("warning", Join(warnings, Untranslated("\n")).original);
     5    }
     6    if (warnings.empty()) {
     7        return;
     8    }
     9    to.pushKV("warnings", UniValue{UniValue::VARR});
    10    for (const auto& s : warnings) {
    11        to["warnings"].push_back(s.original);
    12    }
    13}
    

    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 0:12 am on March 30, 2023:
    Done, thanks.
  8. vasild commented at 4:47 am on March 22, 2023: contributor
    Concept ACK
  9. jonatack force-pushed on Mar 23, 2023
  10. 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/
  11. 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
  12. jonatack force-pushed on Mar 23, 2023
  13. jonatack force-pushed on Mar 23, 2023
  14. jonatack force-pushed on Mar 29, 2023
  15. jonatack force-pushed on Mar 30, 2023
  16. jonatack force-pushed on Mar 30, 2023
  17. jonatack force-pushed on Mar 30, 2023
  18. jonatack marked this as ready for review on Mar 30, 2023
  19. jonatack force-pushed on Mar 30, 2023
  20. 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!

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

    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.

  21. 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.

     0src/interfaces/wallet.h:282:    virtual std::unique_ptr<Handler> handleUnload(UnloadFn fn) = 0;
     1src/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;
     2src/interfaces/wallet.h:326:    virtual util::Result<std::unique_ptr<Wallet>> loadWallet(const std::string& name, std::vector<bilingual_str>& warnings) = 0;
     3src/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;
     4src/interfaces/wallet.h:341:    //! createWallet and loadWallet above, and also triggered when wallets are
     5src/qt/bitcoingui.cpp:111:        connect(walletFrame, &WalletFrame::createWalletButtonClicked, [this] {
     6src/qt/walletcontroller.cpp:233:        createWallet();
     7src/qt/walletcontroller.cpp:240:void CreateWalletActivity::createWallet()
     8src/qt/walletcontroller.cpp:265:        auto wallet{node().walletLoader().createWallet(name, m_passphrase, flags, m_warning_message)};
     9src/qt/walletcontroller.cpp:319:            createWallet();
    10src/qt/walletcontroller.cpp:354:        auto wallet{node().walletLoader().loadWallet(path, m_warning_message)};
    11src/qt/walletcontroller.cpp:406:        auto wallet{node().walletLoader().restoreWallet(backup_file, wallet_name, m_warning_message)};
    12src/qt/walletcontroller.h:128:    void createWallet();
    13src/qt/walletframe.cpp:51:    connect(create_wallet_button, &QPushButton::clicked, this, &WalletFrame::createWalletButtonClicked);
    14src/qt/walletframe.h:50:    void createWalletButtonClicked();
    15src/qt/walletmodel.cpp:429:    m_handler_unload = m_wallet->handleUnload(std::bind(&NotifyUnload, this));
    16src/wallet/interfaces.cpp:488:    std::unique_ptr<Handler> handleUnload(UnloadFn fn) override
    17src/wallet/interfaces.cpp:533:    ~WalletLoaderImpl() override { UnloadWallets(m_context); }
    18src/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
    19src/wallet/interfaces.cpp:571:    util::Result<std::unique_ptr<Wallet>> loadWallet(const std::string& name, std::vector<bilingual_str>& warnings) override
    20src/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
    
  22. 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:
  23. 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.
  24. 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.

  25. pinheadmz commented at 6:41 pm on April 5, 2023: member

    code review ACK @ e67da5fedd526c38e030d6d18f3678313a683dc9

    A few questions.

  26. TheCharlatan commented at 11:23 am on April 6, 2023: contributor
    Approach ACK
  27. 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.
  28. 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:

    0                {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.
  29. 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.
  30. 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.

     0/* The rule of five. */
     1class R5
     2{
     3public:
     4    R5(int x, double y)
     5    {
     6        x_ = x;
     7        y_ = y;
     8        std::cout << "R5::R5() this=" << this << " x=" << x_ << ", y=" << y_ << std::endl;
     9    }
    10
    11    ~R5()
    12    {
    13        std::cout << "R5::~R5() this=" << this << " x=" << x_ << ", y=" << y_ << std::endl;
    14    }
    15
    16    R5(const R5& other)
    17#if 0
    18        = delete;
    19#else
    20    {
    21        std::cout << "R5::R5(const R5&) this=" << this << " other=" << &other << " x=" << other.x_
    22                  << ", y=" << other.y_ << std::endl;
    23        x_ = other.x_;
    24        y_ = other.y_;
    25    }
    26#endif
    27
    28    R5(R5&& other)
    29    {
    30        std::cout << "R5::R5(R5&&) this=" << this << " other=" << &other << " x=" << other.x_
    31                  << ", y=" << other.y_ << std::endl;
    32        x_ = other.x_;
    33        y_ = other.y_;
    34        other.x_ = 0;
    35        other.y_ = 0;
    36    }
    37
    38    R5& operator=(const R5& other)
    39#if 0
    40        = delete;
    41#else
    42    {
    43        std::cout << "R5::operator=(const R5&) this=" << this << " x=" << x_ << ", y=" << y_
    44                  << " other=" << &other << " x=" << other.x_ << ", y=" << other.y_ << std::endl;
    45        x_ = other.x_;
    46        y_ = other.y_;
    47        return *this;
    48    }
    49#endif
    50
    51    R5& operator=(R5&& other)
    52    {
    53        std::cout << "R5::operator=(R5&&) this=" << this << " x=" << x_ << ", y=" << y_
    54                  << " other=" << &other << " x=" << other.x_ << ", y=" << other.y_ << std::endl;
    55        x_ = other.x_;
    56        y_ = other.y_;
    57        other.x_ = 0;
    58        other.y_ = 0;
    59        return *this;
    60    }
    61
    62    std::ostream& operator<<(std::ostream& os) { return os << "(x=" << x_ << ", y=" << y_ << ")"; }
    63
    64    int x_;
    65    double y_;
    66};
    67
    68R5 f()
    69{
    70    R5 result{1, 2.3};
    71    result.x_ = 11;
    72    return result;
    73}
    74
    75int main(int, char**)
    76{
    77    std::cout << "before f()\n";
    78    const R5 r5 = f();
    79    std::cout << "after f()\n";
    80
    81    return 0;
    82}
    

    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.
  31. vasild approved
  32. vasild commented at 12:47 pm on April 7, 2023: contributor

    ACK e67da5fedd526c38e030d6d18f3678313a683dc9

    Some non-blockers below.

  33. 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
  34. rpc: extract wallet "warnings" fields to a util helper 079d8cdda8
  35. 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
  36. test: add test coverage for "warnings" field in createwallet
    and clarify the "warning" field behavior.
    2f4a926e95
  37. 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
  38. test: createwallet "warning" field deprecation test 9ea8b3739a
  39. doc: release note for wallet RPCs "warning" field deprecation 01df011ca2
  40. 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
  41. test: fix importmulti/importdescriptors assertion
    as these RPCs have a "warnings" field, not a "warning" one.
    7ccdd741fe
  42. jonatack force-pushed on Apr 10, 2023
  43. 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.

     0diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
     1index fc7d5a5c282..fb175fa2532 100644
     2--- a/src/wallet/rpc/backup.cpp
     3+++ b/src/wallet/rpc/backup.cpp
     4@@ -1379,7 +1379,7 @@ RPCHelpMan importmulti()
     5 
     6         for (const UniValue& data : requests.getValues()) {
     7             const int64_t timestamp = std::max(GetImportTimestamp(data, now), minimumTimestamp);
     8-            const UniValue& result = ProcessImport(*pwallet, data, timestamp);
     9+            const UniValue result = ProcessImport(*pwallet, data, timestamp);
    10             response.push_back(result);
    11 
    12             if (!fRescan) {
    13@@ -1675,7 +1675,7 @@ RPCHelpMan importdescriptors()
    14         for (const UniValue& request : requests.getValues()) {
    15             // This throws an error if "timestamp" doesn't exist
    16             const int64_t timestamp = std::max(GetImportTimestamp(request, now), minimum_timestamp);
    17-            const UniValue& result = ProcessDescriptorImport(*pwallet, request, timestamp);
    18+            const UniValue result = ProcessDescriptorImport(*pwallet, request, timestamp);
    19             response.push_back(result);
    20 
    21             if (lowest_timestamp > timestamp ) {
    22@@ -1903,7 +1903,7 @@ RPCHelpMan restorewallet()
    23             RPCResult::Type::OBJ, "", "",
    24             {
    25                 {RPCResult::Type::STR, "name", "The wallet name if restored successfully."},
    26-                {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.)"},
    27+                {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.)"},
    28                 {RPCResult::Type::ARR, "warnings", /*optional=*/true, "Warning messages, if any, related to restoring the wallet.",
    29                 {
    30                     {RPCResult::Type::STR, "", ""},
    31diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp
    32index 33466510b03..ea3507bc751 100644
    33--- a/src/wallet/rpc/wallet.cpp
    34+++ b/src/wallet/rpc/wallet.cpp
    35@@ -13,6 +13,7 @@
    36 #include <wallet/rpc/wallet.h>
    37 #include <wallet/rpc/util.h>
    38 #include <wallet/wallet.h>
    39+#include <wallet/walletutil.h>
    40 
    41 #include <optional>
    42 
    43@@ -215,7 +216,7 @@ static RPCHelpMan loadwallet()
    44                     RPCResult::Type::OBJ, "", "",
    45                     {
    46                         {RPCResult::Type::STR, "name", "The wallet name if loaded successfully."},
    47-                        {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.)"},
    48+                        {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.)"},
    49                         {RPCResult::Type::ARR, "warnings", /*optional=*/true, "Warning messages, if any, related to loading the wallet.",
    50                         {
    51                             {RPCResult::Type::STR, "", ""},
    52@@ -350,7 +351,7 @@ static RPCHelpMan createwallet()
    53             RPCResult::Type::OBJ, "", "",
    54             {
    55                 {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."},
    56-                {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.)"},
    57+                {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.)"},
    58                 {RPCResult::Type::ARR, "warnings", /*optional=*/true, "Warning messages, if any, related to creating the wallet.",
    59                 {
    60                     {RPCResult::Type::STR, "", ""},
    61@@ -444,7 +445,7 @@ static RPCHelpMan unloadwallet()
    62                     {"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."},
    63                 },
    64                 RPCResult{RPCResult::Type::OBJ, "", "", {
    65-                    {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.)"},
    66+                    {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.)"},
    67                     {RPCResult::Type::ARR, "warnings", /*optional=*/true, "Warning messages, if any, related to unloading the wallet.",
    68                     {
    69                         {RPCResult::Type::STR, "", ""},
    70diff --git a/test/functional/wallet_backwards_compatibility.py b/test/functional/wallet_backwards_compatibility.py
    71index e02b0c318e9..a6401a76c13 100755
    72--- a/test/functional/wallet_backwards_compatibility.py
    73+++ b/test/functional/wallet_backwards_compatibility.py
    74@@ -265,7 +265,7 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
    75             )
    76             load_res = node_master.loadwallet("u1_v16")
    77             # Make sure this wallet opens without warnings. See [#19054](/bitcoin-bitcoin/19054/)
    78-            if '"warnings"' in node_master.help("loadwallet"):
    79+            if int(node_master.getnetworkinfo()["version"]) >= 249900:
    80                 # loadwallet#warnings (added in v25) -- only present if there is a warning
    81                 assert "warnings" not in load_res
    82             else:
    83diff --git a/test/functional/wallet_createwallet.py b/test/functional/wallet_createwallet.py
    84index dc24151f77b..58cc6befbd3 100755
    85--- a/test/functional/wallet_createwallet.py
    86+++ b/test/functional/wallet_createwallet.py
    87@@ -197,7 +197,7 @@ class CreateWalletTest(BitcoinTestFramework):
    88         self.log.info('Test "warning" field deprecation, i.e. not returned without -deprecatedrpc=walletwarningfield')
    89         self.restart_node(0, extra_args=[])
    90         result = self.nodes[0].createwallet(wallet_name="w7_again", disable_private_keys=False, blank=False, passphrase="")
    91-        assert_equal(set([*result]), {"name", "warnings"})
    92+        assert "warning" not in result
    93 
    
  44. achow101 added this to the milestone 25.0 on Apr 11, 2023
  45. unknown approved
  46. DrahtBot requested review from pinheadmz on Apr 11, 2023
  47. DrahtBot requested review from vasild on Apr 11, 2023
  48. vasild approved
  49. vasild commented at 1:08 pm on April 11, 2023: contributor
    ACK 7ccdd741fe1544c13b2a9b7baa5c5727e84d6e55
  50. achow101 removed this from the milestone 25.0 on Apr 11, 2023
  51. pinheadmz approved
  52. 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

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA256
     2
     3ACK 7ccdd741fe1544c13b2a9b7baa5c5727e84d6e55
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmQ20YEACgkQ5+KYS2KJ
     7yTqF/Q/8DnfN9Esae47Y1bvswkY5oX93SBB59pUEOHSxfTugRWFr18xZyUZI3zHz
     8kK8SFsVEXfm7lWTdMiRLjDSthFcsHMnzv+JNc76qvFuOhD5TYHkX81V7fUXSiDiS
     9VaODlGlVS/Cw6KoOQ52WWXltJxt0bIxPZMYBrtSus7poarSuCaOsXDqCaU+Ma6SM
    10KglES3j1+M4f7FIPUD6QVzHbhfYddTnFuwsBAgwMBxb/c7O8JrEWwxem80vc2jH4
    119erbjBhF8YotvH4ici8Xl0bO6/FmpldLz3CNRqbl7tYJOIxU+z+WsgpcI+D9C2jQ
    12NXOIL6bKCHom8MlOBJpTez892wRoTjV6SXNHa1TrlGuRFUVQpTb8QWMiP4RmSuFm
    13UcDyuwLkDOUGx09SviS2FjZA1BBogCsWiwD7ZpoBcW1sXqmFqGL9VCGrAzmuwqcd
    149xDn9vbzcvSEx8JdQvDYRpjdnQ8y2yt1VsgbpABRtb8uj16GLT1WiDCMFJFGxkV+
    153dSIIHe7DnNSw30v8YFoBYCbrXQBJLhNGu1AsLmlQJ3rpiULOAklRqWV4tE22cp8
    16BU8g9003FTC5gBTJlyXDdZTVkh82ucuTWAlpFQysHXswunmP6nmYvMDhWjSPf46d
    17jdhQo38SR7lZQ49cetwAOOy2G7TxOm7T0/vts2HUCp8Pyx6sjBo=
    18=Q/2X
    19-----END PGP SIGNATURE-----
    

    pinheadmz’s public key is on keybase

  53. achow101 commented at 5:02 pm on April 12, 2023: member
    ACK 7ccdd741fe1544c13b2a9b7baa5c5727e84d6e55
  54. achow101 merged this on Apr 12, 2023
  55. achow101 closed this on Apr 12, 2023

  56. jonatack deleted the branch on Apr 12, 2023
  57. sidhujag referenced this in commit 3d89f7a1c7 on Apr 12, 2023
  58. fanquake referenced this in commit fc8c1a8deb on Apr 18, 2023
  59. fanquake referenced this in commit 15a24781d0 on Apr 18, 2023
  60. achow101 referenced this in commit f0758d8a66 on Jun 16, 2023
  61. sidhujag referenced this in commit 89506d89ba on Jun 19, 2023
  62. bitcoin locked this on Apr 11, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-26 15:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me