Merge #19619: Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp

7bf6dfb wallet: Remove path checking code from bitcoin-wallet tool (Russell Yanofsky)
77d5bb7 wallet: Remove path checking code from createwallet RPC (Russell Yanofsky)
a987438 wallet: Remove path checking code from loadwallet RPC (Russell Yanofsky)
8b5e729 refactor: Pass wallet database into CWallet::Create (Russell Yanofsky)
3c815cf wallet: Remove Verify and IsLoaded methods (Russell Yanofsky)
0d94e60 refactor: Use DatabaseStatus and DatabaseOptions types (Russell Yanofsky)
b5b4141 wallet: Add MakeDatabase function (Russell Yanofsky)
288b4ff Remove WalletLocation class (Russell Yanofsky)

Pull request description:

  Get rid of file path handling in wallet application code and move it down to database layer.

  There is no change in behavior except for some changed error messages.

  Motivation for this change is to make code more understandable, but also to prepare for adding SQLite support in #19077 so SQLite implementation can be contained at the database layer and wallet loading code does not need to become more complicated.

ACKs for top commit:
  achow101:
    ACK 7bf6dfb
  meshcollider:
    Code re-review and functional test run ACK 7bf6dfb

Tree-SHA512: 23ad18324c9e8947f0cf88a3734c2e9fb25536b2cb4d552cf5d1a4ade320fbffb73bb2d1b3a99585c11630aa7092e0fcfc2dd4fe65b91e3a54161433a5cd13cb

Author: meshcollider

src/bench/coin_selection.cpp

@@ -31,7 +31,7 @@ static void CoinSelection(benchmark::Bench& bench)
 {
     NodeContext node;
     auto chain = interfaces::MakeChain(node);
-    CWallet wallet(chain.get(), WalletLocation(), CreateDummyWalletDatabase());
+    CWallet wallet(chain.get(), "", CreateDummyWalletDatabase());
     wallet.SetupLegacyScriptPubKeyMan();
     std::vector<std::unique_ptr<CWalletTx>> wtxs;
     LOCK(wallet.cs_wallet);
@@ -65,7 +65,7 @@ static void CoinSelection(benchmark::Bench& bench)
 typedef std::set<CInputCoin> CoinSet;
 static NodeContext testNode;
 static auto testChain = interfaces::MakeChain(testNode);
-static CWallet testWallet(testChain.get(), WalletLocation(), CreateDummyWalletDatabase());
+static CWallet testWallet(testChain.get(), "", CreateDummyWalletDatabase());
 std::vector<std::unique_ptr<CWalletTx>> wtxn;
 
 // Copied from src/wallet/test/coinselector_tests.cpp

src/bench/wallet_balance.cpp

@@ -26,7 +26,7 @@ static void WalletBalance(benchmark::Bench& bench, const bool set_dirty, const b
 
     NodeContext node;
     std::unique_ptr<interfaces::Chain> chain = interfaces::MakeChain(node);
-    CWallet wallet{chain.get(), WalletLocation(), CreateMockWalletDatabase()};
+    CWallet wallet{chain.get(), "", CreateMockWalletDatabase()};
     {
         wallet.SetupLegacyScriptPubKeyMan();
         bool first_run;

src/interfaces/wallet.cpp

@@ -514,15 +514,22 @@ class WalletClientImpl : public WalletClient
     void setMockTime(int64_t time) override { return SetMockTime(time); }
 
     //! WalletClient methods
-    std::unique_ptr<Wallet> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, WalletCreationStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings) override
+    std::unique_ptr<Wallet> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, bilingual_str& error, std::vector<bilingual_str>& warnings) override
     {
         std::shared_ptr<CWallet> wallet;
-        status = CreateWallet(*m_context.chain, passphrase, wallet_creation_flags, name, true /* load_on_start */, error, warnings, wallet);
-        return MakeWallet(std::move(wallet));
+        DatabaseOptions options;
+        DatabaseStatus status;
+        options.require_create = true;
+        options.create_flags = wallet_creation_flags;
+        options.create_passphrase = passphrase;
+        return MakeWallet(CreateWallet(*m_context.chain, name, true /* load_on_start */, options, status, error, warnings));
     }
     std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) override
     {
-        return MakeWallet(LoadWallet(*m_context.chain, WalletLocation(name), true /* load_on_start */, error, warnings));
+        DatabaseOptions options;
+        DatabaseStatus status;
+        options.require_existing = true;
+        return MakeWallet(LoadWallet(*m_context.chain, name, true /* load_on_start */, options, status, error, warnings));
     }
     std::string getWalletDir() override
     {

src/interfaces/wallet.h

@@ -29,7 +29,6 @@ class CWallet;
 enum class FeeReason;
 enum class OutputType;
 enum class TransactionError;
-enum class WalletCreationStatus;
 enum isminetype : unsigned int;
 struct CRecipient;
 struct PartiallySignedTransaction;
@@ -311,7 +310,7 @@ class WalletClient : public ChainClient
 {
 public:
     //! Create new wallet.
-    virtual std::unique_ptr<Wallet> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, WalletCreationStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0;
+    virtual std::unique_ptr<Wallet> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0;
 
    //! Load existing wallet.
    virtual std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0;

src/qt/test/addressbooktests.cpp

@@ -61,7 +61,7 @@ void TestAddAddressesToSendBook(interfaces::Node& node)
 {
     TestChain100Setup test;
     node.setContext(&test.m_node);
-    std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), WalletLocation(), CreateMockWalletDatabase());
+    std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), "", CreateMockWalletDatabase());
     wallet->SetupLegacyScriptPubKeyMan();
     bool firstRun;
     wallet->LoadWallet(firstRun);

src/qt/test/wallettests.cpp

@@ -139,7 +139,7 @@ void TestGUI(interfaces::Node& node)
         test.CreateAndProcessBlock({}, GetScriptForRawPubKey(test.coinbaseKey.GetPubKey()));
     }
     node.setContext(&test.m_node);
-    std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), WalletLocation(), CreateMockWalletDatabase());
+    std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), "", CreateMockWalletDatabase());
     bool firstRun;
     wallet->LoadWallet(firstRun);
     {

src/qt/walletcontroller.cpp

@@ -249,10 +249,9 @@ void CreateWalletActivity::createWallet()
     }
 
     QTimer::singleShot(500, worker(), [this, name, flags] {
-        WalletCreationStatus status;
-        std::unique_ptr<interfaces::Wallet> wallet = node().walletClient().createWallet(name, m_passphrase, flags, status, m_error_message, m_warning_message);
+        std::unique_ptr<interfaces::Wallet> wallet = node().walletClient().createWallet(name, m_passphrase, flags, m_error_message, m_warning_message);
 
-        if (status == WalletCreationStatus::SUCCESS) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));
+        if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));
 
         QTimer::singleShot(500, this, &CreateWalletActivity::finish);
     });

src/wallet/bdb.cpp

@@ -52,18 +52,6 @@ bool WalletDatabaseFileId::operator==(const WalletDatabaseFileId& rhs) const
     return memcmp(value, &rhs.value, sizeof(value)) == 0;
 }
 
-bool IsBDBWalletLoaded(const fs::path& wallet_path)
-{
-    fs::path env_directory;
-    std::string database_filename;
-    SplitWalletPath(wallet_path, env_directory, database_filename);
-    LOCK(cs_db);
-    auto env = g_dbenvs.find(env_directory.string());
-    if (env == g_dbenvs.end()) return false;
-    auto database = env->second.lock();
-    return database && database->IsDatabaseLoaded(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.
@@ -371,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
@@ -824,3 +811,35 @@ std::unique_ptr<DatabaseBatch> BerkeleyDatabase::MakeBatch(const char* mode, boo
 {
     return MakeUnique<BerkeleyBatch>(*this, mode, flush_on_close);
 }
+
+bool ExistsBerkeleyDatabase(const fs::path& path)
+{
+    fs::path env_directory;
+    std::string data_filename;
+    SplitWalletPath(path, env_directory, data_filename);
+    return IsBerkeleyBtree(env_directory / data_filename);
+}
+
+std::unique_ptr<BerkeleyDatabase> MakeBerkeleyDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error)
+{
+    std::unique_ptr<BerkeleyDatabase> db;
+    {
+        LOCK(cs_db); // Lock env.m_databases until insert in BerkeleyDatabase constructor
+        std::string data_filename;
+        std::shared_ptr<BerkeleyEnvironment> env = GetWalletEnv(path, data_filename);
+        if (env->m_databases.count(data_filename)) {
+            error = Untranslated(strprintf("Refusing to load database. Data file '%s' is already loaded.", (env->Directory() / data_filename).string()));
+            status = DatabaseStatus::FAILED_ALREADY_LOADED;
+            return nullptr;
+        }
+        db = MakeUnique<BerkeleyDatabase>(std::move(env), std::move(data_filename));
+    }
+
+    if (options.verify && !db->Verify(error)) {
+        status = DatabaseStatus::FAILED_VERIFY;
+        return nullptr;
+    }
+
+    status = DatabaseStatus::SUCCESS;
+    return db;
+}

src/wallet/bdb.h

@@ -63,7 +63,6 @@ class BerkeleyEnvironment
 
     bool IsMock() const { return fMockDb; }
     bool IsInitialized() const { return fDbEnvInit; }
-    bool IsDatabaseLoaded(const std::string& db_filename) const { return m_databases.find(db_filename) != m_databases.end(); }
     fs::path Directory() const { return strPath; }
 
     bool Open(bilingual_str& error);
@@ -87,8 +86,8 @@ class BerkeleyEnvironment
 /** Get BerkeleyEnvironment and database filename given a wallet path. */
 std::shared_ptr<BerkeleyEnvironment> GetWalletEnv(const fs::path& wallet_path, std::string& database_filename);
 
-/** Return whether a BDB wallet database is currently loaded. */
-bool IsBDBWalletLoaded(const fs::path& wallet_path);
+/** Check format of database file */
+bool IsBerkeleyBtree(const fs::path& path);
 
 class BerkeleyBatch;
 
@@ -143,7 +142,10 @@ class BerkeleyDatabase : public WalletDatabase
     void ReloadDbEnv() override;
 
     /** Verifies the environment and database file */
-    bool Verify(bilingual_str& error) override;
+    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.
@@ -224,4 +226,10 @@ class BerkeleyBatch : public DatabaseBatch
 
 std::string BerkeleyDatabaseVersion();
 
+//! Check if Berkeley database exists at specified path.
+bool ExistsBerkeleyDatabase(const fs::path& path);
+
+//! Return object giving access to Berkeley database at specified path.
+std::unique_ptr<BerkeleyDatabase> MakeBerkeleyDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error);
+
 #endif // BITCOIN_WALLET_BDB_H

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;
-}