Create new wallet databases as directories rather than files

This change should make it easier for users to make complete backups of wallets
because they can now just back up the specified `-wallet=<path>` path directly,
instead of having to back up the specified path as well as the transaction log
directory (for incompletely flushed wallets).

Another advantage of this change is that if two wallets are located in the same
directory, they will now use their own BerkeleyDB environments instead using a
shared environment. Using a shared environment makes it difficult to manage and
back up wallets separately because transaction log files will contain a mix of
data from all wallets in the environment.

doc/release-notes.md

@@ -67,10 +67,31 @@ External wallet files
 ---------------------
 
 The `-wallet=<path>` option now accepts full paths instead of requiring wallets
-to be located in the -walletdir directory. When wallets are located in
-different directories, wallet data will be stored independently, so data from
-every wallet is not mixed into the same <walletdir>/database/log.??????????
-files.
+to be located in the -walletdir directory.
+
+Newly created wallet format
+---------------------------
+
+If `-wallet=<path>` is specified with a path that does not exist, it will now
+create a wallet directory at the specified location (containing a wallet.dat
+data file, a db.log file, and database/log.?????????? files) instead of just
+creating a data file at the path and storing log files in the parent
+directory. This should make backing up wallets more straightforward than
+before because the specified wallet path can just be directly archived without
+having to look in the parent directory for transaction log files.
+
+For backwards compatibility, wallet paths that are names of existing data files
+in the `-walletdir` directory will continue to be accepted and interpreted the
+same as before.
+
+Low-level RPC changes
+---------------------
+
+- When bitcoin is not started with any `-wallet=<path>` options, the name of
+  the default wallet returned by `getwalletinfo` and `listwallets` RPCs is
+  now the empty string `""` instead of `"wallet.dat"`. If bitcoin is started
+  with any `-wallet=<path>` options, there is no change in behavior, and the
+  name of any wallet is just its `<path>` string.
 
 Credits
 =======

src/bitcoin-cli.cpp

@@ -339,8 +339,8 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co
 
     // check if we should use a special wallet endpoint
     std::string endpoint = "/";
+    if (!gArgs.GetArgs("-rpcwallet").empty()) {
         std::string walletName = gArgs.GetArg("-rpcwallet", "");
-    if (!walletName.empty()) {
         char *encodedURI = evhttp_uriencode(walletName.c_str(), walletName.size(), false);
         if (encodedURI) {
             endpoint = "/wallet/"+ std::string(encodedURI);

src/wallet/db.cpp

@@ -59,8 +59,19 @@ std::map<std::string, CDBEnv> g_dbenvs; //!< Map from directory name to open db
 
 CDBEnv* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename)
 {
-    fs::path env_directory = wallet_path.parent_path();
+    fs::path env_directory;
+    if (fs::is_regular_file(wallet_path)) {
+        // Special case for backwards compatibility: if wallet path points to an
+        // existing file, treat it as the path to a BDB data file in a parent
+        // directory that also contains BDB log files.
+        env_directory = wallet_path.parent_path();
         database_filename = wallet_path.filename().string();
+    } else {
+        // Normal case: Interpret wallet path as a directory path containing
+        // data and log files.
+        env_directory = wallet_path;
+        database_filename = "wallet.dat";
+    }
     LOCK(cs_db);
     // Note: An ununsed temporary CDBEnv object may be created inside the
     // emplace function if the key already exists. This is a little inefficient,

src/wallet/init.cpp

@@ -35,7 +35,7 @@ std::string GetWalletHelpString(bool showDebug)
     strUsage += HelpMessageOpt("-spendzeroconfchange", strprintf(_("Spend unconfirmed change when sending transactions (default: %u)"), DEFAULT_SPEND_ZEROCONF_CHANGE));
     strUsage += HelpMessageOpt("-txconfirmtarget=<n>", strprintf(_("If paytxfee is not set, include enough fee so transactions begin confirmation on average within n blocks (default: %u)"), DEFAULT_TX_CONFIRM_TARGET));
     strUsage += HelpMessageOpt("-upgradewallet", _("Upgrade wallet to latest format on startup"));
-    strUsage += HelpMessageOpt("-wallet=<path>", _("Specify wallet database path. Can be specified multiple times to load multiple wallets. Path is interpreted relative to <walletdir> if it is not absolute, and will be created if it does not exist.") + " " + strprintf(_("(default: %s)"), DEFAULT_WALLET_DAT));
+    strUsage += HelpMessageOpt("-wallet=<path>", _("Specify wallet database path. Can be specified multiple times to load multiple wallets. Path is interpreted relative to <walletdir> if it is not absolute, and will be created if it does not exist (as a directory containing a wallet.dat file and log files). For backwards compatibility this will also accept names of existing data files in <walletdir>.)"));
     strUsage += HelpMessageOpt("-walletbroadcast", _("Make the wallet broadcast transactions") + " " + strprintf(_("(default: %u)"), DEFAULT_WALLETBROADCAST));
     strUsage += HelpMessageOpt("-walletdir=<dir>", _("Specify directory to hold wallets (default: <datadir>/wallets if it exists, otherwise <datadir>)"));
     strUsage += HelpMessageOpt("-walletnotify=<cmd>", _("Execute command when a wallet transaction changes (%s in cmd is replaced by TxID)"));
@@ -66,7 +66,7 @@ bool WalletParameterInteraction()
         return true;
     }
 
-    gArgs.SoftSetArg("-wallet", DEFAULT_WALLET_DAT);
+    gArgs.SoftSetArg("-wallet", "");
     const bool is_multiwallet = gArgs.GetArgs("-wallet").size() > 1;
 
     if (gArgs.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY) && gArgs.SoftSetBoolArg("-walletbroadcast", false)) {
@@ -230,10 +230,22 @@ bool VerifyWallets()
     std::set<fs::path> wallet_paths;
 
     for (const std::string& walletFile : gArgs.GetArgs("-wallet")) {
+        // Do some checking on wallet path. It should be either a:
+        //
+        // 1. Path where a directory can be created.
+        // 2. Path to an existing directory.
+        // 3. Path to a symlink to a directory.
+        // 4. For backwards compatibility, the name of a data file in -walletdir.
         fs::path wallet_path = fs::absolute(walletFile, GetWalletDir());
-
-        if (fs::exists(wallet_path) && (!fs::is_regular_file(wallet_path) || fs::is_symlink(wallet_path))) {
-            return InitError(strprintf(_("Error loading wallet %s. -wallet filename must be a regular file."), walletFile));
+        fs::file_type path_type = fs::symlink_status(wallet_path).type();
+        if (!(path_type == fs::file_not_found || path_type == fs::directory_file ||
+              (path_type == fs::symlink_file && fs::is_directory(wallet_path)) ||
+              (path_type == fs::regular_file && fs::path(walletFile).filename() == walletFile))) {
+            return InitError(strprintf(
+                _("Invalid -wallet path '%s'. -wallet path should point to a directory where wallet.dat and "
+                  "database/log.?????????? files can be stored, a location where such a directory could be created, "
+                  "or (for backwards compatibility) the name of an existing data file in -walletdir (%s)"),
+                walletFile, GetWalletDir()));
         }
 
         if (!wallet_paths.insert(wallet_path).second) {

src/wallet/wallet.cpp

@@ -45,7 +45,6 @@ OutputType g_address_type = OUTPUT_TYPE_NONE;
 OutputType g_change_type = OUTPUT_TYPE_NONE;
 bool g_wallet_allow_fallback_fee = true; //<! will be defined via chainparams
 
-const char * DEFAULT_WALLET_DAT = "wallet.dat";
 const uint32_t BIP32_HARDENED_KEY_LIMIT = 0x80000000;
 
 /**

src/wallet/wallet.h

@@ -68,8 +68,6 @@ static const bool DEFAULT_WALLET_RBF = false;
 static const bool DEFAULT_WALLETBROADCAST = true;
 static const bool DEFAULT_DISABLE_WALLET = false;
 
-extern const char * DEFAULT_WALLET_DAT;
-
 static const int64_t TIMESTAMP_MIN = 0;
 
 class CBlockIndex;

test/functional/feature_config_args.py

@@ -37,13 +37,13 @@ class ConfArgsTest(BitcoinTestFramework):
         os.mkdir(new_data_dir)
         self.start_node(0, ['-conf='+conf_file, '-wallet=w1'])
         self.stop_node(0)
-        assert os.path.isfile(os.path.join(new_data_dir, 'regtest', 'wallets', 'w1'))
+        assert os.path.exists(os.path.join(new_data_dir, 'regtest', 'wallets', 'w1'))
 
         # Ensure command line argument overrides datadir in conf
         os.mkdir(new_data_dir_2)
         self.nodes[0].datadir = new_data_dir_2
         self.start_node(0, ['-datadir='+new_data_dir_2, '-conf='+conf_file, '-wallet=w2'])
-        assert os.path.isfile(os.path.join(new_data_dir_2, 'regtest', 'wallets', 'w2'))
+        assert os.path.exists(os.path.join(new_data_dir_2, 'regtest', 'wallets', 'w2'))
 
 if __name__ == '__main__':
     ConfArgsTest().main()

test/functional/wallet_multiwallet.py

@@ -29,13 +29,24 @@ class MultiWalletTest(BitcoinTestFramework):
         self.stop_nodes()
         assert_equal(os.path.isfile(wallet_dir('wallet.dat')), True)
 
+        # create symlink to verify wallet directory path can be referenced
+        # through symlink
+        os.mkdir(wallet_dir('w7'))
+        os.symlink('w7', wallet_dir('w7_symlink'))
+
+        # rename wallet.dat to make sure plain wallet file paths (as opposed to
+        # directory paths) can be loaded
+        os.rename(wallet_dir("wallet.dat"), wallet_dir("w8"))
+
         # restart node with a mix of wallet names:
         #   w1, w2, w3 - to verify new wallets created when non-existing paths specified
         #   w          - to verify wallet name matching works when one wallet path is prefix of another
         #   sub/w5     - to verify relative wallet path is created correctly
         #   extern/w6  - to verify absolute wallet path is created correctly
-        #   wallet.dat - to verify existing wallet file is loaded correctly
-        wallet_names = ['w1', 'w2', 'w3', 'w', 'sub/w5', os.path.join(self.options.tmpdir, 'extern/w6'), 'wallet.dat']
+        #   w7_symlink - to verify symlinked wallet path is initialized correctly
+        #   w8         - to verify existing wallet file is loaded correctly
+        #   ''         - to verify default wallet file is created correctly
+        wallet_names = ['w1', 'w2', 'w3', 'w', 'sub/w5', os.path.join(self.options.tmpdir, 'extern/w6'), 'w7_symlink', 'w8', '']
         extra_args = ['-wallet={}'.format(n) for n in wallet_names]
         self.start_node(0, extra_args)
         assert_equal(set(node.listwallets()), set(wallet_names))
@@ -43,10 +54,13 @@ class MultiWalletTest(BitcoinTestFramework):
         # check that all requested wallets were created
         self.stop_node(0)
         for wallet_name in wallet_names:
+            if os.path.isdir(wallet_dir(wallet_name)):
+                assert_equal(os.path.isfile(wallet_dir(wallet_name, "wallet.dat")), True)
+            else:
                 assert_equal(os.path.isfile(wallet_dir(wallet_name)), True)
 
         # should not initialize if wallet path can't be created
-        self.assert_start_raises_init_error(0, ['-wallet=wallet.dat/bad'], 'File exists')
+        self.assert_start_raises_init_error(0, ['-wallet=wallet.dat/bad'], 'Not a directory')
 
         self.assert_start_raises_init_error(0, ['-walletdir=wallets'], 'Error: Specified -walletdir "wallets" does not exist')
         self.assert_start_raises_init_error(0, ['-walletdir=wallets'], 'Error: Specified -walletdir "wallets" is a relative path', cwd=data_dir())
@@ -55,17 +69,13 @@ class MultiWalletTest(BitcoinTestFramework):
         # should not initialize if there are duplicate wallets
         self.assert_start_raises_init_error(0, ['-wallet=w1', '-wallet=w1'], 'Error loading wallet w1. Duplicate -wallet filename specified.')
 
-        # should not initialize if wallet file is a directory
-        os.mkdir(wallet_dir('w11'))
-        self.assert_start_raises_init_error(0, ['-wallet=w11'], 'Error loading wallet w11. -wallet filename must be a regular file.')
-
         # should not initialize if one wallet is a copy of another
-        shutil.copyfile(wallet_dir('w2'), wallet_dir('w22'))
-        self.assert_start_raises_init_error(0, ['-wallet=w2', '-wallet=w22'], 'duplicates fileid')
+        shutil.copyfile(wallet_dir('w8'), wallet_dir('w8_copy'))
+        self.assert_start_raises_init_error(0, ['-wallet=w8', '-wallet=w8_copy'], 'duplicates fileid')
 
         # should not initialize if wallet file is a symlink
-        os.symlink(wallet_dir('w1'), wallet_dir('w12'))
-        self.assert_start_raises_init_error(0, ['-wallet=w12'], 'Error loading wallet w12. -wallet filename must be a regular file.')
+        os.symlink('w8', wallet_dir('w8_symlink'))
+        self.assert_start_raises_init_error(0, ['-wallet=w8_symlink'], 'Invalid -wallet path')
 
         # should not initialize if the specified walletdir does not exist
         self.assert_start_raises_init_error(0, ['-walletdir=bad'], 'Error: Specified -walletdir "bad" does not exist')