wallet: Remove wallet version and several legacy related functions #32977

pull w0xlt wants to merge 7 commits into bitcoin:master from w0xlt:remove_nversion changing 15 files +15 −215
  1. w0xlt commented at 8:20 am on July 15, 2025: contributor

    This PR incorporates the suggestion provided by PRabahy and pablomartin4btc in #32944 of removing CWallet::nWalletVersion and several related functions, such as SetMinVersion(), GetVersion(), GetClosestWalletFeature(), IsFeatureSupported(), CanSupportFeature(), etc …

    This field is no longer used in the descriptor wallet and there is still a lot of code related to it, so the changes here provide a good cleanup in the wallet code.

    Built on top of https://github.com/bitcoin/bitcoin/pull/32944

  2. wallet: Remove `upgradewallet` RPC 3f8cce456f
  3. DrahtBot added the label Wallet on Jul 15, 2025
  4. DrahtBot commented at 8:20 am on July 15, 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/32977.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK maflcko
    Concept ACK pablomartin4btc

    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:

    • #33043 ([POC] wallet: Enable non-electronic (paper-based) wallet backup with codex32 by w0xlt)
    • #33034 (wallet: Store transactions in a separate sqlite table by achow101)
    • #32966 (Silent Payments: Receiving by Eunovo)
    • #32944 (wallet: Remove upgradewallet RPC by w0xlt)
    • #32895 (wallet: Prepare for future upgrades by recording versions of last client to open and decrypt by achow101)
    • #32636 (Split CWallet::Create() into CreateNew and LoadExisting by davidgumberg)
    • #32603 (rpc, doc: clarify wallet version in getwalletinfo help by rkrux)
    • #27865 (wallet: Track no-longer-spendable TXOs separately 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.

  5. fanquake marked this as a draft on Jul 15, 2025
  6. in src/wallet/rpc/wallet.cpp:43 in 395433d5df outdated 
    39@@ -40,7 +40,6 @@ static RPCHelpMan getwalletinfo()
40                     {
41                         {
42                         {RPCResult::Type::STR, "walletname", "the wallet name"},
43-                        {RPCResult::Type::NUM, "walletversion", "the wallet version"},
    pablomartin4btc commented at 2:11 pm on July 15, 2025:
    I’d keep it for backwards compatibility. {RPCResult::Type::NUM, "walletversion", "(DEPRECATED) only related to unsupported legacy wallet, returns the latest version 169900 for backwards compatibility"},
  7. in src/wallet/rpc/wallet.cpp:87 in 395433d5df outdated 
    82@@ -84,14 +83,11 @@ static RPCHelpMan getwalletinfo()
83 
84     size_t kpExternalSize = pwallet->KeypoolCountExternalKeys();
85     obj.pushKV("walletname", pwallet->GetName());
86-    obj.pushKV("walletversion", pwallet->GetVersion());
    pablomartin4btc commented at 2:11 pm on July 15, 2025:

    Same as above, please keep it for backwards compatibility:

    obj.pushKV(“walletversion”, 169900);

    (or a const somewhere if we need that number for a little while)

  8. in src/wallet/scriptpubkeyman.cpp:629 in 395433d5df outdated 
    628+    int wallet_version = 0;
629+    auto x = m_storage.GetDatabase().MakeBatch();
630+    x->Read(DBKeys::MINVERSION, wallet_version);
631+
632+    bool can_support_hd_split_feature = wallet_version >= FEATURE_HD_SPLIT;
633+
    pablomartin4btc commented at 2:11 pm on July 15, 2025:

    I had a chat with @achow101 about it yesterday, she commented to me that the “hd chain split versioning is stored in CHDChain too” so we can get rid of FEATURE_HD_SPLIT and the the entire enum there.

  9. in src/wallet/walletutil.h:17 in 395433d5df outdated 
    19-    FEATURE_WALLETCRYPT = 40000, // wallet encryption
20-    FEATURE_COMPRPUBKEY = 60000, // compressed public keys
21-
22-    FEATURE_HD = 130000, // Hierarchical key derivation after BIP32 (HD Wallet)
23-
24     FEATURE_HD_SPLIT = 139900, // Wallet with HD chain split (change outputs will use m/0'/1'/k)
    pablomartin4btc commented at 2:12 pm on July 15, 2025:
    This can be also removed, as mentioned above in LegacyDataSPKM::MigrateToDescriptor(), the hd split versioning can be obtained from the CHDCHAIN.
  10. in test/functional/wallet_createwallet.py:176 in 395433d5df outdated 
    169@@ -170,11 +170,10 @@ def run_test(self):
170         with node.assert_debug_log(expected_msgs=[], unexpected_msgs=["Last client version = ", "Wallet file version = "]):
171             node.createwallet("version_check")
172         wallet = node.get_wallet_rpc("version_check")
173-        wallet_version = wallet.getwalletinfo()["walletversion"]
174         client_version = node.getnetworkinfo()["version"]
175         wallet.unloadwallet()
176         with node.assert_debug_log(
177-            expected_msgs=[f"Last client version = {client_version}", f"Wallet file version = {wallet_version}"],
178+            expected_msgs=[f"Last client version = {client_version}"],
    pablomartin4btc commented at 2:13 pm on July 15, 2025:
    Same as above, we can keep it for another release until it gets removed completely.
    w0xlt commented at 5:44 pm on July 15, 2025:
    In this case, it needs to be removed. Since the wallet version is no longer loaded from the database file, this record no longer appears in the log.
  11. pablomartin4btc commented at 2:13 pm on July 15, 2025: member

    Concept ACK

    I’d clarify in the description that this PR is based on top of #32944.

    The title of this PR could be: “wallet: Remove wallet version and several legacy related functions.

    I’ve left a few comments.

  12. wallet: Remove `GetVersion()` fc4d563edb
  13. wallet: Remove `GetClosestWalletFeature()` 6ea94c30c7
  14. w0xlt force-pushed on Jul 15, 2025
  15. wallet: `MigrateToDescriptor` no longer calls `CanSupportFeature` 269c43bdb0
  16. wallet: Remove `IsFeatureSupported()` and `CanSupportFeature()` f5f643a7cd
  17. w0xlt force-pushed on Jul 15, 2025
  18. DrahtBot added the label CI failed on Jul 15, 2025
  19. DrahtBot commented at 5:32 pm on July 15, 2025: contributor

    🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/46032167315 LLM reason (✨ experimental): The CI failure is caused by lint errors, including an unused variable warning and trailing whitespace issues, which caused the lint check to fail.

    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.

  20. wallet: Remove `CWallet::nWalletVersion` and related functions dbf8dbf4b1
  21. wallet: Remove unused `WalletFeature` enums 8462d84c59
  22. w0xlt force-pushed on Jul 15, 2025
  23. w0xlt renamed this:
    wallet: Remove `CWallet::nWalletVersion` and several related functions
    wallet: Remove wallet version and several legacy related functions
    on Jul 15, 2025
  24. w0xlt commented at 5:46 pm on July 15, 2025: contributor
    @pablomartin4btc thanks for the detailed review. I’ve addressed all your comments.
  25. in src/wallet/wallet.cpp:2870 in dbf8dbf4b1 outdated 
    2845@@ -2866,9 +2846,6 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
2846     {
2847         LOCK(walletInstance->cs_wallet);
2848 
2849-        // ensure this wallet.dat can only be opened by clients supporting HD with chain split and expects no default key
2850-        walletInstance->SetMinVersion(FEATURE_LATEST);
    achow101 commented at 6:36 pm on July 15, 2025:

    In dbf8dbf4b1792686be70050c3b19e477769d142b “wallet: Remove CWallet::nWalletVersion and related functions”

    We still need to write the minversion record for newly created wallets so that they can be opened in older versions with the expected feature set.

    pablomartin4btc commented at 7:31 pm on July 15, 2025:
    Ok, perhaps we can rename the function to SetInitialVersion, SetCompatibleVersion or similar with a comment/ note about the intention. Is this something that needs to be kept forever or some “deprecation” case?
    w0xlt commented at 8:20 pm on July 15, 2025:
    I had changed it to WriteLatestLegacyWalletVersion / SetLatestLegacyWalletVersion before seeing your comment. If that doesn’t work, we can change it to something else.
    maflcko commented at 8:14 am on July 24, 2025:

    We still need to write the minversion record for newly created wallets so that they can be opened in older versions with the expected feature set.

    I think I may be missing something, but I don’t understand what feature set this is referring to. CanSupportFeature() is only called on legacy BDB wallets, but they can’t open sqlite descriptor wallets anyway, so retaining the code here for compatibility does not seem needed.

    It is fine to keep it for consistency or for documentation purposes, but I was wondering if there is a real end-user visible feature difference.

    I am asking, because if this is something to support, it may be good to write tests for it.

    The only thing I could find is a minor difference in the RPC output:

    0    if (pwallet->CanSupportFeature(FEATURE_HD_SPLIT)) {
1        obj.pushKV("keypoolsize_hd_internal",   (int64_t)(pwallet->GetKeyPoolSize() - kpExternalSize));
2    }

    However, it probably is not worth to write a test for that?

    achow101 commented at 5:40 pm on July 24, 2025:
    Hmm, yeah, I guess we don’t really need it.
    w0xlt commented at 5:55 pm on July 24, 2025:
    Reverted the PR to the previous version
  26. w0xlt force-pushed on Jul 15, 2025
  27. DrahtBot removed the label CI failed on Jul 15, 2025
  28. w0xlt force-pushed on Jul 24, 2025
  29. in src/wallet/rpc/wallet.cpp:87 in fc4d563edb outdated 
    81@@ -82,9 +82,11 @@ static RPCHelpMan getwalletinfo()
82 
83     UniValue obj(UniValue::VOBJ);
84 
85+    const int latest_legacy_wallet_version = 169900;
86+
87     size_t kpExternalSize = pwallet->KeypoolCountExternalKeys();
88     obj.pushKV("walletname", pwallet->GetName());
89-    obj.pushKV("walletversion", pwallet->GetVersion());
    maflcko commented at 8:38 am on July 25, 2025:
    fc4d563edb5feab273e5770f1b369cb6fe8a0c8c: Returning a constant seems confusing in the context of #32895. Edit: Actually, this is fine, because GetVersion does not return the version, but the minversion.
  30. in src/wallet/rpc/wallet.cpp:85 in fc4d563edb outdated 
    81@@ -82,9 +82,11 @@ static RPCHelpMan getwalletinfo()
82 
83     UniValue obj(UniValue::VOBJ);
84 
85+    const int latest_legacy_wallet_version = 169900;
    maflcko commented at 9:36 am on July 25, 2025:

    nit in fc4d563edb5feab273e5770f1b369cb6fe8a0c8c: This is the minversion, so could write:

    0    const int latest_legacy_wallet_minversion{169900};
  31. in src/wallet/scriptpubkeyman.cpp:628 in 269c43bdb0 outdated 
    623@@ -624,10 +624,13 @@ std::optional<MigrationData> LegacyDataSPKM::MigrateToDescriptor()
624     for (const auto& chain_pair : m_inactive_hd_chains) {
625         chains.push_back(chain_pair.second);
626     }
627+
628+    bool can_support_hd_split_feature = m_hd_chain.nVersion >= CHDChain::VERSION_HD_CHAIN_SPLIT;
    maflcko commented at 9:52 am on July 25, 2025:
    nit in 269c43bdb04768954e59119363afdfa061849ed0: I haven’t reviewed this, and it could make sense to add a test for this.
  32. in src/wallet/rpc/wallet.cpp:93 in f5f643a7cd outdated 
    89@@ -90,10 +90,8 @@ static RPCHelpMan getwalletinfo()
90     obj.pushKV("format", pwallet->GetDatabase().Format());
91     obj.pushKV("txcount",       (int)pwallet->mapWallet.size());
92     obj.pushKV("keypoolsize", (int64_t)kpExternalSize);
93+    obj.pushKV("keypoolsize_hd_internal",   (int64_t)(pwallet->GetKeyPoolSize() - kpExternalSize));
    maflcko commented at 9:55 am on July 25, 2025:
    nit in f5f643a7cd5ab251d2491661318553fd7463acaf: No need to cast the value?
  33. in src/wallet/walletutil.h:33 in f5f643a7cd outdated 
    29@@ -30,8 +30,6 @@ enum WalletFeature
30     FEATURE_LATEST = FEATURE_PRE_SPLIT_KEYPOOL
31 };
32 
33-bool IsFeatureSupported(int wallet_version, int feature_version);
    maflcko commented at 9:55 am on July 25, 2025:
    nit in f5f643a7cd5ab251d2491661318553fd7463acaf: Forgot to remove the function in the cpp file?
  34. in test/functional/wallet_createwallet.py:177 in dbf8dbf4b1 outdated 
    176         wallet.unloadwallet()
177         with node.assert_debug_log(
178-            expected_msgs=[f"Last client version = {client_version}", f"Wallet file version = {wallet_version}"],
179-            unexpected_msgs=["Wallet file version = 10500"]
180+            expected_msgs=[f"Last client version = {client_version}"],
181+            unexpected_msgs=[]
    maflcko commented at 10:00 am on July 25, 2025:
    nit in dbf8dbf4b1792686be70050c3b19e477769d142b: Why pass an empty array?
  35. maflcko commented at 10:03 am on July 25, 2025: member

    review ACK 8462d84c596315d1979d26b964657079ed5bc09b 🥙

    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 8462d84c596315d1979d26b964657079ed5bc09b 🥙
3lRw+32iOuclYdNxGCTx59/jbXpsc/NOjYUarihSoqmRF9rPfCvI4ESSmYKUZK/mmNY5Dzi+5izmKXmrVKT9eCw==
  36. DrahtBot requested review from pablomartin4btc on Jul 25, 2025


w0xlt DrahtBot pablomartin4btc achow101 maflcko


pablomartin4btc

Labels
Wallet

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: 2025-07-25 18:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me