From d30ad4a9129da04e249e3938f643dc47bf060e7e Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 13 Jan 2026 13:10:53 -0800 Subject: [PATCH 1/3] wallet, rpc: Use HandleWalletError in createwallet Use the utility function HandleWalletError to deal with wallet creation errors in createwallet. --- src/wallet/rpc/util.cpp | 3 +++ src/wallet/rpc/wallet.cpp | 5 +---- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp index d68e6c65269..46e7d4371c5 100644 --- a/src/wallet/rpc/util.cpp +++ b/src/wallet/rpc/util.cpp @@ -145,6 +145,9 @@ void HandleWalletError(const std::shared_ptr wallet, DatabaseStatus& st case DatabaseStatus::FAILED_INVALID_BACKUP_FILE: code = RPC_INVALID_PARAMETER; break; + case DatabaseStatus::FAILED_ENCRYPT: + code = RPC_WALLET_ENCRYPTION_FAILED; + break; default: // RPC_WALLET_ERROR is returned for all other cases. break; } diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index c7c3f459fff..49808a8d696 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -422,10 +422,7 @@ static RPCHelpMan createwallet() bilingual_str error; std::optional load_on_start = request.params[6].isNull() ? std::nullopt : std::optional(request.params[6].get_bool()); const std::shared_ptr wallet = CreateWallet(context, request.params[0].get_str(), load_on_start, options, status, error, warnings); - if (!wallet) { - RPCErrorCode code = status == DatabaseStatus::FAILED_ENCRYPT ? RPC_WALLET_ENCRYPTION_FAILED : RPC_WALLET_ERROR; - throw JSONRPCError(code, error.original); - } + HandleWalletError(wallet, status, error); UniValue obj(UniValue::VOBJ); obj.pushKV("name", wallet->GetName()); From 5875a9c502632eb5c74df07e41af38582da6e884 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 6 Jan 2026 14:01:28 -0800 Subject: [PATCH 2/3] 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(), ['']) From 75b704df9d5ccb262d19ffe95b7ca6d1a934f3fb Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 6 Jan 2026 15:43:29 -0800 Subject: [PATCH 3/3] wallettool: Disallow creating new unnamed wallets --- src/wallet/dump.cpp | 5 +++++ src/wallet/wallettool.cpp | 6 +++++- test/functional/tool_wallet.py | 17 +++++++++++------ 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/wallet/dump.cpp b/src/wallet/dump.cpp index eb6fa0e4ec0..f7f6554b57a 100644 --- a/src/wallet/dump.cpp +++ b/src/wallet/dump.cpp @@ -121,6 +121,11 @@ static void WalletToolReleaseWallet(CWallet* wallet) bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs::path& wallet_path, bilingual_str& error, std::vector& warnings) { + if (name.empty()) { + tfm::format(std::cerr, "Wallet name cannot be empty\n"); + return false; + } + // Get the dumpfile std::string dump_filename = args.GetArg("-dumpfile", ""); if (dump_filename.empty()) { diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index 3e335bc36bd..1ac0bbb72f0 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -111,7 +111,7 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) tfm::format(std::cerr, "The -dumpfile option can only be used with the \"dump\" and \"createfromdump\" commands.\n"); return false; } - if (command == "create" && !args.IsArgSet("-wallet")) { + if ((command == "create" || command == "createfromdump") && !args.IsArgSet("-wallet")) { tfm::format(std::cerr, "Wallet name must be provided when creating a new wallet.\n"); return false; } @@ -119,6 +119,10 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) const fs::path path = fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(name)); if (command == "create") { + if (name.empty()) { + tfm::format(std::cerr, "Wallet name cannot be empty\n"); + return false; + } DatabaseOptions options; ReadDatabaseArgs(args, options); options.require_create = true; diff --git a/test/functional/tool_wallet.py b/test/functional/tool_wallet.py index 44ac1da96fa..9255b26166c 100755 --- a/test/functional/tool_wallet.py +++ b/test/functional/tool_wallet.py @@ -136,6 +136,7 @@ class ToolWalletTest(BitcoinTestFramework): self.assert_raises_tool_error('Error parsing command line arguments: Invalid parameter -foo', '-foo') self.assert_raises_tool_error('No method provided. Run `bitcoin-wallet -help` for valid methods.') self.assert_raises_tool_error('Wallet name must be provided when creating a new wallet.', 'create') + self.assert_raises_tool_error('Wallet name must be provided when creating a new wallet.', 'createfromdump') error = f"SQLiteDatabase: Unable to obtain an exclusive lock on the database, is it being used by another instance of {self.config['environment']['CLIENT_NAME']}?" self.assert_raises_tool_error( error, @@ -319,12 +320,6 @@ class ToolWalletTest(BitcoinTestFramework): self.write_dump(dump_data, bad_sum_wallet_dump) self.assert_raises_tool_error('Error: Checksum is not the correct size', '-wallet=badload', '-dumpfile={}'.format(bad_sum_wallet_dump), 'createfromdump') assert not (self.nodes[0].wallets_path / "badload").is_dir() - self.assert_raises_tool_error('Error: Checksum is not the correct size', '-wallet=', '-dumpfile={}'.format(bad_sum_wallet_dump), 'createfromdump') - assert self.nodes[0].wallets_path.exists() - assert not (self.nodes[0].wallets_path / "wallet.dat").exists() - - self.log.info('Checking createfromdump with an unnamed wallet') - self.do_tool_createfromdump("", "wallet.dump") def test_chainless_conflicts(self): self.log.info("Test wallet tool when wallet contains conflicting transactions") @@ -427,6 +422,15 @@ class ToolWalletTest(BitcoinTestFramework): self.assert_raises_tool_error("Invalid parameter -descriptors", "-wallet=legacy", "-descriptors=false", "create") assert not (self.nodes[0].wallets_path / "legacy").exists() + def test_no_create_unnamed(self): + self.log.info("Test that unnamed (default) wallets cannot be created") + + self.assert_raises_tool_error("Wallet name cannot be empty", "-wallet=", "create") + assert not (self.nodes[0].wallets_path / "wallet.dat").exists() + + self.assert_raises_tool_error("Wallet name cannot be empty", "-wallet=", "-dumpfile=wallet.dump", "createfromdump") + assert not (self.nodes[0].wallets_path / "wallet.dat").exists() + def run_test(self): self.wallet_path = self.nodes[0].wallets_path / self.default_wallet_name / self.wallet_data_filename self.test_invalid_tool_commands_and_args() @@ -439,6 +443,7 @@ class ToolWalletTest(BitcoinTestFramework): self.test_chainless_conflicts() self.test_dump_very_large_records() self.test_no_create_legacy() + self.test_no_create_unnamed() if __name__ == '__main__':