refactor: Pass wallet database into CWallet::Create

ryanofsky · ryanofsky · commit 8b5e7297c02f · 2020-09-03T12:24:32.000-04:00
No changes in behavior
diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp
@@ -359,7 +359,6 @@ void BerkeleyDatabase::Open(const char* pszMode)
             if (ret != 0) {
                 throw std::runtime_error(strprintf("BerkeleyDatabase: Error %d, can't open database %s", ret, strFile));
             }
-            m_file_path = (env->Directory() / strFile).string();
 
             // Call CheckUniqueFileid on the containing BDB environment to
             // avoid BDB data consistency bugs that happen when different data
diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h
@@ -144,6 +144,9 @@ class BerkeleyDatabase : public WalletDatabase
     /** Verifies the environment and database file */
     bool Verify(bilingual_str& error);
 
+    /** Return path to main database filename */
+    std::string Filename() override { return (env->Directory() / strFile).string(); }
+
     /**
      * Pointer to shared database environment.
      *
diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp
@@ -23,11 +23,3 @@ void SplitWalletPath(const fs::path& wallet_path, fs::path& env_directory, std::
         database_filename = "wallet.dat";
     }
 }
-
-fs::path WalletDataFilePath(const fs::path& wallet_path)
-{
-    fs::path env_directory;
-    std::string database_filename;
-    SplitWalletPath(wallet_path, env_directory, database_filename);
-    return env_directory / database_filename;
-}
diff --git a/src/wallet/db.h b/src/wallet/db.h
@@ -18,8 +18,6 @@
 
 struct bilingual_str;
 
-/** Given a wallet directory path or legacy file path, return path to main data file in the wallet database. */
-fs::path WalletDataFilePath(const fs::path& wallet_path);
 void SplitWalletPath(const fs::path& wallet_path, fs::path& env_directory, std::string& database_filename);
 
 /** RAII class that provides access to a WalletDatabase */
@@ -142,13 +140,14 @@ class WalletDatabase
 
     virtual void ReloadDbEnv() = 0;
 
+    /** Return path to main database file for logs and error messages. */
+    virtual std::string Filename() = 0;
+
     std::atomic<unsigned int> nUpdateCounter;
     unsigned int nLastSeen;
     unsigned int nLastFlushed;
     int64_t nLastWalletUpdate;
 
-    std::string m_file_path;
-
     /** Make a DatabaseBatch connected to this database */
     virtual std::unique_ptr<DatabaseBatch> MakeBatch(const char* mode = "r+", bool flush_on_close = true) = 0;
 };
@@ -189,6 +188,7 @@ class DummyDatabase : public WalletDatabase
     bool PeriodicFlush() override { return true; }
     void IncrementUpdateCounter() override { ++nUpdateCounter; }
     void ReloadDbEnv() override {}
+    std::string Filename() override { return "dummy"; }
     std::unique_ptr<DatabaseBatch> MakeBatch(const char* mode = "r+", bool flush_on_close = true) override { return MakeUnique<DummyBatch>(); }
 };
 
diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp
@@ -68,10 +68,14 @@ bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wal
 bool LoadWallets(interfaces::Chain& chain, const std::vector<std::string>& wallet_files)
 {
     try {
-        for (const std::string& walletFile : wallet_files) {
+        for (const std::string& name : wallet_files) {
+            DatabaseOptions options;
+            DatabaseStatus status;
+            options.verify = false; // No need to verify, assuming verified earlier in VerifyWallets()
             bilingual_str error;
             std::vector<bilingual_str> warnings;
-            std::shared_ptr<CWallet> pwallet = CWallet::CreateWalletFromFile(chain, walletFile, error, warnings);
+            std::unique_ptr<WalletDatabase> database = MakeWalletDatabase(name, options, status, error);
+            std::shared_ptr<CWallet> pwallet = database ? CWallet::Create(chain, name, std::move(database), options.create_flags, error, warnings) : nullptr;
             if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n")));
             if (!pwallet) {
                 chain.initError(error);
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
@@ -37,9 +37,12 @@ BOOST_FIXTURE_TEST_SUITE(wallet_tests, WalletTestingSetup)
 
 static std::shared_ptr<CWallet> TestLoadWallet(interfaces::Chain& chain)
 {
+    DatabaseOptions options;
+    DatabaseStatus status;
     bilingual_str error;
     std::vector<bilingual_str> warnings;
-    auto wallet = CWallet::CreateWalletFromFile(chain, "", error, warnings);
+    auto database = MakeWalletDatabase("", options, status, error);
+    auto wallet = CWallet::Create(chain, "", std::move(database), options.create_flags, error, warnings);
     wallet->postInitProcess();
     return wallet;
 }
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -203,12 +203,13 @@ namespace {
 std::shared_ptr<CWallet> LoadWalletInternal(interfaces::Chain& chain, const std::string& name, Optional<bool> load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings)
 {
     try {
-        if (!MakeWalletDatabase(name, options, status, error)) {
+        std::unique_ptr<WalletDatabase> database = MakeWalletDatabase(name, options, status, error);
+        if (!database) {
             error = Untranslated("Wallet file verification failed.") + Untranslated(" ") + error;
             return nullptr;
         }
 
-        std::shared_ptr<CWallet> wallet = CWallet::CreateWalletFromFile(chain, name, error, warnings);
+        std::shared_ptr<CWallet> wallet = CWallet::Create(chain, name, std::move(database), options.create_flags, error, warnings);
         if (!wallet) {
             error = Untranslated("Wallet loading failed.") + Untranslated(" ") + error;
             return nullptr;
@@ -260,7 +261,8 @@ std::shared_ptr<CWallet> CreateWallet(interfaces::Chain& chain, const std::strin
     }
 
     // Wallet::Verify will check if we're trying to create a wallet with a duplicate name.
-    if (!MakeWalletDatabase(name, options, status, error)) {
+    std::unique_ptr<WalletDatabase> database = MakeWalletDatabase(name, options, status, error);
+    if (!database) {
         error = Untranslated("Wallet file verification failed.") + Untranslated(" ") + error;
         status = DatabaseStatus::FAILED_VERIFY;
         return nullptr;
@@ -274,7 +276,7 @@ std::shared_ptr<CWallet> CreateWallet(interfaces::Chain& chain, const std::strin
     }
 
     // Make the wallet
-    std::shared_ptr<CWallet> wallet = CWallet::CreateWalletFromFile(chain, name, error, warnings, wallet_creation_flags);
+    std::shared_ptr<CWallet> wallet = CWallet::Create(chain, name, std::move(database), wallet_creation_flags, error, warnings);
     if (!wallet) {
         error = Untranslated("Wallet creation failed.") + Untranslated(" ") + error;
         status = DatabaseStatus::FAILED_CREATE;
@@ -3803,18 +3805,17 @@ std::unique_ptr<WalletDatabase> MakeWalletDatabase(const std::string& name, cons
     return MakeDatabase(wallet_path, options, status, error_string);
 }
 
-std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain, const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings, uint64_t wallet_creation_flags)
+std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, const std::string& name, std::unique_ptr<WalletDatabase> database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector<bilingual_str>& warnings)
 {
-    fs::path path = fs::absolute(name, GetWalletDir());
-    const std::string walletFile = WalletDataFilePath(path).string();
+    const std::string& walletFile = database->Filename();
 
     chain.initMessage(_("Loading wallet...").translated);
 
     int64_t nStart = GetTimeMillis();
     bool fFirstRun = true;
     // TODO: Can't use std::make_shared because we need a custom deleter but
     // should be possible to use std::allocate_shared.
-    std::shared_ptr<CWallet> walletInstance(new CWallet(&chain, name, CreateWalletDatabase(path)), ReleaseWallet);
+    std::shared_ptr<CWallet> walletInstance(new CWallet(&chain, name, std::move(database)), ReleaseWallet);
     DBErrors nLoadWalletRet = walletInstance->LoadWallet(fFirstRun);
     if (nLoadWalletRet != DBErrors::LOAD_OK) {
         if (nLoadWalletRet == DBErrors::CORRUPT) {
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
@@ -1146,7 +1146,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     bool MarkReplaced(const uint256& originalHash, const uint256& newHash);
 
     /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */
-    static std::shared_ptr<CWallet> CreateWalletFromFile(interfaces::Chain& chain, const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings, uint64_t wallet_creation_flags = 0);
+    static std::shared_ptr<CWallet> Create(interfaces::Chain& chain, const std::string& name, std::unique_ptr<WalletDatabase> database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector<bilingual_str>& warnings);
 
     /**
      * Wallet post-init setup

Original file line number	Diff line number	Diff line change
`@@ -359,7 +359,6 @@ void BerkeleyDatabase::Open(const char* pszMode)`
`359`	`359`	`if (ret != 0) {`
`360`	`360`	`throw std::runtime_error(strprintf("BerkeleyDatabase: Error %d, can't open database %s", ret, strFile));`
`361`	`361`	`}`
`362`		`- m_file_path = (env->Directory() / strFile).string();`
`363`	`362`
`364`	`363`	`// Call CheckUniqueFileid on the containing BDB environment to`
`365`	`364`	`// avoid BDB data consistency bugs that happen when different data`
Original file line number	Diff line number	Diff line change
`@@ -144,6 +144,9 @@ class BerkeleyDatabase : public WalletDatabase`
`144`	`144`	`/** Verifies the environment and database file */`
`145`	`145`	`bool Verify(bilingual_str& error);`
`146`	`146`
	`147`	`+ /** Return path to main database filename */`
	`148`	`+ std::string Filename() override { return (env->Directory() / strFile).string(); }`
	`149`	`+`
`147`	`150`	`/**`
`148`	`151`	`* Pointer to shared database environment.`
`149`	`152`	`*`
Original file line number	Diff line number	Diff line change
`@@ -23,11 +23,3 @@ void SplitWalletPath(const fs::path& wallet_path, fs::path& env_directory, std::`
`23`	`23`	`database_filename = "wallet.dat";`
`24`	`24`	`}`
`25`	`25`	`}`
`26`		`-`
`27`		`-fs::path WalletDataFilePath(const fs::path& wallet_path)`
`28`		`-{`
`29`		`- fs::path env_directory;`
`30`		`- std::string database_filename;`
`31`		`- SplitWalletPath(wallet_path, env_directory, database_filename);`
`32`		`- return env_directory / database_filename;`
`33`		`-}`
Original file line number	Diff line number	Diff line change
`@@ -37,9 +37,12 @@ BOOST_FIXTURE_TEST_SUITE(wallet_tests, WalletTestingSetup)`
`37`	`37`
`38`	`38`	`static std::shared_ptr<CWallet> TestLoadWallet(interfaces::Chain& chain)`
`39`	`39`	`{`
	`40`	`+ DatabaseOptions options;`
	`41`	`+ DatabaseStatus status;`
`40`	`42`	`bilingual_str error;`
`41`	`43`	`std::vector<bilingual_str> warnings;`
`42`		`- auto wallet = CWallet::CreateWalletFromFile(chain, "", error, warnings);`
	`44`	`+ auto database = MakeWalletDatabase("", options, status, error);`
	`45`	`+ auto wallet = CWallet::Create(chain, "", std::move(database), options.create_flags, error, warnings);`
`43`	`46`	`wallet->postInitProcess();`
`44`	`47`	`return wallet;`
`45`	`48`	`}`