wallet: Fix migration of blank wallets

achow101 commented at 9:02 pm on November 30, 2023: member

Blank wallets (wallets without any keys are scripts) are being detected as already being descriptor wallets even though they are not. This is because the check for whether a wallet is already a descriptor wallet uses the presence of a LegacyScriptPubKeyMan which is only setup when keys or scripts are found. This PR resolves this issue by checking for the descriptor wallet flag instead and subsequently skipping the keys and scripts part of migration for blank wallets.

Fixes the issue mentioned in #28868 (comment)

DrahtBot commented at 9:03 pm on November 30, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	furszy, ryanofsky
Concept ACK	BrandonOdiwuor

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:

#28868 (wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs 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.

achow101 renamed this:
~~Migrate blank~~
wallet: Fix migration of blank wallets
on Nov 30, 2023

DrahtBot added the label Wallet on Nov 30, 2023

fanquake commented at 9:29 pm on November 30, 2023: member

For backport or no?

achow101 commented at 10:09 pm on November 30, 2023: member

For backport or no?

No, I don’t think this is a regression, nor is it really a bug that is that important. If someone runs into it, the wallet is completely blank and empty, so they can just delete it and make a new descriptors wallet.

in src/wallet/wallet.cpp:3838 in be6584882f outdated

3832@@ -3833,7 +3833,11 @@ std::optional<MigrationData> CWallet::GetDescriptorsForLegacy(bilingual_str& err
3833     AssertLockHeld(cs_wallet);
3834 
3835     LegacyScriptPubKeyMan* legacy_spkm = GetLegacyScriptPubKeyMan();
3836-    assert(legacy_spkm);
3837+    if (!Assume(legacy_spkm)) {
3838+        // This shouldn't happen
3839+        error = _("Error: Legacy wallet data missing");

maflcko commented at 8:12 am on December 1, 2023:

If this truly should never happen, and it seems like an internal bug when it happens, then I wonder if the translation is needed and if STR_INTERNAL_BUG can be used?

0        error = Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing"));

maflcko commented at 8:12 am on December 1, 2023:

(same below)

achow101 commented at 5:24 pm on December 1, 2023:

Done

BrandonOdiwuor commented at 11:43 am on December 1, 2023: contributor

Concept ACK

Evaluating descriptor wallets using descriptor wallet flag instead of the presence of LegacyScriptPubKeyMan when migrating wallets

achow101 force-pushed on Dec 1, 2023

in src/wallet/wallet.cpp:4255 in 0e7eb54bfe outdated

4230@@ -4231,8 +4231,11 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
4231         // First change to using SQLite
4232         if (!local_wallet->MigrateToSQLite(error)) return util::Error{error};
4233 
4234-        // Do the migration, and cleanup if it fails
4235-        success = DoMigration(*local_wallet, context, error, res);
4236+        // Do the migration of keys and scripts for non-blank wallets, and cleanup if it fails
4237+        success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);

furszy commented at 11:47 pm on December 1, 2023:

In 0e7eb54bfe6:

Are we sure we always have unset the blank flag when an address, script or key was imported across all of our previous versions?

Because, if we are not, could change this line for:

0success = !WalletBatch(local_wallet->GetDatabase()).HasLegacyRecords();

. The implementation of HasLegacyRecords would be:

0bool WalletBatch::HasLegacyRecords()
1{
2    return std::any_of(DBKeys::LEGACY_TYPES.begin(), DBKeys::LEGACY_TYPES.end(), [&](const auto& type) { return m_batch->Exists(type); });
3}

achow101 commented at 0:30 am on December 2, 2023:

Are we sure we always have unset the blank flag when an address, script or key was imported across all of our previous versions?

I’m pretty sure that it was unset whenever something was imported.

ryanofsky commented at 8:32 pm on January 31, 2024:

In commit “wallet: Skip key and script migration for blank wallets” (8c127ff1edb6b9a607bf1ad247893347252a38e3)

I’m pretty sure that it was unset whenever something was imported.

This should be true but it would be good to raise an error if the blank flag is set and these records do exist. Seems like it would easy to check for, given the HasLegacyRecords() function. It would document the assumption being made by this code, and provide better error checking.

in test/functional/wallet_migration.py:883 in 658d2140ab outdated

874@@ -876,6 +875,13 @@ def test_failed_migration_cleanup(self):
875             _, _, magic = struct.unpack("QII", data)
876             assert_equal(magic, BTREE_MAGIC)
877 
878+    def test_blank(self):
879+        self.log.info("Test that a blank wallet is migrated")
880+        wallet = self.create_legacy_wallet("blank", blank=True)
881+        assert_equal(wallet.getwalletinfo()["blank"], True)
882+        wallet.migratewallet()
883+        assert_equal(wallet.getwalletinfo()["blank"], True)

furszy commented at 2:15 am on December 2, 2023:

In 658d2140ab: nit: should unload the wallet after finishing the test.

furszy approved

furszy commented at 2:23 am on December 2, 2023: member

Code review ACK 658d2140

DrahtBot requested review from BrandonOdiwuor on Dec 2, 2023

DrahtBot removed review request from BrandonOdiwuor on Dec 2, 2023

DrahtBot requested review from BrandonOdiwuor on Dec 2, 2023

DrahtBot added the label Needs rebase on Jan 8, 2024

wallet: Skip key and script migration for blank wallets

Blank wallets don't have any keys or scripts to migrate

8c127ff1ed

wallet: Check for descriptors flag before migration

Previously we would check that there is no LegacySPKM in order to
determine whether a wallet is already a descriptor wallet and doesn't
need to be migrated. However blank legacy wallets will also not have a
LegacySPKM, so we need to be checking for the descriptors flag instead.

b1d2c771d4

wallet: Better error message when missing LegacySPKM during migration 563b2a60d6

tests: Test migration of blank wallets c11c404281

achow101 force-pushed on Jan 11, 2024

DrahtBot removed the label Needs rebase on Jan 11, 2024

DrahtBot added the label CI failed on Jan 12, 2024

furszy approved

furszy commented at 1:54 pm on January 12, 2024: member

reACK c11c404281d2d0e22967e30e16c0733db84f4eee. CI failure is unrelated.

DrahtBot removed review request from BrandonOdiwuor on Jan 12, 2024

DrahtBot requested review from BrandonOdiwuor on Jan 12, 2024

DrahtBot removed review request from BrandonOdiwuor on Jan 12, 2024

DrahtBot requested review from BrandonOdiwuor on Jan 12, 2024

ryanofsky approved

ryanofsky commented at 8:38 pm on January 31, 2024: contributor

Code review ACK c11c404281d2d0e22967e30e16c0733db84f4eee

DrahtBot removed review request from BrandonOdiwuor on Jan 31, 2024

DrahtBot requested review from BrandonOdiwuor on Jan 31, 2024

DrahtBot removed review request from BrandonOdiwuor on Jan 31, 2024

DrahtBot requested review from BrandonOdiwuor on Jan 31, 2024

ryanofsky merged this on Jan 31, 2024

ryanofsky closed this on Jan 31, 2024

murchandamus commented at 9:07 pm on January 31, 2024: contributor

Post merge crACK c11c404281d2d0e22967e30e16c0733db84f4eee

wallet: Fix migration of blank wallets #28976

Code Coverage

Reviews

Conflicts