Specify custom wallet directory with -walletdir param

meshcollider commented at 9:55 am on October 9, 2017: contributor

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.

fanquake added the label Wallet on Oct 9, 2017

in src/util.cpp:594 in f8dc7acb8c outdated

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

promag commented at 10:06 am on October 9, 2017:

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

meshcollider commented at 10:48 am on October 9, 2017:

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

in src/wallet/rpcwallet.cpp:2629 in f8dc7acb8c outdated

2625@@ -2626,17 +2626,16 @@ UniValue listwallets(const JSONRPCRequest& request)
2626             "Returns a list of currently loaded wallets.\n"
2627             "For full information on the wallet, use \"getwalletinfo\"\n"
2628             "\nResult:\n"
2629-            "[                         (json array of strings)\n"
2630-            "  \"walletname\"            (string) the wallet name\n"
2631-            "   ...\n"
2632-            "]\n"
2633+            "{\n"

promag commented at 10:12 am on October 9, 2017:

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

promag commented at 10:18 am on October 9, 2017:

Another interface is:

0{
1  "wallets": [
2    { "name": "foo", "loaded": true },
3    { "name": "bar", "loaded": false }
4  ]
5}

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

meshcollider commented at 10:54 am on October 9, 2017:

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

meshcollider commented at 6:16 am on October 10, 2017:

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

ryanofsky commented at 1:43 pm on October 10, 2017:

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

promag commented at 1:52 pm on October 10, 2017:

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.~~

jnewbery commented at 5:04 pm on October 10, 2017:

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.

in src/wallet/rpcwallet.cpp:2652 in f8dc7acb8c outdated

2649     }
2650 
2651-    return obj;
2652+    UniValue available(UniValue::VARR);
2653+    fs::path walletdir = GetWalletDir();
2654+    for (fs::directory_iterator it(walletdir); it != fs::directory_iterator(); it++) {

promag commented at 10:12 am on October 9, 2017:

++it.

in src/util.cpp:594 in f8dc7acb8c outdated

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

promag commented at 10:14 am on October 9, 2017:

When is this directory created?

meshcollider commented at 10:50 am on October 9, 2017:

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

in src/util.cpp:589 in f8dc7acb8c outdated

584+    fs::path path;
585+
586+    if (gArgs.IsArgSet("-walletdir")) {
587+        path = fs::system_complete(gArgs.GetArg("-walletdir", ""));
588+        if (!fs::is_directory(path)) {
589+            path = "";

promag commented at 10:15 am on October 9, 2017:

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?

meshcollider commented at 10:49 am on October 9, 2017:

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

jnewbery commented at 5:14 pm on October 10, 2017:

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?

in src/qt/bitcoin.cpp:629 in f8dc7acb8c outdated

625@@ -626,6 +626,12 @@ int main(int argc, char *argv[])
626                               QObject::tr("Error: Specified data directory \"%1\" does not exist.").arg(QString::fromStdString(gArgs.GetArg("-datadir", ""))));
627         return EXIT_FAILURE;
628     }
629+    if (gArgs.IsArgSet("-walletdir") && !fs::is_directory(GetWalletDir()))

promag commented at 10:16 am on October 9, 2017:

{ in this line.

in src/wallet/rpcwallet.cpp:2655 in f8dc7acb8c outdated

2652+    UniValue available(UniValue::VARR);
2653+    fs::path walletdir = GetWalletDir();
2654+    for (fs::directory_iterator it(walletdir); it != fs::directory_iterator(); it++) {
2655+        if (is_regular_file(*it)
2656+         && it->path().filename().string().length() > 4
2657+         && it->path().filename().string().substr(it->path().filename().string().length() - 4) == ".dat") {

promag commented at 10:19 am on October 9, 2017:

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

in doc/release-notes.md:65 in f8dc7acb8c outdated

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

promag commented at 10:20 am on October 9, 2017:

👏 to release notes.

in doc/release-notes.md:81 in f8dc7acb8c outdated

73@@ -74,6 +74,13 @@ Due to a backward-incompatible change in the wallet database, wallets created
74 with version 0.16.0 will be rejected by previous versions. Also, version 0.16.0
75 will only create hierarchical deterministic (HD) wallets.
76 
77+Custom wallet directories
78+---------------------
79+The ability to specify a directory other than the default data directory in which to store
80+wallets has been added. An existing directory can be specified using the `-walletdir=<dir>`
81+argument. The parameter defaults to using the data directory location if not included.

promag commented at 10:21 am on October 9, 2017:

Missing datadir/wallets case?

meshcollider commented at 10:54 am on October 9, 2017:

Will add when implemented :+1:

in src/init.cpp:370 in f8dc7acb8c outdated

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

promag commented at 10:28 am on October 9, 2017:

Specify directory to hold wallets?

in doc/release-notes.md:68 in f8dc7acb8c outdated

73@@ -74,6 +74,13 @@ Due to a backward-incompatible change in the wallet database, wallets created
74 with version 0.16.0 will be rejected by previous versions. Also, version 0.16.0
75 will only create hierarchical deterministic (HD) wallets.
76 
77+Custom wallet directories
78+---------------------
79+The ability to specify a directory other than the default data directory in which to store
80+wallets has been added. An existing directory can be specified using the `-walletdir=<dir>`

promag commented at 10:28 am on October 9, 2017:

Nit, should be -walletsdir (plural)?

meshcollider commented at 10:52 am on October 9, 2017:

I prefer walletdir, no real difference though imo

laanwj commented at 11:07 am on November 17, 2017:

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

promag commented at 10:28 am on October 9, 2017: member

Concept ACK. Regardless of being WIP, some comments.

jonasschnelli commented at 7:55 pm on October 9, 2017: contributor

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 commented at 8:51 pm on October 9, 2017: contributor

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

meshcollider commented at 6:12 am on October 10, 2017: contributor

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

in src/init.cpp:370 in ead92c2d03 outdated

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

jnewbery commented at 5:07 pm on October 10, 2017:

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

in src/init.cpp:1228 in ead92c2d03 outdated

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

jnewbery commented at 5:09 pm on October 10, 2017:

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

in src/bitcoin-cli.cpp:108 in ead92c2d03 outdated

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

jnewbery commented at 5:19 pm on October 10, 2017:

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

in test/functional/multiwallet.py:18 in ead92c2d03 outdated

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

jnewbery commented at 5:40 pm on October 10, 2017:

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.

meshcollider commented at 5:03 am on October 11, 2017:

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.

in test/functional/multiwallet.py:42 in ead92c2d03 outdated

44+        self.assert_start_raises_init_error(0, ['-walletdir=bad'], 'Error: Specified wallet directory "bad" does not exist.')
45+
46+        # running the node with specified walletdir should only have the default wallet in it
47+        os.mkdir(os.path.join(self.options.tmpdir, 'node0', 'regtest', 'walletdir'))
48+        self.start_node(0, ['-walletdir=' + os.path.join(self.options.tmpdir, 'node0', 'regtest', 'walletdir')])
49+        assert_equal(set(self.nodes[0].listwallets()['loaded']), {"wallet.dat"})

jnewbery commented at 5:42 pm on October 10, 2017:

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

in doc/release-notes.md:82 in ead92c2d03 outdated

73@@ -74,6 +74,13 @@ Due to a backward-incompatible change in the wallet database, wallets created
74 with version 0.16.0 will be rejected by previous versions. Also, version 0.16.0
75 will only create hierarchical deterministic (HD) wallets.
76 
77+Custom wallet directories
78+---------------------
79+The ability to specify a directory other than the default data directory in which to store
80+wallets has been added. An existing directory can be specified using the `-walletdir=<dir>`
81+argument. The parameter defaults to using the data directory location if not included.
82+Wallets loaded via `-wallet` arguments are relative to this wallet directory.

jnewbery commented at 5:51 pm on October 10, 2017:

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

jnewbery commented at 5:53 pm on October 10, 2017:

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.

in src/wallet/init.cpp:227 in ead92c2d03 outdated

207@@ -208,7 +208,7 @@ bool VerifyWallets()
208         }
209 
210         std::string strError;
211-        if (!CWalletDB::VerifyEnvironment(walletFile, GetDataDir().string(), strError)) {
212+        if (!CWalletDB::VerifyEnvironment(walletFile, GetWalletDir().string(), strError)) {

jnewbery commented at 5:55 pm on October 10, 2017:

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 commented at 4:48 am on October 11, 2017:

Fixed

jnewbery commented at 6:00 pm on October 10, 2017: member

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).

meshcollider commented at 11:15 am on October 11, 2017: contributor

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.

meshcollider renamed this:
~~[WIP] Specify custom wallet directory with -walletdir param~~
Specify custom wallet directory with -walletdir param
on Oct 11, 2017

jnewbery commented at 2:23 pm on October 11, 2017: member

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

 0/<dataDir>
 1  .
 2  .-/wallets
 3      .
 4      .-/wallet1
 5      .   .
 6      .   .-wallet1.dat
 7      .   .-db.log
 8      .   ./database
 9      .
10      .-/wallet2
11      .   ....
12      ....

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 commented at 2:34 pm on October 11, 2017: member

Since we are relying on filesystem then :+1: to @jnewbery suggestion.

meshcollider commented at 10:47 pm on October 13, 2017: contributor

Rebased

in src/wallet/db.cpp:238 in 30daa93e64 outdated

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

jnewbery commented at 2:58 pm on October 24, 2017:

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 https://github.com/bitcoin/bitcoin/blob/57ee73990f1ce29916adfd99f93eae1ccea1a43b/src/wallet/init.cpp#L193 . Perhaps @jonasschnelli can confirm?

in test/functional/multiwallet.py:29 in 30daa93e64 outdated

25@@ -26,13 +26,34 @@ def run_test(self):
26         self.assert_start_raises_init_error(0, ['-wallet=w1', '-wallet=w1'], 'Error loading wallet w1. Duplicate -wallet filename specified.')
27 
28         # should not initialize if wallet file is a directory
29-        os.mkdir(os.path.join(self.options.tmpdir, 'node0', 'regtest', 'w11'))
30+        os.mkdir(os.path.join(self.options.tmpdir, 'node0', 'regtest', 'wallets', 'w11'))

jnewbery commented at 3:21 pm on October 24, 2017:

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

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

or whatever

in src/wallet/walletutil.cpp:1 in 30daa93e64 outdated

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

jnewbery commented at 3:23 pm on October 24, 2017:

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

jnewbery commented at 3:40 pm on October 24, 2017: member

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)

meshcollider commented at 2:25 am on October 27, 2017: contributor

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

MarcoFalke commented at 6:22 pm on October 28, 2017: member

Concept ACK c36cb54711fe7677db6efc4db00a6e7d42e62f8d

ryanofsky commented at 6:20 pm on October 31, 2017: member

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 commented at 6:22 pm on October 31, 2017: member

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

ryanofsky commented at 6:54 pm on October 31, 2017: member

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 commented at 9:07 am on November 1, 2017: contributor

@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

MarcoFalke commented at 3:12 pm on November 1, 2017: member

… 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 commented at 6:28 pm on November 1, 2017: contributor

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

MarcoFalke commented at 6:32 pm on November 1, 2017: member

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

meshcollider commented at 9:24 pm on November 1, 2017: contributor

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 commented at 0:25 am on November 2, 2017: member

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 commented at 2:42 pm on November 10, 2017: member

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).

in src/util.cpp:580 in c36cb54711 outdated

573@@ -574,7 +574,10 @@ const fs::path &GetDataDir(bool fNetSpecific)
574     if (fNetSpecific)
575         path /= BaseParams().DataDir();
576 
577-    fs::create_directories(path);
578+    if (fs::create_directories(path)) {
579+        // This is the first run, create wallets subdirectory too
580+        fs::create_directories(path / "wallets");
581+    }

luke-jr commented at 2:45 pm on November 10, 2017:

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).

jnewbery commented at 5:39 pm on November 10, 2017:

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.

laanwj commented at 10:46 am on November 17, 2017:

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 commented at 2:47 pm on November 10, 2017: member

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…?

laanwj commented at 9:47 am on November 17, 2017: member

I think we should go forward with this, instead of turning this into a big argument. walletdir might not be the perfect solution for everything (e.g. it cannot handle multiple wallets in different places), but for people storing their wallets on a different filesystem this is enough, and much safer than the dangerous symlink tricks they’re using currently.

And after @sipa’s idea, or #11687, -walletdir will just be the “default wallet directory”. But that will require implementing that in a safe way first (and needs a more longer testing/review cycle). This is a good first step.

meshcollider commented at 9:57 am on November 17, 2017: contributor

Rebased

in src/qt/intro.cpp:217 in aa9bbcf1eb outdated

213@@ -214,7 +214,10 @@ bool Intro::pickDataDirectory()
214             }
215             dataDir = intro.getDataDirectory();
216             try {
217-                TryCreateDirectories(GUIUtil::qstringToBoostPath(dataDir));
218+                if(TryCreateDirectories(GUIUtil::qstringToBoostPath(dataDir))) {

laanwj commented at 10:39 am on November 17, 2017:

u-nit: space after if

meshcollider commented at 10:57 am on November 17, 2017: contributor

@laanwj nit addressed, and travis is passing, so shall I squash the last two commits now?

in src/wallet/walletutil.cpp:1 in b15bd53015 outdated

0@@ -0,0 +1,27 @@
1+// Copyright (c) 2017 The Bitcoin Core developers

laanwj commented at 11:09 am on November 17, 2017:

Agree on creating a separate walletutil, this avoids picking up #ifdef WALLET_ENABLE logic in util.cpp.

in src/wallet/init.cpp:38 in b15bd53015 outdated

34@@ -34,6 +35,7 @@ std::string GetWalletHelpString(bool showDebug)
35     strUsage += HelpMessageOpt("-upgradewallet", _("Upgrade wallet to latest format on startup"));
36     strUsage += HelpMessageOpt("-wallet=<file>", _("Specify wallet file (within data directory)") + " " + strprintf(_("(default: %s)"), DEFAULT_WALLET_DAT));
37     strUsage += HelpMessageOpt("-walletbroadcast", _("Make the wallet broadcast transactions") + " " + strprintf(_("(default: %u)"), DEFAULT_WALLETBROADCAST));
38+    strUsage += HelpMessageOpt("-walletdir=<dir>", _("Specify directory to hold wallets"));

laanwj commented at 11:14 am on November 17, 2017:

Need information about the default here: <datadir>/wallets if it exists, otherwise <datadir>

in src/wallet/init.cpp:241 in b15bd53015 outdated

237@@ -230,7 +238,7 @@ bool VerifyWallets()
238         }
239 
240         std::string strWarning;
241-        bool dbV = CWalletDB::VerifyDatabaseFile(walletFile, GetDataDir().string(), strWarning, strError);
242+        bool dbV = CWalletDB::VerifyDatabaseFile(walletFile, GetWalletDir().string(), strWarning, strError);

laanwj commented at 11:21 am on November 17, 2017:

Seems overkill to call GetWalletDir() so many times in this function (VerifyWallets()), we could just call and store it once. But no, no need to fix in this PR, this is easier to review.

in src/wallet/db.cpp:288 in b15bd53015 outdated

282@@ -282,18 +283,18 @@ bool CDB::VerifyEnvironment(const std::string& walletFile, const fs::path& dataD
283         }
284 
285         // try again
286-        if (!bitdb.Open(dataDir)) {
287+        if (!bitdb.Open(walletDir)) {
288             // if it still fails, it probably means we can't even create the database env
289-            errorStr = strprintf(_("Error initializing wallet database environment %s!"), GetDataDir());
290+            errorStr = strprintf(_("Error initializing wallet database environment %s!"), GetWalletDir());

laanwj commented at 11:22 am on November 17, 2017:

You already have walletDir here, just log that?

in doc/release-notes.md:73 in bfacfbfff7 outdated

68+wallets has been added. An existing directory can be specified using the `-walletdir=<dir>`
69+argument. Wallets loaded via `-wallet` arguments must be in this wallet directory. Care should be taken
70+when choosing a wallet directory location, as if  it becomes unavailable during operation,
71+funds may be lost.
72+
73+On new installations (if the data directory doesn't exist), this parameter will default to a

laanwj commented at 11:24 am on November 17, 2017:

I think this is a very important change from the viewpoint of users (“where’s my wallet.dat?! all wiki articles say it’s in the data directory!”), and probably warrants its own section.

meshcollider commented at 11:30 am on November 17, 2017:

Agreed, although I would hope most users would be able to work out what the new “wallets/” directory was for ;)

laanwj commented at 11:31 am on November 17, 2017: member

Manually tested:

Current datadir (testnet) - ok, still uses existing wallet
Create new datadir (testnet) - ok, wallets is created and wallet.dat is there
Create new datadir (testnet), different -wallet=test.dat name - ok, wallets is created and test.dat is there
-choosedatadir with GUI, new datadir - ok, wallets is created and wallet.dat is there
-choosedatadir with GUI, re-select default datadir - ok, datadir is left as-is, uses existing wallet
-walletdir override to existing directory, and no existing wallet - ok, wallet is created and used in supplied directory
-walletdir override to non-existing directory - ok, gives non-existing error as expected, bitcoin-qt shuts down cleanly
-walletdir override to existing directory with wallet - ok, wallet is used as expected

Add -walletdir parameter to specify custom wallet dir 0530ba0eae

Add test for -walletdir 80c5cbc14f

Add release notes for -walletdir and wallets/ dir d9878890e4

Default walletdir is wallets/ if it exists 9587a9c12b

Create walletdir if datadir doesn't exist and fix tests 8263f6a5ac

Make debugging test crash easier c1e5d40e16

meshcollider commented at 11:52 am on November 17, 2017: contributor

Squashed fixup commits

laanwj commented at 11:52 am on November 17, 2017: member

Tested ACK c1e5d40

in src/qt/intro.cpp:219 in c1e5d40e16

213@@ -214,7 +214,10 @@ bool Intro::pickDataDirectory()
214             }
215             dataDir = intro.getDataDirectory();
216             try {
217-                TryCreateDirectories(GUIUtil::qstringToBoostPath(dataDir));
218+                if (TryCreateDirectories(GUIUtil::qstringToBoostPath(dataDir))) {
219+                    // If a new data directory has been created, make wallets subdirectory too
220+                    TryCreateDirectories(GUIUtil::qstringToBoostPath(dataDir) / "wallets");

promag commented at 2:18 pm on November 17, 2017:

The logic data_dir + “/wallets” exists in a couple of places. Doesn’t this also ignores -walletdir?

laanwj commented at 2:44 pm on November 17, 2017:

This is just for creating the initial wallets directory. If it’s not used it will stay empty.

It always creates a wallets directory under the data directory to make sure it isn’t recognized as a legacy data directory (with wallets at the top level) in case any wallets are ever added to it.

in src/wallet/walletutil.cpp:15 in c1e5d40e16

10+
11+    if (gArgs.IsArgSet("-walletdir")) {
12+        path = fs::system_complete(gArgs.GetArg("-walletdir", ""));
13+        if (!fs::is_directory(path)) {
14+            // If the path specified doesn't exist, we return the deliberately
15+            // invalid empty string.

promag commented at 2:21 pm on November 17, 2017:

Why? Couldn’t create it? Is it bad setting?

laanwj commented at 2:45 pm on November 17, 2017:

Unlike the datadir, walletdir is not created if it doesn’t exist. This throws an error in init:

0$ src/bitcoind -regtest -walletdir=/tmp/walletsdf
1Error: Error: Specified wallet directory "/tmp/walletsdf" does not exist.

That’s the right behavior, IMO. (nit: doubled Error:)

promag commented at 3:06 pm on November 17, 2017:

Nit, if /tmp/walletsdf is non-directory then the error could be different.

laanwj commented at 2:48 pm on November 17, 2017: member

There’s a small thing I’m unsure about here, which just came to me, which only affects testnet and regtest: do we want to subdir the -walletdir with the chain name (like done with datadir?)? I think this would make sense, because currently it’s impossible to specify walletdir in bitcoin.conf without running into conflicts. (this can be skipped if the wallets directory in the data directory is used, because it’s already under the chain-specific dir)

promag commented at 3:08 pm on November 17, 2017: member

Tested ACK c1e5d40.

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

ryanofsky commented at 3:19 pm on November 17, 2017: member

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 commented at 3:35 pm on November 17, 2017: member

@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 commented at 5:26 pm on November 17, 2017: contributor

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

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

in doc/release-notes.md:70 in c1e5d40e16

61@@ -62,6 +62,20 @@ Due to a backward-incompatible change in the wallet database, wallets created
62 with version 0.16.0 will be rejected by previous versions. Also, version 0.16.0
63 will only create hierarchical deterministic (HD) wallets.
64 
65+Custom wallet directories
66+---------------------
67+The ability to specify a directory other than the default data directory in which to store
68+wallets has been added. An existing directory can be specified using the `-walletdir=<dir>`
69+argument. Wallets loaded via `-wallet` arguments must be in this wallet directory. Care should be taken
70+when choosing a wallet directory location, as if  it becomes unavailable during operation,

jonasschnelli commented at 6:41 pm on November 17, 2017:

nit: double space (if it).

meshcollider commented at 0:45 am on November 18, 2017: contributor

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

laanwj merged this on Nov 18, 2017

laanwj closed this on Nov 18, 2017

laanwj referenced this in commit d080a7d503 on Nov 18, 2017

meshcollider deleted the branch on Nov 19, 2017

MarcoFalke referenced this in commit 604e08c83c on Dec 20, 2017

ryanofsky referenced this in commit 23fc59a941 on Jan 18, 2018

virtload referenced this in commit 95bf4bc379 on Apr 4, 2018

PastaPastaPasta referenced this in commit c68dc2bf5e on Jan 31, 2020

PastaPastaPasta referenced this in commit e2edefa22e on Feb 4, 2020

PastaPastaPasta referenced this in commit 2c7b29bac5 on Feb 9, 2020

UdjinM6 referenced this in commit 588f34c1ff on Feb 29, 2020

PastaPastaPasta referenced this in commit be7bf2e9d8 on Mar 4, 2020

ckti referenced this in commit d26cd3fea8 on Mar 28, 2021

ckti referenced this in commit 00d5d478ae on Mar 28, 2021

gades referenced this in commit 2ca7534241 on Jun 30, 2021

gades referenced this in commit fd48b7614e on Jun 30, 2021

furszy referenced this in commit b04e1f5a90 on Jul 23, 2021

DrahtBot locked this on Sep 8, 2021

Specify custom wallet directory with -walletdir param #11466