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

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

src/wallet/rpc/util.cpp

@@ -142,6 +142,7 @@ void HandleWalletError(const std::shared_ptr<CWallet> 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;

src/wallet/wallet.cpp

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

src/wallet/wallet.h

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

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

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 = ""
+        assert_raises_rpc_error(-8, "Wallet name cannot be empty", node.restorewallet, "", backup_file)
-        res = node.restorewallet(wallet_name, backup_file)
+        assert not (node.wallets_path / "wallet.dat").exists()
-        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")
 
     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):

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

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