util: Avoid buggy std::filesystem:::create_directories() call #24266

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:220204-dirs changing 1 files +8 −4

hebasto commented at 9:22 PM on February 4, 2022: member

Compiled with some libstdc++ versions (e.g., on Ubuntu 20.04) std::filesystem:::create_directories() call fails to handle symbol links properly.

No behavior change in comparison to the pre-20744 master branch.

Fixes bitcoin/bitcoin#24257.
DrahtBot added the label Utils/log/libs on Feb 4, 2022
hebasto marked this as a draft on Feb 4, 2022

nymkappa commented at 1:28 AM on February 5, 2022: none

May I ask why you've added the sub directory /wallets ? While bitcoind and bitcoin-cli now runs fine without -datadir option (following my symbolic link), wallets are not found anymore because of that change.

2022-02-05T01:25:45Z Warning: Skipping -wallet path that doesn't exist. Failed to load database path '/home/me/.bitcoin/wallets/watching'. Path does not exist.
Warning: Skipping -wallet path that doesn't exist. Failed to load database path '/home/me/.bitcoin/wallets/watching'. Path does not exist.

Note that the sub directory /wallets was properly created

~/btc-things/bitcoin$ ll /home/me/.bitcoin/wallets/
total 4
drwxrwxrwx 1 root root    0 Feb  5 10:24 ./
drwxrwxrwx 1 root root 4096 Feb  5 10:26 ../

nymkappa commented at 2:29 AM on February 5, 2022: none

If you create the /wallets directory all the time, it will break current setups which don't use it. Now of course I could just move my wallet folder into the /wallets sub-directory and it would work fine, but that's probably not the way to go.

See here the wallet directory loading logic https://github.com/bitcoin/bitcoin/blob/master/src/wallet/walletutil.cpp#L25
nymkappa commented at 3:16 AM on February 5, 2022: none

@hebasto How about something like this instead? https://github.com/bitcoin/bitcoin/pull/24267
DrahtBot commented at 9:00 AM on February 5, 2022: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #24274 (Introduce GetFileArg and use it where possible by prusnak)
- #24265 (Drop StripRedundantLastElementsOfPath() function by hebasto)
- #23931 (create bitcoin.conf on first run with template by prayank23)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

util: Avoid buggy std::filesystem:::create_directories() call

Compiled with some libstdc++ versions (e.g., on Ubuntu 20.04)
std::filesystem:::create_directories() call fails to handle symbol links
properly.

b9c113af75

hebasto force-pushed on Feb 5, 2022
hebasto marked this as ready for review on Feb 5, 2022
hebasto commented at 4:40 PM on February 5, 2022: member
Updated 94fe141d207999483d54b73a6318cc12fb853b46 -> b9c113af754540341d9529532fbadb7525168102 (pr24266.01 -> pr24266.02, diff):
- addressed @nymkappa's comment
nymkappa commented at 8:08 AM on February 6, 2022: none
Updated 94fe141 -> b9c113a (pr24266.01 -> pr24266.02, diff):

addressed @nymkappa's comment
Tested ACK. This fix is superior to #24267 as it keeps pre-20744 master branch behavior.

Additional perk is that it allows to follow symbolik links with a multiple depth levels, for example this works fine with this PR:
```
lrwxrwxrwx 1 me me 27 Feb  6 17:05 /home/me/.bitcoin -> /home/me/.bitcoindatadir/
lrwxrwxrwx 1 me me 31 Feb  6 17:04 /home/me/.bitcoindatadir -> /home/me/Documents/.bitcoin//
```
fanquake added this to the milestone 23.0 on Feb 6, 2022
fanquake requested review from ryanofsky on Feb 6, 2022
in src/util/system.cpp:450 in b9c113af75
```
 442 | @@ -443,14 +443,18 @@ const fs::path& ArgsManager::GetDataDir(bool net_specific) const
 443 |      } else {
 444 |          path = GetDefaultDataDir();
 445 |      }
 446 | -    if (net_specific)
 447 | -        path /= fs::PathFromString(BaseParams().DataDir());
 448 |  
 449 | -    if (fs::create_directories(path)) {
 450 | -        // This is the first run, create wallets subdirectory too
```
MarcoFalke commented at 9:56 AM on February 6, 2022:

why is this comment removed?

hebasto commented at 10:09 AM on February 6, 2022:

Code if (!fs::exists(path)) { fs::create_directories(path / "wallets"); } expresses the same idea, therefore, the removed comment is redundant.

ryanofsky commented at 4:59 PM on February 7, 2022:

In commit "util: Avoid buggy std::filesystem:::create_directories() call" (b9c113af754540341d9529532fbadb7525168102)

This is a tangent, but it would be nice to avoid creating this wallets directory here in the future. This is done for the backward compatibility, so wallet startup code is able to set -walletdir=<datadir> instead of -walletdir=<datadir>/wallets later if this directory doesn't exist.

I think a better approach would be to always use -walletdir=<datadir>/wallets and use a wallets -> . symlink for compatibility instead. On startup, datadir code would not have to create any "wallets" subdirectory. Instead wallet startup code would look for a <datadir>/wallets directory or symlink and use it if it exists. Otherwise it would call wallet::ListDatabases(GetDataDir()), and if the list is empty create the <datadir>/wallets directory. If the list is nonempty, create a <datadir>/wallets -> . symlink.

hebasto commented at 5:36 PM on February 7, 2022:

@ryanofsky

Agree. I was thinking about the same some time back and now. And I think it deserves its own PR, keeping this one focused on a workaround for buggy libc++ preserving the current behavior.
in src/util/system.cpp:458 in b9c113af75
```
 457 | +        if (!fs::exists(path)) {
 458 | +            fs::create_directories(path / "wallets");
 459 | +        }
 460 | +    }
 461 | +
 462 |      path = StripRedundantLastElementsOfPath(path);
```
ryanofsky commented at 4:47 PM on February 7, 2022:

In commit "util: Avoid buggy std::filesystem:::create_directories() call" (b9c113af754540341d9529532fbadb7525168102)

Could actually drop this line now. It won't do anything anymore because of the the new !empty check added above. But keeping this line is also fine. It's harmless and #24265 removes it anyway.

hebasto commented at 5:33 PM on February 7, 2022:

It's harmless and #24265 removes it anyway.

Exactly :)
ryanofsky approved
ryanofsky commented at 5:04 PM on February 7, 2022: member

Code review ACK b9c113af754540341d9529532fbadb7525168102. Nice simplification and fix

in src/util/system.cpp:447 in b9c113af75

 442 | @@ -443,14 +443,18 @@ const fs::path& ArgsManager::GetDataDir(bool net_specific) const
 443 |      } else {
 444 |          path = GetDefaultDataDir();
 445 |      }
 446 | -    if (net_specific)
 447 | -        path /= fs::PathFromString(BaseParams().DataDir());
 448 |  
 449 | -    if (fs::create_directories(path)) {
 450 | -        // This is the first run, create wallets subdirectory too
 451 | +    if (!fs::exists(path)) {

MarcoFalke commented at 1:35 PM on February 8, 2022:

Looks like this is also adding a new feature?

Previously, when a test chain was used on the first run, it wouldn't create the wallets dir in the root datadir (main). Now it does?

hebasto commented at 2:02 PM on February 8, 2022:

On master (8afcc89a8f96692dd880b6102c9a85fac18b9021):

$ rm -rf ~/.bitcoin
$ ./src/bitcoind -testnet
$ find ~/.bitcoin -type d
/home/hebasto/.bitcoin
/home/hebasto/.bitcoin/wallets
/home/hebasto/.bitcoin/testnet3
/home/hebasto/.bitcoin/testnet3/wallets
/home/hebasto/.bitcoin/testnet3/chainstate
/home/hebasto/.bitcoin/testnet3/blocks
/home/hebasto/.bitcoin/testnet3/blocks/index

MarcoFalke commented at 2:37 PM on February 8, 2022:

Ah ok, looks like this is a side effect of reading the config file in the main datadir.

MarcoFalke approved

MarcoFalke commented at 2:42 PM on February 8, 2022: member

review ACK b9c113af754540341d9529532fbadb7525168102 🐬

<details><summary>Show signature</summary>

Signature:

</details>

MarcoFalke merged this on Feb 8, 2022
MarcoFalke closed this on Feb 8, 2022
hebasto deleted the branch on Feb 8, 2022
sidhujag referenced this in commit 7b80800859 on Feb 9, 2022
laanwj commented at 2:40 PM on February 11, 2022: member

I run into a similar issue with the blocks directory. I think it would be better to define our own drop in replacement, say, fsbridge::create_directories and use it everywhere that fs::create_directories is, not work around this in every single place.
laanwj referenced this in commit 36a681581e on Feb 14, 2022
laanwj referenced this in commit e6fafbdfd0 on Feb 14, 2022
laanwj referenced this in commit 01c5aefa03 on Feb 14, 2022
laanwj referenced this in commit b9dff100ff on Feb 14, 2022
laanwj referenced this in commit 2df2c10ee1 on Feb 16, 2022
laanwj referenced this in commit b15bde7530 on Feb 16, 2022
laanwj referenced this in commit c936fc30ec on Feb 16, 2022
hebasto referenced this in commit 1a1f795f74 on Feb 16, 2022
laanwj referenced this in commit 1f46b6e46e on Feb 17, 2022
fanquake referenced this in commit 003523d239 on Feb 17, 2022
sidhujag referenced this in commit fd145549cb on Feb 18, 2022
hmel referenced this in commit 273f3fbc06 on Feb 20, 2022
DrahtBot locked this on Feb 14, 2023

Contributors

Labels

Milestone
23.0

Linked (view graph)

#24257 bitcoind and bitcoin-cli does not supports symbolic link anymore #24267 util: if `DataDir` is a symbolic link, manually follow it #24338 util: Work around libstdc++ create_directories issue

util: Avoid buggy std::filesystem:::create_directories() call #24266

Conflicts