rpc: remove deprecated "warning" field from {create,load,restore,unload}wallet #27757

pull theStack wants to merge 3 commits into bitcoin:master from theStack:202305-deprecate_walletwarningfield changing 4 files +16 −31

theStack commented at 2:47 PM on May 25, 2023: contributor

The "warning" string field for wallet creating/loading RPCs (createwallet, loadwallet, unloadwallet and restorewallet) has been deprecated with the configuration option -deprecatedrpc=walletwarningfield in PR #27279 (released in v25.0). For the next release v26.0, the field and the configuration option can be removed.
DrahtBot commented at 2:47 PM on May 25, 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 jonatack, achow101

Stale ACK kevkevinpal

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:
- #27322 (Move IsDeprecatedRPCEnabled to rpc/util, rm redundant rpcEnableDeprecated by jonatack)
- #20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)
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.
DrahtBot added the label RPC/REST/ZMQ on May 25, 2023
theStack force-pushed on May 25, 2023
theStack force-pushed on May 25, 2023
DrahtBot added the label CI failed on May 25, 2023
theStack force-pushed on May 25, 2023
DrahtBot removed the label CI failed on May 25, 2023

Type	Reviewers
ACK	jonatack, achow101
Stale ACK	kevkevinpal

kevkevinpal commented at 6:14 PM on May 26, 2023: contributor

ACK d78f9a7

compiled and ran the code below and got the following response

--> ./src/bitcoin-cli -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."
  ]
}

in src/wallet/rpc/wallet.cpp:496 in d78f9a7ed7 outdated
```
 481 | @@ -491,9 +482,6 @@ static RPCHelpMan unloadwallet()
 482 |          }
 483 |      }
 484 |      UniValue result(UniValue::VOBJ);
 485 | -    if (wallet->chain().rpcEnableDeprecated("walletwarningfield")) {
 486 | -        result.pushKV("warning", Join(warnings, Untranslated("\n")).original);
 487 | -    }
```
jonatack commented at 10:57 AM on May 31, 2023:

See the diff here in https://github.com/jonatack/bitcoin/commits/2023-05-remove-deprecated-warning-fields making the same change a few weeks ago. I moved the move back to its original position before the deprecation, as we no longer need to access wallet for the deprecation.

theStack commented at 10:27 AM on June 2, 2023:

Thanks, included the move back of the UnloadWallet call and added you as co-author.

in doc/release-notes-27757.md:7 in d78f9a7ed7 outdated

   0 | @@ -0,0 +1,8 @@
   1 | +Wallet
   2 | +------
   3 | +
   4 | +- The `deprecatedrpc=walletwarningfield` configuration option has been removed.
   5 | +  The `createwallet`, `loadwallet`, `restorewallet` and `unloadwallet` RPCs
   6 | +  no longer return the "warning" string field. The same information can be
   7 | +  accessed through the "warnings" field which returns a JSON array of strings.

jonatack commented at 11:01 AM on May 31, 2023:

Suggestions/fixups

s/can be accessed/is provided/
missing comma before "which"
s/single string field/"warning" string field/ seems clearer
perhaps mention when the "warnings" field was added

  accessed with the "warnings" field added in v25.0, which returns a JSON array of strings.

theStack commented at 10:27 AM on June 2, 2023:

Thanks, done.

jonatack commented at 11:06 AM on May 31, 2023: contributor

Approach ACK. Did this a few weeks back in https://github.com/bitcoin/bitcoin/compare/master...jonatack:bitcoin:2023-05-remove-deprecated-warning-fields and the diff is the same except as where noted below. Feel free to pull in 39f14968b6f6dd6151f4fbb05c8fe385cff9a15e in that branch if you think it's worthwhile.
theStack force-pushed on Jun 2, 2023
theStack commented at 10:29 AM on June 2, 2023: contributor

@jonatack: Oh sorry, wasn't aware you already put time into this. Pulled in your documentation fixup commit and took also your other suggestions for the removal and release note commits.
rpc: remove deprecated "warning" field from {create,load,restore,unload}wallet
Co-authored-by: Jon Atack <jon@atack.com>
a00ae31fcc
Restorewallet/createwallet help documentation fixups/improvements 5c77db7354
doc: add release note about removal of `deprecatedrpc=walletwarningfield` flag
Co-authored-by: Jon Atack <jon@atack.com>
5524fa00fa
theStack force-pushed on Jun 4, 2023
jonatack approved
jonatack commented at 3:55 PM on June 15, 2023: contributor

@theStack Oh, you couldn't know. Thanks for updating!

ACK 5524fa00faebfe040f126a4152640f9e9ed572b1
fanquake requested review from achow101 on Jun 16, 2023
achow101 commented at 7:05 PM on June 16, 2023: member

ACK 5524fa00faebfe040f126a4152640f9e9ed572b1
DrahtBot removed review request from achow101 on Jun 16, 2023
achow101 merged this on Jun 16, 2023
achow101 closed this on Jun 16, 2023
theStack deleted the branch on Jun 16, 2023
sidhujag referenced this in commit 89506d89ba on Jun 19, 2023
bitcoin locked this on Jun 15, 2024

Contributors

Labels

Linked (view graph)

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

rpc: remove deprecated "warning" field from {create,load,restore,unload}wallet #27757

Reviews

Conflicts