From 5875a9c502632eb5c74df07e41af38582da6e884 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 6 Jan 2026 14:01:28 -0800 Subject: [PATCH] wallet: disallow unnamed wallets in createwallet and restorewallet Migration still needs to be able to restore unnamed wallets, so allow_unnamed is added to RestoreWallet to explicitly allow that behavior for migration only. --- src/wallet/db.h | 1 + src/wallet/rpc/util.cpp | 1 + src/wallet/wallet.cpp | 19 +++++++++++++++++-- src/wallet/wallet.h | 2 +- test/functional/test_framework/test_node.py | 1 - test/functional/wallet_backup.py | 12 +++--------- test/functional/wallet_createwallet.py | 1 + test/functional/wallet_startup.py | 16 +++++++++++++++- 8 files changed, 39 insertions(+), 14 deletions(-) diff --git a/src/wallet/db.h b/src/wallet/db.h index b953ab1f25b..b549e2a7514 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -196,6 +196,7 @@ enum class DatabaseStatus { FAILED_VERIFY, FAILED_ENCRYPT, FAILED_INVALID_BACKUP_FILE, + FAILED_NEW_UNNAMED, }; /** Recursively list database paths in directory. */ diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp index 46e7d4371c5..012e0996e7e 100644 --- a/src/wallet/rpc/util.cpp +++ b/src/wallet/rpc/util.cpp @@ -142,6 +142,7 @@ void HandleWalletError(const std::shared_ptr wallet, DatabaseStatus& st case DatabaseStatus::FAILED_ALREADY_EXISTS: code = RPC_WALLET_ALREADY_EXISTS; break; + case DatabaseStatus::FAILED_NEW_UNNAMED: case DatabaseStatus::FAILED_INVALID_BACKUP_FILE: code = RPC_INVALID_PARAMETER; break; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 56c676c189f..f02b254c80b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -376,6 +376,13 @@ std::shared_ptr LoadWallet(WalletContext& context, const std::string& n std::shared_ptr CreateWallet(WalletContext& context, const std::string& name, std::optional load_on_start, DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings) { + // Wallet must have a non-empty name + if (name.empty()) { + error = Untranslated("Wallet name cannot be empty"); + status = DatabaseStatus::FAILED_NEW_UNNAMED; + return nullptr; + } + uint64_t wallet_creation_flags = options.create_flags; const SecureString& passphrase = options.create_passphrase; @@ -461,8 +468,16 @@ std::shared_ptr CreateWallet(WalletContext& context, const std::string& // Re-creates wallet from the backup file by renaming and moving it into the wallet's directory. // If 'load_after_restore=true', the wallet object will be fully initialized and appended to the context. -std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector& warnings, bool load_after_restore) +std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector& warnings, bool load_after_restore, bool allow_unnamed) { + // Error if the wallet name is empty and allow_unnamed == false + // allow_unnamed == true is only used by migration to migrate an unnamed wallet + if (!allow_unnamed && wallet_name.empty()) { + error = Untranslated("Wallet name cannot be empty"); + status = DatabaseStatus::FAILED_NEW_UNNAMED; + return nullptr; + } + DatabaseOptions options; ReadDatabaseArgs(*context.args, options); options.require_existing = true; @@ -4442,7 +4457,7 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // Restore the backup // Convert the backup file to the wallet db file by renaming it and moving it into the wallet's directory. bilingual_str restore_error; - const auto& ptr_wallet = RestoreWallet(context, backup_path, wallet_name, /*load_on_start=*/std::nullopt, status, restore_error, warnings, /*load_after_restore=*/false); + const auto& ptr_wallet = RestoreWallet(context, backup_path, wallet_name, /*load_on_start=*/std::nullopt, status, restore_error, warnings, /*load_after_restore=*/false, /*allow_unnamed=*/true); if (!restore_error.empty()) { error += restore_error + _("\nUnable to restore backup of wallet."); return util::Error{error}; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 24d6b07d356..b39827b6b8f 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -97,7 +97,7 @@ std::shared_ptr GetDefaultWallet(WalletContext& context, size_t& count) std::shared_ptr GetWallet(WalletContext& context, const std::string& name); std::shared_ptr LoadWallet(WalletContext& context, const std::string& name, std::optional load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings); std::shared_ptr CreateWallet(WalletContext& context, const std::string& name, std::optional load_on_start, DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings); -std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector& warnings, bool load_after_restore = true); +std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector& warnings, bool load_after_restore = true, bool allow_unnamed = false); std::unique_ptr HandleLoadWallet(WalletContext& context, LoadWalletFn load_wallet); void NotifyWalletLoaded(WalletContext& context, const std::shared_ptr& wallet); std::unique_ptr MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error); diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 77d48fe5987..b56cf64ce99 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -907,7 +907,6 @@ class TestNode(): def wait_until(self, test_function, timeout=60, check_interval=0.05): return wait_until_helper_internal(test_function, timeout=timeout, timeout_factor=self.timeout_factor, check_interval=check_interval) - class TestNodeCLIAttr: def __init__(self, cli, command): self.cli = cli diff --git a/test/functional/wallet_backup.py b/test/functional/wallet_backup.py index 5cf02ba39b9..06b8799d348 100755 --- a/test/functional/wallet_backup.py +++ b/test/functional/wallet_backup.py @@ -184,13 +184,8 @@ class WalletBackupTest(BitcoinTestFramework): # This is also useful to test the migration recovery after failure logic node = self.nodes[3] backup_file = self.nodes[0].datadir_path / 'wallet.bak' - wallet_name = "" - res = node.restorewallet(wallet_name, backup_file) - assert_equal(res['name'], "") - assert (node.wallets_path / "wallet.dat").exists() - # Clean for follow-up tests - node.unloadwallet("") - os.remove(node.wallets_path / "wallet.dat") + assert_raises_rpc_error(-8, "Wallet name cannot be empty", node.restorewallet, "", backup_file) + assert not (node.wallets_path / "wallet.dat").exists() def test_pruned_wallet_backup(self): self.log.info("Test loading backup on a pruned node when the backup was created close to the prune height of the restoring node") @@ -213,9 +208,8 @@ class WalletBackupTest(BitcoinTestFramework): self.log.info("Test restore on a pruned node when the backup was beyond the pruning point") backup_file = self.nodes[0].datadir_path / 'wallet.bak' - wallet_name = "" error_message = "Wallet loading failed. Prune: last wallet synchronisation goes beyond pruned data. You need to -reindex (download the whole blockchain again in case of a pruned node)" - assert_raises_rpc_error(-4, error_message, node.restorewallet, wallet_name, backup_file) + assert_raises_rpc_error(-4, error_message, node.restorewallet, "restore_pruned", backup_file) assert node.wallets_path.exists() # ensure the wallets dir exists def run_test(self): diff --git a/test/functional/wallet_createwallet.py b/test/functional/wallet_createwallet.py index 9dd085bc5e0..217a331d26d 100755 --- a/test/functional/wallet_createwallet.py +++ b/test/functional/wallet_createwallet.py @@ -32,6 +32,7 @@ class CreateWalletTest(BitcoinTestFramework): # Run createwallet with invalid parameters. This must not prevent a new wallet with the same name from being created with the correct parameters. assert_raises_rpc_error(-4, "Passphrase provided but private keys are disabled. A passphrase is only used to encrypt private keys, so cannot be used for wallets with private keys disabled.", self.nodes[0].createwallet, wallet_name='w0', disable_private_keys=True, passphrase="passphrase") + assert_raises_rpc_error(-8, "Wallet name cannot be empty", self.nodes[0].createwallet, "") self.nodes[0].createwallet(wallet_name='w0') w0 = node.get_wallet_rpc('w0') diff --git a/test/functional/wallet_startup.py b/test/functional/wallet_startup.py index 17958b75ed2..d2355360ac2 100755 --- a/test/functional/wallet_startup.py +++ b/test/functional/wallet_startup.py @@ -6,6 +6,9 @@ Verify that a bitcoind node can maintain list of wallets loading on startup """ +import shutil +import uuid + from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, @@ -24,13 +27,24 @@ class WalletStartupTest(BitcoinTestFramework): self.add_nodes(self.num_nodes) self.start_nodes() + def create_unnamed_wallet(self, **kwargs): + """ + createwallet disallows empty wallet names, so create a temporary named wallet + and move its wallet.dat to the unnamed wallet location + """ + wallet_name = uuid.uuid4().hex + self.nodes[0].createwallet(wallet_name=wallet_name, **kwargs) + self.nodes[0].unloadwallet(wallet_name) + shutil.move(self.nodes[0].wallets_path / wallet_name / "wallet.dat", self.nodes[0].wallets_path / "wallet.dat") + shutil.rmtree(self.nodes[0].wallets_path / wallet_name) + def run_test(self): self.log.info('Should start without any wallets') assert_equal(self.nodes[0].listwallets(), []) assert_equal(self.nodes[0].listwalletdir(), {'wallets': []}) self.log.info('New default wallet should load by default when there are no other wallets') - self.nodes[0].createwallet(wallet_name='', load_on_startup=False) + self.create_unnamed_wallet(load_on_startup=False) self.restart_node(0) assert_equal(self.nodes[0].listwallets(), [''])