wallet: Disable creating and loading legacy wallets by achow101 · Pull Request #31250 · bitcoin/bitcoin

achow101 · 2024-11-07T18:42:43Z

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 #28710

DrahtBot · 2024-11-07T18:42:47Z

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.

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

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.

Sjors · 2025-04-24T09:44:52Z

re-ACK 17bb63f

pablomartin4btc

re-ACK 17bb63f

Since my last review:

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

laanwj

re-ACK 17bb63f
git range-diff b2bb27f..e39118ab0bf08fdaf68b16c86ad304b9133b43c7 9a4c92e..17bb63f

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 · 2025-04-24T16:23:13Z

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 · 2025-04-24T19:12:14Z

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 · 2025-04-24T22:15:43Z

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.

./build_31250/bin/bitcoin-cli -regtest -datadir=/tmp/btc-wallet restorewallet "restored_legacy_master" /tmp/btc-wallet/regtest/wallets/backup_legacy_master.dat true
error code: -18
error message:
Wallet 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 · 2025-04-24T22:20:26Z

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 · 2025-04-25T08:02:30Z

(fresh CI)

maflcko · 2025-04-25T10:22:54Z


 class RPCOverloadWrapper():
-    def __init__(self, rpc, cli=False, descriptors=False):
+    def __init__(self, rpc, cli=False):


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

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

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

Actually, the cli field can be removed. Done in #32360

maflcko · 2025-04-25T11:04:40Z

-
    def importprivkey(self, privkey, label=None, rescan=None):
        wallet_info = self.getwalletinfo()
        if 'descriptors' not in wallet_info or ('descriptors' in wallet_info and not wallet_info['descriptors']):


forgot to remove this?

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

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.

fanquake · 2025-04-25T12:47:44Z

Looks like is_bdb_compiled & skip_if_no_bdb should have been dropped.

pablomartin4btc · 2025-04-25T13:28:49Z

Looks like is_bdb_compiled & skip_if_no_bdb should have been dropped.

That's done in #28710 (commit: "build, wallet, doc: Remove BDB").

davidgumberg · 2025-04-28T20:05:47Z

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 (#20160) for more context.

DrahtBot added the Wallet label Nov 7, 2024

achow101 mentioned this pull request Nov 7, 2024

Proposed Timeline for Legacy Wallet and BDB removal #20160

Closed

48 tasks

achow101 force-pushed the disable-legacy-wallets branch from 4fcb431 to 47ac41b Compare November 7, 2024 18:54

achow101 mentioned this pull request Nov 7, 2024

Remove the legacy wallet and BDB dependency #28710

Merged

DrahtBot added the CI failed label Nov 7, 2024

DrahtBot added the Needs rebase label Nov 12, 2024

achow101 force-pushed the disable-legacy-wallets branch from 47ac41b to 4f0e11b Compare November 12, 2024 15:49

achow101 added 2 commits April 23, 2025 12:11

wallet: Disallow creating legacy wallets

9f04e02

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

wallet: Disallow loading legacy wallets

17bb63f

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.

pablomartin4btc reviewed Apr 24, 2025

View reviewed changes

laanwj approved these changes Apr 24, 2025

View reviewed changes

maflcko reviewed Apr 25, 2025

View reviewed changes

Comment thread test/functional/feature_bip68_sequence.py

This comment was marked as abuse.

Sign in to view

fanquake mentioned this pull request May 1, 2025

test: functional test failures under -DENABLE_WALLET=OFF #32347

Closed

jvsena42 mentioned this pull request May 2, 2025

Tópicos para o Bitdevs de Maio! plebemineira/bhbitdevs.org#17

Closed

Talkless mentioned this pull request May 18, 2025

Will Bitcoin Core's #31250 end EPS use case (in the current implementation)? chris-belcher/electrum-personal-server#309

Open

rkrux mentioned this pull request May 23, 2025

wallet, rpc, doc: various legacy wallet removal cleanups in RPCs #32596

Merged

rkrux mentioned this pull request Jun 12, 2025

wallet: Fix wallet interface detection of encrypted wallets #32620

Merged

pablomartin4btc mentioned this pull request Jun 15, 2025

wallet, refactor: Remove Legacy wallet unused warnings and errors #32481

Merged

theStack mentioned this pull request Nov 4, 2025

test: remove obsolete get_{key,multisig} helpers from wallet_util.py #33782

Merged

nahuhh mentioned this pull request Jan 3, 2026

bitcoin: bump to 29.2 basicswap/basicswap#413

Merged

Uh oh!

Conversation

achow101 commented Nov 7, 2024 • edited by fanquake Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Sjors commented Apr 24, 2025

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj left a comment

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc commented Apr 24, 2025

Uh oh!

achow101 commented Apr 24, 2025

Uh oh!

pablomartin4btc commented Apr 24, 2025

Uh oh!

pablomartin4btc commented Apr 24, 2025

Uh oh!

maflcko commented Apr 25, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko Apr 25, 2025

Choose a reason for hiding this comment

Uh oh!

achow101 Apr 25, 2025

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko Apr 25, 2025

Choose a reason for hiding this comment

Uh oh!

achow101 Apr 25, 2025

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fanquake commented Apr 25, 2025

Uh oh!

Uh oh!

pablomartin4btc commented Apr 25, 2025

Uh oh!

This comment was marked as abuse.

davidgumberg commented Apr 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

achow101 commented Nov 7, 2024 •

edited by fanquake

Loading

DrahtBot commented Nov 7, 2024 •

edited

Loading