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	Sjors, brunoerg
Concept ACK	pablomartin4btc
Stale 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:

#32069 (test: fix intermittent failure in wallet_reorgsrestore.py by furszy)
#31974 (Drop testnet3 by Sjors)
#31936 (rpc: Support v3 raw transactions creation by Bue-von-hon)
#31868 ([IBD] specialize block serialization by l0rinc)
#31866 (test, refactor: Add TestNode.binaries to hold binary paths by ryanofsky)
#31723 (qa debug: Add –debug_runs/-waitfordebugger [DRAFT] by hodlinator)
#31668 (Added rescan option for import descriptors by saikiran57)
#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)
#30860 (test: autogenerate bash completion by BrandonOdiwuor)
#30221 (wallet: Ensure best block matches wallet scan state 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)
#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.

Sjors commented at 2:17 pm on March 7, 2025:

Testing the PSBT workflow would be still be useful though.

In 801d2b1c3f25c6922df04cce4b2baa2047173eab “test: Remove legacy wallet unit tests”

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:653 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.

Sjors commented at 8:29 pm on February 27, 2025:

Done in #31961

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

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==

Sjors commented at 7:43 pm on February 27, 2025: member

At first glance it makes sense to make 189eb8c02b5b81fe3239df34cd9ef48871e2e5de its own PR, i.e. making sqlite mandatory for wallet building (done in #31961). See #31250 (review)

achow101 force-pushed on Mar 5, 2025

DrahtBot added the label CI failed 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/38265204038

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 CI failed on Mar 5, 2025

in test/functional/test_runner.py:89 in 56e9bef655 outdated

85@@ -86,7 +86,6 @@
86     'feature_pruning.py',
87     'feature_dbcrash.py',
88     'feature_index_prune.py',
89-    'wallet_pruning.py --legacy-wallet',

brunoerg commented at 1:12 pm on March 6, 2025:

56e9bef655673becb3e265948143a9e860a5c68b: nit: could also update the example in test_runner.py:

0for exclude_test in exclude_tests:
1    # A space in the name indicates it has arguments such as "wallet_basic.py --descriptors"
2    if ' ' in exclude_test:

in src/wallet/wallet.cpp:3056 in ba2042c31f outdated

3048@@ -3049,7 +3049,11 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
3049             error = strprintf(_("Unexpected legacy entry in descriptor wallet found. Loading wallet %s\n\n"
3050                                 "The wallet might have been tampered with or created with malicious intent.\n"), walletFile);
3051             return nullptr;
3052-        } else {
3053+        } else if (nLoadWalletRet == DBErrors::LEGACY_WALLET) {
3054+            error = strprintf(_("Error loading %s: Wallet is a legacy wallet. Please migrate to a descriptor wallet using the migration tool (migratewallet RPC)."), walletFile);

brunoerg commented at 2:19 pm on March 6, 2025:

ba2042c31f1630678a1f08bb68e01c1d98cc9abc: Is this verification reachable? The scenario I can see is: someone unloaded the wallet using an old version, upgraded and then tried to load it again. However, before reaching here it would fail during the database creation (MakeDatabase).

achow101 commented at 4:52 pm on March 6, 2025:

There is a theoretical legacy-sqlite wallet configuration that could reach this. This is actually an unsupported configuration, although it technically works today.

brunoerg commented at 5:07 pm on March 6, 2025:

Got it, thanks.

in test/functional/wallet_descriptor.py:152 in 8afd005cbf outdated

147@@ -148,18 +148,6 @@ def run_test(self):
148         addr = recv_wrpc.getnewaddress()
149         send_wrpc.sendtoaddress(addr, 10)
150 
151-        # Make sure things are disabled
152-        self.log.info("Test disabled RPCs")

Sjors commented at 2:12 pm on March 7, 2025:

In 8afd005cbf9209e90a5cc1bcdb2c0ff400c2de33 “test: rpcs disabled for descriptor wallets were removed”: I’m confused by the commit message. Should this commit go later? Or maybe reword it as “will go away in the next commits”.

At this commit they still exist and there’s other tests that refer to them, e.g. rpcnestedtests.cpp

in src/wallet/test/db_tests.cpp:64 in 801d2b1c3f outdated

129-#ifdef USE_BDB
130-    dbs.emplace_back(MakeBerkeleyDatabase(path_root / "bdb", options, status, error));
131-    // Needs BDB to make the DB to read
132-    dbs.emplace_back(std::make_unique<BerkeleyRODatabase>(BDBDataFile(path_root / "bdb"), /*open=*/false));
133-#endif
134+    // Unable to test BerkeleyRO since we cannot create a new BDB database to open

Sjors commented at 2:21 pm on March 7, 2025:

In 801d2b1c3f25c6922df04cce4b2baa2047173eab “test: Remove legacy wallet unit tests”: we could include a bdb file, but there’s probably enough coverage of BerkeleyRO through the migration tests - which use previous releases to generate them on the fly.

laanwj commented at 10:02 am on March 14, 2025:

We could, but including binary files in the repo should be a last resort. The approach of creating them in the functional tests is preferable.

in src/wallet/test/wallet_tests.cpp:198 in 801d2b1c3f outdated

194@@ -195,141 +195,6 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
195     }
196 }
197 
198-BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup)

Sjors commented at 2:23 pm on March 7, 2025:

In 801d2b1c3f25c6922df04cce4b2baa2047173eab “test: Remove legacy wallet unit tests”: for descriptor wallets, do we now rely entirely on functional tests for import and rescan coverage?

in src/wallet/test/walletload_tests.cpp:114 in 801d2b1c3f outdated

109-    SerializeMany(s, args...);
110-    return {s.begin(), s.end()};
111-}
112-
113-
114-BOOST_FIXTURE_TEST_CASE(wallet_load_ckey, TestingSetup)

Sjors commented at 2:26 pm on March 7, 2025:

In 801d2b1c3f25c6922df04cce4b2baa2047173eab “test: Remove legacy wallet unit tests”: could this be converted to using a descriptor wallet? Or is there really no interesting coverage here?

Sjors approved

Sjors commented at 3:33 pm on March 7, 2025: member

ACK ba2042c31f1630678a1f08bb68e01c1d98cc9abc

Though there’s a few places where we could salvage test coverage (in a followup).

I ran the test suite on all commits.

wallet_upgradewallet.py is dropped. The test itself was only run for legacy wallets. However the upgradewallet RPC isn’t legacy-only, even though it doesn’t (currently) do anything useful for descriptor wallets. Maybe we should drop it here or in #28710? Or otherwise at least have some trivial coverage.

Should test/functional/wallet_pruning.py be retrofitted for descriptor wallets instead of deleting it?

First commit now matches ac6cde731314d981391bc313c98d671c68211d33 from #31961.

Note that 928760e6dd8383db2dbe87f99185b07cf13bcab3 drops backward compatibility tests for the legacy wallet, but ba2042c31f1630678a1f08bb68e01c1d98cc9abc brings new coverage (and we also have dedicated migration coverage).

DrahtBot requested review from maflcko on Mar 7, 2025

in test/functional/wallet_backwards_compatibility.py:331 in ba2042c31f outdated

345+
346+            hdkeypath = addr_info["hdkeypath"].replace("'", "h")
347+            pubkey = addr_info["pubkey"]
348+
349+            # Make a backup of the wallet file
350+            backup_path = os.path.join(self.options.tmpdir, f"{wallet_name}.dat")

brunoerg commented at 1:21 pm on March 11, 2025:

ba2042c31f1630678a1f08bb68e01c1d98cc9abc: both backup_path and pubkey are unused.

brunoerg approved

brunoerg commented at 1:40 pm on March 11, 2025: contributor

code review ACK ba2042c31f1630678a1f08bb68e01c1d98cc9abc no blockers!

8afd005cbf9209e90a5cc1bcdb2c0ff400c2de33: checked it removes disable RPCs related to legacy wallets (import/dump stuff) - I had same doubt of https://github.com/bitcoin/bitcoin/pull/31250/commits/8afd005cbf9209e90a5cc1bcdb2c0ff400c2de33#r1985126112
56e9bef655673becb3e265948143a9e860a5c68b: checked it removes legacy wallet functional tests and get rid off –descriptor –legacy-wallet. Left a nit about a comment that mentions –descriptor as example that could be removed.
1cafc80e87d98f43bf00f8663aa96d598085c1d8: checked that restorewo_wallet is a descriptor wallet (can be confirmed with getwalletinfo - {'walletname': 'wo', 'walletversion': 169900, 'format': 'sqlite', 'balance': Decimal('0E-8'), 'unconfirmed_balance': Decimal('0E-8'), 'immature_balance': Decimal('0E-8'), 'txcount': 0, 'keypoolsize': 0, 'keypoolsize_hd_internal': 0, 'paytxfee': Decimal('0E-8'), 'private_keys_enabled': False, 'avoid_reuse': False, 'scanning': False, 'descriptors': True, 'external_signer': False, 'blank': False, 'lastprocessedblock': {'hash': '77a656f562fcce93cb25747e1d3ef773b0803feca410587bd3e6cff9975dd5fc', 'height': 611}}), so there is no need to rescan since importaddress will call importdescriptors and therefore do it.
52c2774269d568c92d5c65b37c8ce32c745edfe7: since we cannot create or load legacy wallet, seems fine to remove salvage. Maybe we could also remove comments that mention it? (e.g. “Try using the wallet tool bitcoin-wallet to salvage or restoring a backup.” - src/wallet/bdb.cpp)?

Successfully ran bench, unit and functional tests on MacOS 14.3.

DrahtBot added the label Needs rebase on Mar 14, 2025

fanquake referenced this in commit 698f86964c 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

achow101 force-pushed on Mar 14, 2025

DrahtBot removed the label Needs rebase on Mar 14, 2025

Sjors commented at 9:12 am on March 14, 2025: member

Hoorway, #31961 landed.

re-ACK 32a522f2d825957f0c85d7b4ea9185a053b018e3

DrahtBot requested review from brunoerg on Mar 14, 2025

in src/bitcoin-wallet.cpp:49 in 32a522f2d8

44     argsman.AddArg("-printtoconsole", "Send trace/debug info to console (default: 1 when no -debug is true, 0 otherwise).", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
45     argsman.AddArg("-withinternalbdb", "Use the internal Berkeley DB parser when dumping a Berkeley DB wallet file (default: false)", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
46 
47     argsman.AddCommand("info", "Get wallet info");
48     argsman.AddCommand("create", "Create new wallet file");
49-    argsman.AddCommand("salvage", "Attempt to recover private keys from a corrupt wallet. Warning: 'salvage' is experimental.");

laanwj commented at 9:39 am on March 14, 2025:

At some point we might want to have something like -salvage for corrupted sqlite wallets, but makes sense to remove it for now.

brunoerg approved

brunoerg commented at 9:49 am on March 14, 2025: contributor

reACK 32a522f2d825957f0c85d7b4ea9185a053b018e3

in src/wallet/dump.cpp:183 in 32a522f2d8

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 = strprintf(_("Error: Dumpfile specifies an unsupported database format (%s). Only sqlite database dumps are supported"), format_value);

laanwj commented at 9:51 am on March 14, 2025:

missing return false;?

in src/wallet/wallet.cpp:3058 in 32a522f2d8

3051@@ -3052,7 +3052,11 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
3052             error = strprintf(_("Unexpected legacy entry in descriptor wallet found. Loading wallet %s\n\n"
3053                                 "The wallet might have been tampered with or created with malicious intent.\n"), walletFile);
3054             return nullptr;
3055-        } else {
3056+        } else if (nLoadWalletRet == DBErrors::LEGACY_WALLET) {
3057+            error = strprintf(_("Error loading %s: Wallet is a legacy wallet. Please migrate to a descriptor wallet using the migration tool (migratewallet RPC)."), walletFile);
3058+            return nullptr;
3059+        }

laanwj commented at 10:25 am on March 14, 2025:

nit: don’t we normally use same-line } else {?

in src/wallet/walletdb.cpp:490 in 32a522f2d8

484@@ -485,6 +485,11 @@ static DBErrors LoadWalletFlags(CWallet* pwallet, DatabaseBatch& batch) EXCLUSIV
485             pwallet->WalletLogPrintf("Error reading wallet database: Unknown non-tolerable wallet flags found\n");
486             return DBErrors::TOO_NEW;
487         }
488+        // All wallets must be descriptor wallets unless opened with a bdb_ro db
489+        // bdb_ro is only used for legacy to descriptor migration.
490+        if (pwallet->GetDatabase().Format() != "bdb_ro" && !pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {

laanwj commented at 10:28 am on March 14, 2025:

Why do we compare strings here instead of use the enum DatabaseFormat?

Edit: Okay, looks like WalletDatabase doesn’t have such a field, there’s only the string, no need to change that here.

DrahtBot added the label Needs rebase on Mar 16, 2025

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

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

pablomartin4btc commented at 11:42 am on March 21, 2025: member

Concept ACK

Light cr (by commit - will do another round next week) & tACK 32a522f2d825957f0c85d7b4ea9185a053b018e3

Verified when I try to load a legacy wallet (via RPC or from QT menu) I get: “... The wallet appears to be a Legacy wallet, please use the wallet migration tool (migratewallet RPC).”

Also, running QT with legacy wallets in the settings.json will produce same output above closing the app with that error description ("Error: Failed to open database path...").

I can see we are still able to create legacy via RPC (n longer the case in final #28710 - with -deprecatedrpc=create_bdb), and if I run te command in the QT console I get the wallet loaded:

Is this correct or needs to be fixed in this PR?

pablomartin4btc commented at 3:56 pm on March 24, 2025: member

Adding more details from my previous comment:

The legacy wallet is being created as a sqlite DB (the unsupported legacy-sqlite mentioned above): https://github.com/bitcoin/bitcoin/blob/32a522f2d825957f0c85d7b4ea9185a053b018e3/src/wallet/walletdb.cpp#L1471-L1477
In getwalletinfo we could see "format": "sqlite",;

wallet: Disable creating and loading legacy wallets #31250

Code Coverage & Benchmarks

Reviews

Conflicts