Merge #2423: [Wallet] Introduce wallets directory configuration and e…

…xternal wallet files capabilities

056f4e8 [GUI] settings information widget setting the correct data directory. (furszy)
f765611 bugfix: Remove dangling wallet env instance (João Barbosa)
524103f Implement and connect CWallet::GetPathToFile to GUI. (furszy)
e7aa6bd [Refactor] First load all wallets, then back em (random-zebra)
91b112b [Refactor][Bug] Fix automatic backups, final code deduplication (random-zebra)
12a1e39 [BUG] Sanitize wallet name in GetUniqueWalletBackupName (random-zebra)
7aa251d wallet: Fix backupwallet for multiwallets (Daniel Kraft)
351d2c8 wallet_tests: mock wallet db. (furszy)
565abcd db: fix db path not removed from the open db environments map. (furszy)
4cae8dc test: Add unit test for LockDirectory  Add a unit test for LockDirectory, introduced in btc#11281. (W. J. van der Laan)
16b4651 util: Fix multiple use of LockDirectory  This commit fixes problems with calling LockDirectory multiple times on the same directory, or from multiple threads. It also fixes the build on OpenBSD. (W. J. van der Laan)
9ae619a Test: Use specific testing setups for wallet_zkeys_tests tests (furszy)
d86cd4f wallet: Add missing check for backup to source wallet file. (furszy)
d9e1c6b Abstract VerifyWalletPath and connect it to init and GUI. (furszy)
23458ca GUI: Implement and connect WalletModel::getWalletPath(). (furszy)
c2d3a07 Create new wallet databases as directories rather than files (Russell Yanofsky)
daa7fe5 Allow wallet files not in -walletdir directory (Russell Yanofsky)
9b2dae1 Allow wallet files in multiple directories (furszy)
d36185a wallet: unify backup creation process. (furszy)
8b8725d wallet_tests: Use dummy wallet instance instead of wallet pointer. (furszy)
434ed75 Abstract LockDirectory into system.cpp (furszy)
6a0380a Make .walletlock distinct from .lock (MeshCollider)
d8539bb Generalise walletdir lock error message for correctness (MeshCollider)
ddcfd4a Enable test for wallet directory locking (furszy)
a238a8d Add a lock to the wallet directory (MeshCollider)
1dc2219 Don't allow relative -walletdir paths (Russell Yanofsky)
dcb43ea Create walletdir if datadir doesn't exist and correct tests (furszy)
03db5c8 Default walletdir is wallets/ if it exists (MeshCollider)
359b01d Add release notes for -walletdir and wallets/ dir (MeshCollider)
71a4701 Add -walletdir parameter to specify custom wallet dir (furszy)
5b31813 Use unique_ptr for dbenv (DbEnv) (practicalswift)
a1bef4f Refactor: Modernize disallowed copy constructors/assignment (Dan Raviv)

Pull request description:

  > Adding more flexibility in where the wallets directory can be located. Before, the wallet database files were stored at the top level of the PIVX data directory. Now the location of the wallets directory can be overridden by specifying a `-walletdir=<path>` option where `<path>` can be an absolute path to a directory or directory symlink.
  >
  >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.

  Coming from:
  * bitcoin#11351 -> Refactor: Modernize disallowed copy constructors/assignment.
  * bitcoin#11466 -> Specify custom wallet directory with -walletdir param.
  * bitcoin#11726 -> Cleanups + nit fixes for -walletdir work.
  * bitcoin#11904 -> Add lock to the wallet directory.
  * bitcoin#11687 -> External wallet files support.
  * bitcoin#12166 -> Doc better -walletdir description.
  * bitcoin#12220 -> Error if relative -walletdir is specified.

  This finishes the work started in #2337, enabling all the commented tests inside `wallet_multiwallet.py` and more.

  PR built on top of #2369.

  Note: Test this properly.

ACKs for top commit:
  random-zebra:
    utACK 056f4e8
  Fuzzbawls:
    ACK 056f4e8

Tree-SHA512: 98ae515dd664f959d35b63a0998bd93ca3bcea30ca67caccd2a694a440d10e18f831a54720ede0415d8f5e13af252bc6048a820491863d243df70ccc9d5fa7c6

CMakeLists.txt

@@ -267,6 +267,7 @@ set(WALLET_SOURCES
         ./src/legacy/stakemodifier.cpp
         ./src/wallet/wallet.cpp
         ./src/wallet/walletdb.cpp
+        ./src/wallet/walletutil.cpp
         ./src/zpiv/zpivmodule.cpp
         ./src/zpiv/zpos.cpp
         ./src/stakeinput.cpp

doc/release-notes.md

@@ -299,6 +299,56 @@ The `getwalletinfo` RPC method returns the name of the wallet used for the query
 
 Note that while multi-wallet is now fully supported, the RPC multi-wallet interface should be considered unstable for version 6.0.0, and there may backwards-incompatible changes in future versions.
 
+Wallets directory configuration (`-walletdir`)
+----------------------------------------------
+
+PIVX Core now has more flexibility in where the wallets directory can be
+located. Previously wallet database files were stored at the top level of the
+PIVX data directory. The behavior is now:
+
+- For new installations (where the data directory doesn't already exist),
+  wallets will now be stored in a new `wallets/` subdirectory inside the data
+  directory by default.
+- For existing nodes (where the data directory already exists), wallets will be
+  stored in the data directory root by default. If a `wallets/` subdirectory
+  already exists in the data directory root, then wallets will be stored in the
+  `wallets/` subdirectory by default.
+- The location of the wallets directory can be overridden by specifying a
+  `-walletdir=<path>` option where `<path>` can be an absolute path to a
+  directory or directory symlink.
+
+Care should be taken when choosing the wallets directory location, as if it
+becomes unavailable during operation, funds may be lost.
+
+External wallet files
+---------------------
+
+The `-wallet=<path>` option now accepts full paths instead of requiring wallets
+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 PIVX 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 PIVX is started
+  with any `-wallet=<path>` options, there is no change in behavior, and the
+  name of any wallet is just its `<path>` string.
 
 Database cache memory increased
 --------------------------------

src/Makefile.am

@@ -305,6 +305,7 @@ BITCOIN_CORE_H = \
   wallet/init.h \
   wallet/wallet.h \
   wallet/walletdb.h \
+  wallet/walletutil.h \
   warnings.h \
   zpivchain.h \
   zpiv/zpos.h \
@@ -408,6 +409,7 @@ libbitcoin_wallet_a_SOURCES = \
   destination_io.cpp \
   wallet/wallet.cpp \
   wallet/walletdb.cpp \
+  wallet/walletutil.cpp \
   stakeinput.cpp \
   zpiv/zpos.cpp \
   $(BITCOIN_CORE_H) \

src/coins.h

@@ -300,6 +300,11 @@ class CCoinsViewCache : public CCoinsViewBacked
 public:
     CCoinsViewCache(CCoinsView *baseIn);
 
+    /**
+     * By deleting the copy constructor, we prevent accidentally using it when one intends to create a cache on top of a base cache.
+     */
+    CCoinsViewCache(const CCoinsViewCache &) = delete;
+
     // Sapling methods
     bool GetSaplingAnchorAt(const uint256 &rt, SaplingMerkleTree &tree) const override;
     bool GetNullifier(const uint256 &nullifier) const override;
@@ -430,11 +435,6 @@ class CCoinsViewCache : public CCoinsViewBacked
             const uint256 &currentRoot,
             Tree &tree
     );
-
-    /**
-      * By making the copy constructor private, we prevent accidentally using it when one intends to create a cache on top of a base cache.
-      */
-    CCoinsViewCache(const CCoinsViewCache &);
 };
 
 //! Utility function to add all of a transaction's outputs to a cache.

src/init.cpp

@@ -847,8 +847,6 @@ namespace { // Variables internal to initialization process only
     int nUserMaxConnections;
     int nFD;
     ServiceFlags nLocalServices = NODE_NETWORK;
-
-    std::string strWalletFile;
 }
 
 bool AppInitBasicSetup()
@@ -1173,7 +1171,6 @@ bool AppInitParameterInteraction()
     }
 
 #ifdef ENABLE_WALLET
-    strWalletFile = gArgs.GetArg("-wallet", DEFAULT_WALLET_DAT);
     if (!WalletParameterInteraction())
         return false;
 #endif // ENABLE_WALLET
@@ -1199,27 +1196,10 @@ bool AppInitParameterInteraction()
 
 static bool LockDataDirectory(bool probeOnly)
 {
-    std::string strDataDir = GetDataDir().string();
-
     // Make sure only a single PIVX process is using the data directory.
-    fs::path pathLockFile = GetDataDir() / ".lock";
-    FILE* file = fsbridge::fopen(pathLockFile, "a"); // empty lock file; created if it doesn't exist.
-    if (file) fclose(file);
-
-    try {
-        static boost::interprocess::file_lock lock(pathLockFile.string().c_str());
-        // Wait maximum 10 seconds if an old wallet is still running. Avoids lockup during restart
-        if (!lock.timed_lock(boost::get_system_time() + boost::posix_time::seconds(10))) {
-            return UIError(strprintf(_("Cannot obtain a lock on data directory %s. %s is probably already running."),
-                                     strDataDir, _(PACKAGE_NAME)));
-        }
-        if (probeOnly) {
-            lock.unlock();
-        }
-    } catch (const boost::interprocess::interprocess_exception& e) {
-        return UIError(
-                strprintf(_("Cannot obtain a lock on data directory %s. %s is probably already running.") + " %s.",
-                          strDataDir, _(PACKAGE_NAME), e.what()));
+    fs::path datadir = GetDataDir();
+    if (!LockDirectory(datadir, ".lock", probeOnly)) {
+        return UIError(strprintf(_("Cannot obtain a lock on data directory %s. %s is probably already running."), datadir.string(), _(PACKAGE_NAME)));
     }
     return true;
 }
@@ -1272,6 +1252,15 @@ bool AppInitMain()
     LogPrintf("Using at most %i connections (%i file descriptors available)\n", nMaxConnections, nFD);
     std::ostringstream strErrors;
 
+    // Warn about relative -datadir path.
+    if (gArgs.IsArgSet("-datadir") && !fs::path(gArgs.GetArg("-datadir", "")).is_absolute()) {
+        LogPrintf("Warning: relative datadir option '%s' specified, which will be interpreted relative to the "
+                  "current working directory '%s'. This is fragile because if PIVX is started in the future "
+                  "from a different location. It will be unable to locate the current data files. There could "
+                  "also be data loss if PIVX is started while in a temporary directory.\n",
+            gArgs.GetArg("-datadir", ""), fs::current_path().string());
+    }
+
     InitSignatureCache();
 
     LogPrintf("Using %u threads for script verification\n", nScriptCheckThreads);

src/net.h

@@ -631,12 +631,10 @@ class CNode
 
     CNode(NodeId id, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const std::string& addrNameIn = "", bool fInboundIn = false);
     ~CNode();
+    CNode(const CNode&) = delete;
+    CNode& operator=(const CNode&) = delete;
 
 private:
-    CNode(const CNode&);
-    void operator=(const CNode&);
-
-
     const uint64_t nLocalHostNonce;
     // Services offered to this peer
     const ServiceFlags nLocalServices;

src/pivx-cli.cpp

@@ -43,7 +43,7 @@ std::string HelpMessageCli()
     strUsage += HelpMessageOpt("-rpcuser=<user>", _("Username for JSON-RPC connections"));
     strUsage += HelpMessageOpt("-rpcpassword=<pw>", _("Password for JSON-RPC connections"));
     strUsage += HelpMessageOpt("-rpcclienttimeout=<n>", strprintf(_("Timeout in seconds during HTTP requests, or 0 for no timeout. (default: %d)"), DEFAULT_HTTP_CLIENT_TIMEOUT));
-    strUsage += HelpMessageOpt("-rpcwallet=<walletname>", _("Send RPC for non-default wallet on RPC server (argument is wallet filename in pivxd directory, required if pivxd/-Qt runs with multiple wallets)"));
+    strUsage += HelpMessageOpt("-rpcwallet=<walletname>", _("Send RPC for non-default wallet on RPC server (needs to exactly match corresponding -wallet option passed to pivxd)"));
 
     return strUsage;
 }
@@ -193,8 +193,8 @@ UniValue CallRPC(const std::string& strMethod, const UniValue& params)
 
     // check if we should use a special wallet endpoint
     std::string endpoint = "/";
-    std::string walletName = gArgs.GetArg("-rpcwallet", "");
-    if (!walletName.empty()) {
+    if (!gArgs.GetArgs("-rpcwallet").empty()) {
+        std::string walletName = gArgs.GetArg("-rpcwallet", "");
         char* encodedURI = evhttp_uriencode(walletName.c_str(), walletName.size(), false);
         if (encodedURI) {
             endpoint = "/wallet/"+ std::string(encodedURI);

src/pivxd.cpp

@@ -11,7 +11,6 @@
 #include "init.h"
 #include "masternodeconfig.h"
 #include "noui.h"
-#include "rpc/server.h"
 #include "util/system.h"
 
 #include <stdio.h>