[docs] Clarify -walletdir usage #12166

jnewbery · 2018-01-11T22:14:02Z

After discussion with @ryanofsky around #11687 , I think this documentation is a bit clearer for how the new -walletdir argument works.

ryanofsky · 2018-01-11T22:33:16Z

Attn @meshcollider, this is trying to clarify some things in the release notes you wrote for #11466.

Note that this documentation doesn't really have anything at all to do with 11687. There was just some discussion about #11466 that spilled into the comments there:

#11687 (comment)
#11687 (comment)
#11687 (comment)
#11687 (review)

ryanofsky · 2018-01-11T22:36:58Z

doc/release-notes.md

+
+Bitcoin Core now has more flexibility in where the wallets directory can be located.
+
+- For new installations (where the data directory doesn't already exist),


I would add something to ground this in existing behavior. Maybe say "Previously wallet database files were stored at the top level of the bitcoin data directory. Now for new installations [...]"

Good suggestion. Added.

ryanofsky · 2018-01-11T22:38:13Z

doc/release-notes.md

+  `wallets/` subdirectory by default.
+- The location of the wallets directory can be overridden by specifying a
+  `-walletdir=<path>` option where `<path>` can be an absolute path or a
+  relative path (relative to the current working directory). `<path>` can


Relative to the data directory, not the current working directory, I believe.

No, I've retested and it's relative to the cwd. See

bitcoin/src/wallet/walletutil.cpp

Line 12 in 44080a9

path = fs::system_complete(gArgs.GetArg("-walletdir", ""));

.

This is part of what prompted my comment here: #11687 (comment) - it's slightly confusing that -walletdir relative paths are relative to the cwd and -wallet paths are relative to the walletdir. That's also why I think #11687 can't be thought of as being completely independent from the changes in #11466 . The documentation needs to describe clearly the interactions between the two options.

Attn @meshcollider

it's slightly confusing that -walletdir relative paths are relative to the cwd

This is worse than confusing. This sounds like a mistake that should be fixed. Current working directory is helpful thing to rely on in scripts and command line tools, but long running apps and daemons should not depend on the current working directory. If a relative -walletdir path is specified in bitcoin.conf, it would be footgun for that directory to point to a completely different location if one day the user happens to be in a different directory when starting up bitcoin.

I think a new PR should be opened to either forbid relative -walletdir paths, or interpret them stably relative to <datadir>.

The -walletdir is relative in the same way -datadir is, I think that consistency is less confusing than making walletdir relative to datadir. Keep in mind an error will be thrown if the wallet directory doesn't exist so it's unlikely to start if the cwd was different

I don't have a strong preference on how this works. As @meshcollider says, the relativity of -walletdir is consistent with -datadir.

Any approach (make walletdir relative to datadir, make walletdir relative to cwd, or disallow relative paths for walletdir) seems fine to me as long as it's clearly documented.

I've noticed, on occasion, that people ask "Where is my wallet.dat file stored?" I wonder if this "cwd" default behaviour might be a culprit. I certainly favour clearer documentation, and I wonder if perhaps it could be helpful to raise an informational notice indicating the full path when a new wallet file is created.

@randolf this is unrelated, this change has not been in any release yet, it will be released in 0.16 :)

meshcollider · 2018-01-15T20:17:56Z

Looks good to me, ACK 97c3cad

Sjors · 2018-01-17T17:11:28Z

Tested ACK 97c3cad

jonasschnelli

ACK 97c3cad

@ryanofsky

97c3cad [docs] Clarify -walletdir usage (John Newbery) Pull request description: After discussion with @ryanofsky around #11687 , I think this documentation is a bit clearer for how the new `-walletdir` argument works. Tree-SHA512: f279cab82524dbc0d75e6f9891f0e228ec4c8d0df3e16f351057fa243ddd263ff786f05383fd00a09b89edcc07dab211be5b64387f77271edf8af0177bcf667d

maflcko · 2018-01-18T20:12:37Z

Post-merge ACK 97c3cad

…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 Docs label Jan 11, 2018

ryanofsky reviewed Jan 11, 2018

View reviewed changes

[docs] Clarify -walletdir usage

97c3cad

jnewbery force-pushed the clarify_walletdir_usage branch from b5075fe to 97c3cad Compare January 15, 2018 15:26

Sjors mentioned this pull request Jan 18, 2018

External wallet files #11687

Merged

ryanofsky mentioned this pull request Jan 18, 2018

Error if relative -walletdir is specified #12220

Merged

jonasschnelli approved these changes Jan 18, 2018

View reviewed changes

laanwj merged commit 97c3cad into bitcoin:master Jan 18, 2018

jnewbery deleted the clarify_walletdir_usage branch January 18, 2018 21:21

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[docs] Clarify -walletdir usage #12166

[docs] Clarify -walletdir usage #12166

jnewbery commented Jan 11, 2018

ryanofsky commented Jan 11, 2018

ryanofsky Jan 11, 2018

jnewbery Jan 15, 2018

ryanofsky Jan 11, 2018

jnewbery Jan 15, 2018

ryanofsky Jan 15, 2018 •

edited

meshcollider Jan 15, 2018

jnewbery Jan 15, 2018

randolf Jan 18, 2018 •

edited

meshcollider Jan 18, 2018

meshcollider commented Jan 15, 2018

Sjors commented Jan 17, 2018

jonasschnelli left a comment

maflcko commented Jan 18, 2018


		Bitcoin Core now has more flexibility in where the wallets directory can be located.

		- For new installations (where the data directory doesn't already exist),

[docs] Clarify -walletdir usage #12166

[docs] Clarify -walletdir usage #12166

Conversation

jnewbery commented Jan 11, 2018

ryanofsky commented Jan 11, 2018

ryanofsky Jan 11, 2018

Choose a reason for hiding this comment

jnewbery Jan 15, 2018

Choose a reason for hiding this comment

ryanofsky Jan 11, 2018

Choose a reason for hiding this comment

jnewbery Jan 15, 2018

Choose a reason for hiding this comment

ryanofsky Jan 15, 2018 • edited

Choose a reason for hiding this comment

meshcollider Jan 15, 2018

Choose a reason for hiding this comment

jnewbery Jan 15, 2018

Choose a reason for hiding this comment

randolf Jan 18, 2018 • edited

Choose a reason for hiding this comment

meshcollider Jan 18, 2018

Choose a reason for hiding this comment

meshcollider commented Jan 15, 2018

Sjors commented Jan 17, 2018

jonasschnelli left a comment

Choose a reason for hiding this comment

maflcko commented Jan 18, 2018

ryanofsky Jan 15, 2018 •

edited

randolf Jan 18, 2018 •

edited