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:

#31895 (doc: Improve dependencies.md by NicolaLS)
#31866 (test, refactor: Add TestNode.binaries to hold binary paths by ryanofsky)
#31802 (Add bitcoin-{node,gui} to release binaries for IPC by Sjors)
#31741 (multiprocess: Add libmultiprocess git subtree by ryanofsky)
#31731 (doc: update translation generation cmake example by l0rinc)
#31723 (qa debug: Add –debug_runs/-waitfordebugger [DRAFT] by hodlinator)
#31668 (Added rescan option for import descriptors by saikiran57)
#31665 (build: Make config warnings fatal if -DWCONFIGURE_ERROR=ON by davidgumberg)
#31650 (refactor: Enforce readability-avoid-const-params-in-decls by maflcko)
#31649 (consensus: Remove checkpoints (take 2) by marcofleon)
#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)
#31360 (depends: Avoid using helper variables in toolchain file by hebasto)
#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)
#31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
#31244 (descriptors: MuSig2 by achow101)
#31243 (descriptor: Move filling of keys from DescriptorImpl::MakeScripts to PubkeyProvider::GetPubKey by achow101)
#31176 (ci: Test cross-built Windows executables on Windows natively by hebasto)
#31158 (build, ci: Fix linking bitcoin-chainstate.exe to bitcoinkernel.dll on Windows by hebasto)
#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)
#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)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
#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)
#27865 (wallet: Track no-longer-spendable TXOs separately 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:3473 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

test: rpcs disabled for descriptor wallets were removed 2fa6619ef7

test: Remove legacy wallet unit tests bfe204cb8c

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.

e52deb0aff

test: wallet_signer.py bdb will be removed 52c6245e98

test: Remove legacy wallet tests from wallet_backwards_compatibility.py da2e286cc9

test: Remove legacy wallet tests from wallet_reindex.py 5fe6637e3f

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.

fda935a5ac

wallet: Remove -format and bdb from wallet tool's createfromdump 77b3b6cd37

wallet: Remove wallettool salvage

Salvage is bdb only which is about to be removed.

c3020a759e

bench: Remove WalletLoadingLegacy benchmark 281ea1a605

achow101 force-pushed on Feb 19, 2025

build: Require sqlite when --enable-wallet

Require sqlite is available in order to compile the wallet. Removes
instances of USE_SQLITE since it is no longer possible to not have
sqlite available.

189eb8c02b

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.

b8494154e1

build, wallet, doc: Remove BDB 393c05a732

wallet, rpc: Remove legacy wallet only RPCs 6491e81ea8

wallet: Delete LegacySPKM

Deletes LegacyScriptPubKeyMan and related tests

bdb4d7f8d9

wallet: Remove unused db functions

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

b0821fc9d8

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

achow101 force-pushed on Feb 19, 2025

DrahtBot added the label Needs rebase on Feb 20, 2025

DrahtBot commented at 5:29 pm on February 20, 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