Error if relative -walletdir is specified #12220

pull ryanofsky wants to merge 1 commits into bitcoin:master from ryanofsky:pr/wdabs changing 7 files +32 −16
  1. ryanofsky commented at 6:45 PM on January 18, 2018: member

    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.

  2. laanwj commented at 6:49 PM on January 18, 2018: member

    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.

  3. ryanofsky commented at 6:51 PM on January 18, 2018: member

    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 (review). It could be changed, but @jnewbery and @MeshCollider pointed out this would be treating -walletdir inconsistently from -datadir.

  4. meshcollider commented at 7:05 PM on January 18, 2018: contributor

    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

  5. jnewbery commented at 7:55 PM on January 18, 2018: member

    As noted here: #12166 (review), 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.

  6. laanwj commented at 8:00 PM on January 18, 2018: member

    this would be treating -walletdir inconsistently from -datadir.

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

  7. laanwj added this to the milestone 0.16.0 on Jan 18, 2018
  8. laanwj added the label Wallet on Jan 18, 2018
  9. MarcoFalke commented at 8:08 PM on January 18, 2018: member

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

  10. Don't allow relative -walletdir paths
    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.
    ec527c6c88
  11. jnewbery commented at 8:11 PM on January 18, 2018: member

    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/

  12. MarcoFalke added the label Needs release notes on Jan 18, 2018
  13. ryanofsky force-pushed on Jan 18, 2018
  14. ryanofsky commented at 8:19 PM on January 18, 2018: member

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

  15. in src/bitcoind.cpp:162 in b84b0ddd21 outdated 
     158 | @@ -159,6 +159,14 @@ bool AppInit(int argc, char* argv[])
 159 |              return false;
 160 |  #endif // HAVE_DECL_DAEMON
 161 |          }
 162 | +        // Warn about relative -datadir path.
    MarcoFalke commented at 8:34 PM on January 18, 2018:

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

    ryanofsky commented at 9:20 PM on January 18, 2018:

    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 46c40bfe2dbfc4ea05258c9b90273ed28cdc3278

  16. in src/wallet/init.cpp:219 in b84b0ddd21 outdated 
     218 | +            return InitError(strprintf(_("Specified -walletdir \"%s\" is a relative path"), wallet_dir.string()));
 219 |          }
 220 | -        return InitError(strprintf(_("Specified -walletdir \"%s\" does not exist"), gArgs.GetArg("-walletdir", "").c_str()));
 221 |      }
 222 |  
 223 |      LogPrintf("Using wallet directory %s\n", GetWalletDir().string());
    MarcoFalke commented at 8:37 PM on January 18, 2018:

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

    ryanofsky commented at 9:21 PM on January 18, 2018:

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

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

  17. ryanofsky force-pushed on Jan 18, 2018
  18. ryanofsky force-pushed on Jan 18, 2018
  19. ryanofsky referenced this in commit 46c40bfe2d on Jan 18, 2018
  20. jnewbery commented at 9:20 PM on January 18, 2018: member

    Tested ACK ec527c6c88146d5b36de38a1fcebe4f6ea72bd1b modulo @MarcoFalke's comments.

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

  21. ryanofsky renamed this:
    RFC: Error if relative -walletdir is specified
    Error if relative -walletdir is specified
    on Jan 18, 2018
  22. ryanofsky commented at 9:26 PM on January 18, 2018: member

    Added 1 commits b84b0ddd21b20857417b6e120456dac94331bcc3 -> 46c40bfe2dbfc4ea05258c9b90273ed28cdc3278 (pr/wdabs.2 -> pr/wdabs.3, compare) Squashed 46c40bfe2dbfc4ea05258c9b90273ed28cdc3278 -> ec527c6c88146d5b36de38a1fcebe4f6ea72bd1b (pr/wdabs.3 -> pr/wdabs.4)

  23. MarcoFalke commented at 9:49 PM on January 18, 2018: member

    Will test after squash

  24. ryanofsky force-pushed on Jan 18, 2018
  25. in src/wallet/init.cpp:214 in ec527c6c88 
     212 | +        fs::path wallet_dir = gArgs.GetArg("-walletdir", "");
 213 | +        if (!fs::exists(wallet_dir)) {
 214 | +            return InitError(strprintf(_("Specified -walletdir \"%s\" does not exist"), wallet_dir.string()));
 215 | +        } else if (!fs::is_directory(wallet_dir)) {
 216 | +            return InitError(strprintf(_("Specified -walletdir \"%s\" is not a directory"), wallet_dir.string()));
 217 | +        } else if (!wallet_dir.is_absolute()) {
    jnewbery commented at 1:45 AM on January 19, 2018:

    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?

  26. jnewbery commented at 1:47 AM on January 19, 2018: member

    tested reACK ec527c6c88146d5b36de38a1fcebe4f6ea72bd1b

  27. jonasschnelli commented at 7:01 AM on January 19, 2018: contributor

    utACK ec527c6c88146d5b36de38a1fcebe4f6ea72bd1b

  28. laanwj commented at 4:46 PM on January 19, 2018: member

    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.

  29. laanwj merged this on Jan 19, 2018
  30. laanwj closed this on Jan 19, 2018
  31. laanwj referenced this in commit f4c942e361 on Jan 19, 2018
  32. jnewbery commented at 6:03 PM on January 19, 2018: member

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

  33. MarcoFalke commented at 6:26 PM on January 19, 2018: member 
    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

utACK ec527c6c88146d5b36de38a1fcebe4f6ea72bd1b
-----BEGIN PGP SIGNATURE-----

iQIcBAEBCgAGBQJaYjgpAAoJENLqSFDnUoslY6cP/jklOQrp19+pqj9yBkCRdLAX
LQAGiee3vtHgam//akHAnhz3o0uRir8r5IglXAGOxNL7au3slhNN7SCLUgP34VwE
jRirwAjjZZx8hpmRoCtxc6XOx2jNNgSLwUbT1dn96NHk78fxE6MJHLAZVaHCHTlt
UnTvrHMBnoB/Smd0YyVMmKCfg/RP7bDqtHPl+Bh0dly/KKmjckzNLnbpCWsGBN8I
JNvXnMY8mf60poL+qvQRAwf8yxlf1NF8N9tO/AC1MlmgBmxKCBdOshjFgwGxpLQa
F2i/hV8ETt5GaO+j4CJLHJPo0mqK+mKUd4VNQBLGBoF6rgGUf29sI4MK9t73fI26
8Ha6r3hlsFSm9RkfRiSQ8QEcpZiu/AKw7VXGqA84OCe9gzIEzBsJd08yIdyKPo4+
Dh+ONiUbQVT9dX6COHzgArM2xIKQLoBjcwiKbfYna+TYrvNOPRTsCfLO7M/I7nf9
VTWhbYDUChQ50C5d9Z6+0HRTtcMIYHEogakgvwHF3IawHGwnm+RuL4bd3Hn7e+p6
zJVxxPhhbvzpGnFNKs/AY+VZ4HpXGHo1W2l72n5vUld1uYSB6moKCU8ZkxkTvbKA
wmN8pnCTma7FufUMxASjSi0fJJbZeb8PQu7sy35LdWJnts5Y2mHKsavog59MIw7B
ldv6suhFLIKUuxCHiXfg
=pTUI
-----END PGP SIGNATURE-----
  34. sipa removed the label Needs release notes on Aug 14, 2018
  35. PastaPastaPasta referenced this in commit 9804715fde on Feb 25, 2020
  36. PastaPastaPasta referenced this in commit 7b75f290a4 on Feb 27, 2020
  37. PastaPastaPasta referenced this in commit 9402b45d12 on Feb 27, 2020
  38. PastaPastaPasta referenced this in commit df19c2c6a9 on Feb 27, 2020
  39. UdjinM6 referenced this in commit bf98709e9c on Feb 29, 2020
  40. PastaPastaPasta referenced this in commit 64e33f715f on Mar 4, 2020
  41. ckti referenced this in commit 3c79094150 on Mar 28, 2021
  42. gades referenced this in commit 7998c9d90d on Jun 30, 2021
  43. furszy referenced this in commit b04e1f5a90 on Jul 23, 2021
  44. MarcoFalke locked this on Sep 8, 2021
Contributors
ryanofskylaanwjmeshcolliderjnewberyMarcoFalkejonasschnelli
Labels
Wallet

Milestone
0.16.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-21 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me