Error if relative -walletdir is specified #12220

ryanofsky · 2018-01-18T18:45:11Z

This makes it an error to explicitly specify a non-absolute -walletdir path, and also adds a debug.log warning if a relative rather than absolute -datadir path is configured.

Specifying paths relative to the current working directory in a daemon process can be dangerous, because files can fail to be located even if the configuration doesn't change, but the daemon is started up differently.

Specifying a relative -datadir now adds a warning to the debug log. It would not be backwards-compatible to forbid relative -datadir paths entirely, and it could also be inconvenient for command line testing.

Specifying a relative -walletdir now results in a startup error. But since the -walletdir option is new in 0.16.0, there should be no compatibility issues. Another reason not to use working directory paths for -walletdir specifically is that the default -walletdir is a "wallets" subdirectory inside the datadir, so it could be surprising that setting -walletdir manually would choose a directory rooted in a completely different location.

laanwj · 2018-01-18T18:49:17Z

IMO, -walletdir should be relative to datadir if a relative path is provided. This is the case with many other path options. Isn't this the case here?

I do completely agree that arguments to daemons should never be relative to the current directory.

ryanofsky · 2018-01-18T18:51:30Z

IMO, -walletdir should be relative to datadir if a relative path is provided. This is the case with many other path options. Isn't this the case here?

I agree and I thought this was the case but it turns out it not to be: #12166 (comment). It could be changed, but @jnewbery and @meshcollider pointed out this would be treating -walletdir inconsistently from -datadir.

meshcollider · 2018-01-18T19:05:14Z

Note that if the wallet directory doesnt exist an error will be given anyway so the risk of accidently using a different directory is minimised

jnewbery · 2018-01-18T19:55:06Z

As noted here: #12166 (comment), I don't have a strong preference on this if it's changed before v0.16.

I don't think we should change this after v0.16 branches.

laanwj · 2018-01-18T20:00:37Z

this would be treating -walletdir inconsistently from -datadir.

That's true. I'm ok with it just failing with relative paths, too.

maflcko · 2018-01-18T20:08:23Z

Concept ACK. Being more strict seems safer, as explained in OP.

Also warn if bitcoind is configured to use a relative -datadir path. Specifying paths relative to the current working directory in a daemon process can be dangerous, because files can fail to be located even if the configuration doesn't change, but the daemon is started up differently. Specifying a relative -datadir now adds a warning to the debug log. It would not be backwards-compatible to forbid relative -datadir paths entirely, and it could also be also inconvenient for command line testing. Specifying a relative -walletdir now results in a startup error. But since the -walletdir option is new in 0.16.0, there should be no compatibility issues. Another reason not to use working directory paths for -walletdir specifically is that the default -walletdir is a "wallets" subdirectory inside the datadir, so it could be surprising that setting -walletdir manually would choose a directory rooted in a completely different location.

jnewbery · 2018-01-18T20:11:04Z

Release notes should be updated. Assuming #12166 is merged, the change should be:

s/can be an absolute path or a relative path (relative to the current working directory)/must be an absolute path/

ryanofsky · 2018-01-18T20:19:48Z

Rebased 2214a15 -> b84b0dd (pr/wdabs.1 -> pr/wdabs.2). Only change is release notes update.

maflcko · 2018-01-18T20:34:35Z

src/bitcoind.cpp

@@ -159,6 +159,14 @@ bool AppInit(int argc, char* argv[])
            return false;
 #endif // HAVE_DECL_DAEMON
        }
+        // Warn about relative -datadir path.


Any reason this is not in AppInitMain instead? I am asking, because I don't see the warning when using the gui.

Any reason this is not in AppInitMain instead? I am asking, because I don't see the warning when using the gui.

No reason, and this is a good thought. Moved in 46c40bf

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

src/wallet/init.cpp

        }
-        return InitError(strprintf(_("Specified -walletdir \"%s\" does not exist"), gArgs.GetArg("-walletdir", "").c_str()));
    }

    LogPrintf("Using wallet directory %s\n", GetWalletDir().string());


Nit: Could remove call to fs::system_complete in GetWalletDir?

Nit: Could remove call to fs::system_complete in GetWalletDir?

I'm not sure it was necessary to begin with, but now removed in 46c40bf.

Change suggested bitcoin#12220 (comment) bitcoin#12220 (comment)

jnewbery · 2018-01-18T21:20:59Z

Tested ACK ec527c6 modulo @MarcoFalke's comments.

You can probably remove the RFC since it seems like people want this to be merged for v0.16

ryanofsky

Added 1 commits b84b0dd -> 46c40bf (pr/wdabs.2 -> pr/wdabs.3, compare)
Squashed 46c40bf -> ec527c6 (pr/wdabs.3 -> pr/wdabs.4)

ryanofsky · 2018-01-18T21:20:28Z

src/bitcoind.cpp

@@ -159,6 +159,14 @@ bool AppInit(int argc, char* argv[])
            return false;
 #endif // HAVE_DECL_DAEMON
        }
+        // Warn about relative -datadir path.


Any reason this is not in AppInitMain instead? I am asking, because I don't see the warning when using the gui.

No reason, and this is a good thought. Moved in 46c40bf

ryanofsky · 2018-01-18T21:21:27Z

src/wallet/init.cpp

        }
-        return InitError(strprintf(_("Specified -walletdir \"%s\" does not exist"), gArgs.GetArg("-walletdir", "").c_str()));
    }

    LogPrintf("Using wallet directory %s\n", GetWalletDir().string());


Nit: Could remove call to fs::system_complete in GetWalletDir?

I'm not sure it was necessary to begin with, but now removed in 46c40bf.

maflcko · 2018-01-18T21:49:23Z

Will test after squash

jnewbery · 2018-01-19T01:45:20Z

src/wallet/init.cpp

+            return InitError(strprintf(_("Specified -walletdir \"%s\" does not exist"), wallet_dir.string()));
+        } else if (!fs::is_directory(wallet_dir)) {
+            return InitError(strprintf(_("Specified -walletdir \"%s\" is not a directory"), wallet_dir.string()));
+        } else if (!wallet_dir.is_absolute()) {


It makes slightly more sense to me to have this as the first check, ie we should error out immediately if the user has provided a relative path, rather than first checking whether it exists and is a directory. Is there any particular reason that you've placed this as the last check?

jnewbery · 2018-01-19T01:47:25Z

tested reACK ec527c6

jonasschnelli · 2018-01-19T07:01:16Z

utACK ec527c6

laanwj · 2018-01-19T16:46:03Z

utACK ec527c6

I'm going forward an merging this, @jnewbery's suggestion makes sense but is no reason to hold this up with so many utACKs of the current state, it can be switched around later.

ec527c6 Don't allow relative -walletdir paths (Russell Yanofsky) Pull request description: This makes it an error to explicitly specify a non-absolute -walletdir path, and also adds a debug.log warning if a relative rather than absolute -datadir path is configured. Specifying paths relative to the current working directory in a daemon process can be dangerous, because files can fail to be located even if the configuration doesn't change, but the daemon is started up differently. Specifying a relative -datadir now adds a warning to the debug log. It would not be backwards-compatible to forbid relative -datadir paths entirely, and it could also be inconvenient for command line testing. Specifying a relative -walletdir now results in a startup error. But since the -walletdir option is new in 0.16.0, there should be no compatibility issues. Another reason not to use working directory paths for -walletdir specifically is that the default -walletdir is a "wallets" subdirectory inside the datadir, so it could be surprising that setting -walletdir manually would choose a directory rooted in a completely different location. Tree-SHA512: 67cbdae677f82487a9031c5ec96b0205a488ab08718a0f4f49365e025119f3d7f6cfc88ba1eba04c1ecd8b9561a5b2c42e2e1a267af7c08c76b83e5e361f5a31

jnewbery · 2018-01-19T18:03:33Z

@jnewbery's suggestion makes sense

Agree that this should be merged without changing the order. This was just a suggestion - I don't even think it needs to be changed later.

maflcko · 2018-01-19T18:26:12Z

ec527c6 Don't allow relative -walletdir paths (Russell Yanofsky) Pull request description: This makes it an error to explicitly specify a non-absolute -walletdir path, and also adds a debug.log warning if a relative rather than absolute -datadir path is configured. Specifying paths relative to the current working directory in a daemon process can be dangerous, because files can fail to be located even if the configuration doesn't change, but the daemon is started up differently. Specifying a relative -datadir now adds a warning to the debug log. It would not be backwards-compatible to forbid relative -datadir paths entirely, and it could also be inconvenient for command line testing. Specifying a relative -walletdir now results in a startup error. But since the -walletdir option is new in 0.16.0, there should be no compatibility issues. Another reason not to use working directory paths for -walletdir specifically is that the default -walletdir is a "wallets" subdirectory inside the datadir, so it could be surprising that setting -walletdir manually would choose a directory rooted in a completely different location. Tree-SHA512: 67cbdae677f82487a9031c5ec96b0205a488ab08718a0f4f49365e025119f3d7f6cfc88ba1eba04c1ecd8b9561a5b2c42e2e1a267af7c08c76b83e5e361f5a31

…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

laanwj added this to the 0.16.0 milestone Jan 18, 2018

laanwj added the Wallet label Jan 18, 2018

maflcko added the Needs release note label Jan 18, 2018

ryanofsky force-pushed the pr/wdabs branch from 2214a15 to b84b0dd Compare January 18, 2018 20:19

maflcko reviewed Jan 18, 2018

View reviewed changes

ryanofsky force-pushed the pr/wdabs branch 2 times, most recently from ec527c6 to 46c40bf Compare January 18, 2018 21:19

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 18, 2018

fixup! Don't allow relative -walletdir paths

46c40bf

Change suggested bitcoin#12220 (comment) bitcoin#12220 (comment)

ryanofsky changed the title ~~RFC: Error if relative -walletdir is specified~~ Error if relative -walletdir is specified Jan 18, 2018

ryanofsky commented Jan 18, 2018

View reviewed changes

ryanofsky force-pushed the pr/wdabs branch from 46c40bf to ec527c6 Compare January 18, 2018 23:57

jnewbery reviewed Jan 19, 2018

View reviewed changes

laanwj merged commit ec527c6 into bitcoin:master Jan 19, 2018

sipa removed the Needs release note label Aug 14, 2018

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

Error if relative -walletdir is specified #12220

Error if relative -walletdir is specified #12220

ryanofsky commented Jan 18, 2018 •

edited

laanwj commented Jan 18, 2018 •

edited

ryanofsky commented Jan 18, 2018 •

edited

meshcollider commented Jan 18, 2018

jnewbery commented Jan 18, 2018

laanwj commented Jan 18, 2018

maflcko commented Jan 18, 2018

jnewbery commented Jan 18, 2018

ryanofsky commented Jan 18, 2018

maflcko Jan 18, 2018

ryanofsky Jan 18, 2018

maflcko Jan 18, 2018

ryanofsky Jan 18, 2018

jnewbery commented Jan 18, 2018

ryanofsky left a comment

ryanofsky Jan 18, 2018

ryanofsky Jan 18, 2018

maflcko commented Jan 18, 2018

jnewbery Jan 19, 2018

jnewbery commented Jan 19, 2018

jonasschnelli commented Jan 19, 2018

laanwj commented Jan 19, 2018

jnewbery commented Jan 19, 2018

maflcko commented Jan 19, 2018

Error if relative -walletdir is specified #12220

Error if relative -walletdir is specified #12220

Conversation

ryanofsky commented Jan 18, 2018 • edited

laanwj commented Jan 18, 2018 • edited

ryanofsky commented Jan 18, 2018 • edited

meshcollider commented Jan 18, 2018

jnewbery commented Jan 18, 2018

laanwj commented Jan 18, 2018

maflcko commented Jan 18, 2018

jnewbery commented Jan 18, 2018

ryanofsky commented Jan 18, 2018

maflcko Jan 18, 2018

Choose a reason for hiding this comment

ryanofsky Jan 18, 2018

Choose a reason for hiding this comment

maflcko Jan 18, 2018

Choose a reason for hiding this comment

ryanofsky Jan 18, 2018

Choose a reason for hiding this comment

jnewbery commented Jan 18, 2018

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Jan 18, 2018

Choose a reason for hiding this comment

ryanofsky Jan 18, 2018

Choose a reason for hiding this comment

maflcko commented Jan 18, 2018

jnewbery Jan 19, 2018

Choose a reason for hiding this comment

jnewbery commented Jan 19, 2018

jonasschnelli commented Jan 19, 2018

laanwj commented Jan 19, 2018

jnewbery commented Jan 19, 2018

maflcko commented Jan 19, 2018

ryanofsky commented Jan 18, 2018 •

edited

laanwj commented Jan 18, 2018 •

edited

ryanofsky commented Jan 18, 2018 •

edited