wallet: Migrate legacy wallets to descriptor wallets

achow101 commented at 8:22 pm on July 27, 2020: member

This PR adds a new migratewallet RPC which migrates a legacy wallet to a descriptor wallet. Migrated wallets will need a new backup. If a wallet has watchonly stuff in it, a new watchonly descriptor wallet will be created containing those watchonly things. The related transactions, labels, and descriptors for those watchonly things will be removed from the original wallet. Migrated wallets will not have any of the legacy things be available for fetching from getnewaddress or getrawchangeaddress. Wallets that have private keys enabled will have newly generated descriptors. Wallets with private keys disabled will not have any active ScriptPubKeyMans.

For the basic HD wallet case of just generated keys, in addition to the standard descriptor wallet descriptors using the master key derived from the pre-existing hd seed, the migration will also create 3 descriptors for each HD chain in: a ranged combo external, a ranged combo internal, and a single key combo for the seed (the seed is a valid key that we can receive coins at!). The migrated wallet will then have newly generated descriptors as the active ScriptPubKeyMans. This is equivalent to creating a new descriptor wallet and importing the 3 descriptors for each HD chain. For wallets containing non-HD keys, each key will have its own combo descriptor.

There are also tests.

DrahtBot added the label Build system on Jul 27, 2020

DrahtBot added the label Mempool on Jul 27, 2020

DrahtBot added the label RPC/REST/ZMQ on Jul 27, 2020

DrahtBot added the label Tests on Jul 27, 2020

DrahtBot added the label UTXO Db and Indexes on Jul 27, 2020

DrahtBot added the label Wallet on Jul 27, 2020

DrahtBot commented at 11:26 pm on July 27, 2020: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25957 (wallet: fast rescan with BIP157 block filters for descriptor wallets by theStack)
#25907 (wallet: rpc to add automatically generated descriptors by achow101)
#25297 (wallet: group independent db writes on single batched db transaction by furszy)
#23417 (wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager by achow101)
#22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)
#22341 (rpc: add getxpub by Sjors)
#20205 (wallet: Properly support a wallet id 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.

hebasto commented at 10:55 am on July 29, 2020: member

@achow101 It seems this PR has problems with linters.

achow101 force-pushed on Jul 29, 2020

luke-jr commented at 0:53 am on July 31, 2020: member

Why doesn’t this extend upgradewallet?

achow101 commented at 6:13 pm on July 31, 2020: member

Why doesn’t this extend upgradewallet?

It didn’t feel like it fit upgradewallet as I see that more for changing the wallet version number and using legacy wallet features. But this is changing a legacy wallet to something completely new.

This is also pretty invasive and fundamentally changes how the wallet is working, so I wanted to keep it separate from something that people might still want to use on legacy wallets.

achow101 force-pushed on Aug 3, 2020

laanwj commented at 6:09 pm on August 12, 2020: member

I think we should only add this functionality after sqlite wallets? Otherwise you’d keep migrating.

achow101 commented at 6:41 pm on August 12, 2020: member

I think we should only add this functionality after sqlite wallets? Otherwise you’d keep migrating.

I think there’s a question of whether we want to keep sqlite separate from descriptors. We might want to allow legacy wallets to migrate to sqlite without migrating to descriptors since they are orthogonal.

in src/wallet/rpcwallet.cpp:4116 in 97e34cd88d outdated

4112@@ -4113,6 +4113,33 @@ static UniValue upgradewallet(const JSONRPCRequest& request)
4113     return error.original;
4114 }
4115 
4116+static UniValue migratewallet(const JSONRPCRequest& request)

maflcko commented at 12:17 pm on August 14, 2020:

nit: might be good to use the non-deprecated constructor to avoid having to change this again. See e.g. #19528

achow101 commented at 5:53 pm on August 31, 2020:

Getting a build error when I try to do this:

 0    { "wallet",             "migratewallet",                    &migratewallet,                 {} },
 1    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2./rpc/server.h:120:5: note: candidate constructor not viable: no known conversion from 'RPCHelpMan (*)(const JSONRPCRequest &)' to 'rpcfn_type' (aka 'UniValue (*)(const JSONRPCRequest &)') for 3rd argument
 3    CRPCCommand(const char* category, const char* name, rpcfn_type fn, std::initializer_list<const char*> args)
 4    ^
 5./rpc/server.h:107:5: note: candidate constructor not viable: no known conversion from 'RPCHelpMan (*)(const JSONRPCRequest &)' to 'RpcMethodFnType' (aka 'RPCHelpMan (*)()') for 3rd argument
 6    CRPCCommand(std::string category, std::string name_in, RpcMethodFnType fn, std::vector<std::string> args_in)
 7    ^
 8./rpc/server.h:100:5: note: candidate constructor not viable: requires 5 arguments, but 4 were provided
 9    CRPCCommand(std::string category, std::string name, Actor actor, std::vector<std::string> args, intptr_t unique_id)
10    ^
11./rpc/server.h:91:7: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 4 were provided
12class CRPCCommand
13      ^
14./rpc/server.h:91:7: note: candidate constructor (the implicit move constructor) not viable: requires 1 argument, but 4 were provided
151 error generated.

maflcko commented at 6:27 pm on August 31, 2020:

Not sure why. This diff compiles for me:

 0diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
 1index cc89c4992b..1361955a4b 100644
 2--- a/src/wallet/rpcwallet.cpp
 3+++ b/src/wallet/rpcwallet.cpp
 4@@ -4171,9 +4171,9 @@ static UniValue upgradewallet(const JSONRPCRequest& request)
 5     return error.original;
 6 }
 7 
 8-static UniValue migratewallet(const JSONRPCRequest& request)
 9+static RPCHelpMan migratewallet()
10 {
11-    RPCHelpMan{"migratewallet",
12+    return RPCHelpMan{"migratewallet",
13         "\nMigrate the wallet to a descriptor wallet.\n"
14         "A new wallet backup will need to be made.",
15         {},
16@@ -4181,9 +4181,9 @@ static UniValue migratewallet(const JSONRPCRequest& request)
17         RPCExamples{
18             HelpExampleCli("migratewallet", "")
19             + HelpExampleRpc("migratewallet", "")
20-        }
21-    }.Check(request);
22-
23+        },
24+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
25+{
26     std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
27     if (!wallet) return NullUniValue;
28     CWallet* const pwallet = wallet.get();
29@@ -4196,6 +4196,8 @@ static UniValue migratewallet(const JSONRPCRequest& request)
30         throw JSONRPCError(RPC_WALLET_ERROR, error.original);
31     }
32     return error.original;
33+},
34+    };
35 }
36 
37 RPCHelpMan abortrescan();

achow101 commented at 10:57 pm on August 31, 2020:

Ah, I seem to have made a mistake somewhere.

maflcko commented at 12:17 pm on August 14, 2020: member

Left a style-nit. Feel free to ignore.

meshcollider commented at 11:49 pm on August 15, 2020: contributor

Do you know what the performance of a wallet would be like, in the case of a non-HD wallet being migrated?

achow101 commented at 5:05 am on August 16, 2020: member

Do you know what the performance of a wallet would be like, in the case of a non-HD wallet being migrated?

Probably worse.

meshcollider commented at 3:19 am on August 17, 2020: contributor

Probably worse.

Almost certainly, that’s why I’m asking :) It would be nice to know how much worse it would be

maflcko removed the label Tests on Aug 23, 2020

DrahtBot added the label Needs rebase on Aug 31, 2020

Sjors commented at 6:18 pm on August 31, 2020: member

Concept ACK, but I prefer this to be in the wallet tool.

achow101 force-pushed on Aug 31, 2020

achow101 commented at 10:58 pm on August 31, 2020: member

but I prefer this to be in the wallet tool.

I would prefer this to be accessible to most users so that more people can move to using descriptor wallets. For most wallets, this should be painless and not result in a horribly bloated wallet.

DrahtBot removed the label Needs rebase on Sep 1, 2020

Sjors commented at 12:09 pm on September 1, 2020: member

The wallet tool is just as available as the RPC. We can print the necessary incantation in the help if needed. It does make sense to have an upgrade button in the GUI though.

achow101 commented at 4:10 pm on September 1, 2020: member

The wallet tool is just as available as the RPC.

Not really. The RPC has a dedicated window within the GUI. The wallet tool requires actually using a terminal.

jonatack commented at 7:09 pm on September 1, 2020: contributor

Concept ACK, a few quick thoughts before reviewing:

debug build clean and local tests green
the help in bitcoin-cli help migratewallet is pretty sparse? – could benefit from a lot more helpful info to explain what/why/how to users, de-risk wallet migrations a bit and encourage users to move to descriptor wallets
agree with @meshcollider that performance info would be good here in the PR and in the doc (descriptors.md or a new one?) and/or rpc help
test is missing logging

achow101 commented at 4:48 pm on September 14, 2020: member

Hmm, I think the scriptPubKey set we are generating here is incorrect. I’m going to work on a slightly separate thing for LegacyScriptPubKeyMan that ensures we have the correct scriptPubKey set.

achow101 force-pushed on Sep 14, 2020

achow101 commented at 10:20 pm on September 14, 2020: member

Fixed the IsMine issue and added some tests for that. Also rebased onto master as there was a hidden merge conflict.

* the help in `bitcoin-cli help migratewallet` is pretty sparse? -- could benefit from a lot more helpful info to explain what/why/how to users, de-risk wallet migrations a bit and encourage users to move to descriptor wallets

Any suggestions?

* agree with [@meshcollider](/bitcoin-bitcoin/contributor/meshcollider/) that performance info would be good here in the PR and in the doc (`descriptors.md` or a new one?) and/or rpc help

I’ll do that eventually. After I figure out how.

* test is missing logging

Added some logging

achow101 force-pushed on Sep 30, 2020

achow101 force-pushed on Oct 1, 2020

jonatack commented at 7:41 pm on October 1, 2020: contributor

Thanks @achow101, I need to review and test this.

DrahtBot added the label Needs rebase on Oct 15, 2020

achow101 force-pushed on Oct 18, 2020

DrahtBot removed the label Needs rebase on Oct 18, 2020

achow101 force-pushed on Oct 24, 2020

DrahtBot added the label Needs rebase on Nov 9, 2020

achow101 force-pushed on Nov 9, 2020

DrahtBot removed the label Needs rebase on Nov 9, 2020

in test/functional/wallet_migration.py:9 in 208c296a57 outdated

0@@ -0,0 +1,280 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2020 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test Migrating a wallet from legacy to descriptor."""
6+
7+import os
8+
9+from test_framework.key import ECKey

adamjonas commented at 1:02 am on November 10, 2020:

The linter is complaining this going unused.

achow101 commented at 1:07 am on November 10, 2020:

Fixed.

achow101 force-pushed on Nov 10, 2020

laanwj referenced this in commit 8ffaf5c2f5 on Jan 13, 2021

sidhujag referenced this in commit 6a595aaab6 on Jan 13, 2021

achow101 force-pushed on Jan 13, 2021

maflcko removed the label Build system on Jan 14, 2021

maflcko removed the label Mempool on Jan 14, 2021

maflcko removed the label RPC/REST/ZMQ on Jan 14, 2021

maflcko removed the label UTXO Db and Indexes on Jan 14, 2021

maflcko removed the label Wallet on Jan 14, 2021

DrahtBot added the label RPC/REST/ZMQ on Jan 14, 2021

DrahtBot added the label Wallet on Jan 14, 2021

sidhujag referenced this in commit 6c60aa4ae4 on Jan 20, 2021

DrahtBot added the label Needs rebase on Jan 28, 2021

achow101 force-pushed on Jan 28, 2021

DrahtBot removed the label Needs rebase on Jan 28, 2021

DrahtBot added the label Needs rebase on Feb 18, 2021

in src/wallet/wallet.cpp:3699 in 1452840f94 outdated

4629+    // Close this database and delete the file
4630+    fs::path db_path = fs::path(m_database->Filename());
4631+    fs::path db_dir = db_path.branch_path();
4632+    std::string db_filename = db_path.leaf().string();
4633+    m_database->Close();
4634+    fs::remove(db_path);

laanwj commented at 12:40 pm on February 18, 2021:

This looks scary to me. Let’s not delete the old wallet, but rename it. You know, just in case something with the migration didn’t go well it’s good to have a backup.

achow101 commented at 5:50 pm on February 18, 2021:

~20 lines earlier we make a backup of the wallet. So there is still a backup and we’re just removing the original after it is backed up.

laanwj commented at 6:29 pm on February 18, 2021:

Okay, sounds good to me then.

achow101 force-pushed on Feb 18, 2021

DrahtBot removed the label Needs rebase on Feb 18, 2021

DrahtBot added the label Needs rebase on Apr 13, 2021

achow101 force-pushed on Apr 13, 2021

DrahtBot removed the label Needs rebase on Apr 13, 2021

DrahtBot added the label Needs rebase on Jun 24, 2021

achow101 force-pushed on Jun 24, 2021

DrahtBot removed the label Needs rebase on Jun 24, 2021

DrahtBot added the label Needs rebase on Jul 1, 2021

achow101 force-pushed on Jul 1, 2021

DrahtBot removed the label Needs rebase on Jul 1, 2021

DrahtBot added the label Needs rebase on Aug 9, 2021

achow101 force-pushed on Aug 17, 2021

DrahtBot removed the label Needs rebase on Aug 17, 2021

DrahtBot added the label Needs rebase on Sep 7, 2021

achow101 force-pushed on Sep 7, 2021

DrahtBot removed the label Needs rebase on Sep 7, 2021

laanwj added this to the milestone 23.0 on Sep 9, 2021

laanwj commented at 12:49 pm on September 9, 2021: member

Adding to the 23.0 milestone, I think it would be good to have optional migration in as soon as possible.

achow101 force-pushed on Sep 9, 2021

ghost commented at 0:21 am on September 10, 2021: none

Concept ACK

achow101 force-pushed on Sep 10, 2021

katesalazar commented at 11:12 am on September 26, 2021: contributor

What’s the minimum wallet version this tool will support?

achow101 commented at 10:29 pm on September 27, 2021: member

What’s the minimum wallet version this tool will support?

The minimum that can be opened currently.

DrahtBot added the label Needs rebase on Oct 4, 2021

achow101 force-pushed on Oct 4, 2021

DrahtBot removed the label Needs rebase on Oct 4, 2021

achow101 force-pushed on Oct 4, 2021

achow101 force-pushed on Oct 5, 2021

achow101 force-pushed on Oct 8, 2021

achow101 force-pushed on Oct 15, 2021

achow101 commented at 8:42 pm on November 9, 2021: member

Rebased

achow101 force-pushed on Nov 9, 2021

sipa commented at 9:22 pm on November 23, 2021: member

and a single key combo for the seed (the seed is a valid key that we can receive coins at!)

🤦

Nice catch.

in src/wallet/rpcwallet.cpp:4864 in 05d0543dd5 outdated

4859+    EnsureWalletIsUnlocked(*pwallet);
4860+
4861+    WalletContext& context = EnsureWalletContext(request.context);
4862+
4863+    bilingual_str error;
4864+    std::vector<bilingual_str> warnings;

maflcko commented at 7:37 pm on December 7, 2021:

is this ignored?

achow101 commented at 8:15 pm on December 7, 2021:

Removed, as well as in the Migrate* functions that aren’t actually using warnings.

in src/wallet/scriptpubkeyman.cpp:1680 in 05d0543dd5 outdated

1675+
1676+    MigrationData out;
1677+
1678+    std::unordered_set<CScript, SaltedSipHasher> spks;
1679+    auto spks_temp = GetScriptPubKeys();
1680+    spks.insert(spks_temp.begin(), spks_temp.end());

maflcko commented at 7:39 pm on December 7, 2021:

0    auto spks{GetScriptPubKeys()};

Does this not compile?

achow101 commented at 8:15 pm on December 7, 2021:

Done

in src/wallet/wallet.cpp:3582 in 05d0543dd5 outdated

3577+        assert(i != context.wallets.end());
3578+        this_wallet = *i;
3579+        context.wallets.erase(i);
3580+    }
3581+    // Now do the database stuff
3582+    if (!wallet.MigrateToSQLite(error ,warnings)) return false;

maflcko commented at 7:42 pm on December 7, 2021:

clang-format?

achow101 commented at 8:15 pm on December 7, 2021:

No longer relevant with warnings being removed

maflcko commented at 7:42 pm on December 7, 2021: member

left some nits/questions

achow101 force-pushed on Dec 7, 2021

in src/wallet/wallet.cpp:3948 in 86e1ff3e7a outdated

3606+        }
3607+        if (data->watch_descs.size() > 0) {
3608+            wallet.WalletLogPrintf("Making a new watchonly wallet containing the watched scripts\n");
3609+
3610+            DatabaseStatus status;
3611+            std::vector<bilingual_str> warnings;

maflcko commented at 8:18 pm on December 7, 2021:

well, now you are ignoring those?

achow101 commented at 9:01 pm on December 7, 2021:

I don’t think that those warnings matter. There aren’t any warnings for newly created wallets. These were being ignored originally anyways.

achow101 force-pushed on Dec 7, 2021

DrahtBot added the label Needs rebase on Dec 8, 2021

achow101 force-pushed on Dec 8, 2021

DrahtBot removed the label Needs rebase on Dec 8, 2021

DrahtBot added the label Needs rebase on Jan 11, 2022

achow101 force-pushed on Jan 11, 2022

DrahtBot removed the label Needs rebase on Jan 11, 2022

achow101 force-pushed on Jan 11, 2022

in test/functional/wallet_migration.py:111 in f07785d39b outdated

61+        self.nodes[0].createwallet(wallet_name="basic1")
62+        basic1 = self.nodes[0].get_wallet_rpc("basic1")
63+        assert_equal(basic1.getwalletinfo()["descriptors"], False)
64+
65+        for i in range(0, 10):
66+            default.sendtoaddress(basic1.getnewaddress(), 1)

maflcko commented at 8:26 am on January 12, 2022:

                               test_framework.authproxy.JSONRPCException: Requested wallet does not exist or is not loaded (-18)

https://cirrus-ci.com/task/6547488835371008?logs=ci#L4027

achow101 force-pushed on Jan 13, 2022

DrahtBot added the label Needs rebase on Jan 25, 2022

achow101 force-pushed on Jan 25, 2022

DrahtBot removed the label Needs rebase on Jan 25, 2022

laanwj removed this from the milestone 23.0 on Feb 10, 2022

laanwj added this to the milestone 24.0 on Feb 10, 2022

achow101 force-pushed on Mar 3, 2022

amovfx commented at 6:06 pm on March 9, 2022: none

Went through this for the bitcoin pr review club

michaelfolkson commented at 6:11 pm on March 9, 2022: contributor

Concept ACK, Approach ACK.

Definitely need a migration tool from legacy and/or BDB to descriptor and sqlite. I haven’t tested yet but the instructions I intend to follow are:

Create signet legacy-bdb wallet
Generate multiple signet addresses
Send funds to a particular signet address
Use the migratewallet RPC in this PR
Check the funds are now stored at the equivalent descriptor, the other generated descriptor equivalents are there too (the migration generally worked fine)

DrahtBot added the label Needs rebase on Apr 26, 2022

achow101 force-pushed on Apr 26, 2022

DrahtBot removed the label Needs rebase on Apr 26, 2022

DrahtBot added the label Needs rebase on Jul 12, 2022

achow101 force-pushed on Jul 13, 2022

DrahtBot removed the label Needs rebase on Jul 13, 2022

DrahtBot added the label Needs rebase on Aug 5, 2022

achow101 force-pushed on Aug 5, 2022

achow101 force-pushed on Aug 8, 2022

DrahtBot removed the label Needs rebase on Aug 8, 2022

Sjors commented at 6:22 pm on August 10, 2022: member

I’m tempted to suggest that we shouldn’t touch the original wallet, especially with the way this handles watch-only addresses. Maybe it’s better to create two new wallets (if there’s any watch-only address), copy the relevant transactions over and then unload the original wallet. Perhaps we can even add a flag to the original wallet to disable new address generation?

Also, although we should still recommend a backup, if the user doesn’t, it would be much safer if we just reuse the original seed instead generating a new one. Worst case they would find their old backup, upgrade it again, rescan the chain and find all their transactions.

achow101 commented at 6:31 pm on August 10, 2022: member

I’m tempted to suggest that we shouldn’t touch the original wallet, especially with the way this handles watch-only addresses. Maybe it’s better to create two new wallets (if there’s any watch-only address), copy the relevant transactions over and then unload the original wallet. Perhaps we can even add a flag to the original wallet to disable new address generation?

I think it might be confusing to have the original wallet under the original name, and then the migrated wallets with new names. Particularly if users have specified their wallet(s) in the bitcoin.conf file, as then the original would still always be opened. I suppose with settings.json we can at least ensure that the migrated ones are also always opened.

Sjors commented at 6:46 pm on August 10, 2022: member

Maybe rename the old one to …-archive-DATE?

achow101 commented at 6:47 pm on August 10, 2022: member

Maybe rename the old one to …-archive-DATE?

We already make a backup of it of the form name-timestamp.legacy.bak

Sjors commented at 6:55 pm on August 10, 2022: member

True. I suppose the upgrade is only potentially confusing for users with a mix of spendable and watch-only addresses. So perhaps there could be two seperate flows:

Spendable OR watch-only wallets: as implemented
Mixed wallet: keep the .legacy.bak wallet open and loaded, create the two wallets as you do now, but move the transactions to the correct wallet. Have a popup explain the split, and tell user they can should close .legacy.bak wallet when done.

In this case my suggestion to add a flag to prevent new address generation wouldn’t work, because we probably shouldn’t touch the backup at all. (we could have a convention of disabling the button for .bak files or something, but meh). Using the same master seed in the new wallet should work though?

achow101 commented at 7:12 pm on August 10, 2022: member

Mixed wallet: keep the .legacy.bak wallet open and loaded,

I don’t think it’s necessary to open the backup and then require the user to close it.

Using the same master seed in the new wallet should work though?

That can be done.

achow101 force-pushed on Aug 10, 2022

Sjors commented at 8:16 am on August 11, 2022: member

I don’t think it’s necessary to open the backup and then require the user to close it.

Another idea could be to refuse the migration if the wallet contains a combination of private key and watch-only stuff. Or to require that user specifies what the behaviour should be (keeping them in the same wallet or moving them to a fresh watch-only wallet).

achow101 commented at 3:13 pm on August 11, 2022: member

Another idea could be to refuse the migration if the wallet contains a combination of private key and watch-only stuff. Or to require that user specifies what the behaviour should be (keeping them in the same wallet or moving them to a fresh watch-only wallet).

I think anything that prevents the user from migrating or requires the user to make decisions or jump through hoops to do the migration is just going to result in people not migrating their wallets. We need people to migrate their wallets, and for the process to go as smoothly as possible, so that we can get rid of the legacy wallet.

achow101 force-pushed on Aug 11, 2022

achow101 commented at 7:59 pm on August 11, 2022: member

I’ve added an experimental warning on the RPC and some release notes describing what migratewallet will do and some additional warnings and instructions for users.

in src/wallet/scriptpubkeyman.cpp:1752 in b3c9806908 outdated

1742+                keyid_it = keyids.erase(keyid_it);
1743+                continue;
1744+            }
1745+        }
1746+        keyid_it++;
1747+    }

furszy commented at 9:14 pm on August 11, 2022:

Guess that you are looping first through mapKeys and mapCryptedKeys because in non-HD wallets we were not storing an entry inside mapKeyMetadata for each key?

Because otherwise, if we always have an entry inside mapKeyMetadata for each key, we could just loop through mapKeyMetadata to generate each single key combo descriptor directly.

achow101 commented at 9:21 pm on August 11, 2022:

There are some situations where a key record can exist without a corresponding metadata record. So it’s safer to go through all of the keys and pull up their metadata than just iterating mapKeyMetadata

achow101 force-pushed on Aug 11, 2022

in src/wallet/scriptpubkeyman.cpp:1970 in 0e4f63e871 outdated

1946@@ -1947,6 +1947,69 @@ std::optional<MigrationData> LegacyScriptPubKeyMan::MigrateToDescriptor()
1947     return out;
1948 }
1949 
1950+bool LegacyScriptPubKeyMan::DeleteRecords(bilingual_str& error)
1951+{
1952+    LOCK(cs_KeyStore);
1953+    WalletBatch batch(m_storage.GetDatabase());

furszy commented at 12:31 pm on August 12, 2022:

let’s begin a db txn here and commit it at the end of the function.

otherwise if an error arises in the middle, we will end up with half of the information removed.

achow101 commented at 3:37 pm on August 12, 2022:

Good point.

furszy commented at 12:41 pm on August 12, 2022: member

had an issue during migration (unknown cause..) and half of the info got removed from disk while the other half remained there.

now every time that I start the wallet, I get “seed not found” which means that the key got removed but the hd chain seed id not.

So, left a comment to solve the non-recoverable state at least. We should either remove all the records in a single atomic write or not remove them at all and fail.

achow101 force-pushed on Aug 12, 2022

in test/functional/wallet_migration.py:37 in 47b4eedc75 outdated

27+
28+    def assert_is_sqlite(self, wallet_name):
29+        wallet_file_path = os.path.join(self.nodes[0].datadir, "regtest/wallets", wallet_name, self.wallet_data_filename)
30+        with open(wallet_file_path, 'rb') as f:
31+            file_magic = f.read(16)
32+            assert_equal(file_magic, b'SQLite format 3\x00')

furszy commented at 1:11 pm on August 15, 2022:

couldn’t just do a getwalletinfo()["format"] == "sqlite"?

achow101 commented at 10:27 pm on August 15, 2022:

For these upgrade tests, I prefer to have checks external to the actual code as in-memory state could be different from on-disk state.

furszy commented at 8:03 pm on August 15, 2022: member

The PR description doesn’t seems to be accurate to the current sources. It says:

For the basic HD wallet case of just generated keys, the migration will create 3 descriptors for each HD chain: a ranged combo external, a ranged combo internal, and a single key combo for the seed (the seed is a valid key that we can receive coins at!)

And, for the basic case of an HD wallet, we are actually creating 11 descriptors on the migration process:

BIP32 descriptors; form of “0’/0’/” and “0’/1’/” (2 descriptors)
BIP44 descriptors; “44’/1’/0’/0/” and “44’/1’/0’/1/” (2 descriptors)
BIP49 descriptors, P2SH(P2WPKH); form of “49’/1’/0’/0/” and “49’/1’/0’/1/” (2 descriptors)
BIP84 descriptors, P2WPKH; form of “84’/1’/0’/1/” and “84’/1’/0’/1/” (2 descriptors)
BIP86 descriptors, P2TR; form of “86’/1’/0’/0/” and “86’/1’/0’/1/” (2 descriptors)
A combo(PK) descriptor for the wallet master key.

Then, while was checking what was going on, have expanded the test coverage of the basic HD wallet migration case with the 11 descriptors check and the following points:

Test case for a wallet with balance received on the seed. Receiving coins in different outputs created from the seed key (P2PKH, P2WPKH, P2SH(P2WPKH)) and verifying that after migration the wallet still has them.
Verify getaddressinfo remains unchanged after migration. (solvability, hdkeypath, ischange, hdmasterfingerprint)
Verify that the migrated wallet was flushed to disk by restarting the node and checking that balance/txes are still there.

If you like it, can cherry-pick it from: https://github.com/furszy/bitcoin/commit/e31779c3edca96a4a3f26ab554f7b9824ea0338b

The more coverage we can get over this, the better. Will continue reviewing :).

achow101 commented at 10:26 pm on August 15, 2022: member

For the basic HD wallet case of just generated keys, the migration will create 3 descriptors for each HD chain: a ranged combo external, a ranged combo internal, and a single key combo for the seed (the seed is a valid key that we can receive coins at!)

And, for the basic case of an HD wallet, we are actually creating 11 descriptors on the migration process:

It was meant to be read as “in addition to the usual descriptors”. I’ve updated the description to clarify that.

If you like it, can cherry-pick it from: furszy/bitcoin@e31779c

Squashed it into the test commit.

achow101 force-pushed on Aug 15, 2022

DrahtBot added the label Needs rebase on Aug 17, 2022

achow101 force-pushed on Aug 17, 2022

DrahtBot removed the label Needs rebase on Aug 17, 2022

in doc/release-notes-19602.md:25 in 5729eb9c8c outdated

20+Given that there is an extremely large number of possible configurations for the scripts that
21+Legacy wallets can know about, be watching for, and be able to sign for, `migratewallet` only
22+makes a best effort attempt to capture all of these things into Descriptor wallets. There may be
23+unforseen configurations which result in some scripts being excluded. If a migration fails
24+unexpectedly or otherwise misses any scripts, please create an issue on GitHub. A backup of the
25+original wallet can be found in the wallet directory with the name `<name>-<timestamp>.legacy.bak`.

Sjors commented at 11:35 am on August 17, 2022:

What should the user do if there’s a .log file? Delete it?

Sjors commented at 11:38 am on August 17, 2022:

(in the context of restoring a backup, since it’s a bit unclear if the log file belongs to the backup, the current version - could it interfere? - or if it’s just an artefact from before the backup was made, and safe to delete)

achow101 commented at 3:39 pm on August 17, 2022:

It’s safe to delete. The backup uses our normal backup code which compacts everything into the db so log files are no longer needed.

Sjors commented at 11:54 am on August 17, 2022: member

@achow101 I sent you a signet wallet with some watch-only keys that didn’t migrate correctly.

achow101 force-pushed on Aug 17, 2022

achow101 commented at 6:11 pm on August 17, 2022: member

@achow101 I sent you a signet wallet with some watch-only keys that didn’t migrate correctly.

This should be fixed now. Also added a few test cases for that failure.

achow101 force-pushed on Aug 17, 2022

achow101 commented at 9:16 pm on August 17, 2022: member

Latest push adds cleanup on failure.

achow101 force-pushed on Aug 17, 2022

achow101 force-pushed on Aug 18, 2022

in src/wallet/rpc/wallet.cpp:709 in 515dec58fb outdated

700@@ -701,6 +701,36 @@ RPCHelpMan simulaterawtransaction()
701     };
702 }
703 
704+static RPCHelpMan migratewallet()
705+{
706+    return RPCHelpMan{"migratewallet",
707+        "\nEXPERIMENTAL warning: This call may not work as expected and may be changed in future releases\n"
708+        "\nMigrate the wallet to a descriptor wallet.\n"
709+        "A new wallet backup will need to be made.",

darosior commented at 2:31 pm on August 19, 2022:

Maybe this could mention that a backup will be created in the same manner as by using backupwallet, that can be restored using restorewallet?

achow101 commented at 4:21 pm on August 19, 2022:

Added some text about that.

in doc/managing-wallets.md:134 in 9e66a06566 outdated

131+a newly created Descriptor wallet that has the same name as the original wallet. Because Descriptor
132+wallets do not support having private keys and watch-only scripts, there may be up to two
133+additional wallets created after migration. In addition to a descriptor wallet of the same name,
134+there may also be a wallet named `<name>_watchonly` and `<name>_solvables`. `<name>_watchonly`
135+contains all of the watchonly scripts. `<name>_solvables` contains any scripts which the wallet
136+knows the but is not watching the corresponding P2(W)SH scripts.

darosior commented at 2:37 pm on August 19, 2022:

nit: typo “knows the but is not watching”

achow101 commented at 4:22 pm on August 19, 2022:

Fixed.

in doc/managing-wallets.md:125 in 9e66a06566 outdated

119@@ -120,4 +120,29 @@ After that, `getwalletinfo` can be used to check if the wallet has been fully re
120 $ bitcoin-cli -rpcwallet="restored-wallet" getwalletinfo
121 ```
122 
123-The restored wallet can also be loaded in the GUI via `File` ->`Open wallet`.
124\ No newline at end of file
125+The restored wallet can also be loaded in the GUI via `File` ->`Open wallet`.
126+
127+## Migrating Legacy Wallets to Descriptor Wallets

darosior commented at 2:40 pm on August 19, 2022:

Thanks for adding documentation here.

nit: instead of duplicating the text in the release notes, maybe we could link to this document? Also, we could link to this document from places were we say BDB is legacy (such as in build-UNIX.md for instance)?

achow101 commented at 4:22 pm on August 19, 2022:

Done

darosior commented at 2:56 pm on August 19, 2022: member

I unfortunately won’t have time to review the code before feature freeze, but at least i successfully tested this with a mainnet watchonly legacy wallet (with a couple hundred transactions and a couple thousands entries in the address book) along with a legacy testnet wallets (with private keys) with a few hundreds transactions and a couple dozen entries in the address book.

achow101 force-pushed on Aug 19, 2022

Sjors commented at 3:07 pm on August 22, 2022: member

It would be good to explicitly test wallets with coinbase transactions on pk() “addresses”, since afaik that’s what the built-in miner used.

achow101 commented at 5:02 pm on August 22, 2022: member

It would be good to explicitly test wallets with coinbase transactions on pk() “addresses”, since afaik that’s what the built-in miner used.

Done

achow101 force-pushed on Aug 22, 2022

achow101 force-pushed on Aug 23, 2022

in src/wallet/scriptpubkeyman.cpp:1802 in 5967384781 outdated

1796+    // Handle HD keys by using the CHDChains
1797+    std::set<CHDChain> chains;
1798+    chains.insert(m_hd_chain);
1799+    for (const auto& chain_pair : m_inactive_hd_chains) {
1800+        chains.insert(chain_pair.second);
1801+    }

w0xlt commented at 5:59 pm on August 24, 2022:

Could this be a vector, assuming m_inactive_hd_chains doesn’t store repeated CHDChain items ? This way, changing src/wallet/walletdb.h would not be necessary..

0    std::vector<CHDChain> chains;
1    chains.push_back(m_hd_chain);
2    for (const auto& chain_pair : m_inactive_hd_chains) {
3        chains.push_back(chain_pair.second);
4    }

achow101 commented at 7:10 pm on August 24, 2022:

Done

w0xlt commented at 5:59 pm on August 24, 2022: contributor

Approach ACK

achow101 force-pushed on Aug 24, 2022

in src/wallet/scriptpubkeyman.cpp:1959 in 7427877158 outdated

1954+    batch.TxnBegin();
1955+    // Remove the watchonly things
1956+    for (const CScript& script : setWatchOnly) {
1957+        if (!batch.EraseWatchOnly(script)) {
1958+            error = strprintf(_("Error: Could not delete watch only script %s"), HexStr(script));
1959+            return false;

w0xlt commented at 11:36 pm on August 24, 2022:

Is batch.Tx Abort(); required in case any deletion fails here?

0            batch.TxnAbort();
1            return false;

achow101 commented at 11:43 pm on August 24, 2022:

No, TxnAbort will be called by the destructor.

in src/wallet/wallet.cpp:1433 in 7427877158 outdated

1424@@ -1425,6 +1425,12 @@ bool CWallet::IsMine(const CTransaction& tx) const
1425     for (const CTxOut& txout : tx.vout)
1426         if (IsMine(txout))
1427             return true;
1428+    for (const CTxIn& txin : tx.vin) {
1429+        const CWalletTx* wtx = GetWalletTx(txin.prevout.hash);
1430+        if (wtx && wtx->tx->vout.size() > txin.prevout.n && IsMine(wtx->tx->vout[txin.prevout.n])) {
1431+            return true;
1432+        }
1433+    }

w0xlt commented at 4:01 am on August 25, 2022:

The purpose here is to identify the transactions whose inputs belong to the wallet ?

achow101 commented at 4:39 am on August 25, 2022:

Yes

furszy commented at 12:25 pm on August 25, 2022:

This IsMine change will have implications inside AddToWalletIfInvolvingMe as well. We are checking tx ownership using the following conditions: if (fExisted || IsMine(tx) || IsFromMe(tx))

So, with this changes, the middle IsMine will check what IsFromMe currently checks. So.. either here or in a follow-up PR, can remove the IsFromMe function entirely.

Other point is that you can simplify the code:

0for (const CTxIn& txin : tx.vin) {
1    if (IsMine(txin.prevout)) return true; 
2}

achow101 commented at 7:37 pm on August 25, 2022:

I’ve removed this change and switched to using IsFromMe in addition to IsMine. Generally we should avoid changing IsMine semantics when possible.

Sjors commented at 1:30 pm on August 25, 2022: member

I noticed that the migrated combo() descriptor and the addr() descriptor in the watch-only part of the migrated wallet uses timestamp 0. Can it use the original timestamps instead?

The new bech32m descriptor can’t be used in the GUI unless you reload the wallet, since the dropdown is not updated.

I sent some coins to the newly generated address and then repeated the migration (using the backup) while it was still in the mempool. That causes the new transaction to not show up as well as a warning in the log:

02022-08-25T13:32:06Z [main] GUI: TransactionTablePriv::updateWallet: Warning: Got CT_NEW, but transaction is not in wallet
12022-08-25T13:32:06Z [main] GUI: TransactionTablePriv::updateWallet: Warning: Got CT_NEW, but transaction is not in wallet

A simple restart does the trick there. In any case, new funds are safu.

Other than that it all works with my test wallet. Will look at the code.

in src/wallet/scriptpubkeyman.h:246 in 220fe61f70 outdated

241@@ -242,6 +242,9 @@ class ScriptPubKeyMan
242 
243     virtual uint256 GetID() const { return uint256(); }
244 
245+    /** Returns a vector of all the scriptPubKeys that this ScriptPubKeyMan watches */
246+    virtual const std::unordered_set<CScript, SaltedSipHasher> GetScriptPubKeys() const { return {}; };

Sjors commented at 2:25 pm on August 25, 2022:

220fe61f70f6093a695fd8b6e38e928b63c2b6ad: I assume the goal here is to get faster lookups and random access deletions? E.g. in the erase() call in the next commit.

Afaik there’s no guarantee in the standard library that std::unordered_set will put elements in the same order between runs. So if deterministic behaviour is the goal of this change, it might be better to just stick to std::vector but call sort() on it before returning. But that’s slower.

Code comment nit: not a “vector”

Suggest renaming commit to: “scriptpubkeyman: Implement GetScriptPubKeys for Legacy” (the current title suggests it’s an entirely new method)

achow101 commented at 3:15 pm on August 25, 2022:

We don’t need deterministic behavior. That’s why I elected to use std::unordered_set and SaltedSipHasher (which is initialized with random salts).

achow101 commented at 3:26 pm on August 25, 2022:

Fixed the comment and commit message.

in src/wallet/wallet.cpp:3636 in 220fe61f70 outdated

3632@@ -3633,7 +3633,7 @@ ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const Flat
3633         }
3634 
3635         CTxDestination dest;
3636-        if (!internal && ExtractDestination(script_pub_keys.at(0), dest)) {
3637+        if (!internal && ExtractDestination(*script_pub_keys.begin(), dest)) {

Sjors commented at 2:50 pm on August 25, 2022:

220fe61f70f6093a695fd8b6e38e928b63c2b6ad: shouldn’t this be a loop over all elements anyway, for combo(). It seems it would always label the P2PK variant before (which ExtractDestination turns into P2PKH), and a random one after.

achow101 commented at 3:26 pm on August 25, 2022:

Done

Sjors commented at 4:05 pm on August 25, 2022:

That broke tool_wallet.py —descriptors because Address Book: needs to be 3 now instead of 1.

achow101 commented at 7:57 pm on August 25, 2022:

Extracted to a separate commit and fixed the test.

in src/wallet/wallet.cpp:3949 in 7ab26895d8 outdated

3945@@ -3946,7 +3946,7 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error,
3946 
3947             DatabaseStatus status;
3948             std::vector<bilingual_str> warnings;
3949-            std::string wallet_name = wallet.Getname() + "_watchonly";
3950+            std::string wallet_name = wallet.GetName() + "_watchonly";

w0xlt commented at 3:10 pm on August 25, 2022:

Perhaps the change in src/wallet/wallet.cpp in https://github.com/bitcoin/bitcoin/commit/7ab26895d879e8cadd70109bd84c0b7e586c31a2 (‘Add migratewallet RPC’) could be squashed into https://github.com/bitcoin/bitcoin/commit/211c12bcd4545aa457a2efda17dfbf6a486407c3 (‘Implement MigrateLegacyToDescriptor’).

Currently, https://github.com/bitcoin/bitcoin/commit/211c12bcd4545aa457a2efda17dfbf6a486407c3 does not compile..

achow101 commented at 3:26 pm on August 25, 2022:

Done

achow101 force-pushed on Aug 25, 2022

achow101 commented at 3:47 pm on August 25, 2022: member

The new bech32m descriptor can’t be used in the GUI unless you reload the wallet, since the dropdown is not updated.

This appears to be an issue in general. Importing a tr descriptor to a wallet that does not have one yet does not cause the dropdown to be updated.

achow101 force-pushed on Aug 25, 2022

in src/wallet/scriptpubkeyman.h:517 in fc33d30687 outdated

512@@ -511,6 +513,9 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
513 
514     std::set<CKeyID> GetKeys() const override;
515     const std::unordered_set<CScript, SaltedSipHasher> GetScriptPubKeys() const override;
516+
517+    /** Get the DescriptScriptPubKeyMans that have the same scriptPubKeys as this LegacyScriptPubKeyMan */

Sjors commented at 3:55 pm on August 25, 2022:

fc33d306874ec3bca38c34ff5ac49cbcfa7d832f nit: DescriptorScriptPubKeyMans

Sjors commented at 7:03 pm on August 25, 2022:

fc33d306874ec3bca38c34ff5ac49cbcfa7d832f Description could be slightly more elaborate. Maybe mention that it’s non-invasive and requires the wallet to be unlocked.

achow101 commented at 7:12 pm on August 25, 2022:

Done

in src/wallet/scriptpubkeyman.cpp:1829 in fc33d30687 outdated

1823+            WalletDescriptor w_desc(std::move(desc), 0, 0, chain_counter, 0);
1824+
1825+            // Make the DescriptorScriptPubKeyMan and get the scriptPubKeys
1826+            auto desc_spk_man = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(m_storage, w_desc));
1827+            desc_spk_man->AddDescriptorKey(master_key.key, master_key.key.GetPubKey());
1828+            desc_spk_man->TopUp();

Sjors commented at 4:15 pm on August 25, 2022:

fc33d306874ec3bca38c34ff5ac49cbcfa7d832f: I think you need to pass chain_counter here, because DescriptorScriptPubKeyMan::TopUp defaults to -keypool, not to the range you passed into the descriptor.

achow101 commented at 4:59 pm on August 25, 2022:

No, it will account for the range_end.

Sjors commented at 5:04 pm on August 25, 2022:

Oh wait, I only looked at the first few lines where it sets target_size. But then later on it also looks at m_wallet_descriptor.range_end

in src/wallet/scriptpubkeyman.cpp:1771 in fc33d30687 outdated

1765+        }
1766+
1767+        // Get the key origin
1768+        // Maybe this doesn't matter because floating keys here shouldn't have origins
1769+        KeyOriginInfo info;
1770+        bool has_info = GetKeyOrigin(keyid, info);

Sjors commented at 6:36 pm on August 25, 2022:

fc33d306874ec3bca38c34ff5ac49cbcfa7d832f: GetKeyOrigin always returns true for LegacyScriptPubKeyMan, but we probably don’t want to add spurious origin info for floating keys. So maybe GetKeyOrigin can return false instead, or you add a new HasKeyOrigin method.

achow101 commented at 7:30 pm on August 25, 2022:

Done.

in src/wallet/scriptpubkeyman.cpp:1790 in fc33d30687 outdated

1784+        auto desc_spks = desc_spk_man->GetScriptPubKeys();
1785+
1786+        // Remove the scriptPubKeys from our current set
1787+        for (const CScript& spk : desc_spks) {
1788+            size_t erased = spks.erase(spk);
1789+            assert(erased == 1);

Sjors commented at 6:42 pm on August 25, 2022:

fc33d306874ec3bca38c34ff5ac49cbcfa7d832f Wouldn’t this crash if for some reason the legacy wallet didn’t have the full combo collection?

How did we handle uncompressed keys? Did we never have them? Do they have a different ID? In other words, erased can’t be 2?

achow101 commented at 7:20 pm on August 25, 2022:

No. This is a check that it erased something rather than it didn’t erase more than one. spks is a set so it can’t erase more than 1 anyways. This check ensures that the descriptors we create actually correspond to scriptPubKeys the legacy wallet watched.

in src/wallet/scriptpubkeyman.cpp:1806 in fc33d30687 outdated

1800+        chains.push_back(chain_pair.second);
1801+    }
1802+    for (const CHDChain& chain : chains) {
1803+        for (int i = 0; i < 2; ++i) {
1804+            // Skip if doing internal chain and split chain is not supported
1805+            if (chain.seed_id.IsNull() || (i == 1 && !m_storage.CanSupportFeature(FEATURE_HD_SPLIT))) {

Sjors commented at 6:49 pm on August 25, 2022:

fc33d306874ec3bca38c34ff5ac49cbcfa7d832f: when can seed_id be absent?

achow101 commented at 7:23 pm on August 25, 2022:

Generally it can’t be.

in src/wallet/scriptpubkeyman.cpp:1824 in fc33d30687 outdated

1818+            std::string desc_str = "combo(" + xpub + "/0'/" + ToString(i) + "'/*')";
1819+            FlatSigningProvider keys;
1820+            std::string error;
1821+            std::unique_ptr<Descriptor> desc = Parse(desc_str, keys, error, false);
1822+            uint32_t chain_counter = std::max((i == 1 ? chain.nInternalChainCounter : chain.nExternalChainCounter), (uint32_t)0);
1823+            WalletDescriptor w_desc(std::move(desc), 0, 0, chain_counter, 0);

Sjors commented at 6:53 pm on August 25, 2022:

fc33d306874ec3bca38c34ff5ac49cbcfa7d832f: can we get a creation_time time from somewhere?

achow101 commented at 7:24 pm on August 25, 2022:

No. CHDChains do not have any time metadata.

Sjors commented at 8:15 pm on August 25, 2022:

Sad, not even when they were first put in the keypool? Though perhaps that’s too risky if we’re upgrading a wallet that was itself restored from a backup. The downside of setting 0 is that if a user were to dump the descriptors and restore them into a fresh wallet, they’d have to rescan from genesis. Though that’s not the backup / recovery flow atm.

in src/wallet/scriptpubkeyman.cpp:1879 in fc33d30687 outdated

1874+            // Get the scriptPubKeys without writing this to the wallet
1875+            FlatSigningProvider provider;
1876+            desc->Expand(0, provider, desc_spks, provider);
1877+        } else {
1878+            // Make the DescriptorScriptPubKeyMan and get the scriptPubKeys
1879+            WalletDescriptor w_desc(std::move(desc), 0, 0, 0, 0);

Sjors commented at 6:56 pm on August 25, 2022:

fc33d306874ec3bca38c34ff5ac49cbcfa7d832f: can we get a creation_time time from somewhere?

achow101 commented at 7:24 pm on August 25, 2022:

Done

in src/wallet/scriptpubkeyman.cpp:1963 in fc33d30687 outdated

1941+            }
1942+        }
1943+    }
1944+
1945+    // Make sure that we have accounted for all scriptPubKeys
1946+    assert(spks.size() == 0);

Sjors commented at 7:00 pm on August 25, 2022:

fc33d306874ec3bca38c34ff5ac49cbcfa7d832f: we should probably fail more gracefully.

achow101 commented at 7:25 pm on August 25, 2022:

An assert is used here because it is a critical bug. The purpose is to have something that obviously fails during testing.

Sjors commented at 8:12 pm on August 25, 2022:

But can we really rule out that this happens in production on some obscure old weird wallet? If not, it would be better to have the RPC throw and maybe provide some info about the scripts that are left.

achow101 commented at 8:45 pm on August 25, 2022:

I’m pretty confident that we can.

Sjors commented at 7:03 pm on August 25, 2022: member

Reviewed fc33d306874ec3bca38c34ff5ac49cbcfa7d832f which looks mostly good. I skimmed over the multisig part though. Will review the other commits later.

achow101 force-pushed on Aug 25, 2022

achow101 commented at 7:14 pm on August 25, 2022: member

I noticed that the migrated combo() descriptor and the addr() descriptor in the watch-only part of the migrated wallet uses timestamp 0. Can it use the original timestamps instead?

Done

A simple restart does the trick there. In any case, new funds are safu.

I’ve changed it to reload the wallet after migrating so that all of the signals and gui stuff get refreshed.

This happened to expose a bug where not all legacy records were being removed from all wallets. I’ve pushed a change to how we do the deletions to make sure there is no lingering data.

achow101 force-pushed on Aug 25, 2022

Apply label to all scriptPubKeys of imported combo() e664af2976

scriptpubkeyman: Implement GetScriptPubKeys in Legacy ea1ab390e4

Implement LegacyScriptPubKeyMan::MigrateToDescriptor 35f428fae6

achow101 force-pushed on Aug 25, 2022

in test/functional/wallet_migration.py:40 in fb10bae0e6 outdated

35+        with open(wallet_file_path, 'rb') as f:
36+            file_magic = f.read(16)
37+            assert_equal(file_magic, b'SQLite format 3\x00')
38+        assert_equal(self.nodes[0].get_wallet_rpc(wallet_name).getwalletinfo()["format"], "sqlite")
39+
40+    def create_wallet(self, wallet_name):

w0xlt commented at 9:52 pm on August 25, 2022:

nit: makes the purpose of the function clearer.

0    def create_legacy_wallet(self, wallet_name):

achow101 commented at 6:43 pm on August 26, 2022:

Done

in test/functional/wallet_migration.py:168 in fb10bae0e6 outdated

163+
164+        # Contrived case where all the multisig keys are in a single wallet
165+        self.log.info("Test migration of a wallet with all keys for a multisig")
166+        self.nodes[0].createwallet(wallet_name="multisig0")
167+        multisig0 = self.nodes[0].get_wallet_rpc("multisig0")
168+        assert_equal(multisig0.getwalletinfo()["descriptors"], False)

w0xlt commented at 1:43 am on August 26, 2022:

nit: (or create_legacy_wallet, as suggested above).

0        multisig0 = self.create_wallet("multisig0")

achow101 commented at 6:44 pm on August 26, 2022:

Done

in src/wallet/rpc/wallet.cpp:733 in fb10bae0e6 outdated

728+
729+    bilingual_str error;
730+    if (!MigrateLegacyToDescriptor(std::move(wallet), context, error)) {
731+        throw JSONRPCError(RPC_WALLET_ERROR, error.original);
732+    }
733+    return NullUniValue;

w0xlt commented at 2:08 am on August 26, 2022:

Maybe it can return a success message and others indicating the backup, watchonly, solvable and multisig file path?

achow101 commented at 6:44 pm on August 26, 2022:

Done

in src/wallet/walletdb.cpp:62 in c50135edb6 outdated

58@@ -59,6 +59,7 @@ const std::string WALLETDESCRIPTORCKEY{"walletdescriptorckey"};
59 const std::string WALLETDESCRIPTORKEY{"walletdescriptorkey"};
60 const std::string WATCHMETA{"watchmeta"};
61 const std::string WATCHS{"watchs"};
62+const std::unordered_set<std::string> LEGACY_TYPES{CRYPTED_KEY, CSCRIPT, DEFAULTKEY, HDCHAIN, KEYMETA, KEY, OLD_KEY, POOL, WATCHMETA, WATCHS};

Sjors commented at 8:03 am on August 26, 2022:

c50135edb6414fa5fd598f65e13eaa0e639573b9: maybe add comment: // These keys are deleted by the migratewallet RPC to ensure we don’t add more stuff to this list without making sure they get migrated.

Note to other reviewers (plus some open questions), these types are captured by the backup as follows:

CRYPTED_KEY: when walletdb.cpp loads them, it puts them into mapCryptedKeys via LoadCryptedKey(). MigrateToDescriptor() iterates over that map.
CSCRIPT: when walletdb.cpp loads them, it puts them into mapScripts via LoadCScript().
- MigrateToDescriptor() iterates over any multisig scripts in that map
- TODO: I’m unclear what happens to mapScripts entries that are not multisig (see comment on earlier commit)
- caveat: old records with redeemScript.size() > MAX_SCRIPT_ELEMENT_SIZE are lost in the upgrade (they’re in the backup)
DEFAULTKEY: this is only used to detect corruption, and is not migrated, newer wallets don’t have this key
HDCHAIN: m_hd_chain which we migrate
KEYMETA: ends up in mapKeyMetadata via LoadKeyMetadata, which we migrate (at least for every key we have).
KEY: when walletdb.cpp loads them, it puts them into mapKeys via LoadKey(). MigrateToDescriptor() iterates over that map.
OLD_KEY: these can only be loaded with v0.18 and older. They are skipped during wallet load, so they are lost in the migration.
POOL: these are ignored, we regenerate the keypool using descriptors
WATCHMETA: put in m_script_metadata by LoadScriptMetadata, and indexed by their CScriptID, TODO: same question as with CSCRIPT
WATCHS put in mapWatchKeys via LoadWatchOnly, and its keys are used to monitor PKH, etc via ImplicitlyLearnRelatedKeyScripts. We don’t iterate over mapWatchKeys directly, so I’m a bit confused how these are migrated.

in src/wallet/scriptpubkeyman.cpp:1927 in 35f428fae6 outdated

1922+            creation_time = it->second.nCreateTime;
1923+        }
1924+
1925+        std::vector<std::vector<unsigned char>> sols;
1926+        TxoutType type = Solver(script, sols);
1927+        if (type == TxoutType::MULTISIG) {

Sjors commented at 8:22 am on August 26, 2022:

35f428fae68ad974abdce0fa905148f620a9443c What happens to mapScripts entries that are not MULTISIG?

achow101 commented at 5:01 pm on August 26, 2022:

They will be part of spks and added to the watchonly wallet. It is not possible to import other scripts without also watching them (and their P2(W)SH). Multisigs are special because we have addmultisigaddress which add the multisig but does not watch them. All of the import rpcs will watch.

in src/wallet/scriptpubkeyman.cpp:1967 in c50135edb6 outdated

1963@@ -1964,6 +1964,13 @@ std::optional<MigrationData> LegacyScriptPubKeyMan::MigrateToDescriptor()
1964     return out;
1965 }
1966 
1967+bool LegacyScriptPubKeyMan::DeleteRecords()

Sjors commented at 8:44 am on August 26, 2022:

c50135edb6414fa5fd598f65e13eaa0e639573b9 : DeleteLegacyRecords (since we keep plenty of other stuff)

achow101 commented at 5:04 pm on August 26, 2022:

We delete all of the LegacyScriptPubKeyMan records (which this function is a member of). There are no remaining LegacySPKM records after this.

Sjors commented at 8:46 am on August 26, 2022: member

Thanks for the changes. Reviewed another commit, will continue later.

in src/wallet/wallet.cpp:3950 in a6c0e2f844 outdated

3943+            wallet.WalletLogPrintf("Making a new watchonly wallet containing the watched scripts\n");
3944+
3945+            DatabaseStatus status;
3946+            std::vector<bilingual_str> warnings;
3947+            std::string wallet_name = wallet.GetName() + "_watchonly";
3948+            data->watchonly_wallet = CreateWallet(context, wallet_name, std::nullopt, options, status, error, warnings);

Sjors commented at 12:24 pm on August 26, 2022:

a6c0e2f844f589df4f2d19e38c2e350952e1b4bf: bit of an aesthetic point, but I don’t think the MigrationData struct should have the CWallet pointers. Instead you could just pass watchonly_wallet and solvable_wallet into ApplyMigrationData. That also makes it more clear what the data is applied to.

achow101 commented at 5:06 pm on August 26, 2022:

I don’t think that’s any clearer, and would like to keep the function signature small.

in src/wallet/wallet.h:929 in a6c0e2f844 outdated

919@@ -920,6 +920,16 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
920 
921     //! Add a descriptor to the wallet, return a ScriptPubKeyMan & associated output type
922     ScriptPubKeyMan* AddWalletDescriptor(WalletDescriptor& desc, const FlatSigningProvider& signing_provider, const std::string& label, bool internal) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
923+
924+    //! Migrate a BDB wallet to SQLite wallet
925+    bool MigrateToSQLite(bilingual_str& error) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

Sjors commented at 12:40 pm on August 26, 2022:

a6c0e2f844f589df4f2d19e38c2e350952e1b4bf might be worth giving this its own commit. Ideally with a round-trip test, though something simpler might be fine too.

To distinguish this a bit better from the other “DoMigrate” like functions:

0/* Migrate all records from the BDB storage to a new SQLite wallet with the same file name.
1 * Does not create a backup and deletes the original file. May crash at any point if something
2 * unexpected happens in the filesystem. */

achow101 commented at 6:44 pm on August 26, 2022:

Done

in src/wallet/wallet.cpp:4039 in a6c0e2f844 outdated

4034+            });
4035+        assert(i != context.wallets.end());
4036+        context.wallets.erase(i);
4037+    }
4038+    // Now do the database stuff
4039+    if (!wallet->MigrateToSQLite(error)) return false;

Sjors commented at 12:53 pm on August 26, 2022:

a6c0e2f844f589df4f2d19e38c2e350952e1b4bf: if this fails after the file deletion, can we try to move the backup back in place? If that fails too, have the RPC throw an error telling the user the name of the backup file and they need to copy / rename it.

Similarly the asserts inside MigrateToSQLite could return false instead, though in that case moving the backup file back might fail.

MigrateToSQLite could take a second argument bool& deleted so we know whether or not the backup file needs to be moved.

Or you could create a file with a different name, defer deleting all the way to the end, and then rename the new file to match the original name. But then the m_database hot swap stuff won’t work.

achow101 commented at 6:42 pm on August 26, 2022:

Since these are filesystem errors, I don’t think restoring the backup would actually work. Furthermore, because m_database has been swapped out, and potentially doesn’t have a backing database, I don’t think it’s safe to continue execution in that state as the CWallet would still exist, but nowhere to write data to.

in src/wallet/wallet.cpp:3706 in a6c0e2f844 outdated

3701+    // Make new DB
3702+    DatabaseOptions opts;
3703+    opts.require_create = true;
3704+    opts.require_format = DatabaseFormat::SQLITE;
3705+    DatabaseStatus db_status;
3706+    std::unique_ptr<WalletDatabase> new_db = MakeDatabase(db_dir, opts, db_status, error);

Sjors commented at 1:22 pm on August 26, 2022:

a6c0e2f844f589df4f2d19e38c2e350952e1b4bf: maybe move the file creation to a common method, so that if we ever decide to use some new opts field, we don’t forget to apply it here.

achow101 commented at 5:08 pm on August 26, 2022:

Perhaps for a followup to do this everywhere we are making wallets.

in src/wallet/wallet.cpp:3707 in a6c0e2f844 outdated

3702+    DatabaseOptions opts;
3703+    opts.require_create = true;
3704+    opts.require_format = DatabaseFormat::SQLITE;
3705+    DatabaseStatus db_status;
3706+    std::unique_ptr<WalletDatabase> new_db = MakeDatabase(db_dir, opts, db_status, error);
3707+    assert(new_db); // This is to prevent doing anything further with this wallet. The original file was deleted, but a backup exists.

Sjors commented at 1:23 pm on August 26, 2022:

a6c0e2f844f589df4f2d19e38c2e350952e1b4bf: I don’t understand this comment.

Also, should we check db_status?

achow101 commented at 5:09 pm on August 26, 2022:

No. it just contains more granular errors. There’s no “success by with caveats” that would be relevant here.

in src/wallet/wallet.cpp:3671 in a6c0e2f844 outdated

3666+        return false;
3667+    }
3668+
3669+    // Get all of the records for DB type migration
3670+    std::unique_ptr<DatabaseBatch> batch = m_database->MakeBatch();
3671+    std::map<SerializeData, SerializeData> records;

Sjors commented at 1:28 pm on August 26, 2022:

a6c0e2f844f589df4f2d19e38c2e350952e1b4bf: I know record sequence shouldn’t matter, but at the same time it’s trivial to write them back in the same order just in case, with a std::vector<std::pair<SerializeData, SerializeData>>

achow101 commented at 6:44 pm on August 26, 2022:

Done

in src/wallet/wallet.cpp:3741 in a6c0e2f844 outdated

3734+        error = _("Error: This wallet is already a descriptor wallet");
3735+        return std::nullopt;
3736+    }
3737+
3738+    std::optional<MigrationData> res = legacy_spkm->MigrateToDescriptor();
3739+    if (res == std::nullopt) {

Sjors commented at 1:35 pm on August 26, 2022:

a6c0e2f844f589df4f2d19e38c2e350952e1b4bf: maybe just explicitly check for a lock before calling MigrateToDescriptor, so we can allow that function to fail and return nothing in other situations.

achow101 commented at 5:12 pm on August 26, 2022:

I don’t understand what you’re talking about.

Sjors commented at 10:25 am on August 29, 2022:

You’re assuming here that the reason std::nullopt is returned is that the wallet is locked, rather than some other failure.

furszy commented at 8:19 pm on August 29, 2022:

You’re assuming here that the reason std::nullopt is returned is that the wallet is locked, rather than some other failure.

The RPC handler is ensuring that the wallet is unlocked (third line there). This check is redundant for now, and might be useful when a migration button is implemented on the GUI.

achow101 commented at 9:29 pm on August 29, 2022:

I don’t think the message implies that the only reason it would return nullopt is because it is locked. However, it is likely that a failure would be due to being locked, hence the message. We do this in a couple of other places too where failure is often being locked, but could be some weird db condition that actually happened.

in src/wallet/wallet.h:931 in a6c0e2f844 outdated

926+
927+    //! Get all of the descriptors from a legacy wallet
928+    std::optional<MigrationData> GetDescriptorsForLegacy(bilingual_str& error) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
929+
930+    //! Adds the ScriptPubKeyMans given in MigrationData, removes LegacyScriptPubKeyMan, and moves tx and address book
931+    //! to their relevant wallets.

Sjors commented at 1:51 pm on August 26, 2022:

a6c0e2f844f589df4f2d19e38c2e350952e1b4bf: could add this (doesn’t?) requires the wallet storage to have been converted to SQLite. Also “given in MigrationData to this wallet”

0//! , and where needed moves tx and address book entries to watchonly_wallet or solvable_wallet.

achow101 commented at 6:44 pm on August 26, 2022:

Done

Sjors commented at 2:01 pm on August 26, 2022: member

Reviewed up to a6c0e2f844f589df4f2d19e38c2e350952e1b4bf. Mostly happy. I’m a bit confused about how legacy wallets deal with (unwatched) solvable scripts, so I’ll review that aspect again once I wrap my head around it.

bitcoin deleted a comment on Aug 26, 2022

Implement LegacyScriptPubKeyMan::DeleteRecords 22401f17e0

wallet: Refactor SetupDescSPKMs to take CExtKey

Refactors SetupDescSPKMs so that the DescSPKM loops are in their own
function. This allows us to call it later during migration with a key
that was already generated.

5b62f095e7

achow101 force-pushed on Aug 26, 2022

bitcoin deleted a comment on Aug 26, 2022

w0xlt approved

w0xlt commented at 10:25 pm on August 26, 2022: contributor

ACK 3299574f5e27bdb6c12fffd541e7cbfd148ab883

in src/wallet/rpc/wallet.cpp:720 in 4aad336eef outdated

715+        RPCResult{
716+            RPCResult::Type::OBJ, "", "",
717+            {
718+                {RPCResult::Type::STR, "wallet_name", "The name of the primary migrated wallet"},
719+                {RPCResult::Type::STR, "watchonly_name", /*optional=*/true, "The name of the migrated wallet containing the watchonly scripts"},
720+                {RPCResult::Type::STR, "solvables_name", /*optional=*/true, "The name of th migrated wallet containing solvable but not watched scripts"},

Sjors commented at 10:38 am on August 29, 2022:

4aad336eefe0f0625d4f812a2e4029582f95acee nit: th

achow101 commented at 4:31 pm on August 29, 2022:

Done

Sjors commented at 1:12 pm on August 29, 2022: member

I started the GUI, made some transactions and then called migratewallet from the command line. Between adding the watch-only wallet and setting the new spkMans there was a 9 minute gap in which nothing seemed to happen. The log then showed “Wallet migration complete.”, but the RPC call still didn’t return. The GUI was unresponive. Eventually it hit the RPC client timeout, but GUI remained unresponsive.

I was unable to do a clean shutdown with ctrl+c from the terminal (where I started QT from). A gentle kill didn’t work either (kill -9 did). I guess there’s a lock order issue somewhere.

I repeated the experiment calling from the GUI console. There was again a 9 minute gap after watch-only was created, and the console got stuck in Executing, except the GUI never times out.

After migration the watch-only labels were missing. This also happened when I repeated the migration using the GUI console. I’ll send you the wallet (maybe it’s related to the stuff I added below).

Multisigs are special because we have addmultisigaddress which add the multisig but does not watch them. All of the import rpcs will watch.

Also, IIUC along with wrapped SegWit, multisig is the only p2sh redeemscript our wallet knows how to spend.

I played around a bit with importmulti on the legacy wallet, creating a p2sh address for a simple relative timelock. I used a key that’s in the wallet, but because we don’t recognize that script type, it’s considered not solvable and watch-only. This causes it to be migrated to the watch-only wallet, away from its corresponding private key. We should probably warn about it in the documentation.

Interestingly, once we have Miniscript support, there could be cases where a (P2W)SH script goes from watch-only and non-solvable to spendable, assuming the migration script picks it up.

in test/functional/wallet_migration.py:195 in fb5e0957e0 outdated

190+        ms_info2 = multisig1.addmultisigaddress(2, [multisig1.getnewaddress(), pub1, pub2])
191+        assert_equal(multisig1.getwalletinfo()["descriptors"], False)
192+
193+        addr = ms_info["address"]
194+        txid = default.sendtoaddress(addr, 10)
195+        multisig1.importaddress(addr)

Sjors commented at 1:19 pm on August 29, 2022:

fb5e0957e066bd0825947a399242242f1f1a2ae6: why do you need to call importaddress after addmultisigaddress? Is that makes it marked as watch-only (as oppose to the second address)? Why?

(update: see above, but I still don’t understand why)

achow101 commented at 4:32 pm on August 29, 2022:

So that this particular address is being watched and will end up in the _watchonly wallet.

in src/wallet/rpc/wallet.cpp:750 in 3299574f5e outdated

745+        r.pushKV("watchonly_name", res->watchonly_wallet->GetName());
746+    }
747+    if (res->solvables_wallet) {
748+        r.pushKV("solvables_name", res->solvables_wallet->GetName());
749+    }
750+    r.pushKV("backup_path", res->backup_path);

maflcko commented at 1:21 pm on August 29, 2022:

0    r.pushKV("backup_path", res->backup_path.u8string());

C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\wallet\rpc\wallet.cpp(753,1): message : Reason: cannot convert from ‘fs::path’ to ‘const UniValue’ [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\libbitcoin_wallet\libbitcoin_wallet.vcxproj]

achow101 commented at 4:31 pm on August 29, 2022:

Done

in src/wallet/rpc/wallet.cpp:707 in 3299574f5e outdated

700@@ -701,6 +701,59 @@ RPCHelpMan simulaterawtransaction()
701     };
702 }
703 
704+static RPCHelpMan migratewallet()
705+{
706+    return RPCHelpMan{"migratewallet",
707+        "\nEXPERIMENTAL warning: This call may not work as expected and may be changed in future releases\n"

maflcko commented at 1:22 pm on August 29, 2022:

nit: No need for the \n.

0        "EXPERIMENTAL warning: This call may not work as expected and may be changed in future releases\n"

achow101 commented at 4:31 pm on August 29, 2022:

Done

in test/functional/wallet_migration.py:202 in fb5e0957e0 outdated

197+        assert_equal(multisig1.getaddressinfo(ms_info["address"])["iswatchonly"], True)
198+        assert_equal(multisig1.getaddressinfo(ms_info["address"])["solvable"], True)
199+        self.generate(self.nodes[0], 1)
200+        multisig1.gettransaction(txid)
201+        assert_equal(multisig1.getbalances()["watchonly"]["trusted"], 10)
202+        assert_equal(multisig1.getaddressinfo(ms_info2["address"])["ismine"], False)

Sjors commented at 1:22 pm on August 29, 2022:

fb5e0957e066bd0825947a399242242f1f1a2ae6 : Maybe do addr2 = ms_info2["address"] and rename addr to addr1 for clarity.

achow101 commented at 4:32 pm on August 29, 2022:

Done

in src/wallet/rpc/wallet.cpp:731 in 3299574f5e outdated

726+            + HelpExampleRpc("migratewallet", "")
727+        },
728+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
729+{
730+    std::shared_ptr<CWallet> wallet = GetWalletForJSONRPCRequest(request);
731+    if (!wallet) return NullUniValue;

maflcko commented at 1:23 pm on August 29, 2022:

nit: For new code, can use correct spacing?

0        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
1        {
2            std::shared_ptr<CWallet> wallet = GetWalletForJSONRPCRequest(request);
3            if (!wallet) return NullUniValue;

achow101 commented at 4:31 pm on August 29, 2022:

Done

maflcko commented at 1:24 pm on August 29, 2022: member

I think it is expected that you run into deadlocks, given that the thread sanitizer doesn’t pass CI.

Also, the Windows compilation fails, see u8string suggestion.

in src/wallet/rpc/wallet.cpp:900 in 3299574f5e outdated

896@@ -844,6 +897,7 @@ Span<const CRPCCommand> GetWalletRPCCommands()
897         {"wallet", &walletpassphrase},
898         {"wallet", &walletpassphrasechange},
899         {"wallet", &walletprocesspsbt},
900+        {"wallet", &migratewallet},

maflcko commented at 1:25 pm on August 29, 2022:

nit: Keep this list sorted?

achow101 commented at 4:31 pm on August 29, 2022:

Done

in test/functional/wallet_migration.py:212 in fb5e0957e0 outdated

207+        # A new wallet multisig1_watchonly is created which has the multisig address
208+        # Transaction to multisig is in multisig1_watchonly and not multisig1
209+        multisig1.migratewallet()
210+        assert_equal(multisig1.getwalletinfo()["descriptors"], True)
211+        self.assert_is_sqlite("multisig1")
212+        assert_equal(multisig1.getaddressinfo(ms_info["address"])["ismine"], False)

Sjors commented at 1:26 pm on August 29, 2022:

fb5e0957e066bd0825947a399242242f1f1a2ae6 : Maybe use addr instead of ms_info["address"]

achow101 commented at 4:32 pm on August 29, 2022:

Done

in test/functional/wallet_migration.py:227 in fb5e0957e0 outdated

222+        assert_equal(ms1_wallet_info['descriptors'], True)
223+        assert_equal(ms1_wallet_info['private_keys_enabled'], False)
224+        self.assert_is_sqlite("multisig1_watchonly")
225+        assert_equal(ms1_watchonly.getaddressinfo(ms_info["address"])["ismine"], True)
226+        assert_equal(ms1_watchonly.getaddressinfo(ms_info["address"])["solvable"], True)
227+        assert_equal(ms1_watchonly.getaddressinfo(ms_info2["address"])["ismine"], False)

Sjors commented at 1:27 pm on August 29, 2022:

fb5e0957e066bd0825947a399242242f1f1a2ae6: maybe comment here that, because it wasn’t watch-only, it doesn’t get moved to the watch-only wallet, but to the solvables wallet (as shown below in the test)

achow101 commented at 4:33 pm on August 29, 2022:

Done

Sjors commented at 1:30 pm on August 29, 2022: member

As you said:

Multisigs are special because we have addmultisigaddress which add the multisig but does not watch them.

The test could use some comments around that behavior. It correctly demonstrates it though.

Sjors commented at 2:21 pm on August 29, 2022: member

I was confused about the difference between solvable and watch-only.

solvable: anything we have any (public) keys for and we know who to spend (e.g. a multisig for which we have one public key, and we know the other two because we have the full redeemscript)
watch-only: may or may not be solvable, e.g. an unsolvable multisig is one for which we only know the address (scriptPubKey), not the full script, i.e. we don’t know which keys it needs

A solvable address is not automatically watch-only, but all import methods except importmulti ensure it is.

importmulti adds a redeemscript so it’s solvable, but it’s not marked watch-only. The net-effect of that is that we see the full script with getaddressinfo, the wallet ignores transactions to it, but we can sign them. This may be useful if you’re a co-signer and you don’t want those transactions to show up.

Sjors commented at 3:58 pm on August 29, 2022: member

I also noticed the label given by addmultisigaddress is not migrated to the solvables wallet. In fact it stays behind, as seen by getaddressinfo. The other info does move.

We should consider making the solvables wallet legacy for now. Because right now it’s just another watch-only wallet, except without transaction history. But future transaction will show up and rescan will reveal the old ones. This particular use case of having scripts but not seeing their transactions, seems a legacy feature we can’t migrate yet. And in that case, we might as well move or copy corresponding private keys over too. Unless I totally misunderstand this multisig use case, you need to keep those around.

achow101 force-pushed on Aug 29, 2022

achow101 commented at 4:35 pm on August 29, 2022: member

The issues with locks and hanging should be resolved now. I’ve changed it to just unload a wallet before migrating, then loading it but without connecting all of the signals, etc. for the migration itself. This should fix the issues with locks and hanging (which was caused by waiting for the shared_ptr to be released by the GUI).

We should consider making the solvables wallet legacy for now.

I don’t think we should. The point is to make it so that migrated wallets do not have any dependency on legacy stuff at all so that we can remove the legacy wallet. I agree that it can be confusing, but I don’t think it is helpful or less confusing to make a new legacy wallet when the user is expecting everything to go to descriptors.

Sjors commented at 5:01 pm on August 29, 2022: member

The problem is that the solvables wallet is completely useless. It can’t sign things the legacy wallet could sign. You might as well put these things in the watch-only wallet, unless I’m missing something.

Another approach could be to handle the special (and only?) case of a multisig wallet more gracefully. The best substitute we can make for how that worked before, is to create a blank descriptor wallet and then import the (inferred) descriptor with the private keys that we have. They’ll lose the “feature” of not seeing transactions, but they’ll retain the ability to sign, which is more important.

The locking issue seems resolved. It’s also no longer freezing the UI while the migration is in progress. Labels

It’s quite a slow call (several minutes), so a nice followup improvement would be a way to abort it and maybe a progress bar. It’s a one-off task, but probably one that makes users very nervous.

Notice one glitch: if you call this from GUI console, since it closes the wallet, you’ll suddenly see a different wallet selected in the console, even though the command is still in progress. That’s confusing, though maybe it’s enough for now to just to warn the user to ignore that. It also crashes if you touch the dropdown though.

achow101 commented at 5:18 pm on August 29, 2022: member

The problem is that the solvables wallet is completely useless. It can’t sign things the legacy wallet could sign. You might as well put these things in the watch-only wallet, unless I’m missing something.

The solvables wallet can update a PSBT with the UTXO, scripts, and pubkeys, for inputs that spend the any of the scripts it contains. Because the user had not watched those scripts when the wallet is a legacy wallet, it does not make sense to have them be in the watchonly wallet.

Sjors commented at 6:23 pm on August 29, 2022: member

The slowness seems to be macOS thing, or even particular to one of my machines. The exact same wallet takes ~9 minutes to upgrade on one on my macOS machines, and less than a minute on the other and on Ubuntu. When the upgrade is fast, the labels are preserved fine too.

It would be useful to know how people use(d) legacy multisig wallets, especially before PSBT. It’s not clear to me with which wallet they would craft the transaction.

A solveable-only legacy wallet can sign them and add more metadata, but because it lacks transaction history, they can’t craft them. Presumably the user would have different wallet for that purpose, which they may or may not be upgrading separately. As long as that other wallet can produce a PSBT, then our upgraded main wallet can sign it just fine. The _solvable wallet’s only purpose then is to backup the redeemscript. They should never need it.

If their other wallet can’t produce a psbt, then the upgrade process breaks their ability to sign, since the private keys are now no longer in the same wallet as the redeemscript. So we should warn about that. And we could a utility that takes a raw unsigned transaction and transform it into a PSBT to address that potential problem.

Knowing these use cases would also help us write upgrade documentation for them, to explain what new workflows they should consider.

in src/wallet/wallet.cpp:3727 in feaf85a8ad outdated

3720+            fs::remove(m_database->Filename());
3721+            assert(false); // This is a critical error, the new db could not be written to. The original db exists as a backup, but we should not continue execution.
3722+        }
3723+    }
3724+    batch->TxnCommit();
3725+    return true;

furszy commented at 8:13 pm on August 29, 2022:

TxnCommit can fail and return false (same as TxnBegin).

Same as it’s done above, could throw a critical error.

achow101 commented at 9:30 pm on August 29, 2022:

Added some asserts for this and TxnBegin.

Implement MigrateToSQLite e7b16f925a

Implement MigrateLegacyToDescriptor 0bf7b38bff

Add migratewallet RPC 31764c3f87

descriptors: addr() and raw() should return false for ToPrivateString

They don't have any private data and they can't be nested so they
should return false for ToPrivateString.

0b26e7cdf2

Test migratewallet

Co-Authored-By: furszy <matiasfurszyfer@protonmail.com>

9c44bfe244

doc: Release notes and other docs for migration 53e7ed075c

achow101 force-pushed on Aug 29, 2022

Sjors commented at 11:57 am on August 31, 2022: member

tACK 53e7ed075c49f853cc845afc7b2f058cabad0cb0

I think this is good enough for an experimental RPC. Hopefully we can figure out why some macOS machines have difficulty with the RPC, but if not, we can just mention that in the release notes. Similarly we can revisit the way solvable multisig is handled; anyone using that feature will know how to restore the backup.

w0xlt approved

w0xlt commented at 1:28 am on September 1, 2022: contributor

reACK https://github.com/bitcoin/bitcoin/commit/53e7ed075c49f853cc845afc7b2f058cabad0cb0

achow101 merged this on Sep 1, 2022

achow101 closed this on Sep 1, 2022

sidhujag referenced this in commit f32f4a4667 on Sep 1, 2022

hebasto referenced this in commit d190003700 on Sep 14, 2022

in src/wallet/wallet.cpp:3835 in 0bf7b38bff outdated

3830+    // Check the address book data in the same way we did for transactions
3831+    std::vector<CTxDestination> dests_to_delete;
3832+    for (const auto& addr_pair : m_address_book) {
3833+        // Labels applied to receiving addresses should go based on IsMine
3834+        if (addr_pair.second.purpose == "receive") {
3835+            if (!IsMine(addr_pair.first)) {

ryanofsky commented at 9:25 pm on March 13, 2023:

In commit “Implement MigrateLegacyToDescriptor” (0bf7b38bff422e7413bcd3dc0abe2568dd918ddc)

It seems like this should say IsMine not !IsMine. Also I’m not sure why addr_pair.second.purpose == "receive" check is necessary. It seems like just checking IsMine should be enough, and it would be less robust to rely on purpose field being present.

furszy commented at 9:40 pm on March 13, 2023:

It’s because the migration process divides data between different wallets. It first deletes and unloads the legacy spkm from the main wallet (check the beginning of the function), and setup new descriptors. Then moves the data to two possible wallets; a solvable and a watch-only wallet.

So the !IsMine means that the record in the addressbook is from the legacy spkm (not loaded anymore), so it needs migration to the new wallets.

It also took me a while to get it while was rewriting it for #26836. (sorry for the double PR mention, saw late your response in the other PR)

bitcoin locked this on Mar 12, 2024

wallet: Migrate legacy wallets to descriptor wallets #19602

Conflicts