Specify custom wallet directory with -walletdir param #11466

meshcollider · 2017-10-09T09:55:29Z

Adds a -walletdir parameter which specifies a directory to use for wallets, allowing them to be stored separately from the 'main' data directory. Creates a new wallets/ directory in datadir if this is the first time running, and defaults to using it if it exists.

Includes tests and release notes. Things which might need to be considered more:

there is no 'lock' on the wallets directory, which might be needed?
because this uses a new wallets/ directory by default, downgrading to an earlier version won't see the wallets in that directory (not a big deal though, users can just copy them up to the main dir)
jnewbery suggested putting each wallet in its own directory, which is a good idea, but out of scope for this PR IMO. EDIT: this is being done in External wallet files #11687
doc/files.md needs updating (will do soon)

I also considered including a cleanup by removing caching of data directory paths and instead just initialise them once on startup (c.f. #3073), but decided it wasn't super relevant here will just complicate review.

promag

Concept ACK. Regardless of being WIP, some comments.

promag · 2017-10-09T10:06:50Z

src/util.cpp

@@ -590,6 +590,10 @@ fs::path GetWalletDir()
        }
    } else {
        path = GetDataDir();
+        // If a wallets directory exists, use that, otherwise default to GetDataDir
+        if (fs::is_directory(path / "wallets")) {


In commit Default wallet directory is wallets/ if it exists, missing test?

This is part of the WIP, I'll add a test for that when I make it actually initialise the directory :)

promag · 2017-10-09T10:12:32Z

src/wallet/rpcwallet.cpp

-            "  \"walletname\"            (string) the wallet name\n"
-            "   ...\n"
-            "]\n"
+            "{\n"


This is breaking change. Also, this is not so relevant to this PR? I suggest to create a PR with this to separate discussions.

Another interface is:

{ "wallets": [ { "name": "foo", "loaded": true }, { "name": "bar", "loaded": false } ] }

Which has the advantage of allowing to add new attributes later.

Re: breaking change, I believe it was only added in 0.15 anyway so its expected to be more unstable. Can separate into a new PR if desired, but I think its only really a useful change in this context, so I'll wait to see what others think

Also re: interface, that idea is nice but it doesn't work well with the current logic of looking for *.dat files imo, available is slightly less suggestive that they are actually wallets than putting them all in a wallets list, you'd end up with { "name": "peers.dat", "loaded": false} if you were using the datadir. If you or anyone has a better idea for finding wallets then that would be ok

If you or anyone has a better idea for finding wallets then that would be ok

You could look for berkeleydb magic bytes to at least filter out files which can't be bdb databases:

https://github.com/file/file/blob/master/magic/Magdir/database

What is the goal? Why anyone wants to list available wallets? I guess that whoever wants multiwallet keeps a record of the wallets externally? ~~Anyway, I strongly suggest to move that discussion to a new PR.~~

This change may have been influenced by my comment here: #11348 (comment). See the original post in that issue for motivation.

My view is that listwallets was announced as experimental in v0.15 (along with all the other multiwallet RPCs) so we have a bit more licence to make breaking changes than normal. I think this change should stay, but like @meshcollider says, there may be better ways to achieve it.

promag · 2017-10-09T10:12:57Z

src/wallet/rpcwallet.cpp

-    return obj;
+    UniValue available(UniValue::VARR);
+    fs::path walletdir = GetWalletDir();
+    for (fs::directory_iterator it(walletdir); it != fs::directory_iterator(); it++) {


promag · 2017-10-09T10:14:57Z

src/util.cpp

+    } else {
+        path = GetDataDir();
+        // If a wallets directory exists, use that, otherwise default to GetDataDir
+        if (fs::is_directory(path / "wallets")) {


When is this directory created?

It's not yet, it's on the to-do list ;)

promag · 2017-10-09T10:15:53Z

src/util.cpp

+    if (gArgs.IsArgSet("-walletdir")) {
+        path = fs::system_complete(gArgs.GetArg("-walletdir", ""));
+        if (!fs::is_directory(path)) {
+            path = "";


At the moment GetWalletDir is used in the startup and in runtime.

IMO, in runtime, this should not be possible. Create a bool IsValidWalletDir() (or whatever) to use in startup and here assert is_directory?

Yep I'll include something similar when reworking the directory caching stuff I think, datadir and walletdir shouldn't change throughout execution so no point rechecking every time

promag · 2017-10-09T10:19:25Z

src/wallet/rpcwallet.cpp

+    for (fs::directory_iterator it(walletdir); it != fs::directory_iterator(); it++) {
+        if (is_regular_file(*it)
+         && it->path().filename().string().length() > 4
+         && it->path().filename().string().substr(it->path().filename().string().length() - 4) == ".dat") {


This is kind of hard to read. Care to simplify?

promag · 2017-10-09T10:20:39Z

doc/release-notes.md

@@ -74,6 +74,13 @@ Due to a backward-incompatible change in the wallet database, wallets created
 with version 0.16.0 will be rejected by previous versions. Also, version 0.16.0
 will only create hierarchical deterministic (HD) wallets.

+Custom wallet directories


👏 to release notes.

promag · 2017-10-09T10:21:07Z

doc/release-notes.md

+---------------------
+The ability to specify a directory other than the default data directory in which to store
+wallets has been added. An existing directory can be specified using the `-walletdir=<dir>`
+argument. The parameter defaults to using the data directory location if not included.


Missing datadir/wallets case?

Will add when implemented 👍

promag · 2017-10-09T10:28:02Z

src/init.cpp

@@ -367,6 +367,7 @@ std::string HelpMessage(HelpMessageMode mode)
    strUsage += HelpMessageOpt("-sysperms", _("Create new files with system default permissions, instead of umask 077 (only effective with disabled wallet functionality)"));
 #endif
    strUsage += HelpMessageOpt("-txindex", strprintf(_("Maintain a full transaction index, used by the getrawtransaction rpc call (default: %u)"), DEFAULT_TXINDEX));
+    strUsage += HelpMessageOpt("-walletdir=<dir>", _("Specify wallet directory"));


Specify directory to hold wallets?

promag · 2017-10-09T10:28:29Z

doc/release-notes.md

+Custom wallet directories
+---------------------
+The ability to specify a directory other than the default data directory in which to store
+wallets has been added. An existing directory can be specified using the `-walletdir=<dir>`


Nit, should be -walletsdir (plural)?

I prefer walletdir, no real difference though imo

walletsdir seems like something I'd fat finger into walletdir every time, prefer to leave out the extra letter :)

jonasschnelli · 2017-10-09T19:55:20Z

I haven't looked at the code, but how is this interacting with the multi-wallet-mode?
Does it allow a directory per wallet?

meshcollider · 2017-10-09T20:51:17Z

@jonasschnelli no, all wallets would be inside the walletdir specified

meshcollider · 2017-10-10T06:12:08Z

Addressed @promag nits, will work on the directory creation stuff as soon as I can

jnewbery

This looks like a good start. I've add a few comments inline. Some more high-level feedback:

The GetWalletDir() is currently in utils.cpp, which is part of the libbitcoin_util module. I think it makes more sense to move it to libbitcoin_wallet, since none of the other modules (server, qt, etc) should need to know about it.
On startup, I think that if this is the first run AND there is no -walletdir, then bitcoind should create the wallet dir in the datadir (ie fresh installs should default to using the datadir/wallet structure).

jnewbery · 2017-10-10T17:04:24Z

src/wallet/rpcwallet.cpp

-            "  \"walletname\"            (string) the wallet name\n"
-            "   ...\n"
-            "]\n"
+            "{\n"


This change may have been influenced by my comment here: #11348 (comment). See the original post in that issue for motivation.

My view is that listwallets was announced as experimental in v0.15 (along with all the other multiwallet RPCs) so we have a bit more licence to make breaking changes than normal. I think this change should stay, but like @meshcollider says, there may be better ways to achieve it.

jnewbery · 2017-10-10T17:07:36Z

src/init.cpp

@@ -367,6 +367,7 @@ std::string HelpMessage(HelpMessageMode mode)
    strUsage += HelpMessageOpt("-sysperms", _("Create new files with system default permissions, instead of umask 077 (only effective with disabled wallet functionality)"));
 #endif
    strUsage += HelpMessageOpt("-txindex", strprintf(_("Maintain a full transaction index, used by the getrawtransaction rpc call (default: %u)"), DEFAULT_TXINDEX));
+    strUsage += HelpMessageOpt("-walletdir=<dir>", _("Specify directory to hold wallets"));


I think this help text should go in GetWalletHelpString() in src/wallet/init.cpp

jnewbery · 2017-10-10T17:09:29Z

src/init.cpp

@@ -1224,6 +1225,7 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
        LogPrintf("Startup time: %s\n", DateTimeStrFormat("%Y-%m-%d %H:%M:%S", GetTime()));
    LogPrintf("Default data directory %s\n", GetDefaultDataDir().string());
    LogPrintf("Using data directory %s\n", GetDataDir().string());
+    LogPrintf("Using wallet directory %s\n", GetWalletDir().string());


Again, I think this log should go in the wallet module rather than server.

jnewbery · 2017-10-10T17:14:43Z

src/util.cpp

+    if (gArgs.IsArgSet("-walletdir")) {
+        path = fs::system_complete(gArgs.GetArg("-walletdir", ""));
+        if (!fs::is_directory(path)) {
+            path = "";


I don't love this. At the moment you're using path = "" to essentially return an error to the caller (since on start-up, the calling function checks that the returned fs::path is a directory). That's not at all clear from reading just this function.

Can you be more explicit about how the error is returned, either by explicitly returning bool or at the least by adding a comment here?

jnewbery · 2017-10-10T17:19:13Z

src/bitcoin-cli.cpp

@@ -105,6 +105,10 @@ static int AppInitRPC(int argc, char* argv[])
        fprintf(stderr, "Error: Specified data directory \"%s\" does not exist.\n", gArgs.GetArg("-datadir", "").c_str());
        return EXIT_FAILURE;
    }
+    if (gArgs.IsArgSet("-walletdir") && !fs::is_directory(GetWalletDir())) {


Is this necessary? Where is the wallet name used in bitcoin-cli.cpp?

jnewbery · 2017-10-10T17:40:42Z

test/functional/multiwallet.py

@@ -15,30 +15,41 @@ class MultiWalletTest(BitcoinTestFramework):
    def set_test_params(self):
        self.setup_clean_chain = True
        self.num_nodes = 1
-        self.extra_args = [['-wallet=w1', '-wallet=w2', '-wallet=w3']]
+        self.extra_args = [['-wallet=w1.dat', '-wallet=w2.dat', '-wallet=w3.dat']]


Wallet names are currently allowed to not end with .dat. This test verified that we don't regress that support.

I'm not sure if we should go on supporting that, but we should discuss that before removing support.

Agreed, I'm unsure if there are any real benefits of adding an available list to the listwallets RPC after all, or if it just complicates the PR. Will discuss on IRC and then remove if not.

jnewbery · 2017-10-10T17:42:52Z

test/functional/multiwallet.py

+        # running the node with specified walletdir should only have the default wallet in it
+        os.mkdir(os.path.join(self.options.tmpdir, 'node0', 'regtest', 'walletdir'))
+        self.start_node(0, ['-walletdir=' + os.path.join(self.options.tmpdir, 'node0', 'regtest', 'walletdir')])
+        assert_equal(set(self.nodes[0].listwallets()['loaded']), {"wallet.dat"})


This test should also test having multiple wallets in the walletdir, and issuing an RPC command to one of them.

jnewbery · 2017-10-10T17:51:40Z

doc/release-notes.md

+The ability to specify a directory other than the default data directory in which to store
+wallets has been added. An existing directory can be specified using the `-walletdir=<dir>`
+argument. The parameter defaults to using the data directory location if not included.
+Wallets loaded via `-wallet` arguments are relative to this wallet directory.


relative to this wallet directory or must be in this wallet directory

jnewbery · 2017-10-10T17:53:37Z

doc/release-notes.md

+The ability to specify a directory other than the default data directory in which to store
+wallets has been added. An existing directory can be specified using the `-walletdir=<dir>`
+argument. The parameter defaults to using the data directory location if not included.
+Wallets loaded via `-wallet` arguments are relative to this wallet directory.


I'm not sure, but perhaps we should add a warning that -walletdir must refer to a reliable directory in the filesystem. For example, I think we want to discourage people using ephemeral, removable storage for their -walletdir since if the directory becomes unavailable during operation, funds may be lost.

jnewbery · 2017-10-10T17:55:19Z

src/wallet/init.cpp

@@ -208,7 +208,7 @@ bool VerifyWallets()
        }

        std::string strError;
-        if (!CWalletDB::VerifyEnvironment(walletFile, GetDataDir().string(), strError)) {
+        if (!CWalletDB::VerifyEnvironment(walletFile, GetWalletDir().string(), strError)) {


The arguments in the declaration and definition for VerifyEnvironment() and VerifyDatabaseFile() are still dataDir. Those should be updated to indicate that they're now refering to the wallet directory.

meshcollider · 2017-10-11T11:15:49Z

I've split the listwallets RPC changes out into #11485. Squashed nit-fix commits, reworked the datadir caching stuff. Needs some careful testing and review now but I think its ready for proper review, let's see how travis goes. Will fix stuff in the morning.

jnewbery · 2017-10-11T14:23:25Z

It might eventually make sense to have each wallet in each own directory, ie the structure would be:

/<dataDir>
  .
  .-/wallets
      .
      .-/wallet1
      .   .
      .   .-wallet1.dat
      .   .-db.log
      .   ./database
      .
      .-/wallet2
      .   ....
      ....

That way, every wallet has its own bdb environment and we avoid nasty surprises like #11429 (comment).

That might involve a large code change, which would be necessary anyway if we want to separate wallets into their own processes.

I don't think that has any impact on this PR right now, but it's worth considering what we want the final structure to look like.

promag · 2017-10-11T14:34:28Z

Since we are relying on filesystem then 👍 to @jnewbery suggestion.

meshcollider · 2017-10-13T22:47:31Z

Rebased

jnewbery

Looks good. A few minor nits inline.

This PR could do with some judicious squashing. I'm not a fan of PRs adding and then removing code in later commits. Consider:

squashing commit 'Move GetWalletDir to new walletutil file' into the initial 'Add -walletdir parameter to specify custom wallet dir' commit
squashing the two release notes commits together.
squashing 'Fix tests and test wallets/ subdir behaviour' into the 'Create walletdir if datadir doesn't exist' commit (test/code change atomicity is good because it doesn't break git bisect)

jnewbery · 2017-10-24T14:58:17Z

src/wallet/db.cpp

 {
    LogPrintf("Using BerkeleyDB version %s\n", DbEnv::version(0, 0, 0));
    LogPrintf("Using wallet %s\n", walletFile);

    // Wallet file must be a plain filename without a directory
    if (walletFile != fs::basename(walletFile) + fs::extension(walletFile))
    {
-        errorStr = strprintf(_("Wallet %s resides outside data directory %s"), walletFile, dataDir.string());
+        errorStr = strprintf(_("Wallet %s resides outside data directory %s"), walletFile, walletDir.string());


This error message still says 'data directory'. The whole error message is a bit confusing, and I think it's rendered obsolete by the additional error checking at

bitcoin/src/wallet/init.cpp

Line 193 in 57ee739

return InitError(strprintf(_("Error loading wallet %s. -wallet parameter must only specify a filename (not a path)."), walletFile));

. Perhaps @jonasschnelli can confirm?

jnewbery · 2017-10-24T15:21:05Z

test/functional/multiwallet.py

@@ -26,13 +26,34 @@ def run_test(self):
        self.assert_start_raises_init_error(0, ['-wallet=w1', '-wallet=w1'], 'Error loading wallet w1. Duplicate -wallet filename specified.')

        # should not initialize if wallet file is a directory
-        os.mkdir(os.path.join(self.options.tmpdir, 'node0', 'regtest', 'w11'))
+        os.mkdir(os.path.join(self.options.tmpdir, 'node0', 'regtest', 'wallets', 'w11'))


Perhaps make some local variables here rather than repeating the same calls to os.path.join():

wallet_dir = os.path.join(self.options.tmpdir, 'node0', 'regtest', 'wallets') ... wallet_dir2 = os.path.join(self.options.tmpdir, 'node0', 'regtest', 'walletdir')

or whatever

jnewbery · 2017-10-24T15:23:48Z

src/wallet/walletutil.cpp

@@ -0,0 +1,28 @@
+// Copyright (c) 2009-2010 Satoshi Nakamoto


Are you Satoshi? 🙂 If not, I guess you can remove this copyright line (and the same from walletutil.h)

meshcollider · 2017-10-27T02:25:10Z

Fixed @jnewbery's nits and squashed as suggested. Thanks :)

maflcko · 2017-10-28T18:22:01Z

Concept ACK c36cb54711fe7677db6efc4db00a6e7d42e62f8d

ryanofsky · 2017-10-31T18:20:06Z

Adding a -walletdir option seems more cumbersome than just tweaking the existing -wallet option to allow directory separator characters (\/).

Advantages of allowing -wallet directory paths instead of adding -walletdir option:

Simpler for users: they don't need to look at 3 different options (-wallet, -walletdir, -datadir) to figure out where wallet will be stored. They just need to look at 1 or 2 (-wallet and -datadir if wallet path is not absolute).
More flexible: Users can store wallets in different locations.
Doesn't add 100 lines of code.
100% backwards compatible. New bitcoin releases continue to store wallet.dat in same places releases.

Disadvantages of allowing -wallet directory paths instead of adding -walletdir option:

Because users are not forced to store all wallets in single directory, we lose the ability to have a listwallets RPC that can return a complete list of all possible wallets (though it's still possible to list wallets in datadir).

Is this a fair analysis or am I missing something?

sipa · 2017-10-31T18:22:10Z

Supporting multiple wallet directories also requires support for multiple BDB environments first, which would be extra code complication.

ryanofsky · 2017-10-31T18:54:42Z

Supporting multiple wallet directories also requires support for multiple BDB environments first, which would be extra code complication.

Oh right, that is the thing I forgot. Allowing -wallet paths would be a little more complicated to implement than I was suggesting, but not much more. And this by itself doesn't seem like a good enough reason to make user experience worse and add complexity that we will need to support into the future.

meshcollider · 2017-11-01T09:07:00Z

@ryanofsky 100% backwards compatible. New bitcoin releases continue to store wallet.dat in same places releases.

Note that this PR doesn't rely on moving the default directory location, that was just suggested as a good idea in #11348 (that was the main point of that issue). So that point is orthogonal to -walletdir or a relative -wallet, and probably should still be done

maflcko · 2017-11-01T15:12:20Z

... or a relative -wallet, and probably should still be done

I don't see how we could reasonably test behavior when -datadir, -walletdir and -wallet support directories as paths. Extending -wallet to accept file names, relative paths (to the datadir) or absolute paths should make the need to specify a -walletdir obsolete.

meshcollider · 2017-11-01T18:28:32Z

@MarcoFalke sorry I meant the moving of default walletdir to datadir/wallets/ not making it accept relative paths :)

maflcko · 2017-11-01T18:32:59Z

Yeah, right. Why would you change the default wallet dir when already could specify a path through -wallet?

meshcollider · 2017-11-01T21:24:18Z

Just to keep the wallets and their BDB environments contained in their own subdirectory, a cleanup and bit more separation. Would make it clearer to users what they should back up, for example, in the case of multiwallet. That's what #11348 was discussing

sipa · 2017-11-02T00:25:21Z

An alternative idea would be to not have a -walletdir option, but have it be implied from the -wallet arguments. In a first version, a requirement could exist that all wallet files are in the same directory (which would then become the db env directory).

luke-jr · 2017-11-10T14:42:58Z

IMO it's a bad idea to infer the wallet dir from the wallet filename. This would encourage confusion among users with regard to the db.log file and database directory - they'll wonder why they exist, possibly delete them, and/or not notice and neglect to back them up (although backups ought to always be made using Core, some users still don't know that).

luke-jr · 2017-11-10T14:45:34Z

src/util.cpp

+    if (fs::create_directories(path)) {
+        // This is the first run, create wallets subdirectory too
+        fs::create_directories(path / "wallets");
+    }


Here and above: We should never create the wallets directory if wallet support is disabled. Additionally, if we're going to default to a wallet subdir, we should create it when there is no wallet yet, regardless of whether the datadir exists or not (eg, if datadir exists, but wallet.dat does not, make and use the wallet subdir).

I had the same reaction when I first saw this, but I don't think it's a problem. If wallet support is disabled, we'll just have an empty wallet directory in the datadir.

I think it's fine to create an empty wallets directory, no matter if wallets are enabled or not (either at build or runtime). Aesthetically it would be better otherwise but practically it makes sense:

As the presence of this directory is used as signal where to put wallets, transitioning to it for an existing data directory is non-trivial. How would it even know if a legacy datadir has no wallets and you're creating the first one? That would be some nightmare heuristic logic...
Better to leave it to the user to create wallets themselves an move any relevant wallets there.

luke-jr · 2017-11-10T14:47:55Z

Perhaps another option would be to allow specifying a directory (ending in a /) for -wallet, and treat the entire directory as dedicated to that wallet, including naming the file itself wallet.dat unconditionally...?

promag · 2017-11-17T15:08:57Z

Tested ACK c1e5d40.

Nits and improvements to regtest and testnet can be left for followups?

ryanofsky · 2017-11-17T15:19:38Z

You guys should look at #11687, too. It works standalone, but combined with this PR, it basically implements what john suggested here: #11466 (comment)

It also has a number of other improvements. I'm currently fixing a crazy problem with the tests (they were only failing on travis and I spent hours yesterday trying to reproduce them locally before eventually succeeding this morning and finding a shutdown bug.) Luke-jr posted a very reasonable comment there objecting to an earlier version of the PR that didn't include the third commit, but I think that should be addressed now. I'm planning on posting an update there after I fix travis, but the PR is very well documented and includes detailed release notes that I think should be pretty clear.

laanwj · 2017-11-17T15:35:48Z

@ryanofsky will look at that one next

Nits and improvements to regtest and testnet can be left for followups?

Yes, fine with me, will leave it up to @meshcollider .

meshcollider · 2017-11-17T17:26:55Z

Yes, fine with me, will leave it up to @meshcollider .

Yep I'm happy to open a follow-up PR

jonasschnelli · 2017-11-17T18:41:19Z

doc/release-notes.md

+The ability to specify a directory other than the default data directory in which to store
+wallets has been added. An existing directory can be specified using the `-walletdir=<dir>`
+argument. Wallets loaded via `-wallet` arguments must be in this wallet directory. Care should be taken
+when choosing a wallet directory location, as if  it becomes unavailable during operation,


nit: double space (if it).

meshcollider · 2017-11-18T00:45:30Z

Nit cleanup and regtest/testnet change on new branch here: https://github.com/MeshCollider/bitcoin/tree/201711_walletdir
The second commit requires more testing so best if I leave them both for a new PR, approach can be discussed there too

c1e5d40 Make debugging test crash easier (MeshCollider) 8263f6a Create walletdir if datadir doesn't exist and fix tests (MeshCollider) 9587a9c Default walletdir is wallets/ if it exists (MeshCollider) d987889 Add release notes for -walletdir and wallets/ dir (MeshCollider) 80c5cbc Add test for -walletdir (MeshCollider) 0530ba0 Add -walletdir parameter to specify custom wallet dir (MeshCollider) Pull request description: Closes #11348 Adds a `-walletdir` parameter which specifies a directory to use for wallets, allowing them to be stored separately from the 'main' data directory. Creates a new `wallets/` directory in datadir if this is the first time running, and defaults to using it if it exists. Includes tests and release notes. Things which might need to be considered more: - there is no 'lock' on the wallets directory, which might be needed? - because this uses a new wallets/ directory by default, downgrading to an earlier version won't see the wallets in that directory (not a big deal though, users can just copy them up to the main dir) - jnewbery suggested putting each wallet in its own directory, which is a good idea, but out of scope for this PR IMO. EDIT: this is being done in #11687 - doc/files.md needs updating (will do soon) I also considered including a cleanup by removing caching of data directory paths and instead just initialise them once on startup (c.f. #3073), but decided it wasn't super relevant here will just complicate review. Tree-SHA512: c8ac04bfe9a810c32055f2c8b8fa0d535e56125ceb8d96f12447dd3538bf3e5ee992b60b1cd2173bf5f3fa023a9feab12c9963593bf27ed419df929bb413398d

@promag

aac6b3f Update files.md for new wallets/ subdirectory (MeshCollider) b673429 Cleanups for walletdir PR (MeshCollider) Pull request description: This addresses the remaining nits from #11466 - Updates `doc/files.md` with respect to the new default wallet directory - Fixes @promag and @laanwj's error message nit, and Jonas' release notes nit - ~Addresses @laanwj's net-specific wallet subdirectory concern in the case that a walletdir is specified~ - Changes the #includes from "" to <> style after #11651 Tree-SHA512: b86bf5fdc4de54c1b0f65b60a83af3cf82b35d216ce9c0de724803bfba6934796238b6c412659dcc29ae2e3e856d4eb97ae777c80f36f4089d8acecfddefe9aa

Reasons for rename: 1) Default value is `<datadir>/wallets` so calling it `-walletsdir` instead of `-walletdir` would be more internally consistent 2) Directory can contain more than one wallet so plural makes more sense 3) This makes it harder to confuse -walletdir option with -wallet option if we store wallets in their own directories as proposed in bitcoin#11466 (comment) and implemented in bitcoin#12216 -BEGIN VERIFY SCRIPT- git grep -l walletdir | xargs sed -i s/walletdir/walletsdir/g -END VERIFY SCRIPT-

@promag

aac6b3f067 Update files.md for new wallets/ subdirectory (MeshCollider) b67342906c Cleanups for walletdir PR (MeshCollider) Pull request description: This addresses the remaining nits from bitcoin/bitcoin#11466 - Updates `doc/files.md` with respect to the new default wallet directory - Fixes @promag and @laanwj's error message nit, and Jonas' release notes nit - ~Addresses @laanwj's net-specific wallet subdirectory concern in the case that a walletdir is specified~ - Changes the #includes from "" to <> style after #11651 Tree-SHA512: b86bf5fdc4de54c1b0f65b60a83af3cf82b35d216ce9c0de724803bfba6934796238b6c412659dcc29ae2e3e856d4eb97ae777c80f36f4089d8acecfddefe9aa

…param c1e5d40 Make debugging test crash easier (MeshCollider) 8263f6a Create walletdir if datadir doesn't exist and fix tests (MeshCollider) 9587a9c Default walletdir is wallets/ if it exists (MeshCollider) d987889 Add release notes for -walletdir and wallets/ dir (MeshCollider) 80c5cbc Add test for -walletdir (MeshCollider) 0530ba0 Add -walletdir parameter to specify custom wallet dir (MeshCollider) Pull request description: Closes bitcoin#11348 Adds a `-walletdir` parameter which specifies a directory to use for wallets, allowing them to be stored separately from the 'main' data directory. Creates a new `wallets/` directory in datadir if this is the first time running, and defaults to using it if it exists. Includes tests and release notes. Things which might need to be considered more: - there is no 'lock' on the wallets directory, which might be needed? - because this uses a new wallets/ directory by default, downgrading to an earlier version won't see the wallets in that directory (not a big deal though, users can just copy them up to the main dir) - jnewbery suggested putting each wallet in its own directory, which is a good idea, but out of scope for this PR IMO. EDIT: this is being done in bitcoin#11687 - doc/files.md needs updating (will do soon) I also considered including a cleanup by removing caching of data directory paths and instead just initialise them once on startup (c.f. dashpay#3073), but decided it wasn't super relevant here will just complicate review. Tree-SHA512: c8ac04bfe9a810c32055f2c8b8fa0d535e56125ceb8d96f12447dd3538bf3e5ee992b60b1cd2173bf5f3fa023a9feab12c9963593bf27ed419df929bb413398d

@promag

aac6b3f Update files.md for new wallets/ subdirectory (MeshCollider) b673429 Cleanups for walletdir PR (MeshCollider) Pull request description: This addresses the remaining nits from bitcoin#11466 - Updates `doc/files.md` with respect to the new default wallet directory - Fixes @promag and @laanwj's error message nit, and Jonas' release notes nit - ~Addresses @laanwj's net-specific wallet subdirectory concern in the case that a walletdir is specified~ - Changes the #includes from "" to <> style after bitcoin#11651 Tree-SHA512: b86bf5fdc4de54c1b0f65b60a83af3cf82b35d216ce9c0de724803bfba6934796238b6c412659dcc29ae2e3e856d4eb97ae777c80f36f4089d8acecfddefe9aa

@promag

aac6b3f Update files.md for new wallets/ subdirectory (MeshCollider) b673429 Cleanups for walletdir PR (MeshCollider) Pull request description: This addresses the remaining nits from bitcoin#11466 - Updates `doc/files.md` with respect to the new default wallet directory - Fixes @promag and @laanwj's error message nit, and Jonas' release notes nit - ~Addresses @laanwj's net-specific wallet subdirectory concern in the case that a walletdir is specified~ - Changes the #includes from "" to <> style after bitcoin#11651 Tree-SHA512: b86bf5fdc4de54c1b0f65b60a83af3cf82b35d216ce9c0de724803bfba6934796238b6c412659dcc29ae2e3e856d4eb97ae777c80f36f4089d8acecfddefe9aa

…param c1e5d40 Make debugging test crash easier (MeshCollider) 8263f6a Create walletdir if datadir doesn't exist and fix tests (MeshCollider) 9587a9c Default walletdir is wallets/ if it exists (MeshCollider) d987889 Add release notes for -walletdir and wallets/ dir (MeshCollider) 80c5cbc Add test for -walletdir (MeshCollider) 0530ba0 Add -walletdir parameter to specify custom wallet dir (MeshCollider) Pull request description: Closes bitcoin#11348 Adds a `-walletdir` parameter which specifies a directory to use for wallets, allowing them to be stored separately from the 'main' data directory. Creates a new `wallets/` directory in datadir if this is the first time running, and defaults to using it if it exists. Includes tests and release notes. Things which might need to be considered more: - there is no 'lock' on the wallets directory, which might be needed? - because this uses a new wallets/ directory by default, downgrading to an earlier version won't see the wallets in that directory (not a big deal though, users can just copy them up to the main dir) - jnewbery suggested putting each wallet in its own directory, which is a good idea, but out of scope for this PR IMO. EDIT: this is being done in bitcoin#11687 - doc/files.md needs updating (will do soon) I also considered including a cleanup by removing caching of data directory paths and instead just initialise them once on startup (c.f. dashpay#3073), but decided it wasn't super relevant here will just complicate review. Tree-SHA512: c8ac04bfe9a810c32055f2c8b8fa0d535e56125ceb8d96f12447dd3538bf3e5ee992b60b1cd2173bf5f3fa023a9feab12c9963593bf27ed419df929bb413398d

@promag

aac6b3f Update files.md for new wallets/ subdirectory (MeshCollider) b673429 Cleanups for walletdir PR (MeshCollider) Pull request description: This addresses the remaining nits from bitcoin#11466 - Updates `doc/files.md` with respect to the new default wallet directory - Fixes @promag and @laanwj's error message nit, and Jonas' release notes nit - ~Addresses @laanwj's net-specific wallet subdirectory concern in the case that a walletdir is specified~ - Changes the #includes from "" to <> style after bitcoin#11651 Tree-SHA512: b86bf5fdc4de54c1b0f65b60a83af3cf82b35d216ce9c0de724803bfba6934796238b6c412659dcc29ae2e3e856d4eb97ae777c80f36f4089d8acecfddefe9aa

…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 Oct 9, 2017

promag reviewed Oct 9, 2017

View reviewed changes

jnewbery mentioned this pull request Oct 9, 2017

rfc: Wallets should be in their own directory #11348

Closed

jnewbery reviewed Oct 10, 2017

View reviewed changes

meshcollider changed the title ~~[WIP] Specify custom wallet directory with -walletdir param~~ Specify custom wallet directory with -walletdir param Oct 11, 2017

meshcollider mentioned this pull request Oct 11, 2017

Add available field to listwallets RPC #11485

Closed

jnewbery reviewed Oct 24, 2017

View reviewed changes

luke-jr reviewed Nov 10, 2017

View reviewed changes

meshcollider mentioned this pull request Nov 14, 2017

External wallet files #11687

Merged

ryanofsky mentioned this pull request Nov 15, 2017

added -walletallowsymboliclink (default false) #11343

Closed

jonasschnelli reviewed Nov 17, 2017

View reviewed changes

laanwj merged commit c1e5d40 into bitcoin:master Nov 18, 2017

meshcollider deleted the 201710_walletdir branch November 19, 2017 00:47

meshcollider mentioned this pull request Nov 19, 2017

Cleanups + nit fixes for walletdir PR #11726

Merged

TheBlueMatt mentioned this pull request Dec 13, 2017

Prevent Opening Wallets Simultaneously in Two Instances #11888

Closed

ajtowns mentioned this pull request Dec 21, 2017

SegWit wallet support #11403

Merged

ryanofsky mentioned this pull request Jan 11, 2018

[docs] Clarify -walletdir usage #12166

Merged

ryanofsky mentioned this pull request Jan 18, 2018

RFC: Rename -walletdir option to -walletsdir (scripted-diff) #12221

Closed

promag mentioned this pull request Mar 21, 2019

-wallet is a "network-only" option but -walletdir is not #15630

Closed

markblundeberg mentioned this pull request Jun 15, 2019

odd behaviour of GetDataDir creating wallets/ subdirectory #16220

Closed

n-thumann mentioned this pull request Oct 3, 2020

bitcoin-cli needlessly creates empty ~/.bitcoin/wallets directory #20070

Closed

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

Specify custom wallet directory with -walletdir param #11466

Specify custom wallet directory with -walletdir param #11466

Uh oh!

Conversation

meshcollider commented Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshcollider Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Oct 9, 2017

Uh oh!

meshcollider commented Oct 9, 2017

Uh oh!

meshcollider commented Oct 10, 2017

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

meshcollider commented Oct 9, 2017 •

edited

Loading

meshcollider Oct 9, 2017 •

edited

Loading

promag Oct 10, 2017 •

edited

Loading

meshcollider commented Oct 11, 2017 •

edited

Loading