wallet, rpc, doc: various legacy wallet removal cleanups in RPCs

theStack commented at 10:46 pm on May 22, 2025: contributor

This PR contains a few smaller wallet RPC cleanups based on that we only ever operate on descriptor wallets now:

remove the now obsolete “keypoololdest” field from the getwalletinfo RPC and the corresponding CWallet/ScriptPubKeyMan methods
in RPCs where potential fast wallet rescan is documented, remove the “descriptor wallet” mentions (back then introduced in commit ca48a4694f73e5be8f971ae482ebc2cce4caef44, PR #25957)
for the createwallet RPC examples, remove the “descriptors” parameters that always have to be true now (proposed in #31250 (review); corresponds to 86de8c1668005304b2c630ca2ad4a8ca8e348e90, PR #32544 which did the same for functional tests)

wallet, rpc: remove obsolete "keypoololdest" result field/code

This `getwalletinfo()` result field was only ever returned for
legacy wallets and is hence not relevant anymore, so we can
delete it and the corresponding CWallet/ScriptPubKeyMan code
behind it.

db465a50e2

rpc: doc: drop descriptor wallet mentions in fast wallet rescan related RPCs

Now that we only ever operate on descriptor wallets, mentioning
that a faster rescan is only available for them is redundant and
can be removed.

These texts were originally introduced in commit
ca48a4694f73e5be8f971ae482ebc2cce4caef44 (PR #25957).

7a05f941bb

rpc: doc: remove redundant "descriptors" parameter in `createwallet` examples

This is the RPC example counterpart to commit
86de8c1668005304b2c630ca2ad4a8ca8e348e90 (PR #32544).
Since the recent legacy wallet removal this parameter *must* be
true, so providing it in the examples doesn't contain valuable
information anymore and it seems best to remove them.

e5cbea416b

DrahtBot commented at 10:46 pm on May 22, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32596.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	1440000bytes, rkrux, achow101

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

1440000bytes commented at 0:21 am on May 23, 2025: none

ACK https://github.com/bitcoin/bitcoin/pull/32596/commits/e5cbea416b2f63e5d99819052f3e69a6383336d6

laanwj added the label Tests on May 23, 2025

in src/wallet/rpc/wallet.cpp:49 in db465a50e2 outdated

45@@ -46,7 +46,6 @@ static RPCHelpMan getwalletinfo()
46                         {RPCResult::Type::STR_AMOUNT, "unconfirmed_balance", "DEPRECATED. Identical to getbalances().mine.untrusted_pending"},
47                         {RPCResult::Type::STR_AMOUNT, "immature_balance", "DEPRECATED. Identical to getbalances().mine.immature"},
48                         {RPCResult::Type::NUM, "txcount", "the total number of transactions in the wallet"},
49-                        {RPCResult::Type::NUM_TIME, "keypoololdest", /*optional=*/true, "the " + UNIX_EPOCH_TIME + " of the oldest pre-generated key in the key pool. Legacy wallets only."},

rkrux commented at 2:55 pm on May 23, 2025:

I assume this is a breaking change but fine because it will go in the same release that contains the removal of legacy wallets as well.

theStack commented at 3:13 pm on May 23, 2025:

As this is an optional return field, I wouldn’t consider its removal as a breaking change, considering that the condition for returning a value (i.e. executing the RPC on a legacy wallet) can’t be fulfilled anymore. Curious about other opinions though; I guess one could argue that there is still a point in mentioning the removal in the release notes?

rkrux commented at 3:27 pm on May 23, 2025:

Ah, I note now that its presence was conditional on legacy wallets.

in src/wallet/rpc/wallet.cpp:355 in e5cbea416b

350@@ -351,8 +351,8 @@ static RPCHelpMan createwallet()
351         RPCExamples{
352             HelpExampleCli("createwallet", "\"testwallet\"")
353             + HelpExampleRpc("createwallet", "\"testwallet\"")
354-            + HelpExampleCliNamed("createwallet", {{"wallet_name", "descriptors"}, {"avoid_reuse", true}, {"descriptors", true}, {"load_on_startup", true}})
355-            + HelpExampleRpcNamed("createwallet", {{"wallet_name", "descriptors"}, {"avoid_reuse", true}, {"descriptors", true}, {"load_on_startup", true}})
356+            + HelpExampleCliNamed("createwallet", {{"wallet_name", "descriptors"}, {"avoid_reuse", true}, {"load_on_startup", true}})
357+            + HelpExampleRpcNamed("createwallet", {{"wallet_name", "descriptors"}, {"avoid_reuse", true}, {"load_on_startup", true}})

rkrux commented at 2:58 pm on May 23, 2025:

Nice, thanks: #31250 (review)

theStack commented at 3:06 pm on May 23, 2025:

Ah interesting, wasn’t aware that this was proposed already in an earlier PR. Added a link to your comment in the PR description.

rkrux approved

rkrux commented at 2:58 pm on May 23, 2025: contributor

ACK e5cbea416b2f63e5d99819052f3e69a6383336d6

achow101 commented at 7:20 pm on May 23, 2025: member

ACK e5cbea416b2f63e5d99819052f3e69a6383336d6

achow101 merged this on May 23, 2025

achow101 closed this on May 23, 2025

theStack deleted the branch on May 23, 2025

wallet, rpc, doc: various legacy wallet removal cleanups in RPCs #32596

Code Coverage & Benchmarks

Reviews