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.

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, pablomartin4btc, laanwj
Concept ACK	rkrux
Stale ACK	maflcko, brunoerg

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:

#32290 (test: allow all functional tests to be run or skipped with –usecli by mzumsande)
#32273 (wallet: Fix relative path backup during migration. by davidgumberg)
#32174 (test: Verify that a message is not in rpc errors raised (follow-up 31451) by pablomartin4btc)
#32123 (wallet: make coinbase that will mature on the next block available for selection by luisschwab)
#31936 (rpc: Support v3 raw transactions creation by Bue-von-hon)
#31723 (qa debug: Add –debug_runs/-waitfordebugger [DRAFT] by hodlinator)
#31668 (Added rescan option for import descriptors by saikiran57)
#31423 (wallet: migration, don’t create spendable wallet from a watch-only legacy wallet by furszy)
#29770 (index: Check all necessary block data is available before starting to sync by fjahr)
#29124 (test: Test that migration automatically repairs 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)
#28710 (Remove the legacy wallet and BDB dependency 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)

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”

achow101 commented at 4:17 am on April 12, 2025:

Added a commit to make the test work for descriptor wallets.

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?

davidgumberg commented at 6:03 pm on April 23, 2025:

AFAICT the workaround is correct, and this test is already using importdescriptors on master.

This test succeeds as-is on this branch with this commit (https://github.com/bitcoin/bitcoin/pull/31250/commits/e6c900f9c0e1b143098663e1c5c80e1811946b2d) dropped.

achow101 commented at 7:10 pm on April 23, 2025:

I certain that this commit was required when writing this PR originally, however, I no longer recall the error that it fixed. Dropping this commit.

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

achow101 commented at 4:19 am on April 12, 2025:

Done

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

pablomartin4btc commented at 8:00 pm on April 11, 2025:

I second this, e.g. importprivkey and other rpc are not removed until #28710, just curious what’s the rationale to remove the calls from here and not from other places?

achow101 commented at 4:16 am on April 12, 2025:

Moved this commit to #28710

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?

achow101 commented at 4:10 am on April 12, 2025:

Yes. I don’t think having this unit test is all that useful.

in src/wallet/test/walletload_tests.cpp:115 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?

achow101 commented at 4:09 am on April 12, 2025:

This test case is no longer interesting because it is related to auto upgrade behavior for legacy wallets that will not occur for descriptor wallets.

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

achow101 commented at 4:23 am on April 12, 2025:

backup_path is used, but pubkey and hdkeypath are not. Removed those.

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

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 outdated

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

achow101 commented at 0:51 am on April 10, 2025:

Oops yes, done.

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

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 {?

achow101 commented at 0:52 am on April 10, 2025:

Done

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

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

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",;

achow101 force-pushed on Apr 10, 2025

achow101 commented at 0:52 am on April 10, 2025: member

Rebased and addressed comments.

Will look into the qt issue tomorrow.

DrahtBot removed the label Needs rebase on Apr 10, 2025

DrahtBot added the label CI failed on Apr 10, 2025

DrahtBot commented at 2:34 am on April 10, 2025: contributor

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

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.

achow101 force-pushed on Apr 10, 2025

DrahtBot removed the label CI failed on Apr 10, 2025

pablomartin4btc commented at 12:46 pm on April 10, 2025: member

Will look into the qt issue tomorrow.

I was using QT for testing it but I think the issue is happening at the node level.

achow101 force-pushed on Apr 10, 2025

achow101 commented at 8:47 pm on April 10, 2025: member

The issue with creating a legacy-sqlite wallet should be fixed now.

achow101 force-pushed on Apr 10, 2025

DrahtBot commented at 8:51 pm on April 10, 2025: contributor

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

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 Apr 10, 2025

achow101 force-pushed on Apr 10, 2025

DrahtBot removed the label CI failed on Apr 11, 2025

laanwj approved

laanwj commented at 8:56 am on April 11, 2025: member

Code review ACK 545a36ae0aa5687366fc4fdd54058072f333e73f Assuming this is still planned for 30.0, it’s probably good to merge the BDB removal early in the release window, so that new PRs no longer need to take both cases into account, and possible problems can come to light before the release.

DrahtBot requested review from Sjors on Apr 11, 2025

DrahtBot requested review from brunoerg on Apr 11, 2025

DrahtBot requested review from pablomartin4btc on Apr 11, 2025

in test/functional/wallet_backwards_compatibility.py:225 in 545a36ae0a outdated

238             wallet_name = f"up_{node.version}"
239-            if self.major_version_at_least(node, 21):
240-                node.rpc.createwallet(wallet_name=wallet_name, descriptors=self.options.descriptors)
241-            else:
242-                node.rpc.createwallet(wallet_name=wallet_name)
243+            node.rpc.createwallet(wallet_name=wallet_name, descriptors=True)

pablomartin4btc commented at 7:47 pm on April 11, 2025:

nit: not a blocker… descriptors=True is the default… there are several places where can be removed

0            node.rpc.createwallet(wallet_name=wallet_name)

achow101 commented at 4:25 am on April 12, 2025:

Done

achow101 commented at 4:29 am on April 12, 2025:

Actually, no. This parameter needs to be here because node includes older versions where descriptors is not the default.

pablomartin4btc commented at 3:21 pm on April 14, 2025:

But those are descriptors nodes already (for node in descriptors_nodes:).

achow101 commented at 6:04 pm on April 14, 2025:

Those are nodes where descriptor wallets are available, not when they were the default. We did not default to descriptor wallets until a couple of releases after introduction.

pablomartin4btc commented at 7:57 pm on April 14, 2025:

Ok I see, checked the comment in the code, thanks!

pablomartin4btc commented at 8:00 pm on April 11, 2025: member

tACK 545a36ae0aa5687366fc4fdd54058072f333e73f

0./build_3125/bin/bitcoind -regtest -deprecatedrpc="create_bdb"
1...

(on another terminal)

0./build_31250/bin/bitcoin-cli -regtest createwallet "legacy_test" true true "" false false
1error code: -4
2error message:
3descriptors argument must be set to "true"; It is no longer possible to create a legacy wallet.

Left a couple of comments.

achow101 force-pushed on Apr 12, 2025

DrahtBot added the label CI failed on Apr 12, 2025

DrahtBot commented at 4:18 am on April 12, 2025: contributor

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

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.

achow101 force-pushed on Apr 12, 2025

DrahtBot removed the label CI failed on Apr 12, 2025

in src/wallet/rpc/wallet.cpp:358 in 150a83ca4f outdated

354@@ -355,9 +355,7 @@ static RPCHelpMan createwallet()
355             {"blank", RPCArg::Type::BOOL, RPCArg::Default{false}, "Create a blank wallet. A blank wallet has no keys or HD seed. One can be set using sethdseed."},
356             {"passphrase", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "Encrypt the wallet with this passphrase."},
357             {"avoid_reuse", RPCArg::Type::BOOL, RPCArg::Default{false}, "Keep track of coin reuse, and treat dirty and clean coins differently with privacy considerations in mind."},
358-            {"descriptors", RPCArg::Type::BOOL, RPCArg::Default{true}, "Create a native descriptor wallet. The wallet will use descriptors internally to handle address creation."
359-                                                                       " Setting to \"false\" will create a legacy wallet; This is only possible with the -deprecatedrpc=create_bdb setting because, the legacy wallet type is being deprecated and"
360-                                                                       " support for creating and opening legacy wallets will be removed in the future."},
361+            {"descriptors", RPCArg::Type::BOOL, RPCArg::Default{true}, "If set, must be \"true\""},

rkrux commented at 11:23 am on April 14, 2025:

“If set, must be "true"”

What’s the main reason to keep the descriptors argument around now?

Couple possible reasons I can think of are:

to test not being able to create legacy wallets
also not having to update the call sites in the functional tests in this PR but I don’t see this argument removed in #28710 later.

Is there any reason from the user’s POV as well to keep this argument?

rkrux commented at 12:16 pm on April 14, 2025:

The RPC examples below contain {"descriptors", true} argument. IMO we can remove it as I feel we don’t want to encourage the usage of this argument anymore as there is just one mode left now - descriptors.

achow101 commented at 7:20 pm on April 14, 2025:

The main reason is for backwards compatibility with older scripts, bitcoin-cli, etc. Typically, we don’t ever remove positional arguments, and instead turn them into dummy arguments that must be set to a particular value. This PR doesn’t rename it to dummy yet since I had some issues with doing that because of the backwards compatibility test.

in test/functional/wallet_backwards_compatibility.py:297 in 380f5e2359 outdated

292+            if self.major_version_less_than(node, 17):
293+                # createwallet is only available in 0.17+
294+                self.restart_node(node.index, extra_args=[f"-wallet={wallet_name}"])
295+                wallet_prev = node.get_wallet_rpc(wallet_name)
296+                address = wallet_prev.getnewaddress('', "bech32")
297+                addr_info = wallet_prev.validateaddress(address)

rkrux commented at 11:36 am on April 14, 2025:

Is this block still required after the v17 node being removed from the list in this test as done in this #32214 PR? The earliest version being tested in this test is v18 now.

Don’t think this was required before #32214 as well as the check is of strictly lesser than.

achow101 commented at 7:26 pm on April 14, 2025:

Removed

in src/wallet/walletdb.cpp:1485 in 380f5e2359 outdated

1484@@ -1478,9 +1485,6 @@ std::unique_ptr<WalletDatabase> MakeDatabase(const fs::path& path, const Databas
1485     // If the format is not specified or detected, choose the default format based on what is available. We prefer BDB over SQLite for now.

rkrux commented at 12:05 pm on April 14, 2025:

choose the default format based on what is available. We prefer BDB over SQLite for now.

This is not required anymore imo.

achow101 commented at 7:26 pm on April 14, 2025:

Removed

in src/wallet/rpc/wallet.cpp:405 in 150a83ca4f outdated

407-            throw JSONRPCError(RPC_WALLET_ERROR, "BDB wallet creation is deprecated and will be removed in a future release."
408-                                                 " In this release it can be re-enabled temporarily with the -deprecatedrpc=create_bdb setting.");
409-        }
410+    flags |= WALLET_FLAG_DESCRIPTORS;
411+    if (!self.Arg<bool>("descriptors")) {
412+        throw JSONRPCError(RPC_WALLET_ERROR, "descriptors argument must be set to \"true\"; It is no longer possible to create a legacy wallet.");

rkrux commented at 12:14 pm on April 14, 2025:

Nit: s/It/it

0- throw JSONRPCError(RPC_WALLET_ERROR, "descriptors argument must be set to \"true\"; It is no longer possible to create a legacy wallet.");
1+ throw JSONRPCError(RPC_WALLET_ERROR, "descriptors argument must be set to \"true\"; it is no longer possible to create a legacy wallet.");

achow101 commented at 7:26 pm on April 14, 2025:

Done

in src/wallet/rpc/wallet.cpp:408 in 150a83ca4f outdated

410+    flags |= WALLET_FLAG_DESCRIPTORS;
411+    if (!self.Arg<bool>("descriptors")) {
412+        throw JSONRPCError(RPC_WALLET_ERROR, "descriptors argument must be set to \"true\"; It is no longer possible to create a legacy wallet.");
413     }
414     if (!request.params[7].isNull() && request.params[7].get_bool()) {
415 #ifdef ENABLE_EXTERNAL_SIGNER

rkrux commented at 12:31 pm on April 14, 2025:

Few lines below.

https://github.com/bitcoin/bitcoin/pull/31250/commits/150a83ca4fcad4e158cef3e6752b3a84ffb7d0b6#diff-7b13e03c457b62251e26d2c366180c262a3fe9da45995277a9a9c1c5abbaae6cR415-R419

0#ifndef USE_BDB
1    if (!(flags & WALLET_FLAG_DESCRIPTORS)) {
2        throw JSONRPCError(RPC_WALLET_ERROR, "Compiled without bdb support (required for legacy wallets)");
3    }
4#endif

I don’t believe this is required anymore, the inner condition can never be true now as WALLET_FLAG_DESCRIPTORS are set few lines up unconditionally.

rkrux commented at 5:26 pm on April 14, 2025:

I see it’s removed later in #28710 but can be removed here as well as I feel it has become redundant in this PR itself.

achow101 commented at 7:26 pm on April 14, 2025:

Removed

rkrux commented at 2:07 pm on April 14, 2025: contributor

Concept ACK 380f5e2359447ed2e9a1b54cffbefef1eb4f9b6a

I’ve looked at the last two commits only wherein creation of legacy wallets is disallowed and loading of legacy wallets is disallowed respectively.

For the latter, the BERKELEY database format is removed & MakeDatabase instead defaults to using the BERKELEY_RO database format, before loading the wallet, while ensuring that BERKELEY_RO can only be used during the wallet migration flow wherein the require_format is explicitly set to BERKELEY_RO already in the migration flow - this ensures that legacy-bdb wallets are not loaded.

However, a theoretical legacy-sqlite wallet can still bypass this^, that is later prohibited to be loaded while checking for legacy non bdb_ro wallets during loading the wallet flags.

Thus, I believe both legacy bdb and sqlite wallets are disallowed to be loaded with the exclusion of loading/using legacy wallets only during the migration flow.

achow101 force-pushed on Apr 14, 2025

DrahtBot added the label CI failed on Apr 14, 2025

DrahtBot removed the label CI failed on Apr 14, 2025

Sjors commented at 8:34 am on April 18, 2025: member

re-ACK 1ffea3fba3b22c40bbcc44cd7c3a637e0450315a

Main changes since my last review:

commit 3841da0f62fa5e26f679a12c6efbafe1cb17c25f to preserve PSBT GUI coverage
#32214 landed which made 3841da0f62fa5e26f679a12c6efbafe1cb17c25f and 1ffea3fba3b22c40bbcc44cd7c3a637e0450315a smaller
#31866 landed which required some some changes in 7dbf06c33508820486eed59aba78d852c6212864

DrahtBot requested review from laanwj on Apr 18, 2025

DrahtBot requested review from pablomartin4btc on Apr 18, 2025

DrahtBot requested review from rkrux on Apr 18, 2025

pablomartin4btc commented at 2:16 pm on April 18, 2025: member

re-ACK 1ffea3fba3b22c40bbcc44cd7c3a637e0450315a

Since my last review:

commit “test: rpcs disabled for descriptor wallets were removed” was moved to #28710
addressed feedback from @rkrux

pablomartin4btc commented at 6:35 pm on April 18, 2025: member

-legacy argument shouldn’t be removed from bitcoin-wallet/ wallettool.cpp? (leaving -descriptors true by default):

 0/build/bin/bitcoin-wallet -legacy -wallet="pepe" create
 1Topping up keypool...
 2Wallet info
 3===========
 4Name: pepe
 5Format: sqlite
 6Descriptors: no
 7Encrypted: no
 8HD (hd seed available): yes
 9Keypool Size: 2000
10Transactions: 0
11Address Book: 0

achow101 commented at 9:48 pm on April 18, 2025: member

-legacy argument shouldn’t be removed from bitcoin-wallet/ wallettool.cpp? (leaving -descriptors true by default):

Good catch! Added a commit to disable legacy wallet creation from the wallet tool.

achow101 force-pushed on Apr 18, 2025

laanwj approved

laanwj commented at 11:29 am on April 22, 2025: member

Re-ACK e39118ab0bf08fdaf68b16c86ad304b9133b43c7 git range-diff b8cefeb221490868d62b7a0695f8b5ea392d3654..545a36ae0aa5687366fc4fdd54058072f333e73f b2bb27f40c7ab7b51720e41d136dcc9077a525b3..e39118ab0bf08fdaf68b16c86ad304b9133b43c7 commit

test: rpcs disabled for descriptor wallets were removed was dropped and moved to another PR
tests, gui: Use descriptors watchonly wallet for watchonly test was added
test: Remove legacy wallet unit tests: qt related changes were removed, replaced by the above commit
test: remove legacy wallet functional tests: comment improvement
wallet: Disallow legacy wallet creation from the wallet tool was added
wallet: Disallow loading legacy wallets: createwallet RPC error message improvement, remove redundant USE_BDB gated code
wallet: Disallow loading legacy wallets: set format for new databases, remove redundant comment

DrahtBot requested review from pablomartin4btc on Apr 22, 2025

in src/qt/test/wallettests.cpp:201 in 3841da0f62 outdated

216     wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
217-    wallet->SetupDescriptorScriptPubKeyMans();
218+    if (watch_only) {
219+        wallet->SetWalletFlag(WALLET_FLAG_DISABLE_PRIVATE_KEYS);
220+    } else {
221+        wallet->SetupDescriptorScriptPubKeyMans();

davidgumberg commented at 5:40 pm on April 22, 2025:

For other reviewers: Unlike for legacy wallets, watch only¹ descriptor wallets do not need/have their ScriptPubKeyMans set up during wallet creation.

https://github.com/bitcoin/bitcoin/blob/e5a00b24972461f7a181bc184dd461cedcce6161/src/wallet/wallet.cpp#L3073-L3080

Tangentially, it might be nice to refactor SetupDescriptorsWallet() to reuse CWallet::Create() rather than imitate it.

WALLET_FLAG_DISABLE_PRIVATE_KEYS ↩︎

in src/wallet/wallettool.cpp:134 in ec5d2c7fca outdated

135+        if (command != "create") {
136+            tfm::format(std::cerr, "The -legacy option can only be used with the 'create' command.\n");
137+            return false;
138+        }
139+        if (args.GetBoolArg("-legacy", true)) {
140+            tfm::format(std::cerr, "The -legacy option must be set to \"false\"");

Sjors commented at 8:38 am on April 23, 2025:

In ec5d2c7fca5e6c36aa59aeb7ce64b98739b6d094 “wallet: Disallow legacy wallet creation from the wallet tool”: I’m getting a trailing % on macOS

achow101 commented at 7:11 pm on April 23, 2025:

I think the % is your command line prompt?

This was missing a newline, fixed.

DrahtBot requested review from Sjors on Apr 23, 2025

in test/functional/wallet_backwards_compatibility.py:276 in ec6c95b1e0 outdated

315@@ -325,7 +316,7 @@ def run_test(self):
316 
317             wallet.unloadwallet()
318 
319-            # Check that no automatic upgrade broke downgrading the wallet
320+            # Check that no automatic upgrade broke the downgrading the wallet

davidgumberg commented at 6:06 pm on April 23, 2025:

Nit: Extra “the”.

achow101 commented at 7:12 pm on April 23, 2025:

Fixed

tests, gui: Use descriptors watchonly wallet for watchonly test d9ac9dbd8e

test: Remove legacy wallet unit tests f94f9399ac

test: wallet_signer.py bdb will be removed aff80298d0

achow101 force-pushed on Apr 23, 2025

test: Remove legacy wallet tests from wallet_backwards_compatibility.py 446d480cb2

test: Remove legacy wallet tests from wallet_reindex.py 20a9173717

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.

c847dee148

wallet: Remove -format and bdb from wallet tool's createfromdump 7a41c939f0

wallet: Remove wallettool salvage

Salvage is bdb only which is about to be removed.

56f959d829

bench: Remove WalletLoadingLegacy benchmark 5e93b1fd6c

wallet: Disallow legacy wallet creation from the wallet tool 6b247279b7

achow101 force-pushed on Apr 23, 2025

wallet: Disallow creating legacy wallets

Remove the option to set descriptors=False when creating a wallet, and
enforce this in RPC and in CreateWallet

9f04e02ffa

wallet: Disallow loading 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.

17bb63f9f9

achow101 force-pushed on Apr 23, 2025

Sjors commented at 9:44 am on April 24, 2025: member

re-ACK 17bb63f9f9b08e6af60c089234fe878657dbc88e

DrahtBot requested review from laanwj on Apr 24, 2025

pablomartin4btc commented at 9:53 am on April 24, 2025: member

re-ACK 17bb63f9f9b08e6af60c089234fe878657dbc88e

Since my last review:

Added a commit to disable legacy wallet creation from the wallet tool.
Addresed @davidgumberg’s feedback.

laanwj approved

laanwj commented at 10:53 am on April 24, 2025: member

re-ACK 17bb63f9f9b08e6af60c089234fe878657dbc88e git range-diff b2bb27f40c7ab7b51720e41d136dcc9077a525b3..e39118ab0bf08fdaf68b16c86ad304b9133b43c7 9a4c92eb9ac29204df3d826f5ae526ab16b8ad65..17bb63f9f9b08e6af60c089234fe878657dbc88e

test: wallet_transactiontime_rescan importdescriptors always rescans: commit removed (test change not necessary here)
test: Remove legacy wallet tests from wallet_backwards_compatibility.py: comment change typo reverted
wallet: Disallow legacy wallet creation from the wallet tool, wallet: Disallow creating legacy wallets: add newlines to tfm::formats
- (ideally these would both be in the first commit as that’s where they’re introduced, maybe if you need to re-touch anyway)

pablomartin4btc commented at 4:23 pm on April 24, 2025: member

If a user tries to restore a legacy wallet (using RPC or QT) setting “load_on_startup” (can’t be done on QT but it’s being set in the wallet interface code), next time the node or QT starts it will be closed with the error "… Failed to open database path … The wallet appears to be a Legacy wallet, please use the wallet migration tool… “. That case shouldn’t be handled here? We shouldn’t allow load_on_startup on legacy…

(Currently this situation is not happening as a side effect of #31451).

achow101 commented at 7:12 pm on April 24, 2025: member

If a user tries to restore a legacy wallet (using RPC or QT) setting “load_on_startup” (can’t be done on QT but it’s being set in the wallet interface code), next time the node or QT starts it will be closed with the error "… Failed to open database path … The wallet appears to be a Legacy wallet, please use the wallet migration tool… “. That case shouldn’t be handled here? We shouldn’t allow load_on_startup on legacy…

(Currently this situation is not happening as a side effect of #31451).

I don’t quite follow what you’re saying. I tried to restore a legacy wallet with this PR and it doesn’t do it, nor are there any issues with startup the next time.

Do you mean there is a possibility here if the interface is used incorrectly internally? In that case, I think it can be handled in a followup/if it becomes a reachable problem.

pablomartin4btc commented at 10:15 pm on April 24, 2025: member

I tried to restore a legacy wallet with this PR and it doesn’t do it, nor are there any issues…

Ok, I thought the restore would allow to do it and then the load_on_startup would fail but the restore fail on the wallet file verification, so all good.

Tested restorewallet creating a legacy on master, backed it up there and trying to restore it on this PR’s branch.

0./build_31250/bin/bitcoin-cli -regtest -datadir=/tmp/btc-wallet restorewallet "restored_legacy_master" /tmp/btc-wallet/regtest/wallets/backup_legacy_master.dat true
1error code: -18
2error message:
3Wallet file verification failed. Failed to open database path '/tmp/btc-wallet/regtest/wallets/restored_legacy_master'. The wallet appears to be a Legacy wallet, please use the wallet migration tool (migratewallet RPC).

pablomartin4btc commented at 10:20 pm on April 24, 2025: member

Do you mean there is a possibility here if the interface is used incorrectly internally?

I’ve tested it with this branch’s QT and same failure on wallet file verifitcation while trying to restore a legacy backup using the GUI. All good.

maflcko commented at 8:02 am on April 25, 2025: member

(fresh CI)

maflcko closed this on Apr 25, 2025

maflcko reopened this on Apr 25, 2025

in test/functional/test_framework/test_framework.py:164 in c847dee148 outdated

159@@ -160,8 +160,9 @@ def __init__(self, test_file) -> None:
160         # are not imported.
161         self.wallet_names = None
162         # By default the wallet is not required. Set to true by skip_if_no_wallet().
163-        # When False, we ignore wallet_names regardless of what it is.
164-        self._requires_wallet = False
165+        # Can also be set to None to indicate that the wallet will be used if available.
166+        # When False or None, we ignore wallet_names regardless of what it is.

maflcko commented at 9:28 am on April 25, 2025:

in c847dee1488a294c9a9632a00ba1134b21e41947: I don’t think this comment is correct. It is true that wallet_names are ignored in setup_nodes, but it is still possible to manually call import_deterministic_coinbase_privkeys or init_wallet.

My recommendation would be to remove the comment, or replace we with setup_nodes.

maflcko commented at 12:18 pm on April 25, 2025:

You can also test this claim yourself with the diff on top of that commit:

 0diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py
 1index ee79a27392..12bcf1c722 100755
 2--- a/test/functional/feature_rbf.py
 3+++ b/test/functional/feature_rbf.py
 4@@ -35,6 +35,7 @@ class ReplaceByFeeTest(BitcoinTestFramework):
 5         ]
 6         self.supports_cli = False
 7         self.uses_wallet = None
 8+        self.wallet_names = []
 9 
10     def run_test(self):
11         self.wallet = MiniWallet(self.nodes[0])

The test will fail

maflcko commented at 12:19 pm on April 25, 2025:

Also, you forgot to remove REQUIRE_WALLET_TYPE_SET in this commit.

achow101 commented at 4:08 pm on April 25, 2025:

Added a commit to #28710 changing the comment, and removing REQUIRE_WALLET_TYPE_SET.

in test/functional/rpc_createmultisig.py:35 in c847dee148 outdated

30-
31     def set_test_params(self):
32         self.setup_clean_chain = True
33         self.num_nodes = 3
34         self.supports_cli = False
35-        self.enable_wallet_if_possible()

maflcko commented at 10:10 am on April 25, 2025:

forgot to remove enable_wallet_if_possible?

achow101 commented at 4:09 pm on April 25, 2025:

Added a commit to #28710 to remove

in test/functional/rpc_createmultisig.py:66 in c847dee148 outdated

58         for (sigs, keys) in m_of_n:
59             for output_type in ["bech32", "p2sh-segwit", "legacy"]:
60-                self.do_multisig(keys, sigs, output_type, wallet_multi)
61+                self.do_multisig(keys, sigs, output_type)
62 
63-        self.test_multisig_script_limit(wallet_multi)

maflcko commented at 10:13 am on April 25, 2025:

why is the call removed? Either the function should be removed along with the call, or the call should be kept

achow101 commented at 4:09 pm on April 25, 2025:

Added a commit to #28710 to restore

in test/functional/rpc_rawtransaction.py:77 in c847dee148 outdated

73@@ -77,6 +74,7 @@ def set_test_params(self):
74         # whitelist peers to speed up tx relay / mempool sync
75         self.noban_tx_relay = True
76         self.supports_cli = False
77+        self.uses_wallet = None

maflcko commented at 10:19 am on April 25, 2025:

This test doesn’t seem wallet related? Why is this set?

achow101 commented at 4:09 pm on April 25, 2025:

Added a commit to #28710 to remove

in test/functional/test_framework/test_node.py:928 in c847dee148 outdated

924@@ -926,22 +925,16 @@ def send_cli(self, clicommand=None, *args, **kwargs):
925             return cli_stdout.rstrip("\n")
926 
927 class RPCOverloadWrapper():
928-    def __init__(self, rpc, cli=False, descriptors=False):
929+    def __init__(self, rpc, cli=False):

maflcko commented at 10:22 am on April 25, 2025:

unrelated in c847dee1488a294c9a9632a00ba1134b21e41947: However, while touching, it could make sense to force named args for literal args, especially integral (boolean) ones:

0    def __init__(self, rpc, *, cli=False):

achow101 commented at 4:09 pm on April 25, 2025:

I’ll let the next person to touch it to change this.

maflcko commented at 8:42 am on April 28, 2025:

Actually, the cli field can be removed. Done in https://github.com/bitcoin/bitcoin/pull/32360

in test/functional/test_runner.py:199 in c847dee148 outdated

194     'mempool_resurrect.py',
195     'wallet_txn_doublespend.py --mineblock',
196     'tool_bitcoin_chainstate.py',
197-    'tool_wallet.py --legacy-wallet',
198-    'tool_wallet.py --legacy-wallet --bdbro',
199-    'tool_wallet.py --legacy-wallet --bdbro --swap-bdb-endian',

maflcko commented at 10:49 am on April 25, 2025:

why remove bdbro and swap-bdb-endian? The options remain, so either they should be tested, or fully removed.

maflcko commented at 10:59 am on April 25, 2025:

Same for dump_bdb_kv: Either keep it and test it, or remove it fully.

achow101 commented at 4:10 pm on April 25, 2025:

They are removed in #28710

in test/functional/test_framework/test_node.py:940 in c847dee148 outdated

942-            descriptors = self.descriptors
943-        return self.__getattr__('createwallet')(wallet_name, disable_private_keys, blank, passphrase, avoid_reuse, descriptors, load_on_startup, external_signer)
944-
945     def importprivkey(self, privkey, label=None, rescan=None):
946         wallet_info = self.getwalletinfo()
947         if 'descriptors' not in wallet_info or ('descriptors' in wallet_info and not wallet_info['descriptors']):

maflcko commented at 11:04 am on April 25, 2025:

forgot to remove this?

achow101 commented at 3:55 pm on April 25, 2025:

The importprivkey overload was kept because several tests still use it.

maflcko commented at 8:41 am on April 28, 2025:

I was referring to if 'descriptors' not in wallet_info or ('descriptors' in wallet_info and not wallet_info['descriptors']):. However, I see that it may be used by tests using previous releases.

in test/functional/wallet_createwallet.py:152 in c847dee148 outdated

149             assert_equal(walletinfo['keypoolsize_hd_internal'], keys)
150         # Allow empty passphrase, but there should be a warning
151         resp = self.nodes[0].createwallet(wallet_name='w7', disable_private_keys=False, blank=False, passphrase='')
152-        assert_equal(resp["warnings"], [EMPTY_PASSPHRASE_MSG] if self.options.descriptors else [EMPTY_PASSPHRASE_MSG, LEGACY_WALLET_MSG])
153+        assert_equal(resp["warnings"], [EMPTY_PASSPHRASE_MSG])
154

maflcko commented at 11:46 am on April 25, 2025:

forgot to remove LEGACY_WALLET_MSG?

achow101 commented at 4:10 pm on April 25, 2025:

Added a commit to #28710 to remove

fanquake merged this on Apr 25, 2025

fanquake closed this on Apr 25, 2025

fanquake commented at 12:47 pm on April 25, 2025: member

Looks like is_bdb_compiled & skip_if_no_bdb should have been dropped.

in test/functional/feature_bip68_sequence.py:58 in 17bb63f9f9

54@@ -58,6 +55,7 @@ def set_test_params(self):
55                 '-testactivationheight=csv@432',
56             ],
57         ]
58+        self.uses_wallet = None

maflcko commented at 12:59 pm on April 25, 2025:

the test doesn’t look wallet related, so can probably remove this (albeit harmless)

achow101 commented at 4:10 pm on April 25, 2025:

added a commit to #28710 to remove

pablomartin4btc commented at 1:28 pm on April 25, 2025: member

Looks like is_bdb_compiled & skip_if_no_bdb should have been dropped.

That’s done in #28710 (commit: “build, wallet, doc: Remove BDB”).

amtriorix commented at 7:43 pm on April 25, 2025: none

I do disagree with this merge Are you crazy or what. Backwards compatibility is needed for wallet.dat

davidgumberg commented at 8:05 pm on April 28, 2025: contributor

Backwards compatibility is needed for wallet.dat

Bitcoin Core will still be able to migrate wallets that are in the legacy format to the new format. See the migratewallet rpc command: https://bitcoincore.org/en/doc/29.0.0/rpc/wallet/migratewallet/ and the legacy wallet deprecation tracking issue (https://github.com/bitcoin/bitcoin/issues/20160) for more context.

wallet: Disable creating and loading legacy wallets #31250

Code Coverage & Benchmarks

Reviews

Conflicts