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
This commit is contained in:
Wladimir J. van der Laan 2018-01-16 10:54:13 +01:00 committed by Pasta
parent 9d3c7c3ca6
commit 9bedb900a9
6 changed files with 64 additions and 44 deletions

View File

@ -1484,23 +1484,10 @@ bool AppInitParameterInteraction()
static bool LockDataDirectory(bool probeOnly)
{
std::string strDataDir = GetDataDir().string();
// Make sure only a single Dash Core 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());
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()));
fs::path datadir = GetDataDir();
if (!LockDirectory(datadir, ".lock", probeOnly)) {
return InitError(strprintf(_("Cannot obtain a lock on data directory %s. %s is probably already running."), datadir.string(), _(PACKAGE_NAME)));
}
return true;
}

View File

@ -79,6 +79,7 @@
#include <boost/algorithm/string/predicate.hpp> // for startswith() and endswith()
#include <boost/algorithm/string/split.hpp>
#include <boost/algorithm/string/classification.hpp>
#include <boost/interprocess/sync/file_lock.hpp>
#include <boost/program_options/detail/config_file.hpp>
#include <boost/program_options/parsers.hpp>
#include <boost/thread.hpp>
@ -468,6 +469,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)
{

View File

@ -225,6 +225,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);

View File

@ -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))
{
// 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;
}
if (!bitdb.Open(walletDir, true)) {
errorStr = strprintf(_("Error initializing wallet database environment %s!"), walletDir);
return false;
}
return true;
}

View File

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

View File

@ -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.extra_args = [['-wallet=w1', '-wallet=w2', '-wallet=w3', '-wallet=w']]
self.num_nodes = 2
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()
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())
@ -63,19 +63,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'], 500)
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")