odd behaviour of GetDataDir creating wallets/ subdirectory

markblundeberg commented at 5:44 pm on June 15, 2019: none

While backporting some things to Bitcoin ABC, I notice that GetDataDir is a bit awkward since it not only creates the directory in question, it also creates the wallets/ subdirectory … but only if it was the first time creating the datadir (see #11466).

Why this is a bit weird:

If something else during initialization happens to create the directory in question first (like GetBlocksDir), the wallets/ directory won’t get created. This is particularly relevant for the net-specific case. In Bitcoin Core, GetDataDir(true) gets called before GetBlocksDir() but seemingly by accident: the former is called during InitLogging when fetching the default debug.log location, and the latter during AppInitParameterInteraction shortly afterwards. If InitLogging were changed slightly, it could inadvertently cause testnet/regtest to stop creating wallets/ directory.
If someone creates the ~/.bitcoin directory just to host the bitcoin.conf before running first time, it won’t create the mainnet wallets/ subdirectory.
If running in -nowallet mode, it still creates the wallets/ directory. If running -regtest and -nowallet, it creates two wallets/ directories (the mainnet one and net-specific one) since both GetDataDir(false) and GetDataDir(true) get called during init.
If a user were to run in -nowallet, then deleted the empty and apparently superfluous wallets/ directory, it will not get recreated if later run with wallet enabled. Then bitcoind will instead create wallet.dat in the bare directory.
If bitcoind is compiled without wallet support, it still creates wallets/ directory (sometimes).

I think a more elegant solution would be to have wallets/ directory created in the wallet initialization, however I’m not an expert on the wallet part of the code and how this ought to behave.

(pinging @meshcollider from original PR)

hebasto commented at 10:54 pm on June 16, 2019: member

@markblundeberg could you look into #15864?

markblundeberg commented at 4:06 am on June 17, 2019: none

@hebasto Hmm interesting, thanks for pointing that out. I think you’re onto something good there. To be honest I don’t fully grok the init code so I can’t provide a good review there.

Relating to this wallets/ directory issue though, I wonder if in the case of #15240, it would have been desired for bitcoind to create a wallets/ directory on first run, even if /var/local/lib/bitcoind already exists (it would have been created by a sysadmin).

There must be some elegant solution to all this, but I don’t have any great ideas to offer.

laanwj commented at 7:58 am on June 18, 2019: member

I think a more elegant solution would be to have wallets/ directory created in the wallet initialization, however I’m not an expert on the wallet part of the code and how this ought to behave.

It would be, however the reason that wallets/ is not created when the .bitcoin directory already exists is backwards compatibility. If there is an existing datadir but no wallets directory it’s assumed to be an old data directory, and creating wallets would effectively hide the existing wallets in the top level dir, possibly leading to people to suspect loss of funds.

There would be other possibilities such as checking for any existing wallets first, but that was deemed more dangerous: no naming convention was enforced for wallets before, so it’s non-trivial and brittle to do this detection.

There was a lot of discussion around this at the time.

I guess there could be another heuristic: if the datadir is completely empty, do create the wallets directory. Then again, it’s kind of piling on hacks (do you want to exclude bitcoin.conf in this count?) I wish there was a better way.

laanwj added the label Wallet on Jun 18, 2019

maflcko referenced this in commit 83112db129 on Aug 19, 2019

sidhujag referenced this in commit 894dd2f309 on Aug 19, 2019

ryanofsky commented at 3:46 pm on February 14, 2023: contributor

I found this in my TODO notes from a previous discussion about this (https://github.com/bitcoin/bitcoin/pull/24266#discussion_r800865220), and think it could be a good solution

util/system datadir code should not care about wallets or look for or create any wallet paths
If -walletdir is specified, wallet code should use that directory to locate and list wallets.
If -walletdir is specified and path is not a directory, wallet init should return a fatal error and refuse to start
If -walletdir is not specified, wallet code should use <datadir>/wallets as the directory to locate and list wallets.
If -walletdir is not specified and <datadir>/wallets does not exist, wallet init code should create it
- Before creating <datadir>/wallets wallet should call ~~ListWalletDir~~ ListDatabases on <datadir>
- If list is empty wallet should create <datadir>/wallets as a directory, otherwise it should create it as a symlink pointing to .

BrandonOdiwuor commented at 9:44 am on September 13, 2023: contributor

would like to work on this, with the solution proposed by @ryanofsky

BrandonOdiwuor referenced this in commit 8963ac17fe on Sep 21, 2023

BrandonOdiwuor referenced this in commit 0d4b5d9527 on Sep 21, 2023

BrandonOdiwuor referenced this in commit ac741738c8 on Sep 25, 2023

BrandonOdiwuor referenced this in commit 3b7d0e90c4 on Sep 25, 2023

BrandonOdiwuor referenced this in commit 4544d039bb on Sep 26, 2023

BrandonOdiwuor referenced this in commit aa2d382968 on Sep 27, 2023

BrandonOdiwuor referenced this in commit 9c3b79ebab on Sep 27, 2023

BrandonOdiwuor commented at 6:10 am on October 11, 2023: contributor

@ryanofsky @hebasto @laanwj could you please look at #28514, I created a PR regarding this issue

BrandonOdiwuor referenced this in commit e37e60f7b9 on Oct 17, 2023

BrandonOdiwuor referenced this in commit 6b4ad101e3 on Oct 17, 2023

BrandonOdiwuor referenced this in commit 17b4955e4b on Oct 18, 2023

BrandonOdiwuor referenced this in commit dfe62e9acc on Oct 21, 2023

achow101 commented at 2:50 pm on April 9, 2024: member

The feature request didn’t seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open issues.

Closing due to lack of interest. Pull requests with improvements are always welcome.

achow101 closed this on Apr 9, 2024

darosior commented at 2:50 pm on April 9, 2024: member

Is it just that it creates a “wallets” folder even without wallets? If so it doesn’t seem important.

odd behaviour of GetDataDir creating wallets/ subdirectory #16220