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

pull w0xlt wants to merge 6 commits into bitcoin:master from w0xlt:remove_nversion changing 12 files +11 −108
  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. DrahtBot added the label Wallet on Jul 15, 2025
  3. 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, pablomartin4btc, achow101

    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)
    • #32895 (wallet: Prepare for future upgrades by recording versions of last client to open and decrypt by achow101)
    • #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.

  4. fanquake marked this as a draft on Jul 15, 2025
  5. 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"},
  6. 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)

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

  8. 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.
  9. 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.
  10. 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.

  11. w0xlt force-pushed on Jul 15, 2025
  12. w0xlt force-pushed on Jul 15, 2025
  13. DrahtBot added the label CI failed on Jul 15, 2025
  14. 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.

  15. w0xlt force-pushed on Jul 15, 2025
  16. w0xlt renamed this:
    wallet: Remove `CWallet::nWalletVersion` and several related functions
    wallet: Remove wallet version and several legacy related functions
    on Jul 15, 2025
  17. w0xlt commented at 5:46 pm on July 15, 2025: contributor
    @pablomartin4btc thanks for the detailed review. I’ve addressed all your comments.
  18. in src/wallet/wallet.cpp:2875 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
  19. w0xlt force-pushed on Jul 15, 2025
  20. DrahtBot removed the label CI failed on Jul 15, 2025
  21. w0xlt force-pushed on Jul 24, 2025
  22. 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.
  23. 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};
    w0xlt commented at 9:46 pm on July 25, 2025:
    Done. Thanks.
  24. 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.
    w0xlt commented at 9:50 pm on July 25, 2025:
    I will look into this.
  25. 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?
    w0xlt commented at 9:46 pm on July 25, 2025:
    Done. Thanks.
  26. 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?
    w0xlt commented at 9:50 pm on July 25, 2025:
    Done. Thanks.
  27. 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?
    w0xlt commented at 9:50 pm on July 25, 2025:
    Done. Thanks.
  28. 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==
  29. DrahtBot requested review from pablomartin4btc on Jul 25, 2025
  30. w0xlt force-pushed on Jul 25, 2025
  31. wallet: Remove `GetVersion()` e27da3150b
  32. wallet: Remove `GetClosestWalletFeature()` 63acee2797
  33. wallet: `MigrateToDescriptor` no longer calls `CanSupportFeature` ba01585229
  34. w0xlt force-pushed on Jul 25, 2025
  35. wallet: Remove `IsFeatureSupported()` and `CanSupportFeature()` 7cda3d0f5b
  36. w0xlt force-pushed on Jul 25, 2025
  37. wallet: Remove `CWallet::nWalletVersion` and related functions 66de58208a
  38. wallet: Remove unused `WalletFeature` enums 60d1042b9a
  39. w0xlt force-pushed on Jul 25, 2025
  40. w0xlt commented at 9:53 pm on July 25, 2025: contributor
    Rebased. Ready for reviews.
  41. maflcko commented at 10:39 am on July 28, 2025: member

    lgtm, but I haven’t reviewed the hd-split refactor. I’d say it would be good to add a test for this, using a previous release to create an ancient wallet, and then checking that the migration works as expected?

    review ACK 60d1042b9a4db8daf9fffdc29053652e99b7126e 🐾

    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 60d1042b9a4db8daf9fffdc29053652e99b7126e 🐾
3BpiTgZvW7U15rsCHuiBRDBIwD0rs/b7vScLRfC2s1IWNtrwW3cJXMZypARYkTwzRNhBXI5/kGmwWHkrYhA8BCg==
  42. fanquake marked this as ready for review on Aug 1, 2025
  43. pablomartin4btc commented at 1:31 am on August 11, 2025: member

    ACK 60d1042b9a4db8daf9fffdc29053652e99b7126e

    I’ll review it again once the split HD support test and the ancient wallet migration version check (perhaps could be either in wallet_migration or wallet_backwards_compatibility), both suggested by @maflcko, are added.

  44. w0xlt commented at 6:55 pm on August 13, 2025: contributor

    From what I understand, you’re requesting tests for the non-HD wallet and the single-chain HD wallet (VERSION_HD_BASE) because, as far as I know, the dual-chain wallet migration (VERSION_HD_CHAIN_SPLIT) has already been tested in wallet_backwards_compatibility.py.

    If that’s the case, I’ve added those tests in #33186.

  45. achow101 commented at 11:28 pm on August 15, 2025: member
    ACK 60d1042b9a4db8daf9fffdc29053652e99b7126e
  46. achow101 merged this on Aug 15, 2025
  47. achow101 closed this on Aug 15, 2025


w0xlt DrahtBot pablomartin4btc achow101 maflcko

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