wallet: Migrate legacy wallets to descriptor wallets without requiring BDB

[achow101]

trafficstars

#26606 introduced BerkeleyRODatabase which is an independent parser for BDB files. This PR uses this in legacy wallet migration so that migration will continue to work once the legacy wallet and BDB are removed. LegacyDataSPKM is introduced to have the minimum data and functions necessary for a legacy wallet to be loaded for migration.

[DrahtBot]

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	cbergqvist, theStack, furszy

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:

• #29124 (wallet: Automatically repair corrupted metadata with doubled derivation path by achow101)
• #28574 (wallet: optimize migration process, batch db transactions by furszy)
• #28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively 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.

[Sjors]

I think it would be easier to review if you made a separate PR for BerkeleyRODatabase and have it work as a regular, albeit read-only, wallet database backend.

I'm pleasantly surprised at how small the implementation is.

[luke-jr]

I don't see how reimplementing BDB is better than just continuing to use BDB.

[achow101]

I don't see how reimplementing BDB is better than just continuing to use BDB.

The reimplementation is quite compact because it is for only reading BDB files, only the features that we use, and none of the actual DB system stuff. BDB itself contains a ton of code, a lot of which we don't use. Maintaining it as a dependency will eventually be more burdensome - we already need to patch it in order to compile in the depends system. This reimplementation does not carry those issues - it implements a format that isn't going to change, and it only depends on things that are already being used elsewhere in the codebase.

[achow101]

I think it would be easier to review if you made a separate PR for BerkeleyRODatabase and have it work as a regular, albeit read-only, wallet database backend.

I've opeend #26606 for just BerkeleyRODatabase and its use in wallettool's dump.

[DrahtBot]

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/21780750809}

[achow101]

Rebased following #26606. Now ready for review.

[achow101]

Is the goal to delete LegacyScriptPubKeyMan in the future when ripping out BDB and only keep LegacyDataSPKM?

Yes.

We are trying to remove the legacy wallet in its entirety, leaving only behind the absolute minimum necessary to perform migration.

Why perform the inlining of IsMine() in 31e918a before the function is removed? Seems risky in case of other PRs modifying it prior to removal etc.

IsMine() is unlikely to change. Besides the fact that any changes to the legacy wallet are likely to be closed instead of seriously considered, IsMine() has not had any material changes in the last several years. It could be included in #28710 but that PR is big enough as it is.

LegacyScriptPubKeyMan::NewKeyPool() only touches data from LegacyDataSPKM, should it be moved to that class as public (or protected and then exposed in the subclass?)

No. The separation exists now to indicate which things can be removed at the end. LegacyDataSPKM is the bare minimum that must stick around for migration, everything else remains in LegacyScriptPubKeyMan and will be removed at the same time.

[theStack]

Still trying to fully grasp the IsMine inlining commit 89503710386f52d9162f67fcd707eceffa954faa, meanwhile a few comments:

• The PR description is outdated, as it still mentions adding BerkeleyRODatabase which was already merged in #26606
• in c653f4fdbfe0607ad9be27e80ad22c9aee314bb7: typo in commit subject (s/MigrateToLegacy/MigrateToDescriptor/)
• To my understanding, the class LegacyDataSPKM is supposed to never write to the database (as BerkeleyRODatabase obviously doesn't support that), but LegacyDataSPKM::CheckDecryptionKey contains a code-path for that (for a necessary rewrite): https://github.com/achow101/bitcoin/blob/a8f8a2d5d0e2328cbe2e569c7e12f653d18f456d/src/wallet/scriptpubkeyman.cpp#L254-L255 Should CheckDecryptionKey stay in LegacyScriptPubKeyMan?

[achow101]

I believe this PR could get merged quite fast if you leave 8950371 for a follow-up and make IsMine compatible with LegacyDataSPKM in this PR.

Moved to #30328

The IsMine removal is also nice but I wouldn't miss a deadline for it.

I think it needs to be in the next release as it is a critical part of migration. I would prefer to have it in a release that people can use before deleting the legacy wallet.

• The PR description is outdated, as it still mentions adding BerkeleyRODatabase which was already merged in wallet: Implement independent BDB parser #26606

Updated

• in c653f4f: typo in commit subject (s/MigrateToLegacy/MigrateToDescriptor/)

Fixed

• To my understanding, the class LegacyDataSPKM is supposed to never write to the database (as BerkeleyRODatabase obviously doesn't support that), but LegacyDataSPKM::CheckDecryptionKey contains a code-path for that (for a necessary rewrite): https://github.com/achow101/bitcoin/blob/a8f8a2d5d0e2328cbe2e569c7e12f653d18f456d/src/wallet/scriptpubkeyman.cpp#L254-L255 Should CheckDecryptionKey stay in LegacyScriptPubKeyMan?

No, we should not be migrating a wallet if we cannot decrypt the keys. BerkeleyRODatabase always returns true for all write operations so it doesn't matter. Also, I don't think it is useful to invert that and have to have specific bypasses for BerkeleyRODatabase in all functions that attempts to write to it.

[furszy]

Now that the IsMine changes were moved to another PR, could reorder the commits just so 0cce574 is introduced before c3373a39a1acc94dbf644103fab2e477d49da671. This will let us keep the IsMine assertions inside MigrateToDescriptor for now.

Other than that, code review ACK 9700c21.

[achow101]

Reordered the commits and preserved the IsMine asserts.