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

achow101 · 2022-11-28T23:46:09Z

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 · 2022-11-28T23:46:12Z

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.

ishaanam

Concept ACK

src/wallet/wallet.cpp

src/wallet/rpc/wallet.cpp

ishaanam · 2022-12-06T03:18:19Z

src/wallet/rpc/wallet.cpp

+            } else {
+                res = MigrateLegacyToDescriptor(wallet_name, context);
+            }


In 433f9f8 "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?

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.

test/functional/wallet_migration.py

w0xlt

Approach ACK

w0xlt

ACK 8d957e3

ishaanam

ACK 8d957e3

furszy

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. furszy@456780b
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.

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

src/util/result.h

src/wallet/wallet.h

src/wallet/wallet.cpp

achow101 · 2023-01-24T22:36:15Z

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 · 2023-01-25T18:16:41Z

Rebased for a silent merge conflict.

john-moffett

ACK 3e521b2

ishaanam · 2023-02-21T07:09:48Z

reACK 3e521b2

test/functional/wallet_migration.py

john-moffett

reACK 9486509

pinheadmz

ACK 9486509

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

Show Signature

pinheadmz's public key is on keybase

furszy

ACK 9486509

…sphrase to migratewallet 9486509 wallet, rpc: Update migratewallet help text for encrypted wallets (Andrew Chow) aaf02b5 tests: Tests for migrating wallets by name, and providing passphrase (Andrew Chow) 7fd125b wallet: Be able to unlock the wallet for migration (Andrew Chow) 6bdbc5f rpc: Allow users to specify wallet name for migratewallet (Andrew Chow) dbfa345 wallet: Allow MigrateLegacyToDescriptor to take a wallet name (Andrew Chow) Pull request description: `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 bitcoin#27048 ACKs for top commit: john-moffett: reACK 9486509 pinheadmz: ACK 9486509 furszy: ACK 9486509 Tree-SHA512: 35e2ba69a148e129a41e20d7fb99c4cab7947b1b7e7c362f4fd06ff8ac6e79e476e07207e063ba5b80e1a33e2343f4b4f1d72d7930ce80c34571c130d2f5cff4

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. Github-Pull: bitcoin#26595 Reabsed-From: dbfa345

Github-Pull: bitcoin#26595 Rebased-From: 6bdbc5f

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. Github-Pull: bitcoin#26595 Rebased-From: 7fd125b

Github-Pull: bitcoin#26595 Rebased-From: aaf02b5

Github-Pull: bitcoin#26595 Rebased-From: 9486509

fanquake · 2023-02-27T14:21:23Z

Added to #26878 for backporting to 24.x.

784a754 wallet, rpc: Update migratewallet help text for encrypted wallets (Andrew Chow) debcfe3 tests: Tests for migrating wallets by name, and providing passphrase (Andrew Chow) ccc72fe wallet: Be able to unlock the wallet for migration (Andrew Chow) 50dd8b1 rpc: Allow users to specify wallet name for migratewallet (Andrew Chow) 648b062 wallet: Allow MigrateLegacyToDescriptor to take a wallet name (Andrew Chow) ab3bd45 i2p: use consistent number of tunnels with i2pd and Java I2P (Vasil Dimov) 29cdf42 i2p: lower the number of tunnels for transient sessions (Vasil Dimov) 5027e93 i2p: reuse created I2P sessions if not used (Vasil Dimov) a62c541 wallet: reuse change dest when recreating TX with avoidpartialspends (Matthew Zipkin) 64e7db6 Zero out wallet master key upon lock (John Moffett) b7e242e Correctly limit overview transaction list (John Moffett) cff6718 depends: fix systemtap download URL (fanquake) 7cf73df Add missing includes to fix gcc-13 compile error (MarcoFalke) 07397cd addrdb: Only call Serialize() once (Martin Zumsande) 91f83db hash: add HashedSourceWriter (Martin Zumsande) 5c824ac For feebump, ignore abandoned descendant spends (John Moffett) 428dcd5 wallet: Skip rescanning if wallet is more recent than tip (Andrew Chow) cbcdafa test: wallet: check that labels are migrated to watchonly wallet (Sebastian Falbesoner) 342abfb wallet: fully migrate address book entries for watchonly/solvable wallets (Sebastian Falbesoner) Pull request description: Backports: * #26595 * #26675 * #26679 * #26761 * #26837 * #26909 * #26924 * #26944 * bitcoin-core/gui#704 * #27053 * #27080 ACKs for top commit: instagibbs: ACK 784a754 achow101: ACK 784a754 hebasto: ACK 784a754, I've made backporting locally and got a diff between my branch and this PR as follows: Tree-SHA512: 8ea84aa02d7907ff1e202e1302b441ce9ed2198bf383620ad40056a5d7e8ea88e1047abef0b92d85648016bf9b3195c974be3806ccebd85bef4f85c326869e43

DrahtBot added the Wallet label Nov 28, 2022

achow101 mentioned this pull request Nov 28, 2022

wallet: Migrate legacy wallets to descriptor wallets without requiring BDB #26596

Merged

achow101 force-pushed the migratewallet-by-name-and-passphrase branch 4 times, most recently from 486ad67 to d0b8bc9 Compare November 29, 2022 19:35

DrahtBot mentioned this pull request Nov 29, 2022

wallet: Avoid a segfault in migratewallet failure cleanup #26594

Merged

achow101 force-pushed the migratewallet-by-name-and-passphrase branch from d0b8bc9 to fd6cdea Compare November 29, 2022 21:25

This was referenced Nov 30, 2022

refactor: Use util::Result class for wallet loading #25722

Draft

refactor: Add util::Result failure values, multiple error and warning messages #25665

Open

DrahtBot added the Needs rebase label Dec 1, 2022

achow101 force-pushed the migratewallet-by-name-and-passphrase branch from fd6cdea to 7c1d18a Compare December 1, 2022 18:52

achow101 marked this pull request as ready for review December 1, 2022 18:52

DrahtBot removed the Needs rebase label Dec 1, 2022

ishaanam reviewed Dec 6, 2022

View reviewed changes

achow101 force-pushed the migratewallet-by-name-and-passphrase branch 2 times, most recently from 513debb to 424ee3e Compare December 13, 2022 20:42

ishaanam reviewed Dec 15, 2022

View reviewed changes

test/functional/wallet_migration.py Outdated Show resolved Hide resolved

w0xlt reviewed Dec 16, 2022

View reviewed changes

achow101 force-pushed the migratewallet-by-name-and-passphrase branch from 424ee3e to 8d957e3 Compare December 16, 2022 17:40

w0xlt approved these changes Dec 16, 2022

View reviewed changes

ishaanam reviewed Dec 20, 2022

View reviewed changes

DrahtBot mentioned this pull request Dec 22, 2022

wallet: Migrate wallets that are not in a wallet dir #26740

Merged

DrahtBot mentioned this pull request Jan 18, 2023

wallet: migrate wallet, exit early if no legacy data exist #26910

Merged

furszy reviewed Jan 19, 2023

View reviewed changes

src/util/result.h Outdated Show resolved Hide resolved

src/wallet/wallet.h Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jan 21, 2023

refactor: Replace std::optional<bilingual_str> with util::Result #25977

Merged

achow101 force-pushed the migratewallet-by-name-and-passphrase branch from 8d957e3 to 9ec4b11 Compare January 24, 2023 22:35

DrahtBot requested review from ishaanam and w0xlt February 20, 2023 19:18

john-moffett approved these changes Feb 20, 2023

View reviewed changes

pinheadmz reviewed Feb 21, 2023

View reviewed changes

test/functional/wallet_migration.py Outdated Show resolved Hide resolved

achow101 force-pushed the migratewallet-by-name-and-passphrase branch from 3e521b2 to a0a9038 Compare February 21, 2023 20:45

achow101 added 2 commits February 21, 2023 15:51

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

aaf02b5

wallet, rpc: Update migratewallet help text for encrypted wallets

9486509

achow101 force-pushed the migratewallet-by-name-and-passphrase branch from a0a9038 to 9486509 Compare February 21, 2023 20:51

fanquake requested review from pinheadmz and john-moffett and removed request for w0xlt and pinheadmz February 22, 2023 09:29

john-moffett approved these changes Feb 22, 2023

View reviewed changes

DrahtBot requested review from furszy and w0xlt February 22, 2023 14:39

pinheadmz approved these changes Feb 22, 2023

View reviewed changes

furszy reviewed Feb 22, 2023

View reviewed changes

fanquake merged commit 63893d5 into bitcoin:master Feb 22, 2023

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Feb 27, 2023

rpc: Allow users to specify wallet name for migratewallet

50dd8b1

Github-Pull: bitcoin#26595 Rebased-From: 6bdbc5f

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Feb 27, 2023

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

debcfe3

Github-Pull: bitcoin#26595 Rebased-From: aaf02b5

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Feb 27, 2023

wallet, rpc: Update migratewallet help text for encrypted wallets

784a754

Github-Pull: bitcoin#26595 Rebased-From: 9486509

fanquake mentioned this pull request Feb 27, 2023

[24.x] Backports #26878

Merged

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

bitcoin locked and limited conversation to collaborators Feb 27, 2024

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

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

Uh oh!

Conversation

achow101 commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

ishaanam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ishaanam Dec 6, 2022

Choose a reason for hiding this comment

Uh oh!

achow101 Dec 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

ishaanam left a comment

Choose a reason for hiding this comment

Uh oh!

furszy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

achow101 commented Jan 24, 2023

Uh oh!

achow101 commented Jan 25, 2023

Uh oh!

john-moffett left a comment

Choose a reason for hiding this comment

Uh oh!

ishaanam commented Feb 21, 2023

Uh oh!

Uh oh!

john-moffett left a comment

Choose a reason for hiding this comment

Uh oh!

pinheadmz left a comment

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Feb 27, 2023

Uh oh!

Uh oh!

achow101 commented Nov 28, 2022 •

edited

Loading

DrahtBot commented Nov 28, 2022 •

edited

Loading

furszy left a comment •

edited

Loading