wallet: Give deprecation warning when loading a legacy wallet #27869

pull achow101 wants to merge 1 commits into bitcoin:master from achow101:deprecation-warning-legacy-wallet changing 2 files +8 −2

achow101 commented at 10:15 pm on June 12, 2023: member

Next step in legacy wallet deprecation.

DrahtBot commented at 10:15 pm on June 12, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	jonatack, S3RK
Concept ACK	fanquake, glozow, darosior
Stale ACK	dimitaracev

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:

#28027 (test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets 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.

DrahtBot added the label Wallet on Jun 12, 2023
dimitaracev commented at 7:40 am on June 13, 2023: contributor

ACK 956d05b
fanquake commented at 11:03 am on June 13, 2023: member

Concept ACK
glozow commented at 1:44 pm on June 13, 2023: member

Concept ACK

in src/wallet/wallet.cpp:250 in 956d05bad2 outdated

244@@ -245,6 +245,11 @@ std::shared_ptr<CWallet> LoadWalletInternal(WalletContext& context, const std::s
245             return nullptr;
246         }
247 
248+        // Legacy wallets are being deprecated, warn if the loaded wallet is legacy
249+        if (!wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
250+            warnings.push_back(_("Wallet loaded successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future."));

darosior commented at 2:15 pm on June 13, 2023:

Maybe hint at how to migrate?

achow101 commented at 4:15 pm on June 13, 2023:

Added a hint.

darosior commented at 2:16 pm on June 13, 2023: member

Concept ACK. How about also logging for wallets that are loaded on startup?
achow101 force-pushed on Jun 13, 2023
DrahtBot added the label CI failed on Jun 13, 2023
S3RK commented at 6:22 am on June 14, 2023: contributor

ACK 2bf7183ea9b42a9edf76bccdf00708341a1d2c0a
in test/functional/wallet_backwards_compatibility.py:267 in 2bf7183ea9 outdated
266@@ -267,7 +267,8 @@ def run_test(self): 267 # Make sure this wallet opens without warnings. See https://github.com/bitcoin/bitcoin/pull/19054
jonatack commented at 3:11 pm on June 14, 2023:

It looks like this line could be updated or removed.

achow101 commented at 8:37 pm on June 23, 2023:

updated

jonatack commented at 3:21 pm on June 14, 2023: member

ACK 2bf7183ea9b42a9edf76bccdf00708341a1d2c0a

Indeed, there is no debug log warning yet as suggested in #27869#pullrequestreview-1477293036.

Sanity check:

 0$ ./src/bitcoin-cli -signet listwallets                 
 1[
 2  "sqlite3",
 3  "bdb"
 4]
 5$ ./src/bitcoin-cli -signet unloadwallet bdb            
 6{
 7}
 8$ ./src/bitcoin-cli -signet loadwallet bdb            
 9{
10  "name": "bdb",
11  "warnings": [
12    "Wallet loaded successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future. Legacy wallets can be migrated to a descriptor wallet with migratewallet."
13  ]
14}

wallet: Give deprecation warning when loading a legacy wallet 8fbb6e99bf
achow101 force-pushed on Jun 23, 2023
DrahtBot removed the label CI failed on Jun 28, 2023
fanquake added this to the milestone 26.0 on Jun 30, 2023
achow101 requested review from S3RK on Jul 5, 2023
achow101 requested review from jonatack on Jul 5, 2023
achow101 requested review from darosior on Jul 5, 2023
jonatack commented at 4:43 pm on July 5, 2023: member

re-ACK 8fbb6e99bfc85a1b9003cae402a7335843a86abd

modulo #27869#pullrequestreview-1477293036

Tested on the command line and with the GUI

in src/wallet/wallet.cpp:250 in 8fbb6e99bf

244@@ -245,6 +245,11 @@ std::shared_ptr<CWallet> LoadWalletInternal(WalletContext& context, const std::s
245             return nullptr;
246         }
247 
248+        // Legacy wallets are being deprecated, warn if the loaded wallet is legacy
249+        if (!wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
250+            warnings.push_back(_("Wallet loaded successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future. Legacy wallets can be migrated to a descriptor wallet with migratewallet."));

jonatack commented at 4:49 pm on July 5, 2023:

nit, the similar warning at the end of CreateWallet() could be updated to use this expanded one

S3RK commented at 7:03 am on July 6, 2023: contributor

reACK 8fbb6e99bfc85a1b9003cae402a7335843a86abd
DrahtBot removed review request from S3RK on Jul 6, 2023
glozow merged this on Jul 6, 2023
glozow closed this on Jul 6, 2023
sidhujag referenced this in commit 1c9cc3f201 on Jul 6, 2023
bitcoin locked this on Dec 5, 2024

Contributors
achow101 DrahtBot dimitaracev fanquake glozow darosior S3RK jonatack

Review Requested
darosior

Labels
Wallet

Milestone
26.0