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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | delta1, ryanofsky, epiccurious, murchandamus |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
tested ACK 3904123da954f9ebd905de4129aec9f9bab9efc7
confirmed that the test assertion fails without the change to wallet.cpp
4256@@ -4257,6 +4257,9 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
4257 success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);
4258 if (!success) {
4259 success = DoMigration(*local_wallet, context, error, res);
4260+ } else {
4261+ // Make sure that descriptors flag is actually set
4262+ local_wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
Given that the PR describes the setting of the flag as a special case for blank wallets, I’d be more comfortable if the setting of the flag were conditional on a wallet being blank instead of success being true here.
0- success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);
1+ if(local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET)) {
2+ local_wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
3+ success = true;
4+ }
5+
6 if (!success) {
7 success = DoMigration(*local_wallet, context, error, res);
8- } else {
9- // Make sure that descriptors flag is actually set
10- local_wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
I’d be more comfortable if the setting of the flag were conditional on a wallet being blank instead of success being true
Just to be clear, these two conditions should be exactly equivalent due to the success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET); line so the suggestion is a style change, not a change in behavior. I agree the suggestion looks a little better, but the style of dealing with the success variable seems here to follow a particular pattern, so would leave it up to achow to decide which way fits better.
success that is instantiated half a code block away and much more likely to change its meaning in the course of the project.
@achow101: I would have expected that DoMigration sets WALLET_FLAG_DESCRIPTOR, but it doesn’t seem to be the case. If you wanted to set it generally, perhaps you could just set it after the if (!success) {…} block here?
DoMigration does do that, that’s why this is only a problem for blank wallets.
As currently implemented, this only sets it for blank wallets, but I do also think the correct behavior is to set it for all wallets, but as I said earlier, I have not analyzed that yet.
re: #29367 (review)
DoMigrationdoes do that, that’s why this is only a problem for blank wallets.
Seems like in a followup it might be cleaner to do this inside DoMigration, so the flag is set in one place. In the meantime it could help to change the “Make sure that descriptors flag is actually set” comment to “Make sure that descriptors flag is set if DoMigration is not called (otherwise DoMigration will set it)”
code review ACK 3904123da954f9ebd905de4129aec9f9bab9efc7
Nit: The code behavior seems correct to me, but I would prefer if either the commit message were adapted to state that the WALLET_FLAG_DESCRIPTOR is being set for all successfully migrated wallets, or if the code were adapted to more explicitly implement the behavior described in the commit as described in my comment above.
I went ahead and merged this in its current form since it is a minimal bugfix for a bug introduced yesterday in #28976. Without #28976, blank wallets would refuse to migrate. But with #28976 and without this fix, blank wallets would appear to successfully migrate but be in an unsupported legacy mode + sqlite backend state, so it seemed better to merge sooner rather than later.
It should be possible to followup on comments here and in the previous PR:
in future wallet migration PRs (maybe #28868)
post-merge ACK 3904123
In the test, would be nice to decouple the migratewallet() call into another function, to consistently check that all migrated wallet contain this flag.