Remove the legacy wallet and BDB dependency

achow101 commented at 11:36 pm on October 23, 2023: member

The final step of #20160.

A bare minimum of legacy wallet code is kept in order to perform wallet migration. Migration of legacy wallets uses the independent BDB parser and a minimal LegacyDataSPKM that allows the legacy data to be loaded so that the migration can be completed.

All tests which tested legacy wallet behavior have been removed. The --descriptors and --legacy-wallet options are removed from the functional tests.

BDB has been removed as a dependency and documentation have been updated to reflect that.

Depends on #26596, https://github.com/bitcoin-core/gui/pull/824, #30265, #30328, and #31250

DrahtBot commented at 11:36 pm on October 23, 2023: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/28710.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32071 (build: Drop option to disable hardening. by davidgumberg)
#32069 (test: fix intermittent failure in wallet_reorgsrestore.py by furszy)
#31989 (BIP-119 (OP_CHECKTEMPLATEVERIFY) (no activation) by jamesob)
#31974 (Drop testnet3 by Sjors)
#31936 (rpc: Support v3 raw transactions creation by Bue-von-hon)
#31895 (doc: Improve dependencies.md by NicolaLS)
#31868 ([IBD] specialize block serialization by l0rinc)
#31866 (test, refactor: Add TestNode.binaries to hold binary paths by ryanofsky)
#31741 (multiprocess: Add libmultiprocess git subtree by ryanofsky)
#31723 (qa debug: Add –debug_runs/-waitfordebugger [DRAFT] by hodlinator)
#31668 (Added rescan option for import descriptors by saikiran57)
#31650 (refactor: Enforce readability-avoid-const-params-in-decls by maflcko)
#31622 (psbt: add non-default sighash types to PSBTs and unify sighash type match checking by achow101)
#31519 (refactor: Use std::span over Span by maflcko)
#31453 (util: detect and warn when using exFAT on MacOS by willcl-ark)
#31423 (wallet: migration, don’t create spendable wallet from a watch-only legacy wallet by furszy)
#31404 (descriptors: inference process, do not return unparsable multisig descriptors by furszy)
#31398 (wallet: refactor: various master key encryption cleanups by theStack)
#31375 (multiprocess: Add bitcoin wrapper executable by ryanofsky)
#31296 (wallet: Translate [default wallet] string in progress messages by ryanofsky)
#31282 (refactor: Make node_id a const& in RemoveBlockRequest by maflcko)
#31275 (doc: corrected lockunspent rpc quoting by Talej)
#31244 (descriptors: MuSig2 by achow101)
#31243 (descriptor: Move filling of keys from DescriptorImpl::MakeScripts to PubkeyProvider::GetPubKey by achow101)
#30975 (ci: build multiprocess on most jobs by Sjors)
#30860 (test: autogenerate bash completion by BrandonOdiwuor)
#30595 (kernel: Introduce initial C header API by TheCharlatan)
#30437 (multiprocess: add bitcoin-mine test program by ryanofsky)
#30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
#30221 (wallet: Ensure best block matches wallet scan state by achow101)
#29694 (fuzz: wallet: add target for spkm migration by brunoerg)
#29675 (wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys by achow101)
#29652 (wallet: Avoid potentially writing incorrect best block locator by ryanofsky)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
#29136 (wallet: addhdkey RPC to add just keys to wallets via new void(KEY) descriptor by achow101)
#29124 (wallet: Automatically repair corrupted metadata with doubled derivation path by achow101)
#28944 (wallet, rpc: add anti-fee-sniping to send and sendall by ishaanam)
#28802 (ArgsManager: support subcommand-specific options by ajtowns)
#28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
#27286 (wallet: Keep track of the wallet’s own transaction outputs in memory by achow101)
#27052 (test: rpc: add last block announcement time to getpeerinfo result by LarryRuane)
#21283 (Implement BIP 370 PSBTv2 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.

achow101 force-pushed on Oct 23, 2023

DrahtBot added the label CI failed on Oct 23, 2023

achow101 force-pushed on Oct 24, 2023

DrahtBot added the label Needs rebase on Oct 25, 2023

achow101 force-pushed on Oct 25, 2023

DrahtBot removed the label Needs rebase on Oct 25, 2023

DrahtBot added the label Needs rebase on Nov 6, 2023

achow101 force-pushed on Nov 13, 2023

DrahtBot removed the label Needs rebase on Nov 13, 2023

DrahtBot added the label Needs rebase on Nov 16, 2023

achow101 force-pushed on Nov 16, 2023

DrahtBot removed the label Needs rebase on Nov 16, 2023

DrahtBot added the label Needs rebase on Nov 22, 2023

achow101 force-pushed on Nov 28, 2023

DrahtBot removed the label Needs rebase on Nov 28, 2023

DrahtBot added the label Needs rebase on Nov 28, 2023

achow101 force-pushed on Nov 28, 2023

DrahtBot removed the label Needs rebase on Nov 28, 2023

DrahtBot added the label Needs rebase on Nov 30, 2023

achow101 force-pushed on Dec 11, 2023

DrahtBot removed the label Needs rebase on Dec 11, 2023

achow101 force-pushed on Dec 11, 2023

DrahtBot added the label Needs rebase on Dec 13, 2023

achow101 force-pushed on Dec 19, 2023

DrahtBot removed the label Needs rebase on Dec 19, 2023

achow101 force-pushed on Dec 19, 2023

DrahtBot added the label Needs rebase on Jan 2, 2024

fanquake referenced this in commit 04978c2e18 on Jan 5, 2024

achow101 force-pushed on Jan 6, 2024

DrahtBot removed the label Needs rebase on Jan 6, 2024

DrahtBot added the label Needs rebase on Jan 11, 2024

achow101 force-pushed on Jan 11, 2024

DrahtBot removed the label Needs rebase on Jan 11, 2024

DrahtBot added the label Needs rebase on Jan 16, 2024

achow101 force-pushed on Jan 16, 2024

DrahtBot removed the label Needs rebase on Jan 16, 2024

DrahtBot added the label Needs rebase on Jan 17, 2024

achow101 force-pushed on Jan 25, 2024

achow101 force-pushed on Feb 1, 2024

DrahtBot removed the label Needs rebase on Feb 1, 2024

ryanofsky referenced this in commit 93e10cab5d on Feb 2, 2024

DrahtBot added the label Needs rebase on Feb 3, 2024

achow101 force-pushed on Feb 3, 2024

DrahtBot removed the label Needs rebase on Feb 3, 2024

DrahtBot added the label Needs rebase on Feb 6, 2024

achow101 force-pushed on Feb 8, 2024

DrahtBot removed the label Needs rebase on Feb 8, 2024

DrahtBot added the label Needs rebase on Feb 10, 2024

achow101 force-pushed on Feb 20, 2024

DrahtBot removed the label Needs rebase on Feb 20, 2024

DrahtBot added the label Needs rebase on Feb 26, 2024

achow101 force-pushed on Feb 29, 2024

DrahtBot removed the label Needs rebase on Feb 29, 2024

jess2505 approved

DrahtBot added the label Needs rebase on Mar 11, 2024

achow101 force-pushed on Mar 11, 2024

DrahtBot removed the label Needs rebase on Mar 11, 2024

DrahtBot added the label Needs rebase on Mar 12, 2024

achow101 force-pushed on Mar 12, 2024

DrahtBot removed the label Needs rebase on Mar 12, 2024

DrahtBot added the label Needs rebase on Mar 18, 2024

achow101 force-pushed on Mar 29, 2024

DrahtBot removed the label Needs rebase on Mar 29, 2024

DrahtBot added the label Needs rebase on Apr 1, 2024

achow101 force-pushed on Apr 1, 2024

DrahtBot removed the label Needs rebase on Apr 1, 2024

achow101 force-pushed on Apr 3, 2024

DrahtBot added the label Needs rebase on Apr 8, 2024

laanwj requested review from laanwj on Apr 9, 2024

in src/wallet/migrate.cpp:46 in 7707db3ad5 outdated

41+enum class RecordType : uint8_t
42+{
43+    KEYDATA = 1,
44+    DUPLICATE = 2,
45+    OVERFLOW_DATA = 3,
46+    DELETE = 0x80, // Indicate this record is deleted. This is AND'd with the real type.

laanwj commented at 10:26 am on April 9, 2024:

OR’ed, i guess?

achow101 force-pushed on Apr 25, 2024

achow101 force-pushed on Apr 26, 2024

achow101 force-pushed on May 21, 2024

DrahtBot removed the label Needs rebase on May 21, 2024

DrahtBot added the label Needs rebase on May 22, 2024

achow101 force-pushed on May 22, 2024

DrahtBot removed the label Needs rebase on May 22, 2024

DrahtBot added the label Needs rebase on May 23, 2024

achow101 force-pushed on May 29, 2024

achow101 force-pushed on Jun 4, 2024

DrahtBot removed the label Needs rebase on Jun 5, 2024

DrahtBot added the label Needs rebase on Jun 5, 2024

achow101 force-pushed on Jun 6, 2024

DrahtBot removed the label Needs rebase on Jun 7, 2024

achow101 force-pushed on Jun 7, 2024

achow101 force-pushed on Jun 10, 2024

DrahtBot removed the label CI failed on Jun 11, 2024

DrahtBot added the label Needs rebase on Jun 11, 2024

achow101 force-pushed on Jun 11, 2024

DrahtBot removed the label Needs rebase on Jun 11, 2024

DrahtBot added the label Needs rebase on Jun 12, 2024

achow101 force-pushed on Jun 13, 2024

DrahtBot removed the label Needs rebase on Jun 13, 2024

DrahtBot added the label CI failed on Jun 14, 2024

DrahtBot commented at 0:09 am on June 14, 2024: contributor

🚧 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/26201268639

achow101 force-pushed on Jun 14, 2024

DrahtBot removed the label CI failed on Jun 14, 2024

DrahtBot added the label Needs rebase on Jun 17, 2024

Sjors commented at 4:10 pm on June 27, 2024: member

You can also drop the BerkeleyDatabaseSanityCheck related suppression in contrib/devtools/check-devs.sh

achow101 force-pushed on Jun 27, 2024

DrahtBot removed the label Needs rebase on Jun 27, 2024

DrahtBot added the label CI failed on Jun 27, 2024

DrahtBot commented at 8:59 pm on June 27, 2024: contributor

🚧 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/26774987403

achow101 force-pushed on Jul 1, 2024

DrahtBot removed the label CI failed on Jul 1, 2024

DrahtBot added the label Needs rebase on Jul 3, 2024

achow101 force-pushed on Jul 11, 2024

DrahtBot removed the label Needs rebase on Jul 11, 2024

DrahtBot added the label Needs rebase on Jul 16, 2024

achow101 force-pushed on Jul 22, 2024

DrahtBot removed the label Needs rebase on Jul 22, 2024

DrahtBot added the label Needs rebase on Jul 25, 2024

achow101 force-pushed on Aug 29, 2024

DrahtBot removed the label Needs rebase on Aug 29, 2024

DrahtBot added the label CI failed on Aug 30, 2024

DrahtBot commented at 2:19 am on August 30, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29441840067

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly 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.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

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

DrahtBot added the label Needs rebase on Aug 31, 2024

achow101 force-pushed on Sep 3, 2024

DrahtBot removed the label Needs rebase on Sep 3, 2024

DrahtBot added the label Needs rebase on Sep 4, 2024

achow101 force-pushed on Sep 10, 2024

DrahtBot removed the label Needs rebase on Sep 10, 2024

DrahtBot added the label Needs rebase on Sep 12, 2024

achow101 force-pushed on Sep 17, 2024

DrahtBot removed the label Needs rebase on Sep 17, 2024

DrahtBot added the label Needs rebase on Sep 20, 2024

achow101 force-pushed on Oct 4, 2024

DrahtBot removed the label Needs rebase on Oct 4, 2024

achow101 force-pushed on Oct 4, 2024

DrahtBot added the label Needs rebase on Oct 5, 2024

achow101 force-pushed on Oct 11, 2024

DrahtBot removed the label Needs rebase on Oct 11, 2024

DrahtBot added the label Needs rebase on Oct 24, 2024

achow101 force-pushed on Oct 24, 2024

DrahtBot removed the label Needs rebase on Oct 24, 2024

DrahtBot removed the label CI failed on Oct 24, 2024

DrahtBot added the label Needs rebase on Oct 25, 2024

achow101 force-pushed on Oct 25, 2024

DrahtBot removed the label Needs rebase on Oct 25, 2024

DrahtBot added the label Needs rebase on Oct 28, 2024

achow101 force-pushed on Oct 28, 2024

DrahtBot removed the label Needs rebase on Oct 28, 2024

DrahtBot added the label Needs rebase on Oct 29, 2024

achow101 force-pushed on Oct 29, 2024

DrahtBot removed the label Needs rebase on Oct 29, 2024

DrahtBot added the label Needs rebase on Nov 1, 2024

achow101 force-pushed on Nov 1, 2024

DrahtBot removed the label Needs rebase on Nov 1, 2024

murchandamus commented at 9:43 pm on November 5, 2024: contributor

It looks like all three dependencies got merged, is this ready for review?

achow101 commented at 10:48 pm on November 5, 2024: member

It looks like all three dependencies got merged, is this ready for review?

Currently it is still dependent on #30328 but I suppose it doesn’t have to be.

DrahtBot added the label Needs rebase on Nov 6, 2024

achow101 force-pushed on Nov 6, 2024

DrahtBot added the label CI failed on Nov 6, 2024

DrahtBot removed the label Needs rebase on Nov 6, 2024

achow101 force-pushed on Nov 7, 2024

DrahtBot added the label Needs rebase on Nov 11, 2024

fanquake referenced this in commit 2b33322169 on Nov 12, 2024

achow101 force-pushed on Nov 13, 2024

DrahtBot removed the label Needs rebase on Nov 13, 2024

DrahtBot added the label Needs rebase on Nov 15, 2024

in src/wallet/wallet.cpp:3479 in 323bc8982a outdated

3487@@ -3684,9 +3488,8 @@ void CWallet::SetupLegacyScriptPubKeyMan()
3488         return;
3489     }
3490 
3491-    std::unique_ptr<ScriptPubKeyMan> spk_manager = m_database->Format() == "bdb_ro" ?
3492-        std::make_unique<LegacyDataSPKM>(*this) :
3493-        std::make_unique<LegacyScriptPubKeyMan>(*this, m_keypool_size);
3494+    Assert(m_database->Format() == "bdb_ro");
3495+    std::unique_ptr<ScriptPubKeyMan> spk_manager = std::make_unique<LegacyDataSPKM>(*this);

furszy commented at 4:00 pm on November 23, 2024:

In order to continue testing this without manually writing the bdb file, we need to allow the ‘mock’ db format here:

0const std::string& db_format{m_database->Format()};
1Assert(db_format == "bdb_ro" || db_format == "mock");

ryanofsky referenced this in commit 2eccb8bc5e on Dec 5, 2024

achow101 force-pushed on Dec 9, 2024

DrahtBot removed the label Needs rebase on Dec 9, 2024

DrahtBot removed the label CI failed on Dec 10, 2024

achow101 force-pushed on Dec 13, 2024

achow101 force-pushed on Dec 16, 2024

in src/wallet/scriptpubkeyman.cpp:84 in e6ccd17275 outdated

80@@ -79,7 +81,7 @@ bool HaveKeys(const std::vector<valtype>& pubkeys, const LegacyDataSPKM& keystor
81 //!                            scripts or simply treat any script that has been
82 //!                            stored in the keystore as spendable
83 // NOLINTNEXTLINE(misc-no-recursion)
84-IsMineResult IsMineInner(const LegacyDataSPKM& keystore, const CScript& scriptPubKey, IsMineSigVersion sigversion, bool recurse_scripthash=true)
85+IsMineResult LegacyWalletIsMineInnerDONOTUSE(const LegacyDataSPKM& keystore, const CScript& scriptPubKey, IsMineSigVersion sigversion, bool recurse_scripthash=true)

Sjors commented at 2:20 pm on January 13, 2025:

e6ccd17275e06e6278473587257f80c2d9bd8834: maybe move the legacy wallet migration code to a separate file? And mark it read-only :-)

e.g. https://github.com/Sjors/bitcoin/commit/364088a306f8de9ece2c5201dbe48a252c39ea03

DrahtBot added the label Needs rebase on Jan 17, 2025

achow101 force-pushed on Jan 20, 2025

DrahtBot removed the label Needs rebase on Jan 20, 2025

DrahtBot added the label CI failed on Jan 21, 2025

DrahtBot added the label Needs rebase on Jan 21, 2025

achow101 force-pushed on Jan 21, 2025

DrahtBot removed the label Needs rebase on Jan 21, 2025

DrahtBot removed the label CI failed on Jan 21, 2025

DrahtBot added the label Needs rebase on Jan 23, 2025

achow101 force-pushed on Feb 1, 2025

DrahtBot added the label CI failed on Feb 1, 2025

DrahtBot commented at 3:36 am on February 1, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/36513189138

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly 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.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

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

DrahtBot removed the label Needs rebase on Feb 4, 2025

bitcoin deleted a comment on Feb 9, 2025

achow101 force-pushed on Feb 10, 2025

DrahtBot removed the label CI failed on Feb 10, 2025

glozow referenced this in commit 96d30ed4f9 on Feb 13, 2025

DrahtBot added the label Needs rebase on Feb 13, 2025

achow101 force-pushed on Feb 13, 2025

DrahtBot removed the label Needs rebase on Feb 13, 2025

DrahtBot added the label Needs rebase on Feb 14, 2025

achow101 force-pushed on Feb 14, 2025

DrahtBot removed the label Needs rebase on Feb 14, 2025

achow101 force-pushed on Feb 14, 2025

achow101 force-pushed on Feb 19, 2025

DrahtBot added the label Needs rebase on Feb 20, 2025

achow101 force-pushed on Mar 5, 2025

DrahtBot commented at 8:28 pm on March 5, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/38265463467

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly 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.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

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

DrahtBot added the label CI failed on Mar 5, 2025

DrahtBot removed the label Needs rebase on Mar 5, 2025

DrahtBot removed the label CI failed on Mar 5, 2025

DrahtBot added the label Needs rebase on Mar 7, 2025

achow101 force-pushed on Mar 7, 2025

DrahtBot removed the label Needs rebase on Mar 7, 2025

DrahtBot added the label Needs rebase on Mar 14, 2025

test: rpcs disabled for descriptor wallets were removed e6705a5ac2

test: Remove legacy wallet unit tests eca5c625d3

test: wallet_transactiontime_rescan importdescriptors always rescans

Since importdescriptors (which the legacy wallet import* RPCs use for
descriptor wallets in the test framework) always rescans, the checks
in wallet_transactiontime_rescan for import* RPCs not rescanning, and
subsequently requring a rescan, are incorrect.

70b1e126b3

test: wallet_signer.py bdb will be removed ab76c33c10

test: Remove legacy wallet tests from wallet_backwards_compatibility.py 99b26a6358

test: Remove legacy wallet tests from wallet_reindex.py db131f6665

test: remove legacy wallet functional tests

Removes all legacy wallet specific functional tests.

Also removes the --descriptor and --legacy-wallet options as these are
no longer necessary with the legacy wallet removed.

c25d9dd011

wallet: Remove -format and bdb from wallet tool's createfromdump 024470c3f0

wallet: Remove wallettool salvage

Salvage is bdb only which is about to be removed.

2000bba3d0

bench: Remove WalletLoadingLegacy benchmark 373afc5d88

wallet: Disallow legacy wallets

Legacy wallets do not have the descriptors flag set. Don't load wallets
without the descriptors flag.

At the same time, we will no longer load BDB databases since they are
only used for legacy wallets.

32a522f2d8

build, wallet, doc: Remove BDB 923bfc0d4a

wallet, rpc: Remove legacy wallet only RPCs 365e056768

wallet: Delete LegacySPKM

Deletes LegacyScriptPubKeyMan and related tests

0dc74d6665

wallet: Remove unused db functions

SOme db functions were for BDB, these are no longer needed.

56f2137215

legacy spkm: Make IsMine() and CanProvide() private and migration only 8944e40139

achow101 force-pushed on Mar 14, 2025

DrahtBot removed the label Needs rebase on Mar 14, 2025

DrahtBot added the label Needs rebase on Mar 16, 2025

DrahtBot commented at 4:29 pm on March 16, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Remove the legacy wallet and BDB dependency #28710

Code Coverage & Benchmarks

Reviews

Conflicts