Merge #11687: External wallet files

be8ab7d Create new wallet databases as directories rather than files (Russell Yanofsky)
26c06f2 Allow wallet files not in -walletdir directory (Russell Yanofsky)
d8a99f6 Allow wallet files in multiple directories (Russell Yanofsky)

Pull request description:

  This change consists of three commits:

  * The first commit is a pure refactoring that removes the restriction that two wallets can only be opened at the same time if they are contained in the same directory.
  * The second commit removes the restriction that `-wallet` filenames can only refer to files in the `-walletdir` directory.
  * The third commit makes second commit a little safer by changing bitcoin to create wallet databases as directories rather than files, so they can be safely backed up.

  All three commits should be straightforward:

  *  The first commit adds around 20 lines of new code and then updates a bunch of function signatures (generally updating them to take plain fs::path parameters, instead of combinations of strings, fs::paths, and objects like CDBEnv and CWalletDBWrapper).
  * The second commit removes two `-wallet` filename checks and adds some test cases to the multiwallet unit test.
  * The third commit just changes the mapping from specified wallet paths to bdb environment & data paths.

    ---

  **Note:** For anybody looking at this PR for the first time, I think you can skip the comments before _20 Nov_ and start reading at bitcoin/bitcoin#11687 (comment). Comments before  _20 Nov_ were about an earlier version of the PR that didn't include the third commit, and then confusion from not seeing the first commit.

Tree-SHA512: 00bbb120fe0df847cf57014f75f1f7f1f58b0b62fa0b3adab4560163ebdfe06ccdfff33b4231693f03c5dc23601cb41954a07bcea9a4919c8d42f7d62bcf6024

Author: laanwj

doc/release-notes.md

@@ -63,6 +63,36 @@ RPC changes
 
 - The `fundrawtransaction` rpc will reject the previously deprecated `reserveChangeKey` option.
 
+External wallet files
+---------------------
+
+The `-wallet=<path>` option now accepts full paths instead of requiring wallets
+to be located in the -walletdir directory.
+
+Newly created wallet format
+---------------------------
+
+If `-wallet=<path>` is specified with a path that does not exist, it will now
+create a wallet directory at the specified location (containing a wallet.dat
+data file, a db.log file, and database/log.?????????? files) instead of just
+creating a data file at the path and storing log files in the parent
+directory. This should make backing up wallets more straightforward than
+before because the specified wallet path can just be directly archived without
+having to look in the parent directory for transaction log files.
+
+For backwards compatibility, wallet paths that are names of existing data files
+in the `-walletdir` directory will continue to be accepted and interpreted the
+same as before.
+
+Low-level RPC changes
+---------------------
+
+- When bitcoin is not started with any `-wallet=<path>` options, the name of
+  the default wallet returned by `getwalletinfo` and `listwallets` RPCs is
+  now the empty string `""` instead of `"wallet.dat"`. If bitcoin is started
+  with any `-wallet=<path>` options, there is no change in behavior, and the
+  name of any wallet is just its `<path>` string.
+
 Credits
 =======
 

src/bench/coin_selection.cpp

@@ -32,7 +32,7 @@ static void addCoin(const CAmount& nValue, const CWallet& wallet, std::vector<CO
 // (https://github.com/bitcoin/bitcoin/issues/7883#issuecomment-224807484)
 static void CoinSelection(benchmark::State& state)
 {
-    const CWallet wallet;
+    const CWallet wallet("dummy", CWalletDBWrapper::CreateDummy());
     std::vector<COutput> vCoins;
     LOCK(wallet.cs_wallet);
 

src/bitcoin-cli.cpp

@@ -46,7 +46,7 @@ std::string HelpMessageCli()
     strUsage += HelpMessageOpt("-rpcport=<port>", strprintf(_("Connect to JSON-RPC on <port> (default: %u or testnet: %u)"), defaultBaseParams->RPCPort(), testnetBaseParams->RPCPort()));
     strUsage += HelpMessageOpt("-rpcuser=<user>", _("Username for JSON-RPC connections"));
     strUsage += HelpMessageOpt("-rpcwait", _("Wait for RPC server to start"));
-    strUsage += HelpMessageOpt("-rpcwallet=<walletname>", _("Send RPC for non-default wallet on RPC server (argument is wallet filename in bitcoind directory, required if bitcoind/-Qt runs with multiple wallets)"));
+    strUsage += HelpMessageOpt("-rpcwallet=<walletname>", _("Send RPC for non-default wallet on RPC server (needs to exactly match corresponding -wallet option passed to bitcoind)"));
     strUsage += HelpMessageOpt("-stdin", _("Read extra arguments from standard input, one per line until EOF/Ctrl-D (recommended for sensitive information such as passphrases).  When combined with -stdinrpcpass, the first line from standard input is used for the RPC password."));
     strUsage += HelpMessageOpt("-stdinrpcpass", strprintf(_("Read RPC password from standard input as a single line.  When combined with -stdin, the first line from standard input is used for the RPC password.")));
 
@@ -339,8 +339,8 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co
 
     // check if we should use a special wallet endpoint
     std::string endpoint = "/";
-    std::string walletName = gArgs.GetArg("-rpcwallet", "");
-    if (!walletName.empty()) {
+    if (!gArgs.GetArgs("-rpcwallet").empty()) {
+        std::string walletName = gArgs.GetArg("-rpcwallet", "");
         char *encodedURI = evhttp_uriencode(walletName.c_str(), walletName.size(), false);
         if (encodedURI) {
             endpoint = "/wallet/"+ std::string(encodedURI);

src/qt/test/wallettests.cpp

@@ -158,9 +158,7 @@ void TestGUI()
     for (int i = 0; i < 5; ++i) {
         test.CreateAndProcessBlock({}, GetScriptForRawPubKey(test.coinbaseKey.GetPubKey()));
     }
-    bitdb.MakeMock();
-    std::unique_ptr<CWalletDBWrapper> dbw(new CWalletDBWrapper(&bitdb, "wallet_test.dat"));
-    CWallet wallet(std::move(dbw));
+    CWallet wallet("mock", CWalletDBWrapper::CreateMock());
     bool firstRun;
     wallet.LoadWallet(firstRun);
     {
@@ -261,9 +259,6 @@ void TestGUI()
     QPushButton* removeRequestButton = receiveCoinsDialog.findChild<QPushButton*>("removeRequestButton");
     removeRequestButton->click();
     QCOMPARE(requestTableModel->rowCount({}), currentRowCount-1);
-
-    bitdb.Flush(true);
-    bitdb.Reset();
 }
 
 }

src/wallet/db.cpp

@@ -52,20 +52,55 @@ void CheckUniqueFileid(const CDBEnv& env, const std::string& filename, Db& db)
         }
     }
 }
+
+CCriticalSection cs_db;
+std::map<std::string, CDBEnv> g_dbenvs; //!< Map from directory name to open db environment.
 } // namespace
 
+CDBEnv* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename)
+{
+    fs::path env_directory;
+    if (fs::is_regular_file(wallet_path)) {
+        // Special case for backwards compatibility: if wallet path points to an
+        // existing file, treat it as the path to a BDB data file in a parent
+        // directory that also contains BDB log files.
+        env_directory = wallet_path.parent_path();
+        database_filename = wallet_path.filename().string();
+    } else {
+        // Normal case: Interpret wallet path as a directory path containing
+        // data and log files.
+        env_directory = wallet_path;
+        database_filename = "wallet.dat";
+    }
+    LOCK(cs_db);
+    // Note: An ununsed temporary CDBEnv 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;
+}
+
 //
 // CDB
 //
 
-CDBEnv bitdb;
-
-void CDBEnv::EnvShutdown()
+void CDBEnv::Close()
 {
     if (!fDbEnvInit)
         return;
 
     fDbEnvInit = false;
+
+    for (auto& db : mapDb) {
+        auto count = mapFileUseCount.find(db.first);
+        assert(count == mapFileUseCount.end() || count->second == 0);
+        if (db.second) {
+            db.second->close(0);
+            delete db.second;
+            db.second = nullptr;
+        }
+    }
+
     int ret = dbenv->close(0);
     if (ret != 0)
         LogPrintf("CDBEnv::EnvShutdown: Error %d shutting down database environment: %s\n", ret, DbEnv::strerror(ret));
@@ -80,29 +115,25 @@ void CDBEnv::Reset()
     fMockDb = false;
 }
 
-CDBEnv::CDBEnv()
+CDBEnv::CDBEnv(const fs::path& dir_path) : strPath(dir_path.string())
 {
     Reset();
 }
 
 CDBEnv::~CDBEnv()
 {
-    EnvShutdown();
+    Close();
 }
 
-void CDBEnv::Close()
-{
-    EnvShutdown();
-}
-
-bool CDBEnv::Open(const fs::path& pathIn, bool retry)
+bool CDBEnv::Open(bool retry)
 {
     if (fDbEnvInit)
         return true;
 
     boost::this_thread::interruption_point();
 
-    strPath = pathIn.string();
+    fs::path pathIn = strPath;
+    TryCreateDirectories(pathIn);
     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;
@@ -150,7 +181,7 @@ bool CDBEnv::Open(const fs::path& pathIn, bool retry)
                 // 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 (!Open(false /* retry */)) {
                 // if it still fails, it probably means we can't even create the database env
                 return false;
             }
@@ -209,12 +240,15 @@ CDBEnv::VerifyResult CDBEnv::Verify(const std::string& strFile, recoverFunc_type
         return RECOVER_FAIL;
 
     // Try to recover:
-    bool fRecovered = (*recoverFunc)(strFile, out_backup_filename);
+    bool fRecovered = (*recoverFunc)(fs::path(strPath) / strFile, out_backup_filename);
     return (fRecovered ? RECOVER_OK : RECOVER_FAIL);
 }
 
-bool CDB::Recover(const std::string& filename, void *callbackDataIn, bool (*recoverKVcallback)(void* callbackData, CDataStream ssKey, CDataStream ssValue), std::string& newFilename)
+bool CDB::Recover(const fs::path& file_path, void *callbackDataIn, bool (*recoverKVcallback)(void* callbackData, CDataStream ssKey, CDataStream ssValue), std::string& newFilename)
 {
+    std::string filename;
+    CDBEnv* env = GetWalletEnv(file_path, filename);
+
     // Recovery procedure:
     // move wallet file to walletfilename.timestamp.bak
     // Call Salvage with fAggressive=true to
@@ -225,7 +259,7 @@ bool CDB::Recover(const std::string& filename, void *callbackDataIn, bool (*reco
     int64_t now = GetTime();
     newFilename = strprintf("%s.%d.bak", filename, now);
 
-    int result = bitdb.dbenv->dbrename(nullptr, filename.c_str(), nullptr,
+    int result = env->dbenv->dbrename(nullptr, filename.c_str(), nullptr,
                                        newFilename.c_str(), DB_AUTO_COMMIT);
     if (result == 0)
         LogPrintf("Renamed %s to %s\n", filename, newFilename);
@@ -236,15 +270,15 @@ bool CDB::Recover(const std::string& filename, void *callbackDataIn, bool (*reco
     }
 
     std::vector<CDBEnv::KeyValPair> salvagedData;
-    bool fSuccess = bitdb.Salvage(newFilename, true, salvagedData);
+    bool fSuccess = env->Salvage(newFilename, true, salvagedData);
     if (salvagedData.empty())
     {
         LogPrintf("Salvage(aggressive) found no records in %s.\n", newFilename);
         return false;
     }
     LogPrintf("Salvage(aggressive) found %u records\n", salvagedData.size());
 
-    std::unique_ptr<Db> pdbCopy = MakeUnique<Db>(bitdb.dbenv.get(), 0);
+    std::unique_ptr<Db> pdbCopy = MakeUnique<Db>(env->dbenv.get(), 0);
     int ret = pdbCopy->open(nullptr,               // Txn pointer
                             filename.c_str(),   // Filename
                             "main",             // Logical db name
@@ -257,7 +291,7 @@ bool CDB::Recover(const std::string& filename, void *callbackDataIn, bool (*reco
         return false;
     }
 
-    DbTxn* ptxn = bitdb.TxnBegin();
+    DbTxn* ptxn = env->TxnBegin();
     for (CDBEnv::KeyValPair& row : salvagedData)
     {
         if (recoverKVcallback)
@@ -279,8 +313,12 @@ bool CDB::Recover(const std::string& filename, void *callbackDataIn, bool (*reco
     return fSuccess;
 }
 
-bool CDB::VerifyEnvironment(const std::string& walletFile, const fs::path& walletDir, std::string& errorStr)
+bool CDB::VerifyEnvironment(const fs::path& file_path, std::string& errorStr)
 {
+    std::string walletFile;
+    CDBEnv* env = GetWalletEnv(file_path, walletFile);
+    fs::path walletDir = env->Directory();
+
     LogPrintf("Using BerkeleyDB version %s\n", DbEnv::version(0, 0, 0));
     LogPrintf("Using wallet %s\n", walletFile);
 
@@ -291,20 +329,24 @@ bool CDB::VerifyEnvironment(const std::string& walletFile, const fs::path& walle
         return false;
     }
 
-    if (!bitdb.Open(walletDir, true)) {
+    if (!env->Open(true /* retry */)) {
         errorStr = strprintf(_("Error initializing wallet database environment %s!"), walletDir);
         return false;
     }
 
     return true;
 }
 
-bool CDB::VerifyDatabaseFile(const std::string& walletFile, const fs::path& walletDir, std::string& warningStr, std::string& errorStr, CDBEnv::recoverFunc_type recoverFunc)
+bool CDB::VerifyDatabaseFile(const fs::path& file_path, std::string& warningStr, std::string& errorStr, CDBEnv::recoverFunc_type recoverFunc)
 {
+    std::string walletFile;
+    CDBEnv* env = GetWalletEnv(file_path, walletFile);
+    fs::path walletDir = env->Directory();
+
     if (fs::exists(walletDir / walletFile))
     {
         std::string backup_filename;
-        CDBEnv::VerifyResult r = bitdb.Verify(walletFile, recoverFunc, backup_filename);
+        CDBEnv::VerifyResult r = env->Verify(walletFile, recoverFunc, backup_filename);
         if (r == CDBEnv::RECOVER_OK)
         {
             warningStr = strprintf(_("Warning: Wallet file corrupt, data salvaged!"
@@ -414,8 +456,8 @@ CDB::CDB(CWalletDBWrapper& dbw, const char* pszMode, bool fFlushOnCloseIn) : pdb
         nFlags |= DB_CREATE;
 
     {
-        LOCK(env->cs_db);
-        if (!env->Open(GetWalletDir()))
+        LOCK(cs_db);
+        if (!env->Open(false /* retry */))
             throw std::runtime_error("CDB: Failed to open database environment.");
 
         pdb = env->mapDb[strFilename];
@@ -442,7 +484,25 @@ CDB::CDB(CWalletDBWrapper& dbw, const char* pszMode, bool fFlushOnCloseIn) : pdb
             if (ret != 0) {
                 throw std::runtime_error(strprintf("CDB: Error %d, can't open database %s", ret, strFilename));
             }
-            CheckUniqueFileid(*env, strFilename, *pdb_temp);
+
+            // Call CheckUniqueFileid on the containing BDB environment to
+            // avoid BDB data consistency bugs that happen when different data
+            // files in the same environment have the same fileid.
+            //
+            // Also call CheckUniqueFileid on all the other g_dbenvs to prevent
+            // bitcoin from opening the same data file through another
+            // environment when the file is referenced through equivalent but
+            // not obviously identical symlinked or hard linked or bind mounted
+            // paths. In the future a more relaxed check for equal inode and
+            // device ids could be done instead, which would allow opening
+            // different backup copies of a wallet at the same time. Maybe even
+            // more ideally, an exclusive lock for accessing the database could
+            // be implemented, so no equality checks are needed at all. (Newer
+            // versions of BDB have an set_lk_exclusive method for this
+            // purpose, but the older version we use does not.)
+            for (auto& env : g_dbenvs) {
+                CheckUniqueFileid(env.second, strFilename, *pdb_temp);
+            }
 
             pdb = pdb_temp.release();
             env->mapDb[strFilename] = pdb;
@@ -490,7 +550,7 @@ void CDB::Close()
         Flush();
 
     {
-        LOCK(env->cs_db);
+        LOCK(cs_db);
         --env->mapFileUseCount[strFile];
     }
 }
@@ -518,7 +578,7 @@ bool CDB::Rewrite(CWalletDBWrapper& dbw, const char* pszSkip)
     const std::string& strFile = dbw.strFile;
     while (true) {
         {
-            LOCK(env->cs_db);
+            LOCK(cs_db);
             if (!env->mapFileUseCount.count(strFile) || env->mapFileUseCount[strFile] == 0) {
                 // Flush log data to the dat file
                 env->CloseDb(strFile);
@@ -646,7 +706,7 @@ bool CDB::PeriodicFlush(CWalletDBWrapper& dbw)
     bool ret = false;
     CDBEnv *env = dbw.env;
     const std::string& strFile = dbw.strFile;
-    TRY_LOCK(bitdb.cs_db,lockDb);
+    TRY_LOCK(cs_db, lockDb);
     if (lockDb)
     {
         // Don't do this if any databases are in use
@@ -694,7 +754,7 @@ bool CWalletDBWrapper::Backup(const std::string& strDest)
     while (true)
     {
         {
-            LOCK(env->cs_db);
+            LOCK(cs_db);
             if (!env->mapFileUseCount.count(strFile) || env->mapFileUseCount[strFile] == 0)
             {
                 // Flush log data to the dat file