Repository: bitcoin/bitcoin (language: cpp)

Issue:
bitcoind and bitcoin-cli does not supports symbolic link anymore
I just pulled latest master at https://github.com/bitcoin/bitcoin/commit/3ace3a17c9bce606cea05192f0da3ac62ac69dda and I cannot run bitcoind or bitcoin-cli without providing a full path for data directory using `-datadir` option. It was working fine couple of days ago (while running https://github.com/bitcoin/bitcoin/commit/807169e10b4a18324356ed6ee4d69587b96a7c70). I suspect it may be related to latest switch to the standard std::filesystem library https://github.com/bitcoin/bitcoin/pull/20744

**Expected behavior**

I use a symbolic link to point my /home/me/.bitcoin to an external hard drive. When running `bitcoind` from command line, or executing `bitcoin-cli` commands, without using the `-datadir` option, it should not terminate or crash unexpectedly. Those commands should be able to follow the symbolic link and be able to read/write into the final folder.

**Actual behavior**

`bitcoind` and `bitcoin-cli` are not able to follow the symbolic link. `bitcoin-qt` runs fine, even when running it from command line. See below commands examples.

```
# Create a symbolic link
$~/btc-things/bitcoin$ ln -s /media/DATA/Bitcoin /home/me/.bitcoin

# Try to run bitcoind - Crashes
$~/btc-things/bitcoin$ ./bitcoind 


************************
EXCEPTION: NSt10filesystem7__cxx1116filesystem_errorE       
filesystem error: cannot create directories: Not a directory [/home/me/.bitcoin]       
bitcoin in AppInit()       

bitcoind: chainparamsbase.cpp:35: const CBaseChainParams& BaseParams(): Assertion `globalChainBaseParams' failed.
Aborted (core dumped)

# Run bitcoin-qt, it works fine, the node runs
$~/btc-things/bitcoin$ ./bitcoin-qt 
qt5ct: using qt5ct plugin

# Try to run a bitcoin-cli command to interact with the running node, does not work
$~/btc-things/bitcoin$ ./bitcoin-cli getblockhash 501726


************************
EXCEPTION: NSt10filesystem7__cxx1116filesystem_errorE       
filesystem error: cannot create directories: Not a directory [/home/me/.bitcoin]       
bitcoin in AppInitRPC()       

# Try the same previous command but give an absolute path for -datadir, works fine
$~/btc-things/bitcoin$ ./bitcoin-cli -datadir=/media/DATA/Bitcoin getblockhash 501726
0000000000000000004b27f9ee7ba33d6f048f684aaeb0eea4befd80f1701126
```

**To reproduce**

Steps are indicated previously.

**System information**

<!-- What version of Bitcoin Core are you using, where did you get it (website, self-compiled, etc)? -->

Running `Bitcoin Core version v22.99.0-3ace3a17c9bc`. Self compiled from `master` branch.

<!-- What type of machine are you observing the error on (OS/CPU and disk type)? -->

I'm using Linux Mint with the following kernel `Linux 5.4.0-96-generic x86_64`.
I'm using an additional hard drive to store the full blockchain (SSD).

<!-- Any extra information that might be useful in the debugging process. -->

I had no prior issues before pulling latest changes. I was previously running at https://github.com/bitcoin/bitcoin/commit/807169e10b4a18324356ed6ee4d69587b96a7c70


Discussion / hints:
@nymkappa 

What is result of running unit and functional tests?
Confirming the bug on Linux Mint 20.2 (x86_64):
```
$ mkdir /home/hebasto/NVMe/TEST_DATADIR
$ ln -s /home/hebasto/NVMe/TEST_DATADIR /home/hebasto/.bitcoin
$ ./src/bitcoind -regtest


************************
EXCEPTION: NSt10filesystem7__cxx1116filesystem_errorE       
filesystem error: cannot create directories: Not a directory [/home/hebasto/.bitcoin]       
bitcoin in AppInit()       

bitcoind: chainparamsbase.cpp:35: const CBaseChainParams &BaseParams(): Assertion `globalChainBaseParams' failed.
Aborted (core dumped)
```
---

UPDATE: The bug is not reproducible on Ubuntu 21.10.

UPDATE 2: Confirming on Ubuntu 20.04.3 LTS.
> @nymkappa
> 
> What is result of running unit and functional tests?

I actually had the same issue that you've described here https://github.com/bitcoin/bitcoin/issues/20869#issuecomment-764884376. Removing `qt5ct` allowed me to run `make check`. No error shown, all tests seem to pass.

I'm still waiting for `feature_dbcrash.py` test to complete, but all others went through without any issue.

UPDATE: All tests passes
Looks like a libstdc++ bug in [`std::filesystem::create_directories`](https://en.cppreference.com/w/cpp/filesystem/create_directory) implementation.

After configuring with `CC=clang-12 CXX='clang++-12 -stdlib=libc++'` and compiling, everything works just fine.
I have the same issue on a RaspberryPi 4b 8gig:
Bitcoin Core version v22.99.0-b2a8371913a8

```
bitcoin@raspberrypi:/media/pi/PiData/bitcoin/.bitcoin $ bitcoind -regtest
************************
EXCEPTION: NSt10filesystem7__cxx1116filesystem_errorE       
filesystem error: cannot create directories: Not a directory [/home/bitcoin/.bitcoin]       
bitcoin in AppInit()       

bitcoind: chainparamsbase.cpp:35: const CBaseChainParams& BaseParams(): Assertion `globalChainBaseParams' failed.
```

UPDATE: Manually implemented https://github.com/bitcoin/bitcoin/pull/24266 locally, it fixed the issue here
Bitcoin Core version v22.99.0-b2a8371913a8-dirty
@nymkappa @sebbavw 

Mind testing #24266?
> Looks like a libstdc++ bug in [`std::filesystem::create_directories`](https://en.cppreference.com/w/cpp/filesystem/create_directory) implementation.

The bug has been fixed:
- upstream [124eaa50e0a34f5f89572c1aa812c50979da58fc](https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=124eaa50e0a34f5f89572c1aa812c50979da58fc)
- backported to gcc-11 [bde28c60c70079416dd2181f882c0694e019eaef](https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=bde28c60c70079416dd2181f882c0694e019eaef) (not in 11.2.0)
- backported to gcc-10 [86bdcd21a1be008df28648b185c12221d917a166](https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=86bdcd21a1be008df28648b185c12221d917a166) (not in 10.3.0)
- backported to gcc-9 [f8ac77533951d79d4e6a65841aa30293b2f11fdd](https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=f8ac77533951d79d4e6a65841aa30293b2f11fdd) (not in 9.4.0)

### Some distros backporting info (as for 2022-02-05)

#### Debian 12 Bookworm, [libstdc++6](https://packages.debian.org/en/bookworm/libstdc++6)

Backported since gcc-11 (11.2.0-2).

#### Debian 11 Bullseye, [libstdc++6](https://packages.debian.org/en/bullseye/libstdc++6)

No backports to gcc-10.

#### Ubuntu 22.04 LTS, [libstdc++6](https://packages.ubuntu.com/jammy/libstdc++6)

Backported since gcc-11 (11.2.0-1ubuntu3).

#### Ubuntu 21.10, [libstdc++6](https://packages.ubuntu.com/impish/libstdc++6)

Backported since gcc-11 (11.2.0-1ubuntu3).

#### Ubuntu 20.04 LTS, [libstdc++6](https://packages.ubuntu.com/focal/libstdc++6)

No backports to gcc-10.

---

 Did not check gcc-10.3.0 we use for GUIX builds.
I don't think this is fixed (as of bcecde64b4ad7bb8e717d5709af8fd17532755ab)? My `blocks` directory is a symlink this used to work but now I get the following error on a run-of-the-mill Ubuntu 20.04 VM.
```
$ src/bitcoind

************************
EXCEPTION: NSt10filesystem7__cxx1116filesystem_errorE
filesystem error: cannot create directories: Not a directory [/home/…/.bitcoin/blocks]
bitcoin in AppInit()
```
I could reproduce the problem (when the blocks directory is a symlink) on Ubuntu 20.04.3 LTS
I tried the patch below and it fixed the mentioned error  but another error still occurs when creating  `blocks/index`.

`filesystem error: cannot create directories: File exists [/home/.../.bitcoin/signet/blocks/index]`

```diff

// src/util/system.cpp
path /= fs::PathFromString(BaseParams().DataDir());
path /= "blocks";
-if (!fs::exists(path)) {
+if (!fs::exists(path) && !fs::is_symlink(path)) {
        fs::create_directories(path);
}

// src/init.cpp
-if (!fs::is_directory(gArgs.GetBlocksDirPath())) {
+if (!fs::is_directory(gArgs.GetBlocksDirPath()) && !fs::is_symlink(gArgs.GetBlocksDirPath())) {
        return InitError(strprintf(_("Specified blocks directory \"%s\" does not exist."), args.GetArg("-blocksdir", "")));
    }
```
I think it would be the most thorough solution to make our own `fs::create_directories` wrapper and use that instead of `fs::create_directories`, everywhere it's used now. It's used in a lot of places in the code, fixing them one by one is error prone.

Fix patch:
diff --git a/src/fs.h b/src/fs.h
index d2299db168c34..00b786453c438 100644
--- a/src/fs.h
+++ b/src/fs.h
@@ -140,6 +140,28 @@ static inline path PathFromString(const std::string& string)
     return std::filesystem::path(string);
 #endif
 }
+
+/**
+ * Create directory (and if necessary its parents), unless the leaf directory
+ * already exists or is a symlink to an existing directory.
+ * This is a temporary workaround for an issue in libstdc++ that has been fixed
+ * upstream [PR101510].
+ */
+static inline bool create_directories(const std::filesystem::path& p)
+{
+    if (std::filesystem::is_symlink(p) && std::filesystem::is_directory(p)) {
+        return false;
+    }
+    return std::filesystem::create_directories(p);
+}
+
+/**
+ * This variant is not used. Delete it to prevent it from accidentally working
+ * around the workaround. If it is needed, add a workaround in the same pattern
+ * as above.
+ */
+bool create_directories(const std::filesystem::path& p, std::error_code& ec) = delete;
+
 } // namespace fs
 
 /** Bridge operations to C stdio */


Test patch:
diff --git a/src/test/dbwrapper_tests.cpp b/src/test/dbwrapper_tests.cpp
index 2f95cc27a3671..fc89fe1450007 100644
--- a/src/test/dbwrapper_tests.cpp
+++ b/src/test/dbwrapper_tests.cpp
@@ -203,7 +203,7 @@ BOOST_AUTO_TEST_CASE(existing_data_no_obfuscate)
 {
     // We're going to share this fs::path between two wrappers
     fs::path ph = m_args.GetDataDirBase() / "existing_data_no_obfuscate";
-    create_directories(ph);
+    fs::create_directories(ph);
 
     // Set up a non-obfuscated wrapper to write some initial data.
     std::unique_ptr<CDBWrapper> dbw = std::make_unique<CDBWrapper>(ph, (1 << 10), false, false, false);
@@ -244,7 +244,7 @@ BOOST_AUTO_TEST_CASE(existing_data_reindex)
 {
     // We're going to share this fs::path between two wrappers
     fs::path ph = m_args.GetDataDirBase() / "existing_data_reindex";
-    create_directories(ph);
+    fs::create_directories(ph);
 
     // Set up a non-obfuscated wrapper to write some initial data.
     std::unique_ptr<CDBWrapper> dbw = std::make_unique<CDBWrapper>(ph, (1 << 10), false, false, false);
diff --git a/src/test/fs_tests.cpp b/src/test/fs_tests.cpp
index 4fb2c23d98cae..313064b29453f 100644
--- a/src/test/fs_tests.cpp
+++ b/src/test/fs_tests.cpp
@@ -152,4 +152,28 @@ BOOST_AUTO_TEST_CASE(rename)
     fs::remove(path2);
 }
 
+#ifndef WIN32
+BOOST_AUTO_TEST_CASE(create_directories)
+{
+    // Test fs::create_directories workaround.
+    const fs::path tmpfolder{m_args.GetDataDirBase()};
+
+    const fs::path dir{GetUniquePath(tmpfolder)};
+    fs::create_directory(dir);
+    BOOST_CHECK(fs::exists(dir));
+    BOOST_CHECK(fs::is_directory(dir));
+    BOOST_CHECK(!fs::create_directories(dir));
+
+    const fs::path symlink{GetUniquePath(tmpfolder)};
+    fs::create_directory_symlink(dir, symlink);
+    BOOST_CHECK(fs::exists(symlink));
+    BOOST_CHECK(fs::is_symlink(symlink));
+    BOOST_CHECK(fs::is_directory(symlink));
+    BOOST_CHECK(!fs::create_directories(symlink));
+
+    fs::remove(symlink);
+    fs::remove(dir);
+}
+#endif // WIN32
+
 BOOST_AUTO_TEST_SUITE_END()
diff --git a/test/functional/feature_dirsymlinks.py b/test/functional/feature_dirsymlinks.py
new file mode 100755
index 0000000000000..85c8e2760081c
--- /dev/null
+++ b/test/functional/feature_dirsymlinks.py
@@ -0,0 +1,39 @@
+#!/usr/bin/env python3
+# Copyright (c) 2022 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test successful startup with symlinked directories.
+"""
+
+import os
+import sys
+
+from test_framework.test_framework import BitcoinTestFramework, SkipTest
+
+
+def rename_and_link(*, from_name, to_name):
+    os.rename(from_name, to_name)
+    os.symlink(to_name, from_name)
+    assert os.path.islink(from_name) and os.path.isdir(from_name)
+
+class SymlinkTest(BitcoinTestFramework):
+    def skip_test_if_missing_module(self):
+        if sys.platform == 'win32':
+            raise SkipTest("Symlinks test skipped on Windows")
+
+    def set_test_params(self):
+        self.num_nodes = 1
+
+    def run_test(self):
+        self.stop_node(0)
+
+        rename_and_link(from_name=os.path.join(self.nodes[0].datadir, self.chain, "blocks"),
+                        to_name=os.path.join(self.nodes[0].datadir, self.chain, "newblocks"))
+        rename_and_link(from_name=os.path.join(self.nodes[0].datadir, self.chain, "chainstate"),
+                        to_name=os.path.join(self.nodes[0].datadir, self.chain, "newchainstate"))
+
+        self.start_node(0)
+
+
+if __name__ == '__main__':
+    SymlinkTest().main()
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 1a0d62aa8f0d1..516e8be638702 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -321,6 +321,7 @@
     'rpc_getdescriptorinfo.py',
     'rpc_mempool_entry_fee_fields_deprecation.py',
     'rpc_help.py',
+    'feature_dirsymlinks.py',
     'feature_help.py',
     'feature_shutdown.py',
     'p2p_ibd_txrelay.py',