wallet: Add a deprecation warning for newly created legacy wallets

achow101 commented at 4:53 pm on March 8, 2022: member

As we slowly deprecate legacy wallets, we need to warn users that are making new legacy wallets that their wallet type is going to be unsupported in the future.

fanquake added the label Wallet on Mar 8, 2022

theStack commented at 5:11 pm on March 8, 2022: member

Concept ACK

fanquake added this to the milestone 24.0 on Mar 8, 2022

gruve-p commented at 6:28 pm on March 8, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/24505/commits/6c629fe292bbb14d65455158fa38e869bdb17921

luke-jr commented at 6:40 pm on March 8, 2022: member

Concept NACK on removal. Users shouldn’t be forced to upgrade their wallets (especially not on such a short timeline).

promag commented at 11:35 pm on March 8, 2022: member

Concept ACK @luke-jr this just adds the warning when a wallet is created, nothing is removed nor an upgrade is enforced. When do you think the warning is okay?

luke-jr commented at 11:50 pm on March 8, 2022: member

The warning text says it will be removed. And if the user has to explicitly choose to create it, I don’t see why a warning is really needed at all.

laanwj commented at 10:25 am on March 9, 2022: member

Concept ACK on discouraging creating new legacy wallets. Agree that the code to support them shouldn’t be removed any time soon, but that kind of policy discussion is out of scope here.

S3RK commented at 7:05 am on March 10, 2022: member

ACK 6c629fe292bbb14d65455158fa38e869bdb17921

MarcoFalke commented at 8:52 am on March 10, 2022: member

Wouldn’t it be better to update the RPC doc instead? This way the user will see the warning before creating the wallet.

0            {"descriptors", RPCArg::Type::BOOL, RPCArg::Default{true}, "Create a native descriptor wallet. The wallet will use descriptors internally to handle address creation"},

in src/wallet/wallet.cpp:280 in 6c629fe292 outdated

274@@ -275,6 +275,11 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string&
275         wallet_creation_flags |= WALLET_FLAG_BLANK_WALLET;
276     }
277 
278+    // Legacy wallets are being deprecated, warn if a newly created wallet is legacy
279+    if (!(wallet_creation_flags & WALLET_FLAG_DESCRIPTORS)) {
280+        warnings.push_back(_("The Legacy wallet type is being deprecated. Support for opening Legacy wallets will be removed in the future"));

jonatack commented at 10:14 am on March 10, 2022:

s/opening/creating/
s/Legacy/legacy/
be unambiguous that the wallet was still created

suggestion:

0        warnings.push_back(_("Wallet successfully created, but be aware that the legacy wallet type is expected to be deprecated, and support for creating legacy wallets may be removed in the future"));

achow101 commented at 10:52 am on March 10, 2022:

Added Wallet created successfully and lowercase legacy. However there is no “expected to be” or “may” about the removal of the legacy wallet. It will be removed in the future, whether that’s in a few years, or in fifty. Additionally, this warning is to inform users that their newly created legacy wallet will have issues with being opened in the future. It’s already created, so warning that creation will be removed is useless. opening is the correct word to use.

jonatack commented at 11:56 am on March 10, 2022:

Thanks!

jonatack commented at 10:19 am on March 10, 2022: member

I agree with @MarcoFalke that the RPC help would need to be updated, either first or at the same time, per the principle of least surprise to the user.

achow101 force-pushed on Mar 10, 2022

achow101 commented at 10:49 am on March 10, 2022: member

Added a warning to the RPC help.

in src/wallet/rpc/wallet.cpp:319 in 8118304df1 outdated

314@@ -315,7 +315,9 @@ static RPCHelpMan createwallet()
315             {"blank", RPCArg::Type::BOOL, RPCArg::Default{false}, "Create a blank wallet. A blank wallet has no keys or HD seed. One can be set using sethdseed."},
316             {"passphrase", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "Encrypt the wallet with this passphrase."},
317             {"avoid_reuse", RPCArg::Type::BOOL, RPCArg::Default{false}, "Keep track of coin reuse, and treat dirty and clean coins differently with privacy considerations in mind."},
318-            {"descriptors", RPCArg::Type::BOOL, RPCArg::Default{true}, "Create a native descriptor wallet. The wallet will use descriptors internally to handle address creation"},
319+            {"descriptors", RPCArg::Type::BOOL, RPCArg::Default{true}, "Create a native descriptor wallet. The wallet will use descriptors internally to handle address creation."
320+                                                                       "Setting to 'false' will create a legacy wallet, however the legacy wallet type is being deprecated and"

jonatack commented at 12:01 pm on March 10, 2022:

missing spaces

06. descriptors             (boolean, optional, default=true) Create a native descriptor wallet. The wallet will use descriptors internally to handle address creation.Setting to 'false' will create a legacy wallet, however the legacy wallet type is being deprecated andsupport for creating and opening legacy wallets will be removed in the future.

suggested diff with added spaces and minor grammar fixups

0                                                                       " Setting to \"false\" will create a legacy wallet; however, the legacy wallet type is being deprecated and "

achow101 commented at 12:33 pm on March 10, 2022:

Done

in src/wallet/wallet.cpp:359 in 8118304df1 outdated

353@@ -354,6 +354,11 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string&
354     // Write the wallet settings
355     UpdateWalletSetting(*context.chain, name, load_on_start, warnings);
356 
357+    // Legacy wallets are being deprecated, warn if a newly created wallet is legacy
358+    if (!(wallet_creation_flags & WALLET_FLAG_DESCRIPTORS)) {
359+        warnings.push_back(_("Wallet created successfully. The legacy wallet type is being deprecated and support for opening legacy wallets will be removed in the future"));

jonatack commented at 12:03 pm on March 10, 2022:

maybe both creating and opening like in the help (and add a period to the second sentence)

0        warnings.push_back(_("Wallet created successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future."));

achow101 commented at 12:33 pm on March 10, 2022:

Done

wallet: Add a deprecation warning for newly created legacy wallets 61152183ab

achow101 force-pushed on Mar 10, 2022

jonatack commented at 1:04 pm on March 10, 2022: member

ACK 61152183ab18960c8b42cf22ff7168762946678e

Tested the help and the test with logging the result

0₿ test/functional/wallet_createwallet.py
1...
22022-03-10T13:00:33.383000Z TestFramework (INFO): Test legacy wallet deprecation
32022-03-10T13:00:33.485000Z TestFramework (INFO): {'name': 'legacy_w0', 'warning': 'Wallet created successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future.'}

0₿ ./src/bitcoin-cli help createwallet
1
26. descriptors             (boolean, optional, default=true) Create a native descriptor wallet. The wallet will use descriptors internally to handle address creation. Setting to "false" will create a legacy wallet; however, the legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future.

theStack approved

theStack commented at 3:42 pm on March 13, 2022: member

ACK 61152183ab18960c8b42cf22ff7168762946678e

 0$ ./src/bitcoin-cli -regtest -named createwallet wallet_name=wallet_native descriptors=false
 1{
 2  "name": "wallet_native",
 3  "warning": "Wallet created successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future."
 4}
 5$ ./src/bitcoin-cli -regtest -named createwallet wallet_name=wallet_desc descriptors=true
 6{
 7  "name": "wallet_desc",
 8  "warning": ""
 9}
10$ ./src/bitcoin-cli -regtest help createwallet | grep "6. descriptors"
116. descriptors             (boolean, optional, default=true) Create a native descriptor wallet. The wallet will use descriptors internally to handle address creation. Setting to "false" will create a legacy wallet; however, the legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future.

S3RK commented at 7:17 am on March 14, 2022: member

reACK 61152183ab18960c8b42cf22ff7168762946678e

MarcoFalke merged this on Mar 14, 2022

MarcoFalke closed this on Mar 14, 2022

sidhujag referenced this in commit b7932ffd2f on Mar 14, 2022

bitcoinhodler referenced this in commit 1e618a228c on Oct 24, 2022

bitcoinhodler referenced this in commit 593a7b3de2 on Oct 25, 2022

bitcoinhodler referenced this in commit 77a80f6203 on Oct 26, 2022

bitcoinhodler referenced this in commit a0745bcd82 on Oct 26, 2022

bitcoinhodler referenced this in commit 5406c9f60b on Oct 27, 2022

DrahtBot locked this on Mar 14, 2023

wallet: Add a deprecation warning for newly created legacy wallets #24505