Add a lock to the wallet directory #11904

meshcollider · 2017-12-14T23:25:52Z

Fixes #11888, needs a 0.16 milestone

Also adds a test that the lock works.

#11687 will probably rework this to a per-wallet lock instead of just the walletdir, but this fixes the current issue

jtimon · 2017-12-15T00:20:39Z

Concept ACK

ryanofsky · 2017-12-15T02:43:31Z

Suggest moving LockWalletDirectory to wallet/db.cpp and calling it from CDBEnv::Open instead of VerifyWallets. That way this will be compatible with #11687. Also would suggest renaming LockWalletDirectory to LockEnvDirectory to be a little more consistent with terminology used in that file (even though the file isn't totally consistent right now).

meshcollider · 2017-12-16T19:14:49Z

@ryanofsky something like this? I had to probe the lock before calling open because of the weird database log backup thing when open fails, perhaps there is a cleaner way to do it e.g. returning different errors from open rather than just true/false. But I think this is ok

promag

Concept ACK.

Is it me or there can be a race between probing and the actual locking?

promag · 2017-12-17T00:45:25Z

src/wallet/db.cpp

+        if (probe_only) {
+            lock.unlock();
+        }
+    } catch(const boost::interprocess::interprocess_exception& e) {


Nit, missing space after catch.

promag · 2017-12-17T00:47:52Z

test/functional/multiwallet.py


+        competing_wallet_dir = os.path.join(self.options.tmpdir, 'competing_walletdir')
+        os.mkdir(competing_wallet_dir)
+        self.start_node(0, ['-walletdir='+competing_wallet_dir])


Use restart_node, and remove stop_node above?

meshcollider · 2017-12-17T07:42:48Z

Removed the probing with a bit of a rework

promag · 2017-12-17T09:48:55Z

src/wallet/db.h

+    enum OpenResult { OPEN_OK,
+                        ENV_LOCKED,
+                        OPEN_FAIL };
+    OpenResult Open(const fs::path& path);


How about bool Open(const fs::path& path, bool* is_locked = 0)?

Could do, but IMO doesn't simplify the code much, if we're returning something why not use return :)

Because you only care of is_locked when Open fails. For me ENV_LOCKED is the reason Open failed.

Also, you can avoid those != OPEN_OK conditions.

But sure, consider my suggestion a nit.

Ah that's true, ok 👍

https://github.com/bitcoin/bitcoin/pull/11904/files#r157363498

Seems fine to use either an enum or a bool, whichever you prefer. The real problem is that VerifyEnvironment is the wrong place for the rename/retry code to live. It creates a bizarre API because you can't reliably open an environment without making two different calls and pointlessly closing and reopening. It's also a bad division of code because the verify function has to make assumptions about what the open function is doing internally.

A better way to organize the code would be to have a single bool CDBEnv::Open(fs::path, bool retry=false) function, where setting the retry parameter to true renames the log directory and reopens if the initial open fails. Verifying would then just call Open(path, true) followed by a Close. Anyway this would be nice to clean up, but no need to do it here as part of fixing the lock issue.

ryanofsky

utACK 8020abc808cbf8e0b8db24f92eafbf3262ea9163

ryanofsky · 2017-12-18T13:35:39Z

src/wallet/db.h

+    enum OpenResult { OPEN_OK,
+                        ENV_LOCKED,
+                        OPEN_FAIL };
+    OpenResult Open(const fs::path& path);


https://github.com/bitcoin/bitcoin/pull/11904/files#r157363498

Seems fine to use either an enum or a bool, whichever you prefer. The real problem is that VerifyEnvironment is the wrong place for the rename/retry code to live. It creates a bizarre API because you can't reliably open an environment without making two different calls and pointlessly closing and reopening. It's also a bad division of code because the verify function has to make assumptions about what the open function is doing internally.

A better way to organize the code would be to have a single bool CDBEnv::Open(fs::path, bool retry=false) function, where setting the retry parameter to true renames the log directory and reopens if the initial open fails. Verifying would then just call Open(path, true) followed by a Close. Anyway this would be nice to clean up, but no need to do it here as part of fixing the lock issue.

promag · 2017-12-20T00:04:33Z

src/wallet/db.cpp

+                // if it still fails, it probably means we can't even create the database env
+                return false;
+            }
+        return false;


Something wrong here, missing }?

Yep oops, bad code move. Fixed.

promag · 2017-12-20T00:38:54Z

src/wallet/db.h

@@ -68,7 +68,7 @@ class CDBEnv
    typedef std::pair<std::vector<unsigned char>, std::vector<unsigned char> > KeyValPair;
    bool Salvage(const std::string& strFile, bool fAggressive, std::vector<KeyValPair>& vResult);

-    bool Open(const fs::path& path);
+    bool Open(const fs::path& path, bool retry = 0);


BTW, how about:

private: bool Open(const fs::path& path, bool retry); public: bool Open(const fs::path& path) { return Open(path, true); }

this way the public API remains, for instance, https://github.com/bitcoin/bitcoin/pull/11904/files#diff-5cd083d2c34da5f0ac380011f7b725d2R312 goes away.

I'd prefer to leave it explicit I think, is there any other benefit to hiding the retry parameter?

It's an internal detail, you should not be able to call Open(path, false), it's only there for the 2nd attempt.

Its also called without retry here:
https://github.com/MeshCollider/bitcoin/blob/f70ce89806a7f0ce1755e7d074f6e07753a85694/src/wallet/db.cpp#L436

Ok, missed that.

promag · 2017-12-21T15:23:26Z

Tested ACK. Needs squash.

ryanofsky

utACK f70ce89806a7f0ce1755e7d074f6e07753a85694. Only change since last review is CDBEnv::Open retry refactor.

ryanofsky · 2017-12-21T16:16:16Z

src/wallet/db.cpp

+    if (file) fclose(file);
+
+    try {
+        static boost::interprocess::file_lock lock(lock_file_path.string().c_str());


Didn't notice this before, and no need for a change now, but just in case #11687 is somehow merged before this PR, static variable here should move to CDBEnv instance variable. (A single static variable can't guard different database directories if different database directories may be opened during the lifetime of the process.)

ryanofsky · 2017-12-21T16:22:32Z

src/wallet/db.cpp

-            return false;
-        }
+    if (!bitdb.Open(walletDir, true)) {
+        errorStr = strprintf(_("Cannot obtain a lock on wallet directory %s. Another instance of bitcoin may be using it."), walletDir);


Might want to keep the previous generic error here ("Error initializing wallet database environment") since this can happen in other conditions than failing to obtain the lock.

meshcollider · 2017-12-23T23:46:40Z

Squashed and added a commit to modify the error message as suggested.

Note travis failure on second commit is just because the last three tests were cancelled due to third commit being pushed before they started

sipa · 2017-12-25T14:51:56Z

src/wallet/db.cpp

+bool LockEnvDirectory(const fs::path& env_path)
+{
+    // Make sure only a single Bitcoin process is using the wallet directory.
+    fs::path lock_file_path = env_path / ".lock";


What if the walletdir is the same as the datadir? Won't there be a problem if both use the same lockfile? I would consider renaming this to ".walletlock" perhaps (which is also clearer to people who see the file).

Reading more, I think this is indeed advisable:

From http://www.boost.org/doc/libs/1_55_0/doc/html/interprocess/synchronization_mechanisms.html#interprocess.synchronization_mechanisms.file_lock:

It's unspecified if a file_lock synchronizes two threads from the same process.

It's unspecified if a process can use two file_lock objects pointing to the same file.

Good point, fixed!

sipa · 2017-12-25T14:53:02Z

src/wallet/db.cpp

@@ -52,6 +53,24 @@ void CheckUniqueFileid(const CDBEnv& env, const std::string& filename, Db& db)
        }
    }
 }
+
+bool LockEnvDirectory(const fs::path& env_path)


A lot in this function seems to be a duplication from LockDataDirectory in init.cpp. What do you think about abstracting them out to a utility function in util.cpp instead?

sipa · 2017-12-26T07:06:10Z

utACK 005d4cdef7a7a9f98cf202db7542a7fd01e048b8

meshcollider · 2017-12-26T07:53:43Z

Thanks for the feedback @sipa

ryanofsky · 2017-12-27T00:32:44Z

src/util.cpp

+    if (file) fclose(file);
+
+    try {
+        static boost::interprocess::file_lock lock(pathLockFile.string().c_str());


From https://botbot.me/freenode/bitcoin-core-dev/msg/95065183/

I've been trying to work out why the last commit in #11904 is breaking the locking function, it works fine before that commit is added, but I can't work out why it doesn't work with that change. with a brand new walletdir, it creates the .walletlock file, but lock.try_lock() fails and the node crashes on startup, does anyone with more boost familiarity than me know why lock.try_lock fails when I move it

I haven't looked closely at changes here yet, but as I was trying to say in #11904 (comment), I think the static declaration on this line becomes broken as soon as more than one directory is locked, because this will only create one single file_lock instance no matter how many different directories need to be locked. I think what you really need here is a map or list of locks, not one single lock. Perhaps something along the lines of (untested):

static std::map<std::string, boost::interprocess::file_lock> locks; boost::interprocess::file_lock& lock = locks.emplace(pathLockFile.string(), pathLockFile.string().c_str()).first->second;

Ah, of course. Fixed. Thank you!

ryanofsky

utACK abcfc2713d4e2696b5178c15975291bfd79a68b7. Main change since previous review was adding the more general LockDirectory util function.

Would also recommend just squashing everything down into a single commit to avoid this being a garden-path pr.

ryanofsky · 2018-01-03T18:06:29Z

src/util.cpp

@@ -375,6 +376,27 @@ int LogPrintStr(const std::string &str)
    return ret;
 }

+bool LockDirectory(const fs::path& directory, const std::string lockfile_name, bool probe_only)


Maybe combine directory and std::string lockfile_name arguments into a single path argument. Having 2 arguments seems more verbose and less flexible.

Indeed, was just for the purpose of printing out the directory in the log message below rather than the filename too. Will modify if/when there's another change too

I kind of like these separated out - after all, the function is called LockDirectory and not CreateLockFile. If you merge the directory and lockfile name, I'd say you'd also have to rename the function because it's not longer specifically about locking directories.

Nit: const std::string &lockfile_name

ryanofsky · 2018-01-03T18:07:30Z

src/util.cpp

+            lock.unlock();
+        }
+    } catch (const boost::interprocess::interprocess_exception& e) {
+        return error("Error while attempting to lock directory %s: %s", directory.string(), e.what());


Would be good to include filename so you could see whether wallet or data lock failed.

Could do, but in both cases another more specific error message is printed out when LockDirectory returns, so it's always on the next log line anyway

laanwj · 2018-01-15T11:48:07Z

utACK, needs rebase

meshcollider · 2018-01-16T06:06:14Z

Rebased

laanwj · 2018-01-16T10:11:24Z

Tested ACK 2f3bd47

2018-01-16 10:09:49 Cannot obtain a lock on wallet directory /tmp/wallet1. Another instance of bitcoin may be using it.

2f3bd47 Abstract directory locking into util.cpp (MeshCollider) 5260a4a Make .walletlock distinct from .lock (MeshCollider) 64226de Generalise walletdir lock error message for correctness (MeshCollider) c9ed4bd Add a test for wallet directory locking (MeshCollider) e60cb99 Add a lock to the wallet directory (MeshCollider) Pull request description: Fixes #11888, needs a 0.16 milestone Also adds a test that the lock works. #11687 will probably rework this to a per-wallet lock instead of just the walletdir, but this fixes the current issue Tree-SHA512: 98e52d67f820e3b8f919cf361ffbb7d928f1bd67603e0ed26c5076ea02d9b3a90c3535ddf7329f3b88171396fa28dd3c87adab3577a8a217bd1e4247bda99138

2f3bd47 Abstract directory locking into util.cpp (MeshCollider) 5260a4a Make .walletlock distinct from .lock (MeshCollider) 64226de Generalise walletdir lock error message for correctness (MeshCollider) c9ed4bd Add a test for wallet directory locking (MeshCollider) e60cb99 Add a lock to the wallet directory (MeshCollider) Pull request description: Fixes bitcoin#11888, needs a 0.16 milestone Also adds a test that the lock works. bitcoin#11687 will probably rework this to a per-wallet lock instead of just the walletdir, but this fixes the current issue Tree-SHA512: 98e52d67f820e3b8f919cf361ffbb7d928f1bd67603e0ed26c5076ea02d9b3a90c3535ddf7329f3b88171396fa28dd3c87adab3577a8a217bd1e4247bda99138

…xternal wallet files capabilities 056f4e8 [GUI] settings information widget setting the correct data directory. (furszy) f765611 bugfix: Remove dangling wallet env instance (João Barbosa) 524103f Implement and connect CWallet::GetPathToFile to GUI. (furszy) e7aa6bd [Refactor] First load all wallets, then back em (random-zebra) 91b112b [Refactor][Bug] Fix automatic backups, final code deduplication (random-zebra) 12a1e39 [BUG] Sanitize wallet name in GetUniqueWalletBackupName (random-zebra) 7aa251d wallet: Fix backupwallet for multiwallets (Daniel Kraft) 351d2c8 wallet_tests: mock wallet db. (furszy) 565abcd db: fix db path not removed from the open db environments map. (furszy) 4cae8dc test: Add unit test for LockDirectory Add a unit test for LockDirectory, introduced in btc#11281. (W. J. van der Laan) 16b4651 util: Fix multiple use of LockDirectory This commit fixes problems with calling LockDirectory multiple times on the same directory, or from multiple threads. It also fixes the build on OpenBSD. (W. J. van der Laan) 9ae619a Test: Use specific testing setups for wallet_zkeys_tests tests (furszy) d86cd4f wallet: Add missing check for backup to source wallet file. (furszy) d9e1c6b Abstract VerifyWalletPath and connect it to init and GUI. (furszy) 23458ca GUI: Implement and connect WalletModel::getWalletPath(). (furszy) c2d3a07 Create new wallet databases as directories rather than files (Russell Yanofsky) daa7fe5 Allow wallet files not in -walletdir directory (Russell Yanofsky) 9b2dae1 Allow wallet files in multiple directories (furszy) d36185a wallet: unify backup creation process. (furszy) 8b8725d wallet_tests: Use dummy wallet instance instead of wallet pointer. (furszy) 434ed75 Abstract LockDirectory into system.cpp (furszy) 6a0380a Make .walletlock distinct from .lock (MeshCollider) d8539bb Generalise walletdir lock error message for correctness (MeshCollider) ddcfd4a Enable test for wallet directory locking (furszy) a238a8d Add a lock to the wallet directory (MeshCollider) 1dc2219 Don't allow relative -walletdir paths (Russell Yanofsky) dcb43ea Create walletdir if datadir doesn't exist and correct tests (furszy) 03db5c8 Default walletdir is wallets/ if it exists (MeshCollider) 359b01d Add release notes for -walletdir and wallets/ dir (MeshCollider) 71a4701 Add -walletdir parameter to specify custom wallet dir (furszy) 5b31813 Use unique_ptr for dbenv (DbEnv) (practicalswift) a1bef4f Refactor: Modernize disallowed copy constructors/assignment (Dan Raviv) Pull request description: > Adding more flexibility in where the wallets directory can be located. Before, the wallet database files were stored at the top level of the PIVX data directory. Now the location of the wallets directory can be overridden by specifying a `-walletdir=<path>` option where `<path>` can be an absolute path to a directory or directory symlink. > >Another advantage of this change is that if two wallets are located in the same directory, they will now use their own BerkeleyDB environments instead using a shared environment. Using a shared environment makes it difficult to manage and back up wallets separately because transaction log files will contain a mix of data from all wallets in the environment. Coming from: * bitcoin#11351 -> Refactor: Modernize disallowed copy constructors/assignment. * bitcoin#11466 -> Specify custom wallet directory with -walletdir param. * bitcoin#11726 -> Cleanups + nit fixes for -walletdir work. * bitcoin#11904 -> Add lock to the wallet directory. * bitcoin#11687 -> External wallet files support. * bitcoin#12166 -> Doc better -walletdir description. * bitcoin#12220 -> Error if relative -walletdir is specified. This finishes the work started in #2337, enabling all the commented tests inside `wallet_multiwallet.py` and more. PR built on top of #2369. Note: Test this properly. ACKs for top commit: random-zebra: utACK 056f4e8 Fuzzbawls: ACK 056f4e8 Tree-SHA512: 98ae515dd664f959d35b63a0998bd93ca3bcea30ca67caccd2a694a440d10e18f831a54720ede0415d8f5e13af252bc6048a820491863d243df70ccc9d5fa7c6

fanquake added the Wallet label Dec 15, 2017

fanquake added this to the 0.16.0 milestone Dec 15, 2017

promag reviewed Dec 17, 2017

View reviewed changes

ryanofsky reviewed Dec 18, 2017

View reviewed changes

promag reviewed Dec 20, 2017

View reviewed changes

ryanofsky reviewed Dec 21, 2017

View reviewed changes

sipa reviewed Dec 25, 2017

View reviewed changes

ryanofsky reviewed Dec 27, 2017

View reviewed changes

ryanofsky reviewed Jan 3, 2018

View reviewed changes

meshcollider added 5 commits January 16, 2018 19:02

Add a lock to the wallet directory

e60cb99

Add a test for wallet directory locking

c9ed4bd

Generalise walletdir lock error message for correctness

64226de

Make .walletlock distinct from .lock

5260a4a

Abstract directory locking into util.cpp

2f3bd47

laanwj merged commit 2f3bd47 into bitcoin:master Jan 16, 2018

ajtowns mentioned this pull request Jan 16, 2018

[tests] Rename functional tests #11774

Merged

meshcollider deleted the 201712_walletdir_lock branch January 17, 2018 00:29

ryanofsky mentioned this pull request Jan 17, 2018

External wallet files #11687

Merged

ryanofsky mentioned this pull request Jul 16, 2020

wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch #19335

Merged

ryanofsky mentioned this pull request Oct 1, 2020

wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets #19077

Merged

furszy mentioned this pull request Jun 15, 2021

[Wallet] Introduce wallets directory configuration and external wallet files capabilities PIVX-Project/PIVX#2423

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Add a lock to the wallet directory #11904

Add a lock to the wallet directory #11904

Conversation

meshcollider commented Dec 14, 2017 • edited

jtimon commented Dec 15, 2017

ryanofsky commented Dec 15, 2017 • edited

meshcollider commented Dec 16, 2017

promag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meshcollider commented Dec 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

promag commented Dec 21, 2017

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Dec 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meshcollider commented Dec 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sipa commented Dec 26, 2017

meshcollider commented Dec 26, 2017 • edited

ryanofsky Dec 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laanwj commented Jan 15, 2018

meshcollider commented Jan 16, 2018

laanwj commented Jan 16, 2018

meshcollider commented Dec 14, 2017 •

edited

ryanofsky commented Dec 15, 2017 •

edited

ryanofsky Dec 21, 2017 •

edited

meshcollider commented Dec 23, 2017 •

edited

meshcollider commented Dec 26, 2017 •

edited

ryanofsky Dec 27, 2017 •

edited

ryanofsky left a comment •

edited