wallet: Set minversion to FEATURE_LATEST during migration #33041

pull achow101 wants to merge 1 commits into bitcoin:master from achow101:migrate-set-latest-minversion changing 1 files +3 −0

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.
DrahtBot added the label Wallet on Jul 22, 2025
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
wallet: Set minversion to FEATURE_LATEST during migration 5aa758861c
achow101 force-pushed on Jul 23, 2025
DrahtBot added the label CI failed on Jul 23, 2025
DrahtBot commented at 12: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.

<details><summary>Hints</summary>

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.

</details>
pablomartin4btc commented at 12: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?
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.
DrahtBot removed the label CI failed on Jul 23, 2025
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
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.
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 .
w0xlt commented at 8:54 PM on July 23, 2025: contributor

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