wallet: be able to specify a wallet name and passphrase to migratewallet

achow101 commented at 11:46 pm on November 28, 2022: member

migratewallet currently operates on wallets that are already loaded, however this is not necessarily required, and in the future, not possible once the legacy wallet is removed. So we need to also be able to give the wallet name to migrate.

Additionally, the passphrase is required when migrating a wallet. Since a wallet may not be loaded when we migrate, and as we currently unload wallets when migrating, we need the passphrase to be given to migratewallet in order to migrate encrypted wallets.

Fixes #27048

DrahtBot commented at 11:46 pm on November 28, 2022: 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	john-moffett
Stale ACK	w0xlt, furszy, ishaanam

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:

#26740 (wallet: Migrate wallets that are not in a wallet dir by achow101)
#26596 (wallet: Migrate legacy wallets to descriptor wallets without requiring BDB 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 Nov 28, 2022

achow101 force-pushed on Nov 28, 2022

achow101 force-pushed on Nov 29, 2022

DrahtBot added the label Needs rebase on Dec 1, 2022

achow101 force-pushed on Dec 1, 2022

achow101 marked this as ready for review on Dec 1, 2022

DrahtBot removed the label Needs rebase on Dec 1, 2022

in src/wallet/wallet.cpp:4171 in 3aa48c855d outdated

4111@@ -4112,26 +4112,25 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error,
4112 
4113 util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>&& wallet, WalletContext& context)
4114 {
4115-    MigrationResult res;
4116     bilingual_str error;

ishaanam commented at 1:18 am on December 6, 2022:

In 3aa48c855d05dc8f7dd43adc017b408064634f17 “wallet: Allow MigrateLegacyToDescriptor to take a wallet name”

This error isn’t used here, it can be removed.

achow101 commented at 7:54 pm on December 13, 2022:

It’s being used by MakeWalletDatabase.

ishaanam commented at 8:29 pm on December 13, 2022:

I don’t think that MakeWalletDatabase is directly run in this function, the error on src/wallet/wallet.cpp:4131 is used for that.

achow101 commented at 8:42 pm on December 13, 2022:

Ah, was looking at the wrong line. Removed.

in src/wallet/rpc/wallet.cpp:753 in 433f9f8d75 outdated

738+            if (GetWalletNameFromJSONRPCRequest(request, wallet_name)) {
739+                if (!(request.params[0].isNull() || request.params[0].get_str() == wallet_name)) {
740+                    throw JSONRPCError(RPC_INVALID_PARAMETER, "RPC endpoint wallet and wallet_name parameter specify different wallets");
741+                }
742+            } else {
743+                wallet_name = request.params[0].get_str();

ishaanam commented at 1:31 am on December 6, 2022:

In 433f9f8d7539efc26b952063d6127315606d248e “rpc: Allow users to specify wallet name for migratewallet”

An error should be thrown if no RPC endpoint wallet is specified and the wallet_name parameter isn’t present. Otherwise the following error is thrown which is vague:

JSONRPCException: JSON value of type null is not of expected type string (-3)

achow101 commented at 8:02 pm on December 13, 2022:

Added an error.

in src/wallet/rpc/wallet.cpp:760 in 433f9f8d75 outdated

752+                }
753+
754+                res = MigrateLegacyToDescriptor(std::move(wallet), context);
755+            } else {
756+                res = MigrateLegacyToDescriptor(wallet_name, context);
757+            }

ishaanam commented at 3:18 am on December 6, 2022:

In 433f9f8d7539efc26b952063d6127315606d248e “rpc: Allow users to specify wallet name for migratewallet”

These lines don’t seem to have any test coverage. Also, if GetWallet is unable to get the wallet, shouldn’t an error be thrown at that point?

achow101 commented at 8:05 pm on December 13, 2022:

These lines don’t seem to have any test coverage.

Tests are added in a later commit.

Also, if GetWallet is unable to get the wallet, shouldn’t an error be thrown at that point?

No, GetWallet only looks for wallets that have been loaded. But it is necessary to be able to migrate wallets that are not currently loaded. MigrateLegacyToDescriptor will try loading the wallet and result in an error if it does not actually exist.

ishaanam commented at 3:43 am on December 6, 2022: contributor

Concept ACK

achow101 force-pushed on Dec 13, 2022

in test/functional/wallet_migration.py:434 in 424ee3e242 outdated

431+        bals = wallet.getbalances()
432+
433+        wallet.unloadwallet()
434+
435+        assert_raises_rpc_error(-8, "RPC endpoint wallet and wallet_name parameter specify different wallets", wallet.migratewallet, "someotherwallet")
436+        assert_raises_rpc_error(-8, "Either RPC endpoint wallet or wallet_name parameter must be used", self.nodes[0].migratewallet)

ishaanam commented at 10:53 pm on December 15, 2022:

To fix failing CI:

0        assert_raises_rpc_error(-8, "Either RPC endpoint wallet or wallet_name parameter must be provided", self.nodes[0].migratewallet)

w0xlt commented at 0:06 am on December 16, 2022:

This fixes the CI error.

achow101 commented at 5:40 pm on December 16, 2022:

Fixed.

w0xlt commented at 0:09 am on December 16, 2022: contributor

Approach ACK

achow101 force-pushed on Dec 16, 2022

w0xlt approved

w0xlt commented at 6:00 pm on December 16, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/26595/commits/8d957e3b2f56f07d1c0af1c843d396cc29aecb94

ishaanam commented at 2:47 am on December 20, 2022: contributor

ACK 8d957e3b2f56f07d1c0af1c843d396cc29aecb94

in src/util/result.h:44 in fae51d77c0 outdated

40@@ -41,6 +41,7 @@ class Result
41     friend bilingual_str ErrorString(const Result<FT>& result);
42 
43 public:
44+    Result() = default;

furszy commented at 12:30 pm on January 18, 2023:

In fae51d77:

I’m not so sure about this, an empty Result is currently equal to Error{} but it’s not defined anywhere. It might change in the future if we move away from using a variant.

Would rather delete the empty constructor and force us to always set either the Error or the “good” object.

achow101 commented at 10:36 pm on January 24, 2023:

Removed

in src/wallet/wallet.h:1010 in b44342298a outdated

1005@@ -1006,8 +1006,8 @@ struct MigrationResult {
1006 };
1007 
1008 //! Do all steps to migrate a legacy wallet to a descriptor wallet
1009-util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>&& wallet, WalletContext& context);
1010-util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& wallet_name, WalletContext& context);
1011+util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>&& wallet, SecureString passphrase, WalletContext& context);
1012+util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& wallet_name, SecureString passphrase, WalletContext& context);

furszy commented at 11:20 pm on January 18, 2023:

could be a const ref to avoid a copy.

achow101 commented at 10:36 pm on January 24, 2023:

Done

in src/wallet/wallet.cpp:4230 in b44342298a outdated

4160@@ -4161,6 +4161,9 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
4161     {
4162         LOCK(local_wallet->cs_wallet);
4163 
4164+        // Unlock the wallet
4165+        local_wallet->Unlock(passphrase);
4166+

furszy commented at 11:35 pm on January 18, 2023:

Could use the Unlock return value to know whether the passphrase is ok or not (failing early if not).

0// Unlock the wallet if needed
1if (local_wallet->IsLocked() && !local_wallet->Unlock(passphrase)) {
2    return util::Error{Untranslated("Error: Wallet decryption failed, please provide the correct wallet's passphrase")};
3}

atm, this fails after doing the whole bdb -> sqlite migration.

achow101 commented at 10:36 pm on January 24, 2023:

Done

furszy commented at 0:04 am on January 19, 2023: member

Approach ACK, some comments:

Thinking that the MigrateLegacyToDescriptor overload isn’t really needed, we only call this function from a single place which always receives a wallet name/path. Could instead decouple the “UnloadWalletForMigration” process into a separate function, and call it at the beginning of the migration process if GetWallet(wallet_name) returns a wallet. e.g. https://github.com/furszy/bitcoin-core/commit/456780b255af490a6dd44e710184cf85106f5390 This would let us remove the added Result default constructor too.
Would be good to add a test for a wallet migration by absolute path (the “notloaded” test provides the wallet dir name and relies on the wallet internals to detect it as a directory inside the wallets dir). e.g.

0# Test migration by absolute path
1wallet_file_path = os.path.join(self.nodes[0].datadir, "regtest/wallets", "notloaded")
2self.nodes[0].migratewallet(wallet_file_path)
3# check migration..

achow101 force-pushed on Jan 24, 2023

achow101 commented at 10:36 pm on January 24, 2023: member

Thinking that the MigrateLegacyToDescriptor overload isn’t really needed, we only call this function from a single place which always receives a wallet name/path. Could instead decouple the “UnloadWalletForMigration” process into a separate function, and call it at the beginning of the migration process if GetWallet(wallet_name) returns a wallet. e.g. furszy@456780b This would let us remove the added Result default constructor too.

Done as suggested

Would be good to add a test for a wallet migration by absolute path (the “notloaded” test provides the wallet dir name and relies on the wallet internals to detect it as a directory inside the wallets dir).

Added

achow101 commented at 6:16 pm on January 25, 2023: member

Rebased for a silent merge conflict.

achow101 force-pushed on Jan 25, 2023

furszy approved

furszy commented at 4:59 pm on January 26, 2023: member

Code review ACK 7ebfd51

achow101 force-pushed on Feb 2, 2023

achow101 commented at 9:46 pm on February 2, 2023: member

Rebased for another silent merge conflict.

furszy approved

furszy commented at 10:12 pm on February 2, 2023: member

rebase diff ACK ee620e99

achow101 commented at 5:29 am on February 6, 2023: member

Added a commit to update the help text about encrypted wallets.

furszy approved

furszy commented at 1:23 pm on February 6, 2023: member

ACK 3a6965d5

in test/functional/wallet_migration.py:424 in 3a6965d560 outdated

416+        assert_equal(info["descriptors"], True)
417+        assert_equal(info["format"], "sqlite")
418+        wallet.gettransaction(txid)
419+
420+        assert_equal(bals, wallet.getbalances())
421+

ishaanam commented at 6:20 pm on February 7, 2023:

In ee620e999a8ce269a90eee15b25988428e7620fd " tests: Tests for migrating wallets by name, and providing passphrase " It would be useful to also test the behavior of the wallet encryption post-migration (eg. that migratewallet re-locks the wallet). A test for when the passphrase is provided but is incorrect could also be added here. A test for when the wallet name is provided but is incorrect could also be added.

achow101 commented at 6:38 pm on February 16, 2023:

It would be useful to also test the behavior of the wallet encryption post-migration (eg. that migratewallet re-locks the wallet).

Added a check for that.

A test for when the passphrase is provided but is incorrect could also be added here.

Done

A test for when the wallet name is provided but is incorrect could also be added.

There’s a test for that already in test_unloaded.

fanquake commented at 11:22 am on February 8, 2023: member

Looks like this should also be backported to 24.x?

fanquake requested review from w0xlt on Feb 8, 2023

fanquake removed review request from w0xlt on Feb 8, 2023

fanquake requested review from ishaanam on Feb 8, 2023

fanquake requested review from john-moffett on Feb 8, 2023

fanquake added the label Needs backport (24.x) on Feb 8, 2023

in src/wallet/rpc/wallet.cpp:760 in 3a6965d560 outdated

761-            WalletContext& context = EnsureWalletContext(request.context);
762+            // Note that the walletpassphrase is stored in request.params[1] which is not mlock()ed
763+            SecureString wallet_pass;
764+            wallet_pass.reserve(100);
765+            // TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string)
766+            // Alternately, find a way to make request.params[0] mlock()'d to begin with.

john-moffett commented at 9:02 pm on February 8, 2023:

Nit: request.params[1]

in src/wallet/rpc/wallet.cpp:761 in 3a6965d560 outdated

762+            // Note that the walletpassphrase is stored in request.params[1] which is not mlock()ed
763+            SecureString wallet_pass;
764+            wallet_pass.reserve(100);
765+            // TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string)
766+            // Alternately, find a way to make request.params[0] mlock()'d to begin with.
767+            wallet_pass = request.params[1].isNull() ? "" : request.params[1].get_str().c_str();

john-moffett commented at 9:28 pm on February 8, 2023:

Possibly for another PR (and possibly a dumb question), but why can’t we do

0if (!request.params[1].isNull()) wallet_pass.assign(request.params[1].get_str());

rather than use .c_str() (and in the other existing places)? This is supported since C++17.

achow101 commented at 6:37 pm on February 16, 2023:

I’ve updated the passphrase handling to match what #27068 does

john-moffett approved

john-moffett commented at 9:29 pm on February 8, 2023: contributor

tACK 3a6965d5606bb8e8f750341dfd6dac12aa7d4870

in src/wallet/wallet.cpp:4174 in ca9bc8a222 outdated

4188-    res.backup_path = backup_path;
4189-
4190-    // Unload the wallet so that nothing else tries to use it while we're changing it
4191-    if (!RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt, warnings)) {
4192-        return util::Error{_("Unable to unload the wallet before migrating")};
4193+    // If the wallet is still loaded, unload it wallet so that nothing else tries to use it while we're changing it

ishaanam commented at 2:25 am on February 11, 2023:

nit:

0    // If the wallet is still loaded, unload it so that nothing else tries to use it while we're changing it

achow101 commented at 6:39 pm on February 16, 2023:

Fixed

ishaanam commented at 4:07 am on February 11, 2023: contributor

reACK 3a6965d5606bb8e8f750341dfd6dac12aa7d4870

Currently if the wallet_name does not specify an actual wallet, the following error is thrown:

Wallet file verification failed. Failed to load database path ‘/tmp/bitcoin_func_test_w3iehdpt/node0/regtest/wallets/wrong_wallet_name’. Path does not exist. (-4)

It would make more sense to throw a simpler error from migratewallet (as opposed to from MakeDatabase). This along with the additional test coverage I have suggested below could be implemented in this PR or as a follow-up.

DrahtBot requested review from w0xlt on Feb 11, 2023

achow101 force-pushed on Feb 16, 2023

wallet: Allow MigrateLegacyToDescriptor to take a wallet name

An overload of MigrateLegacyToDescriptor is added which takes the wallet
name. The original that took a wallet pointer is still available, it
just gets the name, closes the wallet, and calls the new overload.

dbfa345403

rpc: Allow users to specify wallet name for migratewallet 6bdbc5ff59

wallet: Be able to unlock the wallet for migration

Since migration reloads the wallet, the wallet will always be locked
unless the passphrase is given. migratewallet can now take the
passphrase in order to unlock the wallet for migration.

7fd125b27d

achow101 force-pushed on Feb 16, 2023

achow101 commented at 6:40 pm on February 16, 2023: member

Wallet file verification failed. Failed to load database path ‘/tmp/bitcoin_func_test_w3iehdpt/node0/regtest/wallets/wrong_wallet_name’. Path does not exist. (-4)

It would make more sense to throw a simpler error from migratewallet (as opposed to from MakeDatabase).

This is the same error that we currently give for loadwallet. I’m going to leave it as is so these are consistent with each other, but I agree it could be improved.

fanquake removed review request from w0xlt on Feb 20, 2023

fanquake requested review from john-moffett on Feb 20, 2023

fanquake requested review from furszy on Feb 20, 2023

fanquake commented at 5:19 pm on February 20, 2023: member

@pinheadmz review?

furszy approved

furszy commented at 7:18 pm on February 20, 2023: member

rebase diff ACK 3e521b2e

DrahtBot requested review from ishaanam on Feb 20, 2023

DrahtBot requested review from w0xlt on Feb 20, 2023

john-moffett approved

john-moffett commented at 9:00 pm on February 20, 2023: contributor

ACK 3e521b2eb13799e11866562cfda4dc0bd3f7d6b9

ishaanam commented at 7:09 am on February 21, 2023: contributor

reACK 3e521b2eb13799e11866562cfda4dc0bd3f7d6b9

in test/functional/wallet_migration.py:460 in 3e521b2eb1 outdated

457+        self.generate(self.nodes[0], 1)
458+        bals = wallet.getbalances()
459+
460+        wallet.unloadwallet()
461+
462+        wallet_file_path = os.path.join(self.nodes[0].datadir, "regtest", "wallets", "noloaded2")

pinheadmz commented at 7:36 pm on February 21, 2023:

I think you are creating a wallet with the letter “t” in the name, but the path you pass to migratewallet has no “t”:

notloaded2 vs noloaded2

I think this isn’t causing test failure because test_unloaded_by_path() is not added to the run_test list …?

furszy commented at 7:39 pm on February 21, 2023:

yeah, the test case is not being called. Nice catch.

achow101 commented at 8:45 pm on February 21, 2023:

Nice catch! Fixed.

achow101 force-pushed on Feb 21, 2023

tests: Tests for migrating wallets by name, and providing passphrase aaf02b5721

wallet, rpc: Update migratewallet help text for encrypted wallets 9486509be6

achow101 force-pushed on Feb 21, 2023

fanquake removed review request from w0xlt on Feb 22, 2023

fanquake requested review from pinheadmz on Feb 22, 2023

fanquake removed review request from pinheadmz on Feb 22, 2023

fanquake requested review from john-moffett on Feb 22, 2023

john-moffett approved

john-moffett commented at 2:39 pm on February 22, 2023: contributor

reACK 9486509be65f09174a0cb50a337cac58a0c09de4

DrahtBot requested review from furszy on Feb 22, 2023

DrahtBot requested review from w0xlt on Feb 22, 2023

pinheadmz approved

pinheadmz commented at 3:07 pm on February 22, 2023: member

ACK 9486509be65f09174a0cb50a337cac58a0c09de4

reviewed code, ran tests, tested on regtest with cli commands

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 9486509be65f09174a0cb50a337cac58a0c09de4
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmP2L3gACgkQ5+KYS2KJ
 7yToaJA/9GCc9Y84y5GfTPsmAUUYV/jFKr5Z9lDIATQca+Ou323CQ76CF00IQiyBB
 8ECtPlFJSPxaaKac4k5QmwT4BA4EgzcTKKVhjCVlorHyJQYk5yYBD5xOk2G0fPthS
 9VA3CkmikXrpb7YGcxiKVvDLrOQ3h16+S2CI6/DViChgJ32cGbJ+WjZsqIb0Mx6tR
10C7AtMPMjmjzt+oAbuzIhGzCfjglygUngGSIZbIotjRwgQ75ANJEApLYbUNyddz4a
116Pv9+sArKgvkNPzeOMxUbwjR3esTEAR2k70JpnZlxsf3alu6Jp6a8l7XkxZDoh5/
12xyT/jdyHCqCiD0ZFpEAc2q4chFZgXzoMr7dcXesSiHzf64F8TStlG7ZKJn0EAPhG
13Esal14rEIXh7OHVD5NHF1qOviGgzhrXBmkecbsX2jj0pUpcXg/cqH9Ln0bzYc//b
14WBrc8pe1QPijzOlSXiEsHff3eDB8PWgK6uIgLld0y5bOgQ7YD6iex+exROzNcXK6
15i5s8hJ8BNR0/sPWUt5zRA+BJ/pdQiHaJ3e1H2/X+UOkQuCKkLsyh1cLf0BI6Y9uJ
16tUzgd3XDjh9wAUDPzf16o0mbOHzClK/6ak+wv6+WlubdCQXFMgOJdS3LWdDWgIE0
17vwJluifh8hWN+vraJcM2ctDGL4KtdAndJ/UyznTXLBuBhhkmNXI=
18=iPeZ
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

furszy commented at 4:03 pm on February 22, 2023: member

ACK 9486509b

fanquake merged this on Feb 22, 2023

fanquake closed this on Feb 22, 2023

sidhujag referenced this in commit c88c21671f on Feb 25, 2023

fanquake referenced this in commit 648b06256d on Feb 27, 2023

fanquake referenced this in commit 50dd8b13df on Feb 27, 2023

fanquake referenced this in commit ccc72fecd7 on Feb 27, 2023

fanquake referenced this in commit debcfe313a on Feb 27, 2023

fanquake referenced this in commit 784a754aa4 on Feb 27, 2023

fanquake removed the label Needs backport (24.x) on Feb 27, 2023

fanquake commented at 2:21 pm on February 27, 2023: member

Added to #26878 for backporting to 24.x.

glozow referenced this in commit c8c85ca16e on Feb 27, 2023

bitcoin locked this on Feb 27, 2024

wallet: be able to specify a wallet name and passphrase to migratewallet #26595

Reviews

Conflicts