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:

#31617 (build, test: Build db_tests.cpp regardless of USE_BDB by hebasto)
#31613 (depends, NetBSD: Fix bdb package compilation with GCC-14 by hebasto)
#31603 (descriptor: check whitespace in ParsePubkeyInner by brunoerg)
#31519 (refactor: Use std::span over Span by maflcko)
#31507 ([POC] build: Use clang-cl to build on Windows natively by hebasto)
#31495 (wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases by achow101)
#31453 (util: detect and warn when using exFAT on MacOS by willcl-ark)
#31451 (wallet: migration, avoid loading legacy wallet after failure when BDB isn’t compiled by furszy)
#31423 (wallet: migration, don’t create spendable wallet from a watch-only legacy wallet by furszy)
#31416 (doc: Fix incorrect send RPC docs by maflcko)
#31413 (rpc: Remove deprecated dummy alias for listtransactions::label by maflcko)
#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)
#31353 (rpc, cli: add getbalances#total, and use it for -getinfo by jonatack)
#31296 (wallet: Translate [default wallet] string in progress messages by ryanofsky)
#31282 (refactor: Make node_id a const& in RemoveBlockRequest by maflcko)
#31278 (wallet, rpc: Settxfeerate by polespinasa)
#31275 (doc: corrected lockunspent rpc quoting by Talej)
#31250 (wallet: Disable creating and loading legacy wallets by achow101)
#31244 (descriptors: MuSig2 by achow101)
#31243 (descriptor: Move filling of keys from DescriptorImpl::MakeScripts to PubkeyProvider::GetPubKey by achow101)
#31242 (wallet, desc spkm: Return SigningProvider only if we have the privkey by achow101)
#31241 (wallet: remove BDB dependency from wallet migration benchmark by furszy)
#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)
#31061 (refactor: Check translatable format strings at compile-time by maflcko)
#30997 (build: Switch to Qt 6 by hebasto)
#30975 (Add multiprocess binaries to release build by Sjors)
#30860 (test: autogenerate bash completion by BrandonOdiwuor)
#30844 (RPC: improve SFFO arg parsing, error catching and coverage by furszy)
#30727 (rpc: add address_type field in getaddressinfo by jonatack)
#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)
#30125 (test: improve BDB parser (handle internal/overflow pages, support all page sizes) by theStack)
#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)
#29156 (tests: add functional test for miniscript decaying multisig by mjdietzx)
#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)
#28724 (wallet: Cleanup accidental encryption keys in watchonly wallets by achow101)
#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)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#21283 (Implement BIP 370 PSBTv2 by achow101)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
#10102 (Multiprocess bitcoin by ryanofsky)

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:3465 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

wallet: remove BDB dependency from wallet migration benchmark

Stops creating a bdb database in the wallet migration benchmark.
Instead, the benchmark now creates the db in memory and re-uses
it for the migration process.

24a7845b2b

test: rpcs disabled for descriptor wallets were removed f8ae3e5a2c

test: Remove legacy wallet unit tests 550c185fce

test: wallet_transactiontime_rescan importdescriptors always rescans b45a6ded81

test: wallet_signer.py bdb will be removed d471ae1474

test: Remove legacy wallet tests from wallet_backwards_compatibility.py 2fc4d37870

test: Remove legacy wallet tests from wallet_reindex.py 8fff847fe9

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.

6f4f32d226

wallet: Remove -format and bdb from wallet tool's createfromdump 6f3b8e7460

wallet: Remove wallettool salvage

Salvage is bdb only which is about to be removed.

f0c24903f4

bench: Remove WalletLoadingLegacy benchmark f20f8f83a9

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.

2cb846b156

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.

e35ee39ad9

test: Extra verification that migratewallet migrates f7126c40b2

tests: Test migration of additional P2WSH scripts 2e1f62d9a4

legacy spkm: Move CanProvide to LegacyDataSPKM

This function will be needed in migration

e17d1e766f

legacy spkm: use IsMine() to extract watched output scripts

Instead of (partially) trying to reverse IsMine() to get the output
scripts that a LegacySPKM would track, we can preserve it in migration
only code and utilize it to get an accurate set of output scripts.

This is accomplished by computing a set of output script candidates from
map(Crypted)Keys, mapScripts, and setWatchOnly. This candidate set is an
upper bound on the scripts tracked by the wallet. Then IsMine() is used
to filter to the exact output scripts that LegacySPKM would track.

By changing GetScriptPubKeys() this way, we can avoid complexities in
reversing IsMine() and get a more complete set of output scripts.

b496481ad1

migration: Skip descriptors which do not parse

InferDescriptors can sometimes make descriptors which are actually
invalid and cannot be parsed. Detect and skip such descriptors by doing
a Parse() check before adding the descriptor to the wallet.

f318bb8c0b

wallet migration: Determine Solvables with CanProvide

LegacySPKM would determine whether it could provide any script data to a
transaction through the use of the CanProvide function. Instead of
partially reversing signing logic to figure out the output scripts of
solvable things, we use the same candidate set approach in
GetScriptPubKeys() and instead filter the candidate set first for
things that are ISMINE_NO, and second with CanProvide(). This should
give a more accurate solvables wallet.

dc11879cac

test: Test migration of miniscript in legacy wallets 624252ac14

test: Test migration of taproot output scripts 47672e5f96

test: Test migration of a solvable script with no privkeys

The legacy wallet will be able to solve output scripts where the
redeemScript or witnessScript is known, but does not know any of the
private keys involved in that script. These should be migrated to the
solvables wallet.

20219382d1

build, wallet, doc: Remove BDB f47a47787d

wallet, rpc: Remove legacy wallet only RPCs c1f640f903

wallet: Delete LegacySPKM

Deletes LegacyScriptPubKeyMan and related tests

29478596ba

wallet: Remove unused db functions

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

feaca4d974

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

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

Remove the legacy wallet and BDB dependency #28710

Code Coverage & Benchmarks

Reviews

Conflicts