wallet: Migrate legacy wallets to descriptor wallets without requiring BDB #26596

achow101 · 2022-11-28T23:46:50Z

#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 · 2022-11-28T23:46:52Z

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 · 2022-11-29T14:41:14Z

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 · 2022-11-29T21:20:19Z

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

achow101 · 2022-11-29T21:27:56Z

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 · 2022-11-29T21:51:35Z

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.

achow101 · 2024-06-24T16:47:30Z

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

Strange CI https://github.com/bitcoin/bitcoin/actions/runs/9649050504/job/26611651920?pr=26596 error.. it doesn't seem to be related. The "salvage" command opens the wallet in DatabaseFormat::BERKELEY format.

src/wallet/wallet.cpp

When we reopen the wallet to do the migration, instead of opening using BDB, open it using the BerkeleyRO implementation.

src/wallet/scriptpubkeyman.cpp

furszy · 2024-06-27T20:33:25Z

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

Other than that, code review ACK 9700c21.

cbergqvist

git range-diff master a8f8a2d 9700c217

Agree with @furszy on reordering the commits to make it possible to retain the 3 asserts in MigrateToDescriptor().

src/wallet/scriptpubkeyman.cpp

In order to load the necessary data for migrating a legacy wallet without the full LegacyScriptPubKeyMan, move the data storage and loading components to LegacyDataSPKM. LegacyScriptPubKeyMan now subclasses that.

IsMine is necessary for migration. It should be inlined with migration when the legacy wallet is removed.

In SetupLegacyScriptPubKeyMan, a base LegacyDataSPKM will be created if the database has the format "bdb_ro" (i.e. the wallet was opened only for migration purposes). All of the loading functions are now called with a LegacyDataSPKM object instead of LegacyScriptPubKeyMan.

The migration process reloads the wallet after all failures. This commit tests the behavior by trying to obtain a new address after a decryption failure during migration.

achow101 · 2024-07-01T18:26:37Z

Reordered the commits and preserved the IsMine asserts.

cbergqvist

ACK 8ce3739

git range-diff 9700c217~5..9700c217 8ce3739~5..8ce3739 - not the easiest read, also used GH compare.

Passed make check & test/functional/test_runner.py including p2p_handshake.py which failed on CI.

Only one non-critical reservation: #26596 (comment)

theStack

Code-review ACK 8ce3739

theStack · 2024-07-03T15:13:50Z

src/wallet/wallet.cpp

+LegacyDataSPKM* CWallet::GetLegacyDataSPKM() const
+{
+    if (IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
+        return nullptr;
+    }
+    auto it = m_internal_spk_managers.find(OutputType::LEGACY);
+    if (it == m_internal_spk_managers.end()) return nullptr;
+    return dynamic_cast<LegacyDataSPKM*>(it->second);
+}


nit: was about to suggest to deduplicate shared code between this method and ::GetLegacyScriptPubKeyMan above, but since the latter is removed soon anyway (in PR #28710, commit 5652a9c#diff-1f2db0e4d5c12d109c7f0962333c245b49b696cb39ff432da048e9d6c08944d8), it's probably not worth it.

If you re-touch it again. Could:

LegacyDataSPKM* CWallet::GetLegacyDataSPKM() const { if (IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { return nullptr; } return dynamic_cast<LegacyDataSPKM*>(GetScriptPubKeyMan(OutputType::LEGACY, /*internal=*/true)); }

furszy

Code review ACK 8ce3739

furszy · 2024-07-03T19:51:31Z

src/wallet/wallet.cpp

+LegacyDataSPKM* CWallet::GetLegacyDataSPKM() const
+{
+    if (IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
+        return nullptr;
+    }
+    auto it = m_internal_spk_managers.find(OutputType::LEGACY);
+    if (it == m_internal_spk_managers.end()) return nullptr;
+    return dynamic_cast<LegacyDataSPKM*>(it->second);
+}


If you re-touch it again. Could:

LegacyDataSPKM* CWallet::GetLegacyDataSPKM() const { if (IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { return nullptr; } return dynamic_cast<LegacyDataSPKM*>(GetScriptPubKeyMan(OutputType::LEGACY, /*internal=*/true)); }

furszy · 2024-07-03T20:00:08Z

src/wallet/scriptpubkeyman.cpp

 void LegacyScriptPubKeyMan::LoadKeyMetadata(const CKeyID& keyID, const CKeyMetadata& meta)
 {
    LOCK(cs_KeyStore);
+    LegacyDataSPKM::LoadKeyMetadata(keyID, meta);
    UpdateTimeFirstKey(meta.nCreateTime);
-    mapKeyMetadata[keyID] = meta;
+}


In 7461d0c:

At this point, just a non-blocking nit but you are locking cs_KeyStore twice. One here, and another one inside LegacyDataSPKM::LoadKeyMetadata.

Same for LoadScriptMetadata.

furszy · 2024-07-03T20:02:21Z

src/wallet/scriptpubkeyman.cpp

+bool LegacyDataSPKM::AddKeyPubKeyInner(const CKey& key, const CPubKey& pubkey)
+{
+    LOCK(cs_KeyStore);
+    return FillableSigningProvider::AddKeyPubKey(key, pubkey);


In 7461d0c:

FillableSigningProvider::AddKeyPubKey already locks cs_Keystore internally.

DrahtBot added the Wallet label Nov 28, 2022

achow101 force-pushed the indep-desc-migrate2 branch 2 times, most recently from 9d6ce8a to 03fdbaf Compare November 29, 2022 00:44

Sjors mentioned this pull request Nov 29, 2022

Proposed Timeline for Legacy Wallet and BDB removal #20160

Closed

48 tasks

DrahtBot mentioned this pull request Nov 29, 2022

wallet: Avoid a segfault in migratewallet failure cleanup #26594

Merged

achow101 force-pushed the indep-desc-migrate2 branch from 03fdbaf to bb4ea93 Compare November 29, 2022 20:59

achow101 force-pushed the indep-desc-migrate2 branch from bb4ea93 to a8060af Compare November 29, 2022 21:22

achow101 mentioned this pull request Nov 29, 2022

wallet: Implement independent BDB parser #26606

Merged

This was referenced Nov 29, 2022

wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets #26008

Merged

wallet: rpc to add automatically generated descriptors #25907

Closed

achow101 force-pushed the indep-desc-migrate2 branch from a8060af to b29c0bf Compare November 30, 2022 00:14

achow101 force-pushed the indep-desc-migrate2 branch from b29c0bf to cfc73d1 Compare November 30, 2022 02:37

This was referenced Nov 30, 2022

rpc, wallet: addhdseed, infer seed when importing descriptor with xpub #23544

Closed

wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager #23417

Closed

rpc: add path to gethdkey #22341

Closed

achow101 force-pushed the indep-desc-migrate2 branch from cfc73d1 to 1af95bf Compare November 30, 2022 04:10

This was referenced Nov 30, 2022

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Draft

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

DrahtBot added the Needs rebase label Dec 1, 2022

achow101 force-pushed the indep-desc-migrate2 branch from 1af95bf to 928b022 Compare December 1, 2022 18:53

achow101 force-pushed the indep-desc-migrate2 branch from 5cf595a to e94b4e6 Compare June 24, 2024 16:34

achow101 mentioned this pull request Jun 24, 2024

wallet: Remove IsMine from migration code #30328

Closed

furszy reviewed Jun 26, 2024

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

Change MigrateLegacyToDescriptor to reopen wallet as BERKELEY_RO

517e204

When we reopen the wallet to do the migration, instead of opening using BDB, open it using the BerkeleyRO implementation.

achow101 force-pushed the indep-desc-migrate2 branch from e94b4e6 to 9700c21 Compare June 26, 2024 20:39

furszy reviewed Jun 26, 2024

View reviewed changes

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

DrahtBot added the CI failed label Jun 27, 2024

DrahtBot requested review from theStack and cbergqvist June 27, 2024 20:42

cbergqvist reviewed Jun 27, 2024

View reviewed changes

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

DrahtBot requested a review from cbergqvist June 27, 2024 22:27

achow101 and others added 5 commits July 1, 2024 14:24

wallet: Move LegacySPKM data storage and handling to LegacyDataSPKM

7461d0c

In order to load the necessary data for migrating a legacy wallet without the full LegacyScriptPubKeyMan, move the data storage and loading components to LegacyDataSPKM. LegacyScriptPubKeyMan now subclasses that.

wallet: Move LegacyScriptPubKeyMan::IsMine to LegacyDataSPKM

b231f4d

IsMine is necessary for migration. It should be inlined with migration when the legacy wallet is removed.

wallet: Move MigrateToDescriptor and DeleteRecords to LegacyDataSPKM

61d872f

test: verify wallet is still active post-migration failure

8ce3739

The migration process reloads the wallet after all failures. This commit tests the behavior by trying to obtain a new address after a decryption failure during migration.

achow101 force-pushed the indep-desc-migrate2 branch from 9700c21 to 8ce3739 Compare July 1, 2024 18:25

cbergqvist approved these changes Jul 1, 2024

View reviewed changes

DrahtBot requested a review from furszy July 1, 2024 22:03

DrahtBot removed the CI failed label Jul 1, 2024

willcl-ark mentioned this pull request Jul 3, 2024

Memory leak loading legacy wallet (BDB 4.8.30) #27283

Closed

theStack approved these changes Jul 3, 2024

View reviewed changes

furszy reviewed Jul 3, 2024

View reviewed changes

glozow merged commit d9aa7b2 into bitcoin:master Jul 11, 2024

theStack mentioned this pull request Jan 22, 2025

wallet: remove BDB dependency from wallet migration benchmark #31241

Merged

bitcoin locked and limited conversation to collaborators Jul 11, 2025

wallet: Migrate legacy wallets to descriptor wallets without requiring BDB #26596

wallet: Migrate legacy wallets to descriptor wallets without requiring BDB #26596

Conversation

achow101 commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Sjors commented Nov 29, 2022

Uh oh!

luke-jr commented Nov 29, 2022

Uh oh!

achow101 commented Nov 29, 2022

Uh oh!

achow101 commented Nov 29, 2022

Uh oh!

achow101 commented Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbergqvist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Jul 1, 2024

Uh oh!

cbergqvist left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

theStack Jul 3, 2024

Choose a reason for hiding this comment

Uh oh!

furszy Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

furszy Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

furszy Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

furszy Jul 3, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Nov 28, 2022 •

edited

Loading

DrahtBot commented Nov 28, 2022 •

edited

Loading

achow101 commented Jun 24, 2024 •

edited

Loading

furszy commented Jun 27, 2024 •

edited

Loading

furszy Jul 3, 2024 •

edited

Loading

furszy Jul 3, 2024 •

edited

Loading

furszy Jul 3, 2024 •

edited

Loading