wallet: migrate wallet, exit early if no legacy data exist #26910

furszy commented at 7:00 PM on January 17, 2023: member

The process first creates a backup file then return an error, without removing the recently created file, when notices that the db is already running sqlite.

DrahtBot commented at 7:00 PM on January 17, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	john-moffett, achow101, ishaanam
Concept ACK	theStack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26596 (wallet: Migrate legacy wallets to descriptor wallets without requiring BDB by achow101)

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.

DrahtBot added the label Wallet on Jan 17, 2023

in src/wallet/rpc/wallet.cpp:749 in 412a9aa6f0 outdated

 744 | @@ -745,6 +745,10 @@ static RPCHelpMan migratewallet()
 745 |                  throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: migratewallet on encrypted wallets is currently unsupported.");
 746 |              }
 747 |  
 748 | +            if (!wallet->GetLegacyScriptPubKeyMan()) {
 749 | +                throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Error: nothing to migrate, no legacy data contained in wallet \"%s\"", wallet->GetName()));

john-moffett commented at 9:01 PM on January 17, 2023:

I wonder if the message is consistent with how the migration code actually works. For instance, the error returned before this change is "This wallet already uses SQLite":

https://github.com/bitcoin/bitcoin/blob/fbe5e1220a35c35a668ec965c66508a7839aa7dd/src/wallet/wallet.cpp#L3765-L3774

Even if it passed that check (which I believe is impossible in this situation), it does the same check as in this PR and reads "This wallet is already a descriptor wallet":

https://github.com/bitcoin/bitcoin/blob/fbe5e1220a35c35a668ec965c66508a7839aa7dd/src/wallet/wallet.cpp#L3837-L3845

Minor additional nit: I think the RPC error messages typically start with a capital letter after the Error: unless it's an RPC command (as in line 745 above).

furszy commented at 2:34 PM on January 18, 2023:

I wonder if the message is consistent with how the migration code actually works. For instance, the error returned before this change is "This wallet already uses SQLite"

well, think about the command purpose. If there is no legacy data to migrate then why the command should be executed at all? Doesn't really care if the db is sqlite, bdb or anything else.

Minor additional nit: I think the RPC error messages typically start with a capital letter after the Error: unless it's an RPC command (as in line 745 above).

Seems that we have a mix of them, some errors start with a capital letter, some others not (like the encrypt.cpp file). I'm more aligned to this second form but have no real preference for any. Maybe the best move would be to open an issue an align all of them, in one way or another, at once.

john-moffett commented at 3:20 PM on January 18, 2023:

If there is no legacy data to migrate then why the command should be executed at all?

Right, I agree that the message makes logical sense, though I do worry that it suggests that someone can have a descriptor wallet with legacy data, or they may think that "legacy" addresses are "legacy data", like getnewaddress -addresstype legacy.

My personal preference is to copy the Error: This wallet is already a descriptor wallet message, since that better matches the help migratewallet message: Migrate the wallet to a descriptor wallet.

That said, it's not a strong preference. Maybe @achow101 could weigh in?

achow101 commented at 4:38 PM on January 18, 2023:

Right, I agree that the message makes logical sense, though I do worry that it suggests that someone can have a descriptor wallet with legacy data

It's theoretically possible to have a descriptor wallet using BDB for the database as well as a legacy wallet using sqlite. These are unsupported configurations, but are allowed and possible to achieve using the wallettool. That being said, they are unsupported so I think it's fine, and the number of people who would have such wallets should be limited to a very small number of developers.

Regarding the error message, I would also prefer the "already a descriptor wallet" wording.

furszy commented at 6:23 PM on January 18, 2023:

pushed. Moved the GetDescriptorsForLegacy error up to the top of the process to not duplicate it.

john-moffett commented at 9:06 PM on January 17, 2023: contributor

Concept ACK. Tested with and without changes and works as described.

wallet: migrate wallet, exit early if no legacy data exist

otherwise the process will create a backup file then return
an error when notices that the db is already running sqlite.

6d31900e52

furszy force-pushed on Jan 18, 2023

theStack commented at 5:19 PM on January 18, 2023: contributor

Concept ACK

john-moffett commented at 7:12 PM on January 18, 2023: contributor

ACK 6d31900e52efa2c7c7a220d8c8ad6353c412a2aa

Tested and code review.

achow101 commented at 7:27 PM on January 24, 2023: member

ACK 6d31900e52efa2c7c7a220d8c8ad6353c412a2aa

ishaanam commented at 3:49 AM on February 1, 2023: contributor

crACK 6d31900e52efa2c7c7a220d8c8ad6353c412a2aa

achow101 merged this on Feb 1, 2023

achow101 closed this on Feb 1, 2023

sidhujag referenced this in commit fe6f70f8b3 on Feb 3, 2023

furszy deleted the branch on May 27, 2023

bitcoin locked this on May 26, 2024