From 8f1bcf8b7b6e47c05f2e43dd98ec3505b888d8b3 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 15 Jun 2020 14:37:29 -0400 Subject: [PATCH 1/3] walletdb: Combine VerifyDatabaseFile and VerifyEnvironment Combine these two functions into a single Verify function that is a member of WalletDatabase. Additionally, these are no longer static. --- src/wallet/bdb.cpp | 20 +++++--------------- src/wallet/bdb.h | 7 +++---- src/wallet/salvage.cpp | 5 +++++ src/wallet/wallet.cpp | 6 +----- src/wallet/walletdb.cpp | 10 ---------- src/wallet/wallettool.cpp | 7 +------ test/functional/tool_wallet.py | 3 +-- 7 files changed, 16 insertions(+), 42 deletions(-) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 125bf004e44..b09337b5a69 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -292,11 +292,10 @@ BerkeleyBatch::SafeDbt::operator Dbt*() return &m_dbt; } -bool BerkeleyBatch::VerifyEnvironment(const fs::path& file_path, bilingual_str& errorStr) +bool BerkeleyDatabase::Verify(bilingual_str& errorStr) { - std::string walletFile; - std::shared_ptr env = GetWalletEnv(file_path, walletFile); fs::path walletDir = env->Directory(); + fs::path file_path = walletDir / strFile; LogPrintf("Using BerkeleyDB version %s\n", BerkeleyDatabaseVersion()); LogPrintf("Using wallet %s\n", file_path.string()); @@ -306,19 +305,10 @@ bool BerkeleyBatch::VerifyEnvironment(const fs::path& file_path, bilingual_str& return false; } - return true; -} - -bool BerkeleyBatch::VerifyDatabaseFile(const fs::path& file_path, bilingual_str& errorStr) -{ - std::string walletFile; - std::shared_ptr env = GetWalletEnv(file_path, walletFile); - fs::path walletDir = env->Directory(); - - if (fs::exists(walletDir / walletFile)) + if (fs::exists(file_path)) { - if (!env->Verify(walletFile)) { - errorStr = strprintf(_("%s corrupt. Try using the wallet tool bitcoin-wallet to salvage or restoring a backup."), walletFile); + if (!env->Verify(strFile)) { + errorStr = strprintf(_("%s corrupt. Try using the wallet tool bitcoin-wallet to salvage or restoring a backup."), file_path); return false; } } diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index c121bb42282..f8ecdd75d40 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -141,6 +141,9 @@ public: unsigned int nLastFlushed; int64_t nLastWalletUpdate; + /** Verifies the environment and database file */ + bool Verify(bilingual_str& error); + /** * Pointer to shared database environment. * @@ -215,10 +218,6 @@ public: /* flush the wallet passively (TRY_LOCK) ideal to be called periodically */ static bool PeriodicFlush(BerkeleyDatabase& database); - /* verifies the database environment */ - static bool VerifyEnvironment(const fs::path& file_path, bilingual_str& errorStr); - /* verifies the database file */ - static bool VerifyDatabaseFile(const fs::path& file_path, bilingual_str& errorStr); template bool Read(const K& key, T& value) diff --git a/src/wallet/salvage.cpp b/src/wallet/salvage.cpp index d42950ee42a..e6e62332c00 100644 --- a/src/wallet/salvage.cpp +++ b/src/wallet/salvage.cpp @@ -20,6 +20,11 @@ bool RecoverDatabaseFile(const fs::path& file_path) std::string filename; std::shared_ptr env = GetWalletEnv(file_path, filename); + if (!env->Open(true /* retry */)) { + tfm::format(std::cerr, "Error initializing wallet database environment %s!", env->Directory()); + return false; + } + // Recovery procedure: // move wallet file to walletfilename.timestamp.bak // Call Salvage with fAggressive=true to diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4037e23b696..bbe9277e741 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3708,15 +3708,11 @@ bool CWallet::Verify(interfaces::Chain& chain, const WalletLocation& location, b std::unique_ptr database = CreateWalletDatabase(wallet_path); try { - if (!WalletBatch::VerifyEnvironment(wallet_path, error_string)) { - return false; - } + return database->Verify(error_string); } catch (const fs::filesystem_error& e) { error_string = Untranslated(strprintf("Error loading wallet %s. %s", location.GetName(), fsbridge::get_filesystem_error_message(e))); return false; } - - return WalletBatch::VerifyDatabaseFile(wallet_path, error_string); } std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, const WalletLocation& location, bilingual_str& error, std::vector& warnings, uint64_t wallet_creation_flags) diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 6f383980764..1c7649de670 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -973,16 +973,6 @@ void MaybeCompactWalletDB() fOneThread = false; } -bool WalletBatch::VerifyEnvironment(const fs::path& wallet_path, bilingual_str& errorStr) -{ - return BerkeleyBatch::VerifyEnvironment(wallet_path, errorStr); -} - -bool WalletBatch::VerifyDatabaseFile(const fs::path& wallet_path, bilingual_str& errorStr) -{ - return BerkeleyBatch::VerifyDatabaseFile(wallet_path, errorStr); -} - bool WalletBatch::WriteDestData(const std::string &address, const std::string &key, const std::string &value) { return WriteIC(std::make_pair(DBKeys::DESTDATA, std::make_pair(address, key)), value); diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index 77ed6beb5dd..8a45d81456f 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -112,7 +112,7 @@ static bool SalvageWallet(const fs::path& path) // Initialize the environment before recovery bilingual_str error_string; try { - WalletBatch::VerifyEnvironment(path, error_string); + database->Verify(error_string); } catch (const fs::filesystem_error& e) { error_string = Untranslated(strprintf("Error loading wallet. %s", fsbridge::get_filesystem_error_message(e))); } @@ -140,11 +140,6 @@ bool ExecuteWalletToolFunc(const std::string& command, const std::string& name) tfm::format(std::cerr, "Error: no wallet file at %s\n", name); return false; } - bilingual_str error; - if (!WalletBatch::VerifyEnvironment(path, error)) { - tfm::format(std::cerr, "%s\nError loading %s. Is wallet being used by other process?\n", error.original, name); - return false; - } if (command == "info") { std::shared_ptr wallet_instance = LoadWallet(name, path); diff --git a/test/functional/tool_wallet.py b/test/functional/tool_wallet.py index 524e1593bae..18f0beb5981 100755 --- a/test/functional/tool_wallet.py +++ b/test/functional/tool_wallet.py @@ -71,8 +71,7 @@ class ToolWalletTest(BitcoinTestFramework): self.assert_raises_tool_error('Error: two methods provided (info and create). Only one method should be provided.', 'info', 'create') self.assert_raises_tool_error('Error parsing command line arguments: Invalid parameter -foo', '-foo') self.assert_raises_tool_error( - 'Error initializing wallet database environment "{}"!\nError loading wallet.dat. Is wallet being used by other process?' - .format(os.path.join(self.nodes[0].datadir, self.chain, 'wallets')), + 'Error loading wallet.dat. Is wallet being used by another process?', '-wallet=wallet.dat', 'info', ) From 91d109156d63ff81cda534bd7bec8369af0027dd Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 15 Jun 2020 14:39:26 -0400 Subject: [PATCH 2/3] walletdb: Move PeriodicFlush into WalletDatabase Make PeriodicFlush a non-static member of WalletDatabase instead of WalletBatch. --- src/wallet/bdb.cpp | 6 ++---- src/wallet/bdb.h | 7 +++---- src/wallet/walletdb.cpp | 2 +- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index b09337b5a69..4856f4bff82 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -614,14 +614,12 @@ void BerkeleyEnvironment::Flush(bool fShutdown) } } -bool BerkeleyBatch::PeriodicFlush(BerkeleyDatabase& database) +bool BerkeleyDatabase::PeriodicFlush() { - if (database.IsDummy()) { + if (IsDummy()) { return true; } bool ret = false; - BerkeleyEnvironment *env = database.env.get(); - const std::string& strFile = database.strFile; TRY_LOCK(cs_db, lockDb); if (lockDb) { diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index f8ecdd75d40..d15efc810fa 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -131,6 +131,9 @@ public: /** Make sure all changes are flushed to disk. */ void Flush(bool shutdown); + /* flush the wallet passively (TRY_LOCK) + ideal to be called periodically */ + bool PeriodicFlush(); void IncrementUpdateCounter(); @@ -215,10 +218,6 @@ public: void Flush(); void Close(); - /* flush the wallet passively (TRY_LOCK) - ideal to be called periodically */ - static bool PeriodicFlush(BerkeleyDatabase& database); - template bool Read(const K& key, T& value) { diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 1c7649de670..0879f2d66eb 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -964,7 +964,7 @@ void MaybeCompactWalletDB() } if (dbh.nLastFlushed != nUpdateCounter && GetTime() - dbh.nLastWalletUpdate >= 2) { - if (BerkeleyBatch::PeriodicFlush(dbh)) { + if (dbh.PeriodicFlush()) { dbh.nLastFlushed = nUpdateCounter; } } From d8e9ca66d119d80acfb2bb3c8940c386ce0fc226 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 15 Jun 2020 15:31:02 -0400 Subject: [PATCH 3/3] walletdb: Move Rewrite into BerkeleyDatabase Make Rewrite actually a member of BerkeleyDatabase instead of a static function in BerkeleyBatch --- src/wallet/bdb.cpp | 13 +++---------- src/wallet/bdb.h | 2 -- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 4856f4bff82..bfd2b9b45ea 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -484,13 +484,11 @@ void BerkeleyEnvironment::ReloadDbEnv() Open(true); } -bool BerkeleyBatch::Rewrite(BerkeleyDatabase& database, const char* pszSkip) +bool BerkeleyDatabase::Rewrite(const char* pszSkip) { - if (database.IsDummy()) { + if (IsDummy()) { return true; } - BerkeleyEnvironment *env = database.env.get(); - const std::string& strFile = database.strFile; while (true) { { LOCK(cs_db); @@ -504,7 +502,7 @@ bool BerkeleyBatch::Rewrite(BerkeleyDatabase& database, const char* pszSkip) LogPrintf("BerkeleyBatch::Rewrite: Rewriting %s...\n", strFile); std::string strFileRes = strFile + ".rewrite"; { // surround usage of db with extra {} - BerkeleyBatch db(database, "r"); + BerkeleyBatch db(*this, "r"); std::unique_ptr pdbCopy = MakeUnique(env->dbenv.get(), 0); int ret = pdbCopy->open(nullptr, // Txn pointer @@ -654,11 +652,6 @@ bool BerkeleyDatabase::PeriodicFlush() return ret; } -bool BerkeleyDatabase::Rewrite(const char* pszSkip) -{ - return BerkeleyBatch::Rewrite(*this, pszSkip); -} - bool BerkeleyDatabase::Backup(const std::string& strDest) const { if (IsDummy()) { diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index d15efc810fa..73169abde4d 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -287,8 +287,6 @@ public: bool TxnBegin(); bool TxnCommit(); bool TxnAbort(); - - bool static Rewrite(BerkeleyDatabase& database, const char* pszSkip = nullptr); }; std::string BerkeleyDatabaseVersion();