Same as with a blank wallet (#28976), wallets with no legacy records (i.e. empty, non-blank, watch-only wallet) do not require to be migrated.
Steps to reproduce the issue:
1.- createwallet "empty_wo_noblank_legacy_wallet" true false "" false false
2.- migratewallet
0wallet/wallet.cpp:4071 GetDescriptorsForLegacy: Assertion `legacy_spkm' failed.
1Aborted (core dumped)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32149.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | fjahr |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
4541+ empty_wallet = !HasLegacyRecords(local_wallet.get(), *db_batch);
4542+ }
4543+
4544 // Do the migration of keys and scripts for non-blank wallets, and cleanup if it fails
4545- success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);
4546+ success = empty_wallet || local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);
In 9962614b9060:
I changed my mind here. The blank flag check seems redundant and could even be harmful if it isn’t unset after key import or creation for some reason. This could just be a:
0success = empty_wallet;
444@@ -445,6 +445,39 @@ def test_no_privkeys(self):
445 # After migrating, the "keypool" is empty
446 assert_raises_rpc_error(-4, "Error: This wallet has no available keys", watchonly1.getnewaddress)
447
448+ # Migrating a watch-only blank empty wallet (with no pubkeys)
449+ self.log.info("Test migration of a pure watchonly blank empty wallet (with no pubkeys)")
450+ watchonly2 = self.create_legacy_wallet("watchonly2", disable_private_keys=True, blank=True)
blank=True flag is not needed when you disable private keys.
450+ watchonly2 = self.create_legacy_wallet("watchonly2", disable_private_keys=True, blank=True)
451+ # Before migrating
452+ info = watchonly2.getwalletinfo()
453+ assert_equal(info["descriptors"], False)
454+ assert_equal(info["private_keys_enabled"], False)
455+ assert_equal(info["blank"], True)
In https://github.com/bitcoin/bitcoin/commit/9962614b9060e9713042de91dc04ff5ea6604ae1:
The first check is redundant —create_legacy_wallet() already performs it. The other two checks are unnecessary. You want to verify migration here, not whether createwallet() works as expected, that’s something different.
455+ assert_equal(info["blank"], True)
456+
457+ _, watchonly2 = self.migrate_and_get_rpc("watchonly2")
458+ # After migrating
459+ info = watchonly2.getwalletinfo()
460+ assert_equal(info["descriptors"], True)
In https://github.com/bitcoin/bitcoin/commit/9962614b9060e9713042de91dc04ff5ea6604ae1:
this check is redundant, migrate_and_get_rpc() already checks it.
migratewallet instead (and then get_wallet_rpc - I can do a follow-up to clean it up later). Same for assert_is_sqlite, I’ll remove it, since migrate_and_get_rpc() already calls it within. Thanks!
457+ _, watchonly2 = self.migrate_and_get_rpc("watchonly2")
458+ # After migrating
459+ info = watchonly2.getwalletinfo()
460+ assert_equal(info["descriptors"], True)
461+ assert_equal(info["private_keys_enabled"], False)
462+ assert_equal(info["blank"], True)
In https://github.com/bitcoin/bitcoin/commit/9962614b9060e9713042de91dc04ff5ea6604ae1:
This is actually asserting the wrong behavior. The migrated wallet should have private keys disabled, just like the legacy wallet.
The migrated wallet should have private keys disabled
Yes, it’s doing that (private_keys_enabled = False), otherwise it would be failing.
475+ info = watchonly3.getwalletinfo()
476+ assert_equal(info["descriptors"], True)
477+ assert_equal(info["private_keys_enabled"], False)
478+ assert_equal(info["blank"], False)
479+ self.assert_is_sqlite("watchonly3")
480+
Could probably re-write this test in just a few lines:
0self.log.info("Test migration of a watch-only empty wallet")
1for is_blank in [True, False]:
2 self.create_legacy_wallet("watchonly_empty", disable_private_keys=True, blank=is_blank)
3 _, watchonly_empty = self.migrate_and_get_rpc("watchonly_empty")
4 info = watchonly_empty.getwalletinfo()
5 assert_equal(info["private_keys_enabled"], False)
6 assert_equal(info["blank"], is_blank)
Updates:
47@@ -48,10 +48,13 @@ BOOST_AUTO_TEST_CASE(walletdb_read_write_deadlock)
48 LOCK(wallet->cs_wallet);
49 auto legacy_spkm = wallet->GetOrCreateLegacyScriptPubKeyMan();
50 BOOST_CHECK(legacy_spkm->SetupGeneration(true));
51+ const auto& db_batch = wallet->GetDatabase().MakeBatch();
52+ BOOST_CHECK(HasLegacyRecords(wallet.get(), *db_batch));
This leaves the db batch object alive and could conflict with the Flush() and DeleteRecords() internals, as both access the db.
Better to limit the scope of the db batch object only to this function call.
58- // Now delete all records, which performs a read write operation.
59- BOOST_CHECK(wallet->GetLegacyScriptPubKeyMan()->DeleteRecords());
60+ // Now delete all records, which performs a read write operation.
61+ BOOST_CHECK(wallet->GetLegacyScriptPubKeyMan()->DeleteRecords());
62+ BOOST_CHECK(!HasLegacyRecords(wallet.get(), *db_batch));
63+ }
db_batch object is alive while Flush() and DeleteRecords() are called. My suggestion was to scope each single HasLegacyRecords() call so it does not interfere with the other calls who also access db.
The new helper will be used to fix a crash in the
wallet migration process (watch-only, non-blank,
private keys disabled, empty wallet - no scripts
or addresses imported).
Co-authored-by: Matias Furszyfer <mfurszy@protonmail.com>
Same as with a blank wallet, wallets with no legacy
records (i.e. empty, non-blank, watch-only wallet)
do not require to be migrated.
Updates:
4541+ empty_wallet = !HasLegacyRecords(local_wallet.get(), *db_batch);
4542+ }
4543+
4544 // Do the migration of keys and scripts for non-blank wallets, and cleanup if it fails
4545- success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);
4546+ success = empty_wallet;
Seems like this can be simplified (with maybe an additional comment that an empty wallet means automatic success)
0 {
1 // Keep the batch alive only for this call
2 const auto& db_batch = local_wallet->GetDatabase().MakeBatch();
3 success = !HasLegacyRecords(local_wallet.get(), *db_batch);
4 }
5
6 // Do the migration of keys and scripts for non-blank wallets, and cleanup if it fails
4533@@ -4534,8 +4534,15 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
4534 // First change to using SQLite
4535 if (!local_wallet->MigrateToSQLite(error)) return util::Error{error};
4536
4537+ bool empty_wallet{false};
4538+ {
4539+ // Keep the batch alive only for this call
db_batch to temp_batch IMO it could be removed.
332@@ -333,6 +333,9 @@ bool LoadKey(CWallet* pwallet, DataStream& ssKey, DataStream& ssValue, std::stri
333 bool LoadCryptedKey(CWallet* pwallet, DataStream& ssKey, DataStream& ssValue, std::string& strErr);
334 bool LoadEncryptionKey(CWallet* pwallet, DataStream& ssKey, DataStream& ssValue, std::string& strErr);
335 bool LoadHDChain(CWallet* pwallet, DataStream& ssValue, std::string& strErr);
336+
337+//! Returns true if there is any DBKeys::LEGACY_TYPES record in the wallet db
nit
0//! Returns true if there are any DBKeys::LEGACY_TYPES records in the wallet db
Labels
Wallet