wallet: Set descriptors flag after migrating blank wallets #29367

achow101 · 2024-02-01T23:14:18Z

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.

DrahtBot · 2024-02-01T23:14:21Z

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	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.

delta1

tested ACK 3904123

confirmed that the test assertion fails without the change to wallet.cpp

murchandamus

Concept ACK

murchandamus · 2024-02-02T14:59:10Z

src/wallet/wallet.cpp

        success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);
        if (!success) {
            success = DoMigration(*local_wallet, context, error, res);
+        } else {
+            // Make sure that descriptors flag is actually set
+            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.

- success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET); + if(local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET)) { + local_wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); + success = true; + } + if (!success) { success = DoMigration(*local_wallet, context, error, res); - } else { - // Make sure that descriptors flag is actually set - 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.

I think it should actually unconditionally set the flag, but I haven't fully thought through if that is safe yet. Intuitively, it should be safe since a failed migration would result in the wallet being deleted anyways, but I haven't considered all of the possibilities yet.

@ryanofsky Yeah, I was not proposing a change in behavior. It just seems cleaner to tie the intended behavior directly to the relevant wallet property than to a generic boolean like 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 (comment)

DoMigration does 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)"

ryanofsky

Code review ACK 3904123

ryanofsky · 2024-02-02T16:42:29Z

src/wallet/wallet.cpp

        success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);
        if (!success) {
            success = DoMigration(*local_wallet, context, error, res);
+        } else {
+            // Make sure that descriptors flag is actually set
+            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.

epiccurious · 2024-02-02T16:52:31Z

Tested ACK 3904123.

murchandamus

code review ACK 3904123

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.

murchandamus · 2024-02-02T18:05:51Z

src/wallet/wallet.cpp

        success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);
        if (!success) {
            success = DoMigration(*local_wallet, context, error, res);
+        } else {
+            // Make sure that descriptors flag is actually set
+            local_wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);


@ryanofsky Yeah, I was not proposing a change in behavior. It just seems cleaner to tie the intended behavior directly to the relevant wallet property than to a generic boolean like 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?

ryanofsky · 2024-02-02T19:53:14Z

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)

furszy

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.

Github-Pull: bitcoin#29367 Rebased-From: 072d506

Github-Pull: bitcoin#29367 Rebased-From: 3904123

achow101 added 2 commits February 1, 2024 18:00

wallet: Make sure that the descriptors flag is set for blank wallets

072d506

tests: Test that descriptors flag is set for migrated blank wallets

3904123

DrahtBot added the Wallet label Feb 1, 2024

achow101 added this to the 27.0 milestone Feb 1, 2024

fanquake requested review from ryanofsky, furszy and murchandamus February 2, 2024 09:06

delta1 approved these changes Feb 2, 2024

View reviewed changes

murchandamus reviewed Feb 2, 2024

View reviewed changes

furszy mentioned this pull request Feb 2, 2024

rpc: Drop migratewallet experimental warning #28037

Merged

ryanofsky approved these changes Feb 2, 2024

View reviewed changes

DrahtBot requested a review from murchandamus February 2, 2024 16:45

murchandamus reviewed Feb 2, 2024

View reviewed changes

ryanofsky self-assigned this Feb 2, 2024

ryanofsky merged commit 93e10ca into bitcoin:master Feb 2, 2024
16 checks passed

furszy reviewed Feb 2, 2024

View reviewed changes

Retropex pushed a commit to Retropex/bitcoin that referenced this pull request Mar 28, 2024

wallet: Make sure that the descriptors flag is set for blank wallets

b7e41dd

Github-Pull: bitcoin#29367 Rebased-From: 072d506

Retropex pushed a commit to Retropex/bitcoin that referenced this pull request Mar 28, 2024

tests: Test that descriptors flag is set for migrated blank wallets

66d1d24

Github-Pull: bitcoin#29367 Rebased-From: 3904123

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet: Set descriptors flag after migrating blank wallets #29367

wallet: Set descriptors flag after migrating blank wallets #29367

achow101 commented Feb 1, 2024

DrahtBot commented Feb 1, 2024 •

edited

Loading

delta1 left a comment

murchandamus left a comment

murchandamus Feb 2, 2024

ryanofsky Feb 2, 2024 •

edited

Loading

achow101 Feb 2, 2024

murchandamus Feb 2, 2024

achow101 Feb 2, 2024

ryanofsky Feb 2, 2024

ryanofsky left a comment

ryanofsky Feb 2, 2024 •

edited

Loading

epiccurious commented Feb 2, 2024 •

edited

Loading

murchandamus left a comment •

edited

Loading

murchandamus Feb 2, 2024

ryanofsky commented Feb 2, 2024

furszy left a comment

wallet: Set descriptors flag after migrating blank wallets #29367

wallet: Set descriptors flag after migrating blank wallets #29367

Conversation

achow101 commented Feb 1, 2024

DrahtBot commented Feb 1, 2024 • edited Loading

Code Coverage

Reviews

delta1 left a comment

Choose a reason for hiding this comment

murchandamus left a comment

Choose a reason for hiding this comment

murchandamus Feb 2, 2024

Choose a reason for hiding this comment

ryanofsky Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

achow101 Feb 2, 2024

Choose a reason for hiding this comment

murchandamus Feb 2, 2024

Choose a reason for hiding this comment

achow101 Feb 2, 2024

Choose a reason for hiding this comment

ryanofsky Feb 2, 2024

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

epiccurious commented Feb 2, 2024 • edited Loading

murchandamus left a comment • edited Loading

Choose a reason for hiding this comment

murchandamus Feb 2, 2024

Choose a reason for hiding this comment

ryanofsky commented Feb 2, 2024

furszy left a comment

Choose a reason for hiding this comment

DrahtBot commented Feb 1, 2024 •

edited

Loading

ryanofsky Feb 2, 2024 •

edited

Loading

ryanofsky Feb 2, 2024 •

edited

Loading

epiccurious commented Feb 2, 2024 •

edited

Loading

murchandamus left a comment •

edited

Loading