cryptogera/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-01-31 02:31:07 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#34269: wallet: disallow creating new or restoring to an unnamed (default) wallet

Browse Source 
75b704df9d5ccb262d19ffe95b7ca6d1a934f3fb wallettool: Disallow creating new unnamed wallets (Ava Chow)
5875a9c502632eb5c74df07e41af38582da6e884 wallet: disallow unnamed wallets in createwallet and restorewallet (Ava Chow)
d30ad4a9129da04e249e3938f643dc47bf060e7e wallet, rpc: Use HandleWalletError in createwallet (Ava Chow)

Pull request description:

  We've been moving in the direction that all wallets must have a name. Therefore, we shouldn't allow creating new unnamed wallets. `createwallet`, `restorewallet`, and the wallet tool's `create` and `createfromdump` all now require the user to provide a non-empty wallet name when creating/restoring a wallet.

  The GUI is already enforcing this, but we were not enforcing it for RPCs or in the underlying `CreateWallet` and `RestoreWallet` functions.

  Wallet migration does still need to be able to restore unnamed wallets, so there is a new argument to `RestoreWallet` to explicitly allow that behavior for migration only.

ACKs for top commit:
  rkrux:
    lgtm ACK 75b704df9d5ccb262d19ffe95b7ca6d1a934f3fb
  polespinasa:
    re ACK 75b704df9d5ccb262d19ffe95b7ca6d1a934f3fb

Tree-SHA512: 8bde76d0b091e9276788c69412934af3426da2a7a69a00f94072d36c1a075cd41744ecdd5fef2b72870c1351b76aae061f124f716bb23f4839be20c464fc5ebd
This commit is contained in:
merge-script 2026-01-21 22:56:59 +01:00
commit 2a1234001c
No known key found for this signature in database
GPG Key ID: 9B79B45691DB4173
12 changed files with 64 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
src/wallet/db.h
View File

@ -196,6 +196,7 @@ enum class DatabaseStatus {
FAILED_VERIFY,
FAILED_ENCRYPT,
FAILED_INVALID_BACKUP_FILE,
FAILED_NEW_UNNAMED,
};
/** Recursively list database paths in directory. */

5
src/wallet/dump.cpp
View File

@ -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<bilingual_str>& 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()) {

4
src/wallet/rpc/util.cpp
View File

@ -142,9 +142,13 @@ void HandleWalletError(const std::shared_ptr<CWallet>& wallet, DatabaseStatus& s
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;
case DatabaseStatus::FAILED_ENCRYPT:
code = RPC_WALLET_ENCRYPTION_FAILED;
break;
default: // RPC_WALLET_ERROR is returned for all other cases.
break;
}

5
src/wallet/rpc/wallet.cpp
View File

@ -422,10 +422,7 @@ static RPCHelpMan createwallet()
bilingual_str error;
std::optional<bool> load_on_start = request.params[6].isNull() ? std::nullopt : std::optional<bool>(request.params[6].get_bool());
const std::shared_ptr<CWallet> 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());

19
src/wallet/wallet.cpp
View File

@ -376,6 +376,13 @@ std::shared_ptr<CWallet> LoadWallet(WalletContext& context, const std::string& n
std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string& name, std::optional<bool> load_on_start, DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& 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<CWallet> 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<CWallet> RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings, bool load_after_restore)
std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& 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<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
// 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};

2
src/wallet/wallet.h
View File

@ -97,7 +97,7 @@ std::shared_ptr<CWallet> GetDefaultWallet(WalletContext& context, size_t& count)
std::shared_ptr<CWallet> GetWallet(WalletContext& context, const std::string& name);
std::shared_ptr<CWallet> LoadWallet(WalletContext& context, const std::string& name, std::optional<bool> load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings);
std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string& name, std::optional<bool> load_on_start, DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings);
std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings, bool load_after_restore = true);
std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings, bool load_after_restore = true, bool allow_unnamed = false);
std::unique_ptr<interfaces::Handler> HandleLoadWallet(WalletContext& context, LoadWalletFn load_wallet);
void NotifyWalletLoaded(WalletContext& context, const std::shared_ptr<CWallet>& wallet);
std::unique_ptr<WalletDatabase> MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error);

6
src/wallet/wallettool.cpp
View File

@ -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;

1
test/functional/test_framework/test_node.py
View File

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

17
test/functional/tool_wallet.py
View File

@ -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__':

12
test/functional/wallet_backup.py
View File

@ -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):

1
test/functional/wallet_createwallet.py
View File

@ -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')

16
test/functional/wallet_startup.py
View File

@ -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(), [''])