diff --git a/contrib/teamcity/build-configurations.sh b/contrib/teamcity/build-configurations.sh --- a/contrib/teamcity/build-configurations.sh +++ b/contrib/teamcity/build-configurations.sh @@ -72,8 +72,6 @@ ) CMAKE_FLAGS="${CMAKE_FLAGS[*]}" "${CI_SCRIPTS_DIR}"/build_cmake.sh ninja check - # FIXME Remove when wallet_multiwallet works with asan after backporting at least the following PRs from Core and their dependencies: 13161, 12493, 14320, 14552, 14760, 11911. - TEST_RUNNER_FLAGS="${TEST_RUNNER_FLAGS} --exclude=wallet_multiwallet" ./test/functional/test_runner.py ${TEST_RUNNER_FLAGS} ;; @@ -102,8 +100,6 @@ ) CMAKE_FLAGS="${CMAKE_FLAGS[*]}" "${CI_SCRIPTS_DIR}"/build_cmake.sh ninja check - # FIXME Remove when wallet_multiwallet works with tsan after backporting at least the following PRs from Core and their dependencies: 13161, 12493, 14320, 14552, 14760, 11911. - TEST_RUNNER_FLAGS="${TEST_RUNNER_FLAGS} --exclude=wallet_multiwallet" ./test/functional/test_runner.py ${TEST_RUNNER_FLAGS} ;; diff --git a/src/Makefile.test.include b/src/Makefile.test.include --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -137,6 +137,7 @@ if ENABLE_WALLET BITCOIN_TESTS += \ wallet/test/accounting_tests.cpp \ + wallet/test/db_tests.cpp \ wallet/test/psbt_wallet_tests.cpp \ wallet/test/wallet_tests.cpp \ wallet/test/walletdb_tests.cpp \ diff --git a/src/test/CMakeLists.txt b/src/test/CMakeLists.txt --- a/src/test/CMakeLists.txt +++ b/src/test/CMakeLists.txt @@ -166,6 +166,7 @@ target_sources(test_bitcoin PRIVATE ../wallet/test/accounting_tests.cpp + ../wallet/test/db_tests.cpp ../wallet/test/coinselector_tests.cpp ../wallet/test/psbt_wallet_tests.cpp ../wallet/test/wallet_test_fixture.cpp diff --git a/src/wallet/db.h b/src/wallet/db.h --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -50,6 +50,7 @@ std::condition_variable_any m_db_in_use; BerkeleyEnvironment(const fs::path &env_directory); + BerkeleyEnvironment(); ~BerkeleyEnvironment(); void Reset(); @@ -106,8 +107,8 @@ bool IsWalletLoaded(const fs::path &wallet_path); /** Get BerkeleyEnvironment and database filename given a wallet path. */ -BerkeleyEnvironment *GetWalletEnv(const fs::path &wallet_path, - std::string &database_filename); +std::shared_ptr +GetWalletEnv(const fs::path &wallet_path, std::string &database_filename); /** * An instance of this class represents one database. @@ -123,17 +124,14 @@ nLastWalletUpdate(0), env(nullptr) {} /** Create DB handle to real database */ - BerkeleyDatabase(const fs::path &wallet_path, bool mock = false) + BerkeleyDatabase(std::shared_ptr env, + std::string filename) : nUpdateCounter(0), nLastSeen(0), nLastFlushed(0), - nLastWalletUpdate(0) { - env = GetWalletEnv(wallet_path, strFile); - auto inserted = env->m_databases.emplace(strFile, std::ref(*this)); + nLastWalletUpdate(0), env(std::move(env)), + strFile(std::move(filename)) { + auto inserted = + this->env->m_databases.emplace(strFile, std::ref(*this)); assert(inserted.second); - if (mock) { - env->Close(); - env->Reset(); - env->MakeMock(); - } } ~BerkeleyDatabase() { @@ -145,7 +143,9 @@ /** Return object for accessing database at specified path. */ static std::unique_ptr Create(const fs::path &path) { - return std::make_unique(path); + std::string filename; + return std::make_unique(GetWalletEnv(path, filename), + std::move(filename)); } /** @@ -160,7 +160,8 @@ * Return object for accessing temporary in-memory database. */ static std::unique_ptr CreateMock() { - return std::make_unique("", true /* mock */); + return std::make_unique( + std::make_shared(), ""); } /** @@ -188,6 +189,17 @@ unsigned int nLastFlushed; int64_t nLastWalletUpdate; + /** + * Pointer to shared database environment. + * + * Normally there is only one BerkeleyDatabase object per + * BerkeleyEnvivonment, but in the special, backwards compatible case where + * multiple wallet BDB data files are loaded from the same directory, this + * will point to a shared instance that gets freed when the last data file + * is closed. + */ + std::shared_ptr env; + /** * Database pointer. This is initialized lazily and reset during flushes, * so it can be null. @@ -195,8 +207,6 @@ std::unique_ptr m_db; private: - /** BerkeleyDB specific */ - BerkeleyEnvironment *env; std::string strFile; /** diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -59,8 +59,9 @@ CCriticalSection cs_db; -//!< Map from directory name to open db environment. -std::map g_dbenvs GUARDED_BY(cs_db); +//!< Map from directory name to db environment. +std::map> + g_dbenvs GUARDED_BY(cs_db); } // namespace bool WalletDatabaseFileId::operator==(const WalletDatabaseFileId &rhs) const { @@ -95,23 +96,36 @@ return false; } - return env->second.IsDatabaseLoaded(database_filename); + auto database = env->second.lock(); + return database && database->IsDatabaseLoaded(database_filename); } -BerkeleyEnvironment *GetWalletEnv(const fs::path &wallet_path, - std::string &database_filename) { +/** + * @param[in] wallet_path Path to wallet directory. Or (for backwards + * compatibility only) a path to a berkeley btree data file inside a wallet + * directory. + * @param[out] database_filename Filename of berkeley btree data file inside the + * wallet directory. + * @return A shared pointer to the BerkeleyEnvironment object for the wallet + * directory, never empty because ~BerkeleyEnvironment erases the weak pointer + * from the g_dbenvs map. + * @post A new BerkeleyEnvironment weak pointer is inserted into g_dbenvs if the + * directory path key was not already in the map. + */ +std::shared_ptr +GetWalletEnv(const fs::path &wallet_path, std::string &database_filename) { fs::path env_directory; SplitWalletPath(wallet_path, env_directory, database_filename); LOCK(cs_db); - // Note: An ununsed temporary BerkeleyEnvironment object may be created - // inside the emplace function if the key already exists. This is a little - // inefficient, but not a big concern since the map will be changed in the - // future to hold pointers instead of objects, anyway. - return &g_dbenvs - .emplace(std::piecewise_construct, - std::forward_as_tuple(env_directory.string()), - std::forward_as_tuple(env_directory)) - .first->second; + auto inserted = g_dbenvs.emplace(env_directory.string(), + std::weak_ptr()); + if (inserted.second) { + auto env = + std::make_shared(env_directory.string()); + inserted.first->second = env; + return env; + } + return inserted.first->second.lock(); } // @@ -158,6 +172,8 @@ } BerkeleyEnvironment::~BerkeleyEnvironment() { + LOCK(cs_db); + g_dbenvs.erase(strPath); Close(); } @@ -244,11 +260,10 @@ return true; } -void BerkeleyEnvironment::MakeMock() { - if (fDbEnvInit) { - throw std::runtime_error( - "BerkeleyEnvironment::MakeMock: Already initialized"); - } +//! Construct an in-memory mock Berkeley environment for testing and as a +//! place-holder for g_dbenvs emplace +BerkeleyEnvironment::BerkeleyEnvironment() { + Reset(); boost::this_thread::interruption_point(); @@ -304,7 +319,8 @@ CDataStream ssValue), std::string &newFilename) { std::string filename; - BerkeleyEnvironment *env = GetWalletEnv(file_path, filename); + std::shared_ptr env = + GetWalletEnv(file_path, filename); // Recovery procedure: // Move wallet file to walletfilename.timestamp.bak @@ -369,7 +385,8 @@ bool BerkeleyBatch::VerifyEnvironment(const fs::path &file_path, std::string &errorStr) { std::string walletFile; - BerkeleyEnvironment *env = GetWalletEnv(file_path, walletFile); + std::shared_ptr env = + GetWalletEnv(file_path, walletFile); fs::path walletDir = env->Directory(); LogPrintf("Using BerkeleyDB version %s\n", DbEnv::version(0, 0, 0)); @@ -395,7 +412,8 @@ const fs::path &file_path, std::string &warningStr, std::string &errorStr, BerkeleyEnvironment::recoverFunc_type recoverFunc) { std::string walletFile; - BerkeleyEnvironment *env = GetWalletEnv(file_path, walletFile); + std::shared_ptr env = + GetWalletEnv(file_path, walletFile); fs::path walletDir = env->Directory(); if (fs::exists(walletDir / walletFile)) { @@ -507,7 +525,7 @@ : pdb(nullptr), activeTxn(nullptr) { fReadOnly = (!strchr(pszMode, '+') && !strchr(pszMode, 'w')); fFlushOnClose = fFlushOnCloseIn; - env = database.env; + env = database.env.get(); if (database.IsDummy()) { return; } @@ -575,8 +593,8 @@ // versions of BDB have an set_lk_exclusive method for this // purpose, but the older version we use does not.) for (const auto &dbenv : g_dbenvs) { - CheckUniqueFileid(dbenv.second, strFilename, *pdb_temp, - this->env->m_fileids[strFilename]); + CheckUniqueFileid(*dbenv.second.lock().get(), strFilename, + *pdb_temp, this->env->m_fileids[strFilename]); } pdb = pdb_temp.release(); @@ -680,7 +698,7 @@ if (database.IsDummy()) { return true; } - BerkeleyEnvironment *env = database.env; + BerkeleyEnvironment *env = database.env.get(); const std::string &strFile = database.strFile; while (true) { { @@ -842,7 +860,7 @@ return true; } bool ret = false; - BerkeleyEnvironment *env = database.env; + BerkeleyEnvironment *env = database.env.get(); const std::string &strFile = database.strFile; TRY_LOCK(cs_db, lockDb); if (lockDb) { diff --git a/src/wallet/test/db_tests.cpp b/src/wallet/test/db_tests.cpp new file mode 100644 --- /dev/null +++ b/src/wallet/test/db_tests.cpp @@ -0,0 +1,77 @@ +// Copyright (c) 2018 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include + +#include + +#include + +#include + +#include + +BOOST_FIXTURE_TEST_SUITE(db_tests, BasicTestingSetup) + +BOOST_AUTO_TEST_CASE(getwalletenv_file) { + std::string test_name = "test_name.dat"; + fs::path datadir = SetDataDir("tempdir"); + fs::path file_path = datadir / test_name; + std::ofstream f(file_path.BOOST_FILESYSTEM_C_STR); + f.close(); + + std::string filename; + std::shared_ptr env = + GetWalletEnv(file_path, filename); + BOOST_CHECK(filename == test_name); + BOOST_CHECK(env->Directory() == datadir); +} + +BOOST_AUTO_TEST_CASE(getwalletenv_directory) { + std::string expected_name = "wallet.dat"; + fs::path datadir = SetDataDir("tempdir"); + + std::string filename; + std::shared_ptr env = GetWalletEnv(datadir, filename); + BOOST_CHECK(filename == expected_name); + BOOST_CHECK(env->Directory() == datadir); +} + +BOOST_AUTO_TEST_CASE(getwalletenv_g_dbenvs_multiple) { + fs::path datadir = SetDataDir("tempdir"); + fs::path datadir_2 = SetDataDir("tempdir_2"); + std::string filename; + + std::shared_ptr env_1 = + GetWalletEnv(datadir, filename); + std::shared_ptr env_2 = + GetWalletEnv(datadir, filename); + std::shared_ptr env_3 = + GetWalletEnv(datadir_2, filename); + + BOOST_CHECK(env_1 == env_2); + BOOST_CHECK(env_2 != env_3); +} + +BOOST_AUTO_TEST_CASE(getwalletenv_g_dbenvs_free_instance) { + fs::path datadir = SetDataDir("tempdir"); + fs::path datadir_2 = SetDataDir("tempdir_2"); + std::string filename; + + std::shared_ptr env_1_a = + GetWalletEnv(datadir, filename); + std::shared_ptr env_2_a = + GetWalletEnv(datadir_2, filename); + env_1_a.reset(); + + std::shared_ptr env_1_b = + GetWalletEnv(datadir, filename); + std::shared_ptr env_2_b = + GetWalletEnv(datadir_2, filename); + + BOOST_CHECK(env_1_a != env_1_b); + BOOST_CHECK(env_2_a == env_2_b); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4332,6 +4332,11 @@ return false; } + // Keep same database environment instance across Verify/Recover calls + // below. + std::unique_ptr database = + WalletDatabase::Create(wallet_path); + try { if (!WalletBatch::VerifyEnvironment(wallet_path, error_string)) { return false;