wallet: Remove `upgradewallet` RPC

w0xlt commented at 9:55 pm on July 10, 2025: contributor

Based on discussions in #32803, this PR proposes removing the upgradewallet RPC.

DrahtBot added the label Wallet on Jul 10, 2025

DrahtBot commented at 9:55 pm on July 10, 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/32944.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pablomartin4btc, brunoerg, maflcko, achow101
Concept ACK	Prabhat1308

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:

#32977 (wallet: Remove wallet version and several legacy related functions by w0xlt)
#32636 (Split CWallet::Create() into CreateNew and LoadExisting by davidgumberg)

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.

pablomartin4btc commented at 1:27 am on July 11, 2025: member

ACK 85eb29605ef4051ba23cd32e54a3d5a3ee203f6c

maflcko commented at 5:16 am on July 11, 2025: member

needs release note?

in src/wallet/wallet.cpp:3215 in 85eb29605e outdated

3210-        error = strprintf(_("Cannot upgrade a non HD split wallet from version %i to version %i without upgrading to support pre-split keypool. Please use version %i or no version specified."), prev_version, version, FEATURE_PRE_SPLIT_KEYPOOL);
3211-        return false;
3212-    }
3213-
3214-    // Permanently upgrade to the version
3215-    SetMinVersion(GetClosestWalletFeature(version));

Prabhat1308 commented at 6:58 am on July 11, 2025:

There is another solitary instance of SetMinVersion in this file . Since Createwallet will create a wallet with the newest version , that use is also a dead code . Might as well remove this function too.

pablomartin4btc commented at 12:18 pm on July 11, 2025:

Do we still need SetMinVersion() (GetVersion(), GetClosestWalletFeature(), IsFeatureSupported(), enum WalletFeature) at all?

Prabhat1308 commented at 9:21 pm on July 11, 2025:

I think these all functions can be deleted. For enum WalletFeature , would we want to keep it for historical purpose ? or delete it and document this elsewhere .

pablomartin4btc commented at 9:53 pm on July 11, 2025:

I don’t think we need it either, for the history we could still browse back this PR in the repo.

w0xlt commented at 4:29 am on July 14, 2025:

If we simply delete these functions, there will be issues with the getwalletinfo and migratewallet RPCs, as they call CanSupportFeature, which calls IsFeatureSupported, which checks the CWallet::nWalletVersion attribute.

Those functions can potentially be removed, but the following code snippets need to be addressed first:: https://github.com/bitcoin/bitcoin/blob/6a13a6106e3c1ebe95ba6430184d6260a7b942bd/src/wallet/rpc/wallet.cpp#L92-L94 and https://github.com/bitcoin/bitcoin/blob/6a13a6106e3c1ebe95ba6430184d6260a7b942bd/src/wallet/scriptpubkeyman.cpp#L630-L632

pablomartin4btc commented at 1:39 am on July 15, 2025:

Ok, you are right on the migratewallet process, I’ve missed that, perhaps we can’t go around that one (CanSupportFeature(FEATURE_HD_SPLIT)). The one in getwalletinfo, the condition itself can be removed, leaving the push of keypoolsize_hd_internal, as since legacy wallets can’t no longer be loaded, can’t run this command on them. But I doubt this is part of the scope of this PR, maybe can be done in a follow-up, like removing the rest of the functions (GetVersion(), GetClosestWalletFeature()), not sure about SetMinVersion() now because it’s the one set as default for any descriptor wallet that’s being created, unless we realise we don’t need the wallet version for descriptors anymore and we can remove it from the getwalletinfo RPC.

w0xlt commented at 8:31 am on July 15, 2025:

@Prabhat1308 @pablomartin4btc thanks for the suggestion. Done in #32977.

Prabhat1308 commented at 6:59 am on July 11, 2025: contributor

Concept ACK

w0xlt force-pushed on Jul 14, 2025

w0xlt commented at 6:05 am on July 14, 2025: contributor

Release note added

in doc/release-notes-32944.md:3 in 3f8cce456f outdated

0@@ -0,0 +1,3 @@
1+# RPC
2+
3+- The RPC `upgradewallet` has been removed.

pablomartin4btc commented at 1:40 pm on July 15, 2025:

nit: RPC is already in the title…

0`upgradewallet` has been removed. It was unused and only applied to unsupported legacy wallets.

w0xlt commented at 4:32 pm on July 15, 2025:

Done in https://github.com/bitcoin/bitcoin/pull/32944/commits/305884c859f717a85f5bf9e728c6382d929fc01b

w0xlt force-pushed on Jul 15, 2025

fanquake requested review from furszy on Jul 15, 2025

fanquake requested review from rkrux on Jul 15, 2025

w0xlt force-pushed on Jul 15, 2025

DrahtBot added the label CI failed on Jul 15, 2025

DrahtBot commented at 5:50 pm on July 15, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/46028240688 LLM reason (✨ experimental): The CI failure is due to a lint check failure caused by missing a required trailing newline in a documentation file.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

wallet: Remove `upgradewallet` RPC d89c6fa4a7

w0xlt force-pushed on Jul 15, 2025

in doc/release-notes-32944.md:3 in d89c6fa4a7

0@@ -0,0 +1,3 @@
1+# RPC
2+
3+`upgradewallet` has been removed. It was unused and only applied to unsupported legacy wallets.

pablomartin4btc commented at 7:20 pm on July 15, 2025:

nit (only if you have to re-touch): sorry, my bad, missed the “-” (bullet point) in my previous suggestion but I think that will be added during the release process if there are more updates on RPCs.

0- `upgradewallet` has been removed. It was unused and only applied to unsupported legacy wallets.

jonatack commented at 7:55 pm on July 25, 2025:

For users who don’t follow changes closely, perhaps mention when legacy wallets became no longer supported
Perhaps briefly remind users they can migrate legacy wallets to descriptor ones using RPC migratewallet (and/or link to the “Migrating Legacy Wallets to Descriptor Wallets” section in managing-wallets.md)

pablomartin4btc approved

pablomartin4btc commented at 7:21 pm on July 15, 2025: member

ACK d89c6fa4a71810cdb28395d4609632e1b22249b3

DrahtBot requested review from Prabhat1308 on Jul 15, 2025

DrahtBot removed the label CI failed on Jul 15, 2025

maflcko commented at 7:26 am on July 16, 2025: member

migrated wallets will take over the legacy bdb version, which I am not sure is meaningful or useful. The upgradewallet RPC can be used to bump it. I think it would be good to explain and document the expectations around this, and the interactions with this pull, going forward.

furszy commented at 2:49 pm on July 16, 2025: member

It seems we’re already thinking about upgrades in #32895 ?. If that’s the case, I’d rather keep the RPC. The maintenance cost should be minimal.

pablomartin4btc commented at 3:59 pm on July 16, 2025: member

migrated wallets will take over the legacy bdb version, which I am not sure is meaningful or useful. The upgradewallet RPC can be used to bump it.

As discussed offline with @achow101 a couple of days ago, my understanding was that both version field and ofc the upgradewallet RPC were exclusive to legacy wallets. edited: [ We’d need to keep the version on descriptors (making it deprecated at some point?) in case new created wallets get open with older versions of core. ] I think the new migrated wallet has the latest version of features (walletInstance->SetMinVersion(FEATURE_LATEST)) as descriptors support everything, so they don’t need to be bumped? Also since descriptors have the flags we don’t need features anymore.

It seems we’re already thinking about upgrades in #32895 ?. If that’s the case, I’d rather keep the RPC. The maintenance cost should be minimal.

Ok I saw that very quickly, I’ll move my attention to that one then and I’d say we keep this (and #32977 maybe?) PR (/s) as in draft?

maflcko commented at 4:36 pm on July 16, 2025: member

I think the new migrated wallet has the latest version of features (walletInstance->SetMinVersion(FEATURE_LATEST))

I don’t think this is true, at least locally:

 0> migratewallet ""
 1< {
 2  "wallet_name": "",
 3  "backup_path": "/tmp/regtest/default_wallet_1752683462.legacy.bak"
 4}
 5
 6> getwalletinfo
 7< {
 8  "walletname": "",
 9  "walletversion": 130000,
10  "format": "sqlite",
11  "txcount": 0,
12  "keypoolsize": 4000,
13  "paytxfee": 0.00000000,
14  "private_keys_enabled": true,
15  "avoid_reuse": false,
16  "scanning": false,
17  "descriptors": true,
18  "external_signer": false,
19  "blank": false,
20  "birthtime": 0,
21  "flags": [
22    "last_hardened_xpub_cached",
23    "descriptor_wallet"
24  ],
25  "lastprocessedblock": {
26    "hash": "0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206",
27    "height": 0
28  }
29}
30
31> upgradewallet
32< {
33  "wallet_name": "",
34  "previous_version": 130000,
35  "current_version": 169900,
36  "result": "Wallet upgraded successfully from version 130000 to version 169900."
37}
38
39> getwalletinfo
40< {
41  "walletname": "",
42  "walletversion": 169900,
43  "format": "sqlite",
44  "txcount": 0,
45  "keypoolsize": 4000,
46  "keypoolsize_hd_internal": 4000,
47  "paytxfee": 0.00000000,
48  "private_keys_enabled": true,
49  "avoid_reuse": false,
50  "scanning": false,
51  "descriptors": true,
52  "external_signer": false,
53  "blank": false,
54  "birthtime": 0,
55  "flags": [
56    "last_hardened_xpub_cached",
57    "descriptor_wallet"
58  ],
59  "lastprocessedblock": {
60    "hash": "0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206",
61    "height": 0
62  }
63}

So at least the RPC output is affected. I haven’t checked if other behavior is affected. This is why I am asking for the pull request to clarify it.

pablomartin4btc commented at 4:42 pm on July 16, 2025: member

Ok, thanks for testing it. In that case, that needs to be fixed, keeping the field or not, the returned wallet version for a descriptor should be the latest, unless I’m missing something…

w0xlt commented at 5:14 pm on July 16, 2025: contributor

If I understand correctly, the walletversion (CWallet::nWalletVersion) is not used by descriptor wallets (as can be checked in #32977), so it has no real effect on whether or not this field is updated after a migration.

Unless we intend to use this field in the future, I don’t see the point in keeping it or this RPC.

brunoerg approved

brunoerg commented at 7:52 pm on July 24, 2025: contributor

Concept & light cr ACK d89c6fa4a71810cdb28395d4609632e1b22249b3

maflcko commented at 8:20 am on July 25, 2025: member

review ACK d89c6fa4a71810cdb28395d4609632e1b22249b3 🤙

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK d89c6fa4a71810cdb28395d4609632e1b22249b3 🤙
3mkJJIWBHlPKtbyqO56RKP8l/5iuEzWDhcqa9NoO9xfaQ8jJYBJtHlUgwC91D+zwlz88DnIlP5fY1t3peKDdvBA==

fanquake removed review request from Prabhat1308 on Jul 25, 2025

fanquake requested review from achow101 on Jul 25, 2025

achow101 commented at 7:28 pm on July 25, 2025: member

ACK d89c6fa4a71810cdb28395d4609632e1b22249b3

DrahtBot requested review from Prabhat1308 on Jul 25, 2025

achow101 merged this on Jul 25, 2025

achow101 closed this on Jul 25, 2025

wallet: Remove upgradewallet RPC #32944

Code Coverage & Benchmarks

Reviews

Conflicts

wallet: Remove `upgradewallet` RPC #32944