wallet: Fix migration of blank wallets #28976
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
|
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. |
BrandonOdiwuor
left a comment
BrandonOdiwuor
left a comment
There was a problem hiding this comment.
Concept ACK
Evaluating descriptor wallets using descriptor wallet flag instead of the presence of LegacyScriptPubKeyMan when migrating wallets
achow101
force-pushed
the
migrate-blank
branch
from
be65848 to
658d214
Compare
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
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:
success = !WalletBatch(local_wallet->GetDatabase()).HasLegacyRecords();.
The implementation of HasLegacyRecords would be:
bool WalletBatch::HasLegacyRecords()
{
return std::any_of(DBKeys::LEGACY_TYPES.begin(), DBKeys::LEGACY_TYPES.end(), [&](const auto& type) { return m_batch->Exists(type); });
}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
In commit "wallet: Skip key and script migration for blank wallets" (8c127ff)
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.
test/functional/wallet_migration.py
Outdated
There was a problem hiding this comment.
In 658d2140ab:
nit: should unload the wallet after finishing the test.
DrahtBot
requested review from
BrandonOdiwuor
and removed request for
BrandonOdiwuor
Blank wallets don't have any keys or scripts to migrate
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.
achow101
force-pushed
the
migrate-blank
branch
from
658d214 to
c11c404
Compare
DrahtBot
requested review from
BrandonOdiwuor
and removed request for
BrandonOdiwuor
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
In commit "wallet: Skip key and script migration for blank wallets" (8c127ff)
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.
DrahtBot
requested review from
BrandonOdiwuor
and removed request for
BrandonOdiwuor
3904123 tests: Test that descriptors flag is set for migrated blank wallets (Ava Chow) 072d506 wallet: Make sure that the descriptors flag is set for blank wallets (Ava Chow) Pull request description: While rebasing #28710 after #28976 was merged, I realized that although blank wallets were being moved to sqlite, `WALLET_FLAG_DESCRIPTORS` was not being set so those blank wallets would still continue to be treated as legacy wallets. To fix that, just set the descriptor flags for blank wallets. Also added a test to catch this. ACKs for top commit: epiccurious: Tested ACK 3904123. delta1: tested ACK 3904123 ryanofsky: Code review ACK 3904123 murchandamus: code review ACK 3904123 Tree-SHA512: 9f6fe9c1899ca387ab909b1bb6956cd6bc5acbf941686ddc6c061f9b1ceec2cc9d009ff472486fc86e963f6068f0e2f1ae96282e7c630193797a9734c4f424ab
Blank wallets don't have any keys or scripts to migrate Github-Pull: bitcoin#28976 Rebased-From: 8c127ff
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. Github-Pull: bitcoin#28976 Rebased-From: b1d2c77
Github-Pull: bitcoin#28976 Rebased-From: 563b2a6
Github-Pull: bitcoin#28976 Rebased-From: c11c404
8 participants
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
LegacyScriptPubKeyManwhich 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)