wallet: migrate wallet, exit early if no legacy data exist by furszy · Pull Request #26910 · bitcoin/bitcoin

furszy · 2023-01-17T19:00:07Z

The process first creates a backup file then return an error,
without removing the recently created file, when notices that
the db is already running sqlite.

DrahtBot · 2023-01-17T19:00:10Z

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, achow101, ishaanam
Concept ACK	theStack

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:

#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.

john-moffett

Concept ACK. Tested with and without changes and works as described.

john-moffett · 2023-01-17T21:01:59Z

I wonder if the message is consistent with how the migration code actually works. For instance, the error returned before this change is "This wallet already uses SQLite":

bitcoin/src/wallet/wallet.cpp

Lines 3765 to 3774 in fbe5e12

bool CWallet::MigrateToSQLite(bilingual_str& error)

{

AssertLockHeld(cs_wallet);

WalletLogPrintf("Migrating wallet storage database from BerkeleyDB to SQLite.\n");

if (m_database->Format() == "sqlite") {

error = _("Error: This wallet already uses SQLite");

return false;

}

Even if it passed that check (which I believe is impossible in this situation), it does the same check as in this PR and reads "This wallet is already a descriptor wallet":

bitcoin/src/wallet/wallet.cpp

Lines 3837 to 3845 in fbe5e12

std::optional<MigrationData> CWallet::GetDescriptorsForLegacy(bilingual_str& error) const

{

AssertLockHeld(cs_wallet);

LegacyScriptPubKeyMan* legacy_spkm = GetLegacyScriptPubKeyMan();

if (!legacy_spkm) {

error = _("Error: This wallet is already a descriptor wallet");

return std::nullopt;

}

Minor additional nit: I think the RPC error messages typically start with a capital letter after the Error: unless it's an RPC command (as in line 745 above).

I wonder if the message is consistent with how the migration code actually works. For instance, the error returned before this change is "This wallet already uses SQLite"

well, think about the command purpose. If there is no legacy data to migrate then why the command should be executed at all? Doesn't really care if the db is sqlite, bdb or anything else.

Minor additional nit: I think the RPC error messages typically start with a capital letter after the Error: unless it's an RPC command (as in line 745 above).

Seems that we have a mix of them, some errors start with a capital letter, some others not (like the encrypt.cpp file). I'm more aligned to this second form but have no real preference for any. Maybe the best move would be to open an issue an align all of them, in one way or another, at once.

If there is no legacy data to migrate then why the command should be executed at all?

Right, I agree that the message makes logical sense, though I do worry that it suggests that someone can have a descriptor wallet with legacy data, or they may think that "legacy" addresses are "legacy data", like getnewaddress -addresstype legacy.

My personal preference is to copy the Error: This wallet is already a descriptor wallet message, since that better matches the help migratewallet message: Migrate the wallet to a descriptor wallet.

That said, it's not a strong preference. Maybe @achow101 could weigh in?

Right, I agree that the message makes logical sense, though I do worry that it suggests that someone can have a descriptor wallet with legacy data

It's theoretically possible to have a descriptor wallet using BDB for the database as well as a legacy wallet using sqlite. These are unsupported configurations, but are allowed and possible to achieve using the wallettool. That being said, they are unsupported so I think it's fine, and the number of people who would have such wallets should be limited to a very small number of developers.

Regarding the error message, I would also prefer the "already a descriptor wallet" wording.

pushed.
Moved the GetDescriptorsForLegacy error up to the top of the process to not duplicate it.

otherwise the process will create a backup file then return an error when notices that the db is already running sqlite.

theStack

Concept ACK

john-moffett · 2023-01-18T19:12:37Z

ACK 6d31900

Tested and code review.

achow101 · 2023-01-24T19:27:49Z

ACK 6d31900

ishaanam

crACK 6d31900

…data exist 6d31900 wallet: migrate wallet, exit early if no legacy data exist (furszy) Pull request description: The process first creates a backup file then return an error, without removing the recently created file, when notices that the db is already running sqlite. ACKs for top commit: john-moffett: ACK 6d31900 achow101: ACK 6d31900 ishaanam: crACK 6d31900 Tree-SHA512: 9fb52e80de96e129487ab91bef13647bc4570a782003b1e37940e2a00ca26283fd24ad39bdb63a984ae0a56140b518fd0d74aa2fc59ab04405b2c179b7d3c54a

DrahtBot added the Wallet label Jan 17, 2023

john-moffett reviewed Jan 17, 2023

View reviewed changes

This was referenced Jan 18, 2023

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

Merged

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

Merged

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

6d31900

otherwise the process will create a backup file then return an error when notices that the db is already running sqlite.

furszy force-pushed the 2022_wallet_prevent_descriptors_migration branch from 412a9aa to 6d31900 Compare January 18, 2023 16:47

theStack reviewed Jan 18, 2023

View reviewed changes

DrahtBot mentioned this pull request Jan 21, 2023

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

Draft

ishaanam reviewed Feb 1, 2023

View reviewed changes

achow101 merged commit fdd363e into bitcoin:master Feb 1, 2023

furszy deleted the 2022_wallet_prevent_descriptors_migration branch May 27, 2023 01:45

bitcoin locked and limited conversation to collaborators May 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

wallet: migrate wallet, exit early if no legacy data exist#26910
achow101 merged 1 commit into
bitcoin:masterfrom
furszy:2022_wallet_prevent_descriptors_migration

furszy commented Jan 17, 2023

Uh oh!

DrahtBot commented Jan 17, 2023 •

edited

Loading

Uh oh!

john-moffett left a comment

Uh oh!

john-moffett Jan 17, 2023

Uh oh!

furszy Jan 18, 2023

Uh oh!

john-moffett Jan 18, 2023

Uh oh!

achow101 Jan 18, 2023

Uh oh!

furszy Jan 18, 2023 •

edited

Loading

Uh oh!

theStack left a comment

Uh oh!

john-moffett commented Jan 18, 2023

Uh oh!

achow101 commented Jan 24, 2023

Uh oh!

ishaanam left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

	bool CWallet::MigrateToSQLite(bilingual_str& error)
	{
	AssertLockHeld(cs_wallet);

	WalletLogPrintf("Migrating wallet storage database from BerkeleyDB to SQLite.\n");

	if (m_database->Format() == "sqlite") {
	error = _("Error: This wallet already uses SQLite");
	return false;
	}

	std::optional<MigrationData> CWallet::GetDescriptorsForLegacy(bilingual_str& error) const
	{
	AssertLockHeld(cs_wallet);

	LegacyScriptPubKeyMan* legacy_spkm = GetLegacyScriptPubKeyMan();
	if (!legacy_spkm) {
	error = _("Error: This wallet is already a descriptor wallet");
	return std::nullopt;
	}

Uh oh!

Conversation

furszy commented Jan 17, 2023

Uh oh!

DrahtBot commented Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

john-moffett left a comment

Choose a reason for hiding this comment

Uh oh!

john-moffett Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

furszy Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

john-moffett Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

achow101 Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

furszy Jan 18, 2023 • 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!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

john-moffett commented Jan 18, 2023

Uh oh!

achow101 commented Jan 24, 2023

Uh oh!

ishaanam left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Jan 17, 2023 •

edited

Loading

furszy Jan 18, 2023 •

edited

Loading