Merge #11904: Add a lock to the wallet directory

2f3bd47 Abstract directory locking into util.cpp (MeshCollider)
5260a4a Make .walletlock distinct from .lock (MeshCollider)
64226de Generalise walletdir lock error message for correctness (MeshCollider)
c9ed4bd Add a test for wallet directory locking (MeshCollider)
e60cb99 Add a lock to the wallet directory (MeshCollider)

Pull request description:

  Fixes https://github.com/bitcoin/bitcoin/issues/11888, needs a 0.16 milestone

  Also adds a test that the lock works.

  https://github.com/bitcoin/bitcoin/pull/11687 will probably rework this to a per-wallet lock instead of just the walletdir, but this fixes the current issue

Tree-SHA512: 98e52d67f820e3b8f919cf361ffbb7d928f1bd67603e0ed26c5076ea02d9b3a90c3535ddf7329f3b88171396fa28dd3c87adab3577a8a217bd1e4247bda99138

src/init.cpp

@@ -1143,23 +1143,10 @@ bool AppInitParameterInteraction()
 
 static bool LockDataDirectory(bool probeOnly)
 {
-    std::string strDataDir = GetDataDir().string();
-
     // Make sure only a single Bitcoin process is using the data directory.
-    fs::path pathLockFile = GetDataDir() / ".lock";
+    fs::path datadir = GetDataDir();
-    FILE* file = fsbridge::fopen(pathLockFile, "a"); // empty lock file; created if it doesn't exist.
+    if (!LockDirectory(datadir, ".lock", probeOnly)) {
-    if (file) fclose(file);
+        return InitError(strprintf(_("Cannot obtain a lock on data directory %s. %s is probably already running."), datadir.string(), _(PACKAGE_NAME)));
-
-    try {
-        static boost::interprocess::file_lock lock(pathLockFile.string().c_str());
-        if (!lock.try_lock()) {
-            return InitError(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 InitError(strprintf(_("Cannot obtain a lock on data directory %s. %s is probably already running.") + " %s.", strDataDir, _(PACKAGE_NAME), e.what()));
     }
     return true;
 }

src/util.cpp

@@ -72,6 +72,7 @@
 
 #include <boost/algorithm/string/case_conv.hpp> // for to_lower()
 #include <boost/algorithm/string/predicate.hpp> // for startswith() and endswith()
+#include <boost/interprocess/sync/file_lock.hpp>
 #include <boost/program_options/detail/config_file.hpp>
 #include <boost/thread.hpp>
 #include <openssl/crypto.h>
@@ -375,6 +376,27 @@ int LogPrintStr(const std::string &str)
     return ret;
 }
 
+bool LockDirectory(const fs::path& directory, const std::string lockfile_name, bool probe_only)
+{
+    fs::path pathLockFile = directory / lockfile_name;
+    FILE* file = fsbridge::fopen(pathLockFile, "a"); // empty lock file; created if it doesn't exist.
+    if (file) fclose(file);
+
+    try {
+        static std::map<std::string, boost::interprocess::file_lock> locks;
+        boost::interprocess::file_lock& lock = locks.emplace(pathLockFile.string(), pathLockFile.string().c_str()).first->second;
+        if (!lock.try_lock()) {
+            return false;
+        }
+        if (probe_only) {
+            lock.unlock();
+        }
+    } catch (const boost::interprocess::interprocess_exception& e) {
+        return error("Error while attempting to lock directory %s: %s", directory.string(), e.what());
+    }
+    return true;
+}
+
 /** Interpret string as boolean, for argument parsing */
 static bool InterpretBool(const std::string& strValue)
 {

src/util.h

@@ -173,6 +173,7 @@ bool TruncateFile(FILE *file, unsigned int length);
 int RaiseFileDescriptorLimit(int nMinFD);
 void AllocateFileRange(FILE *file, unsigned int offset, unsigned int length);
 bool RenameOver(fs::path src, fs::path dest);
+bool LockDirectory(const fs::path& directory, const std::string lockfile_name, bool probe_only=false);
 bool TryCreateDirectories(const fs::path& p);
 fs::path GetDefaultDataDir();
 const fs::path &GetDataDir(bool fNetSpecific = true);

src/wallet/db.cpp

@@ -95,7 +95,7 @@ void CDBEnv::Close()
     EnvShutdown();
 }
 
-bool CDBEnv::Open(const fs::path& pathIn)
+bool CDBEnv::Open(const fs::path& pathIn, bool retry)
 {
     if (fDbEnvInit)
         return true;
@@ -103,6 +103,11 @@ bool CDBEnv::Open(const fs::path& pathIn)
     boost::this_thread::interruption_point();
 
     strPath = pathIn.string();
+    if (!LockDirectory(pathIn, ".walletlock")) {
+        LogPrintf("Cannot obtain a lock on wallet directory %s. Another instance of bitcoin may be using it.\n", strPath);
+        return false;
+    }
+
     fs::path pathLogDir = pathIn / "database";
     TryCreateDirectories(pathLogDir);
     fs::path pathErrorFile = pathIn / "db.log";
@@ -134,7 +139,24 @@ bool CDBEnv::Open(const fs::path& pathIn)
                          S_IRUSR | S_IWUSR);
     if (ret != 0) {
         dbenv->close(0);
-        return error("CDBEnv::Open: Error %d opening database environment: %s\n", ret, DbEnv::strerror(ret));
+        LogPrintf("CDBEnv::Open: Error %d opening database environment: %s\n", ret, DbEnv::strerror(ret));
+        if (retry) {
+            // try moving the database env out of the way
+            fs::path pathDatabaseBak = pathIn / strprintf("database.%d.bak", GetTime());
+            try {
+                fs::rename(pathLogDir, pathDatabaseBak);
+                LogPrintf("Moved old %s to %s. Retrying.\n", pathLogDir.string(), pathDatabaseBak.string());
+            } catch (const fs::filesystem_error&) {
+                // failure is ok (well, not really, but it's not worse than what we started with)
+            }
+            // try opening it again one more time
+            if (!Open(pathIn, false)) {
+                // if it still fails, it probably means we can't even create the database env
+                return false;
+            }
+        } else {
+            return false;
+        }
     }
 
     fDbEnvInit = true;
@@ -269,25 +291,11 @@ bool CDB::VerifyEnvironment(const std::string& walletFile, const fs::path& walle
         return false;
     }
 
-    if (!bitdb.Open(walletDir))
+    if (!bitdb.Open(walletDir, true)) {
-    {
-        // try moving the database env out of the way
-        fs::path pathDatabase = walletDir / "database";
-        fs::path pathDatabaseBak = walletDir / strprintf("database.%d.bak", GetTime());
-        try {
-            fs::rename(pathDatabase, pathDatabaseBak);
-            LogPrintf("Moved old %s to %s. Retrying.\n", pathDatabase.string(), pathDatabaseBak.string());
-        } catch (const fs::filesystem_error&) {
-            // failure is ok (well, not really, but it's not worse than what we started with)
-        }
-
-        // try again
-        if (!bitdb.Open(walletDir)) {
-            // if it still fails, it probably means we can't even create the database env
         errorStr = strprintf(_("Error initializing wallet database environment %s!"), walletDir);
         return false;
     }
-    }
+
     return true;
 }
 

src/wallet/db.h

@@ -68,7 +68,7 @@ public:
     typedef std::pair<std::vector<unsigned char>, std::vector<unsigned char> > KeyValPair;
     bool Salvage(const std::string& strFile, bool fAggressive, std::vector<KeyValPair>& vResult);
 
-    bool Open(const fs::path& path);
+    bool Open(const fs::path& path, bool retry = 0);
     void Close();
     void Flush(bool fShutdown);
     void CheckpointLSN(const std::string& strFile);

test/functional/multiwallet.py

@@ -15,8 +15,8 @@ from test_framework.util import assert_equal, assert_raises_rpc_error
 class MultiWalletTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
-        self.num_nodes = 1
+        self.num_nodes = 2
-        self.extra_args = [['-wallet=w1', '-wallet=w2', '-wallet=w3', '-wallet=w']]
+        self.extra_args = [['-wallet=w1', '-wallet=w2', '-wallet=w3', '-wallet=w'], []]
         self.supports_cli = True
 
     def run_test(self):
@@ -28,7 +28,7 @@ class MultiWalletTest(BitcoinTestFramework):
 
         assert_equal(set(node.listwallets()), {"w1", "w2", "w3", "w"})
 
-        self.stop_node(0)
+        self.stop_nodes()
 
         # 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.')
@@ -59,19 +59,21 @@ class MultiWalletTest(BitcoinTestFramework):
         assert_equal(set(node.listwallets()), {"w4", "w5"})
         w5 = wallet("w5")
         w5.generate(1)
-        self.stop_node(0)
 
         # now if wallets/ exists again, but the rootdir is specified as the walletdir, w4 and w5 should still be loaded
         os.rename(wallet_dir2, wallet_dir())
-        self.start_node(0, ['-wallet=w4', '-wallet=w5', '-walletdir=' + data_dir()])
+        self.restart_node(0, ['-wallet=w4', '-wallet=w5', '-walletdir=' + data_dir()])
         assert_equal(set(node.listwallets()), {"w4", "w5"})
         w5 = wallet("w5")
         w5_info = w5.getwalletinfo()
         assert_equal(w5_info['immature_balance'], 50)
 
-        self.stop_node(0)
+        competing_wallet_dir = os.path.join(self.options.tmpdir, 'competing_walletdir')
+        os.mkdir(competing_wallet_dir)
+        self.restart_node(0, ['-walletdir='+competing_wallet_dir])
+        self.assert_start_raises_init_error(1, ['-walletdir='+competing_wallet_dir], 'Error initializing wallet database environment')
 
-        self.start_node(0, self.extra_args[0])
+        self.restart_node(0, self.extra_args[0])
 
         w1 = wallet("w1")
         w2 = wallet("w2")