wallet: Set minversion to FEATURE_LATEST during migration #33041

1. 
   achow101 commented at 11:28 pm on July 22, 2025: member

   Although the minversion is not used by descriptor wallets, we should still update it to the latest version for consistency.

   No test as any test requires very old versions of Bitcoin Core.

2. DrahtBot added the label Wallet on Jul 22, 2025
3. 
   DrahtBot commented at 11:29 pm on July 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/33041.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	pablomartin4btc, maflcko, w0xlt

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

   LLM Linter (✨ experimental)

   Possible typos and grammar issues:

   • typo: “Setup new descriptors” -> “Set up new descriptors” [‘Setup’ is a noun; the verb phrase here should be “set up”]

   drahtbot_id_4_m

4. wallet: Set minversion to FEATURE_LATEST during migration 5aa758861c
5. achow101 force-pushed on Jul 23, 2025
6. DrahtBot added the label CI failed on Jul 23, 2025
7. 
   DrahtBot commented at 0:27 am on July 23, 2025: contributor

   🚧 At least one of the CI tasks failed. Task MSan, depends: https://github.com/bitcoin/bitcoin/runs/46518628354 LLM reason (✨ experimental): The CI failure is caused by a timeout in the bench_sanity_check test.

   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.

8. 
   pablomartin4btc commented at 0:52 am on July 23, 2025: member

   ACK 5aa758861cf1399842fd0bea3a42d5b0cafcb0f6

   I’ve noticed this while reviewing the removal of upgradewallet RPC PR.

   Please consider also the version related functions removal PR, if you haven’t already, shouldn’t SetMinVersion renamed? And perhaps change the first arg to a const so we can remove the features enum?

9. in src/wallet/wallet.cpp:3882 in 5aa758861c

   3877@@ -3878,6 +3878,9 @@ util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch,
   3878     m_external_spk_managers.clear();
   3879     m_internal_spk_managers.clear();
   3880 
   3881+    // Set minversion to FEATURE_LATEST
   3882+    SetMinVersion(FEATURE_LATEST, &local_wallet_batch);
   

   ---

   ---

   

   pablomartin4btc commented at 1:25 am on July 23, 2025:

   How this is related with the SetMinVersion(FEATURE_LATEST call during the wallet creation in the migration process?

   https://github.com/bitcoin/bitcoin/blob/900bb53905aaf0a7b45c1471c5248d7e340ba27b/src/wallet/wallet.cpp#L2861-L2870

   Shouldn’t that instance of the call be removed as the one you are adding here is more generic?

   ---

   

   achow101 commented at 2:02 am on July 23, 2025:

   That is only called on actually newly created wallets. A migrated wallet is not newly created and we don’t use Create for the migrated wallet. It’s only used for the watchonly and solvables wallets.
10. DrahtBot removed the label CI failed on Jul 23, 2025
11. 
    maflcko commented at 7:18 am on July 23, 2025: member

    No test as any test requires very old versions of Bitcoin Core.

    It should be possible to test this, because only a wallet needs to be generated. I am happy to write one as a follow-up.

    lgtm ACK 5aa758861cf1399842fd0bea3a42d5b0cafcb0f6

12. 
    maflcko commented at 7:26 am on July 23, 2025: member

    Actually, on a second thought, I think it is cleaner to go with just #32977, because the only stated reason for this pull request is “consistency”, but I think this ship has sailed anyway, because plenty sqlite wallets exists with a different minversion. They’ll have to be dealt with anyway and keeping the migrate logic as-is documents that better. But no strong opinion, I’d say anything is fine here.
13. 
    Prabhat1308 commented at 8:33 pm on July 23, 2025: contributor

    Agree with the comment by @pablomartin4btc here that if we want to go the route of keeping the wallet Version , its better to set it as a constant since FEATURE_LATEST is the only version being maintained in the practical sense and rename the function along the lines of SetVersion . Then we can even go ahead with the modified #32977 cleanup .
14. 
    w0xlt commented at 8:54 pm on July 23, 2025: contributor

    ACK https://github.com/bitcoin/bitcoin/pull/33041/commits/5aa758861cf1399842fd0bea3a42d5b0cafcb0f6
15. achow101 closed this on Jul 24, 2025

    ---

Contributors
achow101 DrahtBot pablomartin4btc maflcko Prabhat1308 w0xlt

Labels
Wallet