wallet: Disable creating and loading legacy wallets

achow101 commented at 6:42 pm on November 7, 2024: member

To prepare for the deletion of legacy wallet code, disable creating or loading new legacy wallets.

Tests for the legacy wallet specifically are deleted.

Depends on #31248 and #31241

Split from https://github.com/bitcoin/bitcoin/pull/28710

DrahtBot commented at 6:42 pm on November 7, 2024: 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/31250.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko

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:

#31866 (test, refactor: Add TestNode.binaries to hold binary paths by ryanofsky)
#31723 (qa debug: Add –debug_runs/-waitfordebugger [DRAFT] by hodlinator)
#31649 (consensus: Remove checkpoints (take 2) by marcofleon)
#31519 (refactor: Use std::span over Span by maflcko)
#31423 (wallet: migration, don’t create spendable wallet from a watch-only legacy wallet by furszy)
#31375 (multiprocess: Add bitcoin wrapper executable by ryanofsky)
#31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
#30860 (test: autogenerate bash completion by BrandonOdiwuor)
#29500 (test: create assert_not_equal util by kevkevinpal)
#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)
#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)

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.

DrahtBot added the label Wallet on Nov 7, 2024

achow101 force-pushed on Nov 7, 2024

DrahtBot added the label CI failed on Nov 7, 2024

DrahtBot added the label Needs rebase on Nov 12, 2024

achow101 force-pushed on Nov 12, 2024

DrahtBot removed the label CI failed on Nov 12, 2024

DrahtBot removed the label Needs rebase on Nov 12, 2024

DrahtBot added the label Needs rebase on Nov 19, 2024

achow101 force-pushed on Nov 20, 2024

DrahtBot removed the label Needs rebase on Nov 20, 2024

DrahtBot added the label Needs rebase on Dec 2, 2024

achow101 force-pushed on Dec 5, 2024

DrahtBot removed the label Needs rebase on Dec 5, 2024

DrahtBot added the label Needs rebase on Dec 6, 2024

achow101 force-pushed on Dec 9, 2024

DrahtBot removed the label Needs rebase on Dec 9, 2024

DrahtBot added the label Needs rebase on Jan 10, 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

achow101 force-pushed on Jan 21, 2025

DrahtBot removed the label CI failed on Jan 21, 2025

DrahtBot added the label Needs rebase on Jan 25, 2025

achow101 force-pushed on Feb 1, 2025

achow101 commented at 0:35 am on February 1, 2025: member

Marking this as ready for review as it no longer has any prerequisites.

achow101 marked this as ready for review on Feb 1, 2025

achow101 force-pushed on Feb 1, 2025

DrahtBot commented at 0:38 am on February 1, 2025: contributor

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

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 Feb 1, 2025

DrahtBot removed the label Needs rebase on Feb 1, 2025

Armss9936 approved

achow101 force-pushed on Feb 10, 2025

DrahtBot removed the label CI failed on Feb 10, 2025

DrahtBot added the label Needs rebase on Feb 14, 2025

in src/bench/wallet_ismine.cpp:74 in 321b1cf376 outdated

74-static void WalletIsMineLegacy(benchmark::Bench& bench) { WalletIsMine(bench, /*legacy_wallet=*/true); }
75-BENCHMARK(WalletIsMineLegacy, benchmark::PriorityLevel::LOW);
76-#endif
77-
78 #ifdef USE_SQLITE
79 static void WalletIsMineDescriptors(benchmark::Bench& bench) { WalletIsMine(bench, /*legacy_wallet=*/false); }

maflcko commented at 7:44 am on February 18, 2025:

nit in 321b1cf376865ecc720bbd32357553d508704a35: Should remove the redundant bool now as well?

achow101 commented at 6:22 pm on February 18, 2025:

Done

in src/qt/test/wallettests.cpp:400 in 321b1cf376 outdated

375@@ -395,56 +376,6 @@ void TestGUI(interfaces::Node& node, const std::shared_ptr<CWallet>& wallet)
376     QCOMPARE(walletModel.wallet().getAddressReceiveRequests().size(), size_t{0});
377 }
378 
379-void TestGUIWatchOnly(interfaces::Node& node, TestChain100Setup& test)
380-{
381-    const std::shared_ptr<CWallet>& wallet = SetupLegacyWatchOnlyWallet(node, test);

maflcko commented at 7:51 am on February 18, 2025:

nit in https://github.com/bitcoin/bitcoin/commit/321b1cf376865ecc720bbd32357553d508704a35: Should the test be kept and be done with a descriptor wallet instead?

achow101 commented at 6:07 pm on February 18, 2025:

No, watch-only descriptor wallets do not change the GUI as watch-only legacy wallets did.

in test/functional/wallet_transactiontime_rescan.py:147 in bb6a15173e outdated

143@@ -144,20 +144,6 @@ def run_test(self):
144         restorewo_wallet.importaddress(wo2, rescan=False)
145         restorewo_wallet.importaddress(wo3, rescan=False)
146 
147-        # check user has 0 balance and no transactions

maflcko commented at 8:12 am on February 18, 2025:

q in bb6a15173eef114c60e620b431a4306f8de213d0: I don’t understand this commit. It simply states something and removes code, but I don’t see the connection. Maybe a note could help to explain why and when this is needed?

achow101 commented at 6:10 pm on February 18, 2025:

importaddress calls importdescriptors in the test framework. These checks are that importaddress (the legacy wallet RPC) doesn’t rescan and will require a separate rescan. But that will not work once the tests is descriptor wallets only.

Will expand the commit message.

maflcko commented at 8:30 am on February 20, 2025:

Will expand the commit message.

Thanks for explaining. I understand that the test framework calls importdescriptors on importaddress internally. However, if the checks are wrong, why does the test not fail on current master?

In fact there is a comment and workaround to address this specifically in this test:

0        # for descriptor wallets, the test framework maps the importaddress RPC to the
1        # importdescriptors RPC (with argument 'timestamp'='now'), which always rescans
2        # blocks of the past 2 hours, based on the current MTP timestamp; in order to avoid
3        # importing the last address (wo3), we advance the time further and generate 10 blocks

So it seems the workaround is not working?

in test/functional/wallet_backwards_compatibility.py:1 in 0b84b25e92 outdated

maflcko commented at 8:17 am on February 18, 2025:

nit in 0b84b25e92d382ace40f8b52b80925b3fe76e5f3: Forgot to remove unused legacy_nodes?

achow101 commented at 6:22 pm on February 18, 2025:

Done

Edit: It’s not unused.

in test/functional/wallet_reindex.py:21 in 69eb2c4d85 outdated

19-        self.add_wallet_options(parser)
20-
21     def set_test_params(self):
22         self.num_nodes = 1
23         self.setup_clean_chain = True
24+        self.uses_wallet = True

maflcko commented at 8:30 am on February 18, 2025:

nit in 69eb2c4d85098acbad6b89a09fae4ca84e81fff5: Why set to true redundantly, when no other test does this and when skip_if_no_wallet already does it?

achow101 commented at 6:22 pm on February 18, 2025:

Removed

in src/wallet/dump.cpp:183 in f19592a060 outdated

197+    // Make sure that the dump was created from a sqlite database only as that is the only
198+    // type of database that we still support.
199+    // Other formats such as BDB should not be loaded into a sqlite database since they also
200+    // use a different type of wallet entirely which is no longer compatible with this software.
201+    if (format_value != "sqlite") {
202+        error = _("Error: Dumpfile specifies an unsupported database format");

maflcko commented at 8:39 am on February 18, 2025:

nit in f19592a06059b44d55838bf2f8c63fde2917ee83: Could extend the error msg? Something like:

"Error: Dumpfile specifies an unsupported database format (%s). Only sqlite database dumps are supported."?

achow101 commented at 6:22 pm on February 18, 2025:

Done

maflcko added the label Needs release note on Feb 18, 2025

in CMakeLists.txt:647 in eb56556ca4 outdated

623@@ -626,7 +624,6 @@ message("  libbitcoinkernel (experimental) ..... ${BUILD_KERNEL_LIB}")
624 message("Optional features:")
625 message("  wallet support ...................... ${ENABLE_WALLET}")
626 if(ENABLE_WALLET)
627-  message("   - descriptor wallets (SQLite) ...... ${WITH_SQLITE}")
628   message("   - legacy wallets (Berkeley DB) ..... ${WITH_BDB}")

maflcko commented at 8:46 am on February 18, 2025:

eb56556ca4a9104e22cec70698ffceb22719894c: This removes sqlite, but it would be better to remove bdb? Also the commit message needs fixup.

achow101 commented at 6:21 pm on February 18, 2025:

It only removes sqlite as an optional dependency when the wallet is enabled, so I think having this log line is redundant. Removing the BDB line is still incorrect at this time as it can be built with and used (in this commit).

I’m not sure what in the commit message needs fixing.

maflcko commented at 8:38 am on February 20, 2025:

I’m not sure what in the commit message needs fixing.

The commit message refers to the configure option, but the project is using cmake, but this is just a nit.

maflcko commented at 8:59 am on February 20, 2025:

Also, this is the only build-system commit in this pull request. I’d say it could make sense to split it up into it’s own pull request, because:

This allows the “build people” to review it, if they want, and allows them to skip over the test-only bulk changes in this pull.
The change to imply with_sqlite on enable_wallet is fine and required anyway going forward. I don’t see a use-case for a bdb-only build at this point, so a separate pull should be fine.

maflcko approved

maflcko commented at 8:48 am on February 18, 2025: member

Needs rebase.

I left some nits, feel free to ignore.

I skipped over the large commit and the last two.

Otherwise:

review ACK 82fc65c9ab53c416935b5eb20be10fe69ded8b88 👨

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 82fc65c9ab53c416935b5eb20be10fe69ded8b88 👨
31738Jue8VRvURHJG6ZrNb0UzewDcwNZT7SO5vi0tlLfJlYOf/7IVJSk5W9mompg0cCbyr9xc3mF6TE876MUeBg==

achow101 force-pushed on Feb 18, 2025

DrahtBot added the label CI failed on Feb 18, 2025

DrahtBot commented at 7:16 pm on February 18, 2025: contributor

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

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 18, 2025

achow101 force-pushed on Feb 18, 2025

DrahtBot removed the label CI failed on Feb 18, 2025

DrahtBot added the label Needs rebase on Feb 19, 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

achow101 force-pushed on Feb 19, 2025

DrahtBot removed the label Needs rebase on Feb 19, 2025

maflcko removed the label Needs release note on Feb 20, 2025

maflcko approved

maflcko commented at 9:05 am on February 20, 2025: member

Clarified my nits a bit, nothing blocking.

I still haven’t reviewed the last commit and the large commit. I wonder if it is possible to split the large commit into one that removes test files (or test cases) and one that does the test framework change, possibly making review easier.

re-ACK b8494154e174481bbc0350013f1960973f35f3fc 🔑

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK b8494154e174481bbc0350013f1960973f35f3fc 🔑
3FHB3gaGmlXUnVYceoczXV52txwmCFbqMqxMuJLOB1MC2BCTd8xLZoCVRxUH3Kz+37u17PYG18Jd3OynL2tgbDg==

wallet: Disable creating and loading legacy wallets #31250

Code Coverage & Benchmarks

Reviews

Conflicts