util: Avoid buggy std::filesystem:::create

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.

02022-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.
1Warning: 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

0~/btc-things/bitcoin$ ll /home/me/.bitcoin/wallets/
1total 4
2drwxrwxrwx 1 root root    0 Feb  5 10:24 ./
3drwxrwxrwx 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:

0lrwxrwxrwx 1 me me 27 Feb  6 17:05 /home/me/.bitcoin -> /home/me/.bitcoindatadir/
1lrwxrwxrwx 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):

0$ rm -rf ~/.bitcoin
1$ ./src/bitcoind -testnet
2$ find ~/.bitcoin -type d
3/home/hebasto/.bitcoin
4/home/hebasto/.bitcoin/wallets
5/home/hebasto/.bitcoin/testnet3
6/home/hebasto/.bitcoin/testnet3/wallets
7/home/hebasto/.bitcoin/testnet3/chainstate
8/home/hebasto/.bitcoin/testnet3/blocks
9/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 🐬

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK b9c113af754540341d9529532fbadb7525168102 🐬
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjjYQv/cXNDjyxMuXXzCYEPC7+MwDfQfxws/Xyqbj6nJS4ke7gzmzBfn9VW+pp8
 8CABVuHSJuux5ElxAJcVWq1ODdlt/qUPeN8Ili7K3i03DvxtlDKyRUfAXneXoNysE
 9LGVkWd4U2cq4zwidgpjD5yo+QFeXVe9qg00YFxsqevmWQ73o2hJMhJxdAyfVdz8N
10QuMZMthT1jvdoBP9ROq+2qNZob4ZBizHWsoHGCOqmyhFJokgXXfUMHZD4TVT321A
11zc7M6+daVWH8UTNHxuXw8V9cLDbAMHJhKDj+Y4dOLwFjbtXuD13ZJI1VG5KC4VZo
12dN36XSOCsjqpaoNvwQ5dKs28QDNHtAUXw91Dz9vCsYxulY7GrTUtkDxmdGsCjnWX
13EcNYGnD+6JagiR9101WG0RbgeOyRnNk12gOhcURX8jtACgCcW6CzQyhvfUtY9Nhj
14KBnSJYOIOstzhkDq7BBnw41+3R25ezZmaI8yQs+H0lOw04eTtxMiZYmi0Chg9WwY
15LnF0vBuD
16=+A1J
17-----END PGP SIGNATURE-----

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

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

Conflicts