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, and #30265

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

For detailed information about the code coverage, see the test coverage report.

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:

#bitcoin-core/gui/824 (Migrate legacy wallets that are not loaded by achow101)
#bitcoin-core/gui/792 (Correct tooltip wording for watch-only wallets by hernanmarino)
#bitcoin-core/gui/753 (Add new “address type” column to the “receiving tab” address book page by pablomartin4btc)
#bitcoin-core/gui/733 (Deniability - a tool to automatically improve coin ownership privacy by denavila)
#30352 (policy: Add OP_TRUE <0x4e73> as a standard output script by instagibbs)
#30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
#30328 (wallet: Remove IsMine from migration code by achow101)
#30277 ([DO NOT MERGE] Erlay: bandwidth-efficient transaction relay protocol (Full implementation) by sr-gi)
#30272 (doc: use TRUC instead of v3 and add release note by glozow)
#30265 (wallet: Fix listwalletdir listing of migrated default wallets and generated backup files by achow101)
#30221 (wallet: Ensure best block matches wallet scan state by achow101)
#30212 (rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN by willcl-ark)
#30125 (test: improve BDB parser (handle internal/overflow pages, support all page sizes) by theStack)
#30043 (net: Replace libnatpmp with built-in PCP+NATPMP implementation by laanwj)
#29936 (fuzz: wallet: add target for CreateTransaction by brunoerg)
#29775 (Testnet4 including PoW difficulty adjustment fix by fjahr)
#29652 (wallet: Avoid potentially writing incorrect best block locator by ryanofsky)
#29500 (test: create assert_not_equal util by kevkevinpal)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
#29280 (Implement OP_CHECKTEMPLATEVERIFY by reardencode)
#29221 (Implement 64 bit arithmetic op codes in the Script interpreter by Christewart)
#29175 (rpc: validate fee estimation mode case insensitive by torkelrogstad)
#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)
#29129 (wallet, rpc: add BIP44 account in createwallet by brunoerg)
#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)
#28574 (wallet: optimize migration process, batch db transactions by furszy)
#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)
#26596 (wallet: Migrate legacy wallets to descriptor wallets without requiring BDB by achow101)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)
#22729 (Make it possible to disable Tor binds and abort startup on bind failure by vasild)
#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

Change MigrateLegacyToDescriptor to reopen wallet as BERKELEY_RO

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

a4d2de802b

wallet: Move LegacySPKM data storage and handling to LegacyDataSPKM

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.

04e14d69d2

wallet: Move MigrateToLegacy and DeleteRecords to LegacyDataSPKM 8818c74a3d

wallet: Remove IsMine from migration

As IsMine will be removed, the relevant components of IsMine are inlined
into the migration functions.

6f279d5930

wallet: Use LegacyDataSPKM when loading

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.

2ccbd09b65

wallet, interfaces: Include database format in listWalletDir 1078e27c88

gui: Consolidate wallet display name to GUIUtil function

Instead of having the code for the wallet display name being copy and
pasted, use a GUIUtil function to get that for us.

63c2bf3f01

gui: Use wallet name for wallet migration rather than WalletModel

To prepare for migrating wallets that are not loaded, when migration
occurs in the GUI, it should not rely on a WalletModel existing.

87cd3ab695

gui: Use menu for wallet migration

Once legacy wallets can no longer be loaded, we need to be able to
migrate them without loading. Thus we should use a menu that lists the
wallets in the wallet directory instead of an action which migrates the
currently loaded wallet.

ae3feb043c

wallet: Ignore .bak files when listing wallet files

Migration creates backup files in the wallet directory with .bak as the
extension. This pollutes the output of listwalletdir with backup files
that most users should not need to care about.

4cba793ec0

wallet: List sqlite wallets with empty string name

Although it is not explicitly possible to create a default wallet with
descriptors, it is possible to migrate a default wallet and have it end
up being a default wallet with descriptors. These wallets should be
listed by ListDatabases so that it appears in wallet directory listings
to avoid user confusion.

85023fd2fd

test: rpcs disabled for descriptor wallets were removed 7707f0c6d7

test: Add combinerawtransaction test to rpc_createmultisig

The only coverage of combinerawtransaction is in a legacy wallet only
test. So also use it in rpc_createmultisig so that this RPC remains
tested after the legacy wallet is removed.

41ddc7f24b

test: Remove legacy wallet unit tests d4a91ba9cd

test: wallet_transactiontime_rescan importdescriptors always rescans 3501fd5823

test: wallet_signer.py bdb will be removed e229211ea3

test: Rework migratewallet to use previous releases b1b4310d03

test: Remove legacy wallet tests from wallet_backwards_compatibility.py 8b65dd4984

test: Remove legacy wallet tests from wallet_reindex.py 95401013c5

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.

a0fda2cbe0

wallet: Remove -format and bdb from wallet tool's createfromdump 9a73c31a98

wallet: Remove wallettool salvage

Salvage is bdb only which is about to be removed.

09b0b76dd2

bench: Remove WalletLoadingLegacy benchmark e7fbd516d3

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.

fa0dbf78a3

wallet: Don't reload legacy wallet on failed migration

We can't load legacy wallet anymore, so if migration fails, don't try to
load the failed wallet.

36970cf79d

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.

5497783778

build, wallet, doc: Remove BDB 1cc4884406

wallet, rpc: Remove legacy wallet only RPCs f7660119ce

wallet: Delete LegacySPKM

Deletes LegacyScriptPubKeyMan and related tests

18380e4972

wallet: Remove unused db functions

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

9cf8e24ac0

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