Fix windows libc++ `fs::path` `fstream` compile errors

ryanofsky commented at 3:08 pm on October 6, 2025: contributor

Drop support for passing fs::path directly to std::ifstream and std::ofstream constructors and open() functions, because as reported by hebasto in #33545, after https://wg21.link/lwg3430 there is no way this can continue to work in windows builds, and there are already compile errors compiling for windows with newer versions of libc++.

Instead, add an fs::path::std_path() method that returns std::filesystem::path references and use it where needed.

DrahtBot commented at 3:08 pm on October 6, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33550.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, l0rinc, maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27409 (Make GUI and CLI tools use the same datadir by ryanofsky)

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.

hebasto commented at 3:14 pm on October 6, 2025: member

Concept ACK.

Maybe consider removing these lines now:https://github.com/bitcoin/bitcoin/blob/f39d3e1a2f2b9cdcf0e77942fd69eb92a58ce77f/src/util/fs.h#L79-L81 ?

hebasto commented at 3:17 pm on October 6, 2025: member

CI fails:

0 /home/admin/actions-runner/_work/_temp/src/wallet/test/init_test_fixture.cpp:39:21: error: conversion function from 'mapped_type' (aka 'fs::path') to 'const string' (aka 'const basic_string<char>') invokes a deleted function
1   39 |     std::ofstream f{m_walletdir_path_cases["file"]};
2      |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3/home/admin/actions-runner/_work/_temp/src/util/fs.h:65:5: note: 'operator basic_string' has been explicitly marked deleted here
4   65 |     operator string_type() const = delete;
5      |     ^
6/cxx_build/include/c++/v1/fstream:1303:63: note: passing argument to parameter '__s' here
7 1303 |   _LIBCPP_HIDE_FROM_ABI explicit basic_ofstream(const string& __s, ios_base::openmode __mode = ios_base::out);
8      |                                                               ^
91 error generated.

maflcko commented at 3:24 pm on October 6, 2025: member

review ACK f39d3e1a2f2b9cdcf0e77942fd69eb92a58ce77f, but CI fails

DrahtBot requested review from hebasto on Oct 6, 2025

Fix windows libc++ fs::path fstream compile errors

As reported by hebasto in https://github.com/bitcoin/bitcoin/issues/33545,
newer libc++ versions implementing https://wg21.link/lwg3430 will no longer
implicitly convert `fs::path` objects to `std::filesystem::path` objects when
constructing `std::ifstream` and `std::ofstream` types.

This is not a problem in Unix systems since `fs::path` objects use
`std::string` as their native string type, but it causes compile errors on
Windows which use `std::wstring` as their string type, since `fstream`s can't
be constructed from `wstring`s.

Fix the windows libc++ compile errors by adding a new `fs::path::std_path()`
method and using it construct `fstream`s more portably.

Additionally, delete `fs::path`'s implicit `native_string` conversion so these
errors will not go undetected in the future, even though there is not currently
a CI job testing Windows libc++ builds.

b0113afd44

ryanofsky force-pushed on Oct 6, 2025

ryanofsky commented at 3:27 pm on October 6, 2025: contributor

Updated f39d3e1a2f2b9cdcf0e77942fd69eb92a58ce77f -> b0113afd44b4c7c0d0da9883424bd2978de3d18c (pr/winstream.1 -> pr/winstream.2, compare) to fix CI failures caused by missing std_path replacement

ryanofsky commented at 3:43 pm on October 6, 2025: contributor

re: #33550#pullrequestreview-3305372031

Maybe consider removing these lines now:

Nice suggestion. Attempted this in new commit.

Added 1 commits b0113afd44b4c7c0d0da9883424bd2978de3d18c -> d405c76d2e32e957a72c042f1fb82fd0cfcc8e6d (pr/winstream.2 -> pr/winstream.3, compare) simplifying fs::path as suggested

hebasto commented at 3:44 pm on October 6, 2025: member

re: #33550 (review)

Maybe consider removing these lines now:

Nice suggestion. Attempted this in new commit.

Added 1 commits b0113af -> d405c76 (pr/winstream.2 -> pr/winstream.3, compare) simplifying fs::path as suggested

Thanks for taking this.

I assume we still need fs::path::filename() though.

hebasto commented at 4:01 pm on October 6, 2025: member

Tested b0113afd44b4c7c0d0da9883424bd2978de3d18c by cross-compiling for x86_64-w64-mingw32 and aarch64-w64-mingw32 targets using the LLVM MinGW toolchain.

in src/util/fs.h:71 in d405c76d2e outdated

74@@ -65,11 +75,6 @@ class path : public std::filesystem::path
75         const std::u8string& utf8_str{std::filesystem::path::u8string()};
76         return std::string{utf8_str.begin(), utf8_str.end()};
77     }
78-
79-    // Required for path overloads in <fstream>.
80-    // See https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=96e0367ead5d8dcac3bec2865582e76e2fbab190
81-    path& make_preferred() { std::filesystem::path::make_preferred(); return *this; }

hebasto commented at 4:11 pm on October 6, 2025:

This might require a bit more adjustments:

 0--- a/src/common/settings.cpp
 1+++ b/src/common/settings.cpp
 2@@ -78,7 +78,7 @@ bool ReadSettings(const fs::path& path, std::map<std::string, SettingsValue>& va
 3     if (!fs::exists(path)) return true;
 4 
 5     std::ifstream file;
 6-    file.open(path);
 7+    file.open(path.std_path());
 8     if (!file.is_open()) {
 9       errors.emplace_back(strprintf("%s. Please check permissions.", fs::PathToString(path)));
10       return false;
11@@ -133,7 +133,7 @@ bool WriteSettings(const fs::path& path,
12         out.pushKVEnd(value.first, value.second);
13     }
14     std::ofstream file;
15-    file.open(path);
16+    file.open(path.std_path());
17     if (file.fail()) {
18         errors.emplace_back(strprintf("Error: Unable to open settings file %s for writing", fs::PathToString(path)));
19         return false;
20--- a/src/rpc/request.cpp
21+++ b/src/rpc/request.cpp
22@@ -114,7 +114,7 @@ GenerateAuthCookieResult GenerateAuthCookie(const std::optional<fs::perms>& cook
23     if (filepath_tmp.empty()) {
24         return GenerateAuthCookieResult::DISABLED; // -norpccookiefile
25     }
26-    file.open(filepath_tmp);
27+    file.open(filepath_tmp.std_path());
28     if (!file.is_open()) {
29         LogWarning("Unable to open cookie authentication file %s for writing", fs::PathToString(filepath_tmp));
30         return GenerateAuthCookieResult::ERR;
31@@ -153,7 +153,7 @@ bool GetAuthCookie(std::string *cookie_out)
32     if (filepath.empty()) {
33         return true; // -norpccookiefile
34     }
35-    file.open(filepath);
36+    file.open(filepath.std_path());
37     if (!file.is_open())
38         return false;
39     std::getline(file, cookie);
40--- a/src/test/settings_tests.cpp
41+++ b/src/test/settings_tests.cpp
42@@ -45,7 +45,7 @@ inline std::ostream& operator<<(std::ostream& os, const std::pair<std::string, c
43 inline void WriteText(const fs::path& path, const std::string& text)
44 {
45     std::ofstream file;
46-    file.open(path);
47+    file.open(path.std_path());
48     file << text;
49 }
50 
51--- a/src/wallet/dump.cpp
52+++ b/src/wallet/dump.cpp
53@@ -37,7 +37,7 @@ bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& erro
54         return false;
55     }
56     std::ofstream dump_file;
57-    dump_file.open(path);
58+    dump_file.open(path.std_path());
59     if (dump_file.fail()) {
60         error = strprintf(_("Unable to open %s for writing"), fs::PathToString(path));
61         return false;

ryanofsky commented at 4:13 pm on October 6, 2025: contributor

I assume we still need fs::path::filename() though.

I think we could overload it but it seems a little consistent to do that without a clear reason and without also overloading parent_path. For now just dropped some code in bitcoin.cpp which was using it.

Simplify fs::path by dropping filename() and make_preferred() overloads

These overloads were needed to allow passing `fs::path` objects directly to
libstdc++'s `fstream` constructors, but after the previous commit, there is no
longer any remaining code that does pass `fs::path` objects to `fstream`
constructors. Writing new code which does this is also discouraged because the
standard has been updated in https://wg21.link/lwg3430 to disallow it.

Dropping these also means its no longer possible to pass `fs::path` arguments
directly to `fstream::open` in libstdc++, which is somewhat unfortunate but not
a big loss because it is already not possible to pass them to the constructor.
So this commit updates `fstream::open` calls.

Additionally, this change required updates to src/bitcoin.cpp since it was
relying on the overloaded filename() method.

c864a4c194

ryanofsky force-pushed on Oct 6, 2025

ryanofsky commented at 4:15 pm on October 6, 2025: contributor

Updated d405c76d2e32e957a72c042f1fb82fd0cfcc8e6d -> c864a4c1940d682f7eb6fdb3b91b18d638b59330 (pr/winstream.3 -> pr/winstream.4, compare) with CI fixes for second commit

hebasto approved

hebasto commented at 4:31 pm on October 6, 2025: member

ACK c864a4c1940d682f7eb6fdb3b91b18d638b59330.

DrahtBot requested review from maflcko on Oct 6, 2025

hebasto commented at 8:58 pm on October 6, 2025: member

… even though there is not currently a CI job testing Windows libc++ builds.

Added to the nightly builds in https://github.com/hebasto/bitcoin-core-nightly/pull/74.

fanquake commented at 3:49 pm on October 7, 2025: member

Added to the nightly builds in https://github.com/hebasto/bitcoin-core-nightly/pull/74.

Looks like there are failing unit tests (https://github.com/hebasto/bitcoin-core-nightly/actions/runs/18313967330/job/52151177201#step:6:401):

0test\util_string_tests.cpp(43): Entering test case "ConstevalFormatString_NumSpec"
1test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" raised as expected: validation on the raised exception through predicate "HasReason{error}"
2test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" raised as expected: validation on the raised exception through predicate "HasReason{error}"
3...

hebasto commented at 3:51 pm on October 7, 2025: member

Added to the nightly builds in https://github.com/hebasto/bitcoin-core-nightly/pull/74.

Looks like there are failing unit tests (https://github.com/hebasto/bitcoin-core-nightly/actions/runs/18313967330/job/52151177201#step:6:401):

0
1test\util_string_tests.cpp(43): Entering test case "ConstevalFormatString_NumSpec"
2
3test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" raised as expected: validation on the raised exception through predicate "HasReason{error}"
4
5test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" raised as expected: validation on the raised exception through predicate "HasReason{error}"
6
7...

Correct. I’m looking into them.

However, I believe those failures are not related to this PR changes.

hebasto commented at 7:33 pm on October 7, 2025: member

Added to the nightly builds in hebasto/bitcoin-core-nightly#74.

Looks like there are failing unit tests (https://github.com/hebasto/bitcoin-core-nightly/actions/runs/18313967330/job/52151177201#step:6:401):

0test\util_string_tests.cpp(43): Entering test case "ConstevalFormatString_NumSpec"
1test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" raised as expected: validation on the raised exception through predicate "HasReason{error}"
2test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" raised as expected: validation on the raised exception through predicate "HasReason{error}"
3...

The issue appears to be caused by this commit.

Friendly ping @l0rinc @hodlinator as co-authors of that commit :)

l0rinc commented at 10:58 pm on October 7, 2025: contributor

I’m on it, it seems related to the discussion in the original PR which already had some Windows build problems: #30546 (review)

I have reproduced the error in https://github.com/l0rinc/bitcoin-core-nightly/pull/2 with @hebasto’s nightly clone redirected to l0rinc/master.

I pushed a potential fix that adds a const char * overload to HasReason in https://github.com/l0rinc/bitcoin/pull/43 - tested through https://github.com/l0rinc/bitcoin-core-nightly/pull/1.

Also added an alternative which replaces all remaining const char * throws with std::runtime_error in https://github.com/l0rinc/bitcoin/pull/44 - tested through https://github.com/l0rinc/bitcoin-core-nightly/pull/3.

Based on your feedback and on what the CI gods think, I will push one of them as a PR.

Also note that the windows build indicates additional warnings (unused vars because of ifndef __USING_WINDOWS__)

 0/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/randomenv.cpp:61:15: warning: '__p__environ' redeclared without 'dllimport' attribute: previous 'dllimport' ignored [-Winconsistent-dllimport]
 1   61 | extern char** environ; // NOLINT(readability-redundant-declaration): Necessary on some platforms
 2      |               ^
 3/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/llvm_mingw_toolchain/x86_64-w64-mingw32/include/stdlib.h:656:17: note: expanded from macro 'environ'
 4  656 | #define environ _environ
 5      |                 ^
 6/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/llvm_mingw_toolchain/x86_64-w64-mingw32/include/stdlib.h:225:21: note: expanded from macro '_environ'
 7  225 | #define _environ (* __p__environ())
 8      |                     ^
 9/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/llvm_mingw_toolchain/x86_64-w64-mingw32/include/stdlib.h:221:27: note: previous declaration is here
10  221 |   _CRTIMP char ***__cdecl __p__environ(void);
11      |                           ^
12/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/llvm_mingw_toolchain/x86_64-w64-mingw32/include/stdlib.h:221:3: note: previous attribute is here
13  221 |   _CRTIMP char ***__cdecl __p__environ(void);
14      |   ^
15/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/llvm_mingw_toolchain/x86_64-w64-mingw32/include/_mingw.h:52:40: note: expanded from macro '_CRTIMP'
16   52 | #      define _CRTIMP  __attribute__ ((__dllimport__))
17      |

and

0In file included from /home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/util/exec.cpp:8:
1/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/util/subprocess.h:759:10: warning: private field 'parent_' is not used [-Wunused-private-field]
2  759 |   Popen* parent_ = nullptr;
3      |          ^
4/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/util/subprocess.h:760:7: warning: private field 'err_wr_pipe_' is not used [-Wunused-private-field]
5  760 |   int err_wr_pipe_ = -1;
6      |       ^
7/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/util/subprocess.h:1038:7: warning: private field 'child_pid_' is not used [-Wunused-private-field]
8 1038 |   int child_pid_ = -1;
9      |       ^

We should likely fix these as well - I don’t mind pushing a separate PR for this after the other one.

hebasto commented at 11:25 pm on October 7, 2025: member

I’ve submitted an issue to https://github.com/mstorsjo/llvm-mingw. @l0rinc

I have reproduced the error in l0rinc/bitcoin-core-nightly#2 with @hebasto’s nightly clone redirected to l0rinc/master.

I pushed a potential fix that adds a const char * overload to HasReason in l0rinc#43 - tested through l0rinc/bitcoin-core-nightly#1.

Also added an alternative which replaces all remaining const char * throws with std::runtime_error in l0rinc#44 - tested through l0rinc/bitcoin-core-nightly#3.

Based on your feedback and on what the CI gods think, I will push one of them as a PR.

I believe that this is also a good point. Because of array-to-pointer conversion, I’d lean to the second option you mentioned.

Also note that the windows build indicates additional warnings (unused vars because of ifndef __USING_WINDOWS__)

We should likely fix these as well - I don’t mind pushing a separate PR for this after the other one.

Yes. These warnings are unrelated to the current PR changes, and I’d prefer to see them fixed in separated PR(s).

l0rinc commented at 0:56 am on October 8, 2025: contributor

I’d lean to the second option you mentioned

Thanks, closed the other ones and opened #33569 - the dedicated nightly build passed: https://github.com/l0rinc/bitcoin-core-nightly/pull/3

I’d prefer to see them fixed in separated PR(s)

Pushed #33570 as well for extern char** environ - but the one in subprocess.h seems more involved, will leave that to someone else.

l0rinc approved

l0rinc commented at 4:20 pm on October 20, 2025: contributor

Code review ACK c864a4c1940d682f7eb6fdb3b91b18d638b59330

It bothers me a bit that the first commit doesn’t compile, but it seems fixed in the second one - I’m fine with merging as long as the second commit is part of the PR.

I have tried reproducing the error of removing make_preferred and filename on master, but I’m not actually getting any failure - but it doesn’t look dangerous, I’m personally fine with removing those, they don’t seem to be necessary anymore.

 0root@i9-ssd /mnt/my_storage/bitcoin # cat /usr/include/c++/15/fstream | grep -C2 make_preferred
 1  // Enable if _Path is a filesystem::path or experimental::filesystem::path
 2  template<typename _Path, typename _Result = _Path, typename _Path2
 3           = decltype(std::declval<_Path&>().make_preferred().filename())>
 4    using _If_fs_path = enable_if_t<is_same_v<_Path, _Path2>, _Result>;
 5#endif // C++17
 6root@i9-ssd /mnt/my_storage/bitcoin # git log -1
 7commit 0eeae4d174a41c3fc2eae41e76b929fa3114d6f3 (HEAD, origin/master, origin/HEAD)
 8Merge: d30f149360 879c21045e
 9Author: merge-script <fanquake@gmail.com>
10Date:   Sun Oct 19 15:45:47 2025 +0100
11
12    Merge bitcoin/bitcoin#33625: Update secp256k1 subtree to latest master
13
14    3cbf7cb3e6ac51492b354732bddbb4f58ce97ed3 Squashed 'src/secp256k1/' changes from b9313c6e1a..d543c0d917 (fanquake)
15
16    Pull request description:
17
18      Updates the subtree to https://github.com/bitcoin-core/secp256k1/commit/d543c0d917a76a201578948701cc30ef336e0fe6
19      Related to [#33284](/bitcoin-bitcoin/33284/).
20
21    ACKs for top commit:
22      hebasto:
23        ACK 879c21045eba64b3dc875f6f2c2c579abecde1d0.
24      janb84:
25        ACK 879c21045eba64b3dc875f6f2c2c579abecde1d0
26
27    Tree-SHA512: 1802cd84959b5c935170792f458651f30431fe8340ead7966ff381c1c0c3a9f6c21bbb8dd96a07482ffed49642ded49e80b61802e688b8351956b111dffd5a78
28root@i9-ssd /mnt/my_storage/bitcoin # git diff
29diff --git a/src/util/fs.h b/src/util/fs.h
30index 7c313e2072..24f4943048 100644
31--- a/src/util/fs.h
32+++ b/src/util/fs.h
33@@ -65,11 +65,6 @@ public:
34         const std::u8string& utf8_str{std::filesystem::path::u8string()};
35         return std::string{utf8_str.begin(), utf8_str.end()};
36     }
37-
38-    // Required for path overloads in <fstream>.
39-    // See https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=96e0367ead5d8dcac3bec2865582e76e2fbab190
40-    path& make_preferred() { std::filesystem::path::make_preferred(); return *this; }
41-    path filename() const { return std::filesystem::path::filename(); }
42 };
43
44 static inline path u8path(const std::string& utf8_str)

I have also tried reverting std::ofstream fout{file}; and the deleted method does indeed catch it, so the implementation seems correct regardless of whether it fixes the issue - which I have reproduced with @hebasto’s nightly branch anyway.

I have also tested why the auto was needed in wrapper_dir - it seems to be related to mixing paths for the / - seems fine to me.

ryanofsky commented at 6:29 am on October 21, 2025: contributor

Thanks for the review!

It bothers me a bit that the first commit doesn’t compile

It should compile. Github just seems to show a red x next to the commit because the job was cancelled when the second commit was pushed.

I have tried reproducing the error of removing make_preferred and filename on master, but I’m not actually getting any failure

I think the failure only happened on windows. The workaround was introduced here: #20744 (comment)

in src/bitcoin.cpp:212 in c864a4c194

208@@ -209,7 +209,7 @@ static void ExecCommand(const std::vector<const char*>& args, std::string_view w
209 
210     // Try to resolve any symlinks and figure out the directory containing the wrapper executable.
211     std::error_code ec;
212-    fs::path wrapper_dir{fs::weakly_canonical(wrapper_path, ec)};
213+    auto wrapper_dir{fs::weakly_canonical(wrapper_path, ec)};

maflcko commented at 8:04 am on October 21, 2025:

nit in c864a4c1940d682f7eb6fdb3b91b18d638b59330: I understand that you probably used the std namespace here and then used auto to silence the linter? However, this seems to go in the wrong direction. If we think our fs:: namespace should be preferred over the std:: fs one, we should try to use it as much as possible and only call std_path when needed. I guess restoring fs::path here would require some overloads in the wrapper class, or restoring some fs::path constructors below. But this is just a nit or follow-up idea, and maybe this was intentional and I am missing something.

ryanofsky commented at 7:58 am on October 22, 2025:

It was intentional just to minimize verbosity of the code and size of the diff, but it’s true this goes in direction of using std::filesystem::path instead of fs::path, and maybe we don’t want that.

To me, it seems good to prefer using fs::path instead of std::filesystem::path, but using std::filesystem::path is not always avoidable anyway, and it is at least safe to use as long as not doing conversions to or from arbitrary char strings, so it’s ok to tolerate in some places

maflcko commented at 8:04 am on October 21, 2025: member

re-ACK c864a4c1940d682f7eb6fdb3b91b18d638b59330 🌥

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK c864a4c1940d682f7eb6fdb3b91b18d638b59330 🌥
30D0v6O8aoNqJXCgjkkFNkvQkFzEvahdV0ADufk2lsrbmdkWx9WCf6o0r/LORbJuEwFehUzATQ8meSoax1jCVBw==

hebasto merged this on Oct 22, 2025

hebasto closed this on Oct 22, 2025

Fix windows libc++ fs::path fstream compile errors #33550

Code Coverage & Benchmarks

Reviews

Conflicts

Fix windows libc++ `fs::path` `fstream` compile errors #33550