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 |
|---|---|
| ACK | furszy, BrandonOdiwuor, davidgumberg, fjahr, achow101 |
| Concept ACK | rkrux |
| Stale ACK | w0xlt |
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.
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.
HasLegacyRecords so it’d make it even cleaner I think.
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
Updates:
tACK d4cb6d048b0222cf51c8a56fa56f97dd5d2c9add
Changes look good to me and I confirmed that the new test fails on master.
4534@@ -4535,7 +4535,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
4535 if (!local_wallet->MigrateToSQLite(error)) return util::Error{error};
4536
4537 // Do the migration of keys and scripts for non-blank wallets, and cleanup if it fails
4538- success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);
4539+ success = !HasLegacyRecords(local_wallet.get());;
555+
556+ prefix << type;
557+ std::unique_ptr<DatabaseCursor> cursor = batch.GetNewPrefixCursor(prefix);
558+ if (!cursor) {
559+ // Could only happen on a closed db, which means there is an error in the code flow.
560+ pwallet->WalletLogPrintf("Error getting database cursor for '%s' records", type);
pwallet argument could have otherwise been removed.
47@@ -48,10 +48,12 @@ BOOST_AUTO_TEST_CASE(walletdb_read_write_deadlock)
48 LOCK(wallet->cs_wallet);
49 auto legacy_spkm = wallet->GetOrCreateLegacyScriptPubKeyMan();
nit, if you retouch:
0 auto legacy_spkm = wallet->GetOrCreateLegacyScriptPubKeyMan();
1 BOOST_CHECK(!HasLegacyRecords(wallet.get()));
560+ pwallet->WalletLogPrintf("Error getting database cursor for '%s' records", type);
561+ throw std::runtime_error(strprintf("Error getting database cursor for '%s' records", type));
562+ }
563+
564+ DatabaseCursor::Status status = cursor->Next(key, value);
565+ if (status != DatabaseCursor::Status::DONE) {
maybe this should be:
0 if (status == DatabaseCursor::Status::MORE)
as is done in WalletBatch::IsEncrypted(). That would more correctly handle Status::FAIL which I presume is extremely unlikely here.
Edit: And out of scope of this refactor commit, feel free to disregard, just an observation.
ACK eb4333c8aa4a8f1a66ecbe5c48dbe15467332864
Tested that this fixes the issue in the PR description and also fixes another issue, #32112 (see #32112 (comment)). Reviewed and manually verified that the test case added here in test/functional/wallet_migration.py catches the issue in master. This refactor also improves readability of LoadLegacyWalletRecords().
4534@@ -4535,7 +4535,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
4535 if (!local_wallet->MigrateToSQLite(error)) return util::Error{error};
4536
4537 // Do the migration of keys and scripts for non-blank wallets, and cleanup if it fails
The comment can be updated now to account for empty wallets as well.
0- // Do the migration of keys and scripts for non-blank wallets, and cleanup if it fails
1+ // Do the migration of keys and scripts for non-blank and non-empty wallets, and cleanup if it fails
4534@@ -4535,7 +4535,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
4535 if (!local_wallet->MigrateToSQLite(error)) return util::Error{error};
4536
4537 // Do the migration of keys and scripts for non-blank wallets, and cleanup if it fails
4538- success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);
4539+ success = !HasLegacyRecords(local_wallet.get());
4540 if (!success) {
This has become a double negation now that makes it tougher to read imo. We can get rid of setting the success variable here and directly add in the if condition. Moreover, setting success here seems quite unusual to me, just like it was in the previous code as well. I believe it can be directly updated in the if condition.
I notice that the migration is done only if success is false and then updated. But if success evaluates to true at this line, the migration is skipped and success remains true. To handle this, success can be initially set to true. Also, I believe this is an opportunity to narrow the scope of success here because it’s not used in the previous if block at all.
So, the overall diff is below, which is more than originally intended but I believe it makes this code block more readable. All the unit tests and the migration functional tests pass.
0 res.backup_path = backup_path;
1
2- bool success = false;
3-
4 // Unlock the wallet if needed
5 if (local_wallet->IsLocked() && !local_wallet->Unlock(passphrase)) {
6 if (was_loaded) {
7@@ -4529,14 +4527,14 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
8 }
9 }
10
11+ bool success = true;
12 {
13 LOCK(local_wallet->cs_wallet);
14 // First change to using SQLite
15 if (!local_wallet->MigrateToSQLite(error)) return util::Error{error};
16
17 // Do the migration of keys and scripts for non-blank wallets, and cleanup if it fails
18- success = !HasLegacyRecords(local_wallet.get());
19- if (!success) {
20+ if (HasLegacyRecords(local_wallet.get())) {
21 success = DoMigration(*local_wallet, context, error, res);
22 } else {
23 // Make sure that descriptors flag is actually set
Concept ACK eb4333c8aa4a8f1a66ecbe5c48dbe15467332864
Suggested changes based on readability.
556+ prefix << type;
557+ std::unique_ptr<DatabaseCursor> cursor = batch.GetNewPrefixCursor(prefix);
558+ if (!cursor) {
559+ // Could only happen on a closed db, which means there is an error in the code flow.
560+ pwallet->WalletLogPrintf("Error getting database cursor for '%s' records", type);
561+ throw std::runtime_error(strprintf("Error getting database cursor for '%s' records", type));
Throwing a runtime error here now instead of returning DBErrors::CORRUPT that the older code was doing makes sense to me. In case of loading wallet, this will be caught later, ultimately evaluating to the same DBError:
https://github.com/bitcoin/bitcoin/blob/cfa7f70f6c99680851ab0c0df27baf9f7508747b/src/wallet/walletdb.cpp#L1219-L1223
And in case of migrating wallet flow, this will not be caught but bubble up all the way to the RPC response, which seems ok to me.
544+{
545+ const auto& batch = pwallet->GetDatabase().MakeBatch();
546+ return HasLegacyRecords(pwallet, *batch);
547+}
548+
549+bool HasLegacyRecords(CWallet* pwallet, DatabaseBatch& batch)
0bool HasLegacyRecords(CWallet& wallet)
1{
2 const auto& batch = wallet.GetDatabase().MakeBatch();
3 return HasLegacyRecords(wallet, *batch);
4}
5
6bool HasLegacyRecords(CWallet& wallet, DatabaseBatch& batch)
nit (feel free to ignore): If a function does not accept or handle a nullptr as an arg, it is better to just use a reference, which can not be null. This is self-documenting and also makes it clear to the caller that passing a nullptr would lead to UB.
Updates:
4533@@ -4534,9 +4534,8 @@ 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- // Do the migration of keys and scripts for non-blank wallets, and cleanup if it fails
4538- success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);
4539- if (!success) {
4540+ // Do the migration of keys and scripts for non-empty wallets, and cleanup if it fails
4541+ if (HasLegacyRecords(*local_wallet.get())) {
nit: no need for the get() now.
0 if (HasLegacyRecords(*local_wallet)) {
(If you want to fix the test-each-commit CI, a rebase should do it, or the failure can be ignored for now)
Ok I saw other places where * and get() it’s being used, I can do a follow-up and fix them all in 1 go.
For the CI, I’d leave it as is, easier for current reviewers.
Updates:
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>
the second commit has a typo in its title
Fixed.
4510@@ -4511,7 +4511,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
4511 }
4512 res.backup_path = backup_path;
4513
4514- bool success = false;
4515+ bool success{true};
I’m not sure about this change; it could be a source of bugs if we ever add something within the procedure without updating this value.
Yet, if others are happy with it, it is fine for me.
false and set it to true in the else clause where we do local_wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);? (or success = Assert(local_wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS));)
success variable that imho should make it less likely to miss updating this variable.
Another alternative could be moving the initialisation of success close to the conditional as it’s done in CWallet::MarkReplaced:
For simplicity, we could just:
0// Do the migration of keys and scripts for non-empty wallets, and cleanup if it fails
1if (HasLegacyRecords()) {
2 success = DoMigration(*local_wallet, context, error, res);
3} else {
4 // Make sure that descriptors flag is actually set
5 local_wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
6 success = true;
7}
Leaving the success=false initialization.
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.
