util: Avoid buggy std::filesystem:::create_directories() call by hebasto · Pull Request #24266 · bitcoin/bitcoin

hebasto · 2022-02-04T21:22:35Z

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 #24257.

nymkappa · 2022-02-05T01:28:23Z

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 · 2022-02-05T02:29:28Z

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 · 2022-02-05T03:16:44Z

@hebasto How about something like this instead? #24267

DrahtBot · 2022-02-05T09:00:39Z

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.

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

hebasto · 2022-02-05T16:40:16Z

Updated 94fe141 -> b9c113a (pr24266.01 -> pr24266.02, diff):

addressed @nymkappa's comment

nymkappa · 2022-02-06T08:08:32Z

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//

maflcko · 2022-02-06T09:56:38Z

src/util/system.cpp

-        path /= fs::PathFromString(BaseParams().DataDir());

-    if (fs::create_directories(path)) {
-        // This is the first run, create wallets subdirectory too


why is this comment removed?

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

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

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.

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

ryanofsky

Code review ACK b9c113a. Nice simplification and fix

ryanofsky · 2022-02-07T16:47:00Z

src/util/system.cpp

+        }
+    }
+
    path = StripRedundantLastElementsOfPath(path);


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

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.

It's harmless and #24265 removes it anyway.

Exactly :)

ryanofsky · 2022-02-07T16:59:53Z

src/util/system.cpp

-        path /= fs::PathFromString(BaseParams().DataDir());

-    if (fs::create_directories(path)) {
-        // This is the first run, create wallets subdirectory too


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

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.

maflcko · 2022-02-08T13:35:59Z

src/util/system.cpp


-    if (fs::create_directories(path)) {
-        // This is the first run, create wallets subdirectory too
+    if (!fs::exists(path)) {


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?

On master (8afcc89):

$ 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

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

maflcko

review ACK b9c113a 🐬

Show signature

Signature:

…tories() call b9c113a util: Avoid buggy std::filesystem:::create_directories() call (Hennadii Stepanov) Pull request description: Compiled with some libstdc++ versions (e.g., on Ubuntu 20.04) [`std::filesystem:::create_directories()`](https://en.cppreference.com/w/cpp/filesystem/create_directory) call [fails](bitcoin#24257 (comment)) to handle symbol links properly. No behavior change in comparison to the [pre-20744](bitcoin@c194293) master branch. Fixes bitcoin#24257. ACKs for top commit: ryanofsky: Code review ACK b9c113a. Nice simplification and fix MarcoFalke: review ACK b9c113a 🐬 Tree-SHA512: 79d940cfc1f68d9b0548fb2ab005e90850b54ac0fb3bb2940afd632d56288d92687579a3176bac3fd0ea3d2dae71e26444f8f7bdb87862414c12866ae5e857c4

laanwj · 2022-02-11T14:40:46Z

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.

Work around libstdc++ issue [PR101510] with create_directories where the leaf already exists as a symlink. Fixes bitcoin#24257, introduced by the switch to `std::filesystem`. It is meant to be more thorough than bitcoin#24266, which only worked around one instance of the problem. The issue was fixed upstream in https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=124eaa50e0a34f5f89572c1aa812c50979da58fc, but unfortunately we'll have to carry a fix for it for a while. This introduces a function `fs::create_directories` which wraps `std::filesystem::create_directories`. This allows easiliy reverting the workaround when it is no longer necessary.

b223c3c test: Add functional test for symlinked blocks directory (laanwj) ddb75c2 test: Add fs_tests/create_directories unit test (Hennadii Stepanov) 1f46b6e util: Work around libstdc++ create_directories issue (laanwj) Pull request description: Work around libstdc++ issue [PR101510] with create_directories where the leaf already exists as a symlink. Fixes #24257, introduced by the switch to `std::filesystem`. It is meant to be more thorough than #24266, which worked around one instance of the problem. The issue was [fixed upstream](https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=124eaa50e0a34f5f89572c1aa812c50979da58fc), but unfortunately we'll have to carry a fix for it for a while. This introduces a function `fs::create_directories` which wraps `std::filesystem::create_directories`. This allows easiliy reverting the workaround when it is no longer necessary. ACKs for top commit: jonatack: re-ACK b223c3c per `git range-diff df08250 67019cd b223c3c` hebasto: re-ACK b223c3c w0xlt: re-ACK b223c3c vasild: ACK b223c3c Tree-SHA512: 028321717c8b10d16185c3711b35da6b05fb7aa31cee1c8c7e754e92bf5a0b02719a3785cd0f6f8bf052b3bd759f644af212320672baabc9e44e0b93ba464abc

…ssue b223c3c test: Add functional test for symlinked blocks directory (laanwj) ddb75c2 test: Add fs_tests/create_directories unit test (Hennadii Stepanov) 1f46b6e util: Work around libstdc++ create_directories issue (laanwj) Pull request description: Work around libstdc++ issue [PR101510] with create_directories where the leaf already exists as a symlink. Fixes bitcoin#24257, introduced by the switch to `std::filesystem`. It is meant to be more thorough than bitcoin#24266, which worked around one instance of the problem. The issue was [fixed upstream](https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=124eaa50e0a34f5f89572c1aa812c50979da58fc), but unfortunately we'll have to carry a fix for it for a while. This introduces a function `fs::create_directories` which wraps `std::filesystem::create_directories`. This allows easiliy reverting the workaround when it is no longer necessary. ACKs for top commit: jonatack: re-ACK b223c3c per `git range-diff df08250 67019cd b223c3c` hebasto: re-ACK b223c3c w0xlt: re-ACK b223c3c vasild: ACK b223c3c Tree-SHA512: 028321717c8b10d16185c3711b35da6b05fb7aa31cee1c8c7e754e92bf5a0b02719a3785cd0f6f8bf052b3bd759f644af212320672baabc9e44e0b93ba464abc

Work around libstdc++ issue [PR101510] with create_directories where the leaf already exists as a symlink. Fixes bitcoin#24257, introduced by the switch to `std::filesystem`. It is meant to be more thorough than bitcoin#24266, which only worked around one instance of the problem. The issue was fixed upstream in https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=124eaa50e0a34f5f89572c1aa812c50979da58fc, but unfortunately we'll have to carry a fix for it for a while. This introduces a function `fs::create_directories` which wraps `std::filesystem::create_directories`. This allows easiliy reverting the workaround when it is no longer necessary.

hebasto mentioned this pull request Feb 4, 2022

bitcoind and bitcoin-cli does not supports symbolic link anymore #24257

Closed

DrahtBot added the Utils/log/libs label Feb 4, 2022

hebasto marked this pull request as draft February 4, 2022 22:01

nymkappa mentioned this pull request Feb 5, 2022

util: if DataDir is a symbolic link, manually follow it #24267

Closed

This was referenced Feb 5, 2022

Drop StripRedundantLastElementsOfPath() function #24265

Merged

create bitcoin.conf on first run with template #23931

Closed

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

b9c113a

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

hebasto force-pushed the 220204-dirs branch from 94fe141 to b9c113a Compare February 5, 2022 16:33

hebasto marked this pull request as ready for review February 5, 2022 16:40

DrahtBot mentioned this pull request Feb 5, 2022

Introduce GetFileArg and use it where possible #24274

Closed

fanquake added this to the 23.0 milestone Feb 6, 2022

fanquake requested a review from ryanofsky February 6, 2022 08:15

maflcko reviewed Feb 6, 2022

View reviewed changes

ryanofsky approved these changes Feb 7, 2022

View reviewed changes

maflcko reviewed Feb 8, 2022

View reviewed changes

maflcko approved these changes Feb 8, 2022

View reviewed changes

maflcko merged commit 8edb041 into bitcoin:master Feb 8, 2022

hebasto deleted the 220204-dirs branch February 8, 2022 16:50

laanwj mentioned this pull request Feb 14, 2022

util: Work around libstdc++ create_directories issue #24338

Merged

bitcoin locked and limited conversation to collaborators Feb 14, 2023

Conversation

hebasto commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nymkappa commented Feb 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nymkappa commented Feb 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nymkappa commented Feb 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hebasto commented Feb 5, 2022

Uh oh!

nymkappa commented Feb 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Feb 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Feb 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

hebasto commented Feb 4, 2022 •

edited

Loading

nymkappa commented Feb 5, 2022 •

edited

Loading

nymkappa commented Feb 5, 2022 •

edited

Loading

nymkappa commented Feb 5, 2022 •

edited

Loading

DrahtBot commented Feb 5, 2022 •

edited

Loading

hebasto Feb 6, 2022 •

edited

Loading